changes-review

name	changes-review
version	3.0.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.
• Self-recursive loop is protocol-bound (goal-gated when available). Standalone mode's FIRST action (Phase -1) binds the review loop as a standing protocol obligation you self-drive — and installs a /goal Stop-hook condition WHEN available — so stopping is BLOCKED until the loop converges. Findings are never auto-fixed on sight: validate (Phase 6 /why-review --validate-findings, an actual skill call) → SELF-FIX (Phase 7) → restart /changes-review from Phase 0 over the WHOLE updated diff (combined with prior fixes, not just the last fix), looping until one whole pass has zero findings. Inside $workflow-review-changes you skip Phase -1, stop after the report, and hand findings to the parent (which owns the goal).
• 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).
• MUST ATTENTION — run ALL main phases in order (the steps AI keeps forgetting): Phase -1 bind self-recursive review loop — protocol-primary, optional /goal gate when available (standalone, FIRST action) → 0 /graph-blast-radius → 0.1 change-context + full-pipeline trace (tier FE↔BE + cross-service/event) → 0.3 change-type risk tasks → 0.7 surface-detection dimension tasks → 0.5 plan compliance → 1 collect diff + create report → 2 file-by-file review → 3 fresh-context gate (SKIP when findings exist) → 3.5 /code-simplifier (code diffs) → 3.7 /integration-test-review Gate-7 coverage (behavior diffs) → 4 finalize + Dual-Feedback Ledger → 5 docs triage → 6 /why-review --validate-findings → 7 self-fix + full restart from Phase 0 → 7.5 holistic full-mode /why-review → 8 /docs-update (unconditional terminal) — why: a skipped phase silently drops a guard (coverage, spec-drift, holistic review, or docs sync) and ships unreviewed work.

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): changes-review 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:

0. Phase -1: Bind the Self-Recursive Review Loop (FIRST ACTION — standalone-only; protocol-first, /goal optional) — Before any other work, in standalone mode bind the review loop as a standing protocol obligation you self-drive — and, WHEN available, invoke the /goal command as an ACTUAL call — with a self-recursive review-loop condition so stopping is BLOCKED until the loop converges: review the full diff → validate findings (Phase 6) → SELF-FIX validated findings (Phase 7) → restart /changes-review from Phase 0 over the WHOLE updated diff (combined with the prior fixes, never just re-checking the last fix) → loop until one complete pass has zero findings, then docs-update. Skip this gate when running as step 1 inside $workflow-review-changes (the parent owns the goal). Full procedure in Phase -1.
1. Phase 0: Blast Radius — Call /graph-blast-radius skill FIRST (if .code-graph/graph.db exists)
2. Phase 0.1: Change Context & Full-Pipeline Impact Trace (MANDATORY comprehension-first) — Note the change context, then holistically trace the main affected area's full pipeline across BOTH boundaries — client↔server tier (FE↔BE) AND service/event/external — classifying each seam/touchpoint NONE/ADDITIVE/BREAKING (explicit N/A for single-tier or monolith)
3. Phase 0.3: Change Types — Detect high-risk change types; create risk tasks
4. Phase 0.5: Plan Compliance — Verify against active plan (conditional)
5. Phase 0.7: Surface Detection — AI categorizes changed files; creates dimension tasks
6. Phase 1: Collect — Run git status/diff, create report file
7. Phase 2: File Review — Review each changed file, update report incrementally
8. 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
9. 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)
10. 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)
11. Phase 4: Finalize — Generate critical issues, recommendations, suggested commit message
12. Phase 5: Docs Triage — Record stale-doc findings for validation/fix loop
13. 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.
14. Phase 7: Recursive Fix + Full Re-Review Loop (standalone-only) — If validated findings remain in standalone mode, auto-fix them, then re-invoke /changes-review from Phase 0 with a fresh task breakdown over the full current diff; repeat until an entire review pass has zero findings. When inside $workflow-review-changes, parent steps 10-15 own plan/feature-implement/restart.
15. Phase 7.5: Holistic Standalone Full-Mode Why-Review Gate (standalone-only) — Once the dimensional review/fix loop converges clean, invoke /why-review in FULL mode (NOT --validate-findings) ONCE over the WHOLE review target combined with the current changes as a single artifact — a real standalone /why-review call, the same as a user running /why-review against the target directly. The per-file/per-dimension reviewers and the Phase 6 validate-findings gate routinely miss holistic design-rationale and whole-package issues that a standalone full-mode review catches. If it surfaces findings, fix them and re-run Phase 7.5 (run→fix→run) until a full-mode pass returns zero findings. When inside $workflow-review-changes, SKIP this — the parent workflow's dedicated standalone /why-review step (step 13) owns the holistic pass.
16. Phase 8: Mandatory Final Docs-Update Gate (MANDATORY — runs once the review/fix loop converges clean) — After the review reaches zero findings and all fixes are applied, ALWAYS invoke /docs-update over the full changeset as the terminal step so no stale docs survive. This is unconditional (not gated on a flagged finding) — /docs-update independently detects impacted docs the review may not have surfaced. When inside $workflow-review-changes, the parent workflow's /docs-update step owns this; do not run it locally.

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
• FIRST ACTION (standalone): bind the Phase -1 self-recursive review loop — the protocol loop is the primary, host-independent binding you self-drive, plus an optional /goal Stop-hook gate WHEN available — so stopping is blocked until a complete review pass over the whole diff is clean; soft "loop until clean" prose alone is not enough, and the protocol binding makes abandoning the loop early impossible whether or not /goal exists
• After Phase 6 validation, the skill MUST SELF-FIX every validated finding in Phase 7 (standalone) — a surfaced+validated finding that is reported but left unfixed keeps the review loop open (and, when available, the /goal gate); never hand validated findings back to the user as "recommendations" in standalone mode
• Every fix cycle invalidates the prior review result; restart /changes-review from Phase 0 and review the full updated diff AS A WHOLE FROM THE BEGINNING — combined with the prior fixes, NOT just re-reviewing the previous cycle's fix in isolation
• Continue review → validate findings → self-fix → full whole-diff 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 REVIEW STEP)

