Skip to content

fix(imessage): drop dangling surrogates in debounced merge preview and probe snippet#98065

Closed
wangmiao0668000666 wants to merge 1 commit into
openclaw:mainfrom
wangmiao0668000666:fix/imessage-preview-utf16
Closed

fix(imessage): drop dangling surrogates in debounced merge preview and probe snippet#98065
wangmiao0668000666 wants to merge 1 commit into
openclaw:mainfrom
wangmiao0668000666:fix/imessage-preview-utf16

Conversation

@wangmiao0668000666

@wangmiao0668000666 wangmiao0668000666 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

extensions/imessage/src/monitor/monitor-provider.ts:731 and extensions/imessage/src/probe.ts:161 use raw .slice(0, N) on inbound message bodies for debug-log previews and JSONL-parse-failure diagnostics. When an emoji or other astral character straddles position N, the slice keeps only the high-surrogate half (0xD83C for 🎉), producing malformed UTF-16 (?/�) in verbose logs.

Note: extensions/imessage/src/monitor/coalesce.ts:139 already uses sliceUtf16Safe (commit 352f47f888 by llagy009 on 2026-06-29). This PR covers the two remaining iMessage sites that still used raw .slice().

Why This Change Was Made

The plugin SDK provides sliceUtf16Safe / truncateUtf16Safe in openclaw/plugin-sdk/text-utility-runtime for exactly this pattern. Alix-007 landed the same fix for Twitch (#98008) and Signal (#97982); their PR bodies explicitly list "Discord, Feishu, Telegram, Signal, Mattermost, Slack" as already using these helpers. iMessage had one site fixed (coalesce.ts) and two still raw; this PR closes the remaining gap.

The two call sites map to the two helpers:

  • monitor-provider.ts:731 — text.slice(0, 50) followed by a conditional "..." ellipsis at L732. The conditional text.length > 50 must keep firing against the original text.length, so the safe slice returns 49 when a lone surrogate is dropped (one code unit less than text.length). → sliceUtf16Safe(text, 0, 50) keeps the existing ellipsis logic intact.
  • probe.ts:161 — lines[0]?.slice(0, 120) standalone diagnostic snippet. The optional chain becomes a ternary because truncateUtf16Safe doesn't accept undefined. → lines[0] ? truncateUtf16Safe(lines[0], 120) : undefined.

Evidence

Boundary probe — input is 🎉 (U+1F389, surrogate pair 0xD83C 0xDF89) placed immediately after N-1 ASCII a characters so it straddles the truncation point:

[monitor-provider 50-char]
  BEFORE .slice(0, 50):  length=50  tail=[0061 0061 0061 d83c]   ← lone high surrogate, malformed UTF-16
  AFTER  safe fn:          length=49  tail=[0061 0061 0061 0061]   ← clean, emoji dropped whole

[probe 120-char]
  BEFORE .slice(0, 120): length=120 tail=[0061 0061 0061 d83c]
  AFTER  safe fn:          length=119 tail=[0061 0061 0061 0061]

🎉 = U+1F389, high surrogate 0xD83C, low surrogate 0xDF89. The lone d83c half in the BEFORE rows is the malformed UTF-16 the previous code emits; the AFTER rows show a clean ASCII tail with the surrogate pair dropped whole.

After-fix Proof

  • node scripts/run-vitest.mjs extensions/imessage/src/monitor/monitor-provider.truncation.test.ts extensions/imessage/src/probe.test.ts --reporter=verbose
 ✓ |extension-imessage| extensions/imessage/src/monitor/monitor-provider.truncation.test.ts > monitor-provider debounced-merge preview truncation > sliceUtf16Safe drops a trailing surrogate straddling the 50-char boundary 449ms
 ✓ |extension-imessage| extensions/imessage/src/monitor/monitor-provider.truncation.test.ts > monitor-provider debounced-merge preview truncation > ellipsis marker is appended when the original text exceeded 50 chars (no surrogate regression) 1ms
 ✓ |extension-imessage| extensions/imessage/src/monitor/monitor-provider.truncation.test.ts > monitor-provider debounced-merge preview truncation > empty input stays empty 1ms
 ✓ |extension-imessage| extensions/imessage/src/monitor/monitor-provider.truncation.test.ts > monitor-provider debounced-merge preview truncation > emoji fully inside the window is preserved (no false-positive drops) 1ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > imessageRpcSupportsMethod > returns false when the bridge is not available 66ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > imessageRpcSupportsMethod > returns false when status is undefined 1ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > imessageRpcSupportsMethod > returns true when the requested method is in the explicit rpcMethods list 1ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > imessageRpcSupportsMethod > returns false for a method not in the explicit rpcMethods list 1ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > imessageRpcSupportsMethod > falls back to the foundational set when rpcMethods is empty (older imsg builds) 1ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > imessageRpcSupportsMethod > gates newer methods off when rpcMethods is empty (forces upgrade for typing/read/group) 1ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > iMessage private API status cache > drops expiring private API status when the current clock is not a valid date timestamp 2ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > iMessage private API status cache > does not cache private API status with an invalid expiry timestamp 0ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > probe first-line snippet truncation > truncateUtf16Safe drops a trailing surrogate straddling the 120-char boundary 1ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > probe first-line snippet truncation > plain ASCII under the cap passes through unchanged 0ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > probe first-line snippet truncation > undefined input is preserved (no truncation attempted) 0ms
 ✓ |extension-imessage| extensions/imessage/src/probe.test.ts > probe first-line snippet truncation > emoji fully inside the 120-char window is preserved 0ms

 Test Files  2 passed (2)
      Tests  16 passed (16)
   Duration  9.90s

[test] passed 1 Vitest shard in 20.68s
  • node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.scripts.json extensions/imessage/src/monitor/monitor-provider.ts extensions/imessage/src/probe.ts extensions/imessage/src/probe.test.ts extensions/imessage/src/monitor/monitor-provider.truncation.test.ts → EXIT=0
  • git diff --check openclaw/main...HEAD → clean
  • git diff --numstat openclaw/main...HEAD → 4 files changed (2 source + 2 test)

Diff scope

 extensions/imessage/src/monitor/monitor-provider.ts                  |  7 +++--
 extensions/imessage/src/monitor/monitor-provider.truncation.test.ts  | 38 +++++++++++++++++++ (new)
 extensions/imessage/src/probe.ts                                     |  3 +-
 extensions/imessage/src/probe.test.ts                                | 33 ++++++++++++++++
 4 files changed, 77 insertions(+), 3 deletions(-)

Security & Privacy

No user-facing behavior change for ASCII input. The previews render identically for non-emoji text. For emoji-straddling input, the preview is one code unit shorter because the surrogate pair is dropped whole instead of leaving a lone high surrogate — this is the intended UTF-16 boundary fix, not a content loss.

Compatibility

Plugin SDK export (openclaw/plugin-sdk/text-utility-runtime) is already in package.json exports for @openclaw/plugin-sdk (re-exported from src/plugin-sdk/text-utility-runtime.ts). No new dependency added at the package level. The helpers are dependency-free per src/shared/utf16-slice.ts:1-5 (no node: imports), so they bundle cleanly into both server and UI builds.

What was not tested

Live iMessage message flow with a real surrogate-pair payload. The fix is a one-line surrogate-pair guard per call site; the helpers themselves are covered by unit tests in src/shared/utf16-slice.ts. Integration coverage at this layer is intentionally out of scope — Alix-007's PRs #98008 (Twitch) and #97982 (Signal) followed the same boundary and were accepted on the same evidence.

Other channel plugins that already use the helper

Discord, Feishu, Telegram, Signal (#97982), Mattermost, Slack, Twitch (#98008), plus iMessage coalesce (commit 352f47f). iMessage debounced-merge and probe are the new additions.

Related

@openclaw-barnacle openclaw-barnacle Bot added channel: imessage Channel integration: imessage size: S labels Jun 30, 2026
@clawsweeper

clawsweeper Bot commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 30, 2026, 3:21 AM ET / 07:21 UTC.

Summary
The PR replaces two iMessage raw preview/snippet slices with UTF-16-safe Plugin SDK helpers and adds focused boundary tests.

PR surface: Source +4, Tests +70. Total +74 across 4 files.

Reproducibility: yes. at source level. Current main uses raw UTF-16 .slice() at the two affected iMessage preview/snippet sites, and an astral character straddling the code-unit cap can leave a dangling high surrogate.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
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 ClawSweeper repair lane is needed; the branch is already a focused implementation with no actionable review findings.

Security
Cleared: No security or supply-chain concern found; the diff only changes iMessage TypeScript string truncation and local Vitest coverage.

Review details

Best possible solution:

Land the focused iMessage helper substitution and regression tests after ordinary maintainer review and required-check gating.

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

Yes, at source level. Current main uses raw UTF-16 .slice() at the two affected iMessage preview/snippet sites, and an astral character straddling the code-unit cap can leave a dangling high surrogate.

Is this the best way to solve the issue?

Yes. Reusing the existing Plugin SDK UTF-16 helpers at the exact iMessage call sites is the narrow maintainable fix; custom local truncation logic would duplicate an established shared helper.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 56c2d637d940.

Label changes

Label changes:

  • add P3: This fixes low-blast-radius iMessage verbose/debug diagnostic string encoding without changing message delivery, config, auth, storage, or migration behavior.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied before/after boundary output plus focused test output showing the raw slice leaves a lone surrogate and the helper avoids it for this non-visual diagnostic behavior.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes copied before/after boundary output plus focused test output showing the raw slice leaves a lone surrogate and the helper avoids it for this non-visual diagnostic behavior.

Label justifications:

  • P3: This fixes low-blast-radius iMessage verbose/debug diagnostic string encoding without changing message delivery, config, auth, storage, or migration behavior.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes copied before/after boundary output plus focused test output showing the raw slice leaves a lone surrogate and the helper avoids it for this non-visual diagnostic behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied before/after boundary output plus focused test output showing the raw slice leaves a lone surrogate and the helper avoids it for this non-visual diagnostic behavior.
Evidence reviewed

PR surface:

Source +4, Tests +70. Total +74 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 2 7 3 +4
Tests 2 70 0 +70
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 77 3 +74

What I checked:

Likely related people:

  • omarshahine: GitHub path history shows multiple recent iMessage monitor-provider and coalescing changes, including the split-send coalescing work that owns the affected debounced merge path. (role: recent iMessage monitor/coalescing contributor; confidence: high; commits: a0714a3d6855, 9caff5f873cd, fc6400ede389; files: extensions/imessage/src/monitor/monitor-provider.ts, extensions/imessage/src/monitor/coalesce.ts)
  • llagy009: They authored the merged iMessage coalescer fix that changed a nearby raw slice to sliceUtf16Safe for the same surrogate-boundary problem class. (role: adjacent UTF-16 boundary fix author; confidence: high; commits: 352f47f888de; files: extensions/imessage/src/monitor/coalesce.ts)
  • steipete: Recent history shows direct iMessage probe cache work and broader Plugin SDK/text utility surface maintenance near the helper path used by this PR. (role: recent probe and Plugin SDK area contributor; confidence: medium; commits: becd45325bc5, 5269924ff8f5, 827b0de0ce74; files: extensions/imessage/src/probe.ts, src/plugin-sdk/text-utility-runtime.ts)
  • scoootscooob: History shows the iMessage channel was moved into the bundled plugin boundary in their refactor, which is relevant to ownership of the current extension surface. (role: iMessage extension migration contributor; confidence: medium; commits: 0ce23dc62d37; files: extensions/imessage/src/monitor/monitor-provider.ts, extensions/imessage/src/probe.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.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jun 30, 2026
This was referenced Jun 30, 2026
Closed
Closed
Closed
Closed
Closed
@vincentkoc vincentkoc self-assigned this Jun 30, 2026
@vincentkoc

vincentkoc commented Jun 30, 2026

Copy link
Copy Markdown
Member

Closing this as superseded by the broader UTF-16-safe truncation fix now on main: c16bb87.

I kept this as one canonical pass instead of landing the one-off PRs separately so Browser, Discord, Feishu, iMessage, MS Teams, Signal, Twitch, and Voice Call preview/error truncation all use the shared UTF-16 helpers consistently. Thanks for flagging this surface.

Proof for the landed commit:

  • focused Vitest: shared UTF helper plus Browser, Discord, Feishu, iMessage, MS Teams, Signal, Twitch, and Voice Call touched suites
  • autoreview: clean on the UTF commit
  • Crabbox/Azure changed gate: run_33aba0edad9a, lease cbx_09cf3426b424, passed OPENCLAW_TESTBOX=1 node scripts/crabbox-wrapper.mjs run -- env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed

@vincentkoc vincentkoc closed this Jun 30, 2026
@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jul 1, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

channel: imessage Channel integration: imessage P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wangmiao0668000666 @vincentkoc