From c07f3eed0f4bbed5c2fbb77168b0a2d029acf935 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Thu, 10 Sep 2026 12:51:49 +0200 Subject: [PATCH] feat: register searched MCP tools with their real schemas in the chat Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JgzuxyafKNF2uaEeL35XQw --- .../copilot/chat/AIChatManager.svelte.ts | 20 +- .../copilot/chat/AssistantMcpSection.svelte | 11 +- .../copilot/chat/ChatCollapsibleCard.svelte | 5 + .../copilot/chat/ToolExecutionDisplay.svelte | 32 +++ .../chat/WebSearchSourcesDisplay.svelte | 12 +- .../copilot/chat/global/mcpTools.test.ts | 112 +++++++++- .../copilot/chat/global/mcpTools.ts | 205 ++++++++++++++++-- .../src/lib/components/copilot/chat/shared.ts | 4 + .../lib/components/mcp/McpServerIcon.svelte | 35 +++ frontend/src/lib/components/mcp/iconCache.ts | 46 +++- .../src/lib/components/mcp/mcpMenu.svelte.ts | 19 +- frontend/src/lib/components/mcp/serverMark.ts | 37 ++++ frontend/src/lib/utils/faviconUrl.ts | 15 ++ 13 files changed, 505 insertions(+), 48 deletions(-) create mode 100644 frontend/src/lib/components/mcp/McpServerIcon.svelte create mode 100644 frontend/src/lib/components/mcp/serverMark.ts create mode 100644 frontend/src/lib/utils/faviconUrl.ts diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index 43df13f78c..41570610d2 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -134,7 +134,14 @@ import { } from './global/core' import { formatChatJobCompletion } from './datatableTools' import { isGlobalAiEnabled } from './global/gate' -import { createMcpTools, loadMcpServers, type McpServer } from './global/mcpTools' +import { + createMcpTools, + forgetLoadedMcpTools, + loadedMcpServerPaths, + loadedMcpTools, + loadMcpServers, + type McpServer +} from './global/mcpTools' import { pipelineTools, getPipelinePromptSection, @@ -2231,6 +2238,13 @@ export class AIChatManager { // workspace's servers installed would go on advertising its paths against // the workspace switched to. this.mcpServers = workspace === (this.operatingWorkspace ?? '') ? servers : [] + // A tool registered from a server that has since been turned off, deleted, or + // left behind by a workspace switch would still be callable, and would run + // against whichever workspace the chat is on now. + const live = new Set(this.mcpServers.map((s) => s.path)) + for (const path of loadedMcpServerPaths()) { + if (!live.has(path)) forgetLoadedMcpTools(path) + } if (this.mode === AIMode.GLOBAL) { this.configureGlobalMode() } @@ -2683,7 +2697,9 @@ export class AIChatManager { return base }, get tools() { - return [...self.tools, ...self.planMode.tools] + // Re-read every iteration by `chatLoop`, so a remote MCP tool registered + // during one iteration is callable on the next. + return [...self.tools, ...self.planMode.tools, ...loadedMcpTools()] }, get helpers() { return self.helpers diff --git a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte index 9440aa8f58..5ada7e16f8 100644 --- a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte +++ b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte @@ -16,10 +16,12 @@ switch that decides whether this chat carries its tools. import { isMcpEnabled, setMcpEnabled } from '$lib/components/mcp/enabledServers' import { loadProviderIcon } from '$lib/components/mcp/providerIcon' import { + cachedProviderHost, cachedProviderKey, forgetProviderKey, rememberProviderKey } from '$lib/components/mcp/iconCache' + import McpServerIcon from '$lib/components/mcp/McpServerIcon.svelte' import ConfirmationModal from '$lib/components/common/confirmationModal/ConfirmationModal.svelte' import type { Component } from 'svelte' import { ResourceService } from '$lib/gen' @@ -83,6 +85,7 @@ switch that decides whether this chat carries its tools. editedAt?: string enabled: boolean icon?: Component + iconHost?: string }[] >([]) let loading = $state(false) @@ -383,6 +386,7 @@ switch that decides whether this chat carries its tools. const icon = await loadProviderIcon(key) if (seq !== loadSeq) return server.icon = icon + server.iconHost = cachedProviderHost(target, server.path, server.editedAt) }) ) } @@ -469,12 +473,7 @@ switch that decides whether this chat carries its tools.
{#each servers as server (server.path)} {#snippet icon()} - {#if server.icon} - {@const Icon = server.icon} - - {:else} - - {/if} + {/snippet} {#snippet title()} {server.path} diff --git a/frontend/src/lib/components/copilot/chat/ChatCollapsibleCard.svelte b/frontend/src/lib/components/copilot/chat/ChatCollapsibleCard.svelte index f985e23596..f6c364e1a0 100644 --- a/frontend/src/lib/components/copilot/chat/ChatCollapsibleCard.svelte +++ b/frontend/src/lib/components/copilot/chat/ChatCollapsibleCard.svelte @@ -15,6 +15,9 @@ shimmer?: boolean // Pinned to the right of the header row, outside the toggle button. headerRight?: Snippet + // Sits before the label, inside the toggle button, and outside the shimmer: + // a mark belongs to the label it names, but must not sweep with it. + headerLeft?: Snippet // Always-visible content between the header and the expandable body. belowHeader?: Snippet children?: Snippet @@ -31,6 +34,7 @@ toggleable = true, shimmer = false, headerRight, + headerLeft, belowHeader, children, class: className, @@ -63,6 +67,7 @@ disabled={!toggleable} aria-expanded={toggleable ? expanded : undefined} > + {@render headerLeft?.()} {#if shimmer} {@render labelText(false)} diff --git a/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte b/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte index 34b1adcf68..d93ec644ba 100644 --- a/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte @@ -39,6 +39,9 @@ import AskUserQuestionDisplay from './AskUserQuestionDisplay.svelte' import WebSearchSourcesDisplay from './WebSearchSourcesDisplay.svelte' import ExpandableImage from '$lib/components/common/image/ExpandableImage.svelte' + import McpServerIcon from '$lib/components/mcp/McpServerIcon.svelte' + import { resolveMcpServerMark } from '$lib/components/mcp/serverMark' + import { mcpServerForToolName } from './global/mcpTools' interface Props { message: ToolDisplayMessage @@ -46,6 +49,21 @@ let { message }: Props = $props() + // A call records its server on the row, which is what survives a reload. The rest + // is for rows written before it did: the generic wrappers name their server in the + // arguments, and a registered tool is resolved through the in-memory registry. + const MCP_CALL_TOOLS = ['call_mcp_read_tool', 'call_mcp_write_tool'] + const mcpServerPath = $derived.by(() => { + if (message.mcpServer) return message.mcpServer + const name = message.toolName + if (!name) return undefined + if (MCP_CALL_TOOLS.includes(name)) { + const server = message.parameters?.server + return typeof server === 'string' ? server : undefined + } + return mcpServerForToolName(name) + }) + const isPlanReview = $derived(message.toolName === EXIT_PLAN_MODE_TOOL) const isPlanCard = $derived(isPlanCardTool(message.toolName)) const planCopy = $derived( @@ -235,6 +253,19 @@ {/if} {/snippet} + + {#snippet serverMark()} + {#if mcpServerPath && aiChatManager.operatingWorkspace} + {#await resolveMcpServerMark(aiChatManager.operatingWorkspace, mcpServerPath) then mark} + {#if mark.icon || mark.host} + + {/if} + {/await} + {/if} + {/snippet} + @@ -251,6 +282,7 @@ labelClass={showPreviewChip ? 'truncate' : ''} contentClass="space-y-3" headerRight={showPreviewChip ? previewChip : undefined} + headerLeft={mcpServerPath ? serverMark : undefined} > {#snippet belowHeader()} diff --git a/frontend/src/lib/components/copilot/chat/WebSearchSourcesDisplay.svelte b/frontend/src/lib/components/copilot/chat/WebSearchSourcesDisplay.svelte index ff71c44160..81a620b91a 100644 --- a/frontend/src/lib/components/copilot/chat/WebSearchSourcesDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/WebSearchSourcesDisplay.svelte @@ -1,6 +1,7 @@
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 a155115d28..bb7c502e89 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts @@ -27,7 +27,16 @@ vi.mock('$lib/stores', () => ({ userStore: { subscribe: (run: (v: unknown) => void) => (run({ ...session }), () => {}) } })) -import { clearMcpToolsCache, createMcpTools, loadMcpServers, type McpServer } from './mcpTools' +import { + clearMcpToolsCache, + createMcpTools, + forgetLoadedMcpTools, + loadedMcpServerPaths, + loadedMcpTools, + loadMcpServers, + registerMcpTools, + type McpServer +} from './mcpTools' import { setMcpEnabled } from '$lib/components/mcp/enabledServers' const SERVERS: McpServer[] = [{ path: 'u/hugo/github_mcp' }] @@ -94,6 +103,98 @@ describe('tool registration', () => { }) }) +describe('loaded remote tools', () => { + const server = SERVERS[0] + + it('registers a remote tool under its own schema and server-scoped name', () => { + const [name] = registerMcpTools(server, [TOOLS[0]]) + + expect(name).toBe('mcp_u_hugo_github_mcp__get_issue') + const tool = loadedMcpTools().find((t) => t.def.function.name === name) + expect(tool?.def.function.parameters).toEqual(TOOLS[0].inputSchema) + }) + + // The wrappers ask for confirmation by which one the model picked; a registered + // tool carries its own hint, so the gate is per tool. + it('gates a mutating tool and lets a read-only one through', () => { + registerMcpTools(server, [TOOLS[0], TOOLS[1], TOOLS[2]]) + const byName = Object.fromEntries(loadedMcpTools().map((t) => [t.def.function.name, t])) + + expect(byName['mcp_u_hugo_github_mcp__get_issue'].requiresConfirmation).toBeUndefined() + expect(byName['mcp_u_hugo_github_mcp__merge_pull_request'].requiresConfirmation).toBe(true) + // No annotations at all must not read as read-only. + expect(byName['mcp_u_hugo_github_mcp__unannotated_tool'].requiresConfirmation).toBe(true) + }) + + // A registered tool freezes a copy of `readOnlyHint`, which is what the listing + // TTL exists to bound — so it must not outlive the listing it came from. + it('drops loaded tools when the listing cache is cleared', () => { + registerMcpTools(server, [TOOLS[0]]) + expect(loadedMcpTools()).toHaveLength(1) + + clearMcpToolsCache() + + expect(loadedMcpTools()).toEqual([]) + }) + + it('drops only the named server when reconciling against the live list', () => { + registerMcpTools(server, [TOOLS[0]]) + registerMcpTools({ path: 'f/team/linear_mcp' }, [TOOLS[0]]) + expect(loadedMcpServerPaths().sort()).toEqual(['f/team/linear_mcp', 'u/hugo/github_mcp']) + + forgetLoadedMcpTools('f/team/linear_mcp') + + expect(loadedMcpServerPaths()).toEqual(['u/hugo/github_mcp']) + }) + + // The reported bug: the model saw only parameter names, so it invented values for + // constrained arguments (Linear's `orderBy`, whose enum it never saw). Searching + // must put the real schema in front of it, not a list of names. + it('registers what a search matched, with the remote schema', async () => { + setMcpEnabled('test-ws', 'u/hugo/github_mcp', true) + const result = await run('search_mcp_tools', { query: 'issue' }) + + const match = result.matches.find((m: any) => m.tool === 'get_issue') + expect(match.call).toBe('mcp_u_hugo_github_mcp__get_issue') + expect(match.params).toBeUndefined() + + const registered = loadedMcpTools().find((t) => t.def.function.name === match.call) + expect(registered?.def.function.parameters).toEqual(TOOLS[0].inputSchema) + }) + + // The row's provider icon is resolved from this, and the registry of loaded tools + // lives only in memory — so without it on the message, a reloaded transcript loses + // every mark. Recorded before the server is resolved, so an unreachable server + // still marks its own failure row. + it('records the server on the row, even when it cannot be reached', async () => { + getMcpToolsMock.mockRejectedValue(new Error('connection refused')) + const callbacks = createToolCallbacks() + + await getTool('call_mcp_read_tool').fn({ + args: { server: 'u/hugo/github_mcp', tool: 'get_issue', arguments: {} }, + workspace: 'test-ws', + helpers: {}, + toolCallbacks: callbacks, + toolId: 'tool-1' + }) + + expect(callbacks.setToolStatus).toHaveBeenCalledWith('tool-1', { + mcpServer: 'u/hugo/github_mcp' + }) + }) + + it('bounds the loaded set, evicting the least recently registered', () => { + for (let i = 0; i < 30; i++) { + registerMcpTools(server, [{ ...TOOLS[0], name: `tool_${i}` }]) + } + const names = loadedMcpTools().map((t) => t.def.function.name) + + expect(names).toHaveLength(25) + expect(names).not.toContain('mcp_u_hugo_github_mcp__tool_0') + expect(names).toContain('mcp_u_hugo_github_mcp__tool_29') + }) +}) + describe('read/write split', () => { it('refuses a mutating tool on the read path', async () => { const result = await run('call_mcp_read_tool', { @@ -228,15 +329,18 @@ describe('call results', () => { }) describe('search_mcp_tools', () => { - it('returns compact summaries without the full input schemas', async () => { + // The schema reaches the model through the registered tool, not inlined here: the + // result stays a summary so a 10-match search does not also dump 10 schemas into + // the transcript on top of the tool definitions it just created. + it('returns compact summaries that name the registered tool', async () => { const result = await run('search_mcp_tools', { query: 'issue' }) expect(result.matches).toEqual([ { server: 'u/hugo/github_mcp', tool: 'get_issue', + call: 'mcp_u_hugo_github_mcp__get_issue', description: 'Get details of a GitHub issue', - mode: 'read', - params: ['owner', 'repo'] + mode: 'read' } ]) }) diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index d5f16d5c54..db6b236601 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -4,12 +4,17 @@ import { createToolDef, type Tool } from '../shared' import { enabledMcpPaths } from '$lib/components/mcp/enabledServers' /** - * Access to the MCP servers the user has connected (resources of type `mcp`) - * as three static tools — search, read call, write call — instead of one - * registered tool per remote tool. A server like GitHub's exposes ~90 tools, - * whose schemas would otherwise be re-sent on every chat iteration; here only - * matched summaries enter the model's context, and a full input schema only - * after a call fails. + * Access to the MCP servers the user has connected (resources of type `mcp`), + * loaded on demand rather than all at once: a server like GitHub's exposes ~90 + * tools, whose schemas would otherwise sit in the model's context on every + * iteration. + * + * `search_mcp_tools` is the entry point and registers the tools it matched, so + * from the next iteration each one is a tool of its own carrying the remote's + * real input schema. Only searched-for tools are ever paid for. The read/write + * call wrappers remain as a fallback for anything not registered, and take a + * free-form argument object — which is what the model has to guess into, and + * why the registered tool is preferred wherever there is one. */ type McpToolDef = GetMcpToolsResponse[number] @@ -18,6 +23,11 @@ export type McpServer = { path: string; editedAt?: string } const MAX_SEARCH_RESULTS = 10 const MAX_DESCRIPTION_CHARS = 200 +// A registered tool's schema sits in the model's context for the rest of the +// session, so the set is bounded and the least recently called one is evicted. +const MAX_LOADED_TOOLS = 25 +// Both providers cap tool names; OpenAI's 64 is the lower of the two. +const MAX_TOOL_NAME_CHARS = 64 const MAX_RESULT_CHARS = 20_000 // A server writes its own error text, and every enabled server can contribute // one, so search results are capped the same way call results are. @@ -64,6 +74,62 @@ async function loadServerTools( export function clearMcpToolsCache() { cacheGeneration++ toolsCache = {} + forgetLoadedMcpTools() +} + +/** + * Remote tools promoted to first-class chat tools for this session, keyed + * `${server}::${tool}`. `chatLoop` re-reads its tool list on every iteration, so + * one registered while a call is running is callable on the next one. + * + * A registered tool is a frozen copy of an input schema *and* of `readOnlyHint`, + * which is the thing `TOOLS_CACHE_TTL_MS` exists to bound — so these are dropped + * by the same triggers that drop a listing rather than left to age on a timer. + * What keeps that safe in between is the backend, not this map: `executeTool` + * asserts `read_only` against the live server, so a stale hint cannot turn into + * an unconfirmed write. + */ +const loadedTools = new Map>() + +/** Insertion order is call order, so the first entry is the least recently used. */ +function touchLoadedTool(key: string) { + const tool = loadedTools.get(key) + if (!tool) return + loadedTools.delete(key) + loadedTools.set(key, tool) +} + +export function loadedMcpTools(): Tool<{}>[] { + return [...loadedTools.values()] +} + +/** The servers that currently have a tool registered, for reconciling against the live list. */ +export function loadedMcpServerPaths(): string[] { + return [...new Set([...loadedTools.keys()].map((key) => key.slice(0, key.lastIndexOf('::'))))] +} + +/** + * The server a chat-facing tool name belongs to, for marking its transcript row with + * the provider. Resolved through the registry rather than by parsing the name, which + * a long path truncates. + */ +export function mcpServerForToolName(toolName: string): string | undefined { + for (const [key, tool] of loadedTools) { + if (tool.def.function.name === toolName) return key.slice(0, key.lastIndexOf('::')) + } + return undefined +} + +/** Drop every loaded tool, or only those belonging to one server. */ +export function forgetLoadedMcpTools(serverPath?: string) { + if (serverPath === undefined) { + loadedTools.clear() + return + } + const prefix = `${serverPath}::` + for (const key of [...loadedTools.keys()]) { + if (key.startsWith(prefix)) loadedTools.delete(key) + } } /** @@ -78,6 +144,7 @@ function forgetServerTools(workspace: string, path: string) { for (const key of Object.keys(toolsCache)) { if (key.startsWith(prefix)) delete toolsCache[key] } + forgetLoadedMcpTools(path) } /** @@ -149,14 +216,16 @@ function truncate(text: string, max: number): string { return text.length > max ? text.slice(0, max) + '…' : text } -function summarizeTool(server: McpServer, tool: McpToolDef) { +function summarizeTool(server: McpServer, tool: McpToolDef, callName?: string) { const params = schemaPropertyNames(tool.inputSchema) return { server: server.path, tool: tool.name, + // Once the tool is registered its real schema is in front of the model, so the + // bare parameter names it used to guess from are dropped rather than repeated. + ...(callName ? { call: callName } : params.length > 0 ? { params } : {}), description: truncate(tool.description ?? '', MAX_DESCRIPTION_CHARS), - mode: isReadOnly(tool) ? 'read' : 'write', - ...(params.length > 0 ? { params } : {}) + mode: isReadOnly(tool) ? 'read' : 'write' } } @@ -341,8 +410,8 @@ function createCallTool(servers: McpServer[], mode: 'read' | 'write'): Tool<{}> callMcpToolSchema, isRead ? 'call_mcp_read_tool' : 'call_mcp_write_tool', isRead - ? 'Call a read-only tool on a connected MCP server. Use search_mcp_tools first to find the server and tool names; a failed call returns the tool argument schema.' - : 'Call a tool that modifies data on a connected MCP server; the user is asked to confirm. Use search_mcp_tools first to find the server and tool names; a failed call returns the tool argument schema.' + ? 'Fallback for a read-only tool that search_mcp_tools did not give a `call` name for. Prefer that named tool, which carries the real argument schema; a failed call here returns it.' + : 'Fallback for a mutating tool that search_mcp_tools did not give a `call` name for; the user is asked to confirm. Prefer that named tool, which carries the real argument schema; a failed call here returns it.' ), showDetails: true, ...(isRead @@ -353,6 +422,10 @@ function createCallTool(servers: McpServer[], mode: 'read' | 'write'): Tool<{}> }), fn: async ({ args, workspace, toolId, toolCallbacks }) => { const parsed = callMcpToolSchema.parse(args) + // Recorded from the arguments, before resolution: resolving needs a live + // listing, so a server that cannot be reached would otherwise leave its own + // failure row unmarked. + toolCallbacks.setToolStatus(toolId, { mcpServer: parsed.server }) // Listing is a live call to a third party: a server that has gone away // must fail this tool, not the chat loop around it. let resolved: Awaited> @@ -391,6 +464,89 @@ function createCallTool(servers: McpServer[], mode: 'read' | 'write'): Tool<{}> } } +function sanitizeToolNamePart(part: string): string { + return part.replace(/[^a-zA-Z0-9_-]/g, '_') +} + +/** djb2, so two long names truncated to the same prefix stay distinct. */ +function shortHash(text: string): string { + let hash = 5381 + for (let i = 0; i < text.length; i++) hash = ((hash * 33) ^ text.charCodeAt(i)) >>> 0 + return hash.toString(36).padStart(7, '0').slice(0, 7) +} + +/** + * The chat-facing name. It carries the server so two servers exposing + * `list_issues` stay apart, and nothing parses it back: the registry keys on the + * pair, so truncation only has to stay unique. + */ +function registeredToolName(serverPath: string, toolName: string): string { + const full = `mcp_${sanitizeToolNamePart(serverPath)}__${sanitizeToolNamePart(toolName)}` + if (full.length <= MAX_TOOL_NAME_CHARS) return full + return `${full.slice(0, MAX_TOOL_NAME_CHARS - 8)}_${shortHash(full)}` +} + +function evictLoadedTools() { + while (loadedTools.size > MAX_LOADED_TOOLS) { + const oldest = loadedTools.keys().next() + if (oldest.done) return + loadedTools.delete(oldest.value) + } +} + +/** + * Promote remote tools to first-class chat tools, so the model fills a real input + * schema instead of guessing arguments into a free-form object. Returns the names + * it registered, in the order given. + * + * Each tool carries its own `readOnlyHint`, so the confirmation gate is per tool + * and the read/write wrappers' "you used the wrong one" failure cannot arise. + */ +export function registerMcpTools(server: McpServer, tools: McpToolDef[]): string[] { + const names = tools.map((tool) => { + const key = `${server.path}::${tool.name}` + const name = registeredToolName(server.path, tool.name) + const readOnly = isReadOnly(tool) + // Re-registering refreshes the schema and counts as a use. + loadedTools.delete(key) + loadedTools.set(key, { + def: { + type: 'function', + function: { + name, + description: `${tool.description ?? tool.name}\n(MCP server ${server.path})`, + parameters: tool.inputSchema ?? { type: 'object', properties: {} } + } + }, + showDetails: true, + ...(readOnly + ? {} + : { + requiresConfirmation: true, + confirmationMessage: `Call ${tool.name} on ${server.path}` + }), + fn: async ({ args, workspace, toolId, toolCallbacks }) => { + touchLoadedTool(key) + toolCallbacks.setToolStatus(toolId, { + content: `Calling ${tool.name}...`, + mcpServer: server.path + }) + const result = await executeTool(workspace, server, tool, args ?? {}, readOnly) + const ok = JSON.parse(result).success === true + toolCallbacks.setToolStatus(toolId, { + content: ok ? `Called ${tool.name}` : `Call to ${tool.name} failed`, + result, + ...(ok ? {} : { error: `Call to ${tool.name} failed` }) + }) + return result + } + }) + return name + }) + evictLoadedTools() + return names +} + /** * Built per session from the servers the user connected: with none, the tools * are not registered at all, so a workspace without an MCP connection pays no @@ -405,8 +561,12 @@ export function createMcpTools(servers: McpServer[]): Tool<{}>[] { def: createToolDef( searchMcpToolsSchema, 'search_mcp_tools', - 'Search the tools exposed by the MCP servers connected to this workspace (listed in the system prompt). Returns server + tool names to pass to call_mcp_read_tool or call_mcp_write_tool.' + "Search the tools exposed by the MCP servers connected to this workspace (listed in the system prompt). Each match becomes a callable tool of its own, named by `call` in the result, carrying that tool's real argument schema — call it directly rather than guessing arguments." ), + // Openable, so the matches and the `call` names they registered can be read; + // collapsed once it succeeded, like a call result, since a ten-match search + // would otherwise take over the transcript. + showDetails: true, fn: async ({ args, workspace, toolId, toolCallbacks }) => { const parsed = searchMcpToolsSchema.parse(args) toolCallbacks.setToolStatus(toolId, { content: 'Searching MCP tools...' }) @@ -443,8 +603,27 @@ export function createMcpTools(servers: McpServer[]): Tool<{}>[] { return result } const top = scored.slice(0, MAX_SEARCH_RESULTS) + // Register the matches, so the next iteration puts each one's real input + // schema in front of the model instead of leaving it to guess arguments + // into `call_mcp_*`'s free-form object. Bounded by MAX_SEARCH_RESULTS, and + // paid only after a search actually matched. + const callNames = new Map() + const perServerMatches = new Map() + for (const match of top) { + const entry = perServerMatches.get(match.server.path) ?? { + server: match.server, + tools: [] + } + entry.tools.push(match.tool) + perServerMatches.set(match.server.path, entry) + } + for (const { server, tools } of perServerMatches.values()) { + const registered = registerMcpTools(server, tools) + tools.forEach((tool, i) => callNames.set(tool, registered[i])) + } const result = boundedSearch({ - matches: top.map((s) => summarizeTool(s.server, s.tool)), + matches: top.map((s) => summarizeTool(s.server, s.tool, callNames.get(s.tool))), + hint: 'Each match is now a tool of its own, named by `call`. Call that tool directly with its own arguments — it carries the real schema. `call_mcp_read_tool` / `call_mcp_write_tool` are only for a tool that has no `call`.', ...(scored.length > top.length ? { note: `${scored.length - top.length} more match(es) — refine the query to see them.` diff --git a/frontend/src/lib/components/copilot/chat/shared.ts b/frontend/src/lib/components/copilot/chat/shared.ts index 6d6794b56c..c66178cb62 100644 --- a/frontend/src/lib/components/copilot/chat/shared.ts +++ b/frontend/src/lib/components/copilot/chat/shared.ts @@ -575,6 +575,10 @@ export type ToolDisplayMessage = { autoCollapseDetails?: boolean isStreamingArguments?: boolean toolName?: string + /** Path of the MCP server this call reached, which marks the row with its provider. + * Recorded here rather than looked up from the tool name: the registry of loaded + * remote tools lives only in memory, so a reloaded transcript could not resolve it. */ + mcpServer?: string showFade?: boolean actions?: ToolDisplayAction[] userQuestion?: UserQuestionDisplay diff --git a/frontend/src/lib/components/mcp/McpServerIcon.svelte b/frontend/src/lib/components/mcp/McpServerIcon.svelte new file mode 100644 index 0000000000..88ea3c762a --- /dev/null +++ b/frontend/src/lib/components/mcp/McpServerIcon.svelte @@ -0,0 +1,35 @@ + + +{#if icon} + {@const Icon = icon} + +{:else if host && failedFor !== host} + (failedFor = host)} + /> +{:else} + +{/if} diff --git a/frontend/src/lib/components/mcp/iconCache.ts b/frontend/src/lib/components/mcp/iconCache.ts index e3c0f592e2..04f3b99991 100644 --- a/frontend/src/lib/components/mcp/iconCache.ts +++ b/frontend/src/lib/components/mcp/iconCache.ts @@ -9,7 +9,10 @@ import { providerKey } from './providerIcon' * open. `edited_at` comes back with the list, so a row that has not been edited * since it was cached needs no read at all. */ -type Entry = { key: string | null; editedAt?: string } +// `host` backs the favicon fallback for a server Windmill ships no icon for. It is +// cached alongside the key for the same reason: the url is only in the resource +// value, which the list endpoint strips. +type Entry = { key: string | null; editedAt?: string; host?: string } const STORE_KEY = 'mcp_provider_icons' @@ -33,6 +36,42 @@ export function cachedProviderKey( return entry.editedAt === editedAt ? entry.key : undefined } +/** + * The stored mark for a path, ignoring `editedAt`. A transcript row marks a call + * that already happened, so the provider from before a reconnect is still the one + * to draw — and unlike `readOnlyHint`, nothing acts on it. + */ +export function cachedProviderMark( + workspace: string, + path: string +): { key: string | null; host?: string } | undefined { + const entry = read()[workspace]?.[path] + return entry ? { key: entry.key, host: entry.host } : undefined +} + +/** The url's host, when it has one worth drawing a favicon for. */ +export function providerHost(url: unknown): string | undefined { + if (typeof url !== 'string') return undefined + try { + const { hostname } = new URL(url) + // A loopback, a bare address, or an intranet single-label name has no favicon + // to fetch, and asking would disclose it to the favicon service for nothing. + if (!hostname.includes('.') || /^[\d.]+$/.test(hostname)) return undefined + return hostname + } catch { + return undefined + } +} + +export function cachedProviderHost( + workspace: string, + path: string, + editedAt?: string +): string | undefined { + const entry = read()[workspace]?.[path] + return entry?.editedAt === editedAt ? entry?.host : undefined +} + export function rememberProviderKey( workspace: string, path: string, @@ -41,7 +80,10 @@ export function rememberProviderKey( ): string | null { const key = providerKey(url) ?? null const store = read() - store[workspace] = { ...(store[workspace] ?? {}), [path]: { key, editedAt } } + store[workspace] = { + ...(store[workspace] ?? {}), + [path]: { key, editedAt, host: providerHost(url) } + } try { localStorage.setItem(STORE_KEY, JSON.stringify(store)) } catch {} diff --git a/frontend/src/lib/components/mcp/mcpMenu.svelte.ts b/frontend/src/lib/components/mcp/mcpMenu.svelte.ts index 847c03e61a..50d7e1dad6 100644 --- a/frontend/src/lib/components/mcp/mcpMenu.svelte.ts +++ b/frontend/src/lib/components/mcp/mcpMenu.svelte.ts @@ -1,4 +1,4 @@ -import { List, Plug, Plus } from 'lucide-svelte' +import { List, Plus } from 'lucide-svelte' import type { Component } from 'svelte' import { get } from 'svelte/store' import { ResourceService } from '$lib/gen' @@ -7,14 +7,16 @@ import { sendUserToast } from '$lib/toast' import type { Item } from '$lib/utils' import type { AIChatManager } from '../copilot/chat/AIChatManager.svelte' import { isMcpEnabled, setMcpEnabled } from './enabledServers' -import { cachedProviderKey, rememberProviderKey } from './iconCache' +import { cachedProviderHost, cachedProviderKey, rememberProviderKey } from './iconCache' import { loadProviderIcon } from './providerIcon' +import McpServerIcon from './McpServerIcon.svelte' type Row = { path: string editedAt?: string enabled: boolean icon?: Component + iconHost?: string } // A menu is a shortcut, not a directory: past this many the list stops being @@ -103,6 +105,7 @@ export class McpMenu { const icon = await loadProviderIcon(key) if (seq !== this.#seq) return server.icon = icon + server.iconHost = cachedProviderHost(ws, server.path, server.editedAt) }) ) } @@ -168,15 +171,11 @@ export class McpMenu { // to read through the live list rather than the row captured here, since // a reload replaces every row object and a getter bound to the old one // would go on reporting the state it was built with. - get icon() { - // Plug where the provider is unknown, so one nameless server does not - // pull its label out of line with the rest. - return row(path)?.icon ?? Plug - }, - // Provider icons take css lengths and ignore lucide's `size`, so without - // this one of them renders at its 24px default among 14px menu icons. + // One component for every row, so a shipped icon, a favicon and the plug + // all land at the same size and nothing pulls its label out of line. + icon: McpServerIcon, get iconProps() { - return row(path)?.icon ? { width: '14px', height: '14px' } : undefined + return { icon: row(path)?.icon, host: row(path)?.iconHost, size: 14 } }, get toggle() { return row(path)?.enabled ?? false diff --git a/frontend/src/lib/components/mcp/serverMark.ts b/frontend/src/lib/components/mcp/serverMark.ts new file mode 100644 index 0000000000..05d3033251 --- /dev/null +++ b/frontend/src/lib/components/mcp/serverMark.ts @@ -0,0 +1,37 @@ +import type { Component } from 'svelte' +import { ResourceService } from '$lib/gen' +import { cachedProviderMark, providerHost } from './iconCache' +import { loadProviderIcon, providerKey } from './providerIcon' + +/** What identifies a connected server visually: Windmill's icon for that integration + * if it ships one, otherwise the host its favicon can be fetched from. */ +export type McpServerMark = { icon?: Component; host?: string } + +// One resolution per server per session, shared by every transcript row naming it — +// a chat can hold dozens of calls against the same server. +const marks = new Map>() + +export function resolveMcpServerMark(workspace: string, path: string): Promise { + const key = `${workspace}:${path}` + let pending = marks.get(key) + if (!pending) { + pending = load(workspace, path) + marks.set(key, pending) + } + return pending +} + +async function load(workspace: string, path: string): Promise { + const cached = cachedProviderMark(workspace, path) + if (cached) return { icon: await loadProviderIcon(cached.key), host: cached.host } + try { + // Deliberately not written back to the shared cache: that entry is keyed by + // `editedAt` for the server list's sake, and storing one from here — where the + // row is a past call and `editedAt` is unknown — would make every list re-read. + const resource = await ResourceService.getResource({ workspace, path }) + const url = (resource.value as { url?: unknown } | undefined)?.url + return { icon: await loadProviderIcon(providerKey(url)), host: providerHost(url) } + } catch { + return {} + } +} diff --git a/frontend/src/lib/utils/faviconUrl.ts b/frontend/src/lib/utils/faviconUrl.ts new file mode 100644 index 0000000000..3124450e66 --- /dev/null +++ b/frontend/src/lib/utils/faviconUrl.ts @@ -0,0 +1,15 @@ +/** + * A hostname's favicon, from Google's public favicon service. + * + * This discloses each consulted hostname to a third party from the user's + * browser — an accepted tradeoff, and blocked or air-gapped environments must + * degrade to a local icon through the caller's `onerror`. + * + * Hit gstatic directly rather than `www.google.com/s2/favicons`: the app is + * served with COEP require-corp, and the s2 redirect hop carries no + * Cross-Origin-Resource-Policy header, so the browser blocks the image. The + * gstatic endpoint itself responds with CORP: cross-origin. + */ +export function faviconUrl(hostname: string, size = 64): string { + return `https://t3.gstatic.com/faviconV2?client=SOCIAL&type=FAVICON&fallback_opts=TYPE,SIZE,URL&url=https://${encodeURIComponent(hostname)}&size=${size}` +}