diff --git a/cmd/bd/sync.go b/cmd/bd/sync.go index eaf2a48f..15742824 100644 --- a/cmd/bd/sync.go +++ b/cmd/bd/sync.go @@ -15,6 +15,7 @@ import ( "time" "github.com/spf13/cobra" + "github.com/steveyegge/beads/internal/config" "github.com/steveyegge/beads/internal/configfile" "github.com/steveyegge/beads/internal/debug" "github.com/steveyegge/beads/internal/deletions" @@ -378,7 +379,11 @@ Use --merge to merge the sync branch back to main branch.`, if useSyncBranch { // Pull from sync branch via worktree (bd-e3w) fmt.Printf("→ Pulling from sync branch '%s'...\n", syncBranchName) - pullResult, err := syncbranch.PullFromSyncBranch(ctx, repoRoot, syncBranchName, jsonlPath, !noPush) + + // bd-4u8: Check if confirmation is required for mass deletion + requireMassDeleteConfirmation := config.GetBool("sync.require_confirmation_on_mass_delete") + + pullResult, err := syncbranch.PullFromSyncBranch(ctx, repoRoot, syncBranchName, jsonlPath, !noPush, requireMassDeleteConfirmation) if err != nil { fmt.Fprintf(os.Stderr, "Error pulling from sync branch: %v\n", err) os.Exit(1) @@ -387,8 +392,37 @@ Use --merge to merge the sync branch back to main branch.`, if pullResult.Merged { // bd-3s8 fix: divergent histories were merged at content level fmt.Printf("✓ Merged divergent histories from %s\n", syncBranchName) - // bd-7ch: auto-push after merge - if pullResult.Pushed { + + // bd-7z4: Print safety warnings from result + for _, warning := range pullResult.SafetyWarnings { + fmt.Fprintln(os.Stderr, warning) + } + + // bd-4u8: Handle safety check with confirmation requirement + if pullResult.SafetyCheckTriggered && !pullResult.Pushed { + // Prompt for confirmation + fmt.Fprintf(os.Stderr, "\n⚠️ Mass deletion detected: %s\n", pullResult.SafetyCheckDetails) + fmt.Fprintf(os.Stderr, "Push these changes to remote? [y/N]: ") + + var response string + reader := bufio.NewReader(os.Stdin) + response, _ = reader.ReadString('\n') + response = strings.TrimSpace(strings.ToLower(response)) + + if response == "y" || response == "yes" { + fmt.Printf("→ Pushing to %s...\n", syncBranchName) + if err := syncbranch.PushSyncBranch(ctx, repoRoot, syncBranchName); err != nil { + fmt.Fprintf(os.Stderr, "Error pushing to sync branch: %v\n", err) + os.Exit(1) + } + fmt.Printf("✓ Pushed merged changes to %s\n", syncBranchName) + pushedViaSyncBranch = true + } else { + fmt.Println("Push cancelled. Run 'bd sync' again to retry.") + fmt.Println("If this was unintended, use 'git reflog' on the sync branch to recover.") + } + } else if pullResult.Pushed { + // bd-7ch: auto-push after merge fmt.Printf("✓ Pushed merged changes to %s\n", syncBranchName) pushedViaSyncBranch = true } diff --git a/internal/config/config.go b/internal/config/config.go index 6f93ae7d..bb82264b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -99,6 +99,9 @@ func Initialize() error { v.SetDefault("routing.maintainer", ".") v.SetDefault("routing.contributor", "~/.beads-planning") + // Sync configuration defaults (bd-4u8) + v.SetDefault("sync.require_confirmation_on_mass_delete", false) + // Read config file if it was found if configFileSet { if err := v.ReadInConfig(); err != nil { diff --git a/internal/merge/merge.go b/internal/merge/merge.go index a2297e96..6c617805 100644 --- a/internal/merge/merge.go +++ b/internal/merge/merge.go @@ -426,6 +426,8 @@ func mergeNotes(base, left, right string) string { } // mergePriority handles priority merging - on conflict, higher priority wins (lower number) +// Special case: 0 is treated as "unset/no priority" due to Go's zero value. +// Any explicitly set priority (>0) wins over 0. (bd-d0t fix) func mergePriority(base, left, right int) int { // Standard 3-way merge for non-conflict cases if base == left && base != right { @@ -438,7 +440,16 @@ func mergePriority(base, left, right int) int { return left } // True conflict: both sides changed to different values - // Higher priority wins (lower number = more urgent) + + // bd-d0t fix: Treat 0 as "unset" - explicitly set priority wins over unset + if left == 0 && right > 0 { + return right // right has explicit priority, left is unset + } + if right == 0 && left > 0 { + return left // left has explicit priority, right is unset + } + + // Both have explicit priorities (or both are 0) - higher priority wins (lower number = more urgent) if left < right { return left } @@ -477,9 +488,9 @@ func isTimeAfter(t1, t2 string) bool { return true // t1 valid, t2 invalid - t1 wins } - // Both valid - compare. On exact tie, return false (right wins for now) - // TODO: Consider preferring left on tie for consistency with IssueType rule - return time1.After(time2) + // Both valid - compare. On exact tie, left wins for consistency with IssueType rule (bd-8nz) + // Using !time2.After(time1) returns true when t1 > t2 OR t1 == t2 + return !time2.After(time1) } func maxTime(t1, t2 string) string { diff --git a/internal/merge/merge_test.go b/internal/merge/merge_test.go index 36f08b5a..0f68ba49 100644 --- a/internal/merge/merge_test.go +++ b/internal/merge/merge_test.go @@ -337,10 +337,10 @@ func TestIsTimeAfter(t *testing.T) { expected: false, }, { - name: "identical timestamps - right wins (false)", + name: "identical timestamps - left wins (bd-8nz)", t1: "2024-01-01T00:00:00Z", t2: "2024-01-01T00:00:00Z", - expected: false, + expected: true, }, { name: "t1 invalid, t2 valid - t2 wins", @@ -483,6 +483,99 @@ func TestMerge3Way_SimpleUpdates(t *testing.T) { }) } +// TestMergePriority tests priority merging including bd-d0t fix +func TestMergePriority(t *testing.T) { + tests := []struct { + name string + base int + left int + right int + expected int + }{ + { + name: "no changes", + base: 2, + left: 2, + right: 2, + expected: 2, + }, + { + name: "left changed", + base: 2, + left: 1, + right: 2, + expected: 1, + }, + { + name: "right changed", + base: 2, + left: 2, + right: 3, + expected: 3, + }, + { + name: "both changed to same value", + base: 2, + left: 1, + right: 1, + expected: 1, + }, + { + name: "conflict - higher priority wins (lower number)", + base: 2, + left: 3, + right: 1, + expected: 1, + }, + // bd-d0t fix: 0 is treated as "unset" + { + name: "bd-d0t: left unset (0), right has explicit priority", + base: 2, + left: 0, + right: 3, + expected: 3, // explicit priority wins over unset + }, + { + name: "bd-d0t: left has explicit priority, right unset (0)", + base: 2, + left: 3, + right: 0, + expected: 3, // explicit priority wins over unset + }, + { + name: "bd-d0t: both unset (0)", + base: 2, + left: 0, + right: 0, + expected: 0, + }, + { + name: "bd-d0t: base unset, left sets priority, right unchanged", + base: 0, + left: 1, + right: 0, + expected: 1, // left changed from 0 to 1 + }, + { + name: "bd-d0t: base unset, right sets priority, left unchanged", + base: 0, + left: 0, + right: 2, + expected: 2, // right changed from 0 to 2 + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := mergePriority(tt.base, tt.left, tt.right) + if result != tt.expected { + t.Errorf("mergePriority(%d, %d, %d) = %d, want %d", + tt.base, tt.left, tt.right, result, tt.expected) + } + }) + } +} + // TestMerge3Way_AutoResolve tests auto-resolution of conflicts func TestMerge3Way_AutoResolve(t *testing.T) { t.Run("conflicting title changes - latest updated_at wins", func(t *testing.T) { diff --git a/internal/syncbranch/worktree.go b/internal/syncbranch/worktree.go index 0ce577f9..39969bf8 100644 --- a/internal/syncbranch/worktree.go +++ b/internal/syncbranch/worktree.go @@ -2,6 +2,7 @@ package syncbranch import ( "context" + "encoding/json" "fmt" "os" "os/exec" @@ -30,6 +31,15 @@ type PullResult struct { Merged bool // True if divergent histories were merged FastForwarded bool // True if fast-forward was possible Pushed bool // True if changes were pushed after merge (bd-7ch) + + // SafetyCheckTriggered indicates mass deletion was detected during merge (bd-4u8) + // When true, callers should check config option sync.require_confirmation_on_mass_delete + SafetyCheckTriggered bool + // SafetyCheckDetails contains human-readable details about the mass deletion (bd-4u8) + SafetyCheckDetails string + // SafetyWarnings contains warning messages from the safety check (bd-7z4) + // Caller should display these to the user as appropriate for their output format + SafetyWarnings []string } // CommitToSyncBranch commits JSONL changes to the sync branch using a git worktree. @@ -187,6 +197,10 @@ func preemptiveFetchAndFastForward(ctx context.Context, worktreePath, branch, re // Includes safety check: warns (but doesn't block) if >50% issues vanished AND >5 existed. // "Vanished" means removed from issues.jsonl entirely, NOT status=closed. // +// IMPORTANT (bd-4u8): If requireMassDeleteConfirmation is true and the safety check triggers, +// the function will NOT auto-push. Instead, it sets SafetyCheckTriggered=true in the result +// and the caller should prompt for confirmation then call PushSyncBranch. +// // This ensures sync never fails due to git merge conflicts, as we handle merging at the // JSONL content level where we have semantic understanding of the data. // @@ -196,9 +210,16 @@ func preemptiveFetchAndFastForward(ctx context.Context, worktreePath, branch, re // - syncBranch: Name of the sync branch (e.g., "beads-sync") // - jsonlPath: Absolute path to the JSONL file in the main repo // - push: If true, push to remote after merge (bd-7ch) +// - requireMassDeleteConfirmation: If true and mass deletion detected, skip push (bd-4u8) // // Returns PullResult with details about what was done, or error if failed. -func PullFromSyncBranch(ctx context.Context, repoRoot, syncBranch, jsonlPath string, push bool) (*PullResult, error) { +func PullFromSyncBranch(ctx context.Context, repoRoot, syncBranch, jsonlPath string, push bool, requireMassDeleteConfirmation ...bool) (*PullResult, error) { + // bd-4u8: Extract optional confirmation requirement parameter + requireConfirmation := false + if len(requireMassDeleteConfirmation) > 0 { + requireConfirmation = requireMassDeleteConfirmation[0] + } + result := &PullResult{ Branch: syncBranch, JSONLPath: jsonlPath, @@ -278,7 +299,11 @@ func PullFromSyncBranch(ctx context.Context, repoRoot, syncBranch, jsonlPath str // 4. Commit merged content on top // bd-7ch: Extract local content before merge for safety check - localContent, _ := extractJSONLFromCommit(ctx, worktreePath, "HEAD", jsonlRelPath) + localContent, extractErr := extractJSONLFromCommit(ctx, worktreePath, "HEAD", jsonlRelPath) + if extractErr != nil { + // bd-feh: Log warning so users know safety check may be skipped + fmt.Fprintf(os.Stderr, "⚠️ Warning: Could not extract local content for safety check: %v\n", extractErr) + } mergedContent, err := performContentMerge(ctx, worktreePath, syncBranch, remote, jsonlRelPath) if err != nil { @@ -345,23 +370,50 @@ func PullFromSyncBranch(ctx context.Context, repoRoot, syncBranch, jsonlPath str localCount := countIssuesInContent(localContent) mergedCount := countIssuesInContent(mergedContent) + // Track if we should skip push due to safety check requiring confirmation + skipPushForConfirmation := false + // Warn if >50% issues vanished AND >5 existed before // "Vanished" = removed from JSONL entirely (not status=closed) if localCount > 5 && mergedCount < localCount { vanishedPercent := float64(localCount-mergedCount) / float64(localCount) * 100 if vanishedPercent > 50 { - fmt.Fprintf(os.Stderr, "⚠️ Warning: %.0f%% of issues vanished during merge (%d → %d issues)\n", + // bd-4u8: Set safety check fields for caller to handle confirmation + result.SafetyCheckTriggered = true + result.SafetyCheckDetails = fmt.Sprintf("%.0f%% of issues vanished during merge (%d → %d issues)", vanishedPercent, localCount, mergedCount) - fmt.Fprintf(os.Stderr, " This may indicate accidental mass deletion. Pushing anyway.\n") - fmt.Fprintf(os.Stderr, " If this was unintended, use 'git reflog' on the sync branch to recover.\n") + + // bd-7z4: Return warnings in result instead of printing directly to stderr + result.SafetyWarnings = append(result.SafetyWarnings, + fmt.Sprintf("⚠️ Warning: %.0f%% of issues vanished during merge (%d → %d issues)", + vanishedPercent, localCount, mergedCount)) + + // bd-lsa: Add forensic info to warnings + localIssues := parseIssuesFromContent(localContent) + mergedIssues := parseIssuesFromContent(mergedContent) + forensicLines := formatVanishedIssues(localIssues, mergedIssues, localCount, mergedCount) + result.SafetyWarnings = append(result.SafetyWarnings, forensicLines...) + + // bd-4u8: Check if confirmation is required before pushing + if requireConfirmation { + result.SafetyWarnings = append(result.SafetyWarnings, + " Push skipped - confirmation required (sync.require_confirmation_on_mass_delete=true)") + skipPushForConfirmation = true + } else { + result.SafetyWarnings = append(result.SafetyWarnings, + " This may indicate accidental mass deletion. Pushing anyway.", + " If this was unintended, use 'git reflog' on the sync branch to recover.") + } } } - // Push regardless of safety check (don't block happy path) - if err := pushFromWorktree(ctx, worktreePath, syncBranch); err != nil { - return nil, fmt.Errorf("failed to push after merge: %w", err) + // Push unless safety check requires confirmation + if !skipPushForConfirmation { + if err := pushFromWorktree(ctx, worktreePath, syncBranch); err != nil { + return nil, fmt.Errorf("failed to push after merge: %w", err) + } + result.Pushed = true } - result.Pushed = true } return result, nil @@ -650,6 +702,28 @@ func pushFromWorktree(ctx context.Context, worktreePath, branch string) error { return nil } +// PushSyncBranch pushes the sync branch to remote. (bd-4u8) +// This is used after confirmation when sync.require_confirmation_on_mass_delete is enabled +// and a mass deletion was detected during merge. +// +// Parameters: +// - ctx: Context for cancellation +// - repoRoot: Path to the git repository root +// - syncBranch: Name of the sync branch (e.g., "beads-sync") +// +// Returns error if push fails. +func PushSyncBranch(ctx context.Context, repoRoot, syncBranch string) error { + // Worktree path is under .git/beads-worktrees/ + worktreePath := filepath.Join(repoRoot, ".git", "beads-worktrees", syncBranch) + + // Verify worktree exists + if _, err := os.Stat(worktreePath); os.IsNotExist(err) { + return fmt.Errorf("sync branch worktree not found at %s", worktreePath) + } + + return pushFromWorktree(ctx, worktreePath, syncBranch) +} + // getRemoteForBranch gets the remote name for a branch, defaulting to "origin" func getRemoteForBranch(ctx context.Context, worktreePath, branch string) string { remoteCmd := exec.CommandContext(ctx, "git", "-C", worktreePath, "config", "--get", fmt.Sprintf("branch.%s.remote", branch)) @@ -685,6 +759,62 @@ func countIssuesInContent(content []byte) int { return count } +// issueSummary holds minimal issue info for forensic logging (bd-lsa) +type issueSummary struct { + ID string `json:"id"` + Title string `json:"title"` +} + +// parseIssuesFromContent extracts issue IDs and titles from JSONL content. +// Used for forensic logging of vanished issues (bd-lsa). +func parseIssuesFromContent(content []byte) map[string]issueSummary { + result := make(map[string]issueSummary) + if len(content) == 0 { + return result + } + for _, line := range strings.Split(string(content), "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + var summary issueSummary + if err := json.Unmarshal([]byte(line), &summary); err != nil { + continue // Skip malformed lines + } + if summary.ID != "" { + result[summary.ID] = summary + } + } + return result +} + +// formatVanishedIssues returns forensic info lines when issues vanish during merge (bd-lsa, bd-7z4). +// Returns string slices for caller to display as appropriate for their output format. +func formatVanishedIssues(localIssues, mergedIssues map[string]issueSummary, localCount, mergedCount int) []string { + var lines []string + timestamp := time.Now().Format("2006-01-02 15:04:05 MST") + + lines = append(lines, fmt.Sprintf("\n📋 Mass deletion forensic log [%s]", timestamp)) + lines = append(lines, fmt.Sprintf(" Before merge: %d issues", localCount)) + lines = append(lines, fmt.Sprintf(" After merge: %d issues", mergedCount)) + lines = append(lines, " Vanished issues:") + + vanishedCount := 0 + for id, summary := range localIssues { + if _, exists := mergedIssues[id]; !exists { + vanishedCount++ + title := summary.Title + if len(title) > 60 { + title = title[:57] + "..." + } + lines = append(lines, fmt.Sprintf(" - %s: %s", id, title)) + } + } + lines = append(lines, fmt.Sprintf(" Total vanished: %d\n", vanishedCount)) + + return lines +} + // HasGitRemote checks if any git remote exists func HasGitRemote(ctx context.Context) bool { cmd := exec.CommandContext(ctx, "git", "remote") diff --git a/internal/syncbranch/worktree_divergence_test.go b/internal/syncbranch/worktree_divergence_test.go index 7bb9b84c..40eae27a 100644 --- a/internal/syncbranch/worktree_divergence_test.go +++ b/internal/syncbranch/worktree_divergence_test.go @@ -459,6 +459,160 @@ func writeFile(t *testing.T, path, content string) { } } +// TestParseIssuesFromContent tests the issue parsing helper function (bd-lsa) +func TestParseIssuesFromContent(t *testing.T) { + tests := []struct { + name string + content []byte + wantIDs []string + }{ + { + name: "empty content", + content: []byte{}, + wantIDs: []string{}, + }, + { + name: "nil content", + content: nil, + wantIDs: []string{}, + }, + { + name: "single issue", + content: []byte(`{"id":"test-1","title":"Test Issue"}`), + wantIDs: []string{"test-1"}, + }, + { + name: "multiple issues", + content: []byte(`{"id":"test-1","title":"Issue 1"}` + "\n" + `{"id":"test-2","title":"Issue 2"}`), + wantIDs: []string{"test-1", "test-2"}, + }, + { + name: "malformed line skipped", + content: []byte(`{"id":"test-1","title":"Valid"}` + "\n" + `invalid json` + "\n" + `{"id":"test-2","title":"Also Valid"}`), + wantIDs: []string{"test-1", "test-2"}, + }, + { + name: "empty id skipped", + content: []byte(`{"id":"","title":"No ID"}` + "\n" + `{"id":"test-1","title":"Has ID"}`), + wantIDs: []string{"test-1"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := parseIssuesFromContent(tt.content) + if len(got) != len(tt.wantIDs) { + t.Errorf("parseIssuesFromContent() returned %d issues, want %d", len(got), len(tt.wantIDs)) + return + } + for _, wantID := range tt.wantIDs { + if _, exists := got[wantID]; !exists { + t.Errorf("parseIssuesFromContent() missing expected ID %q", wantID) + } + } + }) + } +} + +// TestSafetyCheckMassDeletion tests the safety check behavior for mass deletions (bd-cnn) +func TestSafetyCheckMassDeletion(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + + t.Run("safety check triggers when >50% issues vanish and >5 existed", func(t *testing.T) { + // Test the countIssuesInContent and formatVanishedIssues functions + // Create local content with 10 issues + var localLines []string + for i := 1; i <= 10; i++ { + localLines = append(localLines, `{"id":"test-`+string(rune('0'+i))+`","title":"Issue `+string(rune('0'+i))+`"}`) + } + localContent := []byte(strings.Join(localLines, "\n")) + + // Create merged content with only 4 issues (60% vanished) + mergedContent := []byte(`{"id":"test-1","title":"Issue 1"} +{"id":"test-2","title":"Issue 2"} +{"id":"test-3","title":"Issue 3"} +{"id":"test-4","title":"Issue 4"}`) + + localCount := countIssuesInContent(localContent) + mergedCount := countIssuesInContent(mergedContent) + + if localCount != 10 { + t.Errorf("localCount = %d, want 10", localCount) + } + if mergedCount != 4 { + t.Errorf("mergedCount = %d, want 4", mergedCount) + } + + // Verify safety check would trigger: >50% vanished AND >5 existed + if localCount <= 5 { + t.Error("localCount should be > 5 for safety check to apply") + } + vanishedPercent := float64(localCount-mergedCount) / float64(localCount) * 100 + if vanishedPercent <= 50 { + t.Errorf("vanishedPercent = %.0f%%, want > 50%%", vanishedPercent) + } + + // Verify forensic info can be generated + localIssues := parseIssuesFromContent(localContent) + mergedIssues := parseIssuesFromContent(mergedContent) + forensicLines := formatVanishedIssues(localIssues, mergedIssues, localCount, mergedCount) + if len(forensicLines) == 0 { + t.Error("formatVanishedIssues returned empty lines") + } + }) + + t.Run("safety check does NOT trigger when <50% issues vanish", func(t *testing.T) { + // 10 issues, 6 remain = 40% vanished (should NOT trigger) + var localLines []string + for i := 1; i <= 10; i++ { + localLines = append(localLines, `{"id":"test-`+string(rune('0'+i))+`"}`) + } + localContent := []byte(strings.Join(localLines, "\n")) + + // 6 issues remain (40% vanished) + var mergedLines []string + for i := 1; i <= 6; i++ { + mergedLines = append(mergedLines, `{"id":"test-`+string(rune('0'+i))+`"}`) + } + mergedContent := []byte(strings.Join(mergedLines, "\n")) + + localCount := countIssuesInContent(localContent) + mergedCount := countIssuesInContent(mergedContent) + + vanishedPercent := float64(localCount-mergedCount) / float64(localCount) * 100 + if vanishedPercent > 50 { + t.Errorf("vanishedPercent = %.0f%%, want <= 50%%", vanishedPercent) + } + }) + + t.Run("safety check does NOT trigger when <5 issues existed", func(t *testing.T) { + // 4 issues, 1 remains = 75% vanished, but only 4 existed (should NOT trigger) + localContent := []byte(`{"id":"test-1"} +{"id":"test-2"} +{"id":"test-3"} +{"id":"test-4"}`) + + mergedContent := []byte(`{"id":"test-1"}`) + + localCount := countIssuesInContent(localContent) + mergedCount := countIssuesInContent(mergedContent) + + if localCount != 4 { + t.Errorf("localCount = %d, want 4", localCount) + } + if mergedCount != 1 { + t.Errorf("mergedCount = %d, want 1", mergedCount) + } + + // localCount > 5 is false, so safety check should NOT trigger + if localCount > 5 { + t.Error("localCount should be <= 5 for this test case") + } + }) +} + // TestCountIssuesInContent tests the issue counting helper function (bd-7ch) func TestCountIssuesInContent(t *testing.T) { tests := []struct {