From c2bdee094658fdbe82c15cd589f18e61b12e1997 Mon Sep 17 00:00:00 2001 From: zhao-oai Date: Mon, 8 Dec 2025 09:55:20 -0800 Subject: [PATCH] proposing execpolicy amendment when prompting due to sandbox denial (#7653) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, we only show the “don’t ask again for commands that start with…” option when a command is immediately flagged as needing approval. However, there is another case where we ask for approval: When a command is initially auto-approved to run within sandbox, but it fails to run inside sandbox, we would like to attempt to retry running outside of sandbox. This will require a prompt to the user. This PR addresses this latter case --- codex-rs/core/src/exec_policy.rs | 142 ++++++++++++++---- codex-rs/core/src/tools/runtimes/shell.rs | 3 +- .../core/src/tools/runtimes/unified_exec.rs | 3 +- codex-rs/core/src/tools/sandboxing.rs | 8 + 4 files changed, 121 insertions(+), 35 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 2d1c2efe5..6de2967c7 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -34,6 +34,13 @@ const POLICY_DIR_NAME: &str = "policy"; const POLICY_EXTENSION: &str = "codexpolicy"; const DEFAULT_POLICY_FILE: &str = "default.codexpolicy"; +fn is_policy_match(rule_match: &RuleMatch) -> bool { + match rule_match { + RuleMatch::PrefixRuleMatch { .. } => true, + RuleMatch::HeuristicsRuleMatch { .. } => false, + } +} + #[derive(Debug, Error)] pub enum ExecPolicyError { #[error("failed to read execpolicy files from {dir}: {source}")] @@ -147,49 +154,62 @@ pub(crate) async fn append_execpolicy_amendment_and_update( Ok(()) } -/// Returns a proposed execpolicy amendment only when heuristics caused -/// the prompt decision, so we can offer to apply that amendment for future runs. -/// -/// The amendment uses the first command heuristics marked as `Prompt`. If any explicit -/// execpolicy rule also prompts, we return `None` because applying the amendment would not -/// skip that policy requirement. -/// -/// Examples: +/// Derive a proposed execpolicy amendment when a command requires user approval +/// - If any execpolicy rule prompts, return None, because an amendment would not skip that policy requirement. +/// - Otherwise return the first heuristics Prompt. +/// - Examples: /// - execpolicy: empty. Command: `["python"]`. Heuristics prompt -> `Some(vec!["python"])`. /// - execpolicy: empty. Command: `["bash", "-c", "cd /some/folder && prog1 --option1 arg1 && prog2 --option2 arg2"]`. /// Parsed commands include `cd /some/folder`, `prog1 --option1 arg1`, and `prog2 --option2 arg2`. If heuristics allow `cd` but prompt /// on `prog1`, we return `Some(vec!["prog1", "--option1", "arg1"])`. /// - execpolicy: contains a `prompt for prefix ["prog2"]` rule. For the same command as above, /// we return `None` because an execpolicy prompt still applies even if we amend execpolicy to allow ["prog1", "--option1", "arg1"]. -fn proposed_execpolicy_amendment(evaluation: &Evaluation) -> Option { - if evaluation.decision != Decision::Prompt { +fn try_derive_execpolicy_amendment_for_prompt_rules( + matched_rules: &[RuleMatch], +) -> Option { + if matched_rules + .iter() + .any(|rule_match| is_policy_match(rule_match) && rule_match.decision() == Decision::Prompt) + { return None; } - let mut first_prompt_from_heuristics: Option> = None; - for rule_match in &evaluation.matched_rules { - match rule_match { - RuleMatch::HeuristicsRuleMatch { command, decision } => { - if *decision == Decision::Prompt && first_prompt_from_heuristics.is_none() { - first_prompt_from_heuristics = Some(command.clone()); - } - } - _ if rule_match.decision() == Decision::Prompt => { - return None; - } - _ => {} - } + matched_rules + .iter() + .find_map(|rule_match| match rule_match { + RuleMatch::HeuristicsRuleMatch { + command, + decision: Decision::Prompt, + } => Some(ExecPolicyAmendment::from(command.clone())), + _ => None, + }) +} + +/// - Note: we only use this amendment when the command fails to run in sandbox and codex prompts the user to run outside the sandbox +/// - The purpose of this amendment is to bypass sandbox for similar commands in the future +/// - If any execpolicy rule matches, return None, because we would already be running command outside the sandbox +fn try_derive_execpolicy_amendment_for_allow_rules( + matched_rules: &[RuleMatch], +) -> Option { + if matched_rules.iter().any(is_policy_match) { + return None; } - first_prompt_from_heuristics.map(ExecPolicyAmendment::from) + matched_rules + .iter() + .find_map(|rule_match| match rule_match { + RuleMatch::HeuristicsRuleMatch { + command, + decision: Decision::Allow, + } => Some(ExecPolicyAmendment::from(command.clone())), + _ => None, + }) } /// Only return PROMPT_REASON when an execpolicy rule drove the prompt decision. fn derive_prompt_reason(evaluation: &Evaluation) -> Option { evaluation.matched_rules.iter().find_map(|rule_match| { - if !matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. }) - && rule_match.decision() == Decision::Prompt - { + if is_policy_match(rule_match) && rule_match.decision() == Decision::Prompt { Some(PROMPT_REASON.to_string()) } else { None @@ -215,10 +235,6 @@ pub(crate) async fn create_exec_approval_requirement_for_command( }; let policy = exec_policy.read().await; let evaluation = policy.check_multiple(commands.iter(), &heuristics_fallback); - let has_policy_allow = evaluation.matched_rules.iter().any(|rule_match| { - !matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. }) - && rule_match.decision() == Decision::Allow - }); match evaluation.decision { Decision::Forbidden => ExecApprovalRequirement::Forbidden { @@ -233,7 +249,7 @@ pub(crate) async fn create_exec_approval_requirement_for_command( ExecApprovalRequirement::NeedsApproval { reason: derive_prompt_reason(&evaluation), proposed_execpolicy_amendment: if features.enabled(Feature::ExecPolicy) { - proposed_execpolicy_amendment(&evaluation) + try_derive_execpolicy_amendment_for_prompt_rules(&evaluation.matched_rules) } else { None }, @@ -241,7 +257,15 @@ pub(crate) async fn create_exec_approval_requirement_for_command( } } Decision::Allow => ExecApprovalRequirement::Skip { - bypass_sandbox: has_policy_allow, + // Bypass sandbox if execpolicy allows the command + bypass_sandbox: evaluation.matched_rules.iter().any(|rule_match| { + is_policy_match(rule_match) && rule_match.decision() == Decision::Allow + }), + proposed_execpolicy_amendment: if features.enabled(Feature::ExecPolicy) { + try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules) + } else { + None + }, }, } } @@ -730,4 +754,56 @@ prefix_rule(pattern=["rm"], decision="forbidden") } ); } + + #[tokio::test] + async fn proposed_execpolicy_amendment_is_present_when_heuristics_allow() { + let command = vec!["echo".to_string(), "safe".to_string()]; + + let requirement = create_exec_approval_requirement_for_command( + &Arc::new(RwLock::new(Policy::empty())), + &Features::with_defaults(), + &command, + AskForApproval::OnRequest, + &SandboxPolicy::ReadOnly, + SandboxPermissions::UseDefault, + ) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)), + } + ); + } + + #[tokio::test] + async fn proposed_execpolicy_amendment_is_suppressed_when_policy_matches_allow() { + let policy_src = r#"prefix_rule(pattern=["echo"], decision="allow")"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.codexpolicy", policy_src) + .expect("parse policy"); + let policy = Arc::new(RwLock::new(parser.build())); + let command = vec!["echo".to_string(), "safe".to_string()]; + + let requirement = create_exec_approval_requirement_for_command( + &policy, + &Features::with_defaults(), + &command, + AskForApproval::OnRequest, + &SandboxPolicy::ReadOnly, + SandboxPermissions::UseDefault, + ) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + } + ); + } } diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 2af095ee9..50b6a6785 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -133,7 +133,8 @@ impl Approvable for ShellRuntime { || matches!( req.exec_approval_requirement, ExecApprovalRequirement::Skip { - bypass_sandbox: true + bypass_sandbox: true, + .. } ) { diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 4c1cbb83e..d21e6de1e 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -154,7 +154,8 @@ impl Approvable for UnifiedExecRuntime<'_> { || matches!( req.exec_approval_requirement, ExecApprovalRequirement::Skip { - bypass_sandbox: true + bypass_sandbox: true, + .. } ) { diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 94c81043c..5e6969692 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -95,6 +95,9 @@ pub(crate) enum ExecApprovalRequirement { /// The first attempt should skip sandboxing (e.g., when explicitly /// greenlit by policy). bypass_sandbox: bool, + /// Proposed execpolicy amendment to skip future approvals for similar commands + /// Only applies if the command fails to run in sandbox and codex prompts the user to run outside the sandbox. + proposed_execpolicy_amendment: Option, }, /// Approval required for this tool call. NeedsApproval { @@ -114,6 +117,10 @@ impl ExecApprovalRequirement { proposed_execpolicy_amendment: Some(prefix), .. } => Some(prefix), + Self::Skip { + proposed_execpolicy_amendment: Some(prefix), + .. + } => Some(prefix), _ => None, } } @@ -140,6 +147,7 @@ pub(crate) fn default_exec_approval_requirement( } else { ExecApprovalRequirement::Skip { bypass_sandbox: false, + proposed_execpolicy_amendment: None, } } }