mirror of
https://github.com/openai/codex.git
synced 2026-10-25 18:28:38 +00:00
Fix env file hook CI failures
This commit is contained in:
@@ -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<String, String>) {
|
||||
/// 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<String, String>) -> Vec<String> {
|
||||
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<String, String>, line: &str) {
|
||||
fn apply_env_file_line(env: &mut HashMap<String, String>, line: &str) -> Option<String> {
|
||||
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<String, String>, contents: &str) {
|
||||
fn apply_env_file_lines(env: &mut HashMap<String, String>, contents: &str) -> Vec<String> {
|
||||
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()]
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3244,8 +3244,8 @@ impl Session {
|
||||
self.services.hooks.load_full()
|
||||
}
|
||||
|
||||
pub(crate) fn apply_hook_env_file(&self, env: &mut HashMap<String, String>) {
|
||||
self.services.hook_env_file.apply_to_env(env);
|
||||
pub(crate) fn apply_hook_env_file(&self, env: &mut HashMap<String, String>) -> Vec<String> {
|
||||
self.services.hook_env_file.apply_to_env(env)
|
||||
}
|
||||
|
||||
pub(crate) fn user_shell(&self) -> Arc<shell::Shell> {
|
||||
|
||||
@@ -53,6 +53,7 @@ struct RunExecLikeArgs {
|
||||
tracker: crate::tools::context::SharedTurnDiffTracker,
|
||||
call_id: String,
|
||||
shell_runtime_backend: ShellRuntimeBackend,
|
||||
snapshot_restore_env_keys: Vec<String>,
|
||||
}
|
||||
|
||||
async fn run_exec_like(args: RunExecLikeArgs) -> Result<FunctionToolOutput, FunctionCallError> {
|
||||
@@ -68,6 +69,7 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result<FunctionToolOutput, Func
|
||||
tracker,
|
||||
call_id,
|
||||
shell_runtime_backend,
|
||||
snapshot_restore_env_keys,
|
||||
} = args;
|
||||
|
||||
let Some(turn_environment) = turn.environments.primary() else {
|
||||
@@ -77,7 +79,12 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result<FunctionToolOutput, Func
|
||||
};
|
||||
let fs = turn_environment.environment.get_filesystem();
|
||||
|
||||
let explicit_env_overrides = turn.shell_environment_policy.r#set.clone();
|
||||
let mut explicit_env_overrides = turn.shell_environment_policy.r#set.clone();
|
||||
for key in snapshot_restore_env_keys {
|
||||
// The snapshot wrapper restores the live values for keys listed here after sourcing a
|
||||
// shell snapshot. Include hook env-file keys so persisted updates survive the snapshot.
|
||||
explicit_env_overrides.entry(key).or_default();
|
||||
}
|
||||
let exec_permission_approvals_enabled =
|
||||
session.features().enabled(Feature::ExecPermissionApprovals);
|
||||
let requested_additional_permissions = additional_permissions.clone();
|
||||
|
||||
@@ -42,6 +42,11 @@ pub struct ShellCommandHandler {
|
||||
options: ShellCommandHandlerOptions,
|
||||
}
|
||||
|
||||
pub(super) struct ShellCommandExecParams {
|
||||
pub(super) exec_params: ExecParams,
|
||||
pub(super) snapshot_restore_env_keys: Vec<String>,
|
||||
}
|
||||
|
||||
#[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<ExecParams, FunctionCallError> {
|
||||
) -> Result<ShellCommandExecParams, FunctionCallError> {
|
||||
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<ToolInvocation> 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<ToolInvocation> 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<ToolInvocation> 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)
|
||||
|
||||
@@ -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::<String>::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)
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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<String> = 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(),
|
||||
|
||||
Reference in New Issue
Block a user