feat(sync): add safety check enhancements and merge fixes

- Add forensic logging for mass deletions (bd-lsa): log vanished issue IDs and titles
- Add sync.require_confirmation_on_mass_delete config option (bd-4u8)
- Fix priority merge to treat 0 as "unset" (bd-d0t)
- Fix timestamp tie-breaker to prefer left/local (bd-8nz)
- Add warning log when extraction fails during safety check (bd-feh)
- Refactor safety warnings to return in PullResult (bd-7z4)
- Add TestSafetyCheckMassDeletion integration tests (bd-cnn)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Steve Yegge
2025-12-02 21:48:18 -08:00
parent c93b755344
commit f531691440
6 changed files with 443 additions and 18 deletions

View File

@@ -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
}

View File

@@ -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 {

View File

@@ -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 {

View File

@@ -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) {

View File

@@ -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/<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)
}
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")

View File

@@ -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 {