From 1841ed3f015871978729245de8a3e7b3c64f3986 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Tue, 5 May 2026 10:12:46 -0700 Subject: [PATCH] codex: share turn environment selection for process tools Move tool environment selection onto ResolvedTurnEnvironments so follow-up tool slices can share lookup behavior without keeping a handler-global helper. Update exec_command and shell_command routing to use the shared selection method, and restore focused coverage for resolving path permissions against the selected workdir. Co-authored-by: Codex --- codex-rs/core/src/environment_selection.rs | 41 +++++++ codex-rs/core/src/sandboxing/mod.rs | 109 ++++++++++++++++++ codex-rs/core/src/tools/handlers/mod.rs | 54 +++++---- codex-rs/core/src/tools/handlers/shell.rs | 97 +++------------- .../core/src/tools/handlers/unified_exec.rs | 25 +--- .../src/tools/handlers/unified_exec_tests.rs | 39 +++++++ codex-rs/core/src/tools/runtimes/shell.rs | 16 +-- .../core/src/unified_exec/process_manager.rs | 40 +------ .../src/unified_exec/process_manager_tests.rs | 13 ++- 9 files changed, 261 insertions(+), 173 deletions(-) diff --git a/codex-rs/core/src/environment_selection.rs b/codex-rs/core/src/environment_selection.rs index e9d617cbf4..d96d9425db 100644 --- a/codex-rs/core/src/environment_selection.rs +++ b/codex-rs/core/src/environment_selection.rs @@ -41,6 +41,16 @@ impl ResolvedTurnEnvironments { self.turn_environments.first() } + pub(crate) fn get(&self, environment_id: &str) -> Option<&TurnEnvironment> { + self.turn_environments + .iter() + .find(|environment| environment.environment_id == environment_id) + } + + pub(crate) fn select(&self, environment_id: Option<&str>) -> Option<&TurnEnvironment> { + environment_id.map_or_else(|| self.primary(), |environment_id| self.get(environment_id)) + } + pub(crate) fn primary_environment(&self) -> Option> { self.primary() .map(|environment| Arc::clone(&environment.environment)) @@ -177,4 +187,35 @@ mod tests { "local" ); } + + #[tokio::test] + async fn resolved_environment_selections_selects_by_id_or_primary() { + let cwd = AbsolutePathBuf::current_dir().expect("cwd"); + let manager = EnvironmentManager::default_for_tests(); + + let resolved = resolve_environment_selections( + &manager, + &[TurnEnvironmentSelection { + environment_id: "local".to_string(), + cwd, + }], + ) + .expect("environment selections should resolve"); + + assert_eq!( + resolved + .select(/*environment_id*/ None) + .expect("primary environment") + .environment_id, + "local" + ); + assert_eq!( + resolved + .select(Some("local")) + .expect("selected environment") + .environment_id, + "local" + ); + assert!(resolved.select(Some("unknown")).is_none()); + } } diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index 5070d8da3a..41c0966fcd 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -16,6 +16,7 @@ use crate::exec::execute_exec_request; use crate::spawn::CODEX_SANDBOX_ENV_VAR; use crate::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_network_proxy::NetworkProxy; +use codex_protocol::config_types::ShellEnvironmentPolicy; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::models::PermissionProfile; @@ -41,6 +42,43 @@ pub(crate) struct ExecServerEnvConfig { pub(crate) local_policy_env: HashMap, } +impl ExecServerEnvConfig { + pub(crate) fn from_shell_policy( + policy: &ShellEnvironmentPolicy, + local_policy_env: HashMap, + ) -> Self { + Self { + policy: 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(), + }, + local_policy_env, + } + } + + pub(crate) fn env_overlay( + &self, + request_env: &HashMap, + ) -> HashMap { + request_env + .iter() + .filter(|(key, value)| self.local_policy_env.get(*key) != Some(*value)) + .map(|(key, value)| (key.clone(), value.clone())) + .collect() + } +} + #[derive(Debug)] pub struct ExecRequest { pub command: Vec, @@ -175,3 +213,74 @@ pub async fn execute_exec_request_with_after_spawn( ) -> codex_protocol::error::Result { execute_exec_request(exec_request, stdout_stream, after_spawn).await } + +#[cfg(test)] +mod tests { + use super::*; + use codex_protocol::config_types::EnvironmentVariablePattern; + use codex_protocol::config_types::ShellEnvironmentPolicyInherit; + use pretty_assertions::assert_eq; + + #[test] + fn exec_server_env_config_from_shell_policy_preserves_policy_fields() { + let policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::Core, + ignore_default_excludes: false, + exclude: vec![EnvironmentVariablePattern::new_case_insensitive("SECRET_*")], + r#set: HashMap::from([("FROM_POLICY".to_string(), "set-value".to_string())]), + include_only: vec![EnvironmentVariablePattern::new_case_insensitive("*PATH")], + use_profile: true, + }; + let local_policy_env = HashMap::from([ + ("PATH".to_string(), "/usr/bin".to_string()), + ("FROM_POLICY".to_string(), "set-value".to_string()), + ]); + + let config = ExecServerEnvConfig::from_shell_policy(&policy, local_policy_env.clone()); + + assert_eq!(config.policy.inherit, ShellEnvironmentPolicyInherit::Core); + assert!(!config.policy.ignore_default_excludes); + assert_eq!( + config.policy.exclude, + vec![EnvironmentVariablePattern::new_case_insensitive("SECRET_*")] + ); + assert_eq!(config.policy.r#set, policy.r#set); + assert_eq!( + config.policy.include_only, + vec![EnvironmentVariablePattern::new_case_insensitive("*PATH")] + ); + assert_eq!(config.local_policy_env, local_policy_env); + } + + #[test] + fn exec_server_env_config_env_overlay_keeps_only_runtime_changes() { + let config = ExecServerEnvConfig { + policy: codex_exec_server::ExecEnvPolicy { + inherit: 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()), + ("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()), + ]); + + assert_eq!( + config.env_overlay(&request_env), + HashMap::from([ + ("PATH".to_string(), "/sandbox-path".to_string()), + ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()), + ]) + ); + } +} diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 7f9119583b..ae915e33ef 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -27,10 +27,10 @@ use serde::Deserialize; use serde_json::Value; use std::path::Path; +use crate::environment_selection::ResolvedTurnEnvironments; use crate::function_tool::FunctionCallError; use crate::sandboxing::SandboxPermissions; use crate::session::session::Session; -use crate::session::turn_context::TurnContext; use crate::session::turn_context::TurnEnvironment; pub(crate) use crate::tools::code_mode::CodeModeExecuteHandler; pub(crate) use crate::tools::code_mode::CodeModeWaitHandler; @@ -93,25 +93,39 @@ fn resolve_workdir_base_path( .map_or_else(|| default_cwd.clone(), |workdir| default_cwd.join(workdir))) } -fn resolve_tool_environment<'a>( - turn: &'a TurnContext, - environment_id: Option<&str>, -) -> Result, FunctionCallError> { - environment_id.map_or_else( - || Ok(turn.environments.primary()), - |environment_id| { - turn.environments - .turn_environments - .iter() - .find(|environment| environment.environment_id == environment_id) - .map(Some) - .ok_or_else(|| { - FunctionCallError::RespondToModel(format!( - "unknown turn environment id `{environment_id}`" - )) - }) - }, - ) +#[derive(Debug, Deserialize)] +struct EnvironmentWorkdirArgs { + #[serde(default)] + environment_id: Option, + // Keep this raw until after environment selection; relative paths must be + // resolved against the selected environment cwd, not the process cwd. + #[serde(default)] + workdir: Option, +} + +/// Parses optional `environment_id` and `workdir` fields from tool arguments. +/// +/// Returns the selected turn environment plus the effective execution cwd. The +/// returned cwd is `turn_environment.cwd.join(workdir)` when `workdir` is +/// provided and non-empty, otherwise it is the selected `turn_environment.cwd`. +fn resolve_environment_workdir_target( + arguments: &str, + environments: &ResolvedTurnEnvironments, +) -> Result, FunctionCallError> { + let target_args: EnvironmentWorkdirArgs = parse_arguments(arguments)?; + let Some(turn_environment) = environments.select(target_args.environment_id.as_deref()) else { + return Ok(None); + }; + let cwd = target_args + .workdir + .as_deref() + .filter(|workdir| !workdir.is_empty()) + .map_or_else( + || turn_environment.cwd.clone(), + |workdir| turn_environment.cwd.join(workdir), + ); + + Ok(Some((turn_environment.clone(), cwd))) } /// Validates feature/policy constraints for `with_additional_permissions` and diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 4335f94100..ea91e27ec4 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -26,7 +26,7 @@ use crate::tools::handlers::implicit_granted_permissions; use crate::tools::handlers::normalize_and_validate_additional_permissions; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::parse_arguments_with_base_path; -use crate::tools::handlers::resolve_tool_environment; +use crate::tools::handlers::resolve_environment_workdir_target; use crate::tools::handlers::resolve_workdir_base_path; use crate::tools::hook_names::HookToolName; use crate::tools::orchestrator::ToolOrchestrator; @@ -39,7 +39,6 @@ use crate::tools::runtimes::shell::ShellRuntime; use crate::tools::runtimes::shell::ShellRuntimeBackend; use crate::tools::sandboxing::ToolCtx; use codex_features::Feature; -use codex_protocol::config_types::ShellEnvironmentPolicy; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::ExecCommandSource; use codex_shell_command::is_safe_command::is_known_safe_command; @@ -94,9 +93,7 @@ struct RunExecLikeArgs { tool_name: String, exec_params: ExecParams, hook_command: String, - target_environment: Arc, - remote_execution_enabled: bool, - exec_server_env_config: Option, + environment: Arc, additional_permissions: Option, prefix_rule: Option>, session: Arc, @@ -107,16 +104,6 @@ struct RunExecLikeArgs { shell_runtime_backend: ShellRuntimeBackend, } -#[derive(Debug, serde::Deserialize)] -struct ShellCommandEnvironmentArgs { - #[serde(default)] - environment_id: Option, - // Keep this raw until after environment selection; relative paths must be - // resolved against the selected environment cwd, not the process cwd. - #[serde(default)] - workdir: Option, -} - impl ShellHandler { fn to_exec_params( params: &ShellToolCallParams, @@ -197,36 +184,6 @@ impl ShellCommandHandler { } } -fn exec_server_env_policy_from_shell_policy( - policy: &ShellEnvironmentPolicy, -) -> 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 exec_server_env_config( - policy: &ShellEnvironmentPolicy, - local_policy_env: &std::collections::HashMap, -) -> crate::sandboxing::ExecServerEnvConfig { - crate::sandboxing::ExecServerEnvConfig { - policy: exec_server_env_policy_from_shell_policy(policy), - local_policy_env: local_policy_env.clone(), - } -} - impl From for ShellCommandHandler { fn from(config: ShellCommandBackendConfig) -> Self { let backend = match config { @@ -302,7 +259,7 @@ impl ToolHandler for ShellHandler { tool_name: "shell".to_string(), exec_params, hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), - target_environment: turn + environment: turn .environments .primary() .map(|environment| Arc::clone(&environment.environment)) @@ -311,8 +268,6 @@ impl ToolHandler for ShellHandler { "shell is unavailable in this session".to_string(), ) })?, - remote_execution_enabled: false, - exec_server_env_config: None, additional_permissions: params.additional_permissions.clone(), prefix_rule, session, @@ -391,7 +346,7 @@ impl ToolHandler for ContainerExecHandler { tool_name: "container.exec".to_string(), exec_params, hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), - target_environment: turn + environment: turn .environments .primary() .map(|environment| Arc::clone(&environment.environment)) @@ -400,8 +355,6 @@ impl ToolHandler for ContainerExecHandler { "shell is unavailable in this session".to_string(), ) })?, - remote_execution_enabled: false, - exec_server_env_config: None, additional_permissions: params.additional_permissions.clone(), prefix_rule, session, @@ -483,7 +436,7 @@ impl ToolHandler for LocalShellHandler { tool_name: "local_shell".to_string(), exec_params, hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), - target_environment: turn + environment: turn .environments .primary() .map(|environment| Arc::clone(&environment.environment)) @@ -492,8 +445,6 @@ impl ToolHandler for LocalShellHandler { "shell is unavailable in this session".to_string(), ) })?, - remote_execution_enabled: false, - exec_server_env_config: None, additional_permissions: None, prefix_rule: None, session, @@ -604,22 +555,13 @@ impl ToolHandler for ShellCommandHandler { ))); }; - let environment_args: ShellCommandEnvironmentArgs = parse_arguments(&arguments)?; - let Some(turn_environment) = - resolve_tool_environment(turn.as_ref(), environment_args.environment_id.as_deref())? + let Some((turn_environment, cwd)) = + resolve_environment_workdir_target(&arguments, &turn.environments)? else { return Err(FunctionCallError::RespondToModel( "shell is unavailable in this session".to_string(), )); }; - let cwd = environment_args - .workdir - .as_deref() - .filter(|workdir| !workdir.is_empty()) - .map_or_else( - || turn_environment.cwd.clone(), - |workdir| turn_environment.cwd.join(workdir), - ); let params: ShellCommandToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?; maybe_emit_implicit_skill_invocation( session.as_ref(), @@ -638,17 +580,11 @@ impl ToolHandler for ShellCommandHandler { cwd, turn.tools_config.allow_login_shell, )?; - let exec_server_env_config = turn_environment - .environment - .is_remote() - .then(|| exec_server_env_config(&turn.shell_environment_policy, &exec_params.env)); ShellHandler::run_exec_like(RunExecLikeArgs { tool_name: self.tool_name().display(), exec_params, hook_command: params.command, - target_environment: Arc::clone(&turn_environment.environment), - remote_execution_enabled: true, - exec_server_env_config, + environment: Arc::clone(&turn_environment.environment), additional_permissions: params.additional_permissions.clone(), prefix_rule, session, @@ -668,9 +604,7 @@ impl ShellHandler { tool_name, exec_params, hook_command, - target_environment, - remote_execution_enabled, - exec_server_env_config, + environment, additional_permissions, prefix_rule, session, @@ -682,7 +616,8 @@ impl ShellHandler { } = args; let mut exec_params = exec_params; - let fs = target_environment.get_filesystem(); + let fs = environment.get_filesystem(); + let local_policy_env = exec_params.env.clone(); let dependency_env = session.dependency_env().await; if !dependency_env.is_empty() { @@ -801,11 +736,15 @@ impl ShellHandler { command: exec_params.command.clone(), hook_command, cwd: exec_params.cwd.clone(), - environment: target_environment, - remote_execution_enabled, + exec_server_env_config: environment.is_remote().then(|| { + crate::sandboxing::ExecServerEnvConfig::from_shell_policy( + &turn.shell_environment_policy, + local_policy_env, + ) + }), + environment, timeout_ms: exec_params.expiration.timeout_ms(), env: exec_params.env.clone(), - exec_server_env_config, explicit_env_overrides, network: exec_params.network.clone(), sandbox_permissions: effective_additional_permissions.sandbox_permissions, diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index c257240a4d..4cc7e52760 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -13,7 +13,7 @@ use crate::tools::handlers::implicit_granted_permissions; use crate::tools::handlers::normalize_and_validate_additional_permissions; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::parse_arguments_with_base_path; -use crate::tools::handlers::resolve_tool_environment; +use crate::tools::handlers::resolve_environment_workdir_target; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; use crate::tools::registry::PreToolUsePayload; @@ -69,16 +69,6 @@ pub(crate) struct ExecCommandArgs { prefix_rule: Option>, } -#[derive(Debug, Deserialize)] -struct ExecCommandEnvironmentArgs { - #[serde(default)] - environment_id: Option, - // Keep this raw until after environment selection; relative paths must be - // resolved against the selected environment cwd, not the process cwd. - #[serde(default)] - workdir: Option, -} - #[derive(Debug, Deserialize)] struct WriteStdinArgs { // The model is trained on `session_id`. @@ -191,22 +181,13 @@ impl ToolHandler for ExecCommandHandler { let manager: &UnifiedExecProcessManager = &session.services.unified_exec_manager; let context = UnifiedExecContext::new(session.clone(), turn.clone(), call_id.clone()); - let environment_args: ExecCommandEnvironmentArgs = parse_arguments(&arguments)?; - let Some(turn_environment) = - resolve_tool_environment(turn.as_ref(), environment_args.environment_id.as_deref())? + let Some((turn_environment, cwd)) = + resolve_environment_workdir_target(&arguments, &turn.environments)? else { return Err(FunctionCallError::RespondToModel( "unified exec is unavailable in this session".to_string(), )); }; - let cwd = environment_args - .workdir - .as_deref() - .filter(|workdir| !workdir.is_empty()) - .map_or_else( - || turn_environment.cwd.clone(), - |workdir| turn_environment.cwd.join(workdir), - ); let environment = Arc::clone(&turn_environment.environment); let fs = environment.get_filesystem(); let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?; diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index 8818b2e344..ea1a784e6c 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -1,10 +1,16 @@ use super::*; use crate::shell::default_user_shell; +use crate::tools::handlers::parse_arguments_with_base_path; +use codex_protocol::models::AdditionalPermissionProfile as PermissionProfile; +use codex_protocol::models::FileSystemPermissions; use codex_tools::UnifiedExecShellMode; use codex_tools::ZshForkConfig; use codex_utils_absolute_path::AbsolutePathBuf; +use core_test_support::PathExt; use pretty_assertions::assert_eq; +use std::fs; use std::sync::Arc; +use tempfile::tempdir; use crate::session::tests::make_session_and_context; use crate::tools::context::ExecCommandToolOutput; @@ -178,6 +184,39 @@ fn test_get_command_ignores_explicit_shell_in_zsh_fork_mode() -> anyhow::Result< Ok(()) } +#[test] +fn exec_command_args_resolve_relative_additional_permissions_against_selected_workdir() +-> anyhow::Result<()> { + let selected_cwd = tempdir()?; + let workdir = selected_cwd.path().join("nested"); + fs::create_dir_all(&workdir)?; + let expected_write = workdir.join("relative-write.txt"); + let json = r#"{ + "cmd": "echo hello", + "workdir": "nested", + "additional_permissions": { + "file_system": { + "write": ["./relative-write.txt"] + } + } + }"#; + + let target_cwd = selected_cwd.path().abs().join("nested"); + let args: ExecCommandArgs = parse_arguments_with_base_path(json, &target_cwd)?; + + assert_eq!( + args.additional_permissions, + Some(PermissionProfile { + file_system: Some(FileSystemPermissions::from_read_write_roots( + /*read*/ None, + Some(vec![expected_write.abs()]), + )), + ..Default::default() + }) + ); + Ok(()) +} + #[tokio::test] async fn exec_command_pre_tool_use_payload_uses_raw_command() { let payload = ToolPayload::Function { diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 8f8cb87b2b..546d07514e 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -69,7 +69,6 @@ pub struct ShellRequest { pub hook_command: String, pub cwd: AbsolutePathBuf, pub environment: Arc, - pub remote_execution_enabled: bool, pub timeout_ms: Option, pub env: HashMap, pub exec_server_env_config: Option, @@ -270,12 +269,6 @@ impl ToolRuntime for ShellRuntime { attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, ) -> Result { - if req.environment.is_remote() && !req.remote_execution_enabled { - return Err(ToolError::Rejected( - "shell execution is unavailable for remote environments".to_string(), - )); - } - let session_shell = ctx.session.user_shell(); let managed_network = managed_network_for_sandbox_permissions(req.network.as_ref(), req.sandbox_permissions); @@ -341,14 +334,7 @@ fn exec_server_env_for_request( HashMap, ) { if let Some(exec_server_env_config) = &request.exec_server_env_config { - let env = request - .env - .iter() - .filter(|(key, value)| { - exec_server_env_config.local_policy_env.get(*key) != Some(*value) - }) - .map(|(key, value)| (key.clone(), value.clone())) - .collect(); + let env = exec_server_env_config.env_overlay(&request.env); (Some(exec_server_env_config.policy.clone()), env) } else { (None, request.env.clone()) diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 4f85b1ddf7..a032da3e54 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -50,7 +50,6 @@ 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_protocol::config_types::ShellEnvironmentPolicy; use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; use codex_protocol::protocol::ExecCommandSource; @@ -98,37 +97,6 @@ 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, ) -> ( @@ -138,7 +106,7 @@ fn exec_server_env_for_request( 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), + exec_server_env_config.env_overlay(&request.env), ) } else { (None, request.env.clone()) @@ -990,10 +958,10 @@ impl UnifiedExecProcessManager { 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), + let exec_server_env_config = ExecServerEnvConfig::from_shell_policy( + &context.turn.shell_environment_policy, local_policy_env, - }; + ); let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new( self, 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 bde32c9ab4..4fa3aec0b3 100644 --- a/codex-rs/core/src/unified_exec/process_manager_tests.rs +++ b/codex-rs/core/src/unified_exec/process_manager_tests.rs @@ -52,8 +52,19 @@ fn env_overlay_for_exec_server_keeps_runtime_changes_only() { ), ]); + let config = ExecServerEnvConfig { + policy: codex_exec_server::ExecEnvPolicy { + inherit: codex_protocol::config_types::ShellEnvironmentPolicyInherit::Core, + ignore_default_excludes: false, + exclude: Vec::new(), + r#set: HashMap::new(), + include_only: Vec::new(), + }, + local_policy_env, + }; + assert_eq!( - env_overlay_for_exec_server(&request_env, &local_policy_env), + config.env_overlay(&request_env), HashMap::from([ ("PATH".to_string(), "/sandbox-path".to_string()), ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()),