/implementation-review

name: implementation-review
description: "Pre-commit quality gate. Invoke before every git commit, after /flagrare:staleness-audit. Seven checks, plan gaps, use-case coverage gaps, missing test scenarios, test philosophy violations (Kent Dodds Testing Trophy), SOLID violations, Clean Code violations, and security vulnerabilities. Each check is delegated to a parallel subagent. Surfaces findings before they land in history. Also invoke when the user says review this, am I done, did I miss anything, or check the quality."

Implementation Review

Run this before every commit, after `/flagrare:staleness-audit`. The goal: **what was planned is implemented, what is implemented is tested, and what is tested is correct**.

Each of the seven checks is run by a dedicated subagent. Spawn all seven in parallel, collect their reports, then synthesise into the output format below.

**REQUIRED BACKGROUND for Checks 2-4:** the test-related checks apply `/flagrare:testing-philosophy` (behavior over implementation, the Testing Trophy, the e2e necessity floor). Pass that skill's content into the Check 2, 3, and 4 subagent briefs so they judge against the same definition of "good test" the planning side uses.

**REQUIRED BACKGROUND for Check 7:** the security check applies `/flagrare:security-audit` (threat taxonomy, three-phase methodology, confidence gate, false-positive precedents, generic dependency audit). Pass that skill's content into the Check 7 subagent brief so it judges against the same security discipline a standalone `/flagrare:security-audit` run uses.

---

Step 1: Gather inputs (main agent)

Before spawning subagents, collect in the main agent:

• `git diff --staged`, full diff (the primary input for all subagents)
• `git diff --staged --name-only`, file list
• The **active plan**: look in this order:

1. Session context: did `/flagrare:atdd-plan` run earlier in this conversation? Use that output. 2. `~/.claude/plans/*.md`, the most recently modified file, if the directory exists. 3. The project's decision log (`docs/decisions/`, `docs/adr/`, RFCs, or equivalent), the foundational decision is always the mission/scope anchor. 4. README roadmap, checked/unchecked items define what is in scope.

• 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 seven subagents in parallel

Spawn all seven subagents simultaneously using `model: "sonnet"`. Each subagent receives the relevant slice of inputs (described in each brief below) and returns findings in the format `Check N · [name]: ✓ clean | ⚠ [finding] | ✗ [blocking finding]`.

Do not run checks sequentially in the main agent. Spawn → collect → synthesise.

---

Subagent brief: Check 1: Plan gap analysis

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

Compare what the active plan said would be built against what the staged diff actually implements.

For each item in the plan's "Implementation Phases" or task list:

• Is it in the staged diff? → ✓ implemented
• Is it partially there? → ⚠ partial, what is missing
• Is it absent entirely? → ✗ gap, was it intentionally deferred, or forgotten?

Ask: would a reader of the plan consider this commit "phase complete"? If the plan defined a gate ("Phase N is done when ATs #1-4 pass"), does the commit satisfy it?

Flag every gap. A deferred item is not a gap, but it must be explicitly deferred, not silently absent.

---

Subagent brief: Check 2: Use-case coverage

**Inputs:** full `git diff --staged`, the project's foundational scope document (ADR-0001, RFC-001, mission doc, or equivalent, whichever anchors what this project is for), README feature description.

For every **user-facing capability** in scope for this commit:

• Is there at least one test that exercises it through the public API?
• Is there a code path that implements it?
• **Is the critical happy path covered end-to-end through the real, assembled system?** Per the e2e necessity floor in `/flagrare:testing-philosophy`, a user-facing feature with unit + integration coverage but no e2e/full-stack proof that the layers connect is a gap. E2e generalizes by surface: a browser journey for a UI, running-service-over-HTTP-against-a-real-DB for a backend, a subprocess invocation for a CLI, public-API-as-a-consumer for a library. One or two critical paths suffice, but zero is a finding.

This is 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 that has an implementation but no test, a test but no implementation, or a user-facing happy path with no end-to-end coverage.

---

Subagent brief: Check 3: Missing test scenarios

**Inputs:** full `git diff --staged`, full content of changed test files.

For every behavior introduced or changed in the staged diff, work through this scenario checklist:

| Scenario type | Question to ask | |---|---| | Happy path | Is the basic success case tested? | | Empty / nil / zero | What happens when the input is empty, null, zero, or absent? | | Boundary | First item, last item, exactly one item, max capacity? | | Invalid input | Malformed, out-of-range, wrong type, is the rejection tested? | | Error path | For every success path, is the corresponding failure path tested? | | Idempotency | If the operation can be called twice, is that safe? Is it tested? | | Order sensitivity | Does the result depend on call order? Is that documented and tested? | | Concurrent access | If the code will be called from multiple threads/tasks, is that safe? Is it tested? (Only if relevant.) |

Flag missing scenarios. Not every category applies to every change, exercise judgment, but don't skip a category without a reason.

---

Subagent brief: Check