From 82a47d051a5b761739334155e21f25a6f322931b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A8=8B=E5=BA=8F=E5=91=98=E9=98=BF=E6=B1=9F=28Relakkes?= =?UTF-8?q?=29?= Date: Mon, 25 May 2026 17:34:25 +0800 Subject: [PATCH] fix: require explicit MCP project targets (#585) Desktop MCP local and project scopes were still anchored to the active session workDir when adding or editing servers, which made a selected session worktree look like the target project. The form now treats the target project as explicit input for local/project scopes, and the server update path can remove the old scoped config before writing to the newly selected directory. Constraint: Local MCP scope is private user config keyed by project path; project scope writes the selected project's .mcp.json Rejected: Keep using the active session workDir as the implicit target | it can point at transient session worktrees and hides where config is written Confidence: high Scope-risk: moderate Directive: Do not reintroduce implicit active-session cwd writes for MCP create/edit; local and project scopes need an explicit target project Tested: cd desktop && bun run test -- mcpSettings.test.tsx Tested: bun test src/server/__tests__/mcp.test.ts Tested: cd desktop && bun run check:desktop Tested: bun run check:server Tested: bun run quality:gate --mode baseline --allow-live --only 'provider-smoke:*' --provider-model deepseek:main:deepseek-main Tested: Live filesystem MCP probes with DeepSeek deepseek-v4-pro across local/project scopes; see artifacts/quality-runs/mcp-live-split-2026-05-25T09-15-18-245Z/summary.json Not-tested: Full bun run verify was not rerun after the live MCP smoke Related: #585 --- desktop/src/__tests__/mcpSettings.test.tsx | 127 ++++++++++++++++++++- desktop/src/api/mcp.ts | 3 +- desktop/src/pages/McpSettings.tsx | 53 +++++++-- desktop/src/stores/mcpStore.ts | 3 +- src/server/__tests__/mcp.test.ts | 45 ++++++++ src/server/api/mcp.ts | 12 +- 6 files changed, 228 insertions(+), 15 deletions(-) diff --git a/desktop/src/__tests__/mcpSettings.test.tsx b/desktop/src/__tests__/mcpSettings.test.tsx index 32ee2f6b..5913c738 100644 --- a/desktop/src/__tests__/mcpSettings.test.tsx +++ b/desktop/src/__tests__/mcpSettings.test.tsx @@ -3,12 +3,36 @@ import { act, fireEvent, render, screen, waitFor } from '@testing-library/react' import '@testing-library/jest-dom' import { McpSettings } from '../pages/McpSettings' +import { sessionsApi } from '../api/sessions' import { useMcpStore } from '../stores/mcpStore' import { useSessionStore } from '../stores/sessionStore' import { useSettingsStore } from '../stores/settingsStore' +vi.mock('../api/sessions', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + sessionsApi: { + ...actual.sessionsApi, + getRecentProjects: vi.fn(), + }, + } +}) + describe('McpSettings', () => { beforeEach(() => { + vi.mocked(sessionsApi.getRecentProjects).mockResolvedValue({ + projects: [{ + projectPath: '/workspace/selected-project', + realPath: '/workspace/selected-project', + projectName: 'selected-project', + repoName: 'org/selected-project', + branch: 'main', + isGit: true, + modifiedAt: '2026-05-25T00:00:00.000Z', + sessionCount: 1, + }], + }) useSettingsStore.setState({ locale: 'en' }) useSessionStore.setState({ sessions: [ @@ -222,7 +246,7 @@ describe('McpSettings', () => { expect(toggleServer).toHaveBeenCalledWith(server, '/workspace/project', 'session-1') }) - it('creates local MCP servers by default to match the CLI', async () => { + it('requires an explicitly selected project before creating local MCP servers', async () => { const createdServer = { name: 'context7', scope: 'local', @@ -249,12 +273,25 @@ describe('McpSettings', () => { fireEvent.click(screen.getByRole('button', { name: /add server/i })) }) + expect(screen.getByText('Select a project...')).toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled() + fireEvent.change(screen.getByLabelText(/Name/), { target: { value: 'context7' } }) fireEvent.change(screen.getByLabelText(/Command to launch/), { target: { value: 'npx' } }) fireEvent.change(screen.getByPlaceholderText('chrome-devtools-mcp@latest'), { target: { value: '@upstash/context7-mcp' }, }) + expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled() + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: /Select a project/i })) + }) + + await act(async () => { + fireEvent.click(await screen.findByText('org/selected-project')) + }) + await act(async () => { fireEvent.click(screen.getByRole('button', { name: 'Save' })) }) @@ -270,7 +307,93 @@ describe('McpSettings', () => { env: {}, }, }, - '/workspace/project', + '/workspace/selected-project', + ) + }) + + it('updates project MCP servers using the explicitly selected target project', async () => { + vi.mocked(sessionsApi.getRecentProjects).mockResolvedValue({ + projects: [{ + projectPath: '/workspace/moved-project', + realPath: '/workspace/moved-project', + projectName: 'moved-project', + repoName: 'org/moved-project', + branch: 'main', + isGit: true, + modifiedAt: '2026-05-25T00:00:00.000Z', + sessionCount: 1, + }], + }) + const updateServer = vi.fn().mockResolvedValue({ + name: 'shared-tools', + scope: 'project', + transport: 'stdio', + enabled: true, + status: 'checking' as const, + statusLabel: 'Checking', + configLocation: '/workspace/moved-project/.mcp.json', + summary: 'npx shared-tools', + canEdit: true, + canRemove: true, + canReconnect: true, + canToggle: true, + projectPath: '/workspace/moved-project', + config: { type: 'stdio' as const, command: 'npx', args: ['shared-tools'], env: {} }, + }) + const server = { + name: 'shared-tools', + scope: 'project', + transport: 'stdio', + enabled: true, + status: 'connected', + statusLabel: 'Connected', + configLocation: '/workspace/project/.mcp.json', + summary: 'npx shared-tools', + canEdit: true, + canRemove: true, + canReconnect: true, + canToggle: true, + projectPath: '/workspace/project', + config: { type: 'stdio' as const, command: 'npx', args: ['shared-tools'], env: {} }, + } as const + + useMcpStore.setState({ + servers: [server], + updateServer, + }) + + render() + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Open shared-tools' })) + }) + + expect(screen.getByText('project')).toBeInTheDocument() + + await act(async () => { + fireEvent.click(screen.getByTitle('/workspace/project')) + }) + + await act(async () => { + fireEvent.click(await screen.findByText('org/moved-project')) + }) + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Save' })) + }) + + expect(updateServer).toHaveBeenCalledWith( + server, + { + scope: 'project', + config: { + type: 'stdio', + command: 'npx', + args: ['shared-tools'], + env: {}, + }, + }, + '/workspace/moved-project', ) }) diff --git a/desktop/src/api/mcp.ts b/desktop/src/api/mcp.ts index 973d23ef..f8057ead 100644 --- a/desktop/src/api/mcp.ts +++ b/desktop/src/api/mcp.ts @@ -20,10 +20,11 @@ export const mcpApi = { }) }, - update: (name: string, payload: McpUpsertPayload, cwd?: string) => { + update: (name: string, payload: McpUpsertPayload, cwd?: string, previousCwd?: string) => { return api.put<{ server: McpServerRecord }>(`/api/mcp/${encodeURIComponent(name)}`, { ...payload, ...(cwd ? { cwd } : {}), + ...(previousCwd ? { previousCwd } : {}), }) }, diff --git a/desktop/src/pages/McpSettings.tsx b/desktop/src/pages/McpSettings.tsx index e1de8656..9e432531 100644 --- a/desktop/src/pages/McpSettings.tsx +++ b/desktop/src/pages/McpSettings.tsx @@ -1,5 +1,6 @@ import { useEffect, useMemo, useRef, useState } from 'react' import { Button } from '../components/shared/Button' +import { DirectoryPicker } from '../components/shared/DirectoryPicker' import { Input } from '../components/shared/Input' import { Modal } from '../components/shared/Modal' import { useTranslation } from '../i18n' @@ -30,6 +31,7 @@ type KeyValueRow = { type McpDraft = { name: string scope: McpWritableScope + projectPath: string transport: TransportKind command: string args: StringRow[] @@ -89,6 +91,7 @@ function createEmptyDraft(): McpDraft { return { name: '', scope: 'local', + projectPath: '', transport: 'stdio', command: '', args: [createStringRow('')], @@ -105,6 +108,10 @@ function asWritableScope(scope: string): McpWritableScope { return scope === 'project' || scope === 'user' ? scope : 'local' } +function scopeRequiresProject(scope: McpWritableScope) { + return scope === 'local' || scope === 'project' +} + function isStdioConfig(config: McpServerRecord['config']): config is Extract { return config.type === 'stdio' } @@ -117,6 +124,7 @@ function draftFromServer(server: McpServerRecord): McpDraft { const base = createEmptyDraft() base.name = server.name base.scope = asWritableScope(server.scope) + base.projectPath = scopeRequiresProject(base.scope) ? server.projectPath ?? '' : '' if (isStdioConfig(server.config)) { return { @@ -199,6 +207,7 @@ function buildPayload(draft: McpDraft): McpUpsertPayload { function isDraftValid(draft: McpDraft) { if (!draft.name.trim()) return false + if (scopeRequiresProject(draft.scope) && !draft.projectPath.trim()) return false if (draft.transport === 'stdio') return draft.command.trim().length > 0 return draft.url.trim().length > 0 } @@ -626,9 +635,10 @@ export function McpSettings() { setIsSaving(true) try { const payload = buildPayload(draft) + const operationCwd = scopeRequiresProject(draft.scope) ? draft.projectPath.trim() : undefined const saved = view.type === 'edit' - ? await updateServer(view.server, payload, resolveOperationCwd(view.server)) - : await createServer(draft.name.trim(), payload, currentWorkDir) + ? await updateServer(view.server, payload, operationCwd) + : await createServer(draft.name.trim(), payload, operationCwd) addToast({ type: 'success', @@ -741,6 +751,21 @@ export function McpSettings() { const targetServer = editing ? view.server : null const transportLocked = editing const isBusy = isSaving || isDeleting + const targetProjectPath = draft.projectPath.trim() + const needsProjectTarget = scopeRequiresProject(draft.scope) + const targetProjectHint = draft.scope === 'local' + ? (targetProjectPath + ? t('settings.mcp.targetProject.localSelected', { path: targetProjectPath }) + : currentWorkDir + ? t('settings.mcp.targetProject.emptyWithCurrent', { path: currentWorkDir }) + : t('settings.mcp.targetProject.localEmpty')) + : draft.scope === 'project' + ? (targetProjectPath + ? t('settings.mcp.targetProject.projectSelected', { path: targetProjectPath }) + : currentWorkDir + ? t('settings.mcp.targetProject.emptyWithCurrent', { path: currentWorkDir }) + : t('settings.mcp.targetProject.projectEmpty')) + : t('settings.mcp.targetProject.globalHint') return ( <> @@ -831,11 +856,25 @@ export function McpSettings() { ) })} - {currentWorkDir && (draft.scope === 'local' || draft.scope === 'project') && ( -

- {t('settings.mcp.currentProjectHint', { path: currentWorkDir })} -

- )} + + +
+
+
+
+ {needsProjectTarget ? t('settings.mcp.targetProject.title') : t('settings.mcp.targetProject.globalTitle')} +
+

