# Tdd Review

> Quality check for a completed TDD step. Runs when safeword's TDD flow finishes RED, GREEN, or REFACTOR; not a user-facing entry point.

- Skill: `arcadeai/tdd-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add arcadeai/tdd-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/arcadeai/tdd-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: ArcadeAI (https://skillmd.com/u/arcadeai)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/arcadeai/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:

| 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

<!-- SAFEWORD:EXECUTABLE_RED_RUBRIC_START -->

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.

<!-- SAFEWORD:EXECUTABLE_RED_RUBRIC_END -->

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.