Repository navigation
fix(mobile): prevent Hermes startup crash from Intl.Segmenter - #324
Conversation
…rash) (#11984) (cherry picked from commit 6f7aaff)
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dbalders/TritonAI-Harness/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesMonogram validation
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The startup fix is mergeable. Some uncommon monograms may still be accepted or rejected incorrectly. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change removes the mobile startup dependency without adding privileges or new access paths. The main remaining risk is Unicode validation compatibility across clients, rather than a demonstrated security vulnerability. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Cross-model reviewImplementation (PR footer): GPT-6.1 Sol via Codex autoreview scoped-clean: no accepted/actionable findings in the selected Git scope and priority Using one implementation on every runtime keeps validation consistent between server and mobile. The two-grapheme limit and the existing character pattern are otherwise unchanged. The new tests remove Some expectations depend on the library following Unicode 15.1+ segmentation rules (for example the Indic conjunct rule GB9c, which makes "क्ष" one grapheme). Recent The dependency is pinned to an exact version rather than using the workspace catalog. That is a style choice, not a defect. I found no actionable P0–P2 defects. Automated cross-model review on ChelsClaw, updated 2026-10-02T05:36:19+00:00. |
dbalders
left a comment
There was a problem hiding this comment.
TritonAI Review
No actionable findings in the inspected changes; see coverage limits.
Reviewed 21557ab694b92c1d235b727ee3b217b9bc154146 against efc676978d70cbeb2ed544f22c677261ba5961db.
Inspected 2 file(s) in 2 part(s); reused 1 unchanged review part(s).
Changes
packages/contracts/src/orchestration.mobile-runtime.test.ts: This part adds packages/contracts/src/orchestration.mobile-runtime.test.ts, a regression test for the Hermes startup crash. It stubs a Segmenter-less Intl via vi.stubGlobal, resets the module registry, dynamically imports the full contracts barrel (./index.ts, which re-exports orchestration.ts), and asserts that ProjectIconOverride decodes valid monograms ("T3", combining-mark and Devanagari cluster forms including a ZWJ sequence) and rejects three-cluster inputs, restoring globals and modules in afterEach.packages/contracts/src/orchestration.ts: This part replaces the module-scopeIntl.Segmenterinstance (which crashed on Hermes at app startup) in packages/contracts/src/orchestration.ts with a runtime-independent grapheme-cluster approximation (GRAPHEME_CONTINUATIONregex pluscountGraphemes), and rewires theProjectMonogramText≤2-grapheme filter to use it. The pattern check on line 573 is unchanged context. The other changed file, packages/contracts/src/orchestration.mobile-runtime.test.ts, is reviewed in a separate part.
Coverage limits
- packages/contracts/src/orchestration.ts: Some candidate findings were omitted because their evidence or location could not be verified.
- packages/contracts/src/orchestration.ts: The PR claims this is an exact cherry-pick of upstream #11984; the upstream patch content was not available, so whether the ZWJ and Hangul divergences originate upstream or were introduced downstream could not be verified. If they are upstream's exact code, the mergeability guidance weighs against diverging in this file; the correctness analysis above still applies.
- packages/contracts/src/orchestration.ts: The producer paths for
ProjectMonogramText(the server handler that accepts project icon updates and any client UI that constrains monogram entry) are not in the supplied context, so the P2 findings' exploitability via normal UI versus raw RPC could not be confirmed; the schema-level divergence from the previous Intl.Segmenter behavior stands regardless. This does not cover packages/contracts/src/orchestration.mobile-runtime.test.ts, whose changed lines are handled by a separate part.
CI snapshot
Combined commit status: pending
- status CodeRabbit: pending Review in progress
Check runs:
- Sync PR size label definitions: status=completed conclusion=skipped app=
- Label PR size: status=completed conclusion=success app=
- Mobile Native Static Analysis: status=completed conclusion=skipped app=
- Macroscope - Effect Service Conventions: status=completed conclusion=skipped app=
- Macroscope - Correctness Check: status=completed conclusion=skipped app=
- Prepare PR size config: status=completed conclusion=success app=
- Mobile Native Changes: status=completed conclusion=success app=
- Check: status=in_progress conclusion=none app=
- Test Server 1: status=completed conclusion=success app=
- Test Server 2: status=in_progress conclusion=none app=
- Release Smoke: status=completed conclusion=success app=
- Rust: status=completed conclusion=success app=
- Test Server 3: status=in_progress conclusion=none app=
- Test: status=in_progress conclusion=none app=
- Native fingerprint diff: status=completed conclusion=success app=
CI is reported separately. This code review does not claim to have run tests.
Comment /triton-review to request a fresh review. Findings are advisory; GitHub branch protection remains authoritative.
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 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.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
dbalders
left a comment
There was a problem hiding this comment.
TritonAI Review
No actionable findings in the inspected changes; see coverage limits.
Reviewed 3445a3b6fd04a090b5bcb8ae756b80ab6c89fdba against efc676978d70cbeb2ed544f22c677261ba5961db.
Inspected 4 file(s) in 4 part(s); reused 3 unchanged review part(s).
Changes
packages/contracts/package.json: This part adds a pinned runtime dependency,unicode-segmenter0.17.3, to@t3tools/contracts(the shared contracts package thatsrc/index.tsre-exports barrel-wide and thatpackages/contracts/src/orchestration.tswill use for grapheme counting in place ofIntl.Segmenter). No other lines in this file change; the entry is added underdependenciesalongsideeffect: catalog:.packages/contracts/src/orchestration.mobile-runtime.test.ts: This part adds a new contracts test file, packages/contracts/src/orchestration.mobile-runtime.test.ts, which stubs the global Intl object so Intl.Segmenter is undefined, resets the module registry, re-imports the contracts entry point, and exercises ProjectIconOverride decoding: rejecting three visible letters separated by ZWJ/ZWNJ joiners, accepting persisted international monograms (Indic conjunct, decomposed Hangul, Thai/Lao), and accepting/rejecting a matrix of two-grapheme and three-grapheme inputs. afterEach restores the stubbed globals and resets modules. The stubbing and dynamic-import ordering are correct (stub before resetModules before import), the chosen inputs are consistent with UAX #29 grapheme segmentation rules that the production schema in orchestration.ts relies on (countGraphemes from unicode-segmenter), and the test does not leak the stub across tests because afterEach unconditionally calls vi.unstubAllGlobals(). No correctness, concurrency, security, or data-loss defects were found in the changed lines of this part.packages/contracts/src/orchestration.ts: This part ofpackages/contracts/src/orchestration.tsremoves the module-scopenew Intl.Segmenter(...)construction (which crashed Hermes-based mobile clients at launch) and replaces the grapheme-count filter onProjectMonogramTextwithcountGraphemesfrom theunicode-segmenter/graphemesubpath, imported at module scope. A comment explains the Hermes motivation and the intent to keep segmentation identical across runtimes. The pattern, max-length, and surroundingProjectIconOverrideshapes are unchanged. Thepackage.json, lockfile, and the neworchestration.mobile-runtime.test.tsare handled by other diff parts.pnpm-lock.yaml: This lockfile part adds theunicode-segmenterdependency pinned at exactly 0.17.3 forpackages/contracts. All three required lockfile locations are updated consistently: the importer entry underpackages/contractsdependencies (specifier0.17.3, resolved version0.17.3), the package resolution entry with an integrity hash, and the snapshot entryunicode-segmenter@0.17.3: {}showing no transitive dependencies, which matches the PR description's claim that the package has zero dependencies. The remaining changed lines are context records (@effect/vitest,vite-plus,unified,unist-util-*, etc.) that are unchanged and only shown to anchor the new entries alphabetically. The entry format and placement match the pnpm lockfileVersion 9.0 layout used elsewhere in the file, and the exact-pin specifier matches the correspondingpackages/contracts/package.jsonchange reviewed in another part.
Coverage limits
- packages/contracts/package.json: I cannot verify the contents or layout of the
unicode-segmenterpackage at version 0.17.3 from the supplied context. The upstreamunicode-segmenterproject distributes WASM-backed segmentation; if 0.17.3 loads a.wasmasset at module scope, that would interact with (a) Hermes' limited/experimental WebAssembly support on the mobile runtime this PR exists to fix, (b) the CLI bundler, whose external/bundling policy in scripts/lib/cli-external-packages.ts is prefix-based and would treatunicode-segmenteras an ordinary bundled dependency, potentially breaking on an unhandlednew URL('*.wasm')asset import, and (c) any Edge/browser runtime that imports the contracts barrel viasrc/index.ts. The PR description states that a signed iOS build launched and that tests run withIntl.Segmenterdeleted, which is evidence against a Hermes-side failure, but Node tests would not surface a WASM-asset bundling problem in the CLI. Thepnpm-lock.yamlrecord - packages/contracts/src/orchestration.mobile-runtime.test.ts: This review covers only packages/contracts/src/orchestration.mobile-runtime.test.ts. The changed lines in packages/contracts/src/orchestration.ts (the ProjectMonogramText schema and unicode-segmenter import), packages/contracts/package.json (the new unicode-segmenter 0.17.3 dependency), and pnpm-lock.yaml (the corresponding lockfile entry, which is not present in the supplied lockfile excerpt) are handled by other review parts; their correctness and the exact grapheme-counting behavior of unicode-segmenter 0.17.3 against these test inputs could not be independently verified from the evidence supplied here.
- packages/contracts/src/orchestration.ts: Some candidate findings were omitted because their evidence or location could not be verified.
- packages/contracts/src/orchestration.ts: The implementation of the pinned dependency is not part of the repository or this diff, so the finding's central factual claim — that
unicode-segmenter@0.17.3instantiates WebAssembly at import time and is therefore Hermes-incompatible — could not be confirmed against the actual package code. Manual inspection of the installed package (or an execution test under real Hermes) is required to confirm or dismiss the P1 finding. Thepackage.json/lockfile pinning and the contents ofpackages/contracts/src/orchestration.mobile-runtime.test.tsare handled by other diff parts and were not relied on here. - pnpm-lock.yaml: The diff context does not include registry metadata, so the correctness of the
sha512-hKZwqBjJDmqNrq1...integrity hash forunicode-segmenter@0.17.3and the existence/contents of the published 0.17.3 tarball cannot be independently verified here; a frozen-lockfile install in CI would fail loudly if the hash or version is wrong, so this is a verification gap rather than evidence of a defect. Whetherunicode-segmenter/graphemeis a valid subpath export is reviewed with thepackages/contracts/src/orchestration.tspart.
CI snapshot
Combined commit status: success
- status CodeRabbit: success Review rate limited
Check runs:
- Mobile Native Static Analysis: status=completed conclusion=skipped app=
- Build macOS Apple Silicon preview: status=completed conclusion=skipped app=
- Sync PR size label definitions: status=completed conclusion=skipped app=
- Label PR size: status=completed conclusion=success app=
- Macroscope - Effect Service Conventions: status=completed conclusion=skipped app=
- Publish anonymous download: status=completed conclusion=skipped app=
- Release Smoke: status=completed conclusion=success app=
- Mobile Native Changes: status=completed conclusion=success app=
- Test Server 3: status=completed conclusion=success app=
- Rust: status=completed conclusion=success app=
- Test Server 2: status=completed conclusion=success app=
- Test Server 1: status=completed conclusion=success app=
- Test: status=in_progress conclusion=none app=
- Remove preview download: status=completed conclusion=skipped app=
- Check: status=in_progress conclusion=none app=
- Deploy web preview: status=completed conclusion=skipped app=
- Native fingerprint diff: status=completed conclusion=success app=
- Prepare PR size config: status=completed conclusion=success app=
- Macroscope - Correctness Check: status=completed conclusion=skipped app=
CI is reported separately. This code review does not claim to have run tests.
Comment /triton-review to request a fresh review. Findings are advisory; GitHub branch protection remains authoritative.
Summary
Harness Preview 1.2.0 (4) crashed before opening because shared monogram validation constructed
Intl.Segmenterduring module initialization, which is unavailable in the mobile Hermes runtime. Start from upstream #11984, then use the same Unicode grapheme counter on every runtime to preserve saved international monograms and prevent joiners from bypassing the two-grapheme limit.Why
Restore launch on TestFlight and locally signed iPhone builds while preserving the shared validation behavior.
Validation
vp check --fix: 0 errors; 781 existing warnings.vp run typecheck.vp test: 17,364 passed; 56 skipped. Focused contracts tests: 67 passed.Intl.Segmenterunavailable.3445a3b6fd; current-head cross-model and TritonAI reviews found no actionable findings. CodeRabbit was rate limited and covers the earlier revision.Risk and Recovery
The replacement avoids the unavailable runtime API and pins
unicode-segmenter0.17.3 with no transitive dependencies. Regression coverage exercises accepted and rejected inputs, including Indic conjuncts, decomposed Hangul, Thai/Lao marks, and joiner-separated letters. Reverting would restore the mobile startup crash.Release and Deployment Impact
A new mobile bundle is required to replace the crashing TestFlight version. This PR does not upload a build or trigger a nightly.
Upstream Alignment
The first commit is the exact patch from upstream commit
6f7aaffe26f0e6ae3d47b0ae512b322f890e0f43. Subsequent downstream commits add regression coverage and replace its approximate counter to address verified review findings while keeping validation consistent across runtimes.Checklist
Implementation: GPT-6.1 Sol via Codex