openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-28 18:58:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

Separate apply_patch request preparation from execution (#57261)

Browse Source 
## What changed

Extract patch parsing, verification, and permission assessment into request preparation helpers used by the tool handler and intercepted shell patches. Execute the prepared `ApplyPatchRequest` through the existing runtime and derive auto-approval status from its approval requirement.

Move approval action construction onto `ApplyPatchRequest` and update the existing approval tests to use the new helpers.

GitOrigin-RevId: 7f07d38f0dbf2a971c94b897281651b7a174fd05
This commit is contained in:
swastik-oai
2026-10-26 21:15:23 +00:00
committed by copyberry
parent 51849772f0
commit bd72161881
5 changed files with 119 additions and 125 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/apply_patch.rs
+10 -25
View File
@@ -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<ApplyPatchRuntimeInvocation, FunctionCallError> {
action: &ApplyPatchAction,
) -> Result<ExecApprovalRequirement, FunctionCallError> {
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!(
codex-rs/core/src/apply_patch_tests.rs
+4 -5
View File
@@ -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 { .. }
));
}
codex-rs/core/src/tools/handlers/apply_patch.rs
+88 -79
View File
@@ -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<ApplyPatchRequest, FunctionCallError> {
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<String, FunctionCallError> {
step_context: &StepContext,
) -> Result<ApplyPatchRequest, FunctionCallError> {
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<String, FunctionCallError> {
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
codex-rs/core/src/tools/runtimes/apply_patch.rs
+15 -13
View File
@@ -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<ApplyPatchRequest> for ApplyPatchRuntime {
req: &ApplyPatchRequest,
call_id: &str,
) -> std::io::Result<ApprovalAction> {
Ok(ApplyPatchRuntime::build_approval_action(req, call_id))
Ok(req.approval_action(call_id))
}
fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool {
codex-rs/core/src/tools/runtimes/apply_patch_tests.rs
+2 -3
View File
@@ -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!(