Skip to content

fix: use pathToFileURL for Windows path comparison in generate-base-config-schema#52989

Open
easyteacher wants to merge 1 commit intoopenclaw:mainfrom
easyteacher:work/generateconfig
Open

fix: use pathToFileURL for Windows path comparison in generate-base-config-schema#52989
easyteacher wants to merge 1 commit intoopenclaw:mainfrom
easyteacher:work/generateconfig

Conversation

@easyteacher
Copy link
Copy Markdown

@easyteacher easyteacher commented Mar 23, 2026

Summary

  • Problem: On Windows, the script entry point check import.meta.url === new URL(process.argv[1] ?? "", "file://").href always failed because new URL() does not properly convert Windows backslash paths to valid file URLs.

  • Why it matters: The script would never execute its main logic on Windows because the entry point check would always evaluate to false, breaking the --check and --write CLI functionality for Windows users.

  • What changed: Replaced manual URL construction with Node's pathToFileURL() function, which correctly handles Windows paths (converting D:\path\to\file.ts to file:///D:/path/to/file.ts).

  • What did NOT change (scope boundary): No changes to the actual schema generation logic, output format, or any other functionality beyond the entry point detection.

Change Type (select all)

  • Bug fix

Scope (select all touched areas)

  • UI / DX

Linked Issue/PR

  • Closes #

  • Related #

  • This PR fixes a bug or regression

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, write Unknown.

  • Root cause: The original code used new URL(process.argv[1], "file://") to construct a file URL from process.argv[1]. This works on Unix systems where paths use forward slashes, but on Windows, paths contain backslashes (e.g., D:\path\file.ts). The URL constructor does not normalize backslashes, resulting in malformed URLs like file://D:\path\file.ts instead of the correct file:///D:/path/file.ts.

  • Missing detection / guardrail: No Windows-specific testing or CI coverage for this script's CLI entry point.

  • Prior context (git blame, prior PR, issue, or refactor if known): N/A

  • Why this regressed now: Not a regression; this was likely always broken on Windows but went undetected.

  • If unknown, what was ruled out: N/A

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.

  • Coverage level that should have caught this: Unit test

  • Target test or file: scripts/generate-base-config-schema.ts or a new test file

  • Scenario the test should lock in: Verify that the entry point check correctly identifies when the script is run directly on all platforms.

  • Why this is the smallest reliable guardrail: A simple unit test mocking process.argv[1] and import.meta.url with Windows-style paths would catch this.

  • Existing test that already covers this (if any): None known.

  • If no new test is added, why not: N/A (test recommended)

User-visible / Behavior Changes

List user-visible changes (including defaults/config).

  • Windows users can now successfully run node --import tsx scripts/generate-base-config-schema.ts --check and --write commands.

Security Impact (required)

  • New permissions/capabilities? (No)

  • Secrets/tokens handling changed? (No)

  • New/changed network calls? (No)

  • Command/tool execution surface changed? (No)

  • Data access scope changed? (No)

  • If any Yes, explain risk + mitigation: N/A

Repro + Verification

Environment

  • OS: Windows

  • Runtime/container: Node.js

  • Model/provider: N/A

  • Integration/channel (if any): N/A

  • Relevant config (redacted): N/A

Steps

  1. On Windows, run node --import tsx scripts/generate-base-config-schema.ts --check

  2. Observe that before the fix, the script would silently do nothing (entry point check failed)

  3. After the fix, the script correctly executes and checks the generated schema

Expected

  • Script should execute its main logic and either report success or write the generated file.

Actual

  • Before fix: Script silently exited without executing main logic on Windows.

  • After fix: Script correctly executes on Windows.

Evidence

Attach at least one:

  • Failing test/log before + passing after

  • Trace/log snippets

  • Screenshot/recording

  • Perf numbers (if relevant)

