Merge branch 'main' of github.com:steveyegge/beads

internal/storage/sqlite/migration_invariants.go

@@ -0,0 +1,204 @@
+// Package sqlite - migration safety invariants
+package sqlite
+
+import (
+	"database/sql"
+	"fmt"
+	"sort"
+	"strings"
+)
+
+// Snapshot captures database state before migrations for validation
+type Snapshot struct {
+	IssueCount      int
+	ConfigKeys      []string
+	DependencyCount int
+	LabelCount      int
+}
+
+// MigrationInvariant represents a database invariant that must hold after migrations
+type MigrationInvariant struct {
+	Name        string
+	Description string
+	Check       func(*sql.DB, *Snapshot) error
+}
+
+// invariants is the list of all invariants checked after migrations
+var invariants = []MigrationInvariant{
+	{
+		Name:        "required_config_present",
+		Description: "Required config keys must exist",
+		Check:       checkRequiredConfig,
+	},
+	{
+		Name:        "foreign_keys_valid",
+		Description: "No orphaned dependencies or labels",
+		Check:       checkForeignKeys,
+	},
+	{
+		Name:        "issue_count_stable",
+		Description: "Issue count should not decrease unexpectedly",
+		Check:       checkIssueCount,
+	},
+}
+
+// captureSnapshot takes a snapshot of the database state before migrations
+func captureSnapshot(db *sql.DB) (*Snapshot, error) {
+	snapshot := &Snapshot{}
+
+	// Count issues
+	err := db.QueryRow("SELECT COUNT(*) FROM issues").Scan(&snapshot.IssueCount)
+	if err != nil {
+		return nil, fmt.Errorf("failed to count issues: %w", err)
+	}
+
+	// Get config keys
+	rows, err := db.Query("SELECT key FROM config ORDER BY key")
+	if err != nil {
+		return nil, fmt.Errorf("failed to query config keys: %w", err)
+	}
+	defer rows.Close()
+
+	snapshot.ConfigKeys = []string{}
+	for rows.Next() {
+		var key string
+		if err := rows.Scan(&key); err != nil {
+			return nil, fmt.Errorf("failed to scan config key: %w", err)
+		}
+		snapshot.ConfigKeys = append(snapshot.ConfigKeys, key)
+	}
+	if err := rows.Err(); err != nil {
+		return nil, fmt.Errorf("error reading config keys: %w", err)
+	}
+
+	// Count dependencies
+	err = db.QueryRow("SELECT COUNT(*) FROM dependencies").Scan(&snapshot.DependencyCount)
+	if err != nil {
+		return nil, fmt.Errorf("failed to count dependencies: %w", err)
+	}
+
+	// Count labels
+	err = db.QueryRow("SELECT COUNT(*) FROM labels").Scan(&snapshot.LabelCount)
+	if err != nil {
+		return nil, fmt.Errorf("failed to count labels: %w", err)
+	}
+
+	return snapshot, nil
+}
+
+// verifyInvariants checks all migration invariants and returns error if any fail
+func verifyInvariants(db *sql.DB, snapshot *Snapshot) error {
+	var failures []string
+
+	for _, invariant := range invariants {
+		if err := invariant.Check(db, snapshot); err != nil {
+			failures = append(failures, fmt.Sprintf("%s: %v", invariant.Name, err))
+		}
+	}
+
+	if len(failures) > 0 {
+		return fmt.Errorf("migration invariants failed:\n  - %s", strings.Join(failures, "\n  - "))
+	}
+
+	return nil
+}
+
+// checkRequiredConfig ensures required config keys exist (would have caught GH #201)
+// Only enforces issue_prefix requirement if there are issues in the database
+func checkRequiredConfig(db *sql.DB, snapshot *Snapshot) error {
+	// Check current issue count (not snapshot, since migrations may add/remove issues)
+	var currentCount int
+	err := db.QueryRow("SELECT COUNT(*) FROM issues").Scan(&currentCount)
+	if err != nil {
+		return fmt.Errorf("failed to count issues: %w", err)
+	}
+
+	// Only require issue_prefix if there are issues in the database
+	// New databases can exist without issue_prefix until first issue is created
+	if currentCount == 0 {
+		return nil
+	}
+
+	// Check for required config keys
+	var value string
+	err = db.QueryRow("SELECT value FROM config WHERE key = 'issue_prefix'").Scan(&value)
+	if err == sql.ErrNoRows || value == "" {
+		return fmt.Errorf("required config key missing: issue_prefix (database has %d issues)", currentCount)
+	} else if err != nil {
+		return fmt.Errorf("failed to check config key issue_prefix: %w", err)
+	}
+
+	return nil
+}
+
+// checkForeignKeys ensures no orphaned dependencies or labels exist
+func checkForeignKeys(db *sql.DB, snapshot *Snapshot) error {
+	// Check for orphaned dependencies (issue_id not in issues)
+	var orphanedDepsIssue int
+	err := db.QueryRow(`
+		SELECT COUNT(*)
+		FROM dependencies d
+		WHERE NOT EXISTS (SELECT 1 FROM issues WHERE id = d.issue_id)
+	`).Scan(&orphanedDepsIssue)
+	if err != nil {
+		return fmt.Errorf("failed to check orphaned dependencies (issue_id): %w", err)
+	}
+	if orphanedDepsIssue > 0 {
+		return fmt.Errorf("found %d orphaned dependencies (issue_id not in issues)", orphanedDepsIssue)
+	}
+
+	// Check for orphaned dependencies (depends_on_id not in issues)
+	var orphanedDepsDependsOn int
+	err = db.QueryRow(`
+		SELECT COUNT(*)
+		FROM dependencies d
+		WHERE NOT EXISTS (SELECT 1 FROM issues WHERE id = d.depends_on_id)
+	`).Scan(&orphanedDepsDependsOn)
+	if err != nil {
+		return fmt.Errorf("failed to check orphaned dependencies (depends_on_id): %w", err)
+	}
+	if orphanedDepsDependsOn > 0 {
+		return fmt.Errorf("found %d orphaned dependencies (depends_on_id not in issues)", orphanedDepsDependsOn)
+	}
+
+	// Check for orphaned labels (issue_id not in issues)
+	var orphanedLabels int
+	err = db.QueryRow(`
+		SELECT COUNT(*)
+		FROM labels l
+		WHERE NOT EXISTS (SELECT 1 FROM issues WHERE id = l.issue_id)
+	`).Scan(&orphanedLabels)
+	if err != nil {
+		return fmt.Errorf("failed to check orphaned labels: %w", err)
+	}
+	if orphanedLabels > 0 {
+		return fmt.Errorf("found %d orphaned labels (issue_id not in issues)", orphanedLabels)
+	}
+
+	return nil
+}
+
+// checkIssueCount ensures issue count doesn't decrease unexpectedly
+func checkIssueCount(db *sql.DB, snapshot *Snapshot) error {
+	var currentCount int
+	err := db.QueryRow("SELECT COUNT(*) FROM issues").Scan(&currentCount)
+	if err != nil {
+		return fmt.Errorf("failed to count issues: %w", err)
+	}
+
+	if currentCount < snapshot.IssueCount {
+		return fmt.Errorf("issue count decreased from %d to %d (potential data loss)", snapshot.IssueCount, currentCount)
+	}
+
+	return nil
+}
+
+// GetInvariantNames returns the names of all registered invariants (for testing/inspection)
+func GetInvariantNames() []string {
+	names := make([]string, len(invariants))
+	for i, inv := range invariants {
+		names[i] = inv.Name
+	}
+	sort.Strings(names)
+	return names
+}