IMPORTANT MANDATORY MUST ATTENTION: FIRST review action in every review — only the Phase -1 self-recursive review-loop binding (standalone) precedes it. 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 -1] Bind self-recursive review loop — protocol-primary; optional /goal gate when available (standalone-only; skip inside $workflow-review-changes) - in_progress (MUST ATTENTION BE FIRST)
• [Review Phase 0] Run /graph-blast-radius to analyze change impact - pending (FIRST review step after the goal gate)
• [Review Phase 0.1] Note change context + holistic full-pipeline trace across BOTH boundaries — client↔server tier (FE↔BE) AND service/event/external — classify each seam/touchpoint NONE/ADDITIVE/BREAKING - pending (MANDATORY comprehension-first; record explicit N/A for single-tier or monolith)
• [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 /changes-review from Phase 0 - pending (MANDATORY when validated findings remain)
• [Review Phase 7.5] Run standalone full-mode /why-review over the whole target+diff; fix and re-run until zero findings - pending (MANDATORY when review loop converges clean; standalone-only — skip inside $workflow-review-changes)
• [Review Phase 8] Run /docs-update over full changeset to sync all impacted docs - pending (MANDATORY FINAL — always runs once review converges to zero findings; never skipped)

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 -1: Bind the Self-Recursive Review Loop (FIRST ACTION — standalone-only; protocol-first, /goal optional)

IMPORTANT MANDATORY MUST ATTENTION: This is the VERY FIRST action in standalone mode — before Phase 0, before any git/graph command. Its purpose is to make the review→fix→re-review loop UNABANDONABLE. The loop is bound by TWO layers: the protocol loop (primary, host-independent) you self-drive, plus an optional /goal accelerator (a session Stop hook) WHEN available. Correctness rides on the protocol loop — hooks/commands are accelerators only, so /goal's absence NEVER weakens it.

Entry gate:

• Run in standalone invocation (user called /changes-review directly).
• SKIP when this skill is invoked as step 1 inside $workflow-review-changes — the parent workflow owns the loop goal (its own Phase -1 / step-0 goal gate). Record: Phase -1 deferred to parent $workflow-review-changes goal gate.

Procedure:

1. Set the [Review Phase -1] task to in_progress.

2. Protocol loop — ALWAYS binding (hook/command-independent). You, the running agent, are personally responsible for not stopping until the loop below converges or bounded-escalates. This binds Claude, Codex, and Copilot equally, whether or not /goal exists:

   Review the full diff → run /why-review --validate-findings on every finding → SELF-FIX each validated finding → restart /changes-review from Phase 0 over the WHOLE updated diff (combined with the prior fixes, not just the last fix) → loop until one complete review pass finds zero findings → then run Phase 7.5: one standalone FULL-mode /why-review over the whole target+diff (NOT --validate-findings), fixing and re-running until it is clean → only then run the Phase 8 /docs-update. Do not stop while any validated finding is unfixed, any review pass is non-clean, or the holistic full-mode /why-review has unaddressed findings.

3. /goal command — invoke as an accelerator WHEN AVAILABLE. If a /goal command exists and you are permitted to run it in this environment, ALSO invoke it (the actual command, NOT a paraphrase) with the SAME condition, so a session Stop hook mechanically enforces the loop:

   /goal changes-review self-recursive loop: review the full diff → run /why-review --validate-findings on every finding → SELF-FIX each validated finding → restart /changes-review from Phase 0 over the WHOLE updated diff (combined with the prior fixes, not just the last fix) → loop until one complete review pass finds zero findings → then run Phase 7.5: one standalone FULL-mode /why-review over the whole target+diff (NOT --validate-findings), fixing and re-running until it is clean → only then run the Phase 8 /docs-update. Do not stop while any validated finding is unfixed, any review pass is non-clean, or the holistic full-mode /why-review has unaddressed findings.
   

   The /goal Stop hook blocks stopping until that condition holds and auto-clears when it does — do not tell the user to clear it. If /goal is unavailable, unregistered, or not permitted (e.g. Codex/Copilot, or a Claude run without the command): DO NOT error, DO NOT block, and DO NOT invent a stand-in gate. Record /goal accelerator unavailable — review loop bound by protocol (Phase -1 step 2) on the [Review Phase -1] task and proceed; the protocol loop IS the gate, enforced by discipline instead of a hook.

4. Set the [Review Phase -1] task to completed and proceed to Phase 0.

Why bind the loop, not just prose: the loop rules below ("restart from Phase 0", "continue until zero findings") are soft directives an agent can rationalize away after one cycle. Binding them as a standing protocol obligation (and, WHEN available, a /goal Stop hook) converts them into a mechanical block — the session cannot end with a validated finding still unfixed or a non-clean review pass. — why: a review that reports findings but stops before fixing-and-reproving them ships unreviewed work.

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

IMPORTANT MANDATORY MUST ATTENTION: FIRST review step, after the Phase -1 goal gate and 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.1

Phase 0.1: Change Context Comprehension & Full-Pipeline Impact Trace (MANDATORY — comprehension-first)

IMPORTANT MANDATORY MUST ATTENTION: First comprehension step — before any file-by-file or dimensional review. Blast radius (Phase 0) gathers impact data; here holistically UNDERSTAND the change, trace main affected area's full pipeline across every boundary it crosses. Apply BOTH the Cross-Stack Impact Trace and Cross-Service Check protocols (bodies in the SYNC section below). This holistic first-pass map FEEDS the later Phase 0.3 change-type risk tasks and the conditional Phase 3 synthesis — does not replace them.

Write a one-paragraph Change Context note (what changed · intent · originating tier · main affected feature/flow), then run both traces per the Cross-Stack Impact Trace and Cross-Service Check protocols (SYNC blocks below).

Parent workflow boundary: Inside $workflow-review-changes, still RUN Phase 0.1 (comprehension is local review value) but hand findings to the parent — same pattern as Phase 3.5/3.7.

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): spawn spec-compliance-reviewer FIRST — it verifies the implementation matches the spec (catches spec drift, missing requirements, extra features) BEFORE the code-reviewer quality pass runs. Skip when no spec/PBI governs the change (then code-reviewer is the sole code pass). This is the one wired dispatch site for spec-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, /changes-review owns the UI review and invokes /ui-review 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 — /ui-review 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 /changes-review 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/changes-review-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
• Cross-Boundary Impact (from Phase 0.1 — per client↔server seam AND per service/event/external touchpoint: NONE / ADDITIVE / BREAKING with routed fix; or explicit "Single-tier / monolith — N/A")
• 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 /changes-review 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)

