Compare commits

...
5 changed files with 145 additions and 21 deletions

View file

@ -30,11 +30,16 @@ 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

View file

@ -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 `"<root>"` when the
/// slot is `None`, keeping the wording consistent with the
/// `<parent>` 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<Self>,
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("<root>");
let new_label = new_parent.unwrap_or("<root>");
// 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

View file

@ -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}")),

View file

@ -187,13 +187,15 @@ async fn dispatch(req: &HostRequest, coord: Arc<Coordinator>) -> 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()
}
})

View file

@ -182,18 +182,23 @@ pub fn default_seed(agent_names: &[String]) -> BTreeMap<String, Option<String>>
}
/// 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<String, Option<String>>,
child: &str,
new_parent: Option<&str>,
) -> Result<BTreeMap<String, Option<String>>, 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]