Conversation
|
Updated 8:06 AM PT - May 5th, 2026
❌ @autofix-ci[bot], your commit 581f051 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29084That installs a local version of the PR into your bun-29084 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a regression test for console.table handling of embedded C0 control characters and modifies TablePrinter to quote/escape string cells only when they contain layout-breaking C0 controls, applying that decision consistently to width calculation and rendering. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29082.test.ts`:
- Around line 2-8: In the regression test file issue/29082.test.ts, remove the
multi-line historical bug-description comment block and leave only the
single-line issue URL reference; ensure any remaining comment explains test
rationale (not bug history) and keep the test body unchanged—target the leading
comment block that currently begins with the backtick-marked explanation about
console.table and replace it with a concise one-line reference to the issue.
- Around line 20-22: The helper runTable currently awaits proc.stdout.text() and
proc.exited and asserts expect(exitCode).To allow callers to assert stdout
before exit, remove the internal exit assertion from runTable, have it return
both stdout and exitCode (e.g., return { stdout, exitCode } or a tuple) by
awaiting Promise.all([proc.stdout.text(), proc.exited]) and returning both
values, and update call sites to assert output first then expect(exitCode).
Ensure references to runTable, stdout, exitCode, and proc are used to locate and
update the code.
🪄 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: 9b4ab70a-be17-4d8f-b90d-7bcd65867372
📒 Files selected for processing (2)
src/bun.js/ConsoleObject.zigtest/regression/issue/29082.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/bun.js/ConsoleObject.zig:341-343— TheshouldQuoteStringCellcheck only covers \n, \r, and \t, leaving other C0 control characters (\v, \f, \0, 0x01–0x08, 0x0E–0x1F, 0x7F) unhandled — strings containing those chars are still emitted raw, potentially breaking the table border. Broadening the check to any codepoint < 0x20 or == 0x7F would close the gap.Extended reasoning...
What the bug is
shouldQuoteStringCell(lines 341–343) checks for exactly three ASCII control characters: \n (0x0A), \r (0x0D), and \t (0x09). The broader class of C0 control characters — \v (0x0B, vertical tab), \f (0x0C, form feed), \0 (NUL, 0x00), and all of 0x01–0x08 and 0x0E–0x1F — is never tested. DEL (0x7F) is also omitted.The code path that triggers it
When
shouldQuoteStringCellreturnsfalse,value_formatter.quote_stringsis set tofalsein bothgetWidthForValue(line 359) andprintRow(line 506). Withquote_strings = false, the formatter takes the unquoted path and callswriter.writeAll(slice)verbatim — no escaping occurs. Meanwhile,VisibleCharacterCountercounts those characters as zero visible width (becausevisible.zig'sisZeroWidthCodepointTypetreats all cp ≤ 0x1F as zero-width), so the computed column width is narrower than the raw bytes actually written. The width inconsistency breaks the table border.Why existing code doesn't prevent it
The PR correctly handles the most commonly reported characters from issue #29082, but the guard is written as three explicit
indexOfAsciiCharchecks rather than a range test. There is no fallback that catches other control characters. ThewriteJSONStringpath (triggered whenquote_strings = true) would fully escape every C0 character via JSON escaping, so the fix exists — it just is not triggered for the missing chars.Impact
A string like
"foo\vbar"passed toconsole.tablewould produce a visually broken table in any terminal that interprets vertical tab (0x0B) as cursor movement. Similarly, \f (0x0C) and other C0 chars cause real cursor displacement in many terminal emulators. NUL (0x00) can silently truncate output in some environments. These characters are rare in real-world usage, making this a low-priority nit.Step-by-step proof
console.table([{ a: 'foo\vbar' }])is called.getWidthForValuecallsshouldQuoteStringCellfor'foo\vbar'.- The string contains \v (0x0B).
indexOfAsciiChar('\n')→ null;indexOfAsciiChar('\r')→ null;indexOfAsciiChar('\t')→ null. Function returnsfalse. value_formatter.quote_strings = false. TheVisibleCharacterCountercounts 6 visible characters (foo + bar; \v is zero-width), returning width 6.- In
printRow, same path:shouldQuoteStringCell→ false, raw slicefoo\vbaris written. - The terminal receives \v mid-row and moves the cursor down one line, fragmenting the table border.
How to fix
Replace the three
indexOfAsciiCharcalls with a range check: returntrueif any byte in the string satisfiesb < 0x20 or b == 0x7F. This matches the full set of characters thatVisibleCharacterCountertreats as zero-width and that terminals may interpret as control sequences.
| fn shouldQuoteStringCell(this: *TablePrinter, value: JSValue, tag: ConsoleObject.Formatter.Tag.Result) bun.JSError!bool { | ||
| if (!(tag.tag == .String or tag.tag == .StringPossiblyFormatted)) return true; | ||
| if (!value.isString()) return false; | ||
| var str: bun.String = try bun.String.fromJS(value, this.globalObject); | ||
| defer str.deref(); | ||
| if (str.isUTF16()) { | ||
| for (str.utf16()) |c| { | ||
| if (c < 0x20) return true; | ||
| } | ||
| } else { | ||
| for (str.byteSlice()) |b| { | ||
| if (b < 0x20) return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /// Compute how much horizontal space will take a JSValue when printed | ||
| fn getWidthForValue(this: *TablePrinter, value: JSValue) bun.JSError!u32 { | ||
| var width: usize = 0; |
There was a problem hiding this comment.
🟣 The PR's control-character escaping fix is incomplete: column header names are printed raw at line 646 with writer.print("{f}", .{col.name}) and no equivalent of shouldQuoteStringCell. A JS property name containing \n (e.g. console.table([{['a\nb']: 123}])) will emit a literal newline into the header row, breaking the table border in exactly the same way the original issue did for data cells. This is a pre-existing gap in the same TablePrinter rendering path — the PR didn't introduce it, but it's an opportunity to extend the same fix to headers.
Extended reasoning...
What the bug is and how it manifests
The PR introduces shouldQuoteStringCell and applies it to two places in TablePrinter: getWidthForValue (for column sizing) and printRow (for rendering). Both now correctly quote/escape string cells containing C0 control characters. However, a third place in the same struct — the column header printing path in printTable — is entirely untouched. At line 646, column names are emitted directly: try writer.print("{f}", .{col.name}). JavaScript property names are arbitrary strings, including computed property keys like ['a\nb'], so a column header can contain a raw newline.
The specific code path that triggers it
- User calls
console.table([{['a\nb']: 123}]). - The JS property key
'a\nb'is converted to a column name at line 429:const col_str = String.init(col_key). - Column width is measured at line 627 via
col.name.visibleWidthExcludeANSIColors(false)— this counts visible characters in the raw bytes; the embedded\nhas zero visible width and does not account for the line-splitting effect. - At line 646,
writer.print("{f}", .{col.name})emits the raw column name bytes, including the literal newline, directly into the header row. - The terminal moves the cursor to the next line mid-header, splitting the header row and misaligning all subsequent borders.
Why existing code doesn't prevent it
The shouldQuoteStringCell function added by this PR only gates the quote_strings flag inside getWidthForValue and printRow. The header printing loop in printTable has no analogous check — col.name is always printed raw regardless of its contents. The width measurement via visibleWidthExcludeANSIColors similarly does not model the line-splitting effect of \n, so the measured column width and the actual rendered width diverge.
Impact
Any console.table call with a property key containing a C0 control character (most practically \n via template literals or String.fromCharCode) will produce a broken table border — the same visual corruption as the original issue #29082. In practice, computed property keys with embedded newlines are rare, but they are valid JavaScript and deserve the same protection that data cells now get.
How to fix
Apply the same escaping logic to the header printing path: before printing col.name, check if it contains any byte < 0x20 (mirroring shouldQuoteStringCell), and if so write the JSON-escaped form instead. The width measurement at line 627 should likewise use the escaped length when the name would be quoted, to keep sizing and rendering in sync.
Step-by-step proof
console.table([{["a\nb"]: 123}])— property key is'a\nb'(3 bytes: 0x61, 0x0A, 0x62).- Column name stored as
col.name = 'a\nb'. - Width measured:
visibleWidthExcludeANSIColors('a\nb')counts 'a' (1) + '\n' (0) + 'b' (1) = 2 visible chars. Column sized for 2-char header. - Header row starts:
│ │ a— then literal newline emitted at character 7. - Terminal cursor moves to next line. Rest of header (
b │ ...) prints on line 2. - Table borders are completely misaligned from this point forward — same visual corruption as the original bug for data cells.
- Data cells (e.g. the value 123) were fixed by this PR — but the header for the same column is still broken.
There was a problem hiding this comment.
Confirmed — a property key like ["a\nb"] breaks the header row for the same underlying reason. This is a pre-existing gap (the PR did not introduce it) and the header code path has its own width + render flow with a bun.String column name, not a JSValue. Leaving this one out of scope for this PR to keep it focused on the cell fix from #29082; happy to file a follow-up if you'd like it tracked.
There was a problem hiding this comment.
LGTM — all previously raised issues resolved.
Extended reasoning...
Overview
This PR fixes console.table rendering corruption caused by C0 control characters in string cells (issue #29082). Changes touch src/bun.js/ConsoleObject.zig (adds shouldQuoteStringCell, getWidthForValueWithTag) and a new regression test file. The final commit (a75b85b) incorporates all review feedback.
Security risks
None. This is a display/formatting path with no auth, crypto, or permission logic.
Level of scrutiny
Moderate: the change is in the JS console rendering path, not a hot or security-sensitive path. The fix is well-scoped — it only affects string cells containing C0 bytes (excluding ESC to preserve ANSI colors). Width calculation and render now use the same quote_strings decision, so they stay in sync.
Other factors
All issues I previously raised were addressed: ANSI regression fixed, double-scan eliminated, stderr drained, tests run concurrently, positive assertion added to the C0 test. The pre-existing index-column gap flagged by the bug hunter predates this PR and is out of scope. Test suite passes 9/9 with the fixed build.
There was a problem hiding this comment.
LGTM — all review feedback addressed across multiple rounds; the implementation correctly handles control characters in both data cells and index column, excludes ESC for ANSI compatibility, and deduplicates the byte scans.
Extended reasoning...
Overview
This PR fixes breaking when string cells contain C0 control characters (, , , etc.) by introducing / and routing both width calculation and rendering through the same quoting decision. It also covers the index column (object property keys) and adds a comprehensive regression test suite.
Security risks
None. This is purely a display/formatting fix with no security surface.
Level of scrutiny
Low — the change is scoped to rendering, a non-security, non-critical debug utility. The logic is well-contained and the test suite is thorough.
Other factors
After five rounds of review covering: accidental revert of the Zig fix, ANSI color regression (ESC in C0 range), double-scan inefficiency, index column gap, missing positive test assertions, stderr deadlock risk, and concurrent test usage — all issues have been resolved in the final commits. The pre-existing header-name gap was explicitly scoped out by the author. The current HEAD state is correct.
Fixes #29082. Before: cells containing \n landed a literal newline in the middle of the row, breaking the table border: $ bun -e 'console.table([{ foo: 123, bar: "Hello\nWorld"}])' ┌───┬─────┬────────────┐ │ │ foo │ bar │ ├───┼─────┼────────────┤ │ 0 │ 123 │ Hello World │ └───┴─────┴────────────┘ After: strings containing \n, \r, or \t are rendered in the quoted JSON-escaped form (matching Node.js), so the row stays on one line: ┌───┬─────┬────────────────┐ │ │ foo │ bar │ ├───┼─────┼────────────────┤ │ 0 │ 123 │ "Hello\nWorld" │ └───┴─────┴────────────────┘ TablePrinter.printRow / getWidthForValue previously set quote_strings = false unconditionally for string cells. Bun's stylistic choice is to print plain strings unquoted — that's fine for ordinary strings, but raw control characters break the fixed-width layout because the cell's visible width no longer matches what the writer emits. Promote to the quoted form only when the cell string contains a layout-breaking control character; plain strings still render without quotes.
- trim multi-line bug history to a single issue reference - move exitCode assertion out of runTable so callers check stdout first, matching Bun's test convention
The prior commit accidentally reverted src/bun.js/ConsoleObject.zig while cleaning up the test file — restore shouldQuoteStringCell and both of its call sites, and broaden the check from just \n/\r/\t to every C0 control character (0x00–0x1F). \v and \f move the cursor in real terminals the same way \n does; \0 and the rest are counted as zero visible width but emitted as literal bytes, which still mismatches the column width calculation. Test changes: - drain stderr in runTable so a future caller emitting >64KB of stderr doesn't deadlock on the pipe buffer - describe.concurrent so the 10 subprocess spawns run in parallel - drop the tautological dataRows.length==2 assertion (it was true in both the fixed and unfixed states — a leaked \n splits one row into two lines but only the first starts with the │ border) - add a coverage case for \v, \f, and NUL
Three fixes from review:
1. Exclude 0x1B (ESC) from the C0 range check. Before: chalk.red('FAIL')
output was JSON-escaped as '\u001b[31mFAIL\u001b[0m', destroying
terminal colors and massively oversizing the column. ANSI sequences
are already stripped from visible-width measurement by
VisibleCharacterCounter (exclude_ansi_colors), so raw ESC and the
width calculation agree — same rationale as 0x7F. Add a regression
test for chalk-style output.
2. Avoid calling shouldQuoteStringCell twice per cell in printRow. The
helper does bun.String.fromJS + a byte scan, and was running once
inside getWidthForValue and again directly. Split into
getWidthForValueWithTag (takes the already-decided flag) with a thin
wrapper that keeps the old signature for updateColumnsForRow.
3. Add a positive toContain assertion to the C0 control-char test —
it previously only had a negative not.toMatch which would pass even
if the cell value were silently dropped.
Addresses review note about the index column (column 0) using the old
unprotected path: width was measured via visibleWidthExcludeANSIColors
(counts \n as zero width) and output via writer.print({f}), which
emits raw bytes. console.table({ ['a\nb']: 1 }) still produced a
literal newline in the index column after the initial fix, breaking
the row border in exactly the same way as the original #29082 bug
did for data cells.
Extract the shared C0 control-char scan into stringHasLayoutBreakingControlChar
(bun.String) and reuse it in three new helpers:
- rowKeyWidth: width of a RowKey, accounting for JSON-quoted form
- jsonQuotedStringWidth: width of a bun.String when rendered via
writeJSONString, measured through VisibleCharacterCounter so it
matches the cell-width measurement path
- writeRowKey: render a RowKey, calling writeJSONString for string
keys with layout-breaking chars and falling back to raw print
otherwise
updateColumnsForRow and printRow now route the index column through
these helpers. Add a regression test for the object-key case.
Same pattern as the data-cell dedup from the previous commit: printRow was calling rowKeyWidth + writeRowKey, each of which independently scanned the bun.String for layout-breaking control chars. Split into rowKeyWithQuote / writeRowKeyWithQuote variants that take the flag from the caller, plus thin wrappers that resolve it internally for the first-pass width computation in updateColumnsForRow. Also extracted the UTF-8 / UTF-16 writeJSONString dispatch into a shared writeQuotedBunString helper since both jsonQuotedStringWidth and writeRowKeyWithQuote need it.
ecdf328 to
49b78a4
Compare
There was a problem hiding this comment.
All prior feedback has been addressed and I didn't find anything new, but the final Zig changes are non-trivial (~140 LOC of new TablePrinter helpers, including the UTF-16 @ptrCast path in writeQuotedBunString), so this is worth a human pass before merge.
Extended reasoning...
Overview
This PR fixes #29082 by promoting console.table string cells (and index-column keys) that contain C0 control characters to quoted/JSON-escaped form so the table stays rectangular. It touches src/jsc/ConsoleObject.zig (~140 net new lines across TablePrinter: shouldQuoteStringCell, stringHasLayoutBreakingControlChar, getWidthForValueWithTag, rowKeyWidth[WithQuote], jsonQuotedStringWidth, writeQuotedBunString, writeRowKeyWithQuote) and adds a 12-test regression suite at test/regression/issue/29082.test.ts.
Security risks
None. This is purely output-formatting logic for console.table; no auth, crypto, parsing of untrusted input into privileged state, or filesystem/network access. The only pointer-level operation is a @ptrCast of a []const u16 slice to [*]const u8 for writeJSONString(.utf16), which is a read-only reinterpretation with an explicitly computed length (len * 2).
Level of scrutiny
Medium. console.table is a debug utility, not a hot or security-critical path, so the blast radius of a regression is cosmetic. However, the change is not mechanical: it introduces seven new helpers, restructures the width/render flow for both data cells and the index column, makes a deliberate design carve-out for ESC (0x1B) to preserve ANSI colors, and adds a UTF-16 byte-reinterpretation branch. These are the kinds of changes that benefit from a quick human read of the Zig, particularly writeQuotedBunString and the width/render symmetry between getWidthForValueWithTag and rowKeyWidthWithQuote.
Other factors
The PR went through five rounds of bot review and the author addressed every point: stderr draining, exit-code assertion ordering, describe.concurrent, an accidental revert of the Zig fix, the ESC/ANSI regression, double-scan dedup for both data cells and row keys, missing positive assertions, and extending the fix to the index column. One pre-existing gap (column header names with control chars) was explicitly and reasonably scoped out for a follow-up. CI failures reported by robobun (webview-chrome.test.ts, fetch.upgrade.test.ts) are unrelated to this change. Given the accumulated complexity and the number of iterations it took to converge, I'm deferring rather than auto-approving so a human can confirm the final shape of the Zig helpers.
Per the repo convention (test/CLAUDE.md, root CLAUDE.md), test/regression/issue/N.test.ts is reserved for true regressions — behavior that worked in a previous release and then broke. console.table never escaped control characters before this PR (quote_strings was unconditionally false for string cells), so this is never-correct behavior, not a regression, and belongs in the module's existing feature test file. Append the 12 cases as a new describe.concurrent block in test/js/bun/console/console-table.test.ts, reusing that file's existing runTable subprocess helper so subprocess startup stays parallelized. Keep the discriminating assertRectangular + toContain pattern — they pinpoint the escaping regression without depending on snapshot diffs.
|
bang @Jarred-Sumner dkk, naikin ini dong |
|
Superseded by #32224, which applies the same control-character escaping in the current Rust `TablePrinter` (this PR targets `src/jsc/ConsoleObject.zig`, which was removed when the runtime was ported to Rust). #32224 also covers the ANSI case from #32223 and carries a regression test for the newline/tab cases from #29082. |
|
Closing: this PR's implementation lives entirely in Zig source files that have since been removed from the tree as part of the Rust migration. The change can no longer merge cleanly and the files it edits no longer exist on If the underlying issue is still present, it will need a fresh fix against the Rust implementation. |
2 participants
Fixes #29082.
Repro
The embedded
\nlands in the middle of the row and breaks the table border.Cause
TablePrinter.printRow/getWidthForValueunconditionally setquote_strings = falsefor string cells — Bun's stylistic choice is to renderplain strings unquoted in
console.table. That's fine for ordinary strings,but raw
\n/\r/\tbreak the fixed-width layout: the cell's visiblewidth no longer matches what the writer emits.
Fix
src/bun.js/ConsoleObject.zig: addshouldQuoteStringCell— when a stringcell contains
\n,\r, or\t, promote it to the quoted JSON-escaped form(matching Node.js'
'Hello\\nWorld'). Plain strings still render withoutquotes, preserving Bun's existing style:
The same check runs in both
getWidthForValue(column sizing) andprintRow(actual render), so the width calculation and the output stayin sync.
Verification
bun bd test test/regression/issue/29082.test.ts→ 9 passUSE_SYSTEM_BUN=1 bun test test/regression/issue/29082.test.ts→ 8 failpropertiesarg,mixed newline + plain rows — all render with a rectangular table.