diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index 30d3852b0a..ebfd94324e 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -11,6 +11,8 @@ export const SUBTASK_API_HANG_PARENT_MARKER = "SUBTASK_PARENT_API_HANG_INTERRUPT export const SUBTASK_API_HANG_CHILD_MARKER = "SUBTASK_CHILD_API_HANG_INTERRUPT_RESUME" const SUBTASK_FAST_PARENT_MARKER = "SUBTASK_PARENT_IMMEDIATE_COMPLETION" const SUBTASK_FAST_CHILD_MARKER = "SUBTASK_CHILD_IMMEDIATE_COMPLETION" +const SUBTASK_APPROVAL_RESTORE_PARENT_MARKER = "SUBTASK_PARENT_APPROVAL_RESTORE" +const SUBTASK_APPROVAL_RESTORE_CHILD_MARKER = "SUBTASK_CHILD_APPROVAL_RESTORE" const SUBTASK_XPROFILE_PARENT_MARKER = "SUBTASK_PARENT_CROSS_PROFILE" const SUBTASK_XPROFILE_SAME_CHILD_MARKER = "SUBTASK_CHILD_SAME_PROFILE" const SUBTASK_XPROFILE_DIFFERENT_CHILD_MARKER = "SUBTASK_CHILD_DIFFERENT_PROFILE" @@ -21,6 +23,9 @@ export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9" export const SUBTASK_FAST_CHILD_RESULT = "Fast child completed" const SUBTASK_FAST_CHILD_PROMPT = `${SUBTASK_FAST_CHILD_MARKER}: Complete immediately with the exact result "${SUBTASK_FAST_CHILD_RESULT}".` export const SUBTASK_FAST_PARENT_PROMPT = `${SUBTASK_FAST_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_FAST_CHILD_PROMPT}" Do not answer directly.` +export const SUBTASK_APPROVAL_RESTORE_CHILD_RESULT = "Restored child completed" +const SUBTASK_APPROVAL_RESTORE_CHILD_PROMPT = `${SUBTASK_APPROVAL_RESTORE_CHILD_MARKER}: Complete immediately with the exact result "${SUBTASK_APPROVAL_RESTORE_CHILD_RESULT}".` +export const SUBTASK_APPROVAL_RESTORE_PARENT_PROMPT = `${SUBTASK_APPROVAL_RESTORE_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_APPROVAL_RESTORE_CHILD_PROMPT}" Do not answer directly.` const SUBTASK_INTERRUPT_CHILD_PROMPT = `${SUBTASK_INTERRUPT_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` export const SUBTASK_INTERRUPT_PARENT_PROMPT = `${SUBTASK_INTERRUPT_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_INTERRUPT_CHILD_PROMPT}" Do not answer directly. When the subtask returns, complete with the exact result "Interrupted parent resumed".` @@ -122,6 +127,58 @@ const completionAfterAnswer = (followupId: string, completionId: string) => ({ }) export function addSubtaskFixtures(mock: InstanceType) { + mock.addFixture({ + match: { + userMessage: new RegExp(SUBTASK_APPROVAL_RESTORE_PARENT_MARKER), + sequenceIndex: 0, + }, + response: { + toolCalls: [ + { + name: "new_task", + arguments: JSON.stringify({ + mode: "ask", + message: SUBTASK_APPROVAL_RESTORE_CHILD_PROMPT, + }), + id: "call_subtasks_approval_restore_new_task_001", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + lastUserMessageContains(req, SUBTASK_APPROVAL_RESTORE_CHILD_MARKER) && + !requestContains(req, [SUBTASK_APPROVAL_RESTORE_PARENT_MARKER]), + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: SUBTASK_APPROVAL_RESTORE_CHILD_RESULT }), + id: "call_subtasks_approval_restore_completion_002", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + requestContains(req, [SUBTASK_APPROVAL_RESTORE_PARENT_MARKER, SUBTASK_RESULT_INJECTION]), + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: "Restored parent resumed" }), + id: "call_subtasks_approval_restore_parent_completion_003", + }, + ], + }, + }) + mock.addFixture({ match: { userMessage: new RegExp(SUBTASK_FAST_PARENT_MARKER), diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index 02d3dfe487..4a0e209895 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -9,6 +9,7 @@ import { SCHED_COMPLETED_RESULT, SCHED_STANDALONE_FOLLOWUP_ANSWER, SCHED_STANDALONE_PROMPT, + SUBTASK_APPROVAL_RESTORE_PARENT_PROMPT, SUBTASK_ABANDON_CHILD_FOLLOWUP_ANSWER, SUBTASK_ABANDON_PARENT_PROMPT, SUBTASK_API_HANG_CHILD_MARKER, @@ -174,6 +175,90 @@ suite("Roo Code Subtasks", function () { } }) + test("pending subtask approvals survive leave and return", async () => { + const api = globalThis.api + const asks: Record = {} + + const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { + if (message.type === "ask") { + asks[taskId] = asks[taskId] || [] + asks[taskId].push(message) + } + } + const hasToolAsk = (taskId: string, tool: "newTask" | "finishTask", after = 0) => + asks[taskId]?.slice(after).some((message) => { + if (message.ask !== "tool" || !message.text) return false + try { + return JSON.parse(message.text).tool === tool + } catch { + return false + } + }) ?? false + + api.on(RooCodeEventName.Message, messageHandler) + + try { + const parentTaskId = await api.startNewTask({ + configuration: { + mode: "ask", + alwaysAllowModeSwitch: true, + alwaysAllowSubtasks: false, + autoApprovalEnabled: false, + enableCheckpoints: false, + }, + text: SUBTASK_APPROVAL_RESTORE_PARENT_PROMPT, + }) + + await waitFor(() => hasToolAsk(parentTaskId, "newTask")) + const parentAskCount = asks[parentTaskId]?.length ?? 0 + + await api.clearCurrentTask() + api.resumeTask(parentTaskId) + await waitFor(() => hasToolAsk(parentTaskId, "newTask", parentAskCount)) + assert.ok( + !asks[parentTaskId]?.slice(parentAskCount).some(({ ask }) => ask === "resume_task"), + "Restored parent should present newTask approval instead of Continue", + ) + await api.approveCurrentAsk() + + let childTaskId: string | undefined + await waitFor(() => { + const current = api.getCurrentTaskStack().at(-1) + if (current && current !== parentTaskId) { + childTaskId = current + return true + } + return false + }) + + await waitFor(() => hasToolAsk(childTaskId!, "finishTask")) + const childAskCount = asks[childTaskId!]?.length ?? 0 + + await api.clearCurrentTask() + api.resumeTask(childTaskId!) + await waitFor(() => hasToolAsk(childTaskId!, "finishTask", childAskCount)) + assert.ok( + !asks[childTaskId!] + ?.slice(childAskCount) + .some(({ ask }) => ask === "resume_task" || ask === "resume_completed_task"), + "Restored child should present finishTask approval instead of a generic resume action", + ) + + await api.approveCurrentAsk() + await waitFor(() => api.getCurrentTaskStack().at(-1) === parentTaskId) + const childHistory = await api.getTaskHistoryItem(childTaskId!) + const parentHistory = await api.getTaskHistoryItem(parentTaskId) + assert.strictEqual(childHistory?.status, "completed") + assert.notStrictEqual(parentHistory?.status, "delegated") + assert.strictEqual(parentHistory?.awaitingChildId, undefined) + } finally { + api.off(RooCodeEventName.Message, messageHandler) + while (api.getCurrentTaskStack().length > 0) { + await api.clearCurrentTask() + } + } + }) + // Smoke: child completing normally must resume the parent task. test("child task returns to parent after normal completion", async () => { const api = globalThis.api diff --git a/packages/types/src/__tests__/history.test.ts b/packages/types/src/__tests__/history.test.ts new file mode 100644 index 0000000000..75fcf1d86c --- /dev/null +++ b/packages/types/src/__tests__/history.test.ts @@ -0,0 +1,46 @@ +import { historyItemSchema, pendingTaskActionSchema } from "../history.js" + +describe("pendingTaskActionSchema", () => { + it("accepts create and finish subtask actions", () => { + expect( + pendingTaskActionSchema.parse({ + kind: "create_subtask", + actionId: "create-1", + approvalText: "{}", + mode: "ask", + message: "Child", + todos: [], + }), + ).toMatchObject({ kind: "create_subtask", actionId: "create-1" }) + expect( + pendingTaskActionSchema.parse({ + kind: "finish_subtask", + actionId: "finish-1", + approvalText: "{}", + parentTaskId: "parent-1", + result: "Done", + }), + ).toMatchObject({ kind: "finish_subtask", actionId: "finish-1" }) + }) + + it("round-trips pending actions on history items", () => { + const parsed = historyItemSchema.parse({ + id: "task-1", + number: 1, + ts: 1, + task: "Task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + pendingAction: { + kind: "finish_subtask", + actionId: "finish-1", + approvalText: "{}", + parentTaskId: "parent-1", + result: "Done", + }, + }) + + expect(parsed.pendingAction?.actionId).toBe("finish-1") + }) +}) diff --git a/packages/types/src/history.ts b/packages/types/src/history.ts index 5b173c6a6b..5d9671842e 100644 --- a/packages/types/src/history.ts +++ b/packages/types/src/history.ts @@ -1,9 +1,31 @@ import { z } from "zod" +import { todoItemSchema } from "./todo.js" + /** * HistoryItem */ +export const pendingTaskActionSchema = z.discriminatedUnion("kind", [ + z.object({ + kind: z.literal("create_subtask"), + actionId: z.string(), + approvalText: z.string(), + mode: z.string(), + message: z.string(), + todos: z.array(todoItemSchema), + }), + z.object({ + kind: z.literal("finish_subtask"), + actionId: z.string(), + approvalText: z.string(), + parentTaskId: z.string(), + result: z.string(), + }), +]) + +export type PendingTaskAction = z.infer + export const historyItemSchema = z.object({ id: z.string(), rootTaskId: z.string().optional(), @@ -26,6 +48,7 @@ export const historyItemSchema = z.object({ 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 + pendingAction: pendingTaskActionSchema.optional(), }) export type HistoryItem = z.infer diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index 24ea04dd8e..bacadd0913 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -92,6 +92,51 @@ describe("History resume delegation - parent metadata transitions", () => { vi.clearAllMocks() }) + it("rejects a stale restored completion action before changing parent or child state", async () => { + const parentHistoryItem = { + id: "parent-1", + status: "delegated", + awaitingChildId: "child-1", + ts: Date.now(), + task: "Parent task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const childHistoryItem = { + id: "child-1", + status: "interrupted", + pendingAction: { + kind: "finish_subtask", + actionId: "current-action", + approvalText: JSON.stringify({ tool: "finishTask" }), + parentTaskId: "parent-1", + result: "Done", + }, + } + const taskHistoryStore = makeTaskHistoryStoreStub(childHistoryItem, parentHistoryItem) + const removeClineFromStack = vi.fn() + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentHistoryItem }), + getCurrentTask: vi.fn(() => ({ taskId: "child-1" })), + removeClineFromStack, + taskHistoryStore, + log: vi.fn(), + }) + + const result = await ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-1", + childTaskId: "child-1", + completionResultSummary: "Done", + pendingActionId: "stale-action", + }) + + expect(result).toBe(false) + expect(taskHistoryStore.atomicUpdatePair).not.toHaveBeenCalled() + expect(removeClineFromStack).not.toHaveBeenCalled() + }) + it("reopenParentFromDelegation accepts an active parent awaiting the returning child", async () => { const providerEmit = vi.fn() const parentHistoryItem = { @@ -110,7 +155,20 @@ describe("History resume delegation - parent metadata transitions", () => { } const getTaskWithId = vi.fn().mockResolvedValue({ historyItem: parentHistoryItem }) - const taskHistoryStore = makeTaskHistoryStoreStub({ id: "child-1", status: "active" }, parentHistoryItem) + const taskHistoryStore = makeTaskHistoryStoreStub( + { + id: "child-1", + status: "active", + pendingAction: { + kind: "finish_subtask", + actionId: "finish-action", + approvalText: JSON.stringify({ tool: "finishTask" }), + parentTaskId: "parent-1", + result: "Child done", + }, + }, + parentHistoryItem, + ) const removeClineFromStack = vi.fn().mockResolvedValue(undefined) const createTaskWithHistoryItem = vi.fn().mockResolvedValue({ taskId: "parent-1", @@ -135,6 +193,7 @@ describe("History resume delegation - parent metadata transitions", () => { parentTaskId: "parent-1", childTaskId: "child-1", completionResultSummary: "Child done", + pendingActionId: "finish-action", }) // atomicUpdatePair called with child first, parent second @@ -145,9 +204,20 @@ describe("History resume delegation - parent metadata transitions", () => { // Verify child updater produces completed status and persists completionResultSummary // so startup reconciliation has the real result if the parent write fails. - const updatedChild = firstUpdater({ id: "child-1", status: "active" } as HistoryItem) + const updatedChild = firstUpdater({ + id: "child-1", + status: "active", + pendingAction: { + kind: "finish_subtask", + actionId: "finish-action", + approvalText: "{}", + parentTaskId: "parent-1", + result: "Child done", + }, + } as HistoryItem) expect(updatedChild.status).toBe("completed") expect(updatedChild.completionResultSummary).toBe("Child done") + expect(updatedChild.pendingAction).toBeUndefined() // Verify parent updater produces active status with correct fields const updatedParent = secondUpdater(parentHistoryItem as HistoryItem) diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index 0154027753..8464e4417e 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -43,6 +43,92 @@ const makeParentTask = () => }) as any describe("ClineProvider.delegateParentAndOpenChild()", () => { + it("rejects a stale restored action before delegation side effects", async () => { + const parentTask = makeParentTask() + const removeClineFromStack = vi.fn() + const createTask = vi.fn() + const handleModeSwitch = vi.fn() + const taskHistoryStore = makeStoreStub({ + get: vi.fn().mockReturnValue({ + ...parentHistoryItem, + pendingAction: { + kind: "create_subtask", + actionId: "current-action", + approvalText: "{}", + mode: "code", + message: "Do something", + todos: [], + }, + }), + }) + const provider = { + getCurrentTask: vi.fn(() => parentTask), + removeClineFromStack, + createTask, + handleModeSwitch, + taskHistoryStore, + } as unknown as ClineProvider + + await expect( + ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + pendingActionId: "stale-action", + }), + ).rejects.toThrow("Pending action mismatch") + + expect(parentTask.flushPendingToolResultsToHistory).not.toHaveBeenCalled() + expect(removeClineFromStack).not.toHaveBeenCalled() + expect(handleModeSwitch).not.toHaveBeenCalled() + expect(createTask).not.toHaveBeenCalled() + expect(taskHistoryStore.atomicReadAndUpdate).not.toHaveBeenCalled() + }) + + it("clears a matching pending action when delegation commits", async () => { + const pendingAction = { + kind: "create_subtask" as const, + actionId: "create-action", + approvalText: "{}", + mode: "code", + message: "Do something", + todos: [], + } + let current: HistoryItem = { ...parentHistoryItem, status: "active", pendingAction } + const taskHistoryStore = { + get: vi.fn(() => current), + atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { + current = updater(current) + return [current] + }), + } + const parentTask = makeParentTask() + const child = { taskId: "child-1", run: vi.fn().mockResolvedValue(undefined) } + const provider = { + taskScheduler: new TaskScheduler(), + emit: vi.fn(), + getCurrentTask: vi.fn(() => parentTask), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockResolvedValue(child), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + log: vi.fn(), + isViewLaunched: false, + taskHistoryStore, + } as unknown as ClineProvider + + await ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + pendingActionId: "create-action", + }) + + expect(current.pendingAction).toBeUndefined() + expect(current).toMatchObject({ status: "delegated", awaitingChildId: "child-1" }) + }) + it("persists parent delegation metadata via atomicReadAndUpdate and emits TaskDelegated", async () => { const providerEmit = vi.fn() const parentTask = makeParentTask() diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts index 819cfffcd9..ff204da7c4 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts @@ -2,6 +2,9 @@ import { describe, it, expect, beforeEach, vi } from "vitest" import { presentAssistantMessage } from "../presentAssistantMessage" +import { isValidToolName } from "../../tools/validateToolUse" + +const mockNewTaskHandle = vi.hoisted(() => vi.fn()) // Mock dependencies vi.mock("../../task/Task") @@ -9,6 +12,9 @@ vi.mock("../../tools/validateToolUse", () => ({ validateToolUse: vi.fn(), isValidToolName: vi.fn(() => false), })) +vi.mock("../../tools/NewTaskTool", () => ({ + newTaskTool: { handle: mockNewTaskHandle }, +})) vi.mock("@roo-code/telemetry", () => ({ TelemetryService: { instance: { @@ -22,6 +28,7 @@ describe("presentAssistantMessage - Unknown Tool Handling", () => { let mockTask: any beforeEach(() => { + mockNewTaskHandle.mockReset() // Create a mock Task with minimal properties needed for testing mockTask = { taskId: "test-task-id", @@ -241,4 +248,83 @@ describe("presentAssistantMessage - Unknown Tool Handling", () => { expect(toolResult.is_error).toBe(true) expect(toolResult.content).toContain("due to user rejecting a previous tool") }) + + it("persists and acknowledges queued feedback through the native approval path", async () => { + vi.mocked(isValidToolName).mockReturnValue(true) + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "call_new_task_queued_feedback", + name: "new_task", + params: { mode: "ask", message: "Child task" }, + nativeArgs: { mode: "ask", message: "Child task" }, + partial: false, + }, + ] + mockTask.currentStreamingDidCheckpoint = false + mockTask.checkpointSave = vi.fn().mockResolvedValue(undefined) + mockTask.ask = vi.fn().mockResolvedValue({ + response: "messageResponse", + text: "Handle this first", + queuedMessageId: "queued-message-1", + }) + mockTask.persistQueuedFeedbackAndAcknowledge = vi.fn().mockResolvedValue(true) + mockNewTaskHandle.mockImplementation( + async ( + _task: unknown, + _block: unknown, + callbacks: { askApproval: (type: "tool", text: string) => Promise }, + ) => { + await callbacks.askApproval("tool", JSON.stringify({ tool: "newTask" })) + }, + ) + + await presentAssistantMessage(mockTask) + + expect(mockTask.persistQueuedFeedbackAndAcknowledge).toHaveBeenCalledWith( + "queued-message-1", + "Handle this first", + undefined, + ) + }) + + it("merges ordinary approval feedback into a native tool result", async () => { + vi.mocked(isValidToolName).mockReturnValue(true) + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "call_new_task_approval_feedback", + name: "new_task", + params: { mode: "ask", message: "Child task" }, + nativeArgs: { mode: "ask", message: "Child task" }, + partial: false, + }, + ] + mockTask.currentStreamingDidCheckpoint = false + mockTask.checkpointSave = vi.fn().mockResolvedValue(undefined) + mockTask.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: "Approved context" }) + mockNewTaskHandle.mockImplementation( + async ( + _task: unknown, + _block: unknown, + callbacks: { + askApproval: (type: "tool", text: string) => Promise + pushToolResult: (content: string) => void + }, + ) => { + expect(await callbacks.askApproval("tool", JSON.stringify({ tool: "newTask" }))).toBe(true) + callbacks.pushToolResult("Delegated") + }, + ) + + await presentAssistantMessage(mockTask) + + expect(mockTask.say).toHaveBeenCalledWith("user_feedback", "Approved context", undefined) + expect(mockTask.userMessageContent).toContainEqual( + expect.objectContaining({ + type: "tool_result", + content: expect.stringContaining('"status":"approved"'), + }), + ) + }) }) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 7383a7a35a..79a1888278 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -519,7 +519,7 @@ export async function presentAssistantMessage(cline: Task) { progressStatus?: ToolProgressStatus, isProtected?: boolean, ) => { - const { response, text, images } = await cline.ask( + const { response, text, images, queuedMessageId } = await cline.ask( type, partialMessage, false, @@ -529,8 +529,12 @@ export async function presentAssistantMessage(cline: Task) { if (response !== "yesButtonClicked") { // Handle both messageResponse and noButtonClicked with text. - if (text) { - await cline.say("user_feedback", text, images) + if (text || images?.length) { + if (queuedMessageId) { + await cline.persistQueuedFeedbackAndAcknowledge(queuedMessageId, text, images) + } else { + await cline.say("user_feedback", text ?? "", images) + } pushToolResult(formatResponse.toolResult(formatResponse.toolDeniedWithFeedback(text), images)) } else { pushToolResult(formatResponse.toolDenied()) @@ -542,9 +546,13 @@ export async function presentAssistantMessage(cline: Task) { // Store approval feedback to be merged into tool result (GitHub #10465) // Don't push it as a separate tool_result here - that would create duplicates. // The tool will call pushToolResult, which will merge the feedback into the actual result. - if (text) { - await cline.say("user_feedback", text, images) - approvalFeedback = { text, images } + if (text || images?.length) { + if (queuedMessageId) { + await cline.persistQueuedFeedbackAndAcknowledge(queuedMessageId, text, images) + } else { + await cline.say("user_feedback", text ?? "", images) + } + approvalFeedback = { text: text ?? "", images } } return true @@ -848,6 +856,7 @@ export async function presentAssistantMessage(cline: Task) { pushToolResult, askFinishSubTaskApproval, toolDescription, + toolCallId: block.id, } await attemptCompletionTool.handle( cline, diff --git a/src/core/message-queue/MessageQueueService.ts b/src/core/message-queue/MessageQueueService.ts index fe38bf0194..cc7c35978d 100644 --- a/src/core/message-queue/MessageQueueService.ts +++ b/src/core/message-queue/MessageQueueService.ts @@ -16,6 +16,7 @@ export interface QueueEvents { export class MessageQueueService extends EventEmitter { private _messages: QueuedMessage[] + private claimedMessageIds = new Set() constructor() { super() @@ -59,6 +60,7 @@ export class MessageQueueService extends EventEmitter { } this._messages.splice(index, 1) + this.claimedMessageIds.delete(id) this.emit("stateChanged", this._messages) return true } @@ -78,11 +80,23 @@ export class MessageQueueService extends EventEmitter { } public dequeueMessage(): QueuedMessage | undefined { - const message = this._messages.shift() + const index = this._messages.findIndex((message) => !this.claimedMessageIds.has(message.id)) + if (index === -1) { + return undefined + } + const [message] = this._messages.splice(index, 1) this.emit("stateChanged", this._messages) return message } + public claimNextMessage(): QueuedMessage | undefined { + const message = this._messages.find((candidate) => !this.claimedMessageIds.has(candidate.id)) + if (message) { + this.claimedMessageIds.add(message.id) + } + return message + } + public get messages(): QueuedMessage[] { return this._messages } @@ -93,6 +107,7 @@ export class MessageQueueService extends EventEmitter { public dispose(): void { this._messages = [] + this.claimedMessageIds.clear() this.removeAllListeners() } } diff --git a/src/core/message-queue/__tests__/MessageQueueService.spec.ts b/src/core/message-queue/__tests__/MessageQueueService.spec.ts new file mode 100644 index 0000000000..ed5f4e4aae --- /dev/null +++ b/src/core/message-queue/__tests__/MessageQueueService.spec.ts @@ -0,0 +1,29 @@ +import { MessageQueueService } from "../MessageQueueService" + +describe("MessageQueueService claims", () => { + it("keeps claimed messages unavailable while later messages remain consumable", () => { + const queue = new MessageQueueService() + const first = queue.addMessage("first")! + const second = queue.addMessage("second")! + + expect(queue.claimNextMessage()).toEqual(first) + expect(queue.claimNextMessage()).toEqual(second) + expect(queue.dequeueMessage()).toBeUndefined() + expect(queue.messages).toEqual([first, second]) + }) + + it("clears claim state when a message is removed or the queue is disposed", () => { + const queue = new MessageQueueService() + const message = queue.addMessage("feedback")! + expect(queue.claimNextMessage()).toEqual(message) + + expect(queue.removeMessage(message.id)).toBe(true) + expect(queue.removeMessage(message.id)).toBe(false) + + const next = queue.addMessage("next")! + expect(queue.claimNextMessage()).toEqual(next) + queue.dispose() + expect(queue.messages).toEqual([]) + expect(queue.claimNextMessage()).toBeUndefined() + }) +}) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 4be087394e..6660d65c7a 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -32,6 +32,7 @@ import { type ClineAsk, type ToolProgressStatus, type HistoryItem, + type PendingTaskAction, type CreateTaskOptions, type ModelInfo, type ClineApiReqCancelReason, @@ -139,6 +140,33 @@ import { shouldAddUserMessageToHistory } from "./messageCounting" const MAX_EXPONENTIAL_BACKOFF_SECONDS = 600 // 10 minutes const DEFAULT_USAGE_COLLECTION_TIMEOUT_MS = 5000 // 5 seconds + +type QueuedAskResolution = { response: ClineAskResponse; requiresDurableAck: boolean } + +function queuedResponseForAsk(type: ClineAsk, text?: string): QueuedAskResolution | undefined { + if (type === "command_output") { + return undefined + } + + if (type === "tool") { + try { + const tool = JSON.parse(text || "{}") as { tool?: string } + if (tool.tool === "newTask" || tool.tool === "finishTask") { + return { response: "messageResponse", requiresDurableAck: true } + } + } catch { + // Malformed tool asks retain the existing approve-with-feedback behavior. + } + + return { response: "yesButtonClicked", requiresDurableAck: false } + } + if (type === "command" || type === "use_mcp_server") { + return { response: "yesButtonClicked", requiresDurableAck: false } + } + + return { response: "messageResponse", requiresDurableAck: type === "completion_result" } +} + const FORCED_CONTEXT_REDUCTION_PERCENT = 75 // Keep 75% of context (remove 25%) on context window errors const MAX_CONTEXT_WINDOW_RETRIES = 3 // Maximum retries for context window errors @@ -454,6 +482,7 @@ export class Task extends EventEmitter implements TaskLike { // Initial status for the task's history item (set at creation time to avoid race conditions) private readonly initialStatus?: "active" | "delegated" | "completed" | "interrupted" + private pendingAction?: PendingTaskAction // MessageManager for high-level message operations (lazy initialized) private _messageManager?: MessageManager @@ -542,6 +571,7 @@ export class Task extends EventEmitter implements TaskLike { this.parentTask = parentTask this.taskNumber = taskNumber this.initialStatus = initialStatus + this.pendingAction = historyItem?.pendingAction // 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 @@ -861,6 +891,34 @@ export class Task extends EventEmitter implements TaskLike { this._taskApiConfigName = apiConfigName } + public setPendingTaskAction(pendingAction: PendingTaskAction): void { + this.pendingAction = pendingAction + } + + public async persistQueuedFeedbackAndAcknowledge( + messageId: string, + text?: string, + images?: string[], + ): Promise { + await this.say("user_feedback", text ?? "", images) + while (!this.abort) { + if (await this.saveClineMessages()) { + return this.messageQueueService.removeMessage(messageId) + } + await delay(250) + } + return false + } + + private handleQueuedAskResponse(message: QueuedMessage, resolution: QueuedAskResolution): string | undefined { + this.handleWebviewAskResponse(resolution.response, message.text, message.images) + if (resolution.requiresDurableAck) { + return message.id + } + this.messageQueueService.removeMessage(message.id) + return undefined + } + static create(options: TaskOptions): [Task, Promise] { const instance = new Task({ ...options, startTask: false }) const { images, task, historyItem } = options @@ -886,6 +944,13 @@ export class Task extends EventEmitter implements TaskLike { } private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string) { + const resolvesPendingAction = + this.pendingAction && + message.role === "user" && + Array.isArray(message.content) && + message.content.some( + (block) => block.type === "tool_result" && block.tool_use_id === this.pendingAction?.actionId, + ) this.apiConversationHistory.push( prepareApiConversationMessage({ message, @@ -896,7 +961,22 @@ export class Task extends EventEmitter implements TaskLike { }), ) - await this.saveApiConversationHistory() + const saved = await this.saveApiConversationHistory() + if (saved && resolvesPendingAction && this.pendingAction) { + try { + const cleared = await this.providerRef + .deref() + ?.clearPendingTaskAction(this.taskId, this.pendingAction.actionId) + if (cleared) { + this.pendingAction = undefined + } + } catch (error) { + console.error( + `[Task#addToApiConversationHistory] Failed to clear pending action for ${this.taskId}:`, + error, + ) + } + } } // NOTE: We intentionally do NOT mutate stored messages to merge consecutive user turns. @@ -1159,7 +1239,7 @@ export class Task extends EventEmitter implements TaskLike { partial?: boolean, progressStatus?: ToolProgressStatus, isProtected?: boolean, - ): Promise<{ response: ClineAskResponse; text?: string; images?: string[] }> { + ): Promise<{ response: ClineAskResponse; text?: string; images?: string[]; queuedMessageId?: string }> { // If this Cline instance was aborted by the provider, then the only // thing keeping us alive is a promise still running in the background, // in which case we don't want to send its result to the webview as it @@ -1182,7 +1262,12 @@ export class Task extends EventEmitter implements TaskLike { // rendered, leaving them stuck on-screen). const provider = this.providerRef.deref() const state = provider ? await provider.getState() : undefined - const approval = await checkAutoApproval({ state, ask: type, text, isProtected }) + const queuedMessage = + partial === true || type === "command_output" ? undefined : this.messageQueueService.claimNextMessage() + const queuedAskResolution = queuedMessage ? queuedResponseForAsk(type, text) : undefined + const approval = queuedAskResolution + ? ({ decision: "ask" } as const) + : await checkAutoApproval({ state, ask: type, text, isProtected }) const isAutoAnswered = approval.decision === "approve" || approval.decision === "deny" const autoApprovalDecision = isAutoAnswered ? approval.decision : undefined @@ -1317,6 +1402,7 @@ export class Task extends EventEmitter implements TaskLike { const shouldDrainQueuedMessageForAsk = type !== "command_output" const isStatusMutable = !partial && isBlocking && !isMessageQueued && approval.decision === "ask" + let queuedMessageId: string | undefined if (isStatusMutable) { const statusMutationTimeout = 2_000 @@ -1358,21 +1444,8 @@ export class Task extends EventEmitter implements TaskLike { }, statusMutationTimeout), ) } - } else if (isMessageQueued && shouldDrainQueuedMessageForAsk) { - const message = this.messageQueueService.dequeueMessage() - - if (message) { - // Check if this is a tool approval ask that needs to be handled. - if (type === "tool" || type === "command" || type === "use_mcp_server") { - // For tool approvals, we need to approve first, then send - // the message if there's text/images. - this.handleWebviewAskResponse("yesButtonClicked", message.text, message.images) - } else { - // For other ask types (like followup or command_output), fulfill the ask - // directly. - this.handleWebviewAskResponse("messageResponse", message.text, message.images) - } - } + } else if (isMessageQueued && shouldDrainQueuedMessageForAsk && queuedMessage && queuedAskResolution) { + queuedMessageId = this.handleQueuedAskResponse(queuedMessage, queuedAskResolution) } // Wait for askResponse to be set @@ -1386,15 +1459,10 @@ export class Task extends EventEmitter implements TaskLike { // suggestion click that was incorrectly queued due to UI state), consume it // immediately so the task doesn't hang. if (shouldDrainQueuedMessageForAsk && !this.messageQueueService.isEmpty()) { - const message = this.messageQueueService.dequeueMessage() - if (message) { - // If this is a tool approval ask, we need to approve first (yesButtonClicked) - // and include any queued text/images. - if (type === "tool" || type === "command" || type === "use_mcp_server") { - this.handleWebviewAskResponse("yesButtonClicked", message.text, message.images) - } else { - this.handleWebviewAskResponse("messageResponse", message.text, message.images) - } + const message = this.messageQueueService.claimNextMessage() + const resolution = message ? queuedResponseForAsk(type, text) : undefined + if (message && resolution) { + queuedMessageId = this.handleQueuedAskResponse(message, resolution) } } @@ -1415,7 +1483,12 @@ export class Task extends EventEmitter implements TaskLike { throw new AskIgnoredError("superseded") } - const result = { response: this.askResponse!, text: this.askResponseText, images: this.askResponseImages } + const result = { + response: this.askResponse!, + text: this.askResponseText, + images: this.askResponseImages, + queuedMessageId, + } this.askResponse = undefined this.askResponseText = undefined this.askResponseImages = undefined @@ -2010,6 +2083,20 @@ export class Task extends EventEmitter implements TaskLike { } } + if (this.pendingAction) { + const pendingAskIndex = findLastIndex( + modifiedClineMessages, + (message) => + message.type === "ask" && + message.ask === "tool" && + message.isAnswered !== true && + message.text === this.pendingAction?.approvalText, + ) + if (pendingAskIndex !== -1) { + modifiedClineMessages.splice(pendingAskIndex, 1) + } + } + // Since we don't use `api_req_finished` anymore, we need to check if the // last `api_req_started` has a cost value, if it doesn't and no // cancellation reason to present, then we remove it since it indicates @@ -2038,6 +2125,27 @@ export class Task extends EventEmitter implements TaskLike { // This is important in case the user deletes messages without resuming // the task first. this.apiConversationHistory = await this.getSavedApiConversationHistory() + if ( + this.pendingAction && + this.apiConversationHistory.some( + (message) => + message.role === "user" && + Array.isArray(message.content) && + message.content.some( + (block) => + block.type === "tool_result" && block.tool_use_id === this.pendingAction?.actionId, + ), + ) + ) { + await this.providerRef.deref()?.clearPendingTaskAction(this.taskId, this.pendingAction.actionId) + this.pendingAction = undefined + } + + if (this.pendingAction) { + this.isInitialized = true + await this.resumePendingTaskAction(this.pendingAction) + return + } const lastClineMessage = this.clineMessages .slice() @@ -2219,6 +2327,53 @@ export class Task extends EventEmitter implements TaskLike { } } + private async resumePendingTaskAction(action: PendingTaskAction): Promise { + const provider = this.providerRef.deref() + if (!provider) { + throw new Error(`[Task#resumePendingTaskAction] Provider unavailable for task ${this.taskId}`) + } + + const { response, text, images, queuedMessageId } = await this.ask("tool", action.approvalText, false) + + if (response === "yesButtonClicked") { + if (action.kind === "create_subtask") { + await provider.delegateParentAndOpenChild({ + parentTaskId: this.taskId, + message: action.message, + initialTodos: action.todos, + mode: action.mode, + pendingActionId: action.actionId, + }) + return + } + + const didReopen = await provider.reopenParentFromDelegation({ + parentTaskId: action.parentTaskId, + childTaskId: this.taskId, + completionResultSummary: action.result, + pendingActionId: action.actionId, + }) + if (didReopen) { + return + } + } + + if (queuedMessageId) { + await this.persistQueuedFeedbackAndAcknowledge(queuedMessageId, text, images) + } else if (text || images?.length) { + await this.say("user_feedback", text ?? "", images) + } + + const deniedContent = text ? formatResponse.toolDeniedWithFeedback(text) : formatResponse.toolDenied() + await this.initiateTaskLoop([ + { + type: "tool_result", + tool_use_id: action.actionId, + content: formatResponse.toolResult(deniedContent, images), + }, + ]) + } + /** * Cancels the current HTTP request if one is in progress. * This immediately aborts the underlying stream rather than waiting for the next chunk. diff --git a/src/core/task/__tests__/Task.pending-action.spec.ts b/src/core/task/__tests__/Task.pending-action.spec.ts new file mode 100644 index 0000000000..e3c302ee86 --- /dev/null +++ b/src/core/task/__tests__/Task.pending-action.spec.ts @@ -0,0 +1,129 @@ +import type { PendingTaskAction } from "@roo-code/types" + +import { Task } from "../Task" + +type PendingActionAccess = { + resumePendingTaskAction(action: PendingTaskAction): Promise +} + +const getPendingActionAccess = (task: Task) => task as unknown as PendingActionAccess + +function createTask(provider?: object) { + const task = Object.create(Task.prototype) as Task + Object.assign(task, { + taskId: "task-1", + providerRef: { deref: () => provider }, + ask: vi.fn(), + say: vi.fn().mockResolvedValue(undefined), + initiateTaskLoop: vi.fn().mockResolvedValue(undefined), + persistQueuedFeedbackAndAcknowledge: vi.fn().mockResolvedValue(true), + }) + return task +} + +const createAction: PendingTaskAction = { + kind: "create_subtask", + actionId: "create-action", + approvalText: JSON.stringify({ tool: "newTask" }), + mode: "ask", + message: "Child task", + todos: [], +} + +const finishAction: PendingTaskAction = { + kind: "finish_subtask", + actionId: "finish-action", + approvalText: JSON.stringify({ tool: "finishTask" }), + parentTaskId: "parent-1", + result: "Done", +} + +describe("Task pending action replay", () => { + it("executes an approved create-subtask action", async () => { + const provider = { delegateParentAndOpenChild: vi.fn().mockResolvedValue({ taskId: "child-1" }) } + const task = createTask(provider) + task.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked" }) + + await getPendingActionAccess(task).resumePendingTaskAction(createAction) + + expect(provider.delegateParentAndOpenChild).toHaveBeenCalledWith({ + parentTaskId: "task-1", + message: "Child task", + initialTodos: [], + mode: "ask", + pendingActionId: "create-action", + }) + }) + + it("executes an approved finish-subtask action", async () => { + const provider = { reopenParentFromDelegation: vi.fn().mockResolvedValue(true) } + const task = createTask(provider) + task.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked" }) + + await getPendingActionAccess(task).resumePendingTaskAction(finishAction) + + expect(provider.reopenParentFromDelegation).toHaveBeenCalledWith({ + parentTaskId: "parent-1", + childTaskId: "task-1", + completionResultSummary: "Done", + pendingActionId: "finish-action", + }) + }) + + it("continues with denied queued feedback after durable persistence", async () => { + const provider = { reopenParentFromDelegation: vi.fn().mockResolvedValue(false) } + const task = createTask(provider) + task.ask = vi.fn().mockResolvedValue({ + response: "messageResponse", + text: "Revise this", + queuedMessageId: "queued-1", + }) + + await getPendingActionAccess(task).resumePendingTaskAction(finishAction) + + expect(task.persistQueuedFeedbackAndAcknowledge).toHaveBeenCalledWith("queued-1", "Revise this", undefined) + expect( + (task as unknown as { initiateTaskLoop: ReturnType }).initiateTaskLoop, + ).toHaveBeenCalledWith([expect.objectContaining({ type: "tool_result", tool_use_id: "finish-action" })]) + }) + + it("records ordinary feedback when a restored action is denied", async () => { + const task = createTask({}) + task.ask = vi.fn().mockResolvedValue({ response: "messageResponse", text: "No" }) + + await getPendingActionAccess(task).resumePendingTaskAction(createAction) + + expect(task.say).toHaveBeenCalledWith("user_feedback", "No", undefined) + }) + + it("continues with a textless denial when the user clicks the deny button", async () => { + const provider = { + delegateParentAndOpenChild: vi.fn(), + reopenParentFromDelegation: vi.fn(), + } + const task = createTask(provider) + task.ask = vi.fn().mockResolvedValue({ response: "noButtonClicked" }) + const initiateTaskLoop = (task as unknown as { initiateTaskLoop: ReturnType }).initiateTaskLoop + + await getPendingActionAccess(task).resumePendingTaskAction(createAction) + + expect(provider.delegateParentAndOpenChild).not.toHaveBeenCalled() + expect(provider.reopenParentFromDelegation).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(initiateTaskLoop).toHaveBeenCalledWith([ + { + type: "tool_result", + tool_use_id: "create-action", + content: JSON.stringify({ status: "denied", message: "The user denied this operation." }), + }, + ]) + }) + + it("fails clearly when the provider is unavailable", async () => { + const task = createTask() + + await expect(getPendingActionAccess(task).resumePendingTaskAction(createAction)).rejects.toThrow( + "Provider unavailable", + ) + }) +}) diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 19bd0c7f34..979ffd8f64 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -4,7 +4,7 @@ import * as os from "os" import * as path from "path" import * as vscode from "vscode" -import type { ClineMessage, GlobalState, ProviderSettings } from "@roo-code/types" +import type { ClineMessage, GlobalState, PendingTaskAction, ProviderSettings } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import { Task } from "../Task" @@ -13,8 +13,11 @@ import { ContextProxy } from "../../config/ContextProxy" import { providerIdentifiers } from "@roo-code/types/provider-identifiers" type TaskPersistenceAccess = { + addToApiConversationHistory: (message: { role: "user"; content: unknown[] }) => Promise resumeTaskFromHistory: () => Promise + resumePendingTaskAction: (action: PendingTaskAction) => Promise saveClineMessages: () => Promise + initiateTaskLoop: (content: unknown[]) => Promise } function getTaskPersistenceAccess(task: Task): TaskPersistenceAccess { @@ -584,6 +587,119 @@ describe("Task persistence", () => { }) }) + describe("pending action resume", () => { + const pendingAction: PendingTaskAction = { + kind: "finish_subtask", + actionId: "finish-action", + approvalText: JSON.stringify({ tool: "finishTask" }), + parentTaskId: "parent-1", + result: "Done", + } + + it("replays an unresolved pending action instead of a generic resume ask", async () => { + const messages: ClineMessage[] = [ + { ts: 1, type: "say", say: "text", text: "Child" }, + { ts: 2, type: "ask", ask: "tool", text: pendingAction.approvalText }, + ] + mockReadTaskMessages.mockResolvedValue(messages) + mockReadApiMessages.mockResolvedValue([ + { + role: "assistant", + content: [{ type: "tool_use", id: "finish-action", name: "attempt_completion", input: {} }], + }, + ]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + historyItem: { + id: "child-1", + number: 1, + ts: 1, + task: "Child", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + pendingAction, + }, + startTask: false, + }) + const replay = vi + .spyOn(getTaskPersistenceAccess(task), "resumePendingTaskAction") + .mockResolvedValue(undefined) + const ask = vi.spyOn(task, "ask") + + await getTaskPersistenceAccess(task).resumeTaskFromHistory() + + expect(replay).toHaveBeenCalledWith(pendingAction) + expect(ask).not.toHaveBeenCalled() + expect(mockSaveTaskMessages).toHaveBeenCalledWith( + expect.objectContaining({ + messages: expect.not.arrayContaining([ + expect.objectContaining({ text: pendingAction.approvalText }), + ]), + }), + ) + }) + + it("reconciles an already-persisted tool result before generic resume", async () => { + mockReadTaskMessages.mockResolvedValue([{ ts: 1, type: "say", say: "text", text: "Child" }]) + mockReadApiMessages.mockResolvedValue([ + { role: "user", content: [{ type: "tool_result", tool_use_id: "finish-action", content: "Denied" }] }, + ]) + mockProvider.clearPendingTaskAction = vi.fn().mockResolvedValue(true) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + historyItem: { + id: "child-1", + number: 1, + ts: 1, + task: "Child", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + pendingAction, + }, + startTask: false, + }) + vi.spyOn(task, "ask").mockResolvedValue({ response: "noButtonClicked" }) + vi.spyOn(getTaskPersistenceAccess(task), "initiateTaskLoop").mockResolvedValue(undefined) + const replay = vi.spyOn(getTaskPersistenceAccess(task), "resumePendingTaskAction") + + await getTaskPersistenceAccess(task).resumeTaskFromHistory() + + expect(mockProvider.clearPendingTaskAction).toHaveBeenCalledWith("child-1", "finish-action") + expect(replay).not.toHaveBeenCalled() + expect(task.ask).toHaveBeenCalledWith("resume_task") + }) + + it("clears pending metadata after the matching tool result is saved", async () => { + mockProvider.clearPendingTaskAction = vi.fn().mockResolvedValue(true) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + historyItem: { + id: "child-1", + number: 1, + ts: 1, + task: "Child", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + pendingAction, + }, + startTask: false, + }) + + await getTaskPersistenceAccess(task).addToApiConversationHistory({ + role: "user", + content: [{ type: "tool_result", tool_use_id: "finish-action", content: "Denied" }], + }) + + expect(mockProvider.clearPendingTaskAction).toHaveBeenCalledWith("child-1", "finish-action") + }) + }) + // ── flushPendingToolResultsToHistory — save failure/success ─────────── describe("flushPendingToolResultsToHistory persistence", () => { diff --git a/src/core/task/__tests__/ask-queued-message-drain.spec.ts b/src/core/task/__tests__/ask-queued-message-drain.spec.ts index 06f577881e..4231a2c5c8 100644 --- a/src/core/task/__tests__/ask-queued-message-drain.spec.ts +++ b/src/core/task/__tests__/ask-queued-message-drain.spec.ts @@ -1,10 +1,17 @@ import { Task } from "../Task" +type QueueTaskTestAccess = { + say: Task["say"] + saveClineMessages: () => Promise +} + +const getQueueTaskTestAccess = (task: Task) => task as unknown as QueueTaskTestAccess + // Keep this test focused: if a queued message arrives while Task.ask() is blocked, // it should be consumed and used to fulfill the ask. describe("Task.ask queued message drain", () => { - it("consumes queued message while blocked on followup ask", async () => { + function createTask(provider?: { getState: () => Promise> }) { const task = Object.create(Task.prototype) as Task ;(task as any).abort = false ;(task as any).clineMessages = [] @@ -12,19 +19,21 @@ describe("Task.ask queued message drain", () => { ;(task as any).askResponseText = undefined ;(task as any).askResponseImages = undefined ;(task as any).lastMessageTs = undefined + return import("../../message-queue/MessageQueueService").then(({ MessageQueueService }) => { + ;(task as any).messageQueueService = new MessageQueueService() + ;(task as any).addToClineMessages = vi.fn(async () => {}) + ;(task as any).saveClineMessages = vi.fn(async () => {}) + ;(task as any).updateClineMessage = vi.fn(async () => {}) + ;(task as any).cancelAutoApprovalTimeout = vi.fn(() => {}) + ;(task as any).checkpointSave = vi.fn(async () => {}) + ;(task as any).emit = vi.fn() + ;(task as any).providerRef = { deref: () => provider } + return task + }) + } - // Message queue service exists in constructor; for unit test we can attach a real one. - const { MessageQueueService } = await import("../../message-queue/MessageQueueService") - ;(task as any).messageQueueService = new MessageQueueService() - - // Minimal stubs used by ask() - ;(task as any).addToClineMessages = vi.fn(async () => {}) - ;(task as any).saveClineMessages = vi.fn(async () => {}) - ;(task as any).updateClineMessage = vi.fn(async () => {}) - ;(task as any).cancelAutoApprovalTimeout = vi.fn(() => {}) - ;(task as any).checkpointSave = vi.fn(async () => {}) - ;(task as any).emit = vi.fn() - ;(task as any).providerRef = { deref: () => undefined } + it("consumes queued message while blocked on followup ask", async () => { + const task = await createTask() const askPromise = task.ask("followup", "Q?", false) @@ -37,23 +46,7 @@ describe("Task.ask queued message drain", () => { }) it("does not consume queued messages for command_output asks", async () => { - const task = Object.create(Task.prototype) as Task - ;(task as any).abort = false - ;(task as any).clineMessages = [] - ;(task as any).askResponse = undefined - ;(task as any).askResponseText = undefined - ;(task as any).askResponseImages = undefined - ;(task as any).lastMessageTs = undefined - - const { MessageQueueService } = await import("../../message-queue/MessageQueueService") - ;(task as any).messageQueueService = new MessageQueueService() - ;(task as any).addToClineMessages = vi.fn(async () => {}) - ;(task as any).saveClineMessages = vi.fn(async () => {}) - ;(task as any).updateClineMessage = vi.fn(async () => {}) - ;(task as any).cancelAutoApprovalTimeout = vi.fn(() => {}) - ;(task as any).checkpointSave = vi.fn(async () => {}) - ;(task as any).emit = vi.fn() - ;(task as any).providerRef = { deref: () => undefined } + const task = await createTask() const askPromise = task.ask("command_output", "command is still running...", false) ;(task as any).messageQueueService.addMessage("1+1=?") @@ -69,4 +62,127 @@ describe("Task.ask queued message drain", () => { expect((task as any).messageQueueService.isEmpty()).toBe(false) expect((task as any).messageQueueService.messages[0]?.text).toBe("1+1=?") }) + + it.each(["finishTask", "newTask"])("queued feedback overrides auto-approval for %s", async (tool) => { + const task = await createTask({ + getState: async () => ({ autoApprovalEnabled: true, alwaysAllowSubtasks: true }), + }) + task.messageQueueService.addMessage("Please revise this first") + + const result = await task.ask("tool", JSON.stringify({ tool }), false) + + expect(result).toMatchObject({ + response: "messageResponse", + text: "Please revise this first", + images: undefined, + }) + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + expect(task.messageQueueService.isEmpty()).toBe(false) + expect(task.messageQueueService.removeMessage(result.queuedMessageId!)).toBe(true) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("preserves approve-with-feedback behavior for ordinary tool asks", async () => { + const task = await createTask() + task.messageQueueService.addMessage("Use this context") + + const result = await task.ask("tool", JSON.stringify({ tool: "readFile" }), false) + + expect(result).toMatchObject({ response: "yesButtonClicked", text: "Use this context" }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it.each([ + ["command", "npm test"], + ["use_mcp_server", "{}"], + ["tool", "not-json"], + ] as const)("preserves approve-with-feedback behavior for %s asks", async (type, text) => { + const task = await createTask() + task.messageQueueService.addMessage("Approval context") + + const result = await task.ask(type, text, false) + + expect(result).toMatchObject({ response: "yesButtonClicked", text: "Approval context" }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("claims lifecycle feedback that arrives while an ask is waiting", async () => { + const task = await createTask() + const ask = task.ask("tool", JSON.stringify({ tool: "finishTask" }), false) + task.messageQueueService.addMessage("Late feedback") + + const result = await ask + + expect(result).toMatchObject({ response: "messageResponse", text: "Late feedback" }) + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + expect(task.messageQueueService.claimNextMessage()).toBeUndefined() + }) + + it("uses queued feedback instead of accepting a completion result", async () => { + const task = await createTask() + task.messageQueueService.addMessage("One more change") + + const result = await task.ask("completion_result", "Done", false) + + expect(result).toMatchObject({ response: "messageResponse", text: "One more change" }) + expect(task.messageQueueService.isEmpty()).toBe(false) + task.messageQueueService.removeMessage(result.queuedMessageId!) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("retains lifecycle feedback until its history write succeeds", async () => { + vi.useFakeTimers() + try { + const task = await createTask() + task.messageQueueService.addMessage("Keep this message") + const result = await task.ask("tool", JSON.stringify({ tool: "finishTask" }), false) + const saveClineMessages = vi.fn().mockResolvedValueOnce(false).mockResolvedValueOnce(true) + const taskAccess = getQueueTaskTestAccess(task) + taskAccess.say = vi.fn().mockResolvedValue(undefined) + taskAccess.saveClineMessages = saveClineMessages + + const persistence = task.persistQueuedFeedbackAndAcknowledge( + result.queuedMessageId!, + result.text, + result.images, + ) + await vi.advanceTimersByTimeAsync(0) + expect(task.messageQueueService.isEmpty()).toBe(false) + expect(task.messageQueueService.claimNextMessage()).toBeUndefined() + + await vi.advanceTimersByTimeAsync(250) + expect(await persistence).toBe(true) + expect(task.messageQueueService.isEmpty()).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it("retries a failed feedback write without duplicating the history row", async () => { + vi.useFakeTimers() + try { + const task = await createTask() + task.messageQueueService.addMessage("Retry feedback") + const result = await task.ask("tool", JSON.stringify({ tool: "finishTask" }), false) + const saveClineMessages = vi.fn().mockResolvedValueOnce(false).mockResolvedValueOnce(true) + const say = vi.fn().mockResolvedValue(undefined) + const taskAccess = getQueueTaskTestAccess(task) + taskAccess.say = say + taskAccess.saveClineMessages = saveClineMessages + + const persistence = task.persistQueuedFeedbackAndAcknowledge( + result.queuedMessageId!, + result.text, + result.images, + ) + await vi.advanceTimersByTimeAsync(250) + await persistence + + expect(say).toHaveBeenCalledTimes(1) + expect(saveClineMessages).toHaveBeenCalledTimes(2) + expect(task.messageQueueService.isEmpty()).toBe(true) + } finally { + vi.useRealTimers() + } + }) }) diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index b5f19decb0..bcf7742edc 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -1,6 +1,6 @@ import * as vscode from "vscode" -import { RooCodeEventName, type HistoryItem } from "@roo-code/types" +import { RooCodeEventName, type HistoryItem, type PendingTaskAction } from "@roo-code/types" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" @@ -9,6 +9,7 @@ import type { ToolUse } from "../../shared/tools" import { t } from "../../i18n" import { BaseTool, ToolCallbacks } from "./BaseTool" +import { sanitizeToolUseId } from "../../utils/tool-id" interface AttemptCompletionParams { result: string @@ -26,10 +27,13 @@ export interface AttemptCompletionCallbacks extends ToolCallbacks { interface DelegationProvider { log(message: string): void getTaskWithId(id: string): Promise<{ historyItem: HistoryItem }> + setPendingTaskAction(taskId: string, pendingAction: PendingTaskAction): Promise + clearPendingTaskAction(taskId: string, actionId: string): Promise reopenParentFromDelegation(params: { parentTaskId: string childTaskId: string completionResultSummary: string + pendingActionId?: string }): Promise } @@ -38,7 +42,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { async execute(params: AttemptCompletionParams, task: Task, callbacks: AttemptCompletionCallbacks): Promise { const { result } = params - const { handleError, pushToolResult, askFinishSubTaskApproval } = callbacks + const { handleError, pushToolResult, askFinishSubTaskApproval, toolCallId } = callbacks // Prevent attempt_completion if any tool failed in the current turn if (task.didToolFailInCurrentTurn) { @@ -118,6 +122,18 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { (parentHistory?.status === "delegated" || parentHistory?.status === "active") && parentHistory?.awaitingChildId === task.taskId ) { + const pendingActionId = toolCallId ? sanitizeToolUseId(toolCallId) : undefined + if (pendingActionId) { + const pendingAction: PendingTaskAction = { + kind: "finish_subtask", + actionId: pendingActionId, + approvalText: JSON.stringify({ tool: "finishTask" }), + parentTaskId: task.parentTaskId, + result, + } + await provider.setPendingTaskAction(task.taskId, pendingAction) + task.setPendingTaskAction(pendingAction) + } // Known not to be a stale history replay (status was "active", not // "completed"), so flush telemetry before the delegation call, which // may return early below. hasFlushedTelemetry prevents the shared @@ -130,6 +146,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { task, result, provider, + pendingActionId, askFinishSubTaskApproval, pushToolResult, ) @@ -183,7 +200,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { task.flushTelemetryInstallment("attempt_completion") } - const { response, text, images } = await task.ask("completion_result", "", false) + const { response, text, images, queuedMessageId } = await task.ask("completion_result", "", false) if (response === "yesButtonClicked") { // A stale history replay reruns this handler on a fresh Task instance for a @@ -196,7 +213,11 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { } // User provided feedback - push tool result to continue the conversation - await task.say("user_feedback", text ?? "", images) + if (queuedMessageId) { + await task.persistQueuedFeedbackAndAcknowledge(queuedMessageId, text, images) + } else { + await task.say("user_feedback", text ?? "", images) + } const feedbackText = `\n${text}\n` pushToolResult(formatResponse.toolResult(feedbackText, images)) @@ -216,6 +237,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { task: Task, result: string, provider: DelegationProvider, + pendingActionId: string | undefined, askFinishSubTaskApproval: () => Promise, pushToolResult: (result: string) => void, ): Promise<"delegated" | "denied" | "continue"> { @@ -230,9 +252,13 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { parentTaskId: task.parentTaskId!, childTaskId: task.taskId, completionResultSummary: result, + ...(pendingActionId && { pendingActionId }), }) if (didReopen === false) { + if (pendingActionId) { + await provider.clearPendingTaskAction(task.taskId, pendingActionId) + } return "continue" } diff --git a/src/core/tools/NewTaskTool.ts b/src/core/tools/NewTaskTool.ts index f36d8e1e37..35d72224a0 100644 --- a/src/core/tools/NewTaskTool.ts +++ b/src/core/tools/NewTaskTool.ts @@ -1,6 +1,6 @@ import * as vscode from "vscode" -import { TodoItem } from "@roo-code/types" +import type { PendingTaskAction, TodoItem } from "@roo-code/types" import { Task } from "../task/Task" import { getModeBySlug } from "../../shared/modes" @@ -10,6 +10,7 @@ import { parseMarkdownChecklist } from "./UpdateTodoListTool" import { Package } from "../../shared/package" import { BaseTool, ToolCallbacks } from "./BaseTool" import type { ToolUse } from "../../shared/tools" +import { sanitizeToolUseId } from "../../utils/tool-id" interface NewTaskParams { mode: string @@ -22,7 +23,7 @@ export class NewTaskTool extends BaseTool<"new_task"> { async execute(params: NewTaskParams, task: Task, callbacks: ToolCallbacks): Promise { const { mode, message, todos } = params - const { askApproval, handleError, pushToolResult } = callbacks + const { askApproval, handleError, pushToolResult, toolCallId } = callbacks try { // Validate required parameters. @@ -102,6 +103,19 @@ export class NewTaskTool extends BaseTool<"new_task"> { content: message, todos: todoItems, }) + const pendingActionId = toolCallId ? sanitizeToolUseId(toolCallId) : undefined + if (pendingActionId) { + const pendingAction: PendingTaskAction = { + kind: "create_subtask", + actionId: pendingActionId, + approvalText: toolMessage, + mode, + message: unescapedMessage, + todos: todoItems, + } + await provider.setPendingTaskAction(task.taskId, pendingAction) + task.setPendingTaskAction(pendingAction) + } const didApprove = await askApproval("tool", toolMessage) @@ -115,6 +129,7 @@ export class NewTaskTool extends BaseTool<"new_task"> { message: unescapedMessage, initialTodos: todoItems, mode, + ...(pendingActionId && { pendingActionId }), }) // Reflect delegation in tool result (no pause/unpause, no wait) diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 015be6f6bb..b103339962 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -74,6 +74,8 @@ describe("attemptCompletionTool", () => { apiConfiguration: { apiProvider: "test" } as any, api: { getModel: vi.fn().mockReturnValue({ id: "test-model", info: {} }) } as any, flushTelemetryInstallment: vi.fn(), + setPendingTaskAction: vi.fn(), + persistQueuedFeedbackAndAcknowledge: vi.fn().mockResolvedValue(true), } }) @@ -496,6 +498,8 @@ describe("attemptCompletionTool", () => { } throw new Error(`unexpected task id ${id}`) }), + setPendingTaskAction: vi.fn().mockResolvedValue(undefined), + clearPendingTaskAction: vi.fn().mockResolvedValue(true), reopenParentFromDelegation: vi.fn().mockResolvedValue(true), } @@ -512,15 +516,24 @@ describe("attemptCompletionTool", () => { pushToolResult: mockPushToolResult, askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, + toolCallId: "call-attempt-completion", } await attemptCompletionTool.handle(mockTask as Task, block, callbacks) expect(mockAskFinishSubTaskApproval).toHaveBeenCalled() + expect(mockProvider.setPendingTaskAction).toHaveBeenCalledWith("child-1", { + kind: "finish_subtask", + actionId: "call-attempt-completion", + approvalText: JSON.stringify({ tool: "finishTask" }), + parentTaskId: "parent-1", + result: "9", + }) expect(mockProvider.reopenParentFromDelegation).toHaveBeenCalledWith({ parentTaskId: "parent-1", childTaskId: "child-1", completionResultSummary: "9", + pendingActionId: "call-attempt-completion", }) expect(mockTask.ask).not.toHaveBeenCalled() expect(mockPushToolResult).toHaveBeenCalledWith("") @@ -547,6 +560,8 @@ describe("attemptCompletionTool", () => { } throw new Error(`unexpected task id ${id}`) }), + setPendingTaskAction: vi.fn().mockResolvedValue(undefined), + clearPendingTaskAction: vi.fn().mockResolvedValue(true), reopenParentFromDelegation: vi.fn().mockResolvedValue(false), } @@ -564,6 +579,7 @@ describe("attemptCompletionTool", () => { pushToolResult: mockPushToolResult, askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, + toolCallId: "call-stale-completion", } await attemptCompletionTool.handle(mockTask as Task, block, callbacks) @@ -572,7 +588,9 @@ describe("attemptCompletionTool", () => { parentTaskId: "parent-1", childTaskId: "child-1", completionResultSummary: "9", + pendingActionId: "call-stale-completion", }) + expect(mockProvider.clearPendingTaskAction).toHaveBeenCalledWith("child-1", "call-stale-completion") expect(mockTask.ask).toHaveBeenCalledWith("completion_result", "", false) expect(mockPushToolResult).not.toHaveBeenCalledWith("") // Flush once per validated attempt_completion call, before delegation is @@ -802,6 +820,36 @@ describe("attemptCompletionTool", () => { ) expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("")) }) + + it("durably persists queued completion feedback before continuing", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "Done" }, + nativeArgs: { result: "Done" }, + partial: false, + } + mockTask.ask = vi.fn().mockResolvedValue({ + response: "messageResponse", + text: "One more change", + queuedMessageId: "queued-1", + }) + + await attemptCompletionTool.handle(mockTask as Task, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + }) + + expect(mockTask.persistQueuedFeedbackAndAcknowledge).toHaveBeenCalledWith( + "queued-1", + "One more change", + undefined, + ) + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("One more change")) + }) }) }) }) diff --git a/src/core/tools/__tests__/newTaskTool.spec.ts b/src/core/tools/__tests__/newTaskTool.spec.ts index 9e61bc7fab..ec5bd4386b 100644 --- a/src/core/tools/__tests__/newTaskTool.spec.ts +++ b/src/core/tools/__tests__/newTaskTool.spec.ts @@ -616,6 +616,7 @@ describe("newTaskTool delegation flow", () => { mode: "ask", experiments: {}, }), + setPendingTaskAction: vi.fn().mockResolvedValue(undefined), delegateParentAndOpenChild: vi.fn().mockResolvedValue({ taskId: "child-1" }), handleModeSwitch: vi.fn(), } as any @@ -632,6 +633,7 @@ describe("newTaskTool delegation flow", () => { isPaused: false, pausedModeSlug: "ask", taskId: "mock-parent-task-id", + setPendingTaskAction: vi.fn(), enableCheckpoints: false, checkpointSave: mockCheckpointSave, startSubtask: localStartSubtask, @@ -656,14 +658,24 @@ describe("newTaskTool delegation flow", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "call-new-task", }) + expect(providerSpy.setPendingTaskAction).toHaveBeenCalledWith("mock-parent-task-id", { + kind: "create_subtask", + actionId: "call-new-task", + approvalText: expect.stringContaining('"tool":"newTask"'), + mode: "code", + message: "Do something", + todos: [], + }) // Assert: provider method called with correct params expect(providerSpy.delegateParentAndOpenChild).toHaveBeenCalledWith({ parentTaskId: "mock-parent-task-id", message: "Do something", initialTodos: [], mode: "code", + pendingActionId: "call-new-task", }) // Assert: legacy path not used diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 021a2fba91..3f1bf55e72 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -30,6 +30,7 @@ import { type TerminalActionId, type TerminalActionPromptType, type HistoryItem, + type PendingTaskAction, type CloudUserInfo, type CloudOrganizationMembership, type CreateTaskOptions, @@ -717,6 +718,30 @@ export class ClineProvider return this.taskRegistry.taskIds } + public async setPendingTaskAction(taskId: string, pendingAction: PendingTaskAction): Promise { + await this.taskHistoryStore.atomicReadAndUpdate(taskId, (historyItem) => ({ + ...historyItem, + pendingAction, + })) + this.recentTasksCache = undefined + } + + public async clearPendingTaskAction(taskId: string, actionId: string): Promise { + let cleared = false + await this.taskHistoryStore.atomicReadAndUpdate(taskId, (historyItem) => { + if (historyItem.pendingAction?.actionId !== actionId) { + return historyItem + } + + cleared = true + return { ...historyItem, pendingAction: undefined } + }) + if (cleared) { + this.recentTasksCache = undefined + } + return cleared + } + // Pending Edit Operations Management /** @@ -3712,8 +3737,9 @@ export class ClineProvider message: string initialTodos: TodoItem[] mode: string + pendingActionId?: string }): Promise { - const { parentTaskId, message, initialTodos, mode } = params + const { parentTaskId, message, initialTodos, mode, pendingActionId } = params // Metadata-driven delegation is always enabled @@ -3727,6 +3753,14 @@ export class ClineProvider `[delegateParentAndOpenChild] Parent mismatch: expected ${parentTaskId}, current ${parent.taskId}`, ) } + if (pendingActionId) { + const parentHistory = this.taskHistoryStore.get(parentTaskId) + if (parentHistory?.pendingAction?.actionId !== pendingActionId) { + throw new Error( + `[delegateParentAndOpenChild] Pending action mismatch for parent ${parentTaskId}: expected ${pendingActionId}, found ${parentHistory?.pendingAction?.actionId}`, + ) + } + } // 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. @@ -3822,6 +3856,11 @@ export class ClineProvider try { await this.taskHistoryStore.atomicReadAndUpdate(parentTaskId, (historyItem) => { let base = historyItem + if (pendingActionId && base.pendingAction?.actionId !== pendingActionId) { + throw new Error( + `[delegateParentAndOpenChild] Pending action mismatch for parent ${parentTaskId}: expected ${pendingActionId}, found ${base.pendingAction?.actionId}`, + ) + } if (historyItem.status === "delegated") { // Re-read the awaited child's current status under the store lock. const awaitedChildStatus = historyItem.awaitingChildId @@ -3852,6 +3891,7 @@ export class ClineProvider delegatedToId: child.taskId, awaitingChildId: child.taskId, childIds, + pendingAction: base.pendingAction?.actionId === pendingActionId ? undefined : base.pendingAction, } }) this.recentTasksCache = undefined @@ -3922,13 +3962,21 @@ export class ClineProvider parentTaskId: string childTaskId: string completionResultSummary: string + pendingActionId?: string }): Promise { - const { parentTaskId, childTaskId, completionResultSummary } = params + const { parentTaskId, childTaskId, completionResultSummary, pendingActionId } = params return this.runDelegationTransition(parentTaskId, async () => { const globalStoragePath = this.contextProxy.globalStorageUri.fsPath // 1) Load parent from history and current persisted messages const { historyItem } = await this.getTaskWithId(parentTaskId) + const childHistory = this.taskHistoryStore.get(childTaskId) + if (pendingActionId && childHistory?.pendingAction?.actionId !== pendingActionId) { + this.log( + `[reopenParentFromDelegation] Aborting: child ${childTaskId} pending action does not match ${pendingActionId}`, + ) + return false + } // Guard: re-validate delegation state after the async approval gap. // cancelTask() or removeClineFromStack() may have already detached the parent @@ -4095,8 +4143,17 @@ export class ClineProvider childTaskId, parentTaskId, (child) => { + if (pendingActionId && child.pendingAction?.actionId !== pendingActionId) { + throw new Error(`[reopenParentFromDelegation] Pending action mismatch for child ${childTaskId}`) + } assertValidTransition(child.status, "completed") - return { ...child, status: "completed" as const, completionResultSummary } + return { + ...child, + status: "completed" as const, + completionResultSummary, + pendingAction: + child.pendingAction?.actionId === pendingActionId ? undefined : child.pendingAction, + } }, (parent) => { if (parent.status !== "active") { diff --git a/src/core/webview/__tests__/ClineProvider.pending-action.spec.ts b/src/core/webview/__tests__/ClineProvider.pending-action.spec.ts new file mode 100644 index 0000000000..9ba1f578fb --- /dev/null +++ b/src/core/webview/__tests__/ClineProvider.pending-action.spec.ts @@ -0,0 +1,62 @@ +import type { HistoryItem, PendingTaskAction } from "@roo-code/types" + +import { ClineProvider } from "../ClineProvider" + +const pendingAction: PendingTaskAction = { + kind: "create_subtask", + actionId: "action-1", + approvalText: "{}", + mode: "ask", + message: "Child", + todos: [], +} + +function makeProvider(historyItem: HistoryItem) { + let current = historyItem + const atomicReadAndUpdate = vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { + current = updater(current) + return [current] + }) + const provider = { + taskHistoryStore: { atomicReadAndUpdate }, + recentTasksCache: ["cached"], + } as unknown as ClineProvider + return { provider, atomicReadAndUpdate, current: () => current } +} + +const historyItem = { + id: "task-1", + number: 1, + ts: 1, + task: "Task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, +} satisfies HistoryItem + +describe("ClineProvider pending task actions", () => { + it("sets a pending action atomically and invalidates the recent-task cache", async () => { + const { provider, atomicReadAndUpdate, current } = makeProvider(historyItem) + + await ClineProvider.prototype.setPendingTaskAction.call(provider, "task-1", pendingAction) + + expect(atomicReadAndUpdate).toHaveBeenCalledWith("task-1", expect.any(Function)) + expect(current().pendingAction).toEqual(pendingAction) + expect((provider as unknown as { recentTasksCache?: string[] }).recentTasksCache).toBeUndefined() + }) + + it("clears only the matching action", async () => { + const matching = makeProvider({ ...historyItem, pendingAction }) + + await expect( + ClineProvider.prototype.clearPendingTaskAction.call(matching.provider, "task-1", "action-1"), + ).resolves.toBe(true) + expect(matching.current().pendingAction).toBeUndefined() + + const stale = makeProvider({ ...historyItem, pendingAction }) + await expect( + ClineProvider.prototype.clearPendingTaskAction.call(stale.provider, "task-1", "stale-action"), + ).resolves.toBe(false) + expect(stale.current().pendingAction).toEqual(pendingAction) + }) +}) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index f405adc8df..b3a4e7271a 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -836,7 +836,7 @@ }, "core/task/__tests__/ask-queued-message-drain.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 32 + "count": 18 } }, "core/task/__tests__/flushPendingToolResultsToHistory.spec.ts": {