review: adjudicated-corpus suppression drops its same-path key

.changeset/pra11-adjudicated-crossfile.md

@@ -0,0 +1,5 @@
+---
+"review": minor
+---
+
+The adjudicated-corpus suppression drops its same-path key: a human-settled defect's rephrasing routinely re-anchors on another file (the spec instead of the implementation, the test instead of the function), and the path key is what let the webapp#41290 duplicate families re-post for two weeks after the author had adjudicated them. Measured on that frozen corpus (12 adjudicated threads, 33 labeled candidates, kept privately in the planning tree), dropping the key tripled recall (2/12 to 6/12 true variants suppressed) and added zero false suppressions (both variants make the same single mistake, folding two distinct same-file findings whose wording shares the file's vocabulary). The open-thread corpus stays path-keyed, blocking candidates are still never suppressed here, an adjudicated thread staged without a usable path stays inert rather than becoming a PR-wide matcher, and every other #332 fail-closed guard is unchanged.

workflows/review/README.md

@@ -142,7 +142,9 @@ you can pick the one that says what you mean:
   (and blocking, if it was blocking).
 - **Resolve the thread.** "This is settled." The thread leaves the
   accountability recap, and the defect joins the adjudicated corpus: a later
-  run that re-derives the same defect (any wording, any nearby line) posts
+  run that re-derives the same defect (any wording, any line, and unlike the
+  open-thread corpus any file, since a settled defect's rephrasing often
+  re-anchors on the spec or the test rather than the implementation) posts
   nothing, unless it comes back at BLOCKING severity, which always posts (a
   regression worth stopping the PR for must never be silenced by an old
   resolution). Threads the bot resolved itself (because a push fixed them)

workflows/review/lib/attribution.ts

