Skip to content

fix(shared): truncate no longer splits surrogate pairs - #24611

Open
juliusmarminge wants to merge 3 commits into
mainfrom
takeover/pr-13413
Open

juliusmarminge wants to merge 3 commits into
mainfrom
takeover/pr-13413

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

Thread titles could end in a replacement glyph when their UTF-16 length limit fell between the two halves of an emoji. The shared truncate helper now backs off one code unit at that boundary. ASCII/BMP lengths, trimming and the three-dot suffix are preserved.

Takeover

Original fix by @ValeraZSD in #13413.
Rebuilt on current main as one focused replacement because the submission lives on a contributor fork. The forward grapheme scanner and cluster tests were replaced with a constant-time surrogate-boundary check and three focused tests, matching the requested surrogate-pair scope without changing combining-mark truncation.

Evidence

Fixture matrix · Evidence index · Proof audit pass 2: web rows SUFFICIENT. Native Android rows (M-Android-L/D: native T50 title and the 200-unit work-log labels) are not captured; see Verification log.

Fixture titles pad with U+200B so the cut lands on the emoji while the visible title stays short: T50 = "Text " + "\u200b".repeat(44) + "😀 tail", P50 plan heading "Plan " + "\u200b".repeat(34) + "😀 tail", controls E50 (pair fits exactly) and SHORT.

Environment for all W scenes: real base/head apps from separate checkouts and fresh isolated state; Linux fs-38, Chromium headless-shell 156.0.8078.70 via agent-browser, no device emulation, DPR 1, font scale 1, CSS viewport = PNG pixels, reduced motion, Date frozen at 2026-10-27T15:00:00Z. Fonts/network settled; two pixel-identical frames ≥500 ms apart; all originals/diffs visually reviewed.

Seed: actual composer T50/E50/SHORT sends; actual proposed_plan → Implement in a new thread for P50. Deliberately unavailable custom text-generation model preserves fallback seeds after regeneration fails. Isolated projection metadata equalizes times/branch/settled flags only. seed-recipe.md · provenance.json.

W-1440-L — 1440×1000, light; T50, E50, SHORT, P50

Before
843f4e3
After
43cb239
Diff0 ×8
843f4e3 → 43cb239
DiffT >16/255
843f4e3 → 43cb239
W-1440-L-base.png W-1440-L-head.png W-1440-L-Diff0.png W-1440-L-DiffT.png

W-1440-D — 1440×1000, dark; T50, E50, SHORT, P50

Before
843f4e3
After
43cb239
Diff0 ×8
843f4e3 → 43cb239
DiffT >16/255
843f4e3 → 43cb239
W-1440-D-base.png W-1440-D-head.png W-1440-D-Diff0.png W-1440-D-DiffT.png

W-390-L — 390×844, light; T50, E50, SHORT, P50

Before
843f4e3
After
43cb239
Diff0 ×8
843f4e3 → 43cb239
DiffT >16/255
843f4e3 → 43cb239
W-390-L-base.png W-390-L-head.png W-390-L-Diff0.png W-390-L-DiffT.png

W-390-D — 390×844, dark; T50, E50, SHORT, P50

Before
843f4e3
After
43cb239
Diff0 ×8
843f4e3 → 43cb239
DiffT >16/255
843f4e3 → 43cb239
W-390-D-base.png W-390-D-head.png W-390-D-Diff0.png W-390-D-DiffT.png

What else changed:

  • Intended: T50/P50 replacement glyph disappears and neighboring dots shift; P50 breadcrumb changes at 1440. Base JSON retains U+D83D, SQLite title TEXT stores U+FFFD and Chrome displays its replacement glyph. Head omits the split pair.
  • Nothing else changed in the four accepted web pairs at zero threshold. E50/SHORT, ages, provider reply, plan body, hover/focus and chrome match. Diff0 = absolute RGB difference ×8; DiffT = channels >16/255, unmasked full surfaces. No interaction/timing claim or recording.
  • F/L and truncate-reaching V variants share the matrix’s caller-probe/source provenance; file/context sends were not observed here. LIVE and OUT remain source-excluded. No iOS/Electron-native proof is claimed.

Drivers and seeds: capture-real.py · seed-web.py (reproduction recipe; UI seed was executed manually) · normalize.py · diff.py · audit-bundle.tar.gz (scripts, SQL, settling transcripts and redacted logs).

