spec: user-configurable language servers (#8803)#9848
spec: user-configurable language servers (#8803)#9848lonexreb wants to merge 1 commit 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).
|
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}
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). |
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, |
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. |
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. |
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. |
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. |
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: |
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]].
| - **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. |
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". |
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. |
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. |
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 |
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 |
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 |
|
Thanks for writing this up! Left some comments on the product spec |
2 participants
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!