Review — Specification Quality & Consistency Review

Systematically review spec-forge generated documents for quality, completeness, and internal consistency. Optionally auto-fix issues found.

Core Principles

Evidence-based: Every finding must cite a specific file and section — no vague complaints
Spec-focused: Review specification documents only (SRS, tech-design, feature specs, overview) — not code, not upstream idea drafts
Upstream-aware: Use idea drafts and project manifests as reference context to validate spec accuracy, but do not review them
Prioritized: Findings classified by severity so the user can act on what matters first
Conservative fixes: Auto-fix only touches cited sections; when domain knowledge is missing, leave a  comment instead of guessing
Honest: Report real issues, don't inflate findings to look thorough

Severity Levels

Severity	Meaning	Example
Critical	Wrong, contradictory, or misleading content	Feature spec API signature contradicts tech-design, component boundary mismatch
Major	Significant gap that would block or confuse implementation	Empty required section, missing error handling spec, undefined edge cases
Minor	Quality issue that degrades usefulness but isn't blocking	Vague description, missing cross-reference, inconsistent terminology

Workflow

Step 1: Determine Review Scope

Parse the arguments to determine what to review:

If a feature_name argument is provided, look for:
- docs/{feature_name}/srs.md — the delivery contract; review it first, since a defect here propagates into every downstream document
- docs/{feature_name}/tech-design.md
- All feature specs in docs/features/ (glob for docs/features/*.md)
- Upstream reference: docs/{feature_name}/prd.md and ideas/{feature_name}/draft.md (if they exist)
- Project manifest: docs/project-{feature_name}.md (if exists, for multi-split context)
If no argument, scan docs/ for the most recent SRS and tech-design, plus all feature specs in docs/features/
If no spec documents found, inform the user and stop

Use AskUserQuestion to ask:

Review scope: Review all generated specs, or focus on specific documents? (Options: All / SRS only / Tech design only / Feature specs only / Specific files)
Auto-fix: Should I auto-fix issues found? (Options: Yes — fix Critical+Major automatically / Yes — fix all / No — report only)

Step 2: Document Inventory

Build the list of documents to review:

Review targets (will be reviewed):
- docs/{feature_name}/srs.md
- docs/{feature_name}/tech-design.md
- docs/features/overview.md
- docs/features/{component-1}.md, docs/features/{component-2}.md, etc.
- For multi-split: tech-designs for all sub-features
Reference context (read for context, NOT reviewed):
- docs/{feature_name}/prd.md — upstream business case and capability priorities
- ideas/{feature_name}/draft.md — upstream demand validation
- docs/project-{feature_name}.md — project manifest with sub-feature scope
Read each review target document fully

Display the inventory:

Review scope: {feature_name}
  Review targets: {N} documents
    - docs/{feature_name}/tech-design.md
    - docs/features/overview.md
    - docs/features/{component-1}.md
    - docs/features/{component-2}.md
    ...
  Reference context: {N} documents
    - ideas/{feature_name}/draft.md

Step 3: Review

Check each review target document against the following checklist:

3.1 Completeness

Fast path (preferred): resolve <sf_scripts> (see @../shared/scripts.md) and run the structural gate per document —

python3 "<sf_scripts>/sf-verify-doc.py" "<file>" --strict

for each review target. This deterministically confirms the document is non-empty and titled, has the recommended sections for its type, has well-formed/unique IDs, and carries no leftover template placeholders or {placeholder} variables; use its findings as the completeness result and fix everything it flags. The script does structure, IDs, and placeholders — you still do the semantic checks (§3.3 specificity, API-signature-matching in §3.2). Fallback: if python3 is unavailable or the script is not found, check by hand:

Any empty sections, TBD/TODO markers, placeholder text, or {placeholder} template variables?
Missing required sections per document type?
- SRS: Introduction, Scope Boundaries, Overall Description, Functional Requirements, State Machines, Permission Matrix, Error Catalogue, Non-Functional Requirements, Data Requirements, External Interfaces, Traceability Matrix, Acceptance and Change Control
- Tech design: Goals, Non-goals, Scope, Architecture, API Design, Data Model, Component Overview
- Feature spec: Purpose, API/Interface, Logic/Behavior, Error Handling, Dependencies
- Overview: Feature index listing all generated specs, dependency graph

3.1a SRS Contract Integrity

The SRS is the delivery contract — what an implementation team is held to and what acceptance is settled against. A gap here is not a documentation blemish; it is an unsettleable dispute waiting to happen, and it propagates into every downstream document. Review it first and apply these checks in addition to §3.1:

Out-of-scope table populated (§3.2). An empty table is a Major finding: it does not mean nothing is excluded, it means the exclusions are unwritten, and unwritten exclusions get built or get disputed. "Nothing is excluded" stated explicitly with a justification passes.
Input field rules present and exact. Every requirement that accepts input has the table, and every rule states an exact value rather than a category — "reasonable length", "a valid value", "an appropriate size" are Major findings. Every rejection behavior names a code that exists in the Error Catalogue.
State machines complete. Every entity with a lifecycle has both the legal-transition table and the illegal-transition handling table. A state machine with only legal transitions is Major — the implementer will invent the rest and acceptance testing will discover it.
Permission matrix with denial behavior. Roles × operations × data scope, scopes defined as concrete predicates, and the 403-vs-404 decision stated. A matrix that omits the denial behavior is Minor; one that omits data scope is Major, since "can read" without a scope is ambiguous about whose records.
No generic error entries. INTERNAL_ERROR or "operation failed" standing in for an anticipated condition is Major. Each distinct cause needs its own diagnosable code.
Degradation specified per dependency. For each external dependency, the behavior when it is unavailable is stated. Missing degradation behavior is Major — it is the failure mode that reaches production untested.
Acceptance criteria objectively settleable. A criterion whose pass/fail depends on reviewer judgement ("the interface should feel responsive") is Major: it is a defect in the specification, not a matter to resolve at acceptance time.
NFRs carry a verification method. A target with no agreed way to measure it cannot be accepted or disputed. Missing verification method is Major.
Requirement ID stability. If a prior version of this SRS exists in git history, confirm no issued FR-*/NFR-* ID was silently renumbered or repurposed. A renumbered ID is Critical — it severs every downstream reference, including any code-forge plan already generated from it.

3.2 Internal Consistency

Requirement coverage: every FR-*/NFR-* in the SRS is satisfied by at least one tech-design component, and every component names the requirements it satisfies. An unplaced requirement is Major — it is either a design gap or a requirement that should be renegotiated, and both need to be visible rather than silently dropped. Use sf-trace.py matrix --upstream "<srs.md>" --downstream "<tech-design.md>" for the coverage math, then judge whether each gap is intentional.
Scope boundaries carried through: the tech-design's §3.4 Scope reflects the SRS's §3.2 out-of-scope table. An exclusion that disappears between the requirements and the design is Major — that is how out-of-scope work gets built.
Error catalogue realized: every SRS error code maps to handling in the tech-design or a feature spec. An error code the design cannot produce is Major.
Component name matching: verify the feature-spec files and the tech-design's §8.1 Component Overview describe the same component set.

Fast path (preferred): resolve <sf_scripts> (see @../shared/scripts.md) and run the membership check —
```
python3 "<sf_scripts>/sf-trace.py" chain --tech-design "<tech-design.md>" --features-dir "docs/features" --overview "docs/features/overview.md"
```
Read its fields: slugs_without_feature_file (a §8.1 component with no matching feature spec), features_not_mentioned_in_tech_design (a feature spec with no matching component), and overview_missing_features (used in §3.4). Fallback: if python3 is unavailable or the script is not found, compare the docs/features/{name}.md slugs against the §8.1 rows by hand.

The script detects the mismatch; you classify the severity by whether it would actually break a downstream consumer like code-forge — do not hardcode Critical. A feature spec deliberately written ahead of the tech-design during iterative authoring is a Minor issue, not Critical; a genuine slug divergence that severs the traceability chain a consumer depends on is Critical.
Do feature spec API signatures match the tech-design's API Design section?
Do component boundaries in feature specs align with tech-design's Component Overview?
Are data models consistent across documents? (field names, types, relationships)
Do feature specs reference the same architectural patterns described in the tech-design?
For multi-split: are cross-sub-feature interfaces consistent?

3.3 Specificity

Vague descriptions that should be concrete (e.g., "handles errors appropriately" → specific error codes/behaviors)
Ambiguous quantifiers (e.g., "fast", "large", "many" without concrete thresholds)
Undefined behavior for edge cases or boundary conditions

3.4 Traceability

Do feature specs reference back to tech-design sections they implement?
Does overview.md list ALL generated feature specs? (no missing entries) — the sf-trace.py chain run in §3.2 reports this as overview_missing_features (feature specs absent from overview.md); fall back to scanning the feature index by hand if the script is unavailable.
Are dependency relationships between feature specs documented and consistent?

3.5 Actionability

Could a developer implement from these specs without guessing?
Are input/output formats fully specified?
Are error scenarios and recovery behaviors defined?
Are configuration options and defaults documented?

Step 4: Generate Findings

For each issue found, produce a structured finding:

- [{severity}] {file_path} § {section}: {description of issue} → FIX: {concrete fix instruction}

Compile the full review result:

REVIEW_RESULT: {PASS | ISSUES_FOUND}
CRITICAL_COUNT: {N}
MAJOR_COUNT: {N}
MINOR_COUNT: {N}

FINDINGS:
- [{severity}] {file_path} § {section}: {description} → FIX: {fix instruction}
...

Step 5: Present Results

Display summary:

spec-forge review: {feature_name}

  Documents reviewed: {N}
  Result: {PASS | ISSUES_FOUND}
  Findings: {critical} critical, {major} major, {minor} minor

  {If ISSUES_FOUND, list top findings}

If REVIEW_RESULT: PASS, inform the user and stop.

If REVIEW_RESULT: ISSUES_FOUND, proceed based on the user's auto-fix preference already collected in Step 1 (there is a single decision point — do not re-ask here):

Report only: Display all findings and stop
Fix Critical+Major: auto-fix significant issues — proceed to Step 6
Fix all: auto-fix everything — proceed to Step 6

Step 6: Auto-Fix (Iterative)

Convergence, not a fixed count: iterate fix → re-review until the review passes or a round yields no net reduction in findings (typically 1–2 rounds). Stop and report the remaining issues as soon as a round makes no progress — do not keep looping on findings the fixer cannot resolve.

6.1 Apply Fixes

For each finding to fix:

Read the target file
Apply the concrete fix described in the finding
Rules:
- Only modify the specific section cited in the finding
- Do NOT restructure or rewrite entire documents
- Do NOT add new documents — only fix existing ones
- If a fix requires information you don't have (e.g., specific domain logic), add a  comment instead of guessing
- Do NOT change content unrelated to the findings

6.2 Re-Review

After all fixes are applied, re-run the review (Step 3-4) on the same documents.

If REVIEW_RESULT: PASS: Display success

spec-forge review: PASS after fixes — {N} issues resolved

If REVIEW_RESULT: ISSUES_FOUND (a re-review round yielded no net reduction in findings): Display remaining issues and stop

spec-forge review: {N} issues remain after auto-fix

Remaining issues:
  - [{severity}] {file} § {section}: {description}
  ...

These may require manual attention or domain-specific decisions.

Step 7: Summary

Display final status:

spec-forge review complete: {feature_name}

  Documents reviewed: {N}
  Issues found: {total}
  Issues fixed: {fixed}
  Issues remaining: {remaining}

Next steps:
  /code-forge:plan @docs/features/{component-name}.md   → Generate implementation plan
  /spec-forge:review {feature_name}                      → Re-run review after manual fixes

review

SKILL.mdskills/review/SKILL.md