diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index e58c95657c..9a3aab9a70 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -12,21 +12,14 @@ use codex_utils_path_uri::PathUri; use std::collections::HashMap; use std::path::PathBuf; -#[derive(Debug)] -pub(crate) struct ApplyPatchRuntimeInvocation { - pub(crate) action: ApplyPatchAction, - pub(crate) auto_approved: bool, - pub(crate) exec_approval_requirement: ExecApprovalRequirement, -} - -pub(crate) fn prepare_apply_patch( +pub(crate) fn patch_approval_requirement( step_context: &StepContext, turn_environment: &TurnEnvironment, matching: &PatchPolicyMatcher<'_>, - action: ApplyPatchAction, -) -> Result { + action: &ApplyPatchAction, +) -> Result { match assess_patch_safety( - &action, + action, step_context.settings.approval_policy(), turn_environment.permission_profile(), matching, @@ -34,25 +27,17 @@ pub(crate) fn prepare_apply_patch( .map_err(|error| { FunctionCallError::RespondToModel(format!("failed to check patch permissions: {error}")) })? { - SafetyCheck::AutoApprove => Ok(ApplyPatchRuntimeInvocation { - action, - auto_approved: true, - exec_approval_requirement: ExecApprovalRequirement::Skip { - bypass_sandbox: false, - proposed_execpolicy_amendment: None, - }, + SafetyCheck::AutoApprove => Ok(ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, }), SafetyCheck::AskUser => { // Delegate the approval prompt (including cached approvals) to the // tool runtime, consistent with how shell/unified_exec approvals // are orchestrator-driven. - Ok(ApplyPatchRuntimeInvocation { - action, - auto_approved: false, - exec_approval_requirement: ExecApprovalRequirement::NeedsApproval { - reason: None, - proposed_execpolicy_amendment: None, - }, + Ok(ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, }) } SafetyCheck::Reject { reason } => Err(FunctionCallError::RespondToModel(format!( diff --git a/codex-rs/core/src/apply_patch_tests.rs b/codex-rs/core/src/apply_patch_tests.rs index 8ae27cdc63..818697d89b 100644 --- a/codex-rs/core/src/apply_patch_tests.rs +++ b/codex-rs/core/src/apply_patch_tests.rs @@ -29,7 +29,7 @@ fn convert_apply_patch_maps_add_variant() { } #[tokio::test] -async fn prepare_apply_patch_uses_action_policy_before_turn_policy() { +async fn patch_approval_requirement_uses_action_policy_before_turn_policy() { let (_, mut turn) = make_session_and_context().await; Arc::make_mut(&mut turn.config) .permissions @@ -62,19 +62,18 @@ async fn prepare_apply_patch_uses_action_policy_before_turn_policy() { let sandbox = environment.sandbox_context(/*additional_permissions*/ None); let context = sandbox.policy_context(); - let prepared = prepare_apply_patch( + let requirement = patch_approval_requirement( &step, &environment, &PatchSandboxRoute::Platform(codex_protocol::config_types::WindowsSandboxLevel::Disabled) .prepare_matching(&file_system_policy, &context) .expect("prepare patch matching"), - action, + &action, ) .expect("issuing action policy should request approval"); - assert!(!prepared.auto_approved); assert!(matches!( - prepared.exec_approval_requirement, + requirement, ExecApprovalRequirement::NeedsApproval { .. } )); } diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index cc414484ca..64d6cc196b 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -40,6 +40,7 @@ use crate::tools::registry::ToolArgumentDiffConsumer; use crate::tools::registry::ToolExecutor; use crate::tools::runtimes::apply_patch::ApplyPatchRequest; use crate::tools::runtimes::apply_patch::ApplyPatchRuntime; +use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::ToolCtx; use crate::windows_sandbox::windows_sandbox_level_for_legacy_checks; use codex_apply_patch::ApplyPatchAction; @@ -322,59 +323,17 @@ impl ApplyPatchHandler { "apply_patch handler received unsupported payload".to_string(), )); }; - let args = match codex_apply_patch::parse_patch(&patch_input) { - Ok(args) => args, - Err(parse_error) => { - return Err(FunctionCallError::RespondToModel(format!( - "apply_patch verification failed: {parse_error}" - ))); - } - }; let turn_environment = selected_executor(&environments, &step_context.environments)?; - let fs = turn_environment.environment.get_filesystem(); - let sandbox = turn_environment.sandbox_context(/*additional_permissions*/ None); - match codex_apply_patch::verify_apply_patch_args( - args, - turn_environment.cwd(), - fs.as_ref(), - Some(&sandbox), - ) - .await - { - codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { - let tool_ctx = ToolCtx { - session, - step_context: Arc::clone(&step_context), - cancellation_token, - call_id, - tool_name, - }; - let content = execute_verified_patch( - changes, - 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!( - "apply_patch verification failed: {parse_error}" - ))) - } - codex_apply_patch::MaybeApplyPatchVerified::ShellParseError(error) => { - tracing::trace!("Failed to parse apply_patch input, {error:?}"); - Err(FunctionCallError::RespondToModel( - "apply_patch handler received invalid patch input".to_string(), - )) - } - codex_apply_patch::MaybeApplyPatchVerified::NotApplyPatch => { - Err(FunctionCallError::RespondToModel( - "apply_patch handler received non-apply_patch input".to_string(), - )) - } - } + let request = prepare_patch_request(&patch_input, turn_environment, &step_context).await?; + let tool_ctx = ToolCtx { + session, + step_context, + cancellation_token, + call_id, + tool_name, + }; + let content = execute_prepared_patch(request, Some(&tracker), tool_ctx).await?; + Ok(boxed_tool_output(ApplyPatchToolOutput::from_text(content))) } } @@ -478,6 +437,7 @@ pub(crate) async fn intercept_apply_patch( .await { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { + let request = prepare_verified_patch(changes, turn_environment, &step_context)?; let tool_ctx = ToolCtx { session, step_context, @@ -485,8 +445,7 @@ pub(crate) async fn intercept_apply_patch( call_id: call_id.to_string(), tool_name: ToolName::plain(tool_name), }; - let content = - execute_verified_patch(changes, turn_environment, tracker, tool_ctx).await?; + let content = execute_prepared_patch(request, tracker, tool_ctx).await?; Ok(Some(FunctionToolOutput::from_text(content, Some(true)))) } codex_apply_patch::MaybeApplyPatchVerified::CorrectnessError(parse_error) => { @@ -502,12 +461,56 @@ pub(crate) async fn intercept_apply_patch( } } -async fn execute_verified_patch( +pub(crate) async fn prepare_patch_request( + patch_input: &str, + turn_environment: &TurnEnvironment, + step_context: &StepContext, +) -> Result { + let args = match codex_apply_patch::parse_patch(patch_input) { + Ok(args) => args, + Err(parse_error) => { + return Err(FunctionCallError::RespondToModel(format!( + "apply_patch verification failed: {parse_error}" + ))); + } + }; + let fs = turn_environment.environment.get_filesystem(); + let sandbox = turn_environment.sandbox_context(/*additional_permissions*/ None); + match codex_apply_patch::verify_apply_patch_args( + args, + turn_environment.cwd(), + fs.as_ref(), + Some(&sandbox), + ) + .await + { + codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { + prepare_verified_patch(changes, turn_environment.clone(), step_context) + } + 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:?}"); + Err(FunctionCallError::RespondToModel( + "apply_patch handler received invalid patch input".to_string(), + )) + } + codex_apply_patch::MaybeApplyPatchVerified::NotApplyPatch => { + Err(FunctionCallError::RespondToModel( + "apply_patch handler received non-apply_patch input".to_string(), + )) + } + } +} + +fn prepare_verified_patch( action: ApplyPatchAction, turn_environment: TurnEnvironment, - tracker: Option<&SharedTurnDiffTracker>, - tool_ctx: ToolCtx, -) -> Result { + step_context: &StepContext, +) -> Result { let cwd = action.cwd.clone(); let sandbox_context = turn_environment.sandbox_context(/*additional_permissions*/ None); let policy_context = file_system_sandbox_policy_context_for_cwd(&sandbox_context, &cwd); @@ -521,10 +524,7 @@ async fn execute_verified_patch( }; let environment_id = turn_environment.selection.environment_id.as_str(); let file_paths = file_paths_for_action(&action); - let granted_permissions = tool_ctx - .step_context - .turn - .granted_permissions(environment_id); + let granted_permissions = step_context.turn.granted_permissions(environment_id); let base_file_system_sandbox_policy = turn_environment .permission_profile() .file_system_sandbox_policy(); @@ -544,23 +544,41 @@ async fn execute_verified_patch( FunctionCallError::RespondToModel(format!("failed to check patch permissions: {error}")) })?; let effective_additional_permissions = apply_granted_turn_permissions( - &tool_ctx.step_context, + step_context, &turn_environment, &cwd, crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions, ); - let apply = apply_patch::prepare_apply_patch( - &tool_ctx.step_context, + let exec_approval_requirement = apply_patch::patch_approval_requirement( + step_context, &turn_environment, &matching, - action, + &action, )?; - let changes = convert_apply_patch_to_protocol(&apply.action); + let changes = convert_apply_patch_to_protocol(&action); + Ok(ApplyPatchRequest { + turn_environment, + action, + file_paths, + changes: Arc::new(changes), + exec_approval_requirement, + additional_permissions: effective_additional_permissions.additional_permissions, + permissions_preapproved: effective_additional_permissions.permissions_preapproved, + }) +} +async fn execute_prepared_patch( + request: ApplyPatchRequest, + tracker: Option<&SharedTurnDiffTracker>, + tool_ctx: ToolCtx, +) -> Result { let emitter = ToolEmitter::apply_patch_for_environment( - changes.clone(), - apply.auto_approved, - turn_environment.selection.environment_id.clone(), + request.changes.as_ref().clone(), + matches!( + request.exec_approval_requirement, + ExecApprovalRequirement::Skip { .. } + ), + request.turn_environment.selection.environment_id.clone(), ); let event_ctx = ToolEventCtx::new( tool_ctx.session.as_ref(), @@ -571,15 +589,6 @@ async fn execute_verified_patch( ); emitter.begin(event_ctx).await; - let request = ApplyPatchRequest { - turn_environment, - action: apply.action, - file_paths, - changes: Arc::new(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 diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 981958b17a..bb6dd3e1ca 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -53,6 +53,20 @@ pub struct ApplyPatchRequest { pub permissions_preapproved: bool, } +impl ApplyPatchRequest { + pub(crate) fn approval_action(&self, call_id: &str) -> ApprovalAction { + ApprovalAction::ApplyPatch { + id: call_id.to_string(), + environment_id: self.turn_environment.selection.environment_id.clone(), + cwd: self.action.cwd.clone(), + files: self.file_paths.clone(), + patch: self.action.patch.clone(), + changes: Arc::clone(&self.changes), + permissions_preapproved: self.permissions_preapproved, + } + } +} + #[derive(Default)] pub struct ApplyPatchRuntime { committed_delta: AppliedPatchDelta, @@ -73,18 +87,6 @@ impl ApplyPatchRuntime { &self.committed_delta } - fn build_approval_action(req: &ApplyPatchRequest, call_id: &str) -> ApprovalAction { - ApprovalAction::ApplyPatch { - id: call_id.to_string(), - environment_id: req.turn_environment.selection.environment_id.clone(), - cwd: req.action.cwd.clone(), - files: req.file_paths.clone(), - patch: req.action.patch.clone(), - changes: Arc::clone(&req.changes), - permissions_preapproved: req.permissions_preapproved, - } - } - fn file_system_sandbox_context_for_attempt( req: &ApplyPatchRequest, attempt: &SandboxAttempt<'_>, @@ -130,7 +132,7 @@ impl Approvable for ApplyPatchRuntime { req: &ApplyPatchRequest, call_id: &str, ) -> std::io::Result { - Ok(ApplyPatchRuntime::build_approval_action(req, call_id)) + Ok(req.approval_action(call_id)) } fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool { diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index 8c5d7693a2..d39b0331a5 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -94,7 +94,7 @@ async fn approval_action_preserves_patch_path_uris() { permissions_preapproved: false, }; - let approval_action = ApplyPatchRuntime::build_approval_action(&request, "call-1"); + let approval_action = request.approval_action("call-1"); assert_eq!( approval_action, @@ -131,8 +131,7 @@ async fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() { permissions_preapproved: false, }; - let payload = - ApplyPatchRuntime::build_approval_action(&req, "call-1").permission_request_payload(); + let payload = req.approval_action("call-1").permission_request_payload(); assert_eq!(payload.tool_name.name(), "apply_patch"); assert_eq!(