review-round

model	fable
name	review-round
description	Run the codified three-reviewer loop on a diff or PR: author adversarial per-agent prompts pointing at the riskiest spots, launch code-reviewer + deploy-guard + pr-validator in parallel, reproduce every finding by execution before fixing, prove each fix with a non-vacuous regression test, and post the PR verdict comment that /release gates on. Use before shipping, or when the user says 'review round', 'run the reviewers', 'full review pass', or wants the pre-release review loop. For a spec-anchored review against your requirements or spec docs, run a spec-review pass instead.

Review round

The codified review->fix->re-verify->verdict loop. Three agents driven by hand-authored adversarial prompts catch Criticals that authoring and testing miss; this skill makes the discipline repeatable instead of re-invented per PR. It drives the companion review agents (code-reviewer, deploy-guard, pr-validator), published separately at https://github.com/eranw2000/claude-review-agents.

Model note: the load-bearing work here is judgment (adversarial prompt authoring, adjudicating findings), hence model: fable. If the fix loop turns into a long implementation session, switch explicitly with /model opus.

Two modes

• Pre-release mode (default): the full loop, steps 1-8. Criticals get fixed or formally declined before the verdict posts.
• Checkpoint mode (invoked from /pr-checkpoint step 6): steps 1-3 and 8 always; the fix loop (4-7) is optional and NOTHING blocks the PR. The checkpoint stays advisory by design; the hard gate is /release. Its step-8 verdict says CHECKPOINT-ADVISORY, never CLEAN, so the mode is visible to the gate that reads it later. See step 8 for why the two must not share a word.

The loop

1. Scope

Establish what is under review: git diff "$BASE"...HEAD for a branch, gh pr view / gh pr diff for a PR. Record the reviewed HEAD SHA (git rev-parse HEAD on the branch); the verdict in step 8 binds to it.

Find the spec that belongs to your initiative

A repo hosts more than one initiative over its life. The first writes REQUIREMENTS.md and SPEC.md at the repo root, and a later one writes its own under docs/<initiative>/. So the copy at the root is not always the one you want, and grounding on the wrong initiative's spec produces work that is internally consistent and aimed at the wrong system, which review does not catch.

Say which initiative this work is for, then resolve the file instead of assuming the root copy:

1. List the candidates: the repo root copy, every docs/*/ copy, and any other location this skill already tells you to check.
2. Read the Initiative field of each. That field decides, never the directory name, which is a slug somebody chose and can be stale or wrong.
3. Exactly one names your initiative: use it, and say which path you used.
4. None names it: STOP and tell the user what you found and where. Do not fall back to the root copy.
5. More than one names it: STOP and name every path. Two files claiming one initiative is a thing to report, not to settle by picking one.
6. A candidate carries no Initiative field: say so. Where it is the only candidate you may use it, and you must say you read a spec with no recorded owner.

When the user hands you an explicit path, use that path. Check its Initiative field the same way, and on a mismatch stop and say whether a file for your initiative sits somewhere else.

This round reviews a diff, not a specification, so it opens these files only when a finding is anchored to one. A finding anchored to the wrong initiative's spec is worse than an unanchored one, because the citation makes it look checked.

2. Author the adversarial prompts

Write a per-agent prompt that NAMES the 2-3 riskiest spots in this diff and the specific claims each agent should verify by execution. Point code-reviewer at the paths you are least sure of; point deploy-guard at the config/prose/topology angle; tell pr-validator which added tests must be proven to have run. A generic "review this diff" prompt is a protocol violation: the wins come from aiming the reviewers at the weak spots.

3. Launch all three in ONE message

Spawn code-reviewer, deploy-guard, and pr-validator in a single message so they run in parallel; they are independent. Tell each which mode this is (checkpoint: report only; pre-release: findings will be driven to resolution).

(Checkpoint mode: skip to step 8 unless the caller asked for the fix loop.)

4. Reproduce before fixing

Every Critical / Blocker / Warning gets REPRODUCED by executing the failing input through the real path before any fix is written. A finding that does not reproduce is marked disputed, with the exact command you ran; it is never silently accepted (fixing a non-bug) or silently dropped (losing a real one). Respect the side-effect guard: no reproduction against production, live services, or real credentials; a side-effectful reproduction stays unreproduced and is adjudicated by reading, with that stated.

5. Verify suggestions against the authoritative gate

A reviewer's proposed fix is a hypothesis. Check it against the authoritative release gate or the code before applying (one checker's complaint can be another gate's by-design zero). Declining a finding with a stated reason is a legitimate outcome and goes in the verdict comment.

6. Every fix gets a non-vacuous regression test

Prove each new test by mutation (break the guard, confirm exactly that test fails, restore by re-applying the code, never git checkout) or by contrast (the same input with the gate un-armed produces the opposite outcome). Confirm the printed suite total increased by exactly the number of tests added. Never add tests via shell redirection (cat >>, tee -a); use Edit/Write so the test-integrity hook (shipped in this pack) sees them.

Derive the mutation set from the DIFF, not from the fixes you remember making. Read the guard-shaped lines the branch actually added:

git diff -U0 --no-color "$BASE"...HEAD -- '*.py' '*.js' '*.ts' '*.sh' \
  | grep -E '^\+[[:space:]]*(if |elif |assert |raise |return (None|False|0|\[\]|""|\x27\x27))'

A set assembled from memory covers the lines you were already thinking about, which are the least likely to be wrong. On the PR that produced this rule an author-chosen set of six missed five real guards, and one of the five was that PR's Critical. If a listed line has no test that can fail, say so in the verdict rather than dropping it.

Two traps in the mutation run itself. Purge __pycache__ first and require a zero-fail baseline before mutating, because a same-second mutate and restore can leave stale bytecode active, and then the check lies in either direction. And restore from a snapshot taken before the run, never with git checkout: a reviewer killed mid-mutation leaves the tree mutated, and a fix built on that state ships the mutation.

7. Re-run

Touched suites first, then the FULL suite. A subset of suites is not "green".

8. Post the verdict comment

One PR comment (gh pr comment) containing:

• Per-agent finding counts (code-reviewer Critical/Warning/Suggestion, deploy-guard Blocker/Warning/Note, pr-validator verdict + totals).
• Every Critical/Blocker with its status: Fixed (name the regression test), Declined (state the reason), or Open.
• The reproduction command for each executed finding.
• The machine-readable LAST line, exactly one of:
  • Review-round verdict: CLEAN @ <reviewed-HEAD-sha>
  • Review-round verdict: CRITICALS-OPEN @ <reviewed-HEAD-sha>
  • Review-round verdict: CHECKPOINT-ADVISORY @ <reviewed-HEAD-sha>

Which value, and this is the part that is easy to get wrong. CLEAN means a COMPLETED round: every Critical and Blocker was driven to Fixed or Declined, and the full suite was re-run green. It is a certification, and /release treats it as one.

In checkpoint mode you may not post CLEAN. Post CHECKPOINT-ADVISORY instead whenever the round found nothing blocking. A checkpoint's findings are advisory by contract and its fix loop is optional, so "nothing blocked a checkpoint" is a much weaker claim than "everything was resolved" and must not wear the same word. CRITICALS-OPEN is unchanged and correct in either mode: if a checkpoint round finds an unresolved Critical, saying so should stop a later release, and it does.

The one thing that makes this a gate rather than a convention: a round that ends without completing, because it was interrupted, ran out of budget, or stopped to ask a question nobody answered, leaves whatever it last posted standing. If that was CLEAN, the PR now carries a certification nobody earned. CHECKPOINT-ADVISORY fails safe there, because the release gate stops on it and asks.

This marker string is OWNED by review-round; /release reads it verbatim and compares the SHA to the PR head at release time. The SHA binding is load-bearing: without it, a verdict posted at checkpoint time would silently certify commits pushed AFTER the review. If you fixed anything in steps 4-7, the reviewed SHA is the post-fix HEAD you re-ran the suite on; push first, then post.

Adding a fourth value is allowed, but it lands in /release's A3 in the same change or it is inert: that reader permits on CLEAN alone and stops on anything else, so a new value stops releases until A3 learns it.

Verify the comment actually landed. Posting is not the same as posted, and the difference is invisible from here:

gh pr view <number> --json comments -q '[.comments[].body | select(contains("Review-round verdict:"))] | last'

If that comes back empty, the round produced NO durable evidence: its findings live only in this transcript, and /release will later see a PR that looks unreviewed. Say so in your final message, in those terms, and hand the exact comment body to the user to post. Never end a round reporting a verdict you could not confirm is on the PR.

This is the failure that changed the rule. A real review round ran, its fixes were the PR's head commit, and no comment ever reached the PR. /release read the absence as "no gate applies" and merged 32 files and a data migration to production. Both halves of the contract were self-reported and nothing reconciled them.

Post with gh pr comment <n> --body "$(cat verdict.md)", not --body-file. --body-file appends a trailing newline, so the gate's last-line read returns EMPTY on a comment that looks perfectly posted. Command substitution strips it.

Background-session note

The verdict-then-merge flow runs gh pr comment / gh pr merge, which the auto-mode classifier can block on agent-authored PRs. A transient stage-2 classifier error gets ONE plain retry; a sustained block means asking the user to switch the session to manual mode, not fighting it.

Guardrails

• Never merge, deploy, or push to the base branch from this skill; the ship verb is /release.
• Never post a CLEAN verdict while any Critical/Blocker is unresolved and undeclined.
• Never bind a verdict to a SHA you did not actually review and test.
• In checkpoint mode, nothing here refuses, blocks, or rolls back the PR.