🤖 feat: improve Windows bash runtime detection with WSL support #927
+738
−48
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add BashRuntime types (GitBashRuntime, WslRuntime, UnixBashRuntime) - Detect available runtimes: WSL (preferred) and Git for Windows (fallback) - Automatic Windows path translation for WSL (C:\ -> /mnt/c/) - cwd embedded in script via 'cd' since WSL needs Linux paths - Use -- separator to avoid WSL arg parsing issues - Use absolute paths to bash for each runtime - Always wrap commands in bash -c for consistent behavior - Update BashExecutionService, execAsync, and LocalBaseRuntime - Add comprehensive tests for runtime detection and path translation _Generated with mux_
- Pass bash script via stdin instead of command-line args to avoid PowerShell mangling special characters like %(refname:short) - Add comprehensive PowerShell path detection (pwsh + Windows PowerShell) - Add stdin field to SpawnConfig interface for callers to pipe to process - Update all spawn sites to handle stdin content This approach completely sidesteps command-line escaping issues because the script content never touches PowerShell's argument parser.
![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".
src/node/utils/main/bashPath.ts
Outdated
Comment on lines
89
to
93
| return command.replace( | ||
| /(["']?)([a-zA-Z]):[/\\]([^"'\s]*)\1/g, | ||
| (_match: string, quote: string, drive: string, rest: string) => { | ||
| const wslPath = `/mnt/${drive.toLowerCase()}/${rest.replace(/\\/g, "/")}`; | ||
| return quote ? `${quote}${wslPath}${quote}` : wslPath; |
There was a problem hiding this comment.
translateWindowsPathsInCommand only matches drive paths up to the first whitespace because the regex uses [^"'\s]* (lines 89-93), so a quoted path like "C:\\Users\\John Doe\\repo" is rewritten to "/mnt/c/Users/John Doe\\repo" with the remainder left in Windows form. When WSL is chosen as the preferred runtime, any script that references a Windows path with spaces now receives a broken path and the command fails. The matcher should allow spaces inside quoted paths so the conversion to /mnt/... is complete.
Useful? React with 👍 / 👎.
PowerShell's $input automatic variable doesn't work reliably with -Command because it's designed for pipeline input in functions/filters, not for reading stdin directly. Use [Console]::In.ReadToEnd() instead.
Fixed regex to properly handle paths like "C:\Users\John Doe\repo": - Double-quoted paths: match content up to closing quote (allows spaces) - Single-quoted paths: match content up to closing quote (allows spaces) - Unquoted paths: match up to whitespace (no spaces allowed) Added 3 new tests for paths with spaces.
Stdin piping to PowerShell didn't work reliably ($input and [Console]::In both failed). Instead, use base64 encoding to embed the script safely in the PowerShell command line: 1. Base64 encode the bash script (completely avoids escaping issues) 2. PowerShell decodes it with [System.Text.Encoding]::UTF8.GetString() 3. Echo the decoded script and pipe to WSL bash This approach is robust because base64 strings only contain [A-Za-z0-9+/=] which have no special meaning in PowerShell. Also removes the now-unused stdin field from SpawnConfig.
When using 'echo $s | wsl bash', the output from bash wasn't being captured by PowerShell. Change to 'wsl bash -c $s' which passes the script as an argument and properly captures stdout/stderr.
Without quotes, PowerShell expands $s and bash only receives the first word as the -c argument. The rest becomes positional args to bash. Use escaped double quotes (\`"$s\`") so the entire decoded script is passed as a single argument to bash -c.
The previous approach had PowerShell decode base64 then pass to bash -c, but the decoded script contained double quotes that conflicted with the outer quoting. New approach: pass base64 string directly to bash which decodes it: wsl bash -c 'echo BASE64 | base64 -d | bash' This works because: 1. Base64 only contains [A-Za-z0-9+/=] - no special chars 2. The decode happens inside bash, so no quoting issues 3. Single quotes pass the string literally through PowerShell to WSL
ibetitsmike
force-pushed
the
bash-runtime-detection-windows
branch
from
b4ba586 to
16a2265
Compare
ibetitsmike
force-pushed
the
bash-runtime-detection-windows
branch
from
16a2265 to
fc250b6
Compare
On Windows, detached:true creates a separate console window which interferes with output capture when combined with PowerShell's -WindowStyle Hidden. The detached option is primarily needed on Unix for process group management. On Windows with the PowerShell wrapper, we don't need it and it actively breaks stdout/stderr capture.
Scripts containing 'exec </dev/null' would break when piped to bash via 'echo BASE64 | base64 -d | bash' because the stdin redirection would cut off bash's ability to read the rest of the script. Solution: Use 'eval "$(echo BASE64 | base64 -d)"' which captures the entire decoded script via command substitution before executing it. This way stdin redirection in the script doesn't affect script reading.
eval "$(...)" had nested quote conflicts with commands containing double quotes like --format="%(refname:short)". Use process substitution instead: bash <(echo BASE64 | base64 -d) This creates a file descriptor containing the decoded script, so: - Bash reads script from file descriptor (not stdin or quoted arg) - No quoting conflicts because script content isn't in quotes - Script can still redirect stdin freely (exec </dev/null works)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Improves bash runtime detection on Windows with proper WSL support:
C:\Users\...→/mnt/c/Users/...)Key implementation details
detectBashRuntimes()returns all available runtimes with a preferred choice (WSL > Git for Windows)getSpawnConfig()generates appropriate spawn parameters for each runtime type%(refname:short)-WindowStyle Hiddenin PowerShell to prevent console window flashTesting
Generated with
mux