From c7e119a1cc34ff81bac735098aa0f6c487728d61 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Sun, 14 Dec 2025 17:21:44 -0800 Subject: [PATCH] fix(import): defensive handling for closed issues missing closed_at timestamp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes GH#523: older versions of bd could close issues without setting the closed_at timestamp. When importing such issues, validation would fail with "closed issues must have closed_at timestamp". This fix adds defensive handling in all issue creation/validation paths: - If status is "closed" and closed_at is nil, set closed_at to max(created_at, updated_at) + 1 second - Similarly for tombstones missing deleted_at Applied to: - batch_ops.go: validateBatchIssuesWithCustomStatuses (main import path) - transaction.go: CreateIssue and CreateIssues - queries.go: CreateIssue - multirepo.go: upsertIssueInTx Also adds comprehensive tests for the defensive fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- internal/storage/sqlite/batch_ops.go | 21 ++++ internal/storage/sqlite/batch_ops_test.go | 116 ++++++++++++++++++++++ internal/storage/sqlite/multirepo.go | 21 ++++ internal/storage/sqlite/queries.go | 35 ++++++- internal/storage/sqlite/transaction.go | 66 ++++++++++-- 5 files changed, 247 insertions(+), 12 deletions(-) diff --git a/internal/storage/sqlite/batch_ops.go b/internal/storage/sqlite/batch_ops.go index dcbc52e3..e42a01d4 100644 --- a/internal/storage/sqlite/batch_ops.go +++ b/internal/storage/sqlite/batch_ops.go @@ -32,6 +32,27 @@ func validateBatchIssuesWithCustomStatuses(issues []*types.Issue, customStatuses issue.UpdatedAt = now } + // Defensive fix for closed_at invariant (GH#523): older versions of bd could + // close issues without setting closed_at. Fix by using max(created_at, updated_at) + 1s. + if issue.Status == types.StatusClosed && issue.ClosedAt == nil { + maxTime := issue.CreatedAt + if issue.UpdatedAt.After(maxTime) { + maxTime = issue.UpdatedAt + } + closedAt := maxTime.Add(time.Second) + issue.ClosedAt = &closedAt + } + + // Defensive fix for deleted_at invariant: tombstones must have deleted_at + if issue.Status == types.StatusTombstone && issue.DeletedAt == nil { + maxTime := issue.CreatedAt + if issue.UpdatedAt.After(maxTime) { + maxTime = issue.UpdatedAt + } + deletedAt := maxTime.Add(time.Second) + issue.DeletedAt = &deletedAt + } + if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil { return fmt.Errorf("validation failed for issue %d: %w", i, err) } diff --git a/internal/storage/sqlite/batch_ops_test.go b/internal/storage/sqlite/batch_ops_test.go index fba2bc8e..9967fe85 100644 --- a/internal/storage/sqlite/batch_ops_test.go +++ b/internal/storage/sqlite/batch_ops_test.go @@ -480,3 +480,119 @@ func TestBulkOperations(t *testing.T) { } }) } + +// TestDefensiveClosedAtFix tests GH#523 - closed issues without closed_at timestamp +// from older versions of bd should be automatically fixed during import. +func TestDefensiveClosedAtFix(t *testing.T) { + t.Run("sets closed_at for closed issues missing it", func(t *testing.T) { + now := time.Now() + pastTime := now.Add(-24 * time.Hour) + + issues := []*types.Issue{ + { + Title: "Closed issue without closed_at", + Priority: 1, + IssueType: "task", + Status: "closed", + CreatedAt: pastTime, + UpdatedAt: pastTime.Add(time.Hour), + // ClosedAt intentionally NOT set - simulating old bd data + }, + } + + err := validateBatchIssues(issues) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // closed_at should be set to max(created_at, updated_at) + 1 second + if issues[0].ClosedAt == nil { + t.Fatal("closed_at should have been set") + } + + expectedClosedAt := pastTime.Add(time.Hour).Add(time.Second) + if !issues[0].ClosedAt.Equal(expectedClosedAt) { + t.Errorf("closed_at mismatch: want %v, got %v", expectedClosedAt, *issues[0].ClosedAt) + } + }) + + t.Run("preserves existing closed_at", func(t *testing.T) { + now := time.Now() + pastTime := now.Add(-24 * time.Hour) + closedTime := pastTime.Add(2 * time.Hour) + + issues := []*types.Issue{ + { + Title: "Closed issue with closed_at", + Priority: 1, + IssueType: "task", + Status: "closed", + CreatedAt: pastTime, + UpdatedAt: pastTime.Add(time.Hour), + ClosedAt: &closedTime, + }, + } + + err := validateBatchIssues(issues) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // closed_at should be preserved + if !issues[0].ClosedAt.Equal(closedTime) { + t.Errorf("closed_at should be preserved: want %v, got %v", closedTime, *issues[0].ClosedAt) + } + }) + + t.Run("does not set closed_at for open issues", func(t *testing.T) { + issues := []*types.Issue{ + { + Title: "Open issue", + Priority: 1, + IssueType: "task", + Status: "open", + }, + } + + err := validateBatchIssues(issues) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if issues[0].ClosedAt != nil { + t.Error("closed_at should remain nil for open issues") + } + }) + + t.Run("sets deleted_at for tombstones missing it", func(t *testing.T) { + now := time.Now() + pastTime := now.Add(-24 * time.Hour) + + issues := []*types.Issue{ + { + Title: "Tombstone without deleted_at", + Priority: 1, + IssueType: "task", + Status: "tombstone", + CreatedAt: pastTime, + UpdatedAt: pastTime.Add(time.Hour), + // DeletedAt intentionally NOT set + }, + } + + err := validateBatchIssues(issues) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // deleted_at should be set to max(created_at, updated_at) + 1 second + if issues[0].DeletedAt == nil { + t.Fatal("deleted_at should have been set") + } + + expectedDeletedAt := pastTime.Add(time.Hour).Add(time.Second) + if !issues[0].DeletedAt.Equal(expectedDeletedAt) { + t.Errorf("deleted_at mismatch: want %v, got %v", expectedDeletedAt, *issues[0].DeletedAt) + } + }) +} diff --git a/internal/storage/sqlite/multirepo.go b/internal/storage/sqlite/multirepo.go index a307085e..12a3efce 100644 --- a/internal/storage/sqlite/multirepo.go +++ b/internal/storage/sqlite/multirepo.go @@ -227,6 +227,27 @@ func (s *SQLiteStorage) importJSONLFile(ctx context.Context, jsonlPath, sourceRe // upsertIssueInTx inserts or updates an issue within a transaction. // Uses INSERT OR REPLACE to handle both new and existing issues. func (s *SQLiteStorage) upsertIssueInTx(ctx context.Context, tx *sql.Tx, issue *types.Issue, customStatuses []string) error { + // Defensive fix for closed_at invariant (GH#523): older versions of bd could + // close issues without setting closed_at. Fix by using max(created_at, updated_at) + 1s. + if issue.Status == types.StatusClosed && issue.ClosedAt == nil { + maxTime := issue.CreatedAt + if issue.UpdatedAt.After(maxTime) { + maxTime = issue.UpdatedAt + } + closedAt := maxTime.Add(time.Second) + issue.ClosedAt = &closedAt + } + + // Defensive fix for deleted_at invariant: tombstones must have deleted_at + if issue.Status == types.StatusTombstone && issue.DeletedAt == nil { + maxTime := issue.CreatedAt + if issue.UpdatedAt.After(maxTime) { + maxTime = issue.UpdatedAt + } + deletedAt := maxTime.Add(time.Second) + issue.DeletedAt = &deletedAt + } + // Validate issue (with custom status support, bd-1pj6) if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil { return fmt.Errorf("validation failed: %w", err) diff --git a/internal/storage/sqlite/queries.go b/internal/storage/sqlite/queries.go index 37e6330e..1a27cfaa 100644 --- a/internal/storage/sqlite/queries.go +++ b/internal/storage/sqlite/queries.go @@ -50,16 +50,41 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act return fmt.Errorf("failed to get custom statuses: %w", err) } + // Set timestamps first so defensive fixes can use them + now := time.Now() + if issue.CreatedAt.IsZero() { + issue.CreatedAt = now + } + if issue.UpdatedAt.IsZero() { + issue.UpdatedAt = now + } + + // Defensive fix for closed_at invariant (GH#523): older versions of bd could + // close issues without setting closed_at. Fix by using max(created_at, updated_at) + 1s. + if issue.Status == types.StatusClosed && issue.ClosedAt == nil { + maxTime := issue.CreatedAt + if issue.UpdatedAt.After(maxTime) { + maxTime = issue.UpdatedAt + } + closedAt := maxTime.Add(time.Second) + issue.ClosedAt = &closedAt + } + + // Defensive fix for deleted_at invariant: tombstones must have deleted_at + if issue.Status == types.StatusTombstone && issue.DeletedAt == nil { + maxTime := issue.CreatedAt + if issue.UpdatedAt.After(maxTime) { + maxTime = issue.UpdatedAt + } + deletedAt := maxTime.Add(time.Second) + issue.DeletedAt = &deletedAt + } + // Validate issue before creating (with custom status support) if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil { return fmt.Errorf("validation failed: %w", err) } - // Set timestamps - now := time.Now() - issue.CreatedAt = now - issue.UpdatedAt = now - // Compute content hash (bd-95) if issue.ContentHash == "" { issue.ContentHash = issue.ComputeContentHash() diff --git a/internal/storage/sqlite/transaction.go b/internal/storage/sqlite/transaction.go index b143fbde..d9b069f7 100644 --- a/internal/storage/sqlite/transaction.go +++ b/internal/storage/sqlite/transaction.go @@ -97,16 +97,41 @@ func (t *sqliteTxStorage) CreateIssue(ctx context.Context, issue *types.Issue, a return fmt.Errorf("failed to get custom statuses: %w", err) } + // Set timestamps first so defensive fixes can use them + now := time.Now() + if issue.CreatedAt.IsZero() { + issue.CreatedAt = now + } + if issue.UpdatedAt.IsZero() { + issue.UpdatedAt = now + } + + // Defensive fix for closed_at invariant (GH#523): older versions of bd could + // close issues without setting closed_at. Fix by using max(created_at, updated_at) + 1s. + if issue.Status == types.StatusClosed && issue.ClosedAt == nil { + maxTime := issue.CreatedAt + if issue.UpdatedAt.After(maxTime) { + maxTime = issue.UpdatedAt + } + closedAt := maxTime.Add(time.Second) + issue.ClosedAt = &closedAt + } + + // Defensive fix for deleted_at invariant: tombstones must have deleted_at + if issue.Status == types.StatusTombstone && issue.DeletedAt == nil { + maxTime := issue.CreatedAt + if issue.UpdatedAt.After(maxTime) { + maxTime = issue.UpdatedAt + } + deletedAt := maxTime.Add(time.Second) + issue.DeletedAt = &deletedAt + } + // Validate issue before creating (with custom status support) if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil { return fmt.Errorf("validation failed: %w", err) } - // Set timestamps - now := time.Now() - issue.CreatedAt = now - issue.UpdatedAt = now - // Compute content hash (bd-95) if issue.ContentHash == "" { issue.ContentHash = issue.ComputeContentHash() @@ -185,11 +210,38 @@ func (t *sqliteTxStorage) CreateIssues(ctx context.Context, issues []*types.Issu // Validate and prepare all issues first (with custom status support) now := time.Now() for _, issue := range issues { + // Set timestamps first so defensive fixes can use them + if issue.CreatedAt.IsZero() { + issue.CreatedAt = now + } + if issue.UpdatedAt.IsZero() { + issue.UpdatedAt = now + } + + // Defensive fix for closed_at invariant (GH#523): older versions of bd could + // close issues without setting closed_at. Fix by using max(created_at, updated_at) + 1s. + if issue.Status == types.StatusClosed && issue.ClosedAt == nil { + maxTime := issue.CreatedAt + if issue.UpdatedAt.After(maxTime) { + maxTime = issue.UpdatedAt + } + closedAt := maxTime.Add(time.Second) + issue.ClosedAt = &closedAt + } + + // Defensive fix for deleted_at invariant: tombstones must have deleted_at + if issue.Status == types.StatusTombstone && issue.DeletedAt == nil { + maxTime := issue.CreatedAt + if issue.UpdatedAt.After(maxTime) { + maxTime = issue.UpdatedAt + } + deletedAt := maxTime.Add(time.Second) + issue.DeletedAt = &deletedAt + } + if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil { return fmt.Errorf("validation failed for issue: %w", err) } - issue.CreatedAt = now - issue.UpdatedAt = now if issue.ContentHash == "" { issue.ContentHash = issue.ComputeContentHash() }