From 1e20d702f2aaa0b2137877570187a5e32630e357 Mon Sep 17 00:00:00 2001
From: Steve Yegge <steve.yegge@gmail.com>
Date: Sun, 14 Dec 2025 17:33:07 -0800
Subject: [PATCH] fix(doctor): include tombstones in getCurrentJSONLIDs to
 prevent corruption (#552)

The previous bd-in7q fix had backwards logic - by EXCLUDING tombstones
from currentIDs, they appeared missing when compared to historicalIDs,
causing HydrateDeletionsManifest to erroneously add them to deletions.jsonl.

This corruption manifested when:
1. Issues were migrated to tombstones via migrate-tombstones
2. Doctor hydration ran (directly or via sync)
3. Tombstones were seen as deleted and re-added to deletions.jsonl
4. Next import skipped thousands of issues with in deletions manifest

Fix: Include ALL issues (including tombstones) in currentIDs. Tombstones
represent migrated deletions that ARE accounted for - they should not
trigger new deletion records.

Generated with Claude Code

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
---
 cmd/bd/doctor/fix/deletions.go      |  9 +++++---
 cmd/bd/doctor/fix/deletions_test.go | 36 ++++++++++++++++++-----------
 2 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/cmd/bd/doctor/fix/deletions.go b/cmd/bd/doctor/fix/deletions.go
index df587fdd..c0831a25 100644
--- a/cmd/bd/doctor/fix/deletions.go
+++ b/cmd/bd/doctor/fix/deletions.go
@@ -126,9 +126,12 @@ func getCurrentJSONLIDs(jsonlPath string) (map[string]bool, error) {
 		if err := json.Unmarshal(line, &issue); err != nil {
 			continue
 		}
-		// Skip tombstones - they represent migrated deletions and shouldn't
-		// be re-added to the deletions manifest (bd-in7q fix)
-		if issue.ID != "" && issue.Status != "tombstone" {
+		// Include ALL issues including tombstones (bd-552 fix)
+		// Tombstones represent migrated deletions that ARE accounted for.
+		// By including them in currentIDs, they won't appear "missing" when
+		// compared to historicalIDs, preventing erroneous re-addition to
+		// deletions.jsonl. The previous bd-in7q fix had backwards logic.
+		if issue.ID != "" {
 			ids[issue.ID] = true
 		}
 	}
diff --git a/cmd/bd/doctor/fix/deletions_test.go b/cmd/bd/doctor/fix/deletions_test.go
index a8eb11af..5155ed0b 100644
--- a/cmd/bd/doctor/fix/deletions_test.go
+++ b/cmd/bd/doctor/fix/deletions_test.go
@@ -9,7 +9,12 @@ import (
 	"github.com/steveyegge/beads/internal/types"
 )
 
-func TestGetCurrentJSONLIDs_SkipsTombstones(t *testing.T) {
+// TestGetCurrentJSONLIDs_IncludesTombstones verifies that tombstones ARE included
+// in the current ID set. This is critical for bd-552 fix: tombstones represent
+// migrated deletions that are accounted for. By including them, they won't appear
+// "missing" when compared to historicalIDs, preventing erroneous re-addition to
+// deletions.jsonl.
+func TestGetCurrentJSONLIDs_IncludesTombstones(t *testing.T) {
 	// Setup: Create temp file with mix of normal issues and tombstones
 	tmpDir := t.TempDir()
 	jsonlPath := filepath.Join(tmpDir, "issues.jsonl")
@@ -22,9 +27,9 @@ func TestGetCurrentJSONLIDs_SkipsTombstones(t *testing.T) {
 			Status: types.StatusOpen,
 		},
 		{
-			ID:       "bd-def",
-			Title:    "(deleted)",
-			Status:   types.StatusTombstone,
+			ID:        "bd-def",
+			Title:     "(deleted)",
+			Status:    types.StatusTombstone,
 			DeletedBy: "test-user",
 		},
 		{
@@ -33,9 +38,9 @@ func TestGetCurrentJSONLIDs_SkipsTombstones(t *testing.T) {
 			Status: types.StatusOpen,
 		},
 		{
-			ID:       "bd-jkl",
-			Title:    "(deleted)",
-			Status:   types.StatusTombstone,
+			ID:        "bd-jkl",
+			Title:     "(deleted)",
+			Status:    types.StatusTombstone,
 			DeletedBy: "test-user",
 		},
 	}
@@ -59,10 +64,12 @@ func TestGetCurrentJSONLIDs_SkipsTombstones(t *testing.T) {
 		t.Fatalf("getCurrentJSONLIDs failed: %v", err)
 	}
 
-	// Verify: Should only contain non-tombstone IDs
+	// Verify: Should contain ALL IDs including tombstones (bd-552 fix)
 	expectedIDs := map[string]bool{
 		"bd-abc": true,
+		"bd-def": true, // tombstone - must be included
 		"bd-ghi": true,
+		"bd-jkl": true, // tombstone - must be included
 	}
 
 	if len(ids) != len(expectedIDs) {
@@ -75,12 +82,12 @@ func TestGetCurrentJSONLIDs_SkipsTombstones(t *testing.T) {
 		}
 	}
 
-	// Verify tombstones are NOT included
-	if ids["bd-def"] {
-		t.Error("Tombstone bd-def should not be included in current IDs")
+	// Verify tombstones ARE included (this is the bd-552 fix)
+	if !ids["bd-def"] {
+		t.Error("Tombstone bd-def MUST be included in current IDs (bd-552 fix)")
 	}
-	if ids["bd-jkl"] {
-		t.Error("Tombstone bd-jkl should not be included in current IDs")
+	if !ids["bd-jkl"] {
+		t.Error("Tombstone bd-jkl MUST be included in current IDs (bd-552 fix)")
 	}
 }
 
@@ -144,5 +151,6 @@ invalid json line
 }
 
 // Note: Full integration test for HydrateDeletionsManifest would require git repo setup.
-// The unit tests above verify the core fix (skipping tombstones in getCurrentJSONLIDs).
+// The unit tests above verify the core fix (bd-552: including tombstones in getCurrentJSONLIDs
+// so they aren't erroneously re-added to deletions.jsonl).
 // Integration tests are handled in migrate_tombstones_test.go with full sync cycle.