diff --git a/CLAUDE.md b/CLAUDE.md index baa0812..96744dd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -21,7 +21,9 @@ custom-claude-skills/ ├── log/SKILL.md # End-of-session logging ├── orchestrate/SKILL.md # Container agent dispatch with git worktrees ├── reflect/SKILL.md # Milestone reflection - └── reflect-logs/SKILL.md # Process logs into memory + ├── reflect-logs/SKILL.md # Process logs into memory + ├── review-plan/SKILL.md # Review plan against best practices + └── review-spec/SKILL.md # Review spec against best practices ``` ## Conventions diff --git a/skills/review-plan/SKILL.md b/skills/review-plan/SKILL.md new file mode 100644 index 0000000..c312bd1 --- /dev/null +++ b/skills/review-plan/SKILL.md @@ -0,0 +1,123 @@ +--- +name: review-plan +description: > + Review a plan file against best practices: API design, LLM code security, spec-driven + development, test-driven development, and security architecture. Flags gaps, missing + considerations, and anti-patterns before implementation begins. Invoke with the plan + filename, e.g. /review-plan M2-auth-PLAN.md +allowed-tools: Read, Glob, Grep, Bash(cat *), Bash(ls *), Bash(find *) +--- + +# Plan Review Skill + +You are reviewing the plan 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"` + +### Plan file to review +(Use the Read tool to read the plan file specified in $ARGUMENTS. If no filename is given, look for *-PLAN.md files in the project root and ask which one to review.) + +## Best practice files to load + +Read ALL of the following best practice files before starting the review: + +1. `~/dev/claude/projects/best-practices/api-design.md` -- Transport security, auth, API patterns, input validation, zero-trust +2. `~/dev/claude/projects/best-practices/llm-code-security.md` -- LLM-generated code vulnerabilities, review checklists +3. `~/dev/claude/projects/best-practices/spec-driven-development.md` -- Spec structure, requirements, scenarios +4. `~/dev/claude/projects/best-practices/test-driven-development.md` -- Test derivation, edge cases, property testing +5. `~/dev/claude/projects/best-practices/security-architecture.md` -- Server boundary rule, credential proxying + +Also read any additional best practice files relevant to the plan's technology stack (check the index for Kubernetes, Helm, Docker, secrets management, etc.). + +## Review checklist + +Evaluate the plan against each area below. For each area, report one of: +- **Covered** -- the plan explicitly addresses this +- **Partially covered** -- mentioned but lacks detail or has gaps +- **Missing** -- not addressed and should be +- **N/A** -- not relevant to this plan + +### 1. Security by design + +- [ ] Authentication model defined (who authenticates, how, what protocol) +- [ ] Authorization model defined (who can do what, how enforced) +- [ ] Transport security specified (TLS, mTLS, or explicit justification for plaintext) +- [ ] Secrets handling defined (how injected, never in payloads/URLs/logs, rotation plan) +- [ ] No credentials crossing server boundary to clients (server boundary rule) +- [ ] Input validation strategy at system boundaries +- [ ] Error responses don't leak internals (stack traces, paths, SQL) + +### 2. API design (if the plan involves APIs) + +- [ ] API versioning strategy +- [ ] Pagination on list endpoints +- [ ] Idempotency for state-changing operations +- [ ] Rate limiting considered +- [ ] Structured error responses with stable codes +- [ ] Health checks split into liveness and readiness +- [ ] Request size limits +- [ ] CORS policy (explicit origins, not wildcard) + +### 3. LLM code security awareness + +- [ ] Plan acknowledges that LLM-generated code needs security review +- [ ] Input validation is planned at all external boundaries (not deferred) +- [ ] Dependency versions will be verified from live sources (not LLM memory) +- [ ] Infrastructure manifests include security defaults (securityContext, NetworkPolicy, resource limits) +- [ ] No over-permissive defaults (0.0.0.0 binding, CORS *, chmod 777, verify=False) +- [ ] Secrets never hardcoded -- plan specifies how they're injected + +### 4. Spec-driven development + +- [ ] Plan references or will produce specs before implementation +- [ ] Subsystem boundaries identified (what gets its own spec) +- [ ] Data models and interfaces described (not just "we'll figure it out") +- [ ] Requirements are testable and unambiguous +- [ ] Scenarios included or planned (given/when/then) +- [ ] Spec dependencies mapped (which specs need to be read together) + +### 5. Test-driven development + +- [ ] Test strategy defined (what's tested, how, what tools) +- [ ] Tests derived from spec requirements (requirement IDs in test names) +- [ ] Edge cases and failure modes considered (not just happy path) +- [ ] Integration test plan (not just unit tests) +- [ ] Verification script planned for milestone completion + +### 6. Operational readiness + +- [ ] Logging and observability considered +- [ ] Deployment strategy (how it gets deployed, rollback plan) +- [ ] Configuration via environment variables (not baked in) +- [ ] Health checks and monitoring +- [ ] Backward compatibility considered (existing consumers) + +## Output format + +Structure your review as: + +### Summary +One paragraph: overall assessment of the plan's readiness. + +### Scorecard + +| Area | Status | Notes | +|------|--------|-------| +| Security by design | Covered/Partial/Missing | ... | +| API design | Covered/Partial/Missing/N/A | ... | +| LLM code security | Covered/Partial/Missing | ... | +| Spec-driven development | Covered/Partial/Missing | ... | +| Test-driven development | Covered/Partial/Missing | ... | +| Operational readiness | Covered/Partial/Missing | ... | + +### Critical gaps +Numbered list of issues that should be addressed before implementation begins. Include the specific best practice being violated and a concrete suggestion. + +### Recommendations +Numbered list of improvements that would strengthen the plan but aren't blockers. + +### What's done well +Brief acknowledgment of areas the plan handles correctly -- reinforces good patterns. diff --git a/skills/review-spec/SKILL.md b/skills/review-spec/SKILL.md new file mode 100644 index 0000000..8f97d6d --- /dev/null +++ b/skills/review-spec/SKILL.md @@ -0,0 +1,149 @@ +--- +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, + 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 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 + +Also read any additional best practice files relevant to the spec's technology stack (check the index for Kubernetes, Helm, Docker, secrets management, etc.). + +## 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 + +## 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 | ... | +| 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.