Skip to content

fix(shared): use truncateUtf16Safe in assistant-identity-values#103032

Closed
Simon-XYDT wants to merge 2 commits into
openclaw:mainfrom
Simon-XYDT:fix/assistant-identity-utf16-slice-final
Closed

fix(shared): use truncateUtf16Safe in assistant-identity-values#103032
Simon-XYDT wants to merge 2 commits into
openclaw:mainfrom
Simon-XYDT:fix/assistant-identity-utf16-slice-final

Conversation

@Simon-XYDT

@Simon-XYDT Simon-XYDT commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

The coerceIdentityValue function in src/shared/assistant-identity-values.ts uses raw .slice(0, maxLength) to truncate assistant identity values (name, avatar, emoji). When these values contain emoji at the truncation boundary, this can produce lone surrogates (invalid UTF-16 text), similar to the issue fixed in PR #102477.

Why This Change Was Made

Replace the unsafe .slice() with the canonical truncateUtf16Safe helper to prevent emoji surrogate pair truncation issues. The coerceIdentityValue function is used throughout the gateway to normalize assistant identity metadata, and while typical names don't contain emoji, avatars and emoji fields may include them.

User Impact

None for typical ASCII assistant names. Assistant identity values containing emoji at the truncation boundary will now be safely truncated without producing invalid UTF-16 text.

Real Behavior Proof

  • Behavior addressed: Prevent lone surrogates when truncating assistant identity values containing emoji
  • Real environment tested: Linux, Node 22.19, commit d6812b4
  • Test added: New test case verifies emoji surrogate pairs are handled safely at truncation boundaries
  • Evidence after fix:
pnpm test src/shared/assistant-identity-values.test.ts
# All 5 tests pass, including the new emoji surrogate pair test
  • Observed result after fix: The coerceIdentityValue function now uses truncateUtf16Safe, which safely handles emoji boundaries by dropping incomplete surrogate pairs rather than producing lone surrogates.
  • What was not tested: E2E gateway startup with emoji-rich assistant identity values (requires full gateway initialization)

Related

Simon-XYDT added 2 commits July 9, 2026 23:49
@Simon-XYDT
fix(tlon): add timeout to REST API requests
8870138 
Prevent Tlon REST client requests from hanging when the server accepts
a request but never responds. Add a default 30s timeout to all
fetchWithSsrFGuard calls in tlon-api.ts, following the same pattern as
PR openclaw#102027 (Mattermost timeout fix).

Changes:
- Add TLON_API_REQUEST_TIMEOUT_MS constant (30 seconds)
- Pass timeoutMs to getMemexUploadUrl fetchWithSsrFGuard call
- Pass timeoutMs to Memex upload fetchWithSsrFGuard call
- Pass timeoutMs to custom S3 upload fetchWithSsrFGuard call

This ensures bounded failures instead of stuck upload requests when
self-hosted or remote servers stall after accepting connections.
@Simon-XYDT
fix(shared): use truncateUtf16Safe in assistant-identity-values
9e79720 
Replace raw .slice(0, maxLength) with truncateUtf16Safe to prevent
emoji surrogate pair truncation issues in assistant identity values.

The coerceIdentityValue function is used to truncate assistant name,
avatar, and emoji values. When these values contain emoji at the
truncation boundary, the old implementation could produce lone
surrogates (invalid UTF-16 text).

This fix follows the same pattern as PR openclaw#102477 which addressed
similar issues in talk/fast-context-runtime.ts and
infra/heartbeat-events-filter.ts.
@openclaw-barnacle openclaw-barnacle Bot added channel: tlon Channel integration: tlon size: XS labels Jul 9, 2026
@clawsweeper

clawsweeper Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 1:47 PM ET / 17:47 UTC.

Summary
The PR replaces assistant identity truncation with truncateUtf16Safe and adds a surrogate-boundary test, while also adding a 30s timeout to three Tlon upload fetches.

PR surface: Source +7, Tests +20. Total +27 across 3 files.

Reproducibility: yes. for source-level reproduction: current main uses raw slice(0, maxLength) in coerceIdentityValue, and the shared helper contract shows why a boundary emoji can leave a dangling surrogate. I did not run a failing current-main command in this read-only review.

Review metrics: 1 noteworthy metric.

  • Tlon Timeout Behavior: 1 constant added; 3 upload fetches changed. This changes existing plugin upload behavior outside the assistant-identity fix and needs explicit review before merge.

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

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

Rank-up moves:

  • [P1] Fix the two oxlint errors in the new assistant identity test.
  • Drop or split the Tlon timeout commit into a dedicated Tlon PR with focused coverage.
  • [P1] Add redacted real terminal output or logs exercising the after-fix assistant identity truncation path.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body reports a focused unit test run only; before merge, the contributor should add redacted terminal output, logs, or a short recording that exercises the after-fix assistant identity path and redact private details before posting. Updating the PR body should trigger a fresh ClawSweeper review automatically; if not, ask a maintainer to comment @clawsweeper re-review. 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

  • [P2] The branch changes Tlon uploads to a fixed 30s timeout, which may abort slow existing Memex or custom S3 uploads and is not covered by the PR body, tests, or proof.
  • [P1] The current head has a failing check-lint job from the newly added assistant identity test.
  • [P1] Contributor proof is unit-test-only; it does not show an after-fix real helper, gateway, or UI path producing safe assistant identity text.

