From db8f472fff8f53a1261fdadbe51277e72e6a6412 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Fri, 6 Feb 2026 19:53:12 -0700 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20rewind-on-interrupt=20=E2=80=94=20c?= =?UTF-8?q?lean=20conversation=20state=20on=20abort/resume?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add rewindToLastAskPoint() to truncate clineMessages and sanitize API histories - Add two-phase completion flow (completion_result → resume_completed_task) - Extract normalizeContentBlocks() utility to src/shared/messages.ts - Replace 'as' casts with type predicates and 'satisfies' checks - Remove synthetic 'interrupted' tool_result injection - Neutral fallback message in validateToolResultIds - Comprehensive test coverage: 65 tests across 5 files - Shared test fixtures in rewind-test-setup.ts --- src/core/task/Task.ts | 352 ++++++++----- .../__tests__/fixtures/rewind-test-setup.ts | 90 ++++ .../task/__tests__/rewind-integration.spec.ts | 496 ++++++++++++++++++ .../__tests__/rewindToLastAskPoint.spec.ts | 493 +++++++++++++++++ .../__tests__/validateToolResultIds.spec.ts | 8 +- src/core/task/validateToolResultIds.ts | 2 +- src/core/tools/AttemptCompletionTool.ts | 28 +- .../__tests__/attemptCompletionTool.spec.ts | 161 ++++++ src/shared/__tests__/messages.spec.ts | 40 ++ src/shared/messages.ts | 14 + 10 files changed, 1552 insertions(+), 132 deletions(-) create mode 100644 src/core/task/__tests__/fixtures/rewind-test-setup.ts create mode 100644 src/core/task/__tests__/rewind-integration.spec.ts create mode 100644 src/core/task/__tests__/rewindToLastAskPoint.spec.ts create mode 100644 src/shared/__tests__/messages.spec.ts create mode 100644 src/shared/messages.ts diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index f4e41c1bfd7..e2d2a303eb8 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -63,6 +63,7 @@ import { maybeRemoveImageBlocks } from "../../api/transform/image-cleaning" // shared import { findLastIndex } from "../../shared/array" +import { normalizeContentBlocks } from "../../shared/messages" import { combineApiRequests } from "../../shared/combineApiRequests" import { combineCommandSequences } from "../../shared/combineCommandSequences" import { t } from "../../i18n" @@ -2105,6 +2106,32 @@ export class Task extends EventEmitter implements TaskLike { } } + /** + * Find the tool_use ID for a pending `attempt_completion` call in the + * API conversation history. Returns undefined if not found. + * + * This detects the case where the user accepted completion and the + * process restarted before the follow-up could be resolved — the + * history ends with an assistant message containing an + * `attempt_completion` tool_use without a matching tool_result. + */ + private getPendingAttemptCompletionToolUseId(): string | undefined { + const history = this.apiConversationHistory + if (history.length === 0) return undefined + + const lastMsg = history[history.length - 1] + if (lastMsg.role !== "assistant") return undefined + + const content = normalizeContentBlocks(lastMsg.content) + + const toolUse = content.find( + (block): block is Anthropic.Messages.ToolUseBlockParam => + block.type === "tool_use" && block.name === "attempt_completion", + ) + + return toolUse?.id + } + private async resumeTaskFromHistory() { if (this.enableBridge) { try { @@ -2167,15 +2194,32 @@ export class Task extends EventEmitter implements TaskLike { // the task first. this.apiConversationHistory = await this.getSavedApiConversationHistory() + // No explicit overwriteApiConversationHistory() call here — the history + // was just loaded from disk by getSavedApiConversationHistory(), so + // re-persisting it would be a no-op. overwriteApiConversationHistory() + // has no validation side-effects; validateAndFixToolResultIds only runs + // inside addToApiConversationHistory(). For the interrupted path, + // rewindToLastAskPoint() will persist any truncation it performs. + const lastClineMessage = this.clineMessages .slice() .reverse() .find((m) => !(m.ask === "resume_task" || m.ask === "resume_completed_task")) // Could be multiple resume tasks. + // Check if this is a completed task that has a pending attempt_completion + // tool_use without a tool_result. This happens when the user accepted + // completion (yesButtonClicked) and the process restarted before the + // follow-up ask could be resolved. + const pendingAttemptCompletionToolUseId = this.getPendingAttemptCompletionToolUseId() + let askType: ClineAsk - if (lastClineMessage?.ask === "completion_result") { + if (lastClineMessage?.ask === "completion_result" || pendingAttemptCompletionToolUseId) { askType = "resume_completed_task" } else { + // Rewind conversation state to the last valid ask point. + // Only rewind for non-completed tasks — completed tasks may have + // a pending attempt_completion tool_use that we need to preserve. + await this.rewindToLastAskPoint() askType = "resume_task" } @@ -2192,136 +2236,56 @@ export class Task extends EventEmitter implements TaskLike { responseImages = images } - // Make sure that the api conversation history can be resumed by the API, - // even if it goes out of sync with cline messages. - let existingApiConversationHistory: ApiMessage[] = await this.getSavedApiConversationHistory() - - // Tool blocks are always preserved; native tool calling only. - - // if the last message is an assistant message, we need to check if there's tool use since every tool use has to have a tool response - // if there's no tool use and only a text block, then we can just add a user message - // (note this isn't relevant anymore since we use custom tool prompts instead of tool use blocks, but this is here for legacy purposes in case users resume old tasks) - - // if the last message is a user message, we can need to get the assistant message before it to see if it made tool calls, and if so, fill in the remaining tool responses with 'interrupted' - - let modifiedOldUserContent: Anthropic.Messages.ContentBlockParam[] // either the last message if its user message, or the user message before the last (assistant) message - let modifiedApiConversationHistory: ApiMessage[] // need to remove the last user message to replace with new modified user message - if (existingApiConversationHistory.length > 0) { - const lastMessage = existingApiConversationHistory[existingApiConversationHistory.length - 1] - - if (lastMessage.role === "assistant") { - const content = Array.isArray(lastMessage.content) - ? lastMessage.content - : [{ type: "text", text: lastMessage.content }] - const hasToolUse = content.some((block) => block.type === "tool_use") - - if (hasToolUse) { - const toolUseBlocks = content.filter( - (block) => block.type === "tool_use", - ) as Anthropic.Messages.ToolUseBlock[] - const toolResponses: Anthropic.ToolResultBlockParam[] = toolUseBlocks.map((block) => ({ - type: "tool_result", - tool_use_id: block.id, - content: "Task was interrupted before this tool call could be completed.", - })) - modifiedApiConversationHistory = [...existingApiConversationHistory] // no changes - modifiedOldUserContent = [...toolResponses] - } else { - modifiedApiConversationHistory = [...existingApiConversationHistory] - modifiedOldUserContent = [] - } - } else if (lastMessage.role === "user") { - const previousAssistantMessage: ApiMessage | undefined = - existingApiConversationHistory[existingApiConversationHistory.length - 2] - - const existingUserContent: Anthropic.Messages.ContentBlockParam[] = Array.isArray(lastMessage.content) - ? lastMessage.content - : [{ type: "text", text: lastMessage.content }] - if (previousAssistantMessage && previousAssistantMessage.role === "assistant") { - const assistantContent = Array.isArray(previousAssistantMessage.content) - ? previousAssistantMessage.content - : [{ type: "text", text: previousAssistantMessage.content }] - - const toolUseBlocks = assistantContent.filter( - (block) => block.type === "tool_use", - ) as Anthropic.Messages.ToolUseBlock[] - - if (toolUseBlocks.length > 0) { - const existingToolResults = existingUserContent.filter( - (block) => block.type === "tool_result", - ) as Anthropic.ToolResultBlockParam[] - - const missingToolResponses: Anthropic.ToolResultBlockParam[] = toolUseBlocks - .filter( - (toolUse) => !existingToolResults.some((result) => result.tool_use_id === toolUse.id), - ) - .map((toolUse) => ({ - type: "tool_result", - tool_use_id: toolUse.id, - content: "Task was interrupted before this tool call could be completed.", - })) - - modifiedApiConversationHistory = existingApiConversationHistory.slice(0, -1) // removes the last user message - modifiedOldUserContent = [...existingUserContent, ...missingToolResponses] - } else { - modifiedApiConversationHistory = existingApiConversationHistory.slice(0, -1) - modifiedOldUserContent = [...existingUserContent] - } - } else { - modifiedApiConversationHistory = existingApiConversationHistory.slice(0, -1) - modifiedOldUserContent = [...existingUserContent] - } - } else { - throw new Error("Unexpected: Last message is not a user or assistant message") - } - } else { - throw new Error("Unexpected: No existing API conversation history") - } - - let newUserContent: Anthropic.Messages.ContentBlockParam[] = [...modifiedOldUserContent] + // Build the new user content for the next API call. + let newUserContent: Anthropic.Messages.ContentBlockParam[] = [] - const agoText = ((): string => { - const timestamp = lastClineMessage?.ts ?? Date.now() - const now = Date.now() - const diff = now - timestamp - const minutes = Math.floor(diff / 60000) - const hours = Math.floor(minutes / 60) - const days = Math.floor(hours / 24) + // If this is a completed task with a pending attempt_completion tool_use, + // route the user's message as a tool_result instead of a bare text block. + // This ensures the API contract is maintained after a process restart. + if (pendingAttemptCompletionToolUseId && responseText) { + const feedbackText = `\n${responseText}\n` + newUserContent.push({ + type: "tool_result", + tool_use_id: pendingAttemptCompletionToolUseId, + content: feedbackText, + } satisfies Anthropic.ToolResultBlockParam) - if (days > 0) { - return `${days} day${days > 1 ? "s" : ""} ago` - } - if (hours > 0) { - return `${hours} hour${hours > 1 ? "s" : ""} ago` + if (responseImages && responseImages.length > 0) { + newUserContent.push(...formatResponse.imageBlocks(responseImages)) } - if (minutes > 0) { - return `${minutes} minute${minutes > 1 ? "s" : ""} ago` + } else { + if (responseText) { + newUserContent.push({ + type: "text", + text: `\n${responseText}\n`, + }) } - return "just now" - })() - if (responseText) { - newUserContent.push({ - type: "text", - text: `\n${responseText}\n`, - }) - } - - if (responseImages && responseImages.length > 0) { - newUserContent.push(...formatResponse.imageBlocks(responseImages)) + if (responseImages && responseImages.length > 0) { + newUserContent.push(...formatResponse.imageBlocks(responseImages)) + } } // Ensure we have at least some content to send to the API. // If newUserContent is empty, add a minimal resumption message. if (newUserContent.length === 0) { - newUserContent.push({ - type: "text", - text: "[TASK RESUMPTION] Resuming task...", - }) + if (pendingAttemptCompletionToolUseId) { + newUserContent.push({ + type: "tool_result", + tool_use_id: pendingAttemptCompletionToolUseId, + content: JSON.stringify({ + status: "completed", + message: "The user is satisfied with the result.", + }), + } satisfies Anthropic.ToolResultBlockParam) + } else { + newUserContent.push({ + type: "text", + text: "The user has resumed this task.", + }) + } } - await this.overwriteApiConversationHistory(modifiedApiConversationHistory) - // Task resuming from history item. await this.initiateTaskLoop(newUserContent) } @@ -2349,6 +2313,140 @@ export class Task extends EventEmitter implements TaskLike { this.debouncedEmitTokenUsage.flush() } + /** + * Rewind the conversation state to the last completed ask point. + * + * This implements the "Rewind on Interrupt" model: instead of injecting + * synthetic "interrupted" messages into the conversation, we truncate both + * `clineMessages` and `apiConversationHistory` back to the last valid + * checkpoint where the system was waiting for user input. + * + * The method: + * 1. Finds the last non-partial ask message in clineMessages (the rewind target) + * 2. Truncates clineMessages to include up to and including that ask + * 3. Removes any trailing incomplete assistant message from apiConversationHistory + * 4. Removes orphaned user messages that don't have matching tool_results + * 5. Persists both histories to disk + * + * This ensures the conversation can be cleanly resumed without the model + * seeing fabricated "interrupted" context. + */ + private async rewindToLastAskPoint(): Promise { + try { + // Find the last completed (non-partial) ask message that represents + // a real checkpoint where the system was waiting for user input. + // Skip non-blocking asks (command_output) and resume asks since + // they aren't real interaction points. + const lastAskIndex = findLastIndex( + this.clineMessages, + (m) => + m.type === "ask" && + !m.partial && + m.ask !== "command_output" && + m.ask !== "resume_task" && + m.ask !== "resume_completed_task", + ) + + if (lastAskIndex === -1) { + console.log(`[Task#${this.taskId}] rewindToLastAskPoint: no ask point found, nothing to rewind`) + return false + } + + const lastAsk = this.clineMessages[lastAskIndex] + console.log( + `[Task#${this.taskId}] rewindToLastAskPoint: rewinding to ask "${lastAsk.ask}" at index ${lastAskIndex} (ts=${lastAsk.ts})`, + ) + + // Truncate clineMessages to include up to and including the ask point. + // Also keep any non-interactive messages (like api_req_started cost info) + // that are siblings of this ask in the same turn. + this.clineMessages = this.clineMessages.slice(0, lastAskIndex + 1) + + // Now fix up the API conversation history. + // The goal is to ensure the history ends with either: + // (a) a complete user→assistant→user exchange (all tool_uses have tool_results), or + // (b) a user message (waiting for assistant response) + // + // We need to remove any trailing assistant message that has tool_use + // blocks without corresponding tool_results (the interrupted response). + const history = this.apiConversationHistory + if (history.length > 0) { + const lastMsg = history[history.length - 1] + + if (lastMsg.role === "assistant") { + // Check if this assistant message has tool_use blocks + const content = normalizeContentBlocks(lastMsg.content) + + const hasToolUse = content.some( + (block: Anthropic.Messages.ContentBlockParam) => block.type === "tool_use", + ) + + if (hasToolUse) { + // This assistant message has tool calls that were never answered. + // Remove it entirely - the model will regenerate on resume. + history.pop() + console.log( + `[Task#${this.taskId}] rewindToLastAskPoint: removed trailing assistant message with unanswered tool_use`, + ) + } + // If assistant message is text-only, it's fine to keep + } else if (lastMsg.role === "user") { + // Check if previous assistant message has tool_use blocks + // that this user message should have tool_results for + if (history.length >= 2) { + const prevAssistant = history[history.length - 2] + if (prevAssistant.role === "assistant") { + const assistantContent = normalizeContentBlocks(prevAssistant.content) + + const toolUseBlocks = assistantContent.filter( + (block): block is Anthropic.Messages.ToolUseBlockParam => block.type === "tool_use", + ) + + if (toolUseBlocks.length > 0) { + const userContent = normalizeContentBlocks(lastMsg.content) + + const toolResultIds = new Set( + userContent + .filter( + (block): block is Anthropic.Messages.ToolResultBlockParam => + block.type === "tool_result", + ) + .map((block) => block.tool_use_id), + ) + + const allToolUsesAnswered = toolUseBlocks.every((tu) => toolResultIds.has(tu.id)) + + if (!allToolUsesAnswered) { + // User message has incomplete tool_results. + // Remove both the user message and the assistant message. + history.pop() // remove incomplete user message + history.pop() // remove assistant message with unanswered tools + console.log( + `[Task#${this.taskId}] rewindToLastAskPoint: removed incomplete user+assistant messages`, + ) + } + } + } + } + } + } + + // Persist both histories + await this.overwriteClineMessages(this.clineMessages) + await this.overwriteApiConversationHistory(history) + + console.log( + `[Task#${this.taskId}] rewindToLastAskPoint: rewind complete. ` + + `clineMessages: ${this.clineMessages.length}, apiHistory: ${history.length}`, + ) + + return true + } catch (error) { + console.error(`[Task#${this.taskId}] rewindToLastAskPoint failed:`, error) + return false + } + } + public async abortTask(isAbandoned = false) { // Aborting task @@ -2374,12 +2472,16 @@ export class Task extends EventEmitter implements TaskLike { console.error(`Error during task ${this.taskId}.${this.instanceId} disposal:`, error) // Don't rethrow - we want abort to always succeed } - // Save the countdown message in the automatic retry or other content. - try { - // Save the countdown message in the automatic retry or other content. - await this.saveClineMessages() - } catch (error) { - console.error(`Error saving messages during abort for task ${this.taskId}.${this.instanceId}:`, error) + // Rewind conversation state to the last valid ask point. + // This ensures that on resume, the model doesn't see synthetic + // "interrupted" messages — instead it sees a clean conversation + // ending at the last checkpoint where user input was expected. + if (!isAbandoned) { + try { + await this.rewindToLastAskPoint() + } catch (error) { + console.error(`[Task#${this.taskId}.${this.instanceId}] Error during rewind on abort:`, error) + } } } diff --git a/src/core/task/__tests__/fixtures/rewind-test-setup.ts b/src/core/task/__tests__/fixtures/rewind-test-setup.ts new file mode 100644 index 00000000000..97186eb5f83 --- /dev/null +++ b/src/core/task/__tests__/fixtures/rewind-test-setup.ts @@ -0,0 +1,90 @@ +/** + * Shared test fixtures for the rewind test suites. + * + * NOTE: Every test file that uses these helpers MUST declare its own + * `vi.mock()` blocks — vitest hoists mocks to the top of the *test* file, + * so they cannot be extracted into a shared module. + */ +import * as os from "os" +import * as path from "path" + +import * as vscode from "vscode" + +import type { ProviderSettings } from "@roo-code/types" +import { TelemetryService } from "@roo-code/telemetry" + +import { Task } from "../../Task" +import { ClineProvider } from "../../../webview/ClineProvider" +import { ContextProxy } from "../../../config/ContextProxy" + +/** Default API configuration used across rewind tests. */ +export const DEFAULT_API_CONFIG: ProviderSettings = { + apiProvider: "anthropic", + apiModelId: "claude-3-5-sonnet-20241022", + apiKey: "test-api-key", +} + +/** Ensure the TelemetryService singleton is initialised (idempotent). */ +export function initializeTelemetry(): void { + if (!TelemetryService.hasInstance()) { + TelemetryService.createInstance([]) + } +} + +/** Build a minimal mock `vscode.ExtensionContext`. */ +export function createMockExtensionContext(): vscode.ExtensionContext { + const storageUri = { fsPath: path.join(os.tmpdir(), "test-storage") } + + return { + globalState: { + get: vi.fn().mockReturnValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + keys: vi.fn().mockReturnValue([]), + }, + globalStorageUri: storageUri, + workspaceState: { + get: vi.fn().mockReturnValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + keys: vi.fn().mockReturnValue([]), + }, + secrets: { + get: vi.fn().mockResolvedValue(undefined), + store: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(undefined), + }, + extensionUri: { fsPath: "/mock/extension/path" }, + extension: { packageJSON: { version: "1.0.0" } }, + } as unknown as vscode.ExtensionContext +} + +/** Build a mock `OutputChannel`. */ +export function createMockOutputChannel() { + return { + name: "test", + appendLine: vi.fn(), + append: vi.fn(), + replace: vi.fn(), + clear: vi.fn(), + show: vi.fn(), + hide: vi.fn(), + dispose: vi.fn(), + } +} + +/** Build a `ClineProvider` backed by mocks. */ +export function createMockProvider(context: vscode.ExtensionContext): any { + return new ClineProvider(context, createMockOutputChannel(), "sidebar", new ContextProxy(context)) as any +} + +/** + * Create a `Task` that does NOT auto-start its loop. + * Pass an explicit `apiConfig` or fall back to {@link DEFAULT_API_CONFIG}. + */ +export function createTask(provider: any, apiConfig: ProviderSettings = DEFAULT_API_CONFIG): Task { + return new Task({ + provider, + apiConfiguration: apiConfig, + task: "test task", + startTask: false, + }) +} diff --git a/src/core/task/__tests__/rewind-integration.spec.ts b/src/core/task/__tests__/rewind-integration.spec.ts new file mode 100644 index 00000000000..d09848f287d --- /dev/null +++ b/src/core/task/__tests__/rewind-integration.spec.ts @@ -0,0 +1,496 @@ +// npx vitest run core/task/__tests__/rewind-integration.spec.ts + +import { Anthropic } from "@anthropic-ai/sdk" + +import type { ClineMessage } from "@roo-code/types" + +import { + initializeTelemetry, + createMockExtensionContext, + createMockProvider, + createTask, +} from "./fixtures/rewind-test-setup" + +// --------------------------------------------------------------------------- +// vi.mock() declarations — must live in the test file (vitest hoists them). +// --------------------------------------------------------------------------- + +vi.mock("delay", () => ({ + __esModule: true, + default: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("uuid", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + v7: vi.fn(() => "00000000-0000-7000-8000-000000000000"), + } +}) + +vi.mock("execa", () => ({ + execa: vi.fn(), +})) + +vi.mock("fs/promises", async (importOriginal) => { + const actual = (await importOriginal()) as Record + const mockFunctions = { + mkdir: vi.fn().mockResolvedValue(undefined), + writeFile: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockImplementation((filePath) => { + if (filePath.includes("ui_messages.json")) { + return Promise.resolve(JSON.stringify([])) + } + if (filePath.includes("api_conversation_history.json")) { + return Promise.resolve(JSON.stringify([])) + } + return Promise.resolve("[]") + }), + unlink: vi.fn().mockResolvedValue(undefined), + rmdir: vi.fn().mockResolvedValue(undefined), + } + + return { + ...actual, + ...mockFunctions, + default: mockFunctions, + } +}) + +vi.mock("p-wait-for", () => ({ + default: vi.fn().mockImplementation(async () => Promise.resolve()), +})) + +vi.mock("vscode", () => { + const mockDisposable = { dispose: vi.fn() } + const mockEventEmitter = { event: vi.fn(), fire: vi.fn() } + + return { + TabInputTextDiff: vi.fn(), + CodeActionKind: { + QuickFix: { value: "quickfix" }, + RefactorRewrite: { value: "refactor.rewrite" }, + }, + window: { + createTextEditorDecorationType: vi.fn().mockReturnValue({ dispose: vi.fn() }), + visibleTextEditors: [], + tabGroups: { + all: [], + close: vi.fn(), + onDidChangeTabs: vi.fn(() => ({ dispose: vi.fn() })), + }, + showErrorMessage: vi.fn(), + }, + workspace: { + workspaceFolders: [ + { + uri: { fsPath: "/mock/workspace/path" }, + name: "mock-workspace", + index: 0, + }, + ], + createFileSystemWatcher: vi.fn(() => ({ + onDidCreate: vi.fn(() => mockDisposable), + onDidDelete: vi.fn(() => mockDisposable), + onDidChange: vi.fn(() => mockDisposable), + dispose: vi.fn(), + })), + fs: { + stat: vi.fn().mockResolvedValue({ type: 1 }), + }, + onDidSaveTextDocument: vi.fn(() => mockDisposable), + getConfiguration: vi.fn(() => ({ get: (key: string, defaultValue: any) => defaultValue })), + }, + env: { + uriScheme: "vscode", + language: "en", + }, + EventEmitter: vi.fn().mockImplementation(() => mockEventEmitter), + Disposable: { + from: vi.fn(), + }, + TabInputText: vi.fn(), + } +}) + +vi.mock("../../mentions", () => ({ + parseMentions: vi.fn().mockImplementation((text) => { + return Promise.resolve({ text: `processed: ${text}`, mode: undefined, contentBlocks: [] }) + }), + openMention: vi.fn(), + getLatestTerminalOutput: vi.fn(), +})) + +vi.mock("../../../integrations/misc/extract-text", () => ({ + extractTextFromFile: vi.fn().mockResolvedValue("Mock file content"), +})) + +vi.mock("../../environment/getEnvironmentDetails", () => ({ + getEnvironmentDetails: vi.fn().mockResolvedValue(""), +})) + +vi.mock("../../ignore/RooIgnoreController") + +vi.mock("../../condense", async (importOriginal) => { + const actual = (await importOriginal()) as any + return { + ...actual, + summarizeConversation: vi.fn().mockResolvedValue({ + messages: [{ role: "user", content: [{ type: "text", text: "continued" }], ts: Date.now() }], + summary: "summary", + cost: 0, + newContextTokens: 1, + }), + } +}) + +vi.mock("../../../utils/storage", () => ({ + getTaskDirectoryPath: vi + .fn() + .mockImplementation((globalStoragePath, taskId) => Promise.resolve(`${globalStoragePath}/tasks/${taskId}`)), + getSettingsDirectoryPath: vi + .fn() + .mockImplementation((globalStoragePath) => Promise.resolve(`${globalStoragePath}/settings`)), +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockReturnValue(false), +})) + +// --------------------------------------------------------------------------- + +describe("rewind-integration", () => { + let mockProvider: any + + beforeEach(() => { + initializeTelemetry() + const ctx = createMockExtensionContext() + mockProvider = createMockProvider(ctx) + }) + + it("abort → resume produces clean state", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + // Set up clineMessages with several asks and an in-progress tool + task.clineMessages = [ + { ts: now - 6000, type: "say", say: "text", text: "user task" }, + { ts: now - 5000, type: "ask", ask: "followup", text: "what should I do?" }, + { ts: now - 4000, type: "say", say: "user_feedback", text: "do it" }, + { ts: now - 3000, type: "ask", ask: "tool", text: "approve write_to_file" }, + { ts: now - 2000, type: "say", say: "text", text: "streaming partial response..." }, + { ts: now - 1000, type: "say", say: "api_req_started", text: "{}" }, + ] + + // Set up apiConversationHistory with an interrupted assistant message + task.apiConversationHistory = [ + { + role: "user", + content: [{ type: "text", text: "do something" }], + ts: now - 7000, + }, + { + role: "assistant", + content: [{ type: "text", text: "What should I do?" }], + ts: now - 6000, + }, + { + role: "user", + content: [{ type: "text", text: "do it" }], + ts: now - 5000, + }, + { + role: "assistant", + content: [ + { type: "text", text: "I'll edit the file" }, + { + type: "tool_use", + id: "tool-write-1", + name: "write_to_file", + input: { path: "test.ts", content: "hello" }, + }, + ], + ts: now - 4000, + }, + ] as any[] + + // Abort the task (calls rewindToLastAskPoint internally) + await (task as any).abortTask() + + // After abort + rewind, clineMessages should be truncated to the last valid ask + // The last qualifying ask is "tool" at index 3 + expect(task.clineMessages).toHaveLength(4) + expect(task.clineMessages[3].ask).toBe("tool") + + // apiConversationHistory: the trailing assistant message with unanswered tool_use + // should be removed. Should have 3 messages: user, assistant(text-only), user + expect(task.apiConversationHistory).toHaveLength(3) + expect(task.apiConversationHistory[2].role).toBe("user") + }) + + it("completed task: getPendingAttemptCompletionToolUseId returns tool_use ID", () => { + const task = createTask(mockProvider) + const now = Date.now() + + // Set up apiConversationHistory where the last assistant message has attempt_completion + task.apiConversationHistory = [ + { + role: "user", + content: [{ type: "text", text: "build a hello world app" }], + ts: now - 5000, + }, + { + role: "assistant", + content: [ + { type: "text", text: "I've built the app." }, + { + type: "tool_use", + id: "toolu_completion_123", + name: "attempt_completion", + input: { result: "Here is the completed app." }, + }, + ], + ts: now - 4000, + }, + ] as any[] + + const pendingId: string | undefined = (task as any).getPendingAttemptCompletionToolUseId() + expect(pendingId).toBe("toolu_completion_123") + }) + + it("double rewind is idempotent", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 5000, type: "say", say: "text", text: "user task" }, + { ts: now - 4000, type: "ask", ask: "tool", text: "approve write_to_file" }, + { ts: now - 3000, type: "say", say: "text", text: "streaming response..." }, + { ts: now - 2000, type: "say", say: "api_req_started", text: "{}" }, + ] + + task.apiConversationHistory = [ + { + role: "user", + content: [{ type: "text", text: "do something" }], + ts: now - 6000, + }, + { + role: "assistant", + content: [ + { + type: "tool_use", + id: "tool-1", + name: "write_to_file", + input: { path: "test.ts", content: "hello" }, + }, + ], + ts: now - 5000, + }, + ] as any[] + + // First rewind + const result1: boolean = await (task as any).rewindToLastAskPoint() + expect(result1).toBe(true) + + const clineMessagesAfterFirst: ClineMessage[] = [...task.clineMessages] + const apiHistoryAfterFirst: Anthropic.MessageParam[] = [...task.apiConversationHistory] + + // Second rewind + const result2: boolean = await (task as any).rewindToLastAskPoint() + expect(result2).toBe(true) + + // State should be identical after both rewinds + expect(task.clineMessages).toEqual(clineMessagesAfterFirst) + expect(task.apiConversationHistory).toEqual(apiHistoryAfterFirst) + }) + + it("rewind with no qualifying ask points returns false", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + // Only has command_output asks (non-qualifying) and partial asks + task.clineMessages = [ + { ts: now - 5000, type: "say", say: "text", text: "user task" }, + { ts: now - 4000, type: "ask", ask: "command_output", text: "some output" }, + { ts: now - 3000, type: "ask", ask: "command_output", text: "more output" }, + { ts: now - 2000, type: "ask", ask: "tool", text: "partial ask", partial: true }, + ] + + task.apiConversationHistory = [] + + const result: boolean = await (task as any).rewindToLastAskPoint() + expect(result).toBe(false) + }) + + // ----------------------------------------------------------------------- + // Task C — completion → restart → resume scenarios + // ----------------------------------------------------------------------- + + it("completion → restart → resume with follow-up text routes as tool_result", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + // Simulate persisted state after an attempt_completion was presented. + const savedClineMessages: ClineMessage[] = [ + { ts: now - 5000, type: "say", say: "text", text: "user task" }, + { ts: now - 2000, type: "ask", ask: "completion_result", text: "Here is the completed app." }, + ] + + const savedApiHistory = [ + { + role: "user" as const, + content: [{ type: "text" as const, text: "build a hello world app" }], + ts: now - 5000, + }, + { + role: "assistant" as const, + content: [ + { type: "text" as const, text: "I've built the app." }, + { + type: "tool_use" as const, + id: "toolu_completion_abc", + name: "attempt_completion", + input: { result: "Here is the completed app." }, + }, + ], + ts: now - 4000, + }, + ] + + // Mock the persistence/loading methods so resumeTaskFromHistory can run + // without real disk I/O. + vi.spyOn(task as any, "getSavedClineMessages").mockImplementation(async () => + savedClineMessages.map((m) => ({ ...m })), + ) + vi.spyOn(task as any, "getSavedApiConversationHistory").mockResolvedValue( + savedApiHistory.map((m) => ({ ...m, content: [...(m.content as any[])] })), + ) + vi.spyOn(task as any, "overwriteClineMessages").mockImplementation(async (msgs: any) => { + task.clineMessages = msgs + }) + + // Simulate user providing follow-up text via "New Task" / message response + const askSpy = vi.spyOn(task as any, "ask").mockResolvedValue({ + response: "messageResponse", + text: "looks good, thanks!", + images: [], + }) + vi.spyOn(task as any, "say").mockResolvedValue(undefined) + const loopSpy = vi.spyOn(task as any, "initiateTaskLoop").mockResolvedValue(undefined) + + await (task as any).resumeTaskFromHistory() + + // Should present as resume_completed_task (not resume_task) + expect(askSpy).toHaveBeenCalledWith("resume_completed_task") + + // The follow-up should be routed as a tool_result for the pending + // attempt_completion tool_use — NOT as a bare text block. + expect(loopSpy).toHaveBeenCalledTimes(1) + const sentContent = loopSpy.mock.calls[0][0] as any[] + expect(sentContent).toHaveLength(1) + expect(sentContent[0].type).toBe("tool_result") + expect(sentContent[0].tool_use_id).toBe("toolu_completion_abc") + expect(sentContent[0].content).toContain("looks good, thanks!") + }) + + it("completion → restart → resume without follow-up text injects completed status", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + const savedClineMessages: ClineMessage[] = [ + { ts: now - 5000, type: "say", say: "text", text: "user task" }, + { ts: now - 2000, type: "ask", ask: "completion_result", text: "Done." }, + ] + + const savedApiHistory = [ + { + role: "user" as const, + content: [{ type: "text" as const, text: "build it" }], + ts: now - 5000, + }, + { + role: "assistant" as const, + content: [ + { type: "text" as const, text: "Done." }, + { + type: "tool_use" as const, + id: "toolu_completion_xyz", + name: "attempt_completion", + input: { result: "Done." }, + }, + ], + ts: now - 4000, + }, + ] + + vi.spyOn(task as any, "getSavedClineMessages").mockImplementation(async () => + savedClineMessages.map((m) => ({ ...m })), + ) + vi.spyOn(task as any, "getSavedApiConversationHistory").mockResolvedValue( + savedApiHistory.map((m) => ({ ...m, content: [...(m.content as any[])] })), + ) + vi.spyOn(task as any, "overwriteClineMessages").mockImplementation(async (msgs: any) => { + task.clineMessages = msgs + }) + + // Simulate user clicking "continue" without providing any text + vi.spyOn(task as any, "ask").mockResolvedValue({ + response: "yesButtonClicked", + text: undefined, + images: undefined, + }) + vi.spyOn(task as any, "say").mockResolvedValue(undefined) + const loopSpy = vi.spyOn(task as any, "initiateTaskLoop").mockResolvedValue(undefined) + + await (task as any).resumeTaskFromHistory() + + // Should inject a {"status":"completed"} tool_result as fallback + expect(loopSpy).toHaveBeenCalledTimes(1) + const sentContent = loopSpy.mock.calls[0][0] as any[] + expect(sentContent).toHaveLength(1) + expect(sentContent[0].type).toBe("tool_result") + expect(sentContent[0].tool_use_id).toBe("toolu_completion_xyz") + + const parsedContent = JSON.parse(sentContent[0].content) + expect(parsedContent.status).toBe("completed") + }) + + it("attempt_completion alongside another tool_use in same assistant message", () => { + const task = createTask(mockProvider) + const now = Date.now() + + // Edge case: assistant message contains both a read_file tool_use and + // an attempt_completion tool_use. getPendingAttemptCompletionToolUseId + // should still detect the attempt_completion. + task.apiConversationHistory = [ + { + role: "user", + content: [{ type: "text", text: "build and verify the app" }], + ts: now - 5000, + }, + { + role: "assistant", + content: [ + { type: "text", text: "I'll verify and complete." }, + { + type: "tool_use", + id: "toolu_read_999", + name: "read_file", + input: { path: "package.json" }, + }, + { + type: "tool_use", + id: "toolu_completion_888", + name: "attempt_completion", + input: { result: "App is ready." }, + }, + ], + ts: now - 4000, + }, + ] as any[] + + const pendingId: string | undefined = (task as any).getPendingAttemptCompletionToolUseId() + expect(pendingId).toBe("toolu_completion_888") + }) +}) diff --git a/src/core/task/__tests__/rewindToLastAskPoint.spec.ts b/src/core/task/__tests__/rewindToLastAskPoint.spec.ts new file mode 100644 index 00000000000..90380693f5c --- /dev/null +++ b/src/core/task/__tests__/rewindToLastAskPoint.spec.ts @@ -0,0 +1,493 @@ +// npx vitest run core/task/__tests__/rewindToLastAskPoint.spec.ts + +import { + initializeTelemetry, + createMockExtensionContext, + createMockProvider, + createTask, +} from "./fixtures/rewind-test-setup" + +// --------------------------------------------------------------------------- +// vi.mock() declarations — must live in the test file (vitest hoists them). +// --------------------------------------------------------------------------- + +vi.mock("delay", () => ({ + __esModule: true, + default: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("uuid", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + v7: vi.fn(() => "00000000-0000-7000-8000-000000000000"), + } +}) + +vi.mock("execa", () => ({ + execa: vi.fn(), +})) + +vi.mock("fs/promises", async (importOriginal) => { + const actual = (await importOriginal()) as Record + const mockFunctions = { + mkdir: vi.fn().mockResolvedValue(undefined), + writeFile: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockImplementation((filePath) => { + if (filePath.includes("ui_messages.json")) { + return Promise.resolve(JSON.stringify([])) + } + if (filePath.includes("api_conversation_history.json")) { + return Promise.resolve(JSON.stringify([])) + } + return Promise.resolve("[]") + }), + unlink: vi.fn().mockResolvedValue(undefined), + rmdir: vi.fn().mockResolvedValue(undefined), + } + + return { + ...actual, + ...mockFunctions, + default: mockFunctions, + } +}) + +vi.mock("p-wait-for", () => ({ + default: vi.fn().mockImplementation(async () => Promise.resolve()), +})) + +vi.mock("vscode", () => { + const mockDisposable = { dispose: vi.fn() } + const mockEventEmitter = { event: vi.fn(), fire: vi.fn() } + + return { + TabInputTextDiff: vi.fn(), + CodeActionKind: { + QuickFix: { value: "quickfix" }, + RefactorRewrite: { value: "refactor.rewrite" }, + }, + window: { + createTextEditorDecorationType: vi.fn().mockReturnValue({ dispose: vi.fn() }), + visibleTextEditors: [], + tabGroups: { + all: [], + close: vi.fn(), + onDidChangeTabs: vi.fn(() => ({ dispose: vi.fn() })), + }, + showErrorMessage: vi.fn(), + }, + workspace: { + workspaceFolders: [ + { + uri: { fsPath: "/mock/workspace/path" }, + name: "mock-workspace", + index: 0, + }, + ], + createFileSystemWatcher: vi.fn(() => ({ + onDidCreate: vi.fn(() => mockDisposable), + onDidDelete: vi.fn(() => mockDisposable), + onDidChange: vi.fn(() => mockDisposable), + dispose: vi.fn(), + })), + fs: { + stat: vi.fn().mockResolvedValue({ type: 1 }), + }, + onDidSaveTextDocument: vi.fn(() => mockDisposable), + getConfiguration: vi.fn(() => ({ get: (key: string, defaultValue: any) => defaultValue })), + }, + env: { + uriScheme: "vscode", + language: "en", + }, + EventEmitter: vi.fn().mockImplementation(() => mockEventEmitter), + Disposable: { + from: vi.fn(), + }, + TabInputText: vi.fn(), + } +}) + +vi.mock("../../mentions", () => ({ + parseMentions: vi.fn().mockImplementation((text) => { + return Promise.resolve({ text: `processed: ${text}`, mode: undefined, contentBlocks: [] }) + }), + openMention: vi.fn(), + getLatestTerminalOutput: vi.fn(), +})) + +vi.mock("../../../integrations/misc/extract-text", () => ({ + extractTextFromFile: vi.fn().mockResolvedValue("Mock file content"), +})) + +vi.mock("../../environment/getEnvironmentDetails", () => ({ + getEnvironmentDetails: vi.fn().mockResolvedValue(""), +})) + +vi.mock("../../ignore/RooIgnoreController") + +vi.mock("../../condense", async (importOriginal) => { + const actual = (await importOriginal()) as any + return { + ...actual, + summarizeConversation: vi.fn().mockResolvedValue({ + messages: [{ role: "user", content: [{ type: "text", text: "continued" }], ts: Date.now() }], + summary: "summary", + cost: 0, + newContextTokens: 1, + }), + } +}) + +vi.mock("../../../utils/storage", () => ({ + getTaskDirectoryPath: vi + .fn() + .mockImplementation((globalStoragePath, taskId) => Promise.resolve(`${globalStoragePath}/tasks/${taskId}`)), + getSettingsDirectoryPath: vi + .fn() + .mockImplementation((globalStoragePath) => Promise.resolve(`${globalStoragePath}/settings`)), +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockReturnValue(false), +})) + +// --------------------------------------------------------------------------- + +describe("rewindToLastAskPoint", () => { + let mockProvider: any + + beforeEach(() => { + initializeTelemetry() + const ctx = createMockExtensionContext() + mockProvider = createMockProvider(ctx) + }) + + it("should return false when no ask point exists", async () => { + const task = createTask(mockProvider) + + // Empty clineMessages + task.clineMessages = [] + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(false) + }) + + it("should truncate clineMessages to last ask point", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 5000, type: "say", say: "text", text: "user task" }, + { ts: now - 4000, type: "ask", ask: "tool", text: "approve write_to_file" }, + { ts: now - 3000, type: "say", say: "text", text: "streaming response..." }, + { ts: now - 2000, type: "say", say: "api_req_started", text: "{}" }, + ] + + task.apiConversationHistory = [] + + const overwriteClineSpy = vi.spyOn(task as any, "overwriteClineMessages") + const overwriteApiSpy = vi.spyOn(task as any, "overwriteApiConversationHistory") + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(true) + + // Should truncate to include the ask at index 1 + expect(task.clineMessages).toHaveLength(2) + expect(task.clineMessages[1].ask).toBe("tool") + + // Verify persistence was triggered with the correct truncated data + expect(overwriteClineSpy).toHaveBeenCalledWith(task.clineMessages) + expect(overwriteApiSpy).toHaveBeenCalledWith(task.apiConversationHistory) + }) + + it("should skip non-blocking asks (command_output)", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 5000, type: "say", say: "text", text: "user task" }, + { ts: now - 4000, type: "ask", ask: "followup", text: "what do you want?" }, + { ts: now - 3000, type: "say", say: "user_feedback", text: "this response" }, + { ts: now - 2000, type: "ask", ask: "command_output", text: "output data" }, + ] + + task.apiConversationHistory = [] + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(true) + + // Should rewind to the "followup" ask, skipping command_output + expect(task.clineMessages).toHaveLength(2) + expect(task.clineMessages[1].ask).toBe("followup") + }) + + it("should skip resume_task and resume_completed_task asks", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 5000, type: "say", say: "text", text: "user task" }, + { ts: now - 4000, type: "ask", ask: "completion_result", text: "done" }, + { ts: now - 3000, type: "ask", ask: "resume_completed_task", text: "" }, + ] + + task.apiConversationHistory = [] + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(true) + + // Should rewind to the "completion_result" ask, skipping resume_completed_task + expect(task.clineMessages).toHaveLength(2) + expect(task.clineMessages[1].ask).toBe("completion_result") + }) + + it("should remove trailing assistant message with unanswered tool_use", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 3000, type: "say", say: "text", text: "user task" }, + { ts: now - 2000, type: "ask", ask: "tool", text: "approve write_to_file" }, + ] + + task.apiConversationHistory = [ + { + role: "user", + content: [{ type: "text", text: "do something" }], + ts: now - 4000, + }, + { + role: "assistant", + content: [ + { type: "text", text: "I'll edit the file" }, + { + type: "tool_use", + id: "tool-1", + name: "write_to_file", + input: { path: "test.ts", content: "hello" }, + }, + ], + ts: now - 3000, + }, + ] as any[] + + const overwriteClineSpy = vi.spyOn(task as any, "overwriteClineMessages") + const overwriteApiSpy = vi.spyOn(task as any, "overwriteApiConversationHistory") + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(true) + + // Should remove the assistant message with unanswered tool_use + expect(task.apiConversationHistory).toHaveLength(1) + expect(task.apiConversationHistory[0].role).toBe("user") + + // Verify persistence was triggered with the correct truncated data + expect(overwriteClineSpy).toHaveBeenCalledWith(task.clineMessages) + expect(overwriteApiSpy).toHaveBeenCalledWith(task.apiConversationHistory) + }) + + it("should keep assistant message with text-only content", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 3000, type: "say", say: "text", text: "user task" }, + { ts: now - 2000, type: "ask", ask: "followup", text: "what next?" }, + ] + + task.apiConversationHistory = [ + { + role: "user", + content: [{ type: "text", text: "do something" }], + ts: now - 4000, + }, + { + role: "assistant", + content: [{ type: "text", text: "What would you like me to do?" }], + ts: now - 3000, + }, + ] as any[] + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(true) + + // Should keep the text-only assistant message + expect(task.apiConversationHistory).toHaveLength(2) + expect(task.apiConversationHistory[1].role).toBe("assistant") + }) + + it("should remove incomplete user message and its assistant message", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 3000, type: "say", say: "text", text: "user task" }, + { ts: now - 2000, type: "ask", ask: "tool", text: "approve next op" }, + ] + + // Scenario: assistant made 2 tool calls, user only answered 1 + task.apiConversationHistory = [ + { + role: "user", + content: [{ type: "text", text: "do something" }], + ts: now - 5000, + }, + { + role: "assistant", + content: [ + { + type: "tool_use", + id: "tool-1", + name: "read_file", + input: { path: "test.ts" }, + }, + { + type: "tool_use", + id: "tool-2", + name: "write_to_file", + input: { path: "test.ts", content: "hello" }, + }, + ], + ts: now - 4000, + }, + { + role: "user", + content: [ + { + type: "tool_result", + tool_use_id: "tool-1", + content: "file contents", + }, + // tool-2 tool_result is missing + ], + ts: now - 3000, + }, + ] as any[] + + const overwriteClineSpy = vi.spyOn(task as any, "overwriteClineMessages") + const overwriteApiSpy = vi.spyOn(task as any, "overwriteApiConversationHistory") + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(true) + + // Should remove both the incomplete user message and the assistant message + expect(task.apiConversationHistory).toHaveLength(1) + expect(task.apiConversationHistory[0].role).toBe("user") + + // Verify persistence was triggered with the correct truncated data + expect(overwriteClineSpy).toHaveBeenCalledWith(task.clineMessages) + expect(overwriteApiSpy).toHaveBeenCalledWith(task.apiConversationHistory) + }) + + it("should not remove user message when all tool_results are present", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 3000, type: "say", say: "text", text: "user task" }, + { ts: now - 2000, type: "ask", ask: "tool", text: "approve next op" }, + ] + + task.apiConversationHistory = [ + { + role: "user", + content: [{ type: "text", text: "do something" }], + ts: now - 5000, + }, + { + role: "assistant", + content: [ + { + type: "tool_use", + id: "tool-1", + name: "read_file", + input: { path: "test.ts" }, + }, + ], + ts: now - 4000, + }, + { + role: "user", + content: [ + { + type: "tool_result", + tool_use_id: "tool-1", + content: "file contents", + }, + ], + ts: now - 3000, + }, + ] as any[] + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(true) + + // All tool_results present - should keep everything + expect(task.apiConversationHistory).toHaveLength(3) + }) + + it("should handle empty apiConversationHistory gracefully", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 3000, type: "say", say: "text", text: "user task" }, + { ts: now - 2000, type: "ask", ask: "followup", text: "question" }, + ] + + task.apiConversationHistory = [] + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(true) + + // clineMessages should still be truncated + expect(task.clineMessages).toHaveLength(2) + // apiConversationHistory should remain empty + expect(task.apiConversationHistory).toHaveLength(0) + }) + + it("should skip partial ask messages", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 5000, type: "say", say: "text", text: "user task" }, + { ts: now - 4000, type: "ask", ask: "tool", text: "approve this" }, + { ts: now - 3000, type: "ask", ask: "command", text: "run this", partial: true }, + ] + + task.apiConversationHistory = [] + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(true) + + // Should skip the partial ask and rewind to the completed "tool" ask + expect(task.clineMessages).toHaveLength(2) + expect(task.clineMessages[1].ask).toBe("tool") + }) + + it("should return false when overwriteClineMessages throws an error", async () => { + const task = createTask(mockProvider) + const now = Date.now() + + task.clineMessages = [ + { ts: now - 5000, type: "say", say: "text", text: "user task" }, + { ts: now - 4000, type: "ask", ask: "tool", text: "approve write_to_file" }, + { ts: now - 3000, type: "say", say: "text", text: "streaming response..." }, + ] + + task.apiConversationHistory = [] + + // Make overwriteClineMessages throw to simulate a disk write failure + const overwriteSpy = vi + .spyOn(task as any, "overwriteClineMessages") + .mockRejectedValue(new Error("disk write failed")) + + const result = await (task as any).rewindToLastAskPoint() + expect(result).toBe(false) + + overwriteSpy.mockRestore() + }) +}) diff --git a/src/core/task/__tests__/validateToolResultIds.spec.ts b/src/core/task/__tests__/validateToolResultIds.spec.ts index 0926e899aad..a87f8128032 100644 --- a/src/core/task/__tests__/validateToolResultIds.spec.ts +++ b/src/core/task/__tests__/validateToolResultIds.spec.ts @@ -613,7 +613,7 @@ describe("validateAndFixToolResultIds", () => { expect(resultContent.length).toBe(2) // The missing tool_result is prepended expect(resultContent[0].tool_use_id).toBe("tool-2") - expect(resultContent[0].content).toBe("Tool execution was interrupted before completion.") + expect(resultContent[0].content).toBe("No result available for this tool call.") // The original is fixed expect(resultContent[1].tool_use_id).toBe("tool-1") }) @@ -652,7 +652,7 @@ describe("validateAndFixToolResultIds", () => { expect(resultContent[0].type).toBe("tool_result") expect((resultContent[0] as Anthropic.ToolResultBlockParam).tool_use_id).toBe("tool-123") expect((resultContent[0] as Anthropic.ToolResultBlockParam).content).toBe( - "Tool execution was interrupted before completion.", + "No result available for this tool call.", ) // Original text block should be preserved expect(resultContent[1].type).toBe("text") @@ -738,7 +738,7 @@ describe("validateAndFixToolResultIds", () => { expect(resultContent.length).toBe(2) // Missing tool_result for tool-2 should be prepended expect(resultContent[0].tool_use_id).toBe("tool-2") - expect(resultContent[0].content).toBe("Tool execution was interrupted before completion.") + expect(resultContent[0].content).toBe("No result available for this tool call.") // Existing tool_result should be preserved expect(resultContent[1].tool_use_id).toBe("tool-1") expect(resultContent[1].content).toBe("Content for tool 1") @@ -769,7 +769,7 @@ describe("validateAndFixToolResultIds", () => { expect(resultContent.length).toBe(1) expect(resultContent[0].type).toBe("tool_result") expect(resultContent[0].tool_use_id).toBe("tool-1") - expect(resultContent[0].content).toBe("Tool execution was interrupted before completion.") + expect(resultContent[0].content).toBe("No result available for this tool call.") }) }) diff --git a/src/core/task/validateToolResultIds.ts b/src/core/task/validateToolResultIds.ts index a966d429ed5..2e886bf0c80 100644 --- a/src/core/task/validateToolResultIds.ts +++ b/src/core/task/validateToolResultIds.ts @@ -220,7 +220,7 @@ export function validateAndFixToolResultIds( const missingToolResults: Anthropic.ToolResultBlockParam[] = stillMissingToolUseIds.map((toolUse) => ({ type: "tool_result" as const, tool_use_id: toolUse.id, - content: "Tool execution was interrupted before completion.", + content: "No result available for this tool call.", })) // Insert missing tool_results at the beginning of the content array diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index a406a15c8b4..e473842487c 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -137,16 +137,40 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { const { response, text, images } = await task.ask("completion_result", "", false) if (response === "yesButtonClicked") { + // User accepted completion. Don't push a tool_result yet. + // Instead, wait for a potential follow-up message by asking + // resume_completed_task. This keeps the task alive so any + // subsequent user message routes as a tool_result for this + // attempt_completion tool_use (not as a bare user message). + const resume = await task.ask("resume_completed_task") + + if (resume.response === "messageResponse" && resume.text) { + // User came back with a follow-up message. + // Push it as the tool_result for attempt_completion. + await task.say("user_feedback", resume.text, resume.images) + const feedbackText = `\n${resume.text}\n` + pushToolResult(formatResponse.toolResult(feedbackText, resume.images)) + return + } + + // User didn't provide text (or task was aborted). + // Push a "completed" tool_result to close the tool contract. + pushToolResult( + JSON.stringify({ + status: "completed", + message: "The user is satisfied with the result.", + }), + ) return } - // User provided feedback - push tool result to continue the conversation + // User provided feedback directly (typed text in the completion UI) await task.say("user_feedback", text ?? "", images) const feedbackText = `\n${text}\n` pushToolResult(formatResponse.toolResult(feedbackText, images)) } catch (error) { - await handleError("inspecting site", error as Error) + await handleError("completing task", error as Error) } } diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 9aac6296c6c..e0f0e8bf7cf 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -1,4 +1,5 @@ import { TodoItem } from "@roo-code/types" +import { TelemetryService } from "@roo-code/telemetry" import { AttemptCompletionToolUse } from "../../../shared/tools" @@ -6,6 +7,8 @@ import { AttemptCompletionToolUse } from "../../../shared/tools" vi.mock("../../prompts/responses", () => ({ formatResponse: { toolError: vi.fn((msg: string) => `Error: ${msg}`), + toolResult: vi.fn((text: string, images?: string[]) => text), + imageBlocks: vi.fn((images?: string[]) => []), }, })) @@ -39,6 +42,10 @@ describe("attemptCompletionTool", () => { let mockGetConfiguration: ReturnType beforeEach(() => { + if (!TelemetryService.hasInstance()) { + TelemetryService.createInstance([]) + } + mockPushToolResult = vi.fn() mockAskApproval = vi.fn() mockHandleError = vi.fn() @@ -469,4 +476,158 @@ describe("attemptCompletionTool", () => { }) }) }) + + describe("yesButtonClicked completion acceptance", () => { + it("should wait for follow-up after yesButtonClicked and push 'completed' if no text", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "Task completed successfully" }, + nativeArgs: { result: "Task completed successfully" }, + partial: false, + } + + mockTask.todoList = undefined + mockTask.didToolFailInCurrentTurn = false + + // First ask returns yesButtonClicked (completion_result), + // second ask returns yesButtonClicked with no text (resume_completed_task) + mockTask.ask = vi + .fn() + .mockResolvedValueOnce({ response: "yesButtonClicked", text: "", images: [] }) + .mockResolvedValueOnce({ response: "yesButtonClicked", text: "", images: [] }) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + // handleError should NOT have been called + expect(mockHandleError).not.toHaveBeenCalled() + + // ask should have been called twice: completion_result + resume_completed_task + expect(mockTask.ask).toHaveBeenCalledTimes(2) + expect(mockTask.ask).toHaveBeenNthCalledWith(1, "completion_result", "", false) + expect(mockTask.ask).toHaveBeenNthCalledWith(2, "resume_completed_task") + + // Should push a "completed" tool_result + expect(mockPushToolResult).toHaveBeenCalledTimes(1) + const toolResultArg = mockPushToolResult.mock.calls[0][0] + const parsed = JSON.parse(toolResultArg) + expect(parsed.status).toBe("completed") + }) + + it("should push follow-up text as tool_result when user returns with a message", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "Task completed" }, + nativeArgs: { result: "Task completed" }, + partial: false, + } + + mockTask.todoList = undefined + mockTask.didToolFailInCurrentTurn = false + + // First ask returns yesButtonClicked (completion_result), + // second ask returns messageResponse with follow-up text (resume_completed_task) + mockTask.ask = vi + .fn() + .mockResolvedValueOnce({ response: "yesButtonClicked", text: "", images: [] }) + .mockResolvedValueOnce({ response: "messageResponse", text: "Actually, also fix Y", images: [] }) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + // Should push a tool_result with the follow-up text + expect(mockPushToolResult).toHaveBeenCalledTimes(1) + const toolResultArg = mockPushToolResult.mock.calls[0][0] + expect(toolResultArg).toContain("Actually, also fix Y") + }) + + it("should push tool_result with user feedback when user provides direct feedback", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "Task completed" }, + nativeArgs: { result: "Task completed" }, + partial: false, + } + + mockTask.todoList = undefined + mockTask.didToolFailInCurrentTurn = false + + // Simulate user providing feedback directly (messageResponse on first ask) + mockTask.ask = vi + .fn() + .mockResolvedValue({ response: "messageResponse", text: "Please also fix X", images: [] }) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + // Only one ask call - user provided feedback directly + expect(mockTask.ask).toHaveBeenCalledTimes(1) + + // Should push a tool_result with the user's feedback + expect(mockPushToolResult).toHaveBeenCalledTimes(1) + const toolResultArg = mockPushToolResult.mock.calls[0][0] + expect(toolResultArg).toContain("Please also fix X") + }) + + it("should fall through to error handler when resume_completed_task ask throws (abort)", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "Task completed" }, + nativeArgs: { result: "Task completed" }, + partial: false, + } + + mockTask.todoList = undefined + mockTask.didToolFailInCurrentTurn = false + + // First ask returns yesButtonClicked (completion_result), + // second ask throws (simulating task abort during resume_completed_task) + mockTask.ask = vi + .fn() + .mockResolvedValueOnce({ response: "yesButtonClicked", text: "", images: [] }) + .mockRejectedValueOnce(new Error("Task was aborted")) + + const callbacks: AttemptCompletionCallbacks = { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + } + + await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + + // ask should have been called twice: completion_result + resume_completed_task (which threw) + expect(mockTask.ask).toHaveBeenCalledTimes(2) + + // handleError should have been called with "completing task" + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("completing task", expect.any(Error)) + }) + }) }) diff --git a/src/shared/__tests__/messages.spec.ts b/src/shared/__tests__/messages.spec.ts new file mode 100644 index 00000000000..e82ce425762 --- /dev/null +++ b/src/shared/__tests__/messages.spec.ts @@ -0,0 +1,40 @@ +import { Anthropic } from "@anthropic-ai/sdk" +import { normalizeContentBlocks } from "../messages" + +describe("normalizeContentBlocks", () => { + it("returns the array unchanged when content is already an array", () => { + const blocks: Anthropic.Messages.ContentBlockParam[] = [ + { type: "text", text: "hello" }, + { type: "tool_use", id: "tool-1", name: "read_file", input: { path: "a.ts" } }, + ] + + const result = normalizeContentBlocks(blocks) + expect(result).toBe(blocks) // same reference + }) + + it("wraps a plain string in a single text block", () => { + const result = normalizeContentBlocks("hello world") + expect(result).toEqual([{ type: "text", text: "hello world" }]) + }) + + it("wraps an empty string in a text block with empty text", () => { + const result = normalizeContentBlocks("") + expect(result).toEqual([{ type: "text", text: "" }]) + }) + + it("returns an empty array unchanged", () => { + const empty: Anthropic.Messages.ContentBlockParam[] = [] + const result = normalizeContentBlocks(empty) + expect(result).toBe(empty) + }) + + it("preserves tool_result blocks in the array", () => { + const blocks: Anthropic.Messages.ContentBlockParam[] = [ + { type: "tool_result", tool_use_id: "tool-1", content: "result" }, + ] + + const result = normalizeContentBlocks(blocks) + expect(result).toBe(blocks) + expect(result[0].type).toBe("tool_result") + }) +}) diff --git a/src/shared/messages.ts b/src/shared/messages.ts new file mode 100644 index 00000000000..5cc361df119 --- /dev/null +++ b/src/shared/messages.ts @@ -0,0 +1,14 @@ +import { Anthropic } from "@anthropic-ai/sdk" + +/** + * Normalize a message's `content` field into an array of content blocks. + * + * Anthropic's `MessageParam.content` can be either a plain string or an + * array of typed content blocks. This helper standardises the value to + * always be an array, wrapping a plain string in a single text block. + */ +export function normalizeContentBlocks( + content: string | Anthropic.Messages.ContentBlockParam[], +): Anthropic.Messages.ContentBlockParam[] { + return Array.isArray(content) ? content : [{ type: "text" as const, text: content ?? "" }] +} From 1f04bab83f46689611f8f1196f611e4921f52d15 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Fri, 6 Feb 2026 20:14:28 -0700 Subject: [PATCH 2/2] fix: guard getPendingAttemptCompletionToolUseId against multi-tool_use messages When the last assistant message contains attempt_completion alongside other tool_use blocks (e.g. read_file + attempt_completion) and the process crashes before any tool_result is saved, the method previously returned the attempt_completion ID. This caused resumeTaskFromHistory to bypass the rewind and treat the task as completed, while the other tool_uses never executed. Now returns undefined when other tool_use blocks are present alongside attempt_completion, allowing the rewind path to handle the cleanup correctly. Addresses review feedback from @roomote. --- src/core/task/Task.ts | 7 +++++++ src/core/task/__tests__/rewind-integration.spec.ts | 8 +++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index e2d2a303eb8..4f04d72397b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -2129,6 +2129,13 @@ export class Task extends EventEmitter implements TaskLike { block.type === "tool_use" && block.name === "attempt_completion", ) + // Only treat as pending completion if there are no other unanswered + // tool_use blocks — otherwise the rewind path is more appropriate. + if (toolUse) { + const hasOtherToolUse = content.some((block) => block.type === "tool_use" && block !== toolUse) + if (hasOtherToolUse) return undefined + } + return toolUse?.id } diff --git a/src/core/task/__tests__/rewind-integration.spec.ts b/src/core/task/__tests__/rewind-integration.spec.ts index d09848f287d..6d447461c90 100644 --- a/src/core/task/__tests__/rewind-integration.spec.ts +++ b/src/core/task/__tests__/rewind-integration.spec.ts @@ -456,13 +456,15 @@ describe("rewind-integration", () => { expect(parsedContent.status).toBe("completed") }) - it("attempt_completion alongside another tool_use in same assistant message", () => { + it("attempt_completion alongside another tool_use in same assistant message returns undefined", () => { const task = createTask(mockProvider) const now = Date.now() // Edge case: assistant message contains both a read_file tool_use and // an attempt_completion tool_use. getPendingAttemptCompletionToolUseId - // should still detect the attempt_completion. + // should return undefined so the rewind path is used instead — + // the other tool_uses never executed, so treating it as a pending + // completion would bypass the rewind and produce misleading context. task.apiConversationHistory = [ { role: "user", @@ -491,6 +493,6 @@ describe("rewind-integration", () => { ] as any[] const pendingId: string | undefined = (task as any).getPendingAttemptCompletionToolUseId() - expect(pendingId).toBe("toolu_completion_888") + expect(pendingId).toBeUndefined() }) })