Code review fixes/pr 105 v3 (#115)

* fix(runtime): implement thread-safe singleton with eager initialization

Convert OperationResult.success() singleton from lazy to eager initialization
at module level. Python's import lock ensures thread-safe initialization,
eliminating the race condition in the previous check-and-create pattern.

Addresses CRM-001 from code review.

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

* fix(tooling): include error response body in HTTP error log message

Add truncated error_text (max 500 chars) to the log message when HTTP
errors occur in send_chat_history. This improves debugging by showing
the actual error response from the MCP platform.

Addresses CRM-002 from code review.

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

* fix(tooling): reorder exception handlers for proper timeout handling

Move asyncio.TimeoutError handler before aiohttp.ClientError to ensure
timeouts are caught correctly. Since aiohttp.ServerTimeoutError inherits
from both exceptions, the previous order could misclassify timeouts.

Addresses CRM-003 from code review.

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

* refactor(tooling): add type hints for local variables in send_chat_history

Add Optional[str] type annotations for conversation_id, message_id, and
user_message variables to improve code clarity and IDE support.

Addresses CRM-004 from code review.

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

* docs(tooling): improve validation error messages in ChatHistoryMessage

Update error messages to say "cannot be empty or whitespace-only" instead
of just "cannot be empty" for clearer feedback when whitespace validation
fails.

Addresses CRM-005 from code review.

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

* refactor(tooling): extract timeout and HTTP status code to constants

Add DEFAULT_REQUEST_TIMEOUT_SECONDS and HTTP_STATUS_OK module-level
constants to replace magic values. Improves maintainability and makes
configuration easier to modify.

Addresses CRM-006 from code review.

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

* docs(runtime): enhance defensive copy docstring in OperationResult.errors

Move the defensive copy rationale into a prominent Note section in the
docstring to make it more visible to developers. This clarifies why the
property returns a copy rather than the internal list.

Addresses CRM-007 from code review.

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

* test(tooling): use Mock(spec=TurnContext) for stricter interface validation

Import TurnContext and use it as a spec for the mock fixture. This ensures
the mock matches the actual TurnContext interface, catching potential
issues if the API changes.

Addresses CRM-008 from code review.

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

* docs(tooling): add usage example to send_chat_history docstring

Add an Example section showing how to create ChatHistoryMessage objects
and call send_chat_history with proper error handling. This helps
developers understand the intended usage pattern.

Addresses CRM-009 from code review.

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

---------

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

Author: 3 people

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