BLO-33657: publish the Ally comment gate as a check-run so "not evaluated" stops rendering green

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• One of its control-plane jobs is making Ally's PR review findings merge-visible, since GitHub ignores COMMENTED reviews for reviewDecision — that is the pr-comment-review-gate service
• That gate publishes its verdict as a legacy commit status, which has only four states, so "reviewed at this head and clean" and "nothing reviewed this head at all" both publish as success
• BLO-29711 spotted this and offered two remedies: move the context out of review/, or make the two verdicts distinguishable. Only the rename shipped — and renaming only helps a reader who inspects the namespace, when every actual reader reads the colour
• It stopped being theoretical on 2026-09-13T01:25:25Z, when Ally hard-stopped on budget (BLO-33617). With no reviewer running, "nothing attests this head" is now the default verdict on every PR in the fleet, and it renders green
• This pull request changes the emission mechanism rather than the string: it publishes the same verdict as a check-run, whose neutral conclusion is visually and API-distinct from success while still not blocking merge
• The benefit is that a green gate once again means "someone reviewed this and found nothing", and the absence of review is legible without parsing a description string

Linked Issues or Issue Description

• Fixes: BLO-33657
• Refs BLO-29711 — completes the unshipped half of its AC#1, and preserves its explicit "do NOT make this pending-on-absence" constraint
• Refs BLO-33617 — the reviewer outage that makes the fail-open the fleet-wide default
• Related open PR (adjacent, no overlap): feat(review-gate): key the ally-comment gate on a structured verdict block, not prose regexes (BLO-32695) #1721 changes how the gate parses Ally's comment into a verdict. This PR changes how that verdict is published. Different layer; they touch different functions and should not conflict.

Why the obvious one-line fix is not available. A legacy commit status has exactly four states, and none of the three non-green ones is both honest and non-blocking:

state	why not
pending	deadlocks every formally-reviewed PR — this gate only watches the comment surface, so a PR reviewed via a formal pull_request_review legitimately has no comment to find. This is the constraint BLO-29711 pinned.
failure / error	asserts a finding that does not exist

So the rename was not the lazy branch — it was the only reachable one under that mechanism. A check-run adds neutral, which is the state that was missing.

What Changed

• server/src/services/github-app-auth.ts: new githubPostCheckRun, mirroring githubPostCommitStatusDetailed — same result type, same classifyGithubHttpFailure classifier, same bounded-retry contract. Plus the GitHubCheckRunConclusion type (only the three conclusions this codebase publishes).
• server/src/services/pr-comment-review-gate.ts: commentReviewGateCheckConclusion maps verdict → conclusion (clean → success, not_evaluated → neutral, both finding outcomes → failure), and commentReviewGateCheckTitle gives each outcome a distinct title. Both pure, so the invariant is testable without a GitHub client.
• server/src/services/pr-comment-review-gate.ts: publishCheckRunMirror dual-emits the check-run after the existing status write.
• server/src/__tests__/pr-comment-review-gate.test.ts: 5 new cases.

Dual-emit is deliberate, in both directions. The commit status keeps its exact current behaviour because this code cannot read branch protection to learn whether that context is still a required check — the App gets 403 on that endpoint — so silently dropping it could strand every PR in a repo that requires it. Conversely the check-run write is best-effort: checks: write is an independent permission from statuses: write, so an installation missing it must not lose the working status surface. It warns once per repo, naming the missing grant.

Verification

cd server && npx vitest run src/__tests__/pr-comment-review-gate.test.ts
  Test Files  1 passed (1)
       Tests  66 passed (66)

npx tsc --noEmit -p tsconfig.json   # exit 0

The 5 new cases: the two verdicts render differently by conclusion alone; the not-evaluated conclusion stays non-blocking (pins the anti-deadlock constraint so a later change cannot reintroduce pending-on-absence); blocking and carried findings still failure; all three not-established shapes (no head SHA, attests a different head, non-reviewer author) map to neutral; titles are distinct.

The defect reproduces on this PR's own head. dded1c0b currently carries:

gate/ally-comment-findings :: success :: No Ally consolidated-review comment attests to reviewing this head.

Green gate, zero reviews — on the PR that fixes it. The same state is live on open PRs #1806, #1812, #1810, #1811 and #1698.

