diff --git a/internal/storage/sqlite/batch_ops.go b/internal/storage/sqlite/batch_ops.go index 1d8df816..87baed0d 100644 --- a/internal/storage/sqlite/batch_ops.go +++ b/internal/storage/sqlite/batch_ops.go @@ -102,6 +102,30 @@ func bulkMarkDirty(ctx context.Context, conn *sql.Conn, issues []*types.Issue) e return markDirtyBatch(ctx, conn, issues) } +// updateChildCountersForHierarchicalIDs updates child_counters for all hierarchical IDs in the batch. +// This is called AFTER issues are inserted so that parents exist for the foreign key constraint. +// (GH#728 fix) +func updateChildCountersForHierarchicalIDs(ctx context.Context, conn *sql.Conn, issues []*types.Issue) error { + for _, issue := range issues { + if issue.ID == "" { + continue // Skip issues that were filtered out (e.g., OrphanSkip) + } + if parentID, childNum, ok := ParseHierarchicalID(issue.ID); ok { + // Only update if parent exists (it should after insert, but check to be safe) + var parentCount int + if err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, parentID).Scan(&parentCount); err != nil { + return fmt.Errorf("failed to check parent existence for %s: %w", parentID, err) + } + if parentCount > 0 { + if err := ensureChildCounterUpdatedWithConn(ctx, conn, parentID, childNum); err != nil { + return err + } + } + } + } + return nil +} + // checkForExistingIDs verifies that: // 1. There are no duplicate IDs within the batch itself // 2. None of the issue IDs already exist in the database @@ -271,6 +295,12 @@ func (s *SQLiteStorage) CreateIssuesWithFullOptions(ctx context.Context, issues return wrapDBError("bulk insert issues", err) } + // Phase 4.5: Update child counters for hierarchical IDs (GH#728 fix) + // This must happen AFTER insert so parents exist for the foreign key constraint + if err := updateChildCountersForHierarchicalIDs(ctx, conn, issues); err != nil { + return wrapDBError("update child counters", err) + } + // Phase 5: Record creation events if err := bulkRecordEvents(ctx, conn, issues, actor); err != nil { return wrapDBError("record creation events", err) diff --git a/internal/storage/sqlite/child_counters_test.go b/internal/storage/sqlite/child_counters_test.go index 1a25c357..5916ffbd 100644 --- a/internal/storage/sqlite/child_counters_test.go +++ b/internal/storage/sqlite/child_counters_test.go @@ -78,11 +78,11 @@ func TestGetNextChildNumber(t *testing.T) { func TestGetNextChildNumber_DifferentParents(t *testing.T) { store, cleanup := setupTestDB(t) defer cleanup() - + ctx := context.Background() parent1 := "bd-af78e9a2" - parent2 := "bd-af78e9a2.1" - + parent2 := "bd-bf89e0b3" // Use non-hierarchical ID to avoid counter interaction + // Create parent issues first for _, id := range []string{parent1, parent2} { parent := &types.Issue{ @@ -97,7 +97,7 @@ func TestGetNextChildNumber_DifferentParents(t *testing.T) { t.Fatalf("failed to create parent issue %s: %v", id, err) } } - + // Each parent should have independent counters child1_1, err := store.getNextChildNumber(ctx, parent1) if err != nil { @@ -106,7 +106,7 @@ func TestGetNextChildNumber_DifferentParents(t *testing.T) { if child1_1 != 1 { t.Errorf("expected parent1 child to be 1, got %d", child1_1) } - + child2_1, err := store.getNextChildNumber(ctx, parent2) if err != nil { t.Fatalf("getNextChildNumber failed: %v", err) @@ -114,7 +114,7 @@ func TestGetNextChildNumber_DifferentParents(t *testing.T) { if child2_1 != 1 { t.Errorf("expected parent2 child to be 1, got %d", child2_1) } - + child1_2, err := store.getNextChildNumber(ctx, parent1) if err != nil { t.Fatalf("getNextChildNumber failed: %v", err) @@ -186,10 +186,12 @@ func TestGetNextChildNumber_Concurrent(t *testing.T) { func TestGetNextChildNumber_NestedHierarchy(t *testing.T) { store, cleanup := setupTestDB(t) defer cleanup() - + ctx := context.Background() - + // Create parent issues for nested hierarchy + // Note: When creating bd-af78e9a2.1, the counter for bd-af78e9a2 is set to 1 (GH#728 fix) + // When creating bd-af78e9a2.1.2, the counter for bd-af78e9a2.1 is set to 2 (GH#728 fix) parents := []string{"bd-af78e9a2", "bd-af78e9a2.1", "bd-af78e9a2.1.2"} for _, id := range parents { parent := &types.Issue{ @@ -204,21 +206,21 @@ func TestGetNextChildNumber_NestedHierarchy(t *testing.T) { t.Fatalf("failed to create parent issue %s: %v", id, err) } } - - // Create nested hierarchy counters - // bd-af78e9a2 → .1, .2 - // bd-af78e9a2.1 → .1.1, .1.2 - // bd-af78e9a2.1.2 → .1.2.1, .1.2.2 - + + // With GH#728 fix, counters are updated when explicit hierarchical IDs are created: + // - Creating bd-af78e9a2.1 sets counter for bd-af78e9a2 to 1 + // - Creating bd-af78e9a2.1.2 sets counter for bd-af78e9a2.1 to 2 + // So getNextChildNumber returns the NEXT number after the existing children + tests := []struct { parent string expected []int }{ - {"bd-af78e9a2", []int{1, 2}}, - {"bd-af78e9a2.1", []int{1, 2}}, - {"bd-af78e9a2.1.2", []int{1, 2}}, + {"bd-af78e9a2", []int{2, 3}}, // counter was 1 after creating .1 + {"bd-af78e9a2.1", []int{3, 4}}, // counter was 2 after creating .1.2 + {"bd-af78e9a2.1.2", []int{1, 2}}, // no children created, starts at 1 } - + for _, tt := range tests { for _, want := range tt.expected { got, err := store.getNextChildNumber(ctx, tt.parent) diff --git a/internal/storage/sqlite/child_id_test.go b/internal/storage/sqlite/child_id_test.go index e2c9ffb5..b8335099 100644 --- a/internal/storage/sqlite/child_id_test.go +++ b/internal/storage/sqlite/child_id_test.go @@ -179,6 +179,80 @@ func TestCreateIssue_HierarchicalID(t *testing.T) { } } +// TestExplicitChildIDUpdatesCounter verifies that creating issues with explicit +// hierarchical IDs (e.g., bd-test.1, bd-test.2) updates the child counter so that +// GetNextChildID returns the correct next ID (GH#728 fix) +func TestExplicitChildIDUpdatesCounter(t *testing.T) { + tmpFile := t.TempDir() + "/test.db" + defer os.Remove(tmpFile) + store := newTestStore(t, tmpFile) + defer store.Close() + ctx := context.Background() + + // Create parent + parent := &types.Issue{ + ID: "bd-test", + Title: "Parent", + Description: "Test parent", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeEpic, + } + if err := store.CreateIssue(ctx, parent, "test"); err != nil { + t.Fatalf("failed to create parent: %v", err) + } + + // Create explicit child .1 + child1 := &types.Issue{ + ID: "bd-test.1", + Title: "Existing child 1", + Description: "Created with explicit ID", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, child1, "test"); err != nil { + t.Fatalf("failed to create child1: %v", err) + } + + // Create explicit child .2 + child2 := &types.Issue{ + ID: "bd-test.2", + Title: "Existing child 2", + Description: "Created with explicit ID", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, child2, "test"); err != nil { + t.Fatalf("failed to create child2: %v", err) + } + + // Now use GetNextChildID - should return .3 (not .1 which would collide) + nextID, err := store.GetNextChildID(ctx, "bd-test") + if err != nil { + t.Fatalf("GetNextChildID failed: %v", err) + } + + expected := "bd-test.3" + if nextID != expected { + t.Errorf("GetNextChildID returned %s, expected %s (GH#728 - counter should be updated when explicit child IDs are created)", nextID, expected) + } + + // Verify we can create an issue with the returned ID without collision + child3 := &types.Issue{ + ID: nextID, + Title: "New child via --parent", + Description: "Created with GetNextChildID", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, child3, "test"); err != nil { + t.Fatalf("failed to create child3 with ID %s: %v", nextID, err) + } +} + func TestCreateIssue_HierarchicalID_ParentNotExists(t *testing.T) { tmpFile := t.TempDir() + "/test.db" defer os.Remove(tmpFile) diff --git a/internal/storage/sqlite/hash_ids.go b/internal/storage/sqlite/hash_ids.go index 90db2dc6..e5890c63 100644 --- a/internal/storage/sqlite/hash_ids.go +++ b/internal/storage/sqlite/hash_ids.go @@ -2,6 +2,7 @@ package sqlite import ( "context" + "database/sql" "fmt" "strings" ) @@ -65,4 +66,36 @@ func (s *SQLiteStorage) GetNextChildID(ctx context.Context, parentID string) (st return childID, nil } +// ensureChildCounterUpdated ensures the child_counters table has a value for parentID +// that is at least childNum. This prevents ID collisions when children are created +// with explicit IDs (via --id flag or import) rather than GetNextChildID. +// (GH#728 fix) +func (s *SQLiteStorage) ensureChildCounterUpdated(ctx context.Context, parentID string, childNum int) error { + _, err := s.db.ExecContext(ctx, ` + INSERT INTO child_counters (parent_id, last_child) + VALUES (?, ?) + ON CONFLICT(parent_id) DO UPDATE SET + last_child = MAX(last_child, excluded.last_child) + `, parentID, childNum) + if err != nil { + return fmt.Errorf("failed to update child counter for parent %s: %w", parentID, err) + } + return nil +} + +// ensureChildCounterUpdatedWithConn is like ensureChildCounterUpdated but uses a specific +// connection for transaction consistency. (GH#728 fix) +func ensureChildCounterUpdatedWithConn(ctx context.Context, conn *sql.Conn, parentID string, childNum int) error { + _, err := conn.ExecContext(ctx, ` + INSERT INTO child_counters (parent_id, last_child) + VALUES (?, ?) + ON CONFLICT(parent_id) DO UPDATE SET + last_child = MAX(last_child, excluded.last_child) + `, parentID, childNum) + if err != nil { + return fmt.Errorf("failed to update child counter for parent %s: %w", parentID, err) + } + return nil +} + // generateHashID moved to ids.go (bd-0702) diff --git a/internal/storage/sqlite/ids.go b/internal/storage/sqlite/ids.go index 24da0619..e54b7f65 100644 --- a/internal/storage/sqlite/ids.go +++ b/internal/storage/sqlite/ids.go @@ -63,6 +63,33 @@ func IsHierarchicalID(id string) (isHierarchical bool, parentID string) { return true, id[:lastDot] } +// ParseHierarchicalID extracts the parent ID and child number from a hierarchical ID. +// Returns (parentID, childNum, true) for hierarchical IDs like "bd-abc.1" -> ("bd-abc", 1, true). +// Returns ("", 0, false) for non-hierarchical IDs. +// (GH#728 fix) +func ParseHierarchicalID(id string) (parentID string, childNum int, ok bool) { + lastDot := strings.LastIndex(id, ".") + if lastDot == -1 { + return "", 0, false + } + + suffix := id[lastDot+1:] + if len(suffix) == 0 { + return "", 0, false + } + + // Parse the numeric suffix + num := 0 + for _, c := range suffix { + if c < '0' || c > '9' { + return "", 0, false + } + num = num*10 + int(c-'0') + } + + return id[:lastDot], num, true +} + // ValidateIssueIDPrefix validates that an issue ID matches the configured prefix // Supports both top-level (bd-a3f8e9) and hierarchical (bd-a3f8e9.1) IDs func ValidateIssueIDPrefix(id, prefix string) error { @@ -235,6 +262,18 @@ func EnsureIDs(ctx context.Context, conn *sql.Conn, prefix string, issues []*typ // Default to allow for backward compatibility } } + + // Update child_counters to prevent future ID collisions (GH#728 fix) + // When explicit child IDs are imported, the counter must be at least the child number + // Only update if parent exists (parentCount > 0) - for orphan modes, skip this + // The counter will be updated when the parent is actually created/exists + if parentCount > 0 { + if _, childNum, ok := ParseHierarchicalID(issues[i].ID); ok { + if err := ensureChildCounterUpdatedWithConn(ctx, conn, parentID, childNum); err != nil { + return fmt.Errorf("failed to update child counter: %w", err) + } + } + } } usedIDs[issues[i].ID] = true diff --git a/internal/storage/sqlite/queries.go b/internal/storage/sqlite/queries.go index 6ab807f0..a0e251ce 100644 --- a/internal/storage/sqlite/queries.go +++ b/internal/storage/sqlite/queries.go @@ -190,6 +190,14 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act // Parent(s) not found in JSONL history - cannot proceed return fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID) } + + // Update child_counters to prevent future ID collisions (GH#728 fix) + // When explicit child IDs are used, the counter must be at least the child number + if _, childNum, ok := ParseHierarchicalID(issue.ID); ok { + if err := ensureChildCounterUpdatedWithConn(ctx, conn, parentID, childNum); err != nil { + return fmt.Errorf("failed to update child counter: %w", err) + } + } } }