mirror of
https://github.com/NanmiCoder/cc-haha
synced 2026-07-17 13:13:35 +08:00
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
This commit is contained in:
parent
2db9a210b1
commit
82a47d051a
@ -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<typeof import('../api/sessions')>()
|
||||
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(<McpSettings />)
|
||||
|
||||
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',
|
||||
)
|
||||
})
|
||||
|
||||
|
||||
@ -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 } : {}),
|
||||
})
|
||||
},
|
||||
|
||||
|
||||
@ -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<McpServerRecord['config'], { type: 'stdio' }> {
|
||||
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() {
|
||||
)
|
||||
})}
|
||||
</div>
|
||||
{currentWorkDir && (draft.scope === 'local' || draft.scope === 'project') && (
|
||||
<p className="mt-3 break-all text-xs leading-5 text-[var(--color-text-tertiary)]">
|
||||
{t('settings.mcp.currentProjectHint', { path: currentWorkDir })}
|
||||
</p>
|
||||
)}
|
||||
</section>
|
||||
|
||||
<section className="rounded-[var(--radius-xl)] border border-[var(--color-border)] bg-[var(--color-surface)] p-5">
|
||||
<div className="mb-3 flex flex-wrap items-center justify-between gap-3">
|
||||
<div>
|
||||
<div className="text-sm font-semibold text-[var(--color-text-primary)]">
|
||||
{needsProjectTarget ? t('settings.mcp.targetProject.title') : t('settings.mcp.targetProject.globalTitle')}
|
||||
</div>
|
||||
<p className="mt-1 text-xs leading-5 text-[var(--color-text-tertiary)]">
|
||||
{targetProjectHint}
|
||||
</p>
|
||||
</div>
|
||||
{needsProjectTarget && (
|
||||
<DirectoryPicker
|
||||
value={draft.projectPath}
|
||||
onChange={(path) => setDraftField('projectPath', path)}
|
||||
/>
|
||||
)}
|
||||
</div>
|
||||
</section>
|
||||
|
||||
<section className="rounded-[var(--radius-xl)] border border-[var(--color-border)] bg-[var(--color-surface)] overflow-hidden">
|
||||
|
||||
@ -110,7 +110,8 @@ export const useMcpStore = create<McpStore>((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),
|
||||
|
||||
@ -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,
|
||||
|
||||
@ -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<string, unknown>): Promise<Response> {
|
||||
}
|
||||
|
||||
async function updateServer(name: string, body: Record<string, unknown>): Promise<Response> {
|
||||
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<string, unknown>): 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.
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user