Skip to content

Conversation

@xerrors
Copy link

@xerrors xerrors commented Aug 31, 2025

The original code logic did not implement the filtering process after obtaining a single Server tool. The filtered Server Name should be passed to MultiServerMCPClient for screening.

Text scripts:

"""Test MCP."""

from src.common.mcp import get_mcp_tools, add_mcp_server, get_all_mcp_tools, MCP_SERVERS


async def test_get_mcp_tools():
    """Test get MCP tools."""

    new_config = {
        "url": "https://new.example.com/mcp",
        "transport": "streamable_http",
    }

    add_mcp_server("new_server", new_config)
    assert "new_server" in MCP_SERVERS

    all_tools = await get_all_mcp_tools()
    print(f"Got {len(all_tools)} tools")
    all_tools_sep = []
    for server_name in MCP_SERVERS.keys():
        tools = await get_mcp_tools(server_name)
        print(f"Got {len(tools)} tools from {server_name}")
        all_tools_sep.extend(tools)

    assert len(all_tools) == len(all_tools_sep), "All tools should be separated"


if __name__ == "__main__":
    import asyncio
    asyncio.run(test_get_mcp_tools())

Result:

Got 6 tools
Got 3 tools from deepwiki
Got 3 tools from new_server

@webup
Copy link
Owner

webup commented Sep 1, 2025

@xerrors Appreciate the fix! Please add your test case to the e2e DeepWiki test suite and ensure everything is ready by running make lint. Once that's done, we'll proceed with merging it right away!

@webup webup self-requested a review September 1, 2025 03:30
@xerrors
Copy link
Author

xerrors commented Sep 1, 2025

I'm sorry, but I found that with the current modification logic, many unit tests cannot pass. There might be many breaking changes. I think I'll just close this PR.

In my practice, since it's not possible to distinguish tools by Server name, I didn't adopt the singleton pattern for the client.

@xerrors xerrors closed this Sep 1, 2025
@webup
Copy link
Owner

webup commented Sep 1, 2025

@xerrors Thanks anyway, I'll handle the fixes and tests later.

@webup
Copy link
Owner

webup commented Sep 1, 2025

@claude plz review this fix

@claude
Copy link

claude bot commented Sep 1, 2025

Claude finished @webup's task —— View job


Reviewing MCP Server Filtering Fix

Todo List:

  • Read current MCP code to understand the architecture
  • Analyze the proposed change (+1/-1 diff)
  • Review the test script to understand expected behavior
  • Examine related test files to understand breaking changes
  • Provide comprehensive review feedback

Analysis Summary:

I've analyzed the MCP server filtering issue. The problem is clear from the test script provided in the PR description.

🔍 Root Cause Analysis

Current Issue in src/common/mcp.py:get_mcp_tools():

  1. Gets the global MCP client (which connects to ALL servers in MCP_SERVERS)
  2. Calls client.get_tools() to get ALL tools from ALL servers
  3. Does NOT filter by the requested server_name parameter
  4. Caches all tools under the specific server name, causing incorrect behavior

Expected vs Actual Behavior:

# Expected from the test script:
all_tools = await get_all_mcp_tools()             # Should get 6 tools total
deepwiki_tools = await get_mcp_tools("deepwiki") # Should get 3 tools from deepwiki only  
new_server_tools = await get_mcp_tools("new_server") # Should get 3 tools from new_server only

# Current behavior: both get_mcp_tools calls return ALL 6 tools instead of filtered tools

⚠️ Why the Proposed Fix May Cause Breaking Changes

The fundamental issue is architectural. The current code uses:

  1. Singleton Pattern: One global MultiServerMCPClient for all servers
  2. Global Tool Retrieval: client.get_tools() returns ALL tools from ALL connected servers
  3. No Server-Specific Filtering: No way to distinguish which tools came from which server

Breaking Changes Would Occur Because:

  1. Unit Tests Expectations: The existing unit tests in tests/unit_tests/test_mcp.py mock client.get_tools() to return specific tools for testing, but don't account for server-specific filtering

  2. E2E Test Dependencies: The e2e tests in tests/e2e_tests/test_deepwiki.py rely on get_deepwiki_tools() which calls get_mcp_tools("deepwiki") - if this suddenly returns filtered results instead of all tools, it breaks

  3. Caching Logic: The current caching assumes all tools are cached per server, but if we fix the filtering, the cache keys and logic need to change

🛠️ Proper Solution Architecture

The fix requires multiple coordinated changes:

  1. Tool Metadata Enhancement: Tools need to include server metadata so they can be filtered
  2. Client Architecture: Either:
    • Separate clients per server, OR
    • Enhanced MultiServerMCPClient with server-specific tool retrieval methods
  3. Cache Restructuring: Update caching logic to handle server-specific tools correctly
  4. Test Updates: Modify all tests to account for the new filtering behavior

📝 Recommendation

The PR author was correct to close this PR. A proper fix needs:

  1. API Investigation: Check if langchain_mcp_adapters.client.MultiServerMCPClient supports server-specific tool retrieval
  2. Comprehensive Refactoring: Update the entire MCP module architecture
  3. Test Suite Overhaul: Update all unit, integration, and e2e tests
  4. Backward Compatibility: Ensure get_all_mcp_tools() still works as expected

The "+1/-1" change alone cannot solve this without breaking the existing test suite and functionality.


Job Run | Branch: claude/pr-3-20250901-1320

@webup
Copy link
Owner

webup commented Sep 1, 2025

@xerrors I've made the fix in 0.1.2 version, take a look. :)

@xerrors
Copy link
Author

xerrors commented Sep 1, 2025

@webup Thanks!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants