From 16190659f52c50f66f2c33cccf6bef1aad4c8883 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 26 May 2026 10:33:31 -0700 Subject: [PATCH] Fix env file hook CI failures --- codex-rs/core/src/hook_env.rs | 81 ++++++++++--------- codex-rs/core/src/session/mod.rs | 4 +- codex-rs/core/src/tools/handlers/shell.rs | 9 ++- .../src/tools/handlers/shell/shell_command.rs | 47 ++++++----- .../core/src/tools/handlers/shell_tests.rs | 11 ++- .../core/src/unified_exec/process_manager.rs | 2 +- codex-rs/hooks/src/engine/mod.rs | 17 +++- 7 files changed, 107 insertions(+), 64 deletions(-) diff --git a/codex-rs/core/src/hook_env.rs b/codex-rs/core/src/hook_env.rs index c74112a121..ade621f312 100644 --- a/codex-rs/core/src/hook_env.rs +++ b/codex-rs/core/src/hook_env.rs @@ -13,15 +13,17 @@ pub(crate) struct HookEnvFile { } impl HookEnvFile { - /// Creates the per-thread env file handle and makes its parent directory appendable for hooks. + /// Creates the per-thread env file handle without touching the filesystem. + /// + /// The hook runtime creates the parent directory only when it exposes this path to a + /// SessionStart hook. Later tool executions can still call `apply_to_env` safely when the + /// file was never created. pub(crate) fn new(codex_home: &AbsolutePathBuf, thread_id: ThreadId) -> Self { - let env_file = Self { + Self { path: codex_home .join(HOOK_ENV_DIR) .join(format!("{thread_id}.sh")), - }; - env_file.ensure_parent(); - env_file + } } /// Returns the path exposed to hook commands via CODEX_ENV_FILE and CLAUDE_ENV_FILE. @@ -29,33 +31,30 @@ impl HookEnvFile { &self.path } - /// Best-effort directory setup so a hook can append to the advertised env file path. - fn ensure_parent(&self) { - let Some(parent) = self.path.as_path().parent() else { - return; - }; - if let Err(err) = fs::create_dir_all(parent) { - warn!( - path = %parent.display(), - "failed to create hook env file directory: {err}" - ); - } - } - - /// Applies persisted hook env updates to the command environment used by later local tools. - pub(crate) fn apply_to_env(&self, env: &mut HashMap) { + /// Applies persisted hook env updates to later local tool environments. + /// + /// The returned names are the variables touched by the file. Shell-like tools use that list + /// to keep a shell snapshot from undoing hook updates such as `PATH` prepends. + pub(crate) fn apply_to_env(&self, env: &mut HashMap) -> Vec { match fs::read_to_string(self.path.as_path()) { Ok(contents) => { + let mut applied_names = Vec::new(); for line in contents.lines() { - apply_env_file_line(env, line); + if let Some(name) = apply_env_file_line(env, line) + && !applied_names.contains(&name) + { + applied_names.push(name); + } } + applied_names } - Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Vec::new(), Err(err) => { warn!( path = %self.path.display(), "failed to read hook env file: {err}" ); + Vec::new() } } } @@ -66,28 +65,24 @@ impl HookEnvFile { /// We intentionally support the common hook outputs here rather than sourcing /// the file in a hidden shell wrapper: `export NAME=value`, plus Bash's /// `declare -x NAME=value` form emitted by `export -p`. -fn apply_env_file_line(env: &mut HashMap, line: &str) { +fn apply_env_file_line(env: &mut HashMap, line: &str) -> Option { let line = line.trim(); if line.is_empty() || line.starts_with('#') { - return; + return None; } - let Some(assignment) = line + let assignment = line .strip_prefix("export ") - .or_else(|| line.strip_prefix("declare -x ")) - else { - return; - }; + .or_else(|| line.strip_prefix("declare -x "))?; let assignment = assignment.trim_start(); - let Some((name, value)) = assignment.split_once('=') else { - return; - }; + let (name, value) = assignment.split_once('=')?; let name = name.trim(); if !is_env_name(name) { - return; + return None; } let value = parse_env_value(value.trim_start(), env); env.insert(name.to_string(), value); + Some(name.to_string()) } /// Parses the value side of an env assignment, preserving single-quoted values literally. @@ -198,17 +193,21 @@ mod tests { use super::*; use pretty_assertions::assert_eq; - fn apply_env_file_lines(env: &mut HashMap, contents: &str) { + fn apply_env_file_lines(env: &mut HashMap, contents: &str) -> Vec { + let mut applied_names = Vec::new(); for line in contents.lines() { - apply_env_file_line(env, line); + if let Some(name) = apply_env_file_line(env, line) { + applied_names.push(name); + } } + applied_names } #[test] fn env_file_applies_exports_and_expands_path() { let mut env = HashMap::from([("PATH".to_string(), "/usr/bin".to_string())]); - apply_env_file_lines( + let applied_names = apply_env_file_lines( &mut env, r#" # ignored @@ -226,13 +225,17 @@ export LITERAL='$FOO' ("PATH".to_string(), "/plugin/bin:/usr/bin".to_string()), ]) ); + assert_eq!( + applied_names, + vec!["FOO".to_string(), "PATH".to_string(), "LITERAL".to_string()] + ); } #[test] fn env_file_supports_braced_references_and_declare_exports() { let mut env = HashMap::from([("BASE".to_string(), "base".to_string())]); - apply_env_file_lines( + let applied_names = apply_env_file_lines( &mut env, r#" declare -x FROM_DECLARE="${BASE}/declare" @@ -248,5 +251,9 @@ export FROM_BRACES=${FROM_DECLARE}/braces ("FROM_BRACES".to_string(), "base/declare/braces".to_string()), ]) ); + assert_eq!( + applied_names, + vec!["FROM_DECLARE".to_string(), "FROM_BRACES".to_string()] + ); } } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 796e822ae9..00d4a9b5a4 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3244,8 +3244,8 @@ impl Session { self.services.hooks.load_full() } - pub(crate) fn apply_hook_env_file(&self, env: &mut HashMap) { - self.services.hook_env_file.apply_to_env(env); + pub(crate) fn apply_hook_env_file(&self, env: &mut HashMap) -> Vec { + self.services.hook_env_file.apply_to_env(env) } pub(crate) fn user_shell(&self) -> Arc { diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 09eb29b1e4..1a49817c82 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -53,6 +53,7 @@ struct RunExecLikeArgs { tracker: crate::tools::context::SharedTurnDiffTracker, call_id: String, shell_runtime_backend: ShellRuntimeBackend, + snapshot_restore_env_keys: Vec, } async fn run_exec_like(args: RunExecLikeArgs) -> Result { @@ -68,6 +69,7 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result Result, +} + #[derive(Clone, Copy)] pub(crate) struct ShellCommandHandlerOptions { pub(crate) backend_config: ShellCommandBackendConfig, @@ -88,7 +93,7 @@ impl ShellCommandHandler { turn_context: &TurnContext, thread_id: ThreadId, allow_login_shell: bool, - ) -> Result { + ) -> Result { let shell = session.user_shell(); let use_login_shell = Self::resolve_use_login_shell(params.login, allow_login_shell)?; let command = Self::base_command(shell.as_ref(), ¶ms.command, use_login_shell); @@ -96,23 +101,26 @@ impl ShellCommandHandler { let cwd = turn_context.resolve_path(params.workdir.clone()); let mut env = create_env(&turn_context.shell_environment_policy, Some(thread_id)); - session.apply_hook_env_file(&mut env); + let snapshot_restore_env_keys = session.apply_hook_env_file(&mut env); - Ok(ExecParams { - command, - cwd, - expiration: params.timeout_ms.into(), - capture_policy: ExecCapturePolicy::ShellTool, - env, - network: turn_context.network.clone(), - sandbox_permissions: params.sandbox_permissions.unwrap_or_default(), - windows_sandbox_level: turn_context.windows_sandbox_level, - windows_sandbox_private_desktop: turn_context - .config - .permissions - .windows_sandbox_private_desktop, - justification: params.justification.clone(), - arg0: None, + Ok(ShellCommandExecParams { + exec_params: ExecParams { + command, + cwd, + expiration: params.timeout_ms.into(), + capture_policy: ExecCapturePolicy::ShellTool, + env, + network: turn_context.network.clone(), + sandbox_permissions: params.sandbox_permissions.unwrap_or_default(), + windows_sandbox_level: turn_context.windows_sandbox_level, + windows_sandbox_private_desktop: turn_context + .config + .permissions + .windows_sandbox_private_desktop, + justification: params.justification.clone(), + arg0: None, + }, + snapshot_restore_env_keys, }) } } @@ -177,7 +185,7 @@ impl ToolExecutor for ShellCommandHandler { ) .await; let prefix_rule = params.prefix_rule.clone(); - let exec_params = Self::to_exec_params( + let shell_exec_params = Self::to_exec_params( ¶ms, session.as_ref(), turn.as_ref(), @@ -187,7 +195,7 @@ impl ToolExecutor for ShellCommandHandler { let shell_type = Some(session.user_shell().shell_type.clone()); run_exec_like(RunExecLikeArgs { tool_name, - exec_params, + exec_params: shell_exec_params.exec_params, hook_command: params.command, shell_type, additional_permissions: params.additional_permissions.clone(), @@ -197,6 +205,7 @@ impl ToolExecutor for ShellCommandHandler { tracker, call_id, shell_runtime_backend: self.shell_runtime_backend(), + snapshot_restore_env_keys: shell_exec_params.snapshot_restore_env_keys, }) .await .map(boxed_tool_output) diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index eda23533a3..3c6f25f81b 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -106,7 +106,7 @@ async fn shell_command_handler_to_exec_params_uses_session_shell_and_turn_contex justification: justification.clone(), }; - let exec_params = ShellCommandHandler::to_exec_params( + let shell_exec_params = ShellCommandHandler::to_exec_params( ¶ms, &session, &turn_context, @@ -114,6 +114,7 @@ async fn shell_command_handler_to_exec_params_uses_session_shell_and_turn_contex /*allow_login_shell*/ true, ) .expect("login shells should be allowed"); + let exec_params = shell_exec_params.exec_params; // ExecParams cannot derive Eq due to the CancellationToken field, so we manually compare the fields. assert_eq!(exec_params.command, expected_command); @@ -124,6 +125,10 @@ async fn shell_command_handler_to_exec_params_uses_session_shell_and_turn_contex assert_eq!(exec_params.sandbox_permissions, sandbox_permissions); assert_eq!(exec_params.justification, justification); assert_eq!(exec_params.arg0, None); + assert_eq!( + shell_exec_params.snapshot_restore_env_keys, + Vec::::new() + ); } #[test] @@ -173,7 +178,7 @@ async fn shell_command_handler_defaults_to_non_login_when_disallowed() { justification: None, }; - let exec_params = ShellCommandHandler::to_exec_params( + let shell_exec_params = ShellCommandHandler::to_exec_params( ¶ms, &session, &turn_context, @@ -183,7 +188,7 @@ async fn shell_command_handler_defaults_to_non_login_when_disallowed() { .expect("non-login shells should still be allowed"); assert_eq!( - exec_params.command, + shell_exec_params.exec_params.command, session .user_shell() .derive_exec_args("echo hello", /*use_login_shell*/ false) diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index f0f2178d6a..ce0f273f61 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1011,7 +1011,7 @@ impl UnifiedExecProcessManager { "CODEX_ENV_FILE overlays are local-only; skipping persisted hook env for remote exec", ); } else { - context.session.apply_hook_env_file(&mut env); + let _ = context.session.apply_hook_env_file(&mut env); } env.insert( CODEX_THREAD_ID_ENV_VAR.to_string(), diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 2be268e0fb..51471b392b 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -132,11 +132,26 @@ impl ClaudeHooksEngine { ); let mut handlers = discovered.handlers; if let Some(env_file_path) = env_file_path { - let env_file_path = env_file_path.display().to_string(); + let mut env_file_path_for_handlers: Option = None; for handler in handlers .iter_mut() .filter(|handler| handler.event_name == HookEventName::SessionStart) { + let env_file_path = env_file_path_for_handlers + .get_or_insert_with(|| { + // Hook scripts append to CODEX_ENV_FILE directly, so create the parent + // directory only when the path is actually exposed to a SessionStart hook. + if let Some(parent) = env_file_path.as_path().parent() + && let Err(err) = std::fs::create_dir_all(parent) + { + tracing::warn!( + path = %parent.display(), + "failed to create hook env file directory: {err}" + ); + } + env_file_path.display().to_string() + }) + .clone(); handler.env.insert( crate::CODEX_ENV_FILE_ENV_VAR.to_string(), env_file_path.clone(),