From 83917c594afcf204cc4f0f59f6f75e9091d554aa Mon Sep 17 00:00:00 2001 From: hawkli-1994 <11769524+hawkli-1994@users.noreply.github.com> Date: Wed, 24 Jun 2026 18:54:25 +0800 Subject: [PATCH 1/3] fix(ui): preserve MCPServer argument ordering when creating stdio servers The UI was emitting the package name after the additional arguments, so a command like would produce args [-y, --stdio, package]. Build the args array in the same order shown in the command preview: [commandPrefix..., packageName, additionalArgs...]. Fixes kagent-dev/kagent#2078 --- ui/src/components/mcp/McpServerForm.tsx | 31 +++-------------- ui/src/lib/__tests__/toolUtils.test.ts | 44 +++++++++++++++++++++++++ ui/src/lib/toolUtils.ts | 27 +++++++++++++++ 3 files changed, 75 insertions(+), 27 deletions(-) diff --git a/ui/src/components/mcp/McpServerForm.tsx b/ui/src/components/mcp/McpServerForm.tsx index 04b9413148..d6ba487bf2 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"; @@ -227,44 +228,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, diff --git a/ui/src/lib/__tests__/toolUtils.test.ts b/ui/src/lib/__tests__/toolUtils.test.ts index 7d665c0386..3ec21b2541 100644 --- a/ui/src/lib/__tests__/toolUtils.test.ts +++ b/ui/src/lib/__tests__/toolUtils.test.ts @@ -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", + [{ value: "--stdio" }], + ); + expect(args).toEqual([ + "-y", + "@modelcontextprotocol/server-map", + "--stdio", + ]); + }); + + it('should include only non-empty arguments', () => { + const args = buildMCPServerArgs( + "", + "@acme/mcp-tool", + [{ value: "" }, { value: "--verbose" }, { value: "" }], + ); + expect(args).toEqual(["@acme/mcp-tool", "--verbose"]); + }); + + it('should split the command prefix on whitespace', () => { + const args = buildMCPServerArgs( + "--cache --quiet", + "@acme/mcp-tool", + [{ value: "--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", [{ value: "" }]); + expect(args).toEqual(["@acme/mcp-tool"]); + }); + }); }); diff --git a/ui/src/lib/toolUtils.ts b/ui/src/lib/toolUtils.ts index 959d15b4a0..fc43c973a1 100644 --- a/ui/src/lib/toolUtils.ts +++ b/ui/src/lib/toolUtils.ts @@ -1,6 +1,33 @@ import { k8sRefUtils } from "@/lib/k8sUtils"; import type{ Tool, McpServerTool, ToolsResponse, DiscoveredTool, TypedLocalReference, AgentResponse } from "@/types"; +interface ArgPair { + value: string; +} + +/** + * Build the `args` array for a stdio MCPServer deployment. + * + * Order is preserved as shown in the command preview: + * [commandPrefix...] [additionalArgs...] + */ +export const buildMCPServerArgs = ( + commandPrefix: string, + packageName: string, + argPairs: ArgPair[], +): string[] => { + const args: string[] = []; + if (commandPrefix.trim()) { + args.push(...commandPrefix.trim().split(/\s+/)); + } + if (packageName.trim()) { + args.push(packageName.trim()); + } + argPairs + .filter((arg) => arg.value.trim() !== "") + .forEach((arg) => args.push(arg.value.trim())); + return args; +}; // Constants for MCP server types and defaults const DEFAULT_API_GROUP = "kagent.dev"; From b4ae1b74c63e7f6db895dcb501e868091b44d988 Mon Sep 17 00:00:00 2001 From: hawkli-1994 <11769524+hawkli-1994@users.noreply.github.com> Date: Wed, 24 Jun 2026 19:22:38 +0800 Subject: [PATCH 2/3] style(ui): fix import spacing and clarify buildMCPServerArgs JSDoc Address Copilot review comments: - Use consistent spacing. - Clarify that the executor (cmd) is supplied separately from the returned args array. --- ui/src/lib/toolUtils.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ui/src/lib/toolUtils.ts b/ui/src/lib/toolUtils.ts index fc43c973a1..7e48cab25c 100644 --- a/ui/src/lib/toolUtils.ts +++ b/ui/src/lib/toolUtils.ts @@ -1,5 +1,5 @@ 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"; interface ArgPair { value: string; @@ -8,8 +8,9 @@ interface ArgPair { /** * Build the `args` array for a stdio MCPServer deployment. * - * Order is preserved as shown in the command preview: - * [commandPrefix...] [additionalArgs...] + * The executor (`cmd`) is supplied separately; this helper returns the + * remaining arguments in the same order shown in the command preview: + * [commandPrefix...] [additionalArgs...] */ export const buildMCPServerArgs = ( commandPrefix: string, From 4095e8a0c7ff0cae6ba858552b877f3e880465bc Mon Sep 17 00:00:00 2001 From: Peter Jausovec Date: Fri, 26 Jun 2026 11:28:15 -0700 Subject: [PATCH 3/3] refactor(ui): simplify MCPServer arg pairs to strings and require packageName Replace ArgPair object wrapper with plain strings for argPairs across the form, helper, and tests. buildMCPServerArgs now always includes packageName (validated by the form before calling). --- ui/src/components/mcp/McpServerForm.tsx | 54 ++++++++++++------------- ui/src/lib/__tests__/toolUtils.test.ts | 12 +++--- ui/src/lib/toolUtils.ts | 45 ++++++++++----------- 3 files changed, 52 insertions(+), 59 deletions(-) diff --git a/ui/src/components/mcp/McpServerForm.tsx b/ui/src/components/mcp/McpServerForm.tsx index d6ba487bf2..b02fc2aca4 100644 --- a/ui/src/components/mcp/McpServerForm.tsx +++ b/ui/src/components/mcp/McpServerForm.tsx @@ -22,10 +22,6 @@ export type McpServerFormProps = { onCreate: (serverRequest: ToolServerCreateRequest) => Promise; }; -interface ArgPair { - value: string; -} - interface EnvPair { key: string; value: string; @@ -43,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(""); @@ -81,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 @@ -139,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(); } }); @@ -149,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) => { @@ -158,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); }; @@ -330,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; }; @@ -415,10 +411,10 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF - @@ -522,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" /> @@ -557,7 +553,7 @@ export function McpServerForm({ supportedToolServerTypes, onCreate }: McpServerF
- + @@ -626,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 3ec21b2541..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, @@ -1034,7 +1034,7 @@ describe('Tool Utility Functions', () => { const args = buildMCPServerArgs( "-y", "@modelcontextprotocol/server-map", - [{ value: "--stdio" }], + ["--stdio"], ); expect(args).toEqual([ "-y", @@ -1047,7 +1047,7 @@ describe('Tool Utility Functions', () => { const args = buildMCPServerArgs( "", "@acme/mcp-tool", - [{ value: "" }, { value: "--verbose" }, { value: "" }], + ["", "--verbose", ""], ); expect(args).toEqual(["@acme/mcp-tool", "--verbose"]); }); @@ -1056,7 +1056,7 @@ describe('Tool Utility Functions', () => { const args = buildMCPServerArgs( "--cache --quiet", "@acme/mcp-tool", - [{ value: "--help" }], + ["--help"], ); expect(args).toEqual([ "--cache", @@ -1067,7 +1067,7 @@ describe('Tool Utility Functions', () => { }); it('should handle empty prefix and no additional arguments', () => { - const args = buildMCPServerArgs("", "@acme/mcp-tool", [{ value: "" }]); + 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 7e48cab25c..04c1cc1652 100644 --- a/ui/src/lib/toolUtils.ts +++ b/ui/src/lib/toolUtils.ts @@ -1,32 +1,29 @@ import { k8sRefUtils } from "@/lib/k8sUtils"; import type { Tool, McpServerTool, ToolsResponse, DiscoveredTool, TypedLocalReference, AgentResponse } from "@/types"; -interface ArgPair { - value: string; -} - /** * 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: ArgPair[], + argPairs: string[], ): string[] => { const args: string[] = []; if (commandPrefix.trim()) { args.push(...commandPrefix.trim().split(/\s+/)); } - if (packageName.trim()) { - args.push(packageName.trim()); - } + args.push(packageName.trim()); argPairs - .filter((arg) => arg.value.trim() !== "") - .forEach((arg) => args.push(arg.value.trim())); + .filter((arg) => arg.trim() !== "") + .forEach((arg) => args.push(arg.trim())); return args; }; @@ -47,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; @@ -128,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)); @@ -241,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; }; @@ -254,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: { @@ -294,7 +291,7 @@ export const getDiscoveredToolCategory = (tool: DiscoveredTool, serverRef: strin return parts[0]; } else { return tool.name; - } + } } return serverRef; };