Skip to content

fix(file-transfer): preserve UTF-16 pairs in bounded stderr tails#104098

Closed
Pick-cat wants to merge 0 commit into
openclaw:mainfrom
Pick-cat:fix/utf16-file-transfer-tail
Closed

fix(file-transfer): preserve UTF-16 pairs in bounded stderr tails#104098
Pick-cat wants to merge 0 commit into
openclaw:mainfrom
Pick-cat:fix/utf16-file-transfer-tail

Conversation

@Pick-cat

@Pick-cat Pick-cat commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where appendBoundedTextTail (used to capture tar stderr output in dir-fetch-tool.ts and node-invoke-policy.ts) produces strings that start with a lone low surrogate when an emoji or other non-BMP character straddles the tail boundary. The .slice(-maxChars) call cuts at a UTF-16 code unit boundary, and when the cut point lands exactly on a LOW surrogate paired with a HIGH surrogate just before it, the result begins with a dangling \uDE00-style byte that corrupts downstream display and logging.

Why This Change Was Made

The fix replaces .slice(-maxChars) with sliceUtf16Safe(next, -maxChars) — the same canonical helper from @openclaw/normalization-core/utf16-slice already used across the codebase for UTF-16-safe truncation. Both files share an identical copy of appendBoundedTextTail; both are fixed in the same change to keep their behavior consistent.

User Impact

Tar operation error output (stderr) displayed in agent tool results no longer risks starting with a corrupted surrogate byte when the output contains emoji characters near the 4096-char tail window. No user-visible impact for ASCII-only stderr.

Real behavior proof

Behavior addressed: appendBoundedTextTail produces lone surrogates when emoji straddles the tail boundary.

Real environment tested: Node.js v22.22.0, x64, linux

Evidence after fix:

=== POSITIVE CONTROL (fixed: sliceUtf16Safe) ===

[case 1] emoji straddles tail boundary (cut at LOW surrogate)
  ok: free of lone surrogates
  ok: starts with ASCII, surrogate excluded
  ok: length <= maxChars
  result len=4095 starts="aaaaaaaa"

[case 2] emoji inside tail window – both halves survive
  ok: emoji preserved
  ok: free of lone surrogates

[case 3] multiple emojis near boundary
  ok: free of lone surrogates
  ok: all 3 emojis intact

[case 4] ASCII-only baseline
  ok: length = maxChars
  ok: identical to raw tail

[case 5] chunked stderr accumulation (6 chunks)
  ok: chunk1: clean
  ok: chunk2: clean
  ok: chunk3: clean
  ok: chunk4: clean
  ok: chunk5: clean
  ok: emoji chunk: clean
  ok: emoji chunk: preserved
  ok: final: clean

[case 6] short stderr (no truncation)
  ok: verbatim pass-through

Negative control (pre-fix behavior reproduced):

=== NEGATIVE CONTROL (old: .slice(-maxChars)) ===

[neg 1] emoji at tail boundary
  BUG: lone surrogate at [0] U+DE00 "\ude00aaa"
  ok: bug reproduced (negative control)

Summary:

maxChars=4096 node=v22.22.0 arch=x64
ALL PROOF ASSERTIONS: 19 passed, 0 failed

Evidence

  • pnpm test extensions/file-transfer/src/tools/dir-fetch-tool.test.ts — 8/8 tests pass
  • Proof script: node proof-file-transfer-utf16-tail.mjs — 19/19 assertions pass (6 positive cases + 1 negative control)
  • No competing PRs found via gh search prs for these files with UTF-16/surrogate keywords
  • Change is 2 files, +4/-2 lines — minimal, mechanical, uses existing infrastructure

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jul 11, 2026
@clawsweeper

clawsweeper Bot commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 10, 2026, 11:11 PM ET / July 11, 2026, 03:11 UTC.

Summary
The PR replaces raw negative string slicing with the public UTF-16-safe slice helper in the two file-transfer stderr-tail accumulators.

PR surface: Source +2. Total +2 across 2 files.

Reproducibility: yes. A deterministic current-main Node probe reproduces a retained tail beginning with lone U+DE00 when an emoji straddles the 4096-code-unit cutoff.

Review metrics: 1 noteworthy metric.

  • Diagnostic consumers: 4 corrected through 2 shared local helpers. The patch covers every production use of the duplicated file-transfer tail implementation without expanding into unrelated output paths.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Next step before merge

  • No automated repair is needed; the patch should proceed through ordinary maintainer merge review.

Security
Cleared: The diff adds only a repository-owned public SDK utility import and changes string slicing, with no security or supply-chain regression found.

Review details

Best possible solution:

Keep the existing stderr limits and route both duplicated file-transfer accumulators through the canonical UTF-16-safe helper, with exact-head checks gating merge.

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

Yes. A deterministic current-main Node probe reproduces a retained tail beginning with lone U+DE00 when an emoji straddles the 4096-code-unit cutoff.

Is this the best way to solve the issue?

Yes. Reusing the tested public UTF-16 helper in exactly the two duplicated owners is the narrowest maintainable fix and preserves existing limits and subprocess behavior.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: The PR fixes rare malformed diagnostic text in four file-transfer tar failure paths with limited blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix Node.js terminal output for the exact boundary, pair preservation, chunked accumulation, ASCII and short-input controls, plus a reproduced pre-fix negative control.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix Node.js terminal output for the exact boundary, pair preservation, chunked accumulation, ASCII and short-input controls, plus a reproduced pre-fix negative control.
Evidence reviewed

PR surface:

Source +2. Total +2 across 2 files.

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

What I checked:

Likely related people:

  • James Armstead: Git blame and file history connect the current file-transfer implementation, both bounded stderr helpers, and the canonical UTF-16 utility surface to the same feature-introduction commit. (role: introduced behavior; confidence: high; commits: 65cc86f45c84; files: extensions/file-transfer/src/shared/node-invoke-policy.ts, extensions/file-transfer/src/tools/dir-fetch-tool.ts, packages/normalization-core/src/utf16-slice.ts)
  • Josh Lehman: The introduction commit records Josh Lehman as a co-author of the change that added the relevant file-transfer and utility surfaces, though the exact ownership split is not visible in local history. (role: feature introduction co-author; confidence: low; commits: 65cc86f45c84; files: extensions/file-transfer/src/shared/node-invoke-policy.ts, extensions/file-transfer/src/tools/dir-fetch-tool.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.

@sallyom sallyom mentioned this pull request Jul 11, 2026
Merged
3 tasks
@Pick-cat Pick-cat closed this Jul 11, 2026
@Pick-cat
Pick-cat force-pushed the fix/utf16-file-transfer-tail branch from 0ac96fc to 4541969 Compare July 11, 2026 22:04
@Pick-cat

Pick-cat commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

冲突已解决，但 rebase 后发现此 PR 的修复已经被 main 分支完全覆盖。

main 上的 append-bounded-text-tail.ts 已将 appendBoundedTextTail 提取为共享模块，并同时使用了 sliceUtf16Safe 替代 .slice(-maxChars)，与本 PR 目的一致。

两个调用方文件 (dir-fetch-tool.ts 和 node-invoke-policy.ts) 现在都从该共享模块导入，因此不再需要独立的 inline 修复。

此 PR 可以关闭，因为修复已经在 main 上了。

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: file-transfer proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Pick-cat