mirror of
https://github.com/Gitlawb/openclaude.git
synced 2026-11-06 20:58:14 -04:00
fix: fall back to global disabledMcpServers when project has none configured (#1935)
* 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 <the@jat.mn>
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
@@ -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()
|
||||
})
|
||||
@@ -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:
|
||||
|
||||
+13
-4
@@ -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<GlobalConfig>,
|
||||
) => 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
|
||||
|
||||
Reference in New Issue
Block a user