---
name: review-spec
description: >
  Review a spec file against best practices: API design, LLM code security, spec-driven
  development, test-driven development, security architecture, and mechanical test
  generation. Checks spec structure, requirement quality, security coverage, testability,
  and whether the spec enables mechanical test derivation. Invoke with the spec filename,
  e.g. /review-spec spec/authentication.md
allowed-tools: Read, Glob, Grep, Bash(cat *), Bash(ls *), Bash(find *)
---

# Spec Review Skill

You are reviewing the spec file **$ARGUMENTS** against established best practices.

## Pre-gathered context

### Best practices index
!`cat ~/dev/claude/BESTPRACTICES.md 2>/dev/null || echo "BESTPRACTICES.md not found"`

### Spec index (if exists)
!`cat SPEC.md 2>/dev/null || echo "No SPEC.md found"`

### Spec file to review
(Use the Read tool to read the spec file specified in $ARGUMENTS. If no filename is given, read SPEC.md and ask which spec to review.)

## Best practice files to load

Read ALL of the following core best practice files before starting the review:

1. `~/dev/claude/projects/best-practices/spec-driven-development.md` -- Spec structure, requirements, scenarios, writing guidelines
2. `~/dev/claude/projects/best-practices/test-driven-development.md` -- Test derivation, edge cases, property testing
3. `~/dev/claude/projects/best-practices/mechanical-test-generation.md` -- Spec properties that enable mechanical test writing (module layout, integration boundaries, error messages as test data, pattern tables as parametric matrices, pre-dispatch testability review)
4. `~/dev/claude/projects/best-practices/api-design.md` -- Transport security, auth, API patterns, input validation, zero-trust, RFC 9457 errors, contract testing
5. `~/dev/claude/projects/best-practices/llm-code-security.md` -- LLM-generated code vulnerabilities, review checklists
6. `~/dev/claude/projects/best-practices/security-architecture.md` -- Server boundary rule, credential proxying

### Conditional topic files

Read these ONLY when the spec under review touches that area. Skim the spec first, then load the matching files:

| Load when the spec involves... | File |
|---|---|
| Agent task dispatch, branch-per-task persistence, agent repo lifecycle | `agent-repos.md` |
| Parallel/wave dispatch, multi-agent orchestration, comparative dispatch | `ai-parallel-agents.md` |
| Third-party APIs (consumer side: capability verification, polling sync, SoR mapping) | `api-integration.md` |
| Credential handling beyond the security-architecture basics (SOPS, age, secret scoping, env vs file injection) | `secrets-management.md` |
| Persistence (DB selection, concurrent access, FQDNs/services backed by storage) | `database-selection.md` |
| Container lifecycle, image pinning, UID matching for mounted volumes | `docker.md`, `docker-uid-matching.md` |
| K8s resources, NetworkPolicy, securityContext, probe behaviour, Cilium/Istio | `kubernetes.md` |
| Pre-deploy validation, integration failure categorisation, migration safety | `validation.md` |

Skip for spec review (these are implementation-time concerns, not spec-quality concerns): `debugging`, `linting`, `scripting`, `documentation`, `milestones`, `networking`, `ansible`, `octopus-process-templates`, `git-source-control`, `python-patterns`, `helm`, `ci-container-builds`, `skills-development`. If the spec under review *is* about one of those areas, load the corresponding file then — but don't load them by default.

## Review: spec structure quality

Evaluate the spec against the required structure from spec-driven-development.md:

### Required sections

- [ ] **Overview** -- 2-3 sentences, clear purpose. An agent knows if this spec is relevant after reading this.
- [ ] **Responsibilities** -- What this subsystem owns AND what it delegates. Prevents scope creep.
- [ ] **Dependencies** -- Which other specs to read. Links present and correct.
- [ ] **Data Model** -- Types, schemas, state machines, interfaces with concrete examples (not just abstract schemas).
- [ ] **Requirements** -- Numbered with a consistent prefix (e.g., AU-1, CP-1). Each independently testable.
- [ ] **Scenarios** -- Given/when/then format. Cover happy path AND failure modes.

### Optional sections (flag if missing but relevant)

- [ ] **Interface** -- API surface, endpoints, signatures (required if the subsystem has an external API)
- [ ] **Extension Points** -- How to add capabilities without modifying existing code
- [ ] **Error Handling** -- Failure modes and expected behaviour (prevents agents inventing strategies)

## Review: requirement quality

For each numbered requirement, check:

