Spec Review

---
name: spec-review
description: 'Review a diff on the two axes a dev-flow project can check that a generic reviewer cannot: the **spec** it was built from (`.workflow/PRD.md` + `tasks.md`) and the **contract** it was built under (golden rules, `meta.json#stack`). Two parallel sub-agents, reported side by side and never merged, with a Fowler smell baseline as the floor. Use when the user says "review this branch", "spec review", "review since <ref>", "does this match the PRD?", "controlla il diff contro la spec", or dev-flow proposes it after a feature lands. Requires `.workflow/`; without one use the built-in `/code-review`. Records `meta.json#spec_review`; no phase bump. Not for: generic review with no spec (built-in `/code-review`), UI quality (`shadscan`), legal (`compliance-audit`), Vercel cost (`vercel-doctor`), or running tests (`write-tests`).'
---

# spec-review — did we build what the PRD asked, the way the contract says?

Two-axis review of the diff between `HEAD` and a fixed point:

- **Spec** — does the change faithfully implement what `.workflow/PRD.md` / `tasks.md` asked for?
- **Standards** — does it obey the contract this project was built under?

Both run as **parallel sub-agents** so neither pollutes the other's context, then this skill reports
them side by side.

> **Credit.** The two-axis structure, the parallel-sub-agent split, the refusal to merge verdicts and
> the Fowler smell baseline are adapted from Matt Pocock's `code-review` skill
> ([mattpocock/skills](https://github.com/mattpocock/skills), MIT — the skill now lives at
> [`docs/engineering/code-review.md`](https://github.com/mattpocock/skills/blob/main/docs/engineering/code-review.md);
> both paths checked 2026-08-26) and
> [the essay behind it](https://www.aihero.dev/skills-code-review). What's ours is the half his skill
> has to search for: in a dev-flow project the spec and the standards are **at known paths**.

## Why this is not `/code-review`

Claude Code ships a built-in `/code-review`, and Pocock's own write-up lists the name collision as a
known problem. This skill is deliberately **not** that, in name or in remit:

| | Built-in `/code-review` | `spec-review` |
|---|---|---|
| Reviews | the code, generically | the code **against this project's spec and contract** |
| Knows | the diff | `.workflow/PRD.md`, `tasks.md`, `meta.json#stack`, `#artifacts`, `#history` |
| Answers | "is this good code?" | "is this **the thing we said we'd build**, built **our way**?" |

Run both if you like — they don't overlap. **Without a `.workflow/`, this skill refuses** and points
at the built-in: with no spec and no declared stack, both of its axes are guesswork.

## Workflow

### 1. Pin the fixed point — and fail here, not inside a sub-agent

Whatever the user names is the fixed point (a SHA, `main`, a tag, `HEAD~5`). If they didn't say, ask.

```bash
git rev-parse <fixed-point>                    # must resolve
git diff <fixed-point>...HEAD --stat           # three-dot: compare against the merge-base
git log  <fixed-point>..HEAD --oneline         # the commits under review
```

**A bad ref or an empty diff must fail right here** — discovering it inside two parallel sub-agents
wastes both. Three dots, not two: you want the merge-base, not everything that landed on `main`
meanwhile. And **the diff excludes uncommitted work**, so commit (or stash) before asking for a review,
or the change you most want looked at is the one nobody sees.

### 2. Resolve the Spec axis — deterministic, not a search

This is where the contract earns its keep. In order:

1. **`.workflow/tasks.md`** — the tasks this branch claims to close. Match by branch name, by the task
   ids in the commit messages, or ask which ones are in scope.
2. **`.workflow/PRD.md`** — the requirement behind those tasks; the tasks are the decomposition, the PRD
   is the intent. A diff can satisfy every task and still miss what the PRD asked for.
3. **`meta.json#artifacts`** — what was produced and by which skill, so the reviewer knows whether a file
   is hand-written or generated (holding generated scaffolding to hand-written standards is noise).
4. **Projects run with `scrum`** — the issue on the tracker is the live spec; `meta.json#tracker` names the tracker and maps each task to its issue.

If `.workflow/PRD.md` is absent (a project adopted mid-flight), say so and run the Spec axis against
`tasks.md` alone, or skip it and report that the axis was not run — **never invent the spec from the
diff**. A review that infers the requirements from the code it is reviewing always passes.

### 3. Resolve the Standards axis — the contract first, Fowler as the floor

A dev-flow project's documented standards are not a `CONTRIBUTING.md` someone may have written. They are:

| Source | What it binds |
|---|---|
| **Golden rules** (`references/contracts.md`) | ① identifiers, constants and comments in **English**; ② **i18n from day one** — no hardcoded user-facing copy, `en` + `it` minimum; ③ **UI only from the declared library's primitives** — checked on the diff as below |
| **`meta.json#stack`** | the declared choices: `forms` (every form through `lib/forms/`), `ui` / `ui_base`, `i18n`, `db`, `deploy` — a diff that reaches for a different library than the one declared is a finding |
| **The discipline skills** | `state-discipline` (`useState` is the last resort), `data-fetching` (Server Components first; Server Actions never for reads), `transitions` (every transition ships a `prefers-reduced-motion` path), `composition-patterns-guide` (no boolean-prop pile-ups) |
| **The repo's own docs** | `CONTRIBUTING.md`, `CODING_STANDARDS.md`, `AGENTS.md`/`CLAUDE.md` if present — **these override everything above where they conflict**, including this skill |
| **Fowler baseline** | the floor, below — applies even when nothing above says anything |

**Skip anything tooling already enforces.** `biome`/`eslint`, `tsc`, and the five pre-deploy gates
(`shadscan`, `vercel-doctor`, `compliance-audit`, `launch-audit`) have their own runs; repeating them here produces a
long report that says nothing new. If the project has a lint gate, a review finding about formatting is
noise.

#### Golden rule 3 on the diff — four checks

The design-system lint already refuses the import half (imports of an undeclared component library,
imports of `radix-ui` / `@base-ui/react` / `cmdk` / `vaul`… outside `components/ui/`) — don't repeat it.
**A green lint is not evidence for the rest**: a raw element imports nothing, and on smart-travel
(2026-10-12) 238 hand-made elements sat behind a green lint and screenshots that matched the boards.
The Standards sub-agent checks:

1. **A file outside `components/ui/` that re-implements a pattern an installed primitive covers.** List
   `components/ui/` (shadcn monorepo: `packages/ui/src/components/`) first, then read each new or changed
   component for the pattern it renders: a positioned floating panel is `Popover`/`Tooltip`/`DropdownMenu`,
   an overlay with a focus trap is `Dialog`/`Sheet`, a filterable list is `Combobox`/`Command`, a
   pulsing placeholder is `Skeleton`, a "nothing here" block is `Empty`, a tab strip is `Tabs`.
2. **Imports from a component library `meta.json#stack` does not declare** — including one the lint does
   not know about yet (the lint list is finite; the rule is not).
3. **Behaviour changes inside `components/ui/`** beyond `cva` variants and token classes — new props that
   change markup, handlers, state, focus or ARIA wiring. That is a fork of the primitive.
4. **Hand-made UI: raw elements and classes that redraw a primitive** («the primitive renders the design»). A violation
   with no import at all. Run the lint rule on the files of the diff — it reports raw `<button>`, `<label>`,
   `<input>`, `<select>`, `<textarea>`, `<hr>`, `<details>`, `<summary>`, `<dialog>`, `<progress>`,
   `<meter>`, `<table>`, `<img>`, `<a>`, hand-written roles (`tab`, `radio`, `menu`, `switch`…), and
   `div`/`span` classes that redraw a Card, Badge, Alert or Separator, each with the primitive to use:

   ```bash
   git diff --name-only --diff-filter=AM <base>... -- '*.tsx' '*.jsx' \
     | xargs pnpm exec eslint --no-warn-ignored --rule '{"design/no-handmade-ui":"error"}' 2>&1 \
     | grep -E 'design/no-(handmade-ui|card-in-card)'
   ```

   Without the rule in the project (a preset older than 2026-10-12 — itself a finding: the retrofit is
   `design-md-to-app/scripts/setup_design_lint.py <root>`), fall back to a grep on the **added lines**:

   ```bash
   git diff <base>... -U0 -- '*.tsx' '*.jsx' | grep -nE \
     '^\+.*(<(button|label|input|select|textarea|hr|details|summary|dialog|progress|meter|table|img|a)[ >/]|role="(tab|tablist|radio|radiogroup|checkbox|switch|menu|menuitem|listbox|option|dialog|tooltip|progressbar|separator)"|className="[^"]*rounded-(lg|xl|2xl|3xl)[^"]*\b(border|ring-1)\b)'
   ```

   Then drop the allowed cases by reading each hit: an element passed as a primitive's `render` (or under
   `asChild`), an `<a>` styled by `buttonVariants` (download, `tel:`, `mailto:`), a hidden or file input,
   an `sr-only` table, a file that renders no DOM (`next/og`, react-email, react-pdf), a file in the
   preset's `designExceptions`. **"The board looked different" and "the primitive has no variant for it"
   are not exceptions** — both are fixed in the primitive: its `cva` variant tuned to the board, a new
   variant recorded first in DESIGN.md. Never by correcting the board, never by classes at the call site.
   Report a missing variant as a finding *and* name the variant to add. Also report, under the same check,
   call-site classes that override a primitive's own look (radius, ring/border, selected or hover
   background, weight, padding) — the look belongs in the variant — and, when the diff converts an existing
   screen, the absence of a before/after pixel diff (`design-md-to-app/references/primitives-first.md`
   §Converting existing screens). Also read the diff and the agent's report for the words
   that give it away («no variant», «wrong on hover», «custom because») — the smart-travel evidence was
   in exactly such a comment and such a report.

Each is a finding **unless** `meta.json#stack_config.primitive_exceptions` (or an ADR in `docs/adr/`)
records that component with a reason — then it is not a finding, and say that you checked.

Example finding, in the report's shape:

> **Standards · golden rule 3 · high** — `app/(app)/bookings/_components/guest-picker.tsx:12-88` renders
> an absolutely positioned list under an `<input>` with its own open state, outside-click handler and
> arrow-key navigation. `components/ui/combobox.tsx` is installed and covers this pattern (filtering,
> keyboard navigation, portal, ARIA). No entry in `stack_config.primitive_exceptions`. → Replace with
> `Combobox`; the guest-specific row becomes the item renderer.

> **Standards · golden rule 3 · high** — `components/checkout/review-form.tsx:41` writes
> `<button className="text-sm font-medium underline">Modifica</button>` and `:58`
> `<span className="rounded-full bg-success-soft px-2 text-xs">Pagato</span>`; `design/no-handmade-ui`
> reports both (hand-written `<button>`; classes that redraw a badge). Neither is passed as a `render`, and
> no exception is recorded. The comment at `:40` says the ghost Button «turns grey on hover, wrong here» —
> the primitive renders the design: tune it, not the call site. → `Button variant="link"` (its hover tuned in
> `button.tsx` if the board wants it) and `Badge variant="success"`; if `badge.tsx` has no success variant,
> add it — DESIGN.md first, then the `cva` — not a span.

#### The smell baseline — twelve smells, selected (Fowler, *Refactoring* 2nd ed., ch. 3)

The chapter catalogues more than these; the twelve below are the ones that show up in a *diff*, which is
all this skill ever sees. Each is a **labelled heuristic** — "possible Feature Envy" — never a hard
violation, and **any documented standard above overrides it**.

- **Mysterious Name** — a name that doesn't reveal what it does or holds. → rename; if no honest name comes, the design is murky.
- **Duplicated Code** — the same logic shape in more than one hunk. → extract, call from both.
- **Feature Envy** — a method reaching into another object's data more than its own. → move it onto the data it envies.
- **Data Clumps** — the same few params always travelling together. → one type, passed once.
- **Primitive Obsession** — a string or number standing in for a domain concept. → give the concept its own small type.
- **Repeated Switches** — the same cascade on the same type recurring. → polymorphism, or one shared map.
- **Shotgun Surgery** — one logical change forcing scattered edits. → gather what changes together.
- **Divergent Change** — one module edited for several unrelated reasons. → split it.
- **Speculative Generality** — abstraction for needs the spec doesn't have. → delete it. (This one pairs with the Spec axis: it is usually scope creep wearing a design hat.)
- **Message Chains** — long `a.b().c().d()` the caller shouldn't depend on. → hide the walk.
- **Middle Man** — a unit that mostly delegates onward. → cut it.
- **Refused Bequest** — a subclass ignoring most of what it inherits. → composition instead.

### 4. Spawn both sub-agents, in parallel, and forbid further delegation

Each gets the diff command, the commit list, its own sources **pasted in full** (a sub-agent has no
access to what you read), and an explicit **"do not spawn further agents"** — Pocock's write-up records
recursive spawning as a real failure mode when the brief doesn't forbid it.

**Spec brief** — *"Report: (a) requirements the spec asked for that are missing or partial; (b) behaviour
in the diff nobody asked for (scope creep); (c) requirements that look implemented but where the
implementation looks wrong. **Quote the PRD or task line for each finding.** Under 400 words."*

**Standards brief** — *"Report, per file/hunk: (a) every place the diff breaks a documented standard —
cite it (which rule, which file); (b) any baseline smell — name it and quote the hunk. Documented
breaches can be hard findings; baseline smells are always judgement calls; a documented standard
overrides the baseline. Skip anything lint/tsc/the gates enforce. Under 400 words."*

### 5. Report side by side — and do not merge

Present under `## Spec` and `## Standards`, verbatim or lightly cleaned. **Never merge or re-rank across
axes.** Close with the count per axis and the worst issue *within each*.

That separation is the whole point: a change can follow every convention and implement the wrong
feature (**Standards pass, Spec fail**), or do exactly what the PRD asked while ignoring the stack the
project declared (**Spec pass, Standards fail**). One combined verdict lets either hide behind the other.

**Findings are hypotheses.** Verify before acting. This repo's own `[VERIFY]` discipline exists for the
same reason, and the 2026-08-26 sweep proved the point: of ~55 markers closed across the skill set, a
handful turned out to be **confidently wrong** rather than merely unchecked — a removed module path
still documented as current, a dead source-of-truth link, a package namespace that does not exist, an
upstream issue described as open that had been closed *won't fix*. A report that reads well is not a
report that is right.

## `meta.json#spec_review` block

```jsonc
"spec_review": {
  "last_run_at": "<ISO>",
  "fixed_point": "main",              // what HEAD was compared against
  "commits": 7,
  "spec_sources": ["tasks.md#T12-T15", "PRD.md#3.2"],
  "spec_findings": { "missing": 1, "scope_creep": 2, "wrong": 0 },
  "standards_findings": { "hard": 1, "judgement": 4 },
  "axes_not_run": []                  // e.g. ["spec"] when there is no PRD — say so, never infer it
}
```

## dev-flow hook

Horizontal capability — run it on any diff. dev-flow proposes it **when a chunk of work lands — a page,
a module, a PR, a batch of parallel agent branches — whatever phase `meta.json` records** (a phase left
behind the code must not switch the review off; smart-travel built for three days at `design_extracted`
and this skill was never proposed), rather than only before shipping. In a parallel build the orchestrator
runs the Standards axis on **every** agent diff before accepting it: a spec finding is cheapest while
the branch is still open. It is *not* a sixth pre-deploy gate — the five gates (`compliance-audit`, `vercel-doctor`, `shadscan`,
`launch-audit`, `app-security`) read the finished artefact, this reads the **change**. Records `meta.json#spec_review` +
`history`; **never bumps `phase`**; never blocks anything. Every `scope_creep` finding is routed to a choice the user makes, never the reviewer: change the PRD (the request passes `prd-from-idea/references/scope-check.md` and is recorded) or remove the code. A change whose behaviour nobody proved on the
running app is proved with the project's verification manual (`.claude/skills/verify-<app>/`, from
`app-verification`), not by reading the diff harder.

## What this skill does NOT do

- **Not generic code review** — no `.workflow/`, no run; use the built-in `/code-review`.
- **Doesn't run tests, lint or typecheck** — it reads their results and skips what they cover.
- **Doesn't fix anything.** It reports. Fixes go to the owning skill (`forms`, `data-fetching`, …).
- **Doesn't merge the two axes**, and doesn't pick a winner between them.

## Reference files

- `references/contracts.md` — the `.workflow/` dev-flow contract (vendored).

## Sources

- <https://www.aihero.dev/skills-code-review> · [mattpocock/skills](https://github.com/mattpocock/skills) (MIT) — the two-axis structure and the smell baseline. Both 200 on 2026-08-26; upstream has since moved its skills under `docs/engineering/`, where `code-review.md` still lives.
- Fowler, *Refactoring* (2nd ed.), ch. 3 — the smell catalogue.