mirror of
https://github.com/openai/codex.git
synced 2026-04-30 01:16:54 +00:00
fix(unified_exec): use platform default shell when unified_exec shell… (#7486)
# Unified Exec Shell Selection on Windows ## Problem reference issue #7466 The `unified_exec` handler currently deserializes model-provided tool calls into the `ExecCommandArgs` struct: ```rust #[derive(Debug, Deserialize)] struct ExecCommandArgs { cmd: String, #[serde(default)] workdir: Option<String>, #[serde(default = "default_shell")] shell: String, #[serde(default = "default_login")] login: bool, #[serde(default = "default_exec_yield_time_ms")] yield_time_ms: u64, #[serde(default)] max_output_tokens: Option<usize>, #[serde(default)] with_escalated_permissions: Option<bool>, #[serde(default)] justification: Option<String>, } ``` The `shell` field uses a hard-coded default: ```rust fn default_shell() -> String { "/bin/bash".to_string() } ``` When the model returns a tool call JSON that only contains `cmd` (which is the common case), Serde fills in `shell` with this default value. Later, `get_command` uses that value as if it were a model-provided shell path: ```rust fn get_command(args: &ExecCommandArgs) -> Vec<String> { let shell = get_shell_by_model_provided_path(&PathBuf::from(args.shell.clone())); shell.derive_exec_args(&args.cmd, args.login) } ``` On Unix, this usually resolves to `/bin/bash` and works as expected. However, on Windows this behavior is problematic: - The hard-coded `"/bin/bash"` is not a valid Windows path. - `get_shell_by_model_provided_path` treats this as a model-specified shell, and tries to resolve it (e.g. via `which::which("bash")`), which may or may not exist and may not behave as intended. - In practice, this leads to commands being executed under a non-default or non-existent shell on Windows (for example, WSL bash), instead of the expected Windows PowerShell or `cmd.exe`. The core of the issue is that **"model did not specify `shell`" is currently interpreted as "the model explicitly requested `/bin/bash`"**, which is both Unix-specific and wrong on Windows. ## Proposed Solution Instead of hard-coding `"/bin/bash"` into `ExecCommandArgs`, we should distinguish between: 1. **The model explicitly specifying a shell**, e.g.: ```json { "cmd": "echo hello", "shell": "pwsh" } ``` In this case, we *do* want to respect the model’s choice and use `get_shell_by_model_provided_path`. 2. **The model omitting the `shell` field entirely**, e.g.: ```json { "cmd": "echo hello" } ``` In this case, we should *not* assume `/bin/bash`. Instead, we should use `default_user_shell()` and let the platform decide. To express this distinction, we can: 1. Change `shell` to be optional in `ExecCommandArgs`: ```rust #[derive(Debug, Deserialize)] struct ExecCommandArgs { cmd: String, #[serde(default)] workdir: Option<String>, #[serde(default)] shell: Option<String>, #[serde(default = "default_login")] login: bool, #[serde(default = "default_exec_yield_time_ms")] yield_time_ms: u64, #[serde(default)] max_output_tokens: Option<usize>, #[serde(default)] with_escalated_permissions: Option<bool>, #[serde(default)] justification: Option<String>, } ``` Here, the absence of `shell` in the JSON is represented as `shell: None`, rather than a hard-coded string value.
This commit is contained in:
@@ -295,6 +295,7 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> {
|
||||
|
||||
let call_id = "uexec-begin-event";
|
||||
let args = json!({
|
||||
"shell": "bash".to_string(),
|
||||
"cmd": "/bin/echo hello unified exec".to_string(),
|
||||
"yield_time_ms": 250,
|
||||
});
|
||||
@@ -336,14 +337,8 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> {
|
||||
})
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
begin_event.command,
|
||||
vec![
|
||||
"/bin/bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"/bin/echo hello unified exec".to_string()
|
||||
]
|
||||
);
|
||||
assert_command(&begin_event.command, "-lc", "/bin/echo hello unified exec");
|
||||
|
||||
assert_eq!(begin_event.cwd, cwd.path());
|
||||
|
||||
wait_for_event(&codex, |event| matches!(event, EventMsg::TaskComplete(_))).await;
|
||||
@@ -782,6 +777,7 @@ async fn unified_exec_emits_begin_for_write_stdin() -> Result<()> {
|
||||
|
||||
let open_call_id = "uexec-open-for-begin";
|
||||
let open_args = json!({
|
||||
"shell": "bash".to_string(),
|
||||
"cmd": "bash -i".to_string(),
|
||||
"yield_time_ms": 200,
|
||||
});
|
||||
@@ -843,14 +839,7 @@ async fn unified_exec_emits_begin_for_write_stdin() -> Result<()> {
|
||||
})
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
begin_event.command,
|
||||
vec![
|
||||
"/bin/bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"bash -i".to_string()
|
||||
]
|
||||
);
|
||||
assert_command(&begin_event.command, "-lc", "bash -i");
|
||||
assert_eq!(
|
||||
begin_event.interaction_input,
|
||||
Some("echo hello".to_string())
|
||||
@@ -884,6 +873,7 @@ async fn unified_exec_emits_begin_event_for_write_stdin_requests() -> Result<()>
|
||||
|
||||
let open_call_id = "uexec-open-session";
|
||||
let open_args = json!({
|
||||
"shell": "bash".to_string(),
|
||||
"cmd": "bash -i".to_string(),
|
||||
"yield_time_ms": 250,
|
||||
});
|
||||
@@ -959,14 +949,9 @@ async fn unified_exec_emits_begin_event_for_write_stdin_requests() -> Result<()>
|
||||
.iter()
|
||||
.find(|ev| ev.call_id == open_call_id)
|
||||
.expect("missing exec_command begin");
|
||||
assert_eq!(
|
||||
open_event.command,
|
||||
vec![
|
||||
"/bin/bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"bash -i".to_string()
|
||||
]
|
||||
);
|
||||
|
||||
assert_command(&open_event.command, "-lc", "bash -i");
|
||||
|
||||
assert!(
|
||||
open_event.interaction_input.is_none(),
|
||||
"startup begin events should not include interaction input"
|
||||
@@ -977,14 +962,9 @@ async fn unified_exec_emits_begin_event_for_write_stdin_requests() -> Result<()>
|
||||
.iter()
|
||||
.find(|ev| ev.call_id == poll_call_id)
|
||||
.expect("missing write_stdin begin");
|
||||
assert_eq!(
|
||||
poll_event.command,
|
||||
vec![
|
||||
"/bin/bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"bash -i".to_string()
|
||||
]
|
||||
);
|
||||
|
||||
assert_command(&poll_event.command, "-lc", "bash -i");
|
||||
|
||||
assert!(
|
||||
poll_event.interaction_input.is_none(),
|
||||
"poll begin events should omit interaction input"
|
||||
@@ -2121,3 +2101,17 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> {
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn assert_command(command: &[String], expected_args: &str, expected_cmd: &str) {
|
||||
assert_eq!(command.len(), 3);
|
||||
let shell_path = &command[0];
|
||||
assert!(
|
||||
shell_path == "/bin/bash"
|
||||
|| shell_path == "/usr/bin/bash"
|
||||
|| shell_path == "/usr/local/bin/bash"
|
||||
|| shell_path.ends_with("/bash"),
|
||||
"unexpected bash path: {shell_path}"
|
||||
);
|
||||
assert_eq!(command[1], expected_args);
|
||||
assert_eq!(command[2], expected_cmd);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user