spec: user-configurable language servers (#8803)#9848
Open
lonexreb wants to merge 3 commits intowarpdotdev:masterfrom
Open
spec: user-configurable language servers (#8803)#9848lonexreb wants to merge 3 commits intowarpdotdev:masterfrom
lonexreb wants to merge 3 commits intowarpdotdev:masterfrom
Conversation
Per @kevinyang372's direction on warpdotdev#9562 and warpdotdev#9568 (closing those PRs in favor of a user-configurable LSP path), this spec turns warpdotdev#8803's product direction into testable behavior invariants and a concrete implementation plan grounded in the existing LSP infrastructure. product.md - 15 numbered, testable invariants covering parse/validate, chip behavior (per-workspace enablement, dismissal, persistence across restart), spawn outcomes (success / not-on-PATH / crash / timeout), settings reload diff semantics, coexistence with built-in servers, and graceful shutdown. - TOML configuration shape with `name` / `command` / `file_types` / optional `root_files` / `initialization_options` / `start_timeout_ms`. - Explicit non-goals: auto-install, version pinning, replacing built-in servers, marketplace. - Open questions called out: `~` expansion in `command[0]`, example configs doc, schema-fetching restriction defaults. tech.md - File-by-file implementation plan against `crates/lsp/`, `app/src/settings/`, `app/src/workspace/state.rs`, `app/src/code/editor/`. - Reuses patterns proven in the closed PRs: * `binary_in_path` PATH search with executable-bit check (warpdotdev#9562) * `warpui::r#async::FutureExt::with_timeout` for bounded init (warpdotdev#9562 follow-up commit `31285c4`) * `std::env::join_paths` + `binary_filename` for cross-platform tests (warpdotdev#9562, warpdotdev#9568) * `initialization_options` plumbing for security-sensitive defaults (warpdotdev#9568, lessons from `handledSchemaProtocols`) - Per-invariant test plan mapping each product.md item to a unit / integration test layer. - ASCII flow diagram of settings-edit → chip-render → enable-click → spawn-with-timeout → route-traffic. - Risks called out (crash loops, security of `initialization_options`, `Custom(String)` enum propagation, workspace-state migration).
Contributor
|
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 Powered by Oz |
github-actions
Bot
added
the
external-contributor
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Overview
This spec PR defines the product behavior and implementation plan for user-configurable language servers.
Concerns
- The tech spec contradicts the product requirements by moving the settings UI needed for disable/re-enable behavior into follow-ups.
- The implementation plan has feasibility gaps around crate boundaries, process timeout cleanup, and deriving LSP language IDs from file extensions.
- The config model does not resolve conflicts when multiple custom servers match the same file type.
Security
- Existing enabled workspaces would automatically execute a changed command after settings reload; require a fresh opt-in or invalidate enablement for command/arg changes.
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
| ## Follow-ups (out of this spec) | ||
|
|
||
| - `nix flake check`-validated dev shell with all referenced LSP binaries pre-installed (would help testing). | ||
| - Settings UI: a "Custom language servers" page under Settings → Code intelligence that lists configured servers + workspace-enablement state with `Enable`/`Disable` buttons (currently described only in product.md user-experience section). |
Contributor
There was a problem hiding this comment.
🚨 [CRITICAL] Settings UI cannot be a follow-up because product.md requires disabling and re-enabling dismissed servers through settings (invariants 11 and 14); move this UI into Proposed changes/testing or drop those requirements from the product spec.
|
|
||
| ```rust | ||
| pub struct UserConfiguredLanguageServer { | ||
| config: UserLspServerConfig, |
Contributor
There was a problem hiding this comment.
UserLspServerConfig is defined in app/src/settings/lsp.rs, but this new crates/lsp module cannot depend on app code; specify a shared crate/type boundary or an app-to-lsp conversion layer.
|
|
||
| **Option B — keep the enum closed and add a parallel `LspLanguageId` type that's either `Builtin(LanguageId)` or `Custom { extension: String, identifier: String }`.** | ||
|
|
||
| **Recommendation: Option A.** It's mechanical (one variant + two match arms) and avoids splitting the type system. The downside (more `Custom(...)` arms in match exhaustiveness) is acceptable. |
Contributor
There was a problem hiding this comment.
languageId is not sufficient for many servers (sh → shellscript, phtml → php, etc.); add a configurable language_id/per-extension mapping or define how aliases are handled.
| 1. Compute diff of `[lsp.servers]` between old and new config (by `name`). | ||
| 2. For added entries: nothing immediate; chip will appear on next matching file open. | ||
| 3. For removed entries: shut down any running instance for that name, clear from `enabled_custom_servers` in all workspaces. | ||
| 4. For changed entries (config differs but `name` matches): if currently running, restart with new config. |
Contributor
There was a problem hiding this comment.
command changes executes a new user-supplied binary without a fresh opt-in; require re-confirmation or invalidate enablement for command/arg changes.
| - Wrap the `initialize` request in `warpui::r#async::FutureExt::with_timeout(Duration::from_millis(config.start_timeout_ms))`. Three outcomes (matches the pattern from `feat/9168-php-lsp-intelephense` commit `31285c4`): | ||
| - `Ok(Ok(_))` — server initialized; route LSP traffic. | ||
| - `Ok(Err(err))` — JSON-RPC error; surface notification with err message. | ||
| - `Err(timeout)` — kill child via `Drop`, surface timeout notification. |
Contributor
There was a problem hiding this comment.
std::process::Child; specify explicit kill/kill_on_drop plus wait/reap semantics for timeout cleanup.
|
|
||
| - **Auto-installation of user-configured servers.** The user installs the binary themselves (npm, cargo, system package manager, brew, etc.). Warp does not run package managers on the user's behalf for these. | ||
| - **Version pinning / auto-update of user-configured servers.** Out of scope; the user owns the lifecycle. | ||
| - **Per-file dynamic switching.** A single file is associated with at most one user-configured server, plus any built-in servers that already match. No "try server A, fall back to server B" runtime logic. |
Contributor
There was a problem hiding this comment.
file_types are rejected or how conflicts are resolved.
|
|
||
| **File:** new `app/src/settings/lsp.rs`. Pattern matches `app/src/settings/input.rs` (`define_settings_group!` macro). | ||
|
|
||
| The group holds a single setting, `custom_servers`, of type `Vec<UserLspServerConfig>`. The struct: |
Contributor
There was a problem hiding this comment.
💡 [SUGGESTION] The user-facing TOML is [[lsp.servers]], but the Rust setting is named custom_servers; call out the serde rename/schema mapping so implementers don't accidentally expose [[lsp.custom_servers]].
kevinyang372
reviewed
May 1, 2026
| - **Auto-installation of user-configured servers.** The user installs the binary themselves (npm, cargo, system package manager, brew, etc.). Warp does not run package managers on the user's behalf for these. | ||
| - **Version pinning / auto-update of user-configured servers.** Out of scope; the user owns the lifecycle. | ||
| - **Per-file dynamic switching.** A single file is associated with at most one user-configured server, plus any built-in servers that already match. No "try server A, fall back to server B" runtime logic. | ||
| - **Replacing built-in servers.** A user-configured server with the same `file_types` as a built-in does **not** disable the built-in. Both run; the LSP client merges their results. |
Member
There was a problem hiding this comment.
User-configured server should overwrite built-in servers. It doesn't make sense to have both servers running at the same time
|
|
||
| ### Disabling a server in a workspace | ||
|
|
||
| 1. User opens settings → "Code intelligence" → "Custom language servers". |
Member
There was a problem hiding this comment.
"Code intelligence" doesn't exist in our settings
To start, I also think it's probably fine to not have a dedicated custom language servers section in UI
|
|
||
| 1. User opens settings → "Code intelligence" → "Custom language servers". | ||
| 2. Each configured server lists which workspaces it is enabled for. | ||
| 3. User clicks the workspace row's `Disable` button. Warp shuts down that server's process for that workspace and removes the per-workspace enablement record. |
Member
There was a problem hiding this comment.
Please make sure we are following the existing settings > code > indexing and projects UI
|
|
||
| 1. **Binary not on PATH:** When a user enables a server whose `command[0]` is not on PATH, Warp shows: *"Could not start `<name>`: binary `<cmd>` not found on PATH."* The chip's `Enable` button is replaced with `Open settings`. | ||
| 2. **Binary on PATH but spawn fails:** Warp shows: *"`<name>` exited with status `<n>`. Last 200 bytes of stderr: `<...>`."* with `Open settings` and `Retry` buttons. | ||
| 3. **Spawn hangs:** A configurable `start_timeout` (default 5s) bounds the LSP `initialize` request. On timeout, Warp shows: *"`<name>` did not respond to `initialize` within 5s."* The server's process is killed. |
Member
There was a problem hiding this comment.
I don't think we need to do anything special for custom LSPs here
| command = ["intelephense", "--stdio"] | ||
| file_types = ["php", "phtml"] | ||
| root_files = ["composer.json", "composer.lock"] # optional; default: file's parent dir | ||
| initialization_options = { storagePath = "/tmp/intelephense" } # optional, opaque to Warp |
Member
There was a problem hiding this comment.
We don't have support for these configs yet. Fine to skip for V0
| name = "intelephense" | ||
| command = ["intelephense", "--stdio"] | ||
| file_types = ["php", "phtml"] | ||
| root_files = ["composer.json", "composer.lock"] # optional; default: file's parent dir |
Member
There was a problem hiding this comment.
We should use our current root repo detection logic
| file_types = ["php", "phtml"] | ||
| root_files = ["composer.json", "composer.lock"] # optional; default: file's parent dir | ||
| initialization_options = { storagePath = "/tmp/intelephense" } # optional, opaque to Warp | ||
| start_timeout_ms = 5000 # optional, default 5000 |
Member
|
Thanks for writing this up! Left some comments on the product spec |
…e LSPs Addresses 7 inline comments from @kevinyang372 (maintainer) and 7 findings from oz-for-oss (1 critical, 5 important, 1 suggestion): Maintainer-driven changes (@kevinyang372): - User-configured servers now OVERWRITE built-ins for matched file types in workspaces where enabled (was: coexist alongside built-ins). - Settings UI follows existing Settings > Code > Indexing/Projects pattern; no new "Code Intelligence" section. Moved from follow-up into V0 scope. - Drop user-supplied root_files (use Warp's existing root-repo detection). - Drop initialization_options for V0 (no settings shape for nested config yet). - Drop user-configurable start_timeout_ms (fixed 5s default in code). oz-for-oss-driven changes: - CRITICAL: Settings UI moved into V0 (was follow-up); product invariants 11 and 14 require it. - Crate boundaries: UserLspServerConfig now defined in crates/lsp/src/user_config.rs and re-exported, since crates/lsp cannot depend on app/. - New required language_id field per server (file extension is insufficient for LSP identifiers like sh -> shellscript, phtml -> php). - SECURITY: command/args change on enabled server no longer auto-restarts the new binary. Stored command fingerprint detects the change; chip re-appears with re-enable affordance; user must explicitly re-confirm. - Process cleanup specifies tokio::process::Command with kill_on_drop(true) plus explicit child.kill().await; child.wait().await; on timeout/error paths (std::process::Child::drop is not a portable kill). - Cross-entry duplicate file_types now rejected at parse time (with overwrite semantics, two custom servers claiming the same extension is a configuration error). - Documented [[lsp.servers]] <-> custom_servers serde rename. Invariants restructured: 16 testable invariants (was 15), each mapped to a concrete test layer in the testing matrix.
Contributor
Author
|
Thanks for the thorough reviews! Pushed @kevinyang372 — maintainer comments
oz-for-oss findings
Net spec shapeV0 config is now minimal: [[lsp.servers]]
name = "intelephense"
command = ["intelephense", "--stdio"]
file_types = ["php", "phtml"]
language_id = "php"Four required fields, no optional V0 fields. Re-requesting review when you have a chance — happy to iterate further. |
Self-review of commit 4f85a49 found four spec-internal inconsistencies that were unrelated to the original PR review feedback. Fixing them proactively before the maintainer re-reviews. 1. Settings UI section (tech.md §8) only described `Enabled → Disable` rows. Invariants 11, 12, and 14 also require Settings UI affordances for `Dismissed → Re-enable` and `Pending re-confirmation → Re-enable`, without which a dismissal could never be cleared from the UI. Expanded §8 into a full state table covering all three row states with their predicates and click behaviors. 2. Disable button silently broke invariant 12 ("the chip reappears on next file open"): if the server was previously dismissed, the dismissal record would still suppress the chip after Disable. Spelled out that Disable clears BOTH `enabled_custom_servers[name]` and any `dismissed_custom_servers` entry for the same server. Updated product.md disable section to match. 3. Chip-state cascade in §7 and the end-to-end flow had inconsistent ordering. The flow placed `binary not on PATH → Configure` last, so a not-enabled-not-dismissed server with a missing binary would render as `Enable` (contradicting product misconfig 1's intent). Reordered the cascade in §7 and the flow diagram to match: command- changed (security override) → dismissed → binary missing → Enable default → already-running. 4. Made the security-override behavior explicit in product invariant 11: the command-changed chip surfaces even if the server was previously dismissed in that workspace, because a `command` change is a security signal and outweighs dismissal noise-suppression.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Spec PR for #8803 ("Add support for adding custom language servers"), per @kevinyang372's direction on #9562 and #9568 — closing those PRs in favor of a user-configurable LSP path. This converts that product direction into testable behavior invariants plus a concrete implementation plan grounded in the existing
crates/lsp/infrastructure.Linked Issue
ready-to-specorready-to-implement. (Add support for adding custom language servers #8803 isready-to-spec.)What's in this PR
specs/GH8803/product.md[[lsp.servers]]array-of-tables withname/command/file_types/root_files/initialization_options/start_timeout_ms). Explicit non-goals (auto-install, version pinning, marketplace). User experience walkthrough (add → chip → enable → use → disable). Open questions (~expansion, example-configs doc, network defaults).specs/GH8803/tech.mdUserConfiguredLanguageServerimpl ofLanguageServerCandidate. NewLSPServerType::UserConfigured(...)variant. Per-workspaceenabled_custom_servers/dismissed_custom_serversstate. Settings-reload diff semantics. Per-invariant test plan. Risks called out.Reuses lessons from the closed LSP PRs
This spec deliberately leans on patterns that survived bot review on #9562 and #9568, so the implementation phase doesn't relitigate them:
binary_in_pathPATH search with executable-bit check on Unix — from feat: add PHP language support via Intelephense LSP (#9168) #9562'scrates/lsp/src/servers/intelephense.rs. Used here asis_installed_on_path.warpui::r#async::FutureExt::with_timeoutfor boundedinitialize— from feat: add PHP language support via Intelephense LSP (#9168) #9562's follow-up commit31285c4. Used here as thestart_timeout_msenforcement path.std::env::join_paths+binary_filenamefor cross-platform tests — from feat: add PHP language support via Intelephense LSP (#9168) #9562/feat: add JSON language support via vscode-json-languageserver (#9556) — v2 #9568's test suites. Reused for the newuser_configured_tests.rs.initialization_optionsas the security-knob surface — from feat: add JSON language support via vscode-json-languageserver (#9556) — v2 #9568'shandledSchemaProtocolsdiscovery. Documented as the recommended pattern for user-managed network-fetching restrictions.Scope discipline
Open questions for maintainers (also documented in product.md)
~expansion incommand[0]— Cmd-O //open-filealready useshellexpand::tilde; recommend yes for~, defer$VARto a follow-up. Confirm?docs/custom-lsp-examples.md(intelephense, lua-language-server, zls, bash-language-server) alongside the implementation. Worth gating behind this spec or a separate docs PR?initialization_optionsand an examples doc rather than a generic flag. Confirm?Testing — for the spec itself
31285c4, etc.) and existing files (crates/lsp/src/servers/intelephense.rs,app/src/settings/input.rs) verified at write time.Agent Mode
CHANGELOG-NEW-FEATURE: Spec for user-configurable language servers (#8803) — defines the TOML config shape, per-workspace enablement model, and lifecycle for custom LSPs that aren't shipped built-in. Implementation PR to follow. Thanks @lonexreb!