Features
Review Command

Review Command

AI-driven quality gates for comprehensive work unit validation.

The review command performs deep, multi-dimensional analysis of work units before marking them done. It combines automated checks (ACDD compliance, test coverage, coding standards) with AI-driven code analysis to catch bugs, race conditions, security issues, and anti-patterns that automated tools miss.

The Problem: Incomplete Quality Checks

Without comprehensive review, work units ship with hidden issues:

Manual Review Process:
  ✓ Tests pass
  ✓ Linter passes
  ? ACDD compliance
  ? Test coverage complete
  ? Race conditions
  ? Security vulnerabilities
  ? Architectural anti-patterns

One person's checklist ≠ Team standards

The result: Bugs reach production, technical debt accumulates, quality standards vary by developer.

How Review Works

Review is a multi-phase quality gate that runs ALL checks before work reaches done:

Four Quality Dimensions:

  1. ACDD Compliance: Feature files, scenarios, coverage tracking
  2. Coverage Analysis: Test-to-spec traceability, implementation coverage
  3. Coding Standards: Linting, type checking, formatting
  4. AI Deep Analysis: Bugs, race conditions, security, anti-patterns

Review Output Structure

The review command generates a structured report with multiple sections:

Section 1: Critical Issues

What it contains: Blocking problems that MUST be fixed before marking done.

Examples:

  • Race conditions in concurrent code
  • Security vulnerabilities (SQL injection, XSS)
  • Missing test coverage for critical paths
  • ACDD violations (no feature file, no scenarios)
  • Broken coverage links (files moved/deleted)

Output format:

🚨 CRITICAL ISSUES (3)

1. Race Condition Detected
   File: src/auth/session.ts:45-62
   Issue: Concurrent access to sessionStore without locking
   Impact: Data corruption under high load
   Fix: Add mutex lock or use atomic operations

2. SQL Injection Vulnerability
   File: src/db/query.ts:23
   Issue: User input directly concatenated into SQL
   Impact: Database compromise
   Fix: Use parameterized queries

3. Missing Test Coverage
   Scenario: "Login with expired session"
   Status: No test mapping found
   Fix: Write test and link with fspec link-coverage

Section 2: Warnings

What it contains: Non-blocking issues that should be addressed but won't prevent done status.

Examples:

  • Code complexity metrics high
  • Incomplete error handling
  • Missing JSDoc comments
  • Deprecated API usage
  • Minor style inconsistencies

Output format:

⚠️  WARNINGS (2)

1. High Cyclomatic Complexity
   File: src/auth/validate.ts:validateCredentials()
   Complexity: 15 (threshold: 10)
   Recommendation: Extract validation logic into smaller functions

2. Incomplete Error Handling
   File: src/api/login.ts:42
   Issue: Catch block only logs error, doesn't propagate
   Recommendation: Re-throw or return error response

Section 3: Recommendations

What it contains: Suggestions for improvement, refactoring opportunities, best practices.

Examples:

  • Refactoring opportunities
  • Performance optimizations
  • Architecture improvements
  • Library upgrade suggestions

Output format:

💡 RECOMMENDATIONS (3)

1. Extract Reusable Validation Logic
   Files: src/auth/login.ts, src/auth/register.ts
   Pattern: Duplicate email validation in multiple places
   Suggestion: Create shared validator in src/utils/validation.ts

2. Consider Caching Strategy
   File: src/db/user.ts:getUserById()
   Pattern: Database query on every call
   Suggestion: Add Redis caching for user lookups

3. Use TypeScript Strict Mode
   Config: tsconfig.json
   Benefit: Catch more type errors at compile time
   Change: Set "strict": true

Section 4: ACDD Compliance

What it contains: Acceptance Criteria Driven Development compliance status.

Checks:

  • Feature file exists
  • Scenarios written
  • Test coverage linked
  • Implementation coverage linked
  • @step comments present (for stories/bugs)

Output format:

📋 ACDD COMPLIANCE

✅ Feature file exists: spec/features/user-authentication.feature
✅ Scenarios written: 3 scenarios
✅ Coverage file exists: user-authentication.feature.coverage
✅ Test mappings: 3/3 scenarios have tests
✅ Implementation mappings: 3/3 scenarios have impl links
✅ @step validation: All scenarios have @step comments

