mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
1947353405
commit
b14990b3c0
@@ -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
|
||||
]
|
||||
}
|
||||
|
||||
@@ -27,7 +27,8 @@ const { streamCalls, streamScript, jobCompleted } = vi.hoisted(() => ({
|
||||
jobCompleted: {
|
||||
value: true as boolean | 'throw',
|
||||
gate: undefined as Promise<void> | 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()
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user