diff --git a/hive-bash-mcp/src/lib.rs b/hive-bash-mcp/src/lib.rs index 8f8a6ee3..150bd831 100644 --- a/hive-bash-mcp/src/lib.rs +++ b/hive-bash-mcp/src/lib.rs @@ -9,3 +9,5 @@ pub mod paths; pub mod protocol; pub mod runner; pub mod stats; +#[cfg(test)] +mod test_util; diff --git a/hive-bash-mcp/src/mcp.rs b/hive-bash-mcp/src/mcp.rs index 20efddc0..940b7480 100644 --- a/hive-bash-mcp/src/mcp.rs +++ b/hive-bash-mcp/src/mcp.rs @@ -289,36 +289,7 @@ pub async fn serve_http(addr: std::net::SocketAddr) -> anyhow::Result<()> { mod status_hint_tests { use super::{BASH_IDLE_WAIT_HINT, format_task}; use crate::protocol::{TaskFile, TaskStatus}; - use std::sync::Mutex; - - /// `format_task` unconditionally resolves `crate::paths::task_out`/ - /// `task_err` (to check captured-output length), which panics if - /// `HYPERHIVE_HARNESS_DIR` is unset — exactly cargo's sandboxed test - /// environment. Same helper shape as `runner::tests::with_harness_dir` - /// (module-scope mutex to serialise against parallel test threads - /// mutating the process-wide env var, save/restore on the way out); - /// duplicated rather than shared because the two test modules live in - /// separate files with no existing shared test-util module. - static HARNESS_DIR_ENV_LOCK: Mutex<()> = Mutex::new(()); - - fn with_harness_dir(f: F) { - let _guard = HARNESS_DIR_ENV_LOCK - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - let prev = std::env::var("HYPERHIVE_HARNESS_DIR").ok(); - // SAFETY: serialised by HARNESS_DIR_ENV_LOCK above; restored below - // in the same scope before the guard drops. - unsafe { - std::env::set_var("HYPERHIVE_HARNESS_DIR", "/tmp/hive-bash-mcp-test-harness"); - } - f(); - unsafe { - match prev { - Some(v) => std::env::set_var("HYPERHIVE_HARNESS_DIR", v), - None => std::env::remove_var("HYPERHIVE_HARNESS_DIR"), - } - } - } + use crate::test_util::with_harness_dir; fn task(status: TaskStatus) -> TaskFile { TaskFile { diff --git a/hive-bash-mcp/src/runner.rs b/hive-bash-mcp/src/runner.rs index e2f1843b..7fa0c975 100644 --- a/hive-bash-mcp/src/runner.rs +++ b/hive-bash-mcp/src/runner.rs @@ -796,38 +796,8 @@ fn done_summary(id: &str, summary: &str, output: Option<(bool, bool)>) -> String #[cfg(test)] mod tests { use super::validate_task_name; + use crate::test_util::with_harness_dir; use hive_types::Ident; - use std::sync::Mutex; - - /// `done_summary`'s stderr/stdout-pointer tests format real paths via - /// `crate::paths::task_out`/`task_err`, which resolve through - /// `hive_agent_sock::paths::harness_dir` — that panics if - /// `HYPERHIVE_HARNESS_DIR` is unset (see its doc comment), which is - /// exactly cargo's sandboxed test environment (no real container, no - /// meta-flake-injected env). Set a dummy value for the duration of - /// those tests, serialised on a module mutex so parallel test threads - /// don't race the process-wide env var, and restore whatever was there - /// before on the way out. - static HARNESS_DIR_ENV_LOCK: Mutex<()> = Mutex::new(()); - - fn with_harness_dir(f: F) { - let _guard = HARNESS_DIR_ENV_LOCK - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - let prev = std::env::var("HYPERHIVE_HARNESS_DIR").ok(); - // SAFETY: serialised by HARNESS_DIR_ENV_LOCK above; restored below - // in the same scope before the guard drops. - unsafe { - std::env::set_var("HYPERHIVE_HARNESS_DIR", "/tmp/hive-bash-mcp-test-harness"); - } - f(); - unsafe { - match prev { - Some(v) => std::env::set_var("HYPERHIVE_HARNESS_DIR", v), - None => std::env::remove_var("HYPERHIVE_HARNESS_DIR"), - } - } - } #[test] fn accepts_ident_names() { diff --git a/hive-bash-mcp/src/test_util.rs b/hive-bash-mcp/src/test_util.rs new file mode 100644 index 00000000..e24e0472 --- /dev/null +++ b/hive-bash-mcp/src/test_util.rs @@ -0,0 +1,50 @@ +//! Test-only helper shared by `mcp::status_hint_tests` and `runner::tests`. +//! +//! Both modules need `HYPERHIVE_HARNESS_DIR` set for the duration of a test +//! (`crate::paths::harness_dir` panics if it's unset — deliberately, see +//! its doc comment — and that's exactly cargo's sandboxed test env with no +//! meta-flake-injected value). The env var is one process-global, and both +//! modules land in the *same* test binary (same crate) with tests running +//! on parallel threads, so a single shared lock here is required — two +//! separate per-module mutexes serialise nothing against each other — that +//! caused a CI-only flake, since a container always has the var set so a +//! losing race still finds a valid restored value, while the nix sandbox +//! strips it, so a racing thread can delete the var out from under another +//! mid-test. +//! +//! ⚠️ **This asymmetry also breaks local reproduction**: a plain `cargo +//! test` inside an agent container can never fail this test, fixed or +//! broken — `HYPERHIVE_HARNESS_DIR` is always ambient-set there, so the +//! restore branch always takes `set_var`, never `remove_var`. To actually +//! exercise the sandbox shape (and prove a fix works, or reproduce the +//! original bug), unset the var first: `env -u HYPERHIVE_HARNESS_DIR +//! cargo test -p hive-bash-mcp`. Any *new* helper that reaches for the env +//! var in a test should route through `with_harness_dir` below rather than +//! rolling its own save/restore — a second lock reintroduces exactly this +//! bug. +use std::sync::Mutex; + +static HARNESS_DIR_ENV_LOCK: Mutex<()> = Mutex::new(()); + +/// Run `f` with `HYPERHIVE_HARNESS_DIR` set to a dummy path, serialised +/// against every other caller of this helper in the process so parallel +/// test threads can't race the env var out from under each other. +/// Restores whatever value (or absence) was there before on the way out. +pub fn with_harness_dir(f: F) { + let _guard = HARNESS_DIR_ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let prev = std::env::var("HYPERHIVE_HARNESS_DIR").ok(); + // SAFETY: serialised by HARNESS_DIR_ENV_LOCK above; restored below in + // the same scope before the guard drops. + unsafe { + std::env::set_var("HYPERHIVE_HARNESS_DIR", "/tmp/hive-bash-mcp-test-harness"); + } + f(); + unsafe { + match prev { + Some(v) => std::env::set_var("HYPERHIVE_HARNESS_DIR", v), + None => std::env::remove_var("HYPERHIVE_HARNESS_DIR"), + } + } +}