fix: add Windows path support in toImportSpecifier + add tests (#575)#593
fix: add Windows path support in toImportSpecifier + add tests (#575)#593jlin53882 wants to merge 1 commit intoCortexReach:masterfrom
Conversation
…xReach#575) ## Summary Fix `toImportSpecifier()` to handle Windows drive-letter paths (`C:\...` / `D:/...`) by converting them to `file://` URLs via `pathToFileURL()`. Cherry-pick the APPDATA fallback block from PR CortexReach#576 (already reviewed). ## Problem (Issue CortexReach#575) `toImportSpecifier()` only converted POSIX absolute paths (`/` prefix) to `file://` URLs. Windows paths like `C:\Users\...\extensionAPI.js` fell through to `return trimmed` and were passed unchanged to `import()`, causing `ERR_UNSUPPORTED_ESM_URL_SCHEME` on Windows. ## Fix 1. **`toImportSpecifier()`** (index.ts:420): Add regex check for Windows drive-letter paths (`/^[a-zA-Z]:[/\\]/`) and convert via `pathToFileURL().href`. 2. **Windows APPDATA fallback** (index.ts:440-443): Cherry-picked from PR CortexReach#576 original commit (already reviewed by rwmjhb and Codex Bot). ## Verified by Codex Review (Round 2) - POSIX paths: ✅ `/usr/...` → `file://` URL - Windows paths: ✅ `C:\...` → `file://` URL - UNC paths:⚠️ Out of scope (requires `\\server\share` support) - `OPENCLAW_EXTENSION_API_PATH` with Windows path: ✅ Fixed - TypeScript correctness: ✅ - Scope check: ✅ Minimal diff (7 lines total) ## Tests Added `test/to-import-specifier-windows.test.mjs` with 27 tests: - `toImportSpecifier`: 16 tests (POSIX, Windows, pass-through, edge cases) - `getExtensionApiImportSpecifiers`: 9 tests (env var, dedup, APPDATA fallback) - `pathToFileURL` integration: 2 tests Fixes CortexReach#575
ReviewersThis PR supersedes the closed PR #576. The original PR had a bug where Three rounds of adversarial Codex review. All 27 tests pass. 430/433 existing tests pass (3 pre-existing failures unrelated to this change). cc: @rwmjhb @chatgpt-codex-connector[bot] — your earlier feedback shaped this fix, thank you. |
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a799d65c10
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (trimmed.startsWith("file://")) return trimmed; | ||
| if (trimmed.startsWith("/")) return pathToFileURL(trimmed).href; | ||
| // Handle Windows absolute paths (e.g. C:\Users\... or D:/Program Files/...) | ||
| if (/^[a-zA-Z]:[/\\]/.test(trimmed)) return pathToFileURL(trimmed).href; |
There was a problem hiding this comment.
Normalize UNC absolute paths before dynamic import
The new Windows conversion only matches drive-letter paths (^[a-zA-Z]:[/\\]), so UNC absolute paths like \\server\share\... are still returned unchanged. This matters because the new APPDATA fallback can point to UNC locations in redirected-profile environments; in that case getExtensionApiImportSpecifiers() adds a raw UNC specifier and import(specifier) fails instead of loading extensionAPI.js. Please extend toImportSpecifier to treat UNC roots as filesystem paths and convert them with pathToFileURL() too.
Useful? React with 👍 / 👎.
Summary
Fix
toImportSpecifier()to handle Windows drive-letter paths (C:\.../D:/...) by converting them tofile://URLs, and add the Windows APPDATA fallback from the closed PR #576.Why This PR Exists (vs. the Closed PR #576)
PR #576 was closed because the initial implementation had a critical bug: it used
toImportSpecifier(windowsNpmPath)which does NOT convert Windows paths tofile://URLs — they were passed unchanged toimport(), causingERR_UNSUPPORTED_ESM_URL_SCHEME.This PR supersedes #576 with the correct fix: modifying
toImportSpecifier()itself to detect Windows drive-letter paths and convert them viapathToFileURL().Problem (Issue #575)
toImportSpecifier()only converted POSIX absolute paths (/prefix) tofile://URLs. Windows paths likeC:\Users\...\extensionAPI.jsfell through toreturn trimmedand were passed unchanged toimport(), causing the error:This affected both the Windows APPDATA fallback and any user who set
OPENCLAW_EXTENSION_API_PATHto a Windows path.Solution
Modify
toImportSpecifier()to detect Windows drive-letter paths:function toImportSpecifier(value: string): string { const trimmed = value.trim(); if (!trimmed) return ""; if (trimmed.startsWith("file://")) return trimmed; if (trimmed.startsWith("/")) return pathToFileURL(trimmed).href; + // Handle Windows absolute paths (e.g. C:\Users\... or D:/Program Files/...) + if (/^[a-zA-Z]:[/\\]/.test(trimmed)) return pathToFileURL(trimmed).href; return trimmed; }Also includes the APPDATA fallback from PR #576 (cherry-picked):
Why This Approach (vs. Alternative Fixes)
Three rounds of adversarial Codex review concluded that fixing
toImportSpecifier()itself is the cleanest solution because:OPENCLAW_EXTENSION_API_PATHenv var (hidden issue /lesson命令是自己添加吗 没成功 #1) is also fixedtoImportSpecifier(), the rest is the unchanged cherry-pickTests
27 new tests covering:
file://URLC:\,D:/) →file://URLfile://, bare module specifiers, relative paths)getExtensionApiImportSpecifiers()behaviorCodex Review History
pathToFileURL().href)Fixes #575