From dda8295af78d0acec850e1442aa0247a8b1bfa4b Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Fri, 25 Sep 2026 01:14:20 +0000 Subject: [PATCH] refactor(mcp): trim permission comments and tighten tool override wiring Co-Authored-By: bot_apk --- .../mcp_server/tool_permission_backfill.py | 4 --- ui/litellm-dashboard/eslint-suppressions.json | 3 -- .../MCPToolPermissions.test.tsx | 1 - .../MCPToolPermissions.tsx | 6 ---- .../effectiveMcpServers.ts | 28 ++++--------------- .../components/templates/keyEditFormValues.ts | 9 ++++++ .../components/templates/key_edit_view.tsx | 7 ++--- .../src/utils/mcpToolCrudClassification.ts | 8 +----- 8 files changed, 17 insertions(+), 49 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/tool_permission_backfill.py b/litellm/proxy/_experimental/mcp_server/tool_permission_backfill.py index c0c459512c0..a7f5a425a9f 100644 --- a/litellm/proxy/_experimental/mcp_server/tool_permission_backfill.py +++ b/litellm/proxy/_experimental/mcp_server/tool_permission_backfill.py @@ -257,10 +257,6 @@ async def _convert_one_row( where={ "object_permission_id": row.object_permission_id, "mcp_permission_version": {"in": [0, None]}, - # prisma-client-py has no DbNull/JsonNull sentinel for `equals` on a - # Json? column, so a stored NULL field is left unguarded rather than - # filtered with a wrong null literal; the id and version still bound - # the CAS. **{field: {"equals": value} for field, value in stored_fields if value is not None}, }, data={ diff --git a/ui/litellm-dashboard/eslint-suppressions.json b/ui/litellm-dashboard/eslint-suppressions.json index 4f93fb01047..e292d8cadd9 100644 --- a/ui/litellm-dashboard/eslint-suppressions.json +++ b/ui/litellm-dashboard/eslint-suppressions.json @@ -2065,9 +2065,6 @@ "src/components/templates/key_edit_view.tsx": { "local/filename-pascal-case": { "count": 1 - }, - "max-lines": { - "count": 1 } }, "src/components/templates/key_info_view.tsx": { diff --git a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx index b6323e91307..f480e296155 100644 --- a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx @@ -111,7 +111,6 @@ describe("MCPToolPermissions", () => { error: false, }); - // A closed allowlist makes the server legacy-editable, so Select All writes the list. renderWithProviders( = ({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [servers, accessToken, toolsetsLoading]); - // Every allowlist write goes through here so an edit is authoritative for the SERVER, not for - // one of the equivalent keys that may name it. const writeAllowedTools = (entry: EffectiveMcpServer, allowed: string[]) => { onChange(applyToolPermissionWrite({ toolPermissions, entry, allowed })); }; - // On a convention server there is no snapshot to edit: the checkbox flips this tool's entry in - // the server's overrides and nothing else. A delete tool can only sit in `allow`; a non-delete - // tool is carved out with `deny`. Anything not on the checkbox is left alone. const isDelete = (tool: MCPTool) => classifyToolOp(tool.name, tool.description || "") === "delete"; const writeToolToggle = (entry: EffectiveMcpServer, tool: MCPTool, checked: boolean) => { @@ -161,7 +156,6 @@ const MCPToolPermissions: React.FC = ({ writeAllowedTools(entry, checked ? [...current, tool.name] : current.filter((name) => name !== tool.name)); }; - // Writes the override edits that make every editable displayed tool match `checked`. const writeConventionBulk = (entry: EffectiveMcpServer, tools: readonly MCPTool[], checked: boolean) => { onOverridesChange?.( applyToolOverrideWrites({ diff --git a/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.ts b/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.ts index 44614f3bcb3..fc50b06b46c 100644 --- a/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.ts +++ b/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.ts @@ -19,8 +19,6 @@ export interface McpToolState { readonly locked: boolean; } -// The OpenAPI schema types the stored entry's arrays as optional; the forms and write helpers -// work with concrete arrays, so rows read from the API are defaulted here. export const normalizeMcpToolOverrides = ( raw: Readonly> | null | undefined, ): Record => @@ -54,9 +52,6 @@ export interface EffectiveMcpServer { // What this level actually allows on the server, which is what the backend enforces: the keyed // union widened by the toolset grant. `undefined` means nothing restricts the server from here. readonly allowedTools: readonly string[] | undefined; - // The merged mcp_tool_overrides entry for this server across every key naming it, or `undefined` - // when no key holds one. Overrides are per-tool allow/deny exceptions layered on the convention: - // on an unrestricted server a `deny` removes a non-delete tool and an `allow` re-arms a delete. readonly overrides: McpToolOverrideEntry | undefined; readonly source: McpGrantSource; } @@ -108,9 +103,9 @@ export const mcpServersForIdentifier = (allServers: readonly MCPServer[], identi // Every key in the map that names this server, id first so an id key stays the one an edit keeps. // A key spelled like this server's name still belongs to another server when that string is that // server's id, so the catalog decides membership rather than a field-by-field comparison. -export const mcpToolPermissionKeysFor = ( +export const mcpToolPermissionKeysFor = ( server: MCPServer, - toolPermissions: Readonly>, + toolPermissions: Readonly>, allServers: readonly MCPServer[], ): readonly string[] => [server.server_id, server.server_name, server.alias].filter( @@ -128,9 +123,9 @@ const mcpKeyNamesOneServerOnly = (allServers: readonly MCPServer[], key: string) // The key an edit writes: the first one that names this server and no other, falling back to the // server's own id. When the only entry is a key several servers share, that fallback creates an // id-keyed entry rather than rewriting the shared one, which would edit the other server too. -export const mcpToolPermissionKeyFor = ( +export const mcpToolPermissionKeyFor = ( server: MCPServer, - toolPermissions: Readonly>, + toolPermissions: Readonly>, allServers: readonly MCPServer[], ): string => mcpToolPermissionKeysFor(server, toolPermissions, allServers).find((key) => @@ -140,7 +135,7 @@ export const mcpToolPermissionKeyFor = ( // The union the backend enforces across equivalent keys, first-seen order preserved. export const mcpAllowedToolsFor = ( server: MCPServer, - toolPermissions: Readonly>, + toolPermissions: Readonly>, allServers: readonly MCPServer[], ): readonly string[] | undefined => { const keys = mcpToolPermissionKeysFor(server, toolPermissions, allServers); @@ -148,8 +143,6 @@ export const mcpAllowedToolsFor = ( return [...new Set(keys.flatMap((key) => toolPermissions[key] ?? []))]; }; -// The override entry merged across every key naming this server, `undefined` when no key has one. -// The backend unions allow/deny across equivalent keys the same way it unions allowlists. export const mcpToolOverridesFor = ( server: MCPServer, toolOverrides: Readonly>, @@ -272,10 +265,6 @@ export const resolveEffectiveMcpServers = ({ ); }; -// The checked/locked state a checkbox shows for one tool, mirroring the backend's -// level_allowed_tools: a keyed allowlist stays a closed editable list, a toolset grant is -// checked-but-locked, and an unrestricted (convention) server allows every non-delete tool unless -// a stored deny says otherwise — an allow only re-arms a delete, and deny always beats allow. export const mcpToolState = ( entry: EffectiveMcpServer, toolName: string, @@ -295,14 +284,9 @@ export const mcpToolState = ( return { checked: (isDeleteTool ? false : !denied) || (allowed && !denied), locked: false }; }; -// Whether this server is edited through overrides (convention mode) rather than a closed -// mcp_tool_permissions allowlist. The backend treats the same condition the same way. export const isConventionServer = (entry: EffectiveMcpServer): boolean => entry.keyedTools === undefined && entry.toolsetTools === undefined; -// Apply one checkbox change to the server's override entry: a delete tool can only be added to or -// removed from `allow` (a deny would never let it back on), and a non-delete tool flips its `deny` -// membership. Every other key in the map, and tools this edit did not touch, is preserved as-is. export const applyToolOverrideWrite = ({ toolOverrides, permissionKey, @@ -324,8 +308,6 @@ export const applyToolOverrideWrite = ({ return { ...toolOverrides, [permissionKey]: written }; }; -// Fold a list of {name, isDeleteTool} toggles over the same server entry, for Select All / -// Deselect All applied to the displayed editable tools. export const applyToolOverrideWrites = ({ toolOverrides, permissionKey, diff --git a/ui/litellm-dashboard/src/components/templates/keyEditFormValues.ts b/ui/litellm-dashboard/src/components/templates/keyEditFormValues.ts index 6560862ca3c..30f71bc4271 100644 --- a/ui/litellm-dashboard/src/components/templates/keyEditFormValues.ts +++ b/ui/litellm-dashboard/src/components/templates/keyEditFormValues.ts @@ -1,4 +1,5 @@ import { z } from "zod/v4"; +import type { UseFormReturn } from "react-hook-form"; import { KeyResponse } from "../key_team_helpers/key_list"; import { extractLoggingSettings, formatMetadataForDisplay, stripTagsFromMetadata } from "../key_info_utils"; @@ -177,6 +178,14 @@ export interface MountedFieldGates { canViewPrompts: boolean; } +export const mcpToolFieldProps = (form: Pick, "watch" | "setValue">) => ({ + toolPermissions: form.watch("mcp_tool_permissions") || {}, + onChange: (permissions: Record) => form.setValue("mcp_tool_permissions", permissions), + toolOverrides: form.watch("mcp_tool_overrides") || {}, + onOverridesChange: (overrides: Record) => + form.setValue("mcp_tool_overrides", overrides), +}); + export const toSubmittedValues = ( values: KeyEditFormValues, { canViewPolicies, canViewPrompts }: MountedFieldGates, diff --git a/ui/litellm-dashboard/src/components/templates/key_edit_view.tsx b/ui/litellm-dashboard/src/components/templates/key_edit_view.tsx index 5e99f863411..5d3ed479a7e 100644 --- a/ui/litellm-dashboard/src/components/templates/key_edit_view.tsx +++ b/ui/litellm-dashboard/src/components/templates/key_edit_view.tsx @@ -43,6 +43,7 @@ import { KeyEditFormValues, keyEditFormSchema, McpServersAndGroups, + mcpToolFieldProps, toKeyEditFormValues, toSubmittedValues, } from "./keyEditFormValues"; @@ -149,7 +150,6 @@ export function KeyEditView({ const mcpSelection = form.watch("mcp_servers_and_groups") as | { servers?: string[]; accessGroups?: string[]; toolsets?: string[] } | undefined; - const mcpToolPermissions = form.watch("mcp_tool_permissions"); useEffect(() => { const fetchModels = async () => { @@ -764,10 +764,7 @@ export function KeyEditView({ selectedServers={mcpSelection?.servers || []} selectedAccessGroups={mcpSelection?.accessGroups || []} selectedToolsets={mcpSelection?.toolsets || []} - toolPermissions={(mcpToolPermissions as Record | undefined) || {}} - onChange={(toolPerms) => form.setValue("mcp_tool_permissions", toolPerms)} - toolOverrides={form.watch("mcp_tool_overrides") || {}} - onOverridesChange={(overrides) => form.setValue("mcp_tool_overrides", overrides)} + {...mcpToolFieldProps(form)} /> diff --git a/ui/litellm-dashboard/src/utils/mcpToolCrudClassification.ts b/ui/litellm-dashboard/src/utils/mcpToolCrudClassification.ts index 9f7b948393f..cfeacae03c9 100644 --- a/ui/litellm-dashboard/src/utils/mcpToolCrudClassification.ts +++ b/ui/litellm-dashboard/src/utils/mcpToolCrudClassification.ts @@ -98,11 +98,7 @@ const tokenVariants = (token: string): string[] => (variant) => variant.length > 0, ); -// Read is checked before delete/update/create so that tools like -// `get_removed_entries` — where the primary verb is a read — are not silently -// blocked by the delete-by-default policy for new servers. This mirrors -// litellm/proxy/_experimental/mcp_server/tool_classification.py; the shared -// fixture under tests/test_litellm pins parity between the two. +// Matches litellm/proxy/_experimental/mcp_server/tool_classification.py, fixture-pinned. const classifyTokens = (tokens: string[]): CrudOp => { const variants = new Set(tokens.flatMap((token) => tokenVariants(token))); if ([...variants].some((variant) => READ_TOKENS.has(variant))) return "read"; @@ -112,8 +108,6 @@ const classifyTokens = (tokens: string[]): CrudOp => { return "unknown"; }; -// The name alone decides first; a misleading description cannot reclassify a -// tool whose name already carries a recognized verb. export function classifyToolOp(name: string, description = ""): CrudOp { const byName = classifyTokens(nameTokens(name)); if (byName !== "unknown") return byName;