From 72b95db12f4d7273e25deccb45c3197481642c3a Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 2 Dec 2025 17:54:02 +0000 Subject: [PATCH] feat: intercept apply_patch for unified_exec (#7446) --- codex-rs/core/src/config/mod.rs | 18 +- codex-rs/core/src/git_info.rs | 7 +- codex-rs/core/src/safety.rs | 7 +- .../core/src/tools/handlers/apply_patch.rs | 78 +++++++ codex-rs/core/src/tools/handlers/shell.rs | 90 ++------ .../core/src/tools/handlers/unified_exec.rs | 24 ++- codex-rs/core/src/util.rs | 10 + codex-rs/core/tests/suite/unified_exec.rs | 203 ++++++++++++++++++ 8 files changed, 331 insertions(+), 106 deletions(-) diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 44cc651a0..5e768ba9d 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -34,6 +34,7 @@ use crate::project_doc::DEFAULT_PROJECT_DOC_FILENAME; use crate::project_doc::LOCAL_PROJECT_DOC_FILENAME; use crate::protocol::AskForApproval; use crate::protocol::SandboxPolicy; +use crate::util::resolve_path; use codex_app_server_protocol::Tools; use codex_app_server_protocol::UserSavedConfig; use codex_protocol::config_types::ForcedLoginMethod; @@ -1016,15 +1017,8 @@ impl Config { let additional_writable_roots: Vec = additional_writable_roots .into_iter() .map(|path| { - let absolute = if path.is_absolute() { - path - } else { - resolved_cwd.join(path) - }; - match canonicalize(&absolute) { - Ok(canonical) => canonical, - Err(_) => absolute, - } + let absolute = resolve_path(&resolved_cwd, &path); + canonicalize(&absolute).unwrap_or(absolute) }) .collect(); let active_project = cfg @@ -1299,11 +1293,7 @@ impl Config { return Ok(None); }; - let full_path = if p.is_relative() { - cwd.join(p) - } else { - p.to_path_buf() - }; + let full_path = resolve_path(cwd, p); let contents = std::fs::read_to_string(&full_path).map_err(|e| { std::io::Error::new( diff --git a/codex-rs/core/src/git_info.rs b/codex-rs/core/src/git_info.rs index 4284ff126..065f1280a 100644 --- a/codex-rs/core/src/git_info.rs +++ b/codex-rs/core/src/git_info.rs @@ -2,6 +2,7 @@ use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; +use crate::util::resolve_path; use codex_app_server_protocol::GitSha; use codex_protocol::protocol::GitInfo; use futures::future::join_all; @@ -548,11 +549,7 @@ pub fn resolve_root_git_project_for_trust(cwd: &Path) -> Option { .trim() .to_string(); - let git_dir_path_raw = if Path::new(&git_dir_s).is_absolute() { - PathBuf::from(&git_dir_s) - } else { - base.join(&git_dir_s) - }; + let git_dir_path_raw = resolve_path(base, &PathBuf::from(&git_dir_s)); // Normalize to handle macOS /var vs /private/var and resolve ".." segments. let git_dir_path = std::fs::canonicalize(&git_dir_path_raw).unwrap_or(git_dir_path_raw); diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index 1e81981fd..e82e10745 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -6,6 +6,7 @@ use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use crate::exec::SandboxType; +use crate::util::resolve_path; use crate::protocol::AskForApproval; use crate::protocol::SandboxPolicy; @@ -150,11 +151,7 @@ fn is_write_patch_constrained_to_writable_paths( // and roots are converted to absolute, normalized forms before the // prefix check. let is_path_writable = |p: &PathBuf| { - let abs = if p.is_absolute() { - p.clone() - } else { - cwd.join(p) - }; + let abs = resolve_path(cwd, p); let abs = match normalize(&abs) { Some(v) => v, None => return false, diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 2109f1d2c..850c74f12 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -1,4 +1,5 @@ use std::collections::BTreeMap; +use std::path::Path; use crate::apply_patch; use crate::apply_patch::InternalApplyPatchInvocation; @@ -7,7 +8,10 @@ use crate::client_common::tools::FreeformTool; use crate::client_common::tools::FreeformToolFormat; use crate::client_common::tools::ResponsesApiTool; use crate::client_common::tools::ToolSpec; +use crate::codex::Session; +use crate::codex::TurnContext; use crate::function_tool::FunctionCallError; +use crate::tools::context::SharedTurnDiffTracker; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; @@ -164,6 +168,80 @@ pub enum ApplyPatchToolType { Function, } +#[allow(clippy::too_many_arguments)] +pub(crate) async fn intercept_apply_patch( + command: &[String], + cwd: &Path, + timeout_ms: Option, + session: &Session, + turn: &TurnContext, + tracker: Option<&SharedTurnDiffTracker>, + call_id: &str, + tool_name: &str, +) -> Result, FunctionCallError> { + match codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd) { + codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { + match apply_patch::apply_patch(session, turn, call_id, changes).await { + InternalApplyPatchInvocation::Output(item) => { + let content = item?; + Ok(Some(ToolOutput::Function { + content, + content_items: None, + success: Some(true), + })) + } + InternalApplyPatchInvocation::DelegateToExec(apply) => { + let emitter = ToolEmitter::apply_patch( + convert_apply_patch_to_protocol(&apply.action), + !apply.user_explicitly_approved_this_action, + ); + let event_ctx = + ToolEventCtx::new(session, turn, call_id, tracker.as_ref().copied()); + emitter.begin(event_ctx).await; + + let req = ApplyPatchRequest { + patch: apply.action.patch.clone(), + cwd: apply.action.cwd.clone(), + timeout_ms, + user_explicitly_approved: apply.user_explicitly_approved_this_action, + codex_exe: turn.codex_linux_sandbox_exe.clone(), + }; + + let mut orchestrator = ToolOrchestrator::new(); + let mut runtime = ApplyPatchRuntime::new(); + let tool_ctx = ToolCtx { + session, + turn, + call_id: call_id.to_string(), + tool_name: tool_name.to_string(), + }; + let out = orchestrator + .run(&mut runtime, &req, &tool_ctx, turn, turn.approval_policy) + .await; + let event_ctx = + ToolEventCtx::new(session, turn, call_id, tracker.as_ref().copied()); + let content = emitter.finish(event_ctx, out).await?; + Ok(Some(ToolOutput::Function { + content, + content_items: None, + success: Some(true), + })) + } + } + } + codex_apply_patch::MaybeApplyPatchVerified::CorrectnessError(parse_error) => { + Err(FunctionCallError::RespondToModel(format!( + "apply_patch verification failed: {parse_error}" + ))) + } + codex_apply_patch::MaybeApplyPatchVerified::ShellParseError(error) => { + tracing::trace!("Failed to parse apply_patch input, {error:?}"); + Ok(None) + } + codex_apply_patch::MaybeApplyPatchVerified::NotApplyPatch => Ok(None), + } +} + /// Returns a custom tool that can be used to edit files. Well-suited for GPT-5 models /// https://platform.openai.com/docs/guides/function-calling#custom-tools pub(crate) fn create_apply_patch_freeform_tool() -> ToolSpec { diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 99c822fa5..736da3449 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -3,9 +3,6 @@ use codex_protocol::models::ShellCommandToolCallParams; use codex_protocol::models::ShellToolCallParams; use std::sync::Arc; -use crate::apply_patch; -use crate::apply_patch::InternalApplyPatchInvocation; -use crate::apply_patch::convert_apply_patch_to_protocol; use crate::codex::TurnContext; use crate::exec::ExecParams; use crate::exec_env::create_env; @@ -19,11 +16,10 @@ use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; +use crate::tools::handlers::apply_patch::intercept_apply_patch; use crate::tools::orchestrator::ToolOrchestrator; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; -use crate::tools::runtimes::apply_patch::ApplyPatchRequest; -use crate::tools::runtimes::apply_patch::ApplyPatchRuntime; use crate::tools::runtimes::shell::ShellRequest; use crate::tools::runtimes::shell::ShellRuntime; use crate::tools::sandboxing::ToolCtx; @@ -210,81 +206,19 @@ impl ShellHandler { } // Intercept apply_patch if present. - match codex_apply_patch::maybe_parse_apply_patch_verified( + if let Some(output) = intercept_apply_patch( &exec_params.command, &exec_params.cwd, - ) { - codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { - match apply_patch::apply_patch(session.as_ref(), turn.as_ref(), &call_id, changes) - .await - { - InternalApplyPatchInvocation::Output(item) => { - // Programmatic apply_patch path; return its result. - let content = item?; - return Ok(ToolOutput::Function { - content, - content_items: None, - success: Some(true), - }); - } - InternalApplyPatchInvocation::DelegateToExec(apply) => { - let emitter = ToolEmitter::apply_patch( - convert_apply_patch_to_protocol(&apply.action), - !apply.user_explicitly_approved_this_action, - ); - let event_ctx = ToolEventCtx::new( - session.as_ref(), - turn.as_ref(), - &call_id, - Some(&tracker), - ); - emitter.begin(event_ctx).await; - - let req = ApplyPatchRequest { - patch: apply.action.patch.clone(), - cwd: apply.action.cwd.clone(), - timeout_ms: exec_params.expiration.timeout_ms(), - user_explicitly_approved: apply.user_explicitly_approved_this_action, - codex_exe: turn.codex_linux_sandbox_exe.clone(), - }; - let mut orchestrator = ToolOrchestrator::new(); - let mut runtime = ApplyPatchRuntime::new(); - let tool_ctx = ToolCtx { - session: session.as_ref(), - turn: turn.as_ref(), - call_id: call_id.clone(), - tool_name: tool_name.to_string(), - }; - let out = orchestrator - .run(&mut runtime, &req, &tool_ctx, &turn, turn.approval_policy) - .await; - let event_ctx = ToolEventCtx::new( - session.as_ref(), - turn.as_ref(), - &call_id, - Some(&tracker), - ); - let content = emitter.finish(event_ctx, out).await?; - return Ok(ToolOutput::Function { - content, - content_items: None, - success: Some(true), - }); - } - } - } - codex_apply_patch::MaybeApplyPatchVerified::CorrectnessError(parse_error) => { - return Err(FunctionCallError::RespondToModel(format!( - "apply_patch verification failed: {parse_error}" - ))); - } - codex_apply_patch::MaybeApplyPatchVerified::ShellParseError(error) => { - tracing::trace!("Failed to parse shell command, {error:?}"); - // Fall through to regular shell execution. - } - codex_apply_patch::MaybeApplyPatchVerified::NotApplyPatch => { - // Fall through to regular shell execution. - } + exec_params.expiration.timeout_ms(), + session.as_ref(), + turn.as_ref(), + Some(&tracker), + &call_id, + tool_name, + ) + .await? + { + return Ok(output); } let source = ExecCommandSource::Agent; diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 0c4f0a031..344b4afaf 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -13,6 +13,7 @@ use crate::tools::context::ToolPayload; use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; use crate::tools::events::ToolEventStage; +use crate::tools::handlers::apply_patch::intercept_apply_patch; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::unified_exec::ExecCommandRequest; @@ -103,6 +104,7 @@ impl ToolHandler for UnifiedExecHandler { let ToolInvocation { session, turn, + tracker, call_id, tool_name, payload, @@ -153,12 +155,26 @@ impl ToolHandler for UnifiedExecHandler { ))); } - let workdir = workdir - .as_deref() - .filter(|value| !value.is_empty()) - .map(PathBuf::from); + let workdir = workdir.filter(|value| !value.is_empty()); + + let workdir = workdir.map(|dir| context.turn.resolve_path(Some(dir))); let cwd = workdir.clone().unwrap_or_else(|| context.turn.cwd.clone()); + if let Some(output) = intercept_apply_patch( + &command, + &cwd, + Some(yield_time_ms), + context.session.as_ref(), + context.turn.as_ref(), + Some(&tracker), + &context.call_id, + tool_name.as_str(), + ) + .await? + { + return Ok(output); + } + let event_ctx = ToolEventCtx::new( context.session.as_ref(), context.turn.as_ref(), diff --git a/codex-rs/core/src/util.rs b/codex-rs/core/src/util.rs index 0bce5b443..698e02fed 100644 --- a/codex-rs/core/src/util.rs +++ b/codex-rs/core/src/util.rs @@ -1,3 +1,5 @@ +use std::path::Path; +use std::path::PathBuf; use std::time::Duration; use rand::Rng; @@ -37,6 +39,14 @@ pub(crate) fn try_parse_error_message(text: &str) -> String { text.to_string() } +pub fn resolve_path(base: &Path, path: &PathBuf) -> PathBuf { + if path.is_absolute() { + path.clone() + } else { + base.join(path) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 63964b241..584882ce7 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -1,5 +1,7 @@ #![cfg(not(target_os = "windows"))] use std::collections::HashMap; +use std::ffi::OsStr; +use std::fs; use std::sync::OnceLock; use anyhow::Context; @@ -23,6 +25,7 @@ use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; use core_test_support::skip_if_sandbox; use core_test_support::test_codex::TestCodex; +use core_test_support::test_codex::TestCodexHarness; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use core_test_support::wait_for_event_match; @@ -148,6 +151,130 @@ fn collect_tool_outputs(bodies: &[Value]) -> Result Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let builder = test_codex().with_config(|config| { + config.include_apply_patch_tool = true; + config.use_experimental_unified_exec_tool = true; + config.features.enable(Feature::UnifiedExec); + }); + let harness = TestCodexHarness::with_builder(builder).await?; + + let patch = + "*** Begin Patch\n*** Add File: uexec_apply.txt\n+hello from unified exec\n*** End Patch"; + let command = format!("apply_patch <<'EOF'\n{patch}\nEOF\n"); + let call_id = "uexec-apply-patch"; + let args = json!({ + "cmd": command, + "yield_time_ms": 250, + }); + + let responses = vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "exec_command", &serde_json::to_string(&args)?), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ]; + mount_sse_sequence(harness.server(), responses).await; + + let test = harness.test(); + let codex = test.codex.clone(); + let cwd = test.cwd_path().to_path_buf(); + let session_model = test.session_configured.model.clone(); + + codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "apply patch via unified exec".into(), + }], + final_output_json_schema: None, + cwd, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: session_model, + effort: None, + summary: ReasoningSummary::Auto, + }) + .await?; + + let mut saw_patch_begin = false; + let mut patch_end = None; + let mut saw_exec_begin = false; + let mut saw_exec_end = false; + wait_for_event(&codex, |event| match event { + EventMsg::PatchApplyBegin(begin) if begin.call_id == call_id => { + saw_patch_begin = true; + assert!( + begin + .changes + .keys() + .any(|path| path.file_name() == Some(OsStr::new("uexec_apply.txt"))), + "expected apply_patch changes to target uexec_apply.txt", + ); + false + } + EventMsg::PatchApplyEnd(end) if end.call_id == call_id => { + patch_end = Some(end.clone()); + false + } + EventMsg::ExecCommandBegin(event) if event.call_id == call_id => { + saw_exec_begin = true; + false + } + EventMsg::ExecCommandEnd(event) if event.call_id == call_id => { + saw_exec_end = true; + false + } + EventMsg::TaskComplete(_) => true, + _ => false, + }) + .await; + + assert!( + saw_patch_begin, + "expected apply_patch to emit PatchApplyBegin" + ); + let patch_end = patch_end.expect("expected apply_patch to emit PatchApplyEnd"); + assert!( + patch_end.success, + "expected apply_patch to finish successfully: stdout={:?} stderr={:?}", + patch_end.stdout, patch_end.stderr, + ); + assert!( + !saw_exec_begin, + "apply_patch should be intercepted before exec_command begin" + ); + assert!( + !saw_exec_end, + "apply_patch should not emit exec_command end events" + ); + + let output = harness.function_call_stdout(call_id).await; + assert!( + output.contains("Success. Updated the following files:"), + "expected apply_patch output, got: {output:?}" + ); + assert!( + output.contains("A uexec_apply.txt"), + "expected apply_patch file summary, got: {output:?}" + ); + assert_eq!( + fs::read_to_string(harness.path("uexec_apply.txt"))?, + "hello from unified exec\n" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn unified_exec_emits_exec_command_begin_event() -> Result<()> { skip_if_no_network!(Ok(())); @@ -224,6 +351,82 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn unified_exec_resolves_relative_workdir() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = start_mock_server().await; + + let mut builder = test_codex().with_model("gpt-5").with_config(|config| { + config.use_experimental_unified_exec_tool = true; + config.features.enable(Feature::UnifiedExec); + }); + let TestCodex { + codex, + cwd, + session_configured, + .. + } = builder.build(&server).await?; + + let workdir_rel = std::path::PathBuf::from("uexec_relative_workdir"); + std::fs::create_dir_all(cwd.path().join(&workdir_rel))?; + + let call_id = "uexec-workdir-relative"; + let args = json!({ + "cmd": "pwd", + "yield_time_ms": 250, + "workdir": workdir_rel.to_string_lossy().to_string(), + }); + + let responses = vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "exec_command", &serde_json::to_string(&args)?), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "finished"), + ev_completed("resp-2"), + ]), + ]; + mount_sse_sequence(&server, responses).await; + + let session_model = session_configured.model.clone(); + + codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "run relative workdir test".into(), + }], + final_output_json_schema: None, + cwd: cwd.path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: session_model, + effort: None, + summary: ReasoningSummary::Auto, + }) + .await?; + + let begin_event = wait_for_event_match(&codex, |msg| match msg { + EventMsg::ExecCommandBegin(event) if event.call_id == call_id => Some(event.clone()), + _ => None, + }) + .await; + + assert_eq!( + begin_event.cwd, + cwd.path().join(workdir_rel), + "exec_command cwd should resolve relative workdir against turn cwd", + ); + + wait_for_event(&codex, |event| matches!(event, EventMsg::TaskComplete(_))).await; + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[ignore = "flaky"] async fn unified_exec_respects_workdir_override() -> Result<()> {