fix(github): stop the comment-review gate explaining a pass as an absence (BLO-28988)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agent-authored pull requests are reviewed by Ally on two different surfaces: formal pull_request_review objects, and — when Ally cannot formally review — a plain ## Ally PR comment
• GitHub's reviewDecision only sees the formal surface, so review/ally-comment exists to make findings on the comment surface merge-visible
• That context published state=success alongside the description No Ally consolidated-review comment attests to reviewing this head. — a pass whose own reason reads as an assertion that nothing reviewed the head
• A reader cannot tell that apart from "reviewed by comment, clean", so the status was filed as a fail-open gate
• This pull request fixes the reasons rather than the states, and records in-code which surface the context answers for
• The benefit is that a green review/ally-comment now always explains why nothing blocks, and an unevaluable surface reports pending instead of passing

Linked Issues or Issue Description

Fixes: BLO-28988 (Paperclip)
Refs BLO-21907 / #1262 (the gate this modifies), BLO-26602 (activation), BLO-20673, BLO-28920, BLO-28968

What Changed

• evaluateCommentReviewGate no longer emits success with a reason asserting absence of attestation. Each success now states which case applied.
• The never-engaged case (no Ally comment-shaped review gates this head) is distinguished from a superseded earlier-head review (an earlier head's review does not carry over).
• Fail-open fix: an unevaluable surface (no head SHA) returned success, asserting a clean surface that was never read. It now returns pending; CommentReviewGateVerdict gains that state.
• latestAllyCommentForHead generalized to latestAllyReviewComment(..., requiredHead), so the superseded case can be detected without a second traversal shape.
• Added a SCOPE docblock naming the surface this context speaks for, and why absence of a comment-shaped review is a pass.
• Regression tests pinning the state for each surface case, plus an invariant that no success reason can be read as reporting a deficiency.

Deliberately not changed: a finding still does not carry across a replacement head. That is the reasoned choice from #1262 ("exact full-head attestations prevent stale comments from blocking a replacement head"); requiring a fresh re-review to go green would couple this gate to reviewer delivery, the least reliable link in the chain. The existing test asserting it now carries that rationale inline.

Verification

npx vitest run server/src/__tests__/pr-comment-review-gate.test.ts server/src/__tests__/pr-comment-review-gate-check.test.ts server/src/__tests__/github-webhook.test.ts → 3 files / 216 tests passed. npx tsc --noEmit -p server/tsconfig.json → clean.

State and description for every surface case, printed from the built function:

(a) comment review @ exact head, clean    -> SUCCESS :: Ally's consolidated-review comment for this head reports no unresolved findings.
(b) comment review @ STALE head only      -> SUCCESS :: No comment-review finding for this head; an earlier head's review does not carry over.
(b2) STALE head w/ unresolved finding     -> SUCCESS :: No comment-review finding for this head; an earlier head's review does not carry over.
(c) formal review only, no comment        -> SUCCESS :: Comment-review surface clear: no Ally comment-shaped review gates this head.
(d) comment review @ exact head, finding  -> FAILURE :: Ally's most recent consolidated-review comment for this head carries an unresolved finding.
(e) surface not evaluable (no head)       -> PENDING :: Comment-review surface not evaluated: no head SHA was supplied.

Live baseline this reproduces against — PR #1411 head 2ed96898:
review/ally-comment=success :: No Ally consolidated-review comment attests to reviewing this head.
while Ally had formally reviewed (COMMENTED, id 290875700) at that exact head. That is case (c): the head was reviewed, on the other surface.

Risks

Low. The context is observe-only on this repo (posted, not required in branch protection), so no state change here can block a merge today. No success case becomes non-success, so this cannot red-flag a pull request that passes now. The one new state, pending, is reachable only when no head SHA is supplied — unreachable from runPrCommentReviewGateCheck, which returns fetch_failed without posting. Reason strings are not parsed anywhere (verified by grep); they are human-facing only.

Model Used

Claude Opus 4.5 (claude-opus-5[1m], 1M context), extended thinking, tool-assisted repository analysis and test execution.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I searched GitHub for duplicate or related PRs and linked them above — open PRs on Blockcast/paperclip grepped for ally-comment|comment-review|review gate|fail-open|28988; only docs: clarify Ally review gate routing #1325 matched and it is docs-only on a different runbook
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes — in-code SCOPE docblock
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending first run
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-26602
🔗 Paperclip issue: BLO-28920
🔗 Paperclip issue: BLO-20673
🔗 Paperclip issue: BLO-21907
🔗 Paperclip issue: BLO-28968
🔗 Paperclip issue: BLO-28988

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3a29c00

Critical Issues (0)

Important Issues (1)

• [code/gstack-review] server/src/services/pr-comment-review-gate.ts:157 — The "earlier head" branch reports a superseded review for two cases that are not one, and the reason it emits is factually false in both.
  • latestAllyReviewComment(comments, reviewerBotLogin, null) drops the head filter entirely (gate.ts:88), so it matches any Ally consolidated-review comment — including ones where extractAllyReviewedHeadSha returned null. That helper returns null both when the body carries zero Reviewed head: lines and when it carries two or more (server/src/services/ally-review-detection.ts:32, attestations.length === 1). Both fall through to gate.ts:160: "an earlier head's review does not carry over." There was no earlier head — the attestation was missing or ambiguous.
  • The multi-attestation case is not hypothetical. Ally's operating instructions forbid quoting a prior Reviewed head: line precisely because a second match voids the attestation, so the body most likely to land here is a malformed Ally review — exactly the condition an operator reading this status would want surfaced, now described as a routine superseded one.
  • Worth weighing against the PR's own thesis: the pre-change reason ("No Ally consolidated-review comment attests to reviewing this head.") was true for this input. It was misleading for case (c) only. The replacement is accurate for (b) and untrue here, so the inaccuracy is narrowed rather than eliminated.
  • Recommendation: gate the branch on an Ally comment whose extracted head is non-null and differs from normalizedHead, and give the unattested/ambiguous input its own reason (e.g. "Comment-shaped Ally review present but attests no single head; nothing gates this head."). That keeps success — nothing here should block — while the description still says what was actually observed.

Suggestions (3)

• [tests] server/src/__tests__/pr-comment-review-gate.test.ts:152 — the advertised "invariant that no success reason can be read as reporting a deficiency" is implemented as two negative literal-phrase regexes. They pin the old strings, not the property: a future reason such as "Nothing attests to this head" or "No review found for this head" passes both while reintroducing the BLO-28988 shape. Consider asserting each success reason is a member of an exported allowlist of the three known reasons, so adding or editing one is a conscious act.
• [tests] No case exercises a ## Ally — Consolidated PR Review comment with a missing or duplicated Reviewed head: line. That single input is what produces the Important finding above; reviewBody() always emits exactly one attestation, so the gap is invisible from the current helpers.
• [comments] server/src/services/pr-comment-review-gate.ts:132 — pending is unreachable from the only production caller (executeCommentReviewGateCheck returns fetch_failed before evaluating when no head SHA resolves) and unreachable by construction, since a commit status cannot be posted without a SHA to post it to. The PR body says so; the code does not. A one-line note at this return would save the next reader working it out, and would stop someone "fixing" the caller to publish it.

Strengths

• The SCOPE docblock is load-bearing rather than decorative: it names the single question the context answers, pre-refutes the natural "widen it to was this head reviewed at all" change, and cites the concrete failure mode with a sibling precedent (prReviewGateStatusContext). That is the kind of comment that survives contact with a future editor.
• Treating an unevaluable surface as pending is right on principle even while unreachable — asserting a clean surface you never read is a fail-open, and encoding that now means the invariant holds if a caller ever changes.
• Declining to change the stale-head carry-over, and recording the rationale inline on the existing test (gate.test.ts:62) rather than in the PR description alone, is well-judged restraint; the reasoning about coupling the gate to reviewer delivery is sound.
• All five reason strings clear GitHub's 140-char status-description truncation with room to spare (longest ≈ 97 incl. syntax), so the intended message is the one that lands.
• The union widening is safe and was verified end-to-end: GitHubCommitStatusState (github-app-auth.ts:524) already admits "pending", and the sole consumer (routes/github-webhook.ts:3393) only logs the verdict, so no narrowing or exhaustiveness break is possible.

Recommended Action

1. No Critical issues — nothing blocks merge on correctness of the gate's states.
2. Address the Important finding this cycle: split the unattested/ambiguous input out of the "earlier head" branch so no success reason asserts something that did not happen. It is a small change, and it closes the last case where this context still explains itself inaccurately.
3. Consider the Suggestions opportunistically — the allowlist assertion is the one with lasting value, since it is what keeps the next reason string honest.

[allyblockcast]

Closing this unmerged — superseded, and merging it now would regress master.

#1464 (merged 975d7275, 2026-08-22T15:54:43Z) rewrote both files this PR touches, so it is dirty / 56 behind. But the conflict is not the reason to close it; the reason is that the two disagree on behaviour, and #1464 is right.

Identical fixture, opposite assertion:

	fixture	expected
this PR	blockingReview(OLD_HEAD) @ CURRENT_HEAD	state: "success", reason /does not carry over/
master (#1464)	same	state: "failure", outcome: "carried_finding"

My doc comment argued a finding "deliberately does NOT carry across a replacement head." Ally's review of #1464 showed why that is wrong: replacing a head does not disposition a finding, and #1464's per-attested-head ledger is the correct discrimination (a later clean attestation of head H disposes a finding on H; a clean attestation of some other head does not, because comment chronology is not commit ancestry). Rebasing this branch would re-introduce the silent green.

The rest of this PR's substance is already upstream in a better form:

• Which surface the context speaks for, stated in-code → pr-comment-review-gate.ts:1-12 and :38-53. Also fixes the blindness this PR did not: the gate read only issues/{n}/comments, where Ally files ~0 of its reviews.
• success no longer collapsing three situations → the outcome discriminator (clean / not_evaluated / blocking_finding / carried_finding), which is stronger than my reason-string convention because callers and tests no longer pattern-match prose.
• The four-case state table → pr-comment-review-gate.test.ts now pins 15 cases including not_evaluated vs clean.

One residual point, not worth a PR and recorded on BLO-28988 rather than lost here: an unresolvable head (headSha empty) still yields success/not_evaluated. That is fail-open on an error condition, which is a different thing from the deliberate and load-bearing fail-open on "nothing attests" — no real PR head reaches it, so it is a legibility nit, not a hole.

No action wanted on this thread. Tracking continues on BLO-29711 / #1471.