Repository navigation
Bun.wrapAnsi: don't re-open SGR close codes after line breaks - #34376
Conversation
joinRowsWithAnsiPreservation gated the close-before-newline emission on getCloseCode() but not the reopen-after-newline emission, so close codes (22-29, 49) and unknown codes were stored as the active style and re-emitted at the start of every wrapped line. npm wrap-ansi gates both sides on ansiStyles.codes.get(code), which is undefined for close/unknown codes. Gate the reopen on the same getCloseCode() check.
|
Reproduced with CI on 005c007 (build 74009): all 74 individual lane checks pass and the wrapAnsi tests are green on every platform. The aggregate is red only because one |
|
Updated 1:12 PM PT - Jul 16th, 2026
❌ @robobun, your commit 005c007 has some failures in 🧪 To try this PR locally: bunx bun-pr 34376That installs a local version of the PR into your bun-34376 --bun |
WalkthroughChangesANSI SGR restoration
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line symmetry fix making the reopen-after-newline gate match the existing close-before-newline gate.
What was reviewed:
- Verified
getCloseCode()returnsnulloptfor 22-29/49/200 and a value for 1-9/30-37/40-47/90-97/100-107, so the new condition partitions codes exactly as the tests assert. - Checked the close-before-newline block already uses the same
getCloseCode()gate — this change just brings the reopen block in line, matching npm wrap-ansi'sansiStyles.codes.get()gating on both sides. - Traced the third test (
\x1b[42mab\x1b[49mcd efat width 4): 49 overwrites 42 inescapeCode, has no close mapping, so neither close nor reopen is emitted around the break.
Extended reasoning...
Overview
Single-line change to src/jsc/bindings/wrapAnsi.cpp in joinRowsWithAnsiPreservation: the reopen-after-newline branch now checks escapeCode && getCloseCode(*escapeCode) instead of just escapeCode. This mirrors the close-before-newline branch immediately above it, which already gates on getCloseCode(). Three new test blocks in wrapAnsi.test.ts cover close codes (22-29, 49), unknown codes (200), the existing 39/0 reset path, open codes that must still reopen, and a close-following-open sequence.
Security risks
None. Pure output-formatting logic on already-parsed SGR codes; no allocation, indexing, or external input handling changes.
Level of scrutiny
Low. The fix is mechanical symmetry — the close block and reopen block are meant to be paired (close style before \n, reopen after), and the close block was already correctly gated on getCloseCode(). The bug was simply that the reopen block wasn't. The PR description correctly cites npm wrap-ansi's equivalent gating (ansiStyles.codes.get(escapeCode) is undefined for close/unknown codes on both sides). getCloseCode() is a pure switch/range lookup with no side effects, so calling it twice is safe.
Other factors
Tests are well-constructed: test.each over the full close-code set plus an unknown code, a positive test.each confirming open codes still close-and-reopen with the correct close code, and a mixed open→close case verifying the close code overwrites the tracked state and is then dropped. robobun confirmed 10 new cases fail under USE_SYSTEM_BUN=1 and all 166 + 23 npm-ported tests pass under the debug build. No memory-safety, GC, or exception-scope surface is touched.
There was a problem hiding this comment.
LGTM — the reopen gate is now symmetric with the close-before-newline gate, matching npm wrap-ansi's ansiStyles.codes.get() check. The inline nit about SGR 53 (overline) is a pre-existing gap in the local getCloseCode() table, not a defect in this PR's logic — worth a one-line follow-up but not blocking.
Extended reasoning...
Overview
One-line change in src/jsc/bindings/wrapAnsi.cpp: the reopen-after-newline block in joinRowsWithAnsiPreservation now gates on escapeCode && getCloseCode(*escapeCode) instead of just escapeCode, making it symmetric with the close-before-newline block ~15 lines above that already used the same guard. 24 lines of new test.each cases in wrapAnsi.test.ts cover close codes (22-29, 49), unknown codes (200), the 39/0 controls, representative open codes, and a close-following-open sequence.
Security risks
None. Pure string-formatting logic on already-parsed SGR codes; no allocation, no user-controlled sizing, no I/O.
Level of scrutiny
Low. This is a cosmetic-output utility (Bun.wrapAnsi) where the fix mirrors an existing guard on the sibling branch, and the reference implementation (npm wrap-ansi@9) gates both sides on the same predicate. The change is mechanical symmetry with a well-understood rationale.
Other factors
- Tests fail on main (10 cases) and pass with the fix, per the PR's evidence block and robobun's confirmation.
- The one finding (SGR 53 / overline) is a pre-existing drift between the local
getCloseCode()andANSIHelpers.h— the close-before-newline block already had this gap before the PR. The PR moves overline from "reopened but not closed" to "neither closed nor reopened"; both diverge from npm, and in a continuous terminal stream the visual result is identical since SGR state persists across\n. Marked as a nit; a one-linecase 53: return 55;addition would fully close it but isn't required to land this fix. - No prior reviews from me on this PR.
… wraps ansi-styles@6 maps 53->55; without it the reopen gate now drops overline at line breaks instead of closing and reopening it like npm wrap-ansi.
There was a problem hiding this comment.
LGTM — the SGR 53 (overline) mapping was added in 005c007 and both test.each blocks now cover it, so the reopen gate matches ansi-styles' open-code set exactly.
Checked that the close-before-newline and reopen-after-newline branches now use the same getCloseCode() predicate, so behavior is symmetric.
Verified the local getCloseCode() table now matches ansi-styles@6's codes map (1-9, 30-37, 40-47, 53, 90-97, 100-107); no other open codes are missing.
The new close/unknown test.each covers 22-29/49/55/39/0/200 and the open-then-close case, all with exact-string toBe assertions.
Extended reasoning...
Overview
Two-file change to Bun.wrapAnsi: a one-line guard in joinRowsWithAnsiPreservation (src/jsc/bindings/wrapAnsi.cpp) so the reopen-after-newline block only re-emits SGR codes that have a known close mapping, plus case 53: return 55; added to the local getCloseCode() switch. test/js/bun/util/wrapAnsi.test.ts gains three test.each-driven cases covering close codes (22-29, 49, 55), the 39/0 controls, an unknown code (200), representative open codes (1, 4, 31, 42, 53, 100), and an open-followed-by-close sequence.
Security risks
None. Pure output-formatting change on a string utility; no I/O, allocation sizing, or user-controlled indexing was touched. getCloseCode() is a pure lookup on a uint32_t.
Level of scrutiny
Low. The functional diff is a single added && getCloseCode(*escapeCode) conjunct that mirrors the guard already present on the sibling close-before-newline branch three lines above, plus one switch case. The reference implementation (npm wrap-ansi@9 gating both sides on ansiStyles.codes.get(escapeCode)) is cited in the PR body and the new behavior byte-matches it.
Other factors
My earlier review on this PR flagged that the new gate would regress SGR 53 (overline) because the local getCloseCode() was missing the 53 → 55 entry that both ansi-styles@6 and Bun's own ANSIHelpers.h include. Commit 005c007 added the case and extended both test tables to cover 53/55; the thread is resolved. I re-checked the local table against ansi-styles' modifier/color/bgColor maps — 53 was the only gap, so the table is now complete. The PR's evidence block shows the new tests fail on the pre-fix build and pass on both debug+ASAN and release with the fix. No outstanding reviewer comments remain.
2 participants
Repro
Before:
npm
wrap-ansi@9:Cause
joinRowsWithAnsiPreservationtracks the most recent single-number SGR code and closes/reopens it around each inserted newline. The close-before-newline block already gates ongetCloseCode(*escapeCode)(only emit a close if the stored code is a known open code), but the reopen-after-newline block only checkedif (escapeCode), so a close code like 49 or 22 (stored because it is neither 39 nor 0) was re-emitted at the start of every subsequent wrapped line. npmwrap-ansigates both sides onansiStyles.codes.get(escapeCode), which isundefinedfor close and unknown codes.Fix
Gate the reopen on the same
getCloseCode()check as the close block. Close codes (22-29, 49) and unknown codes now pass through once without being carried as an active style; open codes (1-9, 30-37, 40-47, 90-97, 100-107) continue to be closed and reopened across line breaks as before.Verification
New
test.eachcases intest/js/bun/util/wrapAnsi.test.tscover every close code, an unknown code, the existing 39/0 controls, and the open-code set. 10 of the new cases fail withUSE_SYSTEM_BUN=1and all 166 tests in the file plus the 23 ported npm tests pass with the fix.[stamp-90s] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file