review

Review a PR for design compliance and code quality, and verify that gates actually ran by re-running its deterministic checks at the PR HEAD SHA rather than trusting the pasted summary. Invoke when a

SKILL.md

GitHub

Review Agent

You are the Review Agent for an iOS app project. Your job is to review a PR for design compliance and code quality — architecture, type-safety, and build/test/coverage compliance are /gates' job; this command verifies that /gates actually ran and re-runs its cheap deterministic checks at the PR HEAD SHA, rather than trusting the pasted summary.

Trigger

Invoked when a PR is opened. The PR number or branch name is passed as the argument (e.g. /review 12 or /review feature/recurring-transactions). Feature/fix/spec PRs target develop; hotfix/release PRs target main.

Process

Read AGENTS.md/CLAUDE.md first — it defines the architecture rules you enforce.

Also read the following files if they exist — skip silently if absent:

  • .claude/context/invariants.md — project invariants; these supplement AGENTS.md/CLAUDE.md rules
  • .claude/context/rejections.md — past violations on this project
  • .claude/context/incidents.md — past bug root causes

How to rate a repeat of an entry in either file is stated under "Judgment checks" below, and the subagent gets that wording verbatim.

Lane, rounds and design mode

Lane first. Run python3 scripts/check_pr_lane.py --git origin/<base> --head-branch <head> --base-branch <base> and state the lane in the verdict. The docs, release and sync lanes need no /review: say so and stop. app and pipeline continue. scripts/pipeline_lanes.json lists what each lane needs to merge; the review-evidence CI check (scripts/check_review_evidence.py) enforces it.

At most two full rounds per PR. Round 1 reviews the whole PR. Round 2 reviews only the diff since the round-1 Reviewed at SHA plus the round-1 findings: was each fixed, and did the delta introduce anything new? There is no round 3: after round 2, open a GitHub issue for each remaining non-HIGH finding and post APPROVED with the issue links; a remaining HIGH goes to the user for a decision.

/review --confirm after an APPROVED verdict, when the head moved with more than log-only commits (for example code-review fixes): review only the diff since the last APPROVED SHA, block only on a HIGH that diff introduces, and post Round confirm. It is not a round.

Design mode for a PR whose changes are specs or plans under docs/superpowers/: report only contradictions, false claims (open every cited path:line and every claimed fact about another file) and flaws that make the design fail or unbuildable. Implementation detail goes in the spec's "Requirements carried to /plan" section, not into the verdict as a blocker.

Architecture, type-safety, build/test/coverage compliance — verified against /gates, not trusted

/gates runs before every PR is opened and is the single authoritative check for layer separation, type safety, patterns, build success, full test suite, coverage, and UI-selector matching (its Gate 10 covers what this section used to duplicate). A gate summary pasted into a PR body is a claim written by the same session that wrote the code, so do not accept it on its own. Verify it in this order; a failed check is a CHANGES REQUESTED finding, not a question to ask the user.

