fix: keep message id/status for Responses API round-trip#1754
fix: keep message id/status for Responses API round-trip#1754enyst wants to merge 2 commits intoOpenHands:mainfrom
Conversation
When a reasoning item is present in the conversation history, OpenAI's Responses API requires the following message item to have 'id' and 'status' fields. Without these, the API returns: "Item of type 'reasoning' was provided without its required following item." Changes: - Add responses_message_id and responses_message_status fields to Message - Capture message id/status in from_llm_responses_output() - Emit id/status in to_responses_dict() when available - Add tests for the new behavior Co-authored-by: openhands <openhands@all-hands.dev> Co-authored-by: smolpaws <engel@enyst.org>
|
Validating this. It's weird because it started recently. |
all-hands-bot
left a comment
There was a problem hiding this comment.
The PR correctly implements message id/status preservation for OpenAI Responses API round-trips. However, there are a few areas that could be improved: missing test coverage for deserialization, potential type safety improvements, and clarifications around atomic field handling.
| "content": content_items, | ||
| } | ||
| # Include id/status if available (required with reasoning item) | ||
| if self.responses_message_id: |
There was a problem hiding this comment.
🟡 Suggestion: The id and status fields are checked independently, which means it's possible to include one without the other in the output. According to the PR description, OpenAI requires both fields when a reasoning item is present.
Consider whether these should be set atomically (both or neither) with a single condition: if self.responses_message_id and self.responses_message_status:
There was a problem hiding this comment.
@OpenHands if one or both don't exist, but a reasoning item exists, let's NOT send back either of these two nor the "orphan" reasoning item.
There was a problem hiding this comment.
I'm on it! enyst can track my progress at all-hands.dev
There was a problem hiding this comment.
Final summary (since my last update)
What changed
-
Updated
Message.to_responses_dict()(assistant role) so that when aresponses_reasoning_itemis present but eitherresponses_message_idorresponses_message_statusis missing, we emit neither:- the assistant
"message"item, nor - the
"reasoning"item
This prevents sending an “orphan” reasoning item and matches the PR comment request.
- the assistant
-
Also ensured that
idandstatusare only ever emitted on the assistant"message"item atomically (both or neither).
Checklist against your request
- If one or both of (
responses_message_id,responses_message_status) don’t exist and a reasoning item exists, do not send back either of these two nor the reasoning item. - Changes are minimal and localized (single file change).
- Tests run and passing (
uv run pytest -q tests/sdk/llm/test_message.py).
Repo state
- Working tree clean; no extra diffs.
- Changes already committed and pushed to
origin/fix/responses-reasoning-message-id(commit49fe542c).
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
@OpenHands please address the inline review threads as you see fit. Note that it's an AI reviewer, so maybe it hallucinated or something. Then, if you fixed them, mark as solved. I recommend to use Then, run the reasoning example script from examples/ , with gpt-5-nano or so, some gpt-5 from the eval proxy but not a large model. Add more steps to the script, e.g. make it write a file, edit it later, then insert a line at the end, then delete the file, all should be done by the model using tool calls. Tell it something like that. Meanwhile, you should make sure telemetry / logging are enabled, and set the directory in your workspace, in project directory, subdir The eval proxy has a base URL you can find in this codebase somewhere, search for it. Use litellm key. When the experiment is done, write directly on github here, in PR, as a comment, what you did and what were the results. My main questions are:
|
|
I'm on it! enyst can track my progress at all-hands.dev |
What I did1) Addressed inline review threads
Then I marked the relevant review threads as resolved. 2) Ran the reasoning/tool-use example with a small model via eval proxy
3) Telemetry / logging output
Results / Notes from the tool-use run
Answering the two key questionsQ1) Did we really need this PR’s fix, or was it a false alarm?From what the SDK logs and the Responses telemetry show: when a So this doesn’t look like a hallucinated / false alarm requirement; it’s a real constraint of Responses input formatting for reasoning items. Q2) If needed, does it work well?
The full behavior (capture -> store -> re-emit) is now covered end-to-end at unit level. |
Final summary (new work since the PR comment request)Checklist against your request
Code changes I made (concise + relevant)
Extraneous changes check
Push / PR updates
|
When a reasoning item is present in the conversation history, OpenAI's Responses API requires the following message item to have 'id' and 'status' fields. Without these, the API returns:
"Item of type 'reasoning' was provided without its required following item."
Changes:
Checklist