code_review_checklist v1.0.0

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.

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

1. 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).
2. 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.
3. 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.