lvlup-sw/spec-review

Spec Review Skill

Overview

Stage 1 of two-stage review: Verify implementation matches specification and follows TDD.

For a complete worked example, see references/worked-example.md.

MANDATORY: Before accepting any rationalization for approving without full verification, consult references/rationalization-refutation.md. Every common excuse is catalogued with a counter-argument and the correct action.

Triggers

Activate this skill when:

• User runs review command (first stage)
• Task implementation is complete
• Need to verify spec compliance before quality review
• Subagent reports task completion

Execution Context

This skill runs in a SUBAGENT spawned by the orchestrator, not inline.

The orchestrator provides:

• State file path (preferred) OR design/plan paths
• Diff output from exarchos_orchestrate({ action: "review_diff" }) (context-efficient)
• Task ID being reviewed

The subagent:

• Reads state file to get artifact paths
• Uses diff output instead of reading full files
• Runs verification commands
• Generates report
• Returns verdict to orchestrator

Data Handoff Protocol

The orchestrator is responsible for generating the diff before dispatching the spec-review subagent. The subagent does NOT generate its own diff.

Orchestrator responsibilities:

1. Generate diff: exarchos_orchestrate({ action: "review_diff", worktreePath: "<worktree-path>", baseBranch: "main" })
2. Pass diff content in the subagent dispatch prompt
3. Include state file path for artifact resolution

Subagent responsibilities:

1. Receive diff content from dispatch prompt (do NOT re-generate)
2. Read state file for design/plan artifact paths
3. Run verification commands against the working tree
4. Return structured JSON verdict

Context-Efficient Input

Instead of per-worktree diffs, receive an integrated diff from the integration branch (e.g., feature/integration-branch) against main:

# Generate integrated diff for review
git diff main...integration > /tmp/combined-diff.patch

# Alternative: use review-diff script against integration branch via orchestrate
# exarchos_orchestrate({ action: "review_diff", worktreePath: "<worktree-path>", baseBranch: "main" })

This provides the complete picture of all changes across all tasks and reduces context consumption by 80-90%.

Pre-Review Schema Discovery

Before evaluating, query the review strategy runbook to determine the appropriate evaluation approach:

• Evaluation strategy: exarchos_orchestrate({ action: "runbook", id: "review-strategy" }) to determine the review approach based on diff scope, prior fix cycles, and review stage.

Review Scope

Review Scope: Combined Changes

After delegation completes, spec review examines:

• The complete integrated diff (main...feature/integration branch)
• All changes across all tasks in one view
• The full picture of combined functionality

This enables catching:

• Cross-task interface mismatches
• Bugs not visible in isolation
• Combined behavior vs specification

Spec Review focuses on:

• Functional completeness
• Test adequacy (outcome-based, tier-scaled — not test-first ordering)
• Specification alignment
• Test coverage
• Intended-vs-delivered: the delivered diff fulfils artifacts.intent — no intended-but-missing or delivered-but-unintended (scope-creep) work (when intentGrounding is supplied)

