diff --git a/.changeset/pra11-adjudicated-crossfile.md b/.changeset/pra11-adjudicated-crossfile.md new file mode 100644 index 00000000..af502550 --- /dev/null +++ b/.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. diff --git a/workflows/review/README.md b/workflows/review/README.md index 3d6818e1..6e413ef5 100644 --- a/workflows/review/README.md +++ b/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) diff --git a/workflows/review/lib/attribution.ts b/workflows/review/lib/attribution.ts index a1d8b107..2d79645e 100644 --- a/workflows/review/lib/attribution.ts +++ b/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 ` prefix, and * version segments; tokens shared by ALL bot comments would inflate * similarity between unrelated findings, exactly like the label template diff --git a/workflows/review/lib/dedup-adjudicated.test.ts b/workflows/review/lib/dedup-adjudicated.test.ts index 8ecbcbf6..ccd38a69 100644 --- a/workflows/review/lib/dedup-adjudicated.test.ts +++ b/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({ + 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([]); + }); +}); diff --git a/workflows/review/lib/dedup-adjudicated.ts b/workflows/review/lib/dedup-adjudicated.ts index 34d6ef3f..8cc2deed 100644 --- a/workflows/review/lib/dedup-adjudicated.ts +++ b/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; diff --git a/workflows/review/lib/dedup-cluster.ts b/workflows/review/lib/dedup-cluster.ts index bf36258a..ba2fb93b 100644 --- a/workflows/review/lib/dedup-cluster.ts +++ b/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". */ diff --git a/workflows/review/lib/dedup-crossfile.test.ts b/workflows/review/lib/dedup-crossfile.test.ts index b7f8aa82..0289255e 100644 --- a/workflows/review/lib/dedup-crossfile.test.ts +++ b/workflows/review/lib/dedup-crossfile.test.ts @@ -353,6 +353,69 @@ describe("suppressThenMergeCrossFile", () => { expect(result.crossFileMerges).toHaveLength(1); }); + const staged = (over: Record) => [ + { + 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) => ({ diff --git a/workflows/review/lib/dedup-crossfile.ts b/workflows/review/lib/dedup-crossfile.ts index a68de3e7..ba981085 100644 --- a/workflows/review/lib/dedup-crossfile.ts +++ b/workflows/review/lib/dedup-crossfile.ts @@ -56,11 +56,11 @@ * dispatch-result.json like the other tiers' merges. */ +import {describesSameDefect} from "./dedup"; import { - describesSameDefect, type stagedThreadShapeFailure, type ThreadSuppression, -} from "./dedup"; +} from "./dedup-threads"; import {suppressTrackedDuplicates} from "./dedup-adjudicated"; import {isRecord, type Claim} from "./dispatch-contracts"; @@ -126,16 +126,23 @@ const mergeableAcrossFiles = (a: Claim, b: Claim): boolean => * validated nor separately posted (suppression also precedes validation, so * the cost saving is identical). * - * The position after suppression is load-bearing, not stylistic. - * `bestOpenThreadMatch` only matches a thread to a claim on the thread's own - * path, so if this merge ran first, an open thread on the survivor's file - * would suppress the survivor and silently drop every other file's - * occurrence with it: an author who copies a flawed file A into a new + * The position after suppression is load-bearing, not stylistic. For the + * OPEN corpus `bestOpenThreadMatch` only matches a thread to a claim on the + * thread's own path, so if this merge ran first, an open thread on the + * survivor's file would suppress the survivor and silently drop every other + * file's occurrence with it: an author who copies a flawed file A into a new * sibling B, with A's finding already tracked in an open thread, would never * hear about B, on this run or any later one. Running after suppression, A's * copy exits through the thread and B posts alone. The inverse cost, an open * thread that was itself a merged comment already naming B, is one duplicate * comment on B: the failure direction this module's rules already prefer. + * The ADJUDICATED pass matches cross-file on purpose (`ignorePath`, + * dedup-adjudicated.ts), so there B's near-identical copy exits through A's + * settled thread too, before this merge ever sees the pair. That is that + * corpus's semantics (a human declined the same non-blocking ask, and a + * blocking re-presentation still posts), not a leak in this ordering, and + * the ordering stays load-bearing for the open corpus either way; + * dedup-crossfile.test.ts pins both directions. * * `pathOrder` is the diff's file order (staged files.json); the survivor is * the group's first occurrence in that order, with claim order breaking ties @@ -252,8 +259,10 @@ export const mergeCrossFileDuplicates = ( * The composed suppression-then-merge step dispatch calls, in this order * because the ordering is load-bearing (see {@link mergeCrossFileDuplicates}): * both thread-suppression passes first ({@link suppressTrackedDuplicates}: - * the open corpus, then the adjudicated one), so a suppressed file's copy - * exits through its thread and the other files' occurrences still post; the + * the open corpus, then the adjudicated one), so a file's copy suppressed by + * an OPEN thread exits through it and the other files' occurrences still + * post (an adjudicated thread reaches near-identical sibling copies too, + * deliberately; see {@link mergeCrossFileDuplicates}'s ordering note); the * cross-file merge second, over the survivors. `stagedOpen`, * `stagedAdjudicated`, and `stagedFiles` are the raw staged values; a * missing or malformed files.json degrades to claim order, never to a diff --git a/workflows/review/lib/dedup-pr-level.test.ts b/workflows/review/lib/dedup-pr-level.test.ts index aee250ec..3b105cee 100644 --- a/workflows/review/lib/dedup-pr-level.test.ts +++ b/workflows/review/lib/dedup-pr-level.test.ts @@ -1,6 +1,6 @@ import {describe, it, expect} from "vitest"; -import {suppressOpenThreadDuplicates} from "./dedup"; +import {suppressOpenThreadDuplicates} from "./dedup-threads"; import type {Claim} from "./dispatch-contracts"; /** diff --git a/workflows/review/lib/dedup-text.ts b/workflows/review/lib/dedup-text.ts index 58bb39ee..29e0433d 100644 --- a/workflows/review/lib/dedup-text.ts +++ b/workflows/review/lib/dedup-text.ts @@ -1,11 +1,12 @@ /** * The text-similarity primitives every dedup tier scores with: content * tokenization (lowercased alphanumerics, stopwords and short words - * dropped), token bigrams, and set intersection. Split from `dedup.ts` for - * its max-lines budget (the dedup-cluster.ts precedent); they are - * dependency-free and every floor in dedup.ts (same-line, other-line, - * pr-level) is calibrated against exactly these definitions, so a change - * here re-opens every calibration note there. + * dropped), token bigrams, and set intersection — plus the calibrated + * similarity floors, which are minimums over these exact definitions, so a + * change to either re-opens every calibration note. Split from `dedup.ts` + * for its max-lines budget (the dedup-cluster.ts precedent); the primitives + * are dependency-free, and dedup.ts (the merge tiers) and dedup-threads.ts + * (open-thread suppression) score with nothing else. */ const STOPWORDS = new Set( @@ -41,3 +42,51 @@ export const intersectionSize = (a: Set, b: Set): number => { } return count; }; + +/** + * Similarity floors, in two tiers. An identical `(path, line)` from two + * sources is itself evidence of one defect, so that tier's token floors sit + * just under the weakest real duplicate (run 30301235749's terse correctness + * one-liner against the discursive holistic copy: 0.149 Jaccard, 0.346 + * overlap) and the shared-bigram floor carries the precision alone. A + * DIFFERENT line is weak evidence of two defects, so that tier pays for the + * looser anchor with a higher bigram floor: run 30301235749 merged the + * skill-auditor's AddDate handoff at expiration.go:38 into the test-adequacy + * missing-test todo at :62 on five shared bigrams, and both real + * different-line duplicates (run 29943085279's expiration_test.go :15/:58 + * pair, and the bundled test-adequacy bridge) share six or more. + * + * Every floor is a minimum over real trial claims, so the margins are thin + * by construction: the exact-anchor tier separates on bigrams 4 vs 3, the + * other-line tier on 6 vs 5. Re-derive them from `dedup.test.ts`'s fixtures + * rather than nudging them by feel. + * + * OTHER_LINE_FLOOR carries a SECOND calibration since the adjudicated pass + * dropped its claim-side path key: it is also the cross-file floor for + * `bestOpenThreadMatch({ignorePath: true})` (dedup-threads.ts), calibrated + * on the frozen webapp#41290 family corpus, where the strongest cross-file + * negative scores jaccard 0.168 against the 0.2 floor and the weakest true + * match sits at exactly 7 shared bigrams. `dedup-adjudicated.test.ts` pins + * both edges (the 7-bigram marginal match and a jaccard-only hard negative), + * but a change to any number here must be re-derived against that corpus + * too; see `bestOpenThreadMatch`'s doc. + */ +export const EXACT_ANCHOR_FLOOR = { + jaccard: 0.14, + overlap: 0.34, + sharedBigrams: 4, +}; +export const OTHER_LINE_FLOOR = {jaccard: 0.2, overlap: 0.35, sharedBigrams: 6}; + +/** + * The floor for a claim with NO anchor at all (a pr-level finding) against + * an open thread: the least anchor evidence dedup-threads.ts scores, so it pays + * with the highest bigram floor, one tier above {@link OTHER_LINE_FLOOR}. + * Calibrated on webapp#41290 review 4867627688 (a pr-anchored re-find of a + * data race two open blocking threads tracked re-posted in full, because + * the path gate made pr-level claims unsuppressable): the true counterparts + * score 0.342/0.558/40 and 0.329/0.643/23 (jaccard/overlap/bigrams), the + * six unrelated open threads top out at 0.051/0.180/1. + * `dedup-pr-level.test.ts` carries the run's real texts; re-derive, don't nudge. + */ +export const PR_LEVEL_FLOOR = {jaccard: 0.2, overlap: 0.35, sharedBigrams: 8}; diff --git a/workflows/review/lib/dedup-threads.ts b/workflows/review/lib/dedup-threads.ts new file mode 100644 index 00000000..0d2884f4 --- /dev/null +++ b/workflows/review/lib/dedup-threads.ts @@ -0,0 +1,435 @@ +/** + * Open-thread suppression (trial suggestion g): drop candidate claims that + * describe a defect a still-open bot thread already tracks, so a re-review + * cannot re-post a finding whose thread the humans have not resolved yet. + * Split from `dedup.ts` for its max-lines budget (the dedup-text.ts + * precedent); dedup.ts owns the cross-source MERGE tiers, this module owns + * the thread-versus-candidate comparison, and the two share only the + * text-similarity primitives and calibrated floors in dedup-text.ts. + * + * `suppressOpenThreadDuplicates` is the entry point; dedup-adjudicated.ts + * reuses the matcher against the ADJUDICATED corpus (bot threads a human + * resolved) and carries the justification for that pass's differences. + */ + +import {stripFooters} from "./attribution"; +import { + bigrams, + contentTokens, + intersectionSize, + OTHER_LINE_FLOOR, + PR_LEVEL_FLOOR, +} from "./dedup-text"; +import {isRecord, type Claim} from "./dispatch-contracts"; +import {isBlockingLabel} from "./render-comment"; +// The identity of the review bot, shared with the producer that stages the +// threads this module filters (stage-pr.ts). `threads.ts` owns a GitHub fetch +// but runs nothing at import time and reaches the network only through an +// injected port, so dedup.ts's determinism boundary still holds here. +import {isReviewBotAuthor} from "./threads"; + +/** One still-open bot thread a candidate claim may duplicate. */ +export type OpenThread = { + thread_id: string; + path?: string; + /** The thread's opening comment body (the bot's original finding). */ + body: string; +}; + +export type ThreadSuppression = { + id: string; + source: string; + label: string; + path?: string; // absent for a pr-level claim (no anchor) + line?: number; + thread_id: string; + /** + * Whether the matched thread's OPENING comment carries a blocking label. + * The verdict floor keys on this, not on the candidate's own label: the + * thread's severity survived last run's validation, while a suppressed + * candidate is dropped before validation ever sees it. Read off the + * BEST-matching thread ({@link bestOpenThreadMatch}), since among several + * threads that clear the floor their labels can differ. + */ + threadBlocking: boolean; + /** + * Present (and true) when the matched thread is from the ADJUDICATED + * corpus (a bot thread a human resolved) rather than an open one; see + * dedup-adjudicated.ts. Audit-trail only: an adjudicated suppression can + * never floor the verdict, because that pass never suppresses a blocking + * candidate and submission.ts's floor requires the CANDIDATE's label to + * be blocking too. + */ + adjudicated?: true; +}; + +/** + * Whether an open thread's opener is a blocking finding, read from the + * leading `**label:**` template (tolerating the markdown-stripped form the + * staged bodies sometimes carry) and classified by {@link isBlockingLabel}, + * the single owner of that rule, so a label the taxonomy blocks on cannot be + * missed here (`issue (blocking, best-practice)` is a blocking label too, and + * a hand-rolled `\(blocking\)` match silently reads it as advisory). Spacing + * inside the parentheses is normalized before the lookup; a body with no + * recognizable label reads as non-blocking, since an unvalidated floor is the + * failure mode this guards. + */ +export const threadOpenerIsBlocking = (body: string): boolean => { + const label = /^\s*\*{0,2}([a-z]+ \([^)]*\))\*{0,2}:?/i.exec(body)?.[1]; + return ( + label !== undefined && + isBlockingLabel( + label + .toLowerCase() + .replace(/\s+/g, " ") + .replace(/\s*,\s*/g, ", "), + ) + ); +}; + +/** + * The prose of a previously-posted bot comment, for similarity comparison: + * the collapsed attribution/version footers (attribution.ts's strip), the + * leading `**label:**` template (tolerating the markdown-stripped form + * the staged bodies sometimes carry) and everything from the first code + * fence on (a suggestion block) are dropped, as are rule-quote lines: + * boilerplate shared by ALL bot comments would inflate similarity between + * unrelated findings. + */ +const threadProse = (body: string): string => + stripFooters(body) + .split("```")[0] + .split("\n") + .filter((line) => !line.trimStart().startsWith(">")) + .join(" ") + .replace(/^\s*\*{0,2}[a-z]+ \([^)]*\)\*{0,2}:?\*{0,2}\s*/i, ""); + +/** + * Whether a staged thread carries a "still open" state. `stage-pr.ts` writes + * `resolved: false` on every thread it stages; the `get_review_comments` + * spelling (`is_resolved`) and the camelCase form are still accepted, because + * a staging assembled from that tool's output (the eval's producers, and any + * hand-built staging used to reproduce a run) inherits its shape. Absent + * reads as unknown, not as open. + */ +export const stagedResolvedState = (thread: Record): unknown => + thread["resolved"] ?? thread["is_resolved"] ?? thread["isResolved"]; + +/** + * Build the suppression inputs from staged threads.json. `stage-pr.ts` is the + * producer (one GraphQL fetch, partitioned by opener), and it selects the + * bot's threads through the SAME {@link isReviewBotAuthor} predicate this + * filter admits them by, which is the point of the shared constant: the two + * layers spelling the identity separately is precisely how suppression became + * unreachable for a release (Khan/actions#302). The guards below nonetheless + * stay, because they are the properties suppression depends on and a producer + * bug must degrade to a duplicate comment rather than to a dropped finding: + * - the opener is the bot's, in either spelling. A human thread must never + * silently kill a candidate, and its free-text opener would also read as + * non-blocking and skip the verdict floor. + * - the thread is still open, per the staged `resolved` flag. An + * already-resolved thread would otherwise suppress a genuine regression + * re-flag with nothing to check it against. + * Threads in resolvedIds (reconciler-resolved this run) are exempt as well: a + * fixed defect posting again is a fresh finding. Fails closed on each: a + * thread without a bot-authored opener, or without an explicit + * `resolved: false`, never suppresses (worst case is a duplicate comment). + * + * `path` is read from the thread and falls back to the opening comment: the + * code producer carries `path` per thread, while `get_review_comments` carries + * it per comment, so a staging built from that tool inherits the other shape. + * The fallback is not cosmetic: {@link suppressOpenThreadDuplicates} matches + * on `path`, so a thread staged without one silently suppresses nothing. + */ +export const openThreadsFromStaged = ( + threads: unknown, + resolvedIds: ReadonlySet, +): OpenThread[] => + (Array.isArray(threads) ? threads : []) + .filter(isRecord) + .flatMap((thread) => { + const comments = thread["comments"]; + const opener = + Array.isArray(comments) && isRecord(comments[0]) + ? comments[0] + : undefined; + const author = opener?.["author"]; + if ( + typeof thread["thread_id"] !== "string" || + resolvedIds.has(thread["thread_id"]) || + stagedResolvedState(thread) !== false || + typeof author !== "string" || + !isReviewBotAuthor(author) + ) { + return []; + } + const path = + typeof thread["path"] === "string" + ? thread["path"] + : typeof opener?.["path"] === "string" + ? (opener["path"] as string) + : undefined; + return [ + { + thread_id: thread["thread_id"], + ...(path !== undefined ? {path} : {}), + body: + typeof opener["body"] === "string" + ? opener["body"] + : "", + }, + ]; + }); + +/** How strongly a claim's text matches one open thread's opener. */ +type OpenThreadScore = { + jaccard: number; + overlap: number; + sharedBigrams: number; +}; + +const openThreadScore = (claim: Claim, thread: OpenThread): OpenThreadScore => { + const tokensA = contentTokens( + `${claim.subject} ${claim.discussion} ${claim.failure_scenario}`, + ); + const tokensB = contentTokens(threadProse(thread.body)); + const setA = new Set(tokensA); + const setB = new Set(tokensB); + if (setA.size === 0 || setB.size === 0) { + return {jaccard: 0, overlap: 0, sharedBigrams: 0}; + } + const shared = intersectionSize(setA, setB); + return { + jaccard: shared / (setA.size + setB.size - shared), + overlap: shared / Math.min(setA.size, setB.size), + sharedBigrams: intersectionSize(bigrams(tokensA), bigrams(tokensB)), + }; +}; + +/** Whether a claim clearly describes the defect an open thread tracks. */ +export const describesOpenThreadDefect = ( + claim: Claim, + thread: OpenThread, +): boolean => { + const {jaccard, overlap, sharedBigrams} = openThreadScore(claim, thread); + // This match has no line window at all (see + // `suppressOpenThreadDuplicates`), so it pays for the loose anchor with + // the higher bigram floor, exactly as a cross-source pair on mismatched + // lines does; a pathless (pr-level) claim has no anchor evidence at all + // and pays one tier more. Erring strict is the safe direction: a + // missed suppression posts a duplicate, a false one drops a finding. + const floor = claim.path === undefined ? PR_LEVEL_FLOOR : OTHER_LINE_FLOOR; + return ( + jaccard >= floor.jaccard && + overlap >= floor.overlap && + sharedBigrams >= floor.sharedBigrams + ); +}; + +/** + * The open thread a candidate BEST matches, not merely the first one it clears + * the floor against. + * + * More than one open thread can describe the same area of a file, and taking + * the first match in staging order reads `threadBlocking` off a thread that is + * not the candidate's counterpart. Measured on webapp#41204 run 30650642317, + * the first live run where suppression fired: the fresh test-adequacy todo + * ("no test covers Record's documented zero-valued-Window guarantee") cleared + * the floor against BOTH the blocking nil-map panic thread and its own exact + * counterpart, a non-blocking nitpick whose opener reads "The documented + * zero-value-Window guarantee has no test". Staging order put the blocking + * thread first because it was the older one, so the suppression recorded + * `threadBlocking: true` and added a second entry to submission.ts's verdict + * floor. That run's verdict was already floored correctly by another + * suppression, so nothing was mis-decided, but the direction is the dangerous + * one: attributing a candidate to a blocking thread that is not its match + * forces REQUEST_CHANGES on thinner evidence than the both-sides-blocking rule + * intends. + * + * Ranked on `jaccard` first because it is the length-normalized metric: raw + * `sharedBigrams` grows with the opener's length, and `overlap` divides by the + * smaller token set, which favors the longer, more discursive thread. On the + * observed pair both jaccard (0.253 vs 0.218) and bigrams (13 vs 12) pick the + * true counterpart while overlap alone (0.468 vs 0.511) picks the wrong one, + * so bigrams break jaccard ties and overlap never ranks. Strictly-better + * comparisons keep staging order as the final tiebreak, so an exact scoring + * tie behaves as it did before. + * + * `ignorePath` drops the same-path key on the CLAIM side and keeps every + * floor as is. The ONLY caller is the adjudicated pass + * (dedup-adjudicated.ts, which carries the measured justification), and it + * exempts pathless claims before calling, so under `ignorePath` every claim + * that reaches here is pathed and pays {@link OTHER_LINE_FLOOR}; the + * {@link PR_LEVEL_FLOOR} branch is live only for the path-keyed callers. + * Keeping OTHER_LINE_FLOOR for the cross-file comparisons is itself + * measured, not reused by analogy: on the frozen webapp#41290 family corpus + * every cross-file pair that clears this floor is a true family match, the + * strongest cross-file NEGATIVE scores jaccard 0.168 against the 0.2 floor + * (its bigram counts reach 13, so jaccard is the guard that holds), and the + * weakest true cross-file match sits at exactly 7 shared bigrams, so the + * pr-level tier's floor of 8 would trade a measured true variant for no + * measured precision. Re-derive from that corpus before touching either + * number. The THREAD side keeps a key even here: a corpus member with no + * usable path never matches, because under the path key such a thread could + * never reach a pathed claim (the fail-closed degradation + * {@link openThreadsFromStaged} documents), and dropping the claim-side key + * must not flip that thread into a wildcard suppressor. The open corpus + * stays path-keyed: 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 bestOpenThreadMatch = ( + claim: Claim, + threads: readonly OpenThread[], + options: {ignorePath?: boolean} = {}, +): OpenThread | undefined => { + let best: {thread: OpenThread; score: OpenThreadScore} | undefined; + // A pathless (pr-level) claim compares against EVERY open thread. + for (const thread of threads) { + if ( + (options.ignorePath !== true + ? claim.path !== undefined && thread.path !== claim.path + : // A thread with no usable path stays inert (see the doc + // above): fail-closed, never a PR-wide wildcard. + thread.path === undefined || thread.path === "") || + !describesOpenThreadDefect(claim, thread) + ) { + continue; + } + const score = openThreadScore(claim, thread); + const better = + best === undefined || + score.jaccard > best.score.jaccard || + (score.jaccard === best.score.jaccard && + score.sharedBigrams > best.score.sharedBigrams); + if (better) { + best = {thread, score}; + } + } + return best?.thread; +}; + +/** + * Whether staged threads ALL failed {@link openThreadsFromStaged}'s filter, so + * suppression could not run at all. Lives here beside the filter because it is + * the filter's own failure mode; the caller only logs what this returns. + * + * An empty {@link ThreadSuppression} list cannot distinguish "nothing to + * suppress" from "suppression silently did nothing", and that ambiguity is how + * the author-spelling mismatch above reached production: it survived a whole + * three-round seeded lifecycle (webapp#41197) posting duplicate comments while + * every run reported an empty suppression list and looked correct. + * + * Kept after the producer became code, not deleted. A conforming `stage-pr.ts` + * run can no longer trip it (it stages only bot-opened, unresolved threads, + * selected by this filter's own predicate, and a mass reconciler-resolve is + * excluded per id below), which is the point: a fire now means either that the + * producer and this consumer have drifted apart inside one repo (a code bug, + * still the exact failure class #302 was), or that the staging came from + * somewhere else (the eval's live producer, a hand-built reproduction). + * A tripwire that cannot fire on today's code costs one comparison and is the + * only thing standing between the next shape drift and another silent release. + * + * Threads the reconciler resolved this run are excluded, since those are + * legitimately unusable — counted per thread against `resolvedIds` rather than + * by list length, because the reconciler's `resolve` list is never validated + * against the staged `thread_id`s: a long resolve list would otherwise mask a + * total shape failure in a short staging, silencing this very warning. + * + * Deliberate limit: ONE usable thread returns undefined, so partial shape + * drift (some threads malformed, others fine) stays invisible. This is a + * total-failure tripwire, not a per-thread audit; the per-thread version wants + * a rejection reason on each dropped thread, which is more machinery than the + * failure it would catch currently justifies. + */ +export const stagedThreadShapeFailure = ( + threads: unknown, + openThreads: readonly OpenThread[], + resolvedIds: ReadonlySet, +): {unusableThreads: number; warning: string} | undefined => { + if (openThreads.length > 0) { + return undefined; + } + const unusableThreads = (Array.isArray(threads) ? threads : []) + .filter(isRecord) + .filter((thread) => { + const id = thread["thread_id"]; + return typeof id !== "string" || !resolvedIds.has(id); + }).length; + if (unusableThreads === 0) { + return undefined; + } + return { + unusableThreads, + warning: threadSuppressionUnavailableWarning(unusableThreads), + }; +}; + +/** + * The tripwire's run-log line, built from the count alone. + * + * Shared with `dispatch-gate.ts`, which re-emits it from a real workflow step. + * The dispatcher runs inside the agent's Bash tool, where a `::warning` is only + * text: measured on webapp#41204 run 30654454047, a mis-staged run reported + * `threadSuppressionUnavailable` on dispatch-result.json and printed this line + * into the run log and the step summary, yet raised no annotation on any of the + * six jobs, while the pre-agent staging step's own `::warning` in the same run + * did annotate. The gate rebuilds the line from `unusableThreads` rather than + * forwarding the stored string, so a `dispatch-result.json` an agent could + * rewrite cannot inject workflow commands into a trusted step; that is also why + * this takes a number rather than free text. + */ +export const threadSuppressionUnavailableWarning = ( + unusableThreads: number, +): string => + `::warning title=open-thread suppression::${unusableThreads} staged thread(s), none usable ` + + `(each needs thread_id, an explicit resolved: false, and a bot-authored opener); duplicates may re-post`; + +/** + * Drop candidate claims that describe a defect an open bot thread already + * tracks (trial run S4 r2: the missing-test defect re-flagged at + * expiration.go:42 while its round-1 thread at :62 was still open, so the + * same defect briefly had two open threads). The match is same-path plus + * the calibrated #245 text-similarity floor (a pathless pr-level claim + * skips the path gate and pays the stricter {@link PR_LEVEL_FLOOR}), + * deliberately with NO line window: + * a persisting defect's re-flag routinely lands on a different line + * of the same file (the observed pair sat 20 lines apart); the similarity + * floor carries the precision. The caller excludes threads the reconciler + * resolves this run, so a fixed defect's fresh regression still posts, and + * each suppression records both the candidate's label and the matched + * thread's blocking-ness so the verdict cannot flip to APPROVE over a + * still-open, re-confirmed blocking objection (submission.ts floors only + * when BOTH are blocking: the thread's severity is the validated one, and + * the candidate's re-confirmation at blocking severity is what makes the + * floor more than a stale thread). Which thread a candidate is attributed to + * is {@link bestOpenThreadMatch}'s call, not staging order's, because + * `threadBlocking` is read off it. + */ +export const suppressOpenThreadDuplicates = ( + claims: Claim[], + threads: OpenThread[], +): {kept: Claim[]; suppressed: ThreadSuppression[]} => { + if (threads.length === 0) { + return {kept: claims, suppressed: []}; + } + const kept: Claim[] = []; + const suppressed: ThreadSuppression[] = []; + for (const claim of claims) { + const match = bestOpenThreadMatch(claim, threads); + if (match === undefined) { + kept.push(claim); + continue; + } + suppressed.push({ + id: claim.id, + source: claim.source, + label: claim.label, + ...(claim.path !== undefined ? {path: claim.path} : {}), + ...(claim.line !== undefined ? {line: claim.line} : {}), + thread_id: match.thread_id, + threadBlocking: threadOpenerIsBlocking(match.body), + }); + } + return {kept, suppressed}; +}; diff --git a/workflows/review/lib/dedup.test.ts b/workflows/review/lib/dedup.test.ts index 97cdeaa6..90974518 100644 --- a/workflows/review/lib/dedup.test.ts +++ b/workflows/review/lib/dedup.test.ts @@ -1,11 +1,11 @@ import {describe, it, expect} from "vitest"; +import {dedupeClaims} from "./dedup"; import { - dedupeClaims, openThreadsFromStaged, stagedThreadShapeFailure, suppressOpenThreadDuplicates, -} from "./dedup"; +} from "./dedup-threads"; import type {Claim} from "./dispatch-contracts"; /** diff --git a/workflows/review/lib/dedup.ts b/workflows/review/lib/dedup.ts index 2f172fdf..d7318af2 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -94,9 +94,15 @@ * can cost is bounded by code even where its judgment cannot be checked. */ -import {stripFooters, type AlsoFlagged} from "./attribution"; -import {bigrams, contentTokens, intersectionSize} from "./dedup-text"; -import {isRecord, type Claim, type ProposedCluster} from "./dispatch-contracts"; +import {type AlsoFlagged} from "./attribution"; +import { + bigrams, + contentTokens, + EXACT_ANCHOR_FLOOR, + intersectionSize, + OTHER_LINE_FLOOR, +} from "./dedup-text"; +import {type Claim, type ProposedCluster} from "./dispatch-contracts"; import {isBlockingLabel} from "./render-comment"; import { clusterMemberRejection, @@ -104,11 +110,6 @@ import { verifiableClusters, type ClusterRejection, } from "./dedup-cluster"; -// The identity of the review bot, shared with the producer that stages the -// threads this module filters (stage-pr.ts). `threads.ts` owns a GitHub fetch -// but runs nothing at import time and reaches the network only through an -// injected port, so the determinism boundary above still holds here. -import {isReviewBotAuthor} from "./threads"; /** Which tier identified a merged group (dispatch-result.json audit). */ export type MergeVia = "similarity" | "clusterer" | "both"; @@ -146,40 +147,6 @@ export type ClaimMerge = { evidence?: string; }; -/** - * Similarity floors, in two tiers. An identical `(path, line)` from two - * sources is itself evidence of one defect, so that tier's token floors sit - * just under the weakest real duplicate (run 30301235749's terse correctness - * one-liner against the discursive holistic copy: 0.149 Jaccard, 0.346 - * overlap) and the shared-bigram floor carries the precision alone. A - * DIFFERENT line is weak evidence of two defects, so that tier pays for the - * looser anchor with a higher bigram floor: run 30301235749 merged the - * skill-auditor's AddDate handoff at expiration.go:38 into the test-adequacy - * missing-test todo at :62 on five shared bigrams, and both real - * different-line duplicates (run 29943085279's expiration_test.go :15/:58 - * pair, and the bundled test-adequacy bridge) share six or more. - * - * Every floor is a minimum over real trial claims, so the margins are thin - * by construction: the exact-anchor tier separates on bigrams 4 vs 3, the - * other-line tier on 6 vs 5. Re-derive them from `dedup.test.ts`'s fixtures - * rather than nudging them by feel. - */ -const EXACT_ANCHOR_FLOOR = {jaccard: 0.14, overlap: 0.34, sharedBigrams: 4}; -const OTHER_LINE_FLOOR = {jaccard: 0.2, overlap: 0.35, sharedBigrams: 6}; - -/** - * The floor for a claim with NO anchor at all (a pr-level finding) against - * an open thread: the least anchor evidence this module scores, so it pays - * with the highest bigram floor, one tier above {@link OTHER_LINE_FLOOR}. - * Calibrated on webapp#41290 review 4867627688 (a pr-anchored re-find of a - * data race two open blocking threads tracked re-posted in full, because - * the path gate made pr-level claims unsuppressable): the true counterparts - * score 0.342/0.558/40 and 0.329/0.643/23 (jaccard/overlap/bigrams), the - * six unrelated open threads top out at 0.051/0.180/1. - * `dedup-pr-level.test.ts` carries the run's real texts; re-derive, don't nudge. - */ -const PR_LEVEL_FLOOR = {jaccard: 0.2, overlap: 0.35, sharedBigrams: 8}; - const comparisonKey = (text: string): string => text .toLowerCase() @@ -237,388 +204,6 @@ export const describesSameDefect = (a: Claim, b: Claim): boolean => { ); }; -/* -------------------------------------------------------------------------- */ -/* Open-thread suppression (trial suggestion g) */ -/* -------------------------------------------------------------------------- */ - -/** One still-open bot thread a candidate claim may duplicate. */ -export type OpenThread = { - thread_id: string; - path?: string; - /** The thread's opening comment body (the bot's original finding). */ - body: string; -}; - -export type ThreadSuppression = { - id: string; - source: string; - label: string; - path?: string; // absent for a pr-level claim (no anchor) - line?: number; - thread_id: string; - /** - * Whether the matched thread's OPENING comment carries a blocking label. - * The verdict floor keys on this, not on the candidate's own label: the - * thread's severity survived last run's validation, while a suppressed - * candidate is dropped before validation ever sees it. Read off the - * BEST-matching thread ({@link bestOpenThreadMatch}), since among several - * threads that clear the floor their labels can differ. - */ - threadBlocking: boolean; - /** - * Present (and true) when the matched thread is from the ADJUDICATED - * corpus (a bot thread a human resolved) rather than an open one; see - * dedup-adjudicated.ts. Audit-trail only: an adjudicated suppression can - * never floor the verdict, because that pass never suppresses a blocking - * candidate and submission.ts's floor requires the CANDIDATE's label to - * be blocking too. - */ - adjudicated?: true; -}; - -/** - * Whether an open thread's opener is a blocking finding, read from the - * leading `**label:**` template (tolerating the markdown-stripped form the - * staged bodies sometimes carry) and classified by {@link isBlockingLabel}, - * the single owner of that rule, so a label the taxonomy blocks on cannot be - * missed here (`issue (blocking, best-practice)` is a blocking label too, and - * a hand-rolled `\(blocking\)` match silently reads it as advisory). Spacing - * inside the parentheses is normalized before the lookup; a body with no - * recognizable label reads as non-blocking, since an unvalidated floor is the - * failure mode this guards. - */ -export const threadOpenerIsBlocking = (body: string): boolean => { - const label = /^\s*\*{0,2}([a-z]+ \([^)]*\))\*{0,2}:?/i.exec(body)?.[1]; - return ( - label !== undefined && - isBlockingLabel( - label - .toLowerCase() - .replace(/\s+/g, " ") - .replace(/\s*,\s*/g, ", "), - ) - ); -}; - -/** - * The prose of a previously-posted bot comment, for similarity comparison: - * the collapsed attribution/version footers (attribution.ts's strip), the - * leading `**label:**` template (tolerating the markdown-stripped form - * the staged bodies sometimes carry) and everything from the first code - * fence on (a suggestion block) are dropped, as are rule-quote lines: - * boilerplate shared by ALL bot comments would inflate similarity between - * unrelated findings. - */ -const threadProse = (body: string): string => - stripFooters(body) - .split("```")[0] - .split("\n") - .filter((line) => !line.trimStart().startsWith(">")) - .join(" ") - .replace(/^\s*\*{0,2}[a-z]+ \([^)]*\)\*{0,2}:?\*{0,2}\s*/i, ""); - -/** - * Whether a staged thread carries a "still open" state. `stage-pr.ts` writes - * `resolved: false` on every thread it stages; the `get_review_comments` - * spelling (`is_resolved`) and the camelCase form are still accepted, because - * a staging assembled from that tool's output (the eval's producers, and any - * hand-built staging used to reproduce a run) inherits its shape. Absent - * reads as unknown, not as open. - */ -export const stagedResolvedState = (thread: Record): unknown => - thread["resolved"] ?? thread["is_resolved"] ?? thread["isResolved"]; - -/** - * Build the suppression inputs from staged threads.json. `stage-pr.ts` is the - * producer (one GraphQL fetch, partitioned by opener), and it selects the - * bot's threads through the SAME {@link isReviewBotAuthor} predicate this - * filter admits them by, which is the point of the shared constant: the two - * layers spelling the identity separately is precisely how suppression became - * unreachable for a release (Khan/actions#302). The guards below nonetheless - * stay, because they are the properties suppression depends on and a producer - * bug must degrade to a duplicate comment rather than to a dropped finding: - * - the opener is the bot's, in either spelling. A human thread must never - * silently kill a candidate, and its free-text opener would also read as - * non-blocking and skip the verdict floor. - * - the thread is still open, per the staged `resolved` flag. An - * already-resolved thread would otherwise suppress a genuine regression - * re-flag with nothing to check it against. - * Threads in resolvedIds (reconciler-resolved this run) are exempt as well: a - * fixed defect posting again is a fresh finding. Fails closed on each: a - * thread without a bot-authored opener, or without an explicit - * `resolved: false`, never suppresses (worst case is a duplicate comment). - * - * `path` is read from the thread and falls back to the opening comment: the - * code producer carries `path` per thread, while `get_review_comments` carries - * it per comment, so a staging built from that tool inherits the other shape. - * The fallback is not cosmetic: {@link suppressOpenThreadDuplicates} matches - * on `path`, so a thread staged without one silently suppresses nothing. - */ -export const openThreadsFromStaged = ( - threads: unknown, - resolvedIds: ReadonlySet, -): OpenThread[] => - (Array.isArray(threads) ? threads : []) - .filter(isRecord) - .flatMap((thread) => { - const comments = thread["comments"]; - const opener = - Array.isArray(comments) && isRecord(comments[0]) - ? comments[0] - : undefined; - const author = opener?.["author"]; - if ( - typeof thread["thread_id"] !== "string" || - resolvedIds.has(thread["thread_id"]) || - stagedResolvedState(thread) !== false || - typeof author !== "string" || - !isReviewBotAuthor(author) - ) { - return []; - } - const path = - typeof thread["path"] === "string" - ? thread["path"] - : typeof opener?.["path"] === "string" - ? (opener["path"] as string) - : undefined; - return [ - { - thread_id: thread["thread_id"], - ...(path !== undefined ? {path} : {}), - body: - typeof opener["body"] === "string" - ? opener["body"] - : "", - }, - ]; - }); - -/** How strongly a claim's text matches one open thread's opener. */ -type OpenThreadScore = { - jaccard: number; - overlap: number; - sharedBigrams: number; -}; - -const openThreadScore = (claim: Claim, thread: OpenThread): OpenThreadScore => { - const tokensA = contentTokens( - `${claim.subject} ${claim.discussion} ${claim.failure_scenario}`, - ); - const tokensB = contentTokens(threadProse(thread.body)); - const setA = new Set(tokensA); - const setB = new Set(tokensB); - if (setA.size === 0 || setB.size === 0) { - return {jaccard: 0, overlap: 0, sharedBigrams: 0}; - } - const shared = intersectionSize(setA, setB); - return { - jaccard: shared / (setA.size + setB.size - shared), - overlap: shared / Math.min(setA.size, setB.size), - sharedBigrams: intersectionSize(bigrams(tokensA), bigrams(tokensB)), - }; -}; - -/** Whether a claim clearly describes the defect an open thread tracks. */ -export const describesOpenThreadDefect = ( - claim: Claim, - thread: OpenThread, -): boolean => { - const {jaccard, overlap, sharedBigrams} = openThreadScore(claim, thread); - // This match has no line window at all (see - // `suppressOpenThreadDuplicates`), so it pays for the loose anchor with - // the higher bigram floor, exactly as a cross-source pair on mismatched - // lines does; a pathless (pr-level) claim has no anchor evidence at all - // and pays one tier more. Erring strict is the safe direction: a - // missed suppression posts a duplicate, a false one drops a finding. - const floor = claim.path === undefined ? PR_LEVEL_FLOOR : OTHER_LINE_FLOOR; - return ( - jaccard >= floor.jaccard && - overlap >= floor.overlap && - sharedBigrams >= floor.sharedBigrams - ); -}; - -/** - * The open thread a candidate BEST matches, not merely the first one it clears - * the floor against. - * - * More than one open thread can describe the same area of a file, and taking - * the first match in staging order reads `threadBlocking` off a thread that is - * not the candidate's counterpart. Measured on webapp#41204 run 30650642317, - * the first live run where suppression fired: the fresh test-adequacy todo - * ("no test covers Record's documented zero-valued-Window guarantee") cleared - * the floor against BOTH the blocking nil-map panic thread and its own exact - * counterpart, a non-blocking nitpick whose opener reads "The documented - * zero-value-Window guarantee has no test". Staging order put the blocking - * thread first because it was the older one, so the suppression recorded - * `threadBlocking: true` and added a second entry to submission.ts's verdict - * floor. That run's verdict was already floored correctly by another - * suppression, so nothing was mis-decided, but the direction is the dangerous - * one: attributing a candidate to a blocking thread that is not its match - * forces REQUEST_CHANGES on thinner evidence than the both-sides-blocking rule - * intends. - * - * Ranked on `jaccard` first because it is the length-normalized metric: raw - * `sharedBigrams` grows with the opener's length, and `overlap` divides by the - * smaller token set, which favors the longer, more discursive thread. On the - * observed pair both jaccard (0.253 vs 0.218) and bigrams (13 vs 12) pick the - * true counterpart while overlap alone (0.468 vs 0.511) picks the wrong one, - * so bigrams break jaccard ties and overlap never ranks. Strictly-better - * comparisons keep staging order as the final tiebreak, so an exact scoring - * tie behaves as it did before. - */ -export const bestOpenThreadMatch = ( - claim: Claim, - threads: readonly OpenThread[], -): OpenThread | undefined => { - let best: {thread: OpenThread; score: OpenThreadScore} | undefined; - // A pathless (pr-level) claim compares against EVERY open thread. - for (const thread of threads) { - if ( - (claim.path !== undefined && thread.path !== claim.path) || - !describesOpenThreadDefect(claim, thread) - ) { - continue; - } - const score = openThreadScore(claim, thread); - const better = - best === undefined || - score.jaccard > best.score.jaccard || - (score.jaccard === best.score.jaccard && - score.sharedBigrams > best.score.sharedBigrams); - if (better) { - best = {thread, score}; - } - } - return best?.thread; -}; - -/** - * Whether staged threads ALL failed {@link openThreadsFromStaged}'s filter, so - * suppression could not run at all. Lives here beside the filter because it is - * the filter's own failure mode; the caller only logs what this returns. - * - * An empty {@link ThreadSuppression} list cannot distinguish "nothing to - * suppress" from "suppression silently did nothing", and that ambiguity is how - * the author-spelling mismatch above reached production: it survived a whole - * three-round seeded lifecycle (webapp#41197) posting duplicate comments while - * every run reported an empty suppression list and looked correct. - * - * Kept after the producer became code, not deleted. A conforming `stage-pr.ts` - * run can no longer trip it (it stages only bot-opened, unresolved threads, - * selected by this filter's own predicate, and a mass reconciler-resolve is - * excluded per id below), which is the point: a fire now means either that the - * producer and this consumer have drifted apart inside one repo (a code bug, - * still the exact failure class #302 was), or that the staging came from - * somewhere else (the eval's live producer, a hand-built reproduction). - * A tripwire that cannot fire on today's code costs one comparison and is the - * only thing standing between the next shape drift and another silent release. - * - * Threads the reconciler resolved this run are excluded, since those are - * legitimately unusable — counted per thread against `resolvedIds` rather than - * by list length, because the reconciler's `resolve` list is never validated - * against the staged `thread_id`s: a long resolve list would otherwise mask a - * total shape failure in a short staging, silencing this very warning. - * - * Deliberate limit: ONE usable thread returns undefined, so partial shape - * drift (some threads malformed, others fine) stays invisible. This is a - * total-failure tripwire, not a per-thread audit; the per-thread version wants - * a rejection reason on each dropped thread, which is more machinery than the - * failure it would catch currently justifies. - */ -export const stagedThreadShapeFailure = ( - threads: unknown, - openThreads: readonly OpenThread[], - resolvedIds: ReadonlySet, -): {unusableThreads: number; warning: string} | undefined => { - if (openThreads.length > 0) { - return undefined; - } - const unusableThreads = (Array.isArray(threads) ? threads : []) - .filter(isRecord) - .filter((thread) => { - const id = thread["thread_id"]; - return typeof id !== "string" || !resolvedIds.has(id); - }).length; - if (unusableThreads === 0) { - return undefined; - } - return { - unusableThreads, - warning: threadSuppressionUnavailableWarning(unusableThreads), - }; -}; - -/** - * The tripwire's run-log line, built from the count alone. - * - * Shared with `dispatch-gate.ts`, which re-emits it from a real workflow step. - * The dispatcher runs inside the agent's Bash tool, where a `::warning` is only - * text: measured on webapp#41204 run 30654454047, a mis-staged run reported - * `threadSuppressionUnavailable` on dispatch-result.json and printed this line - * into the run log and the step summary, yet raised no annotation on any of the - * six jobs, while the pre-agent staging step's own `::warning` in the same run - * did annotate. The gate rebuilds the line from `unusableThreads` rather than - * forwarding the stored string, so a `dispatch-result.json` an agent could - * rewrite cannot inject workflow commands into a trusted step; that is also why - * this takes a number rather than free text. - */ -export const threadSuppressionUnavailableWarning = ( - unusableThreads: number, -): string => - `::warning title=open-thread suppression::${unusableThreads} staged thread(s), none usable ` + - `(each needs thread_id, an explicit resolved: false, and a bot-authored opener); duplicates may re-post`; - -/** - * Drop candidate claims that describe a defect an open bot thread already - * tracks (trial run S4 r2: the missing-test defect re-flagged at - * expiration.go:42 while its round-1 thread at :62 was still open, so the - * same defect briefly had two open threads). The match is same-path plus - * the calibrated #245 text-similarity floor (a pathless pr-level claim - * skips the path gate and pays the stricter {@link PR_LEVEL_FLOOR}), - * deliberately with NO line window: - * a persisting defect's re-flag routinely lands on a different line - * of the same file (the observed pair sat 20 lines apart); the similarity - * floor carries the precision. The caller excludes threads the reconciler - * resolves this run, so a fixed defect's fresh regression still posts, and - * each suppression records both the candidate's label and the matched - * thread's blocking-ness so the verdict cannot flip to APPROVE over a - * still-open, re-confirmed blocking objection (submission.ts floors only - * when BOTH are blocking: the thread's severity is the validated one, and - * the candidate's re-confirmation at blocking severity is what makes the - * floor more than a stale thread). Which thread a candidate is attributed to - * is {@link bestOpenThreadMatch}'s call, not staging order's, because - * `threadBlocking` is read off it. - */ -export const suppressOpenThreadDuplicates = ( - claims: Claim[], - threads: OpenThread[], -): {kept: Claim[]; suppressed: ThreadSuppression[]} => { - if (threads.length === 0) { - return {kept: claims, suppressed: []}; - } - const kept: Claim[] = []; - const suppressed: ThreadSuppression[] = []; - for (const claim of claims) { - const match = bestOpenThreadMatch(claim, threads); - if (match === undefined) { - kept.push(claim); - continue; - } - suppressed.push({ - id: claim.id, - source: claim.source, - label: claim.label, - ...(claim.path !== undefined ? {path: claim.path} : {}), - ...(claim.line !== undefined ? {line: claim.line} : {}), - thread_id: match.thread_id, - threadBlocking: threadOpenerIsBlocking(match.body), - }); - } - return {kept, suppressed}; -}; - /** * Same path, any line distance: run 29943085279 posted the * missing-deletion-test defect at expiration_test.go:15 and :58 (43 lines diff --git a/workflows/review/lib/dispatch.ts b/workflows/review/lib/dispatch.ts index dcce8e0e..449bee4b 100644 --- a/workflows/review/lib/dispatch.ts +++ b/workflows/review/lib/dispatch.ts @@ -39,7 +39,8 @@ * note lines) is pure code. No prose about the code under review. */ -import {dedupeClaims, type ClaimMerge, type ThreadSuppression} from "./dedup"; +import {dedupeClaims, type ClaimMerge} from "./dedup"; +import {type ThreadSuppression} from "./dedup-threads"; import { reapplyCrossFileOccurrences, suppressThenMergeCrossFile, @@ -122,12 +123,11 @@ export { type AgentDefinition, type DispatchFs, } from "./dispatch-agents"; +export {dedupeClaims, type ClaimMerge} from "./dedup"; export { - dedupeClaims, suppressOpenThreadDuplicates, - type ClaimMerge, type ThreadSuppression, -} from "./dedup"; +} from "./dedup-threads"; export {type ClusterRejection} from "./dedup-cluster"; /* -------------------------------------------------------------------------- */ diff --git a/workflows/review/lib/forwarded-warnings.ts b/workflows/review/lib/forwarded-warnings.ts index 01524157..54ecc3a8 100644 --- a/workflows/review/lib/forwarded-warnings.ts +++ b/workflows/review/lib/forwarded-warnings.ts @@ -24,7 +24,7 @@ * no filesystem, no clock, no model call. */ -import {threadSuppressionUnavailableWarning} from "./dedup"; +import {threadSuppressionUnavailableWarning} from "./dedup-threads"; /** * Every workflow-command line to re-emit for this run, given the `out/` diff --git a/workflows/review/lib/stage-pr.ts b/workflows/review/lib/stage-pr.ts index 2433acf9..f6f278cf 100644 --- a/workflows/review/lib/stage-pr.ts +++ b/workflows/review/lib/stage-pr.ts @@ -556,7 +556,7 @@ export const runStagePrCli = async ( // particular shape, and everything downstream then depended on a // model-produced file: `hasThreads` (which decides whether the // thread-reconciler is dispatched at all, so it changes the roster), - // open-thread suppression (dedup.ts), and the accountability recap + // open-thread suppression (dedup-threads.ts), and the accountability recap // (rereview.ts). Khan/actions#302 patched a symptom of that seam: a // CONFORMING staging produced zero usable threads for a whole release // because the prompt's selection rule and the code's guard spelled the @@ -622,7 +622,7 @@ export const runStagePrCli = async ( botThreads.map((thread) => ({ ...thread, // Unresolved by construction (the partition above drops - // resolved threads), but written anyway: dedup.ts requires an + // resolved threads), but written anyway: dedup-threads.ts requires an // explicit `resolved: false` and fails closed without one. // That guard stays deliberately, rather than trusting this // producer. diff --git a/workflows/review/lib/stage-threads.test.ts b/workflows/review/lib/stage-threads.test.ts index 396a0121..f7f96f56 100644 --- a/workflows/review/lib/stage-threads.test.ts +++ b/workflows/review/lib/stage-threads.test.ts @@ -1,6 +1,6 @@ import {describe, it, expect} from "vitest"; -import {openThreadsFromStaged, stagedThreadShapeFailure} from "./dedup"; +import {openThreadsFromStaged, stagedThreadShapeFailure} from "./dedup-threads"; import {adjudicatedThreadsFromStaged} from "./dedup-adjudicated"; import {computeRoster} from "./dispatch-roster"; import {runStagePrCli, type GhGet, type StagePrFs} from "./stage-pr"; @@ -17,8 +17,8 @@ import { * following the precedent dispatch-trial-followups.test.ts set; the fixtures * mirror that file's. * - * These cases pin the producer against the consumers it feeds (`dedup.ts`'s - * open-thread suppression and `dispatch-roster.ts`'s reconciler gate) rather + * These cases pin the producer against the consumers it feeds + * (`dedup-threads.ts`'s open-thread suppression and `dispatch-roster.ts`'s reconciler gate) rather * than only against the shape a reader of this file would expect, because * "each layer looked right on its own" is exactly how Khan/actions#302 shipped: * the prompt selected bot threads by one spelling of the login and the code diff --git a/workflows/review/lib/threads.ts b/workflows/review/lib/threads.ts index 1f101cf4..d4496c5c 100644 --- a/workflows/review/lib/threads.ts +++ b/workflows/review/lib/threads.ts @@ -19,7 +19,7 @@ * `reviewThreads` connection carries both. * * This module also owns the answer to "is this login our review bot", because - * the producer's filter and the consumer's guard (`dedup.ts`'s open-thread + * the producer's filter and the consumer's guard (`dedup-threads.ts`'s open-thread * suppression) must not be able to disagree about it. Khan/actions#302 was * exactly that disagreement one layer up: the prompt selected threads by one * spelling of the bot's login and the code admitted another, so a conforming @@ -48,7 +48,7 @@ export const DEFAULT_REVIEW_BOT_LOGIN = "github-actions[bot]"; * The login this workflow's own review comments are authored by, and the * single source of truth for that identity across the producer * (`stage-pr.ts`, which selects the bot's threads) and the consumers - * (`dedup.ts`'s suppression guard). Both layers read it here, which is the + * (`dedup-threads.ts`'s suppression guard). Both layers read it here, which is the * property #302 lost when each spelled the identity itself. * * Deployment config, not a compiled-in constant, because the identity is a @@ -253,7 +253,7 @@ const threadsConnectionOf = ( * `resolved`/`resolvedBy` exist so ONE fetch can serve both the unresolved * partition (threads.json / human-threads.json) and the adjudicated corpus * (adjudicated-threads.json: bot threads a HUMAN resolved, which suppress - * re-derivation of the defect they adjudicated — see dedup.ts's + * re-derivation of the defect they adjudicated — see dedup-adjudicated.ts's * `adjudicatedThreadsFromStaged`). */ export type FetchedThread = StagedThread & { @@ -346,7 +346,7 @@ export const collectReviewThreads = async ( ? str(comment["author"]["login"]) : "", // Verbatim. The label parsers (`rereview.ts`'s recap, - // `dedup.ts`'s suppression) read the leading `**label:**` + // `dedup-threads.ts`'s suppression) read the leading `**label:**` // template off this string; normalising it here is how a // finding becomes unclassifiable. body: str(comment["body"]),