From dbff9f09875c2f613eaba3a802a30a6c251e66c6 Mon Sep 17 00:00:00 2001 From: atlas Date: Sat, 1 Aug 2026 00:36:09 +0200 Subject: [PATCH] fix(#2860): refuse to write a meta flake with no forge URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the agent option nullable, a missing `HIVE_FORGE_URL` would no longer fail anything — it would deploy a whole fleet of agents that silently never log into the forge. The forge is not optional on a running hive, so the hive asserts that itself rather than leaning on a module that legitimately allows "no forge" when evaluated standalone. `sync_agents` checks it before writing anything. That is the moment the hive commits to a flake, and it keeps `render_flake` a pure string operation: the renderer is exercised directly by a dozen tests, so making *it* env-dependent would force each of them to either set a process-wide var — the parallel-test race this module already avoids — or fail for reasons unrelated to what they assert. `require_service_urls` is pure over the already-collected pairs, so its two tests need no process env at all. Refs #2860 --- hive-c0re/src/meta.rs | 101 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 91 insertions(+), 10 deletions(-) diff --git a/hive-c0re/src/meta.rs b/hive-c0re/src/meta.rs index 080982b9..37ba30da 100644 --- a/hive-c0re/src/meta.rs +++ b/hive-c0re/src/meta.rs @@ -85,6 +85,11 @@ async fn stage_generated_meta_files(dir: &std::path::Path) -> Result<()> { /// no-op. pub async fn sync_agents(hive: &HiveEnv, agents: &[AgentSpec]) -> Result<()> { let _guard = META_LOCK.lock().await; + // Before anything is written: a hive without a forge URL would deploy a + // whole fleet of agents that silently never log in, because the agent + // option treats "unset" as "no forge configured" rather than erroring. + // This is the layer that knows a forge is mandatory, so it says so here. + require_service_urls(&forwarded_env_vars())?; let dir = crate::paths::meta_root(); std::fs::create_dir_all(&dir).with_context(|| format!("create {}", dir.display()))?; @@ -715,8 +720,12 @@ const SERVICE_URL_OPTIONS: &[(&str, &str)] = &[ /// other under the default parallel test runner. A pure function over the /// already-collected pairs has no such hazard. /// -/// A var that isn't present emits nothing rather than a guess — see the call -/// site for why that silence is the point. +/// A var that isn't present emits nothing rather than a guess. For an optional +/// service that is the whole point — the agent option defaults to `null`, +/// meaning "not configured", and the units that would use it aren't generated. +/// For a service the hive cannot run without, silence would instead produce a +/// fleet of agents quietly missing an integration, so those are checked by +/// [`require_service_urls`] before this is called. fn push_service_url_options(out: &mut String, vars: &[(&'static str, String)]) { use std::fmt::Write as _; for (var, val) in vars { @@ -728,6 +737,48 @@ fn push_service_url_options(out: &mut String, vars: &[(&'static str, String)]) { } } +/// Service URLs a running hive must always supply, checked before rendering. +/// +/// The forge is not optional on a real hive: `hive-c0re.nix` sets +/// `HIVE_FORGE_URL` unconditionally, so its absence means this daemon was +/// started outside the NixOS module. The agent option is nullable — `null` +/// legitimately means "no forge" when the modules are evaluated on their own — +/// which is exactly why the hive has to assert its own requirement here rather +/// than leaning on the module to reject the empty case. +const REQUIRED_SERVICE_URL_VARS: &[&str] = &["HIVE_FORGE_URL"]; + +/// Fails unless every [`REQUIRED_SERVICE_URL_VARS`] entry is present in +/// `vars`. +/// +/// Pure over the already-collected pairs so it can be tested without touching +/// process env (the same reason [`push_service_url_options`] is split out). +/// +/// Checked by `sync_agents` — the point at which the hive commits a rendered +/// flake to disk — rather than inside the renderer. Rendering is a pure string +/// operation that many tests exercise directly; making *it* env-dependent +/// would mean every one of those tests either sets a process-wide var (the +/// parallel-test race this module already avoids) or fails for reasons that +/// have nothing to do with what it asserts. +/// +/// # Errors +/// +/// When a required var is missing. `hive-c0re.nix` sets it unconditionally, so +/// this means the daemon is running outside the NixOS module; refusing to +/// write the flake beats writing one whose agents would silently all lack a +/// forge. +fn require_service_urls(vars: &[(&'static str, String)]) -> Result<()> { + for required in REQUIRED_SERVICE_URL_VARS { + if !vars.iter().any(|(name, _)| name == required) { + anyhow::bail!( + "{required} is unset — hive-c0re.nix sets it unconditionally, so this process \ + was started outside the NixOS module. Refusing to write a meta flake whose \ + agents would every one of them have no forge configured." + ); + } + } + Ok(()) +} + fn forwarded_env_vars() -> Vec<(&'static str, String)> { FORWARDED_VARS .iter() @@ -1177,10 +1228,16 @@ where // ever correct when the callee shares the caller's netns, and the forge // and homeserver are moving to swarm level, possibly on other hosts. // - // Absent vars emit nothing rather than a guess. Once the defaults are - // gone that surfaces as an eval failure, which is the point: better a - // build that stops than an agent quietly talking to a port on the wrong - // machine. + // Absent vars emit nothing rather than a guess, and the agent option + // treats "unset" as "this service is not configured" rather than + // substituting a loopback address — an absent integration instead of a + // misdirected one. + // + // That is the right default for an optional service and the wrong one for + // the forge, which a running hive always has — so `sync_agents` rejects a + // missing `HIVE_FORGE_URL` before it writes anything (`require_service_urls`). + // The check lives there rather than here because rendering is a pure string + // operation the tests exercise directly. push_service_url_options(&mut out, &forwarded_env_vars()); // GitHub integration is on by default in every agent // (`hyperhive.github.enable`); the host turns it off hive-wide via @@ -1900,12 +1957,36 @@ mod tests { ); } + #[test] + fn require_service_urls_accepts_a_rendered_forge_url() { + require_service_urls(&[ + ("HIVE_FORGE_URL", "http://forge.example.test".to_string()), + ("HYPERHIVE_HIVE_NAME", "pr1ma".to_string()), + ]) + .expect("a forwarded forge URL satisfies the requirement"); + } + + #[test] + fn require_service_urls_refuses_to_write_without_a_forge() { + // The agent option is nullable, so nothing downstream would complain: + // every agent would simply come up with no forge login and no way to + // tell that was unintended. The hive asserts its own requirement + // because it is the only layer that knows a forge is mandatory. + let err = require_service_urls(&[("HYPERHIVE_HIVE_NAME", "pr1ma".to_string())]) + .expect_err("a missing forge URL must stop the write"); + assert!( + err.to_string().contains("HIVE_FORGE_URL is unset"), + "the error must name the missing variable: {err}" + ); + } + #[test] fn service_url_options_emit_nothing_when_absent() { - // No guess when the host doesn't say. Once the nix-side defaults are - // removed this is what turns a missing value into a build failure - // rather than an agent quietly talking to a port on the wrong machine, - // so the absence has to be as deliberate as the presence. + // No guess when the host doesn't say — the agent option stays `null` + // ("not configured") and the units that would use it aren't generated, + // so absence is an absent integration rather than a misdirected one. + // Required services don't reach here: `require_service_urls` rejects + // them first. let mut out = String::new(); push_service_url_options(&mut out, &[("HYPERHIVE_HIVE_NAME", "pr1ma".to_string())]); assert!(