From ed45f852093493f0137386bee5f701cb28262a3a Mon Sep 17 00:00:00 2001 From: jimmyfraiture Date: Tue, 30 Sep 2025 11:27:23 +0100 Subject: [PATCH] RV 1 --- codex-rs/core/src/apply_patch.rs | 25 +++ codex-rs/core/src/codex.rs | 3 +- codex-rs/core/src/executor/backends.rs | 35 ++-- codex-rs/core/src/executor/cache.rs | 26 +++ codex-rs/core/src/executor/runner.rs | 83 +++++++-- codex-rs/core/src/executor/sandbox.rs | 229 ++++++++++++++++++++++--- 6 files changed, 350 insertions(+), 51 deletions(-) diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 8f372fb2d3..7efae19f15 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -109,3 +109,28 @@ pub(crate) fn convert_apply_patch_to_protocol( } result } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + use tempfile::tempdir; + + #[test] + fn convert_apply_patch_maps_add_variant() { + let tmp = tempdir().expect("tmp"); + let p = tmp.path().join("a.txt"); + // Create an action with a single Add change + let action = ApplyPatchAction::new_add_for_test(&p, "hello".to_string()); + + let got = convert_apply_patch_to_protocol(&action); + + assert_eq!( + got.get(&p), + Some(&FileChange::Add { + content: "hello".to_string() + }) + ); + } +} diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 84b1c53b9c..cde64105f4 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -2616,10 +2616,9 @@ async fn handle_container_exec_with_params( stdout_stream, }; - sess.executor.update_environment( + sess.executor.update_environment( // todo this should not be needed ? Not sure what it means turn_context.sandbox_policy.clone(), turn_context.cwd.clone(), - sess.services.codex_linux_sandbox_exe.clone(), ); let output_result = sess diff --git a/codex-rs/core/src/executor/backends.rs b/codex-rs/core/src/executor/backends.rs index aaa7f08030..09c786a9d4 100644 --- a/codex-rs/core/src/executor/backends.rs +++ b/codex-rs/core/src/executor/backends.rs @@ -1,11 +1,13 @@ +use std::collections::HashMap; +use std::env; use std::sync::Arc; use async_trait::async_trait; use crate::apply_patch::ApplyPatchExec; +use crate::CODEX_APPLY_PATCH_ARG1; 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 { @@ -23,14 +25,6 @@ pub(crate) trait ExecutionBackend: Send + Sync { // Required for downcasting the apply_patch. mode: &ExecutionMode, ) -> Result; - - async fn finalize( - &self, - output: ExecToolCallOutput, - _mode: &ExecutionMode, - ) -> Result { - Ok(output) - } } pub(crate) struct BackendStore { @@ -86,7 +80,28 @@ impl ExecutionBackend for ApplyPatchBackend { mode: &ExecutionMode, ) -> Result { match mode { - ExecutionMode::ApplyPatch(exec) => build_exec_params_for_apply_patch(exec, ¶ms), + ExecutionMode::ApplyPatch(exec) => { + 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: params.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: params.with_escalated_permissions, + justification: params.justification, + }) + }, 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 index 5629c8893a..737ecb927c 100644 --- a/codex-rs/core/src/executor/cache.rs +++ b/codex-rs/core/src/executor/cache.rs @@ -23,3 +23,29 @@ impl ApprovalCache { self.inner.lock().map(|g| g.clone()).unwrap_or_default() } } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn insert_ignores_empty_and_dedupes() { + let cache = ApprovalCache::default(); + + // Empty should be ignored + cache.insert(vec![]); + assert!(cache.snapshot().is_empty()); + + // Insert a command and verify snapshot contains it + let cmd = vec!["foo".to_string(), "bar".to_string()]; + cache.insert(cmd.clone()); + let snap1 = cache.snapshot(); + assert!(snap1.contains(&cmd)); + + // Reinserting should not create duplicates + cache.insert(cmd); + let snap2 = cache.snapshot(); + assert_eq!(snap1, snap2); + } +} diff --git a/codex-rs/core/src/executor/runner.rs b/codex-rs/core/src/executor/runner.rs index 708568a73b..a65da13802 100644 --- a/codex-rs/core/src/executor/runner.rs +++ b/codex-rs/core/src/executor/runner.rs @@ -84,12 +84,10 @@ impl Executor { &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; } } @@ -164,11 +162,7 @@ impl Executor { 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) + Ok(raw_output) } /// Fallback path invoked when a sandboxed run is denied so the user can @@ -215,10 +209,7 @@ impl Executor { ) .await?; - backend - .finalize(retry_output, &request.mode) - .await - .map_err(ExecError::from) + Ok(retry_output) } ReviewDecision::Denied | ReviewDecision::Abort => { Err(ExecError::rejection("exec command rejected by user")) @@ -304,3 +295,73 @@ pub(crate) fn normalize_exec_result( } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::error::CodexErr; + use crate::error::EnvVarError; + use crate::error::SandboxErr; + use crate::exec::StreamOutput; + use pretty_assertions::assert_eq; + + fn make_output(text: &str) -> ExecToolCallOutput { + ExecToolCallOutput { + exit_code: 1, + stdout: StreamOutput::new(String::new()), + stderr: StreamOutput::new(String::new()), + aggregated_output: StreamOutput::new(text.to_string()), + duration: Duration::from_millis(123), + timed_out: false, + } + } + + #[test] + fn normalize_success_borrows() { + let out = make_output("ok"); + let result: Result = Ok(out); + let normalized = normalize_exec_result(&result); + assert_eq!(normalized.event_output().aggregated_output.text, "ok"); + } + + #[test] + fn normalize_timeout_borrows_embedded_output() { + let out = make_output("timed out payload"); + let err = CodexErr::Sandbox(SandboxErr::Timeout { + output: Box::new(out), + }); + let result: Result = Err(ExecError::Codex(err)); + let normalized = normalize_exec_result(&result); + assert_eq!( + normalized.event_output().aggregated_output.text, + "timed out payload" + ); + } + + #[test] + fn normalize_function_error_synthesizes_payload() { + let err = FunctionCallError::RespondToModel("boom".to_string()); + let result: Result = Err(ExecError::Function(err)); + let normalized = normalize_exec_result(&result); + assert_eq!(normalized.event_output().aggregated_output.text, "boom"); + } + + #[test] + fn normalize_codex_error_synthesizes_user_message() { + // Use a simple EnvVar error which formats to a clear message + let e = CodexErr::EnvVar(EnvVarError { + var: "FOO".to_string(), + instructions: Some("set it".to_string()), + }); + let result: Result = Err(ExecError::Codex(e)); + let normalized = normalize_exec_result(&result); + assert!( + normalized + .event_output() + .aggregated_output + .text + .contains("Missing environment variable: `FOO`"), + "expected synthesized user-friendly message" + ); + } +} diff --git a/codex-rs/core/src/executor/sandbox.rs b/codex-rs/core/src/executor/sandbox.rs index 1f87d961e6..87fdc0f00a 100644 --- a/codex-rs/core/src/executor/sandbox.rs +++ b/codex-rs/core/src/executor/sandbox.rs @@ -53,34 +53,6 @@ fn should_escalate_on_failure(approval: AskForApproval, sandbox: SandboxType) -> ) } -/// 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( @@ -191,3 +163,204 @@ fn select_apply_patch_sandbox( } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::codex::make_session_and_context; + use crate::exec::ExecParams; + use crate::protocol::SandboxPolicy; + use codex_apply_patch::ApplyPatchAction; + use pretty_assertions::assert_eq; + + #[tokio::test] + async fn select_apply_patch_user_override_when_explicit() { + let (session, _ctx) = make_session_and_context(); + let tmp = tempfile::tempdir().expect("tmp"); + let p = tmp.path().join("a.txt"); + let action = ApplyPatchAction::new_add_for_test(&p, "hello".to_string()); + let exec = ApplyPatchExec { + action, + user_explicitly_approved_this_action: true, + }; + let cfg = ExecutorConfig::new(SandboxPolicy::ReadOnly, std::env::temp_dir(), None); + let request = ExecutionRequest { + params: ExecParams { + command: vec!["apply_patch".into()], + cwd: std::env::temp_dir(), + timeout_ms: None, + env: std::collections::HashMap::new(), + with_escalated_permissions: None, + justification: None, + }, + approval_command: vec!["apply_patch".into()], + mode: ExecutionMode::ApplyPatch(exec), + stdout_stream: None, + }; + let decision = select_sandbox( + &request, + AskForApproval::OnRequest, + Default::default(), + &cfg, + &session, + "sub", + "call", + ) + .await + .expect("ok"); + // Explicit user override runs without sandbox + assert_eq!(decision.initial_sandbox, SandboxType::None); + assert_eq!(decision.escalate_on_failure, false); + } + + #[tokio::test] + async fn select_apply_patch_autoapprove_in_danger() { + let (session, _ctx) = make_session_and_context(); + let tmp = tempfile::tempdir().expect("tmp"); + let p = tmp.path().join("a.txt"); + let action = ApplyPatchAction::new_add_for_test(&p, "hello".to_string()); + let exec = ApplyPatchExec { + action, + user_explicitly_approved_this_action: false, + }; + let cfg = ExecutorConfig::new(SandboxPolicy::DangerFullAccess, std::env::temp_dir(), None); + let request = ExecutionRequest { + params: ExecParams { + command: vec!["apply_patch".into()], + cwd: std::env::temp_dir(), + timeout_ms: None, + env: std::collections::HashMap::new(), + with_escalated_permissions: None, + justification: None, + }, + approval_command: vec!["apply_patch".into()], + mode: ExecutionMode::ApplyPatch(exec), + stdout_stream: None, + }; + let decision = select_sandbox( + &request, + AskForApproval::OnRequest, + Default::default(), + &cfg, + &session, + "sub", + "call", + ) + .await + .expect("ok"); + // On platforms with a sandbox, DangerFullAccess still prefers it + let expected = crate::safety::get_platform_sandbox().unwrap_or(SandboxType::None); + assert_eq!(decision.initial_sandbox, expected); + assert_eq!(decision.escalate_on_failure, false); + } + + #[tokio::test] + async fn select_apply_patch_requires_approval_on_unless_trusted() { + let (session, _ctx) = make_session_and_context(); + let tempdir = tempfile::tempdir().expect("tmpdir"); + let p = tempdir.path().join("a.txt"); + let action = ApplyPatchAction::new_add_for_test(&p, "hello".to_string()); + let exec = ApplyPatchExec { + action, + user_explicitly_approved_this_action: false, + }; + let cfg = ExecutorConfig::new(SandboxPolicy::ReadOnly, std::env::temp_dir(), None); + let request = ExecutionRequest { + params: ExecParams { + command: vec!["apply_patch".into()], + cwd: std::env::temp_dir(), + timeout_ms: None, + env: std::collections::HashMap::new(), + with_escalated_permissions: None, + justification: None, + }, + approval_command: vec!["apply_patch".into()], + mode: ExecutionMode::ApplyPatch(exec), + stdout_stream: None, + }; + let result = select_sandbox( + &request, + AskForApproval::UnlessTrusted, + Default::default(), + &cfg, + &session, + "sub", + "call", + ) + .await; + match result { + Ok(_) => panic!("expected error"), + Err(ExecError::Function(FunctionCallError::RespondToModel(msg))) => { + assert!(msg.contains("requires approval")) + } + Err(other) => panic!("unexpected error: {other:?}"), + } + } + + #[tokio::test] + async fn select_shell_autoapprove_in_danger_mode() { + let (session, _ctx) = make_session_and_context(); + let cfg = ExecutorConfig::new(SandboxPolicy::DangerFullAccess, std::env::temp_dir(), None); + let request = ExecutionRequest { + params: ExecParams { + command: vec!["some-unknown".into()], + cwd: std::env::temp_dir(), + timeout_ms: None, + env: std::collections::HashMap::new(), + with_escalated_permissions: None, + justification: None, + }, + approval_command: vec!["some-unknown".into()], + mode: ExecutionMode::Shell, + stdout_stream: None, + }; + let decision = select_sandbox( + &request, + AskForApproval::OnRequest, + Default::default(), + &cfg, + &session, + "sub", + "call", + ) + .await + .expect("ok"); + assert_eq!(decision.initial_sandbox, SandboxType::None); + assert_eq!(decision.escalate_on_failure, false); + } + + #[cfg(any(target_os = "macos", target_os = "linux"))] + #[tokio::test] + async fn select_shell_escalates_on_failure_with_platform_sandbox() { + let (session, _ctx) = make_session_and_context(); + let cfg = ExecutorConfig::new(SandboxPolicy::ReadOnly, std::env::temp_dir(), None); + let request = ExecutionRequest { + params: ExecParams { + // Unknown command => untrusted but not flagged dangerous + command: vec!["some-unknown".into()], + cwd: std::env::temp_dir(), + timeout_ms: None, + env: std::collections::HashMap::new(), + with_escalated_permissions: None, + justification: None, + }, + approval_command: vec!["some-unknown".into()], + mode: ExecutionMode::Shell, + stdout_stream: None, + }; + let decision = select_sandbox( + &request, + AskForApproval::OnFailure, + Default::default(), + &cfg, + &session, + "sub", + "call", + ) + .await + .expect("ok"); + // On macOS/Linux we should have a platform sandbox and escalate on failure + assert_ne!(decision.initial_sandbox, SandboxType::None); + assert_eq!(decision.escalate_on_failure, true); + } +}