Skip to content

fix(tasks): keep completion failure reason truncation UTF-16 safe#101616

Closed
wm0018 wants to merge 1 commit into
openclaw:mainfrom
wm0018:fix/task-contract-utf16-safe-truncation
Closed

fix(tasks): keep completion failure reason truncation UTF-16 safe#101616
wm0018 wants to merge 1 commit into
openclaw:mainfrom
wm0018:fix/task-contract-utf16-safe-truncation

Conversation

@wm0018

@wm0018 wm0018 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where task completion failure reasons could render broken U+FFFD replacement characters (�) when the failure reason contained an emoji or CJK supplementary character at the 159-character truncation boundary.

normalizeCompletionFailureReason caps failure reasons at 160 chars via normalized.slice(0, 159). String.prototype.slice cuts at UTF-16 code unit boundaries — emoji like 🚀 are surrogate pairs (2 code units) and get split.

Why This Change Was Made

Replace normalized.slice(0, 159) with truncateUtf16Safe(normalized, 159) — the standard helper already used in src/tasks/task-status.ts (same directory).

User Impact

Task completion failure reasons containing emoji or CJK near the 160-character limit now display cleanly truncated text.

Evidence

Tests

$ node scripts/run-vitest.mjs src/tasks/task-completion-contract.test.ts

 Test Files  1 passed (1)
      Tests  3 passed (3)

Files Changed (2 files, +31/-1)

 src/tasks/task-completion-contract.test.ts | 29 +++++++++++++++++++++++++++++
 src/tasks/task-completion-contract.ts      |  3 ++-

@wm0018
fix(tasks): keep completion failure reason truncation UTF-16 safe
92b3540 
String.prototype.slice at offset 159 can split surrogate pairs in
task completion failure reasons, producing broken U+FFFD in terminal
output and task registry views.

Replace raw slice(0, 159) with truncateUtf16Safe.
@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:00 AM ET / 12:00 UTC.

Summary
The branch replaces raw failure-reason slice truncation in task completion contracts with the existing UTF-16-safe helper and adds focused truncation tests.

PR surface: Source +1, Tests +29. Total +30 across 2 files.

Reproducibility: yes. source inspection plus a focused Node check shows the current raw slice can leave a lone high surrogate at the claimed boundary. I did not run a full task-completion UI flow for this 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] Add redacted real behavior proof from a running task-completion failure path showing the emoji-boundary reason renders without replacement characters.
  • Prefer changing or extending the new test to assert the exported completion-delivery resolver output, not only truncateUtf16Safe itself.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body reports a focused Vitest run only; before merge, add redacted real behavior proof such as terminal output, logs, or a screenshot/recording showing the after-fix task-completion failure summary, then update the PR body to trigger re-review.

Risk before merge

  • [P1] The PR body provides only focused Vitest output, not a real after-fix task-completion or terminal/status proof from a running setup.
  • [P1] The added test checks truncateUtf16Safe directly, so it would not fail if the private completion-failure normalization stopped using that helper later.

Maintainer options:

  1. Decide the mitigation before merge
    Merge the narrow helper-based fix after contributor or maintainer proof shows a real task-completion failure reason with an emoji boundary renders cleanly, ideally with a public resolver regression assertion added.
  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 code repair is needed now; the remaining blocker is contributor or maintainer real behavior proof for the external PR.

Security
Cleared: The diff only uses an existing local UTF-16 helper and adds tests; it does not change dependencies, workflows, secrets, packaging, or other supply-chain surfaces.

Review details

Best possible solution:

Merge the narrow helper-based fix after contributor or maintainer proof shows a real task-completion failure reason with an emoji boundary renders cleanly, ideally with a public resolver regression assertion added.

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

Yes, source inspection plus a focused Node check shows the current raw slice can leave a lone high surrogate at the claimed boundary. I did not run a full task-completion UI flow for this review.

Is this the best way to solve the issue?

Yes, using the existing truncateUtf16Safe helper at the failure-reason normalization point is the narrowest maintainable fix. The validation would be stronger if it asserted resolveRequiredCompletionDeliveryFailureTerminalResult output rather than only the helper.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a normal, limited-blast-radius user-facing formatting bug in task completion failure summaries.
  • 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: The PR body reports a focused Vitest run only; before merge, add redacted real behavior proof such as terminal output, logs, or a screenshot/recording showing the after-fix task-completion failure summary, then update the PR body to trigger re-review.

Label justifications:

  • P2: This is a normal, limited-blast-radius user-facing formatting bug in task completion failure summaries.
  • 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: The PR body reports a focused Vitest run only; before merge, add redacted real behavior proof such as terminal output, logs, or a screenshot/recording showing the after-fix task-completion failure summary, then update the PR body to trigger re-review.
Evidence reviewed

PR surface:

Source +1, Tests +29. Total +30 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 2 1 +1
Tests 1 29 0 +29
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 31 1 +30

What I checked:

Likely related people:

  • steipete: Authored and merged the recent task-completion contract and sibling task-status helper surfaces that this PR changes, as shown by blame and the merged fix(zalo): accept opaque string chat IDs #101548 metadata. (role: recent area contributor; confidence: high; commits: 6b76a306d472, 680ec032306e; files: src/tasks/task-completion-contract.ts, src/tasks/task-status.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 7, 2026
@steipete steipete mentioned this pull request Jul 7, 2026
Merged
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Thanks @wm0018. 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
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

@wm0018 @steipete