diff --git a/ui/src/components/mcp/McpServerForm.tsx b/ui/src/components/mcp/McpServerForm.tsx index 04b9413148..b02fc2aca4 100644 --- a/ui/src/components/mcp/McpServerForm.tsx +++ b/ui/src/components/mcp/McpServerForm.tsx @@ -12,6 +12,7 @@ import type { RemoteMCPServer, MCPServer, ToolServerCreateRequest } from "@/type import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@/components/ui/select"; import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from "@/components/ui/tooltip"; import { createRFC1123ValidName, isResourceNameValid } from "@/lib/utils"; +import { buildMCPServerArgs } from "@/lib/toolUtils"; import { NamespaceCombobox } from "@/components/NamespaceCombobox"; import { Checkbox } from "@/components/ui/checkbox"; @@ -21,10 +22,6 @@ export type McpServerFormProps = { onCreate: (serverRequest: ToolServerCreateRequest) => Promise; }; -interface ArgPair { - value: string; -} - interface EnvPair { key: string; value: string; @@ -42,10 +39,10 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF const [commandType, setCommandType] = useState("npx"); const [commandPrefix, setCommandPrefix] = useState(""); const [packageName, setPackageName] = useState(""); - const [argPairs, setArgPairs] = useState([{ value: "" }]); + const [argPairs, setArgPairs] = useState([""]); const [envPairs, setEnvPairs] = useState([{ key: "", value: "" }]); const [commandPreview, setCommandPreview] = useState(""); - + // SseServer parameters const [url, setUrl] = useState(""); @@ -80,36 +77,36 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF const urlObj = new URL(url.trim()); // Convert hostname to RFC 1123 compliant format let hostname = urlObj.hostname.toLowerCase(); - + // Replace invalid characters with hyphens hostname = hostname.replace(/[^a-z0-9.-]/g, "-"); - + // Replace multiple consecutive hyphens with a single hyphen hostname = hostname.replace(/-+/g, "-"); - + // Remove hyphens at the beginning and end hostname = hostname.replace(/^-+|-+$/g, ""); - + // If the hostname starts with a dot, prepend an 'a' if (hostname.startsWith(".")) { hostname = "a" + hostname; } - + // If the hostname ends with a dot, append an 'a' if (hostname.endsWith(".")) { hostname = hostname + "a"; } - + // If it doesn't start with alphanumeric, prepend 'server-' if (!/^[a-z0-9]/.test(hostname)) { hostname = "server-" + hostname; } - + // If it doesn't end with alphanumeric, append '-server' if (!/[a-z0-9]$/.test(hostname)) { hostname = hostname + "-server"; } - + generatedName = hostname; } catch { // If URL is invalid, use a default name @@ -138,8 +135,8 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF } argPairs.forEach((arg) => { - if (arg.value.trim()) { - preview += " " + arg.value.trim(); + if (arg.trim()) { + preview += " " + arg.trim(); } }); @@ -148,7 +145,7 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF }, [activeTab, commandType, commandPrefix, packageName, argPairs]); const addArgPair = () => { - setArgPairs([...argPairs, { value: "" }]); + setArgPairs([...argPairs, ""]); }; const removeArgPair = (index: number) => { @@ -157,7 +154,7 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF const updateArgPair = (index: number, newValue: string) => { const updatedPairs = [...argPairs]; - updatedPairs[index].value = newValue; + updatedPairs[index] = newValue; setArgPairs(updatedPairs); }; @@ -227,44 +224,20 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF // Create MCPServer for stdio-based server let image: string; let cmd: string; - let args: string[]; if (commandType === "uvx") { // Use uvx with the official uv image image = "ghcr.io/astral-sh/uv:debian"; cmd = "uvx"; - - // Build args array: [args..., packageName] - args = []; - if (commandPrefix.trim()) { - // Split command prefix and add to args - args.push(...commandPrefix.trim().split(/\s+/)); - } - // Add additional arguments first - argPairs.filter((arg) => arg.value.trim() !== "").forEach((arg) => { - args.push(arg.value.trim()); - }); - // Add package name at the end - args.push(packageName.trim()); } else { // Use npx with Node.js image image = "node:24-alpine3.21"; cmd = "npx"; - - // Build args array: [args..., packageName] - args = []; - if (commandPrefix.trim()) { - // Split command prefix and add to args - args.push(...commandPrefix.trim().split(/\s+/)); - } - // Add additional arguments first - argPairs.filter((arg) => arg.value.trim() !== "").forEach((arg) => { - args.push(arg.value.trim()); - }); - // Add package name at the end - args.push(packageName.trim()); } + // Build args array: [commandPrefix..., packageName, additionalArgs...] + const args: string[] = buildMCPServerArgs(commandPrefix, packageName, argPairs); + const mcpServer: MCPServer = { metadata: { name: finalServerName, @@ -353,19 +326,19 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF if (errorMsg.includes("already exists")) { return "A server with this name already exists. Please choose a different name."; } - + if (errorMsg.includes("Failed to create server")) { return "Failed to create server. Please check your configuration and try again."; } - + if (errorMsg.includes("Network error")) { return "Network error: Could not connect to the server. Please check your connection and try again."; } - + if (errorMsg.includes("Request timed out")) { return "Request timed out: The server took too long to respond. Please try again later."; } - + // Return the original error if no specific formatting is needed return errorMsg; }; @@ -438,10 +411,10 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF - @@ -545,7 +518,7 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF
{argPairs.map((pair, index) => (
- updateArgPair(index, e.target.value)} className="flex-1" /> + updateArgPair(index, e.target.value)} className="flex-1" /> @@ -580,7 +553,7 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF
- + @@ -649,4 +622,4 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF ); -} \ No newline at end of file +} diff --git a/ui/src/lib/__tests__/toolUtils.test.ts b/ui/src/lib/__tests__/toolUtils.test.ts index 7d665c0386..4070cccd0f 100644 --- a/ui/src/lib/__tests__/toolUtils.test.ts +++ b/ui/src/lib/__tests__/toolUtils.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it, jest, beforeEach, afterEach } from '@jest/globals'; -import { - isMcpTool, +import { + isMcpTool, isAgentTool, groupMcpToolsByServer, getToolIdentifier, @@ -16,6 +16,7 @@ import { getDiscoveredToolDescription, getDiscoveredToolCategory, getDiscoveredToolIdentifier, + buildMCPServerArgs, } from '../toolUtils'; import { k8sRefUtils } from '../k8sUtils'; import { Tool, ToolsResponse, DiscoveredTool } from "@/types"; @@ -1027,4 +1028,47 @@ describe('Tool Utility Functions', () => { expect(getToolDescription(malformedMcpTool, [])).toBe("No description available"); }); }); + + describe('buildMCPServerArgs', () => { + it('should place the package before additional arguments', () => { + const args = buildMCPServerArgs( + "-y", + "@modelcontextprotocol/server-map", + ["--stdio"], + ); + expect(args).toEqual([ + "-y", + "@modelcontextprotocol/server-map", + "--stdio", + ]); + }); + + it('should include only non-empty arguments', () => { + const args = buildMCPServerArgs( + "", + "@acme/mcp-tool", + ["", "--verbose", ""], + ); + expect(args).toEqual(["@acme/mcp-tool", "--verbose"]); + }); + + it('should split the command prefix on whitespace', () => { + const args = buildMCPServerArgs( + "--cache --quiet", + "@acme/mcp-tool", + ["--help"], + ); + expect(args).toEqual([ + "--cache", + "--quiet", + "@acme/mcp-tool", + "--help", + ]); + }); + + it('should handle empty prefix and no additional arguments', () => { + const args = buildMCPServerArgs("", "@acme/mcp-tool", [""]); + expect(args).toEqual(["@acme/mcp-tool"]); + }); + }); }); diff --git a/ui/src/lib/toolUtils.ts b/ui/src/lib/toolUtils.ts index 959d15b4a0..04c1cc1652 100644 --- a/ui/src/lib/toolUtils.ts +++ b/ui/src/lib/toolUtils.ts @@ -1,6 +1,31 @@ import { k8sRefUtils } from "@/lib/k8sUtils"; -import type{ Tool, McpServerTool, ToolsResponse, DiscoveredTool, TypedLocalReference, AgentResponse } from "@/types"; +import type { Tool, McpServerTool, ToolsResponse, DiscoveredTool, TypedLocalReference, AgentResponse } from "@/types"; +/** + * Build the `args` array for a stdio MCPServer deployment. + * + * The executor (`cmd`) is supplied separately; this helper returns the + * remaining arguments in the same order shown in the command preview: + * [commandPrefix...] [additionalArgs...] + * + * `packageName` is required and is always included; callers must validate + * that it is non-empty before invoking this helper. + */ +export const buildMCPServerArgs = ( + commandPrefix: string, + packageName: string, + argPairs: string[], +): string[] => { + const args: string[] = []; + if (commandPrefix.trim()) { + args.push(...commandPrefix.trim().split(/\s+/)); + } + args.push(packageName.trim()); + argPairs + .filter((arg) => arg.trim() !== "") + .forEach((arg) => args.push(arg.trim())); + return args; +}; // Constants for MCP server types and defaults const DEFAULT_API_GROUP = "kagent.dev"; @@ -19,17 +44,17 @@ export const isAgentTool = (value: unknown): value is { type: "Agent"; agent: Ty // Compare server names it handles both "namespace/name" refs and plain names export const serverNamesMatch = (serverName1: string, serverName2: string): boolean => { if (!serverName1 || !serverName2) return false; - + if (serverName1 === serverName2) return true; - + try { - const name1 = k8sRefUtils.isValidRef(serverName1) - ? k8sRefUtils.fromRef(serverName1).name + const name1 = k8sRefUtils.isValidRef(serverName1) + ? k8sRefUtils.fromRef(serverName1).name : serverName1; - const name2 = k8sRefUtils.isValidRef(serverName2) - ? k8sRefUtils.fromRef(serverName2).name + const name2 = k8sRefUtils.isValidRef(serverName2) + ? k8sRefUtils.fromRef(serverName2).name : serverName2; - + return name1 === name2; } catch { return false; @@ -100,10 +125,10 @@ export const groupMcpToolsByServer = (tools: Tool[]): { // Convert to Tool objects- preserve original kind, apiGroup, and namespace from the first tool of each server const groupedMcpTools = Array.from(mcpToolsByServer.entries()).map(([serverNameRef, toolNamesSet]) => { // Find the first tool from this server to get its kind, apiGroup, and namespace - const originalTool = tools.find(tool => + const originalTool = tools.find(tool => isMcpTool(tool) && tool.mcpServer?.name === serverNameRef ); - + const originalMcpServer = originalTool?.mcpServer; const rawApprovals = mcpApprovalsByServer.get(serverNameRef) || new Set(); const mergedApprovals = Array.from(rawApprovals).filter((n) => toolNamesSet.has(n)); @@ -213,7 +238,7 @@ export const getToolResponseCategory = (tool: ToolsResponse | undefined | null): return parts[0]; } else { return (tool as ToolsResponse).id; - } + } } return (tool as ToolsResponse).server_name; }; @@ -226,17 +251,17 @@ export const getToolResponseIdentifier = (tool: ToolsResponse | undefined | null // Convert DiscoveredTool to Tool for agent creation export const toolResponseToAgentTool = (tool: ToolsResponse, serverRef: string): Tool => { const { apiGroup, kind } = parseGroupKind(tool.group_kind); - + // Parse namespace and name from serverRef if it's in namespace/name format let name = serverRef; let namespace: string | undefined; - + if (k8sRefUtils.isValidRef(serverRef)) { const parsed = k8sRefUtils.fromRef(serverRef); name = parsed.name; namespace = parsed.namespace; } - + return { type: MCP_SERVER_TYPE, mcpServer: { @@ -266,7 +291,7 @@ export const getDiscoveredToolCategory = (tool: DiscoveredTool, serverRef: strin return parts[0]; } else { return tool.name; - } + } } return serverRef; };