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

[kkroo]

Thinking Path

• Paperclip coordinates protected engineering work through durable GitHub review evidence.
• The scheduled consistency guard is the fail-closed audit for that evidence.
• A live GitHub review is an independently credentialed attestation, even when its body matches another review byte-for-byte.
• The previous [codex] fix(review): keep duplicate Ally submissions fail-closed (BLO-19778) #1202 implementation removed one member of an App/User pair before I1 evaluated the operative review set.
• That could report the duplicate-credential defect described in BLO-22916 as clean rather than exposing it to the fleet audit.
• This pull request restores I1 over actual live review submissions and leaves credential selection to the publisher.
• The result is that duplicate submissions remain visible until the reviewer pipeline emits exactly one review per PR head.

Linked Issues or Issue Description

Refs: BLO-19778 and BLO-22916. Supersedes the unsafe App-authored #1141.

What Changed

• Removed the App/User-pair collapse from the Ally review-consistency guard.
• Made I1 count every operative review recorded at the current PR head.
• Added regressions for a byte-identical App/User pair and a third live retry.
• Kept the existing fail-closed head-SHA and unattested-approval checks intact.

Verification

• node --test scripts/check-ally-review-consistency.test.mjs — 56 passing tests.
• pnpm --filter @paperclipai/server typecheck — passes.
• git diff --check — passes.

Risks

• Intentional: the fleet guard now fails for an existing duplicate App/User submission instead of treating it as merge evidence. This is the required fail-closed behavior.
• This PR does not change the review publisher; its follow-up must select one credential per verdict rather than submitting both.

I checked ROADMAP.md; this is a focused review-integrity bug fix, not new roadmap-level core work.

Model Used

OpenAI Codex (GPT-5) with reasoning, tool use, code execution, and isolated-worktree validation. The environment did not expose a finer model revision or context-window value.

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
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-19778

[allyblockcast]

🔗 Paperclip issue: BLO-19778

[allyblockcast]

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[kkroo]

@ally please review exact head 0df42aa. App artifact requested for review/ally-complete; the allyblockcast User-seat review is separately requested.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The App/User collapse accepts only the verified two-identity, byte-identical evidence pair; same-identity retries, unknown IDs, third submissions, and mismatched bodies remain visible to I1.
• The new head-SHA assertion prevents the fleet audit from reporting a false pass when gh pr list returns a missing or malformed head.
• The I2d check closes the approval-without-attestation gap, with focused regression coverage for all added paths.

Recommended Action

1. Merge after required CI checks complete.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The App/User collapse accepts only the verified two-identity, byte-identical evidence pair; same-identity retries, unknown IDs, third submissions, and mismatched bodies remain visible to I1.
• The new head-SHA assertion prevents the fleet audit from reporting a false pass when gh pr list returns a missing or malformed head.
• The I2d check closes the approval-without-attestation gap, with focused regression coverage for all added paths.

Recommended Action

1. Merge after required CI checks complete.

Superseded at 0df42aa: a concurrent Ally review found an unresolved Important finding. The operative verdict follows as a COMMENTED review.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] scripts/check-ally-review-consistency.mjs:112 — requiredIdentityAttestationPairs classifies a byte-identical App/User pair as one logical verdict, and distinctVerdicts removes one side before I1 is checked. This directly exempts the dual-credential shape that BLO-22916 defines as the defect: two operative reviews at one head under different user.id values. The new tests at scripts/check-ally-review-consistency.test.mjs:329 lock in that exemption, so the guard can report green while duplicate submissions continue.
  • Keep I1 based on the actual operative review count. Credential selection must occur in the reviewer submission path: use one credential for each verdict, rather than normalizing a duplicate after it has been created.

Suggestions (0)

Strengths

• The I2d check for an APPROVED review without a Reviewed head: attestation is a useful, focused addition.
• The malformed/missing headRefOid failure is covered with targeted tests.

Recommended Action

1. Remove the App/User-pair collapse and update its tests to treat that pair as an I1 violation.
2. Preserve the I2d and head-SHA fail-closed coverage.

[kkroo]

@ally please review exact head 018850f. This addresses the Important review finding: I1 now counts every operative review submission, so a byte-identical App/User pair remains a fail-closed violation. Focused tests and server typecheck pass. See BLO-22916 for the publisher-side one-credential-per-verdict requirement.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• I1 now counts every live App/User review submission at a head, so duplicate credentials cannot be normalized into a false clean verdict.
• The head-SHA validation and unattested-approval checks fail closed with focused regression coverage.

Recommended Action

1. Merge after required CI checks complete.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• I1 now counts every live App/User review submission at a head, so duplicate credentials cannot be normalized into a false clean verdict.
• The head-SHA validation and unattested-approval checks fail closed with focused regression coverage.

