Merge pull request #1721 from edxeth/fix/disable-mcps

fix(mcp): preserve user's enabled:false and apply disabled_mcps to all MCP sources

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)
+    }
+  })
+})
+

src/features/claude-code-mcp-loader/loader.ts

@@ -68,16 +68,24 @@ export function getSystemMcpServerNames(): Set<string> {
   return names
 }
 
-export async function loadMcpConfigs(): Promise<McpLoadResult> {
+export async function loadMcpConfigs(
+  disabledMcps: string[] = []
+): Promise<McpLoadResult> {
   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]

src/plugin-handlers/mcp-config-handler.test.ts

@@ -0,0 +1,167 @@
+/// <reference types="bun-types" />
+
+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<typeof spyOn>
+let createBuiltinMcpsSpy: ReturnType<typeof spyOn>
+
+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> = {}): 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<string, unknown> = { 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<string, Record<string, unknown>>
+    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<string, unknown> = { 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<string, Record<string, unknown>>
+    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<string, unknown> = { 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<string, unknown> = { 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<string, Record<string, unknown>>
+    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<string, unknown> = { 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<string, Record<string, unknown>>
+    expect(mergedMcp).not.toHaveProperty("plugin:custom")
+  })
+})

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<string, unknown>;
+
+function captureUserDisabledMcps(
+  userMcp: Record<string, unknown> | undefined
+): Set<string> {
+  const disabled = new Set<string>();
+  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<string, unknown>;
   pluginConfig: OhMyOpenCodeConfig;
   pluginComponents: PluginComponents;
 }): Promise<void> {
+  const disabledMcps = params.pluginConfig.disabled_mcps ?? [];
+  const userMcp = params.config.mcp as Record<string, unknown> | undefined;
+  const userDisabledMcps = captureUserDisabledMcps(userMcp);
+
   const mcpResult = params.pluginConfig.claude_code?.mcp ?? true
-    ? await loadMcpConfigs()
+    ? await loadMcpConfigs(disabledMcps)
     : { servers: {} };
 
-  params.config.mcp = {
+  const merged = {
-    ...createBuiltinMcps(params.pluginConfig.disabled_mcps, params.pluginConfig),
+    ...createBuiltinMcps(disabledMcps, params.pluginConfig),
-    ...(params.config.mcp as Record<string, unknown>),
+    ...(userMcp ?? {}),
     ...mcpResult.servers,
     ...params.pluginComponents.mcpServers,
-  };
+  } as Record<string, McpEntry>;
+
+  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;
 }