Repository navigation
[Fix] Delegation continues when the parent's pending tool results cannot be saved #2189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<typeof makeParentTask>) => { | ||
| 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<typeof setup>) => { | ||
| 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<typeof makeParentTask>) => { | ||
| parent.flushPendingToolResultsToHistory.mockResolvedValue(false) | ||
| parent.retrySaveApiConversationHistory.mockResolvedValue(false) | ||
| }, | ||
| }, | ||
| { | ||
| name: "the retry save succeeds but the results are still not in history", | ||
| arrange: (parent: ReturnType<typeof makeParentTask>) => { | ||
| parent.flushPendingToolResultsToHistory.mockResolvedValue(false) | ||
| parent.retrySaveApiConversationHistory.mockResolvedValue(true) | ||
| }, | ||
| }, | ||
| { | ||
| name: "the flush throws", | ||
| arrange: (parent: ReturnType<typeof makeParentTask>) => { | ||
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the |
||
| 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, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. History starts as |
||
| 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([]) | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4789,30 +4789,36 @@ | |
| // 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() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the first flush returned false because |
||
| } | ||
| } 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.`, | ||
|
Check warning on line 4815 in src/core/webview/ClineProvider.ts
|
||
| ) | ||
| 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.`, | ||
| ) | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
|
|
||
| // 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. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests stub
flushandretry, so a false-then-true flush is always possible. Would a case with a realTask(saveApiMessagesrejecting once) cover the rollback and the delegate gate together, including a repeat delegation that appends the results once?