diff --git a/.changeset/review-cluster-grounding.md b/.changeset/review-cluster-grounding.md new file mode 100644 index 00000000..4531898e --- /dev/null +++ b/.changeset/review-cluster-grounding.md @@ -0,0 +1,5 @@ +--- +"review": minor +--- + +Run 32390393344 (webapp#41609) posted one finding twice: two sources filed it on the same line of moderation_helpers.go, the claim-clusterer correctly proposed the pair as one cluster, and the grounding tripwire vetoed the merge as "ungrounded" because the cluster's evidence spoke in the hunk's identifiers (`_configIncludesModeration`, `shouldModerateDuringMainCompletion`) while both claims spoke config-side (`pre_flight_moderation_check`, `config_files`), zero shared salient tokens. The evidence is model prose with free word choice, so that check was grading the clusterer's phrasing rather than the identity it asserted. Two changes: an exactly shared anchor (the member sits on the survivor's own line; paths already match structurally) now grounds a proposed member with no vocabulary needed, and salient tokens fold casing styles (`PreFlightModerationCheck` and `pre_flight_moderation_check` are one token) so the vocabulary path tests names, not spellings. Grounding a member against the survivor's own text was considered and rejected: run 30587343777's cap survivor names `staleAfter` in a while-here aside, and the distinct staleAfter finding would falsely ground against it (that counterexample stays pinned in the tests). The 41609 pair is replayed verbatim as a regression fixture and now merges to one comment. Each clusterer-absorbed copy in dispatch-result.json now records which path grounded it (`groundedBy: "anchor" | "evidence"`), so the planned audit of "ungrounded" rejections can tell the two apart, and the clusterer prompt no longer promises the unconditional mechanical discard the code stopped making. diff --git a/workflows/review/lib/dedup-cluster-grounding.test.ts b/workflows/review/lib/dedup-cluster-grounding.test.ts new file mode 100644 index 00000000..66770209 --- /dev/null +++ b/workflows/review/lib/dedup-cluster-grounding.test.ts @@ -0,0 +1,375 @@ +import {describe, it, expect} from "vitest"; + +import {dedupeClaims} from "./dedup"; +import {salientTokens} from "./dedup-cluster"; +import type {Claim} from "./dispatch-contracts"; + +/** + * Tier 2's grounding paths, pinned by the run that reshaped them. Split from + * `dedup-cluster.test.ts` for its max-lines budget, exactly as that file was + * split from `dedup.test.ts`; the `claim` factory mirrors theirs. + */ + +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 32390393344 (webapp#41609, review-v1.17.0): the grounding tripwire's + * one measured false veto, verbatim from that run's `out/claims.json` and + * `out/claim-clusterer.json`. Two sources filed one finding (the experiment + * enrolled 3 configs, the change flips ~112) as two comments on + * `moderation_helpers.go:31`, the clusterer correctly proposed them as one + * cluster, and the run posted both anyway: its evidence spoke in the hunk's + * identifiers (`_configIncludesModeration`, `shouldModerateDuringMainCompletion`) + * while both claims spoke config-side (`pre_flight_moderation_check`, + * `config_files`), zero shared salient tokens, so the member was rejected + * "ungrounded". The shared-anchor grounding path exists because of this run; + * these tests replay it and pin the merge. + */ +const runClaims = (): Claim[] => [ + claim({ + id: "holistic-1", + source: "holistic", + path: "services/ai-guide/chat/ask/v2/moderation_helpers.go", + line: 31, + label: "thought (non-blocking)", + subject: + "Behavior now applies to all v2 moderation configs, not just the 3 the experiment measured.", + discussion: + "Behavior now applies to all v2 moderation configs, not just the 3 the experiment measured. The experiment enrolled only Exercise, activity-tutor-me, and classroom-learner-exercise (the only files that imported moderation-parallelism.yaml), but this gating change flips parallel moderation on for every v2 config containing a moderation modifier — a much larger set (grep for PreFlightModerationCheck/moderation across config_files shows dozens). The parallel path is config-agnostic so the risk is low, but the 'no regressions' evidence covers a subset; worth a conscious confirmation that the broader rollout is intended and safe.", + failure_scenario: + "A v2 config that runs moderation but was never enrolled in the moderationParallelism experiment (e.g. one of the many other config_files that include a moderation modifier) now moderates in parallel; if any such config relied on moderation-first ordering in a way the 3 enrolled configs did not, it regresses without ever having been measured.", + confidence: 0.5, + }), + claim({ + id: "first-principles-1", + source: "first-principles", + path: "services/ai-guide/chat/ask/v2/moderation_helpers.go", + line: 31, + label: "question (non-blocking)", + subject: + "The experiment enrolled 3 configs; this graduates the behavior to every v2 config that runs moderation (~112 config files declare pre_flight_moderation_check).", + discussion: + "The experiment enrolled 3 configs; this graduates the behavior to every v2 config that runs moderation (~112 config files declare pre_flight_moderation_check). I checked config_files/: only Exercise.json, activity-tutor-me.json, and classroom-learner-exercise.json imported the moderation-parallelism partial, while 112 of 151 v2 configs declare the moderation modifier — all of which now flip to parallel in one step. Parallel mode also means every flagged turn still pays for a main completion whose output is discarded, so the cost profile on high-flag-rate configs outside the experiment is unmeasured; was a staged rollout (e.g. graduating the experimented configs first) considered and rejected?", + failure_scenario: + "A config that was never enrolled in the experiment (different traffic shape, flag rate, or completion cost) hits a latency or spend regression that the three-config experiment could not have surfaced.", + confidence: 0.5, + }), + claim({ + id: "first-principles-2", + source: "first-principles", + path: "services/ai-guide/moderation/spec/SPEC.md", + line: 148, + label: "thought (non-blocking)", + subject: + "Graduation removes the last runtime lever for moderation ordering — no kill switch remains, unlike the sibling moderationSystem experiment.", + discussion: + "Graduation removes the last runtime lever for moderation ordering — no kill switch remains, unlike the sibling moderationSystem experiment. The SPEC's flag table shows the neighboring CGC work kept a `global-cgc-enabled` kill switch after its experiment, while this change hardcodes parallelism (the moderation-first v2 path is now unreachable code driven only from tests). Deleting the experiment plumbing matches repo convention, so this is only a thought: given the blast radius above, a short-lived kill switch for one or two deploy cycles might have been cheap insurance.", + failure_scenario: + "If parallel moderation misbehaves in production, the only remedy is a code revert through the full deploy pipeline rather than a flag flip, lengthening incident response on a child-safety-adjacent path.", + confidence: 0.5, + }), +]; + +describe("run 32390393344's vetoed true merge", () => { + it("folds casing styles, so one config key spelled two ways is one token", () => { + expect(salientTokens("`PreFlightModerationCheck`")).toEqual( + salientTokens("pre_flight_moderation_check"), + ); + expect(salientTokens("pre_flight_moderation_check")).toEqual( + new Set(["preflightmoderationcheck"]), + ); + }); + + it("drops an all-underscore token rather than grounding two claims on it", () => { + // `_` is salient by the underscore rule but canonicalizes to nothing; + // without canonicalToken's empty-string guard it would enter the set + // and ground any two claims that each quote a Go blank identifier. + expect(salientTokens("if _, ok := m[k]; !ok")).toEqual(new Set()); + }); + + it("merges the pair on the shared anchor, whatever the evidence's vocabulary", () => { + const {claims, merges, clusterRejections} = dedupeClaims(runClaims(), [ + { + evidence: + "`return _configIncludesModeration(input.Config)` in `shouldModerateDuringMainCompletion` now applies parallel moderation to all v2 configs, not just the 3 the experiment enrolled", + ids: ["holistic-1", "first-principles-1"], + }, + ]); + expect(claims.map((c) => c.id)).toEqual([ + "holistic-1", + "first-principles-2", + ]); + expect(claims[0].also_flagged_by).toEqual([ + { + source: "first-principles", + subject: + "The experiment enrolled 3 configs; this graduates the " + + "behavior to every v2 config that runs moderation (~112 " + + "config files declare pre_flight_moderation_check).", + }, + ]); + expect(merges).toEqual([ + { + survivor: "holistic-1", + merged: [ + { + id: "first-principles-1", + source: "first-principles", + label: "question (non-blocking)", + via: "clusterer", + groundedBy: "anchor", + }, + ], + path: "services/ai-guide/chat/ask/v2/moderation_helpers.go", + line: 31, + via: "clusterer", + evidence: + "`return _configIncludesModeration(input.Config)` in `shouldModerateDuringMainCompletion` now applies parallel moderation to all v2 configs, not just the 3 the experiment enrolled", + }, + ]); + expect(clusterRejections).toEqual([]); + }); +}); + +describe("the vocabulary path's casing fold", () => { + it("grounds a cross-line member that spells the evidence's name in another convention", () => { + // The merge-level case the fold exists for, one step milder than run + // 32390393344 (there the two spellings lived in the two CLAIMS, and + // grounding compares the evidence against each claim, so the fold + // alone could not have saved that merge; the anchor path did). Here + // the evidence names the config key in the hunk's Go casing, the + // member's claim names it in the configs' JSON casing, and the member + // sits off the survivor's line so the anchor path cannot carry it. + // Without `canonicalToken`'s fold the spellings share nothing and the + // member is rejected "ungrounded"; the unit assertion above pins the + // fold itself, this pins a merge outcome on it. + const {claims, merges, clusterRejections} = dedupeClaims( + [ + claim({ + id: "holistic-1", + source: "holistic", + path: "services/ai-guide/chat/ask/v2/moderation_helpers.go", + line: 31, + label: "thought (non-blocking)", + subject: + "Parallel moderation now applies to every v2 config, not just the enrolled three.", + discussion: + "Parallel moderation now applies to every v2 config, not just the enrolled three. `shouldModerateDuringMainCompletion` gates on the moderation modifier alone, so the rollout is no longer scoped to the experiment.", + failure_scenario: + "An unenrolled config regresses without ever having been measured.", + }), + claim({ + id: "first-principles-1", + source: "first-principles", + path: "services/ai-guide/chat/ask/v2/moderation_helpers.go", + line: 45, + label: "question (non-blocking)", + subject: + "112 of 151 v2 configs declare pre_flight_moderation_check; was a staged rollout considered?", + discussion: + "112 of 151 v2 configs declare pre_flight_moderation_check; was a staged rollout considered? The cost profile on high-flag-rate configs outside the experiment is unmeasured.", + failure_scenario: + "A config with a different traffic shape hits a spend regression the three-config experiment could not surface.", + }), + ], + [ + { + evidence: + "`shouldModerateDuringMainCompletion` now returns true for every v2 config that declares `PreFlightModerationCheck`", + ids: ["holistic-1", "first-principles-1"], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual(["holistic-1"]); + expect(merges).toEqual([ + { + survivor: "holistic-1", + merged: [ + { + id: "first-principles-1", + source: "first-principles", + label: "question (non-blocking)", + line: 45, + via: "clusterer", + groundedBy: "evidence", + }, + ], + path: "services/ai-guide/chat/ask/v2/moderation_helpers.go", + line: 31, + via: "clusterer", + evidence: + "`shouldModerateDuringMainCompletion` now returns true for every v2 config that declares `PreFlightModerationCheck`", + }, + ]); + expect(clusterRejections).toEqual([]); + }); +}); + +/** + * What the anchor path gives up, pinned so the trade stays recorded rather + * than rediscovered. Modeled on run 30587343777's counterexample (the fixture + * in dedup-cluster.test.ts): the cap survivor names `staleAfter` in a + * while-here aside, and `documentation-2` is the genuinely distinct + * staleAfter finding that used to be saved from a bad proposal by the + * vocabulary tripwire alone. At its real anchor (:11, three lines off the + * survivor) it still is — the "drops a proposed member whose own text never + * names the shared evidence" test holds that line. Re-anchored to the + * survivor's own :8, no check runs at all: the model's identity assertion is + * taken at its word, the advisory finding folds in, and its subject survives + * only inside also_flagged_by. The bound is the tier's risk grading — only a + * non-blocking copy can be lost this way, and only one the model itself + * proposed. + */ +describe("the anchor path's accepted cost", () => { + it("absorbs a distinct non-blocking finding sitting on the survivor's exact line", () => { + const {claims, merges, clusterRejections} = dedupeClaims( + [ + 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", + }), + claim({ + id: "documentation-2", + source: "documentation", + path: "dev/af19_trial/window.go", + line: 8, + 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.", + }), + ], + [ + { + evidence: + "the `maxSamples` comment claims a cap of 10, not 25", + ids: ["correctness-reviewer-3", "documentation-2"], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual(["correctness-reviewer-3"]); + expect(claims[0].also_flagged_by).toEqual([ + { + source: "documentation", + subject: "Comment restates the constant.", + }, + ]); + expect(merges).toEqual([ + { + survivor: "correctness-reviewer-3", + merged: [ + { + id: "documentation-2", + source: "documentation", + label: "suggestion (non-blocking, documentation)", + via: "clusterer", + groundedBy: "anchor", + }, + ], + path: "dev/af19_trial/window.go", + line: 8, + via: "clusterer", + evidence: "the `maxSamples` comment claims a cap of 10, not 25", + }, + ]); + expect(clusterRejections).toEqual([]); + }); + + it("grounds on the anchor against a tier-1 head the clusterer never named", () => { + // The survivor can be a claim the proposal never saw: tier 1 bridges + // the named member into a higher-severity head, and the member is + // re-checked against THAT claim. The identity chain still holds by + // transitivity (the clusterer asserted member ≡ named claim, tier 1's + // text floor asserted named claim ≡ head), so the anchor grounds here + // too, even though the head never names the evidence, which is + // exactly the survivor-end failure the vocabulary path would veto + // (dedup-cluster.test.ts pins that veto for a member OFF the line). + 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: 8, + 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"], + }, + ], + ); + expect(claims.map((c) => c.id)).toEqual(["holistic-1"]); + expect(merges).toEqual([ + { + survivor: "holistic-1", + merged: [ + { + id: "correctness-reviewer-3", + source: "correctness-reviewer", + label: "note (non-blocking)", + }, + { + id: "documentation-1", + source: "documentation", + label: "suggestion (non-blocking, documentation)", + via: "clusterer", + groundedBy: "anchor", + }, + ], + path: "dev/af19_trial/window.go", + line: 8, + via: "both", + evidence: "the `maxSamples` cap", + }, + ]); + expect(clusterRejections).toEqual([]); + }); +}); diff --git a/workflows/review/lib/dedup-cluster.test.ts b/workflows/review/lib/dedup-cluster.test.ts index d065c439..3f4c0db7 100644 --- a/workflows/review/lib/dedup-cluster.test.ts +++ b/workflows/review/lib/dedup-cluster.test.ts @@ -200,18 +200,21 @@ describe("dedupeClaims with model-proposed clusters", () => { label: "question (non-blocking)", line: 9, via: "clusterer", + groundedBy: "evidence", }, { id: "conventions-1", source: "conventions", label: "nitpick (non-blocking)", via: "clusterer", + groundedBy: "anchor", }, { id: "documentation-1", source: "documentation", label: "suggestion (non-blocking, documentation)", via: "clusterer", + groundedBy: "anchor", }, ], path: "dev/af19_trial/window.go", @@ -281,26 +284,59 @@ describe("dedupeClaims with model-proposed clusters", () => { ]); }); - 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(), [ + it("holds a member off the survivor's line to the evidence, and inert evidence fails it", () => { + // An identity claim this module cannot check on either path is inert, + // not authoritative: "they are all about comments" grounds nothing, so + // the :9 member falls back to tier 1 and stays its own comment. The + // two members ON the survivor's line 8 no longer need the evidence at + // all; the exactly shared anchor is the grounding (and the run's own + // autofix discharged all four asks with one rewritten comment, so the + // merge is the true outcome, not a concession). + const {claims, merges, 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.map((c) => c.id)).toEqual([ + "correctness-reviewer-3", + "skill-auditor-ool-2", + ]); + expect(merges).toEqual([ { - evidence: "these are all about the same comment", - ids: [ - "correctness-reviewer-3", - "skill-auditor-ool-2", - "conventions-1", - "documentation-1", + survivor: "correctness-reviewer-3", + merged: [ + { + id: "conventions-1", + source: "conventions", + label: "nitpick (non-blocking)", + via: "clusterer", + groundedBy: "anchor", + }, + { + id: "documentation-1", + source: "documentation", + label: "suggestion (non-blocking, documentation)", + via: "clusterer", + groundedBy: "anchor", + }, ], + path: "dev/af19_trial/window.go", + line: 8, + via: "clusterer", + evidence: "these are all about the same comment", }, ]); - expect(claims).toHaveLength(4); expect(clusterRejections).toEqual([ {id: "skill-auditor-ool-2", reason: "ungrounded"}, - {id: "conventions-1", reason: "ungrounded"}, - {id: "documentation-1", reason: "ungrounded"}, ]); }); @@ -547,13 +583,15 @@ describe("dedupeClaims with model-proposed clusters", () => { 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. + // Out-ranks both on confidence, worded too thinly for the + // text floor, and anchored one line off the copies so the + // shared-anchor grounding path stays out of this fixture: it + // is a cluster member and nothing else. claim({ ...note, id: "holistic-1", source: "holistic", - line: 8, + line: 9, confidence: 0.9, subject: "the per-key cap disagrees with `maxSamples`", discussion: "the per-key cap disagrees with `maxSamples`", @@ -640,6 +678,7 @@ describe("dedupeClaims with model-proposed clusters", () => { source: "documentation", label: "note (non-blocking)", via: "clusterer", + groundedBy: "anchor", }, { id: "conventions-1", @@ -924,6 +963,7 @@ describe("dedupeClaims with model-proposed clusters", () => { label: "note (non-blocking)", line: 58, via: "clusterer", + groundedBy: "evidence", }, ]); // The record quotes only the copy tier 2 brought: the survivor's own diff --git a/workflows/review/lib/dedup-cluster.ts b/workflows/review/lib/dedup-cluster.ts index ac85465c..bf36258a 100644 --- a/workflows/review/lib/dedup-cluster.ts +++ b/workflows/review/lib/dedup-cluster.ts @@ -79,12 +79,30 @@ const isSalientToken = (raw: string): boolean => raw.includes("_") || /^\d{2,}$/.test(raw); -/** The code-naming tokens in a text, lowercased for comparison. */ +/** + * One canonical form per code name: lowercased with underscores folded out, so + * `PreFlightModerationCheck` and `pre_flight_moderation_check` read as one + * token. Run 32390393344 (webapp#41609) is why: its two claims named the same + * config key in Go casing and JSON casing, and a comparison keyed on casing + * style tests how a name was spelled, not what it names. Salience is still + * judged on the RAW spelling above (the underscore or the interior case change + * is often the evidence of code-ness that folding would erase). + */ +const canonicalToken = (raw: string): string => + raw.toLowerCase().replace(/_/g, ""); + +/** The code-naming tokens in a text, canonicalized 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()); + const canonical = canonicalToken(raw); + // A token of underscores alone (`_`, `__`) is salient on its raw + // spelling but folds to the empty string, which would then ground + // any two claims that each quote one (e.g. Go's blank identifier). + if (canonical !== "") { + tokens.add(canonical); + } } } return tokens; @@ -94,7 +112,7 @@ export const salientTokens = (text: string): Set => { const claimText = (claim: Claim): string => `${claim.subject} ${claim.discussion} ${claim.failure_scenario}`; -export const sharesSalientToken = ( +const sharesSalientToken = ( evidenceTokens: ReadonlySet, claim: Claim, ): boolean => { @@ -128,8 +146,9 @@ export const sharesSalientToken = ( * 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. + * Deliberately absent: any line REQUIREMENT, and any text-similarity floor. + * Both are what tier 2 exists to get past. (An exactly shared line does count + * FOR a member, as grounding; see {@link clusterMemberRejection}.) */ const structuralRejection = ( reference: Claim, @@ -153,19 +172,51 @@ const structuralRejection = ( * 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. + * - **grounded**, by either of two paths: + * + * An exactly shared anchor: the member sits on the survivor's own line (the + * paths already match structurally). A cross-source pair on the identical + * line is the one identity assertion this module can verify without any + * vocabulary at all, and the vocabulary path cannot be the only one: the + * evidence is model prose with free word choice, and run 32390393344 + * (webapp#41609) showed it grading the clusterer's phrasing rather than the + * claims. There the clusterer correctly grouped two same-line copies of one + * finding, wrote its evidence in the hunk's identifiers + * (`_configIncludesModeration`), and both claims spoke config-side + * (`pre_flight_moderation_check`), zero shared tokens, true merge vetoed. + * Only claims the model PROPOSED reach this check, so a same-line neighbour + * it never named cannot ride in on its anchor. + * + * Failing that, the vocabulary tripwire as before: the cluster's evidence + * must name at least one code element ({@link isSalientToken}), the SURVIVOR + * must mention one (tier 1 can have elected a comment the proposal never + * saw, and absorbing a member into an unrelated comment is the failure mode + * this end catches), and the member must mention one too. 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. Grounding + * the member against the survivor's own text instead would NOT be safe: + * run 30587343777's cap survivor names `staleAfter` in a while-here aside, + * and the distinct staleAfter finding would ground against it (the pinned + * counterexample in dedup-cluster.test.ts). */ export const clusterMemberRejection = ( survivor: Claim, member: Claim, evidenceTokens: ReadonlySet, -): ClusterRejection["reason"] | undefined => - structuralRejection(survivor, member) ?? - (sharesSalientToken(evidenceTokens, member) ? undefined : "ungrounded"); +): ClusterRejection["reason"] | undefined => { + const structural = structuralRejection(survivor, member); + if (structural !== undefined) { + return structural; + } + if (member.line !== undefined && member.line === survivor.line) { + return undefined; + } + const evidenceUsable = + evidenceTokens.size > 0 && sharesSalientToken(evidenceTokens, survivor); + return evidenceUsable && sharesSalientToken(evidenceTokens, member) + ? undefined + : "ungrounded"; +}; /** * Hold one proposal's members to the structural rules at parse time. diff --git a/workflows/review/lib/dedup.ts b/workflows/review/lib/dedup.ts index dec1dcb7..2f172fdf 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -88,7 +88,8 @@ * 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 + * text unless the member sits on the survivor's exact line, 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. */ @@ -100,7 +101,6 @@ import {isBlockingLabel} from "./render-comment"; import { clusterMemberRejection, salientTokens, - sharesSalientToken, verifiableClusters, type ClusterRejection, } from "./dedup-cluster"; @@ -129,6 +129,15 @@ export type ClaimMerge = { * tier 1's members to the clusterer. */ via?: "clusterer"; + /** + * Which path grounded a clusterer-absorbed copy: the exactly shared + * anchor, or the evidence's vocabulary. Present exactly when `via` + * is. The planned audit of "ungrounded" rejections reads + * dispatch-result.json, and without this field an anchor-grounded + * merge is indistinguishable there from an evidence-grounded one + * (the stamped evidence string may have contributed nothing). + */ + groundedBy?: "anchor" | "evidence"; }[]; path: string; line: number; @@ -702,7 +711,14 @@ export const dedupeClaims = ( /** 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(); + const absorbed = new Map< + number, + { + index: number; + via?: "clusterer"; + groundedBy?: "anchor" | "evidence"; + }[] + >(); /** Survivor index -> the clusterer's grounding evidence, when tier 2 fired. */ const groundedIn = new Map(); @@ -798,15 +814,14 @@ export const dedupeClaims = ( 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. + // The grounding rules (both ends of the vocabulary check, and the + // shared-anchor path that needs no vocabulary) live in + // clusterMemberRejection, per member: the survivor-end test cannot sit + // out here as a group-level gate or it would veto a member the anchor + // path grounds (run 32390393344's pair, where the survivor shares no + // token with the evidence and the member sits on its exact line). 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) { @@ -816,13 +831,11 @@ export const dedupeClaims = ( // 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, - ); + const reason = clusterMemberRejection( + survivor, + claims[index], + evidenceTokens, + ); if (reason !== undefined) { for (const id of namedByHead.get(index) ?? []) { clusterRejections.push({id, reason}); @@ -833,7 +846,18 @@ export const dedupeClaims = ( // 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"}); + // Mirrors the first grounding test in clusterMemberRejection: a + // member on the survivor's exact line was admitted by the anchor + // before any vocabulary ran, everything else by the evidence. + into.push({ + index, + via: "clusterer", + groundedBy: + claims[index].line !== undefined && + claims[index].line === survivor.line + ? "anchor" + : "evidence", + }); into.push(...(absorbed.get(index) ?? [])); absorbed.delete(index); groundedIn.set(survivorIndex, groupEvidence); @@ -940,7 +964,7 @@ export const dedupeClaims = ( const groupEvidence = groundedIn.get(survivorIndex); merges.push({ survivor: survivor.id, - merged: others.map(({index, via: copyVia}) => { + merged: others.map(({index, via: copyVia, groundedBy}) => { const claim = claims[index]; return { id: claim.id, @@ -950,6 +974,7 @@ export const dedupeClaims = ( ? {line: claim.line} : {}), ...(copyVia === "clusterer" ? {via: copyVia} : {}), + ...(groundedBy === undefined ? {} : {groundedBy}), }; }), path: survivor.path as string, diff --git a/workflows/review/review.md b/workflows/review/review.md index 0195a83d..67b2d7bf 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -1766,9 +1766,9 @@ proximity: **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). +25"). The pipeline checks this: a group whose evidence names no code element, or a +member whose own text never mentions it, is normally discarded. So write evidence that +quotes the code, never a topic ("both are about comments" grounds nothing). **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