fix(import): defensive handling for closed issues missing closed_at timestamp
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 <noreply@anthropic.com>
This commit is contained in:
@@ -32,6 +32,27 @@ func validateBatchIssuesWithCustomStatuses(issues []*types.Issue, customStatuses
|
|||||||
issue.UpdatedAt = now
|
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 {
|
if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil {
|
||||||
return fmt.Errorf("validation failed for issue %d: %w", i, err)
|
return fmt.Errorf("validation failed for issue %d: %w", i, err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|||||||
@@ -227,6 +227,27 @@ func (s *SQLiteStorage) importJSONLFile(ctx context.Context, jsonlPath, sourceRe
|
|||||||
// upsertIssueInTx inserts or updates an issue within a transaction.
|
// upsertIssueInTx inserts or updates an issue within a transaction.
|
||||||
// Uses INSERT OR REPLACE to handle both new and existing issues.
|
// 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 {
|
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)
|
// Validate issue (with custom status support, bd-1pj6)
|
||||||
if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil {
|
if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil {
|
||||||
return fmt.Errorf("validation failed: %w", err)
|
return fmt.Errorf("validation failed: %w", err)
|
||||||
|
|||||||
@@ -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)
|
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)
|
// Validate issue before creating (with custom status support)
|
||||||
if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil {
|
if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil {
|
||||||
return fmt.Errorf("validation failed: %w", err)
|
return fmt.Errorf("validation failed: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Set timestamps
|
|
||||||
now := time.Now()
|
|
||||||
issue.CreatedAt = now
|
|
||||||
issue.UpdatedAt = now
|
|
||||||
|
|
||||||
// Compute content hash (bd-95)
|
// Compute content hash (bd-95)
|
||||||
if issue.ContentHash == "" {
|
if issue.ContentHash == "" {
|
||||||
issue.ContentHash = issue.ComputeContentHash()
|
issue.ContentHash = issue.ComputeContentHash()
|
||||||
|
|||||||
@@ -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)
|
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)
|
// Validate issue before creating (with custom status support)
|
||||||
if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil {
|
if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil {
|
||||||
return fmt.Errorf("validation failed: %w", err)
|
return fmt.Errorf("validation failed: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Set timestamps
|
|
||||||
now := time.Now()
|
|
||||||
issue.CreatedAt = now
|
|
||||||
issue.UpdatedAt = now
|
|
||||||
|
|
||||||
// Compute content hash (bd-95)
|
// Compute content hash (bd-95)
|
||||||
if issue.ContentHash == "" {
|
if issue.ContentHash == "" {
|
||||||
issue.ContentHash = issue.ComputeContentHash()
|
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)
|
// Validate and prepare all issues first (with custom status support)
|
||||||
now := time.Now()
|
now := time.Now()
|
||||||
for _, issue := range issues {
|
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 {
|
if err := issue.ValidateWithCustomStatuses(customStatuses); err != nil {
|
||||||
return fmt.Errorf("validation failed for issue: %w", err)
|
return fmt.Errorf("validation failed for issue: %w", err)
|
||||||
}
|
}
|
||||||
issue.CreatedAt = now
|
|
||||||
issue.UpdatedAt = now
|
|
||||||
if issue.ContentHash == "" {
|
if issue.ContentHash == "" {
|
||||||
issue.ContentHash = issue.ComputeContentHash()
|
issue.ContentHash = issue.ComputeContentHash()
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user