Implementation Review

Run this before every commit, after the live-run check (/run — drive the actual app and see the change work). The goal: what was planned is implemented, what is implemented is tested, and what is tested is correct.

The checks run in subagents, never in the main agent: a subagent reads the diff without this conversation's assumptions, so a session never grades its own work. Seven subagents or one below decides how many. Collect their reports, then synthesise into the output format below.

Two checks pull in other skills' content, pass it into the brief of the subagent that runs them:

• Checks 2-4 apply /testing-philosophy: behavior over implementation detail, the two acid tests, the Testing Trophy, the e2e necessity floor, the scenario rules for acceptance examples, and the test-integrity list. Stack-agnostic. Pass that skill's content into the Check 2, 3, and 4 briefs so they judge tests against one shared bar.
• Check 7 applies /security-audit (which itself layers on /wstg): the confidence gate, false-positive precedents, three-phase methodology, WSTG mapping, and dependency audit. Pass that skill's content into the Check 7 brief.

Narrow mode. --tests-only runs Check 4 alone, as one subagent fed only Check 4's inputs, whatever the diff's size, and reports that one line plus the summary. review-pass decides when to call it (see its Depth rules).

Review target. By default the diff under review is git diff --staged. When a caller passes a target (a commit range, or the working tree vs the merge-base, which is what review-pass scopes), use that diff everywhere below instead, so a change left unstaged or committed earlier on the branch is still in view.

Seven subagents or one. Once Step 1 has the diff, count its files and changed lines, the sum of both numstat columns (added plus deleted, so 9 1 is 10 lines), with the command that matches the review target:

• --staged, the default: git diff --staged --numstat
• a commit range: git diff --numstat <range>
• the working tree vs the merge-base: git diff --numstat <merge-base>, plus every untracked file from git ls-files --others --exclude-standard, each counted as all of its lines

Leave lockfiles out. A binary file (- in numstat) counts as a file with no lines.

• At most 3 files and 50 lines: spawn one subagent that runs all seven checks in order, each against its own inputs below, and returns the seven lines.
• Anything larger: spawn seven, one per check, in parallel.

Under the report's title, say which: Subagents: 1 (N files, M lines) or Subagents: 7 (N files, M lines).

Step 1: Gather inputs (main agent)

Before spawning subagents, collect:

• The diff under review (see Review target above), full diff, the primary input for all subagents.
• Its file list.
• For Checks 2 and 4: the plan as it stood before the change (git show <base>:<plan path>), commit messages in the target range, and every test file that cites the plan's AC ids.
• The active plan, look in this order:
  1. Session context: did a planning skill (/blueprint, /to-prd, /grill-me) run earlier in this conversation? Use that output.
  2. docs/plans/*.md or docs/prds/*.md, the most recently modified file. Falling back further, docs/intent/*.md carries the grill-me design notes and edge-case decisions — thinner than a plan, but enough for Checks 1-3 to have something to reconcile against.
  3. The decision log: docs/adr/, docs/prds/, docs/development/. The foundational decision anchors mission and scope.
  4. README or CLAUDE.md scope notes.
• Full content of test files touched or related to the staged changes.
• Full content of non-test source files in the staged diff.

If no plan is findable, say so explicitly and skip Checks 1-2 in the subagent briefs.

Step 2: Dispatch the subagents

Spawn them with model: "sonnet", seven at once or one, per Seven subagents or one. Each check receives the relevant slice of inputs (below) and returns findings as Check N · [name]: ✓ clean | ⚠ [finding] | ✗ [blocking finding].

Check 1: Plan gap analysis

Inputs: full git diff --staged, the active plan.

Compare what the plan said would be built against what the diff implements. For each plan item: in the diff → ✓ implemented; partially there → ⚠ partial, name what is missing; absent → ✗ gap, deferred or forgotten?

Ask: would a reader of the plan consider this commit "phase complete"? If the plan defined a gate ("Done when: AC-1 to AC-4 pass", or "ATs #1-4" in older plans), does the commit satisfy it, with a test for each of those ACs present and passing? If nothing in the inputs shows those tests ran, report the gate as unverified, not met. A deferred item is not a gap, but it must be explicitly deferred, not silently absent.

Check 2: Use-case coverage

Inputs: full git diff --staged, the foundational scope doc (docs/prds/*, docs/adr/*, docs/development/*, or CLAUDE.md), README feature description, the active plan (for its AC ids and phase gates), and every test file that cites those ids, changed or not (search the repo for them). Apply /testing-philosophy.

For every user-facing capability in scope for this commit: is there a test that exercises it through the public API? A code path that implements it? Is the critical happy path covered end-to-end through the real assembled system (the e2e floor)?

Numbered acceptance criteria first. When the plan numbers ACs (AC-n), reconcile them before anything else: every AC example in this phase's gate needs a test, at the tier the example (or its rule) names, whose name or docstring carries the id in whatever form the language allows (AC-1a, ac_1a, AC1a). List each AC with no test, and each test that cites an AC id it doesn't exercise. An AC still marked (proposed) gets a ⚠: the code is being held to examples no human has approved yet.

Different from Check 1: use cases can be implicit in the product scope even if the plan did not spell them out. Ask "what would a consumer of this code reasonably expect to be able to do?" Flag any use case with an implementation but no test, a test but no implementation, or a user-facing happy path with no end-to-end coverage.

Check 3: Missing test scenarios

Inputs: full git diff --staged, full content of changed test files, the plan's ## Edge Cases table if present (plans written before the rename call it ## The Blind Spots; accept either). Apply /testing-philosophy.

First, reconcile against the ledger. If the plan carries a Blind Spots table, every row is a claim to check:

• handle → is the case actually implemented, and is there a test that fails without it? Implemented-but-untested is a finding. Neither is a ✗.
• defer / won't → is it still absent? Silently implemented is fine but note it. Silently half-implemented is a ✗ — a partial path is worse than none.
• Anything the diff introduced that no row covers → new blind spot, flag it and name the row that should have existed.

No Blind Spots table (bug tier, or the plan predates it) → say so and derive from scratch below.

Then, for every behavior introduced or changed, work the scenario checklist:

Not every category applies to every change, exercise judgment, but do not skip a category without a reason.

Check 4: Test philosophy and integrity (Testing Trophy)

Inputs: full content of changed test files; the test-file hunks of the diff, since a deleted test or an edited expected value only shows there; the source diff, read only for test-aware code; the plan as it stood before this change; and any PR body or commit messages in range. Apply /testing-philosophy. A violation is a finding.

• Test integrity: apply testing-philosophy's integrity tells. This check never clears a tell itself, because every reason it could read can be written by the agent under review. For each tell, report what changed plus any reason in view (an AC or plan entry that changed, a commit message, the PR body, a comment): with a reason → ⚠ needs-a-human, quoting the reason so the user can confirm it in one look; with none → ✗. Both block the commit until the user confirms.
• Scenario level: an acceptance or Given/When/Then test that scripts the UI (clicks, selectors, field names) or checks more than one rule → flag. Run the scenario acid test from testing-philosophy.
• Behavior over implementation: does the test name describe a behavior ("rejects an out-of-range index with RangeError") or a mechanism ("calls chooseIndex with the given index")? Does it assert the observable result or how it was produced?
• Public API only: accessing private fields, _inner, unexported functions, or internal state → violation. Asserting an internal function was called (spy / mock-call-count on an owned collaborator) → violation. Mocking types it owns → violation. Mocking only genuine external boundaries → ✓.
• Real collaborators where cheap: anything mocked that could be a real instance without significant cost → flag.
• Refactor-proof: would it break on an internal rename with the public contract unchanged, or on an internal data-structure change with the return value unchanged? → violation.
• Trophy shape (both directions): more unit than integration for cross-unit behavior → flag; something at E2E level that fits cheaper at integration → flag (overuse); a user-facing critical path with no e2e at all → flag (the quieter, more common gap, coordinate with Check 2). A snapshot that will be rubber-stamped on update → flag.
• Test names: it("works"), it("test 1"), it("should work correctly") → violation. Specific behavior + expected error → ✓.

Check 5: SOLID violations

Inputs: non-test source files from the staged diff only.

• Single Responsibility: a class or module with more than one reason to change (validates and persists, formats and dispatches). Flag if methods cluster into two distinct concern groups.
• Open/Closed: adding a variant requires editing existing files (a switch/if-else chain on a type tag in the caller). Flag if extension requires modification rather than addition.
• Liskov: a subtype that throws where its base does not, or ignores a method the base defines. Flag a narrower contract.
• Interface Segregation: an interface that forces implementors to define methods they do not use. Flag fat interfaces.
• Dependency Inversion: new ConcreteType() inside a class body where injection or an abstraction would be natural. Flag it.

Check 6: Clean Code violations

Inputs: full git diff --staged (test and non-test).

Check 7: Security

Inputs: full git diff --staged, file list, the lockfile diff if a lockfile changed.

Pull in the full content of /security-audit and apply it to the staged diff. That skill carries the confidence gate, the three-phase methodology, the false-positive precedents, the dependency audit, and it maps findings to WSTG IDs via /wstg.

The one rule: only report a finding with a concrete exploit path, and only when over 80% confident it is actually exploitable. Review what the change newly introduces, not pre-existing issues the diff sits near. When a lockfile changed, run the repo's native auditor (detect it from the lockfile, see the table in /security-audit) scoped to the changed packages, degrade to an advisory flag if unavailable. Return findings in the house line format. HIGH and MEDIUM block; LOW is advisory.

Step 3: Synthesise (main agent)

Merge the seven check reports, in this format:

Implementation review, [commit subject or staged file summary]
Subagents: [1 or 7] ([N files, M lines])

Check 1 · Plan gaps
  ✓ All phase items present  |  ✗ Gap: [item], [present/partial/absent]
Check 2 · Use-case coverage
  ✓ All use cases covered  |  ✗ [use case], no test / no implementation
Check 3 · Missing test scenarios
  ✓ Scenarios complete  |  ⚠ [behavior]: missing [scenario type]
  Ledger: [N handled / N deferred / N drifted]  |  ✗ [case]: planned [decision], shipped [reality]
Check 4 · Test philosophy and integrity
  ✓ Tests pass  |  ✗ [test name]: [violation]  |  ⚠ integrity: [test or file:line], [tell], reason: [pointer], confirm  |  ✗ integrity: [test or file:line], [deleted / skipped / weakened / expected edited / test-aware code], no reason
Check 5 · SOLID
  ✓ No violations  |  ✗ [file:line]: [principle], [finding]
Check 6 · Clean Code
  ✓ No violations  |  ✗ [file:line]: [issue]
Check 7 · Security
  ✓ No vulnerabilities  |  ✗ [file:line]: [category] ([severity], WSTG-ID), [exploit] + fix  |  ⚠ [lockfile / auditor]

Summary: [N findings, fix before committing / Clean, proceed]

Blocking (fix before committing unless the user overrides): a plan gap, a philosophy violation on a public-API test, any test-integrity tell the user hasn't confirmed, a SOLID violation that breaks extensibility, a HIGH or MEDIUM security finding. Advisory (surface, do not block): an unclear test name, a slightly long function.

Flow position

[code changes complete]
     ↓
/run                       ← does the change actually work end-to-end
     ↓
/implementation-review     ← THIS SKILL, seven checks in subagents
     ↓
git commit
     ↓
/pr-craft                  ← open the PR

Anti-patterns

• Do not skip a check because "the change is small", that is when violations sneak through.
• Do not invent a plan if none is findable, skip Checks 1-2 and say so.
• Do not treat every advisory finding as blocking, use judgment.
• Do not apply Check 4's philosophy rules to non-test files (its integrity rule reads the source diff only for test-aware code), or run Check 5 on test files.
• Do not report a Check 7 finding without a concrete exploit path.
• Do not fail Check 7 when the dependency auditor is missing, degrade to advisory.
• Do not report "✓ clean" without the subagent actually reading the diff.
• Do not run the checks in the main agent, however small the diff. See Seven subagents or one.