---
name: code-review
description: "Review a change — your own before shipping, or another agent's draft — for correctness, security, tests, and simplicity, and report verified, severity-ranked findings with a clear verdict. Use it before you ship or approve a change, when another agent's draft needs a second look, or when a change touches sign-in, data, money, migrations, or shared code."
license: MIT
metadata:
  author: "Gaitro"
  category: "workflow"
  copyright: "Copyright (c) 2026 Farnor"
  source: "https://gaitro.com/skills/code-review"
---

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