From aeb6b22c8bda5f4d4b6ab80ebd3ccdb3f47fb3ab Mon Sep 17 00:00:00 2001 From: swastik-oai Date: Tue, 27 Oct 2026 11:37:27 +0000 Subject: [PATCH] Extract unified exec command preparation into a reusable module (#57435) ## What changed Move argument parsing, working-directory and shell resolution, permission validation, and command lifetime setup into `unified_exec::preparation` with crate-visible parsed and prepared command types. Update `ExecCommandHandler` to use the extracted preparation flow, preserving implicit skill event emission between parsing and preparation. GitOrigin-RevId: 79e9be5d87c1b0ab905afdf1e343ac141fb7d4c6 --- codex-rs/core/src/tools/handlers/mod.rs | 2 +- .../core/src/tools/handlers/unified_exec.rs | 1 + .../handlers/unified_exec/exec_command.rs | 221 ++-------------- .../handlers/unified_exec/preparation.rs | 247 ++++++++++++++++++ 4 files changed, 274 insertions(+), 197 deletions(-) create mode 100644 codex-rs/core/src/tools/handlers/unified_exec/preparation.rs diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 5f0b5e5d31..a83b7e153b 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -284,7 +284,7 @@ pub(crate) fn normalize_and_validate_additional_permissions( } } -pub(super) struct EffectiveAdditionalPermissions { +pub(crate) struct EffectiveAdditionalPermissions { pub sandbox_permissions: SandboxPermissions, pub additional_permissions: Option, pub permissions_preapproved: bool, diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 9164d84196..ec7a7d24aa 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -20,6 +20,7 @@ use std::path::PathBuf; use std::sync::Arc; mod exec_command; +pub(crate) mod preparation; mod write_stdin; pub use exec_command::ExecCommandHandler; diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index 329dc2cfd1..c4664179d2 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -1,9 +1,5 @@ -use std::path::Path; use std::sync::Arc; -use std::time::Duration; -use crate::exec::DEFAULT_EXEC_COMMAND_TIMEOUT_MS; -use crate::exec_policy::prompt_is_rejected_by_policy; use crate::function_tool::FunctionCallError; use crate::maybe_emit_implicit_skill_invocation; use crate::tools::context::ExecCommandToolOutput; @@ -12,15 +8,9 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::handlers::ToolEnvironmentArgs; -use crate::tools::handlers::apply_granted_turn_permissions; use crate::tools::handlers::apply_patch::intercept_apply_patch; -use crate::tools::handlers::file_system_sandbox_policy_context_for_cwd; use crate::tools::handlers::function_tool_environment; -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_sandbox_permissions; use crate::tools::handlers::rewrite_function_string_argument; use crate::tools::handlers::selected_executor; use crate::tools::handlers::updated_hook_command; @@ -39,15 +29,10 @@ use crate::unified_exec::output::CapturedStream; use codex_features::Feature; use codex_otel::SessionTelemetry; use codex_otel::TOOL_CALL_UNIFIED_EXEC_METRIC; -use codex_sandboxing::SandboxManager; -use codex_sandboxing::SandboxType; -use codex_sandboxing::SandboxablePreference; -use codex_shell_command::shell_detect::detect_shell_type; use codex_tools::JsonSchema; use codex_tools::ToolName; use codex_tools::ToolSpec; use codex_utils_output_truncation::approx_token_count; -use codex_utils_path_uri::PathConvention; use codex_utils_path_uri::PathUri; use codex_utils_string::truncate_middle_chars; @@ -55,8 +40,9 @@ use super::super::shell_spec::CommandToolOptions; use super::super::shell_spec::create_exec_command_tool_with_environment_id; use super::ExecCommandArgs; use super::ExecCommandWorkdirArgs; -use super::get_command; use super::post_unified_exec_tool_use_payload; +use super::preparation::ExecCommandLifetime; +use super::preparation::ParsedExecCommand; // A byte limit is a conservative hard token bound even for byte-fallback tokenizers. const EXEC_COMMAND_REJECTION_MAX_BYTES: usize = 900; @@ -69,12 +55,6 @@ pub(crate) struct ExecCommandHandlerOptions { pub(crate) include_windows_shell_guidance: bool, } -#[derive(Clone, Copy)] -enum ExecCommandLifetime { - Interactive, - OneShot, -} - pub struct ExecCommandHandler { options: ExecCommandHandlerOptions, lifetime: ExecCommandLifetime, @@ -158,7 +138,6 @@ impl ExecCommandHandler { let ToolInvocation { environments, session, - turn, step_context, cancellation_token, tracker, @@ -183,173 +162,24 @@ impl ExecCommandHandler { cancellation_token, call_id.clone(), ); - let workdir_args: ExecCommandWorkdirArgs = parse_arguments(&arguments)?; let turn_environment = selected_executor(&environments, &step_context.environments)?; - let cwd = workdir_args.resolve_cwd(turn_environment)?; - let environment = Arc::clone(&turn_environment.environment); - let fs = environment.get_filesystem(); - - // Remote executors enforce URI-native sandbox policy themselves. Only a host-local - // sandbox needs a native cwd for resolving paths nested in the permissions config. - let requires_host_native_cwd = !environment.is_remote() - && SandboxManager::new().select_initial( - turn_environment.permission_profile(), - SandboxablePreference::Auto, - codex_protocol::sandbox::effective_windows_sandbox_type( - turn_environment.config().windows_sandbox_type, - turn_environment.config().windows_sandbox_level, - ), - turn.network.is_some(), - ) != SandboxType::None; - // `to_abs_path()` alone cannot identify foreign drive paths: `file:///C:/repo` is - // representable as `/C:/repo` on POSIX. Require the inferred convention to match too. - let cwd_uses_native_convention = - cwd.infer_path_convention() == Some(PathConvention::native()); - let native_cwd = match cwd.to_abs_path() { - Ok(cwd) if cwd_uses_native_convention => Some(cwd), - _ if !requires_host_native_cwd => None, - Err(err) => return Err(FunctionCallError::RespondToModel(err.to_string())), - Ok(_) => { - return Err(FunctionCallError::RespondToModel(format!( - "path URI `{cwd}` does not use the host's native {} path convention", - PathConvention::native() - ))); - } - }; - let mut args: ExecCommandArgs = match native_cwd.as_ref() { - Some(native_cwd) => { - // The base path only resolves paths nested in the permissions config types. - parse_arguments_with_base_path(&arguments, native_cwd)? - } - None => { - // Foreign executor cwd values cannot seed this host's AbsolutePathBufGuard. - // Sandbox intent and URI-native roots are still sent to the executor. - parse_arguments(&arguments)? - } - }; - if args.tty && !session.features().enabled(Feature::UnifiedExecTty) { - return Err(FunctionCallError::RespondToModel( - "TTY execution is disabled by config; omit `tty` or set it to false.".to_string(), - )); - } - let sandbox_permissions = - resolve_sandbox_permissions(args.sandbox_permissions, args.justification.as_deref())?; - let hook_command = args.cmd.clone(); + let parsed = + ParsedExecCommand::parse(&arguments, &session, &step_context, turn_environment)?; maybe_emit_implicit_skill_invocation( - session.as_ref(), - context.step_context.as_ref(), - &hook_command, - &cwd, - native_cwd.as_ref(), + &session, + &step_context, + &parsed.args.cmd, + &parsed.cwd, + parsed.native_cwd.as_ref(), &turn_environment.selection.environment_id, ) .await; - // Remote environments may use a different OS and must build commands with their native - // shell; fall back to the session shell when the environment did not report one. - let shell = turn_environment - .shell - .clone() - .map(Arc::new) - .unwrap_or_else(|| session.user_shell()); - // TODO(anp): Resolve requested shells in remote environments instead of restricting - // commands to the reported default shell. - if environment.is_remote() - && let Some(requested_shell) = args.shell.take() - { - let Some(remote_shell) = turn_environment.shell.as_ref() else { - return Err(FunctionCallError::RespondToModel(format!( - "environment `{}` does not report a shell", - turn_environment.selection.environment_id - ))); - }; - if detect_shell_type(Path::new(&requested_shell)) != Some(remote_shell.shell_type) { - return Err(FunctionCallError::RespondToModel(format!( - "environment `{}` only supports `{}`", - turn_environment.selection.environment_id, - remote_shell.name() - ))); - } - } - let resolved_command = - get_command(&args, shell, turn_environment.config().allow_login_shell) - .map_err(FunctionCallError::RespondToModel)?; - let command = resolved_command.command; - let ExecCommandArgs { - mut tty, - yield_time_ms, - timeout_ms, - max_output_tokens, - sandbox_permissions: _, - additional_permissions, - justification, - prefix_rule, - .. - } = args; - let completion_timeout = match self.lifetime { - ExecCommandLifetime::Interactive => None, - ExecCommandLifetime::OneShot => { - tty = false; - Some(Duration::from_millis( - timeout_ms.unwrap_or(DEFAULT_EXEC_COMMAND_TIMEOUT_MS), - )) - } - }; - - let exec_permission_approvals_enabled = - session.features().enabled(Feature::ExecPermissionApprovals); - let requested_additional_permissions = additional_permissions.clone(); - let sandbox_context = - turn_environment.sandbox_context(/*additional_permissions*/ None); - let permission_context = file_system_sandbox_policy_context_for_cwd(&sandbox_context, &cwd); - let effective_additional_permissions = apply_granted_turn_permissions( - &context.step_context, - turn_environment, - &cwd, - sandbox_permissions, - additional_permissions, - ); - let additional_permissions_allowed = exec_permission_approvals_enabled - || (session.features().enabled(Feature::RequestPermissionsTool) - && effective_additional_permissions.permissions_preapproved); - - // Sticky turn permissions have already been approved, so they should - // continue through the normal exec approval flow for the command. - let approval_policy = context.step_context.settings.approval_policy(); - if effective_additional_permissions - .sandbox_permissions - .requests_sandbox_override() - && !effective_additional_permissions.permissions_preapproved - && prompt_is_rejected_by_policy(approval_policy, /*prompt_is_rule*/ false).is_some() - { - return Err(FunctionCallError::RespondToModel(format!( - "approval policy is {approval_policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {approval_policy:?}" - ))); - } - - let normalized_additional_permissions = match implicit_granted_permissions( - sandbox_permissions, - requested_additional_permissions.as_ref(), - &effective_additional_permissions, - ) - .map_or_else( - || { - normalize_and_validate_additional_permissions( - additional_permissions_allowed, - approval_policy, - effective_additional_permissions.sandbox_permissions, - effective_additional_permissions.additional_permissions, - effective_additional_permissions.permissions_preapproved, - &permission_context, - ) - }, - |permissions| Ok(Some(permissions)), - ) { - Ok(normalized) => normalized, - Err(err) => { - return Err(FunctionCallError::RespondToModel(err)); - } - }; - + let prepared = parsed.prepare(self.lifetime)?; + let command = prepared.resolved.command; + let cwd = prepared.cwd; + let hook_command = prepared.hook_command; + let max_output_tokens = prepared.max_output_tokens; + let fs = turn_environment.environment.get_filesystem(); let intercepted_patch = intercept_apply_patch( &command, &cwd, @@ -382,7 +212,7 @@ impl ExecCommandHandler { })); } - emit_unified_exec_tty_metric(&step_context.session_telemetry, tty); + emit_unified_exec_tty_metric(&step_context.session_telemetry, prepared.tty); crate::tools::lifecycle::notify_command_start( context.session.as_ref(), context.step_context.turn.as_ref(), @@ -396,24 +226,23 @@ impl ExecCommandHandler { let process_id = manager.allocate_process_id().await; let request = ExecCommandRequest { command, - shell: resolved_command.shell, + shell: prepared.resolved.shell, hook_command: hook_command.clone(), process_id, - yield_time_ms, + yield_time_ms: prepared.yield_time_ms, max_output_tokens, cwd: cwd.clone(), sandbox_cwd: turn_environment.cwd().clone(), turn_environment: turn_environment.clone(), network: context.step_context.turn.network.clone(), - tty, - sandbox_permissions: effective_additional_permissions.sandbox_permissions, - additional_permissions: normalized_additional_permissions, - additional_permissions_preapproved: effective_additional_permissions - .permissions_preapproved, - justification, - prefix_rule, + tty: prepared.tty, + sandbox_permissions: prepared.permissions.sandbox_permissions, + additional_permissions: prepared.permissions.additional_permissions, + additional_permissions_preapproved: prepared.permissions.permissions_preapproved, + justification: prepared.justification, + prefix_rule: prepared.prefix_rule, }; - let result = match completion_timeout { + let result = match prepared.completion_timeout { Some(timeout) => { UnifiedExecProcessManager::exec_command_to_completion(request, &context, timeout) .await diff --git a/codex-rs/core/src/tools/handlers/unified_exec/preparation.rs b/codex-rs/core/src/tools/handlers/unified_exec/preparation.rs new file mode 100644 index 0000000000..642fe8ca58 --- /dev/null +++ b/codex-rs/core/src/tools/handlers/unified_exec/preparation.rs @@ -0,0 +1,247 @@ +//! Resolves exec arguments against the selected environment and effective turn permissions. +//! The live handler emits its skill event between parsing and preparation. + +use super::ExecCommandArgs; +use super::ExecCommandWorkdirArgs; +use super::ResolvedCommand; +use super::get_command; +use crate::exec::DEFAULT_EXEC_COMMAND_TIMEOUT_MS; +use crate::exec_policy::prompt_is_rejected_by_policy; +use crate::function_tool::FunctionCallError; +use crate::sandboxing::SandboxPermissions; +use crate::session::session::Session; +use crate::session::step_context::StepContext; +use crate::session::turn_context::TurnEnvironment; +use crate::tools::handlers::EffectiveAdditionalPermissions; +use crate::tools::handlers::apply_granted_turn_permissions; +use crate::tools::handlers::file_system_sandbox_policy_context_for_cwd; +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_sandbox_permissions; +use codex_features::Feature; +use codex_sandboxing::SandboxManager; +use codex_sandboxing::SandboxType; +use codex_sandboxing::SandboxablePreference; +use codex_shell_command::shell_detect::detect_shell_type; +use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathConvention; +use codex_utils_path_uri::PathUri; +use std::path::Path; +use std::sync::Arc; +use std::time::Duration; + +#[derive(Clone, Copy)] +pub(crate) enum ExecCommandLifetime { + Interactive, + OneShot, +} + +/// Command details available before the live handler reserves a process. +pub(crate) struct PreparedExecCommand { + pub(crate) resolved: ResolvedCommand, + pub(crate) hook_command: String, + pub(crate) cwd: PathUri, + pub(crate) permissions: EffectiveAdditionalPermissions, + pub(crate) justification: Option, + pub(crate) prefix_rule: Option>, + pub(crate) tty: bool, + pub(crate) yield_time_ms: u64, + pub(crate) max_output_tokens: Option, + pub(crate) completion_timeout: Option, +} + +pub(crate) struct ParsedExecCommand<'a> { + session: &'a Session, + step: &'a StepContext, + environment: &'a TurnEnvironment, + pub(super) args: ExecCommandArgs, + pub(super) cwd: PathUri, + pub(super) native_cwd: Option, + sandbox_permissions: SandboxPermissions, +} + +impl<'a> ParsedExecCommand<'a> { + /// Resolve this call's workdir and arguments using the executor selected by the caller. + pub(crate) fn parse( + arguments: &str, + session: &'a Session, + step: &'a StepContext, + turn_environment: &'a TurnEnvironment, + ) -> Result { + let workdir: ExecCommandWorkdirArgs = parse_arguments(arguments)?; + let cwd = workdir.resolve_cwd(turn_environment)?; + let environment = &turn_environment.environment; + + // Remote executors enforce URI-native sandbox policy themselves. Only a host-local + // sandbox needs a native cwd for resolving paths nested in the permissions config. + let requires_host_native_cwd = !environment.is_remote() + && SandboxManager::new().select_initial( + turn_environment.permission_profile(), + SandboxablePreference::Auto, + codex_protocol::sandbox::effective_windows_sandbox_type( + turn_environment.config().windows_sandbox_type, + turn_environment.config().windows_sandbox_level, + ), + step.turn.network.is_some(), + ) != SandboxType::None; + // `to_abs_path()` alone cannot identify foreign drive paths: `file:///C:/repo` is + // representable as `/C:/repo` on POSIX. Require the inferred convention to match too. + let cwd_uses_native_convention = + cwd.infer_path_convention() == Some(PathConvention::native()); + let native_cwd = match cwd.to_abs_path() { + Ok(cwd) if cwd_uses_native_convention => Some(cwd), + _ if !requires_host_native_cwd => None, + Err(err) => return Err(FunctionCallError::RespondToModel(err.to_string())), + Ok(_) => { + return Err(FunctionCallError::RespondToModel(format!( + "path URI `{cwd}` does not use the host's native {} path convention", + PathConvention::native() + ))); + } + }; + let args: ExecCommandArgs = match native_cwd.as_ref() { + Some(native_cwd) => { + // The base path only resolves paths nested in the permissions config types. + parse_arguments_with_base_path(arguments, native_cwd)? + } + None => { + // Foreign executor cwd values cannot seed this host's AbsolutePathBufGuard. + // Sandbox intent and URI-native roots are still sent to the executor. + parse_arguments(arguments)? + } + }; + if args.tty && !session.features().enabled(Feature::UnifiedExecTty) { + return Err(FunctionCallError::RespondToModel( + "TTY execution is disabled by config; omit `tty` or set it to false.".to_string(), + )); + } + let sandbox_permissions = + resolve_sandbox_permissions(args.sandbox_permissions, args.justification.as_deref())?; + Ok(Self { + session, + step, + environment: turn_environment, + args, + cwd, + native_cwd, + sandbox_permissions, + }) + } + + pub(crate) fn prepare( + self, + lifetime: ExecCommandLifetime, + ) -> Result { + let Self { + session, + step, + environment: turn_environment, + mut args, + cwd, + sandbox_permissions, + .. + } = self; + // Remote environments may use a different OS and must build commands with their native + // shell; fall back to the session shell when the environment did not report one. + let shell = turn_environment + .shell + .clone() + .map(Arc::new) + .unwrap_or_else(|| session.user_shell()); + // TODO(anp): Resolve requested shells in remote environments instead of restricting + // commands to the reported default shell. + if turn_environment.environment.is_remote() + && let Some(requested_shell) = args.shell.take() + { + let Some(remote_shell) = turn_environment.shell.as_ref() else { + return Err(FunctionCallError::RespondToModel(format!( + "environment `{}` does not report a shell", + turn_environment.selection.environment_id + ))); + }; + if detect_shell_type(Path::new(&requested_shell)) != Some(remote_shell.shell_type) { + return Err(FunctionCallError::RespondToModel(format!( + "environment `{}` only supports `{}`", + turn_environment.selection.environment_id, + remote_shell.name() + ))); + } + } + let resolved_command = + get_command(&args, shell, turn_environment.config().allow_login_shell) + .map_err(FunctionCallError::RespondToModel)?; + let completion_timeout = match lifetime { + ExecCommandLifetime::Interactive => None, + ExecCommandLifetime::OneShot => { + args.tty = false; + Some(Duration::from_millis( + args.timeout_ms.unwrap_or(DEFAULT_EXEC_COMMAND_TIMEOUT_MS), + )) + } + }; + + let exec_permission_approvals_enabled = + session.features().enabled(Feature::ExecPermissionApprovals); + let requested_additional_permissions = args.additional_permissions.take(); + let sandbox_context = + turn_environment.sandbox_context(/*additional_permissions*/ None); + let permission_context = file_system_sandbox_policy_context_for_cwd(&sandbox_context, &cwd); + let mut permissions = apply_granted_turn_permissions( + step, + turn_environment, + &cwd, + sandbox_permissions, + requested_additional_permissions.clone(), + ); + let additional_permissions_allowed = exec_permission_approvals_enabled + || (session.features().enabled(Feature::RequestPermissionsTool) + && permissions.permissions_preapproved); + + // Sticky turn permissions have already been approved, so they should + // continue through the normal exec approval flow for the command. + let approval_policy = step.settings.approval_policy(); + if permissions.sandbox_permissions.requests_sandbox_override() + && !permissions.permissions_preapproved + && prompt_is_rejected_by_policy(approval_policy, /*prompt_is_rule*/ false).is_some() + { + return Err(FunctionCallError::RespondToModel(format!( + "approval policy is {approval_policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {approval_policy:?}" + ))); + } + + permissions.additional_permissions = implicit_granted_permissions( + sandbox_permissions, + requested_additional_permissions.as_ref(), + &permissions, + ) + .map_or_else( + || { + normalize_and_validate_additional_permissions( + additional_permissions_allowed, + approval_policy, + permissions.sandbox_permissions, + permissions.additional_permissions, + permissions.permissions_preapproved, + &permission_context, + ) + }, + |permissions| Ok(Some(permissions)), + ) + .map_err(FunctionCallError::RespondToModel)?; + + Ok(PreparedExecCommand { + resolved: resolved_command, + hook_command: args.cmd, + yield_time_ms: args.yield_time_ms, + max_output_tokens: args.max_output_tokens, + completion_timeout, + cwd, + tty: args.tty, + permissions, + justification: args.justification, + prefix_rule: args.prefix_rule, + }) + } +}