From 0258ba5d46f6b9c7cc1db98f9553bc6ae8c12526 Mon Sep 17 00:00:00 2001 From: Alex Mikheev Date: Mon, 31 Aug 2026 23:48:16 +0100 Subject: [PATCH] fix(agent): make user_prompt_submit_tests hermetic via TERRAPHIM_DEFAULT_DATA_PATH (Refs #144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The user-prompt-submit hook path uses LearningCaptureConfig::default() which resolves global_dir via dirs::data_dir(). On macOS/Windows that ignores XDG_DATA_HOME and returns $HOME/Library/Application Support, so the test that set HOME and XDG_DATA_HOME never found the file it expected — 3 of 4 tests have been failing on every non-Linux runner since the --lib-only regression of 2026-07-31. Production: - Honour TERRAPHIM_DEFAULT_DATA_PATH in Default::default() (matches the existing settings.toml field of the same name, bringing the hook path in line with every other terraphim-agent learn subcommand). - storage_location() short-circuits to global_dir when TERRAPHIM_DEFAULT_DATA_PATH is set, so a project-local .terraphim/ directory no longer wins. This is a strict extension: when the env var is unset, behaviour is identical. Test: - Rewrite user_prompt_submit_tests to use support::cli_test_env::{create_hermetic_root, set_hermetic_env}. Each test gets a unique temp root; the spawned subprocess inherits TERRAPHIM_DEFAULT_DATA_PATH; the test reads back from the same hermetic root the hook wrote to. - No mocks, no #[ignore], no timeout increases. The platform-specific dirs::data_dir() behaviour no longer matters. All 4 tests pass (3 previously failing + 1 already passing, now non-vacuous on macOS). --- crates/terraphim_agent/src/learnings/mod.rs | 28 +++++- .../tests/support/cli_test_env.rs | 22 ++++- .../tests/user_prompt_submit_tests.rs | 95 ++++++++++--------- 3 files changed, 94 insertions(+), 51 deletions(-) diff --git a/crates/terraphim_agent/src/learnings/mod.rs b/crates/terraphim_agent/src/learnings/mod.rs index 0a90508..1e1a9de 100644 --- a/crates/terraphim_agent/src/learnings/mod.rs +++ b/crates/terraphim_agent/src/learnings/mod.rs @@ -92,10 +92,19 @@ impl Default for LearningCaptureConfig { .join(".terraphim") .join("learnings"); - let global_dir = dirs::data_dir() - .unwrap_or_else(|| PathBuf::from("~/.local/share")) - .join("terraphim") - .join("learnings"); + // Honour TERRAPHIM_DEFAULT_DATA_PATH so tests can steer the storage + // location without depending on the platform-specific dirs::data_dir() + // behaviour (which ignores XDG_DATA_HOME on macOS/Windows). This brings + // the hook path into line with terraphim_settings::DeviceSettings, which + // already reads the same env var. Refs #144. + let global_dir = if let Ok(p) = std::env::var("TERRAPHIM_DEFAULT_DATA_PATH") { + PathBuf::from(p).join("terraphim").join("learnings") + } else { + dirs::data_dir() + .unwrap_or_else(|| PathBuf::from("~/.local/share")) + .join("terraphim") + .join("learnings") + }; Self { project_dir, @@ -122,8 +131,17 @@ impl LearningCaptureConfig { } } - /// Determine storage location based on availability + /// Determine storage location based on availability. + /// + /// When `TERRAPHIM_DEFAULT_DATA_PATH` is set (e.g. by hermetic tests via + /// `support::cli_test_env::create_hermetic_root`), it always wins — the + /// env var explicitly steers storage to a known location. Otherwise, + /// project directory takes precedence over the global fallback so the + /// in-repo `.terraphim/learnings/` is preferred. Refs #144. pub fn storage_location(&self) -> PathBuf { + if std::env::var_os("TERRAPHIM_DEFAULT_DATA_PATH").is_some() { + return self.global_dir.clone(); + } if self.project_dir.exists() || self .project_dir diff --git a/crates/terraphim_agent/tests/support/cli_test_env.rs b/crates/terraphim_agent/tests/support/cli_test_env.rs index 742400b..a3a566e 100644 --- a/crates/terraphim_agent/tests/support/cli_test_env.rs +++ b/crates/terraphim_agent/tests/support/cli_test_env.rs @@ -46,8 +46,28 @@ fn create_unique_test_root() -> Result { Ok(root) } -pub fn apply_hermetic_env(cmd: &mut Command) -> Result<()> { +/// Create a fresh, unique hermetic test root under `std::env::temp_dir()`. +/// Returns the root path so callers that need to read files written by the +/// spawned subprocess (e.g. `user_prompt_submit_tests` reading correction +/// files at `/data/terraphim/learnings/`) can locate them. Refs #144. +pub fn create_hermetic_root() -> Result { let root = create_unique_test_root()?; + let data_dir = root.join("data"); + fs::create_dir_all(&data_dir)?; + Ok(root) +} + +#[allow(dead_code)] +pub fn apply_hermetic_env(cmd: &mut Command) -> Result<()> { + let root = create_hermetic_root()?; + set_hermetic_env(cmd, &root) +} + +/// Apply the hermetic test environment rooted at `root` to `cmd`. Use this +/// when the caller needs to know the root path (e.g. to read files written +/// by the spawned subprocess). Refs #144. +#[allow(dead_code)] +pub fn set_hermetic_env(cmd: &mut Command, root: &PathBuf) -> Result<()> { let home_dir = root.join("home"); let xdg_config_home = home_dir.join(".config"); let terraphim_config_dir = xdg_config_home.join("terraphim"); diff --git a/crates/terraphim_agent/tests/user_prompt_submit_tests.rs b/crates/terraphim_agent/tests/user_prompt_submit_tests.rs index 7b64b14..30e4090 100644 --- a/crates/terraphim_agent/tests/user_prompt_submit_tests.rs +++ b/crates/terraphim_agent/tests/user_prompt_submit_tests.rs @@ -3,10 +3,20 @@ //! Tests that `terraphim-agent learn hook --learn-hook-type user-prompt-submit` //! correctly captures tool preference corrections from user prompts and writes //! `CorrectionType::ToolPreference` files. +//! +//! These tests are hermetic: each test steers the agent binary's data dir +//! through `TERRAPHIM_DEFAULT_DATA_PATH` (which the production hook honours +//! via `LearningCaptureConfig::default()`, Refs #144), so the test reads back +//! from the same path the hook writes to. This avoids platform-specific +//! `dirs::data_dir()` behaviour (macOS/Windows ignore `XDG_DATA_HOME`). + +mod support; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; +use support::cli_test_env::{create_hermetic_root, set_hermetic_env}; + fn agent_binary() -> String { if let Ok(bin) = std::env::var("TERRAPHIM_AGENT_BIN") { return bin; @@ -34,16 +44,23 @@ fn agent_binary() -> String { .to_string() } +/// Derive the learnings dir from the same env var the helper sets on the +/// spawned cmd. The hook (post-#144) uses this var via +/// `LearningCaptureConfig::default()` to compute `global_dir`. +fn hermetic_learnings_dir(root: &Path) -> PathBuf { + root.join("data").join("terraphim").join("learnings") +} + /// Run the user-prompt-submit hook with a JSON payload, returning whether it succeeded. -fn run_user_prompt_submit(binary: &str, prompt: &str, env_home: &str) -> bool { +fn run_user_prompt_submit(binary: &str, prompt: &str, root: &PathBuf) -> bool { let json = format!(r#"{{"user_prompt":"{}"}}"#, prompt); - let output = Command::new(binary) - .args(["learn", "hook", "--learn-hook-type", "user-prompt-submit"]) - .env("HOME", env_home) - .env("XDG_DATA_HOME", format!("{}/data", env_home)) + let mut cmd = Command::new(binary); + cmd.args(["learn", "hook", "--learn-hook-type", "user-prompt-submit"]) .stdin(Stdio::piped()) .stdout(Stdio::piped()) - .stderr(Stdio::piped()) + .stderr(Stdio::piped()); + set_hermetic_env(&mut cmd, root).expect("set hermetic env"); + let output = cmd .spawn() .expect("should spawn hook process") .communicate(json.into_bytes()) @@ -66,16 +83,12 @@ impl Communicate for std::process::Child { } } -/// Return all correction markdown files in the learnings directory. -fn find_correction_files(home: &str) -> Vec { - let learnings_dir = Path::new(home) - .join("data") - .join("terraphim") - .join("learnings"); +/// Return all correction markdown files under the hermetic learnings dir. +fn find_correction_files(learnings_dir: &Path) -> Vec { if !learnings_dir.exists() { return vec![]; } - std::fs::read_dir(&learnings_dir) + std::fs::read_dir(learnings_dir) .expect("should read learnings dir") .filter_map(|entry| entry.ok().map(|e| e.path())) .filter(|path| { @@ -86,28 +99,21 @@ fn find_correction_files(home: &str) -> Vec { .collect() } -/// Clear all correction files from a previous test run. -fn clear_correction_files(home: &str) { - for path in find_correction_files(home) { - let _ = std::fs::remove_file(path); - } -} - #[test] fn user_prompt_submit_use_instead_of_creates_tool_preference() { let binary = agent_binary(); - let tmp = tempfile::tempdir().expect("create temp dir"); - let home = tmp.path().to_string_lossy().to_string(); + let root = create_hermetic_root().expect("create hermetic root"); + let learnings = hermetic_learnings_dir(&root); - clear_correction_files(&home); - let success = run_user_prompt_submit(&binary, "use uv instead of pip", &home); + let success = run_user_prompt_submit(&binary, "use uv instead of pip", &root); assert!(success, "hook should exit 0"); - let files = find_correction_files(&home); + let files = find_correction_files(&learnings); assert_eq!( files.len(), 1, - "expected exactly one correction file, found: {:?}", + "expected exactly one correction file under {}, found: {:?}", + learnings.display(), files ); @@ -132,18 +138,18 @@ fn user_prompt_submit_use_instead_of_creates_tool_preference() { #[test] fn user_prompt_submit_use_not_creates_tool_preference() { let binary = agent_binary(); - let tmp = tempfile::tempdir().expect("create temp dir"); - let home = tmp.path().to_string_lossy().to_string(); + let root = create_hermetic_root().expect("create hermetic root"); + let learnings = hermetic_learnings_dir(&root); - clear_correction_files(&home); - let success = run_user_prompt_submit(&binary, "use cargo not make", &home); + let success = run_user_prompt_submit(&binary, "use cargo not make", &root); assert!(success, "hook should exit 0"); - let files = find_correction_files(&home); + let files = find_correction_files(&learnings); assert_eq!( files.len(), 1, - "expected exactly one correction file, found: {:?}", + "expected exactly one correction file under {}, found: {:?}", + learnings.display(), files ); @@ -168,18 +174,18 @@ fn user_prompt_submit_use_not_creates_tool_preference() { #[test] fn user_prompt_submit_prefer_over_creates_tool_preference() { let binary = agent_binary(); - let tmp = tempfile::tempdir().expect("create temp dir"); - let home = tmp.path().to_string_lossy().to_string(); + let root = create_hermetic_root().expect("create hermetic root"); + let learnings = hermetic_learnings_dir(&root); - clear_correction_files(&home); - let success = run_user_prompt_submit(&binary, "prefer bunx over npx", &home); + let success = run_user_prompt_submit(&binary, "prefer bunx over npx", &root); assert!(success, "hook should exit 0"); - let files = find_correction_files(&home); + let files = find_correction_files(&learnings); assert_eq!( files.len(), 1, - "expected exactly one correction file, found: {:?}", + "expected exactly one correction file under {}, found: {:?}", + learnings.display(), files ); @@ -204,17 +210,16 @@ fn user_prompt_submit_prefer_over_creates_tool_preference() { #[test] fn user_prompt_submit_personal_preference_does_not_capture() { let binary = agent_binary(); - let tmp = tempfile::tempdir().expect("create temp dir"); - let home = tmp.path().to_string_lossy().to_string(); + let root = create_hermetic_root().expect("create hermetic root"); + let learnings = hermetic_learnings_dir(&root); - clear_correction_files(&home); - let success = run_user_prompt_submit(&binary, "I prefer tea over coffee", &home); + let success = run_user_prompt_submit(&binary, "I prefer tea over coffee", &root); assert!(success, "hook should exit 0 (fail-open)"); - let files = find_correction_files(&home); + let files = find_correction_files(&learnings); assert!( files.is_empty(), "personal preference should NOT create a correction file, found: {:?}", files ); -} +} \ No newline at end of file