organvm-iv-taxis/code-review-checklist

Code Review Checklist

A structured checklist for reviewing code changes — whether a diff, a pull request, or a single file — that keeps feedback consistent, prioritized, and actionable instead of scattered or stylistic-only.

When to Use

• Reviewing a pull request or diff before merge
• A human or another agent asks "review this code"
• Self-review before opening a PR
• Auditing an existing file for latent issues

The Five Dimensions

Work through each dimension in order. Skip a dimension only if it is genuinely inapplicable (e.g. "performance" for a docs-only change) — say so explicitly rather than silently omitting it.

1. Correctness

• Does the change do what it claims to do? Trace the logic against the stated intent (PR description, issue, commit message).
• Are edge cases handled: empty input, null/None, zero, negative numbers, empty collections, concurrent access, off-by-one boundaries?
• Do error paths actually handle errors, or just swallow/log and continue silently?
• Does it introduce a regression? Diff against prior behavior, not just the new code in isolation.
• Are there any logic inversions (== vs !=, && vs ||, inclusive vs exclusive bounds)?

2. Security

• Untrusted input: is anything from a user, network request, file, or environment variable interpolated into a shell command, SQL query, HTML template, or file path without sanitization?
• Secrets: any hardcoded credentials, tokens, or keys — including in test fixtures or comments?
• Authorization: does the change check permissions/ownership before acting, or does it trust a client-supplied ID?
• Dependency risk: does it add a new third-party dependency, and if so, is it maintained and reasonably scoped?
• Injection classes to scan for explicitly: command injection, SQL injection, path traversal, SSRF, log injection (unsanitized values written to logs an operator later greps/pastes), and unpinned/mutable CI Action tags used as a supply-chain vector.

3. Performance

• Any obviously quadratic-or-worse operation on a collection that could be large in production (nested loops over the same dataset, repeated linear scans)?
• N+1 query patterns (a loop issuing one DB/API call per iteration instead of batching)?
• Unbounded growth: does the change introduce a cache, buffer, or accumulator with no eviction or size cap?
• Are expensive operations (network calls, disk I/O, crypto) on a hot path that could be memoized, batched, or moved off it?

4. Readability & Maintainability

• Can a stranger to this code understand the change from the diff and its names alone, without needing the author's context?
• Is the change scoped to one concern, or does it mix refactoring with new behavior (making the diff harder to review and revert)?
• Are names accurate and specific (not data, temp, handleStuff)?
• Is there dead code, commented-out blocks, or leftover debug output (console.log, print, pdb.set_trace()) left in?
• Does it follow the file's/repo's existing conventions (formatting, error-handling style, module layout) rather than introducing a new one inconsistently?

5. Test Coverage

• Does the change include tests for the new behavior, or only for the happy path?
• Are the edge cases identified in "Correctness" above actually covered by a test, not just mentioned in a comment?
• If a bug is being fixed, is there a regression test that would have caught it — i.e. does the new test fail against the old code and pass against the new code?
• Do existing tests still pass, and did the change quietly weaken an assertion to make them pass (e.g. loosening a strict equality check, or adding a broad try/except: pass)?

Output Format

Produce a summary, not a transcript of every line examined:

## Code Review: <file/PR>

**Correctness**: <finding or "no issues found">
**Security**: <finding or "no issues found">
**Performance**: <finding or "no issues found">
**Readability**: <finding or "no issues found">
**Test Coverage**: <finding or "no issues found">

### Priority Actions
1. <highest-priority fix, with file:line>
2. <next priority>
...

• Order findings by actual risk/impact, not by which dimension they came from.
• Cite file:line for every finding — a review comment without a location is not actionable.
• Distinguish a blocking issue (correctness bug, security hole, missing critical test) from a suggestion (style, minor readability nit) — do not let nits drown out real defects.
• Never comment on formatting a linter/formatter already enforces automatically.

Anti-Patterns to Avoid

• Rubber-stamping: approving without tracing at least the changed logic paths against their stated intent.
• Style-only reviews: flagging naming/formatting while missing a real correctness or security defect in the same diff.
• Vague feedback: "this could be cleaner" without a concrete alternative.
• Reviewing in isolation: judging a diff hunk without reading enough surrounding context to know whether it's actually correct in situ.