From d1eefd25cbba3e7ff6eb88be73fad494201c713e Mon Sep 17 00:00:00 2001 From: Val Alexander <68980965+BunsDev@users.noreply.github.com> Date: Tue, 21 Jul 2026 17:14:05 -0500 Subject: [PATCH 1/2] fix: make unknown-harness tests hermetic against ambient adapter env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two tests asserted that `hermes` is an unknown harness while reading real adapter config, so they failed on any host where a hermes adapter exists — notably under Coven Cave, which exports COVEN_HARNESS_ADAPTER_DIRS on every spawned session: - harness::tests::command_parts_reject_unknown_harnesses - api::tests::launch_request_with_unknown_harness_returns_400_upfront_no_session_row Fix: neutralize all three ambient adapter sources (external manifest env, external dirs env, COVEN_HOME trust store) in both tests, and introduce a crate-wide test_env module — one shared env lock + EnvVarGuard — replacing the per-module locks in harness.rs and main.rs that could not serialize cross-module env mutation (e.g. main.rs COVEN_HOME tests racing harness.rs adapter reads). lock_env() recovers from poisoning, so one failed env test reports one failure instead of a ~28-test PoisonError cascade. Verified with COVEN_HARNESS_ADAPTER_DIRS=~/.coven/adapters set: both tests red before, green after; full workspace suite green. Closes #442 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/coven-cli/src/api.rs | 12 +++ crates/coven-cli/src/harness.rs | 154 +++++++++++++------------------ crates/coven-cli/src/main.rs | 43 +-------- crates/coven-cli/src/test_env.rs | 55 +++++++++++ 4 files changed, 133 insertions(+), 131 deletions(-) create mode 100644 crates/coven-cli/src/test_env.rs diff --git a/crates/coven-cli/src/api.rs b/crates/coven-cli/src/api.rs index 0f4642f9..dec365df 100644 --- a/crates/coven-cli/src/api.rs +++ b/crates/coven-cli/src/api.rs @@ -6849,7 +6849,19 @@ mod tests { #[test] fn launch_request_with_unknown_harness_returns_400_upfront_no_session_row() -> anyhow::Result<()> { + // Harness validation reads ambient adapter config: the external + // manifest/dirs env vars plus the COVEN_HOME trust store. Neutralize + // all three under the crate-wide env lock so a host with a real + // hermes adapter installed cannot turn this 400 into a 201. + let _env = crate::test_env::lock_env(); + let _manifest = + crate::test_env::EnvVarGuard::remove(crate::harness::EXTERNAL_ADAPTER_MANIFEST_ENV); + let _dirs = crate::test_env::EnvVarGuard::remove(crate::harness::EXTERNAL_ADAPTER_DIRS_ENV); let temp_dir = tempfile::tempdir()?; + let _home = crate::test_env::EnvVarGuard::set( + "COVEN_HOME", + temp_dir.path().join("empty-coven-home"), + ); let project_root = temp_dir.path().join("repo"); std::fs::create_dir_all(&project_root)?; let runtime = RecordingRuntime::default(); diff --git a/crates/coven-cli/src/harness.rs b/crates/coven-cli/src/harness.rs index b2edbab2..c37729b6 100644 --- a/crates/coven-cli/src/harness.rs +++ b/crates/coven-cli/src/harness.rs @@ -1884,17 +1884,12 @@ fn executable_candidates<'a>( #[cfg(test)] mod tests { use super::*; + use crate::test_env::{lock_env, EnvVarGuard}; use std::fs; - use std::sync::{Mutex, OnceLock}; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; - fn env_lock() -> &'static Mutex<()> { - static LOCK: OnceLock> = OnceLock::new(); - LOCK.get_or_init(|| Mutex::new(())) - } - fn restore_adapter_manifest_env(previous: Option) { match previous { Some(value) => env::set_var(EXTERNAL_ADAPTER_MANIFEST_ENV, value), @@ -1909,38 +1904,6 @@ mod tests { } } - fn restore_env_var(name: &str, previous: Option) { - match previous { - Some(value) => env::set_var(name, value), - None => env::remove_var(name), - } - } - - struct EnvVarGuard { - name: &'static str, - previous: Option, - } - - impl EnvVarGuard { - fn set(name: &'static str, value: impl AsRef) -> Self { - let previous = env::var_os(name); - env::set_var(name, value); - Self { name, previous } - } - - fn remove(name: &'static str) -> Self { - let previous = env::var_os(name); - env::remove_var(name); - Self { name, previous } - } - } - - impl Drop for EnvVarGuard { - fn drop(&mut self) { - restore_env_var(self.name, self.previous.clone()); - } - } - #[test] fn executable_exists_in_paths_finds_matching_file() -> anyhow::Result<()> { let temp_dir = tempfile::tempdir()?; @@ -2113,7 +2076,7 @@ mod tests { fn command_parts_for_known_harnesses_append_interactive_prompt() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); assert_eq!( command_parts_for_harness("codex", "fix tests", HarnessLaunchMode::Interactive)?, ( @@ -2135,7 +2098,7 @@ mod tests { fn command_parts_for_known_harnesses_use_noninteractive_entrypoints() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); assert_eq!( command_parts_for_harness("codex", "fix tests", HarnessLaunchMode::NonInteractive)?, ( @@ -2168,7 +2131,7 @@ mod tests { fn dash_prefixed_prompts_stay_positional_behind_double_dash() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); // A prompt starting with `-` must never parse as harness flags. for mode in [ HarnessLaunchMode::Interactive, @@ -2230,10 +2193,16 @@ mod tests { } #[test] - fn command_parts_reject_unknown_harnesses() { - // Spec resolution reads the adapter env vars; hold the shared env - // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + fn command_parts_reject_unknown_harnesses() -> anyhow::Result<()> { + // Spec resolution reads ambient adapter config: the external manifest + // and dirs env vars plus the COVEN_HOME trust store. Neutralize all + // three under the shared env lock so an adapter installed on the host + // (e.g. a real hermes manifest) cannot make these lookups succeed. + let _guard = lock_env(); + let temp_dir = tempfile::tempdir()?; + let _manifest = EnvVarGuard::remove(EXTERNAL_ADAPTER_MANIFEST_ENV); + let _dirs = EnvVarGuard::remove(EXTERNAL_ADAPTER_DIRS_ENV); + let _home = EnvVarGuard::set("COVEN_HOME", temp_dir.path().join("empty-coven-home")); assert!( command_parts_for_harness("shell", "hello", HarnessLaunchMode::Interactive) .unwrap_err() @@ -2246,6 +2215,7 @@ mod tests { .to_string() .contains("unsupported harness") ); + Ok(()) } #[test] @@ -2362,7 +2332,7 @@ mod tests { GROK_BUILD_ADAPTER_MANIFEST, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let _manifest_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_MANIFEST_ENV); let _dirs_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_DIRS_ENV); let _coven_home_guard = EnvVarGuard::set("COVEN_HOME", &coven_home); @@ -2430,7 +2400,7 @@ mod tests { GROK_BUILD_ADAPTER_MANIFEST, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let _manifest_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_MANIFEST_ENV); let _dirs_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_DIRS_ENV); let _coven_home_guard = EnvVarGuard::set("COVEN_HOME", &coven_home); @@ -2470,7 +2440,7 @@ mod tests { }"#, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let previous = env::var_os(EXTERNAL_ADAPTER_MANIFEST_ENV); env::set_var(EXTERNAL_ADAPTER_MANIFEST_ENV, &manifest); let parts = @@ -2531,7 +2501,7 @@ mod tests { let manifest = temp_dir.path().join("streamy.json"); fs::write(&manifest, STREAMY_ADAPTER_MANIFEST)?; - let _guard = env_lock().lock().unwrap_or_else(|p| p.into_inner()); + let _guard = lock_env(); let previous = env::var_os(EXTERNAL_ADAPTER_MANIFEST_ENV); env::set_var(EXTERNAL_ADAPTER_MANIFEST_ENV, &manifest); let supports_stream = harness_supports_stream_mode("streamy"); @@ -2828,7 +2798,7 @@ mod tests { }"#, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let previous_manifest = env::var_os(EXTERNAL_ADAPTER_MANIFEST_ENV); let previous_dirs = env::var_os(EXTERNAL_ADAPTER_DIRS_ENV); env::remove_var(EXTERNAL_ADAPTER_MANIFEST_ENV); @@ -2884,7 +2854,7 @@ mod tests { manifest_for("Second"), )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let previous_manifest = env::var_os(EXTERNAL_ADAPTER_MANIFEST_ENV); let previous_dirs = env::var_os(EXTERNAL_ADAPTER_DIRS_ENV); env::remove_var(EXTERNAL_ADAPTER_MANIFEST_ENV); @@ -2933,7 +2903,7 @@ mod tests { }"#, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let previous_manifest = env::var_os(EXTERNAL_ADAPTER_MANIFEST_ENV); let previous_dirs = env::var_os(EXTERNAL_ADAPTER_DIRS_ENV); env::set_var(EXTERNAL_ADAPTER_MANIFEST_ENV, &manifest); @@ -3017,7 +2987,7 @@ mod tests { }"#, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let previous_manifest = env::var_os(EXTERNAL_ADAPTER_MANIFEST_ENV); let previous_dirs = env::var_os(EXTERNAL_ADAPTER_DIRS_ENV); env::set_var(EXTERNAL_ADAPTER_MANIFEST_ENV, &manifest); @@ -3109,7 +3079,7 @@ mod tests { fn claude_stream_launch_args_unchanged_after_declaration() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let (program, args) = command_parts_for_harness_with_conversation( "claude", "hello", @@ -3159,7 +3129,7 @@ mod tests { }"#, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let previous_manifest = env::var_os(EXTERNAL_ADAPTER_MANIFEST_ENV); let previous_dirs = env::var_os(EXTERNAL_ADAPTER_DIRS_ENV); env::remove_var(EXTERNAL_ADAPTER_MANIFEST_ENV); @@ -3187,7 +3157,7 @@ mod tests { fs::create_dir_all(&adapter_dir)?; fs::write(adapter_dir.join("hermes.json"), HERMES_ADAPTER_MANIFEST)?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let _manifest_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_MANIFEST_ENV); let _dirs_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_DIRS_ENV); let _coven_home_guard = EnvVarGuard::set("COVEN_HOME", &coven_home); @@ -3277,7 +3247,7 @@ mod tests { }"#, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let _manifest_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_MANIFEST_ENV); let _dirs_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_DIRS_ENV); let _coven_home_guard = EnvVarGuard::set("COVEN_HOME", &coven_home); @@ -3335,7 +3305,7 @@ mod tests { }"#, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let _manifest_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_MANIFEST_ENV); let _dirs_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_DIRS_ENV); let _coven_home_guard = @@ -3372,7 +3342,7 @@ mod tests { }"#, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let previous_manifest = env::var_os(EXTERNAL_ADAPTER_MANIFEST_ENV); let previous_dirs = env::var_os(EXTERNAL_ADAPTER_DIRS_ENV); env::set_var(EXTERNAL_ADAPTER_MANIFEST_ENV, &manifest); @@ -3408,7 +3378,7 @@ mod tests { fn claude_init_hint_attaches_session_id_flag_in_print_mode() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let hint = ConversationHint::Init { id: "abc-123".to_string(), }; @@ -3440,7 +3410,7 @@ mod tests { fn claude_resume_hint_attaches_resume_flag_in_print_mode() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let hint = ConversationHint::Resume { id: "abc-123".to_string(), }; @@ -3473,7 +3443,7 @@ mod tests { // `spawn_executable_for_platform("coven-code")` consults engine::resolve(), // which reads HOME/USERPROFILE. Pin env (and clear any managed-engine home) // so this deterministic-argv test does not race the managed-engine test. - let _guard = env_lock().lock().unwrap_or_else(|p| p.into_inner()); + let _guard = lock_env(); let empty = tempfile::tempdir()?; let _home_guard = EnvVarGuard::set("HOME", empty.path()); #[cfg(windows)] @@ -3507,7 +3477,7 @@ mod tests { #[test] fn coven_code_resume_hint_attaches_resume_flag_in_print_mode() -> anyhow::Result<()> { - let _guard = env_lock().lock().unwrap_or_else(|p| p.into_inner()); + let _guard = lock_env(); let empty = tempfile::tempdir()?; let _home_guard = EnvVarGuard::set("HOME", empty.path()); #[cfg(windows)] @@ -3543,7 +3513,7 @@ mod tests { fn interactive_mode_ignores_conversation_hint() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let hint = ConversationHint::Init { id: "abc-123".to_string(), }; @@ -3570,7 +3540,7 @@ mod tests { ) -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let hint = ConversationHint::Init { id: "abc-123".to_string(), }; @@ -3603,7 +3573,7 @@ mod tests { fn codex_resume_hint_uses_exec_resume_subcommand_with_id() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let hint = ConversationHint::Resume { id: "019e5998-7130-7872-8d96-a6b67c5b6406".to_string(), }; @@ -3638,7 +3608,7 @@ mod tests { fn claude_stream_mode_preserves_permission_prompts_by_default() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let (program, args) = command_parts_for_harness_with_conversation( "claude", "hello", @@ -3682,7 +3652,7 @@ mod tests { fn non_claude_harnesses_do_not_get_permission_bypass() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let (_, args) = command_parts_for_harness("codex", "fix tests", HarnessLaunchMode::NonInteractive)?; assert!(!args @@ -3767,7 +3737,7 @@ mod tests { fn codex_forwards_model_before_prompt_with_prefix_stripped() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let (program, args) = command_parts_for_harness_with_conversation( "codex", "fix tests", @@ -3839,7 +3809,7 @@ mod tests { fn claude_forwards_model_with_prefix_stripped() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let (_, args) = command_parts_for_harness_with_conversation( "claude", "hi", @@ -3868,7 +3838,7 @@ mod tests { fn claude_think_maps_to_effort_high() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let (_, args) = command_parts_for_harness_with_conversation( "claude", "hi", @@ -3897,7 +3867,7 @@ mod tests { fn claude_speed_maps_to_effort_levels() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let (_, fast_args) = command_parts_for_harness_with_conversation( "claude", "hi", @@ -3930,7 +3900,7 @@ mod tests { fn codex_ignores_think_and_speed_launch_hints() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let with_hints = command_parts_for_harness_with_conversation( "codex", "fix tests", @@ -3954,7 +3924,7 @@ mod tests { fn no_model_leaves_args_unchanged() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let with_model = command_parts_for_harness_with_conversation( "codex", "fix tests", @@ -4096,7 +4066,7 @@ mod tests { fn codex_forwards_add_dirs_before_prompt() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let add_dirs = vec!["/tmp/roots/a".to_string(), "/tmp/roots/b".to_string()]; let (_, args) = command_parts_for_harness_with_conversation( "codex", @@ -4131,7 +4101,7 @@ mod tests { fn claude_stream_mode_forwards_add_dirs() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); // Blank entries are skipped; real dirs forward as repeated // `--add-dir ` pairs ahead of the declared stream args, // matching the non-stream path (`HarnessCommandSpec::add_dir_args`). @@ -4170,7 +4140,7 @@ mod tests { fn claude_forwards_add_dirs_after_permission() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let add_dirs = vec!["/tmp/roots/a".to_string()]; let (_, args) = command_parts_for_harness_with_conversation( "claude", @@ -4238,7 +4208,7 @@ mod tests { fn codex_forwards_permission_before_prompt() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let (_, args) = command_parts_for_harness_with_conversation( "codex", "fix tests", @@ -4265,7 +4235,7 @@ mod tests { fn claude_forwards_permission_full_maps_to_bypass() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let (_, args) = command_parts_for_harness_with_conversation( "claude", "hi", @@ -4294,7 +4264,7 @@ mod tests { fn no_permission_leaves_args_unchanged() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let with_opt = command_parts_for_harness_with_conversation( "codex", "fix tests", @@ -4330,7 +4300,7 @@ mod tests { }"#, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let previous = env::var_os(EXTERNAL_ADAPTER_MANIFEST_ENV); env::set_var(EXTERNAL_ADAPTER_MANIFEST_ENV, &manifest); let parts = command_parts_for_harness_with_conversation( @@ -4382,7 +4352,7 @@ mod tests { }"#, )?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let previous = env::var_os(EXTERNAL_ADAPTER_MANIFEST_ENV); env::set_var(EXTERNAL_ADAPTER_MANIFEST_ENV, &manifest); let spec = configured_harness_specs()? @@ -4419,7 +4389,7 @@ mod tests { fn none_hint_matches_legacy_command_parts() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let with_none = command_parts_for_harness_with_conversation( "claude", "hello", @@ -4525,7 +4495,7 @@ mod tests { fn copilot_noninteractive_uses_prompt_flag_equals_form() -> anyhow::Result<()> { // Spec resolution reads the adapter env vars; hold the shared env // lock so a concurrent test's tempdir manifest never vanishes mid-read. - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); assert_eq!( command_parts_for_harness("copilot", "fix tests", HarnessLaunchMode::NonInteractive)?, ( @@ -4538,7 +4508,7 @@ mod tests { #[test] fn copilot_interactive_uses_interactive_flag_equals_form() -> anyhow::Result<()> { - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); assert_eq!( command_parts_for_harness("copilot", "polish ui", HarnessLaunchMode::Interactive)?, ( @@ -4551,7 +4521,7 @@ mod tests { #[test] fn copilot_dash_prefixed_prompt_stays_inside_flag_value() -> anyhow::Result<()> { - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); // A prompt starting with `-` must never parse as harness flags; the // `=` form binds it as the prompt flag's value. let (_, args) = command_parts_for_harness( @@ -4569,7 +4539,7 @@ mod tests { #[test] fn copilot_stream_mode_falls_back_to_noninteractive_one_shot() -> anyhow::Result<()> { - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); // Copilot has no stream-json stdin mode; a Stream request must build // the one-shot non-interactive command instead. assert!(!harness_supports_stream_mode("copilot")); @@ -4585,7 +4555,7 @@ mod tests { #[test] fn copilot_init_hint_preassigns_session_id_with_prompt_flag() -> anyhow::Result<()> { - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); assert!(harness_supports_preassigned_session_id("copilot")); let hint = ConversationHint::Init { id: "11111111-2222-4333-8444-555555555555".to_string(), @@ -4615,7 +4585,7 @@ mod tests { #[test] fn copilot_resume_hint_reuses_session_id_flag() -> anyhow::Result<()> { - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); // Copilot's `--resume` only binds its value as `--resume=`, which // the token-pair continuity form can't emit; `--session-id ` // resumes the same session (and self-heals to a fresh session with @@ -4668,7 +4638,7 @@ mod tests { #[test] fn copilot_launch_options_prepend_model_permission_dirs_and_effort() -> anyhow::Result<()> { - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let add_dirs = vec!["/tmp/extra".to_string()]; let (_, args) = command_parts_for_harness_with_conversation( "copilot", @@ -4706,7 +4676,7 @@ mod tests { #[test] fn copilot_identity_preamble_rides_inside_prompt_flag_value() -> anyhow::Result<()> { - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); // Copilot has no system-prompt flag, so familiar identity is injected // as a bracketed preamble inside the prompt flag's value. let familiar = FamiliarContext { @@ -4761,7 +4731,7 @@ mod tests { // not reliably change — so the managed-home path is not test-controllable // cross-platform. The override exercises the same is_executable() gate and // the same harness_available() code path. - let _guard = env_lock().lock().unwrap_or_else(|p| p.into_inner()); + let _guard = lock_env(); let _override_guard = EnvVarGuard::set("COVEN_ENGINE_BIN", &bin); assert!( diff --git a/crates/coven-cli/src/main.rs b/crates/coven-cli/src/main.rs index d8a96aa4..d1902c4a 100644 --- a/crates/coven-cli/src/main.rs +++ b/crates/coven-cli/src/main.rs @@ -41,6 +41,8 @@ mod repos_config; mod settings; mod store; mod stream_json; +#[cfg(test)] +mod test_env; mod theme; mod tui; mod verification; @@ -3956,6 +3958,7 @@ fn coven_store_path_if_exists() -> Result> { #[cfg(test)] mod tests { use super::*; + use crate::test_env::{lock_env, EnvVarGuard}; use crate::tui::cast::{ build_plan, parse_spell, CastHarness, CastIntent, CastRisk, CastStepKind, SafetyDecision, }; @@ -3974,44 +3977,6 @@ mod tests { MagicalTuiMove, MagicalTuiRequest, MAGICAL_TUI_MAX_INNER_WIDTH, }; use crossterm::event::KeyEventKind; - use std::sync::{Mutex, OnceLock}; - - fn env_lock() -> &'static Mutex<()> { - static LOCK: OnceLock> = OnceLock::new(); - LOCK.get_or_init(|| Mutex::new(())) - } - - fn restore_env_var(name: &str, previous: Option) { - match previous { - Some(value) => std::env::set_var(name, value), - None => std::env::remove_var(name), - } - } - - struct EnvVarGuard { - name: &'static str, - previous: Option, - } - - impl EnvVarGuard { - fn set(name: &'static str, value: impl AsRef) -> Self { - let previous = std::env::var_os(name); - std::env::set_var(name, value); - Self { name, previous } - } - - fn remove(name: &'static str) -> Self { - let previous = std::env::var_os(name); - std::env::remove_var(name); - Self { name, previous } - } - } - - impl Drop for EnvVarGuard { - fn drop(&mut self) { - restore_env_var(self.name, self.previous.clone()); - } - } #[test] fn tui_launcher_and_session_browser_are_owned_by_tui_modules() { @@ -5133,7 +5098,7 @@ mod tests { fn coven_home_uses_userprofile_when_home_is_missing() -> anyhow::Result<()> { let temp_dir = tempfile::tempdir()?; let user_profile = temp_dir.path().join("windows-user"); - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let _coven_home = EnvVarGuard::remove("COVEN_HOME"); let _home = EnvVarGuard::remove("HOME"); let _user_profile = EnvVarGuard::set("USERPROFILE", &user_profile); diff --git a/crates/coven-cli/src/test_env.rs b/crates/coven-cli/src/test_env.rs new file mode 100644 index 00000000..ce9feddb --- /dev/null +++ b/crates/coven-cli/src/test_env.rs @@ -0,0 +1,55 @@ +//! Test-only coordination for process-global environment mutation. +//! +//! `cargo test` runs every module's unit tests in one parallel process, so +//! per-module env locks cannot serialize cross-module access: a `main.rs` +//! test mutating `COVEN_HOME` under its own lock still races a `harness.rs` +//! test reading adapter config under a different lock. Every test that reads +//! or mutates env vars shared across modules (the adapter manifest/dirs vars, +//! `COVEN_HOME`, `HOME`, …) must hold [`lock_env`] instead. + +use std::ffi::{OsStr, OsString}; +use std::sync::{Mutex, MutexGuard, OnceLock, PoisonError}; + +/// The single crate-wide env mutex. +pub(crate) fn env_lock() -> &'static Mutex<()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) +} + +/// Acquire the crate-wide env lock, recovering from poisoning so a single +/// panicked env test reports one failure instead of cascading `PoisonError` +/// panics through every other env-touching test in the binary. +pub(crate) fn lock_env() -> MutexGuard<'static, ()> { + env_lock().lock().unwrap_or_else(PoisonError::into_inner) +} + +/// RAII guard that sets or removes one env var and restores the previous +/// value on drop, so a failed assertion cannot leak state into later tests. +/// Hold [`lock_env`] for the guard's whole lifetime. +pub(crate) struct EnvVarGuard { + name: &'static str, + previous: Option, +} + +impl EnvVarGuard { + pub(crate) fn set(name: &'static str, value: impl AsRef) -> Self { + let previous = std::env::var_os(name); + std::env::set_var(name, value); + Self { name, previous } + } + + pub(crate) fn remove(name: &'static str) -> Self { + let previous = std::env::var_os(name); + std::env::remove_var(name); + Self { name, previous } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + match self.previous.take() { + Some(value) => std::env::set_var(self.name, value), + None => std::env::remove_var(self.name), + } + } +} From 0bfcac87a212c05d61f7907cefc3f9e8c07f7925 Mon Sep 17 00:00:00 2001 From: Val Alexander <68980965+BunsDev@users.noreply.github.com> Date: Wed, 22 Jul 2026 04:57:52 -0500 Subject: [PATCH 2/2] fix: convert #443's hermes model test to the shared env lock PR #443 landed after this branch forked and added one more env_lock().lock().unwrap() call site in harness.rs, which fails to compile once this branch removes the per-module lock in favor of crate::test_env::lock_env(). Convert the missed call site; the guard usage is unchanged since the shared EnvVarGuard has the same API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/coven-cli/src/harness.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/coven-cli/src/harness.rs b/crates/coven-cli/src/harness.rs index 1daee5b8..ca61a50a 100644 --- a/crates/coven-cli/src/harness.rs +++ b/crates/coven-cli/src/harness.rs @@ -3158,7 +3158,7 @@ mod tests { fs::create_dir_all(&adapter_dir)?; fs::write(adapter_dir.join("hermes.json"), HERMES_ADAPTER_MANIFEST)?; - let _guard = env_lock().lock().unwrap(); + let _guard = lock_env(); let _manifest_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_MANIFEST_ENV); let _dirs_guard = EnvVarGuard::remove(EXTERNAL_ADAPTER_DIRS_ENV); let _coven_home_guard = EnvVarGuard::set("COVEN_HOME", &coven_home);