Explain PATH mutability hazards in tests

Cargo.toml

@@ -68,6 +68,7 @@ insta = { version = "1", features = ["yaml"] }
 assert_cmd = "2.0.17"
 mockable = { version = "0.3", features = ["mock"] }
 serial_test = "3"
+mockall = "0.11"
 
 [[test]]
 name = "cucumber"

tests/env_path_tests.rs

@@ -0,0 +1,81 @@
+//! Tests for scoped manipulation of `PATH` via `prepend_dir_to_path` and
+//! `PathGuard`.
+
+use mockable::Env;
+use rstest::rstest;
+use serial_test::serial;
+
+#[path = "support/env.rs"]
+mod env;
+mod support;
+use env::{SystemEnv, mocked_path_env, prepend_dir_to_path};
+use support::env_lock::EnvLock;
+
+#[rstest]
+#[serial]
+fn prepend_dir_to_path_sets_and_restores() {
+    let env = mocked_path_env();
+    let original = env.raw("PATH").expect("PATH should be set in mock");
+    let dir = tempfile::tempdir().expect("temp dir");
+    let guard = prepend_dir_to_path(&env, dir.path());
+    let after = std::env::var("PATH").expect("path var");
+    let first = std::env::split_paths(&after).next().expect("first path");
+    assert_eq!(first, dir.path());
+    drop(guard);
+    let restored = std::env::var("PATH").expect("path var");
+    assert_eq!(restored, original);
+}
+
+#[rstest]
+#[serial]
+fn prepend_dir_to_path_handles_empty_path() {
+    let original = std::env::var_os("PATH");
+    {
+        let _lock = EnvLock::acquire();
+        unsafe { std::env::set_var("PATH", "") };
+    }
+    let env = SystemEnv::new();
+    let dir = tempfile::tempdir().expect("temp dir");
+    let guard = prepend_dir_to_path(&env, dir.path());
+    let after = std::env::var_os("PATH").expect("path var");
+    let paths = std::env::split_paths(&after)
+        .filter(|p| !p.as_os_str().is_empty())
+        .collect::<Vec<_>>();
+    assert_eq!(paths, vec![dir.path().to_path_buf()]);
+    drop(guard);
+    assert_eq!(std::env::var_os("PATH"), Some(std::ffi::OsString::new()));
+    {
+        let _lock = EnvLock::acquire();
+        if let Some(path) = original {
+            unsafe { std::env::set_var("PATH", path) };
+        } else {
+            unsafe { std::env::remove_var("PATH") };
+        }
+    }
+}
+
+#[rstest]
+#[serial]
+fn prepend_dir_to_path_handles_missing_path() {
+    let original = std::env::var_os("PATH");
+    {
+        let _lock = EnvLock::acquire();
+        unsafe { std::env::remove_var("PATH") };
+    }
+    let env = SystemEnv::new();
+    let dir = tempfile::tempdir().expect("temp dir");
+    let guard = prepend_dir_to_path(&env, dir.path());
+    let after = std::env::var_os("PATH").expect("PATH should exist after prepend");
+    let paths: Vec<_> = std::env::split_paths(&after).collect();
+    assert_eq!(paths, vec![dir.path().to_path_buf()]);
+    drop(guard);
+    assert!(std::env::var_os("PATH").is_none());
+    {
+        let _lock = EnvLock::acquire();
+        if let Some(path) = original {
+            unsafe { std::env::set_var("PATH", path) };
+        } else {
+            unsafe { std::env::remove_var("PATH") };
+        }
+    }
+}

tests/path_guard_tests.rs

@@ -0,0 +1,42 @@
+//! Tests for PATH restoration behaviour using mock environments.
+//!
+//! Verifies that `PathGuard` restores `PATH` without mutating the real
+//! process environment.
+
+#[path = "support/env_lock.rs"]
+mod env_lock;
+#[path = "support/path_guard.rs"]
+mod path_guard;
+
+use mockall::{Sequence, mock};
+use path_guard::{Env, PathGuard};
+use std::ffi::OsStr;
+
+mock! {
+    pub Env {}
+    impl Env for Env {
+        unsafe fn set_var(&mut self, key: &str, val: &OsStr);
+    }
+}
+
+#[test]
+fn restores_path_without_touching_real_env() {
+    let mut env = MockEnv::new();
+    let mut seq = Sequence::new();
+    env.expect_set_var()
+        .withf(|k, v| k == "PATH" && v == OsStr::new("/tmp"))
+        .times(1)
+        .in_sequence(&mut seq)
+        .return_const(());
+    env.expect_set_var()
+        .withf(|k, v| k == "PATH" && v == OsStr::new("/orig"))
+        .times(1)
+        .in_sequence(&mut seq)
+        .return_const(());
+    {
+        let mut guard = PathGuard::with_env("/orig".into(), env);
+        unsafe {
+            guard.env_mut().set_var("PATH", OsStr::new("/tmp"));
+        }
+    }
+}

tests/runner_tests.rs

@@ -6,49 +6,39 @@ use std::path::{Path, PathBuf};
 
 #[path = "support/check_ninja.rs"]
 mod check_ninja;
+#[path = "support/env.rs"]
+mod env;
 mod support;
-use support::env_lock::EnvLock;
+use env::{SystemEnv, prepend_dir_to_path};
 use support::path_guard::PathGuard;
 
-/// Fixture: Put a fake `ninja` (that checks for a build file) on PATH.
+/// Fixture: Put a fake `ninja` (that checks for a build file) on `PATH`.
+///
+/// In Rust 2024 `std::env::set_var` is `unsafe` because it mutates
+/// process-global state. `EnvLock` serialises the mutation and the returned
+/// [`PathGuard`] restores the prior value, so the risk is confined to this test.
 ///
 /// Returns: (tempdir holding ninja, `ninja_path`, PATH guard)
 #[fixture]
 fn ninja_in_path() -> (tempfile::TempDir, PathBuf, PathGuard) {
     let (ninja_dir, ninja_path) = check_ninja::fake_ninja_check_build_file();
-
-    // Save PATH and prepend our fake ninja directory.
-    let original_path = std::env::var_os("PATH").unwrap_or_default();
-    let mut paths: Vec<_> = std::env::split_paths(&original_path).collect();
-    paths.insert(0, ninja_dir.path().to_path_buf());
-    let new_path = std::env::join_paths(paths).expect("join paths");
-    let _lock = EnvLock::acquire();
-    // Nightly marks set_var unsafe.
-    unsafe { std::env::set_var("PATH", &new_path) };
-
-    let guard = PathGuard::new(original_path);
+    let env = SystemEnv::new();
+    let guard = prepend_dir_to_path(&env, ninja_dir.path());
     (ninja_dir, ninja_path, guard)
 }
 
-/// Fixture: Put a fake `ninja` with a specific exit code on PATH.
+/// Fixture: Put a fake `ninja` with a specific exit code on `PATH`.
 ///
-/// The default exit code is 0, but can be customised via `#[with(...)]`.
+/// The default exit code is 0 but may be customised via `#[with(...)]`. The
+/// fixture uses `EnvLock` and [`PathGuard`] to tame the `unsafe` `set_var` call,
+/// mirroring [`ninja_in_path`].
 ///
 /// Returns: (tempdir holding ninja, `ninja_path`, PATH guard)
 #[fixture]
 fn ninja_with_exit_code(#[default(0)] exit_code: i32) -> (tempfile::TempDir, PathBuf, PathGuard) {
     let (ninja_dir, ninja_path) = support::fake_ninja(exit_code);
-
-    // Save PATH and prepend our fake ninja directory.
-    let original_path = std::env::var_os("PATH").unwrap_or_default();
-    let mut paths: Vec<_> = std::env::split_paths(&original_path).collect();
-    paths.insert(0, ninja_dir.path().to_path_buf());
-    let new_path = std::env::join_paths(paths).expect("join paths");
-    let _lock = EnvLock::acquire();
-    // Nightly marks set_var unsafe.
-    unsafe { std::env::set_var("PATH", &new_path) };
-
-    let guard = PathGuard::new(original_path);
+    let env = SystemEnv::new();
+    let guard = prepend_dir_to_path(&env, ninja_dir.path());
     (ninja_dir, ninja_path, guard)
 }
 

tests/steps/process_steps.rs

@@ -1,31 +1,23 @@
 //! Step definitions for Ninja process execution.
 
-use crate::{CliWorld, check_ninja, env, support};
+use crate::{
+    CliWorld, check_ninja,
+    env::{self, EnvMut},
+    support,
+};
 use cucumber::{given, then, when};
-use mockable::Env;
 use netsuke::runner::{self, BuildTargets, NINJA_PROGRAM};
 use std::fs;
 use std::path::{Path, PathBuf};
-use support::env_lock::EnvLock;
-use support::path_guard::PathGuard;
 use tempfile::{NamedTempFile, TempDir};
 
 /// Installs a test-specific ninja binary and updates the `PATH`.
 #[expect(
     clippy::needless_pass_by_value,
     reason = "helper owns path for simplicity"
 )]
-fn install_test_ninja(env: &impl Env, world: &mut CliWorld, dir: TempDir, ninja_path: PathBuf) {
-    let original = env.raw("PATH").unwrap_or_default();
-    let guard = PathGuard::new(original.clone().into());
-    let new_path = format!("{}:{}", dir.path().display(), original);
-    // SAFETY: `std::env::set_var` is `unsafe` in Rust 2024 due to global state.
-    // `EnvLock` serialises mutations and `PathGuard` restores the prior value.
-    let _lock = EnvLock::acquire();
-    unsafe {
-        std::env::set_var("PATH", &new_path);
-    }
-
+fn install_test_ninja(env: &impl EnvMut, world: &mut CliWorld, dir: TempDir, ninja_path: PathBuf) {
+    let guard = env::prepend_dir_to_path(env, dir.path());
     world.path_guard = Some(guard);
     world.ninja = Some(ninja_path.to_string_lossy().into_owned());
     world.temp = Some(dir);

tests/support/env.rs

@@ -3,9 +3,41 @@
 //! Provides fixtures and utilities for managing `PATH` and writing minimal
 //! manifests.
 
-use mockable::MockEnv;
+use mockable::{DefaultEnv, Env, MockEnv};
 use rstest::fixture;
+use std::ffi::{OsStr, OsString};
 use std::io::{self, Write};
+use std::path::Path;
+
+use crate::support::env_lock::EnvLock;
+use crate::support::path_guard::PathGuard;
+
+/// Alias for the real process environment.
+#[allow(dead_code, reason = "re-exported for tests")]
+pub type SystemEnv = DefaultEnv;
+
+/// Environment trait with mutation capabilities.
+pub trait EnvMut: Env {
+    /// Set `key` to `value` within the environment.
+    ///
+    /// # Safety
+    ///
+    /// Mutating global state is `unsafe` in Rust 2024. Callers must ensure the
+    /// operation is serialised and rolled back appropriately.
+    unsafe fn set_var(&self, key: &str, value: &OsStr);
+}
+
+impl EnvMut for DefaultEnv {
+    unsafe fn set_var(&self, key: &str, value: &OsStr) {
+        unsafe { std::env::set_var(key, value) };
+    }
+}
+
+impl EnvMut for MockEnv {
+    unsafe fn set_var(&self, key: &str, value: &OsStr) {
+        unsafe { std::env::set_var(key, value) };
+    }
+}
 
 /// Fixture: capture the original `PATH` via a mocked environment.
 ///
@@ -25,6 +57,7 @@ pub fn mocked_path_env() -> MockEnv {
 /// Write a minimal manifest to `file`.
 ///
 /// The manifest declares a single `hello` target that prints a greeting.
+#[allow(dead_code, reason = "used in Cucumber tests")]
 pub fn write_manifest(file: &mut impl Write) -> io::Result<()> {
     writeln!(
         file,
@@ -38,3 +71,24 @@ pub fn write_manifest(file: &mut impl Write) -> io::Result<()> {
         ),
     )
 }
+
+/// Prepend `dir` to the real `PATH`, returning a guard that restores it.
+///
+/// Mutating `PATH` is `unsafe` in Rust 2024 because it alters process globals.
+/// `EnvLock` serialises access and `PathGuard` rolls back the change, keeping
+/// the unsafety scoped to a single test.
+#[allow(dead_code, reason = "used in runner tests")]
+pub fn prepend_dir_to_path(env: &impl EnvMut, dir: &Path) -> PathGuard {
+    let original = env.raw("PATH").ok();
+    let original_os = original.clone().map(OsString::from);
+    let mut paths: Vec<_> = original_os
+        .as_ref()
+        .map(|os| std::env::split_paths(os).collect())
+        .unwrap_or_default();
+    paths.insert(0, dir.to_path_buf());
+    let new_path = std::env::join_paths(&paths).expect("Failed to join PATH entries");
+    let _lock = EnvLock::acquire();
+    // SAFETY: `EnvLock` serialises mutations and the guard restores on drop.
+    unsafe { env.set_var("PATH", &new_path) };
+    PathGuard::new(original_os)
+}

tests/support/mod.rs

@@ -17,6 +17,7 @@ use tempfile::TempDir;
 /// Create a fake Ninja executable that exits with `exit_code`.
 ///
 /// Returns the temporary directory and the path to the executable.
+#[allow(dead_code, reason = "used in other test crates")]
 pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) {
     let dir = TempDir::new().expect("temp dir");
     let path = dir.path().join("ninja");