johno/beads
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(doctor): include tombstones in getCurrentJSONLIDs to prevent corruption (#552)

Browse Source 
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>
This commit is contained in:
Steve Yegge
2025-12-14 17:33:07 -08:00
parent e5f185875e
commit 1e20d702f2
2 changed files with 28 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
cmd/bd/doctor/fix/deletions.go
View File

@@ -126,9 +126,12 @@ func getCurrentJSONLIDs(jsonlPath string) (map[string]bool, error) {
if err := json.Unmarshal(line, &issue); err != nil { if err := json.Unmarshal(line, &issue); err != nil {
continue continue
} }
// Skip tombstones - they represent migrated deletions and shouldn't // Include ALL issues including tombstones (bd-552 fix)
// be re-added to the deletions manifest (bd-in7q fix) // Tombstones represent migrated deletions that ARE accounted for.
if issue.ID != "" && issue.Status != "tombstone" { // 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 ids[issue.ID] = true
} }
} }

36
cmd/bd/doctor/fix/deletions_test.go
View File

@@ -9,7 +9,12 @@ import (
"github.com/steveyegge/beads/internal/types" "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 // Setup: Create temp file with mix of normal issues and tombstones
tmpDir := t.TempDir() tmpDir := t.TempDir()
jsonlPath := filepath.Join(tmpDir, "issues.jsonl") jsonlPath := filepath.Join(tmpDir, "issues.jsonl")
@@ -22,9 +27,9 @@ func TestGetCurrentJSONLIDs_SkipsTombstones(t *testing.T) {
Status: types.StatusOpen, Status: types.StatusOpen,
}, },
{ {
ID: "bd-def", ID: "bd-def",
Title: "(deleted)", Title: "(deleted)",
Status: types.StatusTombstone, Status: types.StatusTombstone,
DeletedBy: "test-user", DeletedBy: "test-user",
}, },
{ {
@@ -33,9 +38,9 @@ func TestGetCurrentJSONLIDs_SkipsTombstones(t *testing.T) {
Status: types.StatusOpen, Status: types.StatusOpen,
}, },
{ {
ID: "bd-jkl", ID: "bd-jkl",
Title: "(deleted)", Title: "(deleted)",
Status: types.StatusTombstone, Status: types.StatusTombstone,
DeletedBy: "test-user", DeletedBy: "test-user",
}, },
} }
@@ -59,10 +64,12 @@ func TestGetCurrentJSONLIDs_SkipsTombstones(t *testing.T) {
t.Fatalf("getCurrentJSONLIDs failed: %v", err) 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{ expectedIDs := map[string]bool{
"bd-abc": true, "bd-abc": true,
"bd-def": true, // tombstone - must be included
"bd-ghi": true, "bd-ghi": true,
"bd-jkl": true, // tombstone - must be included
} }
if len(ids) != len(expectedIDs) { if len(ids) != len(expectedIDs) {
@@ -75,12 +82,12 @@ func TestGetCurrentJSONLIDs_SkipsTombstones(t *testing.T) {
} }
} }
// Verify tombstones are NOT included // Verify tombstones ARE included (this is the bd-552 fix)
if ids["bd-def"] { if !ids["bd-def"] {
t.Error("Tombstone bd-def should not be included in current IDs") t.Error("Tombstone bd-def MUST be included in current IDs (bd-552 fix)")
} }
if ids["bd-jkl"] { if !ids["bd-jkl"] {
t.Error("Tombstone bd-jkl should not be included in current IDs") 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. // 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. // Integration tests are handled in migrate_tombstones_test.go with full sync cycle.