Code Review Checklist

---
name: code-review-checklist
description: Reviews a code change across correctness, security, performance, readability and tests, then returns findings ranked from blocker to nit, each pinned to a file and line with a concrete fix and a verdict at the end. Use before merging a branch, after a multi-file edit, or when asked whether a change is ready to ship.
---

# Code Review Checklist

You are the reviewer who reads the diff for what it does rather than what its description claims. Findings are specific, ranked, and each one comes with the change that would resolve it.

## Workflow

1. **Read the stated intent** - the ticket, the pull request description, or the user's sentence. You cannot judge correctness without knowing what correct means.
2. **Read the whole diff once** without commenting, to see its shape and find the parts that carry the risk.
3. **Pass through each lens** below in order. Correctness and security first, because they are the ones that block.
4. **Read the surrounding code** for anything you flag. Half of apparent bugs are answered three lines above the diff.
5. **Run the tests if you can.** Say plainly if you cannot.
6. **Rank every finding** by severity and attach a concrete fix to each.
7. **Give a verdict** with one line of reasoning.

```text
Review checklist
[ ] Intent of the change understood before reading code
[ ] Whole diff read once before commenting
[ ] Correctness: boundaries, empty, null, error paths, concurrency
[ ] Security: untrusted input, authorisation, secrets, output encoding
[ ] Performance: loops, queries, unbounded growth, blocking calls
[ ] Readability: names, function size, hidden control flow, stale comments
[ ] Tests: new behaviour covered, edge cases covered, assertions meaningful
[ ] Surrounding code read for anything flagged
[ ] Tests run, or inability to run them stated
[ ] Every finding has a file, a line, and a fix
[ ] Verdict given with a reason
```

## The five lenses

### Correctness

Does it do what the description says, for every input it can receive?

- Boundaries: zero, one, empty collection, maximum, one past maximum.
- Null and missing values on every path, including the ones added by this change.
- Error paths: what happens when the call fails, and does the caller handle it.
- Operator and comparison mistakes: `<=` where `<` was meant, inverted conditions, assignment inside a condition.
- Concurrency: shared state, read-then-write races, assumptions that two requests cannot overlap.
- Assumptions about input shape that the change now depends on but does not check.

### Security

Trace untrusted input from where it enters to everywhere it lands.

- Input reaching a query, a shell command, a file path, a template, a redirect, or a deserialiser.
- Authorisation checked on the object, not just on the route. "Can this user act on *this* record?"
- Secrets in source, in logs, in error messages, in test fixtures.
- Output encoding for the context it lands in - HTML, attribute, URL, SQL, shell.
- New dependencies, new endpoints, and anything that widens what an unauthenticated caller can reach.

### Performance

Flag it where it plausibly matters, not everywhere it theoretically applies.

- Queries inside loops, the N+1 shape, work repeated per iteration that could be hoisted.
- Unbounded results: no limit, no pagination, loading a whole table to count it.
- Blocking calls on a request path, or in a lock.
- Data structures that turn a lookup into a scan.

### Readability

- Names that mislead, or that mean something different two files away.
- A function doing three jobs, especially when one of them is a side effect its name does not suggest.
- Control flow hidden in a place the reader will not look - a decorator, a signal handler, a global.
- Comments that describe code that no longer exists.
- Magic values that should be named, and named constants that are used for two different things.
- The test: could someone unfamiliar follow this in six months, at speed, at 5pm?

### Tests

- Is the new behaviour actually covered, or only the happy path?
- Do the assertions mean anything, or would they pass whatever the code did?
- Are the edge cases from the correctness pass represented?
- For a bugfix, is there a test that fails on the old code?
- Did anything get deleted, skipped, or loosened to make the suite pass?

## Severity

| Level | Means | Examples |
| --- | --- | --- |
| **Blocker** | Do not merge | Data loss, security hole, breaks a documented behaviour, silent failure |
| **Major** | Fix before merge, or file it with an owner | Wrong on a realistic edge case, missing test for risky logic, N+1 on a hot path |
| **Minor** | Worth fixing now, cheap | Confusing name, duplicated logic, missing error message detail |
| **Nit** | Optional | Formatting the linter does not catch, wording, personal preference |

Mark preferences as nits explicitly. A reviewer who blocks on taste gets ignored on the day they are right.

## Output format

Grouped by severity, highest first. Each finding:

```text
<file>:<line> - <what is wrong, one or two sentences>
Fix: <the concrete change>
```

End with:

```text
Verdict: Approve | Approve with changes | Request changes
<one line of reasoning>
Checked: <what you reviewed, and what you could not - e.g. "tests not run, no local database">
```

## Example

A three-line finding from a real review, showing the level of specificity to aim for:

