Skip to content

fix(agents): use truncateUtf16Safe for raw args preview truncation#102609

Closed
lzyyzznl wants to merge 1 commit into
openclaw:mainfrom
lzyyzznl:fix/truncateUtf16Safe-tool-subscribe
Closed

fix(agents): use truncateUtf16Safe for raw args preview truncation#102609
lzyyzznl wants to merge 1 commit into
openclaw:mainfrom
lzyyzznl:fix/truncateUtf16Safe-tool-subscribe

Conversation

@lzyyzznl

@lzyyzznl lzyyzznl commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

src/agents/embedded-agent-subscribe.handlers.tools.ts uses raw .slice(0, N) for string truncation at raw args preview rawArgsPreview?.slice(0, TOOL_START_WARNING_RAW_PREVIEW_MAX_CHARS). When the text contains multi-byte Unicode characters such as emoji, a code-unit slice can split a UTF-16 surrogate pair, producing invalid Unicode.

Why This Change Was Made

Replacing these .slice(0, N) calls with truncateUtf16Safe ensures the truncated output is always valid Unicode while keeping identical behavior for ASCII-only content.

User Impact

Truncated text in the affected code path remains valid Unicode at the existing size limits. No behavioral, API, or performance changes for ASCII-only input.

Evidence

  • Mechanical one-to-one replacement: .slice(0, N) -> truncateUtf16Safe(str, N)
  • truncateUtf16Safe is already used extensively in the codebase following the same pattern
  • No runtime behavior change for the common case (valid Unicode or ASCII input)

Generated with Claude Code

@lzyyzznl
fix(agents): use truncateUtf16Safe for raw args preview truncation
4d36d70 
One .slice(0, N) truncation site was missed when truncateUtf16Safe
was introduced to this file. Replace the remaining .slice call to
keep the truncated output valid Unicode when the raw args preview
contains multi-byte characters such as emoji.
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. 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, 6:20 AM ET / 10:20 UTC.

Summary
The PR replaces the read-tool missing-path raw args preview .slice(0, N) with truncateUtf16Safe in src/agents/embedded-agent-subscribe.handlers.tools.ts.

PR surface: Source 0. Total 0 across 1 file.

Reproducibility: yes. from source inspection: a read tool_execution_start event with string args longer than the preview limit and an emoji crossing the max + 1 boundary reaches this warning path. I did not run a live harness in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until 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:

  • [P1] Preserve the overflow sentinel for surrogate-boundary truncation and add focused regression coverage for that case.
  • [P1] Add redacted terminal output, copied live output, or logs from the focused run; redact private paths, endpoints, keys, phone numbers, and other private details.
  • Update the PR body after adding proof so ClawSweeper can re-review automatically, or ask a maintainer to comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body has rationale but no redacted terminal output, log excerpt, recording, or linked artifact showing the after-fix read-tool warning with emoji-boundary args. 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 proposed max + 1 safe truncation can return exactly the visible limit when the extra code unit is a high surrogate, so the warning preview can omit the ellipsis even though raw args were truncated.
  • [P1] The external PR body still lacks real after-fix terminal output, logs, a recording, or another redacted artifact that drives the changed read-warning path.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the UTF-16-safe fix, but preserve the overflow sentinel at surrogate boundaries and add focused regression coverage plus redacted terminal/log proof before merge.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Contributor follow-up is needed for the boundary-case patch and real behavior proof; automation should not try to repair while the external proof gate is missing.

Security
Cleared: The diff only changes an in-process diagnostic preview truncation expression and does not add a security or supply-chain surface.

Review findings

  • [P3] Preserve the preview overflow sentinel — src/agents/embedded-agent-subscribe.handlers.tools.ts:985
Review details

Best possible solution:

Keep the UTF-16-safe fix, but preserve the overflow sentinel at surrogate boundaries and add focused regression coverage plus redacted terminal/log proof before merge.

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

