fix: guard parse_response against null output#3317
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34094e2021
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| output_list: List[ParsedResponseOutputItem[TextFormatT]] = [] | ||
|
|
||
| for output in response.output: | ||
| for output in (response.output or []): |
There was a problem hiding this comment.
Preserve streamed output when final response omits it
When a response.completed event has response.output == null after prior streaming output events, this fallback makes parse_response succeed by returning output=[], and ResponseStreamState.accumulate_event stores that as _completed_response instead of the populated stream snapshot. In that exact null-final-output streaming path, get_final_response() and the emitted response.completed.response silently lose all text/tool output that was already accumulated from earlier events; the guard should preserve or parse the existing snapshot rather than replacing it with an empty output list.
Useful? React with 👍 / 👎.
|
Thanks for the quick fix. This is a duplicate of the earlier upstream fix PR: This PR applies the same I compared the diffs and did not see an additional behavior change here that needs to be pulled into #3315. |
|
Update: the automated Codex review here identified a useful stream-state data-loss risk. I pulled that into the earlier PR #3315. #3315 now preserves the accumulated So this PR remains a duplicate of #3315, but the useful review finding from this thread is now incorporated there. |
|
Closing this as a duplicate of #3315, which already covers the same defensive For maintainers comparing approaches, Hermes also now has a layer-side recovery for the same backend shape at: |
This is a minimal defensive guard for
parse_response()when a terminal Responses object arrives withresponse.output=None.Change:
for output in response.output:for output in (response.output or []):Why:
TypeError: 'NoneType' object is not iterable