fix: multiple safety check and sync improvements

- bd-dtm: Changed stderr printing to use SafetyWarnings in worktree.go - bd-ciu: Fixed non-deterministic output order in formatVanishedIssues - bd-dmd: Removed duplicate safety check message in sync.go - bd-k2n: PushSyncBranch now recreates worktree if cleaned up - bd-c5m: Fixed string(rune()) in tests to use strconv.Itoa - bd-8uk: Added test for SafetyWarnings population - bd-1kf: Fixed mergePriority to handle negative priorities - bd-xo9: Documented sync.require_confirmation_on_mass_delete config 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
2025-12-02 22:14:41 -08:00
parent f531691440
commit a067796055
6 changed files with 110 additions and 21 deletions
--- a/cmd/bd/sync.go
+++ b/cmd/bd/sync.go
@@ -400,8 +400,8 @@ Use --merge to merge the sync branch back to main branch.`,

 							// bd-4u8: Handle safety check with confirmation requirement
 							if pullResult.SafetyCheckTriggered && !pullResult.Pushed {
+								// bd-dmd: Don't duplicate SafetyCheckDetails - it's already in SafetyWarnings
 								// 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
--- a/docs/CONFIG.md
+++ b/docs/CONFIG.md
@@ -186,6 +186,8 @@ Configuration keys use dot-notation namespaces to organize settings:
 - `export.skip_encoding_errors` - Skip issues that fail JSON encoding (default: false)
 - `export.write_manifest` - Write .manifest.json with export metadata (default: false)
 - `auto_export.error_policy` - Override error policy for auto-exports (default: `best-effort`)
+- `sync.branch` - Name of the dedicated sync branch for beads data (see docs/PROTECTED_BRANCHES.md)
+- `sync.require_confirmation_on_mass_delete` - Require interactive confirmation before pushing when >50% of issues vanish during a merge (default: `false`)

 ### Integration Namespaces

@@ -325,6 +327,33 @@ bd sync  # Respects import.orphan_handling setting
 - Use `strict` only for controlled imports where you need to guarantee parent existence
 - Use `skip` rarely - only when you want to selectively import a subset

+### Example: Sync Safety Options
+
+Controls for the sync branch workflow (see docs/PROTECTED_BRANCHES.md):
+
+```bash
+# Configure sync branch (required for protected branch workflow)
+bd config set sync.branch beads-metadata
+
+# Enable mass deletion protection (optional, default: false)
+# When enabled, if >50% of issues vanish during a merge, bd sync will:
+# 1. Show forensic info about vanished issues
+# 2. Prompt for confirmation before pushing
+bd config set sync.require_confirmation_on_mass_delete "true"
+```
+
+**When to enable `sync.require_confirmation_on_mass_delete`:**
+
+- Multi-user workflows where accidental mass deletions could propagate
+- Critical projects where data loss prevention is paramount
+- When you want manual review before pushing large changes
+
+**When to keep it disabled (default):**
+
+- Single-user workflows where you trust your local changes
+- CI/CD pipelines that need non-interactive sync
+- When you want hands-free automation
+
 ### Example: Jira Integration

 ```bash
