Review a Pull Request (GitHub)

One responsibility: turn a pull request into a defensible verdict backed by reproduced evidence, packaged as comment text a human posts. The verdict comes from canonical state — the integration branch and the design records the change claims to implement — never from the pull request's own framing of itself, and never from what the reviewing session happens to remember. Nothing is believed that was not either read in canonical state or reproduced by a command whose exit code ends up in the comment.

When to use / Skip if

Use when:

• A pull request in this repository needs review before merge.
• A previously reviewed pull request has a new round of fixes whose claims need re-verification.
• Someone states that a review finding is addressed, and the claim needs checking against the code.

Skip if:

• The change is uncommitted work in the local tree — that is the /review command; there is no pull request to ground against.
• The task is authoring or publishing an issue or ADR — github-cr-issue, github-cr-adr, github-post-issue.
• The ask is to merge, release, or back-merge — that is the release procedure (docs/reference/releasing.md), not a review.

Reviewer policy is out of scope: who reviews is decided by the repository, not by this skill. It enforces one staffing rule and no others — the reviewer is not the author.

Step 0 — detect the repo and pin the target

REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner)
[ -n "$REPO" ] || REPO=$(git remote get-url origin | sed -E 's#(git@|https://)github.com[:/]##; s#\.git$##')
N="${1:?pull request number}"
BASE=$(gh pr view "$N" --repo "$REPO" --json baseRefName -q .baseRefName)

Pass --repo "$REPO" on every gh command, so the review reads the repository the session is in rather than a hardcoded one.

Phase 1 — Ground the review before judging anything

Nothing is assessed until canonical state has been read. In order:

Design records are GitHub issues in this repository today; if they ever move into repository files, step 3–4 become a file read and nothing else in this phase changes.

Three rules make this phase load-bearing:

• The reviewer is not the author. Compare author.login from step 2 against the account this session acts as (gh api user -q .login); if they match, stop here and hand the review to someone else. Nothing downstream restores the independence a self-review gives up.
• The record is the specification, the pull request is a claim about it. A change that satisfies its own description while missing a criterion in the record is not done, and reading the criteria from the description would never reveal it.
• No anchor, no review. A pull request that links no issue or ADR and states no acceptance criteria gets request changes, with a single blocking item asking for the anchor. A change reviewed against its own claims always passes, which is the same as not reviewing it.

Record each ADR's status (Proposed, Accepted, Rejected, Superseded, Deprecated) — status decides how binding the conformance check in references/checklist.md is.

Phase 2 — Stage the head in a throwaway worktree

Review the code as it will exist after merge, in isolation from the working checkout:

git fetch origin "pull/$N/head"
WT="$(dirname "$PWD")/$(basename "$PWD")-wt-pr${N}-review"   # absolute: Phase 4 cd's into it
git worktree add --detach "$WT" FETCH_HEAD
git -C "$WT" rev-parse --short HEAD          # pin every claim in the comment to this sha
git diff "origin/$BASE...FETCH_HEAD"

The three-dot diff shows what the branch adds, not what the base moved on to — a two-dot diff attributes other people's merged work to this author.

Remove the worktree once the comment is written: cd "$ROOT" first — Phase 4 leaves the session standing inside $WT, and git worktree remove / git worktree prune fail when run from inside the directory being removed — then git worktree remove "$WT" (add --force when the suites left artifacts behind), then git worktree prune. A worktree left on disk becomes a stale second checkout that the next review silently reads.

Phase 3 — Dispatch the reviewer roles

The roles are fresh subagents that know only what this skill hands them. Session memory is not an input: a reviewer who remembers the last round inherits its conclusions and re-derives nothing. Launch them in parallel from this session, never nested — a role that spawns the other stops being independent of it. Prompt shapes: references/reviewer-prompts.md.

Independence is the point: agreement between a role that saw the discussion and one that could not is evidence; agreement between two roles fed the same narrative is not. When they disagree, the disagreement goes in the comment — the blind reviewer's finding is not overruled just because the discussion explains it away.

Phase 4 — Verify, do not trust

Every claim in the comment is either reproduced or dropped. The context verifier runs this phase in the worktree from Phase 2; the rest of this section is the specification it works from.

Bootstrap first, or the exit codes mean nothing. A detached worktree carries no virtual environments, so the component suites fall back to an interpreter that cannot import them and every exit code becomes noise. Build them inside the worktree, where the Makefile already prefers them and **/.venv/ keeps them out of the drift check — and where removing the worktree disposes of them:

ROOT="$PWD"   # remembered so teardown can leave $WT before removing it
cd "$WT"   # every command in this phase runs here, never in the working checkout
python3 -m venv tools/spec-linter/.venv && tools/spec-linter/.venv/bin/python -m pip install -e 'tools/spec-linter[dev]'
python3 -m venv tools/spec-judge/.venv  && tools/spec-judge/.venv/bin/python  -m pip install -e tools/spec-linter -e 'tools/spec-judge[dev]'