Concise hot-path pass — OOM first, then structure, then batching. Deep multi-dimension analysis belongs to /performance-review; flag here, route there if it needs measurement.

• [MOST IMPORTANT] OOM / out-of-memory bad practices — bound EVERY result set (page/limit/cursor); no unbounded read-all / SELECT *, no full materialization before paging/filtering, stream/chunk instead of buffering a whole export, no blobs/large-JSON/tracked entities loaded for list views, no unbounded cache/accumulator, no accidental multiple enumeration. Reduce rows AT THE SOURCE — row COUNT before row SIZE
• Best data structure & algorithm for the stack — O(1) Set/Map/dict lookup instead of linear find/includes/contains inside a loop; no O(n²) where O(n log n)/O(n)/O(1) exists
• Batch once, or parallelize — never serial fan-out — collapse per-item query/API/cache calls into one batched call (IN/bulk/aggregate/prefetch); run independent calls bounded-parallel, not sequential awaits (preserve ordering/authorization)
• No N+1 query patterns (batch load related data before iterating); query patterns have appropriate indexes
• Async/await used correctly (no blocking in async context)

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-M7 Compliance Gate — Code-to-Spec Drift (BLOCKING, MUST ATTENTION)

Contract: See .claude/skills/shared/sdd-artifact-contract.md → "AI-SDD Mandates (M1-M7)". 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), create AC/expected-result ambiguity (M4), or ADD A NON-DEMOABLE CASE TO A BUSINESS SPEC (M7)? A FAIL must name the violated mandate ID and cite the changed file + line. Passing an introduced M1-M5/M7 violation makes this review itself defective. (M6 binds THIS review, not the artifact — hence the artifact-facing set reads M1-M5 and M7, never "M1-M6".)

