diff --git a/src/__tests__/ClineProvider.delegation.spec.ts b/src/__tests__/ClineProvider.delegation.spec.ts index 68f39c6517..0f72272ed7 100644 --- a/src/__tests__/ClineProvider.delegation.spec.ts +++ b/src/__tests__/ClineProvider.delegation.spec.ts @@ -95,6 +95,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 e573a7f107..dac5cf9ef4 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -534,6 +534,12 @@ export class Task extends EventEmitter implements TaskLike { * Set to `true` only after the assistant message is durably saved. */ assistantMessageSavedToHistory = false + /** + * True when the pending tool results were already appended to `apiConversationHistory` but the + * save failed. `userMessageContent` is kept for recovery, so a repeat flush must save the history + * as it is instead of appending the same user message again. + */ + private pendingToolResultsInHistory = false private assistantMessagePersistencePromise!: Promise private resolveAssistantMessagePersistence!: (result: AssistantMessagePersistenceResult) => void private assistantMessagePersistenceCancellation?: AssistantMessagePersistenceCancellation @@ -1528,6 +1534,16 @@ export class Task extends EventEmitter implements TaskLike { return false } + if (this.pendingToolResultsInHistory) { + // A previous flush appended the results but could not save them; save, never append twice. + const saved = await this.saveApiConversationHistory() + if (saved) { + this.userMessageContent = [] + this.pendingToolResultsInHistory = false + } + return saved + } + // CRITICAL: Wait for the assistant message to be saved to API history first. // Without this, tool_result blocks would appear BEFORE tool_use blocks in the // conversation history, causing API errors like: @@ -1580,6 +1596,7 @@ export class Task extends EventEmitter implements TaskLike { // Clear the pending content since it's now saved this.userMessageContent = [] } else { + this.pendingToolResultsInHistory = true console.warn( `[Task#${this.taskId}] flushPendingToolResultsToHistory: save failed, retaining pending tool results in memory`, ) @@ -4106,6 +4123,7 @@ export class Task extends EventEmitter implements TaskLike { this.assistantMessageContent = [] this.didCompleteReadingStream = false this.userMessageContent = [] + this.pendingToolResultsInHistory = false this.userMessageContentReady = false this.activeToolContexts.clear() this.didRejectTool = false diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 109eee8dc0..1c8c3cffba 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -2467,5 +2467,58 @@ 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("saves, without appending again, results that were added to history but not saved", async () => { + mockSaveApiMessages.mockRejectedValueOnce(new Error("disk full")).mockResolvedValueOnce([]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + task.assistantMessageSavedToHistory = true + task.userMessageContent = [{ type: "tool_result", tool_use_id: "tool-1", content: "done" }] + + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(false) + expect(toolResultMessages(task)).toHaveLength(1) + + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(true) + + // The retry saved the history as it was; it did not append a second copy of the results. + expect(toolResultMessages(task)).toHaveLength(1) + expect(task.userMessageContent).toEqual([]) + expect(mockSaveApiMessages).toHaveBeenCalledTimes(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 28a42913f3..2aa0511023 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -4042,30 +4042,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.