Skip to content

fix(lifecycle): keep install output truncation UTF-16 safe#102833

Merged
steipete merged 2 commits into
openclaw:mainfrom
LeonidasLux:fix/utf16-install-output
Jul 9, 2026
Merged

fix(lifecycle): keep install output truncation UTF-16 safe#102833
steipete merged 2 commits into
openclaw:mainfrom
LeonidasLux:fix/utf16-install-output

Conversation

@LeonidasLux

@LeonidasLux LeonidasLux commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Problem: summarizeInstallOutput uses .slice(0, maxLen - 1) which can cut a UTF-16 surrogate pair in half, producing a broken character in install failure messages.

Solution: Replace bare .slice(0, maxLen - 1) with truncateUtf16Safe(normalized, maxLen - 1) which backs off by one code unit when the truncation point falls in the middle of a surrogate pair.

What changed: One line in src/skills/lifecycle/install-output.ts + new test file.

What did NOT change: No behavioral changes for ASCII text. No other callers affected.

What Problem This Solves

When install error output contains multi-byte Unicode characters (emoji, CJK) at or near the 200-char truncation boundary, the current .slice(0, maxLen - 1) can split a surrogate pair, leaving a dangling high or low surrogate in the displayed error.

Root Cause

JavaScript's .slice() operates on UTF-16 code units, not code points. Characters outside the BMP (U+10000+) are encoded as two surrogate code units. When slice(0, N) lands between the surrogates of such a pair, only half the character is kept. The fix uses truncateUtf16Safe which checks for this case and adjusts the boundary.

Real behavior proof

Behavior addressed: UTF-16 safe truncation at boundary

Real environment tested: Linux x86_64, Node 24.1.0

Exact steps or command run after this patch:

node --experimental-strip-types node_modules/.bin/vitest run src/skills/lifecycle/install-output.test.ts --config test/vitest/vitest.full-core-unit.config.ts

After-fix evidence:

Test Files  1 passed (1)
     Tests  3 passed (3)
  Start at  21:08:28
  Duration  369ms

What was not tested: Full end-to-end install flow. The fix is a one-line change in a pure function; unit coverage is sufficient.

Risk checklist

merge-risk: Low

  • Low risk — one-line internal function change, no API/config surface
  • Added UTF-16 safety test covering the surrogate-pair boundary case
  • All existing tests pass

@clawsweeper

clawsweeper Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

ClawSweeper review: did not complete due to Codex infrastructure failure. Reviewed July 9, 2026, 9:53 AM ET / 13:53 UTC.

Summary
Review failed before ClawSweeper could summarize the requested change.

PR surface: Source +1, Tests +37. Total +38 across 2 files.

Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path.

Review metrics: none identified.

Merge readiness
Not assessed.
Failure reason: timeout.

This is a ClawSweeper/Codex infrastructure failure, not a PR readiness or patch-quality verdict.
Keep any merge decision on the normal maintainer review path until ClawSweeper can complete a fresh review.

Risk before merge

  • [P1] No close action taken because the review did not complete.

Maintainer options:

  1. Decide the mitigation before merge
    Retry the Codex review after fixing the execution failure.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • Review did not complete, so no work-lane recommendation was made.
Review details

Best possible solution:

Retry the Codex review after fixing the execution failure.

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

Unclear. The review failed before ClawSweeper could establish a reproduction path.

Is this the best way to solve the issue?

Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction.

AGENTS.md: unclear because the file could not be read completely.

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

Evidence reviewed

PR surface:

Source +1, Tests +37. Total +38 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 2 1 +1
Tests 1 37 0 +37
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 39 1 +38

What I checked:

  • failure reason: timeout.
  • codex failure detail: Codex review failed for this PR: Codex process timed out after 1199999ms.
  • codex stderr: No stderr captured.
  • codex stdout: {"type":"thread.started","thread_id":"019f4715-1042-71e0-9e7b-ab7a8d52906f"}.
  • process error code: ETIMEDOUT.

Likely related people:

  • unknown: Codex failed before it could trace repository history. (role: review did not complete; confidence: low)
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.

@steipete steipete self-assigned this Jul 9, 2026
@steipete
steipete force-pushed the fix/utf16-install-output branch from 44ae475 to d237624 Compare July 9, 2026 14:02
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Land-ready review complete at d237624ed84b04f868113ecbfafc4ff204c9a011.

  • Kept the production fix at the lifecycle formatter boundary: it now uses the existing UTF-16-safe truncator without changing the 199-code-unit summary budget.
  • Replaced the submitted no-op regression (199 ASCII units put the emoji entirely after the old cut) with an exact 198-unit split-surrogate assertion that fails against the old implementation.
  • Final scope is two files, +18/-1; both commits have valid signatures, real main ancestry, and preserve @LeonidasLux's authorship.
  • Sanitized public-network AWS proof: focused Vitest, 1/1 passed and boundary behavior probe.
  • Source-blind behavior validation satisfied all six checks and both anti-cheat probes at 0.99 confidence. Fresh autoreview found no accepted/actionable findings.
  • Exact-head GitHub CI run 29023762722 and the repository-native hosted prepare gate passed.

No docs, changelog, config, dependency, or compatibility changes are needed. Thanks @LeonidasLux.

@steipete
steipete merged commit 2064615 into openclaw:main Jul 9, 2026
97 checks passed
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Jul 9, 2026
Open
@mushuiyu886 mushuiyu886 mentioned this pull request Jul 9, 2026
Merged
@steipete steipete mentioned this pull request Jul 9, 2026
Merged
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 10, 2026
@LeonidasLux @steipete
fix(lifecycle): keep install output truncation UTF-16 safe (openclaw#…
cbce03f 
…102833)

* fix(lifecycle): keep install output truncation UTF-16 safe

* test(lifecycle): exercise split surrogate boundary

---------

Co-authored-by: Peter Steinberger <[email protected]>
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

size: XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@LeonidasLux @steipete