ACDD Status: ✅ COMPLIANT

Failure example:

📋 ACDD COMPLIANCE

✅ Feature file exists: spec/features/payment-processing.feature
✅ Scenarios written: 5 scenarios
❌ Coverage file missing
❌ Test mappings: 2/5 scenarios have tests
❌ Implementation mappings: 1/5 scenarios have impl links

Missing Coverage:
- "Process refund request" (no test)
- "Handle failed payment" (no test)
- "Validate payment amount" (no test)

ACDD Status: ❌ NON-COMPLIANT
Next Steps:
1. Run: fspec generate-coverage
2. Write missing tests
3. Link coverage with fspec link-coverage

Section 5: Coverage Analysis

What it contains: Detailed test-to-spec coverage breakdown.

Metrics:

  • Scenario coverage percentage
  • Lines of code covered
  • Test file count
  • Implementation file count

Output format:

📊 COVERAGE ANALYSIS

Coverage: 100% (3/3 scenarios fully covered)

Scenario Breakdown:

✅ Login with valid credentials (FULLY COVERED)
   Test: src/__tests__/auth.test.ts:45-62
   Implementation: src/auth/login.ts:10-24, src/utils/validate.ts:5-7

✅ Login with invalid credentials (FULLY COVERED)
   Test: src/__tests__/auth.test.ts:64-82
   Implementation: src/auth/login.ts:26-35

✅ Login with expired session (FULLY COVERED)
   Test: src/__tests__/auth.test.ts:84-105
   Implementation: src/auth/session.ts:15-30

Summary:
- 3 test files
- 3 implementation files
- 67 total lines covered

Section 6: Final Verdict

What it contains: Pass/fail decision and actionable next steps.

Pass criteria:

  • No critical issues
  • ACDD compliant
  • Test coverage complete

Output format (PASS):

✅ REVIEW PASSED

AUTH-001 is ready to be marked as done.

Quality Summary:
- ✅ No critical issues
- ✅ ACDD compliant
- ✅ 100% test coverage
- ⚠️  2 warnings (non-blocking)
- 💡 3 recommendations

Next Step:
fspec move AUTH-001 done

Output format (FAIL):

❌ REVIEW FAILED

AUTH-001 is NOT ready to be marked as done.

Blocking Issues:
- 🚨 3 critical issues must be fixed
- ❌ ACDD non-compliant (missing 2 test mappings)

Next Steps:
1. Fix race condition in session.ts:45-62
2. Fix SQL injection in query.ts:23
3. Write test for "Login with expired session" scenario
4. Run review again after fixes

When to Use Review

Best practice: Run review BEFORE moving to done status.

Workflow integration:

implementing → validating → [REVIEW] → done

Why before done?

  • Catches issues before marking complete
  • Enforces quality standards
  • Prevents rework after "done"
  • Maintains ACDD compliance

Natural Language Workflows

Scenario 1: Pre-Done Quality Check

YOU: "I think AUTH-001 is ready. Let's mark it done."

AI: Before marking done, let me run a comprehensive review...
    fspec review AUTH-001

Running review for AUTH-001: User login with email/password

Phase 1: ACDD Compliance... ✅
Phase 2: Coverage Analysis... ✅
Phase 3: Coding Standards... ✅
Phase 4: AI Deep Analysis... 🔍

⚠️  WARNINGS (1)

1. High Cyclomatic Complexity
   File: src/auth/validate.ts:validateEmail()
   Complexity: 12 (threshold: 10)
   Recommendation: Extract regex patterns into constants

💡 RECOMMENDATIONS (1)

1. Add Rate Limiting
   File: src/api/login.ts
   Pattern: No rate limiting on login endpoint
   Suggestion: Add rate limiting to prevent brute force attacks

✅ REVIEW PASSED

AUTH-001 is ready to be marked as done.

Quality Summary:
- ✅ No critical issues
- ✅ ACDD compliant
- ✅ 100% test coverage
- ⚠️  1 warning (non-blocking)

YOU: "Great! The warning is minor. Let's mark it done."

AI: fspec move AUTH-001 done

    ✓ AUTH-001 moved to done
    ✓ All quality gates passed

Scenario 2: Review Finds Critical Issues

YOU: "Review AUTH-002 before marking done."

AI: Running comprehensive review...
    fspec review AUTH-002