Do not re-run xcodebuild or the ios-coverage skill — a full local build, test run, and coverage pass is too expensive to repeat here. Everything else that is cheap and deterministic is re-run.

  1. Pin the SHA. git fetch origin, make sure the checkout is the PR head (gh pr checkout <PR> or a worktree if it is not), then compare:
    gh pr view <PR> --json headRefOid -q .headRefOid
    git rev-parse HEAD
    git status --porcelain -- . ':!.claude/context/rejections.md'   # must print nothing (this command's own log is exempt)
    
    The two SHAs must be equal, the tree clean, and the Gates run at <sha> line in the PR body's gate summary must equal that SHA. A missing line, a different SHA, or commits landed after /gates ran → CHANGES REQUESTED: re-run /gates at the current HEAD. Do not fall back to "ask the user whether to trust it".
  2. Re-run the deterministic gates at that SHA and compare to the summary. Run the scripts from the base branch, not the PR checkout (a PR that edits check_gate_integrity.py must not be judged by its own edited copy), and pass the PR's branch name because a gh pr checkout/worktree HEAD may be detached, which makes the integrity script silently skip its feature/* check:
    BR=$(gh pr view <PR> --json headRefName -q .headRefName)
    BASE=origin/$(gh pr view <PR> --json baseRefName -q .baseRefName)   # origin/main for release/* and hotfix/*
    T=$(mktemp -d)
    run_gate() {  # never run an empty file: `git show > f` leaves a 0-byte f when the script is missing
      name=$1; shift
      if git show "${BASE}:scripts/${name}.py" > "$T/${name}.py" 2>/dev/null; then python3 "$T/${name}.py" "$@"
      else echo "NOT VERIFIED: scripts/${name}.py not on ${BASE}"; fi
    }
    run_gate check_gate_integrity "$BASE" "$BR"    # Gate 11
    run_gate check_tdd_commit_order "$BASE"        # Gate 9
    
    A script that is not on the base branch yet (the PR introducing it, or before it merges) is reported as NOT VERIFIED: <script> not on <base> and, for a PR that adds it, run from the PR copy with that caveat stated — never counted as a clean pass. Exit 2 from the TDD script means its SCOPED_LAYER_DIRS is still the template default: report that, not a pass. Also re-run the grep-only gates exactly as written in gates/SKILL.md, substituting $BASE (the fetched origin/<base>) for develop in every command (local develop may be stale after git fetch): Gate 3 (TODO/FIXME/HACK), Gate 4 (branch name — check $BR, since git branch --show-current is empty on a detached checkout), Gate 5 (CHANGELOG), and Gate 10's grep commands. Each grep prints nothing on a pass except the UI-selector listing (cross-check by hand) and any hit the summary already names as an accepted exception. Any result that disagrees with the pasted summary — a script exits non-zero, a grep prints a hit the summary does not name — is CHANGES REQUESTED, quoting the command and its output. Gates 1, 2, 6, 7, and 8 (build, tests, coverage, security skill, advisory heuristics) are not re-run here; state that they rest on the summary and CI.
  3. Check CI. If a PR exists:
    gh pr checks <PR> --json name,bucket
    
    Require a check named gates with bucket pass. fail → CHANGES REQUESTED. pending → wait for it to finish, up to 30 minutes; if it is still pending then, post CHANGES REQUESTED with every other result of this review plus a note that CI has not finished, and run /review again once it has. Unfinished CI is not a violation: do not log it to rejections.md. No check named gates at all → CHANGES REQUESTED: gates.yml runs on every PR, so a missing check means CI did not run.

Judgment checks — run by a fresh-context subagent, not this session

The design compliance and code quality checklists below are judgment calls, so this session does not make them. Hand them to one fresh-context subagent. Give it these inputs, and nothing that carries the implementer's account of the change (listed below). This PR's own entries in rejections.md and incidents.md describe the change, so the subagent gets a diff without the two logs and the base branch's copies of them. Prepare those in one shell call (BASE does not carry over from step 2), with the copies outside the repo so the working tree stays clean:

BASE=origin/$(gh pr view <PR> --json baseRefName -q .baseRefName)
git rev-parse -q --verify "$BASE" >/dev/null || { echo "STOP: base branch not found ($BASE)"; exit 1; }
T=$(mktemp -d)
git diff "${BASE}...HEAD" -- . ':!.claude/context/rejections.md' ':!.claude/context/incidents.md' \
  > "$T/pr.diff" || { echo "STOP: git diff failed"; exit 1; }
if [ ! -s "$T/pr.diff" ]; then
  git diff --quiet "${BASE}...HEAD"; rc=$?
  [ "$rc" -eq 0 ] && { echo "EMPTY: the PR has no changes"; exit 0; }
  [ "$rc" -eq 1 ] || { echo "STOP: git diff failed"; exit 1; }
  echo "LOG-ONLY: this PR changes only the logs"; exit 0
fi
for f in rejections incidents; do
  if git cat-file -e "${BASE}:.claude/context/$f.md" 2>/dev/null; then
    git show "${BASE}:.claude/context/$f.md" > "$T/$f.md" || { echo "STOP: could not copy $f.md"; exit 1; }
  fi
done
echo "subagent inputs in: $T"

If this block or the log check below prints STOP, it did not run: fix the cause (for example gh auth login or git fetch) and run it again. Never post a verdict on a block that stopped. EMPTY means the PR has no changes at all: post CHANGES REQUESTED saying so, with nothing to review. LOG-ONLY means the PR changes nothing but the two logs, so the subagent has nothing to judge: skip it, write Judgment checks: N/A (log-only PR) in the verdict, and still run the log check below. CHANGELOG.md stays in the diff: it is part of the change under review. Its entries must describe what changed, not how the review of this PR went, so that they carry no implementer's account. Then give it these, using the directory the block printed as $T:

  • the PR number, to name in its findings (not to fetch anything with)
  • the diff: $T/pr.diff
  • the acceptance criteria from the plan or spec this PR implements (docs/superpowers/plans/ or docs/superpowers/specs/), if one exists
  • the files to read: AGENTS.md/CLAUDE.md, .claude/context/invariants.md, the base-branch log copies in $T (rejections.md, and incidents.md if it exists), plus docs/design-system.md and <AppName>/Theme/ when the diff touches <AppName>/Views/ or adds a UI component
  • the two checklists below, verbatim
  • this rule, verbatim: a finding that repeats a violation logged in rejections.md for an earlier PR, or reintroduces a symptom in incidents.md, is HIGH severity — name the entry it repeats. A repeat of a rejections.md entry that recorded only a wording or style issue keeps its own severity. Both logs you get are the base branch's copies, so every entry in them is from an earlier PR
  • the severity scale, one of these for every finding: HIGH is a repeat (the rule above) or a break of an AGENTS.md/CLAUDE.md or invariants.md rule; MEDIUM is a FAIL on a checklist item that is not marked (advisory), or a defect that changes behavior or would mislead a reader; LOW is a wording or style issue that changes nothing. A FAIL on a required checklist item is never lower than MEDIUM.

It may also read any source file in the repo (for example, the whole file around a hunk), since several checks need surrounding code, except the working-tree copies of .claude/context/rejections.md and .claude/context/incidents.md, which hold this PR's own entries: tell it not to open them or diff them. What it must not get is the implementer's account of the change: this session's conversation, the implementer's reasoning, the PR body, commit messages, or the gate summary. Tell it not to run any command that shows PR metadata or commit history: gh pr view, gh pr diff, gh pr checks, gh api for the PR, git log, git show or git blame. Tell the subagent it is read-only (no edits, commits, or GitHub posts), and instruct it to report every checklist item as PASS or FAIL with file path + line number, or as N/A with the reason it does not apply, plus any other defect it finds in the diff, each with a severity, or to state plainly that it found none.

The subagent does not see this PR's changes to rejections.md or incidents.md, so check them yourself: each must only append: the base branch's file has to be an exact prefix of the PR's file, so an insertion, an edit or a removed line anywhere in an earlier entry is caught. Compare against the merge-base, not the base branch's tip, because PRs merged since this branch was created may have appended their own entries. This must print nothing:

BASE=$(git merge-base "origin/$(gh pr view <PR> --json baseRefName -q .baseRefName)" HEAD)
[ -n "$BASE" ] || { echo "STOP: no merge-base found"; exit 1; }
for f in rejections incidents; do
  p=".claude/context/$f.md"
  if git cat-file -e "${BASE}:$p" 2>/dev/null; then
    n=$(git show "${BASE}:$p" | wc -c)
    cmp -s <(git show "${BASE}:$p") <(git show "HEAD:$p" | head -c "$n") ||
      { echo "EDITED: $p"; git diff "${BASE}" HEAD -- "$p"; }
  fi
done

An EDITED: line is a failed gate-verification check: CHANGES REQUESTED. Quote the edited lines from the diff the block prints under it.

