tdd-review

name	tdd-review
description	Use when completing a TDD step and wanting a quality check. Reviews test quality after RED, implementation correctness after GREEN, and scenario completeness after REFACTOR.
allowed-tools	*

TDD Review

Step-aware quality review at TDD phase boundaries. Run it as an internal self-check after RED, GREEN, and REFACTOR during implement phase.

These per-step reviews are advisory self-checks — the only hard gates in the implement phase are the commit ledger (test-definitions.md annotations) and the done-gate. Use these reviews to catch problems early; don't treat them as blocking walls.

Stakes set depth. Advisory means it won't block you — not that it can be shallow. The done-gate only runs tests, so a bug your eyes miss here ships. Review each step as if no later gate re-reads this code.

Visibility: ordinary RED/GREEN/REFACTOR reviews stay quiet. Do not surface a chat-facing review after each checkbox flip unless you found a real blocker, a user/scope decision, or a risky external dependency/API finding. Report the review/refactor work in the implementation-exit summary.

Detect Step

Key the review off the last checked box in the current scenario — that's the step you just completed:

Last checked (step just done)	Review focus
RED (test written)	Review test quality
GREEN (implementation passes)	Review implementation
REFACTOR (cleanup done)	Review completed scenario

Depth scales with the step: lightweight after RED, moderate after GREEN, full after REFACTOR.

After RED — adversarially review the test

Focused review (~1 minute). Check the test that was just written:

• Atomic? Tests ONE behavior. Red flag: multiple When/Then pairs.
• Right assertions? Meaningful expectations, not .toBeTruthy() or .not.toThrow().
• Behavior, not implementation? Tests observable outcomes. Red flag: mocking internals, checking call counts.
• Scenario fidelity? For a BDD scenario, does the primary test exercise the When through the actor-facing entry point and observe the Then as the actor-visible result? Setup shortcuts belong in Given; direct store calls, injected lower-level events, and internal-state assertions are supporting evidence only.
• Fails for the right reason — confirmed by the run, not the eye? Execute the test now and read the actual failure: it must report the missing behavior, not a syntax, import, or setup error. This is the one bullet you don't judge by reading — the run is the evidence.
• Right test type? Load the testing skill and consult its scope hierarchy (E2E > Integration > Unit). Was a higher-scope test practical here? Did we drop to unit when integration would catch more?
• Coverage adequacy? Consult testing guide's bug detection matrix. Ask: "What could still break that this test wouldn't catch?" Flag gaps — missing edge cases, error paths, or boundary values. Where a gap goes: a missing scenario is a scope change, not a mid-implement edit — defer it to a follow-up ticket, or loop back to define-behavior, re-run the scenario-gate, and re-enter plan-implementation (update impl-plan.md for the new scenario) before implement. Don't silently append scenarios to a signed-off test-definitions.md.
• Proof matches the claim? Could this test pass while the user-facing claim remains broken? Challenge the actual test boundary, not its title: a same-process test cannot prove caller-exit survival, an injected fake cannot prove real CLI wiring, and a unit test cannot prove a runtime or protocol boundary.

Vacuity guard (the external check). A test that would pass without the feature proves nothing. The RED→GREEN transition is the proof it's wired to the behavior: it must be failing now, and the minimal implementation is what turns it green. If a test ever passes before its implementation exists, it's vacuous — fix the test, not the code.

If issues found: fix before implementing. If clean: commit and proceed to implementation.

After GREEN — review the implementation

Moderate review (~1-2 minutes). Check the implementation:

• Minimal? Only code the test requires. No anticipatory design.
• Correct? Does it actually satisfy the test's intent, not just make it pass by coincidence?
• No regressions? Run the targeted suite for the module under test. The full-suite regression check belongs once per scenario, at scenario close (after REFACTOR) — not at every GREEN.
• Run /refactor for structural cleanup.

If issues found: fix before refactoring. If clean: run /refactor, commit, proceed.

After REFACTOR — review the completed scenario

Full review (~2-3 minutes). The entire scenario is done. Review the complete unit:

• Test + implementation alignment? Does the test cover the scenario's Given/When/Then? This is a local review — no web research needed.
• Scenario fidelity? Does the completed proof still use the scenario's actor-facing entry point and observe its actor-visible result, with implementation-level checks kept as supporting evidence?
• Full suite green? Run the full suite once here to catch cross-module regressions.
• New external dependency or API in this scenario? Only then run /quality-review for ecosystem verification (versions, deprecated APIs, security) — verify it at the moment you introduce it, before later scenarios build on it. A scenario that adds no new third-party/external surface (internal modules, stdlib, or a second use of an already-reviewed dep don't count) skips this; the whole-ticket pass at implement-exit is the catch-all.
• Ready for next scenario? Any loose ends or technical debt to note?

If issues found: address before starting next scenario. If clean: commit and proceed to next [ ] RED.

Concrete example (after RED — reviewing the test)

Context: Agent just wrote a failing test for scenario 2 (verbose shows passing files) — RED is the last checked box, so the review focuses on the test.

Illustrative only — apply the checks in your own words, not this exact wording:

Atomic? Yes — tests one behavior (passing files appear in verbose output). Right assertions? expect(output).toContain('src/index.ts: pass') — asserts on observable output, not internals. Good. Behavior, not implementation? Yes — checks CLI output string, not internal formatter state. Fails for the right reason? Expected string to contain 'src/index.ts: pass' — verbose output doesn't include passing files yet. Correct failure. Right test type? Integration — needs real linter execution to produce output. Unit wouldn't cover the formatter→CLI pipeline. Correct scope. Coverage gaps? Test only checks one file. Could miss edge case where zero files pass. Acceptable for now — boundary cases are in scenario 3.

Clean.

Next: implement minimum code in packages/cli/src/lint.ts to make expect(output).toContain('src/index.ts: pass') pass, then commit.

Output discipline

Close every review with a **Next:** line — imperative, naming the file/command/scenario, not a bare "proceed" or "commit and move on" (the stop hook reads it for the re-entry brief).

• After RED, clean → **Next:** implement minimum code in {file} to make this test pass.
• After RED, issues → **Next:** rewrite assertion in {file}:{line} to check observable output, then re-review.
• After GREEN, clean → **Next:** run /refactor on {file}, then commit and mark next [ ] RED.
• After REFACTOR, clean → **Next:** commit, then start scenario {N} — write the failing test in {file}.

Voice: plainspoken and concise — write to be scanned.