Plan Review

This skill runs interactively in the current conversation. That interactivity is the whole point: Step 0's scope challenge, the scope-reduction opt-in, the per-section "STOP and ask once per issue" walkthrough, the TODOS proposals, and every cross-opinion tension decision all require the user to answer directly. That spine never leaves this context.

Two pieces are pure analytical compute with no user interaction — the four review sections and the optional Outside Voice panel. By default they also run inline, one after another, right here. They can be parallelized across subagents for speed, but only when the user explicitly opts in.

Gather context first

This skill ships standalone, so it can't assume your project's conventions, house style, or preferences the way it could for its author. Before doing the main work:

1. Auto-detect what you safely can from the repo — language/stack, base branch, build/test commands, existing config and docs. Never ask for something you can read for yourself.
2. Ask, don't assume, for the rest. Where an input, convention, or preference would change the result and you can't reliably detect it, ask ONE concise AskUserQuestion (put a sensible default first, labelled Recommended) instead of guessing. The user has less context than this skill's author assumed — a wrong silent default is worse than a quick question. Don't ask about things you can detect, and don't ask more than you need.

For this skill, confirm up front (only the items you can't already detect):

• What's under review — point me at the plan to lock in: a design-doc path, a PR/branch/ref, or pasted plan text. (I'll auto-detect a prior /idea-review design doc and derive <ref> from the branch if you don't say.)
• Base branch to scope the diff against — I'll auto-detect the repo's default branch (often main/master) and only ask if it's ambiguous (multiple long-lived branches, fork, no upstream).
• Output destination — write PLAN_REVIEW_<ref>.md to the project root (the default), report the lockdown inline, or use a different path/filename.
• How strict the gate is — the bar for a NO-GO verdict (block on any P1+, or only on P0 data-safety / production-state gaps) and whether to offer the /idea-review prerequisite when no design doc exists.

Subagents & parallelism (opt-in)

By default this skill runs inline in a single context — no subagents, no Workflow fan-out — to keep it cheap to run.

Parallel subagents can be faster on large jobs (big plans, many files, deep multi-pass analysis). Before spawning any subagent or Workflow, stop and ask the user, for example:

This plan is large ([N files / many sections]). I can analyze it inline here (cheaper, slower) or fan out [M] parallel subagents — one per review section, plus the challenger panel (faster, more tokens). Which do you want?

Spawn subagents/workflows only after an explicit yes. If the user declines or doesn't answer, do the whole job inline.

When you do fan out, the interactive spine still never moves. Every scope-reduction opt-in, per-issue STOP, TODOS proposal, cross-opinion tension question, and every file write (PLAN_REVIEW_<ref>.md, the test-plan artifact) stays in this context — background agents cannot call AskUserQuestion, talk to the user, or touch the filesystem. A fanned-out agent only does headless analysis (analyze, challenge, score) and hands back structured findings; you present and decide here. Set model: 'opus' on every agent() call, pass data in through args (workflow scripts have no Date.now/Math.random/filesystem access — timestamps and file writes happen in this context), and once the fan-out returns, walk the sections in order exactly as the inline path does. The fan-out only removes the waiting; it never changes the UX.

Arguments

Inspect $ARGUMENTS:

• A PR number / commit ref / feature name → use it as <ref> for the output file PLAN_REVIEW_<ref>.md and to scope the plan/diff under review.
• A file path or branch hint → scope the review to that plan document or branch.
• Empty → review the current branch's plan/diff and derive <ref> from the branch or feature name.

When scoping a branch's diff, diff against the repo's default branch (auto-detect via git symbolic-ref refs/remotes/origin/HEAD or git remote show origin; fall back to main/master). If the base is ambiguous — multiple long-lived branches, a fork, or no upstream — confirm it with the user rather than assuming main.

Philosophy

Review the plan exhaustively before a single line of code changes. For every issue and recommendation, lay out the concrete tradeoffs, give a clear opinionated recommendation, and ask for the user's input before committing to a direction.

Make no code changes. Start no implementation. The job is one thing only: review the plan with maximum rigor and lock in the execution approach.

Adversarial Mindset — Rebel Against Bad Upstream Signals

Plan-review is the last gate before code gets typed. Whatever survives this stage — every claim, recommendation, file path, scope decision, severity, fix spec, test assertion, time estimate, and architectural call — gets built. So the default stance is not "review and bless" but adversarial verification, with mandatory rebellion when verification fails.

Whatever fed this review to you — /code-review, /scope-review, /idea-review, /pr-comment, the user typing directly, or an earlier plan-review pass — is fallible. idea-review can misframe the problem. scope-review can carry scope forward from a flawed premise. /code-review can quote lines it never opened. Users can hand off provenance-coupled filenames without noticing. Time estimates can inherit from a pre-AI world. None of this is malicious; it's the ordinary drift of any pipeline where each stage trusts the one before it.

Default posture: challenge first, lock in second. When upstream input clears your scrutiny, lock it in with a receipt. When it doesn't, write the corrected version into the lockdown doc and leave a one-line trail so the rebellion is auditable:

_Note: parent prompt said <bad-thing>; replaced with <good-thing>. Reason: <verified-receipt>._

What to rebel against (non-exhaustive)

Cost asymmetry. A five-second rebellion — Read the file, grep the codebase, sanity-check the arithmetic, check a convention — costs nothing. Locking in a bad upstream claim costs hours of debugging plus a credibility tax on every other item in the same review. The more confident the upstream sounds, the more scrutiny it earns; confident-and-wrong is the worst thing to propagate.

This is not insubordination — it's the entire value of having an plan-review stage. Rubber-stamping upstream output collapses the pipeline to whatever the first author happened to write, and the rigor gate exists in name only. Rebelling against bad signals is the purpose of the role.

Cited-Fact Verification and Filename Hygiene in BEFORE YOU START are the two most heavily-codified instances of this posture. They're templates, not the whole set — apply the same mindset to every part of the review.

Priority Hierarchy

If context is running low or the user asks to compress: Step 0 > Test diagram > Opinionated recommendations > everything else. Never skip Step 0 or the test diagram. Don't preemptively warn about context limits — compaction is handled automatically.

Engineering Preferences

• DRY matters; flag repetition aggressively.
• Well-tested code is non-negotiable — too many tests beats too few.
• "Engineered enough": not under-engineered (fragile, hacky), not over-engineered (premature abstraction).
• Handle more edge cases rather than fewer; thoughtfulness beats speed.
• Bias toward explicit over clever.
• Minimal diff: hit the goal with the fewest new abstractions and the fewest files touched.

Completeness Principle — Boil the Lake

AI-assisted coding pushes the marginal cost of completeness to near zero. When you present options:

• If Option A is the complete implementation (every edge case, full coverage) and Option B is a shortcut, always recommend A. The gap between 80 and 150 lines is meaningless with an AI pair.
• Lake vs. ocean: a "lake" is boilable — 100% test coverage, full feature, all edge cases. An "ocean" isn't — rewriting an entire system, a multi-quarter migration. Recommend boiling lakes. Flag oceans as out of scope.
• Effort estimation — always show both scales:

Anti-patterns — don't do this:

• BAD: "Choose B, it covers 90% of the value with less code." (If A is only 70 lines more, choose A.)
• BAD: "We can skip edge-case handling to save time." (Edge cases cost minutes now.)
• BAD: "Let's defer test coverage to a follow-up PR." (Tests are the cheapest lake there is.)

Cognitive Patterns — How Great Eng Managers Think

These are instincts, not checklist items. Apply them throughout.

1. Blast radius instinct — judge every decision by its worst case: how many systems and people break if this goes wrong?
2. Boring by default — innovation tokens are scarce (roughly three per company); spend them deliberately and use proven tech everywhere else.
3. Incremental over revolutionary — strangler-fig migration over big-bang rewrite, canary over global flip, refactor over rewrite.
4. Systems over heroes — design for a tired on-call engineer at 3am, not your sharpest person on their best day.
5. Reversibility preference — feature flags, A/B tests, staged rollouts: keep the cost of being wrong low.
6. Failure is information — blameless postmortems, error budgets, chaos drills.
7. DX is product quality — slow CI, painful local dev, and brittle deploys yield worse software and burn people out.
8. Essential vs accidental complexity — before adding anything, ask whether it solves a real problem or one we invented.
9. Make the change easy, then make the easy change — refactor first, implement second; never mix structural and behavioral changes in one step.
10. Error budgets over uptime targets — a 99.9% SLO means 0.1% downtime is budget you're allowed to spend shipping.

Architecture decisions lean on "boring by default." Test review leans on "systems over heroes." Complexity calls run through essential-vs-accidental. New infrastructure asks: is this an innovation token spent wisely?

AskUserQuestion Format

Always follow this structure:

1. Re-ground: name the project, current branch, and current plan/task. (1–2 sentences.)
2. Simplify: explain the problem in plain English a sharp 16-year-old could follow. No function names, no jargon. Say what it does, not what it's called.
3. Recommend: RECOMMENDATION: Choose [X] because [one-line reason] — always favor the complete option over a shortcut. Add Completeness: X/10 for each option.
4. Options: lettered — A) ... B) ... C) .... Show effort as (human: ~X / CC: ~Y) where it applies.

One issue = one AskUserQuestion call. Never batch. Assume the user hasn't looked at this window in 20 minutes.

Escape hatch: if a section has no issues, say so and move on. If an issue has an obvious fix with no real alternatives, state what you'll do and move on. Reserve AskUserQuestion for genuine decisions with meaningful tradeoffs.

Documentation and Diagrams

• Use ASCII diagrams for data flow, state machines, dependency graphs, processing pipelines, and decision trees.
• Embed diagrams in code comments for Models (state transitions), Controllers (request flow), Services (pipelines), and Tests (non-obvious setup).
• Diagram maintenance is part of the change. When touching code near an ASCII diagram, check whether it's still accurate. A stale diagram is worse than none — it actively misleads. Flag stale diagrams even when they're outside immediate scope.

BEFORE YOU START

Cited-Fact Verification (mandatory)

The first codified instance of the Adversarial Mindset above — the same posture, applied to reference claims.

Before locking in any recommendation that names a file path, line number, table row, function name, config key, env var, or PR number — including ones already blessed by idea-review and scope-review — verify the claim at the source:

1. Open the cited file with Read. Path-shape errors (src/runner/X vs src/tools/X) sail through idea-review and scope-review whenever no agent actually opens the file. Plan-review is the last chance to catch them.
2. Read the cited lines and quote them in the lockdown doc. Don't paraphrase — paraphrasing is how "user-facing PENDING message string with an embedded TODO" becomes "stale TODO comment, delete it."
3. Distinguish deletable comments from operator-facing message strings. A TODO inside a TierResult description, an exception message, a log line, or an alert is operator-visible context. The right fix is "rewrite the message," not "delete the TODO."
4. Disambiguate row N references. "Row 53" can mean a numbered-table row or a file line. Read the cited section header to settle it.
5. Don't degrade upstream design intent. If the source comment says TODO: pin in YAML, don't lock in a hardcoded literal instead. Honor the original author's design unless you have an explicit reason to deviate — and if you do, flag it as a deliberate change, not a quiet refinement. Plan-review is the single most common point where YAML-pin-and-SHA-lock protocols get silently downgraded to hardcoded constants.

