From f3989f609243c741ad6963dd091660dd9f256189 Mon Sep 17 00:00:00 2001 From: Robby He <448523760@qq.com> Date: Wed, 3 Dec 2025 13:49:25 +0800 Subject: [PATCH] =?UTF-8?q?fix(unified=5Fexec):=20use=20platform=20default?= =?UTF-8?q?=20shell=20when=20unified=5Fexec=20shell=E2=80=A6=20(#7486)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # 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, #[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, #[serde(default)] with_escalated_permissions: Option, #[serde(default)] justification: Option, } ``` 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 { 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, #[serde(default)] shell: Option, #[serde(default = "default_login")] login: bool, #[serde(default = "default_exec_yield_time_ms")] yield_time_ms: u64, #[serde(default)] max_output_tokens: Option, #[serde(default)] with_escalated_permissions: Option, #[serde(default)] justification: Option, } ``` Here, the absence of `shell` in the JSON is represented as `shell: None`, rather than a hard-coded string value. --- .../core/src/tools/handlers/unified_exec.rs | 78 +++++++++++++++++-- codex-rs/core/tests/suite/unified_exec.rs | 58 +++++++------- 2 files changed, 97 insertions(+), 39 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 344b4afaf..4c943c628 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -6,6 +6,7 @@ use crate::protocol::EventMsg; use crate::protocol::ExecCommandOutputDeltaEvent; use crate::protocol::ExecCommandSource; use crate::protocol::ExecOutputStream; +use crate::shell::default_user_shell; use crate::shell::get_shell_by_model_provided_path; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; @@ -31,8 +32,8 @@ struct ExecCommandArgs { cmd: String, #[serde(default)] workdir: Option, - #[serde(default = "default_shell")] - shell: String, + #[serde(default)] + shell: Option, #[serde(default = "default_login")] login: bool, #[serde(default = "default_exec_yield_time_ms")] @@ -65,10 +66,6 @@ fn default_write_stdin_yield_time_ms() -> u64 { 250 } -fn default_shell() -> String { - "/bin/bash".to_string() -} - fn default_login() -> bool { true } @@ -257,7 +254,12 @@ impl ToolHandler for UnifiedExecHandler { } fn get_command(args: &ExecCommandArgs) -> Vec { - let shell = get_shell_by_model_provided_path(&PathBuf::from(args.shell.clone())); + let shell = if let Some(shell_str) = &args.shell { + get_shell_by_model_provided_path(&PathBuf::from(shell_str)) + } else { + default_user_shell() + }; + shell.derive_exec_args(&args.cmd, args.login) } @@ -289,3 +291,65 @@ fn format_response(response: &UnifiedExecResponse) -> String { sections.join("\n") } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_get_command_uses_default_shell_when_unspecified() { + let json = r#"{"cmd": "echo hello"}"#; + + let args: ExecCommandArgs = + serde_json::from_str(json).expect("deserialize ExecCommandArgs"); + + assert!(args.shell.is_none()); + + let command = get_command(&args); + + assert_eq!(command.len(), 3); + assert_eq!(command[2], "echo hello"); + } + + #[test] + fn test_get_command_respects_explicit_bash_shell() { + let json = r#"{"cmd": "echo hello", "shell": "/bin/bash"}"#; + + let args: ExecCommandArgs = + serde_json::from_str(json).expect("deserialize ExecCommandArgs"); + + assert_eq!(args.shell.as_deref(), Some("/bin/bash")); + + let command = get_command(&args); + + assert_eq!(command[2], "echo hello"); + } + + #[test] + fn test_get_command_respects_explicit_powershell_shell() { + let json = r#"{"cmd": "echo hello", "shell": "powershell"}"#; + + let args: ExecCommandArgs = + serde_json::from_str(json).expect("deserialize ExecCommandArgs"); + + assert_eq!(args.shell.as_deref(), Some("powershell")); + + let command = get_command(&args); + + assert_eq!(command[2], "echo hello"); + } + + #[test] + fn test_get_command_respects_explicit_cmd_shell() { + let json = r#"{"cmd": "echo hello", "shell": "cmd"}"#; + + let args: ExecCommandArgs = + serde_json::from_str(json).expect("deserialize ExecCommandArgs"); + + assert_eq!(args.shell.as_deref(), Some("cmd")); + + let command = get_command(&args); + + assert_eq!(command[2], "echo hello"); + } +} diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 584882ce7..dc7bdb6b1 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -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); +}