Fix daemon auto-sync delete mutation not reflected in sync branch (#537)
Fix daemon auto-sync delete mutation not reflected in sync branch When deleting an issue with `bd delete <id> --force`, the daemon auto-sync now properly removes the deleted issue from the sync branch. **Problem:** The merge logic saw fewer local issues (due to deletion) and would re-add the deleted issue. **Solution:** Add `ForceOverwrite` option to bypass merge logic when mutations occur. Mutation-triggered exports are authoritative and should overwrite, not merge. Reviewed-by: stevey
This commit is contained in:
1220
.beads/issues.jsonl
1220
.beads/issues.jsonl
File diff suppressed because one or more lines are too long
@@ -455,7 +455,10 @@ func performExport(ctx context.Context, store storage.Storage, autoCommit, autoP
|
|||||||
// Auto-commit if enabled (skip in git-free mode)
|
// Auto-commit if enabled (skip in git-free mode)
|
||||||
if autoCommit && !skipGit {
|
if autoCommit && !skipGit {
|
||||||
// Try sync branch commit first
|
// Try sync branch commit first
|
||||||
committed, err := syncBranchCommitAndPush(exportCtx, store, autoPush, log)
|
// Use forceOverwrite=true because mutation-triggered exports (create, update, delete)
|
||||||
|
// mean the local state is authoritative and should not be merged with worktree.
|
||||||
|
// This is critical for delete mutations to be properly reflected in the sync branch.
|
||||||
|
committed, err := syncBranchCommitAndPushWithOptions(exportCtx, store, autoPush, true, log)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
log.log("Sync branch commit failed: %v", err)
|
log.log("Sync branch commit failed: %v", err)
|
||||||
return
|
return
|
||||||
|
|||||||
@@ -14,9 +14,18 @@ import (
|
|||||||
"github.com/steveyegge/beads/internal/syncbranch"
|
"github.com/steveyegge/beads/internal/syncbranch"
|
||||||
)
|
)
|
||||||
|
|
||||||
// syncBranchCommitAndPush commits JSONL to the sync branch using a worktree
|
// syncBranchCommitAndPush commits JSONL to the sync branch using a worktree.
|
||||||
// Returns true if changes were committed, false if no changes or sync.branch not configured
|
// Returns true if changes were committed, false if no changes or sync.branch not configured.
|
||||||
|
// This is a convenience wrapper that calls syncBranchCommitAndPushWithOptions with default options.
|
||||||
func syncBranchCommitAndPush(ctx context.Context, store storage.Storage, autoPush bool, log daemonLogger) (bool, error) {
|
func syncBranchCommitAndPush(ctx context.Context, store storage.Storage, autoPush bool, log daemonLogger) (bool, error) {
|
||||||
|
return syncBranchCommitAndPushWithOptions(ctx, store, autoPush, false, log)
|
||||||
|
}
|
||||||
|
|
||||||
|
// syncBranchCommitAndPushWithOptions commits JSONL to the sync branch using a worktree.
|
||||||
|
// Returns true if changes were committed, false if no changes or sync.branch not configured.
|
||||||
|
// If forceOverwrite is true, the local JSONL is copied to the worktree without merging,
|
||||||
|
// which is necessary for delete mutations to be properly reflected in the sync branch.
|
||||||
|
func syncBranchCommitAndPushWithOptions(ctx context.Context, store storage.Storage, autoPush, forceOverwrite bool, log daemonLogger) (bool, error) {
|
||||||
// Check if any remote exists (bd-biwp: support local-only repos)
|
// Check if any remote exists (bd-biwp: support local-only repos)
|
||||||
if !hasGitRemote(ctx) {
|
if !hasGitRemote(ctx) {
|
||||||
return true, nil // Skip sync branch commit/push in local-only mode
|
return true, nil // Skip sync branch commit/push in local-only mode
|
||||||
@@ -77,7 +86,12 @@ func syncBranchCommitAndPush(ctx context.Context, store storage.Storage, autoPus
|
|||||||
return false, fmt.Errorf("failed to get relative JSONL path: %w", err)
|
return false, fmt.Errorf("failed to get relative JSONL path: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := wtMgr.SyncJSONLToWorktree(worktreePath, jsonlRelPath); err != nil {
|
// Use SyncJSONLToWorktreeWithOptions to pass forceOverwrite flag.
|
||||||
|
// When forceOverwrite is true (mutation-triggered sync, especially delete),
|
||||||
|
// the local JSONL is copied directly without merging, ensuring deletions
|
||||||
|
// are properly reflected in the sync branch.
|
||||||
|
syncOpts := git.SyncOptions{ForceOverwrite: forceOverwrite}
|
||||||
|
if err := wtMgr.SyncJSONLToWorktreeWithOptions(worktreePath, jsonlRelPath, syncOpts); err != nil {
|
||||||
return false, fmt.Errorf("failed to sync JSONL to worktree: %w", err)
|
return false, fmt.Errorf("failed to sync JSONL to worktree: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -143,11 +143,33 @@ func (wm *WorktreeManager) CheckWorktreeHealth(worktreePath string) error {
|
|||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// SyncOptions configures the behavior of SyncJSONLToWorktree
|
||||||
|
type SyncOptions struct {
|
||||||
|
// ForceOverwrite bypasses the merge logic and always overwrites the worktree
|
||||||
|
// JSONL with the local JSONL. This should be set to true when the daemon
|
||||||
|
// knows that a mutation (especially delete) occurred, so the local state
|
||||||
|
// is authoritative and should not be merged with potentially stale worktree data.
|
||||||
|
ForceOverwrite bool
|
||||||
|
}
|
||||||
|
|
||||||
// SyncJSONLToWorktree syncs the JSONL file from main repo to worktree.
|
// SyncJSONLToWorktree syncs the JSONL file from main repo to worktree.
|
||||||
// If the worktree has issues that the local repo doesn't have, it merges them
|
// If the worktree has issues that the local repo doesn't have, it merges them
|
||||||
// instead of overwriting. This prevents data loss when a fresh clone syncs
|
// instead of overwriting. This prevents data loss when a fresh clone syncs
|
||||||
// with fewer issues than the remote. (bd-52q fix for GitHub #464)
|
// with fewer issues than the remote. (bd-52q fix for GitHub #464)
|
||||||
|
// Note: This is a convenience wrapper that calls SyncJSONLToWorktreeWithOptions
|
||||||
|
// with default options (ForceOverwrite=false).
|
||||||
func (wm *WorktreeManager) SyncJSONLToWorktree(worktreePath, jsonlRelPath string) error {
|
func (wm *WorktreeManager) SyncJSONLToWorktree(worktreePath, jsonlRelPath string) error {
|
||||||
|
return wm.SyncJSONLToWorktreeWithOptions(worktreePath, jsonlRelPath, SyncOptions{})
|
||||||
|
}
|
||||||
|
|
||||||
|
// SyncJSONLToWorktreeWithOptions syncs the JSONL file from main repo to worktree
|
||||||
|
// with configurable options.
|
||||||
|
// If ForceOverwrite is true, the local JSONL is always copied to the worktree,
|
||||||
|
// bypassing the merge logic. This is used when the daemon knows a mutation
|
||||||
|
// (especially delete) occurred and the local state is authoritative.
|
||||||
|
// If ForceOverwrite is false (default), the function uses merge logic to prevent
|
||||||
|
// data loss when a fresh clone syncs with fewer issues than the remote.
|
||||||
|
func (wm *WorktreeManager) SyncJSONLToWorktreeWithOptions(worktreePath, jsonlRelPath string, opts SyncOptions) error {
|
||||||
// Source: main repo JSONL
|
// Source: main repo JSONL
|
||||||
srcPath := filepath.Join(wm.repoPath, jsonlRelPath)
|
srcPath := filepath.Join(wm.repoPath, jsonlRelPath)
|
||||||
|
|
||||||
@@ -176,6 +198,17 @@ func (wm *WorktreeManager) SyncJSONLToWorktree(worktreePath, jsonlRelPath string
|
|||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ForceOverwrite: When the daemon knows a mutation occurred (especially delete),
|
||||||
|
// the local state is authoritative and should overwrite the worktree without merging.
|
||||||
|
// This fixes the bug where `bd delete` mutations were not reflected in the sync branch
|
||||||
|
// because the merge logic would re-add the deleted issue.
|
||||||
|
if opts.ForceOverwrite {
|
||||||
|
if err := os.WriteFile(dstPath, srcData, 0644); err != nil { // #nosec G306 - JSONL needs to be readable
|
||||||
|
return fmt.Errorf("failed to write destination JSONL: %w", err)
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
// Count issues in both files
|
// Count issues in both files
|
||||||
srcCount := countJSONLIssues(srcData)
|
srcCount := countJSONLIssues(srcData)
|
||||||
dstCount := countJSONLIssues(dstData)
|
dstCount := countJSONLIssues(dstData)
|
||||||
|
|||||||
@@ -697,6 +697,121 @@ func TestSyncJSONLToWorktreeMerge(t *testing.T) {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestSyncJSONLToWorktree_DeleteMutation is a regression test for the bug where
|
||||||
|
// intentional deletions via `bd delete` are not synced to the sync branch.
|
||||||
|
// The issue: SyncJSONLToWorktree uses issue count to decide merge vs overwrite.
|
||||||
|
// When local has fewer issues (due to deletion), it merges instead of overwrites,
|
||||||
|
// which re-adds the deleted issue. This test verifies that when forceOverwrite
|
||||||
|
// is true (indicating an intentional mutation like delete), the local state
|
||||||
|
// is copied to the worktree without merging.
|
||||||
|
// GitHub Issue: #XXX (daemon auto-sync delete mutation not reflected in sync branch)
|
||||||
|
func TestSyncJSONLToWorktree_DeleteMutation(t *testing.T) {
|
||||||
|
repoPath, cleanup := setupTestRepo(t)
|
||||||
|
defer cleanup()
|
||||||
|
|
||||||
|
wm := NewWorktreeManager(repoPath)
|
||||||
|
worktreePath := filepath.Join(t.TempDir(), "beads-worktree-delete")
|
||||||
|
|
||||||
|
// Create worktree
|
||||||
|
if err := wm.CreateBeadsWorktree("beads-metadata", worktreePath); err != nil {
|
||||||
|
t.Fatalf("CreateBeadsWorktree failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
t.Run("forceOverwrite=true overwrites even when local has fewer issues", func(t *testing.T) {
|
||||||
|
// Set up: worktree has 3 issues (simulating sync branch state before delete)
|
||||||
|
worktreeJSONL := filepath.Join(worktreePath, ".beads", "issues.jsonl")
|
||||||
|
worktreeData := `{"id":"bd-100","title":"Issue 1","status":"open","created_at":"2025-01-01T00:00:00Z","created_by":"user1"}
|
||||||
|
{"id":"bd-101","title":"Issue 2","status":"open","created_at":"2025-01-01T00:00:01Z","created_by":"user1"}
|
||||||
|
{"id":"bd-102","title":"Issue 3 - TO BE DELETED","status":"open","created_at":"2025-01-01T00:00:02Z","created_by":"user1"}
|
||||||
|
`
|
||||||
|
if err := os.WriteFile(worktreeJSONL, []byte(worktreeData), 0644); err != nil {
|
||||||
|
t.Fatalf("Failed to write worktree JSONL: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Local has 2 issues (user deleted bd-102 via `bd delete bd-102 --force`)
|
||||||
|
mainJSONL := filepath.Join(repoPath, ".beads", "issues.jsonl")
|
||||||
|
mainData := `{"id":"bd-100","title":"Issue 1","status":"open","created_at":"2025-01-01T00:00:00Z","created_by":"user1"}
|
||||||
|
{"id":"bd-101","title":"Issue 2","status":"open","created_at":"2025-01-01T00:00:01Z","created_by":"user1"}
|
||||||
|
`
|
||||||
|
if err := os.WriteFile(mainJSONL, []byte(mainData), 0644); err != nil {
|
||||||
|
t.Fatalf("Failed to write main JSONL: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Sync with forceOverwrite=true (simulating daemon sync after delete mutation)
|
||||||
|
if err := wm.SyncJSONLToWorktreeWithOptions(worktreePath, ".beads/issues.jsonl", SyncOptions{ForceOverwrite: true}); err != nil {
|
||||||
|
t.Fatalf("SyncJSONLToWorktreeWithOptions failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Read the result
|
||||||
|
resultData, err := os.ReadFile(worktreeJSONL)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("Failed to read result JSONL: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Should have exactly 2 issues (deleted issue should NOT be re-added)
|
||||||
|
resultCount := countJSONLIssues(resultData)
|
||||||
|
if resultCount != 2 {
|
||||||
|
t.Errorf("Expected 2 issues after delete sync, got %d\nContent:\n%s", resultCount, string(resultData))
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify deleted issue is NOT present
|
||||||
|
resultStr := string(resultData)
|
||||||
|
if strings.Contains(resultStr, "bd-102") {
|
||||||
|
t.Error("Deleted issue bd-102 should NOT be in synced result (forceOverwrite=true)")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify remaining issues are present
|
||||||
|
if !strings.Contains(resultStr, "bd-100") || !strings.Contains(resultStr, "bd-101") {
|
||||||
|
t.Error("Remaining issues bd-100 and bd-101 should be present")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("forceOverwrite=false merges when local has fewer issues (fresh clone scenario)", func(t *testing.T) {
|
||||||
|
// Set up: worktree has 3 issues (simulating remote state)
|
||||||
|
worktreeJSONL := filepath.Join(worktreePath, ".beads", "issues.jsonl")
|
||||||
|
worktreeData := `{"id":"bd-200","title":"Remote Issue 1","status":"open","created_at":"2025-01-01T00:00:00Z","created_by":"user1"}
|
||||||
|
{"id":"bd-201","title":"Remote Issue 2","status":"open","created_at":"2025-01-01T00:00:01Z","created_by":"user1"}
|
||||||
|
{"id":"bd-202","title":"Remote Issue 3","status":"open","created_at":"2025-01-01T00:00:02Z","created_by":"user1"}
|
||||||
|
`
|
||||||
|
if err := os.WriteFile(worktreeJSONL, []byte(worktreeData), 0644); err != nil {
|
||||||
|
t.Fatalf("Failed to write worktree JSONL: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Local has 1 issue (fresh clone that hasn't synced yet)
|
||||||
|
mainJSONL := filepath.Join(repoPath, ".beads", "issues.jsonl")
|
||||||
|
mainData := `{"id":"bd-203","title":"Local New Issue","status":"open","created_at":"2025-01-02T00:00:00Z","created_by":"user2"}
|
||||||
|
`
|
||||||
|
if err := os.WriteFile(mainJSONL, []byte(mainData), 0644); err != nil {
|
||||||
|
t.Fatalf("Failed to write main JSONL: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Sync with forceOverwrite=false (default behavior for non-mutation syncs)
|
||||||
|
if err := wm.SyncJSONLToWorktreeWithOptions(worktreePath, ".beads/issues.jsonl", SyncOptions{ForceOverwrite: false}); err != nil {
|
||||||
|
t.Fatalf("SyncJSONLToWorktreeWithOptions failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Read the result
|
||||||
|
resultData, err := os.ReadFile(worktreeJSONL)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("Failed to read result JSONL: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Should have all 4 issues (3 from remote + 1 from local, merged)
|
||||||
|
resultCount := countJSONLIssues(resultData)
|
||||||
|
if resultCount != 4 {
|
||||||
|
t.Errorf("Expected 4 issues after merge, got %d\nContent:\n%s", resultCount, string(resultData))
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify all issues are present
|
||||||
|
resultStr := string(resultData)
|
||||||
|
for _, id := range []string{"bd-200", "bd-201", "bd-202", "bd-203"} {
|
||||||
|
if !strings.Contains(resultStr, id) {
|
||||||
|
t.Errorf("Expected issue %s to be in merged result", id)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
func TestCountJSONLIssues(t *testing.T) {
|
func TestCountJSONLIssues(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
|
|||||||
Reference in New Issue
Block a user