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 <ekzhu@users.noreply.github.com>

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"] = []

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):

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()