Avoid unnecessary allocation in stripANSI for false-positive escape bytes

[robobun]

Problem

stripANSI allocates a buffer and returns a String copy when input contains only false-positive escape bytes (e.g. standalone C1 ST 0x9C), violating the optimization contract that says "for no-escape input we return std::nullopt and the caller reuses the original JSString with zero copies."

When all escape-like bytes are false positives and the last one is at the end of input, the while loop exits via start == end without reaching the !foundANSI guard inside the if (!escPos) branch. Execution falls through to String::adopt(), returning an unnecessary copy.

Additionally, Bun__ANSI__next (the C iterator used by the REPL clipboard) silently drops standalone 0x9C bytes: the false-positive branch did start++; break;, advancing past the byte so it never appeared in any yielded content slice.

Fix

1. Add if (!foundANSI) return std::nullopt; after the while loop in stripANSI, catching the case where the loop exits via start == end with only false positives.
2. Change start++; break; to break; in Bun__ANSI__next so false-positive bytes fall into the next content slice instead of being dropped.
3. Cache Bun.stripANSI before the heapStats baseline in the test to match the established pattern.

Verification

• USE_SYSTEM_BUN=1 bun test test/js/bun/util/stripANSI.test.ts -t "false-positive" → FAIL (heapStats +1)
• bun bd test test/js/bun/util/stripANSI.test.ts → 266 pass, 0 fail

[robobun]

^{Updated 2:11 AM PT - May 7th, 2026}

❌ @robobun, your commit e576479 has some failures in Build #52530 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 28772

That installs a local version of the PR into your bun-28772 executable, so you can run:

bun-28772 --bun

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Return std::nullopt from stripANSI when the scan loop ends without detecting ANSI sequences. Preserve bytes that triggered broad-mask false positives by not advancing the start pointer, and re-scan escape candidates to include such bytes in output. Add a unit test for a trailing lone C1 ST (0x9C).

Changes

Cohort / File(s)	Summary
stripANSI implementation src/bun.js/bindings/stripANSI.cpp	Return std::nullopt if main scan finishes (start == end) with no ANSI found. In the initial skip loop, when consumeANSI returns after == start, break without incrementing start so the byte is preserved. If findEscapeCharacter(start, end) returns start, re-search from start + 1 (or use end) to avoid dropping a false-positive byte.
Tests test/js/bun/util/stripANSI.test.ts	Add test for input ending with a lone C1 ST ("\x9C"): assert Bun.stripANSI(input) returns the same string instance and string heap object counts do not change.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and specifically summarizes the main change: avoiding unnecessary allocation in stripANSI when false-positive escape bytes are encountered.
Description check	✅ Passed	The description includes both required sections with clear explanations of the problem, the fix, and verification steps, though the verification section could be more detailed.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

LGTM — all previously raised issues have been addressed.

Extended reasoning...

Overview

This PR modifies src/bun.js/bindings/stripANSI.cpp and its test file to fix two related bugs around false-positive ANSI escape bytes (0x9C): (1) stripANSI unnecessarily allocated a buffer when the while loop exited via start == end after only false positives, and (2) Bun__ANSI__next silently dropped 0x9C bytes by advancing start++ before breaking.

Security risks

None. The changes are purely in string-processing logic with no security-sensitive surfaces.

Level of scrutiny

Low-to-medium. The fix is targeted and the logic is straightforward: a post-loop !foundANSI guard in stripANSI, and a post-skip-loop escPos == start re-scan in Bun__ANSI__next that includes the false-positive byte in the content slice while still advancing the cursor to avoid an infinite loop.

Previous review iterations

Three rounds of prior review feedback were all addressed: (1) test input was corrected to "hello\x9C" to exercise the new post-loop guard, (2) Bun.stripANSI is now cached before the heap baseline, and (3) the Bun__ANSI__next fix correctly avoids both the data-loss and the infinite loop. The remaining bug report is a minor performance nit (consecutive 0x9C bytes cause one extra ArrayList allocation) with no correctness impact and an essentially impossible real-world occurrence.

[claude]

🟡 The copyToClipboardOSC52 fast path (first.len == text.len) is bypassed for inputs starting with 2+ consecutive \x9C bytes: the new escPos == start re-scan at lines 133-134 immediately finds the second \x9C at start+1, yielding only a 1-byte slice on the first iterator call, so first.len != text.len triggers unnecessary ArrayList allocation even though no real ANSI sequences are present. Output is always correct; this is a minor performance nit affecting an extremely rare input pattern.

Extended reasoning...

Fast path bypassed for consecutive false-positive bytes

What the bug is and how it manifests

The function in uses a heuristic to detect no ANSI sequences stripped and take a zero-copy fast path, avoiding an ArrayList allocation. With the new code in , inputs starting with 2+ consecutive bytes cause the first iterator call to return only a 1-byte slice, defeating this heuristic and triggering the slow path (ArrayList allocation) even though no real ANSI sequences exist in the input.

The specific code path that triggers it

At stripANSI.cpp:133-134, after the skip-loop breaks without advancing (because -- false positive), the post-loop content scan runs . For input , this returns (first ), triggering the branch. immediately returns (the second ), so and . The first call to yields only the 1-byte slice rather than the full string.

Why existing code does not prevent it

explicitly matches in both its SIMD exact mask and the scalar fallback. This means every consecutive byte acts as a new escape candidate boundary, causing the re-scan from to stop immediately at the next . The PR design for avoiding infinite loops (break without advancing + re-scan from start+1) has the side effect of fragmenting slices whenever two false-positive bytes are adjacent.

Step-by-step proof for (6 bytes)

1. Call 1 (cursor=0, start points to first ): skip-loop fires findEscapeCharacter -> returns start; consumeANSI returns start (false positive); break without advancing. Post-loop: findEscapeCharacter(start, end) = start; escPos == start guard fires; findEscapeCharacter(start+1, end) = pointer to second . slice_end = start+1, slice_len = 1. Returns slice (1 byte), cursor=1.
2. In copyToClipboardOSC52: first.len = 1 != text.len = 6 -> slow path fires, allocates ArrayList clean.
3. Subsequent calls yield (1 byte) then (4 bytes); appended to clean.
4. clean = (correct). Encoded for clipboard with unnecessary allocation.

For comparison, a single false-positive (5 bytes): findEscapeCharacter(start+1, end) returns nullptr (no more escapes in "text"), so slice_end = end, slice_len = 5, first.len = 5 == text.len = 5 -- fast path fires correctly.

Addressing the refutations

One refutation correctly notes that the old code also triggered the slow path for (first call yielded an empty 0-byte slice, also failing first.len == text.len). This is accurate -- there is no performance regression vs pre-PR behavior for this specific case, and the new code is strictly better overall (correct output with slow path, vs corrupted output with slow path). The nit is that the re-scan design leaves an optimization gap for consecutive false-positives that could theoretically be closed with a different approach (e.g., scanning past all consecutive false-positive bytes before emitting a slice). Given that consecutive � bytes are essentially impossible in real terminal output, and the slow path allocates only a small transient buffer proportional to text.len, this is purely an academic concern about the fast-path heuristic not firing in an edge case.

[claude]

LGTM — all previously flagged issues have been addressed.

Extended reasoning...

Overview

This PR touches two functions in src/bun.js/bindings/stripANSI.cpp: the JS-facing stripANSI template and the C iterator Bun__ANSI__next. The test file gains one new test case.

Security Risks

No security-sensitive code is involved. The changes are pure string-processing logic with no I/O, no authentication, and no memory-safety concerns beyond the already-audited WTF::Vector and memcpy patterns.

Level of Scrutiny

This required multiple review rounds due to subtle edge-case interactions (false-positive escape bytes, infinite-loop risk, iterator consistency). After those rounds, all critical issues are resolved: the post-loop !foundANSI guard correctly short-circuits for strings ending with false-positive bytes; the Bun__ANSI__next fix avoids both the data-loss bug (dropped bytes) and the infinite-loop risk (via the escPos == start re-scan from start + 1); and the new test exercises the exact code path with the correct input and caching order.

Other Factors

The one remaining nit — the WTF::Vector buffer is still heap-allocated before the !foundANSI guard fires — is a minor C++ performance cost on an extremely rare input pattern (strings ending with standalone 0x9C). It does not affect correctness or the JS-observable behavior. The PR description and inline documentation are clear about the intent of each change.

[claude]

