diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index b8c19f0bc1..2127f86c90 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -3,11 +3,13 @@ use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use crate::command_safety::is_dangerous_command::requires_initial_appoval; use codex_execpolicy2::Decision; use codex_execpolicy2::Evaluation; use codex_execpolicy2::Policy; use codex_execpolicy2::PolicyParser; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::SandboxPolicy; use thiserror::Error; use crate::bash::parse_shell_lc_plain_commands; @@ -133,12 +135,38 @@ pub(crate) fn evaluate_with_policy( } } +pub(crate) fn approval_requirement_for_command( + policy: Option<&Policy>, + command: &[String], + approval_policy: AskForApproval, + sandbox_policy: &SandboxPolicy, + with_escalated_permissions: bool, +) -> ApprovalRequirement { + if let Some(policy) = policy + && let Some(requirement) = evaluate_with_policy(policy, command, approval_policy) + { + return requirement; + } + + if requires_initial_appoval( + approval_policy, + sandbox_policy, + command, + with_escalated_permissions, + ) { + ApprovalRequirement::NeedsApproval { reason: None } + } else { + ApprovalRequirement::Skip + } +} + #[cfg(test)] mod tests { use super::*; use crate::features::Feature; use crate::features::Features; use codex_protocol::protocol::AskForApproval; + use codex_protocol::protocol::SandboxPolicy; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -192,4 +220,48 @@ prefix_rule(pattern=["rm"], decision="forbidden") } ); } + + #[test] + fn approval_requirement_prefers_execpolicy_match() { + let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.codexpolicy", policy_src) + .expect("parse policy"); + let policy = parser.build(); + let command = vec!["rm".to_string()]; + + let requirement = approval_requirement_for_command( + Some(&policy), + &command, + AskForApproval::OnRequest, + &SandboxPolicy::DangerFullAccess, + false, + ); + + assert_eq!( + requirement, + ApprovalRequirement::NeedsApproval { + reason: Some(PROMPT_REASON.to_string()) + } + ); + } + + #[test] + fn approval_requirement_falls_back_to_heuristics() { + let command = vec!["ls".to_string()]; + + let requirement = approval_requirement_for_command( + None, + &command, + AskForApproval::UnlessTrusted, + &SandboxPolicy::ReadOnly, + false, + ); + + assert_eq!( + requirement, + ApprovalRequirement::NeedsApproval { reason: None } + ); + } } diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index e9e3cca50a..428af694e0 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -9,6 +9,7 @@ use crate::apply_patch::convert_apply_patch_to_protocol; use crate::codex::TurnContext; use crate::exec::ExecParams; use crate::exec_env::create_env; +use crate::exec_policy::approval_requirement_for_command; use crate::function_tool::FunctionCallError; use crate::is_safe_command::is_known_safe_command; use crate::protocol::ExecCommandSource; @@ -24,6 +25,7 @@ use crate::tools::runtimes::apply_patch::ApplyPatchRequest; use crate::tools::runtimes::apply_patch::ApplyPatchRuntime; use crate::tools::runtimes::shell::ShellRequest; use crate::tools::runtimes::shell::ShellRuntime; +use crate::tools::sandboxing::ApprovalRequirement; use crate::tools::sandboxing::ToolCtx; pub struct ShellHandler; @@ -303,10 +305,16 @@ impl ShellHandler { env: exec_params.env.clone(), with_escalated_permissions: exec_params.with_escalated_permissions, justification: exec_params.justification.clone(), - exec_policy: if is_user_shell_command { - None + approval_requirement: if is_user_shell_command { + ApprovalRequirement::Skip } else { - turn.exec_policy_v2.clone() + approval_requirement_for_command( + turn.exec_policy_v2.as_deref(), + &exec_params.command, + turn.approval_policy, + &turn.sandbox_policy, + exec_params.with_escalated_permissions.unwrap_or(false), + ) }, }; let mut orchestrator = ToolOrchestrator::new(); diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 0b2efe1963..7e8e152f67 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -17,6 +17,7 @@ use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; +use crate::tools::sandboxing::default_approval_requirement; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; @@ -52,7 +53,10 @@ impl ToolOrchestrator { // 1) Approval let mut already_approved = false; - match tool.approval_requirement(req, approval_policy, &turn_ctx.sandbox_policy) { + let requirement = tool.approval_requirement(req).unwrap_or_else(|| { + default_approval_requirement(approval_policy, &turn_ctx.sandbox_policy) + }); + match requirement { ApprovalRequirement::Skip => { otel.tool_decision(otel_tn, otel_ci, ReviewDecision::Approved, otel_cfg); } diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index c274f135dc..d71c4498e6 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -4,10 +4,7 @@ Runtime: shell Executes shell requests under the orchestrator: asks for approval when needed, builds a CommandSpec, and runs it under the current SandboxAttempt. */ -use crate::command_safety::is_dangerous_command::requires_initial_appoval; use crate::exec::ExecToolCallOutput; -use crate::exec_policy::evaluate_with_policy; -use crate::protocol::SandboxPolicy; use crate::sandboxing::execute_env; use crate::tools::runtimes::build_command_spec; use crate::tools::sandboxing::Approvable; @@ -22,12 +19,9 @@ use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::with_cached_approval; -use codex_execpolicy2::Policy as ExecPolicyV2; -use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; use futures::future::BoxFuture; use std::path::PathBuf; -use std::sync::Arc; #[derive(Clone, Debug)] pub struct ShellRequest { @@ -37,7 +31,7 @@ pub struct ShellRequest { pub env: std::collections::HashMap, pub with_escalated_permissions: Option, pub justification: Option, - pub exec_policy: Option>, + pub approval_requirement: ApprovalRequirement, } impl ProvidesSandboxRetryData for ShellRequest { @@ -119,26 +113,8 @@ impl Approvable for ShellRuntime { }) } - fn approval_requirement( - &self, - req: &ShellRequest, - policy: AskForApproval, - sandbox_policy: &SandboxPolicy, - ) -> ApprovalRequirement { - if let Some(exec_policy) = &req.exec_policy - && let Some(requirement) = evaluate_with_policy(exec_policy, &req.command, policy) - { - requirement - } else if requires_initial_appoval( - policy, - sandbox_policy, - &req.command, - req.with_escalated_permissions.unwrap_or(false), - ) { - ApprovalRequirement::NeedsApproval { reason: None } - } else { - ApprovalRequirement::Skip - } + fn approval_requirement(&self, req: &ShellRequest) -> Option { + Some(req.approval_requirement.clone()) } fn wants_escalated_first_attempt(&self, req: &ShellRequest) -> bool { @@ -170,85 +146,3 @@ impl ToolRuntime for ShellRuntime { Ok(out) } } - -#[cfg(test)] -mod tests { - use super::*; - use codex_execpolicy2::PolicyParser; - use pretty_assertions::assert_eq; - use std::collections::HashMap; - - fn parse_policy(src: &str) -> Arc { - let mut parser = PolicyParser::new(); - parser - .parse("test.codexpolicy", src) - .expect("parse execpolicy2 file"); - Arc::new(parser.build()) - } - - fn shell_request(command: &[&str], exec_policy: Option>) -> ShellRequest { - ShellRequest { - command: command.iter().map(ToString::to_string).collect(), - cwd: PathBuf::from("."), - timeout_ms: None, - env: HashMap::new(), - with_escalated_permissions: None, - justification: None, - exec_policy, - } - } - - #[test] - fn prompt_decision_requires_approval() { - let policy = parse_policy(r#"prefix_rule(pattern=["echo"], decision="prompt")"#); - let req = shell_request(&["echo", "hi"], Some(policy)); - let runtime = ShellRuntime::new(); - - let requirement = runtime.approval_requirement( - &req, - AskForApproval::OnRequest, - &SandboxPolicy::DangerFullAccess, - ); - - assert_eq!( - requirement, - ApprovalRequirement::NeedsApproval { - reason: Some("execpolicy requires approval for this command".to_string()) - } - ); - } - - #[test] - fn prompt_blocked_when_approval_disabled() { - let policy = parse_policy(r#"prefix_rule(pattern=["echo"], decision="prompt")"#); - let req = shell_request(&["echo", "hi"], Some(policy)); - let runtime = ShellRuntime::new(); - - let requirement = runtime.approval_requirement( - &req, - AskForApproval::Never, - &SandboxPolicy::DangerFullAccess, - ); - - assert_eq!( - requirement, - ApprovalRequirement::Forbidden { - reason: "execpolicy requires approval for this command".to_string() - } - ); - } - - #[test] - fn user_shell_commands_skip_execpolicy() { - let req = shell_request(&["echo", "hi"], None); - let runtime = ShellRuntime::new(); - - let requirement = runtime.approval_requirement( - &req, - AskForApproval::OnRequest, - &SandboxPolicy::DangerFullAccess, - ); - - assert_eq!(requirement, ApprovalRequirement::Skip); - } -} diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index c1f5d71da3..5b18476bfc 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -1,5 +1,3 @@ -use crate::command_safety::is_dangerous_command::requires_initial_appoval; -use crate::exec_policy::evaluate_with_policy; /* Runtime: unified exec @@ -24,23 +22,19 @@ use crate::tools::sandboxing::with_cached_approval; use crate::unified_exec::UnifiedExecError; use crate::unified_exec::UnifiedExecSession; use crate::unified_exec::UnifiedExecSessionManager; -use codex_execpolicy2::Policy as ExecPolicyV2; -use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; -use codex_protocol::protocol::SandboxPolicy; use futures::future::BoxFuture; use std::collections::HashMap; use std::path::PathBuf; -use std::sync::Arc; #[derive(Clone, Debug)] pub struct UnifiedExecRequest { pub command: Vec, pub cwd: PathBuf, pub env: HashMap, - pub exec_policy: Option>, pub with_escalated_permissions: Option, pub justification: Option, + pub approval_requirement: ApprovalRequirement, } impl ProvidesSandboxRetryData for UnifiedExecRequest { @@ -68,17 +62,17 @@ impl UnifiedExecRequest { command: Vec, cwd: PathBuf, env: HashMap, - exec_policy: Option>, with_escalated_permissions: Option, justification: Option, + approval_requirement: ApprovalRequirement, ) -> Self { Self { command, cwd, env, - exec_policy, with_escalated_permissions, justification, + approval_requirement, } } } @@ -136,26 +130,8 @@ impl Approvable for UnifiedExecRuntime<'_> { }) } - fn approval_requirement( - &self, - req: &UnifiedExecRequest, - policy: AskForApproval, - sandbox_policy: &SandboxPolicy, - ) -> ApprovalRequirement { - if let Some(exec_policy) = &req.exec_policy - && let Some(requirement) = evaluate_with_policy(exec_policy, &req.command, policy) - { - requirement - } else if requires_initial_appoval( - policy, - sandbox_policy, - &req.command, - req.with_escalated_permissions.unwrap_or(false), - ) { - ApprovalRequirement::NeedsApproval { reason: None } - } else { - ApprovalRequirement::Skip - } + fn approval_requirement(&self, req: &UnifiedExecRequest) -> Option { + Some(req.approval_requirement.clone()) } fn wants_escalated_first_attempt(&self, req: &UnifiedExecRequest) -> bool { diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index d5a3f7bdaf..6b1738fb69 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -93,6 +93,28 @@ pub(crate) enum ApprovalRequirement { Forbidden { reason: String }, } +/// Decide whether an initial user approval should be requested before the +/// first attempt. Defaults to the orchestrator's behavior (pre‑refactor): +/// - Never, OnFailure: do not ask +/// - OnRequest: ask unless sandbox policy is DangerFullAccess +/// - UnlessTrusted: always ask +pub(crate) fn default_approval_requirement( + policy: AskForApproval, + sandbox_policy: &SandboxPolicy, +) -> ApprovalRequirement { + let needs_approval = match policy { + AskForApproval::Never | AskForApproval::OnFailure => false, + AskForApproval::OnRequest => !matches!(sandbox_policy, SandboxPolicy::DangerFullAccess), + AskForApproval::UnlessTrusted => true, + }; + + if needs_approval { + ApprovalRequirement::NeedsApproval { reason: None } + } else { + ApprovalRequirement::Skip + } +} + pub(crate) trait Approvable { type ApprovalKey: Hash + Eq + Clone + Debug + Serialize; @@ -113,28 +135,11 @@ pub(crate) trait Approvable { matches!(policy, AskForApproval::Never) } - /// Decide whether an initial user approval should be requested before the - /// first attempt. Defaults to the orchestrator's behavior (pre‑refactor): - /// - Never, OnFailure: do not ask - /// - OnRequest: ask unless sandbox policy is DangerFullAccess - /// - UnlessTrusted: always ask - fn approval_requirement( - &self, - _req: &Req, - policy: AskForApproval, - sandbox_policy: &SandboxPolicy, - ) -> ApprovalRequirement { - let needs_approval = match policy { - AskForApproval::Never | AskForApproval::OnFailure => false, - AskForApproval::OnRequest => !matches!(sandbox_policy, SandboxPolicy::DangerFullAccess), - AskForApproval::UnlessTrusted => true, - }; - - if needs_approval { - ApprovalRequirement::NeedsApproval { reason: None } - } else { - ApprovalRequirement::Skip - } + /// Override the default approval requirement. Return `Some(_)` to specify + /// a custom requirement, or `None` to fall back to + /// policy-based default. + fn approval_requirement(&self, _req: &Req) -> Option { + None } /// Decide we can request an approval for no-sandbox execution. diff --git a/codex-rs/core/src/unified_exec/session_manager.rs b/codex-rs/core/src/unified_exec/session_manager.rs index f30d8187ea..22d793f411 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -11,6 +11,7 @@ use crate::codex::TurnContext; use crate::exec::ExecToolCallOutput; use crate::exec::StreamOutput; use crate::exec_env::create_env; +use crate::exec_policy::approval_requirement_for_command; use crate::protocol::BackgroundEventEvent; use crate::protocol::EventMsg; use crate::protocol::ExecCommandSource; @@ -442,9 +443,15 @@ impl UnifiedExecSessionManager { command.to_vec(), cwd, create_env(&context.turn.shell_environment_policy), - context.turn.exec_policy_v2.clone(), with_escalated_permissions, justification, + approval_requirement_for_command( + context.turn.exec_policy_v2.as_deref(), + command, + context.turn.approval_policy, + &context.turn.sandbox_policy, + with_escalated_permissions.unwrap_or(false), + ), ); let tool_ctx = ToolCtx { session: context.session.as_ref(),