Skip to content

fix(control-ui-assets): keep emoji / surrogate pairs intact during last-line truncation#101591

Merged
steipete merged 2 commits into
openclaw:mainfrom
maweibin:fix/control-ui-assets-emoji-truncation
Jul 7, 2026
Merged

fix(control-ui-assets): keep emoji / surrogate pairs intact during last-line truncation#101591
steipete merged 2 commits into
openclaw:mainfrom
maweibin:fix/control-ui-assets-emoji-truncation

Conversation

@maweibin

@maweibin maweibin commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

summarizeCommandOutput in src/infra/control-ui-assets.ts truncates text with .slice() which counts UTF-16 code units. When a surrogate-pair character (emoji) straddles the cut boundary, .slice() produces a lone surrogate (e.g. \ud83d) that renders as `` in terminal output.

This is user-visible: the truncated text reaches user-facing CLI, status, or log output, so any content containing emoji near the 238-character boundary can hit this.

Why This Change Was Made

Replace .slice() with truncateUtf16Safe() from @openclaw/normalization-core/utf16-slice so the truncation counts full code points instead of UTF-16 code units. This matches the already-merged PR #101517 (session-cost-usage.ts) and the canonical shortenText helper in src/commands/text-format.ts:5.

User Impact

Users will no longer see `` replacement characters in this output when emoji appear near truncation boundaries. Existing column width / character caps are preserved — the broken surrogate is dropped cleanly rather than rendered as a replacement character.

Evidence

Real environment tested: local OpenClaw source checkout, Node v24.13.1, PR head.

Exact steps after this patch:

node --import tsx -e "
const { truncateUtf16Safe } = await import('@openclaw/normalization-core/utf16-slice');
const content = 'y'.repeat(238) + '\u{1F680}xx';
const before = content.slice(0, 238 + 1) + '…';
const after = truncateUtf16Safe(content, 238 + 1) + '…';
const t = (s) => /[\uD800-\uDBFF](?![\uDC00-\uDFFF])/.test(s);
console.log('BEFORE (.slice 238):', JSON.stringify(before.slice(-12)), 'lone:', t(before));
console.log('AFTER  (truncateUtf16Safe 238):', JSON.stringify(after.slice(-12)), 'lone:', t(after));
"

Evidence after fix:

BEFORE (.slice 238):           "yyyyyyyyyy\ud83d…"  lone: true
AFTER  (truncateUtf16Safe 238): "yyyyyyyyyyy…"       lone: false

Observed result after fix: truncateUtf16Safe preserves full code points; no lone surrogate reaches output. The broken surrogate pair is dropped cleanly rather than producing ``.

What was not tested: unrelated truncation sites in other source files remain unchanged.

Regression Test Plan

node scripts/run-vitest.mjs src/infra/control-ui-assets.test.ts --run

Includes a focused regression test that verifies surrogate pair preservation at the truncation boundary.

AI-assisted.

…st-line truncation

.slice(0, 239) counts UTF-16 code units; an emoji straddling the
cut point produces a lone surrogate rendering as  in WebUI assets.

Use [...last] spread to count full code points instead.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@maweibin
maweibin force-pushed the fix/control-ui-assets-emoji-truncation branch from b4b422e to 8b253d4 Compare July 7, 2026 11:17
@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 7, 2026, 7:28 AM ET / 11:28 UTC.

Summary
The PR replaces raw last-line truncation in Control UI build-failure diagnostics with truncateUtf16Safe and adds a mocked regression test for emoji-boundary truncation.

PR surface: Source +1, Tests +40. Total +41 across 2 files.

Reproducibility: yes. source-level: current main uses last.slice(0, 239) after last.length > 240, so a surrogate pair crossing that boundary can produce a dangling high surrogate. I did not run tests because this review was required to keep the checkout read-only.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Next step before merge

  • No repair lane is needed because review found no actionable patch defect; keep it in the normal maintainer review and merge-gate flow.

Security
Cleared: The diff only changes TypeScript string truncation and a focused test, with no dependency, workflow, secret, permission, network, or package-resolution surface changed.

Review details

Best possible solution:

Land the helper-based truncation and regression test once normal exact-head CI, base freshness, and maintainer review gates clear.

Do we have a high-confidence way to reproduce the issue?

Yes, source-level: current main uses last.slice(0, 239) after last.length > 240, so a surrogate pair crossing that boundary can produce a dangling high surrogate. I did not run tests because this review was required to keep the checkout read-only.

Is this the best way to solve the issue?

Yes. The latest head uses the existing truncateUtf16Safe helper instead of a code-point spread, preserving the existing UTF-16 cap while preventing malformed surrogate output.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 3901094ace58.

Label changes

Label changes:

  • add P2: This is a narrow user-visible diagnostic formatting bug with limited blast radius and a focused PR fix.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal output showing the old truncation leaves a lone surrogate and the safe truncation does not, plus focused test output for the changed path.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.

