Tdd Review

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:

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

After RED — adversarially review the test

For an executable-red independent review, treat execution_attestation as the only execution evidence. Refuse author-pasted output, a missing attestation, or an attestation whose source fingerprint does not bind the supplied scenario, proof-plan row, primary proof target, and declared support files. Approval requires all of the following:

• the exact attested command failed without timing out;
• the expected-failure literal matched the attested full stdout or stderr stream;
• the named proof exercises the scenario's actor-facing When and observes its actor-visible Then;
• the observed failure is the intended missing behavior at that boundary.

Request changes when the failure is caused by syntax, imports, fixtures, configuration, infrastructure, an unrelated actor-boundary assertion, a passing command, timeout, or signal. Do not infer execution from cached suite status or the author's account. Explain the mismatch and name the exact proof or environment correction to make before retrying.

GREEN credit is fail-closed: the shared edit hook calls review gate executable-red for the active scenario and permits the checkbox only for a fresh approved cross-agent receipt. An unavailable reviewer or author-only fallback is not approval; leave GREEN unchecked and report the named recovery action.

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.
• Already exists? Only when the step added shared surface — a helper, util, or type others would reach for. Grep for one before keeping yours; prefer the stdlib or an installed dependency over either. A duplicate gets fixed once and stays broken twice.
• 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.