mirror of
https://github.com/openai/codex.git
synced 2026-05-23 20:44:50 +00:00
fix: honor exec approval policy overrides
This commit is contained in:
@@ -856,10 +856,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);
|
||||
exec_cli.strict_config |= root_strict_config;
|
||||
apply_exec_root_options(&mut exec_cli, &interactive)?;
|
||||
prepend_config_flags(
|
||||
&mut exec_cli.config_overrides,
|
||||
root_config_overrides.clone(),
|
||||
@@ -1652,6 +1649,22 @@ fn prepend_config_flags(
|
||||
subcommand_config_overrides.prepend_root_overrides(cli_config_overrides);
|
||||
}
|
||||
|
||||
fn apply_exec_root_options(exec_cli: &mut ExecCli, interactive: &TuiCli) -> anyhow::Result<()> {
|
||||
exec_cli
|
||||
.shared
|
||||
.inherit_exec_root_options(&interactive.shared);
|
||||
exec_cli.strict_config |= interactive.strict_config;
|
||||
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>,
|
||||
@@ -2121,6 +2134,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 {
|
||||
|
||||
@@ -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;
|
||||
@@ -23,6 +24,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<ApprovalModeCliArg>,
|
||||
|
||||
/// 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,
|
||||
|
||||
@@ -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;
|
||||
@@ -231,6 +232,28 @@ fn exec_stderr_env_filter() -> EnvFilter {
|
||||
.unwrap_or_else(|_| EnvFilter::new("error"))
|
||||
}
|
||||
|
||||
fn cli_overrides_include_approval_policy<T>(cli_kv_overrides: &[(String, T)]) -> bool {
|
||||
cli_kv_overrides
|
||||
.iter()
|
||||
.any(|(key, _)| key == "approval_policy")
|
||||
}
|
||||
|
||||
fn exec_approval_policy_override<T>(
|
||||
dangerously_bypass_approvals_and_sandbox: bool,
|
||||
approval_policy: Option<ApprovalModeCliArg>,
|
||||
cli_kv_overrides: &[(String, T)],
|
||||
) -> Option<AskForApproval> {
|
||||
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() {
|
||||
@@ -245,6 +268,7 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result
|
||||
command,
|
||||
strict_config,
|
||||
shared,
|
||||
approval_policy,
|
||||
skip_git_repo_check,
|
||||
ephemeral,
|
||||
ignore_user_config,
|
||||
@@ -406,8 +430,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,
|
||||
|
||||
@@ -81,6 +81,50 @@ fn exec_default_stderr_filter_suppresses_otel_self_diagnostics() {
|
||||
assert!(logs.contains("real exec error"));
|
||||
}
|
||||
|
||||
#[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();
|
||||
|
||||
Reference in New Issue
Block a user