From c6136da08bc08e5fae4351af96dd5162ed4e4071 Mon Sep 17 00:00:00 2001 From: Alessandro <155005371+3clyp50@users.noreply.github.com> Date: Sat, 25 Jul 2026 00:44:56 +0200 Subject: [PATCH] fix(response): keep completions on the tool path Route native Responses output text through the normalized response-tool executor instead of ending the monologue directly. Preserve active goal overrides and Responses state cleanup, wait for complete native streams, and reject empty response payloads. --- AGENTS.md | 1 + agent.py | 16 ++- models.py | 7 +- plugins/_goal/tests/test_goal_plugin.py | 54 +++++++- tests/test_response_tool_validation.py | 37 +++++- tests/test_responses_architecture.py | 163 +++++++++++++++++------- tests/test_stream_tool_early_stop.py | 32 ++--- tools/response.py | 4 +- tools/response.py.dox.md | 3 +- 9 files changed, 240 insertions(+), 77 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2ee5811e7..3da3cebb6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,6 +28,7 @@ - Preserve authentication and CSRF protections. - Use Linux paths and commands in examples. - When a live Dockerized Agent Zero target is explicitly named, verify that exact runtime instead of assuming a fixed localhost port. +- Message-loop completion flows through a response tool with `break_loop`; plain or malformed Chat Completions text enters repair, and native Responses output text is normalized through the same response-tool path. - Copy live core-plugin changes back into tracked source under `plugins/`. - Develop new custom plugins under ignored `usr/plugins/`; tracked bundled plugins live under `plugins/`. - Use the framework runtime for backend and plugin-hook verification, not the separate agent execution runtime. diff --git a/agent.py b/agent.py index 134f8dc39..c20a7b38b 100644 --- a/agent.py +++ b/agent.py @@ -1119,10 +1119,18 @@ class Agent: or extract_tools.is_misformatted_tool_request(llm_result.reasoning) ): message = llm_result.reasoning - if extract_tools.extract_tool_request(message) is None: - if extract_tools.is_misformatted_tool_request(message): - return await self.process_tools(message) - return message + if ( + llm_result.mode == "responses" + and isinstance(message, str) + and bool(message.strip()) + and extract_tools.extract_tool_request(message) is None + and not extract_tools.is_misformatted_tool_request(message) + ): + return await self._execute_tool_request( + tool_name="response", + tool_args={"text": message}, + message=message, + ) return await self.process_tools(message) async def _execute_tool_request( diff --git a/models.py b/models.py index 805f79c79..e1fa5c423 100644 --- a/models.py +++ b/models.py @@ -738,7 +738,10 @@ class LiteLLMChatWrapper(SimpleChatModel): output["response_delta"] ) ) - if stop_response is not None: + if ( + stop_response is not None + and not transport.policy.using_responses + ): result.response = stop_response break else: @@ -761,7 +764,7 @@ class LiteLLMChatWrapper(SimpleChatModel): provider_model_key=self.model_name, capability=transport._capability_metadata(), ) - if result.output()["response_delta"]: + if result.output()["response_delta"] and not llm_result.function_calls: llm_result.response = result.output()["response_delta"] if result.output()["reasoning_delta"]: llm_result.reasoning = result.output()["reasoning_delta"] diff --git a/plugins/_goal/tests/test_goal_plugin.py b/plugins/_goal/tests/test_goal_plugin.py index bfae7a43f..ad0f74355 100644 --- a/plugins/_goal/tests/test_goal_plugin.py +++ b/plugins/_goal/tests/test_goal_plugin.py @@ -5,7 +5,10 @@ from types import SimpleNamespace import pytest -from helpers import files +from agent import Agent, LoopData +from helpers import extension, files, mcp_handler +from helpers.llm_result import LLMResult +from helpers.log import Log from plugins._goal.api.goal import Goal as GoalApi from plugins._goal.commands import goal_command from plugins._goal.tools import goal @@ -180,3 +183,52 @@ async def test_active_goal_keeps_response_tool_running(context_id: str): response = await tool.execute() assert response.break_loop is True assert response.message == "Can you decide?" + + +@pytest.mark.asyncio +async def test_native_responses_text_uses_active_goal_response_override( + context_id: str, + monkeypatch, +): + goal.create_goal(context_id, "Keep going") + recorded = [] + + async def no_op(*args, **kwargs): + return None + + class NoMcpTools: + def get_tool(self, agent, tool_name): + return None + + agent = object.__new__(Agent) + agent.context = SimpleNamespace(id=context_id, log=Log()) + agent.loop_data = LoopData() + agent.data = {} + agent.handle_intervention = no_op + agent._log_response_builtin_items = no_op + agent.hist_add_tool_result = lambda *args, **kwargs: recorded.append((args, kwargs)) + + def get_tool(name, method, args, message, loop_data, **kwargs): + return ResponseTool(agent, name, method, args, message, loop_data) + + agent.get_tool = get_tool + monkeypatch.setattr(extension, "call_extensions_async", no_op) + monkeypatch.setattr(mcp_handler.MCPConfig, "get_instance", lambda: NoMcpTools()) + + result = await Agent.process_llm_result_tools( + agent, + LLMResult(response="Checkpoint for the user."), + ) + + assert result is None + assert recorded[0][0][0] == "response" + assert recorded[0][0][1].startswith("Goal still active.") + assert recorded[0][1] == {} + + goal.update_goal(context_id, status="complete") + result = await Agent.process_llm_result_tools( + agent, + LLMResult(response="Finished."), + ) + + assert result == "Finished." diff --git a/tests/test_response_tool_validation.py b/tests/test_response_tool_validation.py index feb2fa4fd..b0a3df732 100644 --- a/tests/test_response_tool_validation.py +++ b/tests/test_response_tool_validation.py @@ -23,6 +23,41 @@ async def test_response_tool_accepts_text_or_message(args) -> None: assert response.break_loop is True +@pytest.mark.asyncio +async def test_response_tool_uses_non_empty_legacy_message_fallback() -> None: + tool = ResponseTool( + None, + "response", + None, + {"text": "", "message": "legacy"}, + "", + None, + ) + + response = await tool.execute() + + assert response.message == "legacy" + assert response.break_loop is True + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "args", + [ + {}, + {"text": ""}, + {"text": " "}, + {"text": None}, + {"message": "\n\t"}, + ], +) +async def test_response_tool_rejects_empty_arguments(args) -> None: + tool = ResponseTool(None, "response", None, args, "", None) + + with pytest.raises(RepairableException, match="non-empty top-level"): + await tool.execute() + + @pytest.mark.asyncio async def test_response_tool_rejects_nested_response_args() -> None: tool = ResponseTool( @@ -39,5 +74,5 @@ async def test_response_tool_rejects_nested_response_args() -> None: None, ) - with pytest.raises(RepairableException, match="top-level text or message"): + with pytest.raises(RepairableException, match="non-empty top-level"): await tool.execute() diff --git a/tests/test_responses_architecture.py b/tests/test_responses_architecture.py index 48c7e8833..1a195cb62 100644 --- a/tests/test_responses_architecture.py +++ b/tests/test_responses_architecture.py @@ -1,4 +1,4 @@ -import asyncio +import json import sys from pathlib import Path @@ -44,28 +44,6 @@ class _AsyncEventStream: self.closed = True -class _StallingAsyncEventStream: - def __init__(self, event: dict): - self.event = event - self.sent = False - self.closed = False - self._stalled = asyncio.Event() - - def __aiter__(self): - return self - - async def __anext__(self): - if not self.sent: - self.sent = True - return self.event - await self._stalled.wait() - raise StopAsyncIteration - - async def aclose(self): - self.closed = True - self._stalled.set() - - def test_llm_result_persists_only_durable_responses_metadata(): result = LLMResult.from_response( { @@ -340,13 +318,58 @@ async def test_unified_turn_captures_response_id_without_stop_request(monkeypatc @pytest.mark.asyncio -async def test_unified_turn_stops_responses_stream_after_callback_stop(monkeypatch): - message = ( - '{"thoughts":["test"],"actions":[' - '{"tool_name":"response","tool_args":{"text":"ok"}}]}' - ) - stream = _StallingAsyncEventStream( - {"type": "response.output_text.delta", "delta": message} +async def test_unified_turn_waits_for_completed_native_responses_calls(monkeypatch): + calls = [ + { + "type": "function_call", + "id": "fc_1", + "call_id": "call_1", + "name": "lookup", + "arguments": '{"q":"a0"}', + }, + { + "type": "function_call", + "id": "fc_2", + "call_id": "call_2", + "name": "summarize", + "arguments": '{"style":"short"}', + }, + ] + stream = _AsyncEventStream( + [ + { + "type": "response.output_item.added", + "output_index": 0, + "item": {**calls[0], "arguments": ""}, + }, + { + "type": "response.function_call_arguments.done", + "item_id": "fc_1", + "output_index": 0, + "name": "lookup", + "arguments": calls[0]["arguments"], + }, + { + "type": "response.output_item.added", + "output_index": 1, + "item": {**calls[1], "arguments": ""}, + }, + { + "type": "response.function_call_arguments.done", + "item_id": "fc_2", + "output_index": 1, + "name": "summarize", + "arguments": calls[1]["arguments"], + }, + { + "type": "response.completed", + "response": { + "id": "resp_parallel", + "output": calls, + "usage": {"input_tokens": 10, "output_tokens": 5}, + }, + }, + ] ) async def fake_aresponses(*args, **kwargs): @@ -367,16 +390,26 @@ async def test_unified_turn_stops_responses_stream_after_callback_stop(monkeypat async def response_callback(chunk: str, full: str): return full if extract_tools.extract_tool_request(full) else None - result = await asyncio.wait_for( - wrapper.unified_turn( - messages=[HumanMessage(content="hi")], - response_callback=response_callback, - ), - timeout=1, + result = await wrapper.unified_turn( + messages=[HumanMessage(content="hi")], + response_callback=response_callback, ) - assert result.response == message - assert stream.closed is True + assert stream.index == 5 + assert stream.closed is False + assert result.mode == "responses" + assert result.response_id == "resp_parallel" + assert result.usage == {"input_tokens": 10, "output_tokens": 5} + assert [call.name for call in result.function_calls] == ["lookup", "summarize"] + assert json.loads(result.response) == { + "tool_name": "parallel_tool_calls", + "tool_args": { + "calls": [ + {"tool_name": "lookup", "tool_args": {"q": "a0"}}, + {"tool_name": "summarize", "tool_args": {"style": "short"}}, + ] + }, + } def test_collect_response_ids_from_agent_state_and_history_metadata(): @@ -477,47 +510,81 @@ async def test_agent_executes_native_responses_function_call_and_records_output( @pytest.mark.asyncio -async def test_agent_routes_only_complete_text_tool_requests() -> None: +async def test_agent_routes_chat_retries_and_native_responses_text() -> None: agent = object.__new__(Agent) processed: list[str] = [] + executed: list[dict] = [] async def log_builtin_items(result): return None async def process_tools(message): processed.append(message) - return message + return None + + async def execute_tool_request(**kwargs): + executed.append(kwargs) + return None agent._log_response_builtin_items = log_builtin_items agent.process_tools = process_tools + agent._execute_tool_request = execute_tool_request tool_request = '{"type":"function","name":"response","parameters":{"text":"ok"}}' - for message in ( + chat_messages = ( "Plain final answer.", '{"status":"planning"}', f"Example tool JSON: {tool_request}", - ): + f"∂\n{tool_request}", + ( + '{"thoughts":["Done"],"headline":"Done","tool_args":' + '{"text":"ok","tool_name":"response"}' + ), + ) + for message in chat_messages: assert await Agent.process_llm_result_tools( agent, LLMResult.from_chat(response=message) - ) == message - assert processed == [] + ) is None + assert processed == list(chat_messages) + processed.clear() + responses_messages = ( + "Plain final answer.", + '{"status":"planning"}', + f"Example tool JSON: {tool_request}", + ) + for message in responses_messages: + assert await Agent.process_llm_result_tools( + agent, LLMResult(response=message) + ) is None + assert processed == [] + assert executed == [ + { + "tool_name": "response", + "tool_args": {"text": message}, + "message": message, + } + for message in responses_messages + ] + + processed.clear() + executed.clear() assert await Agent.process_llm_result_tools( agent, LLMResult.from_chat(response=tool_request) - ) == tool_request + ) is None assert processed == [tool_request] processed.clear() assert await Agent.process_llm_result_tools( agent, LLMResult(response="", reasoning=tool_request) - ) == tool_request + ) is None assert processed == [tool_request] processed.clear() assert await Agent.process_llm_result_tools( agent, LLMResult(response="", reasoning='{"status":"planning"}') - ) == "" - assert processed == [] + ) is None + assert processed == [""] @pytest.mark.asyncio diff --git a/tests/test_stream_tool_early_stop.py b/tests/test_stream_tool_early_stop.py index b4235204a..a1805d457 100644 --- a/tests/test_stream_tool_early_stop.py +++ b/tests/test_stream_tool_early_stop.py @@ -235,25 +235,22 @@ def test_provider_defaults_do_not_freeze_litellm_global_kwargs(monkeypatch): @pytest.mark.asyncio -async def test_unified_call_stops_after_canonical_root_snapshot(monkeypatch): +async def test_unified_call_stops_chat_after_canonical_root_snapshot(monkeypatch): stream = _AsyncChunkStream( [ - {"type": "response.created"}, - _response_event('{"tool_name":"response","tool_args":{"text":"hello"}}'), - _response_event(" unreachable"), + _chunk('{"tool_name":"response","tool_args":{"text":"hello"}}'), + _chunk(" unreachable"), ] ) - async def fake_aresponses(*args, **kwargs): + async def fake_acompletion(*args, **kwargs): assert kwargs["stream"] is True - assert kwargs["input"] == "" - assert kwargs["store"] is True return stream async def fake_rate_limiter(*args, **kwargs): return None - monkeypatch.setattr(litellm_transport, "aresponses", fake_aresponses) + monkeypatch.setattr(litellm_transport, "acompletion", fake_acompletion) monkeypatch.setattr(models, "apply_rate_limiter", fake_rate_limiter) monkeypatch.setattr( models.settings, @@ -265,6 +262,7 @@ async def test_unified_call_stops_after_canonical_root_snapshot(monkeypatch): model="test-model", provider="openai", model_config=None, + a0_api_mode="chat", ) seen: list[tuple[str, str]] = [] @@ -280,7 +278,7 @@ async def test_unified_call_stops_after_canonical_root_snapshot(monkeypatch): assert response == '{"tool_name":"response","tool_args":{"text":"hello"}}' assert reasoning == "" - assert stream.index == 2 + assert stream.index == 1 assert stream.closed is True assert len(seen) == 1 assert seen[0][1] == '{"tool_name":"response","tool_args":{"text":"hello"}}' @@ -290,25 +288,22 @@ async def test_unified_call_stops_after_canonical_root_snapshot(monkeypatch): async def test_unified_call_does_not_stop_for_embedded_tool_json(monkeypatch): stream = _AsyncChunkStream( [ - {"type": "response.created"}, - _response_event('Preamble {"note":"not the tool"}.\n'), - _response_event( + _chunk('Preamble {"note":"not the tool"}.\n'), + _chunk( '{"tool_name":"response","tool_args":{"text":"ok"}} trailing text' ), - _response_event(" unreachable"), + _chunk(" unreachable"), ] ) - async def fake_aresponses(*args, **kwargs): + async def fake_acompletion(*args, **kwargs): assert kwargs["stream"] is True - assert kwargs["input"] == "" - assert kwargs["store"] is True return stream async def fake_rate_limiter(*args, **kwargs): return None - monkeypatch.setattr(litellm_transport, "aresponses", fake_aresponses) + monkeypatch.setattr(litellm_transport, "acompletion", fake_acompletion) monkeypatch.setattr(models, "apply_rate_limiter", fake_rate_limiter) monkeypatch.setattr( models.settings, @@ -320,6 +315,7 @@ async def test_unified_call_does_not_stop_for_embedded_tool_json(monkeypatch): model="test-model", provider="openai", model_config=None, + a0_api_mode="chat", ) seen: list[tuple[str, str]] = [] @@ -338,7 +334,7 @@ async def test_unified_call_does_not_stop_for_embedded_tool_json(monkeypatch): '{"tool_name":"response","tool_args":{"text":"ok"}} trailing text unreachable' ) assert reasoning == "" - assert stream.index == 4 + assert stream.index == 3 assert stream.closed is False assert len(seen) == 3 assert seen[0][1] == 'Preamble {"note":"not the tool"}.\n' diff --git a/tools/response.py b/tools/response.py index ec5fd4d16..9e2532b5b 100644 --- a/tools/response.py +++ b/tools/response.py @@ -7,10 +7,10 @@ class ResponseTool(Tool): async def execute(self, **kwargs): for key in ("text", "message"): message = self.args.get(key) - if isinstance(message, str): + if isinstance(message, str) and message.strip(): return Response(message=message, break_loop=True) raise RepairableException( - "response tool requires a top-level text or message string argument" + "response tool requires a non-empty top-level text or message string argument" ) async def before_execution(self, **kwargs): diff --git a/tools/response.py.dox.md b/tools/response.py.dox.md index 2edcee1b9..43fb9fd2c 100644 --- a/tools/response.py.dox.md +++ b/tools/response.py.dox.md @@ -22,7 +22,8 @@ - Update this file whenever tool arguments, output shape, `break_loop` behavior, intervention handling, prompt instructions, or side effects change. - `ResponseTool` is a `Tool`. - `ResponseTool` defines `execute(...)`. -- `ResponseTool` requires a top-level string `text` or legacy `message` argument. +- `ResponseTool` requires a non-empty top-level string `text` or legacy `message` + argument, preferring `text` and falling back to `message` when `text` is blank. Invalid arguments raise `RepairableException` so the agent can surface a correction warning and retry rather than crash. - Imported dependency areas include: `helpers.errors`, `helpers.tool`.