From 6181b33b222790f4b56eea1b74b911c1889f26b8 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Thu, 30 Jul 2026 14:35:39 -0700 Subject: [PATCH 1/2] [jwies/review-thread-suppression-author] review: fix open-thread suppression, unreachable on every conforming run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `openThreadsFromStaged` accepted an opener author of `github-actions[bot]` only. `get_review_comments`, the tool review.md tells the orchestrator to copy `threads.json` from verbatim, renders that same account as bare `github-actions`, and the filter fails closed — so a *correct* staging yielded zero usable threads and suppression never ran. It also read `path` from the thread while the tool carries `path` per comment, and `suppressOpenThreadDuplicates` matches on `path`, so a thread that cleared the author check still suppressed nothing. Measured on webapp#41197's three-round seeded lifecycle (the review-v1.8.0 acceptance trial): `threadSuppressions: []` in all three rounds while the re-reviews duplicated open threads. 6 of round 3's 8 comments landed on the exact path and line of an already-open thread, and the reconciler's `keep` list held those thread IDs in the same run, so the data was present and unusable. Every unit fixture spelled the bot the way the code did, which is why the suite passed throughout. The new cases use the tool's real shape verbatim, assert an end-to-end suppression (with the blocking thread still flooring the verdict) rather than only the parse, and keep a human-opened thread refused so the widened author check cannot drop a finding outright. Both new shape cases fail against the old filter, verified by reverting it. Second half, since a fail-open guard that cannot be seen failing is how this survived a release: `stagedThreadShapeFailure` reports a staging whose shape defeats the filter, as `threadSuppressionUnavailable` on dispatch-result.json plus a run-log warning. "Nothing to suppress" is no longer indistinguishable from "suppression silently did nothing". The condition lives in dedup.ts beside the filter it describes, which also keeps dispatch.ts inside its 1000-line lint ceiling. --- .../review-thread-suppression-author.md | 5 ++ workflows/review/lib/dedup.test.ts | 85 +++++++++++++++++++ workflows/review/lib/dedup.ts | 68 +++++++++++++-- workflows/review/lib/dispatch.ts | 28 ++++-- 4 files changed, 172 insertions(+), 14 deletions(-) create mode 100644 .changeset/review-thread-suppression-author.md diff --git a/.changeset/review-thread-suppression-author.md b/.changeset/review-thread-suppression-author.md new file mode 100644 index 00000000..21cca2db --- /dev/null +++ b/.changeset/review-thread-suppression-author.md @@ -0,0 +1,5 @@ +--- +"review": patch +--- + +Open-thread suppression was unreachable on every conforming run, so re-reviews re-posted findings an open bot thread already tracked. `openThreadsFromStaged` accepted an opener author of `github-actions[bot]` only, but `get_review_comments` — the tool review.md tells the orchestrator to copy `threads.json` from, verbatim — renders that same account as bare `github-actions`, and the filter fails closed, so a *correct* staging produced zero usable threads. It also read `path` from the thread while the tool carries `path` per comment, and `suppressOpenThreadDuplicates` matches on `path`, so a thread that cleared the author check still suppressed nothing. Both spellings are now accepted (`BOT_AUTHORS`, documented against the REST `user.login` surface that legitimately renders the bracketed form and that `stage-pr.ts` reads for prior reviews), and `path` falls back to the opening comment. Measured on webapp#41197's three-round seeded lifecycle, where suppression reported `threadSuppressions: []` in all three rounds while the re-reviews duplicated open threads: 6 of round 3's 8 comments landed on the exact path and line of a thread that was already open, and the reconciler's own `keep` list held those thread IDs in the same run, so the data suppression needed was present and unusable. Every unit fixture spelled the bot the way the code did, which is why the suite passed throughout; the regression cases now use the tool's real shape verbatim, assert an end-to-end suppression (with the blocking thread still flooring the verdict) rather than only the parse, and keep a human-opened thread refused so the widened author check cannot drop a finding outright. Second half of the fix, since a fail-open guard that cannot be seen failing is how this survived a whole release: `stagedThreadShapeFailure` reports a staging whose shape defeats the filter entirely, as a `threadSuppressionUnavailable` field on `dispatch-result.json` and a run-log warning, so "nothing to suppress" is no longer indistinguishable from "suppression silently did nothing." diff --git a/workflows/review/lib/dedup.test.ts b/workflows/review/lib/dedup.test.ts index e7076a5c..dcfb9d7b 100644 --- a/workflows/review/lib/dedup.test.ts +++ b/workflows/review/lib/dedup.test.ts @@ -718,4 +718,89 @@ describe("openThreadsFromStaged", () => { ); expect(noBody).toEqual([{thread_id: "T1", path: "a.ts", body: ""}]); }); + + // The fixtures above all spell the bot `github-actions[bot]`, which is the + // REST spelling (`user.login`) that stage-pr.ts reads for prior reviews. + // `get_review_comments`, the tool that stages THESE threads, renders the + // same account as bare `github-actions` and carries `path` on the comment + // rather than the thread. Every fixture agreeing with the code is why + // suppression passed its unit tests while suppressing nothing across three + // rounds of a seeded lifecycle (webapp#41197). This case is the real tool + // shape, verbatim, so the fixtures can no longer drift back toward the code. + it("accepts the get_review_comments shape: bare `github-actions`, path on the comment", () => { + const fromTool = { + thread_id: "PRRT_kwDOAJgNW86VOObT", + is_resolved: false, + is_outdated: true, + comments: [ + { + author: "github-actions", + body: "**issue (blocking):** Retention cutoff subtracts 180 months, not 180 days.", + path: "services/ai-guide/memory/expiration.go", + }, + ], + }; + expect(openThreadsFromStaged([fromTool], new Set())).toEqual([ + { + thread_id: "PRRT_kwDOAJgNW86VOObT", + path: "services/ai-guide/memory/expiration.go", + body: "**issue (blocking):** Retention cutoff subtracts 180 months, not 180 days.", + }, + ]); + }); + + it("suppresses a re-flag against a thread staged in the tool's shape", () => { + // The end-to-end assertion the unit suite never made: a claim + // re-describing an open thread's defect must not post again. Without + // the author and path fixes this returns the claim unsuppressed, which + // is exactly what production did. + const claim: Claim = { + id: "correctness-reviewer-1", + source: "correctness-reviewer", + label: "issue (blocking)", + path: "services/ai-guide/memory/expiration.go", + line: 38, + subject: "AddDate passes the day count into the months slot", + discussion: + "The retention cutoff subtracts 180 months rather than 180 days, so expiration never fires.", + failure_scenario: + "No memory is ever old enough to match the cutoff, so the retention feature is a silent no-op.", + confidence: 0.9, + }; + const thread = { + thread_id: "PRRT_1", + is_resolved: false, + comments: [ + { + author: "github-actions", + body: "**issue (blocking):** Retention cutoff subtracts 180 months, not 180 days — expiration never fires. AddDate's signature is (years, months, days), so the retention window is 15 years and no memory ever matches.", + path: "services/ai-guide/memory/expiration.go", + }, + ], + }; + const result = suppressOpenThreadDuplicates( + [claim], + openThreadsFromStaged([thread], new Set()), + ); + expect(result.kept).toEqual([]); + expect(result.suppressed).toHaveLength(1); + expect(result.suppressed[0]?.thread_id).toBe("PRRT_1"); + // The thread's opener is blocking, so the verdict floor still applies: + // suppressing the duplicate must not let a verdict flip to APPROVE. + expect(result.suppressed[0]?.threadBlocking).toBe(true); + }); + + it("still refuses a human-opened thread in the tool's shape", () => { + // The author fix widens the accepted spellings; it must not widen them + // to anyone. A human thread killing a candidate would drop a finding + // outright and skip the verdict floor with it. + const humanThread = { + thread_id: "PRRT_2", + is_resolved: false, + comments: [ + {author: "jwbron", body: "please also check X", path: "a.ts"}, + ], + }; + expect(openThreadsFromStaged([humanThread], new Set())).toEqual([]); + }); }); diff --git a/workflows/review/lib/dedup.ts b/workflows/review/lib/dedup.ts index 94109e2f..ad698236 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -230,15 +230,27 @@ const threadProse = (body: string): string => const stagedResolvedState = (thread: Record): unknown => thread["resolved"] ?? thread["is_resolved"] ?? thread["isResolved"]; +/** + * The bot's author spellings, both of which are the same identity. The REST + * surfaces (`user.login` on a review, which stage-pr.ts reads) render a bot + * as `github-actions[bot]`; `get_review_comments`, which stages the threads + * this module consumes, renders the SAME account as bare `github-actions`. + * Accepting only the bracketed form is what made suppression unreachable on + * every conforming run: the prompt tells the orchestrator to copy `author` + * from the tool output verbatim, so a correct staging never matched. + */ +const BOT_AUTHORS = new Set(["github-actions[bot]", "github-actions"]); + /** * Build the suppression inputs from staged threads.json. The staging is * prompt-executed (review.md asks the orchestrator for the unresolved * github-actions[bot] threads; stage-pr.ts deliberately does not stage * threads yet), so BOTH properties suppression depends on are enforced HERE * in code rather than trusted from the prompt: - * - the opener is the bot's. A mis-staged 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 opener is the bot's, in either spelling ({@link BOT_AUTHORS}). A + * mis-staged 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 `resolved` flag staged from the tool's * own `is_resolved`. A mis-staged already-resolved thread would otherwise * suppress a genuine regression re-flag with nothing to check it against. @@ -246,6 +258,12 @@ const stagedResolvedState = (thread: Record): unknown => * 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, since + * `get_review_comments` carries `path` per comment rather than per thread and + * a verbatim staging inherits that 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, @@ -259,20 +277,26 @@ export const openThreadsFromStaged = ( 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 || - opener?.["author"] !== "github-actions[bot]" + typeof author !== "string" || + !BOT_AUTHORS.has(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"], - ...(typeof thread["path"] === "string" - ? {path: thread["path"]} - : {}), + ...(path !== undefined ? {path} : {}), body: typeof opener["body"] === "string" ? opener["body"] @@ -328,6 +352,36 @@ export const describesOpenThreadDefect = ( * the candidate's re-confirmation at blocking severity is what makes the * floor more than a stale 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, and the caller only logs what it 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. Threads the + * reconciler resolved this run are excluded from the count, since those are + * legitimately unusable. + */ +export const stagedThreadShapeFailure = ( + threads: unknown, + openThreads: readonly OpenThread[], + resolvedThisRun: number, +): {stagedThreads: number; warning: string} | undefined => { + const stagedThreads = Array.isArray(threads) ? threads.length : 0; + if (stagedThreads <= resolvedThisRun || openThreads.length > 0) { + return undefined; + } + return { + stagedThreads, + warning: + `::warning title=open-thread suppression::${stagedThreads} staged thread(s), none usable ` + + `(each needs thread_id, an explicit resolved: false, and a bot-authored opener); duplicates may re-post`, + }; +}; + export const suppressOpenThreadDuplicates = ( claims: Claim[], threads: OpenThread[], diff --git a/workflows/review/lib/dispatch.ts b/workflows/review/lib/dispatch.ts index 446eb928..b1508a21 100644 --- a/workflows/review/lib/dispatch.ts +++ b/workflows/review/lib/dispatch.ts @@ -42,6 +42,7 @@ import { dedupeClaims, openThreadsFromStaged, + stagedThreadShapeFailure, suppressOpenThreadDuplicates, type ClaimMerge, type ThreadSuppression, @@ -341,6 +342,8 @@ export type DispatchResult = { * (submission.ts): the open thread is the actionable feedback. */ threadSuppressions: ThreadSuppression[]; + /** Set when every staged thread failed the filter (see dedup.ts). */ + threadSuppressionUnavailable?: {stagedThreads: number; warning: string}; /** The reconciler's decision, when it ran and parsed. */ reconciliation?: {resolve: string[]; keep: string[]; skipLines: unknown}; /** correctness-reviewer `files[]` risk levels (Steps 7-8). */ @@ -837,14 +840,22 @@ export const runDispatch = async ( // re-posted at a new anchor. Threads the reconciler resolves this run // are exempt; when the reconciler was unavailable, nothing resolves, so // every staged bot thread suppresses (fail toward fewer duplicate - // threads). Only bot-authored openers may suppress: the filter lives in - // openThreadsFromStaged (dedup.ts), since threads.json staging is - // prompt-executed and unenforced upstream. - const suppression = suppressOpenThreadDuplicates( - claims, - openThreadsFromStaged(threads, new Set(reconciliation?.resolve ?? [])), - ); + // threads). The bot-opener filter, and the check for a staging whose shape + // defeats it and so suppresses nothing, both live in dedup.ts: threads.json + // staging is prompt-executed and unenforced upstream. + const resolvedIds = new Set(reconciliation?.resolve ?? []); + const openThreads = openThreadsFromStaged(threads, resolvedIds); + const suppression = suppressOpenThreadDuplicates(claims, openThreads); claims = suppression.kept; + const threadSuppressionUnavailable = stagedThreadShapeFailure( + threads, + openThreads, + resolvedIds.size, + ); + if (threadSuppressionUnavailable !== undefined) { + // eslint-disable-next-line no-console + console.error(threadSuppressionUnavailable.warning); + } // Phase 3: claim validation. let validatorRan = false; @@ -922,6 +933,9 @@ export const runDispatch = async ( claims, merges: deduped.merges, threadSuppressions: suppression.suppressed, + ...(threadSuppressionUnavailable !== undefined + ? {threadSuppressionUnavailable} + : {}), ...(reconciliation !== undefined ? {reconciliation} : {}), ...(riskFiles !== undefined ? {riskFiles} : {}), ...(patterns !== undefined ? {patterns} : {}), From 4ed269be4b7a43dfdc404fe55648dd5ba3831f18 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Thu, 30 Jul 2026 15:02:12 -0700 Subject: [PATCH 2/2] [jwies/review-thread-suppression-author] review: address the bot review on the suppression fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings on #302, three of them real defects in the fix itself. - The new function's JSDoc was inserted between `suppressOpenThreadDuplicates`'s doc block and its declaration, orphaning that doc and stacking two blocks. Moved above it; both now sit directly over what they document. - `stagedThreadShapeFailure` compared the staged count against the raw length of the reconciler's `resolve` list, which is never validated against the staged `thread_id`s — so a long resolve list could mask a total shape failure in a short staging, silencing the exact warning it exists to raise. It now counts staged threads per id against `resolvedIds`, and documents the limit it keeps: one usable thread returns nothing, so this is a total-failure tripwire, not a per-thread audit. Field renamed `unusableThreads`, since that is what it counts. - It had no test coverage at all, which for a guard against silent regression is the wrong way round. Three cases: the warning path, both no-report branches, and the per-id counting (which fails against the old list-length comparison). The fourth is the one that mattered most: the fix was half a fix. review.md still told the orchestrator to stage "the unresolved `github-actions[bot]` threads" and to treat "any author other than `github-actions[bot]`" as human — bracketed-only, the same premise the code bug had, one layer up. A literal reading stages zero bot threads before the widened filter ever runs (invisible, since the reporter sees an empty staging), or misfiles a bot thread as human, where `skipLines` makes the submission DROP a fresh finding rather than duplicate one. Both instructions now name either spelling and say why. --- .../review-thread-suppression-author.md | 2 +- workflows/review/lib/dedup.test.ts | 53 ++++++++++++++ workflows/review/lib/dedup.ts | 72 ++++++++++++------- workflows/review/lib/dispatch.ts | 4 +- workflows/review/review.md | 23 ++++-- 5 files changed, 118 insertions(+), 36 deletions(-) diff --git a/.changeset/review-thread-suppression-author.md b/.changeset/review-thread-suppression-author.md index 21cca2db..4faf8037 100644 --- a/.changeset/review-thread-suppression-author.md +++ b/.changeset/review-thread-suppression-author.md @@ -2,4 +2,4 @@ "review": patch --- -Open-thread suppression was unreachable on every conforming run, so re-reviews re-posted findings an open bot thread already tracked. `openThreadsFromStaged` accepted an opener author of `github-actions[bot]` only, but `get_review_comments` — the tool review.md tells the orchestrator to copy `threads.json` from, verbatim — renders that same account as bare `github-actions`, and the filter fails closed, so a *correct* staging produced zero usable threads. It also read `path` from the thread while the tool carries `path` per comment, and `suppressOpenThreadDuplicates` matches on `path`, so a thread that cleared the author check still suppressed nothing. Both spellings are now accepted (`BOT_AUTHORS`, documented against the REST `user.login` surface that legitimately renders the bracketed form and that `stage-pr.ts` reads for prior reviews), and `path` falls back to the opening comment. Measured on webapp#41197's three-round seeded lifecycle, where suppression reported `threadSuppressions: []` in all three rounds while the re-reviews duplicated open threads: 6 of round 3's 8 comments landed on the exact path and line of a thread that was already open, and the reconciler's own `keep` list held those thread IDs in the same run, so the data suppression needed was present and unusable. Every unit fixture spelled the bot the way the code did, which is why the suite passed throughout; the regression cases now use the tool's real shape verbatim, assert an end-to-end suppression (with the blocking thread still flooring the verdict) rather than only the parse, and keep a human-opened thread refused so the widened author check cannot drop a finding outright. Second half of the fix, since a fail-open guard that cannot be seen failing is how this survived a whole release: `stagedThreadShapeFailure` reports a staging whose shape defeats the filter entirely, as a `threadSuppressionUnavailable` field on `dispatch-result.json` and a run-log warning, so "nothing to suppress" is no longer indistinguishable from "suppression silently did nothing." +Open-thread suppression was unreachable on every conforming run, so re-reviews re-posted findings an open bot thread already tracked. `openThreadsFromStaged` accepted an opener author of `github-actions[bot]` only, but `get_review_comments` — the tool review.md tells the orchestrator to copy `threads.json` from, verbatim — renders that same account as bare `github-actions`, and the filter fails closed, so a *correct* staging produced zero usable threads. It also read `path` from the thread while the tool carries `path` per comment, and `suppressOpenThreadDuplicates` matches on `path`, so a thread that cleared the author check still suppressed nothing. Both spellings are now accepted (`BOT_AUTHORS`, documented against the REST `user.login` surface that legitimately renders the bracketed form and that `stage-pr.ts` reads for prior reviews), and `path` falls back to the opening comment. Measured on webapp#41197's three-round seeded lifecycle, where suppression reported `threadSuppressions: []` in all three rounds while the re-reviews duplicated open threads: 6 of round 3's 8 comments landed on the exact path and line of a thread that was already open, and the reconciler's own `keep` list held those thread IDs in the same run, so the data suppression needed was present and unusable. Every unit fixture spelled the bot the way the code did, which is why the suite passed throughout; the regression cases now use the tool's real shape verbatim, assert an end-to-end suppression (with the blocking thread still flooring the verdict) rather than only the parse, and keep a human-opened thread refused so the widened author check cannot drop a finding outright. The prompt's selection layer is widened to match, which is the same premise one layer up: review.md told the orchestrator to stage "the unresolved `github-actions[bot]` threads" and to classify a human thread as "any author other than `github-actions[bot]`", both bracketed-only, so a literal reading could stage zero bot threads before the widened code filter ever ran — or worse, misfile a bot thread into `human-threads.json`, where it lands in `skipLines` and makes the submission drop a fresh finding on that line rather than merely duplicate one. Both instructions now name either spelling and say why. Second half of the fix, since a fail-open guard that cannot be seen failing is how this survived a whole release: `stagedThreadShapeFailure` reports a staging whose shape defeats the filter entirely, as a `threadSuppressionUnavailable` field on `dispatch-result.json` and a run-log warning, so "nothing to suppress" is no longer indistinguishable from "suppression silently did nothing." It counts staged threads per `thread_id` against the reconciler's resolved set rather than by list length, so a long `resolve` list cannot mask a total shape failure in a short staging, and it documents its own limit: one usable thread returns nothing, making it a total-failure tripwire rather than a per-thread audit. diff --git a/workflows/review/lib/dedup.test.ts b/workflows/review/lib/dedup.test.ts index dcfb9d7b..cc0e5225 100644 --- a/workflows/review/lib/dedup.test.ts +++ b/workflows/review/lib/dedup.test.ts @@ -3,6 +3,7 @@ import {describe, it, expect} from "vitest"; import { dedupeClaims, openThreadsFromStaged, + stagedThreadShapeFailure, suppressOpenThreadDuplicates, } from "./dedup"; import type {Claim} from "./dispatch-contracts"; @@ -790,6 +791,58 @@ describe("openThreadsFromStaged", () => { expect(result.suppressed[0]?.threadBlocking).toBe(true); }); + it("reports a staging whose shape defeats the filter entirely", () => { + // The tripwire itself. Untested, an edit flipping its condition would + // silently restore the webapp#41197 blindness this exists to catch. + const unusable = { + thread_id: "PRRT_1", + is_resolved: false, + comments: [{author: "some-human", body: "x", path: "a.ts"}], + }; + const failure = stagedThreadShapeFailure([unusable], [], new Set()); + expect(failure?.unusableThreads).toBe(1); + expect(failure?.warning).toContain("none usable"); + }); + + it("reports nothing when suppression had usable threads or no threads", () => { + const usable = { + thread_id: "PRRT_1", + is_resolved: false, + comments: [{author: "github-actions", body: "b", path: "a.ts"}], + }; + const open = openThreadsFromStaged([usable], new Set()); + expect(open).toHaveLength(1); + // A usable thread means suppression ran; nothing to report. + expect( + stagedThreadShapeFailure([usable], open, new Set()), + ).toBeUndefined(); + // No staging at all is the ordinary first-review case, not a failure. + expect(stagedThreadShapeFailure([], [], new Set())).toBeUndefined(); + expect( + stagedThreadShapeFailure(undefined, [], new Set()), + ).toBeUndefined(); + }); + + it("counts resolved threads per id, not by resolve-list length", () => { + // The reconciler's `resolve` list is never validated against the + // staged thread_ids, so a long list must not mask a total shape + // failure in a short staging by arithmetic alone. + const unusable = { + thread_id: "PRRT_staged", + is_resolved: false, + comments: [{author: "some-human", body: "x", path: "a.ts"}], + }; + const unrelatedResolves = new Set(["PRRT_a", "PRRT_b", "PRRT_c"]); + expect( + stagedThreadShapeFailure([unusable], [], unrelatedResolves) + ?.unusableThreads, + ).toBe(1); + // A staged thread the reconciler DID resolve is legitimately unusable. + expect( + stagedThreadShapeFailure([unusable], [], new Set(["PRRT_staged"])), + ).toBeUndefined(); + }); + it("still refuses a human-opened thread in the tool's shape", () => { // The author fix widens the accepted spellings; it must not widen them // to anyone. A human thread killing a candidate would drop a finding diff --git a/workflows/review/lib/dedup.ts b/workflows/review/lib/dedup.ts index ad698236..0fc8c979 100644 --- a/workflows/review/lib/dedup.ts +++ b/workflows/review/lib/dedup.ts @@ -335,53 +335,71 @@ export const describesOpenThreadDefect = ( ); }; -/** - * 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, 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). - */ /** * 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, and the caller only logs what it returns. + * 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. Threads the - * reconciler resolved this run are excluded from the count, since those are - * legitimately unusable. + * every run reported an empty suppression list and looked correct. + * + * 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[], - resolvedThisRun: number, -): {stagedThreads: number; warning: string} | undefined => { - const stagedThreads = Array.isArray(threads) ? threads.length : 0; - if (stagedThreads <= resolvedThisRun || openThreads.length > 0) { + 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 { - stagedThreads, + unusableThreads, warning: - `::warning title=open-thread suppression::${stagedThreads} staged thread(s), none usable ` + + `::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, 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). + */ export const suppressOpenThreadDuplicates = ( claims: Claim[], threads: OpenThread[], diff --git a/workflows/review/lib/dispatch.ts b/workflows/review/lib/dispatch.ts index b1508a21..98a0191e 100644 --- a/workflows/review/lib/dispatch.ts +++ b/workflows/review/lib/dispatch.ts @@ -343,7 +343,7 @@ export type DispatchResult = { */ threadSuppressions: ThreadSuppression[]; /** Set when every staged thread failed the filter (see dedup.ts). */ - threadSuppressionUnavailable?: {stagedThreads: number; warning: string}; + threadSuppressionUnavailable?: {unusableThreads: number; warning: string}; /** The reconciler's decision, when it ran and parsed. */ reconciliation?: {resolve: string[]; keep: string[]; skipLines: unknown}; /** correctness-reviewer `files[]` risk levels (Steps 7-8). */ @@ -850,7 +850,7 @@ export const runDispatch = async ( const threadSuppressionUnavailable = stagedThreadShapeFailure( threads, openThreads, - resolvedIds.size, + resolvedIds, ); if (threadSuppressionUnavailable !== undefined) { // eslint-disable-next-line no-console diff --git a/workflows/review/review.md b/workflows/review/review.md index 9a1b4d59..2359851f 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -569,8 +569,16 @@ exactly this sequence: (`pull_request_read` `get_review_comments`) and stage two files from them (leave all other threads untouched); the dispatcher's reconciler dispatch reads them from disk: - - `/tmp/gh-aw/review/threads.json` — the unresolved `github-actions[bot]` - threads. For each write `thread_id`, `path`, `line`, `resolved` (the + - `/tmp/gh-aw/review/threads.json` — the unresolved threads opened by THIS + bot. Treat **either** spelling of its author as the bot: + `get_review_comments` renders the account as bare `github-actions`, while + the REST review surfaces render the same account as + `github-actions[bot]`, so which one you see depends on the surface the + output came from. (Matching only the bracketed form is what made + open-thread suppression unreachable on every conforming run until the + code filter was widened: webapp#41197 re-posted findings against open + threads for three straight rounds.) For each write + `thread_id`, `path`, `line`, `resolved` (the thread's `is_resolved` from the `get_review_comments` output, copied verbatim; it is `false` for every thread that belongs in this file, but write it anyway: the dispatcher's open-thread suppression checks the @@ -589,10 +597,13 @@ exactly this sequence: `url` is what lets the re-review accountability section link each still-open thread to its prior comment. - `/tmp/gh-aw/review/human-threads.json` — the `{path, line}` of every - **unresolved thread started by a human** (any author other than - `github-actions[bot]`). These are never resolved or replied to; they - mark lines where a human review conversation is already open, so the - dispatcher defers there. + **unresolved thread started by a human**: any author that is neither + `github-actions` nor `github-actions[bot]` (both spellings are this bot, + per the note above). Getting this wrong is worse than a duplicate + comment: a bot thread misfiled here lands in `skipLines` and makes the + submission drop a fresh finding on that line. These are never resolved + or replied to; they mark lines where a human review conversation is + already open, so the dispatcher defers there. 2. If any staged bot thread's reply chain shows the author factually disputing a claim on the merits, write `/tmp/gh-aw/review/author-disputes.json`: a list of `{path, line, quote}`