node: expose internal/* under --expose-internals, implement internal/errors helpers [1v1cy7]#29823
node: expose internal/* under --expose-internals, implement internal/errors helpers [1v1cy7]#29823robobun wants to merge 10 commits into
internal/* under --expose-internals, implement internal/errors helpers [1v1cy7]#29823Conversation
|
Updated 12:19 PM PT - May 4th, 2026
❌ @robobun, your commit e05878a has some failures in 🧪 To try this PR locally: bunx bun-pr 29823That installs a local version of the PR into your bun-29823 --bun |
Status14/18 tests from node-compat group
Deferred (separate Bun gaps, not
Regression test: CI (#48546): 280 passed / 6 failedNone of this PR's tests appear in any failure. The 6 failed lanes are:
All 25 flagged tests ( |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a compile-time map and runtime fast-path to expose Node-style Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 0/5 reviews remaining, refill in 2 minutes and 25 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/internal/error_serdes.ts`:
- Around line 137-139: The custom-inspect branch should only run when the hook
is callable: change the guard in the block that returns
Buffer.from(StringFromCharCode(kCustomInspectedObject) + inspect(error), "utf8")
so it requires $isCallable(error[customInspectSymbol]) in addition to error !=
null and customInspectSymbol in error; i.e., check that the property exists and
is callable (using the JSC intrinsic $isCallable) before invoking inspect and
emitting the kCustomInspectedObject marker.
- Around line 181-184: The kInspectedSymbol branch uses buf.length (UTF-8 byte
length) to slice the JS string, causing broken non-ASCII symbols; change to
convert buffer to string first and use the string's length for slicing. In the
kInspectedSymbol case, call fromBuffer(error) -> const buf = fromBuffer(error);
const s = buf.toString("utf8"); then use StringPrototypeSubstring.$call(s,
kSymbolStringLength, s.length - 1) (and pass that result into SymbolFor) instead
of using buf.length - 1 so non-ASCII symbol descriptions deserialize correctly.
In `@src/js/internal/errors.ts`:
- Around line 71-76: The code temporarily mutates Error.stackTraceLimit (in the
block guarded by isErrorStackTraceLimitWritable()) but doesn't ensure
restoration on exception; wrap the mutation and creation of the AggregateError
(the code that sets limit, constructs err via new AggregateError(new
SafeArrayIterator([outerError, innerError]), outerError.message), calls
ErrorCaptureStackTrace(err, aggregateTwoErrors), etc.) in a try/finally so
Error.stackTraceLimit is always restored to its previous value; apply the same
try/finally pattern to the other similar blocks that mutate
Error.stackTraceLimit (the blocks that also use AggregateError,
SafeArrayIterator and ErrorCaptureStackTrace) to avoid leaking process-wide
state.
- Line 339: Replace the lint-blocking short-circuit expressions with explicit
conditional statements: instead of relying on the side-effect expression
"Error.stackTraceLimit && ErrorCaptureStackTrace(error, wrappedFn)" use a clear
if (Error.stackTraceLimit) { ErrorCaptureStackTrace(error, wrappedFn); } (and do
the analogous change at the other occurrence around line that uses the same
pattern). This targets the invocation using Error.stackTraceLimit and
ErrorCaptureStackTrace with the variables error and wrappedFn so the linter
no-unused-expressions rule is satisfied.
In `@src/js/internal/test/binding.ts`:
- Around line 6-8: internalBinding currently forwards any input to
process.binding without validation; add argument validation using the internal
validators (import from internal/validators) to ensure name is a non-empty
string and throw the appropriate Node error code (e.g. $ERR_INVALID_ARG_TYPE)
for invalid types/values before calling process.binding; update the function
internalBinding to call the validator (e.g., validateString or
validators.string/validateString with the argument name 'name') and only invoke
process.binding(name) after validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e3fcaf8-592b-4d5b-bb06-996a5d844eb0
📒 Files selected for processing (20)
src/bun.js/ModuleLoader.zigsrc/bun.js/VirtualMachine.zigsrc/bun.js/bindings/BunProcess.cppsrc/js/internal/error_serdes.tssrc/js/internal/errors.tssrc/js/internal/test/binding.tstest/js/node/test/parallel/test-dns-memory-error.jstest/js/node/test/parallel/test-error-aggregateTwoErrors.jstest/js/node/test/parallel/test-error-format-list.jstest/js/node/test/parallel/test-error-value-type-detection.mjstest/js/node/test/parallel/test-errors-aborterror.jstest/js/node/test/parallel/test-errors-systemerror-frozen-intrinsics.jstest/js/node/test/parallel/test-errors-systemerror-stackTraceLimit-custom-setter.jstest/js/node/test/parallel/test-errors-systemerror-stackTraceLimit-deleted-and-Error-sealed.jstest/js/node/test/parallel/test-errors-systemerror-stackTraceLimit-deleted.jstest/js/node/test/parallel/test-errors-systemerror-stackTraceLimit-has-only-a-getter.jstest/js/node/test/parallel/test-errors-systemerror-stackTraceLimit-not-writable.jstest/js/node/test/parallel/test-errors-systemerror.jstest/js/node/test/parallel/test-internal-error-original-names.jstest/js/node/test/parallel/test-uv-unmapped-exception.js
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/js/internal/error_serdes.ts (2)
141-143:⚠️ Potential issue | 🟡 MinorGate custom-inspect serialization on a callable hook.
Line 141 currently branches on property presence only. If
error[customInspectSymbol]is non-callable, this path should not be used.Suggested fix
- if (error != null && customInspectSymbol in error) { - return Buffer.from(StringFromCharCode(kCustomInspectedObject) + inspect(error), "utf8"); - } + if (error != null) { + const customInspect = error[customInspectSymbol]; + if ($isCallable(customInspect)) { + return Buffer.from(StringFromCharCode(kCustomInspectedObject) + inspect(error), "utf8"); + } + }As per coding guidelines, "Use JSC intrinsics (prefixed with
$) such as$Array.from(),$isCallable(), and$newArrayWithSize()for performance-critical operations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/internal/error_serdes.ts` around lines 141 - 143, The branch using customInspectSymbol should only run when error[customInspectSymbol] is callable; change the condition in the block that currently checks "error != null && customInspectSymbol in error" to also verify callability (e.g., using $isCallable(error[customInspectSymbol])). Keep the current behavior of producing Buffer.from(StringFromCharCode(kCustomInspectedObject) + inspect(error), "utf8") and ensure you use the suggested JSC intrinsics (e.g., $isCallable and other $-prefixed intrinsics where appropriate) when validating the hook before calling inspect.
186-188:⚠️ Potential issue | 🟠 MajorSlice inspected Symbol text using string length, not buffer byte length.
Line 187 uses
buf.length - 1while slicing a JS string.buf.lengthis UTF-8 bytes, so non-ASCII symbol descriptions deserialize incorrectly.Suggested fix
case kInspectedSymbol: { - const buf = fromBuffer(error); - return SymbolFor(StringPrototypeSubstring.$call(buf.toString("utf8"), kSymbolStringLength, buf.length - 1)); + const inspected = fromBuffer(error).toString("utf8"); + return SymbolFor( + StringPrototypeSubstring.$call(inspected, kSymbolStringLength, inspected.length - 1), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/internal/error_serdes.ts` around lines 186 - 188, The code is slicing the JS string using buf.length (UTF-8 byte count) which corrupts non-ASCII symbol text; change to decode the buffer first (use fromBuffer -> buf.toString("utf8") into a local str), compute the end index from str.length (e.g. const end = str.length - 1), and call StringPrototypeSubstring.$call(str, kSymbolStringLength, end) before passing to SymbolFor so substring uses character length not byte length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/js/internal/error_serdes.ts`:
- Around line 141-143: The branch using customInspectSymbol should only run when
error[customInspectSymbol] is callable; change the condition in the block that
currently checks "error != null && customInspectSymbol in error" to also verify
callability (e.g., using $isCallable(error[customInspectSymbol])). Keep the
current behavior of producing
Buffer.from(StringFromCharCode(kCustomInspectedObject) + inspect(error), "utf8")
and ensure you use the suggested JSC intrinsics (e.g., $isCallable and other
$-prefixed intrinsics where appropriate) when validating the hook before calling
inspect.
- Around line 186-188: The code is slicing the JS string using buf.length (UTF-8
byte count) which corrupts non-ASCII symbol text; change to decode the buffer
first (use fromBuffer -> buf.toString("utf8") into a local str), compute the end
index from str.length (e.g. const end = str.length - 1), and call
StringPrototypeSubstring.$call(str, kSymbolStringLength, end) before passing to
SymbolFor so substring uses character length not byte length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: de8b4d23-6ff1-490c-ac9a-4ac0cf884045
📒 Files selected for processing (2)
src/js/internal/error_serdes.tstest/js/node/module/expose-internals.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/internal/errors.ts`:
- Around line 644-650: Replace direct array method calls on the local variables
with the tamper-proof prototype wrappers: instead of types.indexOf("object") and
types.splice(pos, 1) use ArrayPrototypeIndexOf.$call(types, "object") and
ArrayPrototypeSplice.$call(types, pos, 1) respectively; also ensure the file
declares the prototype references (e.g., ArrayPrototypeIndexOf and
ArrayPrototypeSplice) alongside the other prototype bindings at the top so the
new .$call usages match the project's tamper-proof pattern and reference the
same symbols as other functions in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16149c19-a995-4473-adf5-57df6cbb0307
📒 Files selected for processing (1)
src/js/internal/errors.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for fixing the ASAN stderr assertion. I didn't find anything else blocking, but given this touches the Zig module loader/resolver and adds ~700 lines of internal/errors infrastructure (including a behavior change to aggregateTwoErrors used by existing builtins), it's worth a human pass.
Extended reasoning...
Overview
This PR makes internal/* specifiers resolvable from userland when is_allowed_to_use_internal_testing_apis is set (or in debug builds), so that Node.js --expose-internals tests can run. It does this via a comptime-derived ExposedInternals string map in ModuleLoader.zig plus fast-path checks in two resolve paths in VirtualMachine.zig. On the JS side it expands src/js/internal/errors.ts from a single aggregateTwoErrors export to a ~700-line port of Node's lib/internal/errors.js (E/codes/SystemError/hideStackFrames/UVException/DNSException/determineSpecificType/formatList/etc.), adds a verbatim port of internal/error_serdes, and a thin internal/test/binding shim. Fourteen upstream Node parallel tests plus one Bun regression test are added.
Security risks
The new internal/* resolution surface is gated behind the same flag as bun:internal-for-testing in release builds, so it doesn't expose internals to ordinary userland. The Environment.isDebug or ... disjunction means debug builds now always resolve every internal:* registry entry from userland — that's a deliberate widening but worth a maintainer's eye since it exposes the entire internal module registry, not just internal/errors. No injection, auth, or data-exposure vectors otherwise; the JS additions are pure error-formatting/serialization helpers.
Level of scrutiny
Moderate-to-high. The Zig changes sit on the module resolution hot path (the gate check runs on every internal/-prefixed specifier), and src/js/internal/errors.ts is loaded by other builtins — notably aggregateTwoErrors now mutates Error.stackTraceLimit and re-captures the caller's stack, which is a behavior change for every existing internal caller (e.g. streams). The error_serdes and test/binding modules are inert unless explicitly required under the flag.
Other factors
All CodeRabbit threads are resolved (mostly upstream-parity rationale). My one prior finding (empty-stderr assertions failing on the x64-asan lane) was fixed in e3eada9. CI on the latest build shows only fleet-wide flakes per the status comment. The PR is well-scoped and well-tested, but at ~1500 added lines spanning the loader, resolver, and a core internal module, it exceeds the threshold for bot auto-approval.
…internal/errors
Node tests flagged '// Flags: --expose-internals' do
require('internal/errors') etc. Bun's src/js/internal/* modules were
only reachable from other builtins via the build-time require
transformer — userland require('internal/errors') failed with 'Cannot
find module'.
- ModuleLoader: derive a comptime map of all internal:* ResolvedSource
tags and, when is_allowed_to_use_internal_testing_apis is set (same
gate as bun:internal-for-testing; always on in debug), resolve and
fetch 'internal/<name>' from the InternalModuleRegistry.
- VirtualMachine: let those specifiers through both resolve paths.
- internal/errors.ts: implement the surface area these tests use —
E/codes, SystemError, getMessage, hideStackFrames, AbortError,
determineSpecificType, formatList, DNSException, UVException[WithHostPort],
kIsNodeError, isErrorStackTraceLimitWritable, aggregateTwoErrors
(now captures the caller's stack like Node).
- Add internal/test/binding (thin process.binding proxy) and
internal/error_serdes.
- BunProcess: fix typo v8_enable_i8n_support -> v8_enable_i18n_support
so common.hasIntl is truthy.
Checks in 14 Node parallel tests from the internal/errors group.
Verifies require/import of internal/errors (and siblings) resolves under the testing flag and that the new helper surface is present; also covers the v8_enable_i18n_support typo fix in process.config.
oxlint no-unused-expressions flagged two '&&'-as-if patterns ported from upstream Node; rewrite as explicit 'if' statements.
Correcting the key name flips common.hasIntl from false to true, which
un-skips 11 pre-existing node parallel tests (test-icu-*, test-url-*,
test-whatwg-*) that each fail for unrelated reasons (missing
process.binding('icu'), different error message wording, URL formatting
differences). Fixing those is out of scope for the internal/errors work;
the typo fix belongs in its own PR where each un-skipped test can be
addressed.
Also drops the corresponding assertion from expose-internals.test.ts.
On the x64-asan lane every JS-executing bunExe() subprocess writes 'WARNING: ASAN interferes with JSC signal handlers...' to stderr, which bunEnv does not suppress. The asan lane died with exit -1 on the last CI run so this never surfaced. Switch to a combined toEqual that keeps stderr in the failure diff (for diagnostics) but accepts any string; stdout == 'ok' + exitCode == 0 remain the actual regression guard.
Jarred-Sumner
force-pushed
the
farm/6e2211c1/expose-internal-errors
branch
from
e3eada9 to
53774a4
Compare
resolveMaybeNeedsTrailingSlash's contract is that callers deref res.result.value on success in addition to deref'ing their own borrowed specifier (ZigGlobalObject.cpp moduleLoaderResolve/moduleLoaderImportModule both do keyZ.deref() + res.result.value.deref()). Storing the borrowed specifier directly was +1 in / -2 out on the same StringImpl for every ESM import of an internal/* module. Match the sibling HardcodedModule branch and return specifier.dupeRef().
This module is required by internal/streams/{destroy,pipeline,readable}
via the build-time requireTransformer, so it evaluates on first
node:stream load — not only under --expose-internals. Reword the
comment so future E() registrations don't assume they're test-only.
There was a problem hiding this comment.
No blockers from my side — the three issues I raised earlier (ASAN stderr assertion, missing dupeRef() in resolveMaybeNeedsTrailingSlash, misleading load-scope comment) are all fixed. Deferring to a human because this touches the module loader/resolver in Zig with manual refcounting and adds ~700 lines to internal/errors on the node:stream cold-start path.
Extended reasoning...
Overview
This PR enables require('internal/*') from userland under the BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING gate (and unconditionally in debug builds) so that Node's --expose-internals test corpus can run. It does so by:
- Adding a comptime
ExposedInternalsstring map insrc/jsc/ModuleLoader.zigand a fast-path infetchBuiltinModule. - Adding matching short-circuits in
VirtualMachine.zig_resolveandresolveMaybeNeedsTrailingSlash(the latter with aspecifier.dupeRef()to honour the caller-derefs contract). - Expanding
src/js/internal/errors.tsfrom ~22 lines to ~708 lines with a near-port of Node'slib/internal/errors.jssurface (E,codes,SystemError,hideStackFrames,UVException/DNSException,AbortError,determineSpecificType,formatList, etc.). - Adding
internal/error_serdes(~200 lines, verbatim Node port) andinternal/test/binding. - Importing 14 upstream Node parallel tests plus one new Bun regression test.
Security risks
The new resolver path is gated by Environment.isDebug or is_allowed_to_use_internal_testing_apis — the same gate as the existing bun:internal-for-testing module — so it does not expose internals to release-build userland by default. The exposed modules are already reachable from other builtins; this only adds a userland entry point under a testing flag. I don't see injection, auth, or data-exposure concerns. The one memory-safety issue (refcount underflow on the echoed specifier) was caught and fixed in fe5ef10.
Level of scrutiny
I'd put this at moderate-to-high. Although most of the line count is upstream Node test files and a fairly mechanical port of lib/internal/errors.js, the parts that warrant a human eye are:
- The Zig module-loader/resolver changes sit on a manual-refcounting boundary (
ErrorableString,bun.String); one real bug was already found here during review. internal/errors.tsis required at module-eval time byinternal/streams/{destroy,pipeline,readable}, so the ~30 prototype captures, six class declarations, and two eagerE()registrations now run on firstnode:streamload in every process. The cost is small, but it's a hot-path change worth a second opinion.- Three upstream test files were edited for JSC vs V8 message-text differences — the edits look surgical (regex widening, frame-label alternation) but a maintainer should confirm the policy on diverging from upstream test text.
Other factors
- All CodeRabbit threads and all three of my prior inline comments are resolved with commits.
- The only remaining bug-hunter finding is a non-blocking
test.concurrentnit on the new regression test. - robobun's CI summary reports the 14 target tests passing and attributes the failed lanes to fleet-wide flakes/agent deaths, but the most recent build banner still shows ❌ on fe5ef10 — worth confirming the fbcd73d build is green before merge.
- The
v8_enable_i18n_supporttypo fix was correctly reverted out of scope (it un-skipped 11 unrelated tests).
Both tests spawn independent bunExe() children with no shared state; per test/CLAUDE.md convention run them concurrently to halve this file's wall-clock on debug/ASAN lanes.
1 participant
What
Node tests flagged
// Flags: --expose-internalsdorequire('internal/errors')(and friends) directly. In Bun,src/js/internal/*.tsmodules were only reachable from other builtins via the build-timerequireTransformer→ numericInternalModuleRegistryIDs; userlandrequire('internal/errors')failed withCannot find module.Module loader
ModuleLoader.zig: addExposedInternals— a comptime-derived string map of everyinternal:*tag inResolvedSource.Tagkeyed asinternal/<name>. Whenis_allowed_to_use_internal_testing_apisis set (same gate asbun:internal-for-testing; always on in debug builds),fetchBuiltinModuleserves those specifiers straight from theInternalModuleRegistry.VirtualMachine.zig: let the same specifiers through both_resolveandresolveMaybeNeedsTrailingSlashunder the same gate.internal/errorsFills in
src/js/internal/errors.tswith the surface area the Node tests exercise — previously it only exportedaggregateTwoErrors:E/codes/getMessage/makeNodeErrorWithCodewith arg-count assertion (ERR_INTERNAL_ASSERTION)SystemError(withinfo/errno/syscall/path/destaccessors,kIsNodeError, custom inspect/toString)hideStackFrames(+.withoutStackTrace),HideStackFramesErrorAbortError,genericNodeError,isErrorStackTraceLimitWritabledetermineSpecificType,formatListDNSException,UVException,UVExceptionWithHostPort,uvErrmapGet(viaprocess.binding('uv'))kIsNodeError,captureLargerStackTrace,useOriginalNameaggregateTwoErrorsnow re-captures the caller's stack like Node doescodes.ERR_INVALID_ARG_TYPE/codes.ERR_ACCESS_DENIEDregistered sonew codes.ERR_INVALID_ARG_TYPE(...)worksOther
internal/test/binding— thinprocess.bindingproxy (Node'sinternalBindingfor tests).internal/error_serdes— error serializer used by worker message passing tests.Tests (14/18 from group
1v1cy7)Regression test:
test/js/node/module/expose-internals.test.ts.Three tests had superficial edits for engine-specific wording only (never
error.code, never assertion intent):test-errors-systemerror.js: V8's"Cannot read properties of undefined (reading 'syscall')"/"…read only property 'Symbol(kIsNodeError)'"→ regexes that also match JSC's phrasing.test-error-aggregateTwoErrors.js,test-dns-memory-error.js: top-of-module frame labelat Object(Node CJS wrapper) → also acceptat <anonymous>. Still asserts the internal frame was stripped and the caller's frame is first.Deferred (each is an unrelated Bun gap)
test-error-serdes.js—util.isDeepStrictEqualrejectsObject.create(Error.prototype, …)vs a realErrorInstance(Bun__deepEqualsErrorInstanceTypebranch inbindings.cpp).test-errors-hide-stack-frames.js— nativevalidateIntegershows up in caught-error.stack; Node wraps its JS validator inhideStackFrames.test-internal-errors.js—console.logdoesn't route throughprocess.stdout.write, sohijackStdoutcaptures nothing.test-webstream-readablestream-pipeto.js—pipeTorejects with the raw abort reason instead of aDOMException('AbortError')wrapper.Dropped from this PR
process.config.variables.v8_enable_i8n_support→v8_enable_i18n_supporttypo fix was reverted (afc51b2): fixing it un-skips 11 unrelatedhasIntl-gated tests (test-icu-*,test-url-format*,test-whatwg-*) that each need their own work. That belongs in a separate PR.Verify
bun bd node ./scripts/runner.node.mjs --quiet --exec-path=./build/debug/bun-debug --node-tests \ test-dns-memory-error test-error-aggregateTwoErrors test-error-format-list \ test-error-value-type-detection test-errors-aborterror test-errors-systemerror \ test-errors-systemerror-frozen-intrinsics test-errors-systemerror-stackTraceLimit-custom-setter \ test-errors-systemerror-stackTraceLimit-deleted test-errors-systemerror-stackTraceLimit-deleted-and-Error-sealed \ test-errors-systemerror-stackTraceLimit-has-only-a-getter test-errors-systemerror-stackTraceLimit-not-writable \ test-internal-error-original-names test-uv-unmapped-exception bun bd test test/js/node/module/expose-internals.test.ts