From 3abc1d46baffad95211380be66ca416737957153 Mon Sep 17 00:00:00 2001 From: edxeth Date: Tue, 10 Feb 2026 15:26:17 +0100 Subject: [PATCH] fix(mcp): preserve user's enabled:false and apply disabled_mcps to all MCP sources Commit 598a4389 refactored config-handler into separate modules but dropped the disabledMcps parameter from loadMcpConfigs() and did not handle the spread-order overwrite where .mcp.json MCPs (hardcoded enabled:true) overwrote user's enabled:false from opencode.json. Changes: - Re-add disabledMcps parameter to loadMcpConfigs() in loader.ts - Capture user's enabled:false MCPs before merge, restore after - Pass disabled_mcps to loadMcpConfigs for .mcp.json filtering - Delete disabled_mcps entries from final merged result - Add 8 new tests covering both fixes --- .../claude-code-mcp-loader/loader.test.ts | 106 ++++++++++- src/features/claude-code-mcp-loader/loader.ts | 10 +- .../mcp-config-handler.test.ts | 167 ++++++++++++++++++ src/plugin-handlers/mcp-config-handler.ts | 49 ++++- 4 files changed, 325 insertions(+), 7 deletions(-) create mode 100644 src/plugin-handlers/mcp-config-handler.test.ts diff --git a/src/features/claude-code-mcp-loader/loader.test.ts b/src/features/claude-code-mcp-loader/loader.test.ts index 848c7aed5d..bd9e206d82 100644 --- a/src/features/claude-code-mcp-loader/loader.test.ts +++ b/src/features/claude-code-mcp-loader/loader.test.ts @@ -229,5 +229,109 @@ describe("getSystemMcpServerNames", () => { } finally { process.chdir(originalCwd) } - }) + }) +}) + +describe("loadMcpConfigs", () => { + beforeEach(() => { + mkdirSync(TEST_DIR, { recursive: true }) + mkdirSync(TEST_HOME, { recursive: true }) + mock.module("os", () => ({ + homedir: () => TEST_HOME, + tmpdir, + })) + mock.module("../../shared", () => ({ + getClaudeConfigDir: () => join(TEST_HOME, ".claude"), + })) + mock.module("../../shared/logger", () => ({ + log: () => {}, + })) + }) + + afterEach(() => { + mock.restore() + rmSync(TEST_DIR, { recursive: true, force: true }) + }) + + it("should skip MCPs in disabledMcps list", async () => { + //#given + const mcpConfig = { + mcpServers: { + playwright: { command: "npx", args: ["@playwright/mcp@latest"] }, + sqlite: { command: "uvx", args: ["mcp-server-sqlite"] }, + active: { command: "npx", args: ["some-mcp"] }, + }, + } + writeFileSync(join(TEST_DIR, ".mcp.json"), JSON.stringify(mcpConfig)) + + const originalCwd = process.cwd() + process.chdir(TEST_DIR) + + try { + //#when + const { loadMcpConfigs } = await import("./loader") + const result = await loadMcpConfigs(["playwright", "sqlite"]) + + //#then + expect(result.servers).not.toHaveProperty("playwright") + expect(result.servers).not.toHaveProperty("sqlite") + expect(result.servers).toHaveProperty("active") + expect(result.loadedServers.find((s) => s.name === "playwright")).toBeUndefined() + expect(result.loadedServers.find((s) => s.name === "sqlite")).toBeUndefined() + expect(result.loadedServers.find((s) => s.name === "active")).toBeDefined() + } finally { + process.chdir(originalCwd) + } + }) + + it("should load all MCPs when disabledMcps is empty", async () => { + //#given + const mcpConfig = { + mcpServers: { + playwright: { command: "npx", args: ["@playwright/mcp@latest"] }, + active: { command: "npx", args: ["some-mcp"] }, + }, + } + writeFileSync(join(TEST_DIR, ".mcp.json"), JSON.stringify(mcpConfig)) + + const originalCwd = process.cwd() + process.chdir(TEST_DIR) + + try { + //#when + const { loadMcpConfigs } = await import("./loader") + const result = await loadMcpConfigs([]) + + //#then + expect(result.servers).toHaveProperty("playwright") + expect(result.servers).toHaveProperty("active") + } finally { + process.chdir(originalCwd) + } + }) + + it("should load all MCPs when disabledMcps is not provided", async () => { + //#given + const mcpConfig = { + mcpServers: { + playwright: { command: "npx", args: ["@playwright/mcp@latest"] }, + }, + } + writeFileSync(join(TEST_DIR, ".mcp.json"), JSON.stringify(mcpConfig)) + + const originalCwd = process.cwd() + process.chdir(TEST_DIR) + + try { + //#when + const { loadMcpConfigs } = await import("./loader") + const result = await loadMcpConfigs() + + //#then + expect(result.servers).toHaveProperty("playwright") + } finally { + process.chdir(originalCwd) + } + }) }) + diff --git a/src/features/claude-code-mcp-loader/loader.ts b/src/features/claude-code-mcp-loader/loader.ts index 754b71a9a9..ae43dc1373 100644 --- a/src/features/claude-code-mcp-loader/loader.ts +++ b/src/features/claude-code-mcp-loader/loader.ts @@ -68,16 +68,24 @@ export function getSystemMcpServerNames(): Set { return names } -export async function loadMcpConfigs(): Promise { +export async function loadMcpConfigs( + disabledMcps: string[] = [] +): Promise { const servers: McpLoadResult["servers"] = {} const loadedServers: LoadedMcpServer[] = [] const paths = getMcpConfigPaths() + const disabledSet = new Set(disabledMcps) for (const { path, scope } of paths) { const config = await loadMcpConfigFile(path) if (!config?.mcpServers) continue for (const [name, serverConfig] of Object.entries(config.mcpServers)) { + if (disabledSet.has(name)) { + log(`Skipping MCP "${name}" (in disabled_mcps)`, { path }) + continue + } + if (serverConfig.disabled) { log(`Disabling MCP server "${name}"`, { path }) delete servers[name] diff --git a/src/plugin-handlers/mcp-config-handler.test.ts b/src/plugin-handlers/mcp-config-handler.test.ts new file mode 100644 index 0000000000..95f73fc0db --- /dev/null +++ b/src/plugin-handlers/mcp-config-handler.test.ts @@ -0,0 +1,167 @@ +/// + +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 + +beforeEach(() => { + loadMcpConfigsSpy = spyOn(mcpLoader, "loadMcpConfigs" as any).mockResolvedValue({ + servers: {}, + }) + createBuiltinMcpsSpy = spyOn(mcpModule, "createBuiltinMcps" as any).mockReturnValue({}) + spyOn(shared, "log" as any).mockImplementation(() => {}) +}) + +afterEach(() => { + loadMcpConfigsSpy.mockRestore() + createBuiltinMcpsSpy.mockRestore() + ;(shared.log as any)?.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", () => { + test("preserves enabled:false from user config after merge with .mcp.json MCPs", async () => { + //#given + const userMcp = { + firecrawl: { type: "remote", url: "https://firecrawl.example.com", enabled: false }, + exa: { type: "remote", url: "https://exa.example.com", enabled: true }, + } + + loadMcpConfigsSpy.mockResolvedValue({ + servers: { + firecrawl: { type: "remote", url: "https://firecrawl.example.com", enabled: true }, + exa: { type: "remote", url: "https://exa.example.com", enabled: true }, + }, + }) + + 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.firecrawl.enabled).toBe(false) + expect(mergedMcp.exa.enabled).toBe(true) + }) + + test("applies disabled_mcps to MCPs from all sources", async () => { + //#given + createBuiltinMcpsSpy.mockReturnValue({ + websearch: { type: "remote", url: "https://mcp.exa.ai/mcp", enabled: true }, + }) + + loadMcpConfigsSpy.mockResolvedValue({ + servers: { + playwright: { type: "local", command: ["npx", "@playwright/mcp"], enabled: true }, + }, + }) + + const config: Record = { mcp: {} } + const pluginConfig = createPluginConfig({ disabled_mcps: ["playwright"] as any }) + + //#when + const { applyMcpConfig } = await import("./mcp-config-handler") + await applyMcpConfig({ + config, + pluginConfig, + pluginComponents: { + ...EMPTY_PLUGIN_COMPONENTS, + mcpServers: { + "plugin:custom": { type: "local", command: ["npx", "custom"], enabled: true }, + }, + }, + }) + + //#then + const mergedMcp = config.mcp as Record> + expect(mergedMcp).not.toHaveProperty("playwright") + expect(mergedMcp).toHaveProperty("websearch") + expect(mergedMcp).toHaveProperty("plugin:custom") + }) + + test("passes disabled_mcps to loadMcpConfigs", async () => { + //#given + const config: Record = { mcp: {} } + const pluginConfig = createPluginConfig({ disabled_mcps: ["firecrawl", "exa"] as any }) + + //#when + const { applyMcpConfig } = await import("./mcp-config-handler") + await applyMcpConfig({ config, pluginConfig, pluginComponents: EMPTY_PLUGIN_COMPONENTS }) + + //#then + expect(loadMcpConfigsSpy).toHaveBeenCalledWith(["firecrawl", "exa"]) + }) + + test("works when no user MCPs have enabled:false", async () => { + //#given + const userMcp = { + exa: { type: "remote", url: "https://exa.example.com", enabled: true }, + } + + loadMcpConfigsSpy.mockResolvedValue({ + servers: { + firecrawl: { type: "remote", url: "https://firecrawl.example.com", enabled: true }, + }, + }) + + 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.exa.enabled).toBe(true) + expect(mergedMcp.firecrawl.enabled).toBe(true) + }) + + test("deletes plugin MCPs that are in disabled_mcps", async () => { + //#given + const config: Record = { mcp: {} } + const pluginConfig = createPluginConfig({ disabled_mcps: ["plugin:custom"] as any }) + + //#when + const { applyMcpConfig } = await import("./mcp-config-handler") + await applyMcpConfig({ + config, + pluginConfig, + pluginComponents: { + ...EMPTY_PLUGIN_COMPONENTS, + mcpServers: { + "plugin:custom": { type: "local", command: ["npx", "custom"], enabled: true }, + }, + }, + }) + + //#then + 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 677469be5f..d4eef1ad73 100644 --- a/src/plugin-handlers/mcp-config-handler.ts +++ b/src/plugin-handlers/mcp-config-handler.ts @@ -3,19 +3,58 @@ import { loadMcpConfigs } from "../features/claude-code-mcp-loader"; import { createBuiltinMcps } from "../mcp"; import type { PluginComponents } from "./plugin-components-loader"; +type McpEntry = Record; + +function captureUserDisabledMcps( + userMcp: Record | undefined +): Set { + const disabled = new Set(); + if (!userMcp) return disabled; + + for (const [name, value] of Object.entries(userMcp)) { + if ( + value && + typeof value === "object" && + "enabled" in value && + (value as McpEntry).enabled === false + ) { + disabled.add(name); + } + } + + return disabled; +} + export async function applyMcpConfig(params: { config: Record; pluginConfig: OhMyOpenCodeConfig; pluginComponents: PluginComponents; }): Promise { + const disabledMcps = params.pluginConfig.disabled_mcps ?? []; + const userMcp = params.config.mcp as Record | undefined; + const userDisabledMcps = captureUserDisabledMcps(userMcp); + const mcpResult = params.pluginConfig.claude_code?.mcp ?? true - ? await loadMcpConfigs() + ? await loadMcpConfigs(disabledMcps) : { servers: {} }; - params.config.mcp = { - ...createBuiltinMcps(params.pluginConfig.disabled_mcps, params.pluginConfig), - ...(params.config.mcp as Record), + const merged = { + ...createBuiltinMcps(disabledMcps, params.pluginConfig), + ...(userMcp ?? {}), ...mcpResult.servers, ...params.pluginComponents.mcpServers, - }; + } as Record; + + for (const name of userDisabledMcps) { + if (merged[name]) { + merged[name] = { ...merged[name], enabled: false }; + } + } + + const disabledSet = new Set(disabledMcps); + for (const name of disabledSet) { + delete merged[name]; + } + + params.config.mcp = merged; }