feat(github): gate comment-shaped Ally review findings (BLO-21907)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Ally reviews PRs on two surfaces: a formal pull_request_review, and (always on self-authored PRs, since the Ally App can't review its own PR) a plain issue comment headed ## Ally — Consolidated PR Review
• GitHub's reviewDecision/mergeStateStatus only read the formal-review surface, so a comment-shaped "before merge" finding has zero effect on merge eligibility — #1022 queued to merge ~65 minutes after an unaddressed Important finding landed as a bare comment, because the standing approval's own criterion ("no unresolved review threads") is vacuously true for every PR Ally reviews
• This is structural, not a one-off: any PR whose review arrives comment-shaped can merge with unaddressed findings, and nothing in the gate notices
• This pull request adds a gate-side check that scans the comment-shaped review surface directly and posts a real GitHub required-check status, keyed on "newest Ally consolidated-review comment since the PR's last push" so an addressed finding doesn't block forever
• The benefit is a comment-shaped blocking finding gets the same merge-time enforcement a formal CHANGES_REQUESTED review already has, without touching that existing path

Linked Issues or Issue Description

Fixes: BLO-21907
Refs: BLO-22029 (near-duplicate filed independently before BLO-21907 was found in search; being closed as duplicate)

What Changed

• server/src/services/ally-review-detection.ts (new): extracted hasAllyConsolidatedReviewHeading / hasActionablePrReviewFeedback out of github-webhook.ts into a standalone module with no dependents of its own, so the new gate service and the webhook route both reuse the same hardened detection regexes (each carries a one-off bug history — BLO-19067, BLO-15942, BLO-18865, feat(chart): stamp the approval plan marker on the API pod template (BLO-20733) #973, [codex] honor configured Ally concurrency 15 #937, fix(heartbeat): crash-time run marking + convergent recovery (BLO-20822) #952 — that a second copy would risk silently diverging from) without creating a route↔service import cycle.
• server/src/services/pr-comment-review-gate.ts (new): evaluateCommentReviewGate — pure predicate over a PR's comments + last-push timestamp. Fails only when the most recent Ally-consolidated-review comment since the last push carries an unresolved Critical/Important finding; a finding that predates the push, or is superseded by a later clean pass, does not block. runPrCommentReviewGateCheck — live wiring that fetches comments + head-commit timestamp from GitHub and posts the verdict as a commit status.
• server/src/services/github-app-auth.ts: three new fetch primitives (githubListIssueCommentsWithTimestamps, githubGetCommitCommittedAt, githubFetchPrHeadSha) alongside the existing githubPostCommitStatusDetailed.
• server/src/routes/github-webhook.ts: two best-effort, fire-and-forget trigger points — issue_comment.created (any Ally-consolidated-review-shaped comment, blocking or clean) and pull_request opened/synchronize/reopened (so a fresh head gets an eager status instead of reading "Expected — waiting for status" forever). Both read the raw payload directly, independent of the Paperclip-identifier-matching/actionable-feedback gating the existing wake logic applies for its own, unrelated purpose. Never affects the wake/response path — errors are caught and logged, not propagated.
• server/src/config.ts + Helm chart (values.yaml, deployment-api.yaml, statefulset.yaml): new prCommentReviewGateStatusContext / PAPERCLIP_PR_COMMENT_REVIEW_GATE_STATUS_CONTEXT, empty by default — same opt-in shape as the existing prReviewGateStatusContext (BLO-17456). Fully inert until an operator sets it.

Verification

• pnpm exec vitest run server/src/__tests__/pr-comment-review-gate.test.ts — 9 tests, replaying the exact fix(dispatch): bound worst-case queue wait for every priority tier (BLO-21792) #1022 sequence (rejects it), an addressed-then-re-reviewed sequence (accepts it), a findings-predate-push case (does not block), and a formal-review-only PR (unaffected — the predicate never reads pull_request_review objects at all).
• pnpm exec vitest run server/src/__tests__/pr-comment-review-gate-check.test.ts — 7 tests covering the live wiring: inert with no context configured, correct status/description posted for blocking vs. clean, head-SHA resolution for the issue_comment trigger (no head SHA in that webhook payload), and fetch/post failure reporting.
• pnpm exec vitest run server/src/__tests__/github-webhook.test.ts — full existing suite (109 tests) still green; the new trigger points don't change any existing wake behavior.
• pnpm exec tsc --noEmit in server/ — clean.
• node --test deploy/helm/paperclip/tests/*.test.mjs — 33/36 pass; the 3 failures are pre-existing (approval-plan-marker.test.mjs, requires a local ruby binary this sandbox doesn't have) and reproduce identically on master before this change.

Manual checklist (cannot be completed by an agent — see below).

Risks

This touches the webhook route that drives every GitHub-originated Paperclip wake, so I scoped the new code to be provably inert by default:

• Both new trigger blocks are void (async () => { try { ... } catch (err) { logger.warn(...) } })() — fire-and-forget, never awaited by the handler, so a failure or slowness in the new code cannot delay or fail the webhook response.
• runPrCommentReviewGateCheck returns { posted: false, reason: "not_configured" } and makes zero network calls when PAPERCLIP_PR_COMMENT_REVIEW_GATE_STATUS_CONTEXT is unset — the default in every environment until an operator opts in.
• The extraction into ally-review-detection.ts is a pure move (same regex source, same function bodies) — the full existing github-webhook.test.ts suite is the regression guard for that.

Known scope boundary: the new triggers only fire when the PR carries a Paperclip identifier (branch/title/body), because they sit downstream of the same context.identifiers.length === 0 early-return the rest of the webhook route already uses. A PR with no Paperclip identifier reference won't get gated. In practice every agent-authored PR here carries one; a human/dependency PR without one would not. Flagging this rather than silently limiting coverage — happy to file a fast-follow if broader coverage is wanted.

Not yet enforced: setting the config context alone does not block anything. Branch protection has to mark the new context required on master, which is an org-admin action — agent tokens get 403 Resource not accessible by integration reading protection, so I cannot do this part myself.

Model Used

Claude Sonnet 5 (claude-sonnet-5[1m], 1M context), agentic coding session with repository inspection, live code tracing of the existing PR-review-gate-status infrastructure (BLO-17456) to reuse rather than reinvent, and test-driven verification.

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 (none found)
• 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 change
• I have updated relevant documentation to reflect my changes (Helm values comments)
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending this PR's own CI run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
• I will address all Greptile and reviewer comments before requesting merge

---

Manual checklist for a human (per BLO-21907's acceptance criteria — agent tokens cannot do this)

1. Set githubApp.prCommentReviewGateStatusContext (e.g. "review/ally-comment-gate") in the Blockcast Helm values for the environment(s) this should apply to, and roll the deployment.
2. On GitHub, mark that status context required in master's branch protection rules.
3. Seed a scratch PR with a comment-shaped Ally finding (or replay fix(dispatch): bound worst-case queue wait for every priority tier (BLO-21792) #1022's sequence) and confirm the PR cannot be added to the merge queue while the finding stands, then confirm it clears after a push + clean re-review.

Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-21907

[allyblockcast]

🔗 Paperclip issue: BLO-21907
🔗 Paperclip issue: BLO-17456
🔗 Paperclip issue: BLO-19067
🔗 Paperclip issue: BLO-18865
🔗 Paperclip issue: BLO-22029
🔗 Paperclip issue: BLO-15942

[allyblockcast]

🔗 Paperclip issue: BLO-21907
🔗 Paperclip issue: BLO-17456
🔗 Paperclip issue: BLO-19067
🔗 Paperclip issue: BLO-18865
🔗 Paperclip issue: BLO-22029
🔗 Paperclip issue: BLO-15942

[kkroo]

Reviewed #1049 after adding the missing author trust boundary: comment-shaped review gate now trusts only the configured Ally GitHub App identity, with spoof regression coverage. Verified focused gate/webhook/github-app-auth tests, server typecheck, Helm smoke tests, and diff check locally.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (1)

• [gstack/review] server/src/services/github-app-auth.ts:522 — The gate treats the head commit's user-controlled committer date as the PR's last-push time. A contributor can push a commit with a future GIT_COMMITTER_DATE; a later blocking Ally comment then compares as predating the push, so evaluateCommentReviewGate posts success. Backdating a replacement head has the inverse problem: an old clean review can be inherited by an unreviewed tree.
  • Bind the verdict to the review body's exact Reviewed head: <sha> attestation, or persist a server-observed synchronize time per head. Commit metadata cannot establish when GitHub received a push.

Important Issues (2)

• [pr-review-toolkit] server/src/services/github-app-auth.ts:496 — Comment retrieval stops after ten oldest-first pages and returns the truncated result as complete. Once a PR has 1,000 comments, a blocking Ally review at position 1,001 is omitted and the evaluator can post success; a later clean review can likewise be missed, leaving an obsolete failure.
  • Follow GitHub pagination to exhaustion. If a safety cap is required, return an incomplete/fetch-failed result instead of evaluating partial history.
• [native-codex] server/src/routes/github-webhook.ts:1870 — Gate evaluations are unsynchronized fire-and-forget jobs that unconditionally append a status. A clean-comment run can snapshot before a later blocking comment, stall, then post success after the blocking-comment run has posted failure; GitHub treats the last status for the context as authoritative.
  • Serialize or version evaluations per (repo, PR, head), and revalidate that the evaluated comment is still latest immediately before writing success. A stale evaluation must not overwrite a newer verdict.

Strengths

• The shared detection module avoids divergence between webhook wake logic and gate logic.
• Both trigger-time and evaluation-time author checks use the configured GitHub App identity, with spoof regression coverage.
• The feature is inert until explicitly configured, and fetch failures do not manufacture a fresh verdict.

Recommended Action

1. Fix the Critical timestamp/head-binding issue before merge.
2. Make pagination complete and status writes monotonic for the current head.
3. Re-run the focused predicate, webhook, auth, and concurrency tests.

This PR is authored by app/allyblockcast, so the App cannot approve its own PR. The exact reviewed head must be reopened under an independent author before an App approval can be issued; the existing User/team approval is not App gate evidence.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:8f8dcda critical 1 — still-present — server/src/services/github-app-auth.ts:541 — The exact current head still derives the push boundary from contributor-controlled commit.committer.date; no immutable review-body head attestation or server-observed synchronize time is used.
• prior:8f8dcda important 1 — still-present — server/src/services/github-app-auth.ts:496 — The exact current head still stops oldest-first comment pagination after ten pages and treats the partial 1,000-comment result as complete.
• prior:8f8dcda important 2 — still-present — server/src/services/pr-comment-review-gate.ts:200 — The exact current head still posts every asynchronously computed verdict without serialization or a latest-comment freshness check, so a delayed success can overwrite a newer failure.

Critical Issues (1)

• [prior:8f8dcda critical 1] server/src/services/github-app-auth.ts:541 — The gate still trusts the head commit's user-controlled committer date as the last-push boundary. A future date hides later blocking reviews; a backdated replacement head can inherit review state from an older tree. Bind evaluation to the consolidated review body's exact Reviewed head: <sha> attestation or persist a server-observed synchronize time keyed by head.

Important Issues (3)

• [prior:8f8dcda important 1] server/src/services/github-app-auth.ts:496 — Comment retrieval still truncates after ten oldest-first pages. A blocking review beyond comment 1,000 is omitted and the partial history can produce success. Paginate to exhaustion, or return an incomplete/fetch-failed result when a safety cap is reached.
• [prior:8f8dcda important 2] server/src/services/pr-comment-review-gate.ts:200 — Concurrent fire-and-forget checks still write statuses unconditionally. A clean run can snapshot before a later blocking comment, stall, and then overwrite the newer failure. Serialize/version evaluations per (repo, PR, head) and revalidate the latest review immediately before writing success.
• [pr-review-toolkit] server/src/services/pr-comment-review-gate.ts:208 — Fetch and status-write failures are terminal for this webhook run and only reach logs. Because the webhook is already acknowledged, a transient failure after a blocking comment can leave the head's previous success standing indefinitely. Preserve retryability, enqueue bounded durable retries, and reconcile unresolved/missing contexts.

Strengths

• Shared Ally review detection avoids regex drift between wake routing and gate evaluation.
• Author identity checks prevent contributors from spoofing gate-authoritative review comments.
• The feature remains inert until an operator explicitly configures its status context.

Recommended Action

1. Fix the immutable head-attribution issue before merge.
2. Make pagination complete and status writes monotonic for each head.
3. Add durable retry/reconciliation coverage for transient GitHub failures.

This PR is authored by app/allyblockcast, so the App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared User/team identity is not App gate evidence.