code-review

name	code-review
description	Use when docs/specs/changes/{name}/tasks.md has every stage marked `- [x]` with evidence, or when user says "review the change", "ship it", "merge the delta", "archive the change", or a prior code-review returned CRITICAL findings that execution has addressed — verifies every ADDED/MODIFIED requirement has code and tests, re-runs every stage's verification command against recorded evidence, then applies the delta wholesale in RENAMED→REMOVED→MODIFIED→ADDED order and archives the change.
metadata	{"tags":["tech:markdown","spec-driven","workflow","review","merge"]}

Code Review

Overview

Two phases:

1. Verify: every ADDED/MODIFIED requirement has code + tests, every stage's verification command re-runs cleanly against recorded evidence.
2. Merge: apply deltas in canonical order (RENAMED → REMOVED → MODIFIED → ADDED), archive the change folder.

Any CRITICAL finding in Phase 1 blocks the entire merge. No partial merges.

Violating the letter of the rules is violating the spirit of the rules.

Running code-review as a subagent

For broad changes — many requirements, multiple domains, or extensive stage verification — the coordinator MAY dispatch Phase 1 (Verify) as a subagent. Phase 1 is the strong candidate: it benefits from context isolation, can itself dispatch reviewer subagents per requirement, and returns inline findings with severity tiers.

Phase 2 (Merge and archive) stays with the coordinator. Delta merges touch stable spec files and archive the change folder; those operations are short, ordered, and stateful, so the isolation benefit is thin and the coordinator should ratify the result. A Phase 1 subagent returns its inline findings; the coordinator then runs Phase 2 (or kicks back to execution on any CRITICAL).

Default to running Phase 1 inline for small tweaks (single-domain, few requirements).

Do NOT apply ANY delta operation to stable specs until: 1. Every ADDED/MODIFIED requirement is verified in code AND tests. 2. Every stage's verification command has been re-run and passed. 3. Phase 1 reports no CRITICAL severity findings.

Any CRITICAL finding blocks the merge entirely. Return to execution with the findings. Do NOT apply a subset of the deltas "to make progress."

The Iron Law

NO DELTA MERGE WITHOUT FULL VERIFICATION FIRST

When to Use

Always:

• tasks.md has all stages marked - [x] with Stage Evidence blocks.
• A previous code-review returned CRITICAL findings and execution has re-run.

Exceptions: none. Deltas merge only through this skill.

AGENTS.md (forwarded, not re-read)

AGENTS.md files were read during plan-review and the notes were captured in tasks.md under ## AGENTS.md Notes. Use those notes to inform CRITICAL vs WARNING severity assessments. Do NOT re-read AGENTS.md from disk.

Process

Phase 1: Verify

1. Enumerate the ADDED and MODIFIED requirements from delta specs.
2. Re-run every stage's verification command, capturing output. Compare to the Stage Evidence recorded in tasks.md. Any divergence is CRITICAL.
3. For each requirement, confirm behavior exists in code AND test coverage exists that exercises the scenarios. Missing implementation or missing tests → CRITICAL.
4. MAY dispatch parallel reviewer subagents for independent requirements to isolate context. Announce: "Batch execute tasks that can be run in parallel sub agents." Use the "Do Not Trust the Report" prompt framing from references/subagent-prompts/spec-reviewer-prompt.md. Aggregate status codes.
5. Quality review: check AGENTS.md conventions (from tasks.md notes), look for obvious issues (unhandled errors, race conditions, dead code).
6. Severity assessment using review-checklist.md (shared with review-changes): CRITICAL blocks; WARNING surfaces; SUGGESTION informational.
7. Report findings inline with severity tiers (CRITICAL / WARNING / SUGGESTION). No file is written; the report lives in the chat response.
8. Decision gate:
   • Any CRITICAL → STOP. Hand the inline findings to execution. Phase 2 does not run.
   • No CRITICAL → proceed to Phase 2.

Phase 2: Merge and archive

