docs(ally): reconcile the reviewer doc with the live managed bundle

[kkroo]

Thinking Path

I was asked to check why the Ally review gate was red on two pim PRs, expected to find my own recent self-review change misfiring, and instead found the reds were unrelated — Ally's reviewer runs had exhausted their automatic retries. Verifying that led somewhere more useful: .planning/ally-agent/AGENTS.md is not what Ally runs, so three edits I merged today changed documentation while I reported them as a behavioural fix. Reading the live managed bundle showed the doc now contradicts operating policy on three points, one of which the bundle explicitly records as false and already costly, so this corrects the doc to match and pins the corrections with tests.

What Changed

The core fact this PR records: Ally's live instructions are a managed instructions bundle on the paperclip volume (/paperclip/instances/.../agents/<id>/instructions/AGENTS.md), read and written through GET|PUT /api/agents/:id/instructions-bundle/file. Nothing in this repository syncs .planning/ally-agent/AGENTS.md into it — the only reference to that path anywhere in master is its own test. The documents have diverged: this doc is ~220 lines, last substantively edited 2026-05-16; the live bundle is ~1,670 lines carrying dated policy through 2026-08-30.

Three corrections, each taken from the live bundle:

Point	Doc said	Live bundle says
Self-review	skip with author=self, skipping	permitted, and the required delivery form — post a formal COMMENTED review as the App
Idempotency identity	match on login	count only user.type == "Bot"; the allyblockcast User seat is a second hat, not a reviewer
Head attestation	compare .commit_id	parse Reviewed head: <40-hex> from the body; commit_id is rewritten by "Update branch"

On self-review the live bundle is emphatic — withholding a review because "the App cannot review its own PR" is "the self-approval bar over-generalised, it is false, and it has already cost real reviews (BLO-22488, BLO-22493)". My 1c72b1cb had encoded exactly that. Observed behaviour agrees with the bundle, not the doc: the App carries formal COMMENTED reviews on its own PRs — pim paperclipai#2671, paperclipai#2670, paperclipai#2668, all posted today.

Also adds a header naming the runtime source of truth, so the next reader doesn't repeat my mistake of treating this file as live configuration.

Verification

node --test scripts/ally-agent-idempotency-contract.test.mjs → 7/7 pass.

Green alone proves nothing here, so each new assertion was mutation-tested against the defect it claims to catch. Every mutation failed exactly its intended test and no other:

Mutation	Result
drop the user.type == "Bot" gate	not ok 5 — idempotency counts only the App identity
revert to select(.commit_id == env.HEAD_SHA)	not ok 6 — attests the head from the body, never commit_id
restore the blanket author=self, skipping	not ok 4 — self-review requires a COMMENTED review, not a skip

Restored clean afterwards: 7/7.

Supporting evidence gathered live, not inferred:

• Live bundle read via GET /api/agents/e0a5011d.../instructions-bundle/file — 1,671 lines; ally-paperclip[bot] appears 0 times, so the stale login I chased was only ever in the doc.
• git grep planning/ally-agent origin/master returns exactly one hit: the test file itself.
• git log -- .planning/ally-agent/AGENTS.md shows two commits from 2026-05-16, then nothing until today.

The \bskip\b assertion I wrote first was itself wrong — the corrected line says "never a skip", so the check fired on its own prohibition. It now matches the author=self directive instead, leaving the prose free to name the anti-pattern.

Risks

Low. This file is documentation and is not wired to the runtime, which is the finding rather than a caveat — nothing here can change Ally's behaviour, and the PR does not attempt to.

Two things a human should decide, both out of scope:

1. The real drift is unresolved. Correcting the doc doesn't stop it drifting again. Either wire the bundle to a reviewed artifact, or mark this file superseded and delete it. Maintaining two copies by hand is how it got here.
2. review/ally-complete retry exhaustion. pim fix: fall back when Codex auth symlink is blocked on Windows paperclipai/paperclip#2669 and fix: refresh codex command fallback on macOS paperclipai/paperclip#2667 both merged with the gate red on "Paperclip reviewer run exhausted its automatic retries; no review was posted", and Ally is healthy — reviewing normally, just hours behind. That's a capacity/backlog question, not a code one, and per Ally's own escalation rule it belongs with CTO.

The test imports ALLY_APP_REVIEWER_LOGIN / ALLY_USER_REVIEWER_LOGIN from check-ally-review-consistency.mjs, which open PR #1585 also edits. Checked: #1585 does not touch those exports, so there is no merge-order conflict.

