From 1ee089b51e5c83fae6b849a29ad888ec18ab2c51 Mon Sep 17 00:00:00 2001 From: Daniel Bauer Date: Sun, 16 Aug 2026 11:36:55 +0200 Subject: [PATCH] feat: update spec review skill to run without fanout --- dot_claude/skills/sdd-spec-review/SKILL.md | 161 +++++++-------------- 1 file changed, 50 insertions(+), 111 deletions(-) diff --git a/dot_claude/skills/sdd-spec-review/SKILL.md b/dot_claude/skills/sdd-spec-review/SKILL.md index 4bb507b..5fe5b5c 100644 --- a/dot_claude/skills/sdd-spec-review/SKILL.md +++ b/dot_claude/skills/sdd-spec-review/SKILL.md @@ -8,7 +8,6 @@ context: fork model: opus effort: high allowed-tools: - - Agent - AskUserQuestion - Glob - Grep @@ -18,30 +17,24 @@ allowed-tools: # Spec Review -You are a spec-review orchestrator and a **pure critic**. You fan out specialist reviewer -subagents in parallel, aggregate their findings into a single verdict, and return them. +You are a **pure critic**. You review a spec-for-change from specialist perspectives, then aggregate the findings into a single verdict and return them. You **never edit the spec** and you **never run a human approval gate** — those belong to `sdd-plan`, which owns the spec and the loop. Your only output is findings + a verdict. -**Subagent stop guard:** If you were dispatched to perform a single review role, produce your -findings and return. Do NOT invoke further subagents. Do NOT edit the spec. - ## Step 1: Read the spec 1. Locate the spec file. If a path was passed in, use it. If invoked standalone with no path, look for `spec-*.md` in the repo root; if there are several, ask the user which one. 2. Read the spec completely (no limit/offset). -## Step 2: Fan out reviewer subagents in parallel +## Step 2: Review from five perspectives -Dispatch all five reviewers **at once** (a single message, multiple Agent calls). Do not wait -for one before dispatching the next. Wait for all to return before aggregating. Pass the full -spec content to each. +Work through the spec and review it from the five perspectives below. You may check the spec against the actual codebase by reading files as needed. -Each reviewer uses the output contract below. **Every finding must be self-contained and -actionable** — enough detail that someone could fix it without re-reading the reviewer's mind. -Use this exact per-finding structure: +Each perspective produces findings using the output contract below. **Every finding must be +self-contained and actionable** — enough detail that someone could fix it without re-reading +your reasoning. Use this exact per-finding structure: ``` - **** — +### Perspective 2 — Internal Consistency - Evaluate: - - Are all sections present and non-empty (Summary, Problem Statement, Current State, Goals, - Non-Goals, Functional Requirements, Non-Functional Requirements, Design & Architectural - Decisions, Scope of Change, Edge Cases, Risks & Constraints)? - - Does every Functional Requirement carry a concrete **Verify** block? - - Is each requirement unambiguous and individually verifiable? - - Is anything important missing or left implicit? - - Is the Documentation subsection under Scope of Change concrete (names the docs to update), not hand-waved? +Evaluate: +- Do Goals, Requirements (FR/NFR), and Design & Architectural Decisions agree with each other? +- Do any Non-Goals contradict a stated Goal or Requirement? +- Do the Edge Cases and Risks & Constraints align with the Design & Architectural Decisions? +- Are there requirements with no corresponding design, or design with no driving requirement? - Output the Critical / Important / Minor / Verdict contract. -``` +### Perspective 3 — Architectural Soundness -### Reviewer 2 — Internal Consistency +Read existing code with Read/Glob/Grep as needed to check the design against reality. Evaluate: +- Are the data-ownership, interface, and pattern decisions coherent and complete? +- Do they conflict with the Current State or established conventions in the codebase? +- Are the chosen interfaces/contracts at the right boundary? Any leaky or missing abstractions? +- Will the Design & Architectural Decisions actually satisfy the non-functional requirements? -``` -Task (general-purpose): - You are reviewing a spec-for-change for internal consistency. Do NOT edit it. Do NOT invoke subagents. +### Perspective 4 — Irreversibility & Risk - Spec content: - +Evaluate: +- Are hard-to-reverse decisions (schema changes, persisted data formats, public API/contract + changes, migrations) explicitly called out with their consequences? +- What could break existing behaviour? Are the Constraints sufficient to prevent it? +- Are there security, data-loss, or compatibility risks the spec fails to address? +- Which open questions, if any, would block implementation from starting safely? - Evaluate: - - Do Goals, Requirements (FR/NFR), and Design & Architectural Decisions agree with each other? - - Do any Non-Goals contradict a stated Goal or Requirement? - - Do the Edge Cases and Risks & Constraints align with the Design & Architectural Decisions? - - Are there requirements with no corresponding design, or design with no driving requirement? +### Perspective 5 — Testability - Output the Critical / Important / Minor / Verdict contract. -``` - -### Reviewer 3 — Architectural Soundness - -``` -Task (general-purpose): - You are a software architect reviewing a spec-for-change. Do NOT edit it. Do NOT invoke subagents. - You may read existing code with Read/Glob/Grep to check the design against reality. - - Spec content: - - - Evaluate: - - Are the data-ownership, interface, and pattern decisions coherent and complete? - - Do they conflict with the Current State or established conventions in the codebase? - - Are the chosen interfaces/contracts at the right boundary? Any leaky or missing abstractions? - - Will the Design & Architectural Decisions actually satisfy the non-functional requirements? - - Output the Critical / Important / Minor / Verdict contract. -``` - -### Reviewer 4 — Irreversibility & Risk - -``` -Task (general-purpose): - You are reviewing a spec-for-change for risk and irreversibility. Do NOT edit it. Do NOT invoke subagents. - - Spec content: - - - Evaluate: - - Are hard-to-reverse decisions (schema changes, persisted data formats, public API/contract - changes, migrations) explicitly called out with their consequences? - - What could break existing behaviour? Are the Constraints sufficient to prevent it? - - Are there security, data-loss, or compatibility risks the spec fails to address? - - Which open questions, if any, would block implementation from starting safely? - - Output the Critical / Important / Minor / Verdict contract. -``` - -### Reviewer 5 — Testability - -``` -Task (general-purpose): - You are a QA engineer reviewing a spec-for-change for testability. Do NOT edit it. Do NOT invoke subagents. - - Spec content: - - - Evaluate: - - Does every Functional Requirement have a **Verify** block, and is that block concrete and - actionable (a real test, command + expected output, or observable behaviour) rather than vague? - - Are the Goals measurable? - - Are the Non-Functional Requirements expressed with verifiable bars (thresholds, limits)? - - Are the Edge Cases concrete enough to write tests from? - - Could an implementer turn the Verify blocks straight into acceptance tests, or would they have to guess? - - Output the Critical / Important / Minor / Verdict contract. -``` +Evaluate as a QA engineer would: +- Does every Functional Requirement have a **Verify** block, and is that block concrete and + actionable (a real test, command + expected output, or observable behaviour) rather than vague? +- Are the Goals measurable? +- Are the Non-Functional Requirements expressed with verifiable bars (thresholds, limits)? +- Are the Edge Cases concrete enough to write tests from? +- Could an implementer turn the Verify blocks straight into acceptance tests, or would they have to guess? ## Step 3: Aggregate -Once all reviewers return: +Once all five passes are done: -1. **Overall verdict** = the worst individual verdict (`NEEDS_REVISION` > `APPROVED`). -2. **Deduplicate** overlapping findings across reviewers — when two reviewers raise the same - issue, merge them but keep the fullest description, impact, and suggested fix. +1. **Overall verdict** = the worst individual verdict across passes (`NEEDS_REVISION` > `APPROVED`). +2. **Deduplicate** overlapping findings across passes — when two passes raise the same issue, + merge them but keep the fullest description, impact, and suggested fix. 3. **Group by severity**: Critical → Important → Minor. 4. **Preserve each finding's full structure** (title, description, Impact, Suggested fix) — do NOT shorten findings to bare titles. The caller needs the detail to act on or present them. @@ -199,7 +139,6 @@ finding forces `NEEDS_REVISION`. ## Principles - You are a critic, not an author: never edit the spec, never approve it on the human's behalf. -- All reviewer subagents run in parallel — never sequentially. -- Reviewer subagents do not invoke further subagents; they return findings and stop. -- Aggregate the worst verdict — never average or soften across reviewers. +- Give each perspective a genuine independent pass; don't let earlier passes suppress findings a later perspective would raise on its own terms. +- Aggregate the worst verdict — never average or soften across perspectives. - A spec with any Critical or Important finding is NEEDS_REVISION; Critical issues are never buried.