fix(ai): keep emoji whole when truncating AI prompts
Brought to you by:
thebguy
Menu
▾
▴
#100 fix(ai): keep emoji whole when truncating AI prompts
Originally created by: theBGuy
Originally owned by: theBGuy
AI PR reviews could fail to run when a third-party bot review or a prior comment contained an emoji: prompt truncation cut on UTF-16 code-unit boundaries, so a cap landing between the two halves of an astral character left a lone high surrogate that the model provider rejected ("unexpected end of hex escape" / "Invalid body") once the prompt was JSON-serialized. This makes all prompt truncation surrogate-aware so emoji are never split.
- Adds
safeSliceinsrc/lib/ai/truncate.ts, which slices to at mostmaxUTF-16 code units but backs off one unit when the boundary character is a high surrogate, avoiding a trailing lone surrogate; the doc comment explains the serde_json / HTTP-API failure it prevents. - Routes the existing truncation sites through
safeSliceinstead of raw.slice(0, cap): the per-file cap and the two review-extras cases inbudgetDiff/budgetReviewExtras(src/lib/ai/truncate.ts),condenseinsrc/lib/ai/external-context.ts, andcondenseOwnCommentpluscapLedgerinsrc/lib/ai/own-context.ts, importingsafeSlicein the latter two. - Adds
changelog.d/fixed-review-prompt-surrogate.mddocumenting the fix for users.
Originally posted by: cloudflare-workers-and-pages[bot]
Deploying gitdesktop with
Cloudflare Pages
8e8f419View logs
Originally posted by: theBGuy
Context for reviewers (author's agent session)
What this fixes: AI PR reviews hard-failed to run whenever a third-party bot review or a prior comment contained an emoji near a truncation cap. This is the exact bug that blocked PR [#99]'s round-2 review — a nice self-referential fix (the review pipeline repairing itself).
Deliberate calls, so rounds focus on the genuinely new:
Root cause (reproduced, don't re-derive): JS
String.prototype.slice(0, cap)cuts on UTF-16 code units, so a cap landing between the halves of an astral char (emoji) leaves a lone high surrogate.condense()inexternal-context.tsslices bot review bodies atREVIEW_BODY_CAP = 1500; Copilot's cleaned review body was 1593 chars with 💡 (U+1F4A1) at index 1499–1500, so.slice(0,1500)split the surrogate pair. The lone surrogate makes the model request body invalid JSON, rejected by every provider — Claude CLI relays serde_json"unexpected end of hex escape at line 1 column N"; HTTP APIs return"Invalid body: failed to parse JSON value".Fix =
safeSlice()(backs off one unit when the last kept char is a high surrogate), applied at all six char-slice sites in the review-prompt path —external-context.ts(the culprit),own-context.ts×2,truncate.ts×3 — fixing the class, not just the one instance.Why bumping the review context size never helped (non-obvious): the offending per-item caps (
INLINE 700 / REVIEW 1500 / COMMENT 1200,OWN_BODY_CAP 1500) are fixed, applied BEFORE the budget allocator, soreviewContextSize(which scales the allocator) doesn't touch them. So please don't suggest "make the caps configurable/larger" — that's orthogonal.Validation: this repo has no frontend test runner (testing posture), so there's no in-repo unit test. Validated two ways instead: (a) offline — reproducing the exact
condenseregex + 1500 cap on Copilot's real review body yields a lone surrogate with.sliceand a well-formed string withsafeSlice; (b) live — [#99]'s round-2 review ran clean once this HMR'd into the dev app.Deliberate scope: a
String.prototype.toWellFormed()catch-all at the request boundary is a reasonable belt-and-suspenders, but I fixed the actual cut at the source instead (toWellFormed replaces lone surrogates with U+FFFD, but wouldn't cover a cut\uXXXXin already-escaped text). Thetruncate.tsKEEP-IN-SYNC Rust mirrormcp_server/generate.rsis a different failure mode (Rust byte-slicing panics on a non-char boundary) on a different path (MCP recipe tools, not the review) — a follow-up to verify, out of scope here.Full write-up of the diagnostic journey (two refuted hypotheses, the provider-switch test that proved it was request-side) is in the linked issue/memory; happy to expand on any point.
Posted by GitDesktop — automated agent comment, verify before acting on it.
Related
Tickets:
#99Originally posted by: theBGuy
🤖 GitDesktop AI security audit ·
opus· automatedNo security issues found in these changes. The diff adds a pure
safeSlicehelper that backs off one UTF-16 code unit to avoid emitting a lone high surrogate, and swaps it in for six existing.slice(0, cap)truncation sites — a self-contained string operation with no new untrusted-input sink, injection, or trust-boundary crossing.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedThe core
safeSliceimplementation is correct and the six converted sites are right — but the fix is incomplete against its own stated scope: the review-prompt builder still has raw.slice()sites that JSON-serialize the same way and can trigger the identical failure.Summary
Introduces
safeSlice()to back off one UTF-16 unit when a truncation cap lands on a high surrogate, and applies it at six slice sites feeding external/own review context.safeSliceitself is logically sound (head-slices can only orphan a high surrogate at the tail, which is exactly the case it guards). The problem is completeness: two live slice sites in the actual review-prompt builder — and one in a sibling AI prompt — were not converted and reproduce the same crash.Correctness / completeness
should-fix —
src/lib/ai/prompt.ts:494(and its twin at:241),input.reviewNotes.trim().slice(0, 8000). These are in the review-prompt path this PR targets, and the author's context note claims the fix was "applied at all six char-slice sites in the review-prompt path." ButreviewNotesis author-supplied free text that can contain emoji and exceed 8000 chars. A PR whose "Notes for reviewers" runs past 8000 chars with an astral char straddling index 8000 makes.slice(0, 8000)keep the lone high surrogate at the tail — the exact"unexpected end of hex escape"/"Invalid body"provider rejection the changelog says is fixed. The comment at:488–491confirms this is the only cap on that text ("capped only by the 8000-char slice"), so nothing downstream repairs it. Fix:safeSlice(input.reviewNotes.trim(), 8000)at both sites.should-fix —
src/lib/ai/conflict-prompt.ts:28,body.slice(0, SECTION_MAX)(80_000) inbuildConflictPrompt. Outside the PR's stated review-prompt scope, but the identical class in a different, un-guarded AI call: a conflicted file over 80 KB whose content has an emoji straddling index 80_000 yields a lone surrogate and the whole conflict-resolution request is rejected by the provider. SincesafeSliceis now exported, converting this is a one-line import + swap. At minimum worth a deliberate decision given the fix explicitly aims at "the class."nit —
src/lib/ai/own-distill.ts:44and:57re-slice each block (b.slice(0, DISTILL_BLOCK_CAP)/capped[...].slice(0, DISTILL_INPUT_CAP)) insidedistillOwnComments, which is part of the own-comments pipeline this PR touches, and can re-split a surrogate at a different offset than the upstreamcondenseOwnCommentcap. Lower severity because the entire distill call is wrapped in thetry/catchinown-context.ts:189–229that falls back to raw blocks, so a rejected request degrades rather than fails the review — but it's the same class and cheap to close.Notes (not findings)
safeSlicelogic is correct for all these head-slice callers:slice(0, n)can only leave a dangling high surrogate at the end (a trailing low surrogate always has its high half kept before it), so guarding onlymax-1covers it. Themax === 0/charCodeAt(-1) → NaNpath also degrades safely to"".safeSlicematches the documented testing posture rather than being an omission.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Thanks — accepted in full, and extended past the three sites called out to the whole class.
reviewNotes(:241/:494) — fixed; both nowsafeSlice(input.reviewNotes.trim(), 8000). You're right that my context note's "all six sites" undercounted: these two live in the review / PR-description builders and were missed.conflict-prompt.ts:28— fixed (safeSlice(body, SECTION_MAX)). Agreed it's the same class in a separate, un-guarded AI call; withsafeSliceexported it's a one-line swap.own-distill.ts:44/:57— fixed both. Confirmed your read that the caller'stry/catch(own-context.ts:189–229) degrades to raw blocks, so this was graceful-fail rather than hard-fail — closed anyway since it's the same class.Beyond those, the same pattern existed in model-bound builders neither review named —
review-tools.tscapHead(agent tool output), the issue-draft prompt (templates + rough notes), the plan prompt (issueBody/goal,topic), andreadme.ts'shardCutfallback — all nowsafeSlice. The Rust mirrormcp_server/generate.rsis already char-boundary-safe (head_slice+chars().take), so no Rust change.Both of your "not findings" hold:
safeSliceonly needs to guard the trailing high surrogate (a head-slice can't orphan a low half), and changelog-only is the right docs call for a truncation fix. PR title generalized to "AI prompts". Fixed in the next push.Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedThis PR routes all model-bound string head-caps through the new
safeSlice(), closing the completeness gaps the previous review flagged. The core logic is correct and the fix is now complete for its class — sound and mergeable.Resolved since last review
prompt.ts:243/:496(reviewNotes) — both nowsafeSlice(input.reviewNotes.trim(), 8000). Fixed (visible in diff).conflict-prompt.ts:28— nowsafeSlice(body, SECTION_MAX). Fixed.own-distill.ts:44/:57— both nowsafeSlice. Fixed.Copilot's finding (remaining raw
.sliceinprompt.ts/review-tools.ts/own-distill.ts/conflict-prompt.ts) is now resolved by this same push — not an issue against the current diff.Verification
I confirmed the fix is complete for the failure class and didn't over-reach:
safeSliceis correct for every caller — all are head-slicesslice(0, cap), which can only orphan a high surrogate at the tail; guardingcharCodeAt(max-1)covers exactly that, and one backoff suffices since a head-slice can create at most one lone surrogate. Themax === 0path degrades to""(charCodeAt(-1) → NaN, comparison false,end = 0)..slice(0, …)sites insrc/lib/ai. The survivors are all either boundary-safe (cut at a\n/space/index —readme.ts:257/261/272/276,truncate.ts:46,prompt.ts:716/762/867/869) or operate on model output being parsed/stored rather than request input (capDescriptionat:863, extracted title at:969/:1342) or are array/code-point slices (prompt.ts:203spreads to code points;:904/:1029/:1535are arrays). None of these serialize a fresh string into a model request body, so leaving them is right — thecapDescriptionexclusion in particular matches the author's recorded reasoning.No correctness, edge-case, or test concerns. The changelog fragment is the appropriate docs surface for a truncation bugfix, and the untested
safeSliceis consistent with the project's no-frontend-test-runner posture — no omission to flag.Posted by GitDesktop — AI output, verify before acting on it.
Ticket changed by: theBGuy