# Review Checklist

Applied in this order during Step 3 of `WORKFLOW.md`. Each item: check it, and if it
fails, record a finding with file:line, why it matters, and a fix.

## 1. Correctness

- [ ] Does the code do what the PR description claims?
- [ ] Are edge cases handled: empty input, null/undefined, zero, negative numbers,
      very large input, empty collections, duplicate entries?
- [ ] Off-by-one errors in loops, slices, pagination, and range checks.
- [ ] Error handling: are errors caught at the right layer, logged with enough context
      to debug, and not silently swallowed (empty `catch` blocks)?
- [ ] Async correctness: unhandled promise rejections, missing `await`, race conditions
      between concurrent operations on shared state.
- [ ] Are new branches (if/switch) exhaustive, or is there a sensible default/`else`?
- [ ] Does the change match the existing behavior contract, or does it silently change
      a function's semantics in a way that breaks callers (see blast-radius check)?
- [ ] State mutations: is shared/mutable state changed safely, or could two callers
      stomp on each other?

## 2. Security

- [ ] **Secrets**: no hardcoded API keys, passwords, tokens, or connection strings —
      including in test fixtures and comments.
- [ ] **Input validation**: is all external input (HTTP request bodies, query params,
      file uploads, CLI args, env vars) validated/sanitized before use, not just typed?
- [ ] **Injection**: SQL/NoSQL built via string concatenation instead of parameterized
      queries; shell commands built from unsanitized input; unsafe deserialization.
- [ ] **AuthZ/AuthN**: does every new/changed endpoint or mutation check that the
      *current* user is allowed to do *this specific* thing (not just "is logged in")?
      Watch for IDOR — trusting a client-supplied ID without an ownership check.
- [ ] **Output encoding**: user-controlled data rendered into HTML/JS/SQL/shell without
      escaping (XSS, injection).
- [ ] **Dependency hygiene**: new dependencies — are they widely used, actively
      maintained, and pinned? Flag any with known CVEs if that's checkable.
- [ ] **Logging PII**: are emails, tokens, full names, addresses, or other personal data
      being logged in plaintext where they shouldn't be?
- [ ] **Crypto**: no custom crypto, no MD5/SHA1 for passwords, no ECB mode, secrets
      compared with constant-time comparison where timing matters.

## 3. Tests

- [ ] New behavior has a test that would fail without the change (see Step 4 of the
      workflow for how to verify this, not just assume it).
- [ ] Bug fixes include a regression test reproducing the original bug.
- [ ] Tests cover the edge cases identified in the Correctness section, not just the
      happy path.
- [ ] No weakened assertions, increased timeouts, or skipped tests introduced to make
      CI green.
- [ ] Test names describe behavior ("returns 404 when user not found"), not
      implementation ("calls getUser").

## 4. Performance

- [ ] N+1 queries: a loop that issues one DB/API call per iteration instead of a
      single batched call.
- [ ] Unbounded queries/loops: pagination missing on a list endpoint, `SELECT *`
      without a `LIMIT` on a table expected to grow.
- [ ] Unnecessary re-computation: expensive work inside a render loop, a hot path, or
      repeated inside a request handler instead of cached/memoized.
- [ ] Big-O regressions: an O(n) lookup replaced with O(n²) (e.g. `.includes()` inside
      a loop over a large array instead of a Set/Map).
- [ ] Resource leaks: unclosed file handles, DB connections, listeners, or timers.
- [ ] For hot paths only — don't demand micro-optimization of code that runs once at
      startup or in an admin-only, low-traffic path.