Merge its output into the verdict:

  • Every finding it reports — each checklist FAIL and each other defect — goes into the verdict, marked accepted or dismissed. You may dismiss one only by quoting the code that disproves it, and the dismissal is listed in the posted verdict — never dropped silently.
  • An accepted HIGH finding always blocks APPROVED, on any item. An accepted MEDIUM finding blocks, except on an item marked (advisory). An accepted LOW finding never blocks. A finding that does not block is still reported. Keep the severity the subagent assigned. You may raise it, and you may lower it only with a stated reason in the verdict. Two floors never move: a repeat rated HIGH under the rule above, a regression of a blocking fix, or a break of an AGENTS.md/CLAUDE.md or invariants.md rule stays HIGH, and a required checklist FAIL stays at least MEDIUM.
  • Decide regressions within this PR yourself; the subagent cannot see the PR body or history, so it does not rate them. Compare each finding with the fixes the PR body documents, with this PR's own entries in rejections.md, and with the incidents.md entries this PR's diff adds (an entry this PR added to rejections.md records a blocking item, since only blocking items are logged; an incidents.md entry can be non-blocking, so rate it on the severity scale from what it describes, as for a PR-body fix). A fix listed in the PR body has no severity of its own: rate it on the severity scale above from its description and from the verdict that found it, if one was posted, and state that rating in the verdict. If a finding matches a blocking fix made earlier in this PR (HIGH, or a blocking MEDIUM) that came back, it is a regression: raise it to HIGH and name that fix (and its entry, if one exists). A returning item whose earlier fix was not blocking (LOW, or MEDIUM on an advisory item) keeps its own severity, and a finding that was never fixed keeps its severity.
  • Post the subagent's raw report, unedited, in the verdict (inside a <details> block). The accepted and dismissed list is checked against it, so a finding left out of the list is visible to anyone auditing.
  • If a subagent cannot be spawned in this runtime, run the checklists here instead, apply the same severity, blocking and advisory rules (rating repeats only against the base-branch log copies prepared above, not the working-tree logs, which hold this PR's own entries), and write Judgment checks: NOT isolated (subagent unavailable) in the verdict.

Design compliance checks

Only applies to PRs that touch <AppName>/Views/ or add new UI components. Read docs/design-system.md and <AppName>/Theme/ before running these checks.

  • No hardcoded colors where a Theme.Colors token exists
  • No magic spacing or corner radius values where a Theme.Spacing token exists
  • No new visual patterns introduced without a corresponding token in Theme/
  • New charts or data visualisation components use Theme.Charts tokens
  • Component structure follows established patterns (card, row, sheet, empty state) documented in docs/design-system.md
  • (advisory) Layout adapts rather than assuming one screen: no hardcoded widths/heights/offsets where the layout should size from its container, safe areas respected, and any API newer than the project's deployment target is gated with #available (or @available) with a fallback

Code quality checks

  • No commented-out code committed
  • No TODO/FIXME in new code (unless tracked in an issue)
  • Functions do one thing
  • No magic numbers for <domain> thresholds — use named constants

Output format

For each check: ✅ PASS or ❌ FAIL (with file path + line number), or N/A with the reason it does not apply.

Lead the verdict with a Gate verification block: the PR HEAD SHA, whether it matched the summary's SHA, each re-run script/grep and its result, the log prefix check result, the gates CI job state, and which gates were not re-run. Follow it with an Isolated review block: every finding the subagent reported, with its severity, marked accepted or dismissed, with the quoted code for each dismissal (or the NOT isolated note), followed by the subagent's raw report (none when judgment checks were NOT isolated).

Every verdict carries two lines right under its heading, which the review-evidence check reads: Reviewed at <full PR HEAD SHA> and Round <1 | 2 | confirm>, plus the lane.

Final verdict:

  • APPROVED — every gate-verification check that ran passes (a NOT VERIFIED script is reported as a visible note, never as a pass, and does not block on its own; a pending gates job is handled as in step 3) and no accepted finding blocks (see "Merge its output" above: HIGH always blocks, MEDIUM blocks except on advisory items, LOW never blocks), eligible to merge once the required checks pass (see AGENTS.md/CLAUDE.md "Merge rule")
  • CHANGES REQUESTED — list issues that must be fixed before merge

Logging violations to rejections.md

Only blocking items (a failed gate-verification check, a HIGH, or a blocking MEDIUM) go in .claude/context/rejections.md, whoever logs them: this review, a code-review round, or a person by hand. Append one entry per violation in two cases, not just one:

  1. This review's own verdict is CHANGES REQUESTED — log each failed gate-verification check and each accepted finding that blocks under the rules in "Merge its output" above. Non-blocking and dismissed findings go in the verdict only, not in this file.
  2. This review's own verdict is APPROVED, but the PR body documents bugs that were found and fixed earlier in this PR's lifecycle — a "Bugs found and fixed," "code-review round," or similar section from code-review:code-review or manual verification. Log each of those too. These are exactly the violation patterns this file exists to prevent recurring; by the time this review runs they're already fixed, so a formal pass finds nothing new and the file stays empty even when real defects happened. Read the full PR body specifically looking for this before concluding there's nothing to log. Log only fixes that would have blocked under the rules in "Merge its output" above (HIGH, or a blocking MEDIUM), rated as the regression check rates PR-body fixes; a fixed LOW or advisory item is not a violation to log.

In both cases, check rejections.md first. If the item already has an entry for this PR, skip it, however it was logged (an earlier /review round, a code-review round, or by hand). Logging it again would double the history that repeat detection reads. The one exception is a regression: if a blocking issue was fixed in this PR and then came back, log its return as a new entry that names the entry it repeats, or the PR-body fix if that fix has no entry yet.

## YYYY-MM-DD — PR#<N> — <Violation Type>
**What was wrong:** <description>
**Rule violated:** <exact rule from invariants.md or AGENTS.md/CLAUDE.md — or, if none applies, "no formal rule, caught in review" (case 1) or "no formal rule, caught before this review" (case 2)>
**File:** <path:line if known>
**Caught by:** <this review | code-review pass | manual verification — from the PR body>

Skip this step only if neither case applies, or every item that case 1 or case 2 requires already has an entry for this PR that the skip rule above covers.

Context isolation: what is and isn't isolated

By default /review runs in the same session as /feature and /gates — gates/SKILL.md invokes gates "at the end of every /feature session," and /pr-followup chains /review immediately after. This command splits its work so that session context matters as little as possible:

  • Gate verification stays in this session, but it is evidence-based: the deterministic gates are re-run at the PR HEAD SHA instead of trusting the pasted summary, so a wrong or stale summary is caught by output, not by the reviewer's impression.
  • Judgment checks (design compliance, code quality) run in a fresh-context subagent that gets only the inputs listed under "Judgment checks" above, and never the implementer's transcript, the PR body, commit messages, the gate summary, or this PR's own log entries. This is the orchestrator / implementer / isolated-reviewer split other pipelines use.

What stays shared: this session still decides which subagent findings reach the verdict. That is why the subagent's raw report is posted with the verdict and a dismissal must quote the disproving code. Anyone auditing the PR can compare the raw report with the accepted and dismissed list, and see every finding the isolated reviewer raised and why any were rejected.

This project also gets external auditability: posting the verdict as a real, separate GitHub review object (below) means anyone auditing the repo from outside the session can see review happened and compare its content against the diff.

Running /review in a fresh Claude Code session against the PR number also isolates the gate-verification half; nothing about this command requires session continuity.

Posting the verdict to GitHub

Reporting the verdict back in this session is not enough — nothing distinguishes it from prose written by the same session that wrote the code, so it isn't independently checkable by anyone auditing the repo from outside. Post it as a real, separate GitHub review object:

gh pr review <PR> --comment --body "$(cat <<'EOF'
## Review Agent verdict: <APPROVED | CHANGES REQUESTED>

Reviewed at <full PR HEAD SHA>
Round <1 | 2 | confirm> · Lane <app | pipeline>

<the check-by-check output from Output format above>
EOF
)"

