feat: add auto-approval for codex exec (#5043)
This commit is contained in:
@@ -1125,6 +1125,15 @@ impl Config {
|
|||||||
.or(cfg.review_model)
|
.or(cfg.review_model)
|
||||||
.unwrap_or_else(default_review_model);
|
.unwrap_or_else(default_review_model);
|
||||||
|
|
||||||
|
let mut approval_policy = approval_policy
|
||||||
|
.or(config_profile.approval_policy)
|
||||||
|
.or(cfg.approval_policy)
|
||||||
|
.unwrap_or_else(AskForApproval::default);
|
||||||
|
|
||||||
|
if features.enabled(Feature::ApproveAll) {
|
||||||
|
approval_policy = AskForApproval::OnRequest;
|
||||||
|
}
|
||||||
|
|
||||||
let config = Self {
|
let config = Self {
|
||||||
model,
|
model,
|
||||||
review_model,
|
review_model,
|
||||||
@@ -1135,10 +1144,7 @@ impl Config {
|
|||||||
model_provider_id,
|
model_provider_id,
|
||||||
model_provider,
|
model_provider,
|
||||||
cwd: resolved_cwd,
|
cwd: resolved_cwd,
|
||||||
approval_policy: approval_policy
|
approval_policy,
|
||||||
.or(config_profile.approval_policy)
|
|
||||||
.or(cfg.approval_policy)
|
|
||||||
.unwrap_or_else(AskForApproval::default),
|
|
||||||
sandbox_policy,
|
sandbox_policy,
|
||||||
shell_environment_policy,
|
shell_environment_policy,
|
||||||
notify: cfg.notify,
|
notify: cfg.notify,
|
||||||
@@ -1432,6 +1438,26 @@ exclude_slash_tmp = true
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn approve_all_feature_forces_on_request_policy() -> std::io::Result<()> {
|
||||||
|
let cfg = r#"
|
||||||
|
[features]
|
||||||
|
approve_all = true
|
||||||
|
"#;
|
||||||
|
let parsed = toml::from_str::<ConfigToml>(cfg)
|
||||||
|
.expect("TOML deserialization should succeed for approve_all feature");
|
||||||
|
let temp_dir = TempDir::new()?;
|
||||||
|
let config = Config::load_from_base_config_with_overrides(
|
||||||
|
parsed,
|
||||||
|
ConfigOverrides::default(),
|
||||||
|
temp_dir.path().to_path_buf(),
|
||||||
|
)?;
|
||||||
|
|
||||||
|
assert!(config.features.enabled(Feature::ApproveAll));
|
||||||
|
assert_eq!(config.approval_policy, AskForApproval::OnRequest);
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn config_defaults_to_auto_oauth_store_mode() -> std::io::Result<()> {
|
fn config_defaults_to_auto_oauth_store_mode() -> std::io::Result<()> {
|
||||||
let codex_home = TempDir::new()?;
|
let codex_home = TempDir::new()?;
|
||||||
|
|||||||
@@ -41,6 +41,8 @@ pub enum Feature {
|
|||||||
ViewImageTool,
|
ViewImageTool,
|
||||||
/// Allow the model to request web searches.
|
/// Allow the model to request web searches.
|
||||||
WebSearchRequest,
|
WebSearchRequest,
|
||||||
|
/// Automatically approve all approval requests from the harness.
|
||||||
|
ApproveAll,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Feature {
|
impl Feature {
|
||||||
@@ -247,4 +249,10 @@ pub const FEATURES: &[FeatureSpec] = &[
|
|||||||
stage: Stage::Stable,
|
stage: Stage::Stable,
|
||||||
default_enabled: false,
|
default_enabled: false,
|
||||||
},
|
},
|
||||||
|
FeatureSpec {
|
||||||
|
id: Feature::ApproveAll,
|
||||||
|
key: "approve_all",
|
||||||
|
stage: Stage::Experimental,
|
||||||
|
default_enabled: false,
|
||||||
|
},
|
||||||
];
|
];
|
||||||
|
|||||||
@@ -17,6 +17,7 @@ use codex_core::ConversationManager;
|
|||||||
use codex_core::NewConversation;
|
use codex_core::NewConversation;
|
||||||
use codex_core::config::Config;
|
use codex_core::config::Config;
|
||||||
use codex_core::config::ConfigOverrides;
|
use codex_core::config::ConfigOverrides;
|
||||||
|
use codex_core::features::Feature;
|
||||||
use codex_core::git_info::get_git_repo_root;
|
use codex_core::git_info::get_git_repo_root;
|
||||||
use codex_core::protocol::AskForApproval;
|
use codex_core::protocol::AskForApproval;
|
||||||
use codex_core::protocol::Event;
|
use codex_core::protocol::Event;
|
||||||
@@ -168,8 +169,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
|||||||
model,
|
model,
|
||||||
review_model: None,
|
review_model: None,
|
||||||
config_profile,
|
config_profile,
|
||||||
// This CLI is intended to be headless and has no affordances for asking
|
// Default to never ask for approvals in headless mode. Feature flags can override.
|
||||||
// the user for approval.
|
|
||||||
approval_policy: Some(AskForApproval::Never),
|
approval_policy: Some(AskForApproval::Never),
|
||||||
sandbox_mode,
|
sandbox_mode,
|
||||||
cwd: cwd.map(|p| p.canonicalize().unwrap_or(p)),
|
cwd: cwd.map(|p| p.canonicalize().unwrap_or(p)),
|
||||||
@@ -192,6 +192,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
|||||||
};
|
};
|
||||||
|
|
||||||
let config = Config::load_with_cli_overrides(cli_kv_overrides, overrides).await?;
|
let config = Config::load_with_cli_overrides(cli_kv_overrides, overrides).await?;
|
||||||
|
let approve_all_enabled = config.features.enabled(Feature::ApproveAll);
|
||||||
|
|
||||||
let otel = codex_core::otel_init::build_provider(&config, env!("CARGO_PKG_VERSION"));
|
let otel = codex_core::otel_init::build_provider(&config, env!("CARGO_PKG_VERSION"));
|
||||||
|
|
||||||
@@ -360,6 +361,34 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
|||||||
if matches!(event.msg, EventMsg::Error(_)) {
|
if matches!(event.msg, EventMsg::Error(_)) {
|
||||||
error_seen = true;
|
error_seen = true;
|
||||||
}
|
}
|
||||||
|
// Auto-approve requests when the approve_all feature is enabled.
|
||||||
|
if approve_all_enabled {
|
||||||
|
match &event.msg {
|
||||||
|
EventMsg::ExecApprovalRequest(_) => {
|
||||||
|
if let Err(e) = conversation
|
||||||
|
.submit(Op::ExecApproval {
|
||||||
|
id: event.id.clone(),
|
||||||
|
decision: codex_core::protocol::ReviewDecision::Approved,
|
||||||
|
})
|
||||||
|
.await
|
||||||
|
{
|
||||||
|
error!("failed to auto-approve exec: {e}");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
EventMsg::ApplyPatchApprovalRequest(_) => {
|
||||||
|
if let Err(e) = conversation
|
||||||
|
.submit(Op::PatchApproval {
|
||||||
|
id: event.id.clone(),
|
||||||
|
decision: codex_core::protocol::ReviewDecision::Approved,
|
||||||
|
})
|
||||||
|
.await
|
||||||
|
{
|
||||||
|
error!("failed to auto-approve patch: {e}");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
_ => {}
|
||||||
|
}
|
||||||
|
}
|
||||||
let shutdown: CodexStatus = event_processor.process_event(event);
|
let shutdown: CodexStatus = event_processor.process_event(event);
|
||||||
match shutdown {
|
match shutdown {
|
||||||
CodexStatus::Running => continue,
|
CodexStatus::Running => continue,
|
||||||
|
|||||||
81
codex-rs/exec/tests/suite/approve_all.rs
Normal file
81
codex-rs/exec/tests/suite/approve_all.rs
Normal file
@@ -0,0 +1,81 @@
|
|||||||
|
#![cfg(not(target_os = "windows"))]
|
||||||
|
#![allow(clippy::expect_used, clippy::unwrap_used)]
|
||||||
|
|
||||||
|
use anyhow::Result;
|
||||||
|
use core_test_support::responses;
|
||||||
|
use core_test_support::responses::ev_assistant_message;
|
||||||
|
use core_test_support::responses::ev_completed;
|
||||||
|
use core_test_support::responses::ev_function_call;
|
||||||
|
use core_test_support::responses::ev_response_created;
|
||||||
|
use core_test_support::responses::mount_sse_sequence;
|
||||||
|
use core_test_support::responses::sse;
|
||||||
|
use core_test_support::skip_if_no_network;
|
||||||
|
use core_test_support::test_codex_exec::test_codex_exec;
|
||||||
|
use serde_json::Value;
|
||||||
|
use serde_json::json;
|
||||||
|
|
||||||
|
async fn run_exec_with_args(args: &[&str]) -> Result<String> {
|
||||||
|
let test = test_codex_exec();
|
||||||
|
|
||||||
|
let call_id = "exec-approve";
|
||||||
|
let exec_args = json!({
|
||||||
|
"command": [
|
||||||
|
if cfg!(windows) { "cmd.exe" } else { "/bin/sh" },
|
||||||
|
if cfg!(windows) { "/C" } else { "-lc" },
|
||||||
|
"echo approve-all-ok",
|
||||||
|
],
|
||||||
|
"timeout_ms": 1500,
|
||||||
|
"with_escalated_permissions": true
|
||||||
|
});
|
||||||
|
|
||||||
|
let response_streams = vec![
|
||||||
|
sse(vec![
|
||||||
|
ev_response_created("resp-1"),
|
||||||
|
ev_function_call(call_id, "shell", &serde_json::to_string(&exec_args)?),
|
||||||
|
ev_completed("resp-1"),
|
||||||
|
]),
|
||||||
|
sse(vec![
|
||||||
|
ev_assistant_message("msg-1", "done"),
|
||||||
|
ev_completed("resp-2"),
|
||||||
|
]),
|
||||||
|
];
|
||||||
|
|
||||||
|
let server = responses::start_mock_server().await;
|
||||||
|
let mock = mount_sse_sequence(&server, response_streams).await;
|
||||||
|
|
||||||
|
test.cmd_with_server(&server).args(args).assert().success();
|
||||||
|
|
||||||
|
let requests = mock.requests();
|
||||||
|
assert!(requests.len() >= 2, "expected at least two responses POSTs");
|
||||||
|
let item = requests[1].function_call_output(call_id);
|
||||||
|
let output_str = item
|
||||||
|
.get("output")
|
||||||
|
.and_then(Value::as_str)
|
||||||
|
.expect("function_call_output.output should be a string");
|
||||||
|
|
||||||
|
Ok(output_str.to_string())
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Setting `features.approve_all=true` should switch to auto-approvals.
|
||||||
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
|
async fn approve_all_auto_accepts_exec() -> Result<()> {
|
||||||
|
skip_if_no_network!(Ok(()));
|
||||||
|
|
||||||
|
let output = run_exec_with_args(&[
|
||||||
|
"--skip-git-repo-check",
|
||||||
|
"-c",
|
||||||
|
"features.approve_all=true",
|
||||||
|
"train",
|
||||||
|
])
|
||||||
|
.await?;
|
||||||
|
assert!(
|
||||||
|
output.contains("Exit code: 0"),
|
||||||
|
"expected Exit code: 0 in output: {output}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
output.contains("approve-all-ok"),
|
||||||
|
"expected command output in response: {output}"
|
||||||
|
);
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
@@ -1,5 +1,6 @@
|
|||||||
// Aggregates all former standalone integration tests as modules.
|
// Aggregates all former standalone integration tests as modules.
|
||||||
mod apply_patch;
|
mod apply_patch;
|
||||||
|
mod approve_all;
|
||||||
mod auth_env;
|
mod auth_env;
|
||||||
mod originator;
|
mod originator;
|
||||||
mod output_schema;
|
mod output_schema;
|
||||||
|
|||||||
Reference in New Issue
Block a user