sliceAnsi: keep the truncated string within the range when a wide cluster straddles the ellipsis cut - #42551
sliceAnsi: keep the truncated string within the range when a wide cluster straddles the ellipsis cut#42551robobun wants to merge 3 commits into
Conversation
…dles the ellipsis cut Bun.sliceAnsi follows slice-ansi 8 and cli-truncate 5: the end of a slice keeps a wide cluster that starts before the cut. With an ellipsis the content end moves to end - width(ellipsis), so that rule made the result one column wider than the range, and more for a cluster that is wider than 2 columns. Upstream changed the rule in slice-ansi 9.0.0 after the same report against cli-truncate. This change applies the new rule to the ellipsis path only. A plain slice keeps the old rule. The width of a cluster is known only when the cluster ends. The fit is checked after the width is added to position, and only once position reaches end, so the walk before that point is unchanged. A cluster that started before the content end and extends past it opens the speculative zone at its own start. It is discarded with the cut and kept when nothing is cut. The negative-index path and a range with only a start ellipsis use the same check.
|
Status Reproduced on 1.4.3-canary.1 (6a92015) and on main (b993710): const W = Bun.stringWidth;
for (const [s, max] of [["安宁哈", 4], ["安宁哈", 2], ["a安宁哈", 3], ["🙂🙂🙂", 4]]) {
const o = Bun.sliceAnsi(s, 0, max, "…");
console.log(JSON.stringify(o), "width", W(o), "max", max);
}Each result is one column wider than
CI on 6a25562 (build 115003: 179 jobs passed, 2 failed). The diff is green.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe change updates ChangessliceAnsi ellipsis handling
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The ellipsis handling retains formatting correctly and keeps truncated output within its width budget in the covered paths. No unresolved merge risk is identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:32 PM PT - Sep 12th, 2026
❌ @robobun, your commit 6a25562 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 42551That installs a local version of the PR into your bun-42551 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… structure The fit check is now `position > contentEnd` right after a cluster's width is added. contentEnd stays SIZE_MAX unless an ellipsis is in the output and a cluster was kept, so a plain slice never takes the branch. The cut detection block and the zone resolution keep their old shape.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the latest push and didn't find any bugs. Because this reworks the speculative-rollback state machine in emitSliceStreaming and changes Bun.sliceAnsi's output for the ellipsis case, a human look is still worthwhile.
What was reviewed:
checkClusterFitis invoked at all three width-accumulation sites (per-cp break, ASCII-bulk finalize, EOF finalize); theposition - gs.width() >= endguard correctly defers to the existing spec-zone entry for clusters that start at/pastend.- The lazy style snapshot (
stylesSavedForCluster) capturesactiveStylesbeforeflushPendingruns on ANSI inside a cluster, andcheckClusterFit's fallback snapshot is taken before the next break's flush — both restore the state atclusterMark. - The
ellipsisEndBudget > 0 || inSpecZonewidening plus the newASSERThold: whenellipsisEndBudget == 0,contentEnd >= specEnd, soposition > contentEndimpliessawCutEndon every path that opens the zone. - The one modified pre-existing assertion (
end4→5) encoded the old overflow; the same input atend=4is re-asserted under the new rule in the added test block.
Extended reasoning...
Overview
This PR changes Bun.sliceAnsi's ellipsis mode so a wide grapheme cluster that straddles the content boundary (end - width(ellipsis)) is dropped rather than kept whole, guaranteeing the truncated result never exceeds the requested column budget. The implementation in src/jsc/bindings/sliceAnsi.cpp adds a clusterMustFit mode with contentEnd/clusterMark/stylesSavedForCluster state, a checkClusterFit helper called at each of the three points where cluster width is committed, a beginInclude helper deduplicating two include-entry blocks, and a widened post-walk discard condition (|| inSpecZone) with an ASSERT on the no-cut branch. Tests add a large targeted block (CJK/emoji/ZWJ/conjunct, both edges, negative indices, 8-bit ambiguous, ASCII bulk path, SGR and OSC-8 inside the dropped cluster, and an exhaustive start×end×ellipsis law loop), tighten three fuzz invariants by removing the +1 tolerance, and add a wide-cluster-dense fuzz generator plus a new range property test. bun.d.ts documents the new width guarantee.
Security risks
None identified. sliceAnsi is a pure string-width utility over in-memory strings with no I/O, allocation of user-sized buffers, auth, crypto, or filesystem/network reach. The added state is stack-local scalars and existing StringBuilder/String members; result.shrink(specZoneMark) uses a mark taken from result.length() earlier in the same walk. No new JS re-entry points or exception-throwing calls were added.
Level of scrutiny
Medium-high. The change is a focused bug fix aligning with upstream slice-ansi 9 / cli-truncate 6 and the existing bun.d.ts promise, but it threads new state through a subtle single-pass streaming state machine with speculative rollback, and it is a semantic change to a public API (~5.5% of generated cases change output per the PR's differential). The interaction between checkClusterFit, the pre-existing enterSpecZone, the lazy style snapshot, and the three code paths that commit cluster width warrants a maintainer's read, as does the decision to apply the new rule only when an ellipsis is present (leaving the plain-slice rule at slice-ansi 8 semantics).
Other factors
Test coverage is unusually thorough: the exhaustive law loop and the tightened fuzz invariants would catch regressions across the variant matrix REVIEW.md asks for (both edges, negative indices, 8-bit vs 16-bit, ASCII fast path vs streaming, SGR/OSC-8 inside the dropped cluster). The one modified pre-existing assertion is explained (it encoded the overflow) and the same input under the new rule is re-asserted nearby, satisfying the "never silently weaken a test" rule. Prior inline findings from this system were followed by commit 6a25562, which restructured the fit check to a single compare per cluster; this run found no issues in that revision. Given the state-machine subtlety and the API-visible behavior change, deferring to a human is the safer call over auto-approval.
This branch has not been deployed
2 participants
Problem
sliceAnsi("安宁哈", 0, 4, "…")is"安宁…", 5 columns for a range of 4.bun.d.tscounts the ellipsis against the budget.emitSliceStreaming(src/jsc/bindings/sliceAnsi.cpp) keeps a cluster when its START column is beforeend - width(ellipsis), the slice-ansi 8 / cli-truncate 5 rule. Upstream got the same report (Truncation exceeds maxWidth for East Asian wide characters sindresorhus/cli-truncate#28) and changed the rule in slice-ansi 9.0.0. cli-truncate 6.1.1 returns"安…".Fix
sliceAnsi("安宁", 0, 4, "…")is still"安宁".sliceAnsi(red + "ab漢" + reset + "xy", 0, 4, "…")expected 5 columns. It moved to(0, 5).test/js/bun/util/sliceAnsi.test.tsandsliceAnsi-fuzz.test.ts(1.4.3-canary fails 3 tests). Release A/B against main on 30 rows: no measurable change (Notes). Self-reviewed: 3 concerns raised, 3 addressed.Background
positionis the start column of the open cluster. Its width is added when the next cluster starts, because a joiner can still change it.[end - ew, end)as tentative output (the speculative zone) and records a mark. A cut shrinks the result to the mark and appends the ellipsis. No cut keeps the zone.Notes
Before and after (
WisBun.stringWidthof the result):sliceAnsi("安宁哈", 0, 4, "…")"安宁…"W 5"安…"W 3sliceAnsi("安宁哈", 0, 2, "…")"安…"W 3"…"W 1sliceAnsi("a安宁哈", 0, 3, "…")"a安…"W 4"a…"W 2sliceAnsi("🙂🙂🙂", 0, 4, "…")"🙂🙂…"W 5"🙂…"W 3sliceAnsi("安宁哈", 0, -2, "…")"安宁…"W 5"安…"W 3sliceAnsi("क्ष्म्य" + "xyz", 0, 2, "…")"क्ष्म्य…"W 5"…"W 1sliceAnsi("安宁", 0, 4, "…")"安宁""安宁"sliceAnsi("安宁哈", 0, 3)(no ellipsis)"安宁""安宁"Where the old rule came from. The
+1was not a slip.sliceAnsifollows slice-ansi 8 and cli-truncate 5.1.1 (the pair inbench/package.json), where the end of a slice keeps a wide character that starts before the cut, andsliceAnsi-fuzz.test.tsrecorded that as a+1tolerance for the ellipsis case too. cli-truncate 5 had the same overflow and got it as a bug report: sindresorhus/cli-truncate#28. slice-ansi 9.0.0 fixed it as a breaking change ("MakeendSliceexclude partial wide graphemes"), and cli-truncate 6 uses it. This PR applies that end rule only when an ellipsis is in the output, because that is wherebun.d.tspromises a width budget and names cli-truncate.Not changed: the plain slice rule. Without an ellipsis a cluster that starts before
endis kept whole, sosliceAnsi("安宁哈", 0, 3)is"安宁". That has its own tests. slice-ansi 9 returns"安". A move of the plain path to the slice-ansi 9 rule is a separate, breaking decision for a maintainer. The check added here (checkClusterFit,clusterMark) is what that change would use.Not changed: an ellipsis that does not fit the range.
sliceAnsi("abcdef", 0, 2, "...")is still"...". Thebun.d.tssentence says "if the ellipsis itself fits the range".Paths covered. The lazy path (non-negative
end, cut detected during the walk), the known-cut path (negative indices, one width pre-pass), and a range that has room only for a start ellipsis all run the same check (checkClusterFit()). The mark also snapshots the active SGR and OSC 8 hyperlink state, so the ellipsis and the close codes match what stays in the result.Clusters wider than 2. An Indic conjunct is one cluster and can be 3 or more columns. The check uses the real cluster width, not a fixed window before the cut.
Slack after a cluster dropped at
start. When a wide cluster straddlesstartit is dropped whole, so the first kept cluster starts one column late. The content may end that much later and still fit.contentEnd = end + (firstKeptColumn - start)keeps those results as they were:sliceAnsi("abc🙂漢,🙂", 1, 9, "...")stays"...漢..."(8 columns for a range of 8).ANSI inside the dropped cluster. ANSI does not break a cluster, so
👩 ESC[31m ZWJ 💻is one cluster with an SGR inside. The zone needs the style state from the start of the cluster. The walk takes that snapshot only when ANSI is flushed inside an open cluster, which is rare.Nothing fits between two ellipses.
sliceAnsi("xy安宁哈", 1, 4, "…")is now"……"(2 columns for a range of 3). Before it was"…安…"(4 columns). The lazy path already returned two ellipses when the first candidate cluster starts at the content end.Performance. The fit check is one compare per cluster,
position > contentEnd, right after the cluster width is added.contentEndstaysSIZE_MAXunless an ellipsis is in the output and a cluster was kept, so a plain slice never takes the branch. The only other per-cluster cost is oneresult.length()store. A first version ran a five-term condition and two stores for every cluster and was 5 to 15% slower on CJK and emoji rows. Release builds of main and of this branch, same toolchain, one changed translation unit, interleaved runs pinned withtaskset, 6 rounds, best-of-round minimum and median p10 in ns per call. Rows and fixtures are the Bun side ofbench/snippets/slice-ansi.mjsplus longer CJK, emoji and Latin-1 rows:ascii-short,ascii-longandtruncate no-cuttake the ASCII fast path, which this PR does not touch, so their deltas (up to +4.4% and -6.2%) show the noise floor of a shared machine. The largest delta on a row that runs the changed code is +2.8% (latin1 plain), inside that floor. The ZWJ and combining rows are a little faster because the join branch no longer callsflushPendingwhen nothing is pending.Differential check. A script generated 180,000
(string, start, end, ellipsis, ambiguousIsNarrow)cases from ASCII, CJK, emoji, ZWJ sequences, flags, conjuncts of width 2 and 3, combining marks, variation selectors, Prepend, zero-width characters, SGR and OSC 8 codes, with non-negative and negative indices. It ran each case on 1.4.3-canary and on this branch. Results: 9,970 changed, all of them over budget before. 0 over budget after. 0 plain-slice results changed. The kept text of each changed result is a prefix of the old kept text. The script skipped 2,198 inputs whereBun.stringWidthandsliceAnsidisagree on the width of a Prepend-led cluster that carries a variation selector (stringWidth("\u0600\u53c0\ufe0f")is 2,sliceAnsicounts 0). That mismatch exists on 1.4.3 and is in the area #41525 reworks. The final version in this PR produces byte-identical output to the first version on all 180,000 cases.Tests. The new unit test holds the ledger cases, the no-cut cases, negative indices, a width-4 conjunct, both edges cut, start ellipsis only, the slack case, 8-bit input, the bulk ASCII path, and SGR and OSC 8 inside or after the dropped cluster. It ends with an exhaustive width law over 5 strings, 3 ellipses, every range, both index forms. Mutants of the fit condition (drop the slack term, drop "was kept", drop "starts before the content end", drop "an ellipsis is in the output") each fail at least one test.
sliceAnsi-fuzz.test.tsloses the+1for ellipsis cases and gains a property over wide-dense random strings.Self-review. Concerns: per-cluster cost on the plain path (restructured, table above), the upstream lineage missing from the text (added), the
bun.d.tssentence and stale test comments (qualified and updated). One suggestion not taken: a tracking issue for the plain slice rule. It is stated above as an open decision instead.Suites run on the debug build:
sliceAnsi.test.ts,sliceAnsi-fuzz.test.ts,stringWidth.test.ts,wrapAnsi.test.ts,stripANSI.test.ts: 933 pass.[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file