From 17c1efdcf482a3f1c52eae8e40fae7730ece5361 Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Sat, 17 Jan 2026 13:32:48 -0800 Subject: [PATCH 1/6] Fix inconsistent string representation for failed result with no errors Improves the __str__ method of OperationResult to return "Failed" instead of "Failed : " when there are no errors. Also removes the extra space before the colon for consistency. Addresses code review Comment 1. Co-Authored-By: Claude --- .../microsoft_agents_a365/runtime/operation_result.py | 2 +- tests/runtime/test_operation_result.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/microsoft-agents-a365-runtime/microsoft_agents_a365/runtime/operation_result.py b/libraries/microsoft-agents-a365-runtime/microsoft_agents_a365/runtime/operation_result.py index 19859eca..ae966d4f 100644 --- a/libraries/microsoft-agents-a365-runtime/microsoft_agents_a365/runtime/operation_result.py +++ b/libraries/microsoft-agents-a365-runtime/microsoft_agents_a365/runtime/operation_result.py @@ -88,4 +88,4 @@ def __str__(self) -> str: return "Succeeded" else: error_messages = ", ".join(str(error.message) for error in self._errors) - return f"Failed : {error_messages}" + return f"Failed: {error_messages}" if error_messages else "Failed" diff --git a/tests/runtime/test_operation_result.py b/tests/runtime/test_operation_result.py index 77c8a3dc..61edbed9 100644 --- a/tests/runtime/test_operation_result.py +++ b/tests/runtime/test_operation_result.py @@ -85,7 +85,7 @@ def test_operation_result_failed_string_representation_no_errors(self): result = OperationResult.failed() # Assert - assert str(result) == "Failed : " + assert str(result) == "Failed" def test_operation_result_failed_string_representation_with_errors(self): """Test that failed OperationResult with errors has correct string representation.""" From 9ad5bdd469646ca174563b7cf166354bb334a8c0 Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Sat, 17 Jan 2026 13:33:19 -0800 Subject: [PATCH 2/6] Return defensive copy of errors list to protect singleton The errors property now returns a copy of the internal errors list to protect the singleton instance returned by success() from accidental modification. Updated docstring to document this behavior. Addresses code review Comment 2. Co-Authored-By: Claude --- .../microsoft_agents_a365/runtime/operation_result.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libraries/microsoft-agents-a365-runtime/microsoft_agents_a365/runtime/operation_result.py b/libraries/microsoft-agents-a365-runtime/microsoft_agents_a365/runtime/operation_result.py index ae966d4f..5a0187b0 100644 --- a/libraries/microsoft-agents-a365-runtime/microsoft_agents_a365/runtime/operation_result.py +++ b/libraries/microsoft-agents-a365-runtime/microsoft_agents_a365/runtime/operation_result.py @@ -47,9 +47,11 @@ def errors(self) -> List[OperationError]: Get the list of errors that occurred during the operation. Returns: - List[OperationError]: List of operation errors. + 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 self._errors + return list(self._errors) @staticmethod def success() -> "OperationResult": From c5ef2439f0dc2b8879f446f79f40e41e758e7b9b Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Sat, 17 Jan 2026 13:34:01 -0800 Subject: [PATCH 3/6] Use explicit None check for timestamp validation Changed timestamp validation from falsy check (if not self.timestamp) to explicit None check (if self.timestamp is None) for safer and more intentional validation behavior. Updated error message and test accordingly. Addresses code review Comment 3. Co-Authored-By: Claude --- .../tooling/models/chat_history_message.py | 4 ++-- tests/tooling/models/test_chat_history_message.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/models/chat_history_message.py b/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/models/chat_history_message.py index bd2fcf9c..56aeaa89 100644 --- a/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/models/chat_history_message.py +++ b/libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/models/chat_history_message.py @@ -38,8 +38,8 @@ def __post_init__(self): raise ValueError("role cannot be empty") if not self.content: raise ValueError("content cannot be empty") - if not self.timestamp: - raise ValueError("timestamp cannot be empty") + if self.timestamp is None: + raise ValueError("timestamp cannot be None") def to_dict(self): """ diff --git a/tests/tooling/models/test_chat_history_message.py b/tests/tooling/models/test_chat_history_message.py index bbd00cb2..570e6141 100644 --- a/tests/tooling/models/test_chat_history_message.py +++ b/tests/tooling/models/test_chat_history_message.py @@ -70,7 +70,7 @@ def test_chat_history_message_requires_non_empty_content(self): def test_chat_history_message_requires_timestamp(self): """Test that ChatHistoryMessage requires a timestamp.""" # Act & Assert - with pytest.raises(ValueError, match="timestamp cannot be empty"): + with pytest.raises(ValueError, match="timestamp cannot be None"): ChatHistoryMessage("msg-001", "user", "Test content", None) def test_chat_history_message_supports_system_role(self): From c093e78899de0ea36f049d39d53bd3242e14612b Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Sat, 17 Jan 2026 13:34:41 -0800 Subject: [PATCH 4/6] Move local imports to top of file Moved OperationError, OperationResult, ChatMessageRequest, and get_chat_history_endpoint imports from inside the send_chat_history method to the top of the file with the other imports. Removed the misleading comment about circular dependencies as there is no cycle in the import graph. Addresses code review Comment 4. Co-Authored-By: Claude --- .../mcp_tool_server_configuration_service.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) 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 bcaeb12e..7902e322 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 @@ -31,11 +31,16 @@ from microsoft_agents.hosting.core import TurnContext # Local imports -from ..models import ChatHistoryMessage, MCPServerConfig, ToolOptions +from ..models import ChatHistoryMessage, ChatMessageRequest, MCPServerConfig, ToolOptions from ..utils import Constants -from ..utils.utility import get_tooling_gateway_for_digital_worker, build_mcp_server_url +from ..utils.utility import ( + get_tooling_gateway_for_digital_worker, + build_mcp_server_url, + get_chat_history_endpoint, +) # Runtime Imports +from microsoft_agents_a365.runtime import OperationError, OperationResult from microsoft_agents_a365.runtime.utility import Utility as RuntimeUtility @@ -520,12 +525,6 @@ async def send_chat_history( Raises: ValueError: If required parameters are invalid or empty. """ - # Import here to avoid circular dependency - from microsoft_agents_a365.runtime import OperationError, OperationResult - - from ..models import ChatMessageRequest - from ..utils.utility import get_chat_history_endpoint - # Validate input parameters if not turn_context: raise ValueError("turn_context cannot be empty or None") From 886175b67e142be8ca2052c2bf2940dda9aca8cc Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Sat, 17 Jan 2026 13:35:09 -0800 Subject: [PATCH 5/6] Change endpoint URL log level from INFO to DEBUG Detailed operational information like endpoint URLs should be logged at DEBUG level rather than INFO level. INFO level is reserved for higher-level operation status messages. Addresses code review Comment 5. Co-Authored-By: Claude --- .../tooling/services/mcp_tool_server_configuration_service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 7902e322..ace0b466 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 @@ -559,7 +559,7 @@ async def send_chat_history( # Get the endpoint URL endpoint = get_chat_history_endpoint() - self._logger.info(f"Sending chat history to endpoint: {endpoint}") + self._logger.debug(f"Sending chat history to endpoint: {endpoint}") # Create the request payload request = ChatMessageRequest( From 897dd64c5883d546062905366ac3a82a42e593c4 Mon Sep 17 00:00:00 2001 From: Johan Broberg Date: Sat, 17 Jan 2026 13:35:49 -0800 Subject: [PATCH 6/6] Use consistent async test pattern for validation tests Converted validation tests from synchronous methods using asyncio.run() to async methods with @pytest.mark.asyncio decorator for consistency with the other tests in the test suite. Addresses code review Comment 7. Co-Authored-By: Claude --- .../services/test_send_chat_history.py | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/tests/tooling/services/test_send_chat_history.py b/tests/tooling/services/test_send_chat_history.py index 90a65843..1783c34c 100644 --- a/tests/tooling/services/test_send_chat_history.py +++ b/tests/tooling/services/test_send_chat_history.py @@ -124,23 +124,24 @@ async def test_send_chat_history_with_options( # Assert assert result.succeeded is True - def test_send_chat_history_validates_turn_context(self, service, chat_history_messages): + @pytest.mark.asyncio + async def test_send_chat_history_validates_turn_context(self, service, chat_history_messages): """Test that send_chat_history validates turn_context parameter.""" # Act & Assert with pytest.raises(ValueError, match="turn_context cannot be empty or None"): - import asyncio - - asyncio.run(service.send_chat_history(None, chat_history_messages)) + await service.send_chat_history(None, chat_history_messages) - def test_send_chat_history_validates_chat_history_messages(self, service, mock_turn_context): + @pytest.mark.asyncio + async def test_send_chat_history_validates_chat_history_messages( + self, service, mock_turn_context + ): """Test that send_chat_history validates chat_history_messages parameter.""" # Act & Assert with pytest.raises(ValueError, match="chat_history_messages cannot be empty or None"): - import asyncio + await service.send_chat_history(mock_turn_context, None) - asyncio.run(service.send_chat_history(mock_turn_context, None)) - - def test_send_chat_history_validates_activity(self, service, chat_history_messages): + @pytest.mark.asyncio + async def test_send_chat_history_validates_activity(self, service, chat_history_messages): """Test that send_chat_history validates turn_context.activity.""" # Arrange mock_context = Mock() @@ -148,11 +149,12 @@ def test_send_chat_history_validates_activity(self, service, chat_history_messag # Act & Assert with pytest.raises(ValueError, match="turn_context.activity cannot be None"): - import asyncio - - asyncio.run(service.send_chat_history(mock_context, chat_history_messages)) + await service.send_chat_history(mock_context, chat_history_messages) - def test_send_chat_history_validates_conversation_id(self, service, chat_history_messages): + @pytest.mark.asyncio + async def test_send_chat_history_validates_conversation_id( + self, service, chat_history_messages + ): """Test that send_chat_history validates conversation_id from activity.""" # Arrange mock_context = Mock() @@ -167,11 +169,10 @@ def test_send_chat_history_validates_conversation_id(self, service, chat_history ValueError, match="conversation_id cannot be empty or None.*turn_context.activity.conversation.id", ): - import asyncio + await service.send_chat_history(mock_context, chat_history_messages) - asyncio.run(service.send_chat_history(mock_context, chat_history_messages)) - - def test_send_chat_history_validates_message_id(self, service, chat_history_messages): + @pytest.mark.asyncio + async def test_send_chat_history_validates_message_id(self, service, chat_history_messages): """Test that send_chat_history validates message_id from activity.""" # Arrange mock_context = Mock() @@ -187,11 +188,10 @@ def test_send_chat_history_validates_message_id(self, service, chat_history_mess with pytest.raises( ValueError, match="message_id cannot be empty or None.*turn_context.activity.id" ): - import asyncio + await service.send_chat_history(mock_context, chat_history_messages) - asyncio.run(service.send_chat_history(mock_context, chat_history_messages)) - - def test_send_chat_history_validates_user_message(self, service, chat_history_messages): + @pytest.mark.asyncio + async def test_send_chat_history_validates_user_message(self, service, chat_history_messages): """Test that send_chat_history validates user_message from activity.""" # Arrange mock_context = Mock() @@ -207,9 +207,7 @@ def test_send_chat_history_validates_user_message(self, service, chat_history_me with pytest.raises( ValueError, match="user_message cannot be empty or None.*turn_context.activity.text" ): - import asyncio - - asyncio.run(service.send_chat_history(mock_context, chat_history_messages)) + await service.send_chat_history(mock_context, chat_history_messages) @pytest.mark.asyncio async def test_send_chat_history_handles_client_error(