🟡 The behavioral changes to Bun__ANSI__next (preserving standalone 0x9C bytes via break without advancing, plus the escPos == start re-scan to maintain liveness) have no test coverage — the new test only exercises Bun.stripANSI, not the C iterator. Given that an earlier iteration of this exact fix introduced an infinite loop caught only by manual review, it would be worth adding a test that drives ANSIIterator over input containing standalone 0x9C and asserts (a) termination and (b) that the yielded slices concatenate back to the original input.

Extended reasoning...

What the gap is

This PR makes two non-trivial behavioral changes to Bun__ANSI__next in src/jsc/bindings/stripANSI.cpp:

1. At line ~128, start++; break; becomes break; so that broad-mask false-positive bytes (standalone 0x9C) are no longer silently dropped.
2. At lines 143–148, an escPos == start guard is added that re-scans from start + 1 so the iterator does not infinite-loop on the byte it just declined to advance past.

Neither change is exercised by any test. grep confirms that ANSIIterator, Bun__ANSI__next, and copyToClipboardOSC52 are referenced only in src/string/immutable.zig, src/jsc/bindings/stripANSI.cpp, and src/cli/repl.zig — there are zero hits in test/. The only new test in this PR (stripANSI.test.ts:17–28) calls Bun.stripANSI, which goes through the templated stripANSI<Char> function — a completely separate code path from the C iterator.

Why this matters here specifically

The PR timeline shows that this exact code went through a broken intermediate state during this review cycle: the first attempt at change (1) — removing start++ without adding the re-scan — introduced an infinite loop where Bun__ANSI__next would forever return true with a zero-length slice at an unchanging cursor, hanging copyToClipboardOSC52. That regression was caught only by a manual review comment, not by a failing test. The fix (the re-scan at lines 143–148) is itself subtle enough that a separate nit was filed about it fragmenting slices for consecutive 0x9C bytes. Code that has already regressed once within a single PR cycle is a strong signal that it needs a regression test.

Why existing coverage does not help

The Bun.stripANSI test suite is comprehensive for the templated path, but Bun__ANSI__next shares only findEscapeCharacter/consumeANSI with it — the loop structure, cursor bookkeeping, and slice-boundary logic are entirely independent. A bug in the iterator's liveness or byte-preservation would not be detected by any existing test; the infinite-loop regression in this PR is direct proof of that.

Step-by-step: what a regression test would have caught

For input "\x9C" against the intermediate (broken) commit:

1. Call 1, cursor=0: skip-loop finds escPos == start, consumeANSI returns start, break without advancing. Post-loop findEscapeCharacter(start, end) returns start again → slice_len = 0, cursor = 0, returns true.
2. Call 2, cursor=0: identical state → identical result. The Zig while (it.next()) |slice| loop in copyToClipboardOSC52 never terminates.

A test that simply iterates strings.ANSIIterator over "a\x9Cb" and asserts the concatenated slices equal "a\x9Cb" would have (a) hung on the broken commit and (b) failed on the pre-PR code (which yielded "a" ++ "b", dropping the byte).

Suggested fix

Add a Zig test (e.g. in src/string/immutable.zig alongside ANSIIterator, or a small case in the bindings tests) that iterates over inputs like "\x9C", "a\x9Cb", and "\x9C\x9C", concatenates the yielded slices, and asserts the result equals the input. This pins down both the termination guarantee and the byte-preservation guarantee that this PR establishes.

This is nit severity — the current implementation traces correctly by hand and all verifiers agree it is sound; this is purely a coverage gap, not a correctness bug, and need not block the PR.

[robobun]

Closing: this was superseded by #33488 (Consolidate the ANSI escape grammar across stringWidth, stripANSI, sliceAnsi and wrapAnsi), which landed on main with the same fix this PR proposed.

Specifically, main now has:

• The post-loop if (!foundANSI) return std::nullopt; guard in stripANSI (src/jsc/bindings/stripANSI.cpp), so inputs containing only false-positive escape bytes (e.g. a trailing standalone C1 ST 0x9C) return the original JSString with zero copies.
• Test coverage in test/js/bun/util/stripANSI.test.ts for standalone 0x9C (trailing, mid-string, and across the dispatch threshold) asserting same-object return and no string heap growth, which strictly covers the test added here.

The merge conflict from the grammar consolidation is unresolvable in a useful way: with the fix already on main, this PR's test passes with and without its diff, so there is nothing left for it to prove.