remove extra store and extend approvable to work for apply_patch

Author: owenlin0

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}")),

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<ApplyPatchFileApprovalKey> {
+    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<ApplyPatchFileApprovalKey> {
+    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);
+    }
+}

codex-rs/core/src/apply_patch_approval_store.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,
         };

codex-rs/core/src/codex.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;

codex-rs/core/src/lib.rs

@@ -67,27 +67,13 @@ pub fn assess_patch_safety(
     policy: AskForApproval,
     sandbox_policy: &SandboxPolicy,
     cwd: &Path,
-    session_patch_approved: bool,
 ) -> SafetyCheck {
     if action.is_empty() {
         return SafetyCheck::Reject {
             reason: "empty patch".to_string(),
         };
     }
 
-    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.

codex-rs/core/src/safety.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<ModelsManager>,
     pub(crate) otel_manager: OtelManager,
     pub(crate) tool_approvals: Mutex<ApprovalStore>,
-    pub(crate) apply_patch_approvals: Mutex<ApplyPatchApprovalStore>,
     pub(crate) skills_manager: Arc<SkillsManager>,
     pub(crate) agent_control: AgentControl,
 }

codex-rs/core/src/state/service.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
                 };