Label justifications:

  • P2: This is a narrow user-visible diagnostic formatting bug with limited blast radius and a focused PR fix.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal output showing the old truncation leaves a lone surrogate and the safe truncation does not, plus focused test output for the changed path.
Evidence reviewed

PR surface:

Source +1, Tests +40. Total +41 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 2 1 +1
Tests 1 40 0 +40
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 42 1 +41

What I checked:

  • Repository policy read: Root AGENTS.md was read fully and applied; its PR review policy required checking caller/callee paths, sibling truncation behavior, current main, proof, and history rather than reviewing only the diff. (AGENTS.md:1)
  • Current main bug path: Current main still truncates the last normalized build-error line with last.slice(0, 239), which can return a dangling surrogate when a pair crosses that boundary. (src/infra/control-ui-assets.ts:288, 3901094ace58)
  • PR head implementation: The latest PR head imports truncateUtf16Safe from the existing normalization package and uses it in the same truncation branch, preserving the 239-code-unit cap while avoiding split surrogate pairs. (src/infra/control-ui-assets.ts:286, ae267db9a0c5)
  • Helper contract: truncateUtf16Safe clamps to a UTF-16 limit and delegates to sliceUtf16Safe, whose boundary checks back off from dangling surrogate halves. (packages/normalization-core/src/utf16-slice.ts:44, 3901094ace58)
  • Export contract: The normalization-core package already exports the ./utf16-slice subpath used by the PR, matching existing imports in other source files. (packages/normalization-core/package.json:47, 3901094ace58)
  • Regression coverage: The PR adds a focused ensureControlUiAssetsBuilt test that mocks a failing UI build whose stderr crosses an emoji boundary and expects the diagnostic to stay within the UTF-16 cap without including the split emoji. (src/infra/control-ui-assets.test.ts:186, ae267db9a0c5)

Likely related people:

  • steipete: Current-main blame points to this account for the control-ui-assets diagnostic helper and shared UTF-16 helper, and the latest PR-head commit also came from this account to preserve the UTF-16 cap. (role: introduced behavior and recent area contributor; confidence: high; commits: 6125dd95e682, ae267db9a0c5; files: src/infra/control-ui-assets.ts, src/infra/control-ui-assets.test.ts, packages/normalization-core/src/utf16-slice.ts)
  • maweibin: This account authored the current PR’s first commit and also authored the merged related infra PR for surrogate-safe session-cost truncation, so they are connected to the same bug class beyond only opening this PR. (role: adjacent UTF-16 truncation contributor; confidence: medium; commits: 8b253d44871c, afdb9fd26408; files: src/infra/control-ui-assets.ts, src/infra/session-cost-usage.ts, src/infra/session-cost-usage.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Land-ready at exact head ae267db9a0c5cce6099f8584033eebd74fc02469.

Maintainer review replaced the proposed code-point truncation with the canonical UTF-16-safe helper, preserving the existing 239-code-unit payload plus ellipsis rather than allowing non-BMP input to exceed the 240-code-unit diagnostic cap. The added regression drives ensureControlUiAssetsBuilt through a failed automatic build and asserts the exact bounded error message when an emoji straddles the cutoff.

Validation:

  • canonical formatting and git diff --check
  • fresh Codex autoreview: clean, correctness confidence 0.91
  • exact-head hosted CI run 28862256403: success; 70 passing PR checks, no failures or pending checks
  • scripts/pr review-validate-artifacts 101591
  • OPENCLAW_TESTBOX=1 scripts/pr prepare-run 101591

The PR body was corrected to describe the actual failed-build diagnostic surface. No known proof gaps.

@steipete
steipete merged commit f6ffe89 into openclaw:main Jul 7, 2026
107 checks passed
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 8, 2026
…st-line truncation (openclaw#101591)

* fix(control-ui-assets): keep emoji / surrogate pairs intact during last-line truncation

.slice(0, 239) counts UTF-16 code units; an emoji straddling the
cut point produces a lone surrogate rendering as  in WebUI assets.

Use [...last] spread to count full code points instead.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

* fix(control-ui-assets): preserve diagnostic length cap

---------

Co-authored-by: Claude Opus 4.8 <[email protected]>
Co-authored-by: Peter Steinberger <[email protected]>
giodl73-repo pushed a commit to giodl73-repo/openclaw that referenced this pull request Jul 8, 2026
…st-line truncation (openclaw#101591)

* fix(control-ui-assets): keep emoji / surrogate pairs intact during last-line truncation

.slice(0, 239) counts UTF-16 code units; an emoji straddling the
cut point produces a lone surrogate rendering as  in WebUI assets.

Use [...last] spread to count full code points instead.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

* fix(control-ui-assets): preserve diagnostic length cap

---------

Co-authored-by: Claude Opus 4.8 <[email protected]>
Co-authored-by: Peter Steinberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants