fix(unified_exec): use platform default shell when unified_exec shell…

[448523760]

Unified Exec Shell Selection on Windows

Problem

reference issue #7466

The unified_exec handler currently deserializes model-provided tool calls into the ExecCommandArgs struct:

#[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:

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:

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.:

   {
     "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.:

   {
     "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:

   #[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.

[chatgpt-codex-connector]

Note

To use Codex here, create a Codex account and connect to github.

[jif-oai]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Hooray!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[etraut-openai]

@448523760, thanks for the contribution! Good analysis and fix. The PR is looking pretty good, but a few of the tests are failing. Please take a look.

[etraut-openai]

@448523760, looks like there are some additional test failures.

[etraut-openai]

Looks good! Merged.