mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Preserve auto-review approval policy in codex exec (#23763)
## Why `codex exec` was forcing headless runs to `approval_policy = "never"` even when the resolved reviewer was `auto_review`. That prevented unattended exec workflows from reaching the reviewed MCP write path they were configured to use. ## What changed - Keep the existing headless `never` default for ordinary exec runs. - Re-resolve exec config without that synthetic override when the final reviewer resolves to `AutoReview`, so configured or requirements-driven approval policy is preserved. - Add regression coverage for: - `auto_review` plus `on-request` from user config - requirements-driven `AutoReview`, asserting exec’s final approval policy matches the no-override control config exactly ## Validation - `just fmt` - `cargo test -p codex-exec`
This commit is contained in:
committed by
GitHub
Unverified
parent
c0ea566bb5
commit
13edafb6ed
+55
-10
@@ -80,6 +80,7 @@ use codex_otel::set_parent_from_context;
|
||||
use codex_otel::traceparent_context_from_env;
|
||||
use codex_protocol::SessionId;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::config_types::SandboxMode;
|
||||
use codex_protocol::models::ActivePermissionProfile;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
@@ -134,6 +135,7 @@ pub use exec_events::TurnStartedEvent;
|
||||
pub use exec_events::Usage;
|
||||
pub use exec_events::WebSearchItem;
|
||||
use serde_json::Value;
|
||||
use std::future::Future;
|
||||
use std::io::IsTerminal;
|
||||
use std::io::Read;
|
||||
use std::path::Path;
|
||||
@@ -399,11 +401,11 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result
|
||||
None // No model specified, will use the default.
|
||||
};
|
||||
|
||||
// Load configuration and determine approval policy
|
||||
let overrides = ConfigOverrides {
|
||||
model,
|
||||
review_model: None,
|
||||
// Default to never ask for approvals in headless mode. Feature flags can override.
|
||||
// Default to never ask for approvals in headless mode. Rebuild below if
|
||||
// the fully resolved reviewer is AutoReview.
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
approvals_reviewer: None,
|
||||
sandbox_mode,
|
||||
@@ -428,14 +430,22 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result
|
||||
additional_writable_roots: add_dir,
|
||||
};
|
||||
|
||||
let config = ConfigBuilder::default()
|
||||
.cli_overrides(cli_kv_overrides)
|
||||
.harness_overrides(overrides)
|
||||
.loader_overrides(loader_overrides)
|
||||
.strict_config(strict_config)
|
||||
.cloud_requirements(cloud_requirements)
|
||||
.build()
|
||||
.await?;
|
||||
let build_config = |overrides| {
|
||||
ConfigBuilder::default()
|
||||
.codex_home(codex_home.to_path_buf())
|
||||
.cli_overrides(cli_kv_overrides.clone())
|
||||
.harness_overrides(overrides)
|
||||
.loader_overrides(loader_overrides.clone())
|
||||
.strict_config(strict_config)
|
||||
.cloud_requirements(cloud_requirements.clone())
|
||||
.build()
|
||||
};
|
||||
let config = build_exec_config(
|
||||
overrides,
|
||||
dangerously_bypass_approvals_and_sandbox || removed_full_auto,
|
||||
build_config,
|
||||
)
|
||||
.await?;
|
||||
|
||||
#[allow(clippy::print_stderr)]
|
||||
match check_execpolicy_for_warnings(&config.config_layer_stack).await {
|
||||
@@ -561,6 +571,41 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result
|
||||
.await
|
||||
}
|
||||
|
||||
async fn build_exec_config<BuildConfig, BuildFuture>(
|
||||
overrides: ConfigOverrides,
|
||||
preserve_headless_approval_policy: bool,
|
||||
build_config: BuildConfig,
|
||||
) -> std::io::Result<Config>
|
||||
where
|
||||
BuildConfig: Fn(ConfigOverrides) -> BuildFuture,
|
||||
BuildFuture: Future<Output = std::io::Result<Config>>,
|
||||
{
|
||||
let build_without_headless_approval_policy = || {
|
||||
build_config(ConfigOverrides {
|
||||
approval_policy: None,
|
||||
..overrides.clone()
|
||||
})
|
||||
};
|
||||
match build_config(overrides.clone()).await {
|
||||
Ok(config)
|
||||
if config.approvals_reviewer == ApprovalsReviewer::AutoReview
|
||||
&& !preserve_headless_approval_policy =>
|
||||
{
|
||||
build_without_headless_approval_policy().await
|
||||
}
|
||||
Ok(config) => Ok(config),
|
||||
Err(headless_error) if !preserve_headless_approval_policy => {
|
||||
match build_without_headless_approval_policy().await {
|
||||
Ok(config) if config.approvals_reviewer == ApprovalsReviewer::AutoReview => {
|
||||
Ok(config)
|
||||
}
|
||||
Ok(_) | Err(_) => Err(headless_error),
|
||||
}
|
||||
}
|
||||
Err(headless_error) => Err(headless_error),
|
||||
}
|
||||
}
|
||||
|
||||
async fn run_exec_session(args: ExecRunArgs) -> anyhow::Result<()> {
|
||||
let ExecRunArgs {
|
||||
in_process_start_args,
|
||||
|
||||
@@ -459,6 +459,92 @@ async fn thread_start_params_include_review_policy_when_auto_review_is_enabled()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn build_exec_config_retries_without_invalid_headless_policy_for_auto_review() {
|
||||
let codex_home = tempdir().expect("create temp codex home");
|
||||
let cwd = tempdir().expect("create temp cwd");
|
||||
std::fs::write(
|
||||
codex_home.path().join("config.toml"),
|
||||
r#"
|
||||
approval_policy = "on-request"
|
||||
approvals_reviewer = "auto_review"
|
||||
"#,
|
||||
)
|
||||
.expect("write config");
|
||||
let requirements_path = codex_home.path().join("requirements.toml");
|
||||
std::fs::write(
|
||||
&requirements_path,
|
||||
r#"
|
||||
allowed_approval_policies = ["never", "on-request"]
|
||||
allowed_sandbox_modes = ["read-only", "workspace-write"]
|
||||
"#,
|
||||
)
|
||||
.expect("write requirements");
|
||||
let mut loader_overrides = LoaderOverrides::without_managed_config_for_tests();
|
||||
loader_overrides.system_requirements_path = Some(requirements_path);
|
||||
let overrides = ConfigOverrides {
|
||||
cwd: Some(cwd.path().to_path_buf()),
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
sandbox_mode: Some(SandboxMode::DangerFullAccess),
|
||||
..Default::default()
|
||||
};
|
||||
let build_config = |overrides| {
|
||||
ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.loader_overrides(loader_overrides.clone())
|
||||
.harness_overrides(overrides)
|
||||
.build()
|
||||
};
|
||||
|
||||
let error = build_config(overrides.clone())
|
||||
.await
|
||||
.expect_err("synthetic headless approval policy should fail");
|
||||
assert!(
|
||||
error
|
||||
.to_string()
|
||||
.contains("`approval_policy = \"never\"` cannot be used")
|
||||
);
|
||||
|
||||
let config = build_exec_config(
|
||||
overrides,
|
||||
/*preserve_headless_approval_policy*/ false,
|
||||
build_config,
|
||||
)
|
||||
.await
|
||||
.expect("auto-review config should retry without the synthetic approval policy");
|
||||
|
||||
assert_eq!(
|
||||
config.permissions.approval_policy.value(),
|
||||
AskForApproval::OnRequest
|
||||
);
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn build_exec_config_preserves_headless_error_when_retry_fails() {
|
||||
let overrides = ConfigOverrides {
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let error = build_exec_config(
|
||||
overrides,
|
||||
/*preserve_headless_approval_policy*/ false,
|
||||
|overrides| async move {
|
||||
let message = if overrides.approval_policy == Some(AskForApproval::Never) {
|
||||
"headless error"
|
||||
} else {
|
||||
"retry error"
|
||||
};
|
||||
Err(std::io::Error::other(message))
|
||||
},
|
||||
)
|
||||
.await
|
||||
.expect_err("failed speculative retry should preserve the original error");
|
||||
|
||||
assert_eq!(error.to_string(), "headless error");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_start_params_include_user_thread_source() {
|
||||
let codex_home = tempdir().expect("create temp codex home");
|
||||
|
||||
@@ -0,0 +1,69 @@
|
||||
#![cfg(not(target_os = "windows"))]
|
||||
#![allow(clippy::expect_used, clippy::unwrap_used)]
|
||||
|
||||
use core_test_support::responses;
|
||||
use core_test_support::test_codex_exec::test_codex_exec;
|
||||
|
||||
async fn run_exec_with_auto_review_config(extra_args: &[&str]) -> anyhow::Result<String> {
|
||||
let test = test_codex_exec();
|
||||
std::fs::write(
|
||||
test.home_path().join("config.toml"),
|
||||
r#"
|
||||
approval_policy = "on-request"
|
||||
approvals_reviewer = "auto_review"
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let server = responses::start_mock_server().await;
|
||||
let body = responses::sse(vec![
|
||||
responses::ev_response_created("response_1"),
|
||||
responses::ev_assistant_message("response_1", "done"),
|
||||
responses::ev_completed("response_1"),
|
||||
]);
|
||||
responses::mount_sse_once(&server, body).await;
|
||||
|
||||
let mut cmd = test.cmd_with_server(&server);
|
||||
let output = cmd
|
||||
.arg("--skip-git-repo-check")
|
||||
.args(extra_args)
|
||||
.arg("check approval mode")
|
||||
.output()?;
|
||||
|
||||
assert!(output.status.success(), "exec run failed: {output:?}");
|
||||
|
||||
Ok(String::from_utf8(output.stderr)?)
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn exec_preserves_on_request_for_auto_review_config() -> anyhow::Result<()> {
|
||||
let stderr = run_exec_with_auto_review_config(&[]).await?;
|
||||
assert!(
|
||||
stderr.contains("approval: on-request"),
|
||||
"stderr missing preserved auto-review approval mode: {stderr}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn exec_bypass_preserves_never_for_auto_review_config() -> anyhow::Result<()> {
|
||||
let stderr =
|
||||
run_exec_with_auto_review_config(&["--dangerously-bypass-approvals-and-sandbox"]).await?;
|
||||
assert!(
|
||||
stderr.contains("approval: never"),
|
||||
"stderr missing bypass approval mode: {stderr}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn exec_full_auto_preserves_never_for_auto_review_config() -> anyhow::Result<()> {
|
||||
let stderr = run_exec_with_auto_review_config(&["--full-auto"]).await?;
|
||||
assert!(
|
||||
stderr.contains("approval: never"),
|
||||
"stderr missing full-auto approval mode: {stderr}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -1,6 +1,7 @@
|
||||
// Aggregates all former standalone integration tests as modules.
|
||||
mod add_dir;
|
||||
mod apply_patch;
|
||||
mod approval_policy;
|
||||
mod auth_env;
|
||||
mod ephemeral;
|
||||
mod mcp_required_exit;
|
||||
|
||||
Reference in New Issue
Block a user