From 0df42aa556868e8ea25f92534c311be56087b258 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Sun, 9 Aug 2026 09:33:37 +0000 Subject: [PATCH 1/3] fix(review): preserve required Ally evidence pairs Treat only the exact App/User pair as one logical verdict and keep retries or unknown credentials fatal. Co-Authored-By: Paperclip --- .github/workflows/ally-review-consistency.yml | 11 +- scripts/check-ally-review-consistency.mjs | 126 ++++++++++-- .../check-ally-review-consistency.test.mjs | 190 +++++++++++++++++- 3 files changed, 309 insertions(+), 18 deletions(-) diff --git a/.github/workflows/ally-review-consistency.yml b/.github/workflows/ally-review-consistency.yml index 999f6d0076e2..a837e613ffa7 100644 --- a/.github/workflows/ally-review-consistency.yml +++ b/.github/workflows/ally-review-consistency.yml @@ -1,10 +1,11 @@ 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 logical Ally verdict at its +# current head (the required App/User exact-head pair is one logical verdict), +# 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: diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index 77b98231334a..249801498637 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -16,15 +16,34 @@ * 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 logical Ally verdict per (PR, head SHA). The exact-head + * gate deliberately requires the App and User-seat review artifacts on + * independently authored PRs; one byte-identical pair from exactly those + * two identities is one logical verdict, not a duplicate verdict. * 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 +75,12 @@ 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 identities required by the exact-head gate. Treating a pair as one +// logical verdict is deliberately fail-closed: any extra, unknown, missing, or +// same-identity submission remains visible to I1. +export const ALLY_APP_REVIEWER_ID = 290875700; +export const ALLY_USER_REVIEWER_ID = 296676656; + export function isAllyLogin(login) { return ALLY_LOGIN_RE.test(String(login ?? "")); } @@ -83,6 +108,47 @@ export function operativeAllyReviews(reviews, headSha) { ); } +/** + * Exact App/User review pairs that attest the same body at one head. The + * protected-review contract requires both artifacts, so this pair is a single + * logical verdict. The exception is deliberately narrow: it accepts only two + * submissions whose user IDs are exactly the known App and User-seat IDs. + * + * A third submission, a retry under one identity, an unknown credential, or a + * missing ID must remain fatal under I1. Broadly grouping by "different IDs" + * would erase exactly the malformed multi-review state this guard exists to + * detect. + */ +export function requiredIdentityAttestationPairs(reviews, headSha) { + const operative = operativeAllyReviews(reviews, headSha); + const byBody = new Map(); + for (const review of operative) { + const key = String(review?.body ?? ""); + byBody.set(key, [...(byBody.get(key) ?? []), review]); + } + return [...byBody.values()].filter( + (group) => + group.length === 2 && + group.some((review) => review?.user?.id === ALLY_APP_REVIEWER_ID) && + group.some((review) => review?.user?.id === ALLY_USER_REVIEWER_ID), + ); +} + +/** + * Operative reviews with one member of each valid required-identity pair + * removed, so I1 counts logical verdicts rather than required evidence copies. + */ +export function distinctVerdicts(reviews, headSha) { + const redundant = new Set( + requiredIdentityAttestationPairs(reviews, headSha) + .flatMap((group) => group.slice(1)) + .map((review) => review?.id), + ); + return operativeAllyReviews(reviews, headSha).filter( + (review) => !redundant.has(review?.id), + ); +} + /** * @param {{number: number, headSha: string, reviews: object[]}} pr * @returns {string[]} human-readable violations; empty when the PR is sound @@ -91,12 +157,13 @@ export function findPrViolations(pr) { const head = pr.headSha; const short = String(head ?? "").slice(0, 8); const operative = operativeAllyReviews(pr.reviews, head); + const verdicts = distinctVerdicts(pr.reviews, head); const violations = []; - if (operative.length > 1) { - const detail = operative.map((r) => `${r.state}/${r.id}`).join(", "); + if (verdicts.length > 1) { + const detail = verdicts.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}: ${verdicts.length} logical Ally verdicts (${detail}) — expected at most 1`, ); } @@ -114,6 +181,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 +214,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 +252,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 +297,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..ea4d729b2df9 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -4,8 +4,10 @@ import { describe, it } from "node:test"; import { pathToFileURL } from "node:url"; import { + assertHeadSha, assertPrListComplete, attestedHead, + distinctVerdicts, findPrViolations, findViolations, hasBlockingFindings, @@ -13,6 +15,7 @@ import { isAllyLogin, isMainModule, operativeAllyReviews, + requiredIdentityAttestationPairs, } from "./check-ally-review-consistency.mjs"; const HEAD = "ff1c72dbfd18014c838cf1373b1640dd17378f3e"; @@ -175,7 +178,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 logical Ally verdicts/); assert.match(violations[1], /^I2b PR #876 @ff1c72db: standing APPROVED \(4829069732\)/); }); @@ -321,3 +324,188 @@ describe("assertPrListComplete", () => { assert.doesNotThrow(() => assertPrListComplete(undefined, "o/r", 10)); }); }); + +const APP_UID = 290875700; +const USER_UID = 296676656; + +/** The required App/User exact-head evidence pair: one body, two identities. */ +function credentialPair(body) { + return [ + { ...review({ id: 11, state: "APPROVED", body }), user: { login: "allyblockcast[bot]", id: APP_UID } }, + { ...review({ id: 12, state: "APPROVED", body }), user: { login: "allyblockcast", id: USER_UID } }, + ]; +} + +describe("requiredIdentityAttestationPairs", () => { + const body = `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n`; + + it("groups an identical body posted under the required App and User identities", () => { + const groups = requiredIdentityAttestationPairs(credentialPair(body), HEAD); + assert.equal(groups.length, 1); + assert.deepEqual(groups[0].map((r) => r.id), [11, 12]); + }); + + it("does not group two identical bodies from the same identity — that is a retry", () => { + const reviews = credentialPair(body).map((r) => ({ ...r, user: { login: "allyblockcast", id: USER_UID } })); + assert.deepEqual(requiredIdentityAttestationPairs(reviews, HEAD), []); + }); + + it("does not group different bodies across the two identities — those are two real verdicts", () => { + const [app, user] = credentialPair(body); + assert.deepEqual( + requiredIdentityAttestationPairs([app, { ...user, body: `${body}\n### Important Issues (1)\n- something` }], HEAD), + [], + ); + }); + + it("ignores a review recorded against a different head", () => { + const [app, user] = credentialPair(body); + assert.deepEqual(requiredIdentityAttestationPairs([app, { ...user, commit_id: OTHER }], HEAD), []); + }); + + it("does not group a triple with a same-identity retry", () => { + const [app, user] = credentialPair(body); + const retry = { ...user, id: 13 }; + assert.deepEqual(requiredIdentityAttestationPairs([app, user, retry], HEAD), []); + }); + + it("does not group an unexpected or missing credential ID", () => { + const [app, user] = credentialPair(body); + assert.deepEqual( + requiredIdentityAttestationPairs( + [app, user, { ...user, id: 13, user: { login: "allyblockcast", id: 42 } }], + HEAD, + ), + [], + ); + assert.deepEqual( + requiredIdentityAttestationPairs( + [app, user, { ...user, id: 13, user: { login: "allyblockcast" } }], + HEAD, + ), + [], + ); + }); +}); + +describe("distinctVerdicts", () => { + const body = `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n`; + + it("collapses the required identity pair to its single logical verdict", () => { + assert.deepEqual(distinctVerdicts(credentialPair(body), HEAD).map((r) => r.id), [11]); + }); + + it("keeps a same-account duplicate, so a retry still trips I1", () => { + const reviews = credentialPair(body).map((r) => ({ ...r, user: { login: "allyblockcast", id: USER_UID } })); + assert.equal(distinctVerdicts(reviews, HEAD).length, 2); + }); +}); + +describe("I1 after required-identity pair collapse", () => { + const body = `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n`; + + it("does not fire on a required App/User pair", () => { + const violations = findPrViolations({ number: 1129, headSha: HEAD, reviews: credentialPair(body) }); + assert.deepEqual(violations.filter((v) => v.startsWith("I1")), []); + }); + + it("still fires when the two accounts submit genuinely different verdicts", () => { + const [app, user] = credentialPair(body); + const blocker = { ...app, body: `${body}\n### Important Issues (1)\n- real finding` }; + const violations = findPrViolations({ number: 876, headSha: HEAD, reviews: [blocker, user] }); + assert.equal(violations.filter((v) => v.startsWith("I1")).length, 1); + }); + + it("keeps an extra same-identity retry visible to I1", () => { + const [app, user] = credentialPair(body); + const violations = findPrViolations({ + number: 1193, + headSha: HEAD, + reviews: [app, user, { ...user, id: 13 }], + }); + assert.equal(violations.filter((v) => v.startsWith("I1")).length, 1); + }); + + it("keeps an unexpected or missing credential ID visible to I1", () => { + const [app, user] = credentialPair(body); + const unexpected = findPrViolations({ + number: 1194, + headSha: HEAD, + reviews: [app, user, { ...user, id: 13, user: { login: "allyblockcast", id: 42 } }], + }); + const missing = findPrViolations({ + number: 1195, + headSha: HEAD, + reviews: [app, user, { ...user, id: 13, user: { login: "allyblockcast" } }], + }); + assert.equal(unexpected.filter((v) => v.startsWith("I1")).length, 1); + assert.equal(missing.filter((v) => v.startsWith("I1")).length, 1); + }); +}); + +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 }), []); + } + }); +}); From 018850f90849a5c09f76cd9109f5850cf46927d0 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Sun, 9 Aug 2026 13:19:17 +0000 Subject: [PATCH 2/3] fix(review): keep duplicate Ally submissions fail-closed Count every live review under I1; credential selection belongs in the review publisher. Co-Authored-By: Paperclip --- .github/workflows/ally-review-consistency.yml | 4 +- scripts/check-ally-review-consistency.mjs | 62 ++-------- .../check-ally-review-consistency.test.mjs | 117 +++--------------- 3 files changed, 29 insertions(+), 154 deletions(-) diff --git a/.github/workflows/ally-review-consistency.yml b/.github/workflows/ally-review-consistency.yml index a837e613ffa7..a09c930d1c4e 100644 --- a/.github/workflows/ally-review-consistency.yml +++ b/.github/workflows/ally-review-consistency.yml @@ -1,7 +1,7 @@ name: Ally Review Consistency Guard -# Asserts that every open PR carries at most one logical Ally verdict at its -# current head (the required App/User exact-head pair is one logical verdict), +# Asserts that every open PR carries at most one operative Ally review 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 diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index 249801498637..46f093a33980 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -16,10 +16,10 @@ * apart both submitted at head ff1c72db, 34 s apart, with opposite verdicts. * * Invariants asserted here: - * I1 At most one logical Ally verdict per (PR, head SHA). The exact-head - * gate deliberately requires the App and User-seat review artifacts on - * independently authored PRs; one byte-identical pair from exactly those - * two identities is one logical verdict, not a duplicate verdict. + * I1 At most one operative Ally review submission per (PR, head SHA). + * Each live GitHub review is an attestation with its own credential; do + * not normalize a duplicate after it has been created. The publisher + * must choose one credential for each verdict instead. * I2 No operative Ally APPROVED whose own body reports a Critical or * Important finding, no operative APPROVED coexisting at a SHA with an * operative Ally review that does, and no operative APPROVED that makes @@ -75,12 +75,6 @@ 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 identities required by the exact-head gate. Treating a pair as one -// logical verdict is deliberately fail-closed: any extra, unknown, missing, or -// same-identity submission remains visible to I1. -export const ALLY_APP_REVIEWER_ID = 290875700; -export const ALLY_USER_REVIEWER_ID = 296676656; - export function isAllyLogin(login) { return ALLY_LOGIN_RE.test(String(login ?? "")); } @@ -108,47 +102,6 @@ export function operativeAllyReviews(reviews, headSha) { ); } -/** - * Exact App/User review pairs that attest the same body at one head. The - * protected-review contract requires both artifacts, so this pair is a single - * logical verdict. The exception is deliberately narrow: it accepts only two - * submissions whose user IDs are exactly the known App and User-seat IDs. - * - * A third submission, a retry under one identity, an unknown credential, or a - * missing ID must remain fatal under I1. Broadly grouping by "different IDs" - * would erase exactly the malformed multi-review state this guard exists to - * detect. - */ -export function requiredIdentityAttestationPairs(reviews, headSha) { - const operative = operativeAllyReviews(reviews, headSha); - const byBody = new Map(); - for (const review of operative) { - const key = String(review?.body ?? ""); - byBody.set(key, [...(byBody.get(key) ?? []), review]); - } - return [...byBody.values()].filter( - (group) => - group.length === 2 && - group.some((review) => review?.user?.id === ALLY_APP_REVIEWER_ID) && - group.some((review) => review?.user?.id === ALLY_USER_REVIEWER_ID), - ); -} - -/** - * Operative reviews with one member of each valid required-identity pair - * removed, so I1 counts logical verdicts rather than required evidence copies. - */ -export function distinctVerdicts(reviews, headSha) { - const redundant = new Set( - requiredIdentityAttestationPairs(reviews, headSha) - .flatMap((group) => group.slice(1)) - .map((review) => review?.id), - ); - return operativeAllyReviews(reviews, headSha).filter( - (review) => !redundant.has(review?.id), - ); -} - /** * @param {{number: number, headSha: string, reviews: object[]}} pr * @returns {string[]} human-readable violations; empty when the PR is sound @@ -157,13 +110,12 @@ export function findPrViolations(pr) { const head = pr.headSha; const short = String(head ?? "").slice(0, 8); const operative = operativeAllyReviews(pr.reviews, head); - const verdicts = distinctVerdicts(pr.reviews, head); const violations = []; - if (verdicts.length > 1) { - const detail = verdicts.map((r) => `${r.state}/${r.id}`).join(", "); + if (operative.length > 1) { + const detail = operative.map((r) => `${r.state}/${r.id}`).join(", "); violations.push( - `I1 PR #${pr.number} @${short}: ${verdicts.length} logical Ally verdicts (${detail}) — expected at most 1`, + `I1 PR #${pr.number} @${short}: ${operative.length} operative Ally reviews (${detail}) — expected at most 1`, ); } diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index ea4d729b2df9..70ba43293d92 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -7,7 +7,6 @@ import { assertHeadSha, assertPrListComplete, attestedHead, - distinctVerdicts, findPrViolations, findViolations, hasBlockingFindings, @@ -15,7 +14,6 @@ import { isAllyLogin, isMainModule, operativeAllyReviews, - requiredIdentityAttestationPairs, } from "./check-ally-review-consistency.mjs"; const HEAD = "ff1c72dbfd18014c838cf1373b1640dd17378f3e"; @@ -178,7 +176,7 @@ describe("findPrViolations", () => { }; const violations = findPrViolations(pr); assert.equal(violations.length, 2); - assert.match(violations[0], /^I1 PR #876 @ff1c72db: 2 logical 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\)/); }); @@ -328,118 +326,43 @@ describe("assertPrListComplete", () => { const APP_UID = 290875700; const USER_UID = 296676656; -/** The required App/User exact-head evidence pair: one body, two identities. */ -function credentialPair(body) { +/** The duplicate-credential shape from BLO-22916: one body, two identities. */ +function duplicateCredentialPair(body) { return [ { ...review({ id: 11, state: "APPROVED", body }), user: { login: "allyblockcast[bot]", id: APP_UID } }, { ...review({ id: 12, state: "APPROVED", body }), user: { login: "allyblockcast", id: USER_UID } }, ]; } -describe("requiredIdentityAttestationPairs", () => { +describe("I1 counts actual operative review submissions", () => { const body = `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n`; - it("groups an identical body posted under the required App and User identities", () => { - const groups = requiredIdentityAttestationPairs(credentialPair(body), HEAD); - assert.equal(groups.length, 1); - assert.deepEqual(groups[0].map((r) => r.id), [11, 12]); - }); - - it("does not group two identical bodies from the same identity — that is a retry", () => { - const reviews = credentialPair(body).map((r) => ({ ...r, user: { login: "allyblockcast", id: USER_UID } })); - assert.deepEqual(requiredIdentityAttestationPairs(reviews, HEAD), []); - }); - - it("does not group different bodies across the two identities — those are two real verdicts", () => { - const [app, user] = credentialPair(body); - assert.deepEqual( - requiredIdentityAttestationPairs([app, { ...user, body: `${body}\n### Important Issues (1)\n- something` }], HEAD), - [], - ); - }); - - it("ignores a review recorded against a different head", () => { - const [app, user] = credentialPair(body); - assert.deepEqual(requiredIdentityAttestationPairs([app, { ...user, commit_id: OTHER }], HEAD), []); - }); - - it("does not group a triple with a same-identity retry", () => { - const [app, user] = credentialPair(body); - const retry = { ...user, id: 13 }; - assert.deepEqual(requiredIdentityAttestationPairs([app, user, retry], HEAD), []); - }); - - it("does not group an unexpected or missing credential ID", () => { - const [app, user] = credentialPair(body); - assert.deepEqual( - requiredIdentityAttestationPairs( - [app, user, { ...user, id: 13, user: { login: "allyblockcast", id: 42 } }], - HEAD, - ), - [], - ); - assert.deepEqual( - requiredIdentityAttestationPairs( - [app, user, { ...user, id: 13, user: { login: "allyblockcast" } }], - HEAD, - ), - [], - ); - }); -}); - -describe("distinctVerdicts", () => { - const body = `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n`; - - it("collapses the required identity pair to its single logical verdict", () => { - assert.deepEqual(distinctVerdicts(credentialPair(body), HEAD).map((r) => r.id), [11]); - }); - - it("keeps a same-account duplicate, so a retry still trips I1", () => { - const reviews = credentialPair(body).map((r) => ({ ...r, user: { login: "allyblockcast", id: USER_UID } })); - assert.equal(distinctVerdicts(reviews, HEAD).length, 2); - }); -}); - -describe("I1 after required-identity pair collapse", () => { - const body = `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n`; - - it("does not fire on a required App/User pair", () => { - const violations = findPrViolations({ number: 1129, headSha: HEAD, reviews: credentialPair(body) }); - assert.deepEqual(violations.filter((v) => v.startsWith("I1")), []); - }); + it("flags a byte-identical App/User pair rather than collapsing it", () => { + const violations = findPrViolations({ + number: 1129, + headSha: HEAD, + reviews: duplicateCredentialPair(body), + }); - it("still fires when the two accounts submit genuinely different verdicts", () => { - const [app, user] = credentialPair(body); - const blocker = { ...app, body: `${body}\n### Important Issues (1)\n- real finding` }; - const violations = findPrViolations({ number: 876, headSha: HEAD, reviews: [blocker, user] }); assert.equal(violations.filter((v) => v.startsWith("I1")).length, 1); + assert.match( + violations.find((v) => v.startsWith("I1")) ?? "", + /^I1 PR #1129 @ff1c72db: 2 operative Ally reviews \(APPROVED\/11, APPROVED\/12\)/, + ); }); - it("keeps an extra same-identity retry visible to I1", () => { - const [app, user] = credentialPair(body); + it("counts every live duplicate, including a third retry", () => { + const [app, user] = duplicateCredentialPair(body); const violations = findPrViolations({ number: 1193, headSha: HEAD, reviews: [app, user, { ...user, id: 13 }], }); - assert.equal(violations.filter((v) => v.startsWith("I1")).length, 1); - }); - it("keeps an unexpected or missing credential ID visible to I1", () => { - const [app, user] = credentialPair(body); - const unexpected = findPrViolations({ - number: 1194, - headSha: HEAD, - reviews: [app, user, { ...user, id: 13, user: { login: "allyblockcast", id: 42 } }], - }); - const missing = findPrViolations({ - number: 1195, - headSha: HEAD, - reviews: [app, user, { ...user, id: 13, user: { login: "allyblockcast" } }], - }); - assert.equal(unexpected.filter((v) => v.startsWith("I1")).length, 1); - assert.equal(missing.filter((v) => v.startsWith("I1")).length, 1); + assert.match( + violations.find((v) => v.startsWith("I1")) ?? "", + /^I1 PR #1193 @ff1c72db: 3 operative Ally reviews/, + ); }); }); From 2501212c12ba6a12719ff7fc665170c9b83040b0 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Sun, 9 Aug 2026 14:36:28 +0000 Subject: [PATCH 3/3] fix(review): allow mandated Ally approval pair Validate the actual protected-merge App/User pair without collapsing reviews, preserving fail-closed retry, identity, and attestation checks. Co-Authored-By: Paperclip --- .github/workflows/ally-review-consistency.yml | 5 +- scripts/check-ally-review-consistency.mjs | 58 ++++++++- .../check-ally-review-consistency.test.mjs | 123 ++++++++++++++---- 3 files changed, 152 insertions(+), 34 deletions(-) diff --git a/.github/workflows/ally-review-consistency.yml b/.github/workflows/ally-review-consistency.yml index a09c930d1c4e..fed984fd7854 100644 --- a/.github/workflows/ally-review-consistency.yml +++ b/.github/workflows/ally-review-consistency.yml @@ -1,7 +1,8 @@ name: Ally Review Consistency Guard # Asserts that every open PR carries at most one operative Ally review at its -# current head, +# 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 @@ -29,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 46f093a33980..db01c24abaef 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -16,10 +16,11 @@ * apart both submitted at head ff1c72db, 34 s apart, with opposite verdicts. * * Invariants asserted here: - * I1 At most one operative Ally review submission per (PR, head SHA). - * Each live GitHub review is an attestation with its own credential; do - * not normalize a duplicate after it has been created. The publisher - * must choose one credential for each verdict instead. + * 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, no operative APPROVED coexisting at a SHA with an * operative Ally review that does, and no operative APPROVED that makes @@ -75,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 ?? "")); } @@ -102,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 @@ -112,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 reviews (${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`, ); } diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index 70ba43293d92..87d309874825 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -4,6 +4,10 @@ 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, @@ -13,6 +17,7 @@ import { hasStillPresentDisposition, isAllyLogin, isMainModule, + isRequiredApprovalPair, operativeAllyReviews, } from "./check-ally-review-consistency.mjs"; @@ -323,47 +328,113 @@ describe("assertPrListComplete", () => { }); }); -const APP_UID = 290875700; -const USER_UID = 296676656; - -/** The duplicate-credential shape from BLO-22916: one body, two identities. */ -function duplicateCredentialPair(body) { +/** The mandated two-principal protected-merge shape. */ +function requiredApprovalPair(app = {}, user = {}) { return [ - { ...review({ id: 11, state: "APPROVED", body }), user: { login: "allyblockcast[bot]", id: APP_UID } }, - { ...review({ id: 12, state: "APPROVED", body }), user: { login: "allyblockcast", id: USER_UID } }, + { + ...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 counts actual operative review submissions", () => { - const body = `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n`; +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 }); - it("flags a byte-identical App/User pair rather than collapsing it", () => { - const violations = findPrViolations({ - number: 1129, - headSha: HEAD, - reviews: duplicateCredentialPair(body), - }); + assert.equal(isRequiredApprovalPair(reviews, HEAD), true); + assert.deepEqual(violations.filter((v) => v.startsWith("I1")), []); + }); - assert.equal(violations.filter((v) => v.startsWith("I1")).length, 1); - assert.match( - violations.find((v) => v.startsWith("I1")) ?? "", - /^I1 PR #1129 @ff1c72db: 2 operative Ally reviews \(APPROVED\/11, APPROVED\/12\)/, + 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("counts every live duplicate, including a third retry", () => { - const [app, user] = duplicateCredentialPair(body); - const violations = findPrViolations({ - number: 1193, - headSha: HEAD, - reviews: [app, user, { ...user, id: 13 }], - }); + 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", () => {