diff --git a/backend/windmill-mcp/src/common/schema.rs b/backend/windmill-mcp/src/common/schema.rs index f7417ec752..aa4aec1d8b 100644 --- a/backend/windmill-mcp/src/common/schema.rs +++ b/backend/windmill-mcp/src/common/schema.rs @@ -264,11 +264,8 @@ pub fn make_schema_compatible(schema: &mut Value) { obj.insert("type".to_string(), Value::String("string".to_string())); } - // 2c. `required` is `uniqueItems` at every subschema, not just the root, so a - // repeat nested inside a property makes a strict validator reject the whole tool - // just as a root-level one does -- and the tool then vanishes from the client's - // list rather than failing loudly. `transform_property_keys` covers only the root, - // where it also has to follow key renames. + // 2c. See the `required` bullet above. `transform_property_keys` covers only the + // root, where it also has to follow key renames. if let Some(Value::Array(required)) = obj.get_mut("required") { let mut seen = HashSet::new(); required.retain(|name| match name.as_str() { @@ -892,9 +889,9 @@ mod tests { #[test] fn enriched_resource_stays_a_string_through_make_schema_compatible() { // A resource param is stored with the resource's own object shape. Enrichment - // retypes it to the `$res:` string, and the leftover `properties` used to make - // `make_schema_compatible` retype it back to "object" with nothing in it -- - // the model then sent `{}` instead of a resource path. + // retypes it to the `$res:` string, and must clear that shape: a node with + // `properties` is retyped back to "object" below, which reads to the model as + // an empty object rather than a resource path. let (cache, types) = aws_resources(); let mut node = json!({ "type": "object", diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index e3b1c68315..b862bc18b2 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -41,8 +41,7 @@ const MAX_TOOL_SCHEMA_CHARS = 8_000 // And a ceiling on the set, since the per-tool bound alone would allow 25 large ones — // far more context than the search indirection exists to save. const MAX_LOADED_SCHEMA_CHARS = 40_000 -// Same reasoning for the description, which rides alongside it. Roomier than the -// search summary's 200: this one is what the model chooses the tool from. +// Roomier than the search summary's 200: this is what the model chooses the tool from. const MAX_TOOL_DESCRIPTION_CHARS = 2_000 const MAX_RESULT_CHARS = 20_000 // A server writes its own error text, and every enabled server can contribute @@ -109,15 +108,11 @@ export function clearMcpToolsCache() { */ type OwnerRegistry = { tools: Map> - /** - * Recency for eviction, kept beside the map rather than as its order. The emitted + /** Recency for eviction, kept beside the map rather than as its order: the emitted * tool list carries Anthropic's `cache_control` breakpoint on its last entry, so - * reordering it on every call would invalidate the cached prefix — system prompt, - * skills and transcript — for the rest of the turn. - */ + * reordering it would invalidate the cached prefix for the rest of the turn. */ lastUsed: Map - /** The server revision each entry's frozen schema came from, so a connection edited - * elsewhere can be told apart from the same path still pointing at the same server. */ + /** The server revision each entry was frozen at (see `loadedMcpServers`). */ editedAt: Map counter: number } @@ -179,10 +174,10 @@ export function loadedMcpTools(owner: string): Tool<{}>[] { } /** - * The servers that currently have a tool registered, each with the revision its schemas - * were frozen from, for reconciling against the live list. The revision matters as much - * as the path: a registered call bypasses the listing cache, so a connection edited - * elsewhere would otherwise keep running against the schema it had before the edit. + * 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 + * registered call bypasses the listing cache, so a connection edited elsewhere would + * keep running against the schema it had before the edit. */ export function loadedMcpServers(owner: string): { path: string; editedAt?: string }[] { const registry = registries.get(owner) @@ -224,6 +219,7 @@ export function forgetLoadedMcpTools(owner: string, serverPath?: string) { if (key.startsWith(prefix)) { registry.tools.delete(key) registry.lastUsed.delete(key) + registry.editedAt.delete(key) } } if (registry.tools.size === 0) registries.delete(owner) @@ -731,9 +727,6 @@ export function registerMcpTools( type: 'function', function: { name, - // Bounded like the schema above and like every other payload the server - // controls: this rides in the request on every iteration, for up to - // MAX_LOADED_TOOLS tools, and servers do ship multi-KB descriptions. description: `${truncate(tool.description ?? tool.name, MAX_TOOL_DESCRIPTION_CHARS)}\n(MCP server ${server.path})`, parameters } @@ -854,7 +847,7 @@ export function createMcpTools(owner: string, servers: McpServer[]): Tool<{}>[] } const result = boundedSearch({ 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`.', + hint: 'Call each `call` name directly; use the wrappers only for a match without one.', ...(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 57d116c765..1cb0e1e483 100644 --- a/frontend/src/lib/components/copilot/chat/shared.ts +++ b/frontend/src/lib/components/copilot/chat/shared.ts @@ -576,13 +576,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. */ + /** What marks this row with its provider. Recorded rather than looked up, because + * both sources — the loaded-tool registry and the server listing — live only in + * memory, and a reloaded transcript has neither. */ mcpServer?: string - /** Icon the MCP server published for the tool this row called (`icons`, per the - * spec), already validated. Recorded here for the same reason as `mcpServer`: the - * listing it came from is not available to a reloaded transcript. */ mcpIconSrc?: string showFade?: boolean actions?: ToolDisplayAction[] diff --git a/frontend/src/lib/components/mcp/mcpIcon.ts b/frontend/src/lib/components/mcp/mcpIcon.ts index 35df32ea13..9d3d08cdbd 100644 --- a/frontend/src/lib/components/mcp/mcpIcon.ts +++ b/frontend/src/lib/components/mcp/mcpIcon.ts @@ -1,5 +1,6 @@ -/** One entry of an MCP `icons` array, as the spec defines it. */ -export type McpIcon = { +/** One entry of an MCP `icons` array, as the spec defines it. Not `McpIcon`: that + * names the Windmill-shipped MCP logo component in `$lib/components/icons`. */ +export type PublishedIcon = { src?: unknown mimeType?: unknown sizes?: unknown @@ -12,26 +13,20 @@ export type McpIcon = { const MAX_ICON_SRC_CHARS = 8_000 /** - * The icon source to render, chosen from what an MCP server published. + * The icon source to render, chosen from what an MCP server published. `src` comes + * from a third party, so the spec's rules are enforced here rather than trusted. * - * **`data:` only.** The spec also permits an HTTPS src, but rendering one makes the - * user's browser fetch from a host the server names, disclosing their IP, user agent - * and the moment they looked — and a per-user URL turns it into a read receipt. COEP - * `require-corp` does not prevent that: it blocks the *response*, so the request has - * already left. A `data:` src carries its bytes over the MCP connection the user - * already made and fetches nothing. - * - * The rest is the spec's own list, enforced rather than trusted, since `src` is chosen - * by a third party: no SVG (it can carry script), and a data URI judged by the type it - * declares rather than the sibling `mimeType`, which the spec calls advisory. + * `data:` only: an HTTPS src would make the browser fetch from a host the server + * names, and COEP blocks the response, not the request. No SVG — it can carry script. + * A data URI is judged by the type it declares; `mimeType` is advisory. */ export function pickMcpIconSrc(icons: unknown): string | undefined { if (!Array.isArray(icons)) return undefined - return icons.find((icon): icon is McpIcon => isRenderable(icon))?.src as string | undefined + return icons.find((icon): icon is PublishedIcon => isRenderable(icon))?.src as string | undefined } function isRenderable(icon: unknown): boolean { - const src = (icon as McpIcon | null)?.src + const src = (icon as PublishedIcon | null)?.src if (typeof src !== 'string' || src.length > MAX_ICON_SRC_CHARS) return false const lower = src.toLowerCase() if (!lower.startsWith('data:image/')) return false diff --git a/frontend/src/lib/components/mcp/registry.ts b/frontend/src/lib/components/mcp/registry.ts index 2b305c2a37..a9bacce3e0 100644 --- a/frontend/src/lib/components/mcp/registry.ts +++ b/frontend/src/lib/components/mcp/registry.ts @@ -68,7 +68,8 @@ export const MCP_REGISTRY: McpRegistryEntry[] = [ icon: LinearIcon, url: 'https://mcp.linear.app/mcp', auth: 'dcr', - tokenHint: 'Use a Linear API key. The Read permission is enough for the read tools.', + tokenHint: + 'Use a Linear API key. The Read permission is enough for the read tools.', docsUrl: 'https://linear.app/docs/mcp' }, { diff --git a/frontend/src/lib/components/mcp/secretVariable.test.ts b/frontend/src/lib/components/mcp/secretVariable.test.ts index 86fa5d8675..ced0f8e611 100644 --- a/frontend/src/lib/components/mcp/secretVariable.test.ts +++ b/frontend/src/lib/components/mcp/secretVariable.test.ts @@ -1,20 +1,14 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { - existsVariable, - getVariable, - createVariable, - updateVariable, - deleteVariable, - existsResource -} = vi.hoisted(() => ({ - existsVariable: vi.fn(), - getVariable: vi.fn(), - createVariable: vi.fn(), - updateVariable: vi.fn(), - deleteVariable: vi.fn(), - existsResource: vi.fn() -})) +const { existsVariable, getVariable, createVariable, updateVariable, deleteVariable, existsResource } = + vi.hoisted(() => ({ + existsVariable: vi.fn(), + getVariable: vi.fn(), + createVariable: vi.fn(), + updateVariable: vi.fn(), + deleteVariable: vi.fn(), + existsResource: vi.fn() + })) vi.mock('$lib/gen', () => ({ VariableService: { existsVariable, getVariable, createVariable, updateVariable, deleteVariable },