fix(#102): handle external file changes via stat-resync, didChangeWatchedFiles, and a populated diagnostics cache#103
Conversation
|
I just realised this probably really needs a token generation count... |
There was a problem hiding this comment.
Pull request overview
This PR fixes #102 by keeping mcpls and underlying LSP servers consistent with on-disk edits made outside mcpls, adds workspace/didChangeWatchedFiles forwarding via a per-server filesystem watcher, and makes diagnostics usable by populating/pulling/merging through a configurable diagnostics_mode.
Changes:
- Re-sync tracked documents on external edits (stat-on-access + didClose/didOpen) to prevent stale per-file tool results.
- Add server-to-client JSON-RPC request handling plus
workspace/didChangeWatchedFilesdynamic registration and anotify-based watcher to keep workspace indexes fresh. - Populate
NotificationCachefrom LSP notifications and addworkspace.diagnostics_modewith hybrid dedup.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/mcpls.toml | Documents new workspace.diagnostics_mode config. |
| docs/adr/0001-external-file-changes.md | ADR capturing the external-change handling design and tradeoffs. |
| crates/mcpls-core/tests/integration/rust_analyzer_tests.rs | Adds ignored rust-analyzer integration regressions for resync, watcher registration, and diagnostics cache. |
| crates/mcpls-core/src/lsp/types.rs | Introduces InboundMessage::Request and tightens notification parsing contract. |
| crates/mcpls-core/src/lsp/transport.rs | Correctly classifies inbound JSON-RPC messages as request/response/notification. |
| crates/mcpls-core/src/lsp/mod.rs | Exposes watcher/request types needed by lifecycle/client. |
| crates/mcpls-core/src/lsp/lifecycle.rs | Wires watcher + request dispatcher into server spawn/init; exposes notification receiver. |
| crates/mcpls-core/src/lsp/file_watcher.rs | Implements notify-backed watcher and LSP event forwarding with glob matching/coalescing. |
| crates/mcpls-core/src/lsp/client.rs | Adds server-to-client request forwarding and response sending in the message loop. |
| crates/mcpls-core/src/lib.rs | Pumps LSP notifications into NotificationCache and wires diagnostics mode from config. |
| crates/mcpls-core/src/config/mod.rs | Adds workspace.diagnostics_mode enum + serde/default behavior and tests. |
| crates/mcpls-core/src/bridge/translator.rs | Implements pull/cached/hybrid diagnostics sourcing and hybrid dedup logic. |
| crates/mcpls-core/src/bridge/state.rs | Adds file signatures + resync logic in DocumentTracker::ensure_open. |
| crates/mcpls-core/src/bridge/mod.rs | Re-exports SyncSignature. |
| crates/mcpls-core/Cargo.toml | Adds notify + globset runtime deps. |
| Cargo.toml | Adds workspace-level notify + globset versions. |
| Cargo.lock | Locks new dependency graph for watcher functionality. |
| CHANGELOG.md | Documents the new watcher support, resync behavior, and diagnostics mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for root in &workspace_roots { | ||
| if let Err(e) = watcher.watch(root, RecursiveMode::Recursive) { | ||
| warn!("file watcher: failed to watch {}: {e}", root.display()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The spawn docstring says this returns an error “if any workspace root cannot be watched”, but the implementation only logs a warning and continues when watcher.watch(root, ...) fails. Either update the docs to reflect the non-fatal/partial-watch behavior, or change the implementation to fail (or at least error) when no roots could be watched.
| for root in &workspace_roots { | |
| if let Err(e) = watcher.watch(root, RecursiveMode::Recursive) { | |
| warn!("file watcher: failed to watch {}: {e}", root.display()); | |
| } | |
| } | |
| let mut watched_root_count = 0usize; | |
| for root in &workspace_roots { | |
| match watcher.watch(root, RecursiveMode::Recursive) { | |
| Ok(()) => watched_root_count += 1, | |
| Err(e) => { | |
| warn!("file watcher: failed to watch {}: {e}", root.display()); | |
| } | |
| } | |
| } | |
| if !workspace_roots.is_empty() && watched_root_count == 0 { | |
| return Err(Error::Transport( | |
| "file watcher: failed to watch any workspace roots".to_string(), | |
| )); | |
| } |
| let uri = self.open(path.to_path_buf(), content.clone())?; | ||
| let state = self | ||
| // Record the signature now that the document is tracked; if the file | ||
| // is replaced before the next access, the next ensure_open will see a | ||
| // mismatch and re-sync. | ||
| self.set_synced_signature(path, signature); | ||
| let language_id = self | ||
| .documents | ||
| .get(path) | ||
| .ok_or_else(|| Error::DocumentNotFound(path.to_path_buf()))?; | ||
|
|
||
| let params = DidOpenTextDocumentParams { | ||
| text_document: TextDocumentItem { | ||
| uri: uri.clone(), | ||
| language_id: state.language_id.clone(), | ||
| version: state.version, | ||
| text: content, | ||
| }, | ||
| }; | ||
|
|
||
| lsp_client.notify("textDocument/didOpen", params).await?; | ||
|
|
||
| .ok_or_else(|| Error::DocumentNotFound(path.to_path_buf()))? | ||
| .language_id | ||
| .clone(); | ||
| send_did_open(lsp_client, &uri, &language_id, 1, content).await?; |
There was a problem hiding this comment.
set_synced_signature is called before the first didOpen is successfully sent. If textDocument/didOpen fails (transient transport issue), the document remains tracked with a matching signature, so subsequent ensure_open calls will short-circuit and never retry the didOpen, leaving the LSP server out of sync. Consider only recording synced_signature (and/or inserting the document) after didOpen succeeds, or roll back the tracked state / set the signature back to UNKNOWN on failure so the next call retries.
bug-ops
left a comment
There was a problem hiding this comment.
Validation pass — 2 blocking issues + 1 CI break.
deny.toml — notify 8.2.0 is licensed under CC0-1.0, which is absent from the allow list. cargo deny check licenses will fail on first CI run. Add "CC0-1.0" to the allow list.
Other findings inline.
Repository-wide instructions (.github/copilot-instructions.md) cover architecture overview, lock scope rules, glob anchoring, channel semantics, and deny.toml license gaps. Path-specific instruction files cover: - bridge/lsp: TOCTOU, version overflow, notification_pump deadlock, file watcher - config/deps: license allow list, serde round-trip tests, TOML defaults - mcp: 1-based positions, JsonSchema derive, tool dispatch completeness - tests: #[ignore] hygiene, resync coverage, glob pattern correctness - ci: feature flag parity, RUSTDOCFLAGS, MSRV guards - CHANGELOG: breaking change format, PR references Instructions are derived from findings in PR #101 and #103 reviews.
Repository-wide instructions (.github/copilot-instructions.md) cover architecture overview, lock scope rules, glob anchoring, channel semantics, and deny.toml license gaps. Path-specific instruction files cover: - bridge/lsp: TOCTOU, version overflow, notification_pump deadlock, file watcher - config/deps: license allow list, serde round-trip tests, TOML defaults - mcp: 1-based positions, JsonSchema derive, tool dispatch completeness - tests: #[ignore] hygiene, resync coverage, glob pattern correctness - ci: feature flag parity, RUSTDOCFLAGS, MSRV guards - CHANGELOG: breaking change format, PR references Instructions are derived from findings in PR #101 and #103 reviews.
ensure_open now stats the file on every call and compares (mtime, size) against the tracked DocumentState. On mismatch we send didClose + bumped didOpen so the LSP server observes the new content; on match we keep the existing fast path. Fixes stale per-file tool results after edits made outside mcpls (git stash/checkout, the MCP host's own edit tools, formatters, code generators) for every configured LSP, including those that do not register workspace/didChangeWatchedFiles. Adds an integration test rooted in a tempdir copy of the rust_workspace fixture that primes a query, overwrites the file on disk, and asserts the next query reflects the new symbol set. ADR 0001 records the design. Refs bug-ops#102.
mcpls now declares workspace.didChangeWatchedFiles.dynamic_registration and relative_pattern_support, handles inbound client/registerCapability and client/unregisterCapability requests, and runs a per-server notify filesystem watcher that forwards matched events as workspace/didChangeWatchedFiles. This keeps the LSP server's workspace index live across external file changes for servers that register watchers (rust-analyzer, gopls, pyright, typescript-language-server, clangd). Files mcpls has never opened are now reflected in workspace search and analysis without restarting the MCP server. Composition with the stat-on-access change in the previous commit: the watcher does not invalidate the document tracker; A's stat path already covers tracked-document freshness, and C is responsible for the workspace-wide view. Other changes: - Transport: the previous classifier mishandled server-to-client requests (any message with an `id` was treated as a response). Add `InboundMessage::Request` and dispatch on (id, method) presence. - Client: add `ClientCommand::SendResponse` and a `ServerRequest` type so the message loop can forward inbound requests to a registered handler and reply with the result. Unhandled methods receive a `MethodNotFound` error so servers do not block. - Watcher: filter `.git`, `target`, `node_modules`, `.cache` before glob matching to avoid drowning in build noise; coalesce events over a 100ms debounce window. Watcher startup failure (e.g. inotify exhaustion) is logged and non-fatal — A's stat path still covers per-file freshness. ADR 0001 is updated to record the final design. Refs bug-ops#102.
Two related changes:
1. Notification cache wiring. LspServer::spawn now creates a notification
mpsc channel and exposes the receiver via take_notification_receiver();
serve() takes each receiver and runs a pump task that drains
publishDiagnostics / logMessage / showMessage into the per-server
NotificationCache. Previously the cache existed but was never
populated, so get_cached_diagnostics always returned empty.
2. workspace.diagnostics_mode config option. Selects how get_diagnostics
sources its results:
- "pull" — only textDocument/diagnostic. Misses rust-analyzer's
flycheck/cargo-check output (push-only).
- "cached" — only the NotificationCache. Cheap; empty for files the
LSP server has not analysed.
- "hybrid" — pull + cached, deduplicated on (range, message, code).
Default.
Hybrid is the default because rust-analyzer's pull-diagnostic provider
does not surface flycheck errors, so pull-only returns empty for the
most useful diagnostics in practice.
Adds an integration test that verifies the cache populates after RA's
flycheck run for a file with an intentional error in the fixture, plus
unit tests for merge_diagnostics dedup and DiagnosticsMode serde.
Refs bug-ops#102.
The previous dedup keyed on (range, message, code), which left through rust-analyzer's common case where its native and flycheck pipelines emit the same error with different qualifications of an identifier: pull E0599 at 87:5–87:13 "no method named foo for enum DocItem" cached E0599 at 87:5–87:13 "no method named foo for enum types::DocItem" These are the same error and across many files they double the token count of get_diagnostics output. The new key: - when `code` is present, key on (range, severity, code) and ignore the message entirely. Two diagnostics with the same code at the same span ARE the same diagnostic. - when `code` is absent, fall back to a path-qualifier-stripped message so qualifier-only differences still merge. `expected DocItem` and `expected types::DocItem` collapse, but `expected DocItem` and `expected SomethingElse` do not. Pull-first ordering preserved, so the structured pull entry wins on collision and the unqualified rendering tends to be kept. Adds focused unit tests for: qualifier-only collapse with code, qualifier-only collapse without code, distinct-code preservation at the same span, distinct non-qualifier messages preserved, and the path normalizer (basic, nested, lone `::`, unicode, idempotence). Refs bug-ops#102.
Owner findings (bug-ops): - bridge/state: close TOCTOU + coarse-mtime resync hole by re-stat'ing after read so the recorded signature describes the bytes actually loaded into the LSP server. - bridge/state: reset version to 1 on i32::MAX wraparound so resync doesn't permanently emit duplicate didOpen versions that rust-analyzer silently discards. - bridge/state: send didOpen before mutating tracker state, so a failed notify leaves SyncSignature::UNKNOWN behind and the next ensure_open retries instead of short-circuiting on a fake match. - file_watcher: anchor bare LSP glob patterns with `**/` because globset matches full paths; bare `*.rs` would never match `/repo/src/lib.rs`. - lib: extract NotificationCache into its own Arc<Mutex>, decoupling the publishDiagnostics pump from the translator lock so handle_diagnostics' 30 s pull no longer head-of-line blocks the push channel that the pull is itself waiting on. Copilot findings: - file_watcher: try_send into the raw notify mpsc; the std mpsc send() blocks when full and would stall notify's delivery thread under heavy churn. - file_watcher: spawn errors when no workspace root could be watched (was: only logged); single-root failures still warn-and-continue. - file_watcher: per-FileSystemWatcher GlobBucket replaces the combined bitmask, so per-glob WatchKind filters from the LSP spec are honored. - lifecycle: parse_params returns -32602 InvalidParams instead of the -32601 MethodNotFound it was incorrectly using for params decode failures. - lifecycle: notification-receiver doc no longer claims "silently dropped" — the channel buffers up to capacity then warns on overflow. - translator/examples: hybrid dedup docs corrected to (range, severity, code) when code present, normalized message otherwise. - deny.toml: allow CC0-1.0 for the `notify` 8.x dependency so cargo-deny check licenses passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8b078a1 to
45cdd26
Compare
Closes #102.
Four commits, each independently passing tests/clippy/fmt:
fix(bridge): re-sync DocumentTracker on external file changes—ensure_opennow stats the file on every call and, on signature mismatch, sendstextDocument/didClosefollowed by a bumped-versiontextDocument/didOpento keep the LSP server in sync with disk. Fixes stale results from every per-file MCP tool (get_hover,get_definition,get_references,get_document_symbols,get_diagnostics,get_completions,get_code_actions,format_document,rename_symbol, call-hierarchy) after edits made outside mcpls (git stash/checkout, the MCP host's ownEdit/Writetools, formatters, code generators). Works for every configured LSP, including those that don't registerworkspace/didChangeWatchedFiles(e.g. zls).feat(lsp): forward workspace/didChangeWatchedFiles to LSP servers— declaresworkspace.didChangeWatchedFiles.dynamic_registration: trueandrelative_pattern_support: true, handles inboundclient/registerCapability/client/unregisterCapability, and runs a per-servernotifyfilesystem watcher that forwards matched events asworkspace/didChangeWatchedFiles. Keeps the LSP server's workspace index live (the half of the bug that survives stat-on-access) for servers that register watchers (rust-analyzer, gopls, pyright, typescript-language-server, clangd). The transport now distinguishes server-to-client requests from responses (InboundMessage::Request); previously such requests were silently misclassified as responses. New runtime dependency:notify(+globsetfor compiled glob matching).feat(bridge): populate NotificationCache, add diagnostics_mode— wirespublishDiagnostics/logMessage/showMessageinto theNotificationCache(the cache existed but was never populated in production — every notification was dropped). Addsworkspace.diagnostics_modeconfig option:pull— onlytextDocument/diagnostic. Misses rust-analyzer's flycheck output.cached— only the now-populated push cache.hybrid— pull + cached, deduplicated. Default.refactor(bridge): tighter dedup for hybrid diagnostics merge— keys on(range, severity, code)when acodeis present (so rust-analyzer'sDocItemvstypes::DocItemqualifier-only duplicates collapse), and falls back to a path-qualifier-stripped message comparison whencodeisNone. Saves ~30–50% of token output in compile-error-heavy phases without dropping legitimately-distinct errors that share a span.Design recorded in
docs/adr/0001-external-file-changes.md.Verified end-to-end
User report from a real workflow before/after:
workspace_symbol_search("BlockItem")get_diagnostics(y_adjust.rs)[]despite 5 cargo errors (file not LSP-opened)get_cached_diagnostics(y_adjust.rs)[](never populated)The "must Read first to trigger didOpen" workaround is no longer needed.
Test plan
test_ensure_open_resyncs_after_external_edit— primes a query, overwrites the file on disk, asserts the next query reflects the new symbols.test_lsp_server_installs_watcher_registration—LspServer::spawncompletes when rust-analyzer sendsclient/registerCapability, exercising the transportRequestvariant + dispatcher +FileWatcher::register.test_notification_cache_populates_from_publish_diagnostics— verifiesget_cached_diagnosticsreflects RA's flycheck output after the pump task delivers the push notification.cargo clippy --workspace --all-targets --all-features -- -D warningscleancargo +nightly fmt --checkclean🤖 Generated with Claude Code