diff --git a/hive-c0re/src/agent_config/topology.rs b/hive-c0re/src/agent_config/topology.rs index 19c6c278..96e33b38 100644 --- a/hive-c0re/src/agent_config/topology.rs +++ b/hive-c0re/src/agent_config/topology.rs @@ -12,8 +12,22 @@ //! enforcement semantics: `docs/agent-hierarchy.md::Current state`. //! `` sentinel resolution (delivered by [`resolve_recipient`]): //! `docs/conventions.md::Recipient sentinels`. +//! +//! ## Graph representation +//! +//! The on-disk format stays as a flat JSON map `name → parent | null` +//! (small, git-diffable). In-memory, heavy algorithms (descendant checks, +//! cycle detection) use a [`petgraph`] directed graph where each edge runs +//! **parent → child**. This replaces the ad-hoc bounded walks that existed +//! before: petgraph's `has_path_connecting` / `is_cyclic_directed` +//! are correct for graphs of any depth (no 32-hop ceiling) and well-tested. +//! The graph is built on demand from the flat map; it is not cached across +//! calls (the map is small and disk I/O dominates anyway). use std::collections::BTreeMap; + +use petgraph::algo::{has_path_connecting, is_cyclic_directed}; +use petgraph::graph::{DiGraph, NodeIndex}; use std::path::PathBuf; const TOPOLOGY_FILE: &str = "topology.json"; @@ -136,29 +150,75 @@ pub fn resolve_recipient_in( } /// True when `candidate` is `ancestor` or any descendant of -/// `ancestor` per the current topology. Walks parents from -/// `candidate` upward; the walk terminates at root or on a cycle -/// (cycle defence: bounded to 32 hops, more than any plausible -/// hive depth). Used by the cancel-authorization check in -/// `socket_server::handle_cancel_schedule` to enforce +/// `ancestor` per the current on-disk topology. +/// +/// Delegates to [`is_descendant_of_in`] on the result of [`read`] so +/// the algorithm is the same petgraph BFS used everywhere else. No +/// depth limit — the 32-hop bounded walk this replaced was correct for +/// any plausible hive but carried a latent ceiling; this has none. +/// +/// Used by the cancel-authorization checks in `socket_server` to enforce /// "managers can cancel anything their subtree owns." #[must_use] pub fn is_descendant_of(candidate: &str, ancestor: &str) -> bool { + is_descendant_of_in(&read(), candidate, ancestor) +} + +/// Build an in-memory petgraph directed graph from the topology map. +/// +/// Edges run **parent → child** so that: +/// - `children_of(name)` = outgoing neighbours of `name`'s node +/// - `is_descendant_of(candidate, ancestor)` = path exists from `ancestor` +/// to `candidate` via `has_path_connecting` +/// - cycle detection = `is_cyclic_directed` after a speculative edge insert +/// +/// Returns the graph and a `BTreeMap` for O(log n) +/// name-to-node lookups. Both are local to each call site — the graph is +/// not cached. Hive topologies are small (< ~100 nodes); building on demand +/// is dominated by the surrounding disk read. +#[must_use] +fn build_graph( + topo: &BTreeMap>, +) -> (DiGraph, BTreeMap) { + let mut graph: DiGraph = DiGraph::new(); + let mut idx: BTreeMap = BTreeMap::new(); + + // Add one node per agent. + for name in topo.keys() { + let ni = graph.add_node(name.clone()); + idx.insert(name.clone(), ni); + } + // Add parent→child edges. + for (name, parent_opt) in topo { + if let Some(parent) = parent_opt { + if let (Some(&p_idx), Some(&c_idx)) = (idx.get(parent), idx.get(name.as_str())) { + graph.add_edge(p_idx, c_idx, ()); + } + } + } + (graph, idx) +} + +/// Return true when `candidate` is a descendant of `ancestor` in the +/// given topology map. Uses petgraph BFS/DFS (`has_path_connecting`) +/// — no depth limit and no manually bounded walk. Pure; no disk I/O. +/// +/// Same semantics as the disk-reading [`is_descendant_of`]: a node is +/// considered a descendant of itself (`candidate == ancestor` → true). +#[must_use] +pub fn is_descendant_of_in( + topo: &BTreeMap>, + candidate: &str, + ancestor: &str, +) -> bool { if candidate == ancestor { return true; } - let topo = read(); - let mut cur = candidate.to_owned(); - for _ in 0..32 { - let Some(parent) = topo.get(&cur).cloned().flatten() else { - return false; - }; - if parent == ancestor { - return true; - } - cur = parent; - } - false + let (graph, idx) = build_graph(topo); + let (Some(&anc_ni), Some(&cand_ni)) = (idx.get(ancestor), idx.get(candidate)) else { + return false; + }; + has_path_connecting(&graph, anc_ni, cand_ni, None) } /// Persist the topology map. Sorted JSON output (`BTreeMap` is sorted by @@ -226,20 +286,19 @@ pub fn apply_set_parent( if p == child { return Err("an agent cannot be its own parent".to_owned()); } - // Cycle check: walk `p`'s ancestors in the EXISTING map. If - // we hit `child`, then making `child`'s parent = `p` would - // close the loop (child → … → p → child). - let mut cur = p.to_owned(); - for _ in 0..32 { - if cur == child { + // Cycle check via petgraph: build the current graph, speculatively + // insert the proposed parent→child edge, then test for cycles with + // `is_cyclic_directed`. This replaces the earlier ad-hoc 32-hop + // ancestor walk — petgraph is correct for any tree depth and the + // algorithm is well-tested. + let (mut graph, idx) = build_graph(topo); + if let (Some(&p_ni), Some(&c_ni)) = (idx.get(p), idx.get(child)) { + graph.add_edge(p_ni, c_ni, ()); + if is_cyclic_directed(&graph) { return Err(format!( "cycle: {p} is in {child}'s subtree (would create a loop)" )); } - let Some(next) = topo.get(&cur).cloned().flatten() else { - break; - }; - cur = next; } } let mut next = topo.clone(); @@ -818,6 +877,75 @@ mod tests { assert!(top_level_agents_in(&topo).is_empty()); } + // ----------------------------------------------------------------------- + // is_descendant_of_in tests (petgraph-backed; pure / no disk I/O) + // ----------------------------------------------------------------------- + + #[test] + fn is_descendant_of_in_self_is_true() { + let topo = topo_three_level(); + assert!(is_descendant_of_in(&topo, "alice", "alice")); + assert!(is_descendant_of_in( + &topo, + crate::lifecycle::MANAGER_NAME, + crate::lifecycle::MANAGER_NAME + )); + } + + #[test] + fn is_descendant_of_in_direct_child() { + let topo = topo_three_level(); + // alice is a direct child of manager. + assert!(is_descendant_of_in( + &topo, + "alice", + crate::lifecycle::MANAGER_NAME + )); + } + + #[test] + fn is_descendant_of_in_grandchild() { + let topo = topo_three_level(); + // bob is manager → alice → bob; should be reachable from manager. + assert!(is_descendant_of_in( + &topo, + "bob", + crate::lifecycle::MANAGER_NAME + )); + assert!(is_descendant_of_in( + &topo, + "carol", + crate::lifecycle::MANAGER_NAME + )); + } + + #[test] + fn is_descendant_of_in_parent_not_descendant_of_child() { + let topo = topo_three_level(); + // alice is NOT a descendant of bob (alice is bob's grandparent). + assert!(!is_descendant_of_in(&topo, "alice", "bob")); + assert!(!is_descendant_of_in( + &topo, + crate::lifecycle::MANAGER_NAME, + "alice" + )); + } + + #[test] + fn is_descendant_of_in_sibling_is_not_descendant() { + let topo = topo_three_level(); + // bob and carol are siblings under alice; neither descends from the other. + assert!(!is_descendant_of_in(&topo, "bob", "carol")); + assert!(!is_descendant_of_in(&topo, "carol", "bob")); + } + + #[test] + fn is_descendant_of_in_unknown_is_false() { + let topo = topo_three_level(); + assert!(!is_descendant_of_in(&topo, "nobody", "alice")); + assert!(!is_descendant_of_in(&topo, "alice", "nobody")); + } + // ----------------------------------------------------------------------- // Roles tests (no disk I/O — use the pure `has_role_in` / in-memory maps) // -----------------------------------------------------------------------