fix(chunk): keep emoji whole when length-splitting a long line in chunkByNewline#96494
fix(chunk): keep emoji whole when length-splitting a long line in chunkByNewline#96494ly-wang19 wants to merge 1 commit into
Conversation
chunkByNewline length-splits a long line with a raw `lineValue.slice(0, firstLimit)`, which can cut through a surrogate pair when firstLimit lands on an odd UTF-16 offset. The first chunk then ends on a lone high surrogate and the next chunk begins with the orphaned low surrogate, rendering a broken/replacement glyph. Guard the boundary with avoidTrailingHighSurrogateBreak (already used by chunkText/chunkMarkdownText in this file and already imported), so the split lands on a code-point boundary. Only the rare mid-surrogate boundary changes; all other inputs are byte-identical. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 12:29 PM ET / 16:29 UTC. Summary PR surface: Source +1, Tests +16. Total +17 across 2 files. Reproducibility: yes. current source uses a raw UTF-16 slice in Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the small shared chunker fix after contributor proof shows the actual patched Do we have a high-confidence way to reproduce the issue? Yes: current source uses a raw UTF-16 slice in Is this the best way to solve the issue? Yes for the code shape: using the existing AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 3217165be770. Label changesLabel justifications:
Evidence reviewedPR surface: Source +1, Tests +16. Total +17 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Closing: superseded by #96951 (merged in Proof:
What would change the decision: a code path or test in #96494 that #96951 missed. None found in the comparison — both the production fix and the surrogate-pair test coverage are present on |
openclaw-barnacle
Bot
added
the
r: too-many-prs
|
Closing this PR because the author has more than 20 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
|
Real behavior proof for the patched Setup
Test sourceimport { describe, expect, it } from "vitest";
import { chunkByNewline } from "./chunk.js";
describe("PR #96494 surrogate-safe chunk proof", () => {
it("chunkByNewline splits emoji run without lone surrogates", () => {
const text = "😀".repeat(8); // 8 codepoints, 16 UTF-16 units
const limit = 3; // forces length-based splitting
const chunks = chunkByNewline(text, limit, { splitLongLines: true, trimLines: false });
// No chunk should contain a lone surrogate.
for (const c of chunks) {
const units = [...c];
for (let i = 0; i < units.length; i++) {
const cp = units[i].codePointAt(0);
const isLoneSurrogate = cp >= 0xD800 && cp <= 0xDFFF && units[i].length === 1;
expect(isLoneSurrogate, `lone surrogate in chunk "${c}"`).toBe(false);
}
}
for (const c of chunks) {
expect(c.length).toBeLessThanOrEqual(limit);
}
});
});Copied terminal output (actual patched function execution)Interpretation
@clawsweeper re-review |
|
🦞🧹 Reason: re-review requires an open issue or PR. |
1 participant
What Problem This Solves
chunkByNewlineinsrc/auto-reply/chunk.tslength-splits a long line with araw
lineValue.slice(0, firstLimit). WhenfirstLimitlands on an odd UTF-16offset, the slice cuts through a surrogate pair: the first chunk ends on a
lone high surrogate and the next chunk begins with the orphaned low surrogate.
The astral character (emoji, CJK extension, etc.) is split across the boundary
and renders as a broken / replacement glyph.
Every other length-splitter in this file (
chunkText,chunkMarkdownText)already guards this boundary with
avoidTrailingHighSurrogateBreak. This pathwas the lone exception, even though the helper is already imported at the top of
the file.
Repro: a single 8-emoji line (16 UTF-16 units),
maxLineLength = 3:lineValue.slice(0, 3)="😀\uD83D"— onefull 😀 followed by a lone high surrogate; the next chunk starts with the
orphaned low surrogate
\uDE00, so the second 😀 is split and renders broken.first chunk is a single whole
"😀"and the rest is carried into the nextchunk — no lone surrogate is ever emitted.
Fix
Apply
avoidTrailingHighSurrogateBreak(lineValue, 0, firstLimit)before slicing,mirroring the guard already used by
chunkText/chunkMarkdownText. Only therare mid-surrogate boundary changes; every other input is byte-identical (verified
below). A regression test is added to
chunk.test.ts.Evidence
Standalone node red-green proof using the real
avoidTrailingHighSurrogateBreakextracted verbatim from
src/shared/text-chunking.ts, plus a disk-content checkasserting the fix is actually written to source:
The buggy raw slice produces
"😀\uD83D"(lone high surrogate) for the repro;the fixed slice produces a single whole
"😀". Plain ASCII and even-offsetboundaries are byte-identical before and after, confirming the change is scoped to
the mid-surrogate case only.
🤖 Generated with Claude Code