Running review for AUTH-002: Password reset flow

Phase 1: ACDD Compliance... ❌
Phase 2: Coverage Analysis... ⚠️
Phase 3: Coding Standards... ✅
Phase 4: AI Deep Analysis... 🚨

🚨 CRITICAL ISSUES (2)

1. Race Condition Detected
   File: src/auth/reset-token.ts:generateToken()
   Issue: Token generation uses non-cryptographic random
   Impact: Predictable reset tokens, account takeover risk
   Fix: Use crypto.randomBytes() instead of Math.random()

2. Missing Test Coverage
   Scenario: "Reset with expired token"
   Status: No test mapping found
   Fix: Write test and link with fspec link-coverage

📋 ACDD COMPLIANCE

✅ Feature file exists
✅ Scenarios written: 3 scenarios
❌ Test mappings: 2/3 scenarios have tests
❌ Missing: "Reset with expired token"

❌ REVIEW FAILED

AUTH-002 is NOT ready to be marked as done.

Blocking Issues:
- 🚨 2 critical issues must be fixed

Next Steps:
1. Replace Math.random() with crypto.randomBytes()
2. Write test for "Reset with expired token"
3. Link test coverage
4. Run review again

AI: I found critical security issues. I'll fix them now...

    [Fixes token generation vulnerability]
    [Writes missing test]
    [Links coverage]

    Now let me re-run the review...
    fspec review AUTH-002

    ✅ REVIEW PASSED
    All critical issues resolved!

    fspec move AUTH-002 done
    ✓ AUTH-002 moved to done

Scenario 3: ACDD Compliance Failure

YOU: "Review PAY-005."

AI: fspec review PAY-005

Running review for PAY-005: Process refund

Phase 1: ACDD Compliance... ❌

📋 ACDD COMPLIANCE

❌ Feature file missing
❌ No scenarios written
❌ Coverage file missing

ACDD Status: ❌ NON-COMPLIANT

❌ REVIEW FAILED

PAY-005 cannot be marked done without ACDD compliance.

Next Steps:
1. Create feature file: fspec create-feature "Payment Refunds"
2. Add scenarios for refund logic
3. Generate coverage: fspec generate-coverage
4. Write tests and link coverage

AI: This work unit doesn't follow ACDD. Let me fix that...

    Creating feature file for payment refunds...
    fspec create-feature "Payment Refunds"

    Adding scenarios based on existing code...
    fspec add-scenario payment-refunds "Process full refund"
    fspec add-scenario payment-refunds "Process partial refund"
    fspec add-scenario payment-refunds "Handle refund failure"

    Generating coverage files...
    fspec generate-coverage

    Now linking existing tests (reverse ACDD)...
    [Links tests and implementation]

    Re-running review...
    fspec review PAY-005

    ✅ REVIEW PASSED
    ACDD compliance established!

Scenario 4: AI Catches Subtle Bug

YOU: "Review API-012."

AI: fspec review API-012

Running review for API-012: User search endpoint

Phase 1: ACDD Compliance... ✅
Phase 2: Coverage Analysis... ✅
Phase 3: Coding Standards... ✅
Phase 4: AI Deep Analysis... 🚨

🚨 CRITICAL ISSUES (1)

1. Potential Race Condition
   File: src/api/search.ts:searchUsers()
   Issue: Async cache read/write without atomicity

   Code analysis:
   ```typescript
   async searchUsers(query: string) {
     const cached = await cache.get(query);
     if (!cached) {
       const results = await db.query(query);
       await cache.set(query, results);  // ← Race window here
       return results;
     }
     return cached;
   }

Problem: Two concurrent requests with same query can both miss cache and query database, causing duplicate writes.

Impact: Cache inconsistency, wasted database queries

Fix: Use cache.getOrSet() pattern or distributed lock

AI: This is a subtle concurrency bug that tests might not catch. Let me fix it by using an atomic cache pattern...

[Refactors to use getOrSet pattern]

Re-running review... fspec review API-012

✅ REVIEW PASSED Race condition eliminated!


## AI Deep Analysis: What It Checks

The AI analysis phase reads your code and performs deep reasoning:

<Mermaid chart={`
graph TB
    A[AI Analysis Engine] --> B[Bug Detection]
    A --> C[Race Condition Analysis]
    A --> D[Security Audit]
    A --> E[Anti-Pattern Detection]
    A --> F[Code Quality Assessment]

    B --> G[Logic Errors Off-by-one Null pointer Type mismatches]

    C --> H[Concurrency Issues Shared state Non-atomic ops Missing locks]

    D --> I[Security Flaws Injection attacks Auth bypass Crypto misuse]

    E --> J[Design Problems God objects Circular deps Code duplication]

    F --> K[Maintainability Complexity Readability Documentation]

    style A fill:#1a3a5c
    style B fill:#ff6b6b
    style C fill:#ff6b6b
    style D fill:#ff6b6b
    style E fill:#b8860b
    style F fill:#70ad47
`} />

### Bug Detection

**What it finds**:
- Logic errors (off-by-one, incorrect conditionals)
- Null pointer dereferences
- Type mismatches
- Unreachable code
- Infinite loops

**Example finding**:

File: src/utils/paginate.ts:15 Issue: Off-by-one error in pagination logic

Code: const totalPages = Math.floor(total / pageSize); // ← Bug

Fix: const totalPages = Math.ceil(total / pageSize);

Impact: Last page items not shown when total not divisible by pageSize


### Race Condition Analysis

**What it finds**:
- Concurrent access to shared state without synchronization
- Non-atomic read-modify-write operations
- Missing mutex locks
- Async/await pitfalls

**Example finding**:

File: src/cache/manager.ts:42-50 Issue: Read-modify-write race condition

Code: const count = await getCount(); await setCount(count + 1); // ← Race window

Fix: Use atomic increment or lock await incrementCount(); // Atomic operation


### Security Audit

**What it finds**:
- SQL injection vulnerabilities
- XSS attack vectors
- CSRF missing protection
- Insecure cryptography
- Authentication bypass
- Authorization flaws

**Example finding**:

File: src/api/users.ts:23 Issue: SQL Injection Vulnerability

Code: const sql = SELECT * FROM users WHERE email = '${email}';

Fix: Use parameterized queries const sql = 'SELECT * FROM users WHERE email = ?'; db.query(sql, [email]);


### Anti-Pattern Detection

**What it finds**:
- God objects (classes doing too much)
- Circular dependencies
- Code duplication
- Tight coupling
- Magic numbers/strings

**Example finding**:

Files: src/auth/login.ts, src/auth/register.ts, src/api/profile.ts Issue: Duplicate email validation logic in 3 places

Recommendation: Extract to shared validator src/utils/validators/email.ts


### Code Quality Assessment

**What it checks**:
- Cyclomatic complexity
- Function length
- Parameter count
- Nesting depth
- Documentation coverage

**Example finding**:

File: src/auth/validate.ts:validateUser() Metrics:

  • Cyclomatic complexity: 15 (threshold: 10)
  • Function length: 85 lines (threshold: 50)
  • Parameter count: 7 (threshold: 5)

Recommendation: Break into smaller functions

  • validateUserEmail()
  • validateUserPassword()
  • validateUserProfile()

## Review Command Options

### Basic Usage

```bash
fspec review <work-unit-id>

Example:

fspec review AUTH-001

Output Formats

Markdown (default):

fspec review AUTH-001

JSON (machine-readable):

fspec review AUTH-001 --format=json

HTML (rich formatting):

fspec review AUTH-001 --format=html --output=review-report.html

Selective Checks

Skip AI analysis (faster, automated checks only):

fspec review AUTH-001 --skip-ai

Only ACDD compliance:

fspec review AUTH-001 --only-acdd

Only coverage analysis:

fspec review AUTH-001 --only-coverage

Integration with Virtual Hooks

Review can be automated as a pre-done hook:

# Add review as blocking hook before marking done
fspec add-virtual-hook AUTH-001 pre-done "fspec review AUTH-001" --blocking

What this does:

  • Automatically runs review before moving to done
  • Blocks done transition if review fails
  • Enforces quality gates programmatically

Example workflow:

YOU: "Move AUTH-001 to done."

AI: fspec move AUTH-001 done

    🪝 Running pre-done hook: review

    [Review runs automatically]

    ❌ Review failed - 2 critical issues found

    Hook blocked the done transition.
    Fix the critical issues first.

Best Practices

