diff --git a/hive-agent-mcp/src/mcp/render.rs b/hive-agent-mcp/src/mcp/render.rs index 435ef834..66a09d61 100644 --- a/hive-agent-mcp/src/mcp/render.rs +++ b/hive-agent-mcp/src/mcp/render.rs @@ -33,6 +33,18 @@ pub fn format_ack( ) -> String { match resp { Ok(hive_core_agent_sock::Response::Ok) => ok_msg, + // Succeeded, with something the caller needs to read. Rendered + // after the success line rather than instead of it: the operation + // DID happen, and a warning shown as though it were a failure + // invites a retry that would only queue a second one. + Ok(hive_core_agent_sock::Response::OkWarn { warnings }) => { + let mut out = ok_msg; + for w in &warnings { + out.push_str("\n⚠️ "); + out.push_str(w); + } + out + } other => reply_err(other, tool), } } @@ -598,7 +610,7 @@ pub fn annotate_retries(mut s: String, retries: u32) -> String { #[cfg(test)] mod tests { - use super::format_recv; + use super::{format_ack, format_recv}; fn msg(id: i64, from: &str, body: &str) -> hive_sh4re::inbox::DeliveredMessage { hive_sh4re::inbox::DeliveredMessage { @@ -651,4 +663,33 @@ mod tests { let batch = 9u64.min(u64::from(hive_sh4re::inbox::RECV_BATCH_MAX)); assert!(out.contains(&format!("max: {batch}"))); } + + #[test] + fn ok_warn_keeps_the_success_line_and_appends_each_warning() { + let out = format_ack( + Ok(hive_core_agent_sock::Response::OkWarn { + warnings: vec!["name is reserved".to_owned(), "second thing".to_owned()], + }), + "request_init_config", + "init_config approval queued for forge".to_owned(), + ); + // The operation HAPPENED — dropping the success line would read as a + // failure and invite a retry that queues a second approval. + assert!(out.starts_with("init_config approval queued for forge")); + assert!(out.contains("⚠️ name is reserved")); + assert!(out.contains("⚠️ second thing")); + } + + #[test] + fn plain_ok_is_untouched_by_the_warning_path() { + // Absence arm: without it, a renderer that always appended a + // warning marker would pass the test above. + let out = format_ack( + Ok(hive_core_agent_sock::Response::Ok), + "request_init_config", + "queued".to_owned(), + ); + assert_eq!(out, "queued"); + assert!(!out.contains('⚠')); + } } diff --git a/hive-c0re/src/socket_server/config_approvals.rs b/hive-c0re/src/socket_server/config_approvals.rs index 83a2856b..abb28b64 100644 --- a/hive-c0re/src/socket_server/config_approvals.rs +++ b/hive-c0re/src/socket_server/config_approvals.rs @@ -29,8 +29,46 @@ pub(super) fn handle_request_init_config( return err; } tracing::info!(%agent, %name, "request_init_config"); + // Warn, do not refuse: an agent already created under a colliding + // name must stay re-initialisable, so the refusal comes later, once + // the warning has had time to be seen. + // + // Checked HERE and not only in `swarm-controller::create_agent`: + // that daemon is opt-in and off on most hives, while this is the + // path the `request_init_config` tool takes on every hive. Guarding + // only the rarer one would have left the common flow exactly as + // unguarded as before. + // + // The blacklist itself comes from nix via `HIVE_RESERVED_NAMES`, so it + // stays a config change rather than a rebuild. An UNSET variable means + // this daemon was never told — which is not the same as "no name is + // reserved", and saying nothing there would be a check that reports + // clean because it could not run. + let raw = hive_types::reserved_names_raw(); + let warnings = match raw.as_deref().map(hive_types::parse_reserved_names) { + None => { + tracing::error!( + var = hive_types::RESERVED_NAMES_ENV, + "request_init_config: reserved-name check could not run — variable not set" + ); + vec![format!( + "the reserved-name check did not run: {} is unset, so {name:?} was accepted \ + without being checked against the protocol literals", + hive_types::RESERVED_NAMES_ENV + )] + } + Some(reserved) if hive_types::is_reserved_name(name, &reserved) => { + tracing::warn!(%agent, %name, "request_init_config: reserved name"); + vec![format!( + "agent name {name:?} is a reserved protocol name — messages from this agent will \ + be indistinguishable from hyperhive's own; this will become an error" + )] + } + Some(_) => Vec::new(), + }; match submit_init_config(coord, name, Some(agent), description) { - Ok(_id) => Response::Ok, + Ok(_id) if warnings.is_empty() => Response::Ok, + Ok(_id) => Response::OkWarn { warnings }, Err(e) => Response::Err { message: format!("{e:#}"), }, diff --git a/hive-core-agent-sock/src/lib.rs b/hive-core-agent-sock/src/lib.rs index 14c59c92..0d983dd8 100644 --- a/hive-core-agent-sock/src/lib.rs +++ b/hive-core-agent-sock/src/lib.rs @@ -245,6 +245,19 @@ pub enum Request { pub enum Response { /// `Send` succeeded. Ok, + /// The operation succeeded **and** raised advisories the caller + /// should see. + /// + /// Distinct from `Err` on purpose: a check that WARNS rather than + /// refuses has no other way to reach the caller. Folding it into + /// `Err` would refuse; folding it into `Ok` would drop it, leaving + /// the daemon's log as the only record — and the person who can act + /// on the advisory is the one holding this response, not the one + /// reading the journal. + /// + /// Additive: every existing site returning `Ok` keeps returning it, + /// so a caller that never matches this variant is unaffected. + OkWarn { warnings: Vec }, /// Either `Send` failed or `Recv` errored. Err { message: String }, /// `Recv` result: zero or more messages, FIFO-ordered, never diff --git a/hive-sh4re/src/manager.rs b/hive-sh4re/src/manager.rs index dcfd72e8..a5cdf017 100644 --- a/hive-sh4re/src/manager.rs +++ b/hive-sh4re/src/manager.rs @@ -131,3 +131,105 @@ 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, RESERVED_NAMES_ENV, is_reserved_name}; + + /// The blacklist as nix rendered it for this test run. + /// + /// **Panics when the variable is absent, deliberately.** The list lives + /// in `nix/reserved-names.nix` now, so this test can no longer read it + /// from the Rust tree; `nix/checks.nix` and `nix/devshell.nix` both + /// export it. Skipping instead would turn "nobody wired the variable" + /// into a green run — the same shape as a checker that reports clean + /// because it crashed, and the whole point of a drift test is that it + /// is the thing that notices. + fn reserved() -> Vec { + let raw = hive_types::reserved_names_raw().unwrap_or_else(|| { + panic!( + "{RESERVED_NAMES_ENV} is unset or blank, so the drift test cannot run. \ + nix/checks.nix and nix/devshell.nix are supposed to export it from \ + nix/reserved-names.nix — fix the plumbing rather than this test." + ) + }); + hive_types::parse_reserved_names(&raw) + .into_iter() + .map(str::to_owned) + .collect() + } + + /// The sentinels declared here and the blacklist nix owns are two + /// spellings of one fact, in places 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() { + let owned = reserved(); + let reserved: Vec<&str> = owned.iter().map(String::as_str).collect(); + for sentinel in [OPERATOR_RECIPIENT, SYSTEM_SENDER] { + assert!( + is_reserved_name(sentinel, &reserved), + "{sentinel:?} is a sentinel an agent could be named — it must be in \ + nix/reserved-names.nix" + ); + } + } + + /// Every entry nix hands us must be a name an agent could actually have + /// been given. One that `Ident::parse` rejects is dead weight — nothing + /// could ever have been created with it, so listing it implies a guard + /// doing nothing. `graceful-stop` is what makes this worth asserting: it + /// is hyphenated, and a charset tightening would silently retire it. + /// + /// This assertion used to live beside the list in `hive-types`; it moved + /// here because here is where the real list is readable. + #[test] + fn every_reserved_name_is_a_valid_ident() { + let owned = reserved(); + assert!( + !owned.is_empty(), + "the blacklist rendered empty — an empty list makes every assertion below vacuous" + ); + for name in &owned { + assert!( + Ident::parse(name).is_ok(), + "{name:?} is reserved but not a parseable ident — one of the two is wrong" + ); + } + } + + /// 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() { + let owned = reserved(); + let reserved: Vec<&str> = owned.iter().map(String::as_str).collect(); + 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, &reserved)); + } + } + + /// `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() { + let owned = reserved(); + let reserved: Vec<&str> = owned.iter().map(String::as_str).collect(); + assert!(Ident::parse(MANAGER_AGENT).is_ok()); + assert!(!is_reserved_name(MANAGER_AGENT, &reserved)); + } +} diff --git a/hive-types/src/lib.rs b/hive-types/src/lib.rs index 40d04857..d1a51508 100644 --- a/hive-types/src/lib.rs +++ b/hive-types/src/lib.rs @@ -6,6 +6,64 @@ //! and get serde-validated parsing at the socket boundary for free — with //! no cross-crate coupling and without growing `hive-sh4re`. +/// The environment variable nix uses to hand this process the blacklist of +/// names that already mean something to the message layer. +/// +/// **Nix owns the list**, not this crate: `nix/reserved-names.nix` is the one +/// copy, and it reaches `hive-c0re`, `swarm-controller`, the swarm collector's +/// own assertion and the test suite from that single file. Keeping the +/// blacklist current is therefore a config change, not a rebuild of a binary +/// — and there is no second list to drift. +/// +/// 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. +/// +/// ⚠️ An **unset** variable means *this process was not told*, which is not +/// the same as *nothing is reserved*. Callers must say so out loud rather +/// than silently treating every name as available. +pub const RESERVED_NAMES_ENV: &str = "HIVE_RESERVED_NAMES"; + +/// Split the value of [`RESERVED_NAMES_ENV`] into names. +/// +/// Whitespace-separated, not JSON, and that is a deliberate departure from +/// the structured env vars elsewhere in the tree. Every entry is an [`Ident`], +/// whose charset is `[a-z0-9-]` — so whitespace cannot occur *inside* a name +/// and the encoding is provably lossless. Paying for a JSON parser here would +/// mean either a dependency in a crate whose entire purpose is to have none, +/// or a separate copy of the parse in every consumer. +#[must_use] +pub fn parse_reserved_names(raw: &str) -> Vec<&str> { + raw.split_whitespace().collect() +} + +/// The raw value of [`RESERVED_NAMES_ENV`], or `None` when this process was +/// never told what the blacklist is. +/// +/// A **blank** value folds into `None` on purpose. Nix always renders a +/// non-empty list, so a variable that is present but empty is a rendering +/// fault, not an operator declaring that nothing is reserved — and the two +/// must not look the same to a caller whose next move is to warn about it. +#[must_use] +pub fn reserved_names_raw() -> Option { + std::env::var(RESERVED_NAMES_ENV) + .ok() + .filter(|raw| !raw.trim().is_empty()) +} + +/// Whether `name` is one of the protocol literals in `reserved`. +/// +/// 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, reserved: &[&str]) -> bool { + reserved.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 +156,7 @@ impl<'de> serde::Deserialize<'de> for Ident { #[cfg(test)] mod ident_tests { - use super::Ident; + use super::{Ident, is_reserved_name, parse_reserved_names}; #[test] fn accepts_canonical_shapes() { @@ -135,6 +193,53 @@ mod ident_tests { } } + #[test] + fn reserved_names_are_flagged_and_ordinary_names_are_not() { + // A sample, not the real list: the real one lives in nix now, and + // what this crate still owns is the *predicate*. The content of the + // blacklist is asserted where the env var is readable — see + // `hive-sh4re`'s drift test. + let reserved = ["operator", "system", "graceful-stop"]; + // Presence arm: every entry must actually be reported. + for name in reserved { + assert!(is_reserved_name(name, &reserved), "{name:?} not reported"); + } + // 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, &reserved), "{ok:?} must NOT be"); + } + // An empty list reserves nothing — the shape a caller gets when the + // env var is unset, and the reason a caller must not treat that + // case as "nothing is reserved" without saying so. + assert!(!is_reserved_name("operator", &[])); + } + + #[test] + fn parses_the_env_encoding() { + assert_eq!( + parse_reserved_names("operator system graceful-stop"), + ["operator", "system", "graceful-stop"] + ); + // Newlines and runs of spaces are what a nix-rendered list looks + // like when someone reformats the file; both must fold away. + assert_eq!( + parse_reserved_names(" operator\n system \n"), + ["operator", "system"] + ); + // Absence and emptiness collapse to the same empty list, which is + // why the CALLER, not this function, has to distinguish them. + assert!(parse_reserved_names("").is_empty()); + // Every name the encoding can carry must survive a round trip + // through `Ident::parse`: a blacklist entry that is not a legal + // ident is dead weight, since nothing could ever be created with + // it. `graceful-stop` is the one that makes this worth asserting — + // it is hyphenated, and a charset tightening would retire it. + for name in parse_reserved_names("operator system todo graceful-stop") { + assert!(Ident::parse(name).is_ok(), "{name:?} is not an ident"); + } + } + #[test] fn round_trips_and_serde_validates() { let id = Ident::parse("damocles").unwrap(); diff --git a/nix/checks.nix b/nix/checks.nix index e9e01878..0944d240 100644 --- a/nix/checks.nix +++ b/nix/checks.nix @@ -62,6 +62,14 @@ in version = "0.1.0"; cargoTestExtraArgs = "--workspace"; HIVE_ASSETS_DIR = "${self.packages.${system}.assets}/share/hyperhive"; + # The reserved-name blacklist moved out of the Rust tree and into + # `reserved-names.nix`, so the test that pins the message layer's + # sentinels against it can only run if nix hands it the same list the + # daemons get. Exported here and in `devshell.nix`, and the test PANICS + # rather than skipping when the variable is missing: a drift test that + # quietly does nothing is worse than no drift test, because it still + # shows up green. + HIVE_RESERVED_NAMES = pkgs.lib.concatStringsSep " " (import ./reserved-names.nix); }; # Rustdoc gate. Builds the workspace's docs and turns rustdoc's own diff --git a/nix/devshell.nix b/nix/devshell.nix index 36c07af8..0d3400aa 100644 --- a/nix/devshell.nix +++ b/nix/devshell.nix @@ -5,6 +5,12 @@ { pkgs, rust }: { default = pkgs.mkShell { + # Same list the daemons are handed and the same one `checks.nix` gives + # the test derivation — `nix/reserved-names.nix`. Without it here, a + # plain `cargo test` in the shell would panic on the drift test, so a + # developer's local run and CI would disagree about a test that exists + # precisely to stop two spellings of one fact from drifting. + HIVE_RESERVED_NAMES = pkgs.lib.concatStringsSep " " (import ./reserved-names.nix); packages = rust.nativeBuildInputs ++ (with pkgs; [ diff --git a/nix/host-modules/hive-c0re/environment.nix b/nix/host-modules/hive-c0re/environment.nix index 92b612e8..c1df0c6e 100644 --- a/nix/host-modules/hive-c0re/environment.nix +++ b/nix/host-modules/hive-c0re/environment.nix @@ -143,6 +143,21 @@ in # mandatory, so this is unconditional (the whole env block is already # gated on hyperhive being enabled). See `docs/gateway.md::HIVE_FORGE_URL`. HIVE_FORGE_URL = "http://${config.services.hyperhive.swarm.forge.domain}"; + + # The one blacklist of names an agent may not take — see + # `nix/reserved-names.nix`, which is also read by the swarm controller, by + # the swarm collector's owner assertion, and by the test suite. Nix owns it + # so that keeping it current is a config change, not a rebuild of a binary. + # + # Whitespace-separated rather than JSON: every entry is an `Ident` + # (`[a-z0-9-]`), so a space can never occur inside a name and the encoding + # cannot be lossy. Spelling it as JSON would put a parser in the crate whose + # whole point is to have no dependencies. + # + # Unconditional on purpose. The consumer treats an ABSENT variable as "I was + # never told" and says so out loud, which is the correct reading — but it is + # a reading no correctly-built hive should ever have to make. + HIVE_RESERVED_NAMES = lib.concatStringsSep " " (import ../../reserved-names.nix); } // lib.optionalAttrs diff --git a/nix/host-modules/swarm-controller.nix b/nix/host-modules/swarm-controller.nix index 8a94855f..5ba54251 100644 --- a/nix/host-modules/swarm-controller.nix +++ b/nix/host-modules/swarm-controller.nix @@ -648,6 +648,13 @@ in # `swarm.peerHives`, `swarm.hives` minus this hive) rather than # peers-minus-self. Consumed by `GET /api/hives` # (swarm-controller/src/main.rs::load_hives). + # The one blacklist of names an agent may not take, shared verbatim + # with hive-c0re and with the collector's owner assertion — see + # `nix/reserved-names.nix`. Whitespace-separated rather than JSON + # like its neighbour below, because every entry is an `Ident` and so + # cannot contain a space; the neighbour carries objects and has no + # such option. + HIVE_RESERVED_NAMES = lib.concatStringsSep " " (import ../reserved-names.nix); SWARM_CONTROLLER_HIVES = builtins.toJSON ( lib.mapAttrsToList (name: h: { inherit name; diff --git a/nix/host-modules/swarm-otel.nix b/nix/host-modules/swarm-otel.nix index 4408ab49..9d5ebd00 100644 --- a/nix/host-modules/swarm-otel.nix +++ b/nix/host-modules/swarm-otel.nix @@ -55,9 +55,16 @@ let # repeated at each site would let the guard and the config drift apart, which # is the failure this guard exists to prevent. swarmTierName = "swarm"; - # Every `` this module claims for itself. One entry today; a second - # swarm-tier pipeline would be added here and inherit the check for free. - reservedOwners = [ swarmTierName ]; + # Every `` no hive may take. Read from `nix/reserved-names.nix`, the + # same file the daemons are handed as `HIVE_RESERVED_NAMES`, because agent + # names and hive names are ONE namespace going forward — a locally-owned + # list here would be a second copy to keep in step, which is the failure a + # single blacklist exists to prevent. + # + # The assertion below still checks the string this module emits: the file + # is asserted to CONTAIN `swarmTierName`, so a rename that dropped it from + # the file would be an eval error rather than a silently missing guard. + reservedOwners = import ../reserved-names.nix; # A published target is declared as ONE url, because that url is also the # audience its token is minted for — but prometheus wants the same fact in @@ -581,6 +588,23 @@ in List the swarm's hives. ''; } + { + # The blacklist now lives in a shared file, so this module no longer + # controls its contents — and a guard whose subject can be edited + # elsewhere has to assert that its own case is still in there. Without + # this, deleting one line from `reserved-names.nix` would silently + # retire the check below rather than fail anything. + assertion = lib.elem swarmTierName reservedOwners; + message = '' + nix/reserved-names.nix no longer contains '${swarmTierName}', which + the swarm collector needs reserved: it names components + `/` and uses the hive name as the owner, so a hive + called '${swarmTierName}' would replace the swarm tier's own + pipelines and lose its own. + + Put it back, or give this module a different swarmTierName. + ''; + } { # A hive whose name is one this module claims for itself collides in # the collector's component namespace, and `//` resolves it silently: diff --git a/nix/reserved-names.nix b/nix/reserved-names.nix new file mode 100644 index 00000000..f7fbb6b2 --- /dev/null +++ b/nix/reserved-names.nix @@ -0,0 +1,67 @@ +# The one blacklist: names no agent and no hive may take. +# +# Nix owns this list and hands it to the Rust side as an environment variable +# (`HIVE_RESERVED_NAMES`), so keeping it current is a config change rather than +# a rebuild of a binary. Read by: +# +# - `host-modules/hive-c0re/environment.nix` -> the env var, for the +# `request_init_config` path every hive uses +# - `host-modules/swarm-controller.nix` -> the same var, for the +# swarm-level `create_agent` path +# - `host-modules/swarm-otel.nix` -> its `` assertion, so +# a hive name and an agent name are checked against ONE list +# - `nix/checks.nix` -> exported into `cargo test`, +# which is what keeps the message layer's sentinels from drifting away +# from this file +# +# A plain nix file rather than a module option because two of those readers are +# flake-level (`checks.nix`) and cannot see a NixOS option. +# +# ⚠️ Agent names and hive names are ONE namespace going forward (mara, 2026-08-27: +# "agent and hive names live in the swarm level going forward"). Adding a name +# here forbids it for both. That is the point: two lists is how they drift. +# +# ⚠️ Every entry must be a value some component actually PRODUCES as a message +# `from`/`to`, or a component name the collector builds pipelines from — not a +# word that merely looked risky. A name in here that nothing emits is a refusal +# with no failure behind it. +[ + # ---- message-layer senders ------------------------------------------- + # The human at the dashboard: a broker recipient (the T4LK box sends + # `{from: "operator", ...}`) 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. + # `hive_sh4re::manager::SYSTEM_SENDER`. + "system" + # A due self-scheduled reminder arrives as its own sender, so 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" + + # ---- swarm-tier component owners ------------------------------------- + # The swarm collector names its components `/` and uses the hive + # name as the owner, so a hive called `swarm` would silently replace the + # swarm tier's own pipelines and lose its own — it would keep accepting + # pushes into a pipeline that routes nowhere. Previously enforced only + # against hive names, in `swarm-otel.nix`'s own `reservedOwners`. + "swarm" +] +# Deliberately absent, and both are load-bearing omissions: +# +# `` / `` — routing recipients the ident charset already +# rejects, so no name can ever equal them; listing them would imply a guard +# that never fires. +# +# `ruth` — a real agent, not a literal. A second agent wanting that name is a +# name that is TAKEN, which the roster answers, not this list. diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index 7b7290f1..bad9edfe 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,61 @@ 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(); + // The blacklist comes from nix via `HIVE_RESERVED_NAMES` — one file, read + // by this daemon, by hive-c0re and by the swarm collector's own owner + // assertion. An UNSET variable means this process was never told, which + // is not the same as "no name is reserved": staying quiet there would be + // a check that reports clean because it could not run. + let raw = hive_types::reserved_names_raw(); + match raw.as_deref().map(hive_types::parse_reserved_names) { + None => { + tracing::error!( + var = hive_types::RESERVED_NAMES_ENV, + "create_agent: reserved-name check could not run — variable not set" + ); + warnings.push(format!( + "the reserved-name check did not run: {} is unset, so {agent:?} was accepted \ + without being checked against the protocol literals", + hive_types::RESERVED_NAMES_ENV + )); + } + Some(reserved) if hive_types::is_reserved_name(&agent, &reserved) => { + // 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. + tracing::warn!(agent = %agent, "create_agent: reserved name"); + warnings.push(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" + )); + } + Some(_) => {} + } + 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 +864,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 +1666,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"]}"#); + } +}