review-tests

Files (1)

• SKILL.md

SKILL.md (first lines)

# Review Tests

Given a built feature, prove its existing tests cover the acceptance criteria, scenarios, and correctness constraints documented in its Blueprint (`blueprint.md`) — then fill the gaps with surface tests.

The pipeline is a single thorough pass: extract the testable expectations from the Blueprint, find coverage gaps with parallel sub-agents, validate and skeptically challenge those gaps to kill false positives, prove-or-refute any "this code is unreachable" claim by actually writing a test that reaches it, surface anything that needs a product decision — including disagreements between the Blueprint's own sections (its product half, technical half, and User Stories section), the ADRs, and the code's actual behavior — and, when nothing is left for the user to decide, have an implementer write the missing tests following the `surface-testing` skill. Then a final **Coverage Sweep** (Phase 6.5) measures the implementation's line coverage and lifts it toward a soft 80% floor: still-uncovered code becomes candidate behaviors that run through the very same validate→skeptic→probe→implement discipline, so the number is raised only by real surface tests — never by assertion-free padding — and any genuine shortfall (unreachable-through-surface code) is reported with evidence rather than gated on. It persists a machine-parseable HTML coverage report so the next run knows what was found before.

**Two principles run through the whole pipeline:**
1. **A claim that code is unreachable / untestable / dead is an empirical claim, not an argument.** It is the highest-scrutiny verdict in this skill. You never assign it by reasoning from the source; you assign it only after *attempting to write a test that reaches the code and failing*, with the specific unbypassable blocker named. Proving a gap exists already requires a failing test — proving the *absence* of a reachable behavior must be just as rigorous, or the pipeline will quietly bias toward false "untestable" verdicts.
2. **When sources disagree, report — do not adjudicate.** The Blueprint's sections (product half, technical half, User Stories), the ADRs, and the code's actual behavior will sometimes contradict each other. The skill surfaces every such disagreement to the user for evaluation; it never silently picks one source as authoritative and records a verdict on that basis.

This skill requires a filesystem, the source code, and the ability to run the test suite. It is Claude Code only.

## How this differs from neighboring skills

- **`dispatch:design-review`** reviews the Blueprint against itself — checking the document for gaps, inconsistencies, and unresolved questions. `review-tests` reviews *code* (the tests) against the Blueprint.
- **`review-validate`** validates a list of *already-identified bugs*, proves each with a failing test, then fixes. `review-tests` starts from the *Blueprint*, finds *untested* behavior, and writes tests to cover it. When `review-tests` discovers that the implementation doesn't actually satisfy a documented criterion, it flags that as a behavior gap — it does not fix the implementation. Handing the flagged behavior gaps to `review-validate` is a reasonable follow-up.
- **`surface-testing`** is the *philosophy* this skill enforces. The implementer phase follows it exactly. Read it; this skill assumes its rules.

## Inputs

If `$ARGUMENTS` is provided, treat it as a path to the feature directory (or to a doc inside it). Otherwise, search for feature directories under `docs/features/` in the current working directory. If more than one feature is found and none was specified, ask the user which feature to review.

Before starting, locate and read these files:
- **`blueprint.md`** — the single Blueprint in the feature directory. Required. It is the source of the acceptance criteria, scenarios, and correctness constraints to be covered. Read the whole document: its **product half** (problem, users, Correctness Constraints — state invariants and behavioral constraints, acceptance criteria, scope/non-goals), its **technical half** (interfaces, data model, failure modes, boundaries), and its optional **User Stories** section. If the feature directory still carries the older split documents (`prd.md`, `tech-spec.md`, `user-stories.md`) instead of a Blueprint, fall back to reading whichever of those exist — at least one is required.
- **`CONTEXT.md`** — the domain glossary at the service docs root (walk up from the feature directory to find `docs/CONTEXT.md`). Optional; use it to map Blueprint terminology onto code and test names. Skip if not found.
- **`review-tests.html`** — the report this skill wrote on prior runs, if present in the feature directory. Parse its embedded JSON (`<script type="application/json" id="review-tests-data">`) to recover the `runs` log and the current criteria list with each criterion's status history. Use it to (a) avoid re-flagging gaps the user already declined, (b) detect regressions where a previously-covered criterion lost its test, (c) carry forward open behavior gaps, (d) preserve the prior run history so this run appends to it rather than overwriting it, and (e) **re-examine, do not trust, any prior `untestable` / `behavior-gap` / `doc-disagreement` verdict whose `evidenceGrade` is not `test-verified`.** A reasoning-only "untestable" verdict that calcifies in the report will suppress re-investigation forever; the highest-stakes verdicts must decay toward re-scrutiny. Only verdicts backed by a test that was written and run are carried forward without re-probing.

