Merge 1420d2458c into sapling-pr-archive-bolinfest

codex-rs/common/src/approval_mode_cli_arg.rs

@@ -26,7 +26,7 @@ pub enum ApprovalModeCliArg {
 impl From<ApprovalModeCliArg> for AskForApproval {
     fn from(value: ApprovalModeCliArg) -> Self {
         match value {
-            ApprovalModeCliArg::Untrusted => AskForApproval::UnlessAllowListed,
+            ApprovalModeCliArg::Untrusted => AskForApproval::UnlessTrusted,
             ApprovalModeCliArg::OnFailure => AskForApproval::OnFailure,
             ApprovalModeCliArg::Never => AskForApproval::Never,
         }

codex-rs/core/src/codex.rs

@@ -1371,7 +1371,7 @@ async fn handle_container_exec_with_params(
             }
         }
         Err(CodexErr::Sandbox(error)) => {
-            handle_sanbox_error(error, sandbox_type, params, sess, sub_id, call_id).await
+            handle_sandbox_error(error, sandbox_type, params, sess, sub_id, call_id).await
         }
         Err(e) => {
             // Handle non-sandbox errors
@@ -1386,7 +1386,7 @@ async fn handle_container_exec_with_params(
     }
 }
 
-async fn handle_sanbox_error(
+async fn handle_sandbox_error(
     error: SandboxErr,
     sandbox_type: SandboxType,
     params: ExecParams,
@@ -1408,7 +1408,14 @@ async fn handle_sanbox_error(
         };
     }
 
-    // Ask the user to retry without sandbox
+    // Note that when `error` is `SandboxErr::Denied`, it could be a false
+    // positive. That is, it may have exited with a non-zero exit code, not
+    // because the sandbox denied it, but because that is its expected behavior,
+    // i.e., a grep command that did not match anything. Ideally we would
+    // include additional metdata on the command to indicate whether non-zero
+    // exit codes merit a retry.
+
+    // For now, we categorically ask the user to retry without sandbox.
     sess.notify_background_event(&sub_id, format!("Execution failed: {error}"))
         .await;
 

codex-rs/core/src/config.rs

@@ -731,7 +731,7 @@ disable_response_storage = true
             model: "gpt-3.5-turbo".to_string(),
             model_provider_id: "openai-chat-completions".to_string(),
             model_provider: fixture.openai_chat_completions_provider.clone(),
-            approval_policy: AskForApproval::UnlessAllowListed,
+            approval_policy: AskForApproval::UnlessTrusted,
             sandbox_policy: SandboxPolicy::new_read_only_policy(),
             shell_environment_policy: ShellEnvironmentPolicy::default(),
             disable_response_storage: false,

codex-rs/core/src/protocol.rs

@@ -120,7 +120,7 @@ pub enum AskForApproval {
     /// Everything else will ask the user to approve.
     #[default]
     #[serde(rename = "untrusted")]
-    UnlessAllowListed,
+    UnlessTrusted,
 
     /// *All* commands are auto‑approved, but they are expected to run inside a
     /// sandbox where network access is disabled and writes are confined to a
@@ -231,12 +231,6 @@ impl SandboxPolicy {
             }
         }
     }
-
-    // TODO(mbolin): This conflates sandbox policy and approval policy and
-    // should go away.
-    pub fn is_unrestricted(&self) -> bool {
-        matches!(self, SandboxPolicy::DangerFullAccess)
-    }
 }
 
 /// User input

codex-rs/core/src/safety.rs

@@ -36,7 +36,7 @@ pub fn assess_patch_safety(
         }
         // TODO(ragona): I'm not sure this is actually correct? I believe in this case
         // we want to continue to the writable paths check before asking the user.
-        AskForApproval::UnlessAllowListed => {
+        AskForApproval::UnlessTrusted => {
             return SafetyCheck::AskUser;
         }
     }
@@ -63,40 +63,87 @@ pub fn assess_patch_safety(
     }
 }
 
