diff --git a/src/client/common.rs b/src/client/common.rs index c24eebb..8f9c63f 100644 --- a/src/client/common.rs +++ b/src/client/common.rs @@ -579,9 +579,9 @@ fn llm_failed_extras( } extras.push(( "COYOTE_LLM_ERROR", - // `{:#}` renders the whole context chain, so the root cause survives - // the `context(...)` wrapping the client trait defaults apply. - err.map(|err| format!("{err:#}")) + // The immediate error only, matching COYOTE_TOOL_ERROR; the context + // chain below it stays out of the env. + err.map(|err| err.to_string()) .unwrap_or_else(|| "Aborted.".to_string()), )); @@ -1862,12 +1862,10 @@ mod tests { failed.envs.get("COYOTE_LLM_STATUS").map(String::as_str), Some("429") ); - assert!( - failed - .envs - .get("COYOTE_LLM_ERROR") - .unwrap() - .contains("error (status: 429)") + assert_eq!( + failed.envs.get("COYOTE_LLM_ERROR").map(String::as_str), + Some("Failed to call chat-completions api"), + "the env must carry the immediate error only, not the context chain" ); assert!(!failed.envs.contains_key("COYOTE_LLM_DURATION_MS")); } diff --git a/src/config/agent.rs b/src/config/agent.rs index c7ba01c..a76b9c1 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -1019,10 +1019,13 @@ impl AgentConfig { if let Some(v) = read_env_value::(&with_prefix("reasoning_effort")) { self.reasoning_effort = v; } - if let Ok(v) = env::var(with_prefix("global_tools")) - && let Ok(v) = serde_json::from_str(&v) - { - self.global_tools = v; + if let Ok(v) = env::var(with_prefix("global_tools")) { + match serde_json::from_str(&v) { + Ok(v) => self.global_tools = v, + Err(err) => { + debug!("Ignoring malformed global_tools env override for agent '{name}': {err}") + } + } } if let Ok(v) = env::var(with_prefix("global_hooks")) { match serde_json::from_str(&v) { diff --git a/src/config/bundles.rs b/src/config/bundles.rs index caa3ce2..c5309c9 100644 --- a/src/config/bundles.rs +++ b/src/config/bundles.rs @@ -241,6 +241,10 @@ impl BundleStore { same_name_other_source: None, }; + // Reserved names are the `--install-builtins` categories. `roles` is + // deliberately NOT among them: builtin roles are not installable that + // way (some are coupled to flag-specific coyote behavior), so the + // asymmetry with InstallFilter is intended. let reserved = AssetCategory::parse(&base).is_some(); let collision = self.source_of_other_bundle(&base, &canonical); if reserved || collision.is_some() { diff --git a/src/config/mod.rs b/src/config/mod.rs index 7cfed24..3ba4323 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -734,6 +734,11 @@ impl Config { if env::var_os(SANDBOX_ENV_FLAG).is_some() { if !config_path.exists() { + // Inspection-only flags are look-don't-touch: no first-run + // config creation, nothing written. + if info_flag { + return Ok(Self::default()); + } create_config_file(&config_path).await?; } @@ -748,6 +753,12 @@ impl Config { { Some(v) => (Self::load_dynamic(&v)?, String::new()), None => { + // Same look-don't-touch rule: never run the interactive + // first-run wizard (or write a config) for a flag that + // only inspects state. + if info_flag { + return Ok(Self::default()); + } if *IS_STDOUT_TERMINAL { create_config_file(&config_path).await?; } @@ -1640,6 +1651,60 @@ hooks: ); } + #[tokio::test] + #[serial_test::serial] + async fn info_flag_with_missing_config_skips_wizard_and_writes_nothing() { + let unique = SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(); + let tmp_dir = env::temp_dir().join(format!("coyote-info-bypass-{unique}")); + create_dir_all(&tmp_dir).unwrap(); + let _config_dir = crate::testing::EnvVarGuard::set(get_env_name("config_dir"), &tmp_dir); + let _config_file = crate::testing::EnvVarGuard::unset(get_env_name("config_file")); + let _sandbox = crate::testing::EnvVarGuard::unset(crate::sandbox::SANDBOX_ENV_FLAG); + let _provider = crate::testing::EnvVarGuard::unset(get_env_name("provider")); + let _platform = crate::testing::EnvVarGuard::unset(get_env_name("platform")); + + let result = Config::load_with_interpolation(true).await; + + let leftover: Vec<_> = std::fs::read_dir(&tmp_dir) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + let _ = std::fs::remove_dir_all(&tmp_dir); + result.expect("inspection flags must load a default config without a config file"); + assert!( + leftover.is_empty(), + "inspection flags must not write anything: {leftover:?}" + ); + } + + #[tokio::test] + #[serial_test::serial] + async fn sandbox_info_flag_with_missing_config_writes_nothing() { + let unique = SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(); + let tmp_dir = env::temp_dir().join(format!("coyote-sandbox-info-bypass-{unique}")); + create_dir_all(&tmp_dir).unwrap(); + let config_path = tmp_dir.join("config.yaml"); + let _config_file = + crate::testing::EnvVarGuard::set(get_env_name("config_file"), &config_path); + let _sandbox = crate::testing::EnvVarGuard::set(crate::sandbox::SANDBOX_ENV_FLAG, "1"); + + let result = Config::load_with_interpolation(true).await; + + let created = config_path.exists(); + let _ = std::fs::remove_dir_all(&tmp_dir); + result.expect("inspection flags must load a default config in sandbox mode too"); + assert!( + !created, + "inspection flags must not create the sandbox config file" + ); + } + #[test] fn unloadable_override_is_stale() { assert!(models_override_is_stale(false, Some(SystemTime::now()), 24)); diff --git a/src/config/tool_scope.rs b/src/config/tool_scope.rs index d106ae3..eee141f 100644 --- a/src/config/tool_scope.rs +++ b/src/config/tool_scope.rs @@ -918,7 +918,7 @@ mod tests { use super::*; use crate::config::mcp_tool_policy::LayerSource; use crate::function::ToolCall; - use crate::testing::{install_warn_collector, warn_messages}; + use crate::testing::{install_log_collector, warn_snapshot}; use std::sync::atomic::Ordering; #[test] @@ -1083,7 +1083,7 @@ mod tests { #[tokio::test] async fn catalog_items_warns_when_resource_listing_fails() { - install_warn_collector(); + install_log_collector(); let fixture = FixtureServer { resources_capability: true, fail_resource_listings: true, @@ -1093,7 +1093,7 @@ mod tests { runtime.catalog_items("fixture").await.unwrap(); - let messages = warn_messages().lock().unwrap(); + let messages = warn_snapshot(); assert!( messages .iter() @@ -1547,7 +1547,7 @@ mod tests { #[tokio::test] async fn prompt_catalog_degrades_when_one_server_fails() { - install_warn_collector(); + install_log_collector(); let healthy = FixtureServer { prompts_capability: true, ..Default::default() @@ -1564,7 +1564,7 @@ mod tests { assert_eq!(items.len(), 1); assert_eq!(items[0].server, "fixture"); - let messages = warn_messages().lock().unwrap(); + let messages = warn_snapshot(); assert!( messages .iter() @@ -1794,7 +1794,7 @@ mod tests { #[tokio::test] async fn catalog_items_warns_on_dead_context_patterns() { - install_warn_collector(); + install_log_collector(); let (mut runtime, _server) = fixture_runtime(FixtureServer::default()).await; let mut filter = single_layer_filter(LayerSource::Global, &["*"]); filter.push_layer(LayerSource::Session, &["zzz_*".to_string()]); @@ -1802,7 +1802,7 @@ mod tests { runtime.catalog_items("fixture").await.unwrap(); - let messages = warn_messages().lock().unwrap(); + let messages = warn_snapshot(); assert!( messages.iter().any(|msg| msg.contains("'zzz_*'") && msg.contains("session (.set)") diff --git a/src/function/agents.rs b/src/function/agents.rs index 4e05b38..c3a22b6 100644 --- a/src/function/agents.rs +++ b/src/function/agents.rs @@ -4,7 +4,7 @@ use crate::client::{Model, ModelType, call_chat_completions}; use crate::config::{ Agent, AgentVariable, AgentVariables, AppState, Input, RequestContext, Role, RoleLike, default_max_agent_depth, effective_max_concurrent_jobs, jobs_enabled, - list_agents_with_descriptions, load_agent_variables, paths, + list_agents_with_descriptions, load_agent_variables, }; use crate::hooks::{self, HookEvent, ResolvedHook}; use crate::supervisor::mailbox::{Envelope, EnvelopePayload, Inbox, PeerRegistry, graph_agent_id}; @@ -15,7 +15,7 @@ use crate::utils::{AbortSignal, create_abort_signal, wait_abort_signal, wait_use use crate::graph; use crate::repl::DEFAULT_CONTINUATION_PROMPT; use anyhow::{Context, Result, anyhow, bail}; -use chrono::{SecondsFormat, Utc}; +use chrono::Utc; use indexmap::IndexMap; use log::{debug, warn}; use parking_lot::RwLock; @@ -997,36 +997,6 @@ fn sync_agent_functions_to_ctx(ctx: &mut RequestContext) -> Result<()> { Ok(()) } -/// Mirrors the base-env contract `hooks::fire` assembles from a live context, -/// for detached call sites that dispatch pre-resolved hook snapshots after -/// the context they were resolved from is gone. The names are captured by -/// the caller when it takes the snapshot; the timestamp is taken here, at -/// fire time, so it reflects when the event actually happened. -pub(crate) fn hook_base_envs( - event: HookEvent, - session_name: Option<&str>, - agent_name: Option<&str>, -) -> Vec<(String, String)> { - let mut envs = vec![ - ("COYOTE_EVENT".to_string(), event.as_str().to_string()), - ( - "COYOTE_CONFIG_DIR".to_string(), - paths::config_dir().display().to_string(), - ), - ( - "COYOTE_EVENT_TIMESTAMP".to_string(), - Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true), - ), - ]; - if let Some(name) = session_name { - envs.push(("COYOTE_SESSION_ID".to_string(), name.to_string())); - } - if let Some(name) = agent_name { - envs.push(("COYOTE_AGENT_NAME".to_string(), name.to_string())); - } - envs -} - /// Pre-resolved `agent.completed`/`agent.failed` snapshots for a spawned /// agent's result arms, captured from the child context before the spawn /// task takes ownership of it. Resolving against the child keeps global @@ -1070,7 +1040,7 @@ impl SpawnResultHooks { hooks::fire_resolved( event, resolved, - hook_base_envs(event, self.session_name.as_deref(), Some(&self.agent_name)), + hooks::base_envs_parts(event, self.session_name.as_deref(), Some(&self.agent_name)), &extras, None, ); @@ -2377,8 +2347,8 @@ mod tests { } #[test] - fn hook_base_envs_carries_optional_names_only_when_present() { - let envs = hook_base_envs(HookEvent::AgentCompleted, None, None); + fn agent_event_base_envs_carry_optional_names_only_when_present() { + let envs = hooks::base_envs_parts(HookEvent::AgentCompleted, None, None); let keys: Vec<&str> = envs.iter().map(|(key, _)| key.as_str()).collect(); assert!(keys.contains(&"COYOTE_EVENT")); assert!(keys.contains(&"COYOTE_CONFIG_DIR")); @@ -2390,7 +2360,7 @@ mod tests { .any(|(key, value)| key == "COYOTE_EVENT" && value == "agent.completed") ); - let envs = hook_base_envs(HookEvent::AgentFailed, Some("sess"), Some("bot")); + let envs = hooks::base_envs_parts(HookEvent::AgentFailed, Some("sess"), Some("bot")); assert!( envs.iter() .any(|(key, value)| key == "COYOTE_SESSION_ID" && value == "sess") diff --git a/src/function/jobs.rs b/src/function/jobs.rs index c9e9a3b..e5e2ac8 100644 --- a/src/function/jobs.rs +++ b/src/function/jobs.rs @@ -111,6 +111,20 @@ impl JobHookSnapshot { } } +/// Fires `job.started` for a freshly accepted job; both `handle_start` +/// branches (MCP and process) dispatch through here. +fn fire_job_started(ctx: &RequestContext, job_id: &str, tool: &str) { + hooks::fire( + HookEvent::JobStarted, + ctx, + &[ + ("COYOTE_JOB_ID", job_id.to_string()), + ("COYOTE_TOOL_NAME", tool.to_string()), + ], + None, + ); +} + pub struct RingBuf { buf: Vec, capacity: usize, @@ -529,15 +543,7 @@ async fn handle_start(ctx: &mut RequestContext, args: &Value) -> Result { let notify_id = job_id.clone(); let notify_tool = tool.clone(); let hook_snapshot = JobHookSnapshot::resolve(ctx, &job_id, &tool); - hooks::fire( - HookEvent::JobStarted, - ctx, - &[ - ("COYOTE_JOB_ID", job_id.clone()), - ("COYOTE_TOOL_NAME", tool.clone()), - ], - None, - ); + fire_job_started(ctx, &job_id, &tool); tokio::spawn(async move { let result = run_mcp_job(job_ctx, server, inner_tool, inner_args).await; let success = result.is_ok(); @@ -559,15 +565,7 @@ async fn handle_start(ctx: &mut RequestContext, args: &Value) -> Result { let notify_id = job_id.clone(); let notify_tool = tool.clone(); let hook_snapshot = JobHookSnapshot::resolve(ctx, &job_id, &tool); - hooks::fire( - HookEvent::JobStarted, - ctx, - &[ - ("COYOTE_JOB_ID", job_id.clone()), - ("COYOTE_TOOL_NAME", tool.clone()), - ], - None, - ); + fire_job_started(ctx, &job_id, &tool); tokio::spawn(async move { let result = run_process_job(snapshot, Arc::clone(&task_state), task_buf).await; let success = matches!(&result, Ok(job_result) if job_result.exit_code == Some(0)); diff --git a/src/function/mod.rs b/src/function/mod.rs index 38b6579..f8a1785 100644 --- a/src/function/mod.rs +++ b/src/function/mod.rs @@ -4795,6 +4795,27 @@ mod tests { fn built_shims_resolve_paths_at_runtime_after_relocation() { use std::os::unix::fs::PermissionsExt; + // A shim written moments ago can still be open for writing somewhere + // in the process (schedule-dependent), making exec fail with + // ETXTBSY; retry briefly instead of flaking. + fn output_with_etxtbsy_retry( + command: &mut process::Command, + ) -> std::io::Result { + let mut attempts = 0; + loop { + match command.output() { + Err(err) + if err.kind() == std::io::ErrorKind::ExecutableFileBusy + && attempts < 10 => + { + attempts += 1; + std::thread::sleep(Duration::from_millis(50)); + } + ret => return ret, + } + } + } + let root = temp_file("-shim-relocate-", ""); let home_a = root.join("home-a"); let config_a = home_a.join("coyote"); @@ -4858,14 +4879,14 @@ mod tests { return; } - let output = process::Command::new(&moved_shim) + let mut command = process::Command::new(&moved_shim); + command .arg("{}") .env_remove(&config_env) .env_remove(&functions_env) .env_remove("LLM_OUTPUT") - .env_remove("LLM_TOOL_DATA_FILE") - .output() - .unwrap(); + .env_remove("LLM_TOOL_DATA_FILE"); + let output = output_with_etxtbsy_retry(&mut command).unwrap(); let stdout = String::from_utf8_lossy(&output.stdout); assert!( output.status.success(), diff --git a/src/graph/agent.rs b/src/graph/agent.rs index edce152..d77066c 100644 --- a/src/graph/agent.rs +++ b/src/graph/agent.rs @@ -340,7 +340,7 @@ mod tests { use super::*; use crate::config::{AppState, WorkingMode, default_max_agent_depth}; use crate::supervisor::mailbox::{Inbox, PeerRegistry, graph_agent_id}; - use crate::testing::{install_warn_collector, warn_messages}; + use crate::testing::{install_log_collector, warn_snapshot}; use serde_json::json; use std::collections::HashMap; use std::sync::Arc; @@ -676,7 +676,7 @@ mod tests { #[tokio::test] async fn retry_transient_warns_once_per_retried_attempt_with_the_context_chain() { - install_warn_collector(); + install_log_collector(); let mut ctx = plain_ctx(); let node = retryable_node(3); let mut attempts = 0u32; @@ -700,12 +700,9 @@ mod tests { .expect_err("all attempts fail"); assert_eq!(attempts, 3); - let warns: Vec = warn_messages() - .lock() - .unwrap() - .iter() + let warns: Vec = warn_snapshot() + .into_iter() .filter(|m| m.contains("'warn_capture'")) - .cloned() .collect(); assert_eq!( warns.len(), @@ -861,7 +858,7 @@ mod tests { #[test] fn outcome_from_fallback_warn_names_the_node_and_the_fallback_target() { - install_warn_collector(); + install_log_collector(); let mut node = node_with("hi", None); node.fallback = Some("recover".into()); let mut state = manager_with(&[]); @@ -875,12 +872,9 @@ mod tests { .unwrap(); assert_eq!(outcome, AgentExecutionOutcome::FellBack("recover".into())); - let warns: Vec = warn_messages() - .lock() - .unwrap() - .iter() + let warns: Vec = warn_snapshot() + .into_iter() .filter(|m| m.contains("'fallback_warn_capture'")) - .cloned() .collect(); assert_eq!(warns.len(), 1, "exactly one fallback warn: {warns:?}"); assert!( diff --git a/src/graph/executor.rs b/src/graph/executor.rs index 686b74b..9f6b6e2 100644 --- a/src/graph/executor.rs +++ b/src/graph/executor.rs @@ -11,7 +11,6 @@ use super::user_interaction::{ApprovalNodeExecutor, InputNodeExecutor}; use super::validator::{AgentValidationContext, GraphValidator}; use super::wall_clock; use crate::config::{AgentVariable, AgentVariables, RenderMode, RequestContext}; -use crate::function::agents::hook_base_envs; use crate::hooks::{self, HookEvent, ResolvedHook}; use crate::supervisor::mailbox::{Inbox, PeerAssignment, PeerRegistry, graph_agent_id}; use crate::utils::{AbortSignal, wait_abort_signal, wait_user_interrupt}; @@ -550,7 +549,7 @@ impl NodeHookEmitter { hooks::fire_resolved( event, resolved.to_vec(), - hook_base_envs( + hooks::base_envs_parts( event, self.session_name.as_deref(), self.agent_name.as_deref(), @@ -1619,7 +1618,7 @@ nodes: /// debug log, and a well-formed sibling on the same event still fires. #[tokio::test] async fn malformed_graph_yaml_hook_is_skipped_without_breaking_the_node() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); let _sink = hooks::test_sink::install(); let mut ctx = make_ctx(); @@ -1660,10 +1659,7 @@ nodes: assert_eq!(valid.len(), 1, "{valid:?}"); assert_eq!(valid[0].envs["COYOTE_NODE_ID"], "done"); - let debugs: Vec = crate::testing::debug_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let debugs = crate::testing::debug_snapshot(); assert!( debugs .iter() diff --git a/src/hooks.rs b/src/hooks.rs index 4c97dee..78bf511 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -73,7 +73,6 @@ pub type HooksMap = IndexMap>; /// Every event hooks can be attached to. `as_str` yields the dotted name used /// as the key in `hooks:` maps and in `global_hooks` whitelist entries. -#[allow(dead_code)] #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum HookEvent { TurnStarted, @@ -150,7 +149,6 @@ impl RequestContext { /// when the active role was moved into (or restored by) a session, from /// the hooks snapshot the session captured via `Session::set_role` / /// `Session::load_from_ctx`. - #[allow(dead_code)] pub fn resolved_hooks(&self, event: HookEvent) -> Vec { resolve_hooks( event, @@ -248,7 +246,6 @@ fn push_defs(event_name: &str, defs: &[HookDef], cwd: &Path, out: &mut Vec, @@ -352,7 +348,6 @@ impl Drop for SpawnAckGuard { /// or until `timeout` elapses. For exit-path call sites only (session end / /// final turn): this waits for child spawn, never completion. Hook children /// deliberately outlive coyote as orphans. -#[allow(dead_code)] pub async fn drain_pending(timeout: Duration) { let deadline = tokio::time::Instant::now() + timeout; while PENDING_SPAWNS.load(Ordering::SeqCst) > 0 { @@ -379,6 +374,11 @@ fn base_envs(event: HookEvent, ctx: &RequestContext) -> Vec<(String, String)> { ) } +/// The single construction site for the base env set every hook receives. +/// [`base_envs`] feeds it from a live context; detached call sites that +/// dispatch pre-resolved snapshots after their context is gone pass the +/// names they captured with the snapshot. The timestamp is taken here, at +/// fire time, so it reflects when the event actually happened. pub(crate) fn base_envs_parts( event: HookEvent, session_name: Option<&str>, @@ -513,7 +513,6 @@ async fn run_hook( /// truncation pass, because that value is engine-generated, inherently short, /// and a truncated path would be corrupt rather than merely trimmed. Exported /// for call sites that need the bound before dispatch. -#[allow(dead_code)] pub fn truncate_env_value(value: &str) -> &str { const MAX_ENV_VALUE_BYTES: usize = 2048; if value.len() <= MAX_ENV_VALUE_BYTES { @@ -604,7 +603,6 @@ pub(crate) mod test_sink { /// A copy of everything recorded so far, for tests that filter by a hook /// name unique to their fixture. - #[allow(dead_code)] pub fn snapshot() -> Vec { CAPTURES .lock() @@ -649,19 +647,16 @@ mod tests { #[serial] fn install_builtin_hooks_installs_executable_scripts_and_honors_force() { let env_name = crate::utils::get_env_name("hooks_dir"); - let prev = env::var_os(&env_name); let unique = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap() .as_nanos(); let root = env::temp_dir().join(format!("coyote-hooks-install-{unique}")); - unsafe { - env::set_var(&env_name, &root); - } + let env_guard = crate::testing::EnvVarGuard::set(&env_name, &root); - // Capture every outcome first and assert only after the env var is - // restored, so a failed assertion cannot leave the hooks-dir override - // pointing at a deleted temp dir for subsequent tests. + // Capture every outcome first and assert only after cleanup: the + // guard restores the env var even on panic, but the temp dir removal + // below still has to run before any assertion can bail out. let notify = root.join("notify.sh"); let log_events = root.join("log-events.sh"); let fresh = install_builtin_hooks(false); @@ -680,12 +675,7 @@ mod tests { let force = install_builtin_hooks(true); let after_force = std::fs::read_to_string(¬ify); - unsafe { - match prev { - Some(v) => env::set_var(&env_name, v), - None => env::remove_var(&env_name), - } - } + drop(env_guard); let _ = std::fs::remove_dir_all(&root); fresh.unwrap(); @@ -897,7 +887,7 @@ mod tests { #[test] fn unknown_whitelist_entries_log_debug_and_stay_inert() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); let global = hooks_map("turn.started", &[("a", "cmd-a")]); let gate = vec![ "turn.started.a".to_string(), @@ -913,10 +903,7 @@ mod tests { ); assert_eq!(names(&resolved), ["a"]); - let debugs: Vec = crate::testing::debug_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let debugs = crate::testing::debug_snapshot(); assert!(debugs.iter().any(|message| { message.contains("turn.started.nonexistent-marker-xyz") && message.contains("for agent 'gate-agent-xyz'") @@ -925,7 +912,7 @@ mod tests { #[test] fn empty_global_map_short_circuits_whitelist_diagnostics() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); let global = HooksMap::default(); let role = hooks_map("turn.started", &[("role-only", "role-cmd")]); let gate = vec!["turn.started.x-shortcircuit-marker-p3q".to_string()]; @@ -939,10 +926,7 @@ mod tests { ); assert_eq!(names(&resolved), ["role-only"]); - let debugs: Vec = crate::testing::debug_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let debugs = crate::testing::debug_snapshot(); assert!( debugs .iter() @@ -1171,15 +1155,12 @@ mod tests { #[tokio::test(start_paused = true)] #[serial] async fn drain_pending_logs_when_the_timeout_expires() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); let pending = SpawnAckGuard::register(); drain_pending(Duration::from_millis(50)).await; - let debugs: Vec = crate::testing::debug_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let debugs = crate::testing::debug_snapshot(); assert!( debugs .iter() @@ -1206,7 +1187,7 @@ mod tests { #[test] #[serial] fn fire_resolved_without_runtime_skips_instead_of_panicking() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); let hook = ResolvedHook { name: "no-runtime-marker-a7c".to_string(), full_name: "turn.failed.no-runtime-marker-a7c".to_string(), @@ -1216,10 +1197,7 @@ mod tests { fire_resolved(HookEvent::TurnFailed, vec![hook], Vec::new(), &[], None); - let debugs: Vec = crate::testing::debug_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let debugs = crate::testing::debug_snapshot(); assert!(debugs.iter().any(|message| { message.contains("no-runtime-marker-a7c") && message.contains("no Tokio runtime on this thread") @@ -1229,7 +1207,7 @@ mod tests { #[test] #[serial] fn fire_without_runtime_skips_instead_of_panicking() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); let ctx = ctx_with_global_hooks(hooks_map( "turn.failed", &[("fire-no-runtime-marker-b8d", "true")], @@ -1237,10 +1215,7 @@ mod tests { fire(HookEvent::TurnFailed, &ctx, &[], None); - let debugs: Vec = crate::testing::debug_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let debugs = crate::testing::debug_snapshot(); assert!(debugs.iter().any(|message| { message.contains("fire-no-runtime-marker-b8d") && message.contains("no Tokio runtime on this thread") @@ -1249,7 +1224,7 @@ mod tests { #[test] fn debug_capture_excludes_non_hooks_targets() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); let hook = ResolvedHook { name: "included-marker-k2v".to_string(), full_name: "turn.failed.included-marker-k2v".to_string(), @@ -1267,10 +1242,7 @@ mod tests { ); fire_resolved(HookEvent::TurnFailed, vec![hook], Vec::new(), &[], None); - let debugs: Vec = crate::testing::debug_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let debugs = crate::testing::debug_snapshot(); assert!( debugs .iter() @@ -1291,7 +1263,7 @@ mod tests { #[test] #[serial] fn debug_capture_recovers_after_buffer_poisoning() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); std::thread::spawn(|| { let _held = crate::testing::debug_messages().lock().unwrap(); panic!("poison the debug buffer"); @@ -1342,27 +1314,21 @@ mod tests { use std::time::{Duration, SystemTime, UNIX_EPOCH}; struct TestConfigDirGuard { - key: String, - previous: Option, + _env: crate::testing::EnvVarGuard, path: PathBuf, } impl TestConfigDirGuard { fn new() -> Self { let key = get_env_name("config_dir"); - let previous = env::var_os(&key); let unique = SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap() .as_nanos(); let path = env::temp_dir().join(format!("coyote-hooks-tests-{unique}")); create_dir_all(&path).unwrap(); - unsafe { - env::set_var(&key, &path); - } Self { - key, - previous, + _env: crate::testing::EnvVarGuard::set(key, &path), path, } } @@ -1370,15 +1336,6 @@ mod tests { impl Drop for TestConfigDirGuard { fn drop(&mut self) { - if let Some(previous) = &self.previous { - unsafe { - env::set_var(&self.key, previous); - } - } else { - unsafe { - env::remove_var(&self.key); - } - } let _ = remove_dir_all(&self.path); } } @@ -1548,7 +1505,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] #[serial] async fn error_paths_log_debug_only() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); let empty_cwd = env::temp_dir(); let hooks = vec![ ResolvedHook { @@ -1568,10 +1525,7 @@ mod tests { fire_resolved(HookEvent::TurnFailed, hooks, Vec::new(), &[], None); drain_pending(Duration::from_secs(10)).await; - let debugs: Vec = crate::testing::debug_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let debugs = crate::testing::debug_snapshot(); let empty_cwd_display = empty_cwd.display().to_string(); assert!(debugs.iter().any(|message| { message.contains("empty-marker-f5b") @@ -1582,17 +1536,14 @@ mod tests { message.contains("Failed to spawn hook 'turn.failed.badcwd-marker-f5b'") && message.contains("/nonexistent/coyote-badcwd-marker-f5b") })); - let warns: Vec = crate::testing::warn_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let warns = crate::testing::warn_snapshot(); assert!(warns.iter().all(|message| !message.contains("marker-f5b"))); } #[tokio::test(flavor = "multi_thread")] #[serial] async fn failing_hook_never_disturbs_the_engine() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); let guard = TestConfigDirGuard::new(); let first = guard.path.join("first-out"); let second = guard.path.join("second-out"); @@ -1618,10 +1569,7 @@ mod tests { ); wait_for("second hook output", || second.exists()).await; - let warns: Vec = crate::testing::warn_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let warns = crate::testing::warn_snapshot(); assert!( warns .iter() @@ -1665,7 +1613,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] #[serial] async fn payload_write_failure_still_runs_hook_without_payload_env() { - crate::testing::install_warn_collector(); + crate::testing::install_log_collector(); let guard = TestConfigDirGuard::new(); let out = guard.path.join("env-out"); let _payload_dir = PayloadDirOverrideGuard::new(guard.path.join("missing-payload-dir")); @@ -1689,10 +1637,7 @@ mod tests { .lines() .all(|line| !line.starts_with("COYOTE_HOOK_PAYLOAD_FILE=")) ); - let debugs: Vec = crate::testing::debug_messages() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .clone(); + let debugs = crate::testing::debug_snapshot(); assert!(debugs.iter().any(|message| message.contains( "Failed to write payload file for hook 'tool.started.payload-fallback-marker'" ))); @@ -1729,6 +1674,43 @@ mod tests { assert_ne!(first_path, second_path); } + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn payload_paths_differ_within_one_fire() { + let guard = TestConfigDirGuard::new(); + let first = guard.path.join("first-path"); + let second = guard.path.join("second-path"); + let command_first = r#"printf '%s' "$COYOTE_HOOK_PAYLOAD_FILE" > "$HOOK_PATH_FIRST.tmp" && mv "$HOOK_PATH_FIRST.tmp" "$HOOK_PATH_FIRST""#; + let command_second = r#"printf '%s' "$COYOTE_HOOK_PAYLOAD_FILE" > "$HOOK_PATH_SECOND.tmp" && mv "$HOOK_PATH_SECOND.tmp" "$HOOK_PATH_SECOND""#; + let ctx = ctx_with_global_hooks(hooks_map( + "tool.started", + &[ + ("same-fire-a", command_first), + ("same-fire-b", command_second), + ], + )); + + fire( + HookEvent::ToolStarted, + &ctx, + &[ + ("HOOK_PATH_FIRST", first.display().to_string()), + ("HOOK_PATH_SECOND", second.display().to_string()), + ], + Some("{}".to_string()), + ); + wait_for("both payload paths", || first.exists() && second.exists()).await; + + let first_path = std::fs::read_to_string(&first).unwrap(); + let second_path = std::fs::read_to_string(&second).unwrap(); + assert!(!first_path.is_empty()); + assert!(!second_path.is_empty()); + assert_ne!( + first_path, second_path, + "each hook of one fire must get its own payload file" + ); + } + #[tokio::test] #[serial] async fn resolved_hooks_gates_globals_and_adds_agent_hooks_through_context() { diff --git a/src/testing.rs b/src/testing.rs index a41d69c..4442565 100644 --- a/src/testing.rs +++ b/src/testing.rs @@ -1,6 +1,7 @@ //! Test-only helpers shared across modules' unit tests. use log::{Level, LevelFilter, Log, Metadata, Record}; +use std::ffi::{OsStr, OsString}; use std::sync::{Mutex, Once, OnceLock}; struct TestLogCollector; @@ -44,6 +45,25 @@ pub(crate) fn debug_messages() -> &'static Mutex> { DEBUG_MESSAGES.get_or_init(Mutex::default) } +/// A copy of everything in [`warn_messages`], recovering from poisoning the +/// same way the collector itself does. +pub(crate) fn warn_snapshot() -> Vec { + snapshot_of(warn_messages()) +} + +/// A copy of everything in [`debug_messages`], recovering from poisoning the +/// same way the collector itself does. +pub(crate) fn debug_snapshot() -> Vec { + snapshot_of(debug_messages()) +} + +fn snapshot_of(buffer: &Mutex>) -> Vec { + buffer + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone() +} + impl Log for TestLogCollector { fn enabled(&self, metadata: &Metadata) -> bool { captures_warn(metadata) || captures_hooks_debug(metadata) @@ -68,15 +88,55 @@ impl Log for TestLogCollector { fn flush(&self) {} } -/// Installs the process-wide log collector. `log::set_logger` accepts one -/// logger per process, so every test that captures warns or hooks debug -/// output must install through this shared entry point. The max level is -/// Debug so the hooks debug capture sees its records; warn capture is -/// unaffected. -pub(crate) fn install_warn_collector() { +/// Installs the process-wide log collector capturing warn-level messages and +/// hooks-module debug messages. `log::set_logger` accepts one logger per +/// process, so every test that captures either stream must install through +/// this shared entry point. The max level is Debug so the hooks debug capture +/// sees its records; warn capture is unaffected. +pub(crate) fn install_log_collector() { static INSTALL: Once = Once::new(); INSTALL.call_once(|| { log::set_logger(&TestLogCollector).expect("no other logger should be installed"); log::set_max_level(LevelFilter::Debug); }); } + +/// Sets (or removes) an environment variable for the guard's lifetime and +/// restores the previous value on drop, including on panic, so a failing +/// assertion cannot leak the override into subsequent tests. Tests using it +/// must serialize (`#[serial]`) — the process environment is global. +pub(crate) struct EnvVarGuard { + key: String, + previous: Option, +} + +impl EnvVarGuard { + pub(crate) fn set(key: impl Into, value: impl AsRef) -> Self { + let key = key.into(); + let previous = std::env::var_os(&key); + unsafe { + std::env::set_var(&key, value); + } + Self { key, previous } + } + + pub(crate) fn unset(key: impl Into) -> Self { + let key = key.into(); + let previous = std::env::var_os(&key); + unsafe { + std::env::remove_var(&key); + } + Self { key, previous } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + unsafe { + match &self.previous { + Some(previous) => std::env::set_var(&self.key, previous), + None => std::env::remove_var(&self.key), + } + } + } +}