Add context bounds validation before LLM calls - #23
Conversation
There was a problem hiding this comment.
Thanks — the logic inside the new method is correct in isolation, but as written this PR does not accomplish what the description claims, so I think it needs another pass.
1. _validate_context() is dead code (agent/core/conversation.py:81).
The PR says it will "verify conversation fits within hard limits before sending to LLM" and "catch violations before API calls," but nothing ever calls it. The only path to the LLM is Conversation.get_messages() (conversation.py:57-60), invoked from Engine._call_llm() (agent/core/engine.py:137), and get_messages() calls only _trim_context(). grep -rn "_validate_context" returns exactly one hit — the definition. As merged, behavior is unchanged and no bounds are enforced. Please wire it into get_messages() (e.g. after _trim_context()) or drop it.
2. Wiring it in as a hard gate needs a defined failure mode, or it can fail unrecoverably.
_trim_context() returns early when len(self.messages) <= 2 (conversation.py:64-65) and its while loop also refuses to trim below 2 messages (line 73). So a system message plus one oversized user message leaves total_chars > max_chars with no way for the trimmer to fix it — _validate_context() would return False on every subsequent call, permanently. Please decide and implement what happens on failure: truncate the offending message content, raise a specific exception the engine handles, or log and proceed. A bare False that no caller can act on is worse than no check.
3. The char count understates the real payload (conversation.py:88).
sum(len(m.content) for m in self.messages) ignores tool_calls, tool_call_id, and name, which Message.to_dict() does serialize (conversation.py:20-28). Tool-call payloads can be large in an agent loop, so a method presented as a "hard limits" check will pass conversations that are well over budget. _trim_context() has the same gap, but it is worth either fixing here or noting explicitly in the docstring that this is a content-only approximation.
4. No test coverage. tests/ has no case for the new method — worth adding at least the over-max_messages, over-max_chars, and within-bounds cases, plus the single-oversized-message edge case from point 2.
No security concerns, and the scope is appropriately narrow (one method, one file). Happy to re-review once it is actually invoked and the failure path is defined.
Nit: trailing whitespace on line 83.
|
Closing in favour of #28. All four of these PRs (#23-#26) propose variants of the same content validation against a pre-#27 base. #27 already merged that validation into #28 fixes the live regression on |
What
Add
_validate_context()method to verify conversation fits within hard limits before sending to LLM.Why
Provides explicit bounds checking to prevent unbounded context window growth and catch violations before API calls.