From dd8f51c4331d289cce642d48a99526ccc4d26db9 Mon Sep 17 00:00:00 2001 From: David Laing Date: Mon, 27 Oct 2025 09:17:07 +0000 Subject: [PATCH] feat: add reverse mode to dependency tree traversal - Add reverse parameter to GetDependencyTree interface - Implement reverse SQL traversal (dependents vs dependencies) - Add comprehensive test for reverse mode (TDD: test-first approach) - Update existing test calls with reverse=false for backward compatibility Reverse mode inverts tree direction to show dependents instead of dependencies: - Normal: JOIN dependencies d ON i.id = d.depends_on_id (traverse UP) - Reverse: JOIN dependencies d ON i.id = d.issue_id (traverse DOWN) All storage tests passing. No regressions. --- internal/storage/sqlite/dependencies.go | 122 +++++++++++++------ internal/storage/sqlite/dependencies_test.go | 83 ++++++++++++- internal/storage/storage.go | 4 +- 3 files changed, 166 insertions(+), 43 deletions(-) diff --git a/internal/storage/sqlite/dependencies.go b/internal/storage/sqlite/dependencies.go index 84690346..afc313a0 100644 --- a/internal/storage/sqlite/dependencies.go +++ b/internal/storage/sqlite/dependencies.go @@ -462,49 +462,97 @@ func (s *SQLiteStorage) GetAllDependencyRecords(ctx context.Context) (map[string // When showAllPaths is false (default), nodes appearing via multiple paths (diamond dependencies) // appear only once at their shallowest depth in the tree. // When showAllPaths is true, all paths are shown with duplicate nodes at different depths. -func (s *SQLiteStorage) GetDependencyTree(ctx context.Context, issueID string, maxDepth int, showAllPaths bool) ([]*types.TreeNode, error) { +// When reverse is true, shows dependent tree (what was discovered from this) instead of dependency tree (what blocks this). +func (s *SQLiteStorage) GetDependencyTree(ctx context.Context, issueID string, maxDepth int, showAllPaths bool, reverse bool) ([]*types.TreeNode, error) { if maxDepth <= 0 { maxDepth = 50 } + // Build SQL query based on direction + // Normal mode: traverse dependencies (what blocks me) - goes UP + // Reverse mode: traverse dependents (what was discovered from me) - goes DOWN + var query string + if reverse { + // Reverse: show dependents (what depends on this issue) + query = ` + WITH RECURSIVE tree AS ( + SELECT + i.id, i.title, i.status, i.priority, i.description, i.design, + i.acceptance_criteria, i.notes, i.issue_type, i.assignee, + i.estimated_minutes, i.created_at, i.updated_at, i.closed_at, + i.external_ref, + 0 as depth, + i.id as path, + i.id as parent_id + FROM issues i + WHERE i.id = ? + + UNION ALL + + SELECT + i.id, i.title, i.status, i.priority, i.description, i.design, + i.acceptance_criteria, i.notes, i.issue_type, i.assignee, + i.estimated_minutes, i.created_at, i.updated_at, i.closed_at, + i.external_ref, + t.depth + 1, + t.path || '→' || i.id, + t.id + FROM issues i + JOIN dependencies d ON i.id = d.issue_id + JOIN tree t ON d.depends_on_id = t.id + WHERE t.depth < ? + AND t.path NOT LIKE '%' || i.id || '%' + ) + SELECT id, title, status, priority, description, design, + acceptance_criteria, notes, issue_type, assignee, + estimated_minutes, created_at, updated_at, closed_at, + external_ref, depth, parent_id + FROM tree + ORDER BY depth, priority, id + ` + } else { + // Normal: show dependencies (what this issue depends on) + query = ` + WITH RECURSIVE tree AS ( + SELECT + i.id, i.title, i.status, i.priority, i.description, i.design, + i.acceptance_criteria, i.notes, i.issue_type, i.assignee, + i.estimated_minutes, i.created_at, i.updated_at, i.closed_at, + i.external_ref, + 0 as depth, + i.id as path, + i.id as parent_id + FROM issues i + WHERE i.id = ? + + UNION ALL + + SELECT + i.id, i.title, i.status, i.priority, i.description, i.design, + i.acceptance_criteria, i.notes, i.issue_type, i.assignee, + i.estimated_minutes, i.created_at, i.updated_at, i.closed_at, + i.external_ref, + t.depth + 1, + t.path || '→' || i.id, + t.id + FROM issues i + 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 || '%' + ) + SELECT id, title, status, priority, description, design, + acceptance_criteria, notes, issue_type, assignee, + estimated_minutes, created_at, updated_at, closed_at, + external_ref, depth, parent_id + FROM tree + ORDER BY depth, priority, id + ` + } + // First, build the complete tree with all paths using recursive CTE // We need to track the full path to handle proper tree structure - rows, err := s.db.QueryContext(ctx, ` - WITH RECURSIVE tree AS ( - SELECT - i.id, i.title, i.status, i.priority, i.description, i.design, - i.acceptance_criteria, i.notes, i.issue_type, i.assignee, - i.estimated_minutes, i.created_at, i.updated_at, i.closed_at, - i.external_ref, - 0 as depth, - i.id as path, - i.id as parent_id - FROM issues i - WHERE i.id = ? - - UNION ALL - - SELECT - i.id, i.title, i.status, i.priority, i.description, i.design, - i.acceptance_criteria, i.notes, i.issue_type, i.assignee, - i.estimated_minutes, i.created_at, i.updated_at, i.closed_at, - i.external_ref, - t.depth + 1, - t.path || '→' || i.id, - t.id - FROM issues i - 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 || '%' - ) - SELECT id, title, status, priority, description, design, - acceptance_criteria, notes, issue_type, assignee, - estimated_minutes, created_at, updated_at, closed_at, - external_ref, depth, parent_id - FROM tree - ORDER BY depth, priority, id - `, issueID, maxDepth) + rows, err := s.db.QueryContext(ctx, query, issueID, maxDepth) if err != nil { return nil, fmt.Errorf("failed to get dependency tree: %w", err) } diff --git a/internal/storage/sqlite/dependencies_test.go b/internal/storage/sqlite/dependencies_test.go index a7f2f734..0ce53208 100644 --- a/internal/storage/sqlite/dependencies_test.go +++ b/internal/storage/sqlite/dependencies_test.go @@ -249,7 +249,7 @@ func TestGetDependencyTree(t *testing.T) { store.AddDependency(ctx, &types.Dependency{IssueID: issue3.ID, DependsOnID: issue2.ID, Type: types.DepBlocks}, "test-user") // Get tree starting from issue3 - tree, err := store.GetDependencyTree(ctx, issue3.ID, 10, false) + tree, err := store.GetDependencyTree(ctx, issue3.ID, 10, false, false) if err != nil { t.Fatalf("GetDependencyTree failed: %v", err) } @@ -311,7 +311,7 @@ func TestGetDependencyTree_TruncationDepth(t *testing.T) { } // Get tree with maxDepth=2 (should only get 3 nodes: depths 0, 1, 2) - tree, err := store.GetDependencyTree(ctx, issues[4].ID, 2, false) + tree, err := store.GetDependencyTree(ctx, issues[4].ID, 2, false, false) if err != nil { t.Fatalf("GetDependencyTree failed: %v", err) } @@ -354,7 +354,7 @@ func TestGetDependencyTree_DefaultDepth(t *testing.T) { }, "test-user") // Get tree with default depth (50) - tree, err := store.GetDependencyTree(ctx, issue2.ID, 50, false) + tree, err := store.GetDependencyTree(ctx, issue2.ID, 50, false, false) if err != nil { t.Fatalf("GetDependencyTree failed: %v", err) } @@ -399,7 +399,7 @@ func TestGetDependencyTree_MaxDepthOne(t *testing.T) { }, "test-user") // Get tree with maxDepth=1 (should get root + one level) - tree, err := store.GetDependencyTree(ctx, issue3.ID, 1, false) + tree, err := store.GetDependencyTree(ctx, issue3.ID, 1, false, false) if err != nil { t.Fatalf("GetDependencyTree failed: %v", err) } @@ -725,3 +725,78 @@ func TestCrossTypeCyclePreventionThreeIssues(t *testing.T) { t.Errorf("Expected no cycles after prevention, but found %d", len(cycles)) } } + +func TestGetDependencyTree_Reverse(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + ctx := context.Background() + + // Create a dependency chain: issue1 <- issue2 <- issue3 + // (issue3 depends on issue2, issue2 depends on issue1) + issue1 := &types.Issue{ + Title: "Base issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + issue2 := &types.Issue{ + Title: "Depends on issue1", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + issue3 := &types.Issue{ + Title: "Depends on issue2", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + + store.CreateIssue(ctx, issue1, "test") + store.CreateIssue(ctx, issue2, "test") + store.CreateIssue(ctx, issue3, "test") + + // Create dependencies: issue3 → issue2 → issue1 + dep1 := &types.Dependency{IssueID: issue2.ID, DependsOnID: issue1.ID, Type: types.DepBlocks} + dep2 := &types.Dependency{IssueID: issue3.ID, DependsOnID: issue2.ID, Type: types.DepBlocks} + store.AddDependency(ctx, dep1, "test") + store.AddDependency(ctx, dep2, "test") + + // Test normal mode: from issue3, should traverse UP to issue1 + normalTree, err := store.GetDependencyTree(ctx, issue3.ID, 10, false, false) + if err != nil { + t.Fatalf("GetDependencyTree normal mode failed: %v", err) + } + if len(normalTree) != 3 { + t.Fatalf("Expected 3 nodes in normal tree, got %d", len(normalTree)) + } + + // Test reverse mode: from issue1, should traverse DOWN to issue3 + reverseTree, err := store.GetDependencyTree(ctx, issue1.ID, 10, false, true) + if err != nil { + t.Fatalf("GetDependencyTree reverse mode failed: %v", err) + } + if len(reverseTree) != 3 { + t.Fatalf("Expected 3 nodes in reverse tree, got %d", len(reverseTree)) + } + + // Verify reverse tree structure: issue1 at depth 0 + depthMap := make(map[string]int) + for _, node := range reverseTree { + depthMap[node.ID] = node.Depth + } + + if depthMap[issue1.ID] != 0 { + t.Errorf("Expected depth 0 for %s in reverse tree, got %d", issue1.ID, depthMap[issue1.ID]) + } + + // issue2 should be at depth 1 (depends on issue1) + if depthMap[issue2.ID] != 1 { + t.Errorf("Expected depth 1 for %s in reverse tree, got %d", issue2.ID, depthMap[issue2.ID]) + } + + // issue3 should be at depth 2 (depends on issue2) + if depthMap[issue3.ID] != 2 { + t.Errorf("Expected depth 2 for %s in reverse tree, got %d", issue3.ID, depthMap[issue3.ID]) + } +} diff --git a/internal/storage/storage.go b/internal/storage/storage.go index 22aaa2ad..de49dd85 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -25,7 +25,7 @@ type Storage interface { GetDependents(ctx context.Context, issueID string) ([]*types.Issue, error) GetDependencyRecords(ctx context.Context, issueID string) ([]*types.Dependency, error) GetAllDependencyRecords(ctx context.Context) (map[string][]*types.Dependency, error) - GetDependencyTree(ctx context.Context, issueID string, maxDepth int, showAllPaths bool) ([]*types.TreeNode, error) + GetDependencyTree(ctx context.Context, issueID string, maxDepth int, showAllPaths bool, reverse bool) ([]*types.TreeNode, error) DetectCycles(ctx context.Context) ([][]*types.Issue, error) // Labels @@ -53,7 +53,7 @@ type Storage interface { // Dirty tracking (for incremental JSONL export) GetDirtyIssues(ctx context.Context) ([]string, error) GetDirtyIssueHash(ctx context.Context, issueID string) (string, error) // For timestamp-only dedup (bd-164) - ClearDirtyIssues(ctx context.Context) error // WARNING: Race condition (bd-52), use ClearDirtyIssuesByID + ClearDirtyIssues(ctx context.Context) error // WARNING: Race condition (bd-52), use ClearDirtyIssuesByID ClearDirtyIssuesByID(ctx context.Context, issueIDs []string) error // Export hash tracking (for timestamp-only dedup, bd-164)