Skip to content

Add product and tech specs for user-configurable LSP servers (gh-8803)#11944

Open
bradleyjames wants to merge 11 commits into
warpdotdev:masterfrom
bradleyjames:brad/8803-spec
Open

Add product and tech specs for user-configurable LSP servers (gh-8803)#11944
bradleyjames wants to merge 11 commits into
warpdotdev:masterfrom
bradleyjames:brad/8803-spec

Conversation

@bradleyjames
Copy link
Copy Markdown
Contributor

@bradleyjames bradleyjames commented May 30, 2026

Description

Spec PR for issue #8803 — proposes a new [[editor.language_servers]] settings entry that lets users register custom LSP servers without modifying Warp's built-in language→server map.

Per CONTRIBUTING.md, this is the spec-only PR for design review. Implementation continues on the existing code PR (#10700, branch brad/8803-lsp-plugins) — this is the "rarer case" the contributing guide mentions where a large spec is broken out so implementation can be reviewed separately.

What's in this PR

  • specs/GH8803/product.md — user-visible behavior. Numbered testable invariants covering the field set (name, command, args, filetypes, env, initialization_options; required vs optional), placeholder substitution ({{workspace_root}}, {{workspace_slug}}, {{cache_dir}}, {{env_VAR}}), filetype matching (glob and literal basename), per-workspace enable persistence, custom-overrides-built-in semantics, and an Eclipse JDT-LS worked example demonstrating the field set is sufficient for non-trivial servers.

  • specs/GH8803/tech.md — implementation plan grounded in the current codebase. Modules touched (crates/lsp/src/descriptor/, app/src/settings/language_servers.rs, app/src/code/lsp_dispatch.rs, app/src/ai/persisted_workspace.rs, app/src/code/footer.rs), the SQLite migration that adds a kind column to workspace_language_server for persisting custom enable state, per-invariant verification mapping, and risks.

Linked Issue

Closes #8803

  • The linked issue is labeled ready-to-spec or ready-to-implement.
  • Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes).

Testing

Docs-only change.

  • I have manually tested my changes locally with ./script/run — N/A, docs-only.

Screenshots / Videos

N/A — docs-only change. Visual evidence of the implementation behavior is attached on the code PR.

Agent Mode

  • Warp Agent Mode - This PR was created via Warp's AI Agent Mode

…devgh-8803)

Spec PR for issue warpdotdev#8803 — defines a new `[[editor.language_servers]]`
settings entry that lets users register custom LSP servers without
modifying Warp's built-in language→server map.

Specs:
- PRODUCT.md — user-visible behavior. Required vs. optional fields,
  placeholder substitution (`{{workspace_root}}`, `{{workspace_slug}}`,
  `{{cache_dir}}`, `{{env_VAR}}`), filetype matching, per-workspace
  enable persistence, override-of-built-in semantics, and an Eclipse
  JDT-LS worked example demonstrating the field set is sufficient for
  non-trivial servers.
- TECH.md — implementation plan grounded in the current codebase:
  descriptor parsing/validation, settings registration via
  `define_settings_group!`, custom-first dispatch, SQLite persistence
  schema with a `kind` column on `workspace_language_server`, and
  per-invariant verification mapping.

Implementation continues on the existing code PR
(brad/8803-lsp-plugins, warpdotdev#10700).

Closes warpdotdev#8803
@cla-bot cla-bot Bot added the cla-signed label May 30, 2026
@github-actions github-actions Bot added the external-contributor Indicates that a PR has been opened by someone outside the Warp team. label May 30, 2026
@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 30, 2026

@bradleyjames

I'm starting a first review of this spec-only pull request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

The product and technical specs cover the intended custom LSP configuration surface and a phased implementation plan, but several sections contradict each other on behavior that implementers need to treat as source-of-truth requirements. The most serious gap is custom workspace-root selection: the product spec says the open Warp directory is the root, while the tech spec plans root-marker walking and tests Gemfile-based roots without defining any root marker field.

Concerns

  • Root resolution/root-marker behavior is undefined and internally inconsistent.
  • Placeholder escaping, validation error handling, settings hot-reload subscriptions, and schema-registration guidance conflict across sections.
  • Security: launch logging needs explicit redaction rules before fully substituted command arguments, environment values, or initialization options can contain secrets.

Security

  • The manual validation plan asks reviewers to confirm fully substituted launch arguments in logs, but the feature explicitly supports env placeholders and hard-coded env values that may carry tokens. The spec needs a redaction policy for command, args, env, and initialization options before implementation.

Verdict

Found: 1 critical, 5 important, 1 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/product.md Outdated
Resolves 7 findings from Oz's review of the user-configurable LSP
servers spec (warpdotdev#11944):

- Remove stale root-marker walk text; cross-reference the existing
  DetectedRepositories workspace-root chain instead
- Align the three manager-subscription mentions to the canonical
  "no subscription, on-demand reads, no auto-restart" rule per
  invariant 19
- Replace stale hand-written SettingSchemaEntry references with the
  actual macro-generated registration; restate the residual risks
  in terms of the conventions that remain unenforced
- Drop the `{{{name}}}` triple-brace escape (conflicts with Handlebars
  semantics); document the resulting gap explicitly
- Align tech.md validation description with invariant 23's
  all-or-nothing rejection rule
- Add invariant 32 committing to log redaction via CustomSecretRegex;
  replace "logs show fully-substituted args" manual validation step
  with a redaction-safe version; add tech.md guidance on what is safe
  to log
- Define explicit `name` field constraints (1-64 chars from
  [A-Za-z0-9._-], no `.`/`..`, no leading `.` or `-`) in invariant 1
  and wire into invariant 23's settings-error list
@bradleyjames
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 30, 2026

@bradleyjames

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR adds product and technical specs for user-configurable LSP server descriptors in Warp settings. The specs cover the intended settings shape, matching, placeholder expansion, footer behavior, persistence, runtime plumbing, and validation strategy.

Concerns

  • The specs introduce a strategic freeze of all future built-in language support, which is broader than the linked custom-LSP configuration request and should be either confirmed as intentional scope or removed from this feature spec.
  • The product and tech specs disagree on settings-error surfacing, especially whether invalid entries create synthesized per-entry banner keys or only the editor.language_servers key with details in logs.
  • The no-migration persistence plan can collide with built-in server enablement rows because custom descriptor names are not reserved from existing serialized built-in keys.
  • The proposed redaction helper places app-layer privacy logic inside the lower-level lsp crate path, which is a security-relevant feasibility gap for the log-redaction guarantee.
  • The placeholder slug uniqueness guarantee and the settings parser pseudo-code need tightening so implementation and tests do not encode behavior that contradicts the product invariants.

Security

  • The log-redaction design depends on crates/lsp applying app/src/settings/privacy.rs::CustomSecretRegex; that dependency direction is not currently available, so the spec needs an implementable ownership boundary before relying on it for secrets in commands, args, env values, or initialization options.

Verdict

Found: 0 critical, 6 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Resolves 6 findings from Oz's re-review of warpdotdev#11944:

- Move the redaction primitive out of crates/lsp (which cannot depend
  on app/) to a trait injected from app at the LSP boundary; describe
  the injection point and verification flag for Phase 4
- Reserve the five built-in LSPServerType variant names from custom
  `name` in the validator and invariant 23 settings-error list; align
  the persistence section with the actual schema (kind column added
  by migration 2026-05-24-180000), removing the stale "zero schema
  changes" claim
- Align tech.md banner contract with invariant 23: SettingsFileError
  carries one bare `editor.language_servers` key; per-entry detail
  goes to log::warn! and stays out of the banner
- Replace the SettingsValue from_file_value Rust example (which had
  a wrong-type silent-accept bug) with prose describing the all-or-
  nothing behavior; tech docs use pseudo-code/prose, not real code
- Soften the workspace_slug uniqueness invariant to match the
  truncated-hash implementation honestly
- Remove the built-in-freeze product strategy claim from both product.md
  and tech.md (scope beyond warpdotdevGH-8803); preserve the design choice of
  keeping built-in/custom runtime tracks separate as its own merit
@bradleyjames
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 31, 2026

@bradleyjames

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This spec PR defines product and technical plans for user-configurable LSP servers via [[editor.language_servers]]. The overall scope is clear, but several details need tightening before implementation: command path trust boundaries, server-name identity/caching behavior, unknown-field handling, and schema generation for the documented filetype forms.

Concerns

  • The command contract allows only absolute paths or bare PATH names, but the implementation plan does not say how relative paths with separators are rejected or guarded.
  • Built-in name reservation and examples conflict around visible built-in names, and cache-directory uniqueness is underspecified on case-insensitive filesystems.
  • The tech spec relies on serde/schema defaults in places where the product spec requires custom behavior: logging unknown fields and accepting both bare-string and inline-table filetypes entries.
  • Phase 5 contradicts Phase 2b by reintroducing the blanket Vec<LspServerDescriptor> settings path after the spec already explains why a custom newtype is necessary.

Security

  • Custom server commands execute with the workspace root as cwd. Without explicit validation or a user-facing guard for relative paths, a descriptor such as ./server or bin/server can execute repository-controlled code when a matching file is opened and enabled.

Verdict

Found: 0 critical, 6 important, 1 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Resolves 7 findings from Oz's third review of warpdotdev#11944, with deeper
investigation against the actual implementation on the code branch
to ensure correctness:

- Drop the bare-string `filetypes` form from invariant 1 entirely:
  the implementation only accepts inline tables, all five worked
  examples already use inline tables, and supporting both forms
  would need custom Deserialize + custom JsonSchema for a small
  ergonomic win
- Broaden the reserved-name list to cover both serialized variant
  names (RustAnalyzer, ...) AND binary display names (rust-analyzer,
  ...), case-insensitively. Reservation sourced from LSPServerType
  via EnumIter so it tracks the enum. Rename the five override
  examples to `my-<binary>` since the display names are now reserved
- Add SECURITY rule for `command`: after `~`/`~/` expansion, value
  must be absolute or a bare name with no `/` or `\` separators;
  relative paths like `./server` or `bin/server` are rejected to
  prevent a malicious settings.toml from executing repo-controlled
  code via the workspace cwd
- Specify case-insensitive `name` uniqueness in invariant 2 to
  prevent macOS/Windows cache-dir collisions between `ruby-lsp`
  and `Ruby-LSP`
- Specify the unknown-field logging mechanism for invariant 24: a
  manual top-level-key walk before `serde_json::from_value`, with
  the known-field set sourced from `RawDescriptor`'s fields. Serde
  silently drops unknowns; this is what makes invariant 24's "logged
  with a warning" promise real without adding `serde_ignored`
- Rewrite Phase 5's `SettingsValue` description to route through
  the `LspServerDescriptors` newtype from Phase 2b (the blanket
  `Vec<...>` path it previously described would skip matcher
  compilation)
- Replace the last stale "no persistence migration" claim with text
  reflecting the actual additive `kind` column migration
@bradleyjames
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 31, 2026

@bradleyjames

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This spec PR defines a user-configurable LSP server feature with product behavior and a phased technical plan. The overall direction addresses the linked issue, but several spec contradictions and design gaps should be resolved before implementation proceeds.

Concerns

  • The command-resolution trust boundary is incomplete for bare commands resolved through PATH when the child process cwd is the workspace root.
  • The product spec contradicts itself on whether initialization_options is passed verbatim or receives placeholder substitution.
  • The filetypes schema alternates between inline-table-only and string-or-inline-table forms.
  • The tech spec describes the future kind persistence column as current architecture, which does not match the current schema and could cause implementation drift.
  • The logging section classifies workspace/cache paths as non-sensitive without a privacy-safe logging policy.

Security

  • Bare-command PATH lookup can still resolve to repo-controlled binaries if PATH contains . or relative/empty entries while the launch cwd is the workspace root.
  • Workspace roots and cache directories can contain usernames, customer names, or private repository names and should not be declared safe for verbatim logging without redaction or safe/full log handling.

Verdict

Found: 0 critical, 5 important, 1 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Resolves 6 findings from Oz's fourth review of warpdotdev#11944. Investigation
went through the actual implementation on the code branch and
existing process-spawn sites in the codebase (MCP servers, built-in
LSPs, CommandBuilder) to ground decisions in current Warp behavior.

- Document the PATH-based residual risk for `command` resolution
  (invariant 16) honestly: bare-name lookup uses the OS process
  loader, which on Unix execvp and Windows cmd.exe walks the user's
  $PATH against the spawned cwd (workspace root) — same trust model
  as built-in LSPs use today via CommandBuilder. Spec chooses
  consistency with that precedent over a custom-LSP-only mitigation;
  PATH sanitization in CommandBuilder is tracked as a follow-up that
  would benefit all LSPs uniformly
- Drop the leftover "string or inline-table" mention from invariant
  23's settings-error list (consistent with the bare-string drop
  from the previous round)
- Fix the invariant 26 "docs link from invariant 9" cross-reference:
  invariant 9 doesn't introduce a docs link or any new footer
  affordance. Install instructions live on the external docs page
  (per invariant 25's external-tooling JSON Schema)
- Align invariant 17 with invariant 5 and the actual expand_json
  implementation: `initialization_options` is sent *after*
  placeholder substitution on string leaves, not verbatim. Same fix
  on the JDTLS worked-example prose and the field definition
- Refine invariant 32's redaction classification: `workspace_root`
  and `cache_dir` are logged verbatim (matching today's built-in LSP
  log behavior) but explicitly called out as PII-bearing, since
  CustomSecretRegex is a token-pattern matcher, not a PII filter
- Rewrite the "Current architecture" persistence bullet to describe
  master state (no kind column); the kind column is described as
  Phase 4 work in the existing Phase 4 persistence section
@bradleyjames
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 31, 2026

This is your last /oz-review for the current 24-hour window. Your next slot opens in ~19h 32m.

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 31, 2026

@bradleyjames

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR adds product and technical specs for user-configurable LSP server descriptors in settings.toml, including matching, placeholder substitution, per-workspace enablement, persistence, schema generation, and logging/redaction behavior.

Concerns

  • The security model rejects relative command paths before substitution, but does not require the same absolute-or-bare-name validation after {{env_VAR}} and other placeholders expand.
  • The persistence plan introduces kind = 'Custom' rows but does not define the typed event/API shape that carries a custom server name through the existing persistence writer, whose current event only accepts LSPServerType.
  • The product and tech specs disagree on whether the existing unsupported-language footer gains a docs-page affordance.

Security

  • Resolve the post-substitution command validation gap so placeholder-expanded command values cannot bypass the relative-path rejection and execute workspace-controlled paths.

Verdict

Found: 0 critical, 3 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread specs/gh-8803/product.md
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Resolves 3 findings from Oz's fifth review of warpdotdev#11944:

- Document the trust-boundary decision around post-substitution
  `command`: the literal-form absolute-or-bare-name rule is NOT
  re-applied after placeholder substitution. Environment variables
  are user-controlled, the same as the literal text of settings.toml.
  Matches Warp's existing treatment of user-controlled inputs (MCP,
  terminal, built-in LSP PATH). Documented in invariant 16 and the
  tech.md validate.rs description so future readers see the
  decision and its reasoning
- Describe the persistence event/API shape for custom servers.
  Implementation uses two ModelEvent variants
  (UpsertWorkspaceLanguageServer for built-ins,
  UpsertWorkspaceCustomLanguageServer for customs) with their own
  dispatch arms, rather than a single ServerKey-typed event. Each
  upsert function sets its own kind column literal; no runtime kind
  discriminator on the event because the variant choice at the call
  site already encodes it
- Fix the second "docs link from invariant 9" contradiction (in
  tech.md). The external docs page exists for the feature (per
  invariants 25 and 26) but is reached through Warp's broader docs
  surface, not through any new footer affordance — invariant 9's
  no-new-affordance rule stays intact
@bradleyjames
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 31, 2026

You've used all 5 /oz-review slots for the current 24-hour window. Your next slot opens in ~18h 57m.

@bradleyjames
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented Jun 4, 2026

@bradleyjames

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR adds product and technical specs for user-configurable custom LSP servers, covering settings shape, matching semantics, launch behavior, persistence, schema generation, testing, risks, and security-sensitive logging. The specs are generally thorough and grounded in the current LSP/settings architecture.

Concerns

  • The testing matrix contradicts the stated all-or-nothing settings-error behavior by saying each error class produces a notification line, while the product and implementation sections say the UI surfaces only the single editor.language_servers invalid setting and writes per-entry details to logs.
  • The custom-server enable flow should explicitly document the trust boundary for executing a user-configured command from the unchanged footer CTA.
  • The command validator should clarify whether Windows UNC/root-relative absolute paths are supported or intentionally rejected.

Verdict

Found: 0 critical, 1 important, 2 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Resolves 3 findings from Oz's sixth review of warpdotdev#11944:

- Fix the test matrix row for invariant 23: the integration test must
  assert one banner line for the bare `editor.language_servers` key
  plus per-error-class detail on `log::warn!` output, NOT a separate
  notification line per error class (the old wording contradicted
  invariant 23 and Phase 2c)
- Make the Enable-CTA trust boundary explicit alongside the footer
  Enable button handler description: the user's settings.toml is the
  trust boundary, the CTA is unchanged from built-in flow and is not
  a per-launch authorization prompt. Pre-launch verification surface
  is a v1.1+ follow-up per invariant 31
- Clarify the cross-platform "absolute path" definition for `command`
  in both product.md invariant 1 and the tech.md validator
  description: Unix `/`-rooted, Windows drive-letter (`C:\` / `C:/`)
  or UNC (`\\server\share`, `//server/share`). Windows root-relative
  (`\path`, `/path` without a drive) is rejected — it depends on the
  current drive at exec time, which is the cwd-dependent resolution
  the rule exists to prevent
@bradleyjames
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented Jun 5, 2026

@bradleyjames

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This specs-only PR proposes user-configurable LSP servers via [[editor.language_servers]], including product invariants, implementation phases, persistence, placeholder substitution, and validation coverage.

Concerns

  • The name validation rules are not sufficient for a value that is also used as a filesystem path component on all supported platforms.
  • The globset::GlobMatcher serde sketch needs an explicit default/placeholder strategy or a different deserialization shape to be compile-feasible.
  • The invalid-settings behavior should distinguish between blocking future custom-server launches and leaving already-running servers untouched.

Security

  • The tech-spec redaction plan does not cover all values that product invariant 32 says must be redacted, especially substituted command values and raw descriptor values emitted during validation logging.

Verdict

Found: 0 critical, 3 important, 1 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/product.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Comment thread specs/GH8803/tech.md Outdated
Resolves 4 findings from Oz's seventh review of warpdotdev#11944:

- Z (Windows-reserved names + trailing dots): change `lsp_server_cache_dir`
  to hash `server_name` into the path component (same SHA-256-prefix
  shape as `workspace_hash`) instead of using the literal name. This
  decouples the cache-dir filesystem segment from `name`'s allowed
  character set and eliminates the whole class of platform-specific
  path-segment edge cases (Windows reserved device names, trailing-dot
  stripping, extreme length) without a per-platform validator. Updated
  invariant 1's `name` description, invariant 2's case-insensitive
  uniqueness rationale, invariant 5's `{{cache_dir}}` description, the
  Phase 4 persistence section's name-reservation rationale, and the
  Phase 1 `lsp_server_cache_dir` description
- AA (invariant 23 wording): "no custom servers run" → "no NEW custom
  servers are launched until the file is fixed (already-running
  servers keep running per invariant 19)"; eliminates the apparent
  conflict with invariant 19's no-restart rule
- BB (redaction completeness): add substituted `command` to the ⚠️
  list in the launch-time redaction policy (was missing); add a new
  parse/validate-time logging policy describing how `log::warn!` from
  `parse.rs`/`validate.rs` includes only the safe summary (constrained
  `name` + error variant name + structural detail), with raw user-typed
  values kept inside the error struct and never reaching the log line,
  since these calls run before the `LogRedactor` is injected from app/
- CC (Phase 5 struct example): replace the `LspServerDescriptor` and
  `LspFiletypePattern` Rust code blocks with prose describing the
  field shape and the serde/schemars decisions (including the
  `#[serde(skip, default = "placeholder_matcher")]` rationale). The
  previous code example was missing `#[serde(skip)]` and the `default`
  function reference, so it wouldn't have compiled — per the saved
  rule that tech.md uses pseudo-code, not real Rust
@bradleyjames
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented Jun 5, 2026

@bradleyjames

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This spec PR defines product and technical plans for user-configurable custom LSP servers, including settings schema, filetype matching, per-workspace enablement, runtime spawning, logging/redaction, and tests.

Concerns

  • The reserved built-in server names are ambiguous because the Pyright binary/display name in the current code is pyright-langserver, while the product spec repeatedly lists pyright.
  • The persistence section specifies writes for custom enablement state but does not spell out startup/read hydration for kind = 'Custom' rows, so cross-restart persistence could be missed despite being a product invariant.
  • The env placeholder and env value design leaves literal secrets in settings.toml as the only reliable option for users whose environment is populated by shell startup files; that should be resolved or explicitly removed from v1 expectations before implementation.

Security

  • The spec acknowledges that dynamic env values may require plaintext values in settings.toml; this is a security-relevant v1 design gap because LSP env vars commonly include API tokens.

Verdict

Found: 0 critical, 3 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread specs/GH8803/product.md Outdated
Comment thread specs/gh-8803/tech.md
Comment thread specs/gh-8803/tech.md
- DD: narrow the reserved-name list to the five binary display names
  only (rust-analyzer, gopls, pyright-langserver, typescript-language-
  server, clangd); drop the variant-name reservation. SQLite kind
  column already prevents on-disk collisions; cache dir is hashed.
  Fixes the pyright/pyright-langserver discrepancy by sourcing from
  LSPServerType::binary_name(). Normalizes "pyright" → "pyright-
  langserver" in colloquial server-list mentions
- EE: document the startup/read path in Phase 4 — loader
  get_all_workspace_language_servers_by_workspace reads the kind
  column and partitions rows into language_servers and
  custom_language_servers
- FF: drop "to inject secrets" framing from invariant 32; env-capture
  limitation already covered in the existing follow-up
@bradleyjames
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented Jun 5, 2026

@bradleyjames

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This spec PR proposes user-configurable [[editor.language_servers]] entries, covering product behavior, settings validation, placeholder substitution, custom-vs-built-in dispatch, persistence, schema generation, tests, and several security-sensitive trust boundaries.

Concerns

  • The tech plan does not account for the existing text-document open path deriving TextDocumentItem.language_id from the built-in LanguageId enum, which would break the primary custom-language flow for filetypes that Warp does not already know.

Verdict

Found: 0 critical, 1 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread specs/GH8803/tech.md Outdated
GG: extend the Phase 3 "identity-free runtime" paragraph to call out
TextDocumentService::did_open's LanguageId::from_path derivation —
the one remaining built-in-only assumption below the spawn line.
Phase 3 plumbing: did_open takes language_id: String from the caller
(matched descriptor for customs, LanguageId::from_path().lsp_language_identifier()
for built-ins). Verified via JDTLS log: did_open currently silently
fails for non-built-in filetypes (JDTLS hid the symptom via project
indexing; other servers won't).
@bradleyjames
Copy link
Copy Markdown
Contributor Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented Jun 5, 2026

This is your last /oz-review for the current 24-hour window. Your next slot opens in ~20h 57m.

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented Jun 5, 2026

@bradleyjames

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I reviewed this pull request and requested human review from: @bnavetta.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR adds product and technical specs for user-configurable LSP servers via [[editor.language_servers]], covering user-facing behavior, validation, placeholder substitution, runtime dispatch, persistence, schema generation, and phased test coverage.

Concerns

  • No blocking spec issues found. The specs cover the linked issue scope and document the main feasibility and security-sensitive tradeoffs, including user-supplied commands, PATH resolution, per-workspace enablement, logging/redaction, and follow-up hardening.

Verdict

Found: 0 critical, 0 important, 0 suggestions

Approve

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

@oz-for-oss oz-for-oss Bot requested a review from bnavetta June 5, 2026 02:49
…ec skill

CONTRIBUTING.md's example uses GH<num>, but it links to the
write-product-spec skill which specifies the canonical form is
gh-<num> (lowercase, hyphenated). All existing specs in the repo
use the older GH<num> form; this is the first to use the new
convention.

Also drop the stale "Linear ticket not yet created... replace with
specs/APP-XXXX/" placeholder from product.md — no other spec in the
repo carries that note, and the skill treats the GitHub-issue form
as a permanent valid id, not a temporary placeholder.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed external-contributor Indicates that a PR has been opened by someone outside the Warp team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for adding custom language servers

1 participant