-
Notifications
You must be signed in to change notification settings - Fork 336
fix: Add Windows platform detection to bash tool #280
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
kevinkre
commented
Oct 22, 2025
- Import platform module for OS detection
- Create getShellCommand() helper for platform-specific shell selection
- Use cmd.exe on Windows, /bin/bash on Unix
- Update BashSession.initialize() and executeBashCommand()
- Maintains backwards compatibility with Unix systems
- Import platform module for OS detection - Create getShellCommand() helper for platform-specific shell selection - Use cmd.exe on Windows, /bin/bash on Unix - Update BashSession.initialize() and executeBashCommand() - Maintains backwards compatibility with Unix systems
|
@codex review this PR |
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.
💡 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".
| try { | ||
| // Start bash process with non-interactive mode to avoid prompt issues | ||
| this.process = spawn('/bin/bash', [], { | ||
| const { shell, args } = getShellCommand(); | ||
| this.process = spawn(shell, args, { | ||
| cwd: this.currentWorkingDir, |
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.
Persistent sessions spawn non-interactive shell
The new getShellCommand always adds ['-c'] (or ['/c'] on Windows) and initialize() now uses those args when starting the persistent shell. Both bash -c and cmd.exe /c immediately exit after running a command, so launching them without a command ends the process at once and the BashSession never stays active. As a result, any call to the tool with persistent: true fails on all platforms because session.executeCommand will throw "Bash session is not running". Persistent sessions need an interactive shell (bash with no -c, cmd.exe without /c or with /K).
Useful? React with 👍 / 👎.
|
@kevinkre
I recommend adjusting the helper so that persistent sessions are launched without -c//c, while one-off executions still append those flags alongside the command string. |