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;