fix(merge): fix tombstone handling edge cases (bd-ki14, bd-ig5, bd-6x5, bd-1sn)
- bd-ki14: Preserve tombstones when other side implicitly deleted In merge3WayWithTTL(), implicit deletion cases now check if the remaining side is a tombstone and preserve it instead of dropping. - bd-ig5: Remove duplicate constants from merge package StatusTombstone, DefaultTombstoneTTL, and ClockSkewGrace now reference the types package to avoid duplication. - bd-6x5: Handle empty DeletedAt in mergeTombstones() Added explicit handling for edge cases where one or both tombstones have empty DeletedAt fields with deterministic behavior. - bd-1sn: Copy tombstone fields in mergeIssue() safety fallback When status becomes tombstone via mergeStatus safety fallback, tombstone fields are now copied from the appropriate side. Added comprehensive tests for all fixed edge cases. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Steve Yegge
@@ -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, ""
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user