---
name: "code_review_checklist"
description: "Guides an agent through reviewing a diff or pull request: a concrete check sequence covering correctness, tests, security, error handling, performance, readability, and backwards compatibility. Triggers when asked to review code, a diff, or a PR."
---

# Code Review Checklist

## Purpose
Run a systematic review of a diff or PR and produce a verdict (approve / request changes / comment) with findings ordered by severity. Every check below names a concrete smell to look for, not a general principle.

## Workflow
Review in this order; stop at step 8 and synthesize the verdict.

### 1. What does the PR claim? (5 minutes, before reading code)
- Read the PR title, description, and linked issue first.
- Note the claimed behavior change in one sentence; everything below is measured against it.
- Flag immediately if the diff touches files unrelated to the claim (e.g. a bugfix PR that also reformats `utils/helpers.py` or bumps a dependency).

### 2. Correctness
- **Trace the happy path mentally**: feed a realistic input through the changed code and check the output matches the claim.
- **Edge cases**: empty input, single item, max-size input, `None`/null where a value is expected. Smell: a function that handles lists but never checks `if not items`.
- **Off-by-ones**: any `<` vs `<=`, slice bounds, or index arithmetic. Smell: `items[len(items)]`, or a retry loop that runs `retries` times instead of `retries + 1`.
- **Logic inversions**: conditions that read backwards. Smell: `if not user.is_banned: deny_access()` — say the condition out loud to check it.
- **Concurrency**: shared mutable state, missing locks, or check-then-act gaps. Smell: two threads incrementing `counter += 1` without synchronization.

### 3. Tests
- **Do the tests fail without the fix?** If the PR claims a bugfix, the new/updated test should exercise the buggy path — ask how it would behave on the old code. A test that passes either way proves nothing.
- **Coverage of the change, not the file**: are the new branches actually asserted? Smell: a test named `test_handle_error` that never triggers the error branch.
- **Assertion strength**: `assert result` vs `assert result == expected`. Weak assertions are a smell.
- **No tests for the change**: default to requesting tests for logic changes; pure refactors and comment-only changes are the usual exception.

### 4. Security
- **Injection**: user input concatenated into queries, shell commands, or HTML. Smell: `f"SELECT * FROM users WHERE name = '{name}'"` — flag even in internal tools.
- **Auth checks**: new endpoints or actions — is there an authorization check, and does it check the *right* identity (the caller, not a value from the request body)? Smell: `user_id = request.json["user_id"]` used to gate access instead of the session token.
- **Secrets in the diff**: hardcoded API keys, tokens, passwords, private keys. Smell: `api_key = "sk-live-..."` — any 20+ char random-looking string literal.
- **Unsafe deserialization**: `pickle.loads()`, `yaml.load()` without a safe loader, or `eval()` on anything that can arrive over the wire. Smell: `pickle.loads(request.body)`.
- **Path traversal**: filenames built from user input without sanitization. Smell: `open(os.path.join(UPLOAD_DIR, filename))` where `filename` comes from a request.

### 5. Error handling
- **Swallowed failures**: empty `except:` / `except: pass`, or errors logged and then execution continuing as if nothing happened. Smell: `except Exception: pass` around a network call.
- **Wrong layer**: is the error raised where it can be handled, and does the message help? Smell: `raise Exception("failed")` with no context about what failed or why.
- **Silent partial success**: a batch loop that skips failures without reporting how many failed. Flag unless the skip is intentional and documented.

### 6. Performance
- **N+1 queries**: a database query inside a loop over records. Smell: `for user in users: db.get_orders(user.id)`.
- **Unbounded work**: loops or recursion over user-controlled or dataset-sized input with no cap. Smell: `while True:` with no break tied to a condition, or fetching "all rows" with no LIMIT/pagination.
- **Missing indexes**: new `WHERE`/`JOIN`/`ORDER BY` columns on large tables — flag as "check whether an index exists," since you can't always see the schema from the diff.
- **Default to flagging only obvious orders-of-magnitude problems**; don't bikeshed micro-optimizations.

### 7. Readability and maintainability
- **Naming**: does the name say what the thing does? Smell: `data2`, `tmp`, `process()` in a file full of specific names.
- **Dead code**: commented-out blocks, unreachable branches, unused parameters or imports. Smell: a 40-line commented function left "for reference."
- **Comment quality**: flag comments that restate the code (`# increment i`) and missing comments where the *why* is non-obvious (a magic number, a workaround). Smell: `timeout = 37  # seconds` with no explanation of why 37.
- **Duplication**: the same logic pasted in two places in the diff — suggest extracting it once. One-off similarity in test fixtures usually doesn't count.

### 8. API and backwards compatibility
- **Signature changes**: renamed/removed parameters, changed defaults, new required arguments. Smell: `def send(to, body)` → `def send(to, body, channel)` breaks existing callers.
- **Response shape changes**: added/removed/renamed fields in API responses or serialized output. Flag even additive changes if consumers are strict parsers.
- **Migration concerns**: schema changes without a migration path, or a migration that can't roll back. Smell: a `DROP COLUMN` in the same deploy that still reads the column.
- **Default to flagging** any externally visible change that lacks a note on how existing callers are handled.

## Output Contract
Deliver the review as:

1. **Verdict** (exactly one): `APPROVE` / `REQUEST CHANGES` / `COMMENT`.
   - `APPROVE`: no blocking issues; nits may exist.
   - `REQUEST CHANGES`: at least one finding that must be fixed before merge.
   - `COMMENT`: findings worth noting, none blocking.
2. **Findings**, ordered by severity: **Blocker** (must fix — breaks correctness, security, or data) → **Major** (should fix — likely bug, real perf/security risk) → **Nit** (optional — style, naming, tiny cleanup).
3. Each finding: severity, `file:line` reference, what's wrong in one sentence, why it matters, and a suggested fix or question. Suggested fixes go under Blockers and Majors; Nits may be one-liners.
4. **Summary** (2–3 lines): what the PR does, whether it's safe to merge, and the single most important thing to address.

Worked example of a finding entry:

> **Blocker** — `orders/service.py:142` — The discount is applied with `total - discount` before the `discount > total` guard on line 147, so a discount larger than the order total produces a negative charge. Move the guard above the subtraction, or clamp: `total = max(0, total - discount)`.

## Operating Rules
1. Review the diff, not the author's reputation or the size of the PR. Small diffs get the same sequence.
2. Default to asking a question rather than demanding a change when the intent is ambiguous ("Is the empty-list case handled here?").
3. Every finding needs a `file:line` reference; no finding without a location ships.
4. Don't re-litigate the design unless it's broken — if the approach is sound but the style isn't your taste, that's a Nit at most.
5. Run the repo's linter and test suite if you have the environment; never claim you ran them if you didn't.
6. Keep the review tight: if everything passes, say so in the summary and stop — don't invent findings to fill space.