diff --git a/internal/merge/merge.go b/internal/merge/merge.go index ba7d0a76..083f5bee 100644 --- a/internal/merge/merge.go +++ b/internal/merge/merge.go @@ -33,6 +33,8 @@ import ( "fmt" "os" "time" + + "github.com/steveyegge/beads/internal/types" ) // Issue represents a beads issue with all possible fields @@ -238,14 +240,14 @@ func makeKey(issue Issue) IssueKey { } } -// StatusTombstone is the status value for soft-deleted issues -const StatusTombstone = "tombstone" +// bd-ig5: Use constants from types package to avoid duplication +const StatusTombstone = string(types.StatusTombstone) -// DefaultTombstoneTTL is the default time-to-live for tombstones (30 days) -const DefaultTombstoneTTL = 30 * 24 * time.Hour - -// ClockSkewGrace is added to TTL to handle clock drift between machines -const ClockSkewGrace = 1 * time.Hour +// Alias TTL constants from types package for local use +var ( + DefaultTombstoneTTL = types.DefaultTombstoneTTL + ClockSkewGrace = types.ClockSkewGrace +) // IsTombstone returns true if the issue has been soft-deleted func IsTombstone(issue Issue) bool { @@ -425,11 +427,21 @@ func merge3WayWithTTL(base, left, right []Issue, ttl time.Duration) ([]Issue, [] result = append(result, merged) } else if inBase && inLeft && !inRight { // Deleted in right (implicitly), maybe modified in left + // bd-ki14: Check if left is a tombstone - tombstones must be preserved + if leftTombstone { + result = append(result, leftIssue) + continue + } // RULE 2: deletion always wins over modification // This is because deletion is an explicit action that should be preserved continue } else if inBase && !inLeft && inRight { // Deleted in left (implicitly), maybe modified in right + // bd-ki14: Check if right is a tombstone - tombstones must be preserved + if rightTombstone { + result = append(result, rightIssue) + continue + } // RULE 2: deletion always wins over modification // This is because deletion is an explicit action that should be preserved continue @@ -447,8 +459,29 @@ func merge3WayWithTTL(base, left, right []Issue, ttl time.Duration) ([]Issue, [] // mergeTombstones merges two tombstones for the same issue. // The tombstone with the later deleted_at timestamp wins. +// +// bd-6x5: Edge cases for empty DeletedAt: +// - If both empty: left wins (arbitrary but deterministic) +// - If left empty, right not: right wins (has timestamp) +// - If right empty, left not: left wins (has timestamp) +// +// Empty DeletedAt shouldn't happen in valid data (validation catches it), +// but we handle it defensively here. func mergeTombstones(left, right Issue) Issue { - // Use later deleted_at as the authoritative tombstone + // bd-6x5: Handle empty DeletedAt explicitly for clarity + if left.DeletedAt == "" && right.DeletedAt == "" { + // Both invalid - left wins as tie-breaker + return left + } + if left.DeletedAt == "" { + // Left invalid, right valid - right wins + return right + } + if right.DeletedAt == "" { + // Right invalid, left valid - left wins + return left + } + // Both valid - use later deleted_at as the authoritative tombstone if isTimeAfter(left.DeletedAt, right.DeletedAt) { return left } @@ -494,6 +527,30 @@ func mergeIssue(base, left, right Issue) (Issue, string) { // Merge dependencies - combine and deduplicate result.Dependencies = mergeDependencies(left.Dependencies, right.Dependencies) + // bd-1sn: If status became tombstone via mergeStatus safety fallback, + // copy tombstone fields from whichever side has them + if result.Status == StatusTombstone { + // Prefer the side with more recent deleted_at, or left if tied + if isTimeAfter(left.DeletedAt, right.DeletedAt) { + result.DeletedAt = left.DeletedAt + result.DeletedBy = left.DeletedBy + result.DeleteReason = left.DeleteReason + result.OriginalType = left.OriginalType + } else if right.DeletedAt != "" { + result.DeletedAt = right.DeletedAt + result.DeletedBy = right.DeletedBy + result.DeleteReason = right.DeleteReason + result.OriginalType = right.OriginalType + } else if left.DeletedAt != "" { + result.DeletedAt = left.DeletedAt + result.DeletedBy = left.DeletedBy + result.DeleteReason = left.DeleteReason + result.OriginalType = left.OriginalType + } + // Note: if neither has DeletedAt, tombstone fields remain empty + // This represents invalid data that validation should catch + } + // All field conflicts are now auto-resolved deterministically return result, "" } diff --git a/internal/merge/merge_test.go b/internal/merge/merge_test.go index 8b1677ba..5c5c0ab3 100644 --- a/internal/merge/merge_test.go +++ b/internal/merge/merge_test.go @@ -1684,3 +1684,224 @@ func TestMergeStatus_Tombstone(t *testing.T) { }) } } + +// TestMerge3Way_TombstoneWithImplicitDeletion tests bd-ki14 fix: +// tombstones should be preserved even when the other side implicitly deleted +func TestMerge3Way_TombstoneWithImplicitDeletion(t *testing.T) { + baseIssue := Issue{ + ID: "bd-abc123", + Title: "Original", + Status: "open", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + } + + tombstone := Issue{ + ID: "bd-abc123", + Title: "Original", + Status: StatusTombstone, + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + DeletedAt: time.Now().Add(-24 * time.Hour).Format(time.RFC3339), + DeletedBy: "user2", + DeleteReason: "Duplicate", + } + + t.Run("bd-ki14: tombstone in left preserved when right implicitly deleted", func(t *testing.T) { + base := []Issue{baseIssue} + left := []Issue{tombstone} + right := []Issue{} // Implicitly deleted in right + + result, conflicts := merge3Way(base, left, right) + if len(conflicts) != 0 { + t.Errorf("unexpected conflicts: %v", conflicts) + } + if len(result) != 1 { + t.Fatalf("expected 1 issue (tombstone preserved), got %d", len(result)) + } + if result[0].Status != StatusTombstone { + t.Errorf("expected tombstone to be preserved, got status %q", result[0].Status) + } + if result[0].DeletedBy != "user2" { + t.Errorf("expected tombstone fields preserved, got DeletedBy %q", result[0].DeletedBy) + } + }) + + t.Run("bd-ki14: tombstone in right preserved when left implicitly deleted", func(t *testing.T) { + base := []Issue{baseIssue} + left := []Issue{} // Implicitly deleted in left + right := []Issue{tombstone} + + result, conflicts := merge3Way(base, left, right) + if len(conflicts) != 0 { + t.Errorf("unexpected conflicts: %v", conflicts) + } + if len(result) != 1 { + t.Fatalf("expected 1 issue (tombstone preserved), got %d", len(result)) + } + if result[0].Status != StatusTombstone { + t.Errorf("expected tombstone to be preserved, got status %q", result[0].Status) + } + }) + + t.Run("bd-ki14: live issue in left still deleted when right implicitly deleted", func(t *testing.T) { + base := []Issue{baseIssue} + modifiedLive := Issue{ + ID: "bd-abc123", + Title: "Modified", + Status: "in_progress", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + } + left := []Issue{modifiedLive} + right := []Issue{} // Implicitly deleted in right + + result, conflicts := merge3Way(base, left, right) + if len(conflicts) != 0 { + t.Errorf("unexpected conflicts: %v", conflicts) + } + // Live issue should be deleted (implicit deletion wins for non-tombstones) + if len(result) != 0 { + t.Errorf("expected implicit deletion to win for live issue, got %d results", len(result)) + } + }) +} + +// TestMergeTombstones_EmptyDeletedAt tests bd-6x5 fix: +// handling empty DeletedAt timestamps in tombstone merging +func TestMergeTombstones_EmptyDeletedAt(t *testing.T) { + tests := []struct { + name string + leftDeletedAt string + rightDeletedAt string + expectedSide string // "left" or "right" + }{ + { + name: "bd-6x5: both empty - left wins as tie-breaker", + leftDeletedAt: "", + rightDeletedAt: "", + expectedSide: "left", + }, + { + name: "bd-6x5: left empty, right valid - right wins", + leftDeletedAt: "", + rightDeletedAt: "2024-01-01T00:00:00Z", + expectedSide: "right", + }, + { + name: "bd-6x5: left valid, right empty - left wins", + leftDeletedAt: "2024-01-01T00:00:00Z", + rightDeletedAt: "", + expectedSide: "left", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + left := Issue{ + ID: "bd-test", + Status: StatusTombstone, + DeletedAt: tt.leftDeletedAt, + DeletedBy: "user-left", + } + right := Issue{ + ID: "bd-test", + Status: StatusTombstone, + DeletedAt: tt.rightDeletedAt, + DeletedBy: "user-right", + } + result := mergeTombstones(left, right) + if tt.expectedSide == "left" && result.DeletedBy != "user-left" { + t.Errorf("expected left tombstone to win, got DeletedBy %q", result.DeletedBy) + } + if tt.expectedSide == "right" && result.DeletedBy != "user-right" { + t.Errorf("expected right tombstone to win, got DeletedBy %q", result.DeletedBy) + } + }) + } +} + +// TestMergeIssue_TombstoneFields tests bd-1sn fix: +// tombstone fields should be copied when status becomes tombstone via safety fallback +func TestMergeIssue_TombstoneFields(t *testing.T) { + t.Run("bd-1sn: tombstone fields copied from left when tombstone via mergeStatus", func(t *testing.T) { + base := Issue{ + ID: "bd-test", + Status: "open", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + } + left := Issue{ + ID: "bd-test", + Status: StatusTombstone, + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + DeletedAt: "2024-01-02T00:00:00Z", + DeletedBy: "user2", + DeleteReason: "Duplicate", + OriginalType: "task", + } + right := Issue{ + ID: "bd-test", + Status: "open", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + } + + result, _ := mergeIssue(base, left, right) + if result.Status != StatusTombstone { + t.Errorf("expected tombstone status, got %q", result.Status) + } + if result.DeletedAt != "2024-01-02T00:00:00Z" { + t.Errorf("expected DeletedAt to be copied, got %q", result.DeletedAt) + } + if result.DeletedBy != "user2" { + t.Errorf("expected DeletedBy to be copied, got %q", result.DeletedBy) + } + if result.DeleteReason != "Duplicate" { + t.Errorf("expected DeleteReason to be copied, got %q", result.DeleteReason) + } + if result.OriginalType != "task" { + t.Errorf("expected OriginalType to be copied, got %q", result.OriginalType) + } + }) + + t.Run("bd-1sn: tombstone fields copied from right when it has later deleted_at", func(t *testing.T) { + base := Issue{ + ID: "bd-test", + Status: "open", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + } + left := Issue{ + ID: "bd-test", + Status: StatusTombstone, + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + DeletedAt: "2024-01-02T00:00:00Z", + DeletedBy: "user-left", + DeleteReason: "Left reason", + } + right := Issue{ + ID: "bd-test", + Status: StatusTombstone, + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + DeletedAt: "2024-01-03T00:00:00Z", // Later + DeletedBy: "user-right", + DeleteReason: "Right reason", + } + + result, _ := mergeIssue(base, left, right) + if result.Status != StatusTombstone { + t.Errorf("expected tombstone status, got %q", result.Status) + } + // Right has later deleted_at, so right's fields should be used + if result.DeletedBy != "user-right" { + t.Errorf("expected DeletedBy from right (later), got %q", result.DeletedBy) + } + if result.DeleteReason != "Right reason" { + t.Errorf("expected DeleteReason from right, got %q", result.DeleteReason) + } + }) +}