From 80cd1f35c09a0f82909781cfb11809479d72782a Mon Sep 17 00:00:00 2001 From: jane Date: Mon, 19 Jan 2026 11:44:09 -0800 Subject: [PATCH] fix(sync): address code review issues in manual conflict resolution Fixes from code review: - Fix duplicate check in merge logic (use else clause) - Handle io.EOF gracefully (treat as quit) - Add quit (q) option to abort resolution early - Add accept-all (a) option to auto-merge remaining conflicts - Fix skipped conflicts to keep local version (not auto-merge) - Handle json.MarshalIndent errors properly - Fix truncateText to use rune count for UTF-8 safety - Update help text with new options - Add UTF-8 and emoji test cases Co-Authored-By: Claude Opus 4.5 --- cmd/bd/sync.go | 22 +++---- cmd/bd/sync_manual.go | 90 ++++++++++++++++++++++---- cmd/bd/sync_manual_test.go | 129 +++++++++++++++++++++++++++++++++---- 3 files changed, 202 insertions(+), 39 deletions(-) diff --git a/cmd/bd/sync.go b/cmd/bd/sync.go index c057f767..f1ddf050 100644 --- a/cmd/bd/sync.go +++ b/cmd/bd/sync.go @@ -45,7 +45,9 @@ The --manual flag shows a diff for each conflict and prompts you to choose: l/local - Keep local version r/remote - Keep remote version m/merge - Auto-merge (LWW for scalars, union for collections) - s/skip - Skip and leave unresolved + s/skip - Skip (keep local, conflict remains for later) + a/all - Accept auto-merge for all remaining conflicts + q/quit - Quit and skip all remaining conflicts d/diff - Show full JSON diff The --full flag provides the legacy full sync behavior for backwards compatibility.`, @@ -1149,22 +1151,14 @@ func resolveSyncConflictsManually(ctx context.Context, jsonlPath, beadsDir strin var mergedIssues []*beads.Issue for id := range allIDSet { if conflictIDSet[id] { - // This was a conflict - use the resolved version if available + // This was a conflict if resolved, ok := resolvedMap[id]; ok { + // User resolved this conflict - use their choice mergedIssues = append(mergedIssues, resolved) - } - // If not in resolvedMap, it was skipped - use the automatic merge result - if _, ok := resolvedMap[id]; !ok { - // Fall back to field-level merge for skipped conflicts - local := localMap[id] - remote := remoteMap[id] - base := baseMap[id] - if local != nil && remote != nil { - mergedIssues = append(mergedIssues, mergeFieldLevel(base, local, remote)) - } else if local != nil { + } else { + // Skipped - keep local version in output, conflict remains for later + if local := localMap[id]; local != nil { mergedIssues = append(mergedIssues, local) - } else if remote != nil { - mergedIssues = append(mergedIssues, remote) } } } else { diff --git a/cmd/bd/sync_manual.go b/cmd/bd/sync_manual.go index 79cfbbed..61ac04a8 100644 --- a/cmd/bd/sync_manual.go +++ b/cmd/bd/sync_manual.go @@ -4,8 +4,10 @@ import ( "bufio" "encoding/json" "fmt" + "io" "os" "strings" + "unicode/utf8" "github.com/steveyegge/beads/internal/beads" "github.com/steveyegge/beads/internal/ui" @@ -21,8 +23,8 @@ type InteractiveConflict struct { // InteractiveResolution represents the user's choice for a conflict type InteractiveResolution struct { - Choice string // "local", "remote", "merged", "skip" - Issue *beads.Issue // The resolved issue (nil if skipped) + Choice string // "local", "remote", "merged", "skip", "quit", "accept-all" + Issue *beads.Issue // The resolved issue (nil if skipped/quit) } // resolveConflictsInteractively handles manual conflict resolution with user prompts. @@ -54,15 +56,42 @@ func resolveConflictsInteractively(conflicts []InteractiveConflict) ([]*beads.Is } switch resolution.Choice { + case "quit": + // Quit - skip all remaining conflicts + remaining := len(conflicts) - i + skipped += remaining + fmt.Printf(" %s Quit - skipping %d remaining conflict(s)\n\n", ui.RenderMuted("⏹"), remaining) + return resolved, skipped, nil + + case "accept-all": + // Auto-merge all remaining conflicts + fmt.Printf(" %s Auto-merging %d remaining conflict(s)...\n", ui.RenderAccent("⚡"), len(conflicts)-i) + for j := i; j < len(conflicts); j++ { + c := conflicts[j] + if c.Local != nil && c.Remote != nil { + merged := mergeFieldLevel(c.Base, c.Local, c.Remote) + resolved = append(resolved, merged) + } else if c.Local != nil { + resolved = append(resolved, c.Local) + } else if c.Remote != nil { + resolved = append(resolved, c.Remote) + } + } + fmt.Printf(" %s Done\n\n", ui.RenderPass("✓")) + return resolved, skipped, nil + case "skip": skipped++ - fmt.Printf(" %s Skipped\n\n", ui.RenderMuted("⏭")) + fmt.Printf(" %s Skipped (will keep local, conflict remains)\n\n", ui.RenderMuted("⏭")) + case "local": resolved = append(resolved, conflict.Local) fmt.Printf(" %s Kept local version\n\n", ui.RenderPass("✓")) + case "remote": resolved = append(resolved, conflict.Remote) fmt.Printf(" %s Kept remote version\n\n", ui.RenderPass("✓")) + case "merged": resolved = append(resolved, resolution.Issue) fmt.Printf(" %s Used field-level merge\n\n", ui.RenderPass("✓")) @@ -187,7 +216,7 @@ func promptConflictResolution(reader *bufio.Reader, conflict InteractiveConflict // Build options based on what's available var options []string - var optionMap = make(map[string]string) + optionMap := make(map[string]string) if local != nil { options = append(options, "l") @@ -205,9 +234,13 @@ func promptConflictResolution(reader *bufio.Reader, conflict InteractiveConflict optionMap["merge"] = "merged" optionMap["merged"] = "merged" } - options = append(options, "s", "d", "?") + options = append(options, "s", "a", "q", "d", "?") optionMap["s"] = "skip" optionMap["skip"] = "skip" + optionMap["a"] = "accept-all" + optionMap["all"] = "accept-all" + optionMap["q"] = "quit" + optionMap["quit"] = "quit" optionMap["d"] = "diff" optionMap["diff"] = "diff" optionMap["?"] = "help" @@ -217,6 +250,10 @@ func promptConflictResolution(reader *bufio.Reader, conflict InteractiveConflict fmt.Printf(" Choice [%s]: ", strings.Join(options, "/")) input, err := reader.ReadString('\n') if err != nil { + if err == io.EOF { + // Treat EOF as quit + return InteractiveResolution{Choice: "quit", Issue: nil}, nil + } return InteractiveResolution{}, err } @@ -261,6 +298,12 @@ func promptConflictResolution(reader *bufio.Reader, conflict InteractiveConflict case "skip": return InteractiveResolution{Choice: "skip", Issue: nil}, nil + + case "accept-all": + return InteractiveResolution{Choice: "accept-all", Issue: nil}, nil + + case "quit": + return InteractiveResolution{Choice: "quit", Issue: nil}, nil } } } @@ -278,7 +321,9 @@ func printResolutionHelp(hasLocal, hasRemote bool) { if hasLocal && hasRemote { fmt.Println(" m, merge - Auto-merge (LWW for scalars, union for collections)") } - fmt.Println(" s, skip - Skip this conflict (leave unresolved)") + fmt.Println(" s, skip - Skip this conflict (keep local, conflict remains)") + fmt.Println(" a, all - Accept auto-merge for all remaining conflicts") + fmt.Println(" q, quit - Quit and skip all remaining conflicts") fmt.Println(" d, diff - Show detailed JSON diff") fmt.Println(" ?, help - Show this help") fmt.Println() @@ -292,8 +337,12 @@ func showDetailedDiff(conflict InteractiveConflict) { if conflict.Local != nil { fmt.Printf(" %s\n", ui.RenderAccent("LOCAL:")) - localJSON, _ := json.MarshalIndent(conflict.Local, " ", " ") - fmt.Println(string(localJSON)) + localJSON, err := json.MarshalIndent(conflict.Local, " ", " ") + if err != nil { + fmt.Printf(" (error marshaling: %v)\n", err) + } else { + fmt.Println(string(localJSON)) + } fmt.Println() } else { fmt.Printf(" %s (deleted)\n", ui.RenderMuted("LOCAL:")) @@ -301,8 +350,12 @@ func showDetailedDiff(conflict InteractiveConflict) { if conflict.Remote != nil { fmt.Printf(" %s\n", ui.RenderAccent("REMOTE:")) - remoteJSON, _ := json.MarshalIndent(conflict.Remote, " ", " ") - fmt.Println(string(remoteJSON)) + remoteJSON, err := json.MarshalIndent(conflict.Remote, " ", " ") + if err != nil { + fmt.Printf(" (error marshaling: %v)\n", err) + } else { + fmt.Println(string(remoteJSON)) + } fmt.Println() } else { fmt.Printf(" %s (deleted)\n", ui.RenderMuted("REMOTE:")) @@ -318,6 +371,8 @@ func valueOrNone(s string) string { return s } +// truncateText truncates a string to maxLen runes (not bytes) for proper UTF-8 handling. +// Replaces newlines with spaces for single-line display. func truncateText(s string, maxLen int) string { if s == "" { return "(empty)" @@ -325,8 +380,17 @@ func truncateText(s string, maxLen int) string { // Replace newlines with spaces for display s = strings.ReplaceAll(s, "\n", " ") s = strings.ReplaceAll(s, "\r", "") - if len(s) > maxLen { - return s[:maxLen-3] + "..." + + // Count runes, not bytes, for proper UTF-8 handling + runeCount := utf8.RuneCountInString(s) + if runeCount <= maxLen { + return s } - return s + + // Truncate by runes + runes := []rune(s) + if maxLen <= 3 { + return "..." + } + return string(runes[:maxLen-3]) + "..." } diff --git a/cmd/bd/sync_manual_test.go b/cmd/bd/sync_manual_test.go index 98f58d95..4bc32a29 100644 --- a/cmd/bd/sync_manual_test.go +++ b/cmd/bd/sync_manual_test.go @@ -45,6 +45,36 @@ func TestTruncateText(t *testing.T) { maxLen: 30, want: "line1 line2 line3", }, + { + name: "very short max", + input: "hello world", + maxLen: 3, + want: "...", + }, + { + name: "UTF-8 characters preserved", + input: "Hello 世界！This is a test", + maxLen: 12, + want: "Hello 世界！...", + }, + { + name: "UTF-8 exact length", + input: "日本語テスト", + maxLen: 6, + want: "日本語テスト", + }, + { + name: "UTF-8 truncate", + input: "日本語テストです", + maxLen: 6, + want: "日本語...", + }, + { + name: "emoji handling", + input: "Hello 🌍🌎🌏 World", + maxLen: 12, + want: "Hello 🌍🌎🌏...", + }, } for _, tt := range tests { @@ -164,6 +194,14 @@ func TestInteractiveConflictDisplay(t *testing.T) { }, }, }, + { + name: "both nil (edge case)", + conflict: InteractiveConflict{ + IssueID: "test-6", + Local: nil, + Remote: nil, + }, + }, } for _, tt := range tests { @@ -177,22 +215,58 @@ func TestInteractiveConflictDisplay(t *testing.T) { func TestShowDetailedDiff(t *testing.T) { now := time.Now() - conflict := InteractiveConflict{ - IssueID: "test-1", - Local: &beads.Issue{ - ID: "test-1", - Title: "Local", - UpdatedAt: now, + tests := []struct { + name string + conflict InteractiveConflict + }{ + { + name: "both exist", + conflict: InteractiveConflict{ + IssueID: "test-1", + Local: &beads.Issue{ + ID: "test-1", + Title: "Local", + UpdatedAt: now, + }, + Remote: &beads.Issue{ + ID: "test-1", + Title: "Remote", + UpdatedAt: now, + }, + }, }, - Remote: &beads.Issue{ - ID: "test-1", - Title: "Remote", - UpdatedAt: now, + { + name: "local nil", + conflict: InteractiveConflict{ + IssueID: "test-2", + Local: nil, + Remote: &beads.Issue{ + ID: "test-2", + Title: "Remote", + UpdatedAt: now, + }, + }, + }, + { + name: "remote nil", + conflict: InteractiveConflict{ + IssueID: "test-3", + Local: &beads.Issue{ + ID: "test-3", + Title: "Local", + UpdatedAt: now, + }, + Remote: nil, + }, }, } - // Just make sure it doesn't panic - showDetailedDiff(conflict) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Just make sure it doesn't panic + showDetailedDiff(tt.conflict) + }) + } } func TestPrintResolutionHelp(t *testing.T) { @@ -272,3 +346,34 @@ func TestInteractiveResolutionMerge(t *testing.T) { t.Errorf("Expected labels to contain 'bug' and 'feature', got %v", merged.Labels) } } + +func TestInteractiveResolutionChoices(t *testing.T) { + // Test InteractiveResolution struct values + tests := []struct { + name string + choice string + issue *beads.Issue + }{ + {"local", "local", &beads.Issue{ID: "test"}}, + {"remote", "remote", &beads.Issue{ID: "test"}}, + {"merged", "merged", &beads.Issue{ID: "test"}}, + {"skip", "skip", nil}, + {"quit", "quit", nil}, + {"accept-all", "accept-all", nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res := InteractiveResolution{Choice: tt.choice, Issue: tt.issue} + if res.Choice != tt.choice { + t.Errorf("Expected choice %q, got %q", tt.choice, res.Choice) + } + if tt.issue == nil && res.Issue != nil { + t.Errorf("Expected nil issue, got %v", res.Issue) + } + if tt.issue != nil && res.Issue == nil { + t.Errorf("Expected non-nil issue, got nil") + } + }) + } +}