[🔥AUDIT🔥] review: fix open-thread suppression, unreachable on every conforming run - #302
Merged
Merged
Conversation
…ression, unreachable on every conforming run `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.
🦋 Changeset detectedLatest commit: 4ed269b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
khan-actions-bot
requested review from
a team,
jaredly and
somewhatabstract
and removed request for
a team
July 30, 2026 21:36
Contributor
Review GuidanceOther risky files (2 files)
Excluded from review (1 file)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * reconciler resolved this run are excluded from the count, since those are | ||
| * legitimately unusable. | ||
| */ | ||
| export const stagedThreadShapeFailure = ( |
Contributor
There was a problem hiding this comment.
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 twinhuman-threads.jsonclassification is keyed only on the bracketedgithub-actions[bot]spelling (review.md), with no code-side author guard. A bot thread staged as baregithub-actionscould be misclassified as human, land inskipLines, and causesubmission.tsto 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.
| * the candidate's re-confirmation at blocking severity is what makes the | ||
| * floor more than a stale thread). | ||
| */ | ||
| /** |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): This inserts stagedThreadShapeFailure and its JSDoc between suppressOpenThreadDuplicates's doc block (ends line 354) and the function itself (now line 385), leaving two stacked doc blocks and suppressOpenThreadDuplicates with no adjacent doc. Every other exported fn in this file keeps its JSDoc directly above it. Move stagedThreadShapeFailure + its doc above the suppressOpenThreadDuplicates block (or below the whole function) so IDE/typedoc hover binds the rationale to the right declaration.
| resolvedThisRun: number, | ||
| ): {stagedThreads: number; warning: string} | undefined => { | ||
| const stagedThreads = Array.isArray(threads) ? threads.length : 0; | ||
| if (stagedThreads <= resolvedThisRun || openThreads.length > 0) { |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): resolvedThisRun here is resolvedIds.size from the caller (dispatch.ts:853) — the raw length of the reconciler's resolve list, which dispatch.ts only filters to strings, never validating against staged thread_ids. So stagedThreads <= resolvedThisRun can hold even when zero staged threads were actually resolved, suppressing the very fail-open warning added to catch a total shape failure. Mirroring the per-thread resolvedIds.has(thread["thread_id"]) check at line 283 (count only staged threads actually in resolvedIds) would make the diagnostic exact. Separately, || openThreads.length > 0 means any one usable thread masks all rejected ones, so partial shape drift stays invisible by design — acceptable as a total-failure tripwire, but worth a comment noting the limit.
| * 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"]); |
Contributor
There was a problem hiding this comment.
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.
…ew on the suppression fix 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.
jwbron
added a commit
that referenced
this pull request
Jul 30, 2026
… dispatch.ts, which is over its line cap (#304) main is red on lint, and neither PR that caused it could have seen it. `@khanacademy/eslint-config` sets max-lines to 1000. #302 took workflows/review/lib/dispatch.ts to exactly 1000 lines; the single line #299 added to SHED_RANKING took it to 1001. Each PR was green against its own base, so the violation existed only in the merge, which is invisible to a per-PR lint run. Every open PR in the repo inherits the failure, including #300. Fixed by extracting a concern rather than by reclaiming a line, so the next addition does not land in the same place, and rather than raising the cap, which lives in the shared Khan config and would be a house-rule deviation. DEFAULT_FINDERS, SHED_RANKING, Roster, RosterShed and computeRoster move to dispatch-roster.ts and are re-exported from dispatch.ts, which already advertises one import surface for the dispatch machinery. The moved code is byte-identical; dispatch.ts goes 1001 -> 920 lines and the new module is 108. Verified: 1573 tests pass unmodified, typecheck clean, and eslint over everything CI lints (actions, utils, workflows, minus the ignorePatterns paths) reports zero errors. Worth recording for next time: a plain local `pnpm run lint` cannot reproduce CI inside a git worktree under .claude, because eslint skips dot-directories by default and silently ignores the whole tree; `--resolve-plugins-relative-to . --no-ignore` scoped to the CI paths is what actually reproduces it.
Merged
Contributor
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus cc11988c0918 (9 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
This was referenced Jul 31, 2026
jwbron
added a commit
that referenced
this pull request
Jul 31, 2026
… from the gate, where it annotates `stagedThreadShapeFailure` writes `threadSuppressionUnavailable` to dispatch-result.json and prints a `::warning`, but the dispatcher runs inside the agent's Bash tool, where a workflow command is only text. Measured on webapp#41204 run 30654454047, a deliberately mis-staged threads.json: the field carried `unusableThreads: 9`, the warning text reached the run log and the step summary, and zero annotations across the run's six jobs mentioned suppression. In the same run the pre-agent staging step's own `::warning` did annotate, which is the contrast that locates the cause. The dispatch-conformance gate re-emits it. The gate is compiled into `post-steps`, runs `if: always()`, already reads every out/ file, and its own `::error`/`::warning` lines annotate today. The line is rebuilt from the numeric `unusableThreads`, never forwarded as stored text: the gate step is trusted while out/ is a directory the agent can write, so a stored string could carry newlines and inject `::error` or `::add-mask` into it. Absent, non-numeric, or non-positive forwards nothing, keeping this as quiet as the tripwire itself. The formatter is shared with dedup.ts so the two texts cannot drift, and the forwarding lives in lib/forwarded-warnings.ts because dispatch-gate.ts is at its 1000-line ceiling (it lands at exactly 1000 with the wiring). This matters more after this PR, not less: a conforming code staging can no longer trip the tripwire, so a fire now means the producer and consumer drifted inside one repo, which is the failure class #302 was.
jwbron
added a commit
that referenced
this pull request
Jul 31, 2026
…e 1) (#308) * [jwies/review-stage-threads] review: stage the review threads deterministically (orchestrator slice 1) `threads.json` / `human-threads.json` move out of the prompt and into `lib/stage-pr.ts`, which is the last load-bearing staging review.md Step 3 still asked the orchestrator to perform. Everything downstream depended on a model-produced file: `hasThreads` (which gates the thread-reconciler dispatch, so it changes the roster), open-thread suppression, and the accountability recap. This is the seam #302 patched a symptom of. There, the prompt selected bot threads by one spelling of the bot's login while `openThreadsFromStaged` admitted another, so a *conforming* staging produced zero usable threads and suppression silently never ran for a whole release. The worse direction was never hit but was always available: `human-threads.json` was specified as "any author other than the bot", and a bot thread misfiled there lands in `skipLines`, which makes the submission DROP a fresh finding on that line rather than merely duplicate one. The staging step now does one GraphQL fetch of every unresolved review thread and partitions it by opener. GraphQL rather than REST because REST exposes neither a thread's resolution state nor the `PRRT_...` node id the resolve safe output takes. Producer and consumer share one bot-identity predicate (`lib/threads.ts`'s `isReviewBotAuthor`, comparing suffix-stripped so REST's `github-actions[bot]` and GraphQL's bare `github-actions` are one account), so the two layers can no longer spell the identity differently, which is the actual defect rather than its symptom. The fetch, its paging and its fail-closed guards live once and are shared with autofix's staging, which had the only copy and whose comments carry both prior postmortems (Khan/webapp#41140's `threadCount: 0`, and GitHub answering a rate limit with HTTP 200 plus an `errors` array). Autofix's `collectThreads` is now that shared fetch plus its by-opener filter, with no behavior change. A failed thread fetch fails the staging step rather than degrading to `[]`: an empty staging drops the flip gate's `keptBlockingCount` to zero, and a reduced-depth re-review may then flip a prior REQUEST_CHANGES to APPROVE past still-open blocking threads nobody read. The step runs before any AI spend and GraphQL's HTTP-200 rate limit is retried. `dedup.ts`'s fail-closed guards stay rather than trusting the new producer, as does the `stagedThreadShapeFailure` tripwire: a conforming code staging can no longer trip it, which is the point, since a fire now means either producer/consumer drift inside one repo or a staging that came from the eval's own producer. One fail-open closed while here: a thread with no opener is staged in neither file, since staging it as human would put a `skipLines` entry on a line that may be the bot's own. * [jwies/review-stage-threads] review: close four fail-open seams the staging review found All six notes on #308 were non-blocking; these are the five worth code, plus the test the sixth asked for. - A thread whose opening comment has `author: null` (a deleted account) maps to `""`, which is a login rather than an absence, so it matched no bot and took the HUMAN path. That is the misfiling direction that matters: a `skipLines` entry on a line that may be the bot's own drops a fresh finding there. The adjacent comment already claimed such a thread is staged in neither file; now it is. - `REVIEW_BOT_LOGIN` makes the identity deployment config, matching `AUTOFIX_BOT_LOGIN` and `REVIEW_SWEEP_BOT_LOGIN` (same default). A consumer posting under its own App could not change a compiled-in constant, so every one of its bot threads would have misfiled as human. - `baseLogin` case-folds BEFORE stripping `[bot]`, so a `…[BOT]` spelling cannot read as a different account. Theoretical today; free to close. - A page claiming `hasNextPage` with no cursor returned the threads collected so far, the partial staging every other guard in the module refuses. It now throws. Refusing still terminates, which is all the infinite-loop guard wanted, so autofix's test for it changes from "stops" to "refuses". - The HTTP-200 `RATE_LIMITED` retry moves into `threads.ts` beside the fetch. It was built inline under `require.main === module`, so no test could reach it and autofix's port had none at all, meaning autofix died on its first throttle. Now both workflows inherit it and four tests cover it: retry then succeed, give up, propagate a non-healing error without spending a retry, and back off. Left alone deliberately: code-enforced per-lens comment caps, which the reviewer raised on #307 and that PR's body already prices as the follow-up. Tests 1445 pass (up from 1383, no test removed), typecheck clean, lint clean. * [jwies/review-stage-threads] review: say where REVIEW_BOT_LOGIN goes The README named the variable but not the block that reaches both readers (the staging step and the dispatcher share the workflow-level `env:`). * [jwies/review-stage-threads] review: re-emit the suppression tripwire from the gate, where it annotates `stagedThreadShapeFailure` writes `threadSuppressionUnavailable` to dispatch-result.json and prints a `::warning`, but the dispatcher runs inside the agent's Bash tool, where a workflow command is only text. Measured on webapp#41204 run 30654454047, a deliberately mis-staged threads.json: the field carried `unusableThreads: 9`, the warning text reached the run log and the step summary, and zero annotations across the run's six jobs mentioned suppression. In the same run the pre-agent staging step's own `::warning` did annotate, which is the contrast that locates the cause. The dispatch-conformance gate re-emits it. The gate is compiled into `post-steps`, runs `if: always()`, already reads every out/ file, and its own `::error`/`::warning` lines annotate today. The line is rebuilt from the numeric `unusableThreads`, never forwarded as stored text: the gate step is trusted while out/ is a directory the agent can write, so a stored string could carry newlines and inject `::error` or `::add-mask` into it. Absent, non-numeric, or non-positive forwards nothing, keeping this as quiet as the tripwire itself. The formatter is shared with dedup.ts so the two texts cannot drift, and the forwarding lives in lib/forwarded-warnings.ts because dispatch-gate.ts is at its 1000-line ceiling (it lands at exactly 1000 with the wiring). This matters more after this PR, not less: a conforming code staging can no longer trip the tripwire, so a fire now means the producer and consumer drifted inside one repo, which is the failure class #302 was.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🖍 This is an audit! 🖍
Summary:
openThreadsFromStagedaccepted an opener author ofgithub-actions[bot]only.
get_review_comments, the tool review.md tells the orchestrator tocopy
threads.jsonfrom verbatim, renders that same account as baregithub-actions, and the filter fails closed — so a correct stagingyielded zero usable threads and suppression never ran. It also read
pathfrom the thread while the tool carries
pathper comment, andsuppressOpenThreadDuplicatesmatches onpath, so a thread that clearedthe 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 there-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
keeplist 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:
stagedThreadShapeFailurereports a staging whoseshape defeats the filter, as
threadSuppressionUnavailableondispatch-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.
Test plan
vitest run workflows/review/lib/dedup.test.ts: 23 pass (20 before; 3 new).vitest run workflows/review/lib/dispatch.test.ts: 30 pass.tsc --noEmitclean;eslintclean on all three changed files (dispatch.tslands at exactly its 1000-line ceiling, which is why the new condition lives indedup.ts).main, unrelated and untouched here:lib/dispatch-runner.test.ts,eval/live-ab.test.ts, andeval/rereview-sweep.test.tsfail identically on a cleanorigin/mainworktree (7 tests, same set).Follow-up
The deeper issue this exposes is that
threads.jsonis still prompt-staged:stage-pr.tsdefers it ("Phase 2; a later slice"), so slice 3 moved the decision into code while leaving its input model-produced. This PR makes the resulting fail-open visible rather than closing that seam. Staging threads in code is the real fix and belongs with the remaining slice work.