diff --git a/apps/cli/src/ui/components/autocomplete/triggers/HistoryTrigger.tsx b/apps/cli/src/ui/components/autocomplete/triggers/HistoryTrigger.tsx index 443fdfa979..925e142d59 100644 --- a/apps/cli/src/ui/components/autocomplete/triggers/HistoryTrigger.tsx +++ b/apps/cli/src/ui/components/autocomplete/triggers/HistoryTrigger.tsx @@ -1,6 +1,8 @@ import { Box, Text } from "ink" import fuzzysort from "fuzzysort" +import type { TaskHistoryStatus } from "@roo-code/types" + import type { AutocompleteTrigger, AutocompleteItem, TriggerDetectionResult } from "../types.js" /** @@ -21,7 +23,7 @@ export interface HistoryResult extends AutocompleteItem { /** Mode the task was run in */ mode?: string /** Task status */ - status?: "active" | "completed" | "delegated" + status?: TaskHistoryStatus } /** @@ -133,8 +135,22 @@ export function createHistoryTrigger(config: HistoryTriggerConfig): Autocomplete renderItem: (item: HistoryResult, isSelected: boolean) => { // Status indicator - const statusIcon = item.status === "completed" ? "✓" : item.status === "active" ? "●" : "○" - const statusColor = item.status === "completed" ? "green" : item.status === "active" ? "yellow" : "gray" + const statusIcon = + item.status === "completed" + ? "✓" + : item.status === "active" + ? "●" + : item.status === "interrupted" + ? "⏸" + : "○" + const statusColor = + item.status === "completed" + ? "green" + : item.status === "active" + ? "yellow" + : item.status === "interrupted" + ? "cyan" + : "gray" // Mode indicator (if available) const modeText = item.mode ? ` [${item.mode}]` : "" @@ -178,7 +194,7 @@ export function toHistoryResult(item: { totalCost?: number workspace?: string mode?: string - status?: "active" | "completed" | "delegated" + status?: TaskHistoryStatus }): HistoryResult { return { key: item.id, // Use task ID as the unique key diff --git a/apps/cli/src/ui/types.ts b/apps/cli/src/ui/types.ts index 3c45377c67..b7e0a4f121 100644 --- a/apps/cli/src/ui/types.ts +++ b/apps/cli/src/ui/types.ts @@ -1,4 +1,4 @@ -import type { ClineAsk, ClineSay, TodoItem } from "@roo-code/types" +import type { ClineAsk, ClineSay, TaskHistoryStatus, TodoItem } from "@roo-code/types" export type MessageRole = "system" | "user" | "assistant" | "tool" | "thinking" @@ -109,7 +109,7 @@ export interface TaskHistoryItem { totalCost?: number workspace?: string mode?: string - status?: "active" | "completed" | "delegated" + status?: TaskHistoryStatus tokensIn?: number tokensOut?: number } diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index d8f6d23299..84b96d439b 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -150,7 +150,7 @@ suite("Roo Code Subtasks", function () { } }) - // Race mitigation: skipDelegationRepair prevents removeClineFromStack from + // Race mitigation: skipChildInterruptMarking prevents removeClineFromStack from // auto-resuming the parent when the child is cancelled (Race 2). test("parent stays paused after subtask cancellation", async () => { const api = globalThis.api @@ -219,9 +219,10 @@ suite("Roo Code Subtasks", function () { } }) - // Race mitigation: runDelegationTransition lock + cancelledDelegationChildIds guard - // ensures cancelTask() wins over a concurrent reopenParentFromDelegation() (Race 3). - test("cancelled child completes in-place and does not reopen parent", async () => { + // Issue #560: interrupted child resumes and reports back to parent. + // cancelTask() marks the child as "interrupted" but preserves the parent-child link, + // so when the child resumes and calls attempt_completion, it delegates back to the parent. + test("interrupted child resumes and reports back to parent", async () => { const api = globalThis.api const asks: Record = {} const messages: Record = {} @@ -237,24 +238,10 @@ suite("Roo Code Subtasks", function () { } } - const findCompletionText = (taskId: string) => - messages[taskId] - ?.filter( - (message) => - message.type === "say" && (message.say === "completion_result" || message.say === "text"), - ) - .map((message) => message.text?.trim()) - .find((text): text is string => !!text) - - const findErrorText = (taskId: string) => - messages[taskId] - ?.filter((message) => message.type === "say" && message.say === "error") - .map((message) => message.text?.trim()) - .find((text): text is string => !!text) - api.on(RooCodeEventName.Message, messageHandler) try { + // 1) Start parent, wait for child to spawn const parentTaskId = await api.startNewTask({ configuration: { mode: "ask", @@ -277,57 +264,127 @@ suite("Roo Code Subtasks", function () { return false }) + // 2) Wait for child to reach a stable point (followup ask) await waitFor( () => asks[spawnedTaskId!]?.some(({ type, ask }) => type === "ask" && ask === "followup") ?? false, ) - const cancelledChildTaskId = spawnedTaskId! + // 3) Cancel the child — it becomes "interrupted", parent stays "delegated" + const interruptedChildTaskId = spawnedTaskId! await api.cancelCurrentTask() - await waitFor(() => api.getCurrentTaskStack().at(-1) === cancelledChildTaskId) + // 4) Wait for the child to show resume_task ask + await waitFor(() => api.getCurrentTaskStack().at(-1) === interruptedChildTaskId) await waitFor( () => - asks[cancelledChildTaskId]?.some(({ type, ask }) => type === "ask" && ask === "resume_task") ?? + asks[interruptedChildTaskId]?.some(({ type, ask }) => type === "ask" && ask === "resume_task") ?? false, ) - const resumedChildTaskId = await waitUntilCompleted({ - api, - start: async () => { - await api.sendMessage(SUBTASK_CHILD_FOLLOWUP_ANSWER) - return cancelledChildTaskId + // 5) Resume the child by answering — it should complete and delegate back to parent + await api.sendMessage(SUBTASK_CHILD_FOLLOWUP_ANSWER) + + // 6) Wait for the parent to complete (child reports back, parent resumes and finishes) + await waitFor( + () => + messages[parentTaskId]?.some( + ({ type, say, text }) => + type === "say" && say === "completion_result" && text === "Parent task resumed", + ) ?? false, + ) + + // 7) Drain the task stack + while (api.getCurrentTaskStack().length > 0) { + await api.clearCurrentTask() + } + } finally { + api.off(RooCodeEventName.Message, messageHandler) + } + }) + + // Fix for orphaned "delegated" parents: when the user resumes a parent + // whose awaited child was interrupted and cleared, the parent self-heals + // from "delegated" to "active" and can be resumed independently. + test("parent self-heals when resumed after interrupted child is cleared", async () => { + const api = globalThis.api + const asks: Record = {} + const says: Record = {} + + const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { + if (message.type === "ask") { + asks[taskId] = asks[taskId] || [] + asks[taskId].push(message) + } + if (message.type === "say" && message.partial === false) { + says[taskId] = says[taskId] || [] + says[taskId].push(message) + } + } + + api.on(RooCodeEventName.Message, messageHandler) + + try { + // 1) Start parent, wait for child to spawn + const parentTaskId = await api.startNewTask({ + configuration: { + mode: "ask", + alwaysAllowModeSwitch: true, + alwaysAllowSubtasks: true, + autoApprovalEnabled: true, + enableCheckpoints: false, }, + text: SUBTASK_PARENT_PROMPT, }) - assert.strictEqual( - resumedChildTaskId, - cancelledChildTaskId, - "Cancelled child task should be resumed in place", - ) - assert.strictEqual( - findErrorText(resumedChildTaskId), - undefined, - "Resumed child task should not emit an error", - ) - assert.strictEqual( - findCompletionText(resumedChildTaskId), - "9", - "Resumed child task should complete with `9`", + let spawnedTaskId: string | undefined + await waitFor(() => { + const stack = api.getCurrentTaskStack() + const current = stack[stack.length - 1] + if (current && current !== parentTaskId) { + spawnedTaskId = current + return true + } + return false + }) + + // 2) Wait for child to reach followup, then cancel it + await waitFor( + () => asks[spawnedTaskId!]?.some(({ type, ask }) => type === "ask" && ask === "followup") ?? false, ) - assert.strictEqual( - api.getCurrentTaskStack().at(-1), - cancelledChildTaskId, - "Cancelled child task should remain the active completed task", + await api.cancelCurrentTask() + + // 3) Wait for the child to show resume_task ask, then clear it + // (simulating user navigating away without resuming the child) + await waitFor(() => api.getCurrentTaskStack().at(-1) === spawnedTaskId) + await waitFor( + () => asks[spawnedTaskId!]?.some(({ type, ask }) => type === "ask" && ask === "resume_task") ?? false, ) + await api.clearCurrentTask() + await waitFor(() => api.getCurrentTaskStack().length === 0) + + // 4) Now resume the parent directly — the self-heal guard in + // createTaskWithHistoryItem detects the child still exists in history + // but the parent should still be resumable (it re-enters the task loop). + assert.ok(await api.isTaskInHistory(parentTaskId), "Parent should still exist in history") + + await api.resumeTask(parentTaskId) + + // 5) Wait for the parent to become the active task on the stack + await waitFor(() => api.getCurrentTaskStack().includes(parentTaskId)) + + // 6) The parent should produce new output (any say message after resume) + // proving it is actively running and not stuck in "delegated" state. + await waitFor(() => (says[parentTaskId]?.length ?? 0) > 0 || (asks[parentTaskId]?.length ?? 0) > 0) + assert.ok( - messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") === - undefined, - "Parent task should not have resumed after the cancelled child completed", + api.getCurrentTaskStack().includes(parentTaskId), + "Parent should be active on the stack after self-healing", ) - - await api.clearCurrentTask() } finally { api.off(RooCodeEventName.Message, messageHandler) + while (api.getCurrentTaskStack().length > 0) { + await api.clearCurrentTask() + } } }) diff --git a/packages/core/src/task-history/index.ts b/packages/core/src/task-history/index.ts index 384bdfc254..d4e47949f1 100644 --- a/packages/core/src/task-history/index.ts +++ b/packages/core/src/task-history/index.ts @@ -1,7 +1,7 @@ import * as fs from "fs/promises" import * as path from "path" -import type { HistoryItem } from "@roo-code/types" +import { TASK_STATUSES, type HistoryItem } from "@roo-code/types" const HISTORY_ITEM_FILENAME = "history_item.json" const HISTORY_INDEX_FILENAME = "_index.json" @@ -41,7 +41,9 @@ function extractSessionEntry(value: unknown): TaskSessionEntry | undefined { ts, workspace: typeof workspace === "string" ? workspace : undefined, mode: typeof mode === "string" ? mode : undefined, - status: status === "active" || status === "completed" || status === "delegated" ? status : undefined, + status: (TASK_STATUSES as readonly string[]).includes(status as string) + ? (status as HistoryItem["status"]) + : undefined, } } diff --git a/packages/types/src/history.ts b/packages/types/src/history.ts index a60d1a75b6..a305e994d6 100644 --- a/packages/types/src/history.ts +++ b/packages/types/src/history.ts @@ -4,6 +4,11 @@ import { z } from "zod" * HistoryItem */ +export const TASK_STATUSES = ["active", "completed", "delegated", "interrupted"] as const +export type TaskHistoryStatus = (typeof TASK_STATUSES)[number] + +export const COMPLETION_SUMMARY_MAX_LENGTH = 32768 + export const historyItemSchema = z.object({ id: z.string(), rootTaskId: z.string().optional(), @@ -20,12 +25,12 @@ export const historyItemSchema = z.object({ workspace: z.string().optional(), mode: z.string().optional(), apiConfigName: z.string().optional(), // Provider profile name for sticky profile feature - status: z.enum(["active", "completed", "delegated"]).optional(), + status: z.enum(TASK_STATUSES).optional(), delegatedToId: z.string().optional(), // Last child this parent delegated to childIds: z.array(z.string()).optional(), // All children spawned by this task awaitingChildId: z.string().optional(), // Child currently awaited (set when delegated) completedByChildId: z.string().optional(), // Child that completed and resumed this parent - completionResultSummary: z.string().optional(), // Summary from completed child + completionResultSummary: z.string().max(COMPLETION_SUMMARY_MAX_LENGTH).optional(), // Summary from completed child }) export type HistoryItem = z.infer diff --git a/packages/types/src/task.ts b/packages/types/src/task.ts index 7447dc772e..9cbd2d16c1 100644 --- a/packages/types/src/task.ts +++ b/packages/types/src/task.ts @@ -5,6 +5,7 @@ import type { RooCodeSettings } from "./global-settings.js" import type { ClineMessage, QueuedMessage, TokenUsage } from "./message.js" import type { ToolUsage, ToolName } from "./tool.js" import type { TodoItem } from "./todo.js" +import type { TaskHistoryStatus } from "./history.js" /** * TaskProviderLike @@ -89,8 +90,8 @@ 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" + /** Initial status for the task's history item (e.g., "active" for child tasks, "interrupted" for cancelled subtasks) */ + initialStatus?: TaskHistoryStatus /** 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 587ae917a6..f985d0f2f2 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -1,7 +1,7 @@ // npx vitest run __tests__/history-resume-delegation.spec.ts import { describe, it, expect, vi, beforeEach } from "vitest" -import { RooCodeEventName } from "@roo-code/types" +import { COMPLETION_SUMMARY_MAX_LENGTH, RooCodeEventName } from "@roo-code/types" /* vscode mock for Task/Provider imports */ vi.mock("vscode", () => { @@ -111,7 +111,7 @@ describe("History resume delegation - parent metadata transitions", () => { // Verify child closed and parent reopened with updated metadata expect(removeClineFromStack).toHaveBeenCalledTimes(1) - expect(removeClineFromStack).toHaveBeenCalledWith({ skipDelegationRepair: true }) + expect(removeClineFromStack).toHaveBeenCalledWith({ skipChildInterruptMarking: true }) expect(createTaskWithHistoryItem).toHaveBeenCalledWith( expect.objectContaining({ status: "active", @@ -937,6 +937,49 @@ describe("History resume delegation - parent metadata transitions", () => { expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting")) }) + it("reopenParentFromDelegation truncates completionResultSummary to COMPLETION_SUMMARY_MAX_LENGTH", async () => { + const updateTaskHistory = vi.fn().mockResolvedValue([]) + const parentInstance = { + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + overwriteClineMessages: vi.fn().mockResolvedValue(undefined), + overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), + } + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ + historyItem: { + id: "parent-trunc", + status: "delegated", + awaitingChildId: "child-trunc", + }, + }), + emit: vi.fn(), + log: vi.fn(), + getCurrentTask: vi.fn(() => ({ taskId: "child-trunc" })), + removeClineFromStack: vi.fn(), + createTaskWithHistoryItem: vi.fn().mockResolvedValue(parentInstance), + updateTaskHistory, + cancelledDelegationChildIds: new Set(), + } as any) + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + const oversizedSummary = "x".repeat(40000) + await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-trunc", + childTaskId: "child-trunc", + completionResultSummary: oversizedSummary, + }) + + // The completionResultSummary written to history should be capped + const parentUpdate = updateTaskHistory.mock.calls.find( + (call: any[]) => call[0]?.id === "parent-trunc" && call[0]?.completionResultSummary !== undefined, + ) + expect(parentUpdate).toBeDefined() + expect(parentUpdate![0].completionResultSummary.length).toBe(COMPLETION_SUMMARY_MAX_LENGTH) + }) + it("serializes delegation transitions and continues after a rejected predecessor", async () => { const provider = makeProviderStub({} as any) as any const calls: string[] = [] diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index bd4519b970..25366a46c6 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -188,8 +188,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { ).rejects.toThrow(persistError) expect(childStart).not.toHaveBeenCalled() - expect(removeClineFromStack).toHaveBeenNthCalledWith(1, { skipDelegationRepair: true }) - expect(removeClineFromStack).toHaveBeenNthCalledWith(2, { skipDelegationRepair: true }) + expect(removeClineFromStack).toHaveBeenNthCalledWith(1, { skipChildInterruptMarking: true }) + expect(removeClineFromStack).toHaveBeenNthCalledWith(2, { skipChildInterruptMarking: true }) expect(deleteTaskWithId).toHaveBeenCalledWith("child-1", false) expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistory) }) diff --git a/src/__tests__/removeClineFromStack-delegation.spec.ts b/src/__tests__/removeClineFromStack-delegation.spec.ts index 6ed2a5c221..28e224f2a2 100644 --- a/src/__tests__/removeClineFromStack-delegation.spec.ts +++ b/src/__tests__/removeClineFromStack-delegation.spec.ts @@ -13,6 +13,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { childTaskId: string parentTaskId?: string parentHistoryItem?: Record + childHistoryItem?: Record getTaskWithIdError?: Error }) { const childTask = { @@ -30,6 +31,9 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { if (id === opts.parentTaskId && opts.parentHistoryItem) { return { historyItem: { ...opts.parentHistoryItem } } } + if (id === opts.childTaskId && opts.childHistoryItem) { + return { historyItem: { ...opts.childHistoryItem } } + } throw new Error("Task not found") }) @@ -44,7 +48,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { return { provider, childTask, updateTaskHistory, getTaskWithId } } - it("repairs parent metadata (delegated → active) when a delegated child is removed", async () => { + it("marks child as interrupted (preserving parent link) when a delegated child is removed", async () => { const { provider, updateTaskHistory, getTaskWithId } = buildMockProvider({ childTaskId: "child-1", parentTaskId: "parent-1", @@ -61,6 +65,16 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { delegatedToId: "child-1", childIds: ["child-1"], }, + childHistoryItem: { + id: "child-1", + task: "Child task", + ts: 2000, + number: 2, + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + parentTaskId: "parent-1", + }, }) await (ClineProvider.prototype as any).removeClineFromStack.call(provider) @@ -68,22 +82,24 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { // Stack should be empty after pop expect(provider.clineStack).toHaveLength(0) - // Parent lookup should have been called + // Both parent and child should have been looked up expect(getTaskWithId).toHaveBeenCalledWith("parent-1") + expect(getTaskWithId).toHaveBeenCalledWith("child-1") - // Parent metadata should be repaired + // Child should be marked as interrupted (parent stays delegated) expect(updateTaskHistory).toHaveBeenCalledTimes(1) - const updatedParent = updateTaskHistory.mock.calls[0][0] - expect(updatedParent).toEqual( + const updatedChild = updateTaskHistory.mock.calls[0][0] + expect(updatedChild).toEqual( expect.objectContaining({ - id: "parent-1", - status: "active", - awaitingChildId: undefined, + id: "child-1", + status: "interrupted", }), ) + // Parent should NOT have been updated + expect(updateTaskHistory).not.toHaveBeenCalledWith(expect.objectContaining({ id: "parent-1" })) - // Log the repair - expect(provider.log).toHaveBeenCalledWith(expect.stringContaining("Repaired parent parent-1 metadata")) + // Log the action + expect(provider.log).toHaveBeenCalledWith(expect.stringContaining("Marked child child-1 as interrupted")) }) it("does NOT modify parent metadata when the task has no parentTaskId (non-delegated)", async () => { @@ -152,7 +168,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { expect(updateTaskHistory).not.toHaveBeenCalled() }) - it("catches and logs errors during parent metadata repair without blocking the pop", async () => { + it("catches and logs errors during child interrupt marking without blocking the pop", async () => { const { provider, childTask, updateTaskHistory, getTaskWithId } = buildMockProvider({ childTaskId: "child-1", parentTaskId: "parent-1", @@ -168,15 +184,44 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { // The abort should still have been called expect(childTask.abortTask).toHaveBeenCalledWith(true) - // Error should be logged as non-fatal + // Error should be logged as fail-closed expect(provider.log).toHaveBeenCalledWith( - expect.stringContaining("Failed to repair parent metadata for parent-1 (non-fatal)"), + expect.stringContaining("Failed to mark child child-1 as interrupted for parent parent-1 (fail-closed"), ) + // Child should be added to cancelledDelegationChildIds (fail-closed guard) + expect((provider as any).cancelledDelegationChildIds.has("child-1")).toBe(true) + // No update should have been attempted expect(updateTaskHistory).not.toHaveBeenCalled() }) + it("skips redundant write when child is already interrupted (idempotent)", async () => { + const { provider, childTask, updateTaskHistory } = buildMockProvider({ + childTaskId: "child-1", + parentTaskId: "parent-1", + parentHistoryItem: { + id: "parent-1", + status: "delegated", + awaitingChildId: "child-1", + }, + childHistoryItem: { + id: "child-1", + status: "interrupted", + parentTaskId: "parent-1", + }, + }) + + await (ClineProvider.prototype as any).removeClineFromStack.call(provider) + + // Stack should be popped + expect(provider.clineStack).toHaveLength(0) + expect(childTask.abortTask).toHaveBeenCalledWith(true) + + // updateTaskHistory should NOT have been called — child already interrupted + expect(updateTaskHistory).not.toHaveBeenCalled() + }) + it("handles empty stack gracefully", async () => { const provider = { clineStack: [] as any[], @@ -194,7 +239,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { expect(provider.updateTaskHistory).not.toHaveBeenCalled() }) - it("skips delegation repair when skipDelegationRepair option is true", async () => { + it("skips delegation repair when skipChildInterruptMarking option is true", async () => { const { provider, updateTaskHistory, getTaskWithId } = buildMockProvider({ childTaskId: "child-1", parentTaskId: "parent-1", @@ -213,8 +258,8 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { }, }) - // Call with skipDelegationRepair: true (as delegateParentAndOpenChild would) - await (ClineProvider.prototype as any).removeClineFromStack.call(provider, { skipDelegationRepair: true }) + // Call with skipChildInterruptMarking: true (as delegateParentAndOpenChild would) + await (ClineProvider.prototype as any).removeClineFromStack.call(provider, { skipChildInterruptMarking: true }) // Stack should be empty after pop expect(provider.clineStack).toHaveLength(0) @@ -226,7 +271,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { it("does NOT reset grandparent during A→B→C nested delegation transition", async () => { // Scenario: A delegated to B, B is now delegating to C. - // delegateParentAndOpenChild() pops B via removeClineFromStack({ skipDelegationRepair: true }). + // delegateParentAndOpenChild() pops B via removeClineFromStack({ skipChildInterruptMarking: true }). // Grandparent A should remain "delegated" — its metadata must not be repaired. const grandparentHistory = { id: "task-A", @@ -266,8 +311,8 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => { updateTaskHistory, } - // Simulate what delegateParentAndOpenChild does: pop B with skipDelegationRepair - await (ClineProvider.prototype as any).removeClineFromStack.call(provider, { skipDelegationRepair: true }) + // Simulate what delegateParentAndOpenChild does: pop B with skipChildInterruptMarking + await (ClineProvider.prototype as any).removeClineFromStack.call(provider, { skipChildInterruptMarking: true }) // B was popped expect(provider.clineStack).toHaveLength(0) diff --git a/src/core/task-persistence/taskMetadata.ts b/src/core/task-persistence/taskMetadata.ts index 4b77126971..bde2643ec5 100644 --- a/src/core/task-persistence/taskMetadata.ts +++ b/src/core/task-persistence/taskMetadata.ts @@ -1,7 +1,7 @@ import NodeCache from "node-cache" import getFolderSize from "get-folder-size" -import type { ClineMessage, HistoryItem } from "@roo-code/types" +import type { ClineMessage, HistoryItem, TaskHistoryStatus } from "@roo-code/types" import { combineApiRequests } from "../../shared/combineApiRequests" import { combineCommandSequences } from "../../shared/combineCommandSequences" @@ -23,8 +23,8 @@ 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" + /** Initial status for the task (e.g., "active" for child tasks, "interrupted" for cancelled subtasks) */ + initialStatus?: TaskHistoryStatus } export async function taskMetadata({ diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 4de3e595fc..19726ddcc3 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -31,6 +31,7 @@ import { type ClineAsk, type ToolProgressStatus, type HistoryItem, + type TaskHistoryStatus, type CreateTaskOptions, type ModelInfo, type ClineApiReqCancelReason, @@ -157,7 +158,7 @@ export interface TaskOptions extends CreateTaskOptions { initialTodos?: TodoItem[] workspacePath?: string /** Initial status for the task's history item (e.g., "active" for child tasks) */ - initialStatus?: "active" | "delegated" | "completed" + initialStatus?: TaskHistoryStatus } export class Task extends EventEmitter implements TaskLike { @@ -413,7 +414,7 @@ export class Task extends EventEmitter implements TaskLike { 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" + private readonly initialStatus?: TaskHistoryStatus // MessageManager for high-level message operations (lazy initialized) private _messageManager?: MessageManager diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index c6c9bc908e..39feae0d9b 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -97,7 +97,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { // Fall through to normal completion ask flow below (outside this if block) // This shows the user the completion result and waits for acceptance // without injecting another tool_result to the parent - } else if (status === "active") { + } else if (status === "active" || status === "interrupted") { historyLookupTaskId = task.parentTaskId const { historyItem: parentHistory } = await provider.getTaskWithId(task.parentTaskId) @@ -132,7 +132,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { // "delegated" would mean this child has its own grandchild pending (shouldn't reach attempt_completion) provider.log( `[AttemptCompletionTool] Unexpected child task status "${status}" for task ${task.taskId}. ` + - `Expected "active" or "completed". Skipping delegation to prevent data corruption.`, + `Expected "active", "interrupted", or "completed". Skipping delegation to prevent data corruption.`, ) // Fall through to normal completion ask flow } diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index cf21eeee3e..6d967f0308 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -536,6 +536,99 @@ describe("attemptCompletionTool", () => { expect(mockPushToolResult).toHaveBeenCalledWith("") }) + it("delegates an interrupted subtask completion back to the parent that still awaits it", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "resumed result" }, + nativeArgs: { result: "resumed result" }, + partial: false, + } + const mockProvider = { + log: vi.fn(), + getTaskWithId: vi.fn().mockImplementation((id: string) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: { id, status: "interrupted" } }) + } + if (id === "parent-1") { + return Promise.resolve({ + historyItem: { id, status: "delegated", awaitingChildId: "child-1" }, + }) + } + throw new Error(`unexpected task id ${id}`) + }), + reopenParentFromDelegation: vi.fn().mockResolvedValue(true), + } + + Object.assign(mockTask, { + taskId: "child-1", + parentTaskId: "parent-1", + providerRef: { deref: () => mockProvider }, + }) + mockAskFinishSubTaskApproval.mockResolvedValue(true) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + expect(mockAskFinishSubTaskApproval).toHaveBeenCalled() + expect(mockProvider.reopenParentFromDelegation).toHaveBeenCalledWith({ + parentTaskId: "parent-1", + childTaskId: "child-1", + completionResultSummary: "resumed result", + }) + expect(mockTask.ask).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalledWith("") + }) + + it("skips delegation and logs when child has unexpected status like delegated", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "unexpected" }, + nativeArgs: { result: "unexpected" }, + partial: false, + } + const mockProvider = { + log: vi.fn(), + getTaskWithId: vi.fn().mockImplementation((id: string) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: { id, status: "delegated" } }) + } + throw new Error(`unexpected task id ${id}`) + }), + reopenParentFromDelegation: vi.fn(), + } + + Object.assign(mockTask, { + taskId: "child-1", + parentTaskId: "parent-1", + providerRef: { deref: () => mockProvider }, + }) + mockTask.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + expect(mockProvider.log).toHaveBeenCalledWith( + expect.stringContaining('Unexpected child task status "delegated"'), + ) + expect(mockProvider.reopenParentFromDelegation).not.toHaveBeenCalled() + }) + it("falls through to standalone completion when parent delegation becomes stale after approval", async () => { const block: AttemptCompletionToolUse = { type: "tool_use", diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index dc19a58ce2..a5d1eabb7d 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -96,6 +96,7 @@ import { CustomModesManager } from "../config/CustomModesManager" import { Task } from "../task/Task" import { webviewMessageHandler } from "./webviewMessageHandler" +import { COMPLETION_SUMMARY_MAX_LENGTH } from "@roo-code/types" import type { ClineMessage, TodoItem } from "@roo-code/types" import { readApiMessages, saveApiMessages, saveTaskMessages, TaskHistoryStore } from "../task-persistence" import { readTaskMessages } from "../task-persistence/taskMessages" @@ -468,9 +469,7 @@ export class ClineProvider // Removes and destroys the top Cline instance (the current finished task), // activating the previous one (resuming the parent task). - async removeClineFromStack(options?: { skipDelegationRepair?: boolean }) { - const callerStack = new Error().stack - + async removeClineFromStack(options?: { skipChildInterruptMarking?: boolean }) { if (this.clineStack.length === 0) { return } @@ -508,35 +507,59 @@ export class ClineProvider // garbage collected. task = undefined - // Delegation-aware parent metadata repair: - // If the popped task was a delegated child, repair the parent's metadata - // so it transitions from "delegated" back to "active" and becomes resumable - // from the task history list. + // Delegation-aware child interrupt marking: + // If the popped task was a delegated child, mark it as "interrupted" + // while preserving the parent-child link so the child can still + // report back to the parent when resumed. // Skip when called from delegateParentAndOpenChild() during nested delegation // transitions (A→B→C), where the caller intentionally replaces the active // child and will update the parent to point at the new child. - if (parentTaskId && childTaskId && !options?.skipDelegationRepair) { + if (parentTaskId && childTaskId && !options?.skipChildInterruptMarking) { + const callerStack = new Error().stack try { await this.runDelegationTransition(parentTaskId, async () => { const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === childTaskId) { + // Mark child as interrupted but preserve the parent-child link + // so the child can still report back when resumed. + const { historyItem: childHistory } = await this.getTaskWithId(childTaskId) + if (childHistory?.status === "interrupted") { + // Already marked — skip redundant write. + return + } await this.updateTaskHistory({ - ...parentHistory, - status: "active", - awaitingChildId: undefined, + ...childHistory, + status: "interrupted", }) - const repairMsg = - `[ClineProvider#removeClineFromStack] Repaired parent ${parentTaskId} metadata: delegated → active (child ${childTaskId} removed). ` + + const logMsg = + `[ClineProvider#removeClineFromStack] Marked child ${childTaskId} as interrupted (parent ${parentTaskId} stays delegated). ` + `Caller stack: ${callerStack?.split("\n").slice(1, 5).join(" | ")}` - this.log(repairMsg) - console.warn(repairMsg) + this.log(logMsg) + console.warn(logMsg) } }) } catch (err) { - // Non-fatal: log but do not block the pop operation. + // Fail closed: if we cannot confirm the child was marked interrupted, + // add it to cancelledDelegationChildIds so that a stale + // attempt_completion cannot reopen the parent after a provider reload. + // Also persist the parentTaskId stripping so the guard survives reload + // (mirrors cancelTask's fail-closed path). + this.cancelledDelegationChildIds.add(childTaskId) + try { + const { historyItem: childHistory } = await this.getTaskWithId(childTaskId) + if (childHistory) { + await this.updateTaskHistory({ + ...childHistory, + parentTaskId: undefined, + rootTaskId: undefined, + }) + } + } catch { + // Best-effort — the in-memory guard is the primary safeguard. + } this.log( - `[ClineProvider#removeClineFromStack] Failed to repair parent metadata for ${parentTaskId} (non-fatal): ${ + `[ClineProvider#removeClineFromStack] Failed to mark child ${childTaskId} as interrupted for parent ${parentTaskId} (fail-closed, added to cancelledDelegationChildIds): ${ err instanceof Error ? err.message : String(err) }`, ) @@ -971,12 +994,68 @@ export class ClineProvider // runtime settings with stale/incomplete persisted profiles. const skipProfileRestoreFromHistory = isCliRuntime + // Self-heal orphaned "delegated" parents on rehydration: if the awaited + // child no longer exists, reset the parent to "active" so it is resumable. + // Also heal when the child exists but has had its parentTaskId stripped + // (fail-closed cancel path), since the child can no longer delegate back. + // Serialized through runDelegationTransition to avoid racing cancelTask/reopenParentFromDelegation. + if (historyItem.status === "delegated" && historyItem.awaitingChildId) { + const awaitedChildId = historyItem.awaitingChildId + const parentId = historyItem.id + try { + historyItem = await this.runDelegationTransition(parentId, async () => { + let shouldSelfHeal = false + let healReason = "" + try { + const { historyItem: childHistory } = await this.getTaskWithId(awaitedChildId) + // Child exists but was severed (fail-closed cancel stripped parentTaskId). + // The child can no longer delegate back, so unstick the parent. + if (childHistory && childHistory.parentTaskId !== parentId) { + shouldSelfHeal = true + healReason = `child exists but parentTaskId is ${childHistory.parentTaskId ?? "undefined"}, expected ${parentId}` + } + } catch (err) { + // Only self-heal for "Task not found". Transient I/O errors should + // not sever a valid parent-child link. + if (err instanceof Error && err.message === "Task not found") { + shouldSelfHeal = true + healReason = "child not found in history" + } else { + this.log( + `[createTaskWithHistoryItem] Transient error checking child ${awaitedChildId} for parent ${parentId} (skipping self-heal): ${err instanceof Error ? err.message : String(err)}`, + ) + } + } + if (shouldSelfHeal) { + this.log( + `[createTaskWithHistoryItem] Self-healing orphaned parent ${parentId}: ${healReason}. Resetting to active.`, + ) + const healed = { ...historyItem, status: "active" as const, awaitingChildId: undefined } + await this.updateTaskHistory(healed).catch((healErr) => + this.log( + `[createTaskWithHistoryItem] Failed to persist self-healed state for ${parentId}: ${healErr instanceof Error ? healErr.message : String(healErr)}`, + ), + ) + return healed + } + return historyItem + }) + } catch (lockErr) { + this.log( + `[createTaskWithHistoryItem] Self-heal lock error for parent ${parentId} (proceeding without heal): ${lockErr instanceof Error ? lockErr.message : String(lockErr)}`, + ) + } + } + // Check if we're rehydrating the current task to avoid flicker const currentTask = this.getCurrentTask() const isRehydratingCurrentTask = currentTask && currentTask.taskId === historyItem.id if (!isRehydratingCurrentTask) { - await this.removeClineFromStack() + // skipChildInterruptMarking: the caller is resuming/opening a task from + // history, not cancelling a delegation — any child on the stack should + // not be collaterally marked as interrupted. + await this.removeClineFromStack({ skipChildInterruptMarking: true }) } // If the history item has a saved mode, restore it and its associated API configuration. @@ -1984,6 +2063,32 @@ export class ClineProvider await collectChildIds(id) } + // Repair orphaned parent if the deleted task was an awaited delegation child. + // Serialized through runDelegationTransition to avoid racing cancelTask/reopenParentFromDelegation. + if (historyItem.parentTaskId) { + try { + await this.runDelegationTransition(historyItem.parentTaskId, async () => { + const { historyItem: parentHistory } = await this.getTaskWithId(historyItem.parentTaskId!) + if (parentHistory && parentHistory.awaitingChildId === id) { + await this.updateTaskHistory({ + ...parentHistory, + status: "active", + awaitingChildId: undefined, + childIds: parentHistory.childIds?.filter((cid) => cid !== id), + }) + this.log( + `[deleteTaskWithId] Repaired parent ${historyItem.parentTaskId}: cleared stale awaitingChildId=${id}, reset to active.`, + ) + } + }) + } catch (err) { + // Parent may already be deleted or not found — nothing to repair. + this.log( + `[deleteTaskWithId] Could not repair parent ${historyItem.parentTaskId} for deleted child ${id}: ${err instanceof Error ? err.message : String(err)}`, + ) + } + } + // Remove from stack if any of the tasks to delete are in the current task stack for (const taskId of allIdsToDelete) { if (taskId === this.getCurrentTask()?.taskId) { @@ -3168,23 +3273,30 @@ export class ClineProvider const { historyItem: parentHistory } = await this.getTaskWithId(task.parentTaskId!) if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === task.taskId) { - await this.updateTaskHistory({ - ...parentHistory, - status: "active", - awaitingChildId: undefined, - }) + // Already marked — skip redundant write (idempotency guard). + if (historyItem!.status === "interrupted") { + return + } + // Mark the child as interrupted but preserve the parent-child link. + // The parent stays "delegated" with awaitingChildId intact so that + // when the user resumes the child and it calls attempt_completion, + // it can still report back to the parent. + // historyItem is guaranteed non-null by the early return guard above. + historyItem = { + ...historyItem!, + status: "interrupted", + } + await this.updateTaskHistory(historyItem) this.log( - `[cancelTask] Detached delegated parent ${task.parentTaskId}: delegated → active (child ${task.taskId} cancelled)`, + `[cancelTask] Marked child ${task.taskId} as interrupted (parent ${task.parentTaskId} stays delegated)`, ) - parentTask = undefined - rootTask = undefined // Clear any stale fail-closed entry from a prior failed cancel attempt. this.cancelledDelegationChildIds.delete(task.taskId) } }) } catch (error) { - // Fail closed: if we cannot prove the parent was detached, make the + // Fail closed: if we cannot prove the child was marked interrupted, make the // rehydrated child standalone so later completions cannot reopen a // stale delegated parent, even after a provider reload. parentTask = undefined @@ -3206,7 +3318,7 @@ export class ClineProvider throw historyError } this.log( - `[cancelTask] Failed to detach delegated parent for ${task.taskId}: ${ + `[cancelTask] Failed to mark child as interrupted for ${task.taskId}: ${ error instanceof Error ? error.message : String(error) }`, ) @@ -3436,7 +3548,7 @@ export class ClineProvider // 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 }) + await this.removeClineFromStack({ skipChildInterruptMarking: true }) } catch (error) { this.log( `[delegateParentAndOpenChild] Error during parent disposal (non-fatal): ${ @@ -3496,7 +3608,7 @@ export class ClineProvider }`, ) try { - await this.removeClineFromStack({ skipDelegationRepair: true }) + await this.removeClineFromStack({ skipChildInterruptMarking: true }) } catch (cleanupError) { this.log( `[delegateParentAndOpenChild] Failed to close paused child ${child.taskId} during rollback: ${ @@ -3547,7 +3659,11 @@ export class ClineProvider childTaskId: string completionResultSummary: string }): Promise { - const { parentTaskId, childTaskId, completionResultSummary } = params + const { parentTaskId, childTaskId, completionResultSummary: rawSummary } = params + const completionResultSummary = + rawSummary.length > COMPLETION_SUMMARY_MAX_LENGTH + ? rawSummary.slice(0, COMPLETION_SUMMARY_MAX_LENGTH) + : rawSummary return this.runDelegationTransition(parentTaskId, async () => { const globalStoragePath = this.contextProxy.globalStorageUri.fsPath @@ -3555,10 +3671,10 @@ export class ClineProvider const { historyItem } = await this.getTaskWithId(parentTaskId) // Guard: re-validate delegation state after the async approval gap. - // cancelTask() or removeClineFromStack() may have already detached the parent - // (setting status → "active", awaitingChildId → undefined) while the user was - // approving the subtask finish. If the parent no longer awaits this child, - // routing output back would corrupt an unrelated task. + // The cancelledDelegationChildIds set (fail-closed fallback) or an explicit + // abandon action may have detached the parent while the user was approving + // the subtask finish. If the parent no longer awaits this child, routing + // output back would corrupt an unrelated task. if ( this.cancelledDelegationChildIds.has(childTaskId) || (historyItem.status !== "delegated" && historyItem.status !== "active") || @@ -3719,7 +3835,7 @@ export class ClineProvider // overwrite a "completed" status set earlier. const current = this.getCurrentTask() if (current?.taskId === childTaskId) { - await this.removeClineFromStack({ skipDelegationRepair: true }) + await this.removeClineFromStack({ skipChildInterruptMarking: true }) } // 5) Update child metadata to "completed" status. diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts index 2710136978..141871d4ea 100644 --- a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -321,7 +321,184 @@ describe("ClineProvider flicker-free cancel", () => { expect((provider as any).clineStack[1]).toBe(mockTask2) }) - it("detaches runtime parent links for a cancelled delegated child while preserving history lineage", async () => { + it("self-heals orphaned delegated parent when awaited child no longer exists", async () => { + // Setup: delegated parent whose awaitingChildId points to a deleted child + const orphanedHistory: HistoryItem = { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + status: "delegated", + awaitingChildId: "deleted-child", + } + + ;(provider as any).clineStack = [] + provider.getTaskWithId = vi.fn().mockImplementation((id) => { + if (id === "deleted-child") { + return Promise.reject(new Error("Task not found")) + } + return Promise.resolve({ historyItem: orphanedHistory }) + }) as any + const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([]) + vi.spyOn(provider, "removeClineFromStack").mockResolvedValue(undefined) + + await provider.createTaskWithHistoryItem(orphanedHistory) + + // Should have persisted the self-healed state + expect(updateTaskHistorySpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: "parent-1", + status: "active", + awaitingChildId: undefined, + }), + ) + }) + + it("self-heals delegated parent when awaited child exists but parentTaskId was severed", async () => { + // Scenario: fail-closed cancel stripped parentTaskId from child; parent is stuck "delegated" + const orphanedHistory: HistoryItem = { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + status: "delegated", + awaitingChildId: "child-1", + } + const severedChildHistory: HistoryItem = { + id: "child-1", + number: 2, + task: "child task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + status: "interrupted", + parentTaskId: undefined, // severed by fail-closed cancel + } + + ;(provider as any).clineStack = [] + provider.getTaskWithId = vi.fn().mockImplementation((id) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: severedChildHistory }) + } + if (id === "parent-1") { + return Promise.resolve({ historyItem: orphanedHistory }) + } + throw new Error(`unexpected task lookup: ${id}`) + }) as any + const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([]) + vi.spyOn(provider, "removeClineFromStack").mockResolvedValue(undefined) + + await provider.createTaskWithHistoryItem(orphanedHistory) + + // Should self-heal because child's parentTaskId doesn't match parent's id + expect(updateTaskHistorySpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: "parent-1", + status: "active", + awaitingChildId: undefined, + }), + ) + }) + + it("does not self-heal delegated parent on transient I/O error from getTaskWithId", async () => { + const delegatedHistory: HistoryItem = { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + status: "delegated", + awaitingChildId: "child-1", + } + + ;(provider as any).clineStack = [] + provider.getTaskWithId = vi.fn().mockImplementation((id) => { + if (id === "child-1") { + return Promise.reject(new Error("EMFILE: too many open files")) + } + if (id === "parent-1") { + return Promise.resolve({ historyItem: delegatedHistory }) + } + throw new Error(`unexpected task lookup: ${id}`) + }) as any + const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([]) + vi.spyOn(provider, "removeClineFromStack").mockResolvedValue(undefined) + + await provider.createTaskWithHistoryItem(delegatedHistory) + + // Should NOT self-heal — the error is transient, not "Task not found" + expect(updateTaskHistorySpy).not.toHaveBeenCalledWith( + expect.objectContaining({ + status: "active", + awaitingChildId: undefined, + }), + ) + }) + + it("does not self-heal delegated parent when awaited child still exists", async () => { + const delegatedHistory: HistoryItem = { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + status: "delegated", + awaitingChildId: "child-1", + } + const childHistory: HistoryItem = { + id: "child-1", + number: 2, + task: "child task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + status: "interrupted", + parentTaskId: "parent-1", + } + + ;(provider as any).clineStack = [] + provider.getTaskWithId = vi.fn().mockImplementation((id) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: childHistory }) + } + if (id === "parent-1") { + return Promise.resolve({ historyItem: delegatedHistory }) + } + throw new Error(`unexpected task lookup: ${id}`) + }) as any + const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([]) + vi.spyOn(provider, "removeClineFromStack").mockResolvedValue(undefined) + + await provider.createTaskWithHistoryItem(delegatedHistory) + + // Should NOT have called updateTaskHistory for self-healing (child exists with matching parentTaskId) + expect(updateTaskHistorySpy).not.toHaveBeenCalledWith( + expect.objectContaining({ + status: "active", + awaitingChildId: undefined, + }), + ) + }) + + it("marks cancelled delegated child as interrupted while preserving parent-child link", async () => { const mockRootTask = { taskId: "root-1" } const mockParentTask = { taskId: "parent-1" } const childHistory: HistoryItem = { @@ -381,25 +558,27 @@ describe("ClineProvider flicker-free cancel", () => { await provider.cancelTask() + // Child should be marked as interrupted (not detached from parent) expect(updateTaskHistorySpy).toHaveBeenCalledWith( expect.objectContaining({ - id: "parent-1", - status: "active", - awaitingChildId: undefined, + id: "child-1", + status: "interrupted", + parentTaskId: "parent-1", + rootTaskId: "root-1", }), ) + // Parent links should be preserved for rehydration expect(createTaskWithHistoryItemSpy).toHaveBeenCalledWith( expect.objectContaining({ id: "child-1", + status: "interrupted", parentTaskId: "parent-1", rootTaskId: "root-1", - parentTask: undefined, - rootTask: undefined, }), ) }) - it("detaches runtime parent links when delegated parent detach fails", async () => { + it("detaches runtime parent links when marking child as interrupted fails", async () => { const mockRootTask = { taskId: "root-1" } const mockParentTask = { taskId: "parent-1" } const childHistory: HistoryItem = { @@ -447,7 +626,9 @@ describe("ClineProvider flicker-free cancel", () => { await provider.cancelTask() expect(mockOutputChannel.appendLine).toHaveBeenCalledWith( - expect.stringContaining("[cancelTask] Failed to detach delegated parent for child-1: parent lookup failed"), + expect.stringContaining( + "[cancelTask] Failed to mark child as interrupted for child-1: parent lookup failed", + ), ) expect(updateTaskHistorySpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -513,4 +694,112 @@ describe("ClineProvider flicker-free cancel", () => { expect(createTaskWithHistoryItemSpy).not.toHaveBeenCalled() expect((provider as any).cancelledDelegationChildIds.has("child-1")).toBe(true) }) + + it("repairs orphaned parent when deleting an awaited delegation child", async () => { + const childHistory: HistoryItem = { + id: "child-1", + number: 2, + task: "child task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + parentTaskId: "parent-1", + rootTaskId: "root-1", + } + const parentHistory: HistoryItem = { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + status: "delegated", + awaitingChildId: "child-1", + childIds: ["child-1", "child-other"], + } + + ;(provider as any).clineStack = [] + provider.getTaskWithId = vi.fn().mockImplementation((id) => { + if (id === "child-1") { + return Promise.resolve({ historyItem: childHistory, taskDirPath: "/test/storage/child-1" }) + } + if (id === "parent-1") { + return Promise.resolve({ historyItem: parentHistory }) + } + throw new Error(`unexpected task lookup: ${id}`) + }) as any + + const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([]) + ;(provider as any).taskHistoryStore = { + deleteMany: vi.fn().mockResolvedValue(undefined), + } + + await provider.deleteTaskWithId("child-1") + + // Parent should be repaired: reset to active with awaitingChildId cleared and childIds cleaned + expect(updateTaskHistorySpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: "parent-1", + status: "active", + awaitingChildId: undefined, + childIds: ["child-other"], + }), + ) + }) + + it("does not repair parent when deleted child is not the awaited one", async () => { + const childHistory: HistoryItem = { + id: "child-2", + number: 3, + task: "other child", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + parentTaskId: "parent-1", + } + const parentHistory: HistoryItem = { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 10, + tokensOut: 20, + totalCost: 0.001, + workspace: "/test/workspace", + status: "delegated", + awaitingChildId: "child-1", // awaiting a different child + } + + ;(provider as any).clineStack = [] + provider.getTaskWithId = vi.fn().mockImplementation((id) => { + if (id === "child-2") { + return Promise.resolve({ historyItem: childHistory, taskDirPath: "/test/storage/child-2" }) + } + if (id === "parent-1") { + return Promise.resolve({ historyItem: parentHistory }) + } + throw new Error(`unexpected task lookup: ${id}`) + }) as any + + const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([]) + ;(provider as any).taskHistoryStore = { + deleteMany: vi.fn().mockResolvedValue(undefined), + } + + await provider.deleteTaskWithId("child-2") + + // Parent should NOT be repaired — it awaits a different child + expect(updateTaskHistorySpy).not.toHaveBeenCalledWith( + expect.objectContaining({ + id: "parent-1", + status: "active", + }), + ) + }) }) diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 6850532e04..3b990fbcbb 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -254,7 +254,7 @@ describe("McpHub", () => { // Create McpHub and let it initialize const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Find the connection const connection = mcpHub.connections.find((conn) => conn.server.name === "union-test-server") @@ -286,7 +286,7 @@ describe("McpHub", () => { // Create McpHub and let it initialize const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Find the connection const connection = mcpHub.connections.find((conn) => conn.server.name === "disabled-union-server") @@ -315,7 +315,7 @@ describe("McpHub", () => { const mcpHub = new McpHub(mockProvider as ClineProvider) // Wait for initialization - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Clear any connections that might have been created mcpHub.connections = [] @@ -426,7 +426,7 @@ describe("McpHub", () => { ) const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Verify watcher was created expect(chokidar.watch).toHaveBeenCalledWith(["/path/to/watch"], expect.any(Object)) @@ -503,7 +503,7 @@ describe("McpHub", () => { ) const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Verify watchers were created expect(chokidar.watch).toHaveBeenCalled() @@ -537,7 +537,7 @@ describe("McpHub", () => { vi.mocked(chokidar.watch).mockClear() const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Verify no watcher was created for disabled server expect(chokidar.watch).not.toHaveBeenCalled() @@ -561,7 +561,7 @@ describe("McpHub", () => { ) const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Find the connection const connection = mcpHub.connections.find((conn) => conn.server.name === "mcp-disabled-server") @@ -587,7 +587,7 @@ describe("McpHub", () => { ) const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Find the connection const connection = mcpHub.connections.find((conn) => conn.server.name === "server-disabled-server") @@ -614,7 +614,7 @@ describe("McpHub", () => { ) const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Find the connection const connection = mcpHub.connections.find((conn) => conn.server.name === "both-reasons-server") @@ -645,7 +645,7 @@ describe("McpHub", () => { const mcpHub = new McpHub(mockProvider as ClineProvider) // Wait for initialization - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // The server should be created as a disconnected connection with null client/transport const connection = mcpHub.connections.find((conn) => conn.server.name === "null-safety-server") @@ -715,7 +715,7 @@ describe("McpHub", () => { ) const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Get the connection const connection = mcpHub.connections.find((conn) => conn.server.name === "type-check-server") @@ -733,7 +733,7 @@ describe("McpHub", () => { it("should handle missing connections safely", async () => { const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Try operations on non-existent server await expect(mcpHub.callTool("non-existent-server", "test-tool", {})).rejects.toThrow( @@ -791,7 +791,7 @@ describe("McpHub", () => { ) const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Delete the connection await mcpHub.deleteConnection("delete-safety-server") @@ -1414,7 +1414,7 @@ describe("McpHub", () => { const mcpHub = new McpHub(mockProvider as ClineProvider) // Wait for initialization - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // The server should be created as a disconnected connection const connection = mcpHub.connections.find((conn) => conn.server.name === "disabled-server") @@ -1445,7 +1445,7 @@ describe("McpHub", () => { const mcpHub = new McpHub(mockProvider as ClineProvider) // Wait for initialization - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // The server should be created as a disconnected connection const connection = mcpHub.connections.find((conn) => conn.server.name === "disabled-server") @@ -1831,7 +1831,7 @@ describe("McpHub", () => { // Create McpHub and let it initialize with MCP enabled const mcpHub = new McpHub(mockProvider as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Verify server is connected const connectedServer = mcpHub.connections.find((conn) => conn.server.name === "toggle-test-server") @@ -1889,7 +1889,7 @@ describe("McpHub", () => { const mcpHub = new McpHub(disabledMockProvider as unknown as ClineProvider) // Wait for initialization - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Find the disabled-test-server const disabledServer = mcpHub.connections.find((conn) => conn.server.name === "disabled-test-server") @@ -1961,7 +1961,7 @@ describe("McpHub", () => { const mcpHub = new McpHub(enabledMockProvider as unknown as ClineProvider) // Wait for initialization - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Find the enabled-test-server const enabledServer = mcpHub.connections.find((conn) => conn.server.name === "enabled-test-server") @@ -2000,7 +2000,7 @@ describe("McpHub", () => { // Create McpHub with disabled MCP const mcpHub = new McpHub(disabledMockProvider as unknown as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Clear previous calls vi.clearAllMocks() @@ -2047,7 +2047,7 @@ describe("McpHub", () => { // Create McpHub with disabled MCP const mcpHub = new McpHub(disabledMockProvider as unknown as ClineProvider) - await new Promise((resolve) => setTimeout(resolve, 100)) + await mcpHub.waitUntilReady() // Set isConnecting to false to ensure it's properly reset mcpHub.isConnecting = false @@ -2125,10 +2125,8 @@ describe("McpHub", () => { } }) - // Create a new McpHub instance - const mcpHub = new McpHub(mockProvider as ClineProvider) - - // Mock the config file read + // Mock the config file read BEFORE creating McpHub to avoid racing + // the constructor's initializeGlobalMcpServers() vi.mocked(fs.readFile).mockResolvedValue( JSON.stringify({ mcpServers: { @@ -2140,8 +2138,9 @@ describe("McpHub", () => { }), ) - // Initialize servers (this will trigger connectToServer) - await mcpHub["initializeGlobalMcpServers"]() + // Create McpHub and wait for constructor init to complete + const mcpHub = new McpHub(mockProvider as ClineProvider) + await mcpHub.waitUntilReady() // Verify StdioClientTransport was called with wrapped command expect(StdioClientTransport).toHaveBeenCalledWith( @@ -2189,10 +2188,7 @@ describe("McpHub", () => { } }) - // Create a new McpHub instance - const mcpHub = new McpHub(mockProvider as ClineProvider) - - // Mock the config file read + // Mock the config file read BEFORE creating McpHub vi.mocked(fs.readFile).mockResolvedValue( JSON.stringify({ mcpServers: { @@ -2204,8 +2200,9 @@ describe("McpHub", () => { }), ) - // Initialize servers (this will trigger connectToServer) - await mcpHub["initializeGlobalMcpServers"]() + // Create McpHub and wait for constructor init to complete + const mcpHub = new McpHub(mockProvider as ClineProvider) + await mcpHub.waitUntilReady() // Verify StdioClientTransport was called without wrapping expect(StdioClientTransport).toHaveBeenCalledWith( @@ -2253,10 +2250,7 @@ describe("McpHub", () => { } }) - // Create a new McpHub instance - const mcpHub = new McpHub(mockProvider as ClineProvider) - - // Mock the config file read with cmd.exe already as command + // Mock the config file read BEFORE creating McpHub vi.mocked(fs.readFile).mockResolvedValue( JSON.stringify({ mcpServers: { @@ -2268,8 +2262,9 @@ describe("McpHub", () => { }), ) - // Initialize servers (this will trigger connectToServer) - await mcpHub["initializeGlobalMcpServers"]() + // Create McpHub and wait for constructor init to complete + const mcpHub = new McpHub(mockProvider as ClineProvider) + await mcpHub.waitUntilReady() // Verify StdioClientTransport was called without double-wrapping expect(StdioClientTransport).toHaveBeenCalledWith( @@ -2300,17 +2295,10 @@ describe("McpHub", () => { onclose: null, } - let callCount = 0 StdioClientTransport.mockImplementation(function (config: any) { - callCount++ - // First call would fail with ENOENT if not wrapped - // Second call should be wrapped with cmd.exe - if (callCount === 1) { - // This simulates what would happen without wrapping - expect(config.command).toBe("cmd.exe") - expect(config.args[0]).toBe("/c") - expect(config.args[1]).toBe("npx") - } + expect(config.command).toBe("cmd.exe") + expect(config.args[0]).toBe("/c") + expect(config.args[1]).toBe("npx") return mockTransport }) @@ -2324,10 +2312,7 @@ describe("McpHub", () => { } }) - // Create a new McpHub instance - const mcpHub = new McpHub(mockProvider as ClineProvider) - - // Mock the config file read - simulating fnm/nvm-windows scenario + // Mock the config file read BEFORE creating McpHub vi.mocked(fs.readFile).mockResolvedValue( JSON.stringify({ mcpServers: { @@ -2345,8 +2330,9 @@ describe("McpHub", () => { }), ) - // Initialize servers (this will trigger connectToServer) - await mcpHub["initializeGlobalMcpServers"]() + // Create McpHub and wait for constructor init to complete + const mcpHub = new McpHub(mockProvider as ClineProvider) + await mcpHub.waitUntilReady() // Verify that the command was wrapped with cmd.exe expect(StdioClientTransport).toHaveBeenCalledWith( @@ -2399,10 +2385,7 @@ describe("McpHub", () => { } }) - // Create a new McpHub instance - const mcpHub = new McpHub(mockProvider as ClineProvider) - - // Mock the config file read with CMD (uppercase) as command + // Mock the config file read BEFORE creating McpHub vi.mocked(fs.readFile).mockResolvedValue( JSON.stringify({ mcpServers: { @@ -2414,8 +2397,9 @@ describe("McpHub", () => { }), ) - // Initialize servers (this will trigger connectToServer) - await mcpHub["initializeGlobalMcpServers"]() + // Create McpHub and wait for constructor init to complete + const mcpHub = new McpHub(mockProvider as ClineProvider) + await mcpHub.waitUntilReady() // Verify StdioClientTransport was called without double-wrapping expect(StdioClientTransport).toHaveBeenCalledWith(