From dded1c0b8693fceb09115b9b87e5ca835cbd8830 Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Sun, 13 Sep 2026 05:28:42 +0000 Subject: [PATCH 1/3] Publish the Ally comment gate as a check-run so "not evaluated" is not green MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BLO-29711's AC#1 was disjunctive: move the comment gate out of the `review/` namespace, or make its not-evaluated verdict distinguishable from reviewed-and-clean without reading the description. Only the rename shipped. Renaming stops the misreading for a reader who inspects the namespace, and nobody does that — they read the colour, and both outcomes are `success`. The rename was not a lazy choice, it was the only reachable one: a legacy commit status has four states and none of the other three is both honest and non-blocking. `pending` deadlocks every formally-reviewed PR (the constraint BLO-29711 pinned), and `failure`/`error` assert a finding that does not exist. So change the mechanism rather than the string. A check-run has `neutral`, which renders distinctly from green and does not block merge. Dual-emit: the commit status keeps its current behaviour because this code cannot read branch protection to learn whether it is still required (the App gets 403 there), so dropping it could strand every PR in a repo that requires it. The check-run write is best-effort for the same reason in reverse — `checks: write` is independent of `statuses: write`, and an installation missing it must not lose the working status surface. It warns once per repo instead. Measured on Blockcast/paperclip while Ally is hard-stopped (BLO-33617): open PRs #1806, #1812, #1810, #1811 and #1698 all carry `gate/ally-comment-findings = success` with the description "No Ally consolidated-review comment attests to reviewing this head." Zero reviews, green gate. Refs BLO-33657. --- .../__tests__/pr-comment-review-gate.test.ts | 74 ++++++++++++++ server/src/services/github-app-auth.ts | 62 ++++++++++++ server/src/services/pr-comment-review-gate.ts | 96 +++++++++++++++++++ 3 files changed, 232 insertions(+) diff --git a/server/src/__tests__/pr-comment-review-gate.test.ts b/server/src/__tests__/pr-comment-review-gate.test.ts index 9dbe86f47f0e..92909cbc445a 100644 --- a/server/src/__tests__/pr-comment-review-gate.test.ts +++ b/server/src/__tests__/pr-comment-review-gate.test.ts @@ -12,6 +12,8 @@ import { hasAllyConsolidatedReviewHeading, } from "../services/ally-review-detection.js"; import { + commentReviewGateCheckConclusion, + commentReviewGateCheckTitle, commentReviewGateRetirementDescription, commentReviewGateRetirementStatus, commentReviewGateVerdictIsMisreadable, @@ -991,3 +993,75 @@ describe("retired context supersede", () => { } }); }); + +describe("commentReviewGateCheckConclusion", () => { + const notEvaluated = evaluateCommentReviewGate({ headSha: CURRENT_HEAD, comments: [] }); + const clean = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(cleanReview(CURRENT_HEAD), "2026-08-04T21:09:19Z")], + }); + const blocking = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(blockingReview(CURRENT_HEAD), "2026-08-04T20:09:19Z")], + }); + const carried = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(blockingReview(OLD_HEAD), "2026-08-04T20:09:19Z")], + }); + + it("renders not-evaluated differently from reviewed-and-clean without reading the description", () => { + // The defect this exists to close: on the commit-status surface both of + // these are `success`, so the only thing separating "reviewed, clean" from + // "nothing reviewed this head" is prose nobody reads (BLO-33657). + expect(notEvaluated.state).toBe(clean.state); + + expect(commentReviewGateCheckConclusion(clean)).toBe("success"); + expect(commentReviewGateCheckConclusion(notEvaluated)).toBe("neutral"); + expect(commentReviewGateCheckConclusion(notEvaluated)).not.toBe( + commentReviewGateCheckConclusion(clean), + ); + }); + + it("keeps the not-evaluated conclusion non-blocking", () => { + // BLO-29711's constraint, pinned so a later change cannot answer the + // distinguishability requirement by reintroducing pending/failure-on-absence + // and deadlocking every formally-reviewed PR. + expect(["success", "neutral"]).toContain(commentReviewGateCheckConclusion(notEvaluated)); + }); + + it("still blocks on a finding, at this head or carried from an earlier one", () => { + expect(commentReviewGateCheckConclusion(blocking)).toBe("failure"); + expect(commentReviewGateCheckConclusion(carried)).toBe("failure"); + }); + + it("covers every not-established shape, not just the empty-comment one", () => { + const cases = [ + // No head supplied to evaluate against. + evaluateCommentReviewGate({ headSha: "", comments: [] }), + // An Ally review that attests some other head. + evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(cleanReview(INTERMEDIATE_HEAD), "2026-08-04T21:09:19Z")], + }), + // A clean review of this head from someone who is not the reviewer. + evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [ + { authorLogin: "someone-else", body: cleanReview(CURRENT_HEAD), createdAt: "2026-08-04T21:09:19Z" }, + ], + }), + ]; + + for (const verdict of cases) { + expect(verdict).toMatchObject({ state: "success", outcome: "not_evaluated" }); + expect(commentReviewGateCheckConclusion(verdict)).toBe("neutral"); + } + }); + + it("gives each outcome its own title so the conclusion is legible unopened", () => { + const titles = [notEvaluated, clean, blocking, carried].map(commentReviewGateCheckTitle); + + expect(new Set(titles).size).toBe(titles.length); + expect(commentReviewGateCheckTitle(notEvaluated)).toMatch(/not evaluated/i); + }); +}); diff --git a/server/src/services/github-app-auth.ts b/server/src/services/github-app-auth.ts index 9cb9173bf418..ecc47ef9454c 100644 --- a/server/src/services/github-app-auth.ts +++ b/server/src/services/github-app-auth.ts @@ -976,6 +976,68 @@ export async function githubPostCommitStatusDetailed(input: { } } +/** + * Conclusions a completed check-run may carry. Only the three this codebase + * publishes are listed; the rest of GitHub's enum is unused here. + * + * `neutral` is the reason check-runs exist in this file at all. A legacy commit + * status has only success/failure/pending/error, so a verdict that is neither + * "reviewed and clean" nor "blocking" has no honest state to occupy: `pending` + * and `failure` block merge, and `success` is indistinguishable from a real + * pass. `neutral` renders distinctly and does not block (BLO-33657). + */ +export type GitHubCheckRunConclusion = "success" | "failure" | "neutral"; + +/** + * Publish a completed check-run as the GitHub App. + * + * Requires the installation's `checks: write` permission — a commit status is + * `statuses: write` and the two are independent, so a deployment that can post + * statuses is not thereby able to post check-runs. + * + * ponytail: creates a new run per call rather than looking up and PATCHing the + * existing one for this name+sha. GitHub takes the latest per name, and commit + * statuses already append the same way, so repeated evaluations of one head + * leave several rows. Switch to find-then-PATCH if that noise ever matters. + */ +export async function githubPostCheckRun(input: { + repoFullName: string; + sha: string; + name: string; + conclusion: GitHubCheckRunConclusion; + title: string; + summary: string; + detailsUrl?: string | null; +}): Promise { + const token = await getInstallationTokenResult(); + if (!token.ok) return asCommitStatusFailure(token); + const headers = { + ...GITHUB_API_HEADERS, + authorization: `Bearer ${token.token}`, + "content-type": "application/json", + }; + const apiBase = gitHubApiBase(GITHUB_HOST); + try { + const res = await ghFetch(`${apiBase}/repos/${input.repoFullName}/check-runs`, { + method: "POST", + headers, + body: JSON.stringify({ + name: input.name, + head_sha: input.sha, + status: "completed", + conclusion: input.conclusion, + output: { title: input.title, summary: input.summary }, + ...(input.detailsUrl ? { details_url: input.detailsUrl } : {}), + }), + }); + if (res.ok) return { ok: true, statusCode: res.status }; + const classified = await classifyGithubHttpFailure("check_run_write", res); + return { ok: false, ...classified, statusCode: res.status }; + } catch { + return { ok: false, retryable: true, reason: "check_run_write_fetch_failed" }; + } +} + /** * Boolean compatibility wrapper for existing call sites. */ diff --git a/server/src/services/pr-comment-review-gate.ts b/server/src/services/pr-comment-review-gate.ts index b323d9e6fd7f..cb7020c6e373 100644 --- a/server/src/services/pr-comment-review-gate.ts +++ b/server/src/services/pr-comment-review-gate.ts @@ -27,8 +27,10 @@ import { githubFetchPrHeadSha, githubListIssueCommentsWithTimestamps, githubListPrReviewsWithTimestamps, + githubPostCheckRun, githubPostCommitStatusDetailed, githubReviewerIdentityMatches, + type GitHubCheckRunConclusion, type GitHubCommitStatusPostResult, } from "./github-app-auth.js"; @@ -370,6 +372,44 @@ export function evaluateCommentReviewGate(input: { }; } +/** + * Check-run conclusion for a verdict. + * + * This is the whole point of publishing a check-run alongside the commit + * status (BLO-33657). The status surface collapses `clean` and `not_evaluated` + * into one green `success`, because a legacy status has only four states and + * none of the other three is both honest and non-blocking: `pending` deadlocks + * every formally-reviewed PR (the constraint BLO-29711 pinned), and + * `failure`/`error` assert a finding that does not exist. + * + * `neutral` is the state that was missing. It renders distinctly from green in + * the UI and in `check-runs` API reads, and it does not block merge — so a + * reader can tell "reviewed and clean" from "nothing reviewed this head" by the + * conclusion alone, without parsing the human-readable description. + */ +export function commentReviewGateCheckConclusion( + verdict: Pick, +): GitHubCheckRunConclusion { + if (verdict.state === "failure") return "failure"; + return verdict.outcome === "clean" ? "success" : "neutral"; +} + +/** Short check-run title, so the conclusion is legible without opening it. */ +export function commentReviewGateCheckTitle( + verdict: Pick, +): string { + switch (verdict.outcome) { + case "clean": + return "Reviewed at this head — no unresolved findings"; + case "blocking_finding": + return "Unresolved finding at this head"; + case "carried_finding": + return "Unresolved finding carried from an earlier head"; + case "not_evaluated": + return "Not evaluated — no comment-shaped review attests this head"; + } +} + /** * A green status published under a `review/`-prefixed context reads as "this * head was reviewed and was clean". For the `not_evaluated` outcome that @@ -379,6 +419,11 @@ export function evaluateCommentReviewGate(input: { * context is now `gate/ally-comment-findings`. This predicate stays as the * assertion point so a future config change cannot silently move the gate back * under `review/` (BLO-29711). + * + * Note this only ever described the *status* surface. Renaming stopped the + * misreading for a reader who inspects the namespace, not for one who reads the + * colour; the check-run's `neutral` conclusion is what addresses the colour + * (BLO-33657). */ export function commentReviewGateVerdictIsMisreadable( verdict: CommentReviewGateVerdict, @@ -642,6 +687,8 @@ async function executeCommentReviewGateCheck( ); if (!posted.ok) return { posted: false, reason: "post_failed", postFailure: posted.reason }; + await publishCheckRunMirror(input, headSha, context, verdict); + const retirementFailures = await supersedeRetiredContexts(input, headSha, context, config, verdict); if (retirementFailures.length > 0) { // NOT "post_failed": the live status published successfully at line 643 @@ -674,6 +721,55 @@ async function executeCommentReviewGateCheck( return withGithubStatusDeliveryLock(input.db, `${input.repoFullName}#${headSha}`, publish); } +/** + * Publish the same verdict as a check-run, alongside the commit status. + * + * Dual-emit, not a replacement. The status context may still be a required + * check somewhere, and this code cannot read branch protection to find out — + * the App gets 403 on that endpoint — so dropping it could strand every PR in a + * repo that requires it. The status keeps its existing states; the check-run + * adds the `neutral` conclusion that the status surface cannot express. + * + * Best-effort, and deliberately so: the verdict is already published on the + * status surface by the time this runs, and the added value here is legibility, + * not enforcement. Failing the whole check because the check-run write was + * refused would turn a *better* signal into an outage of the working one — most + * likely on exactly the deployments whose installation lacks `checks: write`, + * since that permission is independent of `statuses: write`. Logged once per + * repo so a missing grant is diagnosable without a log flood. + */ +const checkRunWriteWarnings = new Set(); + +async function publishCheckRunMirror( + input: PrCommentReviewGateCheckInput, + headSha: string, + context: string, + verdict: CommentReviewGateVerdict, +): Promise { + const result = await withBoundedRetry( + () => + githubPostCheckRun({ + repoFullName: input.repoFullName, + sha: headSha, + name: context, + conclusion: commentReviewGateCheckConclusion(verdict), + title: commentReviewGateCheckTitle(verdict), + summary: verdict.reason, + detailsUrl: input.prUrl ?? null, + }), + (attempt) => !attempt.ok && attempt.retryable, + ); + if (result.ok) return; + if (checkRunWriteWarnings.has(input.repoFullName)) return; + checkRunWriteWarnings.add(input.repoFullName); + console.warn( + `[pr-comment-review-gate] Could not publish the "${context}" check-run on ${input.repoFullName}: ` + + `${result.reason}. The commit status is still authoritative, but "not evaluated" and ` + + `"reviewed clean" both render green there — the check-run is what separates them. ` + + "A 403 here means the installation is missing `checks: write` (BLO-33657).", + ); +} + /** * Overwrite each retired context with a pointer to the live one. * From 4029cdac2a7587720609af032e8d8a25b89de212 Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Sun, 13 Sep 2026 05:35:50 +0000 Subject: [PATCH 2/3] chore: re-run the commitperclip template gate The PR description now follows .github/PULL_REQUEST_TEMPLATE.md. That gate runs on pull_request_target [opened, synchronize, reopened] only, so editing the body does not re-trigger it; this empty commit supplies the synchronize. Empty rather than an amend so that dded1c0b, cited from BLO-33657 and from the PR body, stays resolvable. Refs BLO-33657. From 61e3bbeca09491fc28a5cd3e4adc247b4bf3beda Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Sun, 13 Sep 2026 05:58:03 +0000 Subject: [PATCH 3/3] Make the check-run mirror best-effort against a thrown error, not just a failed result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught by the existing `pr-comment-review-gate-check` suite, which mocks `github-app-auth.js` with an explicit factory listing only the exports it uses. `githubPostCheckRun` was not among them, so the call threw `githubPostCheckRun is not a function` — and because the await was unguarded, the rejection escaped `publish()` and failed the whole gate. That is the exact outcome the mirror is structured to avoid: the commit status had already been written successfully, and letting the nicer surface take out the working one inverts the point of making it best-effort. The mock was the messenger, but the defect was real — a module that fails to load in production would do the same thing. So catch it, and pin it. The new "does not fail the check when the check-run write throws" case fails with exactly this error when the try/catch is removed (verified by reverting the guard: 1 failed, 17 passed), so it tests the behaviour rather than restating it. Also adds the missing mock export, defaults it to ok in beforeEach, and covers the two verdicts that matter on this surface: a clean head mirrors as `success` while an unattested head mirrors as `neutral` with the commit status still `success` — the anti-deadlock constraint and the distinguishability requirement asserted together, since they are only in tension on that one branch. Refs BLO-33657. --- .../pr-comment-review-gate-check.test.ts | 60 +++++++++++++++++++ server/src/services/pr-comment-review-gate.ts | 43 ++++++++----- 2 files changed, 88 insertions(+), 15 deletions(-) diff --git a/server/src/__tests__/pr-comment-review-gate-check.test.ts b/server/src/__tests__/pr-comment-review-gate-check.test.ts index fa3a494709a9..c8ae00203917 100644 --- a/server/src/__tests__/pr-comment-review-gate-check.test.ts +++ b/server/src/__tests__/pr-comment-review-gate-check.test.ts @@ -18,6 +18,7 @@ const mockListComments = vi.hoisted(() => vi.fn()); const mockListReviews = vi.hoisted(() => vi.fn()); const mockFetchHeadSha = vi.hoisted(() => vi.fn()); const mockPostStatus = vi.hoisted(() => vi.fn()); +const mockPostCheckRun = vi.hoisted(() => vi.fn()); const mockStatusDeliveryLock = vi.hoisted(() => vi.fn()); vi.mock("../services/github-app-auth.js", () => ({ @@ -25,6 +26,7 @@ vi.mock("../services/github-app-auth.js", () => ({ githubListIssueCommentsWithTimestamps: mockListComments, githubListPrReviewsWithTimestamps: mockListReviews, githubPostCommitStatusDetailed: mockPostStatus, + githubPostCheckRun: mockPostCheckRun, githubReviewerIdentityMatches: (login: string, configuredLogin: string) => { const candidate = login.trim().toLowerCase().replace(/^@/, ""); const configured = configuredLogin.trim().toLowerCase().replace(/^@/, ""); @@ -70,11 +72,13 @@ beforeEach(() => { mockListReviews.mockReset(); mockFetchHeadSha.mockReset(); mockPostStatus.mockReset(); + mockPostCheckRun.mockReset(); mockStatusDeliveryLock.mockReset(); mockStatusDeliveryLock.mockImplementation(async (_db, _key, operation) => operation()); // Default both surfaces to empty; each test overrides the one it exercises. mockListComments.mockResolvedValue([]); mockListReviews.mockResolvedValue([]); + mockPostCheckRun.mockResolvedValue({ ok: true, statusCode: 201 }); }); afterEach(() => { @@ -341,3 +345,59 @@ describe("retired status contexts", () => { expect(mockPostStatus).toHaveBeenCalledTimes(1); }); }); + +describe("check-run mirror (BLO-33657)", () => { + const clean = { + login: "allyblockcast[bot]", + body: + `## Ally — Consolidated PR Review\nReviewed head: ${TARGET.headSha}\n` + + "### Critical Issues (0)\n### Important Issues (0)", + createdAt: "2026-09-13T05:00:00Z", + }; + + it("mirrors the verdict as a check-run alongside the commit status", async () => { + mockPostStatus.mockResolvedValue({ ok: true, statusCode: 201 }); + mockListComments.mockResolvedValue([clean]); + + await expect(runPrCommentReviewGateCheck(TARGET)).resolves.toMatchObject({ posted: true }); + + expect(mockPostCheckRun).toHaveBeenCalledTimes(1); + expect(mockPostCheckRun.mock.calls[0][0]).toMatchObject({ + sha: TARGET.headSha, + name: "review/ally-comment-gate", + conclusion: "success", + }); + }); + + it("publishes neutral, not success, when nothing attests the head", async () => { + mockPostStatus.mockResolvedValue({ ok: true, statusCode: 201 }); + + await expect(runPrCommentReviewGateCheck(TARGET)).resolves.toMatchObject({ posted: true }); + + // The commit status is green here and has to stay green — going non-green + // on absence deadlocks formally-reviewed PRs (BLO-29711). The check-run is + // what carries the distinction. + expect(mockPostStatus.mock.calls[0][0]).toMatchObject({ state: "success" }); + expect(mockPostCheckRun.mock.calls[0][0]).toMatchObject({ conclusion: "neutral" }); + }); + + it("does not fail the check when the check-run write is refused", async () => { + mockPostStatus.mockResolvedValue({ ok: true, statusCode: 201 }); + mockPostCheckRun.mockResolvedValue({ ok: false, retryable: false, reason: "check_run_write_http_403" }); + + // An installation without `checks: write` must keep the working status + // surface rather than losing it to the surface that is only nicer. + await expect(runPrCommentReviewGateCheck(TARGET)).resolves.toMatchObject({ posted: true }); + }); + + it("does not fail the check when the check-run write throws", async () => { + mockPostStatus.mockResolvedValue({ ok: true, statusCode: 201 }); + mockPostCheckRun.mockRejectedValue(new Error("githubPostCheckRun is not a function")); + + // "Best-effort" has to survive a thrown error too, not just a classified + // failure result — otherwise the rejection escapes and takes down the + // commit status that was already published. + await expect(runPrCommentReviewGateCheck(TARGET)).resolves.toMatchObject({ posted: true }); + expect(mockPostStatus).toHaveBeenCalled(); + }); +}); diff --git a/server/src/services/pr-comment-review-gate.ts b/server/src/services/pr-comment-review-gate.ts index cb7020c6e373..9d0a27bfaebc 100644 --- a/server/src/services/pr-comment-review-gate.ts +++ b/server/src/services/pr-comment-review-gate.ts @@ -746,25 +746,38 @@ async function publishCheckRunMirror( context: string, verdict: CommentReviewGateVerdict, ): Promise { - const result = await withBoundedRetry( - () => - githubPostCheckRun({ - repoFullName: input.repoFullName, - sha: headSha, - name: context, - conclusion: commentReviewGateCheckConclusion(verdict), - title: commentReviewGateCheckTitle(verdict), - summary: verdict.reason, - detailsUrl: input.prUrl ?? null, - }), - (attempt) => !attempt.ok && attempt.retryable, - ); - if (result.ok) return; + let reason: string; + try { + const result = await withBoundedRetry( + () => + githubPostCheckRun({ + repoFullName: input.repoFullName, + sha: headSha, + name: context, + conclusion: commentReviewGateCheckConclusion(verdict), + title: commentReviewGateCheckTitle(verdict), + summary: verdict.reason, + detailsUrl: input.prUrl ?? null, + }), + (attempt) => !attempt.ok && attempt.retryable, + ); + if (result.ok) return; + reason = result.reason; + } catch (error) { + // "Best-effort" has to mean it too. `githubPostCheckRun` returns a + // classified result rather than throwing, but it can still throw for + // reasons outside its own error handling — an unmocked export under test, + // a module that failed to load. Letting that escape would reject the whole + // publish and lose the commit status that was already written, which is the + // exact "a better signal takes out the working one" outcome this function + // is structured to avoid. + reason = error instanceof Error ? error.message : String(error); + } if (checkRunWriteWarnings.has(input.repoFullName)) return; checkRunWriteWarnings.add(input.repoFullName); console.warn( `[pr-comment-review-gate] Could not publish the "${context}" check-run on ${input.repoFullName}: ` + - `${result.reason}. The commit status is still authoritative, but "not evaluated" and ` + + `${reason}. The commit status is still authoritative, but "not evaluated" and ` + `"reviewed clean" both render green there — the check-run is what separates them. ` + "A 403 here means the installation is missing `checks: write` (BLO-33657).", );