fix(doctor,sync): clean up deletions manifest and reduce sync noise

- 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 <noreply@anthropic.com>
2025-12-16 00:42:04 -08:00
parent 171f7de250
commit 8c45069228
5 changed files with 420 additions and 3 deletions
--- 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
+}
--- 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")
+	}
+}
--- 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)
 				}