feat(specflow): F-093 F-093

[jcfischer]

Feature: F-093

Summary

See spec.md for full feature details

Implementation Approach

• DatabaseAdapter interface (all methods)
• DbConfig type
• VCStatus type
• Adapter-specific config types
• Type definitions compile

Files Changed

File	Changes
.specflow	+1 -0
.specify/specs/f-093-dolt-backend/docs.md	+1 -0
.specify/specs/f-093-dolt-backend/plan.md	+20 -0
.specify/specs/f-093-dolt-backend/spec.md	+8 -0
.specify/specs/f-093-dolt-backend/tasks.md	+5 -0
.specify/specs/f-093-dolt-backend/verify.md	+9 -0
CHANGELOG.md	+1 -0
package.json	+1 -0
packages/specflow/package.json	+1 -0
packages/specflow/src/commands/dolt/commit.ts	+1 -0
packages/specflow/src/commands/dolt/diff.ts	+1 -0
packages/specflow/src/commands/dolt/index.ts	+1 -0
packages/specflow/src/commands/dolt/init.ts	+2 -0
packages/specflow/src/commands/dolt/log.ts	+1 -0
packages/specflow/src/commands/dolt/pull.ts	+1 -0
packages/specflow/src/commands/dolt/push.ts	+1 -0
packages/specflow/src/commands/dolt/status.ts	+2 -0
packages/specflow/src/commands/migrate-to-dolt.ts	+4 -0
packages/specflow/src/index.ts	+1 -0
packages/specflow/src/lib/adapters/dolt.ts	+14 -0
packages/specflow/src/lib/adapters/factory.ts	+1 -0
packages/specflow/src/lib/adapters/sqlite.ts	+14 -0
packages/specflow/src/lib/adapters/types.ts	+6 -0
packages/specflow/src/lib/config.ts	+3 -0
.../specflow/src/lib/database-adapter-wrapper.ts	+1 -0

[jcfischer]

AI Code Review - PR #29

❌ CHANGES REQUESTED

This PR requires significant refactoring before approval due to extensive code duplication between the SQLite and Dolt adapters.

---

🔴 CRITICAL: Code Duplication (Dimension 7)

Severity: HIGH - This is a blocking issue.

The implementation contains ~700 lines of near-identical code across two adapter files. Almost every method in SQLiteAdapter has a duplicate in DoltAdapter that differs ONLY in:

• Database handle: db vs conn
• Query execution: db.run() vs await conn.execute()
• Date handling: toISOString() vs new Date()

Duplicated Methods (11+ methods, ~500 lines):

1. createFeature (~36 lines each)
2. updateFeature (~158 lines each - massive duplication)
3. listFeatures (~33 lines each)
4. deleteFeature (~4 lines each)
5. updateFeatureStatus (~36 lines each)
6. updateFeaturePhase, updateFeatureName, updateFeatureDescription, updateFeaturePriority (4 lines each × 4)
7. getNextFeature, getNextReadyFeature, getNextFeatureNeedingPhases (~26 lines each × 3)
8. getStats (~36 lines each)
9. rowToFeature - IDENTICAL 40-line transformation logic
10. Extended lifecycle methods (harden, review, approval operations)

Example - updateFeature duplication:

Both adapters have ~158 lines of IDENTICAL logic building setClauses and values arrays. Only difference:

• SQLite: db.run(query, values)
• Dolt: await conn.execute(query, values)

Required Refactoring:

Option 1: BaseAdapter (Recommended)

abstract class BaseAdapter implements DatabaseAdapter {
  // Shared transformation logic
  protected rowToFeature(row: any): Feature { /* 40 lines */ }
  
  // Template method pattern
  async updateFeature(id: string, updates: Partial<Feature>): Promise<void> {
    const { query, values } = this.buildUpdateQuery(id, updates);
    await this.execute(query, values);
  }
  
  // Abstract primitives
  protected abstract execute(query: string, values: any[]): Promise<void>;
  protected abstract queryOne<T>(query: string, values?: any[]): Promise<T | null>;
  protected abstract queryMany<T>(query: string, values?: any[]): Promise<T[]>;
}

class SQLiteAdapter extends BaseAdapter {
  protected async execute(query: string, values: any[]): Promise<void> {
    this.db!.run(query, values);
  }
}

class DoltAdapter extends BaseAdapter {
  protected async execute(query: string, values: any[]): Promise<void> {
    await this.connection!.execute(query, values);
  }
}

Option 2: Shared Query Builder

// lib/adapters/query-builder.ts
export class FeatureQueryBuilder {
  static buildUpdateQuery(id: string, updates: Partial<Feature>): { query: string; values: any[] } {
    // Shared 158-line logic
  }
  
