From 9b97fd7d75836e9279928042de1ab75cc24d5910 Mon Sep 17 00:00:00 2001 From: Scott Severance Date: Tue, 29 Sep 2026 06:48:27 +0000 Subject: [PATCH 1/2] validate: require tool_call_id and name in add_tool_result --- agent/core/conversation.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/agent/core/conversation.py b/agent/core/conversation.py index 8dfd57d..488cf74 100644 --- a/agent/core/conversation.py +++ b/agent/core/conversation.py @@ -64,6 +64,10 @@ def add_tool_result(self, tool_call_id: str, name: str, content: str): # match, a write that prints nothing), so empty is allowed here. if content is None: raise ValueError("Tool result content cannot be None") + if not tool_call_id or not tool_call_id.strip(): + raise ValueError("Tool result tool_call_id cannot be None or empty") + if not name or not name.strip(): + raise ValueError("Tool result name cannot be None or empty") self.messages.append( Message(role="tool", content=content, tool_call_id=tool_call_id, name=name) ) From 302e87154bc4d55beb6e858f540db95880247e59 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Tue, 29 Sep 2026 06:52:13 +0000 Subject: [PATCH 2/2] fix(review): normalize null id/name from native tool calls before recording results engine.py used dict.get(key, default) for the tool call's id and the function's name, but the default only fires when the key is absent - a present-but-null id or name from a non-conforming Ollama build or proxy passed straight through as None/"". add_tool_result now rejects None/empty tool_call_id and name, so that ValueError escaped process_message uncaught (only OllamaError is caught around _call_llm), crashing the whole turn - the same regression shape as #27/#28. Normalize with `or` at the extraction point, matching the existing `content or ""` guard on the line above, and add end-to-end process_message regression tests for both a null id and a null name. --- agent/core/engine.py | 10 +++++---- tests/test_engine.py | 51 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/agent/core/engine.py b/agent/core/engine.py index 8150321..06f80b9 100644 --- a/agent/core/engine.py +++ b/agent/core/engine.py @@ -110,10 +110,12 @@ def process_message(self, user_input: str) -> str: print(display_content) for tc in tool_calls_from_api: - func = tc.get("function", {}) - name = func.get("name", "unknown") - args = func.get("arguments", {}) - self._execute_and_record_tool(name, args, tc.get("id", name)) + func = tc.get("function", {}) or {} + # `or` guards a present-but-null key, which would + # otherwise reach add_tool_result as None/"". + name = func.get("name") or "unknown" + args = func.get("arguments") or {} + self._execute_and_record_tool(name, args, tc.get("id") or name) elif tool_calls_from_content: # Handle tool calls from content diff --git a/tests/test_engine.py b/tests/test_engine.py index e198e91..531356b 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -249,3 +249,54 @@ def test_native_tool_call_with_empty_content_does_not_crash(self): self.assertEqual(result, "All done.") roles = [m.role for m in engine.conversation.messages] self.assertIn("tool", roles) + + def test_native_tool_call_with_null_id_does_not_crash(self): + """A present-but-null "id" must not reach add_tool_result as None.""" + engine = self._engine() + responses = [ + { + "message": { + "content": "", + "tool_calls": [ + { + "id": None, + "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.") + tool_messages = [m for m in engine.conversation.messages if m.role == "tool"] + self.assertEqual(len(tool_messages), 1) + self.assertTrue(tool_messages[0].tool_call_id) + + def test_native_tool_call_with_null_name_does_not_crash(self): + """A present-but-null function "name" must not reach add_tool_result as None.""" + engine = self._engine() + responses = [ + { + "message": { + "content": "", + "tool_calls": [ + { + "id": "call_1", + "function": {"name": None, "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.") + tool_messages = [m for m in engine.conversation.messages if m.role == "tool"] + self.assertEqual(len(tool_messages), 1) + self.assertEqual(tool_messages[0].name, "unknown") + self.assertIn("Unknown tool", tool_messages[0].content)