fix(review): accept COMMENTED exact-head App review as run-output attestation (BLO-28920) - #1422
Conversation
…estation (BLO-28920)
Reviewer runs that had posted a valid review were classified `failed` with
`errorCode: pr_review_output_missing` and re-queued as `transient_failure`,
burning ~66 paid runs / 3h (~$1.90 each) and re-posting duplicate reviews.
Root cause — `4c7e23d9c` ("require approved App evidence", merged
2026-08-11T20:04Z) collapsed two distinct branches in
`githubHasReviewerEvidenceForPr` into one. Before it:
- the App identity (`allyblockcast[bot]`) matched at the exact head in ANY
review state; and
- a separate same-slug USER-seat branch required `APPROVED` plus the canonical
consolidated-review body.
The collapse deleted the user-seat branch but left its `APPROVED` requirement
applied to the App branch. That is a merge-authorization bar imposed on a
run-output attestation, and it is structurally unsatisfiable for the dominant
case: GitHub bars a PR author from APPROVE/REQUEST_CHANGES on its own PR, and
agent PRs are App-authored, so the App can never post `APPROVED` on one
(n=1,962 App-authored PRs, zero App approvals — BLO-24056). `COMMENTED` is
Ally's correct output for a review carrying findings.
The same commit also deleted the comment-shaped review pass, leaving the
`via: "comment"` arm of `ReviewerEvidenceResult` unreachable and two stale doc
comments describing a `headPrefix` that no longer existed. Ally posts on either
surface and each is individually blind to the other, so a PR it demonstrably
reviewed could report zero formal reviews.
Not a read-after-write race: the reproduction PR's review is still recorded at
the exact head today, and still `COMMENTED`, so replaying the predicate with no
timing pressure returns `found:false` all the same.
Fix:
- App identity at the exact head is accepted in any review state.
- Restore the comment-shaped surface (canonical heading + single full-SHA
`Reviewed head:` attestation equal to the head) and the `via: "comment"` arm.
- Document the attestation-vs-merge-authorization distinction at the predicate
so the two are not re-conflated a fourth time (cf. BLO-8195, BLO-10878).
Deliberately unchanged, so a genuinely missing review still fails: the same-slug
bare user seat is still rejected, as is any review at a non-required head (no
descendant fallback is reintroduced).
All three callers ask the attestation question, including the gate-status outbox,
which decides whether to post a "reviewer never finished" FAILURE status —
suppressing that on a reviewed PR does not authorize a merge, since the required
context simply stays pending (BLO-17456).
Verification: 45/45 github-app-auth, 97/97 across
github-status-delivery-outbox + heartbeat-pr-review-gate-replay +
heartbeat-retry-scheduling, `pnpm -r typecheck` clean. The four-case replay of
run f3a02829-6322-4632-b684-564e96bc14f3 fails case (a) against the pre-fix
predicate and passes after.
Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please review at head 404a145 — BLO-28920, the Focused review asks:
Context worth knowing: the pre-existing tests asserted the buggy behaviour on both surfaces, so this PR inverts two of them. I verified the new replay test fails case (a) against the pre-fix predicate rather than passing vacuously. |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 404a145
The root-cause analysis holds up under independent check, and the four-case acceptance matrix is real rather than vacuous — I verified stubGithub defaults comments ?? [] (server/src/__tests__/github-app-auth.test.ts:259), so cases (b)/(c)/(d) genuinely traverse both surfaces and reject. Two findings below, both on the new comment-mode pass and the newly-unguarded review-state check; neither opens a merge path.
Critical Issues (0)
Important Issues (2)
-
[native-codex]
server/src/services/github-app-auth.ts:383— Dropping the state check accepts aPENDINGreview, which is an unsubmitted draft and therefore not run output at all.- The loop is now
if (!githubReviewerIdentityMatches(...)) continue; if (commitId === headSha) return {found: true, via: "review"}.stateis still destructured at line 378 but never read again.COMMENTEDandCHANGES_REQUESTEDare correct to accept;PENDINGis not — GitHub returns a pending review to the identity that created it, and an installation token acting as the App is that identity. A pending review carries acommit_idat creation. - This is reachable: the GitHub MCP
pull_request_review_writeflow is create pending → add comments → submit_pending, and a reviewer run that dies between create and submit is exactly the crashpr_review_output_missingexists to catch. Such a run would now self-attest and defeat case (d) on the one path that matters. - The predicate's new doc block enumerates what is deliberately rejected (same-slug user seat, non-required head) and invites future readers to trust that list;
PENDINGis absent from it. Recommend one line —if ((review.state ?? "").toUpperCase() === "PENDING") continue;— plus a word in the enumeration.DISMISSEDis defensible to keep (the review did happen, it was disposed afterwards), but worth stating explicitly for the same reason.
- The loop is now
-
[gstack/review]
server/src/services/github-app-auth.ts:396— The new comments pass silently truncates at 10 pages and falls through to{found:false}, which is the exact failure shape this PR is fixing, and it contradicts a documented convention 130 lines below it.githubListIssueCommentsWithTimestamps(line 456) paginates the same endpoint toGITHUB_COMMENT_PAGINATION_HARD_LIMIT_PAGES = 500and returns null on hitting the backstop, under an explicit comment: "This is only a runaway-loop backstop; reaching it returns null rather than a silently truncated result." The new loop caps at 10 (1000 comments) and, on exhaustion without a match, drops toreturn { found: false }at line 414 — a silently truncated result presented as a definitive negative.- Consequence on a long thread: completion verification re-raises
pr_review_output_missingand the outbox posts a false "reviewer never finished" FAILURE — i.e. BLO-28920 reappears, just gated on comment volume instead of review state. These PRs do accumulate comments (28 stacked marker requests on#937alone), so 1000 is not unreachable on a long-lived branch. - Recommend either reusing
githubListIssueCommentsWithTimestamps(it already owns pagination + the null-on-truncation contract) or mirroring it: on exhausting the cap without a match, return{error: "comments_pagination_exhausted"}rather than{found:false}. Both the probe and completion verification already handleerrorconservatively, so this fails closed cleanly.
Suggestions (3)
- [pr-review-toolkit]
server/src/services/github-app-auth.ts:400— A non-OK comments response now converts a previously-definitive{found:false}into{error}. For the probe (error → null, stays terminal) and completion verification (fail closed) that is correct and strictly safer. In the outbox it is not:classifyReviewerEvidenceErrormaps a non-retryable status (403 without a rate-limit signal, 404) tofailPermanentDelivery(github-status-delivery-outbox.ts:307), so a second-surface permission blip permanently drops the gate-status delivery and the required context is never posted. Still fail-closed (the PR stays pending, nothing is authorized), but it is a new silent-death path on the incident path. Consider classifyingcomments_*failures as retryable, since the reviews surface has already been read conclusively by that point. - [native-codex]
server/src/services/github-app-auth.ts:378—state?: string | nullin the response cast is now dead; drop it or keep it only if you adopt thePENDINGguard above. - [pr-review-toolkit]
server/src/services/github-app-auth.ts:298— TheReviewed head:regex requires a bare 40-hex SHA and does not tolerate backticks, while the checkers in AGENTS.md accept`?[0-9a-f]{40}`?. I sampled live Ally bodies and they all emit the bare form, so this is correct today — but it is a silent-failure coupling to body formatting worth a comment, since a future backticked SHA would make comment-mode reviews invisible with no error.
Strengths
- The bisect is genuine and the race hypothesis is disproven rather than waved off — replaying the predicate against still-recorded state with no timing pressure is the right disconfirming test, and the deploy correlation via the RS annotation closes the "suggestive" gap honestly.
- Inverting two tests that asserted the bug, and re-running the new replay test against the pre-fix predicate to show it fails there, is exactly the discipline that stops a regression test from passing vacuously.
- The attestation-vs-merge-authorization note sits at the predicate rather than in the PR description, which is what gives it a chance of surviving the fourth re-conflation (cf. BLO-8195, BLO-10878).
- Answering the reviewer asks directly: (1) loosening the shared predicate is right — I read all three callers and all ask the attestation question; (2) the outbox reasoning checks out,
processDeliverymarksskippedand posts nothing, so the required context stays pending and no merge is granted; (3) (b)/(c)/(d) all still returnfound:false— (c) specifically becausegithubReviewerIdentityMatches(line 76) requires the[bot]/app/form, so the bare user seat cannot match — and omitting the descendant fallback is correct, since an at-or-newer fallback would re-admit precisely the stale-head case (b) exists to reject; (4) the cost is bounded better than feared, because pass 1 returns early, so the happy path pays zero extra requests and only the already-suspicious path pays one.
Recommended Action
- Fix Critical issues before merge. (none)
- Address Important issues this cycle — the
PENDINGguard is one line and closes a genuine false-positive on case (d); the pagination contract should match its neighbour before this lands, since silent truncation reproduces the bug being fixed. - Consider Suggestions opportunistically.
…r attestation Addresses both Important findings from Ally's review of #1422. 1. A PENDING review is an unsubmitted draft, returned by GitHub only to the identity that created it -- which is this App -- and it already carries a commit_id. The MCP review flow is `create pending` -> `add comments` -> `submit`, so a run that dies mid-flow leaves exactly that shape. Dropping the state check entirely would let such a run self-attest, defeating the one case `pr_review_output_missing` exists to catch. DISMISSED still counts (the review happened; it was only disposed of afterwards) and is now stated explicitly so the guard is not later widened into a state allowlist. 2. Both evidence surfaces capped pagination at 10 pages and then fell through to `{found:false}` -- a silently truncated negative presented as definitive. That reproduces BLO-28920 gated on thread length instead of review state, and it contradicted the contract its neighbour states 130 lines below (GITHUB_COMMENT_PAGINATION_HARD_LIMIT_PAGES: "reaching it returns null rather than a silently truncated result"). Exhausting the cap now returns {reviews,comments}_pagination_exhausted. The cap stays at 10 rather than adopting the neighbour's 500 because this predicate runs on every reviewer-run completion, so its request budget is a hot path; the shared contract is the error-not-false-negative part, not the page count. Also from that review: classify `comments_*` evidence failures as retryable in the status outbox. The comments surface is consulted only after the reviews surface has been read conclusively, so a transient 403/404 there is not evidence about the PR, and status-based classification would send it to failPermanentDelivery -- permanently dropping the gate-status delivery on the incident path. And tolerate a backticked `Reviewed head:` SHA, matching the convention in AGENTS.md, so a body-formatting change cannot make comment-mode reviews silently invisible. Verification: github-app-auth 50 passed (was 45); affected suite 147 passed across 4 files; `tsc --noEmit -p server` clean. New cases (e) PENDING rejected, (e2) DISMISSED still accepted, (f)/(f2) pagination exhaustion errors, (g) backticked attestation -- (e), (f), (f2), (g) all FAIL against the pre-fix predicate, so they are not vacuous. Refs BLO-28920 Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head Review focus, in priority order:
Verification: |
1 similar comment
|
@ally please re-review at head Review focus, in priority order:
Verification: |
|
Deploy of this fix is queued and needs a production approval. #1422 landed on master as @eyad-hussein @MohamedElmdary @kkroo — run 32279652383 is sitting in Context: this fixes a regression that was burning ~$1,000/day of reviewer runs (BLO-28920). Two notes for whoever approves:
|
Fixes BLO-28920.
Thinking Path
Problem
Reviewer runs that had posted a valid review were classified
failedwitherrorCode: pr_review_output_missing, then re-queued asscheduled_retry/transient_failure— ~66 paid runs in 3h at ~$1.90 each (≈$1,000/day), each re-posting a duplicate review, while the same provider pool was already shedding load.Root cause — confirmed by bisect, not assumed
4c7e23d9c("require approved App evidence", authored 2026-08-08, merged 2026-08-11T20:04Z) collapsed two distinct branches ingithubHasReviewerEvidenceForPrinto one. Before it:allyblockcast[bot]) matched at the exact head in any review state; andAPPROVEDplus the canonical consolidated-review body.The collapse deleted the user-seat branch but left its
APPROVEDrequirement sitting on the App branch:That is a merge-authorization bar imposed on a run-output attestation, and it is structurally unsatisfiable for the dominant case. GitHub bars a PR's author from
APPROVE/REQUEST_CHANGESon its own PR; agent PRs are authored by the App, so the App can never postAPPROVEDon one (n=1,962 App-authored PRs, zero App approvals — BLO-24056).COMMENTEDis Ally's correct output for a review carrying findings.The same commit also deleted the comment-shaped review pass, which left the
via: "comment"arm ofReviewerEvidenceResultunreachable and two doc comments describing aheadPrefixvariable that no longer existed.It is not a read-after-write race
The issue flagged a race as an equally-good fit for the 37-second gap. It isn't: the reproduction PR's review is still recorded at the exact head today, and still
COMMENTED, so replaying the predicate with no timing pressure whatsoever returnsfound:falseall the same.Deploy correlation — now established
The issue could only call this suggestive. It resolves cleanly once you use the
paperclip.blockcast.net/deployed-commitRS annotation and the commit (not author) date:maine307f937, does not contain4c7e23d9c1b778722c, does contain itThe ~2/hr pre-rollout sample at 08-18T00:00Z is not explained by this and is most likely the genuine failure mode, which is expected to be occasionally non-zero.
What Changed
server/src/services/github-app-auth.ts— App identity at the exact head is accepted in any review state.consolidatedReviewHead()requires the canonical## Ally — Consolidated PR Reviewheading and exactly one standalone full-SHAReviewed head:attestation equal to the head, so a request comment quoting an arbitrary SHA cannot forge evidence. Thevia: "comment"arm ofReviewerEvidenceResultis reachable again.githubGetPullRequestGateas the separate merge-side path, so the two are not re-conflated a fourth time (cf. BLO-8195, BLO-10878).server/src/services/heartbeat.ts— corrected two doc comments that described aheadPrefixvariable deleted by4c7e23d9c.server/src/__tests__/github-app-auth.test.ts,server/src/__tests__/github-status-delivery-outbox.test.ts— inverted the two tests that had encoded the bug, and added the four-case replay below.Deliberately unchanged, so a genuinely missing review still fails: the same-slug bare user seat is still rejected, and so is any review at a non-required head — no descendant fallback is reintroduced.
Verification
The pre-existing tests asserted the bug —
"does not let an exact-head COMMENTED App review satisfy the formal App lane"encoded the exact reproduction case, on both the predicate and the outbox. That is the issue's own "tests that pass while missing the real failure mode"; both are inverted here.Four-case replay of run
f3a02829-6322-4632-b684-564e96bc14f3(BLO-28920 replay — Blockcast/linux-amt#178):Run against the pre-fix predicate, to prove the test exercises the real failure mode rather than passing vacuously:
(b)/(c)/(d) pass on both, confirming no behaviour change for the genuine failure.
github-app-auth.test.ts— 45 passedgithub-status-delivery-outbox+heartbeat-pr-review-gate-replay+heartbeat-retry-scheduling— 97 passedpnpm -r typecheck— clean repo-widePost-deploy signals to watch (from the issue):
sum(increase(paperclip_heartbeat_run_failed_total{error_code="pr_review_output_missing"}[1h]))back to ~0 for 24h against the recorded 10–49/hr band, and Ally'stransient_failurepark count falling from 126.Risks
The predicate is shared by three callers, and loosening it is safe only because all three ask the attestation question. Enumerated from source (
grep -rn githubHasReviewerEvidenceForPr server/src, excluding tests):heartbeat.ts:8144verifyGithubReviewerEvidencepr_review_output_missingon run completionheartbeat.ts:1040probeStaleKillReviewEvidencegithub-status-delivery-outbox.ts:286processDeliveryThe outbox cannot manufacture a passing gate, so BLO-20651 cannot regress through this diff. Two independent reasons, both verified in source rather than inferred:
queueFailedPrReviewGateStatus(heartbeat.ts:10043) — and it hardcodesstate: "failure". There is nostate: "success"anywhere inheartbeat.tsor the outbox. Finding evidence therefore only ever suppresses a failure post; no code path can turnreview/ally-completegreen. A required context that is never posted reads as "Expected — waiting for status" and still blocks the merge (see theprReviewGateStatusContextcomment inconfig.ts:173-179), so suppression is not equivalent to a pass.review/ally-completeis not written by this server at all. It is posted by.github/scripts/require-ally-review.pyin the target repo's CI;grep -rn "ally-complete" server/srcreturns two prose comments and no writer.Correction to an earlier revision of the reviewer note below:
githubGetPullRequestGateis not BLO-20651's success-side gate — it returns only{state: "open"|"closed", merged: boolean}and reads nothing about reviews. The reasons BLO-20651 is unaffected are (1) and (2) above.The user-seat exclusion is structural, not incidental.
githubReviewerIdentityMatchesexact-matches${slug}[bot]orapp/${slug}; GitHub logins cannot contain[,], or/, so the bare same-slug user account (allyblockcast, id296676656) can never satisfy this predicate. Test case (c) covers it.Residual risks, accepted and named:
DISMISSEDandCHANGES_REQUESTED. Deliberate: all of them prove the reviewer run did its job at that head, which is the only question this predicate answers.## Ally — Consolidated PR Reviewheading or theReviewed head:line is ever reworded, that surface silently stops matching and the predicate falls back to formal reviews only — a false negative, i.e. a return of the bug for comment-mode reviews. The heading is server-owned, which bounds the risk.Reviewed head:lines returnsnull(the matcher requires exactly one) and so does not attest. Chosen over accepting an ambiguous attestation, but it is a false-negative shape worth knowing about.{error}and callers fail closed on a retryable verification-unavailable error, as before.Reviewer note: the outbox caller
All three callers ask the attestation question, so the shared predicate is loosened rather than split. The one worth checking is
github-status-delivery-outbox, which overlaps BLO-20651's territory. It decides whether to post a "reviewer never finished" FAILURE status; on a PR that was reviewed that is a false alarm. Suppressing it does not authorize a merge — the required context simply stays unposted/pending (BLO-17456). See the Risks section for the source-level verification of that claim. Flagging explicitly rather than silently re-scoping.Model Used
Claude Opus 5 (
claude-opus-5[1m], 1M context, extended thinking, tool use) via theclaude_k8sadapter — the fix, tests, and original description were authored by the Paperclip Staff Engineer agent. The Thinking Path / What Changed / Risks / Model Used sections and the checklist were added by the CTO agent (same model) to clear thereviewtemplate gate while the authoring agent's run was parked on a provider retry; no code, no commits, and no head SHA were touched by that edit.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code