From 4563dd0a1762ef5472234b660b203bf5c4a2ee52 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Thu, 20 Nov 2025 16:46:52 -0500 Subject: [PATCH 1/3] Add approval allow-prefix flow in core and tui Add explicit prefix-approval decision and wire it through execpolicy/UI snapshots update doc mutating in memory policy instead of reloading using RW locks clippy refactor: adding allow_prefix into ApprovedAllowPrefix fmt do not send allow_prefix if execpolicy is disabled moving args around cleanup exec_policy getters undo diff fixing rw lock bug causing tui to hang updating phrasing integration test . fix compile fix flaky test fix compile error running test with single thread fixup allow_prefix_if_applicable fix formatting fix approvals test only cloning when needed docs add docstring fix rebase bug fixing rebase issues Revert "fixing rebase issues" This reverts commit 79ce7e1f2fc0378c2c0b362408e2e544566540fd. fix rebase errors --- .../app-server/src/bespoke_event_handling.rs | 1 + codex-rs/core/src/apply_patch.rs | 4 +- codex-rs/core/src/codex.rs | 65 +++- codex-rs/core/src/codex_delegate.rs | 1 + codex-rs/core/src/exec_policy.rs | 342 +++++++++++++++--- codex-rs/core/src/tools/handlers/shell.rs | 3 +- codex-rs/core/src/tools/orchestrator.rs | 16 +- .../core/src/tools/runtimes/apply_patch.rs | 1 + codex-rs/core/src/tools/runtimes/shell.rs | 10 +- .../core/src/tools/runtimes/unified_exec.rs | 10 +- codex-rs/core/src/tools/sandboxing.rs | 23 +- .../core/src/unified_exec/session_manager.rs | 2 + codex-rs/core/tests/suite/approvals.rs | 139 ++++++- codex-rs/mcp-server/src/codex_tool_runner.rs | 1 + codex-rs/otel/src/otel_event_manager.rs | 4 +- codex-rs/protocol/src/approvals.rs | 4 + codex-rs/protocol/src/protocol.rs | 8 +- .../tui/src/bottom_pane/approval_overlay.rs | 87 ++++- codex-rs/tui/src/bottom_pane/mod.rs | 1 + codex-rs/tui/src/chatwidget.rs | 1 + ...hatwidget__tests__approval_modal_exec.snap | 5 +- ..._tests__approval_modal_exec_no_reason.snap | 5 +- ...dget__tests__exec_approval_modal_exec.snap | 6 +- ...sts__status_widget_and_approval_modal.snap | 6 +- codex-rs/tui/src/chatwidget/tests.rs | 6 + codex-rs/tui/src/history_cell.rs | 13 + docs/config.md | 2 +- 27 files changed, 659 insertions(+), 107 deletions(-) diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index df4cdb8980..6d03a9f5db 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -179,6 +179,7 @@ pub(crate) async fn apply_bespoke_event_handling( cwd, reason, risk, + allow_prefix: _allow_prefix, parsed_cmd, }) => match api_version { ApiVersion::V1 => { diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index dffe94be61..5cf0ecee70 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -70,7 +70,9 @@ pub(crate) async fn apply_patch( ) .await; match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { + ReviewDecision::Approved + | ReviewDecision::ApprovedAllowPrefix { .. } + | ReviewDecision::ApprovedForSession => { InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { action, user_explicitly_approved_this_action: true, diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index ba7c69eb95..7461bf41fb 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -73,6 +73,7 @@ use crate::error::CodexErr; use crate::error::Result as CodexResult; #[cfg(test)] use crate::exec::StreamOutput; +use crate::exec_policy::ExecPolicyUpdateError; use crate::mcp::auth::compute_auth_statuses; use crate::mcp_connection_manager::McpConnectionManager; use crate::openai_model_info::get_model_info; @@ -293,7 +294,7 @@ pub(crate) struct TurnContext { pub(crate) final_output_json_schema: Option, pub(crate) codex_linux_sandbox_exe: Option, pub(crate) tool_call_gate: Arc, - pub(crate) exec_policy: Arc, + pub(crate) exec_policy: Arc>, pub(crate) truncation_policy: TruncationPolicy, } @@ -349,7 +350,7 @@ pub(crate) struct SessionConfiguration { cwd: PathBuf, /// Execpolicy policy, applied only when enabled by feature flag. - exec_policy: Arc, + exec_policy: Arc>, // TODO(pakrym): Remove config from here original_config_do_not_use: Arc, @@ -870,11 +871,44 @@ impl Session { .await } + pub(crate) async fn persist_command_allow_prefix( + &self, + prefix: &[String], + ) -> Result<(), ExecPolicyUpdateError> { + let features = self.features.clone(); + let (codex_home, current_policy) = { + let state = self.state.lock().await; + ( + state + .session_configuration + .original_config_do_not_use + .codex_home + .clone(), + state.session_configuration.exec_policy.clone(), + ) + }; + + if !features.enabled(Feature::ExecPolicy) { + error!("attempted to append execpolicy rule while execpolicy feature is disabled"); + return Err(ExecPolicyUpdateError::FeatureDisabled); + } + + crate::exec_policy::append_allow_prefix_rule_and_update( + &codex_home, + ¤t_policy, + prefix, + ) + .await?; + + Ok(()) + } + /// Emit an exec approval request event and await the user's decision. /// /// The request is keyed by `sub_id`/`call_id` so matching responses are delivered /// to the correct in-flight turn. If the task is aborted, this returns the /// default `ReviewDecision` (`Denied`). + #[allow(clippy::too_many_arguments)] pub async fn request_command_approval( &self, turn_context: &TurnContext, @@ -883,6 +917,7 @@ impl Session { cwd: PathBuf, reason: Option, risk: Option, + allow_prefix: Option>, ) -> ReviewDecision { let sub_id = turn_context.sub_id.clone(); // Add the tx_approve callback to the map before sending the request. @@ -910,6 +945,7 @@ impl Session { cwd, reason, risk, + allow_prefix, parsed_cmd, }); self.send_event(turn_context, event).await; @@ -1079,6 +1115,10 @@ impl Session { self.features.enabled(feature) } + pub(crate) fn features(&self) -> Features { + self.features.clone() + } + async fn send_raw_response_items(&self, turn_context: &TurnContext, items: &[ResponseItem]) { for item in items { self.send_event( @@ -1513,6 +1553,7 @@ mod handlers { use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::ReviewRequest; use codex_protocol::protocol::TurnAbortReason; + use codex_protocol::protocol::WarningEvent; use codex_protocol::user_input::UserInput; use codex_rmcp_client::ElicitationAction; @@ -1627,7 +1668,21 @@ mod handlers { } } + /// Propagate a user's exec approval decision to the session + /// Also optionally whitelists command in execpolicy pub async fn exec_approval(sess: &Arc, id: String, decision: ReviewDecision) { + if let ReviewDecision::ApprovedAllowPrefix { allow_prefix } = &decision + && let Err(err) = sess.persist_command_allow_prefix(allow_prefix).await + { + let message = format!("Failed to update execpolicy allow list: {err}"); + tracing::warn!("{message}"); + let warning = EventMsg::Warning(WarningEvent { message }); + sess.send_event_raw(Event { + id: id.clone(), + msg: warning, + }) + .await; + } match decision { ReviewDecision::Abort => { sess.interrupt_task().await; @@ -2571,7 +2626,7 @@ mod tests { sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), - exec_policy: Arc::new(ExecPolicy::empty()), + exec_policy: Arc::new(RwLock::new(ExecPolicy::empty())), session_source: SessionSource::Exec, }; @@ -2770,7 +2825,7 @@ mod tests { sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), - exec_policy: Arc::new(ExecPolicy::empty()), + exec_policy: Arc::new(RwLock::new(ExecPolicy::empty())), session_source: SessionSource::Exec, }; @@ -2851,7 +2906,7 @@ mod tests { sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), - exec_policy: Arc::new(ExecPolicy::empty()), + exec_policy: Arc::new(RwLock::new(ExecPolicy::empty())), session_source: SessionSource::Exec, }; diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index b6e4c88f30..1606855669 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -281,6 +281,7 @@ async fn handle_exec_approval( event.cwd, event.reason, event.risk, + event.allow_prefix, ); let decision = await_approval_with_cancel( approval_fut, diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 602e5d679e..44328de14a 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -4,14 +4,18 @@ use std::path::PathBuf; use std::sync::Arc; use crate::command_safety::is_dangerous_command::requires_initial_appoval; +use codex_execpolicy::AmendError; use codex_execpolicy::Decision; +use codex_execpolicy::Error as ExecPolicyRuleError; use codex_execpolicy::Evaluation; use codex_execpolicy::Policy; use codex_execpolicy::PolicyParser; +use codex_execpolicy::blocking_append_allow_prefix_rule; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; use thiserror::Error; use tokio::fs; +use tokio::sync::RwLock; use crate::bash::parse_shell_lc_plain_commands; use crate::features::Feature; @@ -23,6 +27,7 @@ const FORBIDDEN_REASON: &str = "execpolicy forbids this command"; const PROMPT_REASON: &str = "execpolicy requires approval for this command"; const POLICY_DIR_NAME: &str = "policy"; const POLICY_EXTENSION: &str = "codexpolicy"; +const DEFAULT_POLICY_FILE: &str = "default.codexpolicy"; #[derive(Debug, Error)] pub enum ExecPolicyError { @@ -45,12 +50,27 @@ pub enum ExecPolicyError { }, } +#[derive(Debug, Error)] +pub enum ExecPolicyUpdateError { + #[error("failed to update execpolicy file {path}: {source}")] + AppendRule { path: PathBuf, source: AmendError }, + + #[error("failed to update in-memory execpolicy: {source}")] + AddRule { + #[from] + source: ExecPolicyRuleError, + }, + + #[error("cannot append execpolicy rule because execpolicy feature is disabled")] + FeatureDisabled, +} + pub(crate) async fn exec_policy_for( features: &Features, codex_home: &Path, -) -> Result, ExecPolicyError> { +) -> Result>, ExecPolicyError> { if !features.enabled(Feature::ExecPolicy) { - return Ok(Arc::new(Policy::empty())); + return Ok(Arc::new(RwLock::new(Policy::empty()))); } let policy_dir = codex_home.join(POLICY_DIR_NAME); @@ -74,7 +94,7 @@ pub(crate) async fn exec_policy_for( })?; } - let policy = Arc::new(parser.build()); + let policy = Arc::new(RwLock::new(parser.build())); tracing::debug!( "loaded execpolicy from {} files in {}", policy_paths.len(), @@ -84,58 +104,102 @@ pub(crate) async fn exec_policy_for( Ok(policy) } -fn evaluate_with_policy( - policy: &Policy, - command: &[String], - approval_policy: AskForApproval, -) -> Option { - let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]); - let evaluation = policy.check_multiple(commands.iter()); +pub(crate) fn default_policy_path(codex_home: &Path) -> PathBuf { + codex_home.join(POLICY_DIR_NAME).join(DEFAULT_POLICY_FILE) +} - match evaluation { - Evaluation::Match { decision, .. } => match decision { - Decision::Forbidden => Some(ApprovalRequirement::Forbidden { - reason: FORBIDDEN_REASON.to_string(), - }), - Decision::Prompt => { - let reason = PROMPT_REASON.to_string(); - if matches!(approval_policy, AskForApproval::Never) { - Some(ApprovalRequirement::Forbidden { reason }) - } else { - Some(ApprovalRequirement::NeedsApproval { - reason: Some(reason), - }) +pub(crate) async fn append_allow_prefix_rule_and_update( + codex_home: &Path, + current_policy: &Arc>, + prefix: &[String], +) -> Result<(), ExecPolicyUpdateError> { + let policy_path = default_policy_path(codex_home); + blocking_append_allow_prefix_rule(&policy_path, prefix).map_err(|source| { + ExecPolicyUpdateError::AppendRule { + path: policy_path, + source, + } + })?; + + current_policy + .write() + .await + .add_prefix_rule(prefix, Decision::Allow)?; + + Ok(()) +} + +fn requirement_from_decision( + decision: Decision, + approval_policy: AskForApproval, +) -> ApprovalRequirement { + match decision { + Decision::Forbidden => ApprovalRequirement::Forbidden { + reason: FORBIDDEN_REASON.to_string(), + }, + Decision::Prompt => { + let reason = PROMPT_REASON.to_string(); + if matches!(approval_policy, AskForApproval::Never) { + ApprovalRequirement::Forbidden { reason } + } else { + ApprovalRequirement::NeedsApproval { + reason: Some(reason), + allow_prefix: None, } } - Decision::Allow => Some(ApprovalRequirement::Skip { - bypass_sandbox: true, - }), + } + Decision::Allow => ApprovalRequirement::Skip { + bypass_sandbox: true, }, - Evaluation::NoMatch { .. } => None, + } +} + +/// Return an allow-prefix option when a single plain command needs approval without +/// any matching policy rule. We only surface the prefix opt-in when execpolicy did +/// not already drive the decision (NoMatch) and when the command is a single +/// unrolled command (multi-part scripts shouldn’t be whitelisted via prefix) and +/// when execpolicy feature is enabled. +fn allow_prefix_if_applicable( + commands: &[Vec], + features: &Features, +) -> Option> { + if features.enabled(Feature::ExecPolicy) && commands.len() == 1 { + Some(commands[0].clone()) + } else { + None } } pub(crate) async fn create_approval_requirement_for_command( - policy: &Policy, + exec_policy: &Arc>, + features: &Features, command: &[String], approval_policy: AskForApproval, sandbox_policy: &SandboxPolicy, sandbox_permissions: SandboxPermissions, ) -> ApprovalRequirement { - if let Some(requirement) = evaluate_with_policy(policy, command, approval_policy) { - return requirement; - } + let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]); + let policy = exec_policy.read().await; + let evaluation = policy.check_multiple(commands.iter()); - if requires_initial_appoval( - approval_policy, - sandbox_policy, - command, - sandbox_permissions, - ) { - ApprovalRequirement::NeedsApproval { reason: None } - } else { - ApprovalRequirement::Skip { - bypass_sandbox: false, + match evaluation { + Evaluation::Match { decision, .. } => requirement_from_decision(decision, approval_policy), + Evaluation::NoMatch { .. } => { + if requires_initial_appoval( + approval_policy, + sandbox_policy, + command, + sandbox_permissions, + ) { + ApprovalRequirement::NeedsApproval { + reason: None, + allow_prefix: allow_prefix_if_applicable(&commands, features), + } + } else { + ApprovalRequirement::Skip { + bypass_sandbox: false, + } + } } } } @@ -195,6 +259,7 @@ mod tests { use codex_protocol::protocol::SandboxPolicy; use pretty_assertions::assert_eq; use std::fs; + use std::sync::Arc; use tempfile::tempdir; #[tokio::test] @@ -209,7 +274,7 @@ mod tests { let commands = [vec!["rm".to_string()]]; assert!(matches!( - policy.check_multiple(commands.iter()), + policy.read().await.check_multiple(commands.iter()), Evaluation::NoMatch { .. } )); assert!(!temp_dir.path().join(POLICY_DIR_NAME).exists()); @@ -243,7 +308,7 @@ mod tests { .expect("policy result"); let command = [vec!["rm".to_string()]]; assert!(matches!( - policy.check_multiple(command.iter()), + policy.read().await.check_multiple(command.iter()), Evaluation::Match { .. } )); } @@ -262,13 +327,13 @@ mod tests { .expect("policy result"); let command = [vec!["ls".to_string()]]; assert!(matches!( - policy.check_multiple(command.iter()), + policy.read().await.check_multiple(command.iter()), Evaluation::NoMatch { .. } )); } - #[test] - fn evaluates_bash_lc_inner_commands() { + #[tokio::test] + async fn evaluates_bash_lc_inner_commands() { let policy_src = r#" prefix_rule(pattern=["rm"], decision="forbidden") "#; @@ -276,7 +341,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") parser .parse("test.codexpolicy", policy_src) .expect("parse policy"); - let policy = parser.build(); + let policy = Arc::new(RwLock::new(parser.build())); let forbidden_script = vec![ "bash".to_string(), @@ -284,9 +349,15 @@ prefix_rule(pattern=["rm"], decision="forbidden") "rm -rf /tmp".to_string(), ]; - let requirement = - evaluate_with_policy(&policy, &forbidden_script, AskForApproval::OnRequest) - .expect("expected match for forbidden command"); + let requirement = create_approval_requirement_for_command( + &policy, + &Features::with_defaults(), + &forbidden_script, + AskForApproval::OnRequest, + &SandboxPolicy::DangerFullAccess, + SandboxPermissions::UseDefault, + ) + .await; assert_eq!( requirement, @@ -303,11 +374,12 @@ prefix_rule(pattern=["rm"], decision="forbidden") parser .parse("test.codexpolicy", policy_src) .expect("parse policy"); - let policy = parser.build(); + let policy = Arc::new(RwLock::new(parser.build())); let command = vec!["rm".to_string()]; let requirement = create_approval_requirement_for_command( &policy, + &Features::with_defaults(), &command, AskForApproval::OnRequest, &SandboxPolicy::DangerFullAccess, @@ -318,7 +390,8 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, ApprovalRequirement::NeedsApproval { - reason: Some(PROMPT_REASON.to_string()) + reason: Some(PROMPT_REASON.to_string()), + allow_prefix: None, } ); } @@ -330,11 +403,12 @@ prefix_rule(pattern=["rm"], decision="forbidden") parser .parse("test.codexpolicy", policy_src) .expect("parse policy"); - let policy = parser.build(); + let policy = Arc::new(RwLock::new(parser.build())); let command = vec!["rm".to_string()]; let requirement = create_approval_requirement_for_command( &policy, + &Features::with_defaults(), &command, AskForApproval::Never, &SandboxPolicy::DangerFullAccess, @@ -354,9 +428,10 @@ prefix_rule(pattern=["rm"], decision="forbidden") async fn approval_requirement_falls_back_to_heuristics() { let command = vec!["python".to_string()]; - let empty_policy = Policy::empty(); + let empty_policy = Arc::new(RwLock::new(Policy::empty())); let requirement = create_approval_requirement_for_command( &empty_policy, + &Features::with_defaults(), &command, AskForApproval::UnlessTrusted, &SandboxPolicy::ReadOnly, @@ -366,7 +441,164 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, - ApprovalRequirement::NeedsApproval { reason: None } + ApprovalRequirement::NeedsApproval { + reason: None, + allow_prefix: Some(command) + } + ); + } + + #[tokio::test] + async fn append_allow_prefix_rule_updates_policy_and_file() { + let codex_home = tempdir().expect("create temp dir"); + let current_policy = Arc::new(RwLock::new(Policy::empty())); + let prefix = vec!["echo".to_string(), "hello".to_string()]; + + append_allow_prefix_rule_and_update(codex_home.path(), ¤t_policy, &prefix) + .await + .expect("update policy"); + + let evaluation = current_policy.read().await.check(&[ + "echo".to_string(), + "hello".to_string(), + "world".to_string(), + ]); + assert!(matches!( + evaluation, + Evaluation::Match { + decision: Decision::Allow, + .. + } + )); + + let contents = fs::read_to_string(default_policy_path(codex_home.path())) + .expect("policy file should have been created"); + assert_eq!( + contents, + r#"prefix_rule(pattern=["echo", "hello"], decision="allow") +"# + ); + } + + #[tokio::test] + async fn append_allow_prefix_rule_rejects_empty_prefix() { + let codex_home = tempdir().expect("create temp dir"); + let current_policy = Arc::new(RwLock::new(Policy::empty())); + + let result = + append_allow_prefix_rule_and_update(codex_home.path(), ¤t_policy, &[]).await; + + assert!(matches!( + result, + Err(ExecPolicyUpdateError::AppendRule { + source: AmendError::EmptyPrefix, + .. + }) + )); + } + + #[tokio::test] + async fn allow_prefix_is_present_for_single_command_without_policy_match() { + let command = vec!["python".to_string()]; + + let empty_policy = Arc::new(RwLock::new(Policy::empty())); + let requirement = create_approval_requirement_for_command( + &empty_policy, + &Features::with_defaults(), + &command, + AskForApproval::UnlessTrusted, + &SandboxPolicy::ReadOnly, + SandboxPermissions::UseDefault, + ) + .await; + + assert_eq!( + requirement, + ApprovalRequirement::NeedsApproval { + reason: None, + allow_prefix: Some(command) + } + ); + } + + #[tokio::test] + async fn allow_prefix_is_disabled_when_execpolicy_feature_disabled() { + let command = vec!["python".to_string()]; + + let mut features = Features::with_defaults(); + features.disable(Feature::ExecPolicy); + + let requirement = create_approval_requirement_for_command( + &Arc::new(RwLock::new(Policy::empty())), + &features, + &command, + AskForApproval::UnlessTrusted, + &SandboxPolicy::ReadOnly, + SandboxPermissions::UseDefault, + ) + .await; + + assert_eq!( + requirement, + ApprovalRequirement::NeedsApproval { + reason: None, + allow_prefix: None, + } + ); + } + + #[tokio::test] + async fn allow_prefix_is_omitted_when_policy_prompts() { + 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 = Arc::new(RwLock::new(parser.build())); + let command = vec!["rm".to_string()]; + + let requirement = create_approval_requirement_for_command( + &policy, + &Features::with_defaults(), + &command, + AskForApproval::OnRequest, + &SandboxPolicy::DangerFullAccess, + SandboxPermissions::UseDefault, + ) + .await; + + assert_eq!( + requirement, + ApprovalRequirement::NeedsApproval { + reason: Some(PROMPT_REASON.to_string()), + allow_prefix: None, + } + ); + } + + #[tokio::test] + async fn allow_prefix_is_omitted_for_multi_command_scripts() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "python && echo ok".to_string(), + ]; + let requirement = create_approval_requirement_for_command( + &Arc::new(RwLock::new(Policy::empty())), + &Features::with_defaults(), + &command, + AskForApproval::UnlessTrusted, + &SandboxPolicy::ReadOnly, + SandboxPermissions::UseDefault, + ) + .await; + + assert_eq!( + requirement, + ApprovalRequirement::NeedsApproval { + reason: None, + allow_prefix: None, + } ); } } diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index d1b7d3144c..88b9815d5b 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -231,15 +231,16 @@ impl ShellHandler { let event_ctx = ToolEventCtx::new(session.as_ref(), turn.as_ref(), &call_id, None); emitter.begin(event_ctx).await; + let features = session.features(); let approval_requirement = create_approval_requirement_for_command( &turn.exec_policy, + &features, &exec_params.command, turn.approval_policy, &turn.sandbox_policy, SandboxPermissions::from(exec_params.with_escalated_permissions.unwrap_or(false)), ) .await; - let req = ShellRequest { command: exec_params.command.clone(), cwd: exec_params.cwd.clone(), diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index de23d510bf..7510fc6aa4 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -59,12 +59,12 @@ impl ToolOrchestrator { }); match requirement { ApprovalRequirement::Skip { .. } => { - otel.tool_decision(otel_tn, otel_ci, ReviewDecision::Approved, otel_cfg); + otel.tool_decision(otel_tn, otel_ci, &ReviewDecision::Approved, otel_cfg); } ApprovalRequirement::Forbidden { reason } => { return Err(ToolError::Rejected(reason)); } - ApprovalRequirement::NeedsApproval { reason } => { + ApprovalRequirement::NeedsApproval { reason, .. } => { let mut risk = None; if let Some(metadata) = req.sandbox_retry_data() { @@ -88,13 +88,15 @@ impl ToolOrchestrator { }; let decision = tool.start_approval_async(req, approval_ctx).await; - otel.tool_decision(otel_tn, otel_ci, decision, otel_user.clone()); + otel.tool_decision(otel_tn, otel_ci, &decision, otel_user.clone()); match decision { ReviewDecision::Denied | ReviewDecision::Abort => { return Err(ToolError::Rejected("rejected by user".to_string())); } - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => {} + ReviewDecision::Approved + | ReviewDecision::ApprovedAllowPrefix { .. } + | ReviewDecision::ApprovedForSession => {} } already_approved = true; } @@ -169,13 +171,15 @@ impl ToolOrchestrator { }; let decision = tool.start_approval_async(req, approval_ctx).await; - otel.tool_decision(otel_tn, otel_ci, decision, otel_user); + otel.tool_decision(otel_tn, otel_ci, &decision, otel_user); match decision { ReviewDecision::Denied | ReviewDecision::Abort => { return Err(ToolError::Rejected("rejected by user".to_string())); } - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => {} + ReviewDecision::Approved + | ReviewDecision::ApprovedAllowPrefix { .. } + | ReviewDecision::ApprovedForSession => {} } } diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 2334f1e712..7ef8d33767 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -127,6 +127,7 @@ impl Approvable for ApplyPatchRuntime { cwd, Some(reason), risk, + None, ) .await } else if user_explicitly_approved { diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 56c72a8278..ed5ba8a0d8 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -107,7 +107,15 @@ impl Approvable for ShellRuntime { Box::pin(async move { with_cached_approval(&session.services, key, move || async move { session - .request_command_approval(turn, call_id, command, cwd, reason, risk) + .request_command_approval( + turn, + call_id, + command, + cwd, + reason, + risk, + req.approval_requirement.allow_prefix().cloned(), + ) .await }) .await diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 0f306e6ff2..0a23695bde 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -125,7 +125,15 @@ impl Approvable for UnifiedExecRuntime<'_> { Box::pin(async move { with_cached_approval(&session.services, key, || async move { session - .request_command_approval(turn, call_id, command, cwd, reason, risk) + .request_command_approval( + turn, + call_id, + command, + cwd, + reason, + risk, + req.approval_requirement.allow_prefix().cloned(), + ) .await }) .await diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index df10db952e..8efef10e59 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -96,11 +96,27 @@ pub(crate) enum ApprovalRequirement { bypass_sandbox: bool, }, /// Approval required for this tool call - NeedsApproval { reason: Option }, + NeedsApproval { + reason: Option, + /// Prefix that can be whitelisted via execpolicy to skip future approvals for similar commands + allow_prefix: Option>, + }, /// Execution forbidden for this tool call Forbidden { reason: String }, } +impl ApprovalRequirement { + pub fn allow_prefix(&self) -> Option<&Vec> { + match self { + Self::NeedsApproval { + allow_prefix: Some(prefix), + .. + } => Some(prefix), + _ => None, + } + } +} + /// - Never, OnFailure: do not ask /// - OnRequest: ask unless sandbox policy is DangerFullAccess /// - UnlessTrusted: always ask @@ -115,7 +131,10 @@ pub(crate) fn default_approval_requirement( }; if needs_approval { - ApprovalRequirement::NeedsApproval { reason: None } + ApprovalRequirement::NeedsApproval { + reason: None, + allow_prefix: None, + } } else { ApprovalRequirement::Skip { bypass_sandbox: false, diff --git a/codex-rs/core/src/unified_exec/session_manager.rs b/codex-rs/core/src/unified_exec/session_manager.rs index d37ad4d3fc..b9b4ea8dbe 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -556,10 +556,12 @@ impl UnifiedExecSessionManager { context: &UnifiedExecContext, ) -> Result { let env = apply_unified_exec_env(create_env(&context.turn.shell_environment_policy)); + let features = context.session.features(); let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new(self); let approval_requirement = create_approval_requirement_for_command( &context.turn.exec_policy, + &features, command, context.turn.approval_policy, &context.turn.sandbox_policy, diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index caa488a88f..9bb265c206 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1523,7 +1523,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { test.codex .submit(Op::ExecApproval { id: "0".into(), - decision: *decision, + decision: decision.clone(), }) .await?; wait_for_completion(&test).await; @@ -1544,7 +1544,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { test.codex .submit(Op::PatchApproval { id: "0".into(), - decision: *decision, + decision: decision.clone(), }) .await?; wait_for_completion(&test).await; @@ -1557,3 +1557,138 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { Ok(()) } + +#[tokio::test(flavor = "current_thread")] +async fn approving_allow_prefix_persists_policy_and_skips_future_prompts() -> Result<()> { + let server = start_mock_server().await; + let approval_policy = AskForApproval::UnlessTrusted; + let sandbox_policy = SandboxPolicy::DangerFullAccess; + let sandbox_policy_for_config = sandbox_policy.clone(); + let mut builder = test_codex().with_config(move |config| { + config.approval_policy = approval_policy; + config.sandbox_policy = sandbox_policy_for_config; + }); + let test = builder.build(&server).await?; + + let call_id_first = "allow-prefix-first"; + let (first_event, expected_command) = ActionKind::RunCommand { + command: "printf allow-prefix-ok", + } + .prepare(&test, &server, call_id_first, false) + .await?; + let expected_command = + expected_command.expect("allow prefix scenario should produce a shell command"); + let expected_allow_prefix = expected_command + .split_whitespace() + .map(ToString::to_string) + .collect::>(); + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-allow-prefix-1"), + first_event, + ev_completed("resp-allow-prefix-1"), + ]), + ) + .await; + let first_results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-allow-prefix-1", "done"), + ev_completed("resp-allow-prefix-2"), + ]), + ) + .await; + + submit_turn( + &test, + "allow-prefix-first", + approval_policy, + sandbox_policy.clone(), + ) + .await?; + + let approval = expect_exec_approval(&test, expected_command.as_str()).await; + assert_eq!(approval.allow_prefix, Some(expected_allow_prefix.clone())); + + test.codex + .submit(Op::ExecApproval { + id: "0".into(), + decision: ReviewDecision::ApprovedAllowPrefix { + allow_prefix: expected_allow_prefix.clone(), + }, + }) + .await?; + wait_for_completion(&test).await; + + let policy_path = test.home.path().join("policy").join("default.codexpolicy"); + let policy_contents = fs::read_to_string(&policy_path)?; + assert!( + policy_contents + .contains(r#"prefix_rule(pattern=["printf", "allow-prefix-ok"], decision="allow")"#), + "unexpected policy contents: {policy_contents}" + ); + + let first_output = parse_result( + &first_results + .single_request() + .function_call_output(call_id_first), + ); + assert_eq!(first_output.exit_code.unwrap_or(0), 0); + assert!( + first_output.stdout.contains("allow-prefix-ok"), + "unexpected stdout: {}", + first_output.stdout + ); + + let call_id_second = "allow-prefix-second"; + let (second_event, second_command) = ActionKind::RunCommand { + command: "printf allow-prefix-ok", + } + .prepare(&test, &server, call_id_second, false) + .await?; + assert_eq!(second_command.as_deref(), Some(expected_command.as_str())); + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-allow-prefix-3"), + second_event, + ev_completed("resp-allow-prefix-3"), + ]), + ) + .await; + let second_results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-allow-prefix-2", "done"), + ev_completed("resp-allow-prefix-4"), + ]), + ) + .await; + + submit_turn( + &test, + "allow-prefix-second", + approval_policy, + sandbox_policy.clone(), + ) + .await?; + + wait_for_completion_without_approval(&test).await; + + let second_output = parse_result( + &second_results + .single_request() + .function_call_output(call_id_second), + ); + assert_eq!(second_output.exit_code.unwrap_or(0), 0); + assert!( + second_output.stdout.contains("allow-prefix-ok"), + "unexpected stdout: {}", + second_output.stdout + ); + + Ok(()) +} diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 55808f17ca..cbee0fe0e7 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -180,6 +180,7 @@ async fn run_codex_tool_session_inner( call_id, reason: _, risk, + allow_prefix: _, parsed_cmd, }) => { handle_exec_approval_request( diff --git a/codex-rs/otel/src/otel_event_manager.rs b/codex-rs/otel/src/otel_event_manager.rs index c300f3fb82..d3536cd8db 100644 --- a/codex-rs/otel/src/otel_event_manager.rs +++ b/codex-rs/otel/src/otel_event_manager.rs @@ -352,7 +352,7 @@ impl OtelEventManager { &self, tool_name: &str, call_id: &str, - decision: ReviewDecision, + decision: &ReviewDecision, source: ToolDecisionSource, ) { tracing::event!( @@ -369,7 +369,7 @@ impl OtelEventManager { slug = %self.metadata.slug, tool_name = %tool_name, call_id = %call_id, - decision = %decision.to_string().to_lowercase(), + decision = %decision.clone().to_string().to_lowercase(), source = %source.to_string(), ); } diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 9ef3a9e368..54a9efca9f 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -51,6 +51,10 @@ pub struct ExecApprovalRequestEvent { /// Optional model-provided risk assessment describing the blocked command. #[serde(skip_serializing_if = "Option::is_none")] pub risk: Option, + /// Prefix rule that can be added to the user's execpolicy to allow future runs. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional, type = "Array")] + pub allow_prefix: Option>, pub parsed_cmd: Vec, } diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 99d2ec70d3..ef06c2e1dc 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1649,14 +1649,16 @@ pub struct SessionConfiguredEvent { } /// User's decision in response to an ExecApprovalRequest. -#[derive( - Debug, Default, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, Display, JsonSchema, TS, -)] +#[derive(Debug, Default, Clone, Deserialize, Serialize, PartialEq, Eq, Display, JsonSchema, TS)] #[serde(rename_all = "snake_case")] pub enum ReviewDecision { /// User has approved this command and the agent should execute it. Approved, + /// User has approved this command and wants to add the command prefix to + /// the execpolicy allow list so future matching commands are permitted. + ApprovedAllowPrefix { allow_prefix: Vec }, + /// User has approved this command and wants to automatically approve any /// future identical instances (`command` and `cwd` match exactly) for the /// remainder of the session. diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index c6006a9ae7..3defdbf9e3 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -43,6 +43,7 @@ pub(crate) enum ApprovalRequest { command: Vec, reason: Option, risk: Option, + allow_prefix: Option>, }, ApplyPatch { id: String, @@ -104,8 +105,8 @@ impl ApprovalOverlay { header: Box, ) -> (Vec, SelectionViewParams) { let (options, title) = match &variant { - ApprovalVariant::Exec { .. } => ( - exec_options(), + ApprovalVariant::Exec { allow_prefix, .. } => ( + exec_options(allow_prefix.clone()), "Would you like to run the following command?".to_string(), ), ApprovalVariant::ApplyPatch { .. } => ( @@ -160,12 +161,12 @@ impl ApprovalOverlay { return; }; if let Some(variant) = self.current_variant.as_ref() { - match (&variant, &option.decision) { - (ApprovalVariant::Exec { id, command }, ApprovalDecision::Review(decision)) => { - self.handle_exec_decision(id, command, *decision); + match (variant, &option.decision) { + (ApprovalVariant::Exec { id, command, .. }, ApprovalDecision::Review(decision)) => { + self.handle_exec_decision(id, command, decision.clone()); } (ApprovalVariant::ApplyPatch { id, .. }, ApprovalDecision::Review(decision)) => { - self.handle_patch_decision(id, *decision); + self.handle_patch_decision(id, decision.clone()); } ( ApprovalVariant::McpElicitation { @@ -185,7 +186,7 @@ impl ApprovalOverlay { } fn handle_exec_decision(&self, id: &str, command: &[String], decision: ReviewDecision) { - let cell = history_cell::new_approval_decision_cell(command.to_vec(), decision); + let cell = history_cell::new_approval_decision_cell(command.to_vec(), decision.clone()); self.app_event_tx.send(AppEvent::InsertHistoryCell(cell)); self.app_event_tx.send(AppEvent::CodexOp(Op::ExecApproval { id: id.to_string(), @@ -273,7 +274,7 @@ impl BottomPaneView for ApprovalOverlay { && let Some(variant) = self.current_variant.as_ref() { match &variant { - ApprovalVariant::Exec { id, command } => { + ApprovalVariant::Exec { id, command, .. } => { self.handle_exec_decision(id, command, ReviewDecision::Abort); } ApprovalVariant::ApplyPatch { id, .. } => { @@ -336,6 +337,7 @@ impl From for ApprovalRequestState { command, reason, risk, + allow_prefix, } => { let reason = reason.filter(|item| !item.is_empty()); let has_reason = reason.is_some(); @@ -355,7 +357,11 @@ impl From for ApprovalRequestState { } header.extend(full_cmd_lines); Self { - variant: ApprovalVariant::Exec { id, command }, + variant: ApprovalVariant::Exec { + id, + command, + allow_prefix, + }, header: Box::new(Paragraph::new(header).wrap(Wrap { trim: false })), } } @@ -431,6 +437,7 @@ enum ApprovalVariant { Exec { id: String, command: Vec, + allow_prefix: Option>, }, ApplyPatch { id: String, @@ -463,7 +470,7 @@ impl ApprovalOption { } } -fn exec_options() -> Vec { +fn exec_options(allow_prefix: Option>) -> Vec { vec![ ApprovalOption { label: "Yes, proceed".to_string(), @@ -472,18 +479,28 @@ fn exec_options() -> Vec { additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], }, ApprovalOption { - label: "Yes, and don't ask again for this command".to_string(), + label: "Yes, and don't ask again this session".to_string(), decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession), display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], }, - ApprovalOption { - label: "No, and tell Codex what to do differently".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::Abort), - display_shortcut: Some(key_hint::plain(KeyCode::Esc)), - additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))], - }, ] + .into_iter() + .chain(allow_prefix.map(|prefix| ApprovalOption { + label: "Yes, and don't ask again for commands with this prefix".to_string(), + decision: ApprovalDecision::Review(ReviewDecision::ApprovedAllowPrefix { + allow_prefix: prefix, + }), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))], + })) + .chain([ApprovalOption { + label: "No, and tell Codex what to do differently".to_string(), + decision: ApprovalDecision::Review(ReviewDecision::Abort), + display_shortcut: Some(key_hint::plain(KeyCode::Esc)), + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))], + }]) + .collect() } fn patch_options() -> Vec { @@ -539,6 +556,7 @@ mod tests { command: vec!["echo".to_string(), "hi".to_string()], reason: Some("reason".to_string()), risk: None, + allow_prefix: None, } } @@ -571,6 +589,40 @@ mod tests { assert!(saw_op, "expected approval decision to emit an op"); } + #[test] + fn exec_prefix_option_emits_allow_prefix() { + let (tx, mut rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + let mut view = ApprovalOverlay::new( + ApprovalRequest::Exec { + id: "test".to_string(), + command: vec!["echo".to_string()], + reason: None, + risk: None, + allow_prefix: Some(vec!["echo".to_string()]), + }, + tx, + ); + view.handle_key_event(KeyEvent::new(KeyCode::Char('p'), KeyModifiers::NONE)); + let mut saw_op = false; + while let Ok(ev) = rx.try_recv() { + if let AppEvent::CodexOp(Op::ExecApproval { decision, .. }) = ev { + assert_eq!( + decision, + ReviewDecision::ApprovedAllowPrefix { + allow_prefix: vec!["echo".to_string()] + } + ); + saw_op = true; + break; + } + } + assert!( + saw_op, + "expected approval decision to emit an op with allow prefix" + ); + } + #[test] fn header_includes_command_snippet() { let (tx, _rx) = unbounded_channel::(); @@ -581,6 +633,7 @@ mod tests { command, reason: None, risk: None, + allow_prefix: None, }; let view = ApprovalOverlay::new(exec_request, tx); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index a0425c92d7..844442c24c 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -570,6 +570,7 @@ mod tests { command: vec!["echo".into(), "ok".into()], reason: None, risk: None, + allow_prefix: None, } } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index f956ef5c8a..6b63c01446 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1074,6 +1074,7 @@ impl ChatWidget { command: ev.command, reason: ev.reason, risk: ev.risk, + allow_prefix: ev.allow_prefix, }; self.bottom_pane.push_approval_request(request); self.request_redraw(); diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap index b84588e337..eaf0fed3a1 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap @@ -10,7 +10,8 @@ expression: terminal.backend().vt100().screen().contents() $ echo hello world › 1. Yes, proceed (y) - 2. Yes, and don't ask again for this command (a) - 3. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again this session (a) + 3. Yes, and don't ask again for commands with this prefix (p) + 4. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap index 543d367d23..ce2277ce62 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap @@ -7,7 +7,8 @@ expression: terminal.backend().vt100().screen().contents() $ echo hello world › 1. Yes, proceed (y) - 2. Yes, and don't ask again for this command (a) - 3. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again this session (a) + 3. Yes, and don't ask again for commands with this prefix (p) + 4. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap index f986a927e5..ff70d7d492 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap @@ -15,7 +15,7 @@ Buffer { " $ echo hello world ", " ", "› 1. Yes, proceed (y) ", - " 2. Yes, and don't ask again for this command (a) ", + " 2. Yes, and don't ask again this session (a) ", " 3. No, and tell Codex what to do differently (esc) ", " ", " Press enter to confirm or esc to cancel ", @@ -30,8 +30,8 @@ Buffer { x: 7, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, x: 0, y: 9, fg: Cyan, bg: Reset, underline: Reset, modifier: BOLD, x: 21, y: 9, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, - x: 48, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, - x: 49, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 44, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 45, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, x: 48, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, x: 51, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, x: 2, y: 13, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap index f98c807878..5ce388c87b 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap @@ -1,6 +1,5 @@ --- source: tui/src/chatwidget/tests.rs -assertion_line: 1548 expression: terminal.backend() --- " " @@ -13,7 +12,8 @@ expression: terminal.backend() " $ echo 'hello world' " " " "› 1. Yes, proceed (y) " -" 2. Yes, and don't ask again for this command (a) " -" 3. No, and tell Codex what to do differently (esc) " +" 2. Yes, and don't ask again this session (a) " +" 3. Yes, and don't ask again for commands with this prefix (p) " +" 4. No, and tell Codex what to do differently (esc) " " " " Press enter to confirm or esc to cancel " diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 419dab2c87..2b75f960bd 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -688,6 +688,7 @@ fn exec_approval_emits_proposed_command_and_decision_history() { "this is a test reason such as one that would be produced by the model".into(), ), risk: None, + allow_prefix: None, parsed_cmd: vec![], }; chat.handle_codex_event(Event { @@ -732,6 +733,7 @@ fn exec_approval_decision_truncates_multiline_and_long_commands() { "this is a test reason such as one that would be produced by the model".into(), ), risk: None, + allow_prefix: None, parsed_cmd: vec![], }; chat.handle_codex_event(Event { @@ -782,6 +784,7 @@ fn exec_approval_decision_truncates_multiline_and_long_commands() { cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), reason: None, risk: None, + allow_prefix: None, parsed_cmd: vec![], }; chat.handle_codex_event(Event { @@ -1990,6 +1993,7 @@ fn approval_modal_exec_snapshot() { "this is a test reason such as one that would be produced by the model".into(), ), risk: None, + allow_prefix: Some(vec!["echo".into(), "hello".into(), "world".into()]), parsed_cmd: vec![], }; chat.handle_codex_event(Event { @@ -2036,6 +2040,7 @@ fn approval_modal_exec_without_reason_snapshot() { cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), reason: None, risk: None, + allow_prefix: Some(vec!["echo".into(), "hello".into(), "world".into()]), parsed_cmd: vec![], }; chat.handle_codex_event(Event { @@ -2249,6 +2254,7 @@ fn status_widget_and_approval_modal_snapshot() { "this is a test reason such as one that would be produced by the model".into(), ), risk: None, + allow_prefix: Some(vec!["echo".into(), "hello world".into()]), parsed_cmd: vec![], }; chat.handle_codex_event(Event { diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index c4fd31f548..1dcc6d0ef8 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -409,6 +409,19 @@ pub fn new_approval_decision_cell( ], ) } + ApprovedAllowPrefix { .. } => { + let snippet = Span::from(exec_snippet(&command)).dim(); + ( + "✔ ".green(), + vec![ + "You ".into(), + "approved".bold(), + " codex to run ".into(), + snippet, + " and added its prefix to your allow list".bold(), + ], + ) + } ApprovedForSession => { let snippet = Span::from(exec_snippet(&command)).dim(); ( diff --git a/docs/config.md b/docs/config.md index 0f0761a301..6aa1bde8c5 100644 --- a/docs/config.md +++ b/docs/config.md @@ -603,7 +603,7 @@ metadata above): - `codex.tool_decision` - `tool_name` - `call_id` - - `decision` (`approved`, `approved_for_session`, `denied`, or `abort`) + - `decision` (`approved`, `approved_allow_prefix`, `approved_for_session`, `denied`, or `abort`) - `source` (`config` or `user`) - `codex.tool_result` - `tool_name` From be1a90bb6edb2da087ee90cb2e4327a5e4bb46a5 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Wed, 3 Dec 2025 17:46:30 +0000 Subject: [PATCH 2/3] add back line --- codex-rs/core/src/tools/handlers/shell.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 88b9815d5b..887441ba26 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -241,6 +241,7 @@ impl ShellHandler { SandboxPermissions::from(exec_params.with_escalated_permissions.unwrap_or(false)), ) .await; + let req = ShellRequest { command: exec_params.command.clone(), cwd: exec_params.cwd.clone(), From bf0ba872008c88dc51105f9e9d8a1bb54067c347 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Wed, 3 Dec 2025 17:47:45 +0000 Subject: [PATCH 3/3] . --- codex-rs/core/src/codex.rs | 4 + codex-rs/core/src/exec_policy.rs | 91 ++++++++++--------- codex-rs/core/src/tools/handlers/shell.rs | 8 +- codex-rs/core/src/tools/orchestrator.rs | 14 +-- codex-rs/core/src/tools/runtimes/shell.rs | 14 +-- .../core/src/tools/runtimes/unified_exec.rs | 21 +++-- codex-rs/core/src/tools/sandboxing.rs | 23 +++-- .../core/src/unified_exec/session_manager.rs | 6 +- codex-rs/core/tests/suite/approvals.rs | 30 ++++-- 9 files changed, 118 insertions(+), 93 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 7461bf41fb..7e0a1d685d 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -871,6 +871,10 @@ impl Session { .await } + /// Adds a prefix rule to the exec policy + /// + /// This mutates the in-memory execpolicy so the current conversation can use the new + /// prefix and persists the change in default.execpolicy so new conversations will also allow the new prefix. pub(crate) async fn persist_command_allow_prefix( &self, prefix: &[String], diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 44328de14a..7c96deea90 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -16,12 +16,13 @@ use codex_protocol::protocol::SandboxPolicy; use thiserror::Error; use tokio::fs; use tokio::sync::RwLock; +use tokio::task::spawn_blocking; use crate::bash::parse_shell_lc_plain_commands; use crate::features::Feature; use crate::features::Features; use crate::sandboxing::SandboxPermissions; -use crate::tools::sandboxing::ApprovalRequirement; +use crate::tools::sandboxing::ExecApprovalRequirement; const FORBIDDEN_REASON: &str = "execpolicy forbids this command"; const PROMPT_REASON: &str = "execpolicy requires approval for this command"; @@ -55,6 +56,9 @@ pub enum ExecPolicyUpdateError { #[error("failed to update execpolicy file {path}: {source}")] AppendRule { path: PathBuf, source: AmendError }, + #[error("failed to join blocking execpolicy update task: {source}")] + JoinBlockingTask { source: tokio::task::JoinError }, + #[error("failed to update in-memory execpolicy: {source}")] AddRule { #[from] @@ -114,17 +118,23 @@ pub(crate) async fn append_allow_prefix_rule_and_update( prefix: &[String], ) -> Result<(), ExecPolicyUpdateError> { let policy_path = default_policy_path(codex_home); - blocking_append_allow_prefix_rule(&policy_path, prefix).map_err(|source| { - ExecPolicyUpdateError::AppendRule { - path: policy_path, - source, - } + let prefix = prefix.to_vec(); + spawn_blocking({ + let policy_path = policy_path.clone(); + let prefix = prefix.clone(); + move || blocking_append_allow_prefix_rule(&policy_path, &prefix) + }) + .await + .map_err(|source| ExecPolicyUpdateError::JoinBlockingTask { source })? + .map_err(|source| ExecPolicyUpdateError::AppendRule { + path: policy_path, + source, })?; current_policy .write() .await - .add_prefix_rule(prefix, Decision::Allow)?; + .add_prefix_rule(&prefix, Decision::Allow)?; Ok(()) } @@ -132,23 +142,23 @@ pub(crate) async fn append_allow_prefix_rule_and_update( fn requirement_from_decision( decision: Decision, approval_policy: AskForApproval, -) -> ApprovalRequirement { +) -> ExecApprovalRequirement { match decision { - Decision::Forbidden => ApprovalRequirement::Forbidden { + Decision::Forbidden => ExecApprovalRequirement::Forbidden { reason: FORBIDDEN_REASON.to_string(), }, Decision::Prompt => { let reason = PROMPT_REASON.to_string(); if matches!(approval_policy, AskForApproval::Never) { - ApprovalRequirement::Forbidden { reason } + ExecApprovalRequirement::Forbidden { reason } } else { - ApprovalRequirement::NeedsApproval { + ExecApprovalRequirement::NeedsApproval { reason: Some(reason), allow_prefix: None, } } } - Decision::Allow => ApprovalRequirement::Skip { + Decision::Allow => ExecApprovalRequirement::Skip { bypass_sandbox: true, }, } @@ -170,17 +180,16 @@ fn allow_prefix_if_applicable( } } -pub(crate) async fn create_approval_requirement_for_command( +pub(crate) async fn create_exec_approval_requirement_for_command( exec_policy: &Arc>, features: &Features, command: &[String], approval_policy: AskForApproval, sandbox_policy: &SandboxPolicy, sandbox_permissions: SandboxPermissions, -) -> ApprovalRequirement { +) -> ExecApprovalRequirement { let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]); - let policy = exec_policy.read().await; - let evaluation = policy.check_multiple(commands.iter()); + let evaluation = exec_policy.read().await.check_multiple(commands.iter()); match evaluation { Evaluation::Match { decision, .. } => requirement_from_decision(decision, approval_policy), @@ -191,12 +200,12 @@ pub(crate) async fn create_approval_requirement_for_command( command, sandbox_permissions, ) { - ApprovalRequirement::NeedsApproval { + ExecApprovalRequirement::NeedsApproval { reason: None, allow_prefix: allow_prefix_if_applicable(&commands, features), } } else { - ApprovalRequirement::Skip { + ExecApprovalRequirement::Skip { bypass_sandbox: false, } } @@ -349,7 +358,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") "rm -rf /tmp".to_string(), ]; - let requirement = create_approval_requirement_for_command( + let requirement = create_exec_approval_requirement_for_command( &policy, &Features::with_defaults(), &forbidden_script, @@ -361,14 +370,14 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, - ApprovalRequirement::Forbidden { + ExecApprovalRequirement::Forbidden { reason: FORBIDDEN_REASON.to_string() } ); } #[tokio::test] - async fn approval_requirement_prefers_execpolicy_match() { + async fn exec_approval_requirement_prefers_execpolicy_match() { let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#; let mut parser = PolicyParser::new(); parser @@ -377,7 +386,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") let policy = Arc::new(RwLock::new(parser.build())); let command = vec!["rm".to_string()]; - let requirement = create_approval_requirement_for_command( + let requirement = create_exec_approval_requirement_for_command( &policy, &Features::with_defaults(), &command, @@ -389,7 +398,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, - ApprovalRequirement::NeedsApproval { + ExecApprovalRequirement::NeedsApproval { reason: Some(PROMPT_REASON.to_string()), allow_prefix: None, } @@ -397,7 +406,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") } #[tokio::test] - async fn approval_requirement_respects_approval_policy() { + async fn exec_approval_requirement_respects_approval_policy() { let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#; let mut parser = PolicyParser::new(); parser @@ -406,7 +415,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") let policy = Arc::new(RwLock::new(parser.build())); let command = vec!["rm".to_string()]; - let requirement = create_approval_requirement_for_command( + let requirement = create_exec_approval_requirement_for_command( &policy, &Features::with_defaults(), &command, @@ -418,18 +427,18 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, - ApprovalRequirement::Forbidden { + ExecApprovalRequirement::Forbidden { reason: PROMPT_REASON.to_string() } ); } #[tokio::test] - async fn approval_requirement_falls_back_to_heuristics() { - let command = vec!["python".to_string()]; + async fn exec_approval_requirement_falls_back_to_heuristics() { + let command = vec!["cargo".to_string(), "build".to_string()]; let empty_policy = Arc::new(RwLock::new(Policy::empty())); - let requirement = create_approval_requirement_for_command( + let requirement = create_exec_approval_requirement_for_command( &empty_policy, &Features::with_defaults(), &command, @@ -441,7 +450,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, - ApprovalRequirement::NeedsApproval { + ExecApprovalRequirement::NeedsApproval { reason: None, allow_prefix: Some(command) } @@ -499,10 +508,10 @@ prefix_rule(pattern=["rm"], decision="forbidden") #[tokio::test] async fn allow_prefix_is_present_for_single_command_without_policy_match() { - let command = vec!["python".to_string()]; + let command = vec!["cargo".to_string(), "build".to_string()]; let empty_policy = Arc::new(RwLock::new(Policy::empty())); - let requirement = create_approval_requirement_for_command( + let requirement = create_exec_approval_requirement_for_command( &empty_policy, &Features::with_defaults(), &command, @@ -514,7 +523,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, - ApprovalRequirement::NeedsApproval { + ExecApprovalRequirement::NeedsApproval { reason: None, allow_prefix: Some(command) } @@ -523,12 +532,12 @@ prefix_rule(pattern=["rm"], decision="forbidden") #[tokio::test] async fn allow_prefix_is_disabled_when_execpolicy_feature_disabled() { - let command = vec!["python".to_string()]; + let command = vec!["cargo".to_string(), "build".to_string()]; let mut features = Features::with_defaults(); features.disable(Feature::ExecPolicy); - let requirement = create_approval_requirement_for_command( + let requirement = create_exec_approval_requirement_for_command( &Arc::new(RwLock::new(Policy::empty())), &features, &command, @@ -540,7 +549,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, - ApprovalRequirement::NeedsApproval { + ExecApprovalRequirement::NeedsApproval { reason: None, allow_prefix: None, } @@ -557,7 +566,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") let policy = Arc::new(RwLock::new(parser.build())); let command = vec!["rm".to_string()]; - let requirement = create_approval_requirement_for_command( + let requirement = create_exec_approval_requirement_for_command( &policy, &Features::with_defaults(), &command, @@ -569,7 +578,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, - ApprovalRequirement::NeedsApproval { + ExecApprovalRequirement::NeedsApproval { reason: Some(PROMPT_REASON.to_string()), allow_prefix: None, } @@ -581,9 +590,9 @@ prefix_rule(pattern=["rm"], decision="forbidden") let command = vec![ "bash".to_string(), "-lc".to_string(), - "python && echo ok".to_string(), + "cargo build && echo ok".to_string(), ]; - let requirement = create_approval_requirement_for_command( + let requirement = create_exec_approval_requirement_for_command( &Arc::new(RwLock::new(Policy::empty())), &Features::with_defaults(), &command, @@ -595,7 +604,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, - ApprovalRequirement::NeedsApproval { + ExecApprovalRequirement::NeedsApproval { reason: None, allow_prefix: None, } diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 887441ba26..cd05d126bf 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -6,7 +6,7 @@ use std::sync::Arc; use crate::codex::TurnContext; use crate::exec::ExecParams; use crate::exec_env::create_env; -use crate::exec_policy::create_approval_requirement_for_command; +use crate::exec_policy::create_exec_approval_requirement_for_command; use crate::function_tool::FunctionCallError; use crate::is_safe_command::is_known_safe_command; use crate::protocol::ExecCommandSource; @@ -232,7 +232,7 @@ impl ShellHandler { emitter.begin(event_ctx).await; let features = session.features(); - let approval_requirement = create_approval_requirement_for_command( + let exec_approval_requirement = create_exec_approval_requirement_for_command( &turn.exec_policy, &features, &exec_params.command, @@ -241,7 +241,7 @@ impl ShellHandler { SandboxPermissions::from(exec_params.with_escalated_permissions.unwrap_or(false)), ) .await; - + let req = ShellRequest { command: exec_params.command.clone(), cwd: exec_params.cwd.clone(), @@ -249,7 +249,7 @@ impl ShellHandler { env: exec_params.env.clone(), with_escalated_permissions: exec_params.with_escalated_permissions, justification: exec_params.justification.clone(), - approval_requirement, + exec_approval_requirement, }; let mut orchestrator = ToolOrchestrator::new(); let mut runtime = ShellRuntime::new(); diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 7510fc6aa4..5ac3c63509 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -11,14 +11,14 @@ use crate::error::get_error_message_ui; use crate::exec::ExecToolCallOutput; use crate::sandboxing::SandboxManager; use crate::tools::sandboxing::ApprovalCtx; -use crate::tools::sandboxing::ApprovalRequirement; +use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::ProvidesSandboxRetryData; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxOverride; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; -use crate::tools::sandboxing::default_approval_requirement; +use crate::tools::sandboxing::default_exec_approval_requirement; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; @@ -54,17 +54,17 @@ impl ToolOrchestrator { // 1) Approval let mut already_approved = false; - let requirement = tool.approval_requirement(req).unwrap_or_else(|| { - default_approval_requirement(approval_policy, &turn_ctx.sandbox_policy) + let requirement = tool.exec_approval_requirement(req).unwrap_or_else(|| { + default_exec_approval_requirement(approval_policy, &turn_ctx.sandbox_policy) }); match requirement { - ApprovalRequirement::Skip { .. } => { + ExecApprovalRequirement::Skip { .. } => { otel.tool_decision(otel_tn, otel_ci, &ReviewDecision::Approved, otel_cfg); } - ApprovalRequirement::Forbidden { reason } => { + ExecApprovalRequirement::Forbidden { reason } => { return Err(ToolError::Rejected(reason)); } - ApprovalRequirement::NeedsApproval { reason, .. } => { + ExecApprovalRequirement::NeedsApproval { reason, .. } => { let mut risk = None; if let Some(metadata) = req.sandbox_retry_data() { diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index ed5ba8a0d8..48dc5b9990 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -9,7 +9,7 @@ use crate::sandboxing::execute_env; use crate::tools::runtimes::build_command_spec; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; -use crate::tools::sandboxing::ApprovalRequirement; +use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::ProvidesSandboxRetryData; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxOverride; @@ -32,7 +32,7 @@ pub struct ShellRequest { pub env: std::collections::HashMap, pub with_escalated_permissions: Option, pub justification: Option, - pub approval_requirement: ApprovalRequirement, + pub exec_approval_requirement: ExecApprovalRequirement, } impl ProvidesSandboxRetryData for ShellRequest { @@ -114,7 +114,7 @@ impl Approvable for ShellRuntime { cwd, reason, risk, - req.approval_requirement.allow_prefix().cloned(), + req.exec_approval_requirement.allow_prefix().cloned(), ) .await }) @@ -122,15 +122,15 @@ impl Approvable for ShellRuntime { }) } - fn approval_requirement(&self, req: &ShellRequest) -> Option { - Some(req.approval_requirement.clone()) + fn exec_approval_requirement(&self, req: &ShellRequest) -> Option { + Some(req.exec_approval_requirement.clone()) } fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride { if req.with_escalated_permissions.unwrap_or(false) || matches!( - req.approval_requirement, - ApprovalRequirement::Skip { + req.exec_approval_requirement, + ExecApprovalRequirement::Skip { 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 0a23695bde..45d804d688 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -10,7 +10,7 @@ use crate::exec::ExecExpiration; use crate::tools::runtimes::build_command_spec; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; -use crate::tools::sandboxing::ApprovalRequirement; +use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::ProvidesSandboxRetryData; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxOverride; @@ -36,7 +36,7 @@ pub struct UnifiedExecRequest { pub env: HashMap, pub with_escalated_permissions: Option, pub justification: Option, - pub approval_requirement: ApprovalRequirement, + pub exec_approval_requirement: ExecApprovalRequirement, } impl ProvidesSandboxRetryData for UnifiedExecRequest { @@ -66,7 +66,7 @@ impl UnifiedExecRequest { env: HashMap, with_escalated_permissions: Option, justification: Option, - approval_requirement: ApprovalRequirement, + exec_approval_requirement: ExecApprovalRequirement, ) -> Self { Self { command, @@ -74,7 +74,7 @@ impl UnifiedExecRequest { env, with_escalated_permissions, justification, - approval_requirement, + exec_approval_requirement, } } } @@ -132,7 +132,7 @@ impl Approvable for UnifiedExecRuntime<'_> { cwd, reason, risk, - req.approval_requirement.allow_prefix().cloned(), + req.exec_approval_requirement.allow_prefix().cloned(), ) .await }) @@ -140,15 +140,18 @@ impl Approvable for UnifiedExecRuntime<'_> { }) } - fn approval_requirement(&self, req: &UnifiedExecRequest) -> Option { - Some(req.approval_requirement.clone()) + fn exec_approval_requirement( + &self, + req: &UnifiedExecRequest, + ) -> Option { + Some(req.exec_approval_requirement.clone()) } fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride { if req.with_escalated_permissions.unwrap_or(false) || matches!( - req.approval_requirement, - ApprovalRequirement::Skip { + req.exec_approval_requirement, + ExecApprovalRequirement::Skip { bypass_sandbox: true } ) diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 8efef10e59..793c162936 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -88,24 +88,24 @@ pub(crate) struct ApprovalCtx<'a> { // Specifies what tool orchestrator should do with a given tool call. #[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) enum ApprovalRequirement { +pub(crate) enum ExecApprovalRequirement { /// No approval required for this tool call. Skip { /// The first attempt should skip sandboxing (e.g., when explicitly /// greenlit by policy). bypass_sandbox: bool, }, - /// Approval required for this tool call + /// Approval required for this tool call. NeedsApproval { reason: Option, /// Prefix that can be whitelisted via execpolicy to skip future approvals for similar commands allow_prefix: Option>, }, - /// Execution forbidden for this tool call + /// Execution forbidden for this tool call. Forbidden { reason: String }, } -impl ApprovalRequirement { +impl ExecApprovalRequirement { pub fn allow_prefix(&self) -> Option<&Vec> { match self { Self::NeedsApproval { @@ -120,10 +120,10 @@ impl ApprovalRequirement { /// - Never, OnFailure: do not ask /// - OnRequest: ask unless sandbox policy is DangerFullAccess /// - UnlessTrusted: always ask -pub(crate) fn default_approval_requirement( +pub(crate) fn default_exec_approval_requirement( policy: AskForApproval, sandbox_policy: &SandboxPolicy, -) -> ApprovalRequirement { +) -> ExecApprovalRequirement { let needs_approval = match policy { AskForApproval::Never | AskForApproval::OnFailure => false, AskForApproval::OnRequest => !matches!(sandbox_policy, SandboxPolicy::DangerFullAccess), @@ -131,12 +131,12 @@ pub(crate) fn default_approval_requirement( }; if needs_approval { - ApprovalRequirement::NeedsApproval { + ExecApprovalRequirement::NeedsApproval { reason: None, allow_prefix: None, } } else { - ApprovalRequirement::Skip { + ExecApprovalRequirement::Skip { bypass_sandbox: false, } } @@ -168,10 +168,9 @@ pub(crate) trait Approvable { matches!(policy, AskForApproval::Never) } - /// 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 { + /// Return `Some(_)` to specify a custom exec approval requirement, or `None` + /// to fall back to policy-based default. + fn exec_approval_requirement(&self, _req: &Req) -> Option { None } diff --git a/codex-rs/core/src/unified_exec/session_manager.rs b/codex-rs/core/src/unified_exec/session_manager.rs index b9b4ea8dbe..51e5626073 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -15,7 +15,7 @@ use crate::codex::TurnContext; use crate::exec::ExecToolCallOutput; use crate::exec::StreamOutput; use crate::exec_env::create_env; -use crate::exec_policy::create_approval_requirement_for_command; +use crate::exec_policy::create_exec_approval_requirement_for_command; use crate::protocol::BackgroundEventEvent; use crate::protocol::EventMsg; use crate::protocol::ExecCommandSource; @@ -559,7 +559,7 @@ impl UnifiedExecSessionManager { let features = context.session.features(); let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new(self); - let approval_requirement = create_approval_requirement_for_command( + let exec_approval_requirement = create_exec_approval_requirement_for_command( &context.turn.exec_policy, &features, command, @@ -574,7 +574,7 @@ impl UnifiedExecSessionManager { env, with_escalated_permissions, justification, - approval_requirement, + exec_approval_requirement, ); let tool_ctx = ToolCtx { session: context.session.as_ref(), diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 9bb265c206..8ac31fb378 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1559,29 +1559,29 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { } #[tokio::test(flavor = "current_thread")] +#[cfg(unix)] async fn approving_allow_prefix_persists_policy_and_skips_future_prompts() -> Result<()> { let server = start_mock_server().await; let approval_policy = AskForApproval::UnlessTrusted; - let sandbox_policy = SandboxPolicy::DangerFullAccess; + let sandbox_policy = SandboxPolicy::ReadOnly; let sandbox_policy_for_config = sandbox_policy.clone(); let mut builder = test_codex().with_config(move |config| { config.approval_policy = approval_policy; config.sandbox_policy = sandbox_policy_for_config; }); let test = builder.build(&server).await?; + let allow_prefix_path = test.cwd.path().join("allow-prefix.txt"); + let _ = fs::remove_file(&allow_prefix_path); let call_id_first = "allow-prefix-first"; let (first_event, expected_command) = ActionKind::RunCommand { - command: "printf allow-prefix-ok", + command: "touch allow-prefix.txt", } .prepare(&test, &server, call_id_first, false) .await?; let expected_command = expected_command.expect("allow prefix scenario should produce a shell command"); - let expected_allow_prefix = expected_command - .split_whitespace() - .map(ToString::to_string) - .collect::>(); + let expected_allow_prefix = vec!["touch".to_string(), "allow-prefix.txt".to_string()]; let _ = mount_sse_once( &server, @@ -1626,7 +1626,7 @@ async fn approving_allow_prefix_persists_policy_and_skips_future_prompts() -> Re let policy_contents = fs::read_to_string(&policy_path)?; assert!( policy_contents - .contains(r#"prefix_rule(pattern=["printf", "allow-prefix-ok"], decision="allow")"#), + .contains(r#"prefix_rule(pattern=["touch", "allow-prefix.txt"], decision="allow")"#), "unexpected policy contents: {policy_contents}" ); @@ -1637,14 +1637,19 @@ async fn approving_allow_prefix_persists_policy_and_skips_future_prompts() -> Re ); assert_eq!(first_output.exit_code.unwrap_or(0), 0); assert!( - first_output.stdout.contains("allow-prefix-ok"), + first_output.stdout.is_empty(), "unexpected stdout: {}", first_output.stdout ); + assert_eq!( + fs::read_to_string(&allow_prefix_path)?, + "", + "unexpected file contents after first run" + ); let call_id_second = "allow-prefix-second"; let (second_event, second_command) = ActionKind::RunCommand { - command: "printf allow-prefix-ok", + command: "touch allow-prefix.txt", } .prepare(&test, &server, call_id_second, false) .await?; @@ -1685,10 +1690,15 @@ async fn approving_allow_prefix_persists_policy_and_skips_future_prompts() -> Re ); assert_eq!(second_output.exit_code.unwrap_or(0), 0); assert!( - second_output.stdout.contains("allow-prefix-ok"), + second_output.stdout.is_empty(), "unexpected stdout: {}", second_output.stdout ); + assert_eq!( + fs::read_to_string(&allow_prefix_path)?, + "", + "unexpected file contents after second run" + ); Ok(()) }