Skip to content

fix(api): load CommonJS route dependencies with native require in the compiled binary - #5484

Merged
kojiwakayama merged 2 commits into
mainfrom
fix/5471-native-cjs-require
Nov 7, 2026
Merged

kojiwakayama merged 2 commits into
mainfrom
fix/5471-native-cjs-require

Conversation

@kojiwakayama

Copy link
Copy Markdown
Contributor

Summary

In a compiled binary, API routes now load the project's CommonJS dependencies with the runtime's own createRequire. Two bugs on main go away:

  • __vf_interopDefault is not defined: the regex rewrite emitted calls to a helper the injected shim never exported.
  • require is not defined for import("pg"): dynamic imports of CommonJS packages loaded as ES modules.

Each CommonJS dependency a route imports (bare name or subpath) now bundles as a one-line module, module.exports = __vf_native_require("<spec>"). esbuild's interop then gives default, named, namespace and dynamic imports their usual meaning. The dependency's own files are not bundled or rewritten. ES module dependencies still load from file:// URLs. Node, Bun and the Deno source run keep their existing paths.

The compiled-binary require shim now resolves with createRequire and keeps route-level requires inside the project root by realpath. The regex CommonJS rewrite, __vf_interopDefault and toCjsDestructureBindings are removed.

This replaces #5477, which reimplemented Node's CommonJS loader. createRequire loads CommonJS from disk in Deno 2.9 deno compile binaries.

Closes #5471

Tests

  • loader.test.ts: a bundle built with createNativeCommonJSPlugin and the emitted shim runs default, named, namespace, subpath and dynamic imports of a CommonJS package, and the package source is not in the bundle.
  • loader.test.ts: the emitted shim loads a bare package and refuses real files outside the project (absolute path, prefix-sibling directory, .. escape, symlinked entry).
  • compiled-binary-e2e.suite.ts: an API route imports a pg-shaped CommonJS package (nested dependency, built-in events, instance default export, subpath, dynamic import). Passed against a locally built binary.
  • Real pg@8.13.1 checked by hand in a locally built binary: static, named, namespace and dynamic imports work, and a query to a closed port returns ECONNREFUSED.
  • Narrow suites: loader, external-import-rewriter, loader-helpers, import-rewriter.

… compiled binary

The compiled binary rewrote CommonJS dependency imports with regexes into
calls to __vf_interopDefault, a helper the injected shim never exported, and
loaded dynamic imports of CommonJS packages such as pg as ES modules. Both
failed at runtime.

Each CommonJS dependency a route imports now bundles as a one-line module
that loads it through the runtime's createRequire, so esbuild's interop gives
default, named, namespace and dynamic imports their usual meaning. ES module
dependencies keep loading from file:// URLs. The require shim resolves with
createRequire and keeps route-level requires inside the project root by
realpath. The regex rewrite and its helpers are removed.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-07T07:44:58.244784Z bbb0cf5 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

📦 Client bundle boundary

Entrypoint Modules Source size Server leaks
src/index.client.ts 66 366 KiB ✅ 0
src/react/components/Head.tsx 5 74 KiB ✅ 0
src/react/router/index.tsx 5 74 KiB ✅ 0
src/react/context/index.tsx 5 74 KiB ✅ 0
src/react/fonts/index.ts 6 81 KiB ✅ 0
src/react/components/ui/index.ts 111 717 KiB ✅ 0
src/react/components/ui/adapter/contract.ts 1 18 KiB ✅ 0
src/react/components/ui/icons/index.ts 3 17 KiB ✅ 0
src/chat/index.ts 305 2046 KiB ✅ 0
src/chat/ag-ui.ts 61 459 KiB ✅ 0
src/chat/protocol.ts 1 11 KiB ✅ 0
src/chat/types.ts 46 246 KiB ✅ 0
src/chat/message-prep.ts 68 517 KiB ✅ 0
src/markdown/index.ts 6 30 KiB ✅ 0
src/mdx/index.ts 2 2 KiB ✅ 0
src/agent/identity-contracts.ts 1 3 KiB ✅ 0
src/schemas/index.ts 43 220 KiB ✅ 0
src/platform/env.ts 5 53 KiB ✅ 0
src/workflow/claude-code/react/index.ts 9 91 KiB ✅ 0
src/events/index.ts 31 567 KiB ✅ 0
src/events/ag-ui/public.ts 30 561 KiB ✅ 0
src/run-events/index.ts 54 321 KiB ✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

Copy link
Copy Markdown
Contributor

Score: 82/100. This replaces the hand-rolled CJS loader in compiled binaries with the runtime's own createRequire, wrapped in a bundler plugin. The design is sound and the tests are good.

  • Strength: Deleting the roughly 100-line new Function CJS evaluator removes a lot of fragile code (the regex import rewriting and toCjsDestructureBindings). exports maps, #imports, nested node_modules, module.parent and Node error codes now come from the runtime, and esbuild handles default, named, namespace and dynamic interop.
  • Strength: createNativeCommonJSPlugin is small. It emits a one-line module.exports = __vf_native_require(...) module and never bundles the dependency's source. ESM dependencies keep their file:// path.
  • Strength: The VULN-FS-5 containment test is rewritten and strengthened. It uses real files outside the project, covering the prefix-sibling and .. cases, and it runs the emitted shim. A plugin test checks that the dependency's source is not bundled, and a compiled-binary e2e test is added.
  • Concern (security tradeoff): Containment now covers only the route's own requires and the top-level import of each CJS dependency. The new docstring says "requires a dependency makes itself are the runtime's own". A dependency can now require() a symlinked path that resolves outside the project without being blocked. The old shim checked those requires too. This weakens VULN-FS-5. Confirm it is intended, ideally with security sign-off and a test that documents the boundary.
  • Concern (minor): The plugin's onResolve filter /^[^./]/ also matches absolute paths and Windows drive paths. These fall through harmlessly because the package-name lookup misses them. A scoped-package test (@scope/pkg/sub) would lock in the name-extraction branch. The CJS/ESM split relies on resolveEsmUserDependencies. A mis-detected ESM-only package would now get require()d, so add a test for a mixed or exports-conditional package.
  • Minor: __vf_contained calls realpathSync on every require call, with no caching of the resolution. This is cheap, but it sits on route load paths.

No blockers. I'd like the containment-scope decision confirmed before merge. I did not run the test suite in this review.


Generated by Claude Code

@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 5fd40b376b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@codecov

codecov Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.87500% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/routing/api/module-loader/loader.ts 95.00% 0 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: bbb0cf506f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@kojiwakayama
kojiwakayama added this pull request to the merge queue Nov 7, 2026
@sonarqubecloud

sonarqubecloud Bot commented Nov 7, 2026

Copy link
Copy Markdown

Merged via the queue into main with commit d015fb6 Nov 7, 2026
438 checks passed
@kojiwakayama
kojiwakayama deleted the fix/5471-native-cjs-require branch November 7, 2026 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hosted runtime: CommonJS npm packages fail (__vf_interopDefault / require is not defined), app console output missing in logs

2 participants