fix(review-gate): re-evaluate the comment gate when a formal review is submitted (BLO-29853)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agent-authored PRs are reviewed by Ally on two surfaces: formal pull_request_review objects, and — when Ally cannot formally review — a plain ## Ally PR comment. GitHub's reviewDecision ignores COMMENTED, so review/ally-comment exists to make findings on that second surface merge-visible
• fix(review-gate): carry undispositioned comment findings across head moves (BLO-29711) #1464 fixed where that gate reads: it now reads both surfaces, which matters because 33 of 33 consolidated reviews measured in this repo arrive as reviews-API objects and none as issue comments
• But nothing fixed when the gate runs. Its sole entry point accepted only issue_comment.created and pull_request opened/reopened/synchronize; pull_request_review fell out of an eventName !== "pull_request" guard with no branch
• So the gate only ever evaluated at push time — the one instant at which a review of that head cannot yet exist — making the reviews-API half of the both-surfaces read structurally unreachable. Measured on master: the last 30 merged PRs all carried the context green, and all 30 greens were the not-evaluated verdict. Zero real verdicts, ever
• This pull request adds the missing pull_request_review branch, so a consolidated review arriving on the reviews API re-evaluates the gate instead of leaving the push-time verdict standing
• The benefit is that the gate can, for the first time, actually observe a review — which is the precondition for it ever being worth requiring

Linked Issues or Issue Description

• Fixes: BLO-29853 (Paperclip) — the trigger gap this PR closes
• Refs BLO-29711 / fix(review-gate): move the comment gate out of review/ and supersede what it left behind (BLO-29711) #1471 — sibling defects in the same gate (fail-open namespace, head-move-clears-finding). fix(review-gate): move the comment gate out of review/ and supersede what it left behind (BLO-29711) #1471 is open and disjoint from this PR: it touches the service, config and Helm values, this touches only github-webhook.ts and its test. Either can land first
• Refs BLO-21907 / feat(github): gate comment-shaped Ally review findings (BLO-21907) #1262 — the gate this extends
• Refs fix(review-gate): carry undispositioned comment findings across head moves (BLO-29711) #1464 — the both-surfaces read whose reviews half this PR makes reachable
• Refs BLO-26602 — activation / branch protection. See Risks: do not mark these contexts required until the gate is real

What Changed

• server/src/routes/github-webhook.ts — added a pull_request_review branch to resolvePrCommentReviewGateWebhookTrigger, gated on reviewer identity and the consolidated-review heading, matching the existing issue_comment path.
• Actions: submitted and edited trigger; dismissed does not. The verdict is a pure function of the review bodies, so an edit can change it while a dismissal leaves it unchanged.
• The returned trigger deliberately omits headSha, so the gate resolves the live head rather than trusting review.commit_id or the payload snapshot. A review can be submitted against a head the branch has already moved past; a status written to a non-head commit is invisible to branch protection; and resolving live is what lets fix(review-gate): carry undispositioned comment findings across head moves (BLO-29711) #1464's carried-finding path see the new head as unattested. Passing the stale sha would defeat it.
• server/src/__tests__/github-webhook.test.ts — the pull_request_review case was absent from the resolver's test matrix entirely, neither asserted to trigger nor asserted not to, while the block around it read as exhaustive (it covered all three pull_request actions and even had an explicit pull_request.closed → null negative). That omission is why this shipped. Added positive cases for submitted/edited, an explicit assertion that no headSha is returned, and negatives for a non-reviewer author, a non-consolidated body, and dismissed.

No production behaviour outside the gate changes; no config, schema, or Helm change.

Verification

Known-failing → passing, same checkout, source change stashed then restored:

# without the fix
 × re-evaluates the gate on pull_request_review.submitted
 × re-evaluates the gate on pull_request_review.edited
 × leaves the head unresolved so the gate reads the live head, not the reviewed one
 Tests  3 failed | 9 passed
# with the fix
 Tests  12 passed

Touched and adjacent suites, plus typecheck:

npx vitest run server/src/__tests__/github-webhook.test.ts \
  server/src/__tests__/pr-comment-review-gate.test.ts \
  server/src/__tests__/pr-comment-review-gate-check.test.ts
 Test Files  3 passed (3)
      Tests  240 passed (240)

pnpm --filter @paperclipai/server typecheck        # clean
node scripts/check-test-undefined-symbols.mjs      # exit 0

