diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index 530582b915..632bfb4536 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -2253,7 +2253,16 @@ export class AIChatManager { // a different server, and one the user has not opted into. refreshMcpServers = async (workspace = this.operatingWorkspace ?? '') => { const refreshId = ++this.mcpServersRefreshId - const servers = await loadMcpServers(workspace) + let servers: McpServer[] + try { + servers = await loadMcpServers(workspace) + } catch (e) { + // A listing that failed is not a workspace with nothing connected: reconciling + // against it would drop every registered tool and lift every refusal because a + // request happened to fail. Keep what the last good refresh settled on. + console.error('Failed to load MCP servers', e) + return + } if (refreshId !== this.mcpServersRefreshId) { return } diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts index 27a9359048..d6d8a14a15 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts @@ -549,6 +549,65 @@ describe('request rejections that withdraw registered tools', () => { }) }) +// A registration freezes what the remote said, but the remote can change a tool without +// its Windmill resource being touched — the listing TTL is what bounds that everywhere else. +describe('registrations against a changed remote', () => { + const server = SERVERS[0] + + function callArgs() { + return { + args: { owner: 'windmill-labs', repo: 'windmill' }, + workspace: 'test-ws', + helpers: {}, + toolCallbacks: createToolCallbacks(), + toolId: 'tool-1' + } as any + } + + it('refuses to send arguments built from a schema the remote has changed', async () => { + registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]]) + const registered = loadedMcpTools(OWNER)[0] + getMcpToolsMock.mockResolvedValue([ + { ...TOOLS[0], inputSchema: { type: 'object', properties: { issue_id: { type: 'string' } } } } + ]) + + const result = JSON.parse(await registered.fn(callArgs())) + + expect(callMcpToolMock).not.toHaveBeenCalled() + expect(result.success).toBe(false) + expect(result.schema).toEqual({ type: 'object', properties: { issue_id: { type: 'string' } } }) + // Dropped, so the next request stops advertising it and the model searches again. + expect(loadedMcpTools(OWNER)).toEqual([]) + }) + + it('reports a tool the remote no longer exposes', async () => { + registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]]) + const registered = loadedMcpTools(OWNER)[0] + getMcpToolsMock.mockResolvedValue([TOOLS[1]]) + + const result = JSON.parse(await registered.fn(callArgs())) + + expect(callMcpToolMock).not.toHaveBeenCalled() + expect(result.success).toBe(false) + expect(result.error).toContain('no longer exposed') + expect(loadedMcpTools(OWNER)).toEqual([]) + }) + + // The listing is a live call to a third party, and the backend re-asserts read_only + // on every call — so a blip must not take down a call the frozen copy can still make. + it('still calls on the frozen copy when the listing cannot be reached', async () => { + registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]]) + const registered = loadedMcpTools(OWNER)[0] + getMcpToolsMock.mockRejectedValue(new Error('connection refused')) + callMcpToolMock.mockResolvedValue({ content: [{ type: 'text', text: 'ok' }] }) + + const result = JSON.parse(await registered.fn(callArgs())) + + expect(result.success).toBe(true) + expect(callMcpToolMock).toHaveBeenCalled() + }) +}) + describe('read/write split', () => { it('refuses a mutating tool on the read path', async () => { const result = await run('call_mcp_read_tool', { @@ -887,6 +946,15 @@ describe('loadMcpServers', () => { expect(listResourceMock).not.toHaveBeenCalled() }) + // The caller reconciles registered tools and refusals against this list, so a failed + // request must not read as "nothing is connected". + it('fails rather than reporting an empty list when the resources cannot be read', async () => { + setMcpEnabled('test-ws', 'u/hugo/github_mcp', true) + listResourceMock.mockRejectedValue(new Error('503')) + + await expect(loadMcpServers('test-ws')).rejects.toThrow() + }) + it('advertises only the enabled server', async () => { setMcpEnabled('test-ws', 'u/hugo/github_mcp', true) expect(await loadMcpServers('test-ws')).toEqual([{ path: 'u/hugo/github_mcp' }]) diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index 5d0968ca3d..87c5928cef 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -185,8 +185,8 @@ export function loadedMcpTools(owner: string): Tool<{}>[] { } /** - * The servers holding a registered tool, with the revision each was frozen at, for the - * reconcile in `refreshMcpServers`. The revision matters as much as the path: a + * The servers holding a registered tool, with the revision each was frozen at, for + * `reconcileMcpRegistry`. The revision matters as much as the path: a * registered call bypasses the listing cache, so a connection edited elsewhere would * keep running against the schema it had before the edit. */ @@ -299,6 +299,15 @@ export function forgetLoadedMcpTools(owner: string, serverPath?: string) { if (registry.tools.size === 0) registries.delete(owner) } +function forgetRegisteredTool(owner: string, key: string) { + const registry = registries.get(owner) + if (!registry) return + registry.tools.delete(key) + registry.lastUsed.delete(key) + registry.editedAt.delete(key) + if (registry.tools.size === 0) registries.delete(owner) +} + function clearRefused(owner: string, serverPath?: string) { if (serverPath === undefined) { refused.delete(owner) @@ -349,19 +358,17 @@ export async function loadMcpServers(workspace: string): Promise { // without a request — this runs before every send. const enabled = enabledMcpPaths(workspace) if (enabled.length === 0) return [] - try { - const resources = await ResourceService.listResource({ - workspace, - resourceType: 'mcp', - perPage: 100 - }) - return resources - .filter((r) => enabled.includes(r.path)) - .map((r) => ({ path: r.path, editedAt: r.edited_at })) - } catch (e) { - console.error('Failed to load MCP servers', e) - return [] - } + // Throws rather than answering `[]`: the caller reconciles registered tools and + // refusals against this list, and a failed request read as "nothing connected" + // would drop both on a blip. + const resources = await ResourceService.listResource({ + workspace, + resourceType: 'mcp', + perPage: 100 + }) + return resources + .filter((r) => enabled.includes(r.path)) + .map((r) => ({ path: r.path, editedAt: r.edited_at })) } function tokenize(text: string): string[] { @@ -446,6 +453,50 @@ async function resolveTool( return { server, tool } } +/** + * The current definition of a registered tool, or the reason not to run the call. + * + * A registration freezes a schema and a `readOnlyHint` for the conversation, but a remote + * can change or withdraw a tool without its Windmill resource being touched — which is + * what `TOOLS_CACHE_TTL_MS` bounds for every other path. So the listing decides what runs + * here too, as it does for the wrapper. Arguments the model built from the old schema are + * not sent to a new one: the registration is dropped and the model searches again. + */ +async function revalidateRegistration( + owner: string, + workspace: string, + server: McpServer, + key: string, + frozen: McpToolDef +): Promise<{ tool: McpToolDef } | { error: string; schema?: unknown }> { + let tools: McpToolDef[] + try { + tools = await loadServerTools(workspace, server.path, server.editedAt) + } catch { + // Listing is a live call to a third party. One failure must not take down a call + // the frozen copy can still make — the backend re-asserts `read_only` regardless. + return { tool: frozen } + } + const current = tools.find((t) => t.name === frozen.name) + if (!current) { + forgetRegisteredTool(owner, key) + return { + error: `${frozen.name} is no longer exposed by ${server.path}. Use search_mcp_tools to find what is.` + } + } + if ( + JSON.stringify(current.inputSchema) !== JSON.stringify(frozen.inputSchema) || + isReadOnly(current) !== isReadOnly(frozen) + ) { + forgetRegisteredTool(owner, key) + return { + error: `${frozen.name} changed on ${server.path} since it was loaded, so these arguments were not sent. Call search_mcp_tools again to load it as it is now.`, + schema: current.inputSchema + } + } + return { tool: current } +} + /** Flatten the MCP content blocks into the text the model can act on. */ function extractResultData(result: unknown): unknown { const content = (result as { content?: unknown })?.content @@ -846,7 +897,11 @@ export function registerMcpTools( content: `Calling ${tool.name}...`, mcpServer: server.path }) - const result = await executeTool(owner, workspace, server, tool, args ?? {}, readOnly) + const current = await revalidateRegistration(owner, workspace, server, key, tool) + const result = + 'error' in current + ? bounded({ success: false, ...current }) + : await executeTool(owner, workspace, server, current.tool, args ?? {}, readOnly) const ok = JSON.parse(result).success === true toolCallbacks.setToolStatus(toolId, { content: ok ? `Called ${tool.name}` : `Call to ${tool.name} failed`,