diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts new file mode 100644 index 0000000000..33bb2c9cbf --- /dev/null +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -0,0 +1,95 @@ +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" + +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 requestContains = (req: ChatCompletionRequest, expected: string[]) => { + const rawRequest = JSON.stringify(req) + return expected.every((text) => rawRequest.includes(text)) +} + +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`, + ]), + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: "9" }), + id: completionId, + }, + ], + }, +}) + +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(completionAfterAnswer("call_subtasks_child_followup_001", "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..51cf24f6a3 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -2,73 +2,191 @@ import * as assert from "assert" import { RooCodeEventName, type ClineMessage } from "@roo-code/types" -import { sleep, waitFor, waitUntilCompleted } from "./utils" +import { setDefaultSuiteTimeout } from "./test-utils" +import { waitFor, waitUntilCompleted } from "./utils" +import { SUBTASK_CHILD_FOLLOWUP_ANSWER, SUBTASK_PARENT_PROMPT } from "../fixtures/subtasks" -suite.skip("Roo Code Subtasks", () => { - test("Should handle subtask cancellation and resumption correctly", async () => { +suite("Roo Code Subtasks", function () { + setDefaultSuiteTimeout(this) + + // 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 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 = {} - api.on(RooCodeEventName.Message, ({ taskId, 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) } - }) - - 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) + + 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, + ) + + const cancelledChildTaskId = spawnedTaskId! + await api.cancelCurrentTask() - // Start a new task with the same message as the subtask. - const anotherTaskId = await api.startNewTask({ text: childPrompt }) - await waitUntilCompleted({ api, taskId: anotherTaskId }) + await waitFor(() => api.getCurrentTaskStack().at(-1) === cancelledChildTaskId) + await waitFor( + () => + asks[cancelledChildTaskId]?.some(({ type, ask }) => type === "ask" && ask === "resume_task") ?? + false, + ) - // Wait a bit to ensure any task resumption would have happened. - await sleep(2_000) + const resumedChildTaskId = await waitUntilCompleted({ + api, + start: async () => { + await api.sendMessage(SUBTASK_CHILD_FOLLOWUP_ANSWER) + return cancelledChildTaskId + }, + }) - // The parent task should still not have resumed. - assert.ok( - messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === + assert.strictEqual( + resumedChildTaskId, + cancelledChildTaskId, + "Cancelled child task should be resumed in place", + ) + assert.strictEqual( + findErrorText(resumedChildTaskId), undefined, - "Parent task should not have resumed after subtask cancellation", - ) + "Resumed child task should not emit an error", + ) + assert.strictEqual( + findCompletionText(resumedChildTaskId), + "9", + "Resumed child task should complete with `9`", + ) + assert.strictEqual( + api.getCurrentTaskStack().at(-1), + cancelledChildTaskId, + "Cancelled child task should remain the active completed task", + ) + assert.ok( + messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === + undefined, + "Parent task should not have resumed after the cancelled child completed", + ) - // 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/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 a78c41b7c0..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([]) @@ -110,6 +111,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", @@ -120,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: { @@ -145,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 }] @@ -203,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: { @@ -228,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 }] @@ -289,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: { @@ -314,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 }] @@ -349,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: { @@ -369,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([]) @@ -389,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: { @@ -413,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([]) @@ -455,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") { @@ -491,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([]) @@ -502,7 +504,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) @@ -525,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: { @@ -549,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([]) @@ -578,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") { @@ -613,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([]) @@ -658,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") { @@ -694,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([]) @@ -705,7 +707,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( @@ -723,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: { @@ -748,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([]) @@ -760,7 +803,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 +827,138 @@ 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) => + makeProviderStub({ + 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 any) + + 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 = makeProviderStub({ + 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 any) + + 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) => + makeProviderStub({ + 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 any) + + 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")) + }) + + 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 4b04fb5bbb..bd4519b970 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -98,7 +98,10 @@ 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(), + } as any const childStart = vi.fn(() => callOrder.push("child.start")) const updateTaskHistory = vi.fn(async () => { @@ -142,4 +145,52 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { // Verify ordering: createTask → updateTaskHistory → child.start expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"]) }) + + 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, + mode: "code", + } + const parentTask = { + taskId: "parent-1", + emit: vi.fn(), + } as any + const childStart = vi.fn() + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const deleteTaskWithId = vi.fn().mockResolvedValue(undefined) + const createTaskWithHistoryItem = vi.fn().mockResolvedValue(undefined) + + 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), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + deleteTaskWithId, + createTaskWithHistoryItem, + 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(persistError) + + expect(childStart).not.toHaveBeenCalled() + 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/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/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/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index a70576d75f..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"> { @@ -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,18 +97,28 @@ 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) + historyLookupTaskId = task.parentTaskId + 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 + } else { + // Parent already detached, such as when the user cancelled this child. + // Fall through to the normal completion ask flow. } - 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 @@ -121,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 @@ -167,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 6c9b9a2ccc..b64b6c57a0 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -484,6 +484,153 @@ 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(true), + } + + 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("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", + 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 d5d34d0e91..b213c0862d 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -114,6 +114,32 @@ export type ClineProviderEvents = { clineCreated: [cline: Task] } +function runDelegationTransition( + locks: Map>, + parentTaskId: string, + 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( + () => {}, + () => {}, + ) + + 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 +154,8 @@ export class ClineProvider private webviewDisposables: vscode.Disposable[] = [] private view?: vscode.WebviewView | vscode.WebviewPanel private clineStack: Task[] = [] + private delegationTransitionLocks?: Map> + private cancelledDelegationChildIds = new Set() private codeIndexStatusSubscription?: vscode.Disposable private codeIndexManager?: CodeIndexManager private _workspaceTracker?: WorkspaceTracker // workSpaceTracker read-only for access outside this class @@ -146,6 +174,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 @@ -482,18 +515,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 this.runDelegationTransition(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( @@ -2869,7 +2904,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`, @@ -2903,8 +2940,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 +2999,57 @@ export class ClineProvider return } + if (task.parentTaskId) { + try { + await this.runDelegationTransition(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, + }) + + this.log( + `[cancelTask] Detached delegated parent ${task.parentTaskId}: delegated → active (child ${task.taskId} cancelled)`, + ) + parentTask = undefined + rootTask = undefined + // Clear any stale fail-closed entry from a prior failed cancel attempt. + this.cancelledDelegationChildIds.delete(task.taskId) + } + }) + } catch (error) { + // 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) + }`, + ) + throw historyError + } + 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 }) } @@ -3244,6 +3332,35 @@ export class ClineProvider (err as Error)?.message ?? String(err) }`, ) + try { + await this.removeClineFromStack({ skipDelegationRepair: true }) + } catch (cleanupError) { + this.log( + `[delegateParentAndOpenChild] Failed to close paused child ${child.taskId} during rollback: ${ + (cleanupError as Error)?.message ?? String(cleanupError) + }`, + ) + } + try { + await this.deleteTaskWithId(child.taskId, false) + } catch (cleanupError) { + this.log( + `[delegateParentAndOpenChild] Failed to delete paused child ${child.taskId} during rollback: ${ + (cleanupError as Error)?.message ?? String(cleanupError) + }`, + ) + } + 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 } // 6) Start the child task now that parent metadata is safely persisted. @@ -3266,196 +3383,237 @@ export class ClineProvider parentTaskId: string childTaskId: string completionResultSummary: string - }): Promise { + }): Promise { const { parentTaskId, childTaskId, completionResultSummary } = params - const globalStoragePath = this.contextProxy.globalStorageUri.fsPath - - // 1) Load parent from history and current persisted messages - const { historyItem } = await this.getTaskWithId(parentTaskId) + return this.runDelegationTransition(parentTaskId, async () => { + const globalStoragePath = this.contextProxy.globalStorageUri.fsPath - let parentClineMessages: ClineMessage[] = [] - try { - parentClineMessages = await readTaskMessages({ - taskId: parentTaskId, - globalStoragePath, - }) - } catch { - parentClineMessages = [] - } + // 1) Load parent from history and current persisted messages + const { historyItem } = await this.getTaskWithId(parentTaskId) - 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 - } - } - - // 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 - } - } + // 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.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 } - // 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, + let parentClineMessages: ClineMessage[] = [] + try { + parentClineMessages = await readTaskMessages({ + taskId: parentTaskId, + globalStoragePath, }) + } catch { + parentClineMessages = [] } - // 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 + let parentApiMessages: any[] = [] + try { + parentApiMessages = (await readApiMessages({ + taskId: parentTaskId, + globalStoragePath, + })) as any[] + } catch { + parentApiMessages = [] } - } 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}`, - }, - ], + + // 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, - }) - } + } + 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 + 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 + } + } - await saveApiMessages({ messages: parentApiMessages as any, taskId: parentTaskId, globalStoragePath }) + // 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 + } + } + } - // 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() - } + // 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, + }) + } - // 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) - }`, - ) - } + // 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. + 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, + }) + } + } - // 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) + await saveApiMessages({ messages: parentApiMessages as any, taskId: parentTaskId, globalStoragePath }) - // 6) Emit TaskDelegationCompleted (provider-level) - try { - this.emit(RooCodeEventName.TaskDelegationCompleted, parentTaskId, childTaskId, completionResultSummary) - } catch { - // non-fatal - } + // 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) - // 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 }) + // 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 + // overwrite a "completed" status set earlier. + const current = this.getCurrentTask() + if (current?.taskId === childTaskId) { + await this.removeClineFromStack({ skipDelegationRepair: true }) + } - // 8) Inject restored histories into the in-memory instance before resuming - if (parentInstance) { + // 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 4. try { - await parentInstance.overwriteClineMessages(parentClineMessages) + 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) + }`, + ) + } + + // 6) Emit TaskDelegationCompleted (provider-level) + try { + 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.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 4bb01347a3..57c040e661 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,197 @@ 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("detaches runtime parent links 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 updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([]) + 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(updateTaskHistorySpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: "child-1", + parentTaskId: undefined, + rootTaskId: undefined, + }), + ) + expect(createTaskWithHistoryItemSpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: "child-1", + parentTaskId: undefined, + rootTaskId: undefined, + parentTask: undefined, + rootTask: undefined, + }), + ) + 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) + }) })