diff --git a/.planning/ally-agent/AGENTS.md b/.planning/ally-agent/AGENTS.md index 14184f87c8a9..338d7ad2c380 100644 --- a/.planning/ally-agent/AGENTS.md +++ b/.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//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 diff --git a/scripts/ally-agent-idempotency-contract.test.mjs b/scripts/ally-agent-idempotency-contract.test.mjs index a1e662ac08f3..96990a75f87c 100644 --- a/scripts/ally-agent-idempotency-contract.test.mjs +++ b/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", () => {