fix(runtime): truncate assistant summary on UTF-8 char boundary to prevent CJK panic#2167
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the text truncation logic in runtime_threads.rs to use a helper function truncate_with_ellipsis. The reviewer pointed out that the manual length check and conditional cloning are now redundant because the helper function already handles these internally, and suggested simplifying the code by calling the helper directly.
| let asst_summary = if assistant_text.len() > SUMMARY_LIMIT { | ||
| format!("{}...", &assistant_text[..SUMMARY_LIMIT.saturating_sub(3)]) | ||
| crate::utils::truncate_with_ellipsis(&assistant_text, SUMMARY_LIMIT, "...") | ||
| } else { | ||
| assistant_text.clone() | ||
| }; |
There was a problem hiding this comment.
The manual length check assistant_text.len() > SUMMARY_LIMIT and the conditional .clone() are redundant because truncate_with_ellipsis already performs this length check internally and returns a new String. We can simplify this block by directly calling truncate_with_ellipsis.
let asst_summary = crate::utils::truncate_with_ellipsis(&assistant_text, SUMMARY_LIMIT, "...");| let asst_summary = if assistant_text.len() > SUMMARY_LIMIT { | ||
| format!("{}...", &assistant_text[..SUMMARY_LIMIT.saturating_sub(3)]) | ||
| crate::utils::truncate_with_ellipsis(&assistant_text, SUMMARY_LIMIT, "...") | ||
| } else { | ||
| assistant_text.clone() | ||
| }; |
There was a problem hiding this comment.
The outer
if assistant_text.len() > SUMMARY_LIMIT guard is now redundant. truncate_with_ellipsis already contains an identical early-return (if s.len() <= max_len { return s.to_string(); }), so the branch never provides additional protection. The user_text path directly above (line 1414) calls the helper without a guard, making the two paths inconsistent.
| let asst_summary = if assistant_text.len() > SUMMARY_LIMIT { | |
| format!("{}...", &assistant_text[..SUMMARY_LIMIT.saturating_sub(3)]) | |
| crate::utils::truncate_with_ellipsis(&assistant_text, SUMMARY_LIMIT, "...") | |
| } else { | |
| assistant_text.clone() | |
| }; | |
| let asst_summary = crate::utils::truncate_with_ellipsis(&assistant_text, SUMMARY_LIMIT, "..."); |
Bug
When
assistant_textcontains CJK characters (3-byte UTF-8), the byte slice&assistant_text[..SUMMARY_LIMIT.saturating_sub(3)](byte 277) can land inthe middle of a multi-byte sequence, causing a panic:
Fix
Replace the raw byte-index slice with
truncate_with_ellipsis()which alreadyfinds the nearest safe char boundary via
char_indices().Change
1 line changed.
Files
Greptile Summary
This PR fixes a panic that occurred when
assistant_textcontained CJK (or any multi-byte UTF-8) characters by replacing a raw byte-index slice with the existingtruncate_with_ellipsishelper, which already walkschar_indices()to guarantee a safe boundary.runtime_threads.rs(line 1437) is correct:truncate_with_ellipsissafely handles all UTF-8 sequences by finding the last character start index that fits within the byte budget before appending the ellipsis.if assistant_text.len() > SUMMARY_LIMITguard (lines 1436–1440) is now redundant—truncate_with_ellipsisalready has an identical early-return—and is inconsistent with theuser_textpath on line 1414 that calls the helper directly.Confidence Score: 4/5
Safe to merge — the fix correctly eliminates the UTF-8 boundary panic and the only remaining note is a minor cleanup opportunity.
The one-line change replaces a demonstrably dangerous raw byte slice with a well-tested helper that walks char_indices(). The surrounding outer guard is now dead weight and inconsistent with the adjacent user_text path, but it does not introduce wrong behavior.
No files require special attention; the change is isolated to a single expression in runtime_threads.rs.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["assistant_text available?"] -->|yes| B["assistant_text.len() > SUMMARY_LIMIT?"] B -->|yes, OLD path| C["&assistant_text[..SUMMARY_LIMIT - 3]\n⚠️ panics on CJK char boundary"] B -->|yes, NEW path| D["truncate_with_ellipsis()\nwalks char_indices() → safe boundary"] B -->|no| E["assistant_text.clone()"] D --> F["asst_summary = truncated + '...'"] E --> F C --> G["💥 panic: byte index not a char boundary"] F --> H["save_item(TurnItemRecord { summary: asst_summary })"]Reviews (1): Last reviewed commit: "fix(runtime): truncate assistant summary..." | Re-trigger Greptile