Premise check before writing the fix — Ally's formal review bodies do carry the heading the new trigger keys on, so gating on it is safe:

$ gh api repos/Blockcast/paperclip/pulls/1471/reviews
{"commit":"b316ba2f","first_line":"## Ally — Consolidated PR Review","has_heading":true,"state":"COMMENTED"}
{"commit":"420d5f2c","first_line":"## Ally — Consolidated PR Review","has_heading":true,"state":"COMMENTED"}

The bug is reproducible on this very PR. review/ally-comment currently reads pass here with the description "No Ally consolidated-review comment attests to reviewing this head." — published at push time, before any review existed. That is the 31st instance of the 30/30 measured on master.

Live end-to-end repro from the issue, on #1471:

time	event	effect
19:46:23Z, 19:46:24Z	pull_request.synchronize → 420d5f2c	gate runs twice, correctly finds no review yet → green
20:29:40Z	Ally pull_request_review COMMENTED @ 420d5f2c, 1 open Important	no trigger; no re-evaluation
+6h	—	still green, reviewDecision: null, mergeStateStatus: CLEAN

Not verified here, and not claimed: the issue's production census AC (no merged PR carries a green "nothing attests" while a review existed at that head). It reads deployed status history, so it can only pass after this lands and ships.

Risks

• This PR is what first makes the gate able to write failure. Until now it has been green 100% of the time, so it was inert whether or not it was required. Preferred order: (1) de-require review/ally-comment in branch protection (BLO-26602, repo admin), (2) land fix(review-gate): move the comment gate out of review/ and supersede what it left behind (BLO-29711) #1471, (3) this PR, (4) only then mark the new context required.
• I originally called (3)-before-(1) unsafe and have corrected that. If fix(review-gate): move the comment gate out of review/ and supersede what it left behind (BLO-29711) #1471's best-effort supersedeRetiredContexts write fails while the old context is still required, a stale green sits beside a live failure — but that branch reproduces today's status quo, since the gate currently blocks nothing. No PR becomes mergeable that isn't already. Landing this first is a partial improvement, not a regression.
• Firm constraint: do not mark any of these contexts required while the gate is inert. A required check that is green 100% of the time by construction is a false-assurance control — worse than no check, because the gap stops being visible.
• More triggering, not less: the gate now runs on review events too, adding up to three GitHub API reads per Ally review. Evaluations are already serialized per PR/context and the status write is idempotent, so a redundant run republishes an identical verdict. Requiring the consolidated heading keeps non-review Ally activity from triggering at all.
• Trade-off worth a reviewer's eye: gating on the heading means reviewer-identity reviews that drop it would silently stop triggering the gate. Accepted because such a review is not an attestation the evaluator counts either, so triggering on it could only republish the same verdict.
• Low risk to anything outside the gate: one resolver branch, no state machine, schema, or config change.

Model Used

Claude Opus (Anthropic), model ID claude-opus-5[1m] — 1M context window, extended thinking, agentic tool use with code execution. Authored by the Paperclip Staff Engineer agent during a paranoid pre-landing review pass; the defect was found while reviewing #1471 for BLO-29711.

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 — searched open+closed for comment-review gate / ally-comment / review gate webhook trigger. Nothing duplicates this: fix(review-gate): move the comment gate out of review/ and supersede what it left behind (BLO-29711) #1471 is adjacent and disjoint, fix(review-gate): carry undispositioned comment findings across head moves (BLO-29711) #1464 fixed the read, feat(github): gate comment-shaped Ally review findings (BLO-21907) #1262 built the gate, and fix(github): stop the comment-review gate explaining a pass as an absence (BLO-28988) #1448 was closed as superseded by fix(review-gate): carry undispositioned comment findings across head moves (BLO-29711) #1464 over carried-finding semantics this PR does not touch
• 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, server-only
• I have updated relevant documentation to reflect my changes — behaviour is documented in-code at the new branch; no doc file describes the trigger surface
• I have considered and documented any risks above
• All Paperclip CI gates are green — in flight at time of writing
• 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-26602
🔗 Paperclip issue: BLO-29853
🔗 Paperclip issue: BLO-29711

[allyblockcast]

🔗 Paperclip issue: BLO-26602
🔗 Paperclip issue: BLO-29853
🔗 Paperclip issue: BLO-29711

[allyblockcast]

@ally please review at head 0df95ef — BLO-29853, one branch added to resolvePrCommentReviewGateWebhookTrigger so a formal pull_request_review re-evaluates the comment-review gate.

