mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix: reserve the name web search is enabled by so no tool can share it
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
f0c949938d
commit
4e3426a0b2
@@ -243,15 +243,19 @@ fn overlay_tool_inputs(
|
||||
}
|
||||
}
|
||||
|
||||
/// 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";
|
||||
/// What every websearch entry is named by, whatever label it carries. Web search reaches the model
|
||||
/// as a provider capability rather than a tool, so it has no model-facing name of its own, and the
|
||||
/// editor's label is not one: a flow module tool could carry the same one, and enabling that tool
|
||||
/// would then silently turn web search on with it.
|
||||
///
|
||||
/// The space is what makes the name unshareable. A flow module tool's name must match
|
||||
/// `TOOL_NAME_REGEX`, and a resource path carries no spaces either, so nothing else in a roster can
|
||||
/// answer to this.
|
||||
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`.
|
||||
/// model is shown nothing of, which cannot be named by a label others may share. An MCP server is
|
||||
/// named by the resource it points at, web search by `WEBSEARCH_ENABLED_NAME`.
|
||||
///
|
||||
/// 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
|
||||
@@ -259,13 +263,7 @@ const WEBSEARCH_ENABLED_NAME: &str = "web_search";
|
||||
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),
|
||||
),
|
||||
ToolValue::Websearch(_) => Some(WEBSEARCH_ENABLED_NAME),
|
||||
_ => tool.summary.as_deref(),
|
||||
}
|
||||
}
|
||||
@@ -1994,9 +1992,8 @@ mod tests {
|
||||
let names = |tools: &[AgentTool]| -> Vec<String> {
|
||||
tools.iter().filter_map(|t| t.summary.clone()).collect()
|
||||
};
|
||||
let ids = |tools: &[AgentTool]| -> Vec<String> {
|
||||
tools.iter().map(|t| t.id.clone()).collect()
|
||||
};
|
||||
let ids =
|
||||
|tools: &[AgentTool]| -> Vec<String> { 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!(
|
||||
@@ -2037,24 +2034,29 @@ mod tests {
|
||||
);
|
||||
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));
|
||||
// Web search reaches the model as a provider capability rather than a tool, so it has no
|
||||
// name of its own and is enabled by a reserved one, whatever label it was authored with.
|
||||
for label in [None, Some("Web Search")] {
|
||||
let mut with_websearch = roster();
|
||||
with_websearch.push(websearch("w", label));
|
||||
assert_eq!(
|
||||
ids(&narrow_roster(
|
||||
with_websearch,
|
||||
Some(&[WEBSEARCH_ENABLED_NAME.to_string()])
|
||||
)),
|
||||
["w"]
|
||||
);
|
||||
}
|
||||
|
||||
// The reserved name is one nothing else in a roster can answer to, so enabling a tool
|
||||
// cannot switch web search on beside it: a tool named after it would be rejected by
|
||||
// `TOOL_NAME_REGEX`, which is what the space is there to stay outside of.
|
||||
assert!(!TOOL_NAME_REGEX.is_match(WEBSEARCH_ENABLED_NAME));
|
||||
let mut collision = vec![named("t", "web_search")];
|
||||
collision.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"]
|
||||
ids(&narrow_roster(collision, Some(&["web_search".to_string()]))),
|
||||
["t"]
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
Generated
+1
-1
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
@@ -121,7 +121,7 @@ export const AGENT_FIELDS: AgentFieldSpec[] = [
|
||||
group: 'tools',
|
||||
label: 'Enabled tools',
|
||||
tooltip:
|
||||
'Which of the agent tools a run carries, so it costs no more than it needs. Selecting none leaves the agent with no tools, and unsetting the field gives it all of them. Set it to an expression to decide per run. An MCP server selected here carries every tool it exposes, which its own include and exclude lists decide.',
|
||||
'Which of the agent tools a run carries, so it costs no more than it needs. Selecting none leaves the agent with no tools, and unsetting the field gives it all of them. Set it to an expression to decide per run, naming each one the way this list does: a tool by its own name, an MCP server by its resource path, and web search by "web search". An MCP server carries every tool it exposes, which its own include and exclude lists decide.',
|
||||
seed: [],
|
||||
defaultHint: 'Default: all of them'
|
||||
},
|
||||
|
||||
@@ -4,7 +4,7 @@ import { describe, expect, it, vi } from 'vitest'
|
||||
// Only `createAiAgentTool` reads it, and nothing below does.
|
||||
vi.mock('../aiProviderStorage', () => ({ loadStoredConfig: () => undefined }))
|
||||
|
||||
import { toolEnabledName, WEBSEARCH_ENABLED_NAME } from './agentToolUtils'
|
||||
import { getToolNameError, toolEnabledName, WEBSEARCH_ENABLED_NAME } from './agentToolUtils'
|
||||
|
||||
/**
|
||||
* The names this returns are the ones `enabled_tools` holds and `tool_enabled_name` in
|
||||
@@ -30,14 +30,20 @@ describe('toolEnabledName', () => {
|
||||
).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.
|
||||
it('names web search by a reserved name, whatever label it carries', () => {
|
||||
// It reaches the model as a provider capability rather than a tool, so the editor's label is
|
||||
// not a name: something else in the roster could carry it and be switched on with it.
|
||||
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')
|
||||
).toBe(WEBSEARCH_ENABLED_NAME)
|
||||
})
|
||||
|
||||
it('reserves that name against every other kind', () => {
|
||||
// A flow module tool cannot be called it, so enabling a tool never enables web search beside
|
||||
// it. `getToolNameError` is the rule that holds, and the space is what stays outside it.
|
||||
expect(getToolNameError(WEBSEARCH_ENABLED_NAME)).toBeDefined()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -99,15 +99,16 @@ export function toolDisplayName(tool: AgentTool): string | undefined {
|
||||
return tool?.summary || value?.path || value?.resource_path || undefined
|
||||
}
|
||||
|
||||
/** 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'
|
||||
/** What every websearch entry is named by, mirroring `WEBSEARCH_ENABLED_NAME` in `ai_executor.rs`.
|
||||
* The space is load-bearing: it is what stops a flow module tool, whose name `getToolNameError`
|
||||
* holds to letters, digits and underscores, from answering to the same name and being switched on
|
||||
* with web search. */
|
||||
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`.
|
||||
* named by the resource it points at, and web search by `WEBSEARCH_ENABLED_NAME`, since either
|
||||
* summary is a label something else may share and naming one would enable both.
|
||||
*
|
||||
* 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
|
||||
@@ -119,7 +120,7 @@ export function toolEnabledName(tool: AgentTool): string | undefined {
|
||||
return (value?.resource_path as string | undefined)?.replace(/^\$res:/, '') || undefined
|
||||
}
|
||||
if (value?.tool_type === 'websearch') {
|
||||
return tool?.summary?.trim() || WEBSEARCH_ENABLED_NAME
|
||||
return WEBSEARCH_ENABLED_NAME
|
||||
}
|
||||
return toolDisplayName(tool)
|
||||
}
|
||||
|
||||
@@ -158,7 +158,7 @@ export const AI_AGENT_SCHEMA: Schema = {
|
||||
enabled_tools: {
|
||||
type: 'array',
|
||||
description:
|
||||
'Which of the agent tools a run may call, named as the model is shown them, except an MCP server, which is named by its resource path and carries every tool it exposes. Unset carries every tool.',
|
||||
'Which of the agent tools a run may call: a tool by the name the model is shown, an MCP server by its resource path, which carries every tool it exposes, and web search by "web search". Unset carries every tool.',
|
||||
items: {
|
||||
type: 'string'
|
||||
}
|
||||
|
||||
@@ -1074,10 +1074,11 @@ components:
|
||||
description: |
|
||||
Array of strings naming which of the tools configured in `tools` the agent may call
|
||||
this run. Leaving it unset carries every one of them; an empty array carries none.
|
||||
A tool is named as the model is shown it; an MCP server, which the model is shown
|
||||
nothing of, is named by its resource path and carries every tool it exposes (which
|
||||
of them it exposes stays that entry's include_tools/exclude_tools).
|
||||
Example: ['get_user', 'u/admin/github_mcp']
|
||||
A tool is named as the model is shown it. An entry the model is shown nothing of is
|
||||
named by what identifies it instead: an MCP server by its resource path, carrying
|
||||
every tool it exposes (which of them stays that entry's include_tools/exclude_tools),
|
||||
and a websearch entry by the reserved name 'web search', whatever summary it carries.
|
||||
Example: ['get_user', 'u/admin/github_mcp', 'web search']
|
||||
max_completion_tokens:
|
||||
allOf:
|
||||
- $ref: '#/components/schemas/InputTransform'
|
||||
|
||||
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
Reference in New Issue
Block a user