Skip to content

Commit 9032bf5

Browse files
committed
feat: US-067 - Fix Pi SDK import in NodeRuntime when Pi modules declare __filename
1 parent 17bf666 commit 9032bf5

8 files changed

Lines changed: 166 additions & 34 deletions

File tree

.agent/contracts/node-runtime.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,11 @@ The runtime MUST classify JavaScript modules using Node-compatible metadata rule
240240
- **WHEN** a package has `package.json` with `"type": "module"` and sandboxed code loads `./index.js`
241241
- **THEN** the runtime MUST evaluate the file as ESM semantics (including `import.meta` availability and ESM export behavior)
242242

243+
#### Scenario: Require-transformed ESM keeps module-local `__filename` bindings
244+
- **WHEN** sandboxed code loads a package that is transformed for `require()` compatibility from ESM source using `import.meta.url`, and that source also declares its own `const __filename = fileURLToPath(import.meta.url)` or `const __dirname = dirname(__filename)`
245+
- **THEN** the runtime MUST compile and execute that module without colliding with the CommonJS wrapper parameters
246+
- **AND** any compatibility transform MUST preserve the module's own local bindings instead of rewriting source tokens to the wrapper globals
247+
243248
#### Scenario: .js under type commonjs is treated as CJS
244249
- **WHEN** a package has `package.json` with `"type": "commonjs"` (or no ESM override) and sandboxed code loads `./index.js` via `require`
245250
- **THEN** the runtime MUST evaluate the file as CommonJS and return `module.exports`

CLAUDE.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@
146146
- vendored `http2` nghttp2 error-path tests patch `internal/test/binding` `Http2Stream.prototype.respond`; keep that shim wired to the same bridge-facing `Http2Stream` / `internal/http2/util`.`NghttpError` constructors the runtime uses, or the tests stop exercising the real wrapper logic
147147
- bridge exports that userland constructs with `new` must be assigned as constructable function properties, not object-literal method shorthands; shorthand methods like `createReadStream() {}` are not constructable and vendored fs coverage calls `new fs.createReadStream(...)`
148148
- `/proc/sys/kernel/hostname` conformance hits both kernel-backed and standalone NodeRuntime paths; a procfs fix that only lands in the kernel layer still leaves `createTestNodeRuntime()` fs/FileHandle coverage red
149+
- require-transformed ESM must not rely on the CommonJS wrapper's `__filename` / `__dirname` parameter names; keep wrapper internals on private names, synthesize local CJS bindings only for plain CommonJS sources, and compute transformed `import.meta.url` from `pathToFileURL(__secureExecFilename).href`
149150

150151
## Virtual Kernel Architecture
151152

@@ -172,7 +173,7 @@
172173
- instead, use proper tooling: `es-module-lexer` / `cjs-module-lexer` (the same WASM-based lexers Node.js uses), or run the transformation inside the V8 isolate where the JS engine handles parsing correctly
173174
- if a source transformation is needed at the bridge/host level, prefer a battle-tested library over hand-rolled regex
174175
- the V8 runtime already has dual-mode execution (`execute_script` for CJS, `execute_module` for ESM) — lean on V8's native module system rather than pre-transforming source on the host side
175-
- existing regex-based transforms (e.g., `convertEsmToCjs`, `transformDynamicImport`, `isESM`) are known technical debt and should be replaced
176+
- existing regex-based transforms (e.g., `convertEsmToCjs`, `transformDynamicImport`, `isESM`) are known technical debt and should be replaced; when `require()` compatibility needs `import.meta.url`, inject an internal file-URL helper instead of rewriting to the wrapper `__filename`
176177

177178

178179
## Contracts (CRITICAL)

packages/core/isolate-runtime/src/inject/require-setup.ts

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// @ts-nocheck
22
// This file is executed inside the isolate runtime.
3+
const REQUIRE_TRANSFORM_MARKER = '/*__secure_exec_require_esm__*/';
34
const __requireExposeCustomGlobal =
45
typeof globalThis.__runtimeExposeCustomGlobal === "function"
56
? globalThis.__runtimeExposeCustomGlobal
@@ -3973,17 +3974,6 @@
39733974
return parsed;
39743975
}
39753976

