diff --git a/hive-sh4re/src/manager.rs b/hive-sh4re/src/manager.rs index dcfd72e8..02ef7b44 100644 --- a/hive-sh4re/src/manager.rs +++ b/hive-sh4re/src/manager.rs @@ -131,3 +131,52 @@ pub struct SchedulePromptPayload { #[serde(default, skip_serializing_if = "Option::is_none")] pub description: Option, } + +#[cfg(test)] +mod reserved_name_tests { + use super::{ + CHILDREN_RECIPIENT, MANAGER_AGENT, OPERATOR_RECIPIENT, PARENT_RECIPIENT, SYSTEM_SENDER, + }; + use hive_types::{Ident, is_reserved_name}; + + /// The sentinels declared here and the reserved-name list in + /// `hive-types` are two spellings of one fact, in crates that cannot + /// import each other's intent. This pins them together: adding a + /// sentinel without reserving it now fails here rather than years + /// later, when an agent takes the name. + #[test] + fn ident_shaped_sentinels_are_reserved() { + for sentinel in [OPERATOR_RECIPIENT, SYSTEM_SENDER] { + assert!( + is_reserved_name(sentinel), + "{sentinel:?} is a sentinel an agent could be named — it must be in RESERVED_NAMES" + ); + } + } + + /// The other half, and the reason the test above is not vacuous: these + /// sentinels are *unreachable* as agent names because the charset + /// rejects them, so they are correctly absent from the list. If a + /// charset change ever made one parseable, it would become a real + /// collision and this test is what notices. + #[test] + fn bracketed_recipients_cannot_be_agent_names() { + for sentinel in [PARENT_RECIPIENT, CHILDREN_RECIPIENT] { + assert!( + Ident::parse(sentinel).is_err(), + "{sentinel:?} parses as an ident now — it is reachable as an agent name and must be reserved" + ); + assert!(!is_reserved_name(sentinel)); + } + } + + /// `ruth` is a real agent, not a protocol literal, so it is not + /// reserved: a second agent wanting the name is a *taken* name, which + /// the roster check answers. Recorded as a test so the distinction is + /// enforced rather than remembered. + #[test] + fn manager_name_is_taken_not_reserved() { + assert!(Ident::parse(MANAGER_AGENT).is_ok()); + assert!(!is_reserved_name(MANAGER_AGENT)); + } +} diff --git a/hive-types/src/lib.rs b/hive-types/src/lib.rs index 40d04857..0abb2037 100644 --- a/hive-types/src/lib.rs +++ b/hive-types/src/lib.rs @@ -6,6 +6,62 @@ //! and get serde-validated parsing at the socket boundary for free — with //! no cross-crate coupling and without growing `hive-sh4re`. +/// Names that already mean something to the message layer, and so are not +/// available as an agent name. +/// +/// Every entry is a value some component *produces* as a message `from` or +/// `to`, not a word that merely looked risky. An agent holding one of these +/// is indistinguishable, at the broker, from the thing that normally sends +/// it: a wake from `forge` and a wake from an agent named `forge` are the +/// same row. +/// +/// Deliberately **not** enforced inside [`Ident::parse`]. Parsing runs on +/// every read of an already-created name, so rejecting there would make +/// existing agents unreadable rather than un-creatable, and it would be a +/// refusal — which is a stronger action than the warning this list is +/// currently used for. Creation sites call [`is_reserved_name`]; readers do +/// not. +pub const RESERVED_NAMES: &[&str] = &[ + // The human at the dashboard. Both a broker recipient (the T4LK box + // sends `{from: "operator", to, body}`) and the fallback attribution + // for an answered question. + "operator", + // Helper events (`approval_resolved`, `container_crash`, …) — the + // sender an agent is told to treat as hyperhive itself rather than as + // a peer. Named in `hive_sh4re::manager::SYSTEM_SENDER`. + "system", + // A due self-scheduled reminder arrives as its own sender, so that a + // wake I asked for last week is distinguishable from a peer message. + "reminder", + // Forge notification wakes, delivered by the notify daemon. + "forge", + // A scheduled prompt firing, pushed as a trusted sender. + "scheduled", + // Three synthetic wakes the harness itself produces: an in-container + // todo, the follow-up turn after a self-requested compaction, and the + // single flush turn before a graceful stop. + "todo", + "compact", + "graceful-stop", +]; + +// Two sentinels are deliberately absent. `` and `` are +// routing recipients that `Ident::parse` already rejects on charset, so no +// name can ever equal them — listing them would imply a guard that never +// fires. And `ruth` (the manager) is a real agent, not a literal: a second +// agent wanting that name is a name that is *taken*, which is the roster +// check's job, not this list's. `hive-sh4re` pins both claims in a test. + +/// Whether `name` is already a protocol literal — see [`RESERVED_NAMES`]. +/// +/// Call at **creation** sites only. A caller that is reading or routing an +/// existing name must not consult this: the name is already in use, and the +/// question there is where it goes, not whether it should exist. +#[must_use] +pub fn is_reserved_name(name: &str) -> bool { + RESERVED_NAMES.contains(&name) +} + /// A validated hive identifier: 1-63 chars of `[a-z0-9-]`. /// /// The single ident type for agent names, forge labels, and matrix / github @@ -98,7 +154,7 @@ impl<'de> serde::Deserialize<'de> for Ident { #[cfg(test)] mod ident_tests { - use super::Ident; + use super::{Ident, RESERVED_NAMES, is_reserved_name}; #[test] fn accepts_canonical_shapes() { @@ -135,6 +191,34 @@ mod ident_tests { } } + #[test] + fn reserved_names_are_flagged_and_ordinary_names_are_not() { + // Presence arm: every entry must actually be reported. + for name in RESERVED_NAMES { + assert!(is_reserved_name(name), "{name:?} should be reserved"); + } + // Absence arm, and the reason this test can fail: without it a + // predicate that always returns `true` passes the loop above. + for ok in ["atlas", "damocles", "iris", "operator-2", "sys", "forged"] { + assert!(!is_reserved_name(ok), "{ok:?} must NOT be reserved"); + } + } + + #[test] + fn every_reserved_name_is_a_valid_ident() { + // A reserved name that `Ident::parse` already rejects is dead + // weight — nothing could ever have been created with it, so + // listing it implies a guard that is doing nothing. `graceful-stop` + // is the one that makes this worth asserting: it is hyphenated, and + // a charset tightening would silently retire it. + for name in RESERVED_NAMES { + assert!( + Ident::parse(name).is_ok(), + "{name:?} is reserved but not a parseable ident — one of the two is wrong" + ); + } + } + #[test] fn round_trips_and_serde_validates() { let id = Ident::parse("damocles").unwrap(); diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index 7b7290f1..f7106e0b 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -682,6 +682,20 @@ struct CreateAgentRequest { #[derive(Clone, Debug, Serialize, ToSchema)] struct CreateAgentResponse { node_id: u64, + /// Name collisions that were **allowed through**, phrased for the + /// operator who just chose the name. + /// + /// The creation still happened — this is the "loud warning now, refuse + /// after a grace period" step, so a caller that ignores this field gets + /// exactly today's behaviour. It rides on the response rather than + /// living only in the daemon's log because the person who can still fix + /// the name in one keystroke is the one holding this response, and they + /// are not reading the journal. + /// + /// Empty (and omitted from the JSON) in the normal case, so a client + /// that never looks at it is not handed an empty array to reason about. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + warnings: Vec, } /// Queue the whole agent-creation job graph for `name` — two independent @@ -736,9 +750,42 @@ async fn create_agent( // `state.hives` is the swarm directory loaded from `SWARM_CONTROLLER_HIVES` // at startup and never mutated, so this is a scan of a handful of // entries against a value an operator just chose. + // The name is shaped like an identifier; these two ask whether it is + // *available*. Both WARN rather than refuse: an operator with agents + // already created under a colliding name would otherwise be unable to + // re-run creation at all, so the refusal comes after a grace period, + // once the warning has had time to be seen. + // + // Collected rather than logged-and-dropped — see `CreateAgentResponse`. + let mut warnings = Vec::new(); + if hive_types::is_reserved_name(&agent) { + // A protocol literal: the message layer already produces this name + // as a sender or recipient, so wakes from the component and + // messages from the agent become the same broker row. + let detail = format!( + "agent name {agent:?} is a reserved protocol name — messages from this agent will be \ + indistinguishable from hyperhive's own; this will become an error" + ); + tracing::warn!(agent = %agent, "create_agent: reserved name"); + warnings.push(detail); + } + let hive = hive_types::Ident::parse(&req.hive) .map_err(|reason| error_problem(axum::http::StatusCode::BAD_REQUEST, reason))? .into_string(); + + // The roster is the same source the `hive` field is validated against + // immediately below — one scan, asked of the other field. A hive and an + // agent sharing a name collide in the swarm's own directory keys, which + // is what makes a pipeline and a label vanish with no failed assertion. + if state.hives.iter().any(|h| h.name == agent) { + let detail = format!( + "agent name {agent:?} is also a hive in this swarm — they share the swarm's directory \ + keys; this will become an error" + ); + tracing::warn!(agent = %agent, "create_agent: name collides with a hive"); + warnings.push(detail); + } if !state.hives.iter().any(|h| h.name == hive) { let known: Vec<&str> = state.hives.iter().map(|h| h.name.as_str()).collect(); // Name the hives that *would* work: the caller is an operator who @@ -798,7 +845,10 @@ async fn create_agent( let [id] = ids[..] else { unreachable!("exactly one handle was asked for"); }; - Ok(Json(CreateAgentResponse { node_id: id.get() })) + Ok(Json(CreateAgentResponse { + node_id: id.get(), + warnings, + })) } /// Query params for `GET /api/jobq/graph` — `?states=` narrows to root @@ -1597,3 +1647,33 @@ mod tests { } } } + +#[cfg(test)] +mod create_agent_warning_tests { + use super::CreateAgentResponse; + + /// The normal case must not grow a field. A client that has never + /// heard of warnings should see the response it always saw — that is + /// what makes "warn now, refuse later" a non-breaking first step. + #[test] + fn no_warnings_are_omitted_from_the_json_entirely() { + let json = serde_json::to_string(&CreateAgentResponse { + node_id: 7, + warnings: vec![], + }) + .unwrap(); + assert_eq!(json, r#"{"node_id":7}"#); + } + + /// Presence arm — without it, a `skip_serializing_if` that dropped the + /// field unconditionally would pass the test above. + #[test] + fn warnings_are_serialized_when_present() { + let json = serde_json::to_string(&CreateAgentResponse { + node_id: 7, + warnings: vec!["name is reserved".to_owned()], + }) + .unwrap(); + assert_eq!(json, r#"{"node_id":7,"warnings":["name is reserved"]}"#); + } +}