--- name: pr-review description: | Multi-agent PR review with finding verification. Use when asked to "review PR", "review this PR", "code review", or given a PR URL to review. Spawns 6 parallel specialized reviewers (security, QA, performance, code quality, correctness, plan adherence), verifies findings by investigating source code, categorizes by type, and prints a structured report to the terminal. Never comments on GitHub PRs. allowed-tools: Read Edit Bash Agent AskUserQuestion --- # PR Review Skill > **Note:** `{skill_dir}` refers to the absolute "Skill directory" path provided in your system instructions. Multi-agent pull request review. Spawns 6 parallel reviewers (5 Sonnet + 1 Opus) — security, QA, performance, code quality, correctness, and plan adherence — each required to investigate and verify findings before reporting. A Sonnet verification agent then reads source files to confirm or disprove each finding. Results are categorized by type (testing gaps, correctness, security, architecture, decisions needed, etc.) and printed as a structured terminal report. **Never comments on GitHub PRs.** Output is terminal-only. ## Usage ``` /pr-review ``` - `` — required. Full GitHub PR URL (e.g. `https://github.com/owner/repo/pull/123`). --- ## Phase 0 — Setup & Worktree Alignment ### Parse Arguments Extract from `$ARGUMENTS`: - PR URL (first token — required; error if absent) ### Preflight Checks 1. Verify `gh` CLI is available: `gh --version`. If not found, stop with: "gh CLI not found. Install it from https://cli.github.com and authenticate with `gh auth login`." 2. Parse the PR URL to extract owner, repo, and PR number. 3. Verify the CWD repo matches the PR repo: ``` gh repo view --json nameWithOwner -q .nameWithOwner ``` Compare against the owner/repo from the URL. If mismatch, stop with: "PR is from {url_owner}/{url_repo} but CWD is in {local_repo}. cd to the correct repo first." ### Fetch PR Metadata ``` gh pr view --json headRefName,baseRefName,number,title,additions,deletions,changedFiles,files,body,state,isDraft ``` Store: head branch, base branch, PR number, title, body, additions, deletions, changed file count, state (OPEN/CLOSED/MERGED), isDraft flag. The `files` field returns an array of `{path, additions, deletions, changeType}` objects — extract the file paths to build `{changed_files}`. ### Triage Checks (deterministic — no agent) Run before any expensive operations: - **Closed/Merged:** If state is CLOSED or MERGED, stop with: "PR #N is already {state}. Nothing to review." - **Draft:** If isDraft is true, print warning and continue: "Note: PR #N is a draft. Proceeding anyway." - **Trivially simple (lock files only):** Extract the list of changed file paths from the `files` array in PR metadata. If ALL changed files match these patterns — `*.lock`, `*.sum`, `package-lock.json`, `yarn.lock`, `pnpm-lock.yaml`, `*.generated.*` — stop with: "PR #N changes only lock/generated files. No review needed." If ANY file outside these patterns is changed, proceed. ### Worktree Alignment Record the original branch/worktree before any checkout. 1. Run `git worktree list --porcelain` to list all worktrees. 2. Check if any worktree has a branch matching the PR's head branch. 3. If a matching worktree is found: use that worktree's path as the working directory for all subsequent git and file operations. 4. If no matching worktree: check if the current branch matches the PR's head branch. If yes, use the current working directory — no checkout needed. 5. If neither: run `gh pr checkout `. If it fails due to a dirty working tree, run `git stash --include-untracked`, retry the checkout, and remember to `git stash pop` after review. If stash also fails, stop with: "Cannot checkout PR branch and stash failed." 6. After the review completes (Phase 4) — or if any phase fails — return to the original branch or worktree. If changes were stashed in step 5, run `git stash pop` to restore them. If `git stash pop` fails due to conflicts, do NOT run `git stash drop` — inform the user: "Your changes are in git stash. Run `git stash pop` manually and resolve conflicts." Always attempt stash restoration on error paths, not just success paths. ### Read Project Rules From the repo root in the correct worktree: - Read `CLAUDE.md`. If missing, use: `"No CLAUDE.md found."` - Read `CONTRIBUTING.md`. If missing, use: `"No CONTRIBUTING.md found."` Store as `{claude_md_rules}` and `{contributing_md_rules}`. ### Discover Implementation Plan Search for a plan file that matches the PR's topic. Detect the memory directory using the convention in `[{skill_dir}/../../references/project-memory-reference.md](../../references/project-memory-reference.md)` (Directory Detection section). **Primary:** Search `{memory_dir}/plans/` files and parse each file's `**Branch:**` header field. Match the value against the PR's head branch name. If a match is found, this is the plan file. **Fallback:** If no `**Branch:**` header match is found, check `{memory_dir}/plans/` for files whose name relates to the PR title or branch name. If a matching plan file is found, read it and store as `{plan_content}` and record the file path as `{plan_file_path}`. If no plan exists, use `"No implementation plan found."` for `{plan_content}` and `""` (empty string) for `{plan_file_path}`. The Correctness Reviewer and Plan Adherence Reviewer use this to detect plan drift. ### Extract Test Plan If a plan file was found (i.e., `{plan_file_path}` is non-empty), check whether the plan file contains a `## Test Plan` section. If such a section exists, read the `**Test Plan:**` path annotation from it. Path boundary validation: normalize the extracted path (resolve `..` segments) and verify it falls within `{memory_dir}/test-plans/`. If the normalized path escapes that directory, set `{plan_test_plan}` to `""` (empty string) and log a warning: "Warning: test plan path escapes {memory_dir}/test-plans/ boundary — setting {plan_test_plan} to empty string." If the path is valid, attempt to read the file. If the file does not exist, set `{plan_test_plan}` to `""` (empty string) (graceful fallback — no warning). If the file exists and is readable, read its contents and store as `{plan_test_plan}`. If the plan file has no `## Test Plan` section, or if no plan file was found, set `{plan_test_plan}` to `""` (empty string). Pass `{plan_test_plan}` to the QA Reviewer, Correctness Reviewer, and Plan Adherence Reviewer. ### Fetch PR Diff After worktree alignment, ensure the base branch ref is available and compute the diff locally: ``` git fetch origin {base_branch} git diff $(git merge-base origin/{base_branch} HEAD)..HEAD ``` This is faster than `gh pr diff` and gives agents natural access to individual files via `Read`, `Grep`, and per-file `git diff -- ` without needing to parse a monolithic diff string. Store as `{diff}`. ### Build Input Context Assemble these values — they are passed to reviewers in Phase 2: - `{pr_description}` = "PR #N: {title}\n\n{body}" - `{diff}` = full diff output - `{claude_md_rules}` = CLAUDE.md content or placeholder - `{contributing_md_rules}` = CONTRIBUTING.md content or placeholder - `{changed_files}` = newline-separated list of changed file paths - `{plan_content}` = implementation plan content or placeholder - `{plan_file_path}` = path to discovered plan file or empty string --- ## Phase 1 — Reviewer Applicability Determine which of the 6 reviewers apply based on changed file types. Default: all 6 reviewers run. Skip a reviewer only if its domain has zero applicability: | Reviewer | Skip condition | |----------|----------------| | Performance | ALL changed files are `.md`, `.txt`, `.rst`, or other documentation-only formats | | Security | (never skip — security concerns appear in config and docs too) | | QA | (never skip) | | Code Quality | (never skip) | | Correctness | (never skip) | | Plan Adherence | Skip if no implementation plan found in Phase 0 | Record which reviewers will run. --- ## Phase 2 — Parallel Review Read `[{skill_dir}/references/reviewer-prompts.md](./references/reviewer-prompts.md)`. For each applicable reviewer, locate the corresponding prompt template, substitute all placeholders with actual values, and spawn an agent. Most reviewers use `model="sonnet"`; the Plan Adherence Reviewer uses `model="opus"`. Spawn all applicable reviewers simultaneously (parallel Agent calls). ### Domain Reviewers (5 parallel) ``` Agent( description="Security review of PR #{number}", model="sonnet", prompt= ) Agent( description="QA review of PR #{number}", model="sonnet", prompt= ) Agent( description="Performance review of PR #{number}", model="sonnet", prompt= ) Agent( description="Code quality review of PR #{number}", model="sonnet", prompt= ) Agent( description="Correctness review of PR #{number}", model="sonnet", prompt= ) ``` Each domain reviewer receives: `{pr_description}`, `{diff}`, `{claude_md_rules}`, `{contributing_md_rules}`, `{changed_files}`. The Correctness Reviewer also receives `{plan_content}` and `{plan_test_plan}`. The QA Reviewer also receives `{plan_test_plan}`. ### Plan Adherence Reviewer (6th, parallel with above) Only spawned if a plan was found in Phase 0 (i.e., `{plan_file_path}` is non-empty). ``` Agent( description="Plan adherence review of PR #{number}", model="opus", prompt= ) ``` The Plan Adherence Reviewer receives: `{plan_content}`, `{plan_file_path}`, `{plan_test_plan}`, `{diff}`, `{changed_files}`, `{pr_description}`, `{claude_md_rules}`, and `{contributing_md_rules}`. When `{plan_test_plan}` is empty string, omit the `UAT CROSS-REFERENCE` section entirely from the QA Reviewer prompt, omit the `UAT SCENARIOS` section from the Correctness Reviewer and Plan Adherence Reviewer prompts — do not render the heading, label, or empty placeholder in any of these reviewers. ### Collect Findings After all agents complete, collect all findings into a consolidated list. Assign each finding a unique ID (e.g., `sec-1`, `qa-1`, `perf-1`, `cq-1`, `cor-1`, `pa-1`). Preserve: description, file:line, classification, evidence, source reviewer. **Incremental workflow note:** When reviewing a PR created by an incremental swarm run, the PR naturally contains only the files for that PR boundary — no additional scoping needed. pr-review analyzes the PR diff as-is, which is already boundary-scoped. --- ## Phase 3 — Verification (batched) If no findings were reported by any reviewer, skip verification and proceed directly to Phase 4 with the 'no findings' output path. Spawn a **single** Sonnet agent with ALL findings in one call. Do NOT spawn one agent per finding — that pattern is catastrophically slow in Claude Code where each Agent() has significant startup overhead. A single batched call takes ~15 seconds; per-finding agents with 15-20 findings would take 2-5 minutes. Build the findings JSON array: ```json [ { "id": "sec-1", "reviewer": "Security", "description": "...", "location": "file/path.py:42", "classification": "needs-fix | needs-input", "evidence": "...", "diff_context": "±10 lines of diff surrounding the finding location" }, ... ] ``` For each finding, extract ±10 lines from the diff around the finding's file:line location and include it as the `diff_context` field. This gives the verifier surrounding context to investigate each finding. ``` Agent( description="Finding verification for PR #{number}", model="opus", prompt= ) ``` The verifier receives: `{findings_json}` (the array above), `{claude_md_rules}`, `{contributing_md_rules}`, and `{changed_files}`. The verifier **investigates each finding** — it reads the actual source files, traces call chains, and checks whether the finding is real. It returns a JSON array with a verdict for each finding: `[{finding_id, verdict, investigation_summary, category, classification, options}, ...]` Verdicts: `verified` (confirmed real), `false_positive` (investigated and disproven), `needs_context` (cannot confirm or deny — requires human judgment). Parse the verifier's response as JSON. If parsing fails, extract JSON from between the first `[` and last `]` markers. If that also fails, include all findings with verdict `unverified` and a note: "Verification failed — showing all findings unverified." ### Reconcile **Finding fidelity check:** Before categorizing, verify the verifier returned a verdict for every submitted finding. For each finding ID in the original `{findings_json}`, check if a matching `finding_id` exists in the verifier's response. Any finding without a returned verdict is assigned verdict `unverified` with `investigation_summary`: "Verifier did not return a verdict for this finding." This prevents silent finding loss during verification - the same principle as the Fixer verification protocol in `[{skill_dir}/../../references/finding-classification.md](../../references/finding-classification.md)`. ### Categorize The verifier assigns each finding to a category based on its nature (not its classification): | Category | Examples | |----------|----------| | **Testing Gaps** | Missing tests, untested paths, coverage gaps | | **Correctness** | Logic errors, wrong behavior, contract violations | | **Security** | Vulnerabilities, auth issues, injection, secrets | | **Architecture** | Design issues, pattern violations, structural problems | | **Decisions Needed** | Ambiguous intent, trade-offs requiring human judgment | | **Performance** | Bottlenecks, N+1, memory issues | | **Style & Conventions** | CLAUDE.md violations, naming, code quality | ### Filter Remove findings with verdict `false_positive`. Keep all `verified`, `needs_context`, and `unverified` findings. Treat `unverified` findings as `needs_context` for output purposes — they appear in the Needs Context section with the verification failure note. --- ## Phase 3.5 — Needs-Input Resolution If any surviving findings (after filtering false positives) have classification `needs-input`, present them to the user before producing the report. Do NOT skip this step - the skill must not exit with unresolved `needs-input` items. If zero `needs-input` findings remain after Phase 3, skip to Phase 4. ### Present to User Present each `needs-input` finding individually via AskUserQuestion. Each finding gets its own question with full context so the user can make an informed decision. Batch up to 4 findings per AskUserQuestion call (the tool's question limit): ``` AskUserQuestion(questions=[ { "question": "[{id}] [{Reviewer}] {description}\n\nLocation: {file}:{line}\nDecision needed: {input_needed}\n▸dp:file={file},line={line},cat={Reviewer},skill=pr-review", "header": "{id}", "options": [ ... (map each element from the finding's `options` array to {label, description}), {"label": "Defer", "description": "Skip for now — user-deferred"} ], "multiSelect": false }, ... (one question per finding, up to 4 per call) ]) ``` If more than 4 `needs-input` findings exist, make multiple AskUserQuestion calls. ### Record Decisions For each `needs-input` finding: - **Option selected (any non-Defer option):** Promote to `needs-fix` with verdict `verified`. Record the selected option label in the finding's `suggested_fix` field (this tells the Fixer which approach to implement). Place the finding in its normal category section. - **Defer selected:** Update the finding's classification to `user-deferred` in the output. Every `needs-input` item gets a recorded user decision, not silent deferral. --- ## Phase 4 — Terminal Output Print the structured `CODE REVIEW — PR #{number}` report following the `Findings Exist` or `No Findings After Verification` template and edge-case rules in `[{skill_dir}/references/output-and-edge-cases.md](./references/output-and-edge-cases.md)`. Use `━` (U+2501) for divider lines, group verified `needs-fix` findings by category (Testing Gaps → Correctness → Security → Architecture → Decisions Needed → Performance → Style & Conventions), omit empty categories, and place `needs_context` and `user-deferred` items in their dedicated bottom sections. ### Counter Increment After producing the Phase 4 terminal output, increment the `pr-review-cycle` lifecycle counter on the plan file. This is the only plan file modification pr-review makes. The counter increments only after successful review completion (Phase 4 terminal output produced). Error paths that exit before Phase 4 do not increment the counter. 1. If a plan file was discovered in Phase 0 (via branch-header matching), re-read it using the stored `{plan_file_path}` absolute path from Phase 0 (do not re-discover — the working directory may differ after worktree alignment) 2. Find the `**Iterations:**` block 3. Read the current `pr-review-cycle` value N from the line matching `- pr-review-cycle: {N}` 4. Verify N is a non-negative integer (digits only, e.g. `0`, `1`, `12`). If N is not a valid integer, skip the increment silently 5. Increment `pr-review-cycle` by 1: use Edit to replace `- pr-review-cycle: {N}` with `- pr-review-cycle: {N+1}`, where `{N}` is the actual integer read in step 3 6. If no plan file was discovered in Phase 0, or no `**Iterations:**` block found, skip silently ### Cleanup After output and counter increment, return to the original branch or worktree recorded in Phase 0. --- ## Reviewer Prompt Templates Prompt templates and placeholder definitions are in `[{skill_dir}/references/reviewer-prompts.md](./references/reviewer-prompts.md)`. Read that file and substitute placeholders before passing to each Agent call. The templates are not executable — they are documentation that Claude reads and fills in. Do not reference `quality-gate/references/` at runtime; this skill owns its own copies adapted for PR review context.