From 5ed38c43a0ee5bd9cdde056bbd0c494c099afaaa Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Fri, 31 Jul 2026 10:28:48 -0700 Subject: [PATCH 1/8] [jwies/defect-clustering] review: cluster candidate claims by defect identity, not by anchor Four sources flagged one wrong doc comment in run 30587343777 (webapp#41204, a FIRST review at depth: full) and `merges` recorded none of them; autofix later satisfied all four with a single rewritten comment. Replaying that run's claims.json shows the similarity tier is nowhere near reaching it: three of the four share the EXACT anchor and still score 0.060-0.068 Jaccard against a 0.14 floor with 0-1 shared bigrams against a floor of 4. Nor can reweighting the text recover the discriminator, because the pairs dedup deliberately keeps apart share MORE salient tokens than the real duplicates do (an AddDate arithmetic bug and "this behavior is never exercised" on one line share AddDate, MemoryTTLDays, 180, 15). Duplicates are "same ask, different words"; those are "same facts, different ask". So identity moves to the defect: a `claim-clusterer` sub-agent names the groups, dedup.ts verifies them and owns every merge rule. No line agreement is required, which is what makes the same-defect-different-anchor shape mergeable at all. The model must ground each group in a code element its members share, and that is checked; only a non-blocking copy may be absorbed on a model's word, so a false merge costs an advisory comment and never a blocking finding or a verdict. The live A/B now runs dedup (it never did, so this was unmeasurable by construction). Tier 1 runs in both arms; tier 2 rides each arm's own review.md, so the delta prices the clusterer alone and a false merge lands as recall loss. Merge counts are reported per arm and recorded in dispatch-result.json's new `clustering` block. --- .changeset/review-defect-clustering.md | 68 +++ workflows/review/eval/README.md | 13 + workflows/review/eval/live-ab-report.ts | 55 +- workflows/review/eval/live-ab.ts | 19 + workflows/review/eval/live-producer.test.ts | 132 +++++ workflows/review/eval/live-producer.ts | 171 ++++++ workflows/review/lib/dedup-cluster.test.ts | 504 ++++++++++++++++++ workflows/review/lib/dedup.test.ts | 11 +- workflows/review/lib/dedup.ts | 426 ++++++++++++++- workflows/review/lib/dispatch-cluster.test.ts | 342 ++++++++++++ workflows/review/lib/dispatch-cluster.ts | 117 ++++ .../review/lib/dispatch-contracts.test.ts | 62 +++ workflows/review/lib/dispatch-contracts.ts | 62 ++- workflows/review/lib/dispatch.test.ts | 5 + workflows/review/lib/dispatch.ts | 54 +- workflows/review/review.md | 68 ++- 16 files changed, 2075 insertions(+), 34 deletions(-) create mode 100644 .changeset/review-defect-clustering.md create mode 100644 workflows/review/lib/dedup-cluster.test.ts create mode 100644 workflows/review/lib/dispatch-cluster.test.ts create mode 100644 workflows/review/lib/dispatch-cluster.ts diff --git a/.changeset/review-defect-clustering.md b/.changeset/review-defect-clustering.md new file mode 100644 index 00000000..acfaba7c --- /dev/null +++ b/.changeset/review-defect-clustering.md @@ -0,0 +1,68 @@ +--- +"review": minor +--- + +Cross-source dedup gains a second tier: a `claim-clusterer` sub-agent names the +candidate comments that describe ONE defect, and `dedup.ts` verifies that +assertion and merges them. Several reviewers finding one problem now post once. + +Run 30587343777 (webapp#41204) is the case. Four sources flagged one wrong doc +comment (`// Keeps at most 10 samples per key.` above `const maxSamples = 25`) at +window.go :8, :9, :8, :8, and `merges` recorded none of them; it was a FIRST +review at `depth: full`, so not a re-review artifact. Autofix later satisfied all +four with one rewritten comment, which is the proof they were one defect. + +Replaying that run's own claims.json showed the similarity tier is not close to +reaching it. Three of the four share the EXACT anchor and still score +0.060-0.068 Jaccard against a 0.14 floor with 0-1 shared bigrams against a floor +of 4: an order of magnitude below the tier, so no re-derivation from the fixtures +gets there (the floor that admits 0.06 admits everything). Each reviewer wrote +the same defect in different words, and the terser the claim the less text +arithmetic has to work with. Nor can reweighting the text recover the +discriminator: the pairs dedup deliberately keeps apart share MORE salient +tokens than the real duplicates do (run 29943085279's AddDate issue and its +"central behavior never exercised" thought sit on one line and share AddDate, +MemoryTTLDays, 180, 15). Duplicates are "same ask, different words"; those are +"same facts, different ask", which is a semantic judgment. + +So the unit of identity is now the defect, not the anchor. Tier 2 requires no +line agreement at all, which is what makes the same-defect-different-anchor shape +mergeable for the first time (one missing-test defect drew comments at three +anchors in run 29943085279); the line survives as tier-1 evidence and as the +survivor's posting anchor. + +The model contributes identity only. Every merge rule stays in code, and the +clusterer must ground each group in the code element its members share, which is +then checked: a group whose `evidence` names no identifier, literal, or quoted +text is discarded, and so is a member whose own text never mentions it. Same +path and different sources are enforced as in tier 1, and only a NON-BLOCKING +copy may be absorbed on a model's word — with the survivor always the +highest-severity copy, a false tier-2 merge can cost an advisory comment and can +never lose a blocking finding or soften a verdict. The accepted price: one defect +flagged blocking by two sources in different words still posts twice unless tier +1 reaches it. + +Degradation is soft in both directions. Fewer than two claims, or one source, +and the clusterer is never dispatched (no spend). A missing definition or an +unusable reply leaves the run on tier 1, exactly today's behavior, and surfaces +as a run warning plus a `clustering` block in `dispatch-result.json` +(`candidates`, `proposed`, `clusterMerges`, and every rejected member with the +rule that stopped it) rather than as an author-facing note: duplicate hygiene is +not a review dimension. Each merge in `merges` now carries `via` +(`similarity`/`clusterer`/`both`) and the merged copies' own anchors, and the +"also flagged by" note names a source's line when it differs from the survivor's, +so the merge rate reads off the artifact instead of off a PR that autofix has +already tidied. + +The live A/B now runs dedup, which it never did: a change to the merge rules was +unmeasurable by construction before this. Tier 1 runs in both arms (it is shared +code and production has had it since #245) while tier 2 is carried by each arm's +own review.md, exactly like the provenance gate's anchor-snap emulation, so the +arm delta prices the clusterer alone and a false merge shows up as recall loss. +The report gains a "Cross-source claims merged (of candidates)" row with tier 2's +share, and per-case dedup counts. + +`dispatch.ts` was at its 1000-line cap again, so the clustering step lands in +`dispatch-cluster.ts` (dispatch, contract parse, telemetry) rather than raising +the cap; the tier-2 tests live in `dedup-cluster.test.ts` and +`dispatch-cluster.test.ts` for the same reason. diff --git a/workflows/review/eval/README.md b/workflows/review/eval/README.md index 76a7f403..0c9633be 100644 --- a/workflows/review/eval/README.md +++ b/workflows/review/eval/README.md @@ -145,6 +145,19 @@ claiming a band. a few lines off or past a short file's end) is what the gate's anchor-snap fallback repairs; a finding still landing in this bucket was outside both snap windows. +- **Cross-source merges:** the report's "Cross-source claims merged (of + candidates)" row is the duplicate-comment observable, read from the merge + stage rather than from the posted set (merges happen upstream of every later + drop, and in production autofix satisfies surviving duplicates with one edit, + which hides the symptom on the PR). Tier 1, the calibrated text-similarity + floor, is shared code and runs in BOTH arms; tier 2, the `claim-clusterer` + agent, is carried by each arm's own review.md, so a baseline built from a ref + that predates the agent reports `tier 1 only` and the arm delta prices the + clusterer alone. Read it beside recall: a false merge drops a distinct + finding, so it shows up as candidate-arm recall loss, not as a better + duplicate number. `rejected` counts proposals the merge rules refused + (`unknown-id` there means the clusterer named claims that do not exist, which + is a prompt or staging failure rather than a quiet zero). - **Anchor-snap and the arms:** the deterministic pipeline is shared by both arms, but the provenance gate emulates each arm's OWN review.md gate version, keyed on the literal `anchor-snap` marker in the gate step. A diff --git a/workflows/review/eval/live-ab-report.ts b/workflows/review/eval/live-ab-report.ts index db32d462..051c07c2 100644 --- a/workflows/review/eval/live-ab-report.ts +++ b/workflows/review/eval/live-ab-report.ts @@ -13,7 +13,11 @@ import type { RecordedFinding, } from "./corpus/loader"; import type {LiveCaseRun, LiveMetricsReport} from "./live-match"; -import type {LiveReconciliation, PerAgentReport} from "./live-producer"; +import type { + LiveDedupReport, + LiveReconciliation, + PerAgentReport, +} from "./live-producer"; import type {RereviewCaseScore, RereviewMetricsReport} from "./rereview-match"; export type ArmId = "baseline" | "candidate"; @@ -25,6 +29,8 @@ export type ArmProduceResult = { perAgent: PerAgentReport[]; /** The reconciler's decision, for open-PR (rereview) cases. */ reconciliation?: LiveReconciliation; + /** What the cross-source merge did (absent only for a stub producer). */ + dedup?: LiveDedupReport; }; export type ArmProduce = (corpusCase: CorpusCase) => Promise; @@ -51,6 +57,22 @@ export type ArmRunReport = { * here as candidate-arm snaps falling to zero. */ snapped: number; + /** + * The cross-source merge, per case: `candidates` is the pre-merge claim + * count, `merged` the claims it absorbed, and `clusterMerged` the subset + * tier 2 (the `claim-clusterer`) contributed to. Read the duplicate rate + * from these, never from the posted set — merges happen upstream of + * every drop the pipeline applies afterwards, and in production autofix + * later satisfies surviving duplicates with one edit and hides them. + * `clustererAbsent` marks the arm that never had tier 2 at all. + */ + dedup?: { + candidates: number; + merged: number; + clusterMerged: number; + rejected: number; + clustererAbsent: boolean; + }; /** `: ` per failed agent (diagnosable from the report). */ failedAgents: string[]; /** @@ -247,6 +269,32 @@ const ASYMMETRY_HEADING = const snappedTotal = (arm: ArmRunReport): number => arm.perCase.reduce((sum, c) => sum + c.snapped, 0); +/** + * The arm's cross-source merge rate: claims absorbed over claims produced, + * with tier 2's share and any rejected proposal in parentheses. `tier 1 only` + * marks an arm whose review.md defines no `claim-clusterer` — the expected + * shape of the baseline in the A/B that graduates it, and the reason a zero in + * the clusterer column there is asymmetry, not a negative result. + */ +const mergedTotal = (arm: ArmRunReport): string => { + const dedup = arm.perCase.flatMap((c) => (c.dedup ? [c.dedup] : [])); + if (dedup.length === 0) { + return "n/a"; + } + const sum = (pick: (d: typeof dedup[number]) => number): number => + dedup.reduce((total, d) => total + pick(d), 0); + const absent = dedup.every((d) => d.clustererAbsent); + const notes = [ + absent ? "tier 1 only" : `${sum((d) => d.clusterMerged)} by clusterer`, + ...(sum((d) => d.rejected) > 0 + ? [`${sum((d) => d.rejected)} proposal(s) rejected`] + : []), + ]; + return `${sum((d) => d.merged)} / ${sum((d) => d.candidates)} (${notes.join( + ", ", + )})`; +}; + /** `caseId:specKey` -> drop bucket, for every found-but-dropped miss. */ const dropClassByKey = (arm: ArmRunReport): Map => { const map = new Map(); @@ -441,6 +489,11 @@ export const renderMarkdownReport = (report: AbReport): string => { String(snappedTotal(baseline)), String(snappedTotal(candidate)), ), + row( + "Cross-source claims merged (of candidates)", + mergedTotal(baseline), + mergedTotal(candidate), + ), "", ]; diff --git a/workflows/review/eval/live-ab.ts b/workflows/review/eval/live-ab.ts index 6742a23f..1b4faad0 100644 --- a/workflows/review/eval/live-ab.ts +++ b/workflows/review/eval/live-ab.ts @@ -236,6 +236,25 @@ export const runArm = async ( caught: match.caught.length, missed: match.missed, snapped: result.snappedByProvenance.length, + ...(produced.dedup === undefined + ? {} + : { + dedup: { + candidates: produced.dedup.candidates, + merged: produced.dedup.merges.reduce( + (sum, merge) => sum + merge.merged.length, + 0, + ), + clusterMerged: produced.dedup.merges + .filter((merge) => merge.via !== "similarity") + .reduce( + (sum, merge) => sum + merge.merged.length, + 0, + ), + rejected: produced.dedup.rejected.length, + clustererAbsent: produced.dedup.clustererAbsent, + }, + }), failedAgents: produced.perAgent .filter((a) => a.failed !== undefined) .map((a) => `${a.name}: ${a.failed}`), diff --git a/workflows/review/eval/live-producer.test.ts b/workflows/review/eval/live-producer.test.ts index 2b1d705a..ecc28085 100644 --- a/workflows/review/eval/live-producer.test.ts +++ b/workflows/review/eval/live-producer.test.ts @@ -604,3 +604,135 @@ describe("resolveRuntimeImports", () => { expect(skillPrompt).toContain("## the case skills index"); }); }); + +/** + * The cross-source merge, in the arm shape the A/B prices: tier 1 is shared + * code and runs in both arms, while the `claim-clusterer` agent is carried by + * each arm's own review.md, so a baseline predating it measures tier 1 alone. + * + * The fixture is run 30587343777's real shape, trimmed: two reviewers on one + * wrong doc comment, worded with almost nothing in common, which is what tier 1 + * cannot reach. + */ +describe("produceLive cross-source dedup", () => { + const CAP_NOTE = { + path: "src/a.ts", + line: 1, + label: "note (non-blocking)", + subject: "Comment says the per-key cap is 10 but maxSamples is 25.", + discussion: + 'The comment on `maxSamples` reads "Keeps at most 10 samples per key" while the constant is 25.', + failure_scenario: + "Comment says the per-key cap is 10 but maxSamples is 25", + }; + const CAP_NITPICK = { + path: "src/a.ts", + line: 1, + label: "nitpick (non-blocking)", + subject: "Declaration doc comment doesn't begin with the symbol name.", + discussion: + "Every declaration comment here starts with the declared name; `// Keeps at most 10 samples per key.` above `const maxSamples = 25` is the only one that omits the prefix.", + failure_scenario: + "A `go doc` reader won't associate the comment with `maxSamples`.", + }; + const CLUSTER_OUT = JSON.stringify({ + clusters: [ + { + evidence: + "the doc comment on `maxSamples` says 10 while the constant is 25", + ids: [ + "produce-case:live-correctness-reviewer-1", + "produce-case:live-skill-auditor-1", + ], + }, + ], + }); + + const scripts = () => ({ + "correctness-reviewer": [JSON.stringify({findings: [CAP_NOTE]})], + "skill-auditor": [JSON.stringify({findings: [CAP_NITPICK]})], + "money-payments": [JSON.stringify({findings: []})], + "claim-clusterer": [CLUSTER_OUT], + "claim-validator": [validatorOutput([])], + }); + + const withClusterer = new Map([ + ...AGENTS, + ["claim-clusterer", agent("claim-clusterer")], + ]); + + it("merges on the clusterer's proposal, and validates only the survivor", async () => { + const {runner, requests} = scriptedRunner(scripts()); + const vol = caseVol(); + const result = await produceLive(CASE, withClusterer, { + runner, + stageDir: "/stage", + fs: volFs(vol), + }); + expect(requests.map((r) => r.name)).toContain("claim-clusterer"); + // The clusterer reads its own staged file: the PRE-merge candidates. + expect( + JSON.parse( + vol.readFileSync( + "/stage/context/candidates.json", + "utf8", + ) as string, + ), + ).toHaveLength(2); + // One finding survives, carrying the attribution note, and the + // validator is dispatched over the merged set only. + expect(result.findings).toHaveLength(1); + expect(result.findings[0].finding.model_authored_prose).toContain( + "Also flagged by skill.", + ); + expect( + JSON.parse( + vol.readFileSync( + "/stage/context/claims.json", + "utf8", + ) as string, + ), + ).toHaveLength(1); + expect(result.dedup).toMatchObject({ + candidates: 2, + proposed: 1, + rejected: [], + clustererAbsent: false, + }); + expect(result.dedup.merges[0].via).toBe("clusterer"); + }); + + it("runs tier 1 alone on an arm whose review.md has no clusterer", async () => { + const {runner, requests} = scriptedRunner(scripts()); + const result = await produceLive(CASE, AGENTS, { + runner, + stageDir: "/stage", + fs: volFs(caseVol()), + }); + expect(requests.map((r) => r.name)).not.toContain("claim-clusterer"); + // Both copies post: the asymmetric-arm baseline, recorded rather than + // silent, so a reader can tell it from a clusterer that found nothing. + expect(result.findings).toHaveLength(2); + expect(result.dedup).toEqual({ + candidates: 2, + merges: [], + proposed: 0, + rejected: [], + clustererAbsent: true, + }); + }); + + it("never dispatches the clusterer when one source produced everything", async () => { + const {runner, requests} = scriptedRunner({ + ...scripts(), + "skill-auditor": [JSON.stringify({findings: []})], + }); + const result = await produceLive(CASE, withClusterer, { + runner, + stageDir: "/stage", + fs: volFs(caseVol()), + }); + expect(requests.map((r) => r.name)).not.toContain("claim-clusterer"); + expect(result.dedup.candidates).toBe(1); + }); +}); diff --git a/workflows/review/eval/live-producer.ts b/workflows/review/eval/live-producer.ts index dbe3d07b..30c95f7f 100644 --- a/workflows/review/eval/live-producer.ts +++ b/workflows/review/eval/live-producer.ts @@ -27,6 +27,12 @@ * run with read-only tools and treat the unavailable cap as a denied * budget (the prompt's own fallback: stop investigating, report what you * have). + * + * Cross-source dedup is NOT on that list any more, and its absence used to be + * the load-bearing one: production has merged duplicate claims before validation + * since #245, this module never did, so every duplicate production suppressed + * still posted here and no report column could see a change to the merge rules. + * {@link dedupeLiveFindings} closes that, arm-keyed on the clusterer agent. */ import { @@ -40,6 +46,17 @@ import { import {isBlockingLabel, labelForFinding} from "../lib/render-comment"; import {route, type RouterConfig} from "../lib/router"; import {validateFinding, type Finding, type Lens} from "../lib/finding-schema"; +import { + dedupeClaims, + type ClaimMerge, + type ClusterRejection, +} from "../lib/dedup"; +import { + buildClaims as buildLibClaims, + parseClustererOutput, + type Candidate, + type ProposedCluster, +} from "../lib/dispatch-contracts"; import { VERIFICATION_STATES, type CaseVerification, @@ -144,6 +161,15 @@ export type ProduceLiveResult = { * absent — the scorer then counts every prior thread unaccounted). */ reconciliation?: LiveReconciliation; + /** + * What the cross-source merge did this run: the pre-merge candidate count, + * the merged groups, and the clusterer's proposal/rejection counts. The + * report reads its duplicate numbers from HERE rather than from the posted + * set, for the same reason production reads them from + * `dispatch-result.json`: downstream stages (and, in production, autofix) + * hide duplicate comments after the fact. + */ + dedup: LiveDedupReport; }; export type ProduceLiveOptions = { @@ -428,6 +454,121 @@ const parseAgentFindings = ( return findings; }; +/* -------------------------------------------------------------------------- */ +/* Cross-source dedup (production's pre-validation merge) */ +/* -------------------------------------------------------------------------- */ + +const CLUSTERER = "claim-clusterer"; + +/** What an arm's dedup stage did, for the A/B report. */ +export type LiveDedupReport = { + /** Claims entering the merge (the pre-merge candidate count). */ + candidates: number; + /** Merged groups, as `dispatch-result.json` records them. */ + merges: ClaimMerge[]; + /** Well-formed clusters the clusterer proposed (0 when it did not run). */ + proposed: number; + /** Proposed members the merge rules rejected. */ + rejected: ClusterRejection[]; + /** The arm's review.md defines no clusterer: tier 1 only, by construction. */ + clustererAbsent: boolean; +}; + +/** + * Run production's cross-source merge over an arm's live findings. + * + * Why this exists at all: the A/B never ran dedup, so a change to it was + * unmeasurable by construction — the pipeline the eval measured posted every + * duplicate that production merges, and no report column moved when the merge + * rules changed. Both arms run tier 1 (it is shared code, and production has + * had it since #245); tier 2 is carried by the arm's OWN review.md, exactly + * like the provenance gate's anchor-snap emulation, so a baseline built from a + * ref that predates the `claim-clusterer` agent runs tier 1 alone and the arm + * delta prices the clusterer and nothing else. + * + * The claim projection is the LIB's `buildClaims`, not this module's + * validator-contract one: the merge compares `subject` against + * `failure_scenario` (falling back to `discussion`), and the eval's own + * projection puts the whole prose in `subject` and the evidence trace in + * `discussion`. Feeding that shape to the floors would measure a similarity + * arithmetic production never runs. + */ +const dedupeLiveFindings = async ( + findings: LiveFinding[], + clusterer: ExtractedAgent | undefined, + io: { + dispatch: ( + agent: ExtractedAgent, + parse: (output: string) => ProposedCluster[], + ) => Promise<{ + report: PerAgentReport; + parsed?: ProposedCluster[]; + }>; + write: (name: string, content: string) => void; + }, +): Promise<{ + kept: LiveFinding[]; + dedup: {report?: PerAgentReport; result: LiveDedupReport}; +}> => { + const claims = buildLibClaims(findings as Candidate[]); + const sources = new Set(claims.map((claim) => claim.source)); + const clusterable = claims.length > 1 && sources.size > 1; + let proposals: ProposedCluster[] = []; + let report: PerAgentReport | undefined; + if (clusterable && clusterer !== undefined) { + io.write("candidates.json", JSON.stringify(claims, null, 2)); + const dispatched = await io.dispatch(clusterer, parseClustererOutput); + report = dispatched.report; + proposals = dispatched.parsed ?? []; + } + const merged = dedupeClaims(claims, proposals); + const dropped = new Set( + merged.merges.flatMap((merge) => merge.merged.map((m) => m.id)), + ); + const survivors = new Map( + merged.claims.map((claim) => [claim.id, claim] as const), + ); + const kept = findings + .filter((live) => !dropped.has(live.finding.id)) + .map((live) => { + const survivor = survivors.get(live.finding.id); + if ( + survivor === undefined || + !merged.merges.some( + (merge) => merge.survivor === live.finding.id, + ) + ) { + return live; + } + // The survivor's claim carries the "also flagged by" note (the lib + // projection puts the prose in `discussion`) and may have adopted a + // merged copy's suggestion; both must reach the rendered comment. + return { + ...live, + finding: { + ...live.finding, + model_authored_prose: survivor.discussion, + ...(survivor.suggestion !== undefined + ? {suggested_patch: survivor.suggestion} + : {}), + }, + }; + }); + return { + kept, + dedup: { + ...(report !== undefined ? {report} : {}), + result: { + candidates: claims.length, + merges: merged.merges, + proposed: proposals.length, + rejected: merged.clusterRejections, + clustererAbsent: clusterer === undefined, + }, + }, + }; +}; + /* -------------------------------------------------------------------------- */ /* The claims path */ /* -------------------------------------------------------------------------- */ @@ -709,6 +850,35 @@ export const produceLive = async ( } } + // Cross-source dedup, before validation, exactly where production runs it. + const {kept: dedupedFindings, dedup} = await dedupeLiveFindings( + findings, + agents.get(CLUSTERER), + { + dispatch: (agent, parse) => + dispatchWithRetry( + agent, + resolvePrompt(agent), + { + name: agent.name, + model: agent.model, + cwd: staged.checkoutDir, + maxTurns, + timeoutMs, + }, + runner, + parse, + ), + write: (name, content) => + fs.writeFileSync(`${staged.contextDir}/${name}`, content), + }, + ); + if (dedup.report !== undefined) { + perAgent.push(dedup.report); + } + findings.length = 0; + findings.push(...dedupedFindings); + // The claims path: skip entirely when nothing was found (production // skips Phase 3 on an empty candidate set). let validation: CaseVerification[] = []; @@ -777,6 +947,7 @@ export const produceLive = async ( perAgent, staged, ...(reconciliation !== undefined ? {reconciliation} : {}), + dedup: dedup.result, }; }; diff --git a/workflows/review/lib/dedup-cluster.test.ts b/workflows/review/lib/dedup-cluster.test.ts new file mode 100644 index 00000000..a63b2eb7 --- /dev/null +++ b/workflows/review/lib/dedup-cluster.test.ts @@ -0,0 +1,504 @@ +import {describe, it, expect} from "vitest"; + +import {dedupeClaims} from "./dedup"; +import type {Claim} from "./dispatch-contracts"; + +/** + * Dedup tier 2: the merges the `claim-clusterer` unlocks, and every guard that + * keeps a model's identity claim from costing a distinct finding. Split from + * dedup.test.ts for its max-lines budget; the `claim` factory mirrors that + * file's. + * + * The load-bearing fixture is run 30587343777 (webapp#41204), a FIRST review at + * `depth: full` whose four sources flagged one wrong doc comment and merged + * none of it. Replaying that run's own claims.json is what showed the + * similarity tier is not close: three of the four share the exact anchor and + * still score 0.060-0.068 Jaccard against a 0.14 floor with 0-1 shared bigrams + * against a floor of 4. The first test here pins that, so a future re-derivation + * of the floors cannot quietly claim this cluster. + */ + +const claim = (over: Partial & {id: string; source: string}): Claim => ({ + path: "services/ai-guide/memory/expiration.go", + line: 38, + label: "issue (blocking)", + subject: "s", + discussion: "d", + failure_scenario: "f", + confidence: 0.7, + ...over, +}); + +/** + * Run 30587343777's four copies of the wrong-cap comment, verbatim from that + * run's `out/claims.json` (a FIRST review at `depth: full`, so nothing here is + * a re-review artifact), in dispatch order. The seeded file's line 8 is + * `// Keeps at most 10 samples per key.` and line 9 is `const maxSamples = 25`; + * three of the four anchor on the comment and one on the constant. All four are + * non-blocking, and the run's autofix later satisfied every one of them with a + * single rewritten comment. + * + * `conventions-1` is the member worth arguing about: its stated rule is that a + * declaration comment must begin with the symbol name, not that the cap is + * wrong. It belongs here anyway, because the unit of identity is the defect the + * author must fix, and one rewritten comment discharges all four asks. + */ +const wrongCapClaims = (): Claim[] => [ + claim({ + id: "correctness-reviewer-3", + source: "correctness-reviewer", + path: "dev/af19_trial/window.go", + line: 8, + label: "note (non-blocking)", + subject: "Comment says the per-key cap is 10 but maxSamples is 25.", + discussion: + 'Comment says the per-key cap is 10 but maxSamples is 25. Introduced by this change. The comment on `maxSamples` reads "Keeps at most 10 samples per key" while the constant is 25 — a factually wrong comment from the moment it lands, which the repo conventions explicitly call out ("keep comments true"). While here: the adjacent `// staleAfter is 15 minutes.` comment, and the inline `// Append the sample to the slice for this key.` / `// Add the sample to the result.` comments, restate the code rather than explain why — the root contract asks for why-comments; consider dropping them.', + failure_scenario: + "Comment says the per-key cap is 10 but maxSamples is 25", + suggestion: + "// maxSamples caps how many samples one key retains, so a hot key cannot grow\n// without bound.\nconst maxSamples = 25", + }), + claim({ + id: "skill-auditor-ool-2", + source: "skill-auditor (out-of-lane)", + path: "dev/af19_trial/window.go", + line: 9, + label: "question (non-blocking)", + subject: + 'The comment on line 8 says "Keeps at most 10 samples per key." but `const maxSamples = 25`, so the doc and the enforced cap disagree.', + discussion: + 'The comment on line 8 says "Keeps at most 10 samples per key." but `const maxSamples = 25`, so the doc and the enforced cap disagree.', + failure_scenario: + "A caller or maintainer trusting the comment believes each key retains at most 10 samples and sizes downstream buffers or reasoning around 10, while Record actually retains 25, leading to under-provisioned assumptions about memory/behavior.", + }), + claim({ + id: "conventions-1", + source: "conventions", + path: "dev/af19_trial/window.go", + line: 8, + label: "nitpick (non-blocking)", + subject: "Declaration doc comment doesn't begin with the symbol name.", + discussion: + "Declaration doc comment doesn't begin with the symbol name. Every other declaration comment in this file starts with the declared name — e.g. two lines below, `// staleAfter is 15 minutes.` above `const staleAfter`, and likewise `// Sample is...`, `// Window holds...`, `// NewWindow returns...`. The comment `// Keeps at most 10 samples per key.` above `const maxSamples = 25` is the sole one that omits the `maxSamples` prefix.", + failure_scenario: + "A `go doc`/grep-by-symbol reader won't associate this comment with `maxSamples`, and the odd-one-out style reads as an oversight next to its eight siblings.", + suggestion: + "// maxSamples caps how many samples are kept per key.\nconst maxSamples = 25", + }), + claim({ + id: "documentation-1", + source: "documentation", + path: "dev/af19_trial/window.go", + line: 8, + label: "suggestion (non-blocking, documentation)", + subject: "Comment states the wrong cap (10 vs 25).", + discussion: + "Comment states the wrong cap (10 vs 25). The comment `// Keeps at most 10 samples per key.` sits directly on `const maxSamples = 25` — the number in the prose contradicts the value.", + failure_scenario: + "A reader trusts the comment's cap of 10 when reasoning about memory/behavior, but the real cap is 25.", + suggestion: "// Keeps at most 25 samples per key.", + }), +]; + +/** + * Two distinct defects from the same run sitting inside the cluster's own line + * range: hard-coded tunables (at :9, the cluster's second anchor) and a comment + * restating `staleAfter` (at :11). Both are real claims from that + * `claims.json`, and both must survive the cap merge. + */ +const capNeighbourClaims = (): Claim[] => [ + claim({ + id: "first-principles-1", + source: "first-principles", + path: "dev/af19_trial/window.go", + line: 9, + label: "suggestion (non-blocking)", + subject: + "Hard-coded maxSamples/staleAfter contradict the stated goal of unifying several differing call sites.", + discussion: + "Hard-coded maxSamples/staleAfter contradict the stated goal of unifying several differing call sites. The rationale is that several call sites each keep 'their own ad-hoc ring' with the same three operations 'slightly differently' — but the dimensions along which they most plausibly differ (cap size, staleness bound) are frozen as unexported package constants (25 samples, 15 minutes). A shared helper whose only tunables are untunable can't actually replace divergent callers.", + failure_scenario: + "The first real call site that needs a different cap or staleness bound cannot adopt the helper without editing package-level constants, so the ad-hoc rings this PR exists to eliminate stay in place.", + }), + claim({ + id: "documentation-2", + source: "documentation", + path: "dev/af19_trial/window.go", + line: 11, + label: "suggestion (non-blocking, documentation)", + subject: "Comment restates the constant.", + discussion: + "Comment restates the constant. `// staleAfter is 15 minutes.` restates `const staleAfter = 15 * time.Minute` verbatim; the code already says exactly this. Delete the comment.", + failure_scenario: + "The reader maintains a comment that restates the literal it sits on; if the duration changes and the comment doesn't, it becomes a lie.", + }), +]; + +describe("dedupeClaims with model-proposed clusters", () => { + it("leaves run 30587343777's four wrong-cap copies unmerged on the similarity tier alone", () => { + // The production symptom, pinned: a FIRST review at full depth, four + // sources on one wrong doc comment, and `merges` recorded none of them. + // THREE of the four share the exact anchor and still score 0.060-0.068 + // Jaccard on the 0.14 same-line floor with 0-1 shared bigrams on a + // floor of 4, so no re-derivation of tier 1 reaches this cluster: the + // floor that admits 0.06 admits everything. + const {claims, merges} = dedupeClaims(wrongCapClaims()); + expect(claims).toHaveLength(4); + expect(merges).toEqual([]); + }); + + it("merges run 30587343777's four wrong-cap copies on the clusterer's grounded proposal", () => { + const {claims, merges, clusterRejections} = dedupeClaims( + wrongCapClaims(), + [ + { + evidence: + "the doc comment on `maxSamples` says 10 while the constant is 25", + ids: [ + "correctness-reviewer-3", + "skill-auditor-ool-2", + "conventions-1", + "documentation-1", + ], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual(["correctness-reviewer-3"]); + // Anchors differ inside the cluster (:8 and :9), which tier 2 does not + // care about and the note does report. + expect(claims[0].discussion).toContain( + "Also flagged by skill-auditor (out-of-lane) (at line 9), " + + "conventions, documentation.", + ); + expect(merges).toEqual([ + { + survivor: "correctness-reviewer-3", + merged: [ + { + id: "skill-auditor-ool-2", + source: "skill-auditor (out-of-lane)", + label: "question (non-blocking)", + line: 9, + }, + { + id: "conventions-1", + source: "conventions", + label: "nitpick (non-blocking)", + }, + { + id: "documentation-1", + source: "documentation", + label: "suggestion (non-blocking, documentation)", + }, + ], + path: "dev/af19_trial/window.go", + line: 8, + via: "clusterer", + evidence: + "the doc comment on `maxSamples` says 10 while the constant is 25", + }, + ]); + expect(clusterRejections).toEqual([]); + }); + + it("keeps the run's neighbours on the same lines out of the cluster", () => { + // The precision half of the same run: `first-principles-1` sits at :9 + // (the cluster's own second anchor) and `documentation-2` three lines + // down, and both are distinct defects — hard-coded tunables and a + // comment restating `staleAfter`. A proposal naming only the cap copies + // must leave them alone, and their own text must not be pulled in by + // the group they neighbour. + const claims = [...wrongCapClaims(), ...capNeighbourClaims()]; + const {claims: kept} = dedupeClaims(claims, [ + { + evidence: "the `maxSamples` comment claims a cap of 10, not 25", + ids: [ + "correctness-reviewer-3", + "skill-auditor-ool-2", + "conventions-1", + "documentation-1", + ], + }, + ]); + expect(kept.map((c) => c.id)).toEqual([ + "correctness-reviewer-3", + "first-principles-1", + "documentation-2", + ]); + }); + + it("drops a proposed member whose own text never names the shared evidence", () => { + // The grounding tripwire: `documentation-2` is about `staleAfter`, so a + // cluster grounded in the maxSamples cap cannot absorb it however + // confidently the model listed it. + const {claims, merges, clusterRejections} = dedupeClaims( + [...wrongCapClaims(), ...capNeighbourClaims()], + [ + { + evidence: + "the `maxSamples` comment claims a cap of 10, not 25", + ids: [ + "correctness-reviewer-3", + "documentation-1", + "documentation-2", + ], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual([ + "correctness-reviewer-3", + "skill-auditor-ool-2", + "conventions-1", + "first-principles-1", + "documentation-2", + ]); + expect(merges[0].merged.map((m) => m.id)).toEqual(["documentation-1"]); + expect(clusterRejections).toEqual([ + {id: "documentation-2", reason: "ungrounded"}, + ]); + }); + + it("refuses a cluster whose evidence names no code element at all", () => { + // An identity claim this module cannot check is inert, not + // authoritative: "they are all about comments" grounds nothing, so the + // group falls back to tier 1 and stays four comments. + const {claims, clusterRejections} = dedupeClaims(wrongCapClaims(), [ + { + evidence: "these are all about the same comment", + ids: [ + "correctness-reviewer-3", + "skill-auditor-ool-2", + "conventions-1", + "documentation-1", + ], + }, + ]); + expect(claims).toHaveLength(4); + expect(clusterRejections).toEqual([ + {id: "skill-auditor-ool-2", reason: "ungrounded"}, + {id: "conventions-1", reason: "ungrounded"}, + {id: "documentation-1", reason: "ungrounded"}, + ]); + }); + + it("never absorbs a blocking claim on the clusterer's word", () => { + // The risk grading: a false tier-2 merge may cost an advisory comment, + // never a blocking finding. Two blocking copies of one defect are + // exactly the pair tier 1 was calibrated on, so tier 2 declines them + // however grounded the proposal is, and both still post. + const [note, question] = wrongCapClaims(); + const {claims, merges, clusterRejections} = dedupeClaims( + [ + {...note, label: "issue (blocking)"}, + { + ...question, + id: "holistic-9", + source: "holistic", + label: "issue (blocking)", + }, + ], + [ + { + evidence: "the `maxSamples` cap comment says 10, not 25", + ids: ["correctness-reviewer-3", "holistic-9"], + }, + ], + ); + expect(claims).toHaveLength(2); + expect(merges).toEqual([]); + expect(clusterRejections).toEqual([ + {id: "holistic-9", reason: "blocking-member"}, + ]); + }); + + it("absorbs an advisory copy into a blocking survivor, keeping the severity", () => { + // The other side of the same rule: severity is preserved by the + // survivor choice, so the blocking copy is what posts. + const [note, question] = wrongCapClaims(); + const {claims, merges} = dedupeClaims( + [ + note, + { + ...question, + id: "holistic-9", + source: "holistic", + label: "issue (blocking)", + }, + ], + [ + { + evidence: "the `maxSamples` cap comment says 10, not 25", + ids: ["correctness-reviewer-3", "holistic-9"], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual(["holistic-9"]); + expect(claims[0].label).toBe("issue (blocking)"); + expect(merges[0].merged.map((m) => m.id)).toEqual([ + "correctness-reviewer-3", + ]); + }); + + it("enforces tier 1's path and source rules on a proposed cluster", () => { + const [note, question, conventions] = wrongCapClaims(); + const {claims, clusterRejections} = dedupeClaims( + [ + note, + {...question, path: "dev/af19_trial/other.go"}, + {...conventions, source: note.source, id: "correctness-4"}, + ], + [ + { + evidence: "the `maxSamples` cap comment says 10, not 25", + ids: [ + "correctness-reviewer-3", + "skill-auditor-ool-2", + "correctness-4", + ], + }, + ], + ); + expect(claims).toHaveLength(3); + expect(clusterRejections).toEqual([ + {id: "skill-auditor-ool-2", reason: "other-path"}, + {id: "correctness-4", reason: "same-source"}, + ]); + }); + + it("records ids the clusterer invented, and holds each claim to one cluster", () => { + // The webapp#41197 lesson applied to tier 2: an empty merge list must + // never be the only evidence. A clusterer naming claims that do not + // exist is a staging or prompt failure, and the run records it. + const {claims, merges, clusterRejections} = dedupeClaims( + wrongCapClaims(), + [ + { + evidence: "the `maxSamples` cap comment says 10, not 25", + ids: ["correctness-reviewer-3", "documentation-1"], + }, + { + evidence: "the `maxSamples` cap comment again", + ids: [ + "documentation-1", + "conventions-1", + "ghost-reviewer-7", + ], + }, + ], + ); + expect(merges).toHaveLength(1); + expect(merges[0].merged.map((m) => m.id)).toEqual(["documentation-1"]); + expect(claims.map((c) => c.id)).toEqual([ + "correctness-reviewer-3", + "skill-auditor-ool-2", + "conventions-1", + ]); + expect(clusterRejections).toEqual([ + {id: "documentation-1", reason: "already-clustered"}, + {id: "ghost-reviewer-7", reason: "unknown-id"}, + {id: "conventions-1", reason: "cluster-collapsed"}, + ]); + }); + + it("merges a same-defect pair the similarity tier cannot reach across anchors", () => { + // The shape that is unmergeable by construction on tier 1: run + // 29943085279's missing-deletion-test defect at two anchors, worded + // with almost no shared prose. Tier 1 keeps them apart (it needs six + // shared bigrams across lines); the grounded cluster merges them and + // the survivor keeps the blocking label and its own anchor. + const todo = claim({ + id: "test-adequacy-1", + source: "test-adequacy", + path: "services/ai-guide/memory/expiration_test.go", + line: 15, + label: "todo (blocking)", + subject: "Nothing asserts DeleteMulti removes a stale memory.", + failure_scenario: + "A regression leaves ExpireStale identifying keys and never calling DeleteMulti, and CI stays green.", + }); + const note = claim({ + id: "first-principles-4", + source: "first-principles", + path: "services/ai-guide/memory/expiration_test.go", + line: 58, + label: "note (non-blocking)", + subject: "The suite never reaches the delete.", + failure_scenario: + "Both cases stop at DeleteMulti's caller, so the behavior the change exists for is unexercised.", + }); + expect(dedupeClaims([todo, note]).merges).toEqual([]); + const {claims, merges} = dedupeClaims( + [todo, note], + [ + { + evidence: + "no test asserts DeleteMulti deletes a stale memory in ExpireStale", + ids: ["test-adequacy-1", "first-principles-4"], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual(["test-adequacy-1"]); + expect(claims[0].label).toBe("todo (blocking)"); + expect(claims[0].discussion).toContain( + "Also flagged by first-principles (at line 58).", + ); + expect(merges[0].via).toBe("clusterer"); + expect(merges[0].line).toBe(15); + }); + + it("marks a group both tiers contributed to", () => { + // Tier 1 reaches the run-29943085279 todo/question pair; the third copy + // is worded too thinly for any floor and arrives on the proposal. One + // group, one comment, and the record says both tiers found it. + const todo = claim({ + id: "correctness-reviewer-3", + source: "correctness-reviewer", + path: "services/ai-guide/memory/expiration_test.go", + line: 15, + label: "todo (blocking)", + subject: + "No test creates a memory older than the retention window and asserts it gets deleted; the core added behavior (expiration actually expiring something) is untested, and both existing tests pass even when ExpireStale is a total no-op.", + failure_scenario: + "The TTL arithmetic bug (or any future regression that quietly turns expiration into a no-op, e.g. a filter-field typo) ships with green tests, and memories never expire in production with nothing to flag it.", + }); + const question = claim({ + id: "skill-auditor-ool-2", + source: "skill-auditor (out-of-lane)", + path: "services/ai-guide/memory/expiration_test.go", + line: 58, + label: "question (non-blocking)", + subject: + "Both tests only exercise current memories (TestExpirationKeepsCurrentMemories) or an empty user (TestExpirationEmptyUser); neither creates a memory older than the retention window and asserts it is deleted.", + failure_scenario: + "Because no test stores a stale memory and checks it is removed, an incorrect cutoff computation (e.g. the AddDate months-vs-days error) passes CI green, so a retention feature that deletes nothing ships undetected.", + }); + const thin = claim({ + id: "first-principles-4", + source: "first-principles", + path: "services/ai-guide/memory/expiration_test.go", + line: 58, + label: "note (non-blocking)", + subject: "The suite never reaches the delete.", + failure_scenario: + "Both cases stop at DeleteMulti's caller, so the behavior the change exists for is unexercised.", + }); + expect(dedupeClaims([todo, thin]).merges).toEqual([]); + const {claims, merges} = dedupeClaims( + [todo, question, thin], + [ + { + evidence: + "no test asserts ExpireStale reaches DeleteMulti for a stale memory", + ids: ["correctness-reviewer-3", "first-principles-4"], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual(["correctness-reviewer-3"]); + expect(merges).toHaveLength(1); + expect(merges[0].via).toBe("both"); + expect(merges[0].merged.map((m) => m.id)).toEqual([ + "skill-auditor-ool-2", + "first-principles-4", + ]); + }); +}); diff --git a/workflows/review/lib/dedup.test.ts b/workflows/review/lib/dedup.test.ts index cc0e5225..e7bb55f2 100644 --- a/workflows/review/lib/dedup.test.ts +++ b/workflows/review/lib/dedup.test.ts @@ -167,6 +167,7 @@ describe("dedupeClaims", () => { ], path: "services/ai-guide/memory/expiration.go", line: 38, + via: "similarity", }, ]); }); @@ -271,10 +272,12 @@ describe("dedupeClaims", () => { id: "test-adequacy-1", source: "test-adequacy", label: "todo (blocking)", + line: 40, }, ], path: "services/ai-guide/memory/expiration.go", line: 15, + via: "similarity", }, ]); }); @@ -328,8 +331,11 @@ describe("dedupeClaims", () => { expect(claims).toHaveLength(1); expect(claims[0].id).toBe("correctness-reviewer-3"); expect(claims[0].label).toBe("todo (blocking)"); + // The note names the other copy's anchor, since it is not the + // survivor's: an author reading a merge across 43 lines needs to know + // the second reviewer was looking somewhere else. expect(claims[0].discussion).toContain( - "Also flagged by skill-auditor (out-of-lane).", + "Also flagged by skill-auditor (out-of-lane) (at line 58).", ); expect(merges).toEqual([ { @@ -339,10 +345,12 @@ describe("dedupeClaims", () => { id: "skill-auditor-ool-2", source: "skill-auditor (out-of-lane)", label: "question (non-blocking)", + line: 58, }, ], path: "services/ai-guide/memory/expiration_test.go", line: 15, + via: "similarity", }, ]); }); @@ -439,6 +447,7 @@ describe("dedupeClaims", () => { ], path: "services/ai-guide/memory/expiration.go", line: 38, + via: "similarity", }, ]); }); diff --git a/workflows/review/lib/dedup.ts b/workflows/review/lib/dedup.ts index 0fc8c979..19a817a5 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -6,29 +6,108 @@ * and every copy was separately validated — validation is the single largest * sub-agent cost line, so duplicates are merged before it, not after. * - * The merge is deliberately conservative: only claims from DIFFERENT sources, - * anchored on the same path, whose text clearly describes the same defect - * (token-set similarity plus a shared-phrase floor, calibrated on the real - * claim sets of runs 29897276810, 29943085279 and 30301235749). Still no - * line window: run 29943085279's missing-deletion-test defect posted twice - * with anchors 43 lines apart in expiration_test.go (:15 and :58), so - * proximity can never be required. An IDENTICAL anchor is used the other - * way round, as evidence: two sources landing on the same `(path, line)` - * clear a lower text floor than two sources landing on different lines. + * Merges arrive from TWO tiers, and the split is the module's central idea. + * + * 1. **Text similarity** (below): claims from DIFFERENT sources, anchored on + * the same path, whose text clearly describes the same defect (token-set + * similarity plus a shared-phrase floor, calibrated on the real claim sets + * of runs 29897276810, 29943085279 and 30301235749). Still no line window: + * run 29943085279's missing-deletion-test defect posted twice with anchors + * 43 lines apart in expiration_test.go (:15 and :58), so proximity can + * never be required. An IDENTICAL anchor is used the other way round, as + * evidence: two sources landing on the same `(path, line)` clear a lower + * text floor than two sources landing on different lines. + * 2. **Model-proposed defect clusters** ({@link verifiableClusters}): the + * `claim-clusterer` sub-agent reads the candidate set and names the groups + * that describe ONE defect, each grounded in the code element its members + * share. This module verifies that assertion and owns every merge rule; the + * model contributes identity only. + * + * Why a second tier at all — the limit of tier 1, measured. Run 30587343777 + * (webapp#41204, a FIRST review at `depth: full`, so no re-review artifact) + * had four sources flag one wrong doc comment (`// Keeps at most 10 samples + * per key.` above `const maxSamples = 25`) at window.go :8, :9, :8, :8, and + * merged none of them. Replaying that run's own claims.json: THREE of the four + * share the exact anchor and still score 0.060-0.068 Jaccard on a 0.14 floor + * with 0-1 shared bigrams on a floor of 4 — an order of magnitude below the + * tier, not a thin margin. Each reviewer wrote the same defect in different + * words ("wrong cap (10 vs 25)", "per-key cap is 10 but maxSamples is 25", a + * verbatim quote of the comment), and the terser the claim the less text + * arithmetic has to work with. The floor that admits 0.06 admits everything. + * + * And the discriminator cannot be recovered by reweighting the text, because + * the pairs this module deliberately KEEPS apart share more salient tokens + * than the real duplicates do: run 29943085279's AddDate arithmetic issue and + * its "central behavior never exercised" thought sit on one line and share + * AddDate, MemoryTTLDays, 180 and 15. Duplicates are "same ask, different + * words"; those pairs are "same facts, different ask" (a bug versus its + * missing test). Telling those apart is a semantic judgment, so tier 2 asks a + * model for it rather than pretending a fourth threshold would find it. + * + * The unit of identity, restated: the DEFECT, not the anchor. Tier 2 needs no + * line agreement at all, which is what finally makes the + * same-defect-different-anchor shape mergeable (one missing-test defect drew + * comments at three anchors in run 29943085279; tier 1 can only reach the + * pairs that also clear its looser text floor). The line survives as evidence + * inside tier 1 and as the survivor's posting anchor, and nothing more. + * * The survivor is the highest-severity copy, its discussion gains an "also - * flagged by" note, and every merge is recorded for dispatch-result.json. + * flagged by" note (naming each other source's anchor when it differs), and + * every merge is recorded for dispatch-result.json with the tier that found + * it, so the merge rate is readable from the artifact rather than from what + * survives on the PR. * - * Determinism boundary: pure text arithmetic; no model call, no filesystem. + * Determinism boundary: every merge RULE is code — pure text arithmetic, no + * filesystem. The one model input is tier 2's identity assertion, and it + * enters through {@link verifiableClusters}, which trusts none of it: the ids + * must exist, the paths and sources must satisfy the same constraints tier 1 + * enforces, the model's own grounding evidence must appear in every member's + * text, and only a NON-BLOCKING copy may be absorbed on a model's word + * ({@link clusterMemberRejection} carries the reasoning). What a tier-2 error + * can cost is bounded by code even where its judgment cannot be checked. */ -import {isRecord, type Claim} from "./dispatch-contracts"; +import {isRecord, type Claim, type ProposedCluster} from "./dispatch-contracts"; import {isBlockingLabel} from "./render-comment"; +/** Which tier identified a merged group (dispatch-result.json audit). */ +export type MergeVia = "similarity" | "clusterer" | "both"; + export type ClaimMerge = { survivor: string; - merged: {id: string; source: string; label: string}[]; + merged: { + id: string; + source: string; + label: string; + /** The merged copy's own anchor, when it differs from the survivor's. */ + line?: number; + }[]; path: string; line: number; + via: MergeVia; + /** The clusterer's grounding evidence, when tier 2 found this group. */ + evidence?: string; +}; + +/** + * One member a proposed cluster named that did NOT merge, with the rule that + * rejected it. Recorded per run because an empty rejection list and an empty + * proposal list mean opposite things, and the module has already been burned + * by that ambiguity once (see {@link stagedThreadShapeFailure}): a clusterer + * naming ids that do not exist is a prompt or staging failure, and it must not + * read as "no duplicates found". + */ +export type ClusterRejection = { + id: string; + reason: + | "unknown-id" + | "no-anchor" + | "other-path" + | "same-source" + | "blocking-member" + | "ungrounded" + | "already-clustered" + | "cluster-collapsed"; }; /** @@ -143,6 +222,176 @@ export const describesSameDefect = (a: Claim, b: Claim): boolean => { ); }; +/* -------------------------------------------------------------------------- */ +/* Tier 2: model-proposed defect clusters, code-verified */ +/* -------------------------------------------------------------------------- */ + +/** + * Whether a token names something in the code rather than in English: an + * interior case change (`maxSamples`, `AddDate`, `TrimTo`), an all-caps + * initialism (`TTL`), an underscore (`created_at`, `expiration_test`), or a + * multi-digit literal (`10`, `25`, `180`). + * + * This is the vocabulary tier 2's grounding check runs over, and it is + * deliberately narrow. A single-digit number is noise (`0` appears in half of + * all claims), and a bare lowercase word is English until proven otherwise — + * `cutoff` and `samples` would ground almost any two claims about the same + * file, which is precisely the confusion between "same code area" and "same + * defect" that this tier exists to avoid. A defect nameable only in such words + * is not model-mergeable; it falls back to tier 1, and a missed merge costs a + * duplicate comment while a wrong one drops a reviewer's distinct finding. + */ +const isSalientToken = (raw: string): boolean => + /[a-z][A-Z]/.test(raw) || + /^[A-Z]{2,}$/.test(raw) || + raw.includes("_") || + /^\d{2,}$/.test(raw); + +/** The code-naming tokens in a text, lowercased for comparison. */ +const salientTokens = (text: string): Set => { + const tokens = new Set(); + for (const raw of text.match(/[A-Za-z_$][A-Za-z0-9_$]*|\d+/g) ?? []) { + if (isSalientToken(raw)) { + tokens.add(raw.toLowerCase()); + } + } + return tokens; +}; + +/** Everything a claim says, for the grounding check (evidence lives anywhere). */ +const claimText = (claim: Claim): string => + `${claim.subject} ${claim.discussion} ${claim.failure_scenario}`; + +const sharesSalientToken = ( + evidenceTokens: ReadonlySet, + claim: Claim, +): boolean => { + const tokens = salientTokens(claimText(claim)); + for (const token of evidenceTokens) { + if (tokens.has(token)) { + return true; + } + } + return false; +}; + +/** + * The per-member rules a model-proposed merge must satisfy, checked against + * the group's ACTUAL survivor (which union with tier 1 can change after the + * proposal was made, so re-checking here rather than at parse time is what + * keeps the guarantee honest): + * + * - **same path**, as in tier 1. Cross-file merging stays out of both tiers; + * its own calibration is a separate question and a missed merge is cheap. + * - **different source**, as in tier 1: a reviewer does not duplicate itself, + * and collapsing two of one reviewer's findings would silently drop one. + * - **grounded**: the cluster's evidence must name at least one code element + * ({@link isSalientToken}) and the member must mention one of them. This is + * the hallucination tripwire — a group whose members share no named code + * with the identity the model asserted is not an identity claim this module + * can check, so it does not merge. + * - **non-blocking**: tier 2 may absorb an advisory copy into any survivor, + * but a BLOCKING claim only ever merges on tier 1's text floor. This is the + * risk grading, and the one place the tiers deliberately differ in power + * rather than in method. The model owns identity here, so a wrong grouping IS + * possible in a way no code check catches: "same facts, different ask" (run + * 30301235749's AddDate handoff and the missing-test todo it rode both name + * `AddDate`, so grounding cannot separate them; only the clusterer's + * judgment does). Capping what such an error can cost is therefore part of + * the design: since the survivor is always the highest-severity copy, a false + * tier-2 merge can lose an advisory comment and can never lose a blocking + * finding or soften the verdict. The price is real and accepted: one defect + * flagged blocking by two sources in different words still posts twice + * unless tier 1 reaches it. + * + * Deliberately absent: any line requirement, and any text-similarity floor. + * Both are what tier 2 exists to get past. + */ +const clusterMemberRejection = ( + survivor: Claim, + member: Claim, + evidenceTokens: ReadonlySet, +): ClusterRejection["reason"] | undefined => { + if (member.path !== survivor.path) { + return "other-path"; + } + if (member.source === survivor.source) { + return "same-source"; + } + if (isBlockingLabel(member.label)) { + return "blocking-member"; + } + return sharesSalientToken(evidenceTokens, member) + ? undefined + : "ungrounded"; +}; + +/** + * Resolve the clusterer's proposals against the claim set: map ids to claims, + * hold each claim to at most one cluster (first proposal wins, in the model's + * own output order, so the result is deterministic), and drop what cannot + * anchor a comment. Every drop is recorded. + * + * A cluster is kept here only as a MEMBERSHIP hint; the merge rules that + * decide what actually collapses run later against the group's survivor + * ({@link clusterMemberRejection}). + */ +const verifiableClusters = ( + claims: Claim[], + proposals: readonly ProposedCluster[], +): { + /** Claim index -> cluster ordinal. */ + clusterOf: Map; + /** Cluster ordinal -> the model's grounding evidence. */ + evidence: string[]; + rejections: ClusterRejection[]; +} => { + const indexById = new Map(); + claims.forEach((claim, index) => { + if (!indexById.has(claim.id)) { + indexById.set(claim.id, index); + } + }); + const clusterOf = new Map(); + const evidence: string[] = []; + const rejections: ClusterRejection[] = []; + for (const proposal of proposals) { + const members: number[] = []; + for (const id of proposal.ids) { + const index = indexById.get(id); + if (index === undefined) { + rejections.push({id, reason: "unknown-id"}); + continue; + } + if (clusterOf.has(index)) { + rejections.push({id, reason: "already-clustered"}); + continue; + } + const claim = claims[index]; + if (claim.path === undefined || claim.line === undefined) { + rejections.push({id, reason: "no-anchor"}); + continue; + } + members.push(index); + } + if (members.length < 2) { + for (const index of members) { + rejections.push({ + id: claims[index].id, + reason: "cluster-collapsed", + }); + } + continue; + } + const ordinal = evidence.length; + evidence.push(proposal.evidence); + for (const index of members) { + clusterOf.set(index, ordinal); + } + } + return {clusterOf, evidence, rejections}; +}; + /** * Same path, any line distance: run 29943085279 posted the * missing-deletion-test defect at expiration_test.go:15 and :58 (43 lines @@ -468,16 +717,34 @@ const survivorFirst = ( }; /** - * Merge high-confidence cross-source duplicates, preserving claim order. - * Non-anchored claims and everything below the similarity floor pass through - * untouched; when in doubt, don't merge (a false merge silently drops a - * reviewer's distinct finding, a missed merge only costs a duplicate - * comment). + * Merge cross-source duplicates, preserving claim order: the similarity tier + * plus, when the clusterer ran, the defect clusters it proposed (verified + * here, never trusted). Non-anchored claims and everything neither tier + * identifies pass through untouched; when in doubt, don't merge (a false merge + * silently drops a reviewer's distinct finding, a missed merge only costs a + * duplicate comment). + * + * `clusterRejections` is the tier-2 audit trail (see {@link ClusterRejection}); + * it is empty both when the clusterer proposed nothing and when everything it + * proposed merged, so read it beside the proposal count, never alone. */ export const dedupeClaims = ( claims: Claim[], -): {claims: Claim[]; merges: ClaimMerge[]} => { - // Union-find over pairwise-mergeable claims. + proposals: readonly ProposedCluster[] = [], +): { + claims: Claim[]; + merges: ClaimMerge[]; + clusterRejections: ClusterRejection[]; +} => { + const {clusterOf, evidence, rejections} = verifiableClusters( + claims, + proposals, + ); + const clusterRejections = [...rejections]; + + // Union-find over pairwise-mergeable claims, then over each proposed + // cluster's members. Union is membership only: what actually collapses is + // decided per member against the group's survivor, below. const parent = claims.map((_, index) => index); const find = (index: number): number => { while (parent[index] !== index) { @@ -493,6 +760,15 @@ export const dedupeClaims = ( } } } + const clusterAnchor = new Map(); + for (const [index, ordinal] of clusterOf) { + const anchor = clusterAnchor.get(ordinal); + if (anchor === undefined) { + clusterAnchor.set(ordinal, index); + } else { + parent[find(index)] = find(anchor); + } + } const groups = new Map(); claims.forEach((_, index) => { const root = find(index); @@ -510,6 +786,24 @@ export const dedupeClaims = ( survivorFirst(best, index, claims), ); const survivor = claims[survivorIndex]; + // The group's cluster evidence (lowest ordinal present, so the choice + // is deterministic when tier 1 has bridged two clusters). Tokenized + // once: an evidence string naming no code element grounds nothing, and + // that check is what makes an unverifiable identity claim inert rather + // than authoritative. + const ordinals = group + .map((index) => clusterOf.get(index)) + .filter((ordinal): ordinal is number => ordinal !== undefined); + const groupEvidence = + ordinals.length === 0 ? undefined : evidence[Math.min(...ordinals)]; + const evidenceTokens = + groupEvidence === undefined + ? undefined + : salientTokens(groupEvidence); + const clusterUsable = + evidenceTokens !== undefined && + evidenceTokens.size > 0 && + sharesSalientToken(evidenceTokens, survivor); // Star guard: only a member that clears the floor against the // survivor DIRECTLY merges. Union-find alone chains A~B~C through a // bridging claim that bundles two defects (a test-adequacy finding @@ -520,11 +814,44 @@ export const dedupeClaims = ( // their own claims. Both recorded trial merges are unaffected: run // 29897276810's four-way group is pairwise-complete and run // 29943085279's is a direct pair. - const others = group.filter( - (index) => - index !== survivorIndex && - describesSameDefect(survivor, claims[index]), - ); + // + // A cluster member takes the tier-2 path instead: it did not clear the + // floor (that is why the clusterer exists), so it merges on the + // verified rules alone. The evidence check runs against THIS survivor, + // so a group tier 1 has since reshaped is re-verified, not grandfathered. + const viaCluster = new Set(); + const others = group.filter((index) => { + if (index === survivorIndex) { + return false; + } + if (describesSameDefect(survivor, claims[index])) { + return true; + } + if (clusterOf.get(index) === undefined) { + return false; + } + if (!clusterUsable) { + clusterRejections.push({ + id: claims[index].id, + reason: "ungrounded", + }); + return false; + } + const rejection = clusterMemberRejection( + survivor, + claims[index], + evidenceTokens as ReadonlySet, + ); + if (rejection !== undefined) { + clusterRejections.push({ + id: claims[index].id, + reason: rejection, + }); + return false; + } + viaCluster.add(index); + return true; + }); if (others.length === 0) { continue; } @@ -532,13 +859,37 @@ export const dedupeClaims = ( drop.add(index); } const otherClaims = others.map((index) => claims[index]); - const sources = [ - ...new Set(otherClaims.map((claim) => claim.source)), - ].filter((source) => source !== survivor.source); + // One entry per other source, first copy wins, naming that copy's + // anchor when it is not the survivor's. With tier 2 merging across + // anchors, "also flagged by test-adequacy" alone would hide that the + // second reviewer was looking at a different line, which is exactly + // the context an author needs to judge a same-defect-different-anchor + // merge (and to spot a wrong one). + const sources: {source: string; line?: number}[] = []; + for (const claim of otherClaims) { + if ( + claim.source === survivor.source || + sources.some((seen) => seen.source === claim.source) + ) { + continue; + } + sources.push({ + source: claim.source, + ...(claim.line !== undefined && claim.line !== survivor.line + ? {line: claim.line} + : {}), + }); + } const alsoFlagged = sources.length === 0 ? "" - : `\n\nAlso flagged by ${sources.join(", ")}.`; + : `\n\nAlso flagged by ${sources + .map((entry) => + entry.line === undefined + ? entry.source + : `${entry.source} (at line ${entry.line})`, + ) + .join(", ")}.`; const adoptedSuggestion = survivor.suggestion === undefined ? otherClaims.find((claim) => claim.suggestion !== undefined) @@ -560,15 +911,31 @@ export const dedupeClaims = ( ? {author_dispute: adoptedDispute} : {}), }); + const clusterCount = others.filter((index) => + viaCluster.has(index), + ).length; + const via: MergeVia = + clusterCount === 0 + ? "similarity" + : clusterCount === others.length + ? "clusterer" + : "both"; merges.push({ survivor: survivor.id, merged: otherClaims.map((claim) => ({ id: claim.id, source: claim.source, label: claim.label, + ...(claim.line !== undefined && claim.line !== survivor.line + ? {line: claim.line} + : {}), })), path: survivor.path as string, line: survivor.line as number, + via, + ...(via === "similarity" || groupEvidence === undefined + ? {} + : {evidence: groupEvidence}), }); } return { @@ -576,5 +943,6 @@ export const dedupeClaims = ( .map((claim, index) => replacement.get(index) ?? claim) .filter((_, index) => !drop.has(index)), merges, + clusterRejections, }; }; diff --git a/workflows/review/lib/dispatch-cluster.test.ts b/workflows/review/lib/dispatch-cluster.test.ts new file mode 100644 index 00000000..465a9737 --- /dev/null +++ b/workflows/review/lib/dispatch-cluster.test.ts @@ -0,0 +1,342 @@ +import {describe, it, expect} from "vitest"; + +import {runDispatch, type AgentRunner, type DispatchFs} from "./dispatch"; +import {computeDiffProvenance} from "./provenance"; + +/** + * Defect-clustering tests (dedup tier 2): the `claim-clusterer` dispatch, the + * merge it unlocks, and every way it degrades. Split from dispatch.test.ts for + * its max-lines budget; the fixtures mirror that file's. + * + * The finder outputs are run 30587343777's real claim texts, trimmed: two + * reviewers describing ONE wrong doc comment in words that share almost + * nothing, which is the shape text similarity cannot reach (that run posted + * four such copies and merged none of them). + */ + +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)]), + ); + +/** A runner stub: canned final text per agent, throwing for names in fail. */ +const stubRunner = ( + outputs: Record, + fail: string[] = [], +): AgentRunner & {calls: string[]} => { + const calls: string[] = []; + const runner = (async (request) => { + calls.push(request.name); + if (fail.includes(request.name)) { + throw new Error("boom"); + } + 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; +}; + +const DIFF = [ + "diff --git a/a.ts b/a.ts", + "--- a/a.ts", + "+++ b/a.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}, + ]), + [`${REVIEW}/provenance.json`]: JSON.stringify(computeDiffProvenance(DIFF)), +}); + +const CORRECTNESS_OUT = JSON.stringify({ + findings: [ + { + path: "a.ts", + line: 2, + label: "issue (blocking)", + subject: "Broken guard.", + discussion: "The guard was removed.", + failure_scenario: "nil deref on empty input", + }, + ], + files: [{path: "a.ts", risk: "high"}], +}); + +const EMPTY_FINDINGS = JSON.stringify({findings: []}); + +const TRIAGE_OK = JSON.stringify({patterns: [], reviewFiles: ["a.ts"]}); + +const VALIDATOR_CONFIRM = JSON.stringify({ + claims: [ + { + id: "correctness-reviewer-1", + verification: "confirmed", + confidence: 0.9, + }, + ], +}); + +describe("runDispatch defect clustering (dedup tier 2)", () => { + const options = (fs: DispatchFs, runner: AgentRunner) => ({ + fs, + runner, + repoRoot: "/work", + }); + + /** + * Dedup tier 2 (the claim-clusterer): two reviewers describing one wrong + * comment in words that share almost nothing, the shape run 30587343777 + * posted four times. The finder outputs here are that run's real claim + * texts, trimmed; on text similarity alone they stay two comments. + */ + const CAP_NOTE = JSON.stringify({ + findings: [ + { + path: "a.ts", + line: 2, + label: "note (non-blocking)", + subject: + "Comment says the per-key cap is 10 but maxSamples is 25.", + discussion: + 'The comment on `maxSamples` reads "Keeps at most 10 samples per key" while the constant is 25.', + failure_scenario: + "Comment says the per-key cap is 10 but maxSamples is 25", + }, + ], + }); + const CAP_NITPICK = JSON.stringify({ + findings: [ + { + path: "a.ts", + line: 2, + label: "nitpick (non-blocking)", + subject: + "Declaration doc comment doesn't begin with the symbol name.", + discussion: + "Every other declaration comment in this file starts with the declared name; `// Keeps at most 10 samples per key.` above `const maxSamples = 25` is the sole one that omits the prefix.", + failure_scenario: + "A `go doc` reader won't associate this comment with `maxSamples`, and the odd-one-out style reads as an oversight.", + }, + ], + }); + + it("merges a defect the clusterer identifies and similarity cannot reach", async () => { + const fs = makeFakeFs({ + ...baseStaging(), + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-clusterer", + "claim-validator", + ), + }); + const runner = stubRunner({ + "pattern-triage": TRIAGE_OK, + "correctness-reviewer": CAP_NOTE, + "skill-auditor": CAP_NITPICK, + "claim-clusterer": JSON.stringify({ + clusters: [ + { + evidence: + "the doc comment on `maxSamples` says 10 while the constant is 25", + ids: ["correctness-reviewer-1", "skill-auditor-1"], + }, + ], + }), + "claim-validator": JSON.stringify({ + claims: [ + { + id: "correctness-reviewer-1", + verification: "confirmed", + confidence: 0.8, + }, + ], + }), + }); + const result = await runDispatch(options(fs, runner)); + + // The clusterer runs on the PRE-merge candidates, from its own staged + // file, and before the validator (which must never pay for a copy). + expect(runner.calls).toEqual([ + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-clusterer", + "claim-validator", + ]); + expect(JSON.parse(fs.files[`${REVIEW}/candidates.json`])).toHaveLength( + 2, + ); + expect(JSON.parse(fs.files[`${REVIEW}/claims.json`])).toHaveLength(1); + expect(result.claims).toMatchObject([{id: "correctness-reviewer-1"}]); + expect(result.claims[0].discussion).toContain( + "Also flagged by skill-auditor.", + ); + expect(result.merges[0].via).toBe("clusterer"); + // The audit block: candidate count and merge count come from here, not + // from what survives on the PR. + expect(result.clustering).toEqual({ + candidates: 2, + proposed: 1, + clusterMerges: 1, + rejected: [], + }); + expect( + JSON.parse(fs.files[`${REVIEW}/dispatch-result.json`]).clustering, + ).toEqual(result.clustering); + }); + + it("degrades to similarity alone when the clusterer output is unusable", async () => { + const fs = makeFakeFs({ + ...baseStaging(), + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-clusterer", + "claim-validator", + ), + }); + const runner = stubRunner( + { + "pattern-triage": TRIAGE_OK, + "correctness-reviewer": CAP_NOTE, + "skill-auditor": CAP_NITPICK, + "claim-validator": JSON.stringify({claims: []}), + }, + ["claim-clusterer"], + ); + const result = await runDispatch(options(fs, runner)); + // Both copies post, exactly as they do today: a clusterer failure is + // never a dropped or downgraded finding. + expect(result.claims).toHaveLength(2); + expect(result.merges).toEqual([]); + expect(result.clustering).toEqual({ + candidates: 2, + proposed: 0, + clusterMerges: 0, + rejected: [], + unavailable: true, + }); + // Not an author-facing dimension: duplicate hygiene never renders a + // "not assessed this run" note into the review body. + expect(result.noteLines).toEqual([]); + expect(result.skippedDimensions).toEqual([]); + }); + + it("never spends on clustering when one source produced every claim", async () => { + const fs = makeFakeFs({ + ...baseStaging(), + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-clusterer", + "claim-validator", + ), + }); + const runner = stubRunner({ + "pattern-triage": TRIAGE_OK, + "correctness-reviewer": CORRECTNESS_OUT, + "skill-auditor": EMPTY_FINDINGS, + "claim-validator": VALIDATOR_CONFIRM, + }); + const result = await runDispatch(options(fs, runner)); + expect(runner.calls).not.toContain("claim-clusterer"); + expect(result.clustering).toBeUndefined(); + expect(fs.files[`${REVIEW}/candidates.json`]).toBeUndefined(); + }); + + it("records the ids a clusterer invents rather than merging on them", async () => { + const fs = makeFakeFs({ + ...baseStaging(), + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-clusterer", + "claim-validator", + ), + }); + const runner = stubRunner({ + "pattern-triage": TRIAGE_OK, + "correctness-reviewer": CAP_NOTE, + "skill-auditor": CAP_NITPICK, + "claim-clusterer": JSON.stringify({ + clusters: [ + { + evidence: "the `maxSamples` cap comment", + ids: ["correctness-reviewer-1", "holistic-4"], + }, + ], + }), + "claim-validator": JSON.stringify({claims: []}), + }); + const result = await runDispatch(options(fs, runner)); + expect(result.claims).toHaveLength(2); + expect(result.clustering).toEqual({ + candidates: 2, + proposed: 1, + clusterMerges: 0, + rejected: [ + {id: "holistic-4", reason: "unknown-id"}, + {id: "correctness-reviewer-1", reason: "cluster-collapsed"}, + ], + }); + }); +}); diff --git a/workflows/review/lib/dispatch-cluster.ts b/workflows/review/lib/dispatch-cluster.ts new file mode 100644 index 00000000..315d99ba --- /dev/null +++ b/workflows/review/lib/dispatch-cluster.ts @@ -0,0 +1,117 @@ +/** + * The Step 3 defect-clustering step: dedup tier 2's dispatch and its audit + * record. Split out of `dispatch.ts` for the reason `dispatch-roster.ts` and + * `dispatch-contracts.ts` were, its max-lines budget (the shared + * `@khanacademy/eslint-config` caps a file at 1000 and this concern took it + * over), and following the same precedent: one concern per module, no + * behaviour change. + * + * What lives here is only the *plumbing* — stage the candidates, dispatch the + * `claim-clusterer`, parse its contract, and turn the outcome into telemetry. + * Every rule about what may merge, and every check on what the model asserted, + * lives in `dedup.ts` beside the similarity tier it extends. + * + * Determinism boundary: the sub-agent is a model; the skip rule, the parse, + * and the record are pure code. No prose about the code under review. + */ + +import type {ClaimMerge, ClusterRejection} from "./dedup"; +import {type Claim, type ProposedCluster} from "./dispatch-contracts"; + +export const CLUSTERER = "claim-clusterer"; + +/** Dedup tier 2 telemetry (dedup.ts owns the rules; this is the audit). */ +export type DispatchClustering = { + /** Claims the clusterer was given: the pre-merge candidate count. */ + candidates: number; + /** Well-formed clusters it proposed. */ + proposed: number; + /** Groups that merged with a tier-2 contribution (`via` is not similarity). */ + clusterMerges: number; + /** Proposed members that did not merge, with the rule that stopped them. */ + rejected: ClusterRejection[]; + /** The clusterer ran and returned nothing usable (tier 1 only this run). */ + unavailable?: boolean; +}; + +/** The dispatcher seams this step needs (closures over the run). */ +export type ClusterStepIo = { + /** Dispatch one agent, returning its final text (null when it failed). */ + dispatch: (name: string) => Promise; + /** Parse a contract with the dispatcher's one corrective re-dispatch. */ + parse: (name: string, output: string) => Promise; + /** Stage the candidate set the clusterer reads. */ + write: (content: string) => void; + /** Emit a run warning (the console seam, so this module stays pure). */ + warn: (message: string) => void; +}; + +export type ClusterStep = { + proposals: ProposedCluster[]; + dispatched: boolean; + unavailable: boolean; +}; + +/** + * Dispatch the clusterer over the PRE-merge candidate set. Staged as its own + * file rather than reusing `claims.json`, which by contract is the POST-merge + * set the validator reads. + * + * Skipped, with no spend, unless there is something only this tier can find: + * two claims from two different sources. A single reviewer's findings are never + * merged into each other (dedup.ts' rule, both tiers), so a one-source run has + * no candidate pair at all. + * + * Failure is soft in both directions. A missing definition (an extraction + * failure, since review.md and this lib ship at one pinned ref) or an unusable + * output leaves `proposals` empty, which degrades exactly to tier 1 — today's + * behavior. It deliberately does NOT become a skipped dimension: that list + * renders author-facing "not assessed this run" note lines about review + * dimensions, and duplicate hygiene is not a dimension of the review. It + * surfaces as a run warning and in the artifact's `clustering` block instead. + */ +export const runClusterStep = async ( + candidates: Claim[], + io: ClusterStepIo, +): Promise => { + const sources = new Set(candidates.map((claim) => claim.source)); + if (candidates.length < 2 || sources.size < 2) { + return {proposals: [], dispatched: false, unavailable: false}; + } + io.write(JSON.stringify(candidates, null, 2)); + const output = await io.dispatch(CLUSTERER); + const parsed = output === null ? null : await io.parse(CLUSTERER, output); + if (parsed === null) { + io.warn( + `::warning title=defect clustering::${CLUSTERER} output unavailable ` + + `over ${candidates.length} candidate(s); cross-source duplicates ` + + `merge on text similarity alone this run`, + ); + return {proposals: [], dispatched: true, unavailable: true}; + } + return {proposals: parsed, dispatched: true, unavailable: false}; +}; + +/** + * The run's clustering record, or undefined when the step never ran (nothing + * to cluster). `candidates` beside `clusterMerges` is what makes the merge rate + * readable from the artifact, which is the number to trust: duplicate comments + * that survive are later satisfied by one autofix edit, so the PR itself hides + * the symptom this tier exists to fix. + */ +export const clusteringRecord = ( + step: ClusterStep, + candidates: number, + merged: {merges: ClaimMerge[]; clusterRejections: ClusterRejection[]}, +): DispatchClustering | undefined => + step.dispatched + ? { + candidates, + proposed: step.proposals.length, + clusterMerges: merged.merges.filter( + (merge) => merge.via !== "similarity", + ).length, + rejected: merged.clusterRejections, + ...(step.unavailable ? {unavailable: true} : {}), + } + : undefined; diff --git a/workflows/review/lib/dispatch-contracts.test.ts b/workflows/review/lib/dispatch-contracts.test.ts index 4d06fdc9..415dad32 100644 --- a/workflows/review/lib/dispatch-contracts.test.ts +++ b/workflows/review/lib/dispatch-contracts.test.ts @@ -3,7 +3,9 @@ import {describe, it, expect} from "vitest"; import { applyVerifications, buildClaims, + contractValidator, joinProse, + parseClustererOutput, parseFinderOutput, parseValidatorOutput, type Claim, @@ -470,3 +472,63 @@ describe("applyVerifications: corrected-field validation", () => { expect(result[0].line).toBe(12); }); }); + +describe("parseClustererOutput", () => { + it("keeps well-formed clusters and dedupes the ids inside one", () => { + expect( + parseClustererOutput( + JSON.stringify({ + clusters: [ + { + evidence: + "the `maxSamples` comment says 10, not 25", + ids: ["a", "b", "a", "c"], + }, + ], + }), + ), + ).toEqual([ + { + evidence: "the `maxSamples` comment says 10, not 25", + ids: ["a", "b", "c"], + }, + ]); + }); + + it("skips entries that cannot merge anything, rather than voiding the dispatch", () => { + // A single-id "cluster", a missing evidence string, and a non-array + // `ids` each merge nothing on their own; dropping them keeps the rest + // of a mostly-good reply usable (a missed merge costs a duplicate + // comment, so partial credit is the safe direction here). + expect( + parseClustererOutput( + JSON.stringify({ + clusters: [ + {evidence: "`maxSamples` cap", ids: ["only-one"]}, + {ids: ["a", "b"]}, + {evidence: "`maxSamples` cap", ids: "a,b"}, + {evidence: "`maxSamples` cap", ids: ["a", 7, "b"]}, + ], + }), + ), + ).toEqual([{evidence: "`maxSamples` cap", ids: ["a", "b"]}]); + }); + + it("throws on a drifted shape so the corrective re-dispatch fires", () => { + // Reading a drifted reply as "no duplicates" is how a paid-for + // dimension goes missing without a trace. + expect(() => + parseClustererOutput(JSON.stringify({groups: []})), + ).toThrow(/no clusters array/); + expect(() => parseClustererOutput("not json")).toThrow(); + expect(parseClustererOutput(JSON.stringify({clusters: []}))).toEqual( + [], + ); + }); + + it("is the clusterer's structured-final contract check", () => { + const check = contractValidator("claim-clusterer", "clusterer"); + expect(check({clusters: []})).toBeNull(); + expect(check({groups: []})).toMatch(/no clusters array/); + }); +}); diff --git a/workflows/review/lib/dispatch-contracts.ts b/workflows/review/lib/dispatch-contracts.ts index fd0b49a2..3c312d5d 100644 --- a/workflows/review/lib/dispatch-contracts.ts +++ b/workflows/review/lib/dispatch-contracts.ts @@ -323,11 +323,69 @@ export const parseFinderOutput = ( }; }; +/* -------------------------------------------------------------------------- */ +/* Defect clustering (the claim-clusterer contract) */ +/* -------------------------------------------------------------------------- */ + +/** + * One group of candidate claims the `claim-clusterer` says describe ONE + * defect, plus the `evidence` it grounded the identity in: the code element, + * literal, or quoted text every member refers to. + * + * `evidence` is not documentation. It is the load-bearing half of the + * contract, because `dedup.ts` verifies the model's identity claim rather than + * trusting it: a cluster whose evidence names no code element at all is + * rejected outright, and a member whose own text never mentions that element + * is dropped from the group (see `verifiableClusters` there). A model asked + * for a grounded assertion is checkable; one asked only for a grouping is not. + */ +export type ProposedCluster = {evidence: string; ids: string[]}; + +/** + * Parse the clusterer's output, per its contract (review.md): + * `{"clusters": [{"evidence": "...", "ids": ["...", "..."]}]}`. Malformed + * entries are skipped rather than thrown on — every skipped entry simply + * merges nothing, which is the safe direction (a missed merge costs a + * duplicate comment; a bad one drops a reviewer's distinct finding). A + * missing `clusters` array IS thrown on, so the one corrective re-dispatch + * fires: silently reading a drifted shape as "no duplicates" is how a paid-for + * dimension goes missing without a trace. + */ +export const parseClustererOutput = (output: string): ProposedCluster[] => { + const parsed = parseJsonObject(output); + const raw = parsed["clusters"]; + if (!Array.isArray(raw)) { + throw new Error("clusterer output has no clusters array"); + } + return raw.flatMap((entry): ProposedCluster[] => { + if (!isRecord(entry) || typeof entry["evidence"] !== "string") { + return []; + } + const ids = entry["ids"]; + if (!Array.isArray(ids)) { + return []; + } + const strings = [ + ...new Set( + ids.filter((id): id is string => typeof id === "string"), + ), + ]; + return strings.length < 2 + ? [] + : [{evidence: entry["evidence"], ids: strings}]; + }); +}; + /* -------------------------------------------------------------------------- */ /* Structured-final contract checks */ /* -------------------------------------------------------------------------- */ -export type ContractKind = "finder" | "lens" | "validator" | "json"; +export type ContractKind = + | "finder" + | "lens" + | "validator" + | "clusterer" + | "json"; /** * Build the structured-final contract check for one sub-agent: the exact @@ -356,6 +414,8 @@ export const contractValidator = ( const text = JSON.stringify(payload); if (kind === "validator") { parseValidatorOutput(text); + } else if (kind === "clusterer") { + parseClustererOutput(text); } else if (kind === "finder" || kind === "lens") { parseFinderOutput(name, text, new Set(), kind === "lens"); } diff --git a/workflows/review/lib/dispatch.test.ts b/workflows/review/lib/dispatch.test.ts index d614fdc6..b6f9ec85 100644 --- a/workflows/review/lib/dispatch.test.ts +++ b/workflows/review/lib/dispatch.test.ts @@ -583,16 +583,21 @@ describe("runDispatch", () => { "pattern-triage", "correctness-reviewer", "skill-auditor", + "claim-clusterer", "claim-validator", ), }); + // The clusterer runs and proposes nothing: tier 1 owns this pair, and + // a silent tier 2 must not disturb it. const runner = stubRunner({ "pattern-triage": TRIAGE_OK, "correctness-reviewer": duplicate("issue (blocking)"), "skill-auditor": duplicate("nitpick (non-blocking)"), + "claim-clusterer": JSON.stringify({clusters: []}), "claim-validator": VALIDATOR_CONFIRM, }); const result = await runDispatch(options(fs, runner)); + expect(result.merges[0].via).toBe("similarity"); expect(result.claims).toMatchObject([{id: "correctness-reviewer-1"}]); expect(result.claims[0].discussion).toContain("Also flagged by"); // The validator was dispatched on the merged set, and the merge is diff --git a/workflows/review/lib/dispatch.ts b/workflows/review/lib/dispatch.ts index caab43d2..ed35cf25 100644 --- a/workflows/review/lib/dispatch.ts +++ b/workflows/review/lib/dispatch.ts @@ -47,11 +47,18 @@ import { type ClaimMerge, type ThreadSuppression, } from "./dedup"; +import { + clusteringRecord, + runClusterStep, + CLUSTERER, + type DispatchClustering, +} from "./dispatch-cluster"; import {annotateDiffLineNumbers, splitUnifiedDiff} from "./diff"; import { applyScopeFilter, buildClaims, contractValidator, + parseClustererOutput, parseFinderOutput, parseJsonObject, parseValidatorOutput, @@ -75,11 +82,13 @@ export { applyVerifications, buildClaims, contractValidator, + parseClustererOutput, parseFinderOutput, parseValidatorOutput, type Candidate, type Claim, type ContractKind, + type ProposedCluster, type Verification, } from "./dispatch-contracts"; export { @@ -89,10 +98,18 @@ export { type Roster, type RosterShed, } from "./dispatch-roster"; +export { + clusteringRecord, + runClusterStep, + CLUSTERER, + type ClusterStep, + type DispatchClustering, +} from "./dispatch-cluster"; export { dedupeClaims, suppressOpenThreadDuplicates, type ClaimMerge, + type ClusterRejection, type ThreadSuppression, } from "./dedup"; @@ -256,6 +273,15 @@ export type DispatchResult = { claims: Claim[]; /** Cross-source duplicates merged before validation (#245). */ merges: ClaimMerge[]; + /** + * 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 + * source). `candidates` is the pre-merge claim count, so the run's merge + * rate reads off the artifact — the number to trust, since autofix later + * satisfies duplicate comments with one edit and hides the symptom on the + * PR itself. + */ + clustering?: DispatchClustering; /** * Candidates dropped because an open bot thread already tracks the * defect (trial suggestion g). Blocking entries still floor the verdict @@ -374,6 +400,8 @@ export const runDispatch = async ( name, name === VALIDATOR ? "validator" + : name === CLUSTERER + ? "clusterer" : name === TRIAGE || name === RECONCILER ? "json" : lensNames.includes(name) @@ -752,7 +780,30 @@ 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. - const deduped = dedupeClaims(buildClaims(scoped.kept)); + // + // 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). + const candidateClaims = buildClaims(scoped.kept); + const clusterStep = await runClusterStep(candidateClaims, { + dispatch: dispatchAgent, + parse: (name, output) => + parseWithRetry(name, output, parseClustererOutput), + write: (content) => + fs.writeFileSync(`${REVIEW_DIR}/candidates.json`, content), + // eslint-disable-next-line no-console + warn: (message) => console.error(message), + }); + const deduped = dedupeClaims(candidateClaims, clusterStep.proposals); + const clustering = clusteringRecord( + clusterStep, + candidateClaims.length, + deduped, + ); let claims = deduped.claims; // Open-thread suppression (trial suggestion g), also before validation: @@ -852,6 +903,7 @@ export const runDispatch = async ( noteLines, claims, merges: deduped.merges, + ...(clustering !== undefined ? {clustering} : {}), threadSuppressions: suppression.suppressed, ...(threadSuppressionUnavailable !== undefined ? {threadSuppressionUnavailable} diff --git a/workflows/review/review.md b/workflows/review/review.md index fde4de51..a36e0521 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -619,7 +619,10 @@ cd gh-aw-review-lib && REVIEW_REPO_ROOT="$GITHUB_WORKSPACE" \ It runs triage, the reviewer fan-out (roster, budget cap, and planned sheds computed from `routing.json`, every dispatch staged to `out/.json`), the provenance gate, the scope filter, cross-source - dedup, open-thread suppression (a candidate that describes a defect an + dedup (text similarity plus the `claim-clusterer` dispatch, which names the + candidates that describe one defect; every merge rule stays in code, and the + run's merge rate is recorded in the result's `clustering` block), + open-thread suppression (a candidate that describes a defect an open bot thread already tracks is not re-validated or re-posted; a suppressed blocking candidate still floors the verdict when the matched thread's opener is itself blocking), and claim @@ -1518,6 +1521,69 @@ resolve or otherwise touch human threads — they are input only. Return ONLY this JSON object (no prose, no code fence): {"resolve": ["thread_id", "..."], "keep": ["thread_id", "..."], "skipLines": [{"path": "...", "line": 0}]} +## agent: `claim-clusterer` +--- +name: claim-clusterer +description: Groups the candidate comments that describe ONE defect, so several reviewers flagging the same thing post once; returns JSON. +model: claude-sonnet-4-6 +# effort: medium — launch default (clustering). Sonnet, not Opus: this is a +# text-identity judgment over already-written claims, with no code +# investigation and no prose to author. It is the cheapest agent in the +# pipeline and it removes claims from the most expensive one (the validator). +--- +You decide which candidate review comments describe the **same defect**, so that +several reviewers who found one problem leave one comment instead of four. You judge +**identity only** — never whether a claim is true (that is the claim-validator's job) +and never how it is worded. You have **no GitHub access**; read from disk and return +JSON only. + +Read from disk: +- The candidate comments: `/tmp/gh-aw/review/candidates.json` — each has `id`, `source` + (the reviewer that produced it), `path`, `line`, `label`, `subject`, `discussion`, + `failure_scenario` and `confidence`. +- The diff: `/tmp/gh-aw/review/pr.diff`, and the cited code: for each candidate you are + weighing, read the file at its `path` around its `line` from the checkout. Two claims + worded very differently are often obviously about the same line of code once you look + at it. Keep this shallow — you are locating claims, not investigating them. + +**One defect = one edit at one site.** Group candidates when a single change the author +makes would discharge every member's ask. That is the test, not topic similarity and not +proximity: + +- **Group** three reviewers who all say the comment above `const maxSamples = 25` is + wrong, even when one calls it a wrong cap, one quotes the comment, and one cites a + doc-comment convention: one rewritten comment satisfies all three. +- **Group** across lines. A defect is routinely flagged at different anchors (the + function, its doc comment, its test), and the pipeline keeps one anchor. Distance in + the file is not evidence of two defects. +- **Do NOT group** a bug and the missing test for that bug. "The cutoff subtracts months + instead of days" and "no test asserts a stale entry is deleted" cite the same facts and + need two different edits; they are two defects. +- **Do NOT group** two different properties of one symbol: "this query needs an index" + and "this query has no limit" both concern one line and are two defects. +- **Do NOT group** a claim that bundles two defects with either half. Leave it alone. +- **Do NOT group** two comments from the same `source`. A reviewer does not duplicate + itself, and the pipeline discards such a pairing. +- Group only candidates that share a `path`. + +**Ground every group in the code it is about.** Each group carries `evidence`: one short +phrase naming the code element its members share — the identifier, the literal, or the +quoted comment text (e.g. "the doc comment on `maxSamples` says 10 while the constant is +25"). This is checked mechanically: a group whose evidence names no code element, or a +member whose own text never mentions it, is discarded. So write evidence that quotes the +code, never a topic ("both are about comments" grounds nothing and voids the group). + +**When in doubt, leave them separate.** A wrong grouping silently drops a reviewer's +distinct finding; a missed one only costs a duplicate comment. Every `id` you name must +exist in `candidates.json`, and may appear in at most one group. The pipeline keeps the +most severe copy of each group (it absorbs the others into it and records who else +flagged it), so include every copy you find, whatever its label — you do not choose a +survivor, and you never edit a claim. + +Return ONLY this JSON object (no prose, no code fence), with `"clusters": []` when +nothing duplicates: +{"clusters": [{"evidence": "the code element every member is about", "ids": ["...", "..."]}]} + ## agent: `claim-validator` --- name: claim-validator From 93d59dde5795e1887ba54b9fefb6670651be3c48 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Fri, 31 Jul 2026 10:34:05 -0700 Subject: [PATCH 2/8] [jwies/defect-clustering] review: document the dedup eval gate and its recipe The powered-run recipe for a duplicate-comment change (the documentation-enabled cases are the ones that produce multi-source clusters), the report row's reading rules, and a note on the producer's in-place replacement of the pre-merge set. --- workflows/review/eval/README.md | 9 +++++++++ workflows/review/eval/live-producer.ts | 5 +++++ 2 files changed, 14 insertions(+) diff --git a/workflows/review/eval/README.md b/workflows/review/eval/README.md index 0c9633be..d40ff219 100644 --- a/workflows/review/eval/README.md +++ b/workflows/review/eval/README.md @@ -77,6 +77,15 @@ gh workflow run review-eval-ab.yml --ref \ gh workflow run review-eval-ab.yml --ref \ -f base_ref=origin/ -f force_arms=true -f full=true -f repeats=3 -f max_usd=220 +# Powered run for a dedup / duplicate-comment change (~$45): the cases that +# actually produce multi-source clusters are the `documentation`-enabled ones +# (that reviewer contributes the extra copy on a comment defect, which is the +# shape production duplicated in run 30587343777), 5x per arm +gh workflow run review-eval-ab.yml --ref \ + -f base_ref=origin/main \ + -f cases=golden-documentation-stale-and-narrated,golden-documentation-restated-docstring,golden-documentation-missing-why,golden-documentation-commented-out-code,clean-documentation-earned-comments \ + -f repeats=5 -f max_usd=50 + # Pool reports across dispatches (run ids or local paths) pnpm dlx tsx workflows/review/eval/aggregate.ts ... [--out ] ``` diff --git a/workflows/review/eval/live-producer.ts b/workflows/review/eval/live-producer.ts index 30c95f7f..ed152360 100644 --- a/workflows/review/eval/live-producer.ts +++ b/workflows/review/eval/live-producer.ts @@ -851,6 +851,11 @@ export const produceLive = async ( } // Cross-source dedup, before validation, exactly where production runs it. + // The merged set REPLACES the produced one from here on (the claims path, + // the validator's `knownIds`, and the returned findings all act on what + // production would post), so the collected array is rewritten in place + // rather than shadowed: a stray reference to the pre-merge set downstream + // would silently re-post the duplicates this stage just merged. const {kept: dedupedFindings, dedup} = await dedupeLiveFindings( findings, agents.get(CLUSTERER), From 531a00846a94c92bb74e01f68a5f2fede728220a Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Fri, 31 Jul 2026 12:35:30 -0700 Subject: [PATCH 3/8] [jwies/defect-clustering] review: record the merged groups per case, not just the count Auditing run 30651373253 turned up the gap: the report showed that four claims merged on the candidate arm and not WHICH, so reading a suspicious merge meant paying for the run again. A false merge is the failure mode of model-proposed clustering, and it is only diagnosable from the absorbed ids and the evidence the clusterer grounded them in, so perCase.dedup now carries the groups. Report-only: no reviewer-visible behaviour changes, so the powered run's numbers still stand. --- workflows/review/eval/live-ab-report.ts | 14 +++ workflows/review/eval/live-ab.test.ts | 120 ++++++++++++++++++++++++ workflows/review/eval/live-ab.ts | 15 +++ 3 files changed, 149 insertions(+) diff --git a/workflows/review/eval/live-ab-report.ts b/workflows/review/eval/live-ab-report.ts index 051c07c2..bc0e8e89 100644 --- a/workflows/review/eval/live-ab-report.ts +++ b/workflows/review/eval/live-ab-report.ts @@ -12,6 +12,7 @@ import type { CorpusCase, RecordedFinding, } from "./corpus/loader"; +import type {MergeVia} from "../lib/dedup"; import type {LiveCaseRun, LiveMetricsReport} from "./live-match"; import type { LiveDedupReport, @@ -72,6 +73,19 @@ export type ArmRunReport = { clusterMerged: number; rejected: number; clustererAbsent: boolean; + /** + * The merged groups themselves, so a suspicious merge is + * diagnosable from the artifact instead of from a repeat run: the + * survivor, the claim ids absorbed into it, which tier found the + * group, and (for a tier-2 group) the code element the clusterer + * grounded the identity in. + */ + groups: { + survivor: string; + absorbed: string[]; + via: MergeVia; + evidence?: string; + }[]; }; /** `: ` per failed agent (diagnosable from the report). */ failedAgents: string[]; diff --git a/workflows/review/eval/live-ab.test.ts b/workflows/review/eval/live-ab.test.ts index 11bb625a..25c58503 100644 --- a/workflows/review/eval/live-ab.test.ts +++ b/workflows/review/eval/live-ab.test.ts @@ -172,6 +172,126 @@ describe("selectCases", () => { }); }); +describe("runArm dedup accounting", () => { + /** + * The arm-level half of the duplicate-comment gate. A powered run reads its + * merge rate from here, so the plumbing has to carry the numbers AND the + * groups: the run that graduated the clusterer could see that four claims + * merged but not which, and a false merge is only diagnosable from the ids + * and the evidence the clusterer grounded them in. + */ + const produceMerged = + (clustererAbsent: boolean): ArmProduce => + async () => ({ + ...(await produceHit(1)(liveCase("case-1"))), + dedup: { + candidates: 4, + merges: [ + { + survivor: "live-hit", + merged: [ + { + id: "live-doc-1", + source: "documentation", + label: "suggestion (non-blocking, documentation)", + line: 9, + }, + ], + path: "src/a.ts", + line: 1, + via: "clusterer" as const, + evidence: "the `maxSamples` comment says 10, not 25", + }, + { + survivor: "live-hit", + merged: [ + { + id: "live-conv-1", + source: "conventions", + label: "nitpick (non-blocking)", + }, + ], + path: "src/a.ts", + line: 1, + via: "similarity" as const, + }, + ], + proposed: 1, + rejected: [], + clustererAbsent, + }, + }); + + it("carries the per-case counts and the merged groups", async () => { + const report = await runArm( + "candidate", + [liveCase("case-1")], + produceMerged(false), + {maxUsd: 10}, + ); + expect(report.perCase[0].dedup).toEqual({ + candidates: 4, + merged: 2, + clusterMerged: 1, + rejected: 0, + clustererAbsent: false, + groups: [ + { + survivor: "live-hit", + absorbed: ["live-doc-1"], + via: "clusterer", + evidence: "the `maxSamples` comment says 10, not 25", + }, + { + survivor: "live-hit", + absorbed: ["live-conv-1"], + via: "similarity", + }, + ], + }); + }); + + it("renders the merge row, marking an arm that never had the clusterer", async () => { + const baseline = await runArm( + "baseline", + [liveCase("case-1")], + produceMerged(true), + {maxUsd: 10}, + ); + const candidate = await runArm( + "candidate", + [liveCase("case-1")], + produceMerged(false), + {maxUsd: 10}, + ); + const markdown = renderMarkdownReport({ + baseRef: "origin/main", + reviewMdSha: {baseline: "a".repeat(12), candidate: "b".repeat(12)}, + arms: {baseline, candidate}, + regressions: {lost: [], gained: []}, + adversarialFailures: [], + gateRetries: [], + }); + expect(markdown).toContain( + "Cross-source claims merged (of candidates)", + ); + // `tier 1 only` is the expected baseline shape in the A/B that + // graduates the clusterer: a zero there is asymmetry, not a result. + expect(markdown).toContain("2 / 4 (tier 1 only)"); + expect(markdown).toContain("2 / 4 (1 by clusterer)"); + }); + + it("omits the block for a producer that runs no dedup at all", async () => { + const report = await runArm( + "candidate", + [liveCase("case-1")], + produceHit(1), + {maxUsd: 10}, + ); + expect(report.perCase[0].dedup).toBeUndefined(); + }); +}); + describe("runArm", () => { it("scores cases, accounts cost, and reports agent failures", async () => { const report = await runArm( diff --git a/workflows/review/eval/live-ab.ts b/workflows/review/eval/live-ab.ts index 1b4faad0..8b57d7af 100644 --- a/workflows/review/eval/live-ab.ts +++ b/workflows/review/eval/live-ab.ts @@ -253,6 +253,21 @@ export const runArm = async ( ), rejected: produced.dedup.rejected.length, clustererAbsent: produced.dedup.clustererAbsent, + // The groups themselves, not just the count: the + // powered run that graduated tier 2 could see THAT 4 + // claims merged but not WHICH, so auditing a + // suspicious merge meant paying for the run again. + // A false merge is the failure mode here, and it is + // only diagnosable from the ids and the evidence the + // clusterer grounded them in. + groups: produced.dedup.merges.map((merge) => ({ + survivor: merge.survivor, + absorbed: merge.merged.map((m) => m.id), + via: merge.via, + ...(merge.evidence !== undefined + ? {evidence: merge.evidence} + : {}), + })), }, }), failedAgents: produced.perAgent From a4a4bd0bcd262e668399fd6a0c27b20760847bc6 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Fri, 31 Jul 2026 13:34:59 -0700 Subject: [PATCH 4/8] [jwies/defect-clustering] review: hold every cluster member to tier 1's rules, and attribute each merge per copy Review feedback on the clustering tier, in the order it bites. A cluster unions its members on the model's word before any of them is checked, and the star guard then merged anything that cleared the TEXT floor against the survivor. So a proposed member on another path, or from the survivor's own source, merged there without ever meeting the rules `clusterMemberRejection` exists to apply, and the artifact recorded it as `via: "similarity"`: a reviewer's distinct (possibly blocking) finding dropped, attributed to the wrong tier, against the module's own guarantee. The branch now takes the full `mergeable` predicate, so such a member falls through to the tier-2 rules that reject it by name. The existing rules test passed only because its fixtures sit below the floor; the new one uses verbatim-identical copies, so the floor is decidedly not what keeps them apart. `no-anchor`, the one rejection reason with no coverage, gets a case too. `merges[].merged[]` gains a per-copy `via`, because a group's own `via` can be `both` and the A/B's `clusterMerged` column summed the whole group: tier 1's members were being credited to the clusterer in the number that decides graduation. It is counted per absorbed copy now, in the report and in the eval's per-case groups. The "also flagged by" note quotes the subject of any copy tier 2 absorbed. One edit discharging every member's ask does not mean every member asked in the same words: run 30587343777's `conventions` copy wanted the symbol-name prefix, not the corrected number, and the survivor's prose says nothing about it. Tier-1 copies stay unquoted, since clearing the floor against the survivor is the evidence that they restate it, and repeating four near-identical subjects would move the duplicate noise into the surviving comment instead of removing it. The dispatch gate was ">= 2 claims from >= 2 sources", which still pays for a serial clusterer run when the only cross-source pairs sit in different files or are blocking on both sides; neither is mergeable at tier 2 under any proposal. It now gates on a legally-mergeable pair, computed from the candidates already in hand, and the eval's producer imports that same predicate rather than restating it. Also: the clusterer's frontmatter justified sonnet with "no code investigation" while the prompt has it read each candidate's cited lines. The rationale now says what the prompt actually asks for (locating, capped there) so nobody sizes tool permissions off it, and the producer's doc names which parts of the step are shared code and which are the hand-mirrored seam. --- .changeset/review-defect-clustering.md | 32 ++-- workflows/review/eval/README.md | 9 +- workflows/review/eval/live-ab-report.ts | 24 +-- workflows/review/eval/live-ab.test.ts | 42 ++++-- workflows/review/eval/live-ab.ts | 26 +++- workflows/review/eval/live-producer.test.ts | 3 +- workflows/review/eval/live-producer.ts | 14 +- workflows/review/lib/dedup-cluster.test.ts | 139 +++++++++++++++++- workflows/review/lib/dedup.ts | 94 +++++++++--- workflows/review/lib/dispatch-cluster.test.ts | 60 +++++++- workflows/review/lib/dispatch-cluster.ts | 46 +++++- workflows/review/review.md | 10 +- 12 files changed, 417 insertions(+), 82 deletions(-) diff --git a/.changeset/review-defect-clustering.md b/.changeset/review-defect-clustering.md index acfaba7c..ced9eadd 100644 --- a/.changeset/review-defect-clustering.md +++ b/.changeset/review-defect-clustering.md @@ -42,17 +42,27 @@ never lose a blocking finding or soften a verdict. The accepted price: one defec flagged blocking by two sources in different words still posts twice unless tier 1 reaches it. -Degradation is soft in both directions. Fewer than two claims, or one source, -and the clusterer is never dispatched (no spend). A missing definition or an -unusable reply leaves the run on tier 1, exactly today's behavior, and surfaces -as a run warning plus a `clustering` block in `dispatch-result.json` -(`candidates`, `proposed`, `clusterMerges`, and every rejected member with the -rule that stopped it) rather than as an author-facing note: duplicate hygiene is -not a review dimension. Each merge in `merges` now carries `via` -(`similarity`/`clusterer`/`both`) and the merged copies' own anchors, and the -"also flagged by" note names a source's line when it differs from the survivor's, -so the merge rate reads off the artifact instead of off a PR that autofix has -already tidied. +Degradation is soft in both directions. The clusterer is dispatched only when the +candidates hold a pair it could legally merge (two anchored claims on one path +from two sources, at least one non-blocking), so a run with nothing to find never +pays for the step. A missing definition or an unusable reply leaves the run on +tier 1, exactly today's behavior, and surfaces as a run warning plus a +`clustering` block in `dispatch-result.json` (`candidates`, `proposed`, +`clusterMerges`, and every rejected member with the rule that stopped it) rather +than as an author-facing note: duplicate hygiene is not a review dimension. Each +merge in `merges` now carries `via` (`similarity`/`clusterer`/`both`) plus the +tier and anchor of each absorbed copy, so the merge rate reads off the artifact +instead of off a PR that autofix has already tidied. + +The "also flagged by" note names a source's line when it differs from the +survivor's, and quotes the subject of any copy tier 2 absorbed. One edit +discharging every member's ask does not mean every member asked in the same +words; run 30587343777's `conventions` copy wanted the symbol-name prefix, not +the corrected number. Tier 2 is exactly the case where the survivor's own +prose is known not to restate it (the text floor is what those copies could not +clear). Tier-1 copies are not quoted: clearing that floor against the survivor is +the evidence that they say the same thing, and repeating them would move the +duplicate noise into the surviving comment rather than remove it. The live A/B now runs dedup, which it never did: a change to the merge rules was unmeasurable by construction before this. Tier 1 runs in both arms (it is shared diff --git a/workflows/review/eval/README.md b/workflows/review/eval/README.md index d40ff219..a3aba1d7 100644 --- a/workflows/review/eval/README.md +++ b/workflows/review/eval/README.md @@ -164,9 +164,12 @@ claiming a band. that predates the agent reports `tier 1 only` and the arm delta prices the clusterer alone. Read it beside recall: a false merge drops a distinct finding, so it shows up as candidate-arm recall loss, not as a better - duplicate number. `rejected` counts proposals the merge rules refused - (`unknown-id` there means the clusterer named claims that do not exist, which - is a prompt or staging failure rather than a quiet zero). + duplicate number. The `by clusterer` share counts absorbed COPIES, not groups, + so a group both tiers contributed to credits tier 2 only with what it actually + brought. `rejected` counts cluster MEMBERS the merge rules refused, so one bad + proposal naming three ids counts three (`unknown-id` there means the clusterer + named claims that do not exist, which is a prompt or staging failure rather + than a quiet zero). - **Anchor-snap and the arms:** the deterministic pipeline is shared by both arms, but the provenance gate emulates each arm's OWN review.md gate version, keyed on the literal `anchor-snap` marker in the gate step. A diff --git a/workflows/review/eval/live-ab-report.ts b/workflows/review/eval/live-ab-report.ts index bc0e8e89..47768d8e 100644 --- a/workflows/review/eval/live-ab-report.ts +++ b/workflows/review/eval/live-ab-report.ts @@ -60,12 +60,17 @@ export type ArmRunReport = { snapped: number; /** * The cross-source merge, per case: `candidates` is the pre-merge claim - * count, `merged` the claims it absorbed, and `clusterMerged` the subset - * tier 2 (the `claim-clusterer`) contributed to. Read the duplicate rate - * from these, never from the posted set — merges happen upstream of - * every drop the pipeline applies afterwards, and in production autofix - * later satisfies surviving duplicates with one edit and hides them. - * `clustererAbsent` marks the arm that never had tier 2 at all. + * count, `merged` the claims it absorbed, and `clusterMerged` how many + * of those copies tier 2 (the `claim-clusterer`) is what absorbed, + * counted per copy since a `both` group absorbed some of its members on + * the text floor. `rejected` counts proposed MEMBERS the merge rules + * turned down, so one bad proposal naming three ids counts three. + * + * Read the duplicate rate from these, never from the posted set: merges + * happen upstream of every drop the pipeline applies afterwards, and in + * production autofix later satisfies surviving duplicates with one edit + * and hides them. `clustererAbsent` marks the arm that never had tier 2 + * at all. */ dedup?: { candidates: number; @@ -82,7 +87,7 @@ export type ArmRunReport = { */ groups: { survivor: string; - absorbed: string[]; + absorbed: {id: string; via?: "clusterer"}[]; via: MergeVia; evidence?: string; }[]; @@ -285,7 +290,8 @@ const snappedTotal = (arm: ArmRunReport): number => /** * The arm's cross-source merge rate: claims absorbed over claims produced, - * with tier 2's share and any rejected proposal in parentheses. `tier 1 only` + * with tier 2's share and any rejected cluster MEMBER in parentheses (one + * proposal naming three ids that all fail is three). `tier 1 only` * marks an arm whose review.md defines no `claim-clusterer` — the expected * shape of the baseline in the A/B that graduates it, and the reason a zero in * the clusterer column there is asymmetry, not a negative result. @@ -301,7 +307,7 @@ const mergedTotal = (arm: ArmRunReport): string => { const notes = [ absent ? "tier 1 only" : `${sum((d) => d.clusterMerged)} by clusterer`, ...(sum((d) => d.rejected) > 0 - ? [`${sum((d) => d.rejected)} proposal(s) rejected`] + ? [`${sum((d) => d.rejected)} proposed member(s) rejected`] : []), ]; return `${sum((d) => d.merged)} / ${sum((d) => d.candidates)} (${notes.join( diff --git a/workflows/review/eval/live-ab.test.ts b/workflows/review/eval/live-ab.test.ts index 25c58503..783ab206 100644 --- a/workflows/review/eval/live-ab.test.ts +++ b/workflows/review/eval/live-ab.test.ts @@ -195,6 +195,7 @@ describe("runArm dedup accounting", () => { source: "documentation", label: "suggestion (non-blocking, documentation)", line: 9, + via: "clusterer" as const, }, ], path: "src/a.ts", @@ -202,18 +203,29 @@ describe("runArm dedup accounting", () => { via: "clusterer" as const, evidence: "the `maxSamples` comment says 10, not 25", }, + // A group BOTH tiers contributed to: tier 1 reached the + // conventions copy on its own and only the holistic one + // needed the clusterer, so exactly one of these two copies + // is tier 2's to claim. { - survivor: "live-hit", + survivor: "live-hit-2", merged: [ { id: "live-conv-1", source: "conventions", label: "nitpick (non-blocking)", }, + { + id: "live-holistic-1", + source: "holistic", + label: "note (non-blocking)", + via: "clusterer" as const, + }, ], path: "src/a.ts", - line: 1, - via: "similarity" as const, + line: 4, + via: "both" as const, + evidence: "the `staleAfter` window", }, ], proposed: 1, @@ -229,23 +241,31 @@ describe("runArm dedup accounting", () => { produceMerged(false), {maxUsd: 10}, ); + // `clusterMerged` counts absorbed COPIES the clusterer is responsible + // for, not every copy in a group it touched: the `both` group below + // carries one of each, and crediting tier 2 with the pair would + // overstate the delta that decides graduation. expect(report.perCase[0].dedup).toEqual({ candidates: 4, - merged: 2, - clusterMerged: 1, + merged: 3, + clusterMerged: 2, rejected: 0, clustererAbsent: false, groups: [ { survivor: "live-hit", - absorbed: ["live-doc-1"], + absorbed: [{id: "live-doc-1", via: "clusterer"}], via: "clusterer", evidence: "the `maxSamples` comment says 10, not 25", }, { - survivor: "live-hit", - absorbed: ["live-conv-1"], - via: "similarity", + survivor: "live-hit-2", + absorbed: [ + {id: "live-conv-1"}, + {id: "live-holistic-1", via: "clusterer"}, + ], + via: "both", + evidence: "the `staleAfter` window", }, ], }); @@ -277,8 +297,8 @@ describe("runArm dedup accounting", () => { ); // `tier 1 only` is the expected baseline shape in the A/B that // graduates the clusterer: a zero there is asymmetry, not a result. - expect(markdown).toContain("2 / 4 (tier 1 only)"); - expect(markdown).toContain("2 / 4 (1 by clusterer)"); + expect(markdown).toContain("3 / 4 (tier 1 only)"); + expect(markdown).toContain("3 / 4 (2 by clusterer)"); }); it("omits the block for a producer that runs no dedup at all", async () => { diff --git a/workflows/review/eval/live-ab.ts b/workflows/review/eval/live-ab.ts index 8b57d7af..74c0f450 100644 --- a/workflows/review/eval/live-ab.ts +++ b/workflows/review/eval/live-ab.ts @@ -245,12 +245,19 @@ export const runArm = async ( (sum, merge) => sum + merge.merged.length, 0, ), - clusterMerged: produced.dedup.merges - .filter((merge) => merge.via !== "similarity") - .reduce( - (sum, merge) => sum + merge.merged.length, - 0, - ), + // Counted per absorbed copy, not per group: a `both` + // group merged some of its members on the text floor + // and only the rest on the clusterer's word, and + // crediting the whole group to tier 2 would overstate + // the delta this arm is asked to justify. + clusterMerged: produced.dedup.merges.reduce( + (sum, merge) => + sum + + merge.merged.filter( + (copy) => copy.via === "clusterer", + ).length, + 0, + ), rejected: produced.dedup.rejected.length, clustererAbsent: produced.dedup.clustererAbsent, // The groups themselves, not just the count: the @@ -262,7 +269,12 @@ export const runArm = async ( // clusterer grounded them in. groups: produced.dedup.merges.map((merge) => ({ survivor: merge.survivor, - absorbed: merge.merged.map((m) => m.id), + absorbed: merge.merged.map((copy) => ({ + id: copy.id, + ...(copy.via !== undefined + ? {via: copy.via} + : {}), + })), via: merge.via, ...(merge.evidence !== undefined ? {evidence: merge.evidence} diff --git a/workflows/review/eval/live-producer.test.ts b/workflows/review/eval/live-producer.test.ts index ecc28085..33d666da 100644 --- a/workflows/review/eval/live-producer.test.ts +++ b/workflows/review/eval/live-producer.test.ts @@ -683,7 +683,8 @@ describe("produceLive cross-source dedup", () => { // validator is dispatched over the merged set only. expect(result.findings).toHaveLength(1); expect(result.findings[0].finding.model_authored_prose).toContain( - "Also flagged by skill.", + "Also flagged by:\n- skill: Declaration doc comment doesn't begin " + + "with the symbol name.", ); expect( JSON.parse( diff --git a/workflows/review/eval/live-producer.ts b/workflows/review/eval/live-producer.ts index ed152360..fa57bd6f 100644 --- a/workflows/review/eval/live-producer.ts +++ b/workflows/review/eval/live-producer.ts @@ -57,6 +57,7 @@ import { type Candidate, type ProposedCluster, } from "../lib/dispatch-contracts"; +import {hasClusterableCandidatePair} from "../lib/dispatch-cluster"; import { VERIFICATION_STATES, type CaseVerification, @@ -492,6 +493,16 @@ export type LiveDedupReport = { * projection puts the whole prose in `subject` and the evidence trace in * `discussion`. Feeding that shape to the floors would measure a similarity * arithmetic production never runs. + * + * What is shared with production and what is not, since fidelity is the whole + * point: the merge rules (`dedupeClaims`) and the dispatch precondition + * (`hasClusterableCandidatePair`) are the SAME code `runClusterStep` runs. What + * this function re-implements is the plumbing that step cannot lend: the + * dispatch goes through the eval's own agent runner and per-agent cost report, + * and the survivor's merged prose is written back onto a `LiveFinding` rather + * than onto a staged claims.json. That is the seam to keep in step by hand + * (the provenance gate's anchor-snap emulation has the same shape); a rule + * change does not need mirroring here, a change to WHEN the step runs does. */ const dedupeLiveFindings = async ( findings: LiveFinding[], @@ -511,8 +522,7 @@ const dedupeLiveFindings = async ( dedup: {report?: PerAgentReport; result: LiveDedupReport}; }> => { const claims = buildLibClaims(findings as Candidate[]); - const sources = new Set(claims.map((claim) => claim.source)); - const clusterable = claims.length > 1 && sources.size > 1; + const clusterable = hasClusterableCandidatePair(claims); let proposals: ProposedCluster[] = []; let report: PerAgentReport | undefined; if (clusterable && clusterer !== undefined) { diff --git a/workflows/review/lib/dedup-cluster.test.ts b/workflows/review/lib/dedup-cluster.test.ts index a63b2eb7..eefb83f4 100644 --- a/workflows/review/lib/dedup-cluster.test.ts +++ b/workflows/review/lib/dedup-cluster.test.ts @@ -165,10 +165,21 @@ describe("dedupeClaims with model-proposed clusters", () => { ); expect(claims.map((c) => c.id)).toEqual(["correctness-reviewer-3"]); // Anchors differ inside the cluster (:8 and :9), which tier 2 does not - // care about and the note does report. + // care about and the note does report. Each absorbed copy also brings + // its own subject, because tier 2 merged claims whose words the + // survivor's prose does NOT restate: `conventions` asked for the + // symbol-name prefix, not for the wrong number, and one rewritten + // comment discharges both asks only if the author is told about both. expect(claims[0].discussion).toContain( - "Also flagged by skill-auditor (out-of-lane) (at line 9), " + - "conventions, documentation.", + [ + "Also flagged by:", + "- skill-auditor (out-of-lane) (at line 9): The comment on line 8 " + + 'says "Keeps at most 10 samples per key." but `const maxSamples ' + + "= 25`, so the doc and the enforced cap disagree.", + "- conventions: Declaration doc comment doesn't begin with the " + + "symbol name.", + "- documentation: Comment states the wrong cap (10 vs 25).", + ].join("\n"), ); expect(merges).toEqual([ { @@ -179,16 +190,19 @@ describe("dedupeClaims with model-proposed clusters", () => { source: "skill-auditor (out-of-lane)", label: "question (non-blocking)", line: 9, + via: "clusterer", }, { id: "conventions-1", source: "conventions", label: "nitpick (non-blocking)", + via: "clusterer", }, { id: "documentation-1", source: "documentation", label: "suggestion (non-blocking, documentation)", + via: "clusterer", }, ], path: "dev/af19_trial/window.go", @@ -365,6 +379,91 @@ describe("dedupeClaims with model-proposed clusters", () => { ]); }); + it("holds a cluster member to those rules when its text clears the floor too", () => { + // The test above proves the rules only for members tier 1's floor + // cannot reach, which is every fixture the clusterer is built for. The + // dangerous member is the opposite one: a cluster unions on the model's + // word BEFORE any member is checked, so a member whose prose does clear + // the floor would take the similarity branch, merge without ever + // meeting the path or source rule, and be recorded as + // `via: "similarity"`: a reviewer's distinct finding dropped, with the + // artifact naming the wrong tier. These copies are verbatim identical, + // so the floor is decidedly not what keeps them apart. + const [note] = wrongCapClaims(); + const {claims, merges, clusterRejections} = dedupeClaims( + [ + note, + { + ...note, + id: "documentation-9", + source: "documentation", + path: "dev/af19_trial/other.go", + }, + {...note, id: "correctness-reviewer-9"}, + ], + [ + { + evidence: "the `maxSamples` cap comment says 10, not 25", + ids: [ + "correctness-reviewer-3", + "documentation-9", + "correctness-reviewer-9", + ], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual([ + "correctness-reviewer-3", + "documentation-9", + "correctness-reviewer-9", + ]); + expect(merges).toEqual([]); + expect(clusterRejections).toEqual([ + {id: "documentation-9", reason: "other-path"}, + {id: "correctness-reviewer-9", reason: "same-source"}, + ]); + }); + + it("records a proposed member that cannot anchor a comment", () => { + // `path`/`line` are optional on a Claim (a finding whose anchor the + // provenance gate could not place keeps its prose and loses its + // anchor), and the survivor's anchor is what the merged comment posts + // on, so an anchorless member is dropped from the cluster and recorded + // rather than silently swallowed by the group it was named in. + const [note, question, conventions] = wrongCapClaims(); + const {line: _, ...anchorless} = { + ...conventions, + id: "holistic-2", + source: "holistic", + }; + const {claims, merges, clusterRejections} = dedupeClaims( + [note, question, anchorless], + [ + { + evidence: + "the doc comment on `maxSamples` says 10 while the constant is 25", + ids: [ + "correctness-reviewer-3", + "holistic-2", + "skill-auditor-ool-2", + ], + }, + ], + ); + // The rest of the cluster still merges, and the anchorless claim posts + // as its own comment (on whatever anchor rendering gives it). + expect(claims.map((c) => c.id)).toEqual([ + "correctness-reviewer-3", + "holistic-2", + ]); + expect(merges[0].merged.map((m) => m.id)).toEqual([ + "skill-auditor-ool-2", + ]); + expect(clusterRejections).toEqual([ + {id: "holistic-2", reason: "no-anchor"}, + ]); + }); + it("records ids the clusterer invented, and holds each claim to one cluster", () => { // The webapp#41197 lesson applied to tier 2: an empty merge list must // never be the only evidence. A clusterer naming claims that do not @@ -440,7 +539,8 @@ describe("dedupeClaims with model-proposed clusters", () => { expect(claims.map((c) => c.id)).toEqual(["test-adequacy-1"]); expect(claims[0].label).toBe("todo (blocking)"); expect(claims[0].discussion).toContain( - "Also flagged by first-principles (at line 58).", + "Also flagged by:\n- first-principles (at line 58): " + + "The suite never reaches the delete.", ); expect(merges[0].via).toBe("clusterer"); expect(merges[0].line).toBe(15); @@ -496,9 +596,34 @@ describe("dedupeClaims with model-proposed clusters", () => { expect(claims.map((c) => c.id)).toEqual(["correctness-reviewer-3"]); expect(merges).toHaveLength(1); expect(merges[0].via).toBe("both"); - expect(merges[0].merged.map((m) => m.id)).toEqual([ - "skill-auditor-ool-2", - "first-principles-4", + // Per COPY, not per group: only `first-principles-4` needed the + // clusterer, and a reader counting tier 2's contribution from the + // group's own `both` would credit it with the pair tier 1 already + // had. The A/B's `clusterMerged` column reads this field. + expect(merges[0].merged).toEqual([ + { + id: "skill-auditor-ool-2", + source: "skill-auditor (out-of-lane)", + label: "question (non-blocking)", + line: 58, + }, + { + id: "first-principles-4", + source: "first-principles", + label: "note (non-blocking)", + line: 58, + via: "clusterer", + }, ]); + // The note quotes only the copy tier 2 brought: the survivor's own + // prose does not restate it, while the tier-1 copy's clearing of the + // floor is the evidence that it does. + expect(claims[0].discussion).toContain( + [ + "Also flagged by:", + "- skill-auditor (out-of-lane) (at line 58)", + "- first-principles (at line 58): The suite never reaches the delete.", + ].join("\n"), + ); }); }); diff --git a/workflows/review/lib/dedup.ts b/workflows/review/lib/dedup.ts index 19a817a5..c8364ab4 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -52,9 +52,11 @@ * inside tier 1 and as the survivor's posting anchor, and nothing more. * * The survivor is the highest-severity copy, its discussion gains an "also - * flagged by" note (naming each other source's anchor when it differs), and - * every merge is recorded for dispatch-result.json with the tier that found - * it, so the merge rate is readable from the artifact rather than from what + * flagged by" note (naming each other source's anchor when it differs, and + * quoting the subject of any copy tier 2 absorbed, whose ask the survivor's own + * prose is not known to restate), and every merge is recorded for + * dispatch-result.json with the tier that found it (per group and per absorbed + * copy), so the merge rate is readable from the artifact rather than from what * survives on the PR. * * Determinism boundary: every merge RULE is code — pure text arithmetic, no @@ -81,6 +83,14 @@ export type ClaimMerge = { label: string; /** The merged copy's own anchor, when it differs from the survivor's. */ line?: number; + /** + * Present when THIS copy was absorbed on the clusterer's assertion + * rather than on the text floor; absent means tier 1 merged it. Per + * member because a group's own `via` can be `both`, and a reader + * counting tier 2's contribution from the group would then attribute + * tier 1's members to the clusterer. + */ + via?: "clusterer"; }[]; path: string; line: number; @@ -804,8 +814,8 @@ export const dedupeClaims = ( evidenceTokens !== undefined && evidenceTokens.size > 0 && sharesSalientToken(evidenceTokens, survivor); - // Star guard: only a member that clears the floor against the - // survivor DIRECTLY merges. Union-find alone chains A~B~C through a + // Star guard: only a member {@link mergeable} against the survivor + // DIRECTLY merges on tier 1. Union-find alone chains A~B~C through a // bridging claim that bundles two defects (a test-adequacy finding // naming both a missing test and an unbounded read links the two // distinct correctness findings), and collapsing the chain would @@ -815,6 +825,14 @@ export const dedupeClaims = ( // 29897276810's four-way group is pairwise-complete and run // 29943085279's is a direct pair. // + // The full pairwise predicate, not the text floor alone: a group can + // now contain a member tier 1 never proposed (a cluster unions on the + // model's word, which {@link verifiableClusters} does not screen for + // path or source), and a cross-path or same-source member whose prose + // happens to clear the floor must not slip in through this branch and + // be recorded as `via: "similarity"`. It falls through to the tier-2 + // rules below, which reject it by name. + // // A cluster member takes the tier-2 path instead: it did not clear the // floor (that is why the clusterer exists), so it merges on the // verified rules alone. The evidence check runs against THIS survivor, @@ -824,7 +842,7 @@ export const dedupeClaims = ( if (index === survivorIndex) { return false; } - if (describesSameDefect(survivor, claims[index])) { + if (mergeable(survivor, claims[index])) { return true; } if (clusterOf.get(index) === undefined) { @@ -865,8 +883,22 @@ export const dedupeClaims = ( // second reviewer was looking at a different line, which is exactly // the context an author needs to judge a same-defect-different-anchor // merge (and to spot a wrong one). - const sources: {source: string; line?: number}[] = []; - for (const claim of otherClaims) { + // + // A tier-2 copy also carries its own SUBJECT, and only a tier-2 copy + // does. What the note must not lose is an ask the survivor's prose + // does not already make: run 30587343777's `conventions` copy of the + // wrong-cap defect asked for the symbol-name prefix, not for the + // number, and one rewritten comment discharges both only if the author + // is told about both. Tier 1 needs no such quote by construction: a + // copy merged there cleared the text floor AGAINST the survivor, which + // is exactly the evidence that it restates the survivor's own words; + // repeating four near-identical subjects would move the duplicate + // noise into the surviving comment rather than remove it. Tier 2 is + // the case where that evidence is missing (the floor is what it could + // not clear), so there the quote is the only thing carrying the ask. + const sources: {source: string; line?: number; subject?: string}[] = []; + for (const index of others) { + const claim = claims[index]; if ( claim.source === survivor.source || sources.some((seen) => seen.source === claim.source) @@ -878,18 +910,30 @@ export const dedupeClaims = ( ...(claim.line !== undefined && claim.line !== survivor.line ? {line: claim.line} : {}), + ...(viaCluster.has(index) + ? {subject: claim.subject.replace(/\s+/g, " ").trim()} + : {}), }); } + const flaggedBy = (entry: {source: string; line?: number}): string => + entry.line === undefined + ? entry.source + : `${entry.source} (at line ${entry.line})`; const alsoFlagged = sources.length === 0 ? "" - : `\n\nAlso flagged by ${sources - .map((entry) => - entry.line === undefined - ? entry.source - : `${entry.source} (at line ${entry.line})`, + : sources.every((entry) => entry.subject === undefined) + ? `\n\nAlso flagged by ${sources.map(flaggedBy).join(", ")}.` + : `\n\nAlso flagged by:\n${sources + .map( + (entry) => + `- ${flaggedBy(entry)}${ + entry.subject === undefined + ? "" + : `: ${entry.subject}` + }`, ) - .join(", ")}.`; + .join("\n")}`; const adoptedSuggestion = survivor.suggestion === undefined ? otherClaims.find((claim) => claim.suggestion !== undefined) @@ -922,14 +966,20 @@ export const dedupeClaims = ( : "both"; merges.push({ survivor: survivor.id, - merged: otherClaims.map((claim) => ({ - id: claim.id, - source: claim.source, - label: claim.label, - ...(claim.line !== undefined && claim.line !== survivor.line - ? {line: claim.line} - : {}), - })), + merged: others.map((index) => { + const claim = claims[index]; + return { + id: claim.id, + source: claim.source, + label: claim.label, + ...(claim.line !== undefined && claim.line !== survivor.line + ? {line: claim.line} + : {}), + ...(viaCluster.has(index) + ? {via: "clusterer" as const} + : {}), + }; + }), path: survivor.path as string, line: survivor.line as number, via, diff --git a/workflows/review/lib/dispatch-cluster.test.ts b/workflows/review/lib/dispatch-cluster.test.ts index 465a9737..672f2f83 100644 --- a/workflows/review/lib/dispatch-cluster.test.ts +++ b/workflows/review/lib/dispatch-cluster.test.ts @@ -225,7 +225,8 @@ describe("runDispatch defect clustering (dedup tier 2)", () => { expect(JSON.parse(fs.files[`${REVIEW}/claims.json`])).toHaveLength(1); expect(result.claims).toMatchObject([{id: "correctness-reviewer-1"}]); expect(result.claims[0].discussion).toContain( - "Also flagged by skill-auditor.", + "Also flagged by:\n- skill-auditor: Declaration doc comment " + + "doesn't begin with the symbol name.", ); expect(result.merges[0].via).toBe("clusterer"); // The audit block: candidate count and merge count come from here, not @@ -302,6 +303,63 @@ describe("runDispatch defect clustering (dedup tier 2)", () => { expect(fs.files[`${REVIEW}/candidates.json`]).toBeUndefined(); }); + it("never spends on clustering when no candidate pair could legally merge", async () => { + // Two sources, two claims, and still nothing tier 2 may do with them: + // they sit in different files, and cross-file merging is out of both + // tiers. The count-and-source gate alone would pay for a serial + // dispatch whose every proposal the merge rules must reject. + const twoFileDiff = [ + DIFF.trimEnd(), + "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 fs = makeFakeFs({ + ...baseStaging(), + [`${REVIEW}/full.diff`]: twoFileDiff, + [`${REVIEW}/provenance.json`]: JSON.stringify( + computeDiffProvenance(twoFileDiff), + ), + [`${REVIEW}/files.json`]: JSON.stringify([ + {path: "a.ts", status: "modified", hasPatch: true}, + {path: "b.ts", status: "modified", hasPatch: true}, + ]), + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-clusterer", + "claim-validator", + ), + }); + const runner = stubRunner({ + "pattern-triage": JSON.stringify({ + patterns: [], + reviewFiles: ["a.ts", "b.ts"], + }), + "correctness-reviewer": CAP_NOTE, + "skill-auditor": JSON.stringify({ + findings: [ + { + ...JSON.parse(CAP_NITPICK).findings[0], + path: "b.ts", + line: 2, + }, + ], + }), + "claim-validator": JSON.stringify({claims: []}), + }); + const result = await runDispatch(options(fs, runner)); + expect(runner.calls).not.toContain("claim-clusterer"); + expect(result.clustering).toBeUndefined(); + expect(result.claims).toHaveLength(2); + }); + it("records the ids a clusterer invents rather than merging on them", async () => { const fs = makeFakeFs({ ...baseStaging(), diff --git a/workflows/review/lib/dispatch-cluster.ts b/workflows/review/lib/dispatch-cluster.ts index 315d99ba..20d56ceb 100644 --- a/workflows/review/lib/dispatch-cluster.ts +++ b/workflows/review/lib/dispatch-cluster.ts @@ -17,9 +17,45 @@ import type {ClaimMerge, ClusterRejection} from "./dedup"; import {type Claim, type ProposedCluster} from "./dispatch-contracts"; +import {isBlockingLabel} from "./render-comment"; export const CLUSTERER = "claim-clusterer"; +/** + * Whether the candidate set holds a pair tier 2 could legally merge, which is + * the dispatch precondition: two ANCHORED claims on the SAME path from + * DIFFERENT sources, at least one of them non-blocking. Every conjunct is one + * of `dedup.ts`' own rules (a cluster member needs an anchor, cross-file + * merging is out of both tiers, a reviewer never duplicates itself, and only a + * non-blocking copy may be absorbed on a model's word; the survivor being the + * most severe copy, a blocking-only pair has nothing absorbable). + * + * With no such pair the clusterer cannot produce a merge whatever it proposes, + * so the dispatch is pure spend on a serial step. Computed from the candidates + * already in hand, so the check itself costs nothing. + * + * Exported because the live A/B's producer gates on the same precondition; a + * drift there would have an arm paying for (or skipping) a dispatch production + * would not. + */ +export const hasClusterableCandidatePair = ( + candidates: readonly Claim[], +): boolean => { + const anchored = candidates.filter( + (claim) => claim.path !== undefined && claim.line !== undefined, + ); + return anchored.some((a, index) => + anchored + .slice(index + 1) + .some( + (b) => + a.path === b.path && + a.source !== b.source && + (!isBlockingLabel(a.label) || !isBlockingLabel(b.label)), + ), + ); +}; + /** Dedup tier 2 telemetry (dedup.ts owns the rules; this is the audit). */ export type DispatchClustering = { /** Claims the clusterer was given: the pre-merge candidate count. */ @@ -58,9 +94,10 @@ export type ClusterStep = { * set the validator reads. * * Skipped, with no spend, unless there is something only this tier can find: - * two claims from two different sources. A single reviewer's findings are never - * merged into each other (dedup.ts' rule, both tiers), so a one-source run has - * no candidate pair at all. + * a pair {@link hasClusterableCandidatePair} would let merge. A one-source run + * has no candidate pair at all (dedup.ts never merges a reviewer's findings + * into each other, in either tier), and neither does a run whose only + * cross-source pairs sit in different files or are blocking on both sides. * * Failure is soft in both directions. A missing definition (an extraction * failure, since review.md and this lib ship at one pinned ref) or an unusable @@ -74,8 +111,7 @@ export const runClusterStep = async ( candidates: Claim[], io: ClusterStepIo, ): Promise => { - const sources = new Set(candidates.map((claim) => claim.source)); - if (candidates.length < 2 || sources.size < 2) { + if (!hasClusterableCandidatePair(candidates)) { return {proposals: [], dispatched: false, unavailable: false}; } io.write(JSON.stringify(candidates, null, 2)); diff --git a/workflows/review/review.md b/workflows/review/review.md index a36e0521..1cae299e 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -1527,9 +1527,13 @@ name: claim-clusterer description: Groups the candidate comments that describe ONE defect, so several reviewers flagging the same thing post once; returns JSON. model: claude-sonnet-4-6 # effort: medium — launch default (clustering). Sonnet, not Opus: this is a -# text-identity judgment over already-written claims, with no code -# investigation and no prose to author. It is the cheapest agent in the -# pipeline and it removes claims from the most expensive one (the validator). +# text-identity judgment over already-written claims, with no prose to author +# and no investigation to run: it reads the cited lines to see what two claims +# are pointing at, which is a lookup, not an analysis (the prompt caps it at +# that: "you are locating claims, not investigating them"). It is the cheapest +# agent in the pipeline and it removes claims from the most expensive one (the +# validator). Read-only file access is therefore part of the sizing, not an +# exception to it. --- You decide which candidate review comments describe the **same defect**, so that several reviewers who found one problem leave one comment instead of four. You judge From 163f8cfbdad4f0e1358819f2c4216ae74208cab5 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Fri, 31 Jul 2026 14:39:12 -0700 Subject: [PATCH 5/8] review: screen a cluster proposal's structure before it is unioned The blocking review finding: `verifiableClusters` screened only unknown-id / no-anchor / already-clustered, so a structurally invalid proposal still unioned its members into the group. A cross-path member pulled in that way can out-rank the real survivor, and the tier-1 merge underneath then collapses to nothing -- recorded in neither `merges` nor `clusterRejections`. Tier 2 could degrade BELOW tier 1, which is the one thing dispatch-cluster.ts promises it cannot. The path/source/severity rules now run per proposal before any union, on the anchor the proposal's own members elect (its blocking member if it has one, else the first the id screen kept -- only the severity rule is asymmetric, and 'advisory absorbed into blocking' must stay legal). What survives is a membership hint whose members are mutually absorbable; the per-member re-check against the ACTUAL survivor stays, because tier-1 bridging can still elect a claim the proposal never named. Two tests: the lost-merge shape itself (fails without the screen), and the survivor-time re-check that the screen does not subsume -- a member legal against the anchor but same-source as the survivor tier 1 elects. Also splits the modules the merge with main pushed over the 1000-line cap: tier 2 to lib/dedup-cluster.ts (the name its tests already carried) and the eval's dedup stage to eval/live-dedup.ts, mirroring dispatch-cluster.ts on each side. dispatch.ts sheds the empty section header main's dispatch-agents split left behind, and dedup.ts's orphaned `mergeable` doc comment is reattached to `mergeable`. Plus the review's other import ask: live-producer.ts takes the exported CLUSTERER constant rather than restating the agent name. --- workflows/review/eval/live-dedup.ts | 141 ++++++++++ workflows/review/eval/live-producer.ts | 150 +---------- workflows/review/lib/dedup-cluster.test.ts | 123 ++++++++- workflows/review/lib/dedup-cluster.ts | 288 +++++++++++++++++++++ workflows/review/lib/dedup.ts | 218 ++-------------- workflows/review/lib/dispatch-cluster.ts | 3 +- workflows/review/lib/dispatch.ts | 6 +- 7 files changed, 578 insertions(+), 351 deletions(-) create mode 100644 workflows/review/eval/live-dedup.ts create mode 100644 workflows/review/lib/dedup-cluster.ts diff --git a/workflows/review/eval/live-dedup.ts b/workflows/review/eval/live-dedup.ts new file mode 100644 index 00000000..10e72f4f --- /dev/null +++ b/workflows/review/eval/live-dedup.ts @@ -0,0 +1,141 @@ +/** + * The A/B's cross-source dedup stage: production's pre-validation merge + * (`dedupeClaims` plus, when the arm's `review.md` defines the clusterer, + * tier 2's dispatch), run over an arm's live findings. + * + * Split out of `live-producer.ts` for the same reason `dispatch-cluster.ts` is + * split out of `dispatch.ts` — the producer sits at the shared 1000-line cap, + * and the clustering step is the newest separable stage rather than the one + * with the most tangled callers. + */ + +import {dedupeClaims, type ClaimMerge} from "../lib/dedup"; +import type {ClusterRejection} from "../lib/dedup-cluster"; +import { + buildClaims as buildLibClaims, + parseClustererOutput, + type Candidate, + type ProposedCluster, +} from "../lib/dispatch-contracts"; +import {hasClusterableCandidatePair} from "../lib/dispatch-cluster"; +import type {ExtractedAgent} from "./agent-extract"; +// Type-only, so the producer/dedup pair carries no runtime import cycle. +import type {LiveFinding, PerAgentReport} from "./live-producer"; + +/** What an arm's dedup stage did, for the A/B report. */ +export type LiveDedupReport = { + /** Claims entering the merge (the pre-merge candidate count). */ + candidates: number; + /** Merged groups, as `dispatch-result.json` records them. */ + merges: ClaimMerge[]; + /** Well-formed clusters the clusterer proposed (0 when it did not run). */ + proposed: number; + /** Proposed members the merge rules rejected. */ + rejected: ClusterRejection[]; + /** The arm's review.md defines no clusterer: tier 1 only, by construction. */ + clustererAbsent: boolean; +}; + +/** + * Run production's cross-source merge over an arm's live findings. + * + * Why this exists at all: the A/B never ran dedup, so a change to it was + * unmeasurable by construction — the pipeline the eval measured posted every + * duplicate that production merges, and no report column moved when the merge + * rules changed. Both arms run tier 1 (it is shared code, and production has + * had it since #245); tier 2 is carried by the arm's OWN review.md, exactly + * like the provenance gate's anchor-snap emulation, so a baseline built from a + * ref that predates the `claim-clusterer` agent runs tier 1 alone and the arm + * delta prices the clusterer and nothing else. + * + * The claim projection is the LIB's `buildClaims`, not the producer's + * validator-contract one: the merge compares `subject` against + * `failure_scenario` (falling back to `discussion`), and the eval's own + * projection puts the whole prose in `subject` and the evidence trace in + * `discussion`. Feeding that shape to the floors would measure a similarity + * arithmetic production never runs. + * + * What is shared with production and what is not, since fidelity is the whole + * point: the merge rules (`dedupeClaims`) and the dispatch precondition + * (`hasClusterableCandidatePair`) are the SAME code `runClusterStep` runs. What + * this function re-implements is the plumbing that step cannot lend: the + * dispatch goes through the eval's own agent runner and per-agent cost report, + * and the survivor's merged prose is written back onto a `LiveFinding` rather + * than onto a staged claims.json. That is the seam to keep in step by hand + * (the provenance gate's anchor-snap emulation has the same shape); a rule + * change does not need mirroring here, a change to WHEN the step runs does. + */ +export const dedupeLiveFindings = async ( + findings: LiveFinding[], + clusterer: ExtractedAgent | undefined, + io: { + dispatch: ( + agent: ExtractedAgent, + parse: (output: string) => ProposedCluster[], + ) => Promise<{ + report: PerAgentReport; + parsed?: ProposedCluster[]; + }>; + write: (name: string, content: string) => void; + }, +): Promise<{ + kept: LiveFinding[]; + dedup: {report?: PerAgentReport; result: LiveDedupReport}; +}> => { + const claims = buildLibClaims(findings as Candidate[]); + const clusterable = hasClusterableCandidatePair(claims); + let proposals: ProposedCluster[] = []; + let report: PerAgentReport | undefined; + if (clusterable && clusterer !== undefined) { + io.write("candidates.json", JSON.stringify(claims, null, 2)); + const dispatched = await io.dispatch(clusterer, parseClustererOutput); + report = dispatched.report; + proposals = dispatched.parsed ?? []; + } + const merged = dedupeClaims(claims, proposals); + const dropped = new Set( + merged.merges.flatMap((merge) => merge.merged.map((m) => m.id)), + ); + const survivors = new Map( + merged.claims.map((claim) => [claim.id, claim] as const), + ); + const kept = findings + .filter((live) => !dropped.has(live.finding.id)) + .map((live) => { + const survivor = survivors.get(live.finding.id); + if ( + survivor === undefined || + !merged.merges.some( + (merge) => merge.survivor === live.finding.id, + ) + ) { + return live; + } + // The survivor's claim carries the "also flagged by" note (the lib + // projection puts the prose in `discussion`) and may have adopted a + // merged copy's suggestion; both must reach the rendered comment. + return { + ...live, + finding: { + ...live.finding, + model_authored_prose: survivor.discussion, + ...(survivor.suggestion !== undefined + ? {suggested_patch: survivor.suggestion} + : {}), + }, + }; + }); + return { + kept, + dedup: { + ...(report !== undefined ? {report} : {}), + result: { + candidates: claims.length, + merges: merged.merges, + proposed: proposals.length, + rejected: merged.clusterRejections, + clustererAbsent: clusterer === undefined, + }, + }, + }; +}; diff --git a/workflows/review/eval/live-producer.ts b/workflows/review/eval/live-producer.ts index f2dd1f3a..9fb8310d 100644 --- a/workflows/review/eval/live-producer.ts +++ b/workflows/review/eval/live-producer.ts @@ -32,7 +32,8 @@ * the load-bearing one: production has merged duplicate claims before validation * since #245, this module never did, so every duplicate production suppressed * still posted here and no report column could see a change to the merge rules. - * {@link dedupeLiveFindings} closes that, arm-keyed on the clusterer agent. + * `live-dedup.ts`'s `dedupeLiveFindings` closes that, arm-keyed on the + * clusterer agent. */ import {refusalFallbackFor} from "../lib/refusal-fallback"; @@ -47,18 +48,8 @@ import { import {isBlockingLabel, labelForFinding} from "../lib/render-comment"; import {route, type RouterConfig} from "../lib/router"; import {validateFinding, type Finding, type Lens} from "../lib/finding-schema"; -import { - dedupeClaims, - type ClaimMerge, - type ClusterRejection, -} from "../lib/dedup"; -import { - buildClaims as buildLibClaims, - parseClustererOutput, - type Candidate, - type ProposedCluster, -} from "../lib/dispatch-contracts"; -import {hasClusterableCandidatePair} from "../lib/dispatch-cluster"; +import {CLUSTERER} from "../lib/dispatch-cluster"; +import {dedupeLiveFindings, type LiveDedupReport} from "./live-dedup"; import { VERIFICATION_STATES, type CaseVerification, @@ -354,8 +345,11 @@ export const resolveRuntimeImports = ( /* Output parsing: the three sub-agent contracts -> RecordedFinding */ /* -------------------------------------------------------------------------- */ -/** A produced finding plus the claims-path extras the validator reads. */ -type LiveFinding = RecordedFinding & {skill?: string}; +/** + * A produced finding plus the claims-path extras the validator reads. Exported + * for `live-dedup.ts`, which rewrites a merge survivor's prose in this shape. + */ +export type LiveFinding = RecordedFinding & {skill?: string}; const isRecord = (value: unknown): value is Record => typeof value === "object" && value !== null && !Array.isArray(value); @@ -492,130 +486,6 @@ const parseAgentFindings = ( return findings; }; -/* -------------------------------------------------------------------------- */ -/* Cross-source dedup (production's pre-validation merge) */ -/* -------------------------------------------------------------------------- */ - -const CLUSTERER = "claim-clusterer"; - -/** What an arm's dedup stage did, for the A/B report. */ -export type LiveDedupReport = { - /** Claims entering the merge (the pre-merge candidate count). */ - candidates: number; - /** Merged groups, as `dispatch-result.json` records them. */ - merges: ClaimMerge[]; - /** Well-formed clusters the clusterer proposed (0 when it did not run). */ - proposed: number; - /** Proposed members the merge rules rejected. */ - rejected: ClusterRejection[]; - /** The arm's review.md defines no clusterer: tier 1 only, by construction. */ - clustererAbsent: boolean; -}; - -/** - * Run production's cross-source merge over an arm's live findings. - * - * Why this exists at all: the A/B never ran dedup, so a change to it was - * unmeasurable by construction — the pipeline the eval measured posted every - * duplicate that production merges, and no report column moved when the merge - * rules changed. Both arms run tier 1 (it is shared code, and production has - * had it since #245); tier 2 is carried by the arm's OWN review.md, exactly - * like the provenance gate's anchor-snap emulation, so a baseline built from a - * ref that predates the `claim-clusterer` agent runs tier 1 alone and the arm - * delta prices the clusterer and nothing else. - * - * The claim projection is the LIB's `buildClaims`, not this module's - * validator-contract one: the merge compares `subject` against - * `failure_scenario` (falling back to `discussion`), and the eval's own - * projection puts the whole prose in `subject` and the evidence trace in - * `discussion`. Feeding that shape to the floors would measure a similarity - * arithmetic production never runs. - * - * What is shared with production and what is not, since fidelity is the whole - * point: the merge rules (`dedupeClaims`) and the dispatch precondition - * (`hasClusterableCandidatePair`) are the SAME code `runClusterStep` runs. What - * this function re-implements is the plumbing that step cannot lend: the - * dispatch goes through the eval's own agent runner and per-agent cost report, - * and the survivor's merged prose is written back onto a `LiveFinding` rather - * than onto a staged claims.json. That is the seam to keep in step by hand - * (the provenance gate's anchor-snap emulation has the same shape); a rule - * change does not need mirroring here, a change to WHEN the step runs does. - */ -const dedupeLiveFindings = async ( - findings: LiveFinding[], - clusterer: ExtractedAgent | undefined, - io: { - dispatch: ( - agent: ExtractedAgent, - parse: (output: string) => ProposedCluster[], - ) => Promise<{ - report: PerAgentReport; - parsed?: ProposedCluster[]; - }>; - write: (name: string, content: string) => void; - }, -): Promise<{ - kept: LiveFinding[]; - dedup: {report?: PerAgentReport; result: LiveDedupReport}; -}> => { - const claims = buildLibClaims(findings as Candidate[]); - const clusterable = hasClusterableCandidatePair(claims); - let proposals: ProposedCluster[] = []; - let report: PerAgentReport | undefined; - if (clusterable && clusterer !== undefined) { - io.write("candidates.json", JSON.stringify(claims, null, 2)); - const dispatched = await io.dispatch(clusterer, parseClustererOutput); - report = dispatched.report; - proposals = dispatched.parsed ?? []; - } - const merged = dedupeClaims(claims, proposals); - const dropped = new Set( - merged.merges.flatMap((merge) => merge.merged.map((m) => m.id)), - ); - const survivors = new Map( - merged.claims.map((claim) => [claim.id, claim] as const), - ); - const kept = findings - .filter((live) => !dropped.has(live.finding.id)) - .map((live) => { - const survivor = survivors.get(live.finding.id); - if ( - survivor === undefined || - !merged.merges.some( - (merge) => merge.survivor === live.finding.id, - ) - ) { - return live; - } - // The survivor's claim carries the "also flagged by" note (the lib - // projection puts the prose in `discussion`) and may have adopted a - // merged copy's suggestion; both must reach the rendered comment. - return { - ...live, - finding: { - ...live.finding, - model_authored_prose: survivor.discussion, - ...(survivor.suggestion !== undefined - ? {suggested_patch: survivor.suggestion} - : {}), - }, - }; - }); - return { - kept, - dedup: { - ...(report !== undefined ? {report} : {}), - result: { - candidates: claims.length, - merges: merged.merges, - proposed: proposals.length, - rejected: merged.clusterRejections, - clustererAbsent: clusterer === undefined, - }, - }, - }; -}; - /* -------------------------------------------------------------------------- */ /* The claims path */ /* -------------------------------------------------------------------------- */ @@ -1087,3 +957,5 @@ export const produceLive = async ( /** Re-exported so the A/B runner types its recorded outputs without reaching * into internals. */ export type {Finding}; +/** Re-exported so the dedup stage keeps one import surface with the producer. */ +export type {LiveDedupReport}; diff --git a/workflows/review/lib/dedup-cluster.test.ts b/workflows/review/lib/dedup-cluster.test.ts index eefb83f4..097e50e5 100644 --- a/workflows/review/lib/dedup-cluster.test.ts +++ b/workflows/review/lib/dedup-cluster.test.ts @@ -320,8 +320,12 @@ describe("dedupeClaims with model-proposed clusters", () => { ); expect(claims).toHaveLength(2); expect(merges).toEqual([]); + // Rejected before the union, so the proposal collapses to one member + // and that member is recorded too: nothing about this proposal reaches + // the merge core, which is what keeps it from reshaping a tier-1 group. expect(clusterRejections).toEqual([ {id: "holistic-9", reason: "blocking-member"}, + {id: "correctness-reviewer-3", reason: "cluster-collapsed"}, ]); }); @@ -376,17 +380,17 @@ describe("dedupeClaims with model-proposed clusters", () => { expect(clusterRejections).toEqual([ {id: "skill-auditor-ool-2", reason: "other-path"}, {id: "correctness-4", reason: "same-source"}, + {id: "correctness-reviewer-3", reason: "cluster-collapsed"}, ]); }); it("holds a cluster member to those rules when its text clears the floor too", () => { // The test above proves the rules only for members tier 1's floor // cannot reach, which is every fixture the clusterer is built for. The - // dangerous member is the opposite one: a cluster unions on the model's - // word BEFORE any member is checked, so a member whose prose does clear - // the floor would take the similarity branch, merge without ever - // meeting the path or source rule, and be recorded as - // `via: "similarity"`: a reviewer's distinct finding dropped, with the + // dangerous member is the opposite one: a member whose prose DOES clear + // the floor would, if it ever reached the union, take the similarity + // branch and merge without meeting the path or source rule, recorded as + // `via: "similarity"` — a reviewer's distinct finding dropped, with the // artifact naming the wrong tier. These copies are verbatim identical, // so the floor is decidedly not what keeps them apart. const [note] = wrongCapClaims(); @@ -421,6 +425,7 @@ describe("dedupeClaims with model-proposed clusters", () => { expect(clusterRejections).toEqual([ {id: "documentation-9", reason: "other-path"}, {id: "correctness-reviewer-9", reason: "same-source"}, + {id: "correctness-reviewer-3", reason: "cluster-collapsed"}, ]); }); @@ -464,6 +469,114 @@ describe("dedupeClaims with model-proposed clusters", () => { ]); }); + it("never lets an unverifiable proposal cost a merge tier 1 would have made", () => { + // The floor under tier 2: it may add merges, never subtract them. + // Membership used to be unioned before any member was checked, so a + // proposal naming a cross-path claim pulled it into the group, where + // its higher severity won the survivor election — and the genuine + // tier-1 pair beneath it then collapsed to nothing, recorded in + // neither `merges` nor `clusterRejections`. Three comments posted where + // two would have, from a tier whose whole purpose is fewer. + const [note] = wrongCapClaims(); + const pair = (over: Partial & {id: string; source: string}) => + claim({...note, line: 8, ...over}); + const {claims, merges, clusterRejections} = dedupeClaims( + [ + // Cross-path, and blocking so it out-ranks both of the others. + pair({ + id: "holistic-1", + source: "holistic", + path: "dev/af19_trial/other.go", + label: "issue (blocking)", + }), + pair({id: "documentation-1", source: "documentation"}), + pair({id: "conventions-1", source: "conventions"}), + ], + [ + { + evidence: "`maxSamples`", + ids: ["holistic-1", "documentation-1"], + }, + ], + ); + // The tier-1 pair survives as a pair; only the cross-path claim posts + // separately, exactly as it would have with no clusterer at all. + expect(claims.map((c) => c.id)).toEqual([ + "holistic-1", + "documentation-1", + ]); + expect(merges).toEqual([ + { + survivor: "documentation-1", + merged: [ + { + id: "conventions-1", + source: "conventions", + label: "note (non-blocking)", + }, + ], + path: "dev/af19_trial/window.go", + line: 8, + via: "similarity", + }, + ]); + expect(clusterRejections).toEqual([ + {id: "documentation-1", reason: "other-path"}, + {id: "holistic-1", reason: "cluster-collapsed"}, + ]); + }); + + it("re-checks a screened member against the survivor tier 1 elects", () => { + // The pre-screen anchors on the proposal's own members; tier 1 can then + // bridge in a claim that out-ranks the anchor and becomes the survivor, + // which is why the per-member rules run a second time against the + // ACTUAL survivor. Here the cluster member and the elected survivor + // share a source — legal against the anchor, not against the survivor — + // and collapsing them would drop one of that reviewer's two findings. + const [note] = wrongCapClaims(); + const {claims, merges, clusterRejections} = dedupeClaims( + [ + // Bridged to `correctness-reviewer-3` by tier 1 (verbatim + // copy, different source), and blocking, so it survives. + claim({ + ...note, + id: "documentation-2", + source: "documentation", + label: "issue (blocking)", + }), + claim({...note, id: "correctness-reviewer-3"}), + // The clusterer's member: fine against the correctness anchor, + // same source as the survivor tier 1 actually elects. + claim({ + ...note, + id: "documentation-1", + source: "documentation", + subject: "the per-key cap disagrees with `maxSamples`", + discussion: "the per-key cap disagrees with `maxSamples`", + failure_scenario: "the cap disagrees with `maxSamples`", + }), + ], + [ + { + evidence: "`maxSamples`", + ids: ["correctness-reviewer-3", "documentation-1"], + }, + ], + ); + expect(merges).toHaveLength(1); + expect(merges[0].survivor).toBe("documentation-2"); + expect(merges[0].merged.map((m) => m.id)).toEqual([ + "correctness-reviewer-3", + ]); + expect(claims.map((c) => c.id)).toEqual([ + "documentation-2", + "documentation-1", + ]); + expect(clusterRejections).toEqual([ + {id: "documentation-1", reason: "same-source"}, + ]); + }); + it("records ids the clusterer invented, and holds each claim to one cluster", () => { // The webapp#41197 lesson applied to tier 2: an empty merge list must // never be the only evidence. A clusterer naming claims that do not diff --git a/workflows/review/lib/dedup-cluster.ts b/workflows/review/lib/dedup-cluster.ts new file mode 100644 index 00000000..4efb743e --- /dev/null +++ b/workflows/review/lib/dedup-cluster.ts @@ -0,0 +1,288 @@ +/** + * Dedup tier 2: the `claim-clusterer`'s proposed defect clusters, code-verified. + * + * The model contributes IDENTITY and nothing else — which candidate claims + * describe one defect, and the code element they share. Every merge rule lives + * here and in `dedup.ts`; nothing the clusterer says is taken on trust. See + * `dedup.ts`'s header for why a second tier exists at all (tier 1's text floors + * are an order of magnitude away from the real four-way duplicate of run + * 30587343777, and the discriminator is semantic, not arithmetic). + * + * Split from `dedup.ts` because that file sits at the shared 1000-line cap and + * this is its newest separable concern; the tests were already named for the + * split (`dedup-cluster.test.ts`), and `dispatch-cluster.ts` owns the dispatch + * and telemetry side of the same tier. + * + * Verification runs in TWO passes, and the split matters: + * + * 1. {@link verifiableClusters}, before any union: id resolution, anchors, and + * the STRUCTURAL rules (shared path, distinct sources, at most one blocking + * member). A proposal that reaches the union with members tier 2 could never + * absorb does not merely fail to merge — it can change which claim becomes + * the group's survivor and thereby suppress a merge tier 1 would have made + * on its own, which is the one thing this tier must never do. + * 2. {@link clusterMemberRejection}, per member against the group's ACTUAL + * survivor, which tier-1 bridging can change after the proposal was made. + * Re-checking there is what keeps the guarantee honest; the pre-screen + * narrows what can be unioned, it does not replace the final check. + */ + +import {type Claim, type ProposedCluster} from "./dispatch-contracts"; +import {isBlockingLabel} from "./render-comment"; + +/** + * One member a proposed cluster named that did NOT merge, with the rule that + * rejected it. Recorded per run because an empty rejection list and an empty + * proposal list mean opposite things, and the module has already been burned + * by that ambiguity once (see `dedup-threads.ts`'s `stagedThreadShapeFailure`): + * a clusterer naming ids that do not exist is a prompt or staging failure, and + * it must not read as "no duplicates found". + */ +export type ClusterRejection = { + id: string; + reason: + | "unknown-id" + | "no-anchor" + | "other-path" + | "same-source" + | "blocking-member" + | "ungrounded" + | "already-clustered" + | "cluster-collapsed"; +}; + +/** + * Whether a token names something in the code rather than in English: an + * interior case change (`maxSamples`, `AddDate`, `TrimTo`), an all-caps + * initialism (`TTL`), an underscore (`created_at`, `expiration_test`), or a + * multi-digit literal (`10`, `25`, `180`). + * + * This is the vocabulary tier 2's grounding check runs over, and it is + * deliberately narrow. A single-digit number is noise (`0` appears in half of + * all claims), and a bare lowercase word is English until proven otherwise — + * `cutoff` and `samples` would ground almost any two claims about the same + * file, which is precisely the confusion between "same code area" and "same + * defect" that this tier exists to avoid. A defect nameable only in such words + * is not model-mergeable; it falls back to tier 1, and a missed merge costs a + * duplicate comment while a wrong one drops a reviewer's distinct finding. + */ +const isSalientToken = (raw: string): boolean => + /[a-z][A-Z]/.test(raw) || + /^[A-Z]{2,}$/.test(raw) || + raw.includes("_") || + /^\d{2,}$/.test(raw); + +/** The code-naming tokens in a text, lowercased for comparison. */ +export const salientTokens = (text: string): Set => { + const tokens = new Set(); + for (const raw of text.match(/[A-Za-z_$][A-Za-z0-9_$]*|\d+/g) ?? []) { + if (isSalientToken(raw)) { + tokens.add(raw.toLowerCase()); + } + } + return tokens; +}; + +/** Everything a claim says, for the grounding check (evidence lives anywhere). */ +const claimText = (claim: Claim): string => + `${claim.subject} ${claim.discussion} ${claim.failure_scenario}`; + +export const sharesSalientToken = ( + evidenceTokens: ReadonlySet, + claim: Claim, +): boolean => { + const tokens = salientTokens(claimText(claim)); + for (const token of evidenceTokens) { + if (tokens.has(token)) { + return true; + } + } + return false; +}; + +/** + * The structural half of the tier-2 rules, checked against a reference claim: + * + * - **same path**, as in tier 1. Cross-file merging stays out of both tiers; + * its own calibration is a separate question and a missed merge is cheap. + * - **different source**, as in tier 1: a reviewer does not duplicate itself, + * and collapsing two of one reviewer's findings would silently drop one. + * - **non-blocking**: tier 2 may absorb an advisory copy into any survivor, + * but a BLOCKING claim only ever merges on tier 1's text floor. This is the + * risk grading, and the one place the tiers deliberately differ in power + * rather than in method. The model owns identity here, so a wrong grouping IS + * possible in a way no code check catches: "same facts, different ask" (run + * 30301235749's AddDate handoff and the missing-test todo it rode both name + * `AddDate`, so grounding cannot separate them; only the clusterer's + * judgment does). Capping what such an error can cost is therefore part of + * the design: since the survivor is always the highest-severity copy, a false + * tier-2 merge can lose an advisory comment and can never lose a blocking + * finding or soften the verdict. The price is real and accepted: one defect + * flagged blocking by two sources in different words still posts twice + * unless tier 1 reaches it. + * + * Deliberately absent: any line requirement, and any text-similarity floor. + * Both are what tier 2 exists to get past. + */ +const structuralRejection = ( + reference: Claim, + member: Claim, +): "other-path" | "same-source" | "blocking-member" | undefined => { + if (member.path !== reference.path) { + return "other-path"; + } + if (member.source === reference.source) { + return "same-source"; + } + if (isBlockingLabel(member.label)) { + return "blocking-member"; + } + return undefined; +}; + +/** + * The per-member rules a model-proposed merge must satisfy, checked against + * the group's ACTUAL survivor (which union with tier 1 can change after the + * proposal was made, so re-checking here rather than at parse time is what + * keeps the guarantee honest): the structural rules above, plus + * + * - **grounded**: the cluster's evidence must name at least one code element + * ({@link isSalientToken}) and the member must mention one of them. This is + * the hallucination tripwire — a group whose members share no named code + * with the identity the model asserted is not an identity claim this module + * can check, so it does not merge. + */ +export const clusterMemberRejection = ( + survivor: Claim, + member: Claim, + evidenceTokens: ReadonlySet, +): ClusterRejection["reason"] | undefined => + structuralRejection(survivor, member) ?? + (sharesSalientToken(evidenceTokens, member) ? undefined : "ungrounded"); + +/** + * Hold one proposal's members to the structural rules BEFORE they are unioned. + * + * This is not the survivor election — that happens after tier 1 has had its + * say, and {@link clusterMemberRejection} re-runs against whatever claim wins + * it. It is the narrower guarantee that an unverifiable proposal cannot RESHAPE + * a group tier 1 owns: unioning a cross-path or same-source member pulls it + * into the group, where it can out-rank the real survivor, and the merge tier 1 + * would have made on its own then collapses to nothing — invisible in both + * `merges` and `clusterRejections`. Concretely, with `Y+Z` a genuine tier-1 + * pair on one path and a cross-path `X` out-ranking them, an unscreened + * `{ids: ["x", "y"]}` used to drop the `Y+Z` merge entirely. + * + * The anchor is the proposal's own blocking member if it has one, else the + * first member the id screen kept (the model's output order, the tiebreak the + * proposal loop already uses). Only the severity rule is asymmetric — path + * equality and source inequality read the same whichever member is anchor — so + * this is exactly the "at most one blocking member, and it is the one that + * could survive" reading of the rule, and nothing more: a proposal pairing an + * advisory copy with a blocking one is legal and must stay so, since that is + * the merge tier 2 exists to make. + * + * Offending members are dropped, not the whole proposal: a proposal naming one + * bad member alongside a legal pair still merges the pair, and the >= 2 + * collapse rule takes care of what is left. + * + * The rules are checked against the anchor only, exactly as + * {@link clusterMemberRejection} checks them against the survivor — this is a + * filter on what may be unioned, not a stricter tier. Two copies from ONE + * source can therefore still ride into a cluster anchored on a third; that is + * the pre-existing shape of the source rule in both tiers, unchanged here. + */ +const structurallyVerified = ( + members: number[], + claims: Claim[], + rejections: ClusterRejection[], +): number[] => { + if (members.length === 0) { + return []; + } + const anchor = + members.find((index) => isBlockingLabel(claims[index].label)) ?? + members[0]; + const kept: number[] = []; + for (const index of members) { + if (index === anchor) { + kept.push(index); + continue; + } + const reason = structuralRejection(claims[anchor], claims[index]); + if (reason !== undefined) { + rejections.push({id: claims[index].id, reason}); + continue; + } + kept.push(index); + } + return kept; +}; + +/** + * Resolve the clusterer's proposals against the claim set: map ids to claims, + * hold each claim to at most one cluster (first proposal wins, in the model's + * own output order, so the result is deterministic), drop what cannot anchor a + * comment, and hold what is left to the structural rules + * ({@link structurallyVerified}). Every drop is recorded. + * + * What survives is a MEMBERSHIP hint whose members are all legally absorbable + * relative to one another; which of them actually collapses is decided later, + * per member, against the group's survivor ({@link clusterMemberRejection}). + */ +export const verifiableClusters = ( + claims: Claim[], + proposals: readonly ProposedCluster[], +): { + /** Claim index -> cluster ordinal. */ + clusterOf: Map; + /** Cluster ordinal -> the model's grounding evidence. */ + evidence: string[]; + rejections: ClusterRejection[]; +} => { + const indexById = new Map(); + claims.forEach((claim, index) => { + if (!indexById.has(claim.id)) { + indexById.set(claim.id, index); + } + }); + const clusterOf = new Map(); + const evidence: string[] = []; + const rejections: ClusterRejection[] = []; + for (const proposal of proposals) { + const named: number[] = []; + for (const id of proposal.ids) { + const index = indexById.get(id); + if (index === undefined) { + rejections.push({id, reason: "unknown-id"}); + continue; + } + if (clusterOf.has(index)) { + rejections.push({id, reason: "already-clustered"}); + continue; + } + const claim = claims[index]; + if (claim.path === undefined || claim.line === undefined) { + rejections.push({id, reason: "no-anchor"}); + continue; + } + named.push(index); + } + const members = structurallyVerified(named, claims, rejections); + if (members.length < 2) { + for (const index of members) { + rejections.push({ + id: claims[index].id, + reason: "cluster-collapsed", + }); + } + continue; + } + const ordinal = evidence.length; + evidence.push(proposal.evidence); + for (const index of members) { + clusterOf.set(index, ordinal); + } + } + return {clusterOf, evidence, rejections}; +}; diff --git a/workflows/review/lib/dedup.ts b/workflows/review/lib/dedup.ts index b8e59111..1c51f767 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -71,6 +71,13 @@ import {isRecord, type Claim, type ProposedCluster} from "./dispatch-contracts"; import {isBlockingLabel} from "./render-comment"; +import { + clusterMemberRejection, + salientTokens, + sharesSalientToken, + verifiableClusters, + type ClusterRejection, +} from "./dedup-cluster"; // The identity of the review bot, shared with the producer that stages the // threads this module filters (stage-pr.ts). `threads.ts` owns a GitHub fetch // but runs nothing at import time and reaches the network only through an @@ -104,27 +111,6 @@ export type ClaimMerge = { evidence?: string; }; -/** - * One member a proposed cluster named that did NOT merge, with the rule that - * rejected it. Recorded per run because an empty rejection list and an empty - * proposal list mean opposite things, and the module has already been burned - * by that ambiguity once (see {@link stagedThreadShapeFailure}): a clusterer - * naming ids that do not exist is a prompt or staging failure, and it must not - * read as "no duplicates found". - */ -export type ClusterRejection = { - id: string; - reason: - | "unknown-id" - | "no-anchor" - | "other-path" - | "same-source" - | "blocking-member" - | "ungrounded" - | "already-clustered" - | "cluster-collapsed"; -}; - /** * Similarity floors, in two tiers. An identical `(path, line)` from two * sources is itself evidence of one defect, so that tier's token floors sit @@ -237,186 +223,6 @@ export const describesSameDefect = (a: Claim, b: Claim): boolean => { ); }; -/* -------------------------------------------------------------------------- */ -/* Tier 2: model-proposed defect clusters, code-verified */ -/* -------------------------------------------------------------------------- */ - -/** - * Whether a token names something in the code rather than in English: an - * interior case change (`maxSamples`, `AddDate`, `TrimTo`), an all-caps - * initialism (`TTL`), an underscore (`created_at`, `expiration_test`), or a - * multi-digit literal (`10`, `25`, `180`). - * - * This is the vocabulary tier 2's grounding check runs over, and it is - * deliberately narrow. A single-digit number is noise (`0` appears in half of - * all claims), and a bare lowercase word is English until proven otherwise — - * `cutoff` and `samples` would ground almost any two claims about the same - * file, which is precisely the confusion between "same code area" and "same - * defect" that this tier exists to avoid. A defect nameable only in such words - * is not model-mergeable; it falls back to tier 1, and a missed merge costs a - * duplicate comment while a wrong one drops a reviewer's distinct finding. - */ -const isSalientToken = (raw: string): boolean => - /[a-z][A-Z]/.test(raw) || - /^[A-Z]{2,}$/.test(raw) || - raw.includes("_") || - /^\d{2,}$/.test(raw); - -/** The code-naming tokens in a text, lowercased for comparison. */ -const salientTokens = (text: string): Set => { - const tokens = new Set(); - for (const raw of text.match(/[A-Za-z_$][A-Za-z0-9_$]*|\d+/g) ?? []) { - if (isSalientToken(raw)) { - tokens.add(raw.toLowerCase()); - } - } - return tokens; -}; - -/** Everything a claim says, for the grounding check (evidence lives anywhere). */ -const claimText = (claim: Claim): string => - `${claim.subject} ${claim.discussion} ${claim.failure_scenario}`; - -const sharesSalientToken = ( - evidenceTokens: ReadonlySet, - claim: Claim, -): boolean => { - const tokens = salientTokens(claimText(claim)); - for (const token of evidenceTokens) { - if (tokens.has(token)) { - return true; - } - } - return false; -}; - -/** - * The per-member rules a model-proposed merge must satisfy, checked against - * the group's ACTUAL survivor (which union with tier 1 can change after the - * proposal was made, so re-checking here rather than at parse time is what - * keeps the guarantee honest): - * - * - **same path**, as in tier 1. Cross-file merging stays out of both tiers; - * its own calibration is a separate question and a missed merge is cheap. - * - **different source**, as in tier 1: a reviewer does not duplicate itself, - * and collapsing two of one reviewer's findings would silently drop one. - * - **grounded**: the cluster's evidence must name at least one code element - * ({@link isSalientToken}) and the member must mention one of them. This is - * the hallucination tripwire — a group whose members share no named code - * with the identity the model asserted is not an identity claim this module - * can check, so it does not merge. - * - **non-blocking**: tier 2 may absorb an advisory copy into any survivor, - * but a BLOCKING claim only ever merges on tier 1's text floor. This is the - * risk grading, and the one place the tiers deliberately differ in power - * rather than in method. The model owns identity here, so a wrong grouping IS - * possible in a way no code check catches: "same facts, different ask" (run - * 30301235749's AddDate handoff and the missing-test todo it rode both name - * `AddDate`, so grounding cannot separate them; only the clusterer's - * judgment does). Capping what such an error can cost is therefore part of - * the design: since the survivor is always the highest-severity copy, a false - * tier-2 merge can lose an advisory comment and can never lose a blocking - * finding or soften the verdict. The price is real and accepted: one defect - * flagged blocking by two sources in different words still posts twice - * unless tier 1 reaches it. - * - * Deliberately absent: any line requirement, and any text-similarity floor. - * Both are what tier 2 exists to get past. - */ -const clusterMemberRejection = ( - survivor: Claim, - member: Claim, - evidenceTokens: ReadonlySet, -): ClusterRejection["reason"] | undefined => { - if (member.path !== survivor.path) { - return "other-path"; - } - if (member.source === survivor.source) { - return "same-source"; - } - if (isBlockingLabel(member.label)) { - return "blocking-member"; - } - return sharesSalientToken(evidenceTokens, member) - ? undefined - : "ungrounded"; -}; - -/** - * Resolve the clusterer's proposals against the claim set: map ids to claims, - * hold each claim to at most one cluster (first proposal wins, in the model's - * own output order, so the result is deterministic), and drop what cannot - * anchor a comment. Every drop is recorded. - * - * A cluster is kept here only as a MEMBERSHIP hint; the merge rules that - * decide what actually collapses run later against the group's survivor - * ({@link clusterMemberRejection}). - */ -const verifiableClusters = ( - claims: Claim[], - proposals: readonly ProposedCluster[], -): { - /** Claim index -> cluster ordinal. */ - clusterOf: Map; - /** Cluster ordinal -> the model's grounding evidence. */ - evidence: string[]; - rejections: ClusterRejection[]; -} => { - const indexById = new Map(); - claims.forEach((claim, index) => { - if (!indexById.has(claim.id)) { - indexById.set(claim.id, index); - } - }); - const clusterOf = new Map(); - const evidence: string[] = []; - const rejections: ClusterRejection[] = []; - for (const proposal of proposals) { - const members: number[] = []; - for (const id of proposal.ids) { - const index = indexById.get(id); - if (index === undefined) { - rejections.push({id, reason: "unknown-id"}); - continue; - } - if (clusterOf.has(index)) { - rejections.push({id, reason: "already-clustered"}); - continue; - } - const claim = claims[index]; - if (claim.path === undefined || claim.line === undefined) { - rejections.push({id, reason: "no-anchor"}); - continue; - } - members.push(index); - } - if (members.length < 2) { - for (const index of members) { - rejections.push({ - id: claims[index].id, - reason: "cluster-collapsed", - }); - } - continue; - } - const ordinal = evidence.length; - evidence.push(proposal.evidence); - for (const index of members) { - clusterOf.set(index, ordinal); - } - } - return {clusterOf, evidence, rejections}; -}; - -/** - * Same path, any line distance: run 29943085279 posted the - * missing-deletion-test defect at expiration_test.go:15 and :58 (43 lines - * apart), and the old two-line window kept both copies separate; the - * similarity floors carry the precision, tiered on whether the two anchors - * are identical. Cross-FILE merging stays out: that same run flagged the - * defect in expiration.go too (:62, :38) and a floor loose enough to catch - * a cross-file pair needs its own strictly higher calibration; a missed - * merge only costs a duplicate comment. - */ /* -------------------------------------------------------------------------- */ /* Open-thread suppression (trial suggestion g) */ /* -------------------------------------------------------------------------- */ @@ -787,6 +593,16 @@ export const suppressOpenThreadDuplicates = ( return {kept, suppressed}; }; +/** + * Same path, any line distance: run 29943085279 posted the + * missing-deletion-test defect at expiration_test.go:15 and :58 (43 lines + * apart), and the old two-line window kept both copies separate; the + * similarity floors carry the precision, tiered on whether the two anchors + * are identical. Cross-FILE merging stays out: that same run flagged the + * defect in expiration.go too (:62, :38) and a floor loose enough to catch + * a cross-file pair needs its own strictly higher calibration; a missed + * merge only costs a duplicate comment. + */ const mergeable = (a: Claim, b: Claim): boolean => a.source !== b.source && a.path !== undefined && diff --git a/workflows/review/lib/dispatch-cluster.ts b/workflows/review/lib/dispatch-cluster.ts index 20d56ceb..0b3da781 100644 --- a/workflows/review/lib/dispatch-cluster.ts +++ b/workflows/review/lib/dispatch-cluster.ts @@ -15,7 +15,8 @@ * and the record are pure code. No prose about the code under review. */ -import type {ClaimMerge, ClusterRejection} from "./dedup"; +import type {ClaimMerge} from "./dedup"; +import type {ClusterRejection} from "./dedup-cluster"; import {type Claim, type ProposedCluster} from "./dispatch-contracts"; import {isBlockingLabel} from "./render-comment"; diff --git a/workflows/review/lib/dispatch.ts b/workflows/review/lib/dispatch.ts index 7a9823aa..0d70494b 100644 --- a/workflows/review/lib/dispatch.ts +++ b/workflows/review/lib/dispatch.ts @@ -118,9 +118,9 @@ export { dedupeClaims, suppressOpenThreadDuplicates, type ClaimMerge, - type ClusterRejection, type ThreadSuppression, } from "./dedup"; +export {type ClusterRejection} from "./dedup-cluster"; /* -------------------------------------------------------------------------- */ /* Seams */ @@ -217,10 +217,6 @@ const TRIAGE = "pattern-triage"; const RECONCILER = "thread-reconciler"; const VALIDATOR = "claim-validator"; -/* -------------------------------------------------------------------------- */ -/* Agent definitions (.claude/agents/.md) */ -/* -------------------------------------------------------------------------- */ - /* -------------------------------------------------------------------------- */ /* The dispatch run */ /* -------------------------------------------------------------------------- */ From 68bf9221b5d873451f1bd3df3923c6710254ac77 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Fri, 31 Jul 2026 14:45:16 -0700 Subject: [PATCH 6/8] review: answer the review's remaining asks on tier 2's accounting - Production telemetry now records `clusterMerged` (absorbed copies) beside `clusterMerges` (groups). The eval counts per copy and production counted per group, so any run with a `both` group had the two artifacts reporting different numbers for the same quantity -- readable as production contradicting the evidence that graduated the tier. - The A/B's merge row carries tier 2's dollars and wall-clock beside its share. The dispatch precondition is met by most multi-finding reviews, so the steady state is a serial Sonnet call on nearly every run; a merge count with no price beside it cannot answer whether that earns its place. Read off the clusterer's own per-agent entry, so a skipped or absent step is zero. - A test isolating the survivor half of the grounding check: every other tier-2 fixture has a survivor that names the evidence by construction, so the conjunct could regress green. The gap is tier 1 electing a survivor the clusterer never proposed; the new case bridges a blocking claim in on the text floor and asserts the cluster-only member stays its own comment. - dedup.ts records why a finder-emitted identity key was rejected, since it clusters deterministically at zero dispatch and will be re-proposed otherwise: a finder mints its key blind to the other reviewers, and where a blind key does agree is the AddDate pair that must NOT merge. --- .changeset/review-defect-clustering.md | 27 +++- workflows/review/eval/README.md | 9 +- workflows/review/eval/live-ab-report.ts | 26 +++- workflows/review/eval/live-ab.test.ts | 129 +++++++++++------- workflows/review/eval/live-ab.ts | 19 +++ workflows/review/lib/dedup-cluster.test.ts | 67 +++++++++ workflows/review/lib/dedup.ts | 15 ++ workflows/review/lib/dispatch-cluster.test.ts | 3 + workflows/review/lib/dispatch-cluster.ts | 17 +++ 9 files changed, 254 insertions(+), 58 deletions(-) diff --git a/.changeset/review-defect-clustering.md b/.changeset/review-defect-clustering.md index ced9eadd..342315f9 100644 --- a/.changeset/review-defect-clustering.md +++ b/.changeset/review-defect-clustering.md @@ -25,6 +25,15 @@ tokens than the real duplicates do (run 29943085279's AddDate issue and its MemoryTTLDays, 180, 15). Duplicates are "same ask, different words"; those are "same facts, different ask", which is a semantic judgment. +Asking each finder for its own identity key would cluster deterministically at +zero dispatch, and it fails for the same reason. A finder mints its key blind to +the other reviewers, so two of them agreeing on one defect would have to +independently pick the same string; and where a blind key DOES agree is the case +that must not merge, since the AddDate bug and its missing-test todo would both +key on `AddDate`. The clusterer's grounding evidence is not that key: it is +chosen after reading the candidate set, which is what makes it both possible and +checkable against every member's text. + So the unit of identity is now the defect, not the anchor. Tier 2 requires no line agreement at all, which is what makes the same-defect-different-anchor shape mergeable for the first time (one missing-test defect drew comments at three @@ -48,7 +57,8 @@ from two sources, at least one non-blocking), so a run with nothing to find neve pays for the step. A missing definition or an unusable reply leaves the run on tier 1, exactly today's behavior, and surfaces as a run warning plus a `clustering` block in `dispatch-result.json` (`candidates`, `proposed`, -`clusterMerges`, and every rejected member with the rule that stopped it) rather +`clusterMerges` per group, `clusterMerged` per absorbed copy, and every rejected +member with the rule that stopped it) rather than as an author-facing note: duplicate hygiene is not a review dimension. Each merge in `merges` now carries `via` (`similarity`/`clusterer`/`both`) plus the tier and anchor of each absorbed copy, so the merge rate reads off the artifact @@ -70,9 +80,18 @@ code and production has had it since #245) while tier 2 is carried by each arm's own review.md, exactly like the provenance gate's anchor-snap emulation, so the arm delta prices the clusterer alone and a false merge shows up as recall loss. The report gains a "Cross-source claims merged (of candidates)" row with tier 2's -share, and per-case dedup counts. +share, its dollars and wall-clock, and per-case dedup counts. + +Tier 2 can add merges and never subtract them, which the structural rules now +enforce before any membership is unioned rather than only afterward: an +unverifiable proposal that names a cross-path claim used to pull it into the +group, where it could out-rank the real survivor and collapse a merge tier 1 +would have made on its own, recorded in neither `merges` nor the rejection list. +The per-member re-check against the elected survivor stays, since tier-1 bridging +can still elect a claim the proposal never named. `dispatch.ts` was at its 1000-line cap again, so the clustering step lands in `dispatch-cluster.ts` (dispatch, contract parse, telemetry) rather than raising -the cap; the tier-2 tests live in `dedup-cluster.test.ts` and -`dispatch-cluster.test.ts` for the same reason. +the cap, and tier 2's rules in `lib/dedup-cluster.ts` beside the tests that +already carried the name; the eval's dedup stage splits to +`eval/live-dedup.ts` on the same principle. diff --git a/workflows/review/eval/README.md b/workflows/review/eval/README.md index 99811a8c..2727f9f0 100644 --- a/workflows/review/eval/README.md +++ b/workflows/review/eval/README.md @@ -166,7 +166,14 @@ claiming a band. finding, so it shows up as candidate-arm recall loss, not as a better duplicate number. The `by clusterer` share counts absorbed COPIES, not groups, so a group both tiers contributed to credits tier 2 only with what it actually - brought. `rejected` counts cluster MEMBERS the merge rules refused, so one bad + brought; production's `clustering` block records the same per-copy number as + `clusterMerged` (its `clusterMerges` counts groups), so the artifact and the + report that graduated the tier cannot be read as disagreeing. The share + carries tier 2's own dollars and wall-clock beside it, because the dispatch + precondition is satisfied by most multi-finding reviews: the steady state is a + serial Sonnet call on nearly every run, and a merge count is a graduation + argument only next to what those merges cost. `rejected` counts cluster + MEMBERS the merge rules refused, so one bad proposal naming three ids counts three (`unknown-id` there means the clusterer named claims that do not exist, which is a prompt or staging failure rather than a quiet zero). diff --git a/workflows/review/eval/live-ab-report.ts b/workflows/review/eval/live-ab-report.ts index a80d22fd..59b9510a 100644 --- a/workflows/review/eval/live-ab-report.ts +++ b/workflows/review/eval/live-ab-report.ts @@ -78,6 +78,15 @@ export type ArmRunReport = { clusterMerged: number; rejected: number; clustererAbsent: boolean; + /** + * The clusterer's own spend and wall-clock on this case, absent + * when it never ran. Tier 2 is a serial dispatch on nearly every + * multi-finding review and absorbs a fraction of a group per run, + * so its merge count alone cannot answer whether it earns its + * place; these price the count. + */ + clustererUsd?: number; + clustererWallMs?: number; /** * The merged groups themselves, so a suspicious merge is * diagnosable from the artifact instead of from a repeat run: the @@ -308,6 +317,13 @@ const snappedTotal = (arm: ArmRunReport): number => * marks an arm whose review.md defines no `claim-clusterer` — the expected * shape of the baseline in the A/B that graduates it, and the reason a zero in * the clusterer column there is asymmetry, not a negative result. + * + * Tier 2's share carries its PRICE beside it, because the two numbers are only + * meaningful together: the dispatch precondition is satisfied by most + * multi-finding reviews, so the steady state is a serial Sonnet call on nearly + * every run, and "4 merges" is a graduation argument only next to what those + * four merges cost. Tier 1 is free by comparison (pure text arithmetic), so no + * price is shown for it. */ const mergedTotal = (arm: ArmRunReport): string => { const dedup = arm.perCase.flatMap((c) => (c.dedup ? [c.dedup] : [])); @@ -317,8 +333,16 @@ const mergedTotal = (arm: ArmRunReport): string => { const sum = (pick: (d: typeof dedup[number]) => number): number => dedup.reduce((total, d) => total + pick(d), 0); const absent = dedup.every((d) => d.clustererAbsent); + const clustererUsd = sum((d) => d.clustererUsd ?? 0); + const clustererWallMs = sum((d) => d.clustererWallMs ?? 0); const notes = [ - absent ? "tier 1 only" : `${sum((d) => d.clusterMerged)} by clusterer`, + absent + ? "tier 1 only" + : `${sum( + (d) => d.clusterMerged, + )} by clusterer at $${clustererUsd.toFixed(2)} / ${Math.round( + clustererWallMs / 1000, + )}s`, ...(sum((d) => d.rejected) > 0 ? [`${sum((d) => d.rejected)} proposed member(s) rejected`] : []), diff --git a/workflows/review/eval/live-ab.test.ts b/workflows/review/eval/live-ab.test.ts index 783ab206..8d740612 100644 --- a/workflows/review/eval/live-ab.test.ts +++ b/workflows/review/eval/live-ab.test.ts @@ -182,57 +182,77 @@ describe("runArm dedup accounting", () => { */ const produceMerged = (clustererAbsent: boolean): ArmProduce => - async () => ({ - ...(await produceHit(1)(liveCase("case-1"))), - dedup: { - candidates: 4, - merges: [ - { - survivor: "live-hit", - merged: [ - { - id: "live-doc-1", - source: "documentation", - label: "suggestion (non-blocking, documentation)", - line: 9, - via: "clusterer" as const, - }, - ], - path: "src/a.ts", - line: 1, - via: "clusterer" as const, - evidence: "the `maxSamples` comment says 10, not 25", - }, - // A group BOTH tiers contributed to: tier 1 reached the - // conventions copy on its own and only the holistic one - // needed the clusterer, so exactly one of these two copies - // is tier 2's to claim. - { - survivor: "live-hit-2", - merged: [ - { - id: "live-conv-1", - source: "conventions", - label: "nitpick (non-blocking)", - }, - { - id: "live-holistic-1", - source: "holistic", - label: "note (non-blocking)", - via: "clusterer" as const, - }, - ], - path: "src/a.ts", - line: 4, - via: "both" as const, - evidence: "the `staleAfter` window", - }, - ], - proposed: 1, - rejected: [], - clustererAbsent, - }, - }); + async () => { + const base = await produceHit(1)(liveCase("case-1")); + return { + ...base, + // The clusterer's own per-agent entry is where the merge row reads + // tier 2's price: an arm that never had the agent contributes no + // entry and therefore no cost. + perAgent: clustererAbsent + ? base.perAgent + : [ + ...base.perAgent, + { + name: "claim-clusterer", + model: "sonnet", + usd: 0.25, + turns: 2, + wallMs: 21_000, + retried: false, + }, + ], + dedup: { + candidates: 4, + merges: [ + { + survivor: "live-hit", + merged: [ + { + id: "live-doc-1", + source: "documentation", + label: "suggestion (non-blocking, documentation)", + line: 9, + via: "clusterer" as const, + }, + ], + path: "src/a.ts", + line: 1, + via: "clusterer" as const, + evidence: + "the `maxSamples` comment says 10, not 25", + }, + // A group BOTH tiers contributed to: tier 1 reached the + // conventions copy on its own and only the holistic one + // needed the clusterer, so exactly one of these two copies + // is tier 2's to claim. + { + survivor: "live-hit-2", + merged: [ + { + id: "live-conv-1", + source: "conventions", + label: "nitpick (non-blocking)", + }, + { + id: "live-holistic-1", + source: "holistic", + label: "note (non-blocking)", + via: "clusterer" as const, + }, + ], + path: "src/a.ts", + line: 4, + via: "both" as const, + evidence: "the `staleAfter` window", + }, + ], + proposed: 1, + rejected: [], + clustererAbsent, + }, + }; + }; it("carries the per-case counts and the merged groups", async () => { const report = await runArm( @@ -251,6 +271,8 @@ describe("runArm dedup accounting", () => { clusterMerged: 2, rejected: 0, clustererAbsent: false, + clustererUsd: 0.25, + clustererWallMs: 21_000, groups: [ { survivor: "live-hit", @@ -298,7 +320,10 @@ describe("runArm dedup accounting", () => { // `tier 1 only` is the expected baseline shape in the A/B that // graduates the clusterer: a zero there is asymmetry, not a result. expect(markdown).toContain("3 / 4 (tier 1 only)"); - expect(markdown).toContain("3 / 4 (2 by clusterer)"); + // Tier 2's share carries its price: a merge count with no cost beside + // it cannot answer whether a serial dispatch on nearly every run is + // worth its place, which is the question graduation turns on. + expect(markdown).toContain("3 / 4 (2 by clusterer at $0.25 / 21s)"); }); it("omits the block for a producer that runs no dedup at all", async () => { diff --git a/workflows/review/eval/live-ab.ts b/workflows/review/eval/live-ab.ts index e658416f..9c7381ce 100644 --- a/workflows/review/eval/live-ab.ts +++ b/workflows/review/eval/live-ab.ts @@ -94,6 +94,7 @@ import { } from "./rereview-match"; import {runCase} from "./runner"; import {reviewMdHasAnchorSnap} from "../lib/provenance"; +import {CLUSTERER} from "../lib/dispatch-cluster"; import type {ReReviewMode} from "../lib/routing-config"; // The report shapes and renderers live in ./live-ab-report; re-exported so @@ -228,6 +229,9 @@ export const runArm = async ( }); } + const clusterer = produced.perAgent.find( + (agent) => agent.name === CLUSTERER, + ); perCase.push({ caseId: corpusCase.id, usd: caseUsd, @@ -260,6 +264,21 @@ export const runArm = async ( ), rejected: produced.dedup.rejected.length, clustererAbsent: produced.dedup.clustererAbsent, + // What tier 2 COST, beside what it merged. The + // clusterer is a serial step on nearly every + // multi-finding run while absorbing a fraction of a + // group per run, so a merge count alone cannot say + // whether it is worth dispatching; these two make the + // graduation decision a price per merge rather than a + // count. Read off the clusterer's own per-agent + // entry, so a run where it was skipped or absent + // contributes zero rather than nothing. + ...(clusterer === undefined + ? {} + : { + clustererUsd: clusterer.usd, + clustererWallMs: clusterer.wallMs, + }), // The groups themselves, not just the count: the // powered run that graduated tier 2 could see THAT 4 // claims merged but not WHICH, so auditing a diff --git a/workflows/review/lib/dedup-cluster.test.ts b/workflows/review/lib/dedup-cluster.test.ts index 097e50e5..072b4544 100644 --- a/workflows/review/lib/dedup-cluster.test.ts +++ b/workflows/review/lib/dedup-cluster.test.ts @@ -526,6 +526,73 @@ describe("dedupeClaims with model-proposed clusters", () => { ]); }); + it("refuses a cluster whose SURVIVOR does not name the shared evidence", () => { + // The grounding check runs against both ends, and the survivor end has + // no other fixture: everywhere else the survivor is a cluster member + // and names the evidence by construction. The gap it closes is tier 1 + // electing a survivor the clusterer never proposed — here a blocking + // claim bridged in on the text floor, describing the retention window + // rather than the cap. Ungrounded against THAT survivor, the identity + // the model asserted says nothing about the comment the merge would + // post under, so the cluster-only member stays its own comment. + const {claims, merges, clusterRejections} = dedupeClaims( + [ + claim({ + id: "holistic-1", + source: "holistic", + path: "dev/af19_trial/window.go", + line: 8, + label: "issue (blocking)", + subject: + "The declaration comment above the constant states a retention bound the code does not enforce.", + failure_scenario: + "A maintainer sizes downstream buffers from the stated retention bound and under-provisions.", + }), + claim({ + id: "correctness-reviewer-3", + source: "correctness-reviewer", + path: "dev/af19_trial/window.go", + line: 8, + label: "note (non-blocking)", + subject: + "The declaration comment above `maxSamples` states a retention bound the code does not enforce.", + failure_scenario: + "A maintainer sizes downstream buffers from the stated retention bound and under-provisions.", + }), + claim({ + id: "documentation-1", + source: "documentation", + path: "dev/af19_trial/window.go", + line: 10, + label: "suggestion (non-blocking, documentation)", + subject: "Wrong cap in prose: 10 vs 25.", + failure_scenario: + "`maxSamples` is 25 and the doc says 10, so a reader trusts a number that was never true.", + }), + ], + [ + { + evidence: "the `maxSamples` cap", + ids: ["correctness-reviewer-3", "documentation-1"], + }, + ], + ); + // Tier 1's merge stands; tier 2 contributes nothing to it. + expect(merges).toHaveLength(1); + expect(merges[0].survivor).toBe("holistic-1"); + expect(merges[0].via).toBe("similarity"); + expect(merges[0].merged.map((m) => m.id)).toEqual([ + "correctness-reviewer-3", + ]); + expect(claims.map((c) => c.id)).toEqual([ + "holistic-1", + "documentation-1", + ]); + expect(clusterRejections).toEqual([ + {id: "documentation-1", reason: "ungrounded"}, + ]); + }); + it("re-checks a screened member against the survivor tier 1 elects", () => { // The pre-screen anchors on the proposal's own members; tier 1 can then // bridge in a claim that out-ranks the anchor and becomes the survivor, diff --git a/workflows/review/lib/dedup.ts b/workflows/review/lib/dedup.ts index 1c51f767..79813368 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -44,6 +44,21 @@ * missing test). Telling those apart is a semantic judgment, so tier 2 asks a * model for it rather than pretending a fourth threshold would find it. * + * Nor by asking each finder for an identity key of its own, which would + * cluster deterministically at zero dispatch and was the first thing tried on + * paper. A finder mints its key blind: it has not seen the other reviewers' + * findings, so two of them agreeing on one defect would have to independently + * choose the SAME string, which is the semantic-agreement problem the text + * floors already lose, moved into a shorter string with less to work with + * (`maxSamples`, "the doc/constant mismatch" and "window.go:8 comment" are all + * defensible keys for run 30587343777's one defect). Where a blind key DOES + * agree is the case that must not merge: the AddDate pair above would both key + * on `AddDate`, so the scheme collides hardest exactly where "same facts, + * different ask" needs separating. The clusterer's `evidence` is not that key + * and cannot be produced by a finder, because it is chosen AFTER reading the + * candidate set; comparison is what makes it possible, which is why it can be + * verified against every member's text here rather than merely trusted. + * * The unit of identity, restated: the DEFECT, not the anchor. Tier 2 needs no * line agreement at all, which is what finally makes the * same-defect-different-anchor shape mergeable (one missing-test defect drew diff --git a/workflows/review/lib/dispatch-cluster.test.ts b/workflows/review/lib/dispatch-cluster.test.ts index 672f2f83..c7bc69e5 100644 --- a/workflows/review/lib/dispatch-cluster.test.ts +++ b/workflows/review/lib/dispatch-cluster.test.ts @@ -235,6 +235,7 @@ describe("runDispatch defect clustering (dedup tier 2)", () => { candidates: 2, proposed: 1, clusterMerges: 1, + clusterMerged: 1, rejected: [], }); expect( @@ -271,6 +272,7 @@ describe("runDispatch defect clustering (dedup tier 2)", () => { candidates: 2, proposed: 0, clusterMerges: 0, + clusterMerged: 0, rejected: [], unavailable: true, }); @@ -391,6 +393,7 @@ describe("runDispatch defect clustering (dedup tier 2)", () => { candidates: 2, proposed: 1, clusterMerges: 0, + clusterMerged: 0, rejected: [ {id: "holistic-4", reason: "unknown-id"}, {id: "correctness-reviewer-1", reason: "cluster-collapsed"}, diff --git a/workflows/review/lib/dispatch-cluster.ts b/workflows/review/lib/dispatch-cluster.ts index 0b3da781..dc2aefc8 100644 --- a/workflows/review/lib/dispatch-cluster.ts +++ b/workflows/review/lib/dispatch-cluster.ts @@ -65,6 +65,16 @@ export type DispatchClustering = { proposed: number; /** Groups that merged with a tier-2 contribution (`via` is not similarity). */ clusterMerges: number; + /** + * Copies tier 2 is what absorbed, counted per copy rather than per group. + * Both numbers are recorded because they answer different questions and + * differ on any run with a `both` group: `clusterMerges` is how many + * comments tier 2 had a hand in, this is how many duplicate comments it + * removed. The live A/B's `clusterMerged` column is THIS quantity — the + * one the graduation decision reads — so a run's artifact and the report + * that graduated the tier cannot be compared and found to disagree. + */ + clusterMerged: number; /** Proposed members that did not merge, with the rule that stopped them. */ rejected: ClusterRejection[]; /** The clusterer ran and returned nothing usable (tier 1 only this run). */ @@ -148,6 +158,13 @@ export const clusteringRecord = ( clusterMerges: merged.merges.filter( (merge) => merge.via !== "similarity", ).length, + clusterMerged: merged.merges.reduce( + (sum, merge) => + sum + + merge.merged.filter((copy) => copy.via === "clusterer") + .length, + 0, + ), rejected: merged.clusterRejections, ...(step.unavailable ? {unavailable: true} : {}), } From b102a283e353893afc1212bd5c8e543821fb1a61 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 4 Aug 2026 10:45:44 -0700 Subject: [PATCH 7/8] review: settle tier 1 before the clusterer, so tier 2 can only add merges The tier's own docblock promises it may add merges and never subtract them, and no per-member screen can deliver that. Screening structure before the union closed one hole and left two: a same-path member that is structurally legal but ungrounded still unioned, won the survivor election, and dropped the tier-1 pair beneath it (three comments where tier 1 alone posted two); and a member legal in EVERY respect does the same whenever it displaces the survivor of a tier-1 group it was clustered into, orphaning that group's other copies (three comments where tier 1 alone posted one). Both are the same defect: survivor election over a set the cluster helped build. So the tiers now run in order. Tier 1 settles completely; tier 2 sees only the comments it left standing, reads each named member at the comment it now posts under, and carries that comment's own tier-1 copies along when it absorbs it. The guarantee is then structural rather than asserted, and the dense case gets stronger rather than weaker: the shape that used to post three comments now posts one. The parse-time structural screen stays for what it actually does (an illegal member cannot out-rank the legal ones and take a good proposal down with it), as does the per-member re-check against the elected survivor. Two tests, each verified failing on the previous merge core. --- .changeset/review-defect-clustering.md | 20 +- workflows/review/lib/dedup-cluster.test.ts | 119 +++++++++ workflows/review/lib/dedup-cluster.ts | 43 ++-- workflows/review/lib/dedup.ts | 272 +++++++++++++-------- 4 files changed, 323 insertions(+), 131 deletions(-) diff --git a/.changeset/review-defect-clustering.md b/.changeset/review-defect-clustering.md index 342315f9..ab4aed0e 100644 --- a/.changeset/review-defect-clustering.md +++ b/.changeset/review-defect-clustering.md @@ -82,13 +82,19 @@ arm delta prices the clusterer alone and a false merge shows up as recall loss. The report gains a "Cross-source claims merged (of candidates)" row with tier 2's share, its dollars and wall-clock, and per-case dedup counts. -Tier 2 can add merges and never subtract them, which the structural rules now -enforce before any membership is unioned rather than only afterward: an -unverifiable proposal that names a cross-path claim used to pull it into the -group, where it could out-rank the real survivor and collapse a merge tier 1 -would have made on its own, recorded in neither `merges` nor the rejection list. -The per-member re-check against the elected survivor stays, since tier-1 bridging -can still elect a claim the proposal never named. +Tier 2 can add merges and never subtract them, and that comes from the ORDER the +tiers run in rather than from any check on a proposal. Tier 1 settles completely, +and tier 2 then merges only the comments it left standing; a cluster member tier +1 has already absorbed is read at the comment it now posts under, and a comment +tier 2 absorbs carries its own tier-1 copies along. Screening each proposed +member before it was unioned was not enough, because a member can be legal in +every respect and still displace the survivor of a tier-1 group it was clustered +into, orphaning that group's other copies: with three copies folded into one +comment and a higher-confidence claim clustered with just one of them, the old +single-pass merge posted three comments where tier 1 alone posted one. The +structural pre-screen stays for what it does do (an illegal member cannot +out-rank the legal ones and take a good proposal down with it), as does the +per-member re-check against the elected survivor. `dispatch.ts` was at its 1000-line cap again, so the clustering step lands in `dispatch-cluster.ts` (dispatch, contract parse, telemetry) rather than raising diff --git a/workflows/review/lib/dedup-cluster.test.ts b/workflows/review/lib/dedup-cluster.test.ts index 072b4544..966bfb87 100644 --- a/workflows/review/lib/dedup-cluster.test.ts +++ b/workflows/review/lib/dedup-cluster.test.ts @@ -526,6 +526,125 @@ describe("dedupeClaims with model-proposed clusters", () => { ]); }); + it("never lets an UNGROUNDED proposal cost a merge tier 1 would have made", () => { + // The same floor, reached by a member the structural screen cannot + // catch: same path, distinct source, non-blocking — legal in every + // respect except that the evidence grounds nothing. Screening structure + // before the union was not enough, because grounding is only knowable + // against the survivor and the survivor is only known after the union; + // the guarantee comes from tier 1 being settled FIRST instead. + const [note] = wrongCapClaims(); + const copy = (over: Partial & {id: string; source: string}) => + claim({...note, line: 8, ...over}); + const {claims, merges, clusterRejections} = dedupeClaims( + [ + // Out-ranks both on confidence, and worded too thinly for the + // text floor, so it is a cluster member and nothing else. + claim({ + ...note, + id: "holistic-1", + source: "holistic", + line: 8, + confidence: 0.9, + subject: "the per-key cap disagrees with `maxSamples`", + discussion: "the per-key cap disagrees with `maxSamples`", + failure_scenario: "the cap disagrees with `maxSamples`", + }), + copy({id: "documentation-1", source: "documentation"}), + copy({id: "conventions-1", source: "conventions"}), + ], + [ + // Names no code element, so it can ground nothing. + { + evidence: "these are all about comments", + ids: ["holistic-1", "documentation-1"], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual([ + "holistic-1", + "documentation-1", + ]); + expect(merges).toEqual([ + { + survivor: "documentation-1", + merged: [ + { + id: "conventions-1", + source: "conventions", + label: "note (non-blocking)", + }, + ], + path: "dev/af19_trial/window.go", + line: 8, + via: "similarity", + }, + ]); + expect(clusterRejections).toEqual([ + {id: "documentation-1", reason: "ungrounded"}, + ]); + }); + + it("carries a tier-1 group whole when tier 2 absorbs its survivor", () => { + // The subtraction shape no per-member screen can reach: every member + // here is legal and grounded. Tier 1 folds three copies into one + // comment; the clusterer then names ONE of them beside a + // higher-ranked claim of its own. Unioning first would elect that + // claim, absorb the single member named, and orphan the other two into + // separate comments — three where tier 1 alone posted one. Reading the + // named member at the comment it now posts under is what makes tier 2 + // additive: the head comes over with everything folded into it. + const [note] = wrongCapClaims(); + const copy = (over: Partial & {id: string; source: string}) => + claim({...note, line: 8, ...over}); + const {claims, merges} = dedupeClaims( + [ + copy({id: "documentation-1", source: "documentation"}), + copy({id: "conventions-1", source: "conventions"}), + copy({id: "completeness-1", source: "completeness"}), + claim({ + ...note, + id: "holistic-1", + source: "holistic", + line: 8, + confidence: 0.9, + subject: "the per-key cap disagrees with `maxSamples`", + discussion: "the per-key cap disagrees with `maxSamples`", + failure_scenario: "the cap disagrees with `maxSamples`", + }), + ], + [ + { + evidence: "`maxSamples`", + ids: ["holistic-1", "conventions-1"], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual(["holistic-1"]); + expect(merges).toHaveLength(1); + expect(merges[0].via).toBe("both"); + // The tier-1 survivor is the copy tier 2 absorbed; the two it had + // already folded in come along, still credited to the text floor. + expect(merges[0].merged).toEqual([ + { + id: "documentation-1", + source: "documentation", + label: "note (non-blocking)", + via: "clusterer", + }, + { + id: "conventions-1", + source: "conventions", + label: "note (non-blocking)", + }, + { + id: "completeness-1", + source: "completeness", + label: "note (non-blocking)", + }, + ]); + }); + it("refuses a cluster whose SURVIVOR does not name the shared evidence", () => { // The grounding check runs against both ends, and the survivor end has // no other fixture: everywhere else the survivor is a cluster member diff --git a/workflows/review/lib/dedup-cluster.ts b/workflows/review/lib/dedup-cluster.ts index 4efb743e..ac85465c 100644 --- a/workflows/review/lib/dedup-cluster.ts +++ b/workflows/review/lib/dedup-cluster.ts @@ -15,16 +15,23 @@ * * Verification runs in TWO passes, and the split matters: * - * 1. {@link verifiableClusters}, before any union: id resolution, anchors, and - * the STRUCTURAL rules (shared path, distinct sources, at most one blocking - * member). A proposal that reaches the union with members tier 2 could never - * absorb does not merely fail to merge — it can change which claim becomes - * the group's survivor and thereby suppress a merge tier 1 would have made - * on its own, which is the one thing this tier must never do. + * 1. {@link verifiableClusters}, at parse time: id resolution, anchors, one + * cluster per claim, and the STRUCTURAL rules (shared path, distinct + * sources, at most one blocking member) against the proposal's own anchor. + * This is what keeps an illegal member from out-ranking a legal one inside + * the proposal and taking the merge down with it. * 2. {@link clusterMemberRejection}, per member against the group's ACTUAL - * survivor, which tier-1 bridging can change after the proposal was made. - * Re-checking there is what keeps the guarantee honest; the pre-screen - * narrows what can be unioned, it does not replace the final check. + * survivor, which is a claim tier 1 may have elected after the proposal was + * made. Re-checking there is what keeps the guarantee honest; the parse-time + * screen narrows what may be proposed, it does not replace the final check. + * + * Neither pass is what keeps tier 2 from SUBTRACTING a merge — from leaving a + * run with more comments than tier 1 alone would have posted. No per-member + * screen can: a member can be legal in every respect and still displace the + * survivor of a tier-1 group it was clustered into, orphaning that group's + * other copies. That guarantee lives in `dedup.ts`, in the order the tiers run + * (tier 1 settles first, tier 2 sees only what it left standing), and its + * reasoning is written down there. */ import {type Claim, type ProposedCluster} from "./dispatch-contracts"; @@ -34,7 +41,7 @@ import {isBlockingLabel} from "./render-comment"; * One member a proposed cluster named that did NOT merge, with the rule that * rejected it. Recorded per run because an empty rejection list and an empty * proposal list mean opposite things, and the module has already been burned - * by that ambiguity once (see `dedup-threads.ts`'s `stagedThreadShapeFailure`): + * by that ambiguity once (see `dedup.ts`'s `stagedThreadShapeFailure`): * a clusterer naming ids that do not exist is a prompt or staging failure, and * it must not read as "no duplicates found". */ @@ -161,17 +168,15 @@ export const clusterMemberRejection = ( (sharesSalientToken(evidenceTokens, member) ? undefined : "ungrounded"); /** - * Hold one proposal's members to the structural rules BEFORE they are unioned. + * Hold one proposal's members to the structural rules at parse time. * * This is not the survivor election — that happens after tier 1 has had its * say, and {@link clusterMemberRejection} re-runs against whatever claim wins - * it. It is the narrower guarantee that an unverifiable proposal cannot RESHAPE - * a group tier 1 owns: unioning a cross-path or same-source member pulls it - * into the group, where it can out-rank the real survivor, and the merge tier 1 - * would have made on its own then collapses to nothing — invisible in both - * `merges` and `clusterRejections`. Concretely, with `Y+Z` a genuine tier-1 - * pair on one path and a cross-path `X` out-ranking them, an unscreened - * `{ids: ["x", "y"]}` used to drop the `Y+Z` merge entirely. + * it. It is the narrower guarantee that a member tier 2 could never absorb + * cannot cost the merge the proposal was RIGHT about: a cross-path or + * same-source member left in the proposal can out-rank the legal members and + * become the group's survivor, at which point they are rejected against it and + * the whole proposal comes to nothing. * * The anchor is the proposal's own blocking member if it has one, else the * first member the id screen kept (the model's output order, the tiebreak the @@ -188,7 +193,7 @@ export const clusterMemberRejection = ( * * The rules are checked against the anchor only, exactly as * {@link clusterMemberRejection} checks them against the survivor — this is a - * filter on what may be unioned, not a stricter tier. Two copies from ONE + * filter on what may be proposed, not a stricter tier. Two copies from ONE * source can therefore still ride into a cluster anchored on a third; that is * the pre-existing shape of the source rule in both tiers, unchanged here. */ diff --git a/workflows/review/lib/dedup.ts b/workflows/review/lib/dedup.ts index 79813368..819e490f 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -23,6 +23,12 @@ * share. This module verifies that assertion and owns every merge rule; the * model contributes identity only. * + * They run in that ORDER, and {@link dedupeClaims} explains why at length: tier + * 1 settles completely, and tier 2 only ever merges the comments it left + * standing. That is the whole of what makes tier 2 additive — no per-member + * check can promise it — and it is why a run with the clusterer can never post + * more comments than the same run without it. + * * Why a second tier at all — the limit of tier 1, measured. Run 30587343777 * (webapp#41204, a FIRST review at `depth: full`, so no re-review artifact) * had four sources flag one wrong doc comment (`// Keeps at most 10 samples @@ -652,11 +658,32 @@ const survivorFirst = ( /** * Merge cross-source duplicates, preserving claim order: the similarity tier - * plus, when the clusterer ran, the defect clusters it proposed (verified - * here, never trusted). Non-anchored claims and everything neither tier - * identifies pass through untouched; when in doubt, don't merge (a false merge - * silently drops a reviewer's distinct finding, a missed merge only costs a - * duplicate comment). + * FIRST and on its own, then the defect clusters the clusterer proposed + * (verified here, never trusted) over whatever tier 1 left standing. + * Non-anchored claims and everything neither tier identifies pass through + * untouched; when in doubt, don't merge (a false merge silently drops a + * reviewer's distinct finding, a missed merge only costs a duplicate comment). + * + * The pass ORDER is the guarantee, not an implementation detail. Tier 2 may add + * merges and must never subtract them, and that only holds by construction if + * tier 1's groups are settled before a model-proposed cluster can touch them. + * A single union-find over both tiers cannot promise it however carefully each + * member is screened: unioning even a perfectly legal cluster member changes + * who wins the group's survivor election, and the star guard below then orphans + * the tier-1 duplicates that were mergeable against the OLD survivor and not + * against the new one. Concretely, with `b`, `c` and `d` all tier-1 mergeable + * against `a` and a higher-confidence `x` clustered with `b` alone, the merged + * group elects `x`, absorbs `b`, and leaves `a`, `c` and `d` posting three + * comments where tier 1 alone posted one. + * + * So: tier 1 runs to completion, and what it leaves standing — each surviving + * comment, plus every claim it declined to fold in — is the only thing tier 2 + * gets to see. A cluster member tier 1 already absorbed is read at the comment + * it now posts under (its head), and a head absorbed by tier 2 carries its own + * tier-1 copies along, because this pass merges COMMENTS and that comment + * already speaks for them. A cluster whose members tier 1 has already collapsed + * into one head therefore merges nothing and rejects nothing: it asserted an + * identity the text floors had reached first. * * `clusterRejections` is the tier-2 audit trail (see {@link ClusterRejection}); * it is empty both when the clusterer proposed nothing and when everything it @@ -676,9 +703,14 @@ export const dedupeClaims = ( ); const clusterRejections = [...rejections]; - // Union-find over pairwise-mergeable claims, then over each proposed - // cluster's members. Union is membership only: what actually collapses is - // decided per member against the group's survivor, below. + /** Claim index -> the claim whose comment it posts under after tier 1. */ + const head = claims.map((_, index) => index); + /** Survivor index -> the copies folded into it, with the tier that did it. */ + const absorbed = new Map(); + /** Survivor index -> the clusterer's grounding evidence, when tier 2 fired. */ + const groundedIn = new Map(); + + // ---- Tier 1: union-find over pairwise-mergeable claims. const parent = claims.map((_, index) => index); const find = (index: number): number => { while (parent[index] !== index) { @@ -694,24 +726,11 @@ export const dedupeClaims = ( } } } - const clusterAnchor = new Map(); - for (const [index, ordinal] of clusterOf) { - const anchor = clusterAnchor.get(ordinal); - if (anchor === undefined) { - clusterAnchor.set(ordinal, index); - } else { - parent[find(index)] = find(anchor); - } - } 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: ClaimMerge[] = []; for (const group of groups.values()) { if (group.length < 2) { continue; @@ -719,88 +738,134 @@ export const dedupeClaims = ( const survivorIndex = group.reduce((best, index) => survivorFirst(best, index, claims), ); + // Star guard: only a member {@link mergeable} against the survivor + // merges. Union-find alone chains A~B~C through a bridging claim that + // bundles two defects (a test-adequacy finding naming both a missing + // test and an unbounded read links the two distinct correctness + // findings), and collapsing the chain would silently drop a distinct + // finding; with no line window bounding groups, a bridge can span a + // whole file. Chain-only members stay their own claims. Both recorded + // trial merges are unaffected: run 29897276810's four-way group is + // pairwise-complete and run 29943085279's is a direct pair. + const merged = group.filter( + (index) => + index !== survivorIndex && + mergeable(claims[survivorIndex], claims[index]), + ); + if (merged.length === 0) { + continue; + } + for (const index of merged) { + head[index] = survivorIndex; + } + absorbed.set( + survivorIndex, + merged.map((index) => ({index})), + ); + } + + // ---- Tier 2: the verified clusters, over tier 1's heads. + // + // Each named member is read at its head, and a head takes the LOWEST + // ordinal that reaches it, so a head is a candidate in exactly one cluster + // however tier 1 has reshaped things and nothing can be absorbed twice. + // Rejections stay keyed to the ids the clusterer actually named. + const clusterHead = new Map(); + const namedByHead = new Map(); + for (const [index, ordinal] of clusterOf) { + const owner = head[index]; + const seen = clusterHead.get(owner); + clusterHead.set( + owner, + seen === undefined ? ordinal : Math.min(seen, ordinal), + ); + namedByHead.set(owner, [ + ...(namedByHead.get(owner) ?? []), + claims[index].id, + ]); + } + const headsByOrdinal = new Map(); + for (const [owner, ordinal] of clusterHead) { + headsByOrdinal.set(ordinal, [ + ...(headsByOrdinal.get(ordinal) ?? []), + owner, + ]); + } + for (const ordinal of [...headsByOrdinal.keys()].sort((a, b) => a - b)) { + const heads = (headsByOrdinal.get(ordinal) as number[]).sort( + (a, b) => a - b, + ); + if (heads.length < 2) { + continue; + } + const survivorIndex = heads.reduce((best, index) => + survivorFirst(best, index, claims), + ); const survivor = claims[survivorIndex]; - // The group's cluster evidence (lowest ordinal present, so the choice - // is deterministic when tier 1 has bridged two clusters). Tokenized - // once: an evidence string naming no code element grounds nothing, and - // that check is what makes an unverifiable identity claim inert rather - // than authoritative. - const ordinals = group - .map((index) => clusterOf.get(index)) - .filter((ordinal): ordinal is number => ordinal !== undefined); - const groupEvidence = - ordinals.length === 0 ? undefined : evidence[Math.min(...ordinals)]; - const evidenceTokens = - groupEvidence === undefined - ? undefined - : salientTokens(groupEvidence); - const clusterUsable = - evidenceTokens !== undefined && + // An evidence string naming no code element grounds nothing, and the + // survivor must name the element too: tier 1 can have elected a comment + // the proposal never saw, and absorbing a member into an unrelated + // comment is the failure mode this check exists to catch. + const groupEvidence = evidence[ordinal]; + const evidenceTokens = salientTokens(groupEvidence); + const usable = evidenceTokens.size > 0 && sharesSalientToken(evidenceTokens, survivor); - // Star guard: only a member {@link mergeable} against the survivor - // DIRECTLY merges on tier 1. Union-find alone chains A~B~C through a - // bridging claim that bundles two defects (a test-adequacy finding - // naming both a missing test and an unbounded read links the two - // distinct correctness findings), and collapsing the chain would - // silently drop a distinct finding; with no line window bounding - // groups, a bridge can span a whole file. Chain-only members stay - // their own claims. Both recorded trial merges are unaffected: run - // 29897276810's four-way group is pairwise-complete and run - // 29943085279's is a direct pair. - // - // The full pairwise predicate, not the text floor alone: a group can - // now contain a member tier 1 never proposed (a cluster unions on the - // model's word, which {@link verifiableClusters} does not screen for - // path or source), and a cross-path or same-source member whose prose - // happens to clear the floor must not slip in through this branch and - // be recorded as `via: "similarity"`. It falls through to the tier-2 - // rules below, which reject it by name. - // - // A cluster member takes the tier-2 path instead: it did not clear the - // floor (that is why the clusterer exists), so it merges on the - // verified rules alone. The evidence check runs against THIS survivor, - // so a group tier 1 has since reshaped is re-verified, not grandfathered. - const viaCluster = new Set(); - const others = group.filter((index) => { + const into = absorbed.get(survivorIndex) ?? []; + for (const index of heads) { if (index === survivorIndex) { - return false; - } - if (mergeable(survivor, claims[index])) { - return true; - } - if (clusterOf.get(index) === undefined) { - return false; - } - if (!clusterUsable) { - clusterRejections.push({ - id: claims[index].id, - reason: "ungrounded", - }); - return false; + continue; } - const rejection = clusterMemberRejection( - survivor, - claims[index], - evidenceTokens as ReadonlySet, - ); - if (rejection !== undefined) { - clusterRejections.push({ - id: claims[index].id, - reason: rejection, - }); - return false; + // The structural rules re-run here, not only in the parse-time + // screen: they were checked against the proposal's own anchor, and + // the head that survived tier 1 need not be the claim the model + // named. + const reason = !usable + ? ("ungrounded" as const) + : clusterMemberRejection( + survivor, + claims[index], + evidenceTokens, + ); + if (reason !== undefined) { + for (const id of namedByHead.get(index) ?? []) { + clusterRejections.push({id, reason}); + } + continue; } - viaCluster.add(index); - return true; - }); - if (others.length === 0) { - continue; + // The head comes over with everything tier 1 folded into it: this + // pass merges COMMENTS, and that comment already speaks for its own + // absorbed copies. Dropping them here instead would leave them + // posting on their own — tier 2 subtracting a tier-1 merge. + into.push({index, via: "clusterer"}); + into.push(...(absorbed.get(index) ?? [])); + absorbed.delete(index); + groundedIn.set(survivorIndex, groupEvidence); + } + if (into.length > 0) { + absorbed.set(survivorIndex, into); } - for (const index of others) { + } + + // ---- Render one merge per surviving comment, in claim order. + const drop = new Set(); + const replacement = new Map(); + const merges: ClaimMerge[] = []; + const entries = [...absorbed.entries()].filter( + ([, list]) => list.length > 0, + ); + entries.sort( + ([a, listA], [b, listB]) => + Math.min(a, ...listA.map((copy) => copy.index)) - + Math.min(b, ...listB.map((copy) => copy.index)), + ); + for (const [survivorIndex, list] of entries) { + const survivor = claims[survivorIndex]; + const others = [...list].sort((a, b) => a.index - b.index); + for (const {index} of others) { drop.add(index); } - const otherClaims = others.map((index) => claims[index]); + const otherClaims = others.map(({index}) => claims[index]); // One entry per other source, first copy wins, naming that copy's // anchor when it is not the survivor's. With tier 2 merging across // anchors, "also flagged by test-adequacy" alone would hide that the @@ -821,7 +886,7 @@ export const dedupeClaims = ( // the case where that evidence is missing (the floor is what it could // not clear), so there the quote is the only thing carrying the ask. const sources: {source: string; line?: number; subject?: string}[] = []; - for (const index of others) { + for (const {index, via} of others) { const claim = claims[index]; if ( claim.source === survivor.source || @@ -834,7 +899,7 @@ export const dedupeClaims = ( ...(claim.line !== undefined && claim.line !== survivor.line ? {line: claim.line} : {}), - ...(viaCluster.has(index) + ...(via === "clusterer" ? {subject: claim.subject.replace(/\s+/g, " ").trim()} : {}), }); @@ -879,8 +944,8 @@ export const dedupeClaims = ( ? {author_dispute: adoptedDispute} : {}), }); - const clusterCount = others.filter((index) => - viaCluster.has(index), + const clusterCount = others.filter( + (copy) => copy.via === "clusterer", ).length; const via: MergeVia = clusterCount === 0 @@ -888,9 +953,10 @@ export const dedupeClaims = ( : clusterCount === others.length ? "clusterer" : "both"; + const groupEvidence = groundedIn.get(survivorIndex); merges.push({ survivor: survivor.id, - merged: others.map((index) => { + merged: others.map(({index, via: copyVia}) => { const claim = claims[index]; return { id: claim.id, @@ -899,17 +965,13 @@ export const dedupeClaims = ( ...(claim.line !== undefined && claim.line !== survivor.line ? {line: claim.line} : {}), - ...(viaCluster.has(index) - ? {via: "clusterer" as const} - : {}), + ...(copyVia === "clusterer" ? {via: copyVia} : {}), }; }), path: survivor.path as string, line: survivor.line as number, via, - ...(via === "similarity" || groupEvidence === undefined - ? {} - : {evidence: groupEvidence}), + ...(groupEvidence === undefined ? {} : {evidence: groupEvidence}), }); } return { From 5bb4e3c2682e435578eb9061caada63f9fc5af91 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 4 Aug 2026 10:45:44 -0700 Subject: [PATCH 8/8] review: keep a paid clusterer failure out of the zero, and pin three branches - A dispatched clusterer that returned nothing usable is now reported as `N clusterer failure(s)` on the A/B's merge row rather than folded into "0 by clusterer at $X": production already keeps the two apart (`DispatchClustering.unavailable`) precisely because they are the same zero in every other column and only one of them is a measurement. - Three uncovered branches get tests: a same-path, cross-source, BOTH-blocking pair is not dispatched (verified failing with the severity conjunct removed); an absorbed copy's `suggested_patch` reaches the survivor's finding (likewise verified); and the rejected-member note renders, alongside the new failure note. - eval/README.md records tier 2's keep-or-cut bar before the next powered run rather than after it: any recall loss cuts it outright, 0.15 absorbed copies per dispatched case as the rate floor, $0.50 per copy and 8% of arm cost as the price ceiling, with run 30651373253's measured 0.20 / $0.34 / +6% beside each. A failure rate over 10% of dispatches means the run measured plumbing. - Fix the stale `dedup-threads.ts` pointer (it is `dedup.ts`). --- workflows/review/eval/README.md | 23 +++++++- workflows/review/eval/live-ab-report.ts | 19 +++++++ workflows/review/eval/live-ab.test.ts | 40 +++++++++++++- workflows/review/eval/live-ab.ts | 7 +++ workflows/review/eval/live-dedup.ts | 13 +++++ workflows/review/eval/live-producer.test.ts | 53 +++++++++++++++++++ workflows/review/lib/dispatch-cluster.test.ts | 43 +++++++++++++++ 7 files changed, 196 insertions(+), 2 deletions(-) diff --git a/workflows/review/eval/README.md b/workflows/review/eval/README.md index 2727f9f0..aa04a395 100644 --- a/workflows/review/eval/README.md +++ b/workflows/review/eval/README.md @@ -176,7 +176,28 @@ claiming a band. MEMBERS the merge rules refused, so one bad proposal naming three ids counts three (`unknown-id` there means the clusterer named claims that do not exist, which is a prompt or staging failure rather - than a quiet zero). + than a quiet zero). A dispatch that returned nothing usable is reported as + `N clusterer failure(s)` rather than folded into the zero: the arm paid and + measured nothing, which is not the claim that tier 2 found no duplicates. +- **Tier 2's keep-or-cut bar,** written down before the next powered run so the + decision is auditable after it. The tier ships enabled in the default + template, so this is the bar it must keep clearing, not one it must clear to + arrive. Read on the candidate arm of a `--repeats` run: + - **Any recall loss cuts it.** One `lost` spec traceable to a tier-2 merge + ends the tier; no merge rate buys back a dropped finding. Same for a failed + adversarial gate. This one is not traded off against the others. + - **Rate floor: 0.15 absorbed copies per dispatched case** (`by clusterer` + over the cases where the clusterer actually ran). Run 30651373253 measured + 0.20 (4 over 20 case-runs). Below the floor the steady state is a serial + Sonnet call on nearly every review that mostly does nothing, and the + dispatch precondition should be tightened or the tier cut. + - **Price ceiling: $0.50 per absorbed copy, and 8% of the arm's cost.** That + run measured $0.34 and +6% ($22.93 -> $24.30). The two are both needed: a + cheap tier that never fires and an expensive tier that fires often fail in + different ways. + - **Failures are not no-ops.** If `clusterer failure(s)` exceeds 10% of + dispatches the run measured plumbing, not a rate; fix it and re-measure + rather than reading the rate as a negative result. - **Anchor-snap and the arms:** the deterministic pipeline is shared by both arms, but the provenance gate emulates each arm's OWN review.md gate version, keyed on the literal `anchor-snap` marker in the gate step. A diff --git a/workflows/review/eval/live-ab-report.ts b/workflows/review/eval/live-ab-report.ts index 59b9510a..fc264ae5 100644 --- a/workflows/review/eval/live-ab-report.ts +++ b/workflows/review/eval/live-ab-report.ts @@ -78,6 +78,13 @@ export type ArmRunReport = { clusterMerged: number; rejected: number; clustererAbsent: boolean; + /** + * The clusterer was dispatched on this case and returned nothing + * usable. Reported separately because the failure and a clusterer + * that ran and proposed nothing are the same zero in every other + * column, and only one of them is a measurement. + */ + clustererFailed?: true; /** * The clusterer's own spend and wall-clock on this case, absent * when it never ran. Tier 2 is a serial dispatch on nearly every @@ -324,6 +331,10 @@ const snappedTotal = (arm: ArmRunReport): number => * every run, and "4 merges" is a graduation argument only next to what those * four merges cost. Tier 1 is free by comparison (pure text arithmetic), so no * price is shown for it. + * + * A dispatched clusterer that returned nothing usable is called out rather than + * folded into the zero: the arm paid for it and measured nothing, which is not + * the same claim as "tier 2 found no duplicates here". */ const mergedTotal = (arm: ArmRunReport): string => { const dedup = arm.perCase.flatMap((c) => (c.dedup ? [c.dedup] : [])); @@ -335,6 +346,7 @@ const mergedTotal = (arm: ArmRunReport): string => { const absent = dedup.every((d) => d.clustererAbsent); const clustererUsd = sum((d) => d.clustererUsd ?? 0); const clustererWallMs = sum((d) => d.clustererWallMs ?? 0); + const clustererFailures = sum((d) => (d.clustererFailed === true ? 1 : 0)); const notes = [ absent ? "tier 1 only" @@ -346,6 +358,13 @@ const mergedTotal = (arm: ArmRunReport): string => { ...(sum((d) => d.rejected) > 0 ? [`${sum((d) => d.rejected)} proposed member(s) rejected`] : []), + // A dispatched clusterer that returned nothing usable. Without this + // the row reads "0 by clusterer at $0.32" — the arm paid, produced no + // measurement, and the number that graduates the tier records it as a + // negative result. + ...(clustererFailures > 0 + ? [`${clustererFailures} clusterer failure(s)`] + : []), ]; return `${sum((d) => d.merged)} / ${sum((d) => d.candidates)} (${notes.join( ", ", diff --git a/workflows/review/eval/live-ab.test.ts b/workflows/review/eval/live-ab.test.ts index 8d740612..d3c12a9a 100644 --- a/workflows/review/eval/live-ab.test.ts +++ b/workflows/review/eval/live-ab.test.ts @@ -2,6 +2,7 @@ import {describe, it, expect} from "vitest"; import {aggregateSamples, extractSamples} from "./aggregate"; import {parseCase, type CorpusCase} from "./corpus/loader"; +import type {LiveDedupReport} from "./live-dedup"; import { adversarialGateFailures, diffRegressions, @@ -181,7 +182,10 @@ describe("runArm dedup accounting", () => { * and the evidence the clusterer grounded them in. */ const produceMerged = - (clustererAbsent: boolean): ArmProduce => + ( + clustererAbsent: boolean, + over: Partial = {}, + ): ArmProduce => async () => { const base = await produceHit(1)(liveCase("case-1")); return { @@ -250,6 +254,8 @@ describe("runArm dedup accounting", () => { proposed: 1, rejected: [], clustererAbsent, + clustererFailed: false, + ...over, }, }; }; @@ -326,6 +332,38 @@ describe("runArm dedup accounting", () => { expect(markdown).toContain("3 / 4 (2 by clusterer at $0.25 / 21s)"); }); + it("renders rejected members, and a paid dispatch that measured nothing", async () => { + // Both notes hang off the same row and neither was rendered by a test: + // `rejected` is how a clusterer proposing junk shows up at all, and + // `clustererFailed` is what keeps a parse failure from reading as + // "tier 2 ran and found no duplicates" in the row that graduates it. + const report = await runArm( + "candidate", + [liveCase("case-1")], + produceMerged(false, { + clustererFailed: true, + rejected: [ + {id: "live-x-1", reason: "ungrounded"}, + {id: "live-x-2", reason: "other-path"}, + ], + }), + {maxUsd: 10}, + ); + expect(report.perCase[0].dedup?.clustererFailed).toBe(true); + const markdown = renderMarkdownReport({ + baseRef: "origin/main", + reviewMdSha: {baseline: "a".repeat(12), candidate: "b".repeat(12)}, + arms: {baseline: report, candidate: report}, + regressions: {lost: [], gained: []}, + adversarialFailures: [], + gateRetries: [], + }); + expect(markdown).toContain( + "3 / 4 (2 by clusterer at $0.25 / 21s, " + + "2 proposed member(s) rejected, 1 clusterer failure(s))", + ); + }); + it("omits the block for a producer that runs no dedup at all", async () => { const report = await runArm( "candidate", diff --git a/workflows/review/eval/live-ab.ts b/workflows/review/eval/live-ab.ts index 9c7381ce..d311e6d3 100644 --- a/workflows/review/eval/live-ab.ts +++ b/workflows/review/eval/live-ab.ts @@ -264,6 +264,13 @@ export const runArm = async ( ), rejected: produced.dedup.rejected.length, clustererAbsent: produced.dedup.clustererAbsent, + // A dispatched clusterer that returned nothing + // usable, kept apart from one that ran and proposed + // nothing: both are zero merges, and only one of + // them is a result. + ...(produced.dedup.clustererFailed + ? {clustererFailed: true as const} + : {}), // What tier 2 COST, beside what it merged. The // clusterer is a serial step on nearly every // multi-finding run while absorbing a fraction of a diff --git a/workflows/review/eval/live-dedup.ts b/workflows/review/eval/live-dedup.ts index 10e72f4f..46ead928 100644 --- a/workflows/review/eval/live-dedup.ts +++ b/workflows/review/eval/live-dedup.ts @@ -34,6 +34,16 @@ export type LiveDedupReport = { rejected: ClusterRejection[]; /** The arm's review.md defines no clusterer: tier 1 only, by construction. */ clustererAbsent: boolean; + /** + * The clusterer was dispatched and returned nothing usable (no output, or + * an output that would not parse as the contract). Distinct from a + * clusterer that ran and proposed nothing, which is the same `proposed: 0` + * and the same zero merges: production keeps the two apart for the same + * reason (`DispatchClustering.unavailable`), because a paid-for parse + * failure rendered as "0 by clusterer at $X" reads as a negative result + * from the one row the graduation decision turns on. + */ + clustererFailed: boolean; }; /** @@ -86,11 +96,13 @@ export const dedupeLiveFindings = async ( const clusterable = hasClusterableCandidatePair(claims); let proposals: ProposedCluster[] = []; let report: PerAgentReport | undefined; + let clustererFailed = false; if (clusterable && clusterer !== undefined) { io.write("candidates.json", JSON.stringify(claims, null, 2)); const dispatched = await io.dispatch(clusterer, parseClustererOutput); report = dispatched.report; proposals = dispatched.parsed ?? []; + clustererFailed = dispatched.parsed === undefined; } const merged = dedupeClaims(claims, proposals); const dropped = new Set( @@ -135,6 +147,7 @@ export const dedupeLiveFindings = async ( proposed: proposals.length, rejected: merged.clusterRejections, clustererAbsent: clusterer === undefined, + clustererFailed, }, }, }; diff --git a/workflows/review/eval/live-producer.test.ts b/workflows/review/eval/live-producer.test.ts index eae9ae80..903cac44 100644 --- a/workflows/review/eval/live-producer.test.ts +++ b/workflows/review/eval/live-producer.test.ts @@ -787,9 +787,62 @@ describe("produceLive cross-source dedup", () => { proposed: 0, rejected: [], clustererAbsent: true, + clustererFailed: false, }); }); + it("records a clusterer that was paid for and returned nothing usable", async () => { + // Production keeps a failed dispatch apart from a clusterer that ran + // and proposed nothing (`DispatchClustering.unavailable`); the A/B has + // to as well, because the two are the same zero in the merge row that + // decides whether tier 2 keeps its place. + const {runner} = scriptedRunner({ + ...scripts(), + "claim-clusterer": ["not a contract", "still not a contract"], + }); + const result = await produceLive(CASE, withClusterer, { + runner, + stageDir: "/stage", + fs: volFs(caseVol()), + }); + expect(result.dedup.clustererFailed).toBe(true); + expect(result.dedup.proposed).toBe(0); + expect(result.findings).toHaveLength(2); + }); + + it("carries a merged copy's suggested patch onto the survivor", async () => { + // The survivor posts the comment, so it has to post the fix too: an + // absorbed copy's patch is the one committable thing a merge can + // silently throw away, and dedup adopts it only when the survivor has + // none of its own. Nothing downstream of the merge re-reads the + // absorbed finding, so this is the only place it can be checked. + const {runner} = scriptedRunner({ + ...scripts(), + "skill-auditor": [ + JSON.stringify({ + findings: [ + { + ...CAP_NITPICK, + suggestion: "// Keeps at most 25 samples per key.", + }, + ], + }), + ], + }); + const result = await produceLive(CASE, withClusterer, { + runner, + stageDir: "/stage", + fs: volFs(caseVol()), + }); + expect(result.findings).toHaveLength(1); + expect(result.findings[0].finding.id).toContain( + "live-correctness-reviewer-1", + ); + expect(result.findings[0].finding.suggested_patch).toBe( + "// Keeps at most 25 samples per key.", + ); + }); + it("never dispatches the clusterer when one source produced everything", async () => { const {runner, requests} = scriptedRunner({ ...scripts(), diff --git a/workflows/review/lib/dispatch-cluster.test.ts b/workflows/review/lib/dispatch-cluster.test.ts index c7bc69e5..c836aa21 100644 --- a/workflows/review/lib/dispatch-cluster.test.ts +++ b/workflows/review/lib/dispatch-cluster.test.ts @@ -362,6 +362,49 @@ describe("runDispatch defect clustering (dedup tier 2)", () => { expect(result.claims).toHaveLength(2); }); + it("never spends on clustering when both sides of every pair are blocking", async () => { + // The third conjunct of the precondition, and the only one no test + // pinned. These two sit on one path from two sources, so the count, + // the anchor, the path and the source rules all pass; both are + // BLOCKING, and tier 2 only ever absorbs a non-blocking copy, so the + // clusterer has nothing it could legally propose. A future edit that + // loosens the severity conjunct would otherwise start paying for a + // serial dispatch on every all-blocking review with no test noticing. + const blocking = (source: string): string => + JSON.stringify({ + findings: [ + { + ...JSON.parse(source).findings[0], + label: "issue (blocking)", + }, + ], + }); + const fs = makeFakeFs({ + ...baseStaging(), + ...agentFiles( + "pattern-triage", + "correctness-reviewer", + "skill-auditor", + "claim-clusterer", + "claim-validator", + ), + }); + const runner = stubRunner({ + "pattern-triage": TRIAGE_OK, + "correctness-reviewer": blocking(CAP_NOTE), + "skill-auditor": blocking(CAP_NITPICK), + "claim-validator": JSON.stringify({claims: []}), + }); + const result = await runDispatch(options(fs, runner)); + expect(runner.calls).not.toContain("claim-clusterer"); + expect(result.clustering).toBeUndefined(); + expect(fs.files[`${REVIEW}/candidates.json`]).toBeUndefined(); + // And nothing merged them on the text floor either, so the skip is + // what left two comments rather than a merge this test can't see. + expect(result.claims).toHaveLength(2); + expect(result.merges).toEqual([]); + }); + it("records the ids a clusterer invents rather than merging on them", async () => { const fs = makeFakeFs({ ...baseStaging(),