From 07d8d28ce32345e20301a87f32a5890136ca870e Mon Sep 17 00:00:00 2001 From: Robert Queenin <2177841+ecalifornica@users.noreply.github.com> Date: Thu, 13 Aug 2026 14:30:24 -0400 Subject: [PATCH] refactor(cli): remove the last environment reads below the composition root (#184) Delete the free `home_dir()` from config.rs. Its two callers take a `&Config` already: `cmd_share::bail_no_sessions` and `share_config::resolve_remote` now use `Config::home_dir()`. Thread `&Config` into `p derive` and delete its transitional `Config::load()`. `Config::load()` runs once, in `run()`. Test cleanup: - `sync::engine::tests::with_cfg` builds a `Config` only. It drops the `$TOOLPATH_CONFIG_DIR` write and the `TEST_ENV_LOCK` hold. - Two `cmd_resume` tests drop their `$TOOLPATH_CONFIG_DIR` write and their `TEST_ENV_LOCK` hold. `$PATH` is the only variable read below the root. It is the execution environment, not configuration. --- crates/path-cli/src/cmd_derive.rs | 9 ++++----- crates/path-cli/src/cmd_p.rs | 2 +- crates/path-cli/src/cmd_resume.rs | 28 ++++------------------------ crates/path-cli/src/cmd_share.rs | 22 ++++++++++------------ crates/path-cli/src/config.rs | 20 ++++++-------------- crates/path-cli/src/share_config.rs | 8 ++++++-- crates/path-cli/src/sync/engine.rs | 22 ++++------------------ 7 files changed, 35 insertions(+), 76 deletions(-) diff --git a/crates/path-cli/src/cmd_derive.rs b/crates/path-cli/src/cmd_derive.rs index 50413fe9..dc0b9445 100644 --- a/crates/path-cli/src/cmd_derive.rs +++ b/crates/path-cli/src/cmd_derive.rs @@ -7,16 +7,15 @@ use anyhow::Result; +use crate::config::Config; + pub use crate::cmd_import::ImportSource as DeriveSource; -pub fn run(source: DeriveSource, pretty: bool) -> Result<()> { +pub fn run(source: DeriveSource, pretty: bool, config: &Config) -> Result<()> { let args = crate::cmd_import::ImportArgs { source, force: false, no_cache: true, }; - // Transitional: `p derive` does not take `&Config` yet; load one - // for the import path. - let config = crate::config::Config::load()?; - crate::cmd_import::run(args, pretty, &config) + crate::cmd_import::run(args, pretty, config) } diff --git a/crates/path-cli/src/cmd_p.rs b/crates/path-cli/src/cmd_p.rs index 2d3b4df1..2d731ddd 100644 --- a/crates/path-cli/src/cmd_p.rs +++ b/crates/path-cli/src/cmd_p.rs @@ -108,7 +108,7 @@ pub fn run(command: PCommand, pretty: bool, config: &Config) -> Result<()> { PCommand::Render { format } => crate::cmd_render::run(format), PCommand::Merge { inputs, title } => crate::cmd_merge::run(inputs, title, pretty), PCommand::Validate { input } => crate::cmd_validate::run(input), - PCommand::Derive { source } => crate::cmd_derive::run(source, pretty), + PCommand::Derive { source } => crate::cmd_derive::run(source, pretty, config), PCommand::Project { target } => crate::cmd_project::run(target, config), PCommand::Incept { target } => crate::cmd_incept::run(target, config), PCommand::Track { op } => crate::cmd_track::run(op, pretty), diff --git a/crates/path-cli/src/cmd_resume.rs b/crates/path-cli/src/cmd_resume.rs index 8c51dbd4..21eed44d 100644 --- a/crates/path-cli/src/cmd_resume.rs +++ b/crates/path-cli/src/cmd_resume.rs @@ -817,17 +817,9 @@ mod tests { // input at a 500-erroring mock server (so any network round-trip // would surface as an error), and confirm resolve_input still // returns the cached graph. - let _env = crate::config::TEST_ENV_LOCK - .lock() - .unwrap_or_else(|e| e.into_inner()); - - // Pin TOOLPATH_CONFIG_DIR to a tempdir so we don't pollute the - // user's real cache. + // The `Config` pins the config dir to a tempdir, so the test + // never touches the user's real cache. let cfg_dir = tempfile::tempdir().unwrap(); - let prev_cfg = std::env::var_os("TOOLPATH_CONFIG_DIR"); - unsafe { - std::env::set_var("TOOLPATH_CONFIG_DIR", cfg_dir.path()); - } // Seed the cache with a codex-source graph. Cache id keys on the // graph UUID since Pathbase 1.1+ addresses graphs by UUID. @@ -869,26 +861,14 @@ mod tests { toolpath_config_dir: Some(cfg_dir.path().to_path_buf()), ..Config::default() }; - let result = resolve_input(&args, &config); - - // Restore env before asserting so a panic doesn't poison sibling tests. - unsafe { - match prev_cfg { - Some(v) => std::env::set_var("TOOLPATH_CONFIG_DIR", v), - None => std::env::remove_var("TOOLPATH_CONFIG_DIR"), - } - } - - let (g, harness) = result.expect("resolve_input should reuse cache without refetching"); + let (g, harness) = resolve_input(&args, &config) + .expect("resolve_input should reuse cache without refetching"); let _ = ensure_path_with_agent(&g).unwrap(); assert_eq!(harness, Some(Harness::Codex)); } #[test] fn resolve_input_unresolvable_errors_clearly() { - let _env = crate::config::TEST_ENV_LOCK - .lock() - .unwrap_or_else(|e| e.into_inner()); let args = ResumeArgs { input: "definitely/not/a/real/cache/id".to_string(), cwd: None, diff --git a/crates/path-cli/src/cmd_share.rs b/crates/path-cli/src/cmd_share.rs index 9f693443..751bb192 100644 --- a/crates/path-cli/src/cmd_share.rs +++ b/crates/path-cli/src/cmd_share.rs @@ -517,7 +517,7 @@ pub fn run(args: ShareArgs, config: &Config) -> Result<()> { let rows = gather_artifacts(&bundle, &cwd, harness, project_filter); if rows.is_empty() { - return bail_no_sessions(&bundle, project_filter); + return bail_no_sessions(&bundle, project_filter, config); } if !crate::fuzzy::available() { @@ -594,6 +594,7 @@ pub fn run(args: ShareArgs, config: &Config) -> Result<()> { fn bail_no_sessions( bundle: &HarnessBundle, project_filter: Option<&std::path::Path>, + config: &Config, ) -> Result<()> { if let Some(p) = project_filter { anyhow::bail!( @@ -605,35 +606,32 @@ fn bail_no_sessions( let mut summary = String::from("No agent sessions found.\n"); // Pad harness names so the path column lines up: "opencode:" is the // longest at 9 chars (8 + colon). - let home = crate::config::home_dir(); + let home = config.home_dir().map(std::path::PathBuf::as_path); summary.push_str(&format_status_line( "claude", - &harness_status_claude(bundle, home.as_deref()), + &harness_status_claude(bundle, home), )); summary.push_str(&format_status_line( "gemini", - &harness_status_gemini(bundle, home.as_deref()), + &harness_status_gemini(bundle, home), )); summary.push_str(&format_status_line( "codex", - &harness_status_codex(bundle, home.as_deref()), + &harness_status_codex(bundle, home), )); summary.push_str(&format_status_line( "copilot", - &harness_status_copilot(bundle, home.as_deref()), + &harness_status_copilot(bundle, home), )); summary.push_str(&format_status_line( "opencode", - &harness_status_opencode(bundle, home.as_deref()), + &harness_status_opencode(bundle, home), )); summary.push_str(&format_status_line( "cursor", - &harness_status_cursor(bundle, home.as_deref()), - )); - summary.push_str(&format_status_line( - "pi", - &harness_status_pi(bundle, home.as_deref()), + &harness_status_cursor(bundle, home), )); + summary.push_str(&format_status_line("pi", &harness_status_pi(bundle, home))); eprint!("{summary}"); anyhow::bail!("no shareable sessions"); } diff --git a/crates/path-cli/src/config.rs b/crates/path-cli/src/config.rs index c5fa3383..9f7cfc11 100644 --- a/crates/path-cli/src/config.rs +++ b/crates/path-cli/src/config.rs @@ -152,14 +152,6 @@ impl Config { } } -/// Cross-platform `$HOME` lookup matching the providers' internal helpers. -/// Returns `None` only when neither `$HOME` nor `$USERPROFILE` is set. -pub(crate) fn home_dir() -> Option { - std::env::var_os("HOME") - .or_else(|| std::env::var_os("USERPROFILE")) - .map(PathBuf::from) -} - /// Display `path` as `~/relative/part` when it's under `home`, otherwise /// return its absolute lossy form. Pure helper — does no filesystem I/O. pub(crate) fn home_relative(path: &std::path::Path, home: Option<&std::path::Path>) -> String { @@ -176,10 +168,10 @@ pub(crate) fn home_relative(path: &std::path::Path, home: Option<&std::path::Pat path.display().to_string() } -/// Shared lock for tests that manipulate `$TOOLPATH_CONFIG_DIR`. Every -/// test module that calls `set_var` / `remove_var` on this env var should -/// grab this lock first, otherwise parallel tests race and clobber each -/// other's directories. +/// 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(()); @@ -188,8 +180,8 @@ mod tests { use super::*; /// `figment::Jail` restores the variables it sets, but it serializes - /// only against other Jail tests. Hold `TEST_ENV_LOCK` too: other - /// test modules mutate `$HOME` / `$TOOLPATH_CONFIG_DIR` under that + /// only against other Jail tests. Hold `TEST_ENV_LOCK` too: the + /// `$PATH` guard in `cmd_resume` mutates the environment under that /// lock. // result_large_err: the Jail closure returns figment's own // 208-byte error type. diff --git a/crates/path-cli/src/share_config.rs b/crates/path-cli/src/share_config.rs index e54f75bb..98a83985 100644 --- a/crates/path-cli/src/share_config.rs +++ b/crates/path-cli/src/share_config.rs @@ -25,7 +25,7 @@ use anyhow::{Context, Result}; use serde::Deserialize; use std::path::{Path, PathBuf}; -use crate::config::{Config, home_dir, home_relative}; +use crate::config::{Config, home_relative}; use crate::remote::{RepoSpec, parse_remote}; /// A share remote resolved from config. `display` is the remote exactly @@ -48,7 +48,11 @@ pub(crate) fn resolve_remote( session_dir: &Path, ) -> Result> { let global = config.config_dir()?.join(crate::config::CONFIG_FILE_NAME); - resolve_remote_from(&global, home_dir().as_deref(), session_dir) + resolve_remote_from( + &global, + config.home_dir().map(PathBuf::as_path), + session_dir, + ) } fn resolve_remote_from( diff --git a/crates/path-cli/src/sync/engine.rs b/crates/path-cli/src/sync/engine.rs index 265e57d6..664107e3 100644 --- a/crates/path-cli/src/sync/engine.rs +++ b/crates/path-cli/src/sync/engine.rs @@ -478,33 +478,19 @@ fn save_manifest(config_dir: &Path, manifest: &Manifest) -> Result<()> { #[cfg(test)] mod tests { use super::*; - use crate::config::{CONFIG_DIR_ENV, TEST_ENV_LOCK}; use std::path::Path; - /// Run `f` with `$TOOLPATH_CONFIG_DIR` pinned to `/.toolpath`; - /// `f` receives the tempdir root for building provider fixtures and - /// a `Config` carrying the same root, and the config directory - /// itself. + /// Run `f` with a `Config` whose config dir is `/.toolpath`; + /// `f` also receives the tempdir root for building provider fixtures + /// and the config directory itself. fn with_cfg R, R>(f: F) -> R { - let _g = TEST_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let temp = tempfile::tempdir().unwrap(); let config_root = temp.path().join(".toolpath"); - let prev = std::env::var_os(CONFIG_DIR_ENV); - unsafe { - std::env::set_var(CONFIG_DIR_ENV, &config_root); - } let config = Config { toolpath_config_dir: Some(config_root.clone()), ..Config::default() }; - let result = f(temp.path(), &config, &config_root); - unsafe { - match prev { - Some(v) => std::env::set_var(CONFIG_DIR_ENV, v), - None => std::env::remove_var(CONFIG_DIR_ENV), - } - } - result + f(temp.path(), &config, &config_root) } fn write_claude_session(home: &Path, project_slug: &str, session: &str, prompt: &str) {