- [ ] **Testable** -- Can an agent write a test that unambiguously passes or fails?
- [ ] **Unambiguous** -- No "should", "appropriate", "handle errors gracefully". Specific exit codes, status codes, timeouts.
- [ ] **Includes rationale** -- Why this requirement exists (the "Why:" line). Without it, agents follow mechanically and can't judge edge cases.
- [ ] **No duplicates** -- Same requirement doesn't appear under different numbers.
- [ ] **Complete coverage** -- Are there obvious behaviours that lack requirements?

Count the requirements and verify any summary counts in the spec are accurate.

## Review: security coverage

Check the spec against API design and security best practices:

### Authentication and authorization
- [ ] Auth model specified for every endpoint (who can call it, what credential, how validated)
- [ ] Service-to-service auth uses mTLS or short-lived tokens (not shared static keys)
- [ ] Human auth uses OIDC/OAuth2 with PKCE (not implicit flow, not password grant)
- [ ] Token validation is complete (signature, expiry, issuer, audience, algorithm pinned)

### Transport and data protection
- [ ] TLS required (or explicit justification for plaintext)
- [ ] Secrets never in payloads, URLs, query params, or logs
- [ ] Secrets passed via env vars or mounted files
- [ ] Error responses don't expose internals

### Input validation
- [ ] All external inputs validated (types, lengths, ranges, formats)
- [ ] Parameterized queries for database access (no string concatenation)
- [ ] Request size limits specified

### API patterns (if the spec defines an API)
- [ ] Pagination on list endpoints with enforced max page size
- [ ] Idempotency for POST endpoints
- [ ] Rate limiting mentioned or deferred with a reference
- [ ] Structured error responses with stable codes
- [ ] API versioning strategy

### Infrastructure security (if the spec involves K8s/containers)
- [ ] securityContext specified (runAsNonRoot, readOnlyRootFilesystem, drop ALL capabilities)
- [ ] Resource limits defined
- [ ] NetworkPolicy specified or referenced
- [ ] No privileged containers
- [ ] Images pinned to digest or specific version

## Review: testability

Evaluate how well this spec supports test-driven development:

- [ ] Every requirement maps to at least one testable assertion
- [ ] Scenarios cover both happy path and failure modes
- [ ] Edge cases identified (boundary values, empty inputs, concurrent access, timeout)
- [ ] Data model examples are concrete enough to use as test fixtures
- [ ] Extension points describe how to test new extensions

## Review: mechanical test derivation

Evaluate whether a tester (human or agent) could write the test suite from the spec without inventing structure. Source: `mechanical-test-generation.md`.

- [ ] **Module layout is explicit** -- A table maps subsystems → source files → test files (or the spec names them inline). Without this, testers invent module boundaries.
- [ ] **Integration boundaries are marked** -- Requirements that cross a process/network boundary are flagged as integration tests. Pure-logic requirements are flagged as unit tests. No ambiguity about which level a requirement belongs at.
- [ ] **Library/framework semantics are explicit** -- If the spec says "use X library", it states which behaviours the library guarantees vs which the spec adds on top. Testers should not have to read library source to know what to test.
- [ ] **Concrete interfaces over "implementation detail"** -- Function signatures, data shapes, and error types are spelled out. Avoid "the implementation handles this" hand-waves.
- [ ] **Error messages are test data** -- Where the spec specifies error responses, the exact string/code/structure is given (or referenced from a stable source). Tests can assert against these without guessing.
- [ ] **Pattern tables work as parametric matrices** -- Tables of inputs → expected outputs are formatted such that a `@pytest.mark.parametrize` can be derived directly. Avoid prose lists where a table would do.
- [ ] **Pre-dispatch testability review** -- Each requirement has been read with "could I write a failing test for this right now?" If not, the requirement is too vague to dispatch to an implementation agent.

## Output format

Structure your review as:

### Summary
One paragraph: overall quality of the spec and its readiness for implementation.

### Scorecard

| Area | Score | Notes |
|------|-------|-------|
| Structure completeness | 1-5 | ... |
| Requirement quality | 1-5 | ... |
| Security coverage | 1-5 | ... |
| Testability | 1-5 | ... |
| Mechanical test derivation | 1-5 | ... |
| Clarity for AI agents | 1-5 | ... |

(1 = major gaps, 3 = adequate, 5 = exemplary)

### Critical issues
Numbered list of problems that would cause implementation failures or security vulnerabilities. Each includes:
- The specific section/requirement with the issue
- What best practice it violates
- A concrete fix

### Missing requirements
Requirements that should exist but don't. Suggest a requirement ID and text for each.

### Missing scenarios
Scenarios that should exist but don't. Provide given/when/then for each.

### Recommendations
Non-blocking improvements that would strengthen the spec.

### What's done well
Specific sections or requirements that are exemplary -- reinforces good patterns for future specs.