diff --git a/src/web-ui/src/infrastructure/api/service-api/MCPAPI.test.ts b/src/web-ui/src/infrastructure/api/service-api/MCPAPI.test.ts new file mode 100644 index 0000000000..ac22d6fcd1 --- /dev/null +++ b/src/web-ui/src/infrastructure/api/service-api/MCPAPI.test.ts @@ -0,0 +1,73 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { MCPAPI } from './MCPAPI'; + +const invokeMock = vi.hoisted(() => vi.fn()); +const scopeMock = vi.hoisted(() => ({ assertCurrent: vi.fn() })); +vi.mock('./ApiClient', () => ({ api: { invoke: invokeMock } })); +vi.mock('@/infrastructure/peer-device/deviceSurface', () => ({ + getActiveSurfaceScope: () => scopeMock, +})); + +describe('MCP JSON save acknowledgement', () => { + beforeEach(() => { invokeMock.mockReset(); scopeMock.assertCurrent.mockReset(); }); + + it('accepts the existing void success response', async () => { + invokeMock.mockResolvedValue(undefined); + await expect(MCPAPI.saveMCPJsonConfig('{}', 'revision')).resolves.toEqual({ runtimeApplied: true }); + expect(invokeMock).toHaveBeenCalledWith('save_mcp_json_config', { + jsonConfig: '{}', expectedFingerprint: 'revision', + }); + }); + + it.each([ + 'MCP config was saved, but runtime reconciliation failed: connection refused', + new Error('MCP config was saved, but runtime reconciliation failed: connection refused'), + ])('recognizes an explicit persisted acknowledgement: %s', async (error) => { + invokeMock.mockRejectedValue(error); + await expect(MCPAPI.saveMCPJsonConfig('{}', 'revision')).resolves.toEqual({ runtimeApplied: false }); + }); + + it.each([ + 'Failed to save config: permission denied', + 'MCP configuration changed; reload before saving', + new Error('Request timeout: save_mcp_json_config'), + 'connection refused', + ])('does not assume an unacknowledged write succeeded: %s', async (error) => { + invokeMock.mockRejectedValue(error); + await expect(MCPAPI.saveMCPJsonConfig('{}', 'revision')).rejects.toBe(error); + }); + + it('confirms a timed-out save by reading back the same JSON regardless of key order', async () => { + const error = Object.assign(new Error('Request timeout'), { code: 'REQUEST_TIMEOUT' }); + invokeMock.mockRejectedValueOnce(error).mockResolvedValueOnce({ + jsonConfig: '{"mcpServers":{"offline":{"autoStart":true,"args":["a","b"]}}}', + fingerprint: 'saved', + }); + await expect(MCPAPI.saveMCPJsonConfig( + '{"mcpServers":{"offline":{"args":["a","b"],"autoStart":true}}}', 'revision', + )).resolves.toEqual({ runtimeApplied: false }); + expect(invokeMock.mock.calls.map(call => call[0])).toEqual(['save_mcp_json_config', 'load_mcp_json_config']); + }); + + it.each([ + { jsonConfig: '{"mcpServers":{"offline":{"args":["b","a"]}}}', fingerprint: 'other' }, + null, + ])('retains a timeout when read-back cannot confirm the requested content: %s', async (snapshot) => { + const error = Object.assign(new Error('Request timeout'), { code: 'REQUEST_TIMEOUT' }); + invokeMock.mockRejectedValueOnce(error); + if (snapshot) invokeMock.mockResolvedValueOnce(snapshot); + else invokeMock.mockRejectedValueOnce(new Error('Host disconnected')); + await expect(MCPAPI.saveMCPJsonConfig( + '{"mcpServers":{"offline":{"args":["a","b"]}}}', 'revision', + )).rejects.toBe(error); + expect(invokeMock).toHaveBeenCalledTimes(2); + }); + + it('does not read configuration from a newly selected device after a timeout', async () => { + const changed = new Error('Surface changed'); + scopeMock.assertCurrent.mockImplementation(() => { throw changed; }); + invokeMock.mockRejectedValueOnce(Object.assign(new Error('Request timeout'), { code: 'REQUEST_TIMEOUT' })); + await expect(MCPAPI.saveMCPJsonConfig('{}', 'revision')).rejects.toBe(changed); + expect(invokeMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/web-ui/src/infrastructure/api/service-api/MCPAPI.ts b/src/web-ui/src/infrastructure/api/service-api/MCPAPI.ts index 7294e5ad45..122af2c53e 100644 --- a/src/web-ui/src/infrastructure/api/service-api/MCPAPI.ts +++ b/src/web-ui/src/infrastructure/api/service-api/MCPAPI.ts @@ -1,6 +1,16 @@ import { api } from './ApiClient'; +import { getActiveSurfaceScope } from '@/infrastructure/peer-device/deviceSurface'; + +function canonicalConfig(json: string): string { + return JSON.stringify(JSON.parse(json), (_key, value) => { + if (value && typeof value === 'object' && !Array.isArray(value)) { + return Object.fromEntries(Object.keys(value).sort().map(key => [key, value[key]])); + } + return value; + }); +} /** MCP Apps protocol version (aligned with VSCode modelContextProtocolApps.ts). */ export const MCP_APPS_PROTOCOL_VERSION = '2026-01-26'; @@ -335,8 +345,36 @@ export class MCPAPI { } - static async saveMCPJsonConfig(jsonConfig: string, expectedFingerprint: string): Promise { - return api.invoke('save_mcp_json_config', { jsonConfig, expectedFingerprint }); + static async saveMCPJsonConfig( + jsonConfig: string, + expectedFingerprint: string, + ): Promise<{ runtimeApplied: boolean }> { + const scope = getActiveSurfaceScope(); + try { + await api.invoke('save_mcp_json_config', { jsonConfig, expectedFingerprint }); + scope.assertCurrent('save MCP configuration'); + return { runtimeApplied: true }; + } catch (error) { + scope.assertCurrent('confirm saved MCP configuration'); + // Existing hosts return this explicit post-persistence error over the wire. + // Other errors do not prove persistence. A timeout needs a matching + // read-back before the UI may discard the draft. + const message = error instanceof Error ? error.message : error; + if (typeof message === 'string' + && message.startsWith('MCP config was saved, but runtime reconciliation failed:')) { + return { runtimeApplied: false }; + } + if (error instanceof Error && (error as Error & { code?: string }).code === 'REQUEST_TIMEOUT') { + // Connection setup can outlive the invoke deadline after the write + // committed. Read back on the same surface; never replay the mutation. + const snapshot = await this.loadMCPJsonConfig().catch(() => null); + scope.assertCurrent('read back saved MCP configuration'); + if (snapshot && canonicalConfig(snapshot.jsonConfig) === canonicalConfig(jsonConfig)) { + return { runtimeApplied: false }; + } + } + throw error; + } } /** diff --git a/src/web-ui/src/infrastructure/config/components/McpToolsConfig.test.tsx b/src/web-ui/src/infrastructure/config/components/McpToolsConfig.test.tsx index e4ea779d44..ac88a40d6d 100644 --- a/src/web-ui/src/infrastructure/config/components/McpToolsConfig.test.tsx +++ b/src/web-ui/src/infrastructure/config/components/McpToolsConfig.test.tsx @@ -82,7 +82,7 @@ describe('McpToolsConfig remote behavior', () => { jsonConfig: '{"mcpServers":{}}', fingerprint: 'sha256:test', }); - saveJsonConfigMock.mockReset().mockResolvedValue(undefined); + saveJsonConfigMock.mockReset().mockResolvedValue({ runtimeApplied: true }); initializeServersMock.mockReset().mockResolvedValue(undefined); startServerMock.mockReset().mockResolvedValue(undefined); startRemoteOAuthMock.mockReset().mockResolvedValue({ @@ -259,6 +259,66 @@ describe('McpToolsConfig remote behavior', () => { expect(initializeServersMock).not.toHaveBeenCalled(); }); + it('clears a persisted draft and reloads its fingerprint when runtime application fails', async () => { + peerState.active = false; + saveJsonConfigMock.mockResolvedValue({ runtimeApplied: false }); + await act(async () => { root.render(); }); + await act(async () => { + (container.querySelector('[data-testid="mcp-json-toggle"]') as HTMLButtonElement).click(); + }); + const editedJson = '{"mcpServers":{"offline":{"url":"http://127.0.0.1:9999/mcp"}}}'; + await act(async () => { + const textarea = container.querySelector('textarea')!; + Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, 'value')!.set!.call(textarea, editedJson); + textarea.dispatchEvent(new Event('input', { bubbles: true })); + }); + loadJsonConfigMock.mockResolvedValue({ jsonConfig: editedJson, fingerprint: 'sha256:saved' }); + await act(async () => { + (container.querySelector('[data-testid="mcp-json-save"]') as HTMLButtonElement).click(); + }); + expect(notificationMocks.error).not.toHaveBeenCalled(); + expect(notificationMocks.success).not.toHaveBeenCalled(); + expect(notificationMocks.warning).toHaveBeenCalledWith('messages.partialStartFailed', expect.anything()); + expect(loadJsonConfigMock).toHaveBeenCalledTimes(2); + expect(container.querySelector('textarea')).toBeNull(); + await act(async () => { + (container.querySelector('[data-testid="mcp-json-toggle"]') as HTMLButtonElement).click(); + }); + expect(container.querySelector('textarea')!.value).toBe(editedJson); + expect((container.querySelector('[data-testid="mcp-json-save"]') as HTMLButtonElement).disabled).toBe(true); + await act(async () => { + const textarea = container.querySelector('textarea')!; + Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, 'value')!.set!.call(textarea, editedJson + '\n'); + textarea.dispatchEvent(new Event('input', { bubbles: true })); + }); + await act(async () => { + (container.querySelector('[data-testid="mcp-json-save"]') as HTMLButtonElement).click(); + }); + expect(saveJsonConfigMock).toHaveBeenLastCalledWith(editedJson + '\n', 'sha256:saved'); + }); + + it('retains the editor and draft when persistence fails', async () => { + peerState.active = false; + saveJsonConfigMock.mockRejectedValue(new Error('Failed to save config: permission denied')); + await act(async () => { root.render(); }); + await act(async () => { + (container.querySelector('[data-testid="mcp-json-toggle"]') as HTMLButtonElement).click(); + }); + await act(async () => { + const textarea = container.querySelector('textarea')!; + Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, 'value')!.set!.call(textarea, '{"mcpServers":{}}\n'); + textarea.dispatchEvent(new Event('input', { bubbles: true })); + }); + await act(async () => { + (container.querySelector('[data-testid="mcp-json-save"]') as HTMLButtonElement).click(); + }); + expect(notificationMocks.error).toHaveBeenCalled(); + expect(notificationMocks.warning).not.toHaveBeenCalled(); + expect(notificationMocks.success).not.toHaveBeenCalled(); + expect(container.querySelector('textarea')!.value).toBe('{"mcpServers":{}}\n'); + expect(loadJsonConfigMock).toHaveBeenCalledTimes(1); + }); + it('offers start rather than stop for an uninitialized server', async () => { getServersMock.mockResolvedValueOnce([{ id: 'local-test', diff --git a/src/web-ui/src/infrastructure/config/components/McpToolsConfig.tsx b/src/web-ui/src/infrastructure/config/components/McpToolsConfig.tsx index e905518951..206b7f5bf2 100644 --- a/src/web-ui/src/infrastructure/config/components/McpToolsConfig.tsx +++ b/src/web-ui/src/infrastructure/config/components/McpToolsConfig.tsx @@ -461,7 +461,7 @@ const McpToolsConfig: React.FC = () => { const hasPendingAutoStart = servers.some((server) => { if (!server.enabled || !server.autoStart) return false; const status = server.status.trim().toLowerCase(); - return ['uninitialized', 'starting', 'reconnecting', 'stopping'].includes(status); + return ['uninitialized', 'starting', 'reconnecting', 'failed', 'stopping'].includes(status); }); if (!hasPendingAutoStart) return; @@ -553,12 +553,23 @@ const McpToolsConfig: React.FC = () => { if (!jsonConfigFingerprint) { throw new Error('MCP configuration snapshot is unavailable; reload before saving'); } - await MCPAPI.saveMCPJsonConfig(jsonConfig, jsonConfigFingerprint); + const result = await MCPAPI.saveMCPJsonConfig(jsonConfig, jsonConfigFingerprint); if (!capabilityIsCurrent(capabilityEpoch)) return false; - notification.success(tMcp('messages.saveSuccess'), { - title: tMcp('notifications.saveSuccess'), - duration: 3000, - }); + // Persistence succeeded even if applying the runtime failed. Clear the + // draft now, and invalidate the old fingerprint until read-back completes. + setJsonSavedConfig(jsonConfig); + setJsonConfigFingerprint(''); + if (result.runtimeApplied) { + notification.success(tMcp('messages.saveSuccess'), { + title: tMcp('notifications.saveSuccess'), + duration: 3000, + }); + } else { + notification.warning(tMcp('messages.partialStartFailed'), { + title: tMcp('messages.saveSuccess'), + duration: 10000, + }); + } setShowJsonEditor(false); await loadServers(); if (capabilityIsCurrent(capabilityEpoch)) { @@ -1219,6 +1230,7 @@ const McpToolsConfig: React.FC = () => { ) : null}