Merge branch 'main' into python-middleware

python/packages/main/agent_framework/_types.py

@@ -1870,10 +1870,17 @@ class ChatOptions(AFBaseModel):
         # No tool choice if no tools are defined
         if self.tools is None or len(self.tools) == 0:
             default_exclude.add("tool_choice")
+        # No metadata and logit bias if they are empty
+        # Prevents 400 error
+        if not self.logit_bias:
+            default_exclude.add("logit_bias")
+        if not self.metadata:
+            default_exclude.add("metadata")
+
         merged_exclude = default_exclude if exclude is None else default_exclude | set(exclude)
 
         settings = self.model_dump(exclude_none=True, by_alias=by_alias, exclude=merged_exclude)
-        settings = {k: v for k, v in settings.items() if v}
+        settings = {k: v for k, v in settings.items() if v is not None}
         settings.update(self.additional_properties)
         for key in merged_exclude:
             settings.pop(key, None)

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/main/test_types.py

@@ -1138,6 +1138,59 @@ def test_chat_options_tool_choice_dict_mapping(ai_tool):
     assert settings["tool_choice"] == "required"
 
 
+def test_chat_options_to_provider_settings_with_falsy_values():
+    """Test that falsy values (except None) are included in provider settings."""
+    options = ChatOptions(
+        temperature=0.0,  # falsy but not None
+        top_p=0.0,  # falsy but not None
+        presence_penalty=False,  # falsy but not None
+        frequency_penalty=None,  # None - should be excluded
+        additional_properties={"empty_string": "", "zero": 0, "false_flag": False, "none_value": None},
+    )
+
+    settings = options.to_provider_settings()
+
+    # Falsy values that are not None should be included
+    assert "temperature" in settings
+    assert isinstance(settings["temperature"], float)
+    assert settings["temperature"] == 0.0
+    assert "top_p" in settings
+    assert isinstance(settings["top_p"], float)
+    assert settings["top_p"] == 0.0
+    assert "presence_penalty" in settings
+    assert isinstance(settings["presence_penalty"], float)  # converted to float
+    assert settings["presence_penalty"] == 0.0
+
+    # None values should be excluded
+    assert "frequency_penalty" not in settings
+
+    # Additional properties - falsy values should always be included
+    assert "empty_string" in settings
+    assert settings["empty_string"] == ""
+    assert "zero" in settings
+    assert settings["zero"] == 0
+    assert "false_flag" in settings
+    assert settings["false_flag"] is False
+    assert "none_value" in settings
+    assert settings["none_value"] is None
+
+
+def test_chat_options_empty_logit_bias_and_metadata_excluded():
+    """Test that empty logit_bias and metadata are excluded from provider settings."""
+    options = ChatOptions(
+        ai_model_id="gpt-4o",
+        logit_bias={},  # empty dict should be excluded
+        metadata={},  # empty dict should be excluded
+    )
+
+    settings = options.to_provider_settings()
+
+    # Empty logit_bias and metadata should be excluded
+    assert "logit_bias" not in settings
+    assert "metadata" not in settings
+    assert settings["model"] == "gpt-4o"
+
+
 # region AgentRunResponse
 
 

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