-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Python: fix: parse Responses function_call_output so hosted tool results reach transports
#8078
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
22180bb
83df081
4044d6e
a8dd5aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,7 @@ | |
| from openai.types.responses import ( | ||
| FunctionShellToolParam, | ||
| ResponseCustomToolCall, | ||
| ResponseFunctionToolCallOutputItem, | ||
| ResponseToolSearchCall, | ||
| response_create_params, | ||
| ) | ||
|
|
@@ -344,6 +345,63 @@ async def _open_event_stream(raw_response: Any) -> AsyncGenerator[Any]: | |
| yield raw_response | ||
|
|
||
|
|
||
| def _plain_function_call_output(output: Any) -> Any: | ||
| """Render provider content parts as plain data before stringifying a tool result. | ||
|
|
||
| ``function_call_output.output`` is either a string or a list of input-content parts | ||
| (text/image/file). Passing those provider models straight to | ||
| :meth:`_stringify_mcp_output` would fall through to ``json.dumps(..., default=str)`` and embed | ||
| a Python repr in the tool result. Dumping each part first keeps text extraction working and | ||
| turns non-text parts into readable JSON. | ||
| """ | ||
| if output is None or isinstance(output, str): | ||
| return output | ||
| if isinstance(output, Sequence) and not isinstance(output, (str, bytes, bytearray)): | ||
| entries = cast(Sequence[Any], output) | ||
| plain: list[Any] = [] | ||
| for entry in entries: | ||
| model_dump = getattr(entry, "model_dump", None) | ||
| plain.append(model_dump(exclude_none=True) if callable(model_dump) else entry) | ||
| return plain | ||
| return output | ||
|
|
||
|
|
||
| def _function_call_output_has_result(item: Any) -> bool: | ||
| """True when a ``function_call_output`` item carries a result that can be paired. | ||
|
|
||
| ``.added`` can precede a populated ``output``, so an in-progress skeleton must not synthesize | ||
| an empty result. A blank ``call_id`` is rejected as well: these items are synthesized by the | ||
| hosting layer, and a result that cannot be paired to its call is dropped by transports and, | ||
| worse, re-sent as an unpairable ``function_call_output`` input item on the next turn. | ||
| """ | ||
| if getattr(item, "output", None) is None: | ||
| return False | ||
| if not getattr(item, "call_id", None): | ||
| logger.debug("Skipping function_call_output with no call_id: item_id=%s", getattr(item, "id", None)) | ||
| return False | ||
| return True | ||
|
|
||
|
|
||
| def _claim_function_call_output(seen_item_ids: set[str] | None, item: Any) -> bool: | ||
| """Claim a ``function_call_output`` item for emission; return ``False`` if already claimed. | ||
|
|
||
| The Responses stream can surface the same output item on both ``response.output_item.added`` | ||
| and ``response.output_item.done``. Whichever event first carries a populated ``output`` emits | ||
| the result, and this test-and-set keeps the other from producing a duplicate one. Keyed on the | ||
| item id rather than ``call_id``, which is not guaranteed to be unique forever. When no set is | ||
| supplied the item is always claimable, so a single event parsed on its own still yields output. | ||
| """ | ||
| if seen_item_ids is None: | ||
| return True | ||
| item_id = getattr(item, "id", None) | ||
| if not isinstance(item_id, str) or not item_id: | ||
| return True | ||
| if item_id in seen_item_ids: | ||
| return False | ||
| seen_item_ids.add(item_id) | ||
| return True | ||
|
|
||
|
|
||
| def _annotations_to_output_text(annotations: Sequence[Annotation] | None) -> list[dict[str, Any]]: | ||
| """Convert framework `Annotation` objects to Responses API `output_text` annotation dicts. | ||
|
|
||
|
|
@@ -712,6 +770,7 @@ def _inner_get_response( | |
| if stream: | ||
| function_call_ids: dict[int, tuple[str, str]] = {} | ||
| seen_reasoning_delta_item_ids: set[str] = set() | ||
| seen_function_call_output_ids: set[str] = set() | ||
| validated_options: dict[str, Any] | None = None | ||
| # Captured once request options are validated/prepared so the streaming finalizer can | ||
| # still parse the aggregated response into structured output after the stream completes. | ||
|
|
@@ -750,6 +809,7 @@ async def _stream() -> AsyncIterable[ChatResponseUpdate]: | |
| options=validated_options, | ||
| function_call_ids=function_call_ids, | ||
| seen_reasoning_delta_item_ids=seen_reasoning_delta_item_ids, | ||
| seen_function_call_output_ids=seen_function_call_output_ids, | ||
| ) | ||
| if served_model is not None: | ||
| update.model = served_model | ||
|
|
@@ -780,6 +840,7 @@ async def _stream() -> AsyncIterable[ChatResponseUpdate]: | |
| options=validated_options, | ||
| function_call_ids=function_call_ids, | ||
| seen_reasoning_delta_item_ids=seen_reasoning_delta_item_ids, | ||
| seen_function_call_output_ids=seen_function_call_output_ids, | ||
| ) | ||
| else: | ||
| raw_create_response = await client.responses.with_raw_response.create( | ||
|
|
@@ -794,6 +855,7 @@ async def _stream() -> AsyncIterable[ChatResponseUpdate]: | |
| options=validated_options, | ||
| function_call_ids=function_call_ids, | ||
| seen_reasoning_delta_item_ids=seen_reasoning_delta_item_ids, | ||
| seen_function_call_output_ids=seen_function_call_output_ids, | ||
| ) | ||
| if served_model is not None: | ||
| update.model = served_model | ||
|
|
@@ -2628,6 +2690,32 @@ def _parse_hosted_function_call_content( | |
| raw_representation=item, | ||
| ) | ||
|
|
||
| def _parse_function_call_output_content(self, item: ResponseFunctionToolCallOutputItem) -> Content: | ||
| """Create function result content for a Responses ``function_call_output`` item. | ||
|
|
||
| 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`` item rather than on the originating call item. Parsing it keeps | ||
| the call/result pair intact for transports such as AG-UI, which otherwise sees a tool call | ||
| with no result and falls back to treating it as declaration-only (issue #8068). | ||
|
|
||
| ``output`` is either a string or a list of input-content parts, so it is normalized through | ||
| :meth:`_stringify_mcp_output` rather than JSON-encoding provider models. | ||
| """ | ||
| additional_properties: dict[str, Any] = {"item_type": item.type, "status": item.status} | ||
| if item.id: | ||
| additional_properties["item_id"] = item.id | ||
| # `name` (and the other caller-attribution fields) only exist on newer openai SDKs; the | ||
| # declared floor of 2.25.0 ships only call_id/id/output/status/type. | ||
| if tool_name := getattr(item, "name", None): | ||
| additional_properties["name"] = tool_name | ||
| 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, | ||
|
Comment on lines
+2712
to
+2715
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we preserve list-shaped
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, and The mapping I would write, mirroring the outbound direction that already exists at
Then pass the resulting One case I would rather not guess at: I will also keep an unrecognised part type falling back to its dumped JSON as a text item rather than Holding implementation on both of these until the contribution-ownership question you tagged |
||
| raw_representation=item, | ||
| ) | ||
|
|
||
| # region Parse methods | ||
| def _get_finish_reason_from_openai_response(self, response: Any) -> FinishReason | None: | ||
| """Get the framework finish reason from a terminal Responses API response.""" | ||
|
|
@@ -2855,6 +2943,9 @@ def _parse_response_from_openai( | |
| raw_representation=item, | ||
| ) | ||
| ) | ||
| case "function_call_output": # ResponseFunctionToolCallOutputItem | ||
| if _function_call_output_has_result(item): | ||
| contents.append(self._parse_function_call_output_content(item)) | ||
| case "custom_tool_call": | ||
| contents.append( | ||
| self._parse_hosted_function_call_content(item, name=item.name, arguments=item.input) | ||
|
|
@@ -2966,6 +3057,7 @@ def _parse_chunk_from_openai( | |
| options: dict[str, Any], | ||
| function_call_ids: dict[int, tuple[str, str]], | ||
| seen_reasoning_delta_item_ids: set[str] | None = None, | ||
| seen_function_call_output_ids: set[str] | None = None, | ||
| ) -> ChatResponseUpdate: | ||
| """Parse an OpenAI Responses API streaming event into a ChatResponseUpdate.""" | ||
| metadata: dict[str, Any] = {} | ||
|
|
@@ -3330,6 +3422,14 @@ def output_text_properties(output: Any) -> dict[str, Any] | None: | |
| ) | ||
| case "web_search_call" | "file_search_call": | ||
| contents.append(self._parse_search_tool_call_content(event_item)) | ||
| case "function_call_output": # ResponseFunctionToolCallOutputItem | ||
| # Emitted from whichever of `.added` / `.done` first carries a populated | ||
| # `output`; the item id is recorded so the other event cannot emit a second | ||
| # result for the same item (issue #8068). | ||
| if _function_call_output_has_result(event_item) and _claim_function_call_output( | ||
| seen_function_call_output_ids, event_item | ||
| ): | ||
| contents.append(self._parse_function_call_output_content(event_item)) | ||
| case _: | ||
| if getattr(event_item, "type", None) != _AZURE_AI_SEARCH_CALL_OUTPUT_TYPE: | ||
| logger.debug("Unparsed event of type: %s: %s", event.type, event) | ||
|
|
@@ -3534,6 +3634,18 @@ def _get_ann_value(key: str) -> Any: | |
| arguments=tool_search_call.arguments, | ||
| ) | ||
| ) | ||
| elif getattr(done_item, "type", None) == "function_call_output": | ||
| # Counterpart to the `response.output_item.added` branch: whichever event first | ||
| # carries a populated `output` emits the result, and the shared seen-id set | ||
| # keeps the other from duplicating it (issue #8068). | ||
| if _function_call_output_has_result(done_item) and _claim_function_call_output( | ||
| seen_function_call_output_ids, done_item | ||
| ): | ||
| contents.append( | ||
| self._parse_function_call_output_content( | ||
| cast(ResponseFunctionToolCallOutputItem, done_item) | ||
| ) | ||
| ) | ||
| elif getattr(done_item, "type", None) == _AZURE_AI_SEARCH_CALL_OUTPUT_TYPE: | ||
| pass | ||
| case _: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we keep
_parse_chunk_from_openaicompatible with existing overrides? Releasedagent-framework-foundryversions allow anyagent-framework-openai<2but implement the old signature, so upgrading OpenAI alone makes every streaming request fail withTypeErrorwhen_chat_client.py:812passesseen_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, and the resolution path is exactly as you describe:
packages/foundry/pyproject.toml:27pins
agent-framework-openai>=1.10.0,<2, so foundry 1.12.0 + a newer openai 1.x resolves cleanly andthen
_chat_client.py:807-813passesseen_function_call_output_ids=into an override that does notaccept it. Every streaming request raises
TypeError. I hit this locally against the in-reposubclasses 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-openaistilllands on the broken pairing. Raising the floor would protect future foundry releases and nothing
already released. Unless the fix is deferred to
agent-framework-openai2.x, where foundry's<2bound 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_idsas precedent. That precedent has the same latent break. It was addedin 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
optionsdict that is already threaded through every callsite, 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.doneand drop thededup state altogether. I measured on live Foundry that both
.addedand.donefire for eachfunction_call_outputitem, so.donealone is sufficient for the reproduced case, and it is theauthoritative terminal event. I have not verified that
.donealways carries a populatedoutput,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.