  static transformRow(row: any): Feature {
    // Shared 40-line rowToFeature logic
  }
}

// In adapters:
async updateFeature(id: string, updates: Partial<Feature>): Promise<void> {
  const { query, values } = FeatureQueryBuilder.buildUpdateQuery(id, updates);
  await this.execute(query, values);
}

Impact: Current duplication will cause:

• Maintenance burden: bugs must be fixed twice
• Drift risk: implementations will diverge over time
• Tech debt: violates DRY principle at architectural level
• Test burden: identical logic tested twice

---

✅ Strengths

1. Spec Compliance: Implements the adapter pattern as specified
2. Type Safety: Full TypeScript with proper interface contracts
3. Clean Interface: DatabaseAdapter interface is well-designed
4. Factory Pattern: Proper use of factory for adapter creation
5. Migration Command: migrate-to-dolt provides safe migration path
6. No Security Issues: No hardcoded credentials, proper connection handling
7. No Debug Artifacts: Clean code, no console.logs or TODOs

---

⚠️ Other Findings

Architecture (Medium)

• Missing Tests: No test files found for adapters or migration command
• Async Consistency: SQLite adapter wraps sync calls unnecessarily (Bun SQLite is sync, forcing async adds overhead)

Code Quality (Low)

• Type Annotations: Some any types in row handling could be tighter
• Error Messages: Database errors don't include context (feature ID, operation)

---

📊 Review Summary

Dimension	Status	Severity
Spec Compliance	✅ Pass	-
Plan Adherence	✅ Pass	-
Security Review	✅ Pass	-
Code Quality	⚠️ Minor Issues	Low
Architecture Integrity	✅ Pass	-
Edge Cases	⚠️ No Tests	Medium
Code Duplication	❌ FAIL	HIGH

---

🚫 Required Actions Before Approval

1. BLOCKING: Extract shared logic to eliminate duplication

   • Use BaseAdapter pattern OR shared query builder
   • Target: <100 lines duplicated code (down from ~500)
   • Keep only database-specific primitives in adapters

2. RECOMMENDED: Add adapter tests

   • Test suite for DatabaseAdapter contract
   • Both adapters should pass identical test suite
   • Test migration command with fixtures

---

Recommendation: Request changes. The duplication is too extensive to approve. The refactoring is straightforward and will significantly improve maintainability.

[jcfischer]

AI Review: CHANGES REQUESTED

Review: F-093 Dolt Backend

Executive Summary

Status: CHANGES REQUESTED

The implementation successfully delivers the core adapter pattern and both SQLite and Dolt backends. However, there are critical architectural violations and significant code duplication that must be addressed before merging.

Critical Issues (BLOCKING)

1. Architectural Violation: Private Method Access (HIGH SEVERITY)

File: packages/specflow/src/commands/migrate-to-dolt.ts:217, 234

The migration command directly accesses private adapter methods using type casting:

const conn = (adapter as any).getConnection();

Problem:

• Violates adapter interface contract
• Breaks encapsulation
• Bypasses type safety
• Creates tight coupling to DoltAdapter internals

Solution:
Add bulk insert methods to the DatabaseAdapter interface:

interface DatabaseAdapter {
  // ...existing methods...
  
  /** Bulk insert rows (for migration) */
  bulkInsert(table: string, columns: string[], rows: any[][]): Promise<void>;
  
  /** Get row count for table */
  getTableRowCount(table: string): Promise<number>;
}

Then implement these methods properly in both adapters, removing the (adapter as any) casts.

2. Code Duplication: Dolt Command Boilerplate (HIGH SEVERITY)

Files:

• packages/specflow/src/commands/dolt/commit.ts
• packages/specflow/src/commands/dolt/status.ts
• packages/specflow/src/commands/dolt/push.ts
• packages/specflow/src/commands/dolt/pull.ts
• packages/specflow/src/commands/dolt/log.ts
• packages/specflow/src/commands/dolt/diff.ts

Every dolt command contains nearly identical boilerplate:

// Backend validation (duplicated 6x)
if (config.database.backend !== "dolt") {
  console.error("✗ Version control is only available with Dolt backend");
  console.error("  Current backend: SQLite");
  process.exit(1);
}

// Adapter lifecycle (duplicated 6x)
const adapter = await createAdapter(projectPath);
try {
  // ... actual command logic ...
} finally {
  await adapter.disconnect();
}

Solution:
Extract to shared utility:

