[codex] fix(review): separate Ally evidence lanes

[kkroo]

Thinking Path

• Paperclip uses protected GitHub reviews as evidence that an exact PR head is safe to merge.
• Ally produces two distinct evidence lanes: its GitHub App review and the separate allyblockcast user-seat approval.
• The consistency guard currently groups both lanes as duplicate operative verdicts solely because they share a name.
• That creates false failures for valid paired evidence and obscures real defects such as stale, duplicate, or comment-only App attestations.
• This pull request separates the lanes while requiring formal exact-head approvals and preserving blocker precedence for the App.
• The benefit is a review gate that recognizes required independent evidence without allowing either lane to mask a real finding.

Linked Issues or Issue Description

What happened

The scheduled Ally review-consistency guard reports valid paired App and user-seat approvals as duplicate evidence. The live run reports 22 false duplicate violations, even where each lane has exactly one clean approval on the same head.

Expected behavior

One formal, canonical exact-head approval is allowed in each lane. Duplicates or stale reviews within either lane, a comment-only clean App review, or a user-seat approval beside an App blocker must remain failures.

Steps to reproduce

1. Create a PR with one allyblockcast[bot] App approval and one allyblockcast user-seat approval at the same head.
2. Run scripts/check-ally-review-consistency.mjs.
3. Before this change, it reports two operative Ally verdicts; after this change, it accepts the pair and still rejects duplicate/stale/blocking evidence.

Paperclip version

Current master; see the failing scheduled guard run.

What Changed

• Classified Ally App and user-seat reviews as distinct exact-head evidence lanes.
• Allowed one canonical formal approval per lane and independently rejected stale or duplicate attestations.
• Required clean App evidence and user-seat evidence to use APPROVED, not merely COMMENTED.
• Prevented a user-seat approval from masking a blocking App review.
• Added focused regression coverage for lane separation, approval state, stale heads, duplicates, and blockers.

Verification

• node --test scripts/check-ally-review-consistency.test.mjs — 47 passed.
• pnpm typecheck passed.
• git diff --check passed.

Risks

• Existing invalid historical evidence will remain visible: 28 legacy clean App COMMENTED reviews plus stale/duplicate records require valid replacement evidence rather than suppression.
• This guard change does not alter the separate server-side reviewer-evidence enforcement path; that alignment is deliberately follow-up work.

Model Used

• OpenAI Codex, GPT-5, agentic coding workflow with terminal and GitHub tooling.

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 does not duplicate planned core work.
• I have searched GitHub for duplicate or related PRs and linked the related guard run above.
• I have described the underlying bug in-PR using the bug-template fields.
• I have run focused tests locally and they pass.
• I have added regression tests for the changed behavior.
• UI screenshots are not applicable.
• No user-facing documentation change is needed for this CI/review-gate repair.
• I have considered and documented risks above.
• All Paperclip CI gates are green on the exact head.
• Greptile and reviewer follow-ups remain pending.

[allyblockcast]

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

Missing or incomplete:

• 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

[allyblockcast]

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

Missing or incomplete:

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

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] scripts/check-ally-review-consistency.mjs:225 — A clean App review in the self-review case is necessarily COMMENTED: GitHub prohibits the App PR author from approving its own PR. This condition treats every clean COMMENTED App review as an I4 violation, so the required formal self-review for App-authored PRs can never pass the consistency guard.
  • Preserve COMMENTED as valid clean App evidence when the PR author is app/allyblockcast; keep APPROVED mandatory for non-self App reviews and the separate user-seat lane.

Suggestions (0)

Strengths

• The lane separation and exact-head checks are clearly structured and regression-tested in the changed test matrix.

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: 14fdcfd

Prior Findings Dispositioned (1)

• prior:f6dca1d important 1 — fixed — scripts/check-ally-review-consistency.mjs:157 — clean App self-reviews are now explicitly allowed to remain COMMENTED when the PR author is app/allyblockcast.

Critical Issues (0)

Important Issues (1)

• [native-codex] scripts/check-ally-review-consistency.mjs:146 — Review lanes are inferred solely from user.login, so a review is counted as App or User-seat evidence without confirming GitHub's user.type. The reviewer-identity policy requires Bot-type allyblockcast[bot] for the App lane and User-type allyblockcast for the team lane; enforce those types when classifying reviews and add opposite-type regression cases.
  • Classify the complete review.user identity, not only its login, before using it for idempotency or lane invariants.

Suggestions (0)

Strengths

• The two-lane invariants and the App-authored self-review exception are clearly separated and covered by focused unit tests.

Recommended Action

