Code review fixes/pr 105 (#114)

* Fix inconsistent string representation for failed result with no errors

Improves the __str__ method of OperationResult to return "Failed" instead of
"Failed : " when there are no errors. Also removes the extra space before
the colon for consistency.

Addresses code review Comment 1.

Co-Authored-By: Claude <noreply@anthropic.com>

* Return defensive copy of errors list to protect singleton

The errors property now returns a copy of the internal errors list to
protect the singleton instance returned by success() from accidental
modification. Updated docstring to document this behavior.

Addresses code review Comment 2.

Co-Authored-By: Claude <noreply@anthropic.com>

* Use explicit None check for timestamp validation

Changed timestamp validation from falsy check (if not self.timestamp)
to explicit None check (if self.timestamp is None) for safer and more
intentional validation behavior. Updated error message and test accordingly.

Addresses code review Comment 3.

Co-Authored-By: Claude <noreply@anthropic.com>

* Move local imports to top of file

Moved OperationError, OperationResult, ChatMessageRequest, and
get_chat_history_endpoint imports from inside the send_chat_history
method to the top of the file with the other imports. Removed the
misleading comment about circular dependencies as there is no cycle
in the import graph.

Addresses code review Comment 4.

Co-Authored-By: Claude <noreply@anthropic.com>

* Change endpoint URL log level from INFO to DEBUG

Detailed operational information like endpoint URLs should be logged
at DEBUG level rather than INFO level. INFO level is reserved for
higher-level operation status messages.

Addresses code review Comment 5.

Co-Authored-By: Claude <noreply@anthropic.com>

* Use consistent async test pattern for validation tests

Converted validation tests from synchronous methods using asyncio.run()
to async methods with @pytest.mark.asyncio decorator for consistency
with the other tests in the test suite.

Addresses code review Comment 7.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Johan Broberg <johanb@microsoft.com>
Co-authored-by: Claude <noreply@anthropic.com>

Author: 3 people

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(