Improve Windows source-dev support and make scripts cross-platform#53664
Improve Windows source-dev support and make scripts cross-platform#53664Heber-Hu wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR replaces Unix-specific shell idioms in Key changes:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: scripts/prepare-git-hooks.mjs
Line: 25-31
Comment:
**`prepare` no longer silently succeeds on `git config` failure**
The original `prepare` script used `|| exit 0` at the end, which guaranteed the command always exited 0 regardless of whether `git config core.hooksPath git-hooks` succeeded or failed. The new implementation can exit non-zero when `configureHooks.status` is non-zero (e.g. if the repo is marked unsafe by git's `safe.directory` check, the `.git/config` is read-only in CI, or any other git configuration error).
A failed `prepare` causes `pnpm install` / `npm install` to abort, which would be a regression for affected environments.
Consider returning `0` unconditionally after the `git config` call to preserve the original defensive behaviour:
```suggestion
const configureHooks = runGit(["config", "core.hooksPath", "git-hooks"], {
stdio: "inherit",
});
// Mirrors the original `|| exit 0` — always succeed so pnpm install is never blocked.
return 0;
}
```
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/deadcode-report-ci.mjs
Line: 32-42
Comment:
**CI artifact is written even when the underlying tool crashes**
`main()` always returns `0`, which is the intended parity with the original `|| true`. However, there's a subtle difference in what gets written: when `result.status !== 0` (e.g. the tool hard-crashed and produced no output), `result.stdout` and `result.stderr` may both be empty strings, so `sections.filter(Boolean)` will be empty and the file will contain just a bare `"\n"`. The old behaviour would have written an empty file.
More importantly, if `runPackageScript` itself throws before the `result` is assigned (e.g. `spawnSync` hits `ENOENT`), the top-level `catch` in the entry-point block will exit with code `1`, which *would* fail CI — an inconsistency compared to the `|| true` semantics. If you want full parity, consider wrapping the `spawnSync` call:
```suggestion
let result;
try {
result = runPackageScript(scriptName);
} catch {
result = { stdout: "", stderr: "" };
}
```
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-with-env.mjs
Line: 44-49
Comment:
**`bash` commands via `run-with-env` skip the friendly "install Git Bash" error**
`test:install:e2e:anthropic` and `test:install:e2e:openai` both delegate to `run-with-env.mjs` and then spawn `bash` directly. Unlike `run-bash-script.mjs`, this path does not probe for bash first, so on Windows without Git Bash / WSL the error will be a generic Node.js `ENOENT` rather than the actionable message.
Since `run-with-env.mjs` is a generic env-injection helper it probably shouldn't hard-code bash awareness, but the calling scripts in `package.json` could be changed to pipe through `run-bash-script.mjs` instead (passing the env separately), or `run-with-env.mjs` could accept a pre-flight hook. At a minimum, documenting the discrepancy would help future contributors.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Improve Windows source-dev support and m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const configureHooks = runGit(["config", "core.hooksPath", "git-hooks"], { | ||
| stdio: "inherit", | ||
| }); | ||
| if (!configureHooks) { | ||
| return 0; | ||
| } | ||
| return configureHooks.status ?? 0; |
There was a problem hiding this comment.
prepare no longer silently succeeds on git config failure
The original prepare script used || exit 0 at the end, which guaranteed the command always exited 0 regardless of whether git config core.hooksPath git-hooks succeeded or failed. The new implementation can exit non-zero when configureHooks.status is non-zero (e.g. if the repo is marked unsafe by git's safe.directory check, the .git/config is read-only in CI, or any other git configuration error).
A failed prepare causes pnpm install / npm install to abort, which would be a regression for affected environments.
Consider returning 0 unconditionally after the git config call to preserve the original defensive behaviour:
| const configureHooks = runGit(["config", "core.hooksPath", "git-hooks"], { | |
| stdio: "inherit", | |
| }); | |
| if (!configureHooks) { | |
| return 0; | |
| } | |
| return configureHooks.status ?? 0; | |
| const configureHooks = runGit(["config", "core.hooksPath", "git-hooks"], { | |
| stdio: "inherit", | |
| }); | |
| // Mirrors the original `|| exit 0` — always succeed so pnpm install is never blocked. | |
| return 0; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/prepare-git-hooks.mjs
Line: 25-31
Comment:
**`prepare` no longer silently succeeds on `git config` failure**
The original `prepare` script used `|| exit 0` at the end, which guaranteed the command always exited 0 regardless of whether `git config core.hooksPath git-hooks` succeeded or failed. The new implementation can exit non-zero when `configureHooks.status` is non-zero (e.g. if the repo is marked unsafe by git's `safe.directory` check, the `.git/config` is read-only in CI, or any other git configuration error).
A failed `prepare` causes `pnpm install` / `npm install` to abort, which would be a regression for affected environments.
Consider returning `0` unconditionally after the `git config` call to preserve the original defensive behaviour:
```suggestion
const configureHooks = runGit(["config", "core.hooksPath", "git-hooks"], {
stdio: "inherit",
});
// Mirrors the original `|| exit 0` — always succeed so pnpm install is never blocked.
return 0;
}
```
How can I resolve this? If you propose a fix, please make it concise.| const result = runPackageScript(scriptName); | ||
| const sections = []; | ||
| if (result.stdout) { | ||
| sections.push(result.stdout.trimEnd()); | ||
| } | ||
| if (result.stderr) { | ||
| sections.push(result.stderr.trimEnd()); | ||
| } | ||
| fs.writeFileSync(absoluteOutputPath, `${sections.filter(Boolean).join("\n")}\n`, "utf8"); | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
main() always returns 0, which is the intended parity with the original || true. However, there's a subtle difference in what gets written: when result.status !== 0 (e.g. the tool hard-crashed and produced no output), result.stdout and result.stderr may both be empty strings, so sections.filter(Boolean) will be empty and the file will contain just a bare "\n". The old behaviour would have written an empty file.
More importantly, if runPackageScript itself throws before the result is assigned (e.g. spawnSync hits ENOENT), the top-level catch in the entry-point block will exit with code 1, which would fail CI — an inconsistency compared to the || true semantics. If you want full parity, consider wrapping the spawnSync call:
| const result = runPackageScript(scriptName); | |
| const sections = []; | |
| if (result.stdout) { | |
| sections.push(result.stdout.trimEnd()); | |
| } | |
| if (result.stderr) { | |
| sections.push(result.stderr.trimEnd()); | |
| } | |
| fs.writeFileSync(absoluteOutputPath, `${sections.filter(Boolean).join("\n")}\n`, "utf8"); | |
| return 0; | |
| let result; | |
| try { | |
| result = runPackageScript(scriptName); | |
| } catch { | |
| result = { stdout: "", stderr: "" }; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/deadcode-report-ci.mjs
Line: 32-42
Comment:
**CI artifact is written even when the underlying tool crashes**
`main()` always returns `0`, which is the intended parity with the original `|| true`. However, there's a subtle difference in what gets written: when `result.status !== 0` (e.g. the tool hard-crashed and produced no output), `result.stdout` and `result.stderr` may both be empty strings, so `sections.filter(Boolean)` will be empty and the file will contain just a bare `"\n"`. The old behaviour would have written an empty file.
More importantly, if `runPackageScript` itself throws before the `result` is assigned (e.g. `spawnSync` hits `ENOENT`), the top-level `catch` in the entry-point block will exit with code `1`, which *would* fail CI — an inconsistency compared to the `|| true` semantics. If you want full parity, consider wrapping the `spawnSync` call:
```suggestion
let result;
try {
result = runPackageScript(scriptName);
} catch {
result = { stdout: "", stderr: "" };
}
```
How can I resolve this? If you propose a fix, please make it concise.| const child = spawn(command, args, { | ||
| cwd: process.cwd(), | ||
| env, | ||
| stdio: "inherit", | ||
| ...(shouldUseShellForCommand(command) ? { shell: true } : {}), | ||
| }); |
There was a problem hiding this comment.
bash commands via run-with-env skip the friendly "install Git Bash" error
test:install:e2e:anthropic and test:install:e2e:openai both delegate to run-with-env.mjs and then spawn bash directly. Unlike run-bash-script.mjs, this path does not probe for bash first, so on Windows without Git Bash / WSL the error will be a generic Node.js ENOENT rather than the actionable message.
Since run-with-env.mjs is a generic env-injection helper it probably shouldn't hard-code bash awareness, but the calling scripts in package.json could be changed to pipe through run-bash-script.mjs instead (passing the env separately), or run-with-env.mjs could accept a pre-flight hook. At a minimum, documenting the discrepancy would help future contributors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/run-with-env.mjs
Line: 44-49
Comment:
**`bash` commands via `run-with-env` skip the friendly "install Git Bash" error**
`test:install:e2e:anthropic` and `test:install:e2e:openai` both delegate to `run-with-env.mjs` and then spawn `bash` directly. Unlike `run-bash-script.mjs`, this path does not probe for bash first, so on Windows without Git Bash / WSL the error will be a generic Node.js `ENOENT` rather than the actionable message.
Since `run-with-env.mjs` is a generic env-injection helper it probably shouldn't hard-code bash awareness, but the calling scripts in `package.json` could be changed to pipe through `run-bash-script.mjs` instead (passing the env separately), or `run-with-env.mjs` could accept a pre-flight hook. At a minimum, documenting the discrepancy would help future contributors.
How can I resolve this? If you propose a fix, please make it concise.![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: 535a23dc06
ℹ️ 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".
| if (!configureHooks) { | ||
| return 0; | ||
| } | ||
| return configureHooks.status ?? 0; |
There was a problem hiding this comment.
prepare is now hard-failing when git config core.hooksPath git-hooks returns non-zero, which regresses the previous behavior that always exited 0 (... && git config ... || exit 0). In environments where .git/config is temporarily unwriteable (for example a stale .git/config.lock or restricted checkout), pnpm install now fails even though hook setup should be non-blocking; this should remain best-effort to avoid breaking installs.
Useful? React with 👍 / 👎.
|
Tested on a Windows source checkout and verified install/build plus the updated script entrypoints. Happy to make follow-up changes if maintainers want this split further or adjusted for repo conventions. |
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write
N/A. If the cause is unclear, writeUnknown.git blame, prior PR, issue, or refactor if known):Regression Test Plan (if applicable)
For bug fixes or regressions, name the smallest reliable test coverage that should have caught this. Otherwise write
N/A.User-visible / Behavior Changes
List user-visible changes (including defaults/config).
If none, write
None.Security Impact (required)
Yes/No)Yes/No)Yes/No)Yes/No)Yes/No)Yes, explain risk + mitigation:Repro + Verification
Summary
This PR improves local development on Windows for OpenClaw source checkouts and makes several common repository scripts more cross-platform.
What changed
Script improvements
gateway:devgateway:dev:resetprepareformat:docsformat:docs:checkformat:diffdeadcode:report:ci:*protocol:checkValidation
pnpm installpnpm buildpnpm openclaw --version