console: route console.trace() output to stderr - #36506
Conversation
console.trace() was writing to stdout because the stream selection in message_with_type_and_level only checked MessageLevel::Warning/Error and JSC dispatches Trace with MessageLevel::Log. Node.js writes console.trace() to stderr. Add MessageType::Trace to the use_stderr condition and reuse that flag for the color and writer selection so the three stay in sync. Fixes #19952 Co-authored-by: MOHAMMED HANAN M T P <91409429+hanu-14@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 3 minutes 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 (2)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
LGTM — routes console.trace() to stderr to match Node.
What was reviewed:
use_stderrnow includesMessageType::Trace; the two duplicatedmatches!(level, Warning | Error)sites are folded into it — checked that the addedAssertcoverage in the color/writer selection is a no-op (JSC dispatches assert withMessageLevel::Error, and the len==0 arm already usederror_writer()).- Confirmed
use_stderris computed after theStartGroup/EndGroupearly returns and beforeClear, so the lock still guards the right stream on every remaining path. - Tests drain both pipes concurrently, use
bunEnv/bunExe/await using, assert stdout is empty and stderr carries both the message and a stack frame, and would fail on main.
Extended reasoning...
Overview
Two-file change: src/jsc/ConsoleObject.rs adds MessageType::Trace to the use_stderr computation in message_with_type_and_level_, and reuses that flag for the ANSI-color and writer selection instead of repeating matches!(level, Warning | Error) at each site. A new test/js/web/console/console-trace.test.ts adds three concurrent subprocess tests covering console.trace(msg), console.trace() with no args, and interleaving with console.log.
Security risks
None. This only changes which fd (stdout vs stderr) a diagnostic message is written to; no untrusted-input parsing, no allocation, no FFI surface change.
Level of scrutiny
Low. This is a narrow Node-compat fix that has been proposed multiple times (#35058, #35040, #32638, #20020) — the PR consolidates the fix and folds the three duplicated conditions into one flag as REVIEW.md's dedup guidance calls for. I checked the one non-obvious consequence of the dedup: Assert is now included in the color/writer selection via use_stderr, whereas before only the level check applied. JSC's ConsoleClient dispatches console.assert with MessageLevel::Error, so the level check already routed it to stderr; the len==0 assert arm at line 420 also hard-codes error_writer()/enable_ansi_colors_stderr(). So no behavior change beyond Trace.
Other factors
Tests follow harness conventions exactly: it.concurrent for independent spawns, await using on the proc, Promise.all over stdout/stderr/exited to avoid pipe deadlock, exit-code asserted last, and the issue URL as the only comment. The assertions are specific enough to fail on main (stdout must be exactly "" / "before\nafter\n", stderr must contain the marker and "at "). No existing test asserts trace output on stdout, so no snapshots need updating.
|
Closing as a duplicate: #37128 reworks console output as a whole and routes console.trace() to stderr (checked on its current head: message and stack land on stderr, nothing on stdout), so the change here is covered there. |
Problem
console.trace()writes to stdout. Node.js writes it to stderr.Cause
message_with_type_and_levelinsrc/jsc/ConsoleObject.rsselects the output stream based onMessageLevel::Warning | MessageLevel::Error. JSC dispatchesconsole.trace()withMessageType::TraceandMessageLevel::Log, so it fell through to stdout. The stream lock, color selection, and writer selection each repeated the same incomplete condition.Fix
Add
MessageType::Traceto theuse_stderrcomputation and reuse that flag for the color and writer selection so all three stay consistent. This is the fix from #35058 by @hanu-14 with the three duplicated conditions folded into the existinguse_stderrvariable.This PR is scoped to the stderr routing only. #32638 and #20020 also add a Node-style
Trace:header before the message; that is a separate formatting change not included here (#36471 covers it via the JS wrapper path).Fixes #19952
Closes #35058
Closes #35040
Verification
New
test/js/web/console/console-trace.test.tsasserts thatconsole.trace()(with and without arguments) writes nothing to stdout and writes both the message and the stack to stderr. All three cases fail on main and pass with this change.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
self-review · 1 surviving concern
24 concerns were raised and did not survive verification.
root cause · written by the author bot
The stderr routing logic in ConsoleObject.rs decided the output stream solely from the message level, checking only for Warning or Error, so console.trace() was written to stdout instead of stderr as Node.js does. The fix extends the use_stderr computation to also account for MessageType::Trace and reuses that flag at the color and writer selection sites rather than repeating the level check. This routes trace output to stderr without altering behavior for any other console method.