feat: implement Bun.sliceAnsi for ANSI & grapheme-aware string slicing - #26963
Conversation
|
🤖 Claude is monitoring this PR I'll review code, respond to comments, and investigate CI failures.
|
|
Updated 3:46 AM PT - Feb 27th, 2026
❌ @autofix-ci[bot], your commit 108775e has 4 failures in
🧪 To try this PR locally: bunx bun-pr 26963That installs a local version of the PR into your bun-26963 --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 Bun.sliceAnsi: a JSC host binding and implementation for ANSI- and grapheme-aware visible-width string slicing, plus supporting Zig grapheme/emoji helpers, TypeScript typings, comprehensive tests, ANSI helper utilities, and a large batch of new C++ source files across bindings, crypto, WebCore/WebCrypto, Node, platform, and VM utilities. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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: 6
🤖 Fix all issues with AI agents
In `@src/bun.js/bindings/BunObject.cpp`:
- Around line 80-84: Replace the manual declaration of jsFunctionBunSliceAnsi in
BunObject.cpp with an `#include` of the existing header that defines it (include
"sliceAnsi.h") so the function signature comes from a single source; remove the
JSC_DECLARE_HOST_FUNCTION(jsFunctionBunSliceAnsi) line and any duplicate
forward-declaration for jsFunctionBunSliceAnsi to avoid mismatches, leaving the
other declarations (jsFunctionBunStripANSI, jsFunctionBunWrapAnsi) intact.
In `@src/bun.js/bindings/sliceAnsi.cpp`:
- Around line 127-183: sgrCloseCode currently omits blink start codes so blink
(5 and 6) never maps to the blink-reset 25, causing over-close behavior; update
sgrCloseCode to handle openCode values 5 and 6 and return 25 so that blink is
properly closed, and retain isSgrEndCode (which already lists 25) so the
end-code detection matches the mapping in sgrCloseCode.
- Around line 1057-1114: Add a TypeScript declaration for the new Bun.sliceAnsi
function in the bun.d.ts typings near the existing stripANSI declaration:
declare a function named sliceAnsi(input: string, start?: number, end?: number):
string with the provided JSDoc comment (summary, params, returns, example) so
IDEs and TS consumers see the correct signature; ensure the function name is
exported/available the same way stripANSI is declared.
- Around line 1061-1102: The cast from double returned by toIntegerOrInfinity()
to int64_t in jsFunctionBunSliceAnsi (for startIdx and endIdx) can overflow and
invoke undefined behavior; replace the direct static_casts of d with a safe
clamp: when d is finite, if d > std::numeric_limits<int64_t>::max() set the
index to INT64_MAX, if d < std::numeric_limits<int64_t>::min() set it to
INT64_MIN, otherwise cast to int64_t; ensure this clamping is applied for both
the startIdx and endIdx assignment paths after calling toIntegerOrInfinity().
In `@src/string/immutable/visible.zig`:
- Around line 1177-1194: Bun__graphemeBreak currently converts a raw u8 from
state_ptr into grapheme.BreakState via `@enumFromInt` which can produce invalid
enum tags; before calling `@enumFromInt` in Bun__graphemeBreak validate or clamp
the byte (state_ptr.*) to the valid BreakState range (0..=4) and only then
convert, and after calling grapheme.graphemeBreak write back the validated enum
value to state_ptr; for Bun__isEmojiPresentation either rename the function to
Bun__isEmoji or update its doc comment to state that it checks UCHAR_EMOJI
(property 57) via icu_hasBinaryProperty(cp, 57) (not Emoji_Presentation), so
callers and maintainers are not misled about its semantics.
In `@test/js/bun/util/sliceAnsi.test.ts`:
- Around line 14-58: stripOscHyperlinks currently stops parsing and returns the
truncated output when it encounters malformed OSC sequences (e.g., uriStart ===
-1 or sequenceIndex reaches string.length), which drops visible text; instead,
modify stripOscHyperlinks (and its handling of
hyperlinkPrefixes/ESCAPE/ANSI_BELL/C1_STRING_TERMINATOR) to append the remainder
of the input verbatim when a sequence is malformed (or treat the detected prefix
as literal and resume scanning) so visible text is preserved for
stripForVisibleComparison; update the branches where uriStart === -1 and where
sequenceIndex >= string.length (and any early breaks when scanning
sequenceIndex) to push the unconsumed substring onto output and then
break/return accordingly.
| namespace Bun { | ||
| JSC_DECLARE_HOST_FUNCTION(jsFunctionBunStripANSI); | ||
| JSC_DECLARE_HOST_FUNCTION(jsFunctionBunWrapAnsi); | ||
| JSC_DECLARE_HOST_FUNCTION(jsFunctionBunSliceAnsi); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer including sliceAnsi.h instead of redeclaring jsFunctionBunSliceAnsi here.
Now that src/bun.js/bindings/sliceAnsi.h exists, it’s safer to include it and avoid a parallel declaration in BunObject.cpp (prevents signature mismatches during future edits). The LUT entry (Function 3) looks correct for (string, start, end).
Suggested change
@@
`#include` "Secrets.h"
+// Host function declarations
+#include "sliceAnsi.h"
@@
namespace Bun {
JSC_DECLARE_HOST_FUNCTION(jsFunctionBunStripANSI);
JSC_DECLARE_HOST_FUNCTION(jsFunctionBunWrapAnsi);
-JSC_DECLARE_HOST_FUNCTION(jsFunctionBunSliceAnsi);
}Also applies to: 992-993
🤖 Prompt for AI Agents
In `@src/bun.js/bindings/BunObject.cpp` around lines 80 - 84, Replace the manual
declaration of jsFunctionBunSliceAnsi in BunObject.cpp with an `#include` of the
existing header that defines it (include "sliceAnsi.h") so the function
signature comes from a single source; remove the
JSC_DECLARE_HOST_FUNCTION(jsFunctionBunSliceAnsi) line and any duplicate
forward-declaration for jsFunctionBunSliceAnsi to avoid mismatches, leaving the
other declarations (jsFunctionBunStripANSI, jsFunctionBunWrapAnsi) intact.
| /// Grapheme break detection for C++ callers. | ||
| /// Returns true if there should be a grapheme break between cp1 and cp2. | ||
| /// `state` is an opaque u8 that must be initialized to 0 and passed between calls. | ||
| export fn Bun__graphemeBreak(cp1: u32, cp2: u32, state_ptr: *u8) bool { | ||
| var state: grapheme.BreakState = @enumFromInt(state_ptr.*); | ||
| const result = grapheme.graphemeBreak(@truncate(cp1), @truncate(cp2), &state); | ||
| state_ptr.* = @intFromEnum(state); | ||
| return result; | ||
| } | ||
|
|
||
| /// Check if a codepoint has the Emoji property (using ICU). | ||
| export fn Bun__isEmojiPresentation(cp: u32) bool { | ||
| if (cp < 0x203C) return false; | ||
| if (cp >= 0x2C00 and cp < 0x1F000) return false; | ||
| if (cp == 0xFE0E or cp == 0xFE0F or cp == 0x200D) return false; | ||
| // UCHAR_EMOJI = 57 | ||
| return icu_hasBinaryProperty(cp, 57); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the grapheme.zig file to see BreakState enum definition
find . -name "grapheme.zig" -type fRepository: oven-sh/bun
Length of output: 207
🏁 Script executed:
# Search for BreakState enum definition
rg "BreakState" --type zig -A 10Repository: oven-sh/bun
Length of output: 19321
🏁 Script executed:
# Look for ICU property definitions
rg "UCHAR_EMOJI|EMOJI_PRESENTATION|icu_hasBinaryProperty" -A 3 -B 3Repository: oven-sh/bun
Length of output: 2607
🏁 Script executed:
# Check for C++ callers of Bun__graphemeBreak
rg "Bun__graphemeBreak|Bun__isEmojiPresentation" --type cpp --type c -B 2 -A 5Repository: oven-sh/bun
Length of output: 1906
🏁 Script executed:
# Find where ICU property 57 is defined or commented
rg "57" src/string/immutable/visible.zig -B 5 -A 5Repository: oven-sh/bun
Length of output: 6592
🌐 Web query:
ICU UCHAR_EMOJI UCHAR_EMOJI_PRESENTATION property difference meaning
💡 Result:
In ICU, UCHAR_EMOJI and UCHAR_EMOJI_PRESENTATION are two different Unicode (TR51) binary character properties exposed via ICU’s Unicode property APIs. [2]
UCHAR_EMOJI(=\p{Emoji}): “emoji character” — any code point that has the Unicode Emoji property. This is a broad set that includes characters that can participate in emoji presentation and emoji sequences, even if they default to text-style rendering. [3]UCHAR_EMOJI_PRESENTATION(=\p{Emoji_Presentation}): “default emoji presentation character” — the subset ofEmojicharacters that should appear as emoji by default (without needing an emoji-variation selector). [3]
Practical implication (per Unicode): many characters with Emoji=Yes have two variation sequences (VS15 for text style, VS16 for emoji style). If a character has Emoji=Yes but Emoji_Presentation=No, it defaults to text presentation unless you request emoji presentation (typically with VS16). If it has Emoji_Presentation=Yes, it defaults to emoji presentation. [4]
Example idea: characters like © can be Emoji=Yes but Emoji_Presentation=No (text by default), while many “face” emoji are Emoji=Yes and Emoji_Presentation=Yes (emoji by default). [5]
Sources: ICU property list showing both properties [1], ICU/Android NDK ICU docs referencing TR51 [2], TR51 definitions of Emoji vs Emoji_Presentation [3], Unicode Core Spec on emoji variation sequences and default presentation [4], and ICU4X docs illustrating “emoji presentation by default” behavior [5]. [1][2][3][4][5]
🏁 Script executed:
# Check if there's any documentation in the codebase about why Emoji property is used
rg "EMOJI_PRESENTATION|Emoji_Presentation" -i --type zig --type cpp --type c -B 2 -A 2Repository: oven-sh/bun
Length of output: 454
🏁 Script executed:
# Look for comments or documentation about emoji detection choices
rg "emoji|Emoji" src/string/immutable/visible.zig -B 1 -A 1 | head -100Repository: oven-sh/bun
Length of output: 2091
🏁 Script executed:
# Check if there are any issues or TODOs related to emoji property selection
rg "TODO|FIXME|BUG|HACK" src/string/immutable/visible.zig | grep -i emojiRepository: oven-sh/bun
Length of output: 37
Harden Bun__graphemeBreak ABI boundary against out-of-range state + clarify Bun__isEmojiPresentation semantics.
Bun__graphemeBreak accepts state_ptr: *u8 (0–255 range) but constructs grapheme.BreakState (an enum(u3) with only 5 valid values: 0–4) via @enumFromInt. An out-of-range caller value can produce an invalid enum tag, leading to undefined behavior downstream. Although the C++ caller currently initializes and maintains valid state, this exported ABI function should be defensive against misuse.
Additionally, Bun__isEmojiPresentation is named as if it checks UCHAR_EMOJI_PRESENTATION, but it actually checks UCHAR_EMOJI (property 57). In Unicode TR51, these are distinct: Emoji is the broader set of characters that can participate in emoji sequences, while Emoji_Presentation is the subset that should display as emoji by default. The current naming may mislead maintainers or future C++ callers into assuming presentation-default semantics when the actual behavior matches Emoji (used for emoji base/composition detection).
Recommended fixes
For Bun__graphemeBreak, validate the input state before conversion:
export fn Bun__graphemeBreak(cp1: u32, cp2: u32, state_ptr: *u8) bool {
+ // Defensive ABI boundary: BreakState has only 5 valid enum values (0–4).
+ // Reset to default if caller passes an out-of-range value.
+ if (state_ptr.* > 4) state_ptr.* = 0;
+
var state: grapheme.BreakState = `@enumFromInt`(state_ptr.*);
const result = grapheme.graphemeBreak(`@truncate`(cp1), `@truncate`(cp2), &state);
state_ptr.* = `@intFromEnum`(state);
return result;
}For Bun__isEmojiPresentation, either rename to reflect the actual property or update the doc comment to clarify the distinction:
-/// Check if a codepoint has the Emoji property (using ICU).
+/// Check if a codepoint has the Emoji property (property 57, not Emoji_Presentation).
+/// This includes characters that can form emoji sequences, even if they default to text style.
export fn Bun__isEmojiPresentation(cp: u32) bool {
if (cp < 0x203C) return false;
if (cp >= 0x2C00 and cp < 0x1F000) return false;
if (cp == 0xFE0E or cp == 0xFE0F or cp == 0x200D) return false;
- // UCHAR_EMOJI = 57
+ // UCHAR_EMOJI = 57 (not UCHAR_EMOJI_PRESENTATION)
return icu_hasBinaryProperty(cp, 57);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Grapheme break detection for C++ callers. | |
| /// Returns true if there should be a grapheme break between cp1 and cp2. | |
| /// `state` is an opaque u8 that must be initialized to 0 and passed between calls. | |
| export fn Bun__graphemeBreak(cp1: u32, cp2: u32, state_ptr: *u8) bool { | |
| var state: grapheme.BreakState = @enumFromInt(state_ptr.*); | |
| const result = grapheme.graphemeBreak(@truncate(cp1), @truncate(cp2), &state); | |
| state_ptr.* = @intFromEnum(state); | |
| return result; | |
| } | |
| /// Check if a codepoint has the Emoji property (using ICU). | |
| export fn Bun__isEmojiPresentation(cp: u32) bool { | |
| if (cp < 0x203C) return false; | |
| if (cp >= 0x2C00 and cp < 0x1F000) return false; | |
| if (cp == 0xFE0E or cp == 0xFE0F or cp == 0x200D) return false; | |
| // UCHAR_EMOJI = 57 | |
| return icu_hasBinaryProperty(cp, 57); | |
| } | |
| /// Grapheme break detection for C++ callers. | |
| /// Returns true if there should be a grapheme break between cp1 and cp2. | |
| /// `state` is an opaque u8 that must be initialized to 0 and passed between calls. | |
| export fn Bun__graphemeBreak(cp1: u32, cp2: u32, state_ptr: *u8) bool { | |
| // Defensive ABI boundary: BreakState has only 5 valid enum values (0–4). | |
| // Reset to default if caller passes an out-of-range value. | |
| if (state_ptr.* > 4) state_ptr.* = 0; | |
| var state: grapheme.BreakState = `@enumFromInt`(state_ptr.*); | |
| const result = grapheme.graphemeBreak(`@truncate`(cp1), `@truncate`(cp2), &state); | |
| state_ptr.* = `@intFromEnum`(state); | |
| return result; | |
| } | |
| /// Check if a codepoint has the Emoji property (property 57, not Emoji_Presentation). | |
| /// This includes characters that can form emoji sequences, even if they default to text style. | |
| export fn Bun__isEmojiPresentation(cp: u32) bool { | |
| if (cp < 0x203C) return false; | |
| if (cp >= 0x2C00 and cp < 0x1F000) return false; | |
| if (cp == 0xFE0E or cp == 0xFE0F or cp == 0x200D) return false; | |
| // UCHAR_EMOJI = 57 (not UCHAR_EMOJI_PRESENTATION) | |
| return icu_hasBinaryProperty(cp, 57); | |
| } |
🤖 Prompt for AI Agents
In `@src/string/immutable/visible.zig` around lines 1177 - 1194,
Bun__graphemeBreak currently converts a raw u8 from state_ptr into
grapheme.BreakState via `@enumFromInt` which can produce invalid enum tags; before
calling `@enumFromInt` in Bun__graphemeBreak validate or clamp the byte
(state_ptr.*) to the valid BreakState range (0..=4) and only then convert, and
after calling grapheme.graphemeBreak write back the validated enum value to
state_ptr; for Bun__isEmojiPresentation either rename the function to
Bun__isEmoji or update its doc comment to state that it checks UCHAR_EMOJI
(property 57) via icu_hasBinaryProperty(cp, 57) (not Emoji_Presentation), so
callers and maintainers are not misled about its semantics.
| function stripOscHyperlinks(string: string) { | ||
| const hyperlinkPrefixes = [`${ESCAPE}]8;`, `${C1_OSC}8;`]; | ||
| let output = ""; | ||
| let index = 0; | ||
|
|
||
| while (index < string.length) { | ||
| const hyperlinkPrefix = hyperlinkPrefixes.find(prefix => string.startsWith(prefix, index)); | ||
| if (!hyperlinkPrefix) { | ||
| output += string[index]; | ||
| index++; | ||
| continue; | ||
| } | ||
|
|
||
| const uriStart = string.indexOf(";", index + hyperlinkPrefix.length); | ||
| if (uriStart === -1) { | ||
| break; | ||
| } | ||
|
|
||
| let sequenceIndex = uriStart + 1; | ||
| while (sequenceIndex < string.length) { | ||
| if (string[sequenceIndex] === ANSI_BELL) { | ||
| index = sequenceIndex + 1; | ||
| break; | ||
| } | ||
|
|
||
| if (string[sequenceIndex] === ESCAPE && string[sequenceIndex + 1] === "\\") { | ||
| index = sequenceIndex + 2; | ||
| break; | ||
| } | ||
|
|
||
| if (string[sequenceIndex] === C1_STRING_TERMINATOR) { | ||
| index = sequenceIndex + 1; | ||
| break; | ||
| } | ||
|
|
||
| sequenceIndex++; | ||
| } | ||
|
|
||
| if (sequenceIndex >= string.length) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return output; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
stripOscHyperlinks() currently “breaks” on malformed input and drops the remainder (can mask regressions).
In malformed OSC cases, the helper exits and returns output so far, potentially deleting visible text that should remain—making stripForVisibleComparison() comparisons less strict than intended.
If the goal is to avoid false positives, consider appending the remainder verbatim when a sequence is malformed (or treating the prefix as literal and continuing).
🤖 Prompt for AI Agents
In `@test/js/bun/util/sliceAnsi.test.ts` around lines 14 - 58, stripOscHyperlinks
currently stops parsing and returns the truncated output when it encounters
malformed OSC sequences (e.g., uriStart === -1 or sequenceIndex reaches
string.length), which drops visible text; instead, modify stripOscHyperlinks
(and its handling of hyperlinkPrefixes/ESCAPE/ANSI_BELL/C1_STRING_TERMINATOR) to
append the remainder of the input verbatim when a sequence is malformed (or
treat the detected prefix as literal and resume scanning) so visible text is
preserved for stripForVisibleComparison; update the branches where uriStart ===
-1 and where sequenceIndex >= string.length (and any early breaks when scanning
sequenceIndex) to push the unconsumed substring onto output and then
break/return accordingly.
Code ReviewNewest first 🔴
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // - Returns 1 otherwise (default) | ||
| if (nonEmojiWidth >= 2) | ||
| return 2; | ||
| return 1; |
There was a problem hiding this comment.
🔴 Critical: GraphemeWidthState::width() returns 1 for zero-width characters (U+200B, U+200C, etc.) instead of 0, causing incorrect slice boundaries.
The fallback path clamps nonEmojiWidth to a minimum of 1, but the canonical Zig GraphemeState.width() returns the actual accumulated value, including 0. This means Bun.sliceAnsi will compute different column widths than Bun.stringWidth for any string containing zero-width characters.
For example, Bun.sliceAnsi("a\u200Bb", 0, 2) would return "a\u200B" instead of the correct "a\u200Bb".
| return 1; | |
| if (nonEmojiWidth >= 2) | |
| return 2; | |
| return nonEmojiWidth; |
| if (keycap) | ||
| return 2; | ||
| if (regionalIndicator) | ||
| return 2; // Single regional indicator is also width 2 (matching upstream) |
There was a problem hiding this comment.
🟡 Minor: Returns 2 for an unpaired regional indicator, but the Zig GraphemeState.width() returns 1. This creates a width inconsistency between Bun.sliceAnsi and Bun.stringWidth for strings with unpaired regional indicators (e.g., when a flag emoji has been previously truncated).
| return 2; // Single regional indicator is also width 2 (matching upstream) | |
| return 1; // Single regional indicator is width 1 (matching Zig GraphemeState.width()) |
|
No description provided. |
There was a problem hiding this comment.
Both bugs I previously flagged (zero-width width clamping and regional indicator width) are now fixed. No remaining correctness issues found, but this is a ~1276-line new public C++ API with Unicode/ANSI/grapheme complexity and cross-language FFI — worth a human look before merging.
Extended reasoning...
Overview
This PR adds Bun.sliceAnsi(string, start, end) — a new public API for column-width-aware, ANSI-escape-preserving string slicing. It spans 7 files: a 1276-line C++ implementation (sliceAnsi.cpp), a header (sliceAnsi.h), Zig FFI exports for grapheme break and emoji detection (visible.zig), BunObject registration (BunObject.cpp), TypeScript types (bun.d.ts), build config (CxxSources.txt), and a comprehensive 1206-line test suite.
Security risks
No direct security risks. The function processes untrusted string input but operates read-only on JSC string buffers. The UB issue (out-of-range double→int64_t cast) was the closest to a security concern and has been fixed with proper clamping. No file I/O, network, or privilege operations.
Level of scrutiny
High scrutiny warranted. This is production-critical code: a new public API surface that will be used by Bun users for terminal output processing. It involves complex Unicode logic (grapheme clusters, CJK width, emoji sequences), ANSI escape code state machines, and a C++/Zig FFI boundary. The implementation reimplements width calculation logic from Zig in C++, creating a maintenance surface area where the two implementations could diverge.
Other factors
- All previously identified bugs (5 total across CodeRabbit and my reviews) have been addressed in the latest commits
- Remaining unaddressed items are trivial: include header vs redeclare, defensive ABI guard on grapheme break state, and emoji function naming clarification
- The test suite is thorough (130+ tests, 1646 assertions) and covers edge cases well
- The PR is labeled
claude(AI-generated), making human design review especially valuable for API shape decisions - A human should verify the API design choices (function signature, options interface, ellipsis behavior) are consistent with Bun conventions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/js/bun/util/sliceAnsi.test.ts`:
- Around line 1164-1169: The test's "zero-copy" claim is incorrect because
primitive string identity can't be observed with toBe()/Object.is; update the
test in sliceAnsi.test.ts to assert value equality and rename the test to
reflect value preservation (e.g., "no-op slice returns same string value") or,
if you must validate a zero-copy optimization, add proper internal
instrumentation/mocking around Bun.sliceAnsi to verify it returns the original
Buffer/string instance; locate the test using the Bun.sliceAnsi references and
change the test name and assertions accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 16b4c71 and a65b7892ad79cf302d88f689d07e178cc946f833.
📒 Files selected for processing (5)
docs/bundler/executables.mdxpackages/bun-types/bun.d.tssrc/bun.js/bindings/BunObject.cppsrc/bun.js/bindings/sliceAnsi.cpptest/js/bun/util/sliceAnsi.test.ts
| test("no-op slice returns the same string instance (zero-copy)", () => { | ||
| const s = "hello world with no ansi codes"; | ||
| // With start=0 and no end, and no ellipsis, we should get the input back unchanged | ||
| expect(Bun.sliceAnsi(s, 0)).toBe(s); | ||
| expect(Bun.sliceAnsi(s)).toBe(s); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the test file to understand context
cat -n test/js/bun/util/sliceAnsi.test.ts | sed -n '1160,1175p'Repository: oven-sh/bun
Length of output: 866
🌐 Web query:
JavaScript Object.is primitive string equality reference identity
💡 Result:
Object.is(a, b) implements the spec’s SameValue comparison: it does no coercion, treats NaN as equal to itself, and treats +0 and -0 as different. For everything else it’s effectively like ===. [1]
Primitive strings: value equality (not reference identity)
For string primitives, Object.is compares by string value (“same length and same characters in the same order”). There is no “reference identity” concept you can observe for primitives. [1][2]
Object.is("foo", "foo"); // true
Object.is("foo", "bar"); // falseString objects (new String(...)): reference identity
If you compare objects (including new String("foo")), Object.is is true only if both operands are the exact same object reference. Two distinct String objects with the same contents are not the same. [1][2]
Object.is(new String("foo"), new String("foo")); // false (different objects)
const s = new String("foo");
Object.is(s, s); // true (same object)Relationship to ===
Object.is and === behave the same for strings; their key differences are only around NaN and signed zero. [1][3]
🏁 Script executed:
# Check Bun's test framework implementation - look for toBe matcher definition
find . -type f \( -name "*.ts" -o -name "*.js" \) -path "*/test*" | grep -i harness | head -5Repository: oven-sh/bun
Length of output: 183
🏁 Script executed:
# Search for toBe implementation in Bun's test framework
rg "toBe\s*\(" --type ts --type js -A 3 | head -50Repository: oven-sh/bun
Length of output: 2597
🏁 Script executed:
# Check the main test harness file
head -100 test/harness.ts | cat -nRepository: oven-sh/bun
Length of output: 3998
🏁 Script executed:
# Search for toBe matcher implementation in Bun's test implementation
rg "toBe.*=" --type ts -B 2 -A 5 | head -80Repository: oven-sh/bun
Length of output: 5824
🏁 Script executed:
# Check if Bun test uses a standard test framework like Jest/Vitest
rg "expect\(" test/harness.ts -B 3 -A 3Repository: oven-sh/bun
Length of output: 2590
🏁 Script executed:
# Search for Bun's test matcher implementations
rg "toBe" test/harness.ts -B 3 -A 3Repository: oven-sh/bun
Length of output: 2897
🏁 Script executed:
# Check Bun's built-in test module documentation/types
find . -path "*/bun/test*" -name "*.ts" -o -name "*.d.ts" | head -10Repository: oven-sh/bun
Length of output: 602
🏁 Script executed:
# Look for where toBe is defined or documented
fd -e "d.ts" -path "*bun*test*" | head -5Repository: oven-sh/bun
Length of output: 223
🌐 Web query:
Bun test framework toBe matcher implementation Object.is reference identity
💡 Result:
In Bun’s bun:test, expect(received).toBe(expected) is implemented as an identity comparison using Object.is() (see Bun’s matcher list: toBe() → “Identity comparison (Object.is)” and the dedicated toBe.zig matcher file). [1]
Implications (because Object.is is reference identity for objects): [2]
- Objects/arrays/functions: passes only if it’s the exact same instance (same reference), not just same shape/content.
- Numbers edge-cases:
Object.is(NaN, NaN) === true, andObject.is(-0, 0) === false.
So in Bun, toBe = Object.is semantics (reference identity for non-primitives). For deep/structural equality you should use toEqual / toStrictEqual instead. [1]
Sources:
[1] DeepWiki (oven-sh/bun) “Expect API and Matchers” (notes toBe() is “Identity comparison (Object.is)” and points to src/bun.js/test/expect/toBe.zig) (deepwiki.com)
[2] MDN Object.is() reference (same-object/reference rule; NaN and -0 behavior) (developer.mozilla.org)
Citations:
- 1: https://deepwiki.com/oven-sh/bun/6.2-test-runner
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is?utm_source=openai
🏁 Script executed:
# Check if there are any custom matchers for reference identity in the test harness
rg "toBeIdentical|toBeSame|toBeReference" test/Repository: oven-sh/bun
Length of output: 37
Test comment "zero-copy" cannot be validated with toBe() for primitive strings.
For primitive strings, Object.is() (which Bun's toBe() uses) compares by value equality only—there is no observable reference identity concept. The test asserts value preservation, not that the same string instance is returned. Either rename the test to reflect value preservation, or use internal instrumentation to verify reference identity if that's truly a critical optimization path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/util/sliceAnsi.test.ts` around lines 1164 - 1169, The test's
"zero-copy" claim is incorrect because primitive string identity can't be
observed with toBe()/Object.is; update the test in sliceAnsi.test.ts to assert
value equality and rename the test to reflect value preservation (e.g., "no-op
slice returns same string value") or, if you must validate a zero-copy
optimization, add proper internal instrumentation/mocking around Bun.sliceAnsi
to verify it returns the original Buffer/string instance; locate the test using
the Bun.sliceAnsi references and change the test name and assertions
accordingly.
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 `@src/bun.js/bindings/sliceAnsi.cpp`:
- Around line 255-275: The current logic bails out on the first ':' and treats
the entire SGR token as opaque, which drops other semicolon-separated params;
instead, iterate the original sequence by ';'-separated fragments (scan between
seqStart/seqEnd or paramStart/paramEnd), and for each fragment if it contains a
':' treat only that fragment as opaque: build an open string for just that
fragment (use the same StringBuilder approach over the fragment range) and
compute its closeCode via sgrCloseCode(firstParamOfFragment) / makeSgrCode, then
call state.applyStart(openFragmentStr, endStr) for that fragment; for fragments
without ':' fall back to the existing numeric/params handling (using params and
existing close logic) so other SGR styles are preserved.
- Around line 143-150: applyStart currently removes entries with the same
endCode then appends a new Entry, which mutates insertion order and can change
emitted close ordering; modify applyStart to search entries for an existing
Entry with matching endCode and, if found, update its openCode in-place
(preserving its position), otherwise append a new Entry { endCodeStr,
openCodeStr } — operate on the entries container and the
Entry.endCode/Entry.openCode fields rather than removing and re-appending.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between a65b7892ad79cf302d88f689d07e178cc946f833 and 5b3b51d160783fd4ff26658afc87fe507d20f387.
📒 Files selected for processing (2)
src/bun.js/bindings/ANSIHelpers.hsrc/bun.js/bindings/sliceAnsi.cpp
| void applyStart(const String& openCodeStr, const String& endCodeStr) | ||
| { | ||
| // Remove existing entry with same endCode, then add new one | ||
| entries.removeAllMatching([&](const Entry& e) { | ||
| return e.endCode == endCodeStr; | ||
| }); | ||
| entries.append(Entry { endCodeStr, openCodeStr }); | ||
| } |
There was a problem hiding this comment.
applyStart mutates style order when updating an existing end-code key.
Line 145 removes then re-appends matching entries, which changes insertion order. Since closes are emitted in reverse order, this can change emitted close-code ordering for equivalent style state.
Suggested fix
void applyStart(const String& openCodeStr, const String& endCodeStr)
{
- // Remove existing entry with same endCode, then add new one
- entries.removeAllMatching([&](const Entry& e) {
- return e.endCode == endCodeStr;
- });
- entries.append(Entry { endCodeStr, openCodeStr });
+ // Preserve insertion order for existing endCode keys.
+ for (auto& entry : entries) {
+ if (entry.endCode == endCodeStr) {
+ entry.openCode = openCodeStr;
+ return;
+ }
+ }
+ entries.append(Entry { endCodeStr, openCodeStr });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/sliceAnsi.cpp` around lines 143 - 150, applyStart
currently removes entries with the same endCode then appends a new Entry, which
mutates insertion order and can change emitted close ordering; modify applyStart
to search entries for an existing Entry with matching endCode and, if found,
update its openCode in-place (preserving its position), otherwise append a new
Entry { endCodeStr, openCodeStr } — operate on the entries container and the
Entry.endCode/Entry.openCode fields rather than removing and re-appending.
| // Check if any param uses colon separators (opaque extended color) | ||
| bool hasColon = false; | ||
| for (const Char* p = paramStart; p < paramEnd; ++p) { | ||
| if (*p == ':') { | ||
| hasColon = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (hasColon) { | ||
| // Treat the whole sequence as opaque - store as a start with appropriate end code | ||
| uint32_t firstParam = params.isEmpty() ? 0 : params[0]; | ||
| uint32_t closeCode = sgrCloseCode(firstParam); | ||
| String endStr = closeCode ? makeSgrCode(false, closeCode) : "\x1b[0m"_s; | ||
| // Build open code string from original sequence | ||
| StringBuilder openSb; | ||
| for (const Char* p = seqStart; p < seqEnd; ++p) | ||
| openSb.append(static_cast<UChar>(*p)); | ||
| state.applyStart(openSb.toString(), endStr); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Colon handling drops other SGR params in mixed sequences.
Line 264 short-circuits on any : and returns after tracking only firstParam. For mixed tokens like \x1b[1;38:2:255:0:0m, this can miss required active styles/closers and leak formatting across slice boundaries.
Please parse and apply each ;-separated SGR fragment independently, treating only the colon-containing fragment as opaque instead of short-circuiting the whole token.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/sliceAnsi.cpp` around lines 255 - 275, The current logic
bails out on the first ':' and treats the entire SGR token as opaque, which
drops other semicolon-separated params; instead, iterate the original sequence
by ';'-separated fragments (scan between seqStart/seqEnd or
paramStart/paramEnd), and for each fragment if it contains a ':' treat only that
fragment as opaque: build an open string for just that fragment (use the same
StringBuilder approach over the fragment range) and compute its closeCode via
sgrCloseCode(firstParamOfFragment) / makeSgrCode, then call
state.applyStart(openFragmentStr, endStr) for that fragment; for fragments
without ':' fall back to the existing numeric/params handling (using params and
existing close logic) so other SGR styles are preserved.
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 `@src/bun.js/bindings/ANSIHelpers.h`:
- Around line 210-220: Remove the duplicated local UTF‑16 decoder in
wrapAnsi.cpp and replace its usages with the shared implementation: delete the
local decodeUTF16 function and call ANSI::decodeUTF16(...) (the helper defined
in ANSIHelpers.h) wherever the local one was used, mirroring the pattern in
sliceAnsi.cpp; ensure the file includes ANSIHelpers.h (it already does) and
update any namespace or symbol references if needed so callers use
ANSI::decodeUTF16(p, available, outLen).
In `@src/bun.js/bindings/sliceAnsi.cpp`:
- Around line 74-107: The code currently caps nonEmojiWidth at 1023 but width()
returns uint8_t, which can overflow when casting; either make the types
consistent by lowering the cap to 255 (change the clamp in the block that
updates nonEmojiWidth where w > 0 to use 255) or change the width() signature
and return type to uint16_t (and adjust any callers) so the accumulated
nonEmojiWidth can be returned without truncation; reference nonEmojiWidth and
width() (and the cap logic inside the if (w > 0) block) and update all affected
call sites if you opt to widen the return type.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between f280b0e741e82f6de3f4f2df5427b2ae04eb0dd3 and 64eee42fa125ce4ef140b4b0022bea3e59ffd677.
📒 Files selected for processing (2)
src/bun.js/bindings/ANSIHelpers.hsrc/bun.js/bindings/sliceAnsi.cpp
| // ============================================================================ | ||
| // UTF-16 surrogate pair decoding — thin wrapper over ICU's U16_NEXT | ||
| // ============================================================================ | ||
| static inline char32_t decodeUTF16(const UChar* p, size_t available, size_t& outLen) | ||
| { | ||
| size_t i = 0; | ||
| char32_t cp; | ||
| U16_NEXT(p, i, available, cp); | ||
| outLen = i; | ||
| return cp; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find all decodeUTF16 occurrences
echo "=== All decodeUTF16 occurrences ==="
rg -n "decodeUTF16" --type cpp src/bun.js/bindings/
echo -e "\n=== wrapAnsi.cpp content (lines 21-40 to see decodeUTF16) ==="
head -50 src/bun.js/bindings/wrapAnsi.cpp | tail -30
echo -e "\n=== wrapAnsi.cpp includes ==="
head -20 src/bun.js/bindings/wrapAnsi.cpp
echo -e "\n=== ANSIHelpers.h includes ==="
head -30 src/bun.js/bindings/ANSIHelpers.hRepository: oven-sh/bun
Length of output: 3155
🏁 Script executed:
#!/bin/bash
# Read ANSIHelpers.h around line 213 to verify the decodeUTF16 implementation
echo "=== ANSIHelpers.h decodeUTF16 function (lines 210-225) ==="
sed -n '210,225p' src/bun.js/bindings/ANSIHelpers.h
echo -e "\n=== Full context around decodeUTF16 (lines 205-230) ==="
sed -n '205,230p' src/bun.js/bindings/ANSIHelpers.hRepository: oven-sh/bun
Length of output: 1875
Consolidate duplicate UTF-16 decoding implementation.
src/bun.js/bindings/wrapAnsi.cpp (line 22) defines a local decodeUTF16 with manual surrogate pair handling, but ANSIHelpers.h provides a shared decodeUTF16 using ICU's U16_NEXT macro. Since wrapAnsi.cpp already includes ANSIHelpers.h, remove the local implementation and call ANSI::decodeUTF16 instead, matching the pattern used in sliceAnsi.cpp (lines 726, 1099, 1149).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/ANSIHelpers.h` around lines 210 - 220, Remove the
duplicated local UTF‑16 decoder in wrapAnsi.cpp and replace its usages with the
shared implementation: delete the local decodeUTF16 function and call
ANSI::decodeUTF16(...) (the helper defined in ANSIHelpers.h) wherever the local
one was used, mirroring the pattern in sliceAnsi.cpp; ensure the file includes
ANSIHelpers.h (it already does) and update any namespace or symbol references if
needed so callers use ANSI::decodeUTF16(p, available, outLen).
| if (w > 0) { | ||
| uint16_t newWidth = nonEmojiWidth + w; | ||
| nonEmojiWidth = newWidth < 1023 ? newWidth : 1023; | ||
| } | ||
| } | ||
|
|
||
| uint8_t width() const | ||
| { | ||
| if (count == 0) | ||
| return 0; | ||
| if (regionalIndicator && count >= 2) | ||
| return 2; | ||
| if (keycap) | ||
| return 2; | ||
| if (regionalIndicator) | ||
| return 1; // Single (unpaired) regional indicator is width 1 — matches visible.zig | ||
| if (emojiBase && (skinTone || zwj)) | ||
| return 2; | ||
| if (vs15 || vs16) { | ||
| if (baseWidth == 2) | ||
| return 2; | ||
| if (vs16) { | ||
| if ((firstCp >= 0x30 && firstCp <= 0x39) || firstCp == 0x23 || firstCp == 0x2A) | ||
| return 1; | ||
| if (firstCp < 0x80) | ||
| return 1; | ||
| return 2; | ||
| } | ||
| return 1; | ||
| } | ||
| // Match visible.zig GraphemeState.width() exactly: return accumulated width | ||
| // (may be 0 for zero-width-only clusters like U+200B ZWSP). | ||
| return static_cast<uint8_t>(nonEmojiWidth); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential width truncation for pathological grapheme clusters.
Line 76 caps nonEmojiWidth at 1023, but width() returns uint8_t (max 255). If a grapheme cluster accumulates width > 255, the cast on line 106 would truncate.
In practice, grapheme clusters are small (even complex emoji ZWJ sequences have width ≤ 4), so this is unlikely to occur. However, the cap and return type are inconsistent.
Suggested consistency fix
Either cap at 255 to match the return type, or return uint16_t if wider values are expected:
- uint8_t width() const
+ uint16_t width() const
{
...
- return static_cast<uint8_t>(nonEmojiWidth);
+ return static_cast<uint16_t>(nonEmojiWidth);
}Or if uint8_t is intentional:
- nonEmojiWidth = newWidth < 1023 ? newWidth : 1023;
+ nonEmojiWidth = newWidth < 255 ? newWidth : 255;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/sliceAnsi.cpp` around lines 74 - 107, The code currently
caps nonEmojiWidth at 1023 but width() returns uint8_t, which can overflow when
casting; either make the types consistent by lowering the cap to 255 (change the
clamp in the block that updates nonEmojiWidth where w > 0 to use 255) or change
the width() signature and return type to uint16_t (and adjust any callers) so
the accumulated nonEmojiWidth can be returned without truncation; reference
nonEmojiWidth and width() (and the cap logic inside the if (w > 0) block) and
update all affected call sites if you opt to widen the return type.
There was a problem hiding this comment.
No new bugs found and my previously reported issues (zero-width width clamping, regional indicator width) were addressed, but this PR adds ~1400 lines of new C++ with ANSI parsing, SIMD, UTF-16 decoding, and a duplicated GraphemeWidthState that must stay in sync with visible.zig — the scope and complexity warrant a human review before merging.
Extended reasoning...
Overview
This PR implements Bun.sliceAnsi(string, start, end), a column-width-aware string slice that preserves ANSI escape codes. The core is a new ~1400-line C++ file (sliceAnsi.cpp) containing: a GraphemeWidthState struct mirroring the Zig GraphemeState, SGR style tracking (SgrStyleState), ANSI tokenizer (CSI, OSC 8 hyperlinks, DCS/SOS/PM/APC control strings), and a single-pass streaming emitter with speculative zone support for ellipsis. It also modifies ANSIHelpers.h (new shared utilities), BunObject.cpp (binding registration), visible.zig (genuine prev_visible bug fix + new exported C functions), TypeScript types, and adds 1700+ lines of tests.
Security risks
The C++ code processes untrusted user input strings with manual pointer arithmetic, SIMD operations, and UTF-16 surrogate pair decoding via ICU. While I don't see exploitable issues in the current code, the surface area for buffer overruns or out-of-bounds reads in ANSI parsing and grapheme clustering is significant. The StringBuilder usage appears safe, and indices are bounds-checked, but this deserves careful human scrutiny.
Level of scrutiny
This is production-critical code — it's a new public API available to all Bun users, implemented in C++ with manual memory management patterns. The GraphemeWidthState in C++ intentionally duplicates the Zig GraphemeState logic (~80 lines) for performance, creating a drift risk that tests mitigate but don't fully eliminate. The prev_visible fix in visible.zig is a real correctness improvement to the existing stringWidth function. This all warrants high scrutiny from a human reviewer.
Other factors
- The PR has been through multiple review rounds (CodeRabbit, my prior review) with critical bugs found and fixed (UB in double→int64_t casts, zero-width char width, regional indicator width, blink SGR mapping)
- Some CodeRabbit nit-level comments from Feb 26-27 appear unaddressed (applyStart mutation order, colon handling in mixed SGR sequences, width truncation cap at 1023 vs uint8_t return)
- CI failures are in unrelated vendor files (tinycc warnings)
- Test coverage is extensive (130+ unit tests, fuzz/property tests, 1646 assertions)
- The PR carries the
claudelabel indicating AI-generated code, which benefits from human validation of the overall design approach
5480c22 to
5b58b64
Compare
There was a problem hiding this comment.
This is a large feature PR (~1400 LOC new C++, new public API, SIMD paths, Zig FFI, and a bug fix to Bun.stringWidth) that warrants human review; the issues I previously flagged (zero-width width clamping, regional indicator width) are now fixed in the current code, and CI currently shows build failures.
Extended reasoning...
Overview
This PR adds Bun.sliceAnsi(str, start?, end?, options?) — a new public API replacing both slice-ansi and cli-truncate npm packages. It slices strings by terminal column width while preserving ANSI escape codes (SGR, OSC 8 hyperlinks) and respecting grapheme cluster boundaries. The implementation spans 11 files:
sliceAnsi.cpp(1381 lines, new) — core C++ implementation with three-tier dispatch: SIMD ASCII fast path, single-pass streaming emit, negative-index two-passANSIHelpers.h— shared SIMD escape detection, SGR close-code mapping, UTF-16 decodingvisible.zig— bug fix:prev_visibletracking to prevent ANSI bytes from corrupting grapheme break state inBun.stringWidthBunObject.cpp— host function registrationbun.d.ts— TypeScript typeswrapAnsi.cpp— refactored to share helpers- Three new test files (~2000+ lines)
Security risks
The DoS vector from unterminated DCS/SOS/PM/APC control strings consuming to EOF has been addressed (parseControlString returns nullptr). The SGR parameter parser caps at 32 params and clamps values to prevent overflow. Input is taken from JS strings (already length-bounded). No obvious injection or memory safety issues, but the SIMD fast paths and grapheme state machine are complex enough to warrant careful review.
Level of scrutiny
This is production-critical code — a new public API available to all Bun users, with SIMD optimizations, Zig FFI boundaries, and complex Unicode/ANSI state machines. It deserves thorough human review. The prev_visible fix in visible.zig also affects the existing Bun.stringWidth function.
Other factors
- My previous issues are fixed: The zero-width character clamping (line 106 now returns
static_cast<uint8_t>(nonEmojiWidth)without minimum-1 clamp) and unpaired regional indicator width (line 89 returns 1) are both corrected. - CI failures: Build-zig failures reported on the latest commit.
- Outstanding CodeRabbit comments: Several Major-level issues remain from the initial review (applyStart insertion order mutation, colon handling dropping SGR params in mixed sequences) plus recent Nitpick items. Some earlier issues were marked addressed in commits.
- No human review: This PR has been open for ~2 weeks with only automated reviewer activity.
- Test coverage: Comprehensive unit tests (152 cases) and fuzz tests (48 property tests) are included, which is good, but the complexity of the implementation warrants human verification of the algorithmic approach.
c1ffb88 to
de588a0
Compare
There was a problem hiding this comment.
No new bugs found, but this is a ~1400-line new C++ feature with SIMD optimizations, grapheme cluster handling, ANSI parsing, and Zig FFI — it warrants a human review before merging.
Extended reasoning...
Overview
This PR adds Bun.sliceAnsi() as a new public API, implementing ANSI & grapheme-aware string slicing in ~1415 lines of new C++ (sliceAnsi.cpp), plus shared helpers in ANSIHelpers.h, Zig FFI exports in visible.zig, TypeScript types in bun.d.ts, and comprehensive tests (152 unit + 48 fuzz). It also refactors wrapAnsi.cpp to share the new ANSI::decodeUTF16 helper and fixes a real bug in visible.zig where ANSI CSI bytes corrupted grapheme break state.
Security risks
No direct security risks (no auth, crypto, or permissions code). The main concern is robustness against adversarial input: unterminated ANSI sequences, pathological grapheme clusters, and integer overflow in index resolution. The PR addresses these — unterminated C1 controls return nullptr, SGR params are capped at 32, and index resolution uses double-space clamping matching JSC's stringSlice<double> pattern. The Zig FFI boundary (Bun__graphemeBreak taking a *u8 state pointer) has no validation but callers always pass a stack-local uint8_t initialized to 0, so this is safe in practice.
Level of scrutiny
This is production-critical code — a new public API on the Bun global object that will be used by terminal rendering libraries. The implementation involves SIMD fast paths, streaming emit with speculative zones, grapheme cluster width tracking mirrored from Zig, and SGR style state management. Multiple rounds of automated review (CodeRabbit, Claude) identified and fixed several bugs including a critical zero-width character width issue and a grapheme state corruption bug. All flagged issues appear to be addressed in the current code.
Other factors
The test coverage is thorough (152 unit tests + 48 fuzz/property tests), but the core C++ implementation is complex enough — with three-tier dispatch, speculative zones for lazy cutEnd detection, and inline grapheme clustering mirroring Zig's GraphemeState — that a human reviewer should verify the algorithmic correctness and edge case handling. The nonEmojiWidth cap at 1023 vs uint8_t return type inconsistency (flagged by CodeRabbit as trivial) is benign in practice but worth a human decision. No human has reviewed this PR yet.
b5df096 to
d4d1530
Compare
There was a problem hiding this comment.
My two previously raised issues (zero-width character width clamping and unpaired regional indicator width) have been fixed. However, this is a large new feature (~1400 lines of C++) with several remaining CodeRabbit findings (applyStart order mutation, colon handling in mixed SGR sequences) that warrant a human look at the overall design.
Extended reasoning...
Overview
This PR adds Bun.sliceAnsi, a new built-in function that replaces the slice-ansi and cli-truncate npm packages. It introduces ~1400 lines of new C++ code (sliceAnsi.cpp) with SIMD fast paths, grapheme cluster boundary detection, SGR style state tracking, and OSC 8 hyperlink handling. Supporting changes touch ANSIHelpers.h (shared helpers), visible.zig (new exported C-callable grapheme functions + a bugfix), BunObject.cpp (registration), bun.d.ts (TypeScript types), and comprehensive test files (152 unit tests + 48 fuzz tests).
Security risks
No significant security risks. The function processes user-provided strings but uses fixed-size stack buffers (32-entry SGR param array with overflow detection) and does not allocate unbounded heap memory based on untrusted input. The C1 control sequence DoS vector (unterminated DCS/SOS/PM/APC consuming to EOF) was already addressed in the PR.
Level of scrutiny
High scrutiny is warranted. This is production-critical code that will be used by Bun users as a core built-in. The C++ implementation involves subtle Unicode/ANSI parsing with many edge cases. The GraphemeWidthState struct is an intentional duplication of Zig logic for performance reasons, which creates a drift risk.
Other factors
My two previously raised issues (critical: zero-width character width clamped to 1 instead of 0; minor: unpaired regional indicator returning 2 instead of 1) have both been fixed in the current code. Several CodeRabbit findings remain unaddressed: (1) applyStart remove+re-append changes insertion order vs in-place update, (2) colon handling drops other semicolon-separated SGR params in mixed sequences like \x1b[1;38:2:255:0:0m. While these are edge cases unlikely to cause visible issues in practice, the overall size and complexity of this new feature — particularly the intentional C++/Zig logic duplication and the three-tier dispatch architecture — merit human review of the design decisions.
d4d1530 to
6d3828f
Compare
Implements `Bun.sliceAnsi(str, start?, end?, options?, ambiguousIsNarrow?)` —
replaces both `slice-ansi` and `cli-truncate` npm packages. Indices are
terminal column widths (like String.slice but ANSI-aware and grapheme-aware).
The 4th arg accepts a string (ellipsis shorthand), boolean (ambiguousIsNarrow
shorthand), or options object; a 5th boolean arg lets you pass both without
allocating an object.
## API
- `sliceAnsi(s, 0, n)` — slice first n columns, preserving ANSI state
- `sliceAnsi(s, -n)` — last n columns (negative indices like String.slice)
- `sliceAnsi(s, 0, n, '…')` — cli-truncate end-mode
- `sliceAnsi(s, -n, undefined, '…')` — cli-truncate start-mode
- `sliceAnsi(s, 0, n, false)` — ambiguousIsNarrow=false (no {} alloc)
- `sliceAnsi(s, 0, n, '…', false)` — both options positionally
Ellipsis is emitted INSIDE active SGR codes (inherits color/bold) but OUTSIDE
hyperlinks. ambiguousIsNarrow matches stringWidth/wrapAnsi.
## Architecture
Three-tier dispatch, all O(slice-length) not O(input-length):
1. **SIMD ASCII fast path**: `ANSI::firstNonAsciiPrintable<Lane>(span)`
returns the INDEX of the first non-printable byte. Whole-string ASCII OR
slice strictly inside the ASCII prefix → direct substring. Zero-copy when
nothing cut. Scan is capped at `endD + 2` for non-negative finite indices
(no need to scan past the slice target).
2. **Single-pass streaming emit** (non-negative indices, 99% case):
- `position` advances only at cluster boundaries — always correct.
- Inline `graphemeBreak` tracking (no Vector, no pre-pass).
- SIMD `findEscapeCharacter` skip-ahead with scan horizon capped at
`specEnd - position + 4`; bulk-ASCII emit within runs.
- ONE tiny lookahead: 4-entry inline buffer for ANSI between consecutive
visible chars (flushed when next char's break status is known).
- Lazy `cutEnd` for ellipsis: speculative zone → side buffer.
3. **Negative indices** (rare): one `computeTotalWidth` pre-pass + one emit.
Upfront no-op check: `sliceAnsi(s, 0)` / `sliceAnsi(s)` returns null
(→ input JSString reuse) before any scanning.
Index resolution matches JSC's `stringSlice<double>`: clamp in double space,
cast only after `[0, totalW]` verified. No int64, no UB.
## Hardening (found by fuzz testing)
- **DoS fix**: unterminated DCS/SOS/PM/APC (C1 0x90/0x98/0x9E/0x9F or ESC
variants) no longer consumes to EOF. Treated as standalone width-0.
- `SgrParams`: fixed 32-entry stack struct (ECMA-48 caps 16, xterm ~30).
Overflow → opaque passthrough. Param accumulator clamped at 100000.
- Ellipsis passed as zero-copy `StringView` — never materialized.
## Shared helpers (ANSIHelpers.h)
- `firstNonAsciiPrintable<Lane>(span)` — SIMD, templated uint8/uint16
- `sgrCloseCode`/`isSgrEndCode` — dense jump-table switch
- `decodeUTF16` — thin wrapper over ICU's U16_NEXT (wrapAnsi uses this now)
- `findEscapeCharacter` matches 0x9C (C1 ST) for SIMD/tail consistency
## Bun.stringWidth fix (found by fuzz)
`visibleUTF16WidthFn` tracked `prev` for ALL bytes including ANSI. After
`\x1b[1m`, prev='m'; a following VS16/ZWJ/combining mark would
graphemeBreak('m', extender) = false → add() on uninitialized state → width 1
instead of 0. Fix: separate `prev_visible` for grapheme tracking.
## Tests: 349 total, 5654 assertions
- `sliceAnsi.test.ts` — 154 unit tests (upstream parity, hyperlinks, grapheme,
ellipsis, ambiguousIsNarrow including positional overloads)
- `sliceAnsi-fuzz.test.ts` — 48 property/adversarial tests (seeded PRNG,
SIMD boundaries, unterminated sequences, spec-zone ordering, encoding
equivalence, exception safety, negative-index consistency)
- `stringWidth.test.ts` — +5 for ANSI-grapheme invariant
- `wrapAnsi.test.ts` — 32 regression
## Bench: `bench/snippets/slice-ansi.mjs`
Release-build highlights (npm slice-ansi / cli-truncate vs Bun):
- ink-clip (80-col, colored log line): **8.7×** (16.4μs → 1.88μs)
- truncate-end emoji: **301×** (1.57ms → 5.22μs)
- zwj-family [0,100): **8.4×** (75.3μs → 8.97μs)
6d3828f to
41ab5ce
Compare
There was a problem hiding this comment.
My previously flagged bugs (zero-width char clamping, regional indicator width) are both fixed. No new issues found, but this is a large new public API (~1400 lines of C++) with SIMD, grapheme clustering, and ANSI state tracking that warrants human review for API design and architectural decisions.
Extended reasoning...
Overview
This PR adds Bun.sliceAnsi, a new public API that replaces the slice-ansi and cli-truncate npm packages. It introduces ~1434 lines of new C++ in sliceAnsi.cpp, modifies ANSIHelpers.h (shared SIMD/SGR helpers), BunObject.cpp (registration), wrapAnsi.cpp (refactoring to shared helpers), visible.zig (grapheme break exports + bug fix), TypeScript types in bun.d.ts, and ~200 tests across 3 test files plus benchmarks.
Security risks
No direct security risks identified. The code handles untrusted string input but operates purely on string processing without I/O, network, or filesystem access. The SIMD fast path and UTF-16 surrogate decoding are bounded by input length. The DoS vector from unterminated C1 control strings has been addressed (returns nullptr). Integer overflow from double-to-int64_t casts was fixed by keeping indices as doubles (matching JSC patterns).
Level of scrutiny
This is production-critical code — it adds a new public API to Bun that will be used by terminal/CLI tooling. The C++ implementation mirrors Zig logic (GraphemeWidthState vs GraphemeState) with a comment acknowledging the ~80 lines of duplication and relying on tests to catch drift. The three-tier dispatch (SIMD ASCII, single-pass streaming, negative-index 2-pass) is well-documented but architecturally significant. A human should verify the API surface, the duplication strategy, and the edge case handling.
Other factors
My previous critical bug (zero-width chars clamped to width 1 instead of 0) has been fixed — the code now returns nonEmojiWidth directly. The regional indicator width bug (returning 2 instead of 1 for unpaired indicators) is also fixed. The remaining CodeRabbit comments are all nitpick-level: applyStart insertion order (cosmetic for SGR), colon handling treating mixed sequences as opaque (defensive and reasonable), uint8_t vs uint16_t for pathological cluster widths (theoretical only), and duplicate UTF-16 decoder consolidation. Test coverage is thorough with 152 unit tests and 48 fuzz/property tests. The PR has the claude label indicating it was authored by Claude Code, which makes human oversight especially important.
oven-sh#26963) ## `Bun.sliceAnsi(str, start?, end?, options?)` Replaces both `slice-ansi` and `cli-truncate` npm packages. Slices strings by terminal column width while preserving ANSI escape codes (SGR colors, OSC 8 hyperlinks) and respecting grapheme cluster boundaries (emoji, combining marks, flags). ```ts // Plain slice (slice-ansi replacement) Bun.sliceAnsi(line, from, to) Bun.sliceAnsi("\x1b[31mhello\x1b[39m", 1, 4) // "\x1b[31mell\x1b[39m" // Truncation with ellipsis (cli-truncate replacement) Bun.sliceAnsi("unicorn", 0, 4, "…") // "uni…" Bun.sliceAnsi("unicorn", -4, undefined, "…") // "…orn" ``` The ellipsis is emitted **inside** active SGR styles (inherits color/bold) but **outside** hyperlinks. Also supports `{ ambiguousIsNarrow }` matching `stringWidth`/`wrapAnsi`. --- ## Design ### Three-tier dispatch | Tier | Input | Passes | Allocation | |------|-------|--------|------------| | **SIMD ASCII** | All code units ∈ `[0x20, 0x7E]`, or slice range strictly inside the ASCII prefix | 1 SIMD scan | Zero-copy when nothing cut | | **Single-pass streaming** | Non-negative indices (99% of calls) | **1** input walk | Stack only | | **Negative indices** | `start < 0` or `end < 0` | 2 (width + emit) | Stack only | ### Single-pass streaming emit (the hot path) - `position` advances **only at cluster boundaries** (when `graphemeBreak` says a new cluster starts) — always correct at decision points, no correction needed. - Inline grapheme tracking — no Vector, no pre-pass. - **SIMD skip-ahead**: `findEscapeCharacter` finds the next escape byte, then **bulk-emit** the ASCII-printable sub-run in one `append` (process `asciiLen - 1` chars, leave the last for per-char to seed grapheme state). - **One tiny lookahead**: 4-entry inline buffer for ANSI between consecutive visible chars. Flushed when the next char's break status is known (continuation → flush all; break past-end → filter close-only). - **Lazy `cutEnd` for ellipsis**: speculative zone `[end - ew, end)` → side buffer. Cut detected → discard zone, emit ellipsis. EOF first → flush zone, cancel ellipsis. ### Debug build bench (200k iters) | Path | ns/op | |------|------:| | ASCII SIMD fast path | ~2,300 | | ASCII no-op (zero-copy) | ~2,100 | | ANSI + bulk-ASCII emit | ~17,500 | | CJK (per-char width-2) | ~9,500 | | ZWJ emoji (clustering) | ~19,100 | | Negative index (2-pass) | ~128,000 | --- ## Correctness & hardening ### Fixes found by fuzzing - **DoS**: unterminated DCS/SOS/PM/APC (`\x90`, `\x98`, `\x9E`, `\x9F` or ESC variants) previously consumed to EOF. A single `\x90` byte would swallow the entire string. Now treated as a standalone width-0 control char — matches `Bun.stringWidth`. - **`Bun.stringWidth` grapheme bug**: `prev` was updated for ALL bytes including ANSI. After `\x1b[1m`, `prev='m'`; a following VS16/ZWJ/combining mark would `graphemeBreak('m', FE0F) = false` → `add()` on uninitialized state → width 1 instead of 0. Fixed with separate `prev_visible`. ### Bounds & overflow - Index resolution matches JSC's `stringSlice<double>`: clamp in double space, cast to `size_t` only after `[0, totalW]` verified. No int64, no UB. - `SgrParams` is a fixed 32-entry stack struct (ECMA-48 caps at 16, xterm ~30). Overflow → opaque passthrough. Param accumulator clamped at 100,000. - Ellipsis passed as zero-copy `StringView` — never materialized. --- ## Shared helpers (`ANSIHelpers.h`) - `firstNonAsciiPrintable<Lane>(span) → index` — SIMD range check via wrapping sub + unsigned compare, templated for Latin-1/UTF-16 - `sgrCloseCode` / `isSgrEndCode` — dense jump-table switch - `decodeUTF16` — thin wrapper over ICU's `U16_NEXT` (wrapAnsi now also uses this) - `findEscapeCharacter` now matches `0x9C` (C1 ST) for SIMD/tail consistency --- ## Tests **347 tests, 5,647 assertions** across 4 files: - `sliceAnsi.test.ts` — 152 unit tests (upstream slice-ansi parity, OSC 8 hyperlinks, grapheme edge cases, ellipsis style inheritance, `ambiguousIsNarrow`) - `sliceAnsi-fuzz.test.ts` — 48 property/adversarial tests (seeded PRNG, SIMD stride boundaries, unterminated sequences, spec-zone ordering, encoding equivalence, exception safety, negative-index consistency, 300+ property checks per invariant) - `stringWidth.test.ts` — +5 for the ANSI-grapheme invariant `stringWidth(s) == stringWidth(stripANSI(s))` - `wrapAnsi.test.ts` — 32 regression (unchanged) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
oven-sh#26963) ## `Bun.sliceAnsi(str, start?, end?, options?)` Replaces both `slice-ansi` and `cli-truncate` npm packages. Slices strings by terminal column width while preserving ANSI escape codes (SGR colors, OSC 8 hyperlinks) and respecting grapheme cluster boundaries (emoji, combining marks, flags). ```ts // Plain slice (slice-ansi replacement) Bun.sliceAnsi(line, from, to) Bun.sliceAnsi("\x1b[31mhello\x1b[39m", 1, 4) // "\x1b[31mell\x1b[39m" // Truncation with ellipsis (cli-truncate replacement) Bun.sliceAnsi("unicorn", 0, 4, "…") // "uni…" Bun.sliceAnsi("unicorn", -4, undefined, "…") // "…orn" ``` The ellipsis is emitted **inside** active SGR styles (inherits color/bold) but **outside** hyperlinks. Also supports `{ ambiguousIsNarrow }` matching `stringWidth`/`wrapAnsi`. --- ## Design ### Three-tier dispatch | Tier | Input | Passes | Allocation | |------|-------|--------|------------| | **SIMD ASCII** | All code units ∈ `[0x20, 0x7E]`, or slice range strictly inside the ASCII prefix | 1 SIMD scan | Zero-copy when nothing cut | | **Single-pass streaming** | Non-negative indices (99% of calls) | **1** input walk | Stack only | | **Negative indices** | `start < 0` or `end < 0` | 2 (width + emit) | Stack only | ### Single-pass streaming emit (the hot path) - `position` advances **only at cluster boundaries** (when `graphemeBreak` says a new cluster starts) — always correct at decision points, no correction needed. - Inline grapheme tracking — no Vector, no pre-pass. - **SIMD skip-ahead**: `findEscapeCharacter` finds the next escape byte, then **bulk-emit** the ASCII-printable sub-run in one `append` (process `asciiLen - 1` chars, leave the last for per-char to seed grapheme state). - **One tiny lookahead**: 4-entry inline buffer for ANSI between consecutive visible chars. Flushed when the next char's break status is known (continuation → flush all; break past-end → filter close-only). - **Lazy `cutEnd` for ellipsis**: speculative zone `[end - ew, end)` → side buffer. Cut detected → discard zone, emit ellipsis. EOF first → flush zone, cancel ellipsis. ### Debug build bench (200k iters) | Path | ns/op | |------|------:| | ASCII SIMD fast path | ~2,300 | | ASCII no-op (zero-copy) | ~2,100 | | ANSI + bulk-ASCII emit | ~17,500 | | CJK (per-char width-2) | ~9,500 | | ZWJ emoji (clustering) | ~19,100 | | Negative index (2-pass) | ~128,000 | --- ## Correctness & hardening ### Fixes found by fuzzing - **DoS**: unterminated DCS/SOS/PM/APC (`\x90`, `\x98`, `\x9E`, `\x9F` or ESC variants) previously consumed to EOF. A single `\x90` byte would swallow the entire string. Now treated as a standalone width-0 control char — matches `Bun.stringWidth`. - **`Bun.stringWidth` grapheme bug**: `prev` was updated for ALL bytes including ANSI. After `\x1b[1m`, `prev='m'`; a following VS16/ZWJ/combining mark would `graphemeBreak('m', FE0F) = false` → `add()` on uninitialized state → width 1 instead of 0. Fixed with separate `prev_visible`. ### Bounds & overflow - Index resolution matches JSC's `stringSlice<double>`: clamp in double space, cast to `size_t` only after `[0, totalW]` verified. No int64, no UB. - `SgrParams` is a fixed 32-entry stack struct (ECMA-48 caps at 16, xterm ~30). Overflow → opaque passthrough. Param accumulator clamped at 100,000. - Ellipsis passed as zero-copy `StringView` — never materialized. --- ## Shared helpers (`ANSIHelpers.h`) - `firstNonAsciiPrintable<Lane>(span) → index` — SIMD range check via wrapping sub + unsigned compare, templated for Latin-1/UTF-16 - `sgrCloseCode` / `isSgrEndCode` — dense jump-table switch - `decodeUTF16` — thin wrapper over ICU's `U16_NEXT` (wrapAnsi now also uses this) - `findEscapeCharacter` now matches `0x9C` (C1 ST) for SIMD/tail consistency --- ## Tests **347 tests, 5,647 assertions** across 4 files: - `sliceAnsi.test.ts` — 152 unit tests (upstream slice-ansi parity, OSC 8 hyperlinks, grapheme edge cases, ellipsis style inheritance, `ambiguousIsNarrow`) - `sliceAnsi-fuzz.test.ts` — 48 property/adversarial tests (seeded PRNG, SIMD stride boundaries, unterminated sequences, spec-zone ordering, encoding equivalence, exception safety, negative-index consistency, 300+ property checks per invariant) - `stringWidth.test.ts` — +5 for the ANSI-grapheme invariant `stringWidth(s) == stringWidth(stripANSI(s))` - `wrapAnsi.test.ts` — 32 regression (unchanged) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…tor cannot grow (#42929) ### Problem - `Bun.sliceAnsi` and `Bun.stripANSI` each abort on a legal input: `panic(main thread): abort() called`, exit 134. Fuzzing found both, with no user report. - `sliceAnsi` keeps one 24-byte `Vector` entry (`pending`, `src/jsc/bindings/sliceAnsi.cpp:1257`) for each escape sequence that waits for the next visible character. `Vector::append` calls `CRASH()` at the 77,731,543rd: 78 MB of lone `\x9c` bytes. - `stripANSI` writes into a `Vector<Char>` as long as the input (`src/jsc/bindings/stripANSI.cpp:48`). A 16-bit string of 2^30 code units passes `INT32_MAX` bytes, so `Vector::grow` calls `CRASH()`. Bun 1.3.12 returns it (Notes). ### Fix - `sliceAnsi`: both lists grow with `tryAppend` behind `Bun::maxVectorSize<T>()`. On failure the binding throws `RangeError: Out of memory`, as #42833 and #42649 do. - `stripANSI`: the buffer is a `String` from `String::tryCreateUninitialized`, which holds every legal string, so the 2^30 input returns again. A failed allocation throws the same `RangeError`. - Outputs that fit do not change: 1.8 million seeded random calls match. The `String` buffer adds no copy (Notes). - Verified: `test/js/bun/util/sliceAnsi.test.ts` (1 new test), `test/js/bun/util/stripANSI.test.ts` (2 new tests). All 3 fail without the fix. The other ANSI suites pass (Notes). ### Background - `WTF::Vector<T>` holds `INT32_MAX / sizeof(T)` elements and grows by half. Past that, `append` and `grow` call `CRASH()` and `tryAppend` returns false. - A `WTF::String` holds 2^31 - 1 characters, 4 GiB when 16-bit. - A sequence waits in `pending` until the next visible character shows whether it is inside a grapheme cluster or past the range. - `Bun::maxVectorSize<T>()` is the `Vector` bound, lowered by `Bun__stringSyntheticAllocationLimit`. The tests set that to 64 KiB in a child. <details><summary>Notes</summary> **Repros, release builds on Linux x64: `main` against this branch** ```js // 1. Before: exit 134 at 4.7 GB peak. After: RangeError in 3.3 s, 4.6 GB. Bun.sliceAnsi("a" + "\x9c".repeat(77731543) + "b", 0, 2); // 77,731,542 sequences return the whole string before and after. "\x1b[m" gives the same counts (233 MB, 5.4 GB). // 2. Before: exit 134 after 2.3 s, 4.3 GB peak. After: returns 2^30 characters in 5.7 s, 8.4 GB peak. Bun.stripANSI("\x1b[31m" + "\u3042".repeat(2 ** 30)); ``` - In repro 2 the input takes 4 GiB (the repeated string and the flat copy of the rope). The buffer and the result take 2 GiB each. **Where each number comes from** - A `Pending` entry is 24 bytes, so the largest legal capacity is 89,478,485. `FastMalloc::nextCapacity` grows a full `Vector` from 77,731,542 to 116,597,313, which passes it. `tryAppend` refuses at the same step. `maxVectorSize` binds only when a test lowers the limit. #42833 has the same property. - `pendingHl` has one 24-byte tuple for each waiting hyperlink. It is never longer than `pending`, so the `pending` check covers its bound. Its `tryAppend` covers a failed allocation. - `SgrStyleState::entries` stays as it is. It holds one entry for each attribute slot, and `parseSgrParams` clamps a parameter below 1,000,000. **When stripANSI started to abort** - #28767 (in 1.3.12) replaced the `StringBuilder` with `Vector<Char>::grow(input.size())`. At that time `isValidCapacityForVector` accepted `UINT_MAX / sizeof(T)` elements, so the `Vector` held every legal string. - The WebKit upgrade #29161 (in 1.3.13) halved that bound to `(UINT_MAX >> 1) / sizeof(T)`. Bun 1.3.12 returns repro 2 and Bun 1.3.13 aborts. - `sliceAnsi` has had `pending` since #26963 added the function. The old bound only moved its abort to a higher count, so that half was never correct. No issue exists for either, so the tests are in the module test files. **The String buffer adds no copy** - `StringImpl::adopt(Vector&&)` moves the buffer only when the `Vector` allocator is `StringImplMalloc` (`wtf/text/StringImpl.h`). That is `FastCompactMalloc`, and a plain `Vector<Char>` uses `FastMalloc`, so it takes the `create(vector.span())` branch and copies. - The old path was `fastMalloc`, a `fastRealloc` in `shrinkToFit()` when the output was under half of the input, then that copy. The new path is `tryCreateUninitialized` for the buffer, then a copy into a second `tryCreateUninitialized` string of the exact length. Both allocations are fallible, so a failure of the second one also throws. - I also measured `StringImpl::tryReallocate` in place of the copy. It saves the copy for a large input with few sequences, and it keeps up to a third of slack on a small result. The copy keeps the memory of each result as it is today, so this PR uses the copy. **Benchmark of stripANSI**: release builds of `main` (b841a68) and of this change on it, LLVM 21, minimum of 6 interleaved runs. The runs are from before a293e34, which replaced `String(span)` with the same allocation and copy in a fallible form. | input (characters in, out) | `main` | this PR | |---|---|---| | SGR, 8-bit (19, 9) | 90.5 ns | 73.7 ns | | SGR, 16-bit (19, 9) | 87.2 ns | 69.4 ns | | hyperlink (44, 9) | 87.6 ns | 71.6 ns | | mixed SGR (69, 41) | 131.9 ns | 122.6 ns | | one escape (1,005, 1,000) | 286.1 ns | 258.5 ns | | one escape (64,005, 64,000) | 7.98 us | 7.72 us | | dense SGR (5,600, 1,600) | 14.22 us | 14.30 us | | dense SGR (212,992, 49,152) | 404.6 us | 408.9 us | | dense SGR, 16-bit (212,992, 49,152) | 421.2 us | 408.2 us | | no escape (16,384) | 175.5 ns | 176.1 ns | - A `sliceAnsi` benchmark (dense SGR, hyperlinks, 100 waiting sequences between characters) is within 2% of `main`. **No change for outputs that fit** - A seeded generator builds inputs from SGR, OSC 8, C1, unterminated and malformed sequences, control bytes, wide, zero-width, combining and surrogate characters. It calls `stripANSI` once and `sliceAnsi` three times for each input, with random ranges and ellipses. - A hash of 1.8 million results is equal on the two release builds. The debug ASAN build of this branch gives the same hash for the first 600,000. **The tests** - Two tests run a child with `BUN_FEATURE_FLAG_SYNTHETIC_MEMORY_LIMIT=65536`, the knob the #42649 and #42833 tests use. The bounds become 2730 waiting sequences and a buffer of 65,536 characters. - Each bound has an input that sits exactly at it and returns, and the same input one element longer, which throws: SGR sequences (Latin-1 and UTF-16), lone C1 ST bytes, hyperlinks, and the `stripANSI` buffer (Latin-1 and UTF-16). More cases: sequences at the end of the input, runs under the bound with visible characters between them, sequences before the range, and an input over the limit with nothing to strip. - Without the fix every input returns its normal length, so the failure is an assertion diff and not an abort. - The third test runs `stripANSI` at the real size: a 16-bit string of 1,073,741,826 code units. Every code unit is inside an `ESC ( x` sequence, so the output is empty and the child never writes to the buffer. It needs 2.2 GB (2.4 GB and 6 s in a debug ASAN build), and it skips under 8 GiB of memory, as `utf8-conversion-limit.test.ts` does. Without the fix the child exits with SIGABRT. - No test runs `sliceAnsi` at its real size (4.6 GB). - Other suites run on the debug ASAN build: `sliceAnsi-fuzz`, `wrapAnsi`, `wrapAnsi.npm`, `stringWidth`, and the rest of `sliceAnsi` and `stripANSI`. **Not changed here: `Bun.sliceAnsi` aborts that are not a `Vector`** #42931 tracks them. All five exit 134 on a release build of this branch. - The result fits, and a `StringBuilder` doubles its capacity past the longest 16-bit string. oven-sh/WebKit#631 (open) fixes the builder. - `Bun.sliceAnsi("\u3042".repeat(2 ** 30), 0, 4)`: `result.reserveCapacity(input.size())` (`sliceAnsi.cpp:876`) reserves 2^30 8-bit characters, and the first 16-bit append converts the buffer. 2.2 GB, for a result of 2 characters. - `Bun.sliceAnsi("\x1b[31m" + "a".repeat(2 ** 30), 0, 4, "\u2026")`: the same, and the ellipsis is the first 16-bit append. 2.2 GB, for a result of 14 characters. - `Bun.sliceAnsi("\x1b]8;;" + "\u3042".repeat(2 ** 30) + "\x07x", 0, 1)`: the builder of one hyperlink in `parseHyperlink` (`sliceAnsi.cpp:572`). 8.6 GB. - The result passes the string length limit. This is the `sliceAnsi` sibling of #42225 (`wrapAnsi`, open). - `Bun.sliceAnsi("\x1b[31m" + "a".repeat(2 ** 31 - 6), 1)`: `StringBuilder result`. 6.5 GB. - `Bun.sliceAnsi("a".repeat(2 ** 31 - 2), 1, undefined, "\u2026" + "\u200b".repeat(10))`: `makeString` on the ASCII fast path (`sliceAnsi.cpp:1400`). 2.2 GB. **Self-review** - Changes made after it: exact-bound test cases, the real-size `stripANSI` test, one-line comments, the list above, and the 1.3.12 to 1.3.13 account. - After the bot reviews: the exact-length copy in `stripANSI` is fallible too (a293e34). - Deferred: a `sliceAnsi` that needs no list (it scans the waiting run again when the next visible character arrives). That removes the throw. It touches the hot loop, so it is a PR of its own. - #42908 (open) edits other lines of `stripANSI.cpp`. Whichever lands second needs a small rebase. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 4 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 3 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/util/sliceAnsi.test.ts test/js/bun/util/stripANSI.test.ts bun test v1.4.3 (09bb546) test/js/bun/util/stripANSI.test.ts: (pass) Bun.stripANSI > returns same string object when no ANSI sequences present [171.84ms] (pass) Bun.stripANSI > returns new string when ANSI sequences are removed [2.47ms] (pass) Bun.stripANSI > "\u001b[31mred\u001b[39m" [2.01ms] (pass) Bun.stripANSI > "\u001b[32mgreen\u001b[39m" [0.85ms] (pass) Bun.stripANSI > "\u001b[33myellow\u001b[39m" [0.29ms] (pass) Bun.stripANSI > "\u001b[34mblue\u001b[39m" [0.30ms] (pass) Bun.stripANSI > "\u001b[35mmagenta\u001b[39m" [0.37ms] (pass) Bun.stripANSI > "\u001b[36mcyan\u001b[39m" [0.26ms] (pass) Bun.stripANSI > "\u001b[37mwhite\u001b[39m" [0.28ms] (pass) Bun.stripANSI > "\u001b[41mred background\u001b[49m" [0.30ms] (pass) Bun.stripANSI > "\u001b[42mgreen background\u001b[49m" [0.26ms] (pass) Bun.stripANSI > "\u001b[1mbold\u001b[22m" [0.26ms] (pass) Bun.stripANSI > "\u001b[2mdim\u001b[22m" [0.27ms] (pass) Bun.stripANSI > "\u001b[3mitalic\u001b[23m" [0.26ms] (pass) B ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (ee97718) test/js/bun/util/stripANSI.test.ts: (pass) Bun.stripANSI > returns same string object when no ANSI sequences present [2.25ms] (pass) Bun.stripANSI > returns new string when ANSI sequences are removed [0.05ms] (pass) Bun.stripANSI > "\u001b[31mred\u001b[39m" [0.02ms] (pass) Bun.stripANSI > "\u001b[32mgreen\u001b[39m" (pass) Bun.stripANSI > "\u001b[33myellow\u001b[39m" (pass) Bun.stripANSI > "\u001b[34mblue\u001b[39m" (pass) Bun.stripANSI > "\u001b[35mmagenta\u001b[39m" (pass) Bun.stripANSI > "\u001b[36mcyan\u001b[39m" (pass) Bun.stripANSI > "\u001b[37mwhite\u001b[39m" (pass) Bun.stripANSI > "\u001b[41mred background\u001b[49m" (pass) Bun.stripANSI > "\u001b[42mgreen background\u001b[49m" (pass) Bun.stripANSI > "\u001b[1mbold\u001b[22m" (pass) Bun.stripANSI > "\u001b[2mdim\u001b[22m" (pass) Bun.stripANSI > "\u001b[3mitalic\u001b[23m" (pass) Bun.stripANSI > "\u001b[4munderline\u001b[24m" (pass) Bun.stripANSI > "\u001b[5mblink\u001b[25m" (pass) Bun.stripANSI > "\u001b[7mreverse\u001b[27m" (pass) Bun.stripANSI > "\u001b[8mhidden\u001b[28m" (pass) Bun.stripANSI > "\u001b[9mstrikethrough\u001b[29m" (pass) Bun.stripANSI > "\u001b[38;5;1 ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/util/sliceAnsi.test.ts test/js/bun/util/stripANSI.test.ts bun test v1.4.3 (09bb546) test/js/bun/util/stripANSI.test.ts: (pass) Bun.stripANSI > returns same string object when no ANSI sequences present [188.15ms] (pass) Bun.stripANSI > returns new string when ANSI sequences are removed [2.74ms] (pass) Bun.stripANSI > "\u001b[31mred\u001b[39m" [2.55ms] (pass) Bun.stripANSI > "\u001b[32mgreen\u001b[39m" [0.48ms] (pass) Bun.stripANSI > "\u001b[33myellow\u001b[39m" [0.32ms] (pass) Bun.stripANSI > "\u001b[34mblue\u001b[39m" [0.28ms] (pass) Bun.stripANSI > "\u001b[35mmagenta\u001b[39m" [0.36ms] (pass) Bun.stripANSI > "\u001b[36mcyan\u001b[39m" [0.28ms] (pass) Bun.stripANSI > "\u001b[37mwhite\u001b[39m" [0.26ms] (pass) Bun.stripANSI > "\u001b[41mred background\u001b[49m" [0.30ms] (pass) Bun.stripANSI > "\u001b[42mgreen background\u001b[49m" [0.28ms] (pass) Bun.stripANSI > "\u001b[1mbold\u001b[22m" [0.62ms] (pass) Bun.stripANSI > "\u001b[2mdim\u001b[22m" [0.28ms] (pass) Bun.stripANSI > "\u001b[3mitalic\u001b[23m" [0.25ms] (pass) B ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 709ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/8] gen cpp.rs (cppbind) [1/8] cargo bun_runtime → libbun_runtime.a �[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m �[1m�[94m|�[0m �[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl �[1m�[94m|�[0m �[1m�[33m^^^^^^^^^^^^^�[0m �[1m�[94m|�[0m �[1m�[94m= �[0m�[1mnote�[0m: `cargo::non_kebab_case_bins` is set to `warn` by default �[1m�[96mhelp�[0m: to change the binary name to `bun-shim-impl`, convert `bin.name` �[1m�[94m--> �[0msrc/install/windows-shim/Cargo.toml:41:8 �[1m�[94m|�[0m �[1m�[94m41�[0m �[91m- �[0mname = �[91m"bun_shim_impl"�[0m �[1m�[94m41�[0m �[92m+ �[0mname = �[92m"bun-shim-impl"�[0m �[1m�[94m|�[0m �[1m�[33mwarning�[0m: `bun_shim_impl` (manifest) generated 1 warning �[1m�[33mwarning�[0m�[1m: `feature(generic_const_exprs)` is not supported with the next-generation trait solver�[0m �[1m�[94m--> �[0msrc/shell_parser/lib.rs:1:30 �[1m�[94m|�[0m �[1m�[94m1�[0m ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/bindings/sliceAnsi.cpp | 30 ++++++++------ src/jsc/bindings/stripANSI.cpp | 42 +++++++++++--------- test/js/bun/util/sliceAnsi.test.ts | 73 ++++++++++++++++++++++++++++++++++ test/js/bun/util/stripANSI.test.ts | 81 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 196 insertions(+), 30 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/bindings/sliceAnsi.cpp 8 6 26 src/jsc/bindings/stripANSI.cpp 5 12 26 test/js/bun/util/sliceAnsi.test.ts 2 2 17 test/js/bun/util/stripANSI.test.ts 1 1 17 ``` </details> <!-- robobun:evidence:end -->
2 participants
Bun.sliceAnsi(str, start?, end?, options?)Replaces both
slice-ansiandcli-truncatenpm packages. Slices strings by terminal column width while preserving ANSI escape codes (SGR colors, OSC 8 hyperlinks) and respecting grapheme cluster boundaries (emoji, combining marks, flags).The ellipsis is emitted inside active SGR styles (inherits color/bold) but outside hyperlinks. Also supports
{ ambiguousIsNarrow }matchingstringWidth/wrapAnsi.Design
Three-tier dispatch
[0x20, 0x7E], or slice range strictly inside the ASCII prefixstart < 0orend < 0Single-pass streaming emit (the hot path)
positionadvances only at cluster boundaries (whengraphemeBreaksays a new cluster starts) — always correct at decision points, no correction needed.findEscapeCharacterfinds the next escape byte, then bulk-emit the ASCII-printable sub-run in oneappend(processasciiLen - 1chars, leave the last for per-char to seed grapheme state).cutEndfor ellipsis: speculative zone[end - ew, end)→ side buffer. Cut detected → discard zone, emit ellipsis. EOF first → flush zone, cancel ellipsis.Debug build bench (200k iters)
Correctness & hardening
Fixes found by fuzzing
\x90,\x98,\x9E,\x9For ESC variants) previously consumed to EOF. A single\x90byte would swallow the entire string. Now treated as a standalone width-0 control char — matchesBun.stringWidth.Bun.stringWidthgrapheme bug:prevwas updated for ALL bytes including ANSI. After\x1b[1m,prev='m'; a following VS16/ZWJ/combining mark wouldgraphemeBreak('m', FE0F) = false→add()on uninitialized state → width 1 instead of 0. Fixed with separateprev_visible.Bounds & overflow
stringSlice<double>: clamp in double space, cast tosize_tonly after[0, totalW]verified. No int64, no UB.SgrParamsis a fixed 32-entry stack struct (ECMA-48 caps at 16, xterm ~30). Overflow → opaque passthrough. Param accumulator clamped at 100,000.StringView— never materialized.Shared helpers (
ANSIHelpers.h)firstNonAsciiPrintable<Lane>(span) → index— SIMD range check via wrapping sub + unsigned compare, templated for Latin-1/UTF-16sgrCloseCode/isSgrEndCode— dense jump-table switchdecodeUTF16— thin wrapper over ICU'sU16_NEXT(wrapAnsi now also uses this)findEscapeCharacternow matches0x9C(C1 ST) for SIMD/tail consistencyTests
347 tests, 5,647 assertions across 4 files:
sliceAnsi.test.ts— 152 unit tests (upstream slice-ansi parity, OSC 8 hyperlinks, grapheme edge cases, ellipsis style inheritance,ambiguousIsNarrow)sliceAnsi-fuzz.test.ts— 48 property/adversarial tests (seeded PRNG, SIMD stride boundaries, unterminated sequences, spec-zone ordering, encoding equivalence, exception safety, negative-index consistency, 300+ property checks per invariant)stringWidth.test.ts— +5 for the ANSI-grapheme invariantstringWidth(s) == stringWidth(stripANSI(s))wrapAnsi.test.ts— 32 regression (unchanged)🤖 Generated with Claude Code