Yes from source inspection: a read tool_execution_start event with string args longer than the preview limit and an emoji crossing the max + 1 boundary reaches this warning path. I did not run a live harness in this read-only review.

Is this the best way to solve the issue?

No; truncateUtf16Safe is the right primitive, but the PR must preserve a complete extra sanitized code point or explicit truncation marker so overflow remains visible at surrogate boundaries.

Full review comments:

  • [P3] Preserve the preview overflow sentinel — src/agents/embedded-agent-subscribe.handlers.tools.ts:985
    TOOL_START_WARNING_RAW_PREVIEW_MAX_CHARS is intentionally one past the visible limit so sanitizeForConsole can append the ellipsis. With this patch, if that extra code unit is the high half of a surrogate pair, truncateUtf16Safe backs off to exactly 200 visible characters and the sanitizer returns a full-looking preview without the ellipsis even though raw args were truncated. Please preserve an extra complete character before sanitizing or add an explicit truncation marker for this boundary case.
    Confidence: 0.91

Overall correctness: patch is correct
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P3: This is a narrow diagnostic-preview correctness fix with low user impact and no config, security, migration, or runtime availability surface.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • 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 has rationale but no redacted terminal output, log excerpt, recording, or linked artifact showing the after-fix read-tool warning with emoji-boundary args. 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 0. Total 0 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 1 1 0
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 1 1 0

What I checked:

Likely related people:

  • obviyus: git blame and GitHub commit metadata tie the current consolidated agent handler, sanitizer, and UTF-16 helper lines to commit 4cbe49b. (role: introduced current behavior; confidence: high; commits: 4cbe49ba881a; files: src/agents/embedded-agent-subscribe.handlers.tools.ts, src/agents/console-sanitize.ts, packages/normalization-core/src/utf16-slice.ts)
  • ferminquant: Merged PR metadata shows ferminquant authored the pathless read diagnostic correlation work that established this warning path before later consolidation. (role: introduced adjacent diagnostic behavior; confidence: medium; commits: a85a189f469e; files: src/agents/pi-embedded-subscribe.handlers.tools.ts, src/agents/pi-embedded-subscribe.handlers.tools.test.ts)
  • steipete: PR metadata for the pathless read diagnostic work shows steipete merged the diagnostic PR and authored follow-up commits in the same read-tool warning area; history also ties steipete to earlier invalid UTF-16 truncation work. (role: adjacent contributor and merger; confidence: medium; commits: a08ee048b69a, 4b42042a94de, 63f5fa47deb6; files: src/agents/pi-embedded-subscribe.handlers.tools.ts, src/agents/pi-embedded-subscribe.handlers.tools.test.ts, packages/normalization-core/src/utf16-slice.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 (1 earlier review cycle)
  • reviewed 2026-07-09T09:39:17.762Z sha 4d36d70 :: needs real behavior proof before merge. :: [P3] Preserve the preview overflow sentinel

@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jul 9, 2026
Closed
9 tasks
@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
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 9, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 9, 2026
Merged
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Closing this because the full output path shows no malformed UTF-16 reaches the log, and the proposed change weakens the existing truncation signal.

rawArgsPreview.slice(0, 201) is only an input bound for sanitizeForConsole(..., 200). That sanitizer converts to code points, and when the raw slice ends with a lone high surrogate at position 200, its final codePoints.slice(0, 200) excludes that surrogate and appends …. The emitted preview is therefore already valid UTF-16.

Replacing the input bound with truncateUtf16Safe(..., 201) removes the boundary surrogate before sanitizeForConsole sees it. The sanitizer then sees exactly 200 code points and omits the ellipsis, so a truncated value can look complete. This is not a useful fix; current main has the safer observable behavior. Thanks for the report and patch.

@steipete steipete closed this Jul 9, 2026
@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jul 9, 2026
Closed
9 tasks
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

agents Agent runtime and tooling P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. 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

@lzyyzznl @steipete