diff --git a/.github/workflows/ally-review-consistency.yml b/.github/workflows/ally-review-consistency.yml index 999f6d0076e2..fed984fd7854 100644 --- a/.github/workflows/ally-review-consistency.yml +++ b/.github/workflows/ally-review-consistency.yml @@ -1,10 +1,12 @@ name: Ally Review Consistency Guard -# Asserts that every open PR carries at most one operative Ally review verdict -# at its current head, that no standing APPROVED masks a Critical/Important -# finding, and that a review's body-attested head matches the commit GitHub -# recorded it against. See scripts/check-ally-review-consistency.mjs and -# BLO-19778 for the incident this guards. +# Asserts that every open PR carries at most one operative Ally review at its +# current head, except for the exact current-head App/User approval pair +# required by protected merge, +# that no standing APPROVED masks a Critical/Important finding, and that a +# review's body-attested head matches the commit GitHub recorded it against. +# See scripts/check-ally-review-consistency.mjs and BLO-19778 for the incident +# this guards. on: schedule: @@ -28,7 +30,7 @@ jobs: - name: Checkout repository uses: actions/checkout@v5 - - name: Assert one operative Ally verdict per PR head + - name: Assert the allowed operative Ally review shape per PR head env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} ALLY_REVIEW_REPO: ${{ inputs.repo || 'Blockcast/paperclip' }} diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index 77b98231334a..db01c24abaef 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -16,15 +16,35 @@ * apart both submitted at head ff1c72db, 34 s apart, with opposite verdicts. * * Invariants asserted here: - * I1 At most one operative Ally verdict per (PR, head SHA). + * I1 At most one operative Ally review submission per (PR, head SHA), or + * exactly the current-head APPROVED App/User pair mandated by protected + * merge. Every other multi-review shape is fatal. The pair is checked as + * two independent, exact identities—not by removing or normalizing a + * review—so retries and lookalike credentials remain visible. * I2 No operative Ally APPROVED whose own body reports a Critical or - * Important finding, and no operative APPROVED coexisting at a SHA with - * an operative Ally review that does. + * Important finding, no operative APPROVED coexisting at a SHA with an + * operative Ally review that does, and no operative APPROVED that makes + * no `Reviewed head:` attestation at all. * I3 An operative Ally review's body-attested `Reviewed head:` matches the - * commit GitHub recorded it against. `gh pr review` attaches a review to - * whatever the head is at submit time, so a mid-review push silently - * certifies a tree that was never read (seen on #870: body attested - * b3a240ec, commit_id was 67965f1e). + * commit GitHub recorded it against, so a standing approval is not + * presented against a tree its author never read. + * + * On I3's mechanism. An earlier revision of this file said `gh pr review` + * binds a review to the head at submit time, so a mid-review push "certifies a + * tree that was never read". Submit-time binding is real but it is not what + * produces most I3 hits, and the difference matters because the old wording + * blamed the reviewer for a value the reviewer never set. Measured on #1104 + * (2026-08-07): review 4878131987 was submitted at 20:52:00Z attesting + * 2533dc6f, the timeline records `head_ref_force_pushed cee75d97` at 21:07:07Z + * creating a commit dated 21:06:55Z, and the review's `commit_id` now reads + * cee75d97 — a commit that did not exist when it was submitted. A review + * cannot be bound at submit time to a commit created 15 minutes later, so a + * force-push re-anchors existing reviews forward onto the new head. Same shape + * on #1098 (+28m), #1111 (+5m), #1067 (+10h23m). `commit_id` is therefore not + * a record of which tree a review examined; the body's attestation line is. + * I3 stays fatal because the hazard is real either way — a green is being + * presented at a head nobody read — but the remedy is to dismiss or re-review + * the stale approval, not to correct the reviewer. * * "Operative" excludes DISMISSED and PENDING: a dismissed review is disposed, * not a standing attestation. @@ -56,6 +76,14 @@ const STILL_PRESENT_DISPOSITION_RE = /** The single standalone attestation line Ally is required to emit. */ const ATTESTED_HEAD_RE = /^[ \t]*(?:[_*]+)?[ \t]*reviewed head:[ \t]*`?([0-9a-f]{40})`?[ \t]*(?:[_*]+)?[ \t]*$/im; +// The two distinct GitHub principals required by the protected-merge policy. +// Pin both the immutable REST ID and the canonical login: either mismatch is +// not an eligible substitute for the required artifact. +export const ALLY_APP_REVIEWER_ID = 290875700; +export const ALLY_APP_REVIEWER_LOGIN = "allyblockcast[bot]"; +export const ALLY_USER_REVIEWER_ID = 296676656; +export const ALLY_USER_REVIEWER_LOGIN = "allyblockcast"; + export function isAllyLogin(login) { return ALLY_LOGIN_RE.test(String(login ?? "")); } @@ -83,6 +111,43 @@ export function operativeAllyReviews(reviews, headSha) { ); } +function isExpectedApproval(review, { id, login }, headSha) { + return ( + review?.state === "APPROVED" && + review?.user?.id === id && + review?.user?.login === login && + attestedHead(review?.body) === String(headSha ?? "").toLowerCase() + ); +} + +/** + * The only permitted two-review shape: one current-head approval from the + * required App identity and one from the required User seat. This deliberately + * inspects the full operative set instead of deduplicating it; a retry, an + * unexpected identity, or a missing/stale attestation makes the shape fail. + */ +export function isRequiredApprovalPair(reviews, headSha) { + const operative = operativeAllyReviews(reviews, headSha); + if (operative.length !== 2) return false; + + return ( + operative.some((review) => + isExpectedApproval( + review, + { id: ALLY_APP_REVIEWER_ID, login: ALLY_APP_REVIEWER_LOGIN }, + headSha, + ), + ) && + operative.some((review) => + isExpectedApproval( + review, + { id: ALLY_USER_REVIEWER_ID, login: ALLY_USER_REVIEWER_LOGIN }, + headSha, + ), + ) + ); +} + /** * @param {{number: number, headSha: string, reviews: object[]}} pr * @returns {string[]} human-readable violations; empty when the PR is sound @@ -93,10 +158,10 @@ export function findPrViolations(pr) { const operative = operativeAllyReviews(pr.reviews, head); const violations = []; - if (operative.length > 1) { + if (operative.length > 1 && !isRequiredApprovalPair(pr.reviews, head)) { const detail = operative.map((r) => `${r.state}/${r.id}`).join(", "); violations.push( - `I1 PR #${pr.number} @${short}: ${operative.length} operative Ally verdicts (${detail}) — expected at most 1`, + `I1 PR #${pr.number} @${short}: ${operative.length} operative Ally reviews (${detail}) — expected at most 1 or the exact App/User APPROVED pair`, ); } @@ -114,6 +179,18 @@ export function findPrViolations(pr) { `I2c PR #${pr.number} @${short}: review ${review.id} is APPROVED but its body marks a prior finding still-present`, ); } + // An approval that makes no attestation at all is the strictly worse case: + // it counts toward `reviewDecision` while claiming nothing about any tree. + // Seen live on #1114 — a 129-byte APPROVED reading "Approved the current CI + // head … this head only retriggers checks", satisfying required-review on a + // PR whose same-head Ally review carried 3 still-present Important + // findings, with auto-merge armed. Acknowledging a CI retrigger is a + // comment, never an approval. + if (attestedHead(review.body) === null) { + violations.push( + `I2d PR #${pr.number} @${short}: review ${review.id} is APPROVED but its body makes no "Reviewed head:" attestation — an approval with no review behind it`, + ); + } } if (approvals.length > 0 && blocking.length > 0) { @@ -135,7 +212,7 @@ export function findPrViolations(pr) { const attested = attestedHead(review.body); if (attested && attested !== String(head ?? "").toLowerCase()) { violations.push( - `I3 PR #${pr.number} @${short}: review ${review.id} attests head ${attested.slice(0, 8)} but GitHub recorded it against ${short} — it certifies a tree it never reviewed`, + `I3 PR #${pr.number} @${short}: review ${review.id} attests head ${attested.slice(0, 8)} but is now recorded against ${short} — a force-push re-anchored it, so it stands as an attestation of a tree its author never read`, ); } } @@ -173,6 +250,29 @@ export function assertPrListComplete(rows, repo, limit = PR_LIST_LIMIT) { return rows; } +/** + * Every invariant here pivots on `headSha`: `operativeAllyReviews` filters + * `commit_id === headSha`, so a falsy or malformed head matches no review, the + * operative set is empty, and I1/I2/I3 all iterate nothing. The run then prints + * a pass having asserted nothing across every PR at once — the same fail-open + * shape as an unreachable `main()`, one layer up. Verified: with `headSha` set + * to `undefined`, `null` or `""`, a deliberately maximal violation (an APPROVED + * reporting `### Critical Issues (3)`, attesting a different SHA, coexisting + * with a blocking COMMENTED) yields zero violations. Assert it for the same + * reason `assertPrListComplete` throws rather than warns. + */ +export function assertHeadSha(row, repo) { + if (!/^[0-9a-f]{40}$/.test(String(row?.headRefOid ?? ""))) { + throw new Error( + `gh pr list returned no usable headRefOid for ${repo}#${row?.number} ` + + `(got ${JSON.stringify(row?.headRefOid)}). Every invariant in this guard ` + + `filters reviews on commit_id === head, so continuing would assert ` + + `nothing while reporting a pass.`, + ); + } + return row; +} + function fetchOpenPrs(repo) { // number + headRefOid both come back from this one call; fetching the head // via `gh api repos/{repo}/pulls/{number}` instead would pull a ~22 KB @@ -195,7 +295,7 @@ function fetchOpenPrs(repo) { assertPrListComplete(rows, repo); return rows.map((row) => ({ - number: row.number, + number: assertHeadSha(row, repo).number, headSha: row.headRefOid, reviews: JSON.parse( gh(["api", `repos/${repo}/pulls/${row.number}/reviews`, "--paginate"]), diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index c8148682523f..87d309874825 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -4,6 +4,11 @@ import { describe, it } from "node:test"; import { pathToFileURL } from "node:url"; import { + ALLY_APP_REVIEWER_ID, + ALLY_APP_REVIEWER_LOGIN, + ALLY_USER_REVIEWER_ID, + ALLY_USER_REVIEWER_LOGIN, + assertHeadSha, assertPrListComplete, attestedHead, findPrViolations, @@ -12,6 +17,7 @@ import { hasStillPresentDisposition, isAllyLogin, isMainModule, + isRequiredApprovalPair, operativeAllyReviews, } from "./check-ally-review-consistency.mjs"; @@ -175,7 +181,7 @@ describe("findPrViolations", () => { }; const violations = findPrViolations(pr); assert.equal(violations.length, 2); - assert.match(violations[0], /^I1 PR #876 @ff1c72db: 2 operative Ally verdicts/); + assert.match(violations[0], /^I1 PR #876 @ff1c72db: 2 operative Ally reviews/); assert.match(violations[1], /^I2b PR #876 @ff1c72db: standing APPROVED \(4829069732\)/); }); @@ -321,3 +327,179 @@ describe("assertPrListComplete", () => { assert.doesNotThrow(() => assertPrListComplete(undefined, "o/r", 10)); }); }); + +/** The mandated two-principal protected-merge shape. */ +function requiredApprovalPair(app = {}, user = {}) { + return [ + { + ...review({ + id: 11, + state: "APPROVED", + body: `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n\nApp artifact.`, + }), + user: { login: ALLY_APP_REVIEWER_LOGIN, id: ALLY_APP_REVIEWER_ID }, + ...app, + }, + { + ...review({ + id: 12, + state: "APPROVED", + body: `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n\nUser-seat approval.`, + }), + user: { login: ALLY_USER_REVIEWER_LOGIN, id: ALLY_USER_REVIEWER_ID }, + ...user, + }, + ]; +} + +describe("I1 accepts only the protected-merge approval pair", () => { + it("accepts exactly one independently attested App/User approval pair", () => { + const reviews = requiredApprovalPair(); + const violations = findPrViolations({ number: 1129, headSha: HEAD, reviews }); + + assert.equal(isRequiredApprovalPair(reviews, HEAD), true); + assert.deepEqual(violations.filter((v) => v.startsWith("I1")), []); + }); + + it("does not require the two independent reviews to have byte-identical prose", () => { + const reviews = requiredApprovalPair( + { body: `Reviewed head: ${HEAD}\n\nApp reviewed the implementation.` }, + { body: `Reviewed head: ${HEAD}\n\nUser seat independently approved the change.` }, + ); + + assert.equal(isRequiredApprovalPair(reviews, HEAD), true); + assert.deepEqual(findPrViolations({ number: 1130, headSha: HEAD, reviews }), []); + }); + + it("rejects an extra operative retry instead of collapsing it", () => { + const [app, user] = requiredApprovalPair(); + const reviews = [app, user, { ...user, id: 13 }]; + const violations = findPrViolations({ number: 1193, headSha: HEAD, reviews }); + + assert.equal(isRequiredApprovalPair(reviews, HEAD), false); + assert.match( + violations.find((v) => v.startsWith("I1")) ?? "", + /^I1 PR #1193 @ff1c72db: 3 operative Ally reviews/, + ); + }); + + it("rejects a lookalike identity even when it carries the User-seat ID", () => { + const [app, user] = requiredApprovalPair(); + const reviews = [app, { ...user, user: { login: "blockcast-ally", id: ALLY_USER_REVIEWER_ID } }]; + const violations = findPrViolations({ number: 1194, headSha: HEAD, reviews }); + + assert.equal(isRequiredApprovalPair(reviews, HEAD), false); + assert.equal(violations.filter((v) => v.startsWith("I1")).length, 1); + }); + + it("rejects a canonical login with an unexpected immutable ID", () => { + const [app, user] = requiredApprovalPair(); + const reviews = [app, { ...user, user: { login: ALLY_USER_REVIEWER_LOGIN, id: 42 } }]; + const violations = findPrViolations({ number: 1195, headSha: HEAD, reviews }); + + assert.equal(isRequiredApprovalPair(reviews, HEAD), false); + assert.equal(violations.filter((v) => v.startsWith("I1")).length, 1); + }); + + it("requires both required identities to submit APPROVED reviews", () => { + const [app, user] = requiredApprovalPair({}, { state: "COMMENTED" }); + const reviews = [app, user]; + const violations = findPrViolations({ number: 1196, headSha: HEAD, reviews }); + + assert.equal(isRequiredApprovalPair(reviews, HEAD), false); + assert.equal(violations.filter((v) => v.startsWith("I1")).length, 1); + }); + + it("fails closed when either required approval omits its exact-head attestation", () => { + const [app, user] = requiredApprovalPair({ body: "Approved without an attestation." }); + const reviews = [app, user]; + const violations = findPrViolations({ number: 1197, headSha: HEAD, reviews }); + + assert.equal(isRequiredApprovalPair(reviews, HEAD), false); + assert.equal(violations.filter((v) => v.startsWith("I1")).length, 1); + assert.equal(violations.filter((v) => v.startsWith("I2d")).length, 1); + }); + + it("fails closed when either required approval attests a stale head", () => { + const [app, user] = requiredApprovalPair({ body: `Reviewed head: ${OTHER}` }); + const reviews = [app, user]; + const violations = findPrViolations({ number: 1198, headSha: HEAD, reviews }); + + assert.equal(isRequiredApprovalPair(reviews, HEAD), false); + assert.equal(violations.filter((v) => v.startsWith("I1")).length, 1); + assert.equal(violations.filter((v) => v.startsWith("I3")).length, 1); + }); + + it("does not accept a review recorded against an old commit as current-head evidence", () => { + const [app, user] = requiredApprovalPair({}, { commit_id: OTHER }); + + assert.equal(isRequiredApprovalPair([app, user], HEAD), false); + }); +}); + +describe("I2d — APPROVED with no attestation line", () => { + it("fires on the #1114 shape: a short APPROVED that attests nothing", () => { + const reviews = [ + review({ + id: 4879433972, + state: "APPROVED", + body: "Approved the current CI head. The implementation is unchanged; this head only retriggers checks.", + }), + ]; + const violations = findPrViolations({ number: 1114, headSha: HEAD, reviews }); + assert.equal(violations.filter((v) => v.startsWith("I2d")).length, 1); + }); + + it("does not fire on an APPROVED that does attest the head", () => { + const reviews = [review({ id: 1, state: "APPROVED" })]; + assert.deepEqual( + findPrViolations({ number: 1, headSha: HEAD, reviews }).filter((v) => v.startsWith("I2d")), + [], + ); + }); + + it("does not fire on a COMMENTED review with no attestation — only an approval claims soundness", () => { + const reviews = [review({ id: 1, state: "COMMENTED", body: "no attestation here" })]; + assert.deepEqual( + findPrViolations({ number: 1, headSha: HEAD, reviews }).filter((v) => v.startsWith("I2d")), + [], + ); + }); +}); + +describe("assertHeadSha", () => { + it("passes a well-formed 40-hex head", () => { + const row = { number: 1, headRefOid: HEAD }; + assert.equal(assertHeadSha(row, "o/r"), row); + }); + + for (const bad of [undefined, null, "", "not-a-sha", HEAD.slice(0, 39), HEAD.toUpperCase()]) { + it(`throws on ${JSON.stringify(bad)} rather than asserting nothing`, () => { + assert.throws(() => assertHeadSha({ number: 7, headRefOid: bad }, "o/r"), /no usable headRefOid/); + }); + } + + it("names the PR so the failure is actionable", () => { + assert.throws(() => assertHeadSha({ number: 42, headRefOid: null }, "o/r"), /o\/r#42/); + }); +}); + +describe("a falsy head would otherwise silently pass a maximal violation", () => { + it("finds every invariant broken at the real head", () => { + const reviews = [ + review({ id: 1, state: "APPROVED", body: `Reviewed head: ${OTHER}\n### Critical Issues (3)\n- boom` }), + review({ id: 2, state: "COMMENTED", body: `Reviewed head: ${HEAD}\n### Important Issues (1)\n- boom` }), + ]; + assert.ok(findPrViolations({ number: 9, headSha: HEAD, reviews }).length >= 4); + }); + + it("finds nothing at all when the head is falsy — which is why assertHeadSha exists", () => { + const reviews = [ + review({ id: 1, state: "APPROVED", body: `Reviewed head: ${OTHER}\n### Critical Issues (3)\n- boom` }), + review({ id: 2, state: "COMMENTED", body: `Reviewed head: ${HEAD}\n### Important Issues (1)\n- boom` }), + ]; + for (const head of [undefined, null, ""]) { + assert.deepEqual(findPrViolations({ number: 9, headSha: head, reviews }), []); + } + }); +});