build(docs): make check:docs PowerShell-compatible#44824
Open
tianxingleo wants to merge 1 commit intoopenclaw:mainfrom
Open
build(docs): make check:docs PowerShell-compatible#44824tianxingleo wants to merge 1 commit intoopenclaw:mainfrom
tianxingleo wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Contributor
Greptile SummaryThis PR replaces the Key concern:
Minor:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: scripts/format-docs.mjs
Line: 64
Comment:
**`spawnSync` won't resolve `.cmd` shims on Windows**
`oxfmt` is an npm devDependency. When installed by npm/pnpm on Windows, the binary is exposed via a `node_modules/.bin/oxfmt.cmd` batch-file shim — there is no `oxfmt.exe` in that directory. Node.js's `child_process.spawnSync` on Windows calls `CreateProcess` directly, which only searches for `.com`/`.exe` extensions and **does not** honor `PATHEXT`, so it will fail to locate `oxfmt.cmd` even if `node_modules/.bin` is on `PATH`. The result would be `ENOENT` (captured silently in `result.error`), causing the script to exit with code 1 with no useful diagnostic — the very Windows compatibility problem this PR is meant to fix.
Two straightforward options:
**Option A — `shell: true` on Windows** (minimal change):
```suggestion
const result = spawnSync("oxfmt", [mode, ...chunk], {
stdio: "inherit",
shell: process.platform === "win32",
});
```
**Option B — add `cross-spawn`** as a dev dependency and replace `spawnSync` with `crossSpawn.sync`, which handles the `.cmd` lookup transparently on all platforms.
Note: the `spawnSync("git", ...)` call in `runGitList` is unaffected because `git` is distributed as a native `git.exe` on Windows.
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/format-docs.mjs
Line: 30-32
Comment:
**Silent failure when command is not found**
When `spawnSync` fails to launch the child process (e.g., `git` or `oxfmt` not in `PATH`), Node.js sets `result.status = null` and `result.error` to an `Error` with code `ENOENT`. The current check `result.status !== 0` correctly catches this case (`null !== 0` is `true`), but the error is discarded silently — the user just sees a bare exit code 1 with no explanation.
Consider logging `result.error` before exiting:
```suggestion
if (result.status !== 0) {
if (result.error) {
console.error(`format:docs: failed to run git: ${result.error.message}`);
}
process.exit(result.status ?? 1);
}
```
The same pattern applies to the `spawnSync` call in `runOxfmt` (line 64–68).
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 3af3ebd |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const chunks = chunkFiles(files); | ||
|
|
||
| for (const chunk of chunks) { | ||
| const result = spawnSync("oxfmt", [mode, ...chunk], { |
Contributor
There was a problem hiding this comment.
spawnSync won't resolve .cmd shims on Windows
oxfmt is an npm devDependency. When installed by npm/pnpm on Windows, the binary is exposed via a node_modules/.bin/oxfmt.cmd batch-file shim — there is no oxfmt.exe in that directory. Node.js's child_process.spawnSync on Windows calls CreateProcess directly, which only searches for .com/.exe extensions and does not honor PATHEXT, so it will fail to locate oxfmt.cmd even if node_modules/.bin is on PATH. The result would be ENOENT (captured silently in result.error), causing the script to exit with code 1 with no useful diagnostic — the very Windows compatibility problem this PR is meant to fix.
Two straightforward options:
Option A — shell: true on Windows (minimal change):
Suggested change
| const result = spawnSync("oxfmt", [mode, ...chunk], { | |
| const result = spawnSync("oxfmt", [mode, ...chunk], { | |
| stdio: "inherit", | |
| shell: process.platform === "win32", | |
| }); |
Option B — add cross-spawn as a dev dependency and replace spawnSync with crossSpawn.sync, which handles the .cmd lookup transparently on all platforms.
Note: the spawnSync("git", ...) call in runGitList is unaffected because git is distributed as a native git.exe on Windows.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/format-docs.mjs
Line: 64
Comment:
**`spawnSync` won't resolve `.cmd` shims on Windows**
`oxfmt` is an npm devDependency. When installed by npm/pnpm on Windows, the binary is exposed via a `node_modules/.bin/oxfmt.cmd` batch-file shim — there is no `oxfmt.exe` in that directory. Node.js's `child_process.spawnSync` on Windows calls `CreateProcess` directly, which only searches for `.com`/`.exe` extensions and **does not** honor `PATHEXT`, so it will fail to locate `oxfmt.cmd` even if `node_modules/.bin` is on `PATH`. The result would be `ENOENT` (captured silently in `result.error`), causing the script to exit with code 1 with no useful diagnostic — the very Windows compatibility problem this PR is meant to fix.
Two straightforward options:
**Option A — `shell: true` on Windows** (minimal change):
```suggestion
const result = spawnSync("oxfmt", [mode, ...chunk], {
stdio: "inherit",
shell: process.platform === "win32",
});
```
**Option B — add `cross-spawn`** as a dev dependency and replace `spawnSync` with `crossSpawn.sync`, which handles the `.cmd` lookup transparently on all platforms.
Note: the `spawnSync("git", ...)` call in `runGitList` is unaffected because `git` is distributed as a native `git.exe` on Windows.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+30
to
+32
| if (result.status !== 0) { | ||
| process.exit(result.status ?? 1); | ||
| } |
Contributor
There was a problem hiding this comment.
Silent failure when command is not found
When spawnSync fails to launch the child process (e.g., git or oxfmt not in PATH), Node.js sets result.status = null and result.error to an Error with code ENOENT. The current check result.status !== 0 correctly catches this case (null !== 0 is true), but the error is discarded silently — the user just sees a bare exit code 1 with no explanation.
Consider logging result.error before exiting:
Suggested change
| if (result.status !== 0) { | |
| process.exit(result.status ?? 1); | |
| } | |
| if (result.status !== 0) { | |
| if (result.error) { | |
| console.error(`format:docs: failed to run git: ${result.error.message}`); | |
| } | |
| process.exit(result.status ?? 1); | |
| } |
The same pattern applies to the spawnSync call in runOxfmt (line 64–68).
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/format-docs.mjs
Line: 30-32
Comment:
**Silent failure when command is not found**
When `spawnSync` fails to launch the child process (e.g., `git` or `oxfmt` not in `PATH`), Node.js sets `result.status = null` and `result.error` to an `Error` with code `ENOENT`. The current check `result.status !== 0` correctly catches this case (`null !== 0` is `true`), but the error is discarded silently — the user just sees a bare exit code 1 with no explanation.
Consider logging `result.error` before exiting:
```suggestion
if (result.status !== 0) {
if (result.error) {
console.error(`format:docs: failed to run git: ${result.error.message}`);
}
process.exit(result.status ?? 1);
}
```
The same pattern applies to the `spawnSync` call in `runOxfmt` (line 64–68).
How can I resolve this? If you propose a fix, please make it concise.
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
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
Fixes #44293.
pnpm check:docspreviously depended onxargs, which is unavailable in native Windows PowerShell by default.This PR replaces the shell pipeline with a cross-platform Node script that:
git ls-files -zoxfmtdirectly withoutxargsChanges
scripts/format-docs.mjsformat:docs->node scripts/format-docs.mjs --writeformat:docs:check->node scripts/format-docs.mjs --checkValidation
node --check scripts/format-docs.mjs