diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 6b1613543..e1477c733 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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 diff --git a/packages/coding-agent/src/lsp/client.ts b/packages/coding-agent/src/lsp/client.ts index 109409944..a6b736f46 100644 --- a/packages/coding-agent/src/lsp/client.ts +++ b/packages/coding-agent/src/lsp/client.ts @@ -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 { } 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; } } diff --git a/packages/coding-agent/src/lsp/clients/lsp-linter-client.ts b/packages/coding-agent/src/lsp/clients/lsp-linter-client.ts index c3586e208..784525180 100644 --- a/packages/coding-agent/src/lsp/clients/lsp-linter-client.ts +++ b/packages/coding-agent/src/lsp/clients/lsp-linter-client.ts @@ -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 { diff --git a/packages/coding-agent/src/lsp/index.ts b/packages/coding-agent/src/lsp/index.ts index 4a81f027b..72cf86b99 100644 --- a/packages/coding-agent/src/lsp/index.ts +++ b/packages/coding-agent/src/lsp/index.ts @@ -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 { + 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; +type ServerVersionMap = Map; + +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; async function captureDiagnosticVersions( cwd: string, servers: Array<[string, ServerConfig]>, -): Promise { +): Promise { const versions = new Map(); 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 { + const uri = fileToUri(absolutePath); + const versions = new Map(); + 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 { + 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; requestId: number; - diagnostics: Map; + diagnostics: Map; diagnosticsVersion: number; openFiles: Map; pendingRequests: Map; diff --git a/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts b/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts new file mode 100644 index 000000000..89a0a0454 --- /dev/null +++ b/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts @@ -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"); + }); +});