From 418df0e2f41c8fba429d5111b4c2a665b63472fb Mon Sep 17 00:00:00 2001 From: Ally Date: Sun, 16 Aug 2026 08:46:15 +0000 Subject: [PATCH 1/5] test(ally-guard): reject one verdict submitted under both credentials (BLO-22916) --- scripts/check-ally-review-consistency.mjs | 35 ++++++++- .../check-ally-review-consistency.test.mjs | 77 ++++++++++++++++++- 2 files changed, 109 insertions(+), 3 deletions(-) diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index db01c24abaef..dd89335c204a 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -125,10 +125,22 @@ function isExpectedApproval(review, { id, login }, headSha) { * 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. 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 *byte-identical* + * bodies 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. */ export function isRequiredApprovalPair(reviews, headSha) { const operative = operativeAllyReviews(reviews, headSha); if (operative.length !== 2) return false; + if (operative[0].body === operative[1].body) return false; return ( operative.some((review) => @@ -148,6 +160,21 @@ export function isRequiredApprovalPair(reviews, headSha) { ); } +/** + * True when two operative reviews carry byte-identical bodies 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). + */ +export function duplicateBodyAcrossIdentities(operative) { + const reviews = operative ?? []; + return reviews.some((a, i) => + reviews.some( + (b, j) => j > i && a?.body === b?.body && 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 +187,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..fdbc84c32064 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,8 +353,40 @@ function requiredApprovalPair(app = {}, user = {}) { ]; } -describe("I1 accepts only the protected-merge approval pair", () => { - it("accepts exactly one independently attested App/User approval pair", () => { +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, + ); + }); + + 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); + }); +}); + +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 }); @@ -371,6 +404,46 @@ 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/, + ); + }); + + 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 }]; From bbeab7ac004851745d648c806dacc203cfdfa844 Mon Sep 17 00:00:00 2001 From: Ally Date: Sun, 16 Aug 2026 08:47:32 +0000 Subject: [PATCH 2/5] fix(ally-guard): reject one verdict submitted under both credentials (BLO-22916) The I1 exemption for the App/User approval pair accepted any two bodies, so the 17 byte-identical dual-credential pairs this guard was pointed at all read as SOUND. Require the two bodies to differ: the exemption exists for a gate that genuinely needs both seats, where the User seat adds a short distinct approval, not for one verdict posted twice. --- .github/workflows/ally-review-consistency.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ally-review-consistency.yml b/.github/workflows/ally-review-consistency.yml index fed984fd7854..aea4500efbc7 100644 --- a/.github/workflows/ally-review-consistency.yml +++ b/.github/workflows/ally-review-consistency.yml @@ -2,7 +2,8 @@ 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, +# 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 From 1a57d8b109e94d1b7be9a9f47d69cf383fdcdab4 Mon Sep 17 00:00:00 2001 From: "allyblockcast[bot]" Date: Sun, 16 Aug 2026 09:01:40 +0000 Subject: [PATCH 3/5] refactor(ally-guard): don't call two bodiless approvals a duplicate verdict Follow-up to the consolidated review on #1385. Three suggestions, all confirmed against the live repo. 1. `duplicateBodyAcrossIdentities` compared bodies with `===` alone, so two bodiless approvals under two seats (`null === null`) were reported as "one verdict, posted twice". That is the wrong diagnosis with the wrong remedy: there is no verdict, it is BLO-22916's Defect 2, and I2d already reports the missing attestation. Guard on a truthy body. 2. Document the parameter contract. Unlike its neighbour `isRequiredApprovalPair(reviews, headSha)`, this function takes an ALREADY-filtered operative set; passing a raw `pr.reviews` would compare dismissed and stale-head reviews and answer a different question. 3. Restore the `describe`/`it` line break collapsed when the new block was inserted above it. Verification: - `node --test scripts/check-ally-review-consistency.test.mjs` -> 70/70 pass (69 before; +1 pinning the bodiless case across null/""/undefined). - Mutation-checked: dropping the truthiness guard fails that test. - Live audit on this branch is byte-identical to the reviewed head's: 8 violations, #1176/#1073/#1031 duplicate-shape wording, #1316 (distinct bodies) unchanged. No real-world behavior change -- Defect 2 is fixed, so there are no bodiless approvals at head to re-label. Refs BLO-22916 --- scripts/check-ally-review-consistency.mjs | 15 ++++++++++++++- .../check-ally-review-consistency.test.mjs | 19 ++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index dd89335c204a..7a730e6b4cc2 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -165,12 +165,25 @@ export function isRequiredApprovalPair(reviews, headSha) { * 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). + * + * An empty body 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 ?? []; return reviews.some((a, i) => reviews.some( - (b, j) => j > i && a?.body === b?.body && a?.user?.id !== b?.user?.id, + (b, j) => + j > i && Boolean(a?.body) && a?.body === b?.body && a?.user?.id !== b?.user?.id, ), ); } diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index fdbc84c32064..a1eda4b096f7 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -384,9 +384,26 @@ describe("duplicateBodyAcrossIdentities", () => { 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. + it("does NOT fire on bodiless reviews under two identities", () => { + for (const empty of [null, "", undefined]) { + 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", () => { +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 }); From 12d2f2091e8e8435438ad0a8f8e07df557dee0ad Mon Sep 17 00:00:00 2001 From: "allyblockcast[bot]" Date: Sun, 16 Aug 2026 09:11:08 +0000 Subject: [PATCH 4/5] fix(ally-guard): compare review bodies after trimming whitespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit duplicateBodyAcrossIdentities compared bodies byte-exactly, so an App/User pair differing only by a trailing newline audited as SOUND. The named defect mechanism (one --body-file passed to both `gh pr review` calls) does produce byte-identical bodies, but a stray trailing newline is still one verdict posted twice. Normalizing once also closes a hole in the emptiness guard: `Boolean(" ")` is truthy, so a whitespace-only pair was reported as a duplicate verdict — contradicting the documented contract that bodiless pairs belong to I2d ("an approval with no review behind it"), whose remedy differs. Guarding on the trimmed value makes the two tests agree. Trimming is deliberately the only normalization; substantially-overlapping bodies are a larger scope question than this predicate should answer. Also closes the em-dash aside in the workflow header comment, which left the second and third assertions reading as part of the `since ...` clause rather than as list items. Comment-only; non-comment bytes are unchanged. Tests: 70 -> 71. Removing the .trim() fails 2. --- .github/workflows/ally-review-consistency.yml | 13 +++++---- scripts/check-ally-review-consistency.mjs | 29 ++++++++++++------- .../check-ally-review-consistency.test.mjs | 17 ++++++++++- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ally-review-consistency.yml b/.github/workflows/ally-review-consistency.yml index aea4500efbc7..f1dc8027aa3a 100644 --- a/.github/workflows/ally-review-consistency.yml +++ b/.github/workflows/ally-review-consistency.yml @@ -1,11 +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 — 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. +# 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 7a730e6b4cc2..fdc73376ba43 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -161,16 +161,24 @@ export function isRequiredApprovalPair(reviews, headSha) { } /** - * True when two operative reviews carry byte-identical bodies 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). + * 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). * - * An empty body 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. + * Bodies are compared after trimming surrounding whitespace. 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. + * Trimming is deliberately the only normalization: two bodies that differ in + * substance are two write-ups, and deciding when overlapping prose counts as + * one verdict is a larger question than this predicate should answer. + * + * 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 @@ -180,10 +188,11 @@ export function isRequiredApprovalPair(reviews, headSha) { */ export function duplicateBodyAcrossIdentities(operative) { const reviews = operative ?? []; + const bodies = reviews.map((review) => String(review?.body ?? "").trim()); return reviews.some((a, i) => reviews.some( (b, j) => - j > i && Boolean(a?.body) && a?.body === b?.body && a?.user?.id !== b?.user?.id, + j > i && bodies[i] !== "" && bodies[i] === bodies[j] && a?.user?.id !== b?.user?.id, ), ); } diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index a1eda4b096f7..439547f42947 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -363,6 +363,20 @@ describe("duplicateBodyAcrossIdentities", () => { ); }); + // 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")]), @@ -388,8 +402,9 @@ describe("duplicateBodyAcrossIdentities", () => { // 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]) { + for (const empty of [null, "", undefined, " ", "\n\n", "\t "]) { assert.equal( duplicateBodyAcrossIdentities([ at(1, 290875700, empty), From 6d4b4e52313a379291de18bb51fa5528c45e7059 Mon Sep 17 00:00:00 2001 From: "allyblockcast[bot]" Date: Sun, 16 Aug 2026 09:21:54 +0000 Subject: [PATCH 5/5] fix(ally-guard): trim at the branch that decides the verdict, not downstream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit trimmed bodies in duplicateBodyAcrossIdentities, but that predicate does not gate anything — it selects the wording after I1 has already fired, and I1 fires only when isRequiredApprovalPair returns false. That function still compared raw bodies, so an App/User pair differing only by a trailing newline was classified as the legitimate distinct-body exemption and returned SOUND. The trim was real but unreachable for the case it targeted. Extract normalizedBody(review) and use it at both comparison sites. Keeping one helper is the actual fix for the class of bug: the two sites must agree on what "the same body" means, and the deciding one runs first, so normalizing either alone is a silent no-op rather than a partial improvement. Behavior, verified end-to-end through findPrViolations: - `+ "\n"`, `+ " "`, and `"\n" +` variants now report `I1 … one verdict, posted twice (BLO-22916)`; before, all three returned []. - Bodies differing in substance still pass, so the two-seat exemption for an App-authored or CODEOWNERS-gated PR is intact. - The bodiless path is unchanged across null, "" and " ": I1 with the bare count plus two I2d, not the duplicate wording, since I2d's remedy is to post a comment rather than drop a submission. The regression test asserts through findPrViolations, not against the predicate in isolation. The existing whitespace unit test passed against the broken build — a green test on a predicate that decides nothing is what let this ship — so the new case is checked to fail when the deciding comparison is reverted. Addresses Ally review feedback on #1385 (Important + Suggestion, both at scripts/check-ally-review-consistency.mjs). Refs BLO-22916. --- scripts/check-ally-review-consistency.mjs | 67 +++++++++++++------ .../check-ally-review-consistency.test.mjs | 40 +++++++++++ 2 files changed, 88 insertions(+), 19 deletions(-) diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index fdc73376ba43..120bb47692e0 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -120,27 +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. 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 *byte-identical* - * bodies 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. + * 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 (operative[0].body === operative[1].body) return false; + if (normalizedBody(operative[0]) === normalizedBody(operative[1])) return false; return ( operative.some((review) => @@ -166,13 +199,9 @@ export function isRequiredApprovalPair(reviews, headSha) { * submitting its verdict twice rather than two independent passes (two passes * produce two different write-ups). * - * Bodies are compared after trimming surrounding whitespace. 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. - * Trimming is deliberately the only normalization: two bodies that differ in - * substance are two write-ups, and deciding when overlapping prose counts as - * one verdict is a larger question than this predicate should answer. + * 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 — @@ -188,7 +217,7 @@ export function isRequiredApprovalPair(reviews, headSha) { */ export function duplicateBodyAcrossIdentities(operative) { const reviews = operative ?? []; - const bodies = reviews.map((review) => String(review?.body ?? "").trim()); + const bodies = reviews.map(normalizedBody); return reviews.some((a, i) => reviews.some( (b, j) => diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index 439547f42947..cecc846381e2 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -454,6 +454,46 @@ describe("I1 accepts only the protected-merge approval pair", () => { ); }); + // 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 });