Skip to content

Commit a799d65

Browse files
committed
fix: add Windows path support in toImportSpecifier + add tests (CortexReach#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
1 parent 0988a46 commit a799d65

2 files changed

Lines changed: 267 additions & 0 deletions

File tree

index.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,8 @@ function toImportSpecifier(value: string): string {
416416
if (!trimmed) return "";
417417
if (trimmed.startsWith("file://")) return trimmed;
418418
if (trimmed.startsWith("/")) return pathToFileURL(trimmed).href;
419+
// Handle Windows absolute paths (e.g. C:\Users\... or D:/Program Files/...)
420+
if (/^[a-zA-Z]:[/\\]/.test(trimmed)) return pathToFileURL(trimmed).href;
419421
return trimmed;
420422
}
421423
function getExtensionApiImportSpecifiers(): string[] {
@@ -435,6 +437,11 @@ function getExtensionApiImportSpecifiers(): string[] {
435437
specifiers.push(toImportSpecifier("/usr/local/lib/node_modules/openclaw/dist/extensionAPI.js"));
436438
specifiers.push(toImportSpecifier("/opt/homebrew/lib/node_modules/openclaw/dist/extensionAPI.js"));
437439

440+
if (process.platform === "win32" && process.env.APPDATA) {
441+
const windowsNpmPath = join(process.env.APPDATA, "npm", "node_modules", "openclaw", "dist", "extensionAPI.js");
442+
specifiers.push(toImportSpecifier(windowsNpmPath));
443+
}
444+
438445
return [...new Set(specifiers.filter(Boolean))];
439446
}
440447

Lines changed: 260 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,260 @@
1+
/**
2+
* Test: toImportSpecifier and Windows path fallback
3+
* PR #576 - Windows APPDATA path fallback for extensionAPI.js
4+
*
5+
* Tests the behavior of `toImportSpecifier` and `getExtensionApiImportSpecifiers`
6+
* using local implementations that mirror the PR #576 code exactly.
7+
* Functions are NOT exported from index.ts, so we copy the logic to test it.
8+
*/
9+
import { describe, it } from "node:test";
10+
import assert from "node:assert/strict";
11+
import { join } from "node:path";
12+
import { pathToFileURL } from "node:url";
13+
14+
// Copy of the PR #576 toImportSpecifier implementation (index.ts:414-423)
15+
function toImportSpecifier(value) {
16+
const trimmed = value.trim();
17+
if (!trimmed) return "";
18+
if (trimmed.startsWith("file://")) return trimmed;
19+
if (trimmed.startsWith("/")) return pathToFileURL(trimmed).href;
20+
// Handle Windows absolute paths (e.g. C:\Users\... or D:/Program Files/...)
21+
if (/^[a-zA-Z]:[/\\]/.test(trimmed)) return pathToFileURL(trimmed).href;
22+
return trimmed;
23+
}
24+
25+
// Copy of the PR #576 getExtensionApiImportSpecifiers implementation (index.ts:425-444)
26+
// Note: intentionally does NOT include the requireFromHere.resolve() call (dead code)
27+
function getExtensionApiImportSpecifiers() {
28+
const envPath = process.env.OPENCLAW_EXTENSION_API_PATH?.trim();
29+
const specifiers = [];
30+
31+
if (envPath) specifiers.push(toImportSpecifier(envPath));
32+
specifiers.push("openclaw/dist/extensionAPI.js");
33+
34+
specifiers.push(toImportSpecifier("/usr/lib/node_modules/openclaw/dist/extensionAPI.js"));
35+
specifiers.push(toImportSpecifier("/usr/local/lib/node_modules/openclaw/dist/extensionAPI.js"));
36+
specifiers.push(toImportSpecifier("/opt/homebrew/lib/node_modules/openclaw/dist/extensionAPI.js"));
37+
38+
if (process.platform === "win32" && process.env.APPDATA) {
39+
const windowsNpmPath = join(process.env.APPDATA, "npm", "node_modules", "openclaw", "dist", "extensionAPI.js");
40+
specifiers.push(toImportSpecifier(windowsNpmPath));
41+
}
42+
43+
return [...new Set(specifiers.filter(Boolean))];
44+
}
45+
46+
// Env helper: set key to value, run fn, restore original
47+
function withEnv(key, value, fn) {
48+
const original = process.env[key];
49+
if (value === undefined) {
50+
delete process.env[key];
51+
} else {
52+
process.env[key] = value;
53+
}
54+
try {
55+
fn();
56+
} finally {
57+
if (original === undefined) {
58+
delete process.env[key];
59+
} else {
60+
process.env[key] = original;
61+
}
62+
}
63+
}
64+
65+
// ============================================================================
66+
// toImportSpecifier tests
67+
// ============================================================================
68+
69+
describe("toImportSpecifier", () => {
70+
// --- POSIX paths ---
71+
it("converts POSIX absolute path to file:// URL", () => {
72+
const result = toImportSpecifier("/usr/local/lib/node_modules/openclaw/dist/extensionAPI.js");
73+
assert.ok(result.startsWith("file://"), `Expected file:// URL, got: ${result}`);
74+
assert.ok(result.includes("/usr/local/lib"));
75+
});
76+
77+
it("converts POSIX path with spaces to file:// URL", () => {
78+
const result = toImportSpecifier("/opt/My App/node_modules/test.js");
79+
assert.ok(result.startsWith("file://"), `Expected file:// URL, got: ${result}`);
80+
});
81+
82+
// --- Windows paths (PR #576 new fix) ---
83+
it("converts Windows drive-letter backslash path to file:// URL", () => {
84+
const result = toImportSpecifier("C:\\Users\\admin\\AppData\\Roaming\\npm\\node_modules\\openclaw\\dist\\extensionAPI.js");
85+
assert.ok(result.startsWith("file://"), `Expected file:// URL, got: ${result}`);
86+
assert.ok(result.includes("C:/"), `Expected C:/ prefix, got: ${result}`);
87+
});
88+
89+
it("converts Windows drive-letter forward-slash path to file:// URL", () => {
90+
const result = toImportSpecifier("D:/Program Files/openclaw/dist/extensionAPI.js");
91+
assert.ok(result.startsWith("file://"), `Expected file:// URL, got: ${result}`);
92+
assert.ok(result.includes("D:/"), `Expected D:/ prefix, got: ${result}`);
93+
});
94+
95+
it("converts Windows path with spaces to file:// URL", () => {
96+
const result = toImportSpecifier("E:\\code\\my project\\file.js");
97+
assert.ok(result.startsWith("file://"), `Expected file:// URL, got: ${result}`);
98+
});
99+
100+
it("rejects Windows drive letter without separator (C: -> unchanged)", () => {
101+
const result = toImportSpecifier("C:");
102+
assert.equal(result, "C:");
103+
});
104+
105+
it("rejects DOS 8.3 short path (C:path\\to\\file.js -> unchanged)", () => {
106+
const result = toImportSpecifier("C:path\\to\\file.js");
107+
assert.equal(result, "C:path\\to\\file.js");
108+
});
109+
110+
it("rejects single-backslash UNC-like path (\\server\\share -> unchanged)", () => {
111+
const result = toImportSpecifier("\\server\\share\\file.js");
112+
assert.equal(result, "\\server\\share\\file.js");
113+
});
114+
115+
// --- Pass-through cases ---
116+
it("passes through file:// POSIX URL unchanged", () => {
117+
const input = "file:///usr/local/lib/extensionAPI.js";
118+
const result = toImportSpecifier(input);
119+
assert.equal(result, input);
120+
});
121+
122+
it("passes through file:// Windows path unchanged", () => {
123+
const input = "file:///C:/Users/admin/AppData/Roaming/test.js";
124+
const result = toImportSpecifier(input);
125+
assert.equal(result, input);
126+
});
127+
128+
it("passes through bare module specifier unchanged", () => {
129+
const input = "openclaw/dist/extensionAPI.js";
130+
const result = toImportSpecifier(input);
131+
assert.equal(result, input);
132+
});
133+
134+
it("passes through relative path unchanged", () => {
135+
const input = "./lib/extensionAPI.js";
136+
const result = toImportSpecifier(input);
137+
assert.equal(result, input);
138+
});
139+
140+
// --- Edge cases ---
141+
it("returns empty string for whitespace-only input", () => {
142+
const result = toImportSpecifier(" ");
143+
assert.equal(result, "");
144+
});
145+
146+
it("handles path with trailing slash", () => {
147+
const result = toImportSpecifier("C:\\Users\\admin\\");
148+
assert.ok(result.startsWith("file://"), `Expected file:// URL, got: ${result}`);
149+
});
150+
151+
it("handles lowercase drive letter", () => {
152+
const result = toImportSpecifier("c:\\users\\test\\file.js");
153+
assert.ok(result.startsWith("file://"), `Expected file:// URL, got: ${result}`);
154+
});
155+
156+
it("handles uppercase drive letter", () => {
157+
const result = toImportSpecifier("E:\\Users\\Admin\\Desktop\\file.js");
158+
assert.ok(result.startsWith("file://"), `Expected file:// URL, got: ${result}`);
159+
});
160+
});
161+
162+
// ============================================================================
163+
// getExtensionApiImportSpecifiers tests
164+
// ============================================================================
165+
166+
describe("getExtensionApiImportSpecifiers", () => {
167+
it("always includes bare module specifier", () => {
168+
const specifiers = getExtensionApiImportSpecifiers();
169+
assert.ok(specifiers.includes("openclaw/dist/extensionAPI.js"), "Should include bare module specifier");
170+
});
171+
172+
it("includes OPENCLAW_EXTENSION_API_PATH POSIX path as file:// URL", () => {
173+
withEnv("OPENCLAW_EXTENSION_API_PATH", "/custom/path/extensionAPI.js", () => {
174+
const specifiers = getExtensionApiImportSpecifiers();
175+
const found = specifiers.find(s => s.includes("/custom/path"));
176+
assert.ok(found, `Expected custom path, got: ${JSON.stringify(specifiers)}`);
177+
assert.ok(found.startsWith("file://"), `Expected file:// URL, got: ${found}`);
178+
});
179+
});
180+
181+
it("converts OPENCLAW_EXTENSION_API_PATH Windows path to file:// URL (hidden issue #1 fix)", () => {
182+
withEnv("OPENCLAW_EXTENSION_API_PATH", "C:\\Program Files\\openclaw\\dist\\extensionAPI.js", () => {
183+
const specifiers = getExtensionApiImportSpecifiers();
184+
const winSpec = specifiers.find(s => s.startsWith("file:///C:/") && s.includes("openclaw") && s.includes("dist") && s.includes("extensionAPI"));
185+
assert.ok(winSpec, `Expected Windows path as file:// URL: ${JSON.stringify(specifiers)}`);
186+
assert.ok(winSpec.includes("Program") || winSpec.includes("Program%20"), `Expected Program Files in path, got: ${winSpec}`);
187+
});
188+
});
189+
190+
it("includes POSIX fallback paths on all platforms", () => {
191+
const specifiers = getExtensionApiImportSpecifiers();
192+
assert.ok(specifiers.some(s => s.includes("/usr/lib")), `Expected /usr/lib path, got: ${JSON.stringify(specifiers)}`);
193+
assert.ok(specifiers.some(s => s.includes("/usr/local")), `Expected /usr/local path, got: ${JSON.stringify(specifiers)}`);
194+
assert.ok(specifiers.some(s => s.includes("/opt/homebrew")), `Expected /opt/homebrew path, got: ${JSON.stringify(specifiers)}`);
195+
});
196+
197+
it("returns deduped specifiers (no duplicates)", () => {
198+
const specifiers = getExtensionApiImportSpecifiers();
199+
const unique = [...new Set(specifiers)];
200+
assert.equal(specifiers.length, unique.length, `Found duplicate specifiers: ${JSON.stringify(specifiers)}`);
201+
});
202+
203+
it("does not include empty strings", () => {
204+
const specifiers = getExtensionApiImportSpecifiers();
205+
assert.ok(!specifiers.includes(""), "Should not contain empty strings");
206+
assert.ok(!specifiers.some(s => typeof s === "string" && s.trim() === ""), "Should not contain whitespace-only strings");
207+
});
208+
209+
it("on non-win32, does NOT add APPDATA fallback", () => {
210+
if (process.platform !== "win32") {
211+
const specifiers = getExtensionApiImportSpecifiers();
212+
const hasAppData = specifiers.some(s => s.includes("AppData") && s.includes("npm"));
213+
assert.ok(!hasAppData, "Non-Windows should not add APPDATA fallback");
214+
}
215+
});
216+
217+
it("on win32 with APPDATA, includes APPDATA fallback as file:// URL", () => {
218+
if (process.platform === "win32" && process.env.APPDATA) {
219+
const specifiers = getExtensionApiImportSpecifiers();
220+
const appDataSpec = specifiers.find(s => s.includes("AppData") && s.includes("npm"));
221+
assert.ok(appDataSpec, `Expected APPDATA path in specifiers: ${JSON.stringify(specifiers)}`);
222+
assert.ok(appDataSpec.startsWith("file://"), `APPDATA specifier should be file:// URL, got: ${appDataSpec}`);
223+
}
224+
});
225+
226+
it("on win32 without APPDATA env var, does not crash", () => {
227+
if (process.platform === "win32") {
228+
const original = process.env.APPDATA;
229+
delete process.env.APPDATA;
230+
try {
231+
// Should not throw - just skip the APPDATA fallback
232+
const specifiers = getExtensionApiImportSpecifiers();
233+
assert.ok(Array.isArray(specifiers), "Should return array even without APPDATA");
234+
} finally {
235+
if (original !== undefined) process.env.APPDATA = original;
236+
}
237+
}
238+
});
239+
});
240+
241+
// ============================================================================
242+
// Integration: pathToFileURL Windows path conversion
243+
// ============================================================================
244+
245+
describe("pathToFileURL Windows path conversion", () => {
246+
it("produces valid file:// URL from Windows backslash path", async () => {
247+
const { pathToFileURL } = await import("node:url");
248+
const input = "C:\\Users\\admin\\AppData\\Roaming\\npm\\node_modules\\openclaw\\dist\\extensionAPI.js";
249+
const result = pathToFileURL(input).href;
250+
assert.equal(result, "file:///C:/Users/admin/AppData/Roaming/npm/node_modules/openclaw/dist/extensionAPI.js");
251+
});
252+
253+
it("produces valid file:// URL from Windows forward-slash path", async () => {
254+
const { pathToFileURL } = await import("node:url");
255+
const input = "D:/Program Files/openclaw/dist/extensionAPI.js";
256+
const result = pathToFileURL(input).href;
257+
assert.ok(result.startsWith("file://"));
258+
assert.ok(result.includes("D:/"));
259+
});
260+
});

0 commit comments

Comments
 (0)