From 75bc9bb7d618251849c53af401881c420faad326 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Mon, 17 Nov 2025 16:47:22 +0000 Subject: [PATCH] fix: prevent auto-acceptance of attempt_completion when messages are queued - Check for queued messages before auto-accepting completion - Process queued messages as user feedback instead of accepting - Add tests for queued message handling during attempt_completion Fixes #9314 --- src/core/tools/AttemptCompletionTool.ts | 21 +++ .../__tests__/attemptCompletionTool.spec.ts | 165 ++++++++++++++++++ 2 files changed, 186 insertions(+) diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index 8a7695748b..720d0d5e75 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -80,6 +80,27 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { return } + // Check if there are queued messages before auto-accepting + const hasQueuedMessages = !task.messageQueueService.isEmpty() + + // If there are queued messages, don't auto-accept the completion + // Instead, process the queued messages first + if (hasQueuedMessages) { + // Get the first queued message + const queuedMessage = task.messageQueueService.dequeueMessage() + if (queuedMessage) { + // Add the queued message as user feedback + await task.say("user_feedback", queuedMessage.text ?? "", queuedMessage.images) + + const feedbackText = `The user has provided feedback on the results. Consider their input to continue the task, and then attempt completion again.\n\n${queuedMessage.text}\n` + pushToolResult(formatResponse.toolResult(feedbackText, queuedMessage.images)) + + // Process any remaining queued messages + task.processQueuedMessages() + return + } + } + const { response, text, images } = await task.ask("completion_result", "", false) if (response === "yesButtonClicked") { diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 72b4560c4a..f6598c47a8 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -6,6 +6,12 @@ import { AttemptCompletionToolUse } from "../../../shared/tools" vi.mock("../../prompts/responses", () => ({ formatResponse: { toolError: vi.fn((msg: string) => `Error: ${msg}`), + toolResult: vi.fn((text: string, images?: string[]) => { + if (images && images.length > 0) { + return `${text}\n[Images: ${images.join(", ")}]` + } + return text + }), }, })) @@ -25,6 +31,22 @@ vi.mock("../../../shared/package", () => ({ }, })) +// Mock TelemetryService +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureTaskCompleted: vi.fn(), + }, + }, +})) + +// Mock RooCodeEventName +vi.mock("@roo-code/types", () => ({ + RooCodeEventName: { + TaskCompleted: "TaskCompleted", + }, +})) + import { attemptCompletionTool, AttemptCompletionCallbacks } from "../AttemptCompletionTool" import { Task } from "../../task/Task" import * as vscode from "vscode" @@ -38,6 +60,7 @@ describe("attemptCompletionTool", () => { let mockToolDescription: ReturnType let mockAskFinishSubTaskApproval: ReturnType let mockGetConfiguration: ReturnType + let mockMessageQueueService: any beforeEach(() => { mockPushToolResult = vi.fn() @@ -58,10 +81,33 @@ describe("attemptCompletionTool", () => { // Setup vscode mock vi.mocked(vscode.workspace.getConfiguration).mockImplementation(mockGetConfiguration) + // Setup message queue service mock + mockMessageQueueService = { + isEmpty: vi.fn(() => true), + dequeueMessage: vi.fn(), + } + mockTask = { consecutiveMistakeCount: 0, recordToolError: vi.fn(), todoList: undefined, + messageQueueService: mockMessageQueueService, + say: vi.fn().mockResolvedValue(undefined), + processQueuedMessages: vi.fn(), + ask: vi.fn().mockResolvedValue({ + response: "yesButtonClicked", + text: undefined, + images: undefined, + }), + sayAndCreateMissingParamError: vi.fn(), + emit: vi.fn(), + taskId: "test-task-id", + getTokenUsage: vi.fn().mockReturnValue({}), + toolUsage: {}, + parentTask: undefined, + providerRef: { + deref: vi.fn().mockReturnValue(null), + } as any, } }) @@ -400,4 +446,123 @@ describe("attemptCompletionTool", () => { ) }) }) + + describe("queued messages handling", () => { + it("should process queued messages before asking for completion approval", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "Task completed successfully" }, + partial: false, + } + + // Mock queued message + const queuedMessage = { + id: "msg-1", + text: "Wait, I need to add one more thing", + images: ["image1.png"], + timestamp: Date.now(), + } + + mockMessageQueueService.isEmpty.mockReturnValue(false) + mockMessageQueueService.dequeueMessage.mockReturnValue(queuedMessage) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + removeClosingTag: mockRemoveClosingTag, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + // Verify queued message was processed + expect(mockMessageQueueService.isEmpty).toHaveBeenCalled() + expect(mockMessageQueueService.dequeueMessage).toHaveBeenCalled() + expect(mockTask.say).toHaveBeenCalledWith("user_feedback", queuedMessage.text, queuedMessage.images) + expect(mockTask.processQueuedMessages).toHaveBeenCalled() + + // Verify completion was not auto-accepted + expect(mockPushToolResult).toHaveBeenCalled() + // The tool result should contain the feedback + const toolResultCall = mockPushToolResult.mock.calls[0] + expect(toolResultCall[0]).toContain("The user has provided feedback") + expect(toolResultCall[0]).toContain(queuedMessage.text) + }) + + it("should proceed with normal completion flow when no messages are queued", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "Task completed successfully" }, + partial: false, + } + + mockMessageQueueService.isEmpty.mockReturnValue(true) + + // Mock the ask method to simulate user approval + mockTask.ask = vi.fn().mockResolvedValue({ + response: "yesButtonClicked", + text: undefined, + images: undefined, + }) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + removeClosingTag: mockRemoveClosingTag, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + // Verify normal flow was followed + expect(mockMessageQueueService.isEmpty).toHaveBeenCalled() + expect(mockMessageQueueService.dequeueMessage).not.toHaveBeenCalled() + expect(mockTask.ask).toHaveBeenCalledWith("completion_result", "", false) + expect(mockPushToolResult).toHaveBeenCalledWith("") + }) + + it("should handle multiple queued messages by processing the first one", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "Task completed successfully" }, + partial: false, + } + + // Mock first queued message + const firstQueuedMessage = { + id: "msg-1", + text: "First feedback", + images: undefined, + timestamp: Date.now(), + } + + mockMessageQueueService.isEmpty.mockReturnValue(false) + mockMessageQueueService.dequeueMessage.mockReturnValue(firstQueuedMessage) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + removeClosingTag: mockRemoveClosingTag, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + // Verify first message was processed + expect(mockMessageQueueService.dequeueMessage).toHaveBeenCalledTimes(1) + expect(mockTask.say).toHaveBeenCalledWith("user_feedback", firstQueuedMessage.text, undefined) + + // Verify processQueuedMessages was called to handle remaining messages + expect(mockTask.processQueuedMessages).toHaveBeenCalled() + }) + }) })