---
name: code-review-checklist
description: A structured checklist for the reviewer role — correctness, safety, simplicity, testability, docs — with explicit approve/block markers.
when_to_use: You are a reviewer role or performing a review pass on any code diff.
tags: [foundation, review]
---

# Code review checklist

Work top-down. A block on any earlier item stops the pass — don't score everything before opining.

## 1. Correctness (BLOCKS)

- Does the change match the PLANNER's acceptance criteria for this INT-XX?
- Do the tests exercise the behavior described in the plan, not just the code that was written?
- Any edge cases the plan named but the code doesn't handle? (empty input, boundary values, timezones, unicode)
- Any silent error paths (`unwrap_or_default()`, `.ok()?`) that hide real failures?

## 2. Safety (BLOCKS)

- New `unsafe` blocks — is the justification comment concrete about the invariants?
- New FFI — is the C-side contract checked at every call site?
- SQL — parameterized every user input? No string concatenation into queries?
- HTTP — validates auth, checks workspace scope on every read?

## 3. Simplicity (usually NIT, sometimes BLOCK)

- Any code that a smaller function would replace? (`Iterator::sum`, `.map`, `.filter`)
- Any abstraction added for one caller? BLOCK — introduce when second caller lands.
- Any dead code, commented-out blocks, `// TODO fix later` without a ticket? BLOCK.
- Files > 1500 LOC after the change? Split.

## 4. Performance (BLOCK on hot paths, NIT elsewhere)

- Cold path (init, config load, admin endpoints): NIT.
- Hot path (per-request handler, per-frame render, tight loop): BLOCK on obvious O(n²), unnecessary allocations, blocking sync in async context.

## 5. Testability / observability (BLOCK if downstream on-call would suffer)

- Does the change add meaningful log lines at decision points? (Info at boundaries, warn on retries, error on give-up.)
- Are new metrics wired? Prometheus counter/histogram naming convention followed?

## 6. Docs (NIT if module-private, BLOCK if public API)

- New public function / struct / route → doc comment with example.
- Non-obvious invariant → comment on the declaration.

## Verdict markers

Emit exactly one of these on a line by itself when the pass is done:

```
REVIEW_APPROVE: INT-NN
```

or

```
REVIEW_BLOCK: INT-NN — <specific issue that must be fixed>
```

Multi-line commentary is fine, but the marker must appear literally.