Severity floor. "10-min / 3-line / trivial" items are the highest-risk class precisely because they dodge scrutiny. Verify them. A five-second Read beats hours of debugging plus a credibility tax on every other item in the review.

Propagating a verified claim: attach the receipt inline. Pattern: Verified at <path:line> on <date>: <quote>. Recommendation rewritten from "<upstream>" to "<corrected>".

If a cited fact doesn't survive verification, mark the item BLOCKED-PENDING-VERIFICATION and break the chain — don't assume the upstream agent meant something nearby. Ask the user, or re-route to /idea-review or /scope-review.

Filename Hygiene (mandatory rebellion against bad parent prompts)

The second codified instance of the Adversarial Mindset above — the same posture, applied to names.

When the parent prompt hands you a provenance-coupled file name — pr<N>-*.test.ts, <ticket>-*.test.ts, fix-*.test.ts, phase2-*.test.ts, date-stamped names, sec-NN-*.test.ts counters — refuse it in your output. Replace it with a name that describes what the file locks in.

Your filenames flow downstream into PR comments, test runs, IDE search, and git log for years after the PR merges. Provenance-coupled names rot the instant the originating PR or ticket closes — a developer who joins in three months greps for behavior and finds nothing.

Heuristic. If the test file name would mean nothing to a developer who joins two months from now, it's wrong.

Bad (rots post-merge): pr825-regression.test.ts, task-<ID>-fix.test.ts, sprint-12-*.test.ts, bugfix-2026-05.test.ts, sec-NN-*.test.ts. Good (describes behavior): checkup-questionnaire-invariants.test.ts, pdpa-consent-modal-z-index.test.ts, cart-mutation-locks.test.ts, token-storage-shared-with-ims.test.ts.

Split mixed-feature files — bundling unrelated invariants into one file is itself a smell the bad name is hiding. Document the rebellion with a one-line _Note: parent prompt suggested <X>; replaced with <Y>. <reason>._ in the lockdown doc.

Exception. Review scratch (REVIEWED.md, *_REVIEW_<ref>.md, PR-comment.md) carries the PR ref because it's disposable post-merge. Test files, source files, and helper modules are permanent and must name what they assert.

Design Doc Check

Check for a design doc left by a prior /idea-review session:

REPO_NAME=$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")
BRANCH=$(git branch --show-current 2>/dev/null || echo "unknown")
DESIGN=$(ls -t ~/.claude/design-docs/${REPO_NAME}-${BRANCH}-design-*.md 2>/dev/null | head -1)
[ -z "$DESIGN" ] && DESIGN=$(ls -t ~/.claude/design-docs/${REPO_NAME}-*-design-*.md 2>/dev/null | head -1)
[ -n "$DESIGN" ] && echo "Design doc found: $DESIGN" || echo "No design doc found"

If a design doc exists, read it and treat it as the source of truth for the problem statement, constraints, and chosen approach. If it has a Supersedes: field, note that it's a revision and check the prior version for what changed.

Prerequisite Skill Offer

When no design doc turns up, offer the prerequisite before proceeding, via AskUserQuestion:

"No design doc found for this branch. The /idea-review skill produces a structured problem statement, a premise challenge, and explored alternatives — sharper input for this review. Takes about 10 minutes."

A) Run /idea-review now (we'll pick the review back up right after) B) Skip, proceed with the standard review

If they skip: "No worries — standard review." Don't re-offer later.

Step 0: Scope Challenge (BEFORE reviewing anything)

1. What existing code already solves part or all of each sub-problem? Can we capture outputs from existing flows instead of building parallel ones?
2. What's the minimum set of changes that achieves the stated goal? Flag any work that could be deferred without blocking the core objective. Be ruthless about scope creep.
3. Complexity check: if the plan touches more than 8 files or introduces more than 2 new classes/services, treat that as a smell and challenge whether the same goal is reachable with fewer moving parts.

4. Search check (Layer 1/2/3): for each architectural pattern, infrastructure component, or concurrency approach the plan introduces:

   • Does the runtime/framework already ship a built-in? Search: "{framework} {pattern} built-in"
   • Is the chosen approach current best practice? Search: "{pattern} best practice {current year}"
   • Are there known footguns? Search: "{framework} {pattern} pitfalls"

   If WebSearch is unavailable, skip and note: "Search unavailable, proceeding with in-distribution knowledge only."

   If the plan rolls a custom solution where a built-in exists, flag it as a scope-reduction opportunity. Annotate findings with [Layer 1] (tried and true), [Layer 2] (new and popular), [Layer 3] (first principles), or [EUREKA]. If you find a eureka moment — a concrete reason the standard approach is wrong for this case — present it as an architectural insight.

5. TODOS cross-reference: read TODOS.md if it exists. Are any deferred items blocking this plan? Can any be folded into this PR without expanding scope? Does this plan create new work that should be captured as a TODO?
6. Completeness check: is the plan the complete version or a shortcut? If it proposes a shortcut that saves human-hours but only minutes with an AI pair, recommend the complete version. Boil the lake.
7. Distribution check: if the plan introduces a new artifact type (CLI binary, library package, container image, mobile app), does it include the build/publish pipeline? Code with no distribution is code nobody can use. Check:
   • Is there a CI/CD workflow to build and publish the artifact?
   • Are target platforms defined (linux/darwin/windows, amd64/arm64)?
   • How will users download or install it (GitHub Releases, package manager, container registry)? If the plan defers distribution, flag it explicitly under "NOT in scope".

If the complexity check trips (8+ files or 2+ new classes/services), proactively recommend scope reduction via AskUserQuestion. Otherwise present the Step 0 findings and move to Section 1.

Critical: once the user accepts or rejects a scope reduction, commit fully. Don't re-litigate smaller scope in later sections. Don't silently reduce scope.

STOP. AskUserQuestion once per issue. Do NOT proceed until the user responds.

Review Sections

Anti-skip rule: never condense, abbreviate, or skip any review section (1–4), whatever the plan type (strategy, spec, code, infra). Every section earns its place. "This is a strategy doc, so implementation sections don't apply" is always wrong — implementation detail is exactly where strategy breaks down. If a section truly has zero findings, say "No issues found" and move on, but you must evaluate it.

How to run the sections

The four review sections (plus Section 1.5, Production State Coverage) are independent analytical passes over the agreed plan. By default, work through them sequentially in this context — analyze Section 1, present its diagrams/tables, walk the user through its issues with STOP, one AskUserQuestion per issue, then move to the next. That's the cheap default.

If the user opted into subagents (see Subagents & parallelism above), you may instead pre-compute all four section analyses in parallel — one agent per section — and then walk the user through the findings section by section, in order, exactly the same way. The fan-out only removes the waiting; it never changes the section-by-section UX and never asks the user anything (background agents can't). The test-plan artifact, the coverage-diagram presentation, and every AskUserQuestion stay in this context either way.

When fanning out, dispatch the sections as a Workflow:

export const meta = {
  name: 'plan-review-sections',
  description: 'Parallel analysis of the plan-review sections over an agreed plan',
  phases: [{ title: 'Analyze' }],
}
const SECTION_SCHEMA = {
  type: 'object',
  properties: {
    section: { type: 'string' },
    issues: {
      type: 'array',
      items: {
        type: 'object',
        properties: {
          severity: { type: 'string' },                               // P0 | P1 | P2 | P3
          confidence: { type: 'number' },                             // 1-10
          location: { type: 'string' },                               // file:line when known
          finding: { type: 'string' },
          fix: { type: 'string' },
        },
        required: ['severity', 'confidence', 'finding'],
      },
    },
    diagrams: { type: 'array', items: { type: 'string' } },           // ASCII diagrams as text
    tables: { type: 'array', items: { type: 'string' } },             // coverage maps / registries as text
  },
  required: ['section', 'issues'],
}
const analyses = await parallel(args.sections.map(s => () =>
  agent(
    `You are analyzing ONE section of an engineering-manager plan review. Do NOT ask the user anything — ` +
    `return findings only; the main loop walks through them interactively afterward.\n\n` +
    `PLAN UNDER REVIEW:\n${args.plan}\n\n` +
    `CONTEXT: agreed scope=${args.acceptedScope}; design-doc notes=${args.design}; ` +
    `test framework=${args.testFramework}; system-audit notes=${args.audit}.\n\n` +
    `SECTION TO PRODUCE (${s.key}):\n${s.spec}\n\n` +
    `Every finding MUST carry a confidence score (1-10) and use the format ` +
    `[SEVERITY] (confidence: N/10) file:line — description. Produce every required ASCII ` +
    `diagram and table as text. For the Test section, produce the full ASCII coverage diagram.`,
    { label: `section:${s.key}`, phase: 'Analyze', model: 'opus', schema: SECTION_SCHEMA }
  )
))
return analyses.filter(Boolean)

Pass args: { plan, acceptedScope, design, testFramework, audit, sections: [{ key, spec }, ...] }, where each spec is the corresponding section definition below (verbatim — Section 1, Section 1.5, Section 2, Section 3, Section 4), and plan is the agreed plan plus any accepted scope decisions from Step 0. When the workflow returns, walk the sections in numeric order: present each section's diagrams/tables, then STOP. AskUserQuestion once per issue. Apply the Confidence Calibration display rules (below) here — suppress 3–4 to an appendix, gate 1–2. Write the test-plan artifact here, after presenting Section 3.

If the fan-out fails or times out, fall back to running the sections sequentially in this context.

Section 1: Architecture Review

Evaluate:

• Overall system design and component boundaries.
• The dependency graph and coupling concerns.
• Data-flow patterns and potential bottlenecks.
• Scaling characteristics and single points of failure.
• Security architecture (auth, data access, API boundaries).
• Whether key flows deserve ASCII diagrams.
• For each new codepath or integration point, one realistic production failure scenario and whether the plan accounts for it.
• Distribution architecture: if this introduces a new artifact (binary, package, container), how does it get built, published, and updated? Is CI/CD part of the plan or deferred?

STOP. AskUserQuestion once per issue. One issue per call. Present options, state the recommendation, explain WHY. Do NOT proceed until ALL issues are resolved.

Section 1.5: Production State Coverage

When the plan adds new code paths that read or write production state — durable storage, runtime configuration, container/process topology, or multi-instance class state — answer the four questions below explicitly in the lockdown doc. Skip this section if the plan is purely read-only diagnostics, doc-only, or a refactor with byte-stable output.

The rule exists to catch the recurring "wired in tests, unwired in production" failure: tests pre-populate state that production lacks on first feature use, so the test passes but production breaks. The cited-fact rule catches static-fact errors; this section catches state-coverage gaps.

Q1: Production state inputs

Enumerate every piece of state the new path reads at runtime. Categories: persistent storage (DB, KV, files-as-truth), runtime config (env, config files, flags), container/process topology (which writes persist vs. are ephemeral, which need volume mounts), stateful instance fields (mutable state that outlives a single call).

For each input, state its value on a fresh production system — clean storage, default env, no prior runs. Production is empty until the feature first writes it.

Q2: Empty / missing / stale handling

For each Q1 input, trace what the code does on MISSING / EMPTY (0, null, "") / STALE. Defaults that don't match production truth are the most common bug shape — e.g. default=0 where the value should fall back to env, or default=False where absence means "unconfigured," not "off."

Q3: Multi-instance state audit

For every stateful class the plan touches, grep -rn "ClassName(" <source-dir>. If it's constructed in 2+ places, those are independent instances — updates to one don't propagate. Grep all read sites for every state field the plan modifies. A reader on a different instance than the writer is a stale-instance bug. The lockdown lists every constructor site (file:line) and, per instance, who mutates the state and on what trigger.

Q4: Topology / deployment

For CLIs / one-off invocations / background writers: where does it run (laptop / production host / running container / one-off short-lived container)? Where do writes land (durable volume / ephemeral layer / host FS / network)? Does production mount that path durably? Will the next deploy overwrite operator-side mutations?

Output contract

The lockdown doc must include a Production State Coverage subsection answering Q1–Q4 explicitly, per code path. If any answer is "TBD" or unverified, the verdict drops to NO-GO until it's resolved.

STOP. Don't move to Section 2 until Q1–Q4 are answered for every code path that touches production state.

Confidence Calibration

Every finding MUST carry a confidence score (1–10):

Finding format:

[SEVERITY] (confidence: N/10) file:line — description

Example: [P1] (confidence: 9/10) app/models/user.rb:42 — SQL injection via string interpolation in where clause [P2] (confidence: 5/10) app/controllers/api/v1/users_controller.rb:18 — Possible N+1 query, verify with production logs

Section 2: Code Quality Review

Evaluate:

• Code organization and module structure.
• DRY violations — be aggressive.
• Error-handling patterns and missing edge cases (call these out explicitly).
• Technical-debt hotspots.
• Over-engineered or under-engineered areas.
• Existing ASCII diagrams in touched files — still accurate?

STOP. AskUserQuestion once per issue.

Section 3: Test Review

100% coverage is the goal. Evaluate every codepath in the plan and ensure the plan includes tests for each one. If the plan is missing tests, add them — the plan should be complete enough that implementation ships full test coverage from the start.

Test Framework Detection

Before analyzing coverage, detect the project's test framework:

1. Read CLAUDE.md for a ## Testing section with the test command and framework. If found, treat it as authoritative.
2. If CLAUDE.md has no testing section, auto-detect:

[ -f Gemfile ] && echo "RUNTIME:ruby"
[ -f package.json ] && echo "RUNTIME:node"
[ -f requirements.txt ] || [ -f pyproject.toml ] && echo "RUNTIME:python"
[ -f go.mod ] && echo "RUNTIME:go"
[ -f Cargo.toml ] && echo "RUNTIME:rust"
ls jest.config.* vitest.config.* playwright.config.* cypress.config.* .rspec pytest.ini phpunit.xml 2>/dev/null
ls -d test/ tests/ spec/ __tests__/ cypress/ e2e/ 2>/dev/null

3. If no framework is detected: still produce the coverage diagram, but skip test generation.

Step 1. Trace every codepath in the plan

For each new feature/service/endpoint/component, follow the planned execution — don't just list functions:

1. Read the plan. Understand each component and how it connects to existing code.
2. Trace data flow from each entry point (route handler, exported function, listener, render). For every branch: where input comes from, what transforms it, where it goes, and what can go wrong (null, invalid, network failure, empty).
3. Diagram each changed file in ASCII: every function added/modified, every conditional (if/else, switch, ternary, guard, early return), every error path (try/catch, error boundary, fallback), every call into another function (does it have untested branches?), every edge (null, empty, invalid type).

Every branch needs a test.

Step 2. Map user flows, interactions, and error states

Code coverage alone isn't enough — cover how real users interact:

• User flows: the sequence of actions touching this code. Each step needs a test.
• Interaction edges: double-click resubmit; navigate away mid-op (back button, close tab); stale data (page sat 30 min, session expired); slow connection (API takes 10s); concurrent actions (two tabs, same form).
• Error states the user sees: clear message or silent? Can they recover (retry, go back) or are they stuck? No network? 500 from the API? Invalid server data?
• Empty/zero/boundary: 0 results, 10,000 results, single-char input, max-length input.

A user flow with no test is as much a gap as an untested if/else.

Step 3. Check each branch against existing tests

Go through your diagram branch by branch. For each one, search for a test that exercises it.

Quality scoring rubric:

• ★★★ Tests behavior with edge cases AND error paths
• ★★ Tests correct behavior, happy path only
• ★ Smoke test / existence check (e.g. "it renders," "it doesn't throw")

E2E Test Decision Matrix

RECOMMEND E2E (mark as [→E2E] in the diagram):

• A common user flow spanning 3+ components/services (e.g. signup → verify email → first login)
• An integration point where mocking hides real failures (e.g. API → queue → worker → DB)
• Auth / payment / data-destruction flows — too important to trust to unit tests alone

RECOMMEND EVAL (mark as [→EVAL] in the diagram):

• A critical LLM call that needs a quality eval
• Changes to prompt templates, system instructions, or tool definitions

STICK WITH UNIT TESTS:

• A pure function with clear inputs/outputs
• An internal helper with no side effects
• An edge case of a single function (null input, empty array)
• An obscure/rare flow that isn't customer-facing

REGRESSION RULE (mandatory)

IRON RULE: when the coverage audit finds a REGRESSION — code that previously worked but the diff broke — a regression test is added to the plan as a critical requirement. No AskUserQuestion. No skipping. Regressions are the highest-priority test because they prove something broke.

A regression is when:

• The diff modifies existing behavior (not new code)
• The existing test suite doesn't cover the changed path
• The change introduces a new failure mode for existing callers

When you're unsure whether a change is a regression, err toward writing the test.

Step 4. Output ASCII coverage diagram

Include BOTH code paths and user flows. Mark E2E-worthy and eval-worthy paths:

CODE PATH COVERAGE
===========================
[+] src/services/billing.ts
    │
    ├── processPayment()
    │   ├── [★★★ TESTED] Happy path + card declined + timeout — billing.test.ts:42
    │   ├── [GAP]         Network timeout — NO TEST
    │   └── [GAP]         Invalid currency — NO TEST
    │
    └── refundPayment()
        ├── [★★  TESTED] Full refund — billing.test.ts:89
        └── [★   TESTED] Partial refund (checks non-throw only) — billing.test.ts:101

USER FLOW COVERAGE
===========================
[+] Payment checkout flow
    │
    ├── [★★★ TESTED] Complete purchase — checkout.e2e.ts:15
    ├── [GAP] [→E2E] Double-click submit — needs E2E, not just unit
    ├── [GAP]         Navigate away during payment — unit test sufficient
    └── [★   TESTED]  Form validation errors (checks render only) — checkout.test.ts:40

[+] LLM integration
    │
    └── [GAP] [→EVAL] Prompt template change — needs eval test

─────────────────────────────────
COVERAGE: 5/13 paths tested (38%)
  Code paths: 3/5 (60%)
  User flows: 2/8 (25%)
QUALITY:  ★★★: 2  ★★: 2  ★: 1
GAPS: 8 paths need tests (2 need E2E, 1 needs eval)
─────────────────────────────────

Fast path: all paths covered → "Test review: All new code paths have test coverage ✓" Continue.

Step 5. Add missing tests to the plan

For each GAP, add a test requirement to the plan. Be specific:

• Which test file to create (match existing naming conventions)
• What the test should assert (specific inputs → expected outputs/behavior)
• Unit test, E2E test, or eval (use the decision matrix)
• For regressions: flag as CRITICAL and explain what broke

The plan should be complete enough that implementation ships every test alongside the feature code, not deferred.

Test ambition check

• What's the test that would make you confident shipping at 2am on a Friday?
• What's the test a hostile QA engineer would write to break this?

Test Plan Artifact

After producing the coverage diagram, write a test-plan artifact so QA agents can consume it (inline — workflows can't write files):

mkdir -p ~/.claude/test-plans
REPO_NAME=$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")
BRANCH=$(git branch --show-current 2>/dev/null || echo "unknown")
DATETIME=$(date +%Y%m%d-%H%M%S)

Write to ~/.claude/test-plans/${REPO_NAME}-${BRANCH}-test-plan-${DATETIME}.md:

# Test Plan
Generated by plan-review on {date}
Branch: {branch}
Repo: {owner/repo}

## Affected Pages/Routes
- {URL path} — {what to test and why}

## Key Interactions to Verify
- {interaction description} on {page}

## Edge Cases
- {edge case} on {page}

## Critical Paths
- {end-to-end flow that must work}

Include only what helps a QA tester know what to test and where — not implementation details.

For LLM/prompt changes: check CLAUDE.md for "Prompt/LLM changes" patterns. State which eval suites must run, which cases to add, and which baselines to compare against.

STOP. AskUserQuestion once per issue.

Section 4: Performance Review

Evaluate:

• N+1 queries and database access patterns.
• Loop queries — API/DB calls inside loops that should be batched.
• Memory-usage concerns.
• Caching opportunities.
• Slow or high-complexity code paths.

STOP. AskUserQuestion once per issue.

Outside Voice — Independent Plan Challenge (optional, recommended)

Once all review sections are done, offer an independent second opinion. Several challengers, each with a distinct lens, catch failure modes a single pass misses; convergence across lenses is a stronger signal than one thorough review.

AskUserQuestion: "All review sections complete. Want an outside voice? An independent challenge surfaces logical gaps, feasibility risks, and blind spots. ~2 min. RECOMMENDATION: A. A=9/10, B=7/10." — Options: A) Get the outside voice. B) Skip.

If A, run the three challenger lenses below.

• By default, run them sequentially in this context. For each lens, reset to a brutally-honest framing and analyze the plan fresh against that lens alone — don't repeat the section review, find what it missed.
• If the user opted into subagents (see Subagents & parallelism), you may instead dispatch them as a parallel panel — one agent per lens, each with genuinely fresh context. That buys real independence at the cost of more tokens; only do it after the explicit opt-in.

The lenses:

• logic-gaps — Find logical gaps and unstated assumptions that survived the multi-section review.
• overcomplexity — Is there a fundamentally simpler approach the review was too deep in the weeds to see? Name it.
• feasibility — What feasibility risks did the review take for granted? Missing dependencies or sequencing issues?

When fanning out, dispatch the panel as a Workflow:

export const meta = {
  name: 'plan-review-outside-voice',
  description: 'Independent challenger panel — one lens per agent — over a reviewed plan',
  phases: [{ title: 'Challenge' }],
}
const CHALLENGE_SCHEMA = {
  type: 'object',
  properties: {
    lens: { type: 'string' },
    problems: { type: 'array', items: { type: 'string' } },
  },
  required: ['lens', 'problems'],
}
const LENSES = [
  { key: 'logic-gaps', instruction: 'Find logical gaps and unstated assumptions that survived the multi-section review.' },
  { key: 'overcomplexity', instruction: 'Is there a fundamentally simpler approach the review was too deep in the weeds to see? Name it.' },
  { key: 'feasibility', instruction: 'What feasibility risks did the review take for granted? Missing dependencies or sequencing issues?' },
]
const panel = await parallel(LENSES.map(l => () =>
  agent(
    `You are a brutally honest reviewer examining a plan that already passed multi-section review. ` +
    `Don't repeat that review — find what it missed. Your lens: ${l.instruction}\n` +
    `Direct. Terse. No compliments. Just the problems.\n\n` +
    `THE PLAN:\n${args.plan}`,
    { label: `challenge:${l.key}`, phase: 'Challenge', model: 'opus', schema: CHALLENGE_SCHEMA }
  )
))
return panel.filter(Boolean)

Pass args: { plan: "<plan content>" }. Whichever path you take, present the findings verbatim in a banner block. Then flag cross-opinion tension — points where the outside voice disagrees with earlier findings. Present each tension via AskUserQuestion (one per call): A) Accept the outside voice. B) Keep current. C) Investigate. D) Add to TODOS.md.

