diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fb75c9ffe..000ac8d250 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,17 @@ This section accumulates work toward the **2.3.0** milestone ### Fixed +- **mcp:** Two enabled MCP servers could bind the same tool name (server `a` + exposing `b_x` and server `a_b` exposing `x` both bind `a_b_x`). Both + declarations reached the model and the provider silently kept only one. + Duplicate bound names are now dropped with a warning naming the surviving + server, in enabled-server declaration order. Note the surviving server is + stable only while its discovery keeps succeeding: if the winner's discovery + fails transiently, the later duplicate becomes served again, so the + implementation behind a bound name can alternate across cache generations + with server health. + ([#8135](https://github.com/bytedance/deer-flow/pull/8135)) + - **channels:** WeCom no longer drops images between 2 MB and 10 MB. The adapter capped image uploads at 2 MB — the voice limit in the same protocol — so a larger chart was skipped before upload and the turn arrived without it. diff --git a/backend/packages/harness/deerflow/mcp/tools.py b/backend/packages/harness/deerflow/mcp/tools.py index 513f448a55..9ded2b0c5c 100644 --- a/backend/packages/harness/deerflow/mcp/tools.py +++ b/backend/packages/harness/deerflow/mcp/tools.py @@ -1014,8 +1014,52 @@ def _flatten_server_tool_groups( enabled_order: Collection[str], groups: Mapping[str, ServerDiscoveryResult], ) -> list[BaseTool]: - """Flatten grouped results in enabled-server declaration order.""" - return [tool for name in enabled_order if name in groups for tool in groups[name].tools] + """Flatten grouped results in enabled-server declaration order. + + A tool whose bound name duplicates an earlier one is dropped with a + warning naming the server whose tool survived. Server prefixes do not + make bound names unique across servers (server ``a`` exposing ``b_x`` and + server ``a_b`` exposing ``x`` both bind ``a_b_x``), and a duplicate name + would reach the model as two same-named declarations, where the provider + silently keeps only one. The surviving server is stable only while its + discovery keeps succeeding: when the winner's discovery fails, its group + is absent from both this flatten and the cache initializer's reuse path, + so the later duplicate becomes served again and the implementation behind + a bound name can alternate across cache generations with transient server + health. + + The string checks below only cover a directly constructed + ``ServerDiscoveryResult`` in tests: production tools always pass through + ``_wrap_discovered_server_tools``, which guarantees a validated non-empty + string name before a group can exist. + """ + flattened: list[BaseTool] = [] + serving_server: dict[str, str] = {} + dropped = 0 + for name in enabled_order: + if name not in groups: + continue + for tool in groups[name].tools: + bound_name = getattr(tool, "name", None) + if isinstance(bound_name, str) and bound_name in serving_server: + logger.warning( + "Dropping MCP tool %r from server '%s': its bound name is already served by server '%s'; publishing both would hand the model two same-named declarations.", + bound_name, + name, + serving_server[bound_name], + ) + dropped += 1 + continue + if isinstance(bound_name, str): + serving_server[bound_name] = name + flattened.append(tool) + if dropped: + logger.info( + "Publishing %d MCP tool(s) after dropping %d duplicate bound name(s)", + len(flattened), + dropped, + ) + return flattened def _load_mcp_adapter_symbols() -> tuple[Any, Any] | None: diff --git a/backend/tests/test_mcp_per_server_discovery.py b/backend/tests/test_mcp_per_server_discovery.py index 80e18297dd..62d9c389d5 100644 --- a/backend/tests/test_mcp_per_server_discovery.py +++ b/backend/tests/test_mcp_per_server_discovery.py @@ -61,6 +61,39 @@ def test_flatten_server_tool_groups_preserves_enabled_order_and_empty_success(): assert groups["EMPTY"].tools == () +def test_flatten_server_tool_groups_drops_cross_server_duplicate_names(caplog): + """Server 'a' exposing 'b_x' and server 'a_b' exposing 'x' both bind 'a_b_x'.""" + a_b_x = _tool("a_b_x") + duplicate = _tool("a_b_x") + other = _tool("a_other") + groups = { + "a": ServerDiscoveryResult(tools=(a_b_x, other)), + "a_b": ServerDiscoveryResult(tools=(duplicate,)), + } + + with caplog.at_level(logging.WARNING, logger="deerflow.mcp.tools"): + flattened = _flatten_server_tool_groups(("a", "a_b"), groups) + + assert flattened == [a_b_x, other] + assert flattened[0] is a_b_x + assert duplicate not in flattened + assert "a_b_x" in caplog.text + assert "'a_b'" in caplog.text + + +def test_flatten_server_tool_groups_drops_duplicate_within_one_server(caplog): + first = _tool("A_search") + second = _tool("A_search") + groups = {"A": ServerDiscoveryResult(tools=(first, second))} + + with caplog.at_level(logging.WARNING, logger="deerflow.mcp.tools"): + flattened = _flatten_server_tool_groups(("A",), groups) + + assert flattened == [first] + assert "A_search" in caplog.text + assert "already served by server 'A'" in caplog.text + + def test_server_discovery_result_repr_hides_ownership_handles(): pool = MagicMock() binding = ServerBinding( diff --git a/backend/tests/test_mcp_tool_name_validation.py b/backend/tests/test_mcp_tool_name_validation.py index 1fa983de7a..5e8d77cd30 100644 --- a/backend/tests/test_mcp_tool_name_validation.py +++ b/backend/tests/test_mcp_tool_name_validation.py @@ -94,3 +94,42 @@ def test_drops_only_the_invalid_tool_in_a_mixed_batch(): tools, _ = _load([_tool("srv_ok"), _tool("srv_bad name with spaces"), _tool("srv_also_ok")]) assert {t.name for t in tools} == {"srv_ok", "srv_also_ok"} + + +def test_publishes_one_declaration_per_bound_name_across_servers(): + """End to end: server 'a' exposing 'b_x' and server 'a_b' exposing 'x' both + bind 'a_b_x'; get_mcp_tools() must publish exactly one declaration.""" + tools_a = [_tool("a_b_x")] + tools_ab = [_tool("a_b_x")] + mock_client = MagicMock() + + async def get_tools(*, server_name=None): + return tools_a if server_name == "a" else tools_ab + + mock_client.get_tools = get_tools + extensions_config = ExtensionsConfig.model_validate( + { + "mcpServers": { + "a": {"type": "sse", "url": "https://a.example/mcp"}, + "a_b": {"type": "sse", "url": "https://ab.example/mcp"}, + } + } + ) + + with ( + patch("langchain_mcp_adapters.client.MultiServerMCPClient", return_value=mock_client), + patch("deerflow.config.extensions_config.ExtensionsConfig.from_file", return_value=extensions_config), + patch( + "deerflow.mcp.tools.build_servers_config", + return_value={ + "a": {"transport": "sse"}, + "a_b": {"transport": "sse"}, + }, + ), + patch("deerflow.mcp.tools.get_initial_oauth_headers", new_callable=AsyncMock, return_value={}), + patch("deerflow.mcp.tools.build_oauth_tool_interceptor", return_value=None), + ): + tools = asyncio.run(get_mcp_tools()) + + assert [t.name for t in tools] == ["a_b_x"] + assert tools[0] is tools_a[0]