---
name: code-review
description: Use when reviewing a diff or pull request — check correctness, tests, readability, security, and server/client boundaries before approving.
license: MIT
compatibility: Git working tree or PR diff available
allowed-tools: Read Grep
metadata:
  suite: quality
---

# Code Review

## Overview

Give a focused, constructive review of a change. Separate blocking issues from
optional suggestions. Review the **work product** (diff + tests), not the author's
process.

**Core principle:** Review early, review often — before merge, not after production.

## When to Review

**Mandatory:**
- Before merging to the main branch
- After completing a major feature or multi-file change
- When security-sensitive paths change (auth, payments, user data)

**Valuable:**
- After fixing a non-trivial bug (regression check)
- Before a large refactor (baseline sanity check)

## Process

1. Read the change description and confirm it matches the diff.
2. Identify scope: which files, routes, or modules changed and why.
3. Check correctness: edge cases, error handling, and inputs that could break.
4. Check tests: is new behavior covered? Do existing tests still pass?
5. Check readability: clear names, small functions, no dead code.
6. Check boundaries: server-only code must not leak to client bundles or logs.
7. Note security concerns as **blocking** (see `project-security` skill).

## Review Checklist

| Area | Blocking if… |
| --- | --- |
| **Correctness** | Wrong logic, missing error handling, race conditions |
| **Tests** | New behavior untested; tests deleted without replacement |
| **Security** | Unvalidated input, secrets in code/logs, missing auth |
| **API shape** | Breaking changes without migration or docs |
| **Performance** | Obvious N+1, unbounded loops, sync I/O on hot paths |

## Output Format

Use this structure every time:

```
## Blocking
- [file:line] Issue — concrete fix

## Suggestions
- [file:line] Optional improvement

## Verdict
approve | approve-with-nits | request-changes
```

- List **blocking** issues first, each with a concrete fix.
- Then non-blocking suggestions.
- End with an explicit verdict.

## Integration with TDD

If the change adds behavior without a failing test first, that is a **blocking**
issue unless the author documents why (e.g. config-only change). Ask for tests
that prove the behavior.

## Anti-patterns

- Approving without checking tests or security-sensitive paths.
- Nitpicking style while missing error handling.
- Vague feedback ("this feels wrong") without a concrete fix.
- Reviewing the conversation instead of the diff.
- Rubber-stamping because "it's small."

## When You Disagree

Push back with technical reasoning. Cite failing tests, spec requirements, or
security impact. Request clarification when the intent is unclear.