refactor: state each MCP invariant once and drop the last of the churn

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgzuxyafKNF2uaEeL35XQw

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",

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<string, Tool<{}>>
-	/**
-	 * 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<string, number>
-	/** 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<string, string | undefined>
 	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.`

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[]

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

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'
 	},
 	{

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 },