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
This commit is contained in:
@@ -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)
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -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]
|
||||
|
||||
167
src/plugin-handlers/mcp-config-handler.test.ts
Normal file
167
src/plugin-handlers/mcp-config-handler.test.ts
Normal file
@@ -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")
|
||||
})
|
||||
})
|
||||
@@ -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 = {
|
||||
...createBuiltinMcps(params.pluginConfig.disabled_mcps, params.pluginConfig),
|
||||
...(params.config.mcp as Record<string, unknown>),
|
||||
const merged = {
|
||||
...createBuiltinMcps(disabledMcps, params.pluginConfig),
|
||||
...(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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user