--- a/internal/merge/merge.go
+++ b/internal/merge/merge.go
@@ -427,7 +427,7 @@ 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)
+// Any explicitly set priority (!=0) wins over 0. (bd-d0t fix, bd-1kf fix)
 func mergePriority(base, left, right int) int {
 	// Standard 3-way merge for non-conflict cases
 	if base == left && base != right {
@@ -442,10 +442,11 @@ func mergePriority(base, left, right int) int {
 	// True conflict: both sides changed to different values

 	// bd-d0t fix: Treat 0 as "unset" - explicitly set priority wins over unset
-	if left == 0 && right > 0 {
+	// bd-1kf fix: Use != 0 instead of > 0 to handle negative priorities
+	if left == 0 && right != 0 {
 		return right // right has explicit priority, left is unset
 	}
-	if right == 0 && left > 0 {
+	if right == 0 && left != 0 {
 		return left // left has explicit priority, right is unset
 	}

--- a/internal/merge/merge_test.go
+++ b/internal/merge/merge_test.go
@@ -563,6 +563,35 @@ func TestMergePriority(t *testing.T) {
 			right:    2,
 			expected: 2, // right changed from 0 to 2
 		},
+		// bd-1kf fix: negative priorities should be handled consistently
+		{
+			name:     "bd-1kf: negative priority should win over unset (0)",
+			base:     2,
+			left:     0,
+			right:    -1,
+			expected: -1, // negative priority is explicit, should win over unset
+		},
+		{
+			name:     "bd-1kf: negative priority on left should win over unset (0) on right",
+			base:     2,
+			left:     -1,
+			right:    0,
+			expected: -1, // negative priority is explicit, should win over unset
+		},
+		{
+			name:     "bd-1kf: conflict between negative priorities - lower wins",
+			base:     2,
+			left:     -2,
+			right:    -1,
+			expected: -2, // -2 is higher priority (more urgent) than -1
+		},
+		{
+			name:     "bd-1kf: negative vs positive priority conflict",
+			base:     2,
+			left:     -1,
+			right:    1,
+			expected: -1, // -1 is higher priority (lower number) than 1
+		},
 	}

 	for _, tt := range tests {
--- a/internal/syncbranch/worktree.go
+++ b/internal/syncbranch/worktree.go
@@ -7,6 +7,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -301,8 +302,9 @@ func PullFromSyncBranch(ctx context.Context, repoRoot, syncBranch, jsonlPath str
 	// bd-7ch: Extract local content before merge for safety check
 	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)
+		// bd-feh: Add warning to result so callers can display appropriately (bd-dtm fix)
+		result.SafetyWarnings = append(result.SafetyWarnings,
+			fmt.Sprintf("⚠️  Warning: Could not extract local content for safety check: %v", extractErr))
 	}

 	mergedContent, err := performContentMerge(ctx, worktreePath, syncBranch, remote, jsonlRelPath)
@@ -716,9 +718,10 @@ func PushSyncBranch(ctx context.Context, repoRoot, syncBranch string) error {
 	// Worktree path is under .git/beads-worktrees/<branch>
 	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)
+	// bd-k2n: Recreate worktree if it was cleaned up, using the same pattern as CommitToSyncBranch
+	wtMgr := git.NewWorktreeManager(repoRoot)
+	if err := wtMgr.CreateBeadsWorktree(syncBranch, worktreePath); err != nil {
+		return fmt.Errorf("failed to ensure worktree exists: %w", err)
 	}

 	return pushFromWorktree(ctx, worktreePath, syncBranch)
@@ -799,18 +802,23 @@ func formatVanishedIssues(localIssues, mergedIssues map[string]issueSummary, loc
 	lines = append(lines, fmt.Sprintf("   After merge:  %d issues", mergedCount))
 	lines = append(lines, "   Vanished issues:")

-	vanishedCount := 0
-	for id, summary := range localIssues {
+	// bd-ciu: Collect vanished IDs first, then sort for deterministic output
+	var vanishedIDs []string
+	for id := range localIssues {
 		if _, exists := mergedIssues[id]; !exists {
-			vanishedCount++
-			title := summary.Title
+			vanishedIDs = append(vanishedIDs, id)
+		}
+	}
+	sort.Strings(vanishedIDs)
+
+	for _, id := range vanishedIDs {
+		title := localIssues[id].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))
+	lines = append(lines, fmt.Sprintf("   Total vanished: %d\n", len(vanishedIDs)))

 	return lines
 }
--- a/internal/syncbranch/worktree_divergence_test.go
+++ b/internal/syncbranch/worktree_divergence_test.go
@@ -5,6 +5,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"testing"
 )
@@ -523,9 +524,10 @@ func TestSafetyCheckMassDeletion(t *testing.T) {
 	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
+		// bd-c5m: Use strconv.Itoa instead of string(rune()) which only works for single digits
 		var localLines []string
 		for i := 1; i <= 10; i++ {
-			localLines = append(localLines, `{"id":"test-`+string(rune('0'+i))+`","title":"Issue `+string(rune('0'+i))+`"}`)
+			localLines = append(localLines, `{"id":"test-`+strconv.Itoa(i)+`","title":"Issue `+strconv.Itoa(i)+`"}`)
 		}
 		localContent := []byte(strings.Join(localLines, "\n"))

@@ -561,20 +563,40 @@ func TestSafetyCheckMassDeletion(t *testing.T) {
 		if len(forensicLines) == 0 {
 			t.Error("formatVanishedIssues returned empty lines")
 		}
+
+		// bd-8uk: Verify SafetyWarnings would be populated correctly
+		// Simulate what PullFromSyncBranch does when safety check triggers
+		var safetyWarnings []string
+		safetyWarnings = append(safetyWarnings,
+			"⚠️  Warning: "+strconv.FormatFloat(vanishedPercent, 'f', 0, 64)+"% of issues vanished during merge ("+
+				strconv.Itoa(localCount)+" → "+strconv.Itoa(mergedCount)+" issues)")
+		safetyWarnings = append(safetyWarnings, forensicLines...)
+
+		// Verify warnings contains expected content
+		if len(safetyWarnings) < 2 {
+			t.Errorf("SafetyWarnings should have at least 2 entries (warning + forensics), got %d", len(safetyWarnings))
+		}
+		if !strings.Contains(safetyWarnings[0], "Warning") {
+			t.Error("First SafetyWarning should contain 'Warning'")
+		}
+		if !strings.Contains(safetyWarnings[0], "60%") {
+			t.Errorf("First SafetyWarning should contain '60%%', got: %s", safetyWarnings[0])
+		}
 	})

 	t.Run("safety check does NOT trigger when <50% issues vanish", func(t *testing.T) {
 		// 10 issues, 6 remain = 40% vanished (should NOT trigger)
+		// bd-c5m: Use strconv.Itoa instead of string(rune()) which only works for single digits
 		var localLines []string
 		for i := 1; i <= 10; i++ {
-			localLines = append(localLines, `{"id":"test-`+string(rune('0'+i))+`"}`)
+			localLines = append(localLines, `{"id":"test-`+strconv.Itoa(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))+`"}`)
+			mergedLines = append(mergedLines, `{"id":"test-`+strconv.Itoa(i)+`"}`)
 		}
 		mergedContent := []byte(strings.Join(mergedLines, "\n"))