diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index 1f95d0f6dd..5934e9fe39 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -491,4 +491,175 @@ describe("History resume delegation - parent metadata transitions", () => { }), ) }) + + it("reopenParentFromDelegation uses fallback anchor when subtaskId link is missing but child is valid", async () => { + const provider = { + contextProxy: { globalStorageUri: { fsPath: "/storage" } }, + getTaskWithId: vi.fn().mockImplementation((taskId: string) => { + if (taskId === "parent-fallback") { + return Promise.resolve({ + historyItem: { + id: "parent-fallback", + status: "delegated", + awaitingChildId: "child-fallback", + childIds: ["child-fallback"], // This validates the parent-child relationship + ts: 100, + task: "Parent task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + }, + }) + } + // Child history item with tokens/cost + return Promise.resolve({ + historyItem: { + id: "child-fallback", + tokensIn: 500, + tokensOut: 300, + totalCost: 0.05, + ts: 200, + task: "Child task", + }, + }) + }), + emit: vi.fn(), + getCurrentTask: vi.fn(() => ({ taskId: "child-fallback" })), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTaskWithHistoryItem: vi.fn().mockResolvedValue({ + taskId: "parent-fallback", + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + }), + updateTaskHistory: vi.fn().mockResolvedValue([]), + } as unknown as ClineProvider + + // Parent has all completed todos but NO subtaskId link + const parentMessagesWithCompletedTodos = [ + { + type: "say", + say: "user_edit_todos", + text: JSON.stringify({ + tool: "updateTodoList", + todos: [ + { id: "todo-1", content: "First completed", status: "completed" }, + { id: "todo-2", content: "Second completed", status: "completed" }, + { id: "todo-3", content: "Last completed", status: "completed" }, + // Note: NO subtaskId on any todo - this is the bug scenario + ], + }), + ts: 50, + }, + ] + + vi.mocked(readTaskMessages).mockResolvedValue(parentMessagesWithCompletedTodos as any) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-fallback", + childTaskId: "child-fallback", + completionResultSummary: "Child completed successfully", + }) + + // Verify that saveTaskMessages was called and includes the todo write-back + expect(saveTaskMessages).toHaveBeenCalled() + const savedCall = vi.mocked(saveTaskMessages).mock.calls[0][0] + + // Find the user_edit_todos message that was added for the write-back + const todoEditMessages = savedCall.messages.filter((m: any) => m.type === "say" && m.say === "user_edit_todos") + + // Should have at least 2 todo edit messages (original + write-back) + expect(todoEditMessages.length).toBeGreaterThanOrEqual(1) + + // Parse the last todo edit to verify fallback worked + const lastTodoEdit = todoEditMessages[todoEditMessages.length - 1] + expect(lastTodoEdit.text).toBeDefined() + const parsedTodos = JSON.parse(lastTodoEdit.text as string) + + // The LAST completed todo should have been selected as the fallback anchor + // and should now have subtaskId, tokens, and cost + const anchoredTodo = parsedTodos.todos.find((t: any) => t.subtaskId === "child-fallback") + expect(anchoredTodo).toBeDefined() + expect(anchoredTodo.content).toBe("Last completed") // Fallback picks LAST completed + expect(anchoredTodo.tokens).toBe(800) // 500 + 300 + expect(anchoredTodo.cost).toBe(0.05) + }) + + it("reopenParentFromDelegation does NOT apply fallback when childIds doesn't include the child", async () => { + const provider = { + contextProxy: { globalStorageUri: { fsPath: "/storage" } }, + getTaskWithId: vi.fn().mockImplementation((taskId: string) => { + if (taskId === "parent-no-relation") { + return Promise.resolve({ + historyItem: { + id: "parent-no-relation", + status: "delegated", + awaitingChildId: "some-other-child", + childIds: ["some-other-child"], // Does NOT include child-orphan + ts: 100, + task: "Parent task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + }, + }) + } + return Promise.resolve({ + historyItem: { + id: "child-orphan", + tokensIn: 100, + tokensOut: 50, + totalCost: 0.01, + ts: 200, + task: "Orphan child", + }, + }) + }), + emit: vi.fn(), + getCurrentTask: vi.fn(() => ({ taskId: "child-orphan" })), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTaskWithHistoryItem: vi.fn().mockResolvedValue({ + taskId: "parent-no-relation", + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + }), + updateTaskHistory: vi.fn().mockResolvedValue([]), + } as unknown as ClineProvider + + const parentMessagesWithTodos = [ + { + type: "say", + say: "user_edit_todos", + text: JSON.stringify({ + tool: "updateTodoList", + todos: [{ id: "todo-1", content: "Some task", status: "completed" }], + }), + ts: 50, + }, + ] + + vi.mocked(readTaskMessages).mockResolvedValue(parentMessagesWithTodos as any) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-no-relation", + childTaskId: "child-orphan", + completionResultSummary: "Orphan child completed", + }) + + // Verify saveTaskMessages was called + expect(saveTaskMessages).toHaveBeenCalled() + const savedCall = vi.mocked(saveTaskMessages).mock.calls[0][0] + + // Find todo edit messages (if any were added beyond the original) + const todoEditMessages = savedCall.messages.filter((m: any) => m.type === "say" && m.say === "user_edit_todos") + + // Should only have the original todo edit, no write-back because child isn't in childIds + // The fallback should NOT be triggered for an unrelated child + if (todoEditMessages.length > 1) { + const lastTodoEdit = todoEditMessages[todoEditMessages.length - 1] + const parsedTodos = JSON.parse(lastTodoEdit.text as string) + // If a write-back happened, it should NOT have linked to child-orphan + const orphanLinked = parsedTodos.todos.find((t: any) => t.subtaskId === "child-orphan") + expect(orphanLinked).toBeUndefined() + } + }) }) diff --git a/src/__tests__/new-task-delegation.spec.ts b/src/__tests__/new-task-delegation.spec.ts index b6f6d4d36c..85b107fdac 100644 --- a/src/__tests__/new-task-delegation.spec.ts +++ b/src/__tests__/new-task-delegation.spec.ts @@ -42,3 +42,79 @@ describe("Task.startSubtask() metadata-driven delegation", () => { expect(provider.createTask).not.toHaveBeenCalled() }) }) + +describe("Deterministic todo anchor selection for subtaskId linking", () => { + // Helper to simulate the anchor selection algorithm from delegateParentAndOpenChild + function selectDeterministicAnchor( + todos: Array<{ id: string; content: string; status: string; subtaskId?: string }>, + ): { id: string; content: string; status: string; subtaskId?: string } | undefined { + const inProgress = todos.filter((t) => t?.status === "in_progress") + const pending = todos.filter((t) => t?.status === "pending") + const completed = todos.filter((t) => t?.status === "completed") + + if (inProgress.length > 0) { + return inProgress[0] + } else if (pending.length > 0) { + return pending[0] + } else if (completed.length > 0) { + return completed[completed.length - 1] // Last completed + } + return undefined + } + + it("selects first in_progress todo when available", () => { + const todos = [ + { id: "1", content: "Task A", status: "completed" }, + { id: "2", content: "Task B", status: "in_progress" }, + { id: "3", content: "Task C", status: "pending" }, + ] + + const chosen = selectDeterministicAnchor(todos) + expect(chosen?.id).toBe("2") + expect(chosen?.status).toBe("in_progress") + }) + + it("selects first pending todo when no in_progress", () => { + const todos = [ + { id: "1", content: "Task A", status: "completed" }, + { id: "2", content: "Task B", status: "pending" }, + { id: "3", content: "Task C", status: "pending" }, + ] + + const chosen = selectDeterministicAnchor(todos) + expect(chosen?.id).toBe("2") + expect(chosen?.status).toBe("pending") + }) + + it("selects LAST completed todo when all todos are completed", () => { + const todos = [ + { id: "1", content: "Task A", status: "completed" }, + { id: "2", content: "Task B", status: "completed" }, + { id: "3", content: "Task C", status: "completed" }, + ] + + const chosen = selectDeterministicAnchor(todos) + // Should pick the LAST completed (closest to delegation moment) + expect(chosen?.id).toBe("3") + expect(chosen?.content).toBe("Task C") + }) + + it("returns undefined when no todos exist (triggers synthetic anchor creation)", () => { + const todos: Array<{ id: string; content: string; status: string }> = [] + const chosen = selectDeterministicAnchor(todos) + expect(chosen).toBeUndefined() + }) + + it("handles mixed statuses deterministically", () => { + const todos = [ + { id: "1", content: "Done early", status: "completed" }, + { id: "2", content: "In progress", status: "in_progress" }, + { id: "3", content: "Done late", status: "completed" }, + { id: "4", content: "Still pending", status: "pending" }, + ] + + // Should prefer in_progress over everything + const chosen = selectDeterministicAnchor(todos) + expect(chosen?.id).toBe("2") + }) +}) diff --git a/src/core/tools/UpdateTodoListTool.ts b/src/core/tools/UpdateTodoListTool.ts index cb2c37cb21..068d473b53 100644 --- a/src/core/tools/UpdateTodoListTool.ts +++ b/src/core/tools/UpdateTodoListTool.ts @@ -202,27 +202,94 @@ function normalizeStatus(status: string | undefined): TodoStatus { return "pending" } +/** + * Preserve metadata (subtaskId, tokens, cost) from previous todos onto next todos. + * + * Matching strategy (in priority order): + * 1. **ID match**: If both todos have an `id` field and they match exactly, preserve metadata. + * This handles the common case where ID is stable across updates. + * 2. **Content match with position awareness**: For todos without matching IDs, fall back to + * content-based matching. Duplicates are matched in order (first unmatched previous with + * same content gets matched to first unmatched next with same content). + * + * This approach ensures metadata survives status changes (which can alter the derived ID) + * and handles duplicates deterministically. + */ function preserveTodoMetadata(nextTodos: TodoItem[], previousTodos: TodoItem[]): TodoItem[] { - // Build content -> queue mapping so duplicates are matched in order. - const previousByContent = new Map() - for (const prev of previousTodos ?? []) { - if (!prev || typeof prev.content !== "string") continue - const list = previousByContent.get(prev.content) - if (list) list.push(prev) - else previousByContent.set(prev.content, [prev]) + const safePrevious = previousTodos ?? [] + const safeNext = nextTodos ?? [] + + // Build ID -> todo mapping for O(1) lookup + const previousById = new Map() + for (const prev of safePrevious) { + if (prev?.id && typeof prev.id === "string") { + // Only store the first occurrence for each ID (handle duplicates deterministically) + if (!previousById.has(prev.id)) { + previousById.set(prev.id, prev) + } + } } - return (nextTodos ?? []).map((next) => { - const candidates = previousByContent.get(next.content) - const matchedPrev = candidates?.shift() - if (!matchedPrev) return next + // Track which previous todos have been used (by their index) to avoid double-matching + const usedPreviousIndices = new Set() - return { - ...next, - subtaskId: next.subtaskId ?? matchedPrev.subtaskId, - tokens: next.tokens ?? matchedPrev.tokens, - cost: next.cost ?? matchedPrev.cost, + // Build content -> queue mapping for fallback (content-based matching) + // Each queue entry includes the original index for tracking + const previousByContent = new Map>() + for (let i = 0; i < safePrevious.length; i++) { + const prev = safePrevious[i] + if (!prev || typeof prev.content !== "string") continue + const list = previousByContent.get(prev.content) + if (list) list.push({ todo: prev, index: i }) + else previousByContent.set(prev.content, [{ todo: prev, index: i }]) + } + + return safeNext.map((next) => { + if (!next) return next + + let matchedPrev: TodoItem | undefined = undefined + let matchedIndex: number | undefined = undefined + + // Strategy 1: Try ID-based matching first (most reliable) + if (next.id && typeof next.id === "string") { + const byId = previousById.get(next.id) + if (byId) { + // Find the index of this todo in the original array + const idx = safePrevious.findIndex((p) => p === byId) + if (idx !== -1 && !usedPreviousIndices.has(idx)) { + matchedPrev = byId + matchedIndex = idx + } + } } + + // Strategy 2: Fall back to content-based matching if ID didn't match + if (!matchedPrev && typeof next.content === "string") { + const candidates = previousByContent.get(next.content) + if (candidates) { + // Find first unused candidate + for (const candidate of candidates) { + if (!usedPreviousIndices.has(candidate.index)) { + matchedPrev = candidate.todo + matchedIndex = candidate.index + break + } + } + } + } + + // Mark as used and apply metadata + if (matchedPrev && matchedIndex !== undefined) { + usedPreviousIndices.add(matchedIndex) + return { + ...next, + subtaskId: next.subtaskId ?? matchedPrev.subtaskId, + tokens: next.tokens ?? matchedPrev.tokens, + cost: next.cost ?? matchedPrev.cost, + } + } + + return next }) } diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 9acb26211a..77dfeb86d1 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -523,7 +523,6 @@ export class ClineProvider // Create timeout for automatic cleanup const timeoutId = setTimeout(() => { this.clearPendingEditOperation(operationId) - this.log(`[setPendingEditOperation] Automatically cleared stale pending operation: ${operationId}`) }, ClineProvider.PENDING_OPERATION_TIMEOUT_MS) // Store the operation @@ -532,8 +531,6 @@ export class ClineProvider timeoutId, createdAt: Date.now(), }) - - this.log(`[setPendingEditOperation] Set pending operation: ${operationId}`) } /** @@ -551,7 +548,6 @@ export class ClineProvider if (operation) { clearTimeout(operation.timeoutId) this.pendingOperations.delete(operationId) - this.log(`[clearPendingEditOperation] Cleared pending operation: ${operationId}`) return true } return false @@ -565,7 +561,6 @@ export class ClineProvider clearTimeout(operation.timeoutId) } this.pendingOperations.clear() - this.log(`[clearAllPendingEditOperations] Cleared all pending operations`) } /* @@ -583,22 +578,16 @@ export class ClineProvider } async dispose() { - this.log("Disposing ClineProvider...") - // Clear all tasks from the stack. while (this.clineStack.length > 0) { await this.removeClineFromStack() } - this.log("Cleared all tasks") - // Clear all pending edit operations to prevent memory leaks this.clearAllPendingEditOperations() - this.log("Cleared pending operations") if (this.view && "dispose" in this.view) { this.view.dispose() - this.log("Disposed webview") } this.clearWebviewResources() @@ -624,7 +613,6 @@ export class ClineProvider this.skillsManager = undefined this.marketplaceManager?.cleanup() this.customModesManager?.dispose() - this.log("Disposed all disposables") ClineProvider.activeInstances.delete(this) // Clean up any event listeners attached to this provider @@ -844,10 +832,8 @@ export class ClineProvider webviewView.onDidDispose( async () => { if (inTabMode) { - this.log("Disposing ClineProvider instance for tab view") await this.dispose() } else { - this.log("Clearing webview resources for sidebar view") this.clearWebviewResources() // Reset current workspace manager reference when view is disposed this.codeIndexManager = undefined @@ -1032,16 +1018,8 @@ export class ClineProvider // Perform preparation tasks and set up event listeners await this.performPreparationTasks(task) - - this.log( - `[createTaskWithHistoryItem] rehydrated task ${task.taskId}.${task.instanceId} in-place (flicker-free)`, - ) } else { await this.addClineToStack(task) - - this.log( - `[createTaskWithHistoryItem] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`, - ) } // Check if there's a pending edit after checkpoint restoration @@ -1050,8 +1028,6 @@ export class ClineProvider if (pendingEdit) { this.clearPendingEditOperation(operationId) // Clear the pending edit - this.log(`[createTaskWithHistoryItem] Processing pending edit after checkpoint restoration`) - // Process the pending edit after a short delay to ensure the task is fully initialized setTimeout(async () => { try { @@ -2887,10 +2863,6 @@ export class ClineProvider await this.addClineToStack(task) - this.log( - `[createTask] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`, - ) - return task } @@ -2901,8 +2873,6 @@ export class ClineProvider return } - console.log(`[cancelTask] cancelling task ${task.taskId}.${task.instanceId}`) - const { historyItem, uiMessagesFilePath } = await this.getTaskWithId(task.taskId) // Preserve parent and root task information for history item. @@ -2969,8 +2939,6 @@ export class ClineProvider // This is used when the user cancels a task that is not a subtask. public async clearTask(): Promise { if (this.clineStack.length > 0) { - const task = this.clineStack[this.clineStack.length - 1] - console.log(`[clearTask] clearing task ${task.taskId}.${task.instanceId}`) await this.removeClineFromStack() } } @@ -3202,59 +3170,69 @@ export class ClineProvider // 4.5) Direct todo-subtask linking: set todo.subtaskId = childTaskId at delegation-time // Persist by appending an updateTodoList message to the parent's message history. + // Uses deterministic anchor selection: in_progress > pending > last completed > synthetic anchor. try { const globalStoragePath = this.contextProxy.globalStorageUri.fsPath const parentMessages = await readTaskMessages({ taskId: parentTaskId, globalStoragePath }) - const todos = getLatestTodo(parentMessages) as unknown as TodoItem[] + let todos = (getLatestTodo(parentMessages) as unknown as TodoItem[]) ?? [] + + // Ensure todos is a valid array + if (!Array.isArray(todos)) { + todos = [] + } + + // Deterministic selection algorithm: + // 1. First in_progress todo + // 2. Else first pending todo + // 3. Else last completed todo (closest to delegation moment) + // 4. Else create a synthetic anchor todo + let chosen: TodoItem | undefined = undefined const inProgress = todos.filter((t) => t?.status === "in_progress") const pending = todos.filter((t) => t?.status === "pending") + const completed = todos.filter((t) => t?.status === "completed") - // Deterministic selection rule (in_progress > pending): pick the first matching item - // in the list order, even if multiple candidates exist. - const chosen: TodoItem | undefined = inProgress[0] ?? pending[0] - if (!chosen) { - this.log( - `[delegateParentAndOpenChild] Not linking subtask ${child.taskId}: no in_progress or pending todos found`, - ) + if (inProgress.length > 0) { + chosen = inProgress[0] + } else if (pending.length > 0) { + chosen = pending[0] + } else if (completed.length > 0) { + // Pick the LAST completed todo (closest stable anchor to delegation moment) + chosen = completed[completed.length - 1] } else { - // Log ambiguity (but still link deterministically). - if (inProgress.length > 1) { - this.log( - `[delegateParentAndOpenChild] Multiple in_progress todos (${inProgress.length}); linking first to subtask ${child.taskId}`, - ) - } else if (pending.length > 1 && inProgress.length === 0) { - this.log( - `[delegateParentAndOpenChild] Multiple pending todos (${pending.length}); linking first to subtask ${child.taskId}`, - ) + // No todos exist: append a synthetic anchor todo + const syntheticTodo: TodoItem = { + id: `synthetic-${child.taskId}`, + content: "Delegated to subtask", + status: "completed", + subtaskId: child.taskId, } + todos.push(syntheticTodo) + chosen = syntheticTodo } - if (chosen) { - if (chosen.subtaskId && chosen.subtaskId !== child.taskId) { - this.log( - `[delegateParentAndOpenChild] Overwriting existing todo.subtaskId '${chosen.subtaskId}' -> '${child.taskId}'`, - ) - } + // Set the subtaskId on the chosen todo (unless it's the synthetic one we just created) + if (chosen && !chosen.subtaskId) { chosen.subtaskId = child.taskId - - await saveTaskMessages({ - messages: [ - ...parentMessages, - { - ts: Date.now(), - type: "say", - say: "user_edit_todos", - text: JSON.stringify({ - tool: "updateTodoList", - todos, - }), - }, - ], - taskId: parentTaskId, - globalStoragePath, - }) } + + // Always persist the updated todo list + await saveTaskMessages({ + messages: [ + ...parentMessages, + { + ts: Date.now(), + type: "say", + say: "user_edit_todos", + text: JSON.stringify({ + tool: "updateTodoList", + todos, + }), + }, + ], + taskId: parentTaskId, + globalStoragePath, + }) } catch (error) { this.log( `[delegateParentAndOpenChild] Failed to persist delegation-time todo link (non-fatal): ${ @@ -3356,15 +3334,42 @@ export class ClineProvider parentClineMessages.push(subtaskUiMessage) // 2.5) Persist provider completion write-back: update parent's todo item with tokens/cost. + // Primary: find todo where t.subtaskId === childTaskId. + // Fallback: if not found BUT this parent/child relationship is valid (from historyItem), + // pick the same deterministic anchor todo and set its subtaskId = childTaskId before writing tokens/cost. try { - const todos = getLatestTodo(parentClineMessages) as unknown as TodoItem[] - if (Array.isArray(todos) && todos.length > 0) { - const linkedTodo = todos.find((t) => t?.subtaskId === childTaskId) - if (!linkedTodo) { - this.log( - `[reopenParentFromDelegation] No todo found with subtaskId === ${childTaskId}; skipping cost write-back`, - ) - } else { + let todos = (getLatestTodo(parentClineMessages) as unknown as TodoItem[]) ?? [] + if (!Array.isArray(todos)) { + todos = [] + } + + if (todos.length > 0) { + // Primary lookup by subtaskId + let linkedTodo = todos.find((t) => t?.subtaskId === childTaskId) + + // Fallback: if subtaskId link wasn't found but parent history confirms this child belongs to it, + // use the deterministic anchor selection to establish the link now. + if (!linkedTodo && historyItem.childIds?.includes(childTaskId)) { + const inProgress = todos.filter((t) => t?.status === "in_progress") + const pending = todos.filter((t) => t?.status === "pending") + const completed = todos.filter((t) => t?.status === "completed") + + if (inProgress.length > 0) { + linkedTodo = inProgress[0] + } else if (pending.length > 0) { + linkedTodo = pending[0] + } else if (completed.length > 0) { + // Pick the LAST completed todo (same as delegation-time logic) + linkedTodo = completed[completed.length - 1] + } + + // Set the subtaskId on the fallback anchor if found + if (linkedTodo) { + linkedTodo.subtaskId = childTaskId + } + } + + if (linkedTodo) { linkedTodo.tokens = (childHistoryItem?.tokensIn || 0) + (childHistoryItem?.tokensOut || 0) linkedTodo.cost = childHistoryItem?.totalCost || 0