Code review

A useful review finds the problems that matter and nothing else. Every finding is verified, ranked by how much it matters, and comes with a suggested fix.

Steps

1. Understand the intent first. Read the request, the plan, or the description: what is this change supposed to do, and for whom? Judge the diff against that — not against what you would have built.
2. Get the whole change. git diff main...HEAD (or the draft's diff), plus the files it calls into. Note what's new: endpoints, tables, dependencies, permissions, configuration.
3. Check correctness. Does it do what was asked, in every case? Walk the edges: empty and maximum inputs, errors and timeouts, concurrent requests, partial failures, retries. Look for off-by-one errors, inverted conditions, unhandled promises, and state updated in one place but not another.
4. Check security wherever the change touches input or access. Every new entry point authenticates and checks the caller may touch that specific record; input that reaches SQL, a shell, HTML, file paths, or outbound URLs is handled safely; no secrets in code or logs. The Security audit skill goes deeper.
5. Check data and compatibility. Migrations are additive and reversible; nothing renames or drops a column that running code still reads; APIs and file formats stay compatible with existing clients.
6. Check the tests. Each behaviour the change promises has a test that would fail if it broke — permissions and money above all. Tests assert behaviour, not source text.
7. Check that it's as simple as it can be. Logic that duplicates something that already exists, dead code, abstraction with one user, misleading names, comments that restate the code. Deleting code beats adding it.
8. Verify every finding before you report it. Read the full path — the guard you think is missing may live in a caller or a helper. Run the code or a test when you can. If you couldn't confirm it, report it as a question, not a bug.

Rank findings

• Blocker — wrong behaviour, a security hole, data loss, or a broken build. Fix before it ships.
• Should fix — a real problem with limited impact, such as a missing test for important behaviour or an unhandled error path.
• Consider — simplicity, naming, structure. Optional.
• Question — something you couldn't confirm. Ask; don't assert.

Leave formatting and style to the linter and formatter.

Done when

• You can say what the change does, and whether it does it
• Every new entry point, migration, and dependency has been examined
• Every blocker and should-fix is verified, with its exact location and a suggested fix
• Nothing you only suspect is reported as fact

Report back

A one-line verdict — ship it, ship it after these fixes, or don't ship it — then the findings, most severe first: severity · file:line · what's wrong · why it matters · suggested fix. Finish with what you checked and found sound, so the person knows what was covered.

Traps

• Commenting on style while missing a logic bug.
• Burying the one blocker under twenty minor comments.
• Reporting a "missing" check that exists one call away.
• Approving because the tests pass, without checking that they cover the change.