From 5bc28c6b4f857de637ad1f5c0270083460d415c6 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Wed, 12 Aug 2026 16:39:25 -0400 Subject: [PATCH 1/3] [jwies/review-crossfile-cluster] review: merge one source's identical finding across files into one pattern-level comment --- .changeset/cross-file-duplicate-merge.md | 24 ++ workflows/review/lib/dedup-crossfile.test.ts | 274 ++++++++++++++++++ workflows/review/lib/dedup-crossfile.ts | 240 +++++++++++++++ .../review/lib/dispatch-crossfile.test.ts | 222 ++++++++++++++ workflows/review/lib/dispatch.ts | 28 +- 5 files changed, 775 insertions(+), 13 deletions(-) create mode 100644 .changeset/cross-file-duplicate-merge.md create mode 100644 workflows/review/lib/dedup-crossfile.test.ts create mode 100644 workflows/review/lib/dedup-crossfile.ts create mode 100644 workflows/review/lib/dispatch-crossfile.test.ts 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..6c81dfca --- /dev/null +++ b/workflows/review/lib/dedup-crossfile.test.ts @@ -0,0 +1,274 @@ +import {describe, it, expect} from "vitest"; + +import {mergeCrossFileDuplicates} 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", () => { + // A and C are each near-identical to bridge B but not to each other + // is hard to fabricate with exact texts, so pin the star guard with + // the exact-equality path: A == B on one axis, B == C impossible + // there. Instead verify the guard directly: a group member not + // mergeable against the survivor stays its own claim. Construct it + // by making B identical to A, and C identical to B except for the + // label, which keeps C out of the group entirely. + 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", + ]); + }); +}); diff --git a/workflows/review/lib/dedup-crossfile.ts b/workflows/review/lib/dedup-crossfile.ts new file mode 100644 index 00000000..523138ec --- /dev/null +++ b/workflows/review/lib/dedup-crossfile.ts @@ -0,0 +1,240 @@ +/** + * 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). + * + * 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 {dedupeClaims, describesSameDefect, type ClaimMerge} from "./dedup"; +import {isRecord, type Claim, type ProposedCluster} from "./dispatch-contracts"; +import {type ClusterRejection} from "./dedup-cluster"; + +/** 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 before open-thread suppression and + * validation, so merged copies are neither separately validated nor + * separately posted. + * + * `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); + } + const occurrence = (claim: Claim): string => + claim.line === undefined + ? `\`${claim.path}\`` + : `\`${claim.path}\` (line ${claim.line})`; + const alsoApplies = `\n\nAlso applies to ${merged + .map((index) => occurrence(claims[index])) + .join(", ")}.`; + replacement.set(survivorIndex, { + ...survivor, + discussion: `${survivor.discussion}${alsoApplies}`, + }); + 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 dedup entry point dispatch calls: the cross-source tiers + * (`dedupeClaims`) settle first, then the cross-file pass runs over their + * survivors, so this pass can only ever remove comments the tiers left + * standing, never a merge they made. `stagedFiles` is the raw parsed + * files.json (diff order); a missing or malformed staging degrades to claim + * order, never to a skipped merge. + */ +export const dedupeClaimsWithCrossFile = ( + claims: Claim[], + proposals: readonly ProposedCluster[], + stagedFiles: unknown, +): { + claims: Claim[]; + merges: ClaimMerge[]; + clusterRejections: ClusterRejection[]; + crossFileMerges: CrossFileMerge[]; +} => { + const deduped = dedupeClaims(claims, proposals); + const pathOrder = (Array.isArray(stagedFiles) ? stagedFiles : []) + .map((entry) => (isRecord(entry) ? entry["path"] : undefined)) + .filter((path): path is string => typeof path === "string"); + const crossFile = mergeCrossFileDuplicates(deduped.claims, pathOrder); + return { + claims: crossFile.claims, + merges: deduped.merges, + clusterRejections: deduped.clusterRejections, + crossFileMerges: crossFile.merges, + }; +}; diff --git a/workflows/review/lib/dispatch-crossfile.test.ts b/workflows/review/lib/dispatch-crossfile.test.ts new file mode 100644 index 00000000..ded88ce1 --- /dev/null +++ b/workflows/review/lib/dispatch-crossfile.test.ts @@ -0,0 +1,222 @@ +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("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 0d70494b..915fb119 100644 --- a/workflows/review/lib/dispatch.ts +++ b/workflows/review/lib/dispatch.ts @@ -40,13 +40,16 @@ */ import { - dedupeClaims, openThreadsFromStaged, stagedThreadShapeFailure, suppressOpenThreadDuplicates, type ClaimMerge, type ThreadSuppression, } from "./dedup"; +import { + dedupeClaimsWithCrossFile, + type CrossFileMerge, +} from "./dedup-crossfile"; import { clusteringRecord, runClusterStep, @@ -258,6 +261,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 @@ -801,17 +806,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, @@ -822,7 +818,12 @@ export const runDispatch = async ( // eslint-disable-next-line no-console warn: (message) => console.error(message), }); - const deduped = dedupeClaims(candidateClaims, clusterStep.proposals); + // Tiers 1-2, then the cross-file pass (dedup-crossfile.ts). + const deduped = dedupeClaimsWithCrossFile( + candidateClaims, + clusterStep.proposals, + readJson(fs, `${REVIEW_DIR}/files.json`), + ); const clustering = clusteringRecord( clusterStep, candidateClaims.length, @@ -929,6 +930,7 @@ export const runDispatch = async ( noteLines, claims, merges: deduped.merges, + crossFileMerges: deduped.crossFileMerges, ...(clustering !== undefined ? {clustering} : {}), threadSuppressions: suppression.suppressed, ...(threadSuppressionUnavailable !== undefined From b456f9f05b1199709b086690bb58b60c0641372a Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Wed, 12 Aug 2026 18:47:29 -0400 Subject: [PATCH 2/3] [jwies/review-crossfile-cluster] review: run the cross-file merge after open-thread suppression Review finding on #348: suppression only matches a thread to a claim on the thread's own path, so merging first let an open thread on the survivor's file swallow every other file's occurrence (author copies flawed a.ts to b.ts, a.ts tracked by an open thread: b.ts never posts, on any run). Order is now suppress-then-merge, composed and documented in dedup-crossfile.ts (suppressThenMergeCrossFile) with a dispatch-level regression test. Also documents the accepted validation asymmetry the re-review asked about (survivor-only validation across files). --- workflows/review/lib/dedup-crossfile.ts | 64 ++++++++++++----- .../review/lib/dispatch-crossfile.test.ts | 70 +++++++++++++++++++ workflows/review/lib/dispatch.ts | 28 ++++---- 3 files changed, 130 insertions(+), 32 deletions(-) diff --git a/workflows/review/lib/dedup-crossfile.ts b/workflows/review/lib/dedup-crossfile.ts index 523138ec..29a7fdc3 100644 --- a/workflows/review/lib/dedup-crossfile.ts +++ b/workflows/review/lib/dedup-crossfile.ts @@ -33,6 +33,19 @@ * 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 @@ -43,9 +56,13 @@ * dispatch-result.json like the other tiers' merges. */ -import {dedupeClaims, describesSameDefect, type ClaimMerge} from "./dedup"; -import {isRecord, type Claim, type ProposedCluster} from "./dispatch-contracts"; -import {type ClusterRejection} from "./dedup-cluster"; +import { + describesSameDefect, + suppressOpenThreadDuplicates, + type OpenThread, + type ThreadSuppression, +} from "./dedup"; +import {isRecord, type Claim} from "./dispatch-contracts"; /** One cross-file merge, for the run artifact (`crossFileMerges`). */ export type CrossFileMerge = { @@ -88,9 +105,21 @@ const mergeableAcrossFiles = (a: Claim, b: Claim): boolean => /** * 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 before open-thread suppression and - * validation, so merged copies are neither separately validated nor - * separately posted. + * 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 @@ -209,32 +238,31 @@ export const mergeCrossFileDuplicates = ( }; /** - * The composed dedup entry point dispatch calls: the cross-source tiers - * (`dedupeClaims`) settle first, then the cross-file pass runs over their - * survivors, so this pass can only ever remove comments the tiers left - * standing, never a merge they made. `stagedFiles` is the raw parsed + * The composed suppression-then-merge step dispatch calls, in this order + * because the ordering is load-bearing (see {@link mergeCrossFileDuplicates}): + * open-thread suppression first, 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. `stagedFiles` is the raw parsed * files.json (diff order); a missing or malformed staging degrades to claim * order, never to a skipped merge. */ -export const dedupeClaimsWithCrossFile = ( +export const suppressThenMergeCrossFile = ( claims: Claim[], - proposals: readonly ProposedCluster[], + openThreads: readonly OpenThread[], stagedFiles: unknown, ): { claims: Claim[]; - merges: ClaimMerge[]; - clusterRejections: ClusterRejection[]; + suppressed: ThreadSuppression[]; crossFileMerges: CrossFileMerge[]; } => { - const deduped = dedupeClaims(claims, proposals); + const suppression = suppressOpenThreadDuplicates(claims, openThreads); const pathOrder = (Array.isArray(stagedFiles) ? stagedFiles : []) .map((entry) => (isRecord(entry) ? entry["path"] : undefined)) .filter((path): path is string => typeof path === "string"); - const crossFile = mergeCrossFileDuplicates(deduped.claims, pathOrder); + const crossFile = mergeCrossFileDuplicates(suppression.kept, pathOrder); return { claims: crossFile.claims, - merges: deduped.merges, - clusterRejections: deduped.clusterRejections, + suppressed: suppression.suppressed, crossFileMerges: crossFile.merges, }; }; diff --git a/workflows/review/lib/dispatch-crossfile.test.ts b/workflows/review/lib/dispatch-crossfile.test.ts index ded88ce1..3a241b0f 100644 --- a/workflows/review/lib/dispatch-crossfile.test.ts +++ b/workflows/review/lib/dispatch-crossfile.test.ts @@ -172,6 +172,76 @@ describe("runDispatch cross-file duplicate merge", () => { expect(artifact.crossFileMerges).toHaveLength(1); }); + 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(), diff --git a/workflows/review/lib/dispatch.ts b/workflows/review/lib/dispatch.ts index 915fb119..0ca1cece 100644 --- a/workflows/review/lib/dispatch.ts +++ b/workflows/review/lib/dispatch.ts @@ -40,14 +40,14 @@ */ import { + dedupeClaims, openThreadsFromStaged, stagedThreadShapeFailure, - suppressOpenThreadDuplicates, type ClaimMerge, type ThreadSuppression, } from "./dedup"; import { - dedupeClaimsWithCrossFile, + suppressThenMergeCrossFile, type CrossFileMerge, } from "./dedup-crossfile"; import { @@ -818,12 +818,8 @@ export const runDispatch = async ( // eslint-disable-next-line no-console warn: (message) => console.error(message), }); - // Tiers 1-2, then the cross-file pass (dedup-crossfile.ts). - const deduped = dedupeClaimsWithCrossFile( - candidateClaims, - clusterStep.proposals, - readJson(fs, `${REVIEW_DIR}/files.json`), - ); + // Tiers 1-2; the cross-file pass runs after suppression (see dedup-crossfile.ts). + const deduped = dedupeClaims(candidateClaims, clusterStep.proposals); const clustering = clusteringRecord( clusterStep, candidateClaims.length, @@ -843,8 +839,12 @@ export const runDispatch = async ( // finding. const resolvedIds = new Set(reconciliation?.resolve ?? []); const openThreads = openThreadsFromStaged(threads, resolvedIds); - const suppression = suppressOpenThreadDuplicates(claims, openThreads); - claims = suppression.kept; + const dedupStep = suppressThenMergeCrossFile( + claims, + openThreads, + readJson(fs, `${REVIEW_DIR}/files.json`), + ); + claims = dedupStep.claims; const threadSuppressionUnavailable = stagedThreadShapeFailure( threads, openThreads, @@ -914,9 +914,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).`, ] : []), ]; @@ -930,9 +930,9 @@ export const runDispatch = async ( noteLines, claims, merges: deduped.merges, - crossFileMerges: deduped.crossFileMerges, + crossFileMerges: dedupStep.crossFileMerges, ...(clustering !== undefined ? {clustering} : {}), - threadSuppressions: suppression.suppressed, + threadSuppressions: dedupStep.suppressed, ...(threadSuppressionUnavailable !== undefined ? {threadSuppressionUnavailable} : {}), From dbc211d234c2ff16da8c6e964d451d47955c733a Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Wed, 12 Aug 2026 19:38:55 -0400 Subject: [PATCH 3/3] [jwies/review-crossfile-cluster] review: exercise the star guard's filtering branch; pin staging degradation Re-review notes on #348: the chain test excluded its third claim by label before grouping, so the star guard never ran as a filter; a bigram-chain fixture (A~B and B~C above the floor, A~C below) now reaches it and pins that a bridged member stays its own claim. Direct tests cover suppressThenMergeCrossFile's malformed-staging fallback. The un-merge-on-refutation question stays as designed: a refuted survivor drops its group (documented in the header). --- workflows/review/lib/dedup-crossfile.test.ts | 95 ++++++++++++++++++-- 1 file changed, 87 insertions(+), 8 deletions(-) diff --git a/workflows/review/lib/dedup-crossfile.test.ts b/workflows/review/lib/dedup-crossfile.test.ts index 6c81dfca..857b21f2 100644 --- a/workflows/review/lib/dedup-crossfile.test.ts +++ b/workflows/review/lib/dedup-crossfile.test.ts @@ -1,6 +1,9 @@ import {describe, it, expect} from "vitest"; -import {mergeCrossFileDuplicates} from "./dedup-crossfile"; +import { + mergeCrossFileDuplicates, + suppressThenMergeCrossFile, +} from "./dedup-crossfile"; import {type Claim} from "./dispatch-contracts"; /** @@ -247,13 +250,9 @@ describe("mergeCrossFileDuplicates", () => { }); it("does not chain distinct findings through a bridging claim", () => { - // A and C are each near-identical to bridge B but not to each other - // is hard to fabricate with exact texts, so pin the star guard with - // the exact-equality path: A == B on one axis, B == C impossible - // there. Instead verify the guard directly: a group member not - // mergeable against the survivor stays its own claim. Construct it - // by making B identical to A, and C identical to B except for the - // label, which keeps C out of the group entirely. + // 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}), @@ -271,4 +270,84 @@ describe("mergeCrossFileDuplicates", () => { "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. + const result = suppressThenMergeCrossFile(pair(), [], { + 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(), + [], + [{path: 42}, "not-a-record", {}], + ); + expect(result.claims).toHaveLength(1); + expect(result.crossFileMerges).toHaveLength(1); + }); });