From 4962d9dc6758f6c8fab2bfcdd199b9a6fb4178d4 Mon Sep 17 00:00:00 2001 From: Robert Queenin <2177841+ecalifornica@users.noreply.github.com> Date: Fri, 14 Aug 2026 11:50:36 -0400 Subject: [PATCH] refactor(cli): tests construct Config directly The path-cli integration tests build a Config value against a tempdir instead of mutating the process environment. The Config fields are public so a separate test crate can construct one; Config::load stays the only production constructor. ScopedHome, the test environment locks, and the environment reads in test assertions are deleted. Test assertions derive their expected paths from the tempdir. Every Config field outside home and toolpath_config_dir stays None, so all seven harness resolvers root under the sandbox home. figment::Jail restores the variables it sets and serializes its own tests, so the three Jail tests need no extra lock. --- crates/path-cli/src/config.rs | 47 +++++++----------- crates/path-cli/tests/resume.rs | 63 ++++++++--------------- crates/path-cli/tests/support/mod.rs | 74 ++++++++++------------------ 3 files changed, 66 insertions(+), 118 deletions(-) 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() } }