refactor(deletions): improve API based on code review

- Return LoadResult struct instead of multiple values, with Warnings slice
  for testability (no more hardcoded stderr output)
- Add ID validation in AppendDeletion to prevent invalid records
- Add Sync() call in AppendDeletion for durability
- Document that timestamps may lose sub-second precision
- Document that empty slice in WriteDeletions clears all deletions
- Add test for empty ID validation

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

internal/deletions/deletions.go

@@ -13,29 +13,40 @@ import (
 )
 
 // DeletionRecord represents a single deletion entry in the manifest.
+// Timestamps are serialized as RFC3339 and may lose sub-second precision.
 type DeletionRecord struct {
-	ID        string    `json:"id"`              // Issue ID that was deleted
+	ID        string    `json:"id"`               // Issue ID that was deleted
-	Timestamp time.Time `json:"ts"`              // When the deletion occurred
+	Timestamp time.Time `json:"ts"`               // When the deletion occurred
-	Actor     string    `json:"by"`              // Who performed the deletion
+	Actor     string    `json:"by"`               // Who performed the deletion
 	Reason    string    `json:"reason,omitempty"` // Optional reason for deletion
 }
 
-// LoadDeletions reads the deletions manifest and returns a map for O(1) lookup.
+// LoadResult contains the result of loading deletions, including any warnings.
-// It returns the records, the count of skipped (corrupt) lines, and any error.
+type LoadResult struct {
-// Corrupt JSON lines are skipped with a warning rather than failing the load.
+	Records  map[string]DeletionRecord
-func LoadDeletions(path string) (map[string]DeletionRecord, int, error) {
+	Skipped  int
+	Warnings []string
+}
+
+// LoadDeletions reads the deletions manifest and returns a LoadResult.
+// Corrupt JSON lines are skipped rather than failing the load.
+// Warnings about skipped lines are collected in LoadResult.Warnings.
+func LoadDeletions(path string) (*LoadResult, error) {
+	result := &LoadResult{
+		Records:  make(map[string]DeletionRecord),
+		Warnings: []string{},
+	}
+
 	f, err := os.Open(path) // #nosec G304 - controlled path from caller
 	if err != nil {
 		if os.IsNotExist(err) {
-			// No deletions file yet - return empty map
+			// No deletions file yet - return empty result
-			return make(map[string]DeletionRecord), 0, nil
+			return result, nil
 		}
-		return nil, 0, fmt.Errorf("failed to open deletions file: %w", err)
+		return nil, fmt.Errorf("failed to open deletions file: %w", err)
 	}
 	defer f.Close()
 
-	records := make(map[string]DeletionRecord)
-	skipped := 0
 	lineNo := 0
 
 	scanner := bufio.NewScanner(f)
@@ -51,33 +62,40 @@ func LoadDeletions(path string) (map[string]DeletionRecord, int, error) {
 
 		var record DeletionRecord
 		if err := json.Unmarshal([]byte(line), &record); err != nil {
-			// Skip corrupt line with warning to stderr
+			warning := fmt.Sprintf("skipping corrupt line %d in deletions manifest: %v", lineNo, err)
-			fmt.Fprintf(os.Stderr, "Warning: skipping corrupt line %d in deletions manifest: %v\n", lineNo, err)
+			result.Warnings = append(result.Warnings, warning)
-			skipped++
+			result.Skipped++
 			continue
 		}
 
 		// Validate required fields
 		if record.ID == "" {
-			fmt.Fprintf(os.Stderr, "Warning: skipping line %d in deletions manifest: missing ID\n", lineNo)
+			warning := fmt.Sprintf("skipping line %d in deletions manifest: missing ID", lineNo)
-			skipped++
+			result.Warnings = append(result.Warnings, warning)
+			result.Skipped++
 			continue
 		}
 
 		// Use the most recent record for each ID (last write wins)
-		records[record.ID] = record
+		result.Records[record.ID] = record
 	}
 
 	if err := scanner.Err(); err != nil {
-		return nil, skipped, fmt.Errorf("error reading deletions file: %w", err)
+		return nil, fmt.Errorf("error reading deletions file: %w", err)
 	}
 
-	return records, skipped, nil
+	return result, nil
 }
 
 // AppendDeletion appends a single deletion record to the manifest.
 // Creates the file if it doesn't exist.
+// Returns an error if the record has an empty ID.
 func AppendDeletion(path string, record DeletionRecord) error {
+	// Validate required fields
+	if record.ID == "" {
+		return fmt.Errorf("cannot append deletion record: ID is required")
+	}
+
 	// Ensure directory exists
 	dir := filepath.Dir(path)
 	if err := os.MkdirAll(dir, 0755); err != nil {
@@ -102,11 +120,17 @@ func AppendDeletion(path string, record DeletionRecord) error {
 		return fmt.Errorf("failed to write deletion record: %w", err)
 	}
 
+	// Sync to ensure durability for append-only log
+	if err := f.Sync(); err != nil {
+		return fmt.Errorf("failed to sync deletions file: %w", err)
+	}
+
 	return nil
 }
 
 // WriteDeletions atomically writes the entire deletions manifest.
 // Used for compaction to deduplicate and prune old entries.
+// An empty slice will create an empty file (clearing all deletions).
 func WriteDeletions(path string, records []DeletionRecord) error {
 	// Ensure directory exists
 	dir := filepath.Dir(path)

internal/deletions/deletions_test.go

@@ -8,16 +8,19 @@ import (
 )
 
 func TestLoadDeletions_Empty(t *testing.T) {
-	// Non-existent file should return empty map
+	// Non-existent file should return empty result
-	records, skipped, err := LoadDeletions("/nonexistent/path/deletions.jsonl")
+	result, err := LoadDeletions("/nonexistent/path/deletions.jsonl")
 	if err != nil {
 		t.Fatalf("expected no error for non-existent file, got: %v", err)
 	}
-	if skipped != 0 {
+	if result.Skipped != 0 {
-		t.Errorf("expected 0 skipped, got %d", skipped)
+		t.Errorf("expected 0 skipped, got %d", result.Skipped)
 	}
-	if len(records) != 0 {
+	if len(result.Records) != 0 {
-		t.Errorf("expected empty map, got %d records", len(records))
+		t.Errorf("expected empty map, got %d records", len(result.Records))
+	}
+	if len(result.Warnings) != 0 {
+		t.Errorf("expected no warnings, got %d", len(result.Warnings))
 	}
 }
 
@@ -48,19 +51,19 @@ func TestRoundTrip(t *testing.T) {
 	}
 
 	// Load and verify
-	records, skipped, err := LoadDeletions(path)
+	result, err := LoadDeletions(path)
 	if err != nil {
 		t.Fatalf("LoadDeletions failed: %v", err)
 	}
-	if skipped != 0 {
+	if result.Skipped != 0 {
-		t.Errorf("expected 0 skipped, got %d", skipped)
+		t.Errorf("expected 0 skipped, got %d", result.Skipped)
 	}
-	if len(records) != 2 {
+	if len(result.Records) != 2 {
-		t.Fatalf("expected 2 records, got %d", len(records))
+		t.Fatalf("expected 2 records, got %d", len(result.Records))
 	}
 
 	// Verify record1
-	r1, ok := records["bd-123"]
+	r1, ok := result.Records["bd-123"]
 	if !ok {
 		t.Fatal("record bd-123 not found")
 	}
@@ -72,7 +75,7 @@ func TestRoundTrip(t *testing.T) {
 	}
 
 	// Verify record2
-	r2, ok := records["bd-456"]
+	r2, ok := result.Records["bd-456"]
 	if !ok {
 		t.Fatal("record bd-456 not found")
 	}
@@ -96,20 +99,23 @@ this is not valid json
 		t.Fatalf("failed to write test file: %v", err)
 	}
 
-	records, skipped, err := LoadDeletions(path)
+	result, err := LoadDeletions(path)
 	if err != nil {
 		t.Fatalf("LoadDeletions should not fail on corrupt lines: %v", err)
 	}
-	if skipped != 2 {
+	if result.Skipped != 2 {
-		t.Errorf("expected 2 skipped lines, got %d", skipped)
+		t.Errorf("expected 2 skipped lines, got %d", result.Skipped)
 	}
-	if len(records) != 3 {
+	if len(result.Records) != 3 {
-		t.Errorf("expected 3 valid records, got %d", len(records))
+		t.Errorf("expected 3 valid records, got %d", len(result.Records))
+	}
+	if len(result.Warnings) != 2 {
+		t.Errorf("expected 2 warnings, got %d", len(result.Warnings))
 	}
 
 	// Verify valid records were loaded
 	for _, id := range []string{"bd-001", "bd-002", "bd-003"} {
-		if _, ok := records[id]; !ok {
+		if _, ok := result.Records[id]; !ok {
 			t.Errorf("expected record %s to be loaded", id)
 		}
 	}
@@ -128,16 +134,16 @@ func TestLoadDeletions_MissingID(t *testing.T) {
 		t.Fatalf("failed to write test file: %v", err)
 	}
 
-	records, skipped, err := LoadDeletions(path)
+	result, err := LoadDeletions(path)
 	if err != nil {
 		t.Fatalf("LoadDeletions failed: %v", err)
 	}
 	// Two lines should be skipped: one missing "id" field, one with empty "id"
-	if skipped != 2 {
+	if result.Skipped != 2 {
-		t.Errorf("expected 2 skipped lines (missing/empty ID), got %d", skipped)
+		t.Errorf("expected 2 skipped lines (missing/empty ID), got %d", result.Skipped)
 	}
-	if len(records) != 1 {
+	if len(result.Records) != 1 {
-		t.Errorf("expected 1 valid record, got %d", len(records))
+		t.Errorf("expected 1 valid record, got %d", len(result.Records))
 	}
 }
 
@@ -153,18 +159,18 @@ func TestLoadDeletions_LastWriteWins(t *testing.T) {
 		t.Fatalf("failed to write test file: %v", err)
 	}
 
-	records, skipped, err := LoadDeletions(path)
+	result, err := LoadDeletions(path)
 	if err != nil {
 		t.Fatalf("LoadDeletions failed: %v", err)
 	}
-	if skipped != 0 {
+	if result.Skipped != 0 {
-		t.Errorf("expected 0 skipped, got %d", skipped)
+		t.Errorf("expected 0 skipped, got %d", result.Skipped)
 	}
-	if len(records) != 1 {
+	if len(result.Records) != 1 {
-		t.Errorf("expected 1 record (deduplicated), got %d", len(records))
+		t.Errorf("expected 1 record (deduplicated), got %d", len(result.Records))
 	}
 
-	r := records["bd-001"]
+	r := result.Records["bd-001"]
 	if r.Actor != "user2" {
 		t.Errorf("expected last write to win (user2), got '%s'", r.Actor)
 	}
@@ -188,15 +194,15 @@ func TestWriteDeletions_Atomic(t *testing.T) {
 	}
 
 	// Verify by loading
-	loaded, skipped, err := LoadDeletions(path)
+	result, err := LoadDeletions(path)
 	if err != nil {
 		t.Fatalf("LoadDeletions failed: %v", err)
 	}
-	if skipped != 0 {
+	if result.Skipped != 0 {
-		t.Errorf("expected 0 skipped, got %d", skipped)
+		t.Errorf("expected 0 skipped, got %d", result.Skipped)
 	}
-	if len(loaded) != 2 {
+	if len(result.Records) != 2 {
-		t.Errorf("expected 2 records, got %d", len(loaded))
+		t.Errorf("expected 2 records, got %d", len(result.Records))
 	}
 }
 
@@ -225,14 +231,14 @@ func TestWriteDeletions_Overwrite(t *testing.T) {
 	}
 
 	// Verify only compacted records remain
-	loaded, _, err := LoadDeletions(path)
+	result, err := LoadDeletions(path)
 	if err != nil {
 		t.Fatalf("LoadDeletions failed: %v", err)
 	}
-	if len(loaded) != 1 {
+	if len(result.Records) != 1 {
-		t.Errorf("expected 1 record after compaction, got %d", len(loaded))
+		t.Errorf("expected 1 record after compaction, got %d", len(result.Records))
 	}
-	if _, ok := loaded["bd-002"]; !ok {
+	if _, ok := result.Records["bd-002"]; !ok {
 		t.Error("expected bd-002 to remain after compaction")
 	}
 }
@@ -297,14 +303,33 @@ func TestLoadDeletions_EmptyLines(t *testing.T) {
 		t.Fatalf("failed to write test file: %v", err)
 	}
 
-	records, skipped, err := LoadDeletions(path)
+	result, err := LoadDeletions(path)
 	if err != nil {
 		t.Fatalf("LoadDeletions failed: %v", err)
 	}
-	if skipped != 0 {
+	if result.Skipped != 0 {
-		t.Errorf("empty lines should not count as skipped, got %d", skipped)
+		t.Errorf("empty lines should not count as skipped, got %d", result.Skipped)
 	}
-	if len(records) != 2 {
+	if len(result.Records) != 2 {
-		t.Errorf("expected 2 records, got %d", len(records))
+		t.Errorf("expected 2 records, got %d", len(result.Records))
+	}
+}
+
+func TestAppendDeletion_EmptyID(t *testing.T) {
+	tmpDir := t.TempDir()
+	path := filepath.Join(tmpDir, "deletions.jsonl")
+
+	record := DeletionRecord{
+		ID:        "",
+		Timestamp: time.Now(),
+		Actor:     "testuser",
+	}
+
+	err := AppendDeletion(path, record)
+	if err == nil {
+		t.Fatal("AppendDeletion should fail with empty ID")
+	}
+	if err.Error() != "cannot append deletion record: ID is required" {
+		t.Errorf("unexpected error message: %v", err)
 	}
 }