Make env-prefixed npm scripts work on Windows#45380
Make env-prefixed npm scripts work on Windows#45380GMTekAI wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a Windows compatibility issue where npm scripts using inline Unix-style env assignment would fail immediately on Windows shells. The fix introduces a small Node.js helper ( Key changes:
Minor concerns flagged:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: scripts/with-env.mjs
Line: 34-43
Comment:
**Missing `error` event handler and imprecise signal exit code**
Two small robustness gaps in the spawn setup:
1. There is no `error` listener. If `spawn` fails (e.g. command not found, permission denied), Node.js will throw an unhandled exception and print a raw stack trace instead of the clean usage message. Adding an `error` handler gives a much friendlier failure mode.
2. When the child is killed by a signal (e.g. `SIGINT` from Ctrl-C), the handler exits with a hard-coded `1`. The Unix convention is `128 + signal_number` (e.g. `130` for SIGINT). Some CI systems and shell scripts inspect this exit code to distinguish a normal failure from a signal termination; returning `1` can mask that.
Suggested fix:
```suggestion
const child = spawn(command, args, {
cwd: process.cwd(),
env: childEnv,
stdio: "inherit",
shell: process.platform === "win32",
});
const SIGNALS = { SIGHUP: 1, SIGINT: 2, SIGTERM: 15, SIGKILL: 9 };
child.on("error", (err) => {
console.error(`with-env: failed to start command "${command}": ${err.message}`);
process.exit(1);
});
child.on("exit", (code, signal) => {
if (signal) {
process.exit(128 + (SIGNALS[signal] ?? 1));
}
process.exit(code ?? 1);
});
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 865fb63 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const child = spawn(command, args, { | ||
| cwd: process.cwd(), | ||
| env: childEnv, | ||
| stdio: "inherit", | ||
| shell: process.platform === "win32", | ||
| }); | ||
|
|
||
| child.on("exit", (code, signal) => { | ||
| process.exit(signal ? 1 : (code ?? 1)); | ||
| }); |
There was a problem hiding this comment.
Missing error event handler and imprecise signal exit code
Two small robustness gaps in the spawn setup:
-
There is no
errorlistener. Ifspawnfails (e.g. command not found, permission denied), Node.js will throw an unhandled exception and print a raw stack trace instead of the clean usage message. Adding anerrorhandler gives a much friendlier failure mode. -
When the child is killed by a signal (e.g.
SIGINTfrom Ctrl-C), the handler exits with a hard-coded1. The Unix convention is128 + signal_number(e.g.130for SIGINT). Some CI systems and shell scripts inspect this exit code to distinguish a normal failure from a signal termination; returning1can mask that.
Suggested fix:
| const child = spawn(command, args, { | |
| cwd: process.cwd(), | |
| env: childEnv, | |
| stdio: "inherit", | |
| shell: process.platform === "win32", | |
| }); | |
| child.on("exit", (code, signal) => { | |
| process.exit(signal ? 1 : (code ?? 1)); | |
| }); | |
| const child = spawn(command, args, { | |
| cwd: process.cwd(), | |
| env: childEnv, | |
| stdio: "inherit", | |
| shell: process.platform === "win32", | |
| }); | |
| const SIGNALS = { SIGHUP: 1, SIGINT: 2, SIGTERM: 15, SIGKILL: 9 }; | |
| child.on("error", (err) => { | |
| console.error(`with-env: failed to start command "${command}": ${err.message}`); | |
| process.exit(1); | |
| }); | |
| child.on("exit", (code, signal) => { | |
| if (signal) { | |
| process.exit(128 + (SIGNALS[signal] ?? 1)); | |
| } | |
| process.exit(code ?? 1); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/with-env.mjs
Line: 34-43
Comment:
**Missing `error` event handler and imprecise signal exit code**
Two small robustness gaps in the spawn setup:
1. There is no `error` listener. If `spawn` fails (e.g. command not found, permission denied), Node.js will throw an unhandled exception and print a raw stack trace instead of the clean usage message. Adding an `error` handler gives a much friendlier failure mode.
2. When the child is killed by a signal (e.g. `SIGINT` from Ctrl-C), the handler exits with a hard-coded `1`. The Unix convention is `128 + signal_number` (e.g. `130` for SIGINT). Some CI systems and shell scripts inspect this exit code to distinguish a normal failure from a signal termination; returning `1` can mask that.
Suggested fix:
```suggestion
const child = spawn(command, args, {
cwd: process.cwd(),
env: childEnv,
stdio: "inherit",
shell: process.platform === "win32",
});
const SIGNALS = { SIGHUP: 1, SIGINT: 2, SIGTERM: 15, SIGKILL: 9 };
child.on("error", (err) => {
console.error(`with-env: failed to start command "${command}": ${err.message}`);
process.exit(1);
});
child.on("exit", (code, signal) => {
if (signal) {
process.exit(128 + (SIGNALS[signal] ?? 1));
}
process.exit(code ?? 1);
});
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 5934aa1. I updated scripts/with-env.mjs to handle spawn errors cleanly and to return signal-style exit codes instead of collapsing signaled exits to code 1.
![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: 865fb63980
ℹ️ 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".
scripts/with-env.mjs
Outdated
| }); | ||
|
|
||
| child.on("exit", (code, signal) => { | ||
| process.exit(signal ? 1 : (code ?? 1)); |
There was a problem hiding this comment.
When the wrapped command exits due to a signal, this wrapper always returns exit code 1 instead of preserving the signal semantics (typically 128 + signal from the shell path). For scripts like gateway:dev/test:*, this changes behavior from the previous inline-env form and can misclassify interrupted runs (SIGINT/SIGTERM) as generic failures in tooling that distinguishes cancellation from test failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5934aa1. I updated scripts/with-env.mjs to handle spawn errors cleanly and to return signal-style exit codes instead of collapsing signaled exits to code 1.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5934aa1af3
ℹ️ 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".
scripts/with-env.mjs
Outdated
|
|
||
| child.on("exit", (code, signal) => { | ||
| if (signal) { | ||
| process.exit(128 + (SIGNAL_EXIT_CODES[signal] ?? 1)); |
There was a problem hiding this comment.
with-env only maps four signal names, so when a child exits from any other signal (for example SIGQUIT) this wrapper returns 129 via the ?? 1 fallback instead of 128 + signal_number (131 for SIGQUIT). That changes failure classification versus prior shell behavior and can misreport interrupted runs in tooling that distinguishes specific signal terminations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ebdc245. I switched with-env to use Node's built-in os.constants.signals map so signal-style exit codes are preserved for all supported signals, not just a hand-picked subset.
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
pnpm gateway:devfailed immediately on Windows before OpenClaw could even start.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Windows users can now run env-prefixed npm scripts like
pnpm gateway:devwithout the shell failing on inline env assignment.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
pnpm install.pnpm gateway:dev.Expected
Actual
'OPENCLAW_SKIP_CHANNELS' is not recognized as an internal or external commandEvidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm gateway:devafter the change and confirmed it no longer failed at script startup.Review Conversations
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoFailure Recovery (if this breaks)
package.json.package.json,scripts/with-env.mjsRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.