```text
BLOCKER
api/reports.py:88 - The tenant id comes from the request body rather than the
session, so any authenticated user can read another tenant's reports by changing
one field. The route-level login check does not constrain which tenant.
Fix: take tenant_id from request.session.tenant_id and drop it from the payload
schema. Add a test that a user from tenant A gets 403 for a tenant B report id.

MAJOR
api/reports.py:104 - get_owner() runs inside the row loop, so a 500-row report
issues 501 queries. Median report is 2,000 rows.
Fix: fetch the owners in one query keyed by id before the loop and look up from
the dict.

NIT
api/reports.py:71 - `data2` does not say what it holds.
Fix: `filtered_rows`.
```

## Rules

- Point to a line. A review that says "error handling could be better" is not a review.
- Every finding carries a fix. If you cannot suggest one, say what you would need to know.
- Separate "this is wrong" from "I would have written it differently", and label the second as a nit.
- Review the code, never the author. No finding needs the word "you".
- If you did not run the tests, say so in the verdict rather than implying you did.
- Never approve blankly. If you found nothing, list what you checked so the author knows the depth of the review.
- Check the diff for things that are not there: the deleted test, the removed validation, the error case the change quietly stopped handling.
- If the change is too large to review honestly, say that, and review the part you can.

## Edge cases

- **Huge diff, mostly generated or moved.** Separate the generated and moved parts from the hand-written ones and review the latter properly. Say which parts you skimmed.
- **You cannot run anything.** Review statically and label the verdict accordingly. Do not turn "the tests look right" into "the tests pass".
- **The change is correct but the approach is wrong.** Say it once, clearly, at the top, with the cost of each path. Then review the code as written, because the decision may not be yours.
- **A pre-existing bug in the touched file.** Report it separately and do not block this change on it, unless the change makes it reachable or worse.
- **No tests exist anywhere in the project.** Asking for tests here is asking for a new practice, not a change. Note it, then focus on the correctness pass, which now carries all the weight.
- **You wrote the code yourself.** Review it anyway, with a bias toward the correctness and security lenses. Say that it was self-reviewed.
- **The author disagrees.** Blockers need evidence: the input that breaks it, the query count, the failing case. If you cannot produce evidence, it was a major, not a blocker.

## Files

- `references/examples.md` - two complete reviews of real diffs, with the diff shown and the finished review underneath: a Node API endpoint with an injection and an N+1, and a React form change where the bug was the missing case rather than the code that was written. Read it for how much evidence a blocker needs.

# Worked reviews

Two diffs, each followed by the review as it was written. The second one is included because its most important finding is about code that is not in the diff at all.

## Contents

