code-review-checklist

Files (1)

• SKILL.md

SKILL.md (first lines)

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

From 4444J99/a-i--skills, MIT.

More from 4444J99/a-i--skills