1. Address the Important issue before merge.
2. Consider Suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:14fdcfd important 1 — fixed — scripts/check-ally-review-consistency.mjs:135 — the App lane now requires the Bot-type allyblockcast[bot] REST identity, and scripts/check-ally-review-consistency.mjs:141 independently requires the User-type allyblockcast identity for the user-seat lane.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The implementation keeps App and user-seat evidence separate while preserving same-head duplicate and blocker checks.
• The focused tests cover opposite account types, independent lane duplicates, and the App-authored self-review exception.

Recommended Action

1. Merge when the remaining required checks are green.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:14fdcfd important 1 — fixed — scripts/check-ally-review-consistency.mjs:135 — the App lane now requires the Bot-type allyblockcast[bot] REST identity, and scripts/check-ally-review-consistency.mjs:141 independently requires the User-type allyblockcast identity for the user-seat lane.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The implementation keeps App and user-seat evidence separate while preserving same-head duplicate and blocker checks.
• The focused tests cover opposite account types, independent lane duplicates, and the App-authored self-review exception.

Recommended Action

1. Merge when the remaining required checks are green.

[allyblockcast]

Ally — Consolidated PR Review

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

The lane separation is sound and I verified it empirically: I ran both this head's guard and master's against all 144 open PRs. On #1269 master conflated the legitimate App+User approval pair into a bogus I1 duplicate; this revision correctly reports only the real I3 stale attestation. That is a genuine reduction in false positives, not just a refactor. All 73 unit tests pass on Node 24.

Critical Issues (0)

Important Issues (1)

• [native-codex] scripts/check-ally-review-consistency.mjs:225 — isRequiredApprovalPair requires the App review to be APPROVED, but this file's own I4 doc (lines 31–35) states that on an App-authored PR the App's clean self-review is necessarily COMMENTED because GitHub bars self-approval. The predicate is therefore unsatisfiable for exactly the case the rest of this PR exists to accommodate — and that case is the dominant one: 106 of the 144 currently open PRs are authored by app/allyblockcast. This arrived via the master merge in dad6004d and is currently latent (the export has no consumer outside the test file; the shipped guard path in findPrViolations handles the exception correctly at line 290), so nothing is broken today. But the name and the docstring — "The only permitted two-review shape" — actively invite wiring it into a merge gate, at which point every agent PR would block.
  • Either give it the same App-authored exception isCleanAppSelfReview encodes, or narrow the docstring to state explicitly that it describes the independently-authored-PR shape only and must not be applied to App-authored PRs. A regression test pinning the App-authored case would keep the two paths from drifting again.

Suggestions (2)

• [pr-review-toolkit] deploy/helm/paperclip/tests/prometheus-rule.test.mjs:42 — whitespace-only blank-line removals in a file unrelated to review-evidence lanes. Merge noise; dropping it keeps the diff to its stated subject.
• [gstack/review] scripts/check-ally-review-consistency.mjs:314 — the I2 docstring promises "no operative APPROVED review coexisting with a blocking review in the other lane" (bidirectional), while I2b implements only seat-APPROVED-vs-App-blocking. The reverse direction is in fact covered — a blocking seat review trips I4, and an APPROVED seat body with findings trips I2a — so this is not a fail-open, but the comment overstates what this specific check does.

Strengths

• Lane separation is validated against real data, not just fixtures: the #1269 improvement above is measurable on the live fleet.
• The fail-closed asserts (assertPrListComplete, assertHeadSha) are the right instinct for a guard, and the docstring on assertHeadSha explains why a falsy head is fatal rather than merely asserting it.
• Test coverage is genuinely adversarial — opposite account types, lookalike logins carrying the correct immutable ID, canonical logins with wrong IDs, and per-lane duplicates are all pinned.
• The I3 docstring revision correcting the force-push mechanism (vs. the earlier submit-time-binding claim) is an honest, well-evidenced correction.

Recommended Action

1. Address the Important issue this cycle — clarify or fix isRequiredApprovalPair before anything wires it to a gate.
2. Consider the Suggestions opportunistically.
3. Unrelated to this diff: Build, Canary Dry Run, General tests (server 2/4) and (workspaces-b) are all failing at the same pnpm install step on a missing packages/plugins/sdk/dist/dev-cli.js, ~2m into four unrelated job types. Not attributable to these three files, but it needs to go green before merge.
4. Note the scheduled Ally Review Consistency Guard is already red on master (11 violations) and remains red here (10). It is cron-only, not a PR gate, so it does not block this PR — but #1031 and #1316 need dispositioning separately.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:dad6004 important 1 — fixed — scripts/check-ally-review-consistency.mjs:230 — the App lane now recognizes a clean self-review only for an App-authored PR, while the normal App approval path remains required for independently authored PRs.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• App and User-seat evidence are classified into independent lanes with GitHub account-type checks.
• Exact-head, duplicate, stale-attestation, blocker-precedence, and self-review cases are covered by focused regression tests.
• The review preserves formal approval requirements for independently authored PRs while supporting the required App-authored self-review form.

