From 878cbac76a2d2284a69ba0a07f051d45322ae72f Mon Sep 17 00:00:00 2001 From: Yuneng Jiang Date: Sun, 6 Sep 2026 09:23:54 +0000 Subject: [PATCH] fix(mcp): keep a null tools list a no-op on toolset update Treating a null tools list as a clear meant an existing client that sends tools=null during a partial update, meaning "leave the selection alone", silently lost every tool the toolset grants. That is a permission surface, so the quiet version of it is the worst version. A toolset always has a tool list, the same way it always has a name, so a null on either is now a no-op. Emptying the selection is an explicit [], which cannot be confused with a field the caller left out, and which is what the dashboard already sends. --- .../proxy/_experimental/mcp_server/toolset_db.py | 12 ++++++++---- .../mcp_management_endpoints.py | 3 ++- tests/e2e/proxy_client.py | 12 ++++++++---- .../mcp_server/test_mcp_partial_update.py | 16 ++++++++++++---- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/toolset_db.py b/litellm/proxy/_experimental/mcp_server/toolset_db.py index 750ff2428a4..ca6234b8a5b 100644 --- a/litellm/proxy/_experimental/mcp_server/toolset_db.py +++ b/litellm/proxy/_experimental/mcp_server/toolset_db.py @@ -132,13 +132,17 @@ async def update_mcp_toolset( data: UpdateMCPToolsetRequest, touched_by: str, ) -> MCPToolset | None: - """A partial update: absent keeps, null clears, except that a toolset always has a - name, so a null ``toolset_name`` is ignored.""" + """A partial update: absent keeps, null clears. A toolset always has a name and a + tool list, so a null ``toolset_name`` or ``tools`` is a no-op rather than a clear; + emptying the tool selection is an explicit ``[]``, which cannot be mistaken for a + caller that left the field out.""" data_dict: Final = data.model_dump(exclude_unset=True, exclude={"toolset_id"}) - if "tools" in data_dict: - data_dict["tools"] = json.dumps(data_dict["tools"] or ()) if data_dict.get("toolset_name", "") is None: _ = data_dict.pop("toolset_name") + if data_dict.get("tools", "") is None: + _ = data_dict.pop("tools") + if "tools" in data_dict: + data_dict["tools"] = json.dumps(data_dict["tools"]) data_dict["updated_by"] = touched_by try: row: Final = await _toolset_table(prisma_client).update( diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 6f98717400e..2aa7fdc7393 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -3100,7 +3100,8 @@ if MCP_AVAILABLE: user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth), litellm_changed_by: str | None = Header(None), ): - """Partial update: a field left out of the payload keeps its stored value, and a field sent as null is cleared.""" + """Partial update: a field left out keeps its stored value, and a field sent as null is cleared, except + ``toolset_name`` and ``tools``, which a toolset always has; empty the tool selection with an explicit [].""" prisma_client: Final = get_prisma_client_or_throw("Database not connected. Connect a database to your proxy") if LitellmUserRoles.PROXY_ADMIN != user_api_key_dict.user_role: raise HTTPException( diff --git a/tests/e2e/proxy_client.py b/tests/e2e/proxy_client.py index ea6cc191e63..40b49030f3d 100644 --- a/tests/e2e/proxy_client.py +++ b/tests/e2e/proxy_client.py @@ -540,10 +540,14 @@ class ProxyClient: def replicas_for(self, path: str) -> Mapping[str, Transport]: """The replicas that serve `path`: every data-plane replica for an LLM route, - and for a management route the control-plane replicas, which in a split - deployment is the one service that serves it (the data-plane replicas trim - management routes) and in a monolith is every replica. Never empty: a - read-back against no replica would assert nothing and pass.""" + and for a management route the control-plane replicas, since the data-plane + replicas trim management routes and answer them 404. A monolith serves both + from every replica, so a management read-back polls all of them; a split + deployment exposes one control-plane address (there is one backend process + behind it on the stack these suites run against), so it polls that. A + control plane fronting several backends would need its own replica list to + prove each one converged, the way PROXY_REPLICA_URLS does for the gateways. + Never empty: a read-back against no replica would assert nothing and pass.""" replicas: Final = self.control_replicas if is_control_plane_path(path) else self.replicas assert replicas, f"no replica is configured to serve {path}, so a read-back there would prove nothing" return replicas diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py index ce5fd76b7ad..6bc684af8c3 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py @@ -893,10 +893,18 @@ async def test_toolset_partial_update_omits_the_fields_the_caller_left_out(): @pytest.mark.asyncio -async def test_toolset_partial_update_writes_null_tools_as_an_empty_list(): - """Prisma ``tools`` is a required Json column defaulting to [], so a null clears - the selection to none rather than writing SQL null.""" - assert await _run_toolset_update({"toolset_id": "ts-1", "tools": None}) == {"tools": "[]"} +async def test_toolset_partial_update_ignores_null_tools_rather_than_revoking_them(): + """A client that sends tools=null means "leave the selection alone", so the grants + survive. Clearing them is an explicit [], which cannot be confused with an omitted + field; treating null as a clear would silently revoke every tool the toolset grants.""" + assert await _run_toolset_update({"toolset_id": "ts-1", "tools": None, "description": "kept"}) == { + "description": "kept" + } + + +@pytest.mark.asyncio +async def test_toolset_partial_update_empties_the_selection_on_an_explicit_empty_list(): + assert await _run_toolset_update({"toolset_id": "ts-1", "tools": []}) == {"tools": "[]"} @pytest.mark.asyncio