Code review fixes/pr 105

libraries/microsoft-agents-a365-runtime/microsoft_agents_a365/runtime/operation_result.py

@@ -47,9 +47,11 @@ def errors(self) -> List[OperationError]:
         Get the list of errors that occurred during the operation.
 
         Returns:
-            List[OperationError]: List of operation errors.
+            List[OperationError]: A copy of the list of operation errors.
+                The returned list is a defensive copy to protect the singleton
+                instance returned by success() from accidental modification.
         """
-        return self._errors
+        return list(self._errors)
 
     @staticmethod
     def success() -> "OperationResult":
@@ -88,4 +90,4 @@ def __str__(self) -> str:
             return "Succeeded"
         else:
             error_messages = ", ".join(str(error.message) for error in self._errors)
-            return f"Failed : {error_messages}"
+            return f"Failed: {error_messages}" if error_messages else "Failed"

libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/models/chat_history_message.py

@@ -38,8 +38,8 @@ def __post_init__(self):
             raise ValueError("role cannot be empty")
         if not self.content:
             raise ValueError("content cannot be empty")
-        if not self.timestamp:
-            raise ValueError("timestamp cannot be empty")
+        if self.timestamp is None:
+            raise ValueError("timestamp cannot be None")
 
     def to_dict(self):
         """

libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/services/mcp_tool_server_configuration_service.py

@@ -31,11 +31,16 @@
 from microsoft_agents.hosting.core import TurnContext
 
 # Local imports
-from ..models import ChatHistoryMessage, MCPServerConfig, ToolOptions
+from ..models import ChatHistoryMessage, ChatMessageRequest, MCPServerConfig, ToolOptions
 from ..utils import Constants
-from ..utils.utility import get_tooling_gateway_for_digital_worker, build_mcp_server_url
+from ..utils.utility import (
+    get_tooling_gateway_for_digital_worker,
+    build_mcp_server_url,
+    get_chat_history_endpoint,
+)
 
 # Runtime Imports
+from microsoft_agents_a365.runtime import OperationError, OperationResult
 from microsoft_agents_a365.runtime.utility import Utility as RuntimeUtility
 
 
