From fcb9cc36dd29640c0e4942eba19cb42d14b6b3f2 Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Wed, 28 Jan 2026 16:26:04 -0800 Subject: [PATCH 1/6] fix(semantickernel): address review comment CRM-002 Remove typing.Any and use sk.Kernel type for the kernel parameter in _validate_inputs method, adhering to project coding standards. Co-Authored-By: Claude Opus 4.5 --- .../semantickernel/services/mcp_tool_registration_service.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py b/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py index b4274081..3042ef66 100644 --- a/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py +++ b/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py @@ -15,7 +15,7 @@ import re import uuid 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 +175,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") From 1bc8768d6c90f4ffed746ecaa44f55c00ed09c2b Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Wed, 28 Jan 2026 16:26:47 -0800 Subject: [PATCH 2/6] fix(semantickernel): address review comment CRM-003 Improve exception logging security by logging generic error messages at ERROR level and detailed exception info at DEBUG level to avoid potential exposure of sensitive data from chat history. Co-Authored-By: Claude Opus 4.5 --- .../services/mcp_tool_registration_service.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py b/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py index 3042ef66..4e7af063 100644 --- a/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py +++ b/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py @@ -267,7 +267,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( @@ -351,7 +352,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)) # ============================================================================ From ea6658fb3bf8c6c3ad6812c19c29a9e9ac05111b Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Wed, 28 Jan 2026 16:27:58 -0800 Subject: [PATCH 3/6] fix(semantickernel): address review comment CRM-004 Use dataclasses.replace() to create a new ToolOptions instance instead of mutating the caller's object when orchestrator_name needs to be set. This avoids unexpected side effects for callers who reuse ToolOptions. Co-Authored-By: Claude Opus 4.5 --- .../semantickernel/services/mcp_tool_registration_service.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py b/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py index 4e7af063..613a0296 100644 --- a/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py +++ b/libraries/microsoft-agents-a365-tooling-extensions-semantickernel/microsoft_agents_a365/tooling/extensions/semantickernel/services/mcp_tool_registration_service.py @@ -14,6 +14,7 @@ import os import re import uuid +from dataclasses import replace from datetime import datetime, timezone from typing import List, Optional, Sequence @@ -321,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 From 5c0ec5bc8141a83efca5ba2603a01bb074b6bdc8 Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Wed, 28 Jan 2026 16:29:04 -0800 Subject: [PATCH 4/6] test(semantickernel): address review comment CRM-005 Add test case LF-05 to verify that negative limit values are treated as no limit, sending all messages. This documents the expected behavior of the limit parameter when negative values are passed. Co-Authored-By: Claude Opus 4.5 --- .../services/test_send_chat_history.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/tooling/extensions/semantickernel/services/test_send_chat_history.py b/tests/tooling/extensions/semantickernel/services/test_send_chat_history.py index 3c509142..b2ddc667 100644 --- a/tests/tooling/extensions/semantickernel/services/test_send_chat_history.py +++ b/tests/tooling/extensions/semantickernel/services/test_send_chat_history.py @@ -621,6 +621,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) From 5874c41b9b43f5032cbf05954da881196e9c7810 Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Wed, 28 Jan 2026 16:29:42 -0800 Subject: [PATCH 5/6] test(semantickernel): address review comment CRM-006 Add test case UV-07 to verify that when all input messages are filtered (empty content, whitespace, None), the core service is still called with an empty list. This ensures the user message registration still occurs. Co-Authored-By: Claude Opus 4.5 --- .../services/test_send_chat_history.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/tooling/extensions/semantickernel/services/test_send_chat_history.py b/tests/tooling/extensions/semantickernel/services/test_send_chat_history.py index b2ddc667..a4fe09ba 100644 --- a/tests/tooling/extensions/semantickernel/services/test_send_chat_history.py +++ b/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) From aeb1635a1b9b4671d9d94d7091f7dd574c560f16 Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Wed, 28 Jan 2026 16:30:19 -0800 Subject: [PATCH 6/6] test(semantickernel): address review comment CRM-008 Remove unused sample_tool_message fixture from conftest.py. The fixture was defined but never used in any test. Co-Authored-By: Claude Opus 4.5 --- .../tooling/extensions/semantickernel/services/conftest.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/tooling/extensions/semantickernel/services/conftest.py b/tests/tooling/extensions/semantickernel/services/conftest.py index c7bf8a7b..8dc0faa6 100644 --- a/tests/tooling/extensions/semantickernel/services/conftest.py +++ b/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."""