fixes responses api tool call serialization - #2483
Open
ieaves wants to merge 3 commits into
Open
Conversation
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
ieaves
requested review from
bmahabirbu,
cgruver,
engelmi,
jhjaggars,
maxamillion,
mikebonnet,
olliewalsh,
rhatdan and
swarajpande5
as code owners
March 3, 2026 19:20
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the serialization process for messages sent to the Responses API, specifically addressing how tool calls and their outputs are structured. The changes ensure that the generated payload correctly represents tool interactions as distinct, flattened entries, improving compatibility and correctness with the API's requirements. Additionally, a payload key related to token limits was updated to match the latest API specification. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Contributor
Reviewer's GuideRefactors OpenAI Responses API message serialization to emit a flat list of input items per message, correctly modeling tool calls and outputs according to the Responses schema, and renames the max token option to max_output_tokens. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the serialization logic for the responses API, primarily by flattening tool calls into individual records and adjusting the data structure for function_call and function_call_output types. The changes are logical and the tests have been updated accordingly. I have one suggestion to improve consistency in handling message metadata.
Comment on lines
+218
to
+226
| items.extend( | ||
| { | ||
| "id": call.id, | ||
| "type": "function", | ||
| "function": { | ||
| "name": call.name, | ||
| "arguments": json.dumps(call.arguments, ensure_ascii=False), | ||
| }, | ||
| "type": "function_call", | ||
| "call_id": call.id, | ||
| "name": call.name, | ||
| "arguments": json.dumps(call.arguments, ensure_ascii=False), | ||
| } | ||
| for call in message.tool_calls | ||
| ) |
Contributor
There was a problem hiding this comment.
The generated items for tool calls are missing the metadata from the original AssistantMessage. Other message types and the content part of AssistantMessage include this metadata. This inconsistency might lead to issues if downstream consumers expect metadata to be present on all items. It seems like **message.metadata should be added to the dictionary created for each tool call to ensure consistency.
Suggested change
| items.extend( | |
| { | |
| "id": call.id, | |
| "type": "function", | |
| "function": { | |
| "name": call.name, | |
| "arguments": json.dumps(call.arguments, ensure_ascii=False), | |
| }, | |
| "type": "function_call", | |
| "call_id": call.id, | |
| "name": call.name, | |
| "arguments": json.dumps(call.arguments, ensure_ascii=False), | |
| } | |
| for call in message.tool_calls | |
| ) | |
| items.extend( | |
| { | |
| **message.metadata, | |
| "type": "function_call", | |
| "call_id": call.id, | |
| "name": call.name, | |
| "arguments": json.dumps(call.arguments, ensure_ascii=False), | |
| } | |
| for call in message.tool_calls | |
| ) |
Collaborator
Author
There was a problem hiding this comment.
The function_call schema is specific. I don't think blindly adding message.metadata would be a good idea.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
message_to_responses_itemsforAssistantMessage, the generatedfunction_callitems no longer includemessage.metadatawhereas other items do; consider consistently propagating metadata to function call records if downstream consumers rely on those fields for tracing or correlation. - The
ToolMessageserializer now sends"output": message.textwithout defaulting to an empty string; if the Responses API or your consumers expect a string instead ofnull, you may want to mirror the previous behavior and coerceNoneto"".
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `message_to_responses_items` for `AssistantMessage`, the generated `function_call` items no longer include `message.metadata` whereas other items do; consider consistently propagating metadata to function call records if downstream consumers rely on those fields for tracing or correlation.
- The `ToolMessage` serializer now sends `"output": message.text` without defaulting to an empty string; if the Responses API or your consumers expect a string instead of `null`, you may want to mirror the previous behavior and coerce `None` to `""`.
## Individual Comments
### Comment 1
<location path="ramalama/chat_providers/openai.py" line_range="187-196" />
<code_context>
+ return items
+
+
+@message_to_responses_items.register
+def _(message: ToolMessage) -> list[dict[str, Any]]:
+ if not message.tool_call_id:
+ raise ValueError("Responses tool output requires tool_call_id (mapped to call_id).")
</code_context>
<issue_to_address>
**issue (bug_risk):** Raising on missing tool_call_id changes previous behavior and may break existing ToolMessage usage.
This change turns previously valid `ToolMessage` instances (without `tool_call_id`) into runtime `ValueError`s, which is a breaking behavioral change. If stricter validation is needed, consider enforcing it at creation time, or handling missing IDs more gracefully here (e.g., skipping or mapping them to a generic output) to avoid unexpected failures in existing callers.
</issue_to_address>
### Comment 2
<location path="ramalama/chat_providers/openai.py" line_range="236-246" />
<code_context>
+ if not message.tool_call_id:
+ raise ValueError("Responses tool output requires tool_call_id (mapped to call_id).")
+
+ return [
+ {
+ **message.metadata,
+ "type": "function_call_output",
+ "call_id": message.tool_call_id,
+ "output": message.text,
+ }
]
</code_context>
<issue_to_address>
**suggestion:** ToolMessage output can be None; consider normalizing to a string for API compatibility.
Here `output` is set directly to `message.text`, so a `None` value will serialize as `null`, unlike other message types where missing text is coerced to `""`. If the Responses API expects a string (or at least non-null) for `output`, this could cause inconsistent behavior. Consider matching `create_responses_content` and using something like `message.text or ""` for a predictable payload shape.
```suggestion
if not message.tool_call_id:
raise ValueError("Responses tool output requires tool_call_id (mapped to call_id).")
return [
{
**message.metadata,
"type": "function_call_output",
"call_id": message.tool_call_id,
"output": message.text or "",
}
]
```
</issue_to_address>
### Comment 3
<location path="test/unit/providers/test_openai_provider.py" line_range="96-98" />
<code_context>
assert serialized[1]["type"] == "image_url"
assert serialized[1]["image_url"] == {"url": "http://img", "detail": "high"}
- assert payload["max_completion_tokens"] == 128
+ assert payload["max_output_tokens"] == 128
assert "max_tokens" not in payload
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that the old `max_completion_tokens` key is no longer present in the payload.
Please also assert that the legacy `max_completion_tokens` key is absent from the payload (e.g., `assert "max_completion_tokens" not in payload`) so the rename behavior is fully covered and regressions where both keys are set are caught.
```suggestion
assert serialized[1]["image_url"] == {"url": "http://img", "detail": "high"}
assert payload["max_output_tokens"] == 128
assert "max_tokens" not in payload
assert "max_completion_tokens" not in payload
```
</issue_to_address>
### Comment 4
<location path="test/unit/providers/test_openai_provider.py" line_range="100" />
<code_context>
tool_reply = ToolMessage(text="Clear skies", tool_call_id="call-9")
payload = self.provider.build_payload([assistant, tool_reply], make_options())
- first_input = payload["input"][0]
+ function_call = next(item for item in payload["input"] if item.get("type") == "function_call")
+ function_call_output = next(item for item in payload["input"] if item.get("type") == "function_call_output")
- assert first_input["tool_calls"][0]["function"]["name"] == "lookup"
</code_context>
<issue_to_address>
**suggestion (testing):** Extend tool-call serialization test to assert the number and ordering of flattened items.
Using `next(...)` confirms these entries exist and are shaped correctly, but not that flattening/ordering behave as intended. To better cover the serialization, please also assert:
- The expected length of `payload["input"]` for this scenario.
- The relative order of the assistant message item vs. the `function_call` and `function_call_output` items.
This will help catch regressions where items are reordered or unexpected records are inserted.
```suggestion
def test_streaming_emits_delta_and_completion_events(self):
tool_reply = ToolMessage(text="Clear skies", tool_call_id="call-9")
payload = self.provider.build_payload([assistant, tool_reply], make_options())
inputs = payload["input"]
# Assert expected length of flattened items
assert len(inputs) == 3
# Assert relative ordering: assistant message, then function_call, then function_call_output
assistant_item, function_call, function_call_output = inputs
assert assistant_item.get("role") == "assistant"
assert function_call.get("type") == "function_call"
assert function_call_output.get("type") == "function_call_output"
assert function_call["name"] == "lookup"
assert function_call["arguments"] == '{"city": "NYC"}'
assert function_call_output["call_id"] == "call-9"
```
</issue_to_address>
### Comment 5
<location path="test/unit/providers/test_openai_provider.py" line_range="120-124" />
<code_context>
payload = self.provider.build_payload([assistant, tool_reply], make_options())
- first_input = payload["input"][0]
+ function_call = next(item for item in payload["input"] if item.get("type") == "function_call")
+ function_call_output = next(item for item in payload["input"] if item.get("type") == "function_call_output")
- assert first_input["tool_calls"][0]["function"]["name"] == "lookup"
- assert first_input["tool_calls"][0]["function"]["arguments"] == '{"city": "NYC"}'
- assert payload["input"][1]["tool_call_id"] == "call-9"
+ assert function_call["name"] == "lookup"
+ assert function_call["arguments"] == '{"city": "NYC"}'
+ assert function_call_output["call_id"] == "call-9"
def test_streaming_emits_done_event_for_done_marker(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test covering multiple tool calls in a single assistant message.
The serializer is intended to flatten multiple `assistant.tool_calls` into separate `function_call` items, but this test only exercises a single call. Please add a case where `assistant.tool_calls` has multiple entries and assert that:
- multiple `function_call` items are produced,
- each has the expected `call_id`, `name`, and `arguments`,
- all calls are present in the payload.
This will help detect issues where only the first call is serialized correctly or later calls are malformed.
Suggested implementation:
```python
def test_streaming_emits_delta_and_completion_events(self):
tool_reply = ToolMessage(text="Clear skies", tool_call_id="call-9")
payload = self.provider.build_payload([assistant, tool_reply], make_options())
function_call = next(item for item in payload["input"] if item.get("type") == "function_call")
function_call_output = next(item for item in payload["input"] if item.get("type") == "function_call_output")
assert function_call["name"] == "lookup"
assert function_call["arguments"] == '{"city": "NYC"}'
assert function_call_output["call_id"] == "call-9"
def test_streaming_emits_multiple_tool_calls(self):
# Assistant message with multiple tool calls that should be flattened
multi_tool_assistant = AssistantMessage(
tool_calls=[
{
"id": "call-9",
"type": "function",
"function": {
"name": "lookup",
"arguments": '{"city": "NYC"}',
},
},
{
"id": "call-10",
"type": "function",
"function": {
"name": "lookup",
"arguments": '{"city": "SF"}',
},
},
]
)
tool_reply_1 = ToolMessage(text="Clear skies", tool_call_id="call-9")
tool_reply_2 = ToolMessage(text="Foggy", tool_call_id="call-10")
payload = self.provider.build_payload(
[multi_tool_assistant, tool_reply_1, tool_reply_2],
make_options(),
)
function_calls = [
item for item in payload["input"] if item.get("type") == "function_call"
]
# Multiple `function_call` items are produced
assert len(function_calls) == 2
# Index by call_id for easier assertions
calls_by_id = {call["call_id"]: call for call in function_calls}
# All calls are present in the payload
assert set(calls_by_id.keys()) == {"call-9", "call-10"}
# Each call has the expected name and arguments
assert calls_by_id["call-9"]["name"] == "lookup"
assert calls_by_id["call-9"]["arguments"] == '{"city": "NYC"}'
assert calls_by_id["call-10"]["name"] == "lookup"
assert calls_by_id["call-10"]["arguments"] == '{"city": "SF"}'
def test_streaming_emits_done_event_for_done_marker(self):
events = list(self.provider.parse_stream_chunk(b"data: [DONE]\n\n"))
```
Depending on the existing test utilities and message classes in this file, you might need to:
1. Ensure `AssistantMessage` is imported from the correct module at the top of `test_openai_provider.py` (or adjust `multi_tool_assistant` construction to use whatever assistant/Chat message class or helper factory is already used in this test file).
2. If the provider does not set a `"call_id"` field on `function_call` items and instead uses a different key (e.g. `"tool_call_id"`), update the `calls_by_id` indexing to use that key so the assertions align with the actual payload schema.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert payload["max_output_tokens"] == 128 | ||
| assert "max_tokens" not in payload | ||
|
|
||
| def test_streaming_emits_delta_and_completion_events(self): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Extend tool-call serialization test to assert the number and ordering of flattened items.
Using next(...) confirms these entries exist and are shaped correctly, but not that flattening/ordering behave as intended. To better cover the serialization, please also assert:
- The expected length of
payload["input"]for this scenario. - The relative order of the assistant message item vs. the
function_callandfunction_call_outputitems.
This will help catch regressions where items are reordered or unexpected records are inserted.
Suggested change
| def test_streaming_emits_delta_and_completion_events(self): | |
| def test_streaming_emits_delta_and_completion_events(self): | |
| tool_reply = ToolMessage(text="Clear skies", tool_call_id="call-9") | |
| payload = self.provider.build_payload([assistant, tool_reply], make_options()) | |
| inputs = payload["input"] | |
| # Assert expected length of flattened items | |
| assert len(inputs) == 3 | |
| # Assert relative ordering: assistant message, then function_call, then function_call_output | |
| assistant_item, function_call, function_call_output = inputs | |
| assert assistant_item.get("role") == "assistant" | |
| assert function_call.get("type") == "function_call" | |
| assert function_call_output.get("type") == "function_call_output" | |
| assert function_call["name"] == "lookup" | |
| assert function_call["arguments"] == '{"city": "NYC"}' | |
| assert function_call_output["call_id"] == "call-9" |
Comment on lines
+120
to
+124
| function_call_output = next(item for item in payload["input"] if item.get("type") == "function_call_output") | ||
|
|
||
| assert first_input["tool_calls"][0]["function"]["name"] == "lookup" | ||
| assert first_input["tool_calls"][0]["function"]["arguments"] == '{"city": "NYC"}' | ||
| assert payload["input"][1]["tool_call_id"] == "call-9" | ||
| assert function_call["name"] == "lookup" | ||
| assert function_call["arguments"] == '{"city": "NYC"}' | ||
| assert function_call_output["call_id"] == "call-9" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test covering multiple tool calls in a single assistant message.
The serializer is intended to flatten multiple assistant.tool_calls into separate function_call items, but this test only exercises a single call. Please add a case where assistant.tool_calls has multiple entries and assert that:
- multiple
function_callitems are produced, - each has the expected
call_id,name, andarguments, - all calls are present in the payload.
This will help detect issues where only the first call is serialized correctly or later calls are malformed.
Suggested implementation:
def test_streaming_emits_delta_and_completion_events(self):
tool_reply = ToolMessage(text="Clear skies", tool_call_id="call-9")
payload = self.provider.build_payload([assistant, tool_reply], make_options())
function_call = next(item for item in payload["input"] if item.get("type") == "function_call")
function_call_output = next(item for item in payload["input"] if item.get("type") == "function_call_output")
assert function_call["name"] == "lookup"
assert function_call["arguments"] == '{"city": "NYC"}'
assert function_call_output["call_id"] == "call-9"
def test_streaming_emits_multiple_tool_calls(self):
# Assistant message with multiple tool calls that should be flattened
multi_tool_assistant = AssistantMessage(
tool_calls=[
{
"id": "call-9",
"type": "function",
"function": {
"name": "lookup",
"arguments": '{"city": "NYC"}',
},
},
{
"id": "call-10",
"type": "function",
"function": {
"name": "lookup",
"arguments": '{"city": "SF"}',
},
},
]
)
tool_reply_1 = ToolMessage(text="Clear skies", tool_call_id="call-9")
tool_reply_2 = ToolMessage(text="Foggy", tool_call_id="call-10")
payload = self.provider.build_payload(
[multi_tool_assistant, tool_reply_1, tool_reply_2],
make_options(),
)
function_calls = [
item for item in payload["input"] if item.get("type") == "function_call"
]
# Multiple `function_call` items are produced
assert len(function_calls) == 2
# Index by call_id for easier assertions
calls_by_id = {call["call_id"]: call for call in function_calls}
# All calls are present in the payload
assert set(calls_by_id.keys()) == {"call-9", "call-10"}
# Each call has the expected name and arguments
assert calls_by_id["call-9"]["name"] == "lookup"
assert calls_by_id["call-9"]["arguments"] == '{"city": "NYC"}'
assert calls_by_id["call-10"]["name"] == "lookup"
assert calls_by_id["call-10"]["arguments"] == '{"city": "SF"}'
def test_streaming_emits_done_event_for_done_marker(self):
events = list(self.provider.parse_stream_chunk(b"data: [DONE]\n\n"))Depending on the existing test utilities and message classes in this file, you might need to:
- Ensure
AssistantMessageis imported from the correct module at the top oftest_openai_provider.py(or adjustmulti_tool_assistantconstruction to use whatever assistant/Chat message class or helper factory is already used in this test file). - If the provider does not set a
"call_id"field onfunction_callitems and instead uses a different key (e.g."tool_call_id"), update thecalls_by_idindexing to use that key so the assertions align with the actual payload schema.
Signed-off-by: Ian Eaves <ian.k.eaves@gmail.com>
Collaborator
|
@ieaves is this fix still required? Needs a rebase/merge from main |
Collaborator
Author
|
Sort of up to you. If the plan is not to support multiple API types there's not much point in investing more time into this PR. |
|
A friendly reminder that this PR had no activity for 30 days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes the serialization logic for the responses API: flattening tool calls into individual records and shifting records under the
function_callandfunction_call_outputtypes.Summary by Sourcery
Fix Responses API payload serialization to emit separate input items for assistant tool calls and align token limit naming with the Responses schema.
Bug Fixes:
Tests: