review-pr git:20261027.eff518cA

Immutable. This exact content never changes and is served at /api/v1/blob/48304c7ae5279cba.

---
name: review-pr
description: Reviews a change (PR/MR) diff against main, posts inline review comments and a summary, then marks it ready for review. Tracker- and host-agnostic — GitHub via gh is the factory default; docs/agents/code-host.md overrides. Use when user says "review pr", "review this pr", "/review-pr", or wants to run automated review on a pull request.
---

# Review PR

Reviews the change's diff, posts the review, marks it ready.

**Contract doc.** Change mechanics come from the repo's
`docs/agents/code-host.md` — read that file first if present. The commands
below are the **GitHub factory defaults** (`gh`), used verbatim when that doc is
absent or confirms GitHub; when it defines a different mechanic for an
operation (checkout, read feedback, post review, mark ready), the doc
wins. "PR" below means whatever the code host calls a reviewable change.

**Its annexes are deferred, not optional.** `code-host.md` links phase
annexes — `code-host-ci.md` above all — with the phase that opens each one
spelled out. **Do not read an annex at the start**: open it at the step that
names it, and not before. A worker that reads them all up front pays for the
whole contract in its first turn to use a quarter of it.

## Invoke

```
/review-pr          # reviews PR for current branch
/review-pr 42       # reviews PR #42
```

## Flow

### 1. Identify and check out the PR

If no ref given, get the current branch's change metadata — GitHub default:
```bash
gh pr view --json number,title,headRefName,baseRefName,state
```

Refuse if PR is closed or merged.

If the current branch is not the PR branch (the /developer pipeline runs this
in a fresh worktree), first confirm **where you are**:

```bash
git rev-parse --path-format=absolute --git-dir --git-common-dir   # two different paths = linked worktree
```

`--path-format=absolute` is not optional. Without it git answers with whatever
form is shortest from your cwd, so in the primary checkout's *root* both print
`.git` (equal, correct) but from any *subdirectory* they print an absolute path
and `../.git` — different strings for the same repo, which reads as "linked
worktree" and lets the checkout below run against the user's checkout.

If both paths are equal you are in the **primary checkout** — detaching or
switching it would hijack the user's working state. Never do it: as a
/developer worker end with `RESULT blocked reason=escaped worktree —
refusing to touch the primary checkout`; interactively, stop and tell the
user. Only once that command has answered with two different paths, check out
the change head detached, per the code-host doc's read-only checkout — as
**plain, separate commands**, one per call. GitHub default:

```bash
git fetch origin "pull/<PR>/head"
git checkout --detach FETCH_HEAD
```

**Do not fold the worktree check into the checkout.** The tempting shape —
`[ … ] || { echo …; exit 1; }` ahead of `git fetch && git checkout` — is one
the worktree sandbox refuses as "too complex to verify", costing three failed
calls in every review before you run the bare checkout anyway. Read the
`git rev-parse` answer, decide between calls, then run the two commands.

Never `gh pr checkout` — in a linked worktree it fails with
`fatal: '<branch>' is already used by worktree` because the PR branch is
still checked out in the build worker's worktree. Never `git checkout main`
either — `main` is checked out in the primary worktree.

### 2. Settle the review scope

A PR is reviewed more than once: the fix cycle sends the reviewer back after
every fix pass. Re-reading the whole change each time is the most expensive
thing in the loop and buys the least — the parts nobody touched since the last
review were already reviewed, by this same procedure, and found sound.

So first ask the code host **what revision was last reviewed**, per its
read-the-last-reviewed-revision operation. GitHub default:

```bash
gh api "repos/{owner}/{repo}/pulls/<PR>/reviews" \
  --jq 'map(select(.state != "PENDING" and (.body // "") != "")) | last | .commit_id // empty'
```

Empty output means nobody has reviewed this PR yet. The body filter is not
optional: GitHub stores every reply to a review thread as a review of its
own — `COMMENTED`, empty body, `commit_id` = the head at reply time. The
fixer replies *after* pushing, so without the filter the "last review" is
its reply, pinned to the fix commit, and the re-review wrongly finds nothing
new. A real review always carries a summary (step 4). Then:

```bash
git fetch origin main    # local host: skip the fetch, diff against main
```

- **No previous review** → **full scope**. The review diff is
  `git diff origin/main...HEAD`.
- **A previous review, and its sha is an ancestor of HEAD**
  (`git merge-base --is-ancestor <sha> HEAD`) **and is not HEAD** →
  **incremental scope**. The review diff is `git diff <sha>..HEAD`.
- **The sha is HEAD** → nothing has been pushed since the last review. Do not
  re-review the same commit: as a /developer worker report
  `RESULT blocked reason=no new commits since the last review (<sha>)`;
  interactively, say so and stop.
- **The sha is not an ancestor of HEAD** (force-push, rebase, a review posted
  against a branch that was rewritten) → the anchor is meaningless. Fall back
  to **full scope**.

Under incremental scope, `git diff origin/main...HEAD` is still yours to read
as **context** — the surrounding code a new hunk lives in, the function it
calls — but findings come from the review diff. A line nobody touched since
the last review is not a finding, however tempting: it was reviewed and it
passed. What replaces the re-read is the thread check in step 3.

Say which scope you used in the review summary, naming the anchor sha on an
incremental one.

Reading that context follows the repo's own method: where the agent docs
prescribe a zone map or an index/outline command, use it rather than `cat`,
never truncate it or silence its errors, and batch several lookups into one
command separated by `echo ===`. Each call costs a whole turn, and a reviewer
pays them on top of the diff it came to read.

### 2b. Read the feedback

Whatever the scope, read the existing feedback and the rendered diff —
GitHub default:
```bash
gh pr view --comments   # existing comments
gh pr diff              # rendered diff with context
```

Then locate the **originating spec**: the issue(s) the PR body's
`Closes #N` references (the /developer pipeline guarantees one;
interactively, fall back to issue refs in the branch name or commit
messages). Read each with its comments per the tracker doc — GitHub
default `gh issue view <N> --comments` — including the parent spec/PRD
when the issue's `Parent` section names one. If no spec can be found, say
so in the review summary and skip the spec-fidelity checks below.

If the repo has a `REVIEW.md` at its root, read it now: it is the owners'
word on what blocks and what to ignore in this repo, and where it is
specific its lists override the generic labels in step 3.

### 3. Review

**On incremental scope, start with the previous findings.** Read every
unresolved thread from the last review, per the code-host doc's read-feedback
operation, and for each one decide whether the new commits actually address it
— a reply that says "fixed" is a claim, the diff is the evidence. A finding
still standing is a finding again: repeat it (a reply on its thread, not a new
inline comment) with its label unchanged — a standing `[blocking]` blocks, a
standing `[fix]` does not. This is what pays for not re-reading the rest of
the change: nothing merges without every previous `[blocking]` finding
answered in code.

Then review the scope diff. Check for:
- **Correctness bugs** — logic errors, off-by-ones, null/undefined, wrong types
- **Spec fidelity** — requirements or acceptance criteria from the
  originating issue that are missing, partial, or implemented wrong;
  behaviour the issue never asked for (scope creep)
- **Root cause** (bug fixes) — check the PR's `## Root cause` against the
  code: does the fix act where the wrong value originates, or guard one
  consumer downstream? A downstream guard that leaves other consumers
  exposed is `[fix]`, naming them; `[blocking]` only when the issue's own
  criteria still fail through another path. A bug fix with no
  `## Root cause` section is `[fix]`
- **Missing tests** — acceptance criteria from issue not covered
- **Tests that do not test** — a new or changed test that would still pass
  with the behaviour it names broken: it reaches the result by another path,
  asserts before an asynchronous result arrives, or leans on a default that
  does the work anyway. Ask of each one what would make it fail.
  `[blocking]` when it is the only test of an acceptance criterion, `[fix]`
  otherwise — never a note
- **Security** — injection, unvalidated input, exposed secrets
- **Simplification** — dead code, duplication, over-engineering
- **Refactoring smells** (never blocking) — Fowler's catalogue: mysterious
  name, duplicated code, feature envy, data clumps, primitive obsession,
  repeated switches, shotgun surgery, divergent change, speculative
  generality, message chains, middle man, refused bequest. Each is a
  judgement call — label it as one ("possible Feature Envy"), and skip
  anything a documented repo standard endorses or tooling already enforces
- **Checks** — see below; failures are always blocking

#### Checks — let CI answer where it can

The typecheck and the test suite are the most expensive part of this review,
and on a repo with CI they are the *third* time the same commands run on the
same commit: the builder ran them, the change's CI is running them, and you
would run them again.

So, **before installing anything**: if `docs/agents/code-host.md` declares a
CI system, read the checks recorded for the change's **head sha**. **This is
the step that opens `docs/agents/code-host-ci.md`** (that doc's CI annex) if
the repo has one — take its "read the checks" operation from there. GitHub
default:

```bash
gh pr checks <PR> --json name,state,link --jq \
  '[.[] | select(.state != "SUCCESS" and .state != "SKIPPED")]'
```

- **Green** (empty output, at least one check present) → **skip the install
  and the local suite entirely.** Review the diff and spec fidelity only, and
  say in the summary that checks were taken from CI (name the head sha). The
  suite has already answered; re-running it buys nothing and costs the most
  context of anything you do.
- **Red** → check the failing job **actually executed**, per the CI annex's
  classify-a-red operation (GitHub default: `gh run view <run-id>
  --json jobs`, `<run-id>` from the check's `link` — a failed job with zero
  steps never started). Never started (runner offline, CI minutes
  exhausted) → the red recorded nothing about this change: treat it as **no
  checks recorded** and fall back to the local run below. Executed →
  **NEEDS_FIXES**, with the failing job's **URL** in the finding. Do not
  try to reproduce it locally and do not review around it: the fixer needs
  the job, not your re-run.

  **A red check invalidates that check, and nothing else.** CI reports per
  check, so the ones it reports green for this same head sha are answered —
  read them and move on. This is the branch reviewers get wrong: in a measured
  run a change came back red on **formatting alone**, and the reviewer
  reinstalled dependencies and re-ran the lint gate, clippy, the library tests
  and the full suite locally — all of which CI had already reported green for
  that exact commit. That re-derivation was most of a 118k review, and it
  produced one finding CI had handed it for free. When some checks are red and
  the rest are green, the local run below is **not** the fallback: it is a
  duplicate.
- **Still running** → **do not wait, and do not run the suite locally
  either.** Review the diff and spec fidelity on their own, report your
  verdict now, and name the head sha and pending checks in the summary
  ("checks still running at `<sha>`: `<names>`"). Polling holds a worker slot
  for the whole CI run — 64 seconds of a 4-minute review, measured — and the
  orchestrator's checks gate waits on that same CI again before merging. The
  local run below is the fallback for **no CI at all**, not for CI that has
  not finished.
- **No CI declared** (or no checks recorded for the head sha) → the local run
  below, exactly as before.

**Local run** (the fallback): install dependencies quietly, then run the
project's typecheck and test commands once (see `AGENTS.md` / `CLAUDE.md`)
with its quietest reporter (`--reporter=dot`, `--silent`) — a green suite's
per-test output is pure context cost. On a red run, re-run **only** the
failing file or test name to get the detail you need to write the finding.

Label **every** finding with exactly one severity, written first, as is,
whatever language the review is in:

- **`[blocking]`** — stops the merge: bugs, spec violations (missing or wrong
  requirements, scope creep), failing checks, missing acceptance criteria,
  security, a test that is the only one of a criterion and cannot fail.
- **`[fix]`** — does not stop the merge, but the change should not stay this
  way and the fixer will do it: duplication of a mechanism the change
  already has, a weak test that does not test what it names, dead code the
  change leaves behind. Say what to do, so the fix needs no interpretation.
- **`[note]`** — nobody acts on it: style preferences, questions,
  nice-to-haves, refactoring smells, anything harmless.

Only `[blocking]` findings block. When unsure between `[fix]` and `[note]`,
ask whether you would want it changed before this lands; if you would not
mind either way, it is a note.

**Report the class, not the line.** When a finding is one instance of a
pattern (a missing `await`, an unescaped input, the same off-by-one), search
the scope diff for the others and name every location in that one finding:
inline on the first, the full list in its body. On a re-review the
untouched instances are out of scope, so one left out now ships.

**Pre-existing bugs never block.** A bug in a line the change neither adds
nor modifies is `[note] pre-existing: …`, however serious — fixing it is
another issue's work, and asking the fixer for it grows the diff and costs
a cycle. The exception: the change makes it newly reachable (it now calls
that code with input it never received before). Then it is the change's
bug, labelled as any other.

### 4. Post the review

For each `[blocking]` and `[fix]` finding, post an inline review comment
on the exact line, label first, per the code-host doc's post-review operation; group everything into
one review submission where the host supports it. GitHub default — use
`line` + `side` (`position` is deprecated) and `-F` for the numeric field:

```bash
gh api repos/{owner}/{repo}/pulls/<PR>/reviews \
  --method POST \
  -f event="COMMENT" \
  -f body="<overall summary: every finding with its label, grouped by label>" \
  -f "comments[][path]"="<file>" \
  -F "comments[][line]"=<line> \
  -f "comments[][side]"="RIGHT" \
  -f "comments[][body]"="<finding>"
```

If no `[blocking]` findings: post a review whose summary starts with a clear
"CLEAN" (`[fix]` and `[note]` findings still go in it). Never use an approval
event (`APPROVE`, `glab mr approve`, …) — the pipeline authors changes
under the same identity that reviews them; on GitHub self-approval is
rejected outright (HTTP 422), and everywhere the CLEAN summary is the
approval signal.

### 5. Mark ready

As a `/developer` worker, skip this step — marking the PR ready is the
orchestrator's job (on a local code host it stays yours: `Status: ready`
goes in the same change-file commit as the review).

Interactively, per the code-host doc — GitHub default:

```bash
gh pr ready <PR>
```

## Rules

- Post review even if no findings ("CLEAN" summary; never an approval event)
- Settle the scope before reading anything: a re-review reads the diff since
  the last reviewed sha, never the whole change again
- Under incremental scope, untouched code is out of bounds for new findings —
  it was already reviewed. Every previous `[blocking]` finding, on the other
  hand, must be confirmed fixed in code before the review can be CLEAN
- Never push code changes — review only (exception: a local code host's
  review lives in the change file; committing that one file is the review)
- One review submission, not comment-by-comment (where the host can batch)
- Flag typecheck / test failures as blocking, whether they came from CI or
  from your own run — a red check is never a note
- A red check invalidates only itself — take the checks CI reports green for
  the same head sha as answered, and never re-derive them locally.
- Never run the suite locally when the change's CI already reports green for
  its head sha
- Never wait on a pipeline that is still running — report on the diff, name the
  pending checks, and let the merge gate settle them
- Unattended: never ask the user anything; when unsure whether a finding
  blocks, ask "would this stop me merging?" — if not, it is `[fix]` or
  `[note]`
- One finding per pattern, listing every location in the scope diff
- A bug in lines the change does not touch is `[note] pre-existing`, unless
  the change makes it newly reachable