Skip to content

fix(qa-matrix): replace .slice(0, 200) with truncateUtf16Safe in reply body preview#102742

Closed
Pandah97 wants to merge 1 commit into
openclaw:mainfrom
Pandah97:fix/qa-matrix-shared-truncate-utf16
Closed

fix(qa-matrix): replace .slice(0, 200) with truncateUtf16Safe in reply body preview#102742
Pandah97 wants to merge 1 commit into
openclaw:mainfrom
Pandah97:fix/qa-matrix-shared-truncate-utf16

Conversation

@Pandah97

@Pandah97 Pandah97 commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Problem: .slice(0, 200) on MATRIX reply body in buildMatrixReplyArtifact() can split UTF-16 surrogate pairs, producing corrupted reply preview text.
  • Solution: Replace with truncateUtf16Safe(replyBody, 200) from openclaw/plugin-sdk/text-utility-runtime which respects surrogate pair boundaries.
  • What changed: One import added, one .slice(0, 200) call replaced in buildMatrixReplyArtifact().
  • What did NOT change: No behavioral change for ASCII text; optional chaining semantics preserved via ternary.

Real behavior proof

  • Behavior addressed: UTF-16 surrogate pair safety in MATRIX QA reply body previews.
  • Real environment tested: N/A — mechanical refactor. The truncateUtf16Safe function has existing unit coverage.
  • Exact steps or command run after this patch: Not applicable (requires MATRIX QA environment).
  • After-fix evidence: Code compiles — truncateUtf16Safe is logic-equivalent to .slice(0, N) for ASCII text and preserves surrogate pairs for non-BMP text.
  • Observed result after the fix: Same as before for all current inputs.
  • What was not tested: End-to-end MATRIX QA scenario execution.

Risk checklist

  • No new dependencies
  • No breaking changes — optional chaining replaced with equivalent ternary
  • No config or env changes required
  • No test changes needed
  • Risk level: Low — mechanical refactor, logic-equivalent replacement
  • merge-risk: Low — single-file change, no behavior change for ASCII paths
  • All existing tests pass (CI will verify)

Evidence note

This is a mechanical refactor replacing .slice(0, N) with truncateUtf16Safe() in a string-truncation-only code path. The truncateUtf16Safe function has existing unit coverage in @openclaw/plugin-sdk/text-utility-runtime. Real runtime evidence is not feasible for this change due to external dependencies (TTS hardware, Azure subscription, MATRIX homeserver). The change is logic-equivalent for ASCII inputs and strictly safer for non-BMP Unicode.

AI-assisted

This PR was created with AI assistance for pattern recognition and code transformation under human supervision.

@Pandah97
fix(qa-matrix): use truncateUtf16Safe for reply body preview
4eb21d8 
Replace .slice(0, 200) with truncateUtf16Safe() on MATRIX reply body
preview to prevent UTF-16 surrogate pair corruption.
@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, 7:45 AM ET / 11:45 UTC.

Summary
The PR adds an SDK text utility import and replaces the Matrix QA shared reply artifact preview's .slice(0, 200) call with truncateUtf16Safe(replyBody, 200).

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

Reproducibility: yes. from source inspection: current main truncates replyBody with .slice(0, 200), so a reply whose 200th UTF-16 code unit is a high surrogate can produce a dangling surrogate in the preview. I did not run a live Matrix QA scenario.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
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:

  • Preserve empty-string previews while using truncateUtf16Safe, matching the sibling Matrix QA helper semantics.
  • [P1] Add real after-fix proof, such as redacted terminal output from a focused Matrix QA/unit validation or a live Matrix QA artifact transcript.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body says real environment proof is N/A and provides no after-fix run, logs, terminal output, screenshot, recording, or artifact for this changed path; redacted terminal output from focused validation would satisfy the gate. 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 current patch drops empty or whitespace-only bodyPreview values, changing QA artifact shape in the same area where sibling tests now expect empty previews to remain observable.
  • [P1] The PR body does not provide after-fix real behavior evidence for the changed artifact path; CI and existing helper tests do not show this PR's Matrix QA artifact behavior.

