Conversation
|
Warning Review limit reached
Next review available in: 7 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline notes, I checked that InternalModuleRegistry::Field::InternalConsoleDiagnostics is auto-generated from the presence of src/js/internal/console_diagnostics.ts (no manual registry edit needed), that clearExceptionExceptTermination() has precedent in bindings and preserves termination exceptions (not the raw clearException() REVIEW.md forbids), and that the args passed to JSC::call are held in a MarkedArgumentBuffer so they stay rooted across the publish call.
Extended reasoning...
This PR wires the global native console and Console constructor instances into Node's built-in console.* diagnostics channels. It touches the console.log hot path in C++ with new exception-scope handling and adds a codegen-dependent internal module, and the inline finding about indirect methods (trace/assert/dirxml/etc.) is a real Node-compat scope question a maintainer should decide on — so deferring rather than approving. The note above records the extra checks done beyond the two inline nits so a later pass doesn't re-derive them.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
9e661d1 to
d32d00a
Compare
There was a problem hiding this comment.
No bugs in the current revision — all three earlier findings (channel()-time wrapping mutating the global, uncaptured Object.defineProperty, double-publish when globalThis.console is replaced) are addressed with tests. Deferring to a human because the lazy-wrap design mutates globalThis.console[method] on first subscribe and intentionally diverges from Node for captured refs and indirect methods (trace/assert/count/…); that trade-off vs. publishing from native is worth a maintainer sign-off. One unresolved comment-cop flag remains on the two-line comment at diagnostics_channel.ts:237.
Extended reasoning...
Overview
Adds Node's built-in console.{log,info,debug,warn,error} diagnostics_channel support in two places: (1) src/js/node/diagnostics_channel.ts — markActive() now lazily replaces globalThis.console[method] with a shorthand-method wrapper that publishes args before delegating to the captured native function, guarded by a per-method idempotency flag and a globalThis.console === nativeConsole identity check; (2) src/js/builtins/ConsoleObject.ts — Console.prototype.log/info/debug/warn/error are now five distinct methods that each publish to their own channel (previously info/debug aliased log and error aliased warn). Five new subprocess tests cover the global path, the constructor path, no-mutation-until-subscribe, no-double-publish after replacing the global, and non-leakage into dir/trace/assert.
What was checked this run
- The closure-pins-Channel invariant: once
nativeConsole[method]is reassigned to the wrapper, the wrapper's closure holdschstrongly, so theWeakRefMapkeeps returning the same instance andwrappedConsoleMethods[method]makes subsequent inactive→active transitions no-ops. If wrapping is skipped (replaced console / non-function), the flag is not set, so a later subscribe after restoring the native console can still wrap. consoleChannelMethodshas__proto__: null, so channel names like"toString"or"constructor"don't accidentally match.- The
Console constructor instancestest subscribes before readingconsole.Console, so both the wrapper and the in-method publish exist — but onlyc.log()runs, so no double-count. nativeConsoleis captured at module-load;new console.Console(...)necessarily loads this module first (viacreateConsoleConstructor'srequire), so the capture is always the real native console in the double-publish scenario.
Security risks
None — no untrusted input parsing, no auth/crypto/fs. The wrapper reads/writes globalThis.console, which is inherently user-mutable.
Level of scrutiny
Medium-high. The change is small (~65 LOC of runtime code) but touches the global console object at runtime and lives in src/js/builtins/ + src/js/node/, which have tamper-resistance and hot-path constraints. The PR went through three design iterations with a real bug found each round, which suggests the interaction surface is subtle.
Other factors
- Design trade-off not yet signed off: the lazy-wrap approach keeps console fully native until first subscribe, at the cost of two documented divergences from Node — a
console.logreference captured before subscribe bypasses the channel, and nativeconsole.trace/assert(false)/count/dirxml/group/timeLogdo not publish (whereas Node's do, via delegation). The PR description calls both out, but a maintainer should confirm this is the preferred trade-off vs. publishing from the nativemessageWithTypeAndLevelpath. - Outstanding automated comment: the comment-cop flag on diagnostics_channel.ts:237 is unresolved. The author has twice pushed back on identical flags for the adjacent invariant comment; this one targets the new two-line "replaced console" comment. It's a linter, not a human, but it's technically open.
- Test coverage: new tests are subprocess-isolated, drain both pipes concurrently, use
describe.concurrent, and assert exact structured output. They fail on the pre-fix debug build per the evidence block.
Node.js publishes the argument array to the console.log, console.info,
console.debug, console.warn, and console.error diagnostics_channel channels on
every corresponding console.* call. This is the documented monkeypatch-free way
to intercept console output for log capture / log shipping.
Bun's global console methods are native JSC host functions, so subscribers to
these channels received nothing.
In node:diagnostics_channel, the first time channel('console.<m>') is
materialized (directly or via subscribe), wrap globalThis.console[m] once to
publish the args array before delegating to the original. The closure holds the
Channel strongly so the WeakRefMap keeps returning the same instance and the
method is never re-wrapped. Console stays fully native until a console channel
is materialized; once wrapped, the per-call overhead is one hasSubscribers read
(constant false on an inactive Channel via the prototype swap).
Console constructor instances go through src/js/builtins/ConsoleObject.ts;
log/info/debug/warn/error are now separate methods that each publish to their
own channel before writing, mirroring Node's lib/internal/console/constructor.js.
…apper
Wrapping at channel() caused two problems:
- Reading console.Console (which calls channel('console.*') internally to
set up Console.prototype.log/info/debug/warn/error) replaced the native
global console methods as a side effect, breaking
inspector.console.log === globalThis.console.log and similar identity
checks.
- A plain function expression wrapper is constructible, so
new console.log() stopped throwing (test-console-methods.js).
Wrapping from markActive() instead defers the global-console mutation to
the first subscribe/bindStore on a console.* channel. A wrappedConsoleMethods
guard keeps inactive -> active transitions idempotent.
The wrapper is now a computed shorthand method, so it keeps .name === method
and is not constructible, matching the native host function it replaces.
Extends the 'stays native' test to assert that require(), channel() and
reading console.Console do not mutate globalThis.console.log.
A user who did globalThis.console = new console.Console(...) before subscribing would get every call published twice: once from the wrapper installed by markActive and again from Console.prototype.<method>'s own onLog.publish. Capture the native console at module load and only install the wrapper on that object; a replaced console either already publishes (Console instance) or is user-owned.
07f2b29 to
30c5835
Compare
The globalThis.console !== nativeConsole guard was redundant: the wrap targets nativeConsole[method], and nativeConsole is captured before any Console instance can exist (createConsoleConstructor requires this module first), so orig can never be a function that already publishes. With the guard removed the 'publishes once' test still passes because console.log dispatches to the replacing instance while the wrapper on the original console is unreachable. The guard also silently skipped the wrap in the swap-subscribe-restore sequence, leaving the restored native console unwrapped with no retry path. The assignment to nativeConsole[method] is now in a try/catch so a frozen / non-writable console does not cause subscribe() to throw (it was previously infallible and is infallible in Node). The idempotency flag is set only after the assignment succeeds.
There was a problem hiding this comment.
No issues found in the final revision — all earlier findings (side-effect on console.Console access, double-publish when the global is a Console instance, frozen-console throw, redundant identity guard) are addressed and covered by tests. Deferring to a human for sign-off on the architectural choice: lazy JS-side wrapping of nativeConsole[method] on first subscribe (vs. publishing from the native console path) mutates the global console and leaves the documented caveats (pre-subscribe captured refs; native trace/assert/count/etc. not routing through the wrapped methods).
What was reviewed:
markActive→wrapConsoleMethodForChannelidempotency across subscribe→unsubscribe→resubscribe: closure pins theChannel,wrappedConsoleMethodsguard holds — no re-wrap, same instance returned.createConsoleConstructor'schannel()calls no longer mutate the global (wrap moved tomarkActive);dirxmlstill aliased tologmatches Node.- Replaced-console / frozen-console paths: wrapper targets captured
nativeConsole, try/catch keepssubscribe()infallible; publishes-once test still holds without the dropped identity guard. consoleChannelMethodslookup is__proto__: nulland gated ontypeof name === "string"— no Symbol/proto pollution.
Extended reasoning...
Overview
Adds Node's built-in console.{log,info,debug,warn,error} diagnostics_channel support. Two coordinated changes: (1) src/js/node/diagnostics_channel.ts — markActive() now checks for a console.* channel name and lazily replaces nativeConsole[method] (captured at module load) with a shorthand-method wrapper that publishes then delegates; a __proto__:null idempotency map plus the closure pinning the Channel in the WeakRefMap prevent re-wrapping. (2) src/js/builtins/ConsoleObject.ts — Console.prototype.{log,info,debug,warn,error} are now distinct methods that each publish to their own channel before writing (previously debug/info aliased log and error aliased warn). Six subprocess-based tests added.
Security risks
None identified. No user-controlled data reaches a security-sensitive sink. The consoleChannelMethods lookup uses __proto__: null and is gated on typeof channel.name === "string", so Symbol names and prototype pollution can't reach wrapConsoleMethodForChannel. The wrapper uses orig.$apply (primordial-safe). The only mutation is to nativeConsole[method], wrapped in try/catch so a hardened/frozen console can't turn subscribe() into a throw.
Level of scrutiny
Medium-high. console.log is one of the most-called functions in any program, and this PR mutates it (once, on first subscribe). The interaction between the two publish paths (global-console wrapper vs. Console.prototype in-method publish) took three prior review rounds to converge — each round found a real ordering/coupling issue (channel() side-effect on console.Console read; double-publish when global is a Console instance; strict-mode throw on frozen console; redundant-and-harmful identity guard). The final state is clean and each fix is covered by a dedicated subprocess test.
Other factors
The core reason to defer rather than approve: this is an architectural choice, not a mechanical fix. The original iteration published from the C++ ConsoleObject.cpp path (no global mutation, no captured-ref caveat, indirect methods like trace/assert covered for free) and was replaced with the JS-side lazy-wrap approach. The current approach keeps console fully native until someone subscribes — a reasonable perf trade — but leaves documented gaps (refs captured before first subscribe bypass the wrapper; native console.trace/assert/count/dirxml/group/timeLog don't publish because they don't delegate through the wrapped methods). A maintainer should confirm that trade-off is the one Bun wants, since it's user-observable Node divergence and would be harder to change once shipped.
Test coverage is solid (six describe.concurrent subprocess tests draining both pipes, asserting exact JSON payloads and object identity, plus the frozen-console and replaced-console edge cases). The PR description reports test-console-methods.js, all 113 console tests, and all 35 vendored test-diagnostics-channel-* node-parallel tests pass.
|
Updated 11:07 PM PT - Jul 28th, 2026
❌ @robobun, your commit c28bf9b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36226That installs a local version of the PR into your bun-36226 --bun |
There was a problem hiding this comment.
All prior review feedback has been addressed and I found nothing new on this revision, but I'd like a maintainer to sign off on the design: subscribing to a console.* diagnostics channel now monkeypatches globalThis.console[method] (lazily, on first markActive), with the documented caveat that a console.log reference captured before the first subscribe bypasses publishing.
Checked: wrapping is idempotent across unsubscribe→resubscribe (wrappedConsoleMethods guard); the closure pins the Channel so the WeakRefMap never hands back a fresh instance; channel() and console.Console no longer mutate the global; frozen-console assignment is caught so subscribe() stays infallible; Console.prototype.dirxml still aliasing log matches Node (dirxml publishes to console.log there too).
Extended reasoning...
Overview
Adds Node's built-in console.{log,info,debug,warn,error} diagnostics_channel channels. Two halves: (1) src/js/node/diagnostics_channel.ts — markActive() now lazily wraps nativeConsole[method] (captured at module load) with a shorthand-method closure that publishes args before delegating; (2) src/js/builtins/ConsoleObject.ts — Console.prototype.{log,info,debug,warn,error} are now distinct methods that each publish to their own channel (previously debug/info aliased log and error aliased warn). Six subprocess tests added.
Security risks
None. No auth/crypto/permissions/parsing of untrusted input. The consoleChannelMethods lookup table is null-prototype so a Symbol or polluted-prototype channel name can't reach the wrap path, and typeof channel.name === "string" guards the lookup.
Level of scrutiny
Medium-high. console.log is one of the most-called functions in any JS program, and this PR reassigns it on the global object as a side effect of dc.subscribe("console.log", fn). The approach is deliberately lazy to keep the native fast path for the common case, but that's exactly the kind of tradeoff (and the pre-subscribe-capture caveat, plus the acknowledged divergence for native trace/assert/count/etc. not delegating through the wrapped methods) that a maintainer should confirm they're comfortable with rather than have auto-approved.
Other factors
This PR went through five revisions responding to my earlier findings (wrap moved from channel() to markActive(); Object.defineProperty replaced with a computed shorthand method; the redundant globalThis.console !== nativeConsole guard dropped; assignment wrapped in try/catch with the idempotency flag set only on success). All threads are resolved and the current diff reflects those fixes. Test coverage is solid — subprocess-isolated, covers global console, Console constructor instances, identity preservation until subscribe, replaced-console-publishes-once, and frozen-console. I re-verified the remaining invariants in the message above; the one narrow edge case I previously called out as unfixable by the identity guard (console.log = new console.Console(...).log on the native object → double-publish) remains, but it's obscure and orthogonal to this design.
|
CI status across the two most recent builds:
The six new tests in |
|
Closing: #37128 reworks the console and lists this PR as superseded (the native console publishes It does not publish from |
2 participants
What this fixes
Node.js publishes the argument array to the
console.log,console.info,console.debug,console.warn, andconsole.errordiagnostics_channel channels on every correspondingconsole.*call (documented under diagnostics_channel > Built-in Channels > Console). This is the monkeypatch-free way to intercept console output for log capture / log shipping.Bun's global
console.log/info/debug/warn/errorare native JSC host functions, so subscribers to these channels received nothing.Repro
Fix
node:diagnostics_channel: when aconsole.*channel first becomes active (markActive, i.e. firstsubscribe/bindStore), wrapglobalThis.console[m]once with a computed shorthand method that publishesargswhench.hasSubscribersand delegates to the original. The closure holds theChannelstrongly, so theWeakRefMapkeeps returning the same instance and the method is never re-wrapped; awrappedConsoleMethodsguard keeps inactive→active transitions idempotent. Console stays fully native until someone actually subscribes; once wrapped, the per-call overhead is onehasSubscribersread. The shorthand-method wrapper keeps.name === methodand is not constructible, matching the native host function.console.Consoleconstructor instances go through the JS path insrc/js/builtins/ConsoleObject.ts;log/info/debug/warn/errorare now separate methods that each publish to their own channel before writing, mirroring Node'slib/internal/console/constructor.js. These callchannel()(notsubscribe), so readingconsole.Consoledoes not mutate the global console.Known caveat: a
console.logreference captured before the first subscribe bypasses the wrapper (the lazy approach trades this for keeping console native in the common case). The globalconsole.trace/assert/count/dirxml/group/timeLogare native and do not delegate through the wrappedconsole.error/warn/log, so unlike Node they do not publish; the five documented direct channels are the scope here.Tests
Four new cases in
test/js/node/diagnostics_channel/diagnostics_channel.test.ts(subprocess-based so the runner's own console output isn't affected): global methods publish their raw args array with object identity preserved;Consoleconstructor instances publish; the global console stays native acrossrequire/channel()/readingconsole.Consoleand only changes aftersubscribe; materializingconsole.logdoes not affect other console methods. They fail on main and pass on this build.test-console-methods.js,inspector-profiler.test.ts, all 113 existingconsoletests and all 35 vendoredtest-diagnostics-channel-*node-parallel tests pass.[review] gate passed · iteration 4 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 1 rejected · iteration 4
evidence per changed file