diff --git a/src/__tests__/ClineProvider.delegation.spec.ts b/src/__tests__/ClineProvider.delegation.spec.ts index eceb4cbeb0..fb5e36e13f 100644 --- a/src/__tests__/ClineProvider.delegation.spec.ts +++ b/src/__tests__/ClineProvider.delegation.spec.ts @@ -94,6 +94,92 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(taskHistoryStore.atomicReadAndUpdate).not.toHaveBeenCalled() }) + describe("when the parent's pending tool results cannot be saved", () => { + const setup = (parentTask: ReturnType) => { + const child = { taskId: "child-1", run: vi.fn().mockResolvedValue(undefined) } + const taskHistoryStore = makeStoreStub({ get: vi.fn().mockReturnValue(parentHistoryItem) }) + const provider = { + taskScheduler: new TaskScheduler(), + emit: vi.fn(), + getCurrentTask: vi.fn(() => parentTask), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockResolvedValue(child), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + log: vi.fn(), + isViewLaunched: false, + taskHistoryStore, + } as unknown as ClineProvider + const delegate = () => + ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + }) + return { provider, taskHistoryStore, delegate } + } + + const expectNothingDelegated = ({ provider, taskHistoryStore }: ReturnType) => { + expect(provider.removeClineFromStack).not.toHaveBeenCalled() + expect(provider.createTask).not.toHaveBeenCalled() + expect(taskHistoryStore.atomicReadAndUpdate).not.toHaveBeenCalled() + } + + it.each([ + { + name: "the flush and the retry save both fail", + arrange: (parent: ReturnType) => { + parent.flushPendingToolResultsToHistory.mockResolvedValue(false) + parent.retrySaveApiConversationHistory.mockResolvedValue(false) + }, + }, + { + name: "the retry save succeeds but the results are still not in history", + arrange: (parent: ReturnType) => { + parent.flushPendingToolResultsToHistory.mockResolvedValue(false) + parent.retrySaveApiConversationHistory.mockResolvedValue(true) + }, + }, + { + name: "the flush throws", + arrange: (parent: ReturnType) => { + parent.flushPendingToolResultsToHistory.mockRejectedValue(new Error("unexpected")) + }, + }, + ])("refuses to delegate and leaves the parent open when $name", async ({ arrange }) => { + const parentTask = makeParentTask() + arrange(parentTask) + const harness = setup(parentTask) + + await expect(harness.delegate()).rejects.toThrow("pending tool results could not be saved") + + expectNothingDelegated(harness) + expect(parentTask.emit).not.toHaveBeenCalled() + }) + + it("delegates when a repeat flush after the retry save succeeds", async () => { + const parentTask = makeParentTask() + parentTask.flushPendingToolResultsToHistory.mockResolvedValueOnce(false).mockResolvedValueOnce(true) + parentTask.retrySaveApiConversationHistory.mockResolvedValue(true) + const harness = setup(parentTask) + + await expect(harness.delegate()).resolves.toMatchObject({ taskId: "child-1" }) + + expect(parentTask.flushPendingToolResultsToHistory).toHaveBeenCalledTimes(2) + expect(harness.provider.createTask).toHaveBeenCalledOnce() + }) + + it("delegates without a retry when the first flush succeeds", async () => { + const parentTask = makeParentTask() + const harness = setup(parentTask) + + await expect(harness.delegate()).resolves.toMatchObject({ taskId: "child-1" }) + + expect(parentTask.flushPendingToolResultsToHistory).toHaveBeenCalledOnce() + expect(parentTask.retrySaveApiConversationHistory).not.toHaveBeenCalled() + }) + }) + it("clears a matching pending action when delegation commits", async () => { const pendingAction = { kind: "create_subtask" as const, diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 90cf7bf7fb..dfd15557f5 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1598,6 +1598,12 @@ export class Task extends EventEmitter implements TaskLike { // Clear the pending content since it's now saved this.userMessageContent = [] } else { + // Undo the in-memory append: `userMessageContent` stays the only pending copy, so a retry + // flush or the next request appends these results exactly once. + const appendedIndex = this.apiConversationHistory.indexOf(userMessageWithTs as ApiMessage) + if (appendedIndex !== -1) { + this.apiConversationHistory.splice(appendedIndex, 1) + } console.warn( `[Task#${this.taskId}] flushPendingToolResultsToHistory: save failed, retaining pending tool results in memory`, ) diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index fb7a4e8cc6..ee54e9a003 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -2467,5 +2467,69 @@ describe("Task persistence", () => { // userMessageContent should be cleared on success expect(task.userMessageContent).toEqual([]) }) + + const toolResultMessages = (task: Task) => + task.apiConversationHistory.filter( + (message) => + message.role === "user" && + Array.isArray(message.content) && + message.content.some((block) => block.type === "tool_result"), + ) + + it("keeps an unsaved flush out of history so retries and the next request append the results once", async () => { + mockSaveApiMessages + .mockRejectedValueOnce(new Error("disk full")) + .mockRejectedValueOnce(new Error("disk full")) + .mockResolvedValueOnce([]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + task.assistantMessageSavedToHistory = true + const pending = [{ type: "tool_result" as const, tool_use_id: "tool-1", content: "done" }] + task.userMessageContent = [...pending] + + // Repeated failures leave history untouched and keep the results pending in memory, so the + // request loop (which appends userMessageContent) cannot add a second copy. + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(false) + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(false) + expect(toolResultMessages(task)).toHaveLength(0) + expect(task.userMessageContent).toEqual(pending) + + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(true) + expect(toolResultMessages(task)).toHaveLength(1) + expect(task.userMessageContent).toEqual([]) + + // New results after the successful retry append as their own message. + mockSaveApiMessages.mockResolvedValueOnce([]) + task.userMessageContent = [{ type: "tool_result", tool_use_id: "tool-2", content: "done" }] + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(true) + expect(toolResultMessages(task)).toHaveLength(2) + }) + + it("keeps results that never reached history so a later flush can still add them", async () => { + mockSaveApiMessages.mockResolvedValue([]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + task.userMessageContent = [{ type: "tool_result", tool_use_id: "tool-1", content: "done" }] + vi.spyOn(task, "waitForCurrentAssistantMessagePersistence") + .mockResolvedValueOnce(false) + .mockResolvedValue(true) + + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(false) + expect(toolResultMessages(task)).toHaveLength(0) + expect(task.userMessageContent).toHaveLength(1) + + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(true) + + expect(toolResultMessages(task)).toHaveLength(1) + expect(task.userMessageContent).toEqual([]) + }) }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index dc4ed27621..60bced6a7a 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -4789,30 +4789,36 @@ export class ClineProvider // is already added to apiConversationHistory by the normal flow in // recursivelyMakeClineRequests BEFORE tools start executing. We only need to // flush the pending user message with tool_results. + let pendingResultsPersisted = false try { - const flushSuccess = await parent.flushPendingToolResultsToHistory() + pendingResultsPersisted = await parent.flushPendingToolResultsToHistory() - if (!flushSuccess) { + if (!pendingResultsPersisted) { console.warn(`[delegateParentAndOpenChild] Flush failed for parent ${parentTaskId}, retrying...`) - const retrySuccess = await parent.retrySaveApiConversationHistory() - - if (!retrySuccess) { - console.error( - `[delegateParentAndOpenChild] CRITICAL: Parent ${parentTaskId} API history not persisted to disk. Child return may produce stale state.`, - ) - vscode.window.showWarningMessage( - "Warning: Parent task state could not be saved. The parent task may lose recent context when resumed.", - ) - } + await parent.retrySaveApiConversationHistory() + // Saving the existing history does not add results that never reached it, so flush again. + pendingResultsPersisted = await parent.flushPendingToolResultsToHistory() } } catch (error) { this.log( - `[delegateParentAndOpenChild] Error flushing pending tool results (non-fatal): ${ + `[delegateParentAndOpenChild] Error flushing pending tool results: ${ error instanceof Error ? error.message : String(error) }`, ) } + if (!pendingResultsPersisted) { + // Resuming this parent later would send a tool_use without its tool_result (a provider 400). + // Nothing has been disposed or created yet: the parent stays open with its pending results in + // memory, so the same delegation can simply be retried. + console.error( + `[delegateParentAndOpenChild] CRITICAL: Pending tool results of parent ${parentTaskId} could not be saved.`, + ) + throw new Error( + `Could not start the subtask: the parent task's pending tool results could not be saved to disk. The parent task stays open and nothing was delegated. Check disk space and permissions, then try again.`, + ) + } + // 3) Enforce single-open invariant by closing/disposing the parent first // This ensures we never have >1 tasks open at any time during delegation. // Await abort completion to ensure clean disposal and prevent unhandled rejections.