fix(ally-guard): reject one verdict submitted under both credentials (BLO-22916)

.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.
 

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}`,
     );
   }
 

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 }];