diff --git a/src/core/graph.rs b/src/core/graph.rs index d89051d..307fea8 100644 --- a/src/core/graph.rs +++ b/src/core/graph.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::io; use uuid::Uuid; @@ -35,6 +36,8 @@ impl<'a> BranchGraph<'a> { }]; }; + let mut visited = HashSet::new(); + visited.insert(current_node.id); let mut lineage = vec![BranchLineageNode { branch_name: current_node.branch_name.clone(), pull_request_number: current_node.pull_request.as_ref().map(|pr| pr.number), @@ -52,6 +55,9 @@ impl<'a> BranchGraph<'a> { break; } ParentRef::Branch { node_id } => { + if !visited.insert(*node_id) { + break; // cycle detected + } let Some(parent_node) = self.state.find_branch_by_id(*node_id) else { break; }; @@ -82,9 +88,14 @@ impl<'a> BranchGraph<'a> { pub fn active_descendant_ids(&self, node_id: Uuid) -> Vec { let mut descendants = Vec::new(); + let mut visited = HashSet::new(); + visited.insert(node_id); let mut frontier = self.active_children_ids(node_id); while let Some(child_id) = frontier.pop() { + if !visited.insert(child_id) { + continue; // cycle detected + } descendants.push(child_id); frontier.extend(self.active_children_ids(child_id)); } @@ -97,12 +108,17 @@ impl<'a> BranchGraph<'a> { return 0; }; + let mut visited = HashSet::new(); + visited.insert(node_id); let mut depth = 0; loop { match current_node.parent { ParentRef::Trunk => return depth, ParentRef::Branch { node_id: parent_id } => { + if !visited.insert(parent_id) { + return depth; // cycle detected + } let Some(parent_node) = self.state.find_branch_by_id(parent_id) else { return depth; }; @@ -125,13 +141,37 @@ impl<'a> BranchGraph<'a> { } pub fn subtree(&self, node_id: Uuid) -> io::Result { + let mut visited = HashSet::new(); + self.subtree_inner(node_id, &mut visited) + } + + fn subtree_inner( + &self, + node_id: Uuid, + visited: &mut HashSet, + ) -> io::Result { + if !visited.insert(node_id) { + let branch_info = self + .state + .find_branch_by_id(node_id) + .map(|n| format!(" (branch: {})", n.branch_name)) + .unwrap_or_default(); + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "cycle detected in branch tree at node {}{}", + node_id, branch_info + ), + )); + } + let node = self.state.find_branch_by_id(node_id).ok_or_else(|| { io::Error::new(io::ErrorKind::NotFound, "tracked branch was not found") })?; let mut children = Vec::new(); for child_id in self.active_children_ids(node_id) { - children.push(self.subtree(child_id)?); + children.push(self.subtree_inner(child_id, visited)?); } Ok(BranchTreeNode { @@ -273,4 +313,128 @@ mod tests { } ); } + + /// Helper to build a cyclic state where A's parent is B and B's parent is A. + fn fixture_cycle_state() -> (DaggerState, Uuid, Uuid) { + let id_a = Uuid::new_v4(); + let id_b = Uuid::new_v4(); + + ( + DaggerState { + version: DAGGER_STATE_VERSION, + nodes: vec![ + BranchNode { + id: id_a, + branch_name: "branch-a".into(), + parent: ParentRef::Branch { node_id: id_b }, + base_ref: "branch-b".into(), + fork_point_oid: "aaa".into(), + head_oid_at_creation: "aaa".into(), + created_at_unix_secs: 1, + divergence_state: BranchDivergenceState::Unknown, + pull_request: None, + archived: false, + }, + BranchNode { + id: id_b, + branch_name: "branch-b".into(), + parent: ParentRef::Branch { node_id: id_a }, + base_ref: "branch-a".into(), + fork_point_oid: "bbb".into(), + head_oid_at_creation: "bbb".into(), + created_at_unix_secs: 2, + divergence_state: BranchDivergenceState::Unknown, + pull_request: None, + archived: false, + }, + ], + }, + id_a, + id_b, + ) + } + + #[test] + fn lineage_terminates_on_cycle() { + let (state, _, _) = fixture_cycle_state(); + let graph = BranchGraph::new(&state); + + let result = graph.lineage("branch-a", "main"); + + // Should contain branch-a and branch-b exactly once, cycle broken + assert_eq!(result.len(), 2); + assert_eq!(result[0].branch_name, "branch-a"); + assert_eq!(result[1].branch_name, "branch-b"); + } + + #[test] + fn branch_depth_terminates_on_cycle() { + let (state, id_a, _) = fixture_cycle_state(); + let graph = BranchGraph::new(&state); + + let depth = graph.branch_depth(id_a); + // A -> B -> cycle detected, depth should be 1 + assert_eq!(depth, 1); + } + + #[test] + fn active_descendant_ids_terminates_on_cycle() { + // Create a state where a node appears as its own descendant via a + // circular child reference. We simulate this by making node C point + // to node A as parent while A points to C as parent, forming a cycle + // in the child-walk direction. + let id_a = Uuid::new_v4(); + let id_c = Uuid::new_v4(); + + let state = DaggerState { + version: DAGGER_STATE_VERSION, + nodes: vec![ + BranchNode { + id: id_a, + branch_name: "cycle-a".into(), + parent: ParentRef::Branch { node_id: id_c }, + base_ref: "cycle-c".into(), + fork_point_oid: "aaa".into(), + head_oid_at_creation: "aaa".into(), + created_at_unix_secs: 1, + divergence_state: BranchDivergenceState::Unknown, + pull_request: None, + archived: false, + }, + BranchNode { + id: id_c, + branch_name: "cycle-c".into(), + parent: ParentRef::Branch { node_id: id_a }, + base_ref: "cycle-a".into(), + fork_point_oid: "ccc".into(), + head_oid_at_creation: "ccc".into(), + created_at_unix_secs: 2, + divergence_state: BranchDivergenceState::Unknown, + pull_request: None, + archived: false, + }, + ], + }; + + let graph = BranchGraph::new(&state); + + // A is a child of C and C is a child of A — this creates a cycle in + // descendant traversal. It must terminate. + let descendants = graph.active_descendant_ids(id_a); + // Should contain id_c exactly once (it's a "child" of id_a since id_c's parent is id_a) + assert_eq!(descendants.len(), 1); + assert_eq!(descendants[0], id_c); + } + + #[test] + fn subtree_returns_error_on_cycle() { + let (state, id_a, _) = fixture_cycle_state(); + let graph = BranchGraph::new(&state); + + let result = graph.subtree(id_a); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + assert!(err.to_string().contains("cycle detected")); + } }