Skip to content

Add context bounds validation before LLM calls - #23

Closed
ssevera1 wants to merge 1 commit into
mainfrom
improve/20260812-111824
Closed

Add context bounds validation before LLM calls#23
ssevera1 wants to merge 1 commit into
mainfrom
improve/20260812-111824

Conversation

@ssevera1

Copy link
Copy Markdown
Owner

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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ssevera1

Copy link
Copy Markdown
Owner Author

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 main — and it carries the exact defect the review on this PR flagged: add_assistant/add_tool_result reject empty content, which crashes the agent on native tool calls (engine.py:105) and on successful zero-output tools (engine.py:182).

#28 fixes the live regression on main and adds the regression tests that were missing. Reproduced both crash paths before fixing; full suite green after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant