Skip to content

fix(agent-runner-execution): use truncateUtf16Safe for external run failure detail#102674

Closed
zhangqueping wants to merge 1 commit into
openclaw:mainfrom
zhangqueping:fix/utf16-safe-agent-runner-execution
Closed

fix(agent-runner-execution): use truncateUtf16Safe for external run failure detail#102674
zhangqueping wants to merge 1 commit into
openclaw:mainfrom
zhangqueping:fix/utf16-safe-agent-runner-execution

Conversation

@zhangqueping

@zhangqueping zhangqueping commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

The external run failure detail formatter in formatForwardedExternalRunFailureText truncates sanitized error messages with a raw UTF-16 code-unit slice. When an error message contains an emoji at the truncation boundary, the raw slice can produce an unpaired surrogate in the user-visible failure message.

Why This Change Was Made

Replace .slice(0, EXTERNAL_RUN_FAILURE_DETAIL_MAX_CHARS - 1) with truncateUtf16Safe. The module already imports truncateUtf16Safe for other truncation paths — this extends coverage to the remaining call site. The existing limit, trim, and ellipsis behavior remain unchanged.

User Impact

Agent failure messages shown to users remain valid Unicode when long error text contains emoji or other supplementary characters at the truncation boundary.

Evidence

  • Existing suite covers the external run failure path.
  • npx vitest run src/auto-reply/reply/agent-runner-execution.test.ts — 1 file, 234 tests passed.

Files Changed

File Change
src/auto-reply/reply/agent-runner-execution.ts Replace raw .slice(0,N) with truncateUtf16Safe in formatForwardedExternalRunFailureText.

🤖 Generated with Claude Code

@zhangqueping
fix(agent-runner-execution): use truncateUtf16Safe for external run f…
aa530d6 
…ailure detail

Replace raw UTF-16 slice in formatForwardedExternalRunFailureText
with truncateUtf16Safe to prevent surrogate pair splitting. The
module already imports truncateUtf16Safe for other truncation
paths — this commit extends coverage to the remaining call site.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@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:48 AM ET / 10:48 UTC.

Summary
The PR replaces raw UTF-16 slicing with truncateUtf16Safe when formatting verbose external agent-run failure details.

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

Reproducibility: yes. from source inspection: a verbose external failure detail over 900 UTF-16 code units can hit the raw slice boundary and leave a dangling surrogate. I did not run a live OpenClaw reproduction in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
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] Post redacted after-fix real behavior proof for a long verbose external failure containing emoji or supplementary characters at the truncation boundary.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Needs contributor action: current proof is only a test-command claim/CI, so please add redacted real behavior proof such as terminal output, logs, a recording, or a screenshot that directly shows the fixed failure path; updating the PR body should trigger a fresh ClawSweeper 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

  • [P1] The only merge blocker found is proof quality: the contributor has not shown an after-fix real OpenClaw run, terminal output, recording, or redacted log for the verbose external failure path.

Maintainer options:

  1. Decide the mitigation before merge
    Merge the helper substitution after contributor-provided or maintainer-overridden real behavior proof shows the verbose external failure path keeps long emoji-containing details valid.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Manual review only because the remaining blocker is contributor real-behavior proof, not an automatable code repair.

Security
Cleared: The diff only swaps a user-facing string truncation call to an existing internal helper and does not alter secrets, dependencies, permissions, CI, or code execution paths.

Review details

Best possible solution:

Merge the helper substitution after contributor-provided or maintainer-overridden real behavior proof shows the verbose external failure path keeps long emoji-containing details valid.

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

Yes from source inspection: a verbose external failure detail over 900 UTF-16 code units can hit the raw slice boundary and leave a dangling surrogate. I did not run a live OpenClaw reproduction in this read-only review.

Is this the best way to solve the issue?

Yes for the code shape: replacing the implicated raw slice with the existing surrogate-safe helper is the narrowest maintainable fix. The remaining gap is proof, not a different implementation layer.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a focused user-visible bug fix for invalid Unicode in agent failure details, with limited blast radius.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Needs contributor action: current proof is only a test-command claim/CI, so please add redacted real behavior proof such as terminal output, logs, a recording, or a screenshot that directly shows the fixed failure path; updating the PR body should trigger a fresh ClawSweeper 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 focused user-visible bug fix for invalid Unicode in agent failure details, with limited blast radius.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Needs contributor action: current proof is only a test-command claim/CI, so please add redacted real behavior proof such as terminal output, logs, a recording, or a screenshot that directly shows the fixed failure path; updating the PR body should trigger a fresh ClawSweeper 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 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:

  • Repository policy applied: Root AGENTS.md was read fully; no scoped AGENTS.md owns src/auto-reply, so the root review-depth, read-only, and real-behavior-proof rules apply. (AGENTS.md:10, 5154fe08fa0b)
  • Current main behavior: Current main sanitizes the external failure detail and then truncates with raw .slice(0, EXTERNAL_RUN_FAILURE_DETAIL_MAX_CHARS - 1), which can split a surrogate pair after sanitization. (src/auto-reply/reply/agent-runner-execution.ts:953, 5154fe08fa0b)
  • Proposed patch: The PR changes that exact truncation expression to truncateUtf16Safe(...).trimEnd() without changing the limit, suffix, or surrounding user-facing copy. (src/auto-reply/reply/agent-runner-execution.ts:955, aa530d6289b6)
  • Helper contract: truncateUtf16Safe floors the limit, returns the input when already within the limit, and delegates truncation to sliceUtf16Safe, whose contract is to avoid dangling surrogate halves. (packages/normalization-core/src/utf16-slice.ts:43, 5154fe08fa0b)
  • Adjacent tests: Existing tests cover the verbose external failure path and the UTF-16 helper, but there is no PR-provided after-fix real run showing this user-facing failure path with boundary emoji text. (src/auto-reply/reply/agent-runner-execution.test.ts:6885, 5154fe08fa0b)
  • Proof state: The PR body cites an npx vitest run command and the live PR has no comments or media/log artifact demonstrating the changed behavior in a real OpenClaw setup. (aa530d6289b6)

Likely related people:

  • obviyus: Blame and commit-to-PR lookup show this contributor introduced the current formatter and the UTF-16 helper in the recent run-lifecycle PR that touched agent-runner-execution and its tests. (role: recent area contributor; confidence: high; commits: 4af2ca0a4d78; files: src/auto-reply/reply/agent-runner-execution.ts, src/auto-reply/reply/agent-runner-execution.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.

@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 9, 2026
@steipete steipete mentioned this pull request Jul 9, 2026
Merged
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Thanks @zhangqueping. #102823 landed the same UTF-16-safe external-run failure-detail cap in 5ef269c. The exact runtime change is now on main, so this branch is fully superseded and I’m closing it.

@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

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: 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

@zhangqueping @steipete