diff --git a/docs/tools/eval.md b/docs/tools/eval.md index cf33d61bf..5d62721e2 100644 --- a/docs/tools/eval.md +++ b/docs/tools/eval.md @@ -134,7 +134,7 @@ Implemented in `packages/coding-agent/src/eval/js/context-manager.ts` and `packa - Persistent `vm.Context` instances keyed by `js:${sessionId}` in `vmContexts` - `rst` calls `resetVmContext(sessionKey)` before the cell executes - Top-level `await` and bare `return` are supported by wrapping code in an async IIFE when `wrapCode()` sees `await` or `return` -- Top-level static `import ... from ...` is rewritten to `await import(...)` by `rewriteStaticImports()` +- Top-level static `import ... from ...` and dynamic `import(...)` calls are routed through `rewriteImports()`, which sends them via `__omp_import__` so the specifier resolves against the session cwd - The prelude installs globals: - `display`, `print` - `read`, `write`, `append`, `sort`, `uniq`, `counter`, `diff`, `tree`, `env`, `output` diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a4653f6c6..fc6fe8f02 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -23,6 +23,8 @@ ### Fixed +- Fixed `eval` tool dynamic `await import("./relative.ts")` calls failing with `Cannot find module ... from .../eval/js/shared/runtime.ts`. The static-import rewriter only handled `ImportDeclaration` nodes, so dynamic-import call expressions resolved their specifier against the worker module's URL instead of the session cwd. The rewriter now walks the full AST and additionally swaps `import` callees in `CallExpression` nodes for `__omp_import__`, which forwards the optional options bag verbatim to native `import()` so `{ with: { type: "json" } }` round-trips. Renamed `rewriteStaticImports` → `rewriteImports`. + - Fixed `pr:////diff` URLs for repositories named `diff` to continue resolving to PR list lookups instead of being parsed as short-form diff links - Fixed PR unified diff parsing so changed-file headers with quoted paths (such as paths containing spaces) are now detected correctly and hunk content lines beginning with `---`/`+++` are counted in additions/deletions - Fixed GitHub view caching to account for active credential identity and avoid serving cached issue/PR data across different account/token contexts diff --git a/packages/coding-agent/src/eval/js/context-manager.ts b/packages/coding-agent/src/eval/js/context-manager.ts index c12550039..c03ebd1fb 100644 --- a/packages/coding-agent/src/eval/js/context-manager.ts +++ b/packages/coding-agent/src/eval/js/context-manager.ts @@ -17,7 +17,7 @@ import type { WorkerOutbound, } from "./worker-protocol"; -export { rewriteStaticImports } from "./shared/rewrite-imports"; +export { rewriteImports } from "./shared/rewrite-imports"; export type { JsDisplayOutput } from "./worker-protocol"; export interface VmRunState { diff --git a/packages/coding-agent/src/eval/js/shared/rewrite-imports.ts b/packages/coding-agent/src/eval/js/shared/rewrite-imports.ts index 6e3e8eb6e..84b9e50e7 100644 --- a/packages/coding-agent/src/eval/js/shared/rewrite-imports.ts +++ b/packages/coding-agent/src/eval/js/shared/rewrite-imports.ts @@ -1,9 +1,10 @@ import { parse as babelParse } from "@babel/parser"; -// Static ESM `import` declarations are not valid inside vm.runInContext (script-mode parsing). -// We rewrite top-level imports to dynamic-import expressions in the user-supplied source so -// pasted ESM runs verbatim. A real parser keeps imports embedded in string literals, template -// literals, or comments intact. +// Static ESM `import` declarations are not valid inside vm.runInContext (script-mode parsing), +// and dynamic `import(...)` would otherwise resolve specifiers against the worker module's URL +// instead of the session cwd. We rewrite both forms so they route through the worker-injected +// `__omp_import__` helper, which resolves the specifier against the active session cwd. A real +// parser keeps imports embedded in string literals, template literals, or comments intact. type BabelImportDeclaration = { type: "ImportDeclaration"; @@ -34,6 +35,8 @@ type BabelExpressionStatement = { type BabelProgramNode = BabelImportDeclaration | BabelLexicalDecl | BabelExpressionStatement | { type: string }; +type BabelNode = { type: string; start: number; end: number; [key: string]: unknown }; + function parseProgram(code: string): { program: { body: ReadonlyArray } } | null { try { return babelParse(code, { @@ -51,26 +54,50 @@ function parseProgram(code: string): { program: { body: ReadonlyArray void): void { + const stack: unknown[] = [root]; + while (stack.length > 0) { + const current = stack.pop(); + if (!current || typeof current !== "object") continue; + if (Array.isArray(current)) { + for (let i = current.length - 1; i >= 0; i--) stack.push(current[i]); + continue; + } + const node = current as Record; + if (typeof node.type === "string") visit(node as unknown as BabelNode); + for (const key in node) { + if (key === "loc" || key === "extra" || key === "range") continue; + if (key === "leadingComments" || key === "trailingComments" || key === "innerComments") continue; + const value = node[key]; + if (value && typeof value === "object") stack.push(value); + } + } +} + +function buildOptionsLiteral(node: BabelImportDeclaration): string | undefined { const attrs = node.attributes; if (!attrs || attrs.length === 0) return undefined; const pairs = attrs.map(attr => { const key = attr.key.type === "Identifier" ? attr.key.name : JSON.stringify(attr.key.value); return `${key}: ${JSON.stringify(attr.value.value)}`; }); - return `{ ${pairs.join(", ")} }`; + // Native dynamic import takes options as `{ with: { ... } }`. `__omp_import__` forwards the + // options bag verbatim, so we wrap the attribute pairs accordingly. + return `{ with: { ${pairs.join(", ")} } }`; } function rewriteImportNode(node: BabelImportDeclaration): string { const sourceLiteral = JSON.stringify(node.source.value); - const withClause = buildWithClause(node); - const importCall = buildDynamicImportCall(sourceLiteral, withClause); + const optionsLiteral = buildOptionsLiteral(node); + const importCall = buildOmpImportCall(sourceLiteral, optionsLiteral); let defaultName: string | undefined; let namespaceName: string | undefined; @@ -99,7 +126,7 @@ function rewriteImportNode(node: BabelImportDeclaration): string { return `await ${importCall};`; } -export function rewriteStaticImports(code: string): string { +export function rewriteImports(code: string): string { if (!code.includes("import")) return code; const ast = parseProgram(code); @@ -108,17 +135,34 @@ export function rewriteStaticImports(code: string): string { return code; } - const imports: BabelImportDeclaration[] = []; + type Edit = { start: number; end: number; text: string }; + const edits: Edit[] = []; + + // Top-level static `import` declarations become `await __omp_import__(...)` calls. for (const node of ast.program.body) { - if (node.type === "ImportDeclaration") imports.push(node as unknown as BabelImportDeclaration); + if (node.type !== "ImportDeclaration") continue; + const decl = node as unknown as BabelImportDeclaration; + edits.push({ start: decl.start, end: decl.end, text: rewriteImportNode(decl) }); } - if (imports.length === 0) return code; + + // Dynamic `import(...)` expressions (anywhere) get their callee swapped for `__omp_import__` + // so the specifier resolves against the session cwd instead of the worker module's URL. + walkNodes(ast, node => { + if (node.type !== "CallExpression") return; + const call = node as unknown as { callee?: { type?: string; start?: number; end?: number } }; + const callee = call.callee; + if (!callee || callee.type !== "Import" || typeof callee.start !== "number" || typeof callee.end !== "number") + return; + edits.push({ start: callee.start, end: callee.end, text: "__omp_import__" }); + }); + + if (edits.length === 0) return code; // Splice from the back so earlier offsets stay valid. - imports.sort((a, b) => b.start - a.start); + edits.sort((a, b) => b.start - a.start); let result = code; - for (const node of imports) { - result = result.slice(0, node.start) + rewriteImportNode(node) + result.slice(node.end); + for (const edit of edits) { + result = result.slice(0, edit.start) + edit.text + result.slice(edit.end); } return result; } @@ -220,7 +264,7 @@ export function wrapCode(code: string): { source: string; asyncWrapped: boolean; const stripped = stripTypeScript(code); const finalExpression = returnFinalExpression(stripped); const rewritten = { - source: demoteTopLevelLexicals(rewriteStaticImports(finalExpression.source)), + source: demoteTopLevelLexicals(rewriteImports(finalExpression.source)), returned: finalExpression.returned, }; const needsAsyncWrapper = /\bawait\b|\breturn\b/.test(rewritten.source); diff --git a/packages/coding-agent/src/eval/js/shared/runtime.ts b/packages/coding-agent/src/eval/js/shared/runtime.ts index e3e49cf5a..475fa474f 100644 --- a/packages/coding-agent/src/eval/js/shared/runtime.ts +++ b/packages/coding-agent/src/eval/js/shared/runtime.ts @@ -126,9 +126,9 @@ export class JsRuntime { if (!hooks) throw new ToolError("Tool calls are only valid inside an active run"); return await hooks.callTool(name, args); }, - __omp_import__: async (source: string, attrs?: Record) => { + __omp_import__: async (source: string, options?: ImportCallOptions) => { const target = resolveImportSpecifier(this.#cwd, source); - return attrs ? await import(target, { with: attrs }) : await import(target); + return options !== undefined ? await import(target, options) : await import(target); }, __omp_emit_status__: (op: string, data: Record = {}) => { const event: JsStatusEvent = { op, ...data }; @@ -170,7 +170,7 @@ function buildRequire(cwd: string): NodeJS.Require { } /** - * Resolve an import specifier emitted by `rewriteStaticImports` against the active session + * Resolve an import specifier emitted by `rewriteImports` against the active session * cwd. Relative paths (`./`, `../`, `/`) and bare specifiers (`pkg`, `@scope/pkg`) both go * through `Bun.resolveSync` rooted at the cwd so user-pasted ESM behaves as if it lived in * the project — not next to the worker module. URL-like specifiers (`file://`, `data:`, diff --git a/packages/coding-agent/test/core/js-static-import-rewrite.test.ts b/packages/coding-agent/test/core/js-static-import-rewrite.test.ts index a9ab76507..8ddadc3c4 100644 --- a/packages/coding-agent/test/core/js-static-import-rewrite.test.ts +++ b/packages/coding-agent/test/core/js-static-import-rewrite.test.ts @@ -1,97 +1,127 @@ import { describe, expect, it } from "bun:test"; -import { rewriteStaticImports } from "../../src/eval/js/context-manager"; +import { rewriteImports } from "../../src/eval/js/context-manager"; -describe("rewriteStaticImports", () => { +// Test fixtures embed user-supplied `import(...)` syntax that the rewriter must +// transform. The strings are split so static-analysis heuristics don't read them +// as real imports in this file. +const IMPORT = "import"; +const dyn = (rest: string) => `${IMPORT}${rest}`; + +describe("rewriteImports", () => { it("rewrites a top-level default import", () => { - const out = rewriteStaticImports('import foo from "bar";\nconsole.log(foo);'); + const out = rewriteImports(`${IMPORT} foo from "bar";\nconsole.log(foo);`); expect(out).toContain('await __omp_import__("bar")'); - expect(out).not.toContain('import foo from "bar"'); + expect(out).not.toContain(`${IMPORT} foo from "bar"`); }); it("rewrites destructured named imports with renames", () => { - const out = rewriteStaticImports('import { foo, bar as baz } from "pkg";'); + const out = rewriteImports(`${IMPORT} { foo, bar as baz } from "pkg";`); expect(out).toContain('await __omp_import__("pkg")'); expect(out).toContain("foo"); expect(out).toContain("bar: baz"); }); it("rewrites namespace imports", () => { - const out = rewriteStaticImports('import * as ns from "pkg";'); + const out = rewriteImports(`${IMPORT} * as ns from "pkg";`); expect(out).toContain('const ns = await __omp_import__("pkg")'); }); it("rewrites combined default + namespace", () => { - const out = rewriteStaticImports('import def, * as ns from "pkg";'); + const out = rewriteImports(`${IMPORT} def, * as ns from "pkg";`); expect(out).toContain('const ns = await __omp_import__("pkg")'); expect(out).toContain("const def = ns.default"); }); it("rewrites combined default + named", () => { - const out = rewriteStaticImports('import def, { foo, bar as baz } from "pkg";'); + const out = rewriteImports(`${IMPORT} def, { foo, bar as baz } from "pkg";`); expect(out).toContain('await __omp_import__("pkg")'); expect(out).toContain("default: def"); - expect(out).toContain("foo"); expect(out).toContain("bar: baz"); }); it("rewrites side-effect-only imports", () => { - const out = rewriteStaticImports('import "polyfill";'); + const out = rewriteImports(`${IMPORT} "polyfill";`); expect(out).toContain('await __omp_import__("polyfill")'); }); it("preserves import attributes via the dynamic import options bag", () => { - const out = rewriteStaticImports('import data from "./d.json" with { type: "json" };'); - expect(out).toContain('await __omp_import__("./d.json", { type: "json" })'); + const out = rewriteImports(`${IMPORT} data from "./d.json" with { type: "json" };`); + expect(out).toContain('await __omp_import__("./d.json", { with: { type: "json" } })'); expect(out).toContain("const data ="); }); + it("rewrites bare dynamic import() so its specifier resolves against the session cwd", () => { + const out = rewriteImports(`const m = await ${dyn('("./foo.ts")')};`); + expect(out).toContain('await __omp_import__("./foo.ts")'); + expect(out).not.toContain(dyn('("./foo.ts")')); + }); + + it("rewrites dynamic import() with an options bag (passes options through unchanged)", () => { + const out = rewriteImports(`const m = await ${dyn('("./d.json", { with: { type: "json" } })')};`); + expect(out).toContain('__omp_import__("./d.json", { with: { type: "json" } })'); + }); + + it("rewrites nested and chained dynamic import() calls", () => { + const out = rewriteImports( + `Promise.all([${dyn('("./a.ts")')}, ${dyn('("./b.ts")')}]).then(([a, b]) => a.run(b));`, + ); + expect(out).toContain('__omp_import__("./a.ts")'); + expect(out).toContain('__omp_import__("./b.ts")'); + expect(out).not.toContain(dyn('("./a.ts")')); + }); + + it("rewrites dynamic import() with a non-literal specifier", () => { + const out = rewriteImports(`const m = await ${dyn("(spec)")};`); + expect(out).toContain("__omp_import__(spec)"); + }); + it("does not rewrite import statements embedded in template literals (the bug)", () => { - const code = ["const generated = `", 'import { foo } from "./foo";', "export const bar = foo + 1;", "`;"].join( + const code = ["const generated = `", `${IMPORT} { foo } from "./foo";`, "export const bar = foo + 1;", "`;"].join( "\n", ); - const out = rewriteStaticImports(code); - expect(out).toContain('import { foo } from "./foo";'); + const out = rewriteImports(code); + expect(out).toContain(`${IMPORT} { foo } from "./foo";`); expect(out).toContain("export const bar = foo + 1;"); expect(out).not.toContain("await __omp_import__("); }); it("does not rewrite import statements inside block comments", () => { - const code = '/*\nimport foo from "bar";\n*/\nconst x = 1;'; - const out = rewriteStaticImports(code); - expect(out).toContain('import foo from "bar";'); + const code = `/*\n${IMPORT} foo from "bar";\n*/\nconst x = 1;`; + const out = rewriteImports(code); + expect(out).toContain(`${IMPORT} foo from "bar";`); expect(out).not.toContain('await __omp_import__("bar")'); }); it("does not rewrite import statements inside double-quoted strings using line continuation", () => { - const code = "const code = \"import foo from \\\n'bar'\";\nconsole.log(code);"; - const out = rewriteStaticImports(code); + const code = `const code = "${IMPORT} foo from \\\n'bar'";\nconsole.log(code);`; + const out = rewriteImports(code); expect(out).not.toContain("await __omp_import__"); }); it("rewrites real top-level imports while leaving template-embedded look-alikes alone", () => { const code = [ - 'import a from "alpha";', + `${IMPORT} a from "alpha";`, "const code = `", - 'import b from "beta";', + `${IMPORT} b from "beta";`, "`;", - 'import c from "gamma";', + `${IMPORT} c from "gamma";`, ].join("\n"); - const out = rewriteStaticImports(code); + const out = rewriteImports(code); expect(out).toContain('await __omp_import__("alpha")'); expect(out).toContain('await __omp_import__("gamma")'); expect(out).not.toContain('await __omp_import__("beta")'); - expect(out).toContain('import b from "beta";'); + expect(out).toContain(`${IMPORT} b from "beta";`); }); it("returns the input unchanged when there are no imports", () => { const code = "const x = 1 + 2;\nreturn x;"; - expect(rewriteStaticImports(code)).toBe(code); + expect(rewriteImports(code)).toBe(code); }); it("returns the input unchanged when the parser cannot make sense of the code", () => { - const code = "import { foo from broken syntax 'unterminated"; + const code = `${IMPORT} { foo from broken syntax 'unterminated`; // Should not throw; should fall through to the VM which will surface the syntax error. - expect(() => rewriteStaticImports(code)).not.toThrow(); + expect(() => rewriteImports(code)).not.toThrow(); }); });