1
# Review Checklist
2
3
Applied in this order during Step 3 of `WORKFLOW.md`. Each item: check it, and if it
4
fails, record a finding with file:line, why it matters, and a fix.
5
6
## 1. Correctness
7
8
- [ ] Does the code do what the PR description claims?
9
- [ ] Are edge cases handled: empty input, null/undefined, zero, negative numbers,
10
      very large input, empty collections, duplicate entries?
11
- [ ] Off-by-one errors in loops, slices, pagination, and range checks.
12
- [ ] Error handling: are errors caught at the right layer, logged with enough context
13
      to debug, and not silently swallowed (empty `catch` blocks)?
14
- [ ] Async correctness: unhandled promise rejections, missing `await`, race conditions
15
      between concurrent operations on shared state.
16
- [ ] Are new branches (if/switch) exhaustive, or is there a sensible default/`else`?
17
- [ ] Does the change match the existing behavior contract, or does it silently change
18
      a function's semantics in a way that breaks callers (see blast-radius check)?
19
- [ ] State mutations: is shared/mutable state changed safely, or could two callers
20
      stomp on each other?
21
22
## 2. Security
23
24
- [ ] **Secrets**: no hardcoded API keys, passwords, tokens, or connection strings —
25
      including in test fixtures and comments.
26
- [ ] **Input validation**: is all external input (HTTP request bodies, query params,
27
      file uploads, CLI args, env vars) validated/sanitized before use, not just typed?
28
- [ ] **Injection**: SQL/NoSQL built via string concatenation instead of parameterized
29
      queries; shell commands built from unsanitized input; unsafe deserialization.
30
- [ ] **AuthZ/AuthN**: does every new/changed endpoint or mutation check that the
31
      *current* user is allowed to do *this specific* thing (not just "is logged in")?
32
      Watch for IDOR — trusting a client-supplied ID without an ownership check.
33
- [ ] **Output encoding**: user-controlled data rendered into HTML/JS/SQL/shell without
34
      escaping (XSS, injection).
35
- [ ] **Dependency hygiene**: new dependencies — are they widely used, actively
36
      maintained, and pinned? Flag any with known CVEs if that's checkable.
37
- [ ] **Logging PII**: are emails, tokens, full names, addresses, or other personal data
38
      being logged in plaintext where they shouldn't be?
39
- [ ] **Crypto**: no custom crypto, no MD5/SHA1 for passwords, no ECB mode, secrets
40
      compared with constant-time comparison where timing matters.
41
42
## 3. Tests
43
44
- [ ] New behavior has a test that would fail without the change (see Step 4 of the
45
      workflow for how to verify this, not just assume it).
46
- [ ] Bug fixes include a regression test reproducing the original bug.
47
- [ ] Tests cover the edge cases identified in the Correctness section, not just the
48
      happy path.
49
- [ ] No weakened assertions, increased timeouts, or skipped tests introduced to make
50
      CI green.
51
- [ ] Test names describe behavior ("returns 404 when user not found"), not
52
      implementation ("calls getUser").
53
54
## 4. Performance
55
56
- [ ] N+1 queries: a loop that issues one DB/API call per iteration instead of a
57
      single batched call.
58
- [ ] Unbounded queries/loops: pagination missing on a list endpoint, `SELECT *`
59
      without a `LIMIT` on a table expected to grow.
60
- [ ] Unnecessary re-computation: expensive work inside a render loop, a hot path, or
61
      repeated inside a request handler instead of cached/memoized.
62
- [ ] Big-O regressions: an O(n) lookup replaced with O(n²) (e.g. `.includes()` inside
63
      a loop over a large array instead of a Set/Map).
64
- [ ] Resource leaks: unclosed file handles, DB connections, listeners, or timers.
65
- [ ] For hot paths only — don't demand micro-optimization of code that runs once at
66
      startup or in an admin-only, low-traffic path.
67