Code review fixes/pr 105 v3

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

@@ -46,10 +46,13 @@ def errors(self) -> List[OperationError]:
         """
         Get the list of errors that occurred during the operation.
 
+        Note:
+            This property returns a defensive copy of the internal error list
+            to prevent external modifications, which is especially important for
+            protecting the singleton instance returned by success().
+
         Returns:
             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 list(self._errors)
 
@@ -61,8 +64,6 @@ def success() -> "OperationResult":
         Returns:
             OperationResult: An OperationResult indicating a successful operation.
         """
-        if OperationResult._success_instance is None:
-            OperationResult._success_instance = OperationResult(succeeded=True)
         return OperationResult._success_instance
 
     @staticmethod
@@ -91,3 +92,7 @@ def __str__(self) -> str:
         else:
             error_messages = ", ".join(str(error.message) for error in self._errors)
             return f"Failed: {error_messages}" if error_messages else "Failed"
+
+
+# Module-level eager initialization (thread-safe by Python's import lock)
+OperationResult._success_instance = OperationResult(succeeded=True)

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

@@ -42,11 +42,11 @@ def __post_init__(self):
                         or if timestamp is None.
         """
         if not self.id or not self.id.strip():
-            raise ValueError("id cannot be empty")
+            raise ValueError("id cannot be empty or whitespace-only")
         if not self.role or not self.role.strip():
-            raise ValueError("role cannot be empty")
+            raise ValueError("role cannot be empty or whitespace-only")
         if not self.content or not self.content.strip():
-            raise ValueError("content cannot be empty")
+            raise ValueError("content cannot be empty or whitespace-only")
         if self.timestamp is None:
             raise ValueError("timestamp cannot be None")
 

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

@@ -45,6 +45,17 @@
 from microsoft_agents_a365.runtime.utility import Utility as RuntimeUtility
 
 
+# ==============================================================================
+# CONSTANTS
+# ==============================================================================
+
+# HTTP timeout in seconds for request operations
+DEFAULT_REQUEST_TIMEOUT_SECONDS = 30
+
+# HTTP status code for successful response
+HTTP_STATUS_OK = 200
+
+
 # ==============================================================================
 # MAIN SERVICE CLASS
 # ==============================================================================
@@ -532,6 +543,20 @@ async def send_chat_history(
             ValueError: If turn_context is None, chat_history_messages is None or empty,
                         turn_context.activity is None, or any of the required fields
                         (conversation.id, activity.id, activity.text) are missing or empty.
+
+        Example:
+            >>> from datetime import datetime, timezone
+            >>> from microsoft_agents_a365.tooling.models import ChatHistoryMessage
+            >>>
+            >>> history = [
+            ...     ChatHistoryMessage("msg-1", "user", "Hello", datetime.now(timezone.utc)),
+            ...     ChatHistoryMessage("msg-2", "assistant", "Hi!", datetime.now(timezone.utc))
+            ... ]
+            >>>
+            >>> service = McpToolServerConfigurationService()
+            >>> result = await service.send_chat_history(turn_context, history)
+            >>> if result.succeeded:
+            ...     print("Chat history sent successfully")
         """
         # Validate input parameters
         if turn_context is None:
@@ -543,13 +568,15 @@ async def send_chat_history(
         if not turn_context.activity:
             raise ValueError("turn_context.activity cannot be None")
 
-        conversation_id = (
+        conversation_id: Optional[str] = (
             turn_context.activity.conversation.id if turn_context.activity.conversation else None
         )
-        message_id = turn_context.activity.id
-        user_message = turn_context.activity.text
+        message_id: Optional[str] = turn_context.activity.id
+        user_message: Optional[str] = turn_context.activity.text
 
-        if conversation_id is None or (isinstance(conversation_id, str) and not conversation_id.strip()):
+        if conversation_id is None or (
+            isinstance(conversation_id, str) and not conversation_id.strip()
+        ):
             raise ValueError(
                 "conversation_id cannot be empty or None (from turn_context.activity.conversation.id)"
             )
@@ -592,16 +619,17 @@ async def send_chat_history(
             json_data = json.dumps(request.to_dict())
 
             # Send POST request with timeout to prevent indefinite hangs
-            timeout = aiohttp.ClientTimeout(total=30)  # 30 second timeout
+            timeout = aiohttp.ClientTimeout(total=DEFAULT_REQUEST_TIMEOUT_SECONDS)
             async with aiohttp.ClientSession(timeout=timeout) as session:
                 async with session.post(endpoint, headers=headers, data=json_data) as response:
-                    if response.status == 200:
+                    if response.status == HTTP_STATUS_OK:
                         self._logger.info("Successfully sent chat history to MCP platform")
                         return OperationResult.success()
                     else:
                         error_text = await response.text()
                         self._logger.error(
-                            f"HTTP error sending chat history: HTTP {response.status}"
+                            f"HTTP error sending chat history: HTTP {response.status}. "
+                            f"Response: {error_text[:500]}"
                         )
                         # Use ClientResponseError for consistent error handling
                         http_error = aiohttp.ClientResponseError(
@@ -613,14 +641,16 @@ async def send_chat_history(
                         )
                         return OperationResult.failed(OperationError(http_error))
 
-        except aiohttp.ClientError as http_ex:
-            self._logger.error(f"HTTP error sending chat history to '{endpoint}': {str(http_ex)}")
-            return OperationResult.failed(OperationError(http_ex))
         except asyncio.TimeoutError as timeout_ex:
+            # Catch TimeoutError before ClientError since aiohttp.ServerTimeoutError
+            # inherits from both asyncio.TimeoutError and aiohttp.ClientError
             self._logger.error(
                 f"Request timeout sending chat history to '{endpoint}': {str(timeout_ex)}"
             )
             return OperationResult.failed(OperationError(timeout_ex))
+        except aiohttp.ClientError as http_ex:
+            self._logger.error(f"HTTP error sending chat history to '{endpoint}': {str(http_ex)}")
+            return OperationResult.failed(OperationError(http_ex))
         except Exception as ex:
             self._logger.error(f"Failed to send chat history to '{endpoint}': {str(ex)}")
             return OperationResult.failed(OperationError(ex))

tests/tooling/services/test_send_chat_history.py

@@ -7,6 +7,7 @@
 from unittest.mock import AsyncMock, MagicMock, Mock, patch
 
 import pytest
+from microsoft_agents.hosting.core import TurnContext
 from microsoft_agents_a365.tooling.models import ChatHistoryMessage
 from microsoft_agents_a365.tooling.services import McpToolServerConfigurationService
 
@@ -16,8 +17,8 @@ class TestSendChatHistory:
 
     @pytest.fixture
     def mock_turn_context(self):
-        """Create a mock TurnContext."""
-        mock_context = Mock()
+        """Create a mock TurnContext with spec for stricter interface validation."""
+        mock_context = Mock(spec=TurnContext)
         mock_activity = Mock()
         mock_conversation = Mock()