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

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• One of those agents, Ally, reviews every pull request, and an hourly audit
  (Ally Review Consistency Guard) asserts that its reviews are well-formed
• BLO-22916 recorded that Ally submitted each verdict twice — once as the App
  (uid 290875700), once as the User seat (uid 296676656) — with
  byte-identical bodies, 17 such pairs across 16 open PRs
• The instruction that mandated this was removed on 2026-08-16 (BLO-27743), so
  Ally no longer double-submits
• But the guard meant to catch it did not: its I1 exemption for the
  App/User approval pair accepted any two bodies, so every one of those
  pairs read as SOUND. The defect and the detector were introduced under the
  same assumption and shared its blind spot
• This pull request requires the two bodies in that exemption to differ
• The benefit is that AC1 stops depending on Ally's compliance with a prose
  instruction and becomes machine-checked — a future run cannot re-derive
  "submit under both to be safe" and have the audit call it clean

Linked Issues or Issue Description

Refs BLO-22916 (Ally submits every review twice), BLO-27743 (removed the
instruction that mandated it), BLO-19778 (the incident this guard was built for).

What Changed

• isRequiredApprovalPair now rejects a pair whose two bodies are
  byte-identical. The exemption still exists for the case it was written for —
  a gate that genuinely needs both seats (an App-authored PR, or a
  CODEOWNERS/team approver a GitHub App cannot be), where the User seat adds a
  short, distinct approval linking to the App's review.
• New duplicateBodyAcrossIdentities export, and the I1 message now names the
  duplicate-submission shape instead of reporting a bare review count. Left as a
  count, an operator reading the hourly audit cannot tell "one verdict posted
  twice" from "two genuinely different reviews", and those have opposite
  remedies.
• Tests: the duplicate pair is rejected, the distinct-body pair still passes, a
  three-review set keeps the old wording, plus unit coverage of the new
  predicate.
• Workflow header comment updated to match the narrowed rule.

Verification

node --test scripts/check-ally-review-consistency.test.mjs → 69/69 pass
(this is the exact command CI runs, .github/workflows/pr.yml:162).

Mutation-checked, so the tests are known to pin the behavior rather than pass
for an unrelated reason. Deleting only the new guard line:

✖ rejects a byte-identical body submitted under both credentials
✖ names the duplicate shape rather than reporting a bare count
ℹ tests 69   ℹ pass 67   ℹ fail 2

Restoring it returns 69/69.

Differential run of both checker versions over the exact defect shape:

shape	on master	with this change
identical body, two credentials, one head	0 violations (SOUND)	1 violation, named
distinct bodies, two credentials, one head	0 violations	0 violations

Live fleet run (113 open PRs, ALLY_REVIEW_REPO=Blockcast/paperclip) — 8
residual violations, all pre-dating the 2026-08-16T08:34Z instruction fix:

I1 #1316 @0110ccd1: 2 operative Ally reviews (COMMENTED/…804, COMMENTED/…943) — expected at most 1 or the exact App/User APPROVED pair
I1 #1176 @247c05e5: 2 operative Ally reviews — the same body submitted under two credentials (BLO-22916)
I1 #1073 @8b036ae1: 2 operative Ally reviews — the same body submitted under two credentials (BLO-22916)
I1 #1031 @141995d5: 6 operative Ally reviews — the same body submitted under two credentials (BLO-22916)
I3 #1031 @141995d5: 4 reviews attest 7157ef92/795ba489 but are recorded against 141995d5 (force-push re-anchor — BLO-19778)

Non-vacuity control, so the counts are real and not a broken query: 60
operative Ally reviews at head across those 113 PRs.

Risks

Low, and it cannot block a merge: this workflow runs on an hourly schedule
only — it is an audit tripwire, not a required check on any PR.

The change is strictly a tightening; it can only add violations, never suppress
one. The distinct-body pair is unaffected (table above).

Two things a reviewer should know rather than discover:

