Skip to content

fix(msteams): drop dangling surrogates in message previews#98058

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

fix(msteams): drop dangling surrogates in message previews#98058
wangmiao0668000666 wants to merge 1 commit into
openclaw:mainfrom
wangmiao0668000666:fix/msteams-preview-utf16

Conversation

@wangmiao0668000666

@wangmiao0668000666 wangmiao0668000666 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

extensions/msteams/src/monitor-handler/message-handler.ts:250, :251, and :505 use raw .slice(0, N) on the inbound message body for debug log previews and the system event label. 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. The :505 label flows into core.system.enqueueSystemEvent and then downstream, where the lone surrogate can also break string comparisons.

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. msteams was the remaining gap.

The three call sites map to the two helpers:

  • L250 / L251 (rawText, text) — bare values into a log field → truncateUtf16Safe(value, 50)
  • L505 (preview, with a replace(/\s+/g, " ") upstream) — system event label → sliceUtf16Safe(replaced, 0, 160) so the call site still reads as a slice but the trailing surrogate is dropped whole

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:

[rawText 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

[text 50-char]
  BEFORE .slice(0, 50): length=50 tail=[0061 0061 0061 d83c]
  AFTER  safe fn:                                length=49 tail=[0061 0061 0061 0061]

[preview 160-char]
  BEFORE .slice(0, 160): length=160 tail=[0061 0061 0061 d83c]
  AFTER  safe fn:                                 length=159 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/msteams/src/monitor-handler/message-handler.dm-media.test.ts --reporter=verbose
 ✓ |extension-msteams| extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts > translateMSTeamsDmConversationIdForGraph > translates a: conversation ID to Graph format for DMs 240ms
 ✓ |extension-msteams| extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts > translateMSTeamsDmConversationIdForGraph > passes through non-a: conversation IDs unchanged 1ms
 ✓ |extension-msteams| extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts > translateMSTeamsDmConversationIdForGraph > passes through when aadObjectId is missing 1ms
 ✓ |extension-msteams| extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts > translateMSTeamsDmConversationIdForGraph > passes through when appId is missing 1ms
 ✓ |extension-msteams| extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts > translateMSTeamsDmConversationIdForGraph > passes through for non-DM conversations even with a: prefix 1ms
 ✓ |extension-msteams| extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts > message-handler preview UTF-16 truncation > truncateUtf16Safe drops a surrogate pair straddling the 50-char boundary (rawText path) 1ms
 ✓ |extension-msteams| extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts > message-handler preview UTF-16 truncation > truncateUtf16Safe is a pass-through for plain ASCII (text path) 1ms
 ✓ |extension-msteams| extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts > message-handler preview UTF-16 truncation > sliceUtf16Safe preserves an emoji that sits entirely before the 160-char cut (preview path) 1ms
 ✓ |extension-msteams| extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts > message-handler preview UTF-16 truncation > sliceUtf16Safe drops a trailing surrogate straddling the 160-char cut (preview path) 1ms

 Test Files  1 passed (1)
      Tests  9 passed (9)
   Duration  1.71s

[test] passed 1 Vitest shard in 12.37s
  • node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.scripts.json extensions/msteams/src/monitor-handler/message-handler.ts extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts → EXIT=0
  • git diff --check openclaw/main...HEAD → clean
  • git diff --numstat openclaw/main...HEAD → 2 files changed (1 source + 1 test)

Diff scope

 extensions/msteams/src/monitor-handler/message-handler.ts            | 10 ++++--
 extensions/msteams/src/monitor-handler/message-handler.dm-media.test.ts | 38 ++++++++++++++++++++++
 2 files changed, 45 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 or two code units 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 MS Teams 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). msteams is the new addition.

Related

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: XS 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:12 AM ET / 07:12 UTC.

Summary
The PR replaces three raw MS Teams inbound preview slices with surrogate-safe SDK helpers and adds focused UTF-16 boundary tests.

PR surface: Source +4, Tests +38. Total +42 across 2 files.

Reproducibility: yes. source-reproducible: current main and v2026.6.10 use raw .slice(...) at the affected MS Teams preview boundaries, so an astral character straddling the limit can leave a lone surrogate. I did not execute the handler in this read-only review, but the PR body includes copied before/after boundary output.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
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 has no actionable patch findings and can proceed through normal maintainer review and exact-head checks.

Security
Cleared: No security or supply-chain concern found; the diff only changes MS Teams TypeScript preview truncation and a colocated Vitest file, with no dependencies, workflows, lockfiles, secrets, downloads, or code-execution surface.

Review details

Best possible solution:

Land the localized MS Teams preview truncation fix after normal maintainer review; keep broader sibling-channel UTF-16 cleanup in separate channel-specific PRs.

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

Yes, source-reproducible: current main and v2026.6.10 use raw .slice(...) at the affected MS Teams preview boundaries, so an astral character straddling the limit can leave a lone surrogate. I did not execute the handler in this read-only review, but the PR body includes copied before/after boundary output.

Is this the best way to solve the issue?

Yes. Reusing truncateUtf16Safe and sliceUtf16Safe from the public plugin SDK at the exact preview sites is the narrowest maintainable fix; a custom local truncator would duplicate an existing helper. The PR body overstates the system-event-label path because preview is only used in verbose logging, but the code change is still the right fix for the actual log-preview bug.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P3: This is a low-blast-radius MS Teams log-preview encoding fix with no message-delivery, config, auth, migration, or security-surface change.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied before/after boundary-probe output plus focused test and lint output; that is sufficient for this non-visual string-preview behavior, though it is not a live MS Teams message-flow proof.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit 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-probe output plus focused test and lint output; that is sufficient for this non-visual string-preview behavior, though it is not a live MS Teams message-flow proof.

Label justifications:

  • P3: This is a low-blast-radius MS Teams log-preview encoding fix with no message-delivery, config, auth, migration, or security-surface change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit 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-probe output plus focused test and lint output; that is sufficient for this non-visual string-preview behavior, though it is not a live MS Teams message-flow proof.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied before/after boundary-probe output plus focused test and lint output; that is sufficient for this non-visual string-preview behavior, though it is not a live MS Teams message-flow proof.
Evidence reviewed

PR surface:

Source +4, Tests +38. Total +42 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 7 3 +4
Tests 1 38 0 +38
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 45 3 +42

What I checked:

Likely related people:

  • masatohoshino: git blame on the current MS Teams raw preview slice lines points to the current file snapshot commit, and live PR metadata for the corresponding merged PR lists this person as author. (role: recent file snapshot contributor; confidence: medium; commits: 888f399499c4, 74ad546d24b2; files: extensions/msteams/src/monitor-handler/message-handler.ts)
  • vincentkoc: Live GitHub metadata lists this person as the merger of the PR associated with the current file snapshot that carries the raw preview slice lines. (role: merger of related current-main snapshot; confidence: low; commits: 888f399499c4, 74ad546d24b2; files: extensions/msteams/src/monitor-handler/message-handler.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: msteams Channel integration: msteams 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: XS 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