PS > node --import tsx scripts/generate-base-config-schema.ts --check
Invalid config at C:\Users\wenfushan\.openclaw\openclaw.json:\n- commands: Unrecognized key: "shellProfile"
(node:45248) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
(Use `node --trace-deprecation ...` to show where the warning was created)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios: Reviewed the code diff and confirmed pathToFileURL is the correct Node.js API for cross-platform file URL conversion.

  • Edge cases checked: Verified that pathToFileURL handles empty strings gracefully (returns file://) and that the ?? "" fallback is preserved.

  • What you did not verify: Linux/macOS

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.

  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

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

  • Backward compatible? (Yes)

  • Config/env changes? (No)

  • Migration needed? (No)

  • If yes, exact upgrade steps: N/A

Failure Recovery (if this breaks)

  • How to disable/revert this change quickly: Revert commit

  • Files/config to restore: scripts/generate-base-config-schema.ts

  • Known bad symptoms reviewers should watch for: Script not executing on any platform (would indicate pathToFileURL behaving unexpectedly)

Risks and Mitigations

List only real risks for this PR. Add/remove entries as needed. If none, write None.

  • Risk: Minimal - pathToFileURL is a stable Node.js API available since Node.js v10.12.0.

  • Mitigation: The change is isolated to a single line and only affects the entry point check.

@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: XS labels Mar 23, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 23, 2026

Greptile Summary

This PR fixes a Windows-only bug where the entry point check in scripts/generate-base-config-schema.ts always evaluated to false because new URL(process.argv[1], "file://") does not normalize Windows backslash paths into valid file URLs. The fix correctly replaces it with pathToFileURL(), a stable Node.js built-in (available since v10.12.0) that handles cross-platform path normalization.

  • The import of pathToFileURL is correctly added alongside the existing fileURLToPath import from node:url.
  • The change is isolated to a single line and does not affect schema generation logic, output format, or any other behavior.
  • One minor edge case worth noting: when process.argv[1] is undefined, the ?? "" fallback passes an empty string to pathToFileURL(""). On most Node.js versions this resolves to the current working directory as a file URL (e.g., file:///D:/cwd/) rather than the bare file:// that the old new URL("", "file://") produced. In practice this is harmless since neither value will match import.meta.url, but it is a subtle behavioral difference worth being aware of.

Confidence Score: 5/5

  • Safe to merge — isolated, correct fix using a stable Node.js API with no impact on existing Unix/macOS behavior.
  • The change is a single-line swap of a well-known broken pattern (new URL(path, "file://") on Windows) with the correct pathToFileURL() API. It is fully backward-compatible, touches no logic outside the entry point guard, and pathToFileURL has been stable since Node.js v10.12.0. No test regression risk.
  • No files require special attention.

Reviews (1): Last reviewed commit: "fix: use pathToFileURL for Windows path ..." | Re-trigger Greptile

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 30, 2026

Codex review: needs maintainer review before merge.

Summary
The PR imports pathToFileURL, uses it for direct execution detection in scripts/generate-base-config-schema.ts, and adds a changelog entry.

Reproducibility: yes. with high confidence from source inspection and a Node API probe rather than a fresh Windows run. Current main still has the manual file-URL guard, package scripts invoke the generator directly, and the PR body includes Windows PowerShell output showing the fixed path reaches the command body.

Real behavior proof
Sufficient (live_output): The PR body includes copied Windows PowerShell output after the fix showing the generator executes and emits config validation output; the automated proof check was still failing and may need maintainer rerun or override.

Next step before merge
No automated code repair is needed; the remaining work is maintainer merge/check handling for the prepared head, especially the failed proof-check status.

Security
Cleared: The diff uses a Node built-in and updates a local script guard plus changelog, with no new dependency, network, secret, permission, or package-resolution surface.

Review details

Best possible solution:

Land the narrow guard fix and changelog once the prepared head’s check state is resolved; keep broader generator-entrypoint cleanup separate.

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

Yes, with high confidence from source inspection and a Node API probe rather than a fresh Windows run. Current main still has the manual file-URL guard, package scripts invoke the generator directly, and the PR body includes Windows PowerShell output showing the fixed path reaches the command body.

Is this the best way to solve the issue?

Yes. pathToFileURL(process.argv[1] ?? "").href is the narrowest maintainable repair for this entrypoint check and matches existing script precedent in the repo.

Acceptance criteria:

  • Wait for or resolve GitHub checks on f77464d.
  • Use the existing maintainer-reported local verification as supporting proof: pnpm exec oxfmt --check --threads=1 scripts/generate-base-config-schema.ts CHANGELOG.md, node --import tsx scripts/generate-base-config-schema.ts --check, pnpm build, and pnpm check.
  • Optional strongest final smoke: run node --import tsx scripts/generate-base-config-schema.ts --check on Windows and confirm it does not silently exit.

What I checked:

  • Current main still has the old guard: Current main compares import.meta.url to new URL(process.argv[1] ?? "", "file://").href, which is the Windows path conversion pattern the PR replaces. (scripts/generate-base-config-schema.ts:15, 83caeb839682)
  • Generator is invoked directly by package scripts: check:base-config-schema, config:schema:check, and config:schema:gen run node --import tsx scripts/generate-base-config-schema.ts, so a false direct-entrypoint guard skips the command body. (package.json:1320, 83caeb839682)
  • PR diff is scoped: The live PR diff changes only CHANGELOG.md and the generator script: it adds the built-in node:url import and replaces the guard with pathToFileURL(process.argv[1] ?? "").href. (scripts/generate-base-config-schema.ts:16, f77464d7f485)
  • Node API probe supports the bug and fix: On Node v24.14.1, new URL(String.rawD:\repo\scripts\generate-base-config-schema.ts, 'file://').href produced d:\repo\scripts\generate-base-config-schema.ts, while pathToFileURL(..., { windows: true }).href produced file:///D:/repo/scripts/generate-base-config-schema.ts; POSIX absolute paths matched under both APIs.
  • Existing repo precedent: The shared generated-output CLI helper already uses pathToFileURL(process.argv[1] ?? "").href for direct-entrypoint detection. (scripts/lib/generated-output-utils.mjs:27, 83caeb839682)
  • Real behavior proof in PR body: The PR body includes copied PowerShell output from Windows after the fix showing the generator reaches config validation output instead of silently exiting. (f77464d7f485)

Likely related people:

  • steipete: Git blame and history show Peter Steinberger authored the base config schema generator, hardened related generated-file guards, maintained the schema generator, and converted the current file to the runtime-computed schema flow. (role: introduced behavior and recent maintainer; confidence: high; commits: ca99163b9838, 96d61aa50c3f, 68851f2e9730; files: scripts/generate-base-config-schema.ts, src/config/schema-base.ts, scripts/lib/generated-output-utils.mjs)
  • BradGroux: BradGroux force-pushed and committed the prepared PR head after rebasing it onto current main, resolving the generator conflict, adding the changelog entry, and reporting local verification. (role: recent maintainer follow-up owner; confidence: medium; commits: f77464d7f485; files: scripts/generate-base-config-schema.ts, CHANGELOG.md)

Remaining risk / open question:

  • The prepared head still had failed Real behavior proof check runs and one in-progress check when inspected, so merge should wait for maintainer resolution of check state.
  • This review used source inspection and a Node API probe rather than running a fresh Windows smoke locally.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 83caeb839682.

Re-review progress:

…onfig-schema

The script was using new URL() to compare import.meta.url with process.argv[1],
which fails on Windows because new URL() doesn't properly convert Windows paths
to file URLs. Using pathToFileURL from node:url correctly handles this conversion.
@BradGroux BradGroux force-pushed the work/generateconfig branch from 95024b6 to f77464d Compare May 9, 2026 09:00
@BradGroux
Copy link
Copy Markdown
Member

Maintainer refresh:

  • Rebased this PR onto current origin/main.
  • Resolved the generator conflict by keeping the current runtime-computed base-config schema flow and applying the Windows-safe pathToFileURL direct-entrypoint guard there.
  • Added the required changelog entry with contributor attribution.

Prepared head: f77464d7f485e3f735bd16ef7ed43f037bbcc214
Previous head: 95024b6946e4803e819a2258c83d55baa475546e

Local verification passed:

pnpm install --frozen-lockfile
pnpm exec oxfmt --check --threads=1 scripts/generate-base-config-schema.ts CHANGELOG.md
node --import tsx scripts/generate-base-config-schema.ts --check
pnpm build
pnpm check

Fresh GitHub checks should run on the prepared head before merge.

@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 9, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 9, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scripts Repository scripts size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. triage: risky-infra Candidate: infra/CI/release change needs maintainer review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants