code-review

name	code-review
description	Self-review your diff or branch-vs-base before pushing — catch correctness, data-flow, concurrency, and consistency bugs and surface test gaps. Trigger to review your own changes pre-PR.
effort	high
user-invocable	true

Code Review — self-review before you push

You are reviewing a set of changes before they are pushed, to catch the bugs yourself instead of leaving them for a reviewer to find later. The bug classes below are the ones that actually slip through on NestJS/TypeORM/BullMQ backends and React/Next.js frontends, ordered roughly by how often they bite.

The one principle that catches most of these: re-derive what the code does from the code itself — ignoring what you meant it to do. Every bug in this catalog survived to review because the author read the happy path and trusted their own intent. Read the diff like a hostile stranger who has never seen the ticket.

This skill applies to your own diff. For reviewing someone else's GitHub PR with collaborative inline comments, use the github-pr-review skill — they share the same correctness lens but differ in voice and output.

---

0. Capture the intent first — then establish what changed

Start with why this change exists, not what it does. A reviewer who only reads the diff will flag deliberate behavior changes as bugs — a removed guard, a relaxed validation, a changed default, a new "clear this field" path all look like defects until you know they were the point. An AI PR reviewer always has the PR description to anchor on; your review may run with no PR at all, so you must reconstruct that intent yourself and make it explicit.

Establish the intent, in this order — stop at the first that's clear:

1. What the user asked you to review / the task they're doing.
2. The PR title + description, if a PR exists (gh pr view).
3. The commit messages on the branch, and the branch name / linked ticket ID.
4. The diff + surrounding code — derive what the change is trying to accomplish.
5. If the intent is still ambiguous and a finding hinges on it, ask. "Did you mean to let X through now, or should it still be blocked?" is cheaper than a wrong verdict.

Write the intent down as a one-line premise at the top of the review ("This change intends to …"). Every finding is then judged against it (see §4). This cuts both ways:

• It suppresses false positives — behavior that matches the intent is not a bug, even if it removed a guard or changed a default.
• It sharpens real bugs — once the intent is "X now allows Y," the connected code that still assumes "X never allows Y" is exactly what to surface (the flip side of Pass A). An intentional change that wasn't propagated to every connected data point is the single richest source of real bugs.
• Beware intent stated in a comment. A // we only reach this on a deliberate re-gather comment is a claim, not proof — confirm the code actually only runs in the case the comment assumes before trusting it.

Pick the review target:

Situation	Diff command
Uncommitted work	git diff and git diff --staged
Branch vs its base	git diff $(git merge-base <base> HEAD)...HEAD
A specific PR	gh pr diff <number> --repo <owner/repo>

Detect the base branch — don't assume main. Try in order:

1. An explicit base the user gave you.
2. The PR base, if a PR exists: gh pr view --json baseRefName -q .baseRefName.
3. The repo default: git symbolic-ref refs/remotes/origin/HEAD.
4. Common bases to check for: develop, staging, main, master. Many teams cut feature branches from develop, not main.

Then, for every changed file, read beyond the hunk. This is the single biggest determinant of catching real bugs — most of them live in the relationship between the diff and code that isn't in the diff:

• the full function/class you changed, and its imports;
• the matching symbol — the reader for a write, the consumer for a producer, the sibling handler, the other side of a guard;
• how the surrounding module already does the same kind of thing.

Identify each file's layer (service, queue consumer, migration, controller/route, React component, hook, infra/config) — it tells you which passes below matter.

---

1. The review bar — apply to every change

Hold every change to this standard. These are cleanups, not blockers, but they compound:

• Least duplication. Before accepting new code, look for an existing helper, hook, service, or component that already does it. If logic is copy-pasted across two+ places, or a reusable unit could be created and shared, extract it. Custom code that reimplements an already-imported utility is a defect.
• Consistency with existing patterns. New code must match how the surrounding module already does naming, error handling, query building, folder layout, validation. An inconsistent-but-working change is still worth fixing — divergence is how the next bug hides.
• DRY & SRP. One function/class/file, one responsibility. Business logic belongs in services, not controllers. Domain-specific logic belongs in the domain module, not a generic/shared service.
• Clean, readable code. Clear names that reveal intent without reading the body; early returns over deep if/try nesting; comments only where the why is non-obvious. Not every line needs a comment, and a comment that just restates the code is noise.

The bar matters, but the correctness passes below are where the costly bugs are — spend most of the review there.

---

2. Correctness passes — run these explicitly, in order of yield

Don't free-associate. Walk the passes. Each says what to inspect and why it's missed.

Pass A — Trace two things together (highest yield, by far)

Most real bugs are invisible reading one symbol; they only appear when you read both ends of a boundary in the same sitting. Whenever a value crosses a boundary, open both ends and confirm the names, nesting, shape, units, and ordering agree:

• Producer ↔ consumer of a payload. Queue queue.add(name, payload) ↔ the @Process(name) / onFailed that handles it; a webhook/SDK response ↔ your reader; a JSONB/metadata column ↔ the code that reads it. Real failures: a webhook reader expecting a key at the top level when the provider nests it under data.customer.… → every event silently early-returns; a writer storing metadata.resources[] (array) while the reader reads metadata.resourceId (singular) → the feature does nothing, with no type error.
• Writer ↔ every reader. When you change a shape (rename a key, singular→array, change a column type), grep for every reader/consumer of the old form and update each. A refactor that changes a query hook's data source must move every cache invalidation and selector with it. And if a writer enriches a shape (adds assignees, a computed field), find every reader that still loads the thin row — one that reaches for the new field will crash.
• Sibling handlers of one event family. When you add a side-effect (enqueue / emit / field-write) to one branch of a switch, one provider, or one subscription event, every sibling must do the same or be deliberately exempt. Real failure: subscription.deleted and invoice.paid push a billing sync but the re-subscribe branch returns early without it → stale state.
• A guard duplicated across files, or FE ↔ BE. Fix every copy. Real failure: an authz check switched to an allowlist in a helper while a denylist copy (role === 'member') was left in the service → a newly added viewer role passes the admin check. When the same guard is needed on many routes/reads, fix it at the shared chokepoint (a route guard / resolver) rather than each call site.
• Escape ↔ unescape, decode ↔ encode. Must be paired on every path. Applying an escape in a shared component but wiring the inverse into only some save handlers leaks the escaped form to the backend. Decoding for display while comparing against the raw value breaks dirty-checking (field looks always-dirty → double-encoding). Same for base64 vs base64url — a value encoded one way must be decoded the same way, or a shared callback silently corrupts one provider's payload.
• "set flag" ↔ "reset flag". Every path that sets an in-flight / reconnecting / processing flag needs a guaranteed reset on every exit.
• A read that trusts persistence as proof of liveness. When a row caches synced/derived data (budget, hours, roster, roles), a read that filters only by parentId serves a stale snapshot after a disconnect, a reconnect-before-reselect, or a switch of the selected account/resource. The read must assert the row's source stamp equals the currently-active integration id and the currently-selected external resource id — enumerate those lifecycle events.
• Sibling call sites after a library/SDK upgrade. A version bump can change how one call site must invoke an API; every other call site is a boundary. A missed one often fails silently (falls into an existing catch and degrades) rather than crashing.
• A boolean derived from !!someId. When "is it published/linked/done?" keys off one specific id, confirm no legitimate row reaches the same logical state via a different field or a null — otherwise you re-do the action (re-publish, re-create) on rows that were already there.

Heuristic: for each changed boundary, literally name the two symbols and confirm the contract. If you can't see both ends, you haven't finished the review.

Pass B — Run it twice (retry & idempotency: queues, jobs, webhooks)

For any queue handler, job, webhook, cron, or retried operation:

