From d46de0179ce9275070bc72e440b68df65ffbf8bf Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Wed, 16 Sep 2026 13:02:16 +0200 Subject: [PATCH] fix(chat): word every tool row from the tool that ran MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tool row said one of four things depending on how its call ended, and two of those wordings were the error itself. The row's text is the one thing every reader of a conversation has — the transcript, the chat SDK, anything reading the API — so it should say the same kind of thing every time, and the reason a call failed belongs where every other call's result already lives. All four now word the row from the tool, and a failure puts what it failed with in the row's result. Only where the row is the sole copy: a tool that ran has a job holding both its arguments and its error, and that job is still what the card reads them from. The card shows a failed tool's result rather than discarding it, falling back to the row's text for the rows already written the old way, which keep rendering as they always did. A tool whose job ran and failed streamed success where it stored failure, so the card read as a successful call for the length of the turn and corrected itself on the next reload. The stream now carries what the row is worded from. Co-Authored-By: Claude Opus 5 (1M context) --- backend/windmill-api/openapi.yaml | 3 +- backend/windmill-worker/src/ai/tools.rs | 23 ++++++++++---- frontend/src/lib/components/chat/utils.ts | 10 +++++-- .../conversations/flowChatViewHost.svelte.ts | 22 ++++++++++++-- .../conversations/flowChatViewHost.test.ts | 30 +++++++++++++++++++ 5 files changed, 77 insertions(+), 11 deletions(-) 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') + }) +})