diff --git a/desktop/src/__tests__/mcpStoreKnownProjects.test.ts b/desktop/src/__tests__/mcpStoreKnownProjects.test.ts new file mode 100644 index 00000000..be2ed1d3 --- /dev/null +++ b/desktop/src/__tests__/mcpStoreKnownProjects.test.ts @@ -0,0 +1,83 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('../api/sessions', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + sessionsApi: { + ...actual.sessionsApi, + getRecentProjects: vi.fn(), + }, + } +}) + +vi.mock('../api/mcp', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + mcpApi: { + ...actual.mcpApi, + projectPaths: vi.fn(), + list: vi.fn(), + }, + } +}) + +import { mcpApi } from '../api/mcp' +import { sessionsApi } from '../api/sessions' +import { useMcpStore } from '../stores/mcpStore' +import type { McpServerRecord } from '../types/mcp' + +const record = (name: string, scope: McpServerRecord['scope']): McpServerRecord => ({ + name, + scope, + transport: 'stdio', + enabled: true, + status: 'checking', + statusLabel: 'Checking', + configLocation: '', + summary: 'echo hi', + canEdit: true, + canRemove: true, + canReconnect: true, + canToggle: true, + config: { type: 'stdio', command: 'echo', args: ['hi'], env: {} }, +}) + +describe('fetchServersForKnownProjects', () => { + beforeEach(() => { + useMcpStore.setState({ servers: [], selectedServer: null, isLoading: false, error: null }) + vi.mocked(sessionsApi.getRecentProjects).mockReset() + vi.mocked(mcpApi.projectPaths).mockReset() + vi.mocked(mcpApi.list).mockReset() + }) + + it('queries the union of current cwd, recent projects, and configured MCP paths', async () => { + vi.mocked(sessionsApi.getRecentProjects).mockResolvedValue({ + projects: [{ realPath: '/proj/recent' }], + } as Awaited>) + vi.mocked(mcpApi.projectPaths).mockResolvedValue({ projectPaths: ['/proj/with-mcp'] }) + vi.mocked(mcpApi.list).mockImplementation(async (cwd?: string) => ({ + servers: cwd === '/proj/with-mcp' ? [record('shared-tools', 'project')] : [], + })) + + await useMcpStore.getState().fetchServersForKnownProjects('/proj/current') + + const queried = vi.mocked(mcpApi.list).mock.calls.map(([cwd]) => cwd) + expect(queried).toEqual(['/proj/current', '/proj/recent', '/proj/with-mcp']) + expect(useMcpStore.getState().servers.map((s) => s.name)).toEqual(['shared-tools']) + }) + + it('does not collapse the list to a single-project view when discovery sources fail (GH #1126)', async () => { + // Both discovery calls fail — the refresh must still include the current + // cwd rather than silently fetching nothing. + vi.mocked(sessionsApi.getRecentProjects).mockRejectedValue(new Error('boom')) + vi.mocked(mcpApi.projectPaths).mockRejectedValue(new Error('boom')) + vi.mocked(mcpApi.list).mockResolvedValue({ servers: [record('only-local', 'local')] }) + + await useMcpStore.getState().fetchServersForKnownProjects('/proj/current') + + expect(vi.mocked(mcpApi.list).mock.calls.map(([cwd]) => cwd)).toEqual(['/proj/current']) + expect(useMcpStore.getState().servers.map((s) => s.name)).toEqual(['only-local']) + }) +}) diff --git a/desktop/src/components/plugins/PluginDetail.tsx b/desktop/src/components/plugins/PluginDetail.tsx index 9cd0e175..ab828749 100644 --- a/desktop/src/components/plugins/PluginDetail.tsx +++ b/desktop/src/components/plugins/PluginDetail.tsx @@ -37,7 +37,7 @@ export function PluginDetail() { const fetchSkillDetail = useSkillStore((s) => s.fetchSkillDetail) const fetchAgents = useAgentStore((s) => s.fetchAgents) const selectAgent = useAgentStore((s) => s.selectAgent) - const fetchServers = useMcpStore((s) => s.fetchServers) + const fetchServersForKnownProjects = useMcpStore((s) => s.fetchServersForKnownProjects) const selectServer = useMcpStore((s) => s.selectServer) const t = useTranslation() const [actionKey, setActionKey] = useState(null) @@ -170,7 +170,10 @@ export function PluginDetail() { return } openSettingsTab('mcp') - await fetchServers(undefined, currentWorkDir) + // Query the full known-project set. Fetching only currentWorkDir here + // used to overwrite the whole store with a one-project view, wiping other + // projects' servers from the settings list (GH #1126). + await fetchServersForKnownProjects(currentWorkDir) const state = useMcpStore.getState() const server = state.servers.find((entry) => entry.name === serverName) diff --git a/desktop/src/pages/McpSettings.tsx b/desktop/src/pages/McpSettings.tsx index 8416ad62..e1934252 100644 --- a/desktop/src/pages/McpSettings.tsx +++ b/desktop/src/pages/McpSettings.tsx @@ -15,8 +15,6 @@ import { useTranslation } from '../i18n' import { useUIStore } from '../stores/uiStore' import { useMcpStore } from '../stores/mcpStore' import { useSessionStore } from '../stores/sessionStore' -import { sessionsApi } from '../api/sessions' -import { mcpApi } from '../api/mcp' import type { McpServerRecord, McpUpsertPayload, McpWritableScope } from '../types/mcp' type EditorMode = @@ -462,7 +460,7 @@ function ServerRow({ } export function McpSettings() { - const { servers, selectedServer, isLoading, error, fetchServers, createServer, updateServer, deleteServer, toggleServer, reconnectServer, refreshServerStatus, selectServer } = useMcpStore() + const { servers, selectedServer, isLoading, error, fetchServersForKnownProjects, createServer, updateServer, deleteServer, toggleServer, reconnectServer, refreshServerStatus, selectServer } = useMcpStore() const addToast = useUIStore((s) => s.addToast) const sessions = useSessionStore((s) => s.sessions) const activeSessionId = useSessionStore((s) => s.activeSessionId) @@ -474,7 +472,6 @@ export function McpSettings() { const [busyServerKey, setBusyServerKey] = useState(null) const [pendingDeleteServer, setPendingDeleteServer] = useState(null) const [isInitialLoading, setIsInitialLoading] = useState(true) - const projectPathsForFetchRef = useRef(undefined) const refreshInFlightRef = useRef(new Set()) const activeSession = sessions.find((session) => session.id === activeSessionId) @@ -487,23 +484,7 @@ export function McpSettings() { const loadServers = async () => { try { - const [recentProjectPaths, privateMcpProjectPaths] = await Promise.all([ - sessionsApi.getRecentProjects(8) - .then(({ projects }) => projects.map((project) => project.realPath)) - .catch(() => []), - mcpApi.projectPaths() - .then(({ projectPaths }) => projectPaths) - .catch(() => []), - ]) - if (cancelled) return - const paths = [ - currentWorkDir, - ...recentProjectPaths, - ...privateMcpProjectPaths, - ].filter((path): path is string => !!path) - const projectPathsForFetch = Array.from(new Set(paths)) - projectPathsForFetchRef.current = projectPathsForFetch.length ? projectPathsForFetch : undefined - await fetchServers(projectPathsForFetchRef.current, currentWorkDir) + await fetchServersForKnownProjects(currentWorkDir) } finally { if (!cancelled) setIsInitialLoading(false) } @@ -514,7 +495,7 @@ export function McpSettings() { return () => { cancelled = true } - }, [fetchServers, currentWorkDir]) + }, [fetchServersForKnownProjects, currentWorkDir]) const groupedServers = useMemo(() => { const groups: Partial> = {} @@ -1117,7 +1098,7 @@ export function McpSettings() { size="lg" title={error} retryLabel={t('common.retry')} - onRetry={() => void fetchServers(projectPathsForFetchRef.current, currentWorkDir)} + onRetry={() => void fetchServersForKnownProjects(currentWorkDir)} /> ) : servers.length === 0 ? ( Promise + fetchServersForKnownProjects: (currentWorkDir?: string) => Promise createServer: (name: string, payload: McpUpsertPayload, cwd?: string) => Promise updateServer: (server: McpServerRecord, payload: McpUpsertPayload, cwd?: string) => Promise deleteServer: (server: McpServerRecord, cwd?: string) => Promise @@ -17,6 +19,26 @@ type McpStore = { selectServer: (server: McpServerRecord | null) => void } +/** + * The full set of project paths whose MCP servers should appear in the list: + * the active session's cwd, recent session projects, and every path the + * server knows to hold MCP config (GH #1126). Callers that refresh the whole + * store must query this set — fetching a single cwd would overwrite the list + * with a one-project view. + */ +async function collectKnownProjectPaths(currentWorkDir?: string): Promise { + const [recentProjectPaths, configuredMcpProjectPaths] = await Promise.all([ + sessionsApi.getRecentProjects(8) + .then(({ projects }) => projects.map((project) => project.realPath)) + .catch(() => []), + mcpApi.projectPaths() + .then(({ projectPaths }) => projectPaths) + .catch(() => []), + ]) + return [currentWorkDir, ...recentProjectPaths, ...configuredMcpProjectPaths] + .filter((path): path is string => !!path) +} + function isProjectScoped(server: Pick) { return server.scope === 'local' || server.scope === 'project' } @@ -56,7 +78,7 @@ function replaceServer( let fetchServersRequestId = 0 -export const useMcpStore = create((set) => ({ +export const useMcpStore = create((set, get) => ({ servers: [], selectedServer: null, isLoading: false, @@ -103,6 +125,14 @@ export const useMcpStore = create((set) => ({ } }, + fetchServersForKnownProjects: async (currentWorkDir) => { + const projectPaths = await collectKnownProjectPaths(currentWorkDir) + await get().fetchServers( + projectPaths.length ? projectPaths : undefined, + currentWorkDir, + ) + }, + createServer: async (name, payload, cwd) => { const response = await mcpApi.create(name, payload, cwd) const created = attachProjectPath(response.server, cwd) diff --git a/src/server/__tests__/mcp.test.ts b/src/server/__tests__/mcp.test.ts index 74555c86..ceba9657 100644 --- a/src/server/__tests__/mcp.test.ts +++ b/src/server/__tests__/mcp.test.ts @@ -282,6 +282,142 @@ describe('MCP API', () => { } }) + it('lists project paths whose .mcp.json declares project-scoped MCP servers (GH #1126)', async () => { + const previousNodeEnv = process.env.NODE_ENV + // A directory that never hosted a session — the shape of the bug report. + const freshProject = path.join(tmpDir, 'fresh-project') + await fs.mkdir(freshProject, { recursive: true }) + process.env.NODE_ENV = 'development' + clearConfigPathCaches() + + try { + const create = makeRequest('POST', '/api/mcp', { + cwd: freshProject, + name: 'shared-tools', + scope: 'project', + config: { + type: 'stdio', + command: 'npx', + args: ['shared-mcp'], + env: {}, + }, + }) + const createRes = await handleMcpApi(create.req, create.url, create.segments) + expect(createRes.status).toBe(201) + + // The request the settings page replays after an app restart to decide + // which project paths to query. The target directory must show up here, + // or the saved server silently disappears from the UI. + const projectPaths = makeRequest('GET', '/api/mcp/project-paths') + const projectPathsRes = await handleMcpApi(projectPaths.req, projectPaths.url, projectPaths.segments) + expect(projectPathsRes.status).toBe(200) + const body = await projectPathsRes.json() + + expect(body.projectPaths).toEqual([normalizePathForConfigKey(freshProject)]) + } finally { + if (previousNodeEnv === undefined) { + delete process.env.NODE_ENV + } else { + process.env.NODE_ENV = previousNodeEnv + } + clearConfigPathCaches() + } + }) + + it('self-heals the registry for pre-existing .mcp.json files on first browse (GH #1126)', async () => { + const previousNodeEnv = process.env.NODE_ENV + // Simulates a target project configured by a build without target + // registration (or a hand-written .mcp.json): file exists, no registry entry. + const legacyProject = path.join(tmpDir, 'legacy-project') + await fs.mkdir(legacyProject, { recursive: true }) + await fs.writeFile( + path.join(legacyProject, '.mcp.json'), + JSON.stringify({ mcpServers: { 'legacy-tools': { type: 'stdio', command: 'npx', args: ['x'] } } }), + ) + process.env.NODE_ENV = 'development' + clearConfigPathCaches() + + try { + const before = makeRequest('GET', '/api/mcp/project-paths') + const beforeRes = await handleMcpApi(before.req, before.url, before.segments) + expect((await beforeRes.json()).projectPaths).toEqual([]) + + // Browsing the project (the settings page's per-path list request) + // registers it, and it stays discoverable from then on. + const list = makeRequest('GET', `/api/mcp?cwd=${encodeURIComponent(legacyProject)}`) + const listRes = await handleMcpApi(list.req, list.url, list.segments) + expect(listRes.status).toBe(200) + + const after = makeRequest('GET', '/api/mcp/project-paths') + const afterRes = await handleMcpApi(after.req, after.url, after.segments) + expect((await afterRes.json()).projectPaths).toEqual([ + normalizePathForConfigKey(legacyProject), + ]) + } finally { + if (previousNodeEnv === undefined) { + delete process.env.NODE_ENV + } else { + process.env.NODE_ENV = previousNodeEnv + } + clearConfigPathCaches() + } + }) + + it('keeps a project server discoverable after moving it to another target project (GH #1126)', async () => { + const previousNodeEnv = process.env.NODE_ENV + const projectA = path.join(tmpDir, 'move-src') + const projectB = path.join(tmpDir, 'move-dst') + await fs.mkdir(projectA, { recursive: true }) + await fs.mkdir(projectB, { recursive: true }) + process.env.NODE_ENV = 'development' + clearConfigPathCaches() + + try { + const create = makeRequest('POST', '/api/mcp', { + cwd: projectA, + name: 'shared-tools', + scope: 'project', + config: { + type: 'stdio', + command: 'npx', + args: ['shared-mcp'], + env: {}, + }, + }) + const createRes = await handleMcpApi(create.req, create.url, create.segments) + expect(createRes.status).toBe(201) + + const update = makeRequest('PUT', '/api/mcp/shared-tools', { + cwd: projectB, + previousCwd: projectA, + scope: 'project', + config: { + type: 'stdio', + command: 'npx', + args: ['shared-mcp'], + env: {}, + }, + }) + const updateRes = await handleMcpApi(update.req, update.url, update.segments) + expect(updateRes.status).toBe(200) + + // The new target must be discoverable; the drained source (its + // .mcp.json is now empty) must not linger in the list. + const projectPaths = makeRequest('GET', '/api/mcp/project-paths') + const projectPathsRes = await handleMcpApi(projectPaths.req, projectPaths.url, projectPaths.segments) + const body = await projectPathsRes.json() + + expect(body.projectPaths).toEqual([normalizePathForConfigKey(projectB)]) + } finally { + if (previousNodeEnv === undefined) { + delete process.env.NODE_ENV + } else { + process.env.NODE_ENV = previousNodeEnv + } + clearConfigPathCaches() + } + }) + it('updates project MCP servers from their previous cwd into the selected target cwd', async () => { const projectA = path.join(tmpDir, 'project-a') const projectB = path.join(tmpDir, 'project-b') diff --git a/src/server/api/mcp.ts b/src/server/api/mcp.ts index b0ee8823..bd714ec2 100644 --- a/src/server/api/mcp.ts +++ b/src/server/api/mcp.ts @@ -14,6 +14,8 @@ import { getClaudeCodeMcpConfigs, getMcpConfigByName, isMcpServerDisabled, + projectDirDeclaresMcpServers, + registerCwdProjectIfDeclaresMcpServers, removeMcpConfig, setMcpServerEnabled, } from '../../services/mcp/config.js' @@ -445,6 +447,11 @@ function cleanupSecureStorage(name: string, config: ScopedMcpServerConfig) { } async function listServers(): Promise { + // Self-heal the project registry: .mcp.json files written before target + // registration existed (pre-GH#1126 desktop builds, hand-edited files) + // become rediscoverable the first time their project is browsed. + registerCwdProjectIfDeclaresMcpServers() + const { servers } = await getAllMcpConfigs() const visibleServers = Object.entries(servers) .filter(([name, config]) => isVisibleServer(name, config)) @@ -454,10 +461,19 @@ async function listServers(): Promise { }) } -function listProjectPathsWithPrivateMcp(): Response { +// Every project path the settings page must query to see all configured MCP +// servers: local-scope servers live in the registry entry itself +// (projects[path].mcpServers), project-scope servers live in the path's +// .mcp.json on disk. Missing the latter made shared servers vanish from the +// list after an app restart (GH #1126). +function listProjectPathsWithConfiguredMcp(): Response { const projects = getGlobalConfig().projects ?? {} const projectPaths = Object.entries(projects) - .filter(([, projectConfig]) => Object.keys(projectConfig.mcpServers ?? {}).length > 0) + .filter( + ([projectPath, projectConfig]) => + Object.keys(projectConfig.mcpServers ?? {}).length > 0 || + projectDirDeclaresMcpServers(projectPath), + ) .map(([projectPath]) => projectPath) .sort((a, b) => a.localeCompare(b)) @@ -668,7 +684,7 @@ export async function handleMcpApi( return await runWithCwdOverride(resolveRequestCwd(url, body), async () => { if (req.method === 'GET' && serverName === 'project-paths' && !action) { - return listProjectPathsWithPrivateMcp() + return listProjectPathsWithConfiguredMcp() } if (req.method === 'GET' && !serverName) { diff --git a/src/services/mcp/__tests__/projectMcpConfigWrites.test.ts b/src/services/mcp/__tests__/projectMcpConfigWrites.test.ts index 6647b120..3429bb4f 100644 --- a/src/services/mcp/__tests__/projectMcpConfigWrites.test.ts +++ b/src/services/mcp/__tests__/projectMcpConfigWrites.test.ts @@ -2,8 +2,17 @@ import { afterEach, beforeEach, describe, expect, it } from 'bun:test' import * as fs from 'fs/promises' import * as os from 'os' import * as path from 'path' +import { _setGlobalConfigCacheForTesting, enableConfigs, getProjectPathForConfig } from '../../../utils/config.js' import { runWithCwdOverride } from '../../../utils/cwd.js' -import { addMcpConfig, findProjectMcpConfigPath, removeMcpConfig } from '../config.js' +import { getGlobalClaudeFile } from '../../../utils/env.js' +import { normalizePathForConfigKey } from '../../../utils/path.js' +import { + addMcpConfig, + findProjectMcpConfigPath, + parseMcpConfigFromFilePath, + projectDirDeclaresMcpServers, + removeMcpConfig, +} from '../config.js' let tmpDir: string @@ -20,12 +29,32 @@ function readMcpJson(filePath: string) { const STDIO_SERVER = { type: 'stdio', command: 'npx', args: ['some-mcp'] } +let originalConfigDir: string | undefined + +// addMcpConfig('project') also registers the target directory in the global +// config (see GH #1126), so every test needs an isolated CLAUDE_CONFIG_DIR or +// that side effect would land in the developer's real ~/.claude.json. +function clearConfigPathCaches() { + getGlobalClaudeFile.cache.clear?.() + getProjectPathForConfig.cache.clear?.() + _setGlobalConfigCacheForTesting(null) +} + describe('project-scoped MCP config writes', () => { beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-project-removal-')) + originalConfigDir = process.env.CLAUDE_CONFIG_DIR + process.env.CLAUDE_CONFIG_DIR = tmpDir + clearConfigPathCaches() }) afterEach(async () => { + if (originalConfigDir !== undefined) { + process.env.CLAUDE_CONFIG_DIR = originalConfigDir + } else { + delete process.env.CLAUDE_CONFIG_DIR + } + clearConfigPathCaches() await fs.rm(tmpDir, { recursive: true, force: true }) }) @@ -199,5 +228,86 @@ describe('project-scoped MCP config writes', () => { runWithCwdOverride(proj, () => addMcpConfig('taken', STDIO_SERVER, 'project')), ).rejects.toThrow('MCP server taken already exists in .mcp.json') }) + + it('registers a fresh target directory in the global project registry (GH #1126)', async () => { + const previousNodeEnv = process.env.NODE_ENV + // development: getGlobalConfig() must read the sandboxed file, not the + // in-memory test stub, so the check and the write see the same state. + process.env.NODE_ENV = 'development' + clearConfigPathCaches() + enableConfigs() + + try { + const proj = path.join(tmpDir, 'never-seen-before') + await fs.mkdir(proj, { recursive: true }) + + await runWithCwdOverride(proj, () => addMcpConfig('shared', STDIO_SERVER, 'project')) + + const globalConfig = JSON.parse(await fs.readFile(getGlobalClaudeFile(), 'utf8')) + expect(Object.keys(globalConfig.projects ?? {})).toContain( + normalizePathForConfigKey(proj), + ) + } finally { + if (previousNodeEnv === undefined) { + delete process.env.NODE_ENV + } else { + process.env.NODE_ENV = previousNodeEnv + } + clearConfigPathCaches() + } + }) + }) + + describe('shared JSON parse cache isolation', () => { + it('removing a server never poisons parses of byte-identical .mcp.json files (GH #1126)', async () => { + const projA = path.join(tmpDir, 'poison-a') + const projB = path.join(tmpDir, 'poison-b') + const content = { mcpServers: { shared: STDIO_SERVER } } + await writeMcpJson(projA, content) + await writeMcpJson(projB, content) // byte-identical to A + + // Warm the shared safeParseJSON cache with A's content. The removal + // below used to receive this exact cached object and delete the server + // from it in place, so every byte-identical file then parsed as empty. + const warm = parseMcpConfigFromFilePath({ + filePath: path.join(projA, '.mcp.json'), + expandVars: false, + scope: 'project', + }) + expect(Object.keys(warm.config?.mcpServers ?? {})).toEqual(['shared']) + + await runWithCwdOverride(projA, () => removeMcpConfig('shared', 'project')) + + // B is untouched on disk and must still parse with its server present. + const afterRemove = parseMcpConfigFromFilePath({ + filePath: path.join(projB, '.mcp.json'), + expandVars: false, + scope: 'project', + }) + expect(Object.keys(afterRemove.config?.mcpServers ?? {})).toEqual(['shared']) + }) + }) + + describe('projectDirDeclaresMcpServers', () => { + it('is true only for a directory whose .mcp.json declares servers', async () => { + const withServers = path.join(tmpDir, 'with-servers') + const emptyServers = path.join(tmpDir, 'empty-servers') + const noFile = path.join(tmpDir, 'no-file') + await writeMcpJson(withServers, { mcpServers: { shared: STDIO_SERVER } }) + await writeMcpJson(emptyServers, { mcpServers: {} }) + await fs.mkdir(noFile, { recursive: true }) + + expect(projectDirDeclaresMcpServers(withServers)).toBe(true) + expect(projectDirDeclaresMcpServers(emptyServers)).toBe(false) + expect(projectDirDeclaresMcpServers(noFile)).toBe(false) + }) + + it('counts schema-invalid entries so the UI can still surface them', async () => { + const proj = path.join(tmpDir, 'invalid-entry') + // No command: fails validation, but the raw file still declares it. + await writeMcpJson(proj, { mcpServers: { broken: { type: 'stdio' } } }) + + expect(projectDirDeclaresMcpServers(proj)).toBe(true) + }) }) }) diff --git a/src/services/mcp/config.ts b/src/services/mcp/config.ts index 60eb228f..ebe83b65 100644 --- a/src/services/mcp/config.ts +++ b/src/services/mcp/config.ts @@ -10,6 +10,7 @@ import { isClaudeInChromeMCPServer } from '../../utils/claudeInChrome/common.js' import { getCurrentProjectConfig, getGlobalConfig, + getProjectPathForConfig, saveCurrentProjectConfig, saveGlobalConfig, } from '../../utils/config.js' @@ -17,7 +18,7 @@ import { getCwd } from '../../utils/cwd.js' import { logForDebugging } from '../../utils/debug.js' import { getErrnoCode } from '../../utils/errors.js' import { getFsImplementation } from '../../utils/fsOperations.js' -import { safeParseJSON } from '../../utils/json.js' +import { safeParseJSON, safeParseJSONWithoutCache } from '../../utils/json.js' import { logError } from '../../utils/log.js' import { getPluginMcpServers } from '../../utils/plugins/mcpPluginIntegration.js' import { loadAllPluginsCacheOnly } from '../../utils/plugins/pluginLoader.js' @@ -738,6 +739,13 @@ export async function addMcpConfig( } catch (error) { throw new Error(`Failed to write to .mcp.json: ${error}`) } + + // Register the target project in the global config's project registry. + // A directory that has never hosted a session has no `projects` entry, + // and the desktop settings page discovers project-scoped servers by + // scanning registry keys — without this, a .mcp.json written to a fresh + // directory becomes invisible after an app restart (GH #1126). + registerCwdProjectIfDeclaresMcpServers() break } @@ -966,7 +974,11 @@ function readRawMcpJsonFile(mcpJsonPath: string): Record | null throw error } - const parsed = safeParseJSON(contents) + // Callers edit the returned object in place before writing it back, so it + // must be a fresh parse. Handing out the shared safeParseJSON cache entry + // here let a server removal mutate the cached value, and every .mcp.json + // with byte-identical content then parsed as already-deleted (GH #1126). + const parsed = safeParseJSONWithoutCache(contents) if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { return null } @@ -974,6 +986,35 @@ function readRawMcpJsonFile(mcpJsonPath: string): Record | null return parsed as Record } +/** + * True when the directory's own .mcp.json declares at least one MCP server. + * Reads the raw file so entries that fail schema validation still count — the + * desktop settings page must be able to rediscover them for the user to fix. + */ +export function projectDirDeclaresMcpServers(dir: string): boolean { + const mcpServers = readRawMcpJsonFile(join(dir, '.mcp.json'))?.mcpServers + return ( + !!mcpServers && + typeof mcpServers === 'object' && + !Array.isArray(mcpServers) && + Object.keys(mcpServers).length > 0 + ) +} + +/** + * Ensure the cwd's project has a global-config registry entry when its + * .mcp.json declares servers. The registry is how the desktop settings page + * discovers which paths to query, so writes register their target eagerly and + * reads self-heal entries for .mcp.json files created before registration + * existed (or by other tools entirely). Idempotent and cheap when already + * registered. (GH #1126) + */ +export function registerCwdProjectIfDeclaresMcpServers(): void { + if (getGlobalConfig().projects?.[getProjectPathForConfig(getCwd())]) return + if (!projectDirDeclaresMcpServers(getCwd())) return + saveMcpProjectConfig(current => ({ ...current })) +} + /** * Open the cwd's .mcp.json for editing, as a raw object plus a mutable handle * on its mcpServers map. Missing or unusable files yield empty structures so diff --git a/src/utils/json.ts b/src/utils/json.ts index 2cfc67b7..73e857d2 100644 --- a/src/utils/json.ts +++ b/src/utils/json.ts @@ -42,6 +42,11 @@ function parseJSONUncached(json: string, shouldLogError: boolean): CachedParse { const parseJSONCached = memoizeWithLRU(parseJSONUncached, json => json, 50) // Important: memoized for performance (LRU-bounded to 50 entries, small inputs only). +// The cache hands the SAME object to every caller with an equal input string — +// treat the result as immutable. A caller that needs to edit the parsed value +// must use safeParseJSONWithoutCache, or its mutations poison the cache for +// every later parse of byte-identical content (GH #1126: removing an MCP +// server made same-content .mcp.json files in other projects parse as empty). export const safeParseJSON = Object.assign( function safeParseJSON( json: string | null | undefined, @@ -57,6 +62,19 @@ export const safeParseJSON = Object.assign( { cache: parseJSONCached.cache }, ) +/** + * Like safeParseJSON, but always returns a fresh object the caller owns and + * may freely mutate. Never reads from or writes to the shared parse cache. + */ +export function safeParseJSONWithoutCache( + json: string | null | undefined, + shouldLogError: boolean = true, +): unknown { + if (!json) return null + const result = parseJSONUncached(json, shouldLogError) + return result.ok ? result.value : null +} + /** * Safely parse JSON with comments (jsonc). * This is useful for VS Code configuration files like keybindings.json diff --git a/src/utils/safeParseJSONWithoutCache.test.ts b/src/utils/safeParseJSONWithoutCache.test.ts new file mode 100644 index 00000000..e3292515 --- /dev/null +++ b/src/utils/safeParseJSONWithoutCache.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from 'bun:test' +import { safeParseJSON, safeParseJSONWithoutCache } from './json.js' + +describe('safeParseJSONWithoutCache', () => { + it('parses valid JSON and returns null for invalid input', () => { + expect(safeParseJSONWithoutCache('{"a":1}')).toEqual({ a: 1 }) + expect(safeParseJSONWithoutCache('not json', false)).toBeNull() + expect(safeParseJSONWithoutCache(null)).toBeNull() + expect(safeParseJSONWithoutCache('')).toBeNull() + }) + + it('returns a fresh object per call, never the shared cache entry', () => { + const content = '{"mcpServers":{"shared":{"command":"npx"}}}' + + const cached = safeParseJSON(content, false) + const fresh = safeParseJSONWithoutCache(content, false) + expect(fresh).not.toBe(cached) + + // Mutating the fresh copy must not leak into later cached parses — + // this exact leak made removed MCP servers reappear as deleted in every + // byte-identical .mcp.json (GH #1126). + delete (fresh as { mcpServers: Record }).mcpServers.shared + const cachedAgain = safeParseJSON(content, false) as { + mcpServers: Record + } + expect(Object.keys(cachedAgain.mcpServers)).toEqual(['shared']) + }) + + it('two uncached calls never share structure', () => { + const content = '{"nested":{"list":[1,2]}}' + const first = safeParseJSONWithoutCache(content) as { nested: { list: number[] } } + const second = safeParseJSONWithoutCache(content) as { nested: { list: number[] } } + first.nested.list.push(3) + expect(second.nested.list).toEqual([1, 2]) + }) +}) diff --git a/src/utils/settings/__tests__/updateSettingsRawFallback.test.ts b/src/utils/settings/__tests__/updateSettingsRawFallback.test.ts new file mode 100644 index 00000000..b9acf187 --- /dev/null +++ b/src/utils/settings/__tests__/updateSettingsRawFallback.test.ts @@ -0,0 +1,52 @@ +import { afterEach, beforeEach, describe, expect, it } from 'bun:test' +import * as fs from 'fs/promises' +import * as os from 'os' +import * as path from 'path' +import { safeParseJSON } from '../../json.js' +import { updateSettingsForSource } from '../settings.js' + +// updateSettingsForSource falls back to merging on top of the raw parsed file +// when its content is valid JSON but fails schema validation. That merge +// mutates its target in place, so the raw parse must never come from the +// shared safeParseJSON cache (same bug family as GH #1126). +describe('updateSettingsForSource raw fallback', () => { + let tmpDir: string + + beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'settings-raw-fallback-')) + }) + + afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }) + }) + + it('never poisons parses of byte-identical settings content (GH #1126 family)', async () => { + const projectRoot = path.join(tmpDir, 'project') + const settingsDir = path.join(projectRoot, '.claude') + await fs.mkdir(settingsDir, { recursive: true }) + + // Valid JSON, fails schema validation (hooks must be an object) — this is + // exactly what routes the update through the raw-parse fallback. + const content = JSON.stringify({ hooks: 123, env: { FOO: '1' } }) + const settingsPath = path.join(settingsDir, 'settings.json') + await fs.writeFile(settingsPath, content) + + // Warm the shared parse cache with this exact string, as any earlier + // settings read would. + safeParseJSON(content, false) + + const { error } = updateSettingsForSource('projectSettings', { model: 'haiku' }, projectRoot) + expect(error).toBeNull() + + // The merge must have landed on disk… + const onDisk = JSON.parse(await fs.readFile(settingsPath, 'utf8')) + expect(onDisk.model).toBe('haiku') + expect(onDisk.env).toEqual({ FOO: '1' }) + + // …while a byte-identical file elsewhere still parses to its own content, + // without the merged-in fields bleeding through the cache. + const again = safeParseJSON(content, false) as Record + expect(again.model).toBeUndefined() + expect(again).toEqual({ hooks: 123, env: { FOO: '1' } }) + }) +}) diff --git a/src/utils/settings/__tests__/validation.test.ts b/src/utils/settings/__tests__/validation.test.ts new file mode 100644 index 00000000..f4a46b60 --- /dev/null +++ b/src/utils/settings/__tests__/validation.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from 'bun:test' +import { safeParseJSON } from '../../json.js' +import { filterInvalidPermissionRules } from '../validation.js' + +const VALID_RULE = 'Bash(ls:*)' +const INVALID_RULE = 'Bash(ls:*' // unbalanced paren fails rule validation + +describe('filterInvalidPermissionRules', () => { + it('drops invalid rules and reports one warning per drop', () => { + const input = { + permissions: { + allow: [VALID_RULE, INVALID_RULE, 42], + deny: [VALID_RULE], + }, + } + + const { data, warnings } = filterInvalidPermissionRules(input, '/tmp/settings.json') + const perms = (data as { permissions: Record }).permissions + + expect(perms.allow).toEqual([VALID_RULE]) + expect(perms.deny).toEqual([VALID_RULE]) + expect(warnings).toHaveLength(2) + expect(warnings.every(w => w.file === '/tmp/settings.json')).toBe(true) + }) + + it('passes through data without a permissions object untouched', () => { + const input = { model: 'sonnet' } + const { data, warnings } = filterInvalidPermissionRules(input, '/tmp/settings.json') + expect(data).toBe(input) + expect(warnings).toEqual([]) + }) + + it('never mutates its input (GH #1126 cache-poisoning family)', () => { + const input = { + permissions: { allow: [VALID_RULE, INVALID_RULE] }, + } + + filterInvalidPermissionRules(input, '/tmp/settings.json') + + // The caller's object — potentially a shared safeParseJSON cache entry — + // must keep the invalid rule; only the returned copy is filtered. + expect(input.permissions.allow).toEqual([VALID_RULE, INVALID_RULE]) + }) + + it('keeps byte-identical settings content parsing identically after a filter pass', () => { + // End-to-end shape of the original bug: parse (shared cache) → filter → + // a later parse of the same string must still see the invalid rule. + const content = JSON.stringify({ + permissions: { allow: [VALID_RULE, INVALID_RULE] }, + }) + + const first = safeParseJSON(content, false) + filterInvalidPermissionRules(first, '/tmp/a/settings.json') + + const second = safeParseJSON(content, false) as { + permissions: { allow: unknown[] } + } + expect(second.permissions.allow).toEqual([VALID_RULE, INVALID_RULE]) + }) +}) diff --git a/src/utils/settings/mdm/settings.ts b/src/utils/settings/mdm/settings.ts index 61a8785d..447a3003 100644 --- a/src/utils/settings/mdm/settings.ts +++ b/src/utils/settings/mdm/settings.ts @@ -184,12 +184,12 @@ export function parseCommandOutputAsSettings( stdout: string, sourcePath: string, ): { settings: SettingsJson; errors: ValidationError[] } { - const data = safeParseJSON(stdout, false) - if (!data || typeof data !== 'object') { + const rawData = safeParseJSON(stdout, false) + if (!rawData || typeof rawData !== 'object') { return { settings: {}, errors: [] } } - const ruleWarnings = filterInvalidPermissionRules(data, sourcePath) + const { data, warnings: ruleWarnings } = filterInvalidPermissionRules(rawData, sourcePath) const parseResult = SettingsSchema().safeParse(data) if (!parseResult.success) { const errors = formatZodError(parseResult.error, sourcePath) diff --git a/src/utils/settings/settings.ts b/src/utils/settings/settings.ts index 6033dc7a..e6fcbefb 100644 --- a/src/utils/settings/settings.ts +++ b/src/utils/settings/settings.ts @@ -18,7 +18,7 @@ import { writeFileSyncAndFlush_DEPRECATED } from '../file.js' import { readFileSync } from '../fileRead.js' import { getFsImplementation, safeResolvePath } from '../fsOperations.js' import { addFileGlobRuleToGitignore } from '../git/gitignore.js' -import { safeParseJSON } from '../json.js' +import { safeParseJSON, safeParseJSONWithoutCache } from '../json.js' import { logError } from '../log.js' import { getPlatform } from '../platform.js' import { clone, jsonStringify } from '../slowOperations.js' @@ -210,11 +210,11 @@ function parseSettingsFileUncached(path: string): { return { settings: {}, errors: [] } } - const data = safeParseJSON(content, false) + const rawData = safeParseJSON(content, false) // Filter invalid permission rules before schema validation so one bad // rule doesn't cause the entire settings file to be rejected. - const ruleWarnings = filterInvalidPermissionRules(data, path) + const { data, warnings: ruleWarnings } = filterInvalidPermissionRules(rawData, path) const result = SettingsSchema().safeParse(data) @@ -467,7 +467,11 @@ export function updateSettingsForSource( // File doesn't exist — fall through to merge with empty settings } if (content !== null) { - const rawData = safeParseJSON(content) + // Must be an uncached parse: the mergeWith below mutates this object + // (including nested refs), and editing a shared safeParseJSON cache + // entry would corrupt every later parse of byte-identical settings + // content (same bug family as GH #1126). + const rawData = safeParseJSONWithoutCache(content) if (rawData === null) { // JSON syntax error - return validation error instead of overwriting // safeParseJSON will already log the error, so we'll just return the error here diff --git a/src/utils/settings/validation.ts b/src/utils/settings/validation.ts index fc4744c1..eb589b42 100644 --- a/src/utils/settings/validation.ts +++ b/src/utils/settings/validation.ts @@ -219,16 +219,22 @@ export function validateSettingsFileContent(content: string): /** * Filters invalid permission rules from raw parsed JSON data before schema validation. * This prevents one bad rule from poisoning the entire settings file. - * Returns warnings for each filtered rule. + * Returns the filtered data plus warnings for each filtered rule. + * + * Pure: never modifies the input. Callers hand us safeParseJSON output, which + * may be a shared cache entry — editing it in place poisoned every later + * parse of byte-identical settings content (same bug family as GH #1126). */ export function filterInvalidPermissionRules( data: unknown, filePath: string, -): ValidationError[] { - if (!data || typeof data !== 'object') return [] +): { data: unknown; warnings: ValidationError[] } { + if (!data || typeof data !== 'object') return { data, warnings: [] } const obj = data as Record - if (!obj.permissions || typeof obj.permissions !== 'object') return [] - const perms = obj.permissions as Record + if (!obj.permissions || typeof obj.permissions !== 'object') { + return { data, warnings: [] } + } + const perms = { ...(obj.permissions as Record) } const warnings: ValidationError[] = [] for (const key of ['allow', 'deny', 'ask']) { @@ -261,5 +267,5 @@ export function filterInvalidPermissionRules( return true }) } - return warnings + return { data: { ...obj, permissions: perms }, warnings } }