Conversation
|
Warning Review limit reached
Next review available in: 36 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 (3)
Comment |
|
Updated 8:59 PM PT - Jul 6th, 2026
❌ @robobun, your commit 44f59a6 has some failures in 🧪 To try this PR locally: bunx bun-pr 33445That installs a local version of the PR into your bun-33445 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR does not fix either of those, so I'm not adding the #31777 ( The format string is a primitive string in first position, so the gate this PR adds never applies to it. The bug is the $ bun -e "console.log('%i %i', [1, 2, 3, 4, 5])"
NaN %i # before this PR
NaN %i # after this PR#8627 ( That issue is about the single-argument inspect path, which this PR does not touch. $ bun -e 'console.log(new String("a")); console.log(String.prototype)'
[String: "a"]
# <- empty line, unchanged by this PRWhat this PR does change for |
There was a problem hiding this comment.
LGTM — clean, well-tested fix aligning console format-specifier substitution with the WHATWG spec and Node.
Extended reasoning...
Overview
This PR fixes console.log (and other console levels) to only apply %s/%d/%f/etc. format-specifier substitution to the first argument, and only when that argument is a primitive string — matching the WHATWG Console Formatter spec and Node.js. Previously, any string argument with arguments after it was treated as a format string, so e.g. console.log("a", "100%foo", 42) printed a 10042oo (the %f consumed 42).
The change touches src/jsc/ConsoleObject.rs (the format2 function) and adds tests to test/js/web/console/console-log.test.ts.
Security risks
None. This is a pure output-formatting predicate change with no allocation, lifetime, or FFI surface touched.
Level of scrutiny
Low-to-moderate. The Rust change is ~15 lines: a new is_format_string helper (4 boolean conditions) and a reorder of any = true to after the predicate check in both copies of the per-argument loop (color and no-color). I verified:
is_string_literal()is the correct narrowing (primitiveJSType::Stringonly, excludingStringObject/DerivedStringObject/RegExpObjectwhichTagPayload::Stringotherwise covers — confirmed at ConsoleObject.rs:2373-2374).- The
anyreorder is safe: it's only read at loop-top for the space separator and now insideis_format_string; the separator behavior is unchanged. - Both loop copies are updated identically via the shared helper.
Other factors
Test coverage is thorough: a table of 10 cases (including the extra-args-after-substitution case, non-string first arg, RegExp first arg, and positive controls that still substitute) run through both the color and no-color branches with an explicit assertion that the colored branch actually emits ANSI, plus a separate new String() boxed-object test. Tests follow harness conventions (bunEnv, await using, concurrent stdout/stderr/exited drain). No prior reviews or outstanding comments on the PR.
The per-argument loop in format2 promoted every top-level TagPayload::String
to StringPossiblyFormatted whenever arguments remained, so a later argument
that merely contained a '%' consumed and reformatted the arguments after it:
console.log("a", "100%foo", 42) printed "a 10042oo".
Gate the promotion on being the first argument, and on that argument being a
primitive string, matching the WHATWG console Formatter and node. A RegExp or
String object in first position is no longer treated as a format string.
fb57010 to
e87861c
Compare
There was a problem hiding this comment.
LGTM — tight, spec-aligned fix with both loop copies covered and positive controls in the test table; the sibling-copy nit is dead code and non-blocking.
Extended reasoning...
Overview
This PR changes one predicate in format2 (src/jsc/ConsoleObject.rs): the promotion of a console argument to StringPossiblyFormatted is now gated on (a) being the first argument (!any, with any = true moved after the check), (b) having remaining args to consume, and (c) being a primitive string (value.is_string_literal(), which excludes StringObject/DerivedStringObject/RegExpObject even though they map to TagPayload::String). The predicate is factored into a shared is_format_string helper and applied identically to both the color and no-color loop bodies. The len == 1 fast path earlier in the function is untouched. Tests add a 10-case table run through both color modes (with an explicit assertion that ANSI codes are present when FORCE_COLOR=1), plus a separate new String("%d") case.
Security risks
None. This is output-formatting logic for console.*; no parsing of untrusted input into privileged operations, no allocation sizing driven by external data, no auth/crypto/permissions.
Level of scrutiny
Low-to-moderate. It's a user-visible behavior change to console.log, but strictly in the direction of matching Node.js and the WHATWG Console Formatter spec — the previous behavior silently dropped arguments (console.log("a", "100%foo", 42) lost the 42), which is unambiguously a bug. The diff is ~15 native lines, mechanically verifiable by inspection: I confirmed is_string_literal() is the primitive-string-only check (JSValue.rs:281) and that TagPayload::String otherwise covers RegExp/StringObject (ConsoleObject.rs:2373-2374), so the added is_string_literal() gate is what fixes the /%d/ and new String("%d") cases.
Other factors
- Test quality is high:
await using+ concurrent pipe drain, positive controls ("%d/%s", 7, "k"and"%%d", 1) confirm first-arg substitution still works, and thecolored: stdout.includes("\x1b[")guard prevents the color-branch test from silently passing on the wrong path. - The one bug-hunter finding is a nit about a byte-identical copy in
pretty_format.rsthat still carries the old predicate — but that loop is provably unreachable (all callers passlen=1), so it's a maintenance-drift note, not a shipping bug. - No CODEOWNERS on the touched paths, no outstanding human review comments, PR description is thorough with a full before/after-vs-node table.
pretty_format.rs carries a byte-identical copy of format2's per-argument loop with the same pre-fix predicate. The multi-value path is unreachable today (all three callers pass len = 1), so this is not a behavior change, but leaving the two copies encoding different semantics invites drift.
There was a problem hiding this comment.
LGTM — the sibling copy in pretty_format.rs is now synced, and the fix is a narrow, well-tested spec alignment.
Extended reasoning...
Overview
Tightens the predicate that promotes a console argument to StringPossiblyFormatted in format2 (src/jsc/ConsoleObject.rs): only the first argument, and only when it is a primitive string (is_string_literal()), qualifies as a format string. The any = true assignment is moved after the check so !any reliably means "first iteration". The byte-identical loop in src/runtime/test_runner/pretty_format.rs receives the same gate (that path is currently unreachable — all callers pass len=1 — so no behavior change there). New table-driven tests in console-log.test.ts exercise both the color and no-color loop bodies, RegExp / String object exclusion, extra args after substitution, and positive cases that must still substitute.
Security risks
None. This is output formatting for console.*; the change strictly narrows when write_with_formatting runs and does not touch auth, crypto, filesystem, or network paths. is_string_literal() is a cell/type check with no JS side effects.
Level of scrutiny
Low-to-moderate. Small (~30 native lines), self-contained, spec-cited (WHATWG Formatter §2.2), and verified against Node's output. The reordering of any = true is behavior-preserving for the space-separator logic (the if any { write(b" ") } guard is evaluated before the moved assignment in both versions). The only prior review feedback — my nit about the pretty_format.rs sibling — was addressed in 6c0a408 and the thread is resolved.
Other factors
The bug-hunting pass found nothing. Test coverage is thorough for a formatting fix: it asserts the colored branch is actually taken (stdout.includes("\x1b[")), includes positive controls ("%d/%s", 7, "k" still substitutes), and pins Node-matching output line-by-line. The PR description accurately scopes what is and is not fixed relative to #31777/#8627.
There was a problem hiding this comment.
LGTM — the sibling site is now synced and the gate reads correctly in both loop bodies.
Extended reasoning...
Overview
This PR narrows console.log's format-specifier substitution to the first argument only, and only when that argument is a primitive string, matching the WHATWG console Formatter spec and Node's behavior. Touches src/jsc/ConsoleObject.rs (the live format2 loop, both color/no-color copies), src/runtime/test_runner/pretty_format.rs (a dead-code sibling copy synced for maintenance), and adds table-driven tests in test/js/web/console/console-log.test.ts.
The mechanical change is minimal: reorder any = true to after the tag check, and replace the inline predicate with a shared is_format_string() helper that additionally checks !any_printed and value.is_string_literal(). I verified is_string_literal() (src/jsc/JSValue.rs:281) is the primitive-string-only check that excludes StringObject/DerivedStringObject, so the RegExp and new String() cases fall out correctly.
Security risks
None. This is stdout/stderr text formatting with no parsing of untrusted structured input, no allocation changes, no refcount or GC-root changes, and no FFI surface touched.
Level of scrutiny
Low-to-moderate. console.log is extremely hot in terms of call frequency but the change is a pure predicate tightening on an existing branch — it can only reduce the set of values promoted to StringPossiblyFormatted, never expand it. The reorder of any = true is the only control-flow change and I traced it: nothing between the old and new position reads any except the new helper. The pretty_format.rs change is provably unreachable (all three callers pass len=1).
Other factors
- My earlier nit about the
pretty_format.rssibling was addressed in 6c0a408; the two loop bodies are now byte-equivalent modulo the localTagenum. - Test coverage is strong: 10 table cases across both color modes with an explicit assertion that the colored branch is actually taken, positive controls (
"%d/%s", 7, "k"still substitutes), and a separateStringobject case. - The bug hunter found no issues on the current revision.
- No outstanding reviewer comments; the one inline thread is resolved.
Status: diff is green, red lanes are unrelated CI flakeThe change itself passes in CI. The failing lanes are unrelated, each retried once and still flaky on a congested build, and none touches console formatting or either file this PR changes (
Plus infra noise that never ran a test: I pushed one |
What
console.log()(and every other console level) applied%s/%d/%f/%o/%c/%%substitution to every top-level string argument, not justargs[0]. A later argument that merely contained a%silently consumed and reformatted the arguments after it, so they disappeared from the output entirely.Logging a URL, a query string, or a percentage as a second argument is a common pattern, and the
42/8above are gone from the line, not just reformatted.Root cause
src/jsc/ConsoleObject.rs, the per-argument loop informat2(both the color and no-color copies):The loop walks every argument through the same body, so every string argument with something after it got promoted to
StringPossiblyFormattedand ran throughwrite_with_formatting, which pulls its substitutions out ofremaining_values. The promotion is only correct forvals[0].Two smaller divergences fall out of the same predicate:
TagPayload::Stringalso coversStringObject/DerivedStringObject/RegExpObject, soconsole.log(/%d/, 1)printed/1/andconsole.log(new String("%d"), 1)printed1. The WHATWG Formatter only treats a primitive string as a format string (node checkstypeof first === 'string').console.log("%s %s", "a", "b", "%s", "c")printeda b cinstead ofa b %s c.util.formatis implemented separately in JS and was already correct; only the native console path diverged.Fix
Gate the promotion on being the first argument and on that argument being a primitive string, via a shared
is_format_stringhelper used by both copies of the loop.Sibling site
src/runtime/test_runner/pretty_format.rsholds a byte-identical copy of this loop (the jest snapshot/diff formatter, with its own localTagenum). Its multi-value path is unreachable today: all three callers (test_runner/mod.rs:208,test_runner/diff_format.rs:47and:56) passcore::slice::from_ref(&x)withlen = 1, so thelen == 1branch returns first. The same gate is applied there anyway to keep the two copies from silently diverging. No behavior change, so no test covers it.Verification
test/js/web/console/console-log.test.tsgains a table ofconsole.logcalls whose expected output is node's, run through both the color and no-color branches of the loop (with an assertion that the colored branch really is taken), plus theStringobject case.full before/after vs node
"a", "100%foo", 42a 100%foo 42a 10042ooa 100%foo 42"q", "/p?x=%ff", 8q /p?x=%ff 8q /p?x=8fq /p?x=%ff 8"l", "%d %s", 1, "t"l %d %s 1 tl 1 tl %d %s 1 t"x", "%%", 1x %% 1x % 1x %% 1"x", "%c", "color:red", "y"x %c color:red yx yx %c color:red y"%s %s", "a", "b", "%s", "c"a b %s ca b ca b %s c1, "%d", 21 %d 21 21 %d 2/%d/, 1/%d/ 1/1//%d/ 1new String("%d"), 1[String: '%d'] 11[String: "%d"] 1"%d/%s", 7, "k"7/k7/k7/k"%%d", 1%d 1%d 1%d 1Existing coverage is unchanged:
console-log.expected.txt,test/js/bun/console/,test/js/bun/util/inspect.test.js,test/js/node/util/node-inspect-tests/, and thetest-console-*.jsnode parallel tests all still pass.