diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index d1bcf646..7b726dfe 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -31,6 +31,11 @@ use crate::webhook::DeliveryKind; /// The forge org that owns agent repos. Same org `hive-c0re::forge` /// already uses for its own single-hive `CreateRepo` path — this is /// the same forge instance, not a separate one, so the same org. +/// +/// There is no per-agent repo-vs-agent naming convention to maintain here: +/// agent names are unique and every agent's repo lives in this one org, so +/// the agent name IS the repo name at every call site (dropped the +/// `agent_repo` identity function this used to go through, per review). pub const AGENTS_ORG: &str = "agents"; /// The `operators` team, whitelisted for the merge gate on every repo @@ -486,6 +491,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", diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index 090194e9..3c39cf79 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -56,15 +56,21 @@ mod webhook; enum SwarmNodeKind { /// Ensure `agent` exists as an authelia subject at the swarm level. CreateIdentity { agent: String }, - /// Create `repo` in `forge::AGENTS_ORG` with the operator merge gate - /// on its default branch. See `forge::Client::create_repo`. - CreateRepo { repo: String }, - /// Add `agent` as a write collaborator on `repo`. See + /// Create the agent's repo in `forge::AGENTS_ORG` with the operator + /// merge gate on its default branch. See `forge::Client::create_repo`. + CreateRepo { agent: String }, + /// Add `agent` as a write collaborator on its own repo. See /// `forge::Client::add_repo_member`. - AddRepoMember { repo: String, agent: String }, - /// Seed `repo` with `agent.nix` + `flake.nix`. See + AddRepoMember { agent: String }, + /// Seed the agent's repo with `agent.nix` + `flake.nix`. See /// `forge::Client::seed_agent_config`. - InitAgentConfigRepo { repo: String, agent: String }, + /// + /// Deliberately carries no hive: seeding a config repo is the same + /// work whichever hive the agent is bound for, and an agent's config + /// states nothing about where it runs. The hive is an address the + /// swarm routes a deploy message to — it belongs on the node that + /// sends that message, not on this one. + InitAgentConfigRepo { agent: String }, } impl hive_jobq_wire::WireNode for SwarmNodeKind { @@ -78,17 +84,18 @@ impl hive_jobq_wire::WireNode for SwarmNodeKind { } fn data(&self, _id: hive_jobq_wire::WireId) -> serde_json::Value { + // Every node in this graph is about exactly one agent, so they all + // render the same and `label()` is what distinguishes them. Spelled + // out as an or-pattern rather than a catch-all on purpose: a fifth + // variant then fails to compile here instead of silently rendering + // as an agent name. match self { - SwarmNodeKind::CreateIdentity { agent } => { + SwarmNodeKind::CreateIdentity { agent } + | SwarmNodeKind::CreateRepo { agent } + | SwarmNodeKind::AddRepoMember { agent } + | SwarmNodeKind::InitAgentConfigRepo { agent } => { serde_json::json!({ "agent": agent }) } - SwarmNodeKind::CreateRepo { repo } => { - serde_json::json!({ "repo": repo }) - } - SwarmNodeKind::AddRepoMember { repo, agent } - | SwarmNodeKind::InitAgentConfigRepo { repo, agent } => { - serde_json::json!({ "repo": repo, "agent": agent }) - } } } } @@ -142,13 +149,13 @@ async fn run_swarm_node( Err(e) => Outcome::Failed(format!("{e:#}")), }, }, - SwarmNodeKind::CreateRepo { repo } => match deps.forge { + SwarmNodeKind::CreateRepo { agent } => match deps.forge { None => Outcome::Failed( "no forge configured on this host (SWARM_CONTROLLER_FORGE_URL / \ SWARM_CONTROLLER_FORGE_TOKEN_FILE unset)" .to_owned(), ), - Some(client) => match client.create_repo(&repo).await { + Some(client) => match client.create_repo(&agent).await { Ok(full_name) => { tracing::info!(%full_name, "swarm jobq: create_repo done"); Outcome::Done @@ -156,24 +163,24 @@ async fn run_swarm_node( Err(e) => Outcome::Failed(format!("{e:#}")), }, }, - SwarmNodeKind::AddRepoMember { repo, agent } => match deps.forge { + SwarmNodeKind::AddRepoMember { agent } => match deps.forge { None => Outcome::Failed( "no forge configured on this host (SWARM_CONTROLLER_FORGE_URL / \ SWARM_CONTROLLER_FORGE_TOKEN_FILE unset)" .to_owned(), ), - Some(client) => match client.add_repo_member(&repo, &agent).await { + Some(client) => match client.add_repo_member(&agent, &agent).await { Ok(()) => Outcome::Done, Err(e) => Outcome::Failed(format!("{e:#}")), }, }, - SwarmNodeKind::InitAgentConfigRepo { repo, agent } => match deps.forge { + SwarmNodeKind::InitAgentConfigRepo { agent } => match deps.forge { None => Outcome::Failed( "no forge configured on this host (SWARM_CONTROLLER_FORGE_URL / \ SWARM_CONTROLLER_FORGE_TOKEN_FILE unset)" .to_owned(), ), - Some(client) => match client.seed_agent_config(&repo, &agent).await { + Some(client) => match client.seed_agent_config(&agent, &agent).await { Ok(()) => Outcome::Done, Err(e) => Outcome::Failed(format!("{e:#}")), }, @@ -547,13 +554,34 @@ 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. +/// +/// Validated here and carried no further: none of the nodes this endpoint +/// queues talks to a hive, so none of them needs the address. It reaches +/// its consumer when the node that *sends* a deploy message exists, and +/// that node takes it from this field. Settling the request shape now is +/// the point — it is the breaking half, and doing it once is cheaper for +/// every caller than doing it again later. #[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 +625,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" @@ -609,7 +637,33 @@ async fn create_agent( let agent = hive_types::Ident::parse(&req.name) .map_err(|reason| error_problem(axum::http::StatusCode::BAD_REQUEST, reason))? .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 @@ -621,19 +675,20 @@ async fn create_agent( agent: agent.clone(), }); let create_repo = b - .node(SwarmNodeKind::CreateRepo { repo: repo.clone() }) + .node(SwarmNodeKind::CreateRepo { + agent: agent.clone(), + }) .after_ok(create_identity); // Both fan out from `create_repo` directly — independent // operations on the same repo, no ordering requirement on // each other (see the doc comment above). let _add_repo_member = b .node(SwarmNodeKind::AddRepoMember { - repo: repo.clone(), agent: agent.clone(), }) .after_ok(create_repo); let _init_config = b - .node(SwarmNodeKind::InitAgentConfigRepo { repo, agent }) + .node(SwarmNodeKind::InitAgentConfigRepo { agent }) .after_ok(create_repo); vec![create_identity.guid()] }) @@ -976,6 +1031,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, + >, + >; + + 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 { @@ -990,7 +1142,7 @@ mod tests { let id = sched .append( SwarmNodeKind::CreateRepo { - repo: "atlas".to_owned(), + agent: "atlas".to_owned(), }, Vec::new(), None,