Skip to content

fix(scripts): asset generation cannot spawn vite on Windows - #130

Open
MohammedAlkindi wants to merge 1 commit into
openclaw:mainfrom
MohammedAlkindi:fix/windows-generate-assets-path
Open

fix(scripts): asset generation cannot spawn vite on Windows#130
MohammedAlkindi wants to merge 1 commit into
openclaw:mainfrom
MohammedAlkindi:fix/windows-generate-assets-path

Conversation

@MohammedAlkindi

Copy link
Copy Markdown

What Problem This Solves

Resolves a problem where pnpm check cannot get past asset generation on Windows.
generate-assets.mjs builds the vite argv with URL.pathname, which yields a leading-slash path such
as /C:/repo/node_modules/vite/bin/vite.js. Node resolves that against the drive root, so the spawn
fails before any asset is written.

Why This Change Was Made

fileURLToPath is the documented file: URL to path conversion and returns the same string on POSIX,
so this is a no-op for Linux and macOS contributors and unblocks Windows ones.

User Impact

A Windows contributor can run pnpm check through the asset build instead of failing at the first step.

Evidence

On this branch with the change reverted:

Command failed: node.exe /C:/Users/.../node_modules/vite/bin/vite.js build --config /C:/Users/.../vite.config.mjs
Error: Cannot find module 'C:\C:\Users\...\node_modules\vite\bin\vite.js'

With it applied, node scripts/generate-assets.mjs exits cleanly and pnpm check runs through the vite
build to oxfmt.

Could not verify: pnpm check still does not finish here, for an unrelated reason. oxfmt --check
reports format issues in 427 files on a stock Windows clone: the repo has no .gitattributes, so
core.autocrlf=true checks out CRLF while oxfmt expects LF. scripts/generate-assets.mjs is not among
those 427. That is a checkout-wide question rather than something this PR should decide, so I have left
it alone.

scripts/render-brand.mjs:16 has the same URL.pathname pattern. It is referenced by no package script
or workflow and needs rsvg, so I left it rather than change code I cannot exercise.

generate-assets.mjs builds the vite argv with URL.pathname, which on
Windows yields a leading-slash path such as /C:/repo/node_modules/vite/
bin/vite.js. Node resolves that against the drive root, so the spawn
fails with Cannot find module 'C:\C:\repo\node_modules\vite\bin\vite.js'
and pnpm check cannot get past the asset build.

Use fileURLToPath, which is the documented file: URL to path conversion
and is a no-op difference on POSIX.

scripts/render-brand.mjs has the same URL.pathname pattern, but it is
referenced by no package script or workflow and needs rsvg, so it is left
alone here rather than changed without a way to exercise it.
@clawsweeper

clawsweeper Bot commented Sep 8, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 8, 2026
@clawsweeper

clawsweeper Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 8, 2026, 3:30 AM ET / 07:30 UTC.

ClawSweeper review

What this changes

Convert Vite’s executable and configuration file URLs into native filesystem paths so asset generation works on Windows.

Merge readiness

Ready for maintainer review

This PR remains necessary: current main and v0.3.1 retain the Windows path defect. The focused patch is correct, and the contributor describes successful asset generation on Windows after applying it.

Priority: P2
Reviewed head: d7f7df6f6fb226a20f75205e9303bd41adce51bc

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused standard-library repair with relevant Windows behavior evidence and no actionable code findings; repository landing gates remain separate.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The supplied Windows before-and-after account exercises the changed asset generator: the reverted version cannot resolve Vite, while the patched generator completes and the check advances to formatting; the disclosed later formatting failure does not undermine this narrow result.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The supplied Windows before-and-after account exercises the changed asset generator: the reverted version cannot resolve Vite, while the patched generator completes and the check advances to formatting; the disclosed later formatting failure does not undermine this narrow result.
Evidence reviewed 8 items Precisely scoped introduced change: The verified base-to-head diff adds the built-in URL conversion import and replaces both pathname arguments passed to execFile; no dependencies, shell execution settings, or generated files change.
Current main still contains the defect: The main version passes URL.pathname for both the Vite script and configuration path. A GitHub main-commit read independently returned the supplied main SHA.
Latest release remains affected: The supplied latest release, v0.3.1, also uses pathname for both subprocess arguments.
Findings None None.
Security None None.

How this fits together

Crabfleet’s asset generator invokes Vite to bundle the browser application, then packages application and documentation assets for the Worker. Build, check, and several tests enter through this generator.

flowchart LR
  A[Build or check command] --> B[Asset generator]
  C[Vite and config file URLs] --> D[Native filesystem paths]
  D --> B
  B --> E[Vite browser bundle]
  E --> F[Generated Worker assets]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

None.

Technical review

Best possible solution:

Use Node’s native file-URL conversion at both subprocess argument boundaries, keeping unrelated checkout line-ending behavior outside this fix.

Do we have a high-confidence way to reproduce the issue?

Yes: running the current generator from a Windows drive checkout passes malformed drive paths to Node, matching the supplied failure trace and Node’s documented behavior; this review did not execute the build.

Is this the best way to solve the issue?

Yes: converting both file URLs with the standard Node API is the narrow repair and also handles encoded path characters correctly.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against bdd5083b0d3d.

Labels

Label changes:

  • add P2: Repairs a concrete Windows contributor build failure with a small, bounded change.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The supplied Windows before-and-after account exercises the changed asset generator: the reverted version cannot resolve Vite, while the patched generator completes and the check advances to formatting; the disclosed later formatting failure does not undermine this narrow result.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The supplied Windows before-and-after account exercises the changed asset generator: the reverted version cannot resolve Vite, while the patched generator completes and the check advances to formatting; the disclosed later formatting failure does not undermine this narrow result.

Label justifications:

  • P2: Repairs a concrete Windows contributor build failure with a small, bounded change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The supplied Windows before-and-after account exercises the changed asset generator: the reverted version cannot resolve Vite, while the patched generator completes and the check advances to formatting; the disclosed later formatting failure does not undermine this narrow result.
  • proof: sufficient: Contributor real behavior proof is sufficient. The supplied Windows before-and-after account exercises the changed asset generator: the reverted version cannot resolve Vite, while the patched generator completes and the check advances to formatting; the disclosed later formatting failure does not undermine this narrow result.

Evidence

What I checked:

  • Precisely scoped introduced change: The verified base-to-head diff adds the built-in URL conversion import and replaces both pathname arguments passed to execFile; no dependencies, shell execution settings, or generated files change. (scripts/generate-assets.mjs:29, d7f7df6f6fb2)
  • Current main still contains the defect: The main version passes URL.pathname for both the Vite script and configuration path. A GitHub main-commit read independently returned the supplied main SHA. (scripts/generate-assets.mjs:28, bdd5083b0d3d)
  • Latest release remains affected: The supplied latest release, v0.3.1, also uses pathname for both subprocess arguments. (scripts/generate-assets.mjs:28, 8cd813822924)
  • Production entrypoint and existing test integration: package.json runs the generator first in build and check; tests/helpers/generated-assets.ts invokes the same generator. Existing Vite tests cover bundling configuration rather than Windows argument conversion. (package.json:8, d7f7df6f6fb2)
  • Authoritative Node path-conversion contract: The changed import directly depends on Node’s URL API. The official documentation demonstrates that pathname produces an invalid Windows drive path while fileURLToPath returns a native path and correctly decodes spaces and Unicode: Node URL documentation.
  • Contributor Windows before-and-after evidence: The complete supplied body, captured with sourceRevision 74fab7ee27e085d892ecbd7f7cd5029b007df7f0b3a27434b85ca37c0b20cb8f, includes the reverted-branch Windows module-resolution error and reports that applying the patch lets node scripts/generate-assets.mjs complete and pnpm check reach oxfmt. It separately discloses 427 CRLF-related formatting failures; this does not claim a fully passing check. (d7f7df6f6fb2)

Likely related people:

  • vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant