| name | qplan |
| description | Review an implementation plan before any code is written — checks it against the actual codebase for missing steps, wrong assumptions, ignored existing patterns, and unhandled edge cases. Use after drafting a plan/TRD and before implementing; qship runs it as Step 6. |
Plan Review
Review a proposed implementation plan against the codebase and surface gaps, risks, and pattern violations before any code is written. The output is a verdict + a punch list of changes the plan author should make — not a rewrite of the plan.
This is a reviewer, not a drafter. If no plan exists yet, ask the user for one (or for the Jira ticket / spec) before proceeding. Don't invent a plan and then "review" it — that defeats the purpose.
Inputs
Before reviewing, confirm what you're reviewing:
- The plan — a written plan in conversation, a markdown file, a Jira ticket description with steps, a Confluence page, or "the approach we just discussed". If unclear, ask.
- The source of truth for requirements — Jira ticket, spec, bug report, or user goal. Needed for the coverage check below. If absent, note it and skip the AC table.
- Scope hints — repos affected, branches in flight, deadlines. Ask only if non-obvious.
If the plan is too vague to review (e.g. "I'll add an endpoint and a UI"), say so and ask the author to flesh it out before you can usefully review.
Memory
MEMORY.md is already loaded by the auto-memory system — don't re-read it. Just check if any loaded entry is relevant (planning patterns, cross-repo gotchas, analogous-code lessons) and apply it. If a memory entry directly contradicts something in the plan, call it out in the review with a citation.
What a good plan looks like (the rubric)
Apply each of these to the plan. For every miss, add a row to the punch list with the specific change the author should make.
1. Analogous code was found and matched
The strongest signal a plan is grounded is that it points at an existing, similar implementation and proposes to follow it. If the plan doesn't cite an analogue, the author probably hasn't looked.
How to check: Search the codebase yourself for similar features (semantic search via mcp__claude-context-local__search_codebase, then verify with grep if you're skeptical). If you find an analogue the plan ignores, that's a finding — the plan should adopt the existing patterns (filters like is_active, tenant scoping, error handling, return shapes, thresholds) unless it explicitly justifies why it diverges.
Why this matters: The most expensive mistakes come from re-deriving a pattern that already exists, then getting one detail wrong (a missing filter, a different threshold, an inconsistent return type). The existing code is the source of truth.
2. The plan extends, doesn't parallel
If the new functionality already happens somewhere in the codebase — even partially — the plan should extend that path, not introduce a sibling path. Parallel mechanisms drift, duplicate bugs, and create N+1-style surprises.
How to check: For each new "callback" / "service" / "helper" the plan introduces, ask "where does this kind of thing happen today?" If the answer is "in module X already", flag it: the plan should modify X, not add a peer.
3. Acceptance criteria coverage (when applicable)
If there's a Jira ticket or spec with acceptance criteria, build this table and put it in the review:
| AC # | Acceptance Criterion | Plan Step(s) | Test Strategy |
|------|-------------------------------|---------------------|---------------------------|
| AC1 | "User can create X" | Step 3 | Unit + E2E |
| AC2 | "Badge shows count" | Step 5 | Integration + Playwright |
| AC3 | "Cancel reverts state" | MISSING | — |
Every AC must map to at least one step. Every step should serve at least one AC (or have a stated reason — e.g. "preparatory refactor"). If there's no spec, skip the table and note that there's no external requirement to verify against.
4. Test strategy per step
Every step that adds or changes code names what will be tested and how. "Add tests" is not a strategy.
| Code change | Expected test strategy in the plan |
|---|
| New API endpoint | Unit test on handler + integration test on full request cycle |
| New DB model / migration | Migration up/down + model constraint + relationship tests |
| New UI component / callback | Playwright E2E covering render + the interaction |
| Bug fix | Regression test that fails on main and passes after the fix |
| Pure refactor (no behavior δ) | Existing tests must pass; no new tests needed |
If a step has no test strategy and isn't a pure refactor, that's a finding.
5. Risk assessment
Walk this checklist against the plan. Anything YES should already be addressed in the plan; if it's not, add a finding.
| Risk | Trigger | What the plan should include |
|---|
| Cross-repo impact | Touches an interface used by your repos / {{PRIMARY_REPO_NAME}} | Companion update steps in each affected repo, or explicit "no caller change needed" rationale |
| Migration risk | Adds / alters DB tables or columns | Up + down migration, data backfill plan if needed, deploy ordering |
| API contract change | Modifies request or response shape | Enumeration of every consumer (UI, gateway sync, external) and how each is updated |
| Shared function change | Edits a function with multiple callers | List of callers + how each remains correct |
| Hot-path performance | Adds queries to badge callbacks, list endpoints, login flow | Index plan or measured rationale that the cost is acceptable |
| Cache invalidation | Adds a cache layer or modifies cached data | Every mutation path that must invalidate, listed explicitly |
6. Sequencing and reversibility
A good plan orders steps so each one leaves the system in a working state, and risky/irreversible work happens last (or behind a flag). Flag plans that:
- Ship a migration before the code that uses it (or vice versa) without a deploy ordering note.
- Delete or rename a public symbol without a deprecation step.
- Bundle a refactor with a behavior change in the same step (makes review and revert harder).
7. Stated assumptions
The plan should explicitly list what it's assuming about the system (e.g. "assume is_active is the only soft-delete flag", "assume there's exactly one matcher per entity"). Unstated assumptions are the #1 source of plan failure. If a plan reads as if everything is known, push back: ask the author to articulate what they're guessing.
Deep dive (use sparingly)
For non-trivial plans — multi-file, architectural, or fixes where the root cause is uncertain — go further:
- Spawn Explore subagents for specific questions: "Find every caller of
X across all three repos", "Map the data flow from record ingest to reference match", "Confirm whether Y is reached when the user does Z". Don't run the whole review through subagents; use them for the questions you can't answer cheaply.
- Verify assumptions, don't accept them. If the plan says "this only runs once per request", check it. If it says "no other code touches this table", grep for the table name.
- Synthesize before finalizing. Once subagent findings are back, decide which ones change the verdict and which are noise. Don't dump raw findings into the review.
Output format
Produce the review as the last message — concise, scannable, and actionable.
## Plan Review: <one-line subject>
**Verdict:** Approve | Approve with revisions | Needs significant revisions | Cannot review (missing info)
**Summary** (2–3 sentences on the overall shape — what's strong, what's the biggest gap)
### Findings
1. **[Severity]** Short title — what's wrong, where in the plan, what to change. Cite file paths or AC numbers where relevant.
2. ...
### AC coverage
<the table from rubric §3, or "N/A — no external spec">
### Open questions for the author
- ...
Severity scale: Blocker (plan will fail or ship broken if shipped as-is), Major (significant rework needed), Minor (worth fixing but not load-bearing), Nit (preference / polish).
Order findings by severity. Don't pad — if there are no Major issues, say so.
When to push back instead of reviewing
- Plan is too vague to assess → ask for specifics on the steps that matter most.
- Plan is for a problem that hasn't been root-caused (e.g. fix proposed before the bug is understood) → say so; recommend
/qbug or systematic-debugging first.
- Plan contradicts a memory entry the user has already endorsed → cite the memory, don't re-litigate.
- You don't have access to the codebase or the requirements → say what you'd need to do a real review.
A "looks fine 👍" review is almost always wrong. If you can't find anything, you probably haven't looked at the analogue code yet.
