Fix bd-206: Handle status transitions and closed_at constraint
- Updated manageClosedAt to handle both string and types.Status type assertions - Added equalTime function for comparing timestamps in import change detection - Added tests for open→closed and closed→open transitions - Added comment clarifying closed_at is managed automatically The bug occurred when UpdateIssue received types.Status instead of string, causing manageClosedAt to skip setting closed_at when status changed to closed. Amp-Thread-ID: https://ampcode.com/threads/T-ee774f6d-3b90-4311-976d-60c8dd8fe677 Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
@@ -117,6 +117,7 @@
|
||||
{"id":"bd-203","title":"Update LINTING.md with current baseline","description":"After cleanup, document the remaining acceptable baseline in LINTING.md so we can track regression.","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-27T18:53:10.38679-07:00","updated_at":"2025-10-27T18:53:10.38679-07:00"}
|
||||
{"id":"bd-204","title":"Update LINTING.md with current baseline","description":"After cleanup, document the remaining acceptable baseline in LINTING.md so we can track regression.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-27T18:53:14.531787-07:00","updated_at":"2025-10-27T18:53:14.531787-07:00","closed_at":"2025-10-27T18:37:08.880971-07:00"}
|
||||
{"id":"bd-205","title":"Update LINTING.md with current baseline","description":"After cleanup, document the remaining acceptable baseline in LINTING.md so we can track regression.","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-27T18:53:14.532297-07:00","updated_at":"2025-10-27T18:53:14.532297-07:00"}
|
||||
{"id":"bd-206","title":"Import fails when updating open issue to closed without setting closed_at","description":"When importing JSONL, if an issue exists in the database as \"open\" but the JSONL has it as \"closed\", the import tries to UPDATE status to \"closed\" without setting the closed_at timestamp. This violates the CHECK constraint: (status = 'closed') = (closed_at IS NOT NULL).\n\n**Steps to reproduce:**\n1. Have issue bd-X in database with status=\"open\", closed_at=NULL\n2. JSONL has same issue with status=\"closed\", closed_at=\"2025-10-27T...\"\n3. Run bd import -i .beads/beads.jsonl --resolve-collisions\n4. Error: \"constraint failed: CHECK constraint failed: (status = 'closed') = (closed_at IS NOT NULL) (275)\"\n\n**Expected behavior:**\nImport should update both status AND closed_at when transitioning to closed.\n\n**Actual behavior:**\nImport only updates status field, leaving closed_at=NULL, violating constraint.\n\n**Observed during:**\nSyncing two workspaces where collision resolution remapped bd-45. One workspace had it open, the other had it closed. Import tried to update open→closed but didn't copy closed_at from JSONL.\n\n**Impact:**\n- Prevents successful import when JSONL has closed issues that DB has as open\n- Blocks multi-workspace sync scenarios\n- Forces manual database rebuilds\n\n**Suggested fix:**\nIn import code, when updating an issue, if status changes to \"closed\", ensure closed_at is set from JSONL. Similarly, if status changes from \"closed\" to \"open\", ensure closed_at is cleared.","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-10-27T18:58:42.228957-07:00","updated_at":"2025-10-27T19:06:37.684793-07:00","closed_at":"2025-10-27T19:06:37.684793-07:00"}
|
||||
{"id":"bd-21","title":"Fix bd sync prefix mismatch error message suggesting non-existent flag","description":"GH #103: bd sync suggests using --rename-on-import flag that doesn't exist. Need to either implement the flag or fix the error message to suggest the correct workflow.","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-10-22T17:54:24.473508-07:00","updated_at":"2025-10-25T23:15:33.481941-07:00","closed_at":"2025-10-22T17:57:46.973029-07:00"}
|
||||
{"id":"bd-22","title":"Fix MCP close tool method signature error","description":"GH #107: MCP close() tool fails with \"BdDaemonClient.close() takes 1 positional argument but 2 were given\". Need to fix method signature in beads-mcp server.","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-10-22T19:17:05.429429-07:00","updated_at":"2025-10-25T23:15:33.482758-07:00","closed_at":"2025-10-22T19:19:54.601153-07:00"}
|
||||
{"id":"bd-23","title":"Update Claude Code marketplace plugin","description":"Update the beads plugin in the Claude Code marketplace to the latest version. This may help resolve some of the open GitHub issues related to marketplace installation and compatibility (#54, #112).\n\nShould include:\n- Latest beads version\n- Updated documentation\n- Any new features or bug fixes","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-22T22:29:11.293161-07:00","updated_at":"2025-10-25T23:15:33.483625-07:00","closed_at":"2025-10-23T22:27:37.671065-07:00"}
|
||||
|
||||
@@ -181,16 +181,19 @@ func upsertIssues(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, issues
|
||||
updates["acceptance_criteria"] = issue.AcceptanceCriteria
|
||||
updates["notes"] = issue.Notes
|
||||
|
||||
// bd-206: closed_at is managed automatically by UpdateIssue based on status
|
||||
// No need to set it explicitly here
|
||||
|
||||
if issue.Assignee != "" {
|
||||
updates["assignee"] = issue.Assignee
|
||||
updates["assignee"] = issue.Assignee
|
||||
} else {
|
||||
updates["assignee"] = nil
|
||||
updates["assignee"] = nil
|
||||
}
|
||||
|
||||
if issue.ExternalRef != nil && *issue.ExternalRef != "" {
|
||||
updates["external_ref"] = *issue.ExternalRef
|
||||
updates["external_ref"] = *issue.ExternalRef
|
||||
} else {
|
||||
updates["external_ref"] = nil
|
||||
updates["external_ref"] = nil
|
||||
}
|
||||
|
||||
// bd-88: Only update if data actually changed (prevents timestamp churn)
|
||||
|
||||
@@ -5,6 +5,7 @@ import (
|
||||
"fmt"
|
||||
"sort"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"github.com/steveyegge/beads/internal/importer"
|
||||
"github.com/steveyegge/beads/internal/storage"
|
||||
@@ -114,6 +115,29 @@ func (fc *fieldComparator) equalPriority(existing int, newVal interface{}) bool
|
||||
return existing == int(p)
|
||||
}
|
||||
|
||||
// equalTime compares *time.Time field
|
||||
func (fc *fieldComparator) equalTime(existing *time.Time, newVal interface{}) bool {
|
||||
switch t := newVal.(type) {
|
||||
case *time.Time:
|
||||
if existing == nil && t == nil {
|
||||
return true
|
||||
}
|
||||
if existing == nil || t == nil {
|
||||
return false
|
||||
}
|
||||
return existing.Equal(*t)
|
||||
case time.Time:
|
||||
if existing == nil {
|
||||
return false
|
||||
}
|
||||
return existing.Equal(t)
|
||||
case nil:
|
||||
return existing == nil
|
||||
default:
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
// checkFieldChanged checks if a specific field has changed
|
||||
func (fc *fieldComparator) checkFieldChanged(key string, existing *types.Issue, newVal interface{}) bool {
|
||||
switch key {
|
||||
@@ -137,6 +161,8 @@ func (fc *fieldComparator) checkFieldChanged(key string, existing *types.Issue,
|
||||
return !fc.equalStr(existing.Assignee, newVal)
|
||||
case "external_ref":
|
||||
return !fc.equalPtrStr(existing.ExternalRef, newVal)
|
||||
case "closed_at":
|
||||
return !fc.equalTime(existing.ClosedAt, newVal)
|
||||
default:
|
||||
// Unknown field - treat as changed to be conservative
|
||||
// This prevents skipping updates when new fields are added
|
||||
|
||||
@@ -1156,3 +1156,136 @@ func TestAutoImportClosedAtInvariant(t *testing.T) {
|
||||
t.Error("Expected closed_at to be set for closed issue")
|
||||
}
|
||||
}
|
||||
|
||||
// bd-206: Test updating open issue to closed preserves closed_at
|
||||
func TestImportOpenToClosedTransition(t *testing.T) {
|
||||
tmpDir, err := os.MkdirTemp("", "bd-test-open-to-closed-*")
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create temp dir: %v", err)
|
||||
}
|
||||
defer os.RemoveAll(tmpDir)
|
||||
|
||||
dbPath := filepath.Join(tmpDir, "test.db")
|
||||
|
||||
testStore, err := sqlite.New(dbPath)
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create storage: %v", err)
|
||||
}
|
||||
defer testStore.Close()
|
||||
|
||||
ctx := context.Background()
|
||||
|
||||
// Initialize database with prefix
|
||||
if err := testStore.SetConfig(ctx, "issue_prefix", "bd"); err != nil {
|
||||
t.Fatalf("Failed to set issue_prefix: %v", err)
|
||||
}
|
||||
|
||||
// Step 1: Create an open issue in the database
|
||||
openIssue := &types.Issue{
|
||||
ID: "bd-transition-1",
|
||||
Title: "Test transition",
|
||||
Description: "This will be closed",
|
||||
Status: types.StatusOpen,
|
||||
Priority: 1,
|
||||
IssueType: types.TypeBug,
|
||||
CreatedAt: time.Now(),
|
||||
UpdatedAt: time.Now(),
|
||||
ClosedAt: nil,
|
||||
}
|
||||
|
||||
err = testStore.CreateIssue(ctx, openIssue, "test")
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create open issue: %v", err)
|
||||
}
|
||||
|
||||
// Step 2: Update via UpdateIssue with closed status (closed_at managed automatically)
|
||||
updates := map[string]interface{}{
|
||||
"status": types.StatusClosed,
|
||||
}
|
||||
|
||||
err = testStore.UpdateIssue(ctx, "bd-transition-1", updates, "test")
|
||||
if err != nil {
|
||||
t.Fatalf("Update failed: %v", err)
|
||||
}
|
||||
|
||||
// Step 3: Verify the issue is now closed with correct closed_at
|
||||
updated, err := testStore.GetIssue(ctx, "bd-transition-1")
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to get updated issue: %v", err)
|
||||
}
|
||||
|
||||
if updated.Status != types.StatusClosed {
|
||||
t.Errorf("Expected status to be closed, got %s", updated.Status)
|
||||
}
|
||||
|
||||
if updated.ClosedAt == nil {
|
||||
t.Fatal("Expected closed_at to be set after transition to closed")
|
||||
}
|
||||
}
|
||||
|
||||
// bd-206: Test updating closed issue to open clears closed_at
|
||||
func TestImportClosedToOpenTransition(t *testing.T) {
|
||||
tmpDir, err := os.MkdirTemp("", "bd-test-closed-to-open-*")
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create temp dir: %v", err)
|
||||
}
|
||||
defer os.RemoveAll(tmpDir)
|
||||
|
||||
dbPath := filepath.Join(tmpDir, "test.db")
|
||||
|
||||
testStore, err := sqlite.New(dbPath)
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create storage: %v", err)
|
||||
}
|
||||
defer testStore.Close()
|
||||
|
||||
ctx := context.Background()
|
||||
|
||||
// Initialize database with prefix
|
||||
if err := testStore.SetConfig(ctx, "issue_prefix", "bd"); err != nil {
|
||||
t.Fatalf("Failed to set issue_prefix: %v", err)
|
||||
}
|
||||
|
||||
// Step 1: Create a closed issue in the database
|
||||
closedTime := time.Now()
|
||||
closedIssue := &types.Issue{
|
||||
ID: "bd-transition-2",
|
||||
Title: "Test reopening",
|
||||
Description: "This will be reopened",
|
||||
Status: types.StatusClosed,
|
||||
Priority: 1,
|
||||
IssueType: types.TypeBug,
|
||||
CreatedAt: time.Now(),
|
||||
UpdatedAt: closedTime,
|
||||
ClosedAt: &closedTime,
|
||||
}
|
||||
|
||||
err = testStore.CreateIssue(ctx, closedIssue, "test")
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to create closed issue: %v", err)
|
||||
}
|
||||
|
||||
// Step 2: Update via UpdateIssue with open status (closed_at managed automatically)
|
||||
updates := map[string]interface{}{
|
||||
"status": types.StatusOpen,
|
||||
}
|
||||
|
||||
err = testStore.UpdateIssue(ctx, "bd-transition-2", updates, "test")
|
||||
if err != nil {
|
||||
t.Fatalf("Update failed: %v", err)
|
||||
}
|
||||
|
||||
// Step 3: Verify the issue is now open with null closed_at
|
||||
updated, err := testStore.GetIssue(ctx, "bd-transition-2")
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to get updated issue: %v", err)
|
||||
}
|
||||
|
||||
if updated.Status != types.StatusOpen {
|
||||
t.Errorf("Expected status to be open, got %s", updated.Status)
|
||||
}
|
||||
|
||||
if updated.ClosedAt != nil {
|
||||
t.Errorf("Expected closed_at to be nil after reopening, got %v", updated.ClosedAt)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1170,8 +1170,14 @@ func manageClosedAt(oldIssue *types.Issue, updates map[string]interface{}, setCl
|
||||
return setClauses, args
|
||||
}
|
||||
|
||||
newStatus, ok := statusVal.(string)
|
||||
if !ok {
|
||||
// Handle both string and types.Status
|
||||
var newStatus string
|
||||
switch v := statusVal.(type) {
|
||||
case string:
|
||||
newStatus = v
|
||||
case types.Status:
|
||||
newStatus = string(v)
|
||||
default:
|
||||
return setClauses, args
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user