feat(lsp): added diagnostic versioning to suppress stale LSP results

- Added LSP diagnostic versioning support to track document versions and suppress stale diagnostics. - Added options parameter to `waitForDiagnostics` and `getDiagnosticsForFile` with version filtering capabilities. - Enabled `versionSupport` in LSP client capabilities to receive diagnostic version information from servers. - Added test coverage for stale diagnostic suppression scenarios in LSP diagnostic freshness.
2026-04-06 01:16:26 +02:00
parent c9be206b64
commit 97e2acdf60
6 changed files with 234 additions and 28 deletions
@@ -1,6 +1,16 @@
 # Changelog

 ## [Unreleased]
+### Added
+
+- Support for LSP diagnostic versioning to track document versions and suppress stale diagnostics
+- Options parameter to `waitForDiagnostics` for controlling diagnostic freshness validation with `expectedDocumentVersion` and `allowUnversioned` flags
+
+### Changed
+
+- Enabled `versionSupport` in LSP client capabilities to receive diagnostic version information from servers
+- Diagnostics storage now tracks both diagnostics and their associated document version for freshness validation
+- Updated `getDiagnosticsForFile` to accept options object instead of positional parameters for better extensibility

 ## [13.19.0] - 2026-04-05
 ### Added
@@ -3,11 +3,11 @@ import { ToolAbortError, throwIfAborted } from "../tools/tool-errors";
 import { applyWorkspaceEdit } from "./edits";
 import { getLspmuxCommand, isLspmuxSupported } from "./lspmux";
 import type {
-	Diagnostic,
 	LspClient,
 	LspJsonRpcNotification,
 	LspJsonRpcRequest,
 	LspJsonRpcResponse,
+	PublishDiagnosticsParams,
 	ServerConfig,
 	WorkspaceEdit,
 } from "./types";
