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 3 commits intoCortexReach:masterfrom
Conversation
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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.
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 👍 / 👎.
AliceLJY
left a comment
AliceLJY
left a comment
There was a problem hiding this comment.
Production code 改动干净精准，review 通过。
✅ 核实点
toImportSpecifier()新增的 Windows drive-letter 正则^[a-zA-Z]:[/\\]覆盖了C:\D:/大小写盘符 + 两种分隔符，行为正确- 通过
pathToFileURL().href返回file://URL，避免了 PR #576 当时的ERR_UNSUPPORTED_ESM_URL_SCHEME - APPDATA fallback 的 platform guard (
process.platform === "win32") 保证非 Windows 环境不会污染 specifier 列表 - 3 轮 Codex 对抗审查，链条清晰
顺便这个 PR 变相解决了今天提的 Issue #606 Bug 1（Windows 用户在 beta.10 遇到的 path is not defined / ERR_UNSUPPORTED_ESM_URL_SCHEME）——把 #606 Bug 1 的作者 @jlin53882 关联一下可以一起 close。
🟡 Nit（不 block，follow-up）
test/to-import-specifier-windows.test.mjs 的测试复制了一份 toImportSpecifier 和 getExtensionApiImportSpecifiers 的实现（开头明确注释"Copy of the PR #576 implementation"），原因是这两个 function 没从 index.ts export 出来。
这是一个已知反模式：
- 实现改了测试不会 fail（因为测试跑的是 copy）
- 容易产生"测试通过"的假安全感
- 实际生效的是
index.ts里的那份代码，测试只能当 spec 文档参考
建议 follow-up PR：把这两个 function 从 index.ts 挪到 src/import-specifier.ts（或类似的 util 模块）并 export，测试直接 import 真实 function。这样改动很小（一个 move + export），但测试从"spec 文档"升级为"真实回归保护"。
不 block 这个 PR——production code 的正则是 +2 行且已有 27 个 copy-based 测试覆盖场景，风险极低。APPROVE ✅
ℹ️ 按 mlp 流程，approve 后 assign 给 @rwmjhb 合并。
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
Correct fix for a real Windows blocker. The tests need to be wired up before this can be safely merged.
Must fix
MR1 — New test file is structurally excluded from CI
test/to-import-specifier-windows.test.mjs is not registered in scripts/verify-ci-test-manifest.mjs (or equivalent CI entry list), so the 27 new tests never run in any CI context — not because of a stale-base failure, but because the file itself isn't hooked in. A future regression in the fixed regex would pass CI undetected.
Add the file to the CI manifest so it's executed on the Windows runner (or cross-platform if process.platform guards are added — see F3 below).
Non-blocking
-
F1 — All 27 tests copy
toImportSpecifier()verbatim instead of importing fromindex.ts(line 14: "Copy of the PR #576 implementation"). If the regex inindex.ts:419is reverted or mis-merged, every test still passes. AliceLJY suggested exporting the function (even with a_prefix, or via a dedicatedsrc/import-specifier.ts) — the test cases themselves are well-written, only the import source needs to change. -
F3 — The Windows drive-letter guard at
index.ts:419has noprocess.platform === 'win32'check. On POSIX,pathToFileURL('C:\\Users\\test.js')prepends CWD and URL-encodes backslashes, producing a silently-malformedfile://URL. The APPDATA block 10 lines below already uses the platform guard correctly — same pattern should apply here. -
F6 — File header and inline comments reference "PR #576" (the closed predecessor). Replace with "PR #593" to keep
git blamenavigable. -
MR2 — Test file is not portable and already fails on POSIX (Windows-only path assumptions without platform guards in the test itself).
Review 問題修復完成 ✅已根據 review 意見修復所有問題：
驗證結果：
Commit: |
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
Good direction on this fix — the toImportSpecifier() Windows bug is real and high-impact.
Must fix before merge:
-
MR1 / MR2: Test file is excluded from CI and fails on POSIX. The new test file is not wired into the test runner manifest, so the 27 new tests never execute in CI. Additionally, the tests fail on POSIX (the platform the CI actually runs on). These tests need to be registered in the test manifest and made portable before merge.
-
F1: Tests exercise a copied function body, not the production implementation. The test file inlines a verbatim copy of
toImportSpecifierandgetExtensionApiImportSpecifiersrather than importing fromindex.ts. Regressions in the real function won't be caught. Either export the functions and import them in the test, or restructure so the tests exercise the actual production code.
Nice to have (non-blocking):
- F3:
pathToFileURLis called on Windows-style paths without a platform guard — will silently produce malformed URLs on POSIX. - F6: Test file header and comments reference "PR #576" — this is PR #593. Please update.
- F2: The CLI smoke test failure (
undefined !== 1atcli-smoke.mjs:316) is claimed as pre-existing. Please confirm against a cleanmaincheckout so we know this isn't a regression from the APPDATA path being added to specifiers.
Address the must-fix items and this is ready to merge.
…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
jlin53882
force-pushed
the
fix/windows-extension-api-path-v2
branch
from
7396b39 to
65e56a6
Compare
…in tests (F1) - Export getExtensionApiImportSpecifiers from index.ts:423 (was internal function) - Replace local test copy with jiti import from index.ts (tests now exercise production code) - Remove unused 'join' import from node:path - Update test header comment to reflect both functions are now imported from index.ts
F1 修復完成 ✅（PR #593 追加 commit）Commit： 變更內容
驗證結果
已 rebase 到官方 master 最新 ( |
3 participants
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