diff --git a/agent/transports/chat_completions.py b/agent/transports/chat_completions.py index 275a66db3299e..88a5d2ea178ce 100644 --- a/agent/transports/chat_completions.py +++ b/agent/transports/chat_completions.py @@ -272,6 +272,25 @@ def convert_messages( break tool_calls = msg.get("tool_calls") if isinstance(tool_calls, list): + # Defense-in-depth: a strict OpenAI-compatible provider + # (e.g. onerouter / Qwen, DeepSeek v4) rejects an assistant + # message carrying ``tool_calls: []`` (empty array) with + # HTTP 400 "Empty tool_calls is not supported in message." + # The pre-API sanitizer in agent_runtime_helpers drops these, + # but only on the conversation_loop path — other routes can + # reach the wire without it. For every request that + # serializes through this transport (conversation loop and + # any caller using it), this is the last boundary, so + # normalize here. Requests built by fully separate payload + # paths (e.g. some auxiliary clients) never pass through + # this layer and are out of scope for it. (#58755 follow-up) + if ( + msg.get("role") == "assistant" + and "tool_calls" in msg + and not tool_calls + ): + needs_sanitize = True + break for tc in tool_calls: if isinstance(tc, dict) and ( "call_id" in tc @@ -282,6 +301,15 @@ def convert_messages( break if needs_sanitize: break + elif ( + isinstance(tool_calls, type(None)) + and msg.get("role") == "assistant" + and "tool_calls" in msg + ): + # Explicit ``tool_calls: null`` is equally invalid on strict + # providers — treat it like the empty-array case. + needs_sanitize = True + break if not needs_sanitize: return messages @@ -328,6 +356,19 @@ def mutable_msg() -> dict[str, Any]: tool_calls = msg.get("tool_calls") if isinstance(tool_calls, list): + # Strip empty/invalid tool_calls arrays at the transport + # layer (see detection above). Strict OpenAI-compatible + # providers reject ``tool_calls: []`` with HTTP 400; dropping + # the key keeps the message schema-valid. Matches the + # pre-API sanitizer's behaviour so all routes agree. + if ( + msg.get("role") == "assistant" + and "tool_calls" in msg + and not tool_calls + ): + out_msg = mutable_msg() + out_msg.pop("tool_calls", None) + continue copied_tool_calls: list[Any] | None = None for tc_idx, tc in enumerate(tool_calls): if isinstance(tc, dict): @@ -347,6 +388,14 @@ def mutable_msg() -> dict[str, Any]: copied_tool_calls[tc_idx] = copied_tc if copied_tool_calls is not None: mutable_msg()["tool_calls"] = copied_tool_calls + elif ( + isinstance(tool_calls, type(None)) + and msg.get("role") == "assistant" + and "tool_calls" in msg + ): + # Explicit ``tool_calls: null`` is invalid on strict + # providers — drop the key entirely. + mutable_msg().pop("tool_calls", None) return sanitized def convert_tools(self, tools: list[dict[str, Any]]) -> list[dict[str, Any]]: diff --git a/tests/agent/transports/test_chat_completions_empty_tool_calls.py b/tests/agent/transports/test_chat_completions_empty_tool_calls.py new file mode 100644 index 0000000000000..4f547d0e6a715 --- /dev/null +++ b/tests/agent/transports/test_chat_completions_empty_tool_calls.py @@ -0,0 +1,129 @@ +"""Tests for empty / null ``tool_calls`` stripping in ChatCompletionsTransport. + +Strict OpenAI-compatible providers (onerouter / Qwen, DeepSeek v4) reject an +assistant message carrying ``tool_calls: []`` (or ``null``) with HTTP 400 +"Empty tool_calls is not supported in message." The pre-API sanitizer in +``agent_runtime_helpers.sanitize_api_messages`` already drops these on the +conversation_loop path, but the transport layer must also normalize them so +auxiliary / custom-provider routes that bypass that sanitizer cannot reach the +wire with an invalid array. See #58755 (follow-up). +""" + +import pytest + +from agent.transports import get_transport + + +@pytest.fixture +def transport(): + import agent.transports.chat_completions # noqa: F401 + return get_transport("chat_completions") + + +class TestEmptyToolCallsStripping: + """Assistant messages with empty/invalid tool_calls must be normalized.""" + + def test_assistant_empty_list_dropped(self, transport): + msgs = [{"role": "assistant", "content": "ok", "tool_calls": []}] + out = transport.convert_messages(msgs) + assert "tool_calls" not in out[0] + assert out[0]["content"] == "ok" + + def test_assistant_null_dropped(self, transport): + msgs = [{"role": "assistant", "content": "ok", "tool_calls": None}] + out = transport.convert_messages(msgs) + assert "tool_calls" not in out[0] + + def test_assistant_real_calls_preserved(self, transport): + real_tc = [{ + "id": "call_abc", + "type": "function", + "function": {"name": "read_file", "arguments": "{}"}, + }] + msgs = [{"role": "assistant", "content": "c", "tool_calls": real_tc}] + out = transport.convert_messages(msgs) + assert out[0]["tool_calls"] == real_tc + + def test_only_empty_assistant_stripped_in_mixed_batch(self, transport): + msgs = [ + {"role": "user", "content": "hi"}, + {"role": "assistant", "content": "thinking", "tool_calls": []}, + { + "role": "assistant", + "content": "acting", + "tool_calls": [{ + "id": "call_x", + "type": "function", + "function": {"name": "f", "arguments": "{}"}, + }], + }, + ] + out = transport.convert_messages(msgs) + assert "tool_calls" not in out[1] + assert out[1]["content"] == "thinking" + assert out[2]["tool_calls"] and out[2]["tool_calls"][0]["id"] == "call_x" + + def test_user_role_empty_tool_calls_untouched(self, transport): + # User messages should not carry tool_calls at all, but if a stray + # empty array is present we must NOT strip it (it's not the invalid + # assistant shape, and mutating unrelated roles risks breaking the + # schema assumptions elsewhere). The transport only normalizes + # assistant messages. + msgs = [{"role": "user", "content": "hi", "tool_calls": []}] + out = transport.convert_messages(msgs) + assert "tool_calls" in out[0] + assert out[0]["tool_calls"] == [] + + def test_nonempty_array_codex_fields_stripped(self, transport): + # A non-empty tool_calls array carrying codex scaffolding markers + # (call_id, response_item_id) must have those fields stripped while + # the call itself is preserved. + msgs = [{ + "role": "assistant", + "content": "ok", + "tool_calls": [{ + "id": "fc_1", + "call_id": "call_1", + "response_item_id": "fc_1", + "type": "function", + "function": {"name": "f", "arguments": "{}"}, + }], + }] + out = transport.convert_messages(msgs, model="gpt-4o") + # Non-empty array: codex fields stripped, call preserved. + assert out[0]["tool_calls"] + tc = out[0]["tool_calls"][0] + assert "call_id" not in tc + assert "response_item_id" not in tc + + def test_clean_list_is_identity(self, transport): + msgs = [ + {"role": "user", "content": "hi"}, + { + "role": "assistant", + "content": "c", + "tool_calls": [{ + "id": "call_1", + "type": "function", + "function": {"name": "f", "arguments": "{}"}, + }], + }, + ] + assert transport.convert_messages(msgs) is msgs + + def test_empty_array_triggers_copy_on_write(self, transport): + msgs = [{"role": "assistant", "content": "ok", "tool_calls": []}] + out = transport.convert_messages(msgs) + # Original list/message must not be mutated in place. + assert msgs[0]["tool_calls"] == [] + assert "tool_calls" not in out[0] + assert out is not msgs + + @pytest.mark.parametrize( + "model", + ["qwen/qwen3.8-max-preview:free", "deepseek/deepseek-v4-flash", "gpt-4o"], + ) + def test_empty_array_stripped_across_providers(self, transport, model): + msgs = [{"role": "assistant", "content": "ok", "tool_calls": []}] + out = transport.convert_messages(msgs, model=model) + assert "tool_calls" not in out[0]