review-pr

# PR Review Checklist

Systematically verify each area:

## Correctness

- [ ] Does the code do what the PR description claims?
- [ ] Are edge cases handled (null, empty, boundary values)?
- [ ] Are error paths covered with proper error handling?
- [ ] Are async operations handling errors (no unhandled promise rejections)?

## Type Safety

- [ ] No `any` or `unknown` types (except at validated system boundaries)
- [ ] Types accurately reflect the data (no type assertions hiding issues)
- [ ] Generic types used where appropriate

## Security

- [ ] No secrets in code (API keys, tokens, passwords, credentials)
- [ ] No SQL injection vectors (parameterized queries only)
- [ ] No XSS vectors (user input properly escaped)
- [ ] Authorization checks on every mutation accessing user data
- [ ] Input validation at system boundaries (Zod schemas)
- [ ] No PII in logs (names, emails, phone numbers, IPs)
- [ ] No custom cryptography (use established libraries)
- [ ] JWT/session tokens stored securely (httpOnly cookies, not localStorage)
- [ ] CORS policy is restrictive (no wildcard on authenticated endpoints)
- [ ] Error responses don't leak internal details to clients
- [ ] Dependencies free of known critical vulnerabilities

## Privacy & GDPR

- [ ] Personal data processing has documented lawful basis
- [ ] Data retention periods defined for new PII fields
- [ ] Consent collection is granular (not bundled or pre-ticked)
- [ ] Right to erasure supported (soft delete + cleanup job)
- [ ] No PII transferred outside EEA without safeguards (SCCs)

## Database

- [ ] Migrations are backward-compatible
- [ ] Destructive changes are phased or reversible
- [ ] Indexes added for new foreign keys and query patterns
- [ ] No N+1 queries
- [ ] Soft delete used (not hard delete)

## Testing

- [ ] New behavior has new tests
- [ ] No test coverage reduction for changed files
- [ ] Tests are deterministic (no flakiness)
- [ ] External dependencies are mocked appropriately

## Performance

- [ ] No unnecessary re-renders or recomputations
- [ ] No unbounded queries (pagination/limits)
- [ ] No O(n^2) or worse algorithms on large datasets
- [ ] Caching considered where appropriate

## Code Quality

- [ ] Readable by another engineer in 6 months without context
- [ ] No dead code, commented-out code, or TODOs
- [ ] No circular dependencies
- [ ] No console.log in production code
- [ ] No hardcoded magic numbers or URLs

## Scope

- [ ] PR only contains changes related to its stated goal
- [ ] No drive-by refactors or unrelated cleanup
- [ ] PR size is reasonable (flag 15+ files without justification)

---
name: review-pr
description: "Reviews a pull request with structured severity-based feedback. Use when asked to review a PR, asked for a code review, or given a PR number/URL."
---

Review the pull request: $ARGUMENTS

Follow this process:

**why-no-hook:** review workflow guidance; each step requires reading the PR and judging code quality / intent.

1. **Fetch PR details**: run `gh pr view $ARGUMENTS --json title,body,files,commits,additions,deletions,baseRefName,headRefName` `(review-time: see section note)`
2. **Read the diff**: run `gh pr diff $ARGUMENTS` to see all changes `(review-time: see section note)`
3. **Understand intent**: read the PR description, linked issues, and commit messages before reviewing code `(review-time: see section note)`
4. **Spec-conformance pass** (when a spec exists): find the originating spec - issue refs in the commits (via `gh`), a linked issue, or a spec under `.claude/state/specs/` - and check the diff against it, ideally in a parallel sub-agent so it does not pollute the main review context. When the spec has checked criteria, run the `Spec Verifier` for this pass: (a) requirements asked for but missing or partial; (b) behaviour in the diff nobody asked for (scope creep); (c) requirements that look implemented but wrong. Quote the spec line for each finding and place it in the severity buckets below. If there is no spec, skip this pass and note it. `(review-time: see section note)`
5. **Spawn relevant experts**: based on the files changed, spawn the matching expert teammates for domain-specific review (see `~/.agents/rules/agent-routing.md`) `(review-time: see section note)`
6. **Review systematically** using the checklist in [checklist.md](checklist.md) `(review-time: see section note)`
7. **Produce structured output** in this format: `(review-time: see section note)`

```markdown
## Summary

<1-2 sentence overall assessment>

## Verdict: APPROVE / REQUEST_CHANGES / NEEDS_DISCUSSION

<reason>

## Findings

### BLOCKER (must fix before merge)

- **[file:line]** - Description. Why it matters. Suggested fix.

### ISSUE (should fix, may approve with commitment to follow-up)

- **[file:line]** - Description. Recommendation.

### SUGGESTION (take it or leave it)

- **[file:line]** - Description. Alternative approach.

### NIT (style/preference, non-blocking)

- **[file:line]** - Description.

### PRAISE (good patterns worth highlighting)

- **[file:line]** - What's done well and why.
```

If no `$ARGUMENTS` provided, review changes in the current branch against the base branch using `git diff main...HEAD`.