Better mcp series - #18
Open
the-real-wizard-of-oz wants to merge 40 commits into
Open
the-real-wizard-of-oz wants to merge 40 commits into
the-real-wizard-of-oz wants to merge 40 commits into
Conversation
Replace serial &mut self plumbing with Arc<Self> + spawn-per-request so MCP tool calls run in parallel; clients use interior mutability throughout. Introduce a typed LspError enum so transport, timeout, and cancellation failures propagate distinctly from "no result at this position" (which hover/definition/references/completion/symbols still surface as null for backward compatibility). Drop dead diagnostics_test/simple_error magic strings, eliminate handler boilerplate via a generic with_doc helper, cache tools/list output in a OnceLock, fix the open_document race, skip disk reads for already-open files, and migrate logging from log+env_logger to tracing (with stderr writer so stdout stays clean for MCP traffic). Narrow tokio features and switch pending_requests to parking_lot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Negotiates window/workDoneProgress, replies to server-initiated requests (window/workDoneProgress/create, workspace/configuration), and tracks $/progress tokens in a per-client watch map. New helper `wait_for_progress_end(token, timeout)` is level-triggered so callers that arrive after `end` return immediately. Stdin moved to Arc<Mutex> so the response loop can write replies without owning the client. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 200ms post-didOpen sleep was a guess. Replace it with a watch on rust-analyzer's `rustAnalyzer/cachePriming` $/progress token, which is the modern equivalent of the old "Indexing" signal — once it ends, hover/definition/references see resolved symbols. Best-effort: on timeout (5s) we fall through, since handlers already cope with null during indexing. Old DOCUMENT_OPEN_DELAY_MILLIS removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s, workspace_symbol New tools dispatched via the existing with_doc helper (workspace_symbol takes the no-document path since it queries by name across the whole workspace). Capabilities extended in initialize: rename with prepareSupport, signatureHelp with parameter info, inlayHint, and workspace.symbol. Also harden handle_diagnostics: rust-analyzer cancels in-flight pull diagnostics while it re-indexes, which used to be masked by the now-gone 200ms post-didOpen sleep. Swallow transient errors inside the polling loop so the next iteration retries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A monitor task spawned at start() polls Child::try_wait() every 500ms. When rust-analyzer exits, it flips a `process_died` flag on the client and drains every pending oneshot::Sender with LspError::ProcessDied so in-flight callers fail fast instead of timing out. ensure_client_started now treats `is_dead()` as a signal to drop the old client and start a fresh one in place. Restarts are rate-limited: more than MAX_RESTART_COUNT (3) crashes in RESTART_WINDOW_SECS (60) short-circuits with an error rather than spinning up another doomed process. The old client's shutdown runs in a detached task so the next request doesn't pay for cleanup latency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Each spawned tool call now registers its AbortHandle in an `in_flight` map keyed by the canonical-string form of the MCP request id. A `notifications/cancelled` notification looks up the matching handle and calls `abort()`; tasks remove themselves on completion via a small cleanup task that awaits the JoinHandle. Cancellation is coarse: the spawned tool task is killed but any LSP requests it had in flight at rust-analyzer continue to completion (their oneshot::Senders just go nowhere). Threading MCP-id → LSP-id mapping all the way through send_request would touch every handler signature and is left for a follow-up. No response is written for cancelled requests, which matches what the client already discarded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Open documents now carry per-uri version, mtime, and content_hash, so open_document_if_needed can: 1. skip work entirely when the file's mtime hasn't changed 2. emit didChange (full-text variant) on real edits, with version++ 3. fall back to didOpen for first sight mtime is the cheap freshness signal; the content hash guards against mtime-only touches that didn't change bytes (saves rust-analyzer a re-analysis pass). diagnostics for the uri are evicted on real changes so handle_diagnostics doesn't hand back stale results. open_documents migrated from HashSet<String> to HashMap<String, OpenDocState>; the workspace_diagnostics fallback updated to iterate keys. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
restart.rs: pgrep rust-analyzer under the MCP server, SIGKILL it, and verify the next tool call succeeds with a different rust-analyzer pid. Exercises the monitor → drain → restart path end-to-end. cancellation.rs: send a notifications/cancelled for both an unknown request id (must be ignored) and a real in-flight tool call (race- sensitive — the test only asserts the server stays healthy, since the exact "got cancelled vs. completed first" outcome depends on indexing state). The dedicated smoke harness covers the active-cancel path. Adds three small helpers on MCPTestClient: `server_pid`, `send_notification`, and `send_tool_call_raw` for fire-and-forget calls that don't read a response. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rust-analyzer signals "no renamable symbol at this position" via JSON-RPC code -32602 (InvalidParams) instead of returning null. Map that to Ok(null) for prepare_rename and rename so the tools behave like hover/definition/references when the position has no relevant symbol.
The IPC test daemon is shared across integration tests in the same project. test_workspace_change switched the daemon's rust-analyzer to a throwaway workspace and never switched back, so any test that ran afterwards on the same daemon couldn't find its files — test_new_lsp_tools failed in the full suite with "No references found at position" while passing in isolation. Restore the original workspace at the end of the test.
Until now `notifications/cancelled` only aborted the MCP-side spawn. The LSP requests it had in flight kept running inside rust-analyzer and only got dropped when the response eventually arrived at a closed oneshot channel — so a cancelled tool call still cost the upstream work. Wire the cancel through to rust-analyzer: - PendingTracker now indexes pending LSP requests by both LSP id and the originating MCP request id, behind a single mutex so the two views can never disagree. - `send_request` reads the current MCP id from a tokio task-local set by the request handler, so handler signatures stay unchanged. - `cancel_mcp` resolves every affected sender with LspError::Cancelled and sends `$/cancelRequest` per LSP id so rust-analyzer can stop the work it was doing. - `handle_cancellation` calls `cancel_mcp` before aborting the spawn, ensuring the LSP-side cancel goes out while the tracker still knows the ids. Adds three PendingTracker unit tests covering the cross-index consistency and per-MCP eviction.
The IPC test daemon picked target/release/rust-analyzer-mcp whenever it existed, even when the developer was running tests in debug mode. A stale release binary from an earlier session would silently mask debug-build changes — new tools would land in the source and the debug binary, but the daemon would spawn the old release binary and report "Unknown tool". Pick the binary that matches the daemon's own build profile (debug or release), with the other as a fallback if the preferred one hasn't been built.
LSP standard methods that were missing from the suite:
- rust_analyzer_type_definition — textDocument/typeDefinition
- rust_analyzer_implementation — textDocument/implementation
rust-analyzer LSP extensions:
- rust_analyzer_expand_macro — rust-analyzer/expandMacro
- rust_analyzer_parent_module — experimental/parentModule
- rust_analyzer_runnables — experimental/runnables
(position optional; without it returns
every runnable in the file)
- rust_analyzer_related_tests — rust-analyzer/relatedTests
- rust_analyzer_open_docs — experimental/externalDocs
All tools follow the existing handler pattern (`with_doc` for
position-based tools, `lookup_to_null` for "no symbol here / index
not ready"). typeDefinition and implementation also got their
respective entries in the initialize capability block.
Adds test_phase1_tools integration test that exercises every new
tool against test-project and accepts either a payload or null.
Hover markdown caps at 5 KB, completion items at 50 (sorted by sortText),
workspace_symbol pages at 100 with opaque cursor/next_cursor round-trip,
workspace_diagnostics paginates files (50 per page) while keeping the
workspace-wide summary intact. All four tools accept verbose=true to bypass
the default cap (still subject to a 1000-item absolute guardrail), and the
two paginated tools accept explicit limit + cursor params.
Reshaping happens in mcp::handlers via a new mcp::truncate module — the
LSP layer stays a thin passthrough. Null responses are preserved so the
indexer-not-ready signal continues to round-trip.
Output shapes are wrapper objects (e.g. { symbols, total, returned,
next_cursor? }) instead of raw LSP arrays — explicit metadata is more
useful to LLM callers than LSP-spec fidelity here.
19 truncate-module unit tests + 1 phase-2 integration test added. Full
suite: 62/62 green (was 42/42).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tree Adds resources/list and resources/read JSON-RPC methods to the server, plus the resources capability in initialize. First and only resource so far is workspace://files: a recursive file tree of the workspace root, capped at 5000 entries and 16 levels deep, with target/, .git/, node_modules/ and similar dirs filtered out. Symlinks are reported but not followed (loop guard). The walk runs sync std::fs in spawn_blocking so a large tree doesn't stall the request reader. Output is application/json text inside the standard MCP contents wrapper. Truncation surfaces as `truncated: true` on the relevant subtree (max-depth) or `stats.truncated` (max-entries). Step 2 will add workspace://crates (cargo metadata) and per-crate Cargo.toml resources; Phase 3.2 (multi-workspace) is still future work. 6 unit tests + 1 integration test added — 69/69 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends the resources catalogue with two new URI families. workspace://crates returns a reshaped cargo metadata --no-deps summary (per-package name, version, manifest path, targets, declared dependency names — the bulky fields like license, registry source, and resolution graph are dropped). workspace://crate/<name>/Cargo.toml returns the actual TOML manifest text for the named crate. resources/list now runs cargo metadata at list-time (off the request reader via spawn_blocking) so each workspace crate gets its own concrete URI. Workspaces without a Cargo.toml gracefully fall back to just the file-tree resource — no error. Path-traversal guard: per-crate URIs resolve via cargo metadata's manifest_path lookup rather than concatenating the caller-supplied name into a filesystem path, so crafted names like ../../etc/passwd just fail with "Unknown crate" instead of escaping the workspace. 5 new unit tests + extended integration test (cover crates summary, per-crate manifest read, unknown name, traversal block). 74/74 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the single-workspace state on RustAnalyzerMCPServer with a
WorkspaceRegistry that owns one WorkspaceEntry per registered root. Each
entry has its own rust-analyzer subprocess (lazy), its own
restart_history rate-limit window, its own open-document cache, and a
stable opaque id (ws-1, ws-2, ...). The first inserted entry stays the
default — every tool call without an explicit workspace_id resolves
there.
Three new tools manage the registry:
- rust_analyzer_add_workspace(path) → { workspace_id, root }
(boots the client eagerly so the next call doesn't pay the cold start)
- rust_analyzer_remove_workspace(workspace_id)
- rust_analyzer_list_workspaces() → { workspaces: [{id, root, default}] }
Every workspace-scoped tool now accepts an optional workspace_id
argument. Schemas are augmented automatically via inject_workspace_id at
build_tools() time so individual definitions don't have to spell it out;
the four workspace-management tools are excluded by name.
set_workspace stays backward-compatible: it replaces the default
workspace's root in place (preserving its id) and shuts down the
existing client so the next call boots rust-analyzer in the new
directory. test_workspace_change keeps passing as-is.
Cancellation goes from a single-client lookup to a broadcast across all
registered clients — each client's cancel_mcp is O(1) when the MCP id
isn't in its tracker, so the cost is negligible at typical 1–2
workspaces and avoids a second index in in_flight.
MCP resources become per-workspace:
- resources/list emits id-prefixed URIs (workspace://ws-2/files,
workspace://ws-2/crate/foo/Cargo.toml, ...). For the default
workspace it ALSO emits the unprefixed legacy aliases
(workspace://files etc.) so single-workspace clients see no change.
- resources/read parses an optional id segment after workspace://. If
the segment matches a registered id, the URI is normalized (id
stripped) and routed to that workspace; otherwise it falls back to
the default — which keeps legacy paths like
workspace://crate/foo/Cargo.toml working.
- The response URI in `contents[].uri` is rewritten back to the exact
caller-supplied URI so the round-trip is lossless even when an id
prefix was resolved.
Two new integration tests cover the end-to-end add → use → remove flow
and the multi-workspace resource routing (prefixed URI returns the
right tree, unprefixed URI stays on default, response URI round-trips).
76/76 tests green (was 74). cargo +nightly fmt and cargo clippy --lib
--bin rust-analyzer-mcp -- -D warnings: clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three more rust-analyzer custom methods, useful when the LLM is debugging a parse / lowering issue rather than navigating code: - rust_analyzer_syntax_tree (rust-analyzer/syntaxTree) — printed parser-level syntax tree. file_path required; range coords are optional but all-or-nothing (line+character+end_line+end_character together, or none for whole-file). Partial coords are a tool-call error rather than a silent fallback to whole-file, so the LLM can't send an under-specified range and quietly get the wrong scope. - rust_analyzer_view_hir (rust-analyzer/viewHir) — debug-printed HIR for the function body containing the given position. - rust_analyzer_view_mir (rust-analyzer/viewMir) — same for MIR. All three follow the existing with_doc + lookup_to_null pattern, so "position not inside a function body" / "indexer not ready" collapse to null instead of an error — matching the rest of the rust-analyzer/* suite. Output is a free-form string; no truncation, since the value of these tools is in seeing the full lowering and a 5KB cap would cut it mid-statement. New ToolParams::extract_optional_range helper covers the all-or-nothing range pattern for any future optional-range tool. 77/77 tests green (was 76 — one new integration test exercises whole file, narrowed range, partial-range guard, and both view_* tools). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Walks LSP responses and attaches a `snippet` sibling to every Location
(`{ uri, range }`) and LocationLink (`{ targetUri, targetRange }`), so the
LLM consumer doesn't need a follow-up file read after every reference,
definition, or workspace_symbol hit.
- New `src/mcp/snippets.rs` — generic walker `enrich_locations` plus
`enrich_workspace_diagnostics` for the `{ files: { uri: [...] } }` shape.
Per-call file cache, 50-hit budget, ~400 bytes per snippet.
- 9 tools opt in: definition, references, type_definition, implementation,
parent_module, runnables, related_tests, workspace_symbol,
workspace_diagnostics. Each accepts `include_snippets` (default true)
and `snippet_context_lines` (default 2) via `inject_snippet_opts` in
`tools.rs`.
- Pagination runs *before* enrichment so workspace_symbol and
workspace_diagnostics only read files for the page actually returned.
- Integration test: `test_phase2_1_snippets`.
Folded in: working-tree refactor that was sitting unposted (LSP error
plumbing, server/handlers/tools tidy-up, schema helpers, src/util.rs
extraction). Tests 90/90, fmt clean, clippy clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single tool that fans hover + definition + type_definition + parent_module +
references_sample out concurrently via tokio::join! on a once-opened
document, returning all five in one round-trip. Replaces the typical
4-5 follow-up calls a caller makes after locating a symbol.
- references is wrapped in a 2 s timeout so a slow workspace-wide search
can't block the rest of the composite; on timeout the response still
comes back with `references_timed_out: true`.
- references_sample shape: { items: [up to 5], total, shown } — enough
to navigate without burning token budget.
- Per-subcall errors degrade to null (best-effort) rather than poisoning
the whole response.
- Tool is in SNIPPET_ENRICHED_TOOLS, so every nested location gets
pre-attached source snippets.
All 5 sub-calls share the same MCP request id via task_local, so the
existing PendingTracker cancels them as a group with no extra plumbing.
Tests 91/91, fmt clean, clippy clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
§2.4 — Call-Hierarchy + Type-Hierarchy (3 new tools):
- rust_analyzer_call_hierarchy_incoming / _outgoing — prepare→fan-out
composites driven server-side. fromRanges are reshaped into self-
contained {uri, range} call_sites so the snippet walker can enrich
every call site, not just the caller header.
- rust_analyzer_type_hierarchy with direction ∈ {supertypes, subtypes,
both}; super/sub queries run via tokio::join! when both requested.
- 6 thin LSP wrappers (prepare* + incoming/outgoing/super/sub) with
lookup_to_null. callHierarchy/typeHierarchy capabilities declared
in initialize.
- Snippet walker now prefers selectionRange over range when both are
present, so call/type hierarchy items get a name-sized snippet
rather than a full-body one.
§5.3 — impact composite (1 new tool, blast-radius for refactors):
- rust_analyzer_impact aggregates four buckets in parallel: references,
callers (incoming calls flattened to from-items), implementors
(subtypes), and tests (related_tests). Each bucket has shape
{ items: [...up to 10], total, shown, timed_out? }.
- Two-stage parallelism via tokio::join!: stage 1 runs references +
prepareCallHierarchy + prepareTypeHierarchy + relatedTests; stage 2
fans out incomingCalls + subtypes on prepared items. Per-bucket 2 s
timeout so one slow workspace-wide search can't block the rest.
Tests: +2 integration tests, 93/93 green (was 91).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds rust_analyzer_get_type_by_name(name): a composite that resolves
a path-style symbol name without needing a file/position. Splits the
input on "::", fuzzy-searches via workspace_symbol on the last
segment, filters surviving entries against the path prefix
(exact match / endsWith / per-segment substring on containerName),
then runs hover + type_definition in parallel on the first match.
Output shape: { matches: [...up to 10], total, shown,
primary?: { hover, type_definition, location } }. The primary block
is omitted when no match is found.
Limitation: rust-analyzer's workspace_symbol typically only indexes
the local workspace, so external-crate paths like serde_json::Value
won't resolve unless the crate is a workspace member. Documented
implicitly via primary=null on no-match.
Tests: +1 integration test, 94/94 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
30s TTL plus Cargo.toml/Cargo.lock mtime keying so resources/list and resources/read no longer fork cargo on every probe. Manual invalidation on replace_root / shutdown_client. Lazy provider plumbed through list_resources/read_resource so workspace://files reads still skip metadata entirely.
New rust_analyzer_move_file tool fixes the "rename a Rust file but forget to update mod-decls/imports" trap. Sequence: workspace/willRenameFiles gives us a WorkspaceEdit, the new mcp::workspace_edit module applies those text edits on disk (reverse-order TextEdit splicing, scoped to within the workspace root), then std::fs::rename does the physical move. Affected URIs are dropped from rust-analyzer's open-doc state so the next tool call reloads fresh content. Also adds RustAnalyzerClient::close_document and the workspace.fileOperations.willRename client capability so rust-analyzer will emit the edit at all.
Reshape rust-analyzer's raw runnables to a compact envelope:
{ runnables: [{ kind, label, fq_name?, cargo_args, location,
can_run_via_mcp }], total, can_run_via_mcp }
`cargo_args` is a flat argv (cargoArgs + cargoExtraArgs + ["--"] +
executableArgs) ready to feed into the new run_runnable tool. `kind`
is parsed from the leading word of rust-analyzer's label; `fq_name`
is best-effort from executableArgs[0] or label tail. `location`
stays as the original LocationLink so the snippet walker keeps
enriching it.
New tool rust_analyzer_run_runnable(cargo_args, timeout_secs?):
spawns `cargo` inside the workspace root with kill_on_drop(true),
captures stdout/stderr capped at 5 KiB each, default timeout 60 s
(hard cap 600 s). Gated behind RUST_ANALYZER_MCP_ALLOW_RUN=1 so
hosts must opt into in-MCP code execution. Subcommand is
constrained to a whitelist (test/bench/run/build/check/clippy/
doc/nextest); anything else is rejected before spawning.
Cancellation reuses the existing in_flight AbortHandle path —
aborting the tool task drops the Child, kill_on_drop SIGKILLs
cargo. No extra plumbing.
12 new unit tests (reshape/validate/cap), 2 new integration tests
(reshape envelope shape, run_runnable input rejection). Total
120/120 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ion, tags format_diagnostics swallowed three LSP fields that rust-analyzer emits and that an LLM wants first when fixing a bug: - `data.rendered` — cargo-formatted error block with ASCII pointers - `codeDescription.href` — link to the error-index doc page - `tags` — DiagnosticTag (e.g. [1] = unused) The workspace_diagnostics path already passed these through (raw passthrough of each diag value); only the single-file diagnostics path was reshaping them away. Stable shape: keys exist, values are null when absent upstream. Tool description for `rust_analyzer_diagnostics` updated to flag `data.rendered` as the first thing to read on a bug-fix loop. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…location)
rust-analyzer's `workspace/symbol` returns each match twice in many cases
(once per index source). The duplicates eat token budget without adding
signal — confirmed in a spike against eza where workspace_symbol("UseColours")
returned `total: 2` for a single enum and get_type_by_name("UseColours")
likewise reported two identical matches.
Add `dedup_workspace_symbols` that collapses on the
`(name, containerName, location)` triple and apply it before pagination
in `paginate_workspace_symbol` plus inside `handle_get_type_by_name`
(which bypasses the pagination wrapper). First-seen wins so list order
stays stable, and `containerName` is part of the key so co-located
methods on different impl blocks (DirAction::deduce vs RecurseOptions::deduce)
remain distinct.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rust-analyzer's textDocument/documentSymbol returns a hierarchical
DocumentSymbol[] where every nested child carries its own range,
selectionRange, and detail string. For dense files this explodes into
tens of kilobytes (eza's theme/mod.rs hit ~64 KB in the spike).
Default behavior changes to top-level only: each top-level symbol keeps
its name, kind, detail, range, selectionRange and gets a numeric
`child_count` in place of the `children` array. Pass verbose=true to get
the full nested tree unchanged. Output wrapper is `{ symbols,
total_top_level, verbose }` so the LLM can tell at a glance how much
was elided.
Tool description updated to call out the cap and the verbose escape
hatch. Two integration test helpers were reading the legacy flat array
shape; both updated to read the new wrapper.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After the symbols-cap reshape (0d6d869), `rust_analyzer_symbols` returns `{ symbols, total_top_level, verbose }` instead of a flat `DocumentSymbol[]`. The MCPTestClient readiness probe still parsed the response as `Vec<Value>`, which never succeeded against the new shape, so `initialize_and_wait` looped to its 30s timeout. Surfaced as flakes in test_subprocess_restart_on_crash and test_cancellation_doesnt_break_server. Probe now accepts both shapes: a top-level Object reads `.symbols`, a top-level Array is treated as legacy. Either way a non-empty list counts as ready. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The workspace registry has lived in memory only — every server restart forced the LLM to call `add_workspace` again for any workspaces it had previously registered. Spike against eza confirmed this as a real papercut: after a daemon rebuild the secondary workspace was silently gone. Add a small file-backed persistence layer (`src/mcp/persistence.rs`) that mirrors the registered roots to `workspaces.json` under a state directory: - `RUST_ANALYZER_MCP_STATE_DIR` — explicit override; empty string means "persistence disabled". - `XDG_STATE_HOME/rust-analyzer-mcp` — XDG-compliant default. - `$HOME/.local/state/rust-analyzer-mcp` — fallback. `add_workspace`, `remove_workspace`, and `set_workspace_root` mirror the registry to disk via an atomic tempfile-rename. A tokio Mutex serialises writes so concurrent mutations can't interleave. On boot `with_workspace` re-registers any persisted roots, skipping ones that collide with the initial CLI-arg root and warning on roots that no longer exist on disk. Persistence is best-effort: I/O errors are logged but never propagated. Workspace ids are *not* persisted — they're freshly issued each boot in registration order. The two test-support spawn paths set `RUST_ANALYZER_MCP_STATE_DIR=""` so test daemons don't write to the user's real state dir. 7 unit tests cover the state-dir resolver, save/load roundtrip, phantom-path skip, malformed-file handling, and atomic mkdir-on-write. 4 server-level tests cover the boot-replay roundtrip, post-remove absence, initial-root dedup, and the disabled-persistence path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Spike against eza surfaced a real footgun: after editing files via the Edit tool, rust_analyzer_workspace_diagnostics returned `total_errors: 0` while `cargo check` found three missing-match-arm errors. The cause is that rust-analyzer's diagnostics reflect its in-memory view of each file, and external edits never went through `textDocument/didChange`, so the analyser is reading the pre-edit content. Document the failure mode in the tool descriptions for both `diagnostics` and `workspace_diagnostics`. The single-file variant points at any positional tool as the cheap re-open lever; the workspace variant pins `cargo check` as the ground truth when the two disagree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Spike 1 + Spike 2 (notes.md sequences 2 + 12) both lost an iteration to the same off-by-one trap: copying a line number from a `Read`-style 1-based output directly into a position tool (which is 0-based per LSP) silently misses the symbol — `references` returns 10 unrelated module matches, `call_hierarchy_incoming` returns total: 0, etc., with no hint that the position is wrong. Append a uniform suffix to every line/character JSON-schema description "— 0-based per LSP. If copied from a `Read`-style 1-based output, subtract 1." Centralized via line_prop / char_prop so all 12 position schemas (incl. the four optional inline cases for runnables and syntax_tree range) carry the same reminder. Tests: 140/140 still green; fmt + clippy clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Spike notes.md sequence 10 confirmed the 0d6d869 wrapper kicks in but isn't enough for files with hundreds of top-level items: eza's theme/mod.rs still serializes to ~70 KB (3102 JSON lines) under verbose=false because the children-collapse only flattens nested structure, not list breadth. Mirror the workspace_symbol pagination shape: shape_document_symbols now takes (cursor, limit, verbose), default page size 100, DOCUMENT_SYMBOLS_DEFAULT_LIMIT exposed for handlers. Output gains `returned` and optional `next_cursor`. verbose=true keeps the full subtree of the page. Tool description in tools.rs now advertises cursor/limit and the new envelope keys. Tests: 142/142 (87 lib + 28 integration + 7 property + 6 stress + 12 protocol + 2 new pagination unit tests). 4 existing shape_document_symbols tests updated for the new arity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Spike 3 (notes.md sequence 14) hit a silent-no-op scenario: the laufender daemon was the cargo-installed binary (built after commit 39a7965 added persistence) yet add_workspace produced no workspaces.json. Most likely cause is the MCP host spawning the server with a sanitized environment that drops HOME, so default_state_dir() falls through to None. The code is correct (tests stay green); what was missing was a visible warning that persistence requires the spawn env to preserve HOME/XDG_STATE_HOME, plus a knob to set RUST_ANALYZER_MCP_STATE_DIR explicitly when the host doesn't. Add a short README section documenting the resolution order and the silent-no-op failure mode, and append the same caveat to the add_workspace tool description so an LLM caller sees it in tools/list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The user-facing tool list in README was 9 entries deep and stuck on the pre-Phase-1 surface. The server now exposes 36 tools (Phase 1 LSP standards + rust-analyzer specials, Phase 2 truncation/pagination, Phase 3 multi-workspace + resources, ANALYSIS §2.1-§5.3 composites and move_file, plus the post-spike polish patches). All are dropped into 11 thematic groups: diagnostics & hover, navigation, search & discovery, composite (explore_symbol/impact), hierarchy, refactoring (incl. move_file), runnables (incl. run_runnable subprocess), compiler internals, other (completion/signature_help/open_docs), and workspace management. Add a Resources section for the three workspace:// URIs (files, crates, crate manifests) plus the per-workspace prefixed variants for multi-workspace setups. Add an Operational Features section covering the cross-cutting work that doesn't surface as a tool: concurrency + cancellation forwarding, subprocess auto-restart, external-edit didChange sync, snippet enrichment, token-cost guardrails, cargo-metadata cache, and the build-profile-aware spawn lock. Update Project Structure to reflect the actual src/ layout (lsp/, mcp/, diagnostics/, protocol/) and tests/ split. Refresh Contributing with the realistic remaining gaps (update_document, null-result hint, more config knobs) instead of items that have shipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three findings from auditing 12 days of MCP client logs (67 sessions, 17 workspaces): every observed shutdown went through the SIGTERM fallback path, and all "errors" in the log were just routine INFO startup messages bleeding through stderr. - Default tracing filter `info` → `warn`. The MCP client classifies all stderr output as error; routine startup logs no longer pollute that channel. `RUST_LOG=info` (or debug) still works for verbose runs. - Parallelize workspace shutdown via `join_all` with an 80ms budget. Was sequential and unbounded; with multiple workspaces this trivially blew past the client's ~100ms SIGINT grace window. - `std::process::exit(0)` after `run()` returns. `tokio::io::stdin()` is backed by a blocking-pool thread sitting in `read(2)` on a pipe the parent hasn't closed; `Runtime::drop` blocks forever waiting for that thread to finish, so SIGINT-triggered shutdowns never actually exited before the client escalated to SIGTERM. Manual test: shutdown latency dropped from >1000ms (always SIGKILL'd) to ~7-8ms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rust-analyzer keeps every didOpen'ed file as an in-memory overlay and discards its own file-watcher events for it — the LSP client is assumed to own that buffer. An MCP tool call owns nothing: every edit lands on disk, from the Edit tool, another editor, or a git checkout. Since close_document was only ever called on move_file, any file a tool call touched stayed frozen at that content for the rest of the server's life. Measured effects, on isolated copies of test-project: - workspace_symbol never saw a function added to a previously-opened file — the same edit was picked up immediately when the file had never been opened. - references on lib.rs lost a call site added to an already-open utils.rs, i.e. a stale overlay on one file silently corrupted the answer for another file that *was* re-read. resync_open_documents() now stats every open document before dispatch and pushes a didChange for anything whose mtime moved (didClose for anything that vanished). Steady-state cost is one stat per open document, no reads. The regression tests poll until rust-analyzer demonstrably answers correctly before editing, so "still indexing" cannot be mistaken for "answered from stale content". Against the unfixed server both time out after 60s — the overlay never thaws on its own. Also drops the "treat cargo check as ground truth after external edits" advice from the diagnostics tool descriptions, which would now steer callers the wrong way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
At full `cargo test` parallelism the suite ran 31 integration tests, each spawning its own rust-analyzer that indexes the standard library. Adding three more tipped it over a limit on this box: rust-analyzer would occasionally exit with a panic mid-run, failing whichever test was holding it (`inotify` max_user_instances is 128 on stock Linux). Measured, 6 runs each: 28 tests alone → 0 crashes; the 3 new tests alone → 0 crashes; all 31 together → crashes in 4 of 6 runs; all 31 with --test-threads=4 → 0 crashes. So it is concurrent-instance pressure, not a defect in what the tests exercise. Folding the three into one test that reuses a single client removes two instances and the flakiness with it: 6 full-suite runs, 0 crashes. It is also faster, since rust-analyzer only warms up once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The re-sync commit documented a "rust-analyzer's own watcher needs 5-10s" caveat and filed a matching contributing item: build an FS watcher that relays workspace/didChangeWatchedFiles. Both were wrong, and the item would have sent the next contributor down a dead end. Measured with a notify-based watcher actually built and wired up: - A change to a never-opened file reaches workspace_symbol immediately, with and without the watcher. rust-analyzer's own watching is prompt; the VFS is current within milliseconds. - Reference-style queries lag ~5s behind such a change — 5.01s in every run, with the watcher (5.01s), without it (5.01s), and no LSP traffic at all during the wait. Opening the file explicitly makes it worse (10.01s), since that path waits on cache priming. So the lag is internal to rust-analyzer and not a file-sync problem at all. The watcher was dropped rather than committed: no measurable benefit, and it would have doubled this server's use of inotify instances (128 per user on stock Linux). The references tool description now warns about the ~5s window directly, since an agent that edits and immediately queries is exactly who gets caught by it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Picks up the open-document re-sync fix: tool calls no longer answer from content rust-analyzer froze when it first opened a file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 15, 2026
zeenix
reviewed
Aug 15, 2026
zeenix
left a comment
zeenix
left a comment
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: Apologies this sat unreviewed: notifications for this repository were accidentally disabled, so it wasn’t seen until now.
This special-care pass found several independent merge blockers in cancellation/transport handling, mutating tools, URI/position correctness, and resource bounds. Please do not merge the current head. The safest sequence is to land the smaller prerequisites first (#15 notification handling, #14 workspace startup, corrected #19 URI conversion, corrected #20 LSP lifecycle, a ping-only #9, and #8’s stream abstraction), then rebase this series and drop/port the overlapping implementations. The rebase must preserve all of current main/#23: cross-platform ShutdownSignal, explicit runtime shutdown, CLI parsing, package-derived server version, shared IPC fixes, and shutdown tests.
Given the 9k-line scope, I strongly recommend splitting this into staged PRs—runtime/lifecycle; read-only tools and shaping; multi-workspace/resources; and the security-sensitive move_file/run_runnable tools—rather than landing all 40 commits together. CONTRIBUTING.md also requires consistent package/component prefixes; commit 46b38cb is not atomic, and follow-up fixes/tests should be absorbed into their originating commits rather than appended. Finally, the public API and array-to-object response changes are breaking, not additive, so a 0.2.1 patch release is not appropriate unless compatibility is preserved. GitHub Actions currently shows action_required, so there is no green CI result; after rebasing, please run the full cargo test --all-features, nightly fmt, and clippy checks.
| // tracker still has the LSP ids registered. | ||
| let entries = self.workspaces.read().await.list(); | ||
| for entry in entries { | ||
| if let Some(client) = entry.maybe_client().await { |
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: [blocking] Cancellation is blocked before it reaches handle.abort(). A request cold-starting rust-analyzer holds WorkspaceEntry.client’s write lock across client.start().await; cancelling that request waits here for maybe_client()’s read lock, so cancellation cannot take effect until initialization/reload has already finished, potentially after multiple LSP timeouts. Abort promptly before awaiting workspace locks, or make upstream cancellation forwarding non-blocking, and add a cold-start cancellation test.
| } | ||
| }; | ||
|
|
||
| let mut writer = stdout.lock().await; |
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: [blocking] This request remains registered in in_flight while writing its response. A concurrent cancellation can abort write_all after only a prefix has reached the pipe, or between the JSON and newline/flush; the next response then appends to an unterminated frame and corrupts stdio for every later request. Remove the request from cancellation tracking before response emission, or enqueue complete frames to a dedicated non-abortable writer. Fatal writer errors must also be sent back to the main loop so cleanup runs.
| let mcp_request_id = CURRENT_MCP_REQUEST_ID.try_with(|s| s.clone()).ok(); | ||
| let (tx, rx) = oneshot::channel(); | ||
| self.pending_requests.lock().await.insert(id, tx); | ||
| self.pending_requests.insert( |
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: [blocking] This pending entry has no drop guard. The composite handlers wrap calls in 2-second tokio::time::timeouts (for example handlers.rs:404 and 840–849), so expiry drops send_request before its own timeout reaches the cleanup at line 280. If rust-analyzer never answers, both pending indexes remain forever and no $/cancelRequest is sent. Make registration RAII/cancellation-safe and test that dropping a request future removes both indexes and cancels upstream.
| let response = json!({ | ||
| "jsonrpc": "2.0", | ||
| "id": id, | ||
| "result": null, |
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: [blocking] workspace/configuration cannot be acknowledged with null: its result must be an array with one entry per params.items. Returning null can make rust-analyzer reject configuration or continue with unusable settings. Dispatch server-initiated requests by method—#20 already contains the item-count implementation—and return -32601 for unsupported methods rather than falsely acknowledging them.
| info!("Received shutdown signal"); | ||
| *running_clone.lock().await = false; | ||
| }); | ||
| let mut shutdown = std::pin::pin!(tokio::signal::ctrl_c()); |
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: [blocking] This branch predates current main/#23 and replaces its persistent cross-platform ShutdownSignal with one ctrl_c future. Keeping this during rebase would again skip SIGTERM/SIGHUP and Windows console-close cleanup, ignore signal-registration errors, and discard the new shutdown coverage. Please adapt the concurrent loop around current main’s signal abstraction and preserve tests/integration/shutdown.rs.
| for line in &lines[from..to] { | ||
| // +1 for the implicit '\n' so the cap reflects rendered size | ||
| let line_bytes = line.len().saturating_add(1); | ||
| if total_bytes.saturating_add(line_bytes) > SNIPPET_MAX_BYTES_PER_HIT && !out.is_empty() |
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: The !out.is_empty() exception admits an arbitrarily long first line whole, so a 1 MiB line produces a 1 MiB “400-byte-capped” snippet. The implementation also reads and splits the entire file synchronously on an async worker before enforcing the output cap. Bound input reads/cache, move blocking I/O off the Tokio worker, and truncate a long individual line at a UTF-8 boundary.
| let items = dedup_workspace_symbols(items); | ||
| let total = items.len(); | ||
| let start = cursor.min(total); | ||
| let take = if verbose { total - start } else { limit }; |
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: verbose=true ignores the already-resolved limit, including the schema’s documented 1,000-item absolute cap, and returns every remaining symbol. The same pattern appears for completion and workspace diagnostics. Always take limit.min(remaining); verbose should change the default limit, not bypass the absolute/explicit one. Add >1,000-item and verbose + explicit limit tests.
| .with_context(|| format!("writing tempfile {}", tmp.display()))?; | ||
| f.sync_all().ok(); | ||
| } | ||
| fs::rename(&tmp, &target) |
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: std::fs::rename(tmp, target) does not replace an existing destination on Windows. The first save succeeds, later registry saves fail and are only logged, so restart restores stale workspace roots. Use a cross-platform atomic-replace strategy and test two consecutive saves/replacements on Windows.
| .init(); | ||
|
|
||
| // Get workspace path from command line or use current directory. | ||
| let workspace_path = std::env::args() |
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: Current main now uses args_os with help/version/unknown-option/extra-argument handling and an explicit runtime with shutdown_background plus panic handling. This branch restores args() (including the non-UTF-8 panic and treating flags as workspace paths) and ends with process::exit(0), which skips destructors. Resolve this conflict in favor of current main’s CLI/runtime structure while adapting the new server call.
| [package] | ||
| name = "rust-analyzer-mcp" | ||
| version = "0.2.0" | ||
| version = "0.2.1" |
Owner
There was a problem hiding this comment.
🤖 Chatgpt GPT-5.6 on behalf of Zeeshan: This series changes several public response types from arrays to wrapper objects and changes the public server API, so 0.2.1 is not an appropriate patch bump unless backward compatibility is restored; for a pre-1.0 crate this should be a 0.3.0-style breaking release. Also preserve current main’s env!("CARGO_PKG_VERSION") initialize response—the PR still reports hard-coded 0.1.0 at src/mcp/server.rs:480.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
better-mcp-series→mainExpands the MCP server from a 10-tool LSP shim into a 36-tool concurrent
service with cancellation, auto-restart, multi-workspace support, and
LLM-friendly output shaping.
Scope: 36 commits, +8,694 / −827 lines, 32 files.
Tests: 142 green (was ~30) — 89 unit + 28 integration + 7 property + 6 stress + 12 protocol.
Why
documentSymbolcould be 60+ KB;
workspace/symbolreturned duplicates; hover markdownwas uncapped).
cancellation couldn't reach in-flight LSP work, external edits silently
desynced rust-analyzer's view, and the post-
didOpen200 ms sleep wasa guess.
What's new
26 new tools. Highlights:
tokio::join!):explore_symbol(hover + def + type_def + parent + refs sample),impact(refs + callers + implementors + tests — blast radius),get_type_by_name(no file/position needed).rename+prepare_rename,signature_help,inlay_hints,workspace_symbol,type_definition,implementation,call/type hierarchy (both directions).
expand_macro,parent_module,runnables(reshaped envelope) +
run_runnable(opt-incargospawn),related_tests,open_docs,syntax_tree,view_hir,view_mir.move_file(useswillRenameFiles+ applies the edit + renameson disk).
add_workspace,remove_workspace,list_workspaces.MCP resources (new protocol surface).
resources/list+resources/readwith three URI families:
workspace://files(tree, capped),workspace://crates(reshaped cargo metadata),
workspace://crate/<name>/Cargo.toml. Multi-workspacevariants are id-prefixed (
workspace://ws-2/...); the default workspace keepsemitting unprefixed legacy URIs.
Reliability & concurrency
&mut self→Arc<Self>+ spawn-per-request.notifications/cancelledaborts the spawnedtask and forwards
$/cancelRequestto rust-analyzer.PendingTrackercross-indexes by LSP id and originating MCP id (via
task_local), so handlersignatures stay unchanged.
oneshots with
LspError::ProcessDied, next request gets a fresh subprocess.Rate-limited (3 crashes / 60 s).
textDocument/didChangeon real edits instead of staledidOpenstate.$/progressintegration: replaced the 200 ms post-didOpensleep witha level-triggered watch on rust-analyzer's
rustAnalyzer/cachePrimingtoken.LspErrordistinguishes transport / timeout / cancel / process-diedfrom "no result at this position" (still surfaced as
null).LLM-friendliness
symbolscollapses children (with
child_count), workspace_symbol/diagnostics dedupverbose=truebypasses caps.cursor/next_cursoronsymbols,workspace_symbol,workspace_diagnostics.snippets next to every
{ uri, range }, eliminating the follow-upRead.Per-call file cache, 50-hit budget, pagination runs before enrichment.
workspace_symboldedup on(name, containerName, location)—rust-analyzer often returns each entry twice.
data.rendered(cargo-formatted error block),codeDescription.href, andtagsnow reach the LLM (previously stripped).Multi-workspace + persistence
WorkspaceRegistryowns one entry per registered root, each with its ownrust-analyzer subprocess, restart history, open-doc cache, and stable id
(
ws-1,ws-2, ...).workspaces.jsonunder XDG state dir(
RUST_ANALYZER_MCP_STATE_DIR→XDG_STATE_HOME→$HOME/.local/state).Best-effort; set the env var to
""to disable. Workspace ids are notpersisted (reissued each boot).
Operational
Audit of 12 days of MCP-client logs (67 sessions, 17 workspaces) showed every
shutdown hitting the SIGTERM fallback. The final commit fixes that:
info→warn(routine startup logs no longerpollute the MCP client's stderr-as-error channel).
join_allwith an 80 ms budget;kill_on_dropis the backstop.std::process::exit(0)afterrun()returns —tokio::io::stdin()'sblocking-pool thread sits in
read(2)on a pipe the parent hasn't closed,which deadlocks
Runtime::drop. Shutdown latency: >1,000 ms → ~7–8 ms.Backward compatibility
symbols,workspace_symbol,workspace_diagnosticsare now wrapper objects(
{ symbols, total, ... }) — additive, not breaking semantics, but callersthat destructure the raw array need a one-line adjustment.
set_workspacekeeps working (now replaces the default workspace's rootin place, preserving its id).
workspace://...URIs still route to the default workspace.workspace_idis optional on every workspace-scoped tool.Known limitations / follow-ups
$/cancelRequestarrives a few ms after the local task abort(acceptable in practice).
get_type_by_nameonly resolves what rust-analyzer'sworkspace/symbolindexes — typically workspace members; external-crate paths return
primary: null.update_document(write-through edits) intentionally out of scope; externaledits via the host's
Edittool remain the canonical path.