Skip to content

fix(gateway): keep plugin approval title and description truncation UTF-16 safe#101478

Closed
wm0018 wants to merge 6 commits into
openclaw:mainfrom
wm0018:fix/plugin-policy-utf16-safe-truncation
Closed

fix(gateway): keep plugin approval title and description truncation UTF-16 safe#101478
wm0018 wants to merge 6 commits into
openclaw:mainfrom
wm0018:fix/plugin-policy-utf16-safe-truncation

Conversation

@wm0018

@wm0018 wm0018 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where plugin approval requests rendered broken U+FFFD replacement characters () in the gateway approval UI when the approval title or description contained an emoji or CJK supplementary character at the truncation boundaries (80 characters for title, 256 for description).

String.prototype.slice(0, n) cuts at UTF-16 code unit boundaries. Emoji like 🚀 are surrogate pairs (2 code units). When a pair straddles the cut position, the lone high surrogate renders as in downstream approval UI clients.

Why This Change Was Made

Replace input.title.slice(0, 80) and input.description.slice(0, 256) with truncateUtf16Safe — the standard UTF-16-safe truncation helper already used across the codebase. truncateUtf16Safe detects incomplete surrogate pairs and backs out to the last whole character instead of splitting them.

User Impact

Plugin approval requests with emoji or CJK supplementary characters near the truncation limits now deliver cleanly truncated text to the gateway approval UI, without broken replacement characters.

Evidence

Standalone proof (production helper, same head)

$ node --import tsx --input-type=module
node=v22.22.0
head=c2df57826f

// Before fix (raw slice) — approval title
title.length=81
slicedTitle.lastCodeUnit=0xd83d
slicedTitle.hasLoneSurrogate=true

// Before fix (raw slice) — approval description
desc.length=257
slicedDesc.lastCodeUnit=0xd83d
slicedDesc.hasLoneSurrogate=true

// After fix (truncateUtf16Safe) — approval title
safeTitle.length=79
safeTitle.matches=true
safeTitle.hasBrokenChar=false

// After fix (truncateUtf16Safe) — approval description
safeDesc.length=255
safeDesc.matches=true
safeDesc.hasBrokenChar=false

// No regression for plain title within limit
truncateUtf16Safe('Install plugin', 80)="Install plugin"
matches-slice=true

Tests

$ node scripts/run-vitest.mjs src/gateway/node-invoke-plugin-policy.test.ts

 Test Files  4 passed (4)
      Tests  28 passed (28)

Files Changed (2 files, +24/-2)

 src/gateway/node-invoke-plugin-policy.test.ts | 23 ++++++++++++++++++++++-
 src/gateway/node-invoke-plugin-policy.ts      |  3 ++-

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: XS labels Jul 7, 2026
@wm0018

wm0018 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 7, 2026

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 7, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 6:37 AM ET / 10:37 UTC.

Summary
The branch changes gateway plugin node-invoke approval truncation to use truncateUtf16Safe and adds approval-payload regression coverage, while also carrying many unrelated surfaces.

Reproducibility: yes. from source: current main slices plugin approval titles/descriptions at raw UTF-16 code-unit boundaries, and the shared helper tests show why surrogate-boundary truncation needs truncateUtf16Safe. I did not run the PR tests in this read-only review.

Review metrics: 3 noteworthy metrics.

  • Public contract surfaces: 2 changed. The branch changes src/plugin-sdk/core.ts and packages/gateway-protocol/src/schema/sessions.ts, so compatibility review is needed if the broad payload stays.
  • Workflow execution surface: 1 issue_comment workflow added. The new Mantis web UI workflow can be triggered from PR comments and writes issue/PR output, which needs automation and security review beyond the gateway bug fix.
  • Focused fix footprint: 2 gateway files intended, 301 live files changed. The useful UTF-16 fix is small, but the live branch scope is much broader than the PR body and changes the merge decision.

Stored data model
Persistent data-model change detected: persistent cache schema: apps/android/app/src/main/java/ai/openclaw/app/chat/ChatController.kt, persistent cache schema: apps/android/app/src/test/java/ai/openclaw/app/chat/RoomChatTranscriptCacheTest.kt, persistent cache schema: ui/src/i18n/.i18n/th.tm.jsonl, persistent cache schema: ui/src/i18n/.i18n/tr.tm.jsonl, persistent cache schema: ui/src/i18n/.i18n/uk.tm.jsonl, persistent cache schema: ui/src/i18n/.i18n/vi.tm.jsonl, and 8 more. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until stronger real behavior proof is added.

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

