Skip to content

Add content validation to Message class - #25

Closed
ssevera1 wants to merge 1 commit into
mainfrom
improve/20260817-215554
Closed

Add content validation to Message class#25
ssevera1 wants to merge 1 commit into
mainfrom
improve/20260817-215554

Conversation

@ssevera1

Copy link
Copy Markdown
Owner

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.

@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 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:105

When 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:106load() 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.

@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.

@ssevera1 ssevera1 closed this Aug 24, 2026
@ssevera1
ssevera1 deleted the improve/20260817-215554 branch August 24, 2026 16:34
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