bc-36xx.26: ReviewThreads and resolveThread swallow a transient GraphQL failure the… - #655
bc-36xx.26: ReviewThreads and resolveThread swallow a transient GraphQL failure the…#655mordam wants to merge 6 commits into
Conversation
…pty answer
`gh api graphql` — the same API that flapped on 2026-08-17 and cost bc-36xx.14 — backs
both reviewThreads and resolveThread, and neither told a transient failure apart from a
genuine one: reviewThreads returned null identically for "GitHub is down" and "this pull
request has no review threads", and resolveThread returned { resolved: false, reason:
<gh's stderr> } identically for a 503 and a thread that plainly does not exist.
Both now retry through a shared `ghGraphql` helper that reuses `isTransientErr`,
`PROBE_RETRIES` and `PROBE_BACKOFF_MS` — the same classifier `seenBy` already retries the
repo probe with — rather than inventing a second one for the same shape of failure on the
same API. If the outage outlasts the retries: `threadsTransient(dir, number)` is
`probeTransient`'s counterpart for reviewThreads, keyed by pull request since one checkout
answers for several; resolveThread's failure shape gains a `transient: true` field instead,
since it already returns an object rather than null on every failure.
Extends the existing suite (test/pr.mjs) with a fake gh that serves a bounded run of 503s
on `api graphql`, mirroring the `repoOutage` fixture bc-36xx.14 added for `repo view`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ead — until now Three functions from earlier beads (#533, #560, bc-36xx.22) built the seam for GitHub-side review thread resolution and nothing ever called it: reviewThreads and resolveThread (lib/pr.js) had zero production callers, and the review block's own threadId field — added for exactly this — was written by nothing. An independent reviewer (#620, bc-g1oo3 comment c4) found the same gap from the opposite end, auditing the loop rather than building it. lib/reviewsync.js gets two additions to close it. `threadIdsFor(verdict, threads)` matches a fresh verdict's inline comments to the GitHub threads `reviewThreads` reads back after the review that created them — by path, line and the exact text `inlineComments` sent, never by position, since GraphQL's `reviewThreads(first: 100)` makes no promise about order. `reviewFromVerdict` takes an optional `threadIds` map and anchors it onto each comment; passing nothing produces the exact shape it always did, so every existing caller is unaffected. lib/mergequeue.js's `syncReviewVerdict` wires both directions. After a review with inline comments goes out, it reads the threads back and anchors them. Before folding a fresh verdict onto the block, it diffs the prior round's comments against the fresh one: any comment the worker answered that the reviewer's new verdict no longer carries forward is "answered and accepted", and gets resolved on GitHub under the reviewer identity before it is gone from the bead for good. An unanswered comment a reviewer simply drops does not count — only an `answer` field makes it settled, not just absent from the new list. Both `prApi.reviewThreads`/`resolveThread` are checked with `typeof … === 'function'` before use, the same way `reviewerFor` already is, so a fake or a prApi shape that predates this degrades exactly as it did before — no comment gets anchored, nothing gets resolved, and everything else is unchanged. I ran the whole suite via `bin/b7e-gate` in this worktree (420/420 after `node scripts/vendor.js`, which a fresh worktree needs and is unrelated to this diff — public/vendor is gitignored). test/pr.mjs, test/reviewsync.mjs and test/mergequeue.mjs all extended in place rather than given new sections, per this bead's own acceptance criteria. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
A beadcause worker opened this and does not merge its own work. It is on the merge queue as bc-45rd5. |
NeanderthalMan
left a comment
NeanderthalMan
left a comment
There was a problem hiding this comment.
Not approved on #655. Reviewed by the ReviewAdvocate — an agent, not Adam. 3 comments, 1 blocking. threadsTransient answers false when the GraphQL outage hits gh repo view via slugFor rather than the thread query itself — measured — so the one distinction bc-36xx.26 asks for is absent in the outage shape the bead cites; fix the flag (not necessarily its caller) and I approve.
- c1 (blocking)
lib/pr.js:1689— reviewThreads only touches threadsTransientKeys after slugFor(dir) and reviewerContext(dir) have both succeeded, so an outage that 503sgh repo view(slugFor -> resolve -> seenBy, which is GraphQL) returns null with threadsTransient(dir, number) === false. Measured in a sandbox with the existing repoOutage knob: reviewThreads -> null, threadsTransient -> false, probeTransient -> true. The same ordering leaves the delete below those bails, so a stale true survives a later call that never reached GraphQL, contradicting the comment above this line. bc-36xx.26's acceptance criterion is that a caller can tell an outage apart from a genuine empty answer, and the outage the bead cites (2026-08-17, quoted in this PR's own README) is precisely the one where repo view was what 503'd — so the flag reads 'no threads' in the case it was built for. The new test/pr.mjs cases do not catch it because graphqlOutage gates only the api graphql branch of the fake. - c2 (suggestion)
lib/reviewsync.js:148— The three-field match is not pinned by any test: rewriting this predicate top.path === c.filealone leaves all 20 reviewsync checks and all 78 mergequeue checks passing, because the THREADS fixture's two threads differ by path. Separately,threadsis every thread on the PR rather than only the ones just created, so a comment carried forward verbatim into round 2 produces a second thread with a byte-identical body andfindreturns whichever GraphQL lists first — normally round 1's. The exactness of this match is the design's central safety argument — the README says a wrong anchor closing the wrong thread later is worse than the gap this replaced — and right now a later simplification to path+line would ship silently. A fixture with two threads sharing path and line but differing in body would pin it, and preferring an unresolved thread (reviewThreads already returnsresolved, which threadIdsFor drops at line 143) would settle the carried-forward case. - c3 (suggestion)
lib/mergequeue.js:996— resolveThread never throws — it returns { resolved: false, reason } — so.catch(() => null)catches nothing and the result, including thetransient: truethe first half of this PR just added, is discarded. Four lines laterstate.commentsis replaced wholesale, taking the threadId with it. The anchoring read above has the same shape: on a null from an outage every comment is written unanchored and, as the comment says, there is no second tick. The two signals bc-36xx.26 adds have no consumer at their only production caller, so a 503 or a permission refusal leaves the thread open on GitHub with nothing on the bead recording it was meant to close. The cost is cosmetic (open threads a human sees) rather than a wrong merge, which is why this is not blocking — but checking res?.resolved, and keeping a comment whose resolve came back transient, is the difference between the classifier being wired and merely existing.
Submitted as NeanderthalMan, which is the identity the reviewer speaks as here and not a person. The verdict this came from is on bc-45rd5, round 1. Adam has not read this diff.
| // so a flag a caller reads right after this call reflects THIS read, never a leftover | ||
| // from an outage that has since cleared. | ||
| const key = threadsKey(dir, number); | ||
| threadsTransientKeys.delete(key); |
There was a problem hiding this comment.
blocking — reviewThreads only touches threadsTransientKeys after slugFor(dir) and reviewerContext(dir) have both succeeded, so an outage that 503s gh repo view (slugFor -> resolve -> seenBy, which is GraphQL) returns null with threadsTransient(dir, number) === false. Measured in a sandbox with the existing repoOutage knob: reviewThreads -> null, threadsTransient -> false, probeTransient -> true. The same ordering leaves the delete below those bails, so a stale true survives a later call that never reached GraphQL, contradicting the comment above this line. bc-36xx.26's acceptance criterion is that a caller can tell an outage apart from a genuine empty answer, and the outage the bead cites (2026-08-17, quoted in this PR's own README) is precisely the one where repo view was what 503'd — so the flag reads 'no threads' in the case it was built for. The new test/pr.mjs cases do not catch it because graphqlOutage gates only the api graphql branch of the fake.
| for (const c of verdict.comments || []) { | ||
| if (!c.file || !Number.isInteger(c.line) || c.line <= 0) continue; | ||
| const body = inlineBody(c); | ||
| const hit = posted.find((p) => p.path === c.file && p.line === c.line && p.body === body); |
There was a problem hiding this comment.
suggestion — The three-field match is not pinned by any test: rewriting this predicate to p.path === c.file alone leaves all 20 reviewsync checks and all 78 mergequeue checks passing, because the THREADS fixture's two threads differ by path. Separately, threads is every thread on the PR rather than only the ones just created, so a comment carried forward verbatim into round 2 produces a second thread with a byte-identical body and find returns whichever GraphQL lists first — normally round 1's. The exactness of this match is the design's central safety argument — the README says a wrong anchor closing the wrong thread later is worse than the gap this replaced — and right now a later simplification to path+line would ship silently. A fixture with two threads sharing path and line but differing in body would pin it, and preferring an unresolved thread (reviewThreads already returns resolved, which threadIdsFor drops at line 143) would settle the carried-forward case.
| const freshIds = new Set((verdict.comments || []).map((c) => String(c.id))); | ||
| const settled = (state.comments || []).filter((c) => c.threadId && c.answer && !freshIds.has(String(c.id))); | ||
| for (const c of settled) { | ||
| await prApi.resolveThread(dir, c.threadId).catch(() => null); |
There was a problem hiding this comment.
suggestion — resolveThread never throws — it returns { resolved: false, reason } — so .catch(() => null) catches nothing and the result, including the transient: true the first half of this PR just added, is discarded. Four lines later state.comments is replaced wholesale, taking the threadId with it. The anchoring read above has the same shape: on a null from an outage every comment is written unanchored and, as the comment says, there is no second tick. The two signals bc-36xx.26 adds have no consumer at their only production caller, so a 503 or a permission refusal leaves the thread open on GitHub with nothing on the bead recording it was meant to close. The cost is cosmetic (open threads a human sees) rather than a wrong merge, which is why this is not blocking — but checking res?.resolved, and keeping a comment whose resolve came back transient, is the difference between the classifier being wired and merely existing.
…ead query c1 (blocking) was right, and reproduced before it was fixed: `reviewThreads` resolved the repo and the reviewer identity *before* it touched `threadsTransientKeys`, and both of those go through `seenBy` — `gh repo view`, which is GraphQL as well. That is the call the 2026-08-17 outage actually 503'd, on roughly four requests in five, so the likeliest way this function meets an outage is to never reach its own query at all — and it answered `threadsTransient === false` there, "this pull request genuinely has no threads", in precisely the case the flag was built to name. The clear now happens above every bail, and every early return consults `probeTransient(dir)` and inherits its answer rather than guessing at the same classification a second time. Two new checks in test/pr.mjs are the reviewer's own measurement, kept: one that the flag is true under `repoOutage`, one that a later read which dies before GraphQL for an ordinary reason does not leave a stale true behind it. c2 (suggestion), both halves. The three-field match in `threadIdsFor` was pinned by nothing — the review measured `p.path === c.file` alone passing all 20 reviewsync checks and all 78 mergequeue ones — so THREADS now carries three decoy threads, each differing from the real one in exactly one matched field and each listed before it. Dropping any single field from the predicate now returns a decoy; all four mutations were run and all four fail. And the carried-forward case is settled rather than left to GraphQL's ordering: a comment repeated verbatim in a later round matches two threads on all three fields, so the tie goes to an unresolved thread over a resolved one, then to the highest comment `databaseId` — the thread this review just made. c3 (suggestion) is declined on scope and filed as bc-36xx.33, with the reviewer's own sanction for that ending. bc-36xx.26 asks only that a caller *can* tell an outage apart from a real answer — its description says so outright — and the fix c3 sketches means a comment the reviewer dropped surviving into the next round's block, which the worker brief, the merge queue's "waiting on the worker" refusal and the card renderer all read. It also only ever retries on a later verdict, since `syncReviewVerdict` runs only when `freshVerdict` returns something. That is a design decision, not a patch.
|
The beadcause review loop stopped here: The reviewer and the worker did not agree in 2 rounds of review, which is as many as this gets. 2 comments are still unresolved. It is Adam's call now — see bc-45rd5. |
Adam's instruction for the review gate is two claims joined by "while": a veto is dequeued and dealt with separately **while** the merge queue continues to drain. They fail independently and only one of them was ever in doubt, so this pins them independently — test only, no production code touched. **1. The dequeue is real.** The two escalation checks already in `test/mergequeue.mjs` assert the raise with a spy, which cannot tell an escalation that hands the bead over from one that merely says it did. This one wires the real `raiseMergeCard` in, so what is asserted is `merge-queue` actually coming off the bead and `human` going on. **2. The drain survives it.** Three queued, the vetoed one at the *head* of the tick — the arrangement where a `break` in place of the `continue` would look exactly like a quiet afternoon. The two behind it merge in the same sweep, do not lose a merge slot to the veto, and go all the way through to their closes rather than being merged and left half-filed. `sweepMergeQueue` sorts the tick by bead id (`lib/mergequeue.js:404`), so the fixture's *names* are what put the veto first. The draft that named it `zz-veto` sorted it past the two-merge budget and it was never judged at all — worth knowing before writing any other multi-row tick test. **What this settles, and what it does not.** "The queue continues to drain" is now proven and needs no further work: it is `continue`, not `break`. The whole of the remaining work is the strand, which is `bc-i9nz7` — still open, still unanswered, and deliberately not started here. **Filed on the way past: `bc-uxrix` (P1).** `raiseMergeCard` adds the `human` label; the sweep's only read is `bd.listAgent`, which runs `--exclude-label human`. So a merge-bead leaves the only list the sweep reads the instant it becomes a card. `cardedFor` — the take-a-card-back loop of bc-91srt/bc-91ft — selects on exactly that label and can therefore never have run in the daemon, and `strandedPrs` counts every card as a strand, which is the `#661 ... no merge-bead is about it` line repeating in the log. Five green tests cover the reclaim because `fakeBd.listAgent` returns its rows unfiltered. Verified against the live workspace. Not fixed here: waking `cardedFor` up while the review gate is off would take #655 (CLEAN, green) straight back onto the queue and merge it over a reviewer's refusal, which is bc-i9nz7's call, not a side effect to land by accident. **Tests:** node bin/b7e-gate --json in this worktree over the merged tree: 424/424 green, no reds. test/mergequeue.mjs alone is 77/77, up from 75 — the two new checks are 'A VETO IS DEQUEUED FOR REAL' and 'AND THE QUEUE BEHIND IT STILL DRAINS IN THE SAME SWEEP'. **Worth knowing:** Test-only; no production file is touched. The one way it could be wrong is a fixture that does not mean what it says, which is why the first check wires the real raiseMergeCard in rather than a spy, and why the second asserts the merge numbers and the close order rather than just a count. **Left undone:** bc-i9nz7 is still unanswered and the strand itself is untouched — #661 and #655 are both still stranded, deliberately. bc-uxrix (P1) is the cause I found and did not fix: cards are invisible to the sweep's own read, so cardedFor has never run and strandedPrs calls every card a strand. It must not be fixed on its own while the review gate is off — it would merge #655 over a reviewer's veto. <details><summary><b>Files changed</b> — 1 file · +70 −0 · against `main`</summary> ``` test/mergequeue.mjs +70 −0 ``` </details> --- _Opened by a beadcause worker session on bc-36xx.34 — *Pin both halves of a review veto: the dequeue is real, and the queue drains on past it in the same sweep*. It merges itself once the checks report; merging is what closes the bead. If this is still open, something stopped that, and the reason is on bc-36xx.34 and in Adam's inbox._ bead: bc-36xx.34
…d-outage-ka5y26 # Conflicts: # lib/mergequeue.js # test/mergequeue.mjs
2 participants
reviewThreads and resolveThread (lib/pr.js) speak
gh api graphql— the same API thatflapped on 2026-08-17 and cost bc-36xx.14 — and used to fold a transient failure and a
genuine empty answer into the same result: reviewThreads returned null identically for
"GitHub is down" and "no review threads exist", and resolveThread returned the same
{resolved:false, reason}shape for a 503 and for "no such thread". Both now retrythrough a shared
ghGraphqlhelper that reusesisTransientErr/PROBE_RETRIES/PROBE_BACKOFF_MS— the classifierseenByalready retries the repo probe with — ratherthan inventing a second one, and surface the difference (
threadsTransient(dir, number),or
resolveThread'stransient: true) so a caller stops reading an outage as "nothing toresolve."
The second half was the actual dead end this epic left behind: those two functions had
zero production callers and the review block's own
threadIdfield, added across threemerged beads specifically to anchor a comment to its GitHub thread, was written by
nothing — an independent reviewer (#620) found the same gap auditing the loop from the
other end.
lib/reviewsync.jsgainsthreadIdsFor(verdict, threads), matching a freshverdict's inline comments to the threads
reviewThreadsreads back after the review thatcreated them (by path, line and the exact posted text, never by position — GraphQL makes
no order promise).
lib/mergequeue.js'ssyncReviewVerdictanchors them after a reviewgoes out, and — the other direction — resolves on GitHub any prior-round comment the
worker answered that a fresh verdict no longer carries forward, before that comment is
gone from the bead's own notes for good.
Both beads' scope was measured against the actual code rather than assumed from the
brief: the plan expected them on one branch because they share lib/pr.js and .31 is the
caller .26 protects, which held. I split the work into two commits along that same line —
the outage-hardening lib/pr.js needed no knowledge of the wiring, and the wiring's own
commit message documents which comment is "settled" and why an unanswered one is not.
What I'm least sure of: whether "answered and accepted" should also require the comment
not already carry
resolved: truefrom a prior tick — in the current design a droppedcomment never survives into a later block to be re-checked, so double-resolving looks
structurally impossible rather than merely untested, but it is the one behavior in here I
did not find an existing precedent to lean on.
Tests: node test/pr.mjs, test/reviewsync.mjs, test/mergequeue.mjs individually (all green) and the full suite via bin/b7e-gate (420/420, one pre-existing fresh-worktree failure in test/pagealias.mjs fixed by running node scripts/vendor.js, which is gitignored and unrelated to this diff)
Files changed — 7 files · +452 −37 · against `main`
Opened by a beadcause worker session on bc-36xx.26 — reviewThreads and resolveThread swallow a transient GraphQL failure the same way the repo probe used to. It merges itself once the checks report; merging is what closes the bead. If this is still open, something stopped that, and the reason is on bc-36xx.26 and in Adam's inbox.
bead: bc-36xx.26