fix: use byte slice in loop_guard to avoid UTF-8 panic on multibyte content - #389
Conversation
…ontent compute_outcome_hash() truncated the tool result string by slicing at a fixed byte offset (&result[..1000]), which panics when that offset falls inside a multi-byte character such as Chinese or other CJK text. The function only feeds the data to a SHA-256 hasher, so there is no requirement for valid UTF-8 boundaries. Switching to result.as_bytes() and slicing the byte slice avoids the panic entirely.
WalkthroughRefactored the outcome hash computation in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/loop_guard.rs`:
- Around line 451-452: Add a unit test that reproduces the previous panic by
passing a string whose byte length crosses RESULT_HASH_TRUNCATION in the middle
of a multibyte UTF-8 character to record_outcome/compute_outcome_hash and assert
it completes (no panic) and returns a consistent hash; specifically construct a
test string where the 1000th byte falls inside a multibyte character (e.g.,
repeat ASCII until just before RESULT_HASH_TRUNCATION-1 then append a multi-byte
Unicode rune) and call compute_outcome_hash (or record_outcome if it wraps it)
to ensure the function does not panic and returns an expected non-empty hash
value, adding the test to the loop_guard.rs unit tests so future regressions are
caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4655c4d2-fa16-4641-a486-6ab1777fdaf5
📒 Files selected for processing (1)
src/hooks/loop_guard.rs
| let result_bytes = result.as_bytes(); | ||
| hasher.update(&result_bytes[..result_bytes.len().min(RESULT_HASH_TRUNCATION)]); |
There was a problem hiding this comment.
Add a UTF-8 regression test for this truncation path.
The byte-slice fix looks right, but this bug was a panic on specific multibyte boundaries and there’s no test here locking that down. Please add a unit test that feeds record_outcome/compute_outcome_hash a string whose 1000th byte lands inside a multibyte character and asserts it no longer panics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/loop_guard.rs` around lines 451 - 452, Add a unit test that
reproduces the previous panic by passing a string whose byte length crosses
RESULT_HASH_TRUNCATION in the middle of a multibyte UTF-8 character to
record_outcome/compute_outcome_hash and assert it completes (no panic) and
returns a consistent hash; specifically construct a test string where the 1000th
byte falls inside a multibyte character (e.g., repeat ASCII until just before
RESULT_HASH_TRUNCATION-1 then append a multi-byte Unicode rune) and call
compute_outcome_hash (or record_outcome if it wraps it) to ensure the function
does not panic and returns an expected non-empty hash value, adding the test to
the loop_guard.rs unit tests so future regressions are caught.
| }; | ||
| hasher.update(truncated.as_bytes()); | ||
| let result_bytes = result.as_bytes(); | ||
| hasher.update(&result_bytes[..result_bytes.len().min(RESULT_HASH_TRUNCATION)]); |
There was a problem hiding this comment.
Tiny readability nit: since RESULT_HASH_TRUNCATION is a byte cap (consistent with the old len() behavior), consider naming the computed length and clarifying it’s bytes.
| hasher.update(&result_bytes[..result_bytes.len().min(RESULT_HASH_TRUNCATION)]); | |
| let result_bytes = result.as_bytes(); | |
| let truncated_len = result_bytes.len().min(RESULT_HASH_TRUNCATION); // bytes | |
| hasher.update(&result_bytes[..truncated_len]); |
Problem
compute_outcome_hash()insrc/hooks/loop_guard.rstruncates the tool result string with a fixed byte offset:This panics when byte offset 1000 falls inside a multi-byte character. For example, Chinese/CJK characters are 3 bytes each in UTF-8, so any result string containing CJK text is likely to trigger this:
This makes any worker task that reads or produces CJK (or other multibyte) content fail unconditionally.
Fix
compute_outcome_hash()feeds the data directly to a SHA-256 hasher, so there is no requirement for valid UTF-8 boundaries. Slicing the byte representation instead avoids the panic entirely:The hash output is identical for ASCII content and correct for all inputs.
Note
Fixes a critical bug where worker tasks panic when processing multibyte UTF-8 content. The fix replaces string slicing with byte slicing in
compute_outcome_hash(), eliminating the requirement for valid UTF-8 boundaries while maintaining correct hash values.Written by Tembo for commit 3e56129.