+/// If `sandbox_policy` is not `DangerFullAccess` and a sandbox is available for
+/// the current platform, then even a _trusted_ command will be run inside the
+/// sandbox. To run a command _without_ a sandbox, one of the following must
+/// be true:
+///
+/// - `DangerFullAccess` was specified
+/// - the user has explicitly approved the command
+/// - the command is "trusted," but there is no sandbox available
 pub fn assess_command_safety(
     command: &[String],
     approval_policy: AskForApproval,
     sandbox_policy: &SandboxPolicy,
     approved: &HashSet<Vec<String>>,
 ) -> SafetyCheck {
-    let approve_without_sandbox = || SafetyCheck::AutoApprove {
-        sandbox_type: SandboxType::None,
-    };
+    use AskForApproval::*;
+    use SandboxPolicy::*;
 
-    // Previously approved or allow-listed commands
-    // All approval modes allow these commands to continue without sandboxing
-    if is_known_safe_command(command) || approved.contains(command) {
-        // TODO(ragona): I think we should consider running even these inside the sandbox, but it's
-        // a change in behavior so I'm keeping it at parity with upstream for now.
-        return approve_without_sandbox();
-    }
+    // A command is "trusted" because either:
+    // - it belongs to a set of commands we consider "safe" by default, or
+    // - the user has explicitly approved the command for this session
+    let is_trusted_command = is_known_safe_command(command) || approved.contains(command);
 
-    // Command was not known-safe or allow-listed
-    if sandbox_policy.is_unrestricted() {
-        approve_without_sandbox()
-    } else {
-        match get_platform_sandbox() {
-            // We have a sandbox, so we can approve the command in all modes
-            Some(sandbox_type) => SafetyCheck::AutoApprove { sandbox_type },
-            None => {
-                // We do not have a sandbox, so we need to consider the approval policy
-                match approval_policy {
-                    // Never is our "non-interactive" mode; it must automatically reject
-                    AskForApproval::Never => SafetyCheck::Reject {
-                        reason: "auto-rejected by user approval settings".to_string(),
-                    },
-                    // Otherwise, we ask the user for approval
-                    _ => SafetyCheck::AskUser,
+    match (approval_policy, sandbox_policy) {
+        (UnlessTrusted, DangerFullAccess) => {
+            if is_trusted_command {
+                SafetyCheck::AutoApprove {
+                    sandbox_type: SandboxType::None,
+                }
+            } else {
+                // Even though the user has opted into DangerFullAccess, they
+                // also requested that we ask for approval for untrusted
+                // commands.
+                SafetyCheck::AskUser
+            }
+        }
+        (UnlessTrusted, ReadOnly) | (UnlessTrusted, WorkspaceWrite { .. }) => {
+            if is_trusted_command {
+                // Currently, whether a command is "trusted" is a simple boolean,
+                // but we should expand this definition to indicate whether it
+                // should be run inside a sandbox or not.
+                match get_platform_sandbox() {
+                    Some(sandbox_type) => SafetyCheck::AutoApprove { sandbox_type },
+                    // The user has requested a sandboxed environment, but we do
+                    // not have one available. If the user explicitly approves
+                    // the command, we interpret that as justification to run it
+                    // without a sandbox.
+                    None => SafetyCheck::AskUser,
+                }
+            } else {
+                SafetyCheck::AskUser
+            }
+        }
+        (OnFailure, DangerFullAccess) | (Never, DangerFullAccess) => SafetyCheck::AutoApprove {
+            sandbox_type: SandboxType::None,
+        },
+        (Never, ReadOnly)
+        | (Never, WorkspaceWrite { .. })
+        | (OnFailure, ReadOnly)
+        | (OnFailure, WorkspaceWrite { .. }) => {
+            match get_platform_sandbox() {
+                Some(sandbox_type) => SafetyCheck::AutoApprove { sandbox_type },
+                None => {
+                    if is_trusted_command {
+                        // If the command is trusted, run it even though we do
+                        // not have a sandbox available.
+                        SafetyCheck::AutoApprove {
+                            sandbox_type: SandboxType::None,
+                        }
+                    } else if matches!(approval_policy, OnFailure) {
+                        // If the command is not trusted, even though the user
+                        // has requested to only ask for approval on failure, we
+                        // will ask the user because no sandbox is available.
+                        SafetyCheck::AskUser
+                    } else {
+                        // We are in non-interactive mode and lack approval, so
+                        // all we can do is reject the command.
+                        SafetyCheck::Reject {
+                            reason: "auto-rejected because command is not on trusted list"
+                                .to_string(),
+                        }
+                    }
                 }
             }
         }

codex-rs/mcp-server/src/codex_tool_config.rs

@@ -55,7 +55,7 @@ pub(crate) enum CodexToolCallApprovalPolicy {
 impl From<CodexToolCallApprovalPolicy> for AskForApproval {
     fn from(value: CodexToolCallApprovalPolicy) -> Self {
         match value {
-            CodexToolCallApprovalPolicy::Untrusted => AskForApproval::UnlessAllowListed,
+            CodexToolCallApprovalPolicy::Untrusted => AskForApproval::UnlessTrusted,
             CodexToolCallApprovalPolicy::OnFailure => AskForApproval::OnFailure,
             CodexToolCallApprovalPolicy::Never => AskForApproval::Never,
         }