review: cluster candidate claims by defect identity, not by anchor - #312
Conversation
…identity, not by anchor Four sources flagged one wrong doc comment in run 30587343777 (webapp#41204, a FIRST review at depth: full) and `merges` recorded none of them; autofix later satisfied all four with a single rewritten comment. Replaying that run's claims.json shows the similarity tier is nowhere near reaching it: three of the four share the EXACT anchor and still score 0.060-0.068 Jaccard against a 0.14 floor with 0-1 shared bigrams against a floor of 4. Nor can reweighting the text recover the discriminator, because the pairs dedup deliberately keeps apart share MORE salient tokens than the real duplicates do (an AddDate arithmetic bug and "this behavior is never exercised" on one line share AddDate, MemoryTTLDays, 180, 15). Duplicates are "same ask, different words"; those are "same facts, different ask". So identity moves to the defect: a `claim-clusterer` sub-agent names the groups, dedup.ts verifies them and owns every merge rule. No line agreement is required, which is what makes the same-defect-different-anchor shape mergeable at all. The model must ground each group in a code element its members share, and that is checked; only a non-blocking copy may be absorbed on a model's word, so a false merge costs an advisory comment and never a blocking finding or a verdict. The live A/B now runs dedup (it never did, so this was unmeasurable by construction). Tier 1 runs in both arms; tier 2 rides each arm's own review.md, so the delta prices the clusterer alone and a false merge lands as recall loss. Merge counts are reported per arm and recorded in dispatch-result.json's new `clustering` block.
…s recipe The powered-run recipe for a duplicate-comment change (the documentation-enabled cases are the ones that produce multi-source clusters), the report row's reading rules, and a note on the producer's in-place replacement of the pre-merge set.
…not just the count Auditing run 30651373253 turned up the gap: the report showed that four claims merged on the candidate arm and not WHICH, so reading a suspicious merge meant paying for the run again. A false merge is the failure mode of model-proposed clustering, and it is only diagnosable from the absorbed ids and the evidence the clusterer grounded them in, so perCase.dedup now carries the groups. Report-only: no reviewer-visible behaviour changes, so the powered run's numbers still stand.
🦋 Changeset detectedLatest commit: 5bb4e3c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…'s rules, and attribute each merge per copy Review feedback on the clustering tier, in the order it bites. A cluster unions its members on the model's word before any of them is checked, and the star guard then merged anything that cleared the TEXT floor against the survivor. So a proposed member on another path, or from the survivor's own source, merged there without ever meeting the rules `clusterMemberRejection` exists to apply, and the artifact recorded it as `via: "similarity"`: a reviewer's distinct (possibly blocking) finding dropped, attributed to the wrong tier, against the module's own guarantee. The branch now takes the full `mergeable` predicate, so such a member falls through to the tier-2 rules that reject it by name. The existing rules test passed only because its fixtures sit below the floor; the new one uses verbatim-identical copies, so the floor is decidedly not what keeps them apart. `no-anchor`, the one rejection reason with no coverage, gets a case too. `merges[].merged[]` gains a per-copy `via`, because a group's own `via` can be `both` and the A/B's `clusterMerged` column summed the whole group: tier 1's members were being credited to the clusterer in the number that decides graduation. It is counted per absorbed copy now, in the report and in the eval's per-case groups. The "also flagged by" note quotes the subject of any copy tier 2 absorbed. One edit discharging every member's ask does not mean every member asked in the same words: run 30587343777's `conventions` copy wanted the symbol-name prefix, not the corrected number, and the survivor's prose says nothing about it. Tier-1 copies stay unquoted, since clearing the floor against the survivor is the evidence that they restate it, and repeating four near-identical subjects would move the duplicate noise into the surviving comment instead of removing it. The dispatch gate was ">= 2 claims from >= 2 sources", which still pays for a serial clusterer run when the only cross-source pairs sit in different files or are blocking on both sides; neither is mergeable at tier 2 under any proposal. It now gates on a legally-mergeable pair, computed from the candidates already in hand, and the eval's producer imports that same predicate rather than restating it. Also: the clusterer's frontmatter justified sonnet with "no code investigation" while the prompt has it read each candidate's cited lines. The rationale now says what the prompt actually asks for (locating, capped there) so nobody sizes tool permissions off it, and the producer's doc names which parts of the step are shared code and which are the hand-mirrored seam.
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus cc11988c0918 (9 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
Two conflicts, both add-add at the same point: - dispatch.ts: main split the agent loader into dispatch-agents.ts and re-exported it from the same block this branch added the dispatch-cluster re-export to. Kept both. - dispatch-contracts.test.ts: main appended a parseJsonObject empty-vs-malformed describe where this branch appended parseClustererOutput's. Kept both.
The blocking review finding: `verifiableClusters` screened only unknown-id / no-anchor / already-clustered, so a structurally invalid proposal still unioned its members into the group. A cross-path member pulled in that way can out-rank the real survivor, and the tier-1 merge underneath then collapses to nothing -- recorded in neither `merges` nor `clusterRejections`. Tier 2 could degrade BELOW tier 1, which is the one thing dispatch-cluster.ts promises it cannot. The path/source/severity rules now run per proposal before any union, on the anchor the proposal's own members elect (its blocking member if it has one, else the first the id screen kept -- only the severity rule is asymmetric, and 'advisory absorbed into blocking' must stay legal). What survives is a membership hint whose members are mutually absorbable; the per-member re-check against the ACTUAL survivor stays, because tier-1 bridging can still elect a claim the proposal never named. Two tests: the lost-merge shape itself (fails without the screen), and the survivor-time re-check that the screen does not subsume -- a member legal against the anchor but same-source as the survivor tier 1 elects. Also splits the modules the merge with main pushed over the 1000-line cap: tier 2 to lib/dedup-cluster.ts (the name its tests already carried) and the eval's dedup stage to eval/live-dedup.ts, mirroring dispatch-cluster.ts on each side. dispatch.ts sheds the empty section header main's dispatch-agents split left behind, and dedup.ts's orphaned `mergeable` doc comment is reattached to `mergeable`. Plus the review's other import ask: live-producer.ts takes the exported CLUSTERER constant rather than restating the agent name.
- Production telemetry now records `clusterMerged` (absorbed copies) beside `clusterMerges` (groups). The eval counts per copy and production counted per group, so any run with a `both` group had the two artifacts reporting different numbers for the same quantity -- readable as production contradicting the evidence that graduated the tier. - The A/B's merge row carries tier 2's dollars and wall-clock beside its share. The dispatch precondition is met by most multi-finding reviews, so the steady state is a serial Sonnet call on nearly every run; a merge count with no price beside it cannot answer whether that earns its place. Read off the clusterer's own per-agent entry, so a skipped or absent step is zero. - A test isolating the survivor half of the grounding check: every other tier-2 fixture has a survivor that names the evidence by construction, so the conjunct could regress green. The gap is tier 1 electing a survivor the clusterer never proposed; the new case bridges a blocking claim in on the text floor and asserts the cluster-only member stays its own comment. - dedup.ts records why a finder-emitted identity key was rejected, since it clusters deterministically at zero dispatch and will be re-proposed otherwise: a finder mints its key blind to the other reviewers, and where a blind key does agree is the AddDate pair that must NOT merge.
There was a problem hiding this comment.
PR Reviewer — re-review
Re-review at 68bf922. All 6 threads from the prior review were verified addressed against the current code and are being resolved.
Verdict: Request changes — one new blocking correctness defect, introduced by this change, was confirmed by independent claim validation.
issue (blocking)—workflows/review/lib/dedup-cluster.ts: the tier-2 pre-union screen enforces the structural rules (path / source / blocking) but not the grounding check, so a structurally-legal-but-ungrounded same-path proposal member can be unioned, win the survivor election, and suppress a merge tier 1 would have made on its own — the one thing this tier's docblock says it must never do ("may add merges, never subtract them"). Trace inline.
Six non-blocking comments accompany it: a stale doc pointer, three test-coverage gaps, an eval failure-vs-nothing ambiguity, and one open question on the tier's graduation bar.
All 6 prior review threads are resolved.
There was a problem hiding this comment.
Changes requested — see inline comments.
7 of 7 prior review threads are still unaddressed as of 7b8bb2c:
- issue (blocking)
workflows/review/lib/dedup-cluster.ts:271: The pre-union screenstructurallyVerified(called here) enforces only the structural rules — same path, distinct sourc...
6 non-blocking threads still open
- thought (non-blocking)
workflows/review/eval/README.md:174: The clustering tier ships enabled in the default template (claim-clustereris defined directly inreview.md, not beh... - nitpick (non-blocking)
workflows/review/eval/live-ab-report.ts:347: The rejected-member note only renders whensum(rejected) > 0(lines 346-348), but the singlerenderMarkdownReportfi... - suggestion (non-blocking)
workflows/review/eval/live-dedup.ts:123: The survivor'ssuggestionwrite-back intosuggested_patch(lines 122-124) is never asserted by an integration test —... - suggestion (non-blocking)
workflows/review/eval/live-dedup.ts:93:proposals = dispatched.parsed ?? []folds a failed/unparseable clusterer dispatch into the same shape as a clusterer t... - nitpick (non-blocking)
workflows/review/lib/dedup-cluster.ts:37: This comment points readers to`dedup-threads.ts`'s `stagedThreadShapeFailure`, but there is nodedup-threads.ts... - note (non-blocking)
workflows/review/lib/dispatch-cluster.ts:55: The dispatch precondition here gates on!isBlockingLabel(a.label) || !isBlockingLabel(b.label), but `dispatch-cluster....
…rges The tier's own docblock promises it may add merges and never subtract them, and no per-member screen can deliver that. Screening structure before the union closed one hole and left two: a same-path member that is structurally legal but ungrounded still unioned, won the survivor election, and dropped the tier-1 pair beneath it (three comments where tier 1 alone posted two); and a member legal in EVERY respect does the same whenever it displaces the survivor of a tier-1 group it was clustered into, orphaning that group's other copies (three comments where tier 1 alone posted one). Both are the same defect: survivor election over a set the cluster helped build. So the tiers now run in order. Tier 1 settles completely; tier 2 sees only the comments it left standing, reads each named member at the comment it now posts under, and carries that comment's own tier-1 copies along when it absorbs it. The guarantee is then structural rather than asserted, and the dense case gets stronger rather than weaker: the shape that used to post three comments now posts one. The parse-time structural screen stays for what it actually does (an illegal member cannot out-rank the legal ones and take a good proposal down with it), as does the per-member re-check against the elected survivor. Two tests, each verified failing on the previous merge core.
…branches - A dispatched clusterer that returned nothing usable is now reported as `N clusterer failure(s)` on the A/B's merge row rather than folded into "0 by clusterer at $X": production already keeps the two apart (`DispatchClustering.unavailable`) precisely because they are the same zero in every other column and only one of them is a measurement. - Three uncovered branches get tests: a same-path, cross-source, BOTH-blocking pair is not dispatched (verified failing with the severity conjunct removed); an absorbed copy's `suggested_patch` reaches the survivor's finding (likewise verified); and the rejected-member note renders, alongside the new failure note. - eval/README.md records tier 2's keep-or-cut bar before the next powered run rather than after it: any recall loss cuts it outright, 0.15 absorbed copies per dispatched case as the rate floor, $0.50 per copy and 8% of arm cost as the price ceiling, with run 30651373253's measured 0.20 / $0.34 / +6% beside each. A failure rate over 10% of dispatches means the run measured plumbing. - Fix the stale `dedup-threads.ts` pointer (it is `dedup.ts`).
| * reasoning is written down there. | ||
| */ | ||
|
|
||
| import {type Claim, type ProposedCluster} from "./dispatch-contracts"; |
There was a problem hiding this comment.
nitpick (non-blocking): All-type import uses inline {type X} instead of the repo's import type {...} idiom. 113 all-type imports across workflows/review use the import type {...} form (e.g. render-comment.ts:21 import type {Anchor, Finding, Lens} from "./finding-schema";), and this PR's own live-dedup.ts:13 does too (import type {ClusterRejection} from "../lib/dedup-cluster";). The deviating line is import {type Claim, type ProposedCluster} from "./dispatch-contracts"; — the only two exceptions in the tree are this file and dispatch-cluster.ts, both new here. (ESLint strips all @typescript-eslint/* rules, so this is not lint-enforced.)
| import {type Claim, type ProposedCluster} from "./dispatch-contracts"; | |
| import type {Claim, ProposedCluster} from "./dispatch-contracts"; |
Lower-confidence observations (1)
workflows/review/lib/dedup.ts:68question (non-blocking): Was a deterministic anchor-multiplicity rule (>=3 distinct non-blocking sources on one exact anchor) considered before adding a model tier?
| 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 |
There was a problem hiding this comment.
thought (non-blocking): The keep-or-cut bar was derived from a corpus that lacks the motivating shape, and it prices dollars but not latency. The bar (0.15 absorbed copies/dispatched case, $0.50/copy, 8% of arm cost) is calibrated on run 30651373253, whose cases carry 2-6 claims and never the 4-way density of run 30587343777 — the queued dense fixture is exactly the evidence the graduation decision was missing. Checked: production's clustering block now records clusterMerged per run, so the ongoing keep-or-cut read could come from real-run telemetry rather than repeated $47 powered runs; and while clustererWallMs is recorded, the bar sets no wall-clock ceiling even though the tier is a serial step between finders and the validator on nearly every review.
A sketch, not a committable replacement:
Consider (a) making the dense-case fixture a precondition for the NEXT powered run's keep decision rather than a queued follow-up, (b) adding a latency ceiling to the bar alongside the dollar ceilings, and (c) noting that production `clustering` telemetry can carry the rate floor between powered runs.
Four sources flagged one wrong doc comment in review run 30587343777 (webapp#41204) and
mergesrecorded none of them. It was a FIRST review atdepth: fullwith no prior threads, so not a re-review artifact, and autofix later satisfied all four with a single rewritten comment: proof they were one defect.Why the floors could not be re-derived
Replaying that run's own
claims.jsonthrough the current metric:Three of the four already share the EXACT anchor and still fail the looser tier at zero shared bigrams. That is an order of magnitude below the floor, not a thin margin: the floor that admits 0.06 admits everything. Each reviewer wrote the defect in different words ("wrong cap (10 vs 25)", "the 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.
Reweighting the text cannot recover the discriminator either, because the pairs
dedup.tsdeliberately keeps apart share MORE salient tokens than the real duplicates do: run 29943085279'sAddDatearithmetic issue and its "central behavior never exercised" thought sit on one line and shareAddDate,MemoryTTLDays,180,15. Duplicates are "same ask, different words"; those pairs are "same facts, different ask" (a bug versus its missing test). Telling them apart is a semantic judgment, so tier 1 is untouched here (every existing fixture passes unmodified) and a second tier asks a model for the judgment instead of pretending a fourth threshold would find it.The change
Identity is the defect, not the anchor. A
claim-clusterersub-agent (sonnet, no code investigation) names the candidate groups that describe one defect;dedup.tsowns every merge rule and verifies the assertion. Tier 2 requires 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). The line survives as tier-1 evidence and as the survivor's posting anchor.The model contributes identity only. Each group must be grounded in the code element its members share, and that is checked, not trusted: a group whose
evidencenames 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.Risk is graded in code where judgment cannot be checked. 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. Accepted price, stated in the module: one defect flagged blocking by two sources in different words still posts twice unless tier 1 reaches it.
Degradation is soft both ways. Fewer than two claims, or one source, and nothing is dispatched (no spend). A missing definition or unusable reply leaves the run on tier 1 — today's behaviour — and surfaces as a run warning plus a
clusteringblock indispatch-result.json, not as an author-facing note: duplicate hygiene is not a review dimension.The merge rate is readable from the artifact. Each entry in
mergescarriesvia(similarity/clusterer/both) and the absorbed copies' own anchors;clusteringcarriescandidates(the pre-merge count),proposed,clusterMerges, and every rejected member with the rule that stopped it. That is the number to trust: autofix satisfies surviving duplicates with one edit, so the PR itself hides the symptom.The eval gate had to be built first
eval/live-producer.tsnever calleddedupeClaims. A change to the merge rules was unmeasurable by construction: the pipeline the A/B measured posted every duplicate production merges. It now runs the stage, arm-keyed — tier 1 in both arms (shared code, production has had it since #245), tier 2 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 lands as candidate-arm recall loss.Powered run 30651373253: 5
documentation-enabled cases x 2 arms x 5 repeats, $47.23lost)Criteria were written down before the numbers landed. Recall holds with no lost specs; tier 2 fires with a non-zero share; the adversarial gate passes; zero
unknown-idor ungrounded rejections (the clusterer never invented an id or proposed a group it could not ground).All four tier-2 merges were on
golden-documentation-restated-docstring, absorbing thedocumentationcopy into the correctness survivor in 4 of 5 repeats where the baseline merged it in 1. Two were cross-anchor (Also flagged by documentation (at line 13),(at line 10)) — the shape that was unmergeable by construction before. Price withdocumentationenabled: ~$0.07 per case, +6% on the arm.Verdict agreement 100% -> 91% is not this change. Two of five repeats returned REQUEST_CHANGES on
golden-documentation-stale-and-narratedbecause the correctness reviewer itself labeled the stale-comment findingissue (blocking)rather than advisory; the baseline called the same finding advisory in all five. Repeat 1 flipped with ZERO tier-2 merges, and tier 2 only absorbs non-blocking copies into the highest-severity survivor, so it cannot manufacture a blocking label. Pre-existing severity wobble in the correctness pass, inside the README's 75-100% verdict-agreement noise band, and filed as a follow-up rather than fixed here.Honest limit on magnitude: 4 merges over 20 case-runs on a corpus whose cases carry 2-6 claims each. The run demonstrates the mechanism and costs nothing in recall; it does not reproduce the 4-way density of the evidence run, because no corpus case is that dense. Minting one from run 30587343777's real material is the queued follow-up.
Layout
dispatch.tswas at the 1000-line cap again, so the clustering step lands indispatch-cluster.ts(dispatch, contract parse, telemetry) rather than raising the shared cap, and the tier-2 tests live indedup-cluster.test.ts/dispatch-cluster.test.ts— the precedent #304 set. The third commit is report-only: auditing the powered run showed the artifact recorded THAT four claims merged but not WHICH, soperCase.dedup.groupsnow carries the survivor, the absorbed ids, the tier, and the grounding evidence. A false merge is the failure mode of model-proposed clustering; it should be diagnosable without paying for the run again.1601 tests pass, typecheck clean, eslint clean over the CI paths.
Review feedback (commit 4, after the powered run)
One blocking finding and five smaller ones, all addressed:
via: "similarity". The branch now takes the fullmergeablepredicate; such a member falls through to the rules that reject it by name. The existing rules test passed only because its fixtures sit below the floor, so the new one uses verbatim-identical copies.merges[].merged[]carries a per-copyvia. The A/B'sclusterMergedcolumn summed whole groups, so abothgroup credited tier 1's members to the clusterer in the number that decides graduation. (The powered run above is unaffected: its four tier-2 merges were allvia: clusterergroups, noneboth.)conventionscopy of the wrong-cap defect wanted the symbol-name prefix, not the number), and tier 2 is exactly where the survivor's prose is known not to restate it. This changes the rendered note shape for tier-2 merges, so the two cross-anchor examples quoted above read as- documentation (at line 13): <subject>under the new format.>= 2 claims from >= 2 sourcesstill paid for a serial dispatch when the only cross-source pairs were cross-file or blocking on both sides. It now requires a pair tier 2 could legally merge, and the eval producer imports that predicate instead of restating it.no-anchorrejection test (the one reason with no coverage), and the clusterer's frontmatter rationale now matches what its prompt asks for (it reads the cited lines; "no code investigation" undersold the tool access).1604 tests pass, typecheck clean, eslint clean. The merge-rule change can only remove merges that violated the stated rules, and the gate change can only skip dispatches with nothing to merge, so the run's recall and merge counts stand; the note prose is the one thing above that no powered run has priced.
Review feedback (commits 5-6)
One blocking finding and six smaller ones, all addressed.
The blocking one, and the wider defect behind it. The reviewer traced a same-path, structurally-legal but UNGROUNDED cluster member that still unioned, won the survivor election, and dropped the tier-1 pair beneath it: three comments where tier 1 alone posted two. Reproducing it turned up a second instance the proposed fix (screening grounding pre-union) would not have closed, because a member legal in every respect does the same thing whenever it displaces the survivor of a tier-1 group it was clustered into. With
b,canddall tier-1 mergeable againstaand a higher-confidencexclustered withbalone, the union electsx, absorbsb, and leavesa,canddposting three comments where tier 1 alone posted one.Both are survivor election over a set the cluster helped build, so the fix is the ORDER rather than a third screen. Tier 1 now settles completely; tier 2 sees only the comments it left standing, reads each named member at the comment it now posts under, and carries that comment's own tier-1 copies along when it absorbs it. "May add merges, never subtract them" is structural now instead of asserted, and the dense case gets stronger rather than weaker: the second shape above posts ONE comment instead of three. Two tests, each verified failing on the previous merge core. The parse-time structural screen stays for what it actually does (an illegal member cannot out-rank the legal ones and take a good proposal down with it), as does the per-member re-check against the elected survivor.
A paid clusterer failure no longer reads as a zero.
LiveDedupReportcarriesclustererFailed, and the A/B merge row saysN clusterer failure(s)rather than folding a parse failure into0 by clusterer at $X— production already keeps the two apart, and that row is the one the graduation decision reads.The keep-or-cut bar is on the record before the next powered run, since the tier already ships enabled: any recall loss cuts it outright, 0.15 absorbed copies per dispatched case as the rate floor, $0.50 per copy and 8% of arm cost as the price ceiling, with run 30651373253's measured 0.20 / $0.34 / +6% beside each; a failure rate over 10% of dispatches means the run measured plumbing.
Plus three uncovered branches pinned (a both-blocking pair is not dispatched; an absorbed copy's
suggested_patchreaches the survivor; the rejected-member note renders) and the stalededup-threads.tspointer fixed.1655 tests pass, typecheck clean, eslint clean over the CI paths.
What no powered run has priced. The ordering change can only add merges relative to tier 1, so the run's recall floor stands by construction. Relative to the previous candidate it can also add merges in one shape the powered run never exercised: a cluster naming a copy tier 1 had already absorbed now reaches that copy's whole group. Worth a repeat run before merge if the merge-rate number is load-bearing for the decision.