Skip to content

fix(line): truncate template title/altText on grapheme boundaries, not raw UTF-16#96395

Closed
ly-wang19 wants to merge 1 commit into
openclaw:mainfrom
ly-wang19:fix/line-template-surrogate-safe
Closed

fix(line): truncate template title/altText on grapheme boundaries, not raw UTF-16#96395
ly-wang19 wants to merge 1 commit into
openclaw:mainfrom
ly-wang19:fix/line-template-surrogate-safe

Conversation

@ly-wang19

@ly-wang19 ly-wang19 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

createConfirmTemplate / createButtonTemplate / createTemplateCarousel / createCarouselColumn / createImageCarousel (extensions/line/src/template-messages.ts) truncate the title and altText fields with a raw UTF-16 slice:

title: title.slice(0, 40),                 // LINE limit
altText: altText?.slice(0, 400) ?? ...,

When an emoji / astral char straddles the limit (e.g. a 40-char button title ending in an emoji), .slice keeps only the leading high surrogate, producing a lone \uD83D that LINE renders as � or rejects:

createButtonTemplate("x"×39 + "😀", ...).template.title
  before: "x"×39 + "\uD83D"   ❌ lone high surrogate (length 40)
  after : "x"×39              ✅ emoji dropped whole (length 39)

These builders are exported and feed live LINE outbound payloads (buildTemplateMessageFromPayload → deliverLineAutoReply). The text body already uses the grapheme-safe truncateTemplateText; only title/altText (and the confirm-template text) were still slicing raw.

Fix

Route every title/altText/confirm-text truncation through the file's existing grapheme-safe truncateTemplateText (plus a small truncateOptionalTemplateText wrapper for the optional fields). No new import; this just makes the whole file consistent with how the text body is already handled. truncateTemplateText(s, n) is byte-identical to s.slice(0, n) for all-BMP input, so existing behavior is preserved.

Evidence

Added a regression test on createButtonTemplate; verified the grapheme-safe truncation:

title "x"×39 + "😀"  limit 40  : before slice -> 40 chars, lone surrogate=true ; after -> "x"×39, lone=false ✅
title "My Title"     limit 40  : unchanged ✅
text  "a"×239 + "😀" limit 240 : after -> "a"×239, lone=false ✅
"hello world"        limit 5   : after "hello" == slice(0,5) ✅ (ASCII unchanged)

The existing truncates title to 40 characters test (all-ASCII) stays green; only the straddling-emoji case changes, and no fixed output contains a lone surrogate. (The array .slice(0, N) calls that cap action/column counts are unrelated and untouched.)

@ly-wang19 @claude
fix(line): truncate template title/altText on grapheme boundaries, no…
af350de 
…t raw UTF-16

createConfirmTemplate/createButtonTemplate/createTemplateCarousel/createCarouselColumn/
createImageCarousel truncated title and altText with a raw `.slice(0, N)`, so an
emoji straddling a LINE field limit (e.g. a 40-char button title) was cut in half,
leaving a lone high surrogate that LINE renders as the replacement char or rejects.
Route those fields through the file's existing grapheme-safe truncateTemplateText
(already used for the text body) via a small truncateOptionalTemplateText wrapper.
Byte-identical for all-BMP input; only straddling-emoji truncation changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@openclaw-barnacle openclaw-barnacle Bot added channel: line Channel integration: line size: XS labels Jun 24, 2026
@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 8:09 AM ET / 12:09 UTC.

Summary
This PR changes LINE template title, confirm text, and altText truncation to reuse the existing grapheme-aware template text helper and adds a button-title regression test.

PR surface: Source +6, Tests +10. Total +16 across 2 files.

Reproducibility: yes. from source inspection and a focused Node check: current main can split an emoji surrogate pair when raw .slice lands between UTF-16 halves in a template title or altText. I did not exercise a live LINE send path.

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 after-fix real behavior proof, such as terminal output, copied live output, logs, a linked artifact, or a screenshot/video that shows the LINE/template payload path without private details.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body provides regression-test style evidence but no redacted terminal/live output, logs, linked artifact, or LINE send-path proof; updating the PR body with such proof should trigger a fresh ClawSweeper review, or a maintainer can request @clawsweeper re-review. 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 PR still needs redacted real behavior proof from a LINE/template send path before merge; unit-style evidence and CI are not enough for the external-contributor proof gate.
  • [P1] Adjacent LINE flex-message altText paths still use raw .slice(0, 400) and are outside this template-focused PR; maintainers may want a separate follow-up, but that is not a defect introduced here.

