[codex] fix(review): keep duplicate Ally submissions fail-closed (BLO-19778)

.github/workflows/ally-review-consistency.yml

@@ -1,10 +1,12 @@
 name: Ally Review Consistency Guard
 
-# Asserts that every open PR carries at most one operative Ally review verdict
-# at its current head, that no standing APPROVED masks a Critical/Important
-# finding, and that a review's body-attested head matches the commit GitHub
-# recorded it against. See scripts/check-ally-review-consistency.mjs and
-# BLO-19778 for the incident this guards.
+# Asserts that every open PR carries at most one operative Ally review at its
+# current head, except for the exact current-head App/User approval pair
+# required by protected merge,
+# that no standing APPROVED masks a Critical/Important finding, and that a
+# review's body-attested head matches the commit GitHub recorded it against.
+# See scripts/check-ally-review-consistency.mjs and BLO-19778 for the incident
+# this guards.
 
 on:
   schedule:
@@ -28,7 +30,7 @@ jobs:
       - name: Checkout repository
         uses: actions/checkout@v5
 
-      - name: Assert one operative Ally verdict per PR head
+      - name: Assert the allowed operative Ally review shape per PR head
         env:
           GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
           ALLY_REVIEW_REPO: ${{ inputs.repo || 'Blockcast/paperclip' }}

scripts/check-ally-review-consistency.mjs

@@ -16,15 +16,35 @@
  * apart both submitted at head ff1c72db, 34 s apart, with opposite verdicts.
  *
  * Invariants asserted here:
- *   I1  At most one operative Ally verdict per (PR, head SHA).
+ *   I1  At most one operative Ally review submission per (PR, head SHA), or
+ *       exactly the current-head APPROVED App/User pair mandated by protected
+ *       merge. Every other multi-review shape is fatal. The pair is checked as
+ *       two independent, exact identities—not by removing or normalizing a
+ *       review—so retries and lookalike credentials remain visible.
  *   I2  No operative Ally APPROVED whose own body reports a Critical or
- *       Important finding, and no operative APPROVED coexisting at a SHA with
- *       an operative Ally review that does.
+ *       Important finding, no operative APPROVED coexisting at a SHA with an
+ *       operative Ally review that does, and no operative APPROVED that makes
+ *       no `Reviewed head:` attestation at all.
  *   I3  An operative Ally review's body-attested `Reviewed head:` matches the
- *       commit GitHub recorded it against. `gh pr review` attaches a review to
- *       whatever the head is at submit time, so a mid-review push silently
- *       certifies a tree that was never read (seen on #870: body attested
- *       b3a240ec, commit_id was 67965f1e).
+ *       commit GitHub recorded it against, so a standing approval is not
+ *       presented against a tree its author never read.
+ *
+ * On I3's mechanism. An earlier revision of this file said `gh pr review`
+ * binds a review to the head at submit time, so a mid-review push "certifies a
+ * tree that was never read". Submit-time binding is real but it is not what
+ * produces most I3 hits, and the difference matters because the old wording
+ * blamed the reviewer for a value the reviewer never set. Measured on #1104
+ * (2026-08-07): review 4878131987 was submitted at 20:52:00Z attesting
+ * 2533dc6f, the timeline records `head_ref_force_pushed cee75d97` at 21:07:07Z
+ * creating a commit dated 21:06:55Z, and the review's `commit_id` now reads
+ * cee75d97 — a commit that did not exist when it was submitted. A review
+ * cannot be bound at submit time to a commit created 15 minutes later, so a
+ * force-push re-anchors existing reviews forward onto the new head. Same shape
+ * on #1098 (+28m), #1111 (+5m), #1067 (+10h23m). `commit_id` is therefore not
+ * a record of which tree a review examined; the body's attestation line is.
+ * I3 stays fatal because the hazard is real either way — a green is being
+ * presented at a head nobody read — but the remedy is to dismiss or re-review
+ * the stale approval, not to correct the reviewer.
  *
  * "Operative" excludes DISMISSED and PENDING: a dismissed review is disposed,
  * not a standing attestation.
@@ -56,6 +76,14 @@ const STILL_PRESENT_DISPOSITION_RE =
 /** The single standalone attestation line Ally is required to emit. */
 const ATTESTED_HEAD_RE = /^[ \t]*(?:[_*]+)?[ \t]*reviewed head:[ \t]*`?([0-9a-f]{40})`?[ \t]*(?:[_*]+)?[ \t]*$/im;
 
+// The two distinct GitHub principals required by the protected-merge policy.
+// Pin both the immutable REST ID and the canonical login: either mismatch is
+// not an eligible substitute for the required artifact.
+export const ALLY_APP_REVIEWER_ID = 290875700;
+export const ALLY_APP_REVIEWER_LOGIN = "allyblockcast[bot]";
+export const ALLY_USER_REVIEWER_ID = 296676656;
+export const ALLY_USER_REVIEWER_LOGIN = "allyblockcast";
+
 export function isAllyLogin(login) {
   return ALLY_LOGIN_RE.test(String(login ?? ""));
 }
@@ -83,6 +111,43 @@ export function operativeAllyReviews(reviews, headSha) {
   );
 }
 
