diff --git a/skills/review-spec/SKILL.md b/skills/review-spec/SKILL.md index 8f97d6d..a71881a 100644 --- a/skills/review-spec/SKILL.md +++ b/skills/review-spec/SKILL.md @@ -2,8 +2,9 @@ name: review-spec description: > Review a spec file against best practices: API design, LLM code security, spec-driven - development, test-driven development, and security architecture. Checks spec structure, - requirement quality, security coverage, and testability. Invoke with the spec filename, + 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 *) --- @@ -25,15 +26,31 @@ You are reviewing the spec file **$ARGUMENTS** against established best practice ## Best practice files to load -Read ALL of the following best practice files before starting the review: +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/api-design.md` -- Transport security, auth, API patterns, input validation, zero-trust -4. `~/dev/claude/projects/best-practices/llm-code-security.md` -- LLM-generated code vulnerabilities, review checklists -5. `~/dev/claude/projects/best-practices/security-architecture.md` -- Server boundary rule, credential proxying +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 -Also read any additional best practice files relevant to the spec's technology stack (check the index for Kubernetes, Helm, Docker, secrets management, etc.). +### 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 @@ -111,6 +128,18 @@ Evaluate how well this spec supports test-driven development: - [ ] 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: @@ -126,6 +155,7 @@ One paragraph: overall quality of the spec and its readiness for implementation. | 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)