Guard NINJA_ENV overrides with mockable Env

tests/assert_cmd_tests.rs

@@ -8,6 +8,7 @@ use assert_cmd::Command;
 use std::fs;
 use tempfile::tempdir;
 
+#[expect(unused, reason = "support module exports helpers unused in this test")]
 mod support;
 
 #[test]

tests/cucumber.rs

@@ -28,6 +28,7 @@ mod check_ninja;
 #[path = "support/env.rs"]
 mod env;
 mod steps;
+#[expect(unused, reason = "support module exports helpers unused in this test")]
 mod support;
 
 #[tokio::main]

tests/ninja_env_tests.rs

@@ -0,0 +1,49 @@
+use mockable::MockEnv;
+use netsuke::runner::NINJA_ENV;
+use rstest::rstest;
+use std::env::VarError;
+use support::env_lock::EnvLock;
+use support::ninja_env::override_ninja_env;
+
+#[expect(
+    unused,
+    reason = "support module exports helpers unused in these tests"
+)]
+mod support;
+
+#[rstest]
+#[case(Some("orig"))]
+#[case(None)]
+#[case(Some(""))]
+fn override_ninja_env_restores(#[case] original: Option<&'static str>) {
+    let mut env = MockEnv::new();
+    match original {
+        Some(val) => {
+            let returned = val.to_string();
+            env.expect_raw()
+                .withf(|k| k == NINJA_ENV)
+                .times(1)
+                .return_once(move |_| Ok(returned));
+        }
+        None => {
+            env.expect_raw()
+                .withf(|k| k == NINJA_ENV)
+                .times(1)
+                .return_once(|_| Err(VarError::NotPresent));
+        }
+    }
+
+    {
+        let _guard = override_ninja_env(EnvLock::acquire(), &env, "new");
+        assert_eq!(std::env::var(NINJA_ENV).as_deref(), Ok("new"));
+    }
+
+    match original {
+        Some(val) => assert_eq!(std::env::var(NINJA_ENV).as_deref(), Ok(val)),
+        None => assert!(std::env::var(NINJA_ENV).is_err()),
+    }
+
+    let _cleanup = EnvLock::acquire();
+    // SAFETY: `EnvLock` serialises this mutation; see above for details.
+    unsafe { std::env::remove_var(NINJA_ENV) };
+}

tests/runner_tests.rs

@@ -1,5 +1,6 @@
+use mockable::DefaultEnv;
 use netsuke::cli::{BuildArgs, Cli, Commands};
-use netsuke::runner::{BuildTargets, NINJA_ENV, run, run_ninja};
+use netsuke::runner::{BuildTargets, run, run_ninja};
 use rstest::{fixture, rstest};
 use serial_test::serial;
 use std::path::{Path, PathBuf};
@@ -8,6 +9,7 @@ use std::path::{Path, PathBuf};
 mod check_ninja;
 mod support;
 use support::env_lock::EnvLock;
+use support::ninja_env::override_ninja_env;
 use support::path_guard::PathGuard;
 
 /// Fixture: Put a fake `ninja` (that checks for a build file) on PATH.
@@ -220,10 +222,12 @@ fn run_manifest_subcommand_writes_file() {
 #[serial]
 fn run_respects_env_override_for_ninja() {
     let (temp_dir, ninja_path) = support::fake_ninja(0);
-    let original = std::env::var_os(NINJA_ENV);
-    unsafe {
-        std::env::set_var(NINJA_ENV, &ninja_path);
-    }
+    // `NINJA_ENV` expects UTF-8; lossy conversion is acceptable in this test.
+    let program = ninja_path.to_string_lossy().into_owned();
+    let env = DefaultEnv::new();
+    // `set_var` is `unsafe` on Rust 2024; the lock serialises the mutation and
+    // `override_ninja_env` restores the original value via its guard.
+    let _guard = override_ninja_env(EnvLock::acquire(), &env, &program);
 
     let temp = tempfile::tempdir().expect("temp dir");
     let manifest_path = temp.path().join("Netsukefile");
@@ -241,14 +245,6 @@ fn run_respects_env_override_for_ninja() {
 
     let result = run(&cli);
     assert!(result.is_ok());
-
-    unsafe {
-        if let Some(val) = original {
-            std::env::set_var(NINJA_ENV, val);
-        } else {
-            std::env::remove_var(NINJA_ENV);
-        }
-    }
     drop(ninja_path);
     drop(temp_dir);
 }

tests/support/env_lock.rs

@@ -10,6 +10,7 @@ use std::sync::{Mutex, MutexGuard};
 static ENV_LOCK: Mutex<()> = Mutex::new(());
 
 #[allow(dead_code, reason = "only some tests mutate PATH")]
+#[derive(Debug)]
 /// RAII guard that holds the global environment lock.
 pub struct EnvLock(MutexGuard<'static, ()>);
 

tests/support/mod.rs

@@ -4,6 +4,7 @@
 //! logging utilities used in behavioural tests.
 
 pub mod env_lock;
+pub mod ninja_env;
 pub mod path_guard;
 
 #[expect(unused_imports, reason = "re-export for selective test crates")]
@@ -17,6 +18,13 @@ use tempfile::TempDir;
 /// Create a fake Ninja executable that exits with `exit_code`.
 ///
 /// Returns the temporary directory and the path to the executable.
+#[cfg_attr(
+    not(test),
+    expect(
+        unused_code,
+        reason = "only some test crates spawn fake ninja binaries"
+    )
+)]
 pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) {
     let dir = TempDir::new().expect("temp dir");
     let path = dir.path().join("ninja");

tests/support/ninja_env.rs

@@ -0,0 +1,72 @@
+//! Override and restore [`NINJA_ENV`] for tests.
+//!
+//! Provides a helper that sets [`NINJA_ENV`] while ensuring it is restored
+//! afterwards. This uses [`EnvLock`] to serialise mutations to the global
+//! environment and captures the previous value through a `mockable::Env`
+//! implementation so tests can inject their own state.
+
+use super::env_lock::EnvLock;
+use mockable::Env;
+use netsuke::runner::NINJA_ENV;
+
+/// Guard that resets `NINJA_ENV` on drop.
+///
+/// Holding the guard keeps the environment override in place. Dropping it
+/// restores the prior value while releasing the environment lock, cleaning up
+/// global state even if a test panics.
+#[must_use]
+#[derive(Debug)]
+pub struct NinjaEnvGuard {
+    _lock: EnvLock,
+    original: Option<String>,
+}
+
+/// Set [`NINJA_ENV`] to `value`, returning a guard that restores the previous
+/// value when dropped.
+///
+/// # Thread Safety
+///
+/// This function is **not thread-safe**. Callers must supply an
+/// [`EnvLock`](super::env_lock::EnvLock), which is stored in the returned guard
+/// to serialise the mutation and ensure restoration occurs before the lock is
+/// released.
+///
+/// Drop order is enforced: dropping the guard restores [`NINJA_ENV`] and only
+/// then releases the lock.
+///
+/// # Examples
+/// ```ignore
+/// use mockable::DefaultEnv;
+/// use crate::support::{env_lock::EnvLock, ninja_env::override_ninja_env};
+/// let env = DefaultEnv::new();
+/// let _guard = override_ninja_env(EnvLock::acquire(), &env, "/usr/bin/ninja");
+/// ```
+#[cfg_attr(
+    not(test),
+    expect(unused_code, reason = "only some tests override NINJA_ENV")
+)]
+pub fn override_ninja_env(lock: EnvLock, env: &impl Env, value: &str) -> NinjaEnvGuard {
+    let original = env.raw(NINJA_ENV).ok();
+    // Safety: `EnvLock` serialises this mutation. `set_var` is `unsafe` on Rust
+    // 2024 and the guard restores the prior value on drop.
+    unsafe { std::env::set_var(NINJA_ENV, value) };
+    NinjaEnvGuard {
+        _lock: lock,
+        original,
+    }
+}
+
+impl Drop for NinjaEnvGuard {
+    fn drop(&mut self) {
+        // Safety: the guard holds [`EnvLock`] for its lifetime, so these
+        // `set_var`/`remove_var` calls are serialised. Both functions are
+        // `unsafe` on Rust 2024.
+        unsafe {
+            if let Some(ref val) = self.original {
+                std::env::set_var(NINJA_ENV, val);
+            } else {
+                std::env::remove_var(NINJA_ENV);
+            }
+        }
+    }
+}