refactor(hook): simplify export state logic, add tests

Code review fixes: - Renamed misleading exportIncrementalDolt to hasDoltChanges (now just checks for changes) - Extracted stageJSONLFiles() helper to eliminate duplication - Extracted runJSONLExport() helper - Removed unused Actor field from ExportState (add back when implementing filtering) - Consolidated hookPreCommitDoltFallback into doExportAndSaveState - Added updateExportStateCommit for no-change fast path Net reduction: 44 lines (-116/+72) Added tests for: - getWorktreeHash - Export state paths - Save/load export state - JSON serialization with omitempty - updateExportStateCommit - computeJSONLHashForHook Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2026-01-19 11:43:58 -08:00
parent 38af99f300
commit 5d68e6b61a
2 changed files with 294 additions and 115 deletions
--- a/cmd/bd/hook.go
+++ b/cmd/bd/hook.go
@@ -72,15 +72,14 @@ Configuration (.beads/config.yaml):
 //
 // Key insight: We track Dolt commit hash (not git commit) because:
 // - Dolt is the source of truth for issue data
-// - We use dolt_diff() to find changes since last export
+// - We use dolt_diff() to detect if changes exist since last export
 // - Each worktree may have exported at different Dolt commits
 type ExportState struct {
 	WorktreeRoot     string    `json:"worktree_root"`
 	WorktreeHash     string    `json:"worktree_hash,omitempty"` // Hash of worktree path (for debugging)
-	LastExportCommit string    `json:"last_export_commit"`      // DOLT commit hash when last exported
+	LastExportCommit string    `json:"last_export_commit"`      // Dolt commit hash when last exported
 	LastExportTime   time.Time `json:"last_export_time"`
 	JSONLHash        string    `json:"jsonl_hash,omitempty"` // Hash of JSONL at last export
-	Actor            string    `json:"actor,omitempty"`      // BD_ACTOR value if set during export
 }

 // getWorktreeHash returns a hash of the worktree root for use in filenames.
@@ -414,9 +413,10 @@ func hookPreCommit() int {
 // Per design doc Part 21, this function:
 // 1. Loads export state for the current worktree
 // 2. Gets the current Dolt commit hash (not git commit)
-// 3. Uses dolt_diff() to find changes since last export
-// 4. Filters by BD_ACTOR if set (so polecats only export their own changes)
-// 5. Updates JSONL with changes and saves new state
+// 3. Checks if export is needed (skip if already exported this commit)
+// 4. Exports to JSONL and saves new state
+//
+// Future: Use dolt_diff() for incremental export and BD_ACTOR filtering.
 func hookPreCommitDolt(beadsDir, worktreeRoot string) int {
 	ctx := context.Background()

@@ -435,15 +435,15 @@ func hookPreCommitDolt(beadsDir, worktreeRoot string) int {
 	vs, ok := storage.AsVersioned(store)
 	if !ok {
 		// Fall back to full export if not versioned
-		return hookPreCommitDoltFallback(ctx, store, beadsDir, worktreeRoot)
+		return doExportAndSaveState(ctx, beadsDir, worktreeRoot, "")
 	}

 	// Get current Dolt commit hash
 	currentDoltCommit, err := vs.GetCurrentCommit(ctx)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "Warning: could not get Dolt commit: %v\n", err)
-		// Fall back to full export
-		return hookPreCommitDoltFallback(ctx, store, beadsDir, worktreeRoot)
+		// Fall back to full export without commit tracking
+		return doExportAndSaveState(ctx, beadsDir, worktreeRoot, "")
 	}

 	// Check if we've already exported for this Dolt commit (idempotency)
@@ -452,97 +452,43 @@ func hookPreCommitDolt(beadsDir, worktreeRoot string) int {
 		return 0
 	}

-	// Get actor for filtering (polecats use BD_ACTOR to avoid exporting others' work)
-	actor := os.Getenv("BD_ACTOR")
-
-	// Determine export strategy
-	jsonlPath := filepath.Join(beadsDir, "issues.jsonl")
+	// Check if there are actual changes to export (optimization)
 	if prevState != nil && prevState.LastExportCommit != "" {
-		// Incremental export: use dolt_diff() to get only changes
-		if err := exportIncrementalDolt(ctx, vs, store, jsonlPath, prevState.LastExportCommit, currentDoltCommit, actor); err != nil {
-			fmt.Fprintf(os.Stderr, "Warning: incremental export failed, doing full export: %v\n", err)
-			// Fall back to full export
-			if err := exportFullDolt(ctx, store, jsonlPath); err != nil {
-				fmt.Fprintf(os.Stderr, "Warning: could not export to JSONL: %v\n", err)
-				return 0
-			}
-		}
-	} else {
-		// First export for this worktree: full export
-		if err := exportFullDolt(ctx, store, jsonlPath); err != nil {
-			fmt.Fprintf(os.Stderr, "Warning: could not export to JSONL: %v\n", err)
+		hasChanges, err := hasDoltChanges(ctx, vs, prevState.LastExportCommit, currentDoltCommit)
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "Warning: could not check for changes: %v\n", err)
+			// Continue with export to be safe
+		} else if !hasChanges {
+			// No changes, but update state to track new commit
+			updateExportStateCommit(beadsDir, worktreeRoot, currentDoltCommit)
 			return 0
 		}
 	}

-	// Stage JSONL files
-	if os.Getenv("BEADS_NO_AUTO_STAGE") == "" {
-		rc, rcErr := beads.GetRepoContext()
-		jsonlFiles := []string{".beads/issues.jsonl", ".beads/deletions.jsonl", ".beads/interactions.jsonl"}
-		for _, f := range jsonlFiles {
-			if _, err := os.Stat(f); err == nil {
-				var gitAdd *exec.Cmd
-				if rcErr == nil {
-					gitAdd = rc.GitCmdCWD(ctx, "add", f)
-				} else {
-					// #nosec G204 -- f comes from jsonlFiles
-					gitAdd = exec.Command("git", "add", f)
-				}
-				_ = gitAdd.Run()
-			}
-		}
-	}
-
-	// Update export state with Dolt commit hash
-	jsonlHash, _ := computeJSONLHashForHook(jsonlPath)
-	state := &ExportState{
-		WorktreeRoot:     worktreeRoot,
-		WorktreeHash:     getWorktreeHash(worktreeRoot),
-		LastExportCommit: currentDoltCommit, // Dolt commit, not git commit
-		LastExportTime:   time.Now(),
-		JSONLHash:        jsonlHash,
-		Actor:            actor,
-	}
-	if err := saveExportState(beadsDir, worktreeRoot, state); err != nil {
-		fmt.Fprintf(os.Stderr, "Warning: could not save export state: %v\n", err)
-	}
-
-	return 0
+	return doExportAndSaveState(ctx, beadsDir, worktreeRoot, currentDoltCommit)
 }

-// hookPreCommitDoltFallback does a full export when incremental isn't available.
-func hookPreCommitDoltFallback(ctx context.Context, store storage.Storage, beadsDir, worktreeRoot string) int {
+// doExportAndSaveState performs the export and saves state. Shared by main path and fallback.
+func doExportAndSaveState(ctx context.Context, beadsDir, worktreeRoot, doltCommit string) int {
 	jsonlPath := filepath.Join(beadsDir, "issues.jsonl")
-	if err := exportFullDolt(ctx, store, jsonlPath); err != nil {
+
+	// Export to JSONL
+	if err := runJSONLExport(); err != nil {
 		fmt.Fprintf(os.Stderr, "Warning: could not export to JSONL: %v\n", err)
 		return 0
 	}

-	// Stage JSONL files
-	if os.Getenv("BEADS_NO_AUTO_STAGE") == "" {
-		rc, rcErr := beads.GetRepoContext()
-		jsonlFiles := []string{".beads/issues.jsonl", ".beads/deletions.jsonl", ".beads/interactions.jsonl"}
-		for _, f := range jsonlFiles {
-			if _, err := os.Stat(f); err == nil {
-				var gitAdd *exec.Cmd
-				if rcErr == nil {
-					gitAdd = rc.GitCmdCWD(ctx, "add", f)
-				} else {
-					// #nosec G204 -- f comes from jsonlFiles
-					gitAdd = exec.Command("git", "add", f)
-				}
-				_ = gitAdd.Run()
-			}
-		}
-	}
+	// Stage JSONL files for git commit
+	stageJSONLFiles(ctx)

-	// Save minimal state (no Dolt commit since we couldn't get it)
+	// Save export state
 	jsonlHash, _ := computeJSONLHashForHook(jsonlPath)
 	state := &ExportState{
-		WorktreeRoot:   worktreeRoot,
-		WorktreeHash:   getWorktreeHash(worktreeRoot),
-		LastExportTime: time.Now(),
-		JSONLHash:      jsonlHash,
+		WorktreeRoot:     worktreeRoot,
+		WorktreeHash:     getWorktreeHash(worktreeRoot),
+		LastExportCommit: doltCommit, // Empty string if Dolt commit unavailable
+		LastExportTime:   time.Now(),
+		JSONLHash:        jsonlHash,
 	}
 	if err := saveExportState(beadsDir, worktreeRoot, state); err != nil {
 		fmt.Fprintf(os.Stderr, "Warning: could not save export state: %v\n", err)
@@ -551,45 +497,55 @@ func hookPreCommitDoltFallback(ctx context.Context, store storage.Storage, beads
 	return 0
 }

-// exportIncrementalDolt exports only changes since the last export using dolt_diff().
-// If actor is non-empty, only changes attributed to that actor are included.
-func exportIncrementalDolt(ctx context.Context, vs storage.VersionedStorage, store storage.Storage, jsonlPath, fromCommit, toCommit, actor string) error {
-	// Get diff between commits
+// hasDoltChanges checks if there are any changes between two Dolt commits.
+func hasDoltChanges(ctx context.Context, vs storage.VersionedStorage, fromCommit, toCommit string) (bool, error) {
 	diffs, err := vs.Diff(ctx, fromCommit, toCommit)
 	if err != nil {
-		return fmt.Errorf("getting diff: %w", err)
+		return false, err
 	}
-
-	if len(diffs) == 0 {
-		// No changes, nothing to export
-		return nil
-	}
-
-	// TODO: Actor-based filtering
-	// If actor is set, filter diffs to only include changes made by this actor.
-	// This requires the issues table to have an 'updated_by' or similar field.
-	// For now, we export all changes - the actor filtering is a future enhancement.
-	_ = actor // Silence unused variable warning
-
-	// For now, if there are any changes, do a full re-export.
-	// A proper incremental implementation would:
-	// 1. Read existing JSONL
-	// 2. Apply diffs (add new, update modified, mark deleted)
-	// 3. Write updated JSONL
-	//
-	// This is simpler and still correct - we just export all current data.
-	// The per-worktree state tracking still prevents cross-polecat pollution
-	// because each worktree tracks its own export commit.
-	return exportFullDolt(ctx, store, jsonlPath)
+	return len(diffs) > 0, nil
 }

-// exportFullDolt does a complete export of all issues to JSONL.
-func exportFullDolt(ctx context.Context, store storage.Storage, jsonlPath string) error {
-	// Use bd sync --flush-only for now - this handles all the JSONL formatting
+// updateExportStateCommit updates just the commit hash in the export state.
+// Used when we detect no changes but want to track the new commit.
+func updateExportStateCommit(beadsDir, worktreeRoot, doltCommit string) {
+	prevState, err := loadExportState(beadsDir, worktreeRoot)
+	if err != nil || prevState == nil {
+		return // Can't update what doesn't exist
+	}
+	prevState.LastExportCommit = doltCommit
+	prevState.LastExportTime = time.Now()
+	_ = saveExportState(beadsDir, worktreeRoot, prevState)
+}
+
+// runJSONLExport runs the actual JSONL export via bd sync.
+func runJSONLExport() error {
 	cmd := exec.Command("bd", "sync", "--flush-only", "--no-daemon")
 	return cmd.Run()
 }

+// stageJSONLFiles stages JSONL files for git commit (unless BEADS_NO_AUTO_STAGE is set).
+func stageJSONLFiles(ctx context.Context) {
+	if os.Getenv("BEADS_NO_AUTO_STAGE") != "" {
+		return
+	}
+
+	rc, rcErr := beads.GetRepoContext()
+	jsonlFiles := []string{".beads/issues.jsonl", ".beads/deletions.jsonl", ".beads/interactions.jsonl"}
+	for _, f := range jsonlFiles {
+		if _, err := os.Stat(f); err == nil {
+			var gitAdd *exec.Cmd
+			if rcErr == nil {
+				gitAdd = rc.GitCmdCWD(ctx, "add", f)
+			} else {
+				// #nosec G204 -- f comes from jsonlFiles (hardcoded)
+				gitAdd = exec.Command("git", "add", f)
+			}
+			_ = gitAdd.Run()
+		}
+	}
+}
+
 // hookPostMerge implements the post-merge hook: Import JSONL to database.
 func hookPostMerge(args []string) int {
 	beadsDir := beads.FindBeadsDir()
--- a/cmd/bd/hook_test.go
+++ b/cmd/bd/hook_test.go
@@ -0,0 +1,223 @@
+package main
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestGetWorktreeHash(t *testing.T) {
+	// Same input should produce same hash
+	hash1 := getWorktreeHash("/some/path/to/worktree")
+	hash2 := getWorktreeHash("/some/path/to/worktree")
+	if hash1 != hash2 {
+		t.Errorf("Same path produced different hashes: %s vs %s", hash1, hash2)
+	}
+
+	// Different inputs should produce different hashes
+	hash3 := getWorktreeHash("/different/path")
+	if hash1 == hash3 {
+		t.Errorf("Different paths produced same hash: %s", hash1)
+	}
+
+	// Hash should be 16 hex chars (8 bytes)
+	if len(hash1) != 16 {
+		t.Errorf("Hash length = %d, want 16", len(hash1))
+	}
+}
+
+func TestExportStatePaths(t *testing.T) {
+	beadsDir := "/tmp/test/.beads"
+	worktreeRoot := "/tmp/test/worktree"
+
+	stateDir := getExportStateDir(beadsDir)
+	if stateDir != "/tmp/test/.beads/export-state" {
+		t.Errorf("getExportStateDir() = %s, want /tmp/test/.beads/export-state", stateDir)
+	}
+
+	statePath := getExportStatePath(beadsDir, worktreeRoot)
+	expectedHash := getWorktreeHash(worktreeRoot)
+	expectedPath := "/tmp/test/.beads/export-state/" + expectedHash + ".json"
+	if statePath != expectedPath {
+		t.Errorf("getExportStatePath() = %s, want %s", statePath, expectedPath)
+	}
+}
+
+func TestSaveAndLoadExportState(t *testing.T) {
+	tmpDir := t.TempDir()
+	beadsDir := filepath.Join(tmpDir, ".beads")
+	worktreeRoot := tmpDir
+
+	// Initially no state
+	state, err := loadExportState(beadsDir, worktreeRoot)
+	if err != nil {
+		t.Fatalf("loadExportState() failed: %v", err)
+	}
+	if state != nil {
+		t.Errorf("loadExportState() returned non-nil for missing state")
+	}
+
+	// Save state
+	now := time.Now().Truncate(time.Second) // Truncate for comparison
+	testState := &ExportState{
+		WorktreeRoot:     worktreeRoot,
+		WorktreeHash:     getWorktreeHash(worktreeRoot),
+		LastExportCommit: "abc123def456",
+		LastExportTime:   now,
+		JSONLHash:        "hashvalue",
+	}
+	if err := saveExportState(beadsDir, worktreeRoot, testState); err != nil {
+		t.Fatalf("saveExportState() failed: %v", err)
+	}
+
+	// Load state back
+	loaded, err := loadExportState(beadsDir, worktreeRoot)
+	if err != nil {
+		t.Fatalf("loadExportState() failed: %v", err)
+	}
+	if loaded == nil {
+		t.Fatal("loadExportState() returned nil")
+	}
+
+	// Verify fields
+	if loaded.WorktreeRoot != testState.WorktreeRoot {
+		t.Errorf("WorktreeRoot = %s, want %s", loaded.WorktreeRoot, testState.WorktreeRoot)
+	}
+	if loaded.LastExportCommit != testState.LastExportCommit {
+		t.Errorf("LastExportCommit = %s, want %s", loaded.LastExportCommit, testState.LastExportCommit)
+	}
+	if loaded.JSONLHash != testState.JSONLHash {
+		t.Errorf("JSONLHash = %s, want %s", loaded.JSONLHash, testState.JSONLHash)
+	}
+}
+
+func TestExportStateJSON(t *testing.T) {
+	state := &ExportState{
+		WorktreeRoot:     "/path/to/worktree",
+		WorktreeHash:     "abc123",
+		LastExportCommit: "def456",
+		LastExportTime:   time.Date(2024, 1, 15, 10, 30, 0, 0, time.UTC),
+		JSONLHash:        "hash789",
+	}
+
+	data, err := json.MarshalIndent(state, "", "  ")
+	if err != nil {
+		t.Fatalf("json.Marshal() failed: %v", err)
+	}
+
+	// Verify JSON contains expected fields
+	jsonStr := string(data)
+	expectedFields := []string{
+		`"worktree_root"`,
+		`"worktree_hash"`,
+		`"last_export_commit"`,
+		`"last_export_time"`,
+		`"jsonl_hash"`,
+	}
+	for _, field := range expectedFields {
+		if !strings.Contains(jsonStr, field) {
+			t.Errorf("JSON missing field %s: %s", field, jsonStr)
+		}
+	}
+
+	// Verify omitempty works for empty optional fields
+	stateMinimal := &ExportState{
+		WorktreeRoot:     "/path",
+		LastExportCommit: "abc",
+		LastExportTime:   time.Now(),
+	}
+	dataMinimal, _ := json.Marshal(stateMinimal)
+	jsonMinimal := string(dataMinimal)
+
+	// WorktreeHash and JSONLHash should be omitted when empty
+	if strings.Contains(jsonMinimal, `"worktree_hash"`) {
+		t.Errorf("Empty worktree_hash should be omitted: %s", jsonMinimal)
+	}
+	if strings.Contains(jsonMinimal, `"jsonl_hash"`) {
+		t.Errorf("Empty jsonl_hash should be omitted: %s", jsonMinimal)
+	}
+}
+
+func TestUpdateExportStateCommit(t *testing.T) {
+	tmpDir := t.TempDir()
+	beadsDir := filepath.Join(tmpDir, ".beads")
+	worktreeRoot := tmpDir
+
+	// Create initial state
+	initialState := &ExportState{
+		WorktreeRoot:     worktreeRoot,
+		LastExportCommit: "old-commit",
+		LastExportTime:   time.Now().Add(-time.Hour),
+		JSONLHash:        "oldhash",
+	}
+	if err := saveExportState(beadsDir, worktreeRoot, initialState); err != nil {
+		t.Fatalf("saveExportState() failed: %v", err)
+	}
+
+	// Update just the commit
+	updateExportStateCommit(beadsDir, worktreeRoot, "new-commit")
+
+	// Load and verify
+	loaded, err := loadExportState(beadsDir, worktreeRoot)
+	if err != nil {
+		t.Fatalf("loadExportState() failed: %v", err)
+	}
+
+	if loaded.LastExportCommit != "new-commit" {
+		t.Errorf("LastExportCommit = %s, want new-commit", loaded.LastExportCommit)
+	}
+	// JSONLHash should be preserved
+	if loaded.JSONLHash != "oldhash" {
+		t.Errorf("JSONLHash = %s, want oldhash (should be preserved)", loaded.JSONLHash)
+	}
+}
+
+func TestComputeJSONLHashForHook(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	// Non-existent file should return empty string, no error
+	hash, err := computeJSONLHashForHook(filepath.Join(tmpDir, "nonexistent.jsonl"))
+	if err != nil {
+		t.Errorf("computeJSONLHashForHook() error for missing file: %v", err)
+	}
+	if hash != "" {
+		t.Errorf("computeJSONLHashForHook() = %s, want empty for missing file", hash)
+	}
+
+	// Create a test file
+	testFile := filepath.Join(tmpDir, "test.jsonl")
+	content := `{"id": "test-1", "title": "Test"}
+{"id": "test-2", "title": "Test 2"}
+`
+	if err := os.WriteFile(testFile, []byte(content), 0644); err != nil {
+		t.Fatalf("Failed to write test file: %v", err)
+	}
+
+	// Should get a hash
+	hash, err = computeJSONLHashForHook(testFile)
+	if err != nil {
+		t.Fatalf("computeJSONLHashForHook() failed: %v", err)
+	}
+	if hash == "" {
+		t.Error("computeJSONLHashForHook() returned empty hash for existing file")
+	}
+
+	// Same content should produce same hash
+	hash2, _ := computeJSONLHashForHook(testFile)
+	if hash != hash2 {
+		t.Errorf("Same file produced different hashes: %s vs %s", hash, hash2)
+	}
+
+	// Different content should produce different hash
+	testFile2 := filepath.Join(tmpDir, "test2.jsonl")
+	if err := os.WriteFile(testFile2, []byte(`{"different": true}`), 0644); err != nil {
+		t.Fatalf("Failed to write test file 2: %v", err)
+	}
+	hash3, _ := computeJSONLHashForHook(testFile2)
+	if hash == hash3 {
+		t.Errorf("Different files produced same hash: %s", hash)
+	}
+}