Recommended Action

1. Merge when the remaining required checks are green.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:dad6004 important 1 — fixed — scripts/check-ally-review-consistency.mjs:230 — the App lane now recognizes a clean self-review only for an App-authored PR, while the normal App approval path remains required for independently authored PRs.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• App and User-seat evidence are classified into independent lanes with GitHub account-type checks.
• Exact-head, duplicate, stale-attestation, blocker-precedence, and self-review cases are covered by focused regression tests.
• The review preserves formal approval requirements for independently authored PRs while supporting the required App-authored self-review form.

Recommended Action

1. Merge when the remaining required checks are green.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] scripts/check-ally-review-consistency.mjs:174 — The new lane classifier accepts only login plus account type, and findPrViolations uses that classifier directly. It never checks ALLY_APP_REVIEWER_ID or ALLY_USER_REVIEWER_ID; isRequiredApprovalPair still checks them but is now unused by the runtime guard. Consequently, a review from another Bot/User account with the canonical login shape but a different REST id is treated as valid App/User-seat evidence, so the audit can report no violation despite its stated immutable-identity invariant.
  • Require the expected REST id when assigning an App or User-seat lane (or emit a violation for a mismatched id), and add runtime findPrViolations coverage for both mismatched-id lanes.

Suggestions (0)

Strengths

• The App/User-seat split and account-type regressions make the intended evidence model substantially clearer.
• The focused suite passes all 83 tests at this head.

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: 6a67c31

Prior Findings Dispositioned (1)

• prior:3d68761 important 1 — still-present — scripts/check-ally-review-consistency.mjs:182 — operativeAllyReviews classifies a review using allyReviewLane, which checks only login and account type; findPrViolations consumes that result at line 293 without enforcing ALLY_APP_REVIEWER_ID or ALLY_USER_REVIEWER_ID. A canonical-login review with an unexpected immutable REST ID can therefore satisfy the runtime lane checks.

Critical Issues (0)

Important Issues (1)

• [prior:native-codex] scripts/check-ally-review-consistency.mjs:182 — The runtime guard still accepts canonical Ally login/account-type pairs without checking the expected immutable REST IDs. A Bot/User lookalike with the canonical login shape but a different REST ID is treated as valid lane evidence, so the audit can report no violation for a review from the wrong principal.
  • Require the expected REST ID when assigning the App or User-seat lane, and add findPrViolations regression cases for mismatched IDs in both lanes.

Suggestions (0)

Strengths

• The App and User-seat lanes are separated cleanly and account type is now considered during classification.
• Exact-head filtering, canonical App attestations, duplicate-body detection, blocker precedence, and App-authored self-review handling are covered by focused tests.

Recommended Action

1. Address the Important issue this cycle.
2. Consider Suggestions opportunistically.

[kkroo]

@ally please review exact head aaca48e

Follow-up fixes review #4999241869: runtime lane classification now pins both canonical GitHub login/account type and immutable REST ID, with wrong-App-ID and wrong-User-seat-ID coverage. Focused test: node --test scripts/check-ally-review-consistency.test.mjs (86/86).

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:3d68761 important 1 — fixed — scripts/check-ally-review-consistency.mjs:151 — allyReviewIdentityShape requires the canonical login and GitHub account type before assigning a lane, while isAllyAppReviewer and isAllySeatReviewer require the corresponding immutable REST ID. The runtime guard also emits an explicit mismatch violation at scripts/check-ally-review-consistency.mjs:314.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The App and User-seat lanes now require canonical login, account type, and immutable REST identity before runtime evidence is accepted.
• The focused regression suite covers wrong account types, wrong App/User-seat IDs, duplicate lane submissions, stale attestations, blocker precedence, and App-authored self-review behavior.
• Exact-head App attestation remains distinct from the User-seat lane's formal exact-head approval semantics.

Recommended Action

1. Merge when the remaining required checks are green.

[kkroo]

@ally please satisfy the PR's two-lane consistency guard at exact head aaca48ed059f5f35dede5a7de191f8e684a6a689.

This PR is independently authored (kkroo), so the existing exact-head App approval is not the complete evidence set: please run the dedicated reviewer pipeline's separate allyblockcast User-seat lane and leave its formal exact-head approval if the head is clean. GitHub's aggregate reviewDecision=APPROVED is not a reason to skip the custom lane; the guard and review/ally-complete require distinct App and User evidence. Do not change the head or author a replacement PR.