diff --git a/.github/workflows/ally-review-consistency.yml b/.github/workflows/ally-review-consistency.yml index fed984fd7854..f1dc8027aa3a 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 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. +# Asserts three things about every open PR: that it 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, which must carry two +# *distinct* bodies, since one body under both credentials is a single verdict +# posted twice (BLO-22916); 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. diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index db01c24abaef..120bb47692e0 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -120,15 +120,60 @@ function isExpectedApproval(review, { id, login }, headSha) { ); } +/** + * A review body reduced to the form the equality rules below compare. + * + * Trimming is deliberately the only normalization. The named defect mechanism — + * passing one `--body-file` to both `gh pr review` calls — produces + * byte-identical bodies, but a stray trailing newline is still one verdict + * posted twice, and an exact-equality test would audit that pair as sound. Two + * bodies that differ in substance are two write-ups, and deciding when + * overlapping prose counts as one verdict is a larger question than these + * predicates should answer. + * + * This exists as one helper because the rule is applied at two sites that must + * agree: {@link isRequiredApprovalPair}, which decides whether a pair is the + * legitimate two-seat exemption, and {@link duplicateBodyAcrossIdentities}, + * which decides how the resulting violation is worded. Normalizing in only one + * of them is not a partial fix but a silent no-op — the deciding branch runs + * first, so a laxer comparison there exempts the pair before the stricter one + * is ever consulted. That is precisely how the whitespace case survived its + * first fix, with a green unit test asserting on the downstream predicate. + * + * @param {object} review a review object, possibly bodiless + * @returns {string} the body with surrounding whitespace removed; `""` when the + * body is absent, empty, or whitespace-only + */ +export function normalizedBody(review) { + return String(review?.body ?? "").trim(); +} + /** * 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. + * + * The two bodies must also differ, compared via {@link normalizedBody}. This + * exemption exists for the case where a gate genuinely needs both seats — an + * App-authored PR, or a CODEOWNERS/team approver a GitHub App cannot be — and + * there the User seat contributes a short, distinct approval linking to the + * App's review. Two bodies identical up to surrounding whitespace are not that + * case: they are one verdict submitted twice under two credentials, which is + * BLO-22916's defect and the mechanism that held this guard red from + * 2026-08-02 to 2026-08-16. Ally's instructions now forbid passing the same + * `--body-file` to both calls; this is where that is enforced, so a future run + * cannot re-derive "submit under both to be safe" and have the audit call it + * sound. + * + * This is the branch that decides the outcome: `findPrViolations` reports I1 + * only when this returns false, so a pair exempted here is never examined + * further. */ export function isRequiredApprovalPair(reviews, headSha) { const operative = operativeAllyReviews(reviews, headSha); if (operative.length !== 2) return false; + if (normalizedBody(operative[0]) === normalizedBody(operative[1])) return false; return ( operative.some((review) => @@ -148,6 +193,39 @@ export function isRequiredApprovalPair(reviews, headSha) { ); } +/** + * True when two operative reviews carry the same body under different user IDs + * — AC1's literal wording in BLO-22916, and the fingerprint of one run + * submitting its verdict twice rather than two independent passes (two passes + * produce two different write-ups). + * + * Bodies are compared via {@link normalizedBody}, the same helper the deciding + * branch in {@link isRequiredApprovalPair} uses, so the two cannot disagree + * about what counts as the same body. + * + * A body that is empty or whitespace-only is excluded. Two bodiless approvals + * under two seats compare equal, but they are not one verdict posted twice — + * there is no verdict at all. That is Defect 2 (a counting APPROVED with no + * review behind it), I2d already reports it, and its remedy is to post a + * comment rather than to drop one of the two submissions. + * + * @param {object[]} operative reviews ALREADY filtered to the operative set for + * one head, as returned by {@link operativeAllyReviews}. Passing a raw + * `pr.reviews` list would compare dismissed and stale-head reviews and so + * answer a different question than the caller intends. + * @returns {boolean} true when the duplicate-submission shape is present + */ +export function duplicateBodyAcrossIdentities(operative) { + const reviews = operative ?? []; + const bodies = reviews.map(normalizedBody); + return reviews.some((a, i) => + reviews.some( + (b, j) => + j > i && bodies[i] !== "" && bodies[i] === bodies[j] && a?.user?.id !== b?.user?.id, + ), + ); +} + /** * @param {{number: number, headSha: string, reviews: object[]}} pr * @returns {string[]} human-readable violations; empty when the PR is sound @@ -160,8 +238,14 @@ export function findPrViolations(pr) { if (operative.length > 1 && !isRequiredApprovalPair(pr.reviews, head)) { const detail = operative.map((r) => `${r.state}/${r.id}`).join(", "); + // Name the duplicate-submission shape explicitly. Left as a bare count, an + // operator reading the hourly audit cannot tell "one verdict posted twice" + // from "two genuinely different reviews", and those have opposite remedies. + const reason = duplicateBodyAcrossIdentities(operative) + ? "the same body submitted under two credentials — one verdict, posted twice (BLO-22916)" + : "expected at most 1 or the exact App/User APPROVED pair"; violations.push( - `I1 PR #${pr.number} @${short}: ${operative.length} operative Ally reviews (${detail}) — expected at most 1 or the exact App/User APPROVED pair`, + `I1 PR #${pr.number} @${short}: ${operative.length} operative Ally reviews (${detail}) — ${reason}`, ); } diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index 87d309874825..cecc846381e2 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -11,6 +11,7 @@ import { assertHeadSha, assertPrListComplete, attestedHead, + duplicateBodyAcrossIdentities, findPrViolations, findViolations, hasBlockingFindings, @@ -352,6 +353,70 @@ function requiredApprovalPair(app = {}, user = {}) { ]; } +describe("duplicateBodyAcrossIdentities", () => { + const at = (id, uid, body) => ({ id, user: { id: uid }, body }); + + it("fires on one body under two user IDs", () => { + assert.equal( + duplicateBodyAcrossIdentities([at(1, 290875700, "same"), at(2, 296676656, "same")]), + true, + ); + }); + + // One `--body-file` passed to both calls produces byte-identical bodies, but + // a stray trailing newline is still one verdict posted twice. Exact equality + // would audit these pairs as sound. + it("fires when the two bodies differ only in surrounding whitespace", () => { + const body = `## Ally — Consolidated PR Review\nReviewed head: ${"a".repeat(40)}`; + for (const variant of [`${body}\n`, `${body} `, `\n${body}`, `\n ${body}\n\n`]) { + assert.equal( + duplicateBodyAcrossIdentities([at(1, 290875700, body), at(2, 296676656, variant)]), + true, + `expected a duplicate-submission finding for variant ${JSON.stringify(variant)}`, + ); + } + }); + + it("does NOT fire when the bodies differ", () => { + assert.equal( + duplicateBodyAcrossIdentities([at(1, 290875700, "app"), at(2, 296676656, "user")]), + false, + ); + }); + + // A repeat under ONE identity is a retry, not a dual-credential submission; + // the operative-count check already reports it and the remedy differs. + it("does NOT fire when the same identity repeats a body", () => { + assert.equal( + duplicateBodyAcrossIdentities([at(1, 290875700, "same"), at(2, 290875700, "same")]), + false, + ); + }); + + it("tolerates empty and single-element sets", () => { + assert.equal(duplicateBodyAcrossIdentities([]), false); + assert.equal(duplicateBodyAcrossIdentities(undefined), false); + assert.equal(duplicateBodyAcrossIdentities([at(1, 290875700, "solo")]), false); + }); + + // Two bodiless approvals compare equal, but "one verdict, posted twice" is + // the wrong diagnosis: there is no verdict. I2d reports the missing + // attestation, and its remedy (post a comment) differs from this one's. + // A whitespace-only body is bodiless in substance and must land here too. + it("does NOT fire on bodiless reviews under two identities", () => { + for (const empty of [null, "", undefined, " ", "\n\n", "\t "]) { + assert.equal( + duplicateBodyAcrossIdentities([ + at(1, 290875700, empty), + at(2, 296676656, empty), + ]), + false, + `expected no duplicate-submission finding for body ${JSON.stringify(empty)}`, + ); + } + }); +}); + describe("I1 accepts only the protected-merge approval pair", () => { it("accepts exactly one independently attested App/User approval pair", () => { const reviews = requiredApprovalPair(); @@ -371,6 +436,86 @@ describe("I1 accepts only the protected-merge approval pair", () => { assert.deepEqual(findPrViolations({ number: 1130, headSha: HEAD, reviews }), []); }); + // BLO-22916 AC1. The exemption above exists for a gate that genuinely needs + // both seats; it must not launder one verdict submitted twice. Before this + // case the fleet's 17 byte-identical App+User pairs all read as SOUND, so the + // guard certified as clean the exact defect it was pointed at. + it("rejects a byte-identical body submitted under both credentials", () => { + const body = `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n\n### Critical Issues (0)\n### Important Issues (0)\n`; + const reviews = requiredApprovalPair({ body }, { body }); + const violations = findPrViolations({ number: 1176, headSha: HEAD, reviews }); + + assert.equal(reviews[0].body, reviews[1].body); + assert.notEqual(reviews[0].user.id, reviews[1].user.id); + assert.equal(isRequiredApprovalPair(reviews, HEAD), false); + assert.match( + violations.find((v) => v.startsWith("I1")) ?? "", + /the same body submitted under two credentials/, + ); + }); + + // The whitespace case has to be asserted HERE, through findPrViolations, and + // not only against duplicateBodyAcrossIdentities. That predicate does not + // gate anything — it picks the wording after I1 has already fired, and I1 + // fires only when isRequiredApprovalPair returns false. A first attempt at + // this fix trimmed inside the predicate alone; every variant below still + // audited as SOUND because the deciding branch compared raw bodies and + // exempted the pair before the predicate was consulted. The whole suite + // stayed green throughout, which is exactly why the end-to-end assertion is + // the one that matters. + it("rejects a body that differs only in surrounding whitespace under both credentials", () => { + const body = `## Ally — Consolidated PR Review\nReviewed head: ${HEAD}\n\n### Critical Issues (0)\n### Important Issues (0)\n`; + + for (const variant of [`${body}\n`, `${body} `, `\n${body}`, `\n ${body}\n\n`]) { + const reviews = requiredApprovalPair({ body }, { body: variant }); + const context = `variant ${JSON.stringify(variant)}`; + + assert.notEqual(reviews[0].body, reviews[1].body, `${context} must not be byte-identical`); + assert.equal(isRequiredApprovalPair(reviews, HEAD), false, context); + assert.match( + findPrViolations({ number: 1176, headSha: HEAD, reviews }).find((v) => + v.startsWith("I1"), + ) ?? "", + /one verdict, posted twice/, + context, + ); + } + }); + + // The counterweight: trimming must not collapse two genuinely distinct + // write-ups into a "duplicate", or the two-seat exemption stops working. + it("still accepts a pair whose bodies differ in substance, not just whitespace", () => { + const reviews = requiredApprovalPair( + { body: `Reviewed head: ${HEAD}\n\nApp reviewed the implementation. ` }, + { body: `\nReviewed head: ${HEAD}\n\nUser seat approval; see the App review above.\n` }, + ); + + assert.equal(isRequiredApprovalPair(reviews, HEAD), true); + assert.deepEqual(findPrViolations({ number: 1177, headSha: HEAD, reviews }), []); + }); + + it("names the duplicate shape rather than reporting a bare count", () => { + const body = `Reviewed head: ${HEAD}\n\nSame text, two seats.`; + const identical = requiredApprovalPair({ body }, { body }); + const [app, user] = requiredApprovalPair(); + const distinctButExtra = [app, user, { ...user, id: 13 }]; + + assert.match( + findPrViolations({ number: 1, headSha: HEAD, reviews: identical }).find((v) => + v.startsWith("I1"), + ) ?? "", + /one verdict, posted twice/, + ); + // A three-review set is a different failure with a different remedy, and + // must not borrow the duplicate-submission wording. + assert.match( + findPrViolations({ number: 2, headSha: HEAD, reviews: distinctButExtra }).find((v) => + v.startsWith("I1"), + ) ?? "", + /expected at most 1 or the exact App\/User APPROVED pair/, + ); + }); + it("rejects an extra operative retry instead of collapsing it", () => { const [app, user] = requiredApprovalPair(); const reviews = [app, user, { ...user, id: 13 }];