9. Apply deltas wholesale against the real stable spec files in canonical order: RENAMED → REMOVED → MODIFIED → ADDED. For MODIFIED, replace the ENTIRE requirement block (heading through all scenarios). See references/delta-merge.md.
   • Do NOT skip any operation.
   • Do NOT reorder.
10. Archive the change folder:

    mv docs/specs/changes/<name> docs/specs/changes/archive/YYYY-MM-DD-<name>
    

    Use today's date (local time) for the prefix.
11. Report the final state: files merged, archive path, final stage/evidence summary.

Checklist

You MUST work through each of these items and complete them in order:

1. AGENTS.md notes forwarded from tasks.md (no ad-hoc re-reads).
2. Every stage's verification command re-run and confirmed passing.
3. Every ADDED/MODIFIED requirement has implementation AND tests verified.
4. Reviewer subagent results aggregated (if used).
5. CRITICAL-free decision made (or returned to execution).
6. Deltas applied in canonical order: RENAMED → REMOVED → MODIFIED → ADDED.
7. Change folder moved to docs/specs/changes/archive/YYYY-MM-DD-<name>/.

Common Rationalizations

Excuse	Reality
"It's mostly done, archive what's ready and follow-up the rest"	Partial merges corrupt the delta's atomicity. All or nothing.
"Apply order doesn't matter here"	RENAMED → REMOVED → MODIFIED → ADDED is canonical for a reason: later ops depend on earlier state. Follow it.
"MODIFIED is just a patch, I can merge line-by-line"	MODIFIED replaces the ENTIRE requirement block. Line-level merges lose scenarios.
"Archive later, I'll forget"	Archive is part of the merge. A non-archived complete change is a bug waiting to happen.
"I wrote it so I know it works"	Re-run verification. Environments drift.
"The stage verification already ran during execution, trust it"	Environments drift. Re-run in Phase 1; compare against recorded evidence.

Red Flags — STOP

• Applying deltas in a different order than RENAMED → REMOVED → MODIFIED → ADDED
• Partial MODIFIED (replacing only part of a requirement block)
• Skipping archive
• Accepting subagent DONE without verifying the claim
• CRITICAL finding but "it's probably fine" reasoning
• Phase 2 starting with any CRITICAL finding

All of these mean: stop, revert if possible, return to the checklist.

Anti-Pattern: "I wrote it so I know it works"

Re-run verification. The machine, environment, dependencies, or upstream data may have shifted. Evidence recorded during execution is what was true then. Phase 1 confirms it's true now. This applies even when you personally executed the stage.

Example

Phase 1 inline report:

# Code Review: add-admin-2fa

## CRITICAL

(none)

## WARNING

- Stage 2 verification output shows 1 flaky test (`TestTOTPValidator_ClockSkew` failed 1/10 runs). Consider loosening tolerance or pinning time source.

## SUGGESTION

- `totp.go` could surface a typed `ErrCodeExpired` instead of generic error.

Merge summary:

## Merge Applied (2026-04-17)

Operations applied in canonical order:
1. RENAMED: (none)
2. REMOVED: (none)
3. MODIFIED: `docs/specs/auth/spec.md` — Session Expiration
4. ADDED:    `docs/specs/auth/spec.md` — Admin Two-Factor Authentication

Archived: `docs/specs/changes/archive/2026-04-17-add-admin-2fa/`

Verification Checklist

• Phase 1 complete with no CRITICAL
• Deltas applied in canonical order
• Archive folder created at docs/specs/changes/archive/YYYY-MM-DD-<name>/
• docs/specs/changes/<name>/ no longer exists

Integration

Terminal states:

• Merged and archived: done.
• CRITICAL findings: invoke execution with the inline findings.

Do NOT invoke any other skill as a "finish" step.

REQUIRED BACKGROUND: See references/delta-merge.md, references/review-checklist.md (shared with review-changes), and plan-review/references/subagent-dispatch.md.