Maintainer options:

  1. Decide the mitigation before merge
    Merge this focused LINE template fix after real behavior proof or maintainer override, and track any broader LINE flex altText truncation cleanup separately if maintainers want full channel-wide consistency.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The remaining blocker is contributor-supplied real behavior proof or maintainer override, not an automated code repair.

Security
Cleared: The diff only changes LINE template-building TypeScript and a focused Vitest test; it does not touch dependencies, lockfiles, workflows, secrets, install scripts, or publishing metadata.

Review details

Best possible solution:

Merge this focused LINE template fix after real behavior proof or maintainer override, and track any broader LINE flex altText truncation cleanup separately if maintainers want full channel-wide consistency.

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

Yes from source inspection and a focused Node check: current main can split an emoji surrogate pair when raw .slice lands between UTF-16 halves in a template title or altText. I did not exercise a live LINE send path.

Is this the best way to solve the issue?

Yes for the template-message surface: the PR reuses the existing local grapheme-aware helper already used for template body text, keeps the extension boundary local, and avoids new dependencies. It is intentionally narrower than a channel-wide LINE altText cleanup.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a normal LINE outbound template payload correctness fix with limited blast radius and no evidence of data loss, security, or core runtime outage.
  • 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 provides regression-test style evidence but no redacted terminal/live output, logs, linked artifact, or LINE send-path proof; updating the PR body with such proof should trigger a fresh ClawSweeper review, or a maintainer can request @clawsweeper re-review. 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:

  • P2: This is a normal LINE outbound template payload correctness fix with limited blast radius and no evidence of data loss, security, or core runtime outage.
  • 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 provides regression-test style evidence but no redacted terminal/live output, logs, linked artifact, or LINE send-path proof; updating the PR body with such proof should trigger a fresh ClawSweeper review, or a maintainer can request @clawsweeper re-review. 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 +6, Tests +10. Total +16 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 13 7 +6
Tests 1 10 0 +10
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 23 7 +16

What I checked:

  • Repository policy read: Root and scoped extension policies were read; the extension policy keeps bundled plugin code within its local boundary, which this PR does. (extensions/AGENTS.md:1, 2a484a3ff10d)
  • Current main bug path: Current main already uses truncateTemplateText with Intl.Segmenter, but template title and altText fields still use raw .slice(0, N) on UTF-16 code units. (extensions/line/src/template-messages.ts:46, 2a484a3ff10d)
  • PR implementation: The PR head routes confirm text, button title, carousel title, and template altText truncation through truncateTemplateText or the optional wrapper. (extensions/line/src/template-messages.ts:68, af350dedcd7f)
  • Regression coverage: The PR adds a focused test proving a 39-character prefix plus emoji button title drops the emoji instead of keeping a lone high surrogate. (extensions/line/src/message-cards.test.ts:47, af350dedcd7f)
  • Runtime caller path: LINE auto-reply delivery builds template messages from lineData.templateMessage via buildTemplateMessageFromPayload, so the helper feeds outbound LINE payloads. (extensions/line/src/auto-reply-delivery.ts:112, 2a484a3ff10d)
  • Focused source reproduction: A read-only Node check showed ('x'.repeat(39) + '😀').slice(0, 40) ends with lone high surrogate 0xd83d, while the Segmenter loop drops the emoji whole.

Likely related people:

  • vincentkoc: git blame and git show tie the current template builder, raw title/altText slices, and existing grapheme helper to commit d42b86421969693e07dd8b97ed9fc9182c3bf005. (role: introduced current LINE template builder; confidence: high; commits: d42b86421969; files: extensions/line/src/template-messages.ts, extensions/line/src/message-cards.test.ts)
  • ly-wang19: Beyond this PR, the author has a merged adjacent Slack surrogate-truncation fix, so they have recent context on this Unicode truncation class, though not current-main LINE ownership. (role: adjacent Unicode truncation contributor; confidence: medium; commits: 1069c60e1e25; files: extensions/slack/src/truncate.ts, extensions/slack/src/truncate.test.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 Jun 24, 2026
@ly-wang19

ly-wang19 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Closing to stay under the repo's active-PR limit and keep my queue focused on changes I can fully prove headlessly. This channel fix needs live-channel proof I can't generate in my current environment; the code change itself stands and I'll resubmit with live proof when I can run the channel.

@ly-wang19 ly-wang19 closed this Jun 25, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 28, 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: line Channel integration: line 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.

1 participant

@ly-wang19