diff --git a/internal/syncbranch/worktree.go b/internal/syncbranch/worktree.go index b1559c6b..42adbd6c 100644 --- 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 } diff --git a/internal/syncbranch/worktree_divergence_test.go b/internal/syncbranch/worktree_divergence_test.go index d1f18b5d..65e7d559 100644 --- 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) + } + }) +}