Python: fix: parse Responses function_call_output so hosted tool results reach transports - #8078
Conversation
…each transports A hosted tool that executes server-side -- for example a Foundry Toolbox dispatching through its generic `call_tool` wrapper -- returns its result as a standalone `function_call_output` Responses item rather than on the originating call item. None of the three parse dispatch sites handled that item type, so it fell through to the `Unparsed ...` debug log and was discarded. No `Content.from_function_result` was produced, and AG-UI consequently emitted TOOL_CALL_END with no matching TOOL_CALL_RESULT, falling back to treating the call as declaration-only. The model still received the real output, so only the client lost the structured result. Add a `function_call_output` branch to all three sites -- the non-streaming `_parse_response_from_openai` and both streaming `response.output_item.added` / `.done` handlers -- sharing one `_parse_function_call_output_content` helper so the lists cannot drift again. `output` is a string or a list of input-content parts, so it is normalized through the existing `_stringify_mcp_output` rather than JSON-encoding provider models. The streaming handlers emit from whichever event first carries a populated `output` and record the item id in a per-request set, so the other event cannot produce a second result. Keyed on the item id rather than `call_id`, which the function-calling loop contract says must not be assumed unique forever. Reviewed against docs/specs/004-python-function-calling-loop.md, which covers provider serialization of function calls and results: no result is orphaned or duplicated, and the streaming and non-streaming paths agree. Fixes microsoft#8068
…parse overrides `RawFoundryChatClient` and `RawFoundryAgentChatClient` override `_parse_chunk_from_openai` to intercept oauth_consent items and then delegate to `RawOpenAIChatClient`. Both had the pre-change signature, so once the base started passing `seen_function_call_output_ids` every Foundry streaming call raised `TypeError: _parse_chunk_from_openai() got an unexpected keyword argument`. Accept and forward the new parameter in both overrides, and update the two delegation assertions that pin the forwarded argument list. Caught against a live Foundry Responses endpoint; `poe test -P foundry` also reproduces it, but that package is not in the validation command list in docs/specs/004-python-function-calling-loop.md even though it subclasses the OpenAI Responses client.
Review follow-up. `Content.from_function_result` does not validate `call_id`, so a `function_call_output` item carrying a blank one produced an orphaned result: transports drop it (`_emit_tool_result` returns early on a falsy `call_id`) and the outbound serializer would re-send it as an unpairable `function_call_output` input item on the next turn. These items are synthesized by the hosting layer, so a blank `call_id` is a realistic host-side defect rather than a theoretical one, and the function-calling loop contract requires that no result becomes orphaned. Extract the emission gate into `_function_call_output_has_result` so the populated-output and pairable-call_id checks are shared by all three dispatch sites instead of being repeated at each one.
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
There was a problem hiding this comment.
🟡 Changes recommended
Supported older SDK versions can crash, and rich output parts are not serialized into usable results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds parsing for hosted function_call_output items so tool results reach downstream transports.
Changes:
- Parses streaming and non-streaming function outputs.
- Deduplicates streaming results by item ID.
- Propagates parser state through Foundry clients and adds tests.
File summaries
| File | Description |
|---|---|
python/packages/openai/agent_framework_openai/_chat_client.py |
Implements output parsing and deduplication. |
python/packages/openai/tests/openai/test_openai_chat_client.py |
Tests parsing scenarios. |
python/packages/foundry/agent_framework_foundry/_chat_client.py |
Forwards deduplication state. |
python/packages/foundry/agent_framework_foundry/_agent.py |
Forwards deduplication state. |
python/packages/foundry/tests/foundry/test_foundry_chat_client.py |
Updates delegation assertion. |
python/packages/foundry/tests/foundry/test_foundry_agent.py |
Updates delegation assertion. |
Review details
Suppressed comments (1)
python/packages/openai/agent_framework_openai/_chat_client.py:2692
- For list output, the SDK supplies
ResponseInputText/ResponseInputImage/ResponseInputFilemodel instances, not the dictionaries used in the new test. Text happens to work via.text, but image/file parts fall through tojson.dumps(..., default=str), producing quoted Pydantic reprs (and concatenating multiple reprs) rather than preserving usable rich output. Map these provider parts toContentitems, or serialize the full list with_serialize_provider_payloadwhile retaining its boundaries before creating the function result.
return Content.from_function_result(
call_id=item.call_id,
result=self._stringify_mcp_output(item.output),
additional_properties=additional_properties,
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
… SDK floor Address review on two counts. `name` is not on `ResponseFunctionToolCallOutputItem` in openai 2.25.0, the declared floor -- that version ships only call_id/id/output/status/type. Reading it as an attribute raised `AttributeError` out of the shared parse helper, which all three dispatch sites call, so on any supported SDK below the release that added the field the whole response parse failed rather than merely dropping the result. Read it with `getattr`. The other attributes touched here (type/status/id/call_id/output) are all present on the floor, and the two module helpers already used `getattr`. `output` may also be a list of input-content parts. Passing those provider models straight to `_stringify_mcp_output` fell through to `json.dumps(..., default=str)` and embedded a Python repr in the result text sent back to the model -- e.g. `"ResponseInputImage(detail='auto', ...)"`. Dump each part first so text extraction still works and non-text parts serialize as readable JSON. Both paths are now regression-tested, including a stub item shaped like the 2.25.0 field set.
GitHub Actions
Active
GitHub Actions
Active
|
Reproduced end-to-end on real Foundry infrastructure, not just in unit tests, so the before/after is observable rather than inferred. Setup
The host emits the item as reported Matching AG-UI events, same request, same host, only the parser differing
On Two incidental notes from the exercise, neither part of this change
|
|
Flagging a process point on myself before anyone spends review time here.
Read together with "Any change to the function-calling loop or its approval/history/serialization paths", this PR is inside that scope — it changes how a provider serializer parses a function-call result — and I did not check first. That's on me. Rather than just flag it, here is where the change stands against the seven requirements in the same section, so you can judge cheaply whether it is worth continuing or whether you would rather own it:
The ask. On requirement 1 I would rather not guess at the matrix rows unilaterally, since that is exactly the judgment the clause reserves for you. My reading is that this touches "History and provider serialization" and nothing else, but I may be wrong about whether it also implicates the streaming/non-streaming agreement rows. So: would you prefer to
Happy with any of those. One note in case it affects the call: the underlying gap is not specific to a toolbox — |
| return Content.from_function_result( | ||
| call_id=item.call_id, | ||
| result=self._stringify_mcp_output(_plain_function_call_output(item.output)), | ||
| additional_properties=additional_properties, |
There was a problem hiding this comment.
Could we preserve list-shaped function_call_output.output as canonical Content.items instead of flattening it to text here? A valid input_image or input_file becomes JSON inside a text item, so AG-UI cannot expose the rich result and history replay sends a string instead of the supported content-part list. Could _parse_function_call_output_content map these parts to framework content before calling Content.from_function_result?
There was a problem hiding this comment.
Agreed, and from_function_result is explicit that the list is the intended shape — _types.py:899-909
documents list[Content] as "the canonical form" with result derived from the text items for
backwards compatibility. My current _plain_function_call_output -> _stringify_mcp_output chain
collapses every part into one text item, so an input_image arrives as JSON inside text and neither
AG-UI nor history replay can see it as a content part. An earlier review pass moved this from a Python
repr to JSON; you are pointing out that JSON-in-text is still the wrong representation, and that is
right.
The mapping I would write, mirroring the outbound direction that already exists at :1996-2041 and
:2164-2181:
input_text->Content.from_text(text)input_imagewithimage_url->Content.from_uri(uri, media_type=...), keepingdetailin
additional_propertiesinput_filewithfile_url->Content.from_uri; with base64file_data->Content.from_data
Then pass the resulting list[Content] to from_function_result and let it populate result from the
text items, so string-shaped output and existing consumers are unaffected.
One case I would rather not guess at: input_image/input_file carrying a file_id instead of inline
data. Content.from_hosted_file(file_id, media_type=..., name=...) looks like the right target, but a
hosted reference is only resolvable by the provider that issued it, so I do not know whether you want
it surfaced as hosted-file content or left as text. Which would you prefer?
I will also keep an unrecognised part type falling back to its dumped JSON as a text item rather than
dropping it, so a future part type degrades instead of disappearing.
Holding implementation on both of these until the contribution-ownership question you tagged
Eduard van Valkenburg (@eavanvalkenburg) on is settled, so nothing lands here unsanctioned. Both changes are small and I can
turn them around quickly once there is a direction.
There was a problem hiding this comment.
Could we keep _parse_chunk_from_openai compatible with existing overrides? Released agent-framework-foundry versions allow any agent-framework-openai<2 but implement the old signature, so upgrading OpenAI alone makes every streaming request fail with TypeError when _chat_client.py:812 passes seen_function_call_output_ids. Could the deduplication state stay behind the existing hook, or could the Foundry dependency floor be coordinated so this stable 1.x package combination cannot resolve?
There was a problem hiding this comment.
You're right, and the resolution path is exactly as you describe: packages/foundry/pyproject.toml:27
pins agent-framework-openai>=1.10.0,<2, so foundry 1.12.0 + a newer openai 1.x resolves cleanly and
then _chat_client.py:807-813 passes seen_function_call_output_ids= into an override that does not
accept it. Every streaming request raises TypeError. I hit this locally against the in-repo
subclasses and fixed those, which masked the released-wheel case from me.
On your second option, I do not think coordinating the Foundry floor can work here. The constraint
that permits the bad combination lives in wheels that are already published, so no change we make now
can retract it — a user on foundry 1.10/1.11/1.12 who upgrades only agent-framework-openai still
lands on the broken pairing. Raising the floor would protect future foundry releases and nothing
already released. Unless the fix is deferred to agent-framework-openai 2.x, where foundry's <2
bound excludes it by construction, the only shape that actually works is one that does not change the
signature at all.
Worth flagging against my own PR description: I justified this design by pointing at
seen_reasoning_delta_item_ids as precedent. That precedent has the same latent break. It was added
in 5e8fe0b (#5162), while foundry's floor was last touched in 5e056b6 (#4818), so it was never
coordinated either. So this is a pre-existing pattern rather than something my change introduces, and
my change would be the second instance. I should not have leaned on it as evidence the shape was safe.
Taking your first option, the shape I would propose is to keep the per-request dedup state off the
signature entirely by carrying it in the options dict that is already threaded through every call
site, under a private key. That preserves the order-insensitive behaviour the current tests pin while
leaving the signature byte-identical, so existing overrides keep working.
There is a simpler variant worth considering: emit only on response.output_item.done and drop the
dedup state altogether. I measured on live Foundry that both .added and .done fire for each
function_call_output item, so .done alone is sufficient for the reproduced case, and it is the
authoritative terminal event. I have not verified that .done always carries a populated output,
though — only that it fires — so I would want to confirm that on real traffic before betting the fix
on it rather than assume it.
Happy to implement either. Say which you prefer, or whether you would rather this waited for the
2.x boundary.
|
tagging Eduard van Valkenburg (@eavanvalkenburg) here to look at this. |
Motivation & Context
A Foundry hosted agent using a Foundry Toolbox dispatches its knowledge-base tool through the toolbox's generic
call_toolwrapper. The Toolbox executes the inner tool server-side andResponsesHostServerserializes the result as a standardfunction_call_outputResponses item. None of the three parse dispatch sites inagent_framework_openaihandled that item type, so it fell intocase _:/ the end of theelifchain, was logged asUnparsed event of type: ...at debug level, and discarded.No
Content.from_function_result(...)was produced, soagent_framework_ag_uiemittedTOOL_CALL_ENDwith no matchingTOOL_CALL_RESULTand its declaration-only fallback took over (_agent_run.py:3100-3108). The model still received the real output — only the client lost the structured result. The same tool called directly is anmcp_calland works, which is what isolates this to the client parser.Description & Review Guide
What are the major changes?
A
function_call_outputbranch at all three dispatch sites — the non-streaming_parse_response_from_openaiand both streamingresponse.output_item.added/.donehandlers — sharing one_parse_function_call_output_contenthelper and one_function_call_output_has_resultgate, so the three item-type lists cannot drift apart on this type again.outputisstr | list[ResponseInputText|Image|File], so it is normalized through the existing_stringify_mcp_outputrather than JSON-encoding provider models. That helper's name is MCP-flavoured but its logic is generic; happy to rename it if you would prefer (1 caller, 2 dedicated tests).The two streaming handlers emit from whichever event first carries a populated
output, recording the item id in a per-request set so the other cannot emit a second result. Keyed on the item id rather thancall_id, which the function-calling loop contract says must not be assumed unique forever.Two follow-up commits are separated deliberately so they can be read on their own: forwarding the new parameter through the two
RawFoundryChatClient/RawFoundryAgentChatClientoverrides, and rejecting a blankcall_idso no unpairable result is emitted.What is the impact of these changes?
Hosted-toolbox tool results now reach transports as
function_resultcontent, so AG-UI emits the fullTOOL_CALL_START→TOOL_CALL_END→TOOL_CALL_RESULTlifecycle. No AG-UI change was needed:_emit_tool_resultalready handlesfunction_result.Reviewed against
docs/specs/004-python-function-calling-loop.md, which covers provider serialization of function calls and results. No result is orphaned or duplicated, and the streaming and non-streaming paths agree. I have not edited the spec — its checklist asks that the matrix name a regression test for each affected scenario, and I would rather you decide whether this warrants a row than edit a cross-package contract in a bug fix. Glad to add one.Verified against a live Foundry Responses endpoint in addition to the unit tests. Two things that measurement settled:
.addedand.done(confirmed forfunction_call,message,reasoning). Emitting from both handlers without the seen-id set would have produced duplicate results; emitting from only one would have been a guess about which event carries the payload.function_call_outputis re-sent inline on the next turn underprevious_response_id, and the service accepts it. Since the outbound serializer emits onlycall_id/type/outputand no item id, a server-generated result is indistinguishable on the wire from a locally-executed one, so this needs no outbound companion change.What do you want reviewers to focus on?
Three things I could not settle myself, all raised deliberately rather than left for you to find:
seen_reasoning_delta_item_idswas added the same way and both Foundry overrides already carry it. If you would rather this state travelled in a per-request context object, that is a refactor of a spec-004 method and I would do it separately rather than inside a bug fix._parse_chunk_from_openaicounts as public API.RawOpenAIChatClientis exported and its docstring demonstrates subclassing, so an out-of-tree override with the old signature would break the same way Foundry did. I read the leading underscore as private and did not label this a breaking change — tell me if you disagree and I will relabel.packages/foundryis missing from spec 004's minimum validation commands (docs/specs/004-python-function-calling-loop.md), even though it subclasses the OpenAI Responses client the spec governs. That omission is exactly why my first sweep missed the regression above;uv run poe test -P foundryreproduces it. Worth adding for the next contributor.Separately, and not proposed here: the
case _:default at all three sites drops any unknown item type at debug severity, which is why the reporter hit three such types in one session (function_call_outputplus two SharePoint preview ones). Silently discarding a tool result is a correctness event rather than a diagnostic one. I deliberately kept this PR to the reproduced type — the SharePoint types are preview and I have no repro, so guessing their shape risks a wrong parser. If useful I will open a separate issue proposing either a shared dispatch table across the three sites or a warning for unknown*_outputitems.Related Issue
Fixes #8068
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.