From 80e3635b25827b20f0ca0eecb13b4fbdd88606f2 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Thu, 20 Nov 2025 16:46:52 -0500 Subject: [PATCH] Add approval allow-prefix flow in core and tui --- .../app-server/src/bespoke_event_handling.rs | 3 + codex-rs/core/src/codex.rs | 67 +++++- codex-rs/core/src/codex_delegate.rs | 9 +- codex-rs/core/src/exec_policy.rs | 217 ++++++++++++++---- codex-rs/core/src/tools/handlers/shell.rs | 3 +- codex-rs/core/src/tools/orchestrator.rs | 2 +- .../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 | 22 +- .../core/src/unified_exec/session_manager.rs | 3 +- codex-rs/core/tests/suite/approvals.rs | 1 + codex-rs/core/tests/suite/codex_delegate.rs | 1 + codex-rs/core/tests/suite/otel.rs | 6 + codex-rs/mcp-server/src/codex_tool_runner.rs | 1 + codex-rs/mcp-server/src/exec_approval.rs | 1 + codex-rs/protocol/src/approvals.rs | 4 + codex-rs/protocol/src/protocol.rs | 3 + .../tui/src/bottom_pane/approval_overlay.rs | 101 ++++++-- codex-rs/tui/src/bottom_pane/mod.rs | 1 + codex-rs/tui/src/chatwidget.rs | 1 + codex-rs/tui/src/chatwidget/tests.rs | 6 + 22 files changed, 403 insertions(+), 70 deletions(-) diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index b4dd16b9a6..12c5d1497a 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 => { @@ -949,6 +950,7 @@ async fn on_exec_approval_response( .submit(Op::ExecApproval { id: event_turn_id, decision: response.decision, + allow_prefix: None, }) .await { @@ -1153,6 +1155,7 @@ async fn on_command_execution_request_approval_response( .submit(Op::ExecApproval { id: event_turn_id, decision, + allow_prefix: None, }) .await { diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 8b1dfafd21..09d6872d2a 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -71,6 +71,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::model_family::find_family_for_model; @@ -861,11 +862,43 @@ impl Session { .await } + pub(crate) async fn persist_command_allow_prefix( + &self, + prefix: &[String], + ) -> Result<(), ExecPolicyUpdateError> { + let (features, codex_home) = { + let state = self.state.lock().await; + ( + state.session_configuration.features.clone(), + state + .session_configuration + .original_config_do_not_use + .codex_home + .clone(), + ) + }; + + let policy = + crate::exec_policy::append_allow_prefix_rule_and_reload(&features, &codex_home, prefix) + .await?; + + let mut state = self.state.lock().await; + state.session_configuration.exec_policy = policy; + + Ok(()) + } + + pub(crate) async fn current_exec_policy(&self) -> Arc { + let state = self.state.lock().await; + state.session_configuration.exec_policy.clone() + } + /// 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, @@ -874,6 +907,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. @@ -901,6 +935,7 @@ impl Session { cwd, reason, risk, + allow_prefix, parsed_cmd, }); self.send_event(turn_context, event).await; @@ -1429,8 +1464,12 @@ async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiv handlers::user_input_or_turn(&sess, sub.id.clone(), sub.op, &mut previous_context) .await; } - Op::ExecApproval { id, decision } => { - handlers::exec_approval(&sess, id, decision).await; + Op::ExecApproval { + id, + decision, + allow_prefix, + } => { + handlers::exec_approval(&sess, id, decision, allow_prefix).await; } Op::PatchApproval { id, decision } => { handlers::patch_approval(&sess, id, decision).await; @@ -1508,6 +1547,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; @@ -1622,7 +1662,28 @@ mod handlers { } } - pub async fn exec_approval(sess: &Arc, id: String, decision: ReviewDecision) { + pub async fn exec_approval( + sess: &Arc, + id: String, + decision: ReviewDecision, + allow_prefix: Option>, + ) { + if let Some(prefix) = allow_prefix + && matches!( + decision, + ReviewDecision::Approved | ReviewDecision::ApprovedForSession + ) + && let Err(err) = sess.persist_command_allow_prefix(&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; diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 796331d1e8..e4cf077785 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -275,6 +275,7 @@ async fn handle_exec_approval( event.cwd, event.reason, event.risk, + event.allow_prefix, ); let decision = await_approval_with_cancel( approval_fut, @@ -284,7 +285,13 @@ async fn handle_exec_approval( ) .await; - let _ = codex.submit(Op::ExecApproval { id, decision }).await; + let _ = codex + .submit(Op::ExecApproval { + id, + decision, + allow_prefix: None, + }) + .await; } /// Handle an ApplyPatchApprovalRequest by consulting the parent session and replying. diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 15e591648d..bfd531c378 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -4,10 +4,12 @@ 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::Evaluation; use codex_execpolicy::Policy; use codex_execpolicy::PolicyParser; +use codex_execpolicy::append_allow_prefix_rule; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; use thiserror::Error; @@ -23,6 +25,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,6 +48,15 @@ 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 reload execpolicy after updating policy: {0}")] + Reload(#[from] ExecPolicyError), +} + pub(crate) async fn exec_policy_for( features: &Features, codex_home: &Path, @@ -84,37 +96,68 @@ 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_reload( + features: &Features, + codex_home: &Path, + prefix: &[String], +) -> Result, ExecPolicyUpdateError> { + let policy_path = default_policy_path(codex_home); + append_allow_prefix_rule(&policy_path, prefix).map_err(|source| { + ExecPolicyUpdateError::AppendRule { + path: policy_path, + source, + } + })?; + + exec_policy_for(features, codex_home) + .await + .map_err(ExecPolicyUpdateError::Reload) +} + +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). +fn allow_prefix_if_applicable( + commands: &[Vec], + evaluation: &Evaluation, +) -> Option> { + if matches!(evaluation, Evaluation::NoMatch) && commands.len() == 1 { + return Some(commands[0].clone()); + } + + None +} + pub(crate) fn create_approval_requirement_for_command( policy: &Policy, command: &[String], @@ -122,20 +165,27 @@ pub(crate) fn create_approval_requirement_for_command( 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 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, &evaluation), + } + } else { + ApprovalRequirement::Skip { + bypass_sandbox: false, + } + } } } } @@ -253,7 +303,7 @@ mod tests { let temp_dir = tempdir().expect("create temp dir"); fs::write( temp_dir.path().join("root.codexpolicy"), - r#"prefix_rule(pattern=["ls"], decision="prompt")"#, + r#"prefix_rule(pattern=[\"ls\"], decision=\"prompt\")"#, ) .expect("write policy file"); @@ -284,9 +334,13 @@ 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, + &forbidden_script, + AskForApproval::OnRequest, + &SandboxPolicy::DangerFullAccess, + SandboxPermissions::UseDefault, + ); assert_eq!( requirement, @@ -317,7 +371,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, } ); } @@ -363,7 +418,83 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, - ApprovalRequirement::NeedsApproval { reason: None } + ApprovalRequirement::NeedsApproval { + reason: None, + allow_prefix: Some(command) + } + ); + } + + #[test] + fn allow_prefix_is_present_for_single_command_without_policy_match() { + let command = vec!["python".to_string()]; + + let empty_policy = Policy::empty(); + let requirement = create_approval_requirement_for_command( + &empty_policy, + &command, + AskForApproval::UnlessTrusted, + &SandboxPolicy::ReadOnly, + SandboxPermissions::UseDefault, + ); + + assert_eq!( + requirement, + ApprovalRequirement::NeedsApproval { + reason: None, + allow_prefix: Some(command) + } + ); + } + + #[test] + 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 = parser.build(); + let command = vec!["rm".to_string()]; + + let requirement = create_approval_requirement_for_command( + &policy, + &command, + AskForApproval::OnRequest, + &SandboxPolicy::DangerFullAccess, + SandboxPermissions::UseDefault, + ); + + assert_eq!( + requirement, + ApprovalRequirement::NeedsApproval { + reason: Some(PROMPT_REASON.to_string()), + allow_prefix: None, + } + ); + } + + #[test] + 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( + &Policy::empty(), + &command, + AskForApproval::UnlessTrusted, + &SandboxPolicy::ReadOnly, + SandboxPermissions::UseDefault, + ); + + 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 736da34491..7d41c192e5 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -231,6 +231,7 @@ impl ShellHandler { let event_ctx = ToolEventCtx::new(session.as_ref(), turn.as_ref(), &call_id, None); emitter.begin(event_ctx).await; + let exec_policy = session.current_exec_policy().await; let req = ShellRequest { command: exec_params.command.clone(), cwd: exec_params.cwd.clone(), @@ -239,7 +240,7 @@ impl ShellHandler { with_escalated_permissions: exec_params.with_escalated_permissions, justification: exec_params.justification.clone(), approval_requirement: create_approval_requirement_for_command( - &turn.exec_policy, + exec_policy.as_ref(), &exec_params.command, turn.approval_policy, &turn.sandbox_policy, diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index de23d510bf..93b914e570 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -64,7 +64,7 @@ impl ToolOrchestrator { 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() { 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..88592c6ed9 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -96,11 +96,26 @@ pub(crate) enum ApprovalRequirement { bypass_sandbox: bool, }, /// Approval required for this tool call - NeedsApproval { reason: Option }, + NeedsApproval { + reason: Option, + 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 +130,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 37a12bf2df..d01c166b9b 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -554,6 +554,7 @@ impl UnifiedExecSessionManager { let env = apply_unified_exec_env(create_env(&context.turn.shell_environment_policy)); let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new(self); + let exec_policy = context.session.current_exec_policy().await; let req = UnifiedExecToolRequest::new( command.to_vec(), cwd, @@ -561,7 +562,7 @@ impl UnifiedExecSessionManager { with_escalated_permissions, justification, create_approval_requirement_for_command( - &context.turn.exec_policy, + exec_policy.as_ref(), 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..c0ddea7d0c 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1524,6 +1524,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { .submit(Op::ExecApproval { id: "0".into(), decision: *decision, + allow_prefix: None, }) .await?; wait_for_completion(&test).await; diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs index f5fe1a7df9..24d3a6e4cd 100644 --- a/codex-rs/core/tests/suite/codex_delegate.rs +++ b/codex-rs/core/tests/suite/codex_delegate.rs @@ -95,6 +95,7 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() { .submit(Op::ExecApproval { id: "0".into(), decision: ReviewDecision::Approved, + allow_prefix: None, }) .await .expect("submit exec approval"); diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index 8665d3a8ea..045c42e941 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -843,6 +843,7 @@ async fn handle_container_exec_user_approved_records_tool_decision() { .submit(Op::ExecApproval { id: "0".into(), decision: ReviewDecision::Approved, + allow_prefix: None, }) .await .unwrap(); @@ -901,6 +902,7 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision() .submit(Op::ExecApproval { id: "0".into(), decision: ReviewDecision::ApprovedForSession, + allow_prefix: None, }) .await .unwrap(); @@ -959,6 +961,7 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() { .submit(Op::ExecApproval { id: "0".into(), decision: ReviewDecision::Approved, + allow_prefix: None, }) .await .unwrap(); @@ -1017,6 +1020,7 @@ async fn handle_container_exec_user_denies_records_tool_decision() { .submit(Op::ExecApproval { id: "0".into(), decision: ReviewDecision::Denied, + allow_prefix: None, }) .await .unwrap(); @@ -1075,6 +1079,7 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision() .submit(Op::ExecApproval { id: "0".into(), decision: ReviewDecision::ApprovedForSession, + allow_prefix: None, }) .await .unwrap(); @@ -1134,6 +1139,7 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() { .submit(Op::ExecApproval { id: "0".into(), decision: ReviewDecision::Denied, + allow_prefix: None, }) .await .unwrap(); 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/mcp-server/src/exec_approval.rs b/codex-rs/mcp-server/src/exec_approval.rs index 033523ac0d..e5b2e00f45 100644 --- a/codex-rs/mcp-server/src/exec_approval.rs +++ b/codex-rs/mcp-server/src/exec_approval.rs @@ -150,6 +150,7 @@ async fn on_exec_approval_response( .submit(Op::ExecApproval { id: event_id, decision: response.decision, + allow_prefix: None, }) .await { 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 347cc119ff..2c4d793a4b 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -146,6 +146,9 @@ pub enum Op { id: String, /// The user's decision in response to the request. decision: ReviewDecision, + /// When set, persist this prefix to the execpolicy allow list. + #[serde(default, skip_serializing_if = "Option::is_none")] + allow_prefix: Option>, }, /// Approve a code patch diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index c6006a9ae7..80859dcd84 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,9 +161,9 @@ 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, option.allow_prefix.clone()); } (ApprovalVariant::ApplyPatch { id, .. }, ApprovalDecision::Review(decision)) => { self.handle_patch_decision(id, *decision); @@ -184,12 +185,19 @@ impl ApprovalOverlay { self.advance_queue(); } - fn handle_exec_decision(&self, id: &str, command: &[String], decision: ReviewDecision) { + fn handle_exec_decision( + &self, + id: &str, + command: &[String], + decision: ReviewDecision, + allow_prefix: Option>, + ) { let cell = history_cell::new_approval_decision_cell(command.to_vec(), decision); self.app_event_tx.send(AppEvent::InsertHistoryCell(cell)); self.app_event_tx.send(AppEvent::CodexOp(Op::ExecApproval { id: id.to_string(), decision, + allow_prefix, })); } @@ -273,8 +281,8 @@ impl BottomPaneView for ApprovalOverlay { && let Some(variant) = self.current_variant.as_ref() { match &variant { - ApprovalVariant::Exec { id, command } => { - self.handle_exec_decision(id, command, ReviewDecision::Abort); + ApprovalVariant::Exec { id, command, .. } => { + self.handle_exec_decision(id, command, ReviewDecision::Abort, None); } ApprovalVariant::ApplyPatch { id, .. } => { self.handle_patch_decision(id, ReviewDecision::Abort); @@ -336,6 +344,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 +364,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 +444,7 @@ enum ApprovalVariant { Exec { id: String, command: Vec, + allow_prefix: Option>, }, ApplyPatch { id: String, @@ -453,6 +467,7 @@ struct ApprovalOption { decision: ApprovalDecision, display_shortcut: Option, additional_shortcuts: Vec, + allow_prefix: Option>, } impl ApprovalOption { @@ -463,27 +478,39 @@ impl ApprovalOption { } } -fn exec_options() -> Vec { +fn exec_options(allow_prefix: Option>) -> Vec { vec![ ApprovalOption { label: "Yes, proceed".to_string(), decision: ApprovalDecision::Review(ReviewDecision::Approved), display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], + allow_prefix: None, }, ApprovalOption { label: "Yes, and don't ask again for this command".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'))], + allow_prefix: None, }, ] + .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::ApprovedForSession), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))], + allow_prefix: Some(prefix), + })) + .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'))], + allow_prefix: None, + }]) + .collect() } fn patch_options() -> Vec { @@ -493,12 +520,14 @@ fn patch_options() -> Vec { decision: ApprovalDecision::Review(ReviewDecision::Approved), display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], + allow_prefix: None, }, 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'))], + allow_prefix: None, }, ] } @@ -510,18 +539,21 @@ fn elicitation_options() -> Vec { decision: ApprovalDecision::McpElicitation(ElicitationAction::Accept), display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], + allow_prefix: None, }, ApprovalOption { label: "No, but continue without it".to_string(), decision: ApprovalDecision::McpElicitation(ElicitationAction::Decline), display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))], + allow_prefix: None, }, ApprovalOption { label: "Cancel this request".to_string(), decision: ApprovalDecision::McpElicitation(ElicitationAction::Cancel), display_shortcut: Some(key_hint::plain(KeyCode::Esc)), additional_shortcuts: vec![key_hint::plain(KeyCode::Char('c'))], + allow_prefix: None, }, ] } @@ -539,6 +571,7 @@ mod tests { command: vec!["echo".to_string(), "hi".to_string()], reason: Some("reason".to_string()), risk: None, + allow_prefix: None, } } @@ -571,6 +604,41 @@ 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 { + allow_prefix, + decision, + .. + }) = ev + { + assert_eq!(decision, ReviewDecision::ApprovedForSession); + assert_eq!(allow_prefix, Some(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 +649,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 b4255fd979..4c5a8ea258 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -563,6 +563,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 79a1843130..cd4eedfda1 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1069,6 +1069,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/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 890e8bbe1d..c25355a759 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -677,6 +677,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 { @@ -721,6 +722,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 { @@ -771,6 +773,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 { @@ -1969,6 +1972,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 { @@ -2015,6 +2019,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 { @@ -2228,6 +2233,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 {