Fix/docs format check windows clean#48887
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 |
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".
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. |
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
7596519 to
fc46ccf
Compare
|
ProjectClownfish pushed a narrow repair to this branch so the original contributor path can stay canonical. Source PR: #48887 |
|
Codex review: needs maintainer review before merge. Summary Reproducibility: yes. Current main has a clear source-level reproduction path: run Next step before merge Security Review detailsBest possible solution: Land this branch or an equivalent canonical fix after maintainer review, green exact-head checks, and native Windows validation, then use it to resolve the linked PowerShell docs-format issue. Do we have a high-confidence way to reproduce the issue? Yes. Current main has a clear source-level reproduction path: run Is this the best way to solve the issue? Yes. The PR is the narrow maintainable direction: preserve the existing docs formatter surface, batch only the Windows-sensitive Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 89db1e5440f5. |
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.