From 82631ed633c230693aa089579a106317f43b3dc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20-=20=E3=82=A2=E3=83=AC=E3=83=83=E3=82=AF=E3=82=B9?= Date: Wed, 23 Sep 2026 09:44:52 +0200 Subject: [PATCH] fix(composer): key docs fingerprints to the file, not the hunk A finding on a docs path fingerprinted on its hunk id, which is a hash of diff geometry. Any later push to the file re-keyed the finding, its posted marker no longer matched, and the same unfixed defect opened a second thread. Code keeps its enclosing symbol across pushes; prose has no symbol table, so the file is the finest locator that survives an edit elsewhere in it. - Distinct same-category findings in one docs file now share a thread. --- src/pipeline/composer.ts | 9 +++ tests/pipeline-phase5.test.ts | 143 ++++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+) diff --git a/src/pipeline/composer.ts b/src/pipeline/composer.ts index eb9e791..b72abe9 100644 --- a/src/pipeline/composer.ts +++ b/src/pipeline/composer.ts @@ -1815,7 +1815,16 @@ function fingerprintFinding(finding: CandidateFinding, packetsById: Map): string { + if (isDocsPath(finding.path)) { + return FILE_SCOPE_IDENTITY; + } const packet = packetsById.get(finding.producedBy.packetId); const hunkId = finding.anchor?.hunkId ?? inferredHunkId(finding, packet); const symbol = hunkId !== undefined ? symbolForHunk(packet, hunkId) : uniquePacketSymbol(packet); diff --git a/tests/pipeline-phase5.test.ts b/tests/pipeline-phase5.test.ts index 9c0ba82..8a890e8 100644 --- a/tests/pipeline-phase5.test.ts +++ b/tests/pipeline-phase5.test.ts @@ -10107,6 +10107,77 @@ describe("phase 5 pipeline regressions", () => { expect(secondFinal?.fingerprint).toBe(expected); }); + it("keeps docs fingerprints stable when a later push shifts the hunk", async () => { + const docsPath = "docs/plans/service-rollout/design.md"; + const tableRow = "| `POST /widgets` | write | `widgetCreated` |"; + const docsPacket = (hunkId: string, line: number): ReviewPacket => ({ + ...fakePacket({ path: docsPath }), + id: `packet-${hunkId}`, + language: "markdown", + hunks: [ + { + hunkId, + oldStart: line, + oldLines: 1, + newStart: line, + newLines: 1, + contentWithLineNumbers: ` ${line} ${line} +${tableRow}`, + lines: [{ kind: "add", content: tableRow, newLine: line }], + changedNewLineNumbers: [line], + changedOldLineNumbers: [] + } + ] + }); + const docsFinding = (hunkId: string, line: number): CandidateFinding => ({ + ...fakeFinding(), + path: docsPath, + anchor: { path: docsPath, line, side: "RIGHT", hunkId }, + evidence: { changedCode: tableRow }, + producedBy: { ...fakeFinding().producedBy, packetId: `packet-${hunkId}` } + }); + const compose = async (hunkId: string, line: number) => dedupeRankAndComposeReview( + { verified: [docsFinding(hunkId, line)], verdicts: [] }, + fakePlanForHunks([hunkId], docsPath), + { + mode: "branch", + repoRoot: "/tmp/repo", + commits: [], + rawDiff: "" + }, + { + totalHunks: 1, + reviewedHunks: 1, + skippedHunks: 0, + failedHunks: 0, + coverageByLevel: { deep: 0, normal: 1, light: 0, skip: 0 }, + degradedPlanning: false, + budgetStopped: false, + verificationIncompleteCount: 0, + partial: false, + reasons: [] + }, + config(), + nullTelemetry(), + { + runner: { + runStructured: async () => { + throw composerTransientError(); + } + }, + promptBuilder: fakePromptBuilder(), + packets: [docsPacket(hunkId, line)] + } + ); + + const beforePush = await compose("hunk-at-81", 81); + const afterPush = await compose("hunk-at-74", 74); + const expected = sha256Hex([docsPath, "", "correctness", "core/code-review"].join("\0")); + const [before] = [...beforePush.findings, ...beforePush.summaryOnlyFindings]; + const [after] = [...afterPush.findings, ...afterPush.summaryOnlyFindings]; + expect(before?.fingerprint).toBe(after?.fingerprint); + expect(before?.fingerprint).toBe(expected); + }); + it("does not merge unanchored findings from different hunks in one coalesced packet", async () => { const coalescedPacket: ReviewPacket = { ...fakePacket(), @@ -10187,6 +10258,78 @@ describe("phase 5 pipeline regressions", () => { expect(result.summaryOnlyFindings.map((finding) => finding.mergedCandidateIds)).toEqual([["finding-1"], ["finding-2"]]); }); + it("merges same-category docs findings from one file onto a single thread", async () => { + const docsPath = "docs/design.md"; + const docsPacket: ReviewPacket = { + ...fakePacket({ path: docsPath }), + kind: "coalesced-hunks", + language: "markdown", + hunks: [ + { + hunkId: "h1", + oldStart: 1, + oldLines: 1, + newStart: 1, + newLines: 1, + contentWithLineNumbers: " 1 1 +alpha claim", + lines: [{ kind: "add", content: "alpha claim", newLine: 1 }], + changedNewLineNumbers: [1], + changedOldLineNumbers: [] + }, + { + hunkId: "h2", + oldStart: 20, + oldLines: 1, + newStart: 20, + newLines: 1, + contentWithLineNumbers: " 20 20 +beta claim", + lines: [{ kind: "add", content: "beta claim", newLine: 20 }], + changedNewLineNumbers: [20], + changedOldLineNumbers: [] + } + ] + }; + const { anchor: _anchor, ...base } = fakeFinding(); + const first = { ...base, path: docsPath, changedLine: false, evidence: { changedCode: "alpha claim" } }; + const second = { ...first, id: "finding-2", title: "second docs claim", evidence: { changedCode: "beta claim" } }; + const result = await dedupeRankAndComposeReview( + { verified: [first, second], verdicts: [] }, + fakePlanForHunks(["h1", "h2"], docsPath), + { + mode: "branch", + repoRoot: "/tmp/repo", + commits: [], + rawDiff: "" + }, + { + totalHunks: 2, + reviewedHunks: 2, + skippedHunks: 0, + failedHunks: 0, + coverageByLevel: { deep: 0, normal: 2, light: 0, skip: 0 }, + degradedPlanning: false, + budgetStopped: false, + verificationIncompleteCount: 0, + partial: false, + reasons: [] + }, + { ...config(), review: { ...config().review, maxFindings: 100, softCommentCap: 100 } }, + nullTelemetry(), + { + runner: { + runStructured: async () => { + throw composerTransientError(); + } + }, + promptBuilder: fakePromptBuilder(), + packets: [docsPacket] + } + ); + + expect(result.summaryOnlyFindings).toHaveLength(1); + expect(result.summaryOnlyFindings[0]?.mergedCandidateIds).toEqual(["finding-1", "finding-2"]); + }); + it("drops composed groups when any referenced finding id is unknown", async () => { const events: Array> = []; const result = await dedupeRankAndComposeReview(