From f0c949938d9bbfd647cad28e126a06e51a15dcc1 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Mon, 14 Sep 2026 17:20:18 +0200 Subject: [PATCH] fix: name a websearch tool that carries no summary of its own Co-Authored-By: Claude Opus 5 (1M context) --- backend/windmill-worker/src/ai_executor.rs | 55 +++++++++++++++++-- .../components/flows/agentToolUtils.test.ts | 43 +++++++++++++++ .../lib/components/flows/agentToolUtils.ts | 17 ++++-- 3 files changed, 106 insertions(+), 9 deletions(-) create mode 100644 frontend/src/lib/components/flows/agentToolUtils.test.ts diff --git a/backend/windmill-worker/src/ai_executor.rs b/backend/windmill-worker/src/ai_executor.rs index df35a84f31..c58c99db23 100644 --- a/backend/windmill-worker/src/ai_executor.rs +++ b/backend/windmill-worker/src/ai_executor.rs @@ -243,15 +243,29 @@ fn overlay_tool_inputs( } } -/// The name a run enables a roster entry by: the name the model is shown, except for an MCP server, -/// which the model is shown nothing of and which is named by the resource it points at. +/// What a websearch entry is named by when it carries no summary of its own. A summary is a name +/// only for an entry the model is shown; web search reaches the model as a provider capability +/// rather than a tool, so its summary is a label the editor happens to write and JSON authored +/// anywhere else may leave out, and an entry with no name at all could not be enabled. +const WEBSEARCH_ENABLED_NAME: &str = "web_search"; + +/// The name a run enables a roster entry by: the name the model is shown, except for an entry the +/// model is shown nothing of, which is named by whatever identifies it instead. An MCP server is +/// named by the resource it points at, web search by its label or `WEBSEARCH_ENABLED_NAME`. /// -/// The path is bare. The roster stores it as authored, `$res:` and all, but a name is an argument -/// value and one carrying that prefix is resolved to the resource itself before the worker is -/// handed its args, so the prefixed form is not something the list can hold. +/// The MCP path is bare. The roster stores it as authored, `$res:` and all, but a name is an +/// argument value and one carrying that prefix is resolved to the resource itself before the worker +/// is handed its args, so the prefixed form is not something the list can hold. fn tool_enabled_name(tool: &AgentTool) -> Option<&str> { match &tool.value { ToolValue::Mcp(mcp) => Some(mcp.resource_path.trim_start_matches("$res:")), + ToolValue::Websearch(_) => Some( + tool.summary + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) + .unwrap_or(WEBSEARCH_ENABLED_NAME), + ), _ => tool.summary.as_deref(), } } @@ -1962,6 +1976,14 @@ mod tests { }), } } + fn websearch(id: &str, summary: Option<&str>) -> AgentTool { + AgentTool { + id: id.to_string(), + summary: summary.map(str::to_string), + description: None, + value: ToolValue::Websearch(windmill_common::flows::WebsearchToolValue {}), + } + } let roster = || { vec![ named("a", "get_user"), @@ -1972,6 +1994,9 @@ mod tests { let names = |tools: &[AgentTool]| -> Vec { tools.iter().filter_map(|t| t.summary.clone()).collect() }; + let ids = |tools: &[AgentTool]| -> Vec { + tools.iter().map(|t| t.id.clone()).collect() + }; // No list at all: the whole roster, as every agent written before the field expects. assert_eq!( @@ -2011,6 +2036,26 @@ mod tests { ["get_user"] ); assert!(narrow_roster(roster(), Some(&["u/test/other".to_string()])).is_empty()); + + // Web search reaches the model as a provider capability rather than a tool, so its summary + // is a label the editor writes and JSON authored anywhere else may leave out. Without the + // fallback such an entry has no name, and a run that narrows could not keep web search. + let mut with_websearch = roster(); + with_websearch.push(websearch("w", None)); + assert_eq!( + ids(&narrow_roster( + with_websearch, + Some(&[WEBSEARCH_ENABLED_NAME.to_string()]) + )), + ["w"] + ); + // A label of its own still names it, which is what the editor writes. + let mut labelled = roster(); + labelled.push(websearch("w", Some("Web Search"))); + assert_eq!( + ids(&narrow_roster(labelled, Some(&["Web Search".to_string()]))), + ["w"] + ); } #[test] diff --git a/frontend/src/lib/components/flows/agentToolUtils.test.ts b/frontend/src/lib/components/flows/agentToolUtils.test.ts new file mode 100644 index 0000000000..6e6ec8d25e --- /dev/null +++ b/frontend/src/lib/components/flows/agentToolUtils.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it, vi } from 'vitest' + +// `agentToolUtils` reaches the copilot bundle, and Monaco's CSS with it, through this one import. +// Only `createAiAgentTool` reads it, and nothing below does. +vi.mock('../aiProviderStorage', () => ({ loadStoredConfig: () => undefined })) + +import { toolEnabledName, WEBSEARCH_ENABLED_NAME } from './agentToolUtils' + +/** + * The names this returns are the ones `enabled_tools` holds and `tool_enabled_name` in + * `ai_executor.rs` matches against, so the two have to agree: a name only one side produces + * silently drops the tool from every run that narrows. + */ +describe('toolEnabledName', () => { + it('names a flow module tool by the name the model is shown', () => { + expect( + toolEnabledName({ id: 'a', summary: 'get_user', value: { tool_type: 'flowmodule' } } as any) + ).toBe('get_user') + }) + + it('names an MCP server by its bare path, never the summary two servers may share', () => { + // `$res:` and all is how the roster stores it, but a name carrying that prefix is resolved to + // the resource itself before the worker sees it, so the list can only hold the bare path. + expect( + toolEnabledName({ + id: 'm', + summary: 'github', + value: { tool_type: 'mcp', resource_path: '$res:u/admin/gh' } + } as any) + ).toBe('u/admin/gh') + }) + + it('falls back to a constant for web search authored without a label', () => { + // The editor always writes one and offers no way to clear it; JSON authored anywhere else may + // carry none, and an entry with no name could not be enabled at all. + expect(toolEnabledName({ id: 'w', value: { tool_type: 'websearch' } } as any)).toBe( + WEBSEARCH_ENABLED_NAME + ) + expect( + toolEnabledName({ id: 'w', summary: 'Web Search', value: { tool_type: 'websearch' } } as any) + ).toBe('Web Search') + }) +}) diff --git a/frontend/src/lib/components/flows/agentToolUtils.ts b/frontend/src/lib/components/flows/agentToolUtils.ts index 9e2cb89a59..361333d406 100644 --- a/frontend/src/lib/components/flows/agentToolUtils.ts +++ b/frontend/src/lib/components/flows/agentToolUtils.ts @@ -99,11 +99,17 @@ export function toolDisplayName(tool: AgentTool): string | undefined { return tool?.summary || value?.path || value?.resource_path || undefined } -/** The name `enabled_tools` holds a tool by: the name the model is shown, except for an MCP server, - * which the model is shown nothing of and which is named by the resource it points at. Its summary - * is a label two entries may share, so naming one would enable both. +/** What web search is named by with no summary of its own, mirroring `WEBSEARCH_ENABLED_NAME` in + * `ai_executor.rs`. The editor writes a label and offers no way to clear it, but JSON authored + * anywhere else may carry none, and an entry with no name could not be enabled at all. */ +export const WEBSEARCH_ENABLED_NAME = 'web_search' + +/** The name `enabled_tools` holds a tool by: the name the model is shown, except for an entry the + * model is shown nothing of, which is named by whatever identifies it instead. An MCP server is + * named by the resource it points at, since its summary is a label two entries may share and + * naming one would enable both; web search by its label, else `WEBSEARCH_ENABLED_NAME`. * - * The path is offered bare. It is stored with the `$res:` it was authored with, and an + * The MCP path is offered bare. It is stored with the `$res:` it was authored with, and an * `enabled_tools` entry carrying that prefix is resolved to the resource's own value before the * step runs, reaching the worker as an object where a name is expected. Mirrors * `tool_enabled_name` in `ai_executor.rs`. */ @@ -112,6 +118,9 @@ export function toolEnabledName(tool: AgentTool): string | undefined { if (value?.tool_type === 'mcp') { return (value?.resource_path as string | undefined)?.replace(/^\$res:/, '') || undefined } + if (value?.tool_type === 'websearch') { + return tool?.summary?.trim() || WEBSEARCH_ENABLED_NAME + } return toolDisplayName(tool) }