fix(build): prefer usable POSIX shells for Windows bundling#45860
fix(build): prefer usable POSIX shells for Windows bundling#45860henrytien wants to merge 1 commit intoopenclaw:mainfrom
Conversation
![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.
Reviewed commit: 12a48f61c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| candidates.push(path.join(entry, "sh.exe")); | ||
| candidates.push(path.join(entry, "bash.exe")); | ||
| candidates.push(path.join(entry, "sh")); | ||
| candidates.push(path.join(entry, "bash")); |
There was a problem hiding this comment.
The new shell resolver adds sh candidates before bash, so after package.json routes canvas:a2ui:bundle through this wrapper, common Linux/macOS environments pick /bin/sh first. That breaks scripts/bundle-a2ui.sh because it relies on Bash-specific behavior (set -o pipefail, arrays, ${BASH_SOURCE[0]}), causing pnpm build to fail before the TypeScript build step whenever /bin/sh is not Bash (e.g., dash).
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes Windows Key observations:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: scripts/run-bash.mjs
Line: 96-98
Comment:
**Error message recommends Cygwin but Cygwin paths are filtered**
The error message on line 97 tells users to "Install Git Bash, MSYS2, or Cygwin" but `resolvePosixShell` explicitly skips any PATH entry whose normalized form contains `\cygwin` (line 78). A typical Cygwin 64-bit installation adds `C:\cygwin64\bin` to PATH, which contains `\cygwin` as a substring and is therefore silently skipped. A user who follows this advice and installs Cygwin will get the exact same error again.
The message should only recommend shells that will actually be found:
```suggestion
console.error("Unable to find a usable POSIX shell on PATH.");
console.error("Install Git Bash or MSYS2 and retry.");
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: scripts/run-bash.mjs
Line: 122
Comment:
**Default exit code 1 on signal-killed subprocess**
`result.status ?? 1` correctly returns 0 when the subprocess exits normally and 1 when it is killed by a signal (where `result.status` is `null`). However, `result.signal` carries the signal name in that case (e.g. `"SIGTERM"`), and the caller receives no indication of what happened. Consider logging the signal name before returning so build output is easier to diagnose:
```suggestion
if (result.signal) {
console.error(`Shell process killed by signal: ${result.signal}`);
}
return result.status ?? 1;
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 12a48f6 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| console.error("Unable to find a usable POSIX shell on PATH."); | ||
| console.error("Install Git Bash, MSYS2, or Cygwin and retry."); | ||
| return 1; |
There was a problem hiding this comment.
Error message recommends Cygwin but Cygwin paths are filtered
The error message on line 97 tells users to "Install Git Bash, MSYS2, or Cygwin" but resolvePosixShell explicitly skips any PATH entry whose normalized form contains \cygwin (line 78). A typical Cygwin 64-bit installation adds C:\cygwin64\bin to PATH, which contains \cygwin as a substring and is therefore silently skipped. A user who follows this advice and installs Cygwin will get the exact same error again.
The message should only recommend shells that will actually be found:
| console.error("Unable to find a usable POSIX shell on PATH."); | |
| console.error("Install Git Bash, MSYS2, or Cygwin and retry."); | |
| return 1; | |
| console.error("Unable to find a usable POSIX shell on PATH."); | |
| console.error("Install Git Bash or MSYS2 and retry."); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/run-bash.mjs
Line: 96-98
Comment:
**Error message recommends Cygwin but Cygwin paths are filtered**
The error message on line 97 tells users to "Install Git Bash, MSYS2, or Cygwin" but `resolvePosixShell` explicitly skips any PATH entry whose normalized form contains `\cygwin` (line 78). A typical Cygwin 64-bit installation adds `C:\cygwin64\bin` to PATH, which contains `\cygwin` as a substring and is therefore silently skipped. A user who follows this advice and installs Cygwin will get the exact same error again.
The message should only recommend shells that will actually be found:
```suggestion
console.error("Unable to find a usable POSIX shell on PATH.");
console.error("Install Git Bash or MSYS2 and retry.");
```
How can I resolve this? If you propose a fix, please make it concise.| return 1; | ||
| } | ||
|
|
||
| return result.status ?? 1; |
There was a problem hiding this comment.
Default exit code 1 on signal-killed subprocess
result.status ?? 1 correctly returns 0 when the subprocess exits normally and 1 when it is killed by a signal (where result.status is null). However, result.signal carries the signal name in that case (e.g. "SIGTERM"), and the caller receives no indication of what happened. Consider logging the signal name before returning so build output is easier to diagnose:
| return result.status ?? 1; | |
| if (result.signal) { | |
| console.error(`Shell process killed by signal: ${result.signal}`); | |
| } | |
| return result.status ?? 1; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/run-bash.mjs
Line: 122
Comment:
**Default exit code 1 on signal-killed subprocess**
`result.status ?? 1` correctly returns 0 when the subprocess exits normally and 1 when it is killed by a signal (where `result.status` is `null`). However, `result.signal` carries the signal name in that case (e.g. `"SIGTERM"`), and the caller receives no indication of what happened. Consider logging the signal name before returning so build output is easier to diagnose:
```suggestion
if (result.signal) {
console.error(`Shell process killed by signal: ${result.signal}`);
}
return result.status ?? 1;
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
pnpm buildon Windows could fail duringcanvas:a2ui:bundlebecause barebashcould resolve to the WSL shim (C:\Windows\System32\bash.exe) instead of a usable POSIX shell.node/ roamingpnpmpaths.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
canvas:a2ui:bundlethrough the WSLbash.exeshim when a usable MSYS2/Git Bash shell is available.Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
bashresolves to the WSL shim before a usable POSIX shell.pnpm build.canvas:a2ui:bundlebefore the TypeScript build starts.Expected
pnpm buildshould use a usable POSIX shell on Windows and complete successfully when MSYS2/Git Bash is installed.Actual
pnpm buildcould invoke the WSL shim and fail before bundling/build completion.Evidence
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm vitest run test/run-bash.test.tspnpm buildpnpmand activenodelocationsReview Conversations
Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
canvas:a2ui:bundlescript inpackage.jsonpackage.jsonscripts/run-bash.mjstest/run-bash.test.tsRisks and Mitigations