diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts new file mode 100644 index 0000000000..1d46291474 --- /dev/null +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -0,0 +1,117 @@ +import { LLMock } 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.` +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_MARKER), + }, + 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_MARKER), + }, + 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", + 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: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: "9" }), + id: "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({ + 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 b2550559b1..51b3e412aa 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" @@ -92,6 +93,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..d18fb1fd05 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -2,73 +2,142 @@ 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_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", - ) + } + + 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) - // Start a new task with the same message as the subtask. - const anotherTaskId = await api.startNewTask({ text: childPrompt }) - await waitUntilCompleted({ api, taskId: anotherTaskId }) + api.on(RooCodeEventName.Message, messageHandler) - // Wait a bit to ensure any task resumption would have happened. - await sleep(2_000) + try { + const parentTaskId = await api.startNewTask({ + configuration: { + mode: "ask", + alwaysAllowModeSwitch: true, + alwaysAllowSubtasks: true, + autoApprovalEnabled: true, + enableCheckpoints: false, + }, + text: SUBTASK_PARENT_PROMPT, + }) - // The parent task should still not have resumed. - assert.ok( - messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === + 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, + ) + 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, + "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", + () => + 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 }) + + assert.strictEqual( + findErrorText(cancelledChildTaskId), undefined, - "Parent task should not have resumed after subtask cancellation", - ) + "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) + + 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 }) + await api.clearCurrentTask() + } finally { + api.off(RooCodeEventName.Message, messageHandler) + } }) }) 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 37e99054b1..c008f93284 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2900,8 +2900,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" @@ -2959,6 +2959,29 @@ 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, + }) + + 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..766cd69eba 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,142 @@ describe("ClineProvider flicker-free cancel", () => { expect((provider as any).clineStack[0]).toBe(mockParentTask) expect((provider as any).clineStack[1]).toBe(mockTask2) }) + + 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 = { + 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: "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, + }), + ) + }) })