fix: keep truncated UnwrapException message valid UTF-8#99
Conversation
UnwrapException::describe() truncates the value summary at a byte offset, so a long multibyte string (e.g. Japanese) gets cut mid character and the exception message becomes invalid UTF-8 — which makes json_encode() of the message fail. Currently RED. Claude-Session: https://claude.ai/code/session_017XTM7pxbWPVNLV639i5WgK
describe() truncated the value summary with a byte-based substr(), so a long multibyte error value (e.g. a Japanese string) was cut mid character and the exception message became invalid UTF-8. That makes json_encode() of the message return false — a secondary failure in logging pipelines exactly when something has already gone wrong. Use mb_strcut(), which respects the byte budget without splitting a character, and declare the resulting ext-mbstring dependency. Claude-Session: https://claude.ai/code/session_017XTM7pxbWPVNLV639i5WgK
Walkthrough
ChangesUTF-8安全な切り詰め処理
Estimated code review effort: 1 (Trivial) | ~5 minutes レビューコメント
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #99 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 65 65
===========================================
Files 4 4
Lines 130 130
===========================================
Hits 130 130
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the ext-mbstring extension dependency and updates UnwrapException.php to use mb_strcut instead of substr when truncating error messages, preventing malformed UTF-8 sequences. A new test is added to verify this behavior. The review feedback suggests improving the new test by adding a $this->fail() call inside the try block to ensure that the test fails if the expected UnwrapException is not thrown.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try { | ||
| $err->unwrap(); | ||
| } catch (UnwrapException $e) { |
There was a problem hiding this comment.
このテストでは、$err->unwrap() が例外をスローしなかった場合（バグなどにより正常終了してしまった場合）、catch ブロックがスキップされてテストが正常にパスしてしまいます（PHPUnitの設定によっては assertions が実行されず risky と判定されることはありますが、明示的な失敗にはなりません）。
例外が確実にスローされることを保証するために、try ブロックの最後に $this->fail() を追加することをお勧めします。
try {
$err->unwrap();
$this->fail('Expected UnwrapException was not thrown.');
} catch (UnwrapException $e) {![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/ErrTest.php`:
- Around line 150-152: The ErrTest::unwrap() test can silently pass if
$err->unwrap() does not throw, because the catch block is never reached and no
assertion runs. Update the try/catch around $err->unwrap() to explicitly fail
the test when no UnwrapException is thrown by adding a failure path immediately
after the unwrap call, so the test only passes when the exception is actually
raised and caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 838389c9-9247-490f-be28-ce3e3f5d1125
📒 Files selected for processing (3)
composer.jsonsrc/UnwrapException.phptests/ErrTest.php
| try { | ||
| $err->unwrap(); | ||
| } catch (UnwrapException $e) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
unwrap() が例外を投げなかった場合の silent pass を防止すべき
try ブロック内の $err->unwrap() が例外を投げなければ、catch ブロック内のアサーションが一切実行されず、テストが無条件で通過してしまいます。unwrap() の後に $this->fail() を追加して、例外が投げられなかった場合は確実に失敗させるべきです。
As per coding guidelines, t-wada 流 TDD を意識し、テストの信頼性を高めるべきです。
✅ 提案する修正
try {
$err->unwrap();
+ $this->fail('Expected UnwrapException was not thrown');
} catch (UnwrapException $e) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| $err->unwrap(); | |
| } catch (UnwrapException $e) { | |
| try { | |
| $err->unwrap(); | |
| $this->fail('Expected UnwrapException was not thrown'); | |
| } catch (UnwrapException $e) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/ErrTest.php` around lines 150 - 152, The ErrTest::unwrap() test can
silently pass if $err->unwrap() does not throw, because the catch block is never
reached and no assertion runs. Update the try/catch around $err->unwrap() to
explicitly fail the test when no UnwrapException is thrown by adding a failure
path immediately after the unwrap call, so the test only passes when the
exception is actually raised and caught.
Source: Path instructions
There was a problem hiding this comment.
Pull request overview
UnwrapException::describe() が長いマルチバイト文字列を切り詰める際に substr() によって UTF-8 を壊し、例外メッセージの json_encode() が失敗しうる実バグを修正する PR です。エラーハンドリング経路での二次障害（ログ JSON 化失敗）を防ぐ目的で、切り詰め処理を UTF-8 の文字境界を保つ方法へ変更しています。
Changes:
UnwrapException::describe()の切り詰めをsubstr()からmb_strcut(..., 'UTF-8')に変更し、不正 UTF-8 の生成を回避ext-mbstringをcomposer.jsonのrequireに追加- 長いマルチバイトエラー値で、切り詰め後メッセージが valid UTF-8 かつ
json_encode可能であることを検証するテストを追加
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/ErrTest.php | マルチバイト長文エラー値での切り詰め後メッセージが UTF-8 として正当であることを検証するテストを追加 |
| src/UnwrapException.php | 値要約の切り詰めを mb_strcut に変更し、文字境界を保って valid UTF-8 を維持 |
| composer.json | 実装変更に伴う ext-mbstring 依存をランタイム要件として明示 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| $err->unwrap(); | ||
| } catch (UnwrapException $e) { |
概要
コードレビュー（Opus 独立レビュー含む）で見つかった実バグの修正です。
UnwrapException::describe()の値要約の切り詰めがバイト単位のsubstr()を使っているため、マルチバイト文字を含む長い値では 120 バイト目で文字の途中を切って不正な UTF-8 を生成し、例外メッセージのjson_encode()が失敗します。再現（修正前）
エラーハンドリング経路（＝すでに何か失敗している最中）で、ログ基盤が例外メッセージを JSON 化しようとすると二次障害になります。ドックブロックがすべて日本語のプロジェクトであり、エラー値に日本語文字列が載るのは現実的なシナリオです。
修正
src/UnwrapException.php: 切り詰めをsubstr()→mb_strcut($summary, 0, 120, 'UTF-8')に変更。mb_strcutはバイト上限を超えない範囲で文字境界を保って切ります（バイト予算はそのまま維持）。composer.json: これにより生じるext-mbstring依存をrequireに宣言。検証
unwrap_withLongMultibyteError_keepsValidUtf8Message（tests/ErrTest.php）で、切り詰め後のメッセージがmb_check_encoding(..., 'UTF-8')を通りjson_encodeが成功することをピン留めbytes=176 valid_utf8=yes json=okTDD
truncated message must remain valid UTF-8failure）を確認してコミットmb_strcutで実装して GREENhttps://claude.ai/code/session_017XTM7pxbWPVNLV639i5WgK
Summary by CodeRabbit
Bug Fixes
Chores
Tests