fix: add Windows APPDATA path fallback for extensionAPI.js#576
fix: add Windows APPDATA path fallback for extensionAPI.js#576jlin53882 wants to merge 1 commit intoCortexReach:masterfrom
Conversation
On Windows, npm global packages are installed to %APPDATA%\npm\node_modules\. This change adds automatic detection of this path when running on Windows, eliminating the need for users to manually set OPENCLAW_EXTENSION_API_PATH. Fixes CortexReach#575
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8a00774d4
ℹ️ 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 (process.platform === "win32" && process.env.APPDATA) { | ||
| const windowsNpmPath = join(process.env.APPDATA, "npm", "node_modules", "openclaw", "dist", "extensionAPI.js"); | ||
| specifiers.push(toImportSpecifier(windowsNpmPath)); |
There was a problem hiding this comment.
Convert Windows fallback path into a file URL
This fallback still won’t load on Windows because toImportSpecifier() only converts POSIX absolute paths (starting with /) to file:// URLs, so an APPDATA path like C:\Users\...\extensionAPI.js is returned unchanged. In loadEmbeddedPiRunner(), import(specifier) then interprets C: as a URL scheme and fails (typically ERR_UNSUPPORTED_ESM_URL_SCHEME), meaning the new Windows probe cannot succeed unless OPENCLAW_EXTENSION_API_PATH is manually set to a valid import specifier.
Useful? React with 👍 / 👎.
Review:
|
技術分析回應(回覆 Codex Bot #575 分析)你的 Point 有道理,但問題的範圍比描述的更小——不需要改 根因確認
建議修復(方案 B)在 Windows fallback 區塊,直接呼叫 if (process.platform === "win32" && process.env.APPDATA) {
const windowsNpmPath = join(process.env.APPDATA, "npm", "node_modules", "openclaw", "dist", "extensionAPI.js");
specifiers.push(pathToFileURL(windowsNpmPath).href); // 直接轉 file:// URL
}為什麼不選方案 A(改
|
修復說明 — PR #576 Windows Path Fix問題根因(Issue #575)
修復內容1.
|
| 輪次 | 生產程式碼 | 測試 | 結論 |
|---|---|---|---|
| Round 1 | ❌ 未修 | 0 | NEEDS_CHANGES |
| Round 2 | ✅ 正確 | 0 | NEEDS_CHANGES(測試缺口) |
| Round 3 | ✅ 正確 | ✅ 27/27 | APPROVED |
Commit
Branch:fix/windows-extension-api-path-v2
Commit:a799d65
檔案:index.ts(+7 行)+ test/to-import-specifier-windows.test.mjs(+267 行)
注意事項
- UNC 路徑(
\\server\share)不在此 PR 範圍內(regex 需要\\作為 drive letter 後的分隔符) - 原有 3 個測試失敗是預先存在的問題,與本 PR 無關
- 此修復同時解決了
OPENCLAW_EXTENSION_API_PATH環境變數的 Windows 路徑問題(隱藏問題 /lesson命令是自己添加吗 没成功 #1)
修復說明 — PR #576 Windows Path Fix(第二版)問題根因(Issue #575)
修復內容1.
|
| 輪次 | 生產程式碼 | 測試 | 結論 |
|---|---|---|---|
| Round 1 | NEEDS_CHANGES(未修) | 0 | NEEDS_CHANGES |
| Round 2 | 正確 | 0 | NEEDS_CHANGES(測試缺口) |
| Round 3 | 正確 | 27/27 通過 | APPROVED |
Commit(已在本機建立)
Branch:fix/windows-extension-api-path-v2
Commit:a799d65
檔案:index.ts(+7 行)+ test/to-import-specifier-windows.test.mjs(+267 行)
注意事項
- UNC 路徑(
\\server\share)不在此 PR 範圍內 - 原有 3 個測試失敗是預先存在的問題,與本 PR 無關
- 此修復同時解決了
OPENCLAW_EXTENSION_API_PATH環境變數的 Windows 路路徑問題(隱藏問題 /lesson命令是自己添加吗 没成功 #1)
注意:目前無法從本機 push(403 Permission Denied — jlin53882 對 CortexReach/memory-lancedb-pro 沒有寫入權限)。請手動將 fix/windows-extension-api-path-v2 branch push 到你的 fork,或給予 repo 寫入權限後再推。
…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
Superseded by PR #593This PR (#576) was closed because the implementation had a bug — The correct fix is in PR #593: #593 Key difference: instead of working around All 27 tests pass. Three rounds of adversarial Codex review concluded with APPROVED. |
…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
…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
…#593) * fix: add Windows path support in toImportSpecifier + add tests (#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 #576 (already reviewed). ## Problem (Issue #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 #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 #575 * fix: address PR #593 review comments (MR1/F1/F3/F6/MR2) * fix: export getExtensionApiImportSpecifiers and import from index.ts 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 * fix(UNC): extend toImportSpecifier() for UNC path support Address maintainer review comment on PR #593: - UNC paths (\\server\share) were returned unchanged, causing import() to fail in redirected-profile environments where APPDATA points to UNC. - Added Windows-specific UNC detection regex (/^\\\\[^\\]+\\[^\\]+/) and convert to file:// URL via pathToFileURL(). - Extended prefix \\?\UNC\\ also handled (early return, no replace). Tests: - Converted old 'rejects UNC' test to 'converts UNC to file:// URL'. - Added 6 new UNC-specific toImportSpecifier tests. - Added UNC path via OPENCLAW_EXTENSION_API_PATH env var test. - Added pathToFileURL integration test for UNC format. * docs(UNC): clarify comments in toImportSpecifier() — PR #593 * fix(CI): add to-import-specifier-windows.test.mjs to core-regression (MR1) Address rwmjhb review: test file was not registered in CI manifest, so the 34 tests never ran in CI. Added to core-regression group. Also addressed the open question on double-backslash regex: /^[a-zA-Z]:[/\\]/ accepts both C:\\ and C:/ because Windows paths from env vars may use either separator. On POSIX, branch unreachable. * fix(CI): align ci-test-manifest.mjs with verify EXPECTED_BASELINE - Remove 5 orphan entries not in verify baseline (bulk-store*.test.mjs, import-markdown, smart-extractor-bulk-store*.test.mjs) - Fix position and args for to-import-specifier-windows.test.mjs to match verify EXPECTED_BASELINE order * fix(test): add win32 platform guard to getExtensionApiImportSpecifiers env-var tests Issue #696: Skip 2 getExtensionApiImportSpecifiers Windows/UNC env-var tests on non-Windows CI. The production toImportSpecifier() uses process.platform === 'win32' guards, so these tests must have the same guard to avoid running on Linux CI runners. - Wrapped OPENCLAW_EXTENSION_API_PATH Windows path test in if (win32) - Wrapped OPENCLAW_EXTENSION_API_PATH UNC path test in if (win32) - Matches pattern already used by toImportSpecifier() tests (lines 53-120) * docs(test): clean up stale PR #593 comments in test file --------- Co-authored-by: james53882 <james53882@users.noreply.github.com> Co-authored-by: OpenClaw Agent <agent@openclaw.ai>
Summary
Add Windows APPDATA path fallback for
extensionAPI.jsingetExtensionApiImportSpecifiers().Problem
On Windows, npm global packages are installed to
%APPDATA%\npm\node_modules\. ThegetExtensionApiImportSpecifiers()function only checks Unix/macOS paths, causingmemory-reflectionand other features to fail on Windows without manual environment variable configuration.Solution
Add automatic Windows path detection using
process.env.APPDATA:Testing
Fixes
Fixes #575