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

server/src/__tests__/pr-comment-review-gate-check.test.ts

@@ -18,13 +18,15 @@ 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", () => ({
   githubFetchPrHeadSha: mockFetchHeadSha,
   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();
+  });
+});

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);
+  });
+});

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<GitHubCommitStatusPostResult> {
+  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.
  */

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<CommentReviewGateVerdict, "state" | "outcome">,
+): 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<CommentReviewGateVerdict, "state" | "outcome">,
+): 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<string>();
+
+async function publishCheckRunMirror(
+  input: PrCommentReviewGateCheckInput,
+  headSha: string,
+  context: string,
+  verdict: CommentReviewGateVerdict,
+): Promise<void> {
+  let reason: string;
+  try {
+    const result = await withBoundedRetry<GitHubCommitStatusPostResult>(
+      () =>
+        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.
  *