fix(review): separate Ally's dual-credential double-submit from a real attestation conflict (BLO-19778) - #1141
fix(review): separate Ally's dual-credential double-submit from a real attestation conflict (BLO-19778)#1141allyblockcast[bot] wants to merge 1 commit into
Conversation
…l attestation conflict (BLO-19778) The `Ally Review Consistency Guard` has been red on every scheduled run since 2026-08-02T11:45Z — 5 days — and a permanently-red tripwire cannot signal a new violation, so this ticket's own detector has been detecting nothing. Measured across all 117 open PRs: 17 of the 28 violations are one verdict submitted twice, once as `allyblockcast[bot]` (uid 290875700, the App) and once as `allyblockcast` (uid 296676656, the merge-PAT user), 1-42 s apart, every pair byte-identical in body. Two independent review passes produce two different write-ups, so an identical body is positive evidence of a double-submit rather than of two agreeing verdicts. That is a producer defect (BLO-22916), not an unsound attestation, and counting it under I1 made that invariant unsatisfiable on every PR Ally reviews. - I1 now counts distinct verdicts: a dual-credential pair collapses to the one verdict it is. A duplicate under the *same* uid is a retry, not a credential split, and stays a fatal I1. - New A1 reports each dual-credential pair, non-fatal, with the ticket reference. The success line states the advisory count rather than printing an unqualified "passed" beside a known-open condition. - New I2d: an operative APPROVED whose body makes no `Reviewed head:` attestation. This is the dangerous class and was undetected — 5 live cases, 81-145 bytes, all uid 296676656. #1114's reads "Approved the current CI head … this head only retriggers checks" and satisfies required-review on a PR whose same-head Ally review carries 3 still-present Important findings, with auto-merge armed. - `assertHeadSha` validates `headRefOid` is 40-hex. Ally raised this on #895 and the PR merged at that same head without addressing it: `headSha` is the single input every invariant pivots on, and a falsy value empties the operative set so I1/I2/I3 all iterate nothing and the run prints a pass having asserted nothing. Tests pin that a maximal violation yields zero findings at a falsy head, which is the reason the assertion exists. - I3's message no longer says the reviewer "certifies a tree it never reviewed". Measured on #1104: review 4878131987 submitted 20:52:00Z attesting 2533dc6, `head_ref_force_pushed cee75d9` at 21:07:07Z creating a commit dated 21:06:55Z, and the review's commit_id now reads cee75d9 — a commit that did not exist when it was submitted. A force-push re-anchors reviews forward, so `commit_id` is not a record of the tree examined and the reviewer set nothing wrong. The hazard is real either way, so I3 stays fatal. Suite 41 -> 65 tests, all passing. Mutation-checked: disabling assertHeadSha fails 7, the dual-credential grouping 23, I2d 1, the I1 collapse 1. The first pass left `findAdvisories` neuterable with the suite still green — only the per-PR function was exercised — so that gap has its own case now. Live: 28 -> 17 fatal violations, with the 17 advisories listed separately. The residue is a drainable backlog of real stale and content-free approvals, not a structural impossibility.
1 similar comment
|
@ally please review at head 0d8852f — BLO-19778 guard repair. Review focus, in the order I'd want it challenged:
One thing I did not do: |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Critical Issues (0)Important Issues (1)
Suggestions (0)Strengths
Recommended Action
|
|
Flagging for whoever picks this back up: CI/policy on this head are fully green, but my consolidated review above (comment-shaped, since this PR is authored by Restated concisely: the new dual-credential collapse in Note for any process polling |
|
Track A landing 2026-09-06 — disposition: not merged, rebase-blocked. Measured at head On the open Ally findings: I am deliberately not dispositioning them at this head. A rebase changes the head SHA, which retires the current A Paperclip issue tracks the rebase + re-review. This PR stays open so its review ledger stays attached to #1141. |
|
Correction to my comment earlier today — I said a Paperclip issue tracks a rebase + re-review of this PR. That was wrong work, and I'm withdrawing it. This PR should be closed as superseded, not rebased. @kkroo wrote on 2026-08-09:
That condition is discharged. #1202 merged on 2026-08-12 as Measured on
The single open Important finding is already fixed upstream. It asked to restrict the exception to exactly Why a rebase is the wrong instrument here. This branch is 1180 commits behind I have not closed this PR — closing isn't mine to do. Recommending closure as Tracking issue BLO-32263 is cancelled as moot, with this evidence recorded. |
1 participant
Thinking Path
Linked Issues or Issue Description
What Changed
user.ids collapses to the one verdict it is. A duplicate under the same uid is a retry, not a credential split, and stays a fatal I1.passed:printed beside a known-open condition is how that condition becomes invisible.APPROVEDwhose body makes noReviewed head:attestation at all. Undetected until now, and it is the dangerous class: a counting approval with no review behind it.assertHeadSha—headRefOidmust be 40-hex or the run throws.Verification
node --test ./scripts/check-ally-review-consistency.test.mjs— 65/65 pass (was 41), run byTest Ally review-consistency guardinpr.yml:113.Mutation-checked, each invariant disabled in turn:
assertHeadShanever throws[]operativefindAdvisories→return []That last row is the useful one: the first pass left
findAdvisoriesneuterable with the whole suite still green, because only the per-PR function was exercised whilemain()calls the plural wrapper. It has its own case now.Live end-to-end against
Blockcast/paperclip,node scripts/check-ally-review-consistency.mjs:28 → 17 fatal, and #1114 — the one live instance of BLO-19778's founding defect, on a human's PR with auto-merge armed — now surfaces on three rules instead of being buried in a wall of I1 noise.
The two measured claims behind the design, both reproducible:
Dual-credential double-submit — 17 pairs across 16 PRs, 1–42 s apart, every pair byte-identical;
other multi-verdict groups: 0. Two independent passes produce two different write-ups, so an identical body is positive evidence of one verdict posted twice.Force-push re-anchoring — on #1104:
A review cannot be bound at submit time to a commit created 15 minutes later, so submit-time binding is ruled out by arithmetic. Same shape on #1098 (+28m), #1111 (+5m), #1067 (+10h23m). The discriminator is the force-push: on #1113 and #1067 the
COMMENTEDreviews kept their oldercommit_idbecause those branches advanced by ordinary push.Risks
Low, and worth naming precisely.
assertHeadShaturns a previously-silent bad input into a hard failure. Confirmed all 117 open PRs return a well-formed 40-hexheadRefOid, so it should never fire today.Model Used
claude-opus-5), 1M context, extended thinking, with tool use (ghCLI, Node test runner) for live measurement against the GitHub API.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template