--- name: artifact-review description: '[Code Quality] Use when you need to review artifact quality (PBI, user story, test spec, design spec) before handoff. Supports --type={pbi|story|spec-tests|design}.' --- > Codex compatibility note: > > - Invoke repository skills with `$skill-name` in Codex; this mirrored copy rewrites legacy Claude `/skill-name` references. > - Task tracker mandate: BEFORE executing any workflow or skill step, create/update task tracking for all steps and keep it synchronized as progress changes. > - User-question prompts mean to ask the user directly in Codex. > - Ignore Claude-specific mode-switch instructions when they appear. > - Strict execution contract: when a user explicitly invokes a skill, execute that skill protocol as written. > - Subagent authorization: when a skill is user-invoked or AI-detected and its protocol requires subagents, that skill activation authorizes use of the required `spawn_agent` subagent(s) for that task. > - Do not skip, reorder, or merge protocol steps unless the user explicitly approves the deviation first. > - For workflow skills, execute each listed child-skill step explicitly and report step-by-step evidence. > - If a required step/tool cannot run in this environment, stop and ask the user before adapting. ## Codex Project-Reference Loading (No Hooks) Codex uses static project-reference loading instead of runtime-injected project docs. When coding, planning, debugging, testing, or reviewing, open project docs explicitly using this routing. **Always read:** - `docs/project-config.json` (project-specific paths, commands, modules, and workflow/test settings) - `docs/project-reference/docs-index-reference.md` (routes to the full `docs/project-reference/*` catalog) - `docs/project-reference/lessons.md` (always-on guardrails and anti-patterns) **Missing/stale context route:** If `docs/project-config.json`, the docs index, `lessons.md`, `CLAUDE.md`, `AGENTS.md`, or any task-required reference doc is missing or stale, auto-run `$project-init` or the narrow setup route (`$project-config`, `$docs-init`, `$scan-all`, `$scan --target=`, `$claude-md-init`) before ordinary project-specific work. If Codex mirrors or `AGENTS.md` are missing/stale, ask the user to run `$sync-codex`; do not auto-run it. **Situation-based docs:** - Project structure/architecture/tech-stack/deployment/setup (any layer — backend, frontend, or infra): `project-structure-reference.md` - Backend/CQRS/API/domain/entity changes: `backend-patterns-reference.md`, `domain-entities-reference.md` - Frontend/UI/styling/design-system: `frontend-patterns-reference.md`, `scss-styling-guide.md`, `design-system/README.md` - Spec authoring, `docs/specs/` pathing, or TC format: `feature-spec-reference.md`, `spec-system-reference.md`, `spec-principles.md` - Behavior/public-contract changes or spec-test-code sync: `workflow-spec-test-code-cycle-reference.md` plus the spec docs above - Derived spec indexes/ERDs/reimplementation guides: `spec-system-reference.md` and source Feature Specs under `docs/specs/` - Integration test implementation/review: `integration-test-reference.md` - E2E test implementation/review: `e2e-test-reference.md` - Code review/audit work: `code-review-rules.md` plus domain docs above based on changed files Do not read all docs blindly. Start from `docs-index-reference.md`, then open only relevant files for the task. > **[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:** Review an artifact (PBI, design spec, story, test spec) for completeness and quality so reviewed artifacts are complete, evidence-backed, and ready for handoff without missing assumptions or acceptance gaps. **Summary:** - **Purpose:** review ONE artifact (PBI · user story set · test spec · design spec) for completeness + quality so it ships evidence-backed and handoff-ready — no missing assumptions, no acceptance gaps. Default stance = SKEPTIC, not presence-checker: sections that exist but hold weak/untestable content are worse than missing ones — they breed false confidence. - **Main steps (in order):** (1) **Identify** type — dispatch on `--type={pbi|story|spec-tests|design}`, infer if omitted; (2) **Adversarial Mindset** — run ALL 6 techniques (steel-man rejected alternatives · stress-test 3 assumptions · AC-testability · pre-mortem · unseen alternatives · contrarian pass) + clear the Anti-Bias Gate before any verdict; (3) **Type checklist** — score Required + Recommended; (4) **M1-M7 gate** (BLOCKING, ALL types); (5) **Readability checklist**; (6) **Output** per-type template (verdict + Required/Recommended tallies + coverage/AC matrix); (7) **Validated-fix + full re-review loop** — validate findings → fix → restart full review until clean. - **Type dispatch + verdict:** each `--type` has its own Required/Recommended checklist and output template — verdict = PASS (all Required + ≥50% Recommended) | WARN (all Required, <50% Recommended) | FAIL (any Required fails). - **M1-M7 gate (ALL types, BLOCKING):** any **M1-M5 or M7** violation forces NEEDS WORK citing the mandate ID + exact section/line; exempt source identifiers inside evidence carriers (`[Source:]`, `**Evidence**`, `CoveredBy`, legacy `IntegrationTest`, frontmatter, mermaid) — flag tech leakage only in narrative/AC/scenario prose. — why: carriers are the correct home for class/path/test names; flagging them is a false finding. **M7 (business-visibility) is judged on each case's BODY via the demo test, NOT its prose** — a tech-free-sounding case about a consumer/sync/handler passes M1 and STILL fails M7. — why: M1 governs vocabulary, M7 governs subject matter; the gap between them is how business specs rot. - **Validated-fix loop:** before fixing, invoke `$why-review --validate-findings ` on the review report FIRST (validate-before-fix discipline, at parity with `$plan-review`) — NEVER edit the artifact to resolve findings before this gate returns CLEAN. Then fix only validated findings, do not confirm-in-place, restart the FULL review (fresh `general-purpose` sub-agent — artifacts are NOT code), loop until a clean pass — clean review ENDS the loop. **Workflow:** 1. **Identify** — What artifact type is being reviewed 2. **Checklist** — Apply type-specific quality criteria 3. **Verdict** — READY or NEEDS WORK with specific items **Key Rules:** - Use type-specific checklists - Every NEEDS WORK item must be actionable - Focus on completeness — never block on stylistic preferences **Be skeptical. Apply critical thinking, sequential thinking. Every claim needs traced proof, confidence percentages (Idea should be more than 80%).** ## 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, 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**. - Simpler design easy to change beats sophisticated design that isn't. Apply this lens **before** invoking any specific rule, pattern, or checklist below — if downstream rule would raise change cost, this principle wins. --- ## Adversarial Review Mindset (NON-NEGOTIABLE) **Default stance: SKEPTIC challenging artifact quality and completeness, not confirming presence of sections.** > **Presence-quality confusion trap:** Artifact with all required sections LOOKS complete. But sections that exist yet contain weak, ambiguous, or untestable content are worse than missing sections — they create false confidence. This section forces quality challenge beyond existence checks. ### Adversarial Techniques (apply ALL before concluding) **1. Steel-Man the Alternatives** Before accepting chosen approach in any design artifact: argue FOR strongest rejected alternative as vigorously as possible. Would a senior domain expert seriously consider it? If yes — artifact's dismissal needs stronger justification. **2. Assumption Stress Test** List the 3 biggest assumptions embedded in artifact. For each: "What if this is wrong?" An artifact that breaks when 2 of its 3 core assumptions fail is fragile. Flag unaddressed failure modes. **3. Acceptance Criteria Testability** For each acceptance criterion: "Can a QA engineer write a specific automated test for this — without asking clarifying questions?" If not — AC is ambiguous. Flag it. Vague ACs ("feature works correctly") are NOT acceptance criteria. **4. Pre-Mortem** Assume artifact is implemented exactly as written and feature fails in production within 3 months. Write the most plausible failure scenario. If you can't find one, look harder — every implementation has a failure mode. **5. Unseen Alternatives** Identify 1-2 approaches NOT mentioned in artifact. Genuinely not considered, or considered and excluded without documented reasoning? Missing alternatives without exclusion reasoning = incomplete analysis. **6. Contrarian Pass** Before writing any verdict, generate at least 2 sentences arguing the OPPOSITE conclusion. Then decide which argument is stronger based on evidence. ### Forbidden Patterns - **"Required sections present"** → Presence ≠ quality. What's IN them? - **"Acceptance criteria are defined"** → Are they TESTABLE? Name the automated test for each. - **"Scope is well-defined"** → What is explicitly OUT of scope? If nothing is out of scope, the scope is undefined. - **"Alternatives were considered"** → Were they real alternatives, or strawmen set up to lose? - **"Looks complete"** → What specific failure mode is NOT addressed? ### Anti-Bias Gate (MANDATORY before finalizing verdict) - [ ] Steel-manned at least one rejected alternative - [ ] Identified 3 hidden assumptions and stress-tested them - [ ] Verified each AC is unambiguously testable (can write automated test without clarification) - [ ] Ran pre-mortem (one concrete production failure scenario) - [ ] Identified at least 1 unexamined alternative (not in artifact) - [ ] Generated at least 2 sentences arguing the opposite verdict If any box is unchecked → adversarial review incomplete. Go back. ## Type-Specific Checklists (`--type` dispatch) Select the checklist + output template by artifact type. Pass `--type={pbi|story|spec-tests|design}` to force a type; if omitted, infer from the artifact (Phase 1 "Identify"). Each type scores **Required (all must pass)** + **Recommended (≥50% should pass)** → verdict **PASS** (all Required + ≥50% Recommended) | **WARN** (all Required, <50% Recommended) | **FAIL** (any Required fails). | `--type` | Artifact | Output template | | ------------ | -------------------- | ------------------------------------------- | | `pbi` | Product Backlog Item | PBI Review Result | | `story` | User story set | Story Review Result (+ AC Coverage Matrix) | | `spec-tests` | Test specification | Test Spec Review Result (+ Coverage Matrix) | | `design` | Design spec | Artifact Review | ### PBI Review (`--type=pbi`) | # | Check | Presence | Quality Depth | | --- | ---------------------------------------------------------------------------------------------------------- | --------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | 1 | **Problem statement is clear** — the problem being solved is described in concrete terms | Is a problem statement present? Is it 2+ sentences? | Is the problem scoped correctly? Could it be framed differently to lead to a different (simpler) solution? Are symptoms confused with root cause? | | 2 | **Acceptance criteria are testable and measurable** — each AC can be verified by a test | Are ACs present? Do they use measurable language? | Can a QA engineer write an automated test for EACH AC without clarification? Are they specific enough to catch regressions? Vague ACs ("feature works correctly") are not acceptance criteria. | | 3 | **Scope is well-defined (what's in and out)** — both in-scope and out-of-scope items are explicitly listed | Is an in/out scope list present? Does it have both sides? | Are out-of-scope items specific enough to prevent scope creep? Is anything ambiguously in/out? A scope that says nothing is out of scope is an undefined scope. | | 4 | **Dependencies are identified** — all external dependencies the PBI relies on are listed | Is a dependencies section present? Does it list items? | Are ALL dependencies listed (technical, data, service, team)? Are "can-parallel" items truly safe to parallelize, or do they share a shared resource? | | 5 | **Business value is articulated** — the why behind the PBI is stated in terms of user or business outcome | Is business value described? | Is the value quantified or just stated? Does it connect to a user outcome, not just a feature delivery? "Users can now do X" is better than "we implemented feature Y". | | 6 | **Priority is assigned** — the PBI has an explicit priority level | Is a priority level assigned? | Is priority justified with data (RICE/MoSCoW), or arbitrary? Is it consistent with other PBIs in the same sprint? A PBI that is "high priority" without justification is unranked. | ### User Story Review (`--type=story`) | # | Check | Presence | Quality Depth | | --- | ------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | | 1 | **Follows GIVEN/WHEN/THEN format** — the story uses the structured BDD format | Are all three parts (GIVEN, WHEN, THEN) present? | Are all 3 parts present AND meaningful? Or is GIVEN trivial ("Given a user exists")? A GIVEN that describes no precondition adds no value. | | 2 | **Is independent (not dependent on other stories)** — the story can be implemented without requiring another story first | Is independence stated or inferable? | Would descoping other stories prevent this story from being implemented? Implicit dependencies are as blocking as explicit ones. | | 3 | **Is estimable (team can size it)** — the team has enough information to assign story points | Is the story sized or estimable based on content? | Does the team have enough info to estimate? Is "can't estimate" a sign of missing AC? If it can't be sized, it's not ready for sprint. | | 4 | **Is small enough for one sprint** — the story fits within a single sprint's capacity | Is the story sized at ≤8 story points or scoped to one sprint? | Could this be split further? Stories >8SP should always be split. A story that "could fit" in a sprint but requires multiple sub-systems is likely too large. | | 5 | **Has acceptance criteria** — the story defines measurable conditions for completion | Are acceptance criteria present? | Are criteria testable? Would they catch a bug if the feature works in 9/10 cases? ACs that only describe the happy path are incomplete. | ### Design Spec Review (`--type=design`) | # | Check | Presence | Quality Depth | | --- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | 1 | **All component states covered (default, hover, active, disabled, error, loading)** — spec defines visual behavior for all interaction states | Are all 6 states defined? | Are edge-case states (error, loading) as fully designed as the default state, or sketched? An undesigned error state will be improvised in implementation. | | 2 | **Design tokens specified (colors, spacing, typography)** — specific token values are called out, not ad-hoc values | Are token references present instead of raw values? | Are tokens from the project's token system, or are new values introduced? New values outside the token system break design consistency silently.