• What happens on the 2nd run? The first line should re-check terminal state and no-op (if (already completed/tagged) return). Non-idempotent external work — model calls, billing/metering, sending mail — must be guarded against redelivery, or it double-charges.
• @OnQueueFailed / onFailed fires on every attempt, not just the last. Guard terminal-status writes on job.attemptsMade >= (job.opts.attempts ?? 1). Marking an entity failed on attempt 1 can block a successful attempt 2 (e.g. a state-machine that won't transition failed → processing).
• A "failed" status that unlocks a user-facing retry must only be set when BullMQ retries are truly exhausted — otherwise the manual retry runs in parallel with the still-pending automatic retry.
• Don't gate a side-effect on a field you wrote earlier in the same handler — a retry re-reads the row you already mutated, so the guard sees stale state and skips needed work. Gate on immutable event-payload data; order writes so the re-entry guard still holds if a later line throws.
• A job over a collection (ids[]) must handle failure/terminal status for the whole collection, not just the primary id.
• A distributed lock has a lifecycle. If the guarded work can run longer than the lock's TTL, renew it with a heartbeat; if you renew, register an onLost that aborts the in-flight work (a lock that silently expires mid-run lets a second worker overlap and both write the same rows); release it on every exit path.
• A terminal / onFailed handler must only touch state owned by the thing that actually failed. If it clears shared state (a cache key, a status) it will wipe good data when a later step fails after the real work already succeeded — move the cleanup into the catch of the operation it belongs to.
• Non-refundable side-effects (billing, metering, emails) need an idempotency marker (e.g. a billedAt column) gating the call — not just a terminal-status check — or a retry that re-enters after the charge succeeded double-charges.
• A batched upsert's conflict clause must encode the same rule as any pre-read decision. If a find decides "skip manually-set rows" but the following upsert runs ON CONFLICT DO UPDATE, a concurrent writer in the read→write window is clobbered — split into update() for known rows + insert().orIgnore().

Pass C — Failure-path & partial-progress state

Trace what every external call (network, DB, queue, storage) leaves behind on failure:

• Status flipped to in-flight (processing) before an enqueue/external call → if that call throws, the row is stranded with no job and no UI path out. Set the status after the guards, or revert it in a catch.
• Rollback/cleanup that deletes the only surviving copy. If the move/update already happened, the "undo" can destroy the last copy. Ask: at the point this catch runs, is this still the only copy?
• A status/flag written inside a transaction that later throws is rolled back. To persist an error status, write it in a separate committed statement.
• Non-atomic multi-write. For a sequence of 2+ writes, trace a failure of write N: what do writes 1..N-1 leave? Wrap in a transaction or make the compensating cleanup actually correct. Promise.all of writes is concurrent, not atomic.
• Optimistic UI — every setState(x) before a mutation needs an onError revert and an in-flight guard against double-fire. A global error toast notifies the user but does not fix the now-divergent control.

Pass D — Clear vs leave-unchanged (nullable fields & whole-set writes)

• TypeORM .update() / partial saves silently drop undefined props — to NULL a column you must pass null. value ?? undefined leaves the stale value in place; if upstream cleared the field, you've kept old data.
• When an upstream value is removed/emptied, decide explicitly: clear it (write null) or intentionally leave it. Don't drift into stale-by-accident.
• ?? only guards null/undefined, not '' or 0. If an upstream string can legitimately be empty (a title, a name), value ?? fallback leaks the empty string past the fallback — use || when an empty/zero value should fall back too.
• Writes that replace a whole set (PUT the entire array of labels/owners/tags) wipe values added elsewhere. A transient fetch failure that yields [] must not clear server data — omit the field when you have nothing authoritative rather than sending an empty array. Watch for empties manufactured by a response helper too: an HTTP-200 body missing its collection key coerced to [] will drive a reconcile to delete every row.

Pass E — Loading defaults, cache staleness, hook hygiene (frontend)

• data?.x ?? <default> from an async query — what does the UI do during the loading window when the default is wrong ('free', 0, unchecked)? Gate on isLoading, render a state-agnostic view, or use a lazy useState(() => …) initializer so the correct value is there on first paint.
• Stale cache after a mutation — invalidate every query whose result derives from the data you changed, not just the entity's own key (counts, badges, previews built from a different endpoint are the ones that go stale). After a refactor, move invalidations to the new key.
• Hook hygiene — no whole mutation/query objects in dependency arrays (destructure the stable mutateAsync); an interval/effect must not depend on the value it mutates (it churns); watch for an effect that overwrites the user's intended initial state on mount.
• Don't seed state from a one-shot effect when the source hydrates late. "Set this filter once when me/settings arrive, then lock it with a ref" races every later refetch/hydrate (a workspace switch leaves a stale id; a cached response without settings locks the wrong default). Derive the value during render instead of seeding-once-and-locking.
• State owned by a component that can unmount must reconcile with a server marker on remount. Polling or a completion toast living in a collapsible panel dies when the panel unmounts; on remount, read a server-side run marker and resume/refetch rather than assuming local state survived.

Pass F — Boundary, empty/null & type-coercion inputs

• Empty array → IN () is invalid SQL — guard at the top of the query.
• bigint/numeric columns come back from the driver as strings — "0" === 0 is false, so === 0 / === -1 "unlimited/free" branches never fire and arithmetic yields NaN. Wrap in Number() before comparing or computing.
• array[0] destructure of a possibly-empty array; string methods on a maybe-non-string (?. guards nullish, not type — a CLI flag declared optional can arrive as boolean true, and true.trim() throws).
• Dates for persisted/external fields — .utc() before .format('YYYY-MM-DD'), or the value shifts a day for anyone not in the server's timezone.
• External API responses — check res.ok; inspect provider-specific error shapes (some always return HTTP 200 with body.error); don't treat a structured error as success; don't assume the field is an array (Array.isArray). A refactor that moves a fetch out of its try/catch to add 401-handling can silently drop network-error resilience.

Pass G — SQL / ORM / migration correctness (backend)

• Hand-written SQL identifiers for camelCase columns MUST be double-quoted ("deletedAt"), or Postgres folds them to lowercase → "column does not exist" at runtime (often after a destructive statement already committed).
• A new NOT-NULL-semantics or gating column must backfill existing rows in the migration — a new emailVerifiedAt that login gates on will lock out every pre-existing user.
• A new UNIQUE / partial index — can current production data or an existing concurrent code path already violate it? Run the migration mentally against dirty data and on a fresh DB.
• .clear() (TRUNCATE) fails on FK-referenced tables even when empty → use .delete({}).
• Migration up/down identifier quoting must match the column definition exactly.
• Changing a hard delete to a soft delete drops the FK ON DELETE CASCADE. Children that relied on the cascade must now be soft-deleted explicitly — which turns one delete into a non-atomic multi-write, so wrap it in a transaction (Pass C) or a mid-sequence failure leaves an active parent with soft-deleted children.

Pass H — Wiring: DI, modules, routes, jobs (NestJS)

• Added/removed an @InjectRepository(X) or constructor dep → confirm X is in the module's forFeature/providers and in every *.spec.ts testing module (a missing provider fails the whole spec file at compile).
• New controller/module/route → imported in the parent module.
• New queue job name → a matching @Process('<name>') exists somewhere, or the job never runs.
• New middleware/guard on a base path → a wildcard entry covers nested sub-routes (strict path matching means :id won't match :id/children).

Pass I — Security & authz

• A removed/relaxed auth check must be replaced server-side (guard/strategy). A client-side redirect is not authorization.
• Authz is an allowlist (role === 'admin' || role === 'owner'), never a denylist — adding a new role silently passes a denylist check.
• Every token-issuing path (register / login / refresh / SSO) must enforce the same gates — enumerate them and check each; don't fix login and leave register open.
• Nested resource :parentId/:childId — assert the child belongs to the parent before read/mutate/delete (IDOR). findById(childId) alone is a cross-tenant hole.
• Webhook signature verification must fail closed — reject when no secret is configured; never return true.
• dangerouslySetInnerHTML / innerHTML from user content must be sanitized (DOMPurify) — markdown renderers do not sanitize. Decode & last, never decode entities after sanitizing (it re-opens injection).
• Redirect targets from query params validated to same-origin relative (startsWith('/') && !startsWith('//')).
• Config/secrets/infra — an env var used in stage logic must actually be injected by the deploy config (don't assume); verify money units (cents vs dollars), IAM ARNs (cross-region inference needs both foundation-model and inference-profile ARNs), and that no "local debugging" config got committed.

Pass J — Refactor regressions (DRY done wrong)

When you extract repeated markup/logic into a shared component, loop, or helper: diff each original instance against the shared template for a dropped attribute, a per-instance class, an ordering change, or a prop/branch some caller still depends on. Make divergent bits props rather than hardcoding; never hardcode a class that collides with a caller's override.

---

3. Test-gap analysis — always do this pass

• If the change touches a *.spec/test file, run it. The most common test-file bug is a renamed method the spec still calls, or a missing DI mock — either of which fails the whole file. A test that passes for the wrong reason (asserting a plain space where it meant a non-breaking space) is worse than none.
• Changes that must ship with a test (each is where a real bug shipped without one):
  • Classification predicates over plan / role / trial / paying / status — one case per enum value and edge, especially "$0 but paid" and trial states.
  • Queue/job handlers — retry, redelivery no-op, failure/rollback, whole-collection handling.
  • Concurrency — lock acquire/release, contention, heartbeat/TTL renewal.
  • Pure transforms with boundary inputs — empty array, null, out-of-order, decode∘encode round-trip, "edit nothing → Save stays disabled".
  • Constants/units, ORM type-coercion, raw-SQL identifiers, escape/unescape ordering — the cheapest bugs to lock down with a single assertion.
  • Date logic — assert in a non-UTC timezone.
  • The reuse/update path, not just the create path — when a create path gains a field, the "row already exists" branch must apply it too.
  • Empty-string / falsy fallbacks — the exact ''/0 input that ?? would leak past a fallback.
  • Provider-error → friendly-message translation — e.g. a provider 404 surfaces as a not-found, not a 500.
  • Merge/union passes — a second match row clobbering the first, a feature-toggle-off path still doing its cleanup, and an empty-corpus input.
  • Distributed-lock renewal and loss — the heartbeat-renew path and the onLost/expiry path.
  • Integration lifecycle resolves to not-live — disconnect / reconnect-before-select / switch-resource yield the correct not-connected/needs-selection state, not stale numbers.
• A test that wouldn't fail if the behavior regressed is a gap, not coverage. Note missing cases as 🔵 findings.

---

4. Precision filter — verify before you flag

Some findings look like bugs but aren't — raising them is just noise. Before surfacing a finding, rule out these false-positive patterns — and state the evidence you checked, so a real issue is defensible and a non-issue is dropped:

• Library/API behavior asserted from memory → verify against the installed version (an SDK property rename between majors, an ORM method's documented behavior, a base-image package's flags). Check package.json/lockfile or the docs, not your training data. (E.g. class-validator's @IsOptional() skips both null and undefined, not just undefined — a common mis-claim.)
• A codebase convention/wrapper you haven't read → does the rest of the app already do it this way and work? A custom API client may reject with a transformed shape; a constructor may be wrapped; a query builder may legally start with andWhere.
• "TypeScript won't compile" → confirm with tsc --noEmit. UMD globals (using React.X without an import), contravariant param arity (a (a) => void is assignable to (a, b) => void), and single dynamic-segment router.query.id being a string are all legal.
• A "missing guard" where the invariant is established upstream — a value only set when a parent is present, a subscription ordered before the code that needs it. Trace the caller before adding a guard that could introduce a race.
• Domain/data semantics — the "safer default" can regress a legitimate case (a null field that's valid in production; a plan seeded isFree: false). Confirm the premise against the factory/seed/enum.
• Behavior that matches the captured intent (§0) — a removed guard, a relaxed check, or a new "clear this field" path is not a bug if it is the change. Flag only deviations from the stated intent and issues this diff introduced; where a comment asserts intent, confirm the code only runs in the case the comment assumes.
• Nondeterministic ordering — an unordered query is only a bug if the order is observed across two separate reads. When one in-memory snapshot drives both the encode and the decode (build labels, then resolve them against the same array), internal consistency holds regardless of order.
• Stale snapshot — re-anchor to the current code; the issue may already be fixed in a later commit, or the whole pattern replaced.

---

5. Output

Open with the intent premise you captured in §0 ("This change intends to …") so your assumptions are visible and the user can correct a wrong read before trusting the findings. Then a one-line verdict: is it safe to push?

Then group findings by severity:

	Severity	What goes here
🔴	Blocker	Correctness, security, or data-loss bug on a reachable path
🟠	Should-fix	Real bug, lower likelihood / narrower path
🟡	Cleanup	DRY / SRP / consistency / readability (Section 1)
🔵	Test gap	A change that needs a test it doesn't have

For each finding give: file:line · which pass/class it is · a one-line mechanism (what breaks, on which path) · the concrete fix. For a "trace two things" finding, name both symbols that disagree.

• Default to a local report. If asked to --comment and a PR exists, post inline using the collaborative voice from the github-pr-review skill (Can we…?, evidence, no commands).
• Don't pad. A clean diff gets a short verdict plus the one or two things you specifically checked and cleared — never invent issues to hit a quota.

---

Anti-patterns — never do these

• Don't review only the hunk. Read the surrounding code and the matching symbol — that's where the bug is.
• Don't trust your own intent. Re-derive behavior from the code.
• Don't flag formatter/linter noise or anything the framework already handles.
• Don't assert library behavior without checking the installed version.
• Don't pad the report, and don't post essays inline — file:line + mechanism + fix.