Merge branch 'pr-677'

2025-12-21 12:42:11 -08:00
parent 9a10753cff 9db8efaa5c
commit f1e4c4f4a5
5 changed files with 647 additions and 362 deletions
--- a/cmd/bd/doctor/database.go
+++ b/cmd/bd/doctor/database.go
@@ -425,11 +425,25 @@ func CheckDatabaseJSONLSync(path string) DoctorCheck {

 	// If we found issues, report them
 	if len(issues) > 0 {
+		// Provide direction-specific guidance
+		var fixMsg string
+		if dbCount > jsonlCount {
+			fixMsg = "Run 'bd doctor --fix' to automatically export DB to JSONL, or manually run 'bd export'"
+		} else if jsonlCount > dbCount {
+			fixMsg = "Run 'bd doctor --fix' to automatically import JSONL to DB, or manually run 'bd sync --import-only'"
+		} else {
+			// Equal counts but other issues (like prefix mismatch)
+			fixMsg = "Run 'bd doctor --fix' to fix automatically, or manually run 'bd sync --import-only' or 'bd export' depending on which has newer data"
+		}
+		if strings.Contains(strings.Join(issues, " "), "Prefix mismatch") {
+			fixMsg = "Run 'bd import -i " + filepath.Base(jsonlPath) + " --rename-on-import' to fix prefixes"
+		}
+
 		return DoctorCheck{
 			Name:    "DB-JSONL Sync",
 			Status:  StatusWarning,
 			Message: strings.Join(issues, "; "),
-			Fix:     "Run 'bd sync --import-only' to import JSONL updates or 'bd import -i issues.jsonl --rename-on-import' to fix prefixes",
+			Fix:     fixMsg,
 		}
 	}

--- a/cmd/bd/doctor/fix/docs.md
+++ b/cmd/bd/doctor/fix/docs.md
@@ -8,16 +8,53 @@ The `cmd/bd/doctor/fix` directory contains automated remediation functions for i

 ### How it fits into the larger codebase

- **Integration with Doctor Detection**: The `@/cmd/bd/doctor.go` command runs checks to identify workspace problems, then calls functions from this package when `--fix` flag is used. The doctor command is the orchestrator that determines which issues exist and which fixes to apply.
+- **Integration with Doctor Detection**: The `@/cmd/bd/doctor.go` command runs checks to identify workspace problems, then calls functions from this package when `--fix` flag is used. The `CheckDatabaseJSONLSync()` function in `@/cmd/bd/doctor/database.go` (lines 299-486) detects sync issues and provides direction-specific guidance about which fix to run. When DB count differs from JSONL count, it now recommends `bd doctor --fix` to run `DBJSONLSync()` with the appropriate direction.

- **Dependency on Core Libraries**: The fix functions use core libraries like `@/internal/deletions` (for reading/writing deletion manifests), `@/internal/types` (for issue data structures), and git operations via `exec.Command`.
+- **Dependency on Core Libraries**: The fix functions use core libraries like `@/internal/deletions` (for reading/writing deletion manifests), `@/internal/types` (for issue data structures), `@/internal/configfile` (for database path resolution), and git operations via `exec.Command`.

- **Data Persistence Points**: Each fix module directly modifies persistent workspace state: deletions manifest, database files, JSONL files, and git branch configuration. Changes are written to disk and persisted in the git repository.
+- **Data Persistence Points**: Each fix module directly modifies persistent workspace state: deletions manifest, database files, JSONL files, and git branch configuration. Changes are written to disk and persisted in the git repository. The sync fix is unique in that it delegates persistence to `bd export` or `bd sync --import-only` commands.

 - **Deletion Tracking Architecture**: The deletions manifest (`@/internal/deletions/deletions.go`) is an append-only log tracking issue deletions. The fix in `deletions.go` is critical to maintaining the integrity of this log by preventing tombstones from being incorrectly re-added to it after `bd migrate-tombstones` runs.

 - **Tombstone System**: The fix works in concert with the tombstone system (`@/internal/types/types.go` - `Status == StatusTombstone`). Tombstones represent soft-deleted issues that contain deletion metadata. The fix prevents tombstones from being confused with actively deleted issues during deletion hydration.

+- **Database Configuration Management**: The sync fix uses `@/internal/configfile.Load()` to support both canonical and custom database paths, enabling workspaces with non-standard database locations (via `metadata.json`) to be synced correctly.
+
+**Database-JSONL Sync** (`sync.go`):
+
+The `DBJSONLSync()` function fixes synchronization issues between the SQLite database and JSONL export files by detecting data direction and running the appropriate sync command:
+
+1. **Bidirectional Detection** (lines 23-97):
+   - Counts issues in both database (via SQL query) and JSONL file (via line-by-line JSON parsing)
+   - Determines sync direction based on issue counts:
+     - If `dbCount > jsonlCount`: DB has newer data → runs `bd export` to sync JSONL
+     - If `jsonlCount > dbCount`: JSONL has newer data → runs `bd sync --import-only` to import
+     - If counts equal but timestamps differ: Uses file modification times to decide direction
+   - This replaces the previous unidirectional approach that could leave users stuck when DB was the source of truth
+
+2. **Database Path Resolution** (lines 32-37):
+   - Uses `configfile.Load()` to check for custom database paths in `metadata.json`
+   - Falls back to canonical database name (`beads.CanonicalDatabaseName`)
+   - Supports both current and legacy database configurations
+
+3. **JSONL File Discovery** (lines 39-48):
+   - Checks for both canonical (`issues.jsonl`) and legacy (`beads.jsonl`) JSONL file names
+   - Supports workspaces that migrated from one naming convention to another
+   - Returns early if either database or JSONL is missing (nothing to sync)
+
+4. **Helper Functions**:
+   - `countDatabaseIssues()` (lines 124-139): Opens SQLite database and queries `COUNT(*) FROM issues`
+   - `countJSONLIssues()` (lines 141-174): Iterates through JSONL file line-by-line, parsing JSON and counting valid issues with IDs. Skips malformed JSON lines gracefully.
+
+5. **Command Execution** (lines 106-120):
+   - Gets bd binary path safely via `getBdBinary()` to prevent fork bombs in tests
+   - Executes `bd export` or `bd sync --import-only` with workspace directory as working directory
+   - Streams stdout/stderr to user for visibility
+
+**Problem Solved (bd-68e4)**:
+
+Previously, when the database contained more issues than the JSONL export, the doctor would recommend `bd sync --import-only`, which imports JSONL into DB. Since JSONL hadn't changed and the database had newer data, this command was a no-op (0 created, 0 updated), leaving users unable to sync their JSONL file with the database. The bidirectional detection now recognizes this case and runs `bd export` instead.
+
 ### Core Implementation

 **Deletions Manifest Hydration** (`deletions.go`):
@@ -52,6 +89,22 @@ The `cmd/bd/doctor/fix` directory contains automated remediation functions for i
   - Prefix must be alphanumeric with underscores, suffix must be base36 hash or number with optional dots for child issues
   - Used to filter out false positives when parsing JSON

+**Test Coverage** (`fix_test.go`):
+
+The test file includes comprehensive coverage for the sync functionality:
+
+- **TestCountJSONLIssues**: Tests the `countJSONLIssues()` helper with:
+  - Empty JSONL files (returns 0)
+  - Valid issues in JSONL (correct count)
+  - Mixed valid and invalid JSON lines (counts only valid issues)
+  - Nonexistent files (returns error)
+
+- **TestDBJSONLSync_Validation**: Tests validation logic:
+  - Returns without error when no database exists (nothing to sync)
+  - Returns without error when no JSONL exists (nothing to sync)
+
+- **TestDBJSONLSync_MissingDatabase**: Validates graceful handling when only JSONL exists
+
 **Test Coverage** (`deletions_test.go`):

 The test file covers edge cases and validates the bd-in7q fix:
--- a/cmd/bd/doctor/fix/fix_test.go
+++ b/cmd/bd/doctor/fix/fix_test.go
@@ -628,3 +628,83 @@ func TestSyncBranchConfig_InvalidRemoteURL(t *testing.T) {
 	}
 }

+func TestCountJSONLIssues(t *testing.T) {
+	t.Parallel()
+
+	t.Run("empty_JSONL", func(t *testing.T) {
+		dir := setupTestWorkspace(t)
+		jsonlPath := filepath.Join(dir, ".beads", "issues.jsonl")
+
+		// Create empty JSONL
+		if err := os.WriteFile(jsonlPath, []byte(""), 0644); err != nil {
+			t.Fatalf("failed to create JSONL: %v", err)
+		}
+
+		count, err := countJSONLIssues(jsonlPath)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if count != 0 {
+			t.Errorf("expected 0, got %d", count)
+		}
+	})
+
+	t.Run("valid_issues", func(t *testing.T) {
+		dir := setupTestWorkspace(t)
+		jsonlPath := filepath.Join(dir, ".beads", "issues.jsonl")
+
+		// Create JSONL with 3 issues
+		jsonl := []byte(`{"id":"bd-1","title":"First"}
+{"id":"bd-2","title":"Second"}
+{"id":"bd-3","title":"Third"}
+`)
+		if err := os.WriteFile(jsonlPath, jsonl, 0644); err != nil {
+			t.Fatalf("failed to create JSONL: %v", err)
+		}
+
+		count, err := countJSONLIssues(jsonlPath)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if count != 3 {
+			t.Errorf("expected 3, got %d", count)
+		}
+	})
+
+	t.Run("mixed_valid_and_invalid", func(t *testing.T) {
+		dir := setupTestWorkspace(t)
+		jsonlPath := filepath.Join(dir, ".beads", "issues.jsonl")
+
+		// Create JSONL with 2 valid and some invalid lines
+		jsonl := []byte(`{"id":"bd-1","title":"First"}
+invalid json line
+{"id":"bd-2","title":"Second"}
+{"title":"No ID"}
+`)
+		if err := os.WriteFile(jsonlPath, jsonl, 0644); err != nil {
+			t.Fatalf("failed to create JSONL: %v", err)
+		}
+
+		count, err := countJSONLIssues(jsonlPath)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if count != 2 {
+			t.Errorf("expected 2, got %d", count)
+		}
+	})
+
+	t.Run("nonexistent_file", func(t *testing.T) {
+		dir := setupTestWorkspace(t)
+		jsonlPath := filepath.Join(dir, ".beads", "nonexistent.jsonl")
+
+		count, err := countJSONLIssues(jsonlPath)
+		if err == nil {
+			t.Error("expected error for nonexistent file")
+		}
+		if count != 0 {
+			t.Errorf("expected 0, got %d", count)
+		}
+	})
+}
+
--- a/cmd/bd/doctor/fix/sync.go
+++ b/cmd/bd/doctor/fix/sync.go
@@ -1,13 +1,25 @@
 package fix

 import (
+	"bufio"
+	"database/sql"
+	"encoding/json"
 	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
+
+	_ "github.com/ncruces/go-sqlite3/driver"
+	_ "github.com/ncruces/go-sqlite3/embed"
+	"github.com/steveyegge/beads/internal/beads"
+	"github.com/steveyegge/beads/internal/configfile"
 )

-// DBJSONLSync fixes database-JSONL sync issues by running bd sync --import-only
+// DBJSONLSync fixes database-JSONL sync issues by running the appropriate sync command.
+// It detects which has more data and runs the correct direction:
+// - If DB > JSONL: Run 'bd export' (DB→JSONL)
+// - If JSONL > DB: Run 'bd sync --import-only' (JSONL→DB)
+// - If equal but timestamps differ: Use file mtime to decide
 func DBJSONLSync(path string) error {
 	// Validate workspace
 	if err := validateBeadsWorkspace(path); err != nil {
@@ -16,26 +28,72 @@ func DBJSONLSync(path string) error {

 	beadsDir := filepath.Join(path, ".beads")

+	// Get database path (same logic as doctor package)
+	var dbPath string
+	if cfg, err := configfile.Load(beadsDir); err == nil && cfg != nil && cfg.Database != "" {
+		dbPath = cfg.DatabasePath(beadsDir)
+	} else {
+		dbPath = filepath.Join(beadsDir, beads.CanonicalDatabaseName)
+	}
+
+	// Find JSONL file
+	var jsonlPath string
+	issuesJSONL := filepath.Join(beadsDir, "issues.jsonl")
+	beadsJSONL := filepath.Join(beadsDir, "beads.jsonl")
+
+	if _, err := os.Stat(issuesJSONL); err == nil {
+		jsonlPath = issuesJSONL
+	} else if _, err := os.Stat(beadsJSONL); err == nil {
+		jsonlPath = beadsJSONL
+	}
+
 	// Check if both database and JSONL exist
-	dbPath := filepath.Join(beadsDir, "beads.db")
-	jsonlPath := filepath.Join(beadsDir, "issues.jsonl")
-	beadsJSONLPath := filepath.Join(beadsDir, "beads.jsonl")
-
-	hasDB := false
-	if _, err := os.Stat(dbPath); err == nil {
-		hasDB = true
+	if _, err := os.Stat(dbPath); os.IsNotExist(err) {
+		return nil // No database, nothing to sync
+	}
+	if jsonlPath == "" {
+		return nil // No JSONL, nothing to sync
 	}

-	hasJSONL := false
-	if _, err := os.Stat(jsonlPath); err == nil {
-		hasJSONL = true
-	} else if _, err := os.Stat(beadsJSONLPath); err == nil {
-		hasJSONL = true
+	// Count issues in both
+	dbCount, err := countDatabaseIssues(dbPath)
+	if err != nil {
+		return fmt.Errorf("failed to count database issues: %w", err)
 	}

-	if !hasDB || !hasJSONL {
-		// Nothing to sync
-		return nil
+	jsonlCount, err := countJSONLIssues(jsonlPath)
+	if err != nil {
+		return fmt.Errorf("failed to count JSONL issues: %w", err)
+	}
+
+	// Determine sync direction
+	var syncDirection string
+
+	if dbCount == jsonlCount {
+		// Counts are equal, use file modification times to decide
+		dbInfo, err := os.Stat(dbPath)
+		if err != nil {
+			return fmt.Errorf("failed to stat database: %w", err)
+		}
+
+		jsonlInfo, err := os.Stat(jsonlPath)
+		if err != nil {
+			return fmt.Errorf("failed to stat JSONL: %w", err)
+		}
+
+		if dbInfo.ModTime().After(jsonlInfo.ModTime()) {
+			// DB was modified after JSONL → export to update JSONL
+			syncDirection = "export"
+		} else {
+			// JSONL was modified after DB → import to update DB
+			syncDirection = "import"
+		}
+	} else if dbCount > jsonlCount {
+		// DB has more issues → export to sync JSONL
+		syncDirection = "export"
+	} else {
+		// JSONL has more issues → import to sync DB
+		syncDirection = "import"
 	}

 	// Get bd binary path
@@ -44,9 +102,15 @@ func DBJSONLSync(path string) error {
 		return err
 	}

-	// Run bd sync --import-only to import JSONL updates
-	cmd := exec.Command(bdBinary, "sync", "--import-only") // #nosec G204 -- bdBinary from validated executable path
-	cmd.Dir = path                                          // Set working directory without changing process dir
+	// Run the appropriate sync command
+	var cmd *exec.Cmd
+	if syncDirection == "export" {
+		cmd = exec.Command(bdBinary, "export") // #nosec G204 -- bdBinary from validated executable path
+	} else {
+		cmd = exec.Command(bdBinary, "sync", "--import-only") // #nosec G204 -- bdBinary from validated executable path
+	}
+
+	cmd.Dir = path // Set working directory without changing process dir
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr

@@ -56,3 +120,56 @@ func DBJSONLSync(path string) error {

 	return nil
 }
+
+// countDatabaseIssues counts the number of issues in the database.
+func countDatabaseIssues(dbPath string) (int, error) {
+	db, err := sql.Open("sqlite3", dbPath)
+	if err != nil {
+		return 0, fmt.Errorf("failed to open database: %w", err)
+	}
+	defer db.Close()
+
+	var count int
+	err = db.QueryRow("SELECT COUNT(*) FROM issues").Scan(&count)
+	if err != nil {
+		return 0, fmt.Errorf("failed to query database: %w", err)
+	}
+
+	return count, nil
+}
+
+// countJSONLIssues counts the number of valid issues in a JSONL file.
+// Returns only the count (doesn't need prefixes for sync direction decision).
+func countJSONLIssues(jsonlPath string) (int, error) {
+	// jsonlPath is safe: constructed from filepath.Join(beadsDir, hardcoded name)
+	file, err := os.Open(jsonlPath) //nolint:gosec
+	if err != nil {
+		return 0, fmt.Errorf("failed to open JSONL file: %w", err)
+	}
+	defer file.Close()
+
+	count := 0
+	scanner := bufio.NewScanner(file)
+	for scanner.Scan() {
+		line := scanner.Bytes()
+		if len(line) == 0 {
+			continue
+		}
+
+		// Parse JSON to get the ID
+		var issue map[string]interface{}
+		if err := json.Unmarshal(line, &issue); err != nil {
+			continue // Skip malformed lines
+		}
+
+		if id, ok := issue["id"].(string); ok && id != "" {
+			count++
+		}
+	}
+
+	if err := scanner.Err(); err != nil {
+		return count, fmt.Errorf("failed to read JSONL file: %w", err)
+	}
+
+	return count, nil
+}