Address code review feedback: type annotations, test fixes, and code quality

- CRM-003: Add return type annotation to add_tool_servers_to_agent
- CRM-004: Add type annotations to _cleanup_servers and cleanup_all_servers
- CRM-006: Fix conversion error test to actually test error handling
- CRM-007: Update implicit boolean check to explicit None check
- CRM-008: Remove redundant local import of Agent
- CRM-010: Use bare raise instead of raise e
- CRM-011: Rename test to match behavior (skips_message not uses_empty_string)

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

Author: Johan Broberg

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

@@ -69,7 +69,7 @@ async def add_tool_servers_to_agent(
         auth_handler_name: str,
         context: TurnContext,
         auth_token: Optional[str] = None,
-    ):
+    ) -> Agent:
         """
         Add new MCP servers to the agent by creating a new Agent instance.
 
@@ -88,7 +88,7 @@ async def add_tool_servers_to_agent(
             New Agent instance with all MCP servers, or original agent if no new servers
         """
 
-        if not auth_token:
+        if auth_token is None:
             scopes = get_mcp_platform_authentication_scope()
             authToken = await auth.exchange_token(context, scopes, auth_handler_name)
             auth_token = authToken.token
@@ -190,8 +190,6 @@ async def add_tool_servers_to_agent(
                 all_mcp_servers = existing_mcp_servers + new_mcp_servers
 
                 # Recreate the agent with all MCP servers
-                from agents import Agent
-
                 new_agent = Agent(
                     name=agent.name,
                     model=agent.model,
@@ -223,12 +221,12 @@ async def add_tool_servers_to_agent(
                 # Clean up connected servers if agent creation fails
                 self._logger.error(f"Failed to recreate agent with new MCP servers: {e}")
                 await self._cleanup_servers(connected_servers)
-                raise e
+                raise
 
         self._logger.info("No new MCP servers to add to agent")
         return agent
 
-    async def _cleanup_servers(self, servers):
+    async def _cleanup_servers(self, servers: List[MCPServerStreamableHttp]) -> None:
         """Clean up connected MCP servers"""
         for server in servers:
             try:
@@ -238,7 +236,7 @@ async def _cleanup_servers(self, servers):
                 # Log cleanup errors but don't raise them
                 self._logger.debug(f"Error during server cleanup: {e}")
 
-    async def cleanup_all_servers(self):
+    async def cleanup_all_servers(self) -> None:
         """Clean up all connected MCP servers"""
         if hasattr(self, "_connected_servers"):
             await self._cleanup_servers(self._connected_servers)

tests/tooling/extensions/openai/test_message_conversion.py

@@ -139,8 +139,8 @@ def test_convert_message_with_list_content(self, service):
 
     # CV-11
     @pytest.mark.unit
-    def test_convert_empty_content_uses_empty_string(self, service):
-        """Test that empty content is handled - message skipped due to validator."""
+    def test_convert_empty_content_skips_message(self, service):
+        """Test that messages with empty content are skipped during conversion."""
         message = MockUserMessage(content="")
 
         result = service._convert_single_message(message)

tests/tooling/extensions/openai/test_send_chat_history.py

@@ -398,22 +398,20 @@ async def test_send_chat_history_messages_conversion_error(
         self, service, mock_turn_context
     ):
         """Test send_chat_history_messages handles conversion errors gracefully."""
-        # Create a message that might cause conversion issues but still has content
-        problematic_message = MockUserMessage(content="Valid content")
+        sample_messages = [MockUserMessage(content="Hello")]
 
         with patch.object(
-            service.config_service,
-            "send_chat_history",
-            new_callable=AsyncMock,
-        ) as mock_send:
-            mock_send.return_value = OperationResult.success()
+            service, "_convert_openai_messages_to_chat_history"
+        ) as mock_convert:
+            mock_convert.side_effect = Exception("Conversion failed")
 
-            # Should not raise, should handle gracefully
             result = await service.send_chat_history_messages(
-                mock_turn_context, [problematic_message]
+                mock_turn_context, sample_messages
             )
 
-            assert result.succeeded is True
+            assert result.succeeded is False
+            assert len(result.errors) == 1
+            assert "Conversion failed" in str(result.errors[0].message)
 
     # EH-05
     @pytest.mark.asyncio