From 054c2e4ca0f8c42cb913ff09896a35b0a7f6f887 Mon Sep 17 00:00:00 2001 From: Roomote Date: Wed, 13 May 2026 22:21:29 +0000 Subject: [PATCH 1/8] test: unskip subtasks e2e suite --- apps/vscode-e2e/src/fixtures/subtasks.ts | 73 +++++++++++ apps/vscode-e2e/src/runTest.ts | 2 + apps/vscode-e2e/src/suite/subtasks.test.ts | 139 ++++++++++++--------- 3 files changed, 156 insertions(+), 58 deletions(-) create mode 100644 apps/vscode-e2e/src/fixtures/subtasks.ts diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts new file mode 100644 index 0000000000..a79c09a50d --- /dev/null +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -0,0 +1,73 @@ +import { LLMock } from "@copilotkit/aimock" + +export const SUBTASK_PARENT_PROMPT = "SUBTASK_PARENT_CANCELLATION_SMOKE" +export const SUBTASK_CHILD_PROMPT = "SUBTASK_CHILD_CALCULATOR_SMOKE" +export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9" + +export function addSubtaskFixtures(mock: InstanceType) { + mock.addFixture({ + match: { + userMessage: new RegExp(SUBTASK_PARENT_PROMPT), + }, + response: { + toolCalls: [ + { + name: "new_task", + arguments: JSON.stringify({ + mode: "ask", + message: SUBTASK_CHILD_PROMPT, + }), + id: "call_subtasks_parent_new_task_001", + }, + ], + }, + }) + + mock.addFixture({ + match: { + userMessage: new RegExp(SUBTASK_CHILD_PROMPT), + }, + response: { + toolCalls: [ + { + name: "ask_followup_question", + arguments: JSON.stringify({ + question: "What is the square root of 81?", + follow_up: [{ text: SUBTASK_CHILD_FOLLOWUP_ANSWER }], + }), + id: "call_subtasks_child_followup_001", + }, + ], + }, + }) + + mock.addFixture({ + match: { + toolCallId: "call_subtasks_child_followup_001", + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: "9" }), + id: "call_subtasks_child_completion_002", + }, + ], + }, + }) + + mock.addFixture({ + match: { + toolCallId: "call_subtasks_parent_new_task_001", + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: "Parent task resumed" }), + id: "call_subtasks_parent_completion_003", + }, + ], + }, + }) +} diff --git a/apps/vscode-e2e/src/runTest.ts b/apps/vscode-e2e/src/runTest.ts index c5ac0fb932..f3afa51ea2 100644 --- a/apps/vscode-e2e/src/runTest.ts +++ b/apps/vscode-e2e/src/runTest.ts @@ -10,6 +10,7 @@ import { addExecuteCommandResultFixtures } from "./fixtures/execute-command" import { addListFilesResultFixtures } from "./fixtures/list-files" import { addReadFileResultFixtures } from "./fixtures/read-file" import { addSearchFilesResultFixtures } from "./fixtures/search-files" +import { addSubtaskFixtures } from "./fixtures/subtasks" import { addUseMcpToolResultFixtures } from "./fixtures/use-mcp-tool" import { addWriteToFileResultFixtures } from "./fixtures/write-to-file" @@ -110,6 +111,7 @@ async function main() { addListFilesResultFixtures(mock) addReadFileResultFixtures(mock) addSearchFilesResultFixtures(mock) + addSubtaskFixtures(mock) addUseMcpToolResultFixtures(mock) addWriteToFileResultFixtures(mock) diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index e3e3457520..56100b2075 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -2,73 +2,96 @@ import * as assert from "assert" import { RooCodeEventName, type ClineMessage } from "@roo-code/types" +import { setDefaultSuiteTimeout } from "./test-utils" import { sleep, waitFor, waitUntilCompleted } from "./utils" +import { SUBTASK_CHILD_FOLLOWUP_ANSWER, SUBTASK_CHILD_PROMPT, SUBTASK_PARENT_PROMPT } from "../fixtures/subtasks" + +suite("Roo Code Subtasks", function () { + setDefaultSuiteTimeout(this) -suite.skip("Roo Code Subtasks", () => { test("Should handle subtask cancellation and resumption correctly", async () => { const api = globalThis.api - + const asks: Record = {} const messages: Record = {} + const waitForStage = async (label: string, condition: Parameters[0]) => { + try { + await waitFor(condition) + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + throw new Error(`${label}: ${message}`) + } + } + + const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { + if (message.type === "ask") { + asks[taskId] = asks[taskId] || [] + asks[taskId].push(message) + } - api.on(RooCodeEventName.Message, ({ taskId, message }) => { if (message.type === "say" && message.partial === false) { messages[taskId] = messages[taskId] || [] messages[taskId].push(message) } - }) - - const childPrompt = "You are a calculator. Respond only with numbers. What is the square root of 9?" - - // Start a parent task that will create a subtask. - const parentTaskId = await api.startNewTask({ - configuration: { - mode: "ask", - alwaysAllowModeSwitch: true, - alwaysAllowSubtasks: true, - autoApprovalEnabled: true, - enableCheckpoints: false, - }, - text: - "You are the parent task. " + - `Create a subtask by using the new_task tool with the message '${childPrompt}'.` + - "After creating the subtask, wait for it to complete and then respond 'Parent task resumed'.", - }) - - let spawnedTaskId: string | undefined = undefined - - // Wait for the subtask to be spawned and then cancel it. - api.on(RooCodeEventName.TaskSpawned, (_, childTaskId) => (spawnedTaskId = childTaskId)) - await waitFor(() => !!spawnedTaskId) - await sleep(1_000) // Give the task a chance to start and populate the history. - await api.cancelCurrentTask() - - // Wait a bit to ensure any task resumption would have happened. - await sleep(2_000) - - // The parent task should not have resumed yet, so we shouldn't see - // "Parent task resumed". - assert.ok( - messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === - undefined, - "Parent task should not have resumed after subtask cancellation", - ) - - // Start a new task with the same message as the subtask. - const anotherTaskId = await api.startNewTask({ text: childPrompt }) - await waitUntilCompleted({ api, taskId: anotherTaskId }) - - // Wait a bit to ensure any task resumption would have happened. - await sleep(2_000) - - // The parent task should still not have resumed. - assert.ok( - messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === - undefined, - "Parent task should not have resumed after subtask cancellation", - ) - - // Clean up - cancel all tasks. - await api.clearCurrentTask() - await waitUntilCompleted({ api, taskId: parentTaskId }) + } + + api.on(RooCodeEventName.Message, messageHandler) + + try { + const parentTaskId = await api.startNewTask({ + configuration: { + mode: "ask", + alwaysAllowModeSwitch: true, + alwaysAllowSubtasks: true, + autoApprovalEnabled: true, + enableCheckpoints: false, + }, + text: SUBTASK_PARENT_PROMPT, + }) + + let spawnedTaskId: string | undefined + await waitForStage("wait for spawned subtask", () => { + const currentTaskStack = api.getCurrentTaskStack() + const currentTaskId = currentTaskStack[currentTaskStack.length - 1] + if (currentTaskId && currentTaskId !== parentTaskId) { + spawnedTaskId = currentTaskId + return true + } + return false + }) + await waitForStage( + "wait for delegated child followup ask", + () => asks[spawnedTaskId!]?.some(({ type, ask }) => type === "ask" && ask === "followup") ?? false, + ) + + await api.cancelCurrentTask() + + await sleep(2_000) + + assert.ok( + messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === + undefined, + "Parent task should not have resumed after subtask cancellation", + ) + + const anotherTaskId = await api.startNewTask({ text: SUBTASK_CHILD_PROMPT }) + await waitForStage( + "wait for standalone child followup ask", + () => asks[anotherTaskId]?.some(({ type, ask }) => type === "ask" && ask === "followup") ?? false, + ) + await api.sendMessage(SUBTASK_CHILD_FOLLOWUP_ANSWER) + await waitUntilCompleted({ api, taskId: anotherTaskId }) + + await sleep(2_000) + + assert.ok( + messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === + undefined, + "Parent task should not have resumed after subtask cancellation", + ) + + await api.clearCurrentTask() + } finally { + api.off(RooCodeEventName.Message, messageHandler) + } }) }) From f98c4def3ac2c90200e7582f136c10509bd1bc36 Mon Sep 17 00:00:00 2001 From: Roomote Date: Tue, 19 May 2026 21:00:11 +0000 Subject: [PATCH 2/8] test: tighten subtasks cancellation replay --- apps/vscode-e2e/src/fixtures/subtasks.ts | 41 ++++++++++ apps/vscode-e2e/src/suite/subtasks.test.ts | 56 +++++++++++-- src/core/webview/ClineProvider.ts | 32 +++++++- .../ClineProvider.flicker-free-cancel.spec.ts | 78 +++++++++++++++++++ 4 files changed, 200 insertions(+), 7 deletions(-) diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index a79c09a50d..a6167ace34 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -1,8 +1,11 @@ import { LLMock } from "@copilotkit/aimock" +import { toolResultContains } from "./tool-result" + export const SUBTASK_PARENT_PROMPT = "SUBTASK_PARENT_CANCELLATION_SMOKE" export const SUBTASK_CHILD_PROMPT = "SUBTASK_CHILD_CALCULATOR_SMOKE" export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9" +const INTERRUPTED_TOOL_RESULT = "Task was interrupted before this tool call could be completed." export function addSubtaskFixtures(mock: InstanceType) { mock.addFixture({ @@ -44,6 +47,27 @@ export function addSubtaskFixtures(mock: InstanceType) { mock.addFixture({ match: { toolCallId: "call_subtasks_child_followup_001", + predicate: (req) => toolResultContains(req, "call_subtasks_child_followup_001", [INTERRUPTED_TOOL_RESULT]), + }, + response: { + toolCalls: [ + { + name: "ask_followup_question", + arguments: JSON.stringify({ + question: "What is the square root of 81?", + follow_up: [{ text: SUBTASK_CHILD_FOLLOWUP_ANSWER }], + }), + id: "call_subtasks_child_followup_resume_002", + }, + ], + }, + }) + + mock.addFixture({ + match: { + toolCallId: "call_subtasks_child_followup_001", + predicate: (req) => + toolResultContains(req, "call_subtasks_child_followup_001", [SUBTASK_CHILD_FOLLOWUP_ANSWER]), }, response: { toolCalls: [ @@ -56,6 +80,23 @@ export function addSubtaskFixtures(mock: InstanceType) { }, }) + mock.addFixture({ + match: { + toolCallId: "call_subtasks_child_followup_resume_002", + predicate: (req) => + toolResultContains(req, "call_subtasks_child_followup_resume_002", [SUBTASK_CHILD_FOLLOWUP_ANSWER]), + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: "9" }), + id: "call_subtasks_child_completion_resume_003", + }, + ], + }, + }) + mock.addFixture({ match: { toolCallId: "call_subtasks_parent_new_task_001", diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index 56100b2075..d18fb1fd05 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -4,7 +4,7 @@ import { RooCodeEventName, type ClineMessage } from "@roo-code/types" import { setDefaultSuiteTimeout } from "./test-utils" import { sleep, waitFor, waitUntilCompleted } from "./utils" -import { SUBTASK_CHILD_FOLLOWUP_ANSWER, SUBTASK_CHILD_PROMPT, SUBTASK_PARENT_PROMPT } from "../fixtures/subtasks" +import { SUBTASK_CHILD_FOLLOWUP_ANSWER, SUBTASK_PARENT_PROMPT } from "../fixtures/subtasks" suite("Roo Code Subtasks", function () { setDefaultSuiteTimeout(this) @@ -34,6 +34,21 @@ suite("Roo Code Subtasks", function () { } } + const findCompletionText = (taskId: string) => + messages[taskId] + ?.filter( + (message) => + message.type === "say" && (message.say === "completion_result" || message.say === "text"), + ) + .map((message) => message.text?.trim()) + .find((text): text is string => !!text) + + const findErrorText = (taskId: string) => + messages[taskId] + ?.filter((message) => message.type === "say" && message.say === "error") + .map((message) => message.text?.trim()) + .find((text): text is string => !!text) + api.on(RooCodeEventName.Message, messageHandler) try { @@ -62,6 +77,9 @@ suite("Roo Code Subtasks", function () { "wait for delegated child followup ask", () => asks[spawnedTaskId!]?.some(({ type, ask }) => type === "ask" && ask === "followup") ?? false, ) + const cancelledChildTaskId = spawnedTaskId! + const delegatedFollowupCount = + asks[cancelledChildTaskId]?.filter(({ type, ask }) => type === "ask" && ask === "followup").length ?? 0 await api.cancelCurrentTask() @@ -73,13 +91,41 @@ suite("Roo Code Subtasks", function () { "Parent task should not have resumed after subtask cancellation", ) - const anotherTaskId = await api.startNewTask({ text: SUBTASK_CHILD_PROMPT }) await waitForStage( - "wait for standalone child followup ask", - () => asks[anotherTaskId]?.some(({ type, ask }) => type === "ask" && ask === "followup") ?? false, + "wait for cancelled child task to remain active", + () => api.getCurrentTaskStack().at(-1) === cancelledChildTaskId, + ) + await waitForStage( + "wait for cancelled child resume ask", + () => + asks[cancelledChildTaskId]?.some(({ type, ask }) => type === "ask" && ask === "resume_task") ?? + false, + ) + await api.approveCurrentAsk() + await waitForStage( + "wait for resumed child followup ask", + () => + (asks[cancelledChildTaskId]?.filter(({ type, ask }) => type === "ask" && ask === "followup") + .length ?? 0) > delegatedFollowupCount, ) await api.sendMessage(SUBTASK_CHILD_FOLLOWUP_ANSWER) - await waitUntilCompleted({ api, taskId: anotherTaskId }) + await waitUntilCompleted({ api, taskId: cancelledChildTaskId }) + + assert.strictEqual( + findErrorText(cancelledChildTaskId), + undefined, + "Cancelled child should not emit an error", + ) + assert.strictEqual( + findCompletionText(cancelledChildTaskId), + "9", + "Cancelled child should complete with `9`", + ) + assert.strictEqual( + api.getCurrentTaskStack().at(-1), + cancelledChildTaskId, + "Cancelled child should stay active after resuming from cancellation", + ) await sleep(2_000) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index d5d34d0e91..bc3746035b 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2903,8 +2903,8 @@ export class ClineProvider } // Preserve parent and root task information for history item. - const rootTask = task.rootTask - const parentTask = task.parentTask + let rootTask = task.rootTask + let parentTask = task.parentTask // Mark this as a user-initiated cancellation so provider-only rehydration can occur task.abortReason = "user_cancelled" @@ -2962,6 +2962,34 @@ export class ClineProvider return } + if (task.parentTaskId) { + try { + const { historyItem: parentHistory } = await this.getTaskWithId(task.parentTaskId) + + if (parentHistory.status === "delegated" && parentHistory.awaitingChildId === task.taskId) { + await this.updateTaskHistory({ + ...parentHistory, + status: "active", + awaitingChildId: undefined, + }) + + historyItem = { + ...historyItem, + parentTaskId: undefined, + rootTaskId: undefined, + } + parentTask = undefined + rootTask = undefined + } + } catch (error) { + this.log( + `[cancelTask] Failed to detach delegated parent for ${task.taskId}: ${ + error instanceof Error ? error.message : String(error) + }`, + ) + } + } + // Clears task again, so we need to abortTask manually above. await this.createTaskWithHistoryItem({ ...historyItem, rootTask, parentTask }) } diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts index 4bb01347a3..77c9969c48 100644 --- a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -316,4 +316,82 @@ describe("ClineProvider flicker-free cancel", () => { expect((provider as any).clineStack[0]).toBe(mockParentTask) expect((provider as any).clineStack[1]).toBe(mockTask2) }) + + it("detaches a cancelled delegated child before rehydrating it", async () => { + const mockRootTask = { taskId: "root-1" } + const mockParentTask = { taskId: "parent-1" } + const childHistory: HistoryItem = { + id: "child-1", + number: 2, + task: "child task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + parentTaskId: "parent-1", + rootTaskId: "root-1", + } + const parentHistory: HistoryItem = { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + status: "delegated", + awaitingChildId: "child-1", + delegatedToId: "child-1", + } + + Object.assign(mockTask1, { + taskId: "child-1", + instanceId: "instance-child", + rootTask: mockRootTask, + parentTask: mockParentTask, + parentTaskId: "parent-1", + cancelCurrentRequest: vi.fn(), + abortTask: vi.fn(), + abandoned: false, + isStreaming: false, + didFinishAbortingStream: true, + isWaitingForFirstChunk: false, + }) + ;(provider as any).clineStack = [mockTask1] + provider.getTaskWithId = vi.fn().mockImplementation((id) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: childHistory }) + } + if (id === "parent-1") { + return Promise.resolve({ historyItem: parentHistory }) + } + throw new Error(`unexpected task lookup: ${id}`) + }) as any + + const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([]) + const createTaskWithHistoryItemSpy = vi + .spyOn(provider, "createTaskWithHistoryItem") + .mockResolvedValue(undefined as any) + + await provider.cancelTask() + + expect(updateTaskHistorySpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: "parent-1", + status: "active", + awaitingChildId: undefined, + }), + ) + expect(createTaskWithHistoryItemSpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: "child-1", + parentTaskId: undefined, + rootTaskId: undefined, + parentTask: undefined, + rootTask: undefined, + }), + ) + }) }) From e7b7b584b058beabfeac37ee74e31ab558269acc Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Wed, 20 May 2026 03:32:21 +0000 Subject: [PATCH 3/8] test(e2e): validation and bumping codecov --- apps/vscode-e2e/src/fixtures/subtasks.ts | 11 ++- src/core/tools/AttemptCompletionTool.ts | 25 ++--- .../__tests__/attemptCompletionTool.spec.ts | 96 +++++++++++++++++++ src/core/webview/ClineProvider.ts | 5 - .../ClineProvider.flicker-free-cancel.spec.ts | 66 ++++++++++++- 5 files changed, 180 insertions(+), 23 deletions(-) diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index a6167ace34..1d46291474 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -2,15 +2,18 @@ import { LLMock } from "@copilotkit/aimock" import { toolResultContains } from "./tool-result" -export const SUBTASK_PARENT_PROMPT = "SUBTASK_PARENT_CANCELLATION_SMOKE" -export const SUBTASK_CHILD_PROMPT = "SUBTASK_CHILD_CALCULATOR_SMOKE" +const SUBTASK_PARENT_MARKER = "SUBTASK_PARENT_CANCELLATION_SMOKE" +const SUBTASK_CHILD_MARKER = "SUBTASK_CHILD_CALCULATOR_SMOKE" + +export const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` +export const SUBTASK_PARENT_PROMPT = `${SUBTASK_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_CHILD_PROMPT}" Do not answer directly.` export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9" const INTERRUPTED_TOOL_RESULT = "Task was interrupted before this tool call could be completed." export function addSubtaskFixtures(mock: InstanceType) { mock.addFixture({ match: { - userMessage: new RegExp(SUBTASK_PARENT_PROMPT), + userMessage: new RegExp(SUBTASK_PARENT_MARKER), }, response: { toolCalls: [ @@ -28,7 +31,7 @@ export function addSubtaskFixtures(mock: InstanceType) { mock.addFixture({ match: { - userMessage: new RegExp(SUBTASK_CHILD_PROMPT), + userMessage: new RegExp(SUBTASK_CHILD_MARKER), }, response: { toolCalls: [ diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index a70576d75f..74946450b1 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -96,18 +96,21 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { // This shows the user the completion result and waits for acceptance // without injecting another tool_result to the parent } else if (status === "active") { - // Normal subtask completion - do delegation - const delegation = await this.delegateToParent( - task, - result, - provider, - askFinishSubTaskApproval, - pushToolResult, - ) - if (delegation === "delegated") { - this.emitTaskCompleted(task) + const { historyItem: parentHistory } = await provider.getTaskWithId(task.parentTaskId) + + if (parentHistory.status === "delegated" && parentHistory.awaitingChildId === task.taskId) { + const delegation = await this.delegateToParent( + task, + result, + provider, + askFinishSubTaskApproval, + pushToolResult, + ) + if (delegation === "delegated") { + this.emitTaskCompleted(task) + } + if (delegation !== "continue") return } - if (delegation !== "continue") return } else { // Unexpected status (undefined or "delegated") - log error and skip delegation // undefined indicates a bug in status persistence during child creation diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 6c9b9a2ccc..2f903c5a44 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -484,6 +484,102 @@ describe("attemptCompletionTool", () => { }) describe("completion lifecycle", () => { + it("delegates an active subtask completion only when the parent is awaiting that child", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "9" }, + nativeArgs: { result: "9" }, + partial: false, + } + const mockProvider = { + getTaskWithId: vi.fn().mockImplementation((id: string) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: { id, status: "active" } }) + } + if (id === "parent-1") { + return Promise.resolve({ + historyItem: { id, status: "delegated", awaitingChildId: "child-1" }, + }) + } + throw new Error(`unexpected task id ${id}`) + }), + reopenParentFromDelegation: vi.fn().mockResolvedValue(undefined), + } + + Object.assign(mockTask, { + taskId: "child-1", + parentTaskId: "parent-1", + providerRef: { deref: () => mockProvider }, + }) + mockAskFinishSubTaskApproval.mockResolvedValue(true) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + expect(mockAskFinishSubTaskApproval).toHaveBeenCalled() + expect(mockProvider.reopenParentFromDelegation).toHaveBeenCalledWith({ + parentTaskId: "parent-1", + childTaskId: "child-1", + completionResultSummary: "9", + }) + expect(mockTask.ask).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalledWith("") + }) + + it("does not delegate a lineage-preserving subtask when the parent is no longer awaiting it", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "9" }, + nativeArgs: { result: "9" }, + partial: false, + } + const mockProvider = { + getTaskWithId: vi.fn().mockImplementation((id: string) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: { id, status: "active" } }) + } + if (id === "parent-1") { + return Promise.resolve({ + historyItem: { id, status: "active", awaitingChildId: undefined }, + }) + } + throw new Error(`unexpected task id ${id}`) + }), + reopenParentFromDelegation: vi.fn().mockResolvedValue(undefined), + } + + Object.assign(mockTask, { + taskId: "child-1", + parentTaskId: "parent-1", + providerRef: { deref: () => mockProvider }, + }) + mockAskFinishSubTaskApproval.mockResolvedValue(true) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + expect(mockAskFinishSubTaskApproval).not.toHaveBeenCalled() + expect(mockProvider.reopenParentFromDelegation).not.toHaveBeenCalled() + expect(mockTask.ask).toHaveBeenCalledWith("completion_result", "", false) + expect(mockCaptureTaskCompleted).toHaveBeenCalledWith("child-1") + }) + it("emits TaskCompleted only when completion is accepted", async () => { const block: AttemptCompletionToolUse = { type: "tool_use", diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index bc3746035b..fa449b4fb2 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2973,11 +2973,6 @@ export class ClineProvider awaitingChildId: undefined, }) - historyItem = { - ...historyItem, - parentTaskId: undefined, - rootTaskId: undefined, - } parentTask = undefined rootTask = undefined } diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts index 77c9969c48..766cd69eba 100644 --- a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -317,7 +317,7 @@ describe("ClineProvider flicker-free cancel", () => { expect((provider as any).clineStack[1]).toBe(mockTask2) }) - it("detaches a cancelled delegated child before rehydrating it", async () => { + it("detaches runtime parent links for a cancelled delegated child while preserving history lineage", async () => { const mockRootTask = { taskId: "root-1" } const mockParentTask = { taskId: "parent-1" } const childHistory: HistoryItem = { @@ -387,11 +387,71 @@ describe("ClineProvider flicker-free cancel", () => { expect(createTaskWithHistoryItemSpy).toHaveBeenCalledWith( expect.objectContaining({ id: "child-1", - parentTaskId: undefined, - rootTaskId: undefined, + parentTaskId: "parent-1", + rootTaskId: "root-1", parentTask: undefined, rootTask: undefined, }), ) }) + + it("continues rehydrating a cancelled child when delegated parent detach fails", async () => { + const mockRootTask = { taskId: "root-1" } + const mockParentTask = { taskId: "parent-1" } + const childHistory: HistoryItem = { + id: "child-1", + number: 2, + task: "child task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + parentTaskId: "parent-1", + rootTaskId: "root-1", + } + + Object.assign(mockTask1, { + taskId: "child-1", + instanceId: "instance-child", + rootTask: mockRootTask, + parentTask: mockParentTask, + parentTaskId: "parent-1", + cancelCurrentRequest: vi.fn(), + abortTask: vi.fn(), + abandoned: false, + isStreaming: false, + didFinishAbortingStream: true, + isWaitingForFirstChunk: false, + }) + ;(provider as any).clineStack = [mockTask1] + provider.getTaskWithId = vi.fn().mockImplementation((id) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: childHistory }) + } + if (id === "parent-1") { + return Promise.reject(new Error("parent lookup failed")) + } + throw new Error(`unexpected task lookup: ${id}`) + }) as any + + const createTaskWithHistoryItemSpy = vi + .spyOn(provider, "createTaskWithHistoryItem") + .mockResolvedValue(undefined as any) + + await provider.cancelTask() + + expect(mockOutputChannel.appendLine).toHaveBeenCalledWith( + expect.stringContaining("[cancelTask] Failed to detach delegated parent for child-1: parent lookup failed"), + ) + expect(createTaskWithHistoryItemSpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: "child-1", + parentTaskId: "parent-1", + rootTaskId: "root-1", + parentTask: mockParentTask, + rootTask: mockRootTask, + }), + ) + }) }) From 340dfba793a652fd4465fcec69eea71b57c27c05 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Sat, 23 May 2026 20:53:54 +0000 Subject: [PATCH 4/8] fix: prevent child delegation start after parent metadata failure --- src/__tests__/provider-delegation.spec.ts | 50 +++++++++++++++++++++++ src/core/webview/ClineProvider.ts | 5 ++- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index 4b04fb5bbb..34d9a68547 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -142,4 +142,54 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { // Verify ordering: createTask → updateTaskHistory → child.start expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"]) }) + + it("does not start the child when parent delegation metadata cannot be persisted", async () => { + const parentTask = { taskId: "parent-1", emit: vi.fn() } as any + const childStart = vi.fn() + const persistError = new Error("history write failed") + + const providerEmit = vi.fn() + const updateTaskHistory = vi.fn().mockRejectedValue(persistError) + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const createTask = vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart }) + const handleModeSwitch = vi.fn().mockResolvedValue(undefined) + const getTaskWithId = vi.fn().mockResolvedValue({ + historyItem: { + id: "parent-1", + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + childIds: [], + }, + }) + + const provider = { + emit: providerEmit, + getCurrentTask: vi.fn(() => parentTask), + removeClineFromStack, + createTask, + getTaskWithId, + updateTaskHistory, + handleModeSwitch, + log: vi.fn(), + } as unknown as ClineProvider + + await expect( + (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + }), + ).rejects.toThrow("history write failed") + + expect(createTask).toHaveBeenCalledWith("Do something", undefined, parentTask, { + initialTodos: [], + initialStatus: "active", + startTask: false, + }) + expect(childStart).not.toHaveBeenCalled() + expect(providerEmit).not.toHaveBeenCalledWith(RooCodeEventName.TaskDelegated, "parent-1", "child-1") + }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index fa449b4fb2..687c7cd143 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2869,7 +2869,9 @@ export class ClineProvider }) await this.addClineToStack(task) - task.start() + if (options.startTask !== false) { + task.start() + } this.log( `[createTask] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`, @@ -3267,6 +3269,7 @@ export class ClineProvider (err as Error)?.message ?? String(err) }`, ) + throw err } // 6) Start the child task now that parent metadata is safely persisted. From e565a4c71c220060dea9d42ef6b74d309727da2a Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Sat, 23 May 2026 20:56:35 +0000 Subject: [PATCH 5/8] chore: address delegation review cleanup --- apps/vscode-e2e/src/fixtures/subtasks.ts | 55 ++++------ src/__tests__/provider-delegation.spec.ts | 101 ++++++++++++++++-- src/core/tools/AttemptCompletionTool.ts | 12 ++- .../__tests__/attemptCompletionTool.spec.ts | 2 +- src/core/webview/ClineProvider.ts | 54 +++++++++- 5 files changed, 175 insertions(+), 49 deletions(-) diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index 1d46291474..cdf299541a 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -1,15 +1,32 @@ import { LLMock } from "@copilotkit/aimock" +import type { ChatCompletionRequest } from "@copilotkit/aimock" import { toolResultContains } from "./tool-result" const SUBTASK_PARENT_MARKER = "SUBTASK_PARENT_CANCELLATION_SMOKE" const SUBTASK_CHILD_MARKER = "SUBTASK_CHILD_CALCULATOR_SMOKE" -export const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` +const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` export const SUBTASK_PARENT_PROMPT = `${SUBTASK_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_CHILD_PROMPT}" Do not answer directly.` export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9" const INTERRUPTED_TOOL_RESULT = "Task was interrupted before this tool call could be completed." +const completionAfterAnswer = (followupId: string, completionId: string) => ({ + match: { + toolCallId: followupId, + predicate: (req: ChatCompletionRequest) => toolResultContains(req, followupId, [SUBTASK_CHILD_FOLLOWUP_ANSWER]), + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: "9" }), + id: completionId, + }, + ], + }, +}) + export function addSubtaskFixtures(mock: InstanceType) { mock.addFixture({ match: { @@ -66,39 +83,11 @@ export function addSubtaskFixtures(mock: InstanceType) { }, }) - mock.addFixture({ - match: { - toolCallId: "call_subtasks_child_followup_001", - predicate: (req) => - toolResultContains(req, "call_subtasks_child_followup_001", [SUBTASK_CHILD_FOLLOWUP_ANSWER]), - }, - response: { - toolCalls: [ - { - name: "attempt_completion", - arguments: JSON.stringify({ result: "9" }), - id: "call_subtasks_child_completion_002", - }, - ], - }, - }) + mock.addFixture(completionAfterAnswer("call_subtasks_child_followup_001", "call_subtasks_child_completion_002")) - mock.addFixture({ - match: { - toolCallId: "call_subtasks_child_followup_resume_002", - predicate: (req) => - toolResultContains(req, "call_subtasks_child_followup_resume_002", [SUBTASK_CHILD_FOLLOWUP_ANSWER]), - }, - response: { - toolCalls: [ - { - name: "attempt_completion", - arguments: JSON.stringify({ result: "9" }), - id: "call_subtasks_child_completion_resume_003", - }, - ], - }, - }) + mock.addFixture( + completionAfterAnswer("call_subtasks_child_followup_resume_002", "call_subtasks_child_completion_resume_003"), + ) mock.addFixture({ match: { diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index 34d9a68547..3f75d1233a 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -98,7 +98,11 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { it("calls child.start() only after parent metadata is persisted (no race condition)", async () => { const callOrder: string[] = [] - const parentTask = { taskId: "parent-1", emit: vi.fn() } as any + const parentTask = { + taskId: "parent-1", + emit: vi.fn(), + getTaskMode: vi.fn().mockResolvedValue("architect"), + } as any const childStart = vi.fn(() => callOrder.push("child.start")) const updateTaskHistory = vi.fn(async () => { @@ -143,25 +147,32 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"]) }) - it("does not start the child when parent delegation metadata cannot be persisted", async () => { - const parentTask = { taskId: "parent-1", emit: vi.fn() } as any + it("removes the paused child when parent delegation metadata cannot be persisted", async () => { + const parentTask = { + taskId: "parent-1", + emit: vi.fn(), + getTaskMode: vi.fn().mockResolvedValue("architect"), + } as any const childStart = vi.fn() const persistError = new Error("history write failed") const providerEmit = vi.fn() const updateTaskHistory = vi.fn().mockRejectedValue(persistError) const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const deleteTaskFromState = vi.fn().mockResolvedValue(undefined) const createTask = vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart }) + const createTaskWithHistoryItem = vi.fn().mockResolvedValue(undefined) const handleModeSwitch = vi.fn().mockResolvedValue(undefined) + const parentHistory = { + id: "parent-1", + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + childIds: [], + } const getTaskWithId = vi.fn().mockResolvedValue({ - historyItem: { - id: "parent-1", - task: "Parent", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - childIds: [], - }, + historyItem: parentHistory, }) const provider = { @@ -171,6 +182,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { createTask, getTaskWithId, updateTaskHistory, + deleteTaskFromState, + createTaskWithHistoryItem, handleModeSwitch, log: vi.fn(), } as unknown as ClineProvider @@ -190,6 +203,72 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { startTask: false, }) expect(childStart).not.toHaveBeenCalled() + expect(removeClineFromStack).toHaveBeenNthCalledWith(1, { skipDelegationRepair: true }) + expect(removeClineFromStack).toHaveBeenNthCalledWith(2, { skipDelegationRepair: true }) + expect(deleteTaskFromState).toHaveBeenCalledWith("child-1") + expect(createTaskWithHistoryItem).toHaveBeenCalledWith( + { ...parentHistory, mode: "architect" }, + { startTask: false }, + ) expect(providerEmit).not.toHaveBeenCalledWith(RooCodeEventName.TaskDelegated, "parent-1", "child-1") }) + + it("logs rollback cleanup failures while preserving the parent metadata error", async () => { + const parentTask = { + taskId: "parent-1", + emit: vi.fn(), + getTaskMode: vi.fn().mockRejectedValue(new Error("mode unavailable")), + } as any + const childStart = vi.fn() + const persistError = new Error("history write failed") + const log = vi.fn() + + const removeClineFromStack = vi + .fn() + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error("child stack cleanup failed")) + const deleteTaskFromState = vi.fn().mockRejectedValue(new Error("child history cleanup failed")) + const createTaskWithHistoryItem = vi.fn().mockRejectedValue(new Error("parent rehydrate failed")) + const parentHistory = { + id: "parent-1", + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + childIds: [], + mode: "ask", + } + + const provider = { + emit: vi.fn(), + getCurrentTask: vi.fn(() => parentTask), + removeClineFromStack, + createTask: vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentHistory }), + updateTaskHistory: vi.fn().mockRejectedValue(persistError), + deleteTaskFromState, + createTaskWithHistoryItem, + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + log, + } as unknown as ClineProvider + + await expect( + (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + }), + ).rejects.toThrow("history write failed") + + expect(childStart).not.toHaveBeenCalled() + expect(deleteTaskFromState).toHaveBeenCalledWith("child-1") + expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistory, { startTask: false }) + expect(log).toHaveBeenCalledWith(expect.stringContaining("Failed to capture parent mode")) + expect(log).toHaveBeenCalledWith( + expect.stringContaining("Failed to remove child child-1 after parent metadata"), + ) + expect(log).toHaveBeenCalledWith(expect.stringContaining("Failed to remove child child-1 history")) + expect(log).toHaveBeenCalledWith(expect.stringContaining("Failed to rehydrate parent parent-1")) + }) }) diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index 74946450b1..779caa496f 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -86,6 +86,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { // to prevent duplicate tool_results when user revisits from history const provider = task.providerRef.deref() as DelegationProvider | undefined if (provider) { + let historyLookupTaskId = task.taskId try { const { historyItem } = await provider.getTaskWithId(task.taskId) const status = historyItem?.status @@ -96,9 +97,13 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { // This shows the user the completion result and waits for acceptance // without injecting another tool_result to the parent } else if (status === "active") { + historyLookupTaskId = task.parentTaskId const { historyItem: parentHistory } = await provider.getTaskWithId(task.parentTaskId) - if (parentHistory.status === "delegated" && parentHistory.awaitingChildId === task.taskId) { + if ( + parentHistory?.status === "delegated" && + parentHistory?.awaitingChildId === task.taskId + ) { const delegation = await this.delegateToParent( task, result, @@ -110,6 +115,9 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { this.emitTaskCompleted(task) } if (delegation !== "continue") return + } else { + // Parent already detached, such as when the user cancelled this child. + // Fall through to the normal completion ask flow. } } else { // Unexpected status (undefined or "delegated") - log error and skip delegation @@ -124,7 +132,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { } catch (err) { // If we can't get the history, log error and skip delegation console.error( - `[AttemptCompletionTool] Failed to get history for task ${task.taskId}: ${(err as Error)?.message ?? String(err)}. ` + + `[AttemptCompletionTool] Failed to get history for task ${historyLookupTaskId}: ${(err as Error)?.message ?? String(err)}. ` + `Skipping delegation.`, ) // Fall through to normal completion ask flow diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 2f903c5a44..a013652634 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -534,7 +534,7 @@ describe("attemptCompletionTool", () => { expect(mockPushToolResult).toHaveBeenCalledWith("") }) - it("does not delegate a lineage-preserving subtask when the parent is no longer awaiting it", async () => { + it("does not resume the parent when the parent is no longer awaiting this child", async () => { const block: AttemptCompletionToolUse = { type: "tool_use", name: "attempt_completion", diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 687c7cd143..1fa6a351f2 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -484,7 +484,7 @@ export class ClineProvider try { const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) - if (parentHistory.status === "delegated" && parentHistory.awaitingChildId === childTaskId) { + if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === childTaskId) { await this.updateTaskHistory({ ...parentHistory, status: "active", @@ -2968,13 +2968,16 @@ export class ClineProvider try { const { historyItem: parentHistory } = await this.getTaskWithId(task.parentTaskId) - if (parentHistory.status === "delegated" && parentHistory.awaitingChildId === task.taskId) { + if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === task.taskId) { await this.updateTaskHistory({ ...parentHistory, status: "active", awaitingChildId: undefined, }) + this.log( + `[cancelTask] Detached delegated parent ${task.parentTaskId}: delegated → active (child ${task.taskId} cancelled)`, + ) parentTask = undefined rootTask = undefined } @@ -3172,6 +3175,16 @@ export class ClineProvider `[delegateParentAndOpenChild] Parent mismatch: expected ${parentTaskId}, current ${parent.taskId}`, ) } + let parentModeBeforeDelegation: string | undefined + try { + parentModeBeforeDelegation = await parent.getTaskMode() + } catch (error) { + this.log( + `[delegateParentAndOpenChild] Failed to capture parent mode for ${parentTaskId} before delegation (non-fatal): ${ + error instanceof Error ? error.message : String(error) + }`, + ) + } // 2) Flush pending tool results to API history BEFORE disposing the parent. // This is critical: when tools are called before new_task, // their tool_result blocks are in userMessageContent but not yet saved to API history. @@ -3252,8 +3265,10 @@ export class ClineProvider }) // 5) Persist parent delegation metadata BEFORE the child starts writing. + let parentHistoryBeforeDelegation: HistoryItem | undefined try { const { historyItem } = await this.getTaskWithId(parentTaskId) + parentHistoryBeforeDelegation = historyItem const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId])) const updatedHistory: typeof historyItem = { ...historyItem, @@ -3269,6 +3284,41 @@ export class ClineProvider (err as Error)?.message ?? String(err) }`, ) + try { + await this.removeClineFromStack({ skipDelegationRepair: true }) + } catch (cleanupErr) { + this.log( + `[delegateParentAndOpenChild] Failed to remove child ${child.taskId} after parent metadata persistence failure (non-fatal): ${ + (cleanupErr as Error)?.message ?? String(cleanupErr) + }`, + ) + } + try { + await this.deleteTaskFromState(child.taskId) + } catch (cleanupErr) { + this.log( + `[delegateParentAndOpenChild] Failed to remove child ${child.taskId} history after parent metadata persistence failure (non-fatal): ${ + (cleanupErr as Error)?.message ?? String(cleanupErr) + }`, + ) + } + if (parentHistoryBeforeDelegation) { + try { + await this.createTaskWithHistoryItem( + { + ...parentHistoryBeforeDelegation, + mode: parentModeBeforeDelegation ?? parentHistoryBeforeDelegation.mode, + }, + { startTask: false }, + ) + } catch (cleanupErr) { + this.log( + `[delegateParentAndOpenChild] Failed to rehydrate parent ${parentTaskId} after parent metadata persistence failure (non-fatal): ${ + (cleanupErr as Error)?.message ?? String(cleanupErr) + }`, + ) + } + } throw err } From dd9e9310aa18cc75723b21c78c186b9db9a6b7df Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 25 May 2026 01:43:46 +0000 Subject: [PATCH 6/8] fix: delegated subtask lifecycle races --- .../history-resume-delegation.spec.ts | 116 ++++- src/core/tools/AttemptCompletionTool.ts | 11 +- .../__tests__/attemptCompletionTool.spec.ts | 53 ++- src/core/webview/ClineProvider.ts | 426 ++++++++++-------- .../ClineProvider.flicker-free-cancel.spec.ts | 7 +- 5 files changed, 419 insertions(+), 194 deletions(-) diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index a78c41b7c0..40fd95d9da 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -110,6 +110,7 @@ describe("History resume delegation - parent metadata transitions", () => { // Verify child closed and parent reopened with updated metadata expect(removeClineFromStack).toHaveBeenCalledTimes(1) + expect(removeClineFromStack).toHaveBeenCalledWith({ skipDelegationRepair: true }) expect(createTaskWithHistoryItem).toHaveBeenCalledWith( expect.objectContaining({ status: "active", @@ -502,7 +503,7 @@ describe("History resume delegation - parent metadata transitions", () => { childTaskId: "child-rpd06", completionResultSummary: "Subtask finished despite overwrite failures", }), - ).resolves.toBeUndefined() + ).resolves.toBe(true) expect(parentInstance.overwriteClineMessages).toHaveBeenCalledTimes(1) expect(parentInstance.overwriteApiConversationHistory).toHaveBeenCalledTimes(1) @@ -705,7 +706,7 @@ describe("History resume delegation - parent metadata transitions", () => { childTaskId: "child-rpd04", completionResultSummary: "Child completion with persistence failure", }), - ).resolves.toBeUndefined() + ).resolves.toBe(true) expect(logSpy).toHaveBeenCalledWith( expect.stringContaining( @@ -760,7 +761,7 @@ describe("History resume delegation - parent metadata transitions", () => { childTaskId: "c5", completionResultSummary: "Result", }), - ).resolves.toBeUndefined() + ).resolves.toBe(true) // Verify saves still occurred with just the injected message expect(saveTaskMessages).toHaveBeenCalledWith( @@ -784,4 +785,113 @@ describe("History resume delegation - parent metadata transitions", () => { }), ) }) + + it("reopenParentFromDelegation aborts when parent is already active (stale-delegation guard)", async () => { + const logSpy = vi.fn() + const updateTaskHistory = vi.fn() + const saveTaskMessagesMock = vi.mocked(saveTaskMessages) + const saveApiMessagesMock = vi.mocked(saveApiMessages) + + const makeProvider = (historyItem: object) => + ({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem }), + emit: vi.fn(), + log: logSpy, + getCurrentTask: vi.fn(() => null), + removeClineFromStack: vi.fn(), + createTaskWithHistoryItem: vi.fn(), + updateTaskHistory, + }) as unknown as ClineProvider + + const providerActive = makeProvider({ + id: "parent-guard", + status: "active", + awaitingChildId: undefined, + }) + await expect( + (ClineProvider.prototype as any).reopenParentFromDelegation.call(providerActive, { + parentTaskId: "parent-guard", + childTaskId: "child-guard", + completionResultSummary: "should be ignored", + }), + ).resolves.toBe(false) + expect(saveTaskMessagesMock).not.toHaveBeenCalled() + expect(saveApiMessagesMock).not.toHaveBeenCalled() + expect(updateTaskHistory).not.toHaveBeenCalled() + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting")) + }) + + it("reopenParentFromDelegation aborts when in-process cancellation failed closed", async () => { + const logSpy = vi.fn() + const updateTaskHistory = vi.fn() + const saveTaskMessagesMock = vi.mocked(saveTaskMessages) + const saveApiMessagesMock = vi.mocked(saveApiMessages) + const provider = { + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ + historyItem: { + id: "parent-guard", + status: "delegated", + awaitingChildId: "child-guard", + }, + }), + emit: vi.fn(), + log: logSpy, + getCurrentTask: vi.fn(() => null), + removeClineFromStack: vi.fn(), + createTaskWithHistoryItem: vi.fn(), + updateTaskHistory, + cancelledDelegationChildIds: new Set(["child-guard"]), + } as unknown as ClineProvider + + await expect( + (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-guard", + childTaskId: "child-guard", + completionResultSummary: "should be ignored", + }), + ).resolves.toBe(false) + + expect(saveTaskMessagesMock).not.toHaveBeenCalled() + expect(saveApiMessagesMock).not.toHaveBeenCalled() + expect(updateTaskHistory).not.toHaveBeenCalled() + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting")) + }) + + it("reopenParentFromDelegation aborts when parent awaits a different child (stale-delegation guard)", async () => { + const logSpy = vi.fn() + const updateTaskHistory = vi.fn() + const saveTaskMessagesMock = vi.mocked(saveTaskMessages) + const saveApiMessagesMock = vi.mocked(saveApiMessages) + + const makeProvider = (historyItem: object) => + ({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem }), + emit: vi.fn(), + log: logSpy, + getCurrentTask: vi.fn(() => null), + removeClineFromStack: vi.fn(), + createTaskWithHistoryItem: vi.fn(), + updateTaskHistory, + }) as unknown as ClineProvider + + const providerWrongChild = makeProvider({ + id: "parent-guard", + status: "delegated", + awaitingChildId: "other-child", + }) + await expect( + (ClineProvider.prototype as any).reopenParentFromDelegation.call(providerWrongChild, { + parentTaskId: "parent-guard", + childTaskId: "child-guard", + completionResultSummary: "should be ignored", + }), + ).resolves.toBe(false) + expect(saveTaskMessagesMock).not.toHaveBeenCalled() + expect(saveApiMessagesMock).not.toHaveBeenCalled() + expect(updateTaskHistory).not.toHaveBeenCalled() + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting")) + }) }) diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index 779caa496f..3347f63f40 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -30,7 +30,7 @@ interface DelegationProvider { parentTaskId: string childTaskId: string completionResultSummary: string - }): Promise + }): Promise } export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { @@ -178,14 +178,17 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { return "denied" } - pushToolResult("") - - await provider.reopenParentFromDelegation({ + const didReopen = await provider.reopenParentFromDelegation({ parentTaskId: task.parentTaskId!, childTaskId: task.taskId, completionResultSummary: result, }) + if (didReopen === false) { + return "continue" + } + + pushToolResult("") return "delegated" } diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index a013652634..b64b6c57a0 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -504,7 +504,7 @@ describe("attemptCompletionTool", () => { } throw new Error(`unexpected task id ${id}`) }), - reopenParentFromDelegation: vi.fn().mockResolvedValue(undefined), + reopenParentFromDelegation: vi.fn().mockResolvedValue(true), } Object.assign(mockTask, { @@ -534,6 +534,57 @@ describe("attemptCompletionTool", () => { expect(mockPushToolResult).toHaveBeenCalledWith("") }) + it("falls through to standalone completion when parent delegation becomes stale after approval", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "9" }, + nativeArgs: { result: "9" }, + partial: false, + } + const mockProvider = { + getTaskWithId: vi.fn().mockImplementation((id: string) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: { id, status: "active" } }) + } + if (id === "parent-1") { + return Promise.resolve({ + historyItem: { id, status: "delegated", awaitingChildId: "child-1" }, + }) + } + throw new Error(`unexpected task id ${id}`) + }), + reopenParentFromDelegation: vi.fn().mockResolvedValue(false), + } + + Object.assign(mockTask, { + taskId: "child-1", + parentTaskId: "parent-1", + providerRef: { deref: () => mockProvider }, + }) + mockTask.ask = vi.fn().mockResolvedValue({ response: "messageResponse", text: "revise", images: [] }) + mockAskFinishSubTaskApproval.mockResolvedValue(true) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + expect(mockProvider.reopenParentFromDelegation).toHaveBeenCalledWith({ + parentTaskId: "parent-1", + childTaskId: "child-1", + completionResultSummary: "9", + }) + expect(mockTask.ask).toHaveBeenCalledWith("completion_result", "", false) + expect(mockPushToolResult).not.toHaveBeenCalledWith("") + expect(mockCaptureTaskCompleted).not.toHaveBeenCalled() + }) + it("does not resume the parent when the parent is no longer awaiting this child", async () => { const block: AttemptCompletionToolUse = { type: "tool_use", diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 1fa6a351f2..b01141450b 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -114,6 +114,31 @@ export type ClineProviderEvents = { clineCreated: [cline: Task] } +type DelegationLockHost = { + delegationTransitionLocks?: Map> +} + +function runDelegationTransition(host: DelegationLockHost, parentTaskId: string, fn: () => Promise): Promise { + host.delegationTransitionLocks ??= new Map() + const locks = host.delegationTransitionLocks + const previous = locks.get(parentTaskId) ?? Promise.resolve() + const current = previous.then(fn, fn) + const tail = current.then( + () => {}, + () => {}, + ) + + locks.set(parentTaskId, tail) + + tail.finally(() => { + if (locks.get(parentTaskId) === tail) { + locks.delete(parentTaskId) + } + }) + + return current +} + export class ClineProvider extends EventEmitter implements vscode.WebviewViewProvider, TelemetryPropertiesProvider, TaskProviderLike @@ -128,6 +153,8 @@ export class ClineProvider private webviewDisposables: vscode.Disposable[] = [] private view?: vscode.WebviewView | vscode.WebviewPanel private clineStack: Task[] = [] + private delegationTransitionLocks = new Map>() + private cancelledDelegationChildIds = new Set() private codeIndexStatusSubscription?: vscode.Disposable private codeIndexManager?: CodeIndexManager private _workspaceTracker?: WorkspaceTracker // workSpaceTracker read-only for access outside this class @@ -482,18 +509,20 @@ export class ClineProvider // child and will update the parent to point at the new child. if (parentTaskId && childTaskId && !options?.skipDelegationRepair) { try { - const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) - - if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === childTaskId) { - await this.updateTaskHistory({ - ...parentHistory, - status: "active", - awaitingChildId: undefined, - }) - this.log( - `[ClineProvider#removeClineFromStack] Repaired parent ${parentTaskId} metadata: delegated → active (child ${childTaskId} removed)`, - ) - } + await runDelegationTransition(this as unknown as DelegationLockHost, parentTaskId, async () => { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + + if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === childTaskId) { + await this.updateTaskHistory({ + ...parentHistory, + status: "active", + awaitingChildId: undefined, + }) + this.log( + `[ClineProvider#removeClineFromStack] Repaired parent ${parentTaskId} metadata: delegated → active (child ${childTaskId} removed)`, + ) + } + }) } catch (err) { // Non-fatal: log but do not block the pop operation. this.log( @@ -2966,22 +2995,31 @@ export class ClineProvider if (task.parentTaskId) { try { - const { historyItem: parentHistory } = await this.getTaskWithId(task.parentTaskId) + await runDelegationTransition(this as unknown as DelegationLockHost, task.parentTaskId, async () => { + const { historyItem: parentHistory } = await this.getTaskWithId(task.parentTaskId!) - if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === task.taskId) { - await this.updateTaskHistory({ - ...parentHistory, - status: "active", - awaitingChildId: undefined, - }) + if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === task.taskId) { + await this.updateTaskHistory({ + ...parentHistory, + status: "active", + awaitingChildId: undefined, + }) - this.log( - `[cancelTask] Detached delegated parent ${task.parentTaskId}: delegated → active (child ${task.taskId} cancelled)`, - ) - parentTask = undefined - rootTask = undefined - } + this.log( + `[cancelTask] Detached delegated parent ${task.parentTaskId}: delegated → active (child ${task.taskId} cancelled)`, + ) + parentTask = undefined + rootTask = undefined + this.cancelledDelegationChildIds.delete(task.taskId) + } + }) } catch (error) { + // Fail closed: if we cannot prove the parent was detached, keep the + // rehydrated child disconnected from runtime parent links and prevent + // this in-process child completion from reopening the parent later. + parentTask = undefined + rootTask = undefined + this.cancelledDelegationChildIds.add(task.taskId) this.log( `[cancelTask] Failed to detach delegated parent for ${task.taskId}: ${ error instanceof Error ? error.message : String(error) @@ -3342,196 +3380,218 @@ export class ClineProvider parentTaskId: string childTaskId: string completionResultSummary: string - }): Promise { + }): Promise { const { parentTaskId, childTaskId, completionResultSummary } = params - const globalStoragePath = this.contextProxy.globalStorageUri.fsPath + return await runDelegationTransition(this as unknown as DelegationLockHost, parentTaskId, async () => { + const globalStoragePath = this.contextProxy.globalStorageUri.fsPath - // 1) Load parent from history and current persisted messages - const { historyItem } = await this.getTaskWithId(parentTaskId) + // 1) Load parent from history and current persisted messages + const { historyItem } = await this.getTaskWithId(parentTaskId) - let parentClineMessages: ClineMessage[] = [] - try { - parentClineMessages = await readTaskMessages({ - taskId: parentTaskId, - globalStoragePath, - }) - } catch { - parentClineMessages = [] - } + // Guard: re-validate delegation state after the async approval gap. + // cancelTask() or removeClineFromStack() may have already detached the parent + // (setting status → "active", awaitingChildId → undefined) while the user was + // approving the subtask finish. If the parent no longer awaits this child, + // routing output back would corrupt an unrelated task. + if ( + (this.cancelledDelegationChildIds as Set | undefined)?.has(childTaskId) || + historyItem.status !== "delegated" || + historyItem.awaitingChildId !== childTaskId + ) { + this.log( + `[reopenParentFromDelegation] Aborting: parent ${parentTaskId} is no longer delegated to child ${childTaskId} ` + + `(status=${historyItem.status}, awaitingChildId=${historyItem.awaitingChildId})`, + ) + return false + } - let parentApiMessages: any[] = [] - try { - parentApiMessages = (await readApiMessages({ - taskId: parentTaskId, - globalStoragePath, - })) as any[] - } catch { - parentApiMessages = [] - } - - // 2) Inject synthetic records: UI subtask_result and update API tool_result - const ts = Date.now() - - // Defensive: ensure arrays - if (!Array.isArray(parentClineMessages)) parentClineMessages = [] - if (!Array.isArray(parentApiMessages)) parentApiMessages = [] - - const subtaskUiMessage: ClineMessage = { - type: "say", - say: "subtask_result", - text: completionResultSummary, - ts, - } - parentClineMessages.push(subtaskUiMessage) - await saveTaskMessages({ messages: parentClineMessages, taskId: parentTaskId, globalStoragePath }) - - // Find the tool_use_id from the last assistant message's new_task tool_use - let toolUseId: string | undefined - for (let i = parentApiMessages.length - 1; i >= 0; i--) { - const msg = parentApiMessages[i] - if (msg.role === "assistant" && Array.isArray(msg.content)) { - for (const block of msg.content) { - if (block.type === "tool_use" && block.name === "new_task") { - toolUseId = block.id - break + let parentClineMessages: ClineMessage[] = [] + try { + parentClineMessages = await readTaskMessages({ + taskId: parentTaskId, + globalStoragePath, + }) + } catch { + parentClineMessages = [] + } + + let parentApiMessages: any[] = [] + try { + parentApiMessages = (await readApiMessages({ + taskId: parentTaskId, + globalStoragePath, + })) as any[] + } catch { + parentApiMessages = [] + } + + // 2) Inject synthetic records: UI subtask_result and update API tool_result + const ts = Date.now() + + // Defensive: ensure arrays + if (!Array.isArray(parentClineMessages)) parentClineMessages = [] + if (!Array.isArray(parentApiMessages)) parentApiMessages = [] + + const subtaskUiMessage: ClineMessage = { + type: "say", + say: "subtask_result", + text: completionResultSummary, + ts, + } + parentClineMessages.push(subtaskUiMessage) + await saveTaskMessages({ messages: parentClineMessages, taskId: parentTaskId, globalStoragePath }) + + // Find the tool_use_id from the last assistant message's new_task tool_use + let toolUseId: string | undefined + for (let i = parentApiMessages.length - 1; i >= 0; i--) { + const msg = parentApiMessages[i] + if (msg.role === "assistant" && Array.isArray(msg.content)) { + for (const block of msg.content) { + if (block.type === "tool_use" && block.name === "new_task") { + toolUseId = block.id + break + } } + if (toolUseId) break } - if (toolUseId) break - } - } - - // Preferred: if the parent history contains the native tool_use for new_task, - // inject a matching tool_result for the Anthropic message contract: - // user → assistant (tool_use) → user (tool_result) - if (toolUseId) { - // Check if the last message is already a user message with a tool_result for this tool_use_id - // (in case this is a retry or the history was already updated) - const lastMsg = parentApiMessages[parentApiMessages.length - 1] - let alreadyHasToolResult = false - if (lastMsg?.role === "user" && Array.isArray(lastMsg.content)) { - for (const block of lastMsg.content) { - if (block.type === "tool_result" && block.tool_use_id === toolUseId) { - // Update the existing tool_result content - block.content = `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}` - alreadyHasToolResult = true - break + } + + // Preferred: if the parent history contains the native tool_use for new_task, + // inject a matching tool_result for the Anthropic message contract: + // user → assistant (tool_use) → user (tool_result) + if (toolUseId) { + // Check if the last message is already a user message with a tool_result for this tool_use_id + // (in case this is a retry or the history was already updated) + const lastMsg = parentApiMessages[parentApiMessages.length - 1] + let alreadyHasToolResult = false + if (lastMsg?.role === "user" && Array.isArray(lastMsg.content)) { + for (const block of lastMsg.content) { + if (block.type === "tool_result" && block.tool_use_id === toolUseId) { + // Update the existing tool_result content + block.content = `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}` + alreadyHasToolResult = true + break + } } } - } - // If no existing tool_result found, create a NEW user message with the tool_result - if (!alreadyHasToolResult) { + // If no existing tool_result found, create a NEW user message with the tool_result + if (!alreadyHasToolResult) { + parentApiMessages.push({ + role: "user", + content: [ + { + type: "tool_result" as const, + tool_use_id: toolUseId, + content: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, + }, + ], + ts, + }) + } + + // Validate the newly injected tool_result against the preceding assistant message. + // This ensures the tool_result's tool_use_id matches a tool_use in the immediately + // preceding assistant message (Anthropic API requirement). + const lastMessage = parentApiMessages[parentApiMessages.length - 1] + if (lastMessage?.role === "user") { + const validatedMessage = validateAndFixToolResultIds(lastMessage, parentApiMessages.slice(0, -1)) + parentApiMessages[parentApiMessages.length - 1] = validatedMessage + } + } else { + // If there is no corresponding tool_use in the parent API history, we cannot emit a + // tool_result. Fall back to a plain user text note so the parent can still resume. parentApiMessages.push({ role: "user", content: [ { - type: "tool_result" as const, - tool_use_id: toolUseId, - content: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, + type: "text" as const, + text: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, }, ], ts, }) } - // Validate the newly injected tool_result against the preceding assistant message. - // This ensures the tool_result's tool_use_id matches a tool_use in the immediately - // preceding assistant message (Anthropic API requirement). - const lastMessage = parentApiMessages[parentApiMessages.length - 1] - if (lastMessage?.role === "user") { - const validatedMessage = validateAndFixToolResultIds(lastMessage, parentApiMessages.slice(0, -1)) - parentApiMessages[parentApiMessages.length - 1] = validatedMessage - } - } else { - // If there is no corresponding tool_use in the parent API history, we cannot emit a - // tool_result. Fall back to a plain user text note so the parent can still resume. - parentApiMessages.push({ - role: "user", - content: [ - { - type: "text" as const, - text: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, - }, - ], - ts, - }) - } - - await saveApiMessages({ messages: parentApiMessages as any, taskId: parentTaskId, globalStoragePath }) - - // 3) Close child instance if still open (single-open-task invariant). - // This MUST happen BEFORE updating the child's status to "completed" because - // removeClineFromStack() → abortTask(true) → saveClineMessages() writes - // the historyItem with initialStatus (typically "active"), which would - // overwrite a "completed" status set earlier. - const current = this.getCurrentTask() - if (current?.taskId === childTaskId) { - await this.removeClineFromStack() - } - - // 4) Update child metadata to "completed" status. - // This runs after the abort so it overwrites the stale "active" status - // that saveClineMessages() may have written during step 3. - try { - const { historyItem: childHistory } = await this.getTaskWithId(childTaskId) - await this.updateTaskHistory({ - ...childHistory, - status: "completed", - }) - } catch (err) { - this.log( - `[reopenParentFromDelegation] Failed to persist child completed status for ${childTaskId}: ${ - (err as Error)?.message ?? String(err) - }`, - ) - } + await saveApiMessages({ messages: parentApiMessages as any, taskId: parentTaskId, globalStoragePath }) - // 5) Update parent metadata and persist BEFORE emitting completion event - const childIds = Array.from(new Set([...(historyItem.childIds ?? []), childTaskId])) - const updatedHistory: typeof historyItem = { - ...historyItem, - status: "active", - completedByChildId: childTaskId, - completionResultSummary, - awaitingChildId: undefined, - childIds, - } - await this.updateTaskHistory(updatedHistory) + // 3) Close child instance if still open (single-open-task invariant). + // This MUST happen BEFORE updating the child's status to "completed" because + // removeClineFromStack() → abortTask(true) → saveClineMessages() writes + // the historyItem with initialStatus (typically "active"), which would + // overwrite a "completed" status set earlier. + const current = this.getCurrentTask() + if (current?.taskId === childTaskId) { + await this.removeClineFromStack({ skipDelegationRepair: true }) + } - // 6) Emit TaskDelegationCompleted (provider-level) - try { - this.emit(RooCodeEventName.TaskDelegationCompleted, parentTaskId, childTaskId, completionResultSummary) - } catch { - // non-fatal - } + // 4) Update child metadata to "completed" status. + // This runs after the abort so it overwrites the stale "active" status + // that saveClineMessages() may have written during step 3. + try { + const { historyItem: childHistory } = await this.getTaskWithId(childTaskId) + await this.updateTaskHistory({ + ...childHistory, + status: "completed", + }) + } catch (err) { + this.log( + `[reopenParentFromDelegation] Failed to persist child completed status for ${childTaskId}: ${ + (err as Error)?.message ?? String(err) + }`, + ) + } - // 7) Reopen the parent from history as the sole active task (restores saved mode) - // IMPORTANT: startTask=false to suppress resume-from-history ask scheduling - const parentInstance = await this.createTaskWithHistoryItem(updatedHistory, { startTask: false }) + // 5) Update parent metadata and persist BEFORE emitting completion event + const childIds = Array.from(new Set([...(historyItem.childIds ?? []), childTaskId])) + const updatedHistory: typeof historyItem = { + ...historyItem, + status: "active", + completedByChildId: childTaskId, + completionResultSummary, + awaitingChildId: undefined, + childIds, + } + await this.updateTaskHistory(updatedHistory) - // 8) Inject restored histories into the in-memory instance before resuming - if (parentInstance) { + // 6) Emit TaskDelegationCompleted (provider-level) try { - await parentInstance.overwriteClineMessages(parentClineMessages) + this.emit(RooCodeEventName.TaskDelegationCompleted, parentTaskId, childTaskId, completionResultSummary) } catch { // non-fatal } + + // 7) Reopen the parent from history as the sole active task (restores saved mode) + // IMPORTANT: startTask=false to suppress resume-from-history ask scheduling + const parentInstance = await this.createTaskWithHistoryItem(updatedHistory, { startTask: false }) + + // 8) Inject restored histories into the in-memory instance before resuming + if (parentInstance) { + try { + await parentInstance.overwriteClineMessages(parentClineMessages) + } catch { + // non-fatal + } + try { + await parentInstance.overwriteApiConversationHistory(parentApiMessages as any) + } catch { + // non-fatal + } + + // Auto-resume parent without ask("resume_task") + await parentInstance.resumeAfterDelegation() + } + + // 9) Emit TaskDelegationResumed (provider-level) try { - await parentInstance.overwriteApiConversationHistory(parentApiMessages as any) + this.emit(RooCodeEventName.TaskDelegationResumed, parentTaskId, childTaskId) } catch { // non-fatal } - // Auto-resume parent without ask("resume_task") - await parentInstance.resumeAfterDelegation() - } - - // 9) Emit TaskDelegationResumed (provider-level) - try { - this.emit(RooCodeEventName.TaskDelegationResumed, parentTaskId, childTaskId) - } catch { - // non-fatal - } + ;(this.cancelledDelegationChildIds as Set | undefined)?.delete(childTaskId) + return true + }) } /** diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts index 766cd69eba..0ec4d12cb6 100644 --- a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -395,7 +395,7 @@ describe("ClineProvider flicker-free cancel", () => { ) }) - it("continues rehydrating a cancelled child when delegated parent detach fails", async () => { + it("detaches runtime parent links when delegated parent detach fails", async () => { const mockRootTask = { taskId: "root-1" } const mockParentTask = { taskId: "parent-1" } const childHistory: HistoryItem = { @@ -449,9 +449,10 @@ describe("ClineProvider flicker-free cancel", () => { id: "child-1", parentTaskId: "parent-1", rootTaskId: "root-1", - parentTask: mockParentTask, - rootTask: mockRootTask, + parentTask: undefined, + rootTask: undefined, }), ) + expect((provider as any).cancelledDelegationChildIds.has("child-1")).toBe(true) }) }) From f7a7a95b452f71a30261dcbbc4f8b93f6e3fefb0 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 25 May 2026 02:16:42 +0000 Subject: [PATCH 7/8] fix: harden delegated subtask cancellation --- apps/vscode-e2e/src/fixtures/subtasks.ts | 40 +++++----------- apps/vscode-e2e/src/suite/subtasks.test.ts | 44 +++++++++-------- .../presentAssistantMessage.ts | 2 +- src/core/webview/ClineProvider.ts | 47 +++++++++++++------ .../ClineProvider.flicker-free-cancel.spec.ts | 12 ++++- 5 files changed, 77 insertions(+), 68 deletions(-) diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index cdf299541a..c3b5f5b2b6 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -6,15 +6,24 @@ import { toolResultContains } from "./tool-result" const SUBTASK_PARENT_MARKER = "SUBTASK_PARENT_CANCELLATION_SMOKE" const SUBTASK_CHILD_MARKER = "SUBTASK_CHILD_CALCULATOR_SMOKE" -const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` +export const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` export const SUBTASK_PARENT_PROMPT = `${SUBTASK_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_CHILD_PROMPT}" Do not answer directly.` export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9" -const INTERRUPTED_TOOL_RESULT = "Task was interrupted before this tool call could be completed." + +const requestContains = (req: ChatCompletionRequest, expected: string[]) => { + const rawRequest = JSON.stringify(req) + return expected.every((text) => rawRequest.includes(text)) +} const completionAfterAnswer = (followupId: string, completionId: string) => ({ match: { - toolCallId: followupId, - predicate: (req: ChatCompletionRequest) => toolResultContains(req, followupId, [SUBTASK_CHILD_FOLLOWUP_ANSWER]), + predicate: (req: ChatCompletionRequest) => + toolResultContains(req, followupId, [SUBTASK_CHILD_FOLLOWUP_ANSWER]) || + requestContains(req, [followupId, SUBTASK_CHILD_FOLLOWUP_ANSWER]) || + requestContains(req, [ + SUBTASK_CHILD_MARKER, + `\\n${SUBTASK_CHILD_FOLLOWUP_ANSWER}\\n`, + ]), }, response: { toolCalls: [ @@ -64,31 +73,8 @@ export function addSubtaskFixtures(mock: InstanceType) { }, }) - mock.addFixture({ - match: { - toolCallId: "call_subtasks_child_followup_001", - predicate: (req) => toolResultContains(req, "call_subtasks_child_followup_001", [INTERRUPTED_TOOL_RESULT]), - }, - response: { - toolCalls: [ - { - name: "ask_followup_question", - arguments: JSON.stringify({ - question: "What is the square root of 81?", - follow_up: [{ text: SUBTASK_CHILD_FOLLOWUP_ANSWER }], - }), - id: "call_subtasks_child_followup_resume_002", - }, - ], - }, - }) - mock.addFixture(completionAfterAnswer("call_subtasks_child_followup_001", "call_subtasks_child_completion_002")) - mock.addFixture( - completionAfterAnswer("call_subtasks_child_followup_resume_002", "call_subtasks_child_completion_resume_003"), - ) - mock.addFixture({ match: { toolCallId: "call_subtasks_parent_new_task_001", diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index d18fb1fd05..1f29f616bb 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -3,13 +3,13 @@ import * as assert from "assert" import { RooCodeEventName, type ClineMessage } from "@roo-code/types" import { setDefaultSuiteTimeout } from "./test-utils" -import { sleep, waitFor, waitUntilCompleted } from "./utils" +import { waitFor, waitUntilCompleted } from "./utils" import { SUBTASK_CHILD_FOLLOWUP_ANSWER, SUBTASK_PARENT_PROMPT } from "../fixtures/subtasks" suite("Roo Code Subtasks", function () { setDefaultSuiteTimeout(this) - test("Should handle subtask cancellation and resumption correctly", async () => { + test("Should keep parent paused after subtask cancellation", async () => { const api = globalThis.api const asks: Record = {} const messages: Record = {} @@ -78,13 +78,9 @@ suite("Roo Code Subtasks", function () { () => asks[spawnedTaskId!]?.some(({ type, ask }) => type === "ask" && ask === "followup") ?? false, ) const cancelledChildTaskId = spawnedTaskId! - const delegatedFollowupCount = - asks[cancelledChildTaskId]?.filter(({ type, ask }) => type === "ask" && ask === "followup").length ?? 0 await api.cancelCurrentTask() - await sleep(2_000) - assert.ok( messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === undefined, @@ -101,38 +97,40 @@ suite("Roo Code Subtasks", function () { asks[cancelledChildTaskId]?.some(({ type, ask }) => type === "ask" && ask === "resume_task") ?? false, ) - await api.approveCurrentAsk() - await waitForStage( - "wait for resumed child followup ask", - () => - (asks[cancelledChildTaskId]?.filter(({ type, ask }) => type === "ask" && ask === "followup") - .length ?? 0) > delegatedFollowupCount, - ) - await api.sendMessage(SUBTASK_CHILD_FOLLOWUP_ANSWER) - await waitUntilCompleted({ api, taskId: cancelledChildTaskId }) + const resumedChildTaskId = await waitUntilCompleted({ + api, + start: async () => { + await api.sendMessage(SUBTASK_CHILD_FOLLOWUP_ANSWER) + return cancelledChildTaskId + }, + }) + + assert.strictEqual( + resumedChildTaskId, + cancelledChildTaskId, + "Cancelled child task should be resumed in place", + ) assert.strictEqual( - findErrorText(cancelledChildTaskId), + findErrorText(resumedChildTaskId), undefined, - "Cancelled child should not emit an error", + "Resumed child task should not emit an error", ) assert.strictEqual( - findCompletionText(cancelledChildTaskId), + findCompletionText(resumedChildTaskId), "9", - "Cancelled child should complete with `9`", + "Resumed child task should complete with `9`", ) assert.strictEqual( api.getCurrentTaskStack().at(-1), cancelledChildTaskId, - "Cancelled child should stay active after resuming from cancellation", + "Cancelled child task should remain the active completed task", ) - await sleep(2_000) - assert.ok( messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === undefined, - "Parent task should not have resumed after subtask cancellation", + "Parent task should not have resumed after the cancelled child completed", ) await api.clearCurrentTask() diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 7f5862be15..cc675dd948 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -60,7 +60,7 @@ import { sanitizeToolUseId } from "../../utils/tool-id" export async function presentAssistantMessage(cline: Task) { if (cline.abort) { - throw new Error(`[Task#presentAssistantMessage] task ${cline.taskId}.${cline.instanceId} aborted`) + return } if (cline.presentAssistantMessageLocked) { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index b01141450b..3c68b482a6 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -114,13 +114,11 @@ export type ClineProviderEvents = { clineCreated: [cline: Task] } -type DelegationLockHost = { - delegationTransitionLocks?: Map> -} - -function runDelegationTransition(host: DelegationLockHost, parentTaskId: string, fn: () => Promise): Promise { - host.delegationTransitionLocks ??= new Map() - const locks = host.delegationTransitionLocks +function runDelegationTransition( + locks: Map>, + parentTaskId: string, + fn: () => Promise, +): Promise { const previous = locks.get(parentTaskId) ?? Promise.resolve() const current = previous.then(fn, fn) const tail = current.then( @@ -153,7 +151,7 @@ export class ClineProvider private webviewDisposables: vscode.Disposable[] = [] private view?: vscode.WebviewView | vscode.WebviewPanel private clineStack: Task[] = [] - private delegationTransitionLocks = new Map>() + private delegationTransitionLocks?: Map> private cancelledDelegationChildIds = new Set() private codeIndexStatusSubscription?: vscode.Disposable private codeIndexManager?: CodeIndexManager @@ -173,6 +171,11 @@ export class ClineProvider private globalStateWriteThroughTimer: ReturnType | null = null private static readonly GLOBAL_STATE_WRITE_THROUGH_DEBOUNCE_MS = 5000 // 5 seconds private static readonly PENDING_OPERATION_TIMEOUT_MS = 30000 // 30 seconds + + private runDelegationTransition(parentTaskId: string, fn: () => Promise): Promise { + this.delegationTransitionLocks ??= new Map() + return runDelegationTransition(this.delegationTransitionLocks, parentTaskId, fn) + } private readonly pendingEditOperations: PendingEditOperationStore private cloudOrganizationsCache: CloudOrganizationMembership[] | null = null @@ -509,7 +512,7 @@ export class ClineProvider // child and will update the parent to point at the new child. if (parentTaskId && childTaskId && !options?.skipDelegationRepair) { try { - await runDelegationTransition(this as unknown as DelegationLockHost, parentTaskId, async () => { + await ClineProvider.prototype.runDelegationTransition.call(this, parentTaskId, async () => { const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === childTaskId) { @@ -2995,7 +2998,7 @@ export class ClineProvider if (task.parentTaskId) { try { - await runDelegationTransition(this as unknown as DelegationLockHost, task.parentTaskId, async () => { + await ClineProvider.prototype.runDelegationTransition.call(this, task.parentTaskId, async () => { const { historyItem: parentHistory } = await this.getTaskWithId(task.parentTaskId!) if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === task.taskId) { @@ -3014,12 +3017,26 @@ export class ClineProvider } }) } catch (error) { - // Fail closed: if we cannot prove the parent was detached, keep the - // rehydrated child disconnected from runtime parent links and prevent - // this in-process child completion from reopening the parent later. + // Fail closed: if we cannot prove the parent was detached, make the + // rehydrated child standalone so later completions cannot reopen a + // stale delegated parent, even after a provider reload. parentTask = undefined rootTask = undefined this.cancelledDelegationChildIds.add(task.taskId) + historyItem = { + ...historyItem, + parentTaskId: undefined, + rootTaskId: undefined, + } + try { + await this.updateTaskHistory(historyItem) + } catch (historyError) { + this.log( + `[cancelTask] Failed to persist standalone child state for ${task.taskId}: ${ + historyError instanceof Error ? historyError.message : String(historyError) + }`, + ) + } this.log( `[cancelTask] Failed to detach delegated parent for ${task.taskId}: ${ error instanceof Error ? error.message : String(error) @@ -3382,7 +3399,7 @@ export class ClineProvider completionResultSummary: string }): Promise { const { parentTaskId, childTaskId, completionResultSummary } = params - return await runDelegationTransition(this as unknown as DelegationLockHost, parentTaskId, async () => { + return await (ClineProvider.prototype.runDelegationTransition.call(this, parentTaskId, async () => { const globalStoragePath = this.contextProxy.globalStorageUri.fsPath // 1) Load parent from history and current persisted messages @@ -3591,7 +3608,7 @@ export class ClineProvider ;(this.cancelledDelegationChildIds as Set | undefined)?.delete(childTaskId) return true - }) + }) as Promise) } /** diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts index 0ec4d12cb6..6b1879e433 100644 --- a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -435,6 +435,7 @@ describe("ClineProvider flicker-free cancel", () => { throw new Error(`unexpected task lookup: ${id}`) }) as any + const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([]) const createTaskWithHistoryItemSpy = vi .spyOn(provider, "createTaskWithHistoryItem") .mockResolvedValue(undefined as any) @@ -444,11 +445,18 @@ describe("ClineProvider flicker-free cancel", () => { expect(mockOutputChannel.appendLine).toHaveBeenCalledWith( expect.stringContaining("[cancelTask] Failed to detach delegated parent for child-1: parent lookup failed"), ) + expect(updateTaskHistorySpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: "child-1", + parentTaskId: undefined, + rootTaskId: undefined, + }), + ) expect(createTaskWithHistoryItemSpy).toHaveBeenCalledWith( expect.objectContaining({ id: "child-1", - parentTaskId: "parent-1", - rootTaskId: "root-1", + parentTaskId: undefined, + rootTaskId: undefined, parentTask: undefined, rootTask: undefined, }), From 130534232b01ecb5b13bb0eda0417e272748f2c2 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Fri, 29 May 2026 00:15:08 +0000 Subject: [PATCH 8/8] test(e2e): simplyfying e2e tests --- apps/vscode-e2e/src/fixtures/subtasks.ts | 5 +- apps/vscode-e2e/src/suite/subtasks.test.ts | 109 ++++++++++---- .../src/suite/tools/apply-diff.test.ts | 6 +- .../src/suite/tools/execute-command.test.ts | 6 +- .../src/suite/tools/list-files.test.ts | 12 +- .../src/suite/tools/read-file.test.ts | 12 +- .../src/suite/tools/search-files.test.ts | 12 +- .../src/suite/tools/use-mcp-tool.test.ts | 6 +- .../src/suite/tools/write-to-file.test.ts | 6 +- src/__tests__/helpers/provider-stub.ts | 17 +++ .../history-resume-delegation.spec.ts | 123 ++++++++++++---- .../nested-delegation-resume.spec.ts | 5 +- src/__tests__/provider-delegation.spec.ts | 106 ++----------- .../removeClineFromStack-delegation.spec.ts | 5 +- src/core/task/Task.ts | 8 +- src/core/webview/ClineProvider.ts | 139 +++++++++--------- .../ClineProvider.flicker-free-cancel.spec.ts | 46 ++++++ 17 files changed, 371 insertions(+), 252 deletions(-) create mode 100644 src/__tests__/helpers/provider-stub.ts diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index c3b5f5b2b6..33bb2c9cbf 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -6,7 +6,7 @@ import { toolResultContains } from "./tool-result" const SUBTASK_PARENT_MARKER = "SUBTASK_PARENT_CANCELLATION_SMOKE" const SUBTASK_CHILD_MARKER = "SUBTASK_CHILD_CALCULATOR_SMOKE" -export const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` +const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` export const SUBTASK_PARENT_PROMPT = `${SUBTASK_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_CHILD_PROMPT}" Do not answer directly.` export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9" @@ -18,8 +18,11 @@ const requestContains = (req: ChatCompletionRequest, expected: string[]) => { const completionAfterAnswer = (followupId: string, completionId: string) => ({ match: { predicate: (req: ChatCompletionRequest) => + // Preferred: structured tool-result message carries the followup answer. toolResultContains(req, followupId, [SUBTASK_CHILD_FOLLOWUP_ANSWER]) || + // Fallback 1: answer present alongside the tool-call ID but not in a role:tool message. requestContains(req, [followupId, SUBTASK_CHILD_FOLLOWUP_ANSWER]) || + // Fallback 2: answer arrives as a bare user message after task resume (no tool-call ID context). requestContains(req, [ SUBTASK_CHILD_MARKER, `\\n${SUBTASK_CHILD_FOLLOWUP_ANSWER}\\n`, diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index 1f29f616bb..51cf24f6a3 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -9,25 +9,87 @@ import { SUBTASK_CHILD_FOLLOWUP_ANSWER, SUBTASK_PARENT_PROMPT } from "../fixture suite("Roo Code Subtasks", function () { setDefaultSuiteTimeout(this) - test("Should keep parent paused after subtask cancellation", async () => { + // Race mitigation: skipDelegationRepair prevents removeClineFromStack from + // auto-resuming the parent when the child is cancelled (Race 2). + test("parent stays paused after subtask cancellation", async () => { const api = globalThis.api const asks: Record = {} const messages: Record = {} - const waitForStage = async (label: string, condition: Parameters[0]) => { - try { - await waitFor(condition) - } catch (error) { - const message = error instanceof Error ? error.message : String(error) - throw new Error(`${label}: ${message}`) + + const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { + if (message.type === "ask") { + asks[taskId] = asks[taskId] || [] + asks[taskId].push(message) + } + if (message.type === "say" && message.partial === false) { + messages[taskId] = messages[taskId] || [] + messages[taskId].push(message) } } + api.on(RooCodeEventName.Message, messageHandler) + + try { + const parentTaskId = await api.startNewTask({ + configuration: { + mode: "ask", + alwaysAllowModeSwitch: true, + alwaysAllowSubtasks: true, + autoApprovalEnabled: true, + enableCheckpoints: false, + }, + text: SUBTASK_PARENT_PROMPT, + }) + + let spawnedTaskId: string | undefined + await waitFor(() => { + const stack = api.getCurrentTaskStack() + const current = stack[stack.length - 1] + if (current && current !== parentTaskId) { + spawnedTaskId = current + return true + } + return false + }) + + await waitFor( + () => asks[spawnedTaskId!]?.some(({ type, ask }) => type === "ask" && ask === "followup") ?? false, + ) + + await api.cancelCurrentTask() + + assert.ok( + messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === + undefined, + "Parent task should not have resumed after subtask cancellation", + ) + + await waitFor(() => api.getCurrentTaskStack().at(-1) === spawnedTaskId) + await waitFor( + () => asks[spawnedTaskId!]?.some(({ type, ask }) => type === "ask" && ask === "resume_task") ?? false, + ) + + await api.clearCurrentTask() + // The parent task is still in the stack; drain it so it doesn't leak into the next test. + await api.clearCurrentTask() + await waitFor(() => api.getCurrentTaskStack().length === 0) + } finally { + api.off(RooCodeEventName.Message, messageHandler) + } + }) + + // Race mitigation: runDelegationTransition lock + cancelledDelegationChildIds guard + // ensures cancelTask() wins over a concurrent reopenParentFromDelegation() (Race 3). + test("cancelled child completes in-place and does not reopen parent", async () => { + const api = globalThis.api + const asks: Record = {} + const messages: Record = {} + const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { if (message.type === "ask") { asks[taskId] = asks[taskId] || [] asks[taskId].push(message) } - if (message.type === "say" && message.partial === false) { messages[taskId] = messages[taskId] || [] messages[taskId].push(message) @@ -64,35 +126,25 @@ suite("Roo Code Subtasks", function () { }) let spawnedTaskId: string | undefined - await waitForStage("wait for spawned subtask", () => { - const currentTaskStack = api.getCurrentTaskStack() - const currentTaskId = currentTaskStack[currentTaskStack.length - 1] - if (currentTaskId && currentTaskId !== parentTaskId) { - spawnedTaskId = currentTaskId + await waitFor(() => { + const stack = api.getCurrentTaskStack() + const current = stack[stack.length - 1] + if (current && current !== parentTaskId) { + spawnedTaskId = current return true } return false }) - await waitForStage( - "wait for delegated child followup ask", + + await waitFor( () => asks[spawnedTaskId!]?.some(({ type, ask }) => type === "ask" && ask === "followup") ?? false, ) - const cancelledChildTaskId = spawnedTaskId! + const cancelledChildTaskId = spawnedTaskId! await api.cancelCurrentTask() - assert.ok( - messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === - undefined, - "Parent task should not have resumed after subtask cancellation", - ) - - await waitForStage( - "wait for cancelled child task to remain active", - () => api.getCurrentTaskStack().at(-1) === cancelledChildTaskId, - ) - await waitForStage( - "wait for cancelled child resume ask", + await waitFor(() => api.getCurrentTaskStack().at(-1) === cancelledChildTaskId) + await waitFor( () => asks[cancelledChildTaskId]?.some(({ type, ask }) => type === "ask" && ask === "resume_task") ?? false, @@ -126,7 +178,6 @@ suite("Roo Code Subtasks", function () { cancelledChildTaskId, "Cancelled child task should remain the active completed task", ) - assert.ok( messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === undefined, diff --git a/apps/vscode-e2e/src/suite/tools/apply-diff.test.ts b/apps/vscode-e2e/src/suite/tools/apply-diff.test.ts index 39488f8144..36bd847416 100644 --- a/apps/vscode-e2e/src/suite/tools/apply-diff.test.ts +++ b/apps/vscode-e2e/src/suite/tools/apply-diff.test.ts @@ -128,7 +128,7 @@ suite("Roo Code apply_diff Tool", function () { suiteTeardown(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -147,7 +147,7 @@ suite("Roo Code apply_diff Tool", function () { setup(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -164,7 +164,7 @@ suite("Roo Code apply_diff Tool", function () { teardown(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } diff --git a/apps/vscode-e2e/src/suite/tools/execute-command.test.ts b/apps/vscode-e2e/src/suite/tools/execute-command.test.ts index 586bb85c15..db09fba264 100644 --- a/apps/vscode-e2e/src/suite/tools/execute-command.test.ts +++ b/apps/vscode-e2e/src/suite/tools/execute-command.test.ts @@ -43,7 +43,7 @@ suite("Roo Code execute_command Tool", function () { suiteTeardown(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -62,7 +62,7 @@ suite("Roo Code execute_command Tool", function () { setup(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -74,7 +74,7 @@ suite("Roo Code execute_command Tool", function () { teardown(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } diff --git a/apps/vscode-e2e/src/suite/tools/list-files.test.ts b/apps/vscode-e2e/src/suite/tools/list-files.test.ts index d8afa52989..578f00238c 100644 --- a/apps/vscode-e2e/src/suite/tools/list-files.test.ts +++ b/apps/vscode-e2e/src/suite/tools/list-files.test.ts @@ -129,9 +129,9 @@ This directory contains various files and subdirectories for testing the list_fi // Clean up test files and directories after all tests suiteTeardown(async () => { - // Cancel any running tasks before cleanup + // Clear any running tasks before cleanup try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -149,9 +149,9 @@ This directory contains various files and subdirectories for testing the list_fi // Clean up before each test setup(async () => { - // Cancel any previous task + // Clear any previous task try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -162,9 +162,9 @@ This directory contains various files and subdirectories for testing the list_fi // Clean up after each test teardown(async () => { - // Cancel the current task + // Clear the current task try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } diff --git a/apps/vscode-e2e/src/suite/tools/read-file.test.ts b/apps/vscode-e2e/src/suite/tools/read-file.test.ts index a8b646842e..da644dc2db 100644 --- a/apps/vscode-e2e/src/suite/tools/read-file.test.ts +++ b/apps/vscode-e2e/src/suite/tools/read-file.test.ts @@ -67,9 +67,9 @@ suite("Roo Code read_file Tool", function () { // Clean up temporary directory and files after tests suiteTeardown(async () => { - // Cancel any running tasks before cleanup + // Clear any running tasks before cleanup try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -96,9 +96,9 @@ suite("Roo Code read_file Tool", function () { // Clean up before each test setup(async () => { - // Cancel any previous task + // Clear any previous task try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -109,9 +109,9 @@ suite("Roo Code read_file Tool", function () { // Clean up after each test teardown(async () => { - // Cancel the current task + // Clear the current task try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } diff --git a/apps/vscode-e2e/src/suite/tools/search-files.test.ts b/apps/vscode-e2e/src/suite/tools/search-files.test.ts index 06a3c99151..caa048f30f 100644 --- a/apps/vscode-e2e/src/suite/tools/search-files.test.ts +++ b/apps/vscode-e2e/src/suite/tools/search-files.test.ts @@ -240,9 +240,9 @@ The search should find matches across different file types and provide context f // Clean up after all tests suiteTeardown(async () => { - // Cancel any running tasks before cleanup + // Clear any running tasks before cleanup try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -270,9 +270,9 @@ The search should find matches across different file types and provide context f // Clean up before each test setup(async () => { - // Cancel any previous task + // Clear any previous task try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -283,9 +283,9 @@ The search should find matches across different file types and provide context f // Clean up after each test teardown(async () => { - // Cancel the current task + // Clear the current task try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } diff --git a/apps/vscode-e2e/src/suite/tools/use-mcp-tool.test.ts b/apps/vscode-e2e/src/suite/tools/use-mcp-tool.test.ts index c0fda328fd..21e189d7c1 100644 --- a/apps/vscode-e2e/src/suite/tools/use-mcp-tool.test.ts +++ b/apps/vscode-e2e/src/suite/tools/use-mcp-tool.test.ts @@ -190,7 +190,7 @@ suite("Roo Code use_mcp_tool Tool", function () { suiteTeardown(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -201,7 +201,7 @@ suite("Roo Code use_mcp_tool Tool", function () { setup(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -212,7 +212,7 @@ suite("Roo Code use_mcp_tool Tool", function () { teardown(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } diff --git a/apps/vscode-e2e/src/suite/tools/write-to-file.test.ts b/apps/vscode-e2e/src/suite/tools/write-to-file.test.ts index 9c73a94b4e..f72e513e01 100644 --- a/apps/vscode-e2e/src/suite/tools/write-to-file.test.ts +++ b/apps/vscode-e2e/src/suite/tools/write-to-file.test.ts @@ -43,7 +43,7 @@ suite("Roo Code write_to_file Tool", function () { suiteTeardown(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -62,7 +62,7 @@ suite("Roo Code write_to_file Tool", function () { setup(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } @@ -74,7 +74,7 @@ suite("Roo Code write_to_file Tool", function () { teardown(async () => { try { - await globalThis.api.cancelCurrentTask() + await globalThis.api.clearCurrentTask() } catch { // Task might not be running } diff --git a/src/__tests__/helpers/provider-stub.ts b/src/__tests__/helpers/provider-stub.ts new file mode 100644 index 0000000000..d655c2dd6f --- /dev/null +++ b/src/__tests__/helpers/provider-stub.ts @@ -0,0 +1,17 @@ +import { ClineProvider } from "../../core/webview/ClineProvider" + +/** + * Augments a plain stub object with the instance fields and bound methods that + * ClineProvider methods read from `this` (runDelegationTransition, + * delegationTransitionLocks, cancelledDelegationChildIds), so tests can call + * private methods via `(ClineProvider.prototype as any).method.call(stub, …)` + * without instantiating a real ClineProvider. + */ +export function makeProviderStub(stub: T): T { + const s = stub as any + const proto = ClineProvider.prototype as any + s.delegationTransitionLocks ??= new Map() + s.cancelledDelegationChildIds ??= new Set() + s.runDelegationTransition = proto.runDelegationTransition.bind(s) + return s +} diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index 40fd95d9da..6fc0686626 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -38,6 +38,7 @@ vi.mock("../core/task-persistence", () => ({ import { ClineProvider } from "../core/webview/ClineProvider" import { readTaskMessages } from "../core/task-persistence/taskMessages" import { readApiMessages, saveApiMessages, saveTaskMessages } from "../core/task-persistence" +import { makeProviderStub } from "./helpers/provider-stub" describe("History resume delegation - parent metadata transitions", () => { beforeEach(() => { @@ -71,7 +72,7 @@ describe("History resume delegation - parent metadata transitions", () => { resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), }) - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId, emit: providerEmit, @@ -79,7 +80,7 @@ describe("History resume delegation - parent metadata transitions", () => { removeClineFromStack, createTaskWithHistoryItem, updateTaskHistory, - } as unknown as ClineProvider + } as any) // Mock persistence reads to return empty arrays vi.mocked(readTaskMessages).mockResolvedValue([]) @@ -121,7 +122,7 @@ describe("History resume delegation - parent metadata transitions", () => { }) it("reopenParentFromDelegation injects subtask_result into both UI and API histories", async () => { - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/storage" } }, getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { @@ -146,7 +147,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory: vi.fn().mockResolvedValue([]), - } as unknown as ClineProvider + } as any) // Start with existing messages in history const existingUiMessages = [{ type: "ask", ask: "tool", text: "Old tool", ts: 50 }] @@ -204,7 +205,7 @@ describe("History resume delegation - parent metadata transitions", () => { }) it("reopenParentFromDelegation injects tool_result when new_task tool_use exists in API history", async () => { - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/storage" } }, getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { @@ -229,7 +230,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory: vi.fn().mockResolvedValue([]), - } as unknown as ClineProvider + } as any) // Include an assistant message with new_task tool_use to exercise the tool_result path const existingUiMessages = [{ type: "ask", ask: "tool", text: "new_task request", ts: 50 }] @@ -290,7 +291,7 @@ describe("History resume delegation - parent metadata transitions", () => { }) it("reopenParentFromDelegation injects plain text when no new_task tool_use exists in API history", async () => { - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/storage" } }, getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { @@ -315,7 +316,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory: vi.fn().mockResolvedValue([]), - } as unknown as ClineProvider + } as any) // No assistant tool_use in history const existingUiMessages = [{ type: "ask", ask: "tool", text: "subtask request", ts: 50 }] @@ -350,7 +351,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), } - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { @@ -370,7 +371,7 @@ describe("History resume delegation - parent metadata transitions", () => { removeClineFromStack: vi.fn().mockResolvedValue(undefined), createTaskWithHistoryItem: vi.fn().mockResolvedValue(parentInstance), updateTaskHistory: vi.fn().mockResolvedValue([]), - } as unknown as ClineProvider + } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) vi.mocked(readApiMessages).mockResolvedValue([]) @@ -390,7 +391,7 @@ describe("History resume delegation - parent metadata transitions", () => { const emitSpy = vi.fn() const updateTaskHistory = vi.fn().mockResolvedValue([]) - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { @@ -414,7 +415,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory, - } as unknown as ClineProvider + } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) vi.mocked(readApiMessages).mockResolvedValue([]) @@ -456,7 +457,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockRejectedValue(new Error("api overwrite failed")), } - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId: vi.fn().mockImplementation(async (id: string) => { if (id === "parent-rpd06") { @@ -492,7 +493,7 @@ describe("History resume delegation - parent metadata transitions", () => { removeClineFromStack: vi.fn().mockResolvedValue(undefined), createTaskWithHistoryItem: vi.fn().mockResolvedValue(parentInstance), updateTaskHistory: vi.fn().mockResolvedValue([]), - } as unknown as ClineProvider + } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) vi.mocked(readApiMessages).mockResolvedValue([]) @@ -526,7 +527,7 @@ describe("History resume delegation - parent metadata transitions", () => { it("reopenParentFromDelegation does NOT emit TaskPaused or TaskUnpaused (new flow only)", async () => { const emitSpy = vi.fn() - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { @@ -550,7 +551,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory: vi.fn().mockResolvedValue([]), - } as unknown as ClineProvider + } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) vi.mocked(readApiMessages).mockResolvedValue([]) @@ -579,7 +580,7 @@ describe("History resume delegation - parent metadata transitions", () => { const removeClineFromStack = vi.fn().mockResolvedValue(undefined) const createTaskWithHistoryItem = vi.fn().mockResolvedValue(parentInstance) - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId: vi.fn().mockImplementation(async (id: string) => { if (id === "parent-rpd02") { @@ -614,7 +615,7 @@ describe("History resume delegation - parent metadata transitions", () => { removeClineFromStack, createTaskWithHistoryItem, updateTaskHistory, - } as unknown as ClineProvider + } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) vi.mocked(readApiMessages).mockResolvedValue([]) @@ -659,7 +660,7 @@ describe("History resume delegation - parent metadata transitions", () => { return [] }) - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId: vi.fn().mockImplementation(async (id: string) => { if (id === "parent-rpd04") { @@ -695,7 +696,7 @@ describe("History resume delegation - parent metadata transitions", () => { removeClineFromStack: vi.fn().mockResolvedValue(undefined), createTaskWithHistoryItem: vi.fn().mockResolvedValue(parentInstance), updateTaskHistory, - } as unknown as ClineProvider + } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) vi.mocked(readApiMessages).mockResolvedValue([]) @@ -724,8 +725,49 @@ describe("History resume delegation - parent metadata transitions", () => { expect(emitSpy).toHaveBeenCalledWith(RooCodeEventName.TaskDelegationResumed, "parent-rpd04", "child-rpd04") }) + it("reopenParentFromDelegation keeps the child open when parent metadata persistence fails", async () => { + const persistError = new Error("parent status persist failed") + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const createTaskWithHistoryItem = vi.fn() + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ + historyItem: { + id: "parent-rpd05", + status: "delegated", + awaitingChildId: "child-rpd05", + childIds: ["child-rpd05"], + ts: 800, + task: "Parent RPD-05", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + }, + }), + emit: vi.fn(), + getCurrentTask: vi.fn(() => ({ taskId: "child-rpd05" })), + removeClineFromStack, + createTaskWithHistoryItem, + updateTaskHistory: vi.fn().mockRejectedValue(persistError), + } as any) + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await expect( + (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-rpd05", + childTaskId: "child-rpd05", + completionResultSummary: "Child completion", + }), + ).rejects.toThrow(persistError) + + expect(removeClineFromStack).not.toHaveBeenCalled() + expect(createTaskWithHistoryItem).not.toHaveBeenCalled() + }) + it("handles empty history gracefully when injecting synthetic messages", async () => { - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { @@ -749,7 +791,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory: vi.fn().mockResolvedValue([]), - } as unknown as ClineProvider + } as any) // Mock read failures or empty returns vi.mocked(readTaskMessages).mockResolvedValue([]) @@ -793,7 +835,7 @@ describe("History resume delegation - parent metadata transitions", () => { const saveApiMessagesMock = vi.mocked(saveApiMessages) const makeProvider = (historyItem: object) => - ({ + makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId: vi.fn().mockResolvedValue({ historyItem }), emit: vi.fn(), @@ -802,7 +844,7 @@ describe("History resume delegation - parent metadata transitions", () => { removeClineFromStack: vi.fn(), createTaskWithHistoryItem: vi.fn(), updateTaskHistory, - }) as unknown as ClineProvider + } as any) const providerActive = makeProvider({ id: "parent-guard", @@ -827,7 +869,7 @@ describe("History resume delegation - parent metadata transitions", () => { const updateTaskHistory = vi.fn() const saveTaskMessagesMock = vi.mocked(saveTaskMessages) const saveApiMessagesMock = vi.mocked(saveApiMessages) - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { @@ -843,7 +885,7 @@ describe("History resume delegation - parent metadata transitions", () => { createTaskWithHistoryItem: vi.fn(), updateTaskHistory, cancelledDelegationChildIds: new Set(["child-guard"]), - } as unknown as ClineProvider + } as any) await expect( (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { @@ -866,7 +908,7 @@ describe("History resume delegation - parent metadata transitions", () => { const saveApiMessagesMock = vi.mocked(saveApiMessages) const makeProvider = (historyItem: object) => - ({ + makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId: vi.fn().mockResolvedValue({ historyItem }), emit: vi.fn(), @@ -875,7 +917,7 @@ describe("History resume delegation - parent metadata transitions", () => { removeClineFromStack: vi.fn(), createTaskWithHistoryItem: vi.fn(), updateTaskHistory, - }) as unknown as ClineProvider + } as any) const providerWrongChild = makeProvider({ id: "parent-guard", @@ -894,4 +936,29 @@ describe("History resume delegation - parent metadata transitions", () => { expect(updateTaskHistory).not.toHaveBeenCalled() expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting")) }) + + it("serializes delegation transitions and continues after a rejected predecessor", async () => { + const provider = makeProviderStub({} as any) as any + const calls: string[] = [] + let rejectFirst!: (error: Error) => void + + const first = provider.runDelegationTransition("parent-lock", async () => { + calls.push("first") + await new Promise((_resolve, reject) => { + rejectFirst = reject + }) + }) + const second = provider.runDelegationTransition("parent-lock", async () => { + calls.push("second") + return "done" + }) + + await Promise.resolve() + expect(calls).toEqual(["first"]) + + rejectFirst(new Error("first transition failed")) + await expect(first).rejects.toThrow("first transition failed") + await expect(second).resolves.toBe("done") + expect(calls).toEqual(["first", "second"]) + }) }) diff --git a/src/__tests__/nested-delegation-resume.spec.ts b/src/__tests__/nested-delegation-resume.spec.ts index 5dbafc949c..9f78ba14bb 100644 --- a/src/__tests__/nested-delegation-resume.spec.ts +++ b/src/__tests__/nested-delegation-resume.spec.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest" import { RooCodeEventName } from "@roo-code/types" +import { makeProviderStub } from "./helpers/provider-stub" // Mock safe-stable-stringify to avoid runtime error vi.mock("safe-stable-stringify", () => ({ @@ -148,7 +149,7 @@ describe("Nested delegation resume (A → B → C)", () => { return Object.values(historyIndex) }) - const provider = { + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId, emit: emitSpy, @@ -160,7 +161,7 @@ describe("Nested delegation resume (A → B → C)", () => { reopenParentFromDelegation: vi.fn(async (params: any) => { return await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, params) }), - } as unknown as ClineProvider + } as unknown as ClineProvider) // Empty histories for simplicity vi.mocked(readTaskMessages).mockResolvedValue([]) diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index 3f75d1233a..bd4519b970 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -101,7 +101,6 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { const parentTask = { taskId: "parent-1", emit: vi.fn(), - getTaskMode: vi.fn().mockResolvedValue("architect"), } as any const childStart = vi.fn(() => callOrder.push("child.start")) @@ -147,97 +146,24 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"]) }) - it("removes the paused child when parent delegation metadata cannot be persisted", async () => { - const parentTask = { - taskId: "parent-1", - emit: vi.fn(), - getTaskMode: vi.fn().mockResolvedValue("architect"), - } as any - const childStart = vi.fn() - const persistError = new Error("history write failed") - - const providerEmit = vi.fn() - const updateTaskHistory = vi.fn().mockRejectedValue(persistError) - const removeClineFromStack = vi.fn().mockResolvedValue(undefined) - const deleteTaskFromState = vi.fn().mockResolvedValue(undefined) - const createTask = vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart }) - const createTaskWithHistoryItem = vi.fn().mockResolvedValue(undefined) - const handleModeSwitch = vi.fn().mockResolvedValue(undefined) + it("rolls back the paused child and restores the parent when metadata persistence fails", async () => { + const persistError = new Error("parent metadata persist failed") const parentHistory = { id: "parent-1", task: "Parent", tokensIn: 0, tokensOut: 0, totalCost: 0, - childIds: [], + mode: "code", } - const getTaskWithId = vi.fn().mockResolvedValue({ - historyItem: parentHistory, - }) - - const provider = { - emit: providerEmit, - getCurrentTask: vi.fn(() => parentTask), - removeClineFromStack, - createTask, - getTaskWithId, - updateTaskHistory, - deleteTaskFromState, - createTaskWithHistoryItem, - handleModeSwitch, - log: vi.fn(), - } as unknown as ClineProvider - - await expect( - (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { - parentTaskId: "parent-1", - message: "Do something", - initialTodos: [], - mode: "code", - }), - ).rejects.toThrow("history write failed") - - expect(createTask).toHaveBeenCalledWith("Do something", undefined, parentTask, { - initialTodos: [], - initialStatus: "active", - startTask: false, - }) - expect(childStart).not.toHaveBeenCalled() - expect(removeClineFromStack).toHaveBeenNthCalledWith(1, { skipDelegationRepair: true }) - expect(removeClineFromStack).toHaveBeenNthCalledWith(2, { skipDelegationRepair: true }) - expect(deleteTaskFromState).toHaveBeenCalledWith("child-1") - expect(createTaskWithHistoryItem).toHaveBeenCalledWith( - { ...parentHistory, mode: "architect" }, - { startTask: false }, - ) - expect(providerEmit).not.toHaveBeenCalledWith(RooCodeEventName.TaskDelegated, "parent-1", "child-1") - }) - - it("logs rollback cleanup failures while preserving the parent metadata error", async () => { const parentTask = { taskId: "parent-1", emit: vi.fn(), - getTaskMode: vi.fn().mockRejectedValue(new Error("mode unavailable")), } as any const childStart = vi.fn() - const persistError = new Error("history write failed") - const log = vi.fn() - - const removeClineFromStack = vi - .fn() - .mockResolvedValueOnce(undefined) - .mockRejectedValueOnce(new Error("child stack cleanup failed")) - const deleteTaskFromState = vi.fn().mockRejectedValue(new Error("child history cleanup failed")) - const createTaskWithHistoryItem = vi.fn().mockRejectedValue(new Error("parent rehydrate failed")) - const parentHistory = { - id: "parent-1", - task: "Parent", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - childIds: [], - mode: "ask", - } + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const deleteTaskWithId = vi.fn().mockResolvedValue(undefined) + const createTaskWithHistoryItem = vi.fn().mockResolvedValue(undefined) const provider = { emit: vi.fn(), @@ -246,10 +172,10 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { createTask: vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart }), getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentHistory }), updateTaskHistory: vi.fn().mockRejectedValue(persistError), - deleteTaskFromState, - createTaskWithHistoryItem, handleModeSwitch: vi.fn().mockResolvedValue(undefined), - log, + deleteTaskWithId, + createTaskWithHistoryItem, + log: vi.fn(), } as unknown as ClineProvider await expect( @@ -259,16 +185,12 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { initialTodos: [], mode: "code", }), - ).rejects.toThrow("history write failed") + ).rejects.toThrow(persistError) expect(childStart).not.toHaveBeenCalled() - expect(deleteTaskFromState).toHaveBeenCalledWith("child-1") - expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistory, { startTask: false }) - expect(log).toHaveBeenCalledWith(expect.stringContaining("Failed to capture parent mode")) - expect(log).toHaveBeenCalledWith( - expect.stringContaining("Failed to remove child child-1 after parent metadata"), - ) - expect(log).toHaveBeenCalledWith(expect.stringContaining("Failed to remove child child-1 history")) - expect(log).toHaveBeenCalledWith(expect.stringContaining("Failed to rehydrate parent parent-1")) + expect(removeClineFromStack).toHaveBeenNthCalledWith(1, { skipDelegationRepair: true }) + expect(removeClineFromStack).toHaveBeenNthCalledWith(2, { skipDelegationRepair: true }) + expect(deleteTaskWithId).toHaveBeenCalledWith("child-1", false) + expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistory) }) }) diff --git a/src/__tests__/removeClineFromStack-delegation.spec.ts b/src/__tests__/removeClineFromStack-delegation.spec.ts index a72f580d6f..6ed2a5c221 100644 --- a/src/__tests__/removeClineFromStack-delegation.spec.ts +++ b/src/__tests__/removeClineFromStack-delegation.spec.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi } from "vitest" import { ClineProvider } from "../core/webview/ClineProvider" +import { makeProviderStub } from "./helpers/provider-stub" describe("ClineProvider.removeClineFromStack() delegation awareness", () => { /** @@ -32,13 +33,13 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { throw new Error("Task not found") }) - const provider = { + const provider = makeProviderStub({ clineStack: [childTask] as any[], taskEventListeners: new Map(), log: vi.fn(), getTaskWithId, updateTaskHistory, - } + }) return { provider, childTask, updateTaskHistory, getTaskWithId } } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index fcdfd0263d..454109778b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -3470,7 +3470,13 @@ export class Task extends EventEmitter implements TaskLike { // this.userMessageContentReady = true // } - await pWaitFor(() => this.userMessageContentReady) + await pWaitFor(() => this.userMessageContentReady || this.abort || this.abandoned) + + if (this.abort || this.abandoned) { + throw new Error( + `[RooCode#recursivelyMakeRooRequests] task ${this.taskId}.${this.instanceId} aborted`, + ) + } // If the model did not tool use, then we need to tell it to // either use a tool or attempt_completion. diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 3c68b482a6..b213c0862d 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -120,6 +120,9 @@ function runDelegationTransition( fn: () => Promise, ): Promise { const previous = locks.get(parentTaskId) ?? Promise.resolve() + // Fail-forward: run fn even if the previous transition rejected. A failed + // cancelTask must not permanently block a subsequent reopenParentFromDelegation. + // The cancelledDelegationChildIds guard inside each fn is the safety net. const current = previous.then(fn, fn) const tail = current.then( () => {}, @@ -512,7 +515,7 @@ export class ClineProvider // child and will update the parent to point at the new child. if (parentTaskId && childTaskId && !options?.skipDelegationRepair) { try { - await ClineProvider.prototype.runDelegationTransition.call(this, parentTaskId, async () => { + await this.runDelegationTransition(parentTaskId, async () => { const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === childTaskId) { @@ -2998,7 +3001,7 @@ export class ClineProvider if (task.parentTaskId) { try { - await ClineProvider.prototype.runDelegationTransition.call(this, task.parentTaskId, async () => { + await this.runDelegationTransition(task.parentTaskId, async () => { const { historyItem: parentHistory } = await this.getTaskWithId(task.parentTaskId!) if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === task.taskId) { @@ -3013,6 +3016,7 @@ export class ClineProvider ) parentTask = undefined rootTask = undefined + // Clear any stale fail-closed entry from a prior failed cancel attempt. this.cancelledDelegationChildIds.delete(task.taskId) } }) @@ -3036,6 +3040,7 @@ export class ClineProvider historyError instanceof Error ? historyError.message : String(historyError) }`, ) + throw historyError } this.log( `[cancelTask] Failed to detach delegated parent for ${task.taskId}: ${ @@ -3230,16 +3235,6 @@ export class ClineProvider `[delegateParentAndOpenChild] Parent mismatch: expected ${parentTaskId}, current ${parent.taskId}`, ) } - let parentModeBeforeDelegation: string | undefined - try { - parentModeBeforeDelegation = await parent.getTaskMode() - } catch (error) { - this.log( - `[delegateParentAndOpenChild] Failed to capture parent mode for ${parentTaskId} before delegation (non-fatal): ${ - error instanceof Error ? error.message : String(error) - }`, - ) - } // 2) Flush pending tool results to API history BEFORE disposing the parent. // This is critical: when tools are called before new_task, // their tool_result blocks are in userMessageContent but not yet saved to API history. @@ -3320,10 +3315,8 @@ export class ClineProvider }) // 5) Persist parent delegation metadata BEFORE the child starts writing. - let parentHistoryBeforeDelegation: HistoryItem | undefined try { const { historyItem } = await this.getTaskWithId(parentTaskId) - parentHistoryBeforeDelegation = historyItem const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId])) const updatedHistory: typeof historyItem = { ...historyItem, @@ -3341,38 +3334,31 @@ export class ClineProvider ) try { await this.removeClineFromStack({ skipDelegationRepair: true }) - } catch (cleanupErr) { + } catch (cleanupError) { this.log( - `[delegateParentAndOpenChild] Failed to remove child ${child.taskId} after parent metadata persistence failure (non-fatal): ${ - (cleanupErr as Error)?.message ?? String(cleanupErr) + `[delegateParentAndOpenChild] Failed to close paused child ${child.taskId} during rollback: ${ + (cleanupError as Error)?.message ?? String(cleanupError) }`, ) } try { - await this.deleteTaskFromState(child.taskId) - } catch (cleanupErr) { + await this.deleteTaskWithId(child.taskId, false) + } catch (cleanupError) { this.log( - `[delegateParentAndOpenChild] Failed to remove child ${child.taskId} history after parent metadata persistence failure (non-fatal): ${ - (cleanupErr as Error)?.message ?? String(cleanupErr) + `[delegateParentAndOpenChild] Failed to delete paused child ${child.taskId} during rollback: ${ + (cleanupError as Error)?.message ?? String(cleanupError) }`, ) } - if (parentHistoryBeforeDelegation) { - try { - await this.createTaskWithHistoryItem( - { - ...parentHistoryBeforeDelegation, - mode: parentModeBeforeDelegation ?? parentHistoryBeforeDelegation.mode, - }, - { startTask: false }, - ) - } catch (cleanupErr) { - this.log( - `[delegateParentAndOpenChild] Failed to rehydrate parent ${parentTaskId} after parent metadata persistence failure (non-fatal): ${ - (cleanupErr as Error)?.message ?? String(cleanupErr) - }`, - ) - } + try { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + await this.createTaskWithHistoryItem(parentHistory) + } catch (rollbackError) { + this.log( + `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback: ${ + (rollbackError as Error)?.message ?? String(rollbackError) + }`, + ) } throw err } @@ -3399,7 +3385,7 @@ export class ClineProvider completionResultSummary: string }): Promise { const { parentTaskId, childTaskId, completionResultSummary } = params - return await (ClineProvider.prototype.runDelegationTransition.call(this, parentTaskId, async () => { + return this.runDelegationTransition(parentTaskId, async () => { const globalStoragePath = this.contextProxy.globalStorageUri.fsPath // 1) Load parent from history and current persisted messages @@ -3411,7 +3397,7 @@ export class ClineProvider // approving the subtask finish. If the parent no longer awaits this child, // routing output back would corrupt an unrelated task. if ( - (this.cancelledDelegationChildIds as Set | undefined)?.has(childTaskId) || + this.cancelledDelegationChildIds.has(childTaskId) || historyItem.status !== "delegated" || historyItem.awaitingChildId !== childTaskId ) { @@ -3455,7 +3441,14 @@ export class ClineProvider text: completionResultSummary, ts, } - parentClineMessages.push(subtaskUiMessage) + const lastParentClineMessage = parentClineMessages.at(-1) + if ( + lastParentClineMessage?.type !== "say" || + lastParentClineMessage.say !== "subtask_result" || + lastParentClineMessage.text !== completionResultSummary + ) { + parentClineMessages.push(subtaskUiMessage) + } await saveTaskMessages({ messages: parentClineMessages, taskId: parentTaskId, globalStoragePath }) // Find the tool_use_id from the last assistant message's new_task tool_use @@ -3518,21 +3511,45 @@ export class ClineProvider } else { // If there is no corresponding tool_use in the parent API history, we cannot emit a // tool_result. Fall back to a plain user text note so the parent can still resume. - parentApiMessages.push({ - role: "user", - content: [ - { - type: "text" as const, - text: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, - }, - ], - ts, - }) + const fallbackText = `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}` + const lastParentApiMessage = parentApiMessages.at(-1) + const alreadyHasFallback = + lastParentApiMessage?.role === "user" && + Array.isArray(lastParentApiMessage.content) && + lastParentApiMessage.content.some( + (block: { type?: string; text?: string }) => + block.type === "text" && block.text === fallbackText, + ) + if (!alreadyHasFallback) { + parentApiMessages.push({ + role: "user", + content: [ + { + type: "text" as const, + text: fallbackText, + }, + ], + ts, + }) + } } await saveApiMessages({ messages: parentApiMessages as any, taskId: parentTaskId, globalStoragePath }) - // 3) Close child instance if still open (single-open-task invariant). + // 3) Persist parent metadata before closing the child. If persistence fails, + // the delegated child remains active and can retry completion. + const childIds = Array.from(new Set([...(historyItem.childIds ?? []), childTaskId])) + const updatedHistory: typeof historyItem = { + ...historyItem, + status: "active", + completedByChildId: childTaskId, + completionResultSummary, + awaitingChildId: undefined, + childIds, + } + await this.updateTaskHistory(updatedHistory) + + // 4) Close child instance if still open (single-open-task invariant). // This MUST happen BEFORE updating the child's status to "completed" because // removeClineFromStack() → abortTask(true) → saveClineMessages() writes // the historyItem with initialStatus (typically "active"), which would @@ -3542,9 +3559,9 @@ export class ClineProvider await this.removeClineFromStack({ skipDelegationRepair: true }) } - // 4) Update child metadata to "completed" status. + // 5) Update child metadata to "completed" status. // This runs after the abort so it overwrites the stale "active" status - // that saveClineMessages() may have written during step 3. + // that saveClineMessages() may have written during step 4. try { const { historyItem: childHistory } = await this.getTaskWithId(childTaskId) await this.updateTaskHistory({ @@ -3559,18 +3576,6 @@ export class ClineProvider ) } - // 5) Update parent metadata and persist BEFORE emitting completion event - const childIds = Array.from(new Set([...(historyItem.childIds ?? []), childTaskId])) - const updatedHistory: typeof historyItem = { - ...historyItem, - status: "active", - completedByChildId: childTaskId, - completionResultSummary, - awaitingChildId: undefined, - childIds, - } - await this.updateTaskHistory(updatedHistory) - // 6) Emit TaskDelegationCompleted (provider-level) try { this.emit(RooCodeEventName.TaskDelegationCompleted, parentTaskId, childTaskId, completionResultSummary) @@ -3606,9 +3611,9 @@ export class ClineProvider // non-fatal } - ;(this.cancelledDelegationChildIds as Set | undefined)?.delete(childTaskId) + this.cancelledDelegationChildIds.delete(childTaskId) return true - }) as Promise) + }) } /** diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts index 6b1879e433..57c040e661 100644 --- a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -463,4 +463,50 @@ describe("ClineProvider flicker-free cancel", () => { ) expect((provider as any).cancelledDelegationChildIds.has("child-1")).toBe(true) }) + + it("does not rehydrate a cancelled child when standalone persistence also fails", async () => { + const childHistory: HistoryItem = { + id: "child-1", + number: 2, + task: "child task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + parentTaskId: "parent-1", + rootTaskId: "root-1", + } + + Object.assign(mockTask1, { + taskId: "child-1", + instanceId: "instance-child", + parentTaskId: "parent-1", + cancelCurrentRequest: vi.fn(), + abortTask: vi.fn(), + abandoned: false, + isStreaming: false, + didFinishAbortingStream: true, + isWaitingForFirstChunk: false, + }) + ;(provider as any).clineStack = [mockTask1] + provider.getTaskWithId = vi.fn().mockImplementation((id) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: childHistory }) + } + if (id === "parent-1") { + return Promise.reject(new Error("parent lookup failed")) + } + throw new Error(`unexpected task lookup: ${id}`) + }) as any + + vi.spyOn(provider, "updateTaskHistory").mockRejectedValue(new Error("standalone persist failed")) + const createTaskWithHistoryItemSpy = vi + .spyOn(provider, "createTaskWithHistoryItem") + .mockResolvedValue(undefined as any) + + await expect(provider.cancelTask()).rejects.toThrow("standalone persist failed") + expect(createTaskWithHistoryItemSpy).not.toHaveBeenCalled() + expect((provider as any).cancelledDelegationChildIds.has("child-1")).toBe(true) + }) })