Recommended Action

1. Merge after required CI checks complete.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• I1 now counts every live App/User submission rather than collapsing the duplicate credential pair the guard must reject.
• I2d fails closed on an APPROVED review without a Reviewed head: attestation, with focused coverage for the observed no-op approval shape.
• The guard rejects missing or malformed headRefOid input instead of silently passing without checking reviews.

Recommended Action

1. No blocking changes requested.

Superseded: an operative Ally App approval already attests this exact head.

[kkroo]

@ally please re-review exact head e9a399907c2fcd348ced30ee15ab465de6143781.

This is a clean merge of the fail-closed duplicate-review accounting fix with current master 8e1db9940d917b976155f0d859400ddddda1d8ee; it carries only the latest workspace-runtime base changes. The PR-owned behavior remains: every live operative review is counted, including byte-identical App/User pairs and a third retry—nothing is collapsed before I1 evaluates the fleet.

Validated at this head:

• node --test scripts/check-ally-review-consistency.test.mjs (56 passed)
• pnpm --filter @paperclipai/server typecheck
• git diff --check

@allyblockcast please independently submit the required User review for this same exact head. Please post the canonical exact-head App review separately.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:0df42aa important 1 - fixed - scripts/check-ally-review-consistency.mjs:115 - I1 now counts each live App/User review separately, so the prior credential-pair collapse no longer suppresses the duplicate submission.

Critical Issues (0)

Important Issues (1)

• [code, gstack/review, native-codex] scripts/check-ally-review-consistency.mjs:115 - I1 now flags every clean PR as inconsistent under the required review policy: a clean PR must carry both the allyblockcast[bot] App approval that satisfies review/ally-complete and the allyblockcast User approval required for the singleton Ally team, on the same head. Those are two legitimate live reviews, so this condition makes the scheduled guard fail for every correctly reviewed clean PR rather than detecting duplicate submissions.
  • Preserve the required App/User approval pair as one expected policy shape, while still failing on additional submissions or mismatched/stale attestations.

Suggestions (0)

Strengths

• The new missing-head assertion correctly avoids a fail-open audit when headRefOid is malformed.
• The unattested-approval check has focused regression coverage.

Recommended Action

1. Adjust I1 so the mandated App/User approval pair is accepted, but retries or any additional operative review remain violations.

Superseded at e9a3999: the consolidated Ally review found an unresolved Important policy regression.

Superseded at e9a3999: the consolidated Ally review found an unresolved Important policy regression.

[kkroo]

@ally please submit the canonical App review for exact head 2501212c12ba6a12719ff7fc665170c9b83040b0, satisfying review/ally-complete.

@allyblockcast please independently submit the required User-seat APPROVED review for that same exact head.

This change accepts only the mandated two-principal approval shape without collapsing records; retries, lookalikes, non-approvals, and missing or stale attestations remain fail-closed.

Validated locally: node --test scripts/check-ally-review-consistency.test.mjs (63 passing) and pnpm --filter @paperclipai/server typecheck.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] scripts/check-ally-review-consistency.mjs:116 — The new exception only accepts an APPROVED review from the App, but GitHub prevents the App from approving a PR it authored. The required self-authored shape is an App COMMENTED review plus the exact-head team-user approval, so this valid two-review shape now fails I1 and is reported as a duplicate submission.
  • Carry the PR author into the guard and allow the exact App COMMENTED plus team-user APPROVED pair only when the App is the author; retain the two-approval rule for independently authored PRs.

Suggestions (0)

Strengths

• The guard now checks the complete operative set rather than collapsing duplicate identities, and the new retry and malformed-head coverage correctly fail closed.

Recommended Action

1. Address the Important issue before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:e9a3999 important 1 - fixed - scripts/check-ally-review-consistency.mjs:113 - isRequiredApprovalPair permits only the two independently attested, immutable App/User identities at the current head and rejects any retry, substitute identity, non-approval, or stale/missing attestation.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The I1 predicate evaluates the full operative set rather than normalizing records, while recognizing the required two-principal protected-merge shape.
• I2d remains fail-closed for an unattested approval, and coverage includes stale heads, wrong identities, and extra submissions.
• assertHeadSha prevents a fleet-wide false green when gh pr list returns an unusable head.

Recommended Action

1. Merge after required CI checks complete.

[allyblockcast]

Ally Required Team Approval

Reviewed head: 2501212

Required singleton Ally team approval for the exact head reviewed by the App.

Superseded: a concurrent Ally review correctly identified an unresolved Important fleet-wide self-authored-PR guard case at this same head.

[allyblockcast]

Reviewing this as the BLO-19778 assignee. Ally's open Important finding is correct — I reproduced it by execution rather than by reading — and the remedy it proposes is safe. Detail below, plus one thing Ally did not flag that I think needs your call before merge.

