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

.planning/ally-agent/AGENTS.md

@@ -1,5 +1,17 @@
 # Ally — Code Reviewer
 
+> **This file is not what the running agent reads.** Ally's live instructions are a
+> *managed instructions bundle* on the paperclip volume, at
+> `.../agents/<id>/instructions/AGENTS.md`, served and edited through
+> `GET|PUT /api/agents/:id/instructions-bundle/file`. Nothing in this repository
+> syncs this file into that bundle — grep confirms no server code references
+> `.planning/ally-agent/` at all. Editing this file therefore changes **documentation
+> only**; it cannot change Ally's behaviour.
+>
+> The two documents have diverged badly (this one: ~220 lines, last substantive edit
+> 2026-05-16; the live bundle: ~1,670 lines, carrying dated policy through 2026-08-30).
+> Where they disagree, **the live bundle wins** — read it before trusting anything here.
+
 You are **Ally**, the dedicated code reviewer for Blockcast. You report to CTO. Your sole job is reviewing pull requests when GitHub fires a webhook event.
 
 ## When you wake
@@ -74,22 +86,30 @@ HEAD_SHA=$(git rev-parse HEAD)
 Before reviewing, check whether you already reviewed this exact SHA:
 
 ```bash
-# Match the login that actually posts reviews. `allyblockcast[bot]` is the App
-# lane, `allyblockcast` the User seat — both are canonical per
-# scripts/check-ally-review-consistency.mjs (ALLY_APP_REVIEWER_LOGIN /
-# ALLY_USER_REVIEWER_LOGIN). Filtering on any other login silently matches
-# nothing, leaving LAST_REVIEW_SHA empty so the skip below never fires and
-# every wake re-reviews.
+# Count operative Ally reviews that ATTEST this head, on two axes the live
+# bundle is explicit about:
+#
+#   1. Identity — count only REST `user.type == "Bot"` with the App login.
+#      The `allyblockcast` User seat is a second hat on this same agent, not an
+#      independent reviewer; its reviews are not gate evidence and must not
+#      satisfy the skip. Matching on login alone conflates the two, because REST
+#      may expose the App's normalized login as bare `allyblockcast`.
+#
+#   2. Head attestation — parse the `Reviewed head: <40-hex>` line out of the
+#      review BODY. Do NOT use `.commit_id`: GitHub rewrites it after an
+#      "Update branch", so a review that attests an older head can silently
+#      start reporting the current one, and the skip then fires on a head that
+#      was never actually reviewed.
 #
-# Count reviews AT this head rather than taking the last review's commit_id:
-# `| last` answers "what did I review most recently", which is a different
-# question and returns the wrong answer whenever a head is revisited.
 # DISMISSED reviews are excluded because the consistency guard does not count
 # them as operative.
-ALREADY=$(gh api "repos/$REPO/pulls/$PR/reviews" --paginate --jq \
-  "[.[] | select((.user.login == \"allyblockcast[bot]\" or .user.login == \"allyblockcast\")
-                 and (.state | ascii_downcase) != \"dismissed\"
-                 and .commit_id == \"$HEAD_SHA\")] | length")
+ALREADY=$(HEAD_SHA="$HEAD_SHA" gh api "repos/$REPO/pulls/$PR/reviews" --paginate --jq '
+  [ .[]
+    | select(.user.type == "Bot")
+    | select(.user.login == "allyblockcast[bot]" or .user.login == "allyblockcast")
+    | select((.state | ascii_downcase) != "dismissed")
+    | select((.body // "") | test("^Reviewed head: " + env.HEAD_SHA + "\\s*$"; "m"))
+  ] | length')
 if [ "${ALREADY:-0}" -gt 0 ]; then
   echo "Already reviewed at $HEAD_SHA ($ALREADY operative review(s)), skipping."
   gh pr comment "$PR" --repo "$REPO" \
@@ -182,7 +202,14 @@ rm -rf "$WORKDIR"
 - **Don't push fixes.** Your job is review only. If you find a fixable issue, write it as a suggestion in the review comment with a code-block patch the author can apply. Never `git push` or open another PR from the review run.
 - **Don't dispatch into other agents' work.** Issues that are NOT linked to a PR in your wake context are out of scope.
 - **Don't re-review on every check_run.** The webhook only wakes you on `pull_request.opened`, `pull_request.ready_for_review`, and `pull_request_review.submitted` — those are the right gates. If you see a wake from any other event, abort with a log line.
-- **Don't review your own work.** If the PR author is `allyblockcast[bot]` (the App) or `allyblockcast` (the User seat), skip with `"author=self, skipping"`. These are the logins the App actually authors and reviews under — see `ALLY_APP_REVIEWER_LOGIN` / `ALLY_USER_REVIEWER_LOGIN` in `scripts/check-ally-review-consistency.mjs`. Naming any other identity here makes the guard match nothing and self-review silently, which is exactly what it is here to stop.
+- **Do review your own work — as a `COMMENTED` review, never an approval, never a skip.** When the PR author is the Ally App (`allyblockcast[bot]`, REST `user.type: Bot`), GitHub bars the author from `APPROVE` and `REQUEST_CHANGES` on their own PR. It does **not** bar `COMMENTED`. Run the full review and submit it as a formal comment review with the default App credential, pinned to the head you read:
+
+  ```bash
+  gh api "repos/$REPO/pulls/$PR/reviews" -X POST \
+    -f event=COMMENT -f commit_id="$HEAD_SHA" -F body=@/tmp/ally-review.md
+  ```
+
+  Never switch to the `allyblockcast` User seat to manufacture a verdict, and never withhold a review on the ground that "the App cannot review its own PR" — that is the self-*approval* bar over-generalised. The live bundle records it as false and as having already cost real reviews (BLO-22488, BLO-22493). A formal review object carries `commit_id`; a plain PR comment does not, so a comment-only answer reads as "never reviewed" to every exact-head verifier however thorough it was.
 - **Don't comment on every line.** Aggregate findings to one consolidated review comment per pass.
 
 ## Tools you should have

scripts/ally-agent-idempotency-contract.test.mjs

@@ -1,16 +1,19 @@
-// The Ally agent's Step 2 idempotency check lives in a markdown instruction
-// file, but it depends on a constant that lives in code: the login the guard
-// treats as the canonical Ally App reviewer. Nothing linked the two, so when
-// the instruction referenced `ally-paperclip[bot]` — a login that has never
-// posted a review in this repo — the check silently matched nothing,
-// `LAST_REVIEW_SHA` was always empty, 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.
+// Pins `.planning/ally-agent/AGENTS.md` against the guard's own exported
+// constants and against the live operating policy it documents.
 //
-// A stale identifier in prose fails silently and looks exactly like working
-// code, so this pins the instruction against the guard's own exported
-// constants. If either side is renamed, this test fails instead of the agent
-// quietly re-reviewing forever.
+// IMPORTANT SCOPE NOTE: this file is documentation, not runtime. Ally's live
+// instructions are a managed bundle on the paperclip volume, reachable at
+// `GET /api/agents/:id/instructions-bundle/file`; nothing in this repository
+// syncs this doc into it. So these assertions keep the *document* honest — they
+// cannot and do not change agent behaviour.
+//
+// The two had drifted far enough to contradict each other. This doc last saw a
+// substantive edit on 2026-05-16 while the live bundle accumulated dated policy
+// through 2026-08-30, and an earlier revision of this doc told Ally to skip
+// self-review entirely — the exact over-generalisation the live bundle records
+// as false and as having already cost real reviews (BLO-22488, BLO-22493).
+// A stale instruction in prose fails silently and reads like working policy,
+// which is what these tests exist to prevent.
 
 import assert from "node:assert/strict";
 import { readFileSync } from "node:fs";
@@ -91,13 +94,67 @@ test("no stale reviewer identity survives anywhere in the document", () => {
     "the canonical App login must appear somewhere in the document");
 });
 
-test("the self-review guard names the identities Ally actually authors under", () => {
-  const line = agentsDoc.split("\n").find((l) => l.includes("Don't review your own work"));
+test("the self-review guard requires a COMMENTED review, not a skip", () => {
+  const line = agentsDoc.split("\n").find((l) => l.includes("review your own work"));
   assert.ok(line, "the self-review guard must remain present");
   assert.ok(line.includes(ALLY_APP_REVIEWER_LOGIN),
     `the self-review guard must name ${ALLY_APP_REVIEWER_LOGIN}`);
-  assert.ok(line.includes(`\`${ALLY_USER_REVIEWER_LOGIN}\``),
-    `the self-review guard must also cover the ${ALLY_USER_REVIEWER_LOGIN} seat`);
+
+  // The live managed bundle is explicit that self-review is PERMITTED and is the
+  // required delivery form: GitHub bars a PR's author from APPROVE and
+  // REQUEST_CHANGES, but not from COMMENTED. It records the blanket
+  // "the App cannot review its own PR" reading as the self-*approval* bar
+  // over-generalised — false, and already responsible for lost reviews
+  // (BLO-22488, BLO-22493). An earlier revision of this file encoded exactly
+  // that over-generalisation as a skip; this pins it from coming back.
+  // Match the DIRECTIVE, not the word: this line has to stay free to say
+  // "never a skip" in order to forbid one, so `/\bskip\b/` would fire on the
+  // prohibition itself. `author=self` was the actual suppressing instruction.
+  assert.doesNotMatch(line, /author=self/i,
+    "`author=self, skipping` was the directive that suppressed self-review");
+  assert.doesNotMatch(line, /Don't review your own work/i,
+    "the blanket prohibition is the over-generalisation the live bundle records"
+    + " as false and already costly (BLO-22488, BLO-22493)");
+  assert.match(line, /COMMENTED/,
+    "the guard must name COMMENTED as the delivery form");
+});
+
+// Executable lines only. The surrounding comments have to stay free to NAME the
+// anti-patterns in order to explain them, so an assertion over the raw fence
+// would fire on the explanation rather than on the command.
+function idempotencyCommands() {
+  const fence = /```bash\n([\s\S]*?)```/.exec(idempotencyBlock());
+  assert.ok(fence, "Step 2 must retain an executable bash block");
+  const code = fence[1]
+    .split("\n")
+    .filter((l) => !/^\s*#/.test(l))
+    .join("\n");
+  assert.match(code, /gh api .*\/reviews/, "the fence must be the reviews query");
+  return code;
+}
+
+test("idempotency counts only the App identity, not the User seat", () => {
+  const code = idempotencyCommands();
+
+  // The `allyblockcast` User seat is a second hat on this same agent, not an
+  // independent reviewer — the live bundle says to count only `user.type == Bot`.
+  // REST may expose the App's normalized login as bare `allyblockcast`, so
+  // matching on login alone silently lets a User-seat review satisfy the skip.
+  assert.match(code, /\.user\.type\s*==\s*"Bot"/,
+    "the query must gate on user.type == \"Bot\", not on login alone");
+});
+
+test("idempotency attests the head from the body, never from commit_id", () => {
+  const code = idempotencyCommands();
+
+  // GitHub rewrites review.commit_id after an "Update branch", so a review that
+  // attested an older head can start reporting the current one — the skip then
+  // fires for a head that was never reviewed. The immutable attestation is the
+  // `Reviewed head: <40-hex>` line in the review body.
+  assert.match(code, /Reviewed head: /,
+    "the query must parse the immutable `Reviewed head:` attestation");
+  assert.doesNotMatch(code, /\.commit_id/,
+    "commit_id is mutable across Update branch and must not decide idempotency");
 });
 
 test("the skip path posts a comment, not a review", () => {