Code Review skill for Codex

Gives the Codex app a display name, a one-line description and a starter prompt for this skill.

interface:
  display_name: "Code Review"
  short_description: "Find the 3 a.m. bug before the naming nits"
  default_prompt: "$code-review Review PR 482 and tell me if it is safe to merge"

# Area checklists for code review

Use only the sections the diff touches. Each item is a question that has caught real
production bugs; a "no" is a candidate finding that still needs a concrete scenario.

## Database migrations and queries

- Does the migration lock a large table? Adding a column with a volatile default, adding
  `NOT NULL` without a default, changing a column type, or building an index without
  `CONCURRENTLY` (Postgres) / `ALGORITHM=INPLACE, LOCK=NONE` (MySQL) can block writes.
- Is the deploy order safe? New code must work with the old schema and old code with the
  new schema during a rolling deploy (expand, migrate, contract).
- Is there a down migration, or a stated reason why not?
- Are queries built with parameters, never string formatting?
- N+1: does a loop issue one query per item? Look for ORM lazy loads inside loops.
- Unbounded reads: `SELECT *` without `LIMIT` on a growing table, or loading a whole
  table into memory.
- Transactions: is the unit of work atomic? Any external call (HTTP, queue) inside a
  transaction that can hold locks for seconds?

## Authentication and authorisation

- Does every new route, resolver, RPC or job check who is calling and whether they may
  access this specific object (not just "is logged in")? IDOR is the common miss.
- Are permission checks on the server, not only hidden in the UI?
- Are tokens, passwords and keys kept out of logs, error messages and URLs?
- Are comparisons of secrets constant-time (`hmac.compare_digest`, `crypto.timingSafeEqual`)?
- Session or token lifetime changes: what happens to tokens already issued?

## Input handling

- Shell: any user-influenced string reaching `subprocess(..., shell=True)`, `exec`,
  backticks or `os.system`?
- Paths: user input joined into a filesystem path without normalising and checking it
  stays inside the intended directory?
- Redirects and URLs: open redirects; server-side fetches of user-supplied URLs (SSRF).
- Deserialisation of untrusted data (`pickle`, `yaml.load` without SafeLoader, Java
  native serialisation).
- HTML output: values inserted with `innerHTML`, `dangerouslySetInnerHTML`, `|safe`,
  `v-html` without sanitising.
- Size limits on request bodies, uploads, regexes on user input (catastrophic backtracking).

## Error handling and resilience

- Are errors swallowed (`except: pass`, empty `catch`, ignored Go `err`)?
- Do retries have a cap, backoff and jitter, and are retried operations idempotent?
- Does every network call have a timeout?
- Are resources closed on every path (`with`, `defer`, `try/finally`, `using`)?
- Do error messages returned to clients leak stack traces or internals?

## Concurrency

- Shared mutable state accessed from multiple threads, goroutines or async tasks without
  a lock or a single owner?
- Check-then-act races: "if not exists then create" without a unique constraint or lock.
- Async: un-awaited promises or coroutines; `forEach` with an async callback; blocking
  I/O inside an event loop.
- Go: goroutines without a way to stop (context, done channel); writes to a map from
  multiple goroutines; loop variable capture (fixed per-iteration since Go 1.22, but
  check the module's `go` directive).
- Caches: invalidation on write, stampede on expiry, unbounded growth.

## API and compatibility

- Removed or renamed fields, endpoints, CLI flags, env vars, config keys, events?
- Changed defaults or semantics of an existing parameter?
- New required fields on requests that old clients do not send?
- Enum additions that old consumers will not handle (exhaustive switches, strict parsers)?
- Versioning and changelog updated if the repo requires them?

## Tests

- Would the new test fail if the implementation were reverted? Look for assertions that
  only check "no exception" or that a mock was called.
- Are time, randomness and network controlled, or will the test flake?
- Are the edge cases that the code explicitly handles also tested (empty, null, max)?
- Were existing tests deleted or loosened? Ask why.

## Frontend (React and similar)

- Effects: missing or over-broad dependency arrays; effects that should be event
  handlers; missing cleanup for subscriptions and timers.
- Keys: array index as `key` on lists that reorder or filter.
- State derived from props copied into state and then stale.
- Accessibility: interactive elements that are not buttons or links, missing labels,
  focus not managed in dialogs.
- User-visible strings not passed through i18n when the app is localised.

## Language traps

### JavaScript / TypeScript
- `==` vs `===`; `||` defaults that swallow `0` or `""` (use `??`).
- `any`, non-null assertions (`!`) and `as` casts hiding real type errors.
- Floating-point money; `parseInt` without a radix on user input.
- Mutating function arguments or shared module-level objects.

### Python
- Mutable default arguments (`def f(x=[])`).
- Naive vs aware `datetime` mixed; `datetime.utcnow()` is deprecated since 3.12.
- Broad `except Exception` around large blocks.
- Iterating and mutating the same dict or list.

### Go
- Ignored errors; errors wrapped without `%w` so `errors.Is` fails.
- `defer` inside loops holding resources until the function returns.
- Nil map writes; nil interface vs typed nil pointer comparisons.
- Context not propagated into downstream calls.

### Java / Kotlin
- `equals` without `hashCode`; mutable objects as map keys.
- Unclosed streams or connections outside try-with-resources / `use`.
- `Optional.get()` without a presence check; platform types from Java in Kotlin.
- Blocking calls on reactive or coroutine dispatchers.

### SQL
- `NULL` semantics in `NOT IN` subqueries; `=` comparisons against NULL.
- Implicit casts that defeat indexes.
- Time zones in timestamp columns.

## Performance (only when the change is on a hot path)

- Work moved inside a loop that used to be outside it.
- New synchronous calls on request paths (network, disk, heavy CPU).
- Payload size changes: new fields serialised on every list item.
- Missing pagination on new list endpoints.