diff --git a/backend/windmill-worker/src/ai_executor.rs b/backend/windmill-worker/src/ai_executor.rs index 456a34fcb0..a995864bf9 100644 --- a/backend/windmill-worker/src/ai_executor.rs +++ b/backend/windmill-worker/src/ai_executor.rs @@ -276,16 +276,12 @@ fn narrow_roster( .is_some_and(|s| enabled.iter().any(|n| n == s)); match &t.value { ToolValue::Mcp(mcp) => { - // An MCP entry is added with no summary and the roster displays it by its - // resource path, so that path is the only name the form can offer for it. Match - // it too, or naming one tool would be the sole way to keep a whole server. - // Both sides are stripped: the roster stores the path as authored, so the name - // the picker offers carries the `$res:` prefix while `McpToolSource` does not. + // An MCP entry is added with no summary and shows as its resource path, so that + // path is the only name the form can offer for the server as a whole. It is + // matched bare: a name carrying the `$res:` the roster stores would be resolved + // to the resource itself before the worker is handed its args. let path = mcp.resource_path.trim_start_matches("$res:"); - let named = named - || enabled - .iter() - .any(|n| n.trim_start_matches("$res:") == path); + let named = named || enabled.iter().any(|n| n == path); if named { enabled_mcp_paths.insert(path.to_string()); } @@ -329,12 +325,7 @@ fn unmatched_enabled_tools_message( let unmatched: Vec<&str> = enabled_tools .iter() .map(|name| name.as_str()) - // An MCP server is named by the path the roster shows, which carries the `$res:` the - // matched paths are stripped of, so the two are compared without it. - .filter(|name| { - let bare = name.trim_start_matches("$res:"); - !advertised.iter().any(|a| a == name || *a == bare) - }) + .filter(|name| !advertised.contains(name)) .collect(); if unmatched.is_empty() { return None; @@ -2067,30 +2058,19 @@ mod tests { assert_eq!(names(&kept), ["github"]); assert!(paths.is_empty()); - // An MCP entry is added with no summary, and the form offers it by the path the roster - // displays it as. Without this the only way to keep such a server would be naming one of - // the tools it has not been asked for yet. - let unnamed = || { - vec![AgentTool { - summary: None, - ..mcp("c", "github", "$res:u/test/gh") - }] - }; - let (kept, paths) = narrow_roster(unnamed(), Some(&["u/test/gh".to_string()])); - assert_eq!(kept.len(), 1); - assert_eq!(paths.into_iter().collect::>(), ["u/test/gh"]); - let (dropped, paths) = narrow_roster(unnamed(), Some(&["u/test/other".to_string()])); - assert!(dropped.is_empty()); - assert!(paths.is_empty()); - - // The roster stores the path as authored, so the name the picker offers is the `$res:` form - // while the loader's own key is not — a run that selects a server from the form sends this. - let selected = ["$res:u/test/gh".to_string()]; - let (kept, paths) = narrow_roster(unnamed(), Some(&selected)); + // An MCP entry is added with no summary, and the form names it by its resource path, bare. + // Without this the only way to keep such a server would be naming one of the tools it has + // not been asked for yet. + let unnamed = || vec![AgentTool { summary: None, ..mcp("c", "github", "$res:u/test/gh") }]; + let named_server = ["u/test/gh".to_string()]; + let (kept, paths) = narrow_roster(unnamed(), Some(&named_server)); assert_eq!(kept.len(), 1); assert_eq!(paths.into_iter().collect::>(), ["u/test/gh"]); // And having matched, it is not reported as having named nothing. - assert!(unmatched_enabled_tools_message(&selected, &["u/test/gh"]).is_none()); + assert!(unmatched_enabled_tools_message(&named_server, &["u/test/gh"]).is_none()); + let (dropped, paths) = narrow_roster(unnamed(), Some(&["u/test/other".to_string()])); + assert!(dropped.is_empty()); + assert!(paths.is_empty()); } /// The two sides of the server-entry match are different types, and getting it wrong advertises diff --git a/frontend/src/lib/components/flows/agentToolUtils.ts b/frontend/src/lib/components/flows/agentToolUtils.ts index c46868c46e..44600ea1bd 100644 --- a/frontend/src/lib/components/flows/agentToolUtils.ts +++ b/frontend/src/lib/components/flows/agentToolUtils.ts @@ -99,6 +99,16 @@ export function toolDisplayName(tool: AgentTool): string | undefined { return tool?.summary || value?.path || value?.resource_path || undefined } +/** The name `enabled_tools` holds a tool by, which is what the roster shows it as except for an MCP + * server displayed by the resource path it was authored with: a name carrying that `$res:` is + * resolved to the resource's own value before the step runs, so it would reach the worker as an + * object where a name is expected and fail the step outright. */ +export function toolEnabledName(tool: AgentTool): string | undefined { + const name = toolDisplayName(tool) + const value = tool?.value as Record + return name && name === value?.resource_path ? name.replace(/^\$res:/, '') : name +} + /** * Create an AI Agent tool (nested agent) */ diff --git a/frontend/src/lib/components/flows/content/AiAgentStepInputs.svelte b/frontend/src/lib/components/flows/content/AiAgentStepInputs.svelte index da78d2d50f..b9643334e9 100644 --- a/frontend/src/lib/components/flows/content/AiAgentStepInputs.svelte +++ b/frontend/src/lib/components/flows/content/AiAgentStepInputs.svelte @@ -46,7 +46,7 @@ import { Plus, X } from 'lucide-svelte' import type { PickableProperties } from '../previousResults' import type { FlowCopilotContext } from '$lib/components/copilot/flow' - import { toolDisplayName, type AgentTool } from '../agentToolUtils' + import { toolEnabledName, type AgentTool } from '../agentToolUtils' import { AGENT_FIELDS, AGENT_FIELD_GROUPS, @@ -168,15 +168,11 @@ // field's shape from; `flowInfers` hands every step its own copy, so this stays this step's. // A linked step gets the resource's roster here, which is the one it narrows. $effect(() => { - // By the name the roster shows, not the summary alone: an MCP entry is added without one and - // displays as its resource path, so keying on `summary` would leave a whole server with no - // name to pick. `narrow_roster` matches that path for the same reason. - // - // Stripped of `$res:`, which the roster keeps: a static input transform holding one is - // resolved to the resource's own value before the step runs, so the prefixed form would - // reach the worker as an object where a name is expected, and the step would fail outright. + // By what each tool is named, not the summary alone: an MCP entry is added without one and is + // named by its resource path, so keying on `summary` would leave a whole server with no name + // to pick. `narrow_roster` matches that path for the same reason. const names = tools - .map((tool) => toolDisplayName(tool)?.replace(/^\$res:/, '')) + .map((tool) => toolEnabledName(tool)) .filter((name): name is string => !!name) const properties = schemaProperties untrack(() => {