review: cluster candidate claims by defect identity, not by anchor

.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.

workflows/review/eval/README.md

@@ -77,6 +77,15 @@ gh workflow run review-eval-ab.yml --ref <branch> \
 gh workflow run review-eval-ab.yml --ref <branch> \
   -f base_ref=origin/<branch> -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 <branch> \
+  -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 <run-id> <run-id> ... [--out <path>]
 ```
@@ -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

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<ArmProduceResult>;
@@ -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;
+            }[];
+        };
         /** `<agent>: <reason>` 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<string, string> => {
     const map = new Map<string, string>();
@@ -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),
+        ),
         "",
     ];