Imported from organvm-iv-taxis/a-i--skills (
skills/development/code-review-checklist/SKILL.md). Install upstream withnpx skills add organvm-iv-taxis/a-i--skills --skill code-review-checklist. Copyright stays with the author (MIT).
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:linefor 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.