From bacb92b1d7466dd26510ead04787034d41c1903a Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 13 Apr 2026 09:59:08 +0100 Subject: [PATCH] Build remote exec env from exec-server policy (#17216) ## Summary - add an exec-server `envPolicy` field; when present, the server starts from its own process env and applies the shell environment policy there - keep `env` as the exact environment for local/embedded starts, but make it an overlay for remote unified-exec starts - move the shell-environment-policy builder into `codex-config` so Core and exec-server share the inherit/filter/set/include behavior - overlay only runtime/sandbox/network deltas from Core onto the exec-server-derived env ## Why Remote unified exec was materializing the shell env inside Core and forwarding the whole map to exec-server, so remote processes could inherit the orchestrator machine's `HOME`, `PATH`, etc. This keeps the base env on the executor while preserving Core-owned runtime additions like `CODEX_THREAD_ID`, unified-exec defaults, network proxy env, and sandbox marker env. ## Validation - `just fmt` - `git diff --check` - `cargo test -p codex-exec-server --lib` - `cargo test -p codex-core --lib unified_exec::process_manager::tests` - `cargo test -p codex-core --lib exec_env::tests` - `cargo test -p codex-core --lib exec_env_tests` (compile-only; filter matched 0 tests) - `cargo test -p codex-config --lib shell_environment` (compile-only; filter matched 0 tests) - `just bazel-lock-update` ## Known local validation issue - `just bazel-lock-check` is not runnable in this checkout: it invokes `./scripts/check-module-bazel-lock.sh`, which is missing. --------- Co-authored-by: Codex Co-authored-by: pakrym-oai --- codex-rs/Cargo.lock | 1 + codex-rs/config/src/lib.rs | 1 + codex-rs/config/src/shell_environment.rs | 123 ++++++++++++++++++ codex-rs/config/src/types.rs | 2 +- codex-rs/core/src/exec.rs | 1 + codex-rs/core/src/exec_env.rs | 101 ++------------ codex-rs/core/src/sandboxing/mod.rs | 9 ++ codex-rs/core/src/tasks/user_shell.rs | 1 + .../tools/runtimes/shell/unix_escalation.rs | 2 + .../core/src/tools/runtimes/unified_exec.rs | 8 +- codex-rs/core/src/unified_exec/process.rs | 21 +-- .../core/src/unified_exec/process_manager.rs | 105 ++++++++++++--- .../src/unified_exec/process_manager_tests.rs | 86 ++++++++++++ .../core/src/unified_exec/process_tests.rs | 4 +- codex-rs/exec-server/Cargo.toml | 1 + codex-rs/exec-server/src/environment.rs | 1 + codex-rs/exec-server/src/lib.rs | 1 + codex-rs/exec-server/src/local_process.rs | 92 ++++++++++++- codex-rs/exec-server/src/protocol.rs | 13 ++ .../exec-server/src/server/handler/tests.rs | 1 + codex-rs/exec-server/src/server/processor.rs | 1 + codex-rs/exec-server/tests/exec_process.rs | 5 + 22 files changed, 455 insertions(+), 125 deletions(-) create mode 100644 codex-rs/config/src/shell_environment.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 728a404ce..ee8960d88 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2098,6 +2098,7 @@ dependencies = [ "async-trait", "base64 0.22.1", "codex-app-server-protocol", + "codex-config", "codex-protocol", "codex-sandboxing", "codex-utils-absolute-path", diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 1b178fcda..ffed56198 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -14,6 +14,7 @@ pub mod profile_toml; mod project_root_markers; mod requirements_exec_policy; pub mod schema; +pub mod shell_environment; mod skills_config; mod state; pub mod types; diff --git a/codex-rs/config/src/shell_environment.rs b/codex-rs/config/src/shell_environment.rs new file mode 100644 index 000000000..80fe0da42 --- /dev/null +++ b/codex-rs/config/src/shell_environment.rs @@ -0,0 +1,123 @@ +use crate::types::EnvironmentVariablePattern; +use crate::types::ShellEnvironmentPolicy; +use crate::types::ShellEnvironmentPolicyInherit; +use std::collections::HashMap; +use std::collections::HashSet; + +pub const CODEX_THREAD_ID_ENV_VAR: &str = "CODEX_THREAD_ID"; + +/// Construct a shell environment from the supplied process environment and +/// shell-environment policy. +pub fn create_env( + policy: &ShellEnvironmentPolicy, + thread_id: Option<&str>, +) -> HashMap { + create_env_from_vars(std::env::vars(), policy, thread_id) +} + +pub fn create_env_from_vars( + vars: I, + policy: &ShellEnvironmentPolicy, + thread_id: Option<&str>, +) -> HashMap +where + I: IntoIterator, +{ + let mut env_map = populate_env(vars, policy, thread_id); + + if cfg!(target_os = "windows") { + // This is a workaround to address the failures we are seeing in the + // following tests when run via Bazel on Windows: + // + // ``` + // suite::shell_command::unicode_output::with_login + // suite::shell_command::unicode_output::without_login + // ``` + // + // Currently, we can only reproduce these failures in CI, which makes + // iteration times long, so we include this quick fix for now to unblock + // getting the Windows Bazel build running. + if !env_map.keys().any(|k| k.eq_ignore_ascii_case("PATHEXT")) { + env_map.insert("PATHEXT".to_string(), ".COM;.EXE;.BAT;.CMD".to_string()); + } + } + env_map +} + +pub fn populate_env( + vars: I, + policy: &ShellEnvironmentPolicy, + thread_id: Option<&str>, +) -> HashMap +where + I: IntoIterator, +{ + // Step 1 - determine the starting set of variables based on the + // `inherit` strategy. + let mut env_map: HashMap = match policy.inherit { + ShellEnvironmentPolicyInherit::All => vars.into_iter().collect(), + ShellEnvironmentPolicyInherit::None => HashMap::new(), + ShellEnvironmentPolicyInherit::Core => { + let core_vars: HashSet<&str> = COMMON_CORE_VARS + .iter() + .copied() + .chain(PLATFORM_CORE_VARS.iter().copied()) + .collect(); + let is_core_var = |name: &str| { + if cfg!(target_os = "windows") { + core_vars + .iter() + .any(|allowed| allowed.eq_ignore_ascii_case(name)) + } else { + core_vars.contains(name) + } + }; + vars.into_iter().filter(|(k, _)| is_core_var(k)).collect() + } + }; + + // Internal helper - does `name` match any pattern in `patterns`? + let matches_any = |name: &str, patterns: &[EnvironmentVariablePattern]| -> bool { + patterns.iter().any(|pattern| pattern.matches(name)) + }; + + // Step 2 - Apply the default exclude if not disabled. + if !policy.ignore_default_excludes { + let default_excludes = vec![ + EnvironmentVariablePattern::new_case_insensitive("*KEY*"), + EnvironmentVariablePattern::new_case_insensitive("*SECRET*"), + EnvironmentVariablePattern::new_case_insensitive("*TOKEN*"), + ]; + env_map.retain(|k, _| !matches_any(k, &default_excludes)); + } + + // Step 3 - Apply custom excludes. + if !policy.exclude.is_empty() { + env_map.retain(|k, _| !matches_any(k, &policy.exclude)); + } + + // Step 4 - Apply user-provided overrides. + for (key, val) in &policy.r#set { + env_map.insert(key.clone(), val.clone()); + } + + // Step 5 - If include_only is non-empty, keep only the matching vars. + if !policy.include_only.is_empty() { + env_map.retain(|k, _| matches_any(k, &policy.include_only)); + } + + // Step 6 - Populate the thread ID environment variable when provided. + if let Some(thread_id) = thread_id { + env_map.insert(CODEX_THREAD_ID_ENV_VAR.to_string(), thread_id.to_string()); + } + + env_map +} + +const COMMON_CORE_VARS: &[&str] = &["PATH", "SHELL", "TMPDIR", "TEMP", "TMP"]; + +#[cfg(target_os = "windows")] +const PLATFORM_CORE_VARS: &[&str] = &["PATHEXT", "USERNAME", "USERPROFILE"]; + +#[cfg(unix)] +const PLATFORM_CORE_VARS: &[&str] = &["HOME", "LANG", "LC_ALL", "LC_CTYPE", "LOGNAME", "USER"]; diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index a1880c3be..64cfe85d0 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -658,7 +658,7 @@ impl From for codex_app_server_protocol::SandboxSettings } } -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)] +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Default, JsonSchema)] #[serde(rename_all = "kebab-case")] pub enum ShellEnvironmentPolicyInherit { /// "Core" environment variables for the platform. On UNIX, this would diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index af39657e0..fd1395d7b 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -349,6 +349,7 @@ pub(crate) async fn execute_exec_request( command, cwd, env, + exec_server_env_config: _, network, expiration, capture_policy, diff --git a/codex-rs/core/src/exec_env.rs b/codex-rs/core/src/exec_env.rs index a50fcc253..ad94bc51a 100644 --- a/codex-rs/core/src/exec_env.rs +++ b/codex-rs/core/src/exec_env.rs @@ -1,11 +1,10 @@ +#[cfg(test)] use codex_config::types::EnvironmentVariablePattern; use codex_config::types::ShellEnvironmentPolicy; -use codex_config::types::ShellEnvironmentPolicyInherit; use codex_protocol::ThreadId; use std::collections::HashMap; -use std::collections::HashSet; -pub const CODEX_THREAD_ID_ENV_VAR: &str = "CODEX_THREAD_ID"; +pub use codex_config::shell_environment::CODEX_THREAD_ID_ENV_VAR; /// Construct an environment map based on the rules in the specified policy. The /// resulting map can be passed directly to `Command::envs()` after calling @@ -21,9 +20,11 @@ pub fn create_env( policy: &ShellEnvironmentPolicy, thread_id: Option, ) -> HashMap { - create_env_from_vars(std::env::vars(), policy, thread_id) + let thread_id = thread_id.map(|thread_id| thread_id.to_string()); + codex_config::shell_environment::create_env(policy, thread_id.as_deref()) } +#[cfg(all(test, target_os = "windows"))] fn create_env_from_vars( vars: I, policy: &ShellEnvironmentPolicy, @@ -32,35 +33,11 @@ fn create_env_from_vars( where I: IntoIterator, { - let mut env_map = populate_env(vars, policy, thread_id); - - if cfg!(target_os = "windows") { - // This is a workaround to address the failures we are seeing in the - // following tests when run via Bazel on Windows: - // - // ``` - // suite::shell_command::unicode_output::with_login - // suite::shell_command::unicode_output::without_login - // ``` - // - // Currently, we can only reproduce these failures in CI, which makes - // iteration times long, so we include this quick fix for now to unblock - // getting the Windows Bazel build running. - if !env_map.keys().any(|k| k.eq_ignore_ascii_case("PATHEXT")) { - env_map.insert("PATHEXT".to_string(), ".COM;.EXE;.BAT;.CMD".to_string()); - } - } - env_map + let thread_id = thread_id.map(|thread_id| thread_id.to_string()); + codex_config::shell_environment::create_env_from_vars(vars, policy, thread_id.as_deref()) } -const COMMON_CORE_VARS: &[&str] = &["PATH", "SHELL", "TMPDIR", "TEMP", "TMP"]; - -#[cfg(target_os = "windows")] -const PLATFORM_CORE_VARS: &[&str] = &["PATHEXT", "USERNAME", "USERPROFILE"]; - -#[cfg(unix)] -const PLATFORM_CORE_VARS: &[&str] = &["HOME", "LANG", "LC_ALL", "LC_CTYPE", "LOGNAME", "USER"]; - +#[cfg(test)] fn populate_env( vars: I, policy: &ShellEnvironmentPolicy, @@ -69,66 +46,8 @@ fn populate_env( where I: IntoIterator, { - // Step 1 – determine the starting set of variables based on the - // `inherit` strategy. - let mut env_map: HashMap = match policy.inherit { - ShellEnvironmentPolicyInherit::All => vars.into_iter().collect(), - ShellEnvironmentPolicyInherit::None => HashMap::new(), - ShellEnvironmentPolicyInherit::Core => { - let core_vars: HashSet<&str> = COMMON_CORE_VARS - .iter() - .copied() - .chain(PLATFORM_CORE_VARS.iter().copied()) - .collect(); - let is_core_var = |name: &str| { - if cfg!(target_os = "windows") { - core_vars - .iter() - .any(|allowed| allowed.eq_ignore_ascii_case(name)) - } else { - core_vars.contains(name) - } - }; - vars.into_iter().filter(|(k, _)| is_core_var(k)).collect() - } - }; - - // Internal helper – does `name` match **any** pattern in `patterns`? - let matches_any = |name: &str, patterns: &[EnvironmentVariablePattern]| -> bool { - patterns.iter().any(|pattern| pattern.matches(name)) - }; - - // Step 2 – Apply the default exclude if not disabled. - if !policy.ignore_default_excludes { - let default_excludes = vec![ - EnvironmentVariablePattern::new_case_insensitive("*KEY*"), - EnvironmentVariablePattern::new_case_insensitive("*SECRET*"), - EnvironmentVariablePattern::new_case_insensitive("*TOKEN*"), - ]; - env_map.retain(|k, _| !matches_any(k, &default_excludes)); - } - - // Step 3 – Apply custom excludes. - if !policy.exclude.is_empty() { - env_map.retain(|k, _| !matches_any(k, &policy.exclude)); - } - - // Step 4 – Apply user-provided overrides. - for (key, val) in &policy.r#set { - env_map.insert(key.clone(), val.clone()); - } - - // Step 5 – If include_only is non-empty, keep *only* the matching vars. - if !policy.include_only.is_empty() { - env_map.retain(|k, _| matches_any(k, &policy.include_only)); - } - - // Step 6 – Populate the thread ID environment variable when provided. - if let Some(thread_id) = thread_id { - env_map.insert(CODEX_THREAD_ID_ENV_VAR.to_string(), thread_id.to_string()); - } - - env_map + let thread_id = thread_id.map(|thread_id| thread_id.to_string()); + codex_config::shell_environment::populate_env(vars, policy, thread_id.as_deref()) } #[cfg(test)] diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index 2377dbcaf..d4e83a663 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -33,11 +33,18 @@ pub(crate) struct ExecOptions { pub(crate) capture_policy: ExecCapturePolicy, } +#[derive(Clone, Debug)] +pub(crate) struct ExecServerEnvConfig { + pub(crate) policy: codex_exec_server::ExecEnvPolicy, + pub(crate) local_policy_env: HashMap, +} + #[derive(Debug)] pub struct ExecRequest { pub command: Vec, pub cwd: AbsolutePathBuf, pub env: HashMap, + pub(crate) exec_server_env_config: Option, pub network: Option, pub expiration: ExecExpiration, pub capture_policy: ExecCapturePolicy, @@ -72,6 +79,7 @@ impl ExecRequest { command, cwd, env, + exec_server_env_config: None, network, expiration, capture_policy, @@ -121,6 +129,7 @@ impl ExecRequest { command, cwd, env, + exec_server_env_config: None, network, expiration, capture_policy, diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index e0a770387..f90917587 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -162,6 +162,7 @@ pub(crate) async fn execute_user_shell_command( command: exec_command.clone(), cwd: cwd.clone(), env: exec_env_map, + exec_server_env_config: None, network: turn_context.network.clone(), // TODO(zhao-oai): Now that we have ExecExpiration::Cancellation, we // should use that instead of an "arbitrarily large" timeout here. diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 07c78d2e7..0bb4263d2 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -126,6 +126,7 @@ pub(super) async fn try_run_zsh_fork( command, cwd: sandbox_cwd, env: sandbox_env, + exec_server_env_config: _, network: sandbox_network, expiration: _sandbox_expiration, capture_policy: _capture_policy, @@ -734,6 +735,7 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { command: self.command.clone(), cwd: self.cwd.clone(), env: exec_env, + exec_server_env_config: None, network: self.network.clone(), expiration: ExecExpiration::Cancellation(cancel_rx), capture_policy: ExecCapturePolicy::ShellTool, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 94d370fbc..cb0f32876 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -10,6 +10,7 @@ use crate::exec::ExecExpiration; use crate::guardian::GuardianApprovalRequest; use crate::guardian::review_approval_request; use crate::sandboxing::ExecOptions; +use crate::sandboxing::ExecServerEnvConfig; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; use crate::tools::network_approval::NetworkApprovalMode; @@ -52,6 +53,7 @@ pub struct UnifiedExecRequest { pub process_id: i32, pub cwd: AbsolutePathBuf, pub env: HashMap, + pub exec_server_env_config: Option, pub explicit_env_overrides: HashMap, pub network: Option, pub tty: bool, @@ -237,9 +239,10 @@ impl<'a> ToolRuntime for UnifiedExecRunt expiration: ExecExpiration::DefaultTimeout, capture_policy: ExecCapturePolicy::ShellTool, }; - let exec_env = attempt + let mut exec_env = attempt .env_for(command, options, req.network.as_ref()) .map_err(|err| ToolError::Codex(err.into()))?; + exec_env.exec_server_env_config = req.exec_server_env_config.clone(); match zsh_fork_backend::maybe_prepare_unified_exec( req, attempt, @@ -294,9 +297,10 @@ impl<'a> ToolRuntime for UnifiedExecRunt expiration: ExecExpiration::DefaultTimeout, capture_policy: ExecCapturePolicy::ShellTool, }; - let exec_env = attempt + let mut exec_env = attempt .env_for(command, options, req.network.as_ref()) .map_err(|err| ToolError::Codex(err.into()))?; + exec_env.exec_server_env_config = req.exec_server_env_config.clone(); let Some(environment) = ctx.turn.environment.as_ref() else { return Err(ToolError::Rejected( "exec_command is unavailable in this session".to_string(), diff --git a/codex-rs/core/src/unified_exec/process.rs b/codex-rs/core/src/unified_exec/process.rs index 007214c25..62a60b0a8 100644 --- a/codex-rs/core/src/unified_exec/process.rs +++ b/codex-rs/core/src/unified_exec/process.rs @@ -66,10 +66,11 @@ pub(crate) struct OutputHandles { /// Transport-specific process handle used by unified exec. enum ProcessHandle { Local(Box), - Remote(Arc), + ExecServer(Arc), } -/// Unified wrapper over local PTY sessions and exec-server-backed processes. +/// Unified wrapper over directly spawned PTY sessions and exec-server-backed +/// processes. pub(crate) struct UnifiedExecProcess { process_handle: ProcessHandle, output_tx: broadcast::Sender>, @@ -135,7 +136,7 @@ impl UnifiedExecProcess { .send(data.to_vec()) .await .map_err(|_| UnifiedExecError::WriteToStdin), - ProcessHandle::Remote(process_handle) => { + ProcessHandle::ExecServer(process_handle) => { match process_handle.write(data.to_vec()).await { Ok(response) => match response.status { WriteStatus::Accepted => Ok(()), @@ -179,7 +180,7 @@ impl UnifiedExecProcess { let state = self.state_rx.borrow().clone(); match &self.process_handle { ProcessHandle::Local(process_handle) => state.has_exited || process_handle.has_exited(), - ProcessHandle::Remote(_) => state.has_exited, + ProcessHandle::ExecServer(_) => state.has_exited, } } @@ -189,7 +190,7 @@ impl UnifiedExecProcess { ProcessHandle::Local(process_handle) => { state.exit_code.or_else(|| process_handle.exit_code()) } - ProcessHandle::Remote(_) => state.exit_code, + ProcessHandle::ExecServer(_) => state.exit_code, } } @@ -198,7 +199,7 @@ impl UnifiedExecProcess { self.output_closed_notify.notify_waiters(); match &self.process_handle { ProcessHandle::Local(process_handle) => process_handle.terminate(), - ProcessHandle::Remote(process_handle) => { + ProcessHandle::ExecServer(process_handle) => { let process_handle = Arc::clone(process_handle); tokio::spawn(async move { let _ = process_handle.terminate().await; @@ -331,14 +332,14 @@ impl UnifiedExecProcess { Ok(managed) } - pub(super) async fn from_remote_started( + pub(super) async fn from_exec_server_started( started: StartedExecProcess, sandbox_type: SandboxType, ) -> Result { - let process_handle = ProcessHandle::Remote(Arc::clone(&started.process)); + let process_handle = ProcessHandle::ExecServer(Arc::clone(&started.process)); let mut managed = Self::new(process_handle, sandbox_type, /*spawn_lifecycle*/ None); let output_handles = managed.output_handles(); - managed.output_task = Some(Self::spawn_remote_output_task( + managed.output_task = Some(Self::spawn_exec_server_output_task( started, output_handles, managed.output_tx.clone(), @@ -366,7 +367,7 @@ impl UnifiedExecProcess { Ok(managed) } - fn spawn_remote_output_task( + fn spawn_exec_server_output_task( started: StartedExecProcess, output_handles: OutputHandles, output_tx: broadcast::Sender>, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 9d77ac4e9..20cc7c5f7 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -11,9 +11,11 @@ use tokio::time::Duration; use tokio::time::Instant; use tokio_util::sync::CancellationToken; +use crate::exec_env::CODEX_THREAD_ID_ENV_VAR; use crate::exec_env::create_env; use crate::exec_policy::ExecApprovalRequest; use crate::sandboxing::ExecRequest; +use crate::sandboxing::ExecServerEnvConfig; use crate::tools::context::ExecCommandToolOutput; use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; @@ -47,6 +49,7 @@ use crate::unified_exec::process::OutputBuffer; use crate::unified_exec::process::OutputHandles; use crate::unified_exec::process::SpawnLifecycleHandle; use crate::unified_exec::process::UnifiedExecProcess; +use codex_config::types::ShellEnvironmentPolicy; use codex_protocol::protocol::ExecCommandSource; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_output_truncation::approx_token_count; @@ -89,6 +92,70 @@ fn apply_unified_exec_env(mut env: HashMap) -> HashMap codex_exec_server::ExecEnvPolicy { + codex_exec_server::ExecEnvPolicy { + inherit: policy.inherit.clone(), + ignore_default_excludes: policy.ignore_default_excludes, + exclude: policy + .exclude + .iter() + .map(std::string::ToString::to_string) + .collect(), + r#set: policy.r#set.clone(), + include_only: policy + .include_only + .iter() + .map(std::string::ToString::to_string) + .collect(), + } +} + +fn env_overlay_for_exec_server( + request_env: &HashMap, + local_policy_env: &HashMap, +) -> HashMap { + request_env + .iter() + .filter(|(key, value)| local_policy_env.get(*key) != Some(*value)) + .map(|(key, value)| (key.clone(), value.clone())) + .collect() +} + +fn exec_server_env_for_request( + request: &ExecRequest, +) -> ( + Option, + HashMap, +) { + if let Some(exec_server_env_config) = &request.exec_server_env_config { + ( + Some(exec_server_env_config.policy.clone()), + env_overlay_for_exec_server(&request.env, &exec_server_env_config.local_policy_env), + ) + } else { + (None, request.env.clone()) + } +} + +fn exec_server_params_for_request( + process_id: i32, + request: &ExecRequest, + tty: bool, +) -> codex_exec_server::ExecParams { + let (env_policy, env) = exec_server_env_for_request(request); + codex_exec_server::ExecParams { + process_id: exec_server_process_id(process_id).into(), + argv: request.command.clone(), + cwd: request.cwd.to_path_buf(), + env_policy, + env, + tty, + arg0: request.arg0.clone(), + } +} + /// Borrowed process state prepared for a `write_stdin` or poll operation. struct PreparedProcessHandles { process: Arc, @@ -587,12 +654,7 @@ impl UnifiedExecProcessManager { mut spawn_lifecycle: SpawnLifecycleHandle, environment: &codex_exec_server::Environment, ) -> Result { - let (program, args) = request - .command - .split_first() - .ok_or(UnifiedExecError::MissingCommandLine)?; let inherited_fds = spawn_lifecycle.inherited_fds(); - if environment.is_remote() { if !inherited_fds.is_empty() { return Err(UnifiedExecError::create_process( @@ -602,19 +664,17 @@ impl UnifiedExecProcessManager { let started = environment .get_exec_backend() - .start(codex_exec_server::ExecParams { - process_id: exec_server_process_id(process_id).into(), - argv: request.command.clone(), - cwd: request.cwd.to_path_buf(), - env: request.env.clone(), - tty, - arg0: request.arg0.clone(), - }) + .start(exec_server_params_for_request(process_id, request, tty)) .await .map_err(|err| UnifiedExecError::create_process(err.to_string()))?; - return UnifiedExecProcess::from_remote_started(started, request.sandbox).await; + spawn_lifecycle.after_spawn(); + return UnifiedExecProcess::from_exec_server_started(started, request.sandbox).await; } + let (program, args) = request + .command + .split_first() + .ok_or(UnifiedExecError::MissingCommandLine)?; let spawn_result = if tty { codex_utils_pty::pty::spawn_process_with_inherited_fds( program, @@ -649,10 +709,20 @@ impl UnifiedExecProcessManager { cwd: AbsolutePathBuf, context: &UnifiedExecContext, ) -> Result<(UnifiedExecProcess, Option), UnifiedExecError> { - let env = apply_unified_exec_env(create_env( + let local_policy_env = create_env( &context.turn.shell_environment_policy, - Some(context.session.conversation_id), - )); + /*thread_id*/ None, + ); + let mut env = local_policy_env.clone(); + env.insert( + CODEX_THREAD_ID_ENV_VAR.to_string(), + context.session.conversation_id.to_string(), + ); + let env = apply_unified_exec_env(env); + let exec_server_env_config = ExecServerEnvConfig { + policy: exec_env_policy_from_shell_policy(&context.turn.shell_environment_policy), + local_policy_env, + }; let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new( self, @@ -680,6 +750,7 @@ impl UnifiedExecProcessManager { process_id: request.process_id, cwd, env, + exec_server_env_config: Some(exec_server_env_config), explicit_env_overrides: context.turn.shell_environment_policy.r#set.clone(), network: request.network.clone(), tty: request.tty, diff --git a/codex-rs/core/src/unified_exec/process_manager_tests.rs b/codex-rs/core/src/unified_exec/process_manager_tests.rs index 6897f4a5f..2c829cdd0 100644 --- a/codex-rs/core/src/unified_exec/process_manager_tests.rs +++ b/codex-rs/core/src/unified_exec/process_manager_tests.rs @@ -34,6 +34,92 @@ fn unified_exec_env_overrides_existing_values() { assert_eq!(env.get("PATH"), Some(&"/usr/bin".to_string())); } +#[test] +fn env_overlay_for_exec_server_keeps_runtime_changes_only() { + let local_policy_env = HashMap::from([ + ("HOME".to_string(), "/client-home".to_string()), + ("PATH".to_string(), "/client-path".to_string()), + ("SHELL_SET".to_string(), "policy".to_string()), + ]); + let request_env = HashMap::from([ + ("HOME".to_string(), "/client-home".to_string()), + ("PATH".to_string(), "/sandbox-path".to_string()), + ("SHELL_SET".to_string(), "policy".to_string()), + ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()), + ( + "CODEX_SANDBOX_NETWORK_DISABLED".to_string(), + "1".to_string(), + ), + ]); + + assert_eq!( + env_overlay_for_exec_server(&request_env, &local_policy_env), + HashMap::from([ + ("PATH".to_string(), "/sandbox-path".to_string()), + ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()), + ( + "CODEX_SANDBOX_NETWORK_DISABLED".to_string(), + "1".to_string() + ), + ]) + ); +} + +#[test] +fn exec_server_params_use_env_policy_overlay_contract() { + let request = ExecRequest { + command: vec!["bash".to_string(), "-lc".to_string(), "true".to_string()], + cwd: std::env::current_dir() + .expect("current dir") + .try_into() + .expect("absolute path"), + env: HashMap::from([ + ("HOME".to_string(), "/client-home".to_string()), + ("PATH".to_string(), "/sandbox-path".to_string()), + ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()), + ]), + exec_server_env_config: Some(ExecServerEnvConfig { + policy: codex_exec_server::ExecEnvPolicy { + inherit: codex_config::types::ShellEnvironmentPolicyInherit::Core, + ignore_default_excludes: false, + exclude: Vec::new(), + r#set: HashMap::new(), + include_only: Vec::new(), + }, + local_policy_env: HashMap::from([ + ("HOME".to_string(), "/client-home".to_string()), + ("PATH".to_string(), "/client-path".to_string()), + ]), + }), + network: None, + expiration: crate::exec::ExecExpiration::DefaultTimeout, + capture_policy: crate::exec::ExecCapturePolicy::ShellTool, + sandbox: codex_sandboxing::SandboxType::None, + windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel::Disabled, + windows_sandbox_private_desktop: false, + sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: codex_protocol::permissions::FileSystemSandboxPolicy::from( + &codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + ), + network_sandbox_policy: codex_protocol::permissions::NetworkSandboxPolicy::Restricted, + windows_sandbox_filesystem_overrides: None, + arg0: None, + }; + + let params = + exec_server_params_for_request(/*process_id*/ 123, &request, /*tty*/ true); + + assert_eq!(params.process_id.as_str(), "123"); + assert!(params.env_policy.is_some()); + assert_eq!( + params.env, + HashMap::from([ + ("PATH".to_string(), "/sandbox-path".to_string()), + ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()), + ]) + ); +} + #[test] fn exec_server_process_id_matches_unified_exec_process_id() { assert_eq!(exec_server_process_id(/*process_id*/ 4321), "4321"); diff --git a/codex-rs/core/src/unified_exec/process_tests.rs b/codex-rs/core/src/unified_exec/process_tests.rs index 2816d3370..065d65bc9 100644 --- a/codex-rs/core/src/unified_exec/process_tests.rs +++ b/codex-rs/core/src/unified_exec/process_tests.rs @@ -76,7 +76,7 @@ async fn remote_process(write_status: WriteStatus) -> UnifiedExecProcess { }), }; - UnifiedExecProcess::from_remote_started(started, SandboxType::None) + UnifiedExecProcess::from_exec_server_started(started, SandboxType::None) .await .expect("remote process should start") } @@ -133,7 +133,7 @@ async fn remote_process_waits_for_early_exit_event() { let _ = wake_tx.send(1); }); - let process = UnifiedExecProcess::from_remote_started(started, SandboxType::None) + let process = UnifiedExecProcess::from_exec_server_started(started, SandboxType::None) .await .expect("remote process should observe early exit"); diff --git a/codex-rs/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index 805d962ab..251089ce3 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -15,6 +15,7 @@ arc-swap = { workspace = true } async-trait = { workspace = true } base64 = { workspace = true } codex-app-server-protocol = { workspace = true } +codex-config = { workspace = true } codex-protocol = { workspace = true } codex-sandboxing = { workspace = true } codex-utils-absolute-path = { workspace = true } diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index a118468f5..77ead87a8 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -343,6 +343,7 @@ mod tests { process_id: ProcessId::from("default-env-proc"), argv: vec!["true".to_string()], cwd: std::env::current_dir().expect("read current dir"), + env_policy: None, env: Default::default(), tty: false, arg0: None, diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index 81bb8a6cd..a73182697 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -42,6 +42,7 @@ pub use process::ExecProcess; pub use process::StartedExecProcess; pub use process_id::ProcessId; pub use protocol::ExecClosedNotification; +pub use protocol::ExecEnvPolicy; pub use protocol::ExecExitedNotification; pub use protocol::ExecOutputDeltaNotification; pub use protocol::ExecOutputStream; diff --git a/codex-rs/exec-server/src/local_process.rs b/codex-rs/exec-server/src/local_process.rs index 5a5a6a6e6..bf38aa360 100644 --- a/codex-rs/exec-server/src/local_process.rs +++ b/codex-rs/exec-server/src/local_process.rs @@ -5,6 +5,9 @@ use std::time::Duration; use async_trait::async_trait; use codex_app_server_protocol::JSONRPCErrorError; +use codex_config::shell_environment; +use codex_config::types::EnvironmentVariablePattern; +use codex_config::types::ShellEnvironmentPolicy; use codex_utils_pty::ExecCommandSession; use codex_utils_pty::TerminalSize; use tokio::sync::Mutex; @@ -19,6 +22,7 @@ use crate::ProcessId; use crate::StartedExecProcess; use crate::protocol::EXEC_CLOSED_METHOD; use crate::protocol::ExecClosedNotification; +use crate::protocol::ExecEnvPolicy; use crate::protocol::ExecExitedNotification; use crate::protocol::ExecOutputDeltaNotification; use crate::protocol::ExecOutputStream; @@ -150,12 +154,13 @@ impl LocalProcess { process_map.insert(process_id.clone(), ProcessEntry::Starting); } + let env = child_env(¶ms); let spawned_result = if params.tty { codex_utils_pty::spawn_pty_process( program, args, params.cwd.as_path(), - ¶ms.env, + &env, ¶ms.arg0, TerminalSize::default(), ) @@ -165,7 +170,7 @@ impl LocalProcess { program, args, params.cwd.as_path(), - ¶ms.env, + &env, ¶ms.arg0, ) .await @@ -375,6 +380,36 @@ impl LocalProcess { } } +fn child_env(params: &ExecParams) -> HashMap { + let Some(env_policy) = ¶ms.env_policy else { + return params.env.clone(); + }; + + let policy = shell_environment_policy(env_policy); + let mut env = shell_environment::create_env(&policy, /*thread_id*/ None); + env.extend(params.env.clone()); + env +} + +fn shell_environment_policy(env_policy: &ExecEnvPolicy) -> ShellEnvironmentPolicy { + ShellEnvironmentPolicy { + inherit: env_policy.inherit.clone(), + ignore_default_excludes: env_policy.ignore_default_excludes, + exclude: env_policy + .exclude + .iter() + .map(|pattern| EnvironmentVariablePattern::new_case_insensitive(pattern)) + .collect(), + r#set: env_policy.r#set.clone(), + include_only: env_policy + .include_only + .iter() + .map(|pattern| EnvironmentVariablePattern::new_case_insensitive(pattern)) + .collect(), + use_profile: false, + } +} + #[async_trait] impl ExecBackend for LocalProcess { async fn start(&self, params: ExecParams) -> Result { @@ -618,3 +653,56 @@ fn notification_sender(inner: &Inner) -> Option { .unwrap_or_else(std::sync::PoisonError::into_inner) .clone() } + +#[cfg(test)] +mod tests { + use super::*; + use codex_config::types::ShellEnvironmentPolicyInherit; + + fn test_exec_params(env: HashMap) -> ExecParams { + ExecParams { + process_id: ProcessId::from("env-test"), + argv: vec!["true".to_string()], + cwd: std::path::PathBuf::from("/tmp"), + env_policy: None, + env, + tty: false, + arg0: None, + } + } + + #[test] + fn child_env_defaults_to_exact_env() { + let params = test_exec_params(HashMap::from([("ONLY_THIS".to_string(), "1".to_string())])); + + assert_eq!( + child_env(¶ms), + HashMap::from([("ONLY_THIS".to_string(), "1".to_string())]) + ); + } + + #[test] + fn child_env_applies_policy_then_overlay() { + let mut params = test_exec_params(HashMap::from([ + ("OVERLAY".to_string(), "overlay".to_string()), + ("POLICY_SET".to_string(), "overlay-wins".to_string()), + ])); + params.env_policy = Some(ExecEnvPolicy { + inherit: ShellEnvironmentPolicyInherit::None, + ignore_default_excludes: true, + exclude: Vec::new(), + r#set: HashMap::from([("POLICY_SET".to_string(), "policy".to_string())]), + include_only: Vec::new(), + }); + + let mut expected = HashMap::from([ + ("OVERLAY".to_string(), "overlay".to_string()), + ("POLICY_SET".to_string(), "overlay-wins".to_string()), + ]); + if cfg!(target_os = "windows") { + expected.insert("PATHEXT".to_string(), ".COM;.EXE;.BAT;.CMD".to_string()); + } + + assert_eq!(child_env(¶ms), expected); + } +} diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index b35363762..0ccb9794a 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -3,6 +3,7 @@ use std::path::PathBuf; use crate::FileSystemSandboxContext; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; +use codex_config::types::ShellEnvironmentPolicyInherit; use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use serde::Serialize; @@ -64,11 +65,23 @@ pub struct ExecParams { pub process_id: ProcessId, pub argv: Vec, pub cwd: PathBuf, + #[serde(default)] + pub env_policy: Option, pub env: HashMap, pub tty: bool, pub arg0: Option, } +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ExecEnvPolicy { + pub inherit: ShellEnvironmentPolicyInherit, + pub ignore_default_excludes: bool, + pub exclude: Vec, + pub r#set: HashMap, + pub include_only: Vec, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ExecResponse { diff --git a/codex-rs/exec-server/src/server/handler/tests.rs b/codex-rs/exec-server/src/server/handler/tests.rs index 547409946..321bf243a 100644 --- a/codex-rs/exec-server/src/server/handler/tests.rs +++ b/codex-rs/exec-server/src/server/handler/tests.rs @@ -27,6 +27,7 @@ fn exec_params_with_argv(process_id: &str, argv: Vec) -> ExecParams { process_id: ProcessId::from(process_id), argv, cwd: std::env::current_dir().expect("cwd"), + env_policy: None, env: inherited_path_env(), tty: false, arg0: None, diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index 6ea6a55bd..87b970450 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -390,6 +390,7 @@ mod tests { process_id, argv: sleep_then_print_argv(), cwd: std::env::current_dir().expect("cwd"), + env_policy: None, env, tty: false, arg0: None, diff --git a/codex-rs/exec-server/tests/exec_process.rs b/codex-rs/exec-server/tests/exec_process.rs index 489cd251f..afe0c2e35 100644 --- a/codex-rs/exec-server/tests/exec_process.rs +++ b/codex-rs/exec-server/tests/exec_process.rs @@ -51,6 +51,7 @@ async fn assert_exec_process_starts_and_exits(use_remote: bool) -> Result<()> { process_id: ProcessId::from("proc-1"), argv: vec!["true".to_string()], cwd: std::env::current_dir()?, + env_policy: /*env_policy*/ None, env: Default::default(), tty: false, arg0: None, @@ -127,6 +128,7 @@ async fn assert_exec_process_streams_output(use_remote: bool) -> Result<()> { "sleep 0.05; printf 'session output\\n'".to_string(), ], cwd: std::env::current_dir()?, + env_policy: /*env_policy*/ None, env: Default::default(), tty: false, arg0: None, @@ -156,6 +158,7 @@ async fn assert_exec_process_write_then_read(use_remote: bool) -> Result<()> { "import sys; line = sys.stdin.readline(); sys.stdout.write(f'from-stdin:{line}'); sys.stdout.flush()".to_string(), ], cwd: std::env::current_dir()?, + env_policy: /*env_policy*/ None, env: Default::default(), tty: true, arg0: None, @@ -192,6 +195,7 @@ async fn assert_exec_process_preserves_queued_events_before_subscribe( "printf 'queued output\\n'".to_string(), ], cwd: std::env::current_dir()?, + env_policy: /*env_policy*/ None, env: Default::default(), tty: false, arg0: None, @@ -224,6 +228,7 @@ async fn remote_exec_process_reports_transport_disconnect() -> Result<()> { "sleep 10".to_string(), ], cwd: std::env::current_dir()?, + env_policy: /*env_policy*/ None, env: Default::default(), tty: false, arg0: None,