diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index f55f54d6bc..c4bbc0b22f 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -28224,7 +28224,8 @@ components: nullable: true description: >- What that same call returned, including the citations of a provider-native web - search. Null on a call that failed, which `success` reports. + search, and what it failed with when it failed — the row's own text names the + tool rather than the reason. Null for a tool whose job holds the answer. reasoning: type: string nullable: true diff --git a/backend/windmill-worker/src/ai/tools.rs b/backend/windmill-worker/src/ai/tools.rs index cd18482e88..0661a98a4b 100644 --- a/backend/windmill-worker/src/ai/tools.rs +++ b/backend/windmill-worker/src/ai/tools.rs @@ -286,15 +286,19 @@ async fn execute_mcp_tool_call( update_flow_status_module_with_actions_success(ctx.db, parent_job, false).await?; } - // Add tool message to conversation if chat_input_enabled + // Add tool message to conversation if chat_input_enabled. The row is worded from + // the tool, like every other tool row, and the error it failed with is its result + // — the one field a call that produced nothing else still has something to put in. let agent_job_id = ctx.job.id; + let content = format!("Error executing {}", tool_name); add_tool_message_to_chat( ctx, Some(agent_job_id), - &error_msg, + &content, false, Some(MessageExtras { tool_arguments: Some(tool_call.function.arguments.clone()), + tool_result: Some(error_msg.clone()), ..Default::default() }), ) @@ -706,8 +710,13 @@ async fn handle_tool_execution_error( update_flow_status_module_with_actions_success(ctx.db, parent_job, false).await?; } - // Add tool message to conversation if chat_input_enabled (error case) - add_tool_message_to_chat(ctx, Some(job_id), &error_message, false, None).await; + // Add tool message to conversation if chat_input_enabled (error case). Worded from the + // tool like every other tool row; nothing is put on the row because the tool's own job + // holds it — `handle_non_flow_job_error` above completed that job with this error, and it + // was pushed with the arguments the step's input transforms produced rather than the raw + // ones the model supplied. + let content = format!("Error executing {}", tool_call.function.name); + add_tool_message_to_chat(ctx, Some(job_id), &content, false, None).await; Ok(()) } @@ -808,13 +817,15 @@ async fn handle_tool_execution_success( ..Default::default() }); - // Stream tool result (success case) + // The job ran; whether it ran successfully is `success`, and the row stored below is + // worded from it. The stream has to carry the same value, or the card the reader watches + // and the row that replaces it describe the same call differently. if let Some(stream_event_processor) = ctx.stream_event_processor { let tool_result_event = StreamingEvent::ToolResult { call_id: tool_call.id.clone(), function_name: tool_call.function.name.clone(), result: tool_result, - success: true, + success, }; stream_event_processor .send(tool_result_event, final_events_str) diff --git a/frontend/src/lib/components/chat/utils.ts b/frontend/src/lib/components/chat/utils.ts index f2fc7c3ac1..494ce024d1 100644 --- a/frontend/src/lib/components/chat/utils.ts +++ b/frontend/src/lib/components/chat/utils.ts @@ -59,9 +59,15 @@ export function parseStreamEvents(streamData: string): StreamEvent[] { return events } -/** One-line summary of a tool call, for a surface with no room for the call itself. */ +/** + * One-line summary of a tool call, for a surface with no room for the call itself. + * + * Worded as the server words the row it stores for the same call, so the sentence does not + * change under the reader when a reload replaces what the stream wrote with what was + * persisted — and so the two can be told to be the same call by their text alone. + */ export function toolSummary(name: string, success: boolean): string { - return success ? `Used ${name} tool` : `Failed to use ${name} tool` + return success ? `Used ${name} tool` : `Error executing ${name}` } /** diff --git a/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts b/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts index dd97b74ab9..9476f9edcf 100644 --- a/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts +++ b/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts @@ -61,6 +61,19 @@ function parseToolPayload(raw: string | null | undefined): any { } } +/** + * A failed tool's result as the line the card shows for it. + * + * The worker stores what the tool failed with, which is a string for the failures it words + * itself and a structured error for one a job reported. Anything else is a result that says + * nothing about the failure, and the row's own text is the better line. + */ +function asErrorText(result: any): string | undefined { + if (typeof result === 'string') return result + const message = result?.error?.message ?? result?.message + return typeof message === 'string' ? message : undefined +} + function toDisplayMessage( message: ChatMessage, userIndex: number, @@ -109,7 +122,9 @@ function toDisplayMessage( const fromJob = carriesItsOwnCall ? EMPTY_TOOL_CALL : toolCalls.get(message.job_id) const toolName = fromRow.toolName ?? fromJob.toolName const parameters = fromRow.parameters ?? fromJob.parameters - const result = failed ? undefined : (fromRow.result ?? fromJob.result) + // A failed tool's result is what it failed with, which is the whole of what the + // card has to show about it. + const result = fromRow.result ?? fromJob.result return { role: 'tool', tool_call_id: message.id, @@ -123,7 +138,10 @@ function toDisplayMessage( // The card's fold is opt-in (ToolExecutionDisplay reads showDetails), so it is // offered only when there is a call or a result behind it to reveal. showDetails: parameters !== undefined || result !== undefined, - error: failed ? message.content : undefined, + // What the tool failed with, from its result — and from the row's text for one + // written before the worker put the error in the result, where the text was all + // there was. Both keep rendering the same card. + error: failed ? (asErrorText(result) ?? message.content) : undefined, isLoading: message.loading } } diff --git a/frontend/src/lib/components/flows/conversations/flowChatViewHost.test.ts b/frontend/src/lib/components/flows/conversations/flowChatViewHost.test.ts index 591c219d40..1de5e07cb3 100644 --- a/frontend/src/lib/components/flows/conversations/flowChatViewHost.test.ts +++ b/frontend/src/lib/components/flows/conversations/flowChatViewHost.test.ts @@ -412,3 +412,33 @@ describe('the inputs a row shows for the turn just sent', () => { expect(JobService.getJobArgs).not.toHaveBeenCalled() }) }) + +/** + * A tool that failed shows what it failed with. The worker used to word that into the row's + * text, and now stores it as the row's result so the text can be the same sentence every + * other tool row carries — so rows written either way have to keep rendering the same card. + */ +describe('a failed tool call', () => { + const toolRow = (over: Record) => ({ + id: 'row-1', + conversation_id: 'a', + message_type: 'tool', + success: false, + created_seq: 1, + ...over + }) + + it('shows the error the row stores as its result', () => { + const manager = stubManager('a') + manager.messages = [ + toolRow({ content: 'Error executing get_time', tool_result: 'MCP tool error: refused' }) + ] + expect(host(manager).displayMessages[0]?.error).toBe('MCP tool error: refused') + }) + + it('falls back to the row text for one written before the result carried it', () => { + const manager = stubManager('a') + manager.messages = [toolRow({ content: 'MCP tool error: refused' })] + expect(host(manager).displayMessages[0]?.error).toBe('MCP tool error: refused') + }) +})