types: reserve the protocol names an agent must not be called
An agent's name was checked for shape and never for meaning: `Ident::parse` is 1-63 chars of [a-z0-9-] and there was no reserved-name list anywhere in the tree. So an agent could be called `operator`, `forge` or `todo` -- names the message layer already produces as a sender -- and a wake from that component became indistinguishable, at the broker, from a message sent by the agent. Adds `RESERVED_NAMES` + `is_reserved_name` to `hive-types`, the zero-dep leaf both `hive-c0re` and `swarm-controller` already depend on, so neither grows a dependency to use it. Every entry is a value some component actually produces as a message `from`/`to`, taken from `hive-sh4re`'s own sentinel constants rather than guessed: operator, system, reminder, forge, scheduled, todo, compact, graceful-stop. Two sentinels are deliberately absent -- `<parent>` and `<children>` are unreachable as agent names because the charset rejects them, and `ruth` is a real agent, so wanting that name is a name being *taken*, which the roster answers. 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 is used for today. `create_agent` now warns on both halves -- a reserved name, and a name that is also a hive in the roster -- and does not refuse. The warnings ride on `CreateAgentResponse` rather than only the daemon's log, because the person who can still fix the name in one keystroke is holding the response, not reading the journal. `skip_serializing_if` keeps the no-warning JSON byte-identical to before, so this is a non-breaking first step toward refusing later. `hive-sh4re` gains a drift test tying its sentinel constants to the list: two crates that cannot import each other's intent now fail loudly if a sentinel is added without being reserved. Mutation-verified -- forcing the predicate false, forcing it true, and dropping a single entry each turn a different test red.
This commit is contained in:
parent
43ae164d8b
commit
5202e5c5ba
3 changed files with 215 additions and 2 deletions
|
|
@ -131,3 +131,52 @@ pub struct SchedulePromptPayload {
|
|||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub description: Option<String>,
|
||||
}
|
||||
|
||||
#[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));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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. `<parent>` and `<children>` 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();
|
||||
|
|
|
|||
|
|
@ -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<String>,
|
||||
}
|
||||
|
||||
/// 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"]}"#);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue