Add best practices, hooks, memory files, and scripts from recent sessions
Includes: spec-driven and test-driven development best practices, reproduce-before-fixing debugging workflow, require-plan-file hook, find-project-root script, session logs, memory files for decisions/ gotchas/process-lessons, and updates to existing best practice topics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Paul O'Reilly
41
best-practices/.distill-state.json
Normal file
41
best-practices/.distill-state.json
Normal file
@@ -0,0 +1,41 @@
|
||||
{
|
||||
"version": 1,
|
||||
"last_run": "2026-03-15T10:24:05Z",
|
||||
"projects": {
|
||||
"agent-runtimes": {
|
||||
"path": "/home/paul/dev/claude/projects/agent-runtimes",
|
||||
"last_sha": "2adcc5aea4af520d407b062dcf5c81ff5fd69a73",
|
||||
"last_run": "2026-03-15T10:24:05Z"
|
||||
},
|
||||
"claude-foundations": {
|
||||
"path": "/home/paul/dev/claude/projects/claude-foundations",
|
||||
"last_sha": "c3a151a87bf726a239532e6739c58e8c41c45f4e",
|
||||
"last_run": "2026-03-15T10:24:05Z"
|
||||
},
|
||||
"cluster-apps/octopus-deploy": {
|
||||
"path": "/home/paul/dev/claude/projects/cluster-apps/octopus-deploy",
|
||||
"last_sha": "5989ef737cc5bf0885136259aecbb3726c2bf1c1",
|
||||
"last_run": "2026-03-15T10:24:05Z"
|
||||
},
|
||||
"cluster-bootstrap": {
|
||||
"path": "/home/paul/dev/claude/projects/cluster-bootstrap",
|
||||
"last_sha": "ab0460488a1251c6c1afdc7df929b3b7cd147243",
|
||||
"last_run": "2026-03-15T10:24:05Z"
|
||||
},
|
||||
"custom-claude-skills": {
|
||||
"path": "/home/paul/dev/claude/projects/custom-claude-skills",
|
||||
"last_sha": "bb6fb7270590296aa7c1594053cde1bcae114019",
|
||||
"last_run": "2026-03-15T10:24:05Z"
|
||||
},
|
||||
"hugo-accelerator": {
|
||||
"path": "/home/paul/dev/claude/projects/hugo-accelerator",
|
||||
"last_sha": "b300687256b70ef9f82781840863534a814496d3",
|
||||
"last_run": "2026-03-15T10:24:05Z"
|
||||
},
|
||||
"small-scripts": {
|
||||
"path": "/home/paul/dev/claude/small-scripts",
|
||||
"last_sha": "5230e542360f8b07b7375c943ffb99501f4aacd8",
|
||||
"last_run": "2026-03-15T10:24:05Z"
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -36,6 +36,41 @@ Before starting any OIDC integration, research:
|
||||
2. Which claims are in the ID token vs userinfo endpoint
|
||||
3. How the consumer matches RBAC identities (groups? email? username?)
|
||||
|
||||
## Log-First Diagnosis
|
||||
|
||||
- **CrashLoopBackOff: check logs first.** Error messages in pod logs usually point directly to the fix. Don't tweak configuration or security contexts blindly — `kubectl logs <pod>` first.
|
||||
- **Discriminate transient from persistent errors.** CSI lock contention, etcd timeouts during first install, and brief connectivity blips are self-healing. Don't spend time debugging errors that resolve on retry. If you see retry/backoff patterns in logs, wait before intervening.
|
||||
- **Trust controller retry logic.** CSI controllers, operators, and reconciliation loops have built-in retry. Transient failures during rapid provisioning are expected, not bugs.
|
||||
|
||||
## Reproduce Before Fixing
|
||||
|
||||
When a bug is discovered or reported, **do not start by trying to fix it.** The first step is always to write a test that reproduces the failure:
|
||||
|
||||
1. **Write a failing test.** Capture the bug as a test case that demonstrates the broken behaviour. This forces you to understand the bug precisely — what input triggers it, what the wrong output is, and what the correct output should be.
|
||||
2. **Fix the bug in isolation.** Use a subagent or a separate session to write the fix. The fixing agent gets the failing test as its success criterion — it's done when the test passes. This separation prevents the fixer from unconsciously weakening the test to match a broken implementation.
|
||||
3. **The test stays forever.** The reproduction test becomes a permanent regression test. It proves the fix works and prevents the bug from returning.
|
||||
|
||||
This workflow has several advantages:
|
||||
- **Forces precise understanding.** Writing a test means you know exactly what's broken, not just "it doesn't work."
|
||||
- **Prevents partial fixes.** The test defines "done" objectively — the fix either passes or it doesn't.
|
||||
- **Parallelises work.** While one agent fixes the bug, you can continue other work.
|
||||
- **Catches regressions.** The test remains in the suite, guarding against the same class of failure.
|
||||
|
||||
```python
|
||||
# Step 1: Write the failing test FIRST
|
||||
def test_regression_issue_427_empty_payload_crashes():
|
||||
"""Bug #427: Empty payload causes unhandled TypeError in dispatcher.
|
||||
Should return a 400 validation error, not crash."""
|
||||
response = client.post("/dispatch", json={})
|
||||
assert response.status_code == 400 # Currently crashes with 500
|
||||
|
||||
# Step 2: Hand to a subagent/session: "Make this test pass without breaking others"
|
||||
```
|
||||
|
||||
## Pattern Mining Before Authoring
|
||||
|
||||
Before building a new service, component, or script, read existing patterns in the codebase first. This matches conventions on the first attempt and avoids rework on naming, structure, and integration points. Applies to K8s manifests, CI pipelines, skill authoring, and script structure.
|
||||
|
||||
## Grep Your Own Docs
|
||||
|
||||
Known issues documented in CLAUDE.md or MEMORY.md but not applied to new scripts/configs waste debugging time. Search your own documentation before writing automation that touches areas with known gotchas.
|
||||
|
||||
@@ -26,6 +26,11 @@
|
||||
- Remote URL format: `git@<host-alias>:<org>/<repo>.git`
|
||||
- Optionally push-mirror to GitHub for public visibility
|
||||
|
||||
## Access and Clone Gotchas
|
||||
|
||||
- **Org repos require explicit collaborator grants.** Don't assume organizational membership implies write access — verify permissions before setting up automation or CI/CD.
|
||||
- **Shallow clones break push operations.** `git clone --depth 1` is fine for read-only CI jobs, but pipelines that push artifacts, tags, or mirror to other remotes need full clones.
|
||||
|
||||
## Version Management
|
||||
|
||||
- Use the latest stable version of dependencies unless pinned for a reason
|
||||
|
||||
@@ -29,6 +29,33 @@ Manual bootstrap secrets (encryption keys, OIDC client secrets) must be document
|
||||
- Cross-namespace policies need explicit namespace matching (e.g., `matchExpressions` on namespace label).
|
||||
- Always test from the actual consumer namespace, not same-namespace test pods.
|
||||
|
||||
## Probe Strategy
|
||||
|
||||
- **Liveness vs readiness probes serve different purposes.** TCP checks confirm the process is listening (liveness). Exec/command checks confirm the application is ready to serve (readiness). Don't conflate them.
|
||||
- **Probes must match application host validation.** Applications that validate Host headers (e.g., Next.js `ALLOWED_HOSTS`) will reject probes sent to the pod IP. Set `httpGet.httpHeaders` with the expected Host value.
|
||||
- **Don't load credentials into liveness probes.** If readiness requires an authenticated check (e.g., `sqlcmd`), use a simple TCP check for liveness and reserve the authenticated check for readiness only.
|
||||
|
||||
## Init Container Patterns
|
||||
|
||||
- **Writable config via init container + emptyDir.** When apps require writable directories but ConfigMaps are read-only, use an init container to copy config into an emptyDir volume that the main container mounts read-write.
|
||||
- **Privilege separation.** Init containers can run as root to create directories or set ownership, while the main container runs as a non-root UID. Prefer this over running the entire workload as root.
|
||||
- **Non-root images have hidden filesystem requirements.** Many modern images (e.g., MSSQL 2022, UID 10001) need writable directories beyond the obvious ones. Always check image documentation or `docker inspect` before writing manifests.
|
||||
|
||||
## StatefulSet Edge Cases
|
||||
|
||||
- **CrashLoopBackOff pods won't auto-replace on spec update.** The StatefulSet controller won't delete and recreate a crashing pod when you update the spec — manual `kubectl delete pod` is required to force recreation.
|
||||
- **Immutable field diffs can deadlock auto-sync.** StatefulSet fields like `volumeClaimTemplates` are immutable after creation. GitOps controllers (ArgoCD) will show permanent OutOfSync if the desired state differs from the live immutable fields. Force sync or recreate the StatefulSet.
|
||||
|
||||
## GitOps: Imperative vs Declarative
|
||||
|
||||
- **Never use imperative operations on GitOps-managed resources.** `kubectl rollout restart` adds annotations that conflict with the GitOps controller's desired state, causing permanent OutOfSync. Use declarative paths instead — update a configmap hash annotation in Git, or change a pod template label.
|
||||
- **ArgoCD reconciliation has latency.** New Application manifests don't appear immediately due to polling intervals. Use manual refresh annotations when automation needs immediate reconciliation.
|
||||
|
||||
## PodSecurity Alignment
|
||||
|
||||
- **Namespace PodSecurity labels must match container security contexts.** DinD, CSI drivers, and other privileged workloads need `pod-security.kubernetes.io/enforce: privileged` on their namespace. A `baseline` or `restricted` namespace silently blocks privileged pods.
|
||||
- **Document privileged namespace requirements.** When a workload needs elevated privileges, document the specific requirement (e.g., "Docker-in-Docker for CI builds") alongside the namespace label.
|
||||
|
||||
## Miscellaneous
|
||||
|
||||
- `enableServiceLinks: false` may be needed when K8s-injected service env vars conflict with app config (e.g., Authelia interprets `AUTHELIA_*` service vars as configuration).
|
||||
|
||||
@@ -22,8 +22,25 @@ If you run the same 3+ commands in sequence more than once, it should become a s
|
||||
- Steps requiring careful ordering
|
||||
- Multi-step manual processes that are error-prone
|
||||
|
||||
## Error Handling by Tool Purpose
|
||||
|
||||
Not all scripts need the same error handling strategy:
|
||||
- **Destructive scripts** (deploy, configure, delete) should use `set -euo pipefail` — fail fast on any error.
|
||||
- **Reporting/read-only scripts** (status dashboards, aggregation, monitoring) should start without `set -e` — complex data collection from multiple sources is hard to debug under errexit. Use explicit conditional checks instead.
|
||||
- **The choice depends on the tool's purpose.** A script that writes to production needs strict error handling. A script that reads from 10 sources and aggregates results needs resilience.
|
||||
|
||||
## Dryrun Mode
|
||||
|
||||
Every script that modifies state should support `--dryrun` / `-n`:
|
||||
- Makes the script self-documenting about its side effects
|
||||
- Enables safe testing and review before execution
|
||||
- Enables test harnesses that verify output without executing changes
|
||||
- Dryrun output should show exactly what would happen, not a summary
|
||||
|
||||
## Shell Gotchas
|
||||
|
||||
- `((PASS++))` fails under `set -e` when PASS=0 — the expression evaluates to 0 (false), triggering errexit. Use `PASS=$((PASS + 1))` instead.
|
||||
- `set -e` silently terminates complex pipelines and subshells with no output — makes debugging extremely difficult. Also kills command substitutions that capture non-zero exit codes (e.g., `result=$(grep "pattern" file)` exits if grep finds nothing).
|
||||
- `grep` interprets option-like strings (starting with `-`) as flags — use `--` terminator before patterns or input that may start with dashes.
|
||||
- Always quote variables in conditionals and file paths
|
||||
- Use `trap` for cleanup of temp files and credentials
|
||||
|
||||
@@ -36,6 +36,12 @@ Each Ansible project needs `vars_plugins_enabled = host_group_vars,community.sop
|
||||
|
||||
Some secrets are chicken-and-egg (e.g., the age decryption key for ArgoCD's KSOPS). These must be created manually as a bootstrap step and documented clearly.
|
||||
|
||||
## Credential Lifecycle Management
|
||||
|
||||
- **Track credential expiry dates.** OAuth client secrets, API tokens, and certificates have expiry dates that can cause silent failures. Document expiry dates when creating credentials.
|
||||
- **Set alerts before expiry.** For long-lived credentials (e.g., 720-day OAuth client secrets), set calendar reminders or automated monitoring alerts well before they expire.
|
||||
- **Rotation plan.** Know the rotation procedure before you need it — some credential types (e.g., Azure app registrations) require coordinated updates across multiple systems.
|
||||
|
||||
## Backup Considerations
|
||||
|
||||
Backup plans must include encryption keys (age private keys, etc.) so that encrypted data in Git repos remains recoverable.
|
||||
|
||||
@@ -15,6 +15,17 @@
|
||||
- **Use $ARGUMENTS** for user input, `$0`, `$1` etc. for positional args
|
||||
- Dynamic commands in `!`command`` run at skill load time, not during Claude's execution
|
||||
|
||||
## Portable Path Resolution
|
||||
|
||||
- **Use `CLAUDE_PROJECT_ROOT` env var** for cross-project path references in `!`command`` blocks. Hardcoded absolute paths (e.g., `~/dev/claude/...`) are user-specific. Relative paths (`../`) break depending on CWD and can trigger sandbox violations when they resolve outside allowed directories.
|
||||
- **Add a detection fallback.** Include a "Step 0" in skill instructions that detects the project root by walking up the directory tree to find the highest `CLAUDE.md` if the env var isn't set. This makes skills work even without prior setup.
|
||||
- **Keep config paths relative to the root.** Settings files should use paths relative to `CLAUDE_PROJECT_ROOT` (e.g., `projects_dir: projects`) rather than absolute paths, so they're portable across machines.
|
||||
|
||||
## Skill Discovery Timing
|
||||
|
||||
- **Skills are discovered at session start, not dynamically.** Creating or symlink a new skill mid-session requires restarting Claude Code to use it as a slash command.
|
||||
- **Broken symlinks cause silent failures under `set -e`.** `readlink -f` on a broken symlink returns empty string. The install script should validate symlinks and remove stale ones.
|
||||
|
||||
## `!`command`` Gotchas
|
||||
|
||||
- **No `$()` command substitution** — the permission checker rejects commands containing `$()`
|
||||
@@ -22,3 +33,4 @@
|
||||
- **`allowed-tools` patterns must match the command binary** — each binary used in `!`command`` blocks needs its own pattern
|
||||
- **Prefer specific tool patterns over broad ones** — `Bash(git log *)` is safer than `Bash(git *)`
|
||||
- **Fallback to tool instructions for dynamic paths** — if a command needs `$ARGUMENTS` to compute a path, use a plain-text instruction telling Claude to use the Read tool instead
|
||||
- **Env var expansion works** — `${CLAUDE_PROJECT_ROOT}` expands in `!`command`` blocks because they run as shell commands. This is the recommended pattern for portable cross-project paths.
|
||||
|
||||
201
best-practices/spec-driven-development.md
Normal file
201
best-practices/spec-driven-development.md
Normal file
@@ -0,0 +1,201 @@
|
||||
# Spec-Driven Development with AI Agents
|
||||
|
||||
Best practices for using structured specifications to coordinate AI agent implementation work. Extracted from real project experience (agent-runtimes) and industry research (OpenSpec, Codified Context paper, Addy Osmani's workflow guides).
|
||||
|
||||
## Why Specs Matter for AI Agents
|
||||
|
||||
AI agents trust documentation absolutely. A well-written spec gives an agent everything it needs to implement a subsystem without reading the entire codebase. A stale or vague spec causes silent failures where agents generate code that is structurally valid but architecturally wrong.
|
||||
|
||||
Specs serve three functions that CLAUDE.md alone cannot:
|
||||
1. **Compressed context** — an agent reads one spec, not 300 lines of mixed concerns
|
||||
2. **Testable contracts** — numbered requirements and scenarios translate directly to pytest
|
||||
3. **Handoff boundaries** — an agent working on the dispatcher doesn't need to understand the entrypoint internals, just the interface between them
|
||||
|
||||
## Spec Structure
|
||||
|
||||
Each spec follows a consistent template. Sections are ordered so an agent can read top-down and build understanding progressively.
|
||||
|
||||
### Required Sections
|
||||
|
||||
1. **Overview** — What this subsystem does. 2-3 sentences. An agent should know if this spec is relevant after reading this.
|
||||
2. **Responsibilities** — What this subsystem owns and what it delegates. Prevents scope creep during implementation.
|
||||
3. **Dependencies** — Which other specs to read first. Keeps the reading list minimal.
|
||||
4. **Data Model** — Types, schemas, state machines, interfaces. The concrete contract.
|
||||
5. **Requirements** — Numbered functional requirements (e.g., E-1, E-2). Each must be independently testable.
|
||||
6. **Scenarios** — Concrete given/when/then examples. These become test functions.
|
||||
|
||||
### Optional Sections
|
||||
|
||||
7. **Interface** — API surface, function signatures, HTTP endpoints. Include when the subsystem has an external-facing API.
|
||||
8. **Extension Points** — How to add new capabilities without modifying existing code. Step-by-step instructions.
|
||||
9. **Error Handling** — Failure modes and expected behaviour. Prevents agents from inventing their own error strategies.
|
||||
|
||||
### Writing Guidelines
|
||||
|
||||
- **Be specific, not comprehensive.** A spec that says "handle errors appropriately" is useless. A spec that says "return exit code 124 on timeout" is testable.
|
||||
- **Include the why.** Design intent and constraints prevent agents from making structurally valid but architecturally wrong changes. Requirements without rationale are followed mechanically — agents can't judge edge cases or make trade-offs. Every constraint needs a "Why:" line. Example: `"Secrets never in payload"` needs `"because payloads may be logged and stored in task history"`.
|
||||
- **Use concrete examples.** Every data model section should include a realistic JSON/code example, not just a schema.
|
||||
- **Cross-reference, don't duplicate.** If two specs share a concept (e.g., the payload schema), one spec owns it and the other links to it.
|
||||
- **Keep each spec self-contained.** An agent should be able to implement a subsystem by reading the target spec plus its listed dependencies. If it needs to read CLAUDE.md, the spec is incomplete.
|
||||
|
||||
## Requirement Numbering
|
||||
|
||||
Each spec uses a short prefix derived from its name, followed by a sequential number:
|
||||
|
||||
| Spec | Prefix | Example |
|
||||
|---|---|---|
|
||||
| payload.md | P | P-1, P-2 |
|
||||
| entrypoint.md | E | E-1, E-2 |
|
||||
| actions.md | A | A-1, A-2 |
|
||||
| runners.md | R | R-1, R-2 |
|
||||
| dispatcher.md | D | D-1, D-2 |
|
||||
| control-plane.md | CP | CP-1, CP-2 |
|
||||
|
||||
Requirements must be:
|
||||
- **Independently testable** — each maps to one or more test functions
|
||||
- **Unambiguous** — an agent can determine pass/fail without human judgement
|
||||
- **Stable** — changing a requirement number invalidates tests, so avoid renumbering
|
||||
|
||||
## Scenarios as Test Blueprints
|
||||
|
||||
Every scenario in a spec should be directly translatable to a test function. Use this format:
|
||||
|
||||
```markdown
|
||||
### Scenario: Pre-action failure
|
||||
**Given:** Payload with clone pre-action (invalid repo URL)
|
||||
**When:** Clone fails (git returns non-zero)
|
||||
**Then:** on_error actions run, container exits 1. Runner never executes.
|
||||
```
|
||||
|
||||
This becomes:
|
||||
|
||||
```python
|
||||
def test_scenario_preaction_failure_runs_on_error_and_exits_1(...):
|
||||
"""Given clone fails, on_error runs and exits 1. Runner never executes."""
|
||||
```
|
||||
|
||||
Guidelines:
|
||||
- Each scenario tests one behaviour, not a combination
|
||||
- Include both happy paths and error paths
|
||||
- Name the scenario descriptively — it becomes the test function's docstring
|
||||
- Include enough setup detail that an agent can write the test without guessing
|
||||
|
||||
## The Spec → Test → Code Workflow
|
||||
|
||||
This is the core development loop. Tests are written from the spec before code exists.
|
||||
|
||||
### 1. Write or Update the Spec
|
||||
|
||||
Define requirements and scenarios. Get them reviewed. The spec is the source of truth for what the system should do.
|
||||
|
||||
### 2. Write Tests from the Spec
|
||||
|
||||
Translate requirements and scenarios into pytest functions. Tests should:
|
||||
- Map to requirement IDs in their names: `test_e4_preaction_failure_skips_remaining`
|
||||
- Use the scenario's given/when/then as the test body structure
|
||||
- Mock external dependencies (subprocess, HTTP, filesystem)
|
||||
- Run fast (no Docker, no network, no real APIs)
|
||||
|
||||
### 3. Run the Tests — They Should All Fail
|
||||
|
||||
This confirms the tests are actually testing something. If a test passes before implementation, it's either testing the wrong thing or the feature already exists.
|
||||
|
||||
### 4. Implement Until Tests Pass
|
||||
|
||||
Write the minimum code to make tests pass. The spec defines what, the tests verify it, the code implements it.
|
||||
|
||||
### 5. Update Spec if Implementation Reveals Issues
|
||||
|
||||
Sometimes implementation reveals that a requirement is unworkable or incomplete. Update the spec, update the test, then update the code. The spec stays authoritative.
|
||||
|
||||
## Spec Maintenance
|
||||
|
||||
### Preventing Drift
|
||||
|
||||
Specs drift from code when they're treated as planning documents that are "done" after implementation. They must be treated as living contracts.
|
||||
|
||||
**Rules:**
|
||||
- **Spec changes require test changes.** If a requirement changes, its test must change in the same commit.
|
||||
- **Code changes that affect interfaces require spec changes.** If a function signature, API endpoint, or data schema changes, the relevant spec must be updated in the same commit.
|
||||
- **New features require spec-first.** Add the requirement and scenario to the spec, write the test, then implement.
|
||||
|
||||
### CI Enforcement
|
||||
|
||||
Enforce spec hygiene with automated checks:
|
||||
|
||||
1. **Pre-commit hook** — run pytest, block commit on failure (already implemented)
|
||||
2. **Spec coverage check** — a script that verifies every numbered requirement has at least one test function referencing it
|
||||
3. **Orphan test detection** — tests referencing requirement IDs that no longer exist in specs
|
||||
|
||||
### Review Checklist
|
||||
|
||||
When reviewing a PR that touches a spec subsystem:
|
||||
- [ ] Spec updated if interface or behaviour changed
|
||||
- [ ] Test added/updated for new/changed requirements
|
||||
- [ ] Cross-references still valid
|
||||
- [ ] No requirements removed without deprecation note
|
||||
|
||||
## Context Architecture for Agents
|
||||
|
||||
Based on the Codified Context paper (108k-line system, 283 sessions), structure project knowledge in three tiers:
|
||||
|
||||
### Tier 1: Hot Context (Always Loaded)
|
||||
|
||||
CLAUDE.md — conventions, env vars, repo structure, scripts. Loaded every session. Keep under ~300 lines by linking to details elsewhere.
|
||||
|
||||
### Tier 2: Spec Context (Per-Task)
|
||||
|
||||
`spec/` files — loaded based on what the agent is working on. An agent implementing a new action reads `spec/actions.md` + `spec/payload.md`. An agent working on the dispatcher reads `spec/dispatcher.md` + `spec/container-backends.md`.
|
||||
|
||||
The spec index (SPEC.md) has a "read this when..." column to guide selection.
|
||||
|
||||
### Tier 3: Cold Context (On-Demand)
|
||||
|
||||
`memory/` files — gotchas, reflections, decisions. Loaded only when relevant. An agent hitting a weird Cilium issue checks `memory/gotchas-cilium.md`.
|
||||
|
||||
### Routing Context to Agents
|
||||
|
||||
When launching an agent to work on a subsystem:
|
||||
1. Point it at the relevant spec(s) via its prompt
|
||||
2. Include CLAUDE.md for conventions
|
||||
3. Let it pull from memory/ on-demand if it hits issues
|
||||
|
||||
Don't load everything — agents perform better with focused context than with a 50-page dump.
|
||||
|
||||
## Testing Depth
|
||||
|
||||
The spec→test→code workflow defines *when* to write tests. For *how* to write comprehensive tests — edge case discovery, property-based testing, mutation testing, AI agent testing patterns — see [Test-Driven Development](test-driven-development.md).
|
||||
|
||||
## Post-Write Spec Audit
|
||||
|
||||
After writing specs, audit them against best practices before implementation. Common gap categories:
|
||||
|
||||
1. **Missing rationale** — Constraints without "Why:" lines. Agents follow them mechanically but can't judge edge cases.
|
||||
2. **Missing error/failure scenarios** — Happy paths are covered but failure modes aren't specified.
|
||||
3. **Cross-spec interface misalignment** — Two specs describe the same interface differently.
|
||||
4. **Vague requirements** — "Handle errors appropriately" instead of specific error codes and behaviours.
|
||||
5. **Missing specs for discovered subsystems** — Implementation reveals components that weren't planned for.
|
||||
|
||||
Write-then-audit is more productive than trying to get specs perfect on the first pass. The audit step catches systematic gaps across all specs at once.
|
||||
|
||||
## Anti-Patterns
|
||||
|
||||
### Specs as documentation, not contracts
|
||||
**Symptom:** Specs describe what was built, updated after the fact. Tests don't reference spec IDs.
|
||||
**Fix:** Write specs before code. Tests reference requirement IDs. Specs are the input, not the output.
|
||||
|
||||
### Mega-spec
|
||||
**Symptom:** One large spec covering the entire system. Agents must read thousands of lines to find what they need.
|
||||
**Fix:** Split by subsystem. Each spec should be readable in under 5 minutes.
|
||||
|
||||
### Spec without scenarios
|
||||
**Symptom:** Requirements are abstract ("handle errors gracefully"). No concrete examples.
|
||||
**Fix:** Every requirement needs at least one given/when/then scenario with specific inputs and outputs.
|
||||
|
||||
### Implementation details in specs
|
||||
**Symptom:** Spec dictates variable names, algorithm choices, internal data structures.
|
||||
**Fix:** Specs define what and why, not how. The interface is specified; the implementation is free.
|
||||
|
||||
### Untested requirements
|
||||
**Symptom:** Requirements exist in the spec but no test references them. They drift without anyone noticing.
|
||||
**Fix:** Spec coverage check in CI. Every requirement ID must appear in at least one test function name.
|
||||
456
best-practices/test-driven-development.md
Normal file
456
best-practices/test-driven-development.md
Normal file
@@ -0,0 +1,456 @@
|
||||
# Test-Driven Development for Spec-Based Projects
|
||||
|
||||
Best practices for writing comprehensive, regression-catching tests in projects that use structured specifications. Focuses on maximising test value (catching real bugs) rather than test volume (inflating coverage numbers). Extracted from industry research, academic papers (TDAD, Codified Context), and practitioner experience.
|
||||
|
||||
## Core Principle: Tests Are the Spec's Enforcement Layer
|
||||
|
||||
In a spec-driven project, the spec defines *what* and the tests *prove it*. A requirement without a test is an aspiration. A test without a requirement is undocumented behaviour. Keep them tightly coupled:
|
||||
|
||||
- Every numbered requirement (P-1, E-3) has at least one test
|
||||
- Every test function name includes its requirement ID: `test_e3_preaction_failure_exits_1`
|
||||
- Spec changes and test changes ship in the same commit
|
||||
|
||||
## Deriving Tests from Specs
|
||||
|
||||
### Requirements to Tests
|
||||
|
||||
Each spec requirement becomes one or more test functions. The mapping isn't always 1:1 — a requirement like "must respect timeout" needs tests for: default timeout, explicit timeout, timeout=0 (no limit), timeout exceeded.
|
||||
|
||||
```python
|
||||
# From spec: R-4: Runners must respect runtime.timeout.
|
||||
# Default 3600s. Value of 0 means no timeout.
|
||||
|
||||
def test_r4_default_timeout_is_3600():
|
||||
"""R-4: When timeout not specified, default is 3600s."""
|
||||
|
||||
def test_r4_explicit_timeout_is_honoured():
|
||||
"""R-4: When timeout=60, process killed after 60s."""
|
||||
|
||||
def test_r4_zero_timeout_means_no_limit():
|
||||
"""R-4: When timeout=0, no timeout is applied."""
|
||||
|
||||
def test_r4_timeout_returns_exit_code_124():
|
||||
"""R-4 + R-5: Timeout produces exit code 124."""
|
||||
```
|
||||
|
||||
### Scenarios to Tests
|
||||
|
||||
GIVEN/WHEN/THEN scenarios translate directly to Arrange/Act/Assert:
|
||||
|
||||
```python
|
||||
def test_scenario_preaction_failure_runs_on_error():
|
||||
"""Given clone fails, on_error runs and exits 1. Runner never executes."""
|
||||
# GIVEN — arrange
|
||||
payload = make_payload(pre_actions=[{"action": "clone", "repo": "bad-url"}])
|
||||
mock_clone = Mock(side_effect=subprocess.CalledProcessError(128, "git"))
|
||||
|
||||
# WHEN — act
|
||||
exit_code = run_entrypoint(payload, clone_handler=mock_clone)
|
||||
|
||||
# THEN — assert
|
||||
assert exit_code == 1
|
||||
mock_runner.assert_not_called()
|
||||
mock_on_error.assert_called_once()
|
||||
```
|
||||
|
||||
### Parameterised Tests from Spec Enumerations
|
||||
|
||||
When a spec lists multiple valid values, use `@pytest.mark.parametrize`:
|
||||
|
||||
```python
|
||||
# From spec: task states are pending, assigned, running, succeeded, failed, timed_out, cancelled
|
||||
@pytest.mark.parametrize("terminal_state", ["succeeded", "failed", "timed_out", "cancelled"])
|
||||
def test_cp_terminal_state_cannot_be_overwritten(terminal_state):
|
||||
"""CP: Terminal states reject further transitions with 409."""
|
||||
```
|
||||
|
||||
## Systematic Edge Case Discovery
|
||||
|
||||
~80% of bugs cluster at boundaries. Use these techniques to find edge cases systematically rather than by intuition.
|
||||
|
||||
### Boundary Value Analysis
|
||||
|
||||
For every input parameter, test at the edges of its valid range:
|
||||
|
||||
| Input type | Test values |
|
||||
|---|---|
|
||||
| Integer (range 1-100) | 0, 1, 2, 99, 100, 101, -1, MAX_INT |
|
||||
| String | `""`, `"a"`, max-length string, max+1, unicode (`"\u0000"`, emoji), whitespace-only |
|
||||
| List/Array | `[]`, `[single]`, many items, duplicates, `None` |
|
||||
| Dict/Map | `{}`, missing required keys, extra unknown keys, `None` values |
|
||||
| Timeout (seconds) | 0, 1, -1, very large (999999), `None`/missing |
|
||||
| Base64 | valid, invalid chars, empty, padding variants (`=`, `==`, none) |
|
||||
|
||||
### Equivalence Partitioning
|
||||
|
||||
Group inputs into classes where all members should behave identically. Test one from each class:
|
||||
|
||||
```python
|
||||
# Payload validation: prompt field
|
||||
# Class 1: valid string → accepted
|
||||
# Class 2: empty string → rejected (spec says prompt is required)
|
||||
# Class 3: missing key → rejected
|
||||
# Class 4: wrong type (int, list, None) → rejected
|
||||
# Class 5: very long string → accepted (no length limit in spec)
|
||||
|
||||
@pytest.mark.parametrize("prompt,should_pass", [
|
||||
("Fix the bug", True), # Class 1: valid
|
||||
("", False), # Class 2: empty
|
||||
(None, False), # Class 3: missing/None
|
||||
(42, False), # Class 4: wrong type
|
||||
("x" * 100_000, True), # Class 5: long string
|
||||
])
|
||||
def test_p_prompt_validation(prompt, should_pass):
|
||||
...
|
||||
```
|
||||
|
||||
### State Transition Coverage
|
||||
|
||||
For state machines (task states, dispatcher states), test:
|
||||
|
||||
1. **Every valid transition:** `pending → assigned → running → succeeded`
|
||||
2. **Every invalid transition:** `succeeded → running` (should be rejected)
|
||||
3. **Initial state:** newly created tasks start in `pending`
|
||||
4. **Terminal states:** `succeeded`, `failed`, `timed_out`, `cancelled` cannot transition further
|
||||
5. **Re-entrant transitions:** same state → same state (should be idempotent or rejected, per spec)
|
||||
|
||||
```python
|
||||
VALID_TRANSITIONS = [
|
||||
("pending", "assigned"),
|
||||
("assigned", "running"),
|
||||
("running", "succeeded"),
|
||||
("running", "failed"),
|
||||
("running", "timed_out"),
|
||||
("assigned", "cancelled"),
|
||||
("running", "cancelled"),
|
||||
]
|
||||
|
||||
INVALID_TRANSITIONS = [
|
||||
("succeeded", "failed"),
|
||||
("failed", "running"),
|
||||
("cancelled", "pending"),
|
||||
("timed_out", "running"),
|
||||
]
|
||||
|
||||
@pytest.mark.parametrize("from_state,to_state", VALID_TRANSITIONS)
|
||||
def test_valid_state_transition(from_state, to_state):
|
||||
...
|
||||
|
||||
@pytest.mark.parametrize("from_state,to_state", INVALID_TRANSITIONS)
|
||||
def test_invalid_state_transition_rejected(from_state, to_state):
|
||||
...
|
||||
```
|
||||
|
||||
### The Edge Case Checklist
|
||||
|
||||
Walk through this for every function under test:
|
||||
|
||||
1. **Empty/null inputs** — what happens when required fields are missing?
|
||||
2. **Boundary values** — min, max, zero, negative, off-by-one
|
||||
3. **Type mismatches** — string where int expected, list where dict expected
|
||||
4. **Malformed input** — invalid JSON, bad base64, truncated data
|
||||
5. **Concurrent operations** — two tasks claiming the same resource
|
||||
6. **Ordering** — actions that depend on sequence (pre-action before runner)
|
||||
7. **Idempotency** — calling the same operation twice (kill an already-killed container)
|
||||
8. **Resource exhaustion** — at capacity, disk full, timeout expired
|
||||
9. **Partial failure** — first action succeeds, second fails (cleanup?)
|
||||
|
||||
## Property-Based Testing with Hypothesis
|
||||
|
||||
Instead of specifying individual test cases, define *properties* that must hold for all inputs. Hypothesis generates hundreds of inputs including edge cases you'd never think of.
|
||||
|
||||
### When to Use Property-Based Testing
|
||||
|
||||
- **Serialisation roundtrips:** encode → decode returns original
|
||||
- **Parsers:** should never crash on any input
|
||||
- **Data transformations:** invariants that hold regardless of input
|
||||
- **Validators:** valid inputs accepted, invalid inputs rejected (never crash)
|
||||
|
||||
### When NOT to Use It
|
||||
|
||||
- Tests where generating valid inputs is harder than the code itself
|
||||
- Tests where the "property" just restates the implementation
|
||||
- UI or integration tests
|
||||
|
||||
### Patterns
|
||||
|
||||
```python
|
||||
from hypothesis import given, strategies as st, assume, settings
|
||||
from hypothesis import example
|
||||
|
||||
# Roundtrip: base64 encode/decode preserves payload
|
||||
@given(st.text())
|
||||
def test_base64_roundtrip(payload_str):
|
||||
encoded = base64.b64encode(payload_str.encode()).decode()
|
||||
decoded = base64.b64decode(encoded).decode()
|
||||
assert decoded == payload_str
|
||||
|
||||
# Invariant: payload validation never crashes (may reject, never exception)
|
||||
@given(st.dictionaries(st.text(), st.text() | st.integers() | st.none()))
|
||||
def test_payload_validation_never_crashes(raw_payload):
|
||||
# Should return True/False or raise ValidationError — never unhandled exception
|
||||
try:
|
||||
validate_payload(raw_payload)
|
||||
except ValidationError:
|
||||
pass # Expected for invalid input
|
||||
|
||||
# Pin known edge cases alongside random generation
|
||||
@example("") # empty string
|
||||
@example("\x00") # null byte
|
||||
@example("a" * 10**6) # very long
|
||||
@given(st.text())
|
||||
def test_prompt_handling(prompt):
|
||||
...
|
||||
|
||||
# Composite strategies for domain objects
|
||||
@st.composite
|
||||
def valid_payloads(draw):
|
||||
return {
|
||||
"task_id": draw(st.uuids()).hex,
|
||||
"prompt": draw(st.text(min_size=1)),
|
||||
"runtime": {"cli": draw(st.sampled_from(["claude", "codex"]))},
|
||||
}
|
||||
|
||||
@given(valid_payloads())
|
||||
def test_valid_payload_always_accepted(payload):
|
||||
assert validate_payload(payload) is True
|
||||
```
|
||||
|
||||
### Stateful Testing for State Machines
|
||||
|
||||
Hypothesis can generate sequences of operations and check invariants after each step:
|
||||
|
||||
```python
|
||||
from hypothesis.stateful import RuleBasedStateMachine, rule, precondition
|
||||
|
||||
class TaskStateMachine(RuleBasedStateMachine):
|
||||
def __init__(self):
|
||||
super().__init__()
|
||||
self.task = Task(state="pending")
|
||||
|
||||
@rule()
|
||||
@precondition(lambda self: self.task.state == "pending")
|
||||
def assign(self):
|
||||
self.task.transition("assigned")
|
||||
assert self.task.state == "assigned"
|
||||
|
||||
@rule()
|
||||
@precondition(lambda self: self.task.state == "running")
|
||||
def complete(self):
|
||||
self.task.transition("succeeded")
|
||||
assert self.task.state == "succeeded"
|
||||
|
||||
# Invariant: terminal states never change
|
||||
@invariant()
|
||||
def terminal_states_are_final(self):
|
||||
if self.task.state in ("succeeded", "failed", "cancelled"):
|
||||
with pytest.raises(InvalidTransition):
|
||||
self.task.transition("running")
|
||||
|
||||
TestTaskStates = TaskStateMachine.TestCase
|
||||
```
|
||||
|
||||
## Mutation Testing
|
||||
|
||||
Mutation testing answers: "If someone introduced a bug, would our tests catch it?"
|
||||
|
||||
Tools make small code changes (replacing `>` with `>=`, `True` with `False`, deleting statements) and check if tests still pass. Surviving mutants = test gaps.
|
||||
|
||||
### Setup with mutmut
|
||||
|
||||
```toml
|
||||
# pyproject.toml
|
||||
[tool.mutmut]
|
||||
paths_to_mutate = "entrypoint/"
|
||||
tests_dir = "tests/"
|
||||
runner = "python -m pytest tests/ -x -q"
|
||||
```
|
||||
|
||||
```bash
|
||||
# Run mutation testing
|
||||
mutmut run
|
||||
|
||||
# See surviving mutants
|
||||
mutmut results
|
||||
|
||||
# Inspect a specific mutant
|
||||
mutmut show 42
|
||||
```
|
||||
|
||||
### Practical Guidance
|
||||
|
||||
- **Target: mutation score above 80%.** Scores above 90% have diminishing returns (equivalent mutants).
|
||||
- **Focus on business logic** — validators, state machines, parsers. Skip glue code.
|
||||
- **Use mutation testing to audit AI-generated tests.** This is the most powerful combination: AI writes tests from spec, mutation testing verifies those tests catch real faults.
|
||||
- **Run on changed files only in CI** (full suite is slow). Full run nightly or pre-release.
|
||||
|
||||
## Test Architecture
|
||||
|
||||
### The Testing Pyramid for Spec-Driven Projects
|
||||
|
||||
| Layer | Proportion | Speed | What it catches |
|
||||
|---|---|---|---|
|
||||
| Unit tests | 60-70% | <1ms each | Logic errors, boundary violations, state machine bugs |
|
||||
| Property-based | 10-15% | ~10ms each | Edge cases humans miss, roundtrip failures, crash inputs |
|
||||
| Integration | 15-20% | ~100ms each | Component interaction bugs, mock/reality divergence |
|
||||
| E2E / acceptance | 5-10% | ~1s+ each | Full-chain failures, deployment config issues |
|
||||
|
||||
### Test Isolation Principles
|
||||
|
||||
- **No test depends on another test's state.** Each test sets up its own preconditions.
|
||||
- **No test depends on execution order.** `pytest-randomly` catches order dependencies.
|
||||
- **No test touches the real filesystem outside `tmp_path`.** Monkeypatch paths that default to production locations (like `/workspace`).
|
||||
- **No test makes network calls.** Mock HTTP, subprocess, and socket calls.
|
||||
- **Integration tests are marked** (`@pytest.mark.integration`) and excluded by default.
|
||||
|
||||
### Fixture Architecture
|
||||
|
||||
```python
|
||||
# conftest.py — shared fixtures, not test logic
|
||||
|
||||
@pytest.fixture
|
||||
def minimal_payload():
|
||||
"""Smallest valid payload — tests shouldn't need more unless testing specific fields."""
|
||||
return {"task_id": "test-123", "prompt": "do something", "runtime": {"cli": "claude"}}
|
||||
|
||||
@pytest.fixture
|
||||
def encode_payload():
|
||||
"""Helper: dict → base64 string (how the dispatcher passes payloads)."""
|
||||
def _encode(d):
|
||||
return base64.b64encode(json.dumps(d).encode()).decode()
|
||||
return _encode
|
||||
|
||||
# Per-module conftest for module-specific fixtures
|
||||
# tests/test_dispatcher/conftest.py
|
||||
@pytest.fixture
|
||||
def mock_backend():
|
||||
"""Fake container backend that records calls without Docker."""
|
||||
...
|
||||
```
|
||||
|
||||
### Negative Tests Are as Important as Positive Tests
|
||||
|
||||
For every "this works" test, write at least one "this fails correctly" test:
|
||||
|
||||
```python
|
||||
# Positive: valid payload accepted
|
||||
def test_p1_valid_payload_loads():
|
||||
...
|
||||
|
||||
# Negative: missing required field rejected
|
||||
def test_p3_missing_prompt_raises():
|
||||
...
|
||||
|
||||
# Negative: wrong type rejected
|
||||
def test_p_prompt_wrong_type_raises():
|
||||
...
|
||||
|
||||
# Negative: extra unknown fields are ignored (not rejected)
|
||||
def test_p_unknown_fields_ignored():
|
||||
...
|
||||
```
|
||||
|
||||
## AI Agent Testing Patterns
|
||||
|
||||
### The Two-Phase Rule
|
||||
|
||||
**Never let the same agent write both tests and implementation in one pass.** An agent that writes tests and code together will unconsciously write tests that verify its own broken assumptions.
|
||||
|
||||
The workflow:
|
||||
1. **Phase 1:** Agent reads spec → writes tests. Human reviews tests against spec.
|
||||
2. **Phase 2:** Agent (or different agent) reads spec + tests → writes implementation until tests pass.
|
||||
|
||||
### Hidden Test Splits
|
||||
|
||||
Hold back some tests that the implementing agent never sees. Use them as a final validation:
|
||||
|
||||
```python
|
||||
# tests/test_payload.py — agent sees these during development
|
||||
def test_p1_load_from_env_var(): ...
|
||||
def test_p2_missing_payload_exits_1(): ...
|
||||
|
||||
# tests/test_payload_hidden.py — agent never sees these, run post-implementation
|
||||
# (Marked with a custom marker, excluded from default run)
|
||||
@pytest.mark.hidden
|
||||
def test_p1_load_from_file_fallback(): ...
|
||||
@pytest.mark.hidden
|
||||
def test_p_concurrent_payload_loads(): ...
|
||||
```
|
||||
|
||||
### Regression Tests from Real Bugs
|
||||
|
||||
Every bug found in production or during integration testing becomes a permanent test case:
|
||||
|
||||
```python
|
||||
def test_regression_crlf_corruption():
|
||||
"""Regression: smtp-oauth-relay converted \\r\\n to \\n, breaking quoted-printable.
|
||||
Fixed by as_bytes(policy=email_policy.SMTP). See memory/gotchas-email-relay.md."""
|
||||
...
|
||||
```
|
||||
|
||||
These are the highest-value tests because they catch proven failure modes.
|
||||
|
||||
## Test Quality Metrics
|
||||
|
||||
### What to Measure
|
||||
|
||||
| Metric | Target | Why |
|
||||
|---|---|---|
|
||||
| Spec coverage | 100% | Every numbered requirement has at least one test |
|
||||
| Mutation score | >80% | Tests catch real faults, not just inflate coverage |
|
||||
| Line coverage | >90% | Baseline hygiene (necessary but not sufficient) |
|
||||
| Test speed | <10s total | Fast enough for pre-commit hooks |
|
||||
| Assertion density | >1 per test | Tests that don't assert don't catch anything |
|
||||
|
||||
### What NOT to Measure
|
||||
|
||||
- **100% line coverage as a goal.** Chasing 100% leads to tests that exercise code paths without meaningful assertions.
|
||||
- **Test count.** 50 well-targeted tests beat 200 shallow ones.
|
||||
- **Test-to-code ratio.** The ratio depends on the module's complexity, not a universal number.
|
||||
|
||||
## CI Integration
|
||||
|
||||
### Pre-commit (Every Commit)
|
||||
```bash
|
||||
pytest tests/ -x -q --tb=short -m "not integration"
|
||||
```
|
||||
|
||||
### PR Validation (Every Push)
|
||||
```bash
|
||||
# Unit + property-based tests
|
||||
pytest tests/ -q --tb=short -m "not integration"
|
||||
# Mutation testing on changed files only
|
||||
mutmut run --paths-to-mutate="$(git diff --name-only main... | grep '.py$' | tr '\n' ',')"
|
||||
```
|
||||
|
||||
### Nightly
|
||||
```bash
|
||||
# Full mutation testing
|
||||
mutmut run
|
||||
# Integration tests (requires Docker)
|
||||
pytest tests/ -m integration
|
||||
# Hidden test validation
|
||||
pytest tests/ -m hidden
|
||||
```
|
||||
|
||||
## Anti-Patterns
|
||||
|
||||
### Tests that mirror implementation
|
||||
**Symptom:** Test asserts that function calls happen in a specific order, using mock.assert_has_calls with exact sequences. Breaks on any refactor.
|
||||
**Fix:** Test behaviour (inputs → outputs), not implementation details.
|
||||
|
||||
### Tests without assertions
|
||||
**Symptom:** `test_it_runs()` calls the function and checks it doesn't crash. No assertion on the result.
|
||||
**Fix:** Every test must assert something specific about the output, side effects, or raised exceptions.
|
||||
|
||||
### Overmocking
|
||||
**Symptom:** Every dependency is mocked. Tests pass but integration fails because mocks don't match real behaviour.
|
||||
**Fix:** Mock at the boundary (subprocess, HTTP, filesystem), not between your own modules. Use real objects for internal dependencies.
|
||||
|
||||
### Fragile tests
|
||||
**Symptom:** Tests break when unrelated code changes. Usually caused by asserting on implementation details, shared mutable state, or execution order.
|
||||
**Fix:** Test the public interface. Use fixtures for setup. Isolate each test completely.
|
||||
|
||||
### Testing private methods
|
||||
**Symptom:** Tests import `_internal_helper` and test it directly. These break on any refactor.
|
||||
**Fix:** Test through the public API. If a private method is complex enough to need its own tests, it should probably be a separate module with a public interface.
|
||||
Reference in New Issue
Block a user