review-changes

name	review-changes
version	2.6.0
description	[Code Quality] Use when reviewing current changes, staged or unstaged diffs, or branch-to-branch diffs.

[BLOCKING] Execute skill steps in declared order. NEVER skip, reorder, or merge steps without explicit user approval. [BLOCKING] Before each step or sub-skill call, update task tracking: set in_progress when step starts, set completed when step ends. [BLOCKING] Every completed/skipped step MUST include brief evidence or explicit skip reason. [BLOCKING] If Task tools are unavailable, create and maintain an equivalent step-by-step plan tracker with the same status transitions.

[TOP REMINDER — WHY-REVIEW FINDINGS-VALIDATION GATE IS NON-NEGOTIABLE]

If this review produces ANY finding (Critical / High / Medium / Low) in standalone mode, you MUST invoke the /why-review skill through the Skill tool with --validate-findings <report-path> before any fix, docs-update, commit, or handoff. An actual skill call is the ONLY way to pass this gate — re-reading the cited file:lines yourself, "self-validating," or any inline/manual substitute does NOT count.

Create the todo the moment the first finding is recorded — never rely on memory: call TaskCreate → [Review Phase 6] Why-review findings validation gate — invoke /why-review so the gate is tracked and cannot be skipped. Inside $workflow-review-changes, stop after the report; parent step 2 owns validation. Full protocol in Phase 6; mirrored in the Closing Reminders at the bottom.

Quick Summary

Goal: Review current working-tree, staged, branch, or commit diffs across code, docs, config, infra, and non-code artifacts — finding correctness bugs, flaws, missing updates, stale docs, and convention drift with evidence — so every reviewed change is defect-free, evidence-backed, convention-aligned, and synchronized with required tests/docs before handoff; when code files changed, also prove the code stays easy to change.

Summary: read-this-if-nothing-else digest —

• Report-driven and evidence-gated. Every finding is written to plans/reports/code-review-{date}-{slug}.md with file:line proof; speculation is forbidden, "looks fine" is not a verdict, codebase convention (grep 3+ examples) wins over textbook rules.
• Findings are never auto-fixed on sight. Standalone mode runs validate (Phase 6 /why-review --validate-findings, an actual skill call) → fix (Phase 7) → restart /review-changes from Phase 0 over the full diff, looping until one whole pass has zero findings. Inside $workflow-review-changes you stop after the report and hand findings to the parent.
• When code changed, three delegated gates are MANDATORY: Phase 3.5 /code-simplifier (clarity/maintainability), Phase 3.7 /integration-test-review Gate-7 coverage (every behavior change → covering test + spec TC), and — for every behavior change — Spec Drift Adjudication + the Dual-Feedback Ledger (the gap feeds BOTH spec AND tests).
• Docs-update is the unconditional terminal step. Once the loop converges clean, Phase 8 /docs-update ALWAYS runs over the full changeset (deferred only to the parent inside the workflow).

Routing boundary: This skill reviews a git diff — working-tree (default), staged, branch, or commit. For an explicit file-set or SHA-range review, processing received review feedback, or a pre-completion verification gate over already-known scope, use code-review instead.

Shared engine (keep in sync): review-changes and code-review share the same review-protocol SYNC: blocks. Canonical source: .claude/skills/shared/sync-inline-versions.md; policy: SYNC:shared-protocol-duplication-policy. When you change a shared block in one skill, update the canonical file AND the sibling skill so the two never drift. The skills differ only in entry intent (diff vs explicit scope) and diff-specific gates (integration-test-sync, translation-sync, the Phase 3.7 integration-test-review coverage gate) — not in review quality.

Workflow:

