feat(#3434): agent creation takes the hive it is aimed at
`POST /api/agents` now requires `hive` alongside `name`. It is parsed as an `Ident` like `name` already was, and then checked against the roster loaded from `SWARM_CONTROLLER_HIVES` -- a hive that is not in this swarm is a 400 naming the ones that are, rather than a typo accepted and forgotten. The roster check is what makes the field worth having; without it nothing notices until a deploy message is addressed to a hive that does not exist. `hive` is an address, not an attribute of the agent: it is where a deploy message goes over the queue, so nothing writes it into the agent's config repo. A config naming its own hive would be a second statement of where the agent lives, free to drift from the queue that actually delivers to it. It rides on the `InitAgentConfigRepo` node payload because the graph is the only thing carrying the operator's choice forward from the API boundary; seeding does not consume it. The node that routes on it is the deploy node in #3124. The refusal is asserted by effect -- the test checks that *nothing was queued*, not just the status code, since a version that queued the graph and then complained would satisfy a status-only assertion while still creating the agent. This is a breaking change for every existing caller: the swarm-UI create page posts `{name}` only and needs its hive dropdown to land alongside.
This commit is contained in:
atlas
mara
parent
3efeffd95f
commit
8b55a8b9fd
2 changed files with 183 additions and 10 deletions
|
|
@ -486,6 +486,14 @@ fn base64_encode(content: &str) -> String {
|
|||
/// `hive-c0re::lifecycle::setup::initial_agent_nix` writes at the per-hive
|
||||
/// level (this process has no access to that function across the crate
|
||||
/// boundary, and it's three lines — not worth a shared crate for).
|
||||
///
|
||||
/// Deliberately does **not** record which hive the agent belongs to, even
|
||||
/// though `POST /api/agents` now takes one: an agent's config should carry
|
||||
/// no reference to the hive it runs on. The hive is an address the swarm
|
||||
/// routes on, not a property of the agent — a
|
||||
/// config that named its own hive would be a second place stating where
|
||||
/// the agent lives, free to disagree with the queue that actually
|
||||
/// delivers to it.
|
||||
fn initial_agent_nix(name: &str) -> String {
|
||||
format!(
|
||||
"{{ config, pkgs, lib, ... }}:\n{{\n # Per-agent overrides for {name}. This is a regular NixOS module\n # — add packages, services, modules, imports as needed.\n #\n # imports = [ ./extra-module.nix ];\n # environment.systemPackages = with pkgs; [ ];\n}}\n",
|
||||
|
|
|
|||
|
|
@ -64,7 +64,25 @@ enum SwarmNodeKind {
|
|||
AddRepoMember { repo: String, agent: String },
|
||||
/// Seed `repo` with `agent.nix` + `flake.nix`. See
|
||||
/// `forge::Client::seed_agent_config`.
|
||||
InitAgentConfigRepo { repo: String, agent: String },
|
||||
///
|
||||
/// `hive` is the **address** this agent's creation was aimed at — the
|
||||
/// hive a deploy message gets sent to over the queue — and not a
|
||||
/// property of the agent. Seeding does not use it: it rides on the
|
||||
/// graph because that is the only thing carrying the operator's choice
|
||||
/// forward from the API boundary, where it was checked against the
|
||||
/// roster once. The node that routes on it is the deploy node in the
|
||||
/// deploy-coordination work; until that lands the field is carried and
|
||||
/// rendered, not acted on.
|
||||
///
|
||||
/// Carrying the validated value rather than re-reading the roster at
|
||||
/// node time is deliberate — the roster is loaded at startup, and a
|
||||
/// node that looked it up again would be answering a question the
|
||||
/// operator already answered.
|
||||
InitAgentConfigRepo {
|
||||
repo: String,
|
||||
agent: String,
|
||||
hive: String,
|
||||
},
|
||||
}
|
||||
|
||||
impl hive_jobq_wire::WireNode for SwarmNodeKind {
|
||||
|
|
@ -85,10 +103,16 @@ impl hive_jobq_wire::WireNode for SwarmNodeKind {
|
|||
SwarmNodeKind::CreateRepo { repo } => {
|
||||
serde_json::json!({ "repo": repo })
|
||||
}
|
||||
SwarmNodeKind::AddRepoMember { repo, agent }
|
||||
| SwarmNodeKind::InitAgentConfigRepo { repo, agent } => {
|
||||
SwarmNodeKind::AddRepoMember { repo, agent } => {
|
||||
serde_json::json!({ "repo": repo, "agent": agent })
|
||||
}
|
||||
// Rendered with `hive` because it is the one place the graph
|
||||
// states where this creation was aimed — nothing persists it,
|
||||
// so a viewer watching the job is the only reader there is
|
||||
// until the deploy node routes on it.
|
||||
SwarmNodeKind::InitAgentConfigRepo { repo, agent, hive } => {
|
||||
serde_json::json!({ "repo": repo, "agent": agent, "hive": hive })
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -167,7 +191,10 @@ async fn run_swarm_node(
|
|||
Err(e) => Outcome::Failed(format!("{e:#}")),
|
||||
},
|
||||
},
|
||||
SwarmNodeKind::InitAgentConfigRepo { repo, agent } => match deps.forge {
|
||||
// `hive` is deliberately not destructured: seeding writes no hive
|
||||
// into the agent's config, so this node carries the address without
|
||||
// consuming it. See the variant's doc.
|
||||
SwarmNodeKind::InitAgentConfigRepo { repo, agent, .. } => match deps.forge {
|
||||
None => Outcome::Failed(
|
||||
"no forge configured on this host (SWARM_CONTROLLER_FORGE_URL / \
|
||||
SWARM_CONTROLLER_FORGE_TOKEN_FILE unset)"
|
||||
|
|
@ -547,13 +574,27 @@ async fn get_hives_status(
|
|||
}
|
||||
}
|
||||
|
||||
/// Body of `POST /api/agents` — the agent name to create. The repo name
|
||||
/// inside `forge::AGENTS_ORG` is the same string: one repo per agent,
|
||||
/// named after it, same convention `hive-c0re::forge` already uses for its
|
||||
/// own single-hive `CreateRepo` path.
|
||||
/// Body of `POST /api/agents` — the agent name to create, and the hive the
|
||||
/// creation is aimed at. The repo name inside `forge::AGENTS_ORG` is the
|
||||
/// same string as `name`: one repo per agent, named after it, same
|
||||
/// convention `hive-c0re::forge` already uses for its own single-hive
|
||||
/// `CreateRepo` path.
|
||||
///
|
||||
/// `hive` is an **address**, not an attribute of the agent: it is where a
|
||||
/// deploy message goes over the queue. Nothing here writes it into the
|
||||
/// agent's config — a config naming its own hive would be a second
|
||||
/// statement of where the agent lives, free to drift from the queue that
|
||||
/// actually delivers to it.
|
||||
///
|
||||
/// It is required rather than optional because it is only knowable here,
|
||||
/// at creation, from the operator making the choice. An optional field
|
||||
/// would have been the friendlier migration and would have left every
|
||||
/// creation in the state this endpoint exists to avoid — one nothing can
|
||||
/// address.
|
||||
#[derive(Clone, Debug, Deserialize, ToSchema)]
|
||||
struct CreateAgentRequest {
|
||||
name: String,
|
||||
hive: String,
|
||||
}
|
||||
|
||||
/// Where the queued job landed — a caller polls `/api/jobq/graph` (or
|
||||
|
|
@ -597,7 +638,7 @@ struct CreateAgentResponse {
|
|||
request_body = CreateAgentRequest,
|
||||
responses(
|
||||
(status = 200, description = "job chain queued", body = CreateAgentResponse),
|
||||
(status = 400, description = "`name` is not a valid identifier (problem+json)", body = String),
|
||||
(status = 400, description = "`name` or `hive` is not a valid identifier, or `hive` is not in this swarm (problem+json)", body = String),
|
||||
(status = 500, description = "the job chain could not be queued (problem+json)", body = String),
|
||||
),
|
||||
tag = "agents"
|
||||
|
|
@ -611,6 +652,33 @@ async fn create_agent(
|
|||
.into_string();
|
||||
let repo = agent.clone();
|
||||
|
||||
// Two checks, and the second is the one that makes the field worth
|
||||
// having: `Ident::parse` says the string is *shaped* like a hive name,
|
||||
// and the roster says it *is* one. Without the roster check a typo is
|
||||
// accepted, the agent gets created, and nothing notices until a deploy
|
||||
// message is addressed to a hive that does not exist — by which point
|
||||
// the operator who could have corrected it in one keystroke is gone.
|
||||
//
|
||||
// `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.
|
||||
let hive = hive_types::Ident::parse(&req.hive)
|
||||
.map_err(|reason| error_problem(axum::http::StatusCode::BAD_REQUEST, reason))?
|
||||
.into_string();
|
||||
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
|
||||
// just picked one, and "not in this swarm" without the roster
|
||||
// leaves them guessing at a typo they cannot see.
|
||||
let known = if known.is_empty() {
|
||||
"(none configured)".to_owned()
|
||||
} else {
|
||||
known.join(", ")
|
||||
};
|
||||
let detail = format!("hive {hive:?} is not in this swarm — known hives: {known}");
|
||||
return Err(error_problem(axum::http::StatusCode::BAD_REQUEST, &detail));
|
||||
}
|
||||
|
||||
let mut sched = state
|
||||
.jobq
|
||||
.lock()
|
||||
|
|
@ -633,7 +701,7 @@ async fn create_agent(
|
|||
})
|
||||
.after_ok(create_repo);
|
||||
let _init_config = b
|
||||
.node(SwarmNodeKind::InitAgentConfigRepo { repo, agent })
|
||||
.node(SwarmNodeKind::InitAgentConfigRepo { repo, agent, hive })
|
||||
.after_ok(create_repo);
|
||||
vec![create_identity.guid()]
|
||||
})
|
||||
|
|
@ -976,6 +1044,103 @@ mod tests {
|
|||
/// SAFETY: single-threaded mutation of the two `forge` env vars this
|
||||
/// test itself owns, restored before returning — no other test in this
|
||||
/// crate reads them.
|
||||
/// One roster, one agent name, two hives — the accepted one and a
|
||||
/// typo. Built per test so neither can see the other's queue.
|
||||
/// The scheduler handle these tests hold onto so they can assert on
|
||||
/// what the endpoint queued. Aliased because the full type is three
|
||||
/// nested generics deep and reads worse inline than named.
|
||||
type SharedSched = std::sync::Arc<
|
||||
std::sync::Mutex<
|
||||
hive_jobq::scheduler::Scheduler<super::SwarmNodeKind, super::SwarmResourceKind>,
|
||||
>,
|
||||
>;
|
||||
|
||||
fn state_with_roster() -> (super::AppState, SharedSched) {
|
||||
let sched =
|
||||
std::sync::Arc::new(std::sync::Mutex::new(hive_jobq::scheduler::Scheduler::new(
|
||||
hive_jobq::Graph::new(),
|
||||
hive_jobq::resources::ResourceTable::new(),
|
||||
)));
|
||||
let state = super::AppState {
|
||||
hives: std::sync::Arc::new(vec![HiveEntry {
|
||||
name: "pr1ma".to_owned(),
|
||||
domain: "pr1ma.example".to_owned(),
|
||||
}]),
|
||||
links: std::sync::Arc::new(Vec::new()),
|
||||
status: None,
|
||||
jobq: std::sync::Arc::clone(&sched),
|
||||
webhook_secret: None,
|
||||
};
|
||||
(state, sched)
|
||||
}
|
||||
|
||||
/// The roster check is the half that makes the recorded hive worth
|
||||
/// having, so assert it by EFFECT rather than by the message: a hive
|
||||
/// that is not in this swarm must be refused **before anything is
|
||||
/// queued**. A version that queued the graph and then complained would
|
||||
/// satisfy an assertion on the status code alone while still creating
|
||||
/// the agent — which is the failure this exists to stop.
|
||||
#[tokio::test]
|
||||
async fn a_hive_outside_the_roster_is_refused_before_anything_is_queued() {
|
||||
let (state, sched) = state_with_roster();
|
||||
|
||||
let err = super::create_agent(
|
||||
axum::extract::State(state),
|
||||
axum::Json(super::CreateAgentRequest {
|
||||
name: "atlas".to_owned(),
|
||||
hive: "pr1maa".to_owned(),
|
||||
}),
|
||||
)
|
||||
.await
|
||||
.expect_err("a hive outside the roster must be refused");
|
||||
|
||||
// Names what would have worked: an operator who just mistyped a
|
||||
// hive cannot see the roster from the error otherwise.
|
||||
let rendered = format!("{err:?}");
|
||||
assert!(
|
||||
rendered.contains("pr1ma"),
|
||||
"the refusal should name the known hives, got: {rendered}"
|
||||
);
|
||||
|
||||
let queued = sched
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.graph()
|
||||
.nodes()
|
||||
.count();
|
||||
assert_eq!(queued, 0, "a refused creation must queue no work");
|
||||
}
|
||||
|
||||
/// The control for the arm above: the same call with a hive that IS in
|
||||
/// the roster gets through and queues the graph. Without this, "refused"
|
||||
/// could equally mean the endpoint refuses everything.
|
||||
#[tokio::test]
|
||||
async fn a_hive_in_the_roster_is_accepted_and_queues_the_graph() {
|
||||
let (state, sched) = state_with_roster();
|
||||
|
||||
// The response is bound rather than asserted on: node ids start at
|
||||
// zero, so every property I reached for first ("> 0") was a claim
|
||||
// about the id allocator rather than about this endpoint. What the
|
||||
// accept arm is actually for is the queue count below.
|
||||
let _queued = super::create_agent(
|
||||
axum::extract::State(state),
|
||||
axum::Json(super::CreateAgentRequest {
|
||||
name: "atlas".to_owned(),
|
||||
hive: "pr1ma".to_owned(),
|
||||
}),
|
||||
)
|
||||
.await
|
||||
.expect("a hive in the roster must be accepted");
|
||||
|
||||
let queued = sched
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.graph()
|
||||
.nodes()
|
||||
.count();
|
||||
assert!(queued > 0, "an accepted creation must queue work");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn create_repo_node_runs_end_to_end_and_fails_without_forge_configured() {
|
||||
unsafe {
|
||||
|
|
|
|||
Loading…
Reference in a new issue