From 8b0c600b61ea8244f0842a9e6d5b899d18fa0c9d Mon Sep 17 00:00:00 2001 From: myusername Date: Mon, 24 Aug 2026 09:32:05 -0500 Subject: [PATCH 1/2] fix: allow legitimately-empty message content PR #27 added a blanket 'content must be non-empty' guard to every Conversation.add_* method. Two of those paths legitimately carry empty content, so the guard crashes the agent on main: - engine.py:105 records a native tool call with add_assistant(content, tool_calls). Ollama returns content='' when the model emits only a tool call, so any native tool call raises ValueError. - engine.py:182 records str(ToolResult); a successful tool with no output (grep with no match, a silent write) stringifies to '' and raises ValueError. Neither call site is inside the loop's try/except, which only catches OllamaError around _call_llm, so both propagate out of process_message. No test covered process_message with native tool calls, so CI stayed green. Empty is now allowed exactly where it is meaningful (assistant messages carrying tool_calls, tool results) and still rejected everywhere else. None is rejected on all paths. --- agent/core/conversation.py | 16 ++++++++++++---- tests/test_tools.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/agent/core/conversation.py b/agent/core/conversation.py index 8e18eb6..ba2c254 100644 --- a/agent/core/conversation.py +++ b/agent/core/conversation.py @@ -49,15 +49,23 @@ def add_user(self, content: str): self.messages.append(Message(role="user", content=content)) def add_assistant(self, content: str, tool_calls: Optional[list] = None): - if not content or not content.strip(): - raise ValueError("Assistant message content cannot be None or empty") + if content is None: + raise ValueError("Assistant message content cannot be None") + # A native tool call arrives with empty content and a populated + # tool_calls list; that is the normal shape, not a malformed message. + if not tool_calls and not content.strip(): + raise ValueError( + "Assistant message content cannot be empty without tool calls" + ) self.messages.append( Message(role="assistant", content=content, tool_calls=tool_calls) ) def add_tool_result(self, tool_call_id: str, name: str, content: str): - if not content or not content.strip(): - raise ValueError("Tool result content cannot be None or empty") + # A successful tool can legitimately produce no output (grep with no + # match, a write that prints nothing), so empty is allowed here. + if content is None: + raise ValueError("Tool result content cannot be None") self.messages.append( Message(role="tool", content=content, tool_call_id=tool_call_id, name=name) ) diff --git a/tests/test_tools.py b/tests/test_tools.py index cd5d9d3..a15bc7e 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -291,6 +291,41 @@ def test_save_load(self): finally: shutil.rmtree(tmpdir) + def test_assistant_message_may_be_empty_when_tool_calls_present(self): + """Native tool calls arrive with empty content; that is a valid message.""" + conv = Conversation() + conv.add_system("System") + conv.add_assistant("", [{"id": "1", "function": {"name": "read_file"}}]) + + messages = conv.get_messages() + self.assertEqual(messages[-1]["role"], "assistant") + self.assertEqual(messages[-1]["tool_calls"][0]["id"], "1") + + def test_assistant_message_still_rejects_empty_without_tool_calls(self): + conv = Conversation() + with self.assertRaises(ValueError): + conv.add_assistant("") + + def test_assistant_message_rejects_none(self): + conv = Conversation() + with self.assertRaises(ValueError): + conv.add_assistant(None) + + def test_tool_result_may_be_empty(self): + """A successful tool can legitimately produce no output (e.g. grep, no match).""" + conv = Conversation() + conv.add_system("System") + conv.add_tool_result("1", "grep", "") + + messages = conv.get_messages() + self.assertEqual(messages[-1]["role"], "tool") + self.assertEqual(messages[-1]["content"], "") + + def test_tool_result_rejects_none(self): + conv = Conversation() + with self.assertRaises(ValueError): + conv.add_tool_result("1", "grep", None) + class TestSafety(unittest.TestCase): def test_sensitive_files(self): From 8873f1543d7e5a173fbea4bc653f1169da439615 Mon Sep 17 00:00:00 2001 From: myusername Date: Mon, 24 Aug 2026 10:00:08 -0500 Subject: [PATCH 2/2] fix: allow empty assistant turns outright, cover process_message Review caught a third crash path this PR missed: engine.py:130, the final-response branch, calls add_assistant(content) with no tool_calls. An empty or whitespace-only final message - truncation at max tokens, a bare newline, a model that emits nothing after its last tool result - still tripped the guard and escaped process_message(). Taking review option 1: #27's premise was simply wrong. 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. Keeping the guard and special-casing one call site would have left the same trap for the next caller. Also defended the extraction point: response content/tool_calls now use 'or ""' / 'or []', so a present-but-null JSON value cannot reach add_assistant as None. Added TestProcessMessageEmptyResponses, which drives process_message end-to-end rather than Conversation in isolation - the gap that let #27 through CI. Three of its four cases fail against the previous version of this branch. --- agent/core/conversation.py | 10 +++--- agent/core/engine.py | 6 ++-- tests/test_engine.py | 67 ++++++++++++++++++++++++++++++++++++++ tests/test_tools.py | 11 +++++-- 4 files changed, 83 insertions(+), 11 deletions(-) diff --git a/agent/core/conversation.py b/agent/core/conversation.py index ba2c254..8dfd57d 100644 --- a/agent/core/conversation.py +++ b/agent/core/conversation.py @@ -49,14 +49,12 @@ def add_user(self, content: str): self.messages.append(Message(role="user", content=content)) def add_assistant(self, content: str, tool_calls: Optional[list] = None): + # An empty assistant turn is a valid shape in the chat API, with or + # without tool calls: a native tool call carries no prose, and a + # truncated or silent final response legitimately has none either. + # Only None indicates a genuine caller bug. if content is None: raise ValueError("Assistant message content cannot be None") - # A native tool call arrives with empty content and a populated - # tool_calls list; that is the normal shape, not a malformed message. - if not tool_calls and not content.strip(): - raise ValueError( - "Assistant message content cannot be empty without tool calls" - ) self.messages.append( Message(role="assistant", content=content, tool_calls=tool_calls) ) diff --git a/agent/core/engine.py b/agent/core/engine.py index e092f97..46b2288 100644 --- a/agent/core/engine.py +++ b/agent/core/engine.py @@ -91,8 +91,10 @@ def process_message(self, user_input: str) -> str: print(error(error_msg)) return error_msg - content = response.get("message", {}).get("content", "") - tool_calls_from_api = response.get("message", {}).get("tool_calls", []) + # `or ""` guards a present-but-null content key, which would + # otherwise reach add_assistant as None. + content = response.get("message", {}).get("content", "") or "" + tool_calls_from_api = response.get("message", {}).get("tool_calls", []) or [] # Check for tool calls in the content (```tool blocks) tool_calls_from_content = self._extract_tool_calls(content) diff --git a/tests/test_engine.py b/tests/test_engine.py index ae3add6..e198e91 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -182,3 +182,70 @@ def test_git_write_not_auto_approved(self): if __name__ == "__main__": unittest.main() + + +class TestProcessMessageEmptyResponses(unittest.TestCase): + """End-to-end cover for the paths #27 made crashable. + + These drive process_message itself rather than Conversation in + isolation - a unit test on Conversation alone is exactly what let #27 + through CI. + """ + + def _engine(self): + config = Config() + config.web.enabled = False + config.agent.max_turns = 3 + return AgentEngine(config) + + def test_empty_final_response_does_not_crash(self): + engine = self._engine() + with patch.object( + engine, "_call_llm", return_value={"message": {"content": ""}} + ): + result = engine.process_message("hello") + + self.assertEqual(result, "") + self.assertEqual(engine.conversation.messages[-1].role, "assistant") + + def test_whitespace_only_final_response_does_not_crash(self): + engine = self._engine() + with patch.object( + engine, "_call_llm", return_value={"message": {"content": " "}} + ): + result = engine.process_message("hello") + + self.assertEqual(result.strip(), "") + + def test_null_content_does_not_crash(self): + """A present-but-null content key must not reach add_assistant as None.""" + engine = self._engine() + with patch.object( + engine, "_call_llm", return_value={"message": {"content": None}} + ): + result = engine.process_message("hello") + + self.assertEqual(result, "") + + def test_native_tool_call_with_empty_content_does_not_crash(self): + engine = self._engine() + responses = [ + { + "message": { + "content": "", + "tool_calls": [ + { + "id": "call_1", + "function": {"name": "list_directory", "arguments": {}}, + } + ], + } + }, + {"message": {"content": "All done."}}, + ] + with patch.object(engine, "_call_llm", side_effect=responses): + result = engine.process_message("list the files") + + self.assertEqual(result, "All done.") + roles = [m.role for m in engine.conversation.messages] + self.assertIn("tool", roles) diff --git a/tests/test_tools.py b/tests/test_tools.py index a15bc7e..fa381c3 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -301,10 +301,15 @@ def test_assistant_message_may_be_empty_when_tool_calls_present(self): self.assertEqual(messages[-1]["role"], "assistant") self.assertEqual(messages[-1]["tool_calls"][0]["id"], "1") - def test_assistant_message_still_rejects_empty_without_tool_calls(self): + def test_assistant_message_may_be_empty_without_tool_calls(self): + """A truncated or silent final response is still a valid assistant turn.""" conv = Conversation() - with self.assertRaises(ValueError): - conv.add_assistant("") + conv.add_system("System") + conv.add_assistant("") + + messages = conv.get_messages() + self.assertEqual(messages[-1]["role"], "assistant") + self.assertEqual(messages[-1]["content"], "") def test_assistant_message_rejects_none(self): conv = Conversation()