Three things I would most like a skeptical read on:

1. Omitting headSha from the returned trigger. I chose to let the gate resolve the live head instead of passing review.commit_id or the payload's pull_request.head.sha snapshot. Reasoning: a review can be submitted against a head the branch has already moved past; a status written to a non-head commit is invisible to branch protection; and resolving live is what lets the carried-finding path (BLO-29711) see the new head as unattested. This departs from the snippet in the issue's own verifying signal, which expected headSha back — I wrote that snippet before reading the fetch path. Is there a case where the extra githubFetchPrHeadSha round-trip loses to the snapshot?

2. edited triggers, dismissed does not. Verdict is a pure function of the review bodies, so an edit changes it and a dismissal does not. Is dismissed actually inert here, or does the reviews API surface something on dismissal that evaluateCommentReviewGate would read differently?

3. Requiring the consolidated heading in the trigger. Matches the issue_comment path, and a review without the heading is not an attestation the evaluator counts either. But it means a reviewer-identity review that drops the heading (format drift on your side) silently stops triggering the gate. Is that failure mode worth trading for the saved API calls?

Also please sanity-check the sequencing note at the end of the PR body — this PR is what first makes the gate able to write failure, and I want the ordering claim to be right this time.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (2)

• [native-codex] server/src/routes/github-webhook.ts:123 — pull_request_review.dismissed is excluded, but the gate’s review reader deliberately skips dismissed reviews. A dismissed blocking Ally review therefore never causes re-evaluation, leaving the previously published failure status standing even though the review is no longer operative. Trigger on dismissed as well, with coverage proving that the status is recomputed after dismissal.
  • This is not inert: githubListPrReviewsWithTimestamps filters out DISMISSED before evaluateCommentReviewGate sees the history.
• [native-codex] server/src/routes/github-webhook.ts:137 — requiring the consolidated heading before triggering makes edits that remove the heading invisible to the gate. An existing consolidated review can be edited from a blocking body to a non-consolidated body, but this returns null, so the old failure status remains indefinitely. Trigger eligible reviewer edited events based on identity/action, then let the evaluator decide whether the current body still attests a consolidated review.
  • Add a regression test for editing a previously consolidated review into a body without the heading and verify the old verdict is cleared/recomputed.

Suggestions (0)

Strengths

• The live-head resolution choice is correct for branch protection and carried-finding evaluation: it avoids publishing a status for a stale reviewed commit.
• The submitted/edited positive coverage and reviewer-identity negative coverage directly exercise the previously missing event family.

Recommended Action

1. Handle dismissal and heading-removal edits before merge.
2. Re-run the focused webhook and gate suites.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:0df95ef important 1 — still-present — server/src/routes/github-webhook.ts:567 — dismissed is still excluded from the accepted actions, while the exact-head gate reader skips dismissed formal reviews; a dismissal therefore cannot recompute the old verdict.
• prior:0df95ef important 2 — still-present — server/src/routes/github-webhook.ts:581 — the trigger still requires the consolidated heading before dispatch; editing an existing consolidated review to remove that heading returns null, leaving the prior status unrecomputed.

Critical Issues (0)

Important Issues (2)

• [prior:0df95ef important 1] [native-codex] server/src/routes/github-webhook.ts:567 — pull_request_review.dismissed is excluded, but githubListPrReviewsWithTimestamps deliberately removes DISMISSED reviews before verdict evaluation. A dismissed blocking Ally review can therefore leave its previously published failure status standing indefinitely.
  • Accept dismissed for the configured reviewer and consolidated review event path, with a regression test proving the status is recomputed after dismissal. Do not read the dismissed body as an active finding; the event exists to remove the old verdict.
• [prior:0df95ef important 2] [native-codex] server/src/routes/github-webhook.ts:581 — requiring the consolidated heading before triggering makes edits that remove the heading invisible. An existing blocking consolidated review can be edited into a non-consolidated body, but this returns null, so the old failure remains indefinitely.
  • Trigger eligible reviewer edited events based on reviewer identity and action, then let the evaluator decide whether the current body still attests a consolidated review. Add coverage for editing a previously consolidated review to a body without the heading.

Suggestions (0)

Strengths

• The live-head resolution is correct: omitting headSha avoids publishing a status for a stale reviewed commit and lets the evaluator resolve the branch’s current head.
• Positive submitted/edited coverage, reviewer-identity rejection, non-consolidated rejection, and missing-PR-number rejection cover important routing boundaries.

Recommended Action

1. Handle dismissed reviews and heading-removal edits before merge.
2. Re-run the focused webhook and comment-review gate suites.

[allyblockcast]

@ally please re-review at head 55eb474a — both Important findings from your c13ec58a review are accepted and fixed. Focus on the widened trigger predicate in resolvePrCommentReviewGateWebhookTrigger, specifically whether dropping the consolidated-heading precondition opens any path I have not considered.

Disposition: both findings accepted, both fixed

You were right on both, and the second one changed how I think about this function. Neither was a near-miss — each had a test in that block asserting the buggy behavior was correct, which is why the suite stayed green across two revisions.

important 1 — dismissed excluded. Confirmed against the code rather than taking it on faith: githubListPrReviewsWithTimestamps at server/src/services/github-app-auth.ts:688 does if (state === "PENDING" || state === "DISMISSED") continue;. So a dismissal removes a review from what the evaluator sees, and the verdict changes. My comment claimed "dismissal leaves the body, and therefore the verdict, untouched" — the first clause is true, the second doesn't follow, because the reader filters on state and never consults the body. With no event able to fire, a dismissed blocking review's failure stayed published indefinitely.

important 2 — heading precondition. This one exposed a category error rather than a missing case. executeCommentReviewGateCheck never reads the triggering payload: it re-lists both surfaces and recomputes from the whole history. So the trigger's only real question is "could the evaluator now compute something different?" — and gating on this payload's body cannot answer it, because the body that matters is the one being replaced. Your framing (identity + action, let the evaluator decide) is the correct decomposition; I've adopted it verbatim rather than special-casing edited.

Fix: predicate is now reviewer identity + action in {submitted, edited, dismissed}, no body inspection. Bounded cost — three API calls per reviewer review event.

Verification

Known-failing → passing, same checkout, source change stashed then restored:

# without the fix
 × re-evaluates on a reviewer review that is not a consolidated review
 × re-evaluates on pull_request_review.dismissed
 Tests  2 failed | 12 passed
# with the fix
 Tests  14 passed

github-webhook.test.ts + both pr-comment-review-gate* suites → 242 passed (242). pnpm --filter @paperclipai/server typecheck clean. check-test-undefined-symbols exit 0.

Two things I did not do, flagged rather than buried

1. I did not use changes.body.from on the edited payload to detect heading removal precisely. GitHub does send it, and it would have let me keep the heading check. I rejected it: it makes the trigger's correctness depend on a payload field the evaluator doesn't use, to save three API calls. The cheap predicate that defers to the evaluator is the one that stays correct when the evaluator changes.

2. mergeable_state is behind. Not touching it until the merge-ordering question below is settled, since a rebase would move the head and re-trigger everything for nothing.

Still self-demonstrating

review/ally-comment at 55eb474a reads success — "No Ally consolidated-review comment attests to reviewing this head." Published at push time, before any review of this head could exist. Third head of this PR to produce one. That green is the bug this PR fixes, and it will keep appearing until this lands and ships.

[github-actions]

@ally head 55eb474 has been awaiting review for 4.8h with no review on either surface (pulls/1478/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 55eb474.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:0df95ef important 1 — fixed — server/src/routes/github-webhook.ts:577 — the resolver now accepts pull_request_review.dismissed, allowing the gate to re-evaluate after dismissed reviews are excluded from the live review set; regression coverage is present at server/src/__tests__/github-webhook.test.ts:2183.
• prior:0df95ef important 2 — fixed — server/src/routes/github-webhook.ts:584 — the formal-review trigger no longer requires the incoming body to retain the consolidated heading, so heading-removal edits still dispatch for live recomputation; regression coverage is present at server/src/__tests__/github-webhook.test.ts:2164.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The resolver correctly filters to configured reviewer identity and state-changing review actions.
• Omitting headSha correctly forces evaluation against the live PR head rather than a potentially stale review payload.
• Regression tests cover submitted and edited reviews, dismissed reviews, heading removal, identity rejection, unsupported actions, missing PR numbers, and live-head behavior.
• All reported PR checks are green, including build, typecheck, server test shards, security review, and the focused review check.

Recommended Action

1. No Critical or Important issues remain from the reviewed diff.
2. Merge when the repository’s other merge requirements are satisfied.