fix(mcp): route tool calls by server_id so unprefixed names cannot pick an arbitrary server - #33665
fix(mcp): route tool calls by server_id so unprefixed names cannot pick an arbitrary server#33665tin-berri wants to merge 9 commits into
Conversation
…ck an arbitrary server tool_name_to_mcp_server_name_mapping mapped a tool name to a server name and was written with an unqualified, global, last-writer-wins key. Two server entries sharing an upstream URL expose the same tool names, so that key always collided and the winner was whichever server registered last, which meant config declaration order decided which upstream credential left the gateway. server_name is not a sound identity: schema.prisma has no unique constraint on server_name or alias, and the create endpoint only checks server_id. The registry is already keyed by server_id and config servers derive a deterministic one, so server_id was always the real identity. The map now holds frozensets of server ids and accumulates owners instead of overwriting them, so a name served by several reachable servers stays visibly ambiguous and is rejected rather than dispatched to an arbitrary server. Ambiguity is judged against the servers the caller can reach, so a session scoped with x-mcp-servers still resolves its unprefixed names. All three writers now go through one registration chokepoint; they previously disagreed on what the value was. Cleanup withdraws only the departing server's id from each row instead of matching by name, which stopped removing one server from deleting a same-named server's routes. execute_mcp_tool now dispatches the server it resolved rather than passing a name that call_tool re-resolved, closing the same drop on the responses API path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR reworks MCP tool dispatch to use
Confidence Score: 5/5Safe to merge. The routing redesign is thorough, every dispatch path has a scope gate, and the test suite covers the full ambiguity/scoping matrix including the previously-reported edge cases fixed in follow-up commits. The ownership map accumulates correctly across servers and stays authoritative per server; the single-writer chokepoint handles both registration and eviction via the same arithmetic; resolve_tool_route scope narrowing is consistent with what call_tool enforces; the fallback to _get_mcp_server_from_tool_name for unregistered tools is scope-gated at call_tool; and cleanup mutates in place rather than rebinding, preventing lost writes from the concurrent startup task. All three previously-reported issues have been fixed and pinned by dedicated regression tests. Files Needing Attention: No files require special attention. The three previously-reported issues have been fixed and have explicit regression tests.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Core routing redesign: replaces name-keyed last-writer-wins map with id-keyed frozenset accumulation; adds MCPToolRoute tagged union, resolve_tool_route with scope filtering, _replace_server_tool_routes single-writer, and call_tool scope gate. Logic is sound, cleanup mutates in-place correctly, and the _get_mcp_server_from_tool_name prefix path is now anchored to ownership rather than prefix alone. |
| litellm/proxy/_experimental/mcp_server/server.py | execute_mcp_tool now routes through resolve_tool_route for the JSON-RPC path (no requested_server), raises 409 for ambiguous names, threads resolved_server+allowed_server_ids to call_tool, and the scope-blind fallback lookup is guarded by an explicit allowed_server_ids check. |
| litellm/responses/mcp/litellm_proxy_mcp_handler.py | Returns server_ids instead of display names from _get_tools_and_server_names; _deduplicate_mcp_tools uses resolve_tool_route for identity-preserving per-tool routing; _execute_tool_calls dispatches by server_id and fails closed with a tool-result error for unresolvable names rather than dropping them silently. |
| litellm/proxy/guardrails/guardrail_hooks/panw_prisma_airs/panw_prisma_airs.py | Updated to resolve server display name via get_mcp_server_by_id (or _get_mcp_server_from_tool_name) rather than the old string-valued mapping; returns 'unknown' for ambiguous names instead of picking an arbitrary one. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | Comprehensive new tests: _replace_server_tool_routes accumulation/withdrawal semantics, resolve_tool_route scope filtering, cleanup in-place mutation invariant, call_tool scope gate, OpenAPI registration atomicity, and _get_mcp_server_from_tool_name ambiguity refusal. Existing tests updated to frozenset{server_id} values correctly. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py | New integration tests covering ambiguous-name 409, out-of-scope not-dispatched, scoped single-server resolution, same-name server dispatch-by-id, and prefixed name routing; existing tests updated to use frozenset server_id values. |
Reviews (8): Last reviewed commit: "fix(mcp): resolve responses-API tool dis..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e ambiguity check server.py selected the route variant with isinstance. A module reload rebinds the manager and its classes, so resolve_tool_route returned a reloaded variant while server.py still held the pre-reload class; isinstance was False, the ambiguity branch never ran, and the call fell through to the local registry as a 404 instead of the intended 409. Each variant now carries a Literal kind tag and server.py dispatches on it, which is what makes this a tagged union rather than one that leans on class identity. Withdrawing a server's routes also rebuilt the mapping into a fresh dict, orphaning any holder of the previous one. The initialize task is fired without being awaited and can be holding it, so the rebuild now updates the mapping in place.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
…r's scope resolve_tool_route fell through to a scope-blind lookup when the tool had owners but none intersected the caller's allowed set, so a caller scoped to one server could resolve a tool served only by another. The downstream name-based permission check would then misroute the call to a reachable server sharing that name. The scoped branch is now authoritative: known owners with none in scope returns not_found rather than resolving a server the caller cannot reach. Also seed the logging tests from the specific server under test instead of every manager server id, so adding a second fixture server cannot make a tool look ambiguous.
|
@greptileai I pushed fixes for the P1 out-of-scope fallthrough and the P2 test-seeding finding, and replied on the feature-flag thread with why the 409 is intentionally unconditional. Please re-review. |
…itellm_lit4500_mcp_server_id_routing # Conflicts: # litellm/proxy/_experimental/mcp_server/mcp_server_manager.py
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…l-resolution fallback resolve_tool_route fails closed when a tool's only owners are outside the caller's scope, but execute_mcp_tool then re-resolved a still-unset server with a scope-blind lookup so BYOK and credential injection could run on every path. On the JSON-RPC and tool-search paths that fallback undid the scope decision: because no server_name was set for an out-of-scope tool, the permission gate was skipped, and the fallback dispatched to the out-of-scope server with its credentials. The fallback now only accepts a server the caller is already allowed to reach, matching how the requested_server lookups are guarded by the server-id mismatch check. Cleanup of a departed server's routes withdrew its id per row but did so by clearing the whole mapping and repopulating it, which momentarily emptied the shared dict the un-awaited initialize task writes into. It now withdraws the id key by key, so the dict is never empty and never rebound.
|
bugbot run |
|
@greptileai rereview |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit cd3a922. Configure here.
The call_tool and list_tools mock tests seeded a tool's owning-server set from every id in the registry. That holds only while the fixture has exactly one server; adding a second would make the tool look multi-owned and resolve as ambiguous, failing the test for an unrelated reason. Each site now derives the id from the server it actually loaded, matching the pattern already used in the alias-prefixing tests and the logging tests.
|
Addressed the remaining style concern in b0908d5: the mock call_tool/list_tools tests in tests/mcp_tests/test_mcp_server.py seeded a tool's owning-server set from every registry id ( |
|
bugbot run |
|
@greptileai rereview |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b0908d5. Configure here.
…a tool name _get_mcp_server_from_tool_name had two competing sources of truth: the ownership map, and a first-wins prefix-to-server scan of the registry. The prefix path ignored the map's ambiguity, so a prefixed name owned by two servers sharing a prefix resolved to whichever was scanned first instead of None, breaking the function's own contract. It also never checked that the prefix-named server actually owned the tool. Resolution now goes through the ownership map first: a registered tool name with one owner resolves by id and with several owners returns None. Only a name that is not itself registered falls to prefix extraction, and that path resolves to the one server that both matches the prefix and owns the underlying tool, so a shared prefix or a prefix naming a non-owner both stay unresolved. This removes the class of arbitrary-owner resolution that callers were each guarding against individually. The duplicate-prefix regression test set only name, not server_name, so the prefix was never recognized and the prefix path was never exercised; it now sets server_name and a sibling test covers a prefix that names a server which does not own the tool.
|
@greptileai Reworked _get_mcp_server_from_tool_name so the ownership map is the single authority for tool-name resolution (P1 on the prefix path), and fixed the vacuous duplicate-prefix test. Please re-review. |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ad888fc. Configure here.
mateo-berri
left a comment
mateo-berri
left a comment
There was a problem hiding this comment.
I know this was a deliberate behavioral contract change, but is there any worry that customers could observe this as a regression?
Also, nit: union-only route registration might lead to stale-owner false-ambiguity 409s after upstream tool removals until restart, but I do see that this is a narrow edge case
|
Good questions, and the nit was real, so I fixed it in 08f6995 rather than leave it On the contract change being observed as a regressionYes, it is observable, and here is the bound I convinced myself of. Two things both have to hold: the tool name has to resolve to two or more servers inside the caller's own reachable set, and the call has to use a name that is ambiguous within that set, so an unprefixed name or a prefix that two servers share The reachable set is the one narrowed by For that caller I would push back slightly on framing it as losing working behavior. The old map was The honest residual is the customer whose desired server happened to be the last-write winner; that call succeeded before and 409s now. I think that is the right trade for a credential-misrouting bug, and the 409 names the candidate servers and says to use the prefixed form, which is already in that caller's own On the nitYou were right, and it was not as narrow as it looked once I traced it. The fix is to stop registering additively. A server's Two details worth flagging since they are design choices rather than mechanics. A reverse index of each server's rows keeps the withdrawal proportional to that server's own tools instead of to every row in the map, because this runs on every The OpenAPI path now replaces only after the whole spec parses, so a mid-loop failure leaves the previous routes intact instead of committing a partial set that would withdraw operations still being served, and the startup warm-up no longer re-registers what listing already recorded One pre-existing limit I did not change and want on the record: Five new tests, all mutation-checked. Making the replace union-only again kills four of them, dropping the call from the listing path kills three, and pointing eviction at the index kills the OpenAPI eviction test. |
|
bugbot run |
|
@greptileai rereview |
| if desired: | ||
| self._tool_routes_by_server_id[server_id] = desired | ||
| else: | ||
| self._tool_routes_by_server_id.pop(server_id, None) |
There was a problem hiding this comment.
Concurrent route replace orphans map
Medium Severity
Concurrent calls to _replace_server_tool_routes for the same server, often from fire-and-forget startup or tools/list tasks, can lead to race conditions. Without per-server serialization, an older listing might finish after a newer one, re-adding previously withdrawn routes or leaving stale entries in tool_name_to_mcp_server_ids_mapping, causing false multi-owner ambiguity or outdated tool routes.
Reviewed by Cursor Bugbot for commit 08f6995. Configure here.
There was a problem hiding this comment.
Verified this one rather than taking it at face value, and the conclusion is that the flagged state is gone in 0353b24, while the underlying concern is real but strictly smaller than before the change
On the mechanics first. _replace_server_tool_routes and its caller _create_prefixed_tools are both plain def with no await between collecting the names and writing them, so under asyncio they run to completion without interleaving. The map and the index could not tear mid-call, which is the failure mode "orphans map" suggests
What can genuinely happen is out-of-order completion of two listings for the same server, since each awaits its own network round trip and the older one can land last. I reproduced exactly that:
after out-of-order completion: {'echo': ..., 'dropped': ...} stale row present: True
after the next listing: {'echo': ...} self-healed: True
union-only, same sequence: {'echo': ..., 'dropped': ...}
stale 'dropped' still present after two clean listings: True
So the stale row is transient and the next listing corrects it, where the union-only registration this replaced kept it permanently and had no path back short of a restart. That permanent case is the bug this PR is fixing, so serializing per server would be tightening a window the change already shrank from unbounded to one listing
I did drop the _tool_routes_by_server_id index you flagged, though for the reason underneath it rather than the race. It existed to avoid scanning the map on each re-listing, and measured at 20k rows that scan is 0.87 ms, running immediately after that server's 20 to 500 ms tools/list round trip. Sub-millisecond on an I/O-bound path is not worth a second source of truth that can disagree with the first, and it had already cost me once: pointing eviction at the index broke test_update_server_eviction_clears_openapi_routing_artifacts, which seeds the mapping directly and asserts eviction clears rows however they were written. Eviction is now the same replace with an empty set, the scan is exhaustive by construction, and lines 4909-4912 no longer exist
Leaving per-server serialization out deliberately. Happy to file it if you or a reviewer disagrees, but it would be new locking for a self-correcting transient, and there is a related known gap already worth its own ticket, where reload_servers_from_database can swap the registry under an in-flight fire-and-forget listing task
Registering a server's tool routes was union-only, so a routing row could only ever gain owners. Nothing withdrew a tool while its server stayed in the registry: `_cleanup_server_tool_routing_artifacts` withdraws a server's id from every row, but it only runs when the server leaves the registry. So an upstream that stopped exposing a tool left its owner pinned. A name then served by exactly one reachable server kept resolving as ambiguous and kept returning the 409, with no way back short of restarting the proxy. A server's `tools/list` result is its complete listing, so it is the truth rather than an increment. `_replace_server_tool_routes` replaces one server's rows instead of accumulating into them, withdrawing its id from a name it no longer serves and dropping the row once no owner is left. Rows still accumulate across servers, so a genuinely shared name stays ambiguous. Treating a listing as the truth is safe because `_fetch_tools_with_timeout` raises on every failure instead of returning an empty list, and caller-scoped narrowing (`check_allowed_or_banned_tools`, semantic filtering) runs downstream, so neither a failed listing nor one caller's filtered view can evict routes another caller needs. Eviction is the same operation with an empty set, so it delegates rather than keeping its own copy of the withdrawal arithmetic. An earlier cut of this carried a per-server reverse index to avoid scanning the map, which bought under a millisecond on a path that has just made a network round trip, in exchange for a second source of truth that can disagree with the first. It already had: pointing eviction at the index broke `test_update_server_eviction_clears_openapi_routing_artifacts`, which seeds the mapping directly and asserts eviction clears rows however they were written. The scan is exhaustive by construction, so that class is gone. The OpenAPI path replaces once the whole spec has parsed, so a mid-loop failure leaves the previous routes intact rather than committing a partial set that would withdraw operations still served. The startup warm-up no longer re-registers what listing already recorded, which would have re-added names outside the replace and reintroduced rows it cannot withdraw. The OpenAPI registration call site turned out to have no test coverage at all; deleting it left the suite green. It now has three, including the mid-parse failure case.
08f6995 to
0353b24
Compare
|
Reworked the nit fix in 0353b24, which replaces 08f6995 on the branch The first cut carried a per-server reverse index so a re-listing could withdraw stale rows without scanning the map. That was the wrong call and Bugbot's finding sat right on top of it. I measured what the index was actually buying: at 20k rows the full scan is 0.87 ms, and it runs immediately after that server's The index is gone. Eviction is now literally the same replace with an empty set, so the withdrawal arithmetic exists once and the scan is exhaustive by construction. That is what I should have written the first time; it is the shared-helper version rather than new machinery On size, the honest accounting is that this is flat rather than a reduction: net +3 lines of executable code against the pre-fix state, with the rest of the diff being the docstring, which I have trimmed. It cannot go negative, because the primitive now does strictly more than the one it replaces, adding withdrawal to what was an append. What did shrink is the number of moving parts, since one method and one instance field are gone and eviction dropped from six lines to one One thing worth flagging on its own. While mutation-checking I found the OpenAPI registration call site had no coverage at all: deleting the registration outright left the entire mcp_server suite green, both before and after my change. It has three tests now, including the mid-parse failure case, which caught a real gap in my first attempt at that test, since raising on the first operation never exercises the partial-commit path I was claiming to pin. Five mutants now die where two survived earlier: union-only registration kills eight tests, dropping the listing-path replace kills the prefix-resolution test, dropping the OpenAPI replace kills three, removing the eviction delegation kills four, and replacing per operation instead of after the spec parses kills the mid-parse test |
|
@greptileai rereview |
1 similar comment
|
@greptileai rereview |
…t on scope The responses API surface still routed tool calls by server name. It built the caller's reachable set as MCPServer objects, narrowed by both the key's grants and the requested server filter, then discarded that identity by flattening to display names. `tool_server_map` carried a name, and dispatch re-resolved it with `get_mcp_server_by_name`, which walks the whole registry and returns the first match. Server names are not unique, so a tool listed from a reachable server could dispatch to a same-named server the caller cannot reach, sending that server's upstream credential. That is the bug this PR already fixed for MCP JSON-RPC, on the one surface that had not been converted. The fix is the same: stop discarding identity. `tool_server_map` now carries the server_id resolved within the caller's reachable set through `resolve_tool_route`, the same scoped resolver the JSON-RPC path uses, so a name two reachable servers share stays ambiguous here too rather than silently picking one. Dispatch looks the server up by id. A tool with no reachable owner fails closed and reports a result for its tool call, matching how every other failure in that loop is surfaced, rather than being dropped. `resolved_server` is a parameter this PR introduced, and it let a caller hand `call_tool` any server at all. That is the same class of defect one layer down, so the check belongs at the chokepoint rather than at each caller: `call_tool` now takes the reachable set the server was resolved against and refuses to dispatch outside it, covering the caller's server and one it resolves by name itself, which also walks the whole registry. Supplying `resolved_server` without that set is rejected, so caller-supplied identity always arrives with its provenance. Both callers already computed the set, so nothing recomputes it.
|
bugbot run |
|
@greptileai rereview |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e9fd8fc. Configure here.
| server = self._get_mcp_server_from_tool_name(tool_name) | ||
| if server is None: | ||
| return MCPToolRouteNotFound(tool_name=tool_name) | ||
| return MCPToolRouteResolved(server=server) |
There was a problem hiding this comment.
Scoped route fallthrough bypass
High Severity
The resolve_tool_route method, when falling back to prefix-based resolution, can return a resolved server that is outside the caller's allowed_server_ids. This bypasses intended scope checks, contradicting fail-closed behavior for directly mapped tools and potentially allowing unauthorized dispatch.
Reviewed by Cursor Bugbot for commit e9fd8fc. Configure here.
3 participants
Relevant issues
What this PR does:
server_idinstead of by server name, since server names are not uniquex-mcp-serversscoped sessions still resolve their unprefixed namestools/listas authoritative, so a tool an upstream drops stops making a name look ambiguous without needing a restartLinear ticket
Resolves LIT-4500
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Two MCP servers pointing at one upstream URL, each with its own credential, both exposing a tool named
echoThe upstream is a FastMCP echo server that reports back the credential it actually received. Calls go over MCP JSON-RPC (
POST /mcp/), which is the path that carries noserver_idBefore, at 3459956
tools/listadvertises the prefixed names, and both prefixed calls route correctlyThe unprefixed
echosilently pickedecho_zuluand sent that server's credential upstream. Reversing the two entries in the config, changing nothing else, flips which credential leaves the gatewayDeclaration order alone decided which upstream credential was used for the same request
After, at 2f7adb7
The prefixed names still route to their own server, and the unprefixed name is now refused with a 409 that names the candidates instead of silently picking one
Ambiguity is relative to what the caller can reach, so a session scoped to one server with
x-mcp-serversis still served its unprefixed name and resolves to that server's credential; only a caller that can reach both candidates is refusedReversing the two entries in the config, changing nothing else, produces the same rejection rather than the other credential, and a single-server config still resolves the unprefixed name
A scoped caller cannot reach across its scope by using another server's prefixed name either; the tool resolves to no reachable server and the call is refused rather than routed to the out-of-scope server
Type
🐛 Bug Fix
Changes
tool_name_to_mcp_server_name_mappingmapped a tool name to a server name, and it was written with an unqualified, global, last-writer-wins key. Two server entries sharing an upstream URL necessarily expose the same tool names, so that key always collided and the survivor was whichever server registered last. Registry iteration order decided it, which is why reordering the config changed which credential went upstreamserver_nameis not a sound identity for this. Nothing enforces its uniqueness:schema.prismahas no@uniqueonserver_nameoralias, and the create endpoint only checksserver_id. The registry is already keyed byserver_id, and config servers derive a deterministicserver_idvia_generate_stable_server_id, soserver_idwas always the real identityThe map is now
tool_name_to_mcp_server_ids_mapping: dict[str, frozenset[str]], holding server ids and accumulating owners across servers instead of overwriting them, so a name served by several servers stays visibly ambiguous. All the writers now go through one_replace_server_tool_routeschokepoint; previously they disagreed about what the value even was, writingserver_prefix,get_server_prefix(server)andserver.namerespectively for the same serverAccumulating owners is right across servers and wrong within one. A row could only ever gain owners, and nothing withdrew a tool while its server stayed in the registry, so an upstream that stopped exposing a tool left its owner pinned; a name then served by exactly one reachable server kept returning the 409 with no way back short of restarting the proxy. A server's
tools/listresult is its complete listing rather than an increment, so_replace_server_tool_routesreplaces that one server's rows, withdrawing its id from a name it no longer serves and dropping the row once no owner is left. That is safe to treat as the truth because_fetch_tools_with_timeoutraises on every failure instead of returning an empty list, and caller-scoped narrowing (check_allowed_or_banned_tools, semantic filtering) runs downstream of listing, so neither a failed listing nor one caller's filtered view can evict routes another caller still needs. The OpenAPI path replaces only once the whole spec has parsed, so a mid-loop failure leaves the previous routes intact instead of committing a partial set that would withdraw operations still being servedEviction is the same operation with an empty set, so it delegates to that replace instead of keeping its own copy of the withdrawal arithmetic, and the scan is exhaustive by construction. An earlier cut carried a per-server reverse index to avoid scanning the map; at 20k rows that scan measures 0.87 ms and runs immediately after that server's
tools/listround trip, so it was buying well under a percent of an I/O-bound path in exchange for a second source of truth that can disagree with the first. It already had, since pointing eviction at the index broketest_update_server_eviction_clears_openapi_routing_artifacts, which seeds the mapping directly and asserts eviction clears rows however they were written. One pre-existing limit is unchanged;ClientSession.list_tools()is called without a cursor, so a paginating upstream only ever yields its first page, and since every path that populates this map goes through that same single-page listing, replacing cannot drop a row the map legitimately gained elsewhereThe OpenAPI registration call site had no test coverage at all; deleting the registration outright left the whole mcp_server suite green both before and after this change. It now has three tests, including a mid-parse failure that pins replacing only once the spec has fully parsed
resolve_tool_routeis the single place that turns a tool name into a routing decision, and it is authoritative against the caller's reachable set. Ambiguity is judged against the servers the caller can actually reach, not the whole registry, because an unprefixed name is only ambiguous relative to what is in scope: a session narrowed withx-mcp-serversis served unprefixed names precisely because nothing else is reachable for it, and those still resolve. Once the tool's owners are known, resolution stays inside that scoped set: several reachable owners is a 409 naming the candidates, exactly one is dispatched, and owners that exist but none of which the caller can reach is a not-found rather than a fall-through to a scope-blind lookup that could hand back a server outside the caller's scope. Prefixed names are unaffected, and so is a name served by a single server. The route is returned as a tagged union carrying an explicitkinddiscriminator rather than relying on class identity, so a caller that discriminates on the tag keeps working across a module reload where anisinstancecheck would silently misfire and skip the ambiguity branch_cleanup_server_tool_routing_artifactsmatched rows by the mapped name, so removing one of two servers sharing a name deleted the other's routing rows as well. It now withdraws only the departing server's id from each row and drops the row when its last owner leaves, and it does so by mutating the mapping in place rather than rebinding it, since the initialize task is dispatched without being awaited and can be holding the previous dictexecute_mcp_toolresolved the target server and then passed only its name down tocall_tool, which re-derived the server from that name. That round trip discardsserver_idand picks the first registry entry with a matching name, which defeats theserver_idresolution added in #30184 whenever two servers share a name. The resolved server is now threaded through and dispatched verbatim; callers without one still fall back to resolving by name. The same drop existed on the responses API path and is fixed the same wayThe responses API surface routed by name too, and is converted the same way. It already built the caller's reachable set as
MCPServerobjects, narrowed by the key's grants and the requested server filter, then discarded that identity by flattening to display names;tool_server_mapcarried a name and dispatch re-resolved it withget_mcp_server_by_name, first match wins over a non-unique field. The map now carries the server_id resolved throughresolve_tool_routeagainst that reachable set, so a name two reachable servers share stays ambiguous on this surface as well, and dispatch looks the server up by id. A tool with no reachable owner fails closed and reports a result for its tool call rather than being droppedresolved_serveris introduced by this PR and let a caller handcall_toolany server, which is the same class one layer down, so the check sits at the chokepoint rather than at each call site.call_tooltakes the reachable set the server was resolved against and refuses to dispatch outside it, covering both the caller's server and one it resolves by name itself through the scope-blind_resolve_mcp_server_for_tool_call. Supplyingresolved_serverwithout that set is rejected, so caller-supplied identity always arrives with its provenance; both callers already computed the set, so nothing recomputes itPANW Prisma AIRS read the old map to label a server for logging, and would have received an id where it expected a display name. It now resolves through the server object and reuses the id to display-name path directly above it
The behaviour change is limited to a caller that can reach two or more servers exposing the same unprefixed tool name. That call used to succeed against an arbitrary one of them; it now returns 409 telling the caller to address the server it wants. There is no safe way to keep serving it, since the caller cannot express which upstream credential it intended. The 409 is self-clearing: once one of the servers stops exposing the contested name, the next listing withdraws that owner and the name resolves again
Out of scope and worth separate tickets:
MCPRequestHandler.is_tool_allowedcompares a server name against a list of names, so duplicate names weaken it the same way in the authorization layer, andreload_servers_from_databasecan swap the registry underneath an in-flight mapping taskFinal Attestation
Note
High Risk
Changes core MCP tool dispatch, authorization scope, and credential selection; mis-routing could send the wrong upstream secret or block valid calls, though behavior is heavily tested.
Overview
Fixes MCP proxy routing so tool names no longer last-write-win onto a server name and silently pick an upstream credential when several servers expose the same tool.
Routing model:
tool_name_to_mcp_server_name_mappingbecomestool_name_to_mcp_server_ids_mapping(tool → set ofserver_ids)._replace_server_tool_routesmakes each server’stools/list/ OpenAPI registration the authoritative replace for that server’s rows (withdraw dropped tools; eviction clears only that server’s ownership).resolve_tool_routereturns resolved / not_found / ambiguous, judging collisions against the caller’sallowed_server_ids(scopedx-mcp-serverssessions can still use unprefixed names when only one owner is in scope).Dispatch: MCP JSON-RPC returns 409
ambiguous_tool_namewhen multiple reachable servers own an unprefixed name; out-of-scope owners fail closed instead of scope-blind fallback.call_toolacceptsresolved_server+allowed_server_idsand returns 403 if the target is outside the proven reachable set. The responses APItool_server_mapand execution path useserver_idlookup and the same resolver; PANW logging resolves via server object instead of the old map.Tests cover ambiguity, scope, OpenAPI route replace semantics, and credential routing edge cases.
Reviewed by Cursor Bugbot for commit e9fd8fc. Bugbot is set up for automated code reviews on this repo. Configure here.