diff --git a/docs/agent-hierarchy.md b/docs/agent-hierarchy.md index c914499c..36ae505c 100644 --- a/docs/agent-hierarchy.md +++ b/docs/agent-hierarchy.md @@ -30,16 +30,11 @@ operator-driven (#486 / #487): All three converge on `topology::set_parent`, which delegates the validation rules to a pure `apply_set_parent` helper. Refuses: +- reparenting the manager (structurally root), - unknown `child` / `new_parent` (typo guard), - self-parenting, - cycles (32-hop ancestor walk, mirroring `is_descendant_of`). -Post-#743 the manager is reparentable like any other agent — the -"structurally root" carve-out was historical paranoia; the manager's -privileges live on its MCP socket, not its tree position, and the -cycle walk above catches the only real safety concern (moving the -manager under one of its own descendants). - Idempotent no-op fast path skips the disk write when the parent is already what's requested. After a successful write the surfaces call `Coordinator::rescan_containers_and_emit` so connected dashboard diff --git a/hive-c0re/src/coordinator.rs b/hive-c0re/src/coordinator.rs index 90197923..13340fca 100644 --- a/hive-c0re/src/coordinator.rs +++ b/hive-c0re/src/coordinator.rs @@ -509,89 +509,6 @@ impl Coordinator { } } - /// Apply a topology reparent + fan the resulting notifications - /// out to the three affected agents (#743). On success, drops a - /// one-line system message into the inbox of: - /// - /// 1. The **old parent** (if any) — `"{child} moved out of your - /// subtree to {new_parent_or_root}"`. - /// 2. The **new parent** (if any) — `"{child} just moved into - /// your subtree (was previously under - /// {old_parent_or_root})"`. - /// 3. The **moved agent** — `"your parent changed from - /// {old_parent_or_root} to {new_parent_or_root}"`. - /// - /// `_or_root` resolves to the literal string `""` when the - /// slot is `None`, keeping the wording consistent with the - /// `` sentinel's "root → operator" routing (#692). The - /// notifications fire as ordinary broker messages with - /// `from = hive_sh4re::SYSTEM_SENDER` so the dashboard renders - /// them under the existing system-source styling. - /// - /// Idempotent: if `topology::set_parent` skipped the disk write - /// (same-parent no-op), no messages fire. The `topology` module's - /// validation (`apply_set_parent`: unknown agent, cycle, etc.) - /// runs *before* any message is sent, so a refused move never - /// notifies anyone. - /// - /// Also drives a `rescan_containers_and_emit` after the write so - /// the dashboard tree re-renders without polling. - pub async fn reparent_with_notify( - self: &Arc, - child: &str, - new_parent: Option<&str>, - ) -> std::result::Result<(), String> { - // Snapshot the old parent BEFORE the write so the - // notifications can describe both sides of the change. - let topo_before = crate::topology::read(); - let old_parent = topo_before.get(child).cloned().flatten(); - - // The disk write happens here; topology validation + - // idempotent fast-path inside `set_parent` may short-circuit - // (same parent → no-op). We mirror the same idempotent shape - // for the notifications: if nothing changed, send nothing. - crate::topology::set_parent(child, new_parent)?; - - let changed = old_parent.as_deref() != new_parent; - if changed { - let old_label = old_parent.as_deref().unwrap_or(""); - let new_label = new_parent.unwrap_or(""); - // System-source notifications. Best-effort: a failed broker - // send is logged + ignored so a transient sqlite error - // doesn't bubble out and unwind the topology write. - if let Some(op) = old_parent.as_deref() { - let _ = self.broker.send(&hive_sh4re::Message { - from: hive_sh4re::SYSTEM_SENDER.to_owned(), - to: op.to_owned(), - body: format!("{child} moved out of your subtree to {new_label}"), - in_reply_to: None, - }); - } - if let Some(np) = new_parent { - let _ = self.broker.send(&hive_sh4re::Message { - from: hive_sh4re::SYSTEM_SENDER.to_owned(), - to: np.to_owned(), - body: format!( - "{child} just moved into your subtree (was previously under {old_label})" - ), - in_reply_to: None, - }); - } - let _ = self.broker.send(&hive_sh4re::Message { - from: hive_sh4re::SYSTEM_SENDER.to_owned(), - to: child.to_owned(), - body: format!("your parent changed from {old_label} to {new_label}"), - in_reply_to: None, - }); - } - - // Rescan + diff-emit regardless of whether messages fired — - // even an idempotent no-op might have happened concurrently - // with another lifecycle event the dashboard cares about. - self.rescan_containers_and_emit().await; - Ok(()) - } - /// Read-only snapshot of the last cached container view. Used by /// `/api/state` to cold-load page-open clients without re-running /// `nixos-container list` themselves; the diff --git a/hive-c0re/src/dashboard.rs b/hive-c0re/src/dashboard.rs index dedc2ce3..4381033b 100644 --- a/hive-c0re/src/dashboard.rs +++ b/hive-c0re/src/dashboard.rs @@ -2188,21 +2188,16 @@ async fn post_set_parent( .map(str::trim) .filter(|s| !s.is_empty()) .map(str::to_owned); - // `reparent_with_notify` wraps `topology::set_parent` with the - // three notification messages (#743) + the ContainerView rescan. - // Idempotent same-parent calls skip both the messages and the - // disk write per the topology fast-path. - match state - .coord - .reparent_with_notify(&child, new_parent.as_deref()) - .await - { + match crate::topology::set_parent(&child, new_parent.as_deref()) { Ok(()) => { tracing::info!( child = %child, new_parent = ?new_parent, "operator: set-parent via dashboard" ); + // Topology drives ContainerView.parent; refresh the + // snapshot so connected viewers see the new tree. + state.coord.rescan_containers_and_emit().await; (StatusCode::OK, "ok").into_response() } Err(e) => error_response(&format!("set-parent {child} failed: {e}")), diff --git a/hive-c0re/src/server.rs b/hive-c0re/src/server.rs index 52b436c7..30be0364 100644 --- a/hive-c0re/src/server.rs +++ b/hive-c0re/src/server.rs @@ -187,15 +187,13 @@ async fn dispatch(req: &HostRequest, coord: Arc) -> HostResponse { } HostRequest::SetParent { child, new_parent } => { tracing::info!(%child, ?new_parent, "set_parent"); - // `reparent_with_notify` wraps `topology::set_parent` - // with the three notification messages (#743) + the - // ContainerView rescan. Idempotent same-parent calls - // skip both the messages and the disk write per the - // topology fast-path. - coord - .reparent_with_notify(child, new_parent.as_deref()) - .await + crate::topology::set_parent(child, new_parent.as_deref()) .map_err(anyhow::Error::msg)?; + // ContainerView.parent is read from topology.json — a + // change here means every container row potentially + // moves in the dashboard tree. Rescan + diff-emit so + // open viewers repaint without polling. + coord.rescan_containers_and_emit().await; HostResponse::success() } }) diff --git a/hive-c0re/src/topology.rs b/hive-c0re/src/topology.rs index 62affecc..128332cb 100644 --- a/hive-c0re/src/topology.rs +++ b/hive-c0re/src/topology.rs @@ -182,23 +182,18 @@ pub fn default_seed(agent_names: &[String]) -> BTreeMap> } /// Pure validation + apply for [`set_parent`]. Splits off so tests -/// can exercise the rules (cycle / unknown) on an in-memory -/// `BTreeMap` without touching the on-disk `topology.json`. Returns -/// either the post-move map (caller writes it back) or a -/// user-readable error string. -/// -/// Pre-#743 this also refused to reparent the manager -/// ("cannot reparent the manager — it is structurally root") — -/// argus-paranoia from #361 that we dropped per mara's -/// `#9512` / `#9557`: the manager's special powers come from its -/// privileged MCP socket, not its tree position. The cycle walk -/// below covers "moving X under its own descendant" for the -/// manager as much as any other agent. +/// can exercise the rules (cycle / unknown / manager-protect) on +/// an in-memory `BTreeMap` without touching the on-disk +/// `topology.json`. Returns either the post-move map (caller +/// writes it back) or a user-readable error string. pub fn apply_set_parent( topo: &BTreeMap>, child: &str, new_parent: Option<&str>, ) -> Result>, String> { + if child == crate::lifecycle::MANAGER_NAME { + return Err("cannot reparent the manager — it is structurally root".to_owned()); + } if !topo.contains_key(child) { return Err(format!("unknown agent: {child}")); } @@ -347,34 +342,10 @@ mod tests { } #[test] - fn apply_set_parent_allows_manager_move() { - // Post-#743: the manager is reparentable like any other agent - // (its privileges live on the MCP socket, not its tree - // position). Build a topo with an unrelated root-level agent - // `peer` so moving the manager under it doesn't trip the - // cycle walk (every non-manager agent in topo_three_level - // descends from the manager, so that fixture can't exercise - // a legal manager move). - let mut topo = BTreeMap::new(); - topo.insert(crate::lifecycle::MANAGER_NAME.to_owned(), None); - topo.insert("peer".to_owned(), None); - let next = apply_set_parent(&topo, crate::lifecycle::MANAGER_NAME, Some("peer")) - .expect("manager move should succeed post-#743"); - assert_eq!( - next.get(crate::lifecycle::MANAGER_NAME), - Some(&Some("peer".to_owned())) - ); - } - - #[test] - fn apply_set_parent_refuses_manager_under_own_descendant() { - // Moving the manager under `bob` (who already lives under - // `alice` who lives under the manager) would close the loop. - // The general cycle walk catches this; no separate manager - // guard needed. - let err = apply_set_parent(&topo_three_level(), crate::lifecycle::MANAGER_NAME, Some("bob")) + fn apply_set_parent_refuses_manager_move() { + let err = apply_set_parent(&topo_three_level(), crate::lifecycle::MANAGER_NAME, None) .unwrap_err(); - assert!(err.contains("cycle"), "err = {err}"); + assert!(err.contains("manager"), "err = {err}"); } #[test]