Integration rule: outside-voice findings stay INFORMATIONAL until the user approves each one. Never auto-incorporate.

If the fan-out fails or times out, fall back to running the lenses inline, or skip and note: "Outside voice unavailable, proceeding with the single review."

Required Outputs

"NOT in scope" section

List work considered and explicitly deferred, with a one-line rationale each.

"What already exists" section

List existing code/flows that partially solve sub-problems and whether the plan reuses them.

TODOS proposals

Present each potential TODO as its own AskUserQuestion. Never batch.

For each TODO:

• What: one-line description
• Why: the concrete problem it solves or value it unlocks
• Pros: what you gain
• Cons: cost, complexity, or risk
• Context: enough detail for someone picking this up in 3 months
• Depends on / blocked by: prerequisites

Options: A) Add to TODOS.md B) Skip C) Build it now in this PR

Don't just append vague bullet points. A TODO without context is worse than no TODO.

Diagrams (mandatory — produce all that apply)

1. System architecture
2. Data flow (including shadow paths: nil, empty, error)
3. State machine
4. Dependency graph
5. Test coverage map

Identify which files should get inline ASCII diagram comments — especially Models with complex state transitions and Services with multi-step pipelines.

Failure Modes

For each new codepath from the test diagram, list one realistic way it could fail in production and whether:

