fix(ally): name the real identity in the self-review guard

[kkroo]

Thinking Path

#1599 fixed a dead reviewer identity in Ally's Step 2 idempotency check, and scoped its contract assertion to that block. That scoping was right for the command assertions — the surrounding prose has to stay free to name an anti-pattern in order to explain it — but it meant the login assertion only covered one of the two places the identity appeared.

ally-paperclip[bot] also sat in the "don't review your own work" guard at line 185. Ally authors as allyblockcast[bot], so that guard matched nothing and failed open exactly like the idempotency check did: one dead identifier, two disabled guards. pim#2251 is the visible consequence — authored by allyblockcast[bot] and reviewed by allyblockcast[bot].

I found it verifying #1599 by content on master rather than trusting the merge, which is the only reason it surfaced at all.

What Changed

Line 185 now names allyblockcast[bot] and allyblockcast, with a pointer to the exported constants so the next reader knows where the source of truth lives.

The contract test gains a file-wide assertion that the stale identity appears nowhere, plus one pinning the self-review guard to the same constants. Command assertions stay fence-scoped.

Verification

Mutation-tested, not just run green: reverting line 185 on its own fails tests 3 and 4; the fixed file passes 5/5. Both new assertions carry positive controls, so a failed or empty read cannot satisfy the file-wide absence check.

Risks

Instruction-file change; no runtime code path.

The behavioural consequence is that Ally will now actually skip App-authored PRs, which it has not been doing. If anything downstream has come to depend on those self-reviews existing, it will stop seeing them.

Deliberately not resolved here: isCleanAppSelfReview in check-ally-review-consistency.mjs tolerates a non-blocking COMMENTED self-review, because GitHub forbids the App approving its own PR. That sits in tension with this guard's blanket skip. Correcting a dead identifier is not the place to decide which behaviour is intended, so I have flagged it rather than quietly picking a side — a reviewer who owns that gate should say which one wins.

Model Used

claude-opus-5[1m]

---

Related work found while searching

• fix(ci): ratchet the Ally review-consistency guard so it can signal again (PEN-2847) #1585 — fix(ci): ratchet the Ally review-consistency guard so it can signal again (PEN-2847). Addresses the same standing red from the guard side (check-ally-review-consistency.mjs, workflow, baseline). No file overlap with this PR, which changes AGENTS.md — the producer, not the detector. The two are complementary: fix(ci): ratchet the Ally review-consistency guard so it can signal again (PEN-2847) #1585 makes the guard signal usefully again; this stops the producer emitting the duplicates.
• fix(ally-guard): ignore superseded review status updates #1559 — fix(ally-guard): ignore superseded review status updates. Also guard-side only.

Merge-order risk worth flagging: the contract test added here imports ALLY_APP_REVIEWER_LOGIN / ALLY_USER_REVIEWER_LOGIN from check-ally-review-consistency.mjs, which #1585 edits. If #1585 renames or removes those exports, this test fails on master. That coupling is deliberate — pinning prose to the code's own constant is the point — but whichever lands second should re-run scripts/ally-agent-idempotency-contract.test.mjs.

• Searched the GitHub PR list for similar or duplicate PRs — fix(ci): ratchet the Ally review-consistency guard so it can signal again (PEN-2847) #1585 and fix(ally-guard): ignore superseded review status updates #1559 are adjacent but guard-side; none change the self-review guard in AGENTS.md.

[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]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] .planning/ally-agent/AGENTS.md:185 — the new guard skips PRs authored by allyblockcast (the shared User seat), but the authoritative reviewer policy says that seat must never submit a review or approval; App-authored PRs must instead receive a formal COMMENTED review from allyblockcast[bot]. This instruction will therefore suppress the required artifact whenever the User seat authors a PR, leaving the review gate without the mandated App review.
  • Keep the self-review skip scoped to the App author identity (app/allyblockcast / allyblockcast[bot]) and explicitly route App-authored PRs to the formal App COMMENT path. Do not treat the User seat as an equivalent author/self-review case.

Suggestions (0)

Strengths

• The stale ally-paperclip[bot] identifier is removed from the executable instruction.
• The new contract tests provide positive controls and verify the exact canonical App and User constants.
• The targeted contract test passes with 5 tests.

Recommended Action

1. Fix the Important issue before merge.
2. Re-run the Ally contract test and verify App-authored PRs still receive formal App comments.