fix: address code review comments for PR #149

libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py

@@ -14,8 +14,9 @@
 import os
 import re
 import uuid
+from dataclasses import replace
 from datetime import datetime, timezone
-from typing import Any, List, Optional, Sequence
+from typing import List, Optional, Sequence
 
 # Third-party imports
 from semantic_kernel import kernel as sk
@@ -175,7 +176,7 @@ async def add_tool_servers_to_agent(
     # Private Methods - Input Validation & Processing
     # ============================================================================
 
-    def _validate_inputs(self, kernel: Any, agentic_app_id: str, auth_token: str) -> None:
+    def _validate_inputs(self, kernel: sk.Kernel, agentic_app_id: str, auth_token: str) -> None:
         """Validate all required inputs."""
         if kernel is None:
             raise ValueError("kernel cannot be None")
@@ -267,7 +268,8 @@ async def send_chat_history(
             # Re-raise validation errors
             raise
         except Exception as ex:
-            self._logger.error(f"Failed to send chat history: {ex}")
+            self._logger.error("Failed to send chat history")
+            self._logger.debug(f"Exception details: {ex}")
             return OperationResult.failed(OperationError(ex))
 
     async def send_chat_history_messages(
@@ -320,11 +322,11 @@ async def send_chat_history_messages(
 
         self._logger.info(f"Sending {len(messages)} Semantic Kernel messages as chat history")
 
-        # Set default options
+        # Set default options (create new instance to avoid mutating caller's object)
         if options is None:
             options = ToolOptions(orchestrator_name=self._orchestrator_name)
         elif options.orchestrator_name is None:
-            options.orchestrator_name = self._orchestrator_name
+            options = replace(options, orchestrator_name=self._orchestrator_name)
 
         try:
             # Convert Semantic Kernel messages to ChatHistoryMessage format
@@ -351,7 +353,8 @@ async def send_chat_history_messages(
             # Re-raise validation errors from the core service
             raise
         except Exception as ex:
-            self._logger.error(f"Failed to send chat history messages: {ex}")
+            self._logger.error("Failed to send chat history messages")
+            self._logger.debug(f"Exception details: {ex}")
             return OperationResult.failed(OperationError(ex))
 
     # ============================================================================

tests/tooling/extensions/semantickernel/services/conftest.py

@@ -184,12 +184,6 @@ def sample_system_message():
     )
 
 
-@pytest.fixture
-def sample_tool_message():
-    """Create a sample tool message."""
-    return MockChatMessageContent(role=MockAuthorRole.TOOL, content="Tool result here")
-
-
 @pytest.fixture
 def sample_sk_messages():
     """Create a list of sample Semantic Kernel messages."""

tests/tooling/extensions/semantickernel/services/test_send_chat_history.py

@@ -104,6 +104,29 @@ async def test_send_chat_history_messages_whitespace_only_messages_filtered(
         assert len(chat_history_messages) == 1
         assert chat_history_messages[0].content == "Valid message"
 
+    # UV-07
+    @pytest.mark.asyncio
+    @pytest.mark.unit
+    async def test_send_chat_history_all_messages_filtered_still_calls_core_service(
+        self, service, mock_turn_context, mock_config_service
+    ):
+        """Test that when all messages are filtered, core service is still called with empty list."""
+        messages = [
+            MockChatMessageContent(role=MockAuthorRole.USER, content=""),  # Empty - filtered
+            MockChatMessageContent(role=MockAuthorRole.USER, content="   "),  # Whitespace - filtered
+            MockChatMessageContent(role=MockAuthorRole.USER, content=None),  # None - filtered
+        ]
+
+        mock_config_service.send_chat_history.return_value = OperationResult.success()
+
+        result = await service.send_chat_history_messages(mock_turn_context, messages)
+
+        assert result.succeeded is True
+        mock_config_service.send_chat_history.assert_called_once()
+        call_args = mock_config_service.send_chat_history.call_args
+        # Verify empty list was passed to core service
+        assert call_args.kwargs["chat_history_messages"] == []
+
 
 # =============================================================================
 # ROLE MAPPING TESTS (RM-01)
@@ -621,6 +644,28 @@ async def test_send_chat_history_limit_zero_sends_all(
         # limit=0 should be treated as "no limit" per the condition `limit > 0`
         assert len(chat_history_messages) == 5
 
+    # LF-05
+    @pytest.mark.asyncio
+    @pytest.mark.unit
+    async def test_send_chat_history_negative_limit_sends_all(
+        self, service, mock_turn_context, mock_config_service
+    ):
+        """Test that negative limit is treated as no limit (sends all messages)."""
+        messages = [
+            MockChatMessageContent(role=MockAuthorRole.USER, content=f"Message {i}")
+            for i in range(5)
+        ]
+        chat_history = MockChatHistory(messages=messages)
+
+        mock_config_service.send_chat_history.return_value = OperationResult.success()
+
+        await service.send_chat_history(mock_turn_context, chat_history, limit=-1)
+
+        call_args = mock_config_service.send_chat_history.call_args
+        chat_history_messages = call_args.kwargs["chat_history_messages"]
+        # Negative limit should be treated as "no limit" per the condition `limit > 0`
+        assert len(chat_history_messages) == 5
+
 
 # =============================================================================
 # ERROR HANDLING TESTS (EH-01 to EH-06)