# Code Review

> 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.

- Skill: `crowdlinker/code-review` (Agent Skill, multi-file: 2 files)
- Install (CLI): `npx skillmds@latest add crowdlinker/code-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/crowdlinker/code-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: crowdlinker (https://skillmd.com/u/crowdlinker)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/crowdlinker/code-review

---


# 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 `&amp;` **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.