Add content validation to Message class - #25
Conversation
There was a problem hiding this comment.
Thanks — the intent (fail fast on garbage LLM output) is reasonable, but as written this validation rejects content that is legitimately empty on the agent's most common code path, and the resulting ValueError is not caught anywhere. Net effect: the agent crashes during normal operation.
1. agent/core/conversation.py:20-22 breaks native tool calls — agent/core/engine.py:105
content = response.get("message", {}).get("content", "") # engine.py:94
...
if tool_calls_from_api:
self.conversation.add_assistant(content, tool_calls_from_api) # engine.py:105When an Ollama/OpenAI-style model replies with tool_calls, message.content is conventionally "" (or absent, hence the "" default on line 94). Every native tool call therefore now raises ValueError before the tool is ever executed. Repro:
Conversation().add_assistant("", [{"function": {"name": "read_file", "arguments": {}}}])
# ValueError: Message content cannot be None or empty (role=assistant)An assistant message carrying tool_calls must be allowed to have empty content. Suggested guard:
def __post_init__(self):
if self.tool_calls: # tool-call messages legitimately have empty content
return
if not self.content or (isinstance(self.content, str) and not self.content.strip()):
raise ValueError(...)2. agent/core/engine.py:130 — empty final response becomes a hard crash
An empty/whitespace-only completion is a benign, recoverable model output, but add_assistant(content) on line 130 now raises instead of returning. Same for line 118 if _clean_content strips a content block down to nothing.
3. Nothing catches the exception, so it takes down the process
process_message only wraps _call_llm in try/except OllamaError (engine.py:87-92), and the REPL in agent/main.py:240-266 catches only KeyboardInterrupt and EOFError. A ValueError from Message.__post_init__ propagates out of run_repl as an unhandled traceback and exits the program — mid-turn, after tools may already have run.
4. agent/core/conversation.py:106 — load() can no longer read valid saved sessions
save() (line 92) happily persists assistant messages with empty content; load() reconstructs Message objects, so /load (main.py:218) on any session saved before this change now throws on a previously-working file. Consider tolerating (or logging and skipping) invalid entries on load rather than raising.
5. No tests
tests/test_tools.py:246-283 and tests/test_engine.py:71-72 all pass non-empty strings, so nothing exercises the new behavior. Please add cases for: rejecting ""/None/whitespace, and accepting an assistant message with empty content plus tool_calls.
Scope and security are fine — single file, no injection/path/secret concerns. Happy to re-review once the tool_calls exemption and the engine-side handling are in.
|
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
Added validation in Message.post_init to reject None or empty content.
Why
Catches LLM parsing failures early by preventing invalid messages from being created, improving debugging and system reliability.