-
Notifications
You must be signed in to change notification settings - Fork 6.3k
fix(core/shell): unify Windows bash detection with which-resolved path #6328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ test(core/shell): add ignored WSL scenario tests to highlight run vs which bash mismatch and validate new detection
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@LaelLuo thanks for filing the new issue. Could you please fix the lint (clippy cargo) issues so the CI actions pass? Thanks! |
|
@etraut-openai Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find - thanks for the contribution! Rather than add an ignored test, I'd prefer to add some assertion we can run consistently, if possible.
|
Thanks for the suggestion! I've replaced the ignored WSL-only test with a deterministic one. The new |
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Will merge this later today / early tomorrow. We are currently holding off on some changes due to the launch gpt-5.1 and gpt-5.1-codex
|
I have a question. I saw the commit 7b027e7. It seems to be part of your plan. Does this mean that fallback shell will no longer be supported?If it is no longer supported, does that mean this PR is no longer needed? |
|
I'd like to push to get this merged. @LaelLuo, could you please resolve the remaining merge conflict? Assuming that CI is green at that point, I think we're good to go. |
Summary
Make Windows bash fallback detection consistent and predictable by using which::which("bash.exe") to resolve a concrete path and validate that exact path with --version. This ensures "detection source == returned path", eliminating run/which divergence.
Why
On Windows, process execution resolution (current dir → System32 → Windows dir → PATH → App Paths / execution aliases) differs from PATH-based resolution (which).
In WSL + Git Bash setups, this leads to running System32's WSL
bash.exewhilewhichresolves Git Bash (or vice versa), causing inconsistent behavior and confusing logs (see #3159).Returning the which-resolved and validated path ensures "detection source == returned path", eliminating run/which divergence.
How
Windows
default_user_shell:which::which("bash.exe").--versionagainst that exact path.PowerShellConfig.bash_exe_fallback = Some(path)only if it succeeds; otherwiseNone.pwsh.exe, elsepowershell.exe).Tests:
bash.exe --version<which("bash.exe")> --versionSystem32\bash.exein PATH.Validation
cargo test -p codex-core.where bash.exe(or PowerShellGet-Command bash.exe -All).cargo test -p codex-core shell::tests_windows::test_bash_run_vs_which_with_target_bash_priority -- --nocapturebash_exe_fallbackalways equals the validated which-resolved path; no run/which divergence.Impact
Risks
bash.exewhile PATH points elsewhere. This is intentional, documented, and controllable via PATH ordering.Related Issue
Closes #3159
Note
This is a resubmission of PR #4084. The previous PR was closed after a force push to the branch. This PR includes all the same changes and provides an effective fix for the issue.