diff --git a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte index 1637165f87..0ab2812b2e 100644 --- a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte +++ b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte @@ -20,6 +20,7 @@ switch that decides whether this chat carries its tools. 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' @@ -29,6 +30,7 @@ switch that decides whether this chat carries its tools. import { untrack } from 'svelte' import { getAiChatManager } from './aiChatManagerContext' import { clearMcpToolsCache } from './global/mcpTools' + import { forgetMcpServerMarks } from '$lib/components/mcp/serverMark' let { ws, @@ -389,8 +391,10 @@ switch that decides whether this chat carries its tools. async function refresh(target = ws) { // A path can be reconnected to a different server, so the cached tool list - // (and the readOnlyHint the confirmation gate reads) must not survive. + // (and the readOnlyHint the confirmation gate reads) must not survive — nor the + // provider mark the transcript's call rows show. clearMcpToolsCache() + forgetMcpServerMarks() await loadServers(target) // `refreshMcpServers` blanks the list when the workspace it is handed is not // the one the chat is on, so a refresh landing after a switch would take B's @@ -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/ToolExecutionDisplay.svelte b/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte index f5ae22a332..620357ff87 100644 --- a/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte @@ -40,6 +40,8 @@ import RunScriptCard from './RunScriptCard.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' interface Props { message: ToolDisplayMessage @@ -47,6 +49,11 @@ let { message }: Props = $props() + // Recorded by the call itself, from the connected-server list rather than from the + // model's arguments — which is what lets a reloaded transcript still resolve it, and + // what keeps a path the model made up from being read as a workspace resource. + const mcpServer = $derived(message.mcpServer) + const isPlanReview = $derived(message.toolName === EXIT_PLAN_MODE_TOOL) const isPlanCard = $derived(isPlanCardTool(message.toolName)) const planCopy = $derived( @@ -242,6 +249,17 @@ {/if} {/snippet} + + {#snippet serverMark()} + {#if mcpServer?.workspace && mcpServer.path} + {#await resolveMcpServerMark(mcpServer.workspace, mcpServer.path) then mark} + + {/await} + {/if} + {/snippet} + @@ -258,6 +276,7 @@ labelClass={showPreviewChip ? 'truncate' : ''} contentClass="space-y-3" headerRight={showPreviewChip ? previewChip : undefined} + headerLeft={mcpServer?.workspace ? serverMark : undefined} > {#snippet belowHeader()} 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..e5d3af8b36 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts @@ -228,7 +228,7 @@ describe('call results', () => { }) describe('search_mcp_tools', () => { - it('returns compact summaries without the full input schemas', async () => { + it('returns each match with its full input schema', async () => { const result = await run('search_mcp_tools', { query: 'issue' }) expect(result.matches).toEqual([ { @@ -236,7 +236,7 @@ describe('search_mcp_tools', () => { tool: 'get_issue', description: 'Get details of a GitHub issue', mode: 'read', - params: ['owner', 'repo'] + inputSchema: TOOLS[0].inputSchema } ]) }) @@ -330,6 +330,64 @@ describe('result size cap', () => { expect(callMcpToolMock).not.toHaveBeenCalled() }) + it('drops the lowest-ranked schemas before dropping the matches themselves', async () => { + getMcpToolsMock.mockResolvedValue( + ['issue_a', 'issue_b', 'issue_c'].map((name) => ({ + name, + description: 'An issue tool', + inputSchema: { + type: 'object', + properties: { body: { type: 'string', description: 'x'.repeat(9_000) } } + }, + annotations: { readOnlyHint: true } + })) + ) + const result = await run('search_mcp_tools', { query: 'issue' }) + + expect(result.matches.map((m: any) => m.tool)).toEqual(['issue_a', 'issue_b', 'issue_c']) + expect(result.matches[0].inputSchema).toBeDefined() + expect(result.matches[2].inputSchema).toBeUndefined() + expect(result.truncated).toBe(true) + }) + + // Truncating must not swallow the count of matches the score cut off: those + // tools are not in the result at all, so the model has to know to ask again. + it('keeps the more-matches hint when truncation strips schemas', async () => { + getMcpToolsMock.mockResolvedValue( + Array.from({ length: 12 }, (_, i) => ({ + name: `issue_${i}`, + description: 'An issue tool', + inputSchema: { + type: 'object', + properties: { body: { type: 'string', description: 'x'.repeat(3_000) } } + }, + annotations: { readOnlyHint: true } + })) + ) + const result = await run('search_mcp_tools', { query: 'issue' }) + + expect(result.matches).toHaveLength(10) + expect(result.note).toContain('2 more match(es)') + expect(result.note).toContain('inputSchema') + }) + + it('holds the cap when every server failed and nothing matched', async () => { + getMcpToolsMock.mockRejectedValue(new Error('x'.repeat(500))) + const servers = Array.from({ length: 50 }, (_, i) => ({ path: `u/hugo/mcp_${i}` })) + const raw = await createMcpTools(servers) + .find((t) => t.def.function.name === 'search_mcp_tools')! + .fn({ + args: { query: 'issue' }, + workspace: 'test-ws', + helpers: {}, + toolCallbacks: createToolCallbacks(), + toolId: 'tool-1' + }) + + expect(raw.length).toBeLessThanOrEqual(20_000) + expect(JSON.parse(raw).unavailableCount).toBe(50) + }) + it('truncates an oversized tools/list failure in search', async () => { getMcpToolsMock.mockRejectedValue(new Error('x'.repeat(80_000))) const result = await run('search_mcp_tools', { query: 'issue' }) diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index d5f16d5c54..6fd59e4a07 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -8,8 +8,7 @@ import { enabledMcpPaths } from '$lib/components/mcp/enabledServers' * 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. + * the tools a search matched enter the model's context, with their schemas. */ type McpToolDef = GetMcpToolsResponse[number] @@ -17,7 +16,7 @@ type McpToolDef = GetMcpToolsResponse[number] export type McpServer = { path: string; editedAt?: string } const MAX_SEARCH_RESULTS = 10 -const MAX_DESCRIPTION_CHARS = 200 +const MAX_DESCRIPTION_CHARS = 1_000 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. @@ -139,24 +138,17 @@ function isReadOnly(tool: McpToolDef): boolean { return tool.annotations?.readOnlyHint === true } -function schemaPropertyNames(schema: unknown): string[] { - const properties = (schema as { properties?: Record } | null | undefined) - ?.properties - return properties ? Object.keys(properties) : [] -} - function truncate(text: string, max: number): string { return text.length > max ? text.slice(0, max) + '…' : text } function summarizeTool(server: McpServer, tool: McpToolDef) { - const params = schemaPropertyNames(tool.inputSchema) return { server: server.path, tool: tool.name, description: truncate(tool.description ?? '', MAX_DESCRIPTION_CHARS), mode: isReadOnly(tool) ? 'read' : 'write', - ...(params.length > 0 ? { params } : {}) + inputSchema: tool.inputSchema } } @@ -285,28 +277,44 @@ function bounded(payload: { } /** - * The search payload under the same ceiling as a call result. Server error text - * goes first: the matches are what the model asked for. + * The search payload under the same ceiling as a call result. Schemas go before + * matches: a tool listed without its schema is still callable, since a rejected + * call returns the schema, while a tool dropped from the list is not. */ function boundedSearch(payload: { matches: unknown[]; [k: string]: unknown }): string { - const { unavailable, ...rest } = payload - const dropped = Array.isArray(unavailable) ? { unavailableCount: unavailable.length } : {} - let out = JSON.stringify(payload, null, 2) + let out = JSON.stringify(payload) if (out.length <= MAX_RESULT_CHARS) return out - const matches = [...payload.matches] - const build = () => - JSON.stringify( - { - ...rest, - ...dropped, - matches, - truncated: true, - note: `Truncated to ${MAX_RESULT_CHARS} characters. Refine the query.` - }, - null, - 2 - ) + + // The caller's note says how many matches the score cut off, which truncating + // only adds to. + const { unavailable, note: scoreNote, ...rest } = payload + const dropped = Array.isArray(unavailable) ? { unavailableCount: unavailable.length } : {} + const found = payload.matches.length + const matches = payload.matches.map((m) => ({ ...(m as object) })) as Record[] + const build = () => { + const schemaless = matches.filter((m) => !m.inputSchema).length + const note = typeof scoreNote === 'string' ? [scoreNote] : [] + note.push(`Truncated to ${MAX_RESULT_CHARS} characters.`) + if (schemaless > 0) { + note.push( + `${schemaless} lower-ranked match(es) are listed without their inputSchema; calling one returns the schema if the arguments are wrong.` + ) + } + if (matches.length < found) { + note.push(`${found - matches.length} match(es) dropped — refine the query to see them.`) + } + return JSON.stringify({ ...rest, ...dropped, matches, truncated: true, note: note.join(' ') }) + } + // Dropping the server error text is the first reduction, and the only one left + // when nothing matched. out = build() + if (out.length <= MAX_RESULT_CHARS) return out + // `matches` is ordered by score, so the tail is what the query matched least. + for (let i = matches.length - 1; i >= 0; i--) { + delete matches[i].inputSchema + out = build() + if (out.length <= MAX_RESULT_CHARS) return out + } while (out.length > MAX_RESULT_CHARS && matches.length > 0) { matches.pop() out = build() @@ -353,6 +361,14 @@ function createCallTool(servers: McpServer[], mode: 'read' | 'write'): Tool<{}> }), fn: async ({ args, workspace, toolId, toolCallbacks }) => { const parsed = callMcpToolSchema.parse(args) + // Marks the row with the provider it reaches. Recorded before the listing is + // awaited, so an unreachable server still marks its own failure row, and taken + // from the connected list rather than from `parsed.server`, which is a path the + // model could point anywhere. + const named = servers.find((s) => s.path === parsed.server) + if (named) { + toolCallbacks.setToolStatus(toolId, { mcpServer: { workspace, path: named.path } }) + } // 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> @@ -405,7 +421,7 @@ 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). Returns server + tool names to pass to call_mcp_read_tool or call_mcp_write_tool, each with the input schema its arguments must follow.' ), fn: async ({ args, workspace, toolId, toolCallbacks }) => { const parsed = searchMcpToolsSchema.parse(args) diff --git a/frontend/src/lib/components/copilot/chat/shared.ts b/frontend/src/lib/components/copilot/chat/shared.ts index adb0f63be4..bc00e415f6 100644 --- a/frontend/src/lib/components/copilot/chat/shared.ts +++ b/frontend/src/lib/components/copilot/chat/shared.ts @@ -629,6 +629,11 @@ export type ToolDisplayMessage = { autoCollapseDetails?: boolean isStreamingArguments?: boolean toolName?: string + /** What marks this row with its provider. Recorded rather than looked up, because the + * server listing lives only in memory and a reloaded transcript does not have it. The + * workspace rides along: a chat is readable from any workspace, and the same path names + * a different server in each. */ + mcpServer?: { workspace: string; path: 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..b3d005ad7f --- /dev/null +++ b/frontend/src/lib/components/mcp/McpServerIcon.svelte @@ -0,0 +1,29 @@ + + + + diff --git a/frontend/src/lib/components/mcp/iconCache.ts b/frontend/src/lib/components/mcp/iconCache.ts index e3c0f592e2..59ba63adfb 100644 --- a/frontend/src/lib/components/mcp/iconCache.ts +++ b/frontend/src/lib/components/mcp/iconCache.ts @@ -33,6 +33,18 @@ export function cachedProviderKey( return entry.editedAt === editedAt ? entry.key : undefined } +/** + * The stored mark for a path, ignoring `editedAt`: a transcript row has none to + * match against, and nothing acts on a mark. + */ +export function cachedProviderMark( + workspace: string, + path: string +): { key: string | null } | undefined { + const entry = read()[workspace]?.[path] + return entry ? { key: entry.key } : undefined +} + export function rememberProviderKey( workspace: string, path: string, diff --git a/frontend/src/lib/components/mcp/mcpMenu.svelte.ts b/frontend/src/lib/components/mcp/mcpMenu.svelte.ts index 847c03e61a..aea4a7b114 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' @@ -9,6 +9,7 @@ import type { AIChatManager } from '../copilot/chat/AIChatManager.svelte' import { isMcpEnabled, setMcpEnabled } from './enabledServers' import { cachedProviderKey, rememberProviderKey } from './iconCache' import { loadProviderIcon } from './providerIcon' +import McpServerIcon from './McpServerIcon.svelte' type Row = { path: string @@ -164,19 +165,13 @@ export class McpMenu { return [ ...shown.map(({ path }) => ({ displayName: path, + icon: McpServerIcon, // Getters, not snapshots: the menu stays open across a click, and it has // 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. get iconProps() { - return row(path)?.icon ? { width: '14px', height: '14px' } : undefined + return { icon: row(path)?.icon, size: 14 } }, get toggle() { return row(path)?.enabled ?? false diff --git a/frontend/src/lib/components/mcp/serverMark.test.ts b/frontend/src/lib/components/mcp/serverMark.test.ts new file mode 100644 index 0000000000..7e34d27284 --- /dev/null +++ b/frontend/src/lib/components/mcp/serverMark.test.ts @@ -0,0 +1,45 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { getResourceMock } = vi.hoisted(() => ({ getResourceMock: vi.fn() })) + +vi.mock('$lib/gen', () => ({ ResourceService: { getResource: getResourceMock } })) +vi.mock('./iconCache', () => ({ cachedProviderMark: () => undefined })) +vi.mock('./providerIcon', () => ({ + loadProviderIcon: async (key: string | null) => (key ? `icon:${key}` : undefined), + providerKey: (url: unknown) => + typeof url === 'string' && url.includes('linear') ? 'linear' : null +})) + +import { forgetMcpServerMarks, resolveMcpServerMark } from './serverMark' + +describe('resolveMcpServerMark', () => { + beforeEach(() => { + forgetMcpServerMarks() + getResourceMock.mockReset() + }) + + it('reads a server once and shares the answer', async () => { + getResourceMock.mockResolvedValue({ value: { url: 'https://mcp.linear.app/mcp' } }) + + const marks = await Promise.all([ + resolveMcpServerMark('ws', 'u/admin/linear_mcp'), + resolveMcpServerMark('ws', 'u/admin/linear_mcp') + ]) + + expect(marks.map((m) => m.icon)).toEqual(['icon:linear', 'icon:linear']) + expect(getResourceMock).toHaveBeenCalledTimes(1) + }) + + // A failure must not be memoized: one offline blip would otherwise leave the + // server unmarked in every later row until the page reloads. + it('retries after a failed read', async () => { + getResourceMock.mockRejectedValueOnce(new Error('offline')) + expect(await resolveMcpServerMark('ws', 'u/admin/linear_mcp')).toEqual({}) + + getResourceMock.mockResolvedValue({ value: { url: 'https://mcp.linear.app/mcp' } }) + const mark = await resolveMcpServerMark('ws', 'u/admin/linear_mcp') + + expect(mark.icon).toBe('icon:linear') + expect(getResourceMock).toHaveBeenCalledTimes(2) + }) +}) diff --git a/frontend/src/lib/components/mcp/serverMark.ts b/frontend/src/lib/components/mcp/serverMark.ts new file mode 100644 index 0000000000..c728abdead --- /dev/null +++ b/frontend/src/lib/components/mcp/serverMark.ts @@ -0,0 +1,47 @@ +import type { Component } from 'svelte' +import { ResourceService } from '$lib/gen' +import { cachedProviderMark } from './iconCache' +import { loadProviderIcon, providerKey } from './providerIcon' + +/** Windmill's own icon for a connected server's integration, when it ships one. */ +export type McpServerMark = { icon?: Component } + +// 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) { + // A failed read is dropped rather than memoized: one offline blip or a 401 during + // a token refresh would otherwise leave the server unmarked in every later row + // until the page reloads. + pending = load(workspace, path).catch(() => { + marks.delete(key) + return {} + }) + marks.set(key, pending) + } + return pending +} + +/** + * Forget what was resolved, for the settings section to call when it reloads the + * connections: a path can be reconnected to a different provider, and a mark held for + * the life of the page would go on marking new call rows with the old provider's icon. + */ +export function forgetMcpServerMarks() { + marks.clear() +} + +async function load(workspace: string, path: string): Promise { + const cached = cachedProviderMark(workspace, path) + if (cached) return { icon: await loadProviderIcon(cached.key) } + // 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)) } +}