Skip to content

fix(runtime): keep MCP text clipping from splitting surrogate pairs - #4438

Open
shaynhornik wants to merge 3 commits into
apache:mainfrom
shaynhornik:fix/mcp-clip-surrogate
Open

fix(runtime): keep MCP text clipping from splitting surrogate pairs#4438
shaynhornik wants to merge 3 commits into
apache:mainfrom
shaynhornik:fix/mcp-clip-surrogate

Conversation

@shaynhornik

Copy link
Copy Markdown

Summary

clipModelText bounds MCP tool-result text with a raw .slice() at a UTF-16
index, so a clip boundary inside an astral character (any emoji) left an
unpaired high surrogate in the text block handed to the model — ill-formed
UTF-16 that any UTF-8 round trip (provider request body, durable storage)
mangles into U+FFFD. The fix drops the dangling high surrogate before
appending the truncation marker, the same boundary handling
local-memory.ts:311 already uses. Both call sites (appendText and the
resource-text path in summarizeNonVisualBlock) go through the shared helper,
so one change covers both.

Fixes #4435

Verification

  • Added a regression test in packages/runtime/src/__tests__/mcp-tools.test.ts
    that clips '🦊'.repeat(120_000) through the public
    buildMcpToolstoModelOutput path and asserts the result contains no
    unpaired surrogate, keeps the truncation marker, and stays within the
    200,000-char budget. It fails without the fix (7 pass / 1 fail) and passes
    with it (8 pass / 0 fail) via node --test packages/runtime/dist/__tests__/mcp-tools.test.js.
  • npm run lint, npm run format:check, npm run typecheck, and
    npm run build all pass locally (Node 22.21.0, Linux x64).
  • Not run: the graphical/E2E suites — this change is runtime-only text
    handling with no UI surface.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code (Fable 5) found the bug and wrote the fix and
regression test at my direction; I reviewed the change, ran the verification,
and own the submission. The affected commit carries a Generated-by trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 1, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct — the marker is all-BMP so the limit <= TRUNCATION_MARKER.length branch can't split a pair either, and both call sites go through the one helper.

One thing before it lands: this is the fourth hand-written copy of "don't end a truncation on a lone high surrogate."

  • packages/core/src/local-memory.ts:311 — the same regex
  • packages/mcp/src/sep-2243.ts:285 — same rule, written differently
  • packages/runtime/src/bots/telegram-bridge.ts:74 — code-point iteration
  • packages/core/src/thread-search.ts:596Array.from

Five sites, four implementations, and the next clip site will guess again. packages/core/src/text-sanitize.ts is already the UTF-16 authority here — its own comment explains that a surrogate pair counts as one — so exporting a safe-truncate there and pointing this call site at it would make the change net-negative instead of net-additive.

Not blocking if you'd rather land the bug fix as-is, but if you take the helper, converting local-memory.ts:311 in the same PR would be the natural pair.

@shaynhornik

Copy link
Copy Markdown
Author

Took the helper: truncateUtf16Safe is now exported from text-sanitize with direct unit tests, and both this call site and local-memory.ts point at it (the existing local-memory boundary test still passes unchanged). I left sep-2243, telegram-bridge, and thread-search alone since their budgets are code-point/chunking shaped rather than a straight code-unit cap — happy to do those in a follow-up if you'd like.

@shaynhornik

Copy link
Copy Markdown
Author

Went back over the other three sites after your list. The sep-2243 warning ellipsis and the Telegram prefix are both plain code-unit caps, so my "code-point/chunking shaped" was wrong for those two. Both now call truncateUtf16Safe (e4e2979), which drops the Telegram-local prefix walk and its direct test (the same cases live in text-sanitize.test.ts). thread-search's capCodePoints stays: its budget is code points rather than code units, so it's a different contract, not another copy. That leaves two implementations for two different units. mcp 179/179, runtime and core suites green, biome and the ASF header check clean.

@github-actions github-actions Bot added effort/M Under 500 readable lines and removed effort/S Under 100 readable lines labels Sep 2, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving e4e2979. truncateUtf16Safe in text-sanitize is now the one place the surrogate rule lives, with its own tests, and mcp-tools, sep-2243, telegram-bridge and local-memory all call it; the Telegram-local code-point walk and its test are gone with it. Keeping capCodePoints in thread-search is right, its budget is code points, a different contract rather than a fourth copy. Thank you for going back over the other sites. I approved the pending CI runs; merge once they are green.

简体中文

批准合并。代理对规则收进 truncateUtf16Safe 一处,四个调用点都接上,Telegram 本地实现删掉;thread-search 按 code point 计数是另一份契约,留着对。CI 已放行。

@Astro-Han

Copy link
Copy Markdown
Contributor

The Test CI planner failure is not yours: the run was approved today against a merge ref computed on 1 September, before #4523 changed ci.yml, and a rerun reuses that merge commit. A rebase onto main will trigger a fresh run; nothing else needed.

shaynhornik and others added 3 commits September 2, 2026 13:49
clipModelText cut at a raw UTF-16 index, so a clip boundary inside an
astral character left an unpaired high surrogate in the text block
handed to the model; any UTF-8 round trip turns that into U+FFFD.
Drop the dangling high surrogate before appending the truncation
marker, matching the boundary handling local-memory.ts already uses.

Fixes apache#4435

Generated-by: Claude Code (Fable 5)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Export truncateUtf16Safe from text-sanitize, the UTF-16 authority, and point the MCP model-text clip and the local-memory prompt cap at it so the boundary rule has one implementation. Suffix and marker policy stay with the callers.

Generated-by: Claude Code (Fable 5)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The MCP header-warning ellipsis in sep-2243 and the Telegram draft/chunk prefix were two more hand-written copies of the same boundary rule on a UTF-16 code-unit budget. Both now call the shared helper, which retires the Telegram-local prefix walk and its direct test; the same cases live in text-sanitize.test.ts. thread-search's capCodePoints stays as is: its budget is code points, not code units, so it is a different contract rather than another copy.

Generated-by: Claude Code (Fable 5.1)
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FTqr6ZjXfNthXT719ag7Qc
@shaynhornik

Copy link
Copy Markdown
Author

Rebased onto main (a00d1f3); core, mcp and runtime suites still green locally after the rebase.

@shaynhornik
shaynhornik force-pushed the fix/mcp-clip-surrogate branch from e4e2979 to a00d1f3 Compare September 2, 2026 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(runtime): MCP model-text clipping can split a surrogate pair and emit ill-formed text

2 participants