fix(windows) shell_command on windows, minor parsing #6811
Conversation
|
|
||
| /// Extracts the shell and script from a command, regardless of platform | ||
| pub fn extract_shell_command(command: &[String]) -> Option<(&str, &str)> { | ||
| extract_bash_command(command).or_else(|| extract_powershell_command(command)) |
There was a problem hiding this comment.
As noted in the PR description, I don't feel strongly about prioritizing cross-platform shell support in this PR. If we want to keep things simple, we can branch here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| stage: Stage::Experimental, | ||
| default_enabled: false, | ||
| default_enabled: cfg!(windows), | ||
| }, |
There was a problem hiding this comment.
Because this now evaluates to true when the binary is built for Windows, Features::with_defaults() will enable ShellCommandTool by default on that platform. The spec tests in codex-rs/core/src/tools/spec.rs (test_build_specs_gpt5_codex_default, test_build_specs_gpt51_codex_default, test_model_tools_gpt5_codex_default, etc.) still assert that the default tool list contains "shell", so on Windows they now observe "shell_command" instead and the assert_eq! calls fail. Please update those expectations (e.g. branch on cfg!(windows)) so the test suite passes on Windows.
Useful? React with 👍 / 👎.
dylan-hurd-oai
force-pushed
the
dh--shell-command-windows
branch
from
af19be3 to
3f891f2
Compare
Summary
Enables shell_command for windows users, and starts adding some basic command parsing here, to at least remove powershell prefixes. We'll follow this up with command parsing but I wanted to land this change separately with some basic UX.
NOTE: This implementation parses bash and powershell on both platforms. In theory this is possible, since you can use git bash on windows or powershell on linux. In practice, this may not be worth the complexity of supporting, so I don't feel strongly about the current approach vs. platform-specific branching.
Testing