From 649a83d046a005ef8d34fbd52ca87534af0c076f Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Thu, 2 Apr 2026 13:32:11 +0900 Subject: [PATCH] fix(mcp): user config overrides Claude Code .mcp.json with collision warning Previously, Claude Code's .mcp.json would silently override OpenCode user config when MCP server names collided. This was unexpected behavior since users expect their explicit OpenCode configuration to take precedence. Changes: 1. Swapped merge order: Claude Code .mcp.json is now merged BEFORE user config, so user config wins on collision 2. Added warning log when user config overrides a Claude Code MCP server: 'warning: MCP server X from user config overrides Claude Code .mcp.json' 3. Added comprehensive tests for collision scenarios Fixes #2946 --- .../mcp-config-handler-collision.test.ts | 132 ++++++++++++++++++ .../mcp-config-handler.test.ts | 1 + src/plugin-handlers/mcp-config-handler.ts | 11 +- 3 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 src/plugin-handlers/mcp-config-handler-collision.test.ts diff --git a/src/plugin-handlers/mcp-config-handler-collision.test.ts b/src/plugin-handlers/mcp-config-handler-collision.test.ts new file mode 100644 index 000000000..1b8de2fa8 --- /dev/null +++ b/src/plugin-handlers/mcp-config-handler-collision.test.ts @@ -0,0 +1,132 @@ +/// + +import { describe, test, expect, spyOn, beforeEach, afterEach } from "bun:test" +import type { OhMyOpenCodeConfig } from "../config" + +import * as mcpLoader from "../features/claude-code-mcp-loader" +import * as mcpModule from "../mcp" +import * as shared from "../shared" + +let loadMcpConfigsSpy: ReturnType +let createBuiltinMcpsSpy: ReturnType +let logSpy: ReturnType + +beforeEach(() => { + loadMcpConfigsSpy = spyOn(mcpLoader, "loadMcpConfigs").mockResolvedValue({ + servers: {}, + loadedServers: [], + }) + createBuiltinMcpsSpy = spyOn(mcpModule, "createBuiltinMcps").mockReturnValue({}) + logSpy = spyOn(shared, "log").mockImplementation(() => {}) +}) + +afterEach(() => { + loadMcpConfigsSpy.mockRestore() + createBuiltinMcpsSpy.mockRestore() + logSpy.mockRestore() +}) + +function createPluginConfig(overrides: Partial = {}): OhMyOpenCodeConfig { + return { + disabled_mcps: [], + ...overrides, + } as OhMyOpenCodeConfig +} + +const EMPTY_PLUGIN_COMPONENTS = { + commands: {}, + skills: {}, + agents: {}, + mcpServers: {}, + hooksConfigs: [], + plugins: [], + errors: [], +} + +describe("applyMcpConfig collision handling", () => { + test("merges without collision when names are unique", async () => { + //#given + const userMcp = { + userServer: { type: "remote", url: "https://user.example.com", enabled: true }, + } + + loadMcpConfigsSpy.mockResolvedValue({ + servers: { + claudeServer: { type: "remote", url: "https://claude.example.com", enabled: true }, + }, + loadedServers: [], + }) + + const config: Record = { mcp: userMcp } + const pluginConfig = createPluginConfig() + + //#when + const { applyMcpConfig } = await import("./mcp-config-handler") + await applyMcpConfig({ config, pluginConfig, pluginComponents: EMPTY_PLUGIN_COMPONENTS }) + + //#then + const mergedMcp = config.mcp as Record> + expect(mergedMcp).toHaveProperty("userServer") + expect(mergedMcp).toHaveProperty("claudeServer") + expect(mergedMcp.userServer.enabled).toBe(true) + expect(mergedMcp.claudeServer.enabled).toBe(true) + expect(logSpy).not.toHaveBeenCalledWith(expect.stringContaining("overrides Claude Code")) + }) + + test("user config wins on collision with Claude Code and logs warning", async () => { + //#given + const userMcp = { + sharedServer: { type: "remote", url: "https://user.example.com", enabled: true }, + } + + loadMcpConfigsSpy.mockResolvedValue({ + servers: { + sharedServer: { type: "remote", url: "https://claude.example.com", enabled: true }, + }, + loadedServers: [], + }) + + const config: Record = { mcp: userMcp } + const pluginConfig = createPluginConfig() + + //#when + const { applyMcpConfig } = await import("./mcp-config-handler") + await applyMcpConfig({ config, pluginConfig, pluginComponents: EMPTY_PLUGIN_COMPONENTS }) + + //#then + const mergedMcp = config.mcp as Record> + expect(mergedMcp.sharedServer.url).toBe("https://user.example.com") + expect(logSpy).toHaveBeenCalledWith( + 'warning: MCP server "sharedServer" from user config overrides Claude Code .mcp.json' + ) + }) + + test("preserves enabled:false from user config after collision with Claude Code", async () => { + //#given + const userMcp = { + sharedServer: { type: "remote", url: "https://user.example.com", enabled: false }, + } + + loadMcpConfigsSpy.mockResolvedValue({ + servers: { + sharedServer: { type: "remote", url: "https://claude.example.com", enabled: true }, + }, + loadedServers: [], + }) + + const config: Record = { mcp: userMcp } + const pluginConfig = createPluginConfig() + + //#when + const { applyMcpConfig } = await import("./mcp-config-handler") + await applyMcpConfig({ config, pluginConfig, pluginComponents: EMPTY_PLUGIN_COMPONENTS }) + + //#then + const mergedMcp = config.mcp as Record> + expect(mergedMcp.sharedServer.enabled).toBe(false) + expect(mergedMcp.sharedServer.url).toBe("https://user.example.com") + expect(logSpy).toHaveBeenCalledWith( + 'warning: MCP server "sharedServer" from user config overrides Claude Code .mcp.json' + ) + }) +}) diff --git a/src/plugin-handlers/mcp-config-handler.test.ts b/src/plugin-handlers/mcp-config-handler.test.ts index 95f73fc0d..f9fc6472f 100644 --- a/src/plugin-handlers/mcp-config-handler.test.ts +++ b/src/plugin-handlers/mcp-config-handler.test.ts @@ -164,4 +164,5 @@ describe("applyMcpConfig", () => { const mergedMcp = config.mcp as Record> expect(mergedMcp).not.toHaveProperty("plugin:custom") }) + }) diff --git a/src/plugin-handlers/mcp-config-handler.ts b/src/plugin-handlers/mcp-config-handler.ts index d4eef1ad7..82be91942 100644 --- a/src/plugin-handlers/mcp-config-handler.ts +++ b/src/plugin-handlers/mcp-config-handler.ts @@ -2,6 +2,7 @@ import type { OhMyOpenCodeConfig } from "../config"; import { loadMcpConfigs } from "../features/claude-code-mcp-loader"; import { createBuiltinMcps } from "../mcp"; import type { PluginComponents } from "./plugin-components-loader"; +import { log } from "../shared"; type McpEntry = Record; @@ -38,10 +39,18 @@ export async function applyMcpConfig(params: { ? await loadMcpConfigs(disabledMcps) : { servers: {} }; + if (userMcp) { + for (const name of Object.keys(userMcp)) { + if (name in mcpResult.servers) { + log(`warning: MCP server "${name}" from user config overrides Claude Code .mcp.json`); + } + } + } + const merged = { ...createBuiltinMcps(disabledMcps, params.pluginConfig), - ...(userMcp ?? {}), ...mcpResult.servers, + ...(userMcp ?? {}), ...params.pluginComponents.mcpServers, } as Record;