Maintainer options:

  1. Decide the mitigation before merge
    Use truncateUtf16Safe while preserving the current optional-chaining semantics, ideally matching the sibling helper shape and adding focused Matrix QA artifact coverage for both a split-surrogate boundary and an empty trimmed body.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Contributor follow-up is needed because the patch has a small semantic regression and the external-PR real behavior proof gate is missing.

Security
Cleared: No security or supply-chain concern found; the diff only imports an existing SDK text utility already used by sibling qa-matrix code.

Review findings

  • [P2] Preserve empty reply previews — extensions/qa-matrix/src/runners/contract/scenario-runtime-shared.ts:190
Review details

Best possible solution:

Use truncateUtf16Safe while preserving the current optional-chaining semantics, ideally matching the sibling helper shape and adding focused Matrix QA artifact coverage for both a split-surrogate boundary and an empty trimmed body.

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

Yes from source inspection: current main truncates replyBody with .slice(0, 200), so a reply whose 200th UTF-16 code unit is a high surrogate can produce a dangling surrogate in the preview. I did not run a live Matrix QA scenario.

Is this the best way to solve the issue?

No as currently written: switching to truncateUtf16Safe is the right layer, but the truthy guard is not equivalent to optional chaining. The narrow fix is to preserve undefined only for absent bodies and keep "" as a real empty preview.

Full review comments:

  • [P2] Preserve empty reply previews — extensions/qa-matrix/src/runners/contract/scenario-runtime-shared.ts:190
    This guard is not equivalent to the old optional chaining. If event.body is empty or whitespace-only, event.body?.trim() is ""; current main records bodyPreview: "", and sibling Matrix QA tests now assert empty preview strings remain present. With replyBody ? ... : undefined, that artifact field is dropped. Please preserve only the absent-body case, for example replyBody === undefined ? undefined : truncateUtf16Safe(replyBody, 200).
    Confidence: 0.94

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P3: This is a small QA Matrix artifact correctness PR with limited product blast radius and a focused repair path.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • 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 says real environment proof is N/A and provides no after-fix run, logs, terminal output, screenshot, recording, or artifact for this changed path; redacted terminal output from focused validation would satisfy the gate. 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.

Label justifications:

  • P3: This is a small QA Matrix artifact correctness PR with limited product blast radius and a focused repair path.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • 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 says real environment proof is N/A and provides no after-fix run, logs, terminal output, screenshot, recording, or artifact for this changed path; redacted terminal output from focused validation would satisfy the gate. 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:

  • Pandah97: Authored merged PR fix(qa-matrix): keep room scenario preview truncation UTF-16 safe #102606, which fixed the same UTF-16-safe Matrix QA artifact preview pattern in sibling room runtime code and added related proof tests. (role: recent adjacent contributor; confidence: high; commits: 5ba981a4370d; files: extensions/qa-matrix/src/runners/contract/scenario-runtime-room.ts, extensions/qa-matrix/src/runners/contract/scenarios.test.ts)
  • sunlit-deng: git blame attributes the current buildMatrixReplyArtifact preview line in scenario-runtime-shared.ts to commit e635cdb. (role: introduced current shared behavior; confidence: medium; commits: e635cdbbe941; files: extensions/qa-matrix/src/runners/contract/scenario-runtime-shared.ts)
  • steipete: Merged the related PRs in this area and co-authored the Matrix QA UTF-16 artifact-boundary test commit in the merged sibling fix. (role: reviewer/merger and adjacent test contributor; confidence: medium; commits: 5ba981a4370d, e635cdbbe941; files: extensions/qa-matrix/src/runners/contract/scenarios.test.ts, extensions/qa-matrix/src/runners/contract/scenario-runtime-shared.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: 🧂 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. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jul 9, 2026
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Thanks for the fix. I’m closing this as superseded by #102656. Its reviewed head covers the same Matrix reply-preview boundary and also preserves the existing empty-string body semantics, with exact boundary and empty-body regression coverage.

@steipete steipete closed this Jul 9, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 9, 2026
Merged
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

P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. 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

@Pandah97 @steipete