1. A test covers that failure
2. Error handling exists for it
3. The user would see a clear error or a silent failure

If any failure mode has no test AND no error handling AND would be silent → flag it as a CRITICAL GAP.

Worktree Parallelization Strategy

Analyze the plan's implementation steps for parallel execution opportunities — helps split work across git worktrees.

Skip if: all steps touch the same primary module, or the plan has fewer than 2 independent workstreams. Write: "Sequential implementation, no parallelization opportunity."

Otherwise, produce:

1. Dependency table — for each implementation step/workstream:

Work at the module/directory level. Plans describe intent ("add API endpoints"), not specific files. Module-level is reliable; file-level is guesswork.

2. Parallel lanes:
   • Steps with no shared modules and no dependency → separate lanes (parallel)
   • Steps sharing a module directory → same lane (sequential)
   • Steps depending on other steps → later lanes

Format: Lane A: step1 → step2 (sequential, shared models/) / Lane B: step3 (independent)

3. Execution order — which lanes launch in parallel and which wait. Example: "Launch A + B in parallel worktrees. Merge both. Then C."
4. Conflict flags — if two parallel lanes touch the same module directory, flag it: "Lanes X and Y both touch module/, potential merge conflict."

Completion Summary

  +====================================================================+
  |          ENG MANAGER PLAN REVIEW — COMPLETION SUMMARY               |
  +====================================================================+
  | Step 0 (Scope)       | accepted as-is / scope reduced               |
  | Section 1  (Arch)    | ___ issues found                            |
  | Section 2  (Quality) | ___ issues found                            |
  | Section 3  (Tests)   | diagram produced, ___ gaps identified       |
  | Section 4  (Perf)    | ___ issues found                            |
  +--------------------------------------------------------------------+
  | NOT in scope         | written (___ items)                          |
  | What already exists  | written                                     |
  | TODOS proposed       | ___ items                                   |
  | Failure modes        | ___ total, ___ CRITICAL GAPS                |
  | Outside voice        | ran / skipped                                |
  | Parallelization      | ___ lanes, ___ parallel / ___ sequential     |
  | Test plan artifact   | written / skipped                            |
  | Lake Score           | X/Y recommendations chose complete option   |
  | Diagrams produced    | ___ (list types)                            |
  | Unresolved decisions | ___ (listed below)                          |
  +====================================================================+

Unresolved Decisions

If any AskUserQuestion goes unanswered, list it as "Unresolved decisions that may bite you later." Never silently default.

Output File

By default, write the review to PLAN_REVIEW_<ref>.md in the project root (e.g. PLAN_REVIEW_PR200.md) — unless the user asked (see Gather context first) for a different path/filename or to keep the lockdown inline. (Inline write — workflows can't write files.)

• Do NOT write or append to REVIEWED.md — that file is reserved for code-level reviews only.
• Use the PR number, commit ref, or feature name as <ref> (from $ARGUMENTS when provided).

Retrospective Learning

Check the git log for this branch. If prior commits suggest an earlier review cycle (review-driven refactors, reverted changes), note what changed and whether the current plan touches the same areas. Be MORE aggressive reviewing previously problematic areas.

Next Steps — Review Chaining

After the review, check whether additional reviews would help:

• Suggest /design-review if UI changes exist and no design review has run — detect from the test diagram, the architecture review, or any section that touched frontend components, CSS, views, or user-facing interaction flows.
• Mention /scope-review if this is a significant product change and no scope review exists — a soft suggestion. Only mention it if the plan introduces new user-facing features, shifts product direction, or expands scope substantially.
• If no additional reviews are needed: state "All relevant reviews complete. Ready to implement."

Use AskUserQuestion with only the applicable options:

• A) Run /design-review (only if UI scope detected)
• B) Run /scope-review (only if a significant product change)
• C) Ready to implement

Formatting Rules

• NUMBER issues (1, 2, 3…) and LETTER options (A, B, C…).
• Label with NUMBER + LETTER (e.g. "3A", "3B").
• One sentence max per option. Pickable in under 5 seconds.
• After each section, pause and ask for feedback before moving on.

Important Rules

• Never make code changes or start implementation. Review and review-artifact files only (PLAN_REVIEW_<ref>.md, the test-plan artifact).
• One issue = one AskUserQuestion. Never batch.
• Default is fully inline; fan out only after the user opts in (see Subagents & parallelism). Even then, never put a scope-reduction opt-in, a per-issue STOP, a TODOS proposal, a cross-opinion tension question, or any file write inside a Workflow agent() — background agents can't talk to the user or touch the filesystem.
• Never skip Step 0 or the test diagram.