✅ DO

  • Run review before done - Make it part of your workflow
  • Fix critical issues immediately - Don't defer blocking problems
  • Address warnings incrementally - Non-blocking but important
  • Use review as learning tool - Understand WHY issues are flagged
  • Automate with hooks - Add as pre-done virtual hook
  • Review early and often - Don't wait until end of work unit
  • Read AI recommendations - They improve code quality
  • Share review reports - Use for code review discussions

❌ DON'T

  • Skip review - Defeats quality gates
  • Ignore critical issues - They exist for a reason
  • Mark done despite failures - Undermines ACDD compliance
  • Dismiss AI findings without investigation - AI catches real bugs
  • Run review once at the end - Incremental review catches issues earlier
  • Disable all checks - Defeats the purpose

Common Patterns

Pattern 1: Pre-Commit Review

Review before committing to ensure code quality:

# Add as pre-commit hook
fspec add-virtual-hook AUTH-001 pre-validating "fspec review AUTH-001" --blocking

Pattern 2: CI/CD Integration

Integrate review into continuous integration:

# .github/workflows/quality.yml
- name: Review Work Units
  run: |
    fspec list-work-units --status implementing | while read id; do
      fspec review $id --format=json --output=review-$id.json
    done

Pattern 3: Incremental Review

Review after each phase:

After Testing:   fspec review AUTH-001 --only-coverage
After Implementing: fspec review AUTH-001 --skip-ai
Before Done:     fspec review AUTH-001 (full review)

Pattern 4: Batch Review

Review multiple work units:

# Review all work units in validating status
fspec list-work-units --status validating | while read id; do
  echo "Reviewing $id..."
  fspec review $id
done

Troubleshooting

Issue: Review takes too long

Problem: AI analysis phase is slow

Solution: Skip AI for faster review

fspec review AUTH-001 --skip-ai

Or run AI analysis separately:

fspec review AUTH-001 --only-ai

Issue: False positives in AI analysis

Problem: AI flags code that's actually correct

Solution: Document why the code is correct, AI will learn context

// @review-ignore: This looks like race condition but is safe
// because cache.get/set is atomic in Redis
const cached = await cache.get(key);

Issue: Review fails but issues seem minor

Problem: Review marks work as failed for non-critical issues

Solution: Check issue severity

  • 🚨 Critical = must fix
  • ⚠️ Warning = should fix, but non-blocking
  • 💡 Recommendation = optional

If only warnings, review should still pass.

Issue: ACDD compliance fails for legacy code

Problem: Reverse engineering existing code without specs

Solution: Use --skip-acdd for legacy work units

fspec review LEGACY-001 --skip-acdd

Then gradually add ACDD compliance in separate work unit.

Summary

The review command provides comprehensive quality gates:

Before Review:

  • Manual checklists
  • Inconsistent standards
  • Bugs slip through
  • ACDD compliance uncertain

With Review:

  • ✅ Automated ACDD compliance checking
  • ✅ Comprehensive coverage analysis
  • ✅ AI-driven bug detection
  • ✅ Security vulnerability scanning
  • ✅ Race condition identification
  • ✅ Anti-pattern detection
  • ✅ Consistent quality standards
  • ✅ Actionable recommendations

Four Quality Dimensions:

  1. ACDD Compliance: Feature files, scenarios, coverage
  2. Coverage Analysis: Test-to-spec traceability
  3. Coding Standards: Linting, types, formatting
  4. AI Deep Analysis: Bugs, security, race conditions

Output Sections:

  • 🚨 Critical Issues (must fix)
  • ⚠️ Warnings (should fix)
  • 💡 Recommendations (consider)
  • 📋 ACDD Compliance (pass/fail)
  • 📊 Coverage Analysis (gaps)
  • ✅/❌ Final Verdict (ready/not ready)

Core Command:

fspec review <work-unit-id>           # Full review
fspec review <id> --skip-ai           # Faster, no AI
fspec review <id> --only-acdd         # ACDD only
fspec review <id> --format=json       # Machine-readable

Integration Points:

  • Pre-Done Hook: Automatic quality gate
  • CI/CD: Continuous quality monitoring
  • Code Review: Structured review reports
  • ACDD Workflow: Compliance enforcement

Review transforms subjective code review into objective, comprehensive, AI-enhanced quality gates.

Epic ManagementTagging System