--- 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 ` **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:line`s 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 clears that round's exit bar — **zero findings in rounds 1-2, zero CRITICAL/HIGH/MEDIUM from round 3 (a LOW-only round ENDS the loop; list those LOWs as deferred, never fix-and-loop for them)**. 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.8 parallel full-mode `/why-review` rationale sub-agent, spawned in the SAME batch as the 0.7 agents → 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 clears that round's exit bar — **zero findings in rounds 1-2, zero CRITICAL/HIGH/MEDIUM from round 3 (a LOW-only round ENDS the loop; list those LOWs as deferred, never fix-and-loop for them)**, 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 0.8: Parallel Rationale Review** — Spawn a full-mode `/why-review` sub-agent in the SAME parallel batch as the 0.7 dimensional agents; its findings merge into the Phase 4 final evaluation (standalone-only) 7. **Phase 1: Collect** — Run git status/diff, create report file 8. **Phase 2: File Review** — Review each changed file, update report incrementally 9. **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 10. **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) 11. **Phase 3.7: Integration-Test-Review Coverage Gate (MANDATORY when behavior-bearing code changed)** — Invoke `/integration-test-review` over the full diff; its 8 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`) 12. **Phase 3.8: Domain Entity Gate (MANDATORY when the diff touches an entity/VO/aggregate)** — Apply `SYNC:domain-entity-change-gate` over the changed domain types; Mode A reviews inline, Mode B delegates to `/domain-entities-review` (its A–P checklist owns the gate). Findings flow into the same validation/fix loop (skip when no domain-entity surface) 13. **Phase 4: Finalize** — Generate critical issues, recommendations, suggested commit message 14. **Phase 5: Docs Triage** — Record stale-doc findings for validation/fix loop 15. **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. 16. **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 clears that round's exit bar — **zero findings in rounds 1-2, zero CRITICAL/HIGH/MEDIUM from round 3 (a LOW-only round ENDS the loop; list those LOWs as deferred, never fix-and-loop for them)**. When inside `$workflow-review-changes`, parent steps 10-15 own plan/feature-implement/restart. 17. **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 clears that round's exit bar — **zero findings in rounds 1-2, zero CRITICAL/HIGH/MEDIUM from round 3 (a LOW-only round ENDS the loop; list those LOWs as deferred, never fix-and-loop for them)**. When inside `$workflow-review-changes`, SKIP this — the parent workflow's dedicated standalone `/why-review` step (step 13) owns the holistic pass. 18. **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 clears that round's exit bar — **zero findings in rounds 1-2, zero CRITICAL/HIGH/MEDIUM from round 3 (a LOW-only round ENDS the loop; list those LOWs as deferred, never fix-and-loop for them)**; do not add a fresh-context pass just because findings exist or a fix cycle restarted the review - **Severity floor — from round 3, LOW stops blocking.** Rounds 1-2 of the Phase 7 loop require zero findings at any severity. **From round 3 the bar is zero validated CRITICAL/HIGH/MEDIUM — a round whose validated findings are ALL LOW ENDS the loop.** Do NOT restart Phase 0 for LOW findings alone: record them under `## Deferred LOW Findings (severity floor, round ≥3)` in the report and advance to Phase 7.5. NEVER re-tier a real CRITICAL/HIGH/MEDIUM down to LOW to reach the exit, and NEVER apply the floor to a binary gate (a missing §8 TC for a behavior change is HIGH, not a deferrable LOW). Severity tiers per `SYNC:severity-rubric`. > **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 ...` plus `git diff --name-only ...` - Commit range: `git diff ..` plus `git diff --name-only ..` ## 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 unnecessari