diff --git a/internal/storage/sqlite/ids.go b/internal/storage/sqlite/ids.go index 97644aea..774c6a96 100644 --- a/internal/storage/sqlite/ids.go +++ b/internal/storage/sqlite/ids.go @@ -73,6 +73,38 @@ func isValidHex(s string) bool { return true } +// IsHierarchicalID checks if an issue ID is hierarchical (has a parent). +// Hierarchical IDs have the format {parentID}.{N} where N is a numeric child suffix. +// Returns true and the parent ID if hierarchical, false and empty string otherwise. +// +// This correctly handles prefixes that contain dots (e.g., "my.project-abc123" +// is NOT hierarchical, but "my.project-abc123.1" IS hierarchical with parent +// "my.project-abc123"). +// +// The key insight is that hierarchical IDs always end with .{digits} where +// the digits represent the child number (1, 2, 3, etc.). +func IsHierarchicalID(id string) (isHierarchical bool, parentID string) { + lastDot := strings.LastIndex(id, ".") + if lastDot == -1 { + return false, "" + } + + // Check if the suffix after the last dot is purely numeric + suffix := id[lastDot+1:] + if len(suffix) == 0 { + return false, "" + } + + for _, c := range suffix { + if c < '0' || c > '9' { + return false, "" + } + } + + // It's hierarchical - parent is everything before the last dot + return true, id[:lastDot] +} + // 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 { @@ -203,11 +235,8 @@ func EnsureIDs(ctx context.Context, conn *sql.Conn, prefix string, issues []*typ } // For hierarchical IDs (bd-a3f8e9.1), ensure parent exists - if strings.Contains(issues[i].ID, ".") { - // Extract parent ID (everything before the last dot) - lastDot := strings.LastIndex(issues[i].ID, ".") - parentID := issues[i].ID[:lastDot] - + // Use IsHierarchicalID to correctly handle prefixes with dots (GH#508) + if isHierarchical, parentID := IsHierarchicalID(issues[i].ID); isHierarchical { var parentCount int err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, parentID).Scan(&parentCount) if err != nil { diff --git a/internal/storage/sqlite/ids_test.go b/internal/storage/sqlite/ids_test.go new file mode 100644 index 00000000..2b954b99 --- /dev/null +++ b/internal/storage/sqlite/ids_test.go @@ -0,0 +1,134 @@ +package sqlite + +import "testing" + +// TestIsHierarchicalID tests the IsHierarchicalID function which detects +// if an issue ID is hierarchical (has a parent) based on the .N suffix pattern. +// This test covers the fix for GH#508 where prefixes with dots were incorrectly +// flagged as hierarchical. +func TestIsHierarchicalID(t *testing.T) { + tests := []struct { + name string + id string + wantHierarchical bool + wantParentID string + }{ + // Standard hierarchical IDs + { + name: "simple child .1", + id: "bd-abc123.1", + wantHierarchical: true, + wantParentID: "bd-abc123", + }, + { + name: "child .2", + id: "bd-xyz789.2", + wantHierarchical: true, + wantParentID: "bd-xyz789", + }, + { + name: "multi-digit child .10", + id: "bd-test.10", + wantHierarchical: true, + wantParentID: "bd-test", + }, + { + name: "large child number .999", + id: "bd-issue.999", + wantHierarchical: true, + wantParentID: "bd-issue", + }, + { + name: "nested hierarchical", + id: "bd-parent.1.2", + wantHierarchical: true, + wantParentID: "bd-parent.1", + }, + + // Non-hierarchical IDs (no suffix or non-numeric suffix) + { + name: "simple top-level", + id: "bd-abc123", + wantHierarchical: false, + wantParentID: "", + }, + { + name: "no dot at all", + id: "test-issue", + wantHierarchical: false, + wantParentID: "", + }, + + // GH#508: Prefixes with dots should NOT be detected as hierarchical + { + name: "prefix with dot - my.project", + id: "my.project-abc123", + wantHierarchical: false, + wantParentID: "", + }, + { + name: "prefix with multiple dots", + id: "com.example.app-issue1", + wantHierarchical: false, + wantParentID: "", + }, + { + name: "prefix with dot AND hierarchical child", + id: "my.project-abc123.1", + wantHierarchical: true, + wantParentID: "my.project-abc123", + }, + { + name: "complex prefix with hierarchical", + id: "com.example.app-xyz.5", + wantHierarchical: true, + wantParentID: "com.example.app-xyz", + }, + + // Edge cases + { + name: "dot but non-numeric suffix", + id: "bd-abc.def", + wantHierarchical: false, + wantParentID: "", + }, + { + name: "mixed suffix (starts with digit)", + id: "bd-test.1abc", + wantHierarchical: false, + wantParentID: "", + }, + { + name: "trailing dot only", + id: "bd-test.", + wantHierarchical: false, + wantParentID: "", + }, + { + name: "empty after dot", + id: "bd-test.", + wantHierarchical: false, + wantParentID: "", + }, + { + name: "child 0", + id: "bd-parent.0", + wantHierarchical: true, + wantParentID: "bd-parent", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotHierarchical, gotParentID := IsHierarchicalID(tt.id) + if gotHierarchical != tt.wantHierarchical { + t.Errorf("IsHierarchicalID(%q) hierarchical = %v, want %v", + tt.id, gotHierarchical, tt.wantHierarchical) + } + if gotParentID != tt.wantParentID { + t.Errorf("IsHierarchicalID(%q) parentID = %q, want %q", + tt.id, gotParentID, tt.wantParentID) + } + }) + } +} diff --git a/internal/storage/sqlite/migrations/016_orphan_detection.go b/internal/storage/sqlite/migrations/016_orphan_detection.go index d047f4a8..9319fd7a 100644 --- a/internal/storage/sqlite/migrations/016_orphan_detection.go +++ b/internal/storage/sqlite/migrations/016_orphan_detection.go @@ -7,23 +7,36 @@ import ( ) // MigrateOrphanDetection detects orphaned child issues and logs them for user action -// Orphaned children are issues with hierarchical IDs (e.g., "parent.child") where the +// Orphaned children are issues with hierarchical IDs (e.g., "parent.1") where the // parent issue no longer exists in the database. // +// Hierarchical IDs have the format {parentID}.{N} where N is a numeric child suffix. +// This correctly handles prefixes that contain dots (e.g., "my.project-abc123" is NOT +// hierarchical, but "my.project-abc123.1" IS hierarchical). See GH#508. +// // This migration does NOT automatically delete or convert orphans - it only logs them // so the user can decide whether to: // - Delete the orphans if they're no longer needed // - Convert them to top-level issues by renaming them // - Restore the missing parent issues func MigrateOrphanDetection(db *sql.DB) error { - // Query for orphaned children using the pattern from the issue description: - // SELECT id FROM issues WHERE id LIKE '%.%' - // AND substr(id, 1, instr(id || '.', '.') - 1) NOT IN (SELECT id FROM issues) + // Query for orphaned children: + // - Must end with .N where N is 1-4 digits (covers child numbers 0-9999) + // - Parent (everything before the last .N) must not exist in issues table + // - Uses GLOB patterns to ensure suffix is purely numeric + // - rtrim removes trailing digits, then trailing dot, to get parent ID + // + // GH#508: The old query used instr() to find the first dot, which incorrectly + // flagged IDs with dots in the prefix (e.g., "my.project-abc") as orphans. + // The fix uses GLOB patterns to only match IDs ending with .{digits}. rows, err := db.Query(` SELECT id FROM issues - WHERE id LIKE '%.%' - AND substr(id, 1, instr(id || '.', '.') - 1) NOT IN (SELECT id FROM issues) + WHERE + -- Must end with .N where N is 1-4 digits (child number suffix) + (id GLOB '*.[0-9]' OR id GLOB '*.[0-9][0-9]' OR id GLOB '*.[0-9][0-9][0-9]' OR id GLOB '*.[0-9][0-9][0-9][0-9]') + -- Parent (remove trailing digits then dot) must not exist + AND rtrim(rtrim(id, '0123456789'), '.') NOT IN (SELECT id FROM issues) ORDER BY id `) if err != nil { diff --git a/internal/storage/sqlite/migrations_test.go b/internal/storage/sqlite/migrations_test.go index 2a621756..50be9801 100644 --- a/internal/storage/sqlite/migrations_test.go +++ b/internal/storage/sqlite/migrations_test.go @@ -620,4 +620,83 @@ func TestMigrateOrphanDetection(t *testing.T) { } } }) + + // GH#508: Verify that prefixes with dots don't trigger false positives + t.Run("prefix with dots is not flagged as orphan", func(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + db := store.db + + // Override the prefix for this test + _, err := db.Exec(`UPDATE config SET value = 'my.project' WHERE key = 'issue_prefix'`) + if err != nil { + t.Fatalf("failed to update prefix: %v", err) + } + + // Insert issues with dotted prefix directly (bypassing prefix validation) + testCases := []struct { + id string + expectOrphan bool + }{ + // These should NOT be flagged as orphans (dots in prefix) + {"my.project-abc123", false}, + {"my.project-xyz789", false}, + {"com.example.app-issue1", false}, + + // This SHOULD be flagged as orphan (hierarchical, parent doesn't exist) + {"my.project-missing.1", true}, + } + + for _, tc := range testCases { + _, err := db.Exec(` + INSERT INTO issues (id, title, status, priority, issue_type, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, datetime('now'), datetime('now')) + `, tc.id, "Test Issue", "open", 1, "task") + if err != nil { + t.Fatalf("failed to insert %s: %v", tc.id, err) + } + } + + // Query for orphans using the same logic as the migration + rows, err := db.Query(` + SELECT id + FROM issues + WHERE + (id GLOB '*.[0-9]' OR id GLOB '*.[0-9][0-9]' OR id GLOB '*.[0-9][0-9][0-9]' OR id GLOB '*.[0-9][0-9][0-9][0-9]') + AND rtrim(rtrim(id, '0123456789'), '.') NOT IN (SELECT id FROM issues) + ORDER BY id + `) + if err != nil { + t.Fatalf("query failed: %v", err) + } + defer rows.Close() + + var orphans []string + for rows.Next() { + var id string + if err := rows.Scan(&id); err != nil { + t.Fatalf("scan failed: %v", err) + } + orphans = append(orphans, id) + } + + // Verify only the expected orphan is detected + if len(orphans) != 1 { + t.Errorf("expected 1 orphan, got %d: %v", len(orphans), orphans) + } + if len(orphans) == 1 && orphans[0] != "my.project-missing.1" { + t.Errorf("expected orphan 'my.project-missing.1', got %q", orphans[0]) + } + + // Verify non-hierarchical dotted IDs are NOT flagged + for _, tc := range testCases { + if !tc.expectOrphan { + for _, orphan := range orphans { + if orphan == tc.id { + t.Errorf("false positive: %s was incorrectly flagged as orphan", tc.id) + } + } + } + } + }) } diff --git a/internal/storage/sqlite/queries.go b/internal/storage/sqlite/queries.go index 37e6330e..e8290b8e 100644 --- a/internal/storage/sqlite/queries.go +++ b/internal/storage/sqlite/queries.go @@ -122,7 +122,8 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act } // For hierarchical IDs (bd-a3f8e9.1), ensure parent exists - if strings.Contains(issue.ID, ".") { + // Use IsHierarchicalID to correctly handle prefixes with dots (GH#508) + if isHierarchical, parentID := IsHierarchicalID(issue.ID); isHierarchical { // Try to resurrect entire parent chain if any parents are missing // Use the conn-based version to participate in the same transaction resurrected, err := s.tryResurrectParentChainWithConn(ctx, conn, issue.ID) @@ -131,8 +132,6 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act } if !resurrected { // Parent(s) not found in JSONL history - cannot proceed - lastDot := strings.LastIndex(issue.ID, ".") - parentID := issue.ID[:lastDot] return fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID) } } diff --git a/internal/storage/sqlite/transaction.go b/internal/storage/sqlite/transaction.go index b143fbde..a8919074 100644 --- a/internal/storage/sqlite/transaction.go +++ b/internal/storage/sqlite/transaction.go @@ -137,7 +137,8 @@ func (t *sqliteTxStorage) CreateIssue(ctx context.Context, issue *types.Issue, a } // For hierarchical IDs (bd-a3f8e9.1), ensure parent exists - if strings.Contains(issue.ID, ".") { + // Use IsHierarchicalID to correctly handle prefixes with dots (GH#508) + if isHierarchical, parentID := IsHierarchicalID(issue.ID); isHierarchical { // Try to resurrect entire parent chain if any parents are missing resurrected, err := t.parent.tryResurrectParentChainWithConn(ctx, t.conn, issue.ID) if err != nil { @@ -145,8 +146,6 @@ func (t *sqliteTxStorage) CreateIssue(ctx context.Context, issue *types.Issue, a } if !resurrected { // Parent(s) not found in JSONL history - cannot proceed - lastDot := strings.LastIndex(issue.ID, ".") - parentID := issue.ID[:lastDot] return fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID) } }