diff --git a/cmd/bd/autoflush.go b/cmd/bd/autoflush.go index 04261ac7..2b1f93b9 100644 --- a/cmd/bd/autoflush.go +++ b/cmd/bd/autoflush.go @@ -401,11 +401,15 @@ func validateJSONLIntegrity(ctx context.Context, jsonlPath string) (bool, error) jsonlData, err := os.ReadFile(jsonlPath) if err != nil { if os.IsNotExist(err) { - // JSONL doesn't exist but we have a stored hash - clear export_hashes + // JSONL doesn't exist but we have a stored hash - clear export_hashes and jsonl_file_hash fmt.Fprintf(os.Stderr, "⚠️ WARNING: JSONL file missing but export_hashes exist. Clearing export_hashes.\n") if err := store.ClearAllExportHashes(ctx); err != nil { return false, fmt.Errorf("failed to clear export_hashes: %w", err) } + // Also clear jsonl_file_hash to prevent perpetual mismatch warnings (bd-admx) + if err := store.SetJSONLFileHash(ctx, ""); err != nil { + return false, fmt.Errorf("failed to clear jsonl_file_hash: %w", err) + } return true, nil // Signal full export needed } return false, fmt.Errorf("failed to read JSONL file: %w", err) @@ -421,11 +425,15 @@ func validateJSONLIntegrity(ctx context.Context, jsonlPath string) (bool, error) fmt.Fprintf(os.Stderr, "⚠️ WARNING: JSONL file hash mismatch detected (bd-160)\n") fmt.Fprintf(os.Stderr, " This indicates JSONL and export_hashes are out of sync.\n") fmt.Fprintf(os.Stderr, " Clearing export_hashes to force full re-export.\n") - + // Clear export_hashes to force full re-export if err := store.ClearAllExportHashes(ctx); err != nil { return false, fmt.Errorf("failed to clear export_hashes: %w", err) } + // Also clear jsonl_file_hash to prevent perpetual mismatch warnings (bd-admx) + if err := store.SetJSONLFileHash(ctx, ""); err != nil { + return false, fmt.Errorf("failed to clear jsonl_file_hash: %w", err) + } return true, nil // Signal full export needed } diff --git a/cmd/bd/delete.go b/cmd/bd/delete.go index fa859c99..ed83de76 100644 --- a/cmd/bd/delete.go +++ b/cmd/bd/delete.go @@ -685,7 +685,14 @@ func getDeletionsPath() string { // recordDeletion appends a deletion record to the deletions manifest. // This MUST be called BEFORE deleting from the database to ensure // deletion records are never lost. +// After tombstone migration (bd-ffr9), this is a no-op since inline tombstones +// are used instead of deletions.jsonl. func recordDeletion(id, deleteActor, reason string) error { + // bd-ffr9: Skip writing to deletions.jsonl if tombstone migration is complete + beadsDir := filepath.Dir(dbPath) + if deletions.IsTombstoneMigrationComplete(beadsDir) { + return nil + } record := deletions.DeletionRecord{ ID: id, Timestamp: time.Now().UTC(), @@ -698,7 +705,14 @@ func recordDeletion(id, deleteActor, reason string) error { // recordDeletions appends multiple deletion records to the deletions manifest. // This MUST be called BEFORE deleting from the database to ensure // deletion records are never lost. +// After tombstone migration (bd-ffr9), this is a no-op since inline tombstones +// are used instead of deletions.jsonl. func recordDeletions(ids []string, deleteActor, reason string) error { + // bd-ffr9: Skip writing to deletions.jsonl if tombstone migration is complete + beadsDir := filepath.Dir(dbPath) + if deletions.IsTombstoneMigrationComplete(beadsDir) { + return nil + } path := getDeletionsPath() for _, id := range ids { record := deletions.DeletionRecord{ diff --git a/cmd/bd/delete_recording_test.go b/cmd/bd/delete_recording_test.go index af85bc2f..de2b1c5c 100644 --- a/cmd/bd/delete_recording_test.go +++ b/cmd/bd/delete_recording_test.go @@ -62,6 +62,39 @@ func TestRecordDeletion(t *testing.T) { } } +// TestRecordDeletion_SkipsAfterMigration tests that recordDeletion is a no-op after tombstone migration (bd-ffr9) +func TestRecordDeletion_SkipsAfterMigration(t *testing.T) { + tmpDir := t.TempDir() + + // Set up dbPath so getDeletionsPath() works + oldDbPath := dbPath + dbPath = filepath.Join(tmpDir, "beads.db") + defer func() { dbPath = oldDbPath }() + + // Create the .beads directory + if err := os.MkdirAll(tmpDir, 0750); err != nil { + t.Fatalf("failed to create directory: %v", err) + } + + // Create the .migrated marker file to indicate tombstone migration is complete + migratedPath := filepath.Join(tmpDir, "deletions.jsonl.migrated") + if err := os.WriteFile(migratedPath, []byte("{}"), 0644); err != nil { + t.Fatalf("failed to create migrated marker: %v", err) + } + + // Test recordDeletion - should be a no-op + err := recordDeletion("test-abc", "test-user", "test reason") + if err != nil { + t.Fatalf("recordDeletion failed: %v", err) + } + + // Verify deletions.jsonl was NOT created + deletionsPath := getDeletionsPath() + if _, err := os.Stat(deletionsPath); !os.IsNotExist(err) { + t.Error("deletions.jsonl should not be created after tombstone migration") + } +} + // TestRecordDeletions tests that recordDeletions creates multiple deletion manifest entries func TestRecordDeletions(t *testing.T) { tmpDir := t.TempDir() diff --git a/cmd/bd/doctor/fix/deletions.go b/cmd/bd/doctor/fix/deletions.go index c0831a25..cf8b7766 100644 --- a/cmd/bd/doctor/fix/deletions.go +++ b/cmd/bd/doctor/fix/deletions.go @@ -16,12 +16,21 @@ import ( // HydrateDeletionsManifest populates deletions.jsonl from git history. // It finds all issue IDs that were ever in the JSONL but are no longer present, // and adds them to the deletions manifest. +// Note (bd-ffr9): After tombstone migration, this is a no-op since inline tombstones +// are used instead of deletions.jsonl. func HydrateDeletionsManifest(path string) error { if err := validateBeadsWorkspace(path); err != nil { return err } beadsDir := filepath.Join(path, ".beads") + + // bd-ffr9: Skip hydrating deletions.jsonl if tombstone migration is complete + if deletions.IsTombstoneMigrationComplete(beadsDir) { + fmt.Println(" Tombstone migration complete - skipping deletions.jsonl hydration") + return nil + } + // bd-6xd: issues.jsonl is the canonical filename jsonlPath := filepath.Join(beadsDir, "issues.jsonl") diff --git a/cmd/bd/jsonl_integrity_test.go b/cmd/bd/jsonl_integrity_test.go index 1c7b9695..3e8ae92d 100644 --- a/cmd/bd/jsonl_integrity_test.go +++ b/cmd/bd/jsonl_integrity_test.go @@ -100,12 +100,12 @@ func TestJSONLIntegrityValidation(t *testing.T) { if err := os.WriteFile(jsonlPath, []byte(`{"id":"bd-1","title":"Modified"}`+"\n"), 0644); err != nil { t.Fatalf("failed to modify JSONL: %v", err) } - + // Add an export hash to verify it gets cleared if err := testStore.SetExportHash(ctx, "bd-1", "dummy-hash"); err != nil { t.Fatalf("failed to set export hash: %v", err) } - + // Validate should detect mismatch and clear export_hashes needsFullExport, err := validateJSONLIntegrity(ctx, jsonlPath) if err != nil { @@ -114,7 +114,7 @@ func TestJSONLIntegrityValidation(t *testing.T) { if !needsFullExport { t.Fatalf("expected needsFullExport=true after clearing export_hashes") } - + // Verify export_hashes were cleared hash, err := testStore.GetExportHash(ctx, "bd-1") if err != nil { @@ -123,6 +123,15 @@ func TestJSONLIntegrityValidation(t *testing.T) { if hash != "" { t.Fatalf("expected export hash to be cleared, got %q", hash) } + + // Verify jsonl_file_hash was also cleared (bd-admx fix) + fileHash, err := testStore.GetJSONLFileHash(ctx) + if err != nil { + t.Fatalf("failed to get JSONL file hash: %v", err) + } + if fileHash != "" { + t.Fatalf("expected jsonl_file_hash to be cleared to prevent perpetual warnings, got %q", fileHash) + } }) // Test 3: Missing JSONL file @@ -131,17 +140,17 @@ func TestJSONLIntegrityValidation(t *testing.T) { if err := testStore.SetJSONLFileHash(ctx, "some-hash"); err != nil { t.Fatalf("failed to set JSONL file hash: %v", err) } - + // Add an export hash if err := testStore.SetExportHash(ctx, "bd-1", "dummy-hash"); err != nil { t.Fatalf("failed to set export hash: %v", err) } - + // Remove JSONL file if err := os.Remove(jsonlPath); err != nil { t.Fatalf("failed to remove JSONL: %v", err) } - + // Validate should detect missing file and clear export_hashes needsFullExport, err := validateJSONLIntegrity(ctx, jsonlPath) if err != nil { @@ -150,7 +159,7 @@ func TestJSONLIntegrityValidation(t *testing.T) { if !needsFullExport { t.Fatalf("expected needsFullExport=true after clearing export_hashes") } - + // Verify export_hashes were cleared hash, err := testStore.GetExportHash(ctx, "bd-1") if err != nil { @@ -159,6 +168,15 @@ func TestJSONLIntegrityValidation(t *testing.T) { if hash != "" { t.Fatalf("expected export hash to be cleared, got %q", hash) } + + // Verify jsonl_file_hash was also cleared (bd-admx fix) + fileHash, err := testStore.GetJSONLFileHash(ctx) + if err != nil { + t.Fatalf("failed to get JSONL file hash: %v", err) + } + if fileHash != "" { + t.Fatalf("expected jsonl_file_hash to be cleared to prevent perpetual warnings, got %q", fileHash) + } }) } diff --git a/internal/deletions/deletions.go b/internal/deletions/deletions.go index 0590a310..e8cb2937 100644 --- a/internal/deletions/deletions.go +++ b/internal/deletions/deletions.go @@ -181,6 +181,17 @@ func DefaultPath(beadsDir string) string { return filepath.Join(beadsDir, "deletions.jsonl") } +// IsTombstoneMigrationComplete checks if the tombstone migration has been completed. +// After running `bd migrate-tombstones`, the deletions.jsonl file is archived to +// deletions.jsonl.migrated. This function checks for that marker file. +// When migration is complete, new deletion records should NOT be written to +// deletions.jsonl (bd-ffr9). +func IsTombstoneMigrationComplete(beadsDir string) bool { + migratedPath := filepath.Join(beadsDir, "deletions.jsonl.migrated") + _, err := os.Stat(migratedPath) + return err == nil +} + // Count returns the number of lines in the deletions manifest. // This is a fast operation that doesn't parse JSON, just counts lines. // Returns 0 if the file doesn't exist or is empty. diff --git a/internal/deletions/deletions_test.go b/internal/deletions/deletions_test.go index 7e82999b..7faa524c 100644 --- a/internal/deletions/deletions_test.go +++ b/internal/deletions/deletions_test.go @@ -290,6 +290,38 @@ func TestDefaultPath(t *testing.T) { } } +func TestIsTombstoneMigrationComplete(t *testing.T) { + t.Run("no migrated file", func(t *testing.T) { + tmpDir := t.TempDir() + if IsTombstoneMigrationComplete(tmpDir) { + t.Error("expected false when no .migrated file exists") + } + }) + + t.Run("migrated file exists", func(t *testing.T) { + tmpDir := t.TempDir() + migratedPath := filepath.Join(tmpDir, "deletions.jsonl.migrated") + if err := os.WriteFile(migratedPath, []byte("{}"), 0644); err != nil { + t.Fatalf("failed to create migrated file: %v", err) + } + if !IsTombstoneMigrationComplete(tmpDir) { + t.Error("expected true when .migrated file exists") + } + }) + + t.Run("deletions.jsonl exists without migrated", func(t *testing.T) { + tmpDir := t.TempDir() + deletionsPath := filepath.Join(tmpDir, "deletions.jsonl") + if err := os.WriteFile(deletionsPath, []byte("{}"), 0644); err != nil { + t.Fatalf("failed to create deletions file: %v", err) + } + // Should return false because the .migrated marker doesn't exist + if IsTombstoneMigrationComplete(tmpDir) { + t.Error("expected false when only deletions.jsonl exists (not migrated)") + } + }) +} + func TestLoadDeletions_EmptyLines(t *testing.T) { tmpDir := t.TempDir() path := filepath.Join(tmpDir, "deletions.jsonl") diff --git a/internal/importer/importer.go b/internal/importer/importer.go index 954c1c1b..72e02728 100644 --- a/internal/importer/importer.go +++ b/internal/importer/importer.go @@ -180,7 +180,8 @@ func ImportIssues(ctx context.Context, dbPath string, store storage.Storage, iss } // Check and handle prefix mismatches - if err := handlePrefixMismatch(ctx, sqliteStore, issues, opts, result); err != nil { + issues, err = handlePrefixMismatch(ctx, sqliteStore, issues, opts, result) + if err != nil { return result, err } @@ -258,48 +259,90 @@ func getOrCreateStore(ctx context.Context, dbPath string, store storage.Storage) return sqliteStore, true, nil } -// handlePrefixMismatch checks and handles prefix mismatches -func handlePrefixMismatch(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, issues []*types.Issue, opts Options, result *Result) error { +// handlePrefixMismatch checks and handles prefix mismatches. +// Returns a filtered issues slice with tombstoned issues having wrong prefixes removed (bd-6pni). +func handlePrefixMismatch(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, issues []*types.Issue, opts Options, result *Result) ([]*types.Issue, error) { configuredPrefix, err := sqliteStore.GetConfig(ctx, "issue_prefix") if err != nil { - return fmt.Errorf("failed to get configured prefix: %w", err) + return nil, fmt.Errorf("failed to get configured prefix: %w", err) } // Only validate prefixes if a prefix is configured if strings.TrimSpace(configuredPrefix) == "" { if opts.RenameOnImport { - return fmt.Errorf("cannot rename: issue_prefix not configured in database") + return nil, fmt.Errorf("cannot rename: issue_prefix not configured in database") } - return nil + return issues, nil } result.ExpectedPrefix = configuredPrefix // Analyze prefixes in imported issues + // Track tombstones separately - they don't count as "real" mismatches (bd-6pni) + tombstoneMismatchPrefixes := make(map[string]int) + nonTombstoneMismatchCount := 0 + + // Also track which tombstones have wrong prefixes for filtering + var filteredIssues []*types.Issue + var tombstonesToRemove []string + for _, issue := range issues { prefix := utils.ExtractIssuePrefix(issue.ID) if prefix != configuredPrefix { - result.PrefixMismatch = true - result.MismatchPrefixes[prefix]++ + if issue.IsTombstone() { + tombstoneMismatchPrefixes[prefix]++ + tombstonesToRemove = append(tombstonesToRemove, issue.ID) + // Don't add to filtered list - we'll remove these + } else { + result.PrefixMismatch = true + result.MismatchPrefixes[prefix]++ + nonTombstoneMismatchCount++ + filteredIssues = append(filteredIssues, issue) + } + } else { + // Correct prefix - keep the issue + filteredIssues = append(filteredIssues, issue) } } - // If prefix mismatch detected and not handling it, return error or warning - if result.PrefixMismatch && !opts.RenameOnImport && !opts.DryRun && !opts.SkipPrefixValidation { - return fmt.Errorf("prefix mismatch detected: database uses '%s-' but found issues with prefixes: %v (use --rename-on-import to automatically fix)", configuredPrefix, GetPrefixList(result.MismatchPrefixes)) + // bd-6pni: If ALL mismatched prefix issues are tombstones, they're just pollution + // from contributor PRs that used different test prefixes. These are safe to remove. + if nonTombstoneMismatchCount == 0 && len(tombstoneMismatchPrefixes) > 0 { + // Log that we're ignoring tombstoned mismatches + var tombstonePrefixList []string + for prefix, count := range tombstoneMismatchPrefixes { + tombstonePrefixList = append(tombstonePrefixList, fmt.Sprintf("%s- (%d tombstones)", prefix, count)) + } + fmt.Fprintf(os.Stderr, "Ignoring prefix mismatches (all are tombstones): %v\n", tombstonePrefixList) + // Clear mismatch flags - no real issues to worry about + result.PrefixMismatch = false + result.MismatchPrefixes = make(map[string]int) + // Return filtered list without the tombstones + return filteredIssues, nil + } + + // If there are non-tombstone mismatches, we need to include all issues (tombstones too) + // but still report the error for non-tombstones + if result.PrefixMismatch { + // If not handling the mismatch, return error + if !opts.RenameOnImport && !opts.DryRun && !opts.SkipPrefixValidation { + return nil, fmt.Errorf("prefix mismatch detected: database uses '%s-' but found issues with prefixes: %v (use --rename-on-import to automatically fix)", configuredPrefix, GetPrefixList(result.MismatchPrefixes)) + } } // Handle rename-on-import if requested if result.PrefixMismatch && opts.RenameOnImport && !opts.DryRun { if err := RenameImportedIssuePrefixes(issues, configuredPrefix); err != nil { - return fmt.Errorf("failed to rename prefixes: %w", err) + return nil, fmt.Errorf("failed to rename prefixes: %w", err) } // After renaming, clear the mismatch flags since we fixed them result.PrefixMismatch = false result.MismatchPrefixes = make(map[string]int) + return issues, nil } - return nil + // Return original issues if no filtering needed + return issues, nil } // detectUpdates detects same-ID scenarios (which are updates with hash IDs, not collisions) @@ -1034,14 +1077,17 @@ func purgeDeletedIssues(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, } // Backfill the deletions manifest (self-healing) - backfillRecord := deletions.DeletionRecord{ - ID: id, - Timestamp: time.Now().UTC(), - Actor: "git-history-backfill", - Reason: "recovered from git history (pruned from manifest)", - } - if err := deletions.AppendDeletion(deletionsPath, backfillRecord); err != nil { - fmt.Fprintf(os.Stderr, "Warning: failed to backfill deletion record for %s: %v\n", id, err) + // bd-ffr9: Skip writing to deletions.jsonl if tombstone migration is complete + if !deletions.IsTombstoneMigrationComplete(beadsDir) { + backfillRecord := deletions.DeletionRecord{ + ID: id, + Timestamp: time.Now().UTC(), + Actor: "git-history-backfill", + Reason: "recovered from git history (pruned from manifest)", + } + if err := deletions.AppendDeletion(deletionsPath, backfillRecord); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to backfill deletion record for %s: %v\n", id, err) + } } // Convert to tombstone (bd-dve) diff --git a/internal/importer/importer_test.go b/internal/importer/importer_test.go index ac6f6163..9b240f4f 100644 --- a/internal/importer/importer_test.go +++ b/internal/importer/importer_test.go @@ -1596,3 +1596,143 @@ func TestImportCrossPrefixContentMatch(t *testing.T) { t.Error("Cross-prefix issue beta-xyz789 should NOT be created in the database") } } + +// TestImportTombstonePrefixMismatch tests that tombstoned issues with different prefixes +// don't block import (bd-6pni). This handles pollution from contributor PRs that used +// different test prefixes - these tombstones are safe to ignore. +func TestImportTombstonePrefixMismatch(t *testing.T) { + ctx := context.Background() + + tmpDB := t.TempDir() + "/test.db" + store, err := sqlite.New(context.Background(), tmpDB) + if err != nil { + t.Fatalf("Failed to create store: %v", err) + } + defer store.Close() + + // Configure database with "bd" prefix + if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { + t.Fatalf("Failed to set prefix: %v", err) + } + + // Create tombstoned issues with WRONG prefixes (simulating pollution) + deletedAt := time.Now().Add(-time.Hour) + issues := []*types.Issue{ + // Normal issue with correct prefix + { + ID: "bd-good1", + Title: "Good issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + }, + // Tombstone with wrong prefix "beads" + { + ID: "beads-old1", + Title: "(deleted)", + Status: types.StatusTombstone, + Priority: 2, + IssueType: types.TypeTask, + DeletedAt: &deletedAt, + DeletedBy: "cleanup", + DeleteReason: "test cleanup", + }, + // Tombstone with wrong prefix "test" + { + ID: "test-old2", + Title: "(deleted)", + Status: types.StatusTombstone, + Priority: 2, + IssueType: types.TypeTask, + DeletedAt: &deletedAt, + DeletedBy: "cleanup", + DeleteReason: "test cleanup", + }, + } + + // Import should succeed - tombstones with wrong prefixes should be ignored + result, err := ImportIssues(ctx, tmpDB, store, issues, Options{}) + if err != nil { + t.Fatalf("Import should succeed when all mismatched prefixes are tombstones: %v", err) + } + + // Should have created the good issue + // Tombstones with wrong prefixes are skipped (cross-prefix content match logic) + if result.Created < 1 { + t.Errorf("Expected at least 1 created issue, got %d", result.Created) + } + + // PrefixMismatch should be false because all mismatches were tombstones + if result.PrefixMismatch { + t.Error("PrefixMismatch should be false when all mismatched prefixes are tombstones") + } + + // Verify the good issue was imported + goodIssue, err := store.GetIssue(ctx, "bd-good1") + if err != nil { + t.Fatalf("Failed to get good issue: %v", err) + } + if goodIssue.Title != "Good issue" { + t.Errorf("Expected title 'Good issue', got %q", goodIssue.Title) + } +} + +// TestImportMixedPrefixMismatch tests that import fails when there are non-tombstone +// issues with wrong prefixes, even if some tombstones also have wrong prefixes. +func TestImportMixedPrefixMismatch(t *testing.T) { + ctx := context.Background() + + tmpDB := t.TempDir() + "/test.db" + store, err := sqlite.New(context.Background(), tmpDB) + if err != nil { + t.Fatalf("Failed to create store: %v", err) + } + defer store.Close() + + // Configure database with "bd" prefix + if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { + t.Fatalf("Failed to set prefix: %v", err) + } + + deletedAt := time.Now().Add(-time.Hour) + issues := []*types.Issue{ + // Normal issue with correct prefix + { + ID: "bd-good1", + Title: "Good issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + }, + // Tombstone with wrong prefix (should be ignored) + { + ID: "beads-old1", + Title: "(deleted)", + Status: types.StatusTombstone, + Priority: 2, + IssueType: types.TypeTask, + DeletedAt: &deletedAt, + DeletedBy: "cleanup", + DeleteReason: "test cleanup", + }, + // NON-tombstone with wrong prefix (should cause error) + { + ID: "other-bad1", + Title: "Bad issue with wrong prefix", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + }, + } + + // Import should fail due to the non-tombstone with wrong prefix + _, err = ImportIssues(ctx, tmpDB, store, issues, Options{}) + if err == nil { + t.Fatal("Import should fail when there are non-tombstone issues with wrong prefixes") + } + + // Error message should mention prefix mismatch + if !strings.Contains(err.Error(), "prefix mismatch") { + t.Errorf("Error should mention prefix mismatch, got: %v", err) + } +}