From 0d31e4209d852a365012bdb156c65f94a45f80c7 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Tue, 23 Dec 2025 13:47:40 -0800 Subject: [PATCH] fix: preserve pinned field during JSONL import (bd-phtv) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed two code paths where pinned=false from JSONL would overwrite existing pinned=true in database: - importer.go: Only update pinned if explicitly true in JSONL - multirepo.go: Use COALESCE to preserve existing pinned value Added tests for pinned field preservation. Note: Bug may have additional code path - investigation ongoing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- internal/importer/importer.go | 14 ++- internal/importer/importer_test.go | 146 ++++++++++++++++++++++++++- internal/storage/sqlite/multirepo.go | 5 +- 3 files changed, 159 insertions(+), 6 deletions(-) diff --git a/internal/importer/importer.go b/internal/importer/importer.go index 47ecb8f0..a19410f2 100644 --- a/internal/importer/importer.go +++ b/internal/importer/importer.go @@ -567,8 +567,11 @@ func upsertIssues(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, issues updates["acceptance_criteria"] = incoming.AcceptanceCriteria updates["notes"] = incoming.Notes updates["closed_at"] = incoming.ClosedAt - // Pinned field (bd-7h5) - updates["pinned"] = incoming.Pinned + // Pinned field (bd-phtv): Only update if explicitly true in JSONL + // (omitempty means false values are absent, so false = don't change existing) + if incoming.Pinned { + updates["pinned"] = incoming.Pinned + } if incoming.Assignee != "" { updates["assignee"] = incoming.Assignee @@ -662,8 +665,11 @@ func upsertIssues(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, issues updates["acceptance_criteria"] = incoming.AcceptanceCriteria updates["notes"] = incoming.Notes updates["closed_at"] = incoming.ClosedAt - // Pinned field (bd-7h5) - updates["pinned"] = incoming.Pinned + // Pinned field (bd-phtv): Only update if explicitly true in JSONL + // (omitempty means false values are absent, so false = don't change existing) + if incoming.Pinned { + updates["pinned"] = incoming.Pinned + } if incoming.Assignee != "" { updates["assignee"] = incoming.Assignee diff --git a/internal/importer/importer_test.go b/internal/importer/importer_test.go index e11634b0..6ad7e7f0 100644 --- a/internal/importer/importer_test.go +++ b/internal/importer/importer_test.go @@ -1479,7 +1479,151 @@ func TestImportMixedPrefixMismatch(t *testing.T) { } } -// TestMultiRepoPrefixValidation tests GH#686: multi-repo allows foreign prefixes. +// TestImportPreservesPinnedField tests that importing from JSONL (which has omitempty +// for the pinned field) does NOT reset an existing pinned=true issue to pinned=false. +// +// Bug scenario (bd-phtv): +// 1. User runs `bd pin ` which sets pinned=true in SQLite +// 2. Any subsequent bd command (e.g., `bd show`) triggers auto-import from JSONL +// 3. JSONL has pinned=false due to omitempty (field absent means false in Go) +// 4. Import overwrites pinned=true with pinned=false, losing the pinned state +// +// Expected: Import should preserve existing pinned=true when incoming pinned=false +// (since false just means "field was absent in JSONL due to omitempty"). +func TestImportPreservesPinnedField(t *testing.T) { + ctx := context.Background() + + tmpDB := t.TempDir() + "/test.db" + store, err := sqlite.New(context.Background(), tmpDB) + if err != nil { + t.Fatalf("Failed to create store: %v", err) + } + defer store.Close() + + if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil { + t.Fatalf("Failed to set prefix: %v", err) + } + + // Create an issue with pinned=true (simulates `bd pin` command) + pinnedIssue := &types.Issue{ + ID: "test-abc123", + Title: "Pinned Issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + Pinned: true, // This is set by `bd pin` + CreatedAt: time.Now().Add(-time.Hour), + UpdatedAt: time.Now().Add(-time.Hour), + } + pinnedIssue.ContentHash = pinnedIssue.ComputeContentHash() + if err := store.CreateIssue(ctx, pinnedIssue, "test-setup"); err != nil { + t.Fatalf("Failed to create pinned issue: %v", err) + } + + // Verify issue is pinned before import + before, err := store.GetIssue(ctx, "test-abc123") + if err != nil { + t.Fatalf("Failed to get issue before import: %v", err) + } + if !before.Pinned { + t.Fatal("Issue should be pinned before import") + } + + // Import same issue from JSONL (simulates auto-import after git pull) + // JSONL has pinned=false because omitempty means absent fields are false + importedIssue := &types.Issue{ + ID: "test-abc123", + Title: "Pinned Issue", // Same content + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + Pinned: false, // This is what JSONL deserialization produces due to omitempty + CreatedAt: time.Now().Add(-time.Hour), + UpdatedAt: time.Now(), // Newer timestamp to trigger update + } + importedIssue.ContentHash = importedIssue.ComputeContentHash() + + result, err := ImportIssues(ctx, tmpDB, store, []*types.Issue{importedIssue}, Options{}) + if err != nil { + t.Fatalf("Import failed: %v", err) + } + + // Import should recognize this as an update (same ID, different timestamp) + // The unchanged count may vary based on whether other fields changed + t.Logf("Import result: Created=%d Updated=%d Unchanged=%d", result.Created, result.Updated, result.Unchanged) + + // CRITICAL: Verify pinned field was preserved + after, err := store.GetIssue(ctx, "test-abc123") + if err != nil { + t.Fatalf("Failed to get issue after import: %v", err) + } + if !after.Pinned { + t.Error("FAIL (bd-phtv): pinned=true was reset to false by import. " + + "Import should preserve existing pinned field when incoming is false (omitempty).") + } +} + +// TestImportSetsPinnedTrue tests that importing an issue with pinned=true +// correctly sets the pinned field in the database. +func TestImportSetsPinnedTrue(t *testing.T) { + ctx := context.Background() + + tmpDB := t.TempDir() + "/test.db" + store, err := sqlite.New(context.Background(), tmpDB) + if err != nil { + t.Fatalf("Failed to create store: %v", err) + } + defer store.Close() + + if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil { + t.Fatalf("Failed to set prefix: %v", err) + } + + // Create an unpinned issue + unpinnedIssue := &types.Issue{ + ID: "test-abc123", + Title: "Unpinned Issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + Pinned: false, + CreatedAt: time.Now().Add(-time.Hour), + UpdatedAt: time.Now().Add(-time.Hour), + } + unpinnedIssue.ContentHash = unpinnedIssue.ComputeContentHash() + if err := store.CreateIssue(ctx, unpinnedIssue, "test-setup"); err != nil { + t.Fatalf("Failed to create issue: %v", err) + } + + // Import with pinned=true (from JSONL that explicitly has "pinned": true) + importedIssue := &types.Issue{ + ID: "test-abc123", + Title: "Unpinned Issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + Pinned: true, // Explicitly set to true in JSONL + CreatedAt: time.Now().Add(-time.Hour), + UpdatedAt: time.Now(), // Newer timestamp + } + importedIssue.ContentHash = importedIssue.ComputeContentHash() + + result, err := ImportIssues(ctx, tmpDB, store, []*types.Issue{importedIssue}, Options{}) + if err != nil { + t.Fatalf("Import failed: %v", err) + } + t.Logf("Import result: Created=%d Updated=%d Unchanged=%d", result.Created, result.Updated, result.Unchanged) + + // Verify pinned field was set to true + after, err := store.GetIssue(ctx, "test-abc123") + if err != nil { + t.Fatalf("Failed to get issue after import: %v", err) + } + if !after.Pinned { + t.Error("FAIL: pinned=true from JSONL should set the field to true in database") + } +} + func TestMultiRepoPrefixValidation(t *testing.T) { if err := config.Initialize(); err != nil { t.Fatalf("Failed to initialize config: %v", err) diff --git a/internal/storage/sqlite/multirepo.go b/internal/storage/sqlite/multirepo.go index d8826bdc..74f4890b 100644 --- a/internal/storage/sqlite/multirepo.go +++ b/internal/storage/sqlite/multirepo.go @@ -330,6 +330,9 @@ func (s *SQLiteStorage) upsertIssueInTx(ctx context.Context, tx *sql.Tx, issue * } if existingHash != issue.ContentHash { + // Pinned field fix (bd-phtv): Use COALESCE(NULLIF(?, 0), pinned) to preserve + // existing pinned=1 when incoming pinned=0 (which means field was absent in + // JSONL due to omitempty). This prevents auto-import from resetting pinned issues. _, err = tx.ExecContext(ctx, ` UPDATE issues SET content_hash = ?, title = ?, description = ?, design = ?, @@ -337,7 +340,7 @@ func (s *SQLiteStorage) upsertIssueInTx(ctx context.Context, tx *sql.Tx, issue * issue_type = ?, assignee = ?, estimated_minutes = ?, updated_at = ?, closed_at = ?, external_ref = ?, source_repo = ?, deleted_at = ?, deleted_by = ?, delete_reason = ?, original_type = ?, - sender = ?, ephemeral = ?, pinned = ?, is_template = ?, + sender = ?, ephemeral = ?, pinned = COALESCE(NULLIF(?, 0), pinned), is_template = ?, await_type = ?, await_id = ?, timeout_ns = ?, waiters = ? WHERE id = ? `,