diff --git a/internal/storage/sqlite/cycle_detection_test.go b/internal/storage/sqlite/cycle_detection_test.go index 478b04e0..049fcb8e 100644 --- a/internal/storage/sqlite/cycle_detection_test.go +++ b/internal/storage/sqlite/cycle_detection_test.go @@ -505,3 +505,141 @@ func TestDetectCyclesMixedTypes(t *testing.T) { t.Errorf("Expected cycle of length 3, got %d", len(cycle)) } } + +// TestDetectCyclesRelatesToNotACycle tests that bidirectional relates-to links are NOT reported as cycles +// This is the fix for GitHub issue #661: relates-to relationships should be excluded from cycle detection +// because they are inherently bidirectional ("see also" links) and don't affect work ordering. +func TestDetectCyclesRelatesToNotACycle(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create two issues + issue1 := &types.Issue{Title: "Todo 1", Status: types.StatusOpen, Priority: 2, IssueType: types.TypeTask} + issue2 := &types.Issue{Title: "Todo 2", Status: types.StatusOpen, Priority: 2, IssueType: types.TypeTask} + + if err := store.CreateIssue(ctx, issue1, "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + if err := store.CreateIssue(ctx, issue2, "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + // Create bidirectional relates_to links (simulating what bd relate does) + // issue1 relates_to issue2 + _, err := store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issue1.ID, issue2.ID, types.DepRelatesTo) + if err != nil { + t.Fatalf("Insert relates_to dependency failed: %v", err) + } + + // issue2 relates_to issue1 (the reverse link) + _, err = store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issue2.ID, issue1.ID, types.DepRelatesTo) + if err != nil { + t.Fatalf("Insert relates_to dependency failed: %v", err) + } + + // Detect cycles - should find NONE because relates_to is excluded + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) != 0 { + t.Errorf("Expected no cycles for relates_to bidirectional links, but found %d cycles", len(cycles)) + for i, cycle := range cycles { + t.Logf("Cycle %d:", i+1) + for _, issue := range cycle { + t.Logf(" - %s: %s", issue.ID, issue.Title) + } + } + } +} + +// TestDetectCyclesRelatesToWithOtherCycle tests that relates-to is excluded but other cycles are still detected +func TestDetectCyclesRelatesToWithOtherCycle(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create three issues + issue1 := &types.Issue{Title: "Issue A", Status: types.StatusOpen, Priority: 2, IssueType: types.TypeTask} + issue2 := &types.Issue{Title: "Issue B", Status: types.StatusOpen, Priority: 2, IssueType: types.TypeTask} + issue3 := &types.Issue{Title: "Issue C", Status: types.StatusOpen, Priority: 2, IssueType: types.TypeTask} + + if err := store.CreateIssue(ctx, issue1, "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + if err := store.CreateIssue(ctx, issue2, "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + if err := store.CreateIssue(ctx, issue3, "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + // Create bidirectional relates_to between issue1 and issue2 (should NOT trigger cycle) + _, err := store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issue1.ID, issue2.ID, types.DepRelatesTo) + if err != nil { + t.Fatalf("Insert relates_to dependency failed: %v", err) + } + _, err = store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issue2.ID, issue1.ID, types.DepRelatesTo) + if err != nil { + t.Fatalf("Insert relates_to dependency failed: %v", err) + } + + // Create a real cycle with blocks: issue2 -> issue3 -> issue2 + _, err = store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issue2.ID, issue3.ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert blocks dependency failed: %v", err) + } + _, err = store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issue3.ID, issue2.ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert blocks dependency failed: %v", err) + } + + // Detect cycles - should find the blocks cycle but NOT the relates_to "cycle" + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) == 0 { + t.Fatal("Expected to find the blocks cycle, but found none") + } + + // Verify the cycle contains issue2 and issue3, but NOT issue1 + foundIDs := make(map[string]bool) + for _, cycle := range cycles { + for _, issue := range cycle { + foundIDs[issue.ID] = true + } + } + + if !foundIDs[issue2.ID] || !foundIDs[issue3.ID] { + t.Errorf("Expected cycle to contain issue2 and issue3. Found: %v", foundIDs) + } + + // Verify issue1 is NOT in the cycle (it's only connected via relates-to) + if foundIDs[issue1.ID] { + t.Errorf("issue1 should NOT be in cycle (only connected via relates-to), but was found") + } +} diff --git a/internal/storage/sqlite/dependencies.go b/internal/storage/sqlite/dependencies.go index a8ab3429..e28e9e39 100644 --- a/internal/storage/sqlite/dependencies.go +++ b/internal/storage/sqlite/dependencies.go @@ -613,9 +613,12 @@ func (s *SQLiteStorage) GetDependencyTree(ctx context.Context, issueID string, m } // DetectCycles finds circular dependencies and returns the actual cycle paths +// Note: relates-to dependencies are excluded because they are intentionally bidirectional +// ("see also" relationships) and do not represent problematic cycles. func (s *SQLiteStorage) DetectCycles(ctx context.Context) ([][]*types.Issue, error) { // Use recursive CTE to find cycles with full paths // We track the path as a string to work around SQLite's lack of arrays + // Exclude relates-to dependencies since they are inherently bidirectional rows, err := s.db.QueryContext(ctx, ` WITH RECURSIVE paths AS ( SELECT @@ -625,6 +628,7 @@ func (s *SQLiteStorage) DetectCycles(ctx context.Context) ([][]*types.Issue, err issue_id || '→' || depends_on_id as path, 0 as depth FROM dependencies + WHERE type != 'relates-to' UNION ALL @@ -636,7 +640,8 @@ func (s *SQLiteStorage) DetectCycles(ctx context.Context) ([][]*types.Issue, err p.depth + 1 FROM dependencies d JOIN paths p ON d.issue_id = p.depends_on_id - WHERE p.depth < ? + WHERE d.type != 'relates-to' + AND p.depth < ? AND (d.depends_on_id = p.start_id OR p.path NOT LIKE '%' || d.depends_on_id || '→%') ) SELECT DISTINCT path as cycle_path diff --git a/internal/storage/sqlite/dependencies_test.go b/internal/storage/sqlite/dependencies_test.go index be5f8048..c643ae2c 100644 --- a/internal/storage/sqlite/dependencies_test.go +++ b/internal/storage/sqlite/dependencies_test.go @@ -1527,9 +1527,10 @@ func TestDetectCycles_MultipleGraphs(t *testing.T) { } } -// TestDetectCycles_RelatedTypeAllowsAdditionButReportsDetection tests relates-to allows bidirectional links -// even though they're technically cycles. The cycle prevention only skips relates-to during AddDependency. -func TestDetectCycles_RelatedTypeAllowsAdditionButReportsDetection(t *testing.T) { +// TestDetectCycles_RelatesTypeAllowsBidirectionalWithoutCycleReport tests relates-to allows bidirectional links +// and DetectCycles correctly excludes them (they're "see also" links, not problematic cycles). +// This was fixed in GH#661 - relates-to is explicitly excluded from cycle detection. +func TestDetectCycles_RelatesTypeAllowsBidirectionalWithoutCycleReport(t *testing.T) { store, cleanup := setupTestDB(t) defer cleanup() @@ -1552,7 +1553,7 @@ func TestDetectCycles_RelatedTypeAllowsAdditionButReportsDetection(t *testing.T) t.Fatalf("AddDependency for relates-to failed: %v", err) } - // Add B relates-to A (this should succeed despite creating a cycle because relates-to skips cycle detection) + // Add B relates-to A (this should succeed - relates-to skips cycle prevention) err = store.AddDependency(ctx, &types.Dependency{ IssueID: issueB.ID, DependsOnID: issueA.ID, @@ -1562,15 +1563,16 @@ func TestDetectCycles_RelatedTypeAllowsAdditionButReportsDetection(t *testing.T) t.Fatalf("AddDependency for reverse relates-to failed: %v", err) } - // DetectCycles will report the cycle even though AddDependency allowed it + // DetectCycles should NOT report relates-to as cycles (GH#661 fix) + // relates-to is inherently bidirectional ("see also") and doesn't affect work ordering cycles, err := store.DetectCycles(ctx) if err != nil { t.Fatalf("DetectCycles failed: %v", err) } - // Relates-to bidirectional creates cycles (may report multiple entry points for same cycle) - if len(cycles) == 0 { - t.Error("Expected at least 1 cycle detected for bidirectional relates-to") + // relates-to bidirectional should NOT be reported as a cycle + if len(cycles) != 0 { + t.Errorf("Expected 0 cycles for bidirectional relates-to (GH#661 fix), got %d", len(cycles)) } // Verify both directions exist