Skip to content

fix(infra): keep heartbeat/push message preview truncation UTF-16 safe#101606

Closed
lsr911 wants to merge 3 commits into
openclaw:mainfrom
lsr911:fix/infra-utf16-safe-truncation
Closed

fix(infra): keep heartbeat/push message preview truncation UTF-16 safe#101606
lsr911 wants to merge 3 commits into
openclaw:mainfrom
lsr911:fix/infra-utf16-safe-truncation

Conversation

@lsr911

@lsr911 lsr911 commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Heartbeat event emission and APNs push error parsing both truncate user-generated text with .slice(0, 200). When the 200-code-unit boundary lands mid-surrogate-pair (common with emoji and CJK supplementary characters in user messages or notification bodies), the dangling high surrogate renders as U+FFFD in operator-visible heartbeat events and APNs error diagnostics.

Why This Change Was Made

truncateUtf16Safe from @openclaw/normalization-core/utf16-slice is the canonical leaf helper already shipped in the plugin SDK and used across the codebase. Using the leaf import keeps the dependency graph minimal.

Evidence

Vitest — APNs parseReason surrogate-pair regression

Calls sendApnsAlert (the exact production path through parseReason) with an error body containing emoji straddling the 200-code-unit boundary. Asserts no U+FFFD in the returned reason.

$ node scripts/run-vitest.mjs src/infra/push-apns.test.ts

 Test Files  1 passed (1)
      Tests  15 passed (15)

Surrogate boundary proof (branch head f70268b)

Heartbeat preview: 6 emitHeartbeatEvent call sites (heartbeatToolResponse.summary, normalized.text, 4× previewText) — all at 200 code units, all switched to truncateUtf16Safe.

APNs error body: parseReason fallback paths use truncateUtf16Safe(trimmed, 200) instead of trimmed.slice(0, 200).

=== Heartbeat preview @200 ===
Raw .slice(0,200) ends: "xxxx\ud83d" (dangling high surrogate → U+FFFD)
truncateUtf16Safe(..., 200): len=199 | no U+FFFD

=== APNs error body @200 ===
Raw .slice(0,200) ends: "\ud83d" (dangling)
truncateUtf16Safe(..., 200): no U+FFFD

Files Changed

File Change
src/infra/heartbeat-runner.ts 6 preview truncation sites → truncateUtf16Safe
src/infra/push-apns.ts 2 error-body truncation sites → truncateUtf16Safe
src/infra/push-apns.test.ts +1 surrogate-pair regression test (at correct describe scope)

@lsr911
fix(infra): keep heartbeat/push message preview truncation UTF-16 safe
d2e3d71 
Replace raw .slice(0, 200) with truncateUtf16Safe from
@openclaw/normalization-core/utf16-slice in heartbeat-runner
(6 preview truncation sites) and push-apns parseReason
(2 error-body truncation sites) to prevent dangling surrogate
pairs when the 200-code-unit boundary splits an emoji or CJK
supplementary character.

Adds a focused regression test in push-apns.test.ts covering
the non-JSON error-body path with an emoji straddling the
boundary.
@openclaw-barnacle openclaw-barnacle Bot added size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 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 as superseded: the broader open UTF-16 truncation PR carries the same heartbeat/APNs changes, adds sibling coverage, credits this contributor, and is the safer canonical landing path than keeping this overlapping branch open.

Root-cause cluster
Relationship: superseded
Canonical: #101654
Summary: The broader open UTF-16 truncation PR explicitly consolidates this PR's heartbeat/APNs changes and the surrounding sibling fixes.

Members:

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

Canonical path: Use #101654 as the canonical UTF-16 truncation fix and close this overlapping branch.

So I’m closing this here and keeping the remaining discussion on #101654.

Review details

Best possible solution:

Use #101654 as the canonical UTF-16 truncation fix and close this overlapping branch.

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

Yes, source-reproducible with high confidence: current main still slices the affected APNs reason and heartbeat preview strings at raw UTF-16 code-unit 200, so a 199-character prefix followed by an emoji crosses the boundary. I did not run tests because this review is read-only.

Is this the best way to solve the issue?

No for this PR as the landing target. The helper-based fix is narrow and correct, but the broader canonical PR is the better solution because it covers sibling truncation surfaces, carries focused caller tests, and preserves this contributor's credit.

