fix(mattermost): keep slash command error truncation UTF-16 safe#102607
Conversation
Replace `.slice(0, 300)` and `.slice(0, 200)` with `truncateUtf16Safe` in sanitizeCommandLookupError and sanitizeMattermostLogValue to prevent surrogate pair corruption. Ref. lsr911 pattern — mechanical substitution, no behavior change.
openclaw-barnacle
Bot
added
channel: mattermost
|
Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 7:13 AM ET / 11:13 UTC. Summary PR surface: Source +1, Tests +32. Total +33 across 2 files. Reproducibility: yes. from source inspection: current main slices sanitized Mattermost slash-command strings at fixed UTF-16 code-unit lengths, so a surrogate pair crossing the 300 or 200 boundary can be split. I did not establish a live Mattermost reproduction. Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land one narrow Mattermost slash-http truncation fix after redacted real runtime proof demonstrates surrogate-boundary diagnostics, then close the duplicate branch. Do we have a high-confidence way to reproduce the issue? Yes from source inspection: current main slices sanitized Mattermost slash-command strings at fixed UTF-16 code-unit lengths, so a surrogate pair crossing the 300 or 200 boundary can be split. I did not establish a live Mattermost reproduction. Is this the best way to solve the issue? Yes for the code shape: using the existing plugin SDK text helper at the final truncation step is the narrowest maintainable fix and preserves sanitizer ordering and limits. The remaining gap is real behavior proof, not a better implementation layer. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5ba981a4370d. Label changesLabel justifications:
Evidence reviewedPR surface: Source +1, Tests +32. Total +33 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (1 earlier review cycle)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Co-authored-by: 黄剑雄0668001315 <[email protected]>
|
Land-ready review complete on exact head
No docs or changelog entry is required because this only repairs bounded, redacted diagnostics. |
|
Merged via squash.
|
…nclaw#102607) * fix(mattermost): keep slash command error truncation UTF-16 safe Replace `.slice(0, 300)` and `.slice(0, 200)` with `truncateUtf16Safe` in sanitizeCommandLookupError and sanitizeMattermostLogValue to prevent surrogate pair corruption. Ref. lsr911 pattern — mechanical substitution, no behavior change. * test(mattermost): cover UTF-16-safe slash logs Co-authored-by: 黄剑雄0668001315 <[email protected]> --------- Co-authored-by: Peter Steinberger <[email protected]>
2 participants
What Problem This Solves
Mattermost slash-command fallback logs bound untrusted command IDs and upstream error text with raw UTF-16 slicing. An emoji crossing the 200- or 300-code-unit boundary could leave a dangling surrogate in logs.
Why This Change Was Made
Apply the shared
truncateUtf16Safeprimitive after the existing newline and secret sanitization. The sanitizer remains one canonical path and keeps its existing redaction order and limits.User Impact
Mattermost slash-command diagnostics remain valid Unicode without exposing more data. Command validation, fallback behavior, redaction, rate limiting, HTTP responses, and configuration are unchanged.
Evidence
node ../../node_modules/vitest/vitest.mjs run --config test/vitest/vitest.extension-mattermost.config.ts extensions/mattermost/src/mattermost/slash-http.test.ts— 22 passedoxfmt, focusedoxlint,git diff --checkNo live Mattermost server is needed because network requests, validation decisions, and emitted log fields are unchanged; the focused tests exercise the real lookup/fallback paths.
Risk
Low. Presentation-only error formatting; no API, config, dependency, or persistence changes. Production LOC decreases relative to
main.AI-assisted
Initial patch generated with Claude Code; maintainer review simplified the sanitizer and added public-path regression coverage.