Verification log
  • Tests: original PR 2 tests → 3 tests in packages/shared/src/String.test.ts. Merge-base helper: 1 failure, 2 passes; head: all 3 pass. Exact-pair and ASCII/BMP cases guard compatibility.
  • Review round 1 (earlier round): independent GPT-6.1-Sol and Claude Opus 5.5 both returned MERGE, no findings. Review and test log.
  • Evidence round 2 (no code changes): proof audit pass 1 derived six base/head still pairs from seven direct truncate calls (matrix, probe). Four web pairs were captured from real base/head apps. Pass 2 inspected all 8 originals and 8 diffs and rebuilt every diff byte-identically: W-1440-L/D and W-390-L/D are SUFFICIENT; M-Android-L/D are missing.
  • Unverified: native Android (M-Android-L/D). The pool's emulators are reachable, but this head predates the cached mobile:install client, and a local Android build took the VM below its disk floor. The native rows reuse the same helper through apps/mobile/src/lib/threadActivity.ts (200 units) and deriveThreadTitleSeed (50 units). Unexecuted native seed.
  • Earlier-round caller evidence (1440 light, real Codex sends, server title transcript) remains at caller evidence index.
  • Scope: valid surrogate pairs only; existing lone surrogates are not repaired, and grapheme clusters are not preserved. Server CLI titles, Git toasts, watch snippets and restart notes use separate truncators and are not changed here.
  • CI on 43cb23920c: all reported checks passed, including the 8 required ones.
  • Model/harness: tests and implementation by Claude Opus 5.5 through T3 Code; reviews by Claude Opus 5.5 and GPT-6.1-Sol; proof audits and captures by GPT-6.1-Sol (Codex harness) in T3 Code; lead Claude Opus 5.5 through T3 Code.
Original description by @ValeraZSD

Thread titles could end in � when the truncation boundary split an emoji's UTF-16 surrogate pair. The shared helper now selects a safe boundary before adding the ellipsis, preserving joined emoji, modifiers, combining marks, and paired flag indicators.

The implementation uses a small forward code-point scan and avoids Intl.Segmenter, which shared mobile code cannot use under the repository's Hermes constraint. Ordinary short-text trimming remains unchanged.

Two focused tests replace six overlapping cases. They fail with the original helper, and their cluster assertions also fail with a surrogate-only fix. The helper and thread-title suites pass all 7 tests; targeted lint, formatting, and shared-package typecheck pass. After merging current main, the same 7 tests and shared-package typecheck pass at ca8b37b. The merge leaves both PR files unchanged; fresh independent Opus and Sol reviews found no actionable defects. No live client or Hermes verification was performed during this takeover.

Original fix by @ValeraZSD in #13413. Maintainer takeover by Claude Opus 5.5 and Claude Fable 5.1 through T3 Code, independently reviewed by Claude Opus 5.5 and GPT-6.1-Sol through the Codex harness.

@github-actions github-actions Bot added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Oct 24, 2026
@github-actions github-actions Bot added the size:S 10-29 changed lines (additions + deletions). label Oct 24, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 43cb239

Macroscope's review found this PR approvable — This narrowly scoped shared-helper fix prevents truncation from producing lone surrogate characters while preserving existing trimming, length, and suffix behavior. It includes focused regression tests and introduces no schema, configuration, or deployment changes.

You can add or adjust custom eligibility rules. Learn more.

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB +7 B (+0.1%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB +7 B (+0.6%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB 0 B (0.0%) 29.3 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: 843f4e3 · PR result: 43cb239 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 843f4e3 and 43cb239.

Files selected for processing: 2
  • packages/shared/src/String.test.ts
  • packages/shared/src/String.ts

Limit details: You’ve used all 10 included reviews currently available.

Walkthrough

truncate now avoids ending its output between the UTF-16 code units of a surrogate pair. Tests cover that behavior, other truncation lengths, and input that fits the limit.

Changes: Unicode-safe string truncation

Layer / File(s) Summary
Truncation boundary and tests
packages/shared/src/String.ts, packages/shared/src/String.test.ts
truncate removes the final high surrogate when the slice would split a surrogate pair before appending .... Tests cover surrogate-pair boundaries, ASCII and BMP strings, and input that fits the limit.

Priority: Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Possibly related PRs

Suggested reviewers: macroscopeapp

Merge Risk: Minimal · up to 43cb2

The truncation change is mergeable after normal checks; no actionable risk remains in the supplied evidence.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: preventing truncate from splitting surrogate pairs. It also follows the repository's conventional commit format.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description explains the problem, implementation, scope, verification results, limitations, and takeover context. It does not use the exact template headings, but it provides the required informat…
Full details: Description check

Explanation

The description explains the problem, implementation, scope, verification results, limitations, and takeover context. It does not use the exact template headings, but it provides the required information and links the related issue or prior pull request.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge juliusmarminge added bugfix Fix PR opened by an issue lead; enters the PR loop pr:gate-failed A land gate failed pr:in-progress A lead is working on this PR and removed pr:gate-failed A land gate failed pr:in-progress A lead is working on this PR labels Oct 25, 2026

Copy link
Copy Markdown
Member Author

Taking over this one

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fix PR opened by an issue lead; enters the PR loop pr:needs-julius Needs Julius: route gate, unverifiable surface, or blocked size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant