From 0226fdcbf7472018e5a7c39cecef47f5de7464d5 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 2 Aug 2026 21:54:10 -0700 Subject: [PATCH] refactor(config): centralize MCP config commits (#118462) --- src/config/mcp-config.ts | 205 +++++++++++++++------------------------ 1 file changed, 76 insertions(+), 129 deletions(-) diff --git a/src/config/mcp-config.ts b/src/config/mcp-config.ts index 6dcdd7f0bdee..e4c81c980a80 100644 --- a/src/config/mcp-config.ts +++ b/src/config/mcp-config.ts @@ -21,26 +21,18 @@ type McpArgvRestoreResult = | { ok: true; server: Record } | { ok: false; error: string }; -type ConfigMcpReadResult = - | { - ok: true; - path: string; - config: OpenClawConfig; - mcpServers: ConfigMcpServers; - baseHash?: string; - } - | { ok: false; path: string; error: string }; - +type ConfigMcpSuccess = { + path: string; + config: OpenClawConfig; + mcpServers: ConfigMcpServers; +}; +type ConfigMcpFailure = { ok: false; path: string; error: string }; +type ConfigMcpReadResult = (ConfigMcpSuccess & { ok: true; baseHash?: string }) | ConfigMcpFailure; type ConfigMcpWriteResult = - | { - ok: true; - path: string; - config: OpenClawConfig; - mcpServers: ConfigMcpServers; - removed?: boolean; - updated?: boolean; - } - | { ok: false; path: string; error: string }; + | (ConfigMcpSuccess & { ok: true; removed?: boolean; updated?: boolean }) + | ConfigMcpFailure; + +type LoadedConfigMcpServers = Extract; /** Include/exclude tool selection stored for a configured MCP server. */ type McpServerToolSelection = { @@ -81,10 +73,7 @@ function restoreMcpServerArgvSentinels(params: { ) { return { ok: false, - error: - 'Cannot restore MCP args containing "' + - REDACTED_SENTINEL + - '" without the same original argv shape.', + error: `Cannot restore MCP args containing "${REDACTED_SENTINEL}" without the same original argv shape.`, }; } @@ -92,10 +81,7 @@ function restoreMcpServerArgvSentinels(params: { if (incomingArgs.some((arg, index) => arg !== displayedArgs[index])) { return { ok: false, - error: - 'Cannot restore MCP args containing "' + - REDACTED_SENTINEL + - '" after argv changed. Replace every redacted value explicitly before editing args.', + error: `Cannot restore MCP args containing "${REDACTED_SENTINEL}" after argv changed. Replace every redacted value explicitly before editing args.`, }; } return { @@ -126,6 +112,48 @@ export async function listConfiguredMcpServers(): Promise { }; } +async function commitConfiguredMcpServers(params: { + loaded: LoadedConfigMcpServers; + servers: ConfigMcpServers; + errorLabel: string; + success?: { removed?: boolean; updated?: boolean }; + independentlyOwnedName?: string; +}): Promise { + const next = structuredClone(params.loaded.config); + if (Object.keys(params.servers).length > 0) { + next.mcp = { ...next.mcp, servers: params.servers }; + } else if (next.mcp) { + delete next.mcp.servers; + if (Object.keys(next.mcp).length === 0) { + delete next.mcp; + } + } + + const validated = validateConfigObjectWithPlugins(next); + if (!validated.ok) { + const issue = expectDefined(validated.issues[0], "issues entry at 0"); + return { + ok: false, + path: params.loaded.path, + error: `Config invalid after MCP ${params.errorLabel} (${issue.path}: ${issue.message}).`, + }; + } + await replaceConfigFile({ + nextConfig: validated.config, + baseHash: params.loaded.baseHash, + }); + if (params.independentlyOwnedName) { + markClawMcpServerIndependentlyOwned(params.independentlyOwnedName); + } + return { + ok: true, + path: params.loaded.path, + config: validated.config, + mcpServers: params.servers, + ...params.success, + }; +} + async function updateConfiguredMcpServerConfig(params: { name: string; update: (server: Record) => Record; @@ -142,46 +170,19 @@ async function updateConfiguredMcpServerConfig(params: { return loaded; } if (!Object.hasOwn(loaded.mcpServers, name)) { - return { - ok: true, - path: loaded.path, - config: loaded.config, - mcpServers: loaded.mcpServers, - updated: false, - }; + const { baseHash: _baseHash, ...unchanged } = loaded; + return { ...unchanged, updated: false }; } - const next = structuredClone(loaded.config); - const servers = normalizeConfiguredMcpServers(next.mcp?.servers); + const servers = normalizeConfiguredMcpServers(loaded.config.mcp?.servers); servers[name] = params.update({ ...servers[name] }); - next.mcp = { - ...next.mcp, + return commitConfiguredMcpServers({ + loaded, servers, - }; - - const validated = validateConfigObjectWithPlugins(next); - if (!validated.ok) { - const issue = expectDefined(validated.issues[0], "issues entry at 0"); - return { - ok: false, - path: loaded.path, - error: `Config invalid after MCP ${params.errorLabel} (${issue.path}: ${issue.message}).`, - }; - } - await replaceConfigFile({ - nextConfig: validated.config, - baseHash: loaded.baseHash, + errorLabel: params.errorLabel, + success: { updated: true }, + independentlyOwnedName: params.recordIndependentOwner === false ? undefined : name, }); - if (params.recordIndependentOwner !== false) { - markClawMcpServerIndependentlyOwned(name); - } - return { - ok: true, - path: loaded.path, - config: validated.config, - mcpServers: servers, - updated: true, - }; } export async function updateConfiguredMcpServerTools(params: { @@ -301,36 +302,14 @@ export async function setConfiguredMcpServer(params: { return { ok: false, path: loaded.path, error: "MCP server config must be a JSON object." }; } - const next = structuredClone(loaded.config); - const servers = normalizeConfiguredMcpServers(next.mcp?.servers); + const servers = normalizeConfiguredMcpServers(loaded.config.mcp?.servers); servers[name] = canonicalizeConfiguredMcpServer(restoredServer); - next.mcp = { - ...next.mcp, + return commitConfiguredMcpServers({ + loaded, servers, - }; - - const validated = validateConfigObjectWithPlugins(next); - if (!validated.ok) { - const issue = expectDefined(validated.issues[0], "issues entry at 0"); - return { - ok: false, - path: loaded.path, - error: `Config invalid after MCP set (${issue.path}: ${issue.message}).`, - }; - } - await replaceConfigFile({ - nextConfig: validated.config, - baseHash: loaded.baseHash, + errorLabel: "set", + independentlyOwnedName: params.recordIndependentOwner === false ? undefined : name, }); - if (params.recordIndependentOwner !== false) { - markClawMcpServerIndependentlyOwned(name); - } - return { - ok: true, - path: loaded.path, - config: validated.config, - mcpServers: servers, - }; } export async function unsetConfiguredMcpServer(params: { @@ -347,13 +326,8 @@ export async function unsetConfiguredMcpServer(params: { return loaded; } if (!Object.hasOwn(loaded.mcpServers, name)) { - return { - ok: true, - path: loaded.path, - config: loaded.config, - mcpServers: loaded.mcpServers, - removed: false, - }; + const { baseHash: _baseHash, ...unchanged } = loaded; + return { ...unchanged, removed: false }; } const loadedServer = loaded.mcpServers[name]; if ( @@ -369,39 +343,12 @@ export async function unsetConfiguredMcpServer(params: { }; } - const next = structuredClone(loaded.config); - const servers = normalizeConfiguredMcpServers(next.mcp?.servers); + const servers = normalizeConfiguredMcpServers(loaded.config.mcp?.servers); delete servers[name]; - if (Object.keys(servers).length > 0) { - next.mcp = { - ...next.mcp, - servers, - }; - } else if (next.mcp) { - delete next.mcp.servers; - if (Object.keys(next.mcp).length === 0) { - delete next.mcp; - } - } - - const validated = validateConfigObjectWithPlugins(next); - if (!validated.ok) { - const issue = expectDefined(validated.issues[0], "issues entry at 0"); - return { - ok: false, - path: loaded.path, - error: `Config invalid after MCP unset (${issue.path}: ${issue.message}).`, - }; - } - await replaceConfigFile({ - nextConfig: validated.config, - baseHash: loaded.baseHash, + return commitConfiguredMcpServers({ + loaded, + servers, + errorLabel: "unset", + success: { removed: true }, }); - return { - ok: true, - path: loaded.path, - config: validated.config, - mcpServers: servers, - removed: true, - }; }