Skip to content

[🔥AUDIT🔥] review: fix open-thread suppression, unreachable on every conforming run #302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jwbron merged 2 commits into from
Jul 30, 2026
Merged

[🔥AUDIT🔥] review: fix open-thread suppression, unreachable on every conforming run #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6181b33
[jwies/review-thread-suppression-author] review: fix open-thread supp…
jwbron Jul 30, 2026
4ed269b
[jwies/review-thread-suppression-author] review: address the bot revi…
jwbron Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/review-thread-suppression-author.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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. 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.
138 changes: 138 additions & 0 deletions workflows/review/lib/dedup.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {describe, it, expect} from "vitest";
import {
dedupeClaims,
openThreadsFromStaged,
stagedThreadShapeFailure,
suppressOpenThreadDuplicates,
} from "./dedup";
import type {Claim} from "./dispatch-contracts";
Expand Down Expand Up @@ -718,4 +719,141 @@ 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("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
// 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([]);
});
});
86 changes: 79 additions & 7 deletions workflows/review/lib/dedup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,22 +230,40 @@ const threadProse = (body: string): string =>
const stagedResolvedState = (thread: Record<string, unknown>): 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"]);

@github-actions github-actions Bot Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): BOT_AUTHORS fixes the code filter, but the selection layer above it is unchanged: review.md still tells 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. Since get_review_comments renders the bot bare, an orchestrator following that literally could stage zero bot threads (or route them into human-threads.json) before this widened filter ever runs, and with stagedThreads == 0 the new reporter returns undefined, so that variant stays invisible. The prompt-side spelling is worth widening to match — arguably the same premise the PR diagnoses, one layer up.


/**
* 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.
* 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, 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,
Expand All @@ -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"]
Expand Down Expand Up @@ -311,6 +335,54 @@ export const describesOpenThreadDefect = (
);
};

/**
* 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.
*
* 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 = (

@github-actions github-actions Bot Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): stagedThreadShapeFailure and its dispatch wiring have no test coverage — no *.test.ts references stagedThreadShapeFailure or threadSuppressionUnavailable. None of its three branches (the <= exemption, the openThreads.length > 0 short-circuit, the warning path) nor the console.error/DispatchResult wiring in dispatch.ts is pinned. The existing dispatch suppression tests all stage a usable thread, so the reporter returns undefined and the new path is never exercised. A later edit flipping <= to < or dropping the short-circuit would silently re-introduce the webapp#41197 duplicate-posting blindness this reporter exists to catch, with no failing test — worth a direct unit test plus one dispatch case that stages an all-unusable set.

Low-confidence (1)
  • workflows/review/lib/dedup.ts:242 — the twin human-threads.json classification is keyed only on the bracketed github-actions[bot] spelling (review.md), with no code-side author guard. A bot thread staged as bare github-actions could be misclassified as human, land in skipLines, and cause submission.ts to drop a fresh finding on that line — the "silently kill a candidate" case the new dedup.ts comment warns against. Speculative: the misclassification is LLM-mediated at the prompt selection layer.

threads: unknown,
openThreads: readonly OpenThread[],
resolvedIds: ReadonlySet<string>,
): {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:
`::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
Expand Down
28 changes: 21 additions & 7 deletions workflows/review/lib/dispatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import {
dedupeClaims,
openThreadsFromStaged,
stagedThreadShapeFailure,
suppressOpenThreadDuplicates,
type ClaimMerge,
type ThreadSuppression,
Expand Down Expand Up @@ -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?: {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). */
Expand Down Expand Up @@ -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,
);
if (threadSuppressionUnavailable !== undefined) {
// eslint-disable-next-line no-console
console.error(threadSuppressionUnavailable.warning);
}

// Phase 3: claim validation.
let validatorRan = false;
Expand Down Expand Up @@ -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} : {}),
Expand Down
Loading
Loading