// packages/specflow/src/commands/dolt/common.ts
export async function withDoltAdapter<T>(
  fn: (adapter: DatabaseAdapter) => Promise<T>
): Promise<T> {
  const projectPath = process.cwd();
  const config = loadConfig(projectPath);

  if (config.database.backend !== "dolt") {
    console.error("✗ Version control is only available with Dolt backend");
    console.error("  Current backend: SQLite");
    process.exit(1);
  }

  const adapter = await createAdapter(projectPath);
  try {
    return await fn(adapter);
  } finally {
    await adapter.disconnect();
  }
}

Then each command becomes:

export function createDoltCommitCommand(): Command {
  return new Command("commit")
    .description("Commit changes to Dolt database")
    .requiredOption("-m, --message <message>", "Commit message")
    .action(async (options) => {
      await withDoltAdapter(async (adapter) => {
        await adapter.commit?.(options.message);
        console.log("✓ Changes committed");
        console.log(`  Message: ${options.message}`);
      });
    });
}

This reduces ~150 lines of duplication across 6 files.

High Priority Issues

3. Missing Test Coverage

No tests exist for:

• SQLiteAdapter implementation
• DoltAdapter implementation
• Adapter factory
• Migration command
• Dolt commands

Recommendation: Add integration tests for both adapters to ensure:

• CRUD operations work correctly
• Type conversions (timestamps, JSON) are correct
• Version control operations work for Dolt
• Migration preserves data integrity

Medium Priority Issues

4. Error Handling: Migration Rollback

File: migrate-to-dolt.ts:158-161

The migration failure message suggests manual rollback:

console.error("\nRollback: Restore SQLite backend in .specflow/config.json");

Problem: User is left to manually fix config, but backup exists.

Recommendation: Implement automatic rollback that restores the config file and reports the backup location.

5. Schema Initialization Duplication

Both SQLiteAdapter.initializeSchema() and DoltAdapter.initializeSchema() contain similar DDL, just with different SQL syntax.

Recommendation: Consider extracting shared schema definition as a data structure, with adapter-specific SQL generation.

Positive Aspects

✅ Clean adapter pattern - The DatabaseAdapter interface is well-designed
✅ BaseAdapter abstraction - Excellent use of inheritance to share business logic
✅ Type safety - Strong TypeScript types throughout
✅ Backwards compatibility - SQLite adapter wraps existing code cleanly
✅ Configuration management - Clear, validated config structure
✅ Comprehensive migration tool - Handles backup, verification, rollback guidance

Review Dimensions Summary

Dimension	Status	Notes
Spec Compliance	✅ Pass	All requirements addressed
Plan Adherence	✅ Pass	Follows technical plan
Security	✅ Pass	No security issues found
Code Quality	⚠️ Issues	Duplication, architectural violations
Architecture	⚠️ Issues	Private method access, missing interface methods
Edge Cases	✅ Pass	Good error handling
Code Duplication	❌ Fail	Significant duplication in dolt commands

Verdict

CHANGES REQUESTED - The implementation is solid but has critical architectural issues that must be fixed:

1. Remove (adapter as any).getConnection() - Add proper interface methods
2. Extract dolt command boilerplate - Create withDoltAdapter() utility
3. Add test coverage - At minimum, adapter integration tests

These are not optional improvements - they are architectural requirements for maintainability and type safety.

[jcfischer]

AI Code Review - PR #29: F-093 Dolt Backend

Review Summary

I've conducted a comprehensive review of the Dolt backend implementation against all review dimensions. This PR introduces a pluggable database architecture with solid abstraction patterns, but has critical code duplication issues that must be resolved before approval.

---

⚠️ CRITICAL: Code Duplication (Dimension 7)

Issue: The dolt command files (commit.ts, push.ts, pull.ts, status.ts) contain nearly identical boilerplate code:

// Lines 16-24 appear in ALL four files:
const projectPath = process.cwd();
const config = loadConfig(projectPath);

if (config.database.backend !== "dolt") {
  console.error("✗ Version control is only available with Dolt backend");
  console.error("  Current backend: SQLite");
  process.exit(1);
}

Additionally, lines 26-33 (adapter lifecycle) are duplicated across all four files:

const adapter = await createAdapter(projectPath);
try {
  // ... command-specific logic ...
} finally {
  await adapter.disconnect();
}

Impact: This violates DRY principles and creates maintenance burden. Changes to error messages, backend checking, or adapter lifecycle would need to be updated in 4+ places.

Required Action: Extract shared logic into a common helper module (e.g., commands/dolt/common.ts):