Additionally, list files in the `adr/` directory at the service docs root. Do NOT read them upfront — pass the filenames/paths to sub-agents so they can read a specific ADR on demand when a finding needs architectural context (e.g., to confirm whether an untested path is intentionally out of scope).

Derive the `{feature}` slug from the feature directory name under `docs/features/`.

If no Blueprint (or fallback spec document) can be found, ask the user for the feature path. Do not attempt to review tests without a Blueprint to review them against — without documented criteria there is nothing to measure coverage of.

## Phase 0: Scope & Coverage Baseline

**Scope is implementation-package scoped.** Determine which packages/files implement this feature; their tests are the review scope.

1. **Locate the implementation.** Start from explicit file/package references in the Blueprint's technical half. Confirm and expand the set with a targeted search of the codebase (Grep/Glob, or a `codebase-locator` sub-agent for anything ambiguous). Produce a concrete list of implementation packages/directories and the test files in or adjacent to them.
2. **Identify the surface.** For the located code, identify the public entry points (HTTP/gRPC endpoints, CLI `Run()`, exported functions, internal library boundaries) per the `surface-testing` skill. The gap analysis is anchored to these — coverage means a test enters through the surface and asserts the behavior.
3. **Run the coverage tool once** over the implementation packages and capture the result. Detect the project type and use the idiomatic command:
   - Go: `go test ./<pkg>/... -coverprofile=/tmp/review-tests.cover -covermode=atomic` then `go tool cover -func=/tmp/review-tests.cover` (and per-line uncovered regions where useful).
   - Kotlin/JVM: the project's configured coverage (e.g. Kover/JaCoCo) if available.
   - Other: the standard coverage command for the toolchain.
   **Retain two things for the Coverage Sweep (Phase 6.5): the total coverage percentage for the scope, and the per-region uncovered list (file, line range, enclosing function).** Keep the raw profile on disk (e.g. `/tmp/review-tests.cover`) — Phase 6.5 re-reads it after the criteria tests are added.
   If no coverage tooling is configured or it fails to run, note that in the report and proceed with behavioral mapping alone — but in that case run the scope's existing test suite once anyway so its tests actually execute. A `covered` verdict may be graded `test-verified` only if the suite ran this run (the coverage run counts); if neither the coverage run nor a plain test run executed, grade `covered` as `reasoning-only` — reading a test is not the same as seeing it pass. With no coverage tooling, the Phase 6.5 sweep is skipped and the report records `coverageToolRan: false`.
4. **State the scope to the user** before spawning agents: which feature, which docs were found, which implementation packages are in scope, the baseline coverage percentage (if tooling ran), and whether coverage tooling ran. Keep it to a few lines.

**Coverage tooling is a corroborating signal and a discovery source — never a pass/fail gate.** A covered line is not the same as a covered *criterion* (a line can execute incidentally without any assertion proving the behavior), and a criterion can be legitimately covered by a test that lives in another package. Behavioral mapping decides coverage; the coverage profile points agents at suspicious untested code and guards against "I assumed a test exists." The **80% floor** introduced in Phase 6.5 is a *soft* target: still-uncovered code becomes candidate behaviors to investigate through the same surface-test discipline, and the report explains any shortfall rather than failing. The skill never brute-forces a number by writing assertion-free tests, and never tests internals to lift coverage.

## Phase 1: Criteria Extraction

Build a single normalized, numbered checklist of every testable expectation in the Blueprint. Do this yourself (orchestrator) by reading the Blueprint, or delegate to one extraction sub-agent if it is large. Each checklist item has:

- **id** — stable identifier prefixed by `kind`: `AC-` for functional, `INV-` for invariant, `CON-` for constraint, `BND-` for boundary (e.g. `AC-1`, `INV-2`). A fifth prefix, `COV-`, is reserved for coverage-derived candidates, but those are **not** extracted here — they are generated in Phase 6.5 from the uncovered-region list *after* the Blueprint criteria are implemented, so coverage measures what is still dark. Carry forward ids from a prior `review-tests.html` when the criterion is unchanged, so history lines up.
- **source** — which Blueprint section (or story) it came from (quote the relevant text).
- **text** — the expected behavior, stated as an observable condition.
- **surface** — the public entry point a test would use to exercise it (endpoint, CLI command, exported function), or `unknown` if not yet determinable.
- **kind** — one of:
  - `functional` — an acceptance criterion / scenario describing user-visible behavior.
  - `invariant` — a state invariant from the Blueprint's Correctness Constraints section (e.g. "balance is never negative").
  - `constraint` — a behavioral constraint (e.g. "messages are never silently dropped").
  - `boundary` — a component boundary precondition/postcondition.
  - `coverage` — a candidate behavior derived from an uncovered code region (assigned in Phase 6.5, not here).

