From 66cb795034a3170ffb6b12f9ae0d0004e671335f Mon Sep 17 00:00:00 2001 From: LM40R4Y <92870720+rayss868@users.noreply.github.com> Date: Sun, 18 Oct 2026 09:01:53 +0700 Subject: [PATCH] fix: fall back to global disabledMcpServers when project has none configured (#1935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(model): enable model discovery for remote custom OpenAI-compatible providers Only skip descriptor discovery for local custom providers (Ollama, LM Studio) that rely on the legacy scoped-cache path. Remote custom providers (custom gateways, self-hosted APIs) now proceed with descriptor discovery so their dynamically-discovered models appear in the /model picker and /model refresh works. Previously, the routeId === 'custom' guard unconditionally returned null, preventing ALL custom providers from using descriptor discovery. Remote providers fell back to hardcoded default options while local providers correctly used the legacy path via getAdditionalModelOptionsCacheScope(). * fix: fall back to global disabledMcpServers when project has none configured When a project directory has no disabledMcpServers entry in its per-project config isMcpServerDisabled() returns false for every server, causing all configured MCP servers to be enabled in that project. - isMcpServerDisabled() now falls back to the global disabledMcpServers list when the project config does not explicitly set one (undefined). - setMcpServerEnabled() persists toggles both to the project config and the global config, so the enabled/disabled state applies uniformly across all projects. - GlobalConfig type gains the optional disabledMcpServers field. * fix: skip global disabledMcpServers write for builtin default-disabled servers Builtin servers track state through enabledMcpServers (allow-list), and isMcpServerDisabled() returns early on that path without reading disabledMcpServers. Guard the global write with isDefaultDisabledBuiltin() to avoid polluting the persisted config with builtin server names. * fix: address review feedback on #1935 - M1: Only propagate disable to global (enable stays project-local) - L1: Add disabledMcpServers to GLOBAL_CONFIG_KEYS - L2: Add comprehensive test coverage for MCP config toggles - Update PR description to justify model.tsx bundling M2 (empty-list key removal) intentionally omitted — conflicts with M1: enabling a server that exists in global list would remain disabled. * fix: make config tests resilient by directly manipulating test config objects Avoid relying on saveCurrentProjectConfig for test setup since its test-mode Object.assign implementation cannot delete keys. * debug: add NODE_ENV logging to config test * fix: use saveCurrentProjectConfig instead of direct property set Direct property set on getCurrentProjectConfig() returns different objects between calls in CI, but saveCurrentProjectConfig correctly updates the shared test singleton. * fix(mcp): handle global fallback enable/disable edge cases - When enabling a server disabled only via the global fallback, explicitly materialize an empty project-level disabledMcpServers to override it - When disabling a server on a project with no override, initialize the project list from the inherited global list to preserve existing disables - Add sharedMutationLock to config.test.ts for suite safety - Reset disabledMcpServers to undefined in afterEach cleanup - Add regression tests for global fallback enable/disable interactions * fix(mcp): preserve inherited disables and narrow PR scope * Preserve concurrent MCP disables in first project override * Publish stale inherited MCP toggles even when disk matches * fix(mcp): publish local toggles after peer project overrides --------- Co-authored-by: jatmn --- docs/advanced-setup.md | 15 +++ src/services/mcp/config.persistence.test.ts | 87 +++++++++++++++ src/services/mcp/config.test.ts | 118 ++++++++++++++++++++ src/services/mcp/config.ts | 36 +++++- src/utils/config.ts | 17 ++- 5 files changed, 264 insertions(+), 9 deletions(-) create mode 100644 src/services/mcp/config.persistence.test.ts create mode 100644 src/services/mcp/config.test.ts diff --git a/docs/advanced-setup.md b/docs/advanced-setup.md index 5ff1aa7ef..6d23c8622 100644 --- a/docs/advanced-setup.md +++ b/docs/advanced-setup.md @@ -923,3 +923,18 @@ teammate active-message compaction, malformed hard-cap overrides, and pruned-history tool-call/tool-result pairing. They are not a substitute for a multi-hour manual soak, but they pin the bounded-history and conversion invariants that previously let long sessions grow until Node/V8 OOM. + +## MCP enable/disable scope + +Disabling an MCP server through `/mcp` records its name in the project's +`disabledMcpServers` list and the global list in `.openclaude.json`. Projects +without their own list inherit the global disables. Existing project lists, +including an explicit empty list, take precedence over the global list. + +Enabling a server affects only the current project and preserves other disabled +servers. The first state change starts from the inherited list, then saves a project +override; that project no longer inherits later global changes. Remove the +project's `disabledMcpServers` key to restore inheritance. Builtin servers that +are disabled by default keep their separate project-level `enabledMcpServers` +allowlist. Existing running sessions are not forcibly disconnected by a toggle +in another project. diff --git a/src/services/mcp/config.persistence.test.ts b/src/services/mcp/config.persistence.test.ts new file mode 100644 index 000000000..c83c0bb81 --- /dev/null +++ b/src/services/mcp/config.persistence.test.ts @@ -0,0 +1,87 @@ +import { expect, test } from 'bun:test' +import { spawnSync } from 'node:child_process' +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { fileURLToPath } from 'node:url' + +const cwd = fileURLToPath(new URL('../../../', import.meta.url)) +const configImports = ` + import { enableConfigs, getGlobalConfig, getCurrentProjectConfig, saveGlobalConfig } from './src/utils/config.ts' + enableConfigs() +` +const mcpImports = ` + import { isMcpServerDisabled, setMcpServerEnabled } from './src/services/mcp/config.ts' +` + +test.each([ + { cached: ['a'], written: ['a', 'b'], name: 'a', enabled: true, + project: ['b'], global: ['a', 'b'] }, + { cached: ['a'], written: ['a', 'b'], name: 'c', enabled: false, + project: ['a', 'b', 'c'], global: ['a', 'b', 'c'] }, + // Disk already matches the toggle, but this process still sees the opposite. + { cached: ['a'], written: ['a', 'b'], name: 'b', enabled: false, + project: ['a', 'b'], global: ['a', 'b'] }, + { cached: ['a', 'b'], written: ['a'], name: 'b', enabled: true, + project: ['a'], global: ['a'] }, + // A peer already enabled the target by creating an explicit project list. + { cached: ['a', 'b'], written: ['a', 'b'], name: 'a', enabled: true, + peerEnable: true, project: ['b'], global: ['a', 'b'] }, + // Coherent no-ops must keep inheriting rather than pinning a project list. + { cached: ['a', 'b'], written: ['a', 'b'], name: 'b', enabled: false, + project: null, global: ['a', 'b'] }, + { cached: ['a'], written: ['a'], name: 'b', enabled: true, + project: null, global: ['a'] }, +])('first inherited toggle preserves immediate and restarted state: %j', scenario => { + const configDir = mkdtempSync(join(tmpdir(), 'mcp-persistence-')) + const env = { ...process.env, NODE_ENV: 'production', OPENCLAUDE_CONFIG_DIR: configDir } + const run = (script: string) => { + const result = spawnSync(process.execPath, ['--eval', script], { + cwd, env, encoding: 'utf8', timeout: 15_000, + }) + expect(result.error).toBeUndefined() + expect(result.status, result.stderr).toBe(0) + return JSON.parse(result.stdout) + } + try { + const writer = `${configImports}${mcpImports} + saveGlobalConfig(c => ({ ...c, disabledMcpServers: ${JSON.stringify(scenario.written)} })) + ${'peerEnable' in scenario ? "setMcpServerEnabled('a', true)" : ''} + ` + const toggled = run(`${configImports}${mcpImports} + import { spawnSync } from 'node:child_process' + // Start the production watcher before seeding the write-through cache. + getGlobalConfig() + saveGlobalConfig(c => ({ ...c, disabledMcpServers: ${JSON.stringify(scenario.cached)} })) + // Block the watcher while another process commits through the real lock. + const writer = spawnSync(process.execPath, ['--eval', ${JSON.stringify(writer)}], { + env: process.env, encoding: 'utf8', timeout: 15_000, + }) + if (writer.error || writer.status !== 0) throw new Error(writer.stderr || String(writer.error)) + const cached = getGlobalConfig().disabledMcpServers + setMcpServerEnabled(${JSON.stringify(scenario.name)}, ${scenario.enabled}) + console.log(JSON.stringify({ cached, + targetDisabled: isMcpServerDisabled(${JSON.stringify(scenario.name)}), + project: getCurrentProjectConfig().disabledMcpServers ?? null, + })) + `) + expect(toggled.cached).toEqual(scenario.cached) + expect(toggled.targetDisabled).toBe(!scenario.enabled) + expect(toggled.project).toEqual(scenario.project) + + const restarted = run(`${configImports}${mcpImports} + console.log(JSON.stringify({ + global: getGlobalConfig().disabledMcpServers, + project: getCurrentProjectConfig().disabledMcpServers ?? null, + siblingDisabled: isMcpServerDisabled(${JSON.stringify(scenario.name === 'a' ? 'b' : 'a')}), + targetDisabled: isMcpServerDisabled(${JSON.stringify(scenario.name)}), + })) + `) + expect(restarted).toEqual({ + global: scenario.global, project: scenario.project, + siblingDisabled: true, targetDisabled: !scenario.enabled, + }) + } finally { + rmSync(configDir, { recursive: true, force: true }) + } +}, 45_000) diff --git a/src/services/mcp/config.test.ts b/src/services/mcp/config.test.ts new file mode 100644 index 000000000..cb413d21e --- /dev/null +++ b/src/services/mcp/config.test.ts @@ -0,0 +1,118 @@ +import { feature } from 'bun:bundle' +import { afterEach, beforeEach, expect, test } from 'bun:test' +import { + acquireSharedMutationLock, + releaseSharedMutationLock, +} from '../../test/sharedMutationLock.js' +import { + getCurrentProjectConfig, + getGlobalConfig, + saveCurrentProjectConfig, + saveGlobalConfig, +} from '../../utils/config.js' +import { isMcpServerDisabled, setMcpServerEnabled } from './config.js' + +let savedNodeEnv: string | undefined +let savedGlobalDisabled: string[] | undefined +let savedProjectDisabled: string[] | undefined +let savedProjectEnabled: string[] | undefined + +beforeEach(async () => { + await acquireSharedMutationLock('services/mcp/config.test.ts') + savedNodeEnv = process.env.NODE_ENV + process.env.NODE_ENV = 'test' + savedGlobalDisabled = getGlobalConfig().disabledMcpServers + savedProjectDisabled = getCurrentProjectConfig().disabledMcpServers + savedProjectEnabled = getCurrentProjectConfig().enabledMcpServers + saveGlobalConfig(c => ({ ...c, disabledMcpServers: undefined })) + saveCurrentProjectConfig(c => ({ + ...c, + disabledMcpServers: undefined, + enabledMcpServers: undefined, + })) +}) + +afterEach(() => { + try { + saveGlobalConfig(c => ({ ...c, disabledMcpServers: savedGlobalDisabled })) + saveCurrentProjectConfig(c => ({ + ...c, + disabledMcpServers: savedProjectDisabled, + enabledMcpServers: savedProjectEnabled, + })) + if (savedNodeEnv === undefined) delete process.env.NODE_ENV + else process.env.NODE_ENV = savedNodeEnv + } finally { + releaseSharedMutationLock() + } +}) + +test('without project or global lists, servers are enabled', () => { + expect(isMcpServerDisabled('server')).toBe(false) + expect(isMcpServerDisabled('')).toBe(false) +}) + +test('an undefined project list inherits only global disables', () => { + getGlobalConfig().disabledMcpServers = ['a', 'b'] + expect(isMcpServerDisabled('a')).toBe(true) + expect(isMcpServerDisabled('b')).toBe(true) + expect(isMcpServerDisabled('other')).toBe(false) +}) + +test.each([{ list: [] }, { list: ['local'] }])('an explicit project list %j overrides global disables', ({ list }) => { + getGlobalConfig().disabledMcpServers = ['global'] + saveCurrentProjectConfig(c => ({ ...c, disabledMcpServers: [...list] })) + expect(isMcpServerDisabled('global')).toBe(false) + expect(isMcpServerDisabled('local')).toBe(list.length > 0) +}) + +test('disabling preserves inherited entries and propagates globally without duplicates', () => { + getGlobalConfig().disabledMcpServers = ['a'] + setMcpServerEnabled('b', false) + setMcpServerEnabled('b', false) + expect(isMcpServerDisabled('a')).toBe(true) + expect(isMcpServerDisabled('b')).toBe(true) + expect(getCurrentProjectConfig().disabledMcpServers).toEqual(['a', 'b']) + expect(getGlobalConfig().disabledMcpServers).toEqual(['a', 'b']) +}) + +test('toggling an explicit project list preserves its overrides and enables locally', () => { + getGlobalConfig().disabledMcpServers = ['global'] + saveCurrentProjectConfig(c => ({ ...c, disabledMcpServers: ['local'] })) + setMcpServerEnabled('target', false) + expect(isMcpServerDisabled('target')).toBe(true) + setMcpServerEnabled('target', true) + setMcpServerEnabled('target', true) + expect(isMcpServerDisabled('target')).toBe(false) + expect(isMcpServerDisabled('local')).toBe(true) + expect(isMcpServerDisabled('global')).toBe(false) + expect(getGlobalConfig().disabledMcpServers).toEqual(['global', 'target']) +}) + +test('enabling one inherited server preserves other disables and stays local', () => { + getGlobalConfig().disabledMcpServers = ['a', 'b'] + setMcpServerEnabled('a', true) + setMcpServerEnabled('a', true) + expect(isMcpServerDisabled('a')).toBe(false) + expect(isMcpServerDisabled('b')).toBe(true) + expect(getGlobalConfig().disabledMcpServers).toEqual(['a', 'b']) +}) + +test('enabling the last inherited server keeps an explicit empty override', () => { + getGlobalConfig().disabledMcpServers = ['a'] + setMcpServerEnabled('a', true) + expect(getCurrentProjectConfig().disabledMcpServers).toEqual([]) + expect(isMcpServerDisabled('a')).toBe(false) + expect(getGlobalConfig().disabledMcpServers).toEqual(['a']) +}) + +test.skipIf(feature('CHICAGO_MCP') ? false : true)('builtin toggles leave disabled lists unchanged', () => { + getGlobalConfig().disabledMcpServers = ['computer-use', 'other'] + expect(isMcpServerDisabled('computer-use')).toBe(true) + setMcpServerEnabled('computer-use', true) + expect(isMcpServerDisabled('computer-use')).toBe(false) + setMcpServerEnabled('computer-use', false) + expect(isMcpServerDisabled('computer-use')).toBe(true) + expect(getGlobalConfig().disabledMcpServers).toEqual(['computer-use', 'other']) + expect(getCurrentProjectConfig().disabledMcpServers).toBeUndefined() +}) diff --git a/src/services/mcp/config.ts b/src/services/mcp/config.ts index 50eb36477..c79ec9749 100644 --- a/src/services/mcp/config.ts +++ b/src/services/mcp/config.ts @@ -1709,8 +1709,14 @@ export function isMcpServerDisabled(name: string): boolean { const enabledServers = projectConfig.enabledMcpServers || [] return !enabledServers.includes(name) } - const disabledServers = projectConfig.disabledMcpServers || [] - return disabledServers.includes(name) + // Use project-level disabledMcpServers if explicitly set (even if empty), + // otherwise fall back to the global list so MCP state is consistent + // across all projects. + if (projectConfig.disabledMcpServers !== undefined) { + return projectConfig.disabledMcpServers.includes(name) + } + const globalConfig = getGlobalConfig() + return (globalConfig.disabledMcpServers || []).includes(name) } function toggleMembership( @@ -1732,7 +1738,7 @@ export function setMcpServerEnabled(name: string, enabled: boolean): void { const isBuiltinStateChange = isDefaultDisabledBuiltin(name) && isMcpServerDisabled(name) === enabled - saveCurrentProjectConfig(current => { + saveCurrentProjectConfig((current, globalConfig) => { if (isDefaultDisabledBuiltin(name)) { const prev = current.enabledMcpServers || [] const next = toggleMembership(prev, name, enabled) @@ -1740,12 +1746,32 @@ export function setMcpServerEnabled(name: string, enabled: boolean): void { return { ...current, enabledMcpServers: next } } - const prev = current.disabledMcpServers || [] + // Inherit from the same disk snapshot as the project, so a stale global + // cache cannot drop another process's disables from the first override. + const prev = + current.disabledMcpServers ?? globalConfig.disabledMcpServers ?? [] const next = toggleMembership(prev, name, !enabled) - if (next === prev) return current + // Skip only when disk and our cached predicate both match the toggle. + // A peer may have created a project override before our cache refreshed. + if (next === prev && isMcpServerDisabled(name) === !enabled) { + return current + } return { ...current, disabledMcpServers: next } }) + // Propagate only the *disable* direction to global so that disabling a + // server in one project takes effect everywhere by default, but enabling + // stays project-local (avoids silently force-enabling in unrelated projects). + // Builtins are exempt — they track state via enabledMcpServers. + if (!enabled && !isDefaultDisabledBuiltin(name)) { + saveGlobalConfig(current => { + const prev = current.disabledMcpServers || [] + const next = toggleMembership(prev, name, true) + if (next === prev) return current + return { ...current, disabledMcpServers: next } + }) + } + if (isBuiltinStateChange) { logEvent('tengu_builtin_mcp_toggle', { serverName: diff --git a/src/utils/config.ts b/src/utils/config.ts index 5d34cbbac..709e8851e 100644 --- a/src/utils/config.ts +++ b/src/utils/config.ts @@ -704,6 +704,11 @@ export type GlobalConfig = { // Use a different (e.g. cheaper/faster) model for compaction. // Defaults to mainLoopModel when unset. compactModel?: string + + // Global MCP server disabled list. When a project has no disabledMcpServers + // configured, this list is used as the fallback so MCP state is consistent + // across all projects. + disabledMcpServers?: string[] } /** @@ -817,6 +822,7 @@ export const GLOBAL_CONFIG_KEYS = [ 'logoColor', 'maxMessagesCompactionThreshold', 'compactModel', + 'disabledMcpServers', ] as const export type GlobalConfigKey = (typeof GLOBAL_CONFIG_KEYS)[number] @@ -2034,11 +2040,14 @@ export function getCurrentProjectConfig(): ProjectConfig { } export function saveCurrentProjectConfig( - updater: (currentConfig: ProjectConfig) => ProjectConfig, + updater: ( + currentConfig: ProjectConfig, + globalConfig: Readonly, + ) => ProjectConfig, ): void { if (process.env.NODE_ENV === 'test') { const current = getTestProjectConfigForTesting() - const config = updater(current) + const config = updater(current, getTestGlobalConfigForTesting()) // Skip if no changes (same reference returned) if (config === current) { return @@ -2056,7 +2065,7 @@ export function saveCurrentProjectConfig( current => { const currentProjectConfig = current.projects?.[absolutePath] ?? DEFAULT_PROJECT_CONFIG - const newProjectConfig = updater(currentProjectConfig) + const newProjectConfig = updater(currentProjectConfig, current) // Skip if no changes (same reference returned) if (newProjectConfig === currentProjectConfig) { return current @@ -2092,7 +2101,7 @@ export function saveCurrentProjectConfig( } const currentProjectConfig = config.projects?.[absolutePath] ?? DEFAULT_PROJECT_CONFIG - const newProjectConfig = updater(currentProjectConfig) + const newProjectConfig = updater(currentProjectConfig, config) // Skip if no changes (same reference returned) if (newProjectConfig === currentProjectConfig) { return