fix: Code review fixes for auto-migration (bd-jgxi)

Critical fixes from code review:

1. **Moved auto-migration to correct location**
   - Now runs AFTER daemon check but BEFORE opening database
   - Prevents: database opened twice, conflicts with daemon
   - Was: Running too early, before knowing if daemon exists

2. **Fixed context cancellation issue**
   - Check if rootCtx is canceled before using it
   - Fall back to Background() if canceled
   - Fixes: "context canceled" errors in test suite

3. **Updated function signature**
   - Takes dbPath as parameter (no longer searches for it)
   - Simpler, more explicit, easier to test
   - Caller already has dbPath, no need to re-discover

4. **Enhanced test reliability**
   - Save/restore all global state
   - Add debug logging for troubleshooting
   - Verify preconditions before migration

Changes:
- cmd/bd/main.go: Move autoMigrateOnVersionBump call to correct location
- cmd/bd/version_tracking.go: Fix context handling, update signature
- cmd/bd/version_tracking_test.go: Improve test reliability

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Steve Yegge
2025-11-23 18:28:01 -08:00
parent 49db20c594
commit 4a9d6e6dd7
4 changed files with 725 additions and 754 deletions

View File

@@ -119,33 +119,24 @@ func maybeShowUpgradeNotification() {
// autoMigrateOnVersionBump automatically migrates the database when CLI version changes.
// This function is best-effort - failures are silent to avoid disrupting commands.
// Called from PersistentPreRun after trackBdVersion().
// Called from PersistentPreRun after daemon check but before opening DB for main operation.
//
// IMPORTANT: This must be called AFTER determining we're in direct mode (no daemon)
// and BEFORE opening the database, to avoid: 1) conflicts with daemon, 2) opening DB twice.
//
// bd-jgxi: Auto-migrate database on CLI version bump
func autoMigrateOnVersionBump() {
func autoMigrateOnVersionBump(dbPath string) {
// Only migrate if version upgrade was detected
if !versionUpgradeDetected {
return
}
// Find the beads directory
beadsDir := beads.FindBeadsDir()
if beadsDir == "" {
// No .beads directory - nothing to migrate
// Validate dbPath
if dbPath == "" {
debug.Logf("auto-migrate: skipping migration, no database path")
return
}
// Load config to get database path
cfg, err := configfile.Load(beadsDir)
if err != nil || cfg == nil {
// Config load failed or doesn't exist - skip migration
debug.Logf("auto-migrate: skipping migration, config load failed: %v", err)
return
}
// Get database path
dbPath := cfg.DatabasePath(beadsDir)
// Check if database exists
if _, err := os.Stat(dbPath); os.IsNotExist(err) {
// No database file - nothing to migrate
@@ -154,7 +145,13 @@ func autoMigrateOnVersionBump() {
}
// Open database to check current version
ctx := context.Background()
// Use rootCtx if available and not canceled, otherwise use Background
ctx := rootCtx
if ctx == nil || ctx.Err() != nil {
// rootCtx is nil or canceled - use fresh background context
ctx = context.Background()
}
store, err := sqlite.New(ctx, dbPath)
if err != nil {
// Failed to open database - skip migration