Review — Specification Quality & Consistency Review

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

Core Principles

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

Severity Levels

Workflow

Step 1: Determine Review Scope

Parse the arguments to determine what to review:

1. 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)
2. If no argument, scan docs/ for the most recent SRS and tech-design, plus all feature specs in docs/features/
3. 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:

1. 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
2. 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
3. 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:

1. Read the target file
2. Apply the concrete fix described in the finding
3. 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 <!-- REVIEW: {question} --> 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