@@ -520,12 +525,6 @@ async def send_chat_history(
         Raises:
             ValueError: If required parameters are invalid or empty.
         """
-        # Import here to avoid circular dependency
-        from microsoft_agents_a365.runtime import OperationError, OperationResult
-
-        from ..models import ChatMessageRequest
-        from ..utils.utility import get_chat_history_endpoint
-
         # Validate input parameters
         if not turn_context:
             raise ValueError("turn_context cannot be empty or None")
@@ -560,7 +559,7 @@ async def send_chat_history(
         # Get the endpoint URL
         endpoint = get_chat_history_endpoint()
 
-        self._logger.info(f"Sending chat history to endpoint: {endpoint}")
+        self._logger.debug(f"Sending chat history to endpoint: {endpoint}")
 
         # Create the request payload
         request = ChatMessageRequest(

tests/runtime/test_operation_result.py

@@ -85,7 +85,7 @@ def test_operation_result_failed_string_representation_no_errors(self):
         result = OperationResult.failed()
 
         # Assert
-        assert str(result) == "Failed : "
+        assert str(result) == "Failed"
 
     def test_operation_result_failed_string_representation_with_errors(self):
         """Test that failed OperationResult with errors has correct string representation."""

tests/tooling/models/test_chat_history_message.py

@@ -70,7 +70,7 @@ def test_chat_history_message_requires_non_empty_content(self):
     def test_chat_history_message_requires_timestamp(self):
         """Test that ChatHistoryMessage requires a timestamp."""
         # Act & Assert
-        with pytest.raises(ValueError, match="timestamp cannot be empty"):
+        with pytest.raises(ValueError, match="timestamp cannot be None"):
             ChatHistoryMessage("msg-001", "user", "Test content", None)
 
     def test_chat_history_message_supports_system_role(self):

tests/tooling/services/test_send_chat_history.py

@@ -124,35 +124,37 @@ async def test_send_chat_history_with_options(
             # Assert
             assert result.succeeded is True
 
-    def test_send_chat_history_validates_turn_context(self, service, chat_history_messages):
+    @pytest.mark.asyncio
+    async def test_send_chat_history_validates_turn_context(self, service, chat_history_messages):
         """Test that send_chat_history validates turn_context parameter."""
         # Act & Assert
         with pytest.raises(ValueError, match="turn_context cannot be empty or None"):
-            import asyncio
-
-            asyncio.run(service.send_chat_history(None, chat_history_messages))
+            await service.send_chat_history(None, chat_history_messages)
 
-    def test_send_chat_history_validates_chat_history_messages(self, service, mock_turn_context):
+    @pytest.mark.asyncio
+    async def test_send_chat_history_validates_chat_history_messages(
+        self, service, mock_turn_context
+    ):
         """Test that send_chat_history validates chat_history_messages parameter."""
         # Act & Assert
         with pytest.raises(ValueError, match="chat_history_messages cannot be empty or None"):
-            import asyncio
+            await service.send_chat_history(mock_turn_context, None)
 
-            asyncio.run(service.send_chat_history(mock_turn_context, None))
-
-    def test_send_chat_history_validates_activity(self, service, chat_history_messages):
+    @pytest.mark.asyncio
+    async def test_send_chat_history_validates_activity(self, service, chat_history_messages):
         """Test that send_chat_history validates turn_context.activity."""
         # Arrange
         mock_context = Mock()
         mock_context.activity = None
 
         # Act & Assert
         with pytest.raises(ValueError, match="turn_context.activity cannot be None"):
-            import asyncio
-
-            asyncio.run(service.send_chat_history(mock_context, chat_history_messages))
+            await service.send_chat_history(mock_context, chat_history_messages)
 
-    def test_send_chat_history_validates_conversation_id(self, service, chat_history_messages):
+    @pytest.mark.asyncio
+    async def test_send_chat_history_validates_conversation_id(
+        self, service, chat_history_messages
+    ):
         """Test that send_chat_history validates conversation_id from activity."""
         # Arrange
         mock_context = Mock()
@@ -167,11 +169,10 @@ def test_send_chat_history_validates_conversation_id(self, service, chat_history
             ValueError,
             match="conversation_id cannot be empty or None.*turn_context.activity.conversation.id",
         ):
-            import asyncio
+            await service.send_chat_history(mock_context, chat_history_messages)
 
-            asyncio.run(service.send_chat_history(mock_context, chat_history_messages))
-
-    def test_send_chat_history_validates_message_id(self, service, chat_history_messages):
+    @pytest.mark.asyncio
+    async def test_send_chat_history_validates_message_id(self, service, chat_history_messages):
         """Test that send_chat_history validates message_id from activity."""
         # Arrange
         mock_context = Mock()
@@ -187,11 +188,10 @@ def test_send_chat_history_validates_message_id(self, service, chat_history_mess
         with pytest.raises(
             ValueError, match="message_id cannot be empty or None.*turn_context.activity.id"
         ):
-            import asyncio
+            await service.send_chat_history(mock_context, chat_history_messages)
 
-            asyncio.run(service.send_chat_history(mock_context, chat_history_messages))
-
-    def test_send_chat_history_validates_user_message(self, service, chat_history_messages):
+    @pytest.mark.asyncio
+    async def test_send_chat_history_validates_user_message(self, service, chat_history_messages):
         """Test that send_chat_history validates user_message from activity."""
         # Arrange
         mock_context = Mock()
@@ -207,9 +207,7 @@ def test_send_chat_history_validates_user_message(self, service, chat_history_me
         with pytest.raises(
             ValueError, match="user_message cannot be empty or None.*turn_context.activity.text"
         ):
-            import asyncio
-
-            asyncio.run(service.send_chat_history(mock_context, chat_history_messages))
+            await service.send_chat_history(mock_context, chat_history_messages)
 
     @pytest.mark.asyncio
     async def test_send_chat_history_handles_client_error(