Repository navigation
fix(builders): fail the build when the flow bundle still needs require() - #4268
Conversation
The workflow bundle is evaluated with vm.runInContext() in a sandbox that
only shims `module` and `exports` — there is no `require`. esbuild emits
that bundle as CJS, so an import left external becomes a top-level
`require("…")` and an unresolved dynamic `require()` is copied through
verbatim. Both throw `ReferenceError: require is not defined` when the
bundle loads, which takes down every workflow in the deployment.
createNodeModuleErrorPlugin() marks Node.js/Bun builtins external and only
reports the ones it can trace back to an `import … from "pkg"` statement in
user code, so builtins reached through a transitive dependency, a
re-export, or a side-effect-only import shipped a bundle that could never
run.
Add flow-bundle-safety.ts, which inspects the finished bundle:
- external imports come from the metafile, reported with the importing
module and the import chain back to user code;
- free `require` references are found with a lexer that blanks comments,
strings, template text and regexes, so esbuild's own `__require` helper,
`require_<name>()` module wrappers, `obj.require` and `typeof require`
probes are not flagged;
- candidates are then confirmed with esbuild's scope analysis (a `define`
pass only rewrites identifiers that resolve to the global scope), so a
shadowed `require` binding cannot fail a build. That pass only runs when
the cheap scan found something.
WORKFLOW_ALLOW_UNSAFE_FLOW_BUNDLE=1 downgrades the failure to a warning.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Peter Wielander <29887157+VaguelySerious@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 3db4259 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 E2E Test Results✅ All tests passed 🛠 Infra Events (absorbed by the harness)Platform anomalies the e2e harness detected and worked around (e.g. a run the queue never picked up, replaced by a fresh run). Clustered timestamps indicate a backend blip; a steady drip indicates a platform issue worth escalating.
E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 🌐 Cross-language Conformance
✅ dynamic-runs
✅ vercel-http-transport
✅ vercel-multi-region
✅ vercel-ws-transport
|
📊 Workflow Benchmarkscommit Backend:
Streams
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 131526ms → this run 179185ms (Δ +47659ms, +36%) 📈 CRTT drill-down vs main (RTT distributions & profiles)RTT over stream progress (avg per tenth of stream, bars scaled min→max): RTT by chunk size (avg per log size bin, ~160B → ~12KB serialized, bars scaled min→max): Delivery jitter over stream progress (avg positive CDV per tenth of stream, bars scaled min→max): ℹ️ Metric definitions & methodologyStreams: first-chunk RTT (the stream-open path, before any buffering/backpressure), CRTT percentiles, and worst delivery stall (CDV max). Cells are medians across iterations; per-run values in the artifacts. No 🔴/🟢 marks until targets attach. The collapsed STSO distribution section above buckets every step gap, split inline (same warm process — pure framework overhead) vs queue-hop (fresh process — dispatch, reinit, replay). The collapsed CRTT drill-down: per-variant RTT histograms (fixed log bins, Best/P75/P90/P99 deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body (in-deployment start() → first step body) · Fan-out TTFS: fan-out time to first step (in-deployment start() → first of the parallel step bodies to complete) · Fan-out TTLS: fan-out time to last step (in-deployment start() → last of the parallel step bodies to complete, i.e. when the Promise.all resolves) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · CRTT: chunk round-trip time (per-chunk write → read latency, one clock domain: deployment → stream backend → same deployment) · CDV: chunk delay variation / delivery jitter (inter-arrival gap minus inter-write gap per seq-adjacent pair; skew-free; the row is each run's MAX positive value, so one stall moves it) Scenarios — step: one trivial no-op step, no stream; no hooks, so the run stays in turbo mode (in-process fast path) · stream: one streaming step; no hooks, so the run stays in turbo mode (in-process fast path) · hook + stream: registers a hook before one step, which exits turbo mode (dispatch path) · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges, and WO is the whole-run overhead outside step bodies · Promise.all(100 steps): 100 trivial no-op steps started together in a single Promise.all; Fan-out TTFS is the first of them to complete and Fan-out TTLS the last, both from the in-deployment clientStart, so their gap is the spread the runtime adds across the fan-out · paced control (100/s, 60B): the control: 300 tiny (~60B) deltas metronome-paced at 100/s — zero workload structure, so it reads the transport floor and flush cadence, and disambiguates transport-wide vs workload-specific when a replay row moves · size sweep (100/s, 160B-12KB): same pacing as the control with deltas padded in rotation across seven log-spaced sizes (~160B–12KB) — rotation decouples size from stream position, so it isolates whether chunk size causes latency · replay gateway-gpt-5.4-nano-2000t (1x): raw provider SSE cadence captured at the AI gateway boundary (gpt-5.4-nano, the most popular gateway model; per-token deltas p50 208B = the modal production chunk size), replayed exactly as measured — the typical customer's workload; its CDV is the typical customer's real delivery jitter · replay eve-gpt-5.6-sol-2000t (1x): a captured eve turn (gpt-5.6-sol, the most-used demanding eve model; ~2000 output tokens = production p50 turn length) replayed exactly as measured — eve's envelope protocol re-ships the cumulative message so sizes ramp 142B→13KB; the demanding outlier tenant's reality · replay eve-gpt-5.6-sol-2000t (2x): the same eve capture at 2x — the headroom/stress row; real fast-tier models emit the same chunk sizes at proportionally higher rate, so time compression is a faithful speed model · first chunk (pooled): every run's seq-0 RTT pooled across all stream scenarios — the first chunk precedes any workload differentiation, so pooling samples one shared stream-open path with exact percentiles Replay cadences (semantic sha256) — eve-gpt-5.6-sol-2000t 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 All timestamps are deployment-side; runs are triggered in-deployment, so the CI runner and api.vercel.com sit outside every measured window. TTFS = Cold starts stay in the numbers (real bursty-workload latency, inflates P75+); Best is the warm floor. |
Sim WorldSimulated world deterministic testing for races. Traces 🟠 world-sim scenario book — 1 fail of 42 total
Full trace: |
About these numbersSizes are gzip; parentheses show the change against
|
The Next.js dev e2e suite caught a false positive: adding a workflow to
`app/page.tsx` pulls the page's component tree into the flow bundle, and
framer-motion probes for its optional `@emotion/is-prop-valid` dependency
with a `require()` it *expects* to fail:
try {
loadExternalIsValidProp(require("@emotion/is-prop-valid").default);
} catch {
// fallback
}
The sandbox's `ReferenceError` lands in that `catch`, so the bundle loads
fine — but the check failed the rebuild, and every Next dev HMR job with
it.
Track `try { … }` bodies while lexing and skip `require` references inside
them. This covers both shapes the probe takes: a free `require(pkgVar)`
(framer-motion 12.43, reported as a dynamic require) and a literal
`require("pkg")` that esbuild externalizes *because* it is wrapped in
try/catch (12.29, reported as an external import) — esbuild's own
"Could not resolve" hint suggests that wrapping, so the metafile entry
describes code designed to fail. A specifier is only excused when every
one of its call sites is guarded.
The scope-analysis probe applies the same exclusion so a guarded require
cannot confirm an unrelated candidate.
Verified by re-running `packages/core/e2e/dev.test.ts` against the
nextjs-turbopack dev server: fails identically to CI with the guard
removed, passes with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Peter Wielander <29887157+VaguelySerious@users.noreply.github.com>
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
Agent review findings.
| */ | ||
| function isRequireCheckCondition(condition: string): boolean { | ||
| return ( | ||
| typeofDefinedTest('require').test(condition) && !/\|\||\?\?/.test(condition) |
There was a problem hiding this comment.
Agent Find: This treats any condition containing a positive-looking typeof require comparison as a guard, without preserving its polarity or excluding matches inside non-code text. For example, if (!(typeof require !== "undefined")) { require("node:fs"); } executes the branch and throws in the sandbox, yet the scanner reports no violation. A regex or string containing the same text can also mark the block guarded. Please parse the condition structurally, or otherwise prove the positive check dominates the branch, before suppressing its require sites.
There was a problem hiding this comment.
AI Review: Note
Fixed in 31e4dd4. Guards are now decided on an acorn AST instead of text. The sandbox never defines require, so typeof require is always "undefined" there. The walker partially evaluates each if / ?: test under that assumption (handling !, string comparisons, &&/||, sequences) and only excuses the branch that provably does not run. if (!(typeof require !== "undefined")) { require(…) } now evaluates to true, so its consequent is reported. Look-alike text inside a regex or string is a literal node, so it can never produce a guard. New tests: "follows the polarity of a typeof require check" and "does not take a guard from typeof checks inside strings or regexes". Both fail on the previous commit.
| .slice(lineStart + test.index + test[0].length, index) | ||
| .trimStart(); | ||
| if (between.startsWith('?')) between = between.slice(1); | ||
| if (!/[|?:;,]/.test(between)) return true; |
There was a problem hiding this comment.
Agent Find: The punctuation denylist is not equivalent to short-circuit control flow. (typeof require !== "undefined") & require("node:fs") evaluates the RHS but is treated as guarded because & is allowed, while typeof require !== "undefined" && (cached || require("pkg")) is safe but rejected because | appears later. This needs structural analysis of the expression path instead of scanning the intervening characters.
There was a problem hiding this comment.
AI Review: Note
Fixed in 31e4dd4. The punctuation scan is gone. Only the right operand of a LogicalExpression counts as guarded: under && when the left side is provably false in the sandbox, under || when it is provably true. &, | and ?? never guard. So (typeof require !== "undefined") & require("node:fs") is now reported, and typeof require !== "undefined" && (cached || require("pkg")) passes (the outer && guards the whole right subtree). Covered by "only excuses operators that short-circuit" plus the ok5 case in the polarity test.
Cost: the parse only runs when the bundle has a bare require token and at least one evaluated reference. Measured ~140ms/MB for parse plus walk on a 2.5MB file, compared with ~600ms/MB for the esbuild confirmation pass that already runs in that case. If the parse fails, nothing is treated as guarded (the conservative direction), which matches how a failed esbuild probe is handled.
typeof require checks and try/catch blocks are now read from an acorn AST. A check only excuses the branch it provably skips in the sandbox (where typeof require is always "undefined"), so negated checks, non-short-circuit operators, and look-alike text in strings or regexes no longer suppress a require() site. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
Both Agent Find issues are addressed. Guard analysis now preserves polarity, ignores non-code look-alikes, and follows actual short-circuit control flow; the focused builders tests and typecheck pass.
|
No backport to This adds a new build-time validation gate ( To override, re-run the Backport to stable workflow manually via |
Restore the `if (location)` gate. A violation the plugin cannot locate in user code is caught by assertFlowBundleIsSandboxSafe (#4268), which names the builtin and the import chain, excuses `typeof require` guards, and honors WORKFLOW_ALLOW_UNSAFE_FLOW_BUNDLE. Reporting it from the plugin pre-empted all three and broke four flow-bundle-safety tests. The multi-line import fix stays: those imports now get a source location. Signed-off-by: Joel Fickson <joel.fickson@vercel.com>
Closes #4903. `getViolationLocation` tested its import regex one line at a time, so an import split across lines never matched. Biome and Prettier split any import of three or more names. When the lookup fails, the plugin hands the violation to the flow bundle backstop (`assertFlowBundleIsSandboxSafe`, #4268) instead of reporting it. This PR matches the import against the whole file and derives the line from the match offset. Multi-line imports now get the plugin's error, which points at the first use of the import in user code. The specifier pattern is `[^;'"]+?`, not `[\s\S]+?`, so a match cannot start at an earlier import statement and run on into this one. Signed-off-by: Joel Fickson <joel.fickson@vercel.com> Signed-off-by: Peter Wielander <mittgfu@gmail.com> Co-authored-by: Peter Wielander <peter.wielander@vercel.com> Co-authored-by: Peter Wielander <mittgfu@gmail.com>
Closes #4903. `getViolationLocation` tested its import regex one line at a time, so an import split across lines never matched. Biome and Prettier split any import of three or more names. When the lookup fails, the plugin hands the violation to the flow bundle backstop (`assertFlowBundleIsSandboxSafe`, #4268) instead of reporting it. This PR matches the import against the whole file and derives the line from the match offset. Multi-line imports now get the plugin's error, which points at the first use of the import in user code. The specifier pattern is `[^;'"]+?`, not `[\s\S]+?`, so a match cannot start at an earlier import statement and run on into this one. Signed-off-by: Joel Fickson <joel.fickson@vercel.com> Signed-off-by: Peter Wielander <mittgfu@gmail.com> Co-authored-by: Peter Wielander <peter.wielander@vercel.com> Co-authored-by: Peter Wielander <mittgfu@gmail.com> Signed-off-by: Joel Fickson <Joelngozo@gmail.com>
Closes #1161
The workflow ("flow") bundle is evaluated with
vm.runInContext()in a sandbox that only shimsmoduleandexports— there is norequire. esbuild emits that bundle as CJS, so:require("…")hoisted to the top of the bundle, andrequire(…)esbuild could not statically resolve is copied through verbatim.Both throw
ReferenceError: require is not defined. The external case is the worst: it fires while the bundle's top level is still evaluating, so every workflow in the deployment fails to load, not just the code path that wanted the module.This was silent at build time.
createNodeModuleErrorPlugin()marks Node.js/Bun builtinsexternal: trueand only reports the ones it can trace back to animport … from "pkg"statement in user code (node-module-esbuild-plugin.ts) — everything else was externalized without a word.Reproduced before this change: a workflow importing
wrapper-pkg, which requiresleaky-pkg, which requiresnode:fs, builds successfully and emitsWhat this adds
packages/builders/src/flow-bundle-safety.ts, run on the finished interim bundle insidecreateWorkflowsBundle()(and on Next.js dev hot rebuilds, which drive the esbuild context directly and would otherwise only catch it after a restart).External imports — from the metafile.
outputs[*].importswithexternal: trueis authoritative for what esbuild actually emitted;inputs[*].importsgives the importing module, and a BFS over the input graph reconstructs the chain back to user code.Free
require()— text scan + esbuild scope analysis. A lexer blanks comments, strings, template text (substitutions stay code) and regex literals, then looks for therequireidentifier. Confirmation runsesbuild.transform(bundle, { define: { require: <sentinel> } }):defineonly rewrites identifiers that resolve to the global scope, so the bundler's own scope analysis has the final say. That pass only runs when the cheap scan already found a candidate (~600 ms per MB otherwise), so clean builds pay nothing.try { require(…) } catchis excused. Guards are read from an acorn AST of the bundle (parsed only when a candidate exists). Arequire()inside one is how packages probe for an optional dependency — the sandbox'sReferenceErrorlands in thecatchand the bundle still loads. This applies to externals too: esbuild only externalizes an unresolvablerequire()when it is wrapped that way (its "Could not resolve" hint literally suggests doing so), so the metafile can report an import for code designed to fail. A specifier is excused only when every one of its call sites is guarded.Diagnostics:
Compatibility: what this can newly break, and the guard for each
Every case below already produced a bundle that crashes at runtime — this turns a production
ReferenceErrorinto a build error. The risk is mis-detection, so each is either narrowed or has an escape hatch.your-code → pkg-a → pkg-b → node:fsnode_modulespathexport * from 'pkg',import 'pkg'getViolationLocation()'simport … fromregex misses, so they used to ship brokenrequire()in workflow codedefinepass confirms the identifier is globalrequire()inside a dependency (plugin loaders, optional deps)WORKFLOW_ALLOW_UNSAFE_FLOW_BUNDLE=1if the call is genuinely unreachabletypeof requireUMD/CJS probes — extremely common in bundled packagestypeofon an undeclared name cannot throw. Arequirebehind anif/?:/&&/||is excused only when the test is provably false (or true for||andelse) giventypeof require === "undefined", evaluated on the AST, so negated checks and non-short-circuit operators are still reported__require(the__commonJSshim),require_<name>()module wrappers_is a word character), verified against real esbuild output in testsfunction (require, exports) { … }UMD factoriesrequire2, and thedefinepass independently confirms the reference is not globalrequireas a property —obj.require(…),{ require: … }requireinside strings / comments / regexes / template texttry { require('opt') } catch {}— optional-dependency probe (framer-motion ships one); theReferenceErroris caughtTryStatementnodes with ahandlerin the AST, and cover both shapes: a freerequire(pkgVar)and a literalrequire("pkg")that esbuild externalized because of the try/catchtryblock (try { exports.load = n => require(n) } catch {}) — protected lexically but not at call timerequire("node:fs")as both external and dynamicEscape hatch:
WORKFLOW_ALLOW_UNSAFE_FLOW_BUNDLE=1turns the failure into a warning (the bundle still fails at runtime if the code is reached). Documented on thenode-js-module-in-workflowerror page.Tests
flow-bundle-safety.test.ts(19) — lexer offset/newline preservation, division vs. regex, template substitutions, banner comments; helper-wrapper andtypeofexclusions; metafile chain building and<runtime>filtering; external/dynamic de-duplication; shadowed-binding suppression; throw vs. warn.flow-bundle-safety.build.test.ts(6) — realBaseBuilderbuilds against esbuild's actual output: a clean CJS dependency still builds; transitive builtin, re-export, user dynamicrequire, and dependency dynamicrequireall fail with the expected diagnostics. The escape-hatch test also evaluates the produced bundle in a barevmcontext and asserts theReferenceErrorit would have shipped.Validation
pnpm --filter @workflow/builders test— 276 passed.turbo test --continue— 51/51 tasks (@workflow/swc-pluginskipped: nocargoin this environment).pnpm build,pnpm --filter @workflow/builders typecheck,pnpm lint(no new errors; 2 pre-existing-style complexity warnings on the lexer).example,nextjs-turbopack,nextjs-webpack,sveltekit,nuxt,nitro,nitro-v2,nitro-v3,astro,vite,hono,express,fastify,nest,tanstack-startall build clean.packages/core/e2e/dev.test.tsagainst thenextjs-turbopackdev server — 7 passed, 2 skipped. The first push failed the 8 Next dev HMR jobs here: adding a workflow toapp/page.tsxdrags the page's component tree (and framer-motion's optional-dependency probe) into the flow bundle. Reproduced locally, fixed, and confirmed by a negative control — with the try-block guard removed the suite fails identically to CI, with it the suite passes.Out of scope: the Vitest issue.
🤖 Generated with Claude Code