Skip to content

fix(qqbot): use UTF-16-safe truncation for messaging outbound previews#101410

Closed
wangmiao0668000666 wants to merge 3 commits into
openclaw:mainfrom
wangmiao0668000666:fix/qqbot-utf16-messaging-outbound
Closed

fix(qqbot): use UTF-16-safe truncation for messaging outbound previews#101410
wangmiao0668000666 wants to merge 3 commits into
openclaw:mainfrom
wangmiao0668000666:fix/qqbot-utf16-messaging-outbound

Conversation

@wangmiao0668000666

@wangmiao0668000666 wangmiao0668000666 commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • AI-assisted with Codex.
  • QQBot outbound-deliver and outbound-media-send now use UTF-16-safe truncation instead of raw .slice(0, N) so emoji and CJK surrogate pairs in sent-chunk previews and media-path error messages are not split mid-pair.
  • Replaces raw UTF-16 code-unit truncation with the existing truncateUtf16Safe(text, N) helper from openclaw/plugin-sdk/text-utility-runtime.
  • Keeps the existing cap values; out of scope: unrelated slices, response-read bounding, and outbound.ts sites (those will follow in PR 1b).

Linked context

  • No linked issue.
  • Related to the existing UTF-16-safe truncation hardening pattern already used across OpenClaw. Discord, Feishu, Telegram, Signal, Mattermost, Slack, Twitch, msteams, iMessage, voice-call, and (already in qqbot) engine/tools/remind-logic.ts (llagy009 #96575) all use the same truncateUtf16Safe helper. QQBot outbound-deliver / outbound-media-send is the new addition.

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: QQBot outbound-deliver debugLog previews (Sent text chunk ..., Sent text-only chunk ..., Extracted HTTP image from markdown, Extracted bare image URL, Forwarded tool media, Formatted HTTP image, Updated image with size, Sent image via sendPhoto, Collected media URL) and outbound-media-send error returns (Failed to download image, Unsupported image source, Failed to download audio/video/file, fallback: download also failed) now truncate on a UTF-16 code-unit boundary instead of splitting emoji/CJK surrogate pairs.
  • Real environment tested: local OpenClaw source checkout on Node v22.22.0.
  • Exact steps or command run after this patch: 
    node scripts/run-vitest.mjs extensions/qqbot/src/engine/messaging/outbound-deliver.test.ts extensions/qqbot/src/engine/messaging/outbound-media-send.test.ts --run
  • Evidence after fix: focused test passed, 25 tests in outbound-media-send.test.ts (24 pre-existing + 1 runtime UTF-16 proof test). New cap-precise + runtime evidence combined across both touched files, full run is reproducible via node scripts/run-vitest.mjs extensions/qqbot/src/engine/messaging/outbound-deliver.test.ts extensions/qqbot/src/engine/messaging/outbound-media-send.test.ts --run.
  • Observed result after fix: the touched QQBot outbound-error path no longer emits a lone surrogate when truncating text at an emoji or CJK boundary. Each new inline test asserts that the output contains no lone high or low surrogate code unit, AND exercises the EXACT production template literal at outbound-media-send.ts:460.
  • What was not tested: full end-to-end QQBot provider/channel delivery was not run; this is a focused text-boundary fix on internal debugLog previews and error returns.
  • Proof limitations or environment constraints: proof is scoped to the affected local runtime/test path and does not require external services.
  • Before evidence (runtime vitest output, outbound-media-send runtime test): 
    === PR 1a runtime proof: outbound-media-send Failed-to-download-image error ===
input mediaPath (85 code units): /tmp/uploads/xxx...xxx🎉tail
slice(0, 80) hex ends at U+D83C (HIGH surrogate of 🎉, no matching LOW)
truncateUtf16Safe(mediaPath, 80) drops the trailing 🎉 pair at the boundary
BEFORE full message (106 code units):
  Failed to download image: /tmp/uploads/xxx...xxx�          ← lone HIGH surrogate rendered as REPLACEMENT CHARACTER (�)
  hex: U+0046 ... U+D83C                                          ← final code unit is the HIGH surrogate alone
AFTER  full message (105 code units):
  Failed to download image: /tmp/uploads/xxx...xxx                ← clean 79-ASCII prefix
  hex: U+0046 ... U+0078                                          ← no D83C, no DE00
BEFORE contains lone surrogate? true
AFTER  contains lone surrogate? false
    When the source site is reverted to .slice(0, 80) on the same input, the resulting user-facing Failed to download image: error message contains a lone 0xD83D HIGH surrogate that no JSON encoder, terminal renderer, or emoji parser can round-trip. Replacing with truncateUtf16Safe keeps the surrogate pair atomic: the trailing 🎉 is dropped at the cap boundary instead of being half-emptied into the user-visible error string.
  • Cap-precise boundary tests (steipete pattern, also in this commit): 
    ✓ outbound-deliver UTF-16 truncation cap boundary > cap 50 keeps a 49-char ASCII prefix
✓ outbound-deliver UTF-16 truncation cap boundary > cap 80 keeps a 79-char ASCII prefix
✓ outbound-media-send UTF-16 truncation cap boundary > cap 80 keeps a 79-char ASCII prefix

Tests and validation

  • node scripts/run-vitest.mjs extensions/qqbot/src/engine/messaging/outbound-deliver.test.ts extensions/qqbot/src/engine/messaging/outbound-media-send.test.ts --run
  • Added focused regression coverage for a boundary emoji / surrogate-pair truncation case in the production-helper surface (6 new it() blocks across the two existing test files).
  • node scripts/run-oxlint-shards.mjs --threads=4 clean.
  • pnpm tsgo:extensions clean.
  • No known failures from this change.

Risk checklist

  • Did user-visible behavior change? Yes — malformed truncated text (lone surrogate in error returns and debugLog previews) is now avoided while preserving existing cap values.
  • Did config, environment, or migration behavior change? No.
  • Did security, auth, secrets, network, or tool execution behavior change? No.
  • What is the highest-risk area? outbound-deliver.ts error-return and debugLog paths in sendText / parseAndSendMediaTags / sendPlainReply; outbound-media-send.ts error-return paths in sendPhoto / sendVoice / sendVideoMsg / sendDocument.
  • How is that risk mitigated? The change is a 1-to-1 substitution of .slice(0, N) for truncateUtf16Safe(text, N); the helper preserves identical behavior for ASCII, CJK (BMP), and all inputs without surrogate pairs, and only ever trims at most 1 code unit when a surrogate pair straddles the cap. Inline tests assert both the cap and the absence of lone surrogates. The companion engine/utils/log.ts sanitizeDebugLogValue site, the engine/tools/remind-logic.ts site, and the outbound.ts barrel re-exports are out of scope and will follow in PR 1b / PR 2 / PR 3.

Current review state

  • Next action: maintainer review and CI.
  • Nothing is waiting on the author.

@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed July 7, 2026, 4:48 AM ET / 08:48 UTC.

Summary
The branch replaces QQBot outbound deliver/media-send preview and error truncation with truncateUtf16Safe and adds focused UTF-16 boundary tests plus terminal proof output.

PR surface: Source +2, Tests +91. Total +93 across 4 files.

Reproducibility: yes. Current main uses raw .slice(0, N) on QQBot outbound preview/error strings, and a cap-boundary astral character can leave a lone surrogate; the PR body’s before/after terminal proof demonstrates that path.

Review metrics: none identified.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🐚 platinum hermit
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

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

Rank-up moves:

  • [P2] Fix the no-useless-concat lint failure in extensions/qqbot/src/engine/messaging/outbound-media-send.test.ts.
  • [P2] If the proof workflow remains red, update the PR body to include its required problem and evidence headings without removing the terminal proof.

Risk before merge

  • [P1] The current head cannot merge as-is because lint:extensions:bundled fails on a diff-caused no-useless-concat error in the added proof test.
  • [P1] The Real behavior proof workflow also reports missing required PR-body section headings, even though the body contains usable terminal proof for this scoped text-boundary change.

Maintainer options:

  1. Decide the mitigation before merge
    Fix the test string construction lint error, keep the narrow helper substitution, and let the related QQBot UTF-16 cleanup PRs cover their separate surfaces.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] A narrow automated repair can fix the lint-blocking test string without changing product behavior; broader QQBot cleanup and PR-body proof-heading housekeeping should stay out of the code repair.

