fix: resolve P2 sync noise and cleanup issues

- bd-6pni: Auto-filter tombstoned issues with mismatched prefixes during
  import instead of failing. Tombstones from contributor PRs with different
  test prefixes are pollution and safe to ignore.

- bd-ffr9: Stop recreating deletions.jsonl after tombstone migration.
  Added IsTombstoneMigrationComplete() check to all code paths that write
  to the legacy deletions manifest.

- bd-admx: Fix perpetual "JSONL file hash mismatch" warning. Now clears
  both export_hashes AND jsonl_file_hash when mismatch detected, so the
  warning doesn't repeat.

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

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Steve Yegge
2025-12-16 00:55:43 -08:00
parent 88ccce884c
commit 2c86404d65
9 changed files with 341 additions and 30 deletions
+10 -2
View File
@@ -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
}
+14
View File
@@ -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{
+33
View File
@@ -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()
+9
View File
@@ -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")
+25 -7
View File
@@ -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)
}
})
}
+11
View File
@@ -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.
+32
View File
@@ -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")
+67 -21
View File
@@ -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)
+140
View File
@@ -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)
}
}