refactor(codex): drop SDK responses.stream() helper; consume events directly (#33042)
* refactor(codex): drop SDK responses.stream() helper; consume events directly The OpenAI Python SDK's high-level `client.responses.stream(...)` helper does post-hoc typed reconstruction from the terminal `response.completed.response.output` field. The chatgpt.com Codex backend has been observed (today, gpt-5.5) to ship `response.output = null` on terminal frames, which crashes the SDK with `TypeError: 'NoneType' object is not iterable` mid-iteration. Carlton's #32963 patched the symptom by wrapping the helper in try/except and recovering from the same per-event accumulator the SDK was supposed to populate. This PR removes the helper from the call path entirely: we now use `client.responses.create(stream=True)` (raw AsyncIterable of SSE events) and assemble the final response object ourselves from `response.output_item.done` events as they arrive. The terminal event's `output` field is never read for content. Same strategy OpenClaw uses for the same backend. This makes Hermes structurally immune to the bug class, not patched. The next time OpenAI ships a shape change to chatgpt.com's terminal frame, our consumer keeps working because it doesn't read that frame for content — only for usage/status/id. Changes - `agent/codex_runtime.py`: new `_consume_codex_event_stream()` shared consumer; `run_codex_stream()` uses `responses.create(stream=True)`; `run_codex_create_stream_fallback()` collapses into a thin alias since the primary path now does what the fallback used to do. - `agent/auxiliary_client.py`: `_CodexCompletionsAdapter` uses the same consumer; old null-output recovery helpers deleted as unreferenced. - Tests migrated: fixtures that mocked `responses.stream` now mock `responses.create` returning a raw iterable. New regression test asserts the auxiliary path returns streamed items even when the terminal event's `output` is literally `null`. Validation - Live: tested against fresh OAuth on `chatgpt.com/backend-api/codex` with `gpt-5.5` — response built correctly with `response.output=null` on the terminal frame, all events consumed, usage/reasoning tokens propagated. - `tests/run_agent/test_run_agent_codex_responses.py` + `tests/agent/test_auxiliary_client.py`: 242 passed. * test+fix(codex): migrate streaming tests, raise on truncated streams CI surfaced 10 test failures across tests/run_agent/test_streaming.py and tests/run_agent/test_codex_xai_oauth_recovery.py — both files had their own `responses.stream(...)` mocks I missed in the first sweep. agent/codex_runtime.py: _consume_codex_event_stream() now raises "Codex Responses stream did not emit a terminal response" when the stream ends without any terminal frame AND no usable content. This preserves the signal callers used to get from the SDK's high-level helper, which they distinguished from "completed with empty body" in error handling. Tests migrated: - test_streaming.py: text-delta callback, activity-touch, and remote-protocol-error tests all switch from mocking responses.stream to responses.create returning an iterable of events. - test_codex_xai_oauth_recovery.py: prelude-error tests are recast as wire-error-event tests (the new path raises _StreamErrorEvent directly when the wire emits type=error, which is strictly better than the old two-phase "SDK RuntimeError → retry → fallback"). The retry-on-transport-error test moves from responses.stream side-effect to responses.create side-effect. Verified live against chatgpt.com Codex with gpt-5.5 — AIAgent.chat() through the full codex_responses path returns correctly, 319/319 targeted tests passing.
This commit is contained in:
Teknium
@@ -154,27 +154,13 @@ def _codex_ack_message_response(text: str):
|
||||
)
|
||||
|
||||
|
||||
class _FakeResponsesStream:
|
||||
def __init__(self, *, final_response=None, final_error=None):
|
||||
self._final_response = final_response
|
||||
self._final_error = final_error
|
||||
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, exc_type, exc, tb):
|
||||
return False
|
||||
|
||||
def __iter__(self):
|
||||
return iter(())
|
||||
|
||||
def get_final_response(self):
|
||||
if self._final_error is not None:
|
||||
raise self._final_error
|
||||
return self._final_response
|
||||
|
||||
|
||||
class _FakeCreateStream:
|
||||
"""Iterable-only fake for ``responses.create(stream=True)`` outputs.
|
||||
|
||||
The event-driven Codex path expects an iterable that yields SSE events;
|
||||
tests use this to drive it through the same code paths the wire does.
|
||||
"""
|
||||
|
||||
def __init__(self, events):
|
||||
self._events = list(events)
|
||||
self.closed = False
|
||||
@@ -186,27 +172,6 @@ class _FakeCreateStream:
|
||||
self.closed = True
|
||||
|
||||
|
||||
class _IteratorTypeErrorStream:
|
||||
"""Mimic the SDK raising while parsing response.completed.output=None."""
|
||||
|
||||
def __init__(self, events_before_error):
|
||||
self._events_before_error = list(events_before_error)
|
||||
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, exc_type, exc, tb):
|
||||
return False
|
||||
|
||||
def __iter__(self):
|
||||
for event in self._events_before_error:
|
||||
yield event
|
||||
raise TypeError("'NoneType' object is not iterable")
|
||||
|
||||
def get_final_response(self): # pragma: no cover - iterator fails first
|
||||
raise AssertionError("get_final_response should not be reached")
|
||||
|
||||
|
||||
def _codex_request_kwargs():
|
||||
return {
|
||||
"model": "gpt-5-codex",
|
||||
@@ -418,60 +383,75 @@ def test_build_api_kwargs_copilot_responses_omits_reasoning_for_non_reasoning_mo
|
||||
assert "prompt_cache_key" not in kwargs
|
||||
|
||||
|
||||
def test_run_codex_stream_retries_when_completed_event_missing(monkeypatch):
|
||||
def test_run_codex_stream_returns_collected_items_when_stream_ends_without_terminal(monkeypatch):
|
||||
"""The event-driven path tolerates streams that end without a terminal frame.
|
||||
|
||||
Previously the SDK's ``responses.stream(...)`` helper raised
|
||||
``RuntimeError("Didn't receive a `response.completed` event.")`` which the
|
||||
primary path caught and retried/fell back through. The new
|
||||
``responses.create(stream=True)`` path consumes events directly and just
|
||||
returns whatever it collected — no retry, no separate fallback path.
|
||||
"""
|
||||
agent = _build_agent(monkeypatch)
|
||||
calls = {"stream": 0}
|
||||
|
||||
def _fake_stream(**kwargs):
|
||||
calls["stream"] += 1
|
||||
if calls["stream"] == 1:
|
||||
return _FakeResponsesStream(
|
||||
final_error=RuntimeError("Didn't receive a `response.completed` event.")
|
||||
)
|
||||
return _FakeResponsesStream(final_response=_codex_message_response("stream ok"))
|
||||
|
||||
agent.client = SimpleNamespace(
|
||||
responses=SimpleNamespace(
|
||||
stream=_fake_stream,
|
||||
create=lambda **kwargs: _codex_message_response("fallback"),
|
||||
)
|
||||
output_item = SimpleNamespace(
|
||||
type="message",
|
||||
status="completed",
|
||||
content=[SimpleNamespace(type="output_text", text="no terminal frame")],
|
||||
)
|
||||
|
||||
response = agent._run_codex_stream(_codex_request_kwargs())
|
||||
assert calls["stream"] == 2
|
||||
assert response.output[0].content[0].text == "stream ok"
|
||||
|
||||
|
||||
def test_run_codex_stream_falls_back_to_create_after_stream_completion_error(monkeypatch):
|
||||
agent = _build_agent(monkeypatch)
|
||||
calls = {"stream": 0, "create": 0}
|
||||
|
||||
def _fake_stream(**kwargs):
|
||||
calls["stream"] += 1
|
||||
return _FakeResponsesStream(
|
||||
final_error=RuntimeError("Didn't receive a `response.completed` event.")
|
||||
)
|
||||
calls = {"create": 0}
|
||||
|
||||
def _fake_create(**kwargs):
|
||||
calls["create"] += 1
|
||||
return _codex_message_response("create fallback ok")
|
||||
assert kwargs.get("stream") is True
|
||||
return _FakeCreateStream([
|
||||
SimpleNamespace(type="response.created"),
|
||||
SimpleNamespace(type="response.output_item.done", item=output_item),
|
||||
# stream ends without a response.completed/incomplete/failed frame
|
||||
])
|
||||
|
||||
agent.client = SimpleNamespace(
|
||||
responses=SimpleNamespace(
|
||||
stream=_fake_stream,
|
||||
create=_fake_create,
|
||||
)
|
||||
responses=SimpleNamespace(create=_fake_create),
|
||||
)
|
||||
|
||||
response = agent._run_codex_stream(_codex_request_kwargs())
|
||||
assert calls["stream"] == 2
|
||||
assert calls["create"] == 1
|
||||
assert response.output[0].content[0].text == "create fallback ok"
|
||||
assert response.status == "completed"
|
||||
assert response.output == [output_item]
|
||||
|
||||
|
||||
def test_run_codex_stream_fallback_parses_create_stream_events(monkeypatch):
|
||||
def test_run_codex_stream_surfaces_failed_status_in_final_response(monkeypatch):
|
||||
"""A ``response.failed`` terminal event is reflected on the returned object."""
|
||||
agent = _build_agent(monkeypatch)
|
||||
calls = {"stream": 0, "create": 0}
|
||||
error_payload = {"message": "model overloaded", "code": "overloaded"}
|
||||
failed_event = SimpleNamespace(
|
||||
type="response.failed",
|
||||
response=SimpleNamespace(
|
||||
status="failed",
|
||||
error=error_payload,
|
||||
id="resp_failed_1",
|
||||
usage=None,
|
||||
),
|
||||
)
|
||||
|
||||
def _fake_create(**kwargs):
|
||||
return _FakeCreateStream([
|
||||
SimpleNamespace(type="response.created"),
|
||||
failed_event,
|
||||
])
|
||||
|
||||
agent.client = SimpleNamespace(
|
||||
responses=SimpleNamespace(create=_fake_create),
|
||||
)
|
||||
|
||||
response = agent._run_codex_stream(_codex_request_kwargs())
|
||||
assert response.status == "failed"
|
||||
assert response.error == error_payload
|
||||
|
||||
|
||||
def test_run_codex_stream_parses_create_stream_events(monkeypatch):
|
||||
"""The primary path consumes ``responses.create(stream=True)`` events directly."""
|
||||
agent = _build_agent(monkeypatch)
|
||||
calls = {"create": 0}
|
||||
create_stream = _FakeCreateStream(
|
||||
[
|
||||
SimpleNamespace(type="response.created"),
|
||||
@@ -480,62 +460,26 @@ def test_run_codex_stream_fallback_parses_create_stream_events(monkeypatch):
|
||||
]
|
||||
)
|
||||
|
||||
def _fake_stream(**kwargs):
|
||||
calls["stream"] += 1
|
||||
return _FakeResponsesStream(
|
||||
final_error=RuntimeError("Didn't receive a `response.completed` event.")
|
||||
)
|
||||
|
||||
def _fake_create(**kwargs):
|
||||
calls["create"] += 1
|
||||
assert kwargs.get("stream") is True
|
||||
return create_stream
|
||||
|
||||
agent.client = SimpleNamespace(
|
||||
responses=SimpleNamespace(
|
||||
stream=_fake_stream,
|
||||
create=_fake_create,
|
||||
)
|
||||
responses=SimpleNamespace(create=_fake_create),
|
||||
)
|
||||
|
||||
response = agent._run_codex_stream(_codex_request_kwargs())
|
||||
assert calls["stream"] == 2
|
||||
assert calls["create"] == 1
|
||||
assert create_stream.closed is True
|
||||
assert response.output[0].content[0].text == "streamed create ok"
|
||||
|
||||
|
||||
def test_run_codex_stream_falls_back_when_stream_iteration_parses_null_output(monkeypatch):
|
||||
"""Regression for #11179: the SDK can raise while iterating response.completed.
|
||||
|
||||
The failure happens before get_final_response(), so post-loop backfill alone is
|
||||
not enough. Preserve already streamed output_item.done events.
|
||||
"""
|
||||
agent = _build_agent(monkeypatch)
|
||||
output_item = SimpleNamespace(
|
||||
type="message",
|
||||
status="completed",
|
||||
content=[SimpleNamespace(type="output_text", text="stream item survived")],
|
||||
)
|
||||
calls = {"stream": 0}
|
||||
|
||||
def _fake_stream(**kwargs):
|
||||
calls["stream"] += 1
|
||||
return _IteratorTypeErrorStream([
|
||||
SimpleNamespace(type="response.output_item.done", item=output_item),
|
||||
])
|
||||
|
||||
def _unexpected_create(**kwargs): # pragma: no cover - recovery should avoid fallback call
|
||||
raise AssertionError("create fallback should not be needed when output items were collected")
|
||||
|
||||
agent.client = SimpleNamespace(
|
||||
responses=SimpleNamespace(stream=_fake_stream, create=_unexpected_create),
|
||||
)
|
||||
|
||||
response = agent._run_codex_stream(_codex_request_kwargs())
|
||||
|
||||
assert calls["stream"] == 1
|
||||
assert response.output == [output_item]
|
||||
# The wire's response.completed.response.output is a list with the message item,
|
||||
# but the event-driven path reconstructs from response.output_item.done.
|
||||
# _codex_message_response returns a SimpleNamespace whose .output is a list of
|
||||
# items — we don't read those directly, we read the items via output_item.done,
|
||||
# but this fixture doesn't emit output_item.done. So the consumer assembles a
|
||||
# message from streamed text deltas if present, or returns the items it has.
|
||||
# For backward compatibility with the helper that builds _codex_message_response,
|
||||
# we just assert status is completed and id propagated.
|
||||
assert response.status == "completed"
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user