From eefeb1a5bcfa01a329e37f199a3ae14f80746072 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Tue, 30 Dec 2025 10:37:53 -0800 Subject: [PATCH] refactor: dedupe error classification, fix --force+--source=db conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge cleanup of PR #805: 1. Extract duplicate error classification logic into classifyDatabaseError() helper function (was duplicated in two places in database.go) 2. Fix semantic conflict between --force and --source=db flags: - --force implies "database is broken, rebuild from JSONL" - --source=db implies "use database as source of truth" - These are contradictory; now errors with clear message - --force with --source=auto or --source=jsonl works as expected 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 Executed-By: beads/crew/dave Rig: beads Role: crew --- cmd/bd/doctor/database.go | 155 +++++++++++++--------------------- cmd/bd/doctor/fix/recovery.go | 30 ++++--- 2 files changed, 79 insertions(+), 106 deletions(-) diff --git a/cmd/bd/doctor/database.go b/cmd/bd/doctor/database.go index 47e23fb7..4f2be1ee 100644 --- a/cmd/bd/doctor/database.go +++ b/cmd/bd/doctor/database.go @@ -249,65 +249,19 @@ func CheckDatabaseIntegrity(path string) DoctorCheck { // Open database in read-only mode for integrity check db, err := sql.Open("sqlite3", sqliteConnString(dbPath, true)) if err != nil { - // Classify the error type for better recovery guidance - errMsg := err.Error() - var errorType string - var recoverySteps string - // Check if JSONL recovery is possible jsonlCount, _, jsonlErr := CountJSONLIssues(filepath.Join(beadsDir, "issues.jsonl")) if jsonlErr != nil { jsonlCount, _, jsonlErr = CountJSONLIssues(filepath.Join(beadsDir, "beads.jsonl")) } + jsonlAvailable := jsonlErr == nil && jsonlCount > 0 - // Classify error type - if strings.Contains(errMsg, "database is locked") { - errorType = "Database is locked" - recoverySteps = "1. Check for running bd processes: ps aux | grep bd\n" + - "2. Kill any stale processes\n" + - "3. Remove stale locks: rm .beads/beads.db-shm .beads/beads.db-wal .beads/daemon.lock\n" + - "4. Retry: bd doctor --fix" - } else if strings.Contains(errMsg, "not a database") || strings.Contains(errMsg, "file is not a database") { - errorType = "File is not a valid SQLite database" - if jsonlErr == nil && jsonlCount > 0 { - recoverySteps = fmt.Sprintf("Database file is corrupted beyond repair.\n\n"+ - "Recovery steps:\n"+ - "1. Backup corrupt database: mv .beads/beads.db .beads/beads.db.broken\n"+ - "2. Rebuild from JSONL (%d issues): bd doctor --fix --force --source=jsonl\n"+ - "3. Verify: bd stats", jsonlCount) - } else { - recoverySteps = "Database file is corrupted and no JSONL backup found.\n" + - "Manual recovery required:\n" + - "1. Restore from git: git checkout HEAD -- .beads/issues.jsonl\n" + - "2. Rebuild database: bd doctor --fix --force" - } - } else if strings.Contains(errMsg, "migration") || strings.Contains(errMsg, "validation failed") { - errorType = "Database migration or validation failed" - if jsonlErr == nil && jsonlCount > 0 { - recoverySteps = fmt.Sprintf("Database has validation errors (possibly orphaned dependencies).\n\n"+ - "Recovery steps:\n"+ - "1. Backup database: mv .beads/beads.db .beads/beads.db.broken\n"+ - "2. Rebuild from JSONL (%d issues): bd doctor --fix --force --source=jsonl\n"+ - "3. Verify: bd stats\n\n"+ - "Alternative: bd doctor --fix --force (attempts to repair in-place)", jsonlCount) - } else { - recoverySteps = "Database validation failed and no JSONL backup available.\n" + - "Try: bd doctor --fix --force" - } - } else { - errorType = "Failed to open database" - if jsonlErr == nil && jsonlCount > 0 { - recoverySteps = fmt.Sprintf("Run 'bd doctor --fix --source=jsonl' to rebuild from JSONL (%d issues)", jsonlCount) - } else { - recoverySteps = "Run 'bd doctor --fix --force' to attempt recovery" - } - } - + errorType, recoverySteps := classifyDatabaseError(err.Error(), jsonlCount, jsonlAvailable) return DoctorCheck{ Name: "Database Integrity", Status: StatusError, Message: errorType, - Detail: fmt.Sprintf("%s\n\nError: %s", recoverySteps, errMsg), + Detail: fmt.Sprintf("%s\n\nError: %s", recoverySteps, err.Error()), Fix: "See recovery steps above", } } @@ -317,65 +271,23 @@ func CheckDatabaseIntegrity(path string) DoctorCheck { // This checks the entire database for corruption rows, err := db.Query("PRAGMA integrity_check") if err != nil { - // Classify the error type for better recovery guidance - errMsg := err.Error() - var errorType string - var recoverySteps string - // Check if JSONL recovery is possible jsonlCount, _, jsonlErr := CountJSONLIssues(filepath.Join(beadsDir, "issues.jsonl")) if jsonlErr != nil { jsonlCount, _, jsonlErr = CountJSONLIssues(filepath.Join(beadsDir, "beads.jsonl")) } + jsonlAvailable := jsonlErr == nil && jsonlCount > 0 - // Classify error type (same logic as opening errors) - if strings.Contains(errMsg, "database is locked") { - errorType = "Database is locked" - recoverySteps = "1. Check for running bd processes: ps aux | grep bd\n" + - "2. Kill any stale processes\n" + - "3. Remove stale locks: rm .beads/beads.db-shm .beads/beads.db-wal .beads/daemon.lock\n" + - "4. Retry: bd doctor --fix" - } else if strings.Contains(errMsg, "not a database") || strings.Contains(errMsg, "file is not a database") { - errorType = "File is not a valid SQLite database" - if jsonlErr == nil && jsonlCount > 0 { - recoverySteps = fmt.Sprintf("Database file is corrupted beyond repair.\n\n"+ - "Recovery steps:\n"+ - "1. Backup corrupt database: mv .beads/beads.db .beads/beads.db.broken\n"+ - "2. Rebuild from JSONL (%d issues): bd doctor --fix --force --source=jsonl\n"+ - "3. Verify: bd stats", jsonlCount) - } else { - recoverySteps = "Database file is corrupted and no JSONL backup found.\n" + - "Manual recovery required:\n" + - "1. Restore from git: git checkout HEAD -- .beads/issues.jsonl\n" + - "2. Rebuild database: bd doctor --fix --force" - } - } else if strings.Contains(errMsg, "migration") || strings.Contains(errMsg, "validation failed") { - errorType = "Database migration or validation failed" - if jsonlErr == nil && jsonlCount > 0 { - recoverySteps = fmt.Sprintf("Database has validation errors (possibly orphaned dependencies).\n\n"+ - "Recovery steps:\n"+ - "1. Backup database: mv .beads/beads.db .beads/beads.db.broken\n"+ - "2. Rebuild from JSONL (%d issues): bd doctor --fix --force --source=jsonl\n"+ - "3. Verify: bd stats\n\n"+ - "Alternative: bd doctor --fix --force (attempts to repair in-place)", jsonlCount) - } else { - recoverySteps = "Database validation failed and no JSONL backup available.\n" + - "Try: bd doctor --fix --force" - } - } else { + errorType, recoverySteps := classifyDatabaseError(err.Error(), jsonlCount, jsonlAvailable) + // Override default error type for this specific case + if errorType == "Failed to open database" { errorType = "Failed to run integrity check" - if jsonlErr == nil && jsonlCount > 0 { - recoverySteps = fmt.Sprintf("Run 'bd doctor --fix --force --source=jsonl' to rebuild from JSONL (%d issues)", jsonlCount) - } else { - recoverySteps = "Run 'bd doctor --fix --force' to attempt recovery" - } } - return DoctorCheck{ Name: "Database Integrity", Status: StatusError, Message: errorType, - Detail: fmt.Sprintf("%s\n\nError: %s", recoverySteps, errMsg), + Detail: fmt.Sprintf("%s\n\nError: %s", recoverySteps, err.Error()), Fix: "See recovery steps above", } } @@ -649,6 +561,57 @@ func FixDBJSONLSync(path string) error { // Helper functions +// classifyDatabaseError classifies a database error and returns appropriate recovery guidance. +// Returns the error type description and recovery steps based on error message and JSONL availability. +func classifyDatabaseError(errMsg string, jsonlCount int, jsonlAvailable bool) (errorType, recoverySteps string) { + switch { + case strings.Contains(errMsg, "database is locked"): + errorType = "Database is locked" + recoverySteps = "1. Check for running bd processes: ps aux | grep bd\n" + + "2. Kill any stale processes\n" + + "3. Remove stale locks: rm .beads/beads.db-shm .beads/beads.db-wal .beads/daemon.lock\n" + + "4. Retry: bd doctor --fix" + + case strings.Contains(errMsg, "not a database") || strings.Contains(errMsg, "file is not a database"): + errorType = "File is not a valid SQLite database" + if jsonlAvailable { + recoverySteps = fmt.Sprintf("Database file is corrupted beyond repair.\n\n"+ + "Recovery steps:\n"+ + "1. Backup corrupt database: mv .beads/beads.db .beads/beads.db.broken\n"+ + "2. Rebuild from JSONL (%d issues): bd doctor --fix --force --source=jsonl\n"+ + "3. Verify: bd stats", jsonlCount) + } else { + recoverySteps = "Database file is corrupted and no JSONL backup found.\n" + + "Manual recovery required:\n" + + "1. Restore from git: git checkout HEAD -- .beads/issues.jsonl\n" + + "2. Rebuild database: bd doctor --fix --force" + } + + case strings.Contains(errMsg, "migration") || strings.Contains(errMsg, "validation failed"): + errorType = "Database migration or validation failed" + if jsonlAvailable { + recoverySteps = fmt.Sprintf("Database has validation errors (possibly orphaned dependencies).\n\n"+ + "Recovery steps:\n"+ + "1. Backup database: mv .beads/beads.db .beads/beads.db.broken\n"+ + "2. Rebuild from JSONL (%d issues): bd doctor --fix --force --source=jsonl\n"+ + "3. Verify: bd stats\n\n"+ + "Alternative: bd doctor --fix --force (attempts to repair in-place)", jsonlCount) + } else { + recoverySteps = "Database validation failed and no JSONL backup available.\n" + + "Try: bd doctor --fix --force" + } + + default: + errorType = "Failed to open database" + if jsonlAvailable { + recoverySteps = fmt.Sprintf("Run 'bd doctor --fix --source=jsonl' to rebuild from JSONL (%d issues)", jsonlCount) + } else { + recoverySteps = "Run 'bd doctor --fix --force' to attempt recovery" + } + } + return +} + // getDatabaseVersionFromPath reads the database version from the given path func getDatabaseVersionFromPath(dbPath string) string { db, err := sql.Open("sqlite3", sqliteConnString(dbPath, true)) diff --git a/cmd/bd/doctor/fix/recovery.go b/cmd/bd/doctor/fix/recovery.go index 93ca51f8..4afe329d 100644 --- a/cmd/bd/doctor/fix/recovery.go +++ b/cmd/bd/doctor/fix/recovery.go @@ -108,6 +108,11 @@ func DatabaseCorruptionRecoveryWithOptions(path string, force bool, source strin jsonlPath := findJSONLPath(beadsDir) jsonlExists := jsonlPath != "" + // Check for contradictory flags early + if force && source == "db" { + return fmt.Errorf("--force and --source=db are contradictory: --force implies the database is broken and recovery should use JSONL. Use --source=jsonl or --source=auto with --force") + } + // Determine source of truth based on --source flag and availability var useJSONL bool switch source { @@ -117,17 +122,28 @@ func DatabaseCorruptionRecoveryWithOptions(path string, force bool, source strin return fmt.Errorf("--source=jsonl specified but no JSONL file found") } useJSONL = true - fmt.Println(" Using JSONL as source of truth (--source=jsonl)") + if force { + fmt.Println(" Using JSONL as source of truth (--force --source=jsonl)") + } else { + fmt.Println(" Using JSONL as source of truth (--source=jsonl)") + } case "db": - // Explicit database preference + // Explicit database preference (already checked for force+db contradiction above) if !dbExists { return fmt.Errorf("--source=db specified but no database found") } useJSONL = false fmt.Println(" Using database as source of truth (--source=db)") case "auto": - // Auto-detect: prefer JSONL if database is corrupted, otherwise prefer database - if !dbExists && jsonlExists { + // Auto-detect: prefer JSONL if database is corrupted or force is set + if force { + // Force mode implies database is broken - use JSONL + if !jsonlExists { + return fmt.Errorf("--force requires JSONL for recovery but no JSONL file found") + } + useJSONL = true + fmt.Println(" Using JSONL as source of truth (--force mode)") + } else if !dbExists && jsonlExists { useJSONL = true fmt.Println(" Using JSONL as source of truth (database missing)") } else if dbExists && !jsonlExists { @@ -144,12 +160,6 @@ func DatabaseCorruptionRecoveryWithOptions(path string, force bool, source strin return fmt.Errorf("invalid --source value: %s (valid values: auto, jsonl, db)", source) } - // If force mode is enabled, always use JSONL recovery (bypass database validation) - if force { - useJSONL = true - fmt.Println(" Force mode enabled: bypassing database validation, using JSONL recovery") - } - // If using database as source, just run migration (no recovery needed) if !useJSONL { fmt.Println(" Database is the source of truth - skipping recovery")