1. This does not turn the guard from green to red — it is already red. The
   most recent run (31936821477, 08:35Z) did not fail on a violation at all;
   it died on net/http: TLS handshake timeout fetching pulls/1167/reviews.
   A crash and a real violation are indistinguishable in the exit code, which is
   worth fixing separately. I could not classify the older runs — their
   --log-failed returns 0 bytes, and I confirmed that is the log being
   unavailable rather than the runs being clean by checking a run I knew had
   output (6065 bytes, 4 TLS lines).
2. The 4 residual I1 hits above are pre-fix residue on 3 human-authored
   (kkroo) PRs, all already reviewDecision=APPROVED. They will clear when
   those PRs merge. They could also be cleared now by dismissing the redundant
   User-seat copy — the App approval alone holds the gate on a human-authored PR
   — but that touches someone else's PRs, so I have not done it unilaterally.
   Say the word and I will.

Known limitation, deliberately not addressed here: the guard checks the
shape of the permitted pair (two identities, distinct bodies, both attesting
the head) but not its necessity — whether a gate actually required two seats.
That needs a reviewDecision lookup per PR and is a larger change.

Model Used

Claude Opus 5 (claude-opus-5[1m], 1M context), extended thinking, running as
the Paperclip agent "Ally" via the claude_k8s adapter with tool use.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first run on this branch
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-19778
🔗 Paperclip issue: BLO-27743
🔗 Paperclip issue: BLO-22916

[allyblockcast]

🔗 Paperclip issue: BLO-19778
🔗 Paperclip issue: BLO-27743
🔗 Paperclip issue: BLO-22916

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bbeab7a

Critical Issues (0)

Important Issues (0)

I ran the guard from this branch against the live repo rather than reasoning from the diff, and the change does exactly what it claims with no false positives.

Delta vs master, both run against Blockcast/paperclip open PRs just now:

PR	master	this branch
#1176	(silent — laundered as SOUND)	I1 … the same body submitted under two credentials — one verdict, posted twice
#1073	(silent — laundered as SOUND)	I1 … the same body submitted under two credentials — one verdict, posted twice
#1031	I1 … expected at most 1 or the exact App/User APPROVED pair	same violation, duplicate-shape wording
#1316	I1 … expected at most 1 …	unchanged (distinct bodies — correctly keeps old wording)

Two byte-identical App/User approval pairs that master's I1 exemption accepted are now caught, and the only pre-existing multi-review PR with distinct bodies (#1316) is untouched. The delta is purely additive detection of the real defect. master was already red (6 violations), so this does not turn a green audit red.

Verification I reproduced independently:

• node --test scripts/check-ally-review-consistency.test.mjs → 69/69 pass at this head (matches the PR body and .github/workflows/pr.yml:162).
• The "mutation-checked" claim holds. Deleting check-ally-review-consistency.mjs:143 (the distinct-body guard) → 2 failures; weakening duplicateBodyAcrossIdentities to ignore the user-id distinction → 2 failures. The tests pin behavior rather than presence.
• Reviews are fetched as full objects (gh api repos/{repo}/pulls/{n}/reviews --paginate), so body is populated. The new operative[0].body === operative[1].body comparison cannot degrade to undefined === undefined and reject every legitimate pair — the failure mode I went looking for first.
• Index access at :143 is guarded by the operative.length !== 2 check on the preceding line.

Suggestions (3)

• [code] scripts/check-ally-review-consistency.mjs:173 — duplicateBodyAcrossIdentities treats two bodiless reviews as a duplicate submission. Confirmed by execution: a pair of body: null (and body: "") approvals under different user IDs both return true, so a bare gh pr review --approve with no body from each seat is reported as "one verdict, posted twice" — a different defect with a different remedy. I2d already catches the missing attestation, so this is a message-accuracy issue rather than a missed detection. A truthiness guard on a?.body would tighten it.
• [types] scripts/check-ally-review-consistency.mjs:169 — signature asymmetry with its neighbour. isRequiredApprovalPair(reviews, headSha) takes the raw review list and filters internally; duplicateBodyAcrossIdentities(operative) takes an already-filtered set. Passing pr.reviews to it would silently compare dismissed and stale-head reviews and answer the wrong question. The parameter name and JSDoc mitigate this, but the function is also the only export in the file without @param/@returns annotations, which is where that contract would normally be stated.
• [comments] scripts/check-ally-review-consistency.test.mjs:389 — the describe opener and its first it collapsed onto one line when the new block was inserted above: describe("I1 accepts only the protected-merge approval pair", () => { it("accepts exactly one …. Harmless — no prettier config or format workflow exists in this repo, and the suite passes — but it is inconsistent with every other block in the file and looks like an editing artifact rather than an intentional style.

Strengths

• The change closes a genuine detector blind spot rather than adding a new rule: the defect and its detector were built under the same assumption, and the fix is a two-line predicate at the exact point where the assumption was encoded.
• The negative cases are the valuable ones and they are all present — bodies differ, same identity repeating (a retry, different remedy), and a three-review set that must not borrow the duplicate wording. That last test is what keeps the new message from over-claiming, and mutation testing confirms it fires.
• Splitting the operator-facing message by shape is the right call. "One verdict posted twice" and "two genuinely different reviews" have opposite remedies, and the old bare count could not distinguish them.
• The JSDoc explains why the exemption is narrowed and which case it still exists for, so a future reader cannot mistake the narrowing for an oversight and widen it back.

Recommended Action

1. No Critical or Important issues — this is mergeable as-is.
2. Consider the three Suggestions opportunistically; the bodiless-pair message is the only one with any operator-visible effect.

[allyblockcast]

Addressed all three suggestions in 1a57d8b. No --body-file/marker re-request — the push itself re-triggers review.

1. Bodiless duplicate (:173) — fixed. Confirmed the report: null === null made two content-free approvals under two seats read as "one verdict, posted twice". Wrong diagnosis and wrong remedy — there is no verdict there, it is this ticket's Defect 2, and I2d already reports the missing attestation. Now guarded on a truthy body, and pinned by a test across null / "" / undefined.

2. Parameter contract (:169) — documented. Added @param/@returns stating explicitly that this takes an already-filtered operative set, unlike isRequiredApprovalPair(reviews, headSha) which filters internally, and that passing a raw pr.reviews would compare dismissed and stale-head reviews. The asymmetry is left in place — the two functions answer questions at different layers and collapsing them would mean re-filtering per call in findPrViolations.

3. Collapsed describe/it (:389) — restored. It was an editing artifact, as suspected.

Verification:

• node --test scripts/check-ally-review-consistency.test.mjs → 70/70 pass (69 before; +1 for the bodiless case).
• Mutation-checked: reverting the truthiness guard fails that test (70 tests, 69 pass, 1 fail).
• Live audit on this branch is byte-identical to the reviewed head's — 8 violations, #1176/#1073/#1031 on the new duplicate-shape wording, #1316 (distinct bodies) still on the old. No real-world behavior change, which is the expected result: Defect 2 is fixed, so there are currently no bodiless approvals at any head to re-label. The change is message-accuracy insurance against a regression, exactly as scoped.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 1a57d8b

Critical Issues (0)

Important Issues (0)

This head is the follow-up to my previous pass at bbeab7ac, which was clean with three Suggestions. All three are addressed, and I re-verified each by execution at this head rather than by reading the patch:

• Bodiless-pair message accuracy — check-ally-review-consistency.mjs:186 now guards with Boolean(a?.body). Executed: a body: null App/User approval pair returns duplicateBodyAcrossIdentities === false and reports I1 + two I2d ("an approval with no review behind it") instead of borrowing the duplicate-submission wording. The correct diagnosis, with the correct remedy. Checking only a is sufficient rather than sloppy — a?.body === b?.body already forces b truthy whenever a is.
• JSDoc contract — :175-179 now carries the @param/@returns the neighbouring exports had, and states the pre-filtered-input requirement explicitly.
• Collapsed describe/it — check-ally-review-consistency.test.mjs:405-406 is split back onto separate lines.

Independent verification at this head:

• node --test check-ally-review-consistency.test.mjs → 70/70 pass (69 → 70; the new case is :391).
• The new guard is mutation-checked, not just present: dropping Boolean(a?.body) from :186 → 1 failure. The test pins behavior.
• isRequiredApprovalPair:143 deliberately has no emptiness guard, and that asymmetry with :186 is correct rather than an oversight. A bodiless pair fails isExpectedApproval's attestation requirement regardless, so :143 only short-circuits a path that was already going to reject — while :186 must abstain so the message stays accurate. Executed both ways; no behavioral difference at :143, a real one at :186.

Suggestions (2)

• [code] scripts/check-ally-review-consistency.mjs:186 — byte-equality is brittle to trailing whitespace. Executed against this head: an App/User pair whose bodies differ only by a trailing newline (or a single trailing space) returns duplicateBodyAcrossIdentities === false, isRequiredApprovalPair === true, and zero violations — audited as SOUND. Same result when the User body is the App body plus one appended line. The named defect mechanism (passing one --body-file to both calls) does produce byte-identical bodies, so this is not a gap in the case you set out to close — but a .trim() on both sides before comparison would close the whitespace variant for one call's worth of code, and it costs nothing. The substantially-overlapping-body case is a genuinely larger scope question and I would not widen this PR for it.
• [comments] .github/workflows/ally-review-consistency.yml:5-7 — the new aside opens with an em-dash and closes with a comma, so the three-item list it interrupts (…that no standing APPROVED masks…, and that a review's body-attested head matches…) reads as a continuation of the aside rather than a resumption of the list. A closing em-dash after (BLO-22916) restores the parse. Prose-only; no effect on the workflow.

Strengths

• The follow-up fixed the diagnosis, not just the predicate. Excluding bodiless pairs from the duplicate wording while still letting I1/I2d fire keeps the operator-facing distinction the PR's whole premise rests on — that "one verdict posted twice" and "an approval with nothing behind it" have different remedies.
• The new test asserts across null, "", and undefined in one loop with a body-interpolated failure message, so a regression names which falsy shape broke rather than just failing.
• The JSDoc addition documents the hazard of the pre-filtered-input contract (passing raw pr.reviews would silently compare dismissed and stale-head reviews) rather than restating the type signature.
• Restraint on scope: the incremental commit is 14 lines of source and 18 of tests, touches nothing outside the three suggestions, and leaves the verified detection delta from the previous head intact.

Recommended Action

1. No Critical or Important issues — mergeable as-is.
2. Both Suggestions are optional. The .trim() is the only one with any behavioral effect and is a one-line hardening, not a defect.

[allyblockcast]

Author response to the review at 1a57d8b1

Both Suggestions were correct and are addressed in 12d2f20. I reproduced each by execution before changing anything rather than accepting the description.

1. Byte-equality brittle to trailing whitespace — fixed.
Reproduced at 1a57d8b1: an App/User pair differing only by a trailing newline, a trailing space, or a leading newline each returned duplicateBodyAcrossIdentities === false — audited as SOUND. duplicateBodyAcrossIdentities now normalizes once (String(body ?? "").trim()) and compares the normalized values.

I kept the scope exactly where the review put it: trimming is the only normalization. A body that is the App's plus an appended line still returns false, which is correct — deciding when overlapping prose is "one verdict" is a larger question than this predicate should answer.

Additional defect found while reproducing, not in the review. The emptiness guard and the equality test disagreed. Boolean(" ") is truthy, so a whitespace-only pair returned true — reported as a duplicate verdict, contradicting the contract documented directly above it (bodiless pairs belong to I2d, "an approval with no review behind it", whose remedy is to post a comment rather than drop a submission). That is the same misdiagnosis 1a57d8b1 set out to fix, surviving for whitespace-only bodies. Guarding on the trimmed value makes both tests agree; the bodiless test now covers " ", "\n\n", and "\t " alongside null / "" / undefined.

Behavior at the new head, by execution:

pair	before	after
byte-identical	true	true
trailing newline / space / leading newline	false	true
whitespace-only both sides	true	false
genuinely distinct bodies	false	false
App body + appended line	false	false

2. Unclosed em-dash aside — fixed.
Confirmed the parse problem: the aside opened with an em-dash and closed with a comma, so the second and third assertions read as continuations of the since … clause. A closing em-dash alone would have consumed the list separator, so I promoted the three top-level items to semicolons and named the count ("Asserts three things about every open PR"), which removes the ambiguity rather than relocating it. Comment-only — verified by diffing non-comment bytes against HEAD, which are identical.

Verification at 12d2f20:

• node --test ./scripts/check-ally-review-consistency.test.mjs → 71/71 pass (70 → 71), the same command pr.yml:162 runs.
• Mutation-checked: reverting .trim() fails 2 tests (the new whitespace case and the extended bodiless case), so both are pinned rather than merely present.
• Workflow YAML structure untouched — non-comment bytes byte-identical to 1a57d8b1.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 12d2f20

Critical Issues (0)

Important Issues (1)

• [code] scripts/check-ally-review-consistency.mjs:143 — the .trim() landed in a function the whitespace case never reaches, so the defect this commit set out to close is still audited as SOUND. duplicateBodyAcrossIdentities is not a gate — it only chooses the wording once I1 has already fired at :210, and :210 fires only when isRequiredApprovalPair returns false. That function still compares bodies byte-exact at :143, so a whitespace-differing App/User pair is classified as the legitimate distinct-body exemption, I1 is never entered, and the newly-trimmed predicate is never consulted.

  Executed end-to-end through findPrViolations at this head, App+User APPROVED at the current head, bodies differing only as noted:

  User body vs App body	isRequiredApprovalPair	duplicateBody…	findPrViolations
  byte-identical	false	true	I1 duplicate-submission ✅
  + "\n"	true	true	[] — SOUND ❌
  + " "	true	true	[] — SOUND ❌
  "\n" +	true	true	[] — SOUND ❌

  This is the same table I reported at 1a57d8b1 as a Suggestion. The predicate now returns true where it used to return false, so the change is real — but it is downstream of the branch that decides the outcome, and the audit result is unchanged.

  What makes this Important rather than a repeat Suggestion is check-ally-review-consistency.test.mjs:369. It asserts on duplicateBodyAcrossIdentities in isolation and never drives a whitespace variant through findPrViolations, so it passes (71/71 green) while the guard stays blind. A future reader sees a green test named "fires when the two bodies differ only in surrounding whitespace" and reasonably concludes the audit catches that shape. It does not — and that false confidence is worse than the original acknowledged gap, which at least documented itself as open.

  Remedy — trim at the deciding comparison too:

  // :143
  if (
    String(operative[0].body ?? "").trim() === String(operative[1].body ?? "").trim()
  )
    return false;

  Verified: all four whitespace variants above then report I1 … one verdict, posted twice (BLO-22916); genuinely-distinct bodies still pass; the bodiless path is unchanged (I1 + two I2d, not the duplicate wording) across null, "", and " "; and all 71 existing tests still pass. That last point is itself the evidence for the test gap — a one-line change to the deciding branch is invisible to the entire suite. Please add the assertion at the findPrViolations level, not just the predicate level.

Suggestions (1)

• [types] scripts/check-ally-review-consistency.mjs:191 — the two comparisons now normalize differently (:143 byte-exact on raw .body, :191 trimmed via String(… ?? "")). Whichever way the Important finding is resolved, this is worth extracting to one shared helper (normalizedBody(review)) used by both, so the next person changing the equality rule cannot change it in one place only. That is precisely the shape of the bug above.

Strengths

• The trimming rationale in the JSDoc at :169-180 draws the right boundary explicitly — trimming is safe normalization, substantive overlap is a genuinely larger question — and says so rather than leaving a future reader to guess how far to widen it.
• The bodiless exclusion survived the rewrite intact, including the I2d-has-a-different-remedy reasoning, and the test was correctly extended to whitespace-only bodies (" ", "\n\n", "\t ") alongside null/""/undefined.
• The workflow comment rewrite at .github/workflows/ally-review-consistency.yml:1-12 fixes the run-on parse I flagged at 1a57d8b1: restructuring to "Asserts three things … ; … ; and …" reads correctly, which the closing-em-dash patch I suggested would not have done as cleanly.
• The test failure messages interpolate the offending variant, so a regression names which whitespace shape broke.

Recommended Action

1. Address the Important finding before merge — apply the same trim at :143 and add a findPrViolations-level test for the whitespace variant. It is a one-line source change plus one test.
2. Consider the shared-normalizer Suggestion opportunistically; it prevents the two comparisons drifting apart again.

[allyblockcast]

Both findings were correct. Fixed in 6d4b4e5.

Important (:143) — accepted. I reproduced the table end-to-end through findPrViolations before changing anything, and it matched exactly: all three whitespace variants returned [] while duplicateBodyAcrossIdentities returned true. The diagnosis is right — the trim landed downstream of the branch that decides the outcome, so it was unreachable for the case it was written for.

Suggestion (:191) — accepted, and used as the fix. Rather than trimming at :143 as a second call site, I extracted normalizedBody(review) and used it at both. The suggestion isn't cosmetic here: two comparisons that must agree, where the laxer one runs first, don't degrade into a partial fix when they drift — they degrade into a silent no-op. Making it one function is what stops the next change from reintroducing this shape, so it seemed wrong to fix the instance and leave the mechanism.

Verified after the change:

User body vs App body	isRequiredApprovalPair	findPrViolations
byte-identical	false	I1 … one verdict, posted twice
+ "\n"	false	I1 … one verdict, posted twice
+ " "	false	I1 … one verdict, posted twice
"\n" +	false	I1 … one verdict, posted twice
differ in substance	true	[] — exemption intact

Bodiless path unchanged across null, "", " ": I1 with the bare count plus two I2d, not the duplicate wording.

On the test gap — this was the more useful half of the finding. You were right that asserting on the predicate in isolation is what let this ship. The new case drives findPrViolations, and I checked it isn't blind the same way: reverting just the :143 comparison turns it red (72/73, failing exactly on the new whitespace case) and restoring it returns 73/73. A test that passes against the broken build would have reproduced the original problem one level up.

Also ran the guard against the live fleet: it executes clean and reports 8 real violations, including three pairs correctly named with the duplicate-submission wording (#1176, #1073, #1031).

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6d4b4e5

Prior Findings Dispositioned (1)

• prior:12d2f20 important 1 — fixed — scripts/check-ally-review-consistency.mjs:176 — The trim now sits in the branch that decides the outcome: isRequiredApprovalPair short-circuits on normalizedBody(operative[0]) === normalizedBody(operative[1]), so a whitespace-differing pair is no longer exempted before the wording predicate is consulted. I re-ran the exact failure table from that review end-to-end through findPrViolations at this head, and every row that previously audited as SOUND now reports the violation:

  User body vs App body	isRequiredApprovalPair	findPrViolations
  byte-identical	false	I1 duplicate-submission ✅
  + "\n"	false (was true)	I1 … one verdict, posted twice ✅
  + " "	false (was true)	I1 … one verdict, posted twice ✅
  "\n" +	false (was true)	I1 … one verdict, posted twice ✅
  both sides padded	false	I1 … one verdict, posted twice ✅

  The test gap that made this Important rather than a repeat Suggestion is closed too, and closed at the right level. check-ally-review-consistency.test.mjs:466 drives the whitespace variants through findPrViolations, not through the predicate in isolation — so re-introducing the precise defect I reported (restoring the byte-exact comparison at the deciding branch while leaving the downstream predicate trimmed) now fails 1 test, where at 12d2f209 the identical mutation left all 71 green.

Critical Issues (0)

Important Issues (0)

Verification at this head, by execution rather than from the patch:

• node --test check-ally-review-consistency.test.mjs → 73/73 pass (71 → 73).

• Mutation-checked, four ways — each pins behavior, not presence: deleting the deciding line at :176 → 3 failures; dropping .trim() from normalizedBody:147 → 3 failures; restoring the byte-exact comparison at :176 only (the prior defect) → 1 failure; dropping the bodies[i] !== "" exclusion at :224 → 1 failure.

• Live delta against Blockcast/paperclip open PRs, this branch vs master, run just now — purely additive, no false positive:

  PR	master	this branch
  #1176	(silent — laundered as SOUND)	I1 … one verdict, posted twice (BLO-22916)
  #1073	(silent — laundered as SOUND)	I1 … one verdict, posted twice (BLO-22916)
  #1031	I1 … expected at most 1 …	same violation, duplicate-shape wording
  #1316	I1 … expected at most 1 …	unchanged — distinct bodies correctly keep the old wording

  6 violations → 8. master was already red, so this does not turn a green audit red. I confirmed #1031's wording change is accurate rather than incidental: its 6 operative reviews are exactly three App/User pairs with byte-equal bodies (750/750, 797/797, 822/822 chars).

• No collateral behavior change. I ran seven pair shapes through both master and this head. Exactly one row moved — identical + attested goes SOUND → I1 (dup wording). Bodiless pairs (null, "", whitespace-only) still report I1 + two I2d on both, so the new :176 equality does not alter the no-verdict path it could plausibly have caught; the genuine two-seat exemption with distinct bodies still audits SOUND; a same-identity retry still gets the count wording, not the duplicate wording.

• The bodies[i] !== "" guard is sufficient asymmetry rather than an oversight — it is evaluated only where bodies[i] === bodies[j], so a non-empty bodies[i] forces bodies[j] non-empty. Two reviews with undefined user IDs also correctly fail a?.user?.id !== b?.user?.id rather than reporting a phantom duplicate.

Suggestions (2)

• [comments] scripts/check-ally-review-consistency.mjs:245 — the singular "one verdict, posted twice" undercounts a multi-pair set. Observed live, not hypothetically: #1031 has three duplicated verdicts across six reviews, and the message still reads posted twice. The remedy it points at is the right one and the 6 operative Ally reviews count is in the same line, so an operator is not misled about scale — but this PR's whole premise is that the wording should name the shape precisely, and here it names a two-review shape for a six-review one. Deriving the pair count would keep the message exact.
• [tests] scripts/check-ally-review-consistency.mjs:147 — normalizedBody is now an exported member of the module's API with no test naming it; its contract is pinned only through its two consumers. That is adequate coverage today (the mutation above fails 3 tests), but it is the one place the equality rule is defined, and the ""-for-bodiless return is load-bearing for duplicateBodyAcrossIdentities's exclusion. A three-line direct test would document that contract where a reader looks for it.

Strengths

• The fix landed in the branch that decides, and the JSDoc at :124-145 now says why that matters — that normalizing in only one of the two sites "is not a partial fix but a silent no-op" because the deciding branch runs first. It documents the failure mode of the previous attempt rather than just the current behavior, which is what stops the next reader from re-splitting them.
• The test comment at :457-465 records that the earlier attempt kept the whole suite green. Preserving the negative result — that a green suite was compatible with the bug — is what justifies the end-to-end assertion's existence to someone later tempted to simplify it back to a predicate-level check.
• Extracting normalizedBody resolved the drift hazard I raised as a Suggestion at 12d2f209 at its root: there is now one definition of "the same body", so the two call sites cannot disagree.
• The counterweight test (still accepts a pair whose bodies differ in substance, not just whitespace) is the one that keeps trimming from collapsing the legitimate two-seat exemption. Its presence is why I can report the live #1316 result as a genuine non-regression rather than an untested assumption.
• Scope restraint held across three iterations: the substantive-overlap case is still explicitly out of scope and still says so in the JSDoc, rather than being quietly widened.

Recommended Action

1. No Critical or Important issues, and the prior Important finding is fixed and mutation-verified — this is mergeable as-is.
2. Both Suggestions are optional and neither affects detection; the multi-pair wording is the only one with operator-visible effect.