Maintainer options:

  1. Split Out The Tlon Timeout (recommended)
    Drop extensions/tlon/src/tlon-api.ts from this branch, or move it to a dedicated Tlon timeout PR with upload-timeout coverage and proof.
  2. Accept The Tlon Behavior Change
    A maintainer can intentionally keep the fixed 30s Tlon upload timeout here, but should own the slow-upload compatibility risk before merge.

Next step before merge

  • [P1] Not a ClawSweeper repair-lane candidate right now because the external PR needs contributor proof plus branch scoping cleanup before automation should try to carry it.

Security
Cleared: No concrete security or supply-chain regression was found; the diff imports an existing helper and changes Tlon guarded-fetch timeout options without new dependencies, workflows, secrets, or permissions.

Review findings

  • [P2] Fix the assistant identity test lint failures — src/shared/assistant-identity-values.test.ts:30-42
  • [P2] Split the unrelated Tlon timeout change out — extensions/tlon/src/tlon-api.ts:52
Review details

Best possible solution:

Narrow this PR to the assistant identity UTF-16-safe truncation fix, repair the test lint failures, and handle Tlon upload timeouts in a separate Tlon PR with focused timeout tests and real behavior proof.

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

Yes for source-level reproduction: current main uses raw slice(0, maxLength) in coerceIdentityValue, and the shared helper contract shows why a boundary emoji can leave a dangling surrogate. I did not run a failing current-main command in this read-only review.

Is this the best way to solve the issue?

No as submitted: using truncateUtf16Safe is the right code shape for assistant identity values, but the branch is not the best merge shape until the unrelated Tlon timeout change is split out and the lint/proof gaps are fixed.

Full review comments:

  • [P2] Fix the assistant identity test lint failures — src/shared/assistant-identity-values.test.ts:30-42
    The current check-lint job is red on this new test: oxlint reports no-useless-concat for the literal concatenation here and typescript(no-misused-spread) for the later string spread. Please rewrite the fixture and surrogate check so the new regression test passes the repository lint gate.
    Confidence: 0.95
  • [P2] Split the unrelated Tlon timeout change out — extensions/tlon/src/tlon-api.ts:52
    This PR is scoped and proven as an assistant identity UTF-16 truncation fix, but this added Tlon timeout changes existing upload behavior by aborting Memex/custom S3 uploads after 30 seconds. Drop it from this branch, or move it to a dedicated Tlon PR with focused timeout tests and real behavior proof.
    Confidence: 0.9

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 745b8c315eaa.

Label changes

Label changes:

  • add P2: This is a narrow correctness fix with limited blast radius, but the current PR has merge-blocking lint and scoping issues.
  • add merge-risk: 🚨 compatibility: The unrelated Tlon change adds a fixed 30s timeout to existing upload paths, which can change behavior for slow uploads or self-hosted storage.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports a focused unit test run only; before merge, the contributor should add redacted terminal output, logs, or a short recording that exercises the after-fix assistant identity path and redact private details before posting. Updating the PR body should trigger a fresh ClawSweeper review automatically; if not, ask a maintainer to comment @clawsweeper re-review. 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.

Label justifications:

  • P2: This is a narrow correctness fix with limited blast radius, but the current PR has merge-blocking lint and scoping issues.
  • merge-risk: 🚨 compatibility: The unrelated Tlon change adds a fixed 30s timeout to existing upload paths, which can change behavior for slow uploads or self-hosted storage.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports a focused unit test run only; before merge, the contributor should add redacted terminal output, logs, or a short recording that exercises the after-fix assistant identity path and redact private details before posting. Updating the PR body should trigger a fresh ClawSweeper review automatically; if not, ask a maintainer to comment @clawsweeper re-review. 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.
Evidence reviewed

PR surface:

Source +7, Tests +20. Total +27 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 9 2 +7
Tests 1 21 1 +20
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 30 3 +27

What I checked:

Likely related people:

  • steipete: Available blame for the current assistant identity helper and Tlon API file points to f8dbd34ab8, and live PR metadata maps that merged commit to steipete. (role: recent area contributor and merger in available history; confidence: medium; commits: f8dbd34ab8e8; files: src/shared/assistant-identity-values.ts, src/gateway/assistant-identity.ts, extensions/tlon/src/tlon-api.ts)
  • xydt-tanshanshan: Authored the merged related PR that applied truncateUtf16Safe to another truncation path, which this PR explicitly follows as precedent. (role: adjacent UTF-16 truncation contributor; confidence: medium; commits: f0d4d4f2fccd; files: src/talk/fast-context-runtime.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 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. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 9, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 9, 2026
Merged
@steipete steipete self-assigned this Jul 9, 2026
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded by #103034, which landed the assistant-identity fix in bb16ca50d622b62baf56abd725c8d0e6b1e92619.

The landed version moves each UTF-16-safe limit to its gateway or Control UI owner, deletes the arbitrary-limit shared helper, preserves existing identity precedence/fallback behavior, and adds schema-reachable boundary regressions. Exact-head CI, CodeQL, sanitized AWS focused proof (42 assertions), and fresh autoreview passed before merge.

This branch also contains the unrelated Tlon upload-timeout commit, whose fixed 30-second behavior was neither part of the identity bug nor suitable to land without its own compatibility review and focused proof. If that Tlon change is still wanted, please submit it separately with its own problem statement and slow-upload/timeout coverage.

Thanks @Simon-XYDT for identifying the UTF-16 boundary bug and contributing the original fix; contributor credit is preserved in the landed commit history.

@steipete steipete closed this Jul 9, 2026
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

channel: tlon Channel integration: tlon merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS 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

@Simon-XYDT @steipete