plan-review-implementation

Review ALL implementation iterations that need review using an agent team with domain-specialized reviewers. You are the team lead/coordinator. You orchestrate 3 reviewer teammates who review changed files in parallel through different lenses, then you merge findings, document them, and handle Linear/git.

If agent teams are unavailable (TeamCreate fails), fall back to single-agent mode — see "Fallback: Single-Agent Mode" section.

Reference: See [references/code-review-checklist.md](references/code-review-checklist.md) for comprehensive checklist.

Pre-flight Check

1. Read PLANS.md — Understand the full plan and iteration history
2. Read CLAUDE.md — Understand project standards and conventions
3. Verify Linear MCP — Call mcplinearlist_issues with team: "ADVA Administracion" and state: "Review". If unavailable, STOP and tell the user: "Linear MCP is not connected. Run /mcp to reconnect, then re-run this skill."
4. Assess AI-generated code risk — If implementation is large or shows AI patterns, apply extra scrutiny

Linear State Management

This skill moves issues from Review → Merge (preparing for PR).

Same-type transition gotcha: Review and Merge are both type: started. If a name-based transition does not take effect, retry with the state UUID from mcplinearlistissuestatuses (see CLAUDE.md LINEAR INTEGRATION).

If task passes review (no issues):

• Move issue from "Review" to "Merge" using mcplinearsave_issue (pass id and state)

If task needs fixes (Fix Plan path):

• Move original issue from "Review" to "Merge" (the original task was completed)
• Create NEW Linear issue(s) in "Todo" for each bug/fix using mcplinearsave_issue (no id):

- team: "ADVA Administracion" - state: "Todo" (will enter PLANS.md via Fix Plan) - labels: Bug

• Add new issue links to the Fix Plan section in PLANS.md
• These new issues will go through the full cycle when plan-implement runs the Fix Plan

If task needs fixes (inline fix path — ≤3 S-size bugs):

• Move original issue from "Review" to "Merge" (the original task was completed)
• Fix bugs inline with TDD (see Inline Fix Assessment)
• Create NEW Linear issue(s) in "Merge" state for traceability using mcplinearsave_issue (no id):

- team: "ADVA Administracion" - state: "Merge" (already fixed) - labels: Bug

• No Fix Plan needed — bugs resolved in the same session

Identify What to Review

Detection logic:

1. Search PLANS.md for ## Iteration N sections
2. If iterations exist: Build list of iterations needing review:

- Has "Tasks Completed This Iteration" or "### Completed" subsection - Does NOT contain <!-- REVIEW COMPLETE --> marker - Process in order (Iteration 1 first, then 2, etc.)

1. If NO iterations exist: Treat entire plan as single iteration:

- Look for "Completed" or "### Completed" section at plan level - Check if plan already has <!-- REVIEW COMPLETE --> marker - If completed but not reviewed → review as "Iteration 1"

Iteration detection: A plan has iterations if it contains ## Iteration (with or without number).

An iteration is READY FOR REVIEW when it has "Tasks Completed This Iteration" (or legacy "### Completed") section and does NOT have <!-- REVIEW COMPLETE --> marker yet. The presence of ### Tasks Remaining does NOT affect review readiness — review covers only the completed tasks.

If no iteration/plan needs review → Inform user and stop.

Important: Review ALL pending iterations in a single session, not just one.

Collect Changed Files

From each iteration's "Tasks Completed This Iteration" (or legacy "Completed") section, list all files that were created, modified, or had tests added. This is the review scope — reviewers examine ONLY these files.

Review Scope Assessment

Before spawning a review team, assess whether the overhead is justified. Three Sonnet reviewers each load full project context before reviewing — for small iterations, a single-agent sequential review is faster and cheaper.

Count Changed Files

Count the unique files from "Collect Changed Files" above.

Decision

Announce: "N changed files — [single-agent/team] review mode."

Override: If the iteration includes security-sensitive changes (auth, session, tokens, API keys, crypto), always use the team regardless of file count — security review benefits from a dedicated specialist lens.

Team Setup

Skip this section if scope assessment chose single-agent mode — jump to Fallback: Single-Agent Mode.

Create the team

Use TeamCreate:

• team_name: "plan-review"
• description: "Parallel implementation review with domain-specialized reviewers"

If TeamCreate fails, switch to Fallback: Single-Agent Mode (see below).

Create tasks

Use TaskCreate to create 3 review tasks:

1. "Security review" — Security & auth review of changed files
2. "Reliability review" — Bugs, async, resources, edge cases in changed files
3. "Quality review" — Type safety, conventions, test quality in changed files

Spawn 3 reviewer teammates

Use the Agent tool with teamname: "plan-review", subagenttype: "general-purpose", and model: "sonnet" to spawn each reviewer. Spawn all 3 in parallel (3 concurrent Agent calls in one message).

Each reviewer prompt MUST include:

• The common preamble and their domain checklist from [references/reviewer-prompts.md](references/reviewer-prompts.md)
• The exact list of changed files to review (from Collect Changed Files step)
• Instructions to report findings as a structured message to the lead

Assign tasks

After spawning, use TaskUpdate to assign each task to its reviewer by name.

Coordination

While waiting for reviewer messages:

1. Reviewer messages are automatically delivered — do NOT poll or manually check inbox
2. Teammates go idle after each turn — this is normal, not an error. They're done when they send their findings message.
3. Track progress via TaskList
4. When a reviewer sends their findings:

a. Acknowledge receipt and record findings b. Mark their TaskList task as completed via TaskUpdate c. Immediately send shutdown request via SendMessage with type: "shutdown_request" — do not wait for other reviewers to finish

1. When the last reviewer confirms shutdown, call TeamDelete immediately — the team is no longer needed. Deleting it now prevents bug-hunter/verifier/pr-creator subagents from accidentally joining as team members.
2. Wait until ALL 3 reviewers have reported before proceeding to merge findings

If a reviewer gets stuck or stops without reporting: Send them a message asking for their findings. If they don't respond, shut them down and note that domain as "incomplete".

Merge & Evaluate Findings

Once all reviewer findings are collected:

Deduplicate

• Same code location reported by multiple reviewers → merge into the one with higher priority
• Same root cause in multiple locations → combine into one finding

Classify Each Finding

Every finding MUST be classified as either FIX or DISCARD. There is no "document only" category — PLANS.md is overwritten by the next plan, so anything left there is lost.

CRITICAL RULE: Pre-existing bugs are still bugs. If a reviewer finds a real bug in a reviewed file, it MUST be classified as FIX — even if the bug existed before this iteration, even if it was introduced by a different PR, even if it's in code the current iteration didn't modify but is visible in a changed file. The review's job is to catch bugs, not to assign blame for when they were introduced. The only question is: "Is this a real bug?" If yes → FIX.

FIX — real bugs at any severity:

• Would cause runtime errors or crashes (any severity)
• Could corrupt or lose data
• Security vulnerability (OWASP categories)
• Resource leak affecting production
• Test doesn't actually test the behavior
• Violates CLAUDE.md critical rules
• Edge cases that could realistically occur
• Test flakiness from shared mutable state
• Convention violations that affect correctness
• Pre-existing bugs found in reviewed files (still real bugs)

DISCARD — genuinely not bugs (the ONLY valid reasons):

• False positive: reviewer misread the code or missed context — the code is actually correct
• Misdiagnosed: the flagged pattern is actually correct/intentional design
• Impossible in context: the scenario literally cannot occur given the app's constraints (e.g., "SQL injection" in a query that only accepts validated enum values)
• Style-only: cosmetic preference not enforced by CLAUDE.md, with zero correctness impact

NOT valid reasons to DISCARD:

• "Pre-existing" — a bug doesn't stop being a bug because it was there before
• "Already handled elsewhere" — if it's handled, prove it by citing the exact code; if you can't, it's a FIX
• "Low severity" — low severity bugs are still FIX, just with [low] tag
• "Out of scope for this iteration" — the review scope is the changed files, and any bug found in them is in scope

Inline Fix Assessment

After evaluating findings, assess whether small bugs can be fixed inline instead of creating a full Fix Plan + another implementation cycle. This applies in both team and single-agent review modes.

Step 1: Size each FIX finding

Step 2: Decision

Announce: "N fix(es), all S-size — fixing inline." or "N fix(es) including M/L — creating Fix Plan."

Step 3: Fix Inline (if eligible)

1. For each S-size fix, apply TDD:

- Write failing test → run (npx vitest run "pattern") → implement fix → run → verify pass

1. Run full test suite: npm test
2. Run bug-hunter (as a standalone subagent, NOT a teammate -- do NOT pass team_name):

`` Agent tool with subagent_type "bug-hunter" ``

1. If clean: Proceed to Document Findings with "fixed inline" format
2. If bug-hunter finds new issues in the fixes: Abandon inline approach — create Fix Plan as normal. Document the original findings plus new ones from bug-hunter.

Linear Handling for Inline Fixes

Create Linear issues for traceability, but in "Merge" state (already fixed):

• mcplinearsave_issue with team: "ADVA Administracion", state: "Merge", labels: ["Bug"] (no id)

This preserves the audit trail without triggering the Todo → In Progress → Review workflow.

Document Findings

Three branches, each with an exact markdown template. See [references/findings-templates.md](references/findings-templates.md) for full templates:

• Issues found and fixed inline — append "Review Findings" with "fixed inline" summary, Linear Updates (Review→Merge + new issues in Merge), Inline Fix Verification, <!-- REVIEW COMPLETE -->. No Fix Plan.
• Issues found (creating Fix Plan) — append "Review Findings" with FIX/DISCARDED summary, Linear Updates (Review→Merge + new issues in Todo), <!-- REVIEW COMPLETE -->, then ## Fix Plan at h2 with one ### Fix N: section per bug (each with Linear Issue link + TDD steps).
• No issues found (or all discarded) — append "Review Findings" stating no bugs, Linear Updates (all Review→Merge), <!-- REVIEW COMPLETE -->.

Then continue to the next iteration needing review.

Shutdown Team

Reviewers should already be shut down individually during the Coordination phase (each shut down as they reported findings), and TeamDelete should already be called.

If any reviewer was NOT shut down during coordination (e.g., went idle without reporting), send shutdown request now. Call TeamDelete after the last confirmation if not already called.

After ALL Iterations Reviewed

Report Findings to User

MANDATORY: After all iterations are reviewed, present a direct summary to the user. PLANS.md is overwritten by the next plan, so the user must see all findings NOW.

Report format (output directly to user as text):

Review complete for [N] iteration(s). [M] files reviewed.

BUGS FIXED (Linear issues created):
- [CRITICAL] SECURITY: Missing input validation in CUIT parser (ADV-125)
- [HIGH] BUG: Race condition in spreadsheet processing (ADV-126)

DISCARDED FINDINGS (not bugs):
- EDGE CASE: Unicode in CUIT field — CUIT fields only contain digits and hyphens by AFIP specification
- TYPE: Potential null in response — Actually guarded by early return on line 42

[or: No issues found — clean review.]

If there were no findings at all (clean review), a brief "No issues found" summary is sufficient.

Determine Completion

• If Fix Plan exists OR tasks remain unfinished (and no inline fixes resolved all issues) → Do NOT mark complete. More implementation needed.

1. Commit and push (see Termination section) 2. Inform user: "Review complete. Changes committed and pushed. Run /plan-implement to continue implementation."

• If all tasks complete and no fix plans needed (includes iterations where all bugs were fixed inline) → update header status, append final status, then create PR:

1. Update the header on line 3: change Status: IN_PROGRESS to Status: COMPLETE 2. Append the final status section at the bottom of the file:

---

## Status: COMPLETE

All tasks implemented and reviewed successfully. All Linear issues moved to Merge.

Then create the PR:

1. Commit any uncommitted changes
2. Push to remote
3. Create PR using the pr-creator subagent
4. Inform user with PR URL

Fallback: Single-Agent Mode

If the scope assessment chose single-agent mode (≤4 changed files) OR TeamCreate fails, perform the review as a single agent:

1. Inform user: "N changed files — single-agent review mode." (or "Agent teams unavailable. Running review in single-agent mode." if TeamCreate failed)
2. For each iteration needing review:

a. Identify changed files from "Tasks Completed This Iteration" b. Read each file and apply checks from [references/code-review-checklist.md](references/code-review-checklist.md) c. Apply all domain checks (security, reliability, quality) sequentially d. Classify findings (same Merge & Evaluate Findings rules) e. Run Inline Fix Assessment — same threshold applies (≤3 S-size fixes → fix inline) f. Document findings (same format as team mode, including "fixed inline" format if applicable) g. Handle Linear updates (same as team mode)

1. Continue with "After ALL Iterations Reviewed" section

Context Management & Continuation

Context is checked ONLY at iteration boundaries. Never stop mid-iteration review.

After completing each iteration review, estimate remaining context:

• If > 60% → Continue to next pending iteration
• If <= 60% → Stop, commit/push, inform user to re-run

Never stop mid-iteration — once started, complete the full review for that iteration.

Issue Categories Reference

Error Handling

Termination: Commit, Push, and PR

MANDATORY: Before ending, commit all local changes and push to remote.

For Incomplete Plans

1. Stage modified files: git status --porcelain=v1, then git add <file> ... — skip .env, .key, .pem, credentials, secrets*
2. Commit with simple -m flag (no heredoc, no $(), no Co-Authored-By tags): git commit -m "plan: review iteration N - [issues found | no issues]"
3. git push
4. Inform user to run /plan-implement

For Complete Plans

1. Stage modified files: git status --porcelain=v1, then git add <file> ... — skip .env, .key, .pem, credentials, secrets*
2. Commit with simple -m flag (no heredoc, no $(), no Co-Authored-By tags): git commit -m "plan: mark [plan-name] complete"
3. git push
4. Collect ALL Linear issue identifiers managed during this session:

- Original plan issues (from PLANS.md header) - Issues moved Review → Merge during review - Inline-fix issues created in Merge state - Fix-plan issues that completed and were moved to Merge Deduplicate and sort numerically. Format as: ADV-123, ADV-124, ...

1. Create PR using the pr-creator subagent. Include in the prompt: Linear issues to close: ADV-123, ADV-124, ... with the full list from step 4. This overrides PLANS.md scanning and ensures no inline-fix issues are missed.
2. Inform user with PR URL.
3. Launch the post-PR Codex monitor cron (only after pr-creator reports success):

- Capture the PR number from the pr-creator output (e.g., 112). - Capture the current branch name (e.g., from git rev-parse --abbrev-ref HEAD). - Choose a unique MONITORTAG like Codex monitor for PR <N> so the loop can locate its own cron. - Call CronCreate with: - cron: /3 * (every 3 minutes) - recurring: true - prompt (literal — fill in the values): `` <MONITORTAG> — tick. PRNUMBER=<N>. BRANCH=<branch>. MONITORTAG="<MONITORTAG>". Read .claude/skills/plan-review-implementation/references/codex-loop.md and execute exactly one iteration of the per-tick logic. The cron stops itself (CronDelete by MONITORTAG) when the merge phase succeeds. ` - Tell the user: Codex monitor active (every 3 min). It will assess each Codex finding for validity, fix CI failures, and squash-merge + clean up to main once CI is green and Codex is clean. The monitor self-terminates on completion.` - Do NOT block waiting for the cron to finish — the skill exits after launching it. The cron continues across user turns.

Branch handling: Assumes plan-implement already created a feature branch. If on main, create branch first.

Reference: See [references/codex-loop.md](references/codex-loop.md) for the full per-tick logic the cron prompt invokes.

Rules

• Review ALL pending iterations — Don't stop after one
• Do not modify source code during review — Review only, document findings. Exception: when inline-fixing ≤3 S-size bugs (see Inline Fix Assessment), the lead applies TDD fixes directly after the review phase completes.
• Be specific — Include file paths and line numbers for every issue
• One fix per issue — Each finding must have a matching Fix task with Linear issue
• Fix Plan follows TDD — Test first for each fix
• Never modify previous sections — Only add to current iteration or append status
• Mark COMPLETE only when ALL iterations pass — No fix plans pending, all reviewed
• Move issues to Merge — All reviewed issues that pass go Review→Merge
• Create bug issues — Fix Plan bugs → new issues in Todo state. Inline-fixed bugs → new issues in Merge state.
• Always commit and push at termination — Never end without committing progress
• Create PR when plan is complete — Use pr-creator subagent for final PR
• Lead handles all Linear/git writes — Reviewers NEVER create issues or modify PLANS.md
• No co-author attribution — Commit messages must NOT include Co-Authored-By tags
• Never stage sensitive files — Skip .env, .key, .pem, credentials, secrets*
• Check MIGRATIONS.md — If implementation changed spreadsheet schema, column names, folder structure, or env vars, verify that MIGRATIONS.md has a corresponding note AND that the code includes migration logic (startup detection of old format + automatic migration). If migration logic is missing, add it as a FIX finding — breaking changes to persistent data without migration paths are bugs in production.
• Every finding is FIX or DISCARD — No "document only" category. Real bugs at any severity get Linear issues + Fix Plan. Non-bugs get discarded with reasoning.
• Never discard real bugs — Pre-existing, low-severity, or "already handled" are NOT valid reasons to discard. Only discard findings that are genuinely not bugs (false positive, impossible, style-only). When in doubt, FIX.
• Always report findings to user — PLANS.md is overwritten by the next plan. The user must see all findings (fixed and discarded) directly in the conversation output before the skill terminates.
• Inline fix threshold: ≤3 S-size fixes — When all FIX findings are S-size (surgical, single-line or few-line) and count ≤3, fix them inline with TDD instead of creating a Fix Plan. Abandon inline approach if tests fail or bug-hunter finds new issues.
• Inline fixes still get Linear issues — Create issues in "Merge" state for traceability. Never skip the audit trail.
• Review scope assessment — ≤4 changed files → single-agent review. 5+ files → 3 reviewers. Always use team for security-sensitive changes regardless of file count.
• Only reviewers are teammates — Bug-hunter, verifier, and pr-creator are standalone subagents spawned via Agent tool WITHOUT team_name. Only the 3 domain reviewers are team members.
• Shut down reviewers immediately — Send shutdown request to each reviewer as soon as they report findings. Call TeamDelete as soon as the last reviewer is shut down. Do NOT keep teammates alive during merge/evaluate/document phases.
• Launch Codex monitor only after a successful PR — When pr-creator reports success in the For-Complete-Plans flow, create the 3-min cron with the prompt template above. The cron self-terminates when its merge phase completes (CI green, all threads resolved, squash-merge succeeded, on main). See [references/codex-loop.md](references/codex-loop.md) for the per-tick logic.
• Codex findings must be assessed for validity — The monitor never accepts a Codex finding on face value. Each finding is verified against the actual code, accepted patterns, and CLAUDE.md before fixing. Invalid findings are resolved with reasoning, not silently dismissed.