From 3a29c00c8e97b74eec50150705ea809398c46f3f Mon Sep 17 00:00:00 2001 From: CTO Date: Thu, 20 Aug 2026 12:11:40 +0000 Subject: [PATCH] fix(github): stop the comment-review gate explaining a pass as an absence (BLO-28988) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit evaluateCommentReviewGate published state=success alongside the description "No Ally consolidated-review comment attests to reviewing this head." A green status whose own reason asserts that nothing reviewed the head reads as a broken gate, and it cannot be told apart from "reviewed by comment, clean". The states were right; the reasons were not. This context speaks for the COMMENT surface only. Ally reviews most pull requests through a formal pull_request_review object and posts no comment-shaped review, so on those the surface is simply not engaged and there is nothing here to block. Each success reason now states which case applied, and the never-engaged case is distinguished from a superseded earlier-head review. Also fixes a real fail-open: an unevaluable surface (no head SHA) returned success, asserting a clean surface that was never read. It now returns pending. Deliberately NOT changed: a finding still does not carry across a replacement head. That is the reasoned choice from BLO-21907 / #1262 ("exact full-head attestations prevent stale comments from blocking a replacement head"), and requiring a fresh re-review to go green would couple this gate to reviewer delivery, the least reliable link in the chain (BLO-28920, BLO-28968). The SCOPE docblock records which surface this context answers for, so it is not re-widened into a "was this head reviewed at all" gate — that is a branch-protection policy question owned by the formal surface (BLO-26602). Co-Authored-By: Paperclip --- .../__tests__/pr-comment-review-gate.test.ts | 70 ++++++++++++++- server/src/services/pr-comment-review-gate.ts | 88 +++++++++++++++---- 2 files changed, 142 insertions(+), 16 deletions(-) diff --git a/server/src/__tests__/pr-comment-review-gate.test.ts b/server/src/__tests__/pr-comment-review-gate.test.ts index 9b22038a5dea..908100051dfc 100644 --- a/server/src/__tests__/pr-comment-review-gate.test.ts +++ b/server/src/__tests__/pr-comment-review-gate.test.ts @@ -59,8 +59,10 @@ describe("evaluateCommentReviewGate", () => { comments: [allyComment(blockingReview(OLD_HEAD), "2026-08-04T20:09:19Z")], }); + // Deliberate: only an exact-head attestation gates, so a stale comment cannot + // block a head that may already contain the fix (BLO-21907 / #1262). expect(verdict).toMatchObject({ state: "success" }); - expect(verdict.reason).toMatch(/no Ally consolidated-review comment/i); + expect(verdict.reason).toMatch(/does not carry over/i); }); it("requires the configured GitHub App identity, not a same-shaped contributor comment", () => { @@ -106,3 +108,69 @@ describe("evaluateCommentReviewGate", () => { expect(verdict).toMatchObject({ state: "failure" }); }); }); + +// BLO-28988. The gate published `success` alongside the description "No Ally +// consolidated-review comment attests to reviewing this head." — a pass whose +// own reason reads as an assertion that nothing reviewed the head. These pin the +// state each surface case yields and, crucially, that no `success` reason can be +// read as reporting a deficiency. +describe("evaluateCommentReviewGate state/description consistency", () => { + const CASES = [ + { + name: "(a) comment-shaped review at the exact head, clean", + comments: [allyComment(cleanReview(CURRENT_HEAD), "2026-08-04T20:09:19Z")], + state: "success", + }, + { + name: "(b) comment-shaped review at a stale head only", + comments: [allyComment(cleanReview(OLD_HEAD), "2026-08-04T20:09:19Z")], + state: "success", + }, + { + name: "(c) no comment-shaped review (Ally reviewed formally instead)", + comments: [], + state: "success", + }, + { + name: "(d) comment-shaped review at the exact head carrying a finding", + comments: [allyComment(blockingReview(CURRENT_HEAD), "2026-08-04T20:09:19Z")], + state: "failure", + }, + ] as const; + + for (const testCase of CASES) { + it(`yields ${testCase.state} for ${testCase.name}`, () => { + const verdict = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [...testCase.comments], + }); + + expect(verdict.state).toBe(testCase.state); + // The regression: a success must never explain itself as an absence of + // attestation. Only a non-success state may report that nothing attests. + if (verdict.state === "success") { + expect(verdict.reason).not.toMatch(/no ally consolidated-review comment attests/i); + expect(verdict.reason).not.toMatch(/attests to reviewing this head/i); + } + }); + } + + it("reports pending, never success, when the surface cannot be evaluated", () => { + const verdict = evaluateCommentReviewGate({ headSha: "", comments: [] }); + + expect(verdict.state).toBe("pending"); + expect(verdict.reason).toMatch(/not evaluated/i); + }); + + it("distinguishes a superseded earlier review from a surface never engaged", () => { + const superseded = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(cleanReview(OLD_HEAD), "2026-08-04T20:09:19Z")], + }); + const neverEngaged = evaluateCommentReviewGate({ headSha: CURRENT_HEAD, comments: [] }); + + expect(superseded.state).toBe("success"); + expect(neverEngaged.state).toBe("success"); + expect(superseded.reason).not.toBe(neverEngaged.reason); + }); +}); diff --git a/server/src/services/pr-comment-review-gate.ts b/server/src/services/pr-comment-review-gate.ts index 153ac3ffeddc..fcaee7fc6021 100644 --- a/server/src/services/pr-comment-review-gate.ts +++ b/server/src/services/pr-comment-review-gate.ts @@ -5,6 +5,28 @@ * Ally must emit a plain PR comment when it cannot formally review its own * App-authored pull request, so Critical/Important findings on that surface * otherwise have no effect on merge eligibility. + * + * SCOPE — this context speaks for the COMMENT surface only (BLO-28988). + * + * It answers exactly one question: "does Ally's comment-shaped review carry an + * unresolved finding against this head?" It is NOT a "was this head reviewed at + * all" gate, and must not be widened into one. Ally reviews most pull requests + * through a formal `pull_request_review` object (`COMMENTED`) and posts no + * comment-shaped review at all, so on those the comment surface is simply not + * engaged and there is nothing here to block. Reporting a non-success state for + * that case would red-flag every healthy pull request — the same failure mode + * `values.blockcast.yaml` already documents for the sibling + * `prReviewGateStatusContext` ("would leave every healthy PR at 'Expected — + * waiting for status' forever"). + * + * Whether a head must be reviewed by *someone* is a branch-protection policy + * question owned by the formal surface (`reviewDecision`) — see BLO-26602 and + * BLO-20673. Do not answer it here by overloading this context. + * + * What this file must never do is emit `success` alongside a description that + * asserts a deficiency: that pairing reads as "gate passed" while claiming + * nothing attests to the head, which is what BLO-28988 was filed about. Every + * `success` reason below states what was checked and why nothing blocks. */ import { loadConfig } from "../config.js"; import { @@ -30,6 +52,7 @@ export interface CommentReviewGateComment { export type CommentReviewGateVerdict = | { state: "success"; reason: string } + | { state: "pending"; reason: string } | { state: "failure"; reason: string; commentCreatedAt: string }; function toEpochMs(value: string | Date): number { @@ -48,18 +71,21 @@ function isAllyConsolidatedReviewComment( ); } -function latestAllyCommentForHead( +/** + * Latest comment-shaped Ally review, optionally restricted to one exact head. + * Pass `requiredHead: null` to find the most recent one whatever head it named. + */ +function latestAllyReviewComment( comments: CommentReviewGateComment[], - headSha: string, reviewerBotLogin: string, + requiredHead: string | null, ): CommentReviewGateComment | null { - const normalizedHead = headSha.trim().toLowerCase(); let latest: CommentReviewGateComment | null = null; let latestTime = -Infinity; for (const comment of comments) { if (!isAllyConsolidatedReviewComment(comment, reviewerBotLogin)) continue; - if (extractAllyReviewedHeadSha(comment.body) !== normalizedHead) continue; + if (requiredHead !== null && extractAllyReviewedHeadSha(comment.body) !== requiredHead) continue; const commentTime = toEpochMs(comment.createdAt); if (!Number.isFinite(commentTime)) continue; @@ -75,7 +101,23 @@ function latestAllyCommentForHead( /** * Evaluate only the comment-shaped review surface for one exact PR head. - * Formal reviews remain owned by GitHub's normal reviewDecision path. + * Formal reviews remain owned by GitHub's normal reviewDecision path — see the + * SCOPE note at the top of this file before widening any branch here. + * + * State meanings, all scoped to the comment surface: + * failure — a comment-shaped review of THIS head carries an unresolved finding. + * pending — the surface could not be evaluated at all. Not a pass: reporting + * success would assert a clean surface we never actually read. + * success — nothing on this surface blocks. Each success reason states which + * case applied, so a green status is never paired with a description + * a reader can mistake for a deficiency (BLO-28988). + * + * A finding deliberately does NOT carry across a replacement head: only an + * exact-head attestation gates, so a stale comment cannot block a head that may + * already contain the fix (BLO-21907 / #1262). Requiring a *fresh* re-review to + * go green would additionally couple this gate to reviewer delivery, the least + * reliable link in the chain (BLO-28920, BLO-28968). The superseded case + * therefore gets its own reason string rather than its own state. */ export function evaluateCommentReviewGate(input: { comments: CommentReviewGateComment[]; @@ -84,28 +126,44 @@ export function evaluateCommentReviewGate(input: { }): CommentReviewGateVerdict { const reviewerBotLogin = input.reviewerBotLogin?.trim() || DEFAULT_PR_REVIEWER_BOT_LOGIN; const headSha = input.headSha?.trim(); - if (!headSha) return { state: "success", reason: "No head SHA was supplied to evaluate against." }; + // An inability to evaluate is not a pass. Reporting success here would assert + // a clean surface we never read. + if (!headSha) { + return { state: "pending", reason: "Comment-review surface not evaluated: no head SHA was supplied." }; + } - const latest = latestAllyCommentForHead(input.comments ?? [], headSha, reviewerBotLogin); - if (!latest) { + const comments = input.comments ?? []; + const normalizedHead = headSha.toLowerCase(); + const latestForHead = latestAllyReviewComment(comments, reviewerBotLogin, normalizedHead); + + if (latestForHead) { + if (hasActionablePrReviewFeedback(latestForHead.body)) { + return { + state: "failure", + reason: + "Ally's most recent consolidated-review comment for this head carries an unresolved finding.", + commentCreatedAt: new Date(toEpochMs(latestForHead.createdAt)).toISOString(), + }; + } return { state: "success", - reason: "No Ally consolidated-review comment attests to reviewing this head.", + reason: "Ally's consolidated-review comment for this head reports no unresolved findings.", }; } - if (hasActionablePrReviewFeedback(latest.body)) { + // Nothing attests to this head. Name which case it is, so a reader can tell a + // superseded earlier review apart from a surface that was never engaged. Both + // pass: see the note above on why a finding does not carry across heads. + if (latestAllyReviewComment(comments, reviewerBotLogin, null)) { return { - state: "failure", - reason: - "Ally's most recent consolidated-review comment for this head carries an unresolved finding.", - commentCreatedAt: new Date(toEpochMs(latest.createdAt)).toISOString(), + state: "success", + reason: "No comment-review finding for this head; an earlier head's review does not carry over.", }; } return { state: "success", - reason: "Ally's most recent consolidated-review comment for this head reports no unresolved findings.", + reason: "Comment-review surface clear: no Ally comment-shaped review gates this head.", }; }