fix(ally): make the same-SHA idempotency check actually fire

[kkroo]

Thinking Path

one-verdict-per-head (ally-review-consistency.yml, hourly) has failed 98 of its last 100 runs, continuously since 2026-08-28. It is not a required check — master requires only verify — so it has been red without blocking anything.

The first question was whether the guard is over-strict or the data is real. It is real. On #1525 both flagged reviews carry commit_id=05325ee7, which is still the current head, submitted three days apart — so this is not force-push re-anchoring, and the guard's operative filter (Ally identity, not dismissed, commit_id == head) is counting exactly what it should.

The four violations then turned out to have four different shapes, which ruled out a single-cause story: #1304 is the same body twice (identical md5) one minute apart; #1360 is two full canonical reviews 2h39m apart; #1525 is a canonical review plus a 176-char status note filed as a review; #1316 is a ## CTO review posted under the Ally App identity.

The violation set is also churning, not frozen — #1114/#1530 dropped out and #1304/#1525 joined since 2026-08-28 — so the producer is still emitting duplicates and cleanup alone would be a treadmill.

What Changed

.planning/ally-agent/AGENTS.md Step 2 only. Four defects in one block:

1. Wrong login. The filter matched ally-paperclip[bot], which has never posted a review here. All 16 Ally reviews across the four PRs are allyblockcast[bot] — the login the guard itself declares canonical. LAST_REVIEW_SHA was therefore always empty and the skip never fired.
2. | last answered "what did I review most recently" instead of "have I reviewed THIS head". Now counts operative reviews at HEAD_SHA, excluding DISMISSED to match the guard.
3. Wake on github_pr_review always re-reviews regardless of SHA explicitly defeated the check. I1 permits at most one operative App review per head, so a same-head re-review violates by construction whatever the trigger.
4. gh pr review --comment for the skip note files a PullRequestReview, recreating the violation. Pinned to gh pr comment.

Verification

Run against live GitHub data, old filter vs new:

PR	old filter	skip fires	new filter	skip fires
#1304	''	NO	2 at head	YES
#1525	''	NO	2 at head	YES
#1360	''	NO	2 at head	YES

So the corrected check would have prevented all three duplicates.

Discriminating controls on #1304: real head → 2; bogus SHA deadbeef… → 0; a non-head commit (master) → 0. My first attempt at a negative control used #1597, which also returns 2 — it has two Ally reviews at head as well and is merged, so the guard skips it. That was not a control and was replaced.

Tests

scripts/ally-agent-idempotency-contract.test.mjs imports ALLY_APP_REVIEWER_LOGIN / ALLY_USER_REVIEWER_LOGIN from the guard and pins the instruction against them, so this identifier drift fails a test instead of silently disabling the check. Also asserts no wake-reason exemption and that the skip note goes through gh pr comment.

Every assertion carries a positive control, so a mis-scoped or empty extraction cannot pass vacuously. Verified by mutation — each defect reintroduced on its own fails exactly one test; the fixed file passes 3/3.

Worth noting the test caught a defect in my own first draft: the prose quoted the anti-patterns verbatim, so an absence check over the whole section fired on the documentation. The command assertion is now scoped to the bash fence.

Risks

Instruction-file change, so no unit test covers it; verification is empirical against live data.

Two behaviour changes worth naming. A github_pr_review wake on an unchanged head now posts a plain comment and exits instead of re-reviewing — intended, since the second review was the violation, but it does mean a re-review genuinely wanted at the same head now requires a new commit. And the skip note becomes an issue comment rather than a review object, so anything keying off a review record for that note will no longer see one.

This does not clear the four existing violations. Those heads are frozen and COMMENTED reviews cannot be dismissed through the API, so they persist until each head moves or the PR closes. Expect the guard to stay red until then; this stops the set from growing.

Model Used

claude-opus-5[1m]

---

Refs BLO-19778 (cited in the guard's own violation message).

• Searched the GitHub PR list for similar or duplicate PRs — none touch .planning/ally-agent/AGENTS.md or this idempotency check.

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

• 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".
• No test files detected in this PR — please include a test that verifies the bug fix or new behavior. If this PR genuinely doesn't need a test (e.g. a refactor), please retitle with refactor: prefix.

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: 11d6bb3

Critical Issues (1)

• [native-codex] .planning/ally-agent/AGENTS.md:24-27 — The idempotency check treats any allyblockcast User-seat review as equivalent to the required Ally App review, without checking user.type, immutable user ID, or the canonical consolidated body/attestation. A same-head User approval can therefore make ALREADY > 0 and cause Ally to skip the App's required formal review, leaving review/ally-complete unsatisfied (and on an App-authored PR, skipping the required formal App COMMENTED review entirely).
  • Restrict this check to the trusted App identity (user.type == Bot, canonical App login and ID) and require the consolidated header plus exactly one matching Reviewed head: attestation. The User seat must not suppress the App lane.

Important Issues (1)

• [native-codex] .planning/ally-agent/AGENTS.md:24-28 — With --paginate, gh api --jq '[...] | length' evaluates once per response page and emits multiple newline-separated counts. Once the reviews endpoint has more than one page, ALREADY can be 1\n0, and [ "${ALREADY:-0}" -gt 0 ] reports an integer-expression error and falls through to another review instead of skipping.
  • Aggregate pagination before counting, or use a jq expression that emits a single scalar after all pages are collected, then validate the result before the numeric comparison.

Suggestions (0)

Strengths

• The change correctly replaces the last-review lookup with a current-head query and excludes dismissed reviews.
• The skip note correctly uses gh pr comment rather than creating another formal review object.

Recommended Action

1. Fix the App/User identity distinction and pagination aggregation before merge.
2. Add focused shell-level coverage for a User-only same-head review and multi-page review responses.

[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: bdf2d16

Prior Findings Dispositioned (2)

• prior:11d6bb3 critical 1 — still-present — .planning/ally-agent/AGENTS.md:89-92 — The filter still matches by login alone, including the allyblockcast User seat, without requiring Bot type, immutable App ID, canonical consolidated body, or exact attestation. A User review can still suppress the required App review.
• prior:11d6bb3 important 1 — still-present — .planning/ally-agent/AGENTS.md:89-93 — The paginated gh api --paginate --jq "[...] | length" command still emits one count per response page; a multi-page response can make ALREADY newline-separated and cause the numeric comparison to fail rather than skip.

Critical Issues (1)

• [native-codex] .planning/ally-agent/AGENTS.md:89-92 — The idempotency check treats any matching allyblockcast login as sufficient, including the User seat. A same-head User review can make ALREADY > 0, causing Ally to skip the required trusted App review and leaving the review gate unsatisfied.
  • Restrict the predicate to the trusted App identity (user.type == Bot, canonical login and immutable App ID), and require the canonical consolidated header plus exactly one matching Reviewed head: attestation. The User seat must not suppress the App lane.

Important Issues (1)

• [native-codex] .planning/ally-agent/AGENTS.md:89-93 — With --paginate, the --jq filter is evaluated independently for each response page. More than one page can produce output such as 1\n0; [ "${ALREADY:-0}" -gt 0 ] then raises an integer-expression error and falls through to another review instead of reliably skipping.
  • Aggregate all pages before counting, or use a pagination-safe command that emits one validated scalar.

Suggestions (0)

Strengths

• The check now evaluates reviews at the current head instead of using only the last review.
• Dismissed reviews are excluded from the operative count.
• The skip note correctly uses gh pr comment rather than creating another formal review object.

Recommended Action

1. Fix the App/User identity distinction before merge.
2. Make the paginated count produce one scalar and add focused tests for User-only reviews and multi-page responses.