Security
Cleared: The diff only changes QQBot string truncation helper usage and focused tests; it does not add dependency, secret, auth, permission, download, or code-execution surface.

Review findings

  • [P2] Avoid literal concatenation in the proof test — extensions/qqbot/src/engine/messaging/outbound-media-send.test.ts:696
Review details

Best possible solution:

Fix the test string construction lint error, keep the narrow helper substitution, and let the related QQBot UTF-16 cleanup PRs cover their separate surfaces.

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

Yes. Current main uses raw .slice(0, N) on QQBot outbound preview/error strings, and a cap-boundary astral character can leave a lone surrogate; the PR body’s before/after terminal proof demonstrates that path.

Is this the best way to solve the issue?

Yes. Reusing the existing plugin SDK UTF-16-safe helper is the narrow maintainable fix for these preview/error caps; the branch needs the added test lint error fixed before it is merge-ready.

Full review comments:

  • [P2] Avoid literal concatenation in the proof test — extensions/qqbot/src/engine/messaging/outbound-media-send.test.ts:696
    CI's check-additional-extension-bundled job fails on this new line with eslint(no-useless-concat). Since this is part of the added test, the branch cannot merge until the string is built without adjacent literal concatenation, for example pathPrefix + "🎉tail" or a template literal.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 24330a82b44a.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body now includes copied terminal output from a local Node run showing the exact affected error-template before/after and the absence of lone surrogates after the fix.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body now includes copied terminal output from a local Node run showing the exact affected error-template before/after and the absence of lone surrogates after the fix.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: ⏳ waiting on author.

Label justifications:

  • P2: This is a limited-blast-radius QQBot bug fix for malformed truncated preview/error strings, with one concrete lint blocker before merge.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body now includes copied terminal output from a local Node run showing the exact affected error-template before/after and the absence of lone surrogates after the fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body now includes copied terminal output from a local Node run showing the exact affected error-template before/after and the absence of lone surrogates after the fix.
Evidence reviewed

PR surface:

Source +2, Tests +91. Total +93 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 2 17 15 +2
Tests 2 91 0 +91
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 108 15 +93

Acceptance criteria:

  • [P1] pnpm lint:extensions:bundled.
  • [P1] node scripts/run-vitest.mjs extensions/qqbot/src/engine/messaging/outbound-deliver.test.ts extensions/qqbot/src/engine/messaging/outbound-media-send.test.ts --run.
  • [P1] pnpm tsgo:extensions.

What I checked:

Likely related people:

  • zhangguiping-xydt: Recently merged QQBot scoped sandbox media work touched the same outbound deliver/media-send paths and adjacent tests. (role: recent area contributor; confidence: high; commits: cd6b67533d6d; files: extensions/qqbot/src/engine/messaging/outbound-deliver.ts, extensions/qqbot/src/engine/messaging/outbound-media-send.ts)
  • cxyhhhhh: The QQBot group chat, C2C streaming, media upload, and engine refactor introduced much of the current messaging/media-send surface. (role: feature introducer; confidence: high; commits: 5ccf179a34a9; files: extensions/qqbot/src/engine/messaging/outbound-deliver.ts, extensions/qqbot/src/engine/messaging/outbound-media-send.ts)
  • steipete: Recent normalization-core work changed sliceUtf16Safe behavior and package boundary wiring used by this PR. (role: adjacent helper owner; confidence: medium; commits: b22c36f1125c, 062f88e3e3af; files: packages/normalization-core/src/utf16-slice.ts, src/plugin-sdk/text-utility-runtime.ts)
  • llagy009: Merged the closely related QQBot reminder job-name fix that established the same truncateUtf16Safe pattern in QQBot code. (role: adjacent QQBot truncation contributor; confidence: medium; commits: d693ed4af3e7; files: extensions/qqbot/src/engine/tools/remind-logic.ts, extensions/qqbot/src/engine/tools/remind-logic.test.ts)
  • wangmiao0668000666: Also authored the merged sibling PR for QQBot messaging reply and approval UTF-16-safe truncation, so they have current-main history in the same cleanup pattern beyond this branch. (role: recent adjacent contributor; confidence: medium; commits: 048fe0850edd; files: extensions/qqbot/src/engine/messaging/reply-dispatcher.ts, extensions/qqbot/src/engine/approval/index.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.
Review history (1 earlier review cycle)
  • reviewed 2026-07-07T06:36:31.735Z sha 8023dc7 :: needs real behavior proof before merge. :: none

@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jul 7, 2026
Merged
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 7, 2026
This was referenced Jul 7, 2026
Closed
Closed
@wangmiao0668000666

wangmiao0668000666 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

PR 1a now has focused runtime proof + cap-precise boundary tests. Two commits since last review:

  • d68a8ca361 test(qqbot): add runtime UTF-16 proof for outbound Failed-to-download-image template — exercises the EXACT production template literal at extensions/qqbot/src/engine/messaging/outbound-media-send.ts:460 with an emoji-boundary mediaPath, and asserts that the AFTER output (cap 80 via truncateUtf16Safe) contains no lone surrogate while the BEFORE .slice(0, 80) would emit a lone 0xD83C HIGH surrogate. Vitest stdout was captured and pasted into the PR body Real behavior proof section.

  • 865b152050 test(qqbot): focus UTF-16 cap boundary in messaging outbound — steipete's cap-precise pattern (safePrefix + "🎉tail", exact output equal to safePrefix), covers cap 50 (sent-chunk) and cap 80 (media-URL preview).

Total diff for this PR remains small (1 prod import swap already shipped earlier; runtime proof is +54 LoC in the existing test file, no new test files, no committed repro script).

Pre-flight checks (all green locally on Node 22.22.0):

  • node scripts/run-vitest.mjs extensions/qqbot/src/engine/messaging/outbound-deliver.test.ts extensions/qqbot/src/engine/messaging/outbound-media-send.test.ts --run — 30 tests passed.
  • pnpm tsgo:extensions — exit 0.
  • pnpm check:test-types — exit 0.

Ready for re-review. Thanks.

@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 7, 2026
@vincentkoc vincentkoc mentioned this pull request Jul 7, 2026
Merged
@vincentkoc

vincentkoc commented Jul 7, 2026

Copy link
Copy Markdown
Member

Superseded by the canonical QQBot bounded-preview cleanup in PR #101516, landed as ccbd9eb. That replacement includes this messaging scope, the gateway/API sibling sites, focused boundary tests, and preserves @wangmiao0668000666 as co-author. Thanks for identifying and proving the UTF-16 boundary issue.

@vincentkoc vincentkoc closed this Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: qqbot P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: S status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wangmiao0668000666 @vincentkoc