From 5b74f10a7bdf5b8e659421d8f60a5c10f6bfa474 Mon Sep 17 00:00:00 2001 From: jimmyfraiture Date: Mon, 29 Sep 2025 19:34:12 +0100 Subject: [PATCH] Sandboxing iteration 2 --- codex-rs/core/src/codex.rs | 320 +++++------------- codex-rs/core/src/executor/backends.rs | 95 ++++++ codex-rs/core/src/executor/cache.rs | 25 ++ codex-rs/core/src/executor/mod.rs | 11 + codex-rs/core/src/executor/runner.rs | 306 +++++++++++++++++ codex-rs/core/src/executor/sandbox.rs | 193 +++++++++++ codex-rs/core/src/lib.rs | 2 +- .../core/src/sandbox/apply_patch_adapter.rs | 33 -- codex-rs/core/src/sandbox/backend.rs | 87 ----- codex-rs/core/src/sandbox/mod.rs | 51 --- codex-rs/core/src/sandbox/planner.rs | 217 ------------ codex-rs/core/src/state/session.rs | 12 - 12 files changed, 718 insertions(+), 634 deletions(-) create mode 100644 codex-rs/core/src/executor/backends.rs create mode 100644 codex-rs/core/src/executor/cache.rs create mode 100644 codex-rs/core/src/executor/mod.rs create mode 100644 codex-rs/core/src/executor/runner.rs create mode 100644 codex-rs/core/src/executor/sandbox.rs delete mode 100644 codex-rs/core/src/sandbox/apply_patch_adapter.rs delete mode 100644 codex-rs/core/src/sandbox/backend.rs delete mode 100644 codex-rs/core/src/sandbox/mod.rs delete mode 100644 codex-rs/core/src/sandbox/planner.rs diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 70cc6900b3..84b1c53b9c 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1,10 +1,8 @@ use std::borrow::Cow; use std::collections::HashMap; -use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicU64; -use std::time::Duration; use crate::AuthManager; use crate::client_common::REVIEW_PROMPT; @@ -54,11 +52,10 @@ use crate::environment_context::EnvironmentContext; use crate::error::CodexErr; use crate::error::Result as CodexResult; use crate::error::SandboxErr; -use crate::error::get_error_message_ui; use crate::exec::ExecParams; use crate::exec::ExecToolCallOutput; -use crate::exec::SandboxType; use crate::exec::StdoutStream; +#[cfg(test)] use crate::exec::StreamOutput; use crate::exec_command::EXEC_COMMAND_TOOL_NAME; use crate::exec_command::ExecCommandParams; @@ -66,6 +63,12 @@ use crate::exec_command::ExecSessionManager; use crate::exec_command::WRITE_STDIN_TOOL_NAME; use crate::exec_command::WriteStdinParams; use crate::exec_env::create_env; +use crate::executor::ExecError; +use crate::executor::ExecutionMode; +use crate::executor::ExecutionRequest; +use crate::executor::Executor; +use crate::executor::ExecutorConfig; +use crate::executor::normalize_exec_result; use crate::mcp_connection_manager::McpConnectionManager; use crate::mcp_tool_call::handle_mcp_tool_call; use crate::model_family::find_family_for_model; @@ -109,12 +112,6 @@ use crate::protocol::TurnDiffEvent; use crate::protocol::WebSearchBeginEvent; use crate::rollout::RolloutRecorder; use crate::rollout::RolloutRecorderParams; -use crate::sandbox::BackendRegistry; -use crate::sandbox::ExecPlan; -use crate::sandbox::ExecRuntimeContext; -use crate::sandbox::PreparedExec; -use crate::sandbox::prepare_exec_invocation; -use crate::sandbox::run_with_plan; use crate::shell; use crate::state::ActiveTurn; use crate::state::SessionServices; @@ -266,6 +263,7 @@ pub(crate) struct Session { pub(crate) active_turn: Mutex>, services: SessionServices, next_internal_sub_id: AtomicU64, + executor: Executor, } /// The context needed for a single turn of the conversation. @@ -464,6 +462,12 @@ impl Session { show_raw_agent_reasoning: config.show_raw_agent_reasoning, }; + let executor = Executor::new(ExecutorConfig::new( + turn_context.sandbox_policy.clone(), + turn_context.cwd.clone(), + config.codex_linux_sandbox_exe.clone(), + )); + let sess = Arc::new(Session { conversation_id, tx_event: tx_event.clone(), @@ -471,6 +475,7 @@ impl Session { active_turn: Mutex::new(None), services, next_internal_sub_id: AtomicU64::new(0), + executor, }); // Dispatch the SessionConfiguredEvent first and then report any errors. @@ -546,6 +551,11 @@ impl Session { } } + /// Emit an exec approval request event and await the user's decision. + /// + /// The request is keyed by `sub_id`/`call_id` so matching responses are delivered + /// to the correct in-flight turn. If the task is aborted, this returns the + /// default `ReviewDecision` (`Denied`). pub async fn request_command_approval( &self, sub_id: String, @@ -643,11 +653,6 @@ impl Session { } } - pub async fn add_approved_command(&self, cmd: Vec) { - let mut state = self.state.lock().await; - state.add_approved_command(cmd); - } - /// Records input items: always append to conversation history and /// persist these response items to rollout. async fn record_conversation_items(&self, items: &[ResponseItem]) { @@ -901,12 +906,13 @@ impl Session { /// command even on error. /// /// Returns the output of the exec tool call. - async fn run_exec_with_events<'a>( + async fn run_exec_with_events( &self, turn_diff_tracker: &mut TurnDiffTracker, begin_ctx: ExecCommandContext, - exec_args: ExecInvokeArgs<'a>, - ) -> crate::error::Result { + request: ExecutionRequest, + approval_policy: AskForApproval, + ) -> Result { let is_apply_patch = begin_ctx.apply_patch.is_some(); let sub_id = begin_ctx.sub_id.clone(); let call_id = begin_ctx.call_id.clone(); @@ -914,41 +920,14 @@ impl Session { self.on_exec_command_begin(turn_diff_tracker, begin_ctx.clone()) .await; - let ExecInvokeArgs { - params, - plan, - sandbox_policy, - sandbox_cwd, - codex_linux_sandbox_exe, - stdout_stream, - } = exec_args; + let result = self + .executor + .run(request, self, approval_policy, &sub_id, &call_id) + .await; - let registry = BackendRegistry::new(); - let runtime_ctx = ExecRuntimeContext { - sandbox_policy, - sandbox_cwd, - codex_linux_sandbox_exe, - stdout_stream, - }; + let normalized = normalize_exec_result(&result); + let borrowed = normalized.event_output(); - let result = run_with_plan(params, &plan, ®istry, &runtime_ctx).await; - - let output_stderr; - let borrowed: &ExecToolCallOutput = match &result { - Ok(output) => output, - Err(CodexErr::Sandbox(SandboxErr::Timeout { output })) => output, - Err(e) => { - output_stderr = ExecToolCallOutput { - exit_code: -1, - stdout: StreamOutput::new(String::new()), - stderr: StreamOutput::new(get_error_message_ui(e)), - aggregated_output: StreamOutput::new(get_error_message_ui(e)), - duration: Duration::default(), - timed_out: false, - }; - &output_stderr - } - }; self.on_exec_command_end( turn_diff_tracker, &sub_id, @@ -958,13 +937,15 @@ impl Session { ) .await; + drop(normalized); + result } /// Helper that emits a BackgroundEvent with the given message. This keeps /// the call‑sites terse so adding more diagnostics does not clutter the /// core agent logic. - async fn notify_background_event(&self, sub_id: &str, message: impl Into) { + pub(crate) async fn notify_background_event(&self, sub_id: &str, message: impl Into) { let event = Event { id: sub_id.to_string(), msg: EventMsg::BackgroundEvent(BackgroundEventEvent { @@ -2530,15 +2511,6 @@ fn parse_container_exec_arguments( }) } -pub struct ExecInvokeArgs<'a> { - pub params: ExecParams, - pub plan: ExecPlan, - pub sandbox_policy: &'a SandboxPolicy, - pub sandbox_cwd: &'a Path, - pub codex_linux_sandbox_exe: &'a Option, - pub stdout_stream: Option, -} - fn maybe_translate_shell_command( params: ExecParams, sess: &Session, @@ -2599,29 +2571,12 @@ async fn handle_container_exec_with_params( MaybeApplyPatchVerified::NotApplyPatch => None, }; - let approved_session_commands = { - let state = sess.state.lock().await; - state.approved_commands_ref().clone() + let command_for_display = if let Some(exec) = apply_patch_exec.as_ref() { + vec!["apply_patch".to_string(), exec.action.patch.clone()] + } else { + params.command.clone() }; - let prepared = prepare_exec_invocation( - sess, - turn_context, - &sub_id, - &call_id, - params, - apply_patch_exec, - approved_session_commands, - ) - .await?; - - let PreparedExec { - params, - plan, - command_for_display, - apply_patch_exec, - } = prepared; - let exec_command_context = ExecCommandContext { sub_id: sub_id.clone(), call_id: call_id.clone(), @@ -2638,28 +2593,41 @@ async fn handle_container_exec_with_params( ), }; - let params = maybe_translate_shell_command(params, sess, turn_context); - let plan_for_invocation = plan.clone(); + let translated_params = maybe_translate_shell_command(params, sess, turn_context); + let stdout_stream = if exec_command_context.apply_patch.is_some() { + None + } else { + Some(StdoutStream { + sub_id: sub_id.clone(), + call_id: call_id.clone(), + tx_event: sess.tx_event.clone(), + }) + }; + + let mode = match apply_patch_exec { + Some(exec) => ExecutionMode::ApplyPatch(exec), + None => ExecutionMode::Shell, + }; + + let request = ExecutionRequest { + params: translated_params, + approval_command: command_for_display, + mode, + stdout_stream, + }; + + sess.executor.update_environment( + turn_context.sandbox_policy.clone(), + turn_context.cwd.clone(), + sess.services.codex_linux_sandbox_exe.clone(), + ); + let output_result = sess .run_exec_with_events( turn_diff_tracker, - exec_command_context.clone(), - ExecInvokeArgs { - params: params.clone(), - plan: plan_for_invocation, - sandbox_policy: &turn_context.sandbox_policy, - sandbox_cwd: &turn_context.cwd, - codex_linux_sandbox_exe: &sess.services.codex_linux_sandbox_exe, - stdout_stream: if exec_command_context.apply_patch.is_some() { - None - } else { - Some(StdoutStream { - sub_id: sub_id.clone(), - call_id: call_id.clone(), - tx_event: sess.tx_event.clone(), - }) - }, - }, + exec_command_context, + request, + turn_context.approval_policy, ) .await; @@ -2673,142 +2641,16 @@ async fn handle_container_exec_with_params( Err(FunctionCallError::RespondToModel(content)) } } - Err(CodexErr::Sandbox(error)) => { - handle_sandbox_error( - turn_diff_tracker, - params, - exec_command_context, - error, - &plan, - sess, - turn_context, - ) - .await - } - Err(e) => Err(FunctionCallError::RespondToModel(format!( - "execution error: {e:?}" + Err(ExecError::Function(err)) => Err(err), + Err(ExecError::Codex(CodexErr::Sandbox(SandboxErr::Timeout { output }))) => Err( + FunctionCallError::RespondToModel(format_exec_output(&output)), + ), + Err(ExecError::Codex(err)) => Err(FunctionCallError::RespondToModel(format!( + "execution error: {err:?}" ))), } } -async fn handle_sandbox_error( - turn_diff_tracker: &mut TurnDiffTracker, - params: ExecParams, - exec_command_context: ExecCommandContext, - error: SandboxErr, - plan: &ExecPlan, - sess: &Session, - turn_context: &TurnContext, -) -> Result { - let call_id = exec_command_context.call_id.clone(); - let sub_id = exec_command_context.sub_id.clone(); - let cwd = exec_command_context.cwd.clone(); - - if let SandboxErr::Timeout { output } = &error { - let content = format_exec_output(output); - return Err(FunctionCallError::RespondToModel(content)); - } - - let ExecPlan::Approved { - sandbox: sandbox_type, - on_failure_escalate, - .. - } = plan - else { - return Err(FunctionCallError::RespondToModel( - "execution failed without an approved plan".to_string(), - )); - }; - - if !on_failure_escalate { - return Err(FunctionCallError::RespondToModel(format!( - "failed in sandbox {sandbox_type:?} with execution error: {error:?}" - ))); - } - - // Note that when `error` is `SandboxErr::Denied`, it could be a false - // positive. That is, it may have exited with a non-zero exit code, not - // because the sandbox denied it, but because that is its expected behavior, - // i.e., a grep command that did not match anything. Ideally we would - // include additional metadata on the command to indicate whether non-zero - // exit codes merit a retry. - - // For now, we categorically ask the user to retry without sandbox and - // emit the raw error as a background event. - sess.notify_background_event(&sub_id, format!("Execution failed: {error}")) - .await; - - let command_for_retry = params.command.clone(); - let decision = sess - .request_command_approval( - sub_id.clone(), - call_id.clone(), - command_for_retry.clone(), - cwd.clone(), - Some("command failed; retry without sandbox?".to_string()), - ) - .await; - - match decision { - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { - // Persist this command as pre‑approved for the - // remainder of the session so future - // executions skip the sandbox directly. - // TODO(ragona): Isn't this a bug? It always saves the command in an | fork? - sess.add_approved_command(command_for_retry.clone()).await; - // Inform UI we are retrying without sandbox. - sess.notify_background_event(&sub_id, "retrying command without sandbox") - .await; - - // This is an escalated retry; the policy will not be - // examined and the sandbox has been set to `None`. - let retry_output_result = sess - .run_exec_with_events( - turn_diff_tracker, - exec_command_context.clone(), - ExecInvokeArgs { - params, - plan: ExecPlan::approved(SandboxType::None, false, true), - sandbox_policy: &turn_context.sandbox_policy, - sandbox_cwd: &turn_context.cwd, - codex_linux_sandbox_exe: &sess.services.codex_linux_sandbox_exe, - stdout_stream: if exec_command_context.apply_patch.is_some() { - None - } else { - Some(StdoutStream { - sub_id: sub_id.clone(), - call_id: call_id.clone(), - tx_event: sess.tx_event.clone(), - }) - }, - }, - ) - .await; - - match retry_output_result { - Ok(retry_output) => { - let ExecToolCallOutput { exit_code, .. } = &retry_output; - let content = format_exec_output(&retry_output); - if *exit_code == 0 { - Ok(content) - } else { - Err(FunctionCallError::RespondToModel(content)) - } - } - Err(e) => Err(FunctionCallError::RespondToModel(format!( - "retry failed: {e}" - ))), - } - } - ReviewDecision::Denied | ReviewDecision::Abort => { - // Fall through to original failure handling. - Err(FunctionCallError::RespondToModel( - "exec command rejected by user".to_string(), - )) - } - } -} - fn format_exec_output_str(exec_output: &ExecToolCallOutput) -> String { let ExecToolCallOutput { aggregated_output, .. @@ -3366,6 +3208,11 @@ mod tests { user_shell: shell::Shell::Unknown, show_raw_agent_reasoning: config.show_raw_agent_reasoning, }; + let executor = Executor::new(ExecutorConfig::new( + turn_context.sandbox_policy.clone(), + turn_context.cwd.clone(), + None, + )); let session = Session { conversation_id, tx_event, @@ -3373,6 +3220,7 @@ mod tests { active_turn: Mutex::new(None), services, next_internal_sub_id: AtomicU64::new(0), + executor, }; (session, turn_context) } @@ -3433,6 +3281,11 @@ mod tests { user_shell: shell::Shell::Unknown, show_raw_agent_reasoning: config.show_raw_agent_reasoning, }; + let executor = Executor::new(ExecutorConfig::new( + config.sandbox_policy.clone(), + config.cwd.clone(), + None, + )); let session = Arc::new(Session { conversation_id, tx_event, @@ -3440,6 +3293,7 @@ mod tests { active_turn: Mutex::new(None), services, next_internal_sub_id: AtomicU64::new(0), + executor, }); (session, turn_context, rx_event) } diff --git a/codex-rs/core/src/executor/backends.rs b/codex-rs/core/src/executor/backends.rs new file mode 100644 index 0000000000..aaa7f08030 --- /dev/null +++ b/codex-rs/core/src/executor/backends.rs @@ -0,0 +1,95 @@ +use std::sync::Arc; + +use async_trait::async_trait; + +use crate::apply_patch::ApplyPatchExec; +use crate::exec::ExecParams; +use crate::exec::ExecToolCallOutput; +use crate::executor::sandbox::build_exec_params_for_apply_patch; +use crate::function_tool::FunctionCallError; + +pub(crate) enum ExecutionMode { + Shell, + ApplyPatch(ApplyPatchExec), +} + +#[async_trait] +/// Backend-specific hooks that prepare and post-process execution requests for a +/// given [`ExecutionMode`]. +pub(crate) trait ExecutionBackend: Send + Sync { + fn prepare( + &self, + params: ExecParams, + // Required for downcasting the apply_patch. + mode: &ExecutionMode, + ) -> Result; + + async fn finalize( + &self, + output: ExecToolCallOutput, + _mode: &ExecutionMode, + ) -> Result { + Ok(output) + } +} + +pub(crate) struct BackendStore { + shell: Arc, + apply_patch: Arc, +} + +impl BackendStore { + pub(crate) fn new() -> Self { + Self { + shell: Arc::new(ShellBackend), + apply_patch: Arc::new(ApplyPatchBackend), + } + } + + pub(crate) fn for_mode(&self, mode: &ExecutionMode) -> Arc { + match mode { + ExecutionMode::Shell => self.shell.clone(), + ExecutionMode::ApplyPatch(_) => self.apply_patch.clone(), + } + } +} + +pub(crate) fn default_backends() -> BackendStore { + BackendStore::new() +} + +struct ShellBackend; + +#[async_trait] +impl ExecutionBackend for ShellBackend { + fn prepare( + &self, + params: ExecParams, + mode: &ExecutionMode, + ) -> Result { + match mode { + ExecutionMode::Shell => Ok(params), + _ => Err(FunctionCallError::RespondToModel( + "shell backend invoked with non-shell mode".to_string(), + )), + } + } +} + +struct ApplyPatchBackend; + +#[async_trait] +impl ExecutionBackend for ApplyPatchBackend { + fn prepare( + &self, + params: ExecParams, + mode: &ExecutionMode, + ) -> Result { + match mode { + ExecutionMode::ApplyPatch(exec) => build_exec_params_for_apply_patch(exec, ¶ms), + ExecutionMode::Shell => Err(FunctionCallError::RespondToModel( + "apply_patch backend invoked without patch context".to_string(), + )), + } + } +} diff --git a/codex-rs/core/src/executor/cache.rs b/codex-rs/core/src/executor/cache.rs new file mode 100644 index 0000000000..5629c8893a --- /dev/null +++ b/codex-rs/core/src/executor/cache.rs @@ -0,0 +1,25 @@ +use std::collections::HashSet; +use std::sync::Arc; +use std::sync::Mutex; + +#[derive(Clone, Debug, Default)] +/// Thread-safe store of user approvals so repeated commands can reuse +/// previously granted trust. +pub(crate) struct ApprovalCache { + inner: Arc>>>, +} + +impl ApprovalCache { + pub(crate) fn insert(&self, command: Vec) { + if command.is_empty() { + return; + } + if let Ok(mut guard) = self.inner.lock() { + guard.insert(command); + } + } + + pub(crate) fn snapshot(&self) -> HashSet> { + self.inner.lock().map(|g| g.clone()).unwrap_or_default() + } +} diff --git a/codex-rs/core/src/executor/mod.rs b/codex-rs/core/src/executor/mod.rs new file mode 100644 index 0000000000..c3dce856e5 --- /dev/null +++ b/codex-rs/core/src/executor/mod.rs @@ -0,0 +1,11 @@ +mod backends; +mod cache; +mod runner; +mod sandbox; + +pub(crate) use backends::ExecutionMode; +pub(crate) use runner::ExecError; +pub(crate) use runner::ExecutionRequest; +pub(crate) use runner::Executor; +pub(crate) use runner::ExecutorConfig; +pub(crate) use runner::normalize_exec_result; diff --git a/codex-rs/core/src/executor/runner.rs b/codex-rs/core/src/executor/runner.rs new file mode 100644 index 0000000000..708568a73b --- /dev/null +++ b/codex-rs/core/src/executor/runner.rs @@ -0,0 +1,306 @@ +use std::path::PathBuf; +use std::sync::Arc; +use std::sync::RwLock; +use std::time::Duration; + +use thiserror::Error; + +use super::backends::BackendStore; +use super::backends::ExecutionBackend; +use super::backends::ExecutionMode; +use super::backends::default_backends; +use super::cache::ApprovalCache; +use crate::codex::Session; +use crate::error::CodexErr; +use crate::error::SandboxErr; +use crate::error::get_error_message_ui; +use crate::exec::ExecParams; +use crate::exec::ExecToolCallOutput; +use crate::exec::SandboxType; +use crate::exec::StdoutStream; +use crate::exec::StreamOutput; +use crate::exec::process_exec_tool_call; +use crate::executor::sandbox::select_sandbox; +use crate::function_tool::FunctionCallError; +use crate::protocol::AskForApproval; +use crate::protocol::ReviewDecision; +use crate::protocol::SandboxPolicy; + +#[derive(Clone, Debug)] +pub(crate) struct ExecutorConfig { + pub(crate) sandbox_policy: SandboxPolicy, + pub(crate) sandbox_cwd: PathBuf, + codex_linux_sandbox_exe: Option, +} + +impl ExecutorConfig { + pub(crate) fn new( + sandbox_policy: SandboxPolicy, + sandbox_cwd: PathBuf, + codex_linux_sandbox_exe: Option, + ) -> Self { + Self { + sandbox_policy, + sandbox_cwd, + codex_linux_sandbox_exe, + } + } +} + +#[derive(Debug, Error)] +pub enum ExecError { + #[error(transparent)] + Function(#[from] FunctionCallError), + #[error(transparent)] + Codex(#[from] CodexErr), +} + +impl ExecError { + pub(crate) fn rejection(msg: impl Into) -> Self { + FunctionCallError::RespondToModel(msg.into()).into() + } +} + +/// Coordinates sandbox selection, backend-specific preparation, and command +/// execution for tool calls requested by the model. +pub(crate) struct Executor { + backends: BackendStore, + approval_cache: ApprovalCache, + config: Arc>, +} + +impl Executor { + pub(crate) fn new(config: ExecutorConfig) -> Self { + Self { + backends: default_backends(), + approval_cache: ApprovalCache::default(), + config: Arc::new(RwLock::new(config)), + } + } + + /// Updates the sandbox policy and working directory used for future + /// executions without recreating the executor. + pub(crate) fn update_environment( + &self, + sandbox_policy: SandboxPolicy, + sandbox_cwd: PathBuf, + codex_linux_sandbox_exe: Option, + ) { + if let Ok(mut cfg) = self.config.write() { + cfg.sandbox_policy = sandbox_policy; + cfg.sandbox_cwd = sandbox_cwd; + cfg.codex_linux_sandbox_exe = codex_linux_sandbox_exe; + } + } + + /// Runs a prepared execution request end-to-end: prepares parameters, decides on + /// sandbox placement (prompting the user when necessary), launches the command, + /// and lets the backend post-process the final output. + pub(crate) async fn run( + &self, + mut request: ExecutionRequest, + session: &Session, + approval_policy: AskForApproval, + sub_id: &str, + call_id: &str, + ) -> Result { + // Step 1: Normalise parameters via the selected backend. + let backend = self.backends.for_mode(&request.mode); + request.params = backend + .prepare(request.params, &request.mode) + .map_err(ExecError::from)?; + + // Step 2: Snapshot sandbox configuration so it stays stable for this run. + let config = self + .config + .read() + .map_err(|_| ExecError::rejection("executor config poisoned"))? + .clone(); + + // Step 3: Decide sandbox placement, prompting for approval when needed. + let sandbox_decision = select_sandbox( + &request, + approval_policy, + self.approval_cache.snapshot(), + &config, + session, + sub_id, + call_id, + ) + .await?; + if sandbox_decision.record_session_approval { + self.approval_cache.insert(request.approval_command.clone()); + } + + // Step 4: Launch the command within the chosen sandbox. + let first_attempt = self + .spawn( + request.params.clone(), + sandbox_decision.initial_sandbox, + &config, + request.stdout_stream.clone(), + ) + .await; + + // Step 5: Handle sandbox outcomes, optionally escalating to an unsandboxed retry. + let raw_output = match first_attempt { + Ok(output) => output, + Err(CodexErr::Sandbox(SandboxErr::Timeout { output })) => { + return Err(CodexErr::Sandbox(SandboxErr::Timeout { output }).into()); + } + Err(CodexErr::Sandbox(error @ SandboxErr::Denied { .. })) => { + return if sandbox_decision.escalate_on_failure { + self.retry_without_sandbox( + &*backend, &request, &config, session, sub_id, call_id, error, + ) + .await + } else { + Err(ExecError::rejection(format!( + "failed in sandbox {:?} with execution error: {error:?}", + sandbox_decision.initial_sandbox + ))) + }; + } + Err(err) => return Err(err.into()), + }; + + // Step 6: Allow the backend to post-process the raw output. + backend + .finalize(raw_output, &request.mode) + .await + .map_err(ExecError::from) + } + + /// Fallback path invoked when a sandboxed run is denied so the user can + /// approve rerunning without isolation. + #[allow(clippy::too_many_arguments)] + async fn retry_without_sandbox( + &self, + backend: &dyn ExecutionBackend, + request: &ExecutionRequest, + config: &ExecutorConfig, + session: &Session, + sub_id: &str, + call_id: &str, + sandbox_error: SandboxErr, + ) -> Result { + session + .notify_background_event(sub_id, format!("Execution failed: {sandbox_error}")) + .await; + let decision = session + .request_command_approval( + sub_id.to_string(), + call_id.to_string(), + request.approval_command.clone(), + request.params.cwd.clone(), + Some("command failed; retry without sandbox?".to_string()), + ) + .await; + + match decision { + ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { + if matches!(decision, ReviewDecision::ApprovedForSession) { + self.approval_cache.insert(request.approval_command.clone()); + } + session + .notify_background_event(sub_id, "retrying command without sandbox") + .await; + + let retry_output = self + .spawn( + request.params.clone(), + SandboxType::None, + config, + request.stdout_stream.clone(), + ) + .await?; + + backend + .finalize(retry_output, &request.mode) + .await + .map_err(ExecError::from) + } + ReviewDecision::Denied | ReviewDecision::Abort => { + Err(ExecError::rejection("exec command rejected by user")) + } + } + } + + async fn spawn( + &self, + params: ExecParams, + sandbox: SandboxType, + config: &ExecutorConfig, + stdout_stream: Option, + ) -> Result { + process_exec_tool_call( + params, + sandbox, + &config.sandbox_policy, + &config.sandbox_cwd, + &config.codex_linux_sandbox_exe, + stdout_stream, + ) + .await + } +} + +pub(crate) struct ExecutionRequest { + pub params: ExecParams, + pub approval_command: Vec, + pub mode: ExecutionMode, + pub stdout_stream: Option, +} + +pub(crate) struct NormalizedExecOutput<'a> { + borrowed: Option<&'a ExecToolCallOutput>, + synthetic: Option, +} + +impl<'a> NormalizedExecOutput<'a> { + pub(crate) fn event_output(&'a self) -> &'a ExecToolCallOutput { + match (self.borrowed, self.synthetic.as_ref()) { + (Some(output), _) => output, + (None, Some(output)) => output, + (None, None) => unreachable!("normalized exec output missing data"), + } + } +} + +/// Converts a raw execution result into a uniform view that always exposes an +/// [`ExecToolCallOutput`], synthesizing error output when the command fails +/// before producing a response. +pub(crate) fn normalize_exec_result( + result: &Result, +) -> NormalizedExecOutput<'_> { + match result { + Ok(output) => NormalizedExecOutput { + borrowed: Some(output), + synthetic: None, + }, + Err(ExecError::Codex(CodexErr::Sandbox(SandboxErr::Timeout { output }))) => { + NormalizedExecOutput { + borrowed: Some(output.as_ref()), + synthetic: None, + } + } + Err(err) => { + let message = match err { + ExecError::Function(FunctionCallError::RespondToModel(msg)) => msg.clone(), + ExecError::Codex(e) => get_error_message_ui(e), + }; + let synthetic = ExecToolCallOutput { + exit_code: -1, + stdout: StreamOutput::new(String::new()), + stderr: StreamOutput::new(message.clone()), + aggregated_output: StreamOutput::new(message), + duration: Duration::default(), + timed_out: false, + }; + NormalizedExecOutput { + borrowed: None, + synthetic: Some(synthetic), + } + } + } +} diff --git a/codex-rs/core/src/executor/sandbox.rs b/codex-rs/core/src/executor/sandbox.rs new file mode 100644 index 0000000000..1f87d961e6 --- /dev/null +++ b/codex-rs/core/src/executor/sandbox.rs @@ -0,0 +1,193 @@ +use crate::CODEX_APPLY_PATCH_ARG1; +use crate::apply_patch::ApplyPatchExec; +use crate::codex::Session; +use crate::exec::ExecParams; +use crate::exec::SandboxType; +use crate::executor::ExecError; +use crate::executor::ExecutionMode; +use crate::executor::ExecutionRequest; +use crate::executor::ExecutorConfig; +use crate::function_tool::FunctionCallError; +use crate::safety::SafetyCheck; +use crate::safety::assess_command_safety; +use crate::safety::assess_patch_safety; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::ReviewDecision; +use std::collections::HashMap; +use std::collections::HashSet; +use std::env; + +/// Sandbox placement options selected for an execution run, including whether +/// to escalate after failures and whether approvals should persist. +pub(crate) struct SandboxDecision { + pub(crate) initial_sandbox: SandboxType, + pub(crate) escalate_on_failure: bool, + pub(crate) record_session_approval: bool, +} + +impl SandboxDecision { + fn auto(sandbox: SandboxType, escalate_on_failure: bool) -> Self { + Self { + initial_sandbox: sandbox, + escalate_on_failure, + record_session_approval: false, + } + } + + fn user_override(record_session_approval: bool) -> Self { + Self { + initial_sandbox: SandboxType::None, + escalate_on_failure: false, + record_session_approval, + } + } +} + +fn should_escalate_on_failure(approval: AskForApproval, sandbox: SandboxType) -> bool { + matches!( + (approval, sandbox), + ( + AskForApproval::UnlessTrusted | AskForApproval::OnFailure, + SandboxType::MacosSeatbelt | SandboxType::LinuxSeccomp + ) + ) +} + +/// Builds the command-line invocation that shells out to `codex apply_patch` +/// using the provided apply-patch request details. +pub(crate) fn build_exec_params_for_apply_patch( + exec: &ApplyPatchExec, + original: &ExecParams, +) -> Result { + let path_to_codex = env::current_exe() + .ok() + .map(|p| p.to_string_lossy().to_string()) + .ok_or_else(|| { + FunctionCallError::RespondToModel( + "failed to determine path to codex executable".to_string(), + ) + })?; + + let patch = exec.action.patch.clone(); + Ok(ExecParams { + command: vec![path_to_codex, CODEX_APPLY_PATCH_ARG1.to_string(), patch], + cwd: exec.action.cwd.clone(), + timeout_ms: original.timeout_ms, + // Run apply_patch with a minimal environment for determinism and to + // avoid leaking host environment variables into the patch process. + env: HashMap::new(), + with_escalated_permissions: original.with_escalated_permissions, + justification: original.justification.clone(), + }) +} + +/// Determines how a command should be sandboxed, prompting the user when +/// policy requires explicit approval. +pub async fn select_sandbox( + request: &ExecutionRequest, + approval_policy: AskForApproval, + approval_cache: HashSet>, + config: &ExecutorConfig, + session: &Session, + sub_id: &str, + call_id: &str, +) -> Result { + match &request.mode { + ExecutionMode::Shell => { + select_shell_sandbox( + request, + approval_policy, + approval_cache, + config, + session, + sub_id, + call_id, + ) + .await + } + ExecutionMode::ApplyPatch(exec) => { + select_apply_patch_sandbox(exec, approval_policy, config) + } + } +} + +async fn select_shell_sandbox( + request: &ExecutionRequest, + approval_policy: AskForApproval, + approved_snapshot: HashSet>, + config: &ExecutorConfig, + session: &Session, + sub_id: &str, + call_id: &str, +) -> Result { + let command_for_safety = if request.approval_command.is_empty() { + request.params.command.clone() + } else { + request.approval_command.clone() + }; + + let safety = assess_command_safety( + &command_for_safety, + approval_policy, + &config.sandbox_policy, + &approved_snapshot, + request.params.with_escalated_permissions.unwrap_or(false), + ); + + match safety { + SafetyCheck::AutoApprove { sandbox_type } => Ok(SandboxDecision::auto( + sandbox_type, + should_escalate_on_failure(approval_policy, sandbox_type), + )), + SafetyCheck::AskUser => { + let decision = session + .request_command_approval( + sub_id.to_string(), + call_id.to_string(), + request.approval_command.clone(), + request.params.cwd.clone(), + request.params.justification.clone(), + ) + .await; + + match decision { + ReviewDecision::Approved => Ok(SandboxDecision::user_override(false)), + ReviewDecision::ApprovedForSession => Ok(SandboxDecision::user_override(true)), + ReviewDecision::Denied | ReviewDecision::Abort => { + Err(ExecError::rejection("exec command rejected by user")) + } + } + } + SafetyCheck::Reject { reason } => Err(ExecError::rejection(format!( + "exec command rejected: {reason}" + ))), + } +} + +fn select_apply_patch_sandbox( + exec: &ApplyPatchExec, + approval_policy: AskForApproval, + config: &ExecutorConfig, +) -> Result { + if exec.user_explicitly_approved_this_action { + return Ok(SandboxDecision::user_override(false)); + } + + match assess_patch_safety( + &exec.action, + approval_policy, + &config.sandbox_policy, + &config.sandbox_cwd, + ) { + SafetyCheck::AutoApprove { sandbox_type } => Ok(SandboxDecision::auto( + sandbox_type, + should_escalate_on_failure(approval_policy, sandbox_type), + )), + SafetyCheck::AskUser => Err(ExecError::rejection( + "patch requires approval but none was recorded", + )), + SafetyCheck::Reject { reason } => { + Err(ExecError::rejection(format!("patch rejected: {reason}"))) + } + } +} diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 9613e40b12..edc04ab942 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -27,6 +27,7 @@ pub mod error; pub mod exec; mod exec_command; pub mod exec_env; +pub mod executor; mod flags; pub mod git_info; pub mod landlock; @@ -60,7 +61,6 @@ pub mod plan_tool; pub mod project_doc; mod rollout; pub(crate) mod safety; -pub mod sandbox; pub mod seatbelt; pub mod shell; pub mod spawn; diff --git a/codex-rs/core/src/sandbox/apply_patch_adapter.rs b/codex-rs/core/src/sandbox/apply_patch_adapter.rs deleted file mode 100644 index 7e55f43279..0000000000 --- a/codex-rs/core/src/sandbox/apply_patch_adapter.rs +++ /dev/null @@ -1,33 +0,0 @@ -use std::collections::HashMap; -use std::env; - -use crate::apply_patch::ApplyPatchExec; -use crate::apply_patch::CODEX_APPLY_PATCH_ARG1; -use crate::exec::ExecParams; -use crate::function_tool::FunctionCallError; - -pub(crate) fn build_exec_params_for_apply_patch( - exec: &ApplyPatchExec, - original: &ExecParams, -) -> Result { - let path_to_codex = env::current_exe() - .ok() - .map(|p| p.to_string_lossy().to_string()) - .ok_or_else(|| { - FunctionCallError::RespondToModel( - "failed to determine path to codex executable".to_string(), - ) - })?; - - let patch = exec.action.patch.clone(); - Ok(ExecParams { - command: vec![path_to_codex, CODEX_APPLY_PATCH_ARG1.to_string(), patch], - cwd: exec.action.cwd.clone(), - timeout_ms: original.timeout_ms, - // Run apply_patch with a minimal environment for determinism and to - // avoid leaking host environment variables into the patch process. - env: HashMap::new(), - with_escalated_permissions: original.with_escalated_permissions, - justification: original.justification.clone(), - }) -} diff --git a/codex-rs/core/src/sandbox/backend.rs b/codex-rs/core/src/sandbox/backend.rs deleted file mode 100644 index 02e5a87e4f..0000000000 --- a/codex-rs/core/src/sandbox/backend.rs +++ /dev/null @@ -1,87 +0,0 @@ -use std::path::Path; -use std::path::PathBuf; - -use async_trait::async_trait; - -use crate::error::Result; -use crate::exec::ExecParams; -use crate::exec::ExecToolCallOutput; -use crate::exec::SandboxType; -use crate::exec::StdoutStream; -use crate::exec::process_exec_tool_call; -use crate::protocol::SandboxPolicy; - -#[async_trait] -pub trait SpawnBackend: Send + Sync { - fn sandbox_type(&self) -> SandboxType; - - async fn spawn( - &self, - params: ExecParams, - sandbox_policy: &SandboxPolicy, - sandbox_cwd: &Path, - codex_linux_sandbox_exe: &Option, - stdout_stream: Option, - ) -> Result { - process_exec_tool_call( - params, - self.sandbox_type(), - sandbox_policy, - sandbox_cwd, - codex_linux_sandbox_exe, - stdout_stream, - ) - .await - } -} - -#[derive(Clone, Copy, Debug, Default)] -pub struct DirectBackend; - -#[async_trait] -impl SpawnBackend for DirectBackend { - fn sandbox_type(&self) -> SandboxType { - SandboxType::None - } -} - -#[derive(Clone, Copy, Debug, Default)] -pub struct SeatbeltBackend; - -#[async_trait] -impl SpawnBackend for SeatbeltBackend { - fn sandbox_type(&self) -> SandboxType { - SandboxType::MacosSeatbelt - } -} - -#[derive(Clone, Copy, Debug, Default)] -pub struct LinuxBackend; - -#[async_trait] -impl SpawnBackend for LinuxBackend { - fn sandbox_type(&self) -> SandboxType { - SandboxType::LinuxSeccomp - } -} - -#[derive(Default)] -pub struct BackendRegistry { - direct: DirectBackend, - seatbelt: SeatbeltBackend, - linux: LinuxBackend, -} - -impl BackendRegistry { - pub fn new() -> Self { - Self::default() - } - - pub fn for_type(&self, sandbox: SandboxType) -> &dyn SpawnBackend { - match sandbox { - SandboxType::None => &self.direct, - SandboxType::MacosSeatbelt => &self.seatbelt, - SandboxType::LinuxSeccomp => &self.linux, - } - } -} diff --git a/codex-rs/core/src/sandbox/mod.rs b/codex-rs/core/src/sandbox/mod.rs deleted file mode 100644 index d1553f0d17..0000000000 --- a/codex-rs/core/src/sandbox/mod.rs +++ /dev/null @@ -1,51 +0,0 @@ -mod apply_patch_adapter; -mod backend; -mod planner; - -pub use backend::BackendRegistry; -pub use backend::DirectBackend; -pub use backend::LinuxBackend; -pub use backend::SeatbeltBackend; -pub use backend::SpawnBackend; -pub use planner::ExecPlan; -pub use planner::ExecRequest; -pub use planner::PatchExecRequest; -pub(crate) use planner::PreparedExec; -pub use planner::plan_apply_patch; -pub use planner::plan_exec; -pub(crate) use planner::prepare_exec_invocation; - -use crate::error::Result; -use crate::exec::ExecParams; -use crate::exec::ExecToolCallOutput; -use crate::exec::StdoutStream; -use crate::protocol::SandboxPolicy; - -pub struct ExecRuntimeContext<'a> { - pub sandbox_policy: &'a SandboxPolicy, - pub sandbox_cwd: &'a std::path::Path, - pub codex_linux_sandbox_exe: &'a Option, - pub stdout_stream: Option, -} - -pub async fn run_with_plan( - params: ExecParams, - plan: &ExecPlan, - registry: &BackendRegistry, - runtime_ctx: &ExecRuntimeContext<'_>, -) -> Result { - let ExecPlan::Approved { sandbox, .. } = plan else { - unreachable!("run_with_plan called without approved plan"); - }; - - registry - .for_type(*sandbox) - .spawn( - params, - runtime_ctx.sandbox_policy, - runtime_ctx.sandbox_cwd, - runtime_ctx.codex_linux_sandbox_exe, - runtime_ctx.stdout_stream.clone(), - ) - .await -} diff --git a/codex-rs/core/src/sandbox/planner.rs b/codex-rs/core/src/sandbox/planner.rs deleted file mode 100644 index 2799087871..0000000000 --- a/codex-rs/core/src/sandbox/planner.rs +++ /dev/null @@ -1,217 +0,0 @@ -use std::collections::HashSet; -use std::path::Path; - -use codex_apply_patch::ApplyPatchAction; - -use super::apply_patch_adapter::build_exec_params_for_apply_patch; -use crate::apply_patch::ApplyPatchExec; -use crate::codex::Session; -use crate::codex::TurnContext; -use crate::exec::ExecParams; -use crate::exec::SandboxType; -use crate::function_tool::FunctionCallError; -use crate::protocol::AskForApproval; -use crate::protocol::ReviewDecision; -use crate::protocol::SandboxPolicy; -use crate::safety::SafetyCheck; -use crate::safety::assess_command_safety; -use crate::safety::assess_patch_safety; - -#[derive(Clone, Debug)] -pub struct ExecRequest<'a> { - pub params: &'a ExecParams, - pub approval: AskForApproval, - pub policy: &'a SandboxPolicy, - pub approved_session_commands: &'a HashSet>, -} - -#[derive(Clone, Debug)] -pub enum ExecPlan { - Reject { - reason: String, - }, - AskUser { - reason: Option, - }, - Approved { - sandbox: SandboxType, - on_failure_escalate: bool, - approved_by_user: bool, - }, -} - -impl ExecPlan { - pub fn approved( - sandbox: SandboxType, - on_failure_escalate: bool, - approved_by_user: bool, - ) -> Self { - ExecPlan::Approved { - sandbox, - on_failure_escalate, - approved_by_user, - } - } -} - -pub fn plan_exec(req: &ExecRequest<'_>) -> ExecPlan { - let params = req.params; - let with_escalated_permissions = params.with_escalated_permissions.unwrap_or(false); - let safety = assess_command_safety( - ¶ms.command, - req.approval, - req.policy, - req.approved_session_commands, - with_escalated_permissions, - ); - - match safety { - SafetyCheck::AutoApprove { sandbox_type } => ExecPlan::Approved { - sandbox: sandbox_type, - on_failure_escalate: should_escalate_on_failure(req.approval, sandbox_type), - approved_by_user: false, - }, - SafetyCheck::AskUser => ExecPlan::AskUser { - reason: params.justification.clone(), - }, - SafetyCheck::Reject { reason } => ExecPlan::Reject { reason }, - } -} - -#[derive(Clone, Debug)] -pub struct PatchExecRequest<'a> { - pub action: &'a ApplyPatchAction, - pub approval: AskForApproval, - pub policy: &'a SandboxPolicy, - pub cwd: &'a Path, - pub user_explicitly_approved: bool, -} - -pub fn plan_apply_patch(req: &PatchExecRequest<'_>) -> ExecPlan { - if req.user_explicitly_approved { - ExecPlan::Approved { - sandbox: SandboxType::None, - on_failure_escalate: false, - approved_by_user: true, - } - } else { - match assess_patch_safety(req.action, req.approval, req.policy, req.cwd) { - SafetyCheck::AutoApprove { sandbox_type } => ExecPlan::Approved { - sandbox: sandbox_type, - on_failure_escalate: should_escalate_on_failure(req.approval, sandbox_type), - approved_by_user: false, - }, - SafetyCheck::AskUser => ExecPlan::AskUser { reason: None }, - SafetyCheck::Reject { reason } => ExecPlan::Reject { reason }, - } - } -} - -#[derive(Debug)] -pub(crate) struct PreparedExec { - pub(crate) params: ExecParams, - pub(crate) plan: ExecPlan, - pub(crate) command_for_display: Vec, - pub(crate) apply_patch_exec: Option, -} - -pub(crate) async fn prepare_exec_invocation( - sess: &Session, - turn_context: &TurnContext, - sub_id: &str, - call_id: &str, - params: ExecParams, - apply_patch_exec: Option, - approved_session_commands: HashSet>, -) -> Result { - let mut params = params; - - let (plan, command_for_display) = if let Some(exec) = apply_patch_exec.as_ref() { - params = build_exec_params_for_apply_patch(exec, ¶ms)?; - let command_for_display = vec!["apply_patch".to_string(), exec.action.patch.clone()]; - - let plan_req = PatchExecRequest { - action: &exec.action, - approval: turn_context.approval_policy, - policy: &turn_context.sandbox_policy, - cwd: &turn_context.cwd, - user_explicitly_approved: exec.user_explicitly_approved_this_action, - }; - - let plan = match plan_apply_patch(&plan_req) { - plan @ ExecPlan::Approved { .. } => plan, - ExecPlan::AskUser { .. } => { - return Err(FunctionCallError::RespondToModel( - "patch requires approval but none was recorded".to_string(), - )); - } - ExecPlan::Reject { reason } => { - return Err(FunctionCallError::RespondToModel(format!( - "patch rejected: {reason}" - ))); - } - }; - - (plan, command_for_display) - } else { - let command_for_display = params.command.clone(); - - let initial_plan = plan_exec(&ExecRequest { - params: ¶ms, - approval: turn_context.approval_policy, - policy: &turn_context.sandbox_policy, - approved_session_commands: &approved_session_commands, - }); - - let plan = match initial_plan { - plan @ ExecPlan::Approved { .. } => plan, - ExecPlan::AskUser { reason } => { - let decision = sess - .request_command_approval( - sub_id.to_string(), - call_id.to_string(), - params.command.clone(), - params.cwd.clone(), - reason, - ) - .await; - match decision { - ReviewDecision::Approved => ExecPlan::approved(SandboxType::None, false, true), - ReviewDecision::ApprovedForSession => { - sess.add_approved_command(params.command.clone()).await; - ExecPlan::approved(SandboxType::None, false, true) - } - ReviewDecision::Denied | ReviewDecision::Abort => { - return Err(FunctionCallError::RespondToModel( - "exec command rejected by user".to_string(), - )); - } - } - } - ExecPlan::Reject { reason } => { - return Err(FunctionCallError::RespondToModel(format!( - "exec command rejected: {reason:?}" - ))); - } - }; - - (plan, command_for_display) - }; - - Ok(PreparedExec { - params, - plan, - command_for_display, - apply_patch_exec, - }) -} - -fn should_escalate_on_failure(approval: AskForApproval, sandbox: SandboxType) -> bool { - matches!( - (approval, sandbox), - ( - AskForApproval::UnlessTrusted | AskForApproval::OnFailure, - SandboxType::MacosSeatbelt | SandboxType::LinuxSeccomp - ) - ) -} diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index ee0c5fc976..f170a10c18 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -1,7 +1,5 @@ //! Session-wide mutable state. -use std::collections::HashSet; - use codex_protocol::models::ResponseItem; use crate::conversation_history::ConversationHistory; @@ -12,7 +10,6 @@ use crate::protocol::TokenUsageInfo; /// Persistent, session-scoped state previously stored directly on `Session`. #[derive(Default)] pub(crate) struct SessionState { - pub(crate) approved_commands: HashSet>, pub(crate) history: ConversationHistory, pub(crate) token_info: Option, pub(crate) latest_rate_limits: Option, @@ -44,15 +41,6 @@ impl SessionState { self.history.replace(items); } - // Approved command helpers - pub(crate) fn add_approved_command(&mut self, cmd: Vec) { - self.approved_commands.insert(cmd); - } - - pub(crate) fn approved_commands_ref(&self) -> &HashSet> { - &self.approved_commands - } - // Token/rate limit helpers pub(crate) fn update_token_info_from_usage( &mut self,