Rank-up moves:

  • Narrow or rebase the branch to the gateway policy file and focused regression test.
  • Update the PR body proof so it matches the current head after narrowing.
  • [P1] If the broad payload is intentional, add owner-reviewed proof for the workflow, protocol, SDK, mobile, channel, UI, and docs surfaces.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body includes terminal proof for the original gateway UTF-16 helper behavior, but it does not prove the current 301-file branch payload or the later broad workflow/protocol/UI changes; a narrowed current-head proof would be acceptable. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The PR is currently dirty/conflicting and cannot be merged as-is.
  • [P1] The branch adds or changes plugin SDK, gateway protocol, session routing, workflow, app, UI, and generated i18n surfaces that are unrelated to the stated gateway truncation fix.
  • [P1] The new issue-comment-triggered Mantis workflow and broad write permissions require automation/security-owner review if maintainers intend to keep that payload.
  • [P1] The provided terminal proof covers the original gateway UTF-16 behavior, not the current 301-file branch payload.

Maintainer options:

  1. Narrow and rebase before merge (recommended)
    Drop the unrelated branch payload, resolve the dirty merge state, and keep only the gateway UTF-16 fix plus its regression test.
  2. Accept the broad owner review cost
    Proceed only if maintainers intentionally adopt the workflow, SDK, protocol, app, channel, docs, and UI changes with matching owner approvals and proof.
  3. Pause or close this branch
    If the broad branch is accidental, close this PR and ask for a clean replacement that preserves the focused gateway patch.

Next step before merge

  • [P1] A maintainer needs to decide whether to narrow/rebase this conflicted branch or intentionally adopt the broad unrelated payload; this is not a safe automated repair lane.

Maintainer decision needed

  • Question: Should this PR be narrowed to the gateway UTF-16 approval fix, or is the broad 301-file branch intentionally being adopted for review?
  • Rationale: The focused bug fix is maintainable, but the current head includes unrelated workflow, protocol, plugin SDK, mobile, channel, UI, and docs changes plus a merge conflict; maintainer intent is needed because the PR is assigned and later commits were added by steipete.
  • Likely owner: steipete — steipete is the current assignee and authored the later branch commits that changed the PR from a focused gateway diff into a broad conflicted branch.
  • Options:
    • Narrow to gateway-only fix (recommended): Rebase or replace the branch so it contains only the two gateway files and current-head proof for the approval truncation behavior.
    • Adopt broad branch explicitly: Keep the broader branch only if maintainers want to review it as a multi-surface change with security, workflow, protocol, SDK, mobile, channel, and UI owners involved.
    • Close and ask for a fresh PR: Close this branch if preserving the focused fix is easier as a new clean PR than disentangling the current head.

Security
Needs attention: The focused gateway fix is not security-sensitive, but the current branch adds unrelated workflow/code-execution and write-permission surfaces that need security review or removal.

Review findings

  • [P1] Narrow the PR to the gateway approval fix — .github/workflows/mantis-web-ui-chat-proof.yml:1
Review details

Best possible solution:

Keep the UTF-16-safe gateway truncation change, but land it through a narrowed/rebased gateway-only PR or an explicit maintainer-owned adoption of the broader branch with full owner review and proof.

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

Yes, from source: current main slices plugin approval titles/descriptions at raw UTF-16 code-unit boundaries, and the shared helper tests show why surrogate-boundary truncation needs truncateUtf16Safe. I did not run the PR tests in this read-only review.

Is this the best way to solve the issue?

The two-line gateway change is the right narrow fix for the truncation bug, but the current PR branch is not the best merge shape because it includes hundreds of unrelated changes and is conflicted.

