review: stage the review threads deterministically (orchestrator slice 1) - #308
Merged
Conversation
…nistically (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.
🦋 Changeset detectedLatest commit: 64e0082 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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,
jeresig and
kevinb-khan
and removed request for
a team
July 31, 2026 17:17
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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 |
…taging 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.
The README named the variable but not the block that reaches both readers (the staging step and the dispatcher share the workflow-level `env:`).
… 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.
jeresig
approved these changes
Jul 31, 2026
Contributor
Review Guidancegithub-actions (7 files)
Common patterns2 files: Empty-threads GraphQL stub inserted as the new third argument to every pre-existing 2 files: Inline 2 files: Thread-fetch utilities extracted from Excluded from review (5 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
Comment on lines
+60
to
+63
| if (typeof count !== "number" || !Number.isFinite(count) || count <= 0) { | ||
| return []; | ||
| } | ||
| return [threadSuppressionUnavailableWarning(Math.floor(count))]; |
Contributor
There was a problem hiding this comment.
nitpick (non-blocking): The count <= 0 guard runs before the floor, so a fractional unusableThreads (e.g. 0.5, reachable only via an agent-rewritten out/ file — the adversarial input this module is built around) passes the guard and Math.floor(0.5) renders a 0 staged thread(s), none usable annotation, the zero-count warning the doc comment says should never be emitted. Floor first, then check:
Suggested change
| if (typeof count !== "number" || !Number.isFinite(count) || count <= 0) { | |
| return []; | |
| } | |
| return [threadSuppressionUnavailableWarning(Math.floor(count))]; | |
| if (typeof count !== "number" || !Number.isFinite(count)) { | |
| return []; | |
| } | |
| const n = Math.floor(count); | |
| if (n <= 0) { | |
| return []; | |
| } | |
| return [threadSuppressionUnavailableWarning(n)]; |
Lower-confidence observations (below the posting bar)
workflows/review/lib/threads.ts:66— thought: the shared-predicate identity guarantee is code-level only; at runtime it relies onREVIEW_BOT_LOGINreaching both the staging step and the dispatcher identically (mitigated by the README's workflow-levelenv:guidance, worst case a visible duplicate).workflows/review/lib/threads.ts:65— question: env-scopedREVIEW_BOT_LOGINreopens an identity seam between the two processes; staging the resolved login and warning on drift would close it in code.workflows/review/lib/threads.ts:123— thought: suffix-stripped login equality treatsnameandname[bot]as one, which can only collide once a customREVIEW_BOT_LOGINApp is configured; fetching__typenameto require a Bot actor would close it.
| isResolved | ||
| path | ||
| line | ||
| comments(first: 100) { |
Contributor
There was a problem hiding this comment.
note (non-blocking): comments(first: 100) fetches no pageInfo, so a thread with more than 100 comments stages a truncated reply chain with no refusal — at odds with this module's otherwise fail-closed stance (the outer reviewThreads page throws on a missing cursor just above). A >100-comment thread is extreme, but the truncation would silently drop a late author dispute that the reconciler weighs. Consider asserting comments.pageInfo.hasNextPage !== true or paginating the inner connection.
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
2 participants
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.
Summary:
threads.json/human-threads.jsonmove out of the prompt and intolib/stage-pr.ts, completing deterministic-orchestrator slice 1. That moduledeliberately deferred them ("Phase 2; a later slice"), so review.md Step 3 asked
the ORCHESTRATOR to fetch the unresolved threads and write both files in a
particular shape, and everything downstream depended on a model-produced file:
dispatch.tsreads it forhasThreads(which gates the thread-reconcilerdispatch, so it changes the roster) and for open-thread suppression, and
lib/rereview.tsreads it for the accountability section.That seam is the failure class #302 patched a symptom of. There, the prompt
selected bot threads by one spelling of the bot's login while
openThreadsFromStagedadmitted another, so a conforming staging producedzero usable threads and suppression silently never ran for a whole release. The
worse direction was never hit but was always available:
human-threads.jsonwasspecified 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 thatline rather than merely duplicate one.
What the staging does now. One GraphQL fetch of every unresolved review
thread, partitioned by opener: threads this bot opened (full reply chain, bodies
byte-for-byte,
resolved: false, the opener'shtml_url) intothreads.json,everyone else's
{path, line}intohuman-threads.json, deduped and skippingthreads with no RIGHT-side line to skip. A thread is in exactly one file and
neither list is assembled by hand. GraphQL rather than REST because REST exposes
neither a thread's resolution state nor the
PRRT_...node id theresolve-pull-request-review-threadsafe output takes; the workflow's existingpull-requests: readGITHUB_TOKEN covers it, as autofix already demonstrates.One identity, shared by both layers. Producer and consumer now go through
lib/threads.ts'sisReviewBotAuthor, which compares suffix-stripped so REST'sgithub-actions[bot]and GraphQL's baregithub-actionsare one account.dedup.tscalls it instead of its ownBOT_AUTHORSset, so the two layers canno longer spell the identity differently. That is the actual #302 defect rather
than its symptom, and it is why this is more than moving prose into code.
One fetch, not two. The GraphQL query, 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 staged
threadCount: 0on a PR carrying five threads; GitHub answers a rate limit withHTTP 200 plus an
errorsarray). Autofix'scollectThreadsis now that sharedfetch plus its by-opener filter: same signature, same output, its 30 tests
unmodified. A second hand-written copy in the reviewer would have had to
re-derive both bugs.
Fail-closed, deliberately not
[]. A failed thread fetch fails the stagingstep. Staging
[]is not the conservative direction here: it drops the flipgate's
keptBlockingCountto zero, and a reduced-depth re-review may then flip aprior REQUEST_CHANGES to APPROVE past still-open blocking threads nobody read
(
submission.ts'skeptBlockingFloor). The step runs before any AI spend,GraphQL's HTTP-200
RATE_LIMITEDis retried (the status-based retry cannot seeit), and the review re-runs on the next push.
dedup.ts's guards (bot-authoredopener, explicit
resolved: false) stay rather than trusting the new producer,so a producer bug degrades to a duplicate comment, never to a dropped finding.
stagedThreadShapeFailureis kept, not deleted: a conforming code staging can nolonger trip it, which is the point, since a fire now means either that producer
and consumer have drifted inside one repo (a code bug, still the #302 class) or
that the staging came from the eval's own producer or a hand-built reproduction.
One fail-open closed while here: a thread with no opener at all is staged in
NEITHER file. Unreachable on a real PR, but staging it as human would put a
skipLinesentry on a line that may be the bot's own.review.md Step 3 keeps exactly one thread job: reading the reply chains for an
author's factual dispute, which is a judgment.
.github/workflows/review.mdisuntouched; the installed copy is pinned at
review-v1.7.0and gets bumped in itsown PR.
Test plan:
pnpm exec vitest run workflows: 1383 pass, 0 fail, including autofix's 30thread tests unmodified against the shared fetch. Repo-wide, the only red files
are
actions/filter-filesandactions/fix-workflows, which fail at import onmissing
picomatch/yamlin this environment; both are untouched here andfail the same way on
main.lib/stage-threads.test.ts(10 cases, fixtures in the API's own shape: aPRRT_...node id, GraphQL's baregithub-actions). These pin the produceragainst the consumers it feeds rather than against the shape a reader would
expect, because "each layer looked right on its own" is how [🔥AUDIT🔥] review: fix open-thread suppression, unreachable on every conforming run #302 shipped:
openThreadsFromStaged, which must returna usable thread, with
stagedThreadShapeFailuresilent;hasThreadsstill gates the reconciler, asserted both directions through thereal
computeRoster;bot spellings accepted, opener bodies verbatim including CRLF and trailing
whitespace, resolved threads dropped, absent
urlomitted, human threadswithout a line skipped and duplicates collapsed, paging followed;
errorsarray and a malformed body eachfail the staging with nothing staged,
routing.jsonnever written.get_review_commentsfixture indedup.test.tsstill passes:it now covers the tolerance path for stagings that do not come from
stage-pr.ts(the eval's producer, hand-built reproductions)..claude:git ls-files ... | xargs pnpm exec eslint --no-ignore --resolve-plugins-relative-to .over
actions utils workflows config, minus theignorePatternscorpus trees,reports 0 errors. Every touched file stays under the 1000-line cap; the new
tests are their own file for that reason (
stage-pr.test.tswould have hit 930).pnpm run typecheckis clean, though note it does notincludeworkflows/;I also ran
tscdirectly over the touched review/autofix modules and it reportsno new errors (the pre-existing ones under the repo's strict flags are unchanged).
Not run: a live trial. This changes who produces a staged file, not the review's
judgment, and the producer/consumer bind is asserted in unit tests; a re-review
lifecycle run is still the honest confirmation that suppression and the
accountability recap see the threads on a real PR.