fix: use content-level merge for divergence recovery (bd-kpy)

Replace fetchAndRebaseInWorktree with contentMergeRecovery in pushFromWorktree. The problem: When push fails due to non-fast-forward, the old code used git rebase to recover. But git rebase is text-level and does not invoke the JSONL merge driver. This could resurrect tombstones - if remote had a tombstone and local had closed, the rebase would overwrite the tombstone. The fix: Use the same content-level merge algorithm that PullFromSyncBranch uses. This respects tombstone semantics - recent tombstones always win over live issues. Changes: - Add contentMergeRecovery() that does content-level merge instead of rebase - Update pushFromWorktree to call contentMergeRecovery - Mark fetchAndRebaseInWorktree as deprecated (kept for reference) - Add tests for tombstone preservation during merge recovery 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2025-12-24 22:41:40 -08:00
parent 5a46879741
commit 2a6191e49b
2 changed files with 208 additions and 7 deletions
--- a/internal/syncbranch/worktree.go
+++ b/internal/syncbranch/worktree.go
@@ -744,7 +744,72 @@ func isNonFastForwardError(output string) bool {
 		strings.Contains(output, "rejected") && strings.Contains(output, "behind")
 }

-// fetchAndRebaseInWorktree fetches remote and rebases local commits on top
+// contentMergeRecovery performs a content-level merge when push fails due to divergence.
+// This replaces the old fetchAndRebaseInWorktree which used git rebase (text-level).
+//
+// The problem with git rebase: it replays commits textually, which can resurrect
+// tombstones. For example, if remote has a tombstone and local has 'closed',
+// the rebase overwrites the tombstone with 'closed'.
+//
+// This function uses the same content-level merge as PullFromSyncBranch:
+// 1. Fetch remote
+// 2. Find merge base
+// 3. Extract JSONL from base, local, remote
+// 4. Run 3-way content merge (respects tombstones)
+// 5. Reset to remote, commit merged content
+//
+// Fix for bd-kpy: Sync race where rebase-based divergence recovery resurrects tombstones.
+func contentMergeRecovery(ctx context.Context, worktreePath, branch, remote string) error {
+	// The JSONL is always at .beads/issues.jsonl relative to worktree
+	jsonlRelPath := filepath.Join(".beads", "issues.jsonl")
+
+	// Step 1: Fetch latest from remote
+	fetchCmd := exec.CommandContext(ctx, "git", "-C", worktreePath, "fetch", remote, branch)
+	if output, err := fetchCmd.CombinedOutput(); err != nil {
+		return fmt.Errorf("fetch failed: %w\n%s", err, output)
+	}
+
+	// Step 2: Perform content-level merge (same algorithm as PullFromSyncBranch)
+	mergedContent, err := performContentMerge(ctx, worktreePath, branch, remote, jsonlRelPath)
+	if err != nil {
+		return fmt.Errorf("content merge failed: %w", err)
+	}
+
+	// Step 3: Reset worktree to remote's history (adopt their commit graph)
+	resetCmd := exec.CommandContext(ctx, "git", "-C", worktreePath, "reset", "--hard",
+		fmt.Sprintf("%s/%s", remote, branch))
+	if output, err := resetCmd.CombinedOutput(); err != nil {
+		return fmt.Errorf("git reset failed: %w\n%s", err, output)
+	}
+
+	// Step 4: Write merged content
+	worktreeJSONLPath := filepath.Join(worktreePath, jsonlRelPath)
+	if err := os.MkdirAll(filepath.Dir(worktreeJSONLPath), 0750); err != nil {
+		return fmt.Errorf("failed to create directory: %w", err)
+	}
+	if err := os.WriteFile(worktreeJSONLPath, mergedContent, 0600); err != nil {
+		return fmt.Errorf("failed to write merged JSONL: %w", err)
+	}
+
+	// Step 5: Check if merge produced any changes from remote
+	hasChanges, err := hasChangesInWorktree(ctx, worktreePath, worktreeJSONLPath)
+	if err != nil {
+		return fmt.Errorf("failed to check for changes: %w", err)
+	}
+
+	// Step 6: Commit merged content if there are changes
+	if hasChanges {
+		message := "bd sync: merge divergent histories (content-level recovery)"
+		if err := commitInWorktree(ctx, worktreePath, jsonlRelPath, message); err != nil {
+			return fmt.Errorf("failed to commit merged content: %w", err)
+		}
+	}
+
+	return nil
+}
+
+// fetchAndRebaseInWorktree is DEPRECATED - kept for reference only.
+// Use contentMergeRecovery instead to avoid tombstone resurrection (bd-kpy).
 func fetchAndRebaseInWorktree(ctx context.Context, worktreePath, branch, remote string) error {
 	// Fetch latest from remote
 	fetchCmd := exec.CommandContext(ctx, "git", "-C", worktreePath, "fetch", remote, branch)
@@ -824,12 +889,13 @@ func pushFromWorktree(ctx context.Context, worktreePath, branch string) error {

 		// Check if this is a non-fast-forward error (concurrent push conflict)
 		if isNonFastForwardError(outputStr) {
-			// Attempt fetch + rebase to get ahead of remote
-			if rebaseErr := fetchAndRebaseInWorktree(ctx, worktreePath, branch, remote); rebaseErr != nil {
-				// Rebase failed - provide clear recovery options (bd-vckm)
+			// bd-kpy fix: Use content-level merge instead of git rebase.
+			// Git rebase is text-level and can resurrect tombstones.
+			if mergeErr := contentMergeRecovery(ctx, worktreePath, branch, remote); mergeErr != nil {
+				// Content merge failed - provide clear recovery options (bd-vckm)
 				return fmt.Errorf(`sync branch diverged and automatic recovery failed

-The sync branch '%s' has diverged from remote '%s/%s' and automatic rebase failed.
+The sync branch '%s' has diverged from remote '%s/%s' and automatic content merge failed.

 Recovery options:
  1. Reset to remote state (discard local sync changes):
@@ -845,9 +911,9 @@ Recovery options:
     bd sync

 Original error: %v
-Rebase error: %v`, branch, remote, branch, branch, lastErr, rebaseErr)
+Merge error: %v`, branch, remote, branch, branch, lastErr, mergeErr)
 			}
-			// Rebase succeeded - retry push immediately (no backoff needed)
+			// Content merge succeeded - retry push immediately (no backoff needed)
 			continue
 		}

--- a/internal/syncbranch/worktree_divergence_test.go
+++ b/internal/syncbranch/worktree_divergence_test.go
@@ -660,3 +660,138 @@ func TestIsSyncBranchSameAsCurrent(t *testing.T) {
 		}
 	})
 }
+
+// TestContentMergeRecoveryPreservesTombstones tests that contentMergeRecovery
+// uses content-level merge which properly preserves tombstones.
+// This is the fix for bd-kpy: Sync race where rebase-based divergence recovery
+// resurrects tombstones.
+func TestContentMergeRecoveryPreservesTombstones(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test in short mode")
+	}
+
+	ctx := context.Background()
+
+	t.Run("tombstone is preserved during merge recovery", func(t *testing.T) {
+		// This test simulates the race condition described in bd-kpy:
+		// 1. Repo A deletes issue (creates tombstone), pushes successfully
+		// 2. Repo B (with 'closed' status) tries to push, fails (non-fast-forward)
+		// 3. Repo B uses contentMergeRecovery
+		// 4. Verify tombstone from A is preserved, not overwritten by B's 'closed'
+
+		// Create a bare remote repo
+		remoteDir, err := os.MkdirTemp("", "bd-test-remote-*")
+		if err != nil {
+			t.Fatalf("Failed to create remote dir: %v", err)
+		}
+		defer os.RemoveAll(remoteDir)
+		runGit(t, remoteDir, "init", "--bare")
+
+		// Create local repo
+		repoDir := setupTestRepo(t)
+		defer os.RemoveAll(repoDir)
+		runGit(t, repoDir, "remote", "add", "origin", remoteDir)
+
+		syncBranch := "beads-sync"
+		jsonlPath := filepath.Join(repoDir, ".beads", "issues.jsonl")
+
+		// Create base commit: issue with status="closed"
+		runGit(t, repoDir, "checkout", "-b", syncBranch)
+		writeFile(t, jsonlPath, `{"id":"test-1","status":"closed","title":"Test Issue"}`)
+		runGit(t, repoDir, "add", ".")
+		runGit(t, repoDir, "commit", "-m", "base: issue closed")
+
+		// Push base to remote
+		runGit(t, repoDir, "push", "-u", "origin", syncBranch)
+		baseCommit := strings.TrimSpace(getGitOutput(t, repoDir, "rev-parse", "HEAD"))
+
+		// Create tombstone commit (simulating what Repo A did)
+		writeFile(t, jsonlPath, `{"id":"test-1","status":"tombstone","title":"Test Issue"}`)
+		runGit(t, repoDir, "add", ".")
+		runGit(t, repoDir, "commit", "-m", "remote: issue tombstoned")
+
+		// Push tombstone to remote (simulating Repo A's successful push)
+		runGit(t, repoDir, "push", "origin", syncBranch)
+
+		// Now simulate Repo B: reset to base and make different changes
+		runGit(t, repoDir, "reset", "--hard", baseCommit)
+		writeFile(t, jsonlPath, `{"id":"test-1","status":"closed","title":"Test Issue","notes":"local change"}`)
+		runGit(t, repoDir, "add", ".")
+		runGit(t, repoDir, "commit", "-m", "local: added notes")
+
+		// Now local has diverged from remote:
+		// - Local HEAD: status=closed with notes
+		// - Remote origin/beads-sync: status=tombstone
+		// - Common ancestor (base): status=closed
+
+		// Run contentMergeRecovery - this should use content-level merge
+		err = contentMergeRecovery(ctx, repoDir, syncBranch, "origin")
+		if err != nil {
+			t.Fatalf("contentMergeRecovery() error = %v", err)
+		}
+
+		// Read the merged content
+		mergedData, err := os.ReadFile(jsonlPath)
+		if err != nil {
+			t.Fatalf("Failed to read merged JSONL: %v", err)
+		}
+		merged := string(mergedData)
+
+		// The tombstone should be preserved!
+		// The 3-way merge should see:
+		//   base: status=closed
+		//   local: status=closed (unchanged)
+		//   remote: status=tombstone (changed)
+		// Result: status=tombstone (remote wins because it changed)
+		if !strings.Contains(merged, `"status":"tombstone"`) {
+			t.Errorf("contentMergeRecovery() did not preserve tombstone.\nGot: %s\nWant: status=tombstone", merged)
+		}
+	})
+
+	t.Run("both sides tombstone results in tombstone", func(t *testing.T) {
+		// Create a bare remote repo
+		remoteDir, err := os.MkdirTemp("", "bd-test-remote-*")
+		if err != nil {
+			t.Fatalf("Failed to create remote dir: %v", err)
+		}
+		defer os.RemoveAll(remoteDir)
+		runGit(t, remoteDir, "init", "--bare")
+
+		repoDir := setupTestRepo(t)
+		defer os.RemoveAll(repoDir)
+		runGit(t, repoDir, "remote", "add", "origin", remoteDir)
+
+		syncBranch := "beads-sync"
+		jsonlPath := filepath.Join(repoDir, ".beads", "issues.jsonl")
+
+		// Create base: issue open
+		runGit(t, repoDir, "checkout", "-b", syncBranch)
+		writeFile(t, jsonlPath, `{"id":"test-1","status":"open","title":"Test"}`)
+		runGit(t, repoDir, "add", ".")
+		runGit(t, repoDir, "commit", "-m", "base")
+		runGit(t, repoDir, "push", "-u", "origin", syncBranch)
+		baseCommit := strings.TrimSpace(getGitOutput(t, repoDir, "rev-parse", "HEAD"))
+
+		// Remote: tombstone (push to remote)
+		writeFile(t, jsonlPath, `{"id":"test-1","status":"tombstone","title":"Test"}`)
+		runGit(t, repoDir, "add", ".")
+		runGit(t, repoDir, "commit", "-m", "remote tombstone")
+		runGit(t, repoDir, "push", "origin", syncBranch)
+
+		// Local: reset and also tombstone (both deleted independently)
+		runGit(t, repoDir, "reset", "--hard", baseCommit)
+		writeFile(t, jsonlPath, `{"id":"test-1","status":"tombstone","title":"Test"}`)
+		runGit(t, repoDir, "add", ".")
+		runGit(t, repoDir, "commit", "-m", "local tombstone")
+
+		err = contentMergeRecovery(ctx, repoDir, syncBranch, "origin")
+		if err != nil {
+			t.Fatalf("contentMergeRecovery() error = %v", err)
+		}
+
+		mergedData, _ := os.ReadFile(jsonlPath)
+		if !strings.Contains(string(mergedData), `"status":"tombstone"`) {
+			t.Errorf("Expected tombstone to be preserved, got: %s", mergedData)
+		}
+	})
+}