Full review comments:

  • [P1] Narrow the PR to the gateway approval fix — .github/workflows/mantis-web-ui-chat-proof.yml:1
    This gateway truncation PR currently adds a new Mantis workflow plus hundreds of unrelated app, plugin, protocol, docs, UI, and generated files. Late discovery: the broad payload was already visible at earlier reviewed head 3051f5d, but it remains a merge blocker because it makes the focused UTF-16 fix unreviewable and introduces unrelated behavior/security surfaces.
    Confidence: 0.93
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.92

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add merge-risk: 🚨 compatibility: The branch changes plugin SDK and gateway protocol schema surfaces plus many channel/session routing paths outside the stated fix.
  • add merge-risk: 🚨 security-boundary: The added issue-comment workflow has write permissions and validates candidate refs before running proof code, which needs explicit security-boundary review if kept.
  • add merge-risk: 🚨 automation: The branch adds a new Mantis workflow and scenario dispatch path, so merging could affect repository automation behavior.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body includes terminal proof for the original gateway UTF-16 helper behavior, but it does not prove the current 301-file branch payload or the later broad workflow/protocol/UI changes; a narrowed current-head proof would be acceptable. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • add mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The current branch touches Telegram delivery/session-route files with potentially visible chat behavior; this would need Telegram-visible proof if those unrelated changes stay.

Label justifications:

  • P2: The central bug is a normal-priority approval UI correctness fix with limited blast radius, while the broad branch risk is handled through merge-risk labels.
  • merge-risk: 🚨 compatibility: The branch changes plugin SDK and gateway protocol schema surfaces plus many channel/session routing paths outside the stated fix.
  • merge-risk: 🚨 automation: The branch adds a new Mantis workflow and scenario dispatch path, so merging could affect repository automation behavior.
  • merge-risk: 🚨 security-boundary: The added issue-comment workflow has write permissions and validates candidate refs before running proof code, which needs explicit security-boundary review if kept.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body includes terminal proof for the original gateway UTF-16 helper behavior, but it does not prove the current 301-file branch payload or the later broad workflow/protocol/UI changes; a narrowed current-head proof would be acceptable. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The current branch touches Telegram delivery/session-route files with potentially visible chat behavior; this would need Telegram-visible proof if those unrelated changes stay.
Evidence reviewed

Security concerns:

  • [medium] Unrelated issue-comment workflow broadens automation surface — .github/workflows/mantis-web-ui-chat-proof.yml:3
    The branch adds a new issue_comment/workflow_dispatch Mantis workflow with issue and pull-request write permissions; that surface is unrelated to approval text truncation and should be removed from this PR or reviewed as dedicated automation/security work.
    Confidence: 0.86

What I checked:

  • Current main behavior: Current main still truncates plugin node-invoke approval title and description with String.prototype.slice(0, 80/256), which can split surrogate pairs. (src/gateway/node-invoke-plugin-policy.ts:66, 942b44966107)
  • PR targeted fix: The PR patch replaces the two raw slices with truncateUtf16Safe and adds a regression that exercises the pending approval record path for title and description boundaries. (src/gateway/node-invoke-plugin-policy.ts:64, 7d49bc7323b2)
  • Dependency/helper contract: truncateUtf16Safe delegates to sliceUtf16Safe, whose implementation backs out of dangling surrogate halves and has direct tests for truncation at surrogate boundaries. (packages/normalization-core/src/utf16-slice.ts:44, 942b44966107)
  • Branch scope mismatch: Live PR metadata reports 301 changed files with +7965/-1544, and API file listing shows workflow, app, plugin, protocol, docs, UI, package, scripts, and gateway surfaces beyond the two gateway files described in the PR body. (7d49bc7323b2)
  • Mergeability and divergence: The PR is conflicting/dirty and compare against current main shows it is ahead by 6 commits, behind by 49 commits, and still exposes about 300 changed files from the PR branch. (7d49bc7323b2)
  • Unrelated broad commit provenance: Commit 3051f5d69d206438a1e259aa6b5de5674c4ba0cb titled test(gateway): exercise UTF-16 approval payloads actually changes 278 files with +7334/-1444, introducing most of the unrelated branch payload. (3051f5d69d20)

Likely related people:

  • LZY3538: Current-main blame attributes the node-invoke plugin policy file, including the raw title/description slice lines, to commit a6352f9356 associated with merged pull request fix(status): surface auto-fallback model in status and session_status (#96126) #101337. (role: introduced behavior; confidence: high; commits: a6352f9356cc; files: src/gateway/node-invoke-plugin-policy.ts)
  • steipete: Recent gateway approval helper refactors are by Peter Steinberger, and live PR data shows steipete is assigned and authored the later branch test/narrowing commits. (role: recent area contributor and PR assignee; confidence: high; commits: 3ff56020b16f, bb01e49192d3, 0d945dddba29; files: src/gateway/server-methods/approval-shared.ts, src/gateway/node-invoke-plugin-policy.test.ts, src/gateway/node-invoke-plugin-policy.ts)
  • Josh Avant: History for approval-related gateway files shows the async approval hook work in commit 6ade9c4, making this a secondary routing candidate for plugin approval behavior. (role: adjacent approval feature contributor; confidence: medium; commits: 6ade9c474cf1; files: src/infra/plugin-approvals.ts, src/gateway/server-methods/approval-shared.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 (4 earlier review cycles)
  • reviewed 2026-07-07T08:28:28.780Z sha 3d47367 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T08:45:50.440Z sha 3d47367 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T08:53:22.522Z sha c2df578 :: needs changes before merge. :: [P3] Exercise the approval payload path in the regression test
  • reviewed 2026-07-07T08:59:45.779Z sha c2df578 :: needs changes before merge. :: [P3] Exercise the approval payload path in the regression test

@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. P2 Normal backlog priority with limited blast radius. labels Jul 7, 2026
…TF-16 safe

Node.js String.prototype.slice cuts at UTF-16 code unit boundaries,
which can split surrogate pairs (emoji, CJK supplementary characters)
into lone surrogates that render as U+FFFD in approval UI payloads.

Use truncateUtf16Safe for both title and description fields in
createPluginApprovalsAttachment so approval requests never deliver
broken characters to the gateway approval UI.

Fixes #???
@wm0018
wm0018 force-pushed the fix/plugin-policy-utf16-safe-truncation branch from 3d47367 to c2df578 Compare July 7, 2026 08:47
@steipete steipete self-assigned this Jul 7, 2026
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation channel: discord Channel integration: discord channel: googlechat Channel integration: googlechat channel: imessage Channel integration: imessage channel: line Channel integration: line channel: matrix Channel integration: matrix channel: mattermost Channel integration: mattermost channel: msteams Channel integration: msteams channel: nextcloud-talk Channel integration: nextcloud-talk channel: nostr Channel integration: nostr channel: signal Channel integration: signal channel: slack Channel integration: slack channel: telegram Channel integration: telegram channel: tlon Channel integration: tlon channel: voice-call Channel integration: voice-call channel: whatsapp-web Channel integration: whatsapp-web labels Jul 7, 2026
@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts commands Command implementations agents Agent runtime and tooling channel: feishu Channel integration: feishu channel: twitch Channel integration: twitch channel: irc channel: qqbot channel: qa-channel Channel integration: qa-channel channel: synology-chat channel: sms Channel integration: sms size: XL and removed size: XS labels Jul 7, 2026
@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. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. and removed 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. labels Jul 7, 2026
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

The useful fix has landed in clean replacement #101580 as commit 2fe39692ad0d040987d14d92436725ba3f3d6c8b, with @wm0018's original implementation commit and authorship preserved.

This branch accumulated six repeated fixup commits, a stale merge base, 301 unrelated changed files, and merge conflicts, so landing it directly was no longer safe. The replacement contains only the two intended gateway files, passed 28 focused Blacksmith Testbox tests across four gateway projects, fresh clean autoreview, 69 exact-head hosted checks, and the native prepare gate.

Closing as superseded. Thank you for finding and fixing the plugin approval boundary.

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

Labels

agents Agent runtime and tooling app: android App: android app: ios App: ios app: macos App: macos app: web-ui App: web-ui channel: discord Channel integration: discord channel: feishu Channel integration: feishu channel: googlechat Channel integration: googlechat channel: imessage Channel integration: imessage channel: irc channel: line Channel integration: line channel: matrix Channel integration: matrix channel: mattermost Channel integration: mattermost channel: msteams Channel integration: msteams channel: nextcloud-talk Channel integration: nextcloud-talk channel: nostr Channel integration: nostr channel: qa-channel Channel integration: qa-channel channel: qqbot channel: signal Channel integration: signal channel: slack Channel integration: slack channel: sms Channel integration: sms channel: synology-chat channel: telegram Channel integration: telegram channel: tlon Channel integration: tlon channel: twitch Channel integration: twitch channel: voice-call Channel integration: voice-call channel: whatsapp-web Channel integration: whatsapp-web channel: zalo Channel integration: zalo channel: zalouser Channel integration: zalouser cli CLI command changes commands Command implementations docs Improvements or additions to documentation extensions: codex gateway Gateway runtime mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. scripts Repository scripts size: XL status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants