Conversation
Greptile SummaryThis PR replaces the shell-based Key issues found:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: scripts/format-docs-check.js
Line: 1-5
Comment:
**`require()` will throw in an ESM package**
`package.json` declares `"type": "module"`, so Node.js treats all `.js` files as ES modules. Running `node scripts/format-docs-check.js` will immediately throw:
```
ReferenceError: require is not defined in ES module scope
```
The `.cjs` sibling file works correctly because the `.cjs` extension always forces CommonJS mode regardless of `"type"`. This `.js` file is also unreferenced in `package.json`, making it dead code that is broken if anyone tries to invoke it directly. It should either be removed entirely, or — if it is meant as a fallback — converted to ESM (`import { spawnSync } from 'child_process'`, `import fs from 'fs'`).
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-check.cjs
Line: 19-29
Comment:
**Performance regression: N separate `oxfmt` spawns instead of one**
The original `xargs oxfmt --check` invocation passed all matching files in a single (or a few batched) subprocess calls. The new loop spawns a fresh `oxfmt` process for every file. On a repository with many Markdown files this can be significantly slower due to per-process startup overhead.
Consider passing all files to a single `oxfmt` invocation:
```js
function runOxfmt(files) {
const result = spawnSync("oxfmt", ["--check", ...files], {
stdio: "inherit",
});
if (result.error) {
console.error("Failed to run oxfmt:", result.error.message);
process.exit(1);
}
if (result.status !== 0) {
process.exit(result.status ?? 1);
}
}
```
This matches the original behavior and avoids the per-file spawn cost. If you intentionally want per-file output isolation, a comment explaining the rationale would be helpful.
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-check.cjs
Line: 22-26
Comment:
**Spawn errors (e.g. `oxfmt` not on PATH) are silently swallowed**
When `spawnSync` cannot find the executable, it sets `result.error` (an `Error` object) and leaves `result.status` as `null`. `null !== 0` evaluates to `true`, so the code does mark `failed = true` — but it never prints *why*, leaving the user with no output and a confusing non-zero exit. Adding an explicit check improves the developer experience:
```js
if (result.error) {
console.error(`Failed to spawn oxfmt: ${result.error.message}`);
process.exit(1);
}
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: dfa42bc |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
scripts/format-docs-check.js
Outdated
| // scripts/format-docs-check.js | ||
| // Cross-platform docs formatting check for Windows | ||
| const { spawnSync } = require("child_process"); | ||
| const fs = require("fs"); | ||
|
|
There was a problem hiding this comment.
require() will throw in an ESM package
package.json declares "type": "module", so Node.js treats all .js files as ES modules. Running node scripts/format-docs-check.js will immediately throw:
ReferenceError: require is not defined in ES module scope
The .cjs sibling file works correctly because the .cjs extension always forces CommonJS mode regardless of "type". This .js file is also unreferenced in package.json, making it dead code that is broken if anyone tries to invoke it directly. It should either be removed entirely, or — if it is meant as a fallback — converted to ESM (import { spawnSync } from 'child_process', import fs from 'fs').
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/format-docs-check.js
Line: 1-5
Comment:
**`require()` will throw in an ESM package**
`package.json` declares `"type": "module"`, so Node.js treats all `.js` files as ES modules. Running `node scripts/format-docs-check.js` will immediately throw:
```
ReferenceError: require is not defined in ES module scope
```
The `.cjs` sibling file works correctly because the `.cjs` extension always forces CommonJS mode regardless of `"type"`. This `.js` file is also unreferenced in `package.json`, making it dead code that is broken if anyone tries to invoke it directly. It should either be removed entirely, or — if it is meant as a fallback — converted to ESM (`import { spawnSync } from 'child_process'`, `import fs from 'fs'`).
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed - removed format-docs-check.js entirely. It was dead code (unreferenced in package.json) and broken in an ESM package. The .cjs file is the sole implementation.
scripts/format-docs-check.cjs
Outdated
| function runOxfmt(files) { | ||
| let failed = false; | ||
| for (const file of files) { | ||
| const result = spawnSync("oxfmt", ["--check", file], { | ||
| stdio: "inherit", | ||
| }); | ||
| if (result.status !== 0) { | ||
| failed = true; | ||
| } | ||
| } | ||
| if (failed) { |
There was a problem hiding this comment.
oxfmt spawns instead of one
The original xargs oxfmt --check invocation passed all matching files in a single (or a few batched) subprocess calls. The new loop spawns a fresh oxfmt process for every file. On a repository with many Markdown files this can be significantly slower due to per-process startup overhead.
Consider passing all files to a single oxfmt invocation:
function runOxfmt(files) {
const result = spawnSync("oxfmt", ["--check", ...files], {
stdio: "inherit",
});
if (result.error) {
console.error("Failed to run oxfmt:", result.error.message);
process.exit(1);
}
if (result.status !== 0) {
process.exit(result.status ?? 1);
}
}This matches the original behavior and avoids the per-file spawn cost. If you intentionally want per-file output isolation, a comment explaining the rationale would be helpful.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/format-docs-check.cjs
Line: 19-29
Comment:
**Performance regression: N separate `oxfmt` spawns instead of one**
The original `xargs oxfmt --check` invocation passed all matching files in a single (or a few batched) subprocess calls. The new loop spawns a fresh `oxfmt` process for every file. On a repository with many Markdown files this can be significantly slower due to per-process startup overhead.
Consider passing all files to a single `oxfmt` invocation:
```js
function runOxfmt(files) {
const result = spawnSync("oxfmt", ["--check", ...files], {
stdio: "inherit",
});
if (result.error) {
console.error("Failed to run oxfmt:", result.error.message);
process.exit(1);
}
if (result.status !== 0) {
process.exit(result.status ?? 1);
}
}
```
This matches the original behavior and avoids the per-file spawn cost. If you intentionally want per-file output isolation, a comment explaining the rationale would be helpful.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
all files are now passed to a single oxfmt invocation, matching the original xargs batching behaviour.
scripts/format-docs-check.cjs
Outdated
| const result = spawnSync("oxfmt", ["--check", file], { | ||
| stdio: "inherit", | ||
| }); | ||
| if (result.status !== 0) { | ||
| failed = true; |
There was a problem hiding this comment.
oxfmt not on PATH) are silently swallowed
When spawnSync cannot find the executable, it sets result.error (an Error object) and leaves result.status as null. null !== 0 evaluates to true, so the code does mark failed = true — but it never prints why, leaving the user with no output and a confusing non-zero exit. Adding an explicit check improves the developer experience:
if (result.error) {
console.error(`Failed to spawn oxfmt: ${result.error.message}`);
process.exit(1);
}Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/format-docs-check.cjs
Line: 22-26
Comment:
**Spawn errors (e.g. `oxfmt` not on PATH) are silently swallowed**
When `spawnSync` cannot find the executable, it sets `result.error` (an `Error` object) and leaves `result.status` as `null`. `null !== 0` evaluates to `true`, so the code does mark `failed = true` — but it never prints *why*, leaving the user with no output and a confusing non-zero exit. Adding an explicit check improves the developer experience:
```js
if (result.error) {
console.error(`Failed to spawn oxfmt: ${result.error.message}`);
process.exit(1);
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
added result.error checks for both the git ls-files and oxfmt calls with explicit error messages
![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: dfa42bc16d
ℹ️ 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".
scripts/format-docs-check.js
Outdated
| @@ -0,0 +1,40 @@ | |||
| // scripts/format-docs-check.js | |||
| // Cross-platform docs formatting check for Windows | |||
| const { spawnSync } = require("child_process"); | |||
There was a problem hiding this comment.
This newly added .js script is not runnable in this repository because package.json sets "type": "module", so node scripts/format-docs-check.js throws ReferenceError: require is not defined in ES module scope at startup. Even though format:docs:check currently points to the .cjs file, keeping a broken duplicate entrypoint in-tree is a maintainability trap and can break anyone who invokes the .js script directly or wires it into tooling later.
Useful? React with 👍 / 👎.
The existing format:docs:check used `git ls-files | xargs oxfmt` which fails on Windows — xargs is unavailable and single-quoted globs in cmd.exe are treated as literals, silently matching no files. Replace with scripts/format-docs-check.cjs, a Node.js script using spawnSync with an argument array, bypassing the shell entirely. This gives identical behaviour on Windows, macOS, and Linux. - package.json: format:docs:check now calls node scripts/format-docs-check.cjs - scripts/format-docs-check.cjs: passes all files in a single oxfmt invocation (matches original xargs batching), with explicit error handling for missing executables Related openclaw#3460
soyames
force-pushed
the
fix/docs-format-check-windows-clean
branch
from
5932a26 to
7596519
Compare
|
This looks like a solid, low-risk cross-platform improvement. The change is tightly scoped to I checked the current branch/diff and do not see any duplicate As a follow-up, it may be worth adding a small unit or integration test around the helper so future refactors do not accidentally change the file-selection behavior. |
2 participants
Summary
Describe the problem and fix in 2–5 bullets:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
None. Internal dev tooling only — pnpm format:docs:check now works on Windows. No product behavior changes.
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Failure Recovery (if this breaks)
Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.Note: developed with assistance from Claude and GitHub Copilot for cross-platform scripting.