// commands/dolt/common.ts
export async function withDoltAdapter<T>(
  action: (adapter: DatabaseAdapter) => Promise<T>
): Promise<T> {
  const projectPath = process.cwd();
  const config = loadConfig(projectPath);

  if (config.database.backend !== "dolt") {
    throw new Error("Version control is only available with Dolt backend");
  }

  const adapter = await createAdapter(projectPath);
  try {
    return await action(adapter);
  } finally {
    await adapter.disconnect();
  }
}

Then refactor commands to use this:

// commit.ts
await withDoltAdapter(async (adapter) => {
  await adapter.commit?.(options.message);
  console.log("✓ Changes committed");
});

---

✅ Architecture Integrity (Dimension 5)

Strengths:

• Clean adapter pattern using DatabaseAdapter interface
• BaseAdapter abstract class centralizes shared business logic (730 lines)
• Type definitions well-structured in adapters/types.ts
• Factory pattern correctly isolates adapter creation
• Backward compatibility maintained via wrapper pattern

Observations:

• The BaseAdapter implementation is substantial but well-organized with clear section comments
• Optional version control methods (init?, commit?, etc.) allow graceful degradation for SQLite
• Migration strategy preserves existing SQLite behavior

---

✅ Spec Compliance (Dimension 1)

Requirements Met:

• ✓ DatabaseAdapter abstraction with all specified methods
• ✓ SQLiteAdapter wraps existing bun:sqlite implementation
• ✓ DoltAdapter with MySQL2 client and git-like commands
• ✓ Configuration at .specflow/config.json with backend selection
• ✓ CLI commands: dolt init, commit, push, pull, status, log, diff
• ✓ Migration tool: migrate-to-dolt command with verification

Minor Gap:

• Spec mentions spec_data and skip_registry tables, but I don't see explicit operations for these in the adapter interface. However, this may be addressed in other parts of the codebase not included in this PR.

---

⚠️ Security Review (Dimension 3)

Concern: SQL Injection Surface Area

migrate-to-dolt.ts lines 217-218:

const conn = (adapter as any).getConnection();
await conn.execute(query, values);

Issue: Using (adapter as any) bypasses TypeScript's type safety to access internal methods. While the immediate code uses parameterized queries (safe), this pattern creates a backdoor that future maintainers might misuse.

Recommendation:

• Add a public bulkInsert method to DatabaseAdapter interface
• Or document why internal access is necessary with a comment
• Current implementation is safe but fragile

Dolt command injection:
Lines like await exec(\dolt commit -m "${message.replace(/"/g, '\"')}"`)` in dolt.ts:217 correctly escape quotes. However, using shell exec is inherently risky. Consider using Dolt's SQL interface for version control operations where possible.

---

✅ Code Quality (Dimension 4)

Strengths:

• TypeScript strict mode compliance
• Comprehensive type definitions
• Good error handling with descriptive messages
• Clear separation of concerns (types, base logic, adapters, factory)
• Migration verification with row counts
• Dry-run mode for migration command

Minor Issues:

• migrate-to-dolt.ts line 217: (adapter as any).getConnection() type escape (addressed above)
• Error messages could include actionable next steps more consistently

---

✅ Edge Cases & Robustness (Dimension 6)

Well-Handled:

• Database not initialized → clear error messages
• Dolt CLI not installed → helpful installation link
• Backend mismatch → clear error explaining version control requires Dolt
• Migration verification with row counts
• Backup creation before migration
• Connection cleanup in finally blocks

Potential Gaps:

• What happens if dolt status --json returns malformed JSON? (line 199 in dolt.ts)
• What if MySQL connection is lost mid-operation?
• What if DoltHub is unreachable during push/pull?

Recommendation: Add connection retry logic or better error context for network failures.

---

✅ Plan Adherence (Dimension 2)

The implementation follows the technical plan closely:

• ✓ Component 1: DatabaseAdapter abstraction
• ✓ Component 2: SQLiteAdapter wrapper
• ✓ Component 3: DoltAdapter with MySQL2
• ✓ Component 4: Factory and configuration
• ✓ Component 5: CLI commands
• ✓ Component 6: Migration strategy

---

Verdict: CHANGES REQUESTED

Blocking Issue: Code duplication in dolt command files violates established patterns and creates maintenance burden. This must be refactored before merge.

Action Required:

1. Extract common boilerplate into commands/dolt/common.ts helper
2. Refactor all dolt command files to use shared helper
3. Add tests for the new shared helper

Optional Improvements:

• Add bulkInsert method to adapter interface (or document why internal access is needed)
• Add error handling for malformed Dolt status JSON
• Consider connection retry logic for network operations

---

Stats

• Files changed: 25
• Lines added: ~3,800
• Core pattern: Adapter pattern with abstract base class
• Critical duplication: 4 files × 2 code blocks = 8 duplicated sections