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.claude/skills/review/SKILL.md
GitHubReview 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.
- 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:
The two SHAs must be equal, the tree clean, and thegh 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)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/gatesran → CHANGES REQUESTED: re-run/gatesat the current HEAD. Do not fall back to "ask the user whether to trust it". - 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.pymust not be judged by its own edited copy), and pass the PR's branch name because agh pr checkout/worktree HEAD may be detached, which makes the integrity script silently skip itsfeature/*check:
A script that is not on the base branch yet (the PR introducing it, or before it merges) is reported asBR=$(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 9NOT 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 itsSCOPED_LAYER_DIRSis still the template default: report that, not a pass. Also re-run the grep-only gates exactly as written ingates/SKILL.md, substituting$BASE(the fetchedorigin/<base>) fordevelopin every command (localdevelopmay be stale aftergit fetch): Gate 3 (TODO/FIXME/HACK), Gate 4 (branch name — check$BR, sincegit branch --show-currentis 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. - Check CI. If a PR exists:
Require a check namedgh pr checks <PR> --json name,bucketgateswith bucketpass.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/reviewagain once it has. Unfinished CI is not a violation: do not log it torejections.md. No check namedgatesat all → CHANGES REQUESTED:gates.ymlruns 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/ordocs/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, andincidents.mdif it exists), plusdocs/design-system.mdand<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.mdfor an earlier PR, or reintroduces a symptom inincidents.md, is HIGH severity — name the entry it repeats. A repeat of arejections.mdentry 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.mdrule; 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.mdrule 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 theincidents.mdentries this PR's diff adds (an entry this PR added torejections.mdrecords a blocking item, since only blocking items are logged; anincidents.mdentry 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.Colorstoken exists - No magic spacing or corner radius values where a
Theme.Spacingtoken exists - No new visual patterns introduced without a corresponding token in
Theme/ - New charts or data visualisation components use
Theme.Chartstokens - 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 VERIFIEDscript is reported as a visible note, never as a pass, and does not block on its own; a pendinggatesjob 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:
- 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.
- 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-reviewor 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.