From 62e4eaf7c181ee56ac87c7824dd120d90d27a0f5 Mon Sep 17 00:00:00 2001 From: emma Date: Sat, 3 Jan 2026 13:27:19 -0800 Subject: [PATCH] fix(sync): make snapshot protection timestamp-aware (GH#865) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The --protect-left-snapshot mechanism was protecting ALL local issues by ID alone, ignoring timestamps. This caused newer remote changes to be incorrectly skipped during cross-worktree sync. Changes: - Add BuildIDToTimestampMap() to SnapshotManager for timestamp-aware snapshot reading - Change ProtectLocalExportIDs from map[string]bool to map[string]time.Time - Add shouldProtectFromUpdate() helper that compares timestamps - Only protect if local snapshot is newer than incoming; allow update if incoming is newer This fixes data loss scenarios where: 1. Main worktree closes issue at 11:31 2. Test worktree syncs and incorrectly skips the update 3. Test worktree then pushes stale open state, overwriting mains changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- cmd/bd/import.go | 12 +- cmd/bd/import_shared.go | 3 +- cmd/bd/snapshot_manager.go | 42 +++++ internal/importer/importer.go | 31 +++- internal/importer/timestamp_test.go | 268 ++++++++++++++++++++++++++++ 5 files changed, 348 insertions(+), 8 deletions(-) diff --git a/cmd/bd/import.go b/cmd/bd/import.go index 6aeecd92..f13e0e04 100644 --- a/cmd/bd/import.go +++ b/cmd/bd/import.go @@ -262,19 +262,19 @@ NOTE: Import requires direct database access and does not work with daemon mode. OrphanHandling: orphanHandling, } - // If --protect-left-snapshot is set, read the left snapshot and build ID set - // This protects locally exported issues from git-history-backfill + // If --protect-left-snapshot is set, read the left snapshot and build timestamp map + // GH#865: Use timestamp-aware protection - only protect if local is newer than incoming if protectLeftSnapshot && input != "" { beadsDir := filepath.Dir(input) leftSnapshotPath := filepath.Join(beadsDir, "beads.left.jsonl") if _, err := os.Stat(leftSnapshotPath); err == nil { sm := NewSnapshotManager(input) - leftIDs, err := sm.BuildIDSet(leftSnapshotPath) + leftTimestamps, err := sm.BuildIDToTimestampMap(leftSnapshotPath) if err != nil { fmt.Fprintf(os.Stderr, "Warning: failed to read left snapshot: %v\n", err) - } else if len(leftIDs) > 0 { - opts.ProtectLocalExportIDs = leftIDs - fmt.Fprintf(os.Stderr, "Protecting %d issue(s) from left snapshot\n", len(leftIDs)) + } else if len(leftTimestamps) > 0 { + opts.ProtectLocalExportIDs = leftTimestamps + fmt.Fprintf(os.Stderr, "Protecting %d issue(s) from left snapshot (timestamp-aware)\n", len(leftTimestamps)) } } } diff --git a/cmd/bd/import_shared.go b/cmd/bd/import_shared.go index d8a90ca0..0c30d848 100644 --- a/cmd/bd/import_shared.go +++ b/cmd/bd/import_shared.go @@ -2,6 +2,7 @@ package main import ( "context" + "time" "github.com/steveyegge/beads/internal/importer" "github.com/steveyegge/beads/internal/storage" @@ -165,7 +166,7 @@ type ImportOptions struct { SkipPrefixValidation bool // Skip prefix validation (for auto-import) ClearDuplicateExternalRefs bool // Clear duplicate external_ref values instead of erroring OrphanHandling string // Orphan handling mode: strict/resurrect/skip/allow (empty = use config) - ProtectLocalExportIDs map[string]bool // IDs from left snapshot to protect from git-history-backfill (bd-sync-deletion fix) + ProtectLocalExportIDs map[string]time.Time // IDs from left snapshot with timestamps for timestamp-aware protection (GH#865) } // ImportResult contains statistics about the import operation diff --git a/cmd/bd/snapshot_manager.go b/cmd/bd/snapshot_manager.go index f7de8199..a06a6960 100644 --- a/cmd/bd/snapshot_manager.go +++ b/cmd/bd/snapshot_manager.go @@ -289,6 +289,48 @@ func (sm *SnapshotManager) BuildIDSet(path string) (map[string]bool, error) { return sm.buildIDSet(path) } +// BuildIDToTimestampMap reads a JSONL file and returns a map of issue ID to updated_at timestamp. +// This is used for timestamp-aware snapshot protection (GH#865): only protect local issues +// if they are newer than incoming remote versions. +func (sm *SnapshotManager) BuildIDToTimestampMap(path string) (map[string]time.Time, error) { + result := make(map[string]time.Time) + + // #nosec G304 -- snapshot file path derived from internal state + f, err := os.Open(path) + if err != nil { + if os.IsNotExist(err) { + return result, nil // Empty map for missing files + } + return nil, err + } + defer f.Close() + + scanner := bufio.NewScanner(f) + for scanner.Scan() { + line := scanner.Text() + if line == "" { + continue + } + + // Parse ID and updated_at fields + var issue struct { + ID string `json:"id"` + UpdatedAt time.Time `json:"updated_at"` + } + if err := json.Unmarshal([]byte(line), &issue); err != nil { + return nil, fmt.Errorf("failed to parse issue from line: %w", err) + } + + result[issue.ID] = issue.UpdatedAt + } + + if err := scanner.Err(); err != nil { + return nil, err + } + + return result, nil +} + // Private helper methods func (sm *SnapshotManager) createMetadata() snapshotMetadata { diff --git a/internal/importer/importer.go b/internal/importer/importer.go index c931fce5..af424258 100644 --- a/internal/importer/importer.go +++ b/internal/importer/importer.go @@ -39,7 +39,7 @@ type Options struct { SkipPrefixValidation bool // Skip prefix validation (for auto-import) OrphanHandling OrphanHandling // How to handle missing parent issues (default: allow) ClearDuplicateExternalRefs bool // Clear duplicate external_ref values instead of erroring - ProtectLocalExportIDs map[string]bool // IDs from left snapshot to protect from deletion + ProtectLocalExportIDs map[string]time.Time // IDs from left snapshot with timestamps for timestamp-aware protection (GH#865) } // Result contains statistics about the import operation @@ -565,6 +565,12 @@ func upsertIssues(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, issues if existing, found := dbByExternalRef[*incoming.ExternalRef]; found { // Found match by external_ref - update the existing issue if !opts.SkipUpdate { + // GH#865: Check timestamp-aware protection first + // If local snapshot has a newer version, protect it from being overwritten + if shouldProtectFromUpdate(existing.ID, incoming.UpdatedAt, opts.ProtectLocalExportIDs) { + result.Skipped++ + continue + } // Check timestamps - only update if incoming is newer if !incoming.UpdatedAt.After(existing.UpdatedAt) { // Local version is newer or same - skip update @@ -663,6 +669,12 @@ func upsertIssues(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, issues // The update should have been detected earlier by detectUpdates // If we reach here, it means collision wasn't resolved - treat as update if !opts.SkipUpdate { + // GH#865: Check timestamp-aware protection first + // If local snapshot has a newer version, protect it from being overwritten + if shouldProtectFromUpdate(incoming.ID, incoming.UpdatedAt, opts.ProtectLocalExportIDs) { + result.Skipped++ + continue + } // Check timestamps - only update if incoming is newer if !incoming.UpdatedAt.After(existingWithID.UpdatedAt) { // Local version is newer or same - skip update @@ -919,6 +931,23 @@ func importComments(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, issu return nil } +// shouldProtectFromUpdate checks if an update should be skipped due to timestamp-aware protection (GH#865). +// Returns true if the update should be skipped (local is newer), false if the update should proceed. +// If the issue is not in the protection map, returns false (allow update). +func shouldProtectFromUpdate(issueID string, incomingTime time.Time, protectMap map[string]time.Time) bool { + if protectMap == nil { + return false + } + localTime, exists := protectMap[issueID] + if !exists { + // Issue not in protection map - allow update + return false + } + // Only protect if local snapshot is newer than or equal to incoming + // If incoming is newer, allow the update + return !incomingTime.After(localTime) +} + func GetPrefixList(prefixes map[string]int) []string { var result []string keys := make([]string, 0, len(prefixes)) diff --git a/internal/importer/timestamp_test.go b/internal/importer/timestamp_test.go index e26e7ec9..3af86921 100644 --- a/internal/importer/timestamp_test.go +++ b/internal/importer/timestamp_test.go @@ -205,6 +205,274 @@ func TestImportSameTimestamp(t *testing.T) { } } +// TestImportTimestampAwareProtection tests the GH#865 fix: timestamp-aware snapshot protection +// The ProtectLocalExportIDs map should only protect issues if the local snapshot version is newer. +func TestImportTimestampAwareProtection(t *testing.T) { + tmpDir := t.TempDir() + dbPath := filepath.Join(tmpDir, "test.db") + + store, err := sqlite.New(context.Background(), dbPath) + if err != nil { + t.Fatalf("Failed to create storage: %v", err) + } + defer store.Close() + + ctx := context.Background() + + if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { + t.Fatalf("Failed to set prefix: %v", err) + } + + now := time.Now() + + // Create a local issue in the database + localIssue := &types.Issue{ + ID: "bd-protect1", + Title: "Test Issue", + Description: "Local version", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + CreatedAt: now.Add(-2 * time.Hour), + UpdatedAt: now.Add(-1 * time.Hour), // DB has old timestamp + } + localIssue.ContentHash = localIssue.ComputeContentHash() + + if err := store.CreateIssue(ctx, localIssue, "test"); err != nil { + t.Fatalf("Failed to create local issue: %v", err) + } + + t.Run("incoming newer than local snapshot - should update", func(t *testing.T) { + // Scenario: Remote closed the issue after we exported locally + // Local snapshot: issue open at T1 (10:00) + // Incoming: issue closed at T2 (11:30) - NEWER + // Expected: Update should proceed (remote is newer) + + snapshotTime := now.Add(-30 * time.Minute) // Local snapshot at 10:00 + incomingTime := now // Incoming at 11:30 (newer) + closedAt := incomingTime + + incomingIssue := &types.Issue{ + ID: "bd-protect1", + Title: "Test Issue", + Description: "Remote closed version", + Status: types.StatusClosed, + Priority: 1, + IssueType: types.TypeTask, + CreatedAt: now.Add(-2 * time.Hour), + UpdatedAt: incomingTime, + ClosedAt: &closedAt, + } + incomingIssue.ContentHash = incomingIssue.ComputeContentHash() + + // Protection map has the issue with the local snapshot timestamp + protectMap := map[string]time.Time{ + "bd-protect1": snapshotTime, + } + + result, err := ImportIssues(ctx, dbPath, store, []*types.Issue{incomingIssue}, Options{ + SkipPrefixValidation: true, + ProtectLocalExportIDs: protectMap, + }) + if err != nil { + t.Fatalf("Import failed: %v", err) + } + + // Incoming is newer than snapshot, so update should proceed + if result.Updated == 0 { + t.Errorf("Expected 1 update (incoming newer than snapshot), got 0") + } + if result.Skipped > 0 { + t.Errorf("Expected 0 skipped, got %d", result.Skipped) + } + + // Verify the issue was updated to closed + dbIssue, err := store.GetIssue(ctx, "bd-protect1") + if err != nil { + t.Fatalf("Failed to get issue: %v", err) + } + if dbIssue.Status != types.StatusClosed { + t.Errorf("Expected status=closed (remote version), got %s", dbIssue.Status) + } + }) + + t.Run("incoming older than local snapshot - should protect", func(t *testing.T) { + // Reset the issue for next test + resetIssue := &types.Issue{ + ID: "bd-protect2", + Title: "Another Issue", + Description: "Local modified version", + Status: types.StatusInProgress, + Priority: 2, + IssueType: types.TypeBug, + CreatedAt: now.Add(-2 * time.Hour), + UpdatedAt: now, // Current timestamp + } + resetIssue.ContentHash = resetIssue.ComputeContentHash() + if err := store.CreateIssue(ctx, resetIssue, "test"); err != nil { + t.Fatalf("Failed to create reset issue: %v", err) + } + + // Scenario: We modified locally, remote has older version + // Local snapshot: issue in_progress at T2 (11:30) + // Incoming: issue open at T1 (10:00) - OLDER + // Expected: Skip update (protect local changes) + + snapshotTime := now // Local snapshot at 11:30 + incomingTime := now.Add(-30 * time.Minute) // Incoming at 10:00 (older) + + incomingIssue := &types.Issue{ + ID: "bd-protect2", + Title: "Another Issue", + Description: "Old remote version", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeBug, + CreatedAt: now.Add(-2 * time.Hour), + UpdatedAt: incomingTime, + } + incomingIssue.ContentHash = incomingIssue.ComputeContentHash() + + // Protection map has the issue with the local snapshot timestamp + protectMap := map[string]time.Time{ + "bd-protect2": snapshotTime, + } + + result, err := ImportIssues(ctx, dbPath, store, []*types.Issue{incomingIssue}, Options{ + SkipPrefixValidation: true, + ProtectLocalExportIDs: protectMap, + }) + if err != nil { + t.Fatalf("Import failed: %v", err) + } + + // Incoming is older than snapshot, so update should be skipped (protected) + if result.Skipped == 0 { + t.Errorf("Expected 1 skipped (local snapshot newer), got 0") + } + if result.Updated > 0 { + t.Errorf("Expected 0 updates, got %d", result.Updated) + } + + // Verify the issue was NOT updated + dbIssue, err := store.GetIssue(ctx, "bd-protect2") + if err != nil { + t.Fatalf("Failed to get issue: %v", err) + } + if dbIssue.Status != types.StatusInProgress { + t.Errorf("Expected status=in_progress (protected local), got %s", dbIssue.Status) + } + }) + + t.Run("issue not in protection map - normal behavior", func(t *testing.T) { + // Create an issue not in the protection map + unprotectedIssue := &types.Issue{ + ID: "bd-unprotected", + Title: "Unprotected Issue", + Description: "Original", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + CreatedAt: now.Add(-2 * time.Hour), + UpdatedAt: now.Add(-1 * time.Hour), + } + unprotectedIssue.ContentHash = unprotectedIssue.ComputeContentHash() + if err := store.CreateIssue(ctx, unprotectedIssue, "test"); err != nil { + t.Fatalf("Failed to create unprotected issue: %v", err) + } + + // Incoming version is newer + closedAt := now + incomingIssue := &types.Issue{ + ID: "bd-unprotected", + Title: "Unprotected Issue", + Description: "Updated", + Status: types.StatusClosed, + Priority: 1, + IssueType: types.TypeTask, + CreatedAt: now.Add(-2 * time.Hour), + UpdatedAt: now, // Newer than DB + ClosedAt: &closedAt, + } + incomingIssue.ContentHash = incomingIssue.ComputeContentHash() + + // Protection map does NOT contain this issue + protectMap := map[string]time.Time{ + "bd-other": now, // Different issue + } + + result, err := ImportIssues(ctx, dbPath, store, []*types.Issue{incomingIssue}, Options{ + SkipPrefixValidation: true, + ProtectLocalExportIDs: protectMap, + }) + if err != nil { + t.Fatalf("Import failed: %v", err) + } + + // Issue not in protection map, incoming is newer - should update + if result.Updated == 0 { + t.Errorf("Expected 1 update (not in protection map), got 0") + } + + dbIssue, err := store.GetIssue(ctx, "bd-unprotected") + if err != nil { + t.Fatalf("Failed to get issue: %v", err) + } + if dbIssue.Status != types.StatusClosed { + t.Errorf("Expected status=closed (updated), got %s", dbIssue.Status) + } + }) +} + +// TestShouldProtectFromUpdate tests the helper function directly +func TestShouldProtectFromUpdate(t *testing.T) { + now := time.Now() + + t.Run("nil map - no protection", func(t *testing.T) { + if shouldProtectFromUpdate("bd-123", now, nil) { + t.Error("Expected no protection with nil map") + } + }) + + t.Run("issue not in map - no protection", func(t *testing.T) { + protectMap := map[string]time.Time{ + "bd-other": now, + } + if shouldProtectFromUpdate("bd-123", now, protectMap) { + t.Error("Expected no protection when issue not in map") + } + }) + + t.Run("incoming newer than local - no protection", func(t *testing.T) { + localTime := now.Add(-1 * time.Hour) + protectMap := map[string]time.Time{ + "bd-123": localTime, + } + if shouldProtectFromUpdate("bd-123", now, protectMap) { + t.Error("Expected no protection when incoming is newer") + } + }) + + t.Run("incoming same as local - protect", func(t *testing.T) { + protectMap := map[string]time.Time{ + "bd-123": now, + } + if !shouldProtectFromUpdate("bd-123", now, protectMap) { + t.Error("Expected protection when timestamps are equal") + } + }) + + t.Run("incoming older than local - protect", func(t *testing.T) { + localTime := now.Add(1 * time.Hour) // Local is newer + protectMap := map[string]time.Time{ + "bd-123": localTime, + } + if !shouldProtectFromUpdate("bd-123", now, protectMap) { + t.Error("Expected protection when local is newer") + } + }) +} + func TestMain(m *testing.M) { // Ensure test DB files are cleaned up code := m.Run()