[ssevera1]

What

Add validation that Ollama response contains required 'done' field in non-streaming chat() method, matching the validation already present in _stream_chat().

Why

Ensures consistent response validation across both streaming and non-streaming code paths.

[claude]

The intent (parity between chat() and _stream_chat() on the done field) is reasonable and the one-line change is well scoped, but as written it breaks an existing test that was not updated in this PR.

agent/core/llm.py:207 — self._validate_response(result, ["done"]) now rejects any non-streaming response without a top-level done key. tests/test_llm.py:85-94 (test_chat_accepts_tool_call_only_response) patches _request to return:

response = {"message": {"role": "assistant", "tool_calls": [...]}}

with no done field, then asserts client.chat(...) == response. With this change that call raises OllamaError("Malformed API response: missing required fields ['done']") and the test fails. Please add "done": True to that fixture (and any other _request-patched chat fixture) as part of this PR.

Two smaller points while you're in there:

1. tests/test_llm.py:96-100 (test_chat_rejects_non_dict_message) still passes, but only incidentally — {"message": None} now trips the done check before _validate_message_structure ever runs, so the test no longer exercises what its name claims. Adding "done": True to that fixture too would keep it meaningful.
2. There is no new test covering the behavior being added. A test asserting that a non-streaming response missing done raises OllamaError would lock in the parity this PR is after; right now the new line is untested.

Happy to approve once the fixtures are updated and the new validation has coverage.

[claude]

Verified the reviewer's claims directly against the code and they were correct:

Fixed:

• tests/test_llm.py::test_chat_accepts_tool_call_only_response was indeed broken by the new _validate_response(result, ["done"]) call in agent/core/llm.py:207 — confirmed by running it before the fix (failed with OllamaError: missing required fields ['done']). Added "done": True to its _request fixture.
• test_chat_rejects_non_dict_message's fixture ({"message": None}) was tripping the new done check before reaching _validate_message_structure, so it was no longer testing what its name claims. Added "done": True so it again exercises the non-dict-message path.
• Added test_chat_rejects_response_missing_done, a new regression test asserting chat() raises OllamaError when the non-streaming response is missing done — this failed before the fix in agent/core/llm.py (which was already merged in this PR) and locks in the parity with _stream_chat().
• Searched the rest of tests/ for any other _request-patched chat() fixtures; found none, so no other fixtures needed updating.

Checks: pytest -q → 96 passed. mypy --ignore-missing-imports agent → no issues found.

[claude]

Checked agent/core/llm.py:207 against _validate_response and _validate_message_structure: requiring a top-level done on the non-streaming /api/chat response mirrors the existing check in _stream_chat (llm.py:227), and Ollama's non-streaming chat response does include done. The earlier review's concerns are resolved — test_chat_accepts_tool_call_only_response and test_chat_rejects_non_dict_message both carry "done": True so the latter again reaches the message-structure path, and test_chat_rejects_response_missing_done" covers the new behavior; I grepped tests/and found no other_request-patched chat()fixtures (engine tests patch_call_llm`, so they're unaffected). No security surface touched and the change is limited to the one validation call plus its tests.