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/__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..9d0a27bfaebc 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,68 @@ 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 { + 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}: ` + + `${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. *