From e7b22d266cb786e65141fd8b7c55298204d3e39e Mon Sep 17 00:00:00 2001 From: lonexreb Date: Fri, 1 May 2026 15:31:23 -0500 Subject: [PATCH 1/3] spec: user-configurable language servers (#8803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @kevinyang372's direction on #9562 and #9568 (closing those PRs in favor of a user-configurable LSP path), this spec turns #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 (#9562) * `warpui::r#async::FutureExt::with_timeout` for bounded init (#9562 follow-up commit `31285c4`) * `std::env::join_paths` + `binary_filename` for cross-platform tests (#9562, #9568) * `initialization_options` plumbing for security-sensitive defaults (#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). --- specs/GH8803/product.md | 125 ++++++++++++++++++ specs/GH8803/tech.md | 281 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 406 insertions(+) create mode 100644 specs/GH8803/product.md create mode 100644 specs/GH8803/tech.md diff --git a/specs/GH8803/product.md b/specs/GH8803/product.md new file mode 100644 index 000000000..7119c8d25 --- /dev/null +++ b/specs/GH8803/product.md @@ -0,0 +1,125 @@ +# Product Spec: User-configurable language servers + +**Issue:** [warpdotdev/warp#8803](https://github.com/warpdotdev/warp/issues/8803) +**Figma:** none provided + +## Summary + +Let users add language servers Warp does not ship out of the box by declaring them in a settings file (binary path, arguments, file types they apply to, optional root-file globs, optional initialization options). When the user opens a file matching a configured server's file types, Warp offers to enable that server for the workspace and — once enabled — uses it for code intelligence (diagnostics, hover, goto, completions) on that file type, alongside any built-in servers. + +This is the contributor-facing alternative to baking each language server into the Warp binary. It directly addresses why PRs adding specific built-in LSPs (e.g. PHP Intelephense in #9562, JSON in #9568) were closed in favor of this product direction. + +## Problem + +Warp currently ships a closed set of language servers as variants of `crates/lsp/src/supported_servers::LSPServerType`. Every new language requires: + +1. A new `LSPServerType` enum variant. +2. A new `LanguageServerCandidate` impl with detection, install, and `fetch_latest_server_metadata` logic. +3. A new `LanguageId` enum variant in `crates/lsp/src/config.rs` plus extension mapping. +4. A new entry in `LanguageId::lsp_language_identifier`. + +This scales poorly and pulls language-specific install logic into the core. Users with niche languages (Lua, Zig, Swift, Elixir, Solidity, Bash, Tailwind, etc.) cannot add support without modifying the binary. Maintainers cannot accept PRs adding specific servers without committing to ongoing maintenance of those servers' install/version-fetch code paths. + +## Goals + +- A user can add a language server to Warp by editing a settings file — no binary modifications, no agent involvement. +- Configured servers run side-by-side with built-in servers and follow the same code-intelligence surface (diagnostics, hover, goto, completions, semantic tokens). +- Configuration is portable across workspaces and discoverable in Warp's settings UI. +- Enablement is per-workspace by default with a clear opt-in moment, not silent global activation. +- Mis-configuration produces a visible, actionable error — not a silent disabled state. + +## Non-goals + +- **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. +- **Cross-workspace global enablement on first open.** A configured server is *defined* globally but *enabled* per-workspace. +- **Marketplace / discovery of community configs.** Out of scope; users find configs themselves. + +## User experience + +### Adding a server + +1. User edits their Warp settings file (TOML, located at the standard Warp settings path) and adds an `[[lsp.servers]]` entry. (See the "Configuration shape" section below.) +2. Warp detects the new entry on settings reload. No restart required. +3. If the entry is malformed (missing `name`, missing `command`, empty `file_types`), Warp shows a non-blocking notification: *"Custom LSP `` is misconfigured: . See settings."* with a button to open the settings file at the offending line. + +### First time opening a matching file + +1. User opens a file whose extension matches a configured server's `file_types` (e.g. opens `foo.lua` with a Lua server configured). +2. Warp detects the configured server is *defined* but not yet *enabled* for this workspace. +3. The editor footer shows a chip: *"Enable `` for this workspace?"* with `Enable` / `Dismiss` buttons. +4. If `Enable` is pressed, Warp: + - Records the per-workspace enablement. + - Spawns the server process with the configured command and args. + - Starts driving LSP traffic for files matching `file_types` in this workspace. +5. If `Dismiss` is pressed, the chip is suppressed for this workspace until the user re-opens it from settings. + +### Subsequent opens in an enabled workspace + +1. User opens any file matching `file_types` in a workspace where the server is already enabled. +2. The server is already running; no UI surfaces. Diagnostics, hover, goto, completions all behave as they do for built-in servers. + +### Disabling a server in a workspace + +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. + +### Misconfiguration scenarios + +1. **Binary not on PATH:** When a user enables a server whose `command[0]` is not on PATH, Warp shows: *"Could not start ``: binary `` not found on PATH."* The chip's `Enable` button is replaced with `Open settings`. +2. **Binary on PATH but spawn fails:** Warp shows: *"`` exited with status ``. 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: *"`` did not respond to `initialize` within 5s."* The server's process is killed. + +## Configuration shape + +The user's Warp settings TOML grows a new `[lsp]` table. Multiple servers via array-of-tables `[[lsp.servers]]`: + +```toml +[[lsp.servers]] +name = "intelephense" +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 +start_timeout_ms = 5000 # optional, default 5000 +``` + +| Field | Required | Type | Notes | +|---|---|---|---| +| `name` | yes | string | Display name. Must be unique across all configured servers. | +| `command` | yes | array of strings | First element is the binary; rest are args. Resolved against PATH. | +| `file_types` | yes | array of strings | File extensions (no dot) the server handles. Must be non-empty. | +| `root_files` | no | array of strings | Glob patterns whose presence in an ancestor directory marks the workspace root. Default: the file's parent directory. | +| `initialization_options` | no | TOML table | Passed verbatim to the LSP `initialize` request as `initializationOptions`. Opaque to Warp. | +| `start_timeout_ms` | no | integer | Bound on the time we wait for `initialize` to return. Default 5000. | + +Settings reload re-reads the entire `[lsp]` table; servers whose configuration changed are restarted. Removed entries shut down. Added entries become available (but are not auto-enabled). + +## Testable behavior invariants + +Numbered list — each maps to a verification path in the tech spec: + +1. A `[[lsp.servers]]` entry with `name`, `command` (non-empty array), and `file_types` (non-empty array) is accepted at settings parse time. +2. A `[[lsp.servers]]` entry missing any of `name` / `command` / `file_types`, OR with empty `command` / `file_types`, OR with a duplicate `name`, is rejected at parse time and surfaces a settings-error notification with the offending line range. +3. Opening a file whose extension is in `file_types` of a configured-but-not-enabled server shows the "Enable" chip in the editor footer for that file's workspace exactly once per (server, workspace) pair until the user dismisses or enables. +4. Pressing "Enable" on the chip starts a server process via `command[0]` with `command[1..]` as args, sends an LSP `initialize` request (with `initializationOptions` from the config), and on receiving a successful response begins routing LSP traffic for files matching `file_types` in that workspace. +5. If `command[0]` is not on PATH at enablement time, no process is spawned and the user sees an error notification with `Open settings` action. +6. If the spawned process exits non-zero before sending an `initialize` response, the user sees an error notification with the exit status and last 200 bytes of stderr. +7. If `initialize` does not return within `start_timeout_ms` (default 5000), the spawned process is killed via `Drop` of the `Child` handle, the LSP client is torn down cleanly, and the user sees a timeout notification. +8. After a server is enabled in workspace W, opening any file matching `file_types` in W routes LSP requests to that server **without** showing the chip again. +9. After settings change (server entry edited or removed), an enabled-and-running server for the changed entry is restarted with the new config (or shut down if removed) within 1s of settings reload, with no Warp restart required. +10. The user-configured server runs alongside any built-in server whose `LSPServerType` matches the same file extension; both servers receive requests, and the LSP client merges responses (existing built-in client behavior; not changed by this feature). +11. Disabling a server via settings UI shuts down its process for the targeted workspace within 1s, removes the per-workspace enablement record, and the chip reappears on next file open. +12. Restarting Warp preserves per-workspace enablement state — workspaces where a server was enabled before restart auto-spawn the server on the next file open without the chip reappearing. +13. `initialization_options` in the TOML is forwarded to the `initialize` request's `initializationOptions` field byte-equivalent (TOML → JSON conversion preserves nested tables and arrays). +14. The chip is **not** shown for a configured server in a workspace that has explicitly dismissed it; the user must re-enable from settings UI. +15. Shutting down the LSP system (e.g. on Warp quit) sends `shutdown` then `exit` to all running custom servers and waits up to 1s for graceful exit before SIGKILL. + +## Open questions + +- **Should `command` support `~` and `$VAR` expansion?** Cmd-O / `/open-file` use `shellexpand::tilde`; consistent behavior here would be friendly. Recommend yes for `~`, defer `$VAR` to a follow-up. +- **Should we ship example configs?** A `docs/custom-lsp-examples.md` with intelephense / lua-language-server / zls / bash-language-server entries would shorten time-to-first-success. Recommend yes; not part of the core feature gate. +- **Schema-fetching restriction:** The JSON LSP work in #9568 found that VS Code's JSON server fetches remote schemas by default. Should the spec mandate that custom LSPs run with `network_access = false` by default? This is hard to enforce generically (each server has its own config keys for schema fetching). Recommend punting to per-server `initialization_options` and documenting the pattern. diff --git a/specs/GH8803/tech.md b/specs/GH8803/tech.md new file mode 100644 index 000000000..7fea53500 --- /dev/null +++ b/specs/GH8803/tech.md @@ -0,0 +1,281 @@ +# Tech Spec: User-configurable language servers + +**Issue:** [warpdotdev/warp#8803](https://github.com/warpdotdev/warp/issues/8803) + +## Context + +Warp's LSP layer currently treats every language server as a closed-set enum variant on `LSPServerType` (`crates/lsp/src/supported_servers.rs`). Each variant has: + +- A `LanguageServerCandidate` impl in `crates/lsp/src/servers/.rs` (rust_analyzer, gopls, pyright, intelephense, etc.). +- An entry in `LanguageId` (`crates/lsp/src/config.rs:25-36`) that maps file extensions to language identifiers. +- A `LanguageId::lsp_language_identifier` arm and a `LanguageId::from_path` arm. + +Adding a new language requires touching all four sites. PRs #9562 (PHP Intelephense) and #9568 (JSON via vscode-json-languageserver) demonstrated this pattern but were closed by maintainer @kevinyang372 in favor of this user-configurable path. The infrastructure those PRs built (probe-spawn install detection, executable-bit checks, cross-platform PATH handling, `INSTALL_PROBE_TIMEOUT` via `warpui::r#async::FutureExt::with_timeout`) is reusable here. + +### Relevant code + +| Path | Role | +|---|---| +| `crates/lsp/src/language_server_candidate.rs` | The trait every server impl satisfies. The natural extension point — a new impl, `UserConfiguredLanguageServer`, will go here. | +| `crates/lsp/src/supported_servers.rs` | `LSPServerType` enum + the closed registry of impls. Will grow a new arm carrying user config. | +| `crates/lsp/src/config.rs` | `LanguageId` enum + `from_path` extension mapping + `lsp_language_identifier`. Needs a path that bypasses the enum for user-configured types. | +| `crates/lsp/src/manager.rs` | Spawns/owns running LSP processes. The new code lives here for per-workspace lifecycle. | +| `app/src/settings/` | Settings group definitions (see `app/src/settings/input.rs` for the macro pattern). Where `[lsp.servers]` parsing lands. | +| `app/src/code/editor/` | Editor footer rendering — where the "Enable `` for this workspace?" chip surfaces. | + +### Related closed PRs (input to this spec) + +- #9562 — PHP Intelephense as built-in. Closed; lessons: probe-spawn `--stdio` with bounded timeout, executable-bit check on Unix, executable's full PATH search via `binary_in_path` helper, cross-platform tests via `std::env::join_paths`. +- #9568 — JSON via vscode-json-languageserver. Closed; lessons: schema-fetching is a security surface (the JSON server defaults to fetching `http`/`https`/`file` schema URIs); `initializationOptions` is the right place for per-server restrictions like `handledSchemaProtocols`. + +## Proposed changes + +### 1. New settings group: `LspSettings` + +**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`. The struct: + +```rust +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +pub struct UserLspServerConfig { + pub name: String, + pub command: Vec, + pub file_types: Vec, + #[serde(default)] + pub root_files: Vec, + #[serde(default)] + pub initialization_options: Option, + #[serde(default = "default_start_timeout_ms")] + pub start_timeout_ms: u64, +} + +fn default_start_timeout_ms() -> u64 { 5000 } +``` + +Validation runs at parse time (in a `validate()` method called from the settings init path): + +- `name` non-empty and unique across the vec. +- `command` non-empty. +- `file_types` non-empty; each entry stripped of leading `.`. +- `start_timeout_ms` ≥ 100 and ≤ 60_000. + +Validation failures emit a settings-error notification (existing pattern in `app/src/settings/initializer.rs`) with the offending entry index. Other entries continue to load. + +### 2. Per-workspace enablement state + +**File:** new fields on the existing per-workspace settings struct (`app/src/workspace/state.rs` or similar — exact location depends on where workspace-scoped state lives; verify against current code at implementation time). + +```rust +pub struct WorkspaceLspState { + /// Set of `UserLspServerConfig.name` values the user explicitly enabled + /// for this workspace. Survives Warp restart via the existing workspace + /// state persistence path. + enabled_custom_servers: HashSet, + /// Set of `UserLspServerConfig.name` values dismissed via the chip. + /// Suppresses the chip until cleared from settings UI. + dismissed_custom_servers: HashSet, +} +``` + +### 3. New `LanguageServerCandidate` impl + +**File:** new `crates/lsp/src/servers/user_configured.rs`. + +```rust +pub struct UserConfiguredLanguageServer { + config: UserLspServerConfig, + workspace_root: PathBuf, +} + +#[async_trait] +impl LanguageServerCandidate for UserConfiguredLanguageServer { + async fn should_suggest_for_repo(&self, path: &Path, _executor: &CommandBuilder) -> bool { + // Heuristic: any file in `path` matches one of `file_types`, + // OR (if `root_files` is non-empty) one of those globs is present + // in `path` or any ancestor up to the workspace root. + // Implementation-wise: glob over the immediate dir for `file_types`, + // walk parents for `root_files`. + ... + } + + async fn is_installed_in_data_dir(&self, _executor: &CommandBuilder) -> bool { + // We never install user-configured servers. Always false. + false + } + + async fn is_installed_on_path(&self, executor: &CommandBuilder) -> bool { + // Reuse the patterns from #9562's probe: + // 1. `binary_in_path` filesystem check for `command[0]` (with executable-bit check on Unix) + // 2. NO probe-spawn — that contract was specific to npm-installed servers + // that may have stale shims. For user-configured servers we trust + // the user; an unhealthy spawn is surfaced via the start-time + // error toast instead. + binary_in_path(&self.config.command[0], executor.path_env_var()) + } + + async fn install(&self, _: LanguageServerMetadata, _: &CommandBuilder) -> anyhow::Result<()> { + // User-configured servers are never installed by Warp. The user owns + // the lifecycle. Surface a clear error if anything tries to call this. + anyhow::bail!("user-configured LSP `{}` is not installable by Warp", self.config.name) + } + + async fn fetch_latest_server_metadata(&self) -> anyhow::Result { + anyhow::bail!("user-configured LSP `{}` has no version metadata", self.config.name) + } +} +``` + +### 4. `LSPServerType` extension + +**File:** `crates/lsp/src/supported_servers.rs`. + +Add a variant: + +```rust +pub enum LSPServerType { + // ...existing variants... + UserConfigured(UserLspServerConfig), +} +``` + +The construction site that walks built-in servers becomes: + +```rust +fn all_candidates_for(workspace: &Workspace, settings: &LspSettings) -> Vec> { + let mut out = built_in_candidates(); + for cfg in &settings.custom_servers.0 { + if workspace.lsp_state.enabled_custom_servers.contains(&cfg.name) { + out.push(Box::new(UserConfiguredLanguageServer::new(cfg.clone(), workspace.root.clone()))); + } + } + out +} +``` + +### 5. Bypassing `LanguageId` for custom file types + +The `LanguageId` enum is used to populate the `languageId` field in LSP `textDocument/didOpen`. For user-configured servers, the user provides `file_types` directly. Two implementation options: + +**Option A — extend the enum with a `Custom(String)` variant.** The `Custom(String)` carries the file extension and `lsp_language_identifier()` returns it as-is. + +**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. + +### 6. Footer chip + +**File:** `app/src/code/editor/` — extending whatever surface renders the LSP-related chip when a built-in server is detected-but-not-installed (find via `grep -rn "Install" app/src/code/editor/`). + +The chip rendering branches on three states: + +1. Built-in server detected, not installed → existing "Install ``" chip. +2. Custom server defined, not enabled in workspace, not dismissed → new "Enable ``" chip. +3. Custom server defined, but `command[0]` not on PATH → "Configure ``" chip with `Open settings` action (variant of state 2). + +Click handlers: + +- `Enable` → mutate `WorkspaceLspState.enabled_custom_servers`, persist, trigger `manager::start_for_workspace` for the new candidate. +- `Dismiss` → mutate `dismissed_custom_servers`, persist, no spawn. +- `Open settings` → existing settings-open-with-search action, scoped to `lsp.servers`. + +### 7. Settings reload handling + +**File:** `app/src/settings/initializer.rs` — extend the existing reload path. + +On settings change: + +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. + +The diff mechanism reuses existing settings-event hooks; no new infra needed. + +### 8. Lifecycle: spawn, initialize, timeout, shutdown + +**File:** `crates/lsp/src/manager.rs`. + +The existing `start_for_workspace` (or whatever the entry-point is named — verify) spawns built-in servers. Extending it for user-configured servers means: + +- Build the `Command` from `config.command[0]` + `config.command[1..]`. +- `stdin/stdout` piped, `stderr` captured to a per-server ring buffer (last 200 bytes for error toasts). +- 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. +- Forward `config.initialization_options` (TOML → `serde_json::Value`) into `InitializeParams.initialization_options` on the request. +- On Warp shutdown: existing `shutdown` + `exit` flow already handles all running servers; user-configured servers ride the same path. + +## Testing and validation + +Each invariant from `product.md` maps to a test at this layer: + +| Invariant | Test layer | File | +|---|---|---| +| 1, 2 (parse / validate) | unit | `app/src/settings/lsp_tests.rs` (new) — TOML strings → `LspSettings` parse outcomes (success cases + each validation error). | +| 4, 5, 6, 7 (spawn outcomes) | unit | `crates/lsp/src/servers/user_configured_tests.rs` (new) — mock `CommandBuilder` returning success / error / hang. | +| 4 (`initialization_options` forwarded) | unit | same file — assert the `InitializeParams` JSON contains the configured options byte-equivalent. | +| 7 (timeout via `with_timeout`) | unit | same file — wire a future that never resolves, assert the timeout branch fires within `start_timeout_ms + 100`. | +| 9 (settings reload restarts running servers) | unit | `crates/lsp/src/manager_tests.rs` extension — pre-populate a running server, swap config, assert restart. | +| 10 (built-in + user-configured coexist) | integration | `crates/integration/tests/` — open a `.py` file in a workspace where both pyright (built-in) and a user-configured "ruff" server match; assert both are in the active candidate list. | +| 12 (per-workspace enablement persists across restart) | unit | workspace-state serialization round-trip. | +| 3, 8, 11, 14 (chip behavior) | integration | UI integration test under `crates/integration/`. Stub the file-open event, assert chip presence/absence based on enablement+dismissal state. | +| 13 (TOML→JSON shape preservation) | unit | parse a TOML with nested table + array, assert resulting `serde_json::Value` is structurally equivalent. | +| 15 (graceful shutdown on quit) | unit | shutdown handler test — running user-configured server receives `shutdown` then `exit` then has 1s window before SIGKILL. | + +### Cross-platform constraints (lessons from #9562/#9568) + +- Tests building `PATH` strings must use `std::env::join_paths`, not `:`. Reuse the `make_path_var` helper introduced in #9562's tests. +- On Windows, `command[0]` may need `.exe` / `.cmd` resolution. Reuse `binary_filename` helper (also from #9562's tests). +- `std::process::Stdio::null()` for stdin during `is_installed_on_path` check is a no-op for user-configured servers in this design (we don't probe-spawn), but if we ever do, the same pattern from `intelephense.rs:113` applies. + +## End-to-end flow + +``` +User edits settings.toml + └─> [LspSettings::reload] (settings/lsp.rs) + ├─> validate() (rejects malformed) + └─> emit SettingsChanged event + └─> [manager::on_settings_change] (lsp/src/manager.rs) + ├─> diff old vs new custom_servers + ├─> stop removed entries + └─> restart changed entries that are running + +User opens .lua file in workspace W + └─> [editor::on_file_open] (app/src/code/editor/) + └─> [chip_renderer] + ├─> get LspSettings.custom_servers + ├─> filter where file_types contains "lua" + ├─> filter where W.enabled_custom_servers does NOT contain name + ├─> filter where W.dismissed_custom_servers does NOT contain name + └─> render "Enable " chip per remaining entry + +User clicks Enable + └─> [chip_handler::on_enable_clicked] + ├─> W.enabled_custom_servers.insert(name) + ├─> persist W + └─> [manager::start_candidate] (lsp/src/manager.rs) + ├─> construct UserConfiguredLanguageServer (servers/user_configured.rs) + ├─> is_installed_on_path → if false, surface "Configure " toast + ├─> spawn Command with stderr-buffered + ├─> initialize.with_timeout(start_timeout_ms) + │ ├─> Ok(Ok(_)) → register, route LSP traffic + │ ├─> Ok(Err(err)) → surface error toast, drop child + │ └─> Err(_timeout) → kill child via Drop, surface timeout toast + └─> on subsequent file opens, no chip — server already running +``` + +## Risks + +- **`initialization_options` is a security surface.** Some servers (e.g. JSON via vscode-json-languageserver, see #9568) default to fetching remote schemas. The user controls this knob, but they may not realize it. **Mitigation:** ship `docs/custom-lsp-examples.md` with annotated examples that explicitly set network-related options to safe defaults where applicable. +- **A configured server that crashes on every spawn could loop forever.** **Mitigation:** the chip's `Enable` action is one-shot per click. We do not auto-restart on crash; the user re-enables manually. If exit happens after `initialize` succeeded, we surface a "server crashed" toast and the chip returns to the disabled-but-defined state. +- **`Custom(String)` propagation in `LanguageId`.** Adding a `Custom` variant to a closed exhaustive enum touches every match site. **Mitigation:** WARP.md prohibits wildcard matches, so the compiler will surface every site at code-write time. Audit during implementation. +- **Per-workspace state migration.** Existing workspaces have no `enabled_custom_servers` field. **Mitigation:** `#[serde(default)]` on the new fields; absence parses as empty set. + +## 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). +- `~` and `$VAR` expansion in `command[0]`. Recommend yes for `~` (consistency with `/open-file`); defer `$VAR`. +- Document `network_access` semantics if we later add a generic per-server flag. The JSON-server-specific `handledSchemaProtocols` discovery from #9568's review is the model. From 4f85a49235c03c42dbc13f0dd61a558da95e17de Mon Sep 17 00:00:00 2001 From: lonexreb Date: Sun, 3 May 2026 02:47:58 -0500 Subject: [PATCH 2/3] spec: address PR #9848 review feedback for user-configurable 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. --- specs/GH8803/product.md | 91 +++++++------ specs/GH8803/tech.md | 282 +++++++++++++++++++++++++--------------- 2 files changed, 231 insertions(+), 142 deletions(-) diff --git a/specs/GH8803/product.md b/specs/GH8803/product.md index 7119c8d25..8ffe6169c 100644 --- a/specs/GH8803/product.md +++ b/specs/GH8803/product.md @@ -5,7 +5,7 @@ ## Summary -Let users add language servers Warp does not ship out of the box by declaring them in a settings file (binary path, arguments, file types they apply to, optional root-file globs, optional initialization options). When the user opens a file matching a configured server's file types, Warp offers to enable that server for the workspace and — once enabled — uses it for code intelligence (diagnostics, hover, goto, completions) on that file type, alongside any built-in servers. +Let users add language servers Warp does not ship out of the box by declaring them in a settings file (binary path, arguments, file types, LSP language identifier). When the user opens a file matching a configured server's file types, Warp offers to enable that server for the workspace. Once enabled, the user-configured server **takes over** code intelligence (diagnostics, hover, goto, completions) for those file types in that workspace, replacing any built-in server that would otherwise handle them. This is the contributor-facing alternative to baking each language server into the Warp binary. It directly addresses why PRs adding specific built-in LSPs (e.g. PHP Intelephense in #9562, JSON in #9568) were closed in favor of this product direction. @@ -23,8 +23,8 @@ This scales poorly and pulls language-specific install logic into the core. User ## Goals - A user can add a language server to Warp by editing a settings file — no binary modifications, no agent involvement. -- Configured servers run side-by-side with built-in servers and follow the same code-intelligence surface (diagnostics, hover, goto, completions, semantic tokens). -- Configuration is portable across workspaces and discoverable in Warp's settings UI. +- A user-configured server **supersedes** any built-in server for the file extensions it declares (in workspaces where it is enabled). Two servers do not run for the same file type in the same workspace. +- Configuration is portable across workspaces and discoverable in Warp's existing Settings UI. - Enablement is per-workspace by default with a clear opt-in moment, not silent global activation. - Mis-configuration produces a visible, actionable error — not a silent disabled state. @@ -32,10 +32,11 @@ This scales poorly and pulls language-specific install logic into the core. User - **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. +- **Per-file dynamic switching.** Within a workspace, a single file extension is associated with at most one user-configured server. No "try server A, fall back to server B" runtime logic. +- **Coexisting with a built-in server on the same file extension.** When a user-configured server is enabled in workspace W and matches file type X, the built-in server that would otherwise serve X in W is **not** spawned for W (per @kevinyang372's review of the original draft — running both is undesirable). - **Cross-workspace global enablement on first open.** A configured server is *defined* globally but *enabled* per-workspace. - **Marketplace / discovery of community configs.** Out of scope; users find configs themselves. +- **Per-server initialization options (V0).** `initializationOptions` forwarding is deferred to a follow-up; warp does not yet have settings-file shape for arbitrary nested config payloads. ## User experience @@ -43,7 +44,7 @@ This scales poorly and pulls language-specific install logic into the core. User 1. User edits their Warp settings file (TOML, located at the standard Warp settings path) and adds an `[[lsp.servers]]` entry. (See the "Configuration shape" section below.) 2. Warp detects the new entry on settings reload. No restart required. -3. If the entry is malformed (missing `name`, missing `command`, empty `file_types`), Warp shows a non-blocking notification: *"Custom LSP `` is misconfigured: . See settings."* with a button to open the settings file at the offending line. +3. If the entry is malformed (missing `name`, missing `command`, empty `file_types`, missing `language_id`, duplicate `name`, or duplicate `file_types` across user-configured entries), Warp shows a non-blocking notification: *"Custom LSP `` is misconfigured: . See settings."* with a button to open the settings file at the offending line. ### First time opening a matching file @@ -52,26 +53,38 @@ This scales poorly and pulls language-specific install logic into the core. User 3. The editor footer shows a chip: *"Enable `` for this workspace?"* with `Enable` / `Dismiss` buttons. 4. If `Enable` is pressed, Warp: - Records the per-workspace enablement. - - Spawns the server process with the configured command and args. + - Suppresses any built-in server that would otherwise serve `file_types` for this workspace (shutting it down for this workspace if currently running). + - Spawns the user's server process with the configured command and args. - Starts driving LSP traffic for files matching `file_types` in this workspace. -5. If `Dismiss` is pressed, the chip is suppressed for this workspace until the user re-opens it from settings. +5. If `Dismiss` is pressed, the chip is suppressed for this workspace until the user re-enables it from Settings UI. The built-in server (if any) continues to handle the file type. ### Subsequent opens in an enabled workspace 1. User opens any file matching `file_types` in a workspace where the server is already enabled. -2. The server is already running; no UI surfaces. Diagnostics, hover, goto, completions all behave as they do for built-in servers. +2. The user-configured server is already running; no UI surfaces. Diagnostics, hover, goto, completions all behave as they do for built-in servers. ### Disabling a server in a workspace -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. +The disable flow piggybacks on Warp's existing **Settings → Code → Indexing and Projects** UI pattern (per @kevinyang372's direction; no new "Code intelligence" section is introduced for V0): + +1. User opens **Settings → Code**. The existing per-workspace project list grows a sub-row per user-configured LSP that is currently enabled in that workspace. +2. The row exposes a `Disable` toggle. Toggling off shuts down the server's process for that workspace within 1s, removes the per-workspace enablement record, and re-enables the built-in server for that file type if one exists. ### Misconfiguration scenarios 1. **Binary not on PATH:** When a user enables a server whose `command[0]` is not on PATH, Warp shows: *"Could not start ``: binary `` not found on PATH."* The chip's `Enable` button is replaced with `Open settings`. 2. **Binary on PATH but spawn fails:** Warp shows: *"`` exited with status ``. 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: *"`` did not respond to `initialize` within 5s."* The server's process is killed. +3. **Spawn or `initialize` hangs:** Warp bounds the LSP `initialize` request with a fixed default timeout (5s, not user-configurable in V0 per @kevinyang372). On timeout, Warp shows: *"`` did not respond to `initialize` within 5s."* The server's process is killed and reaped. + +### Command/args change after enablement (security path) + +If the user edits the `command` or `args` of an already-enabled server, Warp does **not** silently respawn the new binary. Instead: + +1. The currently running process is shut down cleanly (`shutdown` → `exit` → kill if needed). +2. The server's enablement is moved to a "needs re-confirmation" state. +3. On the next file open matching `file_types`, the chip reappears: *"`` command changed — re-enable?"*. The user must explicitly re-confirm before the new binary runs. + +This protects users whose settings are edited by another tool (sync, IDE, malicious process) from silently executing a different binary in an already-trusted workspace. ## Configuration shape @@ -82,44 +95,48 @@ The user's Warp settings TOML grows a new `[lsp]` table. Multiple servers via ar name = "intelephense" 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 -start_timeout_ms = 5000 # optional, default 5000 +language_id = "php" + +[[lsp.servers]] +name = "bash-language-server" +command = ["bash-language-server", "start"] +file_types = ["sh", "bash"] +language_id = "shellscript" ``` | Field | Required | Type | Notes | |---|---|---|---| | `name` | yes | string | Display name. Must be unique across all configured servers. | -| `command` | yes | array of strings | First element is the binary; rest are args. Resolved against PATH. | -| `file_types` | yes | array of strings | File extensions (no dot) the server handles. Must be non-empty. | -| `root_files` | no | array of strings | Glob patterns whose presence in an ancestor directory marks the workspace root. Default: the file's parent directory. | -| `initialization_options` | no | TOML table | Passed verbatim to the LSP `initialize` request as `initializationOptions`. Opaque to Warp. | -| `start_timeout_ms` | no | integer | Bound on the time we wait for `initialize` to return. Default 5000. | +| `command` | yes | array of strings | First element is the binary; rest are args. Resolved against PATH. Must be non-empty. | +| `file_types` | yes | array of strings | File extensions (no dot) the server handles. Must be non-empty. **No two configured servers may declare overlapping `file_types`** — Warp rejects the settings on parse if they do. | +| `language_id` | yes | string | The LSP `languageId` Warp will send in `textDocument/didOpen.languageId` for files matching `file_types`. Required because file extension is not a reliable proxy (`sh` → `shellscript`, `phtml` → `php`). | + +Settings reload re-reads the entire `[lsp]` table; servers whose `command` or `args` changed enter the re-confirmation flow described above. Removed entries shut down. Added entries become available (but are not auto-enabled). Pure metadata changes (e.g. `language_id` only, no `command` change) restart the process in place without re-confirmation. -Settings reload re-reads the entire `[lsp]` table; servers whose configuration changed are restarted. Removed entries shut down. Added entries become available (but are not auto-enabled). +**Out of V0 (deferred to follow-ups):** `root_files` (Warp's existing root-repo detection is used), `initialization_options`, `start_timeout_ms` (fixed default in code), `~`/`$VAR` expansion in `command[0]`. ## Testable behavior invariants Numbered list — each maps to a verification path in the tech spec: -1. A `[[lsp.servers]]` entry with `name`, `command` (non-empty array), and `file_types` (non-empty array) is accepted at settings parse time. -2. A `[[lsp.servers]]` entry missing any of `name` / `command` / `file_types`, OR with empty `command` / `file_types`, OR with a duplicate `name`, is rejected at parse time and surfaces a settings-error notification with the offending line range. +1. A `[[lsp.servers]]` entry with `name`, `command` (non-empty array), `file_types` (non-empty array), and `language_id` (non-empty string) is accepted at settings parse time. +2. A `[[lsp.servers]]` entry missing any required field, with empty `command` / `file_types`, with a duplicate `name`, or whose `file_types` overlap with another configured entry's `file_types`, is rejected at parse time and surfaces a settings-error notification with the offending line range. 3. Opening a file whose extension is in `file_types` of a configured-but-not-enabled server shows the "Enable" chip in the editor footer for that file's workspace exactly once per (server, workspace) pair until the user dismisses or enables. -4. Pressing "Enable" on the chip starts a server process via `command[0]` with `command[1..]` as args, sends an LSP `initialize` request (with `initializationOptions` from the config), and on receiving a successful response begins routing LSP traffic for files matching `file_types` in that workspace. +4. Pressing "Enable" on the chip starts a server process via `command[0]` with `command[1..]` as args, sends an LSP `initialize` request, and on receiving a successful response begins routing LSP traffic for files matching `file_types` in that workspace. 5. If `command[0]` is not on PATH at enablement time, no process is spawned and the user sees an error notification with `Open settings` action. 6. If the spawned process exits non-zero before sending an `initialize` response, the user sees an error notification with the exit status and last 200 bytes of stderr. -7. If `initialize` does not return within `start_timeout_ms` (default 5000), the spawned process is killed via `Drop` of the `Child` handle, the LSP client is torn down cleanly, and the user sees a timeout notification. -8. After a server is enabled in workspace W, opening any file matching `file_types` in W routes LSP requests to that server **without** showing the chip again. -9. After settings change (server entry edited or removed), an enabled-and-running server for the changed entry is restarted with the new config (or shut down if removed) within 1s of settings reload, with no Warp restart required. -10. The user-configured server runs alongside any built-in server whose `LSPServerType` matches the same file extension; both servers receive requests, and the LSP client merges responses (existing built-in client behavior; not changed by this feature). -11. Disabling a server via settings UI shuts down its process for the targeted workspace within 1s, removes the per-workspace enablement record, and the chip reappears on next file open. -12. Restarting Warp preserves per-workspace enablement state — workspaces where a server was enabled before restart auto-spawn the server on the next file open without the chip reappearing. -13. `initialization_options` in the TOML is forwarded to the `initialize` request's `initializationOptions` field byte-equivalent (TOML → JSON conversion preserves nested tables and arrays). -14. The chip is **not** shown for a configured server in a workspace that has explicitly dismissed it; the user must re-enable from settings UI. -15. Shutting down the LSP system (e.g. on Warp quit) sends `shutdown` then `exit` to all running custom servers and waits up to 1s for graceful exit before SIGKILL. +7. If `initialize` does not return within the fixed 5s timeout, the spawned process is explicitly killed and reaped (`kill()` + `wait()` semantics, not Drop alone), the LSP client is torn down cleanly, and the user sees a timeout notification. +8. After a server is enabled in workspace W, opening any file matching `file_types` in W routes LSP requests to that server **without** showing the chip again, and **without** a built-in server for the same file types running concurrently in W. +9. Enabling a user-configured server in workspace W that matches a file type also handled by a built-in shuts down the built-in for W within 1s; disabling the user-configured server restores the built-in for W within 1s. +10. After settings change to a server entry's metadata fields only (e.g. `language_id`, `file_types` ordering), an enabled-and-running server is restarted with the new metadata within 1s of settings reload, with no Warp restart and no re-confirmation prompt required. +11. After settings change to an enabled server's `command` or args, the process is shut down, enablement moves to "needs re-confirmation", and the chip reappears on next matching file open until the user re-confirms. +12. Disabling a server via Settings UI shuts down its process for the targeted workspace within 1s, removes the per-workspace enablement record, restores the built-in for the file type if any, and the chip reappears on next file open. +13. Restarting Warp preserves per-workspace enablement state — workspaces where a server was enabled before restart auto-spawn the user-configured server on the next file open without the chip reappearing, and the built-in for the same file types is not spawned. +14. The chip is **not** shown for a configured server in a workspace that has explicitly dismissed it; the user must re-enable from Settings UI. +15. The `languageId` field in `textDocument/didOpen` for any file routed to a user-configured server equals that server's configured `language_id`, regardless of file extension. +16. Shutting down the LSP system (e.g. on Warp quit) sends `shutdown` then `exit` to all running custom servers and waits up to 1s for graceful exit before issuing an explicit `kill()` + `wait()`. ## Open questions -- **Should `command` support `~` and `$VAR` expansion?** Cmd-O / `/open-file` use `shellexpand::tilde`; consistent behavior here would be friendly. Recommend yes for `~`, defer `$VAR` to a follow-up. -- **Should we ship example configs?** A `docs/custom-lsp-examples.md` with intelephense / lua-language-server / zls / bash-language-server entries would shorten time-to-first-success. Recommend yes; not part of the core feature gate. -- **Schema-fetching restriction:** The JSON LSP work in #9568 found that VS Code's JSON server fetches remote schemas by default. Should the spec mandate that custom LSPs run with `network_access = false` by default? This is hard to enforce generically (each server has its own config keys for schema fetching). Recommend punting to per-server `initialization_options` and documenting the pattern. +- **Should we ship example configs?** A `docs/custom-lsp-examples.md` with intelephense / lua-language-server / zls / bash-language-server entries would shorten time-to-first-success. Recommend yes; not part of the core feature gate, but in scope for the same release. +- **Settings UI placement specifics.** This spec says "follow Settings → Code → Indexing and Projects pattern." Confirming the exact row layout (one row per server with workspace sub-rows, vs one row per (server, workspace) tuple) is an implementation-time decision aligned with the existing pattern. diff --git a/specs/GH8803/tech.md b/specs/GH8803/tech.md index 7fea53500..2f058fbb0 100644 --- a/specs/GH8803/tech.md +++ b/specs/GH8803/tech.md @@ -10,7 +10,7 @@ Warp's LSP layer currently treats every language server as a closed-set enum var - An entry in `LanguageId` (`crates/lsp/src/config.rs:25-36`) that maps file extensions to language identifiers. - A `LanguageId::lsp_language_identifier` arm and a `LanguageId::from_path` arm. -Adding a new language requires touching all four sites. PRs #9562 (PHP Intelephense) and #9568 (JSON via vscode-json-languageserver) demonstrated this pattern but were closed by maintainer @kevinyang372 in favor of this user-configurable path. The infrastructure those PRs built (probe-spawn install detection, executable-bit checks, cross-platform PATH handling, `INSTALL_PROBE_TIMEOUT` via `warpui::r#async::FutureExt::with_timeout`) is reusable here. +Adding a new language requires touching all four sites. PRs #9562 (PHP Intelephense) and #9568 (JSON via vscode-json-languageserver) demonstrated this pattern but were closed by maintainer @kevinyang372 in favor of this user-configurable path. The infrastructure those PRs built (probe-spawn install detection, executable-bit checks, cross-platform PATH handling, bounded-future timeout via `warpui::r#async::FutureExt::with_timeout`) is reusable here. ### Relevant code @@ -18,67 +18,97 @@ Adding a new language requires touching all four sites. PRs #9562 (PHP Intelephe |---|---| | `crates/lsp/src/language_server_candidate.rs` | The trait every server impl satisfies. The natural extension point — a new impl, `UserConfiguredLanguageServer`, will go here. | | `crates/lsp/src/supported_servers.rs` | `LSPServerType` enum + the closed registry of impls. Will grow a new arm carrying user config. | -| `crates/lsp/src/config.rs` | `LanguageId` enum + `from_path` extension mapping + `lsp_language_identifier`. Needs a path that bypasses the enum for user-configured types. | -| `crates/lsp/src/manager.rs` | Spawns/owns running LSP processes. The new code lives here for per-workspace lifecycle. | +| `crates/lsp/src/config.rs` | `LanguageId` enum + `from_path` extension mapping + `lsp_language_identifier`. Bypassed for user-configured servers (the user supplies `language_id` directly). | +| `crates/lsp/src/manager.rs` | Spawns/owns running LSP processes. New per-workspace lifecycle logic lives here. | | `app/src/settings/` | Settings group definitions (see `app/src/settings/input.rs` for the macro pattern). Where `[lsp.servers]` parsing lands. | | `app/src/code/editor/` | Editor footer rendering — where the "Enable `` for this workspace?" chip surfaces. | +| `app/src/settings/code/` (or current Settings → Code → Indexing/Projects host) | Where the workspace-enablement toggle row is rendered alongside the existing per-workspace project list. | ### Related closed PRs (input to this spec) - #9562 — PHP Intelephense as built-in. Closed; lessons: probe-spawn `--stdio` with bounded timeout, executable-bit check on Unix, executable's full PATH search via `binary_in_path` helper, cross-platform tests via `std::env::join_paths`. -- #9568 — JSON via vscode-json-languageserver. Closed; lessons: schema-fetching is a security surface (the JSON server defaults to fetching `http`/`https`/`file` schema URIs); `initializationOptions` is the right place for per-server restrictions like `handledSchemaProtocols`. +- #9568 — JSON via vscode-json-languageserver. Closed; lessons: schema-fetching is a security surface; this is documented in user-facing docs but the V0 spec defers `initializationOptions` forwarding. -## Proposed changes +## Crate boundaries -### 1. New settings group: `LspSettings` +The user-config type is shared between the `app/` settings layer (which parses TOML into the type) and the `crates/lsp` layer (which constructs `LanguageServerCandidate` implementations from it). `crates/lsp` cannot depend on `app/` (the dependency direction is `app → crates/lsp`, not the reverse). -**File:** new `app/src/settings/lsp.rs`. Pattern matches `app/src/settings/input.rs` (`define_settings_group!` macro). +**Resolution: define the type in `crates/lsp/src/user_config.rs`** and `pub use` it from both layers. `app/src/settings/lsp.rs` imports `warp_lsp::user_config::UserLspServerConfig` and uses it as the field type on the settings group; `crates/lsp` constructs `UserConfiguredLanguageServer` from instances of this same type. This keeps the type in the lower-level crate (where the `LanguageServerCandidate` impl that consumes it lives) and avoids creating a new shared crate just for one struct. Verify the exact module name (`warp_lsp` vs `lsp`) at implementation time against `crates/lsp/Cargo.toml`. + +## Proposed changes -The group holds a single setting, `custom_servers`, of type `Vec`. The struct: +### 1. New shared type: `UserLspServerConfig` + +**File:** new `crates/lsp/src/user_config.rs`. Exported from `crates/lsp/src/lib.rs` so `app/` can use it. ```rust -#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] pub struct UserLspServerConfig { pub name: String, pub command: Vec, pub file_types: Vec, - #[serde(default)] - pub root_files: Vec, - #[serde(default)] - pub initialization_options: Option, - #[serde(default = "default_start_timeout_ms")] - pub start_timeout_ms: u64, + pub language_id: String, } -fn default_start_timeout_ms() -> u64 { 5000 } +impl UserLspServerConfig { + /// Stable identity for change-detection on settings reload. + /// Hashing only `command` + args means metadata-only edits + /// (e.g. `language_id` change) do NOT trigger re-confirmation. + pub fn command_fingerprint(&self) -> u64 { ... } +} ``` +V0 deliberately omits `root_files` (use Warp's existing root-repo detection), `initialization_options` (no settings shape for arbitrary nested config yet), and `start_timeout_ms` (fixed 5s default in code). These are tracked in Follow-ups. + +### 2. New settings group: `LspSettings` + +**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` (the type is imported from `crates/lsp`, not redefined in `app/`). + +**Serde rename mapping:** the user-facing TOML key is `[[lsp.servers]]`, but the Rust field on the settings group is `custom_servers`. The mapping is wired with `#[serde(rename = "servers")]` on the field so the on-disk schema reads naturally: + +```toml +[[lsp.servers]] +name = "intelephense" +command = ["intelephense", "--stdio"] +file_types = ["php", "phtml"] +language_id = "php" +``` + +The Rust field is named `custom_servers` to disambiguate from any built-in registry while keeping the user-facing TOML clean. Document this rename in the field doc-comment so contributors searching for `[[lsp.servers]]` find it. + Validation runs at parse time (in a `validate()` method called from the settings init path): - `name` non-empty and unique across the vec. - `command` non-empty. - `file_types` non-empty; each entry stripped of leading `.`. -- `start_timeout_ms` ≥ 100 and ≤ 60_000. +- `language_id` non-empty. +- **Cross-entry validation:** the union of all entries' `file_types` must contain no duplicates. If two entries both list `"php"`, the entire `[lsp]` block is rejected with an error pointing at both offending entries. -Validation failures emit a settings-error notification (existing pattern in `app/src/settings/initializer.rs`) with the offending entry index. Other entries continue to load. +Validation failures emit a settings-error notification (existing pattern in `app/src/settings/initializer.rs`) with the offending entry index. Other entries continue to load only when the failure is per-entry; cross-entry conflicts disable all custom LSPs until resolved. -### 2. Per-workspace enablement state +### 3. Per-workspace enablement state -**File:** new fields on the existing per-workspace settings struct (`app/src/workspace/state.rs` or similar — exact location depends on where workspace-scoped state lives; verify against current code at implementation time). +**File:** new fields on the existing per-workspace settings struct (verify exact location at implementation time — likely `app/src/workspace/state.rs` or equivalent). ```rust pub struct WorkspaceLspState { - /// Set of `UserLspServerConfig.name` values the user explicitly enabled - /// for this workspace. Survives Warp restart via the existing workspace - /// state persistence path. - enabled_custom_servers: HashSet, - /// Set of `UserLspServerConfig.name` values dismissed via the chip. - /// Suppresses the chip until cleared from settings UI. + /// Map of `UserLspServerConfig.name` → command fingerprint at the time + /// of last user confirmation. If the live config's fingerprint differs, + /// the server is treated as "needs re-confirmation". + enabled_custom_servers: HashMap, + /// Names dismissed via the chip. Suppresses the chip until cleared + /// via Settings UI. dismissed_custom_servers: HashSet, } ``` -### 3. New `LanguageServerCandidate` impl +The fingerprint-keyed map is what enforces invariant 11 (re-confirmation on `command` change): an enabled entry whose live fingerprint no longer matches its stored fingerprint is silently demoted to "not enabled, but eligible to chip" — the user sees the chip again with a "command changed" affordance and must explicitly re-enable. + +`#[serde(default)]` on both fields handles migration from existing workspaces that have no custom-LSP state. + +### 4. New `LanguageServerCandidate` impl **File:** new `crates/lsp/src/servers/user_configured.rs`. @@ -91,11 +121,10 @@ pub struct UserConfiguredLanguageServer { #[async_trait] impl LanguageServerCandidate for UserConfiguredLanguageServer { async fn should_suggest_for_repo(&self, path: &Path, _executor: &CommandBuilder) -> bool { - // Heuristic: any file in `path` matches one of `file_types`, - // OR (if `root_files` is non-empty) one of those globs is present - // in `path` or any ancestor up to the workspace root. - // Implementation-wise: glob over the immediate dir for `file_types`, - // walk parents for `root_files`. + // Heuristic: any file in `path` matches one of `file_types`. + // Root detection itself uses Warp's existing root-repo logic + // (per @kevinyang372 review), so this method does NOT walk + // for `root_files` — that field is V0-deferred. ... } @@ -105,18 +134,15 @@ impl LanguageServerCandidate for UserConfiguredLanguageServer { } async fn is_installed_on_path(&self, executor: &CommandBuilder) -> bool { - // Reuse the patterns from #9562's probe: - // 1. `binary_in_path` filesystem check for `command[0]` (with executable-bit check on Unix) - // 2. NO probe-spawn — that contract was specific to npm-installed servers - // that may have stale shims. For user-configured servers we trust - // the user; an unhealthy spawn is surfaced via the start-time - // error toast instead. + // Reuse `binary_in_path` from #9562's infrastructure (filesystem + // check for `command[0]` with executable-bit check on Unix). + // No probe-spawn — that contract was specific to npm-installed + // servers that may have stale shims. For user-configured servers + // an unhealthy spawn surfaces via the start-time error toast. binary_in_path(&self.config.command[0], executor.path_env_var()) } async fn install(&self, _: LanguageServerMetadata, _: &CommandBuilder) -> anyhow::Result<()> { - // User-configured servers are never installed by Warp. The user owns - // the lifecycle. Surface a clear error if anything tries to call this. anyhow::bail!("user-configured LSP `{}` is not installable by Warp", self.config.name) } @@ -126,9 +152,9 @@ impl LanguageServerCandidate for UserConfiguredLanguageServer { } ``` -### 4. `LSPServerType` extension +### 5. `LSPServerType` extension and built-in suppression -**File:** `crates/lsp/src/supported_servers.rs`. +**File:** `crates/lsp/src/supported_servers.rs` and `crates/lsp/src/manager.rs`. Add a variant: @@ -139,73 +165,102 @@ pub enum LSPServerType { } ``` -The construction site that walks built-in servers becomes: +The candidate-construction site enforces the **overwrite** semantic per @kevinyang372's review (user-configured servers replace built-ins for matched file types in the workspace where they are enabled): ```rust fn all_candidates_for(workspace: &Workspace, settings: &LspSettings) -> Vec> { - let mut out = built_in_candidates(); - for cfg in &settings.custom_servers.0 { - if workspace.lsp_state.enabled_custom_servers.contains(&cfg.name) { + let mut suppressed_extensions: HashSet<&str> = HashSet::new(); + let mut out = Vec::new(); + + for cfg in &settings.custom_servers { + let live_fp = cfg.command_fingerprint(); + let confirmed_fp = workspace.lsp_state.enabled_custom_servers.get(&cfg.name).copied(); + if confirmed_fp == Some(live_fp) { + // Enabled AND command unchanged since last user confirmation. + for ext in &cfg.file_types { + suppressed_extensions.insert(ext.as_str()); + } out.push(Box::new(UserConfiguredLanguageServer::new(cfg.clone(), workspace.root.clone()))); } + // else: not enabled, dismissed, or pending re-confirmation — chip flow handles surface. } + + for builtin in built_in_candidates() { + if builtin.handled_extensions().iter().any(|e| suppressed_extensions.contains(e.as_str())) { + continue; // user-configured server overrides this built-in for this workspace + } + out.push(builtin); + } + out } ``` -### 5. Bypassing `LanguageId` for custom file types +The built-in candidate trait is extended with a `handled_extensions(&self) -> Vec` method (or equivalent) so the suppression filter knows which built-ins to skip. This is a small additive change to the existing trait surface. -The `LanguageId` enum is used to populate the `languageId` field in LSP `textDocument/didOpen`. For user-configured servers, the user provides `file_types` directly. Two implementation options: +### 6. Language ID handling -**Option A — extend the enum with a `Custom(String)` variant.** The `Custom(String)` carries the file extension and `lsp_language_identifier()` returns it as-is. +The user supplies `language_id` directly in the config; we send it verbatim in `textDocument/didOpen.languageId` for any file matching `file_types`. The closed `LanguageId` enum in `crates/lsp/src/config.rs` is **not** extended — it continues to handle built-in servers only. -**Option B — keep the enum closed and add a parallel `LspLanguageId` type that's either `Builtin(LanguageId)` or `Custom { extension: String, identifier: String }`.** +The dispatch site (whatever currently calls `LanguageId::from_path(...).lsp_language_identifier()` to populate `didOpen`) gets a small branch: if the active candidate for this open is a `UserConfiguredLanguageServer`, use its `config.language_id`; otherwise use the existing closed-enum path. This avoids polluting `LanguageId` with a `Custom(String)` variant and the cascading match-arm work that would create. -**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. +This design directly addresses the oz-for-oss feedback that file extension is insufficient (`sh` → `shellscript`, `phtml` → `php`): the user states the canonical languageId once per server, and Warp forwards it. -### 6. Footer chip +### 7. Footer chip **File:** `app/src/code/editor/` — extending whatever surface renders the LSP-related chip when a built-in server is detected-but-not-installed (find via `grep -rn "Install" app/src/code/editor/`). -The chip rendering branches on three states: +The chip rendering branches on these states for user-configured servers: -1. Built-in server detected, not installed → existing "Install ``" chip. -2. Custom server defined, not enabled in workspace, not dismissed → new "Enable ``" chip. -3. Custom server defined, but `command[0]` not on PATH → "Configure ``" chip with `Open settings` action (variant of state 2). +1. Server defined, not enabled in workspace, not dismissed → "Enable ``" chip. +2. Server enabled but live `command_fingerprint` ≠ stored fingerprint → "`` command changed — re-enable?" chip. +3. Server defined, but `command[0]` not on PATH at chip-render time → "Configure ``" chip with `Open settings` action. Click handlers: -- `Enable` → mutate `WorkspaceLspState.enabled_custom_servers`, persist, trigger `manager::start_for_workspace` for the new candidate. +- `Enable` / `Re-enable` → record live `command_fingerprint` in `WorkspaceLspState.enabled_custom_servers`, persist, trigger `manager::start_for_workspace`. The same code path handles initial enable and re-confirmation. - `Dismiss` → mutate `dismissed_custom_servers`, persist, no spawn. - `Open settings` → existing settings-open-with-search action, scoped to `lsp.servers`. -### 7. Settings reload handling +### 8. Settings UI (in scope for V0) + +**File:** the existing **Settings → Code → Indexing and Projects** view (locate via `grep -rn "Indexing" app/src/settings/`). Per @kevinyang372's review, no new "Code Intelligence" section is created; we extend the existing pattern. + +For each enabled user-configured server in the active workspace, render a row beneath the existing per-workspace project list with: + +- The server `name` +- Current state: `Enabled` / `Pending re-confirmation` +- A `Disable` button that writes to `WorkspaceLspState`, fires the manager shutdown for this workspace, and restores any built-in server for the relevant file types within 1s. + +This is the surface that satisfies invariants 11, 12, and 14 (chip can re-appear after dismiss is cleared via Settings UI). Without this row, those invariants are unreachable, which is why oz-for-oss correctly flagged the prior "follow-up" framing as critical. + +### 9. Settings reload handling **File:** `app/src/settings/initializer.rs` — extend the existing reload path. -On settings change: +On settings change, the diff is computed against the previous `[lsp.servers]` list keyed by `name`: -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. +1. **Added entries:** nothing immediate; chip will appear on next matching file open. +2. **Removed entries:** shut down any running instance for that name in any workspace; clear from `enabled_custom_servers` and `dismissed_custom_servers` everywhere. +3. **Changed entries — metadata only** (`language_id` or `file_types` changes, but `command` and args byte-equal old): if currently running, restart in place with new metadata. Stored fingerprint is unchanged so no re-confirmation prompt fires. +4. **Changed entries — `command` or args differ**: shut down any running instance, **leave the `name → fingerprint` entry in `enabled_custom_servers` unchanged**. The candidate-construction filter above will treat the entry as "pending re-confirmation" because live ≠ stored fingerprint. The chip flow surfaces the re-enable affordance. -The diff mechanism reuses existing settings-event hooks; no new infra needed. +This addresses the oz-for-oss security finding directly: a settings-file edit that mutates `command` cannot run a new binary in a workspace that previously trusted a different binary, even if no Warp restart occurs. -### 8. Lifecycle: spawn, initialize, timeout, shutdown +### 10. Lifecycle: spawn, initialize, timeout, shutdown **File:** `crates/lsp/src/manager.rs`. -The existing `start_for_workspace` (or whatever the entry-point is named — verify) spawns built-in servers. Extending it for user-configured servers means: - - Build the `Command` from `config.command[0]` + `config.command[1..]`. - `stdin/stdout` piped, `stderr` captured to a per-server ring buffer (last 200 bytes for error toasts). -- 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`): +- Use **`tokio::process::Command`** with `kill_on_drop(true)` so any panic / unwinding path between spawn and `initialize` reaps the child. This is portable (Tokio handles the platform differences) — `std::process::Child::drop` is documented to **not** kill on Unix and is therefore unsuitable as a kill guarantee. +- Wrap the `initialize` request in `warpui::r#async::FutureExt::with_timeout(Duration::from_millis(5000))`. 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. -- Forward `config.initialization_options` (TOML → `serde_json::Value`) into `InitializeParams.initialization_options` on the request. -- On Warp shutdown: existing `shutdown` + `exit` flow already handles all running servers; user-configured servers ride the same path. + - `Ok(Err(err))` — JSON-RPC error; surface notification with err message; `child.kill().await; child.wait().await;`. + - `Err(timeout)` — explicitly `child.kill().await; child.wait().await;` (don't rely on `Drop` alone), then surface timeout notification. +- On Warp shutdown: existing `shutdown` + `exit` flow already handles all running servers; user-configured servers ride the same path. After the 1s graceful window, fall through to `child.kill().await; child.wait().await;`. + +The explicit `kill().await; wait().await;` pair is the answer to the oz-for-oss portability concern: it works on every platform Tokio supports and avoids leaving zombies on Unix. ## Testing and validation @@ -213,69 +268,86 @@ Each invariant from `product.md` maps to a test at this layer: | Invariant | Test layer | File | |---|---|---| -| 1, 2 (parse / validate) | unit | `app/src/settings/lsp_tests.rs` (new) — TOML strings → `LspSettings` parse outcomes (success cases + each validation error). | -| 4, 5, 6, 7 (spawn outcomes) | unit | `crates/lsp/src/servers/user_configured_tests.rs` (new) — mock `CommandBuilder` returning success / error / hang. | -| 4 (`initialization_options` forwarded) | unit | same file — assert the `InitializeParams` JSON contains the configured options byte-equivalent. | -| 7 (timeout via `with_timeout`) | unit | same file — wire a future that never resolves, assert the timeout branch fires within `start_timeout_ms + 100`. | -| 9 (settings reload restarts running servers) | unit | `crates/lsp/src/manager_tests.rs` extension — pre-populate a running server, swap config, assert restart. | -| 10 (built-in + user-configured coexist) | integration | `crates/integration/tests/` — open a `.py` file in a workspace where both pyright (built-in) and a user-configured "ruff" server match; assert both are in the active candidate list. | -| 12 (per-workspace enablement persists across restart) | unit | workspace-state serialization round-trip. | -| 3, 8, 11, 14 (chip behavior) | integration | UI integration test under `crates/integration/`. Stub the file-open event, assert chip presence/absence based on enablement+dismissal state. | -| 13 (TOML→JSON shape preservation) | unit | parse a TOML with nested table + array, assert resulting `serde_json::Value` is structurally equivalent. | -| 15 (graceful shutdown on quit) | unit | shutdown handler test — running user-configured server receives `shutdown` then `exit` then has 1s window before SIGKILL. | +| 1, 2 (parse / validate) | unit | `app/src/settings/lsp_tests.rs` (new) — TOML strings → `LspSettings` parse outcomes (success cases + each validation error, including cross-entry duplicate `file_types`). | +| 4, 5, 6, 7 (spawn outcomes) | unit | `crates/lsp/src/servers/user_configured_tests.rs` (new) — mock `CommandBuilder` returning success / error / hang. Assert explicit `kill().await; wait().await;` on the timeout branch. | +| 7 (timeout via `with_timeout` + explicit kill) | unit | same file — wire a future that never resolves, assert the timeout branch fires within `5000ms + 100`, and that `kill` was observed before `wait` returned. | +| 8, 9 (overwrite semantics: built-in suppressed) | unit | `crates/lsp/src/manager_tests.rs` extension — register a built-in candidate handling `php`, register an enabled user-configured entry handling `php`, assert the built-in is NOT in `all_candidates_for(workspace)`. Then disable the user entry and assert the built-in returns. | +| 10 (metadata-only reload restarts in place) | unit | manager test — pre-populate running server, swap `language_id`, assert restart with no chip-flow trigger. | +| 11 (command change → re-confirmation) | unit | manager + workspace-state test — pre-populate running server with stored fingerprint F, swap `command`, assert process is killed AND `enabled_custom_servers[name]` still equals F (not deleted), AND chip flow now reports "pending re-confirmation". | +| 12 (Settings UI disable) | integration | UI integration test — toggle disable in the Settings → Code row, assert process shutdown within 1s, assert built-in resumes. | +| 13 (per-workspace enablement persists across restart) | unit | workspace-state serialization round-trip with the fingerprint map. | +| 3, 8, 14 (chip behavior) | integration | UI integration test under `crates/integration/`. Stub the file-open event, assert chip presence/absence based on enablement+dismissal state. | +| 15 (`languageId` forwarding) | unit | parse a config with `language_id = "shellscript"` and `file_types = ["sh"]`, simulate `didOpen` for `foo.sh`, assert outgoing JSON-RPC contains `"languageId":"shellscript"`. | +| 16 (graceful shutdown on quit) | unit | shutdown handler test — running user-configured server receives `shutdown` then `exit` then has 1s window before explicit `kill().await; wait().await;`. | ### Cross-platform constraints (lessons from #9562/#9568) - Tests building `PATH` strings must use `std::env::join_paths`, not `:`. Reuse the `make_path_var` helper introduced in #9562's tests. - On Windows, `command[0]` may need `.exe` / `.cmd` resolution. Reuse `binary_filename` helper (also from #9562's tests). -- `std::process::Stdio::null()` for stdin during `is_installed_on_path` check is a no-op for user-configured servers in this design (we don't probe-spawn), but if we ever do, the same pattern from `intelephense.rs:113` applies. +- `tokio::process::Command::kill_on_drop(true)` is portable; do not substitute `std::process::Child::drop` — it does not kill on Unix. ## End-to-end flow ``` User edits settings.toml └─> [LspSettings::reload] (settings/lsp.rs) - ├─> validate() (rejects malformed) + ├─> validate() (rejects malformed; rejects cross-entry duplicate file_types) └─> emit SettingsChanged event └─> [manager::on_settings_change] (lsp/src/manager.rs) - ├─> diff old vs new custom_servers - ├─> stop removed entries - └─> restart changed entries that are running + ├─> diff old vs new custom_servers (by name) + ├─> stop removed entries everywhere + ├─> metadata-only changes → restart in place + └─> command/args changes → stop running instance, + KEEP stored fingerprint (so candidate filter + treats as "pending re-confirmation"; chip + will surface) User opens .lua file in workspace W └─> [editor::on_file_open] (app/src/code/editor/) └─> [chip_renderer] ├─> get LspSettings.custom_servers ├─> filter where file_types contains "lua" - ├─> filter where W.enabled_custom_servers does NOT contain name - ├─> filter where W.dismissed_custom_servers does NOT contain name - └─> render "Enable " chip per remaining entry - -User clicks Enable + ├─> for each: classify state + │ ├─> enabled & fingerprint match → no chip + │ ├─> enabled & fingerprint mismatch → "command changed — re-enable?" + │ ├─> not enabled & not dismissed → "Enable " + │ ├─> dismissed → no chip + │ └─> binary not on PATH → "Configure " + └─> render appropriate chip per remaining entry + +User clicks Enable / Re-enable └─> [chip_handler::on_enable_clicked] - ├─> W.enabled_custom_servers.insert(name) + ├─> live_fp = config.command_fingerprint() + ├─> W.enabled_custom_servers[name] = live_fp ├─> persist W └─> [manager::start_candidate] (lsp/src/manager.rs) ├─> construct UserConfiguredLanguageServer (servers/user_configured.rs) + ├─> all_candidates_for(W) now suppresses any built-in + │ handling the same file_types in W ├─> is_installed_on_path → if false, surface "Configure " toast - ├─> spawn Command with stderr-buffered - ├─> initialize.with_timeout(start_timeout_ms) + ├─> tokio::Command, kill_on_drop(true), stderr→ring buffer + ├─> initialize.with_timeout(5000ms) │ ├─> Ok(Ok(_)) → register, route LSP traffic - │ ├─> Ok(Err(err)) → surface error toast, drop child - │ └─> Err(_timeout) → kill child via Drop, surface timeout toast - └─> on subsequent file opens, no chip — server already running + │ ├─> Ok(Err(err)) → child.kill().await; child.wait().await; + │ │ surface error toast + │ └─> Err(_timeout) → child.kill().await; child.wait().await; + │ surface timeout toast + └─> on subsequent file opens, no chip — server already running, + built-in for same file_types stays suppressed in W ``` ## Risks -- **`initialization_options` is a security surface.** Some servers (e.g. JSON via vscode-json-languageserver, see #9568) default to fetching remote schemas. The user controls this knob, but they may not realize it. **Mitigation:** ship `docs/custom-lsp-examples.md` with annotated examples that explicitly set network-related options to safe defaults where applicable. -- **A configured server that crashes on every spawn could loop forever.** **Mitigation:** the chip's `Enable` action is one-shot per click. We do not auto-restart on crash; the user re-enables manually. If exit happens after `initialize` succeeded, we surface a "server crashed" toast and the chip returns to the disabled-but-defined state. -- **`Custom(String)` propagation in `LanguageId`.** Adding a `Custom` variant to a closed exhaustive enum touches every match site. **Mitigation:** WARP.md prohibits wildcard matches, so the compiler will surface every site at code-write time. Audit during implementation. -- **Per-workspace state migration.** Existing workspaces have no `enabled_custom_servers` field. **Mitigation:** `#[serde(default)]` on the new fields; absence parses as empty set. +- **Built-in suppression in W is invisible until the user notices missing diagnostics.** Mitigation: the Settings → Code row makes the active user-configured server visible per workspace; a contributor doc note explains that enabling a custom server replaces the built-in for matched file types. +- **A configured server that crashes on every spawn could loop forever.** Mitigation: the chip's `Enable` action is one-shot per click. We do not auto-restart on crash; the user re-enables manually. If exit happens after `initialize` succeeded, we surface a "server crashed" toast and the chip returns to the disabled-but-defined state. +- **Per-workspace state migration.** Existing workspaces have no `enabled_custom_servers` field. Mitigation: `#[serde(default)]` on the new fields; absence parses as empty map / set. +- **Fingerprint stability across Warp versions.** If the fingerprint algorithm changes between releases, every enabled server would silently demote to "pending re-confirmation" on first launch of the new version. Mitigation: define `command_fingerprint` as a stable hash of `command` (vec of bytes) only; pin to a stable hasher (e.g. SipHash with a fixed key, not the default `DefaultHasher` which can change). ## 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). -- `~` and `$VAR` expansion in `command[0]`. Recommend yes for `~` (consistency with `/open-file`); defer `$VAR`. -- Document `network_access` semantics if we later add a generic per-server flag. The JSON-server-specific `handledSchemaProtocols` discovery from #9568's review is the model. +- `root_files` user-supplied glob patterns for root detection (V0 uses Warp's existing root-repo detection per @kevinyang372). +- `initialization_options` forwarding (V0-skipped per @kevinyang372: warp does not yet have settings shape for arbitrary nested config payloads). When added, the JSON-server schema-fetching pattern from #9568's review is the model for documentation. +- User-configurable `start_timeout_ms` (V0 uses fixed 5s). +- `~` and `$VAR` expansion in `command[0]` (consistency with `/open-file`). +- Documentation page `docs/custom-lsp-examples.md` with intelephense / lua-language-server / zls / bash-language-server entries. In scope for the same release as this feature, but tracked separately. From 140434e65edcab85fb52021cb2d81c7b8d54ef9b Mon Sep 17 00:00:00 2001 From: lonexreb Date: Sun, 3 May 2026 03:02:35 -0500 Subject: [PATCH 3/3] spec: fix internal inconsistencies found in self-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- specs/GH8803/product.md | 13 ++++++++----- specs/GH8803/tech.md | 31 ++++++++++++++++++------------- 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/specs/GH8803/product.md b/specs/GH8803/product.md index 8ffe6169c..35c223c17 100644 --- a/specs/GH8803/product.md +++ b/specs/GH8803/product.md @@ -63,12 +63,15 @@ This scales poorly and pulls language-specific install logic into the core. User 1. User opens any file matching `file_types` in a workspace where the server is already enabled. 2. The user-configured server is already running; no UI surfaces. Diagnostics, hover, goto, completions all behave as they do for built-in servers. -### Disabling a server in a workspace +### Disabling, re-enabling, and managing servers in a workspace -The disable flow piggybacks on Warp's existing **Settings → Code → Indexing and Projects** UI pattern (per @kevinyang372's direction; no new "Code intelligence" section is introduced for V0): +The disable / re-enable flow piggybacks on Warp's existing **Settings → Code → Indexing and Projects** UI pattern (per @kevinyang372's direction; no new "Code intelligence" section is introduced for V0): -1. User opens **Settings → Code**. The existing per-workspace project list grows a sub-row per user-configured LSP that is currently enabled in that workspace. -2. The row exposes a `Disable` toggle. Toggling off shuts down the server's process for that workspace within 1s, removes the per-workspace enablement record, and re-enables the built-in server for that file type if one exists. +1. User opens **Settings → Code**. The existing per-workspace project list grows a sub-row per user-configured LSP that has any per-workspace state in this workspace (enabled, dismissed, or pending re-confirmation after a `command` change). +2. The row shows the server's name and current state, with a single context-appropriate action: + - **Enabled:** `Disable` button. Shuts down the server's process for this workspace within 1s, removes the per-workspace enablement record **and any dismissal record for the same server**, and re-enables the built-in server for that file type if one exists. Clearing both records ensures the chip can re-appear on the next matching file open. + - **Dismissed:** `Re-enable` button. Clears the dismissal so the chip surfaces on the next matching file open. Does not start the server immediately — the user re-enters the chip flow. + - **Pending re-confirmation:** `Re-enable` button. Records the new (changed) `command` as confirmed by the user and starts the server. This is the same affordance the chip offers for the security-driven re-confirmation flow described below. ### Misconfiguration scenarios @@ -129,7 +132,7 @@ Numbered list — each maps to a verification path in the tech spec: 8. After a server is enabled in workspace W, opening any file matching `file_types` in W routes LSP requests to that server **without** showing the chip again, and **without** a built-in server for the same file types running concurrently in W. 9. Enabling a user-configured server in workspace W that matches a file type also handled by a built-in shuts down the built-in for W within 1s; disabling the user-configured server restores the built-in for W within 1s. 10. After settings change to a server entry's metadata fields only (e.g. `language_id`, `file_types` ordering), an enabled-and-running server is restarted with the new metadata within 1s of settings reload, with no Warp restart and no re-confirmation prompt required. -11. After settings change to an enabled server's `command` or args, the process is shut down, enablement moves to "needs re-confirmation", and the chip reappears on next matching file open until the user re-confirms. +11. After settings change to an enabled server's `command` or args, the process is shut down, enablement moves to "needs re-confirmation", and the "command changed — re-enable?" chip reappears on next matching file open until the user re-confirms. This chip surfaces **even if the server was previously dismissed in that workspace**: a `command` change is a security signal and overrides dismissal. 12. Disabling a server via Settings UI shuts down its process for the targeted workspace within 1s, removes the per-workspace enablement record, restores the built-in for the file type if any, and the chip reappears on next file open. 13. Restarting Warp preserves per-workspace enablement state — workspaces where a server was enabled before restart auto-spawn the user-configured server on the next file open without the chip reappearing, and the built-in for the same file types is not spawned. 14. The chip is **not** shown for a configured server in a workspace that has explicitly dismissed it; the user must re-enable from Settings UI. diff --git a/specs/GH8803/tech.md b/specs/GH8803/tech.md index 2f058fbb0..9ecbe5473 100644 --- a/specs/GH8803/tech.md +++ b/specs/GH8803/tech.md @@ -210,11 +210,13 @@ This design directly addresses the oz-for-oss feedback that file extension is in **File:** `app/src/code/editor/` — extending whatever surface renders the LSP-related chip when a built-in server is detected-but-not-installed (find via `grep -rn "Install" app/src/code/editor/`). -The chip rendering branches on these states for user-configured servers: +The chip rendering checks these states in cascade order (first match wins). Earlier states take precedence — in particular, the command-changed chip (state 2) overrides dismissal because it represents a security signal that the binary the user previously trusted has changed. -1. Server defined, not enabled in workspace, not dismissed → "Enable ``" chip. -2. Server enabled but live `command_fingerprint` ≠ stored fingerprint → "`` command changed — re-enable?" chip. +1. Server enabled (stored fingerprint exists) AND live `command_fingerprint` ≠ stored fingerprint → "`` command changed — re-enable?" chip. **Surfaces even if user previously dismissed.** +2. Server dismissed in this workspace → no chip (silenced). 3. Server defined, but `command[0]` not on PATH at chip-render time → "Configure ``" chip with `Open settings` action. +4. Server defined, not enabled, not dismissed, binary on PATH → "Enable ``" chip. +5. Server enabled and fingerprint matches → no chip (server already running). Click handlers: @@ -226,13 +228,15 @@ Click handlers: **File:** the existing **Settings → Code → Indexing and Projects** view (locate via `grep -rn "Indexing" app/src/settings/`). Per @kevinyang372's review, no new "Code Intelligence" section is created; we extend the existing pattern. -For each enabled user-configured server in the active workspace, render a row beneath the existing per-workspace project list with: +For each user-configured server that has any per-workspace state in the active workspace — i.e. its `name` appears in `enabled_custom_servers`, `dismissed_custom_servers`, OR has a stored fingerprint that no longer matches live config (pending re-confirmation) — render one row beneath the existing per-workspace project list. Each row shows the server `name`, its current state, and a single context-appropriate action: -- The server `name` -- Current state: `Enabled` / `Pending re-confirmation` -- A `Disable` button that writes to `WorkspaceLspState`, fires the manager shutdown for this workspace, and restores any built-in server for the relevant file types within 1s. +| State | Predicate | Action button | Behavior on click | +|---|---|---|---| +| Enabled | `enabled_custom_servers[name]` exists AND equals live `command_fingerprint()` | `Disable` | Removes BOTH `enabled_custom_servers[name]` AND any `dismissed_custom_servers` entry for `name`, fires manager shutdown for this workspace within 1s, restores any built-in server for the relevant `file_types`. Clearing both fields is what makes invariant 12 ("chip reappears on next file open") hold unconditionally. | +| Pending re-confirmation | `enabled_custom_servers[name]` exists but stored fingerprint ≠ live `command_fingerprint()` | `Re-enable` | Records live fingerprint into `enabled_custom_servers[name]` and triggers `manager::start_for_workspace`. Same code path as the chip's re-enable click. | +| Dismissed | `dismissed_custom_servers` contains `name` (and not in either of the above states) | `Re-enable` | Clears `dismissed_custom_servers[name]`. Does not spawn — the user re-enters the chip flow on next matching file open. | -This is the surface that satisfies invariants 11, 12, and 14 (chip can re-appear after dismiss is cleared via Settings UI). Without this row, those invariants are unreachable, which is why oz-for-oss correctly flagged the prior "follow-up" framing as critical. +This is the surface that satisfies invariants 11, 12, and 14. Without rows for dismissed and pending-re-confirmation entries, those invariants would be unreachable from the UI, which is why oz-for-oss correctly flagged the prior "follow-up" framing as critical. ### 9. Settings reload handling @@ -307,12 +311,13 @@ User opens .lua file in workspace W └─> [chip_renderer] ├─> get LspSettings.custom_servers ├─> filter where file_types contains "lua" - ├─> for each: classify state - │ ├─> enabled & fingerprint match → no chip - │ ├─> enabled & fingerprint mismatch → "command changed — re-enable?" - │ ├─> not enabled & not dismissed → "Enable " + ├─> for each: classify state (cascade; first match wins) + │ ├─> enabled & live fp ≠ stored fp → "command changed — re-enable?" + │ │ (security override — surfaces even if dismissed) │ ├─> dismissed → no chip - │ └─> binary not on PATH → "Configure " + │ ├─> binary not on PATH → "Configure " + │ ├─> not enabled & binary on PATH → "Enable " + │ └─> enabled & fp match → no chip (already running) └─> render appropriate chip per remaining entry User clicks Enable / Re-enable