Skip to content

Make route CommonJS require work after bundling and name unsupported packages - #5472

Closed
kojiwakayama wants to merge 22 commits into
mainfrom
fix/5471-cjs-interop-route-shim
Closed

kojiwakayama wants to merge 22 commits into
mainfrom
fix/5471-cjs-interop-route-shim

Conversation

@kojiwakayama

@kojiwakayama kojiwakayama commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Refs #5471

Change

Route require prologue (loader.ts)

The route runtime shim used to inject require into the esbuild bundle. esbuild reserves top-level require in ESM output, so it renamed the injected binding and its __require helper never saw it. It also tree-shook __vf_interopDefault, because nothing referenced it until the compiled binary rewrote user dependency imports after bundling. As a result:

  • Compiled binary: import pg from "pg" failed with ReferenceError: __vf_interopDefault is not defined. Named, namespace and dynamic imports failed with ReferenceError: require is not defined.
  • Every runtime: bundled project CommonJS that calls require("node:path") failed with Dynamic require of "node:path" is not supported.

The changes:

  • generateRouteRuntimeShim(routeSourceUrl) now injects only the Worker shim.
  • prependRouteRequirePrologue adds generateRouteRequirePrologue(projectDir, compiledBinary, helperPrefix) to the bundled, rewritten handler. On the compiled binary that is the compiled CJS loader; on other runtimes, a createRequire binding.
  • A leading hashbang stays on the first line.
  • chooseRouteHelperPrefix(bundle) picks the first helper prefix (__vf_, __vf1_, ...) none of whose helper names appears anywhere in the bundle. The same prefix goes to the import rewrite, the temporaries and both prologues, so route code can never collide with them.

Compiled CJS loader (external-import-rewriter.ts)

  • The loader's state is kept in a private function scope. Only require, the interop and namespace helpers and the node: import aliases are module-level, under the chosen prefix.
  • The namespace helper takes the specifier and caches one namespace object per resolved module, so repeated imports share it and distinct modules never do.
  • Bare ids resolve from the requiring module's directory, so a package's own non-hoisted node_modules wins over the root copy. Canonicalization and containment checks are unchanged.
  • On MODULE_NOT_FOUND, the error keeps code: "MODULE_NOT_FOUND" and reports either npm package "<name>" is not installed in this runtime's node_modules or npm package "<name>" is installed but "<id>" could not be resolved, without host paths.
  • A resolved .node file throws native addon "<id>" cannot be loaded: native addons are not supported in this runtime.
  • A leading #! in a CommonJS source is commented out before evaluation.
  • Relative and absolute ids resolve locally in require and require.resolve, probing Node's extensions, then a directory's package.json main, then its index. Containment is checked before probing, and a missing module throws Cannot find module "<id>" with code: "MODULE_NOT_FOUND".
  • A blocked path is reported by the requested id, never the host path.
  • CommonJS code runs with module.exports as top-level this. The cache holds module records, so a circular require sees a reassigned module.exports, and a module whose evaluation throws is removed from it.
  • Local probing includes .node, so an extensionless native addon gets the native-addon error. A malformed local package.json is an error, not a fallback to index.
  • Requiring an .mjs file from a CommonJS package fails with ES module file "<id>" cannot be loaded with require.
  • CommonJS code gets module.require, id, filename, path and loaded.
  • Path ids are classified with path.isAbsolute, so Windows drive-letter and UNC ids are paths.
  • Node's prefix-only built-in names (test, sqlite, sea) are not treated as built-ins; such a package resolves from node_modules.
  • Resolver errors other than MODULE_NOT_FOUND keep their code but not their host paths.
  • Every built-in in the runtime's builtinModules loads as a built-in.
  • Helper renaming runs before the project path is embedded, so a path containing a helper name is unchanged.

Compiled import rewrite (route-adapter.ts)

  • The rewrite works on import statements found by the module lexer, so import-like text in strings, template literals and comments is left alone. One clause parser rewrites every static import form (default, named, namespace and combinations) for the package root and subpaths. Side-effect imports become require(...).
  • import * as ns and dynamic import() produce an ES module namespace via __vf_namespace, with default set to the whole module.exports (also for __esModule modules).
  • Dynamic imports are rewritten to Promise.resolve().then(...), so failures reject the promise.
  • A default import of a subpath goes through __vf_interopDefault.
  • Bindings may contain $, Unicode letters and \u escapes. String-literal named imports (import { "a-b" as value }) become quoted destructuring keys. import { default as x } gets the same interop as import x.
  • Imports with attributes (with { type: "json" }) and dynamic imports with an options argument are rewritten.
  • Named and namespace re-exports become local bindings plus an export list; export * from a CommonJS package fails with a clear error.
  • Specifiers embedded in generated code are escaped for <, >, U+2028 and U+2029.

Docs

New "npm packages" section in docs/guides/api-routes.md: supported import forms and namespace shape, the node_modules requirement, exports-map limits, .mjs files inside CommonJS packages, re-exports, the errors, and when static versus dynamic imports fail.

Tests

route CommonJS require (#5471) in loader.test.ts:

  • Compiled pipeline: bundles with the real bundler and the route runtime shim, applies the compiled rewrite and prependRouteRequirePrologue, imports the handler and calls GET. Cases:
    • default import
    • default plus named import
    • dynamic import()
    • a $ binding
    • namespace and dynamic .default on a module.exports = function package
    • a nested non-hoisted dependency
    • a CommonJS entry with a hashbang
    • a route with a hashbang
    • a route declaring the default helper and temporary names, and one that uses a name in a string and a property
    • import-like text inside a template literal
    • one namespace object per module across static and dynamic imports, and distinct namespaces for distinct modules with equal exports
    • a string-literal named import, a non-ASCII binding, imports with attributes, named and namespace re-exports, and export * (rejected)
    • an __esModule package through default, namespace and dynamic imports
    • a default-plus-named subpath import and a side-effect import
    • a package that probes an optional dependency through MODULE_NOT_FOUND
  • Real loader: loadHandlerModule on the bundle path, where a local .cjs calls require("node:path").
  • Emitted loader, run directly: a package that is not installed, an installed package with a missing subpath, a native addon, local require.resolve of an existing and a missing relative id, a directory with a package.json main, a module that throws on first evaluation, top-level this, an async_hooks require, a blocked relative path, a circular require with reassigned exports, a malformed local manifest, an extensionless native addon, an .mjs file in a CommonJS package, module.require, a package named test, and a malformed installed manifest.
  • Prefix choice and literals: chooseRouteHelperPrefix, the renamed shim, a project path containing a helper name, and literal escaping in external-import-rewriter.test.ts; string-literal destructuring in loader-helpers.test.ts.

Every new test was confirmed to fail with its fix reverted. Existing assertions on the compiled rewrite text were updated for the namespace and lazy dynamic forms.

Ran locally:

  • tests: src/routing/api/module-loader/, src/transforms/import-rewriter/, src/routing/api/handler.test.ts
  • checks: deno check on changed files, deno task lint, deno fmt --check, lint:anti-slop, lint:sanitizer-baseline
  • docs: docs:public:check, guide validation

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

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

Copy link
Copy Markdown

Codex Review Summary

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

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-06T14:43:50.611106Z a118734 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 5, 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 2045 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.

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown

@codex review

Copy link
Copy Markdown
Contributor

Score: 84/100. Fixes the compiled-binary __vf_interopDefault / require ReferenceErrors by prepending the CJS loader after bundling, and adds regression tests that exercise the real bundle-then-rewrite pipeline.

  • Strength: The root cause is diagnosed correctly. The loader must be added after bundling, because esbuild never sees the post-bundle rewritten references and so renames require and tree-shakes the helper.
  • Strength: The change is small and well contained. generateRouteRuntimeShim is extracted as a pure function, the non-compiled shim output is unchanged, and rewriteExternalImports has a single non-test caller (loader.ts:2228), so the loader is not prepended twice.
  • Strength: The three tests (default, default plus named, dynamic import) evaluate the handler against a real CJS fixture. The PR states they fail without the fix.
  • Concern (minor): rewriteCompiledBinaryRouteModule prepends the full loader unconditionally, even when the route has no user CJS deps. A cheap guard, such as prepending only when the rewrite changed the code or userDeps is non-empty, would avoid the extra startup work and the Deno.realPathSync call per route. This is optional, but check that nothing else in the bundle relies on require being present.
  • Concern (minor): The "esbuild reserves top-level require" no-collision claim is only covered indirectly. A test where the route itself declares a top-level require or uses a bundled CJS dependency (esbuild's __require fallback) would lock in that assumption. Also confirm the prepended import statements stay valid if the bundle starts with a directive or a sourcemap comment.

Mergeable_state is "blocked", so CI and review are still pending. I did not run the tests locally.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 81992113ae

ℹ️ 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".

esbuild reserves top-level require in ESM output, so the injected require
never reached its __require helper on any runtime. Bundled project CommonJS
could not require Node built-ins outside the compiled binary either.

Refs #5471
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 67372a62bf

ℹ️ 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".

The resolver error carried the host project path, and a .node addon was
evaluated as JavaScript. Document supported npm package patterns for API
routes.

Refs #5471
@kojiwakayama kojiwakayama changed the title Define the compiled-binary CJS loader for rewritten route dependency imports Make route CommonJS require work after bundling and name unsupported packages Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown

@codex review

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ffb8fc06db

ℹ️ 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".

Comment thread docs/guides/api-routes.md Resolved
Comment thread docs/guides/api-routes.md Resolved
Comment thread docs/guides/api-routes.md Resolved

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ffb8fc06db

ℹ️ 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".

Comment thread docs/guides/api-routes.md Resolved
Comment thread src/routing/api/module-loader/loader.ts Resolved
Comment thread src/routing/api/module-loader/loader.ts Resolved
Comment thread src/routing/api/module-loader/loader.ts Resolved
Comment thread src/routing/api/module-loader/loader.ts Resolved
@codecov

codecov Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

…terop

- Keep compiled loader state in a private scope and fail the build when route
  code declares a remaining prologue binding.
- Keep a bundled route hashbang on the first line; strip CJS hashbangs.
- Resolve bare requires from the requiring module's directory.
- Report an installed package with an unresolvable entry or subpath as such.
- Give namespace and dynamic imports an ES module namespace with default.
- Accept $ in rewritten import bindings.
- Document lazy dynamic-import failures and exports-map limits.

Refs #5471
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown

@codex review

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 003421fed0

ℹ️ 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".

…combined clauses

- Reject only declarations of prologue names, not strings or properties.
- Namespace default is the whole module.exports, __esModule included.
- Keep code MODULE_NOT_FOUND on the sanitized resolution errors.
- Rewrite every static import clause form, root or subpath, and
  side-effect imports.

Refs #5471
@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 15f4a64362

ℹ️ 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".

Comment thread src/routing/api/module-loader/loader.ts Resolved
…free helpers

- Rewrite compiled user dependency imports from parsed import statements,
  so import-like text in strings, templates and comments is untouched.
- Prefix prologue helpers and temporaries with a prefix absent from the
  bundle, replacing the declaration check (and its regex escaping).
- Cache one namespace object per CommonJS module.
- Resolve relative and absolute require.resolve ids locally, checking
  containment before probing and naming a missing module by its id.

Refs #5471
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c7aaeeac5

ℹ️ 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".

Comment thread docs/guides/api-routes.md Resolved
…re.cache/resolve.paths, node: built-in check, captured decoder intrinsics (#5471)
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

@codex review

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3077b9343b

ℹ️ 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".

…its, canonical resolve, prototype-free tables (#5471)
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

@codex review

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 53da924716

ℹ️ 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".

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

@codex review

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e224578e45

ℹ️ 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".

… prototypes (#5471)

Capture String, Function and URL prototype methods as uncurried functions,
drop regex and Symbol-dispatching string methods, grow arrays by index, and
resolve bare packages and module-relative file URLs without the runtime's
node:module and node:url helpers, which call replaceable prototype methods.
@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 76f26858ed

ℹ️ 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

Copy link
Copy Markdown
Contributor Author

@codex review

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3346cd70c0

ℹ️ 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".

Comment on lines +475 to +478
const attributes = staticImportAttributes(masked, imported.a, imported.se);
const check = attributes
? `${helperPrefix}validateImportOptions({ with: ${attributes} }, ${specifier}); `
: "";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Require JSON attributes for JSON package imports

When a compiled route statically imports a CommonJS package's JSON subpath without attributes, this conditional emits no validation and require() loads the JSON successfully; the dynamic path similarly skips validation when no options are supplied. Native ESM rejects both import data from "pkg/data.json" and import("pkg/data.json") unless type: "json" is present, so handlers can succeed only in the compiled binary instead of taking their expected failure path. Fresh evidence beyond the earlier module-type mismatch fix is the reverse mismatch: JSON targets are never required to carry the JSON attribute. Validate every rewritten import against its resolved target, including the no-options case.

AGENTS.md reference: AGENTS.md:L23-L25

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fixing this one. I tried it, and it breaks valid routes. esbuild drops with { type: "json" } from external imports before this rewrite runs. So a route that correctly writes import data from "pkg/data.json" with { type: "json" } reaches the rewriter without the attribute, and refusing it there fails that route. The existing test "rewrites static and dynamic package imports with attributes" failed when I enforced it. The rewrite cannot tell an omitted attribute from a dropped one, so it accepts a JSON module without one. A comment in a118734 explains this in both the validator and the rewrite. The opposite case, type: "json" on a non-JSON module, is still refused. Leaving this thread open for the owner's decision.

Posted by Claude Code (AI agent) on behalf of the PR author.

…kup to the project, absolute resolve ids, expression braces (#5471)
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

@codex review

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a118734177

ℹ️ 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".

// "do" and "else" start a statement; the other listed words take an
// expression.
previous = word === "do" || word === "else" ? ";" : __vf_regexAfterWord[word] === true ? "(" : "word";
controlWord = word === "if" || word === "while" || word === "for" || word === "with";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the control context across for await

When a CommonJS dependency contains a valid loop such as for await (const value of values) /import(".\\/child.mjs")/.test(value), reading the intermediate await word clears controlWord, so the closing ) is treated as a value and the following slash as division. The scanner then rewrites the import(...) text inside the regex literal, corrupting the dependency. The earlier control-flow fix therefore still misses the distinct for await path; retain the for condition context through await.

AGENTS.md reference: AGENTS.md:L26-L28

Useful? React with 👍 / 👎.

// the requiring module: the nearest package.json at or above its directory,
// inside the project.
function __vf_resolvePackageImport(id, parentDir) {
for (var dir = __vf_canonicalize(parentDir); ; dir = __vf_dirname(dir)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor node_modules boundaries in ancestor walks

When a manifest-less dependency under node_modules calls require("#x"), this loop crosses the node_modules boundary and incorrectly applies the project's package.json#imports; Node stops package-scope lookup at that boundary. The sibling __vf_resolveBare walk has the inverse form of the same defect: from /project/node_modules/bar it probes /project/node_modules/node_modules/foo before /project/node_modules/foo, so an otherwise ignored directory can shadow the real package. Make both ancestor walks node_modules-aware, stopping package-scope lookup and skipping duplicate node_modules candidates. Fresh evidence beyond the earlier package-import and bounded-lookup fixes is the manifest-less dependency path and the doubled node_modules candidate.

AGENTS.md reference: AGENTS.md:L23-L25

Useful? React with 👍 / 👎.

throw unknown;
}
if (__vf_startsWith(id, "#")) return __vf_resolvePackageImport(id, parentDir);
var name = __vf_packageName(id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve package self-references before nested node_modules

When code inside package foo calls require("foo/sub") and foo/node_modules/foo also exists, this bare-package path searches the nested copy first and loads its subpath. Node resolves a package self-reference through the containing package's own name and exports before searching node_modules, so both require and require.resolve return the outer package in this scenario. Resolve the nearest containing package's self-reference before starting the node_modules walk to avoid loading the wrong package version.

Useful? React with 👍 / 👎.

@kojiwakayama
kojiwakayama marked this pull request as draft November 6, 2026 14:50
@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Paused for a redesign and set to draft. Over 14 Codex review rounds this PR drew about 60 findings, and each round still finds new edge cases in the hand-written CommonJS loader. The replacement design is tracked in veryfront/veryfront-issue-inbox#4177. The tests in route CommonJS require (#5471) (in loader.test.ts) will serve as the acceptance suite for whichever design wins. One finding is still open: the JSON-attribute thread, declined because esbuild drops import attributes. Do not merge until that redesign decision is made.

Posted by Claude Code (AI agent) on behalf of the PR author.

@sonarqubecloud

sonarqubecloud Bot commented Nov 6, 2026

Copy link
Copy Markdown

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Closing unmerged in favour of #5477, which bundles CommonJS dependencies of compiled-binary API routes with esbuild instead of loading them through a hand-written runtime loader (decision in veryfront/veryfront-issue-inbox#4177). The branch stays for reference.

Posted by Claude Code (AI agent) on behalf of the PR author.

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.

3 participants