@@ -104,7 +104,7 @@ const FOOTER_BLOCK_RE = new RegExp(
 
 /**
  * Drop footer boilerplate from a previously-posted bot comment before
- * text-similarity comparison (dedup.ts's `threadProse`). Every posted
+ * text-similarity comparison (dedup-threads.ts's `threadProse`). Every posted
  * comment carries the same summary chip, `found by <source>` prefix, and
  * version segments; tokens shared by ALL bot comments would inflate
  * similarity between unrelated findings, exactly like the label template

workflows/review/lib/dedup-adjudicated.test.ts

@@ -5,6 +5,7 @@ import {
     suppressAdjudicatedDuplicates,
     suppressTrackedDuplicates,
 } from "./dedup-adjudicated";
+import {suppressOpenThreadDuplicates} from "./dedup-threads";
 import type {Claim} from "./dispatch-contracts";
 
 /**
@@ -238,3 +239,153 @@ describe("suppressAdjudicatedDuplicates", () => {
         expect(result.shapeFailure).toBeUndefined();
     });
 });
+
+describe("cross-file adjudicated suppression (the path key dropped)", () => {
+    const adjudicatedThread = () => ({
+        thread_id: "T-adj",
+        path: "services/ai-guide/memory/expiration.go",
+        body: "**suggestion (non-blocking):** No test exercises the deletion path: TestExpiration only asserts that expired keys are identified, so a regression that identifies but never deletes expired memories stays green.",
+    });
+    const crossFile = (over: Partial<Claim> = {}) =>
+        claim({
+            id: "correctness-reviewer-9",
+            source: "correctness-reviewer",
+            path: "services/ai-guide/memory/expiration_test.go",
+            line: 7,
+            label: "suggestion (non-blocking)",
+            subject:
+                "TestExpiration never exercises the deletion path for expired keys.",
+            discussion:
+                "No test exercises the deletion path; TestExpiration asserts expired keys are identified but a regression that never deletes expired memories stays green.",
+            failure_scenario:
+                "A regression that identifies expired memories but skips the deletion ships green.",
+            ...over,
+        });
+
+    it("suppresses a re-derivation re-anchored on another file", () => {
+        // The webapp#41290 failure mode: the same settled defect re-posted
+        // at the spec, the implementation, and the test across runs, and the
+        // path key exempted every re-anchoring from the corpus. Measured on
+        // that frozen corpus (see suppressAdjudicatedDuplicates's doc),
+        // dropping the key tripled recall (2/12 to 6/12) and added zero
+        // false suppressions.
+        const {kept, suppressed} = suppressAdjudicatedDuplicates(
+            [crossFile()],
+            [adjudicatedThread()],
+        );
+        expect(kept).toEqual([]);
+        expect(suppressed).toHaveLength(1);
+        expect(suppressed[0].thread_id).toBe("T-adj");
+        expect(suppressed[0].adjudicated).toBe(true);
+        expect(suppressed[0].path).toBe(
+            "services/ai-guide/memory/expiration_test.go",
+        );
+    });
+
+    it("still never suppresses a blocking candidate, cross-file included", () => {
+        const blocking = crossFile({label: "issue (blocking)"});
+        const {kept, suppressed} = suppressAdjudicatedDuplicates(
+            [blocking],
+            [adjudicatedThread()],
+        );
+        expect(kept).toEqual([blocking]);
+        expect(suppressed).toEqual([]);
+    });
+
+    it("pays OTHER_LINE_FLOOR cross-file, not the pr-level tier", () => {
+        // This fixture scores 7 shared bigrams against T-adj (jaccard 0.481,
+        // overlap 0.722, via the real tokenizer): inside the band where
+        // OTHER_LINE_FLOOR (>=6) and PR_LEVEL_FLOOR (>=8) disagree, so it
+        // pins the documented floor choice rather than clearing both. The
+        // weakest true cross-file match on the frozen 41290 corpus sits at
+        // exactly 7; a floor of 8 would lose it for no measured precision
+        // (see bestOpenThreadMatch's calibration note).
+        const marginal = crossFile({
+            subject: "The deletion path for expired keys is untested.",
+            discussion:
+                "TestExpiration stops at identification; nothing checks the memories are actually removed afterwards.",
+            failure_scenario:
+                "A regression that identifies expired memories but never deletes them stays green.",
+        });
+        const {kept, suppressed} = suppressAdjudicatedDuplicates(
+            [marginal],
+            [adjudicatedThread()],
+        );
+        expect(kept).toEqual([]);
+        expect(suppressed).toHaveLength(1);
+    });
+
+    it("keeps a cross-file hard negative: shared vocabulary alone does not clear the jaccard guard", () => {
+        // The precision half of dropping the path key. This candidate is a
+        // DIFFERENT defect (the sweep holds the lock across the deletion
+        // pass, not a missing test) that reuses the thread's vocabulary
+        // heavily enough to clear the other two floors: 6 shared bigrams
+        // (exactly OTHER_LINE_FLOOR's 6) and overlap 0.444 against the 0.35
+        // floor, with jaccard 0.157 against 0.2, via the real tokenizer.
+        // Only jaccard rejects it, which is the guard the frozen 41290
+        // corpus measured at 0.168 on its strongest cross-file negative
+        // (see bestOpenThreadMatch's calibration note); a false suppression
+        // here drops a finding with no trace, so this pins the floor's
+        // precision side the way the marginal fixture above pins recall.
+        const negative = crossFile({
+            subject:
+                "The expiration sweep holds the write lock for the whole deletion pass.",
+            discussion:
+                "Expire acquires the global mutex once and walks every shard under it; the deletion path never deletes expired memories individually, so the expired keys identified by the scan are removed while readers block on the same mutex.",
+            failure_scenario:
+                "A large batch of expired entries stalls every concurrent reader of the memory store until the sweep's pass finishes.",
+        });
+        const {kept, suppressed} = suppressAdjudicatedDuplicates(
+            [negative],
+            [adjudicatedThread()],
+        );
+        expect(kept).toEqual([negative]);
+        expect(suppressed).toEqual([]);
+    });
+
+    it("picks the best-scoring adjudicated thread across files, independent of staging order", () => {
+        // Both corpus members clear the floor against the candidate (the
+        // discursive SPEC.md thread scores jaccard 0.467, the true
+        // counterpart 0.85), so ranking, not the floor, decides attribution.
+        const weaker = {
+            thread_id: "T-spec",
+            path: "services/ai-guide/memory/spec/SPEC.md",
+            body: "**note (non-blocking):** The spec promises that expired memories are deleted, and TestExpiration exercises only the identification half: expired keys are asserted as identified, deletion is never checked, so a regression that identifies but never deletes expired memories stays green and the spec's deletion promise goes untested.",
+        };
+        const both = [weaker, adjudicatedThread()];
+        const {suppressed} = suppressAdjudicatedDuplicates([crossFile()], both);
+        expect(suppressed[0].thread_id).toBe("T-adj");
+        expect(
+            suppressAdjudicatedDuplicates([crossFile()], [...both].reverse())
+                .suppressed[0],
+        ).toEqual(suppressed[0]);
+    });
+
+    it("an adjudicated thread staged without a usable path suppresses nothing", () => {
+        // Under the path key an anchorless corpus member could never match a
+        // pathed claim (openThreadsFromStaged calls that degradation
+        // fail-closed); dropping the claim-side key must not flip it into a
+        // PR-wide wildcard, so the thread side keeps its gate.
+        for (const path of [undefined, ""]) {
+            const anchorless = {...adjudicatedThread(), path};
+            const {kept, suppressed} = suppressAdjudicatedDuplicates(
+                [crossFile()],
+                [anchorless],
+            );
+            expect(kept).toHaveLength(1);
+            expect(suppressed).toEqual([]);
+        }
+    });
+
+    it("leaves the OPEN corpus path-keyed: the same cross-file pair does not suppress there", () => {
+        // The asymmetry is the point: on the open corpus a false cross-file
+        // match hides an UNDECIDED finding, so its matcher keeps the path
+        // key; only human-settled threads earn the wide match.
+        const {kept, suppressed} = suppressOpenThreadDuplicates(
+            [crossFile()],
+            [adjudicatedThread()],
+        );
+        expect(kept).toHaveLength(1);
+        expect(suppressed).toEqual([]);
+    });
+});

workflows/review/lib/dedup-adjudicated.ts

@@ -36,7 +36,7 @@ import {
     threadOpenerIsBlocking,
     type OpenThread,
     type ThreadSuppression,
-} from "./dedup";
+} from "./dedup-threads";
 import {isRecord, type Claim} from "./dispatch-contracts";
 import {isBlockingLabel} from "./render-comment";
 import {isReviewBotAuthor} from "./threads";
@@ -119,10 +119,36 @@ export const adjudicatedThreadsFromStaged = (threads: unknown): OpenThread[] =>
  *   audit trail, but submission.ts's floor requires the CANDIDATE to be
  *   blocking, which no claim suppressed here is.
  *
- * The match itself is the shared one (same path, no line window, the #245
- * similarity floors via {@link bestOpenThreadMatch}): an adjudicated defect's
- * rephrasing lands on nearby lines with new wording exactly the way a
- * persisting open defect's re-flag does.
+ * The match is the shared matcher with the PATH KEY DROPPED (`ignorePath`;
+ * same #245 similarity floors via {@link bestOpenThreadMatch}, whose doc
+ * carries the cross-file floor calibration): an adjudicated defect's
+ * rephrasing routinely re-anchors on another file (the spec instead of the
+ * implementation, the test instead of the function), and the path key is
+ * what let the webapp#41290 families re-post for two weeks. Measured on that
+ * frozen corpus (Khan/plans,
+ * pr-review-agent/records/family-corpus-41290.json: 12 adjudicated threads,
+ * 33 labeled candidates): path-keyed scores 2/12 recall, key dropped scores
+ * 6/12 with correct family attribution on every match, and BOTH make the
+ * same single false suppression (folding two distinct same-path findings
+ * whose wording shares the file's vocabulary), so the widening tripled
+ * recall and added zero false suppressions there.
+ *
+ * What licenses the reach is the corpus's membership rule plus the blocking
+ * exemption, not a claim that false matches are free: a false match drops a
+ * non-blocking finding whose text sits within the floors of one a human
+ * explicitly settled, and a defect that matters enough to block re-presents
+ * at blocking severity and posts. The reach deliberately includes the
+ * sibling-copy shape (webapp#41440: one source stamping near-identical text
+ * across sibling files): with a settled thread on file A, file B's copy now
+ * exits through the same thread instead of posting alone
+ * (dedup-crossfile.ts documents the interaction with its merge ordering).
+ * The human declined that exact ask once, and the path key never protected
+ * the matching same-file case (a fresh same-file instance of a settled
+ * defect was already suppressed), so this widens the reach of an accepted
+ * risk rather than adding a new class. The OPEN corpus keeps its path key:
+ * its members carry no human judgment, so a false cross-file match there
+ * would hide an undecided finding on nothing but the bot's own earlier
+ * text.
  */
 export const suppressAdjudicatedDuplicates = (
     claims: Claim[],
@@ -137,7 +163,7 @@ export const suppressAdjudicatedDuplicates = (
         const match =
             claim.path === undefined || isBlockingLabel(claim.label)
                 ? undefined
-                : bestOpenThreadMatch(claim, threads);
+                : bestOpenThreadMatch(claim, threads, {ignorePath: true});
         if (match === undefined) {
             kept.push(claim);
             continue;

workflows/review/lib/dedup-cluster.ts

@@ -41,7 +41,7 @@ import {isBlockingLabel} from "./render-comment";
  * One member a proposed cluster named that did NOT merge, with the rule that
  * rejected it. Recorded per run because an empty rejection list and an empty
  * proposal list mean opposite things, and the module has already been burned
- * by that ambiguity once (see `dedup.ts`'s `stagedThreadShapeFailure`):
+ * by that ambiguity once (see `dedup-threads.ts`'s `stagedThreadShapeFailure`):
  * a clusterer naming ids that do not exist is a prompt or staging failure, and
  * it must not read as "no duplicates found".
  */

workflows/review/lib/dedup-crossfile.test.ts

@@ -353,6 +353,69 @@ describe("suppressThenMergeCrossFile", () => {
         expect(result.crossFileMerges).toHaveLength(1);
     });
 
+    const staged = (over: Record<string, unknown>) => [
+        {
+            thread_id: "T-1",
+            path: EXERCISE,
+            comments: [
+                {
+                    author: "github-actions",
+                    body: `**suggestion (non-blocking, documentation):** ${
+                        claim({}).subject
+                    } ${claim({}).discussion}`,
+                },
+            ],
+            ...over,
+        },
+    ];
+
+    it("open corpus: a tracked file's copy exits through its thread and the sibling posts alone", () => {
+        // The ordering rationale in mergeCrossFileDuplicates's doc, pinned:
+        // the open matcher is path-keyed, so the thread on EXERCISE takes
+        // its own file's copy and the TUTOR_ME occurrence still posts.
+        const result = suppressThenMergeCrossFile(
+            pair(),
+            staged({resolved: false}),
+            [],
+            new Set(),
+            [EXERCISE, TUTOR_ME],
+        );
+        expect(result.claims.map((kept) => kept.id)).toEqual([
+            "documentation-2",
+        ]);
+        expect(result.suppressed).toHaveLength(1);
+        expect(result.suppressed[0].id).toBe("documentation-1");
+        expect(result.suppressed[0].adjudicated).toBeUndefined();
+        expect(result.crossFileMerges).toEqual([]);
+    });
+
+    it("adjudicated corpus: a settled thread takes the near-identical sibling copy too", () => {
+        // The deliberate contrast with the open-corpus case above: the
+        // adjudicated matcher drops the path key, so both stamped copies of
+        // the human-settled non-blocking ask exit through the one thread and
+        // nothing reaches the merge. A blocking re-presentation would post
+        // (dedup-adjudicated.test.ts pins that exemption).
+        const result = suppressThenMergeCrossFile(
+            pair(),
+            [],
+            staged({resolved: true, resolvedBy: "sxkosone"}),
+            new Set(),
+            [EXERCISE, TUTOR_ME],
+        );
+        expect(result.claims).toEqual([]);
+        expect(result.suppressed).toHaveLength(2);
+        expect(
+            result.suppressed.map((entry) => ({
+                id: entry.id,
+                adjudicated: entry.adjudicated,
+            })),
+        ).toEqual([
+            {id: "documentation-1", adjudicated: true},
+            {id: "documentation-2", adjudicated: true},
+        ]);
+        expect(result.crossFileMerges).toEqual([]);
+    });
+
     it("re-applies the occurrence list a corrected discussion erased", () => {
         const merged = mergeCrossFileDuplicates(pair(), [EXERCISE, TUTOR_ME]);
         const corrected = merged.claims.map((kept) => ({