Extract correctness-derived items (`invariant`, `constraint`, `boundary`) only when the Blueprint has an explicit Correctness Constraints section — these map onto the **Correctness-Derived Tests** section of the `surface-testing` skill (invariant-violation tests, behavioral-constraint tests, boundary-contract tests). The Blueprint's state invariants map to `invariant` items and its behavioral constraints to `constraint` items. Do not invent correctness constraints the Blueprint does not state.

## Phase 2: Gap-Finding (Parallel Sub-Agents)

Partition the checklist into balanced batches and spawn a few sub-agents in parallel (Agent tool, **foreground**, `model: "sonnet"`). Partition by surface area or by source document so each agent owns a coherent slice — aim for a small number of agents (≈2–4), not one per criterion. Give every agent the full list of implementation-package test files and the relevant coverage output so it can search broadly.

**Sub-agent capabilities.** Every sub-agent in this skill needs at least Read/Grep/Glob to search code and tests. The Phase 4.5 prover and the Phase 6 implementer additionally need **Write and the ability to run the project's test suite** (Bash) — they write tests and execute them. Use the default general-purpose agent (full tool access) unless your environment restricts it; if you pin a `subagent_type`, confirm it grants those tools, or the prove-it and implement phases will silently no-op.

**Sub-agent prompt structure:**

```
You are auditing whether a set of documented acceptance criteria are covered by a feature's existing tests. You determine coverage by reading the actual test code — never assume a test exists.

IMPLEMENTATION PACKAGES (review scope):
<list of packages/dirs and their test files>

SURFACE (public entry points for this feature):
<endpoints / CLI commands / exported functions identified in Phase 0>

COVERAGE TOOL OUTPUT (corroborating signal — not authoritative):
<func-level and notable uncovered regions for the implementation packages, or "coverage tooling unavailable">

CONTEXT (domain glossary): <path or "not found">
ADR FILES (read on demand if a criterion's coverage hinges on an architectural decision):
<list each ADR filename and path, or "none found">

CRITERIA TO ASSESS (your batch):
<the batch of checklist items: id, source, text, surface, kind>

For EACH criterion:
1. Search the test files for any test that exercises this behavior THROUGH THE SURFACE and asserts it.
2. Read that test's body — confirm it actually enters through the public entry point and makes a real assertion about this behavior. A test that calls internal functions directly, or that exercises the path without asserting the behavior, does NOT count as coverage (note it as a surface-testing violation).
3. Classify the criterion:
   - COVERED: a real surface test asserts this behavior. Cite the test as <file>:<TestName>.
   - PARTIAL: a test touches the behavior but misses a case the criterion requires (e.g. happy path covered, the rejection/error case is not), OR the test asserts the behavior but reaches in through internals rather than the surface. Describe precisely what is missing.
   - GAP: no test asserts this behavior. Confirm by searching the whole scope, not just the obvious file.
4. For PARTIAL and GAP, propose how to cover it through the surface: the entry point, the input, and the observable assertion. If the behavior cannot be observed through any public interface, say so and explain why (this becomes an observability/testability question).

Use this output format per criterion:

### <id>: <short title>
**Status**: COVERED | PARTIAL | GAP
**Criterion**: <text>  (source: <doc#section>)
**Surface**: <entry point>
**Existing tests**: <file:TestName, ...> or "none found"
**What's missing** (PARTIAL/GAP): <precise description>
**How to cover** (PARTIAL/GAP): <entry point + input + observable assertion>
**Observability concern** (if any): <why the behavior may not be observable through the surface>

RULES:
- Coverage means a SURFACE test with a real assertion. Be strict about the surface (see the surface-testing skill).
- Search exhaustively before declaring a GAP — the covering test may live in a sibling or parent package.
- Do not propose tests that reach into internals. If the only way to observe a behavior is through internals, that is an observability concern to flag, not a test to write.
- Do not invent criteria beyond the batch you were given.
```

From thrawn01/claude-dotfiles, MIT.

More from thrawn01/claude-dotfiles