From e01cd31553e7d8dd8270f0ae79934f72cdea2a84 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Thu, 12 Feb 2026 00:24:58 -0700 Subject: [PATCH 1/3] fix: race conditions in subtask delegation system MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comprehensive fix for race conditions and error handling gaps in the subtask delegation system. Addresses multiple failure modes that could leave parent tasks permanently stuck in 'delegated' status, causing nested subtasks to hang. Key fixes: - Remove initialStatus from taskMetadata rebuild (eliminates status overwrites) - Persist delegation metadata to per-task files (resolves globalState eviction) - Add delegationInProgress mutex guard (prevents concurrent delegation ops) - TOCTOU race fixes with fresh re-reads before writes - Abort-aware pWaitFor predicate (prevents false 60s timeout on user input) - Remove silent .catch(() => {}) — all errors now logged unless task is aborting - Single-attempt delegation with parent repair on failure (no retry band-aids) - Cancel debouncedEmitTokenUsage in dispose() (prevents zombie callbacks) - new_task isolation truncation for parallel tool calls --- packages/types/src/task.ts | 2 - .../history-resume-delegation.spec.ts | 9 + src/__tests__/provider-delegation.spec.ts | 39 +- .../removeClineFromStack-delegation.spec.ts | 9 + .../presentAssistantMessage.ts | 23 +- .../__tests__/delegationMeta.spec.ts | 178 ++++ src/core/task-persistence/delegationMeta.ts | 138 +++ src/core/task-persistence/index.ts | 1 + src/core/task-persistence/taskMetadata.ts | 7 - src/core/task/Task.ts | 105 ++- src/core/tools/AttemptCompletionTool.ts | 96 ++- src/core/tools/NewTaskTool.ts | 8 +- .../attemptCompletionDelegation.spec.ts | 311 +++++++ .../__tests__/attemptCompletionTool.spec.ts | 125 +++ src/core/tools/__tests__/newTaskTool.spec.ts | 14 +- src/core/webview/ClineProvider.ts | 816 ++++++++++++------ .../webview/__tests__/ClineProvider.spec.ts | 3 + .../removeClineFromStack-delegation.spec.ts | 210 +++++ src/core/webview/webviewMessageHandler.ts | 12 +- src/shared/globalFileNames.ts | 1 + 20 files changed, 1755 insertions(+), 352 deletions(-) create mode 100644 src/core/task-persistence/__tests__/delegationMeta.spec.ts create mode 100644 src/core/task-persistence/delegationMeta.ts create mode 100644 src/core/tools/__tests__/attemptCompletionDelegation.spec.ts create mode 100644 src/core/webview/__tests__/removeClineFromStack-delegation.spec.ts diff --git a/packages/types/src/task.ts b/packages/types/src/task.ts index 55b442cca05..f45ab98d8a9 100644 --- a/packages/types/src/task.ts +++ b/packages/types/src/task.ts @@ -93,8 +93,6 @@ export interface CreateTaskOptions { consecutiveMistakeLimit?: number experiments?: Record initialTodos?: TodoItem[] - /** Initial status for the task's history item (e.g., "active" for child tasks) */ - initialStatus?: "active" | "delegated" | "completed" /** Whether to start the task loop immediately (default: true). * When false, the caller must invoke `task.start()` manually. */ startTask?: boolean diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index fae79bc2912..287b926a04d 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -38,6 +38,10 @@ vi.mock("../core/task-persistence", async (importOriginal) => { saveTaskMessages: vi.fn().mockResolvedValue(undefined), } }) +vi.mock("../core/task-persistence/delegationMeta", () => ({ + readDelegationMeta: vi.fn().mockResolvedValue(null), + saveDelegationMeta: vi.fn().mockResolvedValue(undefined), +})) import { ClineProvider } from "../core/webview/ClineProvider" import { readTaskMessages } from "../core/task-persistence/taskMessages" @@ -149,6 +153,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory: vi.fn().mockResolvedValue([]), + log: vi.fn(), } as unknown as ClineProvider // Start with existing messages in history @@ -232,6 +237,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory: vi.fn().mockResolvedValue([]), + log: vi.fn(), } as unknown as ClineProvider // Include an assistant message with new_task tool_use to exercise the tool_result path @@ -320,6 +326,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory: vi.fn().mockResolvedValue([]), + log: vi.fn(), } as unknown as ClineProvider // No assistant tool_use in history @@ -555,6 +562,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory: vi.fn().mockResolvedValue([]), + log: vi.fn(), } as unknown as ClineProvider vi.mocked(readTaskMessages).mockResolvedValue([]) @@ -754,6 +762,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), updateTaskHistory: vi.fn().mockResolvedValue([]), + log: vi.fn(), } as unknown as ClineProvider // Mock read failures or empty returns diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index 4b04fb5bbb9..4f624fed5ab 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -2,6 +2,12 @@ import { describe, it, expect, vi } from "vitest" import { RooCodeEventName } from "@roo-code/types" + +vi.mock("../core/task-persistence/delegationMeta", () => ({ + readDelegationMeta: vi.fn().mockResolvedValue(null), + saveDelegationMeta: vi.fn().mockResolvedValue(undefined), +})) + import { ClineProvider } from "../core/webview/ClineProvider" describe("ClineProvider.delegateParentAndOpenChild()", () => { @@ -9,7 +15,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { const providerEmit = vi.fn() const parentTask = { taskId: "parent-1", emit: vi.fn() } as any - const childStart = vi.fn() + const childStart = vi.fn().mockResolvedValue(undefined) const updateTaskHistory = vi.fn() const removeClineFromStack = vi.fn().mockResolvedValue(undefined) const createTask = vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart }) @@ -48,6 +54,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { updateTaskHistory, handleModeSwitch, log: vi.fn(), + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, } as unknown as ClineProvider const params = { @@ -63,18 +70,26 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { // Invariant: parent closed before child creation expect(removeClineFromStack).toHaveBeenCalledTimes(1) - // Child task is created with startTask: false and initialStatus: "active" + // Child task is created with startTask: false expect(createTask).toHaveBeenCalledWith("Do something", undefined, parentTask, { initialTodos: [], - initialStatus: "active", startTask: false, }) - // Metadata persistence - parent gets "delegated" status (child status is set at creation via initialStatus) - expect(updateTaskHistory).toHaveBeenCalledTimes(1) + // Metadata persistence - child gets "active" status, parent gets "delegated" status + expect(updateTaskHistory).toHaveBeenCalledTimes(2) - // Parent set to "delegated" - const parentSaved = updateTaskHistory.mock.calls[0][0] + // Child set to "active" (first call) + const childSaved = updateTaskHistory.mock.calls[0][0] + expect(childSaved).toEqual( + expect.objectContaining({ + id: "child-1", + status: "active", + }), + ) + + // Parent set to "delegated" (second call) + const parentSaved = updateTaskHistory.mock.calls[1][0] expect(parentSaved).toEqual( expect.objectContaining({ id: "parent-1", @@ -99,7 +114,10 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { const callOrder: string[] = [] const parentTask = { taskId: "parent-1", emit: vi.fn() } as any - const childStart = vi.fn(() => callOrder.push("child.start")) + const childStart = vi.fn(() => { + callOrder.push("child.start") + return Promise.resolve() + }) const updateTaskHistory = vi.fn(async () => { callOrder.push("updateTaskHistory") @@ -130,6 +148,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { updateTaskHistory, handleModeSwitch, log: vi.fn(), + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, } as unknown as ClineProvider await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { @@ -139,7 +158,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { mode: "code", }) - // Verify ordering: createTask → updateTaskHistory → child.start - expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"]) + // Verify ordering: createTask → updateTaskHistory (child) → updateTaskHistory (parent) → child.start + expect(callOrder).toEqual(["createTask", "updateTaskHistory", "updateTaskHistory", "child.start"]) }) }) diff --git a/src/__tests__/removeClineFromStack-delegation.spec.ts b/src/__tests__/removeClineFromStack-delegation.spec.ts index a72f580d6fe..adc7f1e659f 100644 --- a/src/__tests__/removeClineFromStack-delegation.spec.ts +++ b/src/__tests__/removeClineFromStack-delegation.spec.ts @@ -1,6 +1,12 @@ // npx vitest run __tests__/removeClineFromStack-delegation.spec.ts import { describe, it, expect, vi } from "vitest" + +vi.mock("../core/task-persistence/delegationMeta", () => ({ + readDelegationMeta: vi.fn().mockResolvedValue(null), + saveDelegationMeta: vi.fn().mockResolvedValue(undefined), +})) + import { ClineProvider } from "../core/webview/ClineProvider" describe("ClineProvider.removeClineFromStack() delegation awareness", () => { @@ -38,6 +44,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { log: vi.fn(), getTaskWithId, updateTaskHistory, + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, } return { provider, childTask, updateTaskHistory, getTaskWithId } @@ -183,6 +190,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { log: vi.fn(), getTaskWithId: vi.fn(), updateTaskHistory: vi.fn(), + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, } // Should not throw @@ -263,6 +271,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { log: vi.fn(), getTaskWithId, updateTaskHistory, + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, } // Simulate what delegateParentAndOpenChild does: pop B with skipDelegationRepair diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 6d7d0c366cc..98b4e37db3b 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) { + if (cline.abort || cline.abandoned) { throw new Error(`[Task#presentAssistantMessage] task ${cline.taskId}.${cline.instanceId} aborted`) } @@ -938,6 +938,15 @@ export async function presentAssistantMessage(cline: Task) { // locked. cline.presentAssistantMessageLocked = false + // Early exit if task was aborted/abandoned during tool execution (e.g., new_task delegation). + // Prevents unhandled promise rejections from recursive calls hitting the abort check. + if (cline.abort || cline.abandoned) { + if (cline.didCompleteReadingStream) { + cline.userMessageContentReady = true + } + return + } + // NOTE: When tool is rejected, iterator stream is interrupted and it waits // for `userMessageContentReady` to be true. Future calls to present will // skip execution since `didRejectTool` and iterate until `contentIndex` is @@ -965,7 +974,11 @@ export async function presentAssistantMessage(cline: Task) { if (cline.currentStreamingContentIndex < cline.assistantMessageContent.length) { // There are already more content blocks to stream, so we'll call // this function ourselves. - presentAssistantMessage(cline) + presentAssistantMessage(cline).catch((err) => { + if (!cline.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) return } else { // CRITICAL FIX: If we're out of bounds and the stream is complete, set userMessageContentReady @@ -978,7 +991,11 @@ export async function presentAssistantMessage(cline: Task) { // Block is partial, but the read stream may have finished. if (cline.presentAssistantMessageHasPendingUpdates) { - presentAssistantMessage(cline) + presentAssistantMessage(cline).catch((err) => { + if (!cline.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) } } diff --git a/src/core/task-persistence/__tests__/delegationMeta.spec.ts b/src/core/task-persistence/__tests__/delegationMeta.spec.ts new file mode 100644 index 00000000000..5f46c87f872 --- /dev/null +++ b/src/core/task-persistence/__tests__/delegationMeta.spec.ts @@ -0,0 +1,178 @@ +import { describe, it, expect, vi, beforeEach } from "vitest" +import * as os from "os" +import * as path from "path" +import * as fs from "fs/promises" + +// Mocks (use hoisted to avoid initialization ordering issues) +const hoisted = vi.hoisted(() => ({ + safeWriteJsonMock: vi.fn().mockResolvedValue(undefined), +})) +vi.mock("../../../utils/safeWriteJson", () => ({ + safeWriteJson: hoisted.safeWriteJsonMock, +})) + +// Import after mocks +import { readDelegationMeta, saveDelegationMeta } from "../delegationMeta" +import type { DelegationMeta } from "../delegationMeta" + +let tmpBaseDir: string + +beforeEach(async () => { + hoisted.safeWriteJsonMock.mockClear() + tmpBaseDir = await fs.mkdtemp(path.join(os.tmpdir(), "roo-test-delegation-")) +}) + +describe("delegationMeta.readDelegationMeta", () => { + it("returns null when file does not exist (backward compat)", async () => { + const result = await readDelegationMeta({ + taskId: "task-no-file", + globalStoragePath: tmpBaseDir, + }) + + expect(result).toBeNull() + }) + + it("returns parsed delegation metadata from file", async () => { + const taskId = "task-with-meta" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + const filePath = path.join(taskDir, "delegation_metadata.json") + + const meta: DelegationMeta = { + status: "delegated", + delegatedToId: "child-1", + awaitingChildId: "child-1", + childIds: ["child-1"], + } + await fs.writeFile(filePath, JSON.stringify(meta), "utf8") + + const result = await readDelegationMeta({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toEqual(meta) + }) + + it("filters out unknown keys from parsed data", async () => { + const taskId = "task-extra-keys" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + const filePath = path.join(taskDir, "delegation_metadata.json") + + await fs.writeFile( + filePath, + JSON.stringify({ + status: "active", + unknownField: "should-be-filtered", + childIds: ["c1"], + }), + "utf8", + ) + + const result = await readDelegationMeta({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toEqual({ status: "active", childIds: ["c1"] }) + expect(result).not.toHaveProperty("unknownField") + }) + + it("returns null when file contains invalid JSON", async () => { + const taskId = "task-corrupt" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + const filePath = path.join(taskDir, "delegation_metadata.json") + await fs.writeFile(filePath, "{not valid json!!!", "utf8") + + const result = await readDelegationMeta({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toBeNull() + }) + + it("returns null when file contains a non-object value", async () => { + const taskId = "task-non-object" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + const filePath = path.join(taskDir, "delegation_metadata.json") + await fs.writeFile(filePath, JSON.stringify("hello"), "utf8") + + const result = await readDelegationMeta({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toBeNull() + }) + + it("returns null when file contains an array", async () => { + const taskId = "task-array" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + const filePath = path.join(taskDir, "delegation_metadata.json") + await fs.writeFile(filePath, JSON.stringify([1, 2, 3]), "utf8") + + const result = await readDelegationMeta({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toBeNull() + }) +}) + +describe("delegationMeta.saveDelegationMeta", () => { + it("writes delegation metadata via safeWriteJson", async () => { + const meta: DelegationMeta = { + status: "delegated", + delegatedToId: "child-1", + awaitingChildId: "child-1", + childIds: ["child-1"], + } + + await saveDelegationMeta({ + taskId: "task-1", + globalStoragePath: tmpBaseDir, + meta, + }) + + expect(hoisted.safeWriteJsonMock).toHaveBeenCalledTimes(1) + const [, persisted] = hoisted.safeWriteJsonMock.mock.calls[0] + expect(persisted).toEqual(meta) + }) + + it("filters out unknown keys before writing", async () => { + const meta = { + status: "active", + unknownField: "should-be-filtered", + childIds: ["c1"], + } as any + + await saveDelegationMeta({ + taskId: "task-2", + globalStoragePath: tmpBaseDir, + meta, + }) + + expect(hoisted.safeWriteJsonMock).toHaveBeenCalledTimes(1) + const [, persisted] = hoisted.safeWriteJsonMock.mock.calls[0] + expect(persisted).toEqual({ status: "active", childIds: ["c1"] }) + expect(persisted).not.toHaveProperty("unknownField") + }) + + it("writes to the correct file path", async () => { + await saveDelegationMeta({ + taskId: "task-path-check", + globalStoragePath: tmpBaseDir, + meta: { status: "completed" }, + }) + + expect(hoisted.safeWriteJsonMock).toHaveBeenCalledTimes(1) + const [filePath] = hoisted.safeWriteJsonMock.mock.calls[0] + expect(filePath).toContain(path.join("tasks", "task-path-check", "delegation_metadata.json")) + }) +}) diff --git a/src/core/task-persistence/delegationMeta.ts b/src/core/task-persistence/delegationMeta.ts new file mode 100644 index 00000000000..8a5dfac7d8b --- /dev/null +++ b/src/core/task-persistence/delegationMeta.ts @@ -0,0 +1,138 @@ +import { safeWriteJson } from "../../utils/safeWriteJson" +import * as path from "path" +import * as fs from "fs/promises" + +import { fileExistsAtPath } from "../../utils/fs" + +import { GlobalFileNames } from "../../shared/globalFileNames" +import { getTaskDirectoryPath } from "../../utils/storage" + +/** + * Delegation metadata stored per-task on disk. + * This is the cross-process-safe source of truth for delegation fields, + * complementing the monolithic globalState taskHistory array. + */ +export interface DelegationMeta { + status?: "active" | "delegated" | "completed" + delegatedToId?: string | null + awaitingChildId?: string | null + childIds?: string[] + completedByChildId?: string | null + completionResultSummary?: string | null +} + +/** + * Compile-time safeguard: this record must list exactly the keys of DelegationMeta. + * Adding or removing a key in the interface without updating this record + * will produce a TypeScript error, preventing silent drift. + */ +const _delegationMetaKeyRecord: Record, true> = { + status: true, + delegatedToId: true, + awaitingChildId: true, + childIds: true, + completedByChildId: true, + completionResultSummary: true, +} + +/** Known keys that may appear in a DelegationMeta object. */ +const DELEGATION_META_KEYS: ReadonlySet = new Set(Object.keys(_delegationMetaKeyRecord)) + +export type ReadDelegationMetaOptions = { + taskId: string + globalStoragePath: string +} + +/** + * Read delegation metadata from the per-task file. + * Returns `null` if the file doesn't exist (backward compat for old tasks). + */ +export async function readDelegationMeta({ + taskId, + globalStoragePath, +}: ReadDelegationMetaOptions): Promise { + const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) + const filePath = path.join(taskDir, GlobalFileNames.delegationMetadata) + const fileExists = await fileExistsAtPath(filePath) + + if (!fileExists) { + return null + } + + try { + const raw = JSON.parse(await fs.readFile(filePath, "utf8")) + + if (typeof raw !== "object" || raw === null || Array.isArray(raw)) { + console.warn( + `[readDelegationMeta] Parsed data is not an object (got ${typeof raw}), returning null. TaskId: ${taskId}, Path: ${filePath}`, + ) + return null + } + + // Only extract known delegation keys to prevent pollution + const meta: DelegationMeta = {} + + for (const key of Object.keys(raw)) { + if (DELEGATION_META_KEYS.has(key)) { + ;(meta as Record)[key] = raw[key] + } + } + + // Validate value types to prevent corrupted files from propagating invalid data + if ( + meta.status !== undefined && + meta.status !== null && + !["active", "delegated", "completed"].includes(meta.status as string) + ) { + delete (meta as Record).status + } + if (meta.childIds !== undefined && meta.childIds !== null && !Array.isArray(meta.childIds)) { + delete (meta as Record).childIds + } + for (const key of [ + "delegatedToId", + "awaitingChildId", + "completedByChildId", + "completionResultSummary", + ] as const) { + const val = meta[key] + if (val !== undefined && val !== null && typeof val !== "string") { + delete (meta as Record)[key] + } + } + + return meta + } catch (error) { + console.warn( + `[readDelegationMeta] Failed to parse ${filePath} for task ${taskId}, returning null: ${error instanceof Error ? error.message : String(error)}`, + ) + return null + } +} + +export type SaveDelegationMetaOptions = { + taskId: string + globalStoragePath: string + meta: DelegationMeta +} + +/** + * Save delegation metadata to the per-task file. + * Writes the complete delegation state (not a merge) via safeWriteJson + * which uses proper-lockfile for cross-process safety. + */ +export async function saveDelegationMeta({ taskId, globalStoragePath, meta }: SaveDelegationMetaOptions) { + const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) + const filePath = path.join(taskDir, GlobalFileNames.delegationMetadata) + + // Only write known delegation keys to prevent pollution + const sanitized: DelegationMeta = {} + + for (const key of Object.keys(meta)) { + if (DELEGATION_META_KEYS.has(key)) { + ;(sanitized as Record)[key] = (meta as Record)[key] + } + } + + await safeWriteJson(filePath, sanitized) +} diff --git a/src/core/task-persistence/index.ts b/src/core/task-persistence/index.ts index 65e0cc1dc86..a26fb30de55 100644 --- a/src/core/task-persistence/index.ts +++ b/src/core/task-persistence/index.ts @@ -36,3 +36,4 @@ export { } from "./rooMessage" export { convertAnthropicToRooMessages } from "./converters/anthropicToRoo" export { flattenModelMessagesToStringContent } from "./messageUtils" +export { type DelegationMeta, readDelegationMeta, saveDelegationMeta } from "./delegationMeta" diff --git a/src/core/task-persistence/taskMetadata.ts b/src/core/task-persistence/taskMetadata.ts index 4b771269713..bd4b6023c4a 100644 --- a/src/core/task-persistence/taskMetadata.ts +++ b/src/core/task-persistence/taskMetadata.ts @@ -23,8 +23,6 @@ export type TaskMetadataOptions = { mode?: string /** Provider profile name for the task (sticky profile feature) */ apiConfigName?: string - /** Initial status for the task (e.g., "active" for child tasks) */ - initialStatus?: "active" | "delegated" | "completed" } export async function taskMetadata({ @@ -37,7 +35,6 @@ export async function taskMetadata({ workspace, mode, apiConfigName, - initialStatus, }: TaskMetadataOptions) { const taskDir = await getTaskDirectoryPath(globalStoragePath, id) @@ -90,9 +87,6 @@ export async function taskMetadata({ } // Create historyItem once with pre-calculated values. - // initialStatus is included when provided (e.g., "active" for child tasks) - // to ensure the status is set from the very first save, avoiding race conditions - // where attempt_completion might run before a separate status update. const historyItem: HistoryItem = { id, rootTaskId, @@ -111,7 +105,6 @@ export async function taskMetadata({ workspace, mode, ...(typeof apiConfigName === "string" && apiConfigName.length > 0 ? { apiConfigName } : {}), - ...(initialStatus && { status: initialStatus }), } return { historyItem, tokenUsage } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 71ddc54f10e..814b0e5f17f 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -182,8 +182,6 @@ export interface TaskOptions extends CreateTaskOptions { onCreated?: (task: Task) => void initialTodos?: TodoItem[] workspacePath?: string - /** Initial status for the task's history item (e.g., "active" for child tasks) */ - initialStatus?: "active" | "delegated" | "completed" } export class Task extends EventEmitter implements TaskLike { @@ -467,7 +465,11 @@ export class Task extends EventEmitter implements TaskLike { // Add to content and present this.assistantMessageContent.push(partialToolUse) this.userMessageContentReady = false - presentAssistantMessage(this) + presentAssistantMessage(this).catch((err) => { + if (!this.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) } else if (event.type === "tool_call_delta") { // Process chunk using streaming JSON parser const partialToolUse = NativeToolCallParser.processStreamingChunk(event.id, event.delta) @@ -483,7 +485,11 @@ export class Task extends EventEmitter implements TaskLike { this.assistantMessageContent[toolUseIndex] = partialToolUse // Present updated tool use - presentAssistantMessage(this) + presentAssistantMessage(this).catch((err) => { + if (!this.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) } } } else if (event.type === "tool_call_end") { @@ -509,7 +515,11 @@ export class Task extends EventEmitter implements TaskLike { this.userMessageContentReady = false // Present the finalized tool call - presentAssistantMessage(this) + presentAssistantMessage(this).catch((err) => { + if (!this.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) } else if (toolUseIndex !== undefined) { // finalizeStreamingToolCall returned null (malformed JSON or missing args) // Mark the tool as non-partial so it's presented as complete, but execution @@ -528,7 +538,11 @@ export class Task extends EventEmitter implements TaskLike { this.userMessageContentReady = false // Present the tool call - validation will handle missing params - presentAssistantMessage(this) + presentAssistantMessage(this).catch((err) => { + if (!this.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) } } } @@ -563,9 +577,6 @@ export class Task extends EventEmitter implements TaskLike { // Cloud Sync Tracking private cloudSyncedMessageTimestamps: Set = new Set() - // Initial status for the task's history item (set at creation time to avoid race conditions) - private readonly initialStatus?: "active" | "delegated" | "completed" - // MessageManager for high-level message operations (lazy initialized) private _messageManager?: MessageManager @@ -587,7 +598,6 @@ export class Task extends EventEmitter implements TaskLike { onCreated, initialTodos, workspacePath, - initialStatus, }: TaskOptions) { super() @@ -649,7 +659,6 @@ export class Task extends EventEmitter implements TaskLike { this.parentTask = parentTask this.taskNumber = taskNumber - this.initialStatus = initialStatus // Store the task's mode and API config name when it's created. // For history items, use the stored values; for new tasks, we'll set them @@ -1375,7 +1384,6 @@ export class Task extends EventEmitter implements TaskLike { workspace: this.cwd, mode: this._taskMode || defaultModeSlug, // Use the task's own mode, not the current provider mode. apiConfigName: this._taskApiConfigName, // Use the task's own provider profile, not the current provider profile. - initialStatus: this.initialStatus, }) // Emit token/tool usage updates using debounced function @@ -1388,7 +1396,10 @@ export class Task extends EventEmitter implements TaskLike { await this.providerRef.deref()?.updateTaskHistory(historyItem) return true } catch (error) { - console.error("Failed to save Roo messages:", error) + console.error( + `[Task#saveClineMessages] Failed to save Roo messages for task ${this.taskId}: ${error instanceof Error ? error.message : String(error)}`, + error, + ) return false } } @@ -2048,7 +2059,7 @@ export class Task extends EventEmitter implements TaskLike { * child task begins writing its own history (avoiding a read-modify-write * race on globalState). */ - public start(): void { + public start(): Promise | void { if (this._started) { return } @@ -2057,7 +2068,7 @@ export class Task extends EventEmitter implements TaskLike { const { task, images } = this.metadata if (task || images) { - this.runLifecycleTaskInBackground(this.startTask(task ?? undefined, images ?? undefined), "startTask") + return this.startTask(task ?? undefined, images ?? undefined) } } @@ -2089,6 +2100,27 @@ export class Task extends EventEmitter implements TaskLike { await this.say("text", task, images) + // Verify initial persistence succeeded — if saveClineMessages failed silently, + // this task will be invisible to globalState and delegation will break. + try { + const provider = this.providerRef.deref() + if (provider) { + const history = + ((provider as any).getGlobalState("taskHistory") as Array<{ id: string }> | undefined) ?? [] + if (!history.find((h) => h.id === this.taskId)) { + console.error( + `[Task#startTask] CRITICAL: Task ${this.taskId} not found in globalState after initial say(). ` + + `saveClineMessages may have failed silently. Retrying persistence...`, + ) + await this.saveClineMessages() + } + } + } catch (verifyErr) { + console.error( + `[Task#startTask] Failed to verify initial persistence: ${verifyErr instanceof Error ? verifyErr.message : String(verifyErr)}`, + ) + } + // Check for too many MCP tools and warn the user const { enabledToolCount, enabledServerCount } = await this.getEnabledMcpToolsCount() if (enabledToolCount > MAX_MCP_TOOLS_THRESHOLD) { @@ -2468,6 +2500,13 @@ export class Task extends EventEmitter implements TaskLike { console.error("Error cancelling current request:", error) } + // Cancel debounced token usage emitter to prevent zombie callbacks + try { + this.debouncedEmitTokenUsage.cancel() + } catch (error) { + console.error("Error cancelling debounced token usage emitter:", error) + } + // Remove provider profile change listener try { if (this.providerProfileChangeListener) { @@ -3089,7 +3128,11 @@ export class Task extends EventEmitter implements TaskLike { // Present the tool call to user - presentAssistantMessage will execute // tools sequentially and accumulate all results in userMessageContent - presentAssistantMessage(this) + presentAssistantMessage(this).catch((err) => { + if (!this.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) break } case "text": { @@ -3108,7 +3151,11 @@ export class Task extends EventEmitter implements TaskLike { }) this.userMessageContentReady = false } - presentAssistantMessage(this) + presentAssistantMessage(this).catch((err) => { + if (!this.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) break } case "response_message": @@ -3435,7 +3482,11 @@ export class Task extends EventEmitter implements TaskLike { this.userMessageContentReady = false // Present the finalized tool call - presentAssistantMessage(this) + presentAssistantMessage(this).catch((err) => { + if (!this.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) } else if (toolUseIndex !== undefined) { // finalizeStreamingToolCall returned null (malformed JSON or missing args) // We still need to mark the tool as non-partial so it gets executed @@ -3454,7 +3505,11 @@ export class Task extends EventEmitter implements TaskLike { this.userMessageContentReady = false // Present the tool call - validation will handle missing params - presentAssistantMessage(this) + presentAssistantMessage(this).catch((err) => { + if (!this.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) } } } @@ -3688,7 +3743,11 @@ export class Task extends EventEmitter implements TaskLike { // If there is content to update then it will complete and // update `this.userMessageContentReady` to true, which we // `pWaitFor` before making the next request. - presentAssistantMessage(this) + presentAssistantMessage(this).catch((err) => { + if (!this.abort) { + console.error("[presentAssistantMessage] Unhandled error:", err) + } + }) } if (hasTextContent || hasToolUses) { @@ -3708,7 +3767,11 @@ export class Task extends EventEmitter implements TaskLike { // this.userMessageContentReady = true // } - await pWaitFor(() => this.userMessageContentReady) + await pWaitFor(() => this.userMessageContentReady || this.abort) + + if (this.abort) { + return false + } // 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 a406a15c8b4..17bf7369e54 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -5,6 +5,7 @@ import { TelemetryService } from "@roo-code/telemetry" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" +import type { DelegationMeta } from "../task-persistence/delegationMeta" import { Package } from "../../shared/package" import type { ToolUse } from "../../shared/tools" import { t } from "../../i18n" @@ -31,6 +32,9 @@ interface DelegationProvider { childTaskId: string completionResultSummary: string }): Promise + updateTaskHistory(item: HistoryItem, options?: { broadcast?: boolean }): Promise + persistDelegationMeta(taskId: string, meta: DelegationMeta): Promise + readDelegationMeta(taskId: string): Promise } export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { @@ -124,10 +128,8 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { // Fall through to normal completion ask flow } } 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)}. ` + - `Skipping delegation.`, + `[AttemptCompletionTool] Delegation failed for task ${task.taskId}: ${err instanceof Error ? err.message : String(err)}. Falling through to standalone completion.`, ) // Fall through to normal completion ask flow } @@ -168,15 +170,89 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { return true } - pushToolResult("") + const parentTaskId = task.parentTaskId! + const childTaskId = task.taskId - await provider.reopenParentFromDelegation({ - parentTaskId: task.parentTaskId!, - childTaskId: task.taskId, - completionResultSummary: result, - }) + const attemptDelegation = async (): Promise => { + await provider.reopenParentFromDelegation({ + parentTaskId, + childTaskId, + completionResultSummary: result, + }) + } + + try { + await attemptDelegation() + pushToolResult("") + return true + } catch (error) { + // Delegation failed — repair parent status so it doesn't stay "delegated" + console.error( + `[AttemptCompletionTool] Delegation failed for task ${childTaskId}: ${ + error instanceof Error ? error.message : String(error) + }. Repairing parent and falling through to standalone completion.`, + ) + + try { + const { historyItem: parentHistory } = await provider.getTaskWithId(parentTaskId) + const childIds = Array.from(new Set([...(parentHistory.childIds ?? []), childTaskId])) + await provider.updateTaskHistory({ + ...parentHistory, + status: "active", + awaitingChildId: undefined, + childIds, + }) + await provider.persistDelegationMeta(parentTaskId, { + status: "active", + awaitingChildId: null, + delegatedToId: parentHistory.delegatedToId, + childIds, + completedByChildId: parentHistory.completedByChildId ?? null, + completionResultSummary: parentHistory.completionResultSummary ?? null, + }) + } catch (repairError) { + // Disk-only fallback when parent is missing from globalState. + // Uses read-merge-write to preserve fields like completedByChildId + // and completionResultSummary that may exist from prior delegations. + if (repairError instanceof Error && repairError.message === "Task not found") { + try { + const existingMeta = await provider.readDelegationMeta(parentTaskId) + await provider.persistDelegationMeta(parentTaskId, { + status: "active", + awaitingChildId: null, + delegatedToId: existingMeta?.delegatedToId ?? null, + childIds: existingMeta?.childIds + ? Array.from(new Set([...existingMeta.childIds, childTaskId])) + : [childTaskId], + completedByChildId: existingMeta?.completedByChildId ?? null, + completionResultSummary: existingMeta?.completionResultSummary ?? null, + }) + console.warn( + `[AttemptCompletionTool] Repaired parent ${parentTaskId} via disk fallback (not in globalState)`, + ) + } catch (diskErr) { + console.error( + `[AttemptCompletionTool] Disk fallback repair also failed for ${parentTaskId}: ${ + diskErr instanceof Error ? diskErr.message : String(diskErr) + }`, + ) + } + } else { + console.error( + `[AttemptCompletionTool] Failed to repair parent ${parentTaskId} after delegation failure: ${ + repairError instanceof Error ? repairError.message : String(repairError) + }`, + ) + } + } - return true + pushToolResult( + formatResponse.toolError( + `Delegation to parent task failed: ${error instanceof Error ? error.message : String(error)}. Completing as standalone task.`, + ), + ) + return true + } } override async handlePartial(task: Task, block: ToolUse<"attempt_completion">): Promise { diff --git a/src/core/tools/NewTaskTool.ts b/src/core/tools/NewTaskTool.ts index f36d8e1e379..cd7d798686d 100644 --- a/src/core/tools/NewTaskTool.ts +++ b/src/core/tools/NewTaskTool.ts @@ -110,15 +110,15 @@ export class NewTaskTool extends BaseTool<"new_task"> { } // Delegate parent and open child as sole active task - const child = await (provider as any).delegateParentAndOpenChild({ + await (provider as any).delegateParentAndOpenChild({ parentTaskId: task.taskId, message: unescapedMessage, initialTodos: todoItems, mode, }) - - // Reflect delegation in tool result (no pause/unpause, no wait) - pushToolResult(`Delegated to child task ${child.taskId}`) + // No pushToolResult needed — reopenParentFromDelegation injects the tool_result + // when the child completes. Any result pushed here would be orphaned because + // flushPendingToolResultsToHistory() already persisted userMessageContent. return } catch (error) { await handleError("creating new task", error) diff --git a/src/core/tools/__tests__/attemptCompletionDelegation.spec.ts b/src/core/tools/__tests__/attemptCompletionDelegation.spec.ts new file mode 100644 index 00000000000..3d0d9ddf0e2 --- /dev/null +++ b/src/core/tools/__tests__/attemptCompletionDelegation.spec.ts @@ -0,0 +1,311 @@ +// npx vitest core/tools/__tests__/attemptCompletionDelegation.spec.ts + +import type { HistoryItem } from "@roo-code/types" +import type { DelegationMeta } from "../../task-persistence/delegationMeta" + +// Mock vscode +vi.mock("vscode", () => ({ + workspace: { + getConfiguration: vi.fn(() => ({ + get: vi.fn(), + })), + }, + env: { + uriScheme: "vscode", + }, +})) + +vi.mock("../../../shared/package", () => ({ + Package: { + name: "roo-cline", + publisher: "RooVeterinaryInc", + version: "1.0.0", + outputChannel: "Roo-Code", + }, +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((msg: string) => `Tool Error: ${msg}`), + toolResult: vi.fn((text: string) => text), + toolDenied: vi.fn(() => "The user denied this operation."), + }, +})) + +vi.mock("../../../i18n", () => ({ + t: vi.fn((key: string) => key), +})) + +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureTaskCompleted: vi.fn(), + }, + }, +})) + +import { AttemptCompletionTool } from "../AttemptCompletionTool" + +/** Helper: minimal HistoryItem for a child task. */ +function childHistoryItem(overrides: Partial = {}): HistoryItem { + return { + id: "child-1", + number: 2, + ts: Date.now(), + task: "child task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + status: "active", + ...overrides, + } +} + +/** Helper: minimal HistoryItem for a parent task. */ +function parentHistoryItem(overrides: Partial = {}): HistoryItem { + return { + id: "parent-1", + number: 1, + ts: Date.now(), + task: "parent task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + status: "delegated", + awaitingChildId: "child-1", + delegatedToId: "mode-1", + childIds: ["child-1"], + completedByChildId: "old-child-0", + completionResultSummary: "old result summary", + ...overrides, + } +} + +function createMockProvider() { + return { + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentHistoryItem() }), + reopenParentFromDelegation: vi.fn().mockResolvedValue(undefined), + updateTaskHistory: vi.fn().mockResolvedValue([]), + persistDelegationMeta: vi.fn().mockResolvedValue(undefined), + readDelegationMeta: vi.fn().mockResolvedValue(null), + } +} + +function createMockTask(provider: ReturnType) { + return { + taskId: "child-1", + parentTaskId: "parent-1", + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + say: vi.fn().mockResolvedValue(undefined), + ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: undefined, images: undefined }), + emitFinalTokenUsageUpdate: vi.fn(), + emit: vi.fn(), + getTokenUsage: vi.fn().mockReturnValue({}), + toolUsage: {}, + clineMessages: [], + providerRef: { deref: () => provider }, + sayAndCreateMissingParamError: vi.fn().mockResolvedValue("missing param"), + } +} + +function createCallbacks(overrides: Record = {}) { + return { + askApproval: vi.fn(), + handleError: vi.fn(), + pushToolResult: vi.fn(), + askFinishSubTaskApproval: vi.fn().mockResolvedValue(true), + toolDescription: () => "attempt_completion tool", + ...overrides, + } +} + +describe("AttemptCompletionTool delegation retry and parent-repair", () => { + let tool: AttemptCompletionTool + + beforeEach(() => { + vi.useFakeTimers() + tool = new AttemptCompletionTool() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it("should succeed on retry when first delegation attempt fails", async () => { + const provider = createMockProvider() + const task = createMockTask(provider) + const callbacks = createCallbacks() + + // First call fails, second succeeds + provider.reopenParentFromDelegation + .mockRejectedValueOnce(new Error("transient race")) + .mockResolvedValueOnce(undefined) + + // Child task status is "active" + provider.getTaskWithId.mockResolvedValue({ + historyItem: childHistoryItem(), + }) + + const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any) + + // Advance past the 500ms retry delay + await vi.advanceTimersByTimeAsync(600) + await executePromise + + // Verify retry happened (2 calls to reopenParentFromDelegation) + expect(provider.reopenParentFromDelegation).toHaveBeenCalledTimes(2) + + // Verify delegation succeeded — pushToolResult called with empty string + expect(callbacks.pushToolResult).toHaveBeenCalledWith("") + + // Verify no parent repair was attempted + expect(provider.updateTaskHistory).not.toHaveBeenCalled() + }) + + it("should repair parent to active when both delegation attempts fail", async () => { + const provider = createMockProvider() + const task = createMockTask(provider) + const callbacks = createCallbacks() + + // Both delegation calls fail + provider.reopenParentFromDelegation + .mockRejectedValueOnce(new Error("first failure")) + .mockRejectedValueOnce(new Error("second failure")) + + // First call: child status lookup; second call: parent lookup for repair + provider.getTaskWithId + .mockResolvedValueOnce({ historyItem: childHistoryItem() }) + .mockResolvedValueOnce({ historyItem: parentHistoryItem() }) + + const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any) + + await vi.advanceTimersByTimeAsync(600) + await executePromise + + // Both attempts were made + expect(provider.reopenParentFromDelegation).toHaveBeenCalledTimes(2) + + // Parent was repaired: updateTaskHistory called with status: "active" + expect(provider.updateTaskHistory).toHaveBeenCalledWith( + expect.objectContaining({ + id: "parent-1", + status: "active", + awaitingChildId: undefined, + childIds: ["child-1"], + }), + ) + + // persistDelegationMeta called with status: "active" and null awaitingChildId + expect(provider.persistDelegationMeta).toHaveBeenCalledWith("parent-1", { + status: "active", + awaitingChildId: null, + delegatedToId: "mode-1", + childIds: ["child-1"], + completedByChildId: "old-child-0", + completionResultSummary: "old result summary", + }) + + // Error tool result was pushed + expect(callbacks.pushToolResult).toHaveBeenCalledWith( + expect.stringContaining("Delegation to parent task failed"), + ) + }) + + it("should use read-merge-write disk fallback when parent not in globalState", async () => { + const provider = createMockProvider() + const task = createMockTask(provider) + const callbacks = createCallbacks() + + // Both delegation attempts fail + provider.reopenParentFromDelegation + .mockRejectedValueOnce(new Error("first failure")) + .mockRejectedValueOnce(new Error("second failure")) + + // Child status lookup succeeds, parent lookup fails (not in globalState) + provider.getTaskWithId + .mockResolvedValueOnce({ historyItem: childHistoryItem() }) + .mockRejectedValueOnce(new Error("Task not found")) + + // Existing delegation meta on disk + const existingMeta: DelegationMeta = { + status: "delegated", + delegatedToId: "mode-1", + awaitingChildId: "child-1", + childIds: ["child-1"], + completedByChildId: "prev-child", + completionResultSummary: "previous result", + } + provider.readDelegationMeta.mockResolvedValue(existingMeta) + + const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any) + + await vi.advanceTimersByTimeAsync(600) + await executePromise + + // readDelegationMeta was called for the parent + expect(provider.readDelegationMeta).toHaveBeenCalledWith("parent-1") + + // persistDelegationMeta preserves existing fields via read-merge-write + expect(provider.persistDelegationMeta).toHaveBeenCalledWith("parent-1", { + status: "active", + awaitingChildId: null, + delegatedToId: "mode-1", + childIds: ["child-1"], + completedByChildId: "prev-child", + completionResultSummary: "previous result", + }) + }) + + it("should use defaults when disk fallback has no existing meta", async () => { + const provider = createMockProvider() + const task = createMockTask(provider) + const callbacks = createCallbacks() + + // Both delegation attempts fail + provider.reopenParentFromDelegation + .mockRejectedValueOnce(new Error("first failure")) + .mockRejectedValueOnce(new Error("second failure")) + + // Child status lookup succeeds, parent lookup fails + provider.getTaskWithId + .mockResolvedValueOnce({ historyItem: childHistoryItem() }) + .mockRejectedValueOnce(new Error("Task not found")) + + // No existing delegation meta on disk + provider.readDelegationMeta.mockResolvedValue(null) + + const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any) + + await vi.advanceTimersByTimeAsync(600) + await executePromise + + // persistDelegationMeta uses null defaults when no existing meta + expect(provider.persistDelegationMeta).toHaveBeenCalledWith("parent-1", { + status: "active", + awaitingChildId: null, + delegatedToId: null, + childIds: ["child-1"], + completedByChildId: null, + completionResultSummary: null, + }) + }) + + it("should not attempt delegation when approval is denied", async () => { + const provider = createMockProvider() + const task = createMockTask(provider) + const callbacks = createCallbacks({ + askFinishSubTaskApproval: vi.fn().mockResolvedValue(false), + }) + + // Child task status is "active" + provider.getTaskWithId.mockResolvedValue({ + historyItem: childHistoryItem(), + }) + + await tool.execute({ result: "completed work" }, task as any, callbacks as any) + + // No delegation attempted + expect(provider.reopenParentFromDelegation).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 9aac6296c6c..4bbbdcf0692 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -25,6 +25,15 @@ vi.mock("../../../shared/package", () => ({ }, })) +// Mock TelemetryService +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureTaskCompleted: vi.fn(), + }, + }, +})) + import { attemptCompletionTool, AttemptCompletionCallbacks } from "../AttemptCompletionTool" import { Task } from "../../task/Task" import * as vscode from "vscode" @@ -469,4 +478,120 @@ describe("attemptCompletionTool", () => { }) }) }) + + describe("delegation flow", () => { + let mockProvider: { + getTaskWithId: ReturnType + reopenParentFromDelegation: ReturnType + updateTaskHistory: ReturnType + persistDelegationMeta: ReturnType + readDelegationMeta: ReturnType + } + + let block: AttemptCompletionToolUse + let callbacks: AttemptCompletionCallbacks + + beforeEach(() => { + mockProvider = { + getTaskWithId: vi.fn(), + reopenParentFromDelegation: vi.fn(), + updateTaskHistory: vi.fn(), + persistDelegationMeta: vi.fn(), + readDelegationMeta: vi.fn().mockResolvedValue(null), + } + + Object.defineProperty(mockTask, "parentTaskId", { value: "parent-123", writable: true, configurable: true }) + Object.defineProperty(mockTask, "providerRef", { + value: new WeakRef(mockProvider), + writable: true, + configurable: true, + }) + + // Default: child task is active (triggers delegation path) + mockProvider.getTaskWithId.mockResolvedValue({ + historyItem: { status: "active", childIds: [] }, + }) + + mockAskFinishSubTaskApproval.mockResolvedValue(true) + + block = { + type: "tool_use", + name: "attempt_completion", + params: { result: "Task completed successfully" }, + nativeArgs: { result: "Task completed successfully" }, + partial: false, + } + + callbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + }) + + it("repairs parent to active when both delegation attempts fail", async () => { + mockProvider.reopenParentFromDelegation.mockRejectedValue(new Error("persistent error")) + + // First call: child task check in execute(); second call: parent repair in delegateToParent() + mockProvider.getTaskWithId + .mockResolvedValueOnce({ historyItem: { status: "active", childIds: [] } }) + .mockResolvedValueOnce({ + historyItem: { status: "delegated", childIds: [], delegatedToId: "child-task" }, + }) + + mockProvider.updateTaskHistory.mockResolvedValue([]) + mockProvider.persistDelegationMeta.mockResolvedValue(undefined) + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + expect(mockProvider.updateTaskHistory).toHaveBeenCalledWith( + expect.objectContaining({ status: "active", awaitingChildId: undefined }), + ) + expect(mockProvider.persistDelegationMeta).toHaveBeenCalledWith( + "parent-123", + expect.objectContaining({ status: "active", awaitingChildId: null }), + ) + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegation to parent task failed")) + }) + + it("handles repair failure gracefully when both delegation and repair fail", async () => { + mockProvider.reopenParentFromDelegation.mockRejectedValue(new Error("persistent error")) + + // First call: child task check; second call: parent repair (will succeed but updateTaskHistory throws) + mockProvider.getTaskWithId + .mockResolvedValueOnce({ historyItem: { status: "active", childIds: [] } }) + .mockResolvedValueOnce({ historyItem: { status: "delegated", childIds: [] } }) + + mockProvider.updateTaskHistory.mockRejectedValue(new Error("repair failed")) + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) // should NOT throw — repair error is caught internally + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegation to parent task failed")) + }) + + it("repairs parent via disk fallback when getTaskWithId throws Task not found", async () => { + mockProvider.reopenParentFromDelegation.mockRejectedValue(new Error("persistent error")) + + // First call: child task check in execute(); second call: parent repair throws "Task not found" + mockProvider.getTaskWithId + .mockResolvedValueOnce({ historyItem: { status: "active", childIds: [] } }) + .mockRejectedValueOnce(new Error("Task not found")) + + mockProvider.persistDelegationMeta.mockResolvedValue(undefined) + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + expect(mockProvider.persistDelegationMeta).toHaveBeenCalledWith("parent-123", { + status: "active", + awaitingChildId: null, + delegatedToId: null, + childIds: ["task_1"], + completedByChildId: null, + completionResultSummary: null, + }) + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegation to parent task failed")) + }) + }) }) diff --git a/src/core/tools/__tests__/newTaskTool.spec.ts b/src/core/tools/__tests__/newTaskTool.spec.ts index fc383c13eef..735a9139f3a 100644 --- a/src/core/tools/__tests__/newTaskTool.spec.ts +++ b/src/core/tools/__tests__/newTaskTool.spec.ts @@ -175,7 +175,7 @@ describe("newTaskTool", () => { ) // Verify side effects - expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegated to child task")) + expect(mockPushToolResult).not.toHaveBeenCalled() }) it("should not un-escape single escaped \@", async () => { @@ -280,7 +280,7 @@ describe("newTaskTool", () => { expect(mockStartSubtask).toHaveBeenCalledWith("Test message", [], "code") // Should complete successfully - expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegated to child task")) + expect(mockPushToolResult).not.toHaveBeenCalled() }) it("should work with todos parameter when provided", async () => { @@ -311,7 +311,7 @@ describe("newTaskTool", () => { "code", ) - expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegated to child task")) + expect(mockPushToolResult).not.toHaveBeenCalled() }) it("should error when mode parameter is missing", async () => { @@ -423,7 +423,7 @@ describe("newTaskTool", () => { expect(mockStartSubtask).toHaveBeenCalledWith("Test message", [], "code") // Should complete successfully - expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegated to child task")) + expect(mockPushToolResult).not.toHaveBeenCalled() }) it("should REQUIRE todos when VSCode setting is enabled", async () => { @@ -501,7 +501,7 @@ describe("newTaskTool", () => { ) // Should complete successfully - expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegated to child task")) + expect(mockPushToolResult).not.toHaveBeenCalled() }) it("should work with empty todos string when VSCode setting is enabled", async () => { @@ -536,7 +536,7 @@ describe("newTaskTool", () => { expect(mockStartSubtask).toHaveBeenCalledWith("Test message", [], "code") // Should complete successfully - expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegated to child task")) + expect(mockPushToolResult).not.toHaveBeenCalled() }) it("should check VSCode setting with Package.name configuration key", async () => { @@ -672,6 +672,6 @@ describe("newTaskTool delegation flow", () => { expect(pauseEvents.length).toBe(0) // Assert: tool result reflects delegation - expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegated to child task child-1")) + expect(mockPushToolResult).not.toHaveBeenCalled() }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 36a5612fa8a..c0ffad52aaa 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -110,6 +110,7 @@ import { getToolCallName, getToolResultCallId, } from "../task-persistence" +import { type DelegationMeta, readDelegationMeta, saveDelegationMeta } from "../task-persistence/delegationMeta" import { readTaskMessages } from "../task-persistence/taskMessages" import { getNonce } from "./getNonce" import { getUri } from "./getUri" @@ -160,6 +161,7 @@ export class ClineProvider private taskEventListeners: WeakMap void>> = new WeakMap() private currentWorkspacePath: string | undefined private _disposed = false + private delegationInProgress = false private recentTasksCache?: string[] private taskHistoryWriteLock: Promise = Promise.resolve() @@ -177,6 +179,7 @@ export class ClineProvider private clineMessagesSeq = 0 public isViewLaunched = false + public isTaskCreationInProgress = false public settingsImportedAt?: number public readonly latestAnnouncementId = "feb-2026-v3.47.0-opus-4.6-gpt-5.3-codex" // v3.47.0 Claude Opus 4.6 & GPT-5.3-Codex public readonly providerSettingsManager: ProviderSettingsManager @@ -528,17 +531,63 @@ export class ClineProvider status: "active", awaitingChildId: undefined, }) + const globalStoragePath = this.contextProxy.globalStorageUri.fsPath + await saveDelegationMeta({ + taskId: parentTaskId, + globalStoragePath, + meta: { + status: "active", + awaitingChildId: null, + delegatedToId: parentHistory.delegatedToId, + childIds: parentHistory.childIds, + completedByChildId: parentHistory.completedByChildId, + completionResultSummary: parentHistory.completionResultSummary, + }, + }) 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( - `[ClineProvider#removeClineFromStack] Failed to repair parent metadata for ${parentTaskId} (non-fatal): ${ - err instanceof Error ? err.message : String(err) - }`, - ) + // Disk-only fallback when parent is missing from globalState + if (err instanceof Error && err.message === "Task not found") { + try { + const globalStoragePath = this.contextProxy.globalStorageUri.fsPath + const delegationMeta = await readDelegationMeta({ taskId: parentTaskId, globalStoragePath }) + if ( + delegationMeta?.status === "delegated" && + delegationMeta?.awaitingChildId === childTaskId + ) { + await saveDelegationMeta({ + taskId: parentTaskId, + globalStoragePath, + meta: { + status: "active", + awaitingChildId: null, + delegatedToId: delegationMeta.delegatedToId, + childIds: delegationMeta.childIds, + completedByChildId: delegationMeta.completedByChildId, + completionResultSummary: delegationMeta.completionResultSummary, + }, + }) + this.log( + `[ClineProvider#removeClineFromStack] Repaired parent ${parentTaskId} via disk fallback (not in globalState)`, + ) + } + } catch (diskErr) { + this.log( + `[ClineProvider#removeClineFromStack] Disk fallback repair also failed for ${parentTaskId}: ${ + diskErr instanceof Error ? diskErr.message : String(diskErr) + }`, + ) + } + } else { + this.log( + `[ClineProvider#removeClineFromStack] Failed to repair parent metadata for ${parentTaskId} (non-fatal): ${ + err instanceof Error ? err.message : String(err) + }`, + ) + } } } } @@ -1045,8 +1094,6 @@ export class ClineProvider onCreated: this.taskCreationCallback, startTask: options?.startTask ?? true, enableBridge: BridgeOrchestrator.isEnabled(cloudUserInfo, taskSyncEnabled), - // Preserve the status from the history item to avoid overwriting it when the task saves messages - initialStatus: historyItem.status, }) if (isRehydratingCurrentTask) { @@ -1742,8 +1789,28 @@ export class ClineProvider throw new Error("Task not found") } - const { getTaskDirectoryPath } = await import("../../utils/storage") + // Hoist globalStoragePath so the delegation-meta merge and the file-path + // resolution below share one computation. const globalStoragePath = this.contextProxy.globalStorageUri.fsPath + + // Merge per-task delegation metadata (source of truth for delegation fields). + // Old tasks without a file are unchanged (null → no merge). + try { + const delegationMeta = await readDelegationMeta({ taskId: id, globalStoragePath }) + + if (delegationMeta) { + for (const [key, value] of Object.entries(delegationMeta)) { + ;(historyItem as Record)[key] = value === null ? undefined : value + } + } + } catch (err) { + // Non-fatal: fall back to globalState values + console.warn( + `[getTaskWithId] Failed to read delegation metadata for task ${id} (non-fatal): ${err instanceof Error ? err.message : String(err)}`, + ) + } + + const { getTaskDirectoryPath } = await import("../../utils/storage") const taskDirPath = await getTaskDirectoryPath(globalStoragePath, id) const apiConversationHistoryFilePath = path.join(taskDirPath, GlobalFileNames.apiConversationHistory) const uiMessagesFilePath = path.join(taskDirPath, GlobalFileNames.uiMessages) @@ -1782,6 +1849,11 @@ export class ClineProvider } async showTaskWithId(id: string) { + if (this.delegationInProgress) { + this.log("[showTaskWithId] Skipped: delegation in progress") + vscode.window.showInformationMessage("Task delegation in progress, please wait...") + return + } if (id !== this.getCurrentTask()?.taskId) { // Non-current task. const { historyItem } = await this.getTaskWithId(id) @@ -1825,6 +1897,11 @@ export class ClineProvider // If the task has subtasks (childIds), they will also be deleted recursively async deleteTaskWithId(id: string, cascadeSubtasks: boolean = true) { try { + if (this.delegationInProgress) { + this.log("[deleteTaskWithId] Skipped: delegation in progress") + vscode.window.showInformationMessage("Task delegation in progress, please wait...") + return + } // get the task directory full path and history item const { taskDirPath, historyItem } = await this.getTaskWithId(id) @@ -2609,6 +2686,28 @@ export class ClineProvider }) } + /** + * Convenience wrapper around the standalone saveDelegationMeta function, + * injecting globalStoragePath from this provider's context. + * Exposed so tools (e.g. AttemptCompletionTool) can persist delegation + * metadata through the DelegationProvider interface. + */ + async persistDelegationMeta(taskId: string, meta: DelegationMeta): Promise { + const globalStoragePath = this.contextProxy.globalStorageUri.fsPath + await saveDelegationMeta({ taskId, globalStoragePath, meta }) + } + + /** + * Convenience wrapper around the standalone readDelegationMeta function, + * injecting globalStoragePath from this provider's context. + * Exposed so tools (e.g. AttemptCompletionTool) can read existing delegation + * metadata for read-merge-write patterns through the DelegationProvider interface. + */ + async readDelegationMeta(taskId: string): Promise { + const globalStoragePath = this.contextProxy.globalStorageUri.fsPath + return readDelegationMeta({ taskId, globalStoragePath }) + } + /** * Broadcasts a task history update to the webview. * This sends a lightweight message with just the task history, rather than the full state. @@ -2923,6 +3022,12 @@ export class ClineProvider options: CreateTaskOptions = {}, configuration: RooCodeSettings = {}, ): Promise { + if (this.delegationInProgress && !parentTask) { + this.log("[createTask] Blocked: delegation in progress") + vscode.window.showInformationMessage("Task delegation in progress, please wait...") + throw new Error("Cannot create task while delegation is in progress") + } + if (configuration) { await this.setValues(configuration) @@ -3004,6 +3109,11 @@ export class ClineProvider } public async cancelTask(): Promise { + if (this.delegationInProgress) { + this.log("[cancelTask] Skipped: delegation in progress") + vscode.window.showInformationMessage("Task delegation in progress, please wait...") + return + } const task = this.getCurrentTask() if (!task) { @@ -3242,128 +3352,222 @@ export class ClineProvider }): Promise { const { parentTaskId, message, initialTodos, mode } = params - // Metadata-driven delegation is always enabled - - // 1) Get parent (must be current task) - const parent = this.getCurrentTask() - if (!parent) { - throw new Error("[delegateParentAndOpenChild] No current task") + if (this.delegationInProgress) { + throw new Error("[delegateParentAndOpenChild] Delegation already in progress") } - if (parent.taskId !== parentTaskId) { - throw new Error( - `[delegateParentAndOpenChild] Parent mismatch: expected ${parentTaskId}, current ${parent.taskId}`, - ) - } - // 2) Flush pending tool results to API history BEFORE disposing the parent. - // This is critical: when tools are called before new_task, - // their tool_result blocks are in userMessageContent but not yet saved to API history. - // If we don't flush them, the parent's API conversation will be incomplete and - // cause 400 errors when resumed (missing tool_result for tool_use blocks). - // - // NOTE: We do NOT pass the assistant message here because the assistant message - // is already added to apiConversationHistory by the normal flow in - // recursivelyMakeClineRequests BEFORE tools start executing. We only need to - // flush the pending user message with tool_results. + this.delegationInProgress = true + + const globalStoragePath = this.contextProxy.globalStorageUri.fsPath + try { - const flushSuccess = await parent.flushPendingToolResultsToHistory() + // Metadata-driven delegation is always enabled + + // 1) Get parent (must be current task) + const parent = this.getCurrentTask() + if (!parent) { + throw new Error("[delegateParentAndOpenChild] No current task") + } + if (parent.taskId !== parentTaskId) { + throw new Error( + `[delegateParentAndOpenChild] Parent mismatch: expected ${parentTaskId}, current ${parent.taskId}`, + ) + } + // Capture parent metadata before the parent is removed from the stack (Change A) + const parentMetadata = { task: parent?.metadata?.task, taskNumber: parent?.taskNumber } + + // 2) Flush pending tool results to API history BEFORE disposing the parent. + // This is critical: when tools are called before new_task, + // their tool_result blocks are in userMessageContent but not yet saved to API history. + // If we don't flush them, the parent's API conversation will be incomplete and + // cause 400 errors when resumed (missing tool_result for tool_use blocks). + // + // NOTE: We do NOT pass the assistant message here because the assistant message + // is already added to apiConversationHistory by the normal flow in + // recursivelyMakeClineRequests BEFORE tools start executing. We only need to + // flush the pending user message with tool_results. + try { + const flushSuccess = await parent.flushPendingToolResultsToHistory() - if (!flushSuccess) { - console.warn(`[delegateParentAndOpenChild] Flush failed for parent ${parentTaskId}, retrying...`) - const retrySuccess = await parent.retrySaveApiConversationHistory() + if (!flushSuccess) { + console.warn(`[delegateParentAndOpenChild] Flush failed for parent ${parentTaskId}, retrying...`) + const retrySuccess = await parent.retrySaveApiConversationHistory() - if (!retrySuccess) { - console.error( - `[delegateParentAndOpenChild] CRITICAL: Parent ${parentTaskId} API history not persisted to disk. Child return may produce stale state.`, - ) - vscode.window.showWarningMessage( - "Warning: Parent task state could not be saved. The parent task may lose recent context when resumed.", - ) + if (!retrySuccess) { + console.error( + `[delegateParentAndOpenChild] CRITICAL: Parent ${parentTaskId} API history not persisted to disk. Child return may produce stale state.`, + ) + vscode.window.showWarningMessage( + "Warning: Parent task state could not be saved. The parent task may lose recent context when resumed.", + ) + } } + } catch (error) { + this.log( + `[delegateParentAndOpenChild] Error flushing pending tool results (non-fatal): ${ + error instanceof Error ? error.message : String(error) + }`, + ) } - } catch (error) { - this.log( - `[delegateParentAndOpenChild] Error flushing pending tool results (non-fatal): ${ - error instanceof Error ? error.message : String(error) - }`, - ) - } - // 3) Enforce single-open invariant by closing/disposing the parent first - // This ensures we never have >1 tasks open at any time during delegation. - // Await abort completion to ensure clean disposal and prevent unhandled rejections. - try { - await this.removeClineFromStack({ skipDelegationRepair: true }) - } catch (error) { - this.log( - `[delegateParentAndOpenChild] Error during parent disposal (non-fatal): ${ - error instanceof Error ? error.message : String(error) - }`, - ) - // Non-fatal: proceed with child creation even if parent cleanup had issues - } + // 3) Enforce single-open invariant by closing/disposing the parent first + // This ensures we never have >1 tasks open at any time during delegation. + // Await abort completion to ensure clean disposal and prevent unhandled rejections. + try { + await this.removeClineFromStack({ skipDelegationRepair: true }) + } catch (error) { + this.log( + `[delegateParentAndOpenChild] Error during parent disposal (non-fatal): ${ + error instanceof Error ? error.message : String(error) + }`, + ) + // Non-fatal: proceed with child creation even if parent cleanup had issues + } - // 3) Switch provider mode to child's requested mode BEFORE creating the child task - // This ensures the child's system prompt and configuration are based on the correct mode. - // The mode switch must happen before createTask() because the Task constructor - // initializes its mode from provider.getState() during initializeTaskMode(). - try { - await this.handleModeSwitch(mode as any) - } catch (e) { - this.log( - `[delegateParentAndOpenChild] handleModeSwitch failed for mode '${mode}': ${ - (e as Error)?.message ?? String(e) - }`, - ) - } + // 3) Switch provider mode to child's requested mode BEFORE creating the child task + // This ensures the child's system prompt and configuration are based on the correct mode. + // The mode switch must happen before createTask() because the Task constructor + // initializes its mode from provider.getState() during initializeTaskMode(). + try { + await this.handleModeSwitch(mode as any) + } catch (e) { + this.log( + `[delegateParentAndOpenChild] handleModeSwitch failed for mode '${mode}': ${ + (e as Error)?.message ?? String(e) + }`, + ) + } - // 4) Create child as sole active (parent reference preserved for lineage) - // Pass initialStatus: "active" to ensure the child task's historyItem is created - // with status from the start, avoiding race conditions where the task might - // call attempt_completion before status is persisted separately. - // - // Pass startTask: false to prevent the child from beginning its task loop - // (and writing to globalState via saveClineMessages → updateTaskHistory) - // before we persist the parent's delegation metadata in step 5. - // Without this, the child's fire-and-forget startTask() races with step 5, - // and the last writer to globalState overwrites the other's changes— - // causing the parent's delegation fields to be lost. - const child = await this.createTask(message, undefined, parent as any, { - initialTodos, - initialStatus: "active", - startTask: false, - }) + // 4) Create child as sole active (parent reference preserved for lineage) + // + // Pass startTask: false to prevent the child from beginning its task loop + // (and writing to globalState via saveClineMessages → updateTaskHistory) + // before we persist the parent's delegation metadata in step 5. + // Without this, the child's fire-and-forget startTask() races with step 5, + // and the last writer to globalState overwrites the other's changes— + // causing the parent's delegation fields to be lost. + const child = await this.createTask(message, undefined, parent as any, { + initialTodos, + startTask: false, + }) - // 5) Persist parent delegation metadata BEFORE the child starts writing. - try { - const { historyItem } = await this.getTaskWithId(parentTaskId) - const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId])) - const updatedHistory: typeof historyItem = { - ...historyItem, - status: "delegated", + // 4b) Persist child's initial status in globalState (saveClineMessages no longer writes status) + // Build the history item directly from the child Task object instead of getTaskWithId, + // because the child isn't in globalState yet at this point. + await this.updateTaskHistory( + { + id: child.taskId, + ts: Date.now(), + task: message, + number: child.taskNumber, + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + status: "active", + parentTaskId: parentTaskId, + rootTaskId: child.rootTaskId, + workspace: this.cwd, + } as HistoryItem, + { broadcast: false }, + ) + + // 5) Persist parent delegation metadata BEFORE the child starts writing. + // updateTaskHistory (globalState) is critical — without it, parent won't show as delegated + let parentHistory: HistoryItem + try { + const result = await this.getTaskWithId(parentTaskId) + parentHistory = result.historyItem + } catch (err) { + console.error( + `[delegateParentAndOpenChild] Parent ${parentTaskId} not in globalState, using in-memory fallback: ${ + err instanceof Error ? err.message : String(err) + }`, + ) + parentHistory = { + id: parentTaskId, + ts: Date.now(), + task: parentMetadata.task ?? "", + number: parentMetadata.taskNumber ?? 0, + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + workspace: this.cwd, + } as HistoryItem + } + const childIds = Array.from(new Set([...(parentHistory.childIds ?? []), child.taskId])) + const updatedHistory = { + ...parentHistory, + status: "delegated" as const, delegatedToId: child.taskId, awaitingChildId: child.taskId, childIds, } await this.updateTaskHistory(updatedHistory) - } catch (err) { - this.log( - `[delegateParentAndOpenChild] Failed to persist parent metadata for ${parentTaskId} -> ${child.taskId}: ${ - (err as Error)?.message ?? String(err) - }`, - ) - } - // 6) Start the child task now that parent metadata is safely persisted. - child.start() + // Per-task file backup is non-critical — globalState is the primary source + try { + await saveDelegationMeta({ + taskId: parentTaskId, + globalStoragePath, + meta: { status: "delegated", delegatedToId: child.taskId, awaitingChildId: child.taskId, childIds }, + }) + await saveDelegationMeta({ + taskId: child.taskId, + globalStoragePath, + meta: { status: "active" }, + }) + } catch (err) { + this.log( + `[delegateParentAndOpenChild] Non-critical: Failed to write delegation metadata files for ${parentTaskId} -> ${child.taskId}: ${(err as Error)?.message ?? String(err)}`, + ) + vscode.window.showWarningMessage( + "Delegation metadata could not be saved. Task delegation may be in a degraded state.", + ) + } - // 7) Emit TaskDelegated (provider-level) - try { - this.emit(RooCodeEventName.TaskDelegated, parentTaskId, child.taskId) - } catch { - // non-fatal - } + // 6) Start the child task now that parent metadata is safely persisted. + const startPromise = child.start() + if (startPromise) { + startPromise.catch(async (err) => { + this.log( + `[delegateParentAndOpenChild] child.start() failed for ${child.taskId}: ${ + (err as Error)?.message ?? String(err) + }`, + ) + // Repair parent status back to active + try { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + await this.updateTaskHistory({ ...parentHistory, status: "active", awaitingChildId: undefined }) + await saveDelegationMeta({ + taskId: parentTaskId, + globalStoragePath, + meta: { + status: "active", + awaitingChildId: null, + delegatedToId: parentHistory.delegatedToId, + childIds: parentHistory.childIds, + }, + }) + } catch (repairErr) { + this.log( + `[delegateParentAndOpenChild] Failed to repair parent after child.start() failure: ${ + (repairErr as Error)?.message ?? String(repairErr) + }`, + ) + } + }) + } - return child + // 7) Emit TaskDelegated (provider-level) + try { + this.emit(RooCodeEventName.TaskDelegated, parentTaskId, child.taskId) + } catch { + // non-fatal + } + + return child + } finally { + this.delegationInProgress = false + } } /** @@ -3377,211 +3581,253 @@ export class ClineProvider 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) - - let parentClineMessages: ClineMessage[] = [] - try { - parentClineMessages = await readTaskMessages({ - taskId: parentTaskId, - globalStoragePath, - }) - } catch { - parentClineMessages = [] + if (this.delegationInProgress) { + throw new Error("[reopenParentFromDelegation] Delegation already in progress") } + this.delegationInProgress = true - let parentApiMessages: RooMessage[] = [] try { - parentApiMessages = await readRooMessages({ - taskId: parentTaskId, - globalStoragePath, - }) - } 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 call ID from the last assistant message's new_task tool call - let toolUseId: string | undefined - for (let i = parentApiMessages.length - 1; i >= 0; i--) { - const msg = parentApiMessages[i] - if (isRooAssistantMessage(msg) && Array.isArray(msg.content)) { - for (const block of msg.content) { - const typedBlock = block as unknown as { type: string } - if (isAnyToolCallBlock(typedBlock) && getToolCallName(typedBlock) === "new_task") { - toolUseId = getToolCallId(typedBlock) - break + // 1) Load parent from history and current persisted messages + const { historyItem } = await this.getTaskWithId(parentTaskId) + + let parentClineMessages: ClineMessage[] = [] + try { + parentClineMessages = await readTaskMessages({ + taskId: parentTaskId, + globalStoragePath, + }) + } catch { + parentClineMessages = [] + } + + let parentApiMessages: RooMessage[] = [] + try { + parentApiMessages = await readRooMessages({ + taskId: parentTaskId, + globalStoragePath, + }) + } 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 call ID from the last assistant message's new_task tool call + let toolUseId: string | undefined + for (let i = parentApiMessages.length - 1; i >= 0; i--) { + const msg = parentApiMessages[i] + if (isRooAssistantMessage(msg) && Array.isArray(msg.content)) { + for (const block of msg.content) { + const typedBlock = block as unknown as { type: string } + if (isAnyToolCallBlock(typedBlock) && getToolCallName(typedBlock) === "new_task") { + toolUseId = getToolCallId(typedBlock) + break + } } + if (toolUseId) break } - if (toolUseId) break - } - } - - // Preferred: if the parent history contains a new_task tool call, - // inject a matching tool result for the model message contract. - if (toolUseId) { - // Check if the last message already contains a tool result for this tool call 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" in lastMsg && Array.isArray(lastMsg.content)) { - for (const block of lastMsg.content) { - const typedBlock = block as unknown as { type: string } - if (isAnyToolResultBlock(typedBlock) && getToolResultCallId(typedBlock) === toolUseId) { - const updatedText = `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}` - if (typedBlock.type === "tool-result") { - ;(typedBlock as { output: { type: "text"; value: string } }).output = { - type: "text", - value: updatedText, + } + + // Preferred: if the parent history contains a new_task tool call, + // inject a matching tool result for the model message contract. + if (toolUseId) { + // Check if the last message already contains a tool result for this tool call 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" in lastMsg && Array.isArray(lastMsg.content)) { + for (const block of lastMsg.content) { + const typedBlock = block as unknown as { type: string } + if (isAnyToolResultBlock(typedBlock) && getToolResultCallId(typedBlock) === toolUseId) { + const updatedText = `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}` + if (typedBlock.type === "tool-result") { + ;(typedBlock as { output: { type: "text"; value: string } }).output = { + type: "text", + value: updatedText, + } + } else { + ;(typedBlock as { content: string }).content = updatedText } - } else { - ;(typedBlock as { content: string }).content = updatedText + alreadyHasToolResult = true + break } - alreadyHasToolResult = true - break } } - } - // If no existing tool result found, create a NEW tool message with the tool result - if (!alreadyHasToolResult) { + // If no existing tool result found, create a NEW tool message with the tool result + if (!alreadyHasToolResult) { + parentApiMessages.push({ + role: "tool", + content: [ + { + type: "tool-result" as const, + toolCallId: toolUseId, + toolName: "new_task", + output: { + type: "text" as const, + value: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, + }, + }, + ], + ts, + }) + } + + // Validate the newly injected/updated tool result against the preceding assistant message. + const lastMessage = parentApiMessages[parentApiMessages.length - 1] + if ( + lastMessage && + (isRooToolMessage(lastMessage) || ("role" in lastMessage && lastMessage.role === "user")) + ) { + const validatedMessage = validateAndFixToolResultIds(lastMessage, parentApiMessages.slice(0, -1)) + parentApiMessages[parentApiMessages.length - 1] = validatedMessage as RooMessage + } + } else { + // If there is no corresponding tool call 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: "tool", + role: "user", content: [ { - type: "tool-result" as const, - toolCallId: toolUseId, - toolName: "new_task", - output: { - type: "text" as const, - value: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, - }, + type: "text" as const, + text: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, }, ], ts, }) } - // Validate the newly injected/updated tool result against the preceding assistant message. - const lastMessage = parentApiMessages[parentApiMessages.length - 1] - if ( - lastMessage && - (isRooToolMessage(lastMessage) || ("role" in lastMessage && lastMessage.role === "user")) - ) { - const validatedMessage = validateAndFixToolResultIds(lastMessage, parentApiMessages.slice(0, -1)) - parentApiMessages[parentApiMessages.length - 1] = validatedMessage as RooMessage - } - } else { - // If there is no corresponding tool call in the parent API history, we cannot emit a - // tool result. Fall back to a plain user text note so the parent can still resume. - parentApiMessages.push({ - role: "user", - content: [ - { - type: "text" as const, - text: `Subtask ${childTaskId} completed.\n\nResult:\n${completionResultSummary}`, - }, - ], - ts, + const savedApiMessages = await saveRooMessages({ + messages: parentApiMessages, + taskId: parentTaskId, + globalStoragePath, }) - } - - const savedApiMessages = await saveRooMessages({ - messages: parentApiMessages, - taskId: parentTaskId, - globalStoragePath, - }) - if (savedApiMessages === false) { - this.log(`[reopenParentFromDelegation] Failed to save API messages for parent ${parentTaskId}`) - } - - // 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 (savedApiMessages === false) { + this.log(`[reopenParentFromDelegation] Failed to save API messages for parent ${parentTaskId}`) + } - // 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) - }`, - ) - } + // 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() calls + // updateTaskHistory which would overwrite a "completed" status set earlier + // (updateTaskHistory spreads incoming fields over existing ones). + const current = this.getCurrentTask() + if (current?.taskId === childTaskId) { + await this.removeClineFromStack({ skipDelegationRepair: true }) + } - // 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) + // 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. - // 6) Emit TaskDelegationCompleted (provider-level) - try { - this.emit(RooCodeEventName.TaskDelegationCompleted, parentTaskId, childTaskId, completionResultSummary) - } catch { - // non-fatal - } + try { + const { historyItem: childHistory } = await this.getTaskWithId(childTaskId) + await this.updateTaskHistory({ + ...childHistory, + status: "completed", + }) + await saveDelegationMeta({ + taskId: childTaskId, + globalStoragePath, + meta: { status: "completed" }, + }) + } catch (err) { + this.log( + `[reopenParentFromDelegation] Failed to persist child completed status for ${childTaskId}: ${ + (err as Error)?.message ?? String(err) + }`, + ) + } - // 7) Reopen the parent from history as the sole active task (restores saved mode) - // IMPORTANT: startTask=false to suppress resume-from-history ask scheduling - const parentInstance = await this.createTaskWithHistoryItem(updatedHistory, { startTask: false }) + // 5) Update parent metadata and persist BEFORE emitting completion event + // Re-read parent to avoid stale-read TOCTOU: historyItem was loaded at step 1 + // but many async operations (message injection, child abort) have elapsed since. + const { historyItem: freshParent } = await this.getTaskWithId(parentTaskId) + const childIds = Array.from(new Set([...(freshParent.childIds ?? []), childTaskId])) + const updatedHistory: typeof freshParent = { + ...freshParent, + status: "active", + completedByChildId: childTaskId, + completionResultSummary, + awaitingChildId: undefined, + childIds, + } + await this.updateTaskHistory(updatedHistory) + await saveDelegationMeta({ + taskId: parentTaskId, + globalStoragePath, + meta: { + status: "active", + completedByChildId: childTaskId, + completionResultSummary, + awaitingChildId: null, + childIds, + }, + }) - // 8) Inject restored histories into the in-memory instance before resuming - if (parentInstance) { + // 6) Emit TaskDelegationCompleted (provider-level) try { - await parentInstance.overwriteClineMessages(parentClineMessages) + this.emit(RooCodeEventName.TaskDelegationCompleted, parentTaskId, childTaskId, completionResultSummary) } catch { // non-fatal } + + // 7) Reopen the parent from history as the sole active task (restores saved mode) + // IMPORTANT: startTask=false to suppress resume-from-history ask scheduling + const parentInstance = await this.createTaskWithHistoryItem(updatedHistory, { startTask: false }) + + // 8) Inject restored histories into the in-memory instance before resuming + if (parentInstance) { + try { + await parentInstance.overwriteClineMessages(parentClineMessages) + } catch { + // non-fatal + } + try { + await parentInstance.overwriteApiConversationHistory(parentApiMessages as any) + } catch { + // non-fatal + } + + // Clear delegation guard BEFORE resuming parent so the parent's + // task loop can itself initiate a new delegation (new_task) without + // hitting the "delegation already in progress" guard. This early + // reset is safe because all metadata is persisted and the parent + // instance is fully reconstructed at this point. + // + // The `finally` block at the bottom of this method also sets + // `delegationInProgress = false` as a safety net for error paths — + // if an exception is thrown before reaching this line, the finally + // block ensures the guard is always released. On the happy path + // the finally reset is a harmless no-op. + this.delegationInProgress = false + + // 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 + } finally { + this.delegationInProgress = false } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 4fa79da038f..c4c5b96c161 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3686,6 +3686,9 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { vi.spyOn(fsUtils, "fileExistsAtPath").mockResolvedValue(true) const fsp = await import("fs/promises") + // First readFile call is consumed by readDelegationMeta (delegation_metadata.json) + vi.mocked(fsp.readFile).mockResolvedValueOnce("null" as never) + // Second readFile call is consumed by readRooMessages (api_conversation_history.json) vi.mocked(fsp.readFile).mockResolvedValueOnce( JSON.stringify({ version: 2, diff --git a/src/core/webview/__tests__/removeClineFromStack-delegation.spec.ts b/src/core/webview/__tests__/removeClineFromStack-delegation.spec.ts new file mode 100644 index 00000000000..edf97eb0fd9 --- /dev/null +++ b/src/core/webview/__tests__/removeClineFromStack-delegation.spec.ts @@ -0,0 +1,210 @@ +import { beforeEach, describe, expect, it, vi } from "vitest" +import * as vscode from "vscode" + +import { ClineProvider } from "../ClineProvider" +import { Task } from "../../task/Task" +import { ContextProxy } from "../../config/ContextProxy" +import { readDelegationMeta, saveDelegationMeta } from "../../task-persistence/delegationMeta" + +// Mock dependencies +vi.mock("vscode", () => { + const mockDisposable = { dispose: vi.fn() } + return { + workspace: { + getConfiguration: vi.fn(() => ({ + get: vi.fn().mockReturnValue([]), + update: vi.fn().mockResolvedValue(undefined), + })), + workspaceFolders: [], + onDidChangeConfiguration: vi.fn(() => mockDisposable), + }, + env: { + uriScheme: "vscode", + language: "en", + }, + EventEmitter: vi.fn().mockImplementation(() => ({ + event: vi.fn(), + fire: vi.fn(), + })), + Disposable: { + from: vi.fn(), + }, + window: { + showErrorMessage: vi.fn(), + createTextEditorDecorationType: vi.fn().mockReturnValue({ + dispose: vi.fn(), + }), + onDidChangeActiveTextEditor: vi.fn(() => mockDisposable), + }, + Uri: { + file: vi.fn().mockReturnValue({ toString: () => "file://test" }), + }, + } +}) + +vi.mock("../../task/Task") +vi.mock("../../config/ContextProxy") +vi.mock("../../../services/mcp/McpServerManager", () => ({ + McpServerManager: { + getInstance: vi.fn().mockResolvedValue({ + registerClient: vi.fn(), + }), + unregisterProvider: vi.fn(), + }, +})) +vi.mock("../../../services/marketplace") +vi.mock("../../../integrations/workspace/WorkspaceTracker") +vi.mock("../../config/ProviderSettingsManager") +vi.mock("../../config/CustomModesManager") +vi.mock("../../../utils/path", () => ({ + getWorkspacePath: vi.fn().mockReturnValue("/test/workspace"), +})) + +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + setProvider: vi.fn(), + captureTaskCreated: vi.fn(), + }, + }, +})) + +vi.mock("@roo-code/cloud", () => ({ + CloudService: { + hasInstance: vi.fn().mockReturnValue(false), + instance: { + isAuthenticated: vi.fn().mockReturnValue(false), + }, + }, + BridgeOrchestrator: { + isEnabled: vi.fn().mockReturnValue(false), + }, + getRooCodeApiUrl: vi.fn().mockReturnValue("https://api.roo-code.com"), +})) + +vi.mock("../../../shared/embeddingModels", () => ({ + EMBEDDING_MODEL_PROFILES: [], +})) + +vi.mock("../../task-persistence/delegationMeta", () => ({ + readDelegationMeta: vi.fn(), + saveDelegationMeta: vi.fn(), +})) + +describe("removeClineFromStack — disk fallback delegation repair", () => { + let provider: ClineProvider + let mockContext: any + let mockOutputChannel: any + + const parentTaskId = "parent-task-100" + const childTaskId = "child-task-200" + + beforeEach(() => { + vi.clearAllMocks() + + mockContext = { + globalState: { + get: vi.fn().mockReturnValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + keys: vi.fn().mockReturnValue([]), + }, + globalStorageUri: { fsPath: "/test/storage" }, + secrets: { + get: vi.fn().mockResolvedValue(undefined), + store: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(undefined), + }, + workspaceState: { + get: vi.fn().mockReturnValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + keys: vi.fn().mockReturnValue([]), + }, + extensionUri: { fsPath: "/test/extension" }, + } + + mockOutputChannel = { + appendLine: vi.fn(), + dispose: vi.fn(), + } + + const mockContextProxy = { + getValues: vi.fn().mockReturnValue({}), + getValue: vi.fn().mockReturnValue(undefined), + setValue: vi.fn().mockResolvedValue(undefined), + getProviderSettings: vi.fn().mockReturnValue({ apiProvider: "anthropic" }), + extensionUri: mockContext.extensionUri, + globalStorageUri: mockContext.globalStorageUri, + } + + provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", mockContextProxy as any) + + provider.postStateToWebview = vi.fn().mockResolvedValue(undefined) + provider.postStateToWebviewWithoutTaskHistory = vi.fn().mockResolvedValue(undefined) + }) + + it("repairs parent via disk fallback when parent not in globalState", async () => { + // Mock getTaskWithId to throw "Task not found" (parent missing from globalState) + provider.getTaskWithId = vi.fn().mockRejectedValue(new Error("Task not found")) + + // Mock disk read to return delegated metadata matching the child + vi.mocked(readDelegationMeta).mockResolvedValue({ + status: "delegated", + awaitingChildId: childTaskId, + delegatedToId: null, + childIds: [childTaskId], + }) + + vi.mocked(saveDelegationMeta).mockResolvedValue(undefined) + + // Create a mock task that has parentTaskId + const mockChild: any = { + taskId: childTaskId, + instanceId: "inst-1", + parentTaskId, + emit: vi.fn(), + abortTask: vi.fn().mockResolvedValue(undefined), + } + + // Place the child task on the stack + ;(provider as any).clineStack = [mockChild] + + await provider.removeClineFromStack() + + // Verify saveDelegationMeta was called with repaired status + expect(saveDelegationMeta).toHaveBeenCalledWith({ + taskId: parentTaskId, + globalStoragePath: "/test/storage", + meta: { + status: "active", + awaitingChildId: null, + delegatedToId: null, + childIds: [childTaskId], + }, + }) + }) + + it("handles graceful failure when both globalState and disk fallback fail", async () => { + // Mock getTaskWithId to throw "Task not found" + provider.getTaskWithId = vi.fn().mockRejectedValue(new Error("Task not found")) + + // Mock disk read to also throw + vi.mocked(readDelegationMeta).mockRejectedValue(new Error("ENOENT: file not found")) + + // Create a mock task with parentTaskId + const mockChild: any = { + taskId: childTaskId, + instanceId: "inst-2", + parentTaskId, + emit: vi.fn(), + abortTask: vi.fn().mockResolvedValue(undefined), + } + + ;(provider as any).clineStack = [mockChild] + + // Should NOT throw — failure is non-fatal + await expect(provider.removeClineFromStack()).resolves.toBeUndefined() + + // saveDelegationMeta should NOT have been called since readDelegationMeta failed + expect(saveDelegationMeta).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 2e8b4add12d..041cc729835 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -545,10 +545,14 @@ export const webviewMessageHandler = async ( provider.isViewLaunched = true break case "newTask": - // Initializing new instance of Cline will make sure that any - // agentically running promises in old instance don't affect our new - // task. This essentially creates a fresh slate for the new task. + if (provider.isTaskCreationInProgress) { + break + } + provider.isTaskCreationInProgress = true try { + // Initializing new instance of Cline will make sure that any + // agentically running promises in old instance don't affect our new + // task. This essentially creates a fresh slate for the new task. const resolved = await resolveIncomingImages({ text: message.text, images: message.images }) await provider.createTask(resolved.text, resolved.images) // Task created successfully - notify the UI to reset @@ -560,6 +564,8 @@ export const webviewMessageHandler = async ( vscode.window.showErrorMessage( `Failed to create task: ${error instanceof Error ? error.message : String(error)}`, ) + } finally { + provider.isTaskCreationInProgress = false } break case "customInstructions": diff --git a/src/shared/globalFileNames.ts b/src/shared/globalFileNames.ts index 98b48485f06..75ffc18fdeb 100644 --- a/src/shared/globalFileNames.ts +++ b/src/shared/globalFileNames.ts @@ -4,4 +4,5 @@ export const GlobalFileNames = { mcpSettings: "mcp_settings.json", customModes: "custom_modes.yaml", taskMetadata: "task_metadata.json", + delegationMetadata: "delegation_metadata.json", } From 9a0ec34e39f7d7928ab74adfc69e09572266b31a Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Thu, 12 Feb 2026 00:52:02 -0700 Subject: [PATCH 2/3] fix: write all 6 delegation fields in every saveDelegationMeta call site --- src/core/webview/ClineProvider.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index c0ffad52aaa..c64f99b9838 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3508,7 +3508,14 @@ export class ClineProvider await saveDelegationMeta({ taskId: parentTaskId, globalStoragePath, - meta: { status: "delegated", delegatedToId: child.taskId, awaitingChildId: child.taskId, childIds }, + meta: { + status: "delegated", + delegatedToId: child.taskId, + awaitingChildId: child.taskId, + childIds, + completedByChildId: parentHistory.completedByChildId ?? null, + completionResultSummary: parentHistory.completionResultSummary ?? null, + }, }) await saveDelegationMeta({ taskId: child.taskId, @@ -3545,6 +3552,8 @@ export class ClineProvider awaitingChildId: null, delegatedToId: parentHistory.delegatedToId, childIds: parentHistory.childIds, + completedByChildId: parentHistory.completedByChildId ?? null, + completionResultSummary: parentHistory.completionResultSummary ?? null, }, }) } catch (repairErr) { @@ -3776,6 +3785,7 @@ export class ClineProvider completionResultSummary, awaitingChildId: null, childIds, + delegatedToId: freshParent.delegatedToId ?? null, }, }) From 83e6e05bcc9ebde8afc525e8dd6b94baef902ef6 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Thu, 12 Feb 2026 00:57:23 -0700 Subject: [PATCH 3/3] fix: align delegation tests with single-attempt implementation (no retry) --- .../attemptCompletionDelegation.spec.ts | 77 ++++--------------- 1 file changed, 13 insertions(+), 64 deletions(-) diff --git a/src/core/tools/__tests__/attemptCompletionDelegation.spec.ts b/src/core/tools/__tests__/attemptCompletionDelegation.spec.ts index 3d0d9ddf0e2..d3577af6d86 100644 --- a/src/core/tools/__tests__/attemptCompletionDelegation.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionDelegation.spec.ts @@ -120,71 +120,30 @@ function createCallbacks(overrides: Record = {}) { } } -describe("AttemptCompletionTool delegation retry and parent-repair", () => { +describe("AttemptCompletionTool delegation and parent-repair", () => { let tool: AttemptCompletionTool beforeEach(() => { - vi.useFakeTimers() tool = new AttemptCompletionTool() }) - afterEach(() => { - vi.useRealTimers() - }) - - it("should succeed on retry when first delegation attempt fails", async () => { - const provider = createMockProvider() - const task = createMockTask(provider) - const callbacks = createCallbacks() - - // First call fails, second succeeds - provider.reopenParentFromDelegation - .mockRejectedValueOnce(new Error("transient race")) - .mockResolvedValueOnce(undefined) - - // Child task status is "active" - provider.getTaskWithId.mockResolvedValue({ - historyItem: childHistoryItem(), - }) - - const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any) - - // Advance past the 500ms retry delay - await vi.advanceTimersByTimeAsync(600) - await executePromise - - // Verify retry happened (2 calls to reopenParentFromDelegation) - expect(provider.reopenParentFromDelegation).toHaveBeenCalledTimes(2) - - // Verify delegation succeeded — pushToolResult called with empty string - expect(callbacks.pushToolResult).toHaveBeenCalledWith("") - - // Verify no parent repair was attempted - expect(provider.updateTaskHistory).not.toHaveBeenCalled() - }) - - it("should repair parent to active when both delegation attempts fail", async () => { + it("should repair parent to active when delegation attempt fails", async () => { const provider = createMockProvider() const task = createMockTask(provider) const callbacks = createCallbacks() - // Both delegation calls fail - provider.reopenParentFromDelegation - .mockRejectedValueOnce(new Error("first failure")) - .mockRejectedValueOnce(new Error("second failure")) + // Delegation call fails + provider.reopenParentFromDelegation.mockRejectedValueOnce(new Error("delegation failure")) // First call: child status lookup; second call: parent lookup for repair provider.getTaskWithId .mockResolvedValueOnce({ historyItem: childHistoryItem() }) .mockResolvedValueOnce({ historyItem: parentHistoryItem() }) - const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any) - - await vi.advanceTimersByTimeAsync(600) - await executePromise + await tool.execute({ result: "completed work" }, task as any, callbacks as any) - // Both attempts were made - expect(provider.reopenParentFromDelegation).toHaveBeenCalledTimes(2) + // Single attempt was made (no retry) + expect(provider.reopenParentFromDelegation).toHaveBeenCalledTimes(1) // Parent was repaired: updateTaskHistory called with status: "active" expect(provider.updateTaskHistory).toHaveBeenCalledWith( @@ -217,10 +176,8 @@ describe("AttemptCompletionTool delegation retry and parent-repair", () => { const task = createMockTask(provider) const callbacks = createCallbacks() - // Both delegation attempts fail - provider.reopenParentFromDelegation - .mockRejectedValueOnce(new Error("first failure")) - .mockRejectedValueOnce(new Error("second failure")) + // Delegation attempt fails + provider.reopenParentFromDelegation.mockRejectedValueOnce(new Error("delegation failure")) // Child status lookup succeeds, parent lookup fails (not in globalState) provider.getTaskWithId @@ -238,10 +195,7 @@ describe("AttemptCompletionTool delegation retry and parent-repair", () => { } provider.readDelegationMeta.mockResolvedValue(existingMeta) - const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any) - - await vi.advanceTimersByTimeAsync(600) - await executePromise + await tool.execute({ result: "completed work" }, task as any, callbacks as any) // readDelegationMeta was called for the parent expect(provider.readDelegationMeta).toHaveBeenCalledWith("parent-1") @@ -262,10 +216,8 @@ describe("AttemptCompletionTool delegation retry and parent-repair", () => { const task = createMockTask(provider) const callbacks = createCallbacks() - // Both delegation attempts fail - provider.reopenParentFromDelegation - .mockRejectedValueOnce(new Error("first failure")) - .mockRejectedValueOnce(new Error("second failure")) + // Delegation attempt fails + provider.reopenParentFromDelegation.mockRejectedValueOnce(new Error("delegation failure")) // Child status lookup succeeds, parent lookup fails provider.getTaskWithId @@ -275,10 +227,7 @@ describe("AttemptCompletionTool delegation retry and parent-repair", () => { // No existing delegation meta on disk provider.readDelegationMeta.mockResolvedValue(null) - const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any) - - await vi.advanceTimersByTimeAsync(600) - await executePromise + await tool.execute({ result: "completed work" }, task as any, callbacks as any) // persistDelegationMeta uses null defaults when no existing meta expect(provider.persistDelegationMeta).toHaveBeenCalledWith("parent-1", {