diff --git a/.changeset/review-defect-clustering.md b/.changeset/review-defect-clustering.md new file mode 100644 index 00000000..ab4aed0e --- /dev/null +++ b/.changeset/review-defect-clustering.md @@ -0,0 +1,103 @@ +--- +"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. + +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 +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. 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` 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 +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 +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, its dollars and wall-clock, and per-case dedup counts. + +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 +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 49136942..aa04a395 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 ] ``` @@ -145,6 +154,50 @@ 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. 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; 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). 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 cefe91ec..fc264ae5 100644 --- a/workflows/review/eval/live-ab-report.ts +++ b/workflows/review/eval/live-ab-report.ts @@ -12,8 +12,13 @@ import type { CorpusCase, RecordedFinding, } from "./corpus/loader"; +import type {MergeVia} from "../lib/dedup"; 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 +30,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 +58,56 @@ 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` 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; + merged: number; + 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 + * 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 + * 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: {id: string; via?: "clusterer"}[]; + via: MergeVia; + evidence?: string; + }[]; + }; /** `: ` per failed agent (diagnosable from the report). */ failedAgents: string[]; /** @@ -260,6 +317,60 @@ 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 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. + * + * 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. + * + * 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] : [])); + 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 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" + : `${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`] + : []), + // 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( + ", ", + )})`; +}; + /** `caseId:specKey` -> drop bucket, for every found-but-dropped miss. */ const dropClassByKey = (arm: ArmRunReport): Map => { const map = new Map(); @@ -454,6 +565,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.test.ts b/workflows/review/eval/live-ab.test.ts index 11bb625a..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, @@ -172,6 +173,208 @@ 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, + over: Partial = {}, + ): ArmProduce => + 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, + clustererFailed: false, + ...over, + }, + }; + }; + + it("carries the per-case counts and the merged groups", async () => { + const report = await runArm( + "candidate", + [liveCase("case-1")], + 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: 3, + clusterMerged: 2, + rejected: 0, + clustererAbsent: false, + clustererUsd: 0.25, + clustererWallMs: 21_000, + groups: [ + { + survivor: "live-hit", + absorbed: [{id: "live-doc-1", via: "clusterer"}], + via: "clusterer", + evidence: "the `maxSamples` comment says 10, not 25", + }, + { + survivor: "live-hit-2", + absorbed: [ + {id: "live-conv-1"}, + {id: "live-holistic-1", via: "clusterer"}, + ], + via: "both", + evidence: "the `staleAfter` window", + }, + ], + }); + }); + + 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("3 / 4 (tier 1 only)"); + // 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("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", + [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 18291a8c..d311e6d3 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, @@ -236,6 +240,74 @@ 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, + ), + // 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, + // 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 + // 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 + // 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((copy) => ({ + id: copy.id, + ...(copy.via !== undefined + ? {via: copy.via} + : {}), + })), + via: merge.via, + ...(merge.evidence !== undefined + ? {evidence: merge.evidence} + : {}), + })), + }, + }), failedAgents: produced.perAgent .filter((a) => a.failed !== undefined) .map((a) => `${a.name}: ${a.failed}`), diff --git a/workflows/review/eval/live-dedup.ts b/workflows/review/eval/live-dedup.ts new file mode 100644 index 00000000..46ead928 --- /dev/null +++ b/workflows/review/eval/live-dedup.ts @@ -0,0 +1,154 @@ +/** + * 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; + /** + * 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; +}; + +/** + * 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; + 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( + 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, + clustererFailed, + }, + }, + }; +}; diff --git a/workflows/review/eval/live-producer.test.ts b/workflows/review/eval/live-producer.test.ts index a6dabca2..903cac44 100644 --- a/workflows/review/eval/live-producer.test.ts +++ b/workflows/review/eval/live-producer.test.ts @@ -671,3 +671,189 @@ 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:\n- skill: Declaration doc comment doesn't begin " + + "with the symbol name.", + ); + 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, + 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(), + "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 cae79669..9fb8310d 100644 --- a/workflows/review/eval/live-producer.ts +++ b/workflows/review/eval/live-producer.ts @@ -27,6 +27,13 @@ * 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. + * `live-dedup.ts`'s `dedupeLiveFindings` closes that, arm-keyed on the + * clusterer agent. */ import {refusalFallbackFor} from "../lib/refusal-fallback"; @@ -41,6 +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 {CLUSTERER} from "../lib/dispatch-cluster"; +import {dedupeLiveFindings, type LiveDedupReport} from "./live-dedup"; import { VERIFICATION_STATES, type CaseVerification, @@ -181,6 +190,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 = { @@ -327,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); @@ -827,6 +848,40 @@ 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), + { + 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[] = []; @@ -895,9 +950,12 @@ export const produceLive = async ( perAgent, staged, ...(reconciliation !== undefined ? {reconciliation} : {}), + dedup: dedup.result, }; }; /** 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 new file mode 100644 index 00000000..966bfb87 --- /dev/null +++ b/workflows/review/lib/dedup-cluster.test.ts @@ -0,0 +1,928 @@ +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. 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): 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([ + { + survivor: "correctness-reviewer-3", + merged: [ + { + id: "skill-auditor-ool-2", + 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", + 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([]); + // 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"}, + ]); + }); + + 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"}, + {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 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(); + 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"}, + {id: "correctness-reviewer-3", reason: "cluster-collapsed"}, + ]); + }); + + 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("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("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 + // 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, + // 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 + // 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:\n- first-principles (at line 58): " + + "The suite never reaches the delete.", + ); + 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"); + // 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-cluster.ts b/workflows/review/lib/dedup-cluster.ts new file mode 100644 index 00000000..ac85465c --- /dev/null +++ b/workflows/review/lib/dedup-cluster.ts @@ -0,0 +1,293 @@ +/** + * 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}, 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 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"; +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.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 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 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 + * 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 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. + */ +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.test.ts b/workflows/review/lib/dedup.test.ts index 8a2ac03f..0e9dadca 100644 --- a/workflows/review/lib/dedup.test.ts +++ b/workflows/review/lib/dedup.test.ts @@ -191,6 +191,7 @@ describe("dedupeClaims", () => { ], path: "services/ai-guide/memory/expiration.go", line: 38, + via: "similarity", }, ]); }); @@ -295,10 +296,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", }, ]); }); @@ -352,8 +355,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([ { @@ -363,10 +369,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", }, ]); }); @@ -463,6 +471,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 099fd6d7..819e490f 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -6,34 +6,130 @@ * 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. + * + * 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 + * 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. + * + * 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 + * 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 + * 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: 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"; +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 // injected port, so the determinism boundary above still holds here. import {isReviewBotAuthor} from "./threads"; +/** 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; + /** + * 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; + via: MergeVia; + /** The clusterer's grounding evidence, when tier 2 found this group. */ + evidence?: string; }; /** @@ -148,16 +244,6 @@ export const describesSameDefect = (a: Claim, b: Claim): boolean => { ); }; -/** - * 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) */ /* -------------------------------------------------------------------------- */ @@ -528,6 +614,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 && @@ -561,16 +657,60 @@ const survivorFirst = ( }; /** - * Merge high-confidence cross-source duplicates, preserving claim order. - * Non-anchored claims and everything below the similarity floor pass through + * Merge cross-source duplicates, preserving claim order: the similarity tier + * 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). + * 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 + * 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]; + + /** 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) { @@ -591,10 +731,6 @@ export const dedupeClaims = ( 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; @@ -602,36 +738,191 @@ export const dedupeClaims = ( const survivorIndex = group.reduce((best, index) => survivorFirst(best, index, claims), ); - const survivor = claims[survivorIndex]; - // 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 - // 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 others = group.filter( + // 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 && - describesSameDefect(survivor, claims[index]), + 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 (others.length === 0) { + if (heads.length < 2) { continue; } - for (const index of others) { + const survivorIndex = heads.reduce((best, index) => + survivorFirst(best, index, claims), + ); + const survivor = claims[survivorIndex]; + // 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); + const into = absorbed.get(survivorIndex) ?? []; + for (const index of heads) { + if (index === survivorIndex) { + continue; + } + // 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; + } + // 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); + } + } + + // ---- 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 sources = [ - ...new Set(otherClaims.map((claim) => claim.source)), - ].filter((source) => source !== survivor.source); + 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 + // 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). + // + // 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, via} of others) { + const claim = claims[index]; + 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} + : {}), + ...(via === "clusterer" + ? {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.join(", ")}.`; + : 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("\n")}`; const adoptedSuggestion = survivor.suggestion === undefined ? otherClaims.find((claim) => claim.suggestion !== undefined) @@ -653,15 +944,34 @@ export const dedupeClaims = ( ? {author_dispute: adoptedDispute} : {}), }); + const clusterCount = others.filter( + (copy) => copy.via === "clusterer", + ).length; + const via: MergeVia = + clusterCount === 0 + ? "similarity" + : clusterCount === others.length + ? "clusterer" + : "both"; + const groupEvidence = groundedIn.get(survivorIndex); merges.push({ survivor: survivor.id, - merged: otherClaims.map((claim) => ({ - id: claim.id, - source: claim.source, - label: claim.label, - })), + merged: others.map(({index, via: copyVia}) => { + const claim = claims[index]; + return { + id: claim.id, + source: claim.source, + label: claim.label, + ...(claim.line !== undefined && claim.line !== survivor.line + ? {line: claim.line} + : {}), + ...(copyVia === "clusterer" ? {via: copyVia} : {}), + }; + }), path: survivor.path as string, line: survivor.line as number, + via, + ...(groupEvidence === undefined ? {} : {evidence: groupEvidence}), }); } return { @@ -669,5 +979,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..c836aa21 --- /dev/null +++ b/workflows/review/lib/dispatch-cluster.test.ts @@ -0,0 +1,446 @@ +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:\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 + // from what survives on the PR. + expect(result.clustering).toEqual({ + candidates: 2, + proposed: 1, + clusterMerges: 1, + clusterMerged: 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, + clusterMerged: 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("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("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(), + ...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, + 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 new file mode 100644 index 00000000..dc2aefc8 --- /dev/null +++ b/workflows/review/lib/dispatch-cluster.ts @@ -0,0 +1,171 @@ +/** + * 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} from "./dedup"; +import type {ClusterRejection} from "./dedup-cluster"; +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. */ + candidates: number; + /** Well-formed clusters it proposed. */ + 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). */ + 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: + * 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 + * 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 => { + if (!hasClusterableCandidatePair(candidates)) { + 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, + clusterMerged: merged.merges.reduce( + (sum, merge) => + sum + + merge.merged.filter((copy) => copy.via === "clusterer") + .length, + 0, + ), + 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 c32ff5e5..50208506 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, parseJsonObject, parseValidatorOutput, @@ -472,6 +474,66 @@ describe("applyVerifications: corrected-field validation", () => { }); }); +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/); + }); +}); + describe("parseJsonObject empty-vs-malformed", () => { it("names an empty final as empty, not malformed", () => { // An empty final is how a refusal presents (#294). Calling it diff --git a/workflows/review/lib/dispatch-contracts.ts b/workflows/review/lib/dispatch-contracts.ts index 4187a306..e4383251 100644 --- a/workflows/review/lib/dispatch-contracts.ts +++ b/workflows/review/lib/dispatch-contracts.ts @@ -332,11 +332,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 @@ -365,6 +423,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 62314999..d3e8a277 100644 --- a/workflows/review/lib/dispatch.test.ts +++ b/workflows/review/lib/dispatch.test.ts @@ -667,16 +667,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 3760e687..0d70494b 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, @@ -78,11 +85,13 @@ export { applyVerifications, buildClaims, contractValidator, + parseClustererOutput, parseFinderOutput, parseValidatorOutput, type Candidate, type Claim, type ContractKind, + type ProposedCluster, type Verification, } from "./dispatch-contracts"; export { @@ -92,6 +101,13 @@ export { type Roster, type RosterShed, } from "./dispatch-roster"; +export { + clusteringRecord, + runClusterStep, + CLUSTERER, + type ClusterStep, + type DispatchClustering, +} from "./dispatch-cluster"; export { loadAgents, parseAgentFile, @@ -104,6 +120,7 @@ export { type ClaimMerge, type ThreadSuppression, } from "./dedup"; +export {type ClusterRejection} from "./dedup-cluster"; /* -------------------------------------------------------------------------- */ /* Seams */ @@ -200,10 +217,6 @@ const TRIAGE = "pattern-triage"; const RECONCILER = "thread-reconciler"; const VALIDATOR = "claim-validator"; -/* -------------------------------------------------------------------------- */ -/* Agent definitions (.claude/agents/.md) */ -/* -------------------------------------------------------------------------- */ - /* -------------------------------------------------------------------------- */ /* The dispatch run */ /* -------------------------------------------------------------------------- */ @@ -245,6 +258,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 @@ -366,6 +388,8 @@ export const runDispatch = async ( name, name === VALIDATOR ? "validator" + : name === CLUSTERER + ? "clusterer" : name === TRIAGE || name === RECONCILER ? "json" : lensNames.includes(name) @@ -780,7 +804,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: @@ -882,6 +929,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 cb981d08..70572971 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -609,7 +609,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 @@ -1510,6 +1513,73 @@ 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 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 +**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