Review — independent read of current head

• Head reviewed: e4cdf2e4a01a79c92f4ba72c4ff1c631dfd6bb9d
• Scope: static review of the diff (4 files, +305/−99) against issue [Bug]: Responses stream crashes when terminal response.output is null #11179. I did not check out the branch or run the test suite locally; test results below are from the PR description, not re-executed.
• Mergeability: branch currently reports CONFLICTING / DIRTY against main — a rebase/merge of main is needed before this can land.
• CI: no checks reported on the branch.

The fix is well-targeted and the issue write-up is clear. A few things worth addressing:

High (please verify) — main stream path now bypasses get_final_response() for all providers

Previously _run_codex_stream only handled response.incomplete/response.failed in-loop and always fell through to stream.get_final_response(). It now returns inside the loop on the first response.completed carrying a non-None response:

elif event_type in {"response.completed", "response.incomplete", "response.failed"}:
    resp_obj = getattr(event, "response", None)
    ...
    if resp_obj is not None:
        return self._backfill_responses_stream_output(resp_obj, ...)

This removes the SDK's get_final_response() / parse_response() post-processing from the success path, not just the null-output case — so the blast radius is every request through this path. Could you confirm nothing downstream relied on SDK final-parse behavior the raw terminal-event response doesn't already provide (e.g. structured-output/text_format parsing, typed-object normalization, usage accounting)? The auxiliary and create_stream_fallback paths already returned the event response directly, so there's precedent, but this is new behavior for the main path.

Medium — the get_final_response() TypeError-recovery branch isn't covered by the new tests

Both new regressions emit a response.completed event whose response is a non-None object with output=None. That takes the in-loop return path and never calls get_final_response() — the final_error=TypeError(...) on the fake stream only acts as a guard ("terminal event should be used before final parsing"). So the central try/except recovery:

except TypeError as exc:
    if "NoneType" not in err_text or "not iterable" not in err_text:
        raise
    ...
    final_response = SimpleNamespace(status="completed", model=self.model, output=[], usage=None)

has no test that actually triggers it. Suggest adding a case where the terminal event's response is None (or absent) and get_final_response() raises the TypeError, so the recovery path is exercised end-to-end.

Low

• Brittle error matching: recovery keys off "NoneType" ... "not iterable" substrings in the exception message, which can shift across Python/SDK versions. Reasonable as a narrowing guard, but a comment noting the dependency would help future maintainers.
• Duplicate helpers: _backfill_responses_output (auxiliary_client.py) and _backfill_responses_stream_output (run_agent.py) are nearly identical; only the latter handles model/usage. Fine across modules for now, but they'll drift.
• Type hint: model: str = None should be Optional[str] = None.

Nice

Adding has_tool_calls detection in _run_codex_create_stream_fallback is a real correctness improvement — the old fallback could collapse a function_call response into a synthesized plain-text message, and the shared helper now guards that with elif collected_text_deltas and not has_tool_calls.

Conclusion: not a formal approval — merge conflict must be resolved, and I'd like the High item confirmed and the Medium test gap closed. None of these are hard blockers to the core fix's correctness for the documented null-output scenario.

Superseded by #32963 (cherry-picked from @carltonawong's PR #32890, which ported your fix shape onto current main and added iterator-time regression coverage for the failure shape that broke the Codex backend today). Thanks for the original fix @richard950825-sys — your work is what the salvage was based on. Closes #11179.