Security review:

Security review cleared: The diff only uses an existing internal normalization helper and adds tests; it does not change dependencies, workflows, secrets handling, package metadata, or external code execution.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root AGENTS.md was read fully; no scoped AGENTS.md owns these src/infra files, so the root ClawSweeper PR review, source-proof, and supersession rules apply. (AGENTS.md:18, 60f0749b7fb3)
  • Current main still has the raw slicing: Current main truncates APNs non-JSON/error-body fallback reasons with trimmed.slice(0, 200) and heartbeat event previews with raw .slice(0, 200), which can split a surrogate pair. (src/infra/push-apns.ts:214, 60f0749b7fb3)
  • Safe helper contract exists: truncateUtf16Safe floors the max length and delegates to sliceUtf16Safe, which adjusts both ends away from dangling UTF-16 surrogate halves; the package exports the ./utf16-slice leaf import used by the patch. (packages/normalization-core/src/utf16-slice.ts:44, 60f0749b7fb3)
  • This PR now has the intended narrow patch: The current head replaces the six heartbeat preview slices and two APNs parseReason fallback slices with truncateUtf16Safe, and the regression test is now a sibling under the APNs describe block rather than nested inside another test. (src/infra/heartbeat-runner.ts, f70268b9b0b6)
  • Canonical replacement includes this infra work: The broader PR at fix: keep bounded text truncation UTF-16 safe #101654 modifies the same heartbeat runner and APNs parseReason paths, adds caller-level heartbeat/APNs regression coverage, and its body says it consolidates this overlapping UTF-16 truncation work while preserving contributor credit. (src/infra/heartbeat-runner.ts, 15a1df3f7e35)
  • Canonical replacement is viable and proof-positive: The canonical PR is open, mergeable, based on current main, and its Real behavior proof, lint, production-types, dependency, guard, and test-types checks are green; remaining state was ordinary pending CI at review time. (15a1df3f7e35)

Likely related people:

  • steipete: Git blame ties the current raw truncation and helper lines to 395fbb8, and the broader canonical UTF-16 sweep PR is authored from this account. (role: current implementation introducer and canonical follow-up owner; confidence: high; commits: 395fbb8eb631, 080eb95c554c, 15a1df3f7e35; files: src/infra/heartbeat-runner.ts, src/infra/push-apns.ts, packages/normalization-core/src/utf16-slice.ts)
  • lsr911: Beyond this proposal, merged recent history shows this account already carried adjacent truncateUtf16Safe fixes in channels, ACP, and Codex truncation paths, and the canonical PR credits them as a co-author. (role: recent adjacent UTF-16 truncation contributor; confidence: medium; commits: 78f9449d73af, 42fc9e8a92f9, 37d613a59654; files: src/channels/thread-bindings-messages.ts, src/agents/acp-event-mapper.ts, extensions/codex/src/index.ts)
  • maweibin: Recent merged history shows related safe-truncation fixes in task, control UI, and session-cost paths, and the canonical consolidation PR includes this account as a co-author. (role: adjacent UTF-16 truncation contributor; confidence: medium; commits: 4d5cd05a64bf, f6ffe89df654, afdb9fd26408; files: src/tasks/task-completion-contract.ts, src/infra/control-ui-assets.ts, src/infra/session-cost-usage.ts)

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

@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
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 7, 2026
@steipete steipete mentioned this pull request Jul 7, 2026
Merged
@lsr911
fix(infra): move surrogate-pair test to describe scope level
f70268b 
The regression test in push-apns.test.ts was inadvertently nested
inside the previous it() callback, causing CI failures with
'Calling the test function inside another test function is not
allowed'. Move it to the correct scope inside the describe block.
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Thanks @lsr911. The useful behavior from this PR was consolidated with the sibling UTF-16 boundary fixes into #101654 and landed on main in a9582a1bb62aa70bb0b0ceb72536d0d76f08eab8.

The landed fix uses the existing truncateUtf16Safe helper, preserves this caller's prior code-unit limit and output shape, and adds caller-level surrogate-boundary coverage. Closing this PR as superseded by the canonical landed change.

@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

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lsr911 @steipete