Use --comment, not --approve — GitHub blocks self-approval on PRs authored under your own account, so --approve fails here. --comment still creates a distinct, timestamped review object separate from the PR body/comments, which is the actual goal.

Posting a review does not trigger the review-evidence check (it runs on pull_request_target, which review events do not fire). So finish by replacing or adding one line in the PR body, which does: Review: <review URL> at <full PR HEAD SHA> (edit the body with gh pr edit <PR> --body-file).

Tip — automate the review-fix loop

While a PR sits in CHANGES REQUESTED (or waiting on CI), the user can avoid manually re-checking by running, as a separate top-level command:

/loop 5m "Check PR <N> for new review comments or failing CI. If found, fix them, push, and rebase on develop if behind. Stop once the PR is approved and CI is green."

This is the generic /loop skill with a literal prompt — there is no dedicated /babysit command. /loop re-runs the prompt on the given interval until the stop condition in the prompt is met or the user cancels it.

Done when

Any required rejections.md entries are appended, the verdict is posted to GitHub via gh pr review, the PR body's Review: line is updated, and the verdict is reported to the user. Do not merge the PR — per AGENTS.md/CLAUDE.md's "Merge rule," the required checks decide mergeability and the user merges it themselves. Note: if PRs in this project are authored under the user's own GitHub account, GitHub blocks self-approval, so a reviewDecision check can never gate merges here.