From 8c2679a80e00ebe753ed4e0343c02968705efaf1 Mon Sep 17 00:00:00 2001 From: David Laing Date: Mon, 27 Oct 2025 19:51:41 +0000 Subject: [PATCH] Fix substring bug in dependency tree cycle detection (#159) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add .worktrees/ to .gitignore Prevents git worktree contents from being tracked in the repository. * Fix substring bug in dependency tree cycle detection The cycle detection in GetDependencyTree() was using a simple substring match which incorrectly flagged valid nodes as cycles. For example, "bd-1" would be blocked because "bd-10" contains "bd-1" as a substring. This bug affects any beads project where issue IDs contain each other as substrings (BD-1/BD-10, ISSUE-1/ISSUE-10, etc). Changed from: AND t.path NOT LIKE '%' || i.id || '%' To delimiter-aware checks that respect the → separator: AND t.path != i.id AND t.path NOT LIKE i.id || '→%' AND t.path NOT LIKE '%→' || i.id || '→%' AND t.path NOT LIKE '%→' || i.id This ensures we only match complete issue IDs, not substrings. Added TestGetDependencyTree_SubstringBug to demonstrate and prevent regression of this issue. The test creates a chain from bd-10 to bd-1 and verifies all nodes appear in the dependency tree. Discovered while testing dependency tree visualization with bd-1/bd-10. --- .gitignore | 3 + internal/storage/sqlite/dependencies.go | 5 +- internal/storage/sqlite/dependencies_test.go | 103 +++++++++++++++++++ 3 files changed, 110 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 1f33544b..676e934f 100644 --- a/.gitignore +++ b/.gitignore @@ -53,3 +53,6 @@ result # GoReleaser build artifacts dist/ + +# Git worktrees +.worktrees/ diff --git a/internal/storage/sqlite/dependencies.go b/internal/storage/sqlite/dependencies.go index 84690346..55b34c68 100644 --- a/internal/storage/sqlite/dependencies.go +++ b/internal/storage/sqlite/dependencies.go @@ -496,7 +496,10 @@ func (s *SQLiteStorage) GetDependencyTree(ctx context.Context, issueID string, m JOIN dependencies d ON i.id = d.depends_on_id JOIN tree t ON d.issue_id = t.id WHERE t.depth < ? - AND t.path NOT LIKE '%' || i.id || '%' + AND t.path != i.id + AND t.path NOT LIKE i.id || '→%' + AND t.path NOT LIKE '%→' || i.id || '→%' + AND t.path NOT LIKE '%→' || i.id ) SELECT id, title, status, priority, description, design, acceptance_criteria, notes, issue_type, assignee, diff --git a/internal/storage/sqlite/dependencies_test.go b/internal/storage/sqlite/dependencies_test.go index a7f2f734..729ca9cb 100644 --- a/internal/storage/sqlite/dependencies_test.go +++ b/internal/storage/sqlite/dependencies_test.go @@ -725,3 +725,106 @@ func TestCrossTypeCyclePreventionThreeIssues(t *testing.T) { t.Errorf("Expected no cycles after prevention, but found %d", len(cycles)) } } + +func TestGetDependencyTree_SubstringBug(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create 10 issues so we have both bd-1 and bd-10 (substring issue) + // The bug: when traversing from bd-10, bd-1 gets incorrectly excluded + // because "bd-10" contains "bd-1" as a substring + issues := make([]*types.Issue, 10) + for i := 0; i < 10; i++ { + issues[i] = &types.Issue{ + Title: fmt.Sprintf("Issue %d", i+1), + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + err := store.CreateIssue(ctx, issues[i], "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + } + + // Create chain: bd-10 → bd-9 → bd-8 → bd-2 → bd-1 + // This tests the substring bug where bd-1 should appear but won't due to substring matching + err := store.AddDependency(ctx, &types.Dependency{ + IssueID: issues[9].ID, // bd-10 + DependsOnID: issues[8].ID, // bd-9 + Type: types.DepBlocks, + }, "test-user") + if err != nil { + t.Fatalf("AddDependency bd-10→bd-9 failed: %v", err) + } + + err = store.AddDependency(ctx, &types.Dependency{ + IssueID: issues[8].ID, // bd-9 + DependsOnID: issues[7].ID, // bd-8 + Type: types.DepBlocks, + }, "test-user") + if err != nil { + t.Fatalf("AddDependency bd-9→bd-8 failed: %v", err) + } + + err = store.AddDependency(ctx, &types.Dependency{ + IssueID: issues[7].ID, // bd-8 + DependsOnID: issues[1].ID, // bd-2 + Type: types.DepBlocks, + }, "test-user") + if err != nil { + t.Fatalf("AddDependency bd-8→bd-2 failed: %v", err) + } + + err = store.AddDependency(ctx, &types.Dependency{ + IssueID: issues[1].ID, // bd-2 + DependsOnID: issues[0].ID, // bd-1 + Type: types.DepBlocks, + }, "test-user") + if err != nil { + t.Fatalf("AddDependency bd-2→bd-1 failed: %v", err) + } + + // Get tree starting from bd-10 + tree, err := store.GetDependencyTree(ctx, issues[9].ID, 10, false) + if err != nil { + t.Fatalf("GetDependencyTree failed: %v", err) + } + + // Create map of issue IDs in tree for easy checking + treeIDs := make(map[string]bool) + for _, node := range tree { + treeIDs[node.ID] = true + } + + // Verify all issues in the chain appear in the tree + // This is the KEY test: bd-1 should be in the tree + // With the substring bug, bd-1 will be missing because "bd-10" contains "bd-1" + expectedIssues := []int{9, 8, 7, 1, 0} // bd-10, bd-9, bd-8, bd-2, bd-1 + for _, idx := range expectedIssues { + if !treeIDs[issues[idx].ID] { + t.Errorf("Expected %s in dependency tree, but it was missing (substring bug)", issues[idx].ID) + } + } + + // Verify we have the correct number of nodes + if len(tree) != 5 { + t.Errorf("Expected 5 nodes in tree, got %d. Missing nodes indicate substring bug.", len(tree)) + } + + // Verify depths are correct + depthMap := make(map[string]int) + for _, node := range tree { + depthMap[node.ID] = node.Depth + } + + // Check depths: bd-10(0) → bd-9(1) → bd-8(2) → bd-2(3) → bd-1(4) + if depthMap[issues[9].ID] != 0 { + t.Errorf("Expected bd-10 at depth 0, got %d", depthMap[issues[9].ID]) + } + if depthMap[issues[0].ID] != 4 { + t.Errorf("Expected bd-1 at depth 4, got %d", depthMap[issues[0].ID]) + } +}