Code Review skill for Claude Code

# Full review checklist

Read this when the diff touches auth, money, concurrency, migrations, public APIs, or
when the quick checklist in SKILL.md flags an area. Skip sections that do not apply.

## Contents

- Authentication and authorization
- Input handling and injection
- Secrets and data exposure
- Error handling and resilience
- Concurrency and async
- Data, persistence, and migrations
- Money, time, and numbers
- Performance
- API and contract compatibility
- Tests
- Frontend specifics
- Language traps

## Authentication and authorization

- Every new route, RPC, GraphQL resolver, or job handler checks authentication.
- Object-level authorization: loading `/invoices/:id` verifies the invoice belongs to
  the caller's tenant/user, not just that the caller is logged in (IDOR/BOLA).
- Role checks happen server-side; hiding a button is not authorization.
- Mass assignment: request bodies are mapped through an allowlist (DTO/schema), not
  spread directly into ORM `update()` calls (`role`, `isAdmin`, `tenantId` fields).
- Session/token changes: expiry, rotation, revocation on password change, cookie flags
  (`HttpOnly`, `Secure`, `SameSite`).
- Password or token comparison uses constant-time comparison.

## Input handling and injection

- SQL built with string concatenation or template literals instead of parameters.
  ORMs' raw-query escape hatches (`$queryRawUnsafe`, `.raw()`, `extra()`) deserve a look.
- Shell commands: `exec`/`shell=True` with interpolated input; prefer argument arrays.
- HTML: `dangerouslySetInnerHTML`, `v-html`, `innerHTML`, `|safe`, `html_safe` with
  user-controlled data.
- Paths: user input joined into file paths without normalizing and checking the prefix
  (`../` traversal). Archive extraction (zip slip).
- URLs fetched server-side from user input (SSRF): allowlist hosts, block internal
  ranges and metadata endpoints.
- Deserialization of untrusted data: `pickle`, `yaml.load` without SafeLoader, Java
  native serialization.
- Regex built from user input or with nested quantifiers on user input (ReDoS).
- Validation at the boundary with a schema (Zod, Pydantic, etc.), with size limits.

## Secrets and data exposure

- No credentials, tokens, or private keys in code, tests, fixtures, or config committed.
- Logs do not include passwords, tokens, full card numbers, or unnecessary PII; error
  responses do not leak stack traces or SQL to clients.
- API responses do not return more fields than the client needs (password hashes,
  internal flags).
- New third-party scripts or SDKs: what data do they receive?

## Error handling and resilience

- No empty `catch` blocks; no `catch (e) { return null }` that turns outages into
  wrong answers.
- Errors are either handled or propagated, not both logged-and-rethrown at every level.
- External calls have timeouts. Retries only on idempotent operations, with backoff and
  jitter and a cap.
- Resources released on every path: files, connections, locks, subscriptions, timers.
- Partial failure in multi-step operations: is state left consistent? Is there a
  compensating action or transaction?
- User-facing errors are actionable; internal errors carry context (IDs, operation).

## Concurrency and async

- Check-then-act sequences on shared state (read balance, then write) without a lock,
  transaction, or atomic operation.
- Promises not awaited; `forEach` with async callbacks (does not wait); fire-and-forget
  work with no error handling.
- Unbounded `Promise.all` / goroutines / threads over user-sized input; use a
  concurrency limit.
- Shared mutable module-level state in servers handling concurrent requests.
- Go: goroutine leaks (no cancellation via context), data races (run `go test -race`),
  closing channels from the receiver side.
- Idempotency of webhook and queue consumers: duplicate delivery must not double-apply.

## Data, persistence, and migrations

- Migrations on large tables: adding a column with a volatile default, adding NOT NULL
  without a default, creating indexes without `CONCURRENTLY` (Postgres), changing column
  types that rewrite the table.
- Deploy ordering: code that reads a new column must not ship before the migration; code
  that stops writing an old column must ship before it is dropped (expand/contract).
- Transactions wrap the operations that must be atomic and nothing slow (no HTTP calls
  inside a DB transaction).
- Queries return bounded result sets; pagination uses a stable order.
- Unique constraints exist where the code assumes uniqueness.
- Soft-deleted rows excluded where expected.

## Money, time, and numbers

- Money in integer minor units or a decimal type, never binary floats. Rounding mode is
  explicit and matches finance requirements.
- Currency stored alongside amounts.
- Timestamps stored in UTC; conversion to local time only at display. DST transitions,
  month-end, leap years, and "end of day" in which time zone.
- Integer overflow on counters or IDs (32-bit), division by zero, NaN propagation.
- Comparison of floats with `==`.

## Performance

- Database queries inside loops (N+1); ORM lazy loading in templates or serializers.
- Missing index for a new `WHERE`/`ORDER BY` on a large table.
- Loading entire tables or files into memory when streaming would do.
- Repeated expensive computation that could be hoisted or memoized.
- Synchronous I/O on an event loop (Node `fs.readFileSync` in a request handler,
  blocking calls inside `async def`).
- Frontend: large dependencies added to the main bundle; re-renders caused by new
  object/array literals passed as props on every render.

## API and contract compatibility

- Removed or renamed fields, endpoints, events, CLI flags, env vars, or config keys.
- Changed semantics with the same shape (a field that used to be cents is now dollars).
- New required request fields break existing clients; new response enum values can
  break strict clients.
- Versioning or deprecation path documented; changelog entry for public libraries.
- OpenAPI/GraphQL schema updated alongside the implementation.

## Tests

- New behavior has at least one test that fails without the change.
- Edge cases from the issue and from your adversarial inputs have tests.
- Tests do not depend on order, wall-clock time, network, or randomness without seeds.
- Mocks are limited to boundaries; the code under test is not itself mocked.
- Snapshot updates in the diff were reviewed, not blindly accepted.
- Deleted or skipped tests have a stated reason.

## Frontend specifics

- Loading, empty, and error states handled for every new data fetch.
- Keyboard access and focus management for new interactive elements; labels on inputs;
  images have alt text.
- No user-controlled HTML injection; URLs from data sanitized before `href`.
- Effects with missing or excessive dependencies; state derived from props stored in
  state (goes stale).
- Keys on lists are stable IDs, not array indexes, when items reorder.

## Language traps

- **JavaScript/TypeScript:** `==` vs `===`; `as` casts and `!` non-null assertions hiding
  real nulls; `any` leaking through public types; `parseInt` without radix;
  `array.sort()` on numbers without a comparator; `Date` month is zero-based.
- **Python:** mutable default arguments; late-binding closures in loops; bare `except:`;
  `is` for value comparison; naive vs. aware datetimes mixed.
- **Go:** ignored errors (`_ =`); loop variable capture in pre-1.22 modules; nil map
  writes; `defer` inside loops; returning a typed nil as an interface.
- **Java/Kotlin:** `equals`/`hashCode` mismatch; `Optional.get()` without check;
  `!!` in Kotlin on platform types; unclosed streams.
- **SQL:** `NOT IN` with a subquery that can return NULL; implicit casts defeating
  indexes; `LIKE '%x'` on large tables.
- **Shell:** unquoted variables; missing `set -euo pipefail`; parsing `ls` output.