spec-driven-review

---
name: spec-driven-review
description: Review the code quality of a spec-driven change. Checks readability, security, performance, and best practices before archiving.
license: Apache-2.0
---

You are reviewing the code quality of a completed spec-driven change.

## This Skill's Commands

If you cannot remember the exact command used by this skill, look it up here
before running anything. Do not guess.

```yaml
modify: node {{SKILL_DIR}}/scripts/spec-driven.js modify
apply: node {{SKILL_DIR}}/scripts/spec-driven.js apply <name>
audit-spec-mapping-coverage: node {{SKILL_DIR}}/scripts/spec-driven.js audit-spec-mapping-coverage <spec-path> [--implementation <repo-path> ...] [--tests <repo-path> ...]
audit-unmapped-spec-evidence: node {{SKILL_DIR}}/scripts/spec-driven.js audit-unmapped-spec-evidence [--implementation <repo-path> ...] [--tests <repo-path> ...]
```

## Prerequisites

The `.spec-driven/` directory must exist at the **project root**. Before proceeding, verify:
```
ls .spec-driven/
```
If this fails, the project is not initialized. Run `/spec-driven-init` first.

## Steps

1. **Select the change** — run `node {{SKILL_DIR}}/scripts/spec-driven.js modify` to list active changes. Ask which change to review. If already specified, use it.

2. **Confirm readiness** — run:
   ```
   node {{SKILL_DIR}}/scripts/spec-driven.js apply <name>
   ```
   If `remaining > 0`, stop — the change is not ready for review. Suggest `/spec-driven-apply` first.

3. **Load context** — read:
   - `.spec-driven/changes/<name>/proposal.md` — scope and unchanged behavior
   - `.spec-driven/changes/<name>/specs/` — delta specs describing the intended behavior changes
   - `.spec-driven/changes/<name>/design.md` — approach and decisions
   - `.spec-driven/changes/<name>/tasks.md` — what was implemented
    - `.spec-driven/changes/<name>/questions.md` — resolved answers that may explain decisions or tradeoffs
    - `.spec-driven/config.yaml` — project context and rules, including `test` rules and any `fileMatch` entries
    - mapping frontmatter from relevant main and delta spec files

4. **Identify changed files** — from the completed tasks and mapping
   frontmatter, determine which files were created or modified. Read each file
   fully, including mapped implementation and test files for the relevant spec files.

5. **Identify specialized review checklists** — before issuing findings,
   classify the change using evidence from the proposal, delta specs, design,
   tasks, questions, changed files, mapping frontmatter, and repository context.
   Apply every checklist that fits; checklist routing is additive and never
   replaces the baseline review checks. Treat reviewed code as author-agnostic:
   do not trust or soften review because the code may have been generated by AI,
   previously produced by you, or written by a human.

   Supported checklist types:
   - **Security-sensitive**: changes involving authentication, authorization,
     permissions, secrets, tokens, input boundaries, data exposure, dependency
     trust, or safe failure behavior.
   - **UI**: changes involving user-visible screens, layout, interaction state,
     accessibility, keyboard behavior, responsive behavior, or visual
     regressions.
   - **DX**: changes involving commands, prompts, generated artifacts,
     documentation, operator messages, error messages, setup, or local workflow
     ergonomics.
   - **Migration**: changes involving data transformation, filesystem or schema
     transitions, backwards compatibility, idempotency, rollback, or partial
     failure behavior.
   - **API**: changes involving public or internal contracts, command
     arguments, JSON shapes, error semantics, validation behavior, versioning,
     or caller impact.
   - **Maintenance**: changes involving scheduled or manual maintenance,
     dependency upkeep, generated files, repository hygiene, repeatability, or
     avoiding unrelated churn.

