From 1dba779f760c4712f07b3a2ff76e18559b1e3ad1 Mon Sep 17 00:00:00 2001 From: Yuge Zhang Date: Thu, 18 Sep 2025 12:13:30 +0800 Subject: [PATCH] Python: Enhance tool call handling and function result processing (#790) * Tool call enhancement * add tests * fix comments * resolve comments --------- Co-authored-by: Chris <66376200+crickman@users.noreply.github.com> Co-authored-by: Eric Zhu --- .../agent_framework/openai/_chat_client.py | 11 +- .../main/agent_framework/openai/_shared.py | 29 ++- .../tests/openai/test_openai_chat_client.py | 190 ++++++++++++++++++ 3 files changed, 216 insertions(+), 14 deletions(-) diff --git a/python/packages/main/agent_framework/openai/_chat_client.py b/python/packages/main/agent_framework/openai/_chat_client.py index 90776964be..1a4c63e5fe 100644 --- a/python/packages/main/agent_framework/openai/_chat_client.py +++ b/python/packages/main/agent_framework/openai/_chat_client.py @@ -185,10 +185,10 @@ class OpenAIBaseChatClient(OpenAIBase, BaseChatClient): if choice.finish_reason: finish_reason = FinishReason(value=choice.finish_reason) contents: list[Contents] = [] - if parsed_tool_calls := [tool for tool in self._get_tool_calls_from_chat_choice(choice)]: - contents.extend(parsed_tool_calls) if text_content := self._parse_text_from_choice(choice): contents.append(text_content) + if parsed_tool_calls := [tool for tool in self._get_tool_calls_from_chat_choice(choice)]: + contents.extend(parsed_tool_calls) messages.append(ChatMessage(role="assistant", contents=contents)) return ChatResponse( response_id=response.id, @@ -354,8 +354,13 @@ class OpenAIBaseChatClient(OpenAIBase, BaseChatClient): args["tool_calls"] = [self._openai_content_parser(content)] # type: ignore case FunctionResultContent(): args["tool_call_id"] = content.call_id - if content.result: + if content.result is not None: args["content"] = prepare_function_call_results(content.result) + elif content.exception is not None: + # Send the exception message to the model + # Otherwise we won't have any channels to talk to OpenAI + # TODO(yuge): This should ideally be customizable + args["content"] = "Error: " + str(content.exception) case _: if "content" not in args: args["content"] = [] diff --git a/python/packages/main/agent_framework/openai/_shared.py b/python/packages/main/agent_framework/openai/_shared.py index 9d5ab7e52f..0a62af4b93 100644 --- a/python/packages/main/agent_framework/openai/_shared.py +++ b/python/packages/main/agent_framework/openai/_shared.py @@ -50,21 +50,28 @@ __all__ = [ ] +def _prepare_function_call_results_as_dumpable(content: Contents | Any | list[Contents | Any]) -> Any: + if isinstance(content, list): + # Particularly deal with lists of BaseModel + return [_prepare_function_call_results_as_dumpable(item) for item in content] + if isinstance(content, dict): + return {k: _prepare_function_call_results_as_dumpable(v) for k, v in content.items()} + if isinstance(content, BaseModel): + return content.model_dump(exclude={"raw_representation", "additional_properties"}) + return content + + def prepare_function_call_results(content: Contents | Any | list[Contents | Any]) -> str | list[str]: """Prepare the values of the function call results.""" - if isinstance(content, list): - results: list[str] = [] - for item in content: - res = prepare_function_call_results(item) - if isinstance(res, list): - results.extend(res) - else: - results.append(res) - return results[0] if len(results) == 1 else json.dumps(results) if isinstance(content, BaseModel): - return content.model_dump_json(exclude_none=True, exclude={"raw_representation", "additional_properties"}) + # BaseModel is already dumpable, shortcut for performance + return content.model_dump_json(exclude={"raw_representation", "additional_properties"}) + + dumpable = _prepare_function_call_results_as_dumpable(content) + if isinstance(dumpable, str): + return dumpable # fallback - return json.dumps(content) + return json.dumps(dumpable) class OpenAISettings(AFBaseSettings): diff --git a/python/packages/main/tests/openai/test_openai_chat_client.py b/python/packages/main/tests/openai/test_openai_chat_client.py index dded958f23..3316058a74 100644 --- a/python/packages/main/tests/openai/test_openai_chat_client.py +++ b/python/packages/main/tests/openai/test_openai_chat_client.py @@ -1,11 +1,14 @@ # Copyright (c) Microsoft. All rights reserved. +import json import os +from datetime import datetime from typing import Annotated from unittest.mock import MagicMock, patch import pytest from openai import BadRequestError +from pydantic import BaseModel from agent_framework import ( AgentRunResponse, @@ -17,6 +20,7 @@ from agent_framework import ( ChatResponse, ChatResponseUpdate, DataContent, + FunctionResultContent, HostedWebSearchTool, TextContent, ToolProtocol, @@ -25,6 +29,7 @@ from agent_framework import ( from agent_framework.exceptions import ServiceInitializationError, ServiceResponseException from agent_framework.openai import OpenAIChatClient from agent_framework.openai._exceptions import OpenAIContentFilterException +from agent_framework.openai._shared import prepare_function_call_results skip_if_openai_integration_tests_disabled = pytest.mark.skipif( os.getenv("RUN_INTEGRATION_TESTS", "false").lower() != "true" @@ -592,6 +597,191 @@ async def test_exception_message_includes_original_error_details() -> None: assert original_error_message in exception_message +def test_chat_response_content_order_text_before_tool_calls(openai_unit_test_env: dict[str, str]): + """Test that text content appears before tool calls in ChatResponse contents.""" + # Import locally to avoid break other tests when the import changes + from openai.types.chat.chat_completion import ChatCompletion, Choice + from openai.types.chat.chat_completion_message import ChatCompletionMessage + from openai.types.chat.chat_completion_message_tool_call import ChatCompletionMessageToolCall, Function + + # Create a mock OpenAI response with both text and tool calls + mock_response = ChatCompletion( + id="test-response", + object="chat.completion", + created=1234567890, + model="gpt-4o-mini", + choices=[ + Choice( + index=0, + message=ChatCompletionMessage( + role="assistant", + content="I'll help you with that calculation.", + tool_calls=[ + ChatCompletionMessageToolCall( + id="call-123", + type="function", + function=Function(name="calculate", arguments='{"x": 5, "y": 3}'), + ) + ], + ), + finish_reason="tool_calls", + ) + ], + ) + + client = OpenAIChatClient() + response = client._create_chat_response(mock_response, ChatOptions()) + + # Verify we have both text and tool call content + assert len(response.messages) == 1 + message = response.messages[0] + assert len(message.contents) == 2 + + # Verify text content comes first, tool call comes second + assert message.contents[0].type == "text" + assert message.contents[0].text == "I'll help you with that calculation." + assert message.contents[1].type == "function_call" + assert message.contents[1].name == "calculate" + + +def test_function_result_falsy_values_handling(openai_unit_test_env: dict[str, str]): + """Test that falsy values (like empty list) in function result are properly handled.""" + client = OpenAIChatClient() + + # Test with empty list (falsy but not None) + message_with_empty_list = ChatMessage(role="tool", contents=[FunctionResultContent(call_id="call-123", result=[])]) + + openai_messages = client._openai_chat_message_parser(message_with_empty_list) + assert len(openai_messages) == 1 + assert openai_messages[0]["content"] == "[]" # Empty list should be JSON serialized + + # Test with empty string (falsy but not None) + message_with_empty_string = ChatMessage( + role="tool", contents=[FunctionResultContent(call_id="call-456", result="")] + ) + + openai_messages = client._openai_chat_message_parser(message_with_empty_string) + assert len(openai_messages) == 1 + assert openai_messages[0]["content"] == "" # Empty string should be preserved + + # Test with False (falsy but not None) + message_with_false = ChatMessage(role="tool", contents=[FunctionResultContent(call_id="call-789", result=False)]) + + openai_messages = client._openai_chat_message_parser(message_with_false) + assert len(openai_messages) == 1 + assert openai_messages[0]["content"] == "false" # False should be JSON serialized + + +def test_function_result_exception_handling(openai_unit_test_env: dict[str, str]): + """Test that exceptions in function result are properly handled. + + Feel free to remove this test in case there's another new behavior. + """ + client = OpenAIChatClient() + + # Test with exception (no result) + test_exception = ValueError("Test error message") + message_with_exception = ChatMessage( + role="tool", contents=[FunctionResultContent(call_id="call-123", exception=test_exception)] + ) + + openai_messages = client._openai_chat_message_parser(message_with_exception) + assert len(openai_messages) == 1 + assert openai_messages[0]["content"] == "Error: Test error message" + assert openai_messages[0]["tool_call_id"] == "call-123" + + +def test_prepare_function_call_results_with_basemodel(): + """Test prepare_function_call_results with BaseModel objects.""" + + class TestModel(BaseModel): + name: str + value: int + raw_representation: str = "should be excluded" + additional_properties: dict = {"should": "be excluded"} + + model_instance = TestModel(name="test", value=42) + result = prepare_function_call_results(model_instance) + + assert isinstance(result, str) + parsed = json.loads(result) + assert parsed["name"] == "test" + assert parsed["value"] == 42 + assert "raw_representation" not in parsed + assert "additional_properties" not in parsed + + +def test_prepare_function_call_results_with_nested_structures(): + """Test prepare_function_call_results with complex nested structures.""" + + class NestedModel(BaseModel): + id: int + raw_representation: str = "excluded" + + # Test with list of BaseModel objects + models = [NestedModel(id=1), [NestedModel(id=2)]] + result = prepare_function_call_results(models) + + assert isinstance(result, str) + parsed = json.loads(result) + assert len(parsed) == 2 + assert parsed[0]["id"] == 1 + assert isinstance(parsed[1], list) + assert len(parsed[1]) == 1 + assert parsed[1][0]["id"] == 2 + assert "raw_representation" not in parsed[0] + assert "raw_representation" not in parsed[1][0] + + +def test_prepare_function_call_results_with_dict_containing_basemodel(): + """Test prepare_function_call_results with dictionary containing BaseModel.""" + + class TestModel(BaseModel): + value: str + raw_representation: str = "excluded" + + # Test with dict containing BaseModel + complex_dict = {"model": TestModel(value="test"), "simple": "value", "number": 42} + + result = prepare_function_call_results(complex_dict) + + assert isinstance(result, str) + parsed = json.loads(result) + assert parsed["model"]["value"] == "test" + assert "raw_representation" not in parsed["model"] + assert parsed["simple"] == "value" + assert parsed["number"] == 42 + + +def test_prepare_function_call_results_string_passthrough(): + """Test that string values are passed through directly without JSON encoding.""" + result = prepare_function_call_results("simple string") + assert result == "simple string" + assert isinstance(result, str) + + +def test_prepare_function_call_results_with_none_values(): + """Test that None values in BaseModel fields are preserved to avoid validation errors during reloading.""" + + class Flight(BaseModel): + flight_id: str + departure: datetime | None + arrival: datetime | None + + # Test single BaseModel with None values (performance shortcut) + flight_with_nones = Flight(flight_id="123", departure=None, arrival=None) + result = prepare_function_call_results(flight_with_nones) + + assert isinstance(result, str) + parsed = json.loads(result) + assert parsed["flight_id"] == "123" + assert parsed["departure"] is None + assert parsed["arrival"] is None + + new_flight = Flight.model_validate_json(result) + assert new_flight == flight_with_nones + + def test_openai_content_parser_data_content_image(openai_unit_test_env: dict[str, str]) -> None: """Test _openai_content_parser converts DataContent with image media type to OpenAI format.""" client = OpenAIChatClient()