Fix agent crash on native tool calls and empty tool output

[ssevera1]

The regression

PR #27 (f1b5091) added a blanket non-empty guard to every Conversation.add_*
method. Two call sites legitimately pass empty content, so main currently
crashes the agent on both:

Path	Call site	Why content is empty
Native tool calls	engine.py:105	Ollama returns message.content == "" when the model emits only a tool call
Tool results	engine.py:182	str(ToolResult(True, "")) is "" — grep with no match, a silent write

Neither is inside the loop's try/except, which only catches OllamaError
around _call_llm(), so the ValueError propagates straight out of
process_message().

Reproduced against main:

>>> conv.add_assistant("", [{"id": "1", "function": {"name": "read_file"}}])
ValueError: Assistant message content cannot be None or empty
>>> conv.add_tool_result("1", "grep", "")
ValueError: Tool result content cannot be None or empty

No test exercised process_message with tool_calls_from_api populated —
test_engine.py only covers _extract_tool_calls in isolation — which is why
CI stayed green through the merge.

The fix

Empty is allowed exactly where it carries meaning, and rejected everywhere else:

• add_assistant — empty allowed only when tool_calls is populated
• add_tool_result — empty allowed (a successful tool may produce no output)
• add_system / add_user — unchanged, empty still rejected
• None still rejected on every path

Verification

• 5 regression tests added to TestConversation, covering both crash paths plus
  the cases that must still raise. Both new "may be empty" tests fail on main.
• Full suite: 55 passed
• mypy --ignore-missing-imports agent: clean

Supersedes #23, #24, #25 and #26, which all proposed variants of this same
validation against a pre-#27 base.

[claude]

The two changes in agent/core/conversation.py are correct in isolation, and I verified the premises: ToolResult.__str__ does return "" for ToolResult(True, "") (agent/tools/base.py:15-18), and engine.py:105 / engine.py:182 are outside the try/except OllamaError, so the ValueError from #27 does escape process_message(). The new tests land inside TestConversation and cover both directions. Scope is clean — only conversation.py and tests.

Blocking issue: the third crash path from #27 is still live.

agent/core/engine.py:130 (the else / final-response branch) also calls add_assistant(content) with content that came straight from response.get("message", {}).get("content", ""). When the model returns an empty or whitespace-only final message — truncation at max tokens, a bare "\n", a model that emits nothing after its last tool result — tool_calls_from_api is empty and _extract_tool_calls finds nothing, so control reaches line 130 with content = "". The new guard (if not tool_calls and not content.strip()) fires, and the ValueError propagates out of process_message() exactly like the two paths this PR fixes. Pre-#27 this returned an empty string harmlessly.

So the PR description's table is missing a row, and merging as-is still leaves the agent crashable on an empty LLM response.

Two reasonable ways to close it:

1. Drop the empty check from add_assistant entirely and keep only the None rejection. #27's blanket non-empty premise simply does not hold for assistant messages — an empty assistant turn is a valid API shape with or without tool calls, and the tool_calls-aware branch in this diff is then unnecessary.
2. Keep the guard and normalize at the call site in engine.py:130 — e.g. record and return content or "" without going through the validating path when the model produced nothing.

Either way, please add a regression test that drives process_message with a mocked LLM returning {"message": {"content": ""}} and no tool calls. That is the same gap the description already identifies as the reason CI stayed green through #27: test_engine.py never exercises process_message end to end, so a unit test on Conversation alone will not catch a recurrence.

Minor, non-blocking: response.get("message", {}).get("content", "") returns None if the key is present with a JSON null, which would then hit the content is None raise on all three branches. Worth defending with or "" at the extraction point in the same pass.

[ssevera1]

Confirmed and fixed — you're right, and the description's table was missing a row.

engine.py:130 reaches add_assistant(content) with tool_calls=None and content="" whenever the model returns an empty or whitespace-only final message, and my not tool_calls and not content.strip() guard fired there exactly as you describe.

Took option 1. #27's premise doesn't hold: an empty assistant turn is a valid chat-API shape with or without tool calls, so add_assistant now rejects only None and the tool_calls-aware branch is gone. Option 2 would have kept a semantically wrong guard and worked around it at one call site, leaving the same trap for the next caller.

Minor taken in the same pass: content and tool_calls extraction now use or "" / or [], so a present-but-null JSON value can't reach add_assistant as None.

Regression test at the right level: added TestProcessMessageEmptyResponses, driving process_message end-to-end with a mocked _call_llm — empty content, whitespace-only, null content, and a native tool call followed by a final response. Against the previous version of this branch, three of the four fail:

FAILED test_empty_final_response_does_not_crash
FAILED test_whitespace_only_final_response_does_not_crash
FAILED test_null_content_does_not_crash

That closes the coverage gap that let #27 through in the first place.

Full suite: 59 passed. mypy: clean.

[claude]

Reviewed the diff against agent/core/conversation.py and agent/core/engine.py. The change correctly reverts the over-broad non-empty guard from #27 on exactly the two paths that legitimately carry empty content — add_assistant (native Ollama tool calls arrive with content == "") and add_tool_result (str(ToolResult(True, "")) for a no-match grep or silent write) — while keeping None rejected everywhere and leaving add_system/add_user untouched. The or "" normalization at engine.py:96-97 correctly handles a present-but-null content/tool_calls key, and since the new validation is strictly more permissive than what's on main it cannot introduce a new crash path; the loop still terminates on empty final content rather than spinning. Tests are well targeted (they drive process_message end-to-end, which is the gap that let #27 through CI) and the scope is limited to the regression. One nit, non-blocking: the PR description still says "empty allowed only when tool_calls is populated" for add_assistant, but commit 8873f15 made it unconditional — worth updating the body so it matches the code.