Skip to content

fix(exec): avoid splitting surrogate pairs in approval display#99566

Merged
steipete merged 2 commits into
openclaw:mainfrom
mikasa0818:fix/exec-approval-utf16-truncate
Jul 3, 2026
Merged

fix(exec): avoid splitting surrogate pairs in approval display#99566
steipete merged 2 commits into
openclaw:mainfrom
mikasa0818:fix/exec-approval-utf16-truncate

Conversation

@mikasa0818

@mikasa0818 mikasa0818 commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Exec approval command display text is part of the security decision UI. The sanitizer already handles invisible Unicode characters by full code point, but its final display cap used raw String.prototype.slice. If the 16 KiB cap lands between the high and low surrogate of a supplementary-plane character, the approval preview can contain malformed UTF-16 such as a dangling \uD83D before …[truncated].

That can make the command text rendered in approval prompts differ at the character level from the sanitized string the code intended to show, and can break downstream UI/payload encoding for the approval prompt.

Summary

  • Use the shared UTF-16-safe truncation helper for exec approval command display output caps.
  • Preserve the existing 16 KiB display cap, truncation marker, redaction-before-truncation ordering, and hard oversized-input suppression behavior.
  • Add a regression test for a command whose emoji surrogate pair crosses the display truncation boundary.

Evidence

  • Runtime proof using "a".repeat(16 * 1024 - 1) + "😀tail" returned { "truncated": true, "hasLoneSurrogate": false, "encodable": true, "tail": "aaaaaaaaaaaa…[truncated]" }.
  • node scripts/run-vitest.mjs run src/infra/exec-approval-command-display.test.ts exited 0.
  • node_modules/.bin/vitest run src/infra/exec-approval-command-display.test.ts reported 1 file passed / 33 tests passed.
  • pnpm exec oxfmt --check --threads=1 src/infra/exec-approval-command-display.ts src/infra/exec-approval-command-display.test.ts exited 0. Local pnpm emitted the existing Node engine warning because the machine is on Node v22.17.0 while package metadata asks for >=22.19.0.
  • pnpm exec oxlint src/infra/exec-approval-command-display.ts src/infra/exec-approval-command-display.test.ts exited 0.
  • git diff --check exited 0.

Risk

Low. This only changes the existing display truncation boundary to back off from split surrogate pairs. It does not change redaction patterns, the display size constant, the truncation marker, warning/command marker strings, or the hard input cap behavior.

🤖 Generated with Claude Code

@openclaw-barnacle openclaw-barnacle Bot added size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 3, 2026
@clawsweeper

clawsweeper Bot commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 7:28 PM ET / 23:28 UTC.

Summary
The PR updates exec approval command display sanitization to use UTF-16-safe truncation, escape unpaired surrogate code units, and add sanitizer plus Gateway broadcast regression coverage.

PR surface: Source +2, Tests +47. Total +49 across 3 files.

Reproducibility: yes. Source inspection shows current main still uses raw UTF-16 slice at the 16 KiB display cap, and the PR body plus tests exercise a command where an emoji crosses that boundary and previously could leave a lone surrogate.

Review metrics: none identified.

Root-cause cluster
Relationship: canonical
Canonical: #99566
Summary: This PR is the focused core exec approval sanitizer fix for split surrogate and malformed surrogate approval text; related items are provenance, channel-specific sibling fixes, or broader guardrail work rather than replacements.

Members:

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

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

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

Risk before merge

  • [P1] Hosted CI still had two ordinary pending jobs during this read-only review; merge should wait for required checks to finish even though focused local/Testbox proof is strong.

Maintainer options:

  1. Decide the mitigation before merge
    Merge the focused central sanitizer hardening after maintainer review and required checks complete; keep broad boundary-safety guard work and channel-specific preview fixes separate.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No ClawSweeper repair lane is needed because there are no actionable patch findings; the remaining action is maintainer review and normal required-check gating.

Security
Cleared: No concrete security or supply-chain concern was found; the diff hardens exec approval display encoding without changing dependencies, permissions, secrets handling, or command execution semantics.

Review details

Best possible solution:

Merge the focused central sanitizer hardening after maintainer review and required checks complete; keep broad boundary-safety guard work and channel-specific preview fixes separate.

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

Yes. Source inspection shows current main still uses raw UTF-16 slice at the 16 KiB display cap, and the PR body plus tests exercise a command where an emoji crosses that boundary and previously could leave a lone surrogate.

Is this the best way to solve the issue?

Yes. The central approval-display sanitizer is the narrowest maintainable owner because Gateway approval registration, lookup, and broadcast payloads flow through it; reusing the existing dependency-free UTF-16 helper avoids a duplicate local slicing implementation.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P2: This is a bounded exec approval display correctness and hardening fix with focused source/test changes and limited blast radius.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Sufficient live-output proof is present: the PR body includes after-fix runtime output for the surrogate-boundary case, and the maintainer comment adds exact-head focused tests plus Blacksmith Testbox Gateway/WebSocket approval E2E proof.
  • proof: sufficient: Contributor real behavior proof is sufficient. Sufficient live-output proof is present: the PR body includes after-fix runtime output for the surrogate-boundary case, and the maintainer comment adds exact-head focused tests plus Blacksmith Testbox Gateway/WebSocket approval E2E proof.
Evidence reviewed

PR surface:

Source +2, Tests +47. Total +49 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 1 8 6 +2
Tests 2 48 1 +47
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 56 7 +49

What I checked:

  • Current main still has the raw display cap slice: On current main, truncateForDisplay appends the truncation marker after text.slice(0, EXEC_APPROVAL_MAX_OUTPUT), so the central exec approval sanitizer can still return a dangling surrogate if the cap lands inside a pair. (src/infra/exec-approval-command-display.ts:61, c680312ef6f3)
  • PR head uses the shared UTF-16-safe truncation owner: At the reviewed head, truncateForDisplay uses truncateUtf16Safe(text, EXEC_APPROVAL_MAX_OUTPUT) before appending the existing marker, preserving the cap and marker while avoiding split surrogate pairs. (src/infra/exec-approval-command-display.ts:63, 1d96d5be37f7)
  • PR head escapes malformed surrogate code units: The reviewed head adds Unicode category Cs to the approval invisible-character regexes, so unpaired high or low surrogate code units render as visible \u{...} escapes while valid astral code points remain intact under the Unicode regex flag. (src/infra/exec-approval-command-display.ts:14, 1d96d5be37f7)
  • Shared helper backs off unsafe UTF-16 boundaries: truncateUtf16Safe delegates to sliceUtf16Safe, whose end-boundary logic decrements the end index when it would leave a high surrogate before a low surrogate. (src/shared/utf16-slice.ts:35, c680312ef6f3)
  • Gateway approval path uses this sanitizer before broadcast: exec.approval.request sanitizes effectiveCommandText with sanitizeExecApprovalDisplayTextWithStatus before creating the approval record and broadcasting exec.approval.requested, so the changed sanitizer is the correct central owner for Gateway approval payload text. (src/gateway/server-methods/exec-approval.ts:261, c680312ef6f3)
  • Regression coverage locks the surrogate boundary: The PR adds a sanitizer regression where an emoji begins at the 16 KiB cutoff and asserts the truncated output has no lone surrogate and remains URI-encodable. (src/infra/exec-approval-command-display.test.ts:150, 1d96d5be37f7)

Likely related people:

  • pgondhi987: Authored and merged the exec approval truncation-rejection PR that added SanitizedExecApprovalDisplayText while retaining the raw text.slice output cap now being hardened. (role: introduced current status-cap behavior; confidence: high; commits: 731af9c96bba, 37f3ed50c502; files: src/infra/exec-approval-command-display.ts, src/gateway/server-methods/exec-approval.ts)
  • drobison00: Authored the merged exec approval display sanitizer hardening that established the central sanitizer, invisible-character escaping, redaction-before-truncation behavior, and related tests. (role: introduced sanitizer owner path; confidence: high; commits: 503b748a8ee1, 142212668ce7; files: src/infra/exec-approval-command-display.ts, src/infra/exec-approval-command-display.test.ts)
  • Bartok9: Authored the PR that extracted the dependency-free UTF-16 slice/truncation helper now reused by this patch. (role: introduced shared helper; confidence: medium; commits: d862dfc1c168, e09b9dfc1ba9; files: src/shared/utf16-slice.ts, src/utils.ts)
  • vincentkoc: Merged the shared UTF-16 helper PR and recent channel approval-preview UTF-16 boundary fixes, making this a likely review route for the shared helper and sibling approval-display invariant. (role: recent adjacent owner; confidence: medium; commits: e09b9dfc1ba9, 2e881ab1c679, 90c20d15c22e; files: src/shared/utf16-slice.ts, extensions/googlechat/src/approval-handler.runtime.ts, extensions/slack/src/approval-handler.runtime.ts)
  • steipete: Assigned to the PR, pushed the final malformed-surrogate hardening commit, and posted exact-head maintainer proof covering the Gateway E2E path. (role: recent reviewer and follow-up owner; confidence: high; commits: 1d96d5be37f7; files: src/infra/exec-approval-command-display.ts, src/infra/exec-approval-command-display.test.ts, src/gateway/server-methods/server-methods.test.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. P2 Normal backlog priority with limited blast radius. labels Jul 3, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 3, 2026
@steipete steipete self-assigned this Jul 3, 2026
mikasa0818 and others added 2 commits July 4, 2026 00:20
@mikasa0818 @claude @steipete
fix(exec): avoid splitting surrogate pairs in approval display
87c0ed5 
Use the shared UTF-16 safe truncation helper for exec approval display caps so long sanitized command previews remain well-formed when the cutoff lands inside a surrogate pair.

Co-Authored-By: Claude <[email protected]>
@steipete
fix(exec): sanitize malformed approval text
1d96d5b
@steipete
steipete force-pushed the fix/exec-approval-utf16-truncate branch from 809ce40 to 1d96d5b Compare July 3, 2026 23:20
@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: S and removed size: XS labels Jul 3, 2026
@steipete

steipete commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Maintainer proof for exact head 1d96d5be37f73f80ffe27397e62f99c2e6bbfb20:

  • Best-fix review: the canonical approval-display sanitizer is the correct owner. It now uses the shared truncateUtf16Safe helper at the existing 16 KiB cap; redaction order, cap sizes, markers, command binding, and rejection behavior are unchanged.
  • Maintainer improvement: hostile caller-supplied lone high/low surrogates also broke URI encoding even without truncation. Category Cs is now rendered as a visible \u{...} marker, while valid emoji/astral code points remain unchanged under the Unicode regex contract.
  • Integration regression: the Gateway handler test proves malformed input is sanitized before the exec.approval.requested broadcast.
  • Focused local proof after final rebase: node scripts/run-vitest.mjs src/infra/exec-approval-command-display.test.ts src/gateway/server-methods/server-methods.test.ts — 35 + 162 passed.
  • Local formatting/lint: oxfmt --check and oxlint on the three changed files — clean.
  • Remote focused/live proof: Blacksmith Testbox tbx_01kwn3xx3pdvea419emehnqf55, Actions run 28687246490 — sanitizer 35/35, handler 162/162, and real Gateway/WebSocket operator approval E2E 1/1; wrapper exitCode: 0.
  • Remote changed gate: Blacksmith Testbox tbx_01kwn422b49a80hk33gmapqmcw, Actions run 28687311432 — core production/test types, changed lint, guards, database-first checks, import cycles, webhook and pairing guards passed; wrapper exitCode: 0.
  • Fresh autoreview: no accepted/actionable findings; correctness confidence 0.94.
  • Provenance: the raw display slice arrived in Reject truncated exec approval commands [AI] #81001, authored and merged by @pgondhi987 on 2026-05-12. This is ordinary follow-up hardening, not fault attribution.
  • Duplicate search: no exact open duplicate. fix(boundary-safety): guard response reads and UTF-16 truncation #97774 is a broad boundary-safety PR and does not touch this approval-display owner.

The first local source-checkout Gateway attempt was stopped and cleaned up because missing worktree package-local dependencies stalled sidecar startup; the installed Testbox lane completed the real Gateway E2E. Screenshots are not useful for this nonvisual encoding boundary.

The local prepared and pushed trees are identical (84fbc80a53e82fdc459b1bfc493dbed83918aa1c). Fresh hosted CI is running on this exact head; merge remains gated on green.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 3, 2026
@steipete
steipete merged commit 7430168 into openclaw:main Jul 3, 2026
121 of 125 checks passed
@steipete

steipete commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 4, 2026
@mikasa0818 @claude @steipete
fix(exec): avoid splitting surrogate pairs in approval display (openc…
cc9c48b 
…law#99566)

* fix(exec): avoid splitting surrogate pairs in approval display

Use the shared UTF-16 safe truncation helper for exec approval display caps so long sanitized command previews remain well-formed when the cutoff lands inside a surrogate pair.

Co-Authored-By: Claude <[email protected]>

* fix(exec): sanitize malformed approval text

---------

Co-authored-by: Claude <[email protected]>
Co-authored-by: Peter Steinberger <[email protected]>
This was referenced Jul 5, 2026
Merged
Merged
@clawsweeper clawsweeper Bot mentioned this pull request Jul 5, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

gateway Gateway runtime P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor 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

@mikasa0818 @steipete