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.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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<TInput> {
|
||||
readonly #fuzzyThreshold: number;
|
||||
readonly #writethrough: WritethroughCallback;
|
||||
readonly #editMode?: EditMode;
|
||||
readonly #dedupDiagnostics: boolean;
|
||||
readonly #pendingDeferredFetches = new Map<string, AbortController>();
|
||||
|
||||
constructor(private readonly session: ToolSession) {
|
||||
@@ -306,6 +317,10 @@ export class EditTool implements AgentTool<TInput> {
|
||||
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<TInput> {
|
||||
}
|
||||
|
||||
#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");
|
||||
|
||||
@@ -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<string, Set<string>>();
|
||||
|
||||
reduce(absPath: string, result: FileDiagnosticsResult): FileDiagnosticsResult {
|
||||
const previous = this.#seen.get(absPath);
|
||||
const currentIdentities = new Set<string>();
|
||||
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;
|
||||
}
|
||||
@@ -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<FileDiagnosticsResult | undefined>,
|
||||
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;
|
||||
|
||||
@@ -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
|
||||
// =============================================================================
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<typeof writeSchema, WriteToolDetails
|
||||
const enableLsp = session.enableLsp ?? true;
|
||||
const enableFormat = enableLsp && session.settings.get("lsp.formatOnWrite");
|
||||
const enableDiagnostics = enableLsp && session.settings.get("lsp.diagnosticsOnWrite");
|
||||
const dedup = enableDiagnostics && session.settings.get("lsp.diagnosticsDeduplicate");
|
||||
this.#writethrough = enableLsp
|
||||
? createLspWritethrough(session.cwd, { enableFormat, enableDiagnostics })
|
||||
? createLspWritethrough(session.cwd, {
|
||||
enableFormat,
|
||||
enableDiagnostics,
|
||||
transformDiagnostics: dedup
|
||||
? (path, result) => getDiagnosticsLedger(session).reduce(path, result)
|
||||
: undefined,
|
||||
})
|
||||
: writethroughNoop;
|
||||
this.description = prompt.render(writeDescription);
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user