internal/storage/sqlite/migration_invariants_test.go

@@ -0,0 +1,260 @@
+package sqlite
+
+import (
+	"database/sql"
+	"testing"
+)
+
+func TestCaptureSnapshot(t *testing.T) {
+	db := setupInvariantTestDB(t)
+	defer db.Close()
+
+	// Create some test data
+	_, err := db.Exec(`INSERT INTO issues (id, title) VALUES ('test-1', 'Test Issue')`)
+	if err != nil {
+		t.Fatalf("failed to insert test issue: %v", err)
+	}
+
+	_, err = db.Exec(`INSERT INTO dependencies (issue_id, depends_on_id, created_by) VALUES ('test-1', 'test-1', 'test')`)
+	if err != nil {
+		t.Fatalf("failed to insert test dependency: %v", err)
+	}
+
+	_, err = db.Exec(`INSERT INTO labels (issue_id, label) VALUES ('test-1', 'test-label')`)
+	if err != nil {
+		t.Fatalf("failed to insert test label: %v", err)
+	}
+
+	snapshot, err := captureSnapshot(db)
+	if err != nil {
+		t.Fatalf("captureSnapshot failed: %v", err)
+	}
+
+	if snapshot.IssueCount != 1 {
+		t.Errorf("expected IssueCount=1, got %d", snapshot.IssueCount)
+	}
+
+	if snapshot.DependencyCount != 1 {
+		t.Errorf("expected DependencyCount=1, got %d", snapshot.DependencyCount)
+	}
+
+	if snapshot.LabelCount != 1 {
+		t.Errorf("expected LabelCount=1, got %d", snapshot.LabelCount)
+	}
+}
+
+func TestCheckRequiredConfig(t *testing.T) {
+	db := setupInvariantTestDB(t)
+	defer db.Close()
+
+	// Test with no issues - should pass even without issue_prefix
+	snapshot := &Snapshot{IssueCount: 0}
+	err := checkRequiredConfig(db, snapshot)
+	if err != nil {
+		t.Errorf("expected no error with 0 issues, got: %v", err)
+	}
+
+	// Add an issue to the database
+	_, err = db.Exec(`INSERT INTO issues (id, title) VALUES ('test-1', 'Test Issue')`)
+	if err != nil {
+		t.Fatalf("failed to insert issue: %v", err)
+	}
+
+	// Delete issue_prefix config
+	_, err = db.Exec(`DELETE FROM config WHERE key = 'issue_prefix'`)
+	if err != nil {
+		t.Fatalf("failed to delete config: %v", err)
+	}
+
+	// Should fail now that we have an issue but no prefix
+	err = checkRequiredConfig(db, snapshot)
+	if err == nil {
+		t.Error("expected error for missing issue_prefix with issues, got nil")
+	}
+
+	// Add required config back
+	_, err = db.Exec(`INSERT INTO config (key, value) VALUES ('issue_prefix', 'test')`)
+	if err != nil {
+		t.Fatalf("failed to insert config: %v", err)
+	}
+
+	// Test with required config present
+	err = checkRequiredConfig(db, snapshot)
+	if err != nil {
+		t.Errorf("expected no error with issue_prefix set, got: %v", err)
+	}
+}
+
+func TestCheckForeignKeys(t *testing.T) {
+	db := setupInvariantTestDB(t)
+	defer db.Close()
+
+	snapshot := &Snapshot{}
+
+	// Test with no data - should pass
+	err := checkForeignKeys(db, snapshot)
+	if err != nil {
+		t.Errorf("expected no error with empty db, got: %v", err)
+	}
+
+	// Create an issue
+	_, err = db.Exec(`INSERT INTO issues (id, title) VALUES ('test-1', 'Test Issue')`)
+	if err != nil {
+		t.Fatalf("failed to insert test issue: %v", err)
+	}
+
+	// Add valid dependency
+	_, err = db.Exec(`INSERT INTO dependencies (issue_id, depends_on_id, created_by) VALUES ('test-1', 'test-1', 'test')`)
+	if err != nil {
+		t.Fatalf("failed to insert dependency: %v", err)
+	}
+
+	// Should pass with valid foreign keys
+	err = checkForeignKeys(db, snapshot)
+	if err != nil {
+		t.Errorf("expected no error with valid dependencies, got: %v", err)
+	}
+
+	// Manually create orphaned dependency (bypassing FK constraints for testing)
+	_, err = db.Exec(`PRAGMA foreign_keys = OFF`)
+	if err != nil {
+		t.Fatalf("failed to disable foreign keys: %v", err)
+	}
+
+	_, err = db.Exec(`INSERT INTO dependencies (issue_id, depends_on_id, created_by) VALUES ('orphan-1', 'test-1', 'test')`)
+	if err != nil {
+		t.Fatalf("failed to insert orphaned dependency: %v", err)
+	}
+
+	_, err = db.Exec(`PRAGMA foreign_keys = ON`)
+	if err != nil {
+		t.Fatalf("failed to enable foreign keys: %v", err)
+	}
+
+	// Should fail with orphaned dependency
+	err = checkForeignKeys(db, snapshot)
+	if err == nil {
+		t.Error("expected error for orphaned dependency, got nil")
+	}
+}
+
+func TestCheckIssueCount(t *testing.T) {
+	db := setupInvariantTestDB(t)
+	defer db.Close()
+
+	// Create initial issue
+	_, err := db.Exec(`INSERT INTO issues (id, title) VALUES ('test-1', 'Test Issue')`)
+	if err != nil {
+		t.Fatalf("failed to insert test issue: %v", err)
+	}
+
+	snapshot, err := captureSnapshot(db)
+	if err != nil {
+		t.Fatalf("captureSnapshot failed: %v", err)
+	}
+
+	// Same count - should pass
+	err = checkIssueCount(db, snapshot)
+	if err != nil {
+		t.Errorf("expected no error with same count, got: %v", err)
+	}
+
+	// Add an issue - should pass (count increased)
+	_, err = db.Exec(`INSERT INTO issues (id, title) VALUES ('test-2', 'Test Issue 2')`)
+	if err != nil {
+		t.Fatalf("failed to insert second issue: %v", err)
+	}
+
+	err = checkIssueCount(db, snapshot)
+	if err != nil {
+		t.Errorf("expected no error with increased count, got: %v", err)
+	}
+
+	// Delete both issues to simulate data loss
+	_, err = db.Exec(`DELETE FROM issues`)
+	if err != nil {
+		t.Fatalf("failed to delete issues: %v", err)
+	}
+
+	// Should fail when count decreased
+	err = checkIssueCount(db, snapshot)
+	if err == nil {
+		t.Error("expected error for decreased issue count, got nil")
+	}
+}
+
+func TestVerifyInvariants(t *testing.T) {
+	db := setupInvariantTestDB(t)
+	defer db.Close()
+
+	snapshot, err := captureSnapshot(db)
+	if err != nil {
+		t.Fatalf("captureSnapshot failed: %v", err)
+	}
+
+	// All invariants should pass with empty database
+	err = verifyInvariants(db, snapshot)
+	if err != nil {
+		t.Errorf("expected no errors with empty db, got: %v", err)
+	}
+
+	// Add an issue (which requires issue_prefix)
+	_, err = db.Exec(`INSERT INTO issues (id, title) VALUES ('test-1', 'Test Issue')`)
+	if err != nil {
+		t.Fatalf("failed to insert issue: %v", err)
+	}
+
+	// Capture new snapshot with issue
+	snapshot, err = captureSnapshot(db)
+	if err != nil {
+		t.Fatalf("captureSnapshot failed: %v", err)
+	}
+
+	// Should still pass (issue_prefix is set by newTestStore)
+	err = verifyInvariants(db, snapshot)
+	if err != nil {
+		t.Errorf("expected no errors with issue and prefix, got: %v", err)
+	}
+
+	// Remove required config to trigger failure
+	_, err = db.Exec(`DELETE FROM config WHERE key = 'issue_prefix'`)
+	if err != nil {
+		t.Fatalf("failed to delete config: %v", err)
+	}
+
+	err = verifyInvariants(db, snapshot)
+	if err == nil {
+		t.Error("expected error when issue_prefix missing with issues, got nil")
+	}
+}
+
+func TestGetInvariantNames(t *testing.T) {
+	names := GetInvariantNames()
+
+	expectedNames := []string{
+		"foreign_keys_valid",
+		"issue_count_stable",
+		"required_config_present",
+	}
+
+	if len(names) != len(expectedNames) {
+		t.Errorf("expected %d invariants, got %d", len(expectedNames), len(names))
+	}
+
+	for i, name := range names {
+		if name != expectedNames[i] {
+			t.Errorf("expected invariant[%d]=%s, got %s", i, expectedNames[i], name)
+		}
+	}
+}
+
+// setupInvariantTestDB creates an in-memory test database with schema
+func setupInvariantTestDB(t *testing.T) *sql.DB {
+	t.Helper()
+
+	store := newTestStore(t, ":memory:")
+	t.Cleanup(func() { _ = store.Close() })
+
+	// Return the underlying database connection
+	return store.db
+}

internal/storage/sqlite/migrations.go

@@ -29,13 +29,66 @@ var migrations = []Migration{
 	{"content_hash_column", migrateContentHashColumn},
 }
 
-// RunMigrations executes all registered migrations in order
+// MigrationInfo contains metadata about a migration for inspection
+type MigrationInfo struct {
+	Name        string `json:"name"`
+	Description string `json:"description"`
+}
+
+// ListMigrations returns list of all registered migrations with descriptions
+// Note: This returns ALL registered migrations, not just pending ones (all are idempotent)
+func ListMigrations() []MigrationInfo {
+	result := make([]MigrationInfo, len(migrations))
+	for i, m := range migrations {
+		result[i] = MigrationInfo{
+			Name:        m.Name,
+			Description: getMigrationDescription(m.Name),
+		}
+	}
+	return result
+}
+
+// getMigrationDescription returns a human-readable description for a migration
+func getMigrationDescription(name string) string {
+	descriptions := map[string]string{
+		"dirty_issues_table":           "Adds dirty_issues table for auto-export tracking",
+		"external_ref_column":          "Adds external_ref column to issues table",
+		"composite_indexes":            "Adds composite indexes for better query performance",
+		"closed_at_constraint":         "Adds constraint ensuring closed issues have closed_at timestamp",
+		"compaction_columns":           "Adds compaction tracking columns (compacted_at, compacted_at_commit)",
+		"snapshots_table":              "Adds snapshots table for issue history",
+		"compaction_config":            "Adds config entries for compaction",
+		"compacted_at_commit_column":   "Adds compacted_at_commit to snapshots table",
+		"export_hashes_table":          "Adds export_hashes table for idempotent exports",
+		"content_hash_column":          "Adds content_hash column for collision resolution",
+	}
+	
+	if desc, ok := descriptions[name]; ok {
+		return desc
+	}
+	return "Unknown migration"
+}
+
+// RunMigrations executes all registered migrations in order with invariant checking
 func RunMigrations(db *sql.DB) error {
+	// Capture pre-migration snapshot for validation
+	snapshot, err := captureSnapshot(db)
+	if err != nil {
+		return fmt.Errorf("failed to capture pre-migration snapshot: %w", err)
+	}
+
+	// Run migrations (they are already idempotent)
 	for _, migration := range migrations {
 		if err := migration.Func(db); err != nil {
 			return fmt.Errorf("migration %s failed: %w", migration.Name, err)
 		}
 	}
+
+	// Verify invariants after migrations complete
+	if err := verifyInvariants(db, snapshot); err != nil {
+		return fmt.Errorf("post-migration validation failed: %w", err)
+	}
+
 	return nil
 }