Never install these editable into the ambient interpreter: the worktree is removed at the end of the review, and a global editable install is then left pointing at a path that no longer exists. Separately, make check needs pytest importable by the ambient python3, and make lint needs shellcheck on PATH. When pytest is not importable, do not reach for make install-deps — it installs into the machine's user site. Get the same coverage without mutating anything: uv run --with pytest python3 -m pytest tests/ -q followed by python3 scripts/generate-agent-router.py --check, or one more throwaway venv inside the worktree. A failure traced to a missing dependency is an environment result, reported as such; only a failure that survives a working environment is a finding about the change.

Every relative path below — the suites, the build, the drift check — resolves against the worktree because of that cd. Run them from anywhere else and the drift check reports the state of the working checkout instead of the pull-request head.

Record the exact command and its exit code for each. "Tests pass" without a command is not evidence.

The build is the only reconciler. The committed plugin/ tree is generated from .claude/, plugin-extras/ and tools/ by ./build-plugin.sh; the drift check is the scoped git diff --exit-code plugin/ .claude-plugin/ afterwards, which is exactly what the pipeline runs. Scope matters: the build regenerates the agent-router into .claude/ before it copies, so a bare git status also surfaces that legitimate regeneration — along with any unrelated edit sitting in the tree — and would file all of it as a stale mirror. Two failure shapes to look for on any change that touches the build's sources: a non-empty diff after a rebuild (the committed mirror is stale), and anything in the shipped plugin/ tree still referencing a file the build no longer produces.

Local runs are the evidence, because most CI does not fire here. Confirm what actually ran with gh pr checks "$N" --repo "$REPO", then read it against this table:

A conflicting pull request runs nothing. With no merge commit to build, no workflow is triggered — so an empty or all-green checks column on a conflicting pull request means "nothing ran", not "nothing broke". Check mergeable / mergeStateStatus from Phase 1 before reading any checks column as a signal, and say so in the comment when it applies.

The version rule is a check, not a nit: a pull request into develop must leave plugin/.claude-plugin/plugin.json at main's version. A bump on a develop-target pull request is blocking.

Phase 5 — Synthesize one comment

Merge the roles' findings into a copy of the skeleton in assets/review-comment-template.md, drafted at DRAFT="$(mktemp -t review-comment)" — outside $WT, so Phase 2's teardown cannot delete it — never into the tracked asset itself. Resolve every duplicate; keep every disagreement. Every later reference to the draft uses "$DRAFT".

1. Heading. ## Independent review — <one-line scope>, or ## Independent re-review — <what was verified> for a later round.

2. Verdict line — first sentence, in bold, never buried. Exactly one of:

   Line	Means
   **Verdict: approve.**	Nothing blocking, nothing worth a numbered item.
   **Verdict: approve with changes.**	Nothing blocking, but numbered non-blocking items stand — including any ADR inconsistency recorded rather than fixed.
   **Verdict: request changes.**	At least one blocking item.

   A reader who stops after the first line must still have the answer.

3. ### Verified — each claim with the command and exit code that reproduced it. On a re-review the heading carries the evidence conditions: ### Verified fixed (reproduced locally, isolated worktree at <sha>; <tool versions>).
4. ### Blocking and ### Non-blocking — numbered items; each names the location, the problem, and the fix. "Consider improving error handling" is not an item; "build-plugin.sh, the REPO_LOCAL_SKILLS array — the new repo-local skill is not listed, so the build ships it to consumers; append it" is.
5. ### Not verified (optional) — what was not checked, and why. On a re-review, ### Still open (optional) carries items from earlier rounds that nothing has changed.
6. Closing line — the merge decision stays with a human.

Numbering across rounds — no goalposts moved

Item numbers are continuous across every round of the same review. Round 2 starts where round 1 stopped; a returning item is referred to by its original number, never renumbered. A finding that is genuinely new gets the next number and is marked as new, so nobody has to reconstruct whether it was there before.

An item is never silently re-scoped and never silently dropped. Dropping one is a statement: say it was fixed and how it was verified, or that it was wrong and why. An item that quietly grows between rounds turns a review into a moving target, and the author has no defense against it.

What this skill never does

• It does not approve, and it does not request changes through the API. There is no state-changing pull-request call anywhere in this workflow. The output is comment text; a human posts it, and a human decides what it means for the merge.
• It does not merge, close, or push. Reading the repository and running the suites is the whole footprint — everything it writes lives inside the throwaway worktree, which Phase 2 removes.
• It does not name who should review. Only that the reviewer is not the author.
• It does not carry conclusions between rounds. Fresh roles, re-derived criteria, re-run commands — every round.

References

Related skills

• github-cr-issue · github-cr-adr — draft the records this review grounds itself in.
• github-post-issue — publishes those records; a blocking finding that deserves its own tracked issue is drafted and published there, not buried in the comment.