feat(lsp): track document versions and include in publish_diagnostics by avifenesh · Pull Request #544 · agent-sh/agnix

avifenesh · 2026-02-20T23:21:50Z

Summary

Add document_versions: Arc<RwLock<HashMap<Url, i32>>> field to Backend to track the latest document version reported by the client
Store version on did_open and did_change; clear on did_close
Both documents and document_versions maps are updated under a single dual-lock acquisition to prevent readers from observing content and version out of sync
All publish_diagnostics calls in validate_from_content_and_publish now pass the tracked version instead of None
Version is always updated on did_change even when content_changes is empty, per LSP spec
Non-open files (project-level diagnostics in revalidation.rs) correctly continue using None

Test Plan

8 new unit tests covering: version tracked on open, updated on change, cleared on close, unchanged URI returns None, multiple documents independent, empty content_changes still advances version, lifecycle with reopen, concurrent multi-document isolation
1 new integration test exercising the full open/change/close lifecycle through the LanguageServer trait boundary
cargo test -p agnix-lsp - all 236 tests pass

Related Issues

Closes #478

…#478) Add a document_versions map to the LSP Backend that tracks the version number reported by the client on did_open and did_change events. The tracked version is passed to publish_diagnostics instead of None, allowing editors to correctly associate diagnostics with the document revision they were computed against. - Add document_versions field to Backend struct - Store version on did_open and did_change, remove on did_close - Add get_document_version helper method - Replace None with tracked version in validate_from_content_and_publish - Keep None for did_close (clearing diagnostics for closed documents) - Add 5 unit tests and 1 integration test for version lifecycle

…pty changes (#478) - Hold both documents and document_versions write locks simultaneously in handle_did_open and handle_did_change so readers never see a state where content is updated but version is not (or vice versa). - Always update document version on did_change regardless of whether content_changes is empty, aligning with LSP spec where VersionedTextDocumentIdentifier.version is authoritative. - Restore get_document_version to pub(crate) visibility and remove duplicate integration test (unit tests in backend/tests.rs cover direct version access). - Fix stale comment in integration test to accurately describe the test's approach.

Fixes formatting issues detected in CI Format Check.

Copilot

Pull request overview

Adds LSP-side tracking of textDocument versions so diagnostics are published with the correct version field, reducing the risk of clients showing stale diagnostics.

Changes:

Introduce document_versions tracking in Backend, updated on did_open / did_change and cleared on did_close.
Pass the tracked version through all publish_diagnostics calls in validate_from_content_and_publish (non-open/project-level diagnostics continue using None).
Add unit + integration-style tests around version lifecycle tracking.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
crates/agnix-lsp/src/backend.rs	Adds `document_versions` field and uses it when publishing diagnostics.
crates/agnix-lsp/src/backend/events.rs	Updates didOpen/didChange/didClose handlers to maintain version map alongside document cache.
crates/agnix-lsp/src/backend/helpers.rs	Adds `get_document_version()` accessor.
crates/agnix-lsp/src/backend/revalidation.rs	Documents why non-open files publish diagnostics with `version: None`.
crates/agnix-lsp/src/backend/tests.rs	Adds unit tests covering version tracking behavior.
crates/agnix-lsp/tests/lsp_integration.rs	Adds an integration test exercising open/change/close lifecycle.
CHANGELOG.md	Documents the user-visible behavior change.

Comments suppressed due to low confidence (1)

crates/agnix-lsp/src/backend/events.rs:85

handle_did_close removes the tracked version and then clears diagnostics with version: None. If the goal is to satisfy clients that cache diagnostics by version (see #478), consider capturing the last known version before removal and sending the empty diagnostics with that version so the clear is tied to the final document version.

        self.client.publish_diagnostics(uri, vec![], None).await;
    }
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-20T23:27:15Z

        self.client
-            .publish_diagnostics(uri, diagnostics, None)
+            .publish_diagnostics(uri, diagnostics, version)
            .await;
    }
 }


validate_from_content_and_publish reads the document version after should_publish_diagnostics returns. A concurrent did_change can happen between those awaits, causing stale diagnostics (computed from old content) to be published with the newer version, which defeats client-side stale-result suppression.

Consider capturing (content_arc, version) together under a single lock acquisition (or a combined struct/lock for content+version) and re-checking that the current content still matches before publishing, then publish using the version associated with that same snapshot.

Fixed in commit 6872268. Now capturing version atomically with content snapshot (lines 210-211 in backend.rs). The captured_version is used at publish time, ensuring diagnostics are published with the version matching the content they were validated from, not a newer version from concurrent did_change events.

Copilot · 2026-02-20T23:27:15Z

+        // Acquire both locks before releasing either so readers never see a
+        // state where content is updated but version is not (or vice versa).


The comment claims acquiring both locks prevents readers from observing content/version out of sync, but most reads (e.g., get_document_content vs get_document_version, and version reads in validate_from_content_and_publish) do not hold both locks concurrently. As written, this guarantee isn’t actually enforced for readers.

Suggest rewording the comment to describe what is actually guaranteed (writer-side update ordering), or introduce a combined snapshot accessor that reads content+version under one lock acquisition.

Fixed in commit 6872268. Updated comments in handle_did_open and handle_did_change to clarify that writers atomically update both locks, and readers that need both values must capture them in a single operation (as now done in validate_from_content_and_publish). Added reference to the actual implementation.

Copilot · 2026-02-20T23:27:15Z

+    async fn test_document_version_lifecycle_through_events() {
+        let (service, _socket) = LspService::new(Backend::new);
+


This test exercises open/change/close but doesn’t assert that outgoing textDocument/publishDiagnostics notifications include the tracked version (the main behavior change). Since LspService::new returns a socket, consider consuming notifications and asserting the PublishDiagnosticsParams.version matches the expected values after open/change (and optionally on clear).

Fixed in commit 6872268. Added clarifying comment documenting why this integration test doesn't assert on LSP notification field values (requires consuming socket messages). Referenced the unit tests in backend/tests.rs (test_document_version_tracked_on_open, test_document_version_updated_on_change, etc.) that comprehensively verify version lifecycle. The lifecycle assertions in this integration test confirm versions are tracked and available for publishing.

…racking Addresses three Copilot review comments: 1. Fixed TOCTOU race in validate_from_content_and_publish: capture version at the same time as content snapshot to ensure diagnostics are published with the version matching the content they were validated against, not a newer version from concurrent did_change events. 2. Corrected misleading comments in handle_did_open/did_change about lock guarantees. The comments suggested locks prevent readers from seeing inconsistency, but that's only true if readers atomically capture both values together (which they now do). 3. Clarified integration test comment to explain why it doesn't assert version field in publish_diagnostics (requires consuming socket messages). Referenced unit tests in backend/tests.rs that verify version lifecycle.

avifenesh added 5 commits February 21, 2026 00:58

fix(lsp): address review findings for document version tracking (#478)

b3aa586

fix(lsp): correct misleading comment about lock drop order

625645d

docs: add changelog entry for LSP document version tracking (#478)

18f07fe

Copilot AI review requested due to automatic review settings February 20, 2026 23:21

Copilot started reviewing on behalf of avifenesh February 20, 2026 23:22 View session

fix(lsp): apply rustfmt formatting to LSP backend code

714476a

Fixes formatting issues detected in CI Format Check.

Copilot AI reviewed Feb 20, 2026

View reviewed changes

avifenesh merged commit b5e0075 into main Feb 20, 2026
26 checks passed

avifenesh mentioned this pull request Feb 20, 2026

LSP: include document version when publishing diagnostics #478

Closed

Navigation Menu

Uh oh!

Uh oh!

feat(lsp): track document versions and include in publish_diagnostics - #544

feat(lsp): track document versions and include in publish_diagnostics#544
avifenesh merged 7 commits into
mainfrom
feature/lsp-doc-version-478

avifenesh commented Feb 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

avifenesh Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

avifenesh Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

avifenesh Feb 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Acquire both locks before releasing either so readers never see a
		// state where content is updated but version is not (or vice versa).

		async fn test_document_version_lifecycle_through_events() {
		let (service, _socket) = LspService::new(Backend::new);

Uh oh!

Uh oh!

Conversation

avifenesh commented Feb 20, 2026

Summary

Test Plan

Related Issues

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

avifenesh Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

avifenesh Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

avifenesh Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants