Review pr

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 it 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.

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:

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:

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. Then check out the change head detached, per the code-host doc's read-only checkout — but gate the checkout on the worktree check in the same command, so that if your cwd ever drifts to the primary checkout the git checkout simply does not run (you get a clean blocked report instead of hijacking the user's working state). GitHub default:

[ "$(git rev-parse --path-format=absolute --git-dir)" != "$(git rev-parse --path-format=absolute --git-common-dir)" ] \
  || { echo "escaped worktree — refusing to touch the primary checkout"; exit 1; }
git fetch origin "pull/<PR>/head" && git checkout --detach FETCH_HEAD

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. Read full diff

git fetch origin main    # local host: skip the fetch, diff against main
git diff origin/main...HEAD

Also read the existing feedback and rendered diff — GitHub default:

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.

3. Review

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)
• Missing tests — acceptance criteria from issue not covered
• 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, per its "read the checks" operation. GitHub default:

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 code-host doc'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.
• Still running → do not wait for it. Fall back to the local run below.
• 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.

Separate findings into actionable (require a code change: bugs, spec violations — missing/wrong requirements, scope creep — failing checks, missing acceptance criteria, security) and notes (style preferences, questions, nice-to-haves, refactoring smells). Only actionable findings block.

4. Post the review

For each actionable finding, post an inline review comment on the exact line, 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:

gh api repos/{owner}/{repo}/pulls/<PR>/reviews \
  --method POST \
  -f event="COMMENT" \
  -f body="<overall summary, including non-blocking notes>" \
  -f "comments[][path]"="<file>" \
  -F "comments[][line]"=<line> \
  -f "comments[][side]"="RIGHT" \
  -f "comments[][body]"="<finding>"

If no actionable findings: post a review whose summary starts with a clear "CLEAN" (non-blocking notes may go in the body). 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:

gh pr ready <PR>

Rules

• Post review even if no findings ("CLEAN" summary; never an approval event)
• 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
• Never run the suite locally when the change's CI already reports green for its head sha
• Unattended: never ask the user anything; when unsure whether a finding blocks, ask "would this stop me merging?" — if not, it's a note