From c7a3ce8388d12be627853c30a03d5b40b68f33f1 Mon Sep 17 00:00:00 2001 From: Eva Wong Date: Thu, 7 May 2026 13:06:56 -0700 Subject: [PATCH] fix: honor exec approval policy overrides --- codex-rs/cli/src/main.rs | 74 ++++++++++++++++++++++++++++++++-- codex-rs/exec/src/cli.rs | 10 +++++ codex-rs/exec/src/lib.rs | 33 ++++++++++++++- codex-rs/exec/src/lib_tests.rs | 44 ++++++++++++++++++++ 4 files changed, 156 insertions(+), 5 deletions(-) diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index f3545629c1..a9d9506488 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -789,9 +789,7 @@ async fn cli_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> { root_remote_auth_token_env.as_deref(), "exec", )?; - exec_cli - .shared - .inherit_exec_root_options(&interactive.shared); + apply_exec_root_options(&mut exec_cli, &interactive)?; prepend_config_flags( &mut exec_cli.config_overrides, root_config_overrides.clone(), @@ -1513,6 +1511,21 @@ fn prepend_config_flags( .splice(0..0, cli_config_overrides.raw_overrides); } +fn apply_exec_root_options(exec_cli: &mut ExecCli, interactive: &TuiCli) -> anyhow::Result<()> { + exec_cli + .shared + .inherit_exec_root_options(&interactive.shared); + if exec_cli.approval_policy.is_none() { + exec_cli.approval_policy = interactive.approval_policy; + } + if exec_cli.dangerously_bypass_approvals_and_sandbox && exec_cli.approval_policy.is_some() { + anyhow::bail!( + "--dangerously-bypass-approvals-and-sandbox cannot be used with --ask-for-approval" + ); + } + Ok(()) +} + fn reject_remote_mode_for_subcommand( remote: Option<&str>, remote_auth_token_env: Option<&str>, @@ -1847,6 +1860,61 @@ mod tests { assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); } + #[test] + fn exec_accepts_approval_policy_after_subcommand() { + let cli = MultitoolCli::try_parse_from(["codex", "exec", "-a", "untrusted", "hi"]) + .expect("parse should succeed"); + + let Some(Subcommand::Exec(exec)) = cli.subcommand else { + panic!("expected exec subcommand"); + }; + + assert_matches!( + exec.approval_policy, + Some(codex_utils_cli::ApprovalModeCliArg::Untrusted) + ); + } + + #[test] + fn exec_accepts_root_approval_policy() { + let cli = MultitoolCli::try_parse_from(["codex", "-a", "untrusted", "exec", "hi"]) + .expect("parse should succeed"); + let MultitoolCli { + interactive, + subcommand, + .. + } = cli; + let Some(Subcommand::Exec(mut exec)) = subcommand else { + panic!("expected exec subcommand"); + }; + + assert_matches!( + exec.approval_policy, + Some(codex_utils_cli::ApprovalModeCliArg::Untrusted) + ); + apply_exec_root_options(&mut exec, &interactive).expect("root options apply"); + + assert_matches!( + exec.approval_policy, + Some(codex_utils_cli::ApprovalModeCliArg::Untrusted) + ); + } + + #[test] + fn exec_rejects_subcommand_approval_policy_with_bypass() { + let err = MultitoolCli::try_parse_from([ + "codex", + "exec", + "--dangerously-bypass-approvals-and-sandbox", + "-a", + "untrusted", + "hi", + ]) + .expect_err("conflicting permission flags should be rejected"); + + assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); + } + fn app_server_from_args(args: &[&str]) -> AppServerCommand { let cli = MultitoolCli::try_parse_from(args).expect("parse"); let Subcommand::AppServer(app_server) = cli.subcommand.expect("app-server present") else { diff --git a/codex-rs/exec/src/cli.rs b/codex-rs/exec/src/cli.rs index 2b12898c3c..bc9ee7e0c9 100644 --- a/codex-rs/exec/src/cli.rs +++ b/codex-rs/exec/src/cli.rs @@ -2,6 +2,7 @@ use clap::Args; use clap::FromArgMatches; use clap::Parser; use clap::ValueEnum; +use codex_utils_cli::ApprovalModeCliArg; use codex_utils_cli::CliConfigOverrides; use codex_utils_cli::SharedCliOptions; use std::path::PathBuf; @@ -19,6 +20,15 @@ pub struct Cli { #[clap(flatten)] pub shared: ExecSharedCliOptions, + /// Configure when the model requires human approval before executing a command. + #[arg( + long = "ask-for-approval", + short = 'a', + global = true, + conflicts_with = "dangerously_bypass_approvals_and_sandbox" + )] + pub approval_policy: Option, + /// Allow running Codex outside a Git repository. #[arg(long = "skip-git-repo-check", global = true, default_value_t = false)] pub skip_git_repo_check: bool, diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index a26dc525e6..54f911afd2 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -94,6 +94,7 @@ use codex_protocol::protocol::SessionSource; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::canonicalize_existing_preserving_symlinks; +use codex_utils_cli::ApprovalModeCliArg; use codex_utils_cli::SharedCliOptions; use codex_utils_oss::ensure_oss_provider_ready; use codex_utils_oss::get_default_model_for_oss_provider; @@ -222,6 +223,28 @@ fn exec_root_span() -> tracing::Span { ) } +fn cli_overrides_include_approval_policy(cli_kv_overrides: &[(String, T)]) -> bool { + cli_kv_overrides + .iter() + .any(|(key, _)| key == "approval_policy") +} + +fn exec_approval_policy_override( + dangerously_bypass_approvals_and_sandbox: bool, + approval_policy: Option, + cli_kv_overrides: &[(String, T)], +) -> Option { + if dangerously_bypass_approvals_and_sandbox { + Some(AskForApproval::Never) + } else if let Some(approval_policy) = approval_policy { + Some(approval_policy.into()) + } else if cli_overrides_include_approval_policy(cli_kv_overrides) { + None + } else { + Some(AskForApproval::Never) + } +} + pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> { #[allow(clippy::print_stderr)] if let Some(message) = cli.removed_full_auto_warning() { @@ -235,6 +258,7 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result let Cli { command, shared, + approval_policy, skip_git_repo_check, ephemeral, ignore_user_config, @@ -401,8 +425,13 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result model, review_model: None, config_profile, - // Default to never ask for approvals in headless mode. Feature flags can override. - approval_policy: Some(AskForApproval::Never), + // Default to never ask for approvals in headless mode unless the caller + // explicitly selected an approval policy. + approval_policy: exec_approval_policy_override( + dangerously_bypass_approvals_and_sandbox, + approval_policy, + &cli_kv_overrides, + ), approvals_reviewer: None, sandbox_mode, permission_profile: None, diff --git a/codex-rs/exec/src/lib_tests.rs b/codex-rs/exec/src/lib_tests.rs index caa2921d19..a45e602a91 100644 --- a/codex-rs/exec/src/lib_tests.rs +++ b/codex-rs/exec/src/lib_tests.rs @@ -22,6 +22,50 @@ fn exec_defaults_analytics_to_enabled() { assert_eq!(DEFAULT_ANALYTICS_ENABLED, true); } +#[test] +fn exec_approval_policy_override_defaults_to_never() { + let cli_kv_overrides: Vec<(String, ())> = Vec::new(); + + assert_eq!( + exec_approval_policy_override(false, None, &cli_kv_overrides), + Some(AskForApproval::Never) + ); +} + +#[test] +fn exec_approval_policy_override_uses_cli_approval_policy() { + let cli_kv_overrides: Vec<(String, ())> = Vec::new(); + + assert_eq!( + exec_approval_policy_override( + false, + Some(ApprovalModeCliArg::Untrusted), + &cli_kv_overrides + ), + Some(AskForApproval::UnlessTrusted) + ); +} + +#[test] +fn exec_approval_policy_override_allows_config_approval_policy() { + let cli_kv_overrides = vec![("approval_policy".to_string(), ())]; + + assert_eq!( + exec_approval_policy_override(false, None, &cli_kv_overrides), + None + ); +} + +#[test] +fn exec_approval_policy_override_yolo_forces_never() { + let cli_kv_overrides = vec![("approval_policy".to_string(), ())]; + + assert_eq!( + exec_approval_policy_override(true, Some(ApprovalModeCliArg::Untrusted), &cli_kv_overrides), + Some(AskForApproval::Never) + ); +} + #[test] fn exec_root_span_can_be_parented_from_trace_context() { let subscriber = test_tracing_subscriber();