mirror of
https://github.com/NanmiCoder/claude-code-haha.git
synced 2026-10-10 11:53:10 +08:00
fix(mcp): keep project-scoped servers visible across restarts (#1126)
Project-scoped MCP servers written to a directory that never hosted a session disappeared from the desktop settings list after an app restart: the discovery set (cwd + recent projects + /api/mcp/project-paths) only enumerated registry entries with local-scope servers, and .mcp.json files leave no trace in the global config. - register the target project when addMcpConfig writes a project-scoped server, treat on-disk .mcp.json as the source of truth in project-paths, and self-heal registry entries for pre-existing files on first browse - stop removeMcpConfig from mutating the shared safeParseJSON cache entry: removals poisoned every later parse of byte-identical .mcp.json content, so a server moved between projects parsed as already deleted; add safeParseJSONWithoutCache for callers that edit the parsed value, switch the settings raw-update fallback to it, and make filterInvalidPermissionRules pure for the same reason - fetch the full known-project set when PluginDetail refreshes the MCP store instead of overwriting the list with a one-project view
This commit is contained in:
@@ -0,0 +1,83 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
vi.mock('../api/sessions', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('../api/sessions')>()
|
||||
return {
|
||||
...actual,
|
||||
sessionsApi: {
|
||||
...actual.sessionsApi,
|
||||
getRecentProjects: vi.fn(),
|
||||
},
|
||||
}
|
||||
})
|
||||
|
||||
vi.mock('../api/mcp', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('../api/mcp')>()
|
||||
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<ReturnType<typeof sessionsApi.getRecentProjects>>)
|
||||
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'])
|
||||
})
|
||||
})
|
||||
@@ -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<string | null>(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)
|
||||
|
||||
@@ -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<string | null>(null)
|
||||
const [pendingDeleteServer, setPendingDeleteServer] = useState<McpServerRecord | null>(null)
|
||||
const [isInitialLoading, setIsInitialLoading] = useState(true)
|
||||
const projectPathsForFetchRef = useRef<string[] | undefined>(undefined)
|
||||
const refreshInFlightRef = useRef(new Set<string>())
|
||||
|
||||
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<Record<McpGroupKey, McpServerRecord[]>> = {}
|
||||
@@ -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 ? (
|
||||
<EmptyState
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { create } from 'zustand'
|
||||
import { mcpApi } from '../api/mcp'
|
||||
import { sessionsApi } from '../api/sessions'
|
||||
import type { McpServerRecord, McpUpsertPayload } from '../types/mcp'
|
||||
|
||||
type McpStore = {
|
||||
@@ -8,6 +9,7 @@ type McpStore = {
|
||||
isLoading: boolean
|
||||
error: string | null
|
||||
fetchServers: (projectPaths?: string[], fallbackCwd?: string) => Promise<void>
|
||||
fetchServersForKnownProjects: (currentWorkDir?: string) => Promise<void>
|
||||
createServer: (name: string, payload: McpUpsertPayload, cwd?: string) => Promise<McpServerRecord>
|
||||
updateServer: (server: McpServerRecord, payload: McpUpsertPayload, cwd?: string) => Promise<McpServerRecord>
|
||||
deleteServer: (server: McpServerRecord, cwd?: string) => Promise<void>
|
||||
@@ -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<string[]> {
|
||||
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<McpServerRecord, 'scope'>) {
|
||||
return server.scope === 'local' || server.scope === 'project'
|
||||
}
|
||||
@@ -56,7 +78,7 @@ function replaceServer(
|
||||
|
||||
let fetchServersRequestId = 0
|
||||
|
||||
export const useMcpStore = create<McpStore>((set) => ({
|
||||
export const useMcpStore = create<McpStore>((set, get) => ({
|
||||
servers: [],
|
||||
selectedServer: null,
|
||||
isLoading: false,
|
||||
@@ -103,6 +125,14 @@ export const useMcpStore = create<McpStore>((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)
|
||||
|
||||
@@ -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')
|
||||
|
||||
+19
-3
@@ -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<Response> {
|
||||
// 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<Response> {
|
||||
})
|
||||
}
|
||||
|
||||
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) {
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<string, unknown> | 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<string, unknown> | null
|
||||
return parsed as Record<string, unknown>
|
||||
}
|
||||
|
||||
/**
|
||||
* 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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<string, unknown> }).mcpServers.shared
|
||||
const cachedAgain = safeParseJSON(content, false) as {
|
||||
mcpServers: Record<string, unknown>
|
||||
}
|
||||
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])
|
||||
})
|
||||
})
|
||||
@@ -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<string, unknown>
|
||||
expect(again.model).toBeUndefined()
|
||||
expect(again).toEqual({ hooks: 123, env: { FOO: '1' } })
|
||||
})
|
||||
})
|
||||
@@ -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<string, unknown> }).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])
|
||||
})
|
||||
})
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<string, unknown>
|
||||
if (!obj.permissions || typeof obj.permissions !== 'object') return []
|
||||
const perms = obj.permissions as Record<string, unknown>
|
||||
if (!obj.permissions || typeof obj.permissions !== 'object') {
|
||||
return { data, warnings: [] }
|
||||
}
|
||||
const perms = { ...(obj.permissions as Record<string, unknown>) }
|
||||
|
||||
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 }
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user