- [Review 1: a search endpoint in Node](#review-1-a-search-endpoint-in-node)
- [Review 2: a React form change](#review-2-a-react-form-change)
- [What a good finding looks like](#what-a-good-finding-looks-like)

---

## Review 1: a search endpoint in Node

**Stated intent.** "Add an endpoint so admins can search orders by customer name
and export the results."

### The diff

```diff
--- a/api/routes/orders.js
+++ b/api/routes/orders.js
@@
+router.get('/orders/search', requireAuth, async (req, res) => {
+  const { name, limit } = req.query;
+
+  const sql = `SELECT * FROM orders WHERE customer_name LIKE '%${name}%'`;
+  const rows = await db.query(sql);
+
+  const results = [];
+  for (const row of rows) {
+    const customer = await db.query(
+      'SELECT id, email, tier FROM customers WHERE id = ?', [row.customer_id]
+    );
+    results.push({ ...row, customer: customer[0] });
+  }
+
+  res.json({ count: results.length, results: results.slice(0, limit || 100) });
+});
```

```diff
--- a/api/routes/__tests__/orders.test.js
+++ b/api/routes/__tests__/orders.test.js
@@
+test('search returns orders matching the customer name', async () => {
+  const res = await request(app).get('/orders/search?name=Ada').set(authHeader);
+  expect(res.status).toBe(200);
+  expect(res.body.results.length).toBeGreaterThan(0);
+});
```

### The review

```text
BLOCKER
api/routes/orders.js:4 - The query is built by string interpolation, so anything in
the `name` parameter becomes SQL. `?name=' OR '1'='1` returns every order;
`?name='; DROP TABLE orders; --` is available depending on the driver's multi-
statement setting. Verified locally: the first payload returned all 12,400 rows.
Fix: parameterise it -
  db.query('SELECT ... WHERE customer_name LIKE ?', [`%${name}%`])
and add a test asserting that a quote in the parameter returns zero rows rather
than everything.

BLOCKER
api/routes/orders.js:1 - requireAuth checks that someone is logged in, not that
they are an admin, and not which tenant they belong to. Any authenticated
customer can search every order in the system. The intent says "admins".
Fix: add the existing requireRole('admin') middleware, and scope the query by
req.session.tenantId. Test both: a non-admin gets 403, and an admin from tenant A
gets no rows belonging to tenant B.

MAJOR
api/routes/orders.js:8 - One query per row to fetch the customer. A search matching
2,000 orders issues 2,001 queries; at ~1.5ms each that is roughly three seconds of
database time per request.
Fix: join the customer columns into the original query, or fetch the distinct
customer ids in one `WHERE id IN (...)` and build a lookup map before the loop.

MAJOR
api/routes/orders.js:14 - The limit is applied after loading and enriching every
match, so `limit` reduces the response but not the work. `SELECT *` also pulls
every column including the notes blob.
Fix: push LIMIT into the SQL, validate it as an integer with a maximum (100 seems
intended), and select the columns the response actually uses.

MINOR
api/routes/orders.js:2 - `name` is not validated. An absent parameter produces
`LIKE '%undefined%'`, which silently returns nothing and looks like "no results"
rather than "you forgot a parameter".
Fix: return 400 when name is missing or shorter than two characters.

MINOR
api/routes/orders.js:13 - `count` reports the number of matches while `results`
holds at most 100 of them. A client showing "count" next to the list will display
2,000 above 100 rows.
Fix: return both, named clearly: `total` and `returned`.

TESTS
api/routes/__tests__/orders.test.js:1 - The single test asserts a 200 and a
non-empty array, which would pass with the injection, with the missing role check,
and with the wrong tenant's data. It does not test what the endpoint is for.
Fix: assert the specific orders returned for a known fixture; add the 403 case,
the cross-tenant case, the quote-in-parameter case, and the missing-parameter case.

Verdict: Request changes
Two blockers - the endpoint is injectable and it is not restricted to admins, so it
currently exposes every tenant's orders to any logged-in user.
Checked: read the full diff and the surrounding router, ran the suite (14 passed),
and reproduced the injection locally against the development database.
```

Note that the injection blocker carries the payload and the observed result. That is
what makes it a blocker rather than a concern - nobody has to take it on faith.

---

## Review 2: a React form change

**Stated intent.** "Fix the double-submit on the signup form."

### The diff

```diff
--- a/src/components/SignupForm.tsx
+++ b/src/components/SignupForm.tsx
@@
 export function SignupForm() {
   const [email, setEmail] = useState('');
+  const [submitting, setSubmitting] = useState(false);

   async function handleSubmit(e: FormEvent) {
     e.preventDefault();
+    setSubmitting(true);
     const res = await createAccount({ email });
+    setSubmitting(false);
     if (res.ok) navigate('/welcome');
   }

   return (
     <form onSubmit={handleSubmit}>
       <input value={email} onChange={(e) => setEmail(e.target.value)} />
-      <button type="submit">Create account</button>
+      <button type="submit" disabled={submitting}>Create account</button>
     </form>
   );
 }
```

### The review

```text
MAJOR
src/components/SignupForm.tsx:9 - If createAccount throws, setSubmitting(false)
never runs and the button stays disabled forever. The user's only recovery is a
page reload, and this is the failure path they are most likely to hit - a network
drop mid-signup.
Fix: wrap in try/finally, with the reset in the finally block. Add a test where
createAccount rejects and assert the button is enabled afterwards.

MAJOR
src/components/SignupForm.tsx:11 - The failure case has no user-visible outcome at
all. When res.ok is false the form does nothing: no message, no error state. The
user clicks, waits, and sees the same form. That is the bug they will report next.
Fix: set an error message from the response and render it above the button. This is
arguably outside the stated intent, but the change makes the silence more visible
by disabling the button during the wait.

MINOR
src/components/SignupForm.tsx:7 - Disabling on submit prevents the second click but
not the second submit: pressing Enter in the input fires the form again during the
same tick, before the state update commits.
Fix: guard the handler itself - `if (submitting) return;` as the first line after
preventDefault. The disabled attribute stays as the visual signal.

NIT
src/components/SignupForm.tsx:7 - The button label does not change while the
request is in flight. "Creating..." would make the wait legible.
Fix: optional; label from the submitting state.

Verdict: Approve with changes
The mechanism is right and the two majors are small. The try/finally one matters
most because it converts a transient network failure into a stuck form.
Checked: read the diff and the createAccount client, which does throw on a network
error rather than returning ok:false - that is the path in the first finding. Ran
the component tests, 7 passed. No test covers the rejection path.
```

The two most valuable findings here are about code that is absent: the missing
`finally`, and the missing error display. Reviewing only the added lines would have
found neither. Reading `createAccount` is what turned the first finding from a
theory into a fact.

---

## What a good finding looks like

1. It names a file and a line, so nobody has to search.
2. It says what goes wrong, in terms of what a user or an attacker would experience - not "this is unsafe" but "any logged-in user can read another tenant's orders".
3. It carries evidence when it blocks: the payload, the query count, the reproduction. A blocker without evidence is a strongly-worded major.
4. It ends with a fix specific enough to apply, including the test that should come with it.
5. It is honest about scope. The error-display finding in review 2 says out loud that it goes beyond the stated intent, and explains why the change makes it worse.
6. The verdict says what was actually checked, including what could not be. "Tests not run, no local database" is worth more than silence.