From e816e91ecb2d65f7bfe038187e6530a61000bd92 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Fri, 21 Nov 2025 11:40:37 -0500 Subject: [PATCH] Complete bd-ar2 P2 tasks: metadata, resurrection, and testing improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit addresses the remaining P2 tasks from bd-ar2 code review follow-up: ## Completed Tasks ### bd-ar2.4: Improve parent chain resurrection - Modified `tryResurrectParentWithConn()` to recursively resurrect ancestor chain - When resurrecting bd-root.1.2, now also resurrects bd-root.1 if missing - Handles deeply nested hierarchies where intermediate parents are deleted - All resurrection tests pass including new edge cases ### bd-ar2.5: Add error handling guidance - Documented metadata update failure strategy in `updateExportMetadata()` - Explained trade-off: warnings vs errors (safe, prevents data loss) - Added user-facing message: "Next export may require running 'bd import' first" - Clarifies that worst case is requiring import before next export ### bd-ar2.6: Document transaction boundaries - Added comprehensive documentation for atomicity trade-offs - Explained crash scenarios and recovery (bd import) - Documented decision to defer defensive checks (Option 4) until needed - No code changes - current approach is acceptable for now ### bd-ar2.12: Add metadata key validation - Added keySuffix validation in `updateExportMetadata()` and `hasJSONLChanged()` - Prevents ':' separator in keySuffix to avoid malformed metadata keys - Documented metadata key format in function comments - Single-repo: "last_import_hash", multi-repo: "last_import_hash:" ### bd-ar2.7: Add edge case tests for GetNextChildID resurrection - TestGetNextChildID_ResurrectParent_NotInJSONL: parent not in history - TestGetNextChildID_ResurrectParent_NoJSONL: missing JSONL file - TestGetNextChildID_ResurrectParent_MalformedJSONL: invalid JSON lines - TestGetNextChildID_ResurrectParentChain: deeply nested missing parents - All tests pass, resurrection is robust against edge cases ## Files Changed - cmd/bd/daemon_sync.go: Metadata validation, error handling docs - cmd/bd/integrity.go: Added strings import, keySuffix validation - internal/storage/sqlite/hash_ids.go: Improved resurrection comments - internal/storage/sqlite/resurrection.go: Recursive ancestor resurrection - internal/storage/sqlite/child_id_test.go: Added 4 new edge case tests ## Testing All export, sync, metadata, and resurrection tests pass. Edge cases properly handled: missing JSONL, malformed JSON, deep nesting. ## Remaining Tasks - bd-ar2.8 (P3): Additional export metadata edge case tests (deferred) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- cmd/bd/daemon_sync.go | 26 +++++ cmd/bd/integrity.go | 8 ++ internal/storage/sqlite/child_id_test.go | 141 +++++++++++++++++++++++ internal/storage/sqlite/hash_ids.go | 6 +- internal/storage/sqlite/resurrection.go | 16 ++- 5 files changed, 195 insertions(+), 2 deletions(-) diff --git a/cmd/bd/daemon_sync.go b/cmd/bd/daemon_sync.go index 7e61292b..9354c9fe 100644 --- a/cmd/bd/daemon_sync.go +++ b/cmd/bd/daemon_sync.go @@ -241,7 +241,27 @@ func getRepoKeyForPath(jsonlPath string) string { // updateExportMetadata updates last_import_hash and related metadata after a successful export. // This prevents "JSONL content has changed since last import" errors on subsequent exports (bd-ymj fix). // In multi-repo mode, keySuffix should be the stable repo identifier (e.g., ".", "../frontend"). +// +// Metadata key format (bd-ar2.12): +// - Single-repo mode: "last_import_hash", "last_import_time", "last_import_mtime" +// - Multi-repo mode: "last_import_hash:", "last_import_time:", etc. +// where is a stable repo identifier like "." or "../frontend" +// +// Transaction boundaries (bd-ar2.6): +// This function does NOT provide atomicity between JSONL write, metadata updates, and DB mtime. +// If a crash occurs between these operations, metadata may be inconsistent. However, this is +// acceptable because: +// 1. The worst case is "JSONL content has changed" error on next export +// 2. User can fix by running 'bd import' (safe, no data loss) +// 3. Current approach is simple and doesn't require complex WAL or format changes +// Future: Consider Option 4 (defensive checks on startup) if this becomes a common issue. func updateExportMetadata(ctx context.Context, store storage.Storage, jsonlPath string, log daemonLogger, keySuffix string) { + // Validate keySuffix doesn't contain the separator character (bd-ar2.12) + if keySuffix != "" && strings.Contains(keySuffix, ":") { + log.log("Error: invalid keySuffix contains ':' separator: %s", keySuffix) + return + } + currentHash, err := computeJSONLHash(jsonlPath) if err != nil { log.log("Warning: failed to compute JSONL hash for metadata update: %v", err) @@ -258,8 +278,14 @@ func updateExportMetadata(ctx context.Context, store storage.Storage, jsonlPath mtimeKey += ":" + keySuffix } + // Note: Metadata update failures are treated as warnings, not errors (bd-ar2.5). + // This is acceptable because the worst case is the next export will require + // an import first, which is safe and prevents data loss. + // Alternative: Make this critical and fail the export if metadata updates fail, + // but this makes exports more fragile and doesn't prevent data corruption. if err := store.SetMetadata(ctx, hashKey, currentHash); err != nil { log.log("Warning: failed to update %s: %v", hashKey, err) + log.log("Next export may require running 'bd import' first") } exportTime := time.Now().Format(time.RFC3339) diff --git a/cmd/bd/integrity.go b/cmd/bd/integrity.go index 1d80b0cb..c18b3b7d 100644 --- a/cmd/bd/integrity.go +++ b/cmd/bd/integrity.go @@ -12,6 +12,7 @@ import ( "os" "path/filepath" "sort" + "strings" "github.com/steveyegge/beads/internal/storage" "github.com/steveyegge/beads/internal/types" @@ -96,7 +97,14 @@ func computeJSONLHash(jsonlPath string) (string, error) { // unchanged) while still catching git operations that restore old content with new mtimes. // // In multi-repo mode, keySuffix should be the stable repo identifier (e.g., ".", "../frontend"). +// The keySuffix must not contain the ':' separator character (bd-ar2.12). func hasJSONLChanged(ctx context.Context, store storage.Storage, jsonlPath string, keySuffix string) bool { + // Validate keySuffix doesn't contain the separator character (bd-ar2.12) + if keySuffix != "" && strings.Contains(keySuffix, ":") { + // Invalid keySuffix - treat as changed to trigger proper error handling + return true + } + // Build metadata keys with optional suffix for per-repo tracking (bd-ar2.10, bd-ar2.11) hashKey := "last_import_hash" mtimeKey := "last_import_mtime" diff --git a/internal/storage/sqlite/child_id_test.go b/internal/storage/sqlite/child_id_test.go index 0a3eb04b..e2c9ffb5 100644 --- a/internal/storage/sqlite/child_id_test.go +++ b/internal/storage/sqlite/child_id_test.go @@ -274,3 +274,144 @@ func TestGetNextChildID_ResurrectParent(t *testing.T) { t.Errorf("expected resurrected parent title to be preserved, got %s", resurrectedParent.Title) } } + +// TestGetNextChildID_ResurrectParent_NotInJSONL tests resurrection when parent doesn't exist in JSONL (bd-ar2.7) +func TestGetNextChildID_ResurrectParent_NotInJSONL(t *testing.T) { + tmpDir := t.TempDir() + tmpFile := tmpDir + "/test.db" + defer os.Remove(tmpFile) + store := newTestStore(t, tmpFile) + defer store.Close() + ctx := context.Background() + + // Create empty JSONL file (parent not in history) + jsonlPath := tmpDir + "/issues.jsonl" + if err := os.WriteFile(jsonlPath, []byte(""), 0600); err != nil { + t.Fatalf("failed to create JSONL file: %v", err) + } + + // Attempt to get child ID for non-existent parent not in JSONL + _, err := store.GetNextChildID(ctx, "bd-notfound") + if err == nil { + t.Errorf("expected error for parent not in JSONL, got nil") + } + expectedErr := "parent issue bd-notfound does not exist and could not be resurrected from JSONL history" + if err != nil && err.Error() != expectedErr { + t.Errorf("unexpected error: got %q, want %q", err.Error(), expectedErr) + } +} + +// TestGetNextChildID_ResurrectParent_NoJSONL tests resurrection when JSONL file doesn't exist (bd-ar2.7) +func TestGetNextChildID_ResurrectParent_NoJSONL(t *testing.T) { + tmpDir := t.TempDir() + tmpFile := tmpDir + "/test.db" + defer os.Remove(tmpFile) + store := newTestStore(t, tmpFile) + defer store.Close() + ctx := context.Background() + + // No JSONL file created + // Attempt to get child ID for non-existent parent + _, err := store.GetNextChildID(ctx, "bd-missing") + if err == nil { + t.Errorf("expected error for parent with no JSONL, got nil") + } + expectedErr := "parent issue bd-missing does not exist and could not be resurrected from JSONL history" + if err != nil && err.Error() != expectedErr { + t.Errorf("unexpected error: got %q, want %q", err.Error(), expectedErr) + } +} + +// TestGetNextChildID_ResurrectParent_MalformedJSONL tests resurrection with invalid JSON lines (bd-ar2.7) +func TestGetNextChildID_ResurrectParent_MalformedJSONL(t *testing.T) { + tmpDir := t.TempDir() + tmpFile := tmpDir + "/test.db" + defer os.Remove(tmpFile) + store := newTestStore(t, tmpFile) + defer store.Close() + ctx := context.Background() + + // Create JSONL with malformed lines and one valid parent + jsonlPath := tmpDir + "/issues.jsonl" + jsonlContent := `{invalid json +{"id":"bd-test456","content_hash":"def456","title":"Valid Parent","description":"Should be found","status":"open","priority":1,"type":"epic","created_at":"2025-01-01T00:00:00Z","updated_at":"2025-01-01T00:00:00Z"} +this is not json either +` + if err := os.WriteFile(jsonlPath, []byte(jsonlContent), 0600); err != nil { + t.Fatalf("failed to create JSONL file: %v", err) + } + + // Should successfully resurrect despite malformed lines + childID, err := store.GetNextChildID(ctx, "bd-test456") + if err != nil { + t.Fatalf("GetNextChildID should skip malformed lines and resurrect valid parent, got error: %v", err) + } + + expectedID := "bd-test456.1" + if childID != expectedID { + t.Errorf("expected child ID %s, got %s", expectedID, childID) + } +} + +// TestGetNextChildID_ResurrectParentChain tests resurrection of deeply nested missing parents (bd-ar2.7) +func TestGetNextChildID_ResurrectParentChain(t *testing.T) { + tmpDir := t.TempDir() + tmpFile := tmpDir + "/test.db" + defer os.Remove(tmpFile) + store := newTestStore(t, tmpFile) + defer store.Close() + ctx := context.Background() + + // Create root parent only + root := &types.Issue{ + ID: "bd-root", + ContentHash: "root123", + Title: "Root Issue", + Description: "Root", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeEpic, + } + if err := store.CreateIssue(ctx, root, "test"); err != nil { + t.Fatalf("failed to create root: %v", err) + } + + // Create JSONL with intermediate parents that are deleted + jsonlPath := tmpDir + "/issues.jsonl" + jsonlContent := `{"id":"bd-root","content_hash":"root123","title":"Root Issue","description":"Root","status":"open","priority":1,"type":"epic","created_at":"2025-01-01T00:00:00Z","updated_at":"2025-01-01T00:00:00Z"} +{"id":"bd-root.1","content_hash":"l1abc","title":"Level 1","description":"First level","status":"open","priority":1,"type":"task","created_at":"2025-01-01T00:00:00Z","updated_at":"2025-01-01T00:00:00Z"} +{"id":"bd-root.1.2","content_hash":"l2abc","title":"Level 2","description":"Second level","status":"open","priority":1,"type":"task","created_at":"2025-01-01T00:00:00Z","updated_at":"2025-01-01T00:00:00Z"} +` + if err := os.WriteFile(jsonlPath, []byte(jsonlContent), 0600); err != nil { + t.Fatalf("failed to create JSONL file: %v", err) + } + + // Try to create child of bd-root.1.2 (which doesn't exist in DB, but its parent bd-root.1 also doesn't exist) + // With TryResurrectParentChain (bd-ar2.4), this should work + childID, err := store.GetNextChildID(ctx, "bd-root.1.2") + if err != nil { + t.Fatalf("GetNextChildID should resurrect entire parent chain, got error: %v", err) + } + + expectedID := "bd-root.1.2.1" + if childID != expectedID { + t.Errorf("expected child ID %s, got %s", expectedID, childID) + } + + // Verify both intermediate parents were resurrected + parent1, err := store.GetIssue(ctx, "bd-root.1") + if err != nil { + t.Fatalf("bd-root.1 should have been resurrected: %v", err) + } + if parent1.Status != types.StatusClosed { + t.Errorf("expected resurrected parent to be closed, got %s", parent1.Status) + } + + parent2, err := store.GetIssue(ctx, "bd-root.1.2") + if err != nil { + t.Fatalf("bd-root.1.2 should have been resurrected: %v", err) + } + if parent2.Status != types.StatusClosed { + t.Errorf("expected resurrected parent to be closed, got %s", parent2.Status) + } +} diff --git a/internal/storage/sqlite/hash_ids.go b/internal/storage/sqlite/hash_ids.go index 3614390c..90db2dc6 100644 --- a/internal/storage/sqlite/hash_ids.go +++ b/internal/storage/sqlite/hash_ids.go @@ -34,7 +34,11 @@ func (s *SQLiteStorage) GetNextChildID(ctx context.Context, parentID string) (st return "", fmt.Errorf("failed to check parent existence: %w", err) } if count == 0 { - // Try to resurrect parent from JSONL history before failing (bd-dvd fix) + // Try to resurrect parent from JSONL history before failing (bd-dvd fix, bd-ar2.4) + // Note: Using TryResurrectParent instead of TryResurrectParentChain because we're + // already given the direct parent ID. TryResurrectParent will handle the direct parent, + // and if the parent itself has missing ancestors, those should have been resurrected + // when the parent was originally created. resurrected, resurrectErr := s.TryResurrectParent(ctx, parentID) if resurrectErr != nil { return "", fmt.Errorf("failed to resurrect parent %s: %w", parentID, resurrectErr) diff --git a/internal/storage/sqlite/resurrection.go b/internal/storage/sqlite/resurrection.go index 5e6ee192..39b0cf21 100644 --- a/internal/storage/sqlite/resurrection.go +++ b/internal/storage/sqlite/resurrection.go @@ -47,7 +47,21 @@ func (s *SQLiteStorage) tryResurrectParentWithConn(ctx context.Context, conn *sq if count > 0 { return true, nil // Parent already exists, nothing to do } - + + // Before resurrecting this parent, ensure its entire ancestor chain exists (bd-ar2.4) + // This handles deeply nested cases where we're resurrecting bd-root.1.2 and bd-root.1 is also missing + ancestors := extractParentChain(parentID) + for _, ancestor := range ancestors { + // Recursively resurrect each ancestor in the chain + resurrected, err := s.tryResurrectParentWithConn(ctx, conn, ancestor) + if err != nil { + return false, fmt.Errorf("failed to resurrect ancestor %s: %w", ancestor, err) + } + if !resurrected { + return false, nil // Ancestor not found in history, can't continue + } + } + // Parent doesn't exist - try to find it in JSONL history parentIssue, err := s.findIssueInJSONL(parentID) if err != nil {