diff --git a/libraries/microsoft-agents-a365-tooling-extensions-googleadk/microsoft_agents_a365/tooling/extensions/googleadk/services/mcp_tool_registration_service.py b/libraries/microsoft-agents-a365-tooling-extensions-googleadk/microsoft_agents_a365/tooling/extensions/googleadk/services/mcp_tool_registration_service.py index b9522a9..97fe39d 100644 --- a/libraries/microsoft-agents-a365-tooling-extensions-googleadk/microsoft_agents_a365/tooling/extensions/googleadk/services/mcp_tool_registration_service.py +++ b/libraries/microsoft-agents-a365-tooling-extensions-googleadk/microsoft_agents_a365/tooling/extensions/googleadk/services/mcp_tool_registration_service.py @@ -91,7 +91,16 @@ async def add_tool_servers_to_agent( self._logger.info(f"Loaded {len(mcp_server_configs)} MCP server configurations") - # Convert MCP server configs to McpToolset objects + # Collect existing server URLs to prevent duplicates (use set for O(1) lookup) + existing_server_urls = set() + for tool in agent.tools: + # Check if the tool is an McpToolset and has a connection_params.url + if hasattr(tool, "connection_params") and hasattr(tool.connection_params, "url"): + existing_server_urls.add(tool.connection_params.url) + + self._logger.debug(f"Found {len(existing_server_urls)} existing MCP servers in agent") + + # Convert MCP server configs to McpToolset objects (only new ones) mcp_servers_info = [] mcp_server_headers = { Constants.Headers.AUTHORIZATION: f"{Constants.Headers.BEARER_PREFIX} {auth_token}", @@ -99,6 +108,14 @@ async def add_tool_servers_to_agent( } for server_config in mcp_server_configs: + # Skip if server URL already exists + if server_config.url in existing_server_urls: + self._logger.debug( + f"Skipping MCP server '{server_config.mcp_server_name}' " + f"at {server_config.url} - already exists in agent" + ) + continue + try: server_info = McpToolset( connection_params=StreamableHTTPConnectionParams( @@ -109,6 +126,7 @@ async def add_tool_servers_to_agent( mcp_servers_info.append(server_info) self._connected_servers.append(server_info) + existing_server_urls.add(server_config.url) self._logger.info( f"Created MCP toolset for '{server_config.mcp_server_name}' " f"at {server_config.url}" @@ -119,15 +137,16 @@ async def add_tool_servers_to_agent( self._logger.warning(f"Failed to create MCP toolset for {server_name}: {tool_ex}") continue - # Combine existing tools with new MCP servers - all_tools = list(agent.tools) + mcp_servers_info - - self._logger.info( - f"Successfully configured agent with {len(mcp_servers_info)} MCP tool servers " - f"(total tools: {len(all_tools)})" - ) - - agent.tools = all_tools + # Only modify agent.tools if we have new servers to add + if mcp_servers_info: + all_tools = list(agent.tools) + mcp_servers_info + agent.tools = all_tools + self._logger.info( + f"Successfully configured agent with {len(mcp_servers_info)} new MCP tool servers " + f"(total tools: {len(all_tools)})" + ) + else: + self._logger.info("No new MCP servers to add to agent") async def cleanup(self): """Clean up any resources used by the service.""" diff --git a/tests/tooling/extensions/googleadk/test_mcp_tool_registration_service.py b/tests/tooling/extensions/googleadk/test_mcp_tool_registration_service.py index b711d01..5f13c5c 100644 --- a/tests/tooling/extensions/googleadk/test_mcp_tool_registration_service.py +++ b/tests/tooling/extensions/googleadk/test_mcp_tool_registration_service.py @@ -345,6 +345,130 @@ async def test_add_tool_servers_handles_toolset_creation_error( # Existing tools should still be present assert existing_tool in mock_agent.tools + @pytest.mark.asyncio + @pytest.mark.unit + async def test_add_tool_servers_skips_duplicate_server_urls( + self, mock_agent, mock_authorization, mock_turn_context + ): + """Test that duplicate server URLs are not added multiple times.""" + with ( + patch( + "microsoft_agents_a365.tooling.extensions.googleadk.services.mcp_tool_registration_service.McpToolServerConfigurationService" + ) as mock_config_service_class, + patch( + "microsoft_agents_a365.tooling.extensions.googleadk.services.mcp_tool_registration_service.Utility" + ) as mock_utility, + patch( + "microsoft_agents_a365.tooling.extensions.googleadk.services.mcp_tool_registration_service.McpToolset" + ) as mock_toolset_class, + ): + # Setup mocks + mock_utility.resolve_agent_identity.return_value = "agent-123" + mock_utility.get_user_agent_header.return_value = "Agent365SDK/1.0" + + # Create two server configs with the same URL + mock_server_config1 = MagicMock() + mock_server_config1.mcp_server_name = "server-1" + mock_server_config1.url = "https://test-server.example.com/mcp" + + mock_server_config2 = MagicMock() + mock_server_config2.mcp_server_name = "server-2" + mock_server_config2.url = "https://test-server.example.com/mcp" # Same URL + + mock_config_service = AsyncMock() + mock_config_service.list_tool_servers = AsyncMock( + return_value=[mock_server_config1, mock_server_config2] + ) + mock_config_service_class.return_value = mock_config_service + + # Create mock toolsets + mock_toolset1 = MagicMock() + mock_toolset1.connection_params = MagicMock() + mock_toolset1.connection_params.url = "https://test-server.example.com/mcp" + + mock_toolset_class.return_value = mock_toolset1 + + from microsoft_agents_a365.tooling.extensions.googleadk import ( + McpToolRegistrationService, + ) + + service = McpToolRegistrationService() + + # Set up agent with no existing tools + mock_agent.tools = [] + + # Act + await service.add_tool_servers_to_agent( + agent=mock_agent, + auth=mock_authorization, + auth_handler_name="graph", + context=mock_turn_context, + auth_token="test-token", + ) + + # Assert - toolset should be created only once despite two configs + assert mock_toolset_class.call_count == 1 + assert len(service._connected_servers) == 1 + assert len(mock_agent.tools) == 1 + + @pytest.mark.asyncio + @pytest.mark.unit + async def test_add_tool_servers_skips_existing_servers_in_agent( + self, mock_agent, mock_authorization, mock_turn_context + ): + """Test that servers already in the agent are not added again.""" + with ( + patch( + "microsoft_agents_a365.tooling.extensions.googleadk.services.mcp_tool_registration_service.McpToolServerConfigurationService" + ) as mock_config_service_class, + patch( + "microsoft_agents_a365.tooling.extensions.googleadk.services.mcp_tool_registration_service.Utility" + ) as mock_utility, + patch( + "microsoft_agents_a365.tooling.extensions.googleadk.services.mcp_tool_registration_service.McpToolset" + ) as mock_toolset_class, + ): + # Setup mocks + mock_utility.resolve_agent_identity.return_value = "agent-123" + mock_utility.get_user_agent_header.return_value = "Agent365SDK/1.0" + + # Create server config + mock_server_config = MagicMock() + mock_server_config.mcp_server_name = "existing-server" + mock_server_config.url = "https://existing-server.example.com/mcp" + + mock_config_service = AsyncMock() + mock_config_service.list_tool_servers = AsyncMock(return_value=[mock_server_config]) + mock_config_service_class.return_value = mock_config_service + + from microsoft_agents_a365.tooling.extensions.googleadk import ( + McpToolRegistrationService, + ) + + service = McpToolRegistrationService() + + # Set up agent with existing toolset that has the same URL + existing_toolset = MagicMock() + existing_toolset.connection_params = MagicMock() + existing_toolset.connection_params.url = "https://existing-server.example.com/mcp" + mock_agent.tools = [existing_toolset] + + # Act + await service.add_tool_servers_to_agent( + agent=mock_agent, + auth=mock_authorization, + auth_handler_name="graph", + context=mock_turn_context, + auth_token="test-token", + ) + + # Assert - no new toolset should be created + mock_toolset_class.assert_not_called() + assert len(service._connected_servers) == 0 + # Agent should still have the one existing tool + assert len(mock_agent.tools) == 1 + assert mock_agent.tools[0] == existing_toolset + class TestCleanup: """Tests for cleanup method."""