Add assembly cap guardrails - #4
Conversation
|
Quick heads up: I also have a Hermes side PR here: NousResearch/hermes-agent#8416 That PR is only about making Hermes work cleanly with external context engine plugins. It does not bundle or vendor hermes-lcm into Hermes. I am linking it here because the guardrail knobs in this PR line up with the compatibility path on the Hermes side, especially around assembly and headroom behavior. |
stephenschoettler
left a comment
There was a problem hiding this comment.
Review: Request Changes
Good design with the two-knob approach (max_assembly_tokens + reserve_tokens_floor), but there's a correctness bug in tail trimming.
Bug — non-contiguous tail messages (engine.py ~line 476)
for msg in reversed(tail_messages):
msg_tokens = count_message_tokens(msg)
if used + tail_token_total + msg_tokens > assembly_cap:
continue # ← keeps scanning older messages
kept_tail_reversed.append(msg)
tail_token_total += msg_tokenscontinue skips messages that don't fit but keeps iterating. If a large message doesn't fit but a smaller older message does, the older one is kept while the newer one is dropped — producing a non-contiguous conversation window with gaps. This should be break to maintain a contiguous recent-tail.
Minor notes
-
tail_budgetnaming — This variable is the budget for summaries, not tail messages. Consider renaming tosummary_budget. -
Silent guardrail disable — If
reserve_tokens_floor >= context_length, the reserve cap computes to ≤ 0, gets filtered out, and the guardrail silently does nothing. A log warning for this misconfiguration would be helpful. -
Summary part selection — Same
continue-vs-breakconcern applies to the summary selection loop (~line 508). Skipping a large summary but keeping a later one could produce incoherent context ordering. -
Test gap — Tests use uniform message sizes. Adding a test with varied sizes would expose the non-contiguous gap bug.
-
Drive-by fix — The
l2_budget_ratioandl3_truncate_tokensenv-var bindings added tofrom_envlook correct but are unrelated to this PR — worth noting in the description.
The core design is sound — just needs the continue → break fix and a test for varied message sizes.
|
@stephenschoettler I pushed a follow-up for your review on this PR. I fixed the contiguity issue you called out by switching the guardrail selection loops from skip-and-continue to stop-at-first-overflow behavior:
I also renamed Added regression coverage for:
Validation:
Latest commit on the PR branch: |
|
@stephenschoettler I pushed one more follow-up on top of The contiguity fix in that commit was still correct, but it exposed another case:
This follow-up keeps the newest tail message even when it alone exceeds the cap, while still preserving contiguous stop-at-first-overflow behavior for older tail messages. Added regression coverage for:
Validation:
Latest commit on the PR branch: |
stephenschoettler
left a comment
There was a problem hiding this comment.
Review: Approve (updated)
Both issues from my previous review are fixed:
continue→breakin tail trimming — now produces contiguous recent tailcontinue→breakin summary selection — stops on first overflow- Renamed
tail_budget→summary_budget
The and kept_tail_reversed guard on the break condition is a nice touch — allows a single oversized message to still be included if nothing else has been selected yet.
CI green across 3.11-3.13. LGTM.
This adds two optional guardrails for active-context assembly:
max_assembly_tokens: a hard cap for the assembled contextreserve_tokens_floor: keeps headroom inside the model context windowWhy this helps:
maxAssemblyTokenBudgetstyle control without changing the existing default behaviorTests:
python3 -m pytest tests/test_lcm_engine.py tests/test_lcm_core.py -qI kept this separate from the focus-topic change so each PR is easy to review on its own.