From 3d59b1f68057985179b2d756233398ba029aea25 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Mon, 22 Dec 2025 14:26:49 -0800 Subject: [PATCH 01/16] fix: implement 'Allow this session' for apply_patch --- .../app-server/src/bespoke_event_handling.rs | 35 ++++--- codex-rs/core/src/apply_patch.rs | 12 ++- codex-rs/core/src/codex.rs | 4 + codex-rs/core/src/lib.rs | 2 + codex-rs/core/src/patch_approval_store.rs | 98 +++++++++++++++++++ codex-rs/core/src/safety.rs | 16 ++- codex-rs/core/src/state/service.rs | 3 + .../tui/src/bottom_pane/approval_overlay.rs | 6 ++ ...atwidget__tests__approval_modal_patch.snap | 5 +- .../tui2/src/bottom_pane/approval_overlay.rs | 6 ++ ...atwidget__tests__approval_modal_patch.snap | 5 +- ...atwidget__tests__approval_modal_patch.snap | 5 +- codex-rs/tui2/src/insert_history.rs | 6 +- codex-rs/tui2/src/test_backend.rs | 4 + codex-rs/utils/absolute-path/src/lib.rs | 2 +- 15 files changed, 189 insertions(+), 20 deletions(-) create mode 100644 codex-rs/core/src/patch_approval_store.rs diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index c9b78fe8c88..facc510e195 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -1193,6 +1193,18 @@ fn format_file_change_diff(change: &CoreFileChange) -> String { } } +fn map_file_change_approval_decision( + decision: ApprovalDecision, +) -> (ReviewDecision, Option) { + match decision { + ApprovalDecision::Accept => (ReviewDecision::Approved, None), + ApprovalDecision::AcceptForSession => (ReviewDecision::ApprovedForSession, None), + ApprovalDecision::AcceptWithExecpolicyAmendment { .. } => (ReviewDecision::Approved, None), + ApprovalDecision::Decline => (ReviewDecision::Denied, Some(PatchApplyStatus::Declined)), + ApprovalDecision::Cancel => (ReviewDecision::Abort, Some(PatchApplyStatus::Declined)), + } +} + #[allow(clippy::too_many_arguments)] async fn on_file_change_request_approval_response( event_turn_id: String, @@ -1215,19 +1227,8 @@ async fn on_file_change_request_approval_response( } }); - 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) @@ -1442,6 +1443,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(ApprovalDecision::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/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 67433303e5b..6254bd58c27 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -39,11 +39,16 @@ pub(crate) async fn apply_patch( call_id: &str, action: ApplyPatchAction, ) -> InternalApplyPatchInvocation { + let session_patch_approved = { + let store = sess.services.patch_approvals.lock().await; + store.is_action_approved(&action, &action.cwd) + }; match assess_patch_safety( &action, turn_context.approval_policy, &turn_context.sandbox_policy, &turn_context.cwd, + session_patch_approved, ) { SafetyCheck::AutoApprove { user_explicitly_approved, @@ -69,7 +74,12 @@ pub(crate) async fn apply_patch( None, ) .await; - match rx_approve.await.unwrap_or_default() { + let decision = rx_approve.await.unwrap_or_default(); + if matches!(decision, ReviewDecision::ApprovedForSession) { + let mut store = sess.services.patch_approvals.lock().await; + store.approve_action(&action, &action.cwd); + } + match decision { ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } | ReviewDecision::ApprovedForSession => { diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 23feace8144..6ce4c01a3ee 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -95,6 +95,7 @@ use crate::feedback_tags; use crate::mcp::auth::compute_auth_statuses; use crate::mcp_connection_manager::McpConnectionManager; use crate::model_provider_info::CHAT_WIRE_API_DEPRECATION_SUMMARY; +use crate::patch_approval_store::PatchApprovalStore; use crate::project_doc::get_user_instructions; use crate::protocol::AgentMessageContentDeltaEvent; use crate::protocol::AgentReasoningSectionBreakEvent; @@ -687,6 +688,7 @@ impl Session { otel_manager, models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), + patch_approvals: Mutex::new(PatchApprovalStore::default()), skills_manager, agent_control, }; @@ -3526,6 +3528,7 @@ mod tests { otel_manager: otel_manager.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), + patch_approvals: Mutex::new(PatchApprovalStore::default()), skills_manager, agent_control, }; @@ -3617,6 +3620,7 @@ mod tests { otel_manager: otel_manager.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), + patch_approvals: Mutex::new(PatchApprovalStore::default()), skills_manager, agent_control, }; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index b9f2645b13e..99e7032cf82 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -35,6 +35,8 @@ pub mod landlock; pub mod mcp; mod mcp_connection_manager; pub mod models_manager; +pub mod openai_models; +mod patch_approval_store; pub use mcp_connection_manager::MCP_SANDBOX_STATE_CAPABILITY; pub use mcp_connection_manager::MCP_SANDBOX_STATE_METHOD; pub use mcp_connection_manager::SandboxState; diff --git a/codex-rs/core/src/patch_approval_store.rs b/codex-rs/core/src/patch_approval_store.rs new file mode 100644 index 00000000000..e2c7e60f83e --- /dev/null +++ b/codex-rs/core/src/patch_approval_store.rs @@ -0,0 +1,98 @@ +use codex_apply_patch::ApplyPatchAction; +use codex_apply_patch::ApplyPatchFileChange; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::collections::HashSet; +use std::path::Path; +use std::path::PathBuf; + +#[derive(Debug, Default)] +pub(crate) struct PatchApprovalStore { + approved_paths: HashSet, +} + +impl PatchApprovalStore { + pub fn approve_action(&mut self, action: &ApplyPatchAction, cwd: &Path) { + for (path, change) in action.changes() { + if let Some(abs) = resolve_abs(cwd, path) { + self.approved_paths.insert(abs); + } + if let ApplyPatchFileChange::Update { move_path, .. } = change + && let Some(dest) = move_path + && let Some(abs) = resolve_abs(cwd, dest) + { + self.approved_paths.insert(abs); + } + } + } + + pub fn is_action_approved(&self, action: &ApplyPatchAction, cwd: &Path) -> bool { + for (path, change) in action.changes() { + let Some(abs) = resolve_abs(cwd, path) else { + return false; + }; + if !self.approved_paths.contains(&abs) { + return false; + } + if let ApplyPatchFileChange::Update { move_path, .. } = change + && let Some(dest) = move_path + && let Some(abs) = resolve_abs(cwd, dest) + && !self.approved_paths.contains(&abs) + { + return false; + } + } + true + } +} + +fn resolve_abs(cwd: &Path, path: &PathBuf) -> Option { + AbsolutePathBuf::resolve_path_against_base(path, cwd).ok() +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + fn parse_action(cwd: &Path, patch: &str) -> ApplyPatchAction { + let argv = vec!["apply_patch".to_string(), patch.to_string()]; + match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, cwd) { + codex_apply_patch::MaybeApplyPatchVerified::Body(action) => action, + other => panic!("expected patch body, got: {other:?}"), + } + } + + #[test] + fn approved_for_session_covers_all_touched_files() { + let tmp = TempDir::new().expect("tmp"); + let cwd = tmp.path(); + + let patch_two_files = r#"*** Begin Patch +*** Add File: a.txt ++hello +*** Add File: b.txt ++world +*** End Patch"#; + let action = parse_action(cwd, patch_two_files); + + let mut store = PatchApprovalStore::default(); + assert!(!store.is_action_approved(&action, cwd)); + + store.approve_action(&action, cwd); + assert!(store.is_action_approved(&action, cwd)); + + let patch_a_only = r#"*** Begin Patch +*** Add File: a.txt ++again +*** End Patch"#; + let action_a = parse_action(cwd, patch_a_only); + assert!(store.is_action_approved(&action_a, cwd)); + + let patch_c_only = r#"*** Begin Patch +*** Add File: c.txt ++nope +*** End Patch"#; + let action_c = parse_action(cwd, patch_c_only); + assert!(!store.is_action_approved(&action_c, cwd)); + } +} diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index c3930b4f428..2c96cffaef2 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -67,6 +67,7 @@ pub fn assess_patch_safety( policy: AskForApproval, sandbox_policy: &SandboxPolicy, cwd: &Path, + session_patch_approved: bool, ) -> SafetyCheck { if action.is_empty() { return SafetyCheck::Reject { @@ -74,6 +75,19 @@ pub fn assess_patch_safety( }; } + if session_patch_approved { + let sandbox_type = match sandbox_policy { + SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { + SandboxType::None + } + _ => get_platform_sandbox().unwrap_or(SandboxType::None), + }; + return SafetyCheck::AutoApprove { + sandbox_type, + user_explicitly_approved: true, + }; + } + match policy { AskForApproval::OnFailure | AskForApproval::Never | AskForApproval::OnRequest => { // Continue to see if this can be auto-approved. @@ -277,7 +291,7 @@ mod tests { }; assert_eq!( - assess_patch_safety(&add_inside, AskForApproval::OnRequest, &policy, &cwd,), + assess_patch_safety(&add_inside, AskForApproval::OnRequest, &policy, &cwd, false), SafetyCheck::AutoApprove { sandbox_type: SandboxType::None, user_explicitly_approved: false, diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 8520dbb2526..90033a42300 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -6,6 +6,8 @@ use crate::agent::AgentControl; use crate::exec_policy::ExecPolicyManager; use crate::mcp_connection_manager::McpConnectionManager; use crate::models_manager::manager::ModelsManager; +use crate::openai_models::models_manager::ModelsManager; +use crate::patch_approval_store::PatchApprovalStore; use crate::skills::SkillsManager; use crate::tools::sandboxing::ApprovalStore; use crate::unified_exec::UnifiedExecSessionManager; @@ -28,6 +30,7 @@ pub(crate) struct SessionServices { pub(crate) models_manager: Arc, pub(crate) otel_manager: OtelManager, pub(crate) tool_approvals: Mutex, + pub(crate) patch_approvals: Mutex, pub(crate) skills_manager: Arc, pub(crate) agent_control: AgentControl, } diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index d42861eb1d5..451c5bd2c9b 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 this session".to_string(), + decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('s'))], + }, 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..39daf9a1fcc 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 this session (s) + 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..451c5bd2c9b 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 this session".to_string(), + decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('s'))], + }, 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..0cfb7ef7ae8 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 this session (s) + 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..39daf9a1fcc 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 this session (s) + 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/insert_history.rs b/codex-rs/tui2/src/insert_history.rs index 1b236e8eec3..69e89805fd7 100644 --- a/codex-rs/tui2/src/insert_history.rs +++ b/codex-rs/tui2/src/insert_history.rs @@ -30,10 +30,11 @@ use crossterm::Command; use crossterm::cursor::MoveTo; use crossterm::queue; use crossterm::style::Color as CColor; -use crossterm::style::Colors; use crossterm::style::Print; use crossterm::style::SetAttribute; +use crossterm::style::SetBackgroundColor; use crossterm::style::SetColors; +use crossterm::style::SetForegroundColor; use crossterm::terminal::Clear; use crossterm::terminal::ClearType; use ratatui::layout::Size; @@ -279,6 +280,9 @@ where if next_fg != fg || next_bg != bg { queue!(writer, SetColors(Colors::new(next_fg, next_bg)))?; fg = next_fg; + } + if next_bg != bg { + queue!(writer, SetBackgroundColor(next_bg.into()))?; bg = next_bg; } diff --git a/codex-rs/tui2/src/test_backend.rs b/codex-rs/tui2/src/test_backend.rs index 47f17e6b93a..12ebc4c9075 100644 --- a/codex-rs/tui2/src/test_backend.rs +++ b/codex-rs/tui2/src/test_backend.rs @@ -2,6 +2,7 @@ use std::fmt::{self}; use std::io::Write; use std::io::{self}; +use crossterm::style::Colored; use ratatui::prelude::CrosstermBackend; use ratatui::backend::Backend; @@ -26,6 +27,9 @@ impl VT100Backend { /// Creates a new `TestBackend` with the specified width and height. pub fn new(width: u16, height: u16) -> Self { crossterm::style::Colored::set_ansi_color_disabled(false); + // Our tests assert against ANSI-colored output. crossterm suppresses ANSI color sequences + // when `NO_COLOR` is set, which is common in CI and in sandboxed environments. + Colored::set_ansi_color_disabled(false); Self { crossterm_backend: CrosstermBackend::new(vt100::Parser::new(height, width, 0)), } 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 { From e395603d1cfe9609e6d36203077ad721f0de6dc2 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Mon, 5 Jan 2026 13:15:18 -0800 Subject: [PATCH 02/16] clean up --- codex-rs/core/src/lib.rs | 1 - codex-rs/core/src/state/service.rs | 1 - codex-rs/core/tests/suite/approvals.rs | 117 +++++++++++++++++++++++++ codex-rs/tui2/src/insert_history.rs | 6 +- codex-rs/tui2/src/test_backend.rs | 4 - 5 files changed, 118 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 99e7032cf82..b4d616da23c 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -35,7 +35,6 @@ pub mod landlock; pub mod mcp; mod mcp_connection_manager; pub mod models_manager; -pub mod openai_models; mod patch_approval_store; pub use mcp_connection_manager::MCP_SANDBOX_STATE_CAPABILITY; pub use mcp_connection_manager::MCP_SANDBOX_STATE_METHOD; diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 90033a42300..e76916506d9 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -6,7 +6,6 @@ use crate::agent::AgentControl; use crate::exec_policy::ExecPolicyManager; use crate::mcp_connection_manager::McpConnectionManager; use crate::models_manager::manager::ModelsManager; -use crate::openai_models::models_manager::ModelsManager; use crate::patch_approval_store::PatchApprovalStore; use crate::skills::SkillsManager; use crate::tools::sandboxing::ApprovalStore; 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/tui2/src/insert_history.rs b/codex-rs/tui2/src/insert_history.rs index 69e89805fd7..1b236e8eec3 100644 --- a/codex-rs/tui2/src/insert_history.rs +++ b/codex-rs/tui2/src/insert_history.rs @@ -30,11 +30,10 @@ use crossterm::Command; use crossterm::cursor::MoveTo; use crossterm::queue; use crossterm::style::Color as CColor; +use crossterm::style::Colors; use crossterm::style::Print; use crossterm::style::SetAttribute; -use crossterm::style::SetBackgroundColor; use crossterm::style::SetColors; -use crossterm::style::SetForegroundColor; use crossterm::terminal::Clear; use crossterm::terminal::ClearType; use ratatui::layout::Size; @@ -280,9 +279,6 @@ where if next_fg != fg || next_bg != bg { queue!(writer, SetColors(Colors::new(next_fg, next_bg)))?; fg = next_fg; - } - if next_bg != bg { - queue!(writer, SetBackgroundColor(next_bg.into()))?; bg = next_bg; } diff --git a/codex-rs/tui2/src/test_backend.rs b/codex-rs/tui2/src/test_backend.rs index 12ebc4c9075..47f17e6b93a 100644 --- a/codex-rs/tui2/src/test_backend.rs +++ b/codex-rs/tui2/src/test_backend.rs @@ -2,7 +2,6 @@ use std::fmt::{self}; use std::io::Write; use std::io::{self}; -use crossterm::style::Colored; use ratatui::prelude::CrosstermBackend; use ratatui::backend::Backend; @@ -27,9 +26,6 @@ impl VT100Backend { /// Creates a new `TestBackend` with the specified width and height. pub fn new(width: u16, height: u16) -> Self { crossterm::style::Colored::set_ansi_color_disabled(false); - // Our tests assert against ANSI-colored output. crossterm suppresses ANSI color sequences - // when `NO_COLOR` is set, which is common in CI and in sandboxed environments. - Colored::set_ansi_color_disabled(false); Self { crossterm_backend: CrosstermBackend::new(vt100::Parser::new(height, width, 0)), } From e45c51ad61a000e17005aa5a324a67d437a5a33f Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Mon, 5 Jan 2026 13:28:55 -0800 Subject: [PATCH 03/16] add app-server test --- .../app-server/tests/suite/v2/turn_start.rs | 184 ++++++++++++++++++ 1 file changed, 184 insertions(+) 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..68984f8afee 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -782,6 +782,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: ApprovalDecision::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(())); From b824fdac1810e668d32e3b9393b4929a569fb4b5 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Mon, 5 Jan 2026 13:37:43 -0800 Subject: [PATCH 04/16] ApprovalDecision -> CommandExecutionApprovalDecision + FileChangeApprovalDecision --- .../app-server-protocol/src/protocol/v2.rs | 17 +++++++-- codex-rs/app-server-test-client/src/main.rs | 7 ++-- .../app-server/src/bespoke_event_handling.rs | 36 +++++++++++-------- .../app-server/tests/suite/v2/turn_start.rs | 11 +++--- 4 files changed, 45 insertions(+), 26 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index dedf51ea1bb..af2ff82e561 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -487,7 +487,7 @@ 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 { Accept, /// Approve and remember the approval for the session. AcceptForSession, @@ -498,6 +498,17 @@ pub enum ApprovalDecision { Cancel, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub enum FileChangeApprovalDecision { + Accept, + /// Approve and remember the approval for the session. + AcceptForSession, + Decline, + Cancel, +} + #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] @@ -1884,7 +1895,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 +1915,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 facc510e195..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; @@ -1194,14 +1195,17 @@ fn format_file_change_diff(change: &CoreFileChange) -> String { } fn map_file_change_approval_decision( - decision: ApprovalDecision, + decision: FileChangeApprovalDecision, ) -> (ReviewDecision, Option) { match decision { - ApprovalDecision::Accept => (ReviewDecision::Approved, None), - ApprovalDecision::AcceptForSession => (ReviewDecision::ApprovedForSession, None), - ApprovalDecision::AcceptWithExecpolicyAmendment { .. } => (ReviewDecision::Approved, None), - ApprovalDecision::Decline => (ReviewDecision::Denied, Some(PatchApplyStatus::Declined)), - ApprovalDecision::Cancel => (ReviewDecision::Abort, Some(PatchApplyStatus::Declined)), + FileChangeApprovalDecision::Accept => (ReviewDecision::Approved, None), + FileChangeApprovalDecision::AcceptForSession => (ReviewDecision::ApprovedForSession, None), + FileChangeApprovalDecision::Decline => { + (ReviewDecision::Denied, Some(PatchApplyStatus::Declined)) + } + FileChangeApprovalDecision::Cancel => { + (ReviewDecision::Abort, Some(PatchApplyStatus::Declined)) + } } } @@ -1223,7 +1227,7 @@ 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, } }); @@ -1282,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 { @@ -1299,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), ), @@ -1446,7 +1452,7 @@ mod tests { #[test] fn file_change_accept_for_session_maps_to_approved_for_session() { let (decision, completion_status) = - map_file_change_approval_decision(ApprovalDecision::AcceptForSession); + map_file_change_approval_decision(FileChangeApprovalDecision::AcceptForSession); assert_eq!(decision, ReviewDecision::ApprovedForSession); assert_eq!(completion_status, None); } 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 68984f8afee..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?; @@ -883,7 +884,7 @@ async fn turn_start_file_change_approval_accept_for_session_persists_v2() -> Res mcp.send_response( request_id, serde_json::to_value(FileChangeRequestApprovalResponse { - decision: ApprovalDecision::AcceptForSession, + decision: FileChangeApprovalDecision::AcceptForSession, })?, ) .await?; @@ -1072,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?; From b470dae31afe09ccc5725a78bedb2ad80b81e2e1 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Mon, 5 Jan 2026 14:16:41 -0800 Subject: [PATCH 05/16] remove cached approval checks in apply_patch runtime --- .../core/src/tools/runtimes/apply_patch.rs | 52 +++++++------------ 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 26d04f578c5..53b30ec3d42 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -17,7 +17,6 @@ use crate::tools::sandboxing::SandboxablePreference; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; -use crate::tools::sandboxing::with_cached_approval; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; use futures::future::BoxFuture; @@ -36,12 +35,6 @@ pub struct ApplyPatchRequest { #[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 @@ -87,21 +80,15 @@ impl Sandboxable for ApplyPatchRuntime { } impl Approvable for ApplyPatchRuntime { - type ApprovalKey = ApprovalKey; + type ApprovalKey = (); - fn approval_key(&self, req: &ApplyPatchRequest) -> Self::ApprovalKey { - ApprovalKey { - patch: req.patch.clone(), - cwd: req.cwd.clone(), - } - } + fn approval_key(&self, _req: &ApplyPatchRequest) -> Self::ApprovalKey {} fn start_approval_async<'a>( &'a mut self, 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(); @@ -109,25 +96,22 @@ impl Approvable for ApplyPatchRuntime { let retry_reason = ctx.retry_reason.clone(); let user_explicitly_approved = req.user_explicitly_approved; 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 - } - }) - .await + 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 + } }) } From f1aeeee397c5f5bbd36c27d9d297662848afcb60 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Mon, 5 Jan 2026 14:35:14 -0800 Subject: [PATCH 06/16] PatchApprovalStore -> ApplyPatchApprovalStore --- codex-rs/core/src/apply_patch.rs | 4 ++-- ...ch_approval_store.rs => apply_patch_approval_store.rs} | 6 +++--- codex-rs/core/src/codex.rs | 8 ++++---- codex-rs/core/src/lib.rs | 2 +- codex-rs/core/src/state/service.rs | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-) rename codex-rs/core/src/{patch_approval_store.rs => apply_patch_approval_store.rs} (95%) diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 6254bd58c27..5c056f68411 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -40,7 +40,7 @@ pub(crate) async fn apply_patch( action: ApplyPatchAction, ) -> InternalApplyPatchInvocation { let session_patch_approved = { - let store = sess.services.patch_approvals.lock().await; + let store = sess.services.apply_patch_approvals.lock().await; store.is_action_approved(&action, &action.cwd) }; match assess_patch_safety( @@ -76,7 +76,7 @@ pub(crate) async fn apply_patch( .await; let decision = rx_approve.await.unwrap_or_default(); if matches!(decision, ReviewDecision::ApprovedForSession) { - let mut store = sess.services.patch_approvals.lock().await; + let mut store = sess.services.apply_patch_approvals.lock().await; store.approve_action(&action, &action.cwd); } match decision { diff --git a/codex-rs/core/src/patch_approval_store.rs b/codex-rs/core/src/apply_patch_approval_store.rs similarity index 95% rename from codex-rs/core/src/patch_approval_store.rs rename to codex-rs/core/src/apply_patch_approval_store.rs index e2c7e60f83e..a42c85df62e 100644 --- a/codex-rs/core/src/patch_approval_store.rs +++ b/codex-rs/core/src/apply_patch_approval_store.rs @@ -6,11 +6,11 @@ use std::path::Path; use std::path::PathBuf; #[derive(Debug, Default)] -pub(crate) struct PatchApprovalStore { +pub(crate) struct ApplyPatchApprovalStore { approved_paths: HashSet, } -impl PatchApprovalStore { +impl ApplyPatchApprovalStore { pub fn approve_action(&mut self, action: &ApplyPatchAction, cwd: &Path) { for (path, change) in action.changes() { if let Some(abs) = resolve_abs(cwd, path) { @@ -75,7 +75,7 @@ mod tests { *** End Patch"#; let action = parse_action(cwd, patch_two_files); - let mut store = PatchApprovalStore::default(); + let mut store = ApplyPatchApprovalStore::default(); assert!(!store.is_action_approved(&action, cwd)); store.approve_action(&action, cwd); diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 6ce4c01a3ee..e2277851264 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -95,7 +95,7 @@ use crate::feedback_tags; use crate::mcp::auth::compute_auth_statuses; use crate::mcp_connection_manager::McpConnectionManager; use crate::model_provider_info::CHAT_WIRE_API_DEPRECATION_SUMMARY; -use crate::patch_approval_store::PatchApprovalStore; +use crate::apply_patch_approval_store::ApplyPatchApprovalStore; use crate::project_doc::get_user_instructions; use crate::protocol::AgentMessageContentDeltaEvent; use crate::protocol::AgentReasoningSectionBreakEvent; @@ -688,7 +688,7 @@ impl Session { otel_manager, models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - patch_approvals: Mutex::new(PatchApprovalStore::default()), + apply_patch_approvals: Mutex::new(ApplyPatchApprovalStore::default()), skills_manager, agent_control, }; @@ -3528,7 +3528,7 @@ mod tests { otel_manager: otel_manager.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - patch_approvals: Mutex::new(PatchApprovalStore::default()), + apply_patch_approvals: Mutex::new(ApplyPatchApprovalStore::default()), skills_manager, agent_control, }; @@ -3620,7 +3620,7 @@ mod tests { otel_manager: otel_manager.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - patch_approvals: Mutex::new(PatchApprovalStore::default()), + apply_patch_approvals: Mutex::new(ApplyPatchApprovalStore::default()), skills_manager, agent_control, }; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index b4d616da23c..96d062247d8 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -35,7 +35,7 @@ pub mod landlock; pub mod mcp; mod mcp_connection_manager; pub mod models_manager; -mod patch_approval_store; +mod apply_patch_approval_store; pub use mcp_connection_manager::MCP_SANDBOX_STATE_CAPABILITY; pub use mcp_connection_manager::MCP_SANDBOX_STATE_METHOD; pub use mcp_connection_manager::SandboxState; diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index e76916506d9..d6cad8f4b84 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -6,7 +6,7 @@ use crate::agent::AgentControl; use crate::exec_policy::ExecPolicyManager; use crate::mcp_connection_manager::McpConnectionManager; use crate::models_manager::manager::ModelsManager; -use crate::patch_approval_store::PatchApprovalStore; +use crate::apply_patch_approval_store::ApplyPatchApprovalStore; use crate::skills::SkillsManager; use crate::tools::sandboxing::ApprovalStore; use crate::unified_exec::UnifiedExecSessionManager; @@ -29,7 +29,7 @@ pub(crate) struct SessionServices { pub(crate) models_manager: Arc, pub(crate) otel_manager: OtelManager, pub(crate) tool_approvals: Mutex, - pub(crate) patch_approvals: Mutex, + pub(crate) apply_patch_approvals: Mutex, pub(crate) skills_manager: Arc, pub(crate) agent_control: AgentControl, } From e69db799f5332f21e124c7c75e879e7a1164a1f3 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Mon, 5 Jan 2026 14:37:07 -0800 Subject: [PATCH 07/16] format --- codex-rs/core/src/codex.rs | 2 +- codex-rs/core/src/lib.rs | 2 +- codex-rs/core/src/state/service.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index e2277851264..8bf82098dcb 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -75,6 +75,7 @@ use tracing::warn; use crate::ModelProviderInfo; use crate::WireApi; +use crate::apply_patch_approval_store::ApplyPatchApprovalStore; use crate::client::ModelClient; use crate::client_common::Prompt; use crate::client_common::ResponseEvent; @@ -95,7 +96,6 @@ use crate::feedback_tags; use crate::mcp::auth::compute_auth_statuses; use crate::mcp_connection_manager::McpConnectionManager; use crate::model_provider_info::CHAT_WIRE_API_DEPRECATION_SUMMARY; -use crate::apply_patch_approval_store::ApplyPatchApprovalStore; use crate::project_doc::get_user_instructions; use crate::protocol::AgentMessageContentDeltaEvent; use crate::protocol::AgentReasoningSectionBreakEvent; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 96d062247d8..91d9e2488ed 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -16,6 +16,7 @@ mod codex_conversation; mod compact_remote; pub use codex_conversation::CodexConversation; mod agent; +mod apply_patch_approval_store; mod codex_delegate; mod command_safety; pub mod config; @@ -35,7 +36,6 @@ pub mod landlock; pub mod mcp; mod mcp_connection_manager; pub mod models_manager; -mod apply_patch_approval_store; pub use mcp_connection_manager::MCP_SANDBOX_STATE_CAPABILITY; pub use mcp_connection_manager::MCP_SANDBOX_STATE_METHOD; pub use mcp_connection_manager::SandboxState; diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index d6cad8f4b84..fc022206505 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -3,10 +3,10 @@ use std::sync::Arc; use crate::AuthManager; use crate::RolloutRecorder; use crate::agent::AgentControl; +use crate::apply_patch_approval_store::ApplyPatchApprovalStore; use crate::exec_policy::ExecPolicyManager; use crate::mcp_connection_manager::McpConnectionManager; use crate::models_manager::manager::ModelsManager; -use crate::apply_patch_approval_store::ApplyPatchApprovalStore; use crate::skills::SkillsManager; use crate::tools::sandboxing::ApprovalStore; use crate::unified_exec::UnifiedExecSessionManager; From b9931c230640f7028f30a4d198f57069d4c9f066 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Mon, 5 Jan 2026 15:05:13 -0800 Subject: [PATCH 08/16] shorten: remove 'this session' text --- codex-rs/tui/src/bottom_pane/approval_overlay.rs | 2 +- .../codex_tui__chatwidget__tests__approval_modal_patch.snap | 2 +- codex-rs/tui2/src/bottom_pane/approval_overlay.rs | 2 +- .../codex_tui2__chatwidget__tests__approval_modal_patch.snap | 2 +- .../codex_tui__chatwidget__tests__approval_modal_patch.snap | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 451c5bd2c9b..444f1487608 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -495,7 +495,7 @@ fn patch_options() -> Vec { additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], }, ApprovalOption { - label: "Yes, and don't ask again for these files this session".to_string(), + 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('s'))], 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 39daf9a1fcc..51296fb7a29 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 @@ -14,7 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. Yes, and don't ask again for these files this session (s) + 2. Yes, and don't ask again for these files (s) 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 451c5bd2c9b..444f1487608 100644 --- a/codex-rs/tui2/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui2/src/bottom_pane/approval_overlay.rs @@ -495,7 +495,7 @@ fn patch_options() -> Vec { additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], }, ApprovalOption { - label: "Yes, and don't ask again for these files this session".to_string(), + 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('s'))], 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 0cfb7ef7ae8..d261a8abe85 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 @@ -14,7 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. Yes, and don't ask again for these files this session (s) + 2. Yes, and don't ask again for these files (s) 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 39daf9a1fcc..51296fb7a29 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 @@ -14,7 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. Yes, and don't ask again for these files this session (s) + 2. Yes, and don't ask again for these files (s) 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel From fc1c0fd9b1ecc6d34d706cfb3e6853746db3e286 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Tue, 6 Jan 2026 13:50:26 -0800 Subject: [PATCH 09/16] remove extra store and extend approvable to work for apply_patch --- codex-rs/core/src/apply_patch.rs | 66 ++++--------- codex-rs/core/src/apply_patch_approval_key.rs | 79 +++++++++++++++ .../core/src/apply_patch_approval_store.rs | 98 ------------------- codex-rs/core/src/codex.rs | 4 - codex-rs/core/src/lib.rs | 2 +- codex-rs/core/src/safety.rs | 16 +-- codex-rs/core/src/state/service.rs | 2 - codex-rs/core/src/tools/events.rs | 7 +- .../core/src/tools/handlers/apply_patch.rs | 36 +++---- .../core/src/tools/runtimes/apply_patch.rs | 51 +++++++--- codex-rs/core/src/tools/runtimes/shell.rs | 12 +-- .../core/src/tools/runtimes/unified_exec.rs | 12 +-- codex-rs/core/src/tools/sandboxing.rs | 36 +++++-- 13 files changed, 198 insertions(+), 223 deletions(-) create mode 100644 codex-rs/core/src/apply_patch_approval_key.rs delete mode 100644 codex-rs/core/src/apply_patch_approval_store.rs diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 5c056f68411..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,70 +29,43 @@ 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 { - let session_patch_approved = { - let store = sess.services.apply_patch_approvals.lock().await; - store.is_action_approved(&action, &action.cwd) - }; match assess_patch_safety( &action, turn_context.approval_policy, &turn_context.sandbox_policy, &turn_context.cwd, - session_patch_approved, ) { SafetyCheck::AutoApprove { user_explicitly_approved, .. } => 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; - let decision = rx_approve.await.unwrap_or_default(); - if matches!(decision, ReviewDecision::ApprovedForSession) { - let mut store = sess.services.apply_patch_approvals.lock().await; - store.approve_action(&action, &action.cwd); - } - match decision { - 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/apply_patch_approval_key.rs b/codex-rs/core/src/apply_patch_approval_key.rs new file mode 100644 index 00000000000..60e47a07a69 --- /dev/null +++ b/codex-rs/core/src/apply_patch_approval_key.rs @@ -0,0 +1,79 @@ +use codex_apply_patch::ApplyPatchAction; +use codex_apply_patch::ApplyPatchFileChange; +use codex_utils_absolute_path::AbsolutePathBuf; +use serde::Serialize; +use std::path::Path; + +#[derive(Serialize, Clone, Debug, Eq, PartialEq, Hash)] +pub(crate) struct ApplyPatchFileApprovalKey { + kind: &'static str, + path: AbsolutePathBuf, +} + +impl ApplyPatchFileApprovalKey { + fn new(path: AbsolutePathBuf) -> Self { + Self { + kind: "applyPatchFile", + path, + } + } +} + +pub(crate) fn approval_keys_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) = approval_key_for_path(cwd, path) { + keys.push(key); + } + + if let ApplyPatchFileChange::Update { move_path, .. } = change + && let Some(dest) = move_path + && let Some(key) = approval_key_for_path(cwd, dest) + { + keys.push(key); + } + } + + keys +} + +fn approval_key_for_path(cwd: &Path, path: &Path) -> Option { + AbsolutePathBuf::resolve_path_against_base(path, cwd) + .ok() + .map(ApplyPatchFileApprovalKey::new) +} + +#[cfg(test)] +mod tests { + use super::*; + use codex_apply_patch::MaybeApplyPatchVerified; + 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 = approval_keys_for_action(&action); + assert_eq!(keys.len(), 2); + } +} diff --git a/codex-rs/core/src/apply_patch_approval_store.rs b/codex-rs/core/src/apply_patch_approval_store.rs deleted file mode 100644 index a42c85df62e..00000000000 --- a/codex-rs/core/src/apply_patch_approval_store.rs +++ /dev/null @@ -1,98 +0,0 @@ -use codex_apply_patch::ApplyPatchAction; -use codex_apply_patch::ApplyPatchFileChange; -use codex_utils_absolute_path::AbsolutePathBuf; -use std::collections::HashSet; -use std::path::Path; -use std::path::PathBuf; - -#[derive(Debug, Default)] -pub(crate) struct ApplyPatchApprovalStore { - approved_paths: HashSet, -} - -impl ApplyPatchApprovalStore { - pub fn approve_action(&mut self, action: &ApplyPatchAction, cwd: &Path) { - for (path, change) in action.changes() { - if let Some(abs) = resolve_abs(cwd, path) { - self.approved_paths.insert(abs); - } - if let ApplyPatchFileChange::Update { move_path, .. } = change - && let Some(dest) = move_path - && let Some(abs) = resolve_abs(cwd, dest) - { - self.approved_paths.insert(abs); - } - } - } - - pub fn is_action_approved(&self, action: &ApplyPatchAction, cwd: &Path) -> bool { - for (path, change) in action.changes() { - let Some(abs) = resolve_abs(cwd, path) else { - return false; - }; - if !self.approved_paths.contains(&abs) { - return false; - } - if let ApplyPatchFileChange::Update { move_path, .. } = change - && let Some(dest) = move_path - && let Some(abs) = resolve_abs(cwd, dest) - && !self.approved_paths.contains(&abs) - { - return false; - } - } - true - } -} - -fn resolve_abs(cwd: &Path, path: &PathBuf) -> Option { - AbsolutePathBuf::resolve_path_against_base(path, cwd).ok() -} - -#[cfg(test)] -mod tests { - use super::*; - use tempfile::TempDir; - - fn parse_action(cwd: &Path, patch: &str) -> ApplyPatchAction { - let argv = vec!["apply_patch".to_string(), patch.to_string()]; - match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, cwd) { - codex_apply_patch::MaybeApplyPatchVerified::Body(action) => action, - other => panic!("expected patch body, got: {other:?}"), - } - } - - #[test] - fn approved_for_session_covers_all_touched_files() { - let tmp = TempDir::new().expect("tmp"); - let cwd = tmp.path(); - - let patch_two_files = r#"*** Begin Patch -*** Add File: a.txt -+hello -*** Add File: b.txt -+world -*** End Patch"#; - let action = parse_action(cwd, patch_two_files); - - let mut store = ApplyPatchApprovalStore::default(); - assert!(!store.is_action_approved(&action, cwd)); - - store.approve_action(&action, cwd); - assert!(store.is_action_approved(&action, cwd)); - - let patch_a_only = r#"*** Begin Patch -*** Add File: a.txt -+again -*** End Patch"#; - let action_a = parse_action(cwd, patch_a_only); - assert!(store.is_action_approved(&action_a, cwd)); - - let patch_c_only = r#"*** Begin Patch -*** Add File: c.txt -+nope -*** End Patch"#; - let action_c = parse_action(cwd, patch_c_only); - assert!(!store.is_action_approved(&action_c, cwd)); - } -} diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 8bf82098dcb..23feace8144 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -75,7 +75,6 @@ use tracing::warn; use crate::ModelProviderInfo; use crate::WireApi; -use crate::apply_patch_approval_store::ApplyPatchApprovalStore; use crate::client::ModelClient; use crate::client_common::Prompt; use crate::client_common::ResponseEvent; @@ -688,7 +687,6 @@ impl Session { otel_manager, models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - apply_patch_approvals: Mutex::new(ApplyPatchApprovalStore::default()), skills_manager, agent_control, }; @@ -3528,7 +3526,6 @@ mod tests { otel_manager: otel_manager.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - apply_patch_approvals: Mutex::new(ApplyPatchApprovalStore::default()), skills_manager, agent_control, }; @@ -3620,7 +3617,6 @@ mod tests { otel_manager: otel_manager.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - apply_patch_approvals: Mutex::new(ApplyPatchApprovalStore::default()), skills_manager, agent_control, }; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 91d9e2488ed..8b2a3181583 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -7,6 +7,7 @@ pub mod api_bridge; mod apply_patch; +mod apply_patch_approval_key; pub mod auth; pub mod bash; mod client; @@ -16,7 +17,6 @@ mod codex_conversation; mod compact_remote; pub use codex_conversation::CodexConversation; mod agent; -mod apply_patch_approval_store; mod codex_delegate; mod command_safety; pub mod config; diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index 2c96cffaef2..601a5a8b81e 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -67,7 +67,6 @@ pub fn assess_patch_safety( policy: AskForApproval, sandbox_policy: &SandboxPolicy, cwd: &Path, - session_patch_approved: bool, ) -> SafetyCheck { if action.is_empty() { return SafetyCheck::Reject { @@ -75,19 +74,6 @@ pub fn assess_patch_safety( }; } - if session_patch_approved { - let sandbox_type = match sandbox_policy { - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { - SandboxType::None - } - _ => get_platform_sandbox().unwrap_or(SandboxType::None), - }; - return SafetyCheck::AutoApprove { - sandbox_type, - user_explicitly_approved: true, - }; - } - match policy { AskForApproval::OnFailure | AskForApproval::Never | AskForApproval::OnRequest => { // Continue to see if this can be auto-approved. @@ -291,7 +277,7 @@ mod tests { }; assert_eq!( - assess_patch_safety(&add_inside, AskForApproval::OnRequest, &policy, &cwd, false), + 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/state/service.rs b/codex-rs/core/src/state/service.rs index fc022206505..8520dbb2526 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -3,7 +3,6 @@ use std::sync::Arc; use crate::AuthManager; use crate::RolloutRecorder; use crate::agent::AgentControl; -use crate::apply_patch_approval_store::ApplyPatchApprovalStore; use crate::exec_policy::ExecPolicyManager; use crate::mcp_connection_manager::McpConnectionManager; use crate::models_manager::manager::ModelsManager; @@ -29,7 +28,6 @@ pub(crate) struct SessionServices { pub(crate) models_manager: Arc, pub(crate) otel_manager: OtelManager, pub(crate) tool_approvals: Mutex, - pub(crate) apply_patch_approvals: Mutex, pub(crate) skills_manager: Arc, pub(crate) agent_control: AgentControl, } 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..93783dcf3c8 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -4,6 +4,7 @@ use std::path::Path; use crate::apply_patch; use crate::apply_patch::InternalApplyPatchInvocation; use crate::apply_patch::convert_apply_patch_to_protocol; +use crate::apply_patch_approval_key::approval_keys_for_action; use crate::client_common::tools::FreeformTool; use crate::client_common::tools::FreeformToolFormat; use crate::client_common::tools::ResponsesApiTool; @@ -81,9 +82,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 +92,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 approval_keys = approval_keys_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 +105,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, + approval_keys, + 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 +178,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 +188,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 = approval_keys_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, + 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(), }; diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 53b30ec3d42..8d029504ca1 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -11,24 +11,29 @@ 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; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; +use crate::tools::sandboxing::with_cached_approval_set; +use codex_apply_patch::ApplyPatchAction; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::FileChange; use codex_protocol::protocol::ReviewDecision; 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 approval_keys: Vec, + pub changes: std::collections::HashMap, + pub exec_approval_requirement: ExecApprovalRequirement, pub timeout_ms: Option, - pub user_explicitly_approved: bool, pub codex_exe: Option, } @@ -51,8 +56,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(), @@ -80,9 +85,11 @@ impl Sandboxable for ApplyPatchRuntime { } impl Approvable for ApplyPatchRuntime { - type ApprovalKey = (); + type ApprovalKey = crate::apply_patch_approval_key::ApplyPatchFileApprovalKey; - fn approval_key(&self, _req: &ApplyPatchRequest) -> Self::ApprovalKey {} + fn approval_keys(&self, req: &ApplyPatchRequest) -> Vec { + req.approval_keys.clone() + } fn start_approval_async<'a>( &'a mut self, @@ -92,12 +99,13 @@ impl Approvable for ApplyPatchRuntime { let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); - let cwd = req.cwd.clone(); + let cwd = req.action.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 { if let Some(reason) = retry_reason { - session + return session .request_command_approval( turn, call_id, @@ -106,18 +114,29 @@ impl Approvable for ApplyPatchRuntime { Some(reason), None, ) - .await - } else if user_explicitly_approved { - ReviewDecision::ApprovedForSession - } else { - ReviewDecision::Approved + .await; } + + with_cached_approval_set(&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 }) } fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool { !matches!(policy, AskForApproval::Never) } + + 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..40be7efaaa6 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -22,7 +22,7 @@ use crate::tools::sandboxing::SandboxablePreference; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; -use crate::tools::sandboxing::with_cached_approval; +use crate::tools::sandboxing::with_cached_approval_set; use codex_protocol::protocol::ReviewDecision; use futures::future::BoxFuture; use std::path::PathBuf; @@ -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_set(&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..f56933bf3bc 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -23,7 +23,7 @@ use crate::tools::sandboxing::SandboxablePreference; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; -use crate::tools::sandboxing::with_cached_approval; +use crate::tools::sandboxing::with_cached_approval_set; use crate::unified_exec::UnifiedExecError; use crate::unified_exec::UnifiedExecSession; use crate::unified_exec::UnifiedExecSessionManager; @@ -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_set(&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..5793b1e31a3 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -49,28 +49,39 @@ impl ApprovalStore { } } -pub(crate) async fn with_cached_approval( +/// 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_set( services: &SessionServices, - key: K, + keys: Vec, fetch: F, ) -> ReviewDecision where - K: Serialize + Clone, + K: Serialize, F: FnOnce() -> Fut, Fut: Future, { - { + 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 +172,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). From 2991dcc82548a084c699290de416a82eb831cda3 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Tue, 6 Jan 2026 15:09:08 -0800 Subject: [PATCH 10/16] lint --- codex-rs/core/src/tools/sandboxing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 5793b1e31a3..14b2b185fc8 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -54,7 +54,7 @@ impl ApprovalStore { /// /// - 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. +/// so future requests touching any subset can also skip prompting. pub(crate) async fn with_cached_approval_set( services: &SessionServices, keys: Vec, From 1445ce58c8e45d01d20a52423a80cdf3ce4e9ed4 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Tue, 6 Jan 2026 15:29:58 -0800 Subject: [PATCH 11/16] add docstrings --- codex-rs/app-server-protocol/src/protocol/v2.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index af2ff82e561..d0fd9b2bd8e 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -488,13 +488,18 @@ pub struct ConfigEdit { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] 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, } @@ -502,10 +507,13 @@ pub enum CommandExecutionApprovalDecision { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub enum FileChangeApprovalDecision { + /// User approved the file changes. Accept, - /// Approve and remember the approval for the session. + /// 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, } From 267ccb03781ca47e697a73a6f95dafcd9a4eacb2 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Tue, 6 Jan 2026 15:37:06 -0800 Subject: [PATCH 12/16] rename --- codex-rs/core/src/tools/runtimes/apply_patch.rs | 4 ++-- codex-rs/core/src/tools/runtimes/shell.rs | 4 ++-- codex-rs/core/src/tools/runtimes/unified_exec.rs | 4 ++-- codex-rs/core/src/tools/sandboxing.rs | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 8d029504ca1..376b8adcdcd 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -18,7 +18,7 @@ use crate::tools::sandboxing::SandboxablePreference; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; -use crate::tools::sandboxing::with_cached_approval_set; +use crate::tools::sandboxing::with_cached_approval; use codex_apply_patch::ApplyPatchAction; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::FileChange; @@ -117,7 +117,7 @@ impl Approvable for ApplyPatchRuntime { .await; } - with_cached_approval_set(&session.services, approval_keys, || async move { + with_cached_approval(&session.services, approval_keys, || async move { let rx_approve = session .request_patch_approval(turn, call_id, changes, None, None) .await; diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 40be7efaaa6..49052bc06b9 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -22,7 +22,7 @@ use crate::tools::sandboxing::SandboxablePreference; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; -use crate::tools::sandboxing::with_cached_approval_set; +use crate::tools::sandboxing::with_cached_approval; use codex_protocol::protocol::ReviewDecision; use futures::future::BoxFuture; use std::path::PathBuf; @@ -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_set(&session.services, keys, 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 f56933bf3bc..de47ad62a81 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -23,7 +23,7 @@ use crate::tools::sandboxing::SandboxablePreference; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; -use crate::tools::sandboxing::with_cached_approval_set; +use crate::tools::sandboxing::with_cached_approval; use crate::unified_exec::UnifiedExecError; use crate::unified_exec::UnifiedExecSession; use crate::unified_exec::UnifiedExecSessionManager; @@ -116,7 +116,7 @@ impl Approvable for UnifiedExecRuntime<'_> { .clone() .or_else(|| req.justification.clone()); Box::pin(async move { - with_cached_approval_set(&session.services, keys, || 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 14b2b185fc8..980d7547bbf 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -55,7 +55,7 @@ impl ApprovalStore { /// - 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_set( +pub(crate) async fn with_cached_approval( services: &SessionServices, keys: Vec, fetch: F, From 0d95657705fc83301177cb1d380a0537daf17f7d Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Tue, 6 Jan 2026 15:45:54 -0800 Subject: [PATCH 13/16] change tui shortcut from s to a --- codex-rs/tui/src/bottom_pane/approval_overlay.rs | 2 +- .../codex_tui__chatwidget__tests__approval_modal_patch.snap | 2 +- codex-rs/tui2/src/bottom_pane/approval_overlay.rs | 2 +- .../codex_tui2__chatwidget__tests__approval_modal_patch.snap | 2 +- .../codex_tui__chatwidget__tests__approval_modal_patch.snap | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 444f1487608..0d6706b29e3 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -498,7 +498,7 @@ fn patch_options() -> Vec { 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('s'))], + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], }, ApprovalOption { label: "No, and tell Codex what to do differently".to_string(), 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 51296fb7a29..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 @@ -14,7 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. Yes, and don't ask again for these files (s) + 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 444f1487608..0d6706b29e3 100644 --- a/codex-rs/tui2/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui2/src/bottom_pane/approval_overlay.rs @@ -498,7 +498,7 @@ fn patch_options() -> Vec { 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('s'))], + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], }, ApprovalOption { label: "No, and tell Codex what to do differently".to_string(), 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 d261a8abe85..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 @@ -14,7 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. Yes, and don't ask again for these files (s) + 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 51296fb7a29..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 @@ -14,7 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. Yes, and don't ask again for these files (s) + 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 From f1afe46d0b68da590d51ecde14e39f430ace87db Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Tue, 6 Jan 2026 15:53:11 -0800 Subject: [PATCH 14/16] update --- codex-rs/core/src/tools/runtimes/apply_patch.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 376b8adcdcd..065024cefda 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -5,6 +5,7 @@ //! `codex --codex-run-as-apply-patch`, and runs under the current //! `SandboxAttempt` with a minimal environment. use crate::CODEX_APPLY_PATCH_ARG1; +use crate::apply_patch_approval_key::ApplyPatchFileApprovalKey; use crate::exec::ExecToolCallOutput; use crate::sandboxing::CommandSpec; use crate::sandboxing::SandboxPermissions; @@ -30,7 +31,7 @@ use std::path::PathBuf; #[derive(Debug)] pub struct ApplyPatchRequest { pub action: ApplyPatchAction, - pub approval_keys: Vec, + pub approval_keys: Vec, pub changes: std::collections::HashMap, pub exec_approval_requirement: ExecApprovalRequirement, pub timeout_ms: Option, @@ -85,7 +86,7 @@ impl Sandboxable for ApplyPatchRuntime { } impl Approvable for ApplyPatchRuntime { - type ApprovalKey = crate::apply_patch_approval_key::ApplyPatchFileApprovalKey; + type ApprovalKey = ApplyPatchFileApprovalKey; fn approval_keys(&self, req: &ApplyPatchRequest) -> Vec { req.approval_keys.clone() From df967408c333ffc9f55a9ee6d07ec7ef97bd859c Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Wed, 7 Jan 2026 10:55:57 -0800 Subject: [PATCH 15/16] pr feedback --- codex-rs/core/src/apply_patch_approval_key.rs | 79 ------------------- codex-rs/core/src/lib.rs | 1 - .../core/src/tools/handlers/apply_patch.rs | 68 ++++++++++++++-- .../core/src/tools/runtimes/apply_patch.rs | 12 ++- codex-rs/core/src/tools/sandboxing.rs | 5 ++ 5 files changed, 76 insertions(+), 89 deletions(-) delete mode 100644 codex-rs/core/src/apply_patch_approval_key.rs diff --git a/codex-rs/core/src/apply_patch_approval_key.rs b/codex-rs/core/src/apply_patch_approval_key.rs deleted file mode 100644 index 60e47a07a69..00000000000 --- a/codex-rs/core/src/apply_patch_approval_key.rs +++ /dev/null @@ -1,79 +0,0 @@ -use codex_apply_patch::ApplyPatchAction; -use codex_apply_patch::ApplyPatchFileChange; -use codex_utils_absolute_path::AbsolutePathBuf; -use serde::Serialize; -use std::path::Path; - -#[derive(Serialize, Clone, Debug, Eq, PartialEq, Hash)] -pub(crate) struct ApplyPatchFileApprovalKey { - kind: &'static str, - path: AbsolutePathBuf, -} - -impl ApplyPatchFileApprovalKey { - fn new(path: AbsolutePathBuf) -> Self { - Self { - kind: "applyPatchFile", - path, - } - } -} - -pub(crate) fn approval_keys_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) = approval_key_for_path(cwd, path) { - keys.push(key); - } - - if let ApplyPatchFileChange::Update { move_path, .. } = change - && let Some(dest) = move_path - && let Some(key) = approval_key_for_path(cwd, dest) - { - keys.push(key); - } - } - - keys -} - -fn approval_key_for_path(cwd: &Path, path: &Path) -> Option { - AbsolutePathBuf::resolve_path_against_base(path, cwd) - .ok() - .map(ApplyPatchFileApprovalKey::new) -} - -#[cfg(test)] -mod tests { - use super::*; - use codex_apply_patch::MaybeApplyPatchVerified; - 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 = approval_keys_for_action(&action); - assert_eq!(keys.len(), 2); - } -} diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 8b2a3181583..b9f2645b13e 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -7,7 +7,6 @@ pub mod api_bridge; mod apply_patch; -mod apply_patch_approval_key; pub mod auth; pub mod bash; mod client; diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 93783dcf3c8..adf76e68251 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -4,7 +4,6 @@ use std::path::Path; use crate::apply_patch; use crate::apply_patch::InternalApplyPatchInvocation; use crate::apply_patch::convert_apply_patch_to_protocol; -use crate::apply_patch_approval_key::approval_keys_for_action; use crate::client_common::tools::FreeformTool; use crate::client_common::tools::FreeformToolFormat; use crate::client_common::tools::ResponsesApiTool; @@ -27,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 { @@ -93,7 +119,7 @@ impl ToolHandler for ApplyPatchHandler { } InternalApplyPatchInvocation::DelegateToExec(apply) => { let changes = convert_apply_patch_to_protocol(&apply.action); - let approval_keys = approval_keys_for_action(&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( @@ -106,7 +132,7 @@ impl ToolHandler for ApplyPatchHandler { let req = ApplyPatchRequest { action: apply.action, - approval_keys, + file_paths, changes, exec_approval_requirement: apply.exec_approval_requirement, timeout_ms: None, @@ -189,7 +215,7 @@ pub(crate) async fn intercept_apply_patch( } InternalApplyPatchInvocation::DelegateToExec(apply) => { let changes = convert_apply_patch_to_protocol(&apply.action); - let approval_keys = approval_keys_for_action(&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()); @@ -197,7 +223,7 @@ pub(crate) async fn intercept_apply_patch( let req = ApplyPatchRequest { action: apply.action, - approval_keys, + file_paths: approval_keys, changes, exec_approval_requirement: apply.exec_approval_requirement, timeout_ms, @@ -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 065024cefda..f6c6db4a00f 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -5,7 +5,6 @@ //! `codex --codex-run-as-apply-patch`, and runs under the current //! `SandboxAttempt` with a minimal environment. use crate::CODEX_APPLY_PATCH_ARG1; -use crate::apply_patch_approval_key::ApplyPatchFileApprovalKey; use crate::exec::ExecToolCallOutput; use crate::sandboxing::CommandSpec; use crate::sandboxing::SandboxPermissions; @@ -24,6 +23,7 @@ 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; @@ -31,7 +31,7 @@ use std::path::PathBuf; #[derive(Debug)] pub struct ApplyPatchRequest { pub action: ApplyPatchAction, - pub approval_keys: Vec, + pub file_paths: Vec, pub changes: std::collections::HashMap, pub exec_approval_requirement: ExecApprovalRequirement, pub timeout_ms: Option, @@ -86,10 +86,10 @@ impl Sandboxable for ApplyPatchRuntime { } impl Approvable for ApplyPatchRuntime { - type ApprovalKey = ApplyPatchFileApprovalKey; + type ApprovalKey = AbsolutePathBuf; fn approval_keys(&self, req: &ApplyPatchRequest) -> Vec { - req.approval_keys.clone() + req.file_paths.clone() } fn start_approval_async<'a>( @@ -132,6 +132,10 @@ impl Approvable for ApplyPatchRuntime { !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, diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 980d7547bbf..82af60e3d6f 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -65,6 +65,11 @@ where 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; keys.iter() From 006190e10e1ea19968d06ebe8364cee034e158c1 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Wed, 7 Jan 2026 11:54:42 -0800 Subject: [PATCH 16/16] fix --- codex-rs/core/src/tools/runtimes/apply_patch.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index f6c6db4a00f..7b9d0dccc5d 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -100,22 +100,15 @@ impl Approvable for ApplyPatchRuntime { let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); - let cwd = req.action.cwd.clone(); let retry_reason = ctx.retry_reason.clone(); let approval_keys = self.approval_keys(req); let changes = req.changes.clone(); Box::pin(async move { if let Some(reason) = retry_reason { - return session - .request_command_approval( - turn, - call_id, - vec!["apply_patch".to_string()], - cwd, - Some(reason), - None, - ) + 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 {