harness: opt-in token-budget gate for MicrocompactMiddleware (cache-prefix stability)#46
Conversation
…ache-prefix stability MicrocompactMiddleware blanks older tool-result bodies (keeping the newest keep_recent verbatim) purely on COUNT: it fires whenever tool_idxs.len() > keep_recent. Because 'keep newest N' is a moving boundary, once a run has more than keep_recent tool results it rewrites one more tool result full->placeholder on essentially every model call. Blanking a body that was sent verbatim on an earlier iteration mutates an already-transmitted prefix position, invalidating the provider KV-cache from there on — every iteration — even when the whole transcript still fits the model context window. The trimming then churns the cache to reclaim tokens the model had room for, which can be a net cost loss (observed as heavy input-token bloat with only ~38% cached on tool/delegated turns). Add an opt-in token-budget gate: with_token_budget(budget) skips blanking while the (pre-blank, monotonically growing) transcript estimate stays within budget, so requests remain append-only and fully cache-eligible below it; compaction only kicks in once the transcript would exceed the window — the only time the cache-invalidation cost of blanking pays for itself. The gate reads the un-blanked estimate so it can never oscillate between blanked and full. Default None preserves the existing count-only behaviour byte-for-byte; budget == 0 disables the gate explicitly. Tests cover the no-op-below-budget path (the fix), the still-blanks-over-budget path, and the zero-disables-gate opt-out. Closes tinyhumansai/openhuman#4755
📝 WalkthroughWalkthroughMicrocompactMiddleware gains an optional token-budget configuration. ChangesMicrocompaction token-budget gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
serde_json::to_writer needs W: io::Write, which sha2::Sha256 does not implement — broke 'cargo clippy --all-targets -- -D warnings' (E0277). Serialize to a Vec and feed the bytes via Digest::update instead.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/harness/middleware/library/context.rs (1)
298-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the duplicated token-estimate branching.
The gate is correct (verified
total_message_tokensis computed at most once per call across allemit_events/token_budgetcombinations), but computingfrom_tokensand then re-derivingtokensvia the sameif self.emit_eventsconditional is easy to desynchronize in a future edit. Consider computing the estimate once, gated by whether either consumer needs it.♻️ Proposed simplification
- let from_tokens = if self.emit_events { - total_message_tokens(&request.messages) - } else { - 0 - }; - if let Some(budget) = self.token_budget { - let tokens = if self.emit_events { - from_tokens - } else { - total_message_tokens(&request.messages) - }; - if tokens <= budget { - return Ok(()); - } - } + let needs_estimate = self.emit_events || self.token_budget.is_some(); + let from_tokens = if needs_estimate { + total_message_tokens(&request.messages) + } else { + 0 + }; + if let Some(budget) = self.token_budget { + if from_tokens <= budget { + return Ok(()); + } + }🤖 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/middleware/library/context.rs` around lines 298 - 322, Compute total_message_tokens once in a local value when either emit_events is enabled or token_budget is present, then reuse that value for both from_tokens and the budget check. Remove the duplicated conditional branching while preserving the existing early return behavior and the zero-value path when neither consumer needs the estimate.
🤖 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/middleware/library/context.rs`:
- Around line 298-322: Compute total_message_tokens once in a local value when
either emit_events is enabled or token_budget is present, then reuse that value
for both from_tokens and the budget check. Remove the duplicated conditional
branching while preserving the existing early return behavior and the zero-value
path when neither consumer needs the estimate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81656b5c-b7aa-4d4e-b21e-eb2a19154525
📒 Files selected for processing (3)
src/harness/middleware/library/context.rssrc/harness/middleware/test.rssrc/harness/middleware/types.rs
Problem
MicrocompactMiddlewareblanks older tool-result bodies (keeping the newestkeep_recentverbatim) to bound a long, tool-heavy transcript. But it fires purely on count:Because "keep the newest
keep_recent" is a moving boundary, once a run has more thankeep_recenttool results the middleware rewrites one more tool resultfull → placeholderon essentially every model call. Blanking a body that was sent verbatim on an earlier iteration mutates an already-transmitted prefix position, which invalidates the provider KV-cache from that point onward — on every iteration — even when the whole transcript still fits the model's context window.The result: the trimming churns the prompt cache to reclaim tokens the model still had room for, which is frequently a net cost loss. Downstream (tinyhumansai/openhuman#4755) this shows up as large input-token bloat on tool/delegated turns with only ~38% of input cached.
Fix (minimal, opt-in, backward-compatible)
Add a token-budget gate to
MicrocompactMiddleware:token_budget: Option<u64>(defaultNone);.with_token_budget(budget)(0disables) + atoken_budget()getter;before_model, when a budget is set and the pre-blank transcript estimate is<= budget, return early without blanking.Below the budget the request stays append-only and fully cache-eligible; compaction only kicks in once the transcript would exceed the window — the only time the cache-invalidation cost of blanking an already-sent body actually pays for itself. The gate reads the un-blanked estimate (which grows monotonically as the run appends), so it can never oscillate between blanked and full.
token_budget = None(the default) preserves the existing count-only behaviour byte-for-byte — every existing test is unchanged.Tests
microcompact_with_token_budget_is_a_noop_below_budget— >keep_recenttool results but under budget → nothing blanked (the fix; contrastmicrocompact_clears_older_tool_bodies_and_keeps_recent).microcompact_with_token_budget_blanks_once_over_budget— still reclaims when genuinely over budget.microcompact_with_token_budget_zero_disables_the_gate—0opt-out reverts to legacy blanking.Activation in openhuman (follow-up on the vendored bump)
This lands the harness capability; it is inert until the host opts in. In
openhuman/src/openhuman/tinyagents/middleware.rs(whereMicrocompactMiddleware::new(...)is constructed, ~L340), add:context_windowis already threaded into the turn seam, so a run that fits the window is never compacted (append-only, cacheable) while long runs still compact to stay under it.Closes tinyhumansai/openhuman#4755
Summary by CodeRabbit
New Features
Bug Fixes