From b14990b3c0bde34a8d0b81b3e456d94d10ae2ea7 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Wed, 16 Sep 2026 13:45:47 +0200 Subject: [PATCH] fix(chat): end a turn on a failure when its run failed The answer shown for a turn whose rows never arrived came from the run's result, and the run's own verdict was thrown away with it. A flow that failed before writing anything therefore ended as a successful turn carrying its error envelope rendered as JSON: Retry was not offered, and a message queued behind it went straight out into the same failure. The row now carries what the run said of itself, and a failure is shown as what it failed with rather than as the envelope around it. Co-Authored-By: Claude Opus 5 (1M context) --- .../conversations/FlowChatManager.svelte.ts | 20 ++++++++-- .../conversations/FlowChatManager.test.ts | 40 ++++++++++++++++++- .../conversations/flowChatViewHost.svelte.ts | 14 ++----- 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/frontend/src/lib/components/flows/conversations/FlowChatManager.svelte.ts b/frontend/src/lib/components/flows/conversations/FlowChatManager.svelte.ts index 71bb8234e6..32b8e13a17 100644 --- a/frontend/src/lib/components/flows/conversations/FlowChatManager.svelte.ts +++ b/frontend/src/lib/components/flows/conversations/FlowChatManager.svelte.ts @@ -112,6 +112,14 @@ function toStreamEvent(event: AgentStreamEvent): StreamEvent { } } +/** What a run that failed said, as the line a turn ends on. Windmill wraps a failure in an + * error envelope; anything else is shown as it came. */ +function failedRunMessage(result: unknown): string | undefined { + const message = (result as { error?: { message?: unknown } })?.error?.message + if (typeof message === 'string' && message !== '') return message + return extractChatAnswer(result) +} + function emptyStatus(): TurnStatus { return { isLoading: false, @@ -1486,11 +1494,13 @@ export class FlowChatManager { const api = this.#chatApi() const signal = turn.signal let flowResult: unknown + let flowSucceeded = true while (this.#isCurrent(turn)) { try { - const { completed, result } = await api.getCompletedResult(jobId, signal) + const { completed, success, result } = await api.getCompletedResult(jobId, signal) if (completed) { flowResult = result + flowSucceeded = success !== false break } } catch (error) { @@ -1507,7 +1517,11 @@ export class FlowChatManager { // answer the row would have carried, and showing it is what keeps a finished turn // from reading as one that produced nothing. A reload replaces it with the row. if (read === 0) { - const answer = extractChatAnswer(flowResult) + // A run that failed says so in an error rather than an answer, and the row has to + // carry that: it is what the transcript reads the turn's outcome off, which is what + // offers Retry and what holds a queued message back from running into the same + // failure. + const answer = flowSucceeded ? extractChatAnswer(flowResult) : failedRunMessage(flowResult) if (typeof answer === 'string' && answer !== '') { this.#rowsById[turn.conversationId] = [ ...this.#rowsOf(turn.conversationId), @@ -1519,7 +1533,7 @@ export class FlowChatManager { created_at: new Date().toISOString(), created_seq: 0, job_id: jobId, - success: true + success: flowSucceeded } as ChatMessage ] } diff --git a/frontend/src/lib/components/flows/conversations/FlowChatManager.test.ts b/frontend/src/lib/components/flows/conversations/FlowChatManager.test.ts index 1e73a91336..934eb7d898 100644 --- a/frontend/src/lib/components/flows/conversations/FlowChatManager.test.ts +++ b/frontend/src/lib/components/flows/conversations/FlowChatManager.test.ts @@ -27,7 +27,8 @@ const { streamCalls, streamScript, jobCompleted } = vi.hoisted(() => ({ jobCompleted: { value: true as boolean | 'throw', gate: undefined as Promise | undefined, - result: {} as unknown + result: {} as unknown, + success: true } })) @@ -45,7 +46,11 @@ vi.mock('windmill-chat', async (importOriginal) => { async getCompletedResult() { if (jobCompleted.gate) await jobCompleted.gate if (jobCompleted.value === 'throw') throw new Error('job status unavailable') - return { completed: jobCompleted.value, success: true, result: jobCompleted.result } + return { + completed: jobCompleted.value, + success: jobCompleted.success, + result: jobCompleted.result + } } } return { ...actual, WindmillChatApi: FakeApi } @@ -748,6 +753,7 @@ describe('a conversation opened while its run is still going', () => { .mockReset() .mockResolvedValue([] as any) jobCompleted.value = true + jobCompleted.success = true jobCompleted.result = { windmill_chat_answer: 'the answer the run produced' } const manager = (live = managerWithRows()) ;(manager as any).initialize( @@ -765,6 +771,36 @@ describe('a conversation opened while its run is still going', () => { expect(manager.messages.map((m) => m.content)).toContain('the answer the run produced') }) + /** + * A run that fails before writing anything still has to end the turn as a failure: the + * transcript reads a turn's outcome off its last row, and that is what offers Retry and + * what holds a message queued behind the turn back from running into the same failure. + */ + it('ends on a failure when the run failed and wrote nothing', async () => { + vi.mocked(FlowConversationsService.listConversationMessages) + .mockReset() + .mockResolvedValue([] as any) + jobCompleted.value = true + jobCompleted.success = false + jobCompleted.result = { error: { message: 'the provider refused' } } + const manager = (live = managerWithRows()) + ;(manager as any).initialize( + vi.fn(async () => 'job-1'), + 'u/admin/flow', + false + ) + manager.operatingWorkspace = () => 'ws' + manager.selectedConversationId = 'a' + manager.inputMessage = 'ask' + + await manager.sendMessage(undefined, undefined, 'a') + await vi.waitFor(() => expect(manager.isConversationBusy('a')).toBe(false)) + + const last = manager.messages.at(-1) + expect(last?.content).toBe('the provider refused') + expect(last?.success).toBe(false) + }) + it('leaves no card spinning when Stop ends the turn', async () => { vi.mocked(FlowConversationsService.listConversationMessages) .mockReset() diff --git a/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts b/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts index 44a1eef05e..9fbe738430 100644 --- a/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts +++ b/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts @@ -477,16 +477,10 @@ export class FlowChatViewHost implements ChatViewHost { } attachedLocally = true } - // The one place a run starts, and so the one place to refuse to start one. Every way - // in waits on something first — uploading the attachments, asking a failed job what - // it ran with — and the panel can be replaced while any of them is in flight; the - // manager it would hand the turn to is then one nobody is reading, pointed at - // whichever flow replaced this one. - // The one place a run starts, and so the one place to refuse to start one. Every way - // in waits on something first — uploading the attachments, asking a failed job what - // it ran with — and the panel can be replaced while any of them is in flight; the - // manager it would hand the turn to is then one nobody is reading, pointed at - // whichever flow replaced this one. + // The one place a run starts, and so the one place to refuse to start one: every way + // in waits on something first — an upload, a failed job's arguments — and the panel + // can be replaced while it waits, leaving the manager it would hand the turn to one + // nobody is reading, pointed at whichever flow replaced this one. if (this.#gone) return false const sentInputs = this.#describeSentInputs(args, images, blobs, attachedLocally)