diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index dedf51ea1bb..d0fd9b2bd8e 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -487,14 +487,33 @@ pub struct ConfigEdit { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] -pub enum ApprovalDecision { +pub enum CommandExecutionApprovalDecision { + /// User approved the command. Accept, - /// Approve and remember the approval for the session. + /// User approved the command and future identical commands should run without prompting. AcceptForSession, + /// User approved the command, and wants to apply the proposed execpolicy amendment so future + /// matching commands can run without prompting. AcceptWithExecpolicyAmendment { execpolicy_amendment: ExecPolicyAmendment, }, + /// User denied the command. The agent will continue the turn. Decline, + /// User denied the command. The turn will also be immediately interrupted. + Cancel, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub enum FileChangeApprovalDecision { + /// User approved the file changes. + Accept, + /// User approved the file changes and future changes to the same files should run without prompting. + AcceptForSession, + /// User denied the file changes. The agent will continue the turn. + Decline, + /// User denied the file changes. The turn will also be immediately interrupted. Cancel, } @@ -1884,7 +1903,7 @@ pub struct CommandExecutionRequestApprovalParams { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct CommandExecutionRequestApprovalResponse { - pub decision: ApprovalDecision, + pub decision: CommandExecutionApprovalDecision, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -1904,7 +1923,7 @@ pub struct FileChangeRequestApprovalParams { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[ts(export_to = "v2/")] pub struct FileChangeRequestApprovalResponse { - pub decision: ApprovalDecision, + pub decision: FileChangeApprovalDecision, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server-test-client/src/main.rs b/codex-rs/app-server-test-client/src/main.rs index eefd1231828..70d91fab068 100644 --- a/codex-rs/app-server-test-client/src/main.rs +++ b/codex-rs/app-server-test-client/src/main.rs @@ -17,12 +17,13 @@ use clap::Parser; use clap::Subcommand; use codex_app_server_protocol::AddConversationListenerParams; use codex_app_server_protocol::AddConversationSubscriptionResponse; -use codex_app_server_protocol::ApprovalDecision; use codex_app_server_protocol::AskForApproval; use codex_app_server_protocol::ClientInfo; use codex_app_server_protocol::ClientRequest; +use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::CommandExecutionRequestApprovalParams; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; +use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::FileChangeRequestApprovalParams; use codex_app_server_protocol::FileChangeRequestApprovalResponse; use codex_app_server_protocol::GetAccountRateLimitsResponse; @@ -798,7 +799,7 @@ impl CodexClient { } let response = CommandExecutionRequestApprovalResponse { - decision: ApprovalDecision::Accept, + decision: CommandExecutionApprovalDecision::Accept, }; self.send_server_request_response(request_id, &response)?; println!("< approved commandExecution request for item {item_id}"); @@ -829,7 +830,7 @@ impl CodexClient { } let response = FileChangeRequestApprovalResponse { - decision: ApprovalDecision::Accept, + decision: FileChangeApprovalDecision::Accept, }; self.send_server_request_response(request_id, &response)?; println!("< approved fileChange request for item {item_id}"); diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index c9b78fe8c88..b540f5b2de9 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -13,9 +13,9 @@ use codex_app_server_protocol::AccountRateLimitsUpdatedNotification; use codex_app_server_protocol::AgentMessageDeltaNotification; use codex_app_server_protocol::ApplyPatchApprovalParams; use codex_app_server_protocol::ApplyPatchApprovalResponse; -use codex_app_server_protocol::ApprovalDecision; use codex_app_server_protocol::CodexErrorInfo as V2CodexErrorInfo; use codex_app_server_protocol::CommandAction as V2ParsedCommand; +use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::CommandExecutionOutputDeltaNotification; use codex_app_server_protocol::CommandExecutionRequestApprovalParams; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; @@ -26,6 +26,7 @@ use codex_app_server_protocol::ErrorNotification; use codex_app_server_protocol::ExecCommandApprovalParams; use codex_app_server_protocol::ExecCommandApprovalResponse; use codex_app_server_protocol::ExecPolicyAmendment as V2ExecPolicyAmendment; +use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::FileChangeOutputDeltaNotification; use codex_app_server_protocol::FileChangeRequestApprovalParams; use codex_app_server_protocol::FileChangeRequestApprovalResponse; @@ -1193,6 +1194,21 @@ fn format_file_change_diff(change: &CoreFileChange) -> String { } } +fn map_file_change_approval_decision( + decision: FileChangeApprovalDecision, +) -> (ReviewDecision, Option) { + match decision { + FileChangeApprovalDecision::Accept => (ReviewDecision::Approved, None), + FileChangeApprovalDecision::AcceptForSession => (ReviewDecision::ApprovedForSession, None), + FileChangeApprovalDecision::Decline => { + (ReviewDecision::Denied, Some(PatchApplyStatus::Declined)) + } + FileChangeApprovalDecision::Cancel => { + (ReviewDecision::Abort, Some(PatchApplyStatus::Declined)) + } + } +} + #[allow(clippy::too_many_arguments)] async fn on_file_change_request_approval_response( event_turn_id: String, @@ -1211,23 +1227,12 @@ async fn on_file_change_request_approval_response( .unwrap_or_else(|err| { error!("failed to deserialize FileChangeRequestApprovalResponse: {err}"); FileChangeRequestApprovalResponse { - decision: ApprovalDecision::Decline, + decision: FileChangeApprovalDecision::Decline, } }); - let (decision, completion_status) = match response.decision { - ApprovalDecision::Accept - | ApprovalDecision::AcceptForSession - | ApprovalDecision::AcceptWithExecpolicyAmendment { .. } => { - (ReviewDecision::Approved, None) - } - ApprovalDecision::Decline => { - (ReviewDecision::Denied, Some(PatchApplyStatus::Declined)) - } - ApprovalDecision::Cancel => { - (ReviewDecision::Abort, Some(PatchApplyStatus::Declined)) - } - }; + let (decision, completion_status) = + map_file_change_approval_decision(response.decision); // Allow EventMsg::PatchApplyEnd to emit ItemCompleted for accepted patches. // Only short-circuit on declines/cancels/failures. (decision, completion_status) @@ -1281,16 +1286,18 @@ async fn on_command_execution_request_approval_response( .unwrap_or_else(|err| { error!("failed to deserialize CommandExecutionRequestApprovalResponse: {err}"); CommandExecutionRequestApprovalResponse { - decision: ApprovalDecision::Decline, + decision: CommandExecutionApprovalDecision::Decline, } }); let decision = response.decision; let (decision, completion_status) = match decision { - ApprovalDecision::Accept => (ReviewDecision::Approved, None), - ApprovalDecision::AcceptForSession => (ReviewDecision::ApprovedForSession, None), - ApprovalDecision::AcceptWithExecpolicyAmendment { + CommandExecutionApprovalDecision::Accept => (ReviewDecision::Approved, None), + CommandExecutionApprovalDecision::AcceptForSession => { + (ReviewDecision::ApprovedForSession, None) + } + CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment { execpolicy_amendment, } => ( ReviewDecision::ApprovedExecpolicyAmendment { @@ -1298,11 +1305,11 @@ async fn on_command_execution_request_approval_response( }, None, ), - ApprovalDecision::Decline => ( + CommandExecutionApprovalDecision::Decline => ( ReviewDecision::Denied, Some(CommandExecutionStatus::Declined), ), - ApprovalDecision::Cancel => ( + CommandExecutionApprovalDecision::Cancel => ( ReviewDecision::Abort, Some(CommandExecutionStatus::Declined), ), @@ -1442,6 +1449,14 @@ mod tests { Arc::new(Mutex::new(HashMap::new())) } + #[test] + fn file_change_accept_for_session_maps_to_approved_for_session() { + let (decision, completion_status) = + map_file_change_approval_decision(FileChangeApprovalDecision::AcceptForSession); + assert_eq!(decision, ReviewDecision::ApprovedForSession); + assert_eq!(completion_status, None); + } + #[tokio::test] async fn test_handle_error_records_message() -> Result<()> { let conversation_id = ConversationId::new(); diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index 60fa656b197..ab450ea832c 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -8,9 +8,10 @@ use app_test_support::create_mock_chat_completions_server_unchecked; use app_test_support::create_shell_command_sse_response; use app_test_support::format_with_current_shell_display; use app_test_support::to_response; -use codex_app_server_protocol::ApprovalDecision; +use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::CommandExecutionStatus; +use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::FileChangeOutputDeltaNotification; use codex_app_server_protocol::FileChangeRequestApprovalResponse; use codex_app_server_protocol::ItemCompletedNotification; @@ -426,7 +427,7 @@ async fn turn_start_exec_approval_decline_v2() -> Result<()> { mcp.send_response( request_id, serde_json::to_value(CommandExecutionRequestApprovalResponse { - decision: ApprovalDecision::Decline, + decision: CommandExecutionApprovalDecision::Decline, })?, ) .await?; @@ -722,7 +723,7 @@ async fn turn_start_file_change_approval_v2() -> Result<()> { mcp.send_response( request_id, serde_json::to_value(FileChangeRequestApprovalResponse { - decision: ApprovalDecision::Accept, + decision: FileChangeApprovalDecision::Accept, })?, ) .await?; @@ -782,6 +783,190 @@ async fn turn_start_file_change_approval_v2() -> Result<()> { Ok(()) } +#[tokio::test] +async fn turn_start_file_change_approval_accept_for_session_persists_v2() -> Result<()> { + skip_if_no_network!(Ok(())); + + let tmp = TempDir::new()?; + let codex_home = tmp.path().join("codex_home"); + std::fs::create_dir(&codex_home)?; + let workspace = tmp.path().join("workspace"); + std::fs::create_dir(&workspace)?; + + let patch_1 = r#"*** Begin Patch +*** Add File: README.md ++new line +*** End Patch +"#; + let patch_2 = r#"*** Begin Patch +*** Update File: README.md +@@ +-new line ++updated line +*** End Patch +"#; + + let responses = vec![ + create_apply_patch_sse_response(patch_1, "patch-call-1")?, + create_final_assistant_message_sse_response("patch 1 applied")?, + create_apply_patch_sse_response(patch_2, "patch-call-2")?, + create_final_assistant_message_sse_response("patch 2 applied")?, + ]; + let server = create_mock_chat_completions_server(responses).await; + create_config_toml(&codex_home, &server.uri(), "untrusted")?; + + let mut mcp = McpProcess::new(&codex_home).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let start_req = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + cwd: Some(workspace.to_string_lossy().into_owned()), + ..Default::default() + }) + .await?; + let start_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(start_req)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + + // First turn: expect FileChangeRequestApproval, respond with AcceptForSession, and verify the file exists. + let turn_1_req = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "apply patch 1".into(), + }], + cwd: Some(workspace.clone()), + ..Default::default() + }) + .await?; + let turn_1_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_1_req)), + ) + .await??; + let TurnStartResponse { turn: turn_1 } = to_response::(turn_1_resp)?; + + let started_file_change_1 = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let started_notif = mcp + .read_stream_until_notification_message("item/started") + .await?; + let started: ItemStartedNotification = + serde_json::from_value(started_notif.params.clone().expect("item/started params"))?; + if let ThreadItem::FileChange { .. } = started.item { + return Ok::(started.item); + } + } + }) + .await??; + let ThreadItem::FileChange { id, status, .. } = started_file_change_1 else { + unreachable!("loop ensures we break on file change items"); + }; + assert_eq!(id, "patch-call-1"); + assert_eq!(status, PatchApplyStatus::InProgress); + + let server_req = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_request_message(), + ) + .await??; + let ServerRequest::FileChangeRequestApproval { request_id, params } = server_req else { + panic!("expected FileChangeRequestApproval request") + }; + assert_eq!(params.item_id, "patch-call-1"); + assert_eq!(params.thread_id, thread.id); + assert_eq!(params.turn_id, turn_1.id); + + mcp.send_response( + request_id, + serde_json::to_value(FileChangeRequestApprovalResponse { + decision: FileChangeApprovalDecision::AcceptForSession, + })?, + ) + .await?; + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/fileChange/outputDelta"), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/completed"), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await??; + + let readme_path = workspace.join("README.md"); + assert_eq!(std::fs::read_to_string(&readme_path)?, "new line\n"); + + // Second turn: apply a patch to the same file. Approval should be skipped due to AcceptForSession. + let turn_2_req = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "apply patch 2".into(), + }], + cwd: Some(workspace.clone()), + ..Default::default() + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_2_req)), + ) + .await??; + + let started_file_change_2 = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let started_notif = mcp + .read_stream_until_notification_message("item/started") + .await?; + let started: ItemStartedNotification = + serde_json::from_value(started_notif.params.clone().expect("item/started params"))?; + if let ThreadItem::FileChange { .. } = started.item { + return Ok::(started.item); + } + } + }) + .await??; + let ThreadItem::FileChange { id, status, .. } = started_file_change_2 else { + unreachable!("loop ensures we break on file change items"); + }; + assert_eq!(id, "patch-call-2"); + assert_eq!(status, PatchApplyStatus::InProgress); + + // If the server incorrectly emits FileChangeRequestApproval, the helper below will error + // (it bails on unexpected JSONRPCMessage::Request), causing the test to fail. + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/fileChange/outputDelta"), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/completed"), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await??; + + assert_eq!(std::fs::read_to_string(readme_path)?, "updated line\n"); + + Ok(()) +} + #[tokio::test] async fn turn_start_file_change_approval_decline_v2() -> Result<()> { skip_if_no_network!(Ok(())); @@ -888,7 +1073,7 @@ async fn turn_start_file_change_approval_decline_v2() -> Result<()> { mcp.send_response( request_id, serde_json::to_value(FileChangeRequestApprovalResponse { - decision: ApprovalDecision::Decline, + decision: FileChangeApprovalDecision::Decline, })?, ) .await?; diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 67433303e5b..1a47ca60b7d 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -1,10 +1,9 @@ -use crate::codex::Session; use crate::codex::TurnContext; use crate::function_tool::FunctionCallError; use crate::protocol::FileChange; -use crate::protocol::ReviewDecision; use crate::safety::SafetyCheck; use crate::safety::assess_patch_safety; +use crate::tools::sandboxing::ExecApprovalRequirement; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use std::collections::HashMap; @@ -30,13 +29,12 @@ pub(crate) enum InternalApplyPatchInvocation { #[derive(Debug)] pub(crate) struct ApplyPatchExec { pub(crate) action: ApplyPatchAction, - pub(crate) user_explicitly_approved_this_action: bool, + pub(crate) auto_approved: bool, + pub(crate) exec_approval_requirement: ExecApprovalRequirement, } pub(crate) async fn apply_patch( - sess: &Session, turn_context: &TurnContext, - call_id: &str, action: ApplyPatchAction, ) -> InternalApplyPatchInvocation { match assess_patch_safety( @@ -50,40 +48,24 @@ pub(crate) async fn apply_patch( .. } => InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { action, - user_explicitly_approved_this_action: user_explicitly_approved, + auto_approved: !user_explicitly_approved, + exec_approval_requirement: ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, }), SafetyCheck::AskUser => { - // Compute a readable summary of path changes to include in the - // approval request so the user can make an informed decision. - // - // Note that it might be worth expanding this approval request to - // give the user the option to expand the set of writable roots so - // that similar patches can be auto-approved in the future during - // this session. - let rx_approve = sess - .request_patch_approval( - turn_context, - call_id.to_owned(), - convert_apply_patch_to_protocol(&action), - None, - None, - ) - .await; - match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::ApprovedForSession => { - InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { - action, - user_explicitly_approved_this_action: true, - }) - } - ReviewDecision::Denied | ReviewDecision::Abort => { - InternalApplyPatchInvocation::Output(Err(FunctionCallError::RespondToModel( - "patch rejected by user".to_string(), - ))) - } - } + // Delegate the approval prompt (including cached approvals) to the + // tool runtime, consistent with how shell/unified_exec approvals + // are orchestrator-driven. + InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { + action, + auto_approved: false, + exec_approval_requirement: ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + }) } SafetyCheck::Reject { reason } => InternalApplyPatchInvocation::Output(Err( FunctionCallError::RespondToModel(format!("patch rejected: {reason}")), diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index c3930b4f428..601a5a8b81e 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -277,7 +277,7 @@ mod tests { }; assert_eq!( - assess_patch_safety(&add_inside, AskForApproval::OnRequest, &policy, &cwd,), + assess_patch_safety(&add_inside, AskForApproval::OnRequest, &policy, &cwd), SafetyCheck::AutoApprove { sandbox_type: SandboxType::None, user_explicitly_approved: false, diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index cdfc575cd9b..02aa50d7b61 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -305,7 +305,12 @@ impl ToolEmitter { // Normalize common rejection messages for exec tools so tests and // users see a clear, consistent phrase. let normalized = if msg == "rejected by user" { - "exec command rejected by user".to_string() + match self { + Self::Shell { .. } | Self::UnifiedExec { .. } => { + "exec command rejected by user".to_string() + } + Self::ApplyPatch { .. } => "patch rejected by user".to_string(), + } } else { msg }; diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 14a481f4eae..adf76e68251 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -26,11 +26,38 @@ use crate::tools::sandboxing::ToolCtx; use crate::tools::spec::ApplyPatchToolArgs; use crate::tools::spec::JsonSchema; use async_trait::async_trait; +use codex_apply_patch::ApplyPatchAction; +use codex_apply_patch::ApplyPatchFileChange; +use codex_utils_absolute_path::AbsolutePathBuf; pub struct ApplyPatchHandler; const APPLY_PATCH_LARK_GRAMMAR: &str = include_str!("tool_apply_patch.lark"); +fn file_paths_for_action(action: &ApplyPatchAction) -> Vec { + let mut keys = Vec::new(); + let cwd = action.cwd.as_path(); + + for (path, change) in action.changes() { + if let Some(key) = to_abs_path(cwd, path) { + keys.push(key); + } + + if let ApplyPatchFileChange::Update { move_path, .. } = change + && let Some(dest) = move_path + && let Some(key) = to_abs_path(cwd, dest) + { + keys.push(key); + } + } + + keys +} + +fn to_abs_path(cwd: &Path, path: &Path) -> Option { + AbsolutePathBuf::resolve_path_against_base(path, cwd).ok() +} + #[async_trait] impl ToolHandler for ApplyPatchHandler { fn kind(&self) -> ToolKind { @@ -81,9 +108,7 @@ impl ToolHandler for ApplyPatchHandler { let command = vec!["apply_patch".to_string(), patch_input.clone()]; match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd) { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { - match apply_patch::apply_patch(session.as_ref(), turn.as_ref(), &call_id, changes) - .await - { + match apply_patch::apply_patch(turn.as_ref(), changes).await { InternalApplyPatchInvocation::Output(item) => { let content = item?; Ok(ToolOutput::Function { @@ -93,10 +118,10 @@ impl ToolHandler for ApplyPatchHandler { }) } InternalApplyPatchInvocation::DelegateToExec(apply) => { - let emitter = ToolEmitter::apply_patch( - convert_apply_patch_to_protocol(&apply.action), - !apply.user_explicitly_approved_this_action, - ); + let changes = convert_apply_patch_to_protocol(&apply.action); + let file_paths = file_paths_for_action(&apply.action); + let emitter = + ToolEmitter::apply_patch(changes.clone(), apply.auto_approved); let event_ctx = ToolEventCtx::new( session.as_ref(), turn.as_ref(), @@ -106,10 +131,11 @@ impl ToolHandler for ApplyPatchHandler { emitter.begin(event_ctx).await; let req = ApplyPatchRequest { - patch: apply.action.patch.clone(), - cwd: apply.action.cwd.clone(), + action: apply.action, + file_paths, + changes, + exec_approval_requirement: apply.exec_approval_requirement, timeout_ms: None, - user_explicitly_approved: apply.user_explicitly_approved_this_action, codex_exe: turn.codex_linux_sandbox_exe.clone(), }; @@ -178,7 +204,7 @@ pub(crate) async fn intercept_apply_patch( turn, ) .await; - match apply_patch::apply_patch(session, turn, call_id, changes).await { + match apply_patch::apply_patch(turn, changes).await { InternalApplyPatchInvocation::Output(item) => { let content = item?; Ok(Some(ToolOutput::Function { @@ -188,19 +214,19 @@ pub(crate) async fn intercept_apply_patch( })) } InternalApplyPatchInvocation::DelegateToExec(apply) => { - let emitter = ToolEmitter::apply_patch( - convert_apply_patch_to_protocol(&apply.action), - !apply.user_explicitly_approved_this_action, - ); + let changes = convert_apply_patch_to_protocol(&apply.action); + let approval_keys = file_paths_for_action(&apply.action); + let emitter = ToolEmitter::apply_patch(changes.clone(), apply.auto_approved); 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(), + action: apply.action, + file_paths: approval_keys, + changes, + exec_approval_requirement: apply.exec_approval_requirement, timeout_ms, - user_explicitly_approved: apply.user_explicitly_approved_this_action, codex_exe: turn.codex_linux_sandbox_exe.clone(), }; @@ -342,3 +368,35 @@ It is important to remember: }, }) } + +#[cfg(test)] +mod tests { + use super::*; + use codex_apply_patch::MaybeApplyPatchVerified; + use pretty_assertions::assert_eq; + use tempfile::TempDir; + + #[test] + fn approval_keys_include_move_destination() { + let tmp = TempDir::new().expect("tmp"); + let cwd = tmp.path(); + std::fs::create_dir_all(cwd.join("old")).expect("create old dir"); + std::fs::create_dir_all(cwd.join("renamed/dir")).expect("create dest dir"); + std::fs::write(cwd.join("old/name.txt"), "old content\n").expect("write old file"); + let patch = r#"*** Begin Patch +*** Update File: old/name.txt +*** Move to: renamed/dir/name.txt +@@ +-old content ++new content +*** End Patch"#; + let argv = vec!["apply_patch".to_string(), patch.to_string()]; + let action = match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, cwd) { + MaybeApplyPatchVerified::Body(action) => action, + other => panic!("expected patch body, got: {other:?}"), + }; + + let keys = file_paths_for_action(&action); + assert_eq!(keys.len(), 2); + } +} diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 26d04f578c5..7b9d0dccc5d 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -11,6 +11,7 @@ use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; +use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::Sandboxable; use crate::tools::sandboxing::SandboxablePreference; @@ -18,30 +19,28 @@ use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::with_cached_approval; +use codex_apply_patch::ApplyPatchAction; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::FileChange; use codex_protocol::protocol::ReviewDecision; +use codex_utils_absolute_path::AbsolutePathBuf; use futures::future::BoxFuture; use std::collections::HashMap; use std::path::PathBuf; -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct ApplyPatchRequest { - pub patch: String, - pub cwd: PathBuf, + pub action: ApplyPatchAction, + pub file_paths: Vec, + pub changes: std::collections::HashMap, + pub exec_approval_requirement: ExecApprovalRequirement, pub timeout_ms: Option, - pub user_explicitly_approved: bool, pub codex_exe: Option, } #[derive(Default)] pub struct ApplyPatchRuntime; -#[derive(serde::Serialize, Clone, Debug, Eq, PartialEq, Hash)] -pub(crate) struct ApprovalKey { - patch: String, - cwd: PathBuf, -} - impl ApplyPatchRuntime { pub fn new() -> Self { Self @@ -58,8 +57,8 @@ impl ApplyPatchRuntime { let program = exe.to_string_lossy().to_string(); Ok(CommandSpec { program, - args: vec![CODEX_APPLY_PATCH_ARG1.to_string(), req.patch.clone()], - cwd: req.cwd.clone(), + args: vec![CODEX_APPLY_PATCH_ARG1.to_string(), req.action.patch.clone()], + cwd: req.action.cwd.clone(), expiration: req.timeout_ms.into(), // Run apply_patch with a minimal environment for determinism and to avoid leaks. env: HashMap::new(), @@ -87,13 +86,10 @@ impl Sandboxable for ApplyPatchRuntime { } impl Approvable for ApplyPatchRuntime { - type ApprovalKey = ApprovalKey; + type ApprovalKey = AbsolutePathBuf; - fn approval_key(&self, req: &ApplyPatchRequest) -> Self::ApprovalKey { - ApprovalKey { - patch: req.patch.clone(), - cwd: req.cwd.clone(), - } + fn approval_keys(&self, req: &ApplyPatchRequest) -> Vec { + req.file_paths.clone() } fn start_approval_async<'a>( @@ -101,31 +97,25 @@ impl Approvable for ApplyPatchRuntime { req: &'a ApplyPatchRequest, ctx: ApprovalCtx<'a>, ) -> BoxFuture<'a, ReviewDecision> { - let key = self.approval_key(req); let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); - let cwd = req.cwd.clone(); let retry_reason = ctx.retry_reason.clone(); - let user_explicitly_approved = req.user_explicitly_approved; + let approval_keys = self.approval_keys(req); + let changes = req.changes.clone(); Box::pin(async move { - with_cached_approval(&session.services, key, move || async move { - if let Some(reason) = retry_reason { - session - .request_command_approval( - turn, - call_id, - vec!["apply_patch".to_string()], - cwd, - Some(reason), - None, - ) - .await - } else if user_explicitly_approved { - ReviewDecision::ApprovedForSession - } else { - ReviewDecision::Approved - } + if let Some(reason) = retry_reason { + let rx_approve = session + .request_patch_approval(turn, call_id, changes.clone(), Some(reason), None) + .await; + return rx_approve.await.unwrap_or_default(); + } + + with_cached_approval(&session.services, approval_keys, || async move { + let rx_approve = session + .request_patch_approval(turn, call_id, changes, None, None) + .await; + rx_approve.await.unwrap_or_default() }) .await }) @@ -134,6 +124,17 @@ impl Approvable for ApplyPatchRuntime { fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool { !matches!(policy, AskForApproval::Never) } + + // apply_patch approvals are decided upstream by assess_patch_safety. + // + // This override ensures the orchestrator runs the patch approval flow when required instead + // of falling back to the global exec approval policy. + fn exec_approval_requirement( + &self, + req: &ApplyPatchRequest, + ) -> Option { + Some(req.exec_approval_requirement.clone()) + } } impl ToolRuntime for ApplyPatchRuntime { diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 528efad7409..49052bc06b9 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -74,12 +74,12 @@ impl Sandboxable for ShellRuntime { impl Approvable for ShellRuntime { type ApprovalKey = ApprovalKey; - fn approval_key(&self, req: &ShellRequest) -> Self::ApprovalKey { - ApprovalKey { + fn approval_keys(&self, req: &ShellRequest) -> Vec { + vec![ApprovalKey { command: req.command.clone(), cwd: req.cwd.clone(), sandbox_permissions: req.sandbox_permissions, - } + }] } fn start_approval_async<'a>( @@ -87,7 +87,7 @@ impl Approvable for ShellRuntime { req: &'a ShellRequest, ctx: ApprovalCtx<'a>, ) -> BoxFuture<'a, ReviewDecision> { - let key = self.approval_key(req); + let keys = self.approval_keys(req); let command = req.command.clone(); let cwd = req.cwd.clone(); let reason = ctx @@ -98,7 +98,7 @@ impl Approvable for ShellRuntime { let turn = ctx.turn; let call_id = ctx.call_id.to_string(); Box::pin(async move { - with_cached_approval(&session.services, key, move || async move { + with_cached_approval(&session.services, keys, move || async move { session .request_command_approval( turn, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index da1f518e8fb..de47ad62a81 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -92,12 +92,12 @@ impl Sandboxable for UnifiedExecRuntime<'_> { impl Approvable for UnifiedExecRuntime<'_> { type ApprovalKey = UnifiedExecApprovalKey; - fn approval_key(&self, req: &UnifiedExecRequest) -> Self::ApprovalKey { - UnifiedExecApprovalKey { + fn approval_keys(&self, req: &UnifiedExecRequest) -> Vec { + vec![UnifiedExecApprovalKey { command: req.command.clone(), cwd: req.cwd.clone(), sandbox_permissions: req.sandbox_permissions, - } + }] } fn start_approval_async<'b>( @@ -105,7 +105,7 @@ impl Approvable for UnifiedExecRuntime<'_> { req: &'b UnifiedExecRequest, ctx: ApprovalCtx<'b>, ) -> BoxFuture<'b, ReviewDecision> { - let key = self.approval_key(req); + let keys = self.approval_keys(req); let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); @@ -116,7 +116,7 @@ impl Approvable for UnifiedExecRuntime<'_> { .clone() .or_else(|| req.justification.clone()); Box::pin(async move { - with_cached_approval(&session.services, key, || async move { + with_cached_approval(&session.services, keys, || async move { session .request_command_approval( turn, diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 14dda62a8a6..82af60e3d6f 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -49,28 +49,44 @@ impl ApprovalStore { } } +/// Takes a vector of approval keys and returns a ReviewDecision. +/// There will be one key in most cases, but apply_patch can modify multiple files at once. +/// +/// - If all keys are already approved for session, we skip prompting. +/// - If the user approves for session, we store the decision for each key individually +/// so future requests touching any subset can also skip prompting. pub(crate) async fn with_cached_approval( services: &SessionServices, - key: K, + keys: Vec, fetch: F, ) -> ReviewDecision where - K: Serialize + Clone, + K: Serialize, F: FnOnce() -> Fut, Fut: Future, { - { + // To be defensive here, don't bother with checking the cache if keys are empty. + if keys.is_empty() { + return fetch().await; + } + + let already_approved = { let store = services.tool_approvals.lock().await; - if let Some(decision) = store.get(&key) { - return decision; - } + keys.iter() + .all(|key| matches!(store.get(key), Some(ReviewDecision::ApprovedForSession))) + }; + + if already_approved { + return ReviewDecision::ApprovedForSession; } let decision = fetch().await; if matches!(decision, ReviewDecision::ApprovedForSession) { let mut store = services.tool_approvals.lock().await; - store.put(key, ReviewDecision::ApprovedForSession); + for key in keys { + store.put(key, ReviewDecision::ApprovedForSession); + } } decision @@ -161,7 +177,14 @@ pub(crate) enum SandboxOverride { pub(crate) trait Approvable { type ApprovalKey: Hash + Eq + Clone + Debug + Serialize; - fn approval_key(&self, req: &Req) -> Self::ApprovalKey; + // In most cases (shell, unified_exec), a request will have a single approval key. + // + // However, apply_patch needs session "approve once, don't ask again" semantics that + // apply to multiple atomic targets (e.g., apply_patch approves per file path). Returning + // a list of keys lets the runtime treat the request as approved-for-session only if + // *all* keys are already approved, while still caching approvals per-key so future + // requests touching a subset can be auto-approved. + fn approval_keys(&self, req: &Req) -> Vec; /// Some tools may request to skip the sandbox on the first attempt /// (e.g., when the request explicitly asks for escalated permissions). diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 74e38534bd6..ff8eca6f997 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1561,6 +1561,123 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { Ok(()) } +#[tokio::test(flavor = "current_thread")] +#[cfg(unix)] +async fn approving_apply_patch_for_session_skips_future_prompts_for_same_file() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: false, + exclude_slash_tmp: false, + }; + let sandbox_policy_for_config = sandbox_policy.clone(); + + let mut builder = test_codex() + .with_model("gpt-5.1-codex") + .with_config(move |config| { + config.approval_policy = Constrained::allow_any(approval_policy); + config.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config); + }); + let test = builder.build(&server).await?; + + let target = TargetPath::OutsideWorkspace("apply_patch_allow_session.txt"); + let (path, patch_path) = target.resolve_for_patch(&test); + let _ = fs::remove_file(&path); + + let patch_add = build_add_file_patch(&patch_path, "before"); + let patch_update = format!( + "*** Begin Patch\n*** Update File: {patch_path}\n@@\n-before\n+after\n*** End Patch\n" + ); + + let call_id_1 = "apply_patch_allow_session_1"; + let call_id_2 = "apply_patch_allow_session_2"; + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_apply_patch_function_call(call_id_1, &patch_add), + ev_completed("resp-1"), + ]), + ) + .await; + let _ = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ) + .await; + + submit_turn( + &test, + "apply_patch allow session", + approval_policy, + sandbox_policy.clone(), + ) + .await?; + let _ = expect_patch_approval(&test, call_id_1).await; + test.codex + .submit(Op::PatchApproval { + id: "0".into(), + decision: ReviewDecision::ApprovedForSession, + }) + .await?; + wait_for_completion(&test).await; + assert!(fs::read_to_string(&path)?.contains("before")); + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-3"), + ev_apply_patch_function_call(call_id_2, &patch_update), + ev_completed("resp-3"), + ]), + ) + .await; + let _ = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-2", "done"), + ev_completed("resp-4"), + ]), + ) + .await; + + submit_turn( + &test, + "apply_patch allow session followup", + approval_policy, + sandbox_policy.clone(), + ) + .await?; + + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ApplyPatchApprovalRequest(_) | EventMsg::TaskComplete(_) + ) + }) + .await; + match event { + EventMsg::TaskComplete(_) => {} + EventMsg::ApplyPatchApprovalRequest(event) => { + panic!("unexpected patch approval request: {:?}", event.call_id) + } + other => panic!("unexpected event: {other:?}"), + } + + assert!(fs::read_to_string(&path)?.contains("after")); + let _ = fs::remove_file(path); + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] #[cfg(unix)] async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts() -> Result<()> { diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index d42861eb1d5..0d6706b29e3 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -494,6 +494,12 @@ fn patch_options() -> Vec { display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], }, + ApprovalOption { + label: "Yes, and don't ask again for these files".to_string(), + decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], + }, ApprovalOption { label: "No, and tell Codex what to do differently".to_string(), decision: ApprovalDecision::Review(ReviewDecision::Abort), diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap index ed18675ac39..e394605dcc5 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap @@ -2,6 +2,8 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend().vt100().screen().contents() --- + + Would you like to make the following edits? Reason: The model wants to apply changes @@ -12,6 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for these files (a) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/tui2/src/bottom_pane/approval_overlay.rs b/codex-rs/tui2/src/bottom_pane/approval_overlay.rs index d42861eb1d5..0d6706b29e3 100644 --- a/codex-rs/tui2/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui2/src/bottom_pane/approval_overlay.rs @@ -494,6 +494,12 @@ fn patch_options() -> Vec { display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], }, + ApprovalOption { + label: "Yes, and don't ask again for these files".to_string(), + decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], + }, ApprovalOption { label: "No, and tell Codex what to do differently".to_string(), decision: ApprovalDecision::Review(ReviewDecision::Abort), diff --git a/codex-rs/tui2/src/chatwidget/snapshots/codex_tui2__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui2/src/chatwidget/snapshots/codex_tui2__chatwidget__tests__approval_modal_patch.snap index a5bfd136b78..c2ec1675066 100644 --- a/codex-rs/tui2/src/chatwidget/snapshots/codex_tui2__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui2/src/chatwidget/snapshots/codex_tui2__chatwidget__tests__approval_modal_patch.snap @@ -2,6 +2,8 @@ source: tui2/src/chatwidget/tests.rs expression: terminal.backend().vt100().screen().contents() --- + + Would you like to make the following edits? Reason: The model wants to apply changes @@ -12,6 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for these files (a) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/tui2/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui2/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap index ed18675ac39..e394605dcc5 100644 --- a/codex-rs/tui2/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui2/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap @@ -2,6 +2,8 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend().vt100().screen().contents() --- + + Would you like to make the following edits? Reason: The model wants to apply changes @@ -12,6 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for these files (a) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs index 2eb7d487580..7054103e30c 100644 --- a/codex-rs/utils/absolute-path/src/lib.rs +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -17,7 +17,7 @@ use ts_rs::TS; /// using [AbsolutePathBufGuard::new]. If no base path is set, the /// deserialization will fail unless the path being deserialized is already /// absolute. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, JsonSchema, TS)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, JsonSchema, TS)] pub struct AbsolutePathBuf(PathBuf); impl AbsolutePathBuf {