Skip to content

fix(deploy): externalize bare Node builtins + add createRequire banner for CJS compat - #242

Merged
khaliqgant merged 2 commits into
mainfrom
fix/deploy-cjs-builtins-create-require
Jun 17, 2026
Merged

fix(deploy): externalize bare Node builtins + add createRequire banner for CJS compat#242
khaliqgant merged 2 commits into
mainfrom
fix/deploy-cjs-builtins-create-require

Conversation

@khaliqgant

@khaliqgant khaliqgant commented Jun 17, 2026

Copy link
Copy Markdown
Member

Summary

  • externalize bare Node.js builtins alongside node:* for deploy agent bundles
  • add a createRequire(import.meta.url) banner so bundled transitive CommonJS deps can resolve external builtins from ESM
  • add a regression test that imports a generated agent.bundle.mjs containing a transitive CJS require("process")

Validation

  • pnpm --filter @agentworkforce/persona-kit build && pnpm --filter @agentworkforce/deploy test

This fixes the startup crash seen in cloud deployments: Dynamic require of "process" is not supported.

Review in cubic

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request updates the bundle stager to leave bare Node builtins external for transitive CommonJS dependencies, adding a banner to define require in the ESM bundle and introducing a test to verify this behavior. Feedback on these changes suggests defining require conditionally on globalThis to prevent potential syntax errors from duplicate declarations, and simplifying the NODE_EXTERNALS array by removing redundant node: prefix mappings covered by the wildcard pattern.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +66 to +71
banner: {
js: [
'import { createRequire as __agentworkforceCreateRequire } from "node:module";',
'const require = __agentworkforceCreateRequire(import.meta.url);'
].join('\n')
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Declaring const require in the global/module scope via the banner can cause a syntax error (Identifier 'require' has already been declared) if the user's code or any bundled dependency already defines a top-level require variable (which is a common pattern when using createRequire in ESM).

Since esbuild prepends the banner as a raw string after bundling, it cannot rename conflicting variables in the bundled code to avoid this collision.

To prevent this, we can safely define require on globalThis if it is not already defined. This avoids any duplicate declaration errors while still making require available to esbuild's dynamic require helpers and bundled CommonJS dependencies.

Suggested change
banner: {
js: [
'import { createRequire as __agentworkforceCreateRequire } from "node:module";',
'const require = __agentworkforceCreateRequire(import.meta.url);'
].join('\n')
},
banner: {
js: [
'import { createRequire as __agentworkforceCreateRequire } from "node:module";',
'if (typeof globalThis.require === "undefined") {',
' globalThis.require = __agentworkforceCreateRequire(import.meta.url);',
'}'
].join('\n')
},

Comment on lines +14 to +18
const NODE_EXTERNALS = [
...builtinModules,
...builtinModules.map((name) => `node:${name}`),
'node:*'
];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The ...builtinModules.map((name) => \node:${name}`)entry is redundant because'node:*'is a wildcard pattern that already matches all imports prefixed withnode:`. We can simplify this array to keep it clean and maintainable.

const NODE_EXTERNALS = [
  ...builtinModules,
  'node:*'
];

…ed forms

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@khaliqgant

Copy link
Copy Markdown
Member Author

Bot review comment audit (checked at HEAD 9515b1b)

Comment 1 (high — globalThis.require suggestion): Dismissed. The globalThis.require approach won't work because esbuild's __require shim does typeof require !== "undefined" in module scope, not typeof globalThis.require. Setting globalThis.require leaves the module-scope require undefined, so the shim still throws. The collision risk (duplicate const require) doesn't apply — all bundled code lives inside __commonJS function closures, never at the module top level alongside the banner.

Comment 2 (medium — redundant node:* entries): Valid — applied in 9515b1b. Removed the redundant ...builtinModules.map(name => \node:${name}`)sincenode:*` already covers all prefixed forms. 206/206 tests pass.

@khaliqgant
khaliqgant merged commit b65f245 into main Jun 17, 2026
2 checks passed
@khaliqgant
khaliqgant deleted the fix/deploy-cjs-builtins-create-require branch June 17, 2026 09:30
@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@khaliqgant, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 27 seconds. Learn how PR review limits work.

To continue reviewing without waiting, enable usage-based billing in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 09c28357-083f-4eb4-bbe0-a35d5c1447b9

📥 Commits

Reviewing files that changed from the base of the PR and between cfbf1b4 and 9515b1b.

📒 Files selected for processing (2)
  • packages/deploy/src/bundle.test.ts
  • packages/deploy/src/bundle.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/deploy-cjs-builtins-create-require

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

1 participant