@@ -130,7 +130,7 @@ const CLIENT_CAPABILITIES = {
 		},
 		publishDiagnostics: {
 			relatedInformation: true,
-			versionSupport: false,
+			versionSupport: true,
 			tagSupport: { valueSet: [1, 2] },
 			codeDescriptionSupport: true,
 			dataSupport: true,
@@ -261,8 +261,11 @@ async function startMessageReader(client: LspClient): Promise<void> {
 				} else if ("method" in message) {
 					// Server notification
 					if (message.method === "textDocument/publishDiagnostics" && message.params) {
-						const params = message.params as { uri: string; diagnostics: Diagnostic[] };
-						client.diagnostics.set(params.uri, params.diagnostics);
+						const params = message.params as PublishDiagnosticsParams;
+						client.diagnostics.set(params.uri, {
+							diagnostics: params.diagnostics,
+							version: params.version ?? null,
+						});
 						client.diagnosticsVersion += 1;
 					}
 				}
@@ -77,14 +77,14 @@ export class LspLinterClient implements LinterClient {
 		const timeoutMs = 3000;
 		const start = Date.now();
 		while (Date.now() - start < timeoutMs) {
-			const diagnostics = client.diagnostics.get(uri);
-			if (diagnostics !== undefined) {
-				return diagnostics;
+			const publishedDiagnostics = client.diagnostics.get(uri);
+			if (publishedDiagnostics !== undefined) {
+				return publishedDiagnostics.diagnostics;
 			}
 			await Bun.sleep(100);
 		}

-		return client.diagnostics.get(uri) ?? [];
+		return client.diagnostics.get(uri)?.diagnostics ?? [];
 	}

 	dispose(): void {
@@ -40,6 +40,7 @@ import {
 	type LspParams,
 	type LspToolDetails,
 	lspSchema,
+	type PublishedDiagnostics,
 	type ServerConfig,
 	type SymbolInformation,
 	type TextEdit,
@@ -312,22 +313,59 @@ async function reloadServer(client: LspClient, serverName: string, signal?: Abor
 	return output;
 }

+interface WaitForDiagnosticsOptions {
+	timeoutMs?: number;
+	signal?: AbortSignal;
+	minVersion?: number;
+	expectedDocumentVersion?: number;
+	allowUnversioned?: boolean;
+}
+
+function getAcceptedDiagnostics(
+	publishedDiagnostics: PublishedDiagnostics | undefined,
+	expectedDocumentVersion?: number,
+	allowUnversioned = true,
+): Diagnostic[] | undefined {
+	if (!publishedDiagnostics) {
+		return undefined;
+	}
+	if (expectedDocumentVersion === undefined) {
+		return publishedDiagnostics.diagnostics;
+	}
+	if (publishedDiagnostics.version === expectedDocumentVersion) {
+		return publishedDiagnostics.diagnostics;
+	}
+	if (allowUnversioned && publishedDiagnostics.version == null) {
+		return publishedDiagnostics.diagnostics;
+	}
+	return undefined;
+}
+
 async function waitForDiagnostics(
 	client: LspClient,
 	uri: string,
-	timeoutMs = 3000,
-	signal?: AbortSignal,
-	minVersion?: number,
+	options: WaitForDiagnosticsOptions = {},
 ): Promise<Diagnostic[]> {
+	const { timeoutMs = 3000, signal, minVersion, expectedDocumentVersion, allowUnversioned = true } = options;
 	const start = Date.now();
 	while (Date.now() - start < timeoutMs) {
 		throwIfAborted(signal);
-		const diagnostics = client.diagnostics.get(uri);
 		const versionOk = minVersion === undefined || client.diagnosticsVersion > minVersion;
-		if (diagnostics !== undefined && versionOk) return diagnostics;
+		const diagnostics = getAcceptedDiagnostics(
+			client.diagnostics.get(uri),
+			expectedDocumentVersion,
+			allowUnversioned,
+		);
+		if (diagnostics !== undefined && versionOk) {
+			return diagnostics;
+		}
 		await Bun.sleep(100);
 	}
-	return client.diagnostics.get(uri) ?? [];
+	const versionOk = minVersion === undefined || client.diagnosticsVersion > minVersion;
+	if (!versionOk) {
+		return [];
+	}
+	return getAcceptedDiagnostics(client.diagnostics.get(uri), expectedDocumentVersion, allowUnversioned) ?? [];
 }

 /** Project type detection result */
@@ -426,8 +464,14 @@ export interface FileDiagnosticsResult {
 	formatter?: FileFormatResult;
 }

-/** Captured diagnostic versions per server (before sync) */
-type DiagnosticVersions = Map<string, number>;
+type ServerVersionMap = Map<string, number>;
+
+interface GetDiagnosticsForFileOptions {
+	signal?: AbortSignal;
+	minVersions?: ServerVersionMap;
+	expectedDocumentVersions?: ServerVersionMap;
+	allowUnversionedLspDiagnostics?: boolean;
+}

 /**
 * Capture current diagnostic versions for all LSP servers.
@@ -436,7 +480,7 @@ type DiagnosticVersions = Map<string, number>;
 async function captureDiagnosticVersions(
 	cwd: string,
 	servers: Array<[string, ServerConfig]>,
-): Promise<DiagnosticVersions> {
+): Promise<ServerVersionMap> {
 	const versions = new Map<string, number>();
 	await Promise.allSettled(
 		servers.map(async ([serverName, serverConfig]) => {
@@ -448,6 +492,25 @@ async function captureDiagnosticVersions(
 	return versions;
 }

+async function captureOpenFileVersions(
+	absolutePath: string,
+	cwd: string,
+	servers: Array<[string, ServerConfig]>,
+): Promise<ServerVersionMap> {
+	const uri = fileToUri(absolutePath);
+	const versions = new Map<string, number>();
+	await Promise.allSettled(
+		servers.map(async ([serverName, serverConfig]) => {
+			const client = await getOrCreateClient(serverConfig, cwd);
+			const version = client.openFiles.get(uri)?.version;
+			if (version !== undefined) {
+				versions.set(serverName, version);
+			}
+		}),
+	);
+	return versions;
+}
+
 /**
 * Get diagnostics for a file using LSP or custom linter client.
 *
@@ -461,9 +524,9 @@ async function getDiagnosticsForFile(
 	absolutePath: string,
 	cwd: string,
 	servers: Array<[string, ServerConfig]>,
-	signal?: AbortSignal,
-	minVersions?: DiagnosticVersions,
+	options: GetDiagnosticsForFileOptions = {},
 ): Promise<FileDiagnosticsResult | undefined> {
+	const { signal, minVersions, expectedDocumentVersions, allowUnversionedLspDiagnostics = true } = options;
 	if (servers.length === 0) {
 		return undefined;
 	}
@@ -489,7 +552,14 @@ async function getDiagnosticsForFile(
 			throwIfAborted(signal);
 			// Content already synced + didSave sent, wait for fresh diagnostics
 			const minVersion = minVersions?.get(serverName);
-			const diagnostics = await waitForDiagnostics(client, uri, 3000, signal, minVersion);
+			const expectedDocumentVersion = expectedDocumentVersions?.get(serverName);
+			const diagnostics = await waitForDiagnostics(client, uri, {
+				timeoutMs: 3000,
+				signal,
+				minVersion,
+				expectedDocumentVersion,
+				allowUnversioned: allowUnversionedLspDiagnostics,
+			});
 			return { serverName, diagnostics };
 		}),
 	);
@@ -794,6 +864,7 @@ async function runLspWritethrough(

 	// Capture diagnostic versions BEFORE syncing to detect stale diagnostics
 	const minVersions = enableDiagnostics ? await captureDiagnosticVersions(cwd, servers) : undefined;
+	let expectedDocumentVersions: ServerVersionMap | undefined;

 	let formatter: FileFormatResult | undefined;
 	let diagnostics: FileDiagnosticsResult | undefined;
@@ -835,12 +906,21 @@ async function runLspWritethrough(
 				await getWritePromise();
 			}

+			if (enableDiagnostics) {
+				expectedDocumentVersions = await captureOpenFileVersions(dst, cwd, lspServers);
+			}
+
 			// 5. Notify saved to LSP servers
 			await notifyFileSaved(dst, cwd, lspServers, operationSignal);

 			// 6. Get diagnostics from all servers (wait for fresh results)
 			if (enableDiagnostics) {
-				diagnostics = await getDiagnosticsForFile(dst, cwd, servers, operationSignal, minVersions);
+				diagnostics = await getDiagnosticsForFile(dst, cwd, servers, {
+					signal: operationSignal,
+					minVersions,
+					expectedDocumentVersions,
+					allowUnversionedLspDiagnostics: false,
+				});
 			}
 		});
 	} catch {
@@ -1044,13 +1124,13 @@ export class LspTool implements AgentTool<typeof lspSchema, LspToolDetails, Them
 						const client = await getOrCreateClient(serverConfig, this.session.cwd);
 						const minVersion = client.diagnosticsVersion;
 						await refreshFile(client, resolved, signal);
-						const diagnostics = await waitForDiagnostics(
-							client,
-							uri,
-							diagnosticsWaitTimeoutMs,
+						const expectedDocumentVersion = client.openFiles.get(uri)?.version;
+						const diagnostics = await waitForDiagnostics(client, uri, {
+							timeoutMs: diagnosticsWaitTimeoutMs,
 							signal,
 							minVersion,
-						);
+							expectedDocumentVersion,
+						});
 						allDiagnostics.push(...diagnostics);
 					} catch (err) {
 						if (err instanceof ToolAbortError || signal?.aborted) {
@@ -1372,7 +1452,7 @@ export class LspTool implements AgentTool<typeof lspSchema, LspToolDetails, Them
 				}

 				case "code_actions": {
-					const diagnostics = client.diagnostics.get(uri) ?? [];
+					const diagnostics = client.diagnostics.get(uri)?.diagnostics ?? [];
 					const context: CodeActionContext = {
 						diagnostics,
 						only: !apply && query ? [query] : undefined,
@@ -93,6 +93,17 @@ export interface Diagnostic {
 	data?: unknown;
 }

+export interface PublishedDiagnostics {
+	diagnostics: Diagnostic[];
+	version: number | null;
+}
+
+export interface PublishDiagnosticsParams {
+	uri: string;
+	diagnostics: Diagnostic[];
+	version?: number | null;
+}
+
 // =============================================================================
 // Text Edits
 // =============================================================================
@@ -392,7 +403,7 @@ export interface LspClient {
 	config: ServerConfig;
 	proc: ptree.ChildProcess<"pipe">;
 	requestId: number;
-	diagnostics: Map<string, Diagnostic[]>;
+	diagnostics: Map<string, PublishedDiagnostics>;
 	diagnosticsVersion: number;
 	openFiles: Map<string, OpenFile>;
 	pendingRequests: Map<number, PendingRequest>;
@@ -0,0 +1,102 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test";
+import * as path from "node:path";
+import { createLspWritethrough } from "@oh-my-pi/pi-coding-agent/lsp";
+import * as lspClient from "@oh-my-pi/pi-coding-agent/lsp/client";
+import * as lspConfig from "@oh-my-pi/pi-coding-agent/lsp/config";
+import type { Diagnostic, LspClient, ServerConfig } from "@oh-my-pi/pi-coding-agent/lsp/types";
+import { fileToUri } from "@oh-my-pi/pi-coding-agent/lsp/utils";
+import { type ptree, TempDir } from "@oh-my-pi/pi-utils";
+
+const TEST_SERVER: ServerConfig = {
+	command: "test-lsp",
+	fileTypes: ["ts"],
+	rootMarkers: [],
+};
+
+function createDiagnostic(message: string): Diagnostic {
+	return {
+		message,
+		severity: 1,
+		range: {
+			start: { line: 0, character: 0 },
+			end: { line: 0, character: 1 },
+		},
+	};
+}
+
+function createClient(cwd: string, config: ServerConfig): LspClient {
+	return {
+		name: "test-lsp",
+		cwd,
+		config,
+		proc: {} as ptree.ChildProcess<"pipe">,
+		requestId: 0,
+		diagnostics: new Map(),
+		diagnosticsVersion: 0,
+		openFiles: new Map(),
+		pendingRequests: new Map(),
+		messageBuffer: new Uint8Array(),
+		isReading: false,
+		lastActivity: Date.now(),
+	};
+}
+
+function publishDiagnostics(client: LspClient, uri: string, diagnostics: Diagnostic[], version: number | null): void {
+	client.diagnostics.set(uri, { diagnostics, version });
+	client.diagnosticsVersion += 1;
+}
+
+describe("LSP diagnostics freshness", () => {
+	let tempDir: TempDir;
+
+	beforeEach(() => {
+		tempDir = TempDir.createSync("@omp-lsp-freshness-");
+	});
+
+	afterEach(() => {
+		vi.restoreAllMocks();
+		tempDir.removeSync();
+	});
+
+	it("suppresses stale write diagnostics until the matching document version arrives", async () => {
+		const filePath = path.join(tempDir.path(), "example.ts");
+		const uri = fileToUri(filePath);
+		const client = createClient(tempDir.path(), TEST_SERVER);
+		client.openFiles.set(uri, { version: 1, languageId: "typescript" });
+
+		vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined });
+		vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([["test-lsp", TEST_SERVER]]);
+		vi.spyOn(lspClient, "getOrCreateClient").mockResolvedValue(client);
+		vi.spyOn(lspClient, "syncContent").mockImplementation(async (mockClient, syncedFilePath) => {
+			const syncedUri = fileToUri(syncedFilePath);
+			mockClient.diagnostics.delete(syncedUri);
+			const openFile = mockClient.openFiles.get(syncedUri);
+			if (openFile) {
+				openFile.version += 1;
+			} else {
+				mockClient.openFiles.set(syncedUri, { version: 1, languageId: "typescript" });
+			}
+		});
+		vi.spyOn(lspClient, "notifySaved").mockImplementation(async (mockClient, savedFilePath) => {
+			const savedUri = fileToUri(savedFilePath);
+			setTimeout(() => {
+				publishDiagnostics(mockClient, savedUri, [createDiagnostic("stale error")], null);
+			}, 10);
+			setTimeout(() => {
+				publishDiagnostics(mockClient, savedUri, [], mockClient.openFiles.get(savedUri)?.version ?? null);
+			}, 150);
+		});
+
+		const writethrough = createLspWritethrough(tempDir.path(), {
+			enableFormat: false,
+			enableDiagnostics: true,
+		});
+		const result = await writethrough(filePath, "export const value = 2;\n");
+
+		expect(result).toBeDefined();
+		expect(result?.messages).toEqual([]);
+		expect(result?.summary).toBe("OK");
+		expect(result?.errored).toBe(false);
+		expect(await Bun.file(filePath).text()).toBe("export const value = 2;\n");
+	});
+});