3976-
// Some CJS artifacts include import.meta.url probes that are valid in
3977-
// ESM but a syntax error in Function()-compiled CJS wrappers.
3978-
const normalizedSource =
3979-
typeof source === 'string'
3980-
? source
3981-
.replace(/import\.meta\.url/g, '__filename')
3982-
.replace(/fileURLToPath\(__filename\)/g, '__filename')
3983-
.replace(/url\.fileURLToPath\(__filename\)/g, '__filename')
3984-
.replace(/fileURLToPath\.call\(void 0, __filename\)/g, '__filename')
3985-
: source;
3986-
39873977
// Create module object
39883978
const module = {
39893979
exports: {},
@@ -4001,15 +3991,22 @@
40013991
try {
40023992
// Wrap and execute the code
40033993
let wrapper;
3994+
const isRequireTransformedEsm =
3995+
typeof source === 'string' &&
3996+
source.startsWith(REQUIRE_TRANSFORM_MARKER);
3997+
const wrapperPrologue = isRequireTransformedEsm
3998+
? ''
3999+
: "var __filename = __secureExecFilename;\n" +
4000+
"var __dirname = __secureExecDirname;\n";
40044001
try {
40054002
wrapper = new Function(
40064003
'exports',
40074004
'require',
40084005
'module',
4009-
'__filename',
4010-
'__dirname',
4006+
'__secureExecFilename',
4007+
'__secureExecDirname',
40114008
'__dynamicImport',
4012-
normalizedSource + '\n//# sourceURL=' + resolved
4009+
wrapperPrologue + source + '\n//# sourceURL=' + resolved
40134010
);
40144011
} catch (error) {
40154012
const details =

packages/core/src/generated/isolate-runtime.ts

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

packages/nodejs/src/module-source.ts

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
import { transform, transformSync } from "esbuild";
22
import { init, initSync, parse } from "es-module-lexer";
33

4+
const REQUIRE_TRANSFORM_MARKER = "/*__secure_exec_require_esm__*/";
5+
const IMPORT_META_URL_HELPER = "__secureExecImportMetaUrl__";
6+
47
function isJavaScriptLikePath(filePath: string | undefined): boolean {
58
return filePath === undefined || /\.[cm]?[jt]sx?$/.test(filePath);
69
}
@@ -12,21 +15,26 @@ function parseSourceSyntax(source: string, filePath?: string) {
1215
return { hasModuleSyntax, hasDynamicImport, hasImportMeta };
1316
}
1417

15-
function shouldTransformForRequire(source: string, filePath?: string): boolean {
16-
if (!isJavaScriptLikePath(filePath)) {
17-
return false;
18+
function getRequireTransformOptions(
19+
filePath: string,
20+
syntax: ReturnType<typeof parseSourceSyntax>,
21+
) {
22+
const requiresEsmWrapper =
23+
syntax.hasModuleSyntax || syntax.hasImportMeta;
24+
const bannerLines = requiresEsmWrapper ? [REQUIRE_TRANSFORM_MARKER] : [];
25+
if (syntax.hasImportMeta) {
26+
bannerLines.push(
27+
`const ${IMPORT_META_URL_HELPER} = require("node:url").pathToFileURL(__secureExecFilename).href;`,
28+
);
1829
}
1930

20-
const { hasModuleSyntax, hasDynamicImport, hasImportMeta } =
21-
parseSourceSyntax(source, filePath);
22-
return hasModuleSyntax || hasDynamicImport || hasImportMeta;
23-
}
24-
25-
function getRequireTransformOptions(filePath: string) {
2631
return {
27-
define: {
28-
"import.meta.url": "__filename",
29-
},
32+
banner: bannerLines.length > 0 ? bannerLines.join("\n") : undefined,
33+
define: syntax.hasImportMeta
34+
? {
35+
"import.meta.url": IMPORT_META_URL_HELPER,
36+
}
37+
: undefined,
3038
format: "cjs" as const,
3139
loader: "js" as const,
3240
platform: "node" as const,
@@ -62,12 +70,13 @@ export function transformSourceForRequireSync(
6270
}
6371

6472
initSync();
65-
if (!shouldTransformForRequire(source, filePath)) {
73+
const syntax = parseSourceSyntax(source, filePath);
74+
if (!(syntax.hasModuleSyntax || syntax.hasDynamicImport || syntax.hasImportMeta)) {
6675
return source;
6776
}
6877

6978
try {
70-
return transformSync(source, getRequireTransformOptions(filePath)).code;
79+
return transformSync(source, getRequireTransformOptions(filePath, syntax)).code;
7180
} catch {
7281
return source;
7382
}
@@ -82,13 +91,14 @@ export async function transformSourceForRequire(
8291
}
8392

8493
await init;
85-
if (!shouldTransformForRequire(source, filePath)) {
94+
const syntax = parseSourceSyntax(source, filePath);
95+
if (!(syntax.hasModuleSyntax || syntax.hasDynamicImport || syntax.hasImportMeta)) {
8696
return source;
8797
}
8898

8999
try {
90100
return (
91-
await transform(source, getRequireTransformOptions(filePath))
101+
await transform(source, getRequireTransformOptions(filePath, syntax))
92102
).code;
93103
} catch {
94104
return source;

packages/secure-exec/tests/runtime-driver/node/standalone-dist-smoke.test.ts

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
import { execFile } from "node:child_process";
2+
import { mkdtemp, rm, writeFile } from "node:fs/promises";
3+
import { tmpdir } from "node:os";
24
import { dirname, resolve } from "node:path";
35
import { fileURLToPath, pathToFileURL } from "node:url";
46
import { promisify } from "node:util";
@@ -162,4 +164,94 @@ describe("standalone dist bootstrap", () => {
162164
exports: { answer: 42 },
163165
});
164166
}, 30_000);
167+
168+
it("imports require-transformed ESM modules that declare __filename from import.meta.url", async () => {
169+
const fixtureDir = await mkdtemp(
170+
resolve(tmpdir(), "secure-exec-import-meta-url-"),
171+
);
172+
const fixturePath = resolve(fixtureDir, "entry.mjs");
173+
await writeFile(
174+
fixturePath,
175+
[
176+
'import { dirname } from "node:path";',
177+
'import { fileURLToPath } from "node:url";',
178+
'const __filename = fileURLToPath(import.meta.url);',
179+
"const __dirname = dirname(__filename);",
180+
"export const filePath = __filename;",
181+
"export const dirPath = __dirname;",
182+
].join("\n"),
183+
);
184+
185+
try {
186+
const stdout = await runStandaloneScript(`
187+
import {
188+
NodeRuntime,
189+
allowAll,
190+
createNodeDriver,
191+
createNodeRuntimeDriverFactory,
192+
} from ${JSON.stringify(DIST_INDEX_URL)};
193+
194+
const stdio = [];
195+
const runtime = new NodeRuntime({
196+
onStdio: (event) => {
197+
if (event.channel === "stderr") {
198+
stdio.push(event.message);
199+
}
200+
},
201+
systemDriver: createNodeDriver({
202+
moduleAccess: { cwd: ${JSON.stringify(WORKSPACE_ROOT)} },
203+
permissions: allowAll,
204+
}),
205+
runtimeDriverFactory: createNodeRuntimeDriverFactory(),
206+
});
207+
208+
const result = await runtime.exec(
209+
\`(() => {
210+
try {
211+
const mod = require(${JSON.stringify(fixturePath)});
212+
if (mod.filePath !== ${JSON.stringify(fixturePath)}) {
213+
throw new Error("unexpected filePath export: " + mod.filePath);
214+
}
215+
console.log(JSON.stringify({
216+
ok: true,
217+
filePath: mod.filePath,
218+
dirPath: mod.dirPath,
219+
}));
220+
} catch (error) {
221+
console.error(String(error));
222+
console.error(error && error.stack ? error.stack : "");
223+
process.exitCode = 1;
224+
}
225+
})();\`,
226+
{ cwd: ${JSON.stringify(WORKSPACE_ROOT)} },
227+
);
228+
229+
await runtime.terminate();
230+
await new Promise((resolve, reject) => {
231+
process.stdout.write(JSON.stringify({
232+
code: result.code,
233+
stderr: stdio.join(""),
234+
}), (error) => {
235+
if (error) {
236+
reject(error);
237+
return;
238+
}
239+
resolve();
240+
});
241+
});
242+
process.exit(0);
243+
`);
244+
245+
const result = JSON.parse(stdout) as {
246+
code: number;
247+
stderr: string;
248+
};
249+
250+
expect(result.code).toBe(0);
251+
expect(result.stderr).not.toContain("Identifier '__filename' has already been declared");
252+
expect(result.stderr).toBe("");
253+
} finally {
254+
await rm(fixtureDir, { recursive: true, force: true });
255+
}
256+
}, 30_000);
165257
});

scripts/ralph/prd.json

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1125,8 +1125,24 @@
11251125
"Typecheck passes"
11261126
],
11271127
"priority": 53.1,
1128+
"passes": true,
1129+
"notes": "Fixed in the SecureExec runtime/module-loader layer. `transformSourceForRequire*()` now injects an internal file-URL helper for `import.meta.url` and marks require-transformed ESM, while the isolate CommonJS loader keeps wrapper filename/dirname on private parameter names and only synthesizes local `__filename`/`__dirname` bindings for plain CommonJS sources. Result: importing `@mariozechner/pi-coding-agent@0.60.0` through NodeRuntime no longer fails with `SyntaxError: Identifier '__filename' has already been declared`; the Pi import chain now advances to the next concrete blocker (`ENOENT` for `dist/package.json`)."
1130+
},
1131+
{
1132+
"id": "US-068",
1133+
"title": "Fix Pi package asset discovery after SDK import bootstrap",
1134+
"description": "As a developer, I need Pi's package metadata and bundled assets to resolve correctly inside SecureExec after the import bootstrap succeeds, so the SDK can continue booting beyond `dist/config.js`.",
1135+
"acceptanceCriteria": [
1136+
"Reproduce the new blocker after US-067 by importing `@mariozechner/pi-coding-agent@0.60.0` through NodeRuntime and observing the `ENOENT` for `dist/package.json`",
1137+
"SecureExec resolves Pi's package metadata/assets from the actual package root instead of trying to open `dist/package.json`",
1138+
"The fix applies in SecureExec's runtime/module/filesystem layer rather than by patching Pi locally",
1139+
"The Pi SDK real-provider harness advances beyond package-dir/bootstrap asset discovery and either completes successfully or exposes the next concrete blocker",
1140+
"Tests pass",
1141+
"Typecheck passes"
1142+
],
1143+
"priority": 53.2,
11281144
"passes": false,
1129-
"notes": "Blocked by SecureExec runtime/module-wrapper behavior. Exact repro as of 2026-03-26: `@mariozechner/pi-coding-agent@0.60.0` imports successfully in host Node, but inside NodeRuntime the import chain reaches `dist/config.js` and fails with `SyntaxError: Identifier '__filename' has already been declared`. Proposed unblock path: inspect the CommonJS compilation wrapper / loader path that injects `__filename`/`__dirname` and make it tolerate upstream packages that declare those identifiers in module scope."
1145+
"notes": "Next blocker exposed by US-067. Exact repro as of 2026-03-26: after the `__filename` collision fix, importing `@mariozechner/pi-coding-agent@0.60.0` through NodeRuntime reaches `dist/config.js` and then fails with `ENOENT: no such file or directory, open '<repo>/node_modules/.pnpm/.../@mariozechner/pi-coding-agent/dist/package.json'`. Proposed unblock path: inspect how the sandboxed FS / path resolution is handling Pi's package-root detection and why `getPackageDir()` stops at `dist/` instead of the package root."
11301146
},
11311147
{
11321148
"id": "US-062",

scripts/ralph/progress.txt

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
- For overfitting-prone kernel/network stories, keep host-vs-sandbox cross-validation tests even when they currently expose a mismatch; assert the concrete exit code/stderr/raw-byte delta instead of downgrading back to another same-code-path loopback test.
1616
- Kernel-owned `SocketTable` instances must validate socket owner PIDs against the shared process table; only standalone/internal tables should omit that validation and stay opt-in.
1717
- `packages/wasmvm/src/driver.ts` resolves the worker entry to `packages/wasmvm/dist/kernel-worker.js` unless a sibling `src/kernel-worker.js` exists, so `packages/wasmvm/src/kernel-worker.ts` changes are not live until `pnpm --filter @secure-exec/wasmvm build` refreshes the worker bundle.
18-
- Replace active Node loader rewrites with parser-backed transforms: use `es-module-lexer` for `.js` module-syntax detection and `esbuild` CJS transforms with `supported: { "dynamic-import": false }` plus `define: { "import.meta.url": "__filename" }` so `require()` keeps module-relative `import()` behavior without touching strings/comments.
18+
- Replace active Node loader rewrites with parser-backed transforms: use `es-module-lexer` for `.js` module-syntax detection and `esbuild` CJS transforms with `supported: { "dynamic-import": false }`, inject a private file-URL helper from `pathToFileURL(__secureExecFilename).href`, and keep wrapper `__filename` / `__dirname` bindings private so require-transformed ESM can declare its own locals without collisions.
1919
- Standalone `NodeExecutionDriver` should always keep an internal `SocketTable` for sandbox loopback routing, but it must only attach `createNodeHostNetworkAdapter()` when `SystemDriver.network` is explicitly configured; otherwise omitted network capability silently regains host TCP access.
2020
- Bridge-level HTTP/fetch client work needs explicit in-flight tracking in `NodeExecutionDriver` state; otherwise `runtime.exec()` can finish before async bridge requests settle and payload-limit failures turn into false green exits.
2121
- Kernel-backed HTTP client routing should be limited to the loopback-aware default Node adapter path; preserve a documented legacy fallback for custom `NetworkAdapter` injections so adapter-focused tests and no-network stubs still exercise their own behavior.
@@ -722,3 +722,14 @@
722722
- For investigation stories that may uncover a runtime blocker, the opt-in harness should accept either true end-to-end success or the pinned blocker signature so the test stays useful before and after the runtime fix lands.
723723
- The current Pi SDK blocker is in SecureExec's module/runtime layer, not Pi's provider stack: host Node completes the real-token prompt, but sandbox import of `dist/config.js` dies on a duplicate `__filename` declaration before any provider request is made.
724724
---
725+
## [2026-03-26 13:28 PDT] - US-067
726+
- Fixed the NodeRuntime require-transform path so ESM modules can declare their own `const __filename = fileURLToPath(import.meta.url)` without colliding with SecureExec's CommonJS wrapper globals.
727+
- Updated the host transform to inject a private file-URL helper for `import.meta.url`, taught the isolate loader to keep wrapper filename/dirname on private parameter names and only synthesize local `__filename` / `__dirname` bindings for plain CommonJS sources, and added a standalone-dist regression that requires a transformed ESM fixture outside Vitest's source transforms.
728+
- Confirmed the Pi SDK import now advances past the old bootstrap failure: importing `@mariozechner/pi-coding-agent@0.60.0` through `packages/secure-exec/dist/index.js` no longer throws `SyntaxError: Identifier '__filename' has already been declared` and instead exposes the next blocker, `ENOENT ... /dist/package.json`; recorded that follow-up as `US-068`.
729+
- Verified with `pnpm vitest run packages/secure-exec/tests/runtime-driver/node/standalone-dist-smoke.test.ts`, `pnpm --filter @secure-exec/nodejs check-types`, `pnpm --filter @secure-exec/core check-types`, and `pnpm --filter secure-exec check-types`.
730+
- Files changed: `AGENTS.md`, `.agent/contracts/node-runtime.md`, `packages/core/isolate-runtime/src/inject/require-setup.ts`, `packages/core/src/generated/isolate-runtime.ts`, `packages/nodejs/src/module-source.ts`, `packages/secure-exec/tests/runtime-driver/node/standalone-dist-smoke.test.ts`, `scripts/ralph/prd.json`, `scripts/ralph/progress.txt`
731+
- **Learnings for future iterations:**
732+
- Require-transformed ESM needs different wrapper treatment than plain CommonJS: keep runtime filename/dirname on private parameter names and only synthesize `__filename` / `__dirname` locals for true CommonJS sources.
733+
- `import.meta.url` compatibility cannot be implemented by rewriting to the wrapper `__filename`; it must stay a file URL string, which in this runtime means deriving it from `pathToFileURL(__secureExecFilename).href`.
734+
- Once the duplicate-binding bootstrap bug is removed, Pi v0.60.0 immediately hits package-root discovery (`dist/package.json` ENOENT), so the next investigation should focus on sandboxed filesystem/path resolution rather than provider traffic.
735+
---

0 commit comments

Comments
 (0)