1. Phase 0: Blast Radius — Call /graph-blast-radius skill FIRST (if .code-graph/graph.db exists)
2. Phase 0.3: Change Types — Detect high-risk change types; create risk tasks
3. Phase 0.5: Plan Compliance — Verify against active plan (conditional)
4. Phase 0.7: Surface Detection — AI categorizes changed files; creates dimension tasks
5. Phase 1: Collect — Run git status/diff, create report file
6. Phase 2: File Review — Review each changed file, update report incrementally
7. Phase 3: Fresh-Context Gate — Skip when findings already exist; run a second-round sub-agent only for an explicit user/workflow/high-risk synthesis trigger
8. Phase 3.5: Code-Simplifier Optimization (MANDATORY when code files changed) — Invoke /code-simplifier scoped to the changed code files to surface clarity/consistency/maintainability simplifications; record them as findings that flow into the same validation/fix loop (skip docs-only diffs)
9. Phase 3.7: Integration-Test-Review Coverage Gate (MANDATORY when behavior-bearing code changed) — Invoke /integration-test-review over the full diff; its 7 quality gates audit changed tests AND its Gate 7 (Change Coverage) maps every behavior-changing production file to a covering test (integration-first; unit fallback needs justification) and a spec TC. GAP/SPEC-GAP results become findings for the same validation/fix loop (skip docs-only diffs; deferred to the parent's dedicated step inside $workflow-review-changes)
10. Phase 4: Finalize — Generate critical issues, recommendations, suggested commit message
11. Phase 5: Docs Triage — Record stale-doc findings for validation/fix loop
12. Phase 6: Why-Review Findings Validation (standalone-only; REQUIRED before any standalone fix) — Whenever the report contains one or more findings, you MUST invoke the /why-review skill (an actual Skill-tool call) with --validate-findings to verify every finding is correct, proof-backed, reasonable, and best-practice before fixing. This is a genuine skill invocation — re-reading the cited lines yourself, "self-validating," or any inline/manual substitute does NOT satisfy this gate. When this skill is step 1 inside $workflow-review-changes, stop after the report; parent step 2 owns findings validation.
13. Phase 7: Recursive Fix + Full Re-Review Loop (standalone-only) — If validated findings remain in standalone mode, auto-fix them, then re-invoke from Phase 0 with a fresh task breakdown over the full current diff; repeat until an entire review pass has zero findings. When inside , parent steps 10-15 own plan/feature-implement/restart.

Key Rules:

• Report-driven: ALWAYS write findings to plans/reports/code-review-{date}-{slug}.md
• MUST ATTENTION create todo tasks for ALL phases before starting
• Skeptical: every claim needs file:line proof
• Verify convention by grepping 3+ existing examples before flagging violations
• Actively check DRY violations, YAGNI/KISS over-engineering, correctness bugs
• When changed files include source code, run the Easy-to-Change gate: estimate future edit sites, coupling, hidden state, duplicated knowledge, unclear intent, and abstraction boundary health
• When changed files include source code, run the Phase 3.5 /code-simplifier optimization gate over the changed code files — its simplification opportunities are findings that flow through the same Phase 6 validation → Phase 7 fix loop (never auto-applied unvalidated)
• When changed files include behavior-bearing code, run the Phase 3.7 /integration-test-review coverage gate over the full diff — every behavior change must map to a covering test (integration-first) and a spec TC; GAP/SPEC-GAP verdicts are findings for the same Phase 6 → Phase 7 loop, never silently logged
• Cross-reference changed files against related docs — flag stale docs, test specs, READMEs
• MANDATORY FINAL step: once the review/fix loop converges to zero findings, ALWAYS run the Phase 8 /docs-update sweep over the full changeset — unconditional, never skipped on a clean verdict — why: a clean code review still leaves docs stale unless docs-update reconciles them against the actual changes
• Findings are not eligible for auto-fix until Phase 6 why-review validation returns CLEAN for the current finding set
• Every fix cycle invalidates the prior review result; restart /review-changes from Phase 0 and review the full updated diff, including the fixes
• Continue review → validate findings → fix → full re-review until a complete review pass returns zero findings; do not add a fresh-context pass just because findings exist or a fix cycle restarted the review

MANDATORY Plan ToDo Task to discover and READ project-specific reference docs:

1. Search for code standards docs: *code-review*, *patterns*, *conventions*, *style-guide* — read any found
2. Search for architecture docs: *architecture*, *adr-*, README.md at service/module roots
3. Look for docs referencing changed technology areas (backend, frontend, infra, etc.)
4. Read docs most relevant to the categories of files changed

Prerequisites: MUST ATTENTION READ before executing:

Critical Purpose: Ensure quality — no flaws, no bugs, no missing updates, no stale content. Verify both artifacts AND documentation.

External Memory: For complex or lengthy work (research, analysis, scan, review), write intermediate findings and final results to a report file in plans/reports/ — prevents context loss and serves as deliverable.

Evidence Gate: MANDATORY — every claim, finding, and recommendation requires file:line proof or traced evidence with confidence percentage (>80% to act, <80% must verify first).

OOP & DRY Enforcement: MANDATORY — flag duplicated patterns that should be extracted to a base class, generic, or helper. Classes in the same group or suffix MUST ATTENTION inherit a common base (even if empty now — enables future shared logic and child overrides). Verify project has code linting/analyzer configured for the stack.

Code Review: Current Or Branch Diff

Review current changes or explicit branch/commit diffs against project standards.

Review Scope

Target: current working-tree changes by default; explicit branch/tag/commit diff when user asks branch comparison.

Use these sources:

• Current changes: git status, git diff, and git diff --cached
• Branch diff: git diff <base>...<head> plus git diff --name-only <base>...<head>
• Commit range: git diff <base>..<head> plus git diff --name-only <base>..<head>

Review Mindset (NON-NEGOTIABLE)

Be skeptical. Apply critical thinking, sequential thinking. Every claim needs traced proof, confidence >80%.

• Verify correctness by reading actual implementations, never accept it at face value
• Every finding MUST include file:line evidence (grep results, read confirmations)
• Include a claim only when a trace proves it; otherwise leave it out of the report
• Question assumptions: "Does this actually work?" → trace call path to confirm
• Challenge completeness: "Is this all?" → grep related usages
• Verify side effects: "What else does this change break?" → check consumers and dependents
• No "looks fine" without proof — state what was verified and how

First Principle — Easy to Change

Apply this gate when diff includes source-code or code-adjacent files (.cs, .ts, .html, .scss, .css, tests, scripts, build/config-as-code). Pure docs-only changes skip this gate except for executable examples or code snippets.

Success metric: future change cost. DRY, SRP, abstraction, design patterns, naming, layering, tests — all serve one goal: make next change cheaper.

When evaluating code, refactor, test, or abstraction, ask: does this make next change cheaper or more expensive?

• Reject "best practices" raising change cost: premature abstraction, speculative generality, leaky indirection, ceremony without payoff.
• Name real enemies in findings: coupling, hidden state, duplicated knowledge, unclear intent, irreversible decisions exposed too early.
• Favor project-owned boundaries around external libraries, e.g. component/service input-output contracts, when they localize future library changes; reject pass-through wrappers adding ceremony without lowering change cost.
• Simpler design easy to change beats sophisticated design that isn't.

Apply this lens before specific rules, patterns, or checklists below. If downstream rule raises change cost, this principle wins.

---

Core Principles (ENFORCE ALL)

YAGNI — Flag code solving hypothetical future problems (unused parameters, speculative interfaces, premature abstractions) KISS — Flag unnecessarily complex solutions. "Is there a simpler way meeting same requirement?" DRY — Actively grep for similar/duplicate code before accepting new code. 3+ similar patterns → flag for extraction. Clean Code — Readable > clever. Names reveal intent. Functions do one thing. No deep nesting. Follow Convention — Before flagging ANY pattern violation, grep for 3+ existing examples. Codebase convention wins over textbook rules. No Flaws/No Bugs — Trace logic paths. Verify edge cases (null, empty, boundary values). Check error handling covers failure modes. Proof Required — Every claim backed by file:line evidence or grep results. Speculation FORBIDDEN. Doc Staleness — Cross-reference changed files against related docs (feature docs, test specs, READMEs). Flag stale or missing updates.

Run python .claude/scripts/code_graph batch-query <f1> <f2> --json on changed files for test coverage and caller impact.

Blast Radius Pre-Analysis (MANDATORY FIRST STEP)

IMPORTANT MANDATORY MUST ATTENTION: FIRST action in every review. Call /graph-blast-radius BEFORE any other review work.

If .code-graph/graph.db exists, run graph-blast-radius analysis before reviewing changes:

• Call /graph-blast-radius skill (runs python .claude/scripts/code_graph blast-radius --json)
• Include in review: impacted files count, untested changes, risk level based on blast radius size
• Use results to prioritize file review order (highest-impact files first)

Graph-Assisted Change Review

For each changed file, trace full impact:

1. python .claude/scripts/code_graph trace <changed-file> --direction downstream --json — all files affected by changes
2. Flag any affected file NOT covered by tests
3. Catches cross-service impact simple diff review misses

Review Approach (Report-Driven Multi-Phase — CRITICAL)

MANDATORY FIRST: Create Todo Tasks for Review Phases Before starting, call TaskCreate with:

• [Review Phase 0] Run /graph-blast-radius to analyze change impact - in_progress (MUST ATTENTION BE FIRST)
• [Review Phase 0.3] Detect high-risk change types, create risk tasks - pending
• [Review Phase 0.7] Categorize changed files, create dimension review tasks - pending
• [Review Phase 0.5] Plan compliance check (skip if no active plan) - pending
• [Review Phase 1] Get changes and create report file - pending
• [Review Phase 2] Review file-by-file and update report - pending
• [Review Phase 3] Evaluate fresh-context gate; skip when findings already exist - pending
• [Review Phase 3.5] Run /code-simplifier on changed code files to optimize code quality - pending (MANDATORY when code files changed; skip docs-only diffs)
• [Review Phase 3.7] Run /integration-test-review coverage gate over full diff - pending (MANDATORY when behavior-bearing code changed; skip docs-only diffs; deferred to parent step inside $workflow-review-changes)
• [Review Phase 4] Generate final review findings - pending
• [Review Phase 5] Record stale-doc findings for validation/fix loop - pending
• [Review Phase 6] Why-review findings validation gate before any fix - pending (MANDATORY when findings exist)
• [Review Phase 7] Auto-fix validated findings and restart /review-changes from Phase 0 - pending (MANDATORY when validated findings remain)
• [Review Phase 8] Run /docs-update over full changeset to sync all impacted docs - pending

Update todo status as each phase completes.

Note: If Phase 1 reveals 10+ changed files, replace Phase 2-4 tasks with Systematic Review Protocol tasks: [Review Phase 2] Categorize and fire parallel sub-agents, [Review Phase 3] Synchronize and cross-reference, [Review Phase 3.5] Run /code-simplifier on changed code files, [Review Phase 3.7] Run /integration-test-review coverage gate, [Review Phase 4] Generate consolidated report

Phase 0: Run Graph Blast Radius Analysis (MANDATORY FIRST STEP)

IMPORTANT MANDATORY MUST ATTENTION: FIRST action before ANY other review work.

• Call /graph-blast-radius skill
• Record in report: changed files count, impacted files count, untested changes, risk level
• Use blast radius output to prioritize which files to review most carefully in Phase 2
• If .code-graph/graph.db does not exist, note "Graph not available — skipping blast radius" and proceed to Phase 0.3

Phase 0.3: Change Type Detection + Risk Tasks (MANDATORY)

Purpose: Identify HIGH-RISK change types in this diff before dimensional review. Each detected type creates a focused risk task. Change types are ORTHOGONAL to file category: the same file can be both a migration AND a security change — detect all independently.

Step 1: Detect change types

git diff --name-only HEAD       # unstaged
git diff --cached --name-only   # staged
# For branch or commit-range review, use the user-provided diff source:
git diff --name-only <base>...<head>

Evaluate each change type for this diff:

Change Type	Detection Signal (adapt to project's actual conventions)	TRUE if...
DepUpgrade	Dependency manifest changed (package.json, *.csproj, Gemfile, go.mod, requirements.txt, Cargo.toml, pom.xml, etc.)	A version number changed in any dependency manifest
Migration	File path or name suggests schema change (contains migration, schema, alter_table, or matches project's migration convention)	Any migration-convention file appears in the diff
BusEvent	New or modified event/message definition or consumer (infer from project conventions: consumer naming, message type directories)	A consumer or event class is new or its contract changed
ApiContract	API definition file changed (controller, route handler, OpenAPI/GraphQL schema) with route or field differences	Diff shows route/action/field additions or removals
SecurityChange	Auth/permission definition changed — infer from project conventions (auth middleware, permission constants, policy definitions)	Any auth or permission gate is added, removed, or changed
ConfigChange	Configuration files changed (e.g., *.json, *.yaml, *.env*, *Config*, *Options*, *Settings*, *.toml)	Any config-convention file appears
InfraChange	Infrastructure definition changed (Dockerfile, docker-compose*.yml, CI/CD pipelines, k8s manifests, IaC files)	Any infra-convention file appears

Record in report:

## Change Type Analysis
DepUpgrade: [YES/NO] | Migration: [YES/NO] | BusEvent: [YES/NO]
ApiContract: [YES/NO] | SecurityChange: [YES/NO] | ConfigChange: [YES/NO] | InfraChange: [YES/NO]

Step 2: Create change-type risk tasks (ALWAYS before any review work)

MANDATORY: Call TaskCreate for each TRUE signal. Do NOT create tasks for FALSE signals. The concerns listed are starting points — apply domain knowledge beyond them.

Condition	TaskCreate subject	Key concerns to investigate (starting points — expand with domain knowledge)
DepUpgrade TRUE	[Review-DepUpgrade] Dependency upgrade — semver, breaking changes, security advisories	Major/minor/patch? Read upstream CHANGELOG for breaking API changes. Grep deprecated API usage. Check transitive dependency changes. Known security advisories for new version? Peer dependency compatibility? Tests still passing?
Migration TRUE	[Review-Migration] DB migration — rollback path, volume impact, zero-downtime	Rollback/Down script exists? Table size estimate — large tables need lock analysis. NOT NULL column without default on non-empty table? Indexes created with no-lock option? Deployment ordering (before/after service deploy)? Backfill idempotent if run twice?
BusEvent TRUE	[Review-BusEvent] Cross-service event/message — consumer, idempotency, retry, poison pill	Consumer exists for new event? Retry strategy: prerequisite data not synced → wait-retry vs silent skip? Handler safe to run twice (idempotency)? Malformed message handling / dead-letter configured? Ordering assumptions vs broker guarantees?
ApiContract TRUE	[Review-ApiContract] API contract change — backward compat, client alignment, auth	Additive or breaking? Breaking → versioning or coordinated deploy required. All callers (UI, other services, tests) still compatible? New endpoint protected appropriately? No required response fields added without client update?
SecurityChange TRUE	[Review-SecurityChange] Security/permission change — all paths covered, no privilege escalation	All code paths reaching the gate covered? Negative test verifying unauthorized access DENIED? Privilege escalation possible? BOTH enforcement AND display control updated? Permission definition in single authoritative place (no duplicated strings risking drift)?
ConfigChange TRUE	[Review-ConfigChange] Config/env change — all environments, no secrets committed	New config key present in ALL environment configs? Hardcoded default masking missing production config? Any secret value in the diff? → CRITICAL if yes. Documented in setup guide? App fails fast if config missing?
InfraChange TRUE	[Review-InfraChange] Infrastructure change — env parity, no dev values in prod, reproducible build	Change affects all environments consistently? Hardcoded dev values (localhost, debug flags, dev credentials)? Pinned image/dependency versions? Local dev impact documented? CI/CD secret/permission requirements documented?

AI-SDD risk lenses: Apply these lenses when the changed files touch specs, workflows, tooling, or shared guidance.

Lens	Review focus
Contract/API/routes	Public behavior, clients, generated specs, and regression tests still agree.
Permissions/security-review	Enforcement, display controls, negative tests, and authoritative permission definitions align.
Config/flags	All environments, examples, fail-fast behavior, and docs are current.
Docs/spec/test drift	Canonical specs, Section 8 TCs, dashboards, and test code are synchronized or explicitly N/A.
Generated mirrors	Shared skill/workflow/tooling changes were synced to generated agent surfaces.
Reference-only artifacts	AI-extracted specs/TCs remain draft/reference until accepted by the owning review gate.

Step 3: Work through change-type tasks before dimensional review

For each created change-type task:

1. Set task to in_progress
2. Work through ALL applicable concerns — the table above is a starting point, not a ceiling
3. For each concern: cite file:line for PASS or describe finding for FAIL/WARN
4. Write findings under ## {Task Subject} Findings in report
5. Set task to completed

IMPORTANT: Complete ALL change-type tasks FIRST, then proceed to Phase 0.7. If no change-type signals detected, log "No high-risk change types detected" and proceed.

Phase 0.7: Change Surface Detection + Dynamic Review Tasks (MANDATORY)

Purpose: Let AI categorize the changes by nature and create review tasks accordingly. Derive categories from what the project's actual changed files are, never assume a fixed set. Think, don't classify into a preset grid. The AI owns this step entirely.

Step 1: Derive categories from the diff

git diff --name-only HEAD        # unstaged
git diff --cached --name-only    # staged
# For branch or commit-range review, use the user-provided diff source:
git diff --name-only <base>...<head>

For each changed file, infer its category by examining:

• Language/extension: What technology or domain does this file belong to?
• Directory semantics: What layer, module, or concern does this path represent in the project?
• Change nature: Is this logic, data schema, configuration, documentation, infrastructure, styling, testing, or tooling?

Do NOT map to fixed buckets. Derive categories that fit the current repository's actual structure and vocabulary.

Common category types to consider as starting points (not exhaustive — derive what fits):

• Server-side logic — business rules, API handlers, services, consumers, event processors
• Client-side logic — UI components, state management, API integration
• Data/Schema — migrations, schemas, seed data, domain models
• Styles/Assets — CSS/SCSS, design tokens, images, fonts
• Configuration — app settings, env vars, feature flags
• Infrastructure — Docker, CI/CD, pipelines, cloud manifests
• Documentation/Specs — markdown docs, ADRs, feature specs, test specs
• Tests — unit, integration, E2E test files
• Build/Tooling — build scripts, linters, formatters, bundlers, agent scripts
• Security — auth config, permission definitions, certificates

Record in report:

## Change Surface
{Category name} ({category type}): {N} files
{Category name} ({category type}): {M} files
...

Step 2: For each category, enumerate concerns and create a task

This is where you THINK, not fill in blanks. Apply SYNC:category-review-thinking for each category.

For EACH identified category:

1. Understand the domain: What is this category's purpose? What invariants govern it? Who depends on it?
2. Read project conventions: Grep for style guides, patterns docs, READMEs specific to this area
3. Derive concerns from first principles — DO NOT limit to any fixed list; trust your domain knowledge
4. Create a TaskCreate task named [Review-{Category}] {brief concern summary} listing derived concerns
5. Select the appropriate sub-agent type (see Sub-Agent Type Selection)

ALWAYS create: [Review-General] — universal quality: correctness, YAGNI/KISS/DRY, doc staleness, test coverage. Runs across ALL changed files regardless of other categories.

Sub-Agent Type Selection:

Category Nature	subagent_type
Code logic (any stack)	code-reviewer
Implements a documented spec/PBI	spec-compliance-reviewer (pre-pass, before code-reviewer)
Security, auth, permissions	security-auditor
Performance, query efficiency, latency	performance-optimizer
Documentation, plans, specs, ADRs	general-purpose
Infrastructure, CI/CD, config	general-purpose
Mixed or default	code-reviewer

Spec-compliance pre-pass (when the changeset implements a documented docs/specs/**capability or a PBI/story): spawnspec-compliance-reviewerFIRST — it verifies the implementation matches the spec (catches spec drift, missing requirements, extra features) BEFORE thecode-reviewerquality pass runs. Skip when no spec/PBI governs the change (thencode-revieweris the sole code pass). This is the one wired dispatch site forspec-compliance-reviewer (sub-agent-selection-guide.md "Spec compliance" row).

UI/frontend dimension (OWNED by this skill): When a Client-side logic or Styles/Assets category surfaces frontend files matching the project's configured frontend/UI file patterns, /review-changes owns the UI review and invokes /review-ui as its UI dimension — preferably as a dedicated ui-ux-designer sub-agent spawned in the same parallel batch as the other dimensional agents (inline-fold its checklist only when sub-agent spawning is unavailable). The checklist: long-content overflow (wrap vs ellipsis+tooltip), responsive multi-screen via flex, flex-grow vs fixed sizing (prefer min/max + flex over fixed px), z-index scale discipline (no raw numbers, no !important), and SCSS/BEM quality. This is the SAME behavior in both standalone and workflow contexts — /review-ui is NOT a separate workflow step; it always runs here. Skip entirely if no frontend files changed.

Step 3: Work through tasks in order

For each created task:

1. Set task to in_progress before starting
2. Review ONLY files in that category's scope
3. Apply SYNC:category-review-thinking — trust your domain knowledge beyond the examples there
4. Write findings to report under ## {Task Subject} Findings section
5. Set task to completed before starting next task

NEVER mark a dimension task completed by scanning. Work through each relevant file explicitly. For large categories (10+ files): escalate to a parallel sub-agent using the Systematic Review Protocol.

Phase 0.5: Plan Compliance Check (CONDITIONAL — only when active plan exists)

Check ## Plan Context in injected context:

• If "Plan: none" → skip, log "No active plan — skipping plan compliance"
• If "Plan: {path}" → load plan and verify:

1. Read {plan-path}/plan.md — get phase list and scope
2. Read relevant phase files — extract files to modify, test specifications, success criteria
3. Verify (MUST ATTENTION — all four):
   • Scope match — changed files listed in plan phases (warn on unplanned files)
   • Test evidence — tests mapped to completed phases have evidence (file:line), not "TBD"
   • Success criteria met — phase success criteria satisfied by changes
   • Test intent traceability — mapped tests name the business rule/invariant they protect, not just current behavior
4. Add "Plan Compliance" section to review report

Phase 1: Get Changes and Create Report File (MUST ATTENTION)

• Identify diff source: current working tree, staged changes, branch comparison, or commit range
• Run git status for current changes, or git diff --name-only <base>...<head> for branch comparisons
• Run git diff or git diff <base>...<head> to see actual changes
• Create plans/reports/code-review-{date}-{slug}.md
• Initialize with Scope, Files to Review, Blast Radius Summary sections

Phase 2: File-by-File Review (Build Report Incrementally)

For EACH changed file, read and immediately update report with:

• File path and change type (added/modified/deleted)
• Change Summary: what modified/added
• Purpose: why change exists
• Convention check: Grep 3+ similar patterns — does new code follow existing convention?
• Correctness check: Trace logic paths — handles null, empty, boundary values, error cases?
• DRY check: Grep similar/duplicate code — does this logic already exist elsewhere?
• Intention check: Does change serve stated purpose? Flag unrelated modifications
• Logic trace: Trace one happy path + one error path. Logic matches requirements?
• Semantic correctness: Does the artifact DO what it's supposed to?
• Issues Found: naming, typing, responsibility, patterns, bugs, over-engineering, logic errors
• Continue to next file, repeat

Phase 3: Fresh-Context Gate (Conditional Protocol — branch on findings and Phase 0.7 surface)

Protocol: SYNC:double-round-trip-review + SYNC:fresh-context-review + SYNC:review-protocol-injection (all inlined above). INVARIANT: Phase 3 is review-only. It may add findings, but it MUST NOT fix or validate them. Existing findings do not require a fresh-context re-review; any non-zero finding set flows to Phase 6 why-review validation, then Phase 7 auto-fix + full /review-changes restart from Phase 0. A Phase 7 restart alone is NOT a Phase 3 trigger.

Entry gate:

1. If Phase 2 or any dimensional review already found findings, SKIP Phase 3. Record: Skipped fresh-context pass because findings already exist; Phase 6 why-review validation is the required next gate. Then proceed to Phase 4 consolidation and Phase 6 validation.
2. If there are zero findings and no explicit independent-review trigger, SKIP Phase 3. Record: Skipped fresh-context pass because the current review is clean and no second-round trigger exists. Then proceed to Phase 4 finalization.
3. Run Phase 3 only when the current finding set is zero and at least one trigger exists:
   • the user explicitly requested a second-round/fresh-context review;
   • the selected workflow explicitly requires an independent reviewer for this invocation;
   • high-risk multi-domain changes need synthesis before a clean verdict.

Anti-waste rule: Do not run Phase 3 to re-review known findings before Phase 6. Do not run Phase 3 solely because Phase 7 restarted the review after fixes. The restarted review is already the required full pass; if it has zero findings and no explicit trigger above, finalize cleanly.

If the entry gate allows Phase 3, check categories from Phase 0.7 — if multiple distinct domains changed (e.g., server-side + client-side), run Synthesis Mode. Otherwise run Holistic Mode.

---

[SYNTHESIS MODE — when multiple distinct domains changed]

Spawn a Synthesis Agent as Round 2. Purpose: catch cross-boundary issues individual dimensional tasks cannot see.

When constructing Agent call prompt:

1. Copy Agent call shape from SYNC:review-protocol-injection template verbatim, subagent_type: "code-reviewer"

2. Embed all 11 universal SYNC blocks verbatim

3. Set Task as:

   Synthesis review — cross-boundary concerns ONLY across the changed domains in this diff.
   You have these dimensional findings as context: {summary from each dimensional task}.
   Re-read ALL changed files from scratch via your own tool calls.
   
   Focus ONLY on cross-boundary concerns — do NOT re-review each domain's internals:
   1. Contract Alignment: Do callers match what callees expose? (routes, parameters, field names, types)
   2. Data Consistency: Are field names/types consistent across layer boundaries?
   3. Security Boundary: Is auth enforced on BOTH sides (enforcement AND display control)?
   4. Cross-Layer Naming: Same concept named differently across layers?
   5. Missing Wiring: New producer with no consumer? New consumer with no producer? New feature with no doc?
   6. Documentation: Docs reflect changes in BOTH domains together?
   

4. Set Target Files as "use the selected diff source from Phase 1"

5. Set report path as plans/reports/synthesis-review-{date}.md

After sub-agent returns:

1. Read synthesis report
2. Integrate findings as ## Synthesis Round Findings in main report — DO NOT filter or override
3. If findings exist: do NOT fix here; mark Phase 3 complete and proceed to Phase 6 why-review validation
4. If no findings exist: proceed to Phase 4 finalization as a clean synthesis pass

---

[HOLISTIC MODE — when single domain changed]

No cross-boundary synthesis needed. Spawn standard holistic Round 2.

When constructing Agent call prompt:

1. Copy Agent call shape from SYNC:review-protocol-injection template verbatim
2. Select subagent_type based on domain's dominant concern (see Sub-Agent Type Selection)
3. Set Task as: "Review the selected diff holistically. Focus on big picture — overall technical approach coherence, architecture layers, logic placement (lowest layer), DRY violations, YAGNI/KISS, function complexity. Domain: {category from Phase 0.7} — apply domain knowledge for this category accordingly."
4. Set Target Files as "use the selected diff source from Phase 1"
5. Set report path as plans/reports/code-review-changes-round{N}-{date}.md

After sub-agent returns:

1. Read sub-agent's report
2. Integrate findings as ## Round {N} Findings (Fresh Sub-Agent) in main report — DO NOT filter or override
3. If findings exist: do NOT fix here; mark Phase 3 complete and proceed to Phase 6 why-review validation
4. If no findings exist: proceed to Phase 4 finalization as a clean holistic pass
5. Final verdict must incorporate findings from ALL review passes executed in this invocation

The following checks are handled by sub-agent but can be verified in Phase 4:

Clean Code & Over-engineering Checks:

• MUST ATTENTION YAGNI: Code solving hypothetical future problems? Unused params, speculative interfaces?
• MUST ATTENTION KISS: Unnecessarily complex solution? Could this be simpler while meeting the same requirement?
• MUST ATTENTION Function complexity: Methods too long? Nesting too deep? Multiple responsibilities?
• MUST ATTENTION Readability: Would a new team member understand without reading the full implementation?

Documentation Staleness Check (REQUIRED):

For each changed file, identify related documentation:

• Search for feature docs, architecture references, READMEs at module/service roots, API docs, test specs, setup guides
• Flag any doc where content no longer matches the changed artifact
• Flag missing docs for new features or components that should be documented
• Flag in the report with the specific stale section and what changed. Do not fix yet; Phase 6 must validate the finding before Phase 7 invokes /docs-update or applies doc edits.

Spec Drift Adjudication (REQUIRED when behavior changed): Apply SYNC:spec-drift-adjudication. For every behavior-bearing change, compare it against the canonical Feature Spec under docs/specs/ and classify any divergence as CODE-WRONG (change violates an intended spec rule/AC/invariant → BLOCKING finding, fix code/test), SPEC-STALE (intentional behavior change the spec no longer reflects → route to /spec [update] + /spec [mode=tests] [update]), AMBIGUOUS (AskUserQuestion before editing either side), or SPEC-SILENT (code correctly enforces an invariant no spec artifact states → ENRICH: add the §4 BR/§3 AC + a §8 TC via /spec [update] + /spec [mode=tests], then a guarding test). Record the verdict per changed behavior (Spec in sync when no divergence). Do not normalize drift just because code/tests pass. This is the bidirectional generalization of the post-bugfix "Was spec wrong?" check — it runs for ALL behavior-changing reviews, not only post-bugfix. Flag findings here; Phase 6 validates and Phase 7 fixes (CODE-WRONG fixes route through the fix loop; SPEC-STALE and SPEC-SILENT fixes route to the canonical spec updater before /docs-update).

Correctness & Bug Detection: Apply SYNC:bug-detection — null safety, boundaries, error handling, resource cleanup, concurrency.

Test Spec Verification: Apply SYNC:test-spec-verification — locate specs, verify coverage, flag gaps.

Integration Test Sync: Apply SYNC:integration-test-sync-check — surface missing tests via AskUserQuestion.

Translation Sync: Apply SYNC:translation-sync-check — for multilingual UI text changes, require translation updates or explicit user risk acceptance.

Phase 3.5: Code-Simplifier Quality Optimization (MANDATORY when code files changed)

Purpose: A correctness review proves the change WORKS; this gate proves the changed code stays easy to read, consistent, and cheap to change. Bug-finding (Phases 2-3) and simplification optimization are different lenses — run both. /code-simplifier is the canonical owner of clarity/consistency/maintainability refinement, so this skill delegates to it rather than duplicating that logic.

Entry gate:

• Run when the diff includes source-code or code-adjacent files (.cs, .ts, .tsx, .html, .scss, .css, tests, scripts, build/config-as-code).
• SKIP for docs-only / markdown-only diffs. Record: Skipped Phase 3.5 — no code files in diff.

Protocol:

1. Set the [Review Phase 3.5] task to in_progress.
2. Invoke /code-simplifier scoped to the changed code files only (pass the Phase 1 diff source — working-tree, staged, branch, or commit range — so it refines the related changed files, NOT the whole codebase). Direct it to surface reuse, DRY, KISS/YAGNI, naming, dead-code, altitude/layer-placement, and readability simplifications.
3. Capture, do NOT auto-apply. In review context, /code-simplifier runs in report mode: integrate its recommendations into the main report under ## Code-Simplifier Optimization Findings with file:line evidence and a one-line rationale each. These are findings, not edits.
4. Set the [Review Phase 3.5] task to completed.

Pipeline integration: Phase 3.5 findings are ordinary findings — they consolidate in Phase 4, are validated in Phase 6 (/why-review --validate-findings filters false-positive or change-cost-raising simplifications), and only validated ones are fixed in Phase 7. NEVER let /code-simplifier mutate the working tree before Phase 6 validates its suggestions.

Parent workflow boundary: When this skill is invoked as step 1 inside $workflow-review-changes, still run Phase 3.5 (it is a review dimension, producing findings for the report) but do NOT fix here — the parent workflow's /code-simplifier self-review and /feature-implement fix cycle own application. Record the findings and hand the report to parent step 2.

Phase 3.7: Integration-Test-Review Coverage Gate (MANDATORY when behavior-bearing code changed)

Purpose: Phases 2-3 prove the change is correct as written; this gate proves the change is covered and specced. /integration-test-review is the canonical owner of the 7-gate test-quality audit — its Gate 7 (Change Coverage) maps every behavior-changing production file in the diff to a covering test (integration-first; unit fallback needs explicit justification) AND a spec TC. This skill delegates to it rather than duplicating that logic. SYNC:integration-test-sync-check stays as the lightweight file-pairing check; this gate goes deeper — assertion quality, data-state verification, repeatability, and bidirectional spec↔test↔code alignment over the full change set.

Entry gate:

• Run when the diff includes behavior-bearing source code: handlers, commands, queries, services, entities, event consumers, controllers, background jobs, or frontend logic.
• SKIP for docs-only / markdown-only / pure styling-asset diffs. Record: Skipped Phase 3.7 — no behavior-bearing code in diff.

Protocol:

1. Set the [Review Phase 3.7] task to in_progress.
2. Invoke /integration-test-review scoped to the Phase 1 diff source (working-tree, staged, branch, or commit range) so it audits the FULL change set — changed production code AND changed test files, never just the test files. It runs all 7 quality gates, builds the Gate 7 Coverage Mapping Table, and cross-checks spec TCs in both directions.
3. Capture, do NOT auto-fix. Integrate its output into the main report under ## Integration-Test-Review Findings: per-gate verdicts, the Coverage Mapping Table, and every GAP / SPEC-GAP / unjustified COVERED-UNIT as a finding (GAP = HIGH severity minimum; CRITICAL for auth/money/data-integrity paths).
4. Set the [Review Phase 3.7] task to completed.

Pipeline integration: Phase 3.7 findings are ordinary findings — consolidated in Phase 4, validated in Phase 6, fixed in Phase 7. GAP fixes WRITE the missing test via /integration-test; SPEC-GAP fixes run /spec [mode=tests] [update]. The Phase 7 restart then re-audits coverage over the full updated diff, including the new tests.

Parent workflow boundary: When this skill is invoked as step 1 inside $workflow-review-changes, do NOT run Phase 3.7 locally — the parent workflow's dedicated /integration-test-review step owns the 7-gate audit and coverage mapping. Record Phase 3.7 deferred to parent workflow /integration-test-review step. (SYNC:integration-test-sync-check still applies locally as the lightweight pairing check.)

Phase 4: Generate Final Review Result

Update report with final sections (MUST ATTENTION — include every section below):

• Overall Assessment (big picture summary)
• Critical Issues (must fix before merge)
• High Priority (should fix)
• Architecture Recommendations
• Documentation Staleness (list stale docs with what changed, or "No doc updates needed")
• Spec Drift Adjudication (per behavior-changing file: CODE-WRONG / SPEC-STALE / AMBIGUOUS / SPEC-SILENT / Spec in sync, with the routed fix; or "No behavior change — N/A")
• Dual-Feedback Ledger (REQUIRED — see below; or "No behavior change — N/A")
• Positive Observations
• Suggested commit message (based on changes)

Dual-Feedback Ledger (REQUIRED for every behavior-changing finding). A behavior gap must feed back into BOTH the spec AND the tests — not merely fix the code. The Spec Drift Adjudication row above and the Phase 3.7 Gate 7 coverage row each cover only ONE axis; this ledger unifies them into a single "update BOTH" assertion so neither is silently skipped. For each behavior-changing finding, emit one row with two cells:

Finding (file:line)	Spec feedback	Test feedback
{cite}	{the §8 spec/Feature-Spec update needed — e.g. /spec [update] then /spec [mode=tests], OR N/A-because-CODE-WRONG-spec-already-correct}	{the TC/regression test needed — e.g. new §8 regression TC via /spec [mode=tests], or covering integration test via /integration-test}

• A blank cell on EITHER axis = FAIL. "N/A" alone is not allowed — every N/A must carry its reason inline (e.g. N/A — CODE-WRONG: canonical spec already describes the correct behavior, so no spec edit; only the regression TC is owed).
• CODE-WRONG finding → Spec feedback is typically N/A — spec correct, Test feedback is REQUIRED (regression TC first, per SYNC:spec-drift-adjudication).
• SPEC-STALE finding → BOTH cells are non-N/A: Spec feedback = /spec [update] then /spec [mode=tests]; Test feedback = the new/updated TC + guarding test.
• SPEC-SILENT finding (code correctly enforces an invariant the spec never states) → BOTH cells are non-N/A: Spec feedback = add the missing §4 BR / §3 AC (+ §5 invariant if applicable) and a §8 TC via /spec [update] + /spec [mode=tests]; Test feedback = the new property/regression test guarding the now-written invariant. The highest-value capture — never leave a discovered invariant only in code or only in tests.
• Covered-but-stale TC (Gate 7 SPEC-GAP routed by /integration-test-review — its Gate 7 classifies a TC that exists but no longer describes current behavior as a SPEC-GAP, not a satisfied coverage row): Spec feedback = correct the stale §8 TC via /spec [mode=tests] [update]; Test feedback = update the guarding test to the corrected TC.
• This ledger is itself an ordinary finding set: it consolidates here (Phase 4), is validated in Phase 6 (/why-review --validate-findings confirms BOTH axes are present for each behavior change), and its owed spec/test actions are applied in Phase 7. A ledger row with a blank axis that survives to Phase 7 = the review is INCOMPLETE.

Phase 5: Docs-Update Triage (CONDITIONAL)

If Documentation Staleness Check in Phase 4 identified stale docs:

1. Record impacted documentation and the proposed sync/update path in the review report
2. Add each stale-doc item to the Phase 6 findings validation payload
3. Do NOT invoke /docs-update yet; stale-doc findings are fixed in Phase 7 only after /why-review --validate-findings returns CLEAN for them
4. If Phase 7 later applies doc fixes, the next recursive /review-changes invocation must re-review the updated docs from Phase 0

Phase 5 triages only the docs the review FLAGGED. Regardless of whether anything is flagged here, the mandatory Phase 8 final /docs-update gate still runs once the review converges clean — it independently detects impacted docs this triage may have missed. Phase 5 is conditional; Phase 8 is unconditional.

Readability Checklist (MUST ATTENTION evaluate)

Before approving, verify artifacts are easy to read, maintain, understand:

• Schema visibility — Function computes data structure? Comment shows output shape so readers don't trace code
• Non-obvious data flows — Data transforms through multiple steps? Brief comment explains pipeline
• Self-documenting signatures — Params explain their role; flag unused params
• Magic values — Unexplained numbers/strings → named constants or inline rationale
• Naming clarity — Variables/functions reveal intent without reading implementation

Review Checklist

1. Architecture Compliance (MUST ATTENTION)

• Follows project's layer/module boundaries (read docs/project-config.json or equivalent)
• No cross-module/service direct data access where boundaries exist
• Logic placed in lowest responsible layer (not in orchestrators/top-layer classes)

2. Code Quality & Clean Code (MUST ATTENTION)

• Single Responsibility Principle — each function/class does ONE thing
• No code duplication (DRY) — grep for similar code, extract if 3+ occurrences
• Appropriate error handling following project patterns
• No magic numbers/strings (extract to named constants)
• Type annotations on all functions (where language requires)
• Early returns/guard clauses used
• YAGNI — no speculative features, unused parameters, premature abstractions
• KISS — simplest solution meeting requirement
• Follows existing codebase conventions (verify with grep for 3+ examples)

2.5. Naming Conventions (MUST ATTENTION)

• Names reveal intent (WHAT not HOW)
• Specific names, not generic (orderRecords not data)
• Booleans: prefix with state-indicating verb (isActive, hasPermission, canEdit)
• No cryptic abbreviations

3. Project-Specific Patterns (MUST ATTENTION)

• Read project's patterns/conventions reference docs BEFORE flagging violations
• Verify 3+ existing examples before concluding a pattern is a violation
• Flag deviation from project patterns with evidence (file:line showing existing pattern)

4. Security (MUST ATTENTION)

• No hardcoded credentials, tokens, or secrets
• Proper authorization checks at all entry points
• Input validation at system boundaries (user input, external APIs, message payloads)
• No injection risks (SQL, command, template, etc.)

5. Performance (MUST ATTENTION)

• No O(n²) complexity where O(n) or O(1) is possible (use lookup structures)
• No N+1 query patterns (batch load related data before iterating)
• Pagination for all list queries (never fetch unbounded result sets)
• Parallel operations where independent (not forced sequential)
• Async/await used correctly (no blocking in async context)
• Query patterns have appropriate indexes

6. Common Issues (MUST ATTENTION)

• Unused imports or variables
• Debug/logging statements left in that should not be in production
• Hardcoded values that should be configuration
• Missing async/await or promise handling
• Incorrect or absent exception handling
• Missing validation at boundaries

6.5 Bugfix Debugger Trace Gate (MUST ATTENTION)

For bugfix, failed-verification, stale/incorrect final output, regression, or behavior-changing fixes, FAIL review if any required proof is missing:

• Debugger Trace: End -> Start names the observed final state and final reader/query/renderer/assertion
• backward hops are evidenced from reader -> storage/projection/cache -> writer -> consumer/handler/job -> producer/origin
• all feeder paths that can write the final state are enumerated or explicitly marked unknown
• hypothesis matrix classifies root causes as primary, contributing, ruled out, latent, or unknown
• owning fix layer is justified as the lowest shared owner, not the symptom site by default
• forward convergence proof and regression test/proof mapping show why the final symptom cannot persist

7. Documentation Staleness (MUST ATTENTION)

• For each changed file: identify related docs (feature docs, architecture references, READMEs)
• Changed logic → verify relevant feature/module docs still accurate
• Changed tooling (scripts, configs, CI) → verify setup/getting-started docs still accurate
• New feature/component added → flag if corresponding doc missing
• Test specs reflect current behavior after changes
• API changes reflected in relevant API docs or specs
• Spec-drift adjudication (SYNC:spec-drift-adjudication): for every behavior-changing file, decide whether a divergence from the canonical Feature Spec is CODE-WRONG (change is the defect — BLOCKING, fix code/test), SPEC-STALE (change is intended — update spec via /spec [update] first), AMBIGUOUS (intended behavior unclear — AskUserQuestion before editing either side), or SPEC-SILENT (code correctly enforces an invariant no spec artifact states — enrich: add §4 BR/§3 AC + §8 TC + guarding test). Do not flag a divergence as a one-directional "stale doc" without naming which side is canonical. Unadjudicated behavior-vs-spec divergence is a FAIL; an unwritten-but-enforced invariant left uncaptured is equally a FAIL.

8. M1-M6 Compliance Gate — Code-to-Spec Drift (BLOCKING, MUST ATTENTION)

Contract: See .claude/skills/shared/sdd-artifact-contract.md → "AI-SDD Mandates (M1-M6)". This review enforces M6 for any spec/feature-doc/PBI/story/test-spec touched by — or supposed to be synced by — this change. Frame each check as: did this change introduce M1/M2 prose leakage, break a logical-ID mapping (M3), or create AC/expected-result ambiguity (M4)? A FAIL must name the violated mandate ID and cite the changed file + line. Passing an introduced M1-M5 violation makes this review itself defective.

Carriers are EXEMPT from M1/M2 — source identifiers stay CORRECT inside [Source: ...], **Evidence**, **IntegrationTest** fields, YAML frontmatter, and ```mermaid ``` blocks. Only flag leakage in spec/doc narrative prose. Banned prose token list: docs/project-reference/spec-principles.md §3.2. Scope this gate to changed artifact files (docs/specs/**, PBI/story/test-spec files in the diff); SKIP with a one-line note when the diff touches no such artifact.

• M1 — No introduced tech leakage in prose. FAIL if the diff adds a framework/product, language-native type, or product/design-pattern class name to spec/doc narrative prose, headings, or AC text (banned list in spec-principles.md §3.2). Cite the changed file + line + token.
• M2 — No introduced source code in prose. FAIL if the diff expresses a requirement as a class/method/file-path/namespace used as a noun instead of a business operation. Source identifiers belong only in evidence carriers. Cite the changed line.
• M3 — Logical-ID mapping preserved. FAIL if the change adds a requirement/rule/TC without a logical ID (FR-/BR-/OP-/TC-), strips a logical ID, demotes it below the [Source:] evidence, writes physical code coordinates or repository-root paths instead of a stack-portable abstract anchor ([Source: namespace/service/id]), OR drops the [Source:] abstract-anchor evidence (evidence is REQUIRED and KEPT — SECONDARY to the logical ID; a code move alone does NOT change the anchor — physical coords live only in the provenance sidecar).
• M4 — No introduced AC ambiguity. FAIL if the change leaves an AC/expected-result vague ("handle appropriately", "process normally", "as needed"), implementable two different ways while both claim conformance, or with no observable completion state / named error condition.
• M5 — Spec stays rebuildable. FAIL if the change makes the spec/doc depend on reading the new code to be understood (a zero-codebase-knowledge team could no longer re-implement on a different stack from the artifact alone). Cite the file + missing detail.

If ANY item fails → the verdict is FAIL; list each violated mandate ID with its changed-file/line citation in the Critical Issues or High Priority section.

Output Format

Provide feedback in this format:

Summary: Brief overall assessment

Critical Issues: (Must fix before commit)

• Issue 1: Description and suggested fix

High Priority: (Should fix)

• Issue 1: Description

Suggestions: (Nice to have)

• Suggestion 1

Documentation Staleness: (Docs that may need updating)

• Doc 1: What is stale and why
• No doc updates needed — if no changed file maps to a doc

Spec Drift Adjudication: (Behavior-changing changes only — per SYNC:spec-drift-adjudication)

• <behavior/file> → CODE-WRONG | SPEC-STALE | AMBIGUOUS | SPEC-SILENT — verdict + routed fix (/spec [update], regression TC, AskUserQuestion, or enrich-spec: add §4 BR/§3 AC + §8 TC + guarding test)
• Spec in sync — if changed behavior matches the canonical Feature Spec
• No behavior change — N/A — if the diff is docs/tooling/style only

Debugger Trace Gaps: (Bugfix/behavior-changing changes only)

• Trace complete — if the required trace, feeder paths, hypothesis matrix, owner, and forward proof are present
• Gap 1: Missing or weak trace evidence and why it blocks PASS

Goal Satisfaction: (MANDATORY before any PASS verdict — resolve the active Goal Contract per the goal-contract-satisfaction-loop protocol: active plan goal.md → plans/goals/{YYMMDD-HHmm}-{slug}/goal.md; if none exists, record No active goal — skipped: {one-line reason})

Success Criterion	Evidence	Status
{saved criterion}	{file:line, command output, report path}	PASS/FAIL/BLOCKED

• Overall PASS is BLOCKED while any required criterion is FAIL — a code-quality-clean review that misses the saved goal is NOT a PASS.
• BLOCKED status requires a user-facing escalation reason recorded in the matrix row and the goal file.
• Cite evidence references; never restate the goal text or copy secrets/sensitive payloads into the matrix or goal file.
• After the verdict, update the goal file: append an Iteration Log entry and sync its Goal Satisfaction matrix.

Positive Notes:

• What was done well

Suggested Commit Message:

type(scope): description

- Detail 1
- Detail 2

---

Systematic Review Protocol (for 10+ changed files)

When Phase 1 finds 10+ changed files, apply the Systematic Review Batching protocol (map-reduce: size-capped batches + hierarchical synthesis) defined below.

---

Workflow Recommendation

MANDATORY — NO EXCEPTIONS: If NOT already in a workflow, MUST use AskUserQuestion to ask user. Do NOT judge task complexity or decide "simple enough to skip" — user decides, not you:

1. Activate workflow-review-changes workflow (Recommended) — run the canonical workflow from .claude/workflows.json; it sequences this skill, findings validation, parallel reviewers, code-simplifier self-review, fix-plan cycle, full re-review restart, docs, and handoff.
2. Execute /review-changes directly — run this skill standalone

---

Architecture Boundary Check

For each changed file, verify no import from forbidden layer:

1. Read rules from docs/project-config.json → architectureRules.layerBoundaries
2. Determine layer — For each changed file, match path against each rule's paths glob patterns
3. Scan imports — Grep file for import statements
4. Check violations — If any import path contains layer name listed in cannotImportFrom, it is a violation
5. Exclude framework — Skip files matching any pattern in architectureRules.excludePatterns
6. BLOCK on violation — Report as critical: "BLOCKED: {layer} layer file {filePath} imports from {forbiddenLayer} layer ({importStatement})"

If architectureRules not present in project-config.json, skip silently.

---

Phase 6: Why-Review Findings Validation Gate (MANDATORY before fixing findings)

Purpose: Validate own findings BEFORE any fix. Verify EVERY finding is correct, proof-backed (file:line), reasonable, and convention-aligned. Catch false positives, inflated severity, and missed improvements before code/doc edits.

MANDATORY: REQUIRED todo task whenever findings exist. Register via TaskCreate as [Review Phase 6] Why-review findings validation gate (already in Phase task list above). Do NOT fix, docs-update, commit, or hand off until this gate passes CLEAN or reaches an explicit blocked state.

Trigger: Any finding produced (Critical, High, Medium, OR Low). Skip ONLY when report verdict is unconditional PASS with literally zero findings.

UNCONDITIONAL INVOCATION: If even one finding exists, the /why-review skill MUST be invoked via the Skill tool before any fix, docs-update, commit, or handoff. There is NO inline alternative — manually re-reading the cited file:lines, re-tracing in your head, or declaring the findings "already validated" does NOT count. The only way to pass this gate is an actual /why-review --validate-findings skill call that returns a verdict.

Parent workflow boundary: When this skill is invoked as step 1 inside $workflow-review-changes, do NOT run this Phase 6 locally. Stop after the review report and hand it to parent workflow step 2; the parent runs /why-review --validate-findings before any parallel reviewers or fixes.

Protocol (capped re-do loop):

1. Read own finalized report from plans/reports/{skill}-{date}-{slug}.md
2. Invoke the /why-review skill via the Skill tool (terminal --validate-findings mode — runs in the SAME main-agent session, never spawns a sub-agent, never recurses). "Same session" means the why-review skill executes in this conversation — it does NOT mean you may substitute your own inline re-reading for the skill call. The skill MUST actually run. Pass arg: --validate-findings plans/reports/{skill}-{date}-{slug}.md — for EACH finding verify (a) file:line proof exists and is accurate, (b) the finding is correct (re-trace the cited code), (c) severity is reasonable and not inflated, (d) it reflects project best practices/conventions; steel-man each rejected interpretation; and surface any MISSED finding or enhancement opportunity the review overlooked
3. Read the validation verdict path returned by why-review, expected as plans/reports/why-review-validate-{date}.md
4. Classify the why-review verdict:
   • CLEAN — all findings confirmed correct / proof-backed / reasonable / best-practice, AND no new finding issue or enhancement opportunity surfaced → append ## Why-Review Validation line to own report ("All N findings re-validated against actual code; no changes."), gate PASSES; if N > 0, proceed immediately to Phase 7.
   • HAS ISSUES — why-review demotes/removes a finding, flags a missing or inaccurate proof, OR surfaces a new finding issue / enhancement opportunity → go to step 5.
5. Reconcile: UPDATE own finalized report — revise severities, remove false positives, add the surfaced findings/enhancements, and record a ## Why-Review Validation Notes section citing what changed and why.
6. RE-DO /why-review --validate-findings on the UPDATED report (return to step 2) — re-validation is required ONLY because the report changed. Each pass is terminal (validate mode never recurses); the loop is owned and bounded HERE. Repeat until a why-review round comes back CLEAN, or max 2 re-do rounds (3 total validate passes) is reached.
7. If still not CLEAN after the cap: record unresolved items under ## Why-Review Validation — Unresolved and escalate to the user via AskUserQuestion instead of silently looping.

Skip conditions (record explicit reason if skipping):

• Verdict is unconditional PASS with zero findings → log "Skipped — no findings to validate" and do NOT run Phase 7
• /why-review itself is the active skill context → do NOT recurse; why-review re-validates via its own terminal --validate-findings mode (see its Findings Validation Gate)

Why this exists: AI reports can inherit confirmation bias, false positives, and severity inflation. Validation proves findings before edits; re-validation after report changes closes the gap where corrected findings are never checked again.

---

Phase 7: Recursive Auto-Fix + Full Re-Review Loop (MANDATORY when validated findings remain)

Purpose: Fixes change the review target. Next check MUST be a full new /review-changes invocation from Phase 0, not continuation from old review state.

Trigger: Phase 6 returns CLEAN and the validated report still contains one or more findings, weaknesses, stale-doc items, missing-test items, or required improvements.

Parent workflow boundary: When this skill is invoked as step 1 inside $workflow-review-changes, do NOT auto-fix or re-invoke /review-changes from here. Parent workflow steps 10-15 own /plan, /plan-review, /plan-validate, /why-review, /feature-implement, and the full restart gate.

Protocol:

1. Create fresh fix-cycle tasks before editing: one task per validated finding, one targeted-verification task, one /review-changes restart task.
2. Auto-fix validated findings at the owning layer. Stale docs: run /docs-update or edit canonical docs only after validation. Tests/specs: update canonical artifact before derived dashboards.
3. Run targeted verification for the fix set: tests, lint, docs/spec sync, SDD, graph, or config checks as applicable.
4. Append ## Fix Cycle {N} to the review report: findings fixed, files changed, verification commands/results, and unresolved items with reasons.
5. Re-invoke /review-changes in the SAME main-agent session on the full current review target:
   • Create brand-new task list for all phases
   • Re-run Phase 0 blast radius, Phase 0.3 risk detection, Phase 0.7 surface categorization, Phase 1 diff collection, and later phases
   • Re-read all changed files from scratch, including original changes and Phase 7 fixes
   • Treat previous report as historical context only; never reuse prior findings as truth
6. Repeat Phase 0 → Phase 7 until one complete /review-changes invocation produces unconditional PASS with zero findings and Phase 6 is skipped as "no findings to validate".

Stop conditions:

• If the same validated finding repeats for 3 full review invocations with no observable progress, stop and ask the user for a decision instead of spinning.
• If a finding cannot be safely auto-fixed without product/owner input, record the blocker and ask the user.
• If required verification tools or sub-skills are unavailable, stop and ask before adapting the protocol.

Non-negotiable rules:

• NEVER fix findings before /why-review --validate-findings confirms the current finding set.
• NEVER mark review clean after a fix without rerunning the full /review-changes protocol from Phase 0.
• NEVER review only the fixed files after a fix; review the full current diff because fixes can interact with earlier changes.
• NEVER reuse old todo tasks after restart; each recursive review invocation breaks down all phases again.
• NEVER declare unconditional PASS without the Output Format's Goal Satisfaction matrix showing every required saved criterion PASS (or BLOCKED with a user-facing escalation reason). A required-criterion FAIL is a validated finding for this fix loop.

---

Phase 8: Mandatory Final Docs-Update Gate (MANDATORY — always runs after the review/fix loop converges clean)

Purpose: Guarantee no stale docs survive the change. Phases 5-7 fix only docs the review flagged as findings; this terminal gate runs /docs-update unconditionally so impacted docs the dimensional review never surfaced still get reconciled against the actual changes. A clean code-review verdict does NOT imply docs are current.

Trigger: The review has converged — one full /review-changes pass produced zero findings and all validated fixes are applied. This gate ALWAYS runs in standalone mode; it is NOT gated on a flagged staleness finding.

Parent workflow boundary: When this skill is invoked as step 1 inside $workflow-review-changes, do NOT run Phase 8 locally — the parent workflow's own /docs-update step (after /feature-implement and the restart gate) owns the final docs sync. Record Phase 8 deferred to parent workflow /docs-update step.

Protocol:

1. Set the [Review Phase 8] task to in_progress.
2. Invoke /docs-update over the FULL changeset (the Phase 1 diff source plus any Phase 7 fixes). Let it detect impacted docs from the changes — feature docs, architecture references, READMEs, API docs, test specs, setup/getting-started guides.
3. If the Phase 4 Spec Drift Adjudication (SYNC:spec-drift-adjudication) returned any SPEC-STALE verdict — the canonical Feature Spec no longer reflects the intended behavior (includes the post-bugfix case where the spec documents the bug as correct behavior) — run /spec [update] BEFORE /docs-update so the spec is corrected to intended behavior first. Never let /docs-update codify broken or superseded behavior. CODE-WRONG verdicts are NOT a spec edit — they were already fixed in the Phase 7 code-fix loop. Any SPEC-SILENT verdict — an invariant the code already enforces but no spec artifact states — is an ENRICHMENT: run /spec [update] (add the §4 BR/§3 AC) + /spec [mode=tests] (add the §8 TC) BEFORE /docs-update, then ensure a guarding test exists; never leave the discovered invariant unwritten.
4. Record applied doc updates (or No impacted docs — verified N changed files against related docs) under ## Phase 8 Docs-Update in the review report.
5. Set the [Review Phase 8] task to completed.

Termination guarantee: Distinguish two edit kinds in Phase 8. Docs PROSE edits (narrative/reference doc text, no new spec rule) do NOT re-trigger the loop (no code behavior changed); they ARE subject to the M1-M6 spec-drift check (Review Checklist §8) and a final read-back — termination preserved. SPEC-CONTENT edits — a newly WRITTEN spec rule (a SPEC-SILENT invariant promoted to §3/§4/§8, or a SPEC-STALE correction that changes documented intent) — trigger exactly ONE bounded, module-scoped re-review of the whole package (spec + tests + code for the affected module) against the enriched spec, to confirm the newly-written rule is actually enforced in code and guarded by a test, and to surface any further hidden rule. That single bounded pass terminates unless it itself produces a new validated finding (which then enters the normal loop). Termination stays guaranteed: the bounded pass is module-scoped and runs at most once per enrichment — not a full Phase-0 restart — and a clean bounded pass ends the skill. This converts enrichment from "fires once at the end, never rechecked" into "fires, then gets one convergence pass," instead of looping review↔docs forever.

MANDATORY: Never declare the review complete or hand off until Phase 8 has run (or been explicitly deferred to the parent workflow). A passing review with skipped docs-update is an INCOMPLETE review.

---

Next Steps

MANDATORY — NO EXCEPTIONS after completing this skill, MUST use AskUserQuestion to present options. Do NOT skip because task seems "simple" or "obvious" — user decides:

• "/code-review (Recommended)" — Deeper code quality review
• "/watzup" — Wrap up session and review all changes
• "Skip, continue manually" — user decides

AI Agent Integrity Gate (NON-NEGOTIABLE)

Completion ≠ Correctness. Before reporting ANY work done, prove it:

1. Grep every removed name. Extraction/rename/delete touched N files? Grep confirms 0 dangling refs across ALL file types.
2. Ask WHY before changing. Existing values are intentional until proven otherwise. No "fix" without traced rationale.
3. Verify ALL outputs. One build passing ≠ all builds passing. Check every affected stack.
4. Evaluate pattern fit. Copying nearby code? Verify preconditions match — same scope, lifetime, base class, constraints.
5. New artifact = wired artifact. Created something? Prove it's registered, imported, reachable by all consumers.

Related Skills

Skill	Relationship	When to Call
/docs-update	Mandatory terminal gate (Phase 8) — final docs sync after the review/fix loop converges; also the primary fix path for flagged staleness	ALWAYS at Phase 8 once review is clean (standalone) — unconditional; AND during Phase 7 for validated staleness findings. Deferred to parent in $workflow-review-changes.
/spec-index	Derived index — regenerates the bucket INDEX.md/ERD FROM the Feature Specs (never a source of truth)	After specs change, to refresh navigation aids — NOT for correcting specs
/spec [update]	Canonical spec updater — corrects feature doc §1-8 (the single source of truth)	Called internally by docs-update; call directly for targeted update — and BEFORE docs-update if a spec-was-wrong scenario is detected
/spec [mode=tests] [update]	Test spec updater — called when test cases may be stale	Called internally by docs-update; call directly for targeted test case update
/integration-test-review	Mandatory coverage gate (Phase 3.7) — 7-gate test-quality audit + Gate 7 change-coverage mapping (every behavior change → covering test + spec TC)	ALWAYS at Phase 3.7 when behavior-bearing code changed (standalone); deferred to the parent's dedicated step in $workflow-review-changes. Skip only docs-only diffs
/review-ui	UI/frontend quality gate — overflow, responsive flex, z-index, SCSS/BEM	Owned by this skill — invoked internally as the UI dimension (ui-ux-designer sub-agent) when the diff has frontend/UI files; NOT a separate workflow step
/code-simplifier	Quality-optimization dimension — clarity/consistency/maintainability simplifications	Owned by this skill — invoked internally in Phase 3.5 (report mode) when the diff has code files; its findings flow through Phase 6 validation → Phase 7 fix
/code-review	Code quality — deeper review of changed code	Always follows review-changes quality pass

Standalone Chain

When called outside a workflow (i.e., user ran /review-changes directly):

review-changes (you are here)
  │
  ├─ Phase 3.5: Code-simplifier optimization (INTERNAL — /code-simplifier over changed code files, report mode)
  │    → Simplification findings feed Phase 6 validation → Phase 7 fix (skip docs-only diffs)
  │
  ├─ Phase 3.7: Integration-test-review coverage gate (INTERNAL — /integration-test-review over the FULL diff, 7 gates)