+ {targetProjectHint} +

+
+ {needsProjectTarget && ( + setDraftField('projectPath', path)} + /> + )} +
diff --git a/desktop/src/stores/mcpStore.ts b/desktop/src/stores/mcpStore.ts index 0a61b022..793d53ac 100644 --- a/desktop/src/stores/mcpStore.ts +++ b/desktop/src/stores/mcpStore.ts @@ -110,7 +110,8 @@ export const useMcpStore = create((set) => ({ }, updateServer: async (server, payload, cwd) => { - const response = await mcpApi.update(server.name, payload, cwd) + const previousCwd = isProjectScoped(server) ? server.projectPath : undefined + const response = await mcpApi.update(server.name, payload, cwd, previousCwd) const updated = attachProjectPath(response.server, cwd ?? server.projectPath) set((state) => ({ servers: replaceServer(state.servers, server, updated, cwd ?? server.projectPath), diff --git a/src/server/__tests__/mcp.test.ts b/src/server/__tests__/mcp.test.ts index 77ce42c7..4cbf84ef 100644 --- a/src/server/__tests__/mcp.test.ts +++ b/src/server/__tests__/mcp.test.ts @@ -197,6 +197,51 @@ describe('MCP API', () => { expect(connectSpy).not.toHaveBeenCalled() }) + 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') + await fs.mkdir(projectA, { recursive: true }) + await fs.mkdir(projectB, { recursive: true }) + + const create = makeRequest('POST', '/api/mcp', { + cwd: projectA, + name: 'shared-tools', + scope: 'project', + config: { + type: 'stdio', + command: 'npx', + args: ['old-tools'], + 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: ['new-tools'], + env: {}, + }, + }) + const updateRes = await handleMcpApi(update.req, update.url, update.segments) + expect(updateRes.status).toBe(200) + + const projectAConfig = JSON.parse(await fs.readFile(path.join(projectA, '.mcp.json'), 'utf8')) + const projectBConfig = JSON.parse(await fs.readFile(path.join(projectB, '.mcp.json'), 'utf8')) + + expect(projectAConfig.mcpServers?.['shared-tools']).toBeUndefined() + expect(projectBConfig.mcpServers?.['shared-tools']).toMatchObject({ + type: 'stdio', + command: 'npx', + args: ['new-tools'], + }) + }) + it('checks a single server status on demand', async () => { const create = makeRequest('POST', '/api/mcp', { cwd: projectRoot, diff --git a/src/server/api/mcp.ts b/src/server/api/mcp.ts index f2555464..93fe44a7 100644 --- a/src/server/api/mcp.ts +++ b/src/server/api/mcp.ts @@ -71,6 +71,7 @@ type McpServerDto = { type McpMutationBody = { cwd?: string + previousCwd?: string scope?: string sessionId?: string config?: unknown @@ -481,7 +482,10 @@ async function createServer(body: Record): Promise { } async function updateServer(name: string, body: Record): Promise { - const existing = getMcpConfigByName(name) + const targetCwd = getCwd() + const previousCwd = optionalString(body.previousCwd) + const previousLookupCwd = previousCwd ?? targetCwd + const existing = runWithCwdOverride(previousLookupCwd, () => getMcpConfigByName(name)) if (!existing) { throw ApiError.notFound(`MCP server not found: ${name}`) } @@ -497,13 +501,13 @@ async function updateServer(name: string, body: Record): Promis const previousScope = existing.scope try { - await removeMcpConfig(name, previousScope) + await runWithCwdOverride(previousLookupCwd, () => removeMcpConfig(name, previousScope)) await addMcpConfig(name, nextConfig, nextScope) } catch (error) { try { - const restored = getMcpConfigByName(name) + const restored = runWithCwdOverride(previousLookupCwd, () => getMcpConfigByName(name)) if (!restored) { - await addMcpConfig(name, previousConfig, previousScope) + await runWithCwdOverride(previousLookupCwd, () => addMcpConfig(name, previousConfig, previousScope)) } } catch { // Preserve the original update error below.