diff --git a/crates/cli/src/agents/codex/launch.rs b/crates/cli/src/agents/codex/launch.rs index 58676f8bc..76c8d4195 100644 --- a/crates/cli/src/agents/codex/launch.rs +++ b/crates/cli/src/agents/codex/launch.rs @@ -9,7 +9,7 @@ use crate::agents::CodingAgent; use crate::configuration::{RELAY_PLUGIN_ID, RELAY_SOURCE_PLUGIN_ID}; use crate::error::CliError; use crate::hooks::{generated_policy_hooks, transparent_hook_forward_commands}; -use crate::process::{PreparedAgentLaunch, insert_after_host}; +use crate::process::PreparedAgentLaunch; pub(crate) fn prepare(launch: &mut PreparedAgentLaunch, gateway_url: &str) -> Result<(), CliError> { let has_openai_key = std::env::var("OPENAI_API_KEY") @@ -49,10 +49,101 @@ pub(crate) fn prepare(launch: &mut PreparedAgentLaunch, gateway_url: &str) -> Re } args.push("--config".to_string()); args.push(session_hook_state_override(&hook_groups)?); - insert_after_host(&mut launch.argv, launch.host_index, args); + insert_config_in_command_scope(&mut launch.argv, launch.host_index, args); Ok(()) } +fn insert_config_in_command_scope( + argv: &mut Vec, + host_index: usize, + values: impl IntoIterator, +) { + debug_assert!(host_index < argv.len()); + // Codex accepts configuration at each clap command level, but a nested command's + // overrides replace the parent scope. Keep Relay's provider and hook overrides in the + // deepest model-running `exec` scope. Parse positions rather than searching values so a + // command such as `codex mcp remove exec` keeps the root placement. + let Some(exec_index) = command_position(argv, host_index + 1) + .filter(|&index| matches!(argv[index].as_str(), "exec" | "e")) + else { + argv.splice(host_index + 1..host_index + 1, values); + return; + }; + let insert_at = command_position(argv, exec_index + 1) + .filter(|&index| matches!(argv[index].as_str(), "fork" | "resume" | "review")) + .map_or(exec_index + 1, |index| index + 1); + argv.splice(insert_at..insert_at, values); +} + +fn command_position(argv: &[String], mut index: usize) -> Option { + while let Some(argument) = argv.get(index).map(String::as_str) { + if argument == "--" { + return None; + } + if option_takes_separate_value(argument) { + index += 2; + } else if option_is_self_contained(argument) { + index += 1; + } else { + return Some(index); + } + } + None +} + +fn option_is_self_contained(argument: &str) -> bool { + (argument.starts_with("--") && argument.contains('=')) + || (argument.starts_with('-') && !argument.starts_with("--") && argument.len() > 2) + || matches!( + argument, + "--strict-config" + | "--full-auto" + | "--oss" + | "--dangerously-bypass-approvals-and-sandbox" + | "--dangerously-bypass-hook-trust" + | "--search" + | "--no-alt-screen" + | "--skip-git-repo-check" + | "--ephemeral" + | "--ignore-user-config" + | "--ignore-rules" + | "--json" + | "-h" + | "--help" + | "-V" + | "--version" + ) +} + +fn option_takes_separate_value(argument: &str) -> bool { + matches!( + argument, + "-c" | "--config" + | "--enable" + | "--disable" + | "--remote" + | "--remote-auth-token-env" + | "-i" + | "--image" + | "-m" + | "--model" + | "--local-provider" + | "-p" + | "--profile" + | "-s" + | "--sandbox" + | "-C" + | "--cd" + | "--add-dir" + | "-a" + | "--ask-for-approval" + | "--output-schema" + | "--color" + | "-o" + | "--output-last-message" + ) +} + pub(crate) fn session_hook_state_override(generated: &Value) -> Result { let events = generated .get("hooks") diff --git a/crates/cli/tests/cli_tests.rs b/crates/cli/tests/cli_tests.rs index b6e57c2b6..608703c59 100644 --- a/crates/cli/tests/cli_tests.rs +++ b/crates/cli/tests/cli_tests.rs @@ -3935,8 +3935,7 @@ command = "codex exec" .lines() .find(|line| line.starts_with("argv = ")) .expect("dry-run output should include argv"); - assert!(argv.starts_with("argv = codex "), "{stdout}"); - assert!(argv.ends_with(" exec"), "{stdout}"); + assert!(argv.starts_with("argv = codex exec --config "), "{stdout}"); } #[test] diff --git a/crates/cli/tests/coverage/agents/launcher_tests.rs b/crates/cli/tests/coverage/agents/launcher_tests.rs index 4ba9ffce0..882c488ed 100644 --- a/crates/cli/tests/coverage/agents/launcher_tests.rs +++ b/crates/cli/tests/coverage/agents/launcher_tests.rs @@ -343,6 +343,253 @@ fn prepares_codex_config_overrides() { prepared.restore().unwrap(); } +#[test] +fn prepares_codex_config_overrides_in_exec_scope() { + let resolved = ResolvedConfig { + gateway: GatewayConfig::default(), + agents: AgentConfigs::default(), + ..ResolvedConfig::default() + }; + let prepared = PreparedAgentLaunch::new( + CodingAgent::Codex, + vec![ + "codex".into(), + "--profile".into(), + "root".into(), + "exec".into(), + "--config".into(), + "mcp_servers.example.url=\"http://127.0.0.1:9902\"".into(), + "inspect the workspace".into(), + ], + "http://127.0.0.1:1234", + &resolved, + false, + ) + .unwrap(); + + let exec_index = prepared.argv.iter().position(|arg| arg == "exec").unwrap(); + let provider_index = prepared + .argv + .iter() + .position(|arg| arg == "model_provider=\"nemo-relay-openai\"") + .unwrap(); + assert!(provider_index > exec_index); + assert!( + provider_index + < prepared + .argv + .iter() + .position(|arg| arg.starts_with("mcp_servers.example.url=")) + .unwrap() + ); +} + +#[test] +fn prepares_codex_config_overrides_after_full_auto_in_exec_scope() { + let resolved = ResolvedConfig { + gateway: GatewayConfig::default(), + agents: AgentConfigs::default(), + ..ResolvedConfig::default() + }; + let prepared = PreparedAgentLaunch::new( + CodingAgent::Codex, + vec![ + "codex".into(), + "--full-auto".into(), + "exec".into(), + "--config".into(), + "mcp_servers.example.url=\"http://127.0.0.1:9902\"".into(), + "inspect the workspace".into(), + ], + "http://127.0.0.1:1234", + &resolved, + false, + ) + .unwrap(); + + let exec_index = prepared.argv.iter().position(|arg| arg == "exec").unwrap(); + let provider_index = prepared + .argv + .iter() + .position(|arg| arg == "model_provider=\"nemo-relay-openai\"") + .unwrap(); + assert!(provider_index > exec_index); + assert!( + provider_index + < prepared + .argv + .iter() + .position(|arg| arg.starts_with("mcp_servers.example.url=")) + .unwrap() + ); +} + +#[test] +fn prepares_codex_config_overrides_in_fork_scope() { + let resolved = ResolvedConfig { + gateway: GatewayConfig::default(), + agents: AgentConfigs::default(), + ..ResolvedConfig::default() + }; + let prepared = PreparedAgentLaunch::new( + CodingAgent::Codex, + vec![ + "codex".into(), + "exec".into(), + "--model".into(), + "gpt-5.4".into(), + "fork".into(), + "0198d672-f123-4567-89ab-cdef01234567".into(), + "--config".into(), + "mcp_servers.example.url=\"http://127.0.0.1:9902\"".into(), + "continue".into(), + ], + "http://127.0.0.1:1234", + &resolved, + false, + ) + .unwrap(); + + let fork_index = prepared.argv.iter().position(|arg| arg == "fork").unwrap(); + let provider_index = prepared + .argv + .iter() + .position(|arg| arg == "model_provider=\"nemo-relay-openai\"") + .unwrap(); + assert!(provider_index > fork_index); + assert!( + provider_index + < prepared + .argv + .iter() + .position(|arg| arg == "0198d672-f123-4567-89ab-cdef01234567") + .unwrap() + ); +} + +#[test] +fn prepares_codex_config_overrides_in_resume_scope() { + let resolved = ResolvedConfig { + gateway: GatewayConfig::default(), + agents: AgentConfigs::default(), + ..ResolvedConfig::default() + }; + let prepared = PreparedAgentLaunch::new( + CodingAgent::Codex, + vec![ + "codex".into(), + "exec".into(), + "resume".into(), + "0198d672-f123-4567-89ab-cdef01234567".into(), + "--config".into(), + "mcp_servers.example.url=\"http://127.0.0.1:9902\"".into(), + "continue".into(), + ], + "http://127.0.0.1:1234", + &resolved, + false, + ) + .unwrap(); + + let resume_index = prepared + .argv + .iter() + .position(|arg| arg == "resume") + .unwrap(); + let provider_index = prepared + .argv + .iter() + .position(|arg| arg == "model_provider=\"nemo-relay-openai\"") + .unwrap(); + assert!(provider_index > resume_index); + assert!( + provider_index + < prepared + .argv + .iter() + .position(|arg| arg == "0198d672-f123-4567-89ab-cdef01234567") + .unwrap() + ); +} + +#[test] +fn prepares_codex_config_overrides_in_review_scope() { + let resolved = ResolvedConfig { + gateway: GatewayConfig::default(), + agents: AgentConfigs::default(), + ..ResolvedConfig::default() + }; + let prepared = PreparedAgentLaunch::new( + CodingAgent::Codex, + vec![ + "codex".into(), + "exec".into(), + "--model".into(), + "gpt-5.4".into(), + "review".into(), + "--config".into(), + "mcp_servers.example.url=\"http://127.0.0.1:9902\"".into(), + "inspect the workspace".into(), + ], + "http://127.0.0.1:1234", + &resolved, + false, + ) + .unwrap(); + + let review_index = prepared + .argv + .iter() + .position(|argument| argument == "review") + .unwrap(); + let provider_index = prepared + .argv + .iter() + .position(|argument| argument == "model_provider=\"nemo-relay-openai\"") + .unwrap(); + assert!(provider_index > review_index); + assert!( + provider_index + < prepared + .argv + .iter() + .position(|argument| argument.starts_with("mcp_servers.example.url=")) + .unwrap() + ); +} + +#[test] +fn prepares_codex_config_at_root_for_non_exec_command_with_exec_argument() { + let resolved = ResolvedConfig { + gateway: GatewayConfig::default(), + agents: AgentConfigs::default(), + ..ResolvedConfig::default() + }; + let prepared = PreparedAgentLaunch::new( + CodingAgent::Codex, + vec!["codex".into(), "mcp".into(), "remove".into(), "exec".into()], + "http://127.0.0.1:1234", + &resolved, + false, + ) + .unwrap(); + + assert_eq!(prepared.argv[0], "codex"); + assert_eq!(prepared.argv[1], "--config"); + let provider_index = prepared + .argv + .iter() + .position(|argument| argument == "model_provider=\"nemo-relay-openai\"") + .unwrap(); + let mcp_index = prepared + .argv + .iter() + .position(|argument| argument == "mcp") + .unwrap(); + assert!(provider_index < mcp_index); + assert_eq!(&prepared.argv[mcp_index..], ["mcp", "remove", "exec"]); +} + #[test] fn prepares_codex_config_overrides_with_versioned_trailing_slash_gateway_url() { let _guard = current_dir_lock().lock().unwrap();