+function isExpectedApproval(review, { id, login }, headSha) {
+  return (
+    review?.state === "APPROVED" &&
+    review?.user?.id === id &&
+    review?.user?.login === login &&
+    attestedHead(review?.body) === String(headSha ?? "").toLowerCase()
+  );
+}
+
+/**
+ * The only permitted two-review shape: one current-head approval from the
+ * required App identity and one from the required User seat. This deliberately
+ * inspects the full operative set instead of deduplicating it; a retry, an
+ * unexpected identity, or a missing/stale attestation makes the shape fail.
+ */
+export function isRequiredApprovalPair(reviews, headSha) {
+  const operative = operativeAllyReviews(reviews, headSha);
+  if (operative.length !== 2) return false;
+
+  return (
+    operative.some((review) =>
+      isExpectedApproval(
+        review,
+        { id: ALLY_APP_REVIEWER_ID, login: ALLY_APP_REVIEWER_LOGIN },
+        headSha,
+      ),
+    ) &&
+    operative.some((review) =>
+      isExpectedApproval(
+        review,
+        { id: ALLY_USER_REVIEWER_ID, login: ALLY_USER_REVIEWER_LOGIN },
+        headSha,
+      ),
+    )
+  );
+}
+
 /**
  * @param {{number: number, headSha: string, reviews: object[]}} pr
  * @returns {string[]} human-readable violations; empty when the PR is sound
@@ -93,10 +158,10 @@ export function findPrViolations(pr) {
   const operative = operativeAllyReviews(pr.reviews, head);
   const violations = [];
 
-  if (operative.length > 1) {
+  if (operative.length > 1 && !isRequiredApprovalPair(pr.reviews, head)) {
     const detail = operative.map((r) => `${r.state}/${r.id}`).join(", ");
     violations.push(
-      `I1 PR #${pr.number} @${short}: ${operative.length} operative Ally verdicts (${detail}) — expected at most 1`,
+      `I1 PR #${pr.number} @${short}: ${operative.length} operative Ally reviews (${detail}) — expected at most 1 or the exact App/User APPROVED pair`,
     );
   }
 
@@ -114,6 +179,18 @@ export function findPrViolations(pr) {
         `I2c PR #${pr.number} @${short}: review ${review.id} is APPROVED but its body marks a prior finding still-present`,
       );
     }
+    // An approval that makes no attestation at all is the strictly worse case:
+    // it counts toward `reviewDecision` while claiming nothing about any tree.
+    // Seen live on #1114 — a 129-byte APPROVED reading "Approved the current CI
+    // head … this head only retriggers checks", satisfying required-review on a
+    // PR whose same-head Ally review carried 3 still-present Important
+    // findings, with auto-merge armed. Acknowledging a CI retrigger is a
+    // comment, never an approval.
+    if (attestedHead(review.body) === null) {
+      violations.push(
+        `I2d PR #${pr.number} @${short}: review ${review.id} is APPROVED but its body makes no "Reviewed head:" attestation — an approval with no review behind it`,
+      );
+    }
   }
 
   if (approvals.length > 0 && blocking.length > 0) {
@@ -135,7 +212,7 @@ export function findPrViolations(pr) {
     const attested = attestedHead(review.body);
     if (attested && attested !== String(head ?? "").toLowerCase()) {
       violations.push(
-        `I3 PR #${pr.number} @${short}: review ${review.id} attests head ${attested.slice(0, 8)} but GitHub recorded it against ${short} — it certifies a tree it never reviewed`,
+        `I3 PR #${pr.number} @${short}: review ${review.id} attests head ${attested.slice(0, 8)} but is now recorded against ${short} — a force-push re-anchored it, so it stands as an attestation of a tree its author never read`,
       );
     }
   }
@@ -173,6 +250,29 @@ export function assertPrListComplete(rows, repo, limit = PR_LIST_LIMIT) {
   return rows;
 }
 
+/**
+ * Every invariant here pivots on `headSha`: `operativeAllyReviews` filters
+ * `commit_id === headSha`, so a falsy or malformed head matches no review, the
+ * operative set is empty, and I1/I2/I3 all iterate nothing. The run then prints
+ * a pass having asserted nothing across every PR at once — the same fail-open
+ * shape as an unreachable `main()`, one layer up. Verified: with `headSha` set
+ * to `undefined`, `null` or `""`, a deliberately maximal violation (an APPROVED
+ * reporting `### Critical Issues (3)`, attesting a different SHA, coexisting
+ * with a blocking COMMENTED) yields zero violations. Assert it for the same
+ * reason `assertPrListComplete` throws rather than warns.
+ */
+export function assertHeadSha(row, repo) {
+  if (!/^[0-9a-f]{40}$/.test(String(row?.headRefOid ?? ""))) {
+    throw new Error(
+      `gh pr list returned no usable headRefOid for ${repo}#${row?.number} ` +
+        `(got ${JSON.stringify(row?.headRefOid)}). Every invariant in this guard ` +
+        `filters reviews on commit_id === head, so continuing would assert ` +
+        `nothing while reporting a pass.`,
+    );
+  }
+  return row;
+}
+
 function fetchOpenPrs(repo) {
   // number + headRefOid both come back from this one call; fetching the head
   // via `gh api repos/{repo}/pulls/{number}` instead would pull a ~22 KB
@@ -195,7 +295,7 @@ function fetchOpenPrs(repo) {
   assertPrListComplete(rows, repo);
 
   return rows.map((row) => ({
-    number: row.number,
+    number: assertHeadSha(row, repo).number,
     headSha: row.headRefOid,
     reviews: JSON.parse(
       gh(["api", `repos/${repo}/pulls/${row.number}/reviews`, "--paginate"]),