Carriers are EXEMPT from M1/M2 — source identifiers stay CORRECT inside [Source: ...], **Evidence**, CoveredBy: fields, legacy **IntegrationTest:** migration 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.
• 🔴 M7 — No introduced NON-DEMOABLE case in a business spec. For every case this diff ADDS to a business-tree artifact, apply the demo test to its BODY: "what would a stakeholder SEE change?" — no answer → FAIL as TECHNICAL-ONLY. FAIL an added case whose When is an invocation (a handler runs, a consumer receives, a job fires, data syncs, a model/schema is inspected) or whose Then asserts a schema/type/nullability/column/call-count rather than a business outcome. Cite the changed file + line + the offending When/Then. The case belongs in the technical tree — the fix is to move it there, or rewrite it demoably, NOT to reword it.

🔴 M7 is the gate on the bugfix→spec pump — this is the step where business specs actually rot. The failure has a signature: a technical bug is fixed (a sync, a consumer, an event handler, a load path, a UI defect), no business behavior changes, and a new case is nonetheless appended to the business spec — carefully worded to avoid technical vocabulary, and therefore passing M1 and M2 cleanly while remaining a case no user or QC could ever demo. ⚠️ M1 governs vocabulary; M7 governs subject matter. A reviewer who checks only M1/M2 waves these through forever, one defensible case at a time, and the business tree fills with cases that cannot be demoed.

Ask on every diff that fixes a technical bug: did the BUSINESS behavior change? If NO, the business spec should usually gain NOTHING — ⚠️ "a no-op is a correct outcome" (see the contract's [HARD] A no-op is a correct outcome). A regression test in the technical tree is the correct home for a technical fix. Adding a business TC "for coverage" is the defect, not diligence.

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 /changes-review 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). A Medium or Low severity NEVER exempts a finding — even a single low-severity nit, naming note, or "minor" suggestion triggers the full /why-review --validate-findings gate. The ONLY skip is a literally empty finding set (unconditional PASS, zero findings of any severity). — why: severity is itself a finding claim the gate must validate; letting "it's only Low" bypass validation is exactly the inflation/false-positive the gate exists to catch.

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):