johno/beads
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(deps): exclude relates-to from cycle detection (fixes #661)

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Steve Yegge
2025-12-20 13:33:15 -08:00
parent f92a741501
commit 844e9ffc02
3 changed files with 154 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

138
internal/storage/sqlite/cycle_detection_test.go
View File

@@ -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")
}
}

7
internal/storage/sqlite/dependencies.go
View File

@@ -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

18
internal/storage/sqlite/dependencies_test.go
View File

@@ -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