fix(review-gate): refuse a re-anchored review whose body attests a different head (PEN-3413)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Its GitHub review gate decides whether a PR has reviewer evidence at its current head before the landing routine may enqueue it
• githubHasReviewerEvidenceForPr credited any formal review whose commit_id equals the head, but GitHub rewrites commit_id after a force-push, so a review of an older tree can be credited as evidence for the new head
• Two live PRs (fix(heartbeat,adapter-k8s): a 429 behind a replacement Job is the throttle, not k8s_pod_schedule_failed (BLO-34577) #1936, fix(claude-k8s): cap tool-spawned children with RLIMIT_DATA so an orphaned Bash child cannot OOM-kill the run (BLO-34477) #1937) carried approvals that attested a different head than the one they were stamped on, and one sat in the merge queue on that basis
• This pull request makes the consumer refuse a stamped-at-head review whose body attests a different head (or several), while still accepting bodyless reviews on the stamp alone
• The benefit is that the gate again proves a reviewer looked at the exact tree being landed, without reintroducing the BLO-28920 retry loop for off-template reviews

Linked Issues or Issue Description

• Closes PEN-3413 (Paperclip issue; the investigation and the two candidate mechanisms are written up below)
• Related: BLO-32844 (review submitted to the wrong PR, refused producer-side) and PEN-2865 (re-review superseding a prior approval, not addressed here)

Closes PEN-3413.

The bypass

githubHasReviewerEvidenceForPr credited a formal review on commit_id === headSha alone. That stamp is not an immutable record of what was reviewed, so the check designed to prove "a reviewer looked at this exact tree" was crediting approvals of trees the reviewer never read.

PR	review	commit_id	body-attested head
#1936	5258246839	f6bbb959	3a019931
#1937	5261008302	c890164a	3d9cfb2b

Both APPROVED, both by allyblockcast[bot] (App identity). #1936 was sitting in the master merge queue carrying one.

The mechanism, settled

PEN-3413 named two candidates it could not separate and asked for the distinction in writing, because they have different fixes. Measured 2026-09-21:

	review submitted	commit behind commit_id authored	force-push	gap
#1936 5258246839	2026-09-19T23:12:46Z	f6bbb959 @ 2026-09-20T22:23:44Z	22:33:50Z	+23h11m
#1937 5261008302	2026-09-20T16:03:47Z	c890164a @ 2026-09-21T02:19:58Z	02:21:25Z	+10h16m

Candidate 1 — "Ally submits without pinning a SHA, so GitHub stamps the then-current head (a TOCTOU between fetch and submit)" — is refuted. In both cases the stamped commit did not exist when the review was submitted. A commit authored 23 hours later cannot have been the head at submit time.

Candidate 2 — GitHub re-anchors commit_id after the fact — is confirmed. Corroborating detail:

• Both attested commits are diverged from the new head (compare/3a019931...f6bbb959 → diverged, 4 behind / 27 ahead; 3d9cfb2b...c890164a → diverged, 9 behind / 35 ahead), i.e. orphaned by the force-push.
• Of the 9 Ally reviews across the two PRs, exactly the 2 sitting on the old tip moved. The older intermediate reviews (fix(heartbeat,adapter-k8s): a 429 behind a replacement Job is the throttle, not k8s_pod_schedule_failed (BLO-34577) #1936 eb445a62; fix(claude-k8s): cap tool-spawned children with RLIMIT_DATA so an orphaned Bash child cannot OOM-kill the run (BLO-34477) #1937 19ff304f / fac058ce / f6a323e9 ×2 / 6eba574a) all kept stamps equal to their own attestations.
• The reviewed entry in the issue timeline renders the same rewritten field, so it corroborates nothing — it is not an independent record.

This is the part that decides where the fix belongs. Since the rewrite lands hours after submission, the producer-side remedy PEN-3413 floated — "re-read the head immediately before submit and abort if it moved" — cannot prevent this class. Only a consumer can refuse it. I have therefore not built it.

What I have not established: GitHub's internal rule. "Re-anchors the old-tip review onto the new tip on force-push" fits both cases (n=2) and is inferred from review ordering — the head_ref_force_pushed timeline event carries only the new SHA, so the pre-push tip is not readable directly. The fix does not depend on the rule being exactly that; it depends only on commit_id being mutable, which is now measured rather than assumed.

This corroborates a ruling the repo had already made twice on other reads — pr-review-head-attestation.ts ("GitHub rewrites commit_id when a branch is updated") and evidence-truth.ts (CLEAN keyed on the attestation, never on commit_id). githubHasReviewerEvidenceForPr was the remaining consumer that still trusted the stamp. Distinct from BLO-32844, which is a review submitted to the wrong PR and is refused producer-side.

What Changed

A review at the head whose body attests a different head — or several distinct heads — no longer satisfies the predicate.

Narrow on purpose. PEN-3413's suggested shape was to require both the attestation and commit_id to equal the head. That would fail every bodyless or off-template App review and reproduce the BLO-28920 false pr_review_output_missing retry loop (~66 runs / 3h) that this function's header exists to prevent. So a body carrying no attestation still passes on commit_id alone; only a body that names a different head is refused. Several distinct heads is refused too, on the same fail-closed rule the grammar's exactly-one check already encodes.

extractAllyReviewedHeadShas is added alongside extractAllyReviewedHeadSha rather than replacing it. The singular form collapses "no attestation" and "several" into null — correct for its three existing callers, which must fail closed on ambiguity, and exactly the bit this consumer needs separated. Those callers are untouched.

The agent-facing copy of the same rule (gateSignals, which is the implementation for the hand-performed pr:…:review gate re-check) is corrected to match, with a guard test so a size-motivated trim fails loudly rather than silently reinstating the bypass.

Not addressed here, deliberately: PEN-3413's third suggestion, that a re-review should dismiss or supersede the prior approval. That is a separate change and closest to PEN-2865.

Verification

Failing-before / passing-after, run both ways:

# with the guard reverted to the pre-change predicate
Tests  2 failed | 79 passed (81)
  x PEN-3413: rejects a stamped-at-head review whose body attests a different head
  x PEN-3413: rejects a stamped-at-head review whose body attests several distinct heads

# with the guard
Tests  160 passed (160)   # github-app-auth + ally-review-detection + validators/issue

Exactly the two rejection tests flip. The three PEN-3413 positive controls — attests-the-same-head, attests-no-head-at-all, and a re-anchored review not masking a clean sibling — pass in both runs, which is what shows the rejection is not just the guard breaking every bodied review.

• pnpm typecheck — exit 0
• Downstream consumers of the same modules, all green (172 tests): evidence-truth, pr-review-head-attestation, pr-comment-review-gate, pr-comment-review-gate-check, heartbeat-reviewer-evidence-live-head, github-review-posted-metric

Disposition of #1936 / #1937

Untouched, as PEN-3413 left them. Both have since been re-reviewed at their current heads (#1936 5263242106, #1937 5263421083), each attesting the head it is stamped at — so both now hold a review this predicate accepts on its own merits.

Human-merge gated; not self-merging.

Risks

• Behavioural shift in the review gate: a formal review whose body attests a head other than the one it is stamped on no longer counts as reviewer evidence. A PR relying on such a review must be re-reviewed at its current head (both fix(heartbeat,adapter-k8s): a 429 behind a replacement Job is the throttle, not k8s_pod_schedule_failed (BLO-34577) #1936 and fix(claude-k8s): cap tool-spawned children with RLIMIT_DATA so an orphaned Bash child cannot OOM-kill the run (BLO-34477) #1937 already were).
• Bodyless and off-template reviews are unchanged; only a body naming a different head, or several distinct heads, is refused. This keeps the BLO-28920 retry loop closed.
• No schema or migration changes. The agent-facing gateSignals copy is corrected in step with the implementation and guarded by a test.

Model Used

• The code change was authored by the Ally agent (allyblockcast[bot], Paperclip review and landing agent, Anthropic Claude via Penstock); the exact model id of that authoring run is not recorded on this PR.
• This description was brought up to the PR template by Claude Fable 5.1 (claude-fable-5-1, 1M context, tool use) without changing any code.

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 have searched GitHub for duplicate or related PRs and linked them above (search for PEN-3413 and "re-anchored" in Blockcast/paperclip returns only this PR)
• 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 (no UI change)
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-32844
🔗 Paperclip issue: BLO-28920
🔗 Paperclip issue: PEN-3413
🔗 Paperclip issue: PEN-2865

[allyblockcast]

✅ All checks passing — ready for Greptile review and maintainer approval.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; nested CLI unavailable in this runtime).
Reviewed head: 6d3ad29

CI at this head has produced no test verdict: workflow run 35648952860 ("PR" — General tests, Typecheck, verify) is cancelled and unsuperseded, so the body's "160 passed" is a local result, not a CI one. Per the repo's own rule the remedy is gh api -X POST repos/Blockcast/paperclip/actions/runs/35648952860/rerun, not a push — a push moves the head and voids any at-head attestation. commitperclip PR Review (35648953271) is also red.

Critical Issues (0)

Important Issues (0)

I tried to break this and could not. Verified against the head tree rather than the patch:

• headShaHex (github-app-auth.ts:392) lowercases, and extractAllyReviewedHeadShas lowercases each capture, so the new attested[0] === head comparison cannot fail on casing even though the pattern carries the i flag.
• body: r.body ?? "" at github-app-auth.ts:652 means the string parameter is never null at the call site.
• The early if (attested.length === 0) return false is load-bearing, not redundant — without it a zero-attestation body falls to !(0 === 1 && …) and reads as contradicting.
• matchAll on the shared gi pattern is safe (the spec clones the regex, so lastIndex is not shared), matching what the singular form already does.
• Consecutive attestation lines both match: the trailing (?=\n|$) is a lookahead, so the newline stays available to the next iteration's (?:^|\n). The "several distinct heads" test depends on this and pins it.
• github-app-auth.ts:935 is the only site in the file that credits commit_id === headSha, so the guard is not half-applied. I confirmed the PR's "remaining consumer" claim directly: evidence-truth.ts:213 already keys CLEAN on the attestation and pr-review-head-attestation.ts:39 already refuses the stamp.

Suggestions (3)

• [native-codex] server/src/services/github-app-auth.ts:537 — the documented residual is that an App review carrying no attestation still rides on the stamp alone, so a re-anchored bodyless review still credits the gate. Agreed that requiring presence would reproduce BLO-28920, so the narrowness is right for this PR. Worth noting for follow-up: the measurement in this very header supplies a stamp-independent discriminator that also covers bodyless reviews — a review whose submitted_at precedes the head commit's committer.date provably cannot have been stamped at submit time (+23h11m and +10h16m in the two measured cases). Costs one extra read, which is why it does not belong on this hot path today, but it closes the part this fix cannot.
• [pr-review-toolkit:tests] server/src/__tests__/github-app-auth.test.ts:455 — the guard creates a control-flow path that could not previously exist: a review stamping at head now falls through to listReviewerComments. Before, surface 1 short-circuited and comments were never fetched. The new behaviour is correct (a refused review owes no found:true, and a comment-fetch failure there is a genuine inability to ask), but it interacts with the BLO-28920 note at :744 about {error} vs {found:true}, so it deserves a pin: a re-anchored review plus a valid comment at head should yield {found:true, via:"comment"}.
• [gstack/review] server/src/services/github-app-auth.ts:492 — the plural form de-duplicates and the singular keys on raw count, so the two surfaces now disagree on one input: a body attesting the same head twice is credited via reviewBodyContradictsHead but refused by commentAttestsHead. The doc at ally-review-detection.ts:199 explains why the singular was left alone, which is the right call; it just does not flag that the surfaces diverge here. One clause at :492 would stop a later reader "harmonising" them in the unsafe direction.

Strengths

• The mechanism is measured, not assumed, and the measurement is what selects the fix site. Showing that the stamped commit did not exist at submit time refutes the submit-race hypothesis outright, and that is precisely what rules out every producer-side remedy — so "this must be a consumer-side guard" is derived rather than asserted.
• The narrowness is defended against the specific regression it would otherwise cause, with the BLO-28920 loop named and quantified, and then pinned by a test ("attests no head at all") rather than left to a comment. That test is the one most likely to be deleted by someone tightening the predicate, which is exactly why it earns its place.
• A real positive control sits beside the rejection case, differing only in which head the body names — so the rejection cannot pass by having broken every bodied review. The "does not mask a sibling review" case covers the .some() semantics that a narrower reading would have missed.
• Adding extractAllyReviewedHeadShas alongside the singular form, instead of refactoring it, keeps three fail-closed callers untouched; deriving the singular from the plural would have silently changed a doubly-attested body from null to a credited SHA.
• The gateSignals copy is updated in the same change, and the validator test asserts the invariants rather than the prose — all four of its patterns match the new description, so a size-motivated trim fails loudly instead of quietly reinstating the bypass in the agent-performed half of the check.
• Scope discipline: the dismiss/supersede suggestion from the ticket is explicitly deferred rather than bundled.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.