Skip to content

fix(migrate-claude): use truncateUtf16Safe for skill description truncation#102616

Merged
steipete merged 2 commits into
openclaw:mainfrom
lzyyzznl:fix/truncateUtf16Safe-migrate-claude
Jul 9, 2026
Merged

fix(migrate-claude): use truncateUtf16Safe for skill description truncation#102616
steipete merged 2 commits into
openclaw:mainfrom
lzyyzznl:fix/truncateUtf16Safe-migrate-claude

Conversation

@lzyyzznl

@lzyyzznl lzyyzznl commented Jul 9, 2026 •
edited by steipete
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Claude command migration generates a YAML description from the command's first paragraph. A raw 180-unit UTF-16 slice could leave a dangling surrogate, producing malformed generated skill metadata when an emoji crossed the boundary.

Why This Change Was Made

Use the shared public plugin-SDK truncation primitive at the existing 180-unit bound. Source discovery, paragraph normalization, JSON/YAML quoting, and the imported command body are unchanged.

User Impact

Generated skills from long Claude commands retain valid Unicode metadata. The existing description size and all ASCII output remain unchanged.

Evidence

  • Added an exact apply-path regression that reads the generated SKILL.md and asserts its description line at the surrogate boundary.
  • Focused Claude migration provider suite: 5 passed.
  • Focused oxfmt, oxlint, and git diff --check passed.
  • Fresh autoreview clean (0.99).

Generated with Claude Code; reviewed and improved by an OpenClaw maintainer agent.

@lzyyzznl
fix(migrate-claude): use truncateUtf16Safe for skill description trun…
c57471b 
…cation

One .slice(0, 180) truncation site in the migrate-claude extension
may cut UTF-16 surrogate pairs in half when the skill description
contains multi-byte characters such as emoji. Replace it with
truncateUtf16Safe to keep the truncated output valid Unicode.
@openclaw-barnacle openclaw-barnacle Bot added plugin: migrate-claude 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:41 AM ET / 10:41 UTC.

Summary
The PR imports the SDK UTF-16 truncation helper and uses it for generated migrate-claude command-skill descriptions instead of raw slice(0, 180).

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

Reproducibility: yes. from source inspection and a read-only Node probe: current main's description.slice(0, 180) can leave a dangling high surrogate when an emoji begins at the truncation boundary. I did not run the full migrate-claude flow in this read-only review.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: extensions/migrate-claude/skills.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
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] Add a redacted migrate-claude run or generated SKILL.md excerpt showing a description whose 180-code-unit boundary bisects an emoji is not left with a dangling surrogate.
  • After adding proof, update the PR body so ClawSweeper re-reviews automatically; if it does not, ask a maintainer to comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body and comments do not include after-fix real behavior proof such as a redacted migrate-claude run, copied terminal output, log, recording, or generated SKILL.md excerpt; private paths, tokens, phone numbers, endpoints, and other private details should be redacted before posting evidence. 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 external PR still has no visible after-fix real migrate-claude run, terminal output, log, recording, or generated SKILL.md excerpt proving the boundary-emoji case in a real setup.
  • [P1] A newer open PR has the same diff, so maintainers should land one canonical branch after proof rather than merging duplicate patches.

Maintainer options:

  1. Decide the mitigation before merge
    Land one canonical PR after redacted real migrate-claude output or a generated SKILL.md excerpt proves a description boundary inside an emoji remains valid, then close the duplicate PR.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] No ClawSweeper repair job is appropriate because the remaining blocker is contributor-supplied real behavior proof and duplicate-PR coordination, not an actionable code defect in this branch.

Security
Cleared: The diff only imports an existing SDK helper and changes one string truncation call; it does not alter dependencies, CI, secrets, package metadata, permissions, or code execution paths.

Review details

Best possible solution:

Land one canonical PR after redacted real migrate-claude output or a generated SKILL.md excerpt proves a description boundary inside an emoji remains valid, then close the duplicate PR.

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

Yes from source inspection and a read-only Node probe: current main's description.slice(0, 180) can leave a dangling high surrogate when an emoji begins at the truncation boundary. I did not run the full migrate-claude flow in this read-only review.

Is this the best way to solve the issue?

Yes at the code level: reusing the exported plugin SDK truncateUtf16Safe helper is narrower than a plugin-local duplicate and matches the helper's tested contract. Merge readiness still depends on contributor-supplied real behavior proof and duplicate-PR cleanup.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a focused migrate-claude generated-description bug with limited blast radius and no core runtime outage.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab 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: The PR body and comments do not include after-fix real behavior proof such as a redacted migrate-claude run, copied terminal output, log, recording, or generated SKILL.md excerpt; private paths, tokens, phone numbers, endpoints, and other private details should be redacted before posting evidence. 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 +1. Total +1 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 2 1 +1
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 2 1 +1

What I checked:

Likely related people:

  • obviyus: git blame shows the current generatedCommandSkillContent block and raw slice line came through commit eedeb1e..., associated with merged PR 102160 by obviyus. (role: recent area contributor; confidence: high; commits: eedeb1eccb31; files: extensions/migrate-claude/skills.ts)
  • vincentkoc: Full-history log and tag inspection show v2026.6.11 includes the migrate-claude skills implementation and adjacent provider tests from commit e085fa1... authored by Vincent Koc. (role: shipped feature contributor; confidence: medium; commits: e085fa1a3ffd; files: extensions/migrate-claude/skills.ts, extensions/migrate-claude/provider.test.ts)
  • steipete: History and PR metadata tie the UTF-16 truncation helper surface to PR 567 and repeated normalization-helper maintenance by Peter Steinberger/steipete. (role: adjacent helper owner; confidence: high; commits: 63f5fa47deb6; files: src/utils.ts, packages/normalization-core/src/utf16-slice.ts, packages/normalization-core/src/utf16-slice.test.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 (2 earlier review cycles)
  • reviewed 2026-07-09T09:43:07.835Z sha c57471b :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-09T10:22:47.906Z sha c57471b :: needs real behavior proof before merge. :: none

@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. 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
Closed
@steipete
steipete merged commit d750477 into openclaw:main Jul 9, 2026
93 of 95 checks passed
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 10, 2026
@lzyyzznl @steipete
fix(migrate-claude): use truncateUtf16Safe for skill description trun…
6880ef0 
…cation (openclaw#102616)

* fix(migrate-claude): use truncateUtf16Safe for skill description truncation

One .slice(0, 180) truncation site in the migrate-claude extension
may cut UTF-16 surrogate pairs in half when the skill description
contains multi-byte characters such as emoji. Replace it with
truncateUtf16Safe to keep the truncated output valid Unicode.

* test(migrate-claude): cover UTF-16 skill description

---------

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

No one assigned

Labels

P2 Normal backlog priority with limited blast radius. plugin: migrate-claude 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