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

.changeset/review-defect-clustering.md

@@ -0,0 +1,97 @@
+---
+"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, which the structural rules now
+enforce before any membership is unioned rather than only afterward: an
+unverifiable proposal that names a cross-path claim used to pull it into the
+group, where it could out-rank the real survivor and collapse a merge tier 1
+would have made on its own, recorded in neither `merges` nor the rejection list.
+The per-member re-check against the elected survivor stays, since tier-1 bridging
+can still elect a claim the proposal never named.
+
+`dispatch.ts` was at its 1000-line cap again, so the clustering step lands in
+`dispatch-cluster.ts` (dispatch, contract parse, telemetry) rather than raising
+the cap, 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,29 @@ 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).
 - **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,49 @@ 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'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 +310,48 @@ 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.
+ */
+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 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`]
+            : []),
+    ];
+    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 +546,11 @@ export const renderMarkdownReport = (report: AbReport): string => {
             String(snappedTotal(baseline)),
             String(snappedTotal(candidate)),
         ),
+        row(
+            "Cross-source claims merged (of candidates)",
+            mergedTotal(baseline),
+            mergedTotal(candidate),
+        ),
         "",
     ];