--- name: story-review version: 1.1.0 description: '[Code Quality] Use when you need to review user stories for completeness, coverage, dependencies, and quality before implementation.' --- > **[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. ## Quick Summary **Goal:** Auto-review user stories for completeness, acceptance criteria coverage, dependency ordering, and quality before implementation proceeds. **Key distinction:** AI self-review (automatic), NOT user interview. **Be skeptical. Apply critical thinking, sequential thinking. Every claim needs traced proof, confidence percentages (Idea should be more than 80%).** ### Frontend/UI Context (if applicable) > When this task involves frontend or UI changes, - Component patterns: `docs/project-reference/frontend-patterns-reference.md` (read directly when relevant; do not rely on hook-injected conversation text) - Styling/BEM guide: `docs/project-reference/scss-styling-guide.md` - Design system tokens: `docs/project-reference/design-system/README.md` ## First Principle — Easy to Change > **The success metric of every coding decision is _future change cost_.** > DRY, SRP, abstraction, design patterns, naming, layering, tests — every > technique exists to serve one goal: **making the next change cheaper**. When evaluating code, a refactor, a test, or an abstraction, ask: **does this make the next change cheaper or more expensive?** - Reject "best practices" that raise change cost (premature abstraction, speculative generality, leaky indirection, ceremony without payoff). - Name the real enemies in findings: **coupling, hidden state, duplicated knowledge, unclear intent, irreversible decisions exposed too early**. - A simpler design that is easy to change beats a sophisticated design that isn't. Apply this lens **before** invoking any specific rule, pattern, or checklist below — if a downstream rule would raise change cost, this principle wins. --- ## Adversarial Review Mindset (NON-NEGOTIABLE) **Default stance: SKEPTIC challenging story quality, not confirming it.** > **Self-review trap:** You wrote these stories. You will find them coherent because you made them coherent. This section forces deliberate challenge before rubber-stamping. ### Adversarial Techniques (apply ALL before concluding) **1. Strawman AC Check** For each acceptance criterion: "Is this AC so obvious it was only included to pad coverage?" (e.g., "User can see the page" — trivially true and tests nothing meaningful). Flag trivial ACs that would pass even if the feature is completely broken. **2. Vertical Slice Challenge** For each story: "Can a stakeholder demo THIS STORY ALONE to a real user and get useful feedback?" If the story only delivers a backend endpoint, a DB migration, or a UI component in isolation — it is a horizontal layer, not a vertical slice. Flag it. **3. Dependency Challenge** If story B is blocked by story A: "What happens to the sprint if story A is descoped or delayed?" A story set with rigid sequential dependencies is fragile. Are dependencies truly required, or can stories be resequenced? **4. INVEST Violation Hunt** Deliberately look for the WEAKEST INVEST criterion for each story. Ask: "Which of I/N/V/E/S/T does this story fail most obviously?" If a story is not Estimable — why not? If not Independent — can it be split? **5. Pre-Mortem** Assume all stories in this set are implemented exactly as written. The feature ships and fails. Write the most plausible failure scenario. Which story was the gap? **6. Contrarian Pass** Before writing any verdict, generate at least 2 sentences arguing the OPPOSITE conclusion. Then decide which argument is stronger. ### Forbidden Patterns - **"Stories follow GIVEN/WHEN/THEN"** → Format is NOT quality. Are the scenarios meaningful? - **"Coverage looks complete"** → What failure mode is NOT covered by any story? - **"Dependencies are identified"** → Are they truly required, or is there a split that removes them? - **"Vertical slices"** → Can you actually demo each story independently? Prove it. - **Confirming story set without adversarial challenge** → Forbidden. ### Anti-Bias Gate (MANDATORY before finalizing verdict) - [ ] Identified at least 1 trivial/strawman AC across the story set - [ ] Verified each story delivers a demeable vertical slice - [ ] Checked dependency chain — fragile if >2 sequential dependencies - [ ] Found the weakest INVEST criterion per story - [ ] Ran pre-mortem (plausible failure scenario) - [ ] Generated at least 2 sentences arguing the opposite verdict If any box is unchecked → adversarial review incomplete. Go back. ## Workflow 1. **Locate stories** — Find story artifacts in `team-artifacts/stories/` or plan context 2. **Load source PBI** — Read the parent PBI to cross-reference acceptance criteria 3. **Evaluate checklist** — Score each check 4. **Classify** — PASS/WARN/FAIL 5. **Output verdict** ## Checklist ### Required (all must pass) | # | Check | Presence | Quality Depth | | --- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- | | 1 | **AC coverage** — Every acceptance criterion from PBI has at least one corresponding story | Does every PBI AC have a story? | Does every PBI AC have a story? Or are some ACs split across multiple stories in ways that create coverage gaps? | | 2 | **GIVEN/WHEN/THEN** — Each story has minimum 3 BDD scenarios (happy, edge, error) | Are all 3 BDD parts present per scenario? | Are all 3 BDD parts present per scenario? Are scenarios testing REAL user behavior or just "the system does X"? | | 3 | **INVEST criteria** — Stories are Independent, Negotiable, Valuable, Estimable, Small, Testable | Are all 6 INVEST criteria named or implied? | Are stories genuinely Independent (no hidden chains), Valuable (real user impact), Testable (automatable)? | | 4 | **Story points** — All stories have SP <=8 (>8 must be split) | Are SP assigned and all <=8? | Do SP reflect actual complexity? Is <=8 justified, or is the story undersized to pass the gate? | | 5 | **Dependency table** — Story set includes dependency ordering table (must-after, can-parallel, independent) | Does a dependency ordering table exist? | Does the ordering reflect ACTUAL dependencies, not just arbitrary sequencing? | | 6 | **No overlapping scope** — Stories don't duplicate functionality | Do any 2 stories reference the same AC? | Do any 2 stories claim the same AC? Would implementing both create duplication? | | 7 | **Vertical slices** — Each story delivers end-to-end value (not horizontal layers) | Does each story touch more than one layer (UI + API or API + DB)? | Can a stakeholder demo EACH story to a real user independently? Or do some deliver only infrastructure? | | 8 | **Authorization scenarios** — Every story includes at least 1 authorization scenario (unauthorized access → rejection) per PBI roles table | Is an authorization scenario present per story? | Is the unauthorized-access scenario testing a realistic attack vector, not just "wrong role → 403"? | | 9 | **UI Wireframe section** — If story involves UI: has `## UI Wireframe` section per UI wireframe protocol (wireframe + component tree + interaction flow + states + responsive). If backend-only: explicit "N/A" | Does the section exist (or explicit N/A for backend-only)? | If UI: does the wireframe show interaction flow + states + responsive breakpoints? If backend-only: is "N/A" explicit? | ### Recommended (>=50% should pass) | # | Check | Presence | Quality Depth | | --- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- | | 1 | **Edge cases** — Boundary values, empty states, max limits addressed | Are edge case scenarios listed? | Are boundary values story-specific (not generic "empty state")? Do they include concurrency or partial-data scenarios? | | 2 | **Error scenarios** — Failure paths explicitly covered in stories | Are error path scenarios present? | Do error stories specify the exact error message/code returned, or just "shows error"? | | 3 | **API contract** — If API changes needed, story specifies contract | Is a request/response contract defined? | Does the contract specify request/response schema fully? Are breaking vs non-breaking changes distinguished? | | 4 | **UI/UX visualization** — Frontend stories have component decomposition tree with EXISTING/NEW classification, design token mapping, and responsive breakpoint behavior per UI wireframe protocol | Is a component decomposition tree present? | Are components EXISTING vs NEW classified? Are design token names (not just colors) specified? | | 5 | **Seed data stories** — If PBI has seed data requirements, Sprint 0 seed data story exists | Does a seed data story exist (or N/A if not required)? | If present, does the seed data story specify the exact data shape needed? | | 6 | **Data migration stories** — If PBI has schema changes, data migration story exists | Does a data migration story exist (or N/A if no schema changes)? | If present, does it specify rollback behavior? | ## M1-M6 Compliance Gate (BLOCKING — each check FAILs the review) > **Contract:** See `.claude/skills/shared/sdd-artifact-contract.md` → "AI-SDD Mandates (M1-M6)". This review enforces M6: a story set that violates any of M1-M5 MUST receive a FAIL verdict that names the violated mandate ID and cites the exact story file + line/scenario. Passing an M1-M5 violation makes this review itself defective. > > Carriers are EXEMPT from M1/M2 — source identifiers are CORRECT inside `[Source: ...]`, `**Evidence**`, `**IntegrationTest**` fields, YAML frontmatter, and ` ```mermaid ``` ` blocks. Only flag leakage in story narrative prose (title, description, Given/When/Then scenarios, acceptance-criteria text). Banned prose token list: `spec-principles.md` §3.2. - [ ] **M1 — Tech-agnostic story prose.** FAIL if a story title, description, or BDD scenario names a framework/product, a language-native type, or a product/design-pattern class name (banned-token list in `spec-principles.md` §3.2). Cite the story file + scenario + leaked token. - [ ] **M2 — No source code in story prose.** FAIL if a scenario expresses behavior as a class/method/file-path/namespace (e.g. "the controller calls the handler") instead of a business operation (e.g. "the system records the request"). Source identifiers belong only in evidence carriers. Cite the story + line. - [ ] **M3 — Abstract-IDs-first traceability.** FAIL if a story or its acceptance criteria are not anchored to a logical ID (PBI AC ID, `FR-/BR-/OP-`), OR the `[Source:]` evidence is a physical `file:line`/`src/` path instead of a stack-portable abstract anchor (`[Source: namespace/service/id]`), OR the anchor is treated as the primary citation. `[Source: namespace/service/id]` abstract-anchor evidence is REQUIRED and KEPT — it must be SECONDARY to the logical ID, never the spine and never removed; physical coordinates live only in the provenance sidecar. - [ ] **M4 — Unambiguous acceptance criteria.** FAIL if a THEN clause / AC uses vague language ("handle appropriately", "process normally", "works", "as needed"), OR two engineers could implement it differently while both claiming conformance, OR no observable completion state / specific error condition (code + message) is named. (Reinforces the existing strawman-AC and testability techniques above.) - [ ] **M5 — Implementable without reading source.** FAIL if a competent team with ZERO codebase knowledge could not implement the story's behavior on a different stack from the story + its referenced PBI/spec alone. Cite the story + the missing detail. If ANY box fails → verdict is FAIL; list each violated mandate ID with its concrete story/line citation in the Verdict section. ## Output ```markdown ## Story Review Result **Status:** PASS | WARN | FAIL **Stories reviewed:** {count} **Source PBI:** {pbi-path} ### AC Coverage Matrix | Acceptance Criterion | Covered By Story | Status | | -------------------- | ---------------- | ------ | ### Required ({X}/{Y}) - ✅/❌ Check description ### Recommended ({X}/{Y}) - ✅/⚠️ Check description ### Missing Stories - {Any PBI AC not covered} ### Dependency Issues - {Circular deps, missing ordering} ### Verdict {PROCEED | REVISE_FIRST} ``` ## Round 2+ : Fresh Sub-Agent Re-Review (MANDATORY) > **Protocol:** `SYNC:double-round-trip-review` + `SYNC:fresh-context-review` + `SYNC:review-protocol-injection` (all inlined above in this file). After completing Round 1 checklist evaluation, spawn a **fresh `general-purpose` sub-agent** for Round 2 using the canonical Agent template from `SYNC:review-protocol-injection` above. Story artifact reviews are NOT code reviews — use `subagent_type: "general-purpose"`. When constructing the Agent call prompt: 1. Copy the Agent call shape from the `SYNC:review-protocol-injection` template verbatim 2. Set `subagent_type: "general-purpose"` 3. Embed the full verbatim body of these SYNC blocks: `SYNC:evidence-based-reasoning`, `SYNC:rationalization-prevention`, `SYNC:understand-code-first` (omit code-specific protocols like `SYNC:bug-detection`, `SYNC:design-patterns-quality`, `SYNC:fix-layer-accountability` which are not applicable to story artifacts) 4. Set the Task as `"Review the user story artifacts for completeness and quality. Focus on: implicit assumptions not validated, missing acceptance criteria coverage, edge cases not addressed in BDD scenarios, cross-references not verified, vague language, authorization gaps, INVEST violations."` 5. Set Target Files as the explicit story file paths being reviewed 6. Set report path as `plans/reports/story-review-round{N}-{date}.md` After sub-agent returns: 1. **Read** the sub-agent's report 2. **Integrate** findings as `## Round {N} Findings (Fresh Sub-Agent)` in the main report — DO NOT filter or override 3. **If FAIL:** fix issues in the stories, then spawn a NEW Round N+1 fresh sub-agent (new Agent call — never reuse Round 2's agent) 4. **Max 3 fresh rounds** — escalate to user via `AskUserQuestion` if still failing after 3 rounds 5. **Final verdict** must incorporate findings from ALL rounds ## Key Rules - **FAIL blocks workflow** — If FAIL, do NOT proceed. List specific fixes. - **Cross-reference PBI** — Every check against stories MUST ATTENTION trace back to PBI acceptance criteria. - **No guessing** — Reference specific story content as evidence. - **Flag missing stories** — If a PBI acceptance criterion has no covering story, that's a FAIL. --- ## Next Steps **MANDATORY IMPORTANT MUST ATTENTION — NO EXCEPTIONS** after completing this skill, you MUST ATTENTION use `AskUserQuestion` to present these options. Do NOT skip because the task seems "simple" or "obvious" — the user decides: - **"/plan (Recommended)"** — Create implementation plan from validated stories - **"/story"** — Re-create stories if FAIL verdict - **"/prioritize"** — Prioritize stories in backlog - **"Skip, continue manually"** — user decides > **[IMPORTANT]** Use `TaskCreate` to break ALL work into small tasks BEFORE starting — including tasks for each file read. This prevents context loss from long files. For simple tasks, AI MUST ATTENTION ask user whether to skip. > **Evidence Gate:** MANDATORY IMPORTANT MUST ATTENTION — 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 IMPORTANT MUST ATTENTION — flag duplicated patterns that should be extracted to a base class, generic, or helper. Classes in the same group or suffix (ex *Entity, *Dto, \*Service, etc...) 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. > **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. > **AI Mistake Prevention** — Failure modes to avoid on every task: > > **Check downstream references before deleting.** Deleting components causes documentation and code staleness cascades. Map all referencing files before removal. > **Verify AI-generated content against actual code.** AI hallucinates APIs, class names, and method signatures. Always grep to confirm existence before documenting or referencing. > **Trace full dependency chain after edits.** Changing a definition misses downstream variables and consumers derived from it. Always trace the full chain. > **Trace ALL code paths when verifying correctness.** Confirming code exists is not confirming it executes. Always trace early exits, error branches, and conditional skips — not just happy path. > **When debugging, ask "whose responsibility?" before fixing.** Trace whether bug is in caller (wrong data) or callee (wrong handling). Fix at responsible layer — never patch symptom site. > **Assume existing values are intentional — ask WHY before changing.** Before changing any constant, limit, flag, or pattern: read comments, check git blame, examine surrounding code. > **Verify ALL affected outputs, not just the first.** Changes touching multiple stacks require verifying EVERY output. One green check is not all green checks. > **Holistic-first debugging — resist nearest-attention trap.** When investigating any failure, list EVERY precondition first (config, env vars, DB names, endpoints, DI registrations, data preconditions), then verify each against evidence before forming any code-layer hypothesis. > **Surgical changes — apply the diff test.** Bug fix: every changed line must trace directly to the bug. Don't restyle or improve adjacent code. Enhancement task: implement improvements AND announce them explicitly. > **Surface ambiguity before coding — don't pick silently.** If request has multiple interpretations, present each with effort estimate and ask. Never assume all-records, file-based, or more complex path. > **Estimation Framework** — Bottom-up first; SP DERIVED; output min-max range when likely ≥3d. Stack-agnostic. Baseline: 3-5yr dev, 6 productive hrs/day. AI estimate assumes Claude Code + project context. > > **Method:** > > 1. **Blast Radius pass** (below) — drives code AND test cost > 2. Decompose phases → hours/phase → `bottom_up_hours = Σ phase_hours` > 3. `likely_days = ceil(bottom_up_hours / 6) × productivity_factor` > 4. Sum **Risk Margin** (base + add-ons) → `max_days = likely_days × (1 + margin)` > 5. `min_days = likely_days × 0.9` > 6. Output as range when `likely_days ≥3`; single point allowed `<3` (still record margin) > 7. `man_days_ai` = same range × AI speedup > 8. `story_points` DERIVED from `likely_days` via SP-Days — NEVER driver. Disagreement >50% → trust bottom-up > > **Productivity factor:** 0.8 strong scaffolding+codegen+AI hooks · 1.0 mature default · 1.2 weak patterns · 1.5 greenfield > > **Cost Driver Heuristic (apply BEFORE work-type row):** > > - **UI dominates** in CRUD/business apps — 1.5-3x backend (states, validation, responsive, a11y, polish) > - **Backend dominates ONLY:** multi-aggregate invariants, cross-service contracts, schema migrations, heavy query/perf, new event flows > > **Reuse-vs-Create axis (PRIMARY lever, per layer):** > > | UI tier | Cost | > | -------------------------------------------- | -------- | > | Reuse component on existing screen | 0.1-0.3d | > | Add control/column to existing screen | 0.3-0.8d | > | Compose components into NEW screen | 1-2d | > | NEW screen, custom layout/states/validation | 2-4d | > | NEW shared/common component (themed, tested) | 3-6d+ | > > | Backend tier | Cost | > | ---------------------------------------------------- | --------- | > | Reuse query/handler from new place | 0.1-0.3d | > | Small update existing handler/entity | 0.3-0.8d | > | NEW query on existing repo/model | 0.5-1d | > | NEW command/handler on existing aggregate (additive) | 1-2d | > | NEW aggregate/entity (repo, validation, events) | 2-4d | > | NEW cross-service contract OR schema migration | 2-4d each | > | Multi-aggregate invariant / heavy domain rule | 3-5d | > > **Rule:** Sum tiers across UI+backend+tests, apply productivity factor. Reuse short-circuits tiers — call out. > > **Test-Scope drivers (compute test_count EXPLICITLY — "+tests" hand-wave is #1 failure):** > > | Driver | Count | > | --------------------------------- | ------------------------------------------------------ | > | Happy-path journeys | 1 per story / AC main flow | > | State-machine transitions | reachable transitions × allowed actors | > | Multi-entity state combos | state(A) × state(B) — REACHABLE only, not Cartesian | > | Authorization matrix | (owner, non-owner, elevated, unauth) × each mutation | > | Validation rules | 1 per required field / boundary / format / cross-field | > | UI states (per new screen/dialog) | happy, loading, empty, error, partial — present only | > | Negative paths / invariants | 1 per violatable business rule | > > | Test tier (Trad, incl. setup+assert+flake) | Cost | > | ------------------------------------------ | -------- | > | 1-5 cases, fixtures reused | 0.3-0.5d | > | 6-12 cases, 1 new fixture | 0.5-1d | > | 13-25 cases, multi-entity setup | 1-2d | > | 26-50 cases OR new state-machine coverage | 2-3d | > | >50 cases OR full E2E journey | 3-5d | > > **Test multipliers:** new fixture/seed harness +0.5d · cross-service/bus assertion +0.3d each · UI E2E ×1.5 · each new role +1-2 cases > > **Blast Radius (mandatory pre-pass — affects code AND test):** > > 1. Files/components directly modified — count > 2. Of those, "complex" (>500 LOC, multi-handler, central, frequently-modified) — count > 3. Downstream consumers (callers, event subscribers, cross-service) — list > 4. Shared/common code touched (multi-app blast) — yes/no > 5. Regression scope — areas needing re-test > > **Rule:** Complex touch → add `risk_factors`. Each downstream consumer → +1-3 regression cases. Blast >5 areas OR >2 complex → re-evaluate SPLIT before estimating. > > **Risk Margin (drives max bound):** > > | likely_days | Base margin | > | ------------------- | ------------------------------- | > | <1d trivial | +10% | > | 1-2d small additive | +20% | > | 3-4d real feature | +35% | > | 5-7d large | +50% | > | 8-10d very large | +75% | > | >10d | +100% AND **flag SHOULD SPLIT** | > > **Risk-factor add-ons (additive — enumerate in `risk_factors`):** > > | Factor | +margin | > | --------------------------------------------------------------------- | ------- | > | `touches-complex-existing-feature` (>500 LOC, multi-handler, central) | +20% | > | `cross-service-contract` change | +25% | > | `schema-migration-on-populated-data` | +25% | > | `new-tech-or-unfamiliar-pattern` | +30% | > | `regression-fan-out` (≥3 downstream areas re-test) | +20% | > | `performance-or-latency-critical` | +20% | > | `concurrency-race-event-ordering` | +25% | > | `shared-common-code` (multi-consumer/multi-app) | +25% | > | `unclear-requirements-or-design` | +30% | > > **Collapse rule:** total margin >100% → STOP, split (padding past 2x is dishonesty). Margin <15% on `likely_days ≥5` → under-estimated, widen. > > **Work-Type Caps (hard ceilings on `likely_days`):** > | Work type | Max SP | Max likely | > | --- | --- | --- | > | Single field / config flag / style fix | 1 | 0.5d | > | Add property to existing model + bind to existing UI | 2 | 1d | > | **Additive endpoint + minor UI control** (button/menu/column), reuses fixtures | **3** | **2-3d** | > | Additive endpoint + **NEW UI surface** OR additive multi-layer + new domain rule + 2+ test files | 5 | 3-5d | > | NEW model/aggregate OR migration OR cross-module contract OR heavy test (>1.5d) OR NEW UI + non-trivial backend | 8 | 5-7d | > | NEW UI surface + (NEW aggregate OR migration OR cross-service contract) | 13 | SHOULD split | > | Cross-service contract + migration combined | 13 | SHOULD split | > | Beyond | 21 | MUST split | > > **SP→Days (validation only):** 1=0.5d/0.25d · 2=1d/0.35d · 3=2d/0.65d · 5=4d/1.0d · 8=6d/1.5d · 13=10d/2.0d (Trad/AI likely) > **AI speedup:** SP 1≈2x · 2-3≈3x · 5-8≈4x · 13+≈5x. AI cost = `(code_gen × 1.3) + (test_gen × 1.3)` (30% review overhead). > > **MANDATORY frontmatter:** > > ```yaml > story_points: > complexity: low | medium | high | critical > man_days_traditional: '-d' # range when likely ≥3d; 'd' when <3d > man_days_ai: '-d' > risk_margin_pct: # base + add-ons > risk_factors: [touches-complex-existing-feature, regression-fan-out] # closed-list from add-ons; [] if none > blast_radius: > touched_areas: > complex_touched: > downstream_consumers: [list or count] > shared_common_code: yes | no > estimate_scope_included: [code, integration-tests, frontend, i18n, docs] > estimate_scope_excluded: [unit-tests, e2e, perf, deployment, code-review-rounds] > estimate_reasoning: | > 5-7 lines covering: > (a) UI tier — row applied > (b) Backend tier — row applied > (c) Test scope — case breakdown by driver, file count, fixtures, tier row > (d) Cost driver — dominant tier + why > (e) Blast radius — touched, complex, regression scope > (f) Risk factors — list driving margin; why not larger/smaller > Example: "UI: compose Form/Table/Dialog → NEW screen (~1.5d). Backend: NEW command on existing aggregate, > reuses validation+repo (~1d). Tests: 4 transitions × 2 actors + 3 validation + 2 UI states = 13 cases, > 1 new fixture → tier 13-25 ~1.5d. Driver: UI composition + new states. Blast: 4 areas, 1 complex. > Risk: base 35% + touches-complex +20% = 55% → max 3.9d → range 2.5-4d." > ``` > > **Sanity self-check:** > > - `likely_days ≥3d` and single-point? → reject, must be range > - Margin <15% on `likely_days ≥5d`? → under-estimated, widen > - Margin >100%? → STOP, split instead of buffer > - Complex existing feature touched, no regression budget in `(c)`? → reject > - Blast `>5` areas OR `>2` complex, no split discussion? → reject > - Purely additive on existing model AND existing UI? → cap SP 3 unless tests >1.5d > - NEW UI surface (page/complex form/dashboard)? → SP 5+ even if backend one endpoint > - Backend cross-service / migration / multi-aggregate? → SP 8+ regardless of UI > - `bottom_up_hours / 6` vs SP-Days disagreement >50%? → trust bottom-up, downgrade SP > - Without tests, SP drops ≥1 bucket? → tests dominate; state explicitly > - Reasoning called out UI vs backend vs blast vs risk factors? → if missing, add > **UI System Context** — For ANY task touching `.ts`, `.html`, `.scss`, or `.css` files: > > **MUST ATTENTION READ before implementing:** > > 1. `docs/project-reference/frontend-patterns-reference.md` — component base classes, stores, forms > 2. `docs/project-reference/scss-styling-guide.md` — BEM methodology, SCSS variables, mixins, responsive > 3. `docs/project-reference/design-system/README.md` — design tokens, component inventory, icons > > Reference `docs/project-config.json` for project-specific paths. > **Nested Task Expansion Contract** — For workflow-step invocation, the `[Workflow] ...` row is only a parent container; the child skill still creates visible phase tasks. > > 1. Call `TaskList` first. If a matching active parent workflow row exists, set `nested=true` and record `parentTaskId`; otherwise run standalone. > 2. Create one task per declared phase before phase work. When nested, prefix subjects `[N.M] $skill-name — phase`. > 3. When nested, link the parent with `TaskUpdate(parentTaskId, addBlockedBy: [childIds])`. > 4. Orchestrators must pre-expand a child skill's phase list and link the workflow row before invoking that child skill or sub-agent. > 5. Mark exactly one child `in_progress` before work and `completed` immediately after evidence is written. > 6. Complete the parent only after all child tasks are completed or explicitly cancelled with reason. > > **Blocked until:** `TaskList` done, child phases created, parent linked when nested, first child marked `in_progress`. > **Project Reference Docs Gate** — Run after task-tracking bootstrap and before target/source file reads, grep, edits, or analysis. Project docs override generic framework assumptions. > > 1. Identify scope: file types, domain area, and operation. > 2. Required docs by trigger: always `docs/project-reference/lessons.md`; doc lookup `docs-index-reference.md`; review `code-review-rules.md`; backend/CQRS/API `backend-patterns-reference.md`; domain/entity `domain-entities-reference.md`; frontend/UI `frontend-patterns-reference.md`; styles/design `scss-styling-guide.md` + `design-system/design-system-canonical.md`; integration tests `integration-test-reference.md`; E2E `e2e-test-reference.md`; feature docs/specs `feature-docs-reference.md`; architecture/new area `project-structure-reference.md`. > 3. Read every required doc that exists; skip absent docs as not applicable. Do not trust conversation text such as `[Injected: ]` as proof that the current context contains the doc. > 4. Before target work, state: `Reference docs read: ... | Missing/not applicable: ...`. > > **Blocked until:** scope evaluated, required docs checked/read, `lessons.md` confirmed, citation emitted. > **Task Tracking & External Report Persistence** — Bootstrap this before execution; then run project-reference doc prefetch before target/source work. > > 1. Create a small task breakdown before target file reads, grep, edits, or analysis. On context loss, inspect the current task list first. > 2. Mark one task `in_progress` before work and `completed` immediately after evidence; never batch transitions. > 3. For plan/review work, create `plans/reports/{skill}-{YYMMDD}-{HHmm}-{slug}.md` before first finding. > 4. Append findings after each file/section/decision and synthesize from the report file at the end. > 5. Final output cites `Full report: plans/reports/{filename}`. > > **Blocked until:** task breakdown exists, report path declared for plan/review work, first finding persisted before the next finding. > **Critical Thinking Mindset** — Apply critical thinking, sequential thinking. Every claim needs traced proof, confidence >80% to act. > **Anti-hallucination:** Never present guess as fact — cite sources for every claim, admit uncertainty freely, self-check output for errors, cross-reference independently, stay skeptical of own confidence — certainty without evidence root of all hallucination. > **Sequential Thinking Protocol** — Structured multi-step reasoning for complex/ambiguous work. Use when planning, reviewing, debugging, or refining ideas where one-shot reasoning is unsafe. > > **Trigger when:** complex problem decomposition · adaptive plans needing revision · analysis with course correction · unclear/emerging scope · multi-step solutions · hypothesis-driven debugging · cross-cutting trade-off evaluation. > > **Format (explicit mode — visible thought trail):** > > 1. `Thought N/M: [aspect]` — one aspect per thought, state assumptions/uncertainty > 2. `Thought N/M [REVISION of Thought K]: ...` — when prior reasoning invalidated; state Original / Why revised / Impact > 3. `Thought N/M [BRANCH A from Thought K]: ...` — explore alternative; converge with decision rationale > 4. `Thought N/M [HYPOTHESIS]: ...` then `[VERIFICATION]: ...` — test before acting > 5. `Thought N/N [FINAL]` — only when verified, all critical aspects addressed, confidence >80% > > **Mandatory closers:** Confidence % stated · Assumptions listed · Open questions surfaced · Next action concrete. > > **Stop conditions:** confidence <80% on any critical decision → escalate via AskUserQuestion · ≥3 revisions on same thought → re-frame the problem · branch count >3 → split into sub-task. > > **Implicit mode:** apply methodology internally without visible markers when adding markers would clutter the response (routine work where reasoning aids accuracy). > > **Deep-dive:** see `/sequential-thinking` skill (`.claude/skills/sequential-thinking/SKILL.md`) for worked examples (api-design, debug, architecture), advanced techniques (spiral refinement, hypothesis testing, convergence), and meta-strategies (uncertainty handling, revision cascades). > **Fix-Triggered Re-Review Loop** — Re-review is triggered by a FIX CYCLE, not by a round number. Review purpose: `review → if issues → fix → re-review` until a round finds no issues. **A clean review ENDS the loop — no further rounds required.** > > **Round 1:** Main-session review. Read target files, build understanding, note issues. Output findings + verdict (PASS / FAIL). > > **Decision after Round 1:** > > - **No issues found (PASS, zero findings)** → review ENDS. Do NOT spawn a fresh sub-agent for confirmation. > - **Issues found (FAIL, or any non-zero findings)** → fix the issues, then spawn a fresh sub-agent for Round 2 re-review. > > **Fresh sub-agent re-review (after every fix cycle):** Spawn a NEW `Agent` tool call — never reuse a prior agent. Sub-agent re-reads ALL files from scratch with ZERO memory of prior rounds. See `SYNC:fresh-context-review` for the spawn mechanism and `SYNC:review-protocol-injection` for the canonical Agent prompt template. Each fresh round must catch: > > - Cross-cutting concerns missed in the prior round > - Interaction bugs between changed files > - Convention drift (new code vs existing patterns) > - Missing pieces that should exist but don't > - Subtle edge cases the prior round rationalized away > - Regressions introduced by the fixes themselves > > **Loop termination:** After each fresh round, repeat the same decision: clean → END; issues → fix → next fresh round. Continue until a round finds zero issues, or **3 fresh-subagent rounds max**, then escalate to user via `AskUserQuestion`. > > **Rules:** > > - A clean Round 1 ENDS the review — no mandatory Round 2 > - NEVER skip the fresh sub-agent re-review after a fix cycle (every fix invalidates the prior verdict) > - NEVER reuse a sub-agent across rounds — every iteration spawns a NEW Agent call > - Main agent READS sub-agent reports but MUST NOT filter, reinterpret, or override findings > - Max 3 fresh-subagent rounds per review — if still FAIL, escalate via `AskUserQuestion` (do NOT silently loop) > - Track round count in conversation context (session-scoped) > - Final verdict must incorporate ALL rounds executed > > **Report must include `## Round N Findings (Fresh Sub-Agent)` for every round N≥2 that was executed.** > **Fresh Sub-Agent Review** — Eliminate orchestrator confirmation bias via isolated sub-agents. > > **Why:** The main agent knows what it (or `/cook`) just fixed and rationalizes findings accordingly. A fresh sub-agent has ZERO memory, re-reads from scratch, and catches what the main agent dismissed. Sub-agent bias is mitigated by (1) fresh context, (2) verbatim protocol injection, (3) main agent not filtering the report. > > **When:** ONLY after a fix cycle. A review round that finds zero issues ENDS the loop — do NOT spawn a confirmation sub-agent. A review round that finds issues triggers: fix → fresh sub-agent re-review. > > **How:** > > 1. Spawn a NEW `Agent` tool call — use `code-reviewer` subagent_type for code reviews, `general-purpose` for plan/doc/artifact reviews > 2. Inject ALL required review protocols VERBATIM into the prompt — see `SYNC:review-protocol-injection` for the full list and template. Never reference protocols by file path; AI compliance drops behind file-read indirection (see `SYNC:shared-protocol-duplication-policy`) > 3. Sub-agent re-reads ALL target files from scratch via its own tool calls — never pass file contents inline in the prompt > 4. Sub-agent writes structured report to `plans/reports/{review-type}-round{N}-{date}.md` > 5. Main agent reads the report, integrates findings into its own report, DOES NOT override or filter > > **Rules:** > > - SKIP fresh sub-agent when the prior round found zero issues (no fixes = nothing new to verify) > - NEVER skip fresh sub-agent after a fix cycle — every fix invalidates the prior verdict > - NEVER reuse a sub-agent across rounds — every fresh round spawns a NEW `Agent` call > - Max 3 fresh-subagent rounds per review — escalate via `AskUserQuestion` if still failing; do NOT silently loop or fall back to any prior protocol > - Track iteration count in conversation context (session-scoped, no persistent files) > **Review Protocol Injection** — Every fresh sub-agent review prompt MUST embed 10 protocol blocks VERBATIM. The template below has ALL 10 bodies already expanded inline. Copy the template wholesale into the Agent call's `prompt` field at runtime, replacing only the `{placeholders}` in Task / Round / Reference Docs / Target Files / Output sections with context-specific values. Do NOT touch the embedded protocol sections. > > **Why inline expansion:** Placeholder markers would force file-read indirection at runtime. AI compliance drops significantly behind indirection (see `SYNC:shared-protocol-duplication-policy`). Therefore the template carries all 10 protocol bodies pre-embedded. ### Subagent Type Selection - `code-reviewer` — for code reviews (reviewing source files, git diffs, implementation) - `general-purpose` — for plan / doc / artifact reviews (reviewing markdown plans, docs, specs) ### Canonical Agent Call Template (Copy Verbatim) ``` Agent({ description: "Fresh Round {N} review", subagent_type: "code-reviewer", prompt: ` ## Task {review-specific task — e.g., "Review all uncommitted changes for code quality" | "Review plan files under {plan-dir}" | "Review integration tests in {path}"} ## Round Round {N}. You have ZERO memory of prior rounds. Re-read all target files from scratch via your own tool calls. Do NOT trust anything from the main agent beyond this prompt. ## Protocols (follow VERBATIM — these are non-negotiable) ### Evidence-Based Reasoning Speculation is FORBIDDEN. Every claim needs proof. 1. Cite file:line, grep results, or framework docs for EVERY claim 2. Declare confidence: >80% act freely, 60-80% verify first, <60% DO NOT recommend 3. Cross-service validation required for architectural changes 4. "I don't have enough evidence" is valid and expected output BLOCKED until: Evidence file path (file:line) provided; Grep search performed; 3+ similar patterns found; Confidence level stated. Forbidden without proof: "obviously", "I think", "should be", "probably", "this is because". If incomplete → output: "Insufficient evidence. Verified: [...]. Not verified: [...]." ### Bug Detection MUST check categories 1-4 for EVERY review. Never skip. 1. Null Safety: Can params/returns be null? Are they guarded? Optional chaining gaps? .find() returns checked? 2. Boundary Conditions: Off-by-one (< vs <=)? Empty collections handled? Zero/negative values? Max limits? 3. Error Handling: Try-catch scope correct? Silent swallowed exceptions? Error types specific? Cleanup in finally? 4. Resource Management: Connections/streams closed? Subscriptions unsubscribed on destroy? Timers cleared? Memory bounded? 5. Concurrency (if async): Missing await? Race conditions on shared state? Stale closures? Retry storms? 6. Stack-Specific: JS: === vs ==, typeof null. C#: async void, missing using, LINQ deferred execution. Classify: CRITICAL (crash/corrupt) → FAIL | HIGH (incorrect behavior) → FAIL | MEDIUM (edge case) → WARN | LOW (defensive) → INFO. ### Design Patterns Quality Priority checks for every code change: 1. DRY via OOP: Same-suffix classes (*Entity, *Dto, *Service) MUST share base class. 3+ similar patterns → extract to shared abstraction. 2. Right Responsibility: Logic in LOWEST layer (Entity > Domain Service > Application Service > Controller). Never business logic in controllers. 3. SOLID: Single responsibility (one reason to change). Open-closed (extend, don't modify). Liskov (subtypes substitutable). Interface segregation (small interfaces). Dependency inversion (depend on abstractions). 4. After extraction/move/rename: Grep ENTIRE scope for dangling references. Zero tolerance. 5. YAGNI gate: NEVER recommend patterns unless 3+ occurrences exist. Don't extract for hypothetical future use. Anti-patterns to flag: God Object, Copy-Paste inheritance, Circular Dependency, Leaky Abstraction. ### Logic & Intention Review Verify WHAT code does matches WHY it was changed. 1. Change Intention Check: Every changed file MUST serve the stated purpose. Flag unrelated changes as scope creep. 2. Happy Path Trace: Walk through one complete success scenario through changed code. 3. Error Path Trace: Walk through one failure/edge case scenario through changed code. 4. Acceptance Mapping: If plan context available, map every acceptance criterion to a code change. 5. Tests Verify Intent: For test/spec changes, verify tests name the protected business rule or invariant and would fail if that intent breaks. 6. Migration Test Exclusion: Do not write tests for migration code. Schema/data migrations are one-time execution paths, not core application logic. NEVER mark review PASS without completing both traces (happy + error path). ### Test Spec Verification Map changed code to test specifications. 1. Identify the project's test/spec format from existing docs, test-case files, BDD feature files, or spec folders. 2. Every changed code path MUST map to a corresponding test case/spec (or flag as "needs test case"). 3. New functions/endpoints/handlers → flag for test spec creation. 4. Migration files are excluded from test/spec creation; schema/data migrations are one-time execution paths, not core application logic. 5. If spec evidence fields exist, verify they point to actual code (file:line, not stale references). 6. Verify each meaningful test case names the business intent/invariant; flag behavior-only cases that only mirror implementation details. 7. Auth/data changes → verify corresponding authorization and data-state test cases exist. 8. If no specs exist for a changed path → log the gap and recommend the project's test-spec workflow. NEVER skip test mapping. Untested code paths are the #1 source of production bugs. ### Behavioral Delta Matrix MANDATORY for any bugfix review. Produce input-state × pre-fix × post-fix × delta table BEFORE writing verdict. - Minimum 3 rows; include at least one row OUTSIDE the original bug report. - Any "REGRESSION" delta → review returns FAIL until a preservation test is added. - Narrative descriptions do NOT substitute for the matrix. Example rows (external-record sync fix): | Input | Pre-fix | Post-fix | Delta | | --------------------- | ------- | ------------------------- | ---------- | | Record exists (valid) | Reused | Always recreated → orphan | REGRESSION | | Record missing (404) | Error | Recreated | Fixed | ### Fix-Layer Accountability NEVER fix at the crash site. Trace the full flow, fix at the owning layer. The crash site is a SYMPTOM, not the cause. MANDATORY before ANY fix: 1. Trace full data flow — Map the complete path from data origin to crash site across ALL layers (storage → backend → API → frontend → UI). Identify where bad state ENTERS, not where it CRASHES. 2. Identify the invariant owner — Which layer's contract guarantees this value is valid? Fix at the LOWEST layer that owns the invariant, not the highest layer that consumes it. 3. One fix, maximum protection — If fix requires touching 3+ files with defensive checks, you are at the wrong layer — go lower. 4. Verify no bypass paths — Confirm all data flows through the fix point. Check for direct construction skipping factories, clone/spread without re-validation, raw data not wrapped in domain models, mutations outside the model layer. BLOCKED until: Full data flow traced (origin → crash); Invariant owner identified with file:line evidence; All access sites audited (grep count); Fix layer justified (lowest layer that protects most consumers). Anti-patterns (REJECT): "Fix it where it crashes" (crash site ≠ cause site, trace upstream); "Add defensive checks at every consumer" (scattered defense = wrong layer); "Both fix is safer" (pick ONE authoritative layer). ### Rationalization Prevention AI skips steps via these evasions. Recognize and reject: - "Too simple for a plan" → Simple + wrong assumptions = wasted time. Plan anyway. - "I'll test after" → RED before GREEN. Write/verify test first. - "Already searched" → Show grep evidence with file:line. No proof = no search. - "Just do it" → Still need TaskCreate. Skip depth, never skip tracking. - "Just a small fix" → Small fix in wrong location cascades. Verify file:line first. - "Code is self-explanatory" → Future readers need evidence trail. Document anyway. - "Combine steps to save time" → Combined steps dilute focus. Each step has distinct purpose. ### Graph-Assisted Investigation MANDATORY when .code-graph/graph.db exists. HARD-GATE: MUST run at least ONE graph command on key files before concluding any investigation. Pattern: Grep finds files → trace --direction both reveals full system flow → Grep verifies details. - Investigation/Scout: trace --direction both on 2-3 entry files - Fix/Debug: callers_of on buggy function + tests_for - Feature/Enhancement: connections on files to be modified - Code Review: tests_for on changed functions - Blast Radius: trace --direction downstream CLI: python .claude/scripts/code_graph {command} --json. Use --node-mode file first (10-30x less noise), then --node-mode function for detail. ### Understand Code First HARD-GATE: Do NOT write, plan, or fix until you READ existing code. 1. Search 3+ similar patterns (grep/glob) — cite file:line evidence. 2. Read existing files in target area — understand structure, base classes, conventions. 3. Run python .claude/scripts/code_graph trace --direction both --json when .code-graph/graph.db exists. 4. Map dependencies via connections or callers_of — know what depends on your target. 5. Write investigation to .ai/workspace/analysis/ for non-trivial tasks (3+ files). 6. Re-read analysis file before implementing — never work from memory alone. 7. NEVER invent new patterns when existing ones work — match exactly or document deviation. BLOCKED until: Read target files; Grep 3+ patterns; Graph trace (if graph.db exists); Assumptions verified with evidence. ## Reference Docs (READ before reviewing) - docs/project-reference/code-review-rules.md - {skill-specific reference docs — e.g., integration-test-reference.md for integration-test-review; backend-patterns-reference.md for backend reviews; frontend-patterns-reference.md for frontend reviews} ## Target Files {explicit file list OR "run git diff to see uncommitted changes" OR "read all files under {plan-dir}"} ## Output Write a structured report to plans/reports/{review-type}-round{N}-{date}.md with sections: - Status: PASS | FAIL - Issue Count: {number} - Critical Issues (with file:line evidence) - High Priority Issues (with file:line evidence) - Medium / Low Issues - Cross-cutting findings Return the report path and status to the main agent. Every finding MUST have file:line evidence. Speculation is forbidden. ` }) ``` ### Rules - DO copy the template wholesale — including all 10 embedded protocol sections - DO replace only the `{placeholders}` in Task / Round / Reference Docs / Target Files / Output sections with context-specific content - DO choose `code-reviewer` subagent_type for code reviews and `general-purpose` for plan / doc / artifact reviews - DO NOT paraphrase, summarize, or skip any protocol section - DO NOT pass file contents inline — the sub-agent reads via its own tool calls so it has a fresh context - DO NOT reference protocols by file path or tag name — the bodies are already embedded above - DO NOT introduce placeholder markers for the protocols — they must stay literally expanded > **Graph Impact Analysis** — When `.code-graph/graph.db` exists, run `blast-radius --json` to detect ALL files affected by changes (7 edge types: CALLS, MESSAGE_BUS, API_ENDPOINT, TRIGGERS_EVENT, PRODUCES_EVENT, TRIGGERS_COMMAND_EVENT, INHERITS). Compute gap: impacted_files - changed_files = potentially stale files. Risk: <5 Low, 5-20 Medium, >20 High. Use `trace --direction downstream` for deep chains on high-impact files. - **MANDATORY IMPORTANT MUST ATTENTION** execute the review loop: review → if issues → fix → fresh sub-agent re-review. A round that finds zero issues ENDS the review. **IMPORTANT MUST ATTENTION** run graph blast-radius on changed files to find potentially stale consumers/handlers (when graph.db exists). **IMPORTANT MUST ATTENTION** read frontend-patterns-reference, scss-styling-guide, design-system/README before any UI change. - **MANDATORY MUST ATTENTION** estimation: bottom-up phase hours drive `man_days_traditional` (`Σh/6 × productivity_factor`); SP DERIVED. UI cost usually dominates — bump SP one bucket if NEW UI surface (page/complex form/dashboard). Frontmatter MUST include `story_points`, `complexity`, `man_days_traditional`, `man_days_ai`, `estimate_scope_included`, `estimate_scope_excluded`, `estimate_reasoning` (UI vs backend cost driver). Cap SP 3 for additive-on-existing-model+existing-UI unless test scope >1.5d. SP 13 SHOULD split, SP 21 MUST split. **MUST ATTENTION** apply critical thinking — every claim needs traced proof, confidence >80% to act. Anti-hallucination: never present guess as fact. **MUST ATTENTION** apply sequential-thinking — multi-step Thought N/M, REVISION/BRANCH/HYPOTHESIS markers, confidence % closer; see `/sequential-thinking` skill. **MUST ATTENTION** apply AI mistake prevention — holistic-first debugging, fix at responsible layer, surface ambiguity before coding, re-read files after compaction. - **MANDATORY** Bootstrap task tracking before target work; transition one task at a time. - **MANDATORY** Persist plan/review findings to `plans/reports/` incrementally and synthesize from disk. - **MANDATORY** After task-tracking bootstrap and before target/source work, read required project-reference docs and cite `Reference docs read: ...`. - **MANDATORY** Always include `lessons.md`; project conventions override generic defaults. - **MANDATORY** Parent workflow rows do not replace child phase tracking; expand phases and link the parent when nested. - **MANDATORY** Orchestrators pre-expand child skill phases before invocation; use `[N.M] $skill-name — phase` prefixes and one-`in_progress` discipline. ## Prompt-Enhance Closing Anchors **IMPORTANT MUST ATTENTION** follow declared step order for this skill; NEVER skip, reorder, or merge steps without explicit user approval **IMPORTANT MUST ATTENTION** for every step/sub-skill call: set `in_progress` before execution, set `completed` after execution **IMPORTANT MUST ATTENTION** every skipped step MUST include explicit reason; every completed step MUST include concise evidence **IMPORTANT MUST ATTENTION** if Task tools unavailable, maintain an equivalent step-by-step plan tracker with synchronized statuses ## Closing Reminders **MANDATORY IMPORTANT MUST ATTENTION** break work into small todo tasks using `TaskCreate` BEFORE starting. **MANDATORY IMPORTANT MUST ATTENTION** validate decisions with user via `AskUserQuestion` — never auto-decide. **MANDATORY IMPORTANT MUST ATTENTION** add a final review todo task to verify work quality. **MANDATORY IMPORTANT MUST ATTENTION** READ the following files before starting: **[TASK-PLANNING]** Before acting, analyze task scope and systematically break it into small todo tasks and sub-tasks using TaskCreate. > **[IMPORTANT]** Analyze how big the task is and break it into many small todo tasks systematically before starting — this is very important. --- > **Closing reminder — Easy to Change is the success metric.** Every finding, > test, refactor, and abstraction must answer one question: _does this make > the next change cheaper or more expensive?_ If it doesn't reduce future > change cost, reject it. Coupling, hidden state, duplicated knowledge, and > unclear intent are the real enemies — call them out by name.