From 844e9ffc02ae0d51de413a9dd9e92bc8616911a6 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Sat, 20 Dec 2025 13:33:15 -0800 Subject: [PATCH] fix(deps): exclude relates-to from cycle detection (fixes #661) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit relates-to dependencies are intentionally bidirectional ("see also" links) and should not be reported as cycles by bd dep cycles. This matches the existing behavior in AddDependency which already skips cycle prevention for relates-to. Changes: - Filter relates-to edges in DetectCycles SQL query - Add tests for relates-to exclusion from cycle detection - Update existing test to reflect correct behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .../storage/sqlite/cycle_detection_test.go | 138 ++++++++++++++++++ internal/storage/sqlite/dependencies.go | 7 +- internal/storage/sqlite/dependencies_test.go | 18 ++- 3 files changed, 154 insertions(+), 9 deletions(-) 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