Repository navigation
Conversation
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 2 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Updated 6:54 AM PT - Aug 22nd, 2026
✅ @robobun, your commit e236c3a9cd1e8e818132814070dc28965a0521f3 passed in 🧪 To try this PR locally: bunx bun-pr 40072That installs a local version of the PR into your bun-40072 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused polyfill bugfixes with a lint guard to catch the class going forward.
What was reviewed:
util.js: each fix (kCustomPromisifiedSymboldeclaration,.apply(this, args),queueMicrotask,isBufferduck-typing,debuglogpid) checked against the upstream npmutilbrowser build; all match.build-fallbacks.ts: confirmed the removed locals (mod/file/externals/i/text) were dead — the live external list isexternalModules, computed separately.oxlint.jsonoverride:processas a declared global is needed because the polyfills guard it withtypeof;stream.jsis the only CJS file in the directory.- New test exercises success and error paths for each fixed export, and
itBundledruns the browser bundle (not bun's nativenode:util), so the polyfill code is what executes.
Extended reasoning...
Overview
Fixes five broken exports in the node:util browser polyfill (src/node-fallbacks/util.js) that bun build --target=browser inlines into user bundles: an undeclared kCustomPromisifiedSymbol that made promisify() throw on every call, two fn.apply(this, ...args) sites that spread instead of passing the array, callbackify() calling process.nextTick (not a browser global), isBuffer() doing instanceof Buffer (same), and debuglog() referencing an undeclared pid. Adds an oxlint no-undef override under the browser env for src/node-fallbacks/*.js and wires the directory into bun run lint, so the bug class fails CI. Deletes dead locals from build-fallbacks.ts that the new lint surfaced. Adds browser/NodeUtilPolyfill in test/bundler/bundler_browser.test.ts.
Security risks
None. This is a browser-side polyfill — plain JS embedded into user bundles, never loaded by the Bun runtime. No auth, crypto, filesystem, or network. The isBuffer duck-typing loosens a type check, but that mirrors the upstream util package's browser build exactly and isBuffer is advisory, not a security gate.
Level of scrutiny
Low-to-medium. The polyfill is not on any Bun runtime hot path; it only affects --target=browser bundles. Each change is a small, mechanical correction whose before/after is obvious from the diff (undeclared → declared, spread-into-apply → array-into-apply, node-only global → web-standard equivalent). The lint addition is additive tooling and the PR verified it is clean on main after the fixes.
Other factors
- The test covers each fixed export with both success and error paths (promisify custom symbol, callbackify falsy rejection wrapping, deprecate warn-once with
thisbinding, isBuffer negatives).itBundledruns the bundled output in a subprocess, soconsole.errormutation in the fixture stays isolated. - Checked that hoisting
kCustomPromisifiedSymbolto a top-levelconstmatches the file's existing pattern (formatRegExp,monthsare already un-annotated top-level consts), so tree-shaking behavior is unchanged relative to the rest of the file. queueMicrotaskpreserves the property the comment names: a throw inside the callback surfaces as an uncaught error, not a rejection of the settled promise — same asprocess.nextTick.- The
build-fallbacks.tsdeletions are verifiably dead: the live external list is built frombuiltins.concat(moduleFiles.filter(...))intoexternalModules; the removedexternals/mod/file/iblock computed the same thing and discarded it. - No prior reviewer comments to address; no CODEOWNER on these paths.
…clared names The browser polyfill for node:util was converted by hand from the npm util package. Several exports did not survive the conversion: - promisify() read kCustomPromisifiedSymbol, which was never declared, so every call threw ReferenceError. - callbackify() spread the callback arguments into Function#apply, so the callback never received the result. It also called process.nextTick, and browsers have no process global. - deprecate() spread its arguments into Function#apply the same way. - isBuffer() used instanceof against a Buffer global that browsers do not have. It now duck-types the buffer polyfill like upstream does. - debuglog() read an undeclared pid in its enabled branch. Lint src/node-fallbacks/*.js with no-undef under the browser environment so an undeclared name (a JSC intrinsic, a dropped declaration) fails CI. Remove the dead locals in build-fallbacks.ts that the lint flagged.
cf6e979 to
f332214
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the prior review's coverage gap (no guard on the queueMicrotask change) is now closed by the onAfterBundle assertion in 594d8a2.
What was reviewed:
- Each
util.jsfix traced to its bug: undeclaredkCustomPromisifiedSymbol/pid/Buffer, spread-into-.apply,process.nextTickin a browser bundle.isBufferduck-typing matches upstream browserifyutil. - Confirmed the
build-fallbacks.tsdeletions were dead locals — the first loop'smod/filethat populatemoduleFilesremain. - oxlint override: whitelisting
processis deliberate (guarded reads elsewhere); theonAfterBundlestring check compensates for the one unguarded case that would slip through.
Extended reasoning...
Overview
Fixes five conversion bugs in the node:util browser polyfill (src/node-fallbacks/util.js) that made promisify, callbackify, deprecate, isBuffer, and debuglog throw or misbehave in a browser bundle. Adds an oxlint no-undef override for src/node-fallbacks/*.js under the browser env so undeclared names fail CI, wires src/node-fallbacks into the lint script, deletes dead locals from build-fallbacks.ts that the new lint scope surfaced, and adds a browser/NodeUtilPolyfill bundler test exercising every fixed export.
Security risks
None. This is plain JS shipped into the user's browser bundle; it touches no auth, crypto, filesystem, or network code. The fixes are strictly correctness (declaring a symbol, passing an array to Function#apply, swapping process.nextTick for queueMicrotask, duck-typing Buffer).
Level of scrutiny
Low-to-medium. The polyfills are not part of the Bun runtime — they are inlined only into --target=browser bundles — so blast radius is limited to browser output that already threw ReferenceError on these paths. Every change is a small, mechanical correction with an obvious before/after (.apply(this, ...args) → .apply(this, args)), and each is covered by a specific assertion in the new test plus (for undeclared names) the new lint rule.
Other factors
All prior feedback on the PR is resolved: the two comment-cop warnings about long comments were shortened in 8a81084, and my earlier inline nit about the missing process.nextTick regression guard was addressed in 594d8a2 with an onAfterBundle check mirroring the neighboring NodeEventsOnce test. Hoisting kCustomPromisifiedSymbol to a module-level const moves one Symbol.for call outside the /* @__PURE__ */ IIFE, but that is a negligible one-time evaluation and necessary for the function body to reference it. The build-fallbacks.ts cleanup is safe — the removed block recomputed values already produced by the earlier loop and never read them. No design decisions or API surface changes here; approving.
…ss, guard path.resolve import util from "node:util" gave an object with 8 of the 28 exports, so util.format and util.inspect were undefined in browser bundles. The default export now lists every named export. deprecate() and debuglog() read globalThis.process instead of the bare process global, and path.resolve() falls back to "/" when there is no process.cwd(). With no bare process left in the hand-written polyfills, the lint no longer declares process as a global, so a new unguarded use is a no-undef error. The browser bundle test now deletes process, Buffer and setImmediate before the polyfills run, exits on an unhandled rejection, and checks that a throw inside a callbackify callback is an uncaught exception. This replaces the string check for process.nextTick.
There was a problem hiding this comment.
LGTM — the follow-up commit (aa455a2) resolved my prior note in a stronger way than I suggested: dropping the process lint whitelist and reading globalThis.process everywhere means a bare process.nextTick now fails no-undef, and the test's delete globalThis.process catches a globalThis.process.nextTick regression at runtime.
What was reviewed:
util.js:kCustomPromisifiedSymboldeclared,.apply(this, args)(not spread),queueMicrotaskfor callbackify,isBufferduck-typing matches upstream browserify util,debuglogno longer builds a bogus regex whenNODE_DEBUGis unset, default export now covers every named export.build-fallbacks.ts: only dead locals removed;externalModules(the live one) is untouched andglobalThis.processis not caught by theglobal.processstring replace.- Confirmed no bare
processremains in anysrc/node-fallbacks/*.js, so the newno-undefoverride is clean without a globals whitelist.
Extended reasoning...
Overview
Fixes the node:util browser polyfill (src/node-fallbacks/util.js) — a hand-conversion of the npm util package that had dropped the kCustomPromisifiedSymbol declaration, spread arrays into Function#apply, referenced bare process/Buffer/pid, and shipped a default export missing 20 of its 28 members. Also guards path.resolve() against a missing process.cwd(), adds an oxlint override that runs no-undef over src/node-fallbacks/*.js under the browser environment, wires src/node-fallbacks into bun run lint, deletes dead locals from build-fallbacks.ts, and adds a comprehensive browser/NodeUtilPolyfill bundler test.
Security risks
None. These files are plain-JS polyfills inlined into user bundles by bun build --target=browser; the runtime never loads them. No auth, crypto, or filesystem paths touched. The isBuffer duck-typing matches upstream browserify util and is not used for any trust decision.
Level of scrutiny
Medium-low. The blast radius is limited to browser-target bundler output — a path that was already broken (promisify threw a ReferenceError on every call). Each fix is mechanical and traces directly to the upstream npm util package the file was ported from. The lint addition is additive (a new override) and I grepped every *.js in src/node-fallbacks/ to confirm no bare process remains, so CI won't go red on unrelated files.
Other factors
All prior feedback is resolved. The comment-cop notes were fixed in 8a81084. My previous review asked for a regression guard on the queueMicrotask clause; the author first added the onAfterBundle string check I suggested (594d8a2), then in aa455a2 replaced it with a superior structural fix — removing the process global whitelist so lint flags any bare process, and having the test delete globalThis.process/Buffer/setImmediate before exercising the polyfill. That covers both the bare-process and globalThis.process regression shapes. The new test asserts exact stdout for promisify (both paths + promisify.custom), callbackify (resolve/reject/falsy-reject/uncaught-in-callback), deprecate (with and without process), isBuffer, debuglog, path.resolve, and default-export completeness. The build-fallbacks.ts diff is pure dead-code removal; the live externalModules computation is unchanged.
There was a problem hiding this comment.
LGTM — the earlier feedback about guarding the queueMicrotask clause is addressed: the test now deletes process/Buffer/setImmediate before exercising the polyfills and exits on unhandled rejection, so a process.nextTick revert fails at runtime.
What was reviewed:
util.js: declaredkCustomPromisifiedSymbol, fixed.apply(this, ...args)→.apply(this, args)indeprecate/callbackify,queueMicrotaskincallbackify, duck-typedisBuffer,debuglogno longer referencespidor builds a/^false$/regex, and the default export now mirrors every named export.path.jsresolve()falls back to/whenglobalThis.processis absent; grepped the other polyfill sources and no bareprocess/Bufferreferences remain, so the newno-undefbrowser-env override passes without aglobalsallowlist.build-fallbacks.tsdeletions are provably-dead locals; the--define=process.env.NODE_DEBUGflag still applies to vendored deps and is left in place.browser/NodePolyfillssnapshot is unaffected (only the default export changed, not named exports).
Extended reasoning...
Overview
Fixes multiple bugs in the node:util browser polyfill (src/node-fallbacks/util.js) introduced when it was hand-converted from the npm util package to an ES module: the undeclared kCustomPromisifiedSymbol, fn.apply(this, ...args) spreads that should have been fn.apply(this, args), unguarded process.nextTick/process/Buffer/pid references, and an incomplete default export. Also guards path.resolve() for the no-process.cwd case, adds an oxlint no-undef override for src/node-fallbacks/*.js under the browser environment, extends the lint script to cover that directory, removes dead locals from build-fallbacks.ts, and adds browser/NodeUtilPolyfill in test/bundler/bundler_browser.test.ts.
Security risks
None. These are browser polyfills bundled into user output for --target=browser; they do not touch Bun's runtime node:util, native code, auth, crypto, or filesystem paths. The oxlint override is additive and scoped to one directory.
Level of scrutiny
Low-to-medium. The polyfills are plain JS derived from a well-known npm package, and every change is either a mechanical correctness fix (declare a const, pass an array not a spread, use a browser-available scheduler) or a defensive guard (globalThis.process?. instead of bare process). The blast radius is limited to bun build --target=browser output that imports node:util/node:path, which was already broken (promisify threw a ReferenceError on every call). No design decisions here — the reference is the upstream util package's browser build, which the PR follows for isBuffer duck-typing and callbackify semantics.
Other factors
- Prior feedback addressed. The comment-cop bot's two long-comment flags were resolved (comments shortened in 8a81084). My earlier inline comment about the
queueMicrotaskclause lacking a regression guard was resolved twice: first with anonAfterBundlestring check (594d8a2), then replaced with a stronger runtime guard (aa455a2) — the test deletesprocess,Buffer, andsetImmediatebefore calling into the polyfills and installs anunhandledRejectionhandler that exits nonzero, so a bareprocess.nextTickincallbackifynow ReferenceErrors instead of silently passing under bun. Theglobals: { process }allowlist that motivated the original comment was also dropped from the oxlint override in the same commit. - Test coverage. The new test exercises every fixed export (promisify success/error/custom, callbackify success/rejection/falsy-rejection/uncaught-throw, deprecate with and without
process, isBuffer positive/negative, debuglog, path.resolve fallback, default-vs-namespace parity). PR evidence shows it fails on main and passes on the PR in both ASAN-debug and release. The existingbrowser/NodePolyfillssnapshot still passes because only the default export grew, not the named-export set. - Lint scope. I grepped
src/node-fallbacks/*.jsfor bareprocess— only comments andprocess.js(the shim itself) remain, so the newno-undefoverride passes without whitelisting node globals.stream.jsgets a separatecommonjsenv override because it is built as CJS. - Dead-code removal. The four locals removed from
build-fallbacks.ts(mod,file,externals,i) and the unusedtextparameter were computed and never read; removing them is required for the file to lint clean now thatsrc/node-fallbacksis on the lint path.
|
Closing in favor of #41533. It contains the |
1 participant
Problem
util.promisify()from abun build --target=browserbundle throwsReferenceError: kCustomPromisifiedSymbol is not defined(src/node-fallbacks/util.js:582). The polyfill is a hand conversion of the npmutilpackage and the declaration was dropped.callbackify()anddeprecate()spread their arguments intoFunction#apply, so the callback gets no result anddeprecate(fn)(1, 2)throws.callbackify()usesprocess.nextTickandisBuffer()usesinstanceof Buffer. Browsers have neither. The default export lists 8 of 28 exports, soutil.formatisundefined.events.js. Its self-review found this file.Fix
util.js: declare the symbol, pass the arrays toapply, usequeueMicrotask, duck-type the buffer polyfill inisBuffer()as upstreamutildoes in its browser build, readglobalThis.process, and complete the default export.path.resolve()falls back to/withoutprocess.cwd().src/node-fallbacks/*.jswithno-undefunder the browser environment (oxlint.jsonoverride,lintscript). A node global or a JSC intrinsic in a polyfill source now fails CI. It flags the pre-node-fallbacks: build the once() promise without $newPromiseCapability #40049events.js.queueMicrotaskkeeps the node property that matters: a throw in the callback is an uncaught error, not a rejection.test/bundler/bundler_browser.test.ts(newbrowser/NodeUtilPolyfill, stock bun fails it). The test deletesprocess,BufferandsetImmediatebefore the bundle runs.Background
src/node-fallbacks/*.jsare browser polyfills fornode:*modules.bun build --target=browserinlines them into the user's bundle.promisify.customisSymbol.for("nodejs.util.promisify.custom"). A function can carry its own promisified form under it.Notes
Repro with the released binary. Bundle this with
bun build --target=browser, then run the output withnodeafterdelete globalThis.process; delete globalThis.Bufferto match a browser:With the fix the same file prints
42,null,42,3,true,function.The test runs the bundle under bun, which has every node global. It saves
process, deletesprocess,BufferandsetImmediatefromglobalThis, and exits on an unhandled rejection. With the debug build's polyfill output edited by hand, aprocess.nextTickrevert fails withunhandledRejection: process is not defined, and a synchronouscb(null, ret)fails withunhandledRejection: in callbackwhere the test expectsuncaughtException: in callback.The lint covers the hand-written sources only. The vendored packages that
assert.js,http.js,stream.jsandzlib.jsbundle still referenceprocessinternally. #35484 covers theprocessshim for the stream polyfill.Lint findings on main before this PR, with the new override and no
processglobal:util.jskCustomPromisifiedSymbol(6),pid(1),Buffer(1),process(5),path.jsprocess(1). Nothing else in the 20 polyfill sources. With the pre-#40049events.jsswapped in, the lint also reports$newPromiseCapability.debuglog()readsglobalThis.process?.env?.NODE_DEBUGat run time now. Before,build-fallbacks.tsdefinedprocess.env.NODE_DEBUGasfalseat build time, which also made the match regex/^false$/.build-fallbacks.tsis linted with the base rules now. Its twono-unused-varshits were dead locals (mod,file,externals,i, and the unusedtextparameter). Removed. Its output checks (string matches for a few known bad names) stay as they are.Suites run:
test/bundler/bundler_browser.test.ts(24 pass, 1 skip, 1 todo).[decide:dep] gate passed · iteration 1 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file