From fcd29f5ea48fea8c5158323cc1bad50a0c7fdec2 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Tue, 16 Dec 2025 22:35:29 -0800 Subject: [PATCH] fix: add ID-based fallback matching to prevent ghost resurrection (bd-ncwo) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: The merge driver matched issues by IssueKey (ID+CreatedAt+CreatedBy). When timestamp precision differed (e.g., with/without nanoseconds), the same issue was treated as two different issues, causing both tombstone and closed versions to appear in the merge result. Fix: Added ID-based fallback matching in merge3WayWithTTL. When key-based matching fails but the same ID exists in the other side, use that for merging. Also track processed IDs to prevent duplicates. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- internal/merge/merge.go | 55 ++++++++++++- internal/merge/merge_test.go | 148 +++++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 2 deletions(-) diff --git a/internal/merge/merge.go b/internal/merge/merge.go index 083f5bee..e54cb872 100644 --- a/internal/merge/merge.go +++ b/internal/merge/merge.go @@ -298,7 +298,7 @@ func merge3Way(base, left, right []Issue) ([]Issue, []string) { // merge3WayWithTTL performs a 3-way merge with configurable tombstone TTL. // This is the core merge function that handles tombstone semantics. func merge3WayWithTTL(base, left, right []Issue, ttl time.Duration) ([]Issue, []string) { - // Build maps for quick lookup + // Build maps for quick lookup by IssueKey baseMap := make(map[IssueKey]Issue) for _, issue := range base { baseMap[makeKey(issue)] = issue @@ -314,8 +314,22 @@ func merge3WayWithTTL(base, left, right []Issue, ttl time.Duration) ([]Issue, [] rightMap[makeKey(issue)] = issue } - // Track which issues we've processed + // bd-ncwo: Also build ID-based maps for fallback matching + // This handles cases where the same issue has slightly different CreatedAt/CreatedBy + // (e.g., due to timestamp precision differences between systems) + leftByID := make(map[string]Issue) + for _, issue := range left { + leftByID[issue.ID] = issue + } + + rightByID := make(map[string]Issue) + for _, issue := range right { + rightByID[issue.ID] = issue + } + + // Track which issues we've processed (by both key and ID) processed := make(map[IssueKey]bool) + processedIDs := make(map[string]bool) // bd-ncwo: track processed IDs to avoid duplicates var result []Issue var conflicts []string @@ -341,6 +355,43 @@ func merge3WayWithTTL(base, left, right []Issue, ttl time.Duration) ([]Issue, [] leftIssue, inLeft := leftMap[key] rightIssue, inRight := rightMap[key] + // bd-ncwo: ID-based fallback matching for tombstone preservation + // If key doesn't match but same ID exists in the other side, use that + if !inLeft && inRight { + if fallback, found := leftByID[rightIssue.ID]; found { + leftIssue = fallback + inLeft = true + // Mark the fallback's key as processed to avoid duplicate + processed[makeKey(fallback)] = true + } + } + if !inRight && inLeft { + if fallback, found := rightByID[leftIssue.ID]; found { + rightIssue = fallback + inRight = true + // Mark the fallback's key as processed to avoid duplicate + processed[makeKey(fallback)] = true + } + } + + // bd-ncwo: Check if we've already processed this ID (via a different key) + currentID := key.ID + if currentID == "" { + if inLeft { + currentID = leftIssue.ID + } else if inRight { + currentID = rightIssue.ID + } else if inBase { + currentID = baseIssue.ID + } + } + if currentID != "" && processedIDs[currentID] { + continue + } + if currentID != "" { + processedIDs[currentID] = true + } + // Determine tombstone status leftTombstone := inLeft && IsTombstone(leftIssue) rightTombstone := inRight && IsTombstone(rightIssue) diff --git a/internal/merge/merge_test.go b/internal/merge/merge_test.go index 88b157d2..0c042e88 100644 --- a/internal/merge/merge_test.go +++ b/internal/merge/merge_test.go @@ -2202,3 +2202,151 @@ func TestMerge3Way_TombstoneBaseBothLiveResurrection(t *testing.T) { } }) } + +// TestMerge3Way_TombstoneVsLiveTimestampPrecisionMismatch tests bd-ncwo: +// When the same issue has different CreatedAt timestamp precision (e.g., with/without nanoseconds), +// the tombstone should still win over the live version. +func TestMerge3Way_TombstoneVsLiveTimestampPrecisionMismatch(t *testing.T) { + // This test simulates the ghost resurrection bug where timestamp precision + // differences caused the same issue to be treated as two different issues. + // The key fix (bd-ncwo) adds ID-based fallback matching when keys don't match. + + t.Run("tombstone wins despite different CreatedAt precision", func(t *testing.T) { + // Base: issue with status=closed + baseIssue := Issue{ + ID: "bd-ghost1", + Title: "Original title", + Status: "closed", + Priority: 2, + CreatedAt: "2024-01-01T00:00:00Z", // No fractional seconds + UpdatedAt: "2024-01-10T00:00:00Z", + CreatedBy: "user1", + } + + // Left: tombstone with DIFFERENT timestamp precision (has microseconds) + tombstone := Issue{ + ID: "bd-ghost1", + Title: "(deleted)", + Status: StatusTombstone, + Priority: 2, + CreatedAt: "2024-01-01T00:00:00.000000Z", // WITH fractional seconds + UpdatedAt: "2024-01-15T00:00:00Z", + CreatedBy: "user1", + DeletedAt: time.Now().Add(-24 * time.Hour).Format(time.RFC3339), + DeletedBy: "user2", + DeleteReason: "Duplicate issue", + } + + // Right: same closed issue (same precision as base) + closedIssue := Issue{ + ID: "bd-ghost1", + Title: "Original title", + Status: "closed", + Priority: 2, + CreatedAt: "2024-01-01T00:00:00Z", // No fractional seconds + UpdatedAt: "2024-01-12T00:00:00Z", + CreatedBy: "user1", + } + + base := []Issue{baseIssue} + left := []Issue{tombstone} + right := []Issue{closedIssue} + + result, conflicts := merge3Way(base, left, right) + + if len(conflicts) != 0 { + t.Errorf("unexpected conflicts: %v", conflicts) + } + + // CRITICAL: Should have exactly 1 issue, not 2 (no duplicates) + if len(result) != 1 { + t.Fatalf("expected 1 issue (no duplicates), got %d - this suggests ID-based matching failed", len(result)) + } + + // Tombstone should win over closed + if result[0].Status != StatusTombstone { + t.Errorf("expected tombstone to win, 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("tombstone wins with CreatedBy mismatch", func(t *testing.T) { + // Test case where CreatedBy differs (e.g., empty vs populated) + tombstone := Issue{ + ID: "bd-ghost2", + Title: "(deleted)", + Status: StatusTombstone, + Priority: 2, + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "", // Empty CreatedBy + DeletedAt: time.Now().Add(-24 * time.Hour).Format(time.RFC3339), + DeletedBy: "user2", + DeleteReason: "Cleanup", + } + + closedIssue := Issue{ + ID: "bd-ghost2", + Title: "Original title", + Status: "closed", + Priority: 2, + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", // Non-empty CreatedBy + } + + base := []Issue{} + left := []Issue{tombstone} + right := []Issue{closedIssue} + + result, conflicts := merge3Way(base, left, right) + + if len(conflicts) != 0 { + t.Errorf("unexpected conflicts: %v", conflicts) + } + + // Should have exactly 1 issue + if len(result) != 1 { + t.Fatalf("expected 1 issue (no duplicates), got %d", len(result)) + } + + // Tombstone should win + if result[0].Status != StatusTombstone { + t.Errorf("expected tombstone to win despite CreatedBy mismatch, got status %q", result[0].Status) + } + }) + + t.Run("no duplicates when both have same ID but different keys", func(t *testing.T) { + // Ensure we don't create duplicate entries + liveLeft := Issue{ + ID: "bd-ghost3", + Title: "Left version", + Status: "open", + CreatedAt: "2024-01-01T00:00:00.123456Z", // With nanoseconds + CreatedBy: "user1", + } + + liveRight := Issue{ + ID: "bd-ghost3", + Title: "Right version", + Status: "in_progress", + CreatedAt: "2024-01-01T00:00:00Z", // Without nanoseconds + CreatedBy: "user1", + } + + base := []Issue{} + left := []Issue{liveLeft} + right := []Issue{liveRight} + + result, conflicts := merge3Way(base, left, right) + + if len(conflicts) != 0 { + t.Errorf("unexpected conflicts: %v", conflicts) + } + + // CRITICAL: Should have exactly 1 issue, not 2 + if len(result) != 1 { + t.Fatalf("expected 1 issue (no duplicates for same ID), got %d", len(result)) + } + }) +}