harness: size token estimation over all content blocks, not just text#35
Conversation
Images, structured JSON (large tool results), and model reasoning occupy real context, but char_len counts only Text blocks — so a multimodal or tool-heavy transcript under-counts and should_summarize / MaxTokens trim silently never fire. Add Message::estimated_char_weight over all content blocks and route the token estimator through it; the visible-text accessors (as_text/text/char_len) are left unchanged so reasoning still never leaks into visible output.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ChangesWeighted Token Estimation
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)Not applicable — this change is a localized heuristic/calculation update without multi-component control flow. Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/harness/message/mod.rs (1)
68-90: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRepeated
.to_string()serialization on hot path.
ContentBlock::Json/ProviderExtensionweight requires a full serialization of the value on every call. Since this feedsmessage_token_estimate, which is invoked per-message on everyshould_summarize/trim_messagespass over the transcript, large tool-result JSON payloads will be re-serialized repeatedly as the conversation grows. This is an intentional accuracy tradeoff per the PR's design, but worth keeping in mind if this becomes a hot path bottleneck for large transcripts with big JSON blocks.Consider caching the computed weight alongside the message (e.g., in a wrapper struct) if profiling shows this is a bottleneck.
🤖 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 `@src/harness/message/mod.rs` around lines 68 - 90, The hot path in estimated_char_weight on ContentBlock::Json and ContentBlock::ProviderExtension repeatedly calls to_string(), which can re-serialize large payloads during message_token_estimate/should_summarize/trim_messages passes. Keep the existing accuracy behavior, but avoid repeated serialization by caching the computed weight with the message or wrapper type used by the transcript, and have estimated_char_weight reuse that cached value when available.
🤖 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.
Nitpick comments:
In `@src/harness/message/mod.rs`:
- Around line 68-90: The hot path in estimated_char_weight on ContentBlock::Json
and ContentBlock::ProviderExtension repeatedly calls to_string(), which can
re-serialize large payloads during
message_token_estimate/should_summarize/trim_messages passes. Keep the existing
accuracy behavior, but avoid repeated serialization by caching the computed
weight with the message or wrapper type used by the transcript, and have
estimated_char_weight reuse that cached value when available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f120c9dc-5ca1-421c-baca-a744b00321ff
📒 Files selected for processing (3)
src/harness/message/mod.rssrc/harness/message/test.rssrc/harness/summarization/mod.rs
collapsible_if in StreamAccumulator::push_tool_chunk and a duplicated #[cfg(test)] on the stream test module both fail CI's cargo clippy --all-targets -- -D warnings under rust 1.96. Behavior-neutral.
|
CI guardian: the |
Summary
Token estimation for context-window gating counted only
ContentBlock::Text, so a transcript dominated by images, large tool-result JSON, or model reasoning under-counted by orders of magnitude —SummarizationPolicy::should_summarizeand theMaxTokenstrim would silently never fire even as the real window overflowed.Change
ContentBlock::estimated_char_weight()/Message::estimated_char_weight()that account for all blocks: text, JSON (serialized length), reasoning (Thinking/RedactedThinking), provider extensions, and a flat per-image weight (IMAGE_CHAR_WEIGHT≈ 1024 tokens; images tokenize to a roughly fixed count regardless of encoded byte length).message_token_estimate(summarization) through it.as_text/text/char_len) are unchanged, so reasoning still never leaks into visible assistant output.Tests
estimated_char_weight_counts_non_text_blocks,estimated_char_weight_costs_images_that_carry_no_visible_text.cargo fmt --check,cargo clippy --lib --tests -- -D warnings, andcargo test --liball pass.Closes tinyhumansai/openhuman#4633
Summary by CodeRabbit
New Features
Bug Fixes
Tests