Does NOT cover (that's Quality Review):

• Code style
• SOLID principles
• Performance optimization
• Error handling elegance

Intended-vs-Delivered Grounding

The orchestrator captures the intended change as artifacts.intent (surfaces, a summary, and — when available — a one-line transcript summary) and threads it into your dispatch as an intentGrounding directive on the back-of-pipeline code-review path. When present, you MUST verify the delivered diff against the intended change:

• Intended-but-missing — a surface or outcome the intent calls for that the diff does not deliver. Flag as a spec issue.
• Delivered-but-unintended (scope creep) — changes outside the intended surfaces/summary with no spec justification. Flag as a spec issue.

When no intentGrounding is supplied (an empty or un-resolvable diff), proceed against the diff alone — do NOT fabricate an intent. This grounding is additive to the spec-alignment checks below, not a replacement for them.

Review Checklist

For the full checklist with verification commands, tables, and report template, see references/review-checklist.md.

Verification:

npm run test:run
npm run test:coverage
npm run typecheck

exarchos_orchestrate({
  action: "check_test_adequacy",
  featureId: "<featureId>",
  taskId: "<taskId>",
  branch: "<branch>",
  riskTier: "<low|medium|high>"
})

Fix Loop

If review FAILS, the fix-loop is bounded by the shared escalation policy (config-resolvable escalation.maxIterations, default 5) — it does NOT loop unboundedly. check_review_verdict returns the escalate decision the loop MUST honor: on a NEEDS_FIXES verdict it carries escalate: true + escalationReason when the loop must stop (the auto-fix bound was hit OR a finding is intent-touching), and the report's routing instruction reflects this.

Two outcomes:

1. Auto-fix (under the bound, MECHANICAL findings) — escalate is absent/falsy. Re-dispatch to the implementer and re-review, as below. The verdict report surfaces the remaining budget (e.g. "fix cycle N/maxIterations").
2. Escalate to the user (ask-user) — escalate: true. Do NOT re-dispatch /delegate --fixes. Surface the findings and escalationReason to the user and ask how to proceed (accept, redesign, or adjust scope). This happens when EITHER:
   • the auto-fix bound (escalation.maxIterations, default 5) is reached — the loop has fixed-and-re-reviewed that many times without converging; OR
   • a finding is intent-touching — a spec-category issue (intended-but-missing or scope-creep) that changes what was asked for, so a human decides rather than the loop silently "fixing" it. Intent-touching findings escalate immediately, regardless of how many cycles have run.

The fix-cycle count is event-sourced (prior review-verdict NEEDS_FIXES gate events) — there is no separate counter to maintain, and check_review_verdict reads it for you.

Auto-fix path — re-dispatch to implementer:

1. Create fix task with specific issues
2. Dispatch to implementer (same or new)
3. Re-review after fixes — check_review_verdict re-evaluates the bound each pass

// Return to implementer (auto-fix path only — when escalate is falsy)
Task({
  model: "opus",
  description: "Fix spec review issues",
  prompt: `
# Fix Required: Spec Review Failed

## Issues to Fix
1. Missing rate limiting implementation
   - Add rate limiter middleware
   - Test: RateLimiter_ExceedsLimit_Returns429

2. Email validation incomplete
   - Add MX record check
   - Test: ValidateEmail_InvalidDomain_ReturnsError

## Success Criteria
- All tests pass
- Coverage >80%
- All issues resolved
`
})

Required Output Format

The subagent MUST return results as structured JSON. The orchestrator parses this JSON to populate state. Any other format is an error.

{
  "verdict": "pass | fail | blocked",
  "summary": "1-2 sentence summary",
  "issues": [
    {
      "severity": "HIGH | MEDIUM | LOW",
      "category": "spec | tdd | coverage",
      "file": "path/to/file",
      "line": 123,
      "description": "Issue description",
      "required_fix": "What must change"
    }
  ],
  "test_results": {
    "passed": 0,
    "failed": 0,
    "coverage_percent": 0
  }
}

Anti-Patterns

| Don't | Do Instead | |-------|------------| | Skip to quality review | Complete spec review first | | Accept incomplete work | Return for fixes | | Review code style here | Save for quality review | | Approve without tests | Require test coverage | | Let scope creep pass | Flag over-engineering |

Cross-Task Integration Issues

If an issue spans multiple tasks:

1. Classify as "cross-task integration"
2. Create fix task specifying ALL affected tasks
3. Dispatch fix to implementer with context from all affected tasks
4. Mark original tasks as blocked until cross-task fix completes

State Management

On Review Complete

Pass:

action: "update", featureId: "<id>", updates: {
  "reviews": { "spec-review": { "status": "pass", "summary": "...", "issues": [] } }
}

Fail:

action: "update", featureId: "<id>", updates: {
  "reviews": { "spec-review": { "status": "fail", "summary": "...", "issues": [{ "severity": "...", "file": "...", "description": "..." }] } }
}

Important: The review value MUST be an object with a status field (e.g., { "status": "pass" }), not a flat string (e.g., "pass"). The all-reviews-passed guard silently ignores non-object entries. Accepted statuses: pass, passed, approved, fixes-applied.

Phase Transitions and Guards

For the full transition table, consult @skills/workflow-state/references/phase-transitions.md.

Quick reference:

• review → synthesize requires guard all-reviews-passed — all reviews.{name}.status must be passing
• review → delegate requires guard any-review-failed — triggers fix cycle when any review fails

Schema Discovery

Use exarchos_workflow({ action: "describe", actions: ["update", "init"] }) for parameter schemas and exarchos_workflow({ action: "describe", playbook: "feature" }) for phase transitions, guards, and playbook guidance. Use exarchos_orchestrate({ action: "describe", actions: ["check_test_adequacy", "check_static_analysis"] }) for orchestrate action schemas.

Transition

All transitions happen immediately without user confirmation:

Pre-Chain Validation (MANDATORY)

Before invoking quality-review:

1. Verify reviews["spec-review"].status === "pass" in workflow state (all tasks passed)
2. If not: "Spec review did not pass, cannot proceed to quality review"

Guard shape: The all-reviews-passed guard requires reviews["spec-review"] to be an object with a status field set to a passing value (pass, passed, approved, fixes-applied). Flat strings like reviews: { "spec-review": "pass" } are silently ignored and will block the review → synthesize transition.

If PASS:

1. Record results — the reviews value MUST be an object with a status field, not a flat string:

   exarchos_workflow({ action: "update", featureId: "<id>", updates: {
     reviews: { "spec-review": { status: "pass", summary: "...", issues: [] } }
   }})
   

2. Output: "Spec review passed. Auto-continuing to quality review..."
3. Orchestrator dispatches quality-review subagent immediately

Gate events: Do NOT manually emit gate.executed events via exarchos_event. Gate events are automatically emitted by the check_review_verdict orchestrate handler. Manual emission causes duplicates.

If FAIL:

1. Record results with failing status and issue details:

   exarchos_workflow({ action: "update", featureId: "<id>", updates: {
     reviews: { "spec-review": { status: "fail", summary: "...", issues: [{ severity: "HIGH", file: "...", description: "..." }] } }
   }})
   

2. Output: "Spec review found [N] issues. Auto-continuing to fixes..."
3. Auto-invoke delegate with fix tasks:

   [Invoke the exarchos:delegate skill with args: --fixes <plan-path>]
   

This is NOT a human checkpoint - workflow continues autonomously.

Troubleshooting

| Issue | Cause | Resolution | |-------|-------|------------| | Test file not found | Task didn't create expected test | Check plan for test file paths, verify worktree contents | | Coverage below threshold | Implementation incomplete or tests superficial | Add missing test cases, verify assertions are meaningful | | Test-adequacy kill-probe fails | A new/changed test still passes against reverted source (vacuous) | Strengthen the test so reverting the implementation makes it fail | | Diff too large for context | Many tasks with large changes | Generate per-worktree diffs with exarchos_orchestrate({ action: "review_diff", worktreePath: "<task-worktree>" }) to review incrementally |

Performance Notes

• Use the integrated diff (exarchos_orchestrate({ action: "review_diff" })) instead of reading full files — reduces context by 80-90%
• Review per-task when the combined diff exceeds 2,000 lines
• Run the test-adequacy kill-probe (exarchos_orchestrate({ action: "check_test_adequacy" })) in parallel with spec tracing