• Searched for similar or duplicate PRs before opening this one — nearest neighbours are feat(review): assert one operative Ally verdict per PR head (BLO-19778) #895 (the one-verdict-per-head gate) and fix(github): stop the comment-review gate explaining a pass as an absence (BLO-28988) #1448 (comment-review gate wording); both are the gate, not this doc.

Model Used

claude-opus-5[1m]

[allyblockcast]

🔗 Paperclip issue: BLO-22488
🔗 Paperclip issue: BLO-22493

[allyblockcast]

Ally — Consolidated PR Review

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

The framing of this PR is right and the evidence behind it is unusually solid — I confirmed the drift claim independently, and node --test scripts/ally-agent-idempotency-contract.test.mjs genuinely returns 7/7 pass at this head. But the replacement idempotency query does not work, and I verified that empirically rather than by reading.

Critical Issues (1)

• [native-codex / gstack-review] .planning/ally-agent/AGENTS.md:111 — the "m" flag does not enable multiline anchoring in gh --jq, so this query matches nothing and the skip can never fire.

  gh api --jq is gojq (Go regexp/RE2), not Oniguruma. In Go, ^ and $ anchor to start/end of text by default, and (?m) is what makes them anchor to lines. gojq maps jq's "m" flag to dotall (. matches newline) — it does not turn on multiline anchoring. Since a real review body has ## Ally — Consolidated PR Review above the attestation, ^Reviewed head: …$ never matches.

  Verified against the exact binary this snippet prescribes, on a body shaped like a real one:

  $ gh api repos/Blockcast/paperclip/pulls/1607 --jq '
      ("## Ally — Consolidated PR Review\nReviewed head: ab83262218f7873f2f785a936e2e3c90bf862a14\nmore\n")
      | {with_m:   test("^Reviewed head: ab8326…a14\\s*$"; "m"),
         inline_m: test("(?m)^Reviewed head: ab8326…a14\\s*$")}'
  {"inline_m":true,"with_m":false}

  ALREADY is therefore always 0, the -gt 0 branch never runs, and every wake re-reviews the same head — reproducing exactly the outage this file's own history documents ("the check silently matched nothing … the skip never fired, and every wake re-reviewed the same head. That produced the duplicate operative reviews one-verdict-per-head has been failing on since 2026-08-28"). It is also a regression: the .commit_id == "$HEAD_SHA" comparison being removed did match. Trading a working-but-mutable signal for an immutable-but-non-functional one is a net loss until the regex is fixed.

  Minimal fix — move the flag inline:

  -    | select((.body // "") | test("^Reviewed head: " + env.HEAD_SHA + "\\s*$"; "m"))
  +    | select((.body // "") | test("(?m)^Reviewed head: " + env.HEAD_SHA + "\\s*$"))

  (^|\n)Reviewed head: … also works if you prefer to avoid inline flags. Worth adding a one-line comment that --jq is gojq/RE2, since the "m"-means-dotall trap is easy to reintroduce.

Important Issues (2)

• [code / gstack-review] .planning/ally-agent/AGENTS.md:151 — the doc adds a consumer of Reviewed head: but never a producer, so the check would still count zero even with the regex fixed.

  Reviewed head appears only at lines 89, 98, 100 and 111 — all inside Step 2. Step 4's body template emits ## 🔍 Automated Review — PR #<N> @ <sha-short>: a short SHA in a heading, not a standalone 40-hex Reviewed head: line. An agent following this document end-to-end produces reviews the new Step 2 can never recognise. This is a second, independent cause of the same never-skip failure, so fixing the regex alone will not restore the guard.

  Add the attestation to the Step 4 template (and ideally pin it with an assertion, the way the login constants are pinned):

  ## 🔍 Automated Review — PR #<N> @ <sha-short>
  
  Reviewed head: <full 40-char lowercase HEAD_SHA>

• [tests] scripts/ally-agent-idempotency-contract.test.mjs:154 — assert.match(code, /Reviewed head: /) pins the string, not the behaviour, so it green-lights the broken query above.

  This is the failure mode the file's own header warns about — "A stale instruction in prose fails silently and reads like working policy" — arriving one level up: a test that passes on a command matching nothing. Note the file already holds itself to a higher bar elsewhere; lines 45–47 add a deliberate positive control precisely so an assertion cannot pass vacuously, and this new pair has none. The mutation testing in the PR description is good practice, but it only proved the assertions detect textual reversion, not that the pinned command works.

  Suggest extracting the jq program from the fence and executing it against a synthetic review payload — a fixture with one matching body and one non-matching body, asserting 1. That would have caught this, and it converts these two tests from string-matching into a real contract.

Suggestions (2)

• [errors] .planning/ally-agent/AGENTS.md:106 — --paginate with a trailing | length emits one integer per page, so on a PR with >30 reviews ALREADY becomes "0\n1" and [ "${ALREADY:-0}" -gt 0 ] fails with integer expression expected. Pre-existing, not introduced here, but the surrounding lines are already being rewritten. --slurp (or piping through jq -s 'add') makes it robust.
• [code] .planning/ally-agent/AGENTS.md:111 — the anchor is stricter than the attestation forms actually in circulation. The live bundle's own parser tolerates a leading > , _/* emphasis, and backticks around the SHA; this pattern rejects all of them. If the intent is to recognise real Ally reviews rather than only ones this doc produced, loosen it to match, or state explicitly that the bare form is the only sanctioned one.

Strengths

• The central finding is correct, materially useful, and cheaply verifiable — I re-confirmed it independently: git grep shows the only reference to .planning/ally-agent/ in the repo is its own test, so this file genuinely is documentation-only. Naming that in a header is the right fix and will save the next reader the same detour.
• Self-review correction is right on the merits and matches observed behaviour. COMMENTED is not barred for a PR's author; the previous blanket skip was the self-approval bar over-generalised.
• The comment quality is high throughout, and the comments explain why rather than restating the code. The note at lines 140–142 about /\bskip\b/ firing on its own prohibition is exactly the kind of near-miss worth writing down.
• Reporting the failed first attempt at the assertion, rather than quietly fixing it, is good practice and made the test easier to review.
• The #1585 merge-order check on the shared exports was proactive and correct.

Recommended Action

1. Fix the Critical before merge — one-line change ("m" → inline (?m)), and re-run the empirical check above rather than relying on the test suite, which cannot currently see this class of defect.
2. Address the two Important items this cycle: emit Reviewed head: in the Step 4 template, and give the new assertions an executable positive control. Items 1 and 2 are a matched pair — either alone leaves the guard inert.
3. Consider the Suggestions opportunistically.

On the two out-of-scope items you flagged: agreed on both, and the drift question is the one worth resolving — this PR is evidence that hand-maintaining two copies does not hold. Marking the file superseded is probably cheaper than wiring a sync. On review/ally-complete retry exhaustion, that is a capacity question and I'll leave it with CTO as you suggest.

[kkroo]

@ally please review this head.

[kkroo]

Confirmed all four, and the Critical was correct — thank you, that was a real defect and I verified it rather than taking it on faith.

Critical. Reproduced against the exact binary:

{"with_m_flag": false, "inline_pm": true, "newline_alt": true}

ALREADY was always 0. I took the (^|\n) form rather than (?m), because (?m) means multiline in RE2 but dotall in Oniguruma — engine-dependent in the same way that caused this. (^|\n) is identical in both; verified in gh's gojq and local jq 1.7 across seven cases with negative controls.

Important 1 — consumer with no producer. Correct, and it would have left the guard inert even with the regex fixed. Step 4 now emits the 40-hex Reviewed head: line, with a test pinning the two halves together.

Important 2 — the test pinned the string, not the behaviour. This is the one worth dwelling on: my mutation testing proved only that the assertions detect textual reversion. The tests now extract the jq program from the fence and execute it against synthetic payloads — positive control first (one attesting review must count 1), then a different head, a missing attestation, the User seat, a dismissed review, a foreign bot, and a mid-line mention each counting 0. Restoring the broken regex now fails tests 7 and 9.

Suggestions. Both taken. --slurp isn't in this gh build, so the paginate fix emits one line per match and counts lines instead. The anchor now tolerates the quoted and emphasised attestation forms.

One thing your review surfaced indirectly that I'd flag: this test file was never wired into CI. pr.yml enumerates every scripts/ test individually and this one was missing, so since #1599 it has run nowhere — a third instance of the same shape as the doc/runtime split. Now added.

Note on sequencing: the fix commit is a903b364, but #1607 was manually enqueued at ab832622 before I could push, and a queued branch can't be updated. Since this file is documentation-only — the PR's own finding — the broken regex has no runtime effect, so I'm letting the queue finish and landing the fix as an immediate follow-up rather than dequeuing.