From 850bdd782ab918879fd40dc25fec9ea0ccbfa34e Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Fri, 23 Jan 2026 09:39:26 -0800 Subject: [PATCH 1/5] fix(agentframework): address code review comments CRM-003 and CRM-004 - CRM-003: Add turn_context validation in send_chat_history_async method to ensure fail-fast behavior and consistency with docstring - CRM-004: Document empty message filtering behavior in _convert_chat_messages_to_history docstring and elevate log level from DEBUG to WARNING when messages are skipped Note: CRM-001 and CRM-002 (copyright header format) are not applicable because the existing format ("Microsoft. All rights reserved.") is required by the pyproject.toml linter configuration (notice-rgx), while CLAUDE.md specifies a different format. The linter-enforced format takes precedence to ensure CI passes. Co-Authored-By: Claude Opus 4.5 --- .../services/mcp_tool_registration_service.py | 184 +++++++- tests/tooling/extensions/__init__.py | 3 + .../extensions/agentframework/__init__.py | 3 + .../agentframework/services/__init__.py | 3 + .../services/test_send_chat_history_async.py | 436 ++++++++++++++++++ 5 files changed, 625 insertions(+), 4 deletions(-) create mode 100644 tests/tooling/extensions/__init__.py create mode 100644 tests/tooling/extensions/agentframework/__init__.py create mode 100644 tests/tooling/extensions/agentframework/services/__init__.py create mode 100644 tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py diff --git a/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py b/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py index 267680d6..37c24d89 100644 --- a/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py +++ b/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py @@ -1,21 +1,23 @@ # Copyright (c) Microsoft. All rights reserved. -from typing import Optional, List, Any, Union import logging +import uuid +from datetime import datetime, timezone +from typing import Any, List, Optional, Sequence, Union -from agent_framework import ChatAgent, MCPStreamableHTTPTool +from agent_framework import ChatAgent, ChatMessage, ChatMessageStoreProtocol, MCPStreamableHTTPTool from agent_framework.azure import AzureOpenAIChatClient from agent_framework.openai import OpenAIChatClient from microsoft_agents.hosting.core import Authorization, TurnContext +from microsoft_agents_a365.runtime import OperationResult from microsoft_agents_a365.runtime.utility import Utility +from microsoft_agents_a365.tooling.models import ChatHistoryMessage, ToolOptions from microsoft_agents_a365.tooling.services.mcp_tool_server_configuration_service import ( McpToolServerConfigurationService, ) -from microsoft_agents_a365.tooling.models import ToolOptions from microsoft_agents_a365.tooling.utils.constants import Constants - from microsoft_agents_a365.tooling.utils.utility import ( get_mcp_platform_authentication_scope, ) @@ -154,6 +156,180 @@ async def add_tool_servers_to_agent( self._logger.error(f"Failed to add tool servers to agent: {ex}") raise + def _convert_chat_messages_to_history( + self, + chat_messages: Sequence[ChatMessage], + ) -> List[ChatHistoryMessage]: + """ + Convert Agent Framework ChatMessage objects to ChatHistoryMessage format. + + This internal helper method transforms Agent Framework's native ChatMessage + objects into the ChatHistoryMessage format expected by the MCP platform's + real-time threat protection endpoint. + + Args: + chat_messages: Sequence of ChatMessage objects to convert. + + Returns: + List of ChatHistoryMessage objects ready for the MCP platform. + + Note: + - If message_id is None, a new UUID is generated + - Role is extracted via the .value property of the Role object + - Timestamp is set to current UTC time (ChatMessage has no timestamp) + - Messages with empty or whitespace-only content are filtered out and + logged at WARNING level. This is because ChatHistoryMessage requires + non-empty content for validation. The filtered messages will not be + sent to the MCP platform. + """ + history_messages: List[ChatHistoryMessage] = [] + current_time = datetime.now(timezone.utc) + + for msg in chat_messages: + message_id = msg.message_id if msg.message_id is not None else str(uuid.uuid4()) + role = msg.role.value + content = msg.text if msg.text is not None else "" + + # Skip messages with empty content as ChatHistoryMessage validates non-empty content + if not content or not content.strip(): + self._logger.warning( + f"Skipping message {message_id} with empty content during conversion" + ) + continue + + history_message = ChatHistoryMessage( + id=message_id, + role=role, + content=content, + timestamp=current_time, + ) + history_messages.append(history_message) + + self._logger.debug( + f"Converted message {message_id} with role '{role}' to ChatHistoryMessage" + ) + + return history_messages + + async def send_chat_history_messages_async( + self, + chat_messages: Sequence[ChatMessage], + turn_context: TurnContext, + tool_options: Optional[ToolOptions] = None, + ) -> OperationResult: + """ + Send chat history messages to the MCP platform for real-time threat protection. + + This is the primary implementation method that handles message conversion + and delegation to the core tooling service. + + Args: + chat_messages: Sequence of Agent Framework ChatMessage objects to send. + turn_context: TurnContext from the Agents SDK containing conversation info. + tool_options: Optional configuration for the request. Defaults to + AgentFramework-specific options if not provided. + + Returns: + OperationResult indicating success or failure of the operation. + + Raises: + ValueError: If chat_messages or turn_context is None. + + Example: + >>> service = McpToolRegistrationService() + >>> messages = [ChatMessage(role=Role.USER, text="Hello")] + >>> result = await service.send_chat_history_messages_async(messages, turn_context) + >>> if result.succeeded: + ... print("Chat history sent successfully") + """ + # Input validation + if chat_messages is None: + raise ValueError("chat_messages cannot be None") + + if turn_context is None: + raise ValueError("turn_context cannot be None") + + # Handle empty messages - return success with warning + if len(chat_messages) == 0: + self._logger.warning("Empty message list provided to send_chat_history_messages_async") + return OperationResult.success() + + self._logger.info(f"Send chat history initiated with {len(chat_messages)} messages") + + # Use default options if not provided + if tool_options is None: + tool_options = ToolOptions(orchestrator_name=self._orchestrator_name) + + # Convert messages to ChatHistoryMessage format + history_messages = self._convert_chat_messages_to_history(chat_messages) + + # Check if all messages were filtered out during conversion + if len(history_messages) == 0: + self._logger.warning("All messages were filtered out during conversion (empty content)") + return OperationResult.success() + + # Delegate to core service + result = await self._mcp_server_configuration_service.send_chat_history( + turn_context=turn_context, + chat_history_messages=history_messages, + options=tool_options, + ) + + if result.succeeded: + self._logger.info( + f"Chat history sent successfully with {len(history_messages)} messages" + ) + else: + self._logger.error(f"Failed to send chat history: {result}") + + return result + + async def send_chat_history_async( + self, + chat_message_store: ChatMessageStoreProtocol, + turn_context: TurnContext, + tool_options: Optional[ToolOptions] = None, + ) -> OperationResult: + """ + Send chat history from a ChatMessageStore to the MCP platform. + + This is a convenience method that extracts messages from the store + and delegates to send_chat_history_messages_async(). + + Args: + chat_message_store: ChatMessageStore containing the conversation history. + turn_context: TurnContext from the Agents SDK containing conversation info. + tool_options: Optional configuration for the request. + + Returns: + OperationResult indicating success or failure of the operation. + + Raises: + ValueError: If chat_message_store or turn_context is None. + + Example: + >>> service = McpToolRegistrationService() + >>> result = await service.send_chat_history_async( + ... thread.chat_message_store, turn_context + ... ) + """ + # Input validation + if chat_message_store is None: + raise ValueError("chat_message_store cannot be None") + + if turn_context is None: + raise ValueError("turn_context cannot be None") + + # Extract messages from the store + messages = await chat_message_store.list_messages() + + # Delegate to the primary implementation + return await self.send_chat_history_messages_async( + chat_messages=messages, + turn_context=turn_context, + tool_options=tool_options, + ) + async def cleanup(self): """Clean up any resources used by the service.""" try: diff --git a/tests/tooling/extensions/__init__.py b/tests/tooling/extensions/__init__.py new file mode 100644 index 00000000..401677b5 --- /dev/null +++ b/tests/tooling/extensions/__init__.py @@ -0,0 +1,3 @@ +# Copyright (c) Microsoft. All rights reserved. + +"""Test package for tooling extensions.""" diff --git a/tests/tooling/extensions/agentframework/__init__.py b/tests/tooling/extensions/agentframework/__init__.py new file mode 100644 index 00000000..9cf178c1 --- /dev/null +++ b/tests/tooling/extensions/agentframework/__init__.py @@ -0,0 +1,3 @@ +# Copyright (c) Microsoft. All rights reserved. + +"""Test package for Agent Framework tooling extensions.""" diff --git a/tests/tooling/extensions/agentframework/services/__init__.py b/tests/tooling/extensions/agentframework/services/__init__.py new file mode 100644 index 00000000..c167a2ed --- /dev/null +++ b/tests/tooling/extensions/agentframework/services/__init__.py @@ -0,0 +1,3 @@ +# Copyright (c) Microsoft. All rights reserved. + +"""Test package for Agent Framework tooling extension services.""" diff --git a/tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py b/tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py new file mode 100644 index 00000000..4f5f029b --- /dev/null +++ b/tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py @@ -0,0 +1,436 @@ +# Copyright (c) Microsoft. All rights reserved. + +"""Unit tests for send_chat_history_async methods in McpToolRegistrationService.""" + +# Direct import from service file to work around namespace package resolution issues +# with editable installs in the uv workspace +import importlib.util +import sys +from pathlib import Path + +_service_path = ( + Path(__file__).parent.parent.parent.parent.parent.parent + / "libraries" + / "microsoft-agents-a365-tooling-extensions-agentframework" + / "microsoft_agents_a365" + / "tooling" + / "extensions" + / "agentframework" + / "services" + / "mcp_tool_registration_service.py" +) +_spec = importlib.util.spec_from_file_location("mcp_tool_registration_service", _service_path) +_module = importlib.util.module_from_spec(_spec) +sys.modules["mcp_tool_registration_service"] = _module +_spec.loader.exec_module(_module) +McpToolRegistrationService = _module.McpToolRegistrationService + +from datetime import UTC, datetime # noqa: E402 +from unittest.mock import AsyncMock, Mock, patch # noqa: E402 + +import pytest # noqa: E402 +from microsoft_agents.hosting.core import TurnContext # noqa: E402 +from microsoft_agents_a365.runtime import OperationError, OperationResult # noqa: E402 +from microsoft_agents_a365.tooling.models import ToolOptions # noqa: E402 + + +class TestSendChatHistoryAsync: + """Tests for send_chat_history_messages_async and send_chat_history_async methods.""" + + @pytest.fixture + def mock_turn_context(self): + """Create a mock TurnContext with valid activity data.""" + mock_context = Mock(spec=TurnContext) + mock_activity = Mock() + mock_conversation = Mock() + + mock_conversation.id = "conv-test-123" + mock_activity.conversation = mock_conversation + mock_activity.id = "msg-test-456" + mock_activity.text = "Test user message" + + mock_context.activity = mock_activity + return mock_context + + @pytest.fixture + def mock_role(self): + """Create a mock Role object with .value property.""" + role = Mock() + role.value = "user" + return role + + @pytest.fixture + def mock_assistant_role(self): + """Create a mock Role object for assistant.""" + role = Mock() + role.value = "assistant" + return role + + @pytest.fixture + def sample_chat_messages(self, mock_role, mock_assistant_role): + """Create sample Agent Framework ChatMessage-like objects.""" + msg1 = Mock() + msg1.message_id = "msg-1" + msg1.role = mock_role + msg1.text = "Hello" + + msg2 = Mock() + msg2.message_id = "msg-2" + msg2.role = mock_assistant_role + msg2.text = "Hi there!" + + return [msg1, msg2] + + @pytest.fixture + def mock_chat_message_store(self, sample_chat_messages): + """Create a mock ChatMessageStoreProtocol.""" + store = AsyncMock() + store.list_messages = AsyncMock(return_value=sample_chat_messages) + return store + + @pytest.fixture + def service(self): + """Create McpToolRegistrationService instance with mocked core service.""" + svc = McpToolRegistrationService() + svc._mcp_server_configuration_service = Mock() + svc._mcp_server_configuration_service.send_chat_history = AsyncMock( + return_value=OperationResult.success() + ) + return svc + + # ==================== Validation Tests (8 tests) ==================== + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_validates_messages_none( + self, service, mock_turn_context + ): + """Test that send_chat_history_messages_async raises ValueError for None messages.""" + with pytest.raises(ValueError, match="chat_messages cannot be None"): + await service.send_chat_history_messages_async(None, mock_turn_context) + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_validates_turn_context_none( + self, service, sample_chat_messages + ): + """Test that send_chat_history_messages_async raises ValueError for None turn_context.""" + with pytest.raises(ValueError, match="turn_context cannot be None"): + await service.send_chat_history_messages_async(sample_chat_messages, None) + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_async_validates_store_none(self, service, mock_turn_context): + """Test that send_chat_history_async raises ValueError for None store.""" + with pytest.raises(ValueError, match="chat_message_store cannot be None"): + await service.send_chat_history_async(None, mock_turn_context) + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_async_validates_turn_context_none( + self, service, mock_chat_message_store + ): + """Test that send_chat_history_async raises ValueError for None turn_context.""" + with pytest.raises(ValueError, match="turn_context cannot be None"): + await service.send_chat_history_async(mock_chat_message_store, None) + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_empty_messages_returns_success( + self, service, mock_turn_context + ): + """Test that empty message list returns success with warning log.""" + # Act + result = await service.send_chat_history_messages_async([], mock_turn_context) + + # Assert + assert result.succeeded is True + # Core service should not be called for empty messages + service._mcp_server_configuration_service.send_chat_history.assert_not_called() + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_generates_uuid_for_missing_id( + self, service, mock_turn_context, mock_role + ): + """Test that UUID is generated when message_id is None.""" + # Arrange + msg = Mock() + msg.message_id = None # No message ID + msg.role = mock_role + msg.text = "Hello" + + # Act + await service.send_chat_history_messages_async([msg], mock_turn_context) + + # Assert + call_args = service._mcp_server_configuration_service.send_chat_history.call_args + history_messages = call_args.kwargs["chat_history_messages"] + + assert len(history_messages) == 1 + # Verify a UUID was generated (not None and has UUID format) + assert history_messages[0].id is not None + assert len(history_messages[0].id) == 36 # UUID format: 8-4-4-4-12 + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_generates_timestamp( + self, service, mock_turn_context, sample_chat_messages + ): + """Test that current UTC timestamp is generated for messages.""" + # Act + before_time = datetime.now(UTC) + await service.send_chat_history_messages_async(sample_chat_messages, mock_turn_context) + after_time = datetime.now(UTC) + + # Assert + call_args = service._mcp_server_configuration_service.send_chat_history.call_args + history_messages = call_args.kwargs["chat_history_messages"] + + for msg in history_messages: + assert msg.timestamp is not None + assert before_time <= msg.timestamp <= after_time + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_handles_missing_text( + self, service, mock_turn_context, mock_role + ): + """Test that messages with None text are skipped (empty content not allowed).""" + # Arrange + msg_with_text = Mock() + msg_with_text.message_id = "msg-1" + msg_with_text.role = mock_role + msg_with_text.text = "Hello" + + msg_without_text = Mock() + msg_without_text.message_id = "msg-2" + msg_without_text.role = mock_role + msg_without_text.text = None # No text + + # Act + await service.send_chat_history_messages_async( + [msg_with_text, msg_without_text], mock_turn_context + ) + + # Assert + call_args = service._mcp_server_configuration_service.send_chat_history.call_args + history_messages = call_args.kwargs["chat_history_messages"] + + # Only the message with text should be included + assert len(history_messages) == 1 + assert history_messages[0].content == "Hello" + + # ==================== Success and Delegation Tests (5 tests) ==================== + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_success( + self, service, mock_turn_context, sample_chat_messages + ): + """Test successful send_chat_history_messages_async call.""" + # Act + result = await service.send_chat_history_messages_async( + sample_chat_messages, mock_turn_context + ) + + # Assert + assert result.succeeded is True + assert len(result.errors) == 0 + service._mcp_server_configuration_service.send_chat_history.assert_called_once() + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_async_with_store_success( + self, service, mock_turn_context, mock_chat_message_store + ): + """Test successful send_chat_history_async call with ChatMessageStore.""" + # Act + result = await service.send_chat_history_async(mock_chat_message_store, mock_turn_context) + + # Assert + assert result.succeeded is True + mock_chat_message_store.list_messages.assert_called_once() + service._mcp_server_configuration_service.send_chat_history.assert_called_once() + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_async_delegates_to_messages_async( + self, service, mock_turn_context, mock_chat_message_store, sample_chat_messages + ): + """Test that send_chat_history_async delegates to send_chat_history_messages_async.""" + # Arrange + with patch.object( + service, "send_chat_history_messages_async", new_callable=AsyncMock + ) as mock_messages_method: + mock_messages_method.return_value = OperationResult.success() + + # Act + result = await service.send_chat_history_async( + mock_chat_message_store, mock_turn_context + ) + + # Assert + assert result.succeeded is True + mock_chat_message_store.list_messages.assert_called_once() + mock_messages_method.assert_called_once_with( + chat_messages=sample_chat_messages, + turn_context=mock_turn_context, + tool_options=None, + ) + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_with_tool_options( + self, service, mock_turn_context, sample_chat_messages + ): + """Test that ToolOptions are passed correctly to core service.""" + # Arrange + options = ToolOptions(orchestrator_name="TestOrchestrator") + + # Act + await service.send_chat_history_messages_async( + sample_chat_messages, mock_turn_context, tool_options=options + ) + + # Assert + call_args = service._mcp_server_configuration_service.send_chat_history.call_args + assert call_args.kwargs["options"] == options + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_converts_messages_correctly( + self, service, mock_turn_context, sample_chat_messages + ): + """Test that ChatMessage objects are correctly converted to ChatHistoryMessage.""" + # Act + await service.send_chat_history_messages_async(sample_chat_messages, mock_turn_context) + + # Assert + call_args = service._mcp_server_configuration_service.send_chat_history.call_args + history_messages = call_args.kwargs["chat_history_messages"] + + assert len(history_messages) == 2 + + # Verify first message conversion + assert history_messages[0].id == "msg-1" + assert history_messages[0].role == "user" + assert history_messages[0].content == "Hello" + assert history_messages[0].timestamp is not None + + # Verify second message conversion + assert history_messages[1].id == "msg-2" + assert history_messages[1].role == "assistant" + assert history_messages[1].content == "Hi there!" + assert history_messages[1].timestamp is not None + + # ==================== Error Handling Tests (4 tests) ==================== + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_handles_http_error( + self, service, mock_turn_context, sample_chat_messages + ): + """Test send_chat_history_messages_async handles HTTP errors from core service.""" + # Arrange + error = OperationError(Exception("500, Internal Server Error")) + service._mcp_server_configuration_service.send_chat_history = AsyncMock( + return_value=OperationResult.failed(error) + ) + + # Act + result = await service.send_chat_history_messages_async( + sample_chat_messages, mock_turn_context + ) + + # Assert + assert result.succeeded is False + assert len(result.errors) == 1 + assert "500" in str(result.errors[0].message) + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_handles_timeout( + self, service, mock_turn_context, sample_chat_messages + ): + """Test send_chat_history_messages_async handles timeout errors.""" + # Arrange + error = OperationError(Exception("Request timed out")) + service._mcp_server_configuration_service.send_chat_history = AsyncMock( + return_value=OperationResult.failed(error) + ) + + # Act + result = await service.send_chat_history_messages_async( + sample_chat_messages, mock_turn_context + ) + + # Assert + assert result.succeeded is False + assert len(result.errors) == 1 + assert "timed out" in str(result.errors[0].message) + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_handles_connection_error( + self, service, mock_turn_context, sample_chat_messages + ): + """Test send_chat_history_messages_async handles connection errors.""" + # Arrange + error = OperationError(Exception("Connection failed")) + service._mcp_server_configuration_service.send_chat_history = AsyncMock( + return_value=OperationResult.failed(error) + ) + + # Act + result = await service.send_chat_history_messages_async( + sample_chat_messages, mock_turn_context + ) + + # Assert + assert result.succeeded is False + assert len(result.errors) == 1 + assert "Connection failed" in str(result.errors[0].message) + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_async_role_value_conversion( + self, service, mock_turn_context + ): + """Test that Role.value is used for string conversion.""" + # Arrange - Create messages with different role values + system_role = Mock() + system_role.value = "system" + + user_role = Mock() + user_role.value = "user" + + assistant_role = Mock() + assistant_role.value = "assistant" + + msg1 = Mock() + msg1.message_id = "msg-1" + msg1.role = system_role + msg1.text = "System prompt" + + msg2 = Mock() + msg2.message_id = "msg-2" + msg2.role = user_role + msg2.text = "User message" + + msg3 = Mock() + msg3.message_id = "msg-3" + msg3.role = assistant_role + msg3.text = "Assistant response" + + # Act + await service.send_chat_history_messages_async([msg1, msg2, msg3], mock_turn_context) + + # Assert + call_args = service._mcp_server_configuration_service.send_chat_history.call_args + history_messages = call_args.kwargs["chat_history_messages"] + + assert len(history_messages) == 3 + assert history_messages[0].role == "system" + assert history_messages[1].role == "user" + assert history_messages[2].role == "assistant" From 0f47fe4f94847dc3ee08d30248dbd7ace9c7d77a Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Fri, 23 Jan 2026 22:57:44 -0800 Subject: [PATCH 2/5] fix: update copyright notices to reflect Microsoft Corporation and include license information --- .../agentframework/services/mcp_tool_registration_service.py | 3 ++- tests/tooling/extensions/__init__.py | 3 ++- tests/tooling/extensions/agentframework/__init__.py | 3 ++- tests/tooling/extensions/agentframework/services/__init__.py | 3 ++- .../agentframework/services/test_send_chat_history_async.py | 3 ++- 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py b/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py index 37c24d89..614de4dc 100644 --- a/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py +++ b/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py @@ -1,4 +1,5 @@ -# Copyright (c) Microsoft. All rights reserved. +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. import logging import uuid diff --git a/tests/tooling/extensions/__init__.py b/tests/tooling/extensions/__init__.py index 401677b5..4f0494bc 100644 --- a/tests/tooling/extensions/__init__.py +++ b/tests/tooling/extensions/__init__.py @@ -1,3 +1,4 @@ -# Copyright (c) Microsoft. All rights reserved. +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. """Test package for tooling extensions.""" diff --git a/tests/tooling/extensions/agentframework/__init__.py b/tests/tooling/extensions/agentframework/__init__.py index 9cf178c1..eaac7401 100644 --- a/tests/tooling/extensions/agentframework/__init__.py +++ b/tests/tooling/extensions/agentframework/__init__.py @@ -1,3 +1,4 @@ -# Copyright (c) Microsoft. All rights reserved. +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. """Test package for Agent Framework tooling extensions.""" diff --git a/tests/tooling/extensions/agentframework/services/__init__.py b/tests/tooling/extensions/agentframework/services/__init__.py index c167a2ed..7d0206f4 100644 --- a/tests/tooling/extensions/agentframework/services/__init__.py +++ b/tests/tooling/extensions/agentframework/services/__init__.py @@ -1,3 +1,4 @@ -# Copyright (c) Microsoft. All rights reserved. +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. """Test package for Agent Framework tooling extension services.""" diff --git a/tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py b/tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py index 4f5f029b..04428711 100644 --- a/tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py +++ b/tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py @@ -1,4 +1,5 @@ -# Copyright (c) Microsoft. All rights reserved. +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. """Unit tests for send_chat_history_async methods in McpToolRegistrationService.""" From 9684d9a366a2f4b40f4c2ac7db81ab00d22eef47 Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Sat, 24 Jan 2026 10:31:00 -0800 Subject: [PATCH 3/5] fix(mcp_tool_registration_service): add warning for messages with missing role during conversion --- .../agentframework/services/mcp_tool_registration_service.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py b/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py index 614de4dc..618fd8f5 100644 --- a/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py +++ b/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py @@ -188,6 +188,11 @@ def _convert_chat_messages_to_history( for msg in chat_messages: message_id = msg.message_id if msg.message_id is not None else str(uuid.uuid4()) + if msg.role is None: + self._logger.warning( + f"Skipping message {message_id} with missing role during conversion" + ) + continue role = msg.role.value content = msg.text if msg.text is not None else "" From 4d86ab7ca62825a6be946df6c89a18baa65159ea Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Sat, 24 Jan 2026 18:38:13 -0800 Subject: [PATCH 4/5] Code review fixes/pr 132 (#134) * refactor(agentframework): remove _async suffix from method names (CRM-010) Rename methods to follow Python conventions and codebase patterns: - send_chat_history_messages_async -> send_chat_history_messages - send_chat_history_async -> send_chat_history_from_store Rename test file accordingly: - test_send_chat_history_async.py -> test_send_chat_history.py Co-Authored-By: Claude Opus 4.5 * fix(agentframework): improve role handling and logging (CRM-003, CRM-009, CRM-013) - CRM-003: Add defensive handling for role value access using hasattr check - CRM-009: Convert debug and warning logging to lazy format (% style) - CRM-013: Simplify redundant empty content check (remove `not content or`) Co-Authored-By: Claude Opus 4.5 * test(agentframework): add missing test coverage (CRM-001, 004, 005, 006, 011, 012) - CRM-001: Add test for ChatMessageStore exception propagation - CRM-004: Add test for whitespace-only content filtering - CRM-005: Add test for None role handling - CRM-006: Add test for all messages filtered out scenario - CRM-011: Add test for default ToolOptions creation - CRM-012: Make UUID assertion more robust using uuid.UUID() Also adds test for defensive role handling (string role without .value) Co-Authored-By: Claude Opus 4.5 * fix(tooling): return success for empty chat history list (CRM-008) The core send_chat_history method now returns OperationResult.success() for empty lists instead of raising ValueError. This is consistent with the extension behavior and makes the API more forgiving. Updated test to verify new behavior. Co-Authored-By: Claude Opus 4.5 * docs(agentframework): add Chat History API documentation (CRM-002) Update design.md to include: - send_chat_history_messages and send_chat_history_from_store methods - Parameter tables for both methods - Integration flow diagram showing message conversion - Message filtering behavior documentation - Example usage code Co-Authored-By: Claude Opus 4.5 * docs: add async method naming convention to CLAUDE.md (CRM-010) Document that _async suffix should NOT be used on async methods in this SDK since we only provide async versions. This prevents future naming inconsistencies. Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Johan Broberg Co-authored-by: Claude Opus 4.5 --- CLAUDE.md | 1 + .../docs/design.md | 81 ++++++ .../services/mcp_tool_registration_service.py | 25 +- .../mcp_tool_server_configuration_service.py | 9 +- ...ory_async.py => test_send_chat_history.py} | 256 ++++++++++++++---- .../services/test_send_chat_history.py | 17 +- 6 files changed, 310 insertions(+), 79 deletions(-) rename tests/tooling/extensions/agentframework/services/{test_send_chat_history_async.py => test_send_chat_history.py} (56%) diff --git a/CLAUDE.md b/CLAUDE.md index e0772348..5450bd95 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -173,6 +173,7 @@ Place it before imports with one blank line after. - Use explicit `None` checks: `if x is not None:` not `if x:` - Local imports should be moved to top of file - Return defensive copies of mutable data to protect singletons +- **Async method naming**: Do NOT use `_async` suffix on async methods. The `_async` suffix is only appropriate when providing both sync and async versions of the same method. Since this SDK is async-only, use plain method names (e.g., `send_chat_history_messages` not `send_chat_history_messages_async`) ## CI/CD diff --git a/libraries/microsoft-agents-a365-tooling-extensions-agentframework/docs/design.md b/libraries/microsoft-agents-a365-tooling-extensions-agentframework/docs/design.md index 569f5860..7d8af5ee 100644 --- a/libraries/microsoft-agents-a365-tooling-extensions-agentframework/docs/design.md +++ b/libraries/microsoft-agents-a365-tooling-extensions-agentframework/docs/design.md @@ -76,6 +76,87 @@ mcp_tool = MCPStreamableHTTPTool( ) ``` +### Chat History API + +The service provides methods to send chat history to the MCP platform for real-time threat protection analysis. This enables security scanning of conversation content. + +#### send_chat_history_messages + +The primary method for sending chat history. Converts Agent Framework `ChatMessage` objects to the `ChatHistoryMessage` format expected by the MCP platform. + +```python +from agent_framework import ChatMessage, Role + +service = McpToolRegistrationService() + +# Create messages +messages = [ + ChatMessage(role=Role.USER, text="Hello, how are you?"), + ChatMessage(role=Role.ASSISTANT, text="I'm doing well, thank you!"), +] + +# Send to MCP platform for threat protection +result = await service.send_chat_history_messages(messages, turn_context) + +if result.succeeded: + print("Chat history sent successfully") +else: + print(f"Failed: {result.errors}") +``` + +#### send_chat_history_from_store + +A convenience method that extracts messages from a `ChatMessageStoreProtocol` and delegates to `send_chat_history_messages`. + +```python +# Using a ChatMessageStore directly +result = await service.send_chat_history_from_store( + thread.chat_message_store, + turn_context +) +``` + +#### Chat History API Parameters + +| Method | Parameter | Type | Description | +|--------|-----------|------|-------------| +| `send_chat_history_messages` | `chat_messages` | `Sequence[ChatMessage]` | Messages to send | +| | `turn_context` | `TurnContext` | Conversation context | +| | `tool_options` | `ToolOptions \| None` | Optional configuration | +| `send_chat_history_from_store` | `chat_message_store` | `ChatMessageStoreProtocol` | Message store | +| | `turn_context` | `TurnContext` | Conversation context | +| | `tool_options` | `ToolOptions \| None` | Optional configuration | + +#### Chat History Integration Flow + +``` +Agent Framework ChatMessage objects + │ + ▼ +McpToolRegistrationService.send_chat_history_messages() + │ + ├── Convert ChatMessage → ChatHistoryMessage + │ ├── Extract role via .value property + │ ├── Generate UUID if message_id is None + │ ├── Filter out empty/whitespace content + │ └── Filter out None roles + │ + ▼ +McpToolServerConfigurationService.send_chat_history() + │ + ▼ +MCP Platform Real-Time Threat Protection Endpoint +``` + +#### Message Filtering Behavior + +The conversion process filters out invalid messages: +- Messages with `None` role are skipped (logged at WARNING level) +- Messages with empty or whitespace-only content are skipped +- If all messages are filtered out, the method returns success without calling the backend + +This ensures only valid, meaningful messages are sent for threat analysis. + ## File Structure ``` diff --git a/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py b/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py index 618fd8f5..09cceafa 100644 --- a/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py +++ b/libraries/microsoft-agents-a365-tooling-extensions-agentframework/microsoft_agents_a365/tooling/extensions/agentframework/services/mcp_tool_registration_service.py @@ -190,16 +190,17 @@ def _convert_chat_messages_to_history( message_id = msg.message_id if msg.message_id is not None else str(uuid.uuid4()) if msg.role is None: self._logger.warning( - f"Skipping message {message_id} with missing role during conversion" + "Skipping message %s with missing role during conversion", message_id ) continue - role = msg.role.value + # Defensive handling: use .value if role is an enum, otherwise convert to string + role = msg.role.value if hasattr(msg.role, "value") else str(msg.role) content = msg.text if msg.text is not None else "" # Skip messages with empty content as ChatHistoryMessage validates non-empty content - if not content or not content.strip(): + if not content.strip(): self._logger.warning( - f"Skipping message {message_id} with empty content during conversion" + "Skipping message %s with empty content during conversion", message_id ) continue @@ -212,12 +213,12 @@ def _convert_chat_messages_to_history( history_messages.append(history_message) self._logger.debug( - f"Converted message {message_id} with role '{role}' to ChatHistoryMessage" + "Converted message %s with role '%s' to ChatHistoryMessage", message_id, role ) return history_messages - async def send_chat_history_messages_async( + async def send_chat_history_messages( self, chat_messages: Sequence[ChatMessage], turn_context: TurnContext, @@ -244,7 +245,7 @@ async def send_chat_history_messages_async( Example: >>> service = McpToolRegistrationService() >>> messages = [ChatMessage(role=Role.USER, text="Hello")] - >>> result = await service.send_chat_history_messages_async(messages, turn_context) + >>> result = await service.send_chat_history_messages(messages, turn_context) >>> if result.succeeded: ... print("Chat history sent successfully") """ @@ -257,7 +258,7 @@ async def send_chat_history_messages_async( # Handle empty messages - return success with warning if len(chat_messages) == 0: - self._logger.warning("Empty message list provided to send_chat_history_messages_async") + self._logger.warning("Empty message list provided to send_chat_history_messages") return OperationResult.success() self._logger.info(f"Send chat history initiated with {len(chat_messages)} messages") @@ -290,7 +291,7 @@ async def send_chat_history_messages_async( return result - async def send_chat_history_async( + async def send_chat_history_from_store( self, chat_message_store: ChatMessageStoreProtocol, turn_context: TurnContext, @@ -300,7 +301,7 @@ async def send_chat_history_async( Send chat history from a ChatMessageStore to the MCP platform. This is a convenience method that extracts messages from the store - and delegates to send_chat_history_messages_async(). + and delegates to send_chat_history_messages(). Args: chat_message_store: ChatMessageStore containing the conversation history. @@ -315,7 +316,7 @@ async def send_chat_history_async( Example: >>> service = McpToolRegistrationService() - >>> result = await service.send_chat_history_async( + >>> result = await service.send_chat_history_from_store( ... thread.chat_message_store, turn_context ... ) """ @@ -330,7 +331,7 @@ async def send_chat_history_async( messages = await chat_message_store.list_messages() # Delegate to the primary implementation - return await self.send_chat_history_messages_async( + return await self.send_chat_history_messages( chat_messages=messages, turn_context=turn_context, tool_options=tool_options, diff --git a/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/services/mcp_tool_server_configuration_service.py b/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/services/mcp_tool_server_configuration_service.py index df1ae5b2..275ab69f 100644 --- a/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/services/mcp_tool_server_configuration_service.py +++ b/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/services/mcp_tool_server_configuration_service.py @@ -561,8 +561,13 @@ async def send_chat_history( # Validate input parameters if turn_context is None: raise ValueError("turn_context cannot be None") - if chat_history_messages is None or len(chat_history_messages) == 0: - raise ValueError("chat_history_messages cannot be None or empty") + if chat_history_messages is None: + raise ValueError("chat_history_messages cannot be None") + + # Handle empty messages - return success with warning (consistent with extension behavior) + if len(chat_history_messages) == 0: + self._logger.warning("Empty message list provided to send_chat_history") + return OperationResult.success() # Extract required information from turn context if not turn_context.activity: diff --git a/tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py b/tests/tooling/extensions/agentframework/services/test_send_chat_history.py similarity index 56% rename from tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py rename to tests/tooling/extensions/agentframework/services/test_send_chat_history.py index 04428711..6c88bc59 100644 --- a/tests/tooling/extensions/agentframework/services/test_send_chat_history_async.py +++ b/tests/tooling/extensions/agentframework/services/test_send_chat_history.py @@ -1,7 +1,7 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -"""Unit tests for send_chat_history_async methods in McpToolRegistrationService.""" +"""Unit tests for send_chat_history_from_store methods in McpToolRegistrationService.""" # Direct import from service file to work around namespace package resolution issues # with editable installs in the uv workspace @@ -26,6 +26,7 @@ _spec.loader.exec_module(_module) McpToolRegistrationService = _module.McpToolRegistrationService +import uuid # noqa: E402 from datetime import UTC, datetime # noqa: E402 from unittest.mock import AsyncMock, Mock, patch # noqa: E402 @@ -36,7 +37,7 @@ class TestSendChatHistoryAsync: - """Tests for send_chat_history_messages_async and send_chat_history_async methods.""" + """Tests for send_chat_history_messages and send_chat_history_from_store methods.""" @pytest.fixture def mock_turn_context(self): @@ -103,46 +104,48 @@ def service(self): @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_validates_messages_none( + async def test_send_chat_history_messages_validates_messages_none( self, service, mock_turn_context ): - """Test that send_chat_history_messages_async raises ValueError for None messages.""" + """Test that send_chat_history_messages raises ValueError for None messages.""" with pytest.raises(ValueError, match="chat_messages cannot be None"): - await service.send_chat_history_messages_async(None, mock_turn_context) + await service.send_chat_history_messages(None, mock_turn_context) @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_validates_turn_context_none( + async def test_send_chat_history_messages_validates_turn_context_none( self, service, sample_chat_messages ): - """Test that send_chat_history_messages_async raises ValueError for None turn_context.""" + """Test that send_chat_history_messages raises ValueError for None turn_context.""" with pytest.raises(ValueError, match="turn_context cannot be None"): - await service.send_chat_history_messages_async(sample_chat_messages, None) + await service.send_chat_history_messages(sample_chat_messages, None) @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_async_validates_store_none(self, service, mock_turn_context): - """Test that send_chat_history_async raises ValueError for None store.""" + async def test_send_chat_history_from_store_validates_store_none( + self, service, mock_turn_context + ): + """Test that send_chat_history_from_store raises ValueError for None store.""" with pytest.raises(ValueError, match="chat_message_store cannot be None"): - await service.send_chat_history_async(None, mock_turn_context) + await service.send_chat_history_from_store(None, mock_turn_context) @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_async_validates_turn_context_none( + async def test_send_chat_history_from_store_validates_turn_context_none( self, service, mock_chat_message_store ): - """Test that send_chat_history_async raises ValueError for None turn_context.""" + """Test that send_chat_history_from_store raises ValueError for None turn_context.""" with pytest.raises(ValueError, match="turn_context cannot be None"): - await service.send_chat_history_async(mock_chat_message_store, None) + await service.send_chat_history_from_store(mock_chat_message_store, None) @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_empty_messages_returns_success( + async def test_send_chat_history_messages_empty_messages_returns_success( self, service, mock_turn_context ): """Test that empty message list returns success with warning log.""" # Act - result = await service.send_chat_history_messages_async([], mock_turn_context) + result = await service.send_chat_history_messages([], mock_turn_context) # Assert assert result.succeeded is True @@ -151,7 +154,7 @@ async def test_send_chat_history_messages_async_empty_messages_returns_success( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_generates_uuid_for_missing_id( + async def test_send_chat_history_messages_generates_uuid_for_missing_id( self, service, mock_turn_context, mock_role ): """Test that UUID is generated when message_id is None.""" @@ -162,26 +165,27 @@ async def test_send_chat_history_messages_async_generates_uuid_for_missing_id( msg.text = "Hello" # Act - await service.send_chat_history_messages_async([msg], mock_turn_context) + await service.send_chat_history_messages([msg], mock_turn_context) # Assert call_args = service._mcp_server_configuration_service.send_chat_history.call_args history_messages = call_args.kwargs["chat_history_messages"] assert len(history_messages) == 1 - # Verify a UUID was generated (not None and has UUID format) + # Verify a UUID was generated (not None and valid UUID format) assert history_messages[0].id is not None - assert len(history_messages[0].id) == 36 # UUID format: 8-4-4-4-12 + # Use uuid.UUID() to validate format - raises ValueError if invalid + uuid.UUID(history_messages[0].id) @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_generates_timestamp( + async def test_send_chat_history_messages_generates_timestamp( self, service, mock_turn_context, sample_chat_messages ): """Test that current UTC timestamp is generated for messages.""" # Act before_time = datetime.now(UTC) - await service.send_chat_history_messages_async(sample_chat_messages, mock_turn_context) + await service.send_chat_history_messages(sample_chat_messages, mock_turn_context) after_time = datetime.now(UTC) # Assert @@ -194,7 +198,7 @@ async def test_send_chat_history_messages_async_generates_timestamp( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_handles_missing_text( + async def test_send_chat_history_messages_handles_missing_text( self, service, mock_turn_context, mock_role ): """Test that messages with None text are skipped (empty content not allowed).""" @@ -210,7 +214,7 @@ async def test_send_chat_history_messages_async_handles_missing_text( msg_without_text.text = None # No text # Act - await service.send_chat_history_messages_async( + await service.send_chat_history_messages( [msg_with_text, msg_without_text], mock_turn_context ) @@ -226,14 +230,12 @@ async def test_send_chat_history_messages_async_handles_missing_text( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_success( + async def test_send_chat_history_messages_success( self, service, mock_turn_context, sample_chat_messages ): - """Test successful send_chat_history_messages_async call.""" + """Test successful send_chat_history_messages call.""" # Act - result = await service.send_chat_history_messages_async( - sample_chat_messages, mock_turn_context - ) + result = await service.send_chat_history_messages(sample_chat_messages, mock_turn_context) # Assert assert result.succeeded is True @@ -242,12 +244,14 @@ async def test_send_chat_history_messages_async_success( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_async_with_store_success( + async def test_send_chat_history_from_store_with_store_success( self, service, mock_turn_context, mock_chat_message_store ): - """Test successful send_chat_history_async call with ChatMessageStore.""" + """Test successful send_chat_history_from_store call with ChatMessageStore.""" # Act - result = await service.send_chat_history_async(mock_chat_message_store, mock_turn_context) + result = await service.send_chat_history_from_store( + mock_chat_message_store, mock_turn_context + ) # Assert assert result.succeeded is True @@ -256,18 +260,18 @@ async def test_send_chat_history_async_with_store_success( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_async_delegates_to_messages_async( + async def test_send_chat_history_from_store_delegates_to_messages_async( self, service, mock_turn_context, mock_chat_message_store, sample_chat_messages ): - """Test that send_chat_history_async delegates to send_chat_history_messages_async.""" + """Test that send_chat_history_from_store delegates to send_chat_history_messages.""" # Arrange with patch.object( - service, "send_chat_history_messages_async", new_callable=AsyncMock + service, "send_chat_history_messages", new_callable=AsyncMock ) as mock_messages_method: mock_messages_method.return_value = OperationResult.success() # Act - result = await service.send_chat_history_async( + result = await service.send_chat_history_from_store( mock_chat_message_store, mock_turn_context ) @@ -282,7 +286,7 @@ async def test_send_chat_history_async_delegates_to_messages_async( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_with_tool_options( + async def test_send_chat_history_messages_with_tool_options( self, service, mock_turn_context, sample_chat_messages ): """Test that ToolOptions are passed correctly to core service.""" @@ -290,7 +294,7 @@ async def test_send_chat_history_messages_async_with_tool_options( options = ToolOptions(orchestrator_name="TestOrchestrator") # Act - await service.send_chat_history_messages_async( + await service.send_chat_history_messages( sample_chat_messages, mock_turn_context, tool_options=options ) @@ -300,12 +304,12 @@ async def test_send_chat_history_messages_async_with_tool_options( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_converts_messages_correctly( + async def test_send_chat_history_messages_converts_messages_correctly( self, service, mock_turn_context, sample_chat_messages ): """Test that ChatMessage objects are correctly converted to ChatHistoryMessage.""" # Act - await service.send_chat_history_messages_async(sample_chat_messages, mock_turn_context) + await service.send_chat_history_messages(sample_chat_messages, mock_turn_context) # Assert call_args = service._mcp_server_configuration_service.send_chat_history.call_args @@ -329,10 +333,10 @@ async def test_send_chat_history_messages_async_converts_messages_correctly( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_handles_http_error( + async def test_send_chat_history_messages_handles_http_error( self, service, mock_turn_context, sample_chat_messages ): - """Test send_chat_history_messages_async handles HTTP errors from core service.""" + """Test send_chat_history_messages handles HTTP errors from core service.""" # Arrange error = OperationError(Exception("500, Internal Server Error")) service._mcp_server_configuration_service.send_chat_history = AsyncMock( @@ -340,9 +344,7 @@ async def test_send_chat_history_messages_async_handles_http_error( ) # Act - result = await service.send_chat_history_messages_async( - sample_chat_messages, mock_turn_context - ) + result = await service.send_chat_history_messages(sample_chat_messages, mock_turn_context) # Assert assert result.succeeded is False @@ -351,10 +353,10 @@ async def test_send_chat_history_messages_async_handles_http_error( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_handles_timeout( + async def test_send_chat_history_messages_handles_timeout( self, service, mock_turn_context, sample_chat_messages ): - """Test send_chat_history_messages_async handles timeout errors.""" + """Test send_chat_history_messages handles timeout errors.""" # Arrange error = OperationError(Exception("Request timed out")) service._mcp_server_configuration_service.send_chat_history = AsyncMock( @@ -362,9 +364,7 @@ async def test_send_chat_history_messages_async_handles_timeout( ) # Act - result = await service.send_chat_history_messages_async( - sample_chat_messages, mock_turn_context - ) + result = await service.send_chat_history_messages(sample_chat_messages, mock_turn_context) # Assert assert result.succeeded is False @@ -373,10 +373,10 @@ async def test_send_chat_history_messages_async_handles_timeout( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_handles_connection_error( + async def test_send_chat_history_messages_handles_connection_error( self, service, mock_turn_context, sample_chat_messages ): - """Test send_chat_history_messages_async handles connection errors.""" + """Test send_chat_history_messages handles connection errors.""" # Arrange error = OperationError(Exception("Connection failed")) service._mcp_server_configuration_service.send_chat_history = AsyncMock( @@ -384,9 +384,7 @@ async def test_send_chat_history_messages_async_handles_connection_error( ) # Act - result = await service.send_chat_history_messages_async( - sample_chat_messages, mock_turn_context - ) + result = await service.send_chat_history_messages(sample_chat_messages, mock_turn_context) # Assert assert result.succeeded is False @@ -395,7 +393,7 @@ async def test_send_chat_history_messages_async_handles_connection_error( @pytest.mark.asyncio @pytest.mark.unit - async def test_send_chat_history_messages_async_role_value_conversion( + async def test_send_chat_history_messages_role_value_conversion( self, service, mock_turn_context ): """Test that Role.value is used for string conversion.""" @@ -425,7 +423,7 @@ async def test_send_chat_history_messages_async_role_value_conversion( msg3.text = "Assistant response" # Act - await service.send_chat_history_messages_async([msg1, msg2, msg3], mock_turn_context) + await service.send_chat_history_messages([msg1, msg2, msg3], mock_turn_context) # Assert call_args = service._mcp_server_configuration_service.send_chat_history.call_args @@ -435,3 +433,147 @@ async def test_send_chat_history_messages_async_role_value_conversion( assert history_messages[0].role == "system" assert history_messages[1].role == "user" assert history_messages[2].role == "assistant" + + # ==================== Additional Coverage Tests (CRM-001, 004, 005, 006, 011) ==================== + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_from_store_propagates_store_exception( + self, service, mock_turn_context + ): + """Test that exceptions from chat_message_store.list_messages() propagate (CRM-001).""" + # Arrange + mock_store = AsyncMock() + mock_store.list_messages = AsyncMock(side_effect=RuntimeError("Store connection failed")) + + # Act & Assert + with pytest.raises(RuntimeError, match="Store connection failed"): + await service.send_chat_history_from_store(mock_store, mock_turn_context) + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_skips_whitespace_only_content( + self, service, mock_turn_context, mock_role + ): + """Test that messages with whitespace-only content are filtered out (CRM-004).""" + # Arrange + msg_with_text = Mock() + msg_with_text.message_id = "msg-1" + msg_with_text.role = mock_role + msg_with_text.text = "Valid content" + + msg_whitespace_only = Mock() + msg_whitespace_only.message_id = "msg-2" + msg_whitespace_only.role = mock_role + msg_whitespace_only.text = " \t\n " # Whitespace only + + # Act + await service.send_chat_history_messages( + [msg_with_text, msg_whitespace_only], mock_turn_context + ) + + # Assert + call_args = service._mcp_server_configuration_service.send_chat_history.call_args + history_messages = call_args.kwargs["chat_history_messages"] + + # Only the message with actual content should be included + assert len(history_messages) == 1 + assert history_messages[0].content == "Valid content" + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_skips_messages_with_none_role( + self, service, mock_turn_context, mock_role + ): + """Test that messages with None role are filtered out (CRM-005).""" + # Arrange + msg_with_role = Mock() + msg_with_role.message_id = "msg-1" + msg_with_role.role = mock_role + msg_with_role.text = "Valid message" + + msg_without_role = Mock() + msg_without_role.message_id = "msg-2" + msg_without_role.role = None # No role + msg_without_role.text = "This should be skipped" + + # Act + await service.send_chat_history_messages( + [msg_with_role, msg_without_role], mock_turn_context + ) + + # Assert + call_args = service._mcp_server_configuration_service.send_chat_history.call_args + history_messages = call_args.kwargs["chat_history_messages"] + + # Only the message with a role should be included + assert len(history_messages) == 1 + assert history_messages[0].content == "Valid message" + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_all_filtered_returns_success( + self, service, mock_turn_context, mock_role + ): + """Test that all messages filtered out returns success without calling core (CRM-006).""" + # Arrange - all messages have empty content + msg1 = Mock() + msg1.message_id = "msg-1" + msg1.role = mock_role + msg1.text = "" # Empty + + msg2 = Mock() + msg2.message_id = "msg-2" + msg2.role = mock_role + msg2.text = " " # Whitespace only + + msg3 = Mock() + msg3.message_id = "msg-3" + msg3.role = None # None role + msg3.text = "Valid text but no role" + + # Act + result = await service.send_chat_history_messages([msg1, msg2, msg3], mock_turn_context) + + # Assert + assert result.succeeded is True + # Core service should not be called when all messages are filtered out + service._mcp_server_configuration_service.send_chat_history.assert_not_called() + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_creates_default_tool_options( + self, service, mock_turn_context, sample_chat_messages + ): + """Test that default ToolOptions with AgentFramework orchestrator is created (CRM-011).""" + # Act - call without providing tool_options + await service.send_chat_history_messages(sample_chat_messages, mock_turn_context) + + # Assert + call_args = service._mcp_server_configuration_service.send_chat_history.call_args + options = call_args.kwargs["options"] + + assert options is not None + assert options.orchestrator_name == "AgentFramework" + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_send_chat_history_messages_handles_role_without_value_attribute( + self, service, mock_turn_context + ): + """Test defensive handling when role doesn't have .value attribute (CRM-003).""" + # Arrange - role is a plain string, not an enum + msg = Mock() + msg.message_id = "msg-1" + msg.role = "user" # String, not an enum with .value + msg.text = "Hello" + + # Act + await service.send_chat_history_messages([msg], mock_turn_context) + + # Assert + call_args = service._mcp_server_configuration_service.send_chat_history.call_args + history_messages = call_args.kwargs["chat_history_messages"] + + assert len(history_messages) == 1 + assert history_messages[0].role == "user" diff --git a/tests/tooling/services/test_send_chat_history.py b/tests/tooling/services/test_send_chat_history.py index 67f0e4e3..28d1b2a4 100644 --- a/tests/tooling/services/test_send_chat_history.py +++ b/tests/tooling/services/test_send_chat_history.py @@ -142,17 +142,18 @@ async def test_send_chat_history_validates_chat_history_messages( ): """Test that send_chat_history validates chat_history_messages parameter.""" # Act & Assert - with pytest.raises(ValueError, match="chat_history_messages cannot be None or empty"): + with pytest.raises(ValueError, match="chat_history_messages cannot be None"): await service.send_chat_history(mock_turn_context, None) @pytest.mark.asyncio - async def test_send_chat_history_validates_empty_chat_history_list( - self, service, mock_turn_context - ): - """Test that send_chat_history validates empty chat_history list.""" - # Act & Assert - with pytest.raises(ValueError, match="chat_history_messages cannot be None or empty"): - await service.send_chat_history(mock_turn_context, []) + async def test_send_chat_history_empty_list_returns_success(self, service, mock_turn_context): + """Test that send_chat_history returns success for empty list (CRM-008).""" + # Act + result = await service.send_chat_history(mock_turn_context, []) + + # Assert - empty list should return success, not raise exception + assert result.succeeded is True + assert len(result.errors) == 0 @pytest.mark.asyncio async def test_send_chat_history_validates_activity(self, service, chat_history_messages): From 5026d01736e60036d1869cce7b103d74833b1802 Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Mon, 26 Jan 2026 11:43:43 -0800 Subject: [PATCH 5/5] Refactor test_send_chat_history.py to remove workaround for namespace package resolution and clean up imports --- .../microsoft_agents_a365/tooling/__init__.py | 3 ++ .../services/test_send_chat_history.py | 40 +++++-------------- 2 files changed, 13 insertions(+), 30 deletions(-) diff --git a/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/__init__.py b/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/__init__.py index 567db1bb..edee7a4a 100644 --- a/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/__init__.py +++ b/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/__init__.py @@ -27,3 +27,6 @@ "get_mcp_base_url", "build_mcp_server_url", ] + +# Enable namespace package extension for tooling-extensions-* packages +__path__ = __import__("pkgutil").extend_path(__path__, __name__) diff --git a/tests/tooling/extensions/agentframework/services/test_send_chat_history.py b/tests/tooling/extensions/agentframework/services/test_send_chat_history.py index 6c88bc59..f3a780a3 100644 --- a/tests/tooling/extensions/agentframework/services/test_send_chat_history.py +++ b/tests/tooling/extensions/agentframework/services/test_send_chat_history.py @@ -3,37 +3,17 @@ """Unit tests for send_chat_history_from_store methods in McpToolRegistrationService.""" -# Direct import from service file to work around namespace package resolution issues -# with editable installs in the uv workspace -import importlib.util -import sys -from pathlib import Path - -_service_path = ( - Path(__file__).parent.parent.parent.parent.parent.parent - / "libraries" - / "microsoft-agents-a365-tooling-extensions-agentframework" - / "microsoft_agents_a365" - / "tooling" - / "extensions" - / "agentframework" - / "services" - / "mcp_tool_registration_service.py" +import uuid +from datetime import UTC, datetime +from unittest.mock import AsyncMock, Mock, patch + +import pytest +from microsoft_agents.hosting.core import TurnContext +from microsoft_agents_a365.runtime import OperationError, OperationResult +from microsoft_agents_a365.tooling.extensions.agentframework.services import ( + McpToolRegistrationService, ) -_spec = importlib.util.spec_from_file_location("mcp_tool_registration_service", _service_path) -_module = importlib.util.module_from_spec(_spec) -sys.modules["mcp_tool_registration_service"] = _module -_spec.loader.exec_module(_module) -McpToolRegistrationService = _module.McpToolRegistrationService - -import uuid # noqa: E402 -from datetime import UTC, datetime # noqa: E402 -from unittest.mock import AsyncMock, Mock, patch # noqa: E402 - -import pytest # noqa: E402 -from microsoft_agents.hosting.core import TurnContext # noqa: E402 -from microsoft_agents_a365.runtime import OperationError, OperationResult # noqa: E402 -from microsoft_agents_a365.tooling.models import ToolOptions # noqa: E402 +from microsoft_agents_a365.tooling.models import ToolOptions class TestSendChatHistoryAsync: