From 2651620a4c0c76c6b5b8bc465a0c5875c3980484 Mon Sep 17 00:00:00 2001 From: cbro Date: Sun, 14 Dec 2025 17:18:01 -0500 Subject: [PATCH] fix(storage): persist close_reason to issues table on close (#551) CloseIssue was storing the reason only in the events table, not in the issues.close_reason column. This caused `bd show --json` to return an empty close_reason even when one was provided. - Update CloseIssue in queries.go and transaction.go to set close_reason - Clear close_reason when reopening issues (in manageClosedAt) - Add tests for close_reason in storage and CLI JSON output - Document the dual-storage of close_reason (issues + events tables) --- cmd/bd/cli_fast_test.go | 3 +++ internal/storage/sqlite/queries.go | 13 ++++++++++--- internal/storage/sqlite/sqlite_test.go | 14 ++++++++++++-- internal/storage/sqlite/transaction.go | 5 +++-- internal/storage/sqlite/transaction_test.go | 3 +++ 5 files changed, 31 insertions(+), 7 deletions(-) diff --git a/cmd/bd/cli_fast_test.go b/cmd/bd/cli_fast_test.go index 0de98dd5..623a7c93 100644 --- a/cmd/bd/cli_fast_test.go +++ b/cmd/bd/cli_fast_test.go @@ -333,6 +333,9 @@ func TestCLI_Close(t *testing.T) { if closed[0]["status"] != "closed" { t.Errorf("Expected status 'closed', got: %v", closed[0]["status"]) } + if closed[0]["close_reason"] != "Done" { + t.Errorf("Expected close_reason 'Done', got: %v", closed[0]["close_reason"]) + } } func TestCLI_DepAdd(t *testing.T) { diff --git a/internal/storage/sqlite/queries.go b/internal/storage/sqlite/queries.go index 5417ac27..37e6330e 100644 --- a/internal/storage/sqlite/queries.go +++ b/internal/storage/sqlite/queries.go @@ -500,10 +500,13 @@ func manageClosedAt(oldIssue *types.Issue, updates map[string]interface{}, setCl setClauses = append(setClauses, "closed_at = ?") args = append(args, now) } else if oldIssue.Status == types.StatusClosed { - // Changing from closed to something else: clear closed_at + // Changing from closed to something else: clear closed_at and close_reason updates["closed_at"] = nil setClauses = append(setClauses, "closed_at = ?") args = append(args, nil) + updates["close_reason"] = "" + setClauses = append(setClauses, "close_reason = ?") + args = append(args, "") } return setClauses, args @@ -806,10 +809,14 @@ func (s *SQLiteStorage) CloseIssue(ctx context.Context, id string, reason string } defer func() { _ = tx.Rollback() }() + // NOTE: close_reason is stored in two places: + // 1. issues.close_reason - for direct queries (bd show --json, exports) + // 2. events.comment - for audit history (when was it closed, by whom) + // Keep both in sync. If refactoring, consider deriving one from the other. result, err := tx.ExecContext(ctx, ` - UPDATE issues SET status = ?, closed_at = ?, updated_at = ? + UPDATE issues SET status = ?, closed_at = ?, updated_at = ?, close_reason = ? WHERE id = ? - `, types.StatusClosed, now, now, id) + `, types.StatusClosed, now, now, reason, id) if err != nil { return fmt.Errorf("failed to close issue: %w", err) } diff --git a/internal/storage/sqlite/sqlite_test.go b/internal/storage/sqlite/sqlite_test.go index ffa179db..ca4e702e 100644 --- a/internal/storage/sqlite/sqlite_test.go +++ b/internal/storage/sqlite/sqlite_test.go @@ -706,6 +706,10 @@ func TestCloseIssue(t *testing.T) { if closed.ClosedAt == nil { t.Error("ClosedAt should be set") } + + if closed.CloseReason != "Done" { + t.Errorf("CloseReason not set: got %q, want %q", closed.CloseReason, "Done") + } } func TestClosedAtInvariant(t *testing.T) { @@ -766,7 +770,7 @@ func TestClosedAtInvariant(t *testing.T) { t.Fatalf("CloseIssue failed: %v", err) } - // Verify it's closed with closed_at set + // Verify it's closed with closed_at and close_reason set closed, err := store.GetIssue(ctx, issue.ID) if err != nil { t.Fatalf("GetIssue failed: %v", err) @@ -774,6 +778,9 @@ func TestClosedAtInvariant(t *testing.T) { if closed.ClosedAt == nil { t.Fatal("ClosedAt should be set after closing") } + if closed.CloseReason != "Done" { + t.Errorf("CloseReason should be 'Done', got %q", closed.CloseReason) + } // Reopen the issue updates := map[string]interface{}{ @@ -784,7 +791,7 @@ func TestClosedAtInvariant(t *testing.T) { t.Fatalf("UpdateIssue failed: %v", err) } - // Verify closed_at was cleared + // Verify closed_at and close_reason were cleared reopened, err := store.GetIssue(ctx, issue.ID) if err != nil { t.Fatalf("GetIssue failed: %v", err) @@ -795,6 +802,9 @@ func TestClosedAtInvariant(t *testing.T) { if reopened.ClosedAt != nil { t.Error("ClosedAt should be cleared when reopening issue") } + if reopened.CloseReason != "" { + t.Errorf("CloseReason should be cleared when reopening issue, got %q", reopened.CloseReason) + } }) t.Run("CreateIssue rejects closed issue without closed_at", func(t *testing.T) { diff --git a/internal/storage/sqlite/transaction.go b/internal/storage/sqlite/transaction.go index 96b89f10..b143fbde 100644 --- a/internal/storage/sqlite/transaction.go +++ b/internal/storage/sqlite/transaction.go @@ -462,13 +462,14 @@ func applyUpdatesToIssue(issue *types.Issue, updates map[string]interface{}) { } // CloseIssue closes an issue within the transaction. +// NOTE: close_reason is stored in both issues table and events table - see SQLiteStorage.CloseIssue. func (t *sqliteTxStorage) CloseIssue(ctx context.Context, id string, reason string, actor string) error { now := time.Now() result, err := t.conn.ExecContext(ctx, ` - UPDATE issues SET status = ?, closed_at = ?, updated_at = ? + UPDATE issues SET status = ?, closed_at = ?, updated_at = ?, close_reason = ? WHERE id = ? - `, types.StatusClosed, now, now, id) + `, types.StatusClosed, now, now, reason, id) if err != nil { return fmt.Errorf("failed to close issue: %w", err) } diff --git a/internal/storage/sqlite/transaction_test.go b/internal/storage/sqlite/transaction_test.go index a4240075..ba81e5a4 100644 --- a/internal/storage/sqlite/transaction_test.go +++ b/internal/storage/sqlite/transaction_test.go @@ -268,6 +268,9 @@ func TestTransactionCloseIssue(t *testing.T) { if closed.Status != types.StatusClosed { t.Errorf("expected status 'closed', got %q", closed.Status) } + if closed.CloseReason != "Done" { + t.Errorf("expected close_reason 'Done', got %q", closed.CloseReason) + } } // TestTransactionDeleteIssue tests deleting an issue within a transaction.