From 2c3b62be55ac4a0ec126d499ab86f07e746750ff Mon Sep 17 00:00:00 2001 From: damocles Date: Sun, 31 May 2026 13:23:00 +0200 Subject: [PATCH] topology: drop manager-root special case + notify three agents on reparent (#743) --- hive-c0re/src/coordinator.rs | 83 ++++++++++++++++++++++++++++++++++++ hive-c0re/src/dashboard.rs | 13 ++++-- hive-c0re/src/server.rs | 14 +++--- hive-c0re/src/topology.rs | 49 ++++++++++++++++----- 4 files changed, 139 insertions(+), 20 deletions(-) diff --git a/hive-c0re/src/coordinator.rs b/hive-c0re/src/coordinator.rs index 13340fca..90197923 100644 --- a/hive-c0re/src/coordinator.rs +++ b/hive-c0re/src/coordinator.rs @@ -509,6 +509,89 @@ 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 4381033b..dedc2ce3 100644 --- a/hive-c0re/src/dashboard.rs +++ b/hive-c0re/src/dashboard.rs @@ -2188,16 +2188,21 @@ async fn post_set_parent( .map(str::trim) .filter(|s| !s.is_empty()) .map(str::to_owned); - match crate::topology::set_parent(&child, new_parent.as_deref()) { + // `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 + { 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 30be0364..52b436c7 100644 --- a/hive-c0re/src/server.rs +++ b/hive-c0re/src/server.rs @@ -187,13 +187,15 @@ async fn dispatch(req: &HostRequest, coord: Arc) -> HostResponse { } HostRequest::SetParent { child, new_parent } => { tracing::info!(%child, ?new_parent, "set_parent"); - crate::topology::set_parent(child, new_parent.as_deref()) + // `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 .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 128332cb..62affecc 100644 --- a/hive-c0re/src/topology.rs +++ b/hive-c0re/src/topology.rs @@ -182,18 +182,23 @@ pub fn default_seed(agent_names: &[String]) -> BTreeMap> } /// Pure validation + apply for [`set_parent`]. Splits off so tests -/// 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. +/// 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. 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}")); } @@ -342,10 +347,34 @@ mod tests { } #[test] - fn apply_set_parent_refuses_manager_move() { - let err = apply_set_parent(&topo_three_level(), crate::lifecycle::MANAGER_NAME, None) + 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")) .unwrap_err(); - assert!(err.contains("manager"), "err = {err}"); + assert!(err.contains("cycle"), "err = {err}"); } #[test]