From 48e9383deaa5d56d14f0eac78e48b1750c9fb43c Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:59:49 +0200 Subject: [PATCH] fix: never restore a tool card that nothing left can resolve --- .../copilot/chat/AIChatManager.svelte.ts | 37 +++++++++++--- .../copilot/chat/AIChatManager.test.ts | 49 ++++++++++++++++++- 2 files changed, 76 insertions(+), 10 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index 0bd8ffa4c5..feabff5804 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -1024,7 +1024,8 @@ export class AIChatManager { this.updateJob(job.jobId, { status: 'failure', reported: true, job: trimJob(gone) }) this.applyToolStatus(job.toolCallId, { content: 'Background job could not be retrieved (it may have been removed)', - error: `Job ${job.jobId} was unreachable` + error: `Job ${job.jobId} was unreachable`, + isLoading: false }) anyTerminal = true } else { @@ -1066,8 +1067,14 @@ export class AIChatManager { status === 'canceled' || !job.resultFormat ? undefined : formatChatJobCompletion(completed, job.resultFormat) - // Fill the tool card that launched it (we run outside a turn here). - this.applyToolStatus(job.toolCallId, formatted?.card ?? completedJobToolStatus(completed)) + // Fill the tool card that launched it (we run outside a turn here). isLoading is + // normally already false — processToolCall clears it when the launching tool + // returns — but a card restored from a mid-turn checkpoint never saw that return, + // so only this patch can stop it spinning. + this.applyToolStatus(job.toolCallId, { + ...(formatted?.card ?? completedJobToolStatus(completed)), + isLoading: false + }) // A user-canceled job needs no model note or auto-resume: the user stopped it // deliberately, so announcing it (as "FAILED", since a canceled job isn't a // success) or burning a turn on it would be noise. @@ -4273,6 +4280,15 @@ export class AIChatManager { if (this.isJobNonTerminal(j.status)) j.detached = true } if (this.backgroundJobs.length > 0) this.backgroundJobs = [...this.backgroundJobs] + // Reloading resolves no card on its own. Settle every one the poller above + // will not reach, whoever wrote it — a record from a build that stored cards + // without their jobs would otherwise restore one that spins forever. + const pollable = this.#pollableToolCalls() + this.displayMessages = this.settledToolDisplay( + this.displayMessages, + 'Interrupted', + (message) => !pollable.has(message.tool_call_id) + ) this.#ensureJobPoller() // Message-attached files live in the transcript, not in the store's // persistence — rebuild their rows so the loaded chat's references are @@ -4674,11 +4690,7 @@ export class AIChatManager { * in the same record — registering one does not write it. So the jobs come back * with the transcript that depends on them, and both go into the same saveChat. */ #interruptedSnapshot = (): { display: DisplayMessage[]; jobs: ChatJob[] } => { - const polled = new Set( - this.backgroundJobs - .filter((j) => j.detached || this.isJobNonTerminal(j.status)) - .map((j) => j.toolCallId) - ) + const polled = this.#pollableToolCalls() return { display: this.settledToolDisplay( this.displayMessages, @@ -4688,6 +4700,15 @@ export class AIChatManager { jobs: $state.snapshot(this.backgroundJobs) as ChatJob[] } } + + /** Tool calls a restored transcript can still resolve. loadPastChat re-attaches the + * poller to every non-terminal job and nothing else runs after a reload, so this is + * the whole set — asked identically when storing a card and when restoring one, or + * the two drift and a card is kept by one and stranded by the other. */ + #pollableToolCalls = (): Set => + new Set( + this.backgroundJobs.filter((j) => this.isJobNonTerminal(j.status)).map((j) => j.toolCallId) + ) } export const aiChatManager = new AIChatManager() diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts index 16ba8cec13..33dcb411a3 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts @@ -2840,6 +2840,33 @@ describe('AIChatManager queued messages', () => { expect(manager.modifiedItems?.size).toBe(0) }) + // Reloading resolves no card on its own. Only the poller can, and only for the jobs + // that came back with the transcript — so a stored card without one must arrive + // settled, whichever build wrote it. + it('settles a restored loading card that no job came back to resolve', async () => { + const manager = createManager(createInputMock()) + mocks.getJob.mockResolvedValue({ type: 'QueuedJob', id: 'job-1' }) + vi.spyOn(manager.historyManager, 'loadPastChat').mockReturnValue({ + id: 'reloaded', + title: 'Reloaded', + displayMessages: [ + { role: 'tool', tool_call_id: 'orphan', content: 'Running...', isLoading: true }, + { role: 'tool', tool_call_id: 'polled', content: 'Running...', isLoading: true } + ], + actualMessages: [], + lastModified: 0 + } as unknown as ReturnType) + vi.spyOn(manager.historyManager, 'getBackgroundJobs').mockReturnValue([ + { jobId: 'job-1', toolCallId: 'polled', status: 'running' } + ] as any) + + await manager.loadPastChat('reloaded') + + const card = (id: string) => manager.displayMessages.find((m) => m.tool_call_id === id) as any + expect(card('orphan')).toMatchObject({ isLoading: false, error: 'Interrupted' }) + expect(card('polled').isLoading).toBe(true) + }) + it('seeds a session chat mask from its stored modified-items', async () => { const manager = createManager(createInputMock()) manager.isSessionChat = true @@ -3919,6 +3946,22 @@ describe('AIChatManager background job completion', () => { resultFormat: { kind: 'datatable' as const, datatableName: 'main' } } + // Live, processToolCall clears isLoading when the launching tool returns. A card + // restored from a mid-turn checkpoint never sees that return, so completing its job + // is the only thing left that can stop it spinning. + it('stops a restored card spinning when the poller completes its job', async () => { + const manager = new AIChatManager() + manager.registerJob(datatableJob) + manager.displayMessages = [ + { role: 'tool', tool_call_id: 'tc-1', content: 'Running...', isLoading: true } as any + ] + mocks.getJob.mockResolvedValue(completed({ result: [{ n: 1 }] })) + + await completeDetachedJob(manager) + + expect((manager.displayMessages[0] as any).isLoading).toBe(false) + }) + it('reconstructs the datatable result contract from the persisted resultFormat', async () => { const manager = new AIChatManager() manager.registerJob(datatableJob) @@ -3932,7 +3975,8 @@ describe('AIChatManager background job completion', () => { // the SQL contract (row count + shaped rows) rather than generic job output. expect(applyToolStatus).toHaveBeenCalledWith('tc-1', { content: 'Query returned 2 row(s)', - result: JSON.stringify([{ n: 1 }, { n: 2 }], null, 2) + result: JSON.stringify([{ n: 1 }, { n: 2 }], null, 2), + isLoading: false }) expect(manager.pendingJobNotes).toHaveLength(1) expect(manager.pendingJobNotes[0]).toContain('"rowCount": 2') @@ -3986,7 +4030,8 @@ describe('AIChatManager background job completion', () => { expect(manager.backgroundJobs[0]?.status).toBe('canceled') expect(applyToolStatus).toHaveBeenCalledWith('tc-1', { content: 'Background job canceled', - logs: expect.anything() + logs: expect.anything(), + isLoading: false }) })