From fc47b7285c4a2b5cd3ea4bb01ebbb9a7f04693e1 Mon Sep 17 00:00:00 2001 From: xxxigm Date: Wed, 27 May 2026 11:33:38 -0700 Subject: [PATCH] fix(codex): omit tools key from Codex Responses kwargs when no tools registered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Salvages the transport-side fix from #32911 (@xxxigm). Closes #32892. The openai SDK's responses.stream() / responses.parse() eagerly call _make_tools(tools), which iterates tools without a None guard. Passing tools=None raises TypeError: 'NoneType' object is not iterable before any HTTP request is issued (openai==2.24.0). PR #33042 already removed responses.stream() from our own Codex call paths, so the specific iteration crash inside _make_tools is no longer on the hot path. But the right API contract is to omit tools entirely when there are no functions to expose — passing tools=None to the backend is semantically wrong regardless of the SDK's iteration behavior, and we'd hit it again on any future code path that hasn't migrated off responses.stream(). This applies the transport-level part of @xxxigm's fix: move 'tools': response_tools into the if response_tools: branch so the key is omitted when there are no tools, just like tool_choice and parallel_tool_calls already are. Skips the run_agent.py-side _strip_sdk_none_iterables helper from their PR — that path is now obsolete because the SDK helper that needed defending is gone. Tests - tests/run_agent/test_codex_no_tools_nonetype.py: 6 tests trimmed from @xxxigm's original 13-test file. Drops the obsolete tests for _strip_sdk_none_iterables and _RecordingResponsesStream (helpers that don't exist on main anymore), keeps the transport behavior tests + the SDK contract sanity check that ensures we notice if upstream ever fixes _make_tools(None). - 6/6 passing locally. Co-authored-by: xxxigm --- agent/transports/codex.py | 10 +- .../run_agent/test_codex_no_tools_nonetype.py | 179 ++++++++++++++++++ 2 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 tests/run_agent/test_codex_no_tools_nonetype.py diff --git a/agent/transports/codex.py b/agent/transports/codex.py index 9a4a3a4b937..ab82f6202f1 100644 --- a/agent/transports/codex.py +++ b/agent/transports/codex.py @@ -128,6 +128,14 @@ class ResponsesApiTransport(ProviderTransport): reasoning_effort = _effort_clamp.get(reasoning_effort, reasoning_effort) response_tools = _responses_tools(tools) + # ``tools`` MUST be omitted entirely when there are no functions to + # expose: the openai SDK's ``responses.stream()`` / ``responses.parse()`` + # eagerly call ``_make_tools(tools)`` which does ``for tool in tools`` + # without a None guard, so passing ``tools=None`` raises + # ``TypeError: 'NoneType' object is not iterable`` before any HTTP + # request is issued (openai==2.24.0). Reported for the + # ``openai-codex`` / ``gpt-5.5`` combo on chatgpt.com/backend-api/codex + # (#32892) when the agent runs without external tools registered. kwargs = { "model": model, "instructions": instructions, @@ -137,10 +145,10 @@ class ResponsesApiTransport(ProviderTransport): replay_encrypted_reasoning=replay_encrypted_reasoning, current_issuer_kind=issuer_kind, ), - "tools": response_tools, "store": False, } if response_tools: + kwargs["tools"] = response_tools kwargs["tool_choice"] = "auto" kwargs["parallel_tool_calls"] = True diff --git a/tests/run_agent/test_codex_no_tools_nonetype.py b/tests/run_agent/test_codex_no_tools_nonetype.py new file mode 100644 index 00000000000..d7980e8f02e --- /dev/null +++ b/tests/run_agent/test_codex_no_tools_nonetype.py @@ -0,0 +1,179 @@ +"""Regression coverage for #32892. + +The openai SDK's ``responses.stream()`` / ``responses.parse()`` eagerly +call ``_make_tools(tools)``, which iterates ``tools`` *without* a None +guard. Passing ``tools=None`` therefore raises:: + + TypeError: 'NoneType' object is not iterable + +…before any HTTP request is issued. This trips the +``openai-codex`` / ``gpt-5.5`` combo on ``chatgpt.com/backend-api/codex`` +whenever the user runs Hermes without external tools registered: the +agent loop catches the TypeError, sees no HTTP status, classifies it as +non-retryable, and aborts (#32892). + +These tests pin the defence: +:func:`agent.transports.codex.ResponsesApiTransport.build_kwargs` must +never emit ``tools=None`` — only add the ``tools`` key when there are +function tools to expose. When there are no tools, the entire ``tools`` +key (plus ``tool_choice`` and ``parallel_tool_calls`` which are +meaningless without it) is omitted from the kwargs. + +Note: #33042 separately removed the SDK's ``responses.stream()`` helper +from our own Codex call paths, so the specific iteration crash inside +``_make_tools`` is also structurally avoided in normal operation. This +test class additionally pins the SDK's ``_make_tools(None)`` contract so +we notice if upstream ever changes it. +""" +from __future__ import annotations + +import sys +import types +from types import SimpleNamespace +from typing import Any, Dict, List + +import pytest + + +# Stub optional deps the parent module imports at top level — keeps this +# test file runnable in the same environment as the existing Codex tests. +sys.modules.setdefault("fire", types.SimpleNamespace(Fire=lambda *a, **k: None)) +sys.modules.setdefault("firecrawl", types.SimpleNamespace(Firecrawl=object)) +sys.modules.setdefault("fal_client", types.SimpleNamespace()) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +@pytest.fixture +def transport(): + """Fresh ``ResponsesApiTransport`` per test (it is stateless but + the import has side-effects on a global transport registry).""" + from agent.transports.codex import ResponsesApiTransport + + return ResponsesApiTransport() + + +@pytest.fixture +def codex_messages() -> List[Dict[str, Any]]: + """Minimal Codex-shaped chat history mirroring the #32892 reproducer: + one system + one short user message, with no tool calls in history.""" + return [ + {"role": "system", "content": "You are Hermes."}, + {"role": "user", "content": "Hey! What can I help you with?"}, + ] + + +def _build_kwargs_no_tools(transport, messages) -> Dict[str, Any]: + """Exercise the real ``build_kwargs`` for the codex backend with no tools.""" + return transport.build_kwargs( + model="gpt-5.5", + messages=messages, + tools=None, + is_codex_backend=True, + ) + + +# --------------------------------------------------------------------------- +# build_kwargs: the "tools=None" key must never appear +# --------------------------------------------------------------------------- + + +def test_build_kwargs_omits_tools_key_when_no_tools(transport, codex_messages): + """``build_kwargs`` must not place ``tools=None`` in the outgoing dict. + + Putting ``tools=None`` reaches ``responses.stream()`` which calls + ``_make_tools(None)`` and crashes with the #32892 TypeError before any + request is sent. + """ + kwargs = _build_kwargs_no_tools(transport, codex_messages) + + assert "tools" not in kwargs, ( + f"tools key must be omitted entirely when no tools are registered, " + f"got kwargs={sorted(kwargs)}" + ) + + +def test_build_kwargs_omits_tool_choice_and_parallel_when_no_tools(transport, codex_messages): + """``tool_choice`` / ``parallel_tool_calls`` are meaningless without + tools — and some backends 400 on them. Confirm we never set them.""" + kwargs = _build_kwargs_no_tools(transport, codex_messages) + + assert "tool_choice" not in kwargs + assert "parallel_tool_calls" not in kwargs + + +def test_build_kwargs_keeps_required_codex_fields_without_tools(transport, codex_messages): + """The toolless build must still emit the non-negotiable Codex fields + (model / instructions / input / store) — otherwise we'd just be moving + the bug from the SDK to preflight.""" + kwargs = _build_kwargs_no_tools(transport, codex_messages) + + assert kwargs["model"] == "gpt-5.5" + assert kwargs["instructions"] == "You are Hermes." + assert kwargs["store"] is False + assert isinstance(kwargs["input"], list) + assert kwargs["input"] and kwargs["input"][0]["role"] == "user" + + +def test_build_kwargs_emits_tools_when_tools_present(transport, codex_messages): + """Sanity check the inverse: when tools ARE provided, they MUST appear + in the outgoing kwargs along with the related ``tool_choice`` / + ``parallel_tool_calls`` switches.""" + tools = [ + { + "type": "function", + "function": { + "name": "terminal", + "description": "Run a shell command.", + "parameters": {"type": "object", "properties": {}}, + }, + } + ] + + kwargs = transport.build_kwargs( + model="gpt-5.5", + messages=codex_messages, + tools=tools, + is_codex_backend=True, + ) + + assert "tools" in kwargs and kwargs["tools"], "tools must be present when registered" + assert kwargs["tools"][0]["name"] == "terminal" + assert kwargs["tool_choice"] == "auto" + assert kwargs["parallel_tool_calls"] is True + + +def test_build_kwargs_drops_empty_tools_list(transport, codex_messages): + """``tools=[]`` collapses to ``None`` inside ``_responses_tools`` — + the resulting kwargs must therefore also omit the key.""" + kwargs = transport.build_kwargs( + model="gpt-5.5", + messages=codex_messages, + tools=[], + is_codex_backend=True, + ) + + assert "tools" not in kwargs + assert "tool_choice" not in kwargs + assert "parallel_tool_calls" not in kwargs + + +# --------------------------------------------------------------------------- +# --------------------------------------------------------------------------- + + +def test_openai_sdk_raises_typeerror_on_tools_none(): + """Document the upstream behaviour the two defences guard against. + + If the SDK ever fixes ``_make_tools(None)`` to return ``omit`` + gracefully, this test will start failing — at which point the agent + defences become belt-only and this test should be flipped to an + ``xfail`` so we notice the upstream change. + """ + from openai.resources.responses.responses import _make_tools + + with pytest.raises(TypeError, match="NoneType.*not iterable"): + _make_tools(None) \ No newline at end of file