diff --git a/packages/types/src/task.ts b/packages/types/src/task.ts index 55b442cca0..f45ab98d8a 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 fae79bc291..287b926a04 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 4b04fb5bbb..4f624fed5a 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 a72f580d6f..adc7f1e659 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 6d7d0c366c..98b4e37db3 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 0000000000..5f46c87f87 --- /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 0000000000..8a5dfac7d8 --- /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 65e0cc1dc8..a26fb30de5 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 4b77126971..bd4b6023c4 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 71ddc54f10..814b0e5f17 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 a406a15c8b..17bf7369e5 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 f36d8e1e37..cd7d798686 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 0000000000..d3577af6d8 --- /dev/null +++ b/src/core/tools/__tests__/attemptCompletionDelegation.spec.ts @@ -0,0 +1,260 @@ +// 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 and parent-repair", () => { + let tool: AttemptCompletionTool + + beforeEach(() => { + tool = new AttemptCompletionTool() + }) + + it("should repair parent to active when delegation attempt fails", async () => { + const provider = createMockProvider() + const task = createMockTask(provider) + const callbacks = createCallbacks() + + // 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() }) + + await tool.execute({ result: "completed work" }, task as any, callbacks as any) + + // Single attempt was made (no retry) + expect(provider.reopenParentFromDelegation).toHaveBeenCalledTimes(1) + + // 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() + + // Delegation attempt fails + provider.reopenParentFromDelegation.mockRejectedValueOnce(new Error("delegation 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) + + await tool.execute({ result: "completed work" }, task as any, callbacks as any) + + // 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() + + // Delegation attempt fails + provider.reopenParentFromDelegation.mockRejectedValueOnce(new Error("delegation 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) + + 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", { + 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 9aac6296c6..4bbbdcf069 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 fc383c13ee..735a9139f3 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 36a5612fa8..c64f99b983 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,231 @@ 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, + completedByChildId: parentHistory.completedByChildId ?? null, + completionResultSummary: parentHistory.completionResultSummary ?? null, + }, + }) + 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, + completedByChildId: parentHistory.completedByChildId ?? null, + completionResultSummary: parentHistory.completionResultSummary ?? null, + }, + }) + } 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 +3590,254 @@ 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, + delegatedToId: freshParent.delegatedToId ?? null, + }, + }) - // 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 4fa79da038..c4c5b96c16 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 0000000000..edf97eb0fd --- /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 2e8b4add12..041cc72983 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 98b48485f0..75ffc18fde 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", }