fix: use pathToFileURL for Windows path comparison in generate-base-config-schema#52989
fix: use pathToFileURL for Windows path comparison in generate-base-config-schema#52989easyteacher wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a Windows-only bug where the entry point check in
Confidence Score: 5/5
Reviews (1): Last reviewed commit: "fix: use pathToFileURL for Windows path ..." | Re-trigger Greptile |
5584646 to
95024b6
Compare
|
Codex review: needs maintainer review before merge. Summary 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 Next step before merge Security Review detailsBest 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. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
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.
95024b6 to
f77464d
Compare
|
Maintainer refresh:
Prepared head: Local verification passed: Fresh GitHub checks should run on the prepared head before merge. |
Summary
Problem: On Windows, the script entry point check
import.meta.url === new URL(process.argv[1] ?? "", "file://").hrefalways failed becausenew 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--checkand--writeCLI functionality for Windows users.What changed: Replaced manual URL construction with Node's
pathToFileURL()function, which correctly handles Windows paths (convertingD:\path\to\file.tstofile:///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)
Scope (select all touched areas)
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, writeUnknown.Root cause: The original code used
new URL(process.argv[1], "file://")to construct a file URL fromprocess.argv[1]. This works on Unix systems where paths use forward slashes, but on Windows, paths contain backslashes (e.g.,D:\path\file.ts). TheURLconstructor does not normalize backslashes, resulting in malformed URLs likefile://D:\path\file.tsinstead of the correctfile:///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/AWhy 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.tsor a new test fileScenario 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]andimport.meta.urlwith 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).
node --import tsx scripts/generate-base-config-schema.ts --checkand--writecommands.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/ARepro + Verification
Environment
OS: Windows
Runtime/container: Node.js
Model/provider: N/A
Integration/channel (if any): N/A
Relevant config (redacted): N/A
Steps
On Windows, run
node --import tsx scripts/generate-base-config-schema.ts --checkObserve that before the fix, the script would silently do nothing (entry point check failed)
After the fix, the script correctly executes and checks the generated schema
Expected
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)
Human Verification (required)
What you personally verified (not just CI), and how:
Verified scenarios: Reviewed the code diff and confirmed
pathToFileURLis the correct Node.js API for cross-platform file URL conversion.Edge cases checked: Verified that
pathToFileURLhandles empty strings gracefully (returnsfile://) 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.tsKnown bad symptoms reviewers should watch for: Script not executing on any platform (would indicate
pathToFileURLbehaving unexpectedly)Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.Risk: Minimal -
pathToFileURLis 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.