From 7750465934d97dd3cbcb3b1655d2f622744010d3 Mon Sep 17 00:00:00 2001 From: jif Date: Mon, 3 Aug 2026 11:57:39 +0000 Subject: [PATCH] Consolidate apply_patch runtime execution (#36745) ## What changed - Route verified patches from both direct tool calls and intercepted shell commands through a shared execution helper. - Simplify patch safety preparation to return either a runtime invocation or a rejection directly. - Keep permission resolution, approval handling, event emission, runtime execution, and diff tracking in the common path. GitOrigin-RevId: e16aae8e91d96b8108aefac7f3fbf2da2cbac016 --- codex-rs/core/src/apply_patch.rs | 32 +-- codex-rs/core/src/safety.rs | 20 +- codex-rs/core/src/safety_tests.rs | 5 +- .../core/src/tools/handlers/apply_patch.rs | 247 +++++++----------- 4 files changed, 107 insertions(+), 197 deletions(-) diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 5e9a3f6e5a..fa8cae1a35 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -11,19 +11,6 @@ use codex_utils_path_uri::PathUri; use std::collections::HashMap; use std::path::PathBuf; -pub(crate) enum InternalApplyPatchInvocation { - /// The `apply_patch` call was handled programmatically, without any sort - /// of sandbox, because the user explicitly approved it. This is the - /// result to use with the `shell` function call that contained `apply_patch`. - Output(Result), - - /// The `apply_patch` call was approved, either automatically because it - /// appears that it should be allowed based on the user's sandbox policy - /// *or* because the user explicitly approved it. The runtime realizes the - /// patch through the selected environment filesystem. - DelegateToRuntime(ApplyPatchRuntimeInvocation), -} - #[derive(Debug)] pub(crate) struct ApplyPatchRuntimeInvocation { pub(crate) action: ApplyPatchAction, @@ -31,11 +18,11 @@ pub(crate) struct ApplyPatchRuntimeInvocation { pub(crate) exec_approval_requirement: ExecApprovalRequirement, } -pub(crate) async fn apply_patch( +pub(crate) fn prepare_apply_patch( turn_context: &TurnContext, file_system_sandbox_policy: &FileSystemSandboxPolicy, action: ApplyPatchAction, -) -> InternalApplyPatchInvocation { +) -> Result { match assess_patch_safety( &action, turn_context.approval_policy.value(), @@ -44,12 +31,9 @@ pub(crate) async fn apply_patch( &action.cwd, turn_context.windows_sandbox_level, ) { - SafetyCheck::AutoApprove { - user_explicitly_approved, - .. - } => InternalApplyPatchInvocation::DelegateToRuntime(ApplyPatchRuntimeInvocation { + SafetyCheck::AutoApprove => Ok(ApplyPatchRuntimeInvocation { action, - auto_approved: !user_explicitly_approved, + auto_approved: true, exec_approval_requirement: ExecApprovalRequirement::Skip { bypass_sandbox: false, proposed_execpolicy_amendment: None, @@ -59,7 +43,7 @@ pub(crate) async fn apply_patch( // Delegate the approval prompt (including cached approvals) to the // tool runtime, consistent with how shell/unified_exec approvals // are orchestrator-driven. - InternalApplyPatchInvocation::DelegateToRuntime(ApplyPatchRuntimeInvocation { + Ok(ApplyPatchRuntimeInvocation { action, auto_approved: false, exec_approval_requirement: ExecApprovalRequirement::NeedsApproval { @@ -68,9 +52,9 @@ pub(crate) async fn apply_patch( }, }) } - SafetyCheck::Reject { reason } => InternalApplyPatchInvocation::Output(Err( - FunctionCallError::RespondToModel(format!("patch rejected: {reason}")), - )), + SafetyCheck::Reject { reason } => Err(FunctionCallError::RespondToModel(format!( + "patch rejected: {reason}" + ))), } } diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index 1fe9b0257b..2e5441a078 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -8,7 +8,6 @@ use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::AskForApproval; -use codex_sandboxing::SandboxType; use codex_sandboxing::get_platform_sandbox; use codex_utils_path_uri::PathUri; @@ -19,14 +18,9 @@ const PATCH_REJECTED_READ_ONLY_REASON: &str = #[derive(Debug, PartialEq)] pub enum SafetyCheck { - AutoApprove { - sandbox_type: SandboxType, - user_explicitly_approved: bool, - }, + AutoApprove, AskUser, - Reject { - reason: String, - }, + Reject { reason: String }, } pub fn assess_patch_safety( @@ -70,19 +64,13 @@ pub fn assess_patch_safety( ) { // Disabled and External profiles intentionally do not apply an // outer Codex filesystem sandbox. - SafetyCheck::AutoApprove { - sandbox_type: SandboxType::None, - user_explicitly_approved: false, - } + SafetyCheck::AutoApprove } else { // Only auto‑approve when we can actually enforce a sandbox. Otherwise // fall back to asking the user because the patch may touch arbitrary // paths outside the project. match get_platform_sandbox(windows_sandbox_level != WindowsSandboxLevel::Disabled) { - Some(sandbox_type) => SafetyCheck::AutoApprove { - sandbox_type, - user_explicitly_approved: false, - }, + Some(_) => SafetyCheck::AutoApprove, None => { if rejects_sandbox_approval { SafetyCheck::Reject { diff --git a/codex-rs/core/src/safety_tests.rs b/codex-rs/core/src/safety_tests.rs index a8d5821890..29afacde0c 100644 --- a/codex-rs/core/src/safety_tests.rs +++ b/codex-rs/core/src/safety_tests.rs @@ -88,10 +88,7 @@ fn external_sandbox_auto_approves_in_on_request() { &cwd_uri, WindowsSandboxLevel::Disabled ), - SafetyCheck::AutoApprove { - sandbox_type: SandboxType::None, - user_explicitly_approved: false, - } + SafetyCheck::AutoApprove ); } diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 0950174ba4..0cb197c442 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -7,7 +7,6 @@ use std::time::Duration; use std::time::Instant; use crate::apply_patch; -use crate::apply_patch::InternalApplyPatchInvocation; use crate::apply_patch::convert_apply_patch_to_protocol; use crate::function_tool::FunctionCallError; use crate::session::session::Session; @@ -394,82 +393,21 @@ impl ApplyPatchHandler { .await { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { - let (file_paths, effective_additional_permissions, file_system_sandbox_policy) = - effective_patch_permissions( - session.as_ref(), - turn.as_ref(), - &turn_environment.environment_id, - &changes, - turn_environment.cwd(), - ) - .await - .unwrap_or_else(|_| patch_permissions_without_path_matching(&changes)); - match apply_patch::apply_patch(turn.as_ref(), &file_system_sandbox_policy, changes) - .await - { - InternalApplyPatchInvocation::Output(item) => { - let content = item?; - Ok(boxed_tool_output(ApplyPatchToolOutput::from_text(content))) - } - InternalApplyPatchInvocation::DelegateToRuntime(apply) => { - let changes = convert_apply_patch_to_protocol(&apply.action); - let emitter = ToolEmitter::apply_patch_for_environment( - changes.clone(), - apply.auto_approved, - turn_environment.environment_id.clone(), - ); - let event_ctx = ToolEventCtx::new( - session.as_ref(), - turn.as_ref(), - &call_id, - Some(&tracker), - ); - emitter.begin(event_ctx).await; - - let req = ApplyPatchRequest { - turn_environment: turn_environment.clone(), - action: apply.action, - file_paths, - changes, - exec_approval_requirement: apply.exec_approval_requirement, - additional_permissions: effective_additional_permissions - .additional_permissions, - permissions_preapproved: effective_additional_permissions - .permissions_preapproved, - }; - - let mut orchestrator = ToolOrchestrator::new(); - let mut runtime = ApplyPatchRuntime::new(); - let tool_ctx = ToolCtx { - session: session.clone(), - turn: turn.clone(), - call_id: call_id.clone(), - tool_name: tool_name.clone(), - }; - let out = orchestrator - .run( - &mut runtime, - &req, - &tool_ctx, - turn.as_ref(), - turn.approval_policy.value(), - ) - .await - .map(|result| result.output); - let (out, delta) = match out { - Ok(output) => (Ok(output.exec_output), Some(output.delta)), - Err(error) => (Err(error), Some(runtime.committed_delta().clone())), - }; - let event_ctx = ToolEventCtx::new( - session.as_ref(), - turn.as_ref(), - &call_id, - Some(&tracker), - ); - let content = emitter.finish(event_ctx, out, delta.as_ref()).await?; - Ok(boxed_tool_output(ApplyPatchToolOutput::from_text(content))) - } - } + let tool_ctx = ToolCtx { + session, + turn, + call_id, + tool_name, + }; + let content = execute_verified_patch( + changes, + turn_environment.cwd(), + turn_environment.clone(), + Some(&tracker), + tool_ctx, + ) + .await?; + Ok(boxed_tool_output(ApplyPatchToolOutput::from_text(content))) } codex_apply_patch::MaybeApplyPatchVerified::CorrectnessError(parse_error) => { Err(FunctionCallError::RespondToModel(format!( @@ -558,82 +496,15 @@ pub(crate) async fn intercept_apply_patch( .await { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { - let (approval_keys, effective_additional_permissions, file_system_sandbox_policy) = - effective_patch_permissions( - session.as_ref(), - turn.as_ref(), - &turn_environment.environment_id, - &changes, - cwd, - ) - .await - .unwrap_or_else(|_| patch_permissions_without_path_matching(&changes)); - match apply_patch::apply_patch(turn.as_ref(), &file_system_sandbox_policy, changes) - .await - { - InternalApplyPatchInvocation::Output(item) => { - let content = item?; - Ok(Some(FunctionToolOutput::from_text(content, Some(true)))) - } - InternalApplyPatchInvocation::DelegateToRuntime(apply) => { - let changes = convert_apply_patch_to_protocol(&apply.action); - let emitter = ToolEmitter::apply_patch_for_environment( - changes.clone(), - apply.auto_approved, - turn_environment.environment_id.clone(), - ); - let event_ctx = ToolEventCtx::new( - session.as_ref(), - turn.as_ref(), - call_id, - tracker.as_ref().copied(), - ); - emitter.begin(event_ctx).await; - - let req = ApplyPatchRequest { - turn_environment, - action: apply.action, - file_paths: approval_keys, - changes, - exec_approval_requirement: apply.exec_approval_requirement, - additional_permissions: effective_additional_permissions - .additional_permissions, - permissions_preapproved: effective_additional_permissions - .permissions_preapproved, - }; - - let mut orchestrator = ToolOrchestrator::new(); - let mut runtime = ApplyPatchRuntime::new(); - let tool_ctx = ToolCtx { - session: session.clone(), - turn: turn.clone(), - call_id: call_id.to_string(), - tool_name: ToolName::plain(tool_name), - }; - let out = orchestrator - .run( - &mut runtime, - &req, - &tool_ctx, - turn.as_ref(), - turn.approval_policy.value(), - ) - .await - .map(|result| result.output); - let (out, delta) = match out { - Ok(output) => (Ok(output.exec_output), Some(output.delta)), - Err(error) => (Err(error), Some(runtime.committed_delta().clone())), - }; - let event_ctx = ToolEventCtx::new( - session.as_ref(), - turn.as_ref(), - call_id, - tracker.as_ref().copied(), - ); - let content = emitter.finish(event_ctx, out, delta.as_ref()).await?; - Ok(Some(FunctionToolOutput::from_text(content, Some(true)))) - } - } + let tool_ctx = ToolCtx { + session, + turn, + call_id: call_id.to_string(), + tool_name: ToolName::plain(tool_name), + }; + let content = + execute_verified_patch(changes, cwd, turn_environment, tracker, tool_ctx).await?; + Ok(Some(FunctionToolOutput::from_text(content, Some(true)))) } codex_apply_patch::MaybeApplyPatchVerified::CorrectnessError(parse_error) => { Err(FunctionCallError::RespondToModel(format!( @@ -648,6 +519,76 @@ pub(crate) async fn intercept_apply_patch( } } +async fn execute_verified_patch( + action: ApplyPatchAction, + cwd: &PathUri, + turn_environment: TurnEnvironment, + tracker: Option<&SharedTurnDiffTracker>, + tool_ctx: ToolCtx, +) -> Result { + let (file_paths, effective_additional_permissions, file_system_sandbox_policy) = + effective_patch_permissions( + tool_ctx.session.as_ref(), + tool_ctx.turn.as_ref(), + &turn_environment.environment_id, + &action, + cwd, + ) + .await + .unwrap_or_else(|_| patch_permissions_without_path_matching(&action)); + let apply = apply_patch::prepare_apply_patch( + tool_ctx.turn.as_ref(), + &file_system_sandbox_policy, + action, + )?; + let changes = convert_apply_patch_to_protocol(&apply.action); + let emitter = ToolEmitter::apply_patch_for_environment( + changes.clone(), + apply.auto_approved, + turn_environment.environment_id.clone(), + ); + let event_ctx = ToolEventCtx::new( + tool_ctx.session.as_ref(), + tool_ctx.turn.as_ref(), + &tool_ctx.call_id, + tracker, + ); + emitter.begin(event_ctx).await; + + let request = ApplyPatchRequest { + turn_environment, + action: apply.action, + file_paths, + changes, + exec_approval_requirement: apply.exec_approval_requirement, + additional_permissions: effective_additional_permissions.additional_permissions, + permissions_preapproved: effective_additional_permissions.permissions_preapproved, + }; + let mut orchestrator = ToolOrchestrator::new(); + let mut runtime = ApplyPatchRuntime::new(); + let result = orchestrator + .run( + &mut runtime, + &request, + &tool_ctx, + tool_ctx.turn.as_ref(), + tool_ctx.turn.approval_policy.value(), + ) + .await + .map(|result| result.output); + let (result, delta) = match result { + Ok(output) => (Ok(output.exec_output), Some(output.delta)), + Err(error) => (Err(error), Some(runtime.committed_delta().clone())), + }; + let event_ctx = ToolEventCtx::new( + tool_ctx.session.as_ref(), + tool_ctx.turn.as_ref(), + &tool_ctx.call_id, + tracker, + ); + emitter.finish(event_ctx, result, delta.as_ref()).await +} + fn require_environment_id( parsed_environment_id: Option<&str>, allow_environment_id: bool,