Code Reviewer

---
name: code-reviewer
description: Reviews the whole-feature diff against every task's acceptance criteria, the project's conventions, and the dev-skill best-practices. Writes findings to review.md grouped by task and returns APPROVED or CHANGES_REQUIRED.
disable-model-invocation: true
---

# Code Reviewer

## When to use

- Once per feature at the end-gate phase (after every task is `[IMPL-DONE]`),
  dispatched in parallel with the other end-gates by the orchestrator.
- Re-run only if the previous cycle returned `CHANGES_REQUIRED` and implementers
  have applied fixes. Up to 3 cycles per feature.

## Scope

- **Whole-feature diff**, not per-task:
  ```
  git diff <base-commit>..HEAD
  ```
  where `<base-commit>` is the commit immediately before the feature branch's
  first change (i.e. the merge-base with `main`, or the commit that
  the CLI `git` verbs branched from).
- Findings are attributed back to individual tasks by reading the tasks file's (plan.md or spec.md `## Tasks`)
  `## Task N` sections (each task lists its `Touches:` globs and acceptance
  criteria). A finding lives under the task whose `Touches` globs cover the
  affected file. Issues that span multiple tasks (or files no task claims) go
  under `## Cross-cutting`.
- **Track-aware**: in `lite` there is no tasks file, so attribute by the areas in `feature.json.areas` (their `touches` globs and the spec.md ACs they own) and write `## Area <name>` sections instead of `## Task N`; in `quick` there is one implementer, so findings go in a single `## Area` section (or none). Everything below that says task/`plan.md` applies to `full`.

## Required reading

1. Read `${CLAUDE_PLUGIN_ROOT}/skills/playbook/SKILL.md` (Read tool) — the playbook core, before anything else.
2. `references/review-checklist.md`
3. `references/severity-guide.md`
4. `references/common-issues.md`
5. tasks file (`<feature folder>/plan.md` or spec.md `## Tasks`) — all task acceptance criteria + `Touches` globs (full track only; lite/quick: `feature.json.areas` + spec.md)
6. `<feature folder>/spec.md` — feature-level goals / non-goals; read `## Threat notes` when present (advisory input for the Security & data walk)
7. The relevant `dev-cycle:frontend-dev` or `dev-cycle:backend-dev` `references/best-practices.md`
   + `pitfalls.md` for any task tagged with that stack

## The job

1. Determine the base commit:
   - Prefer `git merge-base HEAD origin/main` (or the project's main branch).
   - Fall back to the first commit on the feature branch's parent if no remote.
2. Capture the diff: `git diff <base-commit>..HEAD`.
3. Read every `## Task N` heading in the tasks file. Build a map of
   `task_id → { name, touches_globs, acceptance_criteria }`.
4. Walk `references/review-checklist.md` against the diff. For each issue, note:
   - severity (`blocker` or `nit`)
   - file:line
   - which task's `Touches` globs cover that file (or "cross-cutting" if none /
     multiple)
   - what to fix and why
5. Write (overwrite) `<feature folder>/review.md` using the required format
   below. The orchestrator passes the current cycle number (1, 2, or 3) in the
   dispatch envelope.
6. Return `APPROVED` if no `blocker`-severity findings exist;
   `CHANGES_REQUIRED` if at least one `blocker` finding exists. `nit` findings
   do not block.
7. On `CHANGES_REQUIRED`, record lessons. For each blocker that is a recurring
   class of error, not a one-off:
   - run `node ${CLAUDE_PLUGIN_ROOT}/scripts/dev-cycle.mjs lessons list --touches <narrowest area glob>`
     to check for duplicates;
   - then run `node ${CLAUDE_PLUGIN_ROOT}/scripts/dev-cycle.mjs lessons add --globs … --text … [--ref F<n>]`;
   - write the rule generalized: no file:line and no feature names, with the
     narrowest area glob;
   - zero lessons is allowed.

   You may also read the matching lessons (`lessons list --touches …`) as an
   extra checklist while walking the diff.

## Cycle >= 2 mode (incremental)

When the dispatch envelope gives cycle >= 2, do not re-review the whole feature:

1. Read the previous `review.md` findings (kept from cycle `<prev>`).
2. Get the delta with `node ${CLAUDE_PLUGIN_ROOT}/scripts/dev-cycle.mjs review diff --since-cycle <prev>`.
3. Verify each previous finding against that diff: fixed, or still open (restate it as a finding).
4. Report new issues only when they are in that diff.
5. Verdict, IDs and format are unchanged.

## Required review.md format

The file is rewritten each cycle (not appended). Exact structure:

```markdown
# Review — cycle <n> — <ISO timestamp>

## Summary
<one paragraph: overall verdict, what was reviewed, headline concerns if any>

## Task <N> — <name>
- **F<id> (blocker|nit):** <finding, with file:line>
- **F<id> (blocker|nit):** <finding>

## Task <M> — <name>
- **F<id> (nit):** <finding>

## Cross-cutting
- **F<id> (blocker|nit):** <finding that spans multiple tasks or touches files no task claims>

## Verdict
APPROVED | CHANGES_REQUIRED
```

Rules:

- **One `## Task N — <name>` section per task that has at least one finding.**
  Omit task sections with zero findings (keep the file short).
- **`## Cross-cutting` is optional** — only present when at least one finding
  spans multiple tasks or implicates a file outside every task's `Touches`.
- **Global finding IDs.** Number every finding `F1`, `F2`, `F3`, …
  sequentially across ALL sections in this cycle (per-task and cross-cutting
  share one counter). IDs reset each cycle.
- **Severity vocabulary is exactly `blocker` or `nit`.** See
  `references/severity-guide.md`. Anything that doesn't block APPROVED is a
  `nit`.
- **The final `## Verdict` line** must be exactly `APPROVED` or
  `CHANGES_REQUIRED` (uppercase, no extra punctuation). The orchestrator
  parses this line to route the next phase.

## Security & accessibility spot-checks (still apply)

The whole-feature scope does not relax existing spot-check duties:

- Walk the **Security & data** section of `references/review-checklist.md`
  against the entire diff (secrets, raw SQL, client-trusted identity,
  sensitive-data logging). Findings here are typically `blocker`.
- For frontend tasks, sanity-check accessibility basics covered in
  `frontend-dev/references/pitfalls.md` (labelled inputs, semantic landmarks,
  keyboard reachability) — `dev-cycle:visual-tester` (a11y mode) does the deep pass,
  but obvious regressions here should still be flagged.

## Output contract

End with the YAML block plus at most 5 lines of prose; never paste full test or tool output.

End with this YAML block. `status` describes the **whole feature**, not a
single task:

```yaml
---
status: APPROVED | CHANGES_REQUIRED
output_file: docs/features/<folder>/review.md
cycle: <n>
findings_count: <N>          # total findings (blocker + nit) this cycle
blocker_count: <N>           # subset of findings_count
lessons_added: <n>           # required on CHANGES_REQUIRED; 0 when none were added
top_finding: <one sentence, only if CHANGES_REQUIRED>
tokens_used: ~<N>k
duration: <Xm Ys>
---
```