Skip to content

fix(cli): keep CLI display/approval truncation UTF-16 safe#101633

Closed
lsr911 wants to merge 1 commit into
openclaw:mainfrom
lsr911:fix/cli-commands-utf16
Closed

fix(cli): keep CLI display/approval truncation UTF-16 safe#101633
lsr911 wants to merge 1 commit into
openclaw:mainfrom
lsr911:fix/cli-commands-utf16

Conversation

@lsr911

@lsr911 lsr911 commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Four CLI/onboarding call sites truncate user-provided text with raw .slice(0, N):

  • cli/error-format: JSON input preview @45 — user-provided CLI args
  • cli/exec-approvals-cli: approval display text @300 — operator-visible messages
  • onboarding-plugin-install: plugin descriptions @179 — plugin readme/spec text
  • onboard-helpers: TUI display lines @119 — user-facing wizard text

All can contain emoji or CJK characters, producing U+FFFD in terminal output.

Why This Change Was Made

truncateUtf16Safe from @openclaw/normalization-core/utf16-slice is the canonical leaf helper.

Evidence

Branch prerun (commit c601761)

=== error-format @45 ===
Raw .slice(0,45) ends: "x\ud83d" (dangling)
truncateUtf16Safe: len=44 | no U+FFFD

=== exec-approvals @300 ===
Raw .slice(0,300) ends: "s\ud83d" (dangling)
truncateUtf16Safe: len=299 | no U+FFFD

Files Changed

File Change
src/cli/error-format.ts .slice(0, 45) → truncateUtf16Safe + import
src/cli/exec-approvals-cli.ts .slice(0, 300) → truncateUtf16Safe + import
src/commands/onboarding-plugin-install.ts .slice(0, 179) → truncateUtf16Safe + import
src/commands/onboard-helpers.ts .slice(0, 119) → truncateUtf16Safe + import

@lsr911
fix(cli): keep CLI display/approval truncation UTF-16 safe
c601761 
Replace raw .slice(0, N) with truncateUtf16Safe in CLI display
paths that may truncate user-provided text containing emoji or
CJK characters:

- cli/error-format: JSON input preview @45
- cli/exec-approvals-cli: approval display text @300
- commands/onboarding-plugin-install: plugin description @179
- commands/onboard-helpers: TUI display line @119
@openclaw-barnacle openclaw-barnacle Bot added cli CLI command changes commands Command implementations size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed 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

Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 8:17 AM ET / 12:17 UTC.

Summary
The branch imports truncateUtf16Safe into four CLI/onboarding modules and replaces raw .slice(0, N) display truncation in strict JSON, exec approvals, gateway probe, and plugin install error paths.

PR surface: Source +54. Total +54 across 4 files.

Reproducibility: yes. from source inspection rather than a live CLI run: current main still truncates user-visible strings with raw .slice(0, N) at the four reported sites. The existing helper tests show surrogate-boundary truncation behavior that avoids dangling halves.

Review metrics: none identified.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until stronger 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] Add redacted real terminal output or logs for the onboarding helper and plugin install error paths, alongside the two CLI truncation examples already shown.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body has copied branch prerun output for two truncation paths, but it does not show the other two changed paths or actual OpenClaw CLI/onboarding runs; add redacted terminal output, logs, screenshots, or copied live output, then update the PR body to trigger re-review or ask a maintainer for @clawsweeper re-review.

Risk before merge

  • [P1] Contributor real-behavior proof currently covers only two of the four changed CLI/onboarding display paths, so the external-PR proof gate is not fully satisfied yet.

Maintainer options:

  1. Decide the mitigation before merge
    Land the narrow helper substitution after the contributor adds redacted real CLI/onboarding output for all changed display paths, without adding new API or config surface.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Needs contributor proof review before merge; there is no narrow ClawSweeper code repair to queue because no patch defect was found.

Security
Cleared: The diff only imports an existing workspace helper and changes CLI display truncation; it does not touch dependencies, workflows, lockfiles, secrets, permissions, or code execution paths.

Review details

Best possible solution:

Land the narrow helper substitution after the contributor adds redacted real CLI/onboarding output for all changed display paths, without adding new API or config surface.

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

Yes, from source inspection rather than a live CLI run: current main still truncates user-visible strings with raw .slice(0, N) at the four reported sites. The existing helper tests show surrogate-boundary truncation behavior that avoids dangling halves.

Is this the best way to solve the issue?

Yes: replacing these preview/error truncations with the existing leaf truncateUtf16Safe helper is the narrowest maintainable fix. A broader grapheme- or width-aware refactor is not needed for these character-count preview clamps.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P3: This is a low-blast-radius CLI/onboarding display correctness fix for truncation polish, with no runtime availability, config, data, or security impact shown.
  • 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 stronger real behavior proof before merge: The PR body has copied branch prerun output for two truncation paths, but it does not show the other two changed paths or actual OpenClaw CLI/onboarding runs; add redacted terminal output, logs, screenshots, or copied live output, then update the PR body to trigger re-review or ask a maintainer for @clawsweeper re-review.

Label justifications:

  • P3: This is a low-blast-radius CLI/onboarding display correctness fix for truncation polish, with no runtime availability, config, data, or security impact shown.
  • 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 stronger real behavior proof before merge: The PR body has copied branch prerun output for two truncation paths, but it does not show the other two changed paths or actual OpenClaw CLI/onboarding runs; add redacted terminal output, logs, screenshots, or copied live output, then update the PR body to trigger re-review or ask a maintainer for @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +54. Total +54 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 4 71 17 +54
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 4 71 17 +54

What I checked:

  • Repository policy applied: Root AGENTS.md was read fully; its PR review, dependency-inspection, owner-boundary, and real-behavior-proof guidance affected this review. (AGENTS.md:7, 2ba622ca3019)
  • Current main still has the four raw truncation sites: Current main uses raw .slice(0, N) in the four display/error paths changed by this PR, so the PR is not already implemented on main. (src/cli/error-format.ts:62, 2ba622ca3019)
  • Helper contract is the right local dependency: truncateUtf16Safe floors the requested limit, clamps it nonnegative, and delegates to sliceUtf16Safe, which adjusts slice edges away from dangling surrogate halves. (packages/normalization-core/src/utf16-slice.ts:44, 2ba622ca3019)
  • Helper has focused surrogate-boundary coverage: Existing tests cover normal truncation, zero/negative/decimal limits, emoji surrogate pairs, and truncating at a surrogate boundary. (packages/normalization-core/src/utf16-slice.test.ts:56, 2ba622ca3019)
  • PR diff is bounded to display truncation and formatting churn: The diff replaces the four raw string truncations with the existing helper and otherwise only applies formatter wrapping in the same files. (src/cli/error-format.ts:60, c601761c3583)
  • Contributor proof is partial: The PR body includes copied branch prerun output for the strict JSON and exec approvals truncation examples, but not for the onboarding helper or plugin install error paths. (c601761c3583)

Likely related people:

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. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jul 7, 2026
@steipete steipete mentioned this pull request Jul 7, 2026
Merged
@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
@clawsweeper clawsweeper Bot mentioned this pull request Jul 8, 2026
Closed
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

cli CLI command changes commands Command implementations P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. 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