Skip to content

refactor(cli): tests construct Config directly #216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ecalifornica wants to merge 1 commit into from
Draft

refactor(cli): tests construct Config directly #216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 19 additions & 28 deletions crates/path-cli/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<PathBuf>,
pub appdata: Option<PathBuf>,
/// `$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<String>,
pub claude_cli_debug: Option<String>,
/// `$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<String>,
pub codex_rollout_strict: Option<String>,
/// `$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<String>,
pub copilot_events_strict: Option<String>,
/// `$COPILOT_HOME`: Copilot CLI session root override.
pub(crate) copilot_home: Option<PathBuf>,
pub copilot_home: Option<PathBuf>,
/// `$GITHUB_TOKEN`: GitHub API token (see `providers`).
pub(crate) github_token: Option<String>,
pub github_token: Option<String>,
/// `$HOME`: config-root fallback and the harness resolvers' root.
pub(crate) home: Option<PathBuf>,
pub home: Option<PathBuf>,
/// `$PATHBASE_URL`: Pathbase server override (see `cmd_pathbase`).
pub(crate) pathbase_url: Option<String>,
pub pathbase_url: Option<String>,
/// `$TOOLPATH_CONFIG_DIR`: overrides the `~/.toolpath` root.
pub(crate) toolpath_config_dir: Option<PathBuf>,
pub toolpath_config_dir: Option<PathBuf>,
/// `$TOOLPATH_QUERY_EXPLAIN`: query-planner diagnostics on stderr.
pub(crate) toolpath_query_explain: Option<String>,
pub toolpath_query_explain: Option<String>,
/// `$USERPROFILE`: Windows home, the fallback when `$HOME` is unset.
pub(crate) userprofile: Option<PathBuf>,
pub userprofile: Option<PathBuf>,
/// `$XDG_DATA_HOME`: opencode's data root (Linux).
pub(crate) xdg_data_home: Option<PathBuf>,
pub xdg_data_home: Option<PathBuf>,
}

/// [`Env`], with values emitted as verbatim strings.
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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();
Expand Down
63 changes: 21 additions & 42 deletions crates/path-cli/tests/resume.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]

Expand All @@ -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();

Expand All @@ -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"),
Expand All @@ -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();

Expand All @@ -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();

Expand All @@ -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();

Expand All @@ -138,9 +128,7 @@ fn file_input_explicit_copilot_projects_and_records_exec() {
assert!(!cap.args[1].is_empty());

// A session-state/<id>/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()
Expand All @@ -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();

Expand Down Expand Up @@ -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();

Expand All @@ -243,26 +229,22 @@ 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");
}

// ── Cache-id input ──────────────────────────────────────────────────

#[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
// <TOOLPATH_CONFIG_DIR>/documents/<id>.json directly.
// <config dir>/documents/<id>.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",
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand All @@ -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");
Expand Down
74 changes: 26 additions & 48 deletions crates/path-cli/tests/support/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Mutex<()>> = 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<OsString>,
prev_config: Option<OsString>,
/// 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()
}
}

Expand Down
Loading