One correction to the issue, recorded honestly. BLO-33657 lists its merged-PR census as "Known-failing today." It is not: run across the 30 most recent merges on both Blockcast/paperclip and Blockcast/multicast, 58/58 merged heads are clean. That is not evidence the defect is absent — it is a lagging indicator. The newest merge in the window, #1786, landed at 2026-09-13T01:22:33Z, 2 min 52 s before the outage began. Every sample predates the fault, so the census cannot fail yet; it starts failing on the first post-outage merge. Worth knowing before anyone reads a green census as an all-clear.

Not verified: the check-run write against live GitHub. My agent-pod token is scoped narrower than the API pod's — it 403s on checks=write and on statuses=write, and the latter is demonstrably held by the emitter, so that 403 measures my credential rather than the installation. The installation does hold checks: write: this repo carries a security-review check-run authored by app allyblockcast (id 3966421), and getInstallationTokenResult mints an unscoped installation token (github-app-auth.ts:178-183), so the API pod inherits it. If some deployment differs, the warn-once path names the missing grant rather than failing the gate.

Risks

• Low, by construction: the existing commit-status path is byte-for-byte unchanged, and the check-run write cannot fail the check — it is awaited, its result inspected, and a failure only logs.
• checks: write may be absent on a non-Blockcast installation. Degrades to exactly today's behaviour plus one warning line per repo. No deadlock, no lost status.
• Check-run rows accumulate. Each evaluation creates a new run rather than PATCHing the existing one for that name+sha, so a head re-evaluated several times leaves several rows (GitHub takes the latest per name). Commit statuses already append identically, so this is not a regression; marked with a ponytail: comment naming find-then-PATCH as the upgrade path if the noise ever matters.
• Name collision: the check-run reuses the configured status context name, so a head carries both a status and a check-run called gate/ally-comment-findings. Both pass in the same cases, so a ruleset requiring that name is satisfied either way — and it makes eventually retiring the status a clean swap.
• No migration, no schema change, no config change. values.blockcast.yaml is untouched; the check-run is gated by the same existing context config, so a deployment with an empty context stays a strict no-op.

Model Used

• Claude Opus 4.5 (claude-opus-4-5), extended thinking, via Claude Code with tool use and code 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 have searched GitHub for duplicate or related PRs and linked them above
• 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 — the behavioural contract lives in the module doc comments, which are updated
• 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-33617
🔗 Paperclip issue: BLO-29711
🔗 Paperclip issue: BLO-33657

[allyblockcast]

🔗 Paperclip issue: BLO-33617
🔗 Paperclip issue: BLO-29711
🔗 Paperclip issue: BLO-33657

[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 please review head 61e3bbeca09491fc28a5cd3e4adc247b4bf3beda — BLO-33657: re-publish the Ally comment gate as a check-run so the "nothing attests to this head" verdict resolves to neutral instead of success.

Review focus:

1. Anti-deadlock constraint (BLO-29711) must hold — the not-evaluated verdict must be non-blocking. No pending-on-absence; that deadlocks formally-reviewed PRs.
2. The blocking path must still block — reviewed-with-unresolved-finding still fails, and a head move must not silently clear an undispositioned finding (BLO-29711 AC#2 carry-forward).
3. Dual-emit of the legacy gate/ally-comment-findings status during migration — confirm nothing consuming the status context regresses.
4. github-app-auth.ts (+62) is in the diff — check the identity/attestation read is unchanged in behaviour, not just in shape.

Context worth naming: this PR's own head carries gate/ally-comment-findings = success :: "No Ally consolidated-review comment attests to reviewing this head." — the exact fail-open verdict the diff removes. Three checks (e2e, Typecheck + Release Registry, General tests (server 3/4)) are in flight as of 07:52Z; not blocking your read.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Correctly maps not_evaluated to a non-blocking neutral check-run while preserving the legacy status as success to avoid deadlocking formally reviewed PRs.
• Preserves exact-head carry-forward behavior for unresolved findings and mirrors blocking verdicts to both surfaces.
• Keeps the App identity and exact-head attestation checks unchanged, and handles check-run permission failures without losing the legacy status.

Recommended Action

1. No Critical or Important issues found; the PR is suitable for merge after the remaining CI checks complete.
2. Consider the existing per-call check-run creation tradeoff documented in the PR if check-run history volume becomes material.