Skip to content

fix(discord): keep persona names UTF-16 safe#101644

Closed
ly85206559 wants to merge 2 commits into
openclaw:mainfrom
ly85206559:codex/discord-utf16-persona-names
Closed

fix(discord): keep persona names UTF-16 safe#101644
ly85206559 wants to merge 2 commits into
openclaw:mainfrom
ly85206559:codex/discord-utf16-persona-names

Conversation

@ly85206559

@ly85206559 ly85206559 commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes Discord bound-thread persona names and webhook usernames being truncated with raw UTF-16 slicing. When a name limit cuts through an emoji or other surrogate pair, the previous .slice(0, 80) paths could leave a lone surrogate in the Discord display name.

Why This Change Was Made

Discord already uses truncateUtf16Safe for nearby thread-title and response-summary truncation. This applies the same helper to all three persona-name paths that still used raw slicing: bound-thread reply identity, outbound webhook persona username resolution, and thread-binding intro webhook persona names.

User Impact

Users with emoji or non-BMP characters in agent/thread labels get clean Discord persona names instead of malformed replacement characters or invalid UTF-16 edge cases when names are truncated.

Evidence

  • node scripts/run-vitest.mjs extensions/discord/src/monitor/thread-bindings.persona.test.ts extensions/discord/src/monitor/reply-delivery.test.ts extensions/discord/src/outbound-adapter.test.ts
    • Passed 3 files / 69 tests after adding the missing thread-binding persona coverage.
  • git diff --check
  • python .agents\skills\autoreview\scripts\autoreview --mode local --no-web-search
    • autoreview clean: no accepted/actionable findings reported

Still needed for ClawSweeper proof label:

  • Redacted live Discord proof showing a boundary emoji persona name renders cleanly after the fix.

@openclaw-barnacle openclaw-barnacle Bot added channel: discord Channel integration: discord size: S labels Jul 7, 2026
@ly85206559
ly85206559 marked this pull request as ready for review July 7, 2026 12:08
@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, 9:25 AM ET / 13:25 UTC.

Summary
The PR changes three Discord persona-name truncation paths from raw UTF-16 slicing to the shared UTF-16-safe helper and adds focused Vitest coverage.

PR surface: Source +4, Tests +133. Total +137 across 6 files.

Reproducibility: yes. from source inspection. Current main raw-slices the affected Discord persona names at 80 UTF-16 code units, and the PR adds boundary tests that exercise the surrogate-pair case.

Review metrics: 1 noteworthy metric.

  • Discord persona truncation paths: 3 changed, 0 known raw-slice persona paths remaining in this PR scope. The prior one-sided helper gap is now covered, so the remaining blocker is real behavior proof rather than source completeness.

Merge readiness
Overall: 🦪 silver shellfish
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 redacted live Discord proof showing a boundary emoji persona name after the fix; updating the PR body should trigger a fresh ClawSweeper review, or a maintainer can comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body explicitly says redacted live Discord proof is still needed; the contributor should add a screenshot, recording, terminal output, copied live output, or logs with private details redacted, then update the PR body for re-review.

Risk before merge

  • [P1] The contributor still has not provided redacted live Discord proof showing the visible webhook/persona username after the fix; unit tests and CI do not satisfy the real-behavior proof gate.
  • [P1] The related broad UTF-16 PR overlaps two Discord paths but omits thread-bindings.persona.ts, so maintainers should avoid replacing this PR with that branch unless the third persona path is preserved there.

Maintainer options:

  1. Decide the mitigation before merge
    Land this focused Discord fix, or a canonical consolidation that includes all three Discord persona-name paths, after redacted live Discord proof shows a boundary emoji persona name rendering cleanly.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] This open implementation PR has no narrow code defect left for repair automation; contributor-provided live Discord proof is the remaining merge gate.

Security
Cleared: The diff only changes Discord string truncation and related tests; it does not add dependency, credential, workflow, package, or supply-chain surface.

Review details

Best possible solution:

Land this focused Discord fix, or a canonical consolidation that includes all three Discord persona-name paths, after redacted live Discord proof shows a boundary emoji persona name rendering cleanly.

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

Yes from source inspection. Current main raw-slices the affected Discord persona names at 80 UTF-16 code units, and the PR adds boundary tests that exercise the surrogate-pair case.

Is this the best way to solve the issue?

Yes. Reusing the existing truncateUtf16Safe helper at all three Discord persona-name call sites is the narrow owner-boundary fix while preserving Discord's existing 80-character limit.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority Discord display bug with limited blast radius, not a setup blocker, security issue, data-loss path, or crash loop.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; 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 explicitly says redacted live Discord proof is still needed; the contributor should add a screenshot, recording, terminal output, copied live output, or logs with private details redacted, then update the PR body for re-review.
Evidence reviewed

PR surface:

Source +4, Tests +133. Total +137 across 6 files.

View PR surface stats
Area Files Added Removed Net
Source 3 7 3 +4
Tests 3 133 0 +133
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 6 140 3 +137

What I checked:

Likely related people:

  • steipete: Current blame for the raw persona helper lines points to Peter Steinberger's 6e792f08f88f4f9b8179059a20568c1eabfb3896, and history shows many recent Discord reply/outbound refactors in the affected files. (role: recent area contributor; confidence: medium; commits: 6e792f08f88f, 73e497f9be, c7347a492e; files: extensions/discord/src/monitor/reply-delivery.ts, extensions/discord/src/outbound-adapter.ts, extensions/discord/src/monitor/thread-bindings.persona.ts)
  • Onur Solmaz: History for the affected Discord reply/thread-binding area includes the thread-bound subagent and ACP thread-bound agent feature commits that made the persona surfaces relevant. (role: feature-history contributor; confidence: medium; commits: 8178ea472d, a7d56e3554, a7929abad8; files: extensions/discord/src/monitor/reply-delivery.ts, extensions/discord/src/monitor/thread-bindings.persona.ts)
  • scoootscooob: History shows scoootscooob moved the Discord channel implementation into extensions/, which is adjacent provenance for the affected plugin files and boundary. (role: migration contributor; confidence: low; commits: 5682ec37fa, 439c21e078, d9c285e930; files: extensions/discord/src/outbound-adapter.ts, extensions/discord/src/monitor/reply-delivery.ts)
  • vincentkoc: History shows Vincent Koc on adjacent Discord outbound/shared-interactive work and a recent release-stabilization touch in the affected file history. (role: adjacent contributor; confidence: low; commits: e085fa1a3f, c7d31bae8a, f889219955; files: extensions/discord/src/outbound-adapter.ts, extensions/discord/src/monitor/reply-delivery.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 (1 earlier review cycle)
  • reviewed 2026-07-07T12:41:31.875Z sha 0383908 :: needs real behavior proof before merge. :: [P2] Cover the remaining persona helper

@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 •
edited
Loading

Copy link
Copy Markdown
Contributor

Correction: #101685 / 176fee5d071d landed two sibling Discord persona boundaries, but it did not include thread-bindings.persona.ts; that remaining hunk was therefore not actually on main when this PR was closed.

The exact omitted fix has now landed via #103543 in b745862361e5, with an exact-output regression and 79 focused Discord tests. Credit remains due to @ly85206559 for the original three-path fix and @MoerAI for the focused current-main repair. Thank you both.

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

channel: discord Channel integration: discord P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S 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

@ly85206559 @steipete