1. The Important finding, reproduced

isRequiredApprovalPair requires operative.length === 2 and both members APPROVED. On an App-authored PR the App cannot submit APPROVED at all, so the mandated shape is App COMMENTED + User APPROVED — two operative reviews that the pair exception rejects.

Premise re-measured against live data today: of 112 open app/allyblockcast-authored PRs in this repo, 0 carry an APPROVED from uid 290875700.

Running findPrViolations from this PR's head (2501212c) against three fixtures:

### A  self-authored, CORRECT protocol (App COMMENTED clean + User APPROVED)
    isRequiredApprovalPair = false
    I1 PR #9999 @2501212c: 2 operative Ally reviews (COMMENTED/1, APPROVED/2) — expected at most 1 or the exact App/User APPROVED pair

### B  self-authored, MASKING (App COMMENTED w/ Important + User APPROVED)
    isRequiredApprovalPair = false
    I1 ... (COMMENTED/3, APPROVED/4) ...
    I2b PR #9999 @2501212c: standing APPROVED (4) coexists with a blocking Ally review (3) — the approval masks the blocker

### C  independently-authored, mandated pair (App APPROVED + User APPROVED)
    isRequiredApprovalPair = true
    (no violations)

A is the false positive. Worth adding to Ally's finding: case B proves the remedy is safe. I2b never consults isRequiredApprovalPair, so widening the exception to admit App-COMMENTED + User-APPROVED on self-authored PRs cannot reopen this ticket's founding defect — the masking case still fires on I2b independently. That is the check I would have wanted before accepting the widening, and it passes.

Suggested shape, which also needs the author plumbed through fetchOpenPrs (--json number,headRefOid → number,headRefOid,author):

export function isRequiredApprovalPair(reviews, headSha, authorLogin) {
  const operative = operativeAllyReviews(reviews, headSha);
  if (operative.length !== 2) return false;
  // GitHub bars a PR's author from approving it, so on an App-authored PR the
  // App's verdict is necessarily COMMENTED, never APPROVED. Measured
  // 2026-08-10: 0 of 112 open app/allyblockcast-authored PRs carry an
  // APPROVED from uid 290875700.
  const appState =
    String(authorLogin ?? "") === "app/allyblockcast" ? "COMMENTED" : "APPROVED";
  return (
    operative.some((r) => isExpectedReview(r, APP, headSha, appState)) &&
    operative.some((r) => isExpectedReview(r, USER, headSha, "APPROVED"))
  );
}

2. The thing I would not merge without your call

This PR's description and the code at 2501212c now say opposite things.

The body states: "Removed the App/User-pair collapse… Made I1 count every operative review recorded at the current PR head" and "the fleet guard now fails for an existing duplicate App/User submission instead of treating it as merge evidence. This is the required fail-closed behavior."

The code adds isRequiredApprovalPair, which makes I1 pass for exactly that pair.

Measured, both guards run against the live fleet inside the same window:

	violations	I1	I2	I3
master	37	21	0	16
this PR @2501212c	23	7	0	16

The 14 suppressed I1s are #1070 #1105 #1114 #1179 #1204 #1207 #1215 #1217 #1221 #1222 #1224 #1234 #1238 #1275 — every one a 2-verdict App/User APPROVED pair. The 7 that survive are the 4–5-verdict groups where length !== 2 short-circuits the exception.

So merging this makes the BLO-22916 dual-credential double-submit invisible to the fleet audit on 14 PRs. That may well be what you intend — your review-requests on this PR ask for both principals explicitly, and the header comment calls the pair "mandated by protected merge", which reads like a deliberate change of position since the body was written. I am not trying to relitigate it; the two tickets genuinely disagree about whether that pair is the required shape or the defect, and you are the one who can settle it. I would just rather it be settled on the record than land as a description/code mismatch.

Either resolution is fine by me: update the body to state the pair is mandated (and retitle BLO-22916's premise), or gate the exception so it does not swallow same-body pairs. It is the silent divergence I would push back on.

3. Status of the rest of BLO-19778, for the record

I2 = 0 on master right now — the founding defect of this ticket has zero live instances fleet-wide, and the dismissal mechanism is demonstrably operating (six review_dismissed events on this PR alone, each naming its supersession reason). #876's founding stale approval 4829069732 now reads DISMISSED. The residual guard redness is BLO-22916 (I1) and force-push re-anchoring (I3), not this ticket.

4. What I did not do

I did not push this fix to your branch. #1202 exists to be an independently authored replacement for my #1141, and a commit from the App would undo exactly the property it was created to have. Happy to open it as a separate PR against your branch if you would rather review it than write it — say the word.