Skip to content

fix(slack): keep inbound message preview truncation surrogate-safe#102514

Closed
zw-xysk wants to merge 2 commits into
openclaw:mainfrom
zw-xysk:fix/slack-prepare-safe
Closed

fix(slack): keep inbound message preview truncation surrogate-safe#102514
zw-xysk wants to merge 2 commits into
openclaw:mainfrom
zw-xysk:fix/slack-prepare-safe

Conversation

@zw-xysk

@zw-xysk zw-xysk commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

prepare.ts truncates Slack message preview text at 160 chars using .slice(0, 160) before adding it to the inbound message context. When the truncation boundary falls inside a UTF-16 surrogate pair (emoji like 🎉, CJK extended characters), the resulting string contains a dangling surrogate that can corrupt the LLM prompt context.

Changes

extensions/slack/src/monitor/message-handler/prepare.ts (+2, −1):

  • Import truncateUtf16Safe from openclaw/plugin-sdk/text-utility-runtime
  • rawBody.slice(0, 160) → truncateUtf16Safe(rawBody, 160)

Evidence

Unit tests (113/113 passed):

 Test Files  1 passed (1)
      Tests  113 passed (113)

Real behavior proof (platinum):

[+0.001s] ═══ Slack prepare.ts proof ═══
[+0.002s] OLD .slice(0,160): ✗ BROKEN
[+0.002s] NEW truncateUtf16Safe: ✓ OK
[+0.002s] RESULT: PASS (platinum)

Review

  • Manually reviewed and verified
  • AI-assisted: Yes

@zw-xysk
fix(slack): keep inbound message preview truncation surrogate-safe
ee74e4d 
Slack message preview text is truncated at 160 chars using .slice(0, 160)
before being added to the inbound message context. When the truncation
boundary falls inside a UTF-16 surrogate pair (emoji, CJK extended),
the resulting string contains a dangling surrogate that can corrupt the
prompt context.

Replace .slice(0, 160) with truncateUtf16Safe(text, 160) so the
truncation point always falls on a complete code-point boundary.
@openclaw-barnacle openclaw-barnacle Bot added channel: slack Channel integration: slack size: XS labels Jul 9, 2026
@zw-xysk

zw-xysk commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 9, 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 commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close because this PR repeats the same PreparedSlackMessage.preview-only Slack patch that a maintainer already closed in #101784 after deciding that field has no production consumer; the current branch adds a narrower test but does not change that product boundary.

Root-cause cluster
Relationship: duplicate
Canonical: #101784
Summary: This PR duplicates a same-surface Slack preview truncation PR that was closed unmerged after maintainer review rejected the preview-only fix path.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Leave the preview-only branch closed and target any remaining user-visible Slack UTF-16 issue at the maintainer-approved Slack label/message boundary instead.

So I’m closing this here because the remaining work is already tracked in the canonical issue.

Review details

Best possible solution:

Leave the preview-only branch closed and target any remaining user-visible Slack UTF-16 issue at the maintainer-approved Slack label/message boundary instead.

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

Yes for the internal string behavior: current main's .slice(0, 160) can split a surrogate pair at the preview boundary. The missing part is not reproduction but relevance, because prior maintainer review says this prepared preview field has no production consumer.

Is this the best way to solve the issue?

No. Although the helper is technically suitable, changing only PreparedSlackMessage.preview is not the best fix after the same patch was closed as an unused projection; visible Slack text should be fixed at the actual label/message output boundary.

Security review:

Security review cleared: The diff imports an existing SDK text helper and adds a focused test; it does not change dependencies, workflows, secrets, permissions, or package resolution.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root AGENTS.md and the scoped extensions policy were read; the extensions policy permits plugin production imports from openclaw/plugin-sdk/*, so the import path itself is not the blocker. (AGENTS.md:26, 769878303744)
  • PR diff surface: The diff is the same surface as the prior closed proposal: replace the Slack prepare preview .slice(0, 160) call with truncateUtf16Safe and add a boundary test. (extensions/slack/src/monitor/message-handler/prepare.ts:1213, 5817f8f30031)
  • Prior maintainer decision: The earlier same-surface PR was closed unmerged after a MEMBER comment said PreparedSlackMessage.preview has no production consumer and the reachable Slack UTF-16 case belongs in wider canonical cleanup. (a9b3c61d5686)
  • Current main still has the unused projection: Current main still builds preview with raw .slice(0, 160) and returns it on the prepared object, confirming this is not implemented-on-main but is the same unused projection discussed in the closed PR. (extensions/slack/src/monitor/message-handler/prepare.ts:1213, 769878303744)
  • Consumer check: A current-source search found no runtime prepared.preview consumer in Slack dispatch; dispatchPreparedSlackMessage destructures the prepared object without reading preview. (extensions/slack/src/monitor/message-handler/dispatch.ts:427, 769878303744)
  • Helper contract inspected: truncateUtf16Safe delegates to sliceUtf16Safe, which backs off before returning a dangling surrogate, so the proposed helper is technically suitable for real truncation sites. (packages/normalization-core/src/utf16-slice.ts:44, 769878303744)

Likely related people:

  • vincentkoc: Closed the earlier same-surface PR with the maintainer decision that the preview field is not a production consumer and the visible Slack UTF-16 case belongs elsewhere. (role: reviewer / decision owner; confidence: high; files: extensions/slack/src/monitor/message-handler/prepare.ts)
  • hugenshen: Authored the earlier same-surface Slack PR and local blame shows the current preview/helper lines carried by the recent merged UTF-16 truncation cleanup commit. (role: recent same-surface contributor; confidence: medium; commits: a3f00d32c914; files: extensions/slack/src/monitor/message-handler/prepare.ts, packages/normalization-core/src/utf16-slice.ts)
  • kevinlin-openai: Recently merged Slack Enterprise Grid work touched the central Slack monitor and prepare paths adjacent to this change. (role: recent Slack area contributor; confidence: medium; commits: 245b91b83de0, 15178ad18a30; files: extensions/slack/src/monitor/message-handler/prepare.ts, extensions/slack/src/monitor/message-handler.ts)
  • steipete: Merged the recent UTF-16 truncation cleanup that introduced the helper into current main and added the current PR's boundary test commit. (role: recent reviewer / merger; confidence: medium; commits: a3f00d32c914, 5817f8f30031; files: packages/normalization-core/src/utf16-slice.ts, extensions/slack/src/monitor/message-handler/prepare.test.ts)

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

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jul 9, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 9, 2026
@clawsweeper clawsweeper Bot closed this Jul 9, 2026
@clawsweeper

clawsweeper Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper autoclose is complete.

Reason: structured ClawSweeper close marker: close-required (sha=5817f8f30031635d2546be4227efb317204cfbea)

Closed:

@zw-xysk

zw-xysk commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@steipete hi, quick question about this PR.

#102514 fixes a truncation safety issue in the prepare.ts preview field — it was flagged as a duplicate of #101784 which was closed because the preview field has no production consumer.

I noticed that prepare-thread-context-root.ts:120 in the same directory already uses truncateUtf16Safe, and most other truncation points in prepare.ts are already safe — the rawBody.slice(0, 160) on line 1213 seems like the only one that was missed.

Could you confirm whether this field truly has no consumer? If it does have a potential use path, the fix is just 1 line and would complete the sweep of this module. Happy to close it if not.

Thanks!

@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jul 9, 2026
Closed
9 tasks
@clawsweeper clawsweeper Bot mentioned this pull request Jul 9, 2026
Merged
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: slack Channel integration: slack P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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

@zw-xysk @steipete