From fbdc0641869c132589ca199fbbdad498b8863441 Mon Sep 17 00:00:00 2001 From: can1357 Date: Mon, 1 Jun 2026 18:03:37 +0200 Subject: [PATCH] feat(lsp): added session-scoped LSP diagnostics deduplication - Added `DiagnosticsLedger` to track diagnostics already surfaced per file, suppressing repeats within a session. - Wired dedup into both edit and write tools via a `transformDiagnostics` hook on the writethrough pipeline. - Added `lsp.diagnosticsDeduplicate` setting (default: true) to control the behavior. --- packages/coding-agent/CHANGELOG.md | 1 + .../src/config/settings-schema.ts | 10 ++ packages/coding-agent/src/edit/index.ts | 26 +++- .../src/lsp/diagnostics-ledger.ts | 51 ++++++++ packages/coding-agent/src/lsp/index.ts | 31 ++--- packages/coding-agent/src/lsp/utils.ts | 21 +++ packages/coding-agent/src/tools/index.ts | 4 + packages/coding-agent/src/tools/write.ts | 10 +- .../test/tools/lsp-diagnostics-dedup.test.ts | 120 ++++++++++++++++++ 9 files changed, 248 insertions(+), 26 deletions(-) create mode 100644 packages/coding-agent/src/lsp/diagnostics-ledger.ts create mode 100644 packages/coding-agent/test/tools/lsp-diagnostics-dedup.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d1215bb84..2ca63b66c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,6 +5,7 @@ - Added `ask` option descriptions so agents can keep short labels and render explanatory text as separate muted rows in the selector. - Added an extension API for rendering supplemental UI below visible assistant thinking blocks. +- Added default-on `lsp.diagnosticsDeduplicate` support so post-edit LSP diagnostics already shown for a file are suppressed within the session and only new or changed diagnostics are surfaced. ### Fixed diff --git a/packages/coding-agent/src/config/settings-schema.ts b/packages/coding-agent/src/config/settings-schema.ts index 52b3fbc7e..8397712c6 100644 --- a/packages/coding-agent/src/config/settings-schema.ts +++ b/packages/coding-agent/src/config/settings-schema.ts @@ -1966,6 +1966,16 @@ export const SETTINGS_SCHEMA = { }, }, + "lsp.diagnosticsDeduplicate": { + type: "boolean", + default: true, + ui: { + tab: "editing", + label: "Deduplicate Diagnostics", + description: "Suppress post-edit LSP diagnostics already shown for a file; only surface new or changed ones", + }, + }, + // Bash interceptor "bashInterceptor.enabled": { type: "boolean", diff --git a/packages/coding-agent/src/edit/index.ts b/packages/coding-agent/src/edit/index.ts index ec297c7be..28128c7c9 100644 --- a/packages/coding-agent/src/edit/index.ts +++ b/packages/coding-agent/src/edit/index.ts @@ -10,6 +10,7 @@ import { type WritethroughDeferredHandle, writethroughNoop, } from "../lsp"; +import { getDiagnosticsLedger } from "../lsp/diagnostics-ledger"; import applyPatchDescription from "../prompts/tools/apply-patch.md" with { type: "text" }; import patchDescription from "../prompts/tools/patch.md" with { type: "text" }; import replaceDescription from "../prompts/tools/replace.md" with { type: "text" }; @@ -102,7 +103,16 @@ function createEditWritethrough(session: ToolSession): WritethroughCallback { const enableLsp = session.enableLsp ?? true; const enableDiagnostics = enableLsp && session.settings.get("lsp.diagnosticsOnEdit"); const enableFormat = enableLsp && session.settings.get("lsp.formatOnWrite"); - return enableLsp ? createLspWritethrough(session.cwd, { enableFormat, enableDiagnostics }) : writethroughNoop; + const dedup = enableDiagnostics && session.settings.get("lsp.diagnosticsDeduplicate"); + return enableLsp + ? createLspWritethrough(session.cwd, { + enableFormat, + enableDiagnostics, + transformDiagnostics: dedup + ? (path, result) => getDiagnosticsLedger(session).reduce(path, result) + : undefined, + }) + : writethroughNoop; } /** Run apply_patch file operations and aggregate their multi-file result. */ @@ -294,6 +304,7 @@ export class EditTool implements AgentTool { readonly #fuzzyThreshold: number; readonly #writethrough: WritethroughCallback; readonly #editMode?: EditMode; + readonly #dedupDiagnostics: boolean; readonly #pendingDeferredFetches = new Map(); constructor(private readonly session: ToolSession) { @@ -306,6 +317,10 @@ export class EditTool implements AgentTool { this.#editMode = resolveConfiguredEditMode(envEditVariant); this.#allowFuzzy = resolveAllowFuzzy(session, editFuzzy); this.#fuzzyThreshold = resolveFuzzyThreshold(session, editFuzzyThreshold); + this.#dedupDiagnostics = + (session.enableLsp ?? true) && + session.settings.get("lsp.diagnosticsOnEdit") && + session.settings.get("lsp.diagnosticsDeduplicate"); this.#writethrough = createEditWritethrough(session); } @@ -495,8 +510,13 @@ export class EditTool implements AgentTool { } #injectLateDiagnostics(path: string, diagnostics: FileDiagnosticsResult): void { - const summary = diagnostics.summary ?? ""; - const lines = diagnostics.messages ?? []; + const effective = this.#dedupDiagnostics + ? getDiagnosticsLedger(this.session).reduce(path, diagnostics) + : diagnostics; + if (this.#dedupDiagnostics && effective.messages.length === 0) return; + + const summary = effective.summary ?? ""; + const lines = effective.messages ?? []; const body = [`Late LSP diagnostics for ${path} (arrived after the edit tool returned):`, summary, ...lines] .filter(Boolean) .join("\n"); diff --git a/packages/coding-agent/src/lsp/diagnostics-ledger.ts b/packages/coding-agent/src/lsp/diagnostics-ledger.ts new file mode 100644 index 000000000..715605e83 --- /dev/null +++ b/packages/coding-agent/src/lsp/diagnostics-ledger.ts @@ -0,0 +1,51 @@ +import type { FileDiagnosticsResult } from "./index"; +import { summarizeDiagnosticMessages } from "./utils"; + +const DIAGNOSTIC_LOCATION_PREFIX_RE = /^.*?:\d+:\d+\s+/; + +export function diagnosticIdentity(message: string): string { + return message.replace(DIAGNOSTIC_LOCATION_PREFIX_RE, ""); +} + +export class DiagnosticsLedger { + readonly #seen = new Map>(); + + reduce(absPath: string, result: FileDiagnosticsResult): FileDiagnosticsResult { + const previous = this.#seen.get(absPath); + const currentIdentities = new Set(); + const fresh: string[] = []; + + for (const message of result.messages) { + const identity = diagnosticIdentity(message); + currentIdentities.add(identity); + if (!previous?.has(identity)) { + fresh.push(message); + } + } + + if (currentIdentities.size === 0) { + this.#seen.delete(absPath); + } else { + this.#seen.set(absPath, currentIdentities); + } + + if (fresh.length === result.messages.length) { + return result; + } + + return { + ...result, + messages: fresh, + ...summarizeDiagnosticMessages(fresh), + }; + } +} + +export interface DiagnosticsLedgerOwner { + diagnosticsLedger?: DiagnosticsLedger; +} + +export function getDiagnosticsLedger(owner: DiagnosticsLedgerOwner): DiagnosticsLedger { + owner.diagnosticsLedger ??= new DiagnosticsLedger(); + return owner.diagnosticsLedger; +} diff --git a/packages/coding-agent/src/lsp/index.ts b/packages/coding-agent/src/lsp/index.ts index e6a851b91..0a4326502 100644 --- a/packages/coding-agent/src/lsp/index.ts +++ b/packages/coding-agent/src/lsp/index.ts @@ -79,6 +79,7 @@ import { resolveDiagnosticTargets, resolveSymbolColumn, sortDiagnostics, + summarizeDiagnosticMessages, symbolKindToIcon, uriToFile, } from "./utils"; @@ -816,12 +817,15 @@ export interface WritethroughOptions { onDeferredDiagnostics?: (diagnostics: FileDiagnosticsResult) => void; /** Signal to cancel a pending deferred diagnostics fetch. */ deferredSignal?: AbortSignal; + /** Transform diagnostics before surfacing them after a successful fetch. */ + transformDiagnostics?: (absPath: string, result: FileDiagnosticsResult) => FileDiagnosticsResult; } /** Internal resolved form of {@link WritethroughOptions} that the writethrough machinery operates on. */ type ResolvedWritethroughOptions = { enableFormat: boolean; enableDiagnostics: boolean; + transformDiagnostics?: (absPath: string, result: FileDiagnosticsResult) => FileDiagnosticsResult; }; /** Per-file deferred LSP diagnostics wiring for {@link WritethroughCallback}. */ @@ -881,6 +885,7 @@ function getOrCreateWritethroughBatch(id: string, options: ResolvedWritethroughO if (existing) { existing.options.enableFormat ||= options.enableFormat; existing.options.enableDiagnostics ||= options.enableDiagnostics; + existing.options.transformDiagnostics ??= options.transformDiagnostics; return existing; } const batch: LspWritethroughBatchState = { @@ -904,27 +909,6 @@ export async function flushLspWritethroughBatch( return flushWritethroughBatch(Array.from(state.entries.values()), cwd, state.options, signal); } -function summarizeDiagnosticMessages(messages: string[]): { summary: string; errored: boolean } { - const counts = { error: 0, warning: 0, info: 0, hint: 0 }; - for (const message of messages) { - const match = message.match(/\[(error|warning|info|hint)\]/i); - if (!match) continue; - const key = match[1].toLowerCase() as keyof typeof counts; - counts[key] += 1; - } - - const parts: string[] = []; - if (counts.error > 0) parts.push(`${counts.error} error(s)`); - if (counts.warning > 0) parts.push(`${counts.warning} warning(s)`); - if (counts.info > 0) parts.push(`${counts.info} info(s)`); - if (counts.hint > 0) parts.push(`${counts.hint} hint(s)`); - - return { - summary: parts.length > 0 ? parts.join(", ") : "no issues", - errored: counts.error > 0, - }; -} - function mergeDiagnostics( results: Array, options: ResolvedWritethroughOptions, @@ -1083,12 +1067,14 @@ async function runLspWritethrough( // 6. Get diagnostics from all servers (wait for fresh results) if (enableDiagnostics) { - diagnostics = await getDiagnosticsForFile(dst, cwd, servers, { + const fetched = await getDiagnosticsForFile(dst, cwd, servers, { signal: operationSignal, minVersions, expectedDocumentVersions, allowUnversionedLspDiagnostics: false, }); + diagnostics = + fetched && options.transformDiagnostics ? options.transformDiagnostics(dst, fetched) : fetched; } }); } catch { @@ -1155,6 +1141,7 @@ export function createLspWritethrough(cwd: string, options?: WritethroughOptions const resolvedOptions: ResolvedWritethroughOptions = { enableFormat: options?.enableFormat ?? false, enableDiagnostics: options?.enableDiagnostics ?? false, + transformDiagnostics: options?.transformDiagnostics, }; if (!resolvedOptions.enableFormat && !resolvedOptions.enableDiagnostics) { return writethroughNoop; diff --git a/packages/coding-agent/src/lsp/utils.ts b/packages/coding-agent/src/lsp/utils.ts index b74ed989b..a2678f975 100644 --- a/packages/coding-agent/src/lsp/utils.ts +++ b/packages/coding-agent/src/lsp/utils.ts @@ -221,6 +221,27 @@ export function formatDiagnosticsSummary(diagnostics: Diagnostic[]): string { return parts.length > 0 ? parts.join(", ") : "no issues"; } +export function summarizeDiagnosticMessages(messages: string[]): { summary: string; errored: boolean } { + const counts = { error: 0, warning: 0, info: 0, hint: 0 }; + for (const message of messages) { + const match = message.match(/\[(error|warning|info|hint)\]/i); + if (!match) continue; + const key = match[1].toLowerCase() as keyof typeof counts; + counts[key] += 1; + } + + const parts: string[] = []; + if (counts.error > 0) parts.push(`${counts.error} error(s)`); + if (counts.warning > 0) parts.push(`${counts.warning} warning(s)`); + if (counts.info > 0) parts.push(`${counts.info} info(s)`); + if (counts.hint > 0) parts.push(`${counts.hint} hint(s)`); + + return { + summary: parts.length > 0 ? parts.join(", ") : "no issues", + errored: counts.error > 0, + }; +} + // ============================================================================= // Location Formatting // ============================================================================= diff --git a/packages/coding-agent/src/tools/index.ts b/packages/coding-agent/src/tools/index.ts index fd5e3cd62..9adfcf986 100644 --- a/packages/coding-agent/src/tools/index.ts +++ b/packages/coding-agent/src/tools/index.ts @@ -257,6 +257,10 @@ export interface ToolSession { * by `getConflictHistory`. */ conflictHistory?: import("./conflict-detect").ConflictHistory; + /** Per-session ledger of post-edit LSP diagnostics already surfaced to the + * model for each file. Lazily initialized by `getDiagnosticsLedger`. */ + diagnosticsLedger?: import("../lsp/diagnostics-ledger").DiagnosticsLedger; + /** Queue a hidden message to be injected at the next agent turn. */ queueDeferredMessage?(message: CustomMessage): void; /** Get the active OpenTelemetry config so subagent dispatch can forward diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index 9c5bb95fb..f3f037f4d 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -15,6 +15,7 @@ import type { RenderResultOptions } from "../extensibility/custom-tools/types"; import { InternalUrlRouter } from "../internal-urls"; import { parseInternalUrl } from "../internal-urls/parse"; import { createLspWritethrough, type FileDiagnosticsResult, type WritethroughCallback, writethroughNoop } from "../lsp"; +import { getDiagnosticsLedger } from "../lsp/diagnostics-ledger"; import { getLanguageFromPath, highlightCode, type Theme } from "../modes/theme/theme"; import writeDescription from "../prompts/tools/write.md" with { type: "text" }; import type { ToolSession } from "../sdk"; @@ -278,8 +279,15 @@ export class WriteTool implements AgentTool getDiagnosticsLedger(session).reduce(path, result) + : undefined, + }) : writethroughNoop; this.description = prompt.render(writeDescription); } diff --git a/packages/coding-agent/test/tools/lsp-diagnostics-dedup.test.ts b/packages/coding-agent/test/tools/lsp-diagnostics-dedup.test.ts new file mode 100644 index 000000000..156d3ed10 --- /dev/null +++ b/packages/coding-agent/test/tools/lsp-diagnostics-dedup.test.ts @@ -0,0 +1,120 @@ +import { describe, expect, it } from "bun:test"; +import type { FileDiagnosticsResult } from "@oh-my-pi/pi-coding-agent/lsp"; +import { DiagnosticsLedger, diagnosticIdentity } from "@oh-my-pi/pi-coding-agent/lsp/diagnostics-ledger"; + +const FILE_A = "/repo/src/a.ts"; +const FILE_B = "/repo/src/b.ts"; + +const TYPE_ERROR = + 'src/a.ts:12:5 [error] pyright: Type "str" is not assignable to declared type "int" (reportAssignmentType)'; +const TYPE_ERROR_SHIFTED = + 'src/a.ts:48:19 [error] pyright: Type "str" is not assignable to declared type "int" (reportAssignmentType)'; +const PRIVATE_IMPORT = + 'src/a.ts:20:7 [warning] pyright: "device" is not exported from module "torch" (reportPrivateImportUsage)'; +const PRIVATE_IMPORT_SHIFTED = + 'src/a.ts:58:11 [warning] pyright: "device" is not exported from module "torch" (reportPrivateImportUsage)'; +const NEW_ERROR = + 'src/a.ts:60:3 [error] pyright: Cannot access attribute "missing" for class "Widget" (reportAttributeAccessIssue)'; + +function makeDiagnostics( + messages: string[], + options: { summary?: string; errored?: boolean; server?: string } = {}, +): FileDiagnosticsResult { + return { + server: options.server ?? "pyright", + messages, + summary: options.summary ?? (messages.length === 0 ? "OK" : `${messages.length} diagnostic(s)`), + errored: options.errored ?? messages.some(message => message.includes("[error]")), + }; +} + +describe("DiagnosticsLedger", () => { + it("returns all messages unchanged the first time a file is reduced", () => { + const ledger = new DiagnosticsLedger(); + const first = makeDiagnostics([TYPE_ERROR, PRIVATE_IMPORT], { summary: "1 error(s), 1 warning(s)" }); + + const reduced = ledger.reduce(FILE_A, first); + + expect(reduced).toBe(first); + expect(reduced.messages).toEqual([TYPE_ERROR, PRIVATE_IMPORT]); + }); + + it("fully suppresses an identical second reduce", () => { + const ledger = new DiagnosticsLedger(); + ledger.reduce(FILE_A, makeDiagnostics([TYPE_ERROR, PRIVATE_IMPORT])); + + const reduced = ledger.reduce(FILE_A, makeDiagnostics([TYPE_ERROR, PRIVATE_IMPORT])); + + expect(reduced.messages).toEqual([]); + expect(reduced.summary).toBe("no issues"); + expect(reduced.errored).toBe(false); + }); + + it("suppresses diagnostics whose line and column shifted", () => { + const ledger = new DiagnosticsLedger(); + ledger.reduce(FILE_A, makeDiagnostics([TYPE_ERROR, PRIVATE_IMPORT])); + + const reduced = ledger.reduce(FILE_A, makeDiagnostics([TYPE_ERROR_SHIFTED, PRIVATE_IMPORT_SHIFTED])); + + expect(reduced.messages).toEqual([]); + }); + + it("returns only genuinely new messages and recomputes summary state", () => { + const ledger = new DiagnosticsLedger(); + ledger.reduce(FILE_A, makeDiagnostics([TYPE_ERROR, PRIVATE_IMPORT])); + + const reduced = ledger.reduce( + FILE_A, + makeDiagnostics([TYPE_ERROR_SHIFTED, PRIVATE_IMPORT_SHIFTED, NEW_ERROR], { + summary: "2 error(s), 1 warning(s)", + }), + ); + + expect(reduced.messages).toEqual([NEW_ERROR]); + expect(reduced.summary).toBe("1 error(s)"); + expect(reduced.errored).toBe(true); + expect(reduced.server).toBe("pyright"); + }); + + it("re-surfaces a diagnostic after it was removed", () => { + const ledger = new DiagnosticsLedger(); + ledger.reduce(FILE_A, makeDiagnostics([TYPE_ERROR])); + ledger.reduce(FILE_A, makeDiagnostics([])); + + const reduced = ledger.reduce(FILE_A, makeDiagnostics([TYPE_ERROR])); + + expect(reduced.messages).toEqual([TYPE_ERROR]); + }); + + it("tracks files independently", () => { + const ledger = new DiagnosticsLedger(); + ledger.reduce(FILE_A, makeDiagnostics([TYPE_ERROR])); + + const reduced = ledger.reduce(FILE_B, makeDiagnostics([TYPE_ERROR])); + + expect(reduced.messages).toEqual([TYPE_ERROR]); + }); +}); + +describe("diagnosticIdentity", () => { + it("strips path, line, and column while preserving diagnostic identity", () => { + const first = "fixtures/pkg:2/example.ts:12:5 [error] pyright: Broken import (E1)"; + const shifted = "fixtures/pkg:2/example.ts:99:27 [error] pyright: Broken import (E1)"; + + expect(diagnosticIdentity(first)).toBe("[error] pyright: Broken import (E1)"); + expect(diagnosticIdentity(shifted)).toBe(diagnosticIdentity(first)); + }); + + it("distinguishes severity and code changes", () => { + const base = diagnosticIdentity("src/a.ts:1:1 [error] pyright: Broken import (E1)"); + + expect(diagnosticIdentity("src/a.ts:1:1 [warning] pyright: Broken import (E1)")).not.toBe(base); + expect(diagnosticIdentity("src/a.ts:1:1 [error] pyright: Broken import (E2)")).not.toBe(base); + }); + + it("falls back to the full message when the prefix is unparseable", () => { + const message = "pyright: Broken import (E1)"; + + expect(diagnosticIdentity(message)).toBe(message); + }); +});