From 8c450692280f7f39ddaae32b26489d1b397f11a0 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Tue, 16 Dec 2025 00:42:04 -0800 Subject: [PATCH] fix(doctor,sync): clean up deletions manifest and reduce sync noise MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - bd-8v5o: When doctor --fix hydrates issues from git history, also remove them from the deletions manifest to prevent perpetual skip warnings during sync - bd-wsqt: Remove verbose per-issue "Skipping bd-xxx" messages during sync. Caller already shows summary of skipped issues. Added RemoveDeletions() function to deletions package with tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- cmd/bd/doctor/fix/migrate.go | 28 ++++ cmd/bd/doctor/fix/sync.go | 109 +++++++++++++ internal/deletions/deletions.go | 62 ++++++++ internal/deletions/deletions_test.go | 219 +++++++++++++++++++++++++++ internal/importer/importer.go | 5 +- 5 files changed, 420 insertions(+), 3 deletions(-) diff --git a/cmd/bd/doctor/fix/migrate.go b/cmd/bd/doctor/fix/migrate.go index 67f1122f..0bfa9405 100644 --- a/cmd/bd/doctor/fix/migrate.go +++ b/cmd/bd/doctor/fix/migrate.go @@ -36,6 +36,18 @@ func DatabaseVersion(path string) error { if err := cmd.Run(); err != nil { return fmt.Errorf("failed to initialize database: %w", err) } + + // bd-8v5o: Clean up deletions manifest for hydrated issues + // After init, remove any issues from deletions.jsonl that exist in JSONL + // This prevents perpetual "Skipping bd-xxx (in deletions manifest)" warnings + jsonlPath := findJSONLPath(beadsDir) + if jsonlPath != "" { + if err := cleanupDeletionsManifest(beadsDir, jsonlPath); err != nil { + // Non-fatal - just log warning + fmt.Printf(" Warning: failed to clean up deletions manifest: %v\n", err) + } + } + return nil } @@ -52,6 +64,22 @@ func DatabaseVersion(path string) error { return nil } +// findJSONLPath returns the path to the JSONL file in the beads directory. +// Returns empty string if no JSONL file exists. +func findJSONLPath(beadsDir string) string { + jsonlPath := filepath.Join(beadsDir, "issues.jsonl") + if _, err := os.Stat(jsonlPath); err == nil { + return jsonlPath + } + + beadsJSONLPath := filepath.Join(beadsDir, "beads.jsonl") + if _, err := os.Stat(beadsJSONLPath); err == nil { + return beadsJSONLPath + } + + return "" +} + // SchemaCompatibility fixes schema compatibility issues by running bd migrate func SchemaCompatibility(path string) error { return DatabaseVersion(path) diff --git a/cmd/bd/doctor/fix/sync.go b/cmd/bd/doctor/fix/sync.go index fa97d465..63dc7285 100644 --- a/cmd/bd/doctor/fix/sync.go +++ b/cmd/bd/doctor/fix/sync.go @@ -1,10 +1,14 @@ package fix import ( + "bufio" + "encoding/json" "fmt" "os" "os/exec" "path/filepath" + + "github.com/steveyegge/beads/internal/deletions" ) // DBJSONLSync fixes database-JSONL sync issues by running bd sync --import-only @@ -27,10 +31,13 @@ func DBJSONLSync(path string) error { } hasJSONL := false + actualJSONLPath := "" if _, err := os.Stat(jsonlPath); err == nil { hasJSONL = true + actualJSONLPath = jsonlPath } else if _, err := os.Stat(beadsJSONLPath); err == nil { hasJSONL = true + actualJSONLPath = beadsJSONLPath } if !hasDB || !hasJSONL { @@ -54,5 +61,107 @@ func DBJSONLSync(path string) error { return fmt.Errorf("failed to sync database with JSONL: %w", err) } + // bd-8v5o: Clean up deletions manifest for hydrated issues + // After sync, remove any issues from deletions.jsonl that exist in JSONL + // This prevents perpetual "Skipping bd-xxx (in deletions manifest)" warnings + if err := cleanupDeletionsManifest(beadsDir, actualJSONLPath); err != nil { + // Non-fatal - just log warning + fmt.Printf(" Warning: failed to clean up deletions manifest: %v\n", err) + } + return nil } + +// cleanupDeletionsManifest removes issues from deletions.jsonl that exist in JSONL. +// This is needed because when issues are hydrated from git history (e.g., via bd init +// or bd sync --import-only), they may still be in the deletions manifest from a +// previous deletion. This causes perpetual skip warnings during sync. +func cleanupDeletionsManifest(beadsDir, jsonlPath string) error { + deletionsPath := deletions.DefaultPath(beadsDir) + + // Check if deletions manifest exists + if _, err := os.Stat(deletionsPath); os.IsNotExist(err) { + return nil // No deletions manifest, nothing to clean up + } + + // Load deletions manifest + loadResult, err := deletions.LoadDeletions(deletionsPath) + if err != nil { + return fmt.Errorf("failed to load deletions manifest: %w", err) + } + + if len(loadResult.Records) == 0 { + return nil // No deletions, nothing to clean up + } + + // Get IDs from JSONL (excluding tombstones) + jsonlIDs, err := getNonTombstoneJSONLIDs(jsonlPath) + if err != nil { + return fmt.Errorf("failed to read JSONL: %w", err) + } + + // Find IDs that are in both deletions manifest and JSONL + var idsToRemove []string + for id := range loadResult.Records { + if jsonlIDs[id] { + idsToRemove = append(idsToRemove, id) + } + } + + if len(idsToRemove) == 0 { + return nil // No conflicting entries + } + + // Remove conflicting entries from deletions manifest + result, err := deletions.RemoveDeletions(deletionsPath, idsToRemove) + if err != nil { + return fmt.Errorf("failed to remove deletions: %w", err) + } + + if result.RemovedCount > 0 { + fmt.Printf(" Removed %d issue(s) from deletions manifest (now hydrated in JSONL)\n", result.RemovedCount) + } + + return nil +} + +// getNonTombstoneJSONLIDs reads the JSONL file and returns a set of IDs +// that are not tombstones (status != "tombstone"). +func getNonTombstoneJSONLIDs(jsonlPath string) (map[string]bool, error) { + ids := make(map[string]bool) + + file, err := os.Open(jsonlPath) // #nosec G304 - path validated by caller + if err != nil { + if os.IsNotExist(err) { + return ids, nil + } + return nil, err + } + defer func() { + _ = file.Close() + }() + + scanner := bufio.NewScanner(file) + scanner.Buffer(make([]byte, 0, 64*1024), 10*1024*1024) + + for scanner.Scan() { + line := scanner.Bytes() + if len(line) == 0 { + continue + } + + var issue struct { + ID string `json:"id"` + Status string `json:"status"` + } + if err := json.Unmarshal(line, &issue); err != nil { + continue + } + // Only include non-tombstone issues + if issue.ID != "" && issue.Status != "tombstone" { + ids[issue.ID] = true + } + } + + return ids, scanner.Err() +} diff --git a/internal/deletions/deletions.go b/internal/deletions/deletions.go index 2a965728..0590a310 100644 --- a/internal/deletions/deletions.go +++ b/internal/deletions/deletions.go @@ -266,3 +266,65 @@ func PruneDeletions(path string, retentionDays int) (*PruneResult, error) { return result, nil } + +// RemoveResult contains the result of a remove operation. +type RemoveResult struct { + RemovedCount int + RemovedIDs []string + KeptCount int +} + +// RemoveDeletions removes specific IDs from the deletions manifest. +// This is used when issues are hydrated from git history to prevent +// perpetual skip warnings during sync. +// If the file doesn't exist or is empty, returns zero counts with no error. +func RemoveDeletions(path string, idsToRemove []string) (*RemoveResult, error) { + result := &RemoveResult{ + RemovedIDs: []string{}, + } + + if len(idsToRemove) == 0 { + return result, nil + } + + loadResult, err := LoadDeletions(path) + if err != nil { + return nil, fmt.Errorf("failed to load deletions: %w", err) + } + + if len(loadResult.Records) == 0 { + return result, nil + } + + // Build a set of IDs to remove for O(1) lookup + removeSet := make(map[string]bool) + for _, id := range idsToRemove { + removeSet[id] = true + } + + // Filter out the IDs to remove + var kept []DeletionRecord + for id, record := range loadResult.Records { + if removeSet[id] { + result.RemovedCount++ + result.RemovedIDs = append(result.RemovedIDs, id) + } else { + kept = append(kept, record) + } + } + + result.KeptCount = len(kept) + + // Only rewrite if we actually removed something + if result.RemovedCount > 0 { + // Sort for deterministic output + sort.Slice(kept, func(i, j int) bool { + return kept[i].ID < kept[j].ID + }) + if err := WriteDeletions(path, kept); err != nil { + return nil, fmt.Errorf("failed to write updated deletions: %w", err) + } + } + + return result, nil +} diff --git a/internal/deletions/deletions_test.go b/internal/deletions/deletions_test.go index 6caf1ba0..7e82999b 100644 --- a/internal/deletions/deletions_test.go +++ b/internal/deletions/deletions_test.go @@ -608,3 +608,222 @@ func TestCount_WithEmptyLines(t *testing.T) { t.Errorf("expected 2 (excluding empty lines), got %d", count) } } + +// Tests for RemoveDeletions (bd-8v5o) + +func TestRemoveDeletions_Empty(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "deletions.jsonl") + + // Remove from non-existent file should succeed + result, err := RemoveDeletions(path, []string{"bd-001"}) + if err != nil { + t.Fatalf("RemoveDeletions should not fail on non-existent file: %v", err) + } + if result.RemovedCount != 0 { + t.Errorf("expected 0 removed, got %d", result.RemovedCount) + } + if result.KeptCount != 0 { + t.Errorf("expected 0 kept, got %d", result.KeptCount) + } +} + +func TestRemoveDeletions_EmptyIDList(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "deletions.jsonl") + + now := time.Now() + records := []DeletionRecord{ + {ID: "bd-001", Timestamp: now, Actor: "user1"}, + {ID: "bd-002", Timestamp: now, Actor: "user2"}, + } + for _, r := range records { + if err := AppendDeletion(path, r); err != nil { + t.Fatalf("AppendDeletion failed: %v", err) + } + } + + // Remove with empty ID list should be a no-op + result, err := RemoveDeletions(path, []string{}) + if err != nil { + t.Fatalf("RemoveDeletions failed: %v", err) + } + if result.RemovedCount != 0 { + t.Errorf("expected 0 removed with empty list, got %d", result.RemovedCount) + } + + // Verify file unchanged + loaded, err := LoadDeletions(path) + if err != nil { + t.Fatalf("LoadDeletions failed: %v", err) + } + if len(loaded.Records) != 2 { + t.Errorf("expected 2 records unchanged, got %d", len(loaded.Records)) + } +} + +func TestRemoveDeletions_SomeMatches(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "deletions.jsonl") + + now := time.Now() + records := []DeletionRecord{ + {ID: "bd-001", Timestamp: now, Actor: "user1"}, + {ID: "bd-002", Timestamp: now, Actor: "user2"}, + {ID: "bd-003", Timestamp: now, Actor: "user3"}, + } + for _, r := range records { + if err := AppendDeletion(path, r); err != nil { + t.Fatalf("AppendDeletion failed: %v", err) + } + } + + // Remove bd-001 and bd-003 + result, err := RemoveDeletions(path, []string{"bd-001", "bd-003"}) + if err != nil { + t.Fatalf("RemoveDeletions failed: %v", err) + } + if result.RemovedCount != 2 { + t.Errorf("expected 2 removed, got %d", result.RemovedCount) + } + if result.KeptCount != 1 { + t.Errorf("expected 1 kept, got %d", result.KeptCount) + } + + // Verify removed IDs + removedMap := make(map[string]bool) + for _, id := range result.RemovedIDs { + removedMap[id] = true + } + if !removedMap["bd-001"] || !removedMap["bd-003"] { + t.Errorf("expected bd-001 and bd-003 to be removed, got %v", result.RemovedIDs) + } + + // Verify file was updated + loaded, err := LoadDeletions(path) + if err != nil { + t.Fatalf("LoadDeletions failed: %v", err) + } + if len(loaded.Records) != 1 { + t.Errorf("expected 1 record after removal, got %d", len(loaded.Records)) + } + if _, ok := loaded.Records["bd-002"]; !ok { + t.Error("expected bd-002 to remain") + } +} + +func TestRemoveDeletions_AllMatches(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "deletions.jsonl") + + now := time.Now() + records := []DeletionRecord{ + {ID: "bd-001", Timestamp: now, Actor: "user1"}, + {ID: "bd-002", Timestamp: now, Actor: "user2"}, + } + for _, r := range records { + if err := AppendDeletion(path, r); err != nil { + t.Fatalf("AppendDeletion failed: %v", err) + } + } + + // Remove all records + result, err := RemoveDeletions(path, []string{"bd-001", "bd-002"}) + if err != nil { + t.Fatalf("RemoveDeletions failed: %v", err) + } + if result.RemovedCount != 2 { + t.Errorf("expected 2 removed, got %d", result.RemovedCount) + } + if result.KeptCount != 0 { + t.Errorf("expected 0 kept, got %d", result.KeptCount) + } + + // Verify file is empty + loaded, err := LoadDeletions(path) + if err != nil { + t.Fatalf("LoadDeletions failed: %v", err) + } + if len(loaded.Records) != 0 { + t.Errorf("expected 0 records after removal, got %d", len(loaded.Records)) + } +} + +func TestRemoveDeletions_NoMatches(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "deletions.jsonl") + + now := time.Now() + records := []DeletionRecord{ + {ID: "bd-001", Timestamp: now, Actor: "user1"}, + {ID: "bd-002", Timestamp: now, Actor: "user2"}, + } + for _, r := range records { + if err := AppendDeletion(path, r); err != nil { + t.Fatalf("AppendDeletion failed: %v", err) + } + } + + // Try to remove IDs that don't exist + result, err := RemoveDeletions(path, []string{"bd-999", "bd-888"}) + if err != nil { + t.Fatalf("RemoveDeletions failed: %v", err) + } + if result.RemovedCount != 0 { + t.Errorf("expected 0 removed (no matches), got %d", result.RemovedCount) + } + if result.KeptCount != 2 { + t.Errorf("expected 2 kept, got %d", result.KeptCount) + } + + // Verify file unchanged + loaded, err := LoadDeletions(path) + if err != nil { + t.Fatalf("LoadDeletions failed: %v", err) + } + if len(loaded.Records) != 2 { + t.Errorf("expected 2 records unchanged, got %d", len(loaded.Records)) + } +} + +func TestRemoveDeletions_MixedExistingAndNonExisting(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "deletions.jsonl") + + now := time.Now() + records := []DeletionRecord{ + {ID: "bd-001", Timestamp: now, Actor: "user1"}, + {ID: "bd-002", Timestamp: now, Actor: "user2"}, + {ID: "bd-003", Timestamp: now, Actor: "user3"}, + } + for _, r := range records { + if err := AppendDeletion(path, r); err != nil { + t.Fatalf("AppendDeletion failed: %v", err) + } + } + + // Try to remove mix of existing and non-existing IDs + result, err := RemoveDeletions(path, []string{"bd-001", "bd-999", "bd-003"}) + if err != nil { + t.Fatalf("RemoveDeletions failed: %v", err) + } + // Only bd-001 and bd-003 should be removed + if result.RemovedCount != 2 { + t.Errorf("expected 2 removed, got %d", result.RemovedCount) + } + if result.KeptCount != 1 { + t.Errorf("expected 1 kept, got %d", result.KeptCount) + } + + // Verify only bd-002 remains + loaded, err := LoadDeletions(path) + if err != nil { + t.Fatalf("LoadDeletions failed: %v", err) + } + if len(loaded.Records) != 1 { + t.Errorf("expected 1 record, got %d", len(loaded.Records)) + } + if _, ok := loaded.Records["bd-002"]; !ok { + t.Error("expected bd-002 to remain") + } +} diff --git a/internal/importer/importer.go b/internal/importer/importer.go index 2b05d87f..954c1c1b 100644 --- a/internal/importer/importer.go +++ b/internal/importer/importer.go @@ -151,13 +151,12 @@ func ImportIssues(ctx context.Context, dbPath string, store storage.Storage, iss continue } - if del, found := loadResult.Records[issue.ID]; found { + if _, found := loadResult.Records[issue.ID]; found { // Non-tombstone issue is in deletions manifest - skip it // (this maintains backward compatibility during transition) + // Note: Individual skip messages removed (bd-wsqt) - caller shows summary result.SkippedDeleted++ result.SkippedDeletedIDs = append(result.SkippedDeletedIDs, issue.ID) - fmt.Fprintf(os.Stderr, "Skipping %s (in deletions manifest: deleted %s by %s)\n", - issue.ID, del.Timestamp.Format("2006-01-02"), del.Actor) } else { filteredIssues = append(filteredIssues, issue) }