6. **Review code quality** — for each changed file, run the baseline review
   checks plus any specialized checklist checks identified in Step 5:
   - **Impartiality**: judge the code that exists, not who wrote it; do not
     suppress, soften, or skip findings for AI-authored or self-authored code
   - **Readability**: clear naming, reasonable function length, no unnecessary complexity
   - **Security**: no injection vulnerabilities, no hardcoded secrets, proper input validation at system boundaries
   - **Error handling**: appropriate error handling for external calls and user input; no swallowed errors
   - **Performance**: no obvious N+1 queries, unnecessary allocations, or blocking calls in async contexts
   - **Best practices**: follows the project's conventions (from config.yaml context), no dead code, no debug artifacts left behind
   - **Security-sensitive checklist**: verify authorization boundaries, secret
     handling, injection resistance, data exposure, validation, and safe failure
     behavior.
   - **UI checklist**: verify key interactions, accessibility, layout stability,
     state transitions, responsive behavior, and user-visible regression
     coverage.
   - **DX checklist**: verify command ergonomics, documentation or prompt
     clarity, actionable errors, setup compatibility, and consistency with the
     existing workflow.
   - **Migration checklist**: verify idempotency, backward compatibility,
     rollback or recovery expectations, partial failure handling, and data
     preservation.
   - **API checklist**: verify contract compatibility, validation, error
     semantics, versioning or migration expectations, and caller impact.
   - **Maintenance checklist**: verify repeatability, dependency or generated
     artifact safety, repository hygiene, and absence of unrelated churn.

7. **Check test quality** — read the test files associated with this change:
     - Do tests cover the key scenarios from the delta specs?
     - Are tests independent and repeatable?
     - Do tests follow `rules.test` from config.yaml?
     - Are `mapping.tests` entries current and useful for future verification?
     - Do tests cover the relevant specialized checklist risks when those risks
       are observable and testable?

8. **Audit mapping quality** — for each touched spec file relevant to this
   change:
   - Compare `mapping.implementation` and `mapping.tests` against the change's primary implementation files and directly verifying test files
   - Use the smallest confident evidence set from changed files, delta specs, completed tasks, and mapped files already read
   - Run `node {{SKILL_DIR}}/scripts/spec-driven.js audit-spec-mapping-coverage <spec-path> [--implementation <repo-path> ...] [--tests <repo-path> ...]` when it helps make the comparison explicit
   - Run `node {{SKILL_DIR}}/scripts/spec-driven.js audit-unmapped-spec-evidence [--implementation <repo-path> ...] [--tests <repo-path> ...]` when it helps confirm whether reviewed implementation or test files are missing from all main-spec mappings
   - Report stale or misleading mappings as at least SHOULD FIX
   - Escalate to MUST FIX when the mismatch would materially mislead future verification or archive readiness
   - If the unmapped audit shows that a primary implementation file or directly verifying test file for this reviewed change is missing from all main-spec mappings, treat that as MUST FIX
   - If the unmapped audit only finds files outside this review scope or weakly related candidates, report them separately as SHOULD FIX or repository debt

9. **Output a review report**:
    ```
    MUST FIX (blocks archive):
      - [list or "none"]

   SHOULD FIX (recommended):
     - [list or "none"]

   NITS (optional):
     - [list or "none"]
   ```

10. **Recommend next step**:
    - If MUST FIX issues: address them before archiving
    - If only SHOULD FIX: ask user if they want to address them or proceed
    - If clean: suggest `/spec-driven-archive <name>`

## Rules
- Read every changed file before commenting on it — never review code you haven't read
- Focus on real issues, not style preferences already handled by linters
- Review code based on evidence in the code and tests, never on authorship or
  assumed intent
- MUST FIX = security vulnerabilities, data loss risks, broken functionality
- SHOULD FIX = maintainability concerns, missing error handling, unclear logic
- NITS = naming suggestions, minor simplifications, documentation gaps
- Do not re-review code that was not changed by this change
- Respect config.yaml rules — violations of project rules are SHOULD FIX at minimum
- Report stale or misleading spec mappings when they would make future
  verification unreliable
- Specialized checklist findings use the same severity model as the baseline
  review: MUST FIX for archive blockers, SHOULD FIX for recommended fixes, and
  NITS for optional cleanup
- Never give self-authored or AI-authored code lighter treatment than
  human-authored code