diff --git a/.changeset/cross-file-duplicate-merge.md b/.changeset/cross-file-duplicate-merge.md new file mode 100644 index 00000000..21a47c6a --- /dev/null +++ b/.changeset/cross-file-duplicate-merge.md @@ -0,0 +1,24 @@ +--- +"review": minor +--- + +review: merge one source's identical finding across files into a single pattern-level comment + +Both dedup tiers require the same path and different sources by design: they +merge agreement between reviewers, not repetition by one reviewer. Measured on +Khan/webapp#41440, one source posted byte-identical documentation suggestions +on two sibling eval files as two comments (inline comments 3764122555 and +3764122558), and no existing rule could reach the pair. + +A new pass (`dedup-crossfile.ts`) runs after the cross-source tiers settle: +findings from the same source with the same label on different files merge +when their text is identical, or near-identical above the strict +different-line similarity floor (equal line numbers in different files never +buy the laxer exact-anchor tier). The survivor is the first occurrence in +diff order; its discussion gains one trailing line naming the other +occurrences (path, and line where known); merged copies skip validation and +posting and are recorded in the run artifact under `crossFileMerges`. Label +equality keeps the verdict arithmetic unchanged: a merged blocking group +keeps one blocking claim and floors the verdict exactly once. Any doubt in +similarity posts separately; a missed merge costs a duplicate comment, a +wrong one drops a finding. diff --git a/workflows/review/lib/dedup-crossfile.test.ts b/workflows/review/lib/dedup-crossfile.test.ts new file mode 100644 index 00000000..b7f8aa82 --- /dev/null +++ b/workflows/review/lib/dedup-crossfile.test.ts @@ -0,0 +1,374 @@ +import {describe, it, expect} from "vitest"; + +import { + mergeCrossFileDuplicates, + reapplyCrossFileOccurrences, + suppressThenMergeCrossFile, +} from "./dedup-crossfile"; +import {type Claim} from "./dispatch-contracts"; + +/** + * Cross-file duplicate merge: one source's same finding on several files. + * The exact-duplicate fixtures reproduce the measured shape from + * Khan/webapp#41440 (inline comments 3764122555 and 3764122558: byte-identical + * documentation suggestions on two sibling eval YAML files). + */ + +const EXERCISE = + "services/ai-guide/eval/tut/DiagramTriggeringT1Exercise.eval.yaml"; +const TUTOR_ME = + "services/ai-guide/eval/tut/DiagramTriggeringT1TutorMe.eval.yaml"; + +const claim = (overrides: Partial): Claim => ({ + id: "documentation-1", + source: "documentation", + path: EXERCISE, + line: 12, + label: "suggestion (non-blocking, documentation)", + subject: "Comment names a v1 variant the versions block does not define.", + discussion: + "The header comment says the file compares a v1 variant against v2, " + + "but the versions block defines only v0 and v2, so the comment " + + "promises a comparison the eval never runs and the next editor has " + + "to reverse-engineer which label is stale.", + failure_scenario: + "A reader trusts the comment, looks for the v1 variant, and edits " + + "the wrong versions entry.", + confidence: 0.8, + ...overrides, +}); + +const pair = (): Claim[] => [ + claim({id: "documentation-1", path: EXERCISE, line: 12}), + claim({id: "documentation-2", path: TUTOR_ME, line: 139}), +]; + +describe("mergeCrossFileDuplicates", () => { + it("merges an exact cross-file duplicate into the first diff-order occurrence", () => { + const result = mergeCrossFileDuplicates(pair(), [EXERCISE, TUTOR_ME]); + expect(result.claims).toHaveLength(1); + const survivor = result.claims[0]; + expect(survivor.id).toBe("documentation-1"); + expect(survivor.path).toBe(EXERCISE); + expect(survivor.discussion).toContain( + `Also applies to \`${TUTOR_ME}\` (line 139).`, + ); + expect(result.merges).toEqual([ + { + survivor: "documentation-1", + source: "documentation", + label: "suggestion (non-blocking, documentation)", + path: EXERCISE, + line: 12, + merged: [{id: "documentation-2", path: TUTOR_ME, line: 139}], + via: "cross-file", + }, + ]); + }); + + it("elects the survivor by diff order, not claim order", () => { + // The reviewer reported TutorMe first, but Exercise comes first in + // the diff; the comment must land where a reader meets the pattern. + const claims = [ + claim({id: "documentation-1", path: TUTOR_ME, line: 139}), + claim({id: "documentation-2", path: EXERCISE, line: 12}), + ]; + const result = mergeCrossFileDuplicates(claims, [EXERCISE, TUTOR_ME]); + expect(result.claims).toHaveLength(1); + expect(result.claims[0].id).toBe("documentation-2"); + expect(result.claims[0].path).toBe(EXERCISE); + }); + + it("falls back to claim order when no diff order is provided", () => { + const result = mergeCrossFileDuplicates(pair()); + expect(result.claims).toHaveLength(1); + expect(result.claims[0].id).toBe("documentation-1"); + }); + + it("merges a near-identical pair that clears the strict floor", () => { + // Same prose with one clause reworded: still far above the + // different-line floor (high token overlap, many shared bigrams). + const reworded = claim({ + id: "documentation-2", + path: TUTOR_ME, + line: 139, + discussion: + "The header comment says the file compares a v1 variant " + + "against v2, but the versions block defines only v0 and v2, " + + "so the comment promises a comparison the eval never runs " + + "and the stale label misleads the next editor.", + }); + const result = mergeCrossFileDuplicates( + [claim({}), reworded], + [EXERCISE, TUTOR_ME], + ); + expect(result.claims).toHaveLength(1); + expect(result.merges).toHaveLength(1); + }); + + it("does not merge below the similarity floor", () => { + const different = claim({ + id: "documentation-2", + path: TUTOR_ME, + line: 139, + subject: "Stale version labels across the new eval files.", + discussion: + "This file's versionKey is v1-prompt while the sibling " + + "diagram evals use v2; inconsistent naming will confuse " + + "anyone comparing runs across the suite.", + failure_scenario: + "An engineer filters eval dashboards by version key and " + + "misses half the runs.", + }); + const result = mergeCrossFileDuplicates( + [claim({}), different], + [EXERCISE, TUTOR_ME], + ); + expect(result.claims).toHaveLength(2); + expect(result.merges).toHaveLength(0); + }); + + it("never buys the lax exact-anchor floor with coincidentally equal lines", () => { + // Two moderately similar claims at the SAME line number in different + // files: below the strict different-line floor, and the equal lines + // must not downgrade to the exact-anchor tier and merge them. + // This pair sits BETWEEN the floors: it clears the exact-anchor + // floor when the lines read as equal, and fails the different-line + // floor when they are stripped (verified against describesSameDefect + // directly when the fixture was built). + const a = claim({ + id: "documentation-1", + path: EXERCISE, + line: 42, + subject: "Stale promptVersion comment in the eval header.", + discussion: + "The versions block comment references promptVersion v0 " + + "gating for markdown tables.", + failure_scenario: "Editor misreads the versions block comment.", + }); + const b = claim({ + id: "documentation-2", + path: TUTOR_ME, + line: 42, + subject: "Stale promptVersion comment in the eval header.", + discussion: + "The versions block comment references promptVersion gating " + + "the fragment never defines.", + failure_scenario: + "Editor trusts the header and edits the wrong file.", + }); + const result = mergeCrossFileDuplicates([a, b], [EXERCISE, TUTOR_ME]); + expect(result.claims).toHaveLength(2); + expect(result.merges).toHaveLength(0); + }); + + it("never merges across labels, even with identical bodies", () => { + const claims = [ + claim({id: "documentation-1"}), + claim({ + id: "documentation-2", + path: TUTOR_ME, + line: 139, + label: "note (non-blocking)", + }), + ]; + const result = mergeCrossFileDuplicates(claims, [EXERCISE, TUTOR_ME]); + expect(result.claims).toHaveLength(2); + expect(result.merges).toHaveLength(0); + }); + + it("never merges across sources, even with identical bodies", () => { + const claims = [ + claim({id: "documentation-1"}), + claim({ + id: "skill-auditor-1", + source: "skill-auditor", + path: TUTOR_ME, + line: 139, + }), + ]; + const result = mergeCrossFileDuplicates(claims, [EXERCISE, TUTOR_ME]); + expect(result.claims).toHaveLength(2); + expect(result.merges).toHaveLength(0); + }); + + it("leaves same-path pairs to tier 1", () => { + const claims = [ + claim({id: "documentation-1"}), + claim({id: "documentation-2", line: 40}), + ]; + const result = mergeCrossFileDuplicates(claims, [EXERCISE]); + expect(result.claims).toHaveLength(2); + expect(result.merges).toHaveLength(0); + }); + + it("skips unanchored claims", () => { + const claims = [ + claim({id: "documentation-1", line: undefined}), + claim({id: "documentation-2", path: TUTOR_ME, line: 139}), + ]; + const result = mergeCrossFileDuplicates(claims, [EXERCISE, TUTOR_ME]); + expect(result.claims).toHaveLength(2); + expect(result.merges).toHaveLength(0); + }); + + it("keeps exactly one blocking claim when a blocking group merges", () => { + const blocking = (id: string, path: string, line: number): Claim => + claim({id, path, line, label: "issue (blocking)"}); + const result = mergeCrossFileDuplicates( + [ + blocking("correctness-1", EXERCISE, 5), + blocking("correctness-2", TUTOR_ME, 9), + ], + [EXERCISE, TUTOR_ME], + ); + expect(result.claims).toHaveLength(1); + expect(result.claims[0].label).toBe("issue (blocking)"); + expect(result.merges[0].merged).toHaveLength(1); + }); + + it("merges a three-file group into one comment listing both other occurrences", () => { + const third = + "services/ai-guide/eval/tut/DiagramTriggeringT1Video.eval.yaml"; + const claims = [ + claim({id: "documentation-1", path: EXERCISE, line: 12}), + claim({id: "documentation-2", path: TUTOR_ME, line: 139}), + claim({id: "documentation-3", path: third, line: 77}), + ]; + const result = mergeCrossFileDuplicates(claims, [ + EXERCISE, + TUTOR_ME, + third, + ]); + expect(result.claims).toHaveLength(1); + expect(result.claims[0].discussion).toContain( + `Also applies to \`${TUTOR_ME}\` (line 139), \`${third}\` (line 77).`, + ); + expect(result.merges[0].merged.map((copy) => copy.id)).toEqual([ + "documentation-2", + "documentation-3", + ]); + }); + + it("does not chain distinct findings through a bridging claim", () => { + // The label mismatch keeps C out of the group before the star + // guard runs; the bigram-chain case below is the one that reaches + // the guard's filtering branch. + const claims = [ + claim({id: "documentation-1", path: EXERCISE, line: 12}), + claim({id: "documentation-2", path: TUTOR_ME, line: 139}), + claim({ + id: "documentation-3", + path: TUTOR_ME, + line: 139, + label: "note (non-blocking)", + }), + ]; + const result = mergeCrossFileDuplicates(claims, [EXERCISE, TUTOR_ME]); + expect(result.claims).toHaveLength(2); + expect(result.claims.map((kept) => kept.id)).toEqual([ + "documentation-1", + "documentation-3", + ]); + }); + + it("keeps a bridged member as its own claim (the star guard as a filter)", () => { + // A genuine similarity chain: bridge B clears the near-identical + // floor against A (shared subject) and against C (B's failure + // scenario is C's subject), while A and C share no content tokens. + // Union-find chains all three into one group; the star guard must + // merge only B into A's survivor and leave C standing, not drop or + // wrongly merge it. + const shared = + "Retry loop rereads deletion cursor after every batch completes."; + const bridgeText = + "Pagination token resets whenever filters change between successive requests."; + const third = + "services/ai-guide/eval/tut/DiagramTriggeringT1Video.eval.yaml"; + const claims = [ + claim({ + id: "documentation-1", + path: EXERCISE, + line: 12, + subject: shared, + failure_scenario: + "Logging sink swallows write errors during shutdown flush window.", + }), + claim({ + id: "documentation-2", + path: TUTOR_ME, + line: 139, + subject: shared, + failure_scenario: bridgeText, + }), + claim({ + id: "documentation-3", + path: third, + line: 77, + subject: bridgeText, + failure_scenario: + "Metrics counter increments twice inside recovered panic handler path.", + }), + ]; + const result = mergeCrossFileDuplicates(claims, [ + EXERCISE, + TUTOR_ME, + third, + ]); + expect(result.claims.map((kept) => kept.id)).toEqual([ + "documentation-1", + "documentation-3", + ]); + expect(result.claims[0].discussion).toContain( + `Also applies to \`${TUTOR_ME}\` (line 139).`, + ); + expect(result.claims[0].discussion).not.toContain(third); + expect(result.merges).toHaveLength(1); + expect(result.merges[0].merged.map((copy) => copy.id)).toEqual([ + "documentation-2", + ]); + }); +}); + +describe("suppressThenMergeCrossFile", () => { + it("degrades to claim order when the staging is malformed", () => { + // The docstring promises a malformed files.json degrades to claim + // order, never to a skipped merge. Thread stagings are equally raw + // (absent here), and absent corpora suppress nothing. + const result = suppressThenMergeCrossFile(pair(), [], [], new Set(), { + not: "an array", + }); + expect(result.claims).toHaveLength(1); + expect(result.claims[0].id).toBe("documentation-1"); + expect(result.crossFileMerges).toHaveLength(1); + }); + + it("degrades to claim order when entries lack string paths", () => { + const result = suppressThenMergeCrossFile(pair(), [], [], new Set(), [ + {path: 42}, + "not-a-record", + {}, + ]); + expect(result.claims).toHaveLength(1); + expect(result.crossFileMerges).toHaveLength(1); + }); + + it("re-applies the occurrence list a corrected discussion erased", () => { + const merged = mergeCrossFileDuplicates(pair(), [EXERCISE, TUTOR_ME]); + const corrected = merged.claims.map((kept) => ({ + ...kept, + discussion: "Validator-corrected prose without the list.", + })); + const repaired = reapplyCrossFileOccurrences(corrected, merged.merges); + expect(repaired[0].discussion).toBe( + "Validator-corrected prose without the list." + + `\n\nAlso applies to \`${TUTOR_ME}\` (line 139).`, + ); + // A survivor whose discussion kept the line is left alone. + expect( + reapplyCrossFileOccurrences(merged.claims, merged.merges), + ).toEqual(merged.claims); + // A dropped survivor stays dropped. + expect(reapplyCrossFileOccurrences([], merged.merges)).toEqual([]); + }); +}); diff --git a/workflows/review/lib/dedup-crossfile.ts b/workflows/review/lib/dedup-crossfile.ts new file mode 100644 index 00000000..a68de3e7 --- /dev/null +++ b/workflows/review/lib/dedup-crossfile.ts @@ -0,0 +1,316 @@ +/** + * Cross-file duplicate merge: ONE source posting the SAME finding on several + * files collapses to one comment. + * + * Both existing dedup tiers require the same path and different sources by + * design (dedup.ts, dedup-cluster.ts): they merge AGREEMENT, several + * reviewers landing on one defect. This pass merges REPETITION, one reviewer + * stamping the same finding across sibling files. Measured on Khan/webapp + * PR 41440: inline comments 3764122555 and 3764122558 are byte-identical + * 566-character documentation suggestions on two sibling eval YAML files, + * posted by one source in one run; in-run clustering could not reach them + * because every rule it has forbids exactly this shape. + * + * Rules, all of them narrower than the tiers this rides beside: + * + * - **Same source and same label, exactly.** A source does not disagree with + * itself about severity, so a label mismatch means the findings differ in + * ask even when their prose is close; and merging across sources is the + * other tiers' job, with their own calibration. The label equality also + * makes the verdict arithmetic trivial: a merged blocking group keeps one + * blocking claim, so it floors the verdict exactly once, and a wrong merge + * can never soften a verdict (the survivor carries the same label every + * copy did). + * - **Different paths, both anchored.** A same-path pair belongs to tier 1; + * an unanchored (pr-level) claim has no occurrence list to speak of. + * - **Identical text, or near-identical above the STRICT floor.** Identical + * subject/discussion/failure-scenario text merges outright (the measured + * 41440 shape; short identical bodies must not fail on a bigram count). + * Anything else must clear `describesSameDefect` with the claims' lines + * stripped, which forces the stricter OTHER_LINE_FLOOR: equal line numbers + * in different files are coincidence, not anchor evidence, and must not + * buy the laxer exact-anchor tier. Any doubt posts separately, matching + * the dedup philosophy (a missed merge costs a duplicate comment; a wrong + * one drops a finding). + * + * Validation asymmetry, accepted deliberately: the validator sees only the + * survivor, and its verdict speaks for copies anchored on OTHER files whose + * contents it never re-checked. The cross-source tiers have the same + * one-validation-per-group shape without the cross-file exposure (their + * copies share a path). What bounds the cost here: every copy passed the + * change-provenance gate on its own anchor before dedup ran, the merge + * floors above demand identical-or-near-identical text from ONE source, and + * the occurrence list is prose ("Also applies to..."), not an anchored + * claim, so a wrong merge publishes a weaker statement about the other file + * than a separate comment would have. A refuted survivor drops its whole + * group; that is the same failure direction as tier 1 and costs a missed + * comment only when the SAME text was somehow valid on the sibling file. + * + * The survivor is the first occurrence in diff order (the staged files.json + * order, when provided; claim order otherwise), so the comment lands where a + * reader meets the pattern first. Its discussion gains one trailing line + * naming the other occurrences (path, and line where known); those + * occurrences are prose, not anchors, so the change-provenance gate applies + * to the survivor's own anchor only, which every copy already passed + * individually before dedup ran. Merged-away copies are recorded for + * dispatch-result.json like the other tiers' merges. + */ + +import { + describesSameDefect, + type stagedThreadShapeFailure, + type ThreadSuppression, +} from "./dedup"; +import {suppressTrackedDuplicates} from "./dedup-adjudicated"; +import {isRecord, type Claim} from "./dispatch-contracts"; + +/** + * The occurrence list a merge appends to its survivor's discussion. One + * renderer for the merge site and {@link reapplyCrossFileOccurrences}, so + * the re-apply check can never miss the line over a formatting drift. + */ +const alsoAppliesLine = ( + occurrences: readonly {path?: string; line?: number}[], +): string => + `\n\nAlso applies to ${occurrences + .map((occurrence) => + occurrence.line === undefined + ? `\`${occurrence.path}\`` + : `\`${occurrence.path}\` (line ${occurrence.line})`, + ) + .join(", ")}.`; + +/** One cross-file merge, for the run artifact (`crossFileMerges`). */ +export type CrossFileMerge = { + survivor: string; + source: string; + label: string; + path: string; + line: number; + merged: {id: string; path: string; line?: number}[]; + via: "cross-file"; +}; + +/** The normalized text identity of a claim, for the exact-equality fast path. */ +const exactKey = (claim: Claim): string => + [claim.subject, claim.discussion, claim.failure_scenario] + .join("\n") + .replace(/\s+/g, " ") + .trim(); + +/** + * Whether two same-source, same-label claims on different files carry the + * same finding. Exact text equality merges outright; otherwise the pair must + * clear the similarity floors with lines stripped, so the strict + * different-line tier applies whatever the line numbers are. + */ +const sameCrossFileFinding = (a: Claim, b: Claim): boolean => + exactKey(a) === exactKey(b) || + describesSameDefect({...a, line: undefined}, {...b, line: undefined}); + +const mergeableAcrossFiles = (a: Claim, b: Claim): boolean => + a.source === b.source && + a.label === b.label && + a.path !== undefined && + b.path !== undefined && + a.path !== b.path && + a.line !== undefined && + b.line !== undefined && + sameCrossFileFinding(a, b); + +/** + * Merge one source's cross-file duplicates. Runs AFTER `dedupeClaims` (the + * cross-source tiers settle first, so a survivor here already speaks for + * whatever they folded into it) and AFTER open-thread suppression, but + * before validation, so merged copies are still neither separately + * validated nor separately posted (suppression also precedes validation, so + * the cost saving is identical). + * + * The position after suppression is load-bearing, not stylistic. + * `bestOpenThreadMatch` only matches a thread to a claim on the thread's own + * path, so if this merge ran first, an open thread on the survivor's file + * would suppress the survivor and silently drop every other file's + * occurrence with it: an author who copies a flawed file A into a new + * sibling B, with A's finding already tracked in an open thread, would never + * hear about B, on this run or any later one. Running after suppression, A's + * copy exits through the thread and B posts alone. The inverse cost, an open + * thread that was itself a merged comment already naming B, is one duplicate + * comment on B: the failure direction this module's rules already prefer. + * + * `pathOrder` is the diff's file order (staged files.json); the survivor is + * the group's first occurrence in that order, with claim order breaking ties + * and standing in entirely when the order is absent or does not know a path. + */ +export const mergeCrossFileDuplicates = ( + claims: Claim[], + pathOrder: readonly string[] = [], +): {claims: Claim[]; merges: CrossFileMerge[]} => { + const orderOf = new Map(); + pathOrder.forEach((path, index) => { + if (!orderOf.has(path)) { + orderOf.set(path, index); + } + }); + const rank = (index: number): [number, number] => { + const path = claims[index].path; + return [ + path !== undefined && orderOf.has(path) + ? (orderOf.get(path) as number) + : Number.MAX_SAFE_INTEGER, + index, + ]; + }; + const firstInDiffOrder = (indexA: number, indexB: number): number => { + const [pathA, claimA] = rank(indexA); + const [pathB, claimB] = rank(indexB); + if (pathA !== pathB) { + return pathA < pathB ? indexA : indexB; + } + return claimA < claimB ? indexA : indexB; + }; + + // Union-find over pairwise-mergeable claims, mirroring tier 1's shape. + const parent = claims.map((_, index) => index); + const find = (index: number): number => { + while (parent[index] !== index) { + parent[index] = parent[parent[index]]; + index = parent[index]; + } + return index; + }; + for (let i = 0; i < claims.length; i += 1) { + for (let j = i + 1; j < claims.length; j += 1) { + if (mergeableAcrossFiles(claims[i], claims[j])) { + parent[find(j)] = find(i); + } + } + } + const groups = new Map(); + claims.forEach((_, index) => { + const root = find(index); + groups.set(root, [...(groups.get(root) ?? []), index]); + }); + + const drop = new Set(); + const replacement = new Map(); + const merges: CrossFileMerge[] = []; + for (const group of [...groups.values()].sort((a, b) => a[0] - b[0])) { + if (group.length < 2) { + continue; + } + const survivorIndex = group.reduce(firstInDiffOrder); + const survivor = claims[survivorIndex]; + // Star guard, as in tier 1: only a copy mergeable against the + // survivor itself merges. Union-find alone chains A~B~C through a + // bridging claim, and collapsing the chain would silently drop a + // finding the survivor's text does not cover. + const merged = group + .filter( + (index) => + index !== survivorIndex && + mergeableAcrossFiles(survivor, claims[index]), + ) + .sort((a, b) => (firstInDiffOrder(a, b) === a ? -1 : 1)); + if (merged.length === 0) { + continue; + } + for (const index of merged) { + drop.add(index); + } + replacement.set(survivorIndex, { + ...survivor, + discussion: `${survivor.discussion}${alsoAppliesLine( + merged.map((index) => claims[index]), + )}`, + }); + merges.push({ + survivor: survivor.id, + source: survivor.source, + label: survivor.label, + path: survivor.path as string, + line: survivor.line as number, + merged: merged.map((index) => { + const claim = claims[index]; + return { + id: claim.id, + path: claim.path as string, + ...(claim.line !== undefined ? {line: claim.line} : {}), + }; + }), + via: "cross-file", + }); + } + return { + claims: claims + .map((claim, index) => replacement.get(index) ?? claim) + .filter((_, index) => !drop.has(index)), + merges, + }; +}; + +/** + * The composed suppression-then-merge step dispatch calls, in this order + * because the ordering is load-bearing (see {@link mergeCrossFileDuplicates}): + * both thread-suppression passes first ({@link suppressTrackedDuplicates}: + * the open corpus, then the adjudicated one), so a suppressed file's copy + * exits through its thread and the other files' occurrences still post; the + * cross-file merge second, over the survivors. `stagedOpen`, + * `stagedAdjudicated`, and `stagedFiles` are the raw staged values; a + * missing or malformed files.json degrades to claim order, never to a + * skipped merge. + */ +export const suppressThenMergeCrossFile = ( + claims: Claim[], + stagedOpen: unknown, + stagedAdjudicated: unknown, + resolvedIds: ReadonlySet, + stagedFiles: unknown, +): { + claims: Claim[]; + suppressed: ThreadSuppression[]; + shapeFailure: ReturnType; + crossFileMerges: CrossFileMerge[]; +} => { + const suppression = suppressTrackedDuplicates( + claims, + stagedOpen, + stagedAdjudicated, + resolvedIds, + ); + const pathOrder = (Array.isArray(stagedFiles) ? stagedFiles : []) + .map((entry) => (isRecord(entry) ? entry["path"] : undefined)) + .filter((path): path is string => typeof path === "string"); + const crossFile = mergeCrossFileDuplicates(suppression.kept, pathOrder); + return { + claims: crossFile.claims, + suppressed: suppression.suppressed, + shapeFailure: suppression.shapeFailure, + crossFileMerges: crossFile.merges, + }; +}; + +/** + * Re-append each merged survivor's occurrence list after validation. A + * validator `corrected.discussion` replaces the survivor's free text + * wholesale (applyVerifications), which silently erased the "Also applies + * to" line and with it every merged-away file's finding from the posted + * output. The merge record carries the occurrences, so the line is re-built + * from data rather than preserved by hope; a survivor whose discussion + * still carries the exact line (validator confirmed without correcting) is + * left alone, and a survivor validation dropped stays dropped (the + * documented group-drop failure direction). + */ +export const reapplyCrossFileOccurrences = ( + claims: Claim[], + merges: readonly CrossFileMerge[], +): Claim[] => { + const lineFor = new Map( + merges.map((merge) => [merge.survivor, alsoAppliesLine(merge.merged)]), + ); + return claims.map((claim) => { + const line = lineFor.get(claim.id); + return line === undefined || claim.discussion.includes(line) + ? claim + : {...claim, discussion: `${claim.discussion}${line}`}; + }); +}; diff --git a/workflows/review/lib/dispatch-crossfile.test.ts b/workflows/review/lib/dispatch-crossfile.test.ts new file mode 100644 index 00000000..90c90708 --- /dev/null +++ b/workflows/review/lib/dispatch-crossfile.test.ts @@ -0,0 +1,336 @@ +import {describe, it, expect} from "vitest"; + +import {runDispatch, type AgentRunner, type DispatchFs} from "./dispatch"; +import {computeDiffProvenance} from "./provenance"; + +/** + * Cross-file duplicate merge at the dispatch level: one source's identical + * finding on two files collapses to one claim, the merged copy skips + * validation and posting, and the run artifact records the merge. Fixtures + * mirror dispatch-cluster.test.ts; the duplicate shape is Khan/webapp#41440's + * (byte-identical documentation suggestions on sibling files). + */ + +const REVIEW = "/tmp/gh-aw/review"; +const AGENTS = "/work/.claude/agents"; + +const makeFakeFs = ( + files: Record = {}, +): DispatchFs & {files: Record} => { + const state = {...files}; + return { + files: state, + readFileSync: (p: string) => { + if (!(p in state)) { + throw new Error(`ENOENT: ${p}`); + } + return state[p]; + }, + writeFileSync: (p: string, data: string) => { + state[p] = data; + }, + existsSync: (p: string) => + p in state || Object.keys(state).some((f) => f.startsWith(`${p}/`)), + mkdirSync: () => {}, + readdirSync: (p: string) => { + const prefix = `${p}/`; + return [ + ...new Set( + Object.keys(state) + .filter((f) => f.startsWith(prefix)) + .map((f) => f.slice(prefix.length).split("/")[0]), + ), + ]; + }, + }; +}; + +const agentFile = (name: string): string => + `---\nname: ${name}\ndescription: d\nmodel: claude-opus-4-8\n---\nYou are ${name}. Read from disk and return JSON.`; + +const agentFiles = (...names: string[]): Record => + Object.fromEntries( + names.map((name) => [`${AGENTS}/${name}.md`, agentFile(name)]), + ); + +const stubRunner = ( + outputs: Record, +): AgentRunner & {calls: string[]} => { + const calls: string[] = []; + const runner = (async (request) => { + calls.push(request.name); + const output = outputs[request.name]; + if (output === undefined) { + throw new Error(`no canned output for ${request.name}`); + } + return {output, usd: 0.5, turns: 3, wallMs: 100}; + }) as AgentRunner & {calls: string[]}; + runner.calls = calls; + return runner; +}; + +// Two sibling files, each with one added line, in a fixed diff order. +const DIFF = [ + "diff --git a/a.ts b/a.ts", + "--- a/a.ts", + "+++ b/a.ts", + "@@ -1,2 +1,3 @@", + " ctx", + "+added line", + " ctx", + "diff --git a/b.ts b/b.ts", + "--- a/b.ts", + "+++ b/b.ts", + "@@ -1,2 +1,3 @@", + " ctx", + "+added line", + " ctx", + "", +].join("\n"); + +const baseStaging = (): Record => ({ + [`${REVIEW}/routing.json`]: JSON.stringify({ + enabledReviewers: [], + lensesToSpawn: [], + runBudget: {maxReviewerInvocations: 6, tier: "High"}, + }), + [`${REVIEW}/rereview-plan.json`]: JSON.stringify({depth: "full"}), + [`${REVIEW}/full.diff`]: DIFF, + [`${REVIEW}/files.json`]: JSON.stringify([ + {path: "a.ts", status: "modified", hasPatch: true}, + {path: "b.ts", status: "modified", hasPatch: true}, + ]), + [`${REVIEW}/provenance.json`]: JSON.stringify(computeDiffProvenance(DIFF)), +}); + +const finding = (path: string) => ({ + path, + line: 2, + label: "suggestion (non-blocking)", + subject: "Comment names a v1 variant the versions block does not define.", + discussion: + "The header comment says the file compares a v1 variant against v2, but the versions block defines only v0 and v2, so the comment promises a comparison that never runs.", + failure_scenario: + "A reader trusts the comment and edits the wrong versions entry.", +}); + +describe("runDispatch cross-file duplicate merge", () => { + it("collapses one source's identical finding on two files and records the merge", async () => { + const fs = makeFakeFs({ + ...baseStaging(), + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-validator", + ), + }); + const runner = stubRunner({ + "pattern-triage": JSON.stringify({ + patterns: [], + reviewFiles: ["a.ts", "b.ts"], + }), + "correctness-reviewer": JSON.stringify({ + findings: [finding("a.ts"), finding("b.ts")], + }), + "skill-auditor": JSON.stringify({findings: []}), + "claim-validator": JSON.stringify({ + claims: [ + { + id: "correctness-reviewer-1", + verification: "confirmed", + confidence: 0.9, + }, + ], + }), + }); + const result = await runDispatch({fs, runner, repoRoot: "/work"}); + + // One claim survives, anchored on the diff's first file, with the + // other occurrence carried in prose. + expect(result.claims).toHaveLength(1); + expect(result.claims[0].path).toBe("a.ts"); + expect(result.claims[0].discussion).toContain( + "Also applies to `b.ts` (line 2).", + ); + + // The merge is in the artifact, and the validator saw ONE claim. + expect(result.crossFileMerges).toEqual([ + { + survivor: "correctness-reviewer-1", + source: "correctness-reviewer", + label: "suggestion (non-blocking)", + path: "a.ts", + line: 2, + merged: [{id: "correctness-reviewer-2", path: "b.ts", line: 2}], + via: "cross-file", + }, + ]); + const staged = JSON.parse(fs.files[`${REVIEW}/claims.json`]); + expect(staged).toHaveLength(1); + const artifact = JSON.parse(fs.files[`${REVIEW}/dispatch-result.json`]); + expect(artifact.crossFileMerges).toHaveLength(1); + }); + + it("keeps the occurrence list when the validator corrects the survivor's discussion", async () => { + // applyVerifications replaces discussion wholesale on a corrected + // verdict, which erased the merge's "Also applies to" line and lost + // the sibling file's finding from every posted surface. + const fs = makeFakeFs({ + ...baseStaging(), + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-validator", + ), + }); + const runner = stubRunner({ + "pattern-triage": JSON.stringify({ + patterns: [], + reviewFiles: ["a.ts", "b.ts"], + }), + "correctness-reviewer": JSON.stringify({ + findings: [finding("a.ts"), finding("b.ts")], + }), + "skill-auditor": JSON.stringify({findings: []}), + "claim-validator": JSON.stringify({ + claims: [ + { + id: "correctness-reviewer-1", + verification: "confirmed", + confidence: 0.9, + corrected: { + discussion: + "Corrected: the versions block defines only v0 and v2.", + }, + }, + ], + }), + }); + const result = await runDispatch({fs, runner, repoRoot: "/work"}); + expect(result.claims).toHaveLength(1); + expect(result.claims[0].discussion).toBe( + "Corrected: the versions block defines only v0 and v2." + + "\n\nAlso applies to `b.ts` (line 2).", + ); + }); + + it("posts the new file's copy when an open thread suppresses the survivor's file", async () => { + // Regression for the merge-before-suppression loss case: an open bot + // thread tracks the finding on a.ts (the author copied flawed a.ts + // into sibling b.ts); the source re-finds it on both files. Thread + // suppression only matches a claim on the thread's own path, so a + // cross-file merge running FIRST would collapse b.ts into a.ts's + // survivor and suppression would then drop b.ts's occurrence with + // it, on this run and every later one. Order is + // suppression-then-merge: a.ts exits through the thread, b.ts posts + // alone. + const fs = makeFakeFs({ + ...baseStaging(), + [`${REVIEW}/threads.json`]: JSON.stringify([ + { + thread_id: "T1", + path: "a.ts", + resolved: false, + comments: [ + { + author: "github-actions", + body: `**suggestion (non-blocking):** ${ + finding("a.ts").subject + }\n\n${finding("a.ts").discussion}`, + }, + ], + }, + ]), + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-validator", + ), + }); + const runner = stubRunner({ + "pattern-triage": JSON.stringify({ + patterns: [], + reviewFiles: ["a.ts", "b.ts"], + }), + "correctness-reviewer": JSON.stringify({ + findings: [finding("a.ts"), finding("b.ts")], + }), + "skill-auditor": JSON.stringify({findings: []}), + "claim-validator": JSON.stringify({ + claims: [ + { + id: "correctness-reviewer-2", + verification: "confirmed", + confidence: 0.9, + }, + ], + }), + }); + const result = await runDispatch({fs, runner, repoRoot: "/work"}); + + // b.ts's occurrence posts on its own anchor, unmerged and without + // the prose occurrence list. + expect(result.claims).toHaveLength(1); + expect(result.claims[0].path).toBe("b.ts"); + expect(result.claims[0].discussion).not.toContain("Also applies to"); + expect(result.crossFileMerges).toEqual([]); + + // a.ts's copy exited through the open thread, recorded as such. + expect(result.threadSuppressions).toHaveLength(1); + expect(result.threadSuppressions[0]).toMatchObject({ + path: "a.ts", + thread_id: "T1", + }); + }); + + it("posts both when the same source's findings differ", async () => { + const fs = makeFakeFs({ + ...baseStaging(), + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-validator", + ), + }); + const other = { + path: "b.ts", + line: 2, + label: "suggestion (non-blocking)", + subject: "Unbounded read on the response body.", + discussion: + "io.ReadAll on the response with no size cap; a large body exhausts memory.", + failure_scenario: "A multi-gigabyte response OOMs the process.", + }; + const runner = stubRunner({ + "pattern-triage": JSON.stringify({ + patterns: [], + reviewFiles: ["a.ts", "b.ts"], + }), + "correctness-reviewer": JSON.stringify({ + findings: [finding("a.ts"), other], + }), + "skill-auditor": JSON.stringify({findings: []}), + "claim-validator": JSON.stringify({ + claims: [ + { + id: "correctness-reviewer-1", + verification: "confirmed", + confidence: 0.9, + }, + { + id: "correctness-reviewer-2", + verification: "confirmed", + confidence: 0.9, + }, + ], + }), + }); + const result = await runDispatch({fs, runner, repoRoot: "/work"}); + expect(result.claims).toHaveLength(2); + expect(result.crossFileMerges).toEqual([]); + }); +}); diff --git a/workflows/review/lib/dispatch.ts b/workflows/review/lib/dispatch.ts index c984f022..e9508d19 100644 --- a/workflows/review/lib/dispatch.ts +++ b/workflows/review/lib/dispatch.ts @@ -40,7 +40,11 @@ */ import {dedupeClaims, type ClaimMerge, type ThreadSuppression} from "./dedup"; -import {suppressTrackedDuplicates} from "./dedup-adjudicated"; +import { + reapplyCrossFileOccurrences, + suppressThenMergeCrossFile, + type CrossFileMerge, +} from "./dedup-crossfile"; import { clusteringRecord, runClusterStep, @@ -253,6 +257,8 @@ export type DispatchResult = { claims: Claim[]; /** Cross-source duplicates merged before validation (#245). */ merges: ClaimMerge[]; + /** One source's same finding on several files, collapsed (dedup-crossfile.ts). */ + crossFileMerges: CrossFileMerge[]; /** * Dedup tier 2's audit block, present when the clusterer was dispatched * (absent when there was nothing to cluster: fewer than two claims, or one @@ -796,17 +802,8 @@ export const runDispatch = async ( } } - // Cross-source duplicate merge (#245), BEFORE validation so duplicate - // claims are neither separately validated (the largest sub-agent cost - // line) nor separately posted. - // - // Tier 2 (the claim-clusterer) runs here, between the fan-out and - // validation, for the same reason: run 30587343777 paid to validate four - // copies of one wrong doc comment and posted all four. Its input is the - // pre-merge candidate set — the model sees what tier 1 would collapse - // anyway, which costs a few hundred tokens and keeps the merge decision in - // ONE place (dedup.ts folds both tiers into one group, so a survivor gains - // one "also flagged by" note rather than a stack of them). + // Duplicate merge, BEFORE validation so duplicates are neither validated + // nor posted; dedup.ts's header carries the rationale and measured runs. const candidateClaims = buildClaims(scoped.kept); const clusterStep = await runClusterStep(candidateClaims, { dispatch: dispatchAgent, @@ -817,6 +814,7 @@ export const runDispatch = async ( // eslint-disable-next-line no-console warn: (message) => console.error(message), }); + // Tiers 1-2; the cross-file pass runs after suppression (see dedup-crossfile.ts). const deduped = dedupeClaims(candidateClaims, clusterStep.proposals); const clustering = clusteringRecord( clusterStep, @@ -835,15 +833,18 @@ export const runDispatch = async ( // re-flag at blocking severity must stay visible). Every filter and guard // lives in dedup.ts / dedup-adjudicated.ts beside the rules it enforces; // a producer bug or an older staging without adjudicated-threads.json - // degrades to a duplicate comment, never to a dropped finding. - const suppression = suppressTrackedDuplicates( + // degrades to a duplicate comment, never to a dropped finding. The + // cross-file merge runs AFTER both passes, inside the composed step + // (dedup-crossfile.ts carries the ordering rationale). + const dedupStep = suppressThenMergeCrossFile( claims, threads, readJson(fs, `${REVIEW_DIR}/adjudicated-threads.json`), new Set(reconciliation?.resolve ?? []), + readJson(fs, `${REVIEW_DIR}/files.json`), ); - claims = suppression.kept; - const threadSuppressionUnavailable = suppression.shapeFailure; + claims = dedupStep.claims; + const threadSuppressionUnavailable = dedupStep.shapeFailure; if (threadSuppressionUnavailable !== undefined) { // eslint-disable-next-line no-console console.error(threadSuppressionUnavailable.warning); @@ -878,6 +879,10 @@ export const runDispatch = async ( cause: "unavailable", }); } + // A validator corrected.discussion replaces a survivor's free text + // wholesale, erasing the merge's "Also applies to" line; re-build + // it from the merge records so no correction loses an occurrence. + claims = reapplyCrossFileOccurrences(claims, dedupStep.crossFileMerges); } const dispatched = [ @@ -908,9 +913,9 @@ export const runDispatch = async ( "Note: change-provenance gate skipped this run (diff staging unparseable).", ] : []), - ...(suppression.suppressed.length > 0 + ...(dedupStep.suppressed.length > 0 ? [ - `Note: ${suppression.suppressed.length} finding(s) not re-posted (already tracked in open review threads).`, + `Note: ${dedupStep.suppressed.length} finding(s) not re-posted (already tracked in open review threads).`, ] : []), ]; @@ -924,8 +929,9 @@ export const runDispatch = async ( noteLines, claims, merges: deduped.merges, + crossFileMerges: dedupStep.crossFileMerges, ...(clustering !== undefined ? {clustering} : {}), - threadSuppressions: suppression.suppressed, + threadSuppressions: dedupStep.suppressed, ...(threadSuppressionUnavailable !== undefined ? {threadSuppressionUnavailable} : {}),