diff --git a/crates/path-cli/src/config.rs b/crates/path-cli/src/config.rs index 4dccd4c6..ff7f41ca 100644 --- a/crates/path-cli/src/config.rs +++ b/crates/path-cli/src/config.rs @@ -46,41 +46,43 @@ pub(crate) const DOCUMENTS_DIR_NAME: &str = "documents"; /// /// Public because `cmd_resume::run_with_strategy` takes a `&Config` /// across the crate boundary. It is a test seam, not API: the item is -/// `#[doc(hidden)]` and the fields stay crate-private, so -/// [`Config::load`] is the only constructor outside the crate. +/// `#[doc(hidden)]`. The fields are public so integration tests build +/// a `Config` directly; integration tests are separate crates and +/// cannot see `#[cfg(test)]` items. [`Config::load`] stays the only +/// production constructor. #[doc(hidden)] #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] pub struct Config { /// `$APPDATA`: Windows harness data root. - pub(crate) appdata: Option, + pub appdata: Option, /// `$CLAUDE_CLI_DEBUG`: the Claude reader warns about every /// unparseable conversation line, not just the first 5. Presence is /// the signal; the value is not read. - pub(crate) claude_cli_debug: Option, + pub claude_cli_debug: Option, /// `$CODEX_ROLLOUT_STRICT`: the Codex reader errors on an /// unparseable rollout line. Presence is the signal; the value is /// not read. - pub(crate) codex_rollout_strict: Option, + pub codex_rollout_strict: Option, /// `$COPILOT_EVENTS_STRICT`: the Copilot reader errors on a /// malformed events line. Presence is the signal; the value is not /// read. - pub(crate) copilot_events_strict: Option, + pub copilot_events_strict: Option, /// `$COPILOT_HOME`: Copilot CLI session root override. - pub(crate) copilot_home: Option, + pub copilot_home: Option, /// `$GITHUB_TOKEN`: GitHub API token (see `providers`). - pub(crate) github_token: Option, + pub github_token: Option, /// `$HOME`: config-root fallback and the harness resolvers' root. - pub(crate) home: Option, + pub home: Option, /// `$PATHBASE_URL`: Pathbase server override (see `cmd_pathbase`). - pub(crate) pathbase_url: Option, + pub pathbase_url: Option, /// `$TOOLPATH_CONFIG_DIR`: overrides the `~/.toolpath` root. - pub(crate) toolpath_config_dir: Option, + pub toolpath_config_dir: Option, /// `$TOOLPATH_QUERY_EXPLAIN`: query-planner diagnostics on stderr. - pub(crate) toolpath_query_explain: Option, + pub toolpath_query_explain: Option, /// `$USERPROFILE`: Windows home, the fallback when `$HOME` is unset. - pub(crate) userprofile: Option, + pub userprofile: Option, /// `$XDG_DATA_HOME`: opencode's data root (Linux). - pub(crate) xdg_data_home: Option, + pub xdg_data_home: Option, } /// [`Env`], with values emitted as verbatim strings. @@ -196,27 +198,18 @@ pub(crate) fn home_relative(path: &std::path::Path, home: Option<&std::path::Pat path.display().to_string() } -/// Shared lock for tests that mutate the process environment. Every test -/// that calls `set_var` / `remove_var`, or that runs a `figment::Jail`, -/// grabs this lock first, otherwise parallel tests clobber each other's -/// values. -#[cfg(test)] -pub(crate) static TEST_ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); - #[cfg(test)] mod tests { use super::*; - /// `figment::Jail` restores the variables it sets, but it serializes - /// only against other Jail tests. Hold `TEST_ENV_LOCK` too: it - /// serializes against every other test that mutates the - /// environment. + /// `figment::Jail` restores the variables it sets, and it + /// serializes its own tests. No other test mutates the environment, + /// so the Jail tests need no further lock. // result_large_err: the Jail closure returns figment's own // 208-byte error type. #[test] #[allow(clippy::result_large_err)] fn load_maps_every_owned_env_var() { - let _g = TEST_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); figment::Jail::expect_with(|jail| { jail.set_env(CONFIG_DIR_ENV, "/tmp/cfg-root"); jail.set_env("HOME", "/home/jailed"); @@ -262,7 +255,6 @@ mod tests { #[test] #[allow(clippy::result_large_err)] fn load_ignores_unowned_env_vars() { - let _g = TEST_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); figment::Jail::expect_with(|jail| { jail.set_env(PATHBASE_URL_ENV, "https://real.example"); jail.set_env("PATHBASE_URL_BACKUP", "https://wrong.example"); @@ -291,7 +283,6 @@ mod tests { #[test] #[allow(clippy::result_large_err)] fn load_keeps_scalar_looking_values_verbatim() { - let _g = TEST_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); figment::Jail::expect_with(|jail| { jail.set_env("TOOLPATH_QUERY_EXPLAIN", "01"); let config = Config::load().unwrap(); diff --git a/crates/path-cli/tests/resume.rs b/crates/path-cli/tests/resume.rs index 6de318e3..48ca48c8 100644 --- a/crates/path-cli/tests/resume.rs +++ b/crates/path-cli/tests/resume.rs @@ -2,9 +2,9 @@ //! //! Tests dispatch through `path_cli::cmd_resume::run_with_strategy` //! with a `RecordingExec` strategy so the would-be `execvp` becomes a -//! captured `(binary, args, cwd)` tuple. Each test isolates `$HOME` and -//! `$TOOLPATH_CONFIG_DIR` via an RAII guard under a shared lock, and -//! passes a tempdir of fake binaries as the search path. +//! captured `(binary, args, cwd)` tuple. Each test passes a `Config` +//! rooted at a `TestHome` tempdir plus a tempdir of fake binaries as +//! the search path, so the process environment stays untouched. #![cfg(not(target_os = "emscripten"))] @@ -18,8 +18,7 @@ use support::*; #[test] fn file_input_explicit_claude_projects_and_records_exec() { - let _env = env_lock(); - let home = ScopedHome::new(); + let home = TestHome::new(); let bin = fake_bin_dir(&["claude"]); let cwd = tempfile::tempdir().unwrap(); @@ -41,10 +40,8 @@ fn file_input_explicit_claude_projects_and_records_exec() { assert!(!cap.args[1].is_empty(), "session id should be non-empty"); assert_eq!(cap.cwd, std::fs::canonicalize(cwd.path()).unwrap()); - // Side effect: a JSONL was written under HOME/.claude/projects. - let projects = std::env::var_os("HOME") - .map(|h| std::path::PathBuf::from(h).join(".claude/projects")) - .unwrap(); + // Side effect: a JSONL was written under the sandbox ~/.claude/projects. + let projects = home.home_dir().join(".claude/projects"); assert!(projects.exists(), "claude projects dir not created"); assert!( dir_contains_file_with_ext(&projects, "jsonl"), @@ -54,8 +51,7 @@ fn file_input_explicit_claude_projects_and_records_exec() { #[test] fn file_input_explicit_gemini_projects_and_records_exec() { - let _env = env_lock(); - let home = ScopedHome::new(); + let home = TestHome::new(); let bin = fake_bin_dir(&["gemini"]); let cwd = tempfile::tempdir().unwrap(); @@ -76,16 +72,13 @@ fn file_input_explicit_gemini_projects_and_records_exec() { assert_eq!(cap.args[0], "--resume"); assert!(!cap.args[1].is_empty()); - let tmp_root = std::env::var_os("HOME") - .map(|h| std::path::PathBuf::from(h).join(".gemini/tmp")) - .unwrap(); + let tmp_root = home.home_dir().join(".gemini/tmp"); assert!(tmp_root.exists(), "gemini tmp dir not created"); } #[test] fn file_input_explicit_codex_projects_and_records_exec() { - let _env = env_lock(); - let home = ScopedHome::new(); + let home = TestHome::new(); let bin = fake_bin_dir(&["codex"]); let cwd = tempfile::tempdir().unwrap(); @@ -106,16 +99,13 @@ fn file_input_explicit_codex_projects_and_records_exec() { assert_eq!(cap.args[0], "resume"); assert!(!cap.args[1].is_empty()); - let sessions = std::env::var_os("HOME") - .map(|h| std::path::PathBuf::from(h).join(".codex/sessions")) - .unwrap(); + let sessions = home.home_dir().join(".codex/sessions"); assert!(sessions.exists(), "codex sessions dir not created"); } #[test] fn file_input_explicit_copilot_projects_and_records_exec() { - let _env = env_lock(); - let home = ScopedHome::new(); + let home = TestHome::new(); let bin = fake_bin_dir(&["copilot"]); let cwd = tempfile::tempdir().unwrap(); @@ -138,9 +128,7 @@ fn file_input_explicit_copilot_projects_and_records_exec() { assert!(!cap.args[1].is_empty()); // A session-state//events.jsonl was projected under the temp ~/.copilot. - let state = std::env::var_os("HOME") - .map(|h| std::path::PathBuf::from(h).join(".copilot/session-state")) - .unwrap(); + let state = home.home_dir().join(".copilot/session-state"); assert!(state.exists(), "copilot session-state dir not created"); let has_events = std::fs::read_dir(&state) .unwrap() @@ -151,8 +139,7 @@ fn file_input_explicit_copilot_projects_and_records_exec() { #[test] fn file_input_explicit_opencode_projects_and_records_exec() { - let _env = env_lock(); - let home = ScopedHome::new(); + let home = TestHome::new(); let bin = fake_bin_dir(&["opencode"]); let cwd = tempfile::tempdir().unwrap(); @@ -221,8 +208,7 @@ fn file_input_explicit_opencode_projects_and_records_exec() { #[test] fn file_input_explicit_pi_projects_and_records_exec() { - let _env = env_lock(); - let home = ScopedHome::new(); + let home = TestHome::new(); let bin = fake_bin_dir(&["pi"]); let cwd = tempfile::tempdir().unwrap(); @@ -243,9 +229,7 @@ fn file_input_explicit_pi_projects_and_records_exec() { assert_eq!(cap.args[0], "--session"); assert!(!cap.args[1].is_empty()); - let sessions = std::env::var_os("HOME") - .map(|h| std::path::PathBuf::from(h).join(".pi/agent/sessions")) - .unwrap(); + let sessions = home.home_dir().join(".pi/agent/sessions"); assert!(sessions.exists(), "pi sessions dir not created"); } @@ -253,16 +237,14 @@ fn file_input_explicit_pi_projects_and_records_exec() { #[test] fn cache_id_input_loads_and_projects() { - let _env = env_lock(); - let home = ScopedHome::new(); + let home = TestHome::new(); let bin = fake_bin_dir(&["claude"]); let cwd = tempfile::tempdir().unwrap(); // Seed a cache entry by writing the graph to - // /documents/.json directly. + // /documents/.json directly. let cache_id = "claude-resume-cache-test"; - let documents = std::path::PathBuf::from(std::env::var_os("TOOLPATH_CONFIG_DIR").unwrap()) - .join("documents"); + let documents = home.config_dir().join("documents"); std::fs::create_dir_all(&documents).unwrap(); let graph = toolpath::v1::Graph::from_path(make_convo_path( "agent:claude-code", @@ -301,8 +283,7 @@ fn cache_id_input_loads_and_projects() { #[test] fn multi_path_graph_returns_clear_error() { - let _env = env_lock(); - let home = ScopedHome::new(); + let home = TestHome::new(); let bin = fake_bin_dir(&["claude"]); let cwd = tempfile::tempdir().unwrap(); @@ -336,8 +317,7 @@ fn multi_path_graph_returns_clear_error() { #[test] fn agentless_path_returns_clear_error() { - let _env = env_lock(); - let home = ScopedHome::new(); + let home = TestHome::new(); let bin = fake_bin_dir(&["claude"]); let cwd = tempfile::tempdir().unwrap(); @@ -358,8 +338,7 @@ fn agentless_path_returns_clear_error() { #[test] fn explicit_harness_not_on_path_errors() { - let _env = env_lock(); - let home = ScopedHome::new(); + let home = TestHome::new(); let cwd = tempfile::tempdir().unwrap(); let path = make_convo_path("agent:claude-code", "claude-code://no-binary"); diff --git a/crates/path-cli/tests/support/mod.rs b/crates/path-cli/tests/support/mod.rs index cf62ad45..bdd39bf8 100644 --- a/crates/path-cli/tests/support/mod.rs +++ b/crates/path-cli/tests/support/mod.rs @@ -3,78 +3,56 @@ //! These are NOT integration-test entry points — they're a support //! module imported by `tests/resume.rs`. Lives under `tests/` so it //! doesn't leak into the production library API. +//! +//! [`TestHome`] is the sandbox: a tempdir plus the `Config` that points +//! at it. Tests pass that `Config` to the code under test, so no test +//! reads or mutates the process environment. #![allow(dead_code)] use std::collections::HashMap; -use std::ffi::OsString; use std::path::{Path, PathBuf}; -use std::sync::{Mutex, OnceLock}; use path_cli::cmd_resume::ResumeArgs; use path_cli::config::Config; use path_cli::harness::Harness; -/// Process-wide lock for tests that mutate `$HOME` or -/// `$TOOLPATH_CONFIG_DIR`. Integration tests under `tests/resume.rs` -/// can't reach the library's internal `crate::config::TEST_ENV_LOCK`, -/// so we use a separate lock here. Crucially, no library test holds -/// this lock — but library tests now properly save+restore env vars -/// (see commit 23deeb2), so the integration suite can be self-isolating. -pub fn env_lock() -> std::sync::MutexGuard<'static, ()> { - static LOCK: OnceLock> = OnceLock::new(); - LOCK.get_or_init(|| Mutex::new(())) - .lock() - .unwrap_or_else(|e| e.into_inner()) -} - -/// RAII guard that pins `$HOME` and `$TOOLPATH_CONFIG_DIR` to a tempdir. -pub struct ScopedHome { - _td: tempfile::TempDir, - prev_home: Option, - prev_config: Option, +/// A tempdir that stands in for the user's home directory. +pub struct TestHome { + td: tempfile::TempDir, } -impl ScopedHome { +impl TestHome { pub fn new() -> Self { - let td = tempfile::tempdir().unwrap(); - let prev_home = std::env::var_os("HOME"); - let prev_config = std::env::var_os("TOOLPATH_CONFIG_DIR"); - unsafe { - std::env::set_var("HOME", td.path()); - std::env::set_var("TOOLPATH_CONFIG_DIR", td.path().join(".toolpath")); - } Self { - _td: td, - prev_home, - prev_config, + td: tempfile::tempdir().unwrap(), } } pub fn home_dir(&self) -> PathBuf { - PathBuf::from(self._td.path()) + self.td.path().to_path_buf() + } + + /// The toolpath config directory inside the sandbox. + pub fn config_dir(&self) -> PathBuf { + self.td.path().join(".toolpath") } - /// The `Config` the CLI extracts at its composition root. Loaded - /// under this guard, so every path it carries points into the - /// sandbox. + /// The `Config` the code under test receives. Every other field + /// stays `None`, so all seven harness resolvers root under the + /// sandbox home whatever the developer's environment holds. pub fn config(&self) -> Config { - Config::load().expect("load config") + Config { + home: Some(self.home_dir()), + toolpath_config_dir: Some(self.config_dir()), + ..Config::default() + } } } -impl Drop for ScopedHome { - fn drop(&mut self) { - unsafe { - match &self.prev_home { - Some(v) => std::env::set_var("HOME", v), - None => std::env::remove_var("HOME"), - } - match &self.prev_config { - Some(v) => std::env::set_var("TOOLPATH_CONFIG_DIR", v), - None => std::env::remove_var("TOOLPATH_CONFIG_DIR"), - } - } +impl Default for TestHome { + fn default() -> Self { + Self::new() } }