From 944679d9fd34e554380b2f706cffff9729cb18bf Mon Sep 17 00:00:00 2001 From: simonrosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:39:21 +0200 Subject: [PATCH] [AgentProfile][canvas] Give SSE/shttp MCP servers referenceable names (custom editor + marketplace) (#1386) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(mcp): give SSE/shttp MCP servers a user-given name SSE/shttp servers were serialized under auto-generated dict keys ("sse", "shttp", "shttp_1"), making them unreferenceable by name in AgentProfile mcp_server_refs. Surface an optional "Server name" field so they get a stable, user-meaningful key — consistent with stdio. - Add name? to MCPSSEServer / MCPSHTTPServer - toSdkMcpConfig: reserve(entry.name || "sse"/"shttp") so the name becomes the dict key; reserve() still de-dups collisions - parseMcpConfig: round-trip user-given names, but treat auto-generated keys as nameless so existing configs re-serialize unchanged - Plumb name through the add/update mutations and flattenMcpConfig - Add optional "Server name" input to the SSE/shttp editor form Part of #3726 (AgentProfile epic #3713). Co-Authored-By: Claude Opus 4.8 (1M context) * feat(mcp): name marketplace remote installs after the catalog slug The marketplace install path builds its own payload, separate from the custom editor. Stdio entries already carry serverName, but remote (sse/shttp) installs set no name — so GitHub, Linear, etc. landed under the auto-generated "sse"/"shttp" key and were unreferenceable in mcp_server_refs, the same gap the custom-editor fix addressed. Set name: entry.id on the remote-install payload so e.g. GitHub keys as "github". reserve() still de-dups repeat installs. Co-Authored-By: Claude Opus 4.8 (1M context) * test(e2e): expect GitHub MCP install under the "github" key The marketplace remote-install path now names servers after the catalog slug, so a GitHub install persists under mcpServers.github (referenceable in mcp_server_refs) rather than the auto-generated "shttp" fallback. Update the install/delete e2e assertions accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(mcp): validate the optional SSE/shttp server name as a safe key The Server name we added for SSE/shttp becomes the mcp_config dict key (and the mcp_server_refs reference), but the form only validated stdio names. An sse/shttp name with spaces/special chars would produce a malformed key. Apply the same ^[a-zA-Z0-9_-]+$ rule stdio uses, while keeping the field optional. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Debug Agent Co-authored-by: Claude Opus 4.8 (1M context) --- .../mcp-page/install-server-modal.test.tsx | 5 +- .../mcp-server-form.validation.test.tsx | 83 +++++++++++++++++ __tests__/utils/mcp-config.test.ts | 92 +++++++++++++++++++ .../mcp-page/install-server-modal.tsx | 4 + .../settings/mcp-settings/mcp-server-form.tsx | 21 +++++ src/hooks/mutation/use-add-mcp-server.ts | 2 + src/hooks/mutation/use-update-mcp-server.ts | 2 + src/i18n/translation.json | 17 ++++ src/types/settings.ts | 2 + src/utils/mcp-config.ts | 28 +++++- src/utils/mcp-installed-servers.ts | 2 + .../mock-llm/mcp/mock-llm-mcp-github.spec.ts | 12 ++- 12 files changed, 262 insertions(+), 8 deletions(-) diff --git a/__tests__/components/features/mcp-page/install-server-modal.test.tsx b/__tests__/components/features/mcp-page/install-server-modal.test.tsx index 8a16af8130..bb786e797a 100644 --- a/__tests__/components/features/mcp-page/install-server-modal.test.tsx +++ b/__tests__/components/features/mcp-page/install-server-modal.test.tsx @@ -250,8 +250,11 @@ describe("InstallServerModal", () => { .agent_settings_diff as { mcp_config: { mcpServers: Record }; }; + // Remote installs are now keyed by the catalog slug ("linear") rather + // than the auto-generated "shttp" fallback, so the server is + // referenceable by name in mcp_server_refs. expect(sent.mcp_config.mcpServers).toMatchObject({ - shttp: { + linear: { url: "https://mcp.linear.app/mcp", headers: { Authorization: "Bearer lin_api_secret" }, }, diff --git a/__tests__/components/features/settings/mcp-settings/mcp-server-form.validation.test.tsx b/__tests__/components/features/settings/mcp-settings/mcp-server-form.validation.test.tsx index 6b290c94b6..c0bac84a2b 100644 --- a/__tests__/components/features/settings/mcp-settings/mcp-server-form.validation.test.tsx +++ b/__tests__/components/features/settings/mcp-settings/mcp-server-form.validation.test.tsx @@ -56,6 +56,89 @@ describe("MCPServerForm validation", () => { expect(onSubmit).toHaveBeenCalledTimes(1); }); + it("includes an optional server name for sse/shttp servers", () => { + const onSubmit = vi.fn(); + + render( + , + ); + + fireEvent.change(screen.getByTestId("server-name-input"), { + target: { value: "my-search" }, + }); + fireEvent.change(screen.getByTestId("url-input"), { + target: { value: "https://api.example.com" }, + }); + + fireEvent.click(screen.getByTestId("submit-button")); + + expect(onSubmit).toHaveBeenCalledTimes(1); + expect(onSubmit.mock.calls[0][0]).toMatchObject({ + type: "sse", + name: "my-search", + url: "https://api.example.com", + }); + }); + + it("rejects an sse/shttp server name with unsafe characters", () => { + const onSubmit = vi.fn(); + + render( + , + ); + + fireEvent.change(screen.getByTestId("server-name-input"), { + target: { value: "my server" }, + }); + fireEvent.change(screen.getByTestId("url-input"), { + target: { value: "https://api.example.com" }, + }); + + fireEvent.click(screen.getByTestId("submit-button")); + + // A name with a space can't be a safe mcp_config key, so submission is + // blocked rather than persisted under a malformed key. + expect( + screen.getByText("SETTINGS$MCP_ERROR_NAME_INVALID"), + ).toBeInTheDocument(); + expect(onSubmit).not.toHaveBeenCalled(); + }); + + it("omits the name when the sse/shttp name field is left blank", () => { + const onSubmit = vi.fn(); + + render( + , + ); + + fireEvent.change(screen.getByTestId("url-input"), { + target: { value: "https://api.example.com" }, + }); + + fireEvent.click(screen.getByTestId("submit-button")); + + expect(onSubmit).toHaveBeenCalledTimes(1); + expect(onSubmit.mock.calls[0][0].name).toBeUndefined(); + }); + it("rejects duplicate URLs across sse/shttp types", () => { const onSubmit = vi.fn(); diff --git a/__tests__/utils/mcp-config.test.ts b/__tests__/utils/mcp-config.test.ts index 61d2dceec7..b978872b04 100644 --- a/__tests__/utils/mcp-config.test.ts +++ b/__tests__/utils/mcp-config.test.ts @@ -101,6 +101,98 @@ describe("toSdkMcpConfig", () => { expect(Object.keys(out!.mcpServers)).toEqual(["stdio", "stdio_1"]); }); + it("uses a user-given name as the sse/shttp dict key", () => { + const config: MCPConfig = { + sse_servers: [{ name: "my-search", url: "https://sse.example" }], + shttp_servers: [{ name: "my-docs", url: "https://shttp.example" }], + stdio_servers: [], + }; + + const out = toSdkMcpConfig(config); + + expect(Object.keys(out!.mcpServers)).toEqual(["my-search", "my-docs"]); + expect(out!.mcpServers["my-search"]).toMatchObject({ + url: "https://sse.example", + transport: "sse", + }); + }); + + it("falls back to the base name for unnamed sse/shttp entries", () => { + const config: MCPConfig = { + sse_servers: [{ name: "named", url: "https://a" }, { url: "https://b" }], + shttp_servers: [{ url: "https://c" }], + stdio_servers: [], + }; + + const out = toSdkMcpConfig(config); + + expect(Object.keys(out!.mcpServers)).toEqual(["named", "sse", "shttp"]); + }); + + it("de-dups colliding user-given sse/shttp names with a suffix", () => { + const config: MCPConfig = { + sse_servers: [ + { name: "search", url: "https://a" }, + { name: "search", url: "https://b" }, + ], + shttp_servers: [], + stdio_servers: [], + }; + + const out = toSdkMcpConfig(config); + + expect(Object.keys(out!.mcpServers)).toEqual(["search", "search_1"]); + }); + + it("round-trips a user-given sse/shttp name through parse → write", () => { + const persisted = { + mcpServers: { + "my-search": { url: "https://x", transport: "sse" }, + "my-docs": { url: "https://y" }, + }, + }; + + const parsed = parseMcpConfig(persisted); + + expect(parsed.sse_servers).toEqual([ + { name: "my-search", url: "https://x" }, + ]); + expect(parsed.shttp_servers).toEqual([ + { name: "my-docs", url: "https://y" }, + ]); + + const written = toSdkMcpConfig(parsed); + expect(Object.keys(written!.mcpServers).sort()).toEqual([ + "my-docs", + "my-search", + ]); + }); + + it("does not surface auto-generated sse/shttp keys as user names", () => { + // Keys matching the fallback pattern carry no user intent, so parsing + // must leave `name` unset — otherwise the auto key would become a + // sticky, user-facing name on the next edit. + const persisted = { + mcpServers: { + sse: { url: "https://a", transport: "sse" }, + sse_1: { url: "https://b", transport: "sse" }, + shttp: { url: "https://c" }, + shttp_2: { url: "https://d" }, + }, + }; + + const parsed = parseMcpConfig(persisted); + + expect(parsed.sse_servers).toEqual([ + { url: "https://a" }, + { url: "https://b" }, + ]); + expect(parsed.shttp_servers).toEqual([ + { url: "https://c" }, + { url: "https://d" }, + ]); + }); + it("returns null when there are no servers", () => { expect( toSdkMcpConfig({ sse_servers: [], shttp_servers: [], stdio_servers: [] }), diff --git a/src/components/features/mcp-page/install-server-modal.tsx b/src/components/features/mcp-page/install-server-modal.tsx index 4eb6183f9a..59355970b0 100644 --- a/src/components/features/mcp-page/install-server-modal.tsx +++ b/src/components/features/mcp-page/install-server-modal.tsx @@ -261,6 +261,10 @@ export function InstallServerModal({ const payload: MCPServerConfig = { id: `${template.kind}-${uuidv4()}`, type: template.kind, + // Name remote servers after the catalog slug (e.g. "github") so they + // get a referenceable mcp_config key instead of the auto-generated + // "sse"/"shttp" fallback. Stdio installs already carry serverName. + name: entry.id, url: template.url, ...(needsCredential && apiKey && { api_key: apiKey }), }; diff --git a/src/components/features/settings/mcp-settings/mcp-server-form.tsx b/src/components/features/settings/mcp-settings/mcp-server-form.tsx index 6c6d7788e3..1fec7f22b8 100644 --- a/src/components/features/settings/mcp-settings/mcp-server-form.tsx +++ b/src/components/features/settings/mcp-settings/mcp-server-form.tsx @@ -173,6 +173,14 @@ export function MCPServerForm({ const urlDupError = validateUrlUniqueness(url); if (urlDupError) return urlDupError; + // The name is optional, but when provided it becomes the mcp_config + // key (and the reference used in mcp_server_refs), so hold it to the + // same safe-identifier rule as stdio names. + const name = formData.get("name")?.toString().trim() || ""; + if (name && !/^[a-zA-Z0-9_-]+$/.test(name)) { + return t(I18nKey.SETTINGS$MCP_ERROR_NAME_INVALID); + } + // Validate timeout for SHTTP servers only if (serverType === "shttp") { const timeoutStr = formData.get("timeout")?.toString() || ""; @@ -222,12 +230,14 @@ export function MCPServerForm({ }; if (serverType === "sse" || serverType === "shttp") { + const name = formData.get("name")?.toString().trim(); const url = formData.get("url")?.toString().trim(); const apiKey = formData.get("api_key")?.toString().trim(); const timeoutStr = formData.get("timeout")?.toString().trim(); const serverConfig: MCPServerConfig = { ...baseConfig, + ...(name && { name }), url: url!, ...(apiKey && { api_key: apiKey }), }; @@ -323,6 +333,17 @@ export function MCPServerForm({ {(serverType === "sse" || serverType === "shttp") && ( <> + + ; type SdkMcpConfig = { mcpServers: Record }; +/** + * The dict key for an SSE/SHTTP server doubles as its display name once a + * user provides one. Keys that match the auto-generated fallback pattern + * (``sse``, ``shttp``, ``shttp_1``, …) emitted by {@link toSdkMcpConfig} + * carry no user intent, so we surface them as "no name" — re-serializing + * regenerates the identical fallback key and existing configs are left + * untouched. + */ +function userGivenServerName( + serverName: string, + base: "sse" | "shttp", +): string | undefined { + const autoGenerated = new RegExp(`^${base}(_\\d+)?$`); + return autoGenerated.test(serverName) ? undefined : serverName; +} + function apiKeyFromAuthorizationHeader(value: unknown): string | undefined { if (Array.isArray(value)) { return value @@ -114,11 +130,15 @@ export function parseMcpConfig(value: unknown): MCPConfig { if (apiKey) server.api_key = apiKey; migratedShttpServers.push(server); } else if (transport === "sse") { + const name = userGivenServerName(serverName, "sse"); const server: MCPSSEServer = { url }; + if (name) server.name = name; if (apiKey) server.api_key = apiKey; sseServers.push(server); } else { + const name = userGivenServerName(serverName, "shttp"); const server: MCPSHTTPServer = { url }; + if (name) server.name = name; if (apiKey) server.api_key = apiKey; if (serverConfig.timeout != null) { server.timeout = serverConfig.timeout as number; @@ -180,26 +200,30 @@ export function toSdkMcpConfig(config: MCPConfig): SdkMcpConfig | null { for (const entry of config.sse_servers) { const server: SdkMcpServerConfig = {}; + let name: string | undefined; if (typeof entry === "string") { server.url = entry; } else { + name = entry.name; server.url = entry.url; Object.assign(server, getAuthorizationHeaders(entry.api_key)); } server.transport = "sse"; - mcpServers[reserve("sse")] = server; + mcpServers[reserve(name || "sse")] = server; } for (const entry of config.shttp_servers) { const server: SdkMcpServerConfig = {}; + let name: string | undefined; if (typeof entry === "string") { server.url = entry; } else { + name = entry.name; server.url = entry.url; Object.assign(server, getAuthorizationHeaders(entry.api_key)); if (entry.timeout != null) server.timeout = entry.timeout; } - mcpServers[reserve("shttp")] = server; + mcpServers[reserve(name || "shttp")] = server; } for (const entry of config.stdio_servers) { diff --git a/src/utils/mcp-installed-servers.ts b/src/utils/mcp-installed-servers.ts index 403402b13d..d1465fcc56 100644 --- a/src/utils/mcp-installed-servers.ts +++ b/src/utils/mcp-installed-servers.ts @@ -6,6 +6,7 @@ export function flattenMcpConfig(config: MCPConfig): MCPServerConfig[] { ...config.sse_servers.map((server, index) => ({ id: `sse-${index}`, type: "sse" as const, + name: typeof server === "object" ? server.name : undefined, url: typeof server === "string" ? server : server.url, api_key: typeof server === "object" ? server.api_key : undefined, })), @@ -20,6 +21,7 @@ export function flattenMcpConfig(config: MCPConfig): MCPServerConfig[] { ...config.shttp_servers.map((server, index) => ({ id: `shttp-${index}`, type: "shttp" as const, + name: typeof server === "object" ? server.name : undefined, url: typeof server === "string" ? server : server.url, api_key: typeof server === "object" ? server.api_key : undefined, timeout: typeof server === "object" ? server.timeout : undefined, diff --git a/tests/e2e/mock-llm/mcp/mock-llm-mcp-github.spec.ts b/tests/e2e/mock-llm/mcp/mock-llm-mcp-github.spec.ts index 85b4a387ed..001cfa2cf1 100644 --- a/tests/e2e/mock-llm/mcp/mock-llm-mcp-github.spec.ts +++ b/tests/e2e/mock-llm/mcp/mock-llm-mcp-github.spec.ts @@ -152,12 +152,14 @@ test.describe("MCP GitHub server install flow", () => { const mcpConfig = settings?.agent_settings?.mcp_config; expect(mcpConfig).toBeTruthy(); - // The GitHub server should be stored as a hosted streamable HTTP server. - // The settings API redacts persisted secrets, so the raw PAT must not be - // readable after installation. + // The GitHub server should be stored as a hosted streamable HTTP server, + // keyed by the catalog slug ("github") so it is referenceable by name in + // mcp_server_refs — not the auto-generated "shttp" fallback. The settings + // API redacts persisted secrets, so the raw PAT must not be readable after + // installation. const mcpServers = mcpConfig?.mcpServers ?? mcpConfig?.shttp_servers; expect(mcpServers).toBeTruthy(); - expect(mcpServers?.shttp).toMatchObject({ + expect(mcpServers?.github).toMatchObject({ url: GITHUB_HOSTED_MCP_URL, headers: { Authorization: "", @@ -178,7 +180,7 @@ test.describe("MCP GitHub server install flow", () => { agent_settings_diff: { mcp_config: { mcpServers: { - shttp: { + github: { url: GITHUB_HOSTED_MCP_URL, headers: { Authorization: `Bearer ${FAKE_PAT}`,