diff --git a/packages/types/src/task.ts b/packages/types/src/task.ts index cac34cc6f2..57894d3e7c 100644 --- a/packages/types/src/task.ts +++ b/packages/types/src/task.ts @@ -127,7 +127,7 @@ export interface TaskLike { approveAsk(options?: { text?: string; images?: string[] }): void denyAsk(options?: { text?: string; images?: string[] }): void - submitUserMessage(text: string, images?: string[], mode?: string, providerProfile?: string): Promise + submitUserMessage(text: string, images?: string[], mode?: string, providerProfile?: string): Promise abortTask(): Promise } diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-auto-deny.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-auto-deny.spec.ts index 1a3cbcefd8..42dc767074 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-auto-deny.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-auto-deny.spec.ts @@ -81,6 +81,7 @@ interface MockTask { } } say: ReturnType + sayUserFeedbackAndAckQueued: ReturnType ask: ReturnType pushToolResultToUserContent: ReturnType getTaskMode: ReturnType @@ -119,6 +120,8 @@ function buildMockTask(): MockTask { }), }, say: vi.fn().mockResolvedValue(undefined), + // The merged askApproval routes feedback through the durable queued ack. + sayUserFeedbackAndAckQueued: vi.fn().mockResolvedValue(undefined), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), pushToolResultToUserContent: vi.fn(), getTaskMode: vi.fn().mockResolvedValue("code"), diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts index 7381b2c2d0..e1d8bfab4d 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts @@ -2,6 +2,7 @@ import type { Anthropic } from "@anthropic-ai/sdk" import { describe, it, expect, beforeEach, vi, type Mock } from "vitest" +import { providerIdentifiers } from "@roo-code/types" import { presentAssistantMessage } from "../presentAssistantMessage" import { validateToolUse } from "../../tools/validateToolUse" import { getModeBySlug } from "../../../shared/modes" @@ -39,6 +40,7 @@ vi.mock("@roo-code/telemetry", () => ({ instance: { captureToolUsage: vi.fn(), captureConsecutiveMistakeError: vi.fn(), + captureException: vi.fn(), captureEvent: vi.fn(), }, }, @@ -64,6 +66,7 @@ interface MockTask { api: { getModel: () => { id: string; info: Record } } recordToolUsage: ReturnType recordToolError: ReturnType + apiConfiguration?: { apiProvider: string } toolRepetitionDetector: { check: ReturnType } providerRef: { deref: () => @@ -74,6 +77,7 @@ interface MockTask { | undefined } say: ReturnType + sayUserFeedbackAndAckQueued: ReturnType ask: ReturnType pushToolResultToUserContent: ReturnType } @@ -83,6 +87,10 @@ describe("presentAssistantMessage - tool usage attribution", () => { beforeEach(() => { vi.clearAllMocks() + // clearAllMocks keeps queued one-shot implementations alive; tests that skip the + // validation arm would otherwise leak a mockImplementationOnce throw into a + // later test that runs the validated arm. + vi.mocked(validateToolUse).mockReset() vi.mocked(validateToolUse).mockImplementation(() => undefined) mockTask = { @@ -117,6 +125,7 @@ describe("presentAssistantMessage - tool usage attribution", () => { }), }, say: vi.fn().mockResolvedValue(undefined), + sayUserFeedbackAndAckQueued: vi.fn().mockResolvedValue(undefined), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), pushToolResultToUserContent: vi.fn(), } @@ -651,6 +660,113 @@ describe("presentAssistantMessage - tool usage attribution", () => { expect(mockTask.consecutiveMistakeCount).toBe(1) expect(mockTask.didAlreadyUseTool).toBe(false) }) + + it("routes MCP approval feedback through the queued-ack wrapper", async () => { + mockTask.providerRef = { + deref: () => ({ + getState: vi.fn().mockResolvedValue({ + mode: "code", + customModes: [], + }), + getMcpHub: () => ({ + findServerNameBySanitizedName: () => "my_server", + getAllServers: () => [ + { + name: "my_server", + tools: [{ name: "do_thing", enabledForPrompt: true }], + }, + ], + }), + }), + } + mockTask.assistantMessageContent = [ + { + type: "mcp_tool_use", + id: "call_native_mcp_feedback", + name: "mcp_my_server_do_thing", + serverName: "my_server", + toolName: "do_thing", + arguments: {}, + partial: false, + }, + ] + mockTask.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: "Careful with this server" }) + + await presentAssistantMessage(mockTask as unknown as Task) + + expect(mockTask.sayUserFeedbackAndAckQueued).toHaveBeenCalledExactlyOnceWith( + "Careful with this server", + undefined, + undefined, + ) + expect(mockTask.say).not.toHaveBeenCalledWith("user_feedback", expect.anything(), expect.anything()) + }) + + it("routes tool-repetition feedback through the queued-ack wrapper", async () => { + mockTask.toolRepetitionDetector.check = vi.fn().mockReturnValue({ + allowExecution: false, + askUser: { + messageKey: "mistake_limit_reached", + messageDetail: "The tool {toolName} was called consecutively without progress.", + }, + }) + mockTask.apiConfiguration = { apiProvider: providerIdentifiers.anthropic } + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "call_repetition_feedback", + name: "read_file", + params: { path: "a.txt" }, + nativeArgs: { path: "a.txt" }, + partial: false, + }, + ] + mockTask.ask = vi.fn().mockResolvedValue({ response: "messageResponse", text: "Try another approach" }) + + await presentAssistantMessage(mockTask as unknown as Task) + + expect(mockTask.sayUserFeedbackAndAckQueued).toHaveBeenCalledExactlyOnceWith( + "Try another approach", + undefined, + undefined, + ) + expect(mockTask.say).not.toHaveBeenCalledWith("user_feedback", expect.anything(), expect.anything()) + expect(mockTask.userMessageContent).toContainEqual( + expect.objectContaining({ + type: "text", + text: expect.stringContaining("Try another approach"), + }), + ) + }) + + it("acks tool-repetition feedback before it reaches the API turn", async () => { + mockTask.toolRepetitionDetector.check = vi.fn().mockReturnValue({ + allowExecution: false, + askUser: { + messageKey: "mistake_limit_reached", + messageDetail: "The tool {toolName} was called consecutively without progress.", + }, + }) + mockTask.apiConfiguration = { apiProvider: providerIdentifiers.anthropic } + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "call_repetition_feedback", + name: "read_file", + params: { path: "a.txt" }, + nativeArgs: { path: "a.txt" }, + partial: false, + }, + ] + mockTask.ask = vi.fn().mockResolvedValue({ response: "messageResponse", text: "Try another approach" }) + mockTask.sayUserFeedbackAndAckQueued = vi.fn().mockRejectedValue(new Error("persist failed")) + + await expect(presentAssistantMessage(mockTask as unknown as Task)).rejects.toThrow("persist failed") + + // The durable ack failed and the entry is re-queued for redelivery: + // the feedback must not already be part of the API turn. + expect(mockTask.userMessageContent).toHaveLength(0) + }) }) describe("undefined provider state", () => { 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 8cfaa10972..06ee989a88 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts @@ -62,6 +62,7 @@ describe("presentAssistantMessage - Unknown Tool Handling", () => { }), }, say: vi.fn().mockResolvedValue(undefined), + sayUserFeedbackAndAckQueued: vi.fn().mockResolvedValue(undefined), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), } diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index b11538a2c6..4a0350dc01 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -218,7 +218,7 @@ export async function presentAssistantMessage(cline: Task) { isProtected?: boolean, autoApprovalContext?: AutoApprovalContext, ) => { - const { response, text, images, autoDenyDetail } = await cline.ask( + const { response, text, images, queuedMessageId, autoDenyDetail } = await cline.ask( type, partialMessage, false, @@ -250,8 +250,8 @@ export async function presentAssistantMessage(cline: Task) { return false } + await cline.sayUserFeedbackAndAckQueued(text, images, queuedMessageId) if (text) { - await cline.say("user_feedback", text, images) pushToolResult(formatResponse.toolResult(formatResponse.toolDeniedWithFeedback(text), images)) } else { pushToolResult(formatResponse.toolDenied()) @@ -263,8 +263,8 @@ 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. + await cline.sayUserFeedbackAndAckQueued(text, images, queuedMessageId) if (text) { - await cline.say("user_feedback", text, images) approvalFeedback = { text, images } } @@ -787,12 +787,18 @@ export async function presentAssistantMessage(cline: Task) { // If execution is not allowed, notify user and break. if (!repetitionCheck.allowExecution && repetitionCheck.askUser) { // Handle repetition similar to mistake_limit_reached pattern. - const { response, text, images } = await cline.ask( + const { response, text, images, queuedMessageId } = await cline.ask( repetitionCheck.askUser.messageKey as ClineAsk, repetitionCheck.askUser.messageDetail.replace("{toolName}", block.name), ) if (response === "messageResponse") { + // Durable ack first: the feedback must reach history before + // the API turn. When the ack fails the entry is re-queued + // for redelivery, and the model must not already have seen + // the text (mirrors AttemptCompletionTool's ordering). + await cline.sayUserFeedbackAndAckQueued(text, images, queuedMessageId) + // Add user feedback to userContent. cline.userMessageContent.push( { @@ -801,9 +807,6 @@ export async function presentAssistantMessage(cline: Task) { }, ...formatResponse.imageBlocks(images), ) - - // Add user feedback to chat. - await cline.say("user_feedback", text, images) } // Track tool repetition in telemetry via PostHog exception tracking and event. diff --git a/src/core/message-queue/MessageQueueService.ts b/src/core/message-queue/MessageQueueService.ts index 85a2192217..c1e6b18b52 100644 --- a/src/core/message-queue/MessageQueueService.ts +++ b/src/core/message-queue/MessageQueueService.ts @@ -113,6 +113,23 @@ export class MessageQueueService extends EventEmitter { return this._messages.length === 0 } + /** + * Reserve a specific queued message by ID when it is still present and + * unclaimed. Lets a consumer hold a consumed entry through its durable ack + * so neither another ask nor a background drain can claim it again + * mid-persistence. + */ + public claimMessage(id: string): boolean { + if (this.claimedMessageIds.has(id)) { + return false + } + if (!this._messages.some((message) => message.id === id)) { + return false + } + this.claimedMessageIds.add(id) + return true + } + /** * Whether at least one queued message is still available to be claimed. * diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 4de2b84590..6f361641d7 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -163,6 +163,19 @@ const DEFAULT_USAGE_COLLECTION_TIMEOUT_MS = 5000 // 5 seconds export const MODEL_FETCH_TIMEOUT_MS = 5_000 const QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS = [250, 1_000, 4_000] as const +// Compares two queued-entry image lists by value. An edit replaces the entry's +// array with a fresh one, so identity alone cannot tell "edited to equal +// content" apart from "not edited". +function queuedEntryImagesEqual(a: string[] | undefined, b: string[] | undefined): boolean { + if (a === b) { + return true + } + if (!a || !b || a.length !== b.length) { + return false + } + return a.every((image, index) => image === b[index]) +} + type QueuedAskResolution = { response: ClineAskResponse; requiresDurableAck: boolean } /** @@ -225,12 +238,29 @@ function queuedResponseForAsk(type: ClineAsk, text?: string): QueuedAskResolutio // Malformed tool asks retain the existing approve-with-feedback behavior. } + // Approval by queued message is policy-gated downstream (blanket deny, + // drain-site re-check): the claim path may convert a queued message to + // yesButtonClicked, but a raw conversational submission can never be an + // approval on its own. return { response: "yesButtonClicked", requiresDurableAck: false } } if (type === "command" || type === "use_mcp_server") { + // Approval by queued message is policy-gated downstream (blanket deny, + // drain-site re-check): the claim path may convert a queued message to + // yesButtonClicked, but a raw conversational submission can never be an + // approval on its own. return { response: "yesButtonClicked", requiresDurableAck: false } } + if (type === "api_req_failed" || type === "auto_approval_max_req_reached") { + // Failure-gate retry prompts: any non-yes answer aborts the task, so a + // queued conversational message must not be converted into an answer — + // its text and images would be destroyed with no history row and a + // surprise abort. The message stays queued for the next conversational + // ask, like the approval-gating asks above. + return undefined + } + return { response: "messageResponse", requiresDurableAck: type === "completion_result" } } @@ -428,6 +458,11 @@ export class Task extends EventEmitter implements TaskLike { private askResponse?: ClineAskResponse private askResponseText?: string private askResponseImages?: string[] + // ID of the queued message that produced the current ask-response slot + // value (set only when a queued-message drain submitted it). Direct + // responses clear it, so Task.ask can tie consumption to message identity + // instead of matching response text/images. + private askResponseQueuedMessageId?: string /** * Structured detail of the automatic denial that resolved the current ask, * set when `checkAutoApproval` denies via policy (denylist, blanket @@ -488,6 +523,56 @@ export class Task extends EventEmitter implements TaskLike { // Message Queue Service public readonly messageQueueService: MessageQueueService private messageQueueStateChangedHandler: (() => void) | undefined + // Serializes queued-message drains: a drain claims and submits only after + // the previous drain's submission handoff finished, so two concurrent + // drains cannot submit different messages into the single ask-response + // slot (which would drop the earlier response after both messages were + // already removed from the queue). + private queuedMessageDrainChain: Promise = Promise.resolve() + // ID of the last drain-submitted queued message. A successful submit only + // posts into the pending ask-response slot (which records this ID); the + // message stays queued until an ask consumes that response. The consuming + // ask matches the slot's recorded ID against this one to decide whether the + // submission was consumed (hand the ID to the caller for a durable ack) or + // overwritten unconsumed (retain for a later ask). + private pendingSubmittedQueuedMessageId: string | undefined + // The asks currently inside Task.ask, tracked per ask so an exiting ask + // (including a re-entrant one from a Message listener) can never clear + // another ask's gate. A background drain consults queuedResponseForAsk + // against these gates so a queued conversational message can never answer + // an approval-gating ask, exactly matching the claim path's resolution + // gate. + private inFlightAskGates: Set<{ type: ClineAsk; text?: string }> | undefined + /** + * True while a raw queued submission must not be posted into the ask slot: + * an ask is in flight AND either its slot already carries a direct response, + * the ask type resolves to undefined (approval-gating failure gates), or the + * ask is an approval ask (command/use_mcp_server/tool). Approval asks are + * refused even though the claim path resolves them: a raw conversational + * post can never be an approval — it would deny-with-feedback without user + * intent — so only the policy-gated claim path (blanket deny, drain-site + * re-check) may convert a queued message there. Outside an in-flight ask the + * slot holds only stale residue the next ask clears, so between-turn + * submissions proceed. + */ + private inFlightAskBlocksQueuedSubmission(): boolean { + for (const gate of this.inFlightAskGates ?? []) { + if ( + this.askResponse !== undefined || + queuedResponseForAsk(gate.type, gate.text) === undefined || + gate.type === "command" || + gate.type === "use_mcp_server" || + gate.type === "tool" + ) { + return true + } + } + return false + } + // Association between a queued message ID and the user_feedback row its ack + // persisted. A redelivery after a partial save failure reconciles the same + // row instead of appending a duplicate feedback row. + private queuedFeedbackRows = new Map() // Streaming isWaitingForFirstChunk = false @@ -1021,24 +1106,217 @@ export class Task extends EventEmitter implements TaskLike { text?: string, images?: string[], ): Promise { - await this.say("user_feedback", text ?? "", images) - for (let attempt = 0; attempt <= QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS.length; attempt++) { - if (this.abort) { + let row = this.queuedFeedbackRows.get(messageId) + // The drain-side tracker only guards a submitted message against + // re-submission while an ask has not consumed it yet. Once persistence + // settles — success (entry removed), failure (entry re-queued), abort, + // or a failed row write — a stale tracker would block every later + // drain from resubmitting this message (and starve the messages + // behind it), so release it on every exit path. + const releasePendingTracker = () => { + if (this.pendingSubmittedQueuedMessageId === messageId) { + this.pendingSubmittedQueuedMessageId = undefined + } + } + // The entry stays claimed (and editable in the queue UI) for the whole + // ack, so its content can drift from the submission copy while saves + // retry. Snapshot the entry now and reconcile the row against it before + // every save; comparing against this snapshot rather than the row keeps + // the trimmed submission text from reading as an edit next to the + // untrimmed entry text. + const entrySnapshot = (() => { + const entry = this.messageQueueService.messages.find((queued) => queued.id === messageId) + return entry ? { text: entry.text, images: entry.images } : undefined + })() + try { + try { + if (row) { + // Redelivery after a partial save failure (e.g. the message file + // was written but metadata persistence failed): reconcile the same + // row instead of appending a duplicate feedback row. Awaited so a + // failed webview update reaches the catch below, which releases + // the queued message instead of acking over a stale row. + row.text = text ?? "" + row.images = images + await this.updateClineMessage(row) + } else { + row = { + ts: Date.now(), + type: "say", + say: "user_feedback", + text: text ?? "", + images, + } + // Mirrors say()'s interactive user_feedback append: bump + // lastMessageTs and let the retry loop below own persistence. + // The association is registered before the append: the append's + // only uncaught throw is a synchronously throwing Message + // listener, which runs after the row is already pushed, so a + // redelivery can reconcile the same row instead of appending a + // duplicate feedback row. + this.lastMessageTs = row.ts + this.queuedFeedbackRows.set(messageId, row) + await this.addToClineMessages(row) + } + for (let attempt = 0; attempt <= QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS.length; attempt++) { + if (this.abort || this.abandoned) { + this.messageQueueService.releaseMessage(messageId) + return false + } + if (entrySnapshot) { + // An edit during a failed-save backoff changed only the queue + // entry: fold it into the row before this attempt's save so the + // ack persists the edit instead of the stale submission copy. + await this.reconcileQueuedFeedbackRowWithQueueEntry(row, messageId, entrySnapshot) + } + if (await this.saveClineMessages()) { + // The save clones history up front, so an edit that lands + // while it is in flight is missing from the persisted copy. + // Confirm the entry still matches the row and repeat the save + // when it does not; only then may the entry be removed. + let confirmed = !entrySnapshot + while (entrySnapshot && !this.abort && !this.abandoned) { + if (!(await this.reconcileQueuedFeedbackRowWithQueueEntry(row, messageId, entrySnapshot))) { + confirmed = true + break + } + if (!(await this.saveClineMessages())) { + break + } + } + if (confirmed) { + this.queuedFeedbackRows.delete(messageId) + // The row is durably saved. A missing entry means the + // user deleted it during the ack (the webview remove + // handler ignores claims); that is not a save failure. + this.messageQueueService.removeMessage(messageId) + return true + } + } + if (attempt < QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS.length) { + // Interruptible backoff: an abort or abandonment during the wait + // resolves promptly (releasing the claim at the loop-top check) + // instead of retaining the task through the full delay. + await this.waitForQueuedFeedbackBackoff(QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS[attempt]) + } + } + console.error( + `[Task#persistQueuedFeedbackAndAcknowledge] Failed to durably save queued feedback ${messageId} after ${QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS.length + 1} attempts`, + ) this.messageQueueService.releaseMessage(messageId) return false + } catch (error) { + // A failed write must not leave the message claimed: release it so a + // later drain can redeliver it. (No-op when the drain path already + // released the claim.) The pending tracker is released here too: + // this catch rethrows past the finally below, which must not be + // the only place that clears it. + this.messageQueueService.releaseMessage(messageId) + releasePendingTracker() + throw error } - if (await this.saveClineMessages()) { - return this.messageQueueService.removeMessage(messageId) + } finally { + releasePendingTracker() + } + } + + /** + * Fold the latest content of a retained (claimed) queue entry into its + * in-flight feedback row. The queue UI accepts edits while the entry is + * claimed, so during a failed-save backoff an edit would otherwise change + * only the entry and be lost when the ack removes it. The row receives the + * trimmed text, mirroring what submitting the entry would have sent. + * Returns true when an edit was folded in. + */ + private async reconcileQueuedFeedbackRowWithQueueEntry( + row: ClineMessage, + messageId: string, + snapshot: { text: string; images?: string[] }, + ): Promise { + const latest = this.messageQueueService.messages.find((queued) => queued.id === messageId) + if (!latest) { + return false + } + if (latest.text === snapshot.text && queuedEntryImagesEqual(latest.images, snapshot.images)) { + return false + } + snapshot.text = latest.text + snapshot.images = latest.images + row.text = latest.text.trim() + row.images = latest.images + await this.updateClineMessage(row) + return true + } + + private waitForQueuedFeedbackBackoff(ms: number): Promise { + return new Promise((resolve) => { + const finish = () => { + clearTimeout(timer) + clearInterval(poll) + resolve() } - if (attempt < QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS.length) { - await delay(QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS[attempt]) + const timer = setTimeout(finish, ms) + const poll = setInterval(() => { + if (this.abort === true || this.abandoned === true) { + finish() + } + }, 50) + }) + } + + /** + * Persist the user feedback carried by an ask result. When the ask consumed + * a queued message (intercepted its drain submission), the queue entry is + * removed only after the feedback's history write succeeds; a failed write + * re-queues it. Consumers that surface returned feedback as user_feedback + * must call this instead of say("user_feedback", ...) so a consumed queued + * message is acked exactly once instead of being redelivered by a later + * drain or claim. + */ + public async sayUserFeedbackAndAckQueued( + text: string | undefined, + images: string[] | undefined, + queuedMessageId: string | undefined, + ): Promise { + if (queuedMessageId) { + const persisted = await this.persistQueuedFeedbackAndAcknowledge(queuedMessageId, text, images) + if (!persisted) { + throw new Error(`[Task] Failed to persist queued feedback ${queuedMessageId}`) } + return + } + if (text || images?.length) { + await this.say("user_feedback", text ?? "", images) + } + } + + /** + * Drop a queued message whose response this ask consumed without persisting + * feedback (the consumer only inspected the button response, e.g. retry and + * approval gates). Removes the entry without inventing a history write. + */ + public discardConsumedQueuedMessage(queuedMessageId: string | undefined): void { + if (queuedMessageId) { + this.messageQueueService.removeMessage(queuedMessageId) + } + } + + /** + * Apply a webview edit to a queued message. Besides updating the queue + * entry, the task-owned pending submission is updated in place: while the + * ask-response slot still carries that submission (no direct response has + * overwritten it), the edited text/images replace the original copy so the + * consuming ask returns the edited content instead of the stale one. + */ + public editQueuedMessage(queuedMessageId: string, text: string, images?: string[]): void { + this.messageQueueService.updateMessage(queuedMessageId, text, images) + if ( + this.pendingSubmittedQueuedMessageId === queuedMessageId && + this.askResponseQueuedMessageId === queuedMessageId + ) { + this.askResponseText = text + this.askResponseImages = images } - console.error( - `[Task#persistQueuedFeedbackAndAcknowledge] Failed to durably save queued feedback ${messageId} after ${QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS.length + 1} attempts`, - ) - this.messageQueueService.releaseMessage(messageId) - return false } /** @@ -1150,7 +1428,11 @@ export class Task extends EventEmitter implements TaskLike { private handleQueuedAskResponse(message: QueuedMessage, resolution: QueuedAskResolution): string | undefined { this.handleWebviewAskResponse(resolution.response, message.text, message.images) - if (resolution.requiresDurableAck) { + if (resolution.requiresDurableAck || (this.queuedFeedbackRows?.has(message.id) ?? false)) { + // A registered feedback row means an earlier delivery attempt left + // history behind: hand the ID back so the consumer reconciles that + // row through the durable ack instead of appending a duplicate via + // the say("user_feedback") fallback. return message.id } this.messageQueueService.removeMessage(message.id) @@ -1580,7 +1862,8 @@ export class Task extends EventEmitter implements TaskLike { return readTaskMessages({ taskId: this.taskId, globalStoragePath: this.globalStoragePath }) } - private async addToClineMessages(message: ClineMessage) { + /** Appends a message, posts it, and persists. @returns whether the history save succeeded. */ + private async addToClineMessages(message: ClineMessage): Promise { message.messageId ??= crypto.randomUUID() this.clineMessages.push(message) const provider = this.providerRef.deref() @@ -1611,7 +1894,7 @@ export class Task extends EventEmitter implements TaskLike { } } this.emit(RooCodeEventName.Message, { action: "created", message }) - await this.saveClineMessages() + const saved = await this.saveClineMessages() const shouldCaptureMessage = message.partial !== true && CloudService.isEnabled() @@ -1623,6 +1906,8 @@ export class Task extends EventEmitter implements TaskLike { // Track that this message has been synced to cloud this.cloudSyncedMessageTimestamps.add(message.ts) } + + return saved } /** @@ -1752,6 +2037,46 @@ export class Task extends EventEmitter implements TaskLike { * rejection: the denial is scoped to its own tool call. */ autoDenyDetail?: AutoDenyDetail + }> { + // Arm the drain gate synchronously at ask() entry, before the first + // await, and hold it for the whole ask lifecycle: a queued submission + // landing anywhere in the ask prefix (auto-approval, partial handling) + // must hit the same queuedResponseForAsk gate as one landing in the + // response wait, so an approval-gating ask can never be answered by + // the queue. The thin-wrapper shape keeps the body at its long-standing + // indentation instead of re-indenting it under a wrapping try block, + // so whitespace-only changes do not count against the mutation-diff + // line budget. + const gate = { type, text } + // Lazily initialized: Object.create(Task.prototype)-based harnesses skip + // field initializers, and the gate must exist before the try's finally. + const gates = (this.inFlightAskGates ??= new Set<{ type: ClineAsk; text?: string }>()) + gates.add(gate) + try { + return await this.askImpl(type, text, partial, progressStatus, isProtected, autoApprovalContext) + } finally { + gates.delete(gate) + } + } + + private async askImpl( + type: ClineAsk, + text?: string, + partial?: boolean, + progressStatus?: ToolProgressStatus, + isProtected?: boolean, + autoApprovalContext?: AutoApprovalContext, + ): Promise<{ + response: ClineAskResponse + text?: string + images?: string[] + queuedMessageId?: string + /** + * Present when the ask was resolved by an automatic (policy) denial + * rather than a user click. Consumers must not treat this as a user + * rejection: the denial is scoped to its own tool call. + */ + autoDenyDetail?: AutoDenyDetail }> { // If this Cline instance was aborted by the provider, then the only // thing keeping us alive is a promise still running in the background, @@ -1790,14 +2115,22 @@ export class Task extends EventEmitter implements TaskLike { // has to see and stalling a hands-free session. Leaving the message // unclaimed lets the policy decision stand and keeps the message queued. const queueMayAnswerThisAsk = !(blanketDenyEngaged && type === "command") - const queuedMessage = + // Resolve before claiming: approval-gating and failure-gate ask types + // never consume a queued conversational message, and claiming without a + // resolution would leak the claim. The resolution is only actionable + // when a message was actually claimed: otherwise it must not force a + // manual ask (that would bypass auto-approval for empty-queue lifecycle + // asks). The claim itself is deferred to the handoff branch below: it + // runs at the point of use with no prefix await in between, so a throw + // from the prefix (e.g. a synchronously throwing Message listener in + // addToClineMessages) can never strand the claim in claimedMessageIds. + const queuedAskResolution = partial === true || type === "command_output" || !queueMayAnswerThisAsk || !this.mayDrainQueuedMessageForAsk() ? undefined - : this.messageQueueService.claimNextMessage() - const queuedAskResolution = queuedMessage ? queuedResponseForAsk(type, text) : undefined + : queuedResponseForAsk(type, text) // `this.cwd`, not `provider.cwd`: // The path inside `text` was made relative to this task's workspace, // which for a resumed or child task need not be the one the provider @@ -1816,10 +2149,9 @@ export class Task extends EventEmitter implements TaskLike { const autoApprovalDecision = isAutoAnswered ? approval.decision : undefined // Re-check: an abort during the getState/checkAutoApproval awaits must not post an ask row. + // No queued claim exists yet at this point — claiming is deferred to the handoff + // branch below, which owns its release — so there is nothing to release here. if (this.abort) { - if (queuedMessage) { - this.messageQueueService.releaseMessage(queuedMessage.id) - } throw new Error(`[RooCode#ask] task ${this.taskId}.${this.instanceId} aborted`) } @@ -1865,6 +2197,7 @@ export class Task extends EventEmitter implements TaskLike { this.askResponse = undefined this.askResponseText = undefined this.askResponseImages = undefined + this.askResponseQueuedMessageId = undefined // Bug for the history books: // In the webview we use the ts as the chatrow key for the @@ -1898,6 +2231,7 @@ export class Task extends EventEmitter implements TaskLike { this.askResponse = undefined this.askResponseText = undefined this.askResponseImages = undefined + this.askResponseQueuedMessageId = undefined askTs = Date.now() this.lastMessageTs = askTs await this.addToClineMessages({ @@ -1916,6 +2250,7 @@ export class Task extends EventEmitter implements TaskLike { this.askResponse = undefined this.askResponseText = undefined this.askResponseImages = undefined + this.askResponseQueuedMessageId = undefined askTs = Date.now() this.lastMessageTs = askTs await this.addToClineMessages({ @@ -2061,65 +2396,72 @@ export class Task extends EventEmitter implements TaskLike { if (isStatusMutable) { armAskStatusTimers() - } else if (queuedMessage && queuedAskResolution) { - if (type === "command") { - // The snapshot gate is frozen; blanket deny may have engaged since the - // ask began. Re-read policy before the message stands in for approval. - // Drain-site parity for cancellation and cleanup: the fresh policy - // read ends in an uncancellable provider read, so an abort poller - // aborts a signal the re-check itself awaits — settling it on the - // abort instead of leaving a pending promise that retains this task - // and runs policy post-abort — and one `finally` releases the claim - // on the abort, throw, supersession, and "release" outcomes alike. - const recheckAbort = new AbortController() - const checkAbort = () => { - if (this.abort) { - recheckAbort.abort() + } else if (queuedAskResolution && this.mayDrainQueuedMessageForAsk()) { + // Claim at the point of handoff: for non-command asks the response is + // posted synchronously below (no await between claim and use), and the + // command re-check owns its own release in its finally — so a prefix + // throw can never strand this claim. + const queuedMessage = this.messageQueueService.claimNextMessage() + if (queuedMessage) { + if (type === "command") { + // The snapshot gate is frozen; blanket deny may have engaged since the + // ask began. Re-read policy before the message stands in for approval. + // Drain-site parity for cancellation and cleanup: the fresh policy + // read ends in an uncancellable provider read, so an abort poller + // aborts a signal the re-check itself awaits — settling it on the + // abort instead of leaving a pending promise that retains this task + // and runs policy post-abort — and one `finally` releases the claim + // on the abort, throw, supersession, and "release" outcomes alike. + const recheckAbort = new AbortController() + const checkAbort = () => { + if (this.abort) { + recheckAbort.abort() + } } - } - checkAbort() - const abortWatcher = setInterval(checkAbort, 100) - try { - const action = await this.recheckQueuedCommandPolicy( - { - text, - isProtected, - dcgDecision: autoApprovalContext?.dcgDecision, - }, - recheckAbort.signal, - ) - if (!this.abort && this.askResponse === undefined && this.lastMessageTs === askTs) { - // Any outcome on a still-live ask — the user answered or the - // ask was superseded — leaves the message for the next - // consumer; the `finally` below releases the claim and the ask - // resolves with the user's own response via the pWaitFor. - queuedMessageId = this.applyQueuedCommandPolicyAction( - action, - queuedMessage, - queuedAskResolution, + checkAbort() + const abortWatcher = setInterval(checkAbort, 100) + try { + const action = await this.recheckQueuedCommandPolicy( + { + text, + isProtected, + dcgDecision: autoApprovalContext?.dcgDecision, + }, + recheckAbort.signal, ) - // A "release" outcome leaves the ask pending; consume/deny/approve - // resolved it, and the arm's pending check declines to arm then. + if (!this.abort && this.askResponse === undefined && this.lastMessageTs === askTs) { + // Any outcome on a still-live ask — the user answered or the + // ask was superseded — leaves the message for the next + // consumer; the `finally` below releases the claim and the ask + // resolves with the user's own response via the pWaitFor. + queuedMessageId = this.applyQueuedCommandPolicyAction( + action, + queuedMessage, + queuedAskResolution, + ) + // A "release" outcome leaves the ask pending; consume/deny/approve + // resolved it, and the arm's pending check declines to arm then. + armAskStatusTimers() + } + } catch (error) { + // Drain-site parity: a failed re-check must not reject ask() nor + // strand the claim; the prompt stays pending for the user. + console.error("[Task#ask] queued command policy re-check failed:", error) armAskStatusTimers() + } finally { + clearInterval(abortWatcher) + // One release path for abort, throw, supersession, and "release". + // `releaseMessage` is idempotent, so it stays safe beside the + // helper-internal release. The durable consume is the one outcome + // that must keep its claim until persistence removes the message — + // it is the only path that assigned `queuedMessageId` here. + if (queuedMessageId !== queuedMessage.id) { + this.messageQueueService.releaseMessage(queuedMessage.id) + } } - } catch (error) { - // Drain-site parity: a failed re-check must not reject ask() nor - // strand the claim; the prompt stays pending for the user. - console.error("[Task#ask] queued command policy re-check failed:", error) - armAskStatusTimers() - } finally { - clearInterval(abortWatcher) - // One release path for abort, throw, supersession, and "release". - // `releaseMessage` is idempotent, so it stays safe beside the - // helper-internal release. The durable consume is the one outcome - // that must keep its claim until persistence removes the message — - // it is the only path that assigned `queuedMessageId` here. - if (queuedMessageId !== queuedMessage.id) { - this.messageQueueService.releaseMessage(queuedMessage.id) - } + } else { + queuedMessageId = this.handleQueuedAskResponse(queuedMessage, queuedAskResolution) } - } else { - queuedMessageId = this.handleQueuedAskResponse(queuedMessage, queuedAskResolution) } } else if (shouldDrainQueuedMessageForAsk && isMessageQueued) { // The claim gate (per-turn latch, or blanket deny engaged for a command @@ -2181,7 +2523,9 @@ export class Task extends EventEmitter implements TaskLike { } } - // Wait for askResponse to be set + // Wait for askResponse to be set. The drain gate has been armed since + // ask() entry, so a drain landing anywhere in the lifecycle consults + // the same queuedResponseForAsk gate as the claim path. try { await pWaitFor( () => { @@ -2193,15 +2537,18 @@ 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. Command asks under blanket deny are // excluded (`queueMayAnswerThisAsk`): a queued message must never stand in - // for the explicit approval the policy withheld. + // for the explicit approval the policy withheld. The resolution gate runs + // BEFORE the claim: approval-gating and failure-gate ask types resolve to + // undefined, and claiming without a resolution would leak the claim and + // stall every later conversational ask on a message no one can consume. if ( queueMayAnswerThisAsk && shouldDrainQueuedMessageForAsk && !queuedCommandPolicyCheck && this.mayDrainQueuedMessageForAsk() ) { - const message = this.messageQueueService.claimNextMessage() - const resolution = message ? queuedResponseForAsk(type, text) : undefined + const resolution = queuedResponseForAsk(type, text) + const message = resolution ? this.messageQueueService.claimNextMessage() : undefined if (message && resolution) { if (type === "command") { // Claim first, then verify the policy off-predicate: a @@ -2257,6 +2604,47 @@ export class Task extends EventEmitter implements TaskLike { throw new AskIgnoredError("superseded") } + // Tie a drain-submitted queued message to actual consumption by identity: + // the ask consumed the submission only if the pending slot still carries + // that message's ID. A direct response clears the slot ID even when its + // text/images are identical, so it cannot consume the queue entry. The + // claim path is excluded (durable flows already carry queuedMessageId; + // non-durable flows removed the message inline). + if (this.pendingSubmittedQueuedMessageId) { + const consumedViaPendingSlot = + queuedMessageId === undefined && + this.askResponseQueuedMessageId === this.pendingSubmittedQueuedMessageId + if (consumedViaPendingSlot) { + // Hand the ID to the caller instead of removing inline: the queue + // entry is deleted only after the feedback is durably saved + // (persistQueuedFeedbackAndAcknowledge), so a failed history write + // cannot lose a message that was already dequeued. The ID is + // handed only when the entry can also be reserved: the user may + // have deleted the queued message between submission and + // consumption, and acking an entry that no longer exists would + // fail at removeMessage — without a reservation the response is + // treated as direct feedback instead. + const submittedId = this.pendingSubmittedQueuedMessageId + this.pendingSubmittedQueuedMessageId = undefined + // Reserve the entry through the durable ack: while persistence + // retries (which can take seconds), neither a second ask nor a + // background drain may claim it again and persist the same + // message twice. Persistence settles the reservation itself — + // removeMessage on success, releaseMessage on failure/abort. + if (this.messageQueueService.claimMessage(submittedId)) { + queuedMessageId = submittedId + } + } else if ( + !this.messageQueueService.messages.some( + (message) => message.id === this.pendingSubmittedQueuedMessageId, + ) + ) { + // The message was consumed via the ask claim path or discarded + // by an existing path; the tracker is stale, so clear it. + this.pendingSubmittedQueuedMessageId = undefined + } + } + const result = { response: this.askResponse!, text: this.askResponseText, @@ -2267,6 +2655,7 @@ export class Task extends EventEmitter implements TaskLike { this.askResponse = undefined this.askResponseText = undefined this.askResponseImages = undefined + this.askResponseQueuedMessageId = undefined this.pendingAutoDenyDetail = undefined // Switch back to an active state. @@ -2289,13 +2678,19 @@ export class Task extends EventEmitter implements TaskLike { } } - handleWebviewAskResponse(askResponse: ClineAskResponse, text?: string, images?: string[]) { + handleWebviewAskResponse( + askResponse: ClineAskResponse, + text?: string, + images?: string[], + sourceQueuedMessageId?: string, + ) { // Clear any pending auto-approval timeout when user responds this.cancelAutoApprovalTimeout() this.askResponse = askResponse this.askResponseText = text this.askResponseImages = images + this.askResponseQueuedMessageId = sourceQueuedMessageId // Create a checkpoint whenever the user sends a message. // Use allowEmpty=true to ensure a checkpoint is recorded even if there are no file changes. @@ -2375,18 +2770,30 @@ export class Task extends EventEmitter implements TaskLike { this.api = buildApiHandler(this.apiConfiguration) } + /** + * Submit a user message through the ask-response channel. + * + * @param sourceQueuedMessageId - ID of the durable queue message being + * submitted, when this submission drains the queue. Recorded on the + * ask-response slot so the consuming ask can tie consumption to message + * identity; direct (non-queue) submissions leave it undefined. + * @returns true when the message was handed to the ask-response channel; + * false when there was nothing to submit or the handoff failed (the failure + * is logged either way). Callers draining a durable queue must check this. + */ public async submitUserMessage( text: string, images?: string[], mode?: string, providerProfile?: string, - ): Promise { + sourceQueuedMessageId?: string, + ): Promise { try { text = (text ?? "").trim() images = images ?? [] if (text.length === 0 && images.length === 0) { - return + return false } const provider = this.providerRef.deref() @@ -2409,17 +2816,52 @@ export class Task extends EventEmitter implements TaskLike { } } + // Cancellation guard immediately before the slot write: abort and + // dispose both set this flag, and the emit/checkpoint side effects + // below must never run on a cancelled task. The check is adjacent + // to the write so no cancellation can interleave (single-threaded). + if (this.abort || this.abandoned) { + console.error("[Task#submitUserMessage] Task aborted, dropping user message submission") + return false + } + + // Never overwrite a response an ask is blocked waiting on: a + // direct response that already landed in the slot must not be + // replaced (an approval would become a conversational answer). + // A queued (drain) submission must additionally never answer an + // approval ask or a failure gate on its own — a raw conversational + // post carries no user intent for those — while a direct + // submission is exactly that intent and may answer an approval + // ask (deny-with-feedback), matching the pre-drain behavior. + // Queue drains treat the returned false as "leave the message + // queued". + const inFlightAskGates = this.inFlightAskGates + const directAskBlocksSubmission = + inFlightAskGates !== undefined && + [...inFlightAskGates].some(({ type, text }) => queuedResponseForAsk(type, text) === undefined) + const inFlightAskHasResponse = (inFlightAskGates?.size ?? 0) > 0 && this.askResponse !== undefined + if ( + inFlightAskHasResponse || + (sourceQueuedMessageId === undefined && directAskBlocksSubmission) || + (sourceQueuedMessageId !== undefined && this.inFlightAskBlocksQueuedSubmission()) + ) { + return false + } + this.emit(RooCodeEventName.TaskUserMessage, this.taskId) // Handle the message directly instead of routing through the webview. // This avoids a race condition where the webview's message state hasn't // hydrated yet, causing it to interpret the message as a new task request. - this.handleWebviewAskResponse("messageResponse", text, images) + this.handleWebviewAskResponse("messageResponse", text, images, sourceQueuedMessageId) + return true } else { console.error("[Task#submitUserMessage] Provider reference lost") + return false } } catch (error) { console.error("[Task#submitUserMessage] Failed to submit user message:", error) + return false } } @@ -2578,7 +3020,7 @@ export class Task extends EventEmitter implements TaskLike { ) // Process any queued messages after condensing completes - this.processQueuedMessages() + await this.processQueuedMessages() } async say( @@ -2593,7 +3035,7 @@ export class Task extends EventEmitter implements TaskLike { } = {}, contextCondense?: ContextCondense, contextTruncation?: ContextTruncation, - ): Promise { + ): Promise { if (this.abort) { throw new Error(`[RooCode#say] task ${this.taskId}.${this.instanceId} aborted`) } @@ -2618,6 +3060,7 @@ export class Task extends EventEmitter implements TaskLike { this.updateClineMessage(lastMessage).catch((error) => { console.error("[Task#say] updateClineMessage failed:", error) }) + return true } else { // This is a new partial message, so add it with partial state. const sayTs = Date.now() @@ -2626,7 +3069,7 @@ export class Task extends EventEmitter implements TaskLike { this.lastMessageTs = sayTs } - await this.addToClineMessages({ + return this.addToClineMessages({ ts: sayTs, type: "say", say: type, @@ -2653,7 +3096,7 @@ export class Task extends EventEmitter implements TaskLike { // Instead of streaming partialMessage events, we do a save // and post like normal to persist to disk. - await this.saveClineMessages() + const saved = await this.saveClineMessages() // More performant than an entire `postStateToWebview`. // Fire-and-forget: see updateClineMessage call above for the @@ -2661,6 +3104,7 @@ export class Task extends EventEmitter implements TaskLike { this.updateClineMessage(lastMessage).catch((error) => { console.error("[Task#say] updateClineMessage failed:", error) }) + return saved } else { // This is a new and complete message, so add it like normal. const sayTs = Date.now() @@ -2669,7 +3113,7 @@ export class Task extends EventEmitter implements TaskLike { this.lastMessageTs = sayTs } - await this.addToClineMessages({ + return this.addToClineMessages({ ts: sayTs, type: "say", say: type, @@ -2692,7 +3136,7 @@ export class Task extends EventEmitter implements TaskLike { this.lastMessageTs = sayTs } - await this.addToClineMessages({ + return this.addToClineMessages({ ts: sayTs, type: "say", say: type, @@ -2987,13 +3431,22 @@ export class Task extends EventEmitter implements TaskLike { this.isInitialized = true - const { response, text, images } = await this.ask(askType) // Calls `postStateToWebview`. + const { response, text, images, queuedMessageId } = await this.ask(askType) // Calls `postStateToWebview`. let responseText: string | undefined let responseImages: string[] | undefined if (response === "messageResponse") { - await this.say("user_feedback", text, images) + if (queuedMessageId) { + const persisted = await this.persistQueuedFeedbackAndAcknowledge(queuedMessageId, text, images) + if (!persisted) { + throw new Error( + `[Task#resumeTaskFromHistory] Failed to persist queued feedback ${queuedMessageId}`, + ) + } + } else { + await this.say("user_feedback", text, images) + } responseText = text responseImages = images } @@ -3590,20 +4043,24 @@ export class Task extends EventEmitter implements TaskLike { ), ) - const { response, text, images } = await this.ask( + const { response, text, images, queuedMessageId } = await this.ask( "mistake_limit_reached", t("common:errors.mistake_limit_guidance"), ) if (response === "messageResponse") { + // Durable ack first: the feedback must reach history before + // the API turn. When the ack fails the entry is re-queued for + // redelivery, and the model must not already have seen the text + // (mirrors AttemptCompletionTool's ordering). + await this.sayUserFeedbackAndAckQueued(text, images, queuedMessageId) + currentUserContent.push( ...[ { type: "text" as const, text: formatResponse.tooManyMistakes(text) }, ...formatResponse.imageBlocks(images), ], ) - - await this.say("user_feedback", text, images) } this.consecutiveMistakeCount = 0 @@ -4301,7 +4758,12 @@ export class Task extends EventEmitter implements TaskLike { } else if (error instanceof OutputTokenLimitError) { // Truncation repeats on an identical request, so never auto-retry it // (even with auto-approval); let the user decide once. - const { response } = await this.ask("api_req_failed", rawErrorMessage) + const { response, queuedMessageId } = await this.ask("api_req_failed", rawErrorMessage) + // Failure-gate asks refuse queued-message conversion + // (queuedResponseForAsk returns undefined), so queuedMessageId + // is always undefined here today; the discard stays as a + // defensive no-op should that seam ever change. + this.discardConsumedQueuedMessage(queuedMessageId) if (response !== "yesButtonClicked") { throw new Error("API request failed") @@ -4764,10 +5226,13 @@ export class Task extends EventEmitter implements TaskLike { continue } else { // Prompt the user for retry decision - const { response } = await this.ask( + const { response, queuedMessageId } = await this.ask( "api_req_failed", "The model returned no assistant messages. This may indicate an issue with the API or the model's output.", ) + // Only the button response is inspected; a consumed queued + // message is dropped without inventing a history write. + this.discardConsumedQueuedMessage(queuedMessageId) if (response === "yesButtonClicked") { await this.say("api_req_retried") @@ -5423,7 +5888,13 @@ export class Task extends EventEmitter implements TaskLike { const approvalResult = await this.autoApprovalHandler.checkAutoApprovalLimits( state, this.combineMessages(this.clineMessages.slice(1)), - async (type, data) => this.ask(type, data), + async (type, data) => { + const result = await this.ask(type, data) + // The handler only inspects the button response; a consumed + // queued message is dropped without inventing a history write. + this.discardConsumedQueuedMessage(result.queuedMessageId) + return result + }, ) if (!approvalResult.shouldProceed) { @@ -5578,10 +6049,13 @@ export class Task extends EventEmitter implements TaskLike { return } else { - const { response } = await this.ask( + const { response, queuedMessageId } = await this.ask( "api_req_failed", error.message ?? JSON.stringify(serializeError(error), null, 2), ) + // Only the button response is inspected; a consumed queued + // message is dropped without inventing a history write. + this.discardConsumedQueuedMessage(queuedMessageId) if (response !== "yesButtonClicked") { // This will never happen since if noButtonClicked, we will @@ -6013,26 +6487,112 @@ export class Task extends EventEmitter implements TaskLike { } /** - * Process any queued messages by dequeuing and submitting them. - * This ensures that queued user messages are sent when appropriate, - * preventing them from getting stuck in the queue. + * Process the next queued message by claiming and submitting it. + * + * The message is claimed — not dequeued — before submission, and the claim + * is released after the submission handoff succeeds so the message STAYS + * queued: removal is tied to ask-consumption (Task.ask claims queued + * messages and removes them via handleQueuedAskResponse, or hands them to + * persistQueuedFeedbackAndAcknowledge), not to submit-return. A message + * submitted between command completion and the next conversational ask is + * therefore retained until an ask actually consumes it, instead of being + * dropped if the turn fails or restarts first. When submission fails, the + * claim is released so the message stays queued for a later drain, and the + * failure propagates to the caller instead of being logged and dropped. + * + * Drains are serialized per task: each run claims only after the previous + * run's submission handoff completed, so the background-completion drain + * and the post-result drain cannot interleave two different messages into + * the single pending ask-response. A drain whose claimed message is still + * pending consumption (already submitted, not yet observed by an ask) does + * not re-post it; it releases the claim and resolves true, leaving the + * message queued so a distinct pending response cannot be overwritten. A + * rejected drain does not block later drains. * - * @param context - Context string for logging (e.g., the calling tool name) + * @returns Promise resolving to true when a queued message was submitted + * (and remains queued until consumed) or was already pending submission and + * left queued; false when the queue was empty. */ - public processQueuedMessages(): void { + public processQueuedMessages(): Promise { + const run = this.queuedMessageDrainChain.then(() => this.claimAndSubmitNextQueuedMessage()) + // A rejected drain must not poison the chain for later drains; the + // caller still receives this run's outcome through the returned promise. + this.queuedMessageDrainChain = run.then( + () => {}, + () => {}, + ) + return run + } + + private async claimAndSubmitNextQueuedMessage(): Promise { + // A cancelled task must not submit: abortTask sets this flag before its + // awaited webview flush, and dispose clears the queue afterwards, so this + // guard closes the window where a drain could otherwise post into a + // dying task. + if (this.abort || this.abandoned) { + return false + } + const queued = this.messageQueueService.claimNextMessage() + if (!queued) { + return false + } + // An earlier drain already submitted this message and no ask has + // consumed it yet: re-posting would overwrite a distinct pending + // response and lose it, so release the claim and keep the message + // queued for the ask. + if (this.pendingSubmittedQueuedMessageId === queued.id) { + this.messageQueueService.releaseMessage(queued.id) + return true + } + // An approval-gating ask is blocked in its response wait. The claim path + // refuses to answer it with a queued conversational message; the drain + // must honor the same gate instead of posting messageResponse into the + // ask-response slot, so release the claim and leave the message queued + // for a conversational turn. + if (this.inFlightAskBlocksQueuedSubmission()) { + this.messageQueueService.releaseMessage(queued.id) + return true + } try { - if (!this.messageQueueService.isEmpty()) { - const queued = this.messageQueueService.dequeueMessage() - if (queued) { - setTimeout(() => { - this.submitUserMessage(queued.text, queued.images).catch((err) => - console.error(`[Task] Failed to submit queued message:`, err), - ) - }, 0) + const submitted = await this.submitUserMessage(queued.text, queued.images, undefined, undefined, queued.id) + if (!submitted) { + if (this.abort || this.abandoned) { + // Cancelled mid-handoff: the guard in submitUserMessage + // dropped the write, so release quietly instead of failing + // the drain on a task that is already stopping. + this.messageQueueService.releaseMessage(queued.id) + return false + } + if (this.inFlightAskBlocksQueuedSubmission()) { + // A direct response landed or an approval-gating ask is + // waiting: the submission was dropped to keep the slot + // intact, so leave the message queued instead of failing + // the drain. + this.messageQueueService.releaseMessage(queued.id) + return true + } + throw new Error(`[Task] Failed to submit queued message ${queued.id}`) + } + if (this.abort || this.abandoned) { + // The submission raced with cancellation: no consuming ask will + // run on a dying task, so do not track it as pending. + if (this.pendingSubmittedQueuedMessageId === queued.id) { + this.pendingSubmittedQueuedMessageId = undefined } + this.messageQueueService.releaseMessage(queued.id) + return false } - } catch (e) { - console.error(`[Task] Queue processing error:`, e) + } catch (error) { + // Release the claim so a later drain can retry the message. + this.messageQueueService.releaseMessage(queued.id) + throw error } + // Submission succeeded, but the message is only removed once an ask + // consumes it. Track the ID so the consuming ask can tell interception + // (consumption → durable ack) from an unconsumed overwrite (retain), + // and release the claim so the ask path can claim it. + this.pendingSubmittedQueuedMessageId = queued.id + this.messageQueueService.releaseMessage(queued.id) + return true } } diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 109eee8dc0..9b2f36148b 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -525,7 +525,7 @@ describe("Task persistence", () => { let saving: Promise | undefined try { - vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "say").mockResolvedValue(true) vi.spyOn(task, "ask").mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) vi.spyOn(task, "emitFinalTokenUsageUpdate").mockImplementation(() => undefined) vi.spyOn(task, "flushTelemetryInstallment").mockImplementation(() => undefined) @@ -620,7 +620,7 @@ describe("Task persistence", () => { toolDescription: vi.fn(), toolCallId: completionCallId, } - vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "say").mockResolvedValue(true) vi.spyOn(task, "ask").mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) vi.spyOn(task, "emitFinalTokenUsageUpdate").mockImplementation(() => undefined) vi.spyOn(task, "flushTelemetryInstallment").mockImplementation(() => undefined) @@ -690,7 +690,7 @@ describe("Task persistence", () => { toolDescription: vi.fn(), toolCallId: completionCallId, } - vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "say").mockResolvedValue(true) vi.spyOn(task, "ask").mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) vi.spyOn(task, "emitFinalTokenUsageUpdate").mockImplementation(() => undefined) vi.spyOn(task, "flushTelemetryInstallment").mockImplementation(() => undefined) @@ -1958,6 +1958,75 @@ describe("Task persistence", () => { expect(task.ask).toHaveBeenCalledWith("resume_task") }) + it("persists and acks queued feedback returned by the resume ask before continuing", async () => { + mockReadTaskMessages.mockResolvedValue([{ ts: 1, type: "say", say: "text", text: "Child" }]) + mockReadApiMessages.mockResolvedValue([{ role: "user", content: "resume me" }]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + historyItem: { + id: "child-1", + number: 1, + ts: 1, + task: "Child", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + }, + startTask: false, + }) + vi.spyOn(task, "ask").mockResolvedValue({ + response: "messageResponse", + text: "queued resume feedback", + queuedMessageId: "queued-resume", + }) + const persist = vi.spyOn(task, "persistQueuedFeedbackAndAcknowledge").mockResolvedValue(true) + const initiateTaskLoop = vi + .spyOn(getTaskPersistenceAccess(task), "initiateTaskLoop") + .mockResolvedValue(undefined) + + await getTaskPersistenceAccess(task).resumeTaskFromHistory() + + expect(persist).toHaveBeenCalledExactlyOnceWith("queued-resume", "queued resume feedback", undefined) + // The ack runs before the task loop continues. + expect(persist.mock.invocationCallOrder[0]).toBeLessThan(initiateTaskLoop.mock.invocationCallOrder[0]) + expect(initiateTaskLoop).toHaveBeenCalled() + }) + + it("stops resumption when the queued resume feedback cannot be persisted", async () => { + mockReadTaskMessages.mockResolvedValue([{ ts: 1, type: "say", say: "text", text: "Child" }]) + mockReadApiMessages.mockResolvedValue([{ role: "user", content: "resume me" }]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + historyItem: { + id: "child-1", + number: 1, + ts: 1, + task: "Child", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + }, + startTask: false, + }) + vi.spyOn(task, "ask").mockResolvedValue({ + response: "messageResponse", + text: "queued resume feedback", + queuedMessageId: "queued-resume", + }) + vi.spyOn(task, "persistQueuedFeedbackAndAcknowledge").mockResolvedValue(false) + const initiateTaskLoop = vi + .spyOn(getTaskPersistenceAccess(task), "initiateTaskLoop") + .mockResolvedValue(undefined) + + await expect(getTaskPersistenceAccess(task).resumeTaskFromHistory()).rejects.toThrow( + "Failed to persist queued feedback queued-resume", + ) + + expect(initiateTaskLoop).not.toHaveBeenCalled() + }) + it("clears pending metadata after the matching tool result is saved", async () => { mockProvider.clearPendingTaskAction = vi.fn().mockResolvedValue(true) const task = new Task({ diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index b484d327d2..889293f7a9 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -52,6 +52,7 @@ type TaskTestAccess = { getFilesReadByRooSafely: (context: string) => Promise addToApiConversationHistory: (message: unknown, reasoning?: string) => Promise resetAssistantMessagePersistence: () => void + checkpointSave: (force?: boolean, suppressMessage?: boolean) => Promise buildCleanConversationHistory: ( messages: ApiMessage[], requestModelInfo: ModelInfo, @@ -1469,7 +1470,7 @@ describe("Cline", () => { info: ctxModelInfo, }) Object.assign(task.api, { ensureModelFetched: vi.fn().mockResolvedValue(undefined) }) - vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) task.apiConversationHistory = [{ role: "user", content: [{ type: "text", text: "x" }], ts: Date.now() }] const getSystemPromptSpy = vi @@ -1565,7 +1566,7 @@ describe("Cline", () => { condenseId: "condense-id", }) const overwriteSpy = vi.spyOn(task, "overwriteApiConversationHistory").mockResolvedValue(undefined) - vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) await expect(task.condenseContext()).resolves.toBeUndefined() @@ -1642,7 +1643,7 @@ describe("Cline", () => { startTask: false, }) - const saySpy = vi.spyOn(cline, "say").mockResolvedValue(undefined) + const saySpy = vi.spyOn(cline, "say").mockResolvedValue(true) // relPath provided -> the "...WithPath" message branch. const withPath = await cline.sayAndCreateMissingParamError("read_file", "path", "src/foo.ts") @@ -2811,7 +2812,12 @@ describe("Cline", () => { await task.submitUserMessage("test message", ["image1.png"]) // Verify handleWebviewAskResponse was called directly (not webview) - expect(handleResponseSpy).toHaveBeenCalledWith("messageResponse", "test message", ["image1.png"]) + expect(handleResponseSpy).toHaveBeenCalledWith( + "messageResponse", + "test message", + ["image1.png"], + undefined, + ) // Should NOT route through webview anymore expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() }) @@ -2906,7 +2912,7 @@ describe("Cline", () => { task.clineMessages = [] await task.submitUserMessage("new task", ["image1.png"]) - expect(handleResponseSpy).toHaveBeenCalledWith("messageResponse", "new task", ["image1.png"]) + expect(handleResponseSpy).toHaveBeenCalledWith("messageResponse", "new task", ["image1.png"], undefined) // Clear mock handleResponseSpy.mockClear() @@ -2922,7 +2928,12 @@ describe("Cline", () => { ] await task.submitUserMessage("follow-up message", ["image2.png"]) - expect(handleResponseSpy).toHaveBeenCalledWith("messageResponse", "follow-up message", ["image2.png"]) + expect(handleResponseSpy).toHaveBeenCalledWith( + "messageResponse", + "follow-up message", + ["image2.png"], + undefined, + ) }) it("should handle undefined provider gracefully", async () => { @@ -2955,6 +2966,103 @@ describe("Cline", () => { // Restore console.error consoleErrorSpy.mockRestore() }) + + it("returns true when the message is handed to the ask-response channel", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "initial task", + startTask: false, + }) + const handleResponseSpy = vi.spyOn(task, "handleWebviewAskResponse").mockImplementation(() => {}) + + const submitted = await task.submitUserMessage("test message", ["image1.png"]) + + expect(submitted).toBe(true) + expect(handleResponseSpy).toHaveBeenCalledWith( + "messageResponse", + "test message", + ["image1.png"], + undefined, + ) + }) + + it("returns false when there is nothing to submit", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "initial task", + startTask: false, + }) + const handleResponseSpy = vi.spyOn(task, "handleWebviewAskResponse") + + // Empty text without images. + await expect(task.submitUserMessage("", [])).resolves.toBe(false) + // Whitespace-only text without images. + await expect(task.submitUserMessage(" ", [])).resolves.toBe(false) + + expect(handleResponseSpy).not.toHaveBeenCalled() + }) + + it("returns false when the provider reference is lost", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "initial task", + startTask: false, + }) + Object.defineProperty(task, "providerRef", { + value: { deref: () => undefined }, + writable: false, + configurable: true, + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await expect(task.submitUserMessage("test message")).resolves.toBe(false) + expect(consoleErrorSpy).toHaveBeenCalledWith("[Task#submitUserMessage] Provider reference lost") + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("returns false when the submission handoff throws", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "initial task", + startTask: false, + }) + vi.spyOn(task, "handleWebviewAskResponse").mockImplementation(() => { + throw new Error("emit failed") + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await expect(task.submitUserMessage("test message")).resolves.toBe(false) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[Task#submitUserMessage] Failed to submit user message:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("coerces a nullish text into an images-only submission", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "initial task", + startTask: false, + }) + const handleResponseSpy = vi.spyOn(task, "handleWebviewAskResponse").mockImplementation(() => {}) + + // Runtime callers outside the type system can pass a nullish text; + // the guard coerces it so an images-only message still submits. + const submitted = await task.submitUserMessage(undefined as unknown as string, ["image1.png"]) + + expect(submitted).toBe(true) + expect(handleResponseSpy).toHaveBeenCalledWith("messageResponse", "", ["image1.png"], undefined) + }) }) }) @@ -3042,7 +3150,7 @@ describe("Cline", () => { ask: "resume_task" as const, } - await expect(taskAccess.addToClineMessages(message)).resolves.toBeUndefined() + await expect(taskAccess.addToClineMessages(message)).resolves.toBe(true) expect(consoleErrorSpy).toHaveBeenCalledWith( "[Task#addToClineMessages] postStateToWebviewThrottled failed:", @@ -4134,7 +4242,7 @@ describe("Cline", () => { totalTokensOut: 0, contextTokens: 0, }) - vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "say").mockResolvedValue(true) task.apiConversationHistory = [ { role: "user", content: [{ type: "text", text: "test message" }], ts: Date.now() }, ] @@ -4668,7 +4776,7 @@ describe("Cline", () => { await task.getTaskMode() vi.spyOn(mockProvider, "getState").mockResolvedValue(providerStateWith()) vi.spyOn(task, "dispose").mockResolvedValue(undefined) - vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) // The wait never settles on its own; only the bound expires it. Object.assign(task.api, { ensureModelFetched: () => new Promise(() => {}) }) task.apiConversationHistory = [ @@ -4716,7 +4824,7 @@ describe("Cline", () => { }) await task.getTaskMode() vi.spyOn(mockProvider, "getState").mockResolvedValue(providerStateWith()) - vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) Object.assign(task.api, { ensureModelFetched: vi.fn().mockResolvedValue(undefined) }) task.abandoned = true // Only an unstarted prompt build attributes the skip to the entry @@ -4747,7 +4855,7 @@ describe("Cline", () => { await task.getTaskMode() vi.spyOn(mockProvider, "getState").mockResolvedValue(providerStateWith()) vi.spyOn(task, "dispose").mockResolvedValue(undefined) - vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) Object.assign(task.api, { ensureModelFetched: vi.fn().mockResolvedValue(undefined) }) task.apiConversationHistory = [ { role: "user", content: [{ type: "text", text: "test message" }], ts: Date.now() }, @@ -4760,7 +4868,7 @@ describe("Cline", () => { // The early return this guards never reaches say; the spy only keeps // the aborted task's post-overwrite say from throwing before the // summarize/overwrite assertions can report a regression. - vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "say").mockResolvedValue(true) const overwriteSpy = vi.spyOn(task, "overwriteApiConversationHistory").mockResolvedValue(undefined) // The summarizeConversation module mock is never cleared, so pin the // call count this condense starts from. @@ -4803,7 +4911,7 @@ describe("Cline", () => { await task.getTaskMode() vi.spyOn(mockProvider, "getState").mockResolvedValue(providerStateWith()) vi.spyOn(task, "dispose").mockResolvedValue(undefined) - vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) Object.assign(task.api, { ensureModelFetched: vi.fn().mockResolvedValue(undefined) }) task.apiConversationHistory = [ { role: "user", content: [{ type: "text", text: "test message" }], ts: Date.now() }, @@ -4812,7 +4920,7 @@ describe("Cline", () => { // The early return this guards never reaches say; the spy only keeps // the aborted task's post-overwrite say from throwing before the // overwrite assertion can report the regression. - const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) + const saySpy = vi.spyOn(task, "say").mockResolvedValue(true) const overwriteSpy = vi.spyOn(task, "overwriteApiConversationHistory").mockResolvedValue(undefined) // The summarizeConversation module mock is never cleared, so pin the // call count this condense starts from. @@ -4856,13 +4964,13 @@ describe("Cline", () => { await task.getTaskMode() vi.spyOn(mockProvider, "getState").mockResolvedValue(providerStateWith()) vi.spyOn(task, "dispose").mockResolvedValue(undefined) - vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) Object.assign(task.api, { ensureModelFetched: vi.fn().mockResolvedValue(undefined) }) task.apiConversationHistory = [ { role: "user", content: [{ type: "text", text: "test message" }], ts: Date.now() }, ] vi.spyOn(getTaskTestAccess(task), "getSystemPrompt").mockResolvedValue("mock system prompt") - vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "say").mockResolvedValue(true) const overwriteSpy = vi.spyOn(task, "overwriteApiConversationHistory").mockResolvedValue(undefined) // Suspend inside the collector so the abort lands while the // summarization request cannot have started yet. @@ -4903,13 +5011,13 @@ describe("Cline", () => { await task.getTaskMode() vi.spyOn(mockProvider, "getState").mockResolvedValue(providerStateWith()) vi.spyOn(task, "dispose").mockResolvedValue(undefined) - vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) Object.assign(task.api, { ensureModelFetched: vi.fn().mockResolvedValue(undefined) }) task.apiConversationHistory = [ { role: "user", content: [{ type: "text", text: "test message" }], ts: Date.now() }, ] vi.spyOn(getTaskTestAccess(task), "getSystemPrompt").mockResolvedValue("mock system prompt") - vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "say").mockResolvedValue(true) const overwriteSpy = vi.spyOn(task, "overwriteApiConversationHistory").mockResolvedValue(undefined) let releaseFilesRead!: (value: string[] | undefined) => void const filesReadGate = new Promise((resolve) => { @@ -5384,7 +5492,7 @@ describe("Cline", () => { releasePrompt = () => resolve("mock system prompt") }) vi.mocked(SYSTEM_PROMPT).mockReturnValueOnce(promptGate) - vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) vi.useFakeTimers() @@ -5585,7 +5693,7 @@ describe("Cline", () => { expect(task.clineMessages).toEqual([]) await pendingPostState }) - const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) + const saySpy = vi.spyOn(task, "say").mockResolvedValue(true) vi.spyOn(taskAccess, "getEnabledMcpToolsCount").mockResolvedValue({ enabledToolCount: 0, enabledServerCount: 0, @@ -6035,6 +6143,11 @@ describe("Cline", () => { describe("Queued message processing after condense", () => { function createProvider(): ClineProvider { + // The provider constructor reads TelemetryService.instance; suites that + // construct a real provider must ensure the singleton exists. + if (!TelemetryService.hasInstance()) { + TelemetryService.createInstance([]) + } const storageUri = { fsPath: path.join(os.tmpdir(), "test-storage") } const ctx = { globalState: { @@ -6097,20 +6210,25 @@ describe("Queued message processing after condense", () => { // Make condense fast + deterministic vi.spyOn(getTaskTestAccess(task), "getSystemPrompt").mockResolvedValue("system") - const submitSpy = vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + const submitSpy = vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) // Queue a message during condensing task.messageQueueService.addMessage("queued text", ["img1.png"]) - // Use fake timers to capture setTimeout(0) in processQueuedMessages - vi.useFakeTimers() await task.condenseContext() - // Flush the microtask that submits the queued message - vi.runAllTimers() - vi.useRealTimers() - - expect(submitSpy).toHaveBeenCalledWith("queued text", ["img1.png"]) + expect(submitSpy).toHaveBeenCalledWith( + "queued text", + ["img1.png"], + undefined, + undefined, + task.messageQueueService.messages[0]?.id, + ) + // Submission does not remove: the message stays queued until a + // conversational ask consumes it. + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["queued text"]) + const result = await task.ask("followup", "Anything else?", false) + expect(result).toMatchObject({ response: "messageResponse", text: "queued text" }) expect(task.messageQueueService.isEmpty()).toBe(true) }) @@ -6134,30 +6252,189 @@ describe("Queued message processing after condense", () => { vi.spyOn(getTaskTestAccess(taskA), "getSystemPrompt").mockResolvedValue("system") vi.spyOn(getTaskTestAccess(taskB), "getSystemPrompt").mockResolvedValue("system") - const spyA = vi.spyOn(taskA, "submitUserMessage").mockResolvedValue(undefined) - const spyB = vi.spyOn(taskB, "submitUserMessage").mockResolvedValue(undefined) + const spyA = vi.spyOn(taskA, "submitUserMessage").mockResolvedValue(true) + const spyB = vi.spyOn(taskB, "submitUserMessage").mockResolvedValue(true) taskA.messageQueueService.addMessage("A message") taskB.messageQueueService.addMessage("B message") // Condense in task A should only drain A's queue - vi.useFakeTimers() await taskA.condenseContext() - vi.runAllTimers() - vi.useRealTimers() - expect(spyA).toHaveBeenCalledWith("A message", undefined) + expect(spyA).toHaveBeenCalledWith( + "A message", + undefined, + undefined, + undefined, + taskA.messageQueueService.messages[0]?.id, + ) expect(spyB).not.toHaveBeenCalled() expect(taskB.messageQueueService.isEmpty()).toBe(false) // Now condense in task B should drain B's queue - vi.useFakeTimers() await taskB.condenseContext() - vi.runAllTimers() - vi.useRealTimers() - expect(spyB).toHaveBeenCalledWith("B message", undefined) + expect(spyB).toHaveBeenCalledWith( + "B message", + undefined, + undefined, + undefined, + taskB.messageQueueService.messages[0]?.id, + ) + // Drains submit but do not remove; each task retains its own message + // until an ask consumes it. + expect(taskA.messageQueueService.messages.map((message) => message.text)).toEqual(["A message"]) + expect(taskB.messageQueueService.messages.map((message) => message.text)).toEqual(["B message"]) + + const resultB = await taskB.ask("followup", "Anything else?", false) + expect(resultB).toMatchObject({ response: "messageResponse", text: "B message" }) expect(taskB.messageQueueService.isEmpty()).toBe(true) + // Consuming B's message must not touch A's queue. + expect(taskA.messageQueueService.messages.map((message) => message.text)).toEqual(["A message"]) + }) + + describe("processQueuedMessages drain semantics", () => { + const createQueueTask = () => + new Task({ + provider: createProvider(), + apiConfiguration: apiConfig, + task: "initial task", + startTask: false, + }) + + it("submits the next queued message and retains it until an ask consumes it", async () => { + const task = createQueueTask() + vi.spyOn(getTaskTestAccess(task), "checkpointSave").mockResolvedValue(undefined) + task.messageQueueService.addMessage("queued text", ["img1.png"]) + + await expect(task.processQueuedMessages()).resolves.toBe(true) + + // The real submit succeeded, but removal is tied to ask-consumption: + // the message stays queued until an ask claims it. + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["queued text"]) + + // The retained message answers the approval ask through the + // policy-gated claim path (base + #1760 semantics): yesButtonClicked + // with the queued text as feedback, consumed inline (non-durable). + const approval = await task.ask("tool", JSON.stringify({ tool: "readFile" }), false) + expect(approval).toMatchObject({ response: "yesButtonClicked", text: "queued text", images: ["img1.png"] }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("delivers queued feedback through the next conversational ask instead of losing it", async () => { + const task = createQueueTask() + vi.spyOn(getTaskTestAccess(task), "checkpointSave").mockResolvedValue(undefined) + task.messageQueueService.addMessage("one more change") + + // Command completion drain: the real submit succeeds and the webview + // shows the feedback, but no ask has consumed the response yet. + await expect(task.processQueuedMessages()).resolves.toBe(true) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["one more change"]) + + // The next conversational ask discards the unconsumed pending response + // at its start; the retained queued message must survive that discard + // and be consumed by the ask instead of being dropped. + const result = await task.ask("followup", "Anything else?", false) + + expect(result).toMatchObject({ response: "messageResponse", text: "one more change" }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("resolves false when the queue is empty", async () => { + const task = createQueueTask() + const submitSpy = vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) + + await expect(task.processQueuedMessages()).resolves.toBe(false) + + expect(submitSpy).not.toHaveBeenCalled() + }) + + it("releases the queued message and propagates when submission fails", async () => { + const task = createQueueTask() + vi.spyOn(task, "submitUserMessage").mockResolvedValue(false) + task.messageQueueService.addMessage("retry me") + + await expect(task.processQueuedMessages()).rejects.toThrow("Failed to submit queued message") + + // The claim is released, so the message stays queued for a later drain. + expect(task.messageQueueService.claimNextMessage()?.text).toBe("retry me") + }) + + it("releases the queued message and propagates when submission throws", async () => { + const task = createQueueTask() + vi.spyOn(task, "submitUserMessage").mockRejectedValue(new Error("emit failed")) + task.messageQueueService.addMessage("retry me too") + + await expect(task.processQueuedMessages()).rejects.toThrow("emit failed") + + expect(task.messageQueueService.claimNextMessage()?.text).toBe("retry me too") + }) + + it("serializes concurrent drains so a blocked submission holds the next message queued", async () => { + const task = createQueueTask() + vi.spyOn(getTaskTestAccess(task), "checkpointSave").mockResolvedValue(undefined) + task.messageQueueService.addMessage("first") + task.messageQueueService.addMessage("second") + + // Block the first submission mid-handoff so its response stays pending. + let releaseFirstSubmission!: () => void + const firstSubmission = new Promise((resolve) => { + releaseFirstSubmission = () => resolve(true) + }) + const submitSpy = vi + .spyOn(task, "submitUserMessage") + .mockReturnValueOnce(firstSubmission) + .mockResolvedValue(true) + + const firstDrain = task.processQueuedMessages() + const secondDrain = task.processQueuedMessages() + + // The first drain reaches its blocked submission; the serialized + // second drain must not claim or submit the next message yet. + await Promise.resolve() + expect(submitSpy).toHaveBeenCalledTimes(1) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["first", "second"]) + + releaseFirstSubmission() + await expect(firstDrain).resolves.toBe(true) + await expect(secondDrain).resolves.toBe(true) + + // The second drain saw "first" still pending consumption and did not + // re-post it (a re-post could overwrite a distinct pending response); + // both messages stay queued until asks consume them in order. + expect(submitSpy).toHaveBeenCalledTimes(1) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["first", "second"]) + + const firstResult = await task.ask("followup", "first question?", false) + expect(firstResult).toMatchObject({ response: "messageResponse", text: "first" }) + const secondResult = await task.ask("followup", "second question?", false) + expect(secondResult).toMatchObject({ response: "messageResponse", text: "second" }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("does not let a failed drain block later drains", async () => { + const task = createQueueTask() + vi.spyOn(getTaskTestAccess(task), "checkpointSave").mockResolvedValue(undefined) + vi.spyOn(task, "submitUserMessage").mockRejectedValueOnce(new Error("emit failed")).mockResolvedValue(true) + task.messageQueueService.addMessage("retry me") + task.messageQueueService.addMessage("still deliverable") + + await expect(task.processQueuedMessages()).rejects.toThrow("emit failed") + // The failed claim is released; the next drain retries it in order. + await expect(task.processQueuedMessages()).resolves.toBe(true) + await expect(task.processQueuedMessages()).resolves.toBe(true) + + // The retried submissions stay queued until asks consume them in order. + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual([ + "retry me", + "still deliverable", + ]) + const firstResult = await task.ask("followup", "first question?", false) + expect(firstResult).toMatchObject({ response: "messageResponse", text: "retry me" }) + const secondResult = await task.ask("followup", "second question?", false) + expect(secondResult).toMatchObject({ response: "messageResponse", text: "still deliverable" }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) }) }) diff --git a/src/core/task/__tests__/ask-abort-race.spec.ts b/src/core/task/__tests__/ask-abort-race.spec.ts index aa5dbe4a82..8efdbf8b2a 100644 --- a/src/core/task/__tests__/ask-abort-race.spec.ts +++ b/src/core/task/__tests__/ask-abort-race.spec.ts @@ -14,7 +14,7 @@ function buildTask(getState: () => Promise>) { task["abort"] = false task["clineMessages"] = [] task["lastMessageTs"] = undefined - task["addToClineMessages"] = vi.fn(async () => {}) + task["addToClineMessages"] = vi.fn(async () => true) task["saveClineMessages"] = vi.fn(async () => true) task["updateClineMessage"] = vi.fn(async () => {}) // Double assertion: `providerRef` is a `WeakRef`; `Task.ask` only calls `deref()` and `getState()`. diff --git a/src/core/task/__tests__/ask-allowlist-cwd.spec.ts b/src/core/task/__tests__/ask-allowlist-cwd.spec.ts index 3b3682f969..3b130c21d8 100644 --- a/src/core/task/__tests__/ask-allowlist-cwd.spec.ts +++ b/src/core/task/__tests__/ask-allowlist-cwd.spec.ts @@ -29,7 +29,7 @@ function buildTask(provider: ProviderStub, taskCwd: string) { task["askResponseText"] = undefined task["askResponseImages"] = undefined task["lastMessageTs"] = undefined - task["addToClineMessages"] = vi.fn(async () => {}) + task["addToClineMessages"] = vi.fn(async () => true) task["saveClineMessages"] = vi.fn(async () => true) task["updateClineMessage"] = vi.fn(async () => {}) task["cancelAutoApprovalTimeout"] = vi.fn(() => {}) diff --git a/src/core/task/__tests__/ask-auto-deny.spec.ts b/src/core/task/__tests__/ask-auto-deny.spec.ts index 4254791025..1b26eb83d2 100644 --- a/src/core/task/__tests__/ask-auto-deny.spec.ts +++ b/src/core/task/__tests__/ask-auto-deny.spec.ts @@ -37,7 +37,8 @@ function buildTask(provider: ProviderStub, taskCwd: string) { task["askResponseText"] = undefined task["askResponseImages"] = undefined task["lastMessageTs"] = undefined - task["addToClineMessages"] = vi.fn(async () => {}) + // Merged Task.addToClineMessages reports the save outcome (Promise). + task["addToClineMessages"] = vi.fn(async () => true) task["saveClineMessages"] = vi.fn(async () => true) task["updateClineMessage"] = vi.fn(async () => {}) task["cancelAutoApprovalTimeout"] = vi.fn(() => {}) @@ -80,7 +81,8 @@ function attachTurnHarness(task: Task) { Object.defineProperty(task, "diffViewProvider", { value: { isEditing: false, reset: async () => {} } }) Object.defineProperty(task, "streamingToolCallIndices", { value: new Map() }) task["saveApiConversationHistory"] = vi.fn(async () => true) - task["say"] = vi.fn(async () => undefined) + // Merged Task.say reports the save outcome (Promise). + task["say"] = vi.fn(async () => true) // The loop rewrites the newest api_req_started row with cost data; the // no-op `say` stub above never adds one itself. task["clineMessages"].push({ type: "say", say: "api_req_started", text: "{}", ts: Date.now() }) @@ -101,6 +103,7 @@ const TASK_CWD = "/path/to/task-workspace" function recordClineMessages(task: Task) { const addToClineMessages = vi.fn(async (message: ClineMessage) => { task["clineMessages"].push(message) + return true }) task["addToClineMessages"] = addToClineMessages return addToClineMessages 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 b137130174..1578a69661 100644 --- a/src/core/task/__tests__/ask-queued-message-drain.spec.ts +++ b/src/core/task/__tests__/ask-queued-message-drain.spec.ts @@ -1,11 +1,19 @@ +import type { ClineMessage } from "@roo-code/types" + import { Task } from "../Task" type QueueTaskTestAccess = { say: Task["say"] saveClineMessages: () => Promise - addToClineMessages: () => Promise + addToClineMessages: (message?: ClineMessage) => Promise + updateClineMessage: (message?: ClineMessage) => Promise + clineMessages: ClineMessage[] + queuedFeedbackRows: Map lastMessageTs?: number abort: boolean + abandoned: boolean + queuedMessageDrainChain: Promise + emit: (event: string, ...args: unknown[]) => void } const getQueueTaskTestAccess = (task: Task) => task as unknown as QueueTaskTestAccess @@ -24,6 +32,12 @@ describe("Task.ask queued message drain", () => { ;(task as any).lastMessageTs = undefined return import("../../message-queue/MessageQueueService").then(({ MessageQueueService }) => { ;(task as any).messageQueueService = new MessageQueueService() + // Object.create skips field initializers; the drain chain must exist + // for processQueuedMessages to schedule behind it. + getQueueTaskTestAccess(task).queuedMessageDrainChain = Promise.resolve() + // Object.create skips field initializers; the drain chain and the + // feedback-row association map must exist for their paths. + getQueueTaskTestAccess(task).queuedFeedbackRows = new Map() ;(task as any).addToClineMessages = vi.fn(async () => {}) ;(task as any).saveClineMessages = vi.fn(async () => {}) ;(task as any).updateClineMessage = vi.fn(async () => {}) @@ -48,6 +62,767 @@ describe("Task.ask queued message drain", () => { expect(result.text).toBe("picked answer") }) + it("acks a drained padded message through the consuming ask", async () => { + const task = await createTask({ getState: async () => ({}) }) + + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + // editQueuedMessage saves untrimmed text; submitUserMessage trims before + // posting, so the drained submission is the trimmed text. + task.messageQueueService.addMessage(" padded correction ") + const drain = task.processQueuedMessages() + + const result = await askPromise + await drain + + expect(result).toMatchObject({ response: "messageResponse", text: "padded correction" }) + // Interception is consumption, but removal is deferred to the durable + // ack: the entry stays queued until the history write succeeds. + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + expect(task.messageQueueService.isEmpty()).toBe(false) + + getQueueTaskTestAccess(task).saveClineMessages = vi.fn(async () => true) + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).resolves.toBe(true) + expect(task.messageQueueService.isEmpty()).toBe(true) + + setTimeout(() => task.approveAsk(), 0) + const nextResult = await task.ask("tool", JSON.stringify({ tool: "readFile" }), false) + expect(nextResult).toMatchObject({ response: "yesButtonClicked", text: undefined }) + }) + + it("acks an intercepted drained message through the consuming ask", async () => { + const task = await createTask({ getState: async () => ({}) }) + + // Park a completion ask in the real pWaitFor: no auto-approval and + // nothing queued at ask start, so it blocks. A completion ask has a + // queued-ask resolution, so a mid-block drain may intercept it. + const askPromise = task.ask("completion_result", "Done", false) + // Let the ask reach its pWaitFor before the drain runs. + await new Promise((resolve) => setTimeout(resolve, 150)) + + // Background-completion style drain while the ask is blocked: the real + // submit posts the message into the pending ask-response slot. + task.messageQueueService.addMessage("queued correction") + const drain = task.processQueuedMessages() + + const result = await askPromise + await drain + + // Interception: the blocked tool ask is answered with the submitted + // message (the claim path would have answered yesButtonClicked). + expect(result).toMatchObject({ response: "messageResponse", text: "queued correction" }) + // The entry stays queued until the consuming ask's durable ack removes it. + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + expect(task.messageQueueService.isEmpty()).toBe(false) + + getQueueTaskTestAccess(task).saveClineMessages = vi.fn(async () => true) + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).resolves.toBe(true) + expect(task.messageQueueService.isEmpty()).toBe(true) + + setTimeout(() => task.approveAsk(), 0) + const nextResult = await task.ask("tool", JSON.stringify({ tool: "readFile" }), false) + expect(nextResult).toMatchObject({ response: "yesButtonClicked", text: undefined }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("retains a drained message when a user response overwrites it before consumption", async () => { + const task = await createTask({ getState: async () => ({}) }) + + const askPromise = task.ask("tool", JSON.stringify({ tool: "readFile" }), false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + // The drain would post the message into the pending slot, but the + // approval-gating gate drops the submission instead; the user then + // answers the blocked ask directly. + task.messageQueueService.addMessage("queued correction") + await task.processQueuedMessages() + setTimeout(() => task.approveAsk(), 0) + + const result = await askPromise + + expect(result).toMatchObject({ response: "yesButtonClicked", text: undefined }) + // The overwritten submission was not consumed, so the message stays + // queued for a later ask instead of being removed or lost. + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["queued correction"]) + + const nextResult = await task.ask("followup", "Q?", false) + expect(nextResult).toMatchObject({ response: "messageResponse", text: "queued correction" }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("does not consume a drained message when a direct response has identical text and images", async () => { + const task = await createTask({ getState: async () => ({}) }) + + const askPromise = task.ask("tool", JSON.stringify({ tool: "readFile" }), false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + task.messageQueueService.addMessage("same words", ["img.png"]) + await task.processQueuedMessages() + + // A direct user response with the exact same trimmed text and images + // lands before the ask observes the pending slot; identity, not + // content, decides consumption. + task.handleWebviewAskResponse("messageResponse", "same words", ["img.png"]) + + const result = await askPromise + + expect(result).toMatchObject({ response: "messageResponse", text: "same words", images: ["img.png"] }) + expect(result.queuedMessageId).toBeUndefined() + // The direct response did not consume the queue entry. + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["same words"]) + }) + + it("does not resubmit a drained message that is still pending consumption", async () => { + const task = await createTask({ getState: async () => ({}) }) + const submitSpy = vi.spyOn(task, "submitUserMessage") + + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + task.messageQueueService.addMessage("queued correction") + await task.processQueuedMessages() + + // A direct response lands before the ask observes the pending slot. + task.handleWebviewAskResponse("yesButtonClicked") + + // The second drain (background-completion + post-result orchestration) + // must not re-post the retained message over the direct response. + const secondDrain = task.processQueuedMessages() + + const result = await askPromise + await secondDrain + + expect(submitSpy).toHaveBeenCalledTimes(1) + expect(result).toMatchObject({ response: "yesButtonClicked", text: undefined }) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["queued correction"]) + + // The retained message is still deliverable to a later ask. + const nextResult = await task.ask("followup", "Q?", false) + expect(nextResult).toMatchObject({ response: "messageResponse", text: "queued correction" }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("retains an intercepted drained message until its history write succeeds", async () => { + const task = await createTask({ getState: async () => ({}) }) + + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + task.messageQueueService.addMessage("Keep this correction") + const drain = task.processQueuedMessages() + + const result = await askPromise + await drain + + expect(result).toMatchObject({ response: "messageResponse", text: "Keep this correction" }) + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + + // A failed history write must keep the message queued; the ack retries + // and removes the entry only after the save succeeds. + const taskAccess = getQueueTaskTestAccess(task) + taskAccess.say = vi.fn().mockResolvedValue(true) + taskAccess.saveClineMessages = vi.fn().mockResolvedValueOnce(false).mockResolvedValue(true) + + vi.useFakeTimers() + try { + const persistence = task.persistQueuedFeedbackAndAcknowledge( + result.queuedMessageId!, + result.text, + result.images, + ) + await vi.advanceTimersByTimeAsync(0) + expect(task.messageQueueService.isEmpty()).toBe(false) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["Keep this correction"]) + + await vi.advanceTimersByTimeAsync(250) + await expect(persistence).resolves.toBe(true) + expect(task.messageQueueService.isEmpty()).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it("re-queues an intercepted drained message when its history write keeps failing", async () => { + const task = await createTask({ getState: async () => ({}) }) + + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + task.messageQueueService.addMessage("Do not lose me") + const drain = task.processQueuedMessages() + + const result = await askPromise + await drain + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + + const taskAccess = getQueueTaskTestAccess(task) + taskAccess.say = vi.fn().mockResolvedValue(true) + taskAccess.saveClineMessages = vi.fn().mockResolvedValue(false) + + vi.useFakeTimers() + try { + const persistence = task.persistQueuedFeedbackAndAcknowledge( + result.queuedMessageId!, + result.text, + result.images, + ) + await vi.runAllTimersAsync() + + await expect(persistence).resolves.toBe(false) + expect(taskAccess.saveClineMessages).toHaveBeenCalledTimes(4) + // The message is released back to the queue for a later drain. + expect(task.messageQueueService.messages).toHaveLength(1) + expect(task.messageQueueService.claimNextMessage()?.text).toBe("Do not lose me") + } finally { + vi.useRealTimers() + } + }) + + it("persists an entry edit made during a failed-save backoff", async () => { + const task = await createTask({ getState: async () => ({}) }) + const submitSpy = vi.spyOn(task, "submitUserMessage") + + task.messageQueueService.addMessage("original text") + // Between-turns drain: submits the message and tracks it as pending. + await expect(task.processQueuedMessages()).resolves.toBe(true) + expect(submitSpy).toHaveBeenCalledTimes(1) + + // A completion ask claims the retained entry through the durable path. + const result = await task.ask("completion_result", "Done", false) + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + + const access = getQueueTaskTestAccess(task) + access.say = vi.fn().mockResolvedValue(undefined) + access.saveClineMessages = vi.fn().mockResolvedValueOnce(false).mockResolvedValue(true) + + vi.useFakeTimers() + try { + const persistence = task.persistQueuedFeedbackAndAcknowledge( + result.queuedMessageId!, + result.text, + result.images, + ) + // First save attempt fails; the ack is now waiting in the backoff delay. + await vi.advanceTimersByTimeAsync(0) + expect(task.messageQueueService.isEmpty()).toBe(false) + // The queue UI still accepts edits while the entry is retained. + task.editQueuedMessage(result.queuedMessageId!, "edited text") + await vi.advanceTimersByTimeAsync(250) + await expect(persistence).resolves.toBe(true) + } finally { + vi.useRealTimers() + } + + expect(task.messageQueueService.isEmpty()).toBe(true) + // One retry after the initial failure, with no extra save storm. + expect(access.saveClineMessages).toHaveBeenCalledTimes(2) + // The ack persisted the edit, not the stale submission copy. + expect(access.updateClineMessage).toHaveBeenCalledWith(expect.objectContaining({ text: "edited text" })) + }) + + it("releases the drain tracker when a claimed durable ack keeps failing", async () => { + const task = await createTask({ getState: async () => ({}) }) + const submitSpy = vi.spyOn(task, "submitUserMessage") + + task.messageQueueService.addMessage("Do not lose me") + // Between-turns drain: submits the message and tracks it as pending. + await expect(task.processQueuedMessages()).resolves.toBe(true) + expect(submitSpy).toHaveBeenCalledTimes(1) + + // A completion ask claims the retained entry through the durable path; + // the drain-side tracker is still set while the ack runs. + const result = await task.ask("completion_result", "Done", false) + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + + const access = getQueueTaskTestAccess(task) + access.say = vi.fn().mockResolvedValue(undefined) + access.saveClineMessages = vi.fn().mockResolvedValue(false) + + vi.useFakeTimers() + try { + const persistence = task.persistQueuedFeedbackAndAcknowledge( + result.queuedMessageId!, + result.text, + result.images, + ) + await vi.runAllTimersAsync() + await expect(persistence).resolves.toBe(false) + } finally { + vi.useRealTimers() + } + expect(access.saveClineMessages).toHaveBeenCalledTimes(4) + expect(task.messageQueueService.messages).toHaveLength(1) + + // The tracker must be cleared even though the message was re-queued, so + // this drain resubmits it instead of stalling on the stale pending ID + // (which would also starve every message behind it). + await expect(task.processQueuedMessages()).resolves.toBe(true) + expect(submitSpy).toHaveBeenCalledTimes(2) + }) + + it("releases the drain tracker when the row write throws after a durable claim", async () => { + const task = await createTask({ getState: async () => ({}) }) + const submitSpy = vi.spyOn(task, "submitUserMessage") + + task.messageQueueService.addMessage("Do not lose me") + // Between-turns drain: submits the message and tracks it as pending. + await expect(task.processQueuedMessages()).resolves.toBe(true) + expect(submitSpy).toHaveBeenCalledTimes(1) + + // A completion ask claims the retained entry through the durable path. + const result = await task.ask("completion_result", "Done", false) + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + + // The row write throws (a synchronously throwing Message listener): + // the catch releases the entry and rethrows, and must ALSO release the + // pending tracker — it cannot be the finally's job alone. + const access = getQueueTaskTestAccess(task) + access.addToClineMessages = vi.fn(async () => { + throw new Error("listener boom") + }) + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).rejects.toThrow("listener boom") + + // The re-queued message must be resubmitted by the next drain instead + // of stalling on the stale tracker ID forever. + expect(task.messageQueueService.messages).toHaveLength(1) + await expect(task.processQueuedMessages()).resolves.toBe(true) + expect(submitSpy).toHaveBeenCalledTimes(2) + }) + + it("arms the drain gate for the whole ask lifecycle, including the prefix", async () => { + const task = await createTask({ getState: async () => ({}) }) + const submitSpy = vi.spyOn(task, "submitUserMessage") + const access = getQueueTaskTestAccess(task) + let finishAddingAsk!: () => void + const addingAsk = new Promise((resolve) => { + finishAddingAsk = resolve + }) + // Block the ask in its prefix (the addToClineMessages await) so a drain + // lands before the response wait begins. A failure-gate ask type keeps + // the claim path from answering it with the queued message. + access.addToClineMessages = vi.fn(() => addingAsk.then(() => true)) + + const askPromise = task.ask("api_req_failed", "stream failed", false) + await Promise.resolve() + + task.messageQueueService.addMessage("queued note") + await expect(task.processQueuedMessages()).resolves.toBe(true) + expect(submitSpy).not.toHaveBeenCalled() + + finishAddingAsk() + setTimeout(() => task.approveAsk(), 0) + const result = await askPromise + expect(result).toMatchObject({ response: "yesButtonClicked", text: undefined }) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["queued note"]) + }) + + it("applies a queue edit to the pending submitted response before the ask consumes it", async () => { + const task = await createTask({ getState: async () => ({}) }) + + // Park a completion ask, then drain: the submission lands in the + // ask-response slot and stays queued until the durable ack. + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + task.messageQueueService.addMessage("original text") + await task.processQueuedMessages() + const queuedEntry = task.messageQueueService.messages.at(0) + if (!queuedEntry) throw new Error("queued message missing") + expect(task["askResponseText"]).toBe("original text") + + // The user edits the queued message before the ask observes the slot: + // the task-owned pending submission must carry the edited content. + task.editQueuedMessage(queuedEntry.id, "edited text", ["edited.png"]) + + const result = await askPromise + expect(result).toMatchObject({ response: "messageResponse", text: "edited text", images: ["edited.png"] }) + expect(result.queuedMessageId).toBe(queuedEntry.id) + }) + + it("leaves a queued message redeliverable when the ask prefix throws", async () => { + const task = await createTask() + task.messageQueueService.addMessage("stranded no more") + const access = getQueueTaskTestAccess(task) + // A synchronously throwing Message listener during the ask prefix. + access.addToClineMessages = vi.fn(async () => { + throw new Error("listener boom") + }) + + await expect(task.ask("followup", "Q?", false)).rejects.toThrow("listener boom") + + // The claim is taken at the handoff, not in the prefix, so the throw + // cannot strand it in claimedMessageIds: the message is immediately + // claimable again for a later ask or drain. + expect(task.messageQueueService.claimNextMessage()?.text).toBe("stranded no more") + }) + + it("reserves a consumed drain submission through its durable ack", async () => { + const task = await createTask({ getState: async () => ({}) }) + + // Interception: the ask is blocked before the drain submits, so the + // pending-slot consumption path hands the ID back. + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + task.messageQueueService.addMessage("Reserve me") + await task.processQueuedMessages() + const result = await askPromise + const entry = task.messageQueueService.messages.at(0) + if (!entry) throw new Error("queued message missing") + expect(result.queuedMessageId).toBe(entry.id) + + // While the durable ack has not settled, the entry stays reserved: + // neither a later ask nor a background drain can claim it again and + // persist the same message twice. + expect(task.messageQueueService.claimNextMessage()).toBeUndefined() + const submitSpy = vi.spyOn(task, "submitUserMessage") + await expect(task.processQueuedMessages()).resolves.toBe(false) + expect(submitSpy).not.toHaveBeenCalled() + + // A failed ack releases the reservation for redelivery. + const access = getQueueTaskTestAccess(task) + access.say = vi.fn().mockResolvedValue(undefined) + access.saveClineMessages = vi.fn(async () => false) + vi.useFakeTimers() + try { + const persistence = task.persistQueuedFeedbackAndAcknowledge(entry.id, result.text, result.images) + await vi.runAllTimersAsync() + await expect(persistence).resolves.toBe(false) + } finally { + vi.useRealTimers() + } + expect(task.messageQueueService.claimNextMessage()?.text).toBe("Reserve me") + }) + + it("keeps a blocked ask's drain gate when a superseded ask exits", async () => { + const task = await createTask({ getState: async () => ({}) }) + + // Ask A blocks in its wait. + const askA = task.ask("followup", "A?", false).catch((error: unknown) => error) + await new Promise((resolve) => setTimeout(resolve, 150)) + + // Ask B starts: its prefix supersedes A (lastMessageTs moves) and arms + // its own gate before A's superseded-exit finally runs. A's finally + // must remove only A's gate. + const askB = task.ask("api_req_failed", "B failure gate", false).catch((error: unknown) => error) + await new Promise((resolve) => setTimeout(resolve, 200)) + + // A has exited; B is still blocked. A's exit must not have cleared B's + // gate: a queued submission is still refused. + await expect(task.submitUserMessage("queued note")).resolves.toBe(false) + + setTimeout(() => task.approveAsk(), 0) + const resultB = await askB + expect(resultB).toMatchObject({ response: "yesButtonClicked" }) + const resultA = await askA + expect(String(resultA)).toContain("superseded") + }) + + it("treats a consumed submission as direct feedback when the entry was deleted mid-flight", async () => { + const task = await createTask({ getState: async () => ({}) }) + + // Interception: blocked ask, drain submits, then the user deletes the + // queued message before the ask observes the pending slot. + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + task.messageQueueService.addMessage("deleted before consume") + await task.processQueuedMessages() + const entry = task.messageQueueService.messages.at(0) + if (!entry) throw new Error("queued message missing") + task.messageQueueService.removeMessage(entry.id) + + const result = await askPromise + + // No entry remains to ack: the ID must not be handed back, and the + // response text is delivered as direct feedback instead. + expect(result).toMatchObject({ response: "messageResponse", text: "deleted before consume" }) + expect(result.queuedMessageId).toBeUndefined() + }) + it("delivers an intercepted message exactly once when a consumer acks through the durable helper", async () => { + const task = await createTask({ getState: async () => ({}) }) + const submitSpy = vi.spyOn(task, "submitUserMessage") + + // Blocked completion ask: the queue is empty at ask start, so a drain + // that posts mid-block intercepts the ask (the approval-gating gate + // does not apply to a resolvable ask). + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + task.messageQueueService.addMessage("user correction") + const drain = task.processQueuedMessages() + + const result = await askPromise + await drain + + expect(result).toMatchObject({ response: "messageResponse", text: "user correction" }) + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + + // The consumer persists the feedback through the acking helper, which + // removes the queue entry only after the history write succeeds. + getQueueTaskTestAccess(task).saveClineMessages = vi.fn(async () => true) + await task.sayUserFeedbackAndAckQueued(result.text, result.images, result.queuedMessageId) + expect(task.messageQueueService.isEmpty()).toBe(true) + + // No later drain or claim may redeliver the consumed message. + await expect(task.processQueuedMessages()).resolves.toBe(false) + expect(submitSpy).toHaveBeenCalledTimes(1) + }) + + it("drops an intercepted message consumed without persisting feedback", async () => { + const task = await createTask({ getState: async () => ({}) }) + const submitSpy = vi.spyOn(task, "submitUserMessage") + + // Followup-style conversational ask: the consumer only inspects the + // button response, so an intercepted queued message can be discarded + // instead of acked. + const askPromise = task.ask("followup", "Anything to add?", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + task.messageQueueService.addMessage("queued note") + const drain = task.processQueuedMessages() + + const result = await askPromise + await drain + + expect(result).toMatchObject({ response: "messageResponse", text: "queued note" }) + expect(result.queuedMessageId).toBe(task.messageQueueService.messages[0]?.id) + + task.discardConsumedQueuedMessage(result.queuedMessageId) + expect(task.messageQueueService.isEmpty()).toBe(true) + + // No redelivery: the next drain finds an empty queue. + await expect(task.processQueuedMessages()).resolves.toBe(false) + expect(submitSpy).toHaveBeenCalledTimes(1) + }) + + it("keeps exactly one feedback row when a redelivery follows a partial save failure", async () => { + const task = await createTask({ getState: async () => ({}) }) + + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + task.messageQueueService.addMessage("dedupe me") + const drain = task.processQueuedMessages() + + const result = await askPromise + await drain + const messageId = result.queuedMessageId! + + const taskAccess = getQueueTaskTestAccess(task) + taskAccess.addToClineMessages = async (message) => { + taskAccess.clineMessages.push(message!) + return true + } + // The messages-file write succeeds but metadata persistence fails, so + // saveClineMessages reports false and the entry is released queued. + const saveClineMessages = vi.fn().mockResolvedValue(false) + taskAccess.saveClineMessages = saveClineMessages + + vi.useFakeTimers() + try { + const first = task.persistQueuedFeedbackAndAcknowledge(messageId, result.text, result.images) + await vi.runAllTimersAsync() + await expect(first).resolves.toBe(false) + } finally { + vi.useRealTimers() + } + expect(task.messageQueueService.messages).toHaveLength(1) + expect(taskAccess.clineMessages.filter((message) => message.say === "user_feedback")).toHaveLength(1) + + // Redelivery: the retained entry is acked again and the reconciled + // attempt must update the same row instead of appending a duplicate. + saveClineMessages.mockResolvedValue(true) + await expect(task.persistQueuedFeedbackAndAcknowledge(messageId, result.text, result.images)).resolves.toBe( + true, + ) + const rows = taskAccess.clineMessages.filter((message) => message.say === "user_feedback") + expect(rows).toHaveLength(1) + expect(rows[0].text).toBe("dedupe me") + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("returns success when the user deletes the queue entry during a failed-save backoff", async () => { + const task = await createTask({ getState: async () => ({}) }) + + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + task.messageQueueService.addMessage("deleted during ack") + const drain = task.processQueuedMessages() + + const result = await askPromise + await drain + const messageId = result.queuedMessageId! + + const taskAccess = getQueueTaskTestAccess(task) + taskAccess.addToClineMessages = async (message) => { + taskAccess.clineMessages.push(message!) + return true + } + const saveClineMessages = vi.fn().mockResolvedValue(false) + taskAccess.saveClineMessages = saveClineMessages + + vi.useFakeTimers() + try { + const persistence = task.persistQueuedFeedbackAndAcknowledge(messageId, result.text, result.images) + // The first save fails and the retry backoff starts; while it runs, + // the user deletes the entry from the queue UI (the webview remove + // handler ignores claims). The next save succeeds, so the feedback + // row is durable even though the cleanup removal no-ops. + await vi.advanceTimersByTimeAsync(1) + task.messageQueueService.removeMessage(messageId) + saveClineMessages.mockResolvedValue(true) + await vi.runAllTimersAsync() + await expect(persistence).resolves.toBe(true) + } finally { + vi.useRealTimers() + } + expect(taskAccess.clineMessages.filter((message) => message.say === "user_feedback")).toHaveLength(1) + }) + + it("associates the feedback row before the append so a throwing listener cannot strand it", async () => { + const task = await createTask({ getState: async () => ({}) }) + task.messageQueueService.addMessage("dedupe me") + const result = await task.ask("completion_result", "Done", false) + const messageId = result.queuedMessageId! + + const access = getQueueTaskTestAccess(task) + // Mirror the real append: the row is pushed before the Message emit, + // and a consumer-attached listener can throw synchronously. + access.addToClineMessages = vi.fn(async (message?: ClineMessage) => { + access.clineMessages.push(message!) + access.emit("message") + return true + }) + access.emit = vi.fn(() => { + throw new Error("listener boom") + }) + + await expect(task.persistQueuedFeedbackAndAcknowledge(messageId, result.text, result.images)).rejects.toThrow( + "listener boom", + ) + + // The pushed row must stay associated with the queue entry, and the + // failed write must release the entry for redelivery. + expect(access.clineMessages.filter((message) => message.say === "user_feedback")).toHaveLength(1) + expect(access.queuedFeedbackRows.has(messageId)).toBe(true) + expect(task.messageQueueService.messages).toHaveLength(1) + + // Redelivery reconciles the same row instead of appending a duplicate. + access.emit = vi.fn() + const updateClineMessage = vi.fn(async () => {}) + access.updateClineMessage = updateClineMessage + access.saveClineMessages = vi.fn(async () => true) + await expect(task.persistQueuedFeedbackAndAcknowledge(messageId, result.text, result.images)).resolves.toBe( + true, + ) + expect(updateClineMessage).toHaveBeenCalledTimes(1) + expect(access.clineMessages.filter((message) => message.say === "user_feedback")).toHaveLength(1) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("does not submit queued messages once the task is aborted", async () => { + const task = await createTask({ getState: async () => ({}) }) + const submitSpy = vi.spyOn(task, "submitUserMessage") + task.messageQueueService.addMessage("too late") + getQueueTaskTestAccess(task).abort = true + + await expect(task.processQueuedMessages()).resolves.toBe(false) + + expect(submitSpy).not.toHaveBeenCalled() + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["too late"]) + // The ask-response slot stays empty: no emission, no checkpoint. + expect(task["askResponse"]).toBeUndefined() + }) + + it("drops the drain quietly when abort lands mid-submission", async () => { + const task = await createTask({ getState: async () => ({}) }) + task.messageQueueService.addMessage("too late") + const realSubmit = task.submitUserMessage.bind(task) + vi.spyOn(task, "submitUserMessage").mockImplementation((...args) => { + getQueueTaskTestAccess(task).abort = true + return realSubmit(...args) + }) + + await expect(task.processQueuedMessages()).resolves.toBe(false) + + expect(task.messageQueueService.messages).toHaveLength(1) + expect(task["askResponse"]).toBeUndefined() + }) + + it("does not submit queued messages once the task is abandoned", async () => { + const task = await createTask({ getState: async () => ({}) }) + const submitSpy = vi.spyOn(task, "submitUserMessage") + task.messageQueueService.addMessage("too late") + getQueueTaskTestAccess(task).abandoned = true + + await expect(task.processQueuedMessages()).resolves.toBe(false) + + expect(submitSpy).not.toHaveBeenCalled() + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["too late"]) + expect(task["askResponse"]).toBeUndefined() + }) + + it("drops the drain quietly when abandonment lands mid-submission", async () => { + const task = await createTask({ getState: async () => ({}) }) + task.messageQueueService.addMessage("too late") + const realSubmit = task.submitUserMessage.bind(task) + vi.spyOn(task, "submitUserMessage").mockImplementation((...args) => { + getQueueTaskTestAccess(task).abandoned = true + return realSubmit(...args) + }) + + await expect(task.processQueuedMessages()).resolves.toBe(false) + + expect(task.messageQueueService.messages).toHaveLength(1) + expect(task["askResponse"]).toBeUndefined() + }) + + describe("sayUserFeedbackAndAckQueued", () => { + it("delegates to the durable ack when a queued message was consumed", async () => { + const task = await createTask() + const persist = vi.spyOn(task, "persistQueuedFeedbackAndAcknowledge").mockResolvedValue(true) + const say = vi.spyOn(task, "say") + + await task.sayUserFeedbackAndAckQueued("words", ["img.png"], "queued-1") + + expect(persist).toHaveBeenCalledExactlyOnceWith("queued-1", "words", ["img.png"]) + expect(say).not.toHaveBeenCalled() + }) + + it("throws when the durable ack fails", async () => { + const task = await createTask() + vi.spyOn(task, "persistQueuedFeedbackAndAcknowledge").mockResolvedValue(false) + + await expect(task.sayUserFeedbackAndAckQueued("words", undefined, "queued-1")).rejects.toThrow( + "Failed to persist queued feedback queued-1", + ) + }) + + it("says user feedback without a queued message", async () => { + const task = await createTask() + const say = vi.spyOn(task, "say").mockResolvedValue(true) + + await task.sayUserFeedbackAndAckQueued("direct words", undefined, undefined) + + expect(say).toHaveBeenCalledExactlyOnceWith("user_feedback", "direct words", undefined) + }) + + it("skips saying when there is no feedback content and no queued message", async () => { + const task = await createTask() + const say = vi.spyOn(task, "say") + + await task.sayUserFeedbackAndAckQueued(undefined, undefined, undefined) + + expect(say).not.toHaveBeenCalled() + }) + }) + it("does not consume queued messages for command_output asks", async () => { const task = await createTask() @@ -98,7 +873,10 @@ describe("Task.ask queued message drain", () => { expect(task.messageQueueService.isEmpty()).toBe(true) }) - it("preserves approve-with-feedback behavior for ordinary tool asks", async () => { + it("answers a tool approval ask with a queued message (policy-gated approve-with-feedback)", async () => { + // Upstream semantics (base + #1760): a queued message answers an + // approval ask through the policy-gated claim path, converting to + // yesButtonClicked with the queued text as feedback. const task = await createTask() task.messageQueueService.addMessage("Use this context") @@ -112,7 +890,10 @@ describe("Task.ask queued message drain", () => { ["command", "npm test"], ["use_mcp_server", "{}"], ["tool", "not-json"], - ] as const)("preserves approve-with-feedback behavior for %s asks", async (type, text) => { + ] as const)("lets queued conversational text answer a %s approval ask (policy-gated)", async (type, text) => { + // Upstream semantics (base + #1760): the claim path converts a queued + // message to yesButtonClicked with the queued text as feedback whenever + // the policy allows it (blanket deny disengaged here). const task = await createTask() task.messageQueueService.addMessage("Approval context") @@ -122,6 +903,144 @@ describe("Task.ask queued message drain", () => { expect(task.messageQueueService.isEmpty()).toBe(true) }) + it.each(["finishTask", "newTask"])( + "auto-approves a %s tool ask when the queue is empty (resolution stays inert)", + async (tool) => { + const task = await createTask({ + getState: async () => ({ autoApprovalEnabled: true, alwaysAllowSubtasks: true }), + }) + // No queued message: the ask resolution must not force a manual ask, + // or auto-approval would be bypassed and delegation flows would hang. + const result = await task.ask("tool", JSON.stringify({ tool }), false) + + expect(result).toMatchObject({ response: "yesButtonClicked", text: undefined }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }, + ) + + it("lets a later command ask claim a drained queued message as policy-gated approval", async () => { + // Upstream semantics (base + #1760): the between-turns drain retains the + // entry, and the next command ask claims it through the policy-gated + // path, converting to yesButtonClicked with the queued text. + const task = await createTask({ getState: async () => ({}) }) + const submitSpy = vi.spyOn(task, "submitUserMessage") + + task.messageQueueService.addMessage("also fix the tests") + await expect(task.processQueuedMessages()).resolves.toBe(true) + expect(submitSpy).toHaveBeenCalledTimes(1) + + const result = await task.ask("command", "git push --force", false) + expect(result).toMatchObject({ response: "yesButtonClicked", text: "also fix the tests" }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it.each([ + ["api_req_failed", "stream failed"], + ["auto_approval_max_req_reached", "{}"], + ] as const)("keeps a drain from answering a blocked %s failure gate", async (type, text) => { + // Failure gates are the asks a queued message must never answer: the + // claim path resolves them to undefined and the drain gate refuses the + // submission, so the ask keeps waiting for an explicit user response. + // (Approval asks are different: #1760's policy-gated claim path answers + // them with yesButtonClicked — covered by ask-auto-deny.spec.) + const task = await createTask({ getState: async () => ({}) }) // auto-approval disabled + const submitSpy = vi.spyOn(task, "submitUserMessage") + + const askPromise = task.ask(type, text, false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + task.messageQueueService.addMessage("queued note") + await expect(task.processQueuedMessages()).resolves.toBe(true) + expect(submitSpy).not.toHaveBeenCalled() + + let settled = false + void askPromise.then(() => { + settled = true + }) + await new Promise((resolve) => setTimeout(resolve, 150)) + expect(settled).toBe(false) + + setTimeout(() => task.approveAsk(), 0) + const result = await askPromise + expect(result).toMatchObject({ response: "yesButtonClicked", text: undefined }) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["queued note"]) + + const followup = await task.ask("followup", "anything else?", false) + expect(followup).toMatchObject({ response: "messageResponse", text: "queued note" }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("refuses to overwrite a response a blocked ask is waiting on", async () => { + const task = await createTask({ getState: async () => ({}) }) + + const askPromise = task.ask("followup", "Q?", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + // A direct response (e.g. an Approve click) lands first; a drain racing + // behind it must not overwrite the slot the blocked ask is polling. + task.handleWebviewAskResponse("yesButtonClicked") + await expect(task.submitUserMessage("queued note", undefined, undefined, undefined, "queued-1")).resolves.toBe( + false, + ) + + const result = await askPromise + expect(result).toMatchObject({ response: "yesButtonClicked", text: undefined }) + }) + + it("refuses a queued submission during an approval-gating ask but lets a direct one deny it", async () => { + const task = await createTask({ getState: async () => ({}) }) + + const askPromise = task.ask("command", "npm test", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + // A queued submission carries no user intent and must never answer an + // approval ask on its own. + await expect(task.submitUserMessage("queued note", undefined, undefined, undefined, "queued-1")).resolves.toBe( + false, + ) + + // A direct submission is explicit user intent: it answers the approval + // ask as deny-with-feedback, matching the pre-drain behavior the + // headless API relies on. + await expect(task.submitUserMessage("no, do not run it")).resolves.toBe(true) + + const result = await askPromise + expect(result).toMatchObject({ response: "messageResponse", text: "no, do not run it" }) + }) + + it.each([ + ["api_req_failed", "stream failed"], + ["auto_approval_max_req_reached", JSON.stringify({ count: 3, type: "requests" })], + ] as const)( + "keeps a queued message out of the %s failure gate and delivers it at the next conversational ask", + async (type, text) => { + const task = await createTask({ getState: async () => ({}) }) // auto-approval disabled + task.messageQueueService.addMessage("typed feedback") + + // A failure-gate ask is in flight: any non-yes answer aborts the + // task, so the queued message must not be read as its answer. + const askPromise = task.ask(type, text, false) + await new Promise((resolve) => setTimeout(resolve, 150)) + let settled = false + void askPromise.then(() => { + settled = true + }) + await new Promise((resolve) => setTimeout(resolve, 150)) + expect(settled).toBe(false) + + // The user explicitly retries/approves; the typed text is retained. + setTimeout(() => task.approveAsk(), 0) + const result = await askPromise + expect(result).toMatchObject({ response: "yesButtonClicked", text: undefined }) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["typed feedback"]) + + // The retained message is delivered to the next conversational ask. + const followup = await task.ask("followup", "anything else?", false) + expect(followup).toMatchObject({ response: "messageResponse", text: "typed feedback" }) + 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) @@ -181,9 +1100,12 @@ describe("Task.ask queued message drain", () => { 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 + const addToClineMessages = vi.fn(async (message?: ClineMessage) => { + taskAccess.clineMessages.push(message!) + return true + }) + taskAccess.addToClineMessages = addToClineMessages taskAccess.saveClineMessages = saveClineMessages const persistence = task.persistQueuedFeedbackAndAcknowledge( @@ -194,8 +1116,9 @@ describe("Task.ask queued message drain", () => { await vi.advanceTimersByTimeAsync(250) await persistence - expect(say).toHaveBeenCalledTimes(1) + expect(addToClineMessages).toHaveBeenCalledTimes(1) expect(saveClineMessages).toHaveBeenCalledTimes(2) + expect(taskAccess.clineMessages.filter((message) => message.say === "user_feedback")).toHaveLength(1) expect(task.messageQueueService.isEmpty()).toBe(true) } finally { vi.useRealTimers() @@ -209,7 +1132,7 @@ describe("Task.ask queued message drain", () => { finishAddingAsk = resolve }) const access = getQueueTaskTestAccess(task) - access.addToClineMessages = vi.fn(() => addingAsk) + access.addToClineMessages = vi.fn(() => addingAsk.then(() => true)) task.messageQueueService.addMessage("Still durable") const ask = task.ask("tool", JSON.stringify({ tool: "finishTask" }), false) await Promise.resolve() @@ -228,7 +1151,7 @@ describe("Task.ask queued message drain", () => { finishAddingAsk = resolve }) const access = getQueueTaskTestAccess(task) - access.addToClineMessages = vi.fn(() => addingAsk) + access.addToClineMessages = vi.fn(() => addingAsk.then(() => true)) task.messageQueueService.addMessage("Persist me later") const ask = task.ask("completion_result", "Done", false) await Promise.resolve() @@ -295,4 +1218,126 @@ describe("Task.ask queued message drain", () => { vi.useRealTimers() } }) + + it("releases the claim promptly when abort lands mid-backoff, without further save attempts", async () => { + vi.useFakeTimers() + try { + const task = await createTask() + task.messageQueueService.addMessage("Retry after abort") + const result = await task.ask("completion_result", "Done", false) + const access = getQueueTaskTestAccess(task) + access.say = vi.fn().mockResolvedValue(true) + const saveClineMessages = vi.fn().mockResolvedValue(false) + access.saveClineMessages = saveClineMessages + + const persistence = task.persistQueuedFeedbackAndAcknowledge( + result.queuedMessageId!, + result.text, + result.images, + ) + await vi.advanceTimersByTimeAsync(0) + expect(saveClineMessages).toHaveBeenCalledTimes(1) + + // Abort inside the 250ms backoff: the wait must interrupt well + // before the delay expires, release the claim, and skip the + // remaining retries instead of retaining the task. + access.abort = true + await vi.advanceTimersByTimeAsync(100) + + await expect(persistence).resolves.toBe(false) + expect(saveClineMessages).toHaveBeenCalledTimes(1) + expect(task.messageQueueService.messages).toHaveLength(1) + expect(task.messageQueueService.claimNextMessage()?.text).toBe("Retry after abort") + } finally { + vi.useRealTimers() + } + }) + it("hands the queued ID back through a non-durable claim when a feedback row is registered", async () => { + const task = await createTask({ getState: async () => ({}) }) + task.messageQueueService.addMessage("dedupe me") + // First delivery: the durable interception leaves a registered row and + // a re-queued entry when persistence fails. + const result = await task.ask("completion_result", "Done", false) + const messageId = result.queuedMessageId! + const access = getQueueTaskTestAccess(task) + access.addToClineMessages = vi.fn(async (message?: ClineMessage) => { + access.clineMessages.push(message!) + return true + }) + access.saveClineMessages = vi.fn(async () => false) + access.say = vi.fn().mockResolvedValue(undefined) + + vi.useFakeTimers() + try { + const first = task.persistQueuedFeedbackAndAcknowledge(messageId, result.text, result.images) + await vi.runAllTimersAsync() + await expect(first).resolves.toBe(false) + } finally { + vi.useRealTimers() + } + expect(task.messageQueueService.messages).toHaveLength(1) + expect(access.queuedFeedbackRows.has(messageId)).toBe(true) + + // Redelivery consumed by a non-durable ask (followup): the registered + // row must route the consumer through the durable ack so it reconciles + // the same row instead of appending a duplicate via the + // say("user_feedback") fallback. + const followup = await task.ask("followup", "Q?", false) + expect(followup).toMatchObject({ response: "messageResponse", text: "dedupe me" }) + expect(followup.queuedMessageId).toBe(messageId) + // The entry is not removed inline; the durable ack owns its removal. + expect(task.messageQueueService.messages).toHaveLength(1) + + access.saveClineMessages = vi.fn(async () => true) + await task.sayUserFeedbackAndAckQueued(followup.text, followup.images, followup.queuedMessageId) + expect(access.clineMessages.filter((message) => message.say === "user_feedback")).toHaveLength(1) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("releases the queued message when the reconciled row update fails", async () => { + const task = await createTask({ getState: async () => ({}) }) + + const askPromise = task.ask("completion_result", "Done", false) + await new Promise((resolve) => setTimeout(resolve, 150)) + + task.messageQueueService.addMessage("dedupe me") + const drain = task.processQueuedMessages() + + const result = await askPromise + await drain + const messageId = result.queuedMessageId! + + const taskAccess = getQueueTaskTestAccess(task) + const updateClineMessage = vi.fn(async () => {}) + taskAccess.updateClineMessage = updateClineMessage + taskAccess.addToClineMessages = async (message) => { + taskAccess.clineMessages.push(message!) + return true + } + // First ack: all saves fail, releasing the entry queued while the row + // association is retained for a later redelivery. + const saveClineMessages = vi.fn().mockResolvedValue(false) + taskAccess.saveClineMessages = saveClineMessages + + vi.useFakeTimers() + try { + const first = task.persistQueuedFeedbackAndAcknowledge(messageId, result.text, result.images) + await vi.runAllTimersAsync() + await expect(first).resolves.toBe(false) + } finally { + vi.useRealTimers() + } + expect(task.messageQueueService.messages).toHaveLength(1) + + // Redelivery: the reconciled row update fails. The failure must + // propagate and release the entry — it must NOT be acked/removed. + updateClineMessage.mockRejectedValueOnce(new Error("webview update failed")) + saveClineMessages.mockResolvedValue(true) + + await expect(task.persistQueuedFeedbackAndAcknowledge(messageId, result.text, result.images)).rejects.toThrow( + "webview update failed", + ) + expect(saveClineMessages).toHaveBeenCalledTimes(4) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["dedupe me"]) + }) }) diff --git a/src/core/task/__tests__/grace-retry-errors.spec.ts b/src/core/task/__tests__/grace-retry-errors.spec.ts index dc057957bc..c1825548eb 100644 --- a/src/core/task/__tests__/grace-retry-errors.spec.ts +++ b/src/core/task/__tests__/grace-retry-errors.spec.ts @@ -291,7 +291,7 @@ describe("Grace Retry Error Handling", () => { startTask: false, }) - const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) + const saySpy = vi.spyOn(task, "say").mockResolvedValue(true) // Simulate first empty response - should NOT show error task.consecutiveNoAssistantMessagesCount = 0 @@ -315,7 +315,7 @@ describe("Grace Retry Error Handling", () => { startTask: false, }) - const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) + const saySpy = vi.spyOn(task, "say").mockResolvedValue(true) // Simulate second consecutive empty response task.consecutiveNoAssistantMessagesCount = 1 @@ -339,7 +339,7 @@ describe("Grace Retry Error Handling", () => { startTask: false, }) - const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) + const saySpy = vi.spyOn(task, "say").mockResolvedValue(true) // Simulate third consecutive empty response task.consecutiveNoAssistantMessagesCount = 2 @@ -411,7 +411,7 @@ describe("Grace Retry Error Handling", () => { startTask: false, }) - const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) + const saySpy = vi.spyOn(task, "say").mockResolvedValue(true) // Simulate the error condition (2 consecutive failures) task.consecutiveNoAssistantMessagesCount = 2 diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 3b664b3bd2..3bd3dd818b 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -216,7 +216,9 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { if (!didApprove) { await task.diffViewProvider.revertChanges() - task.processQueuedMessages() + void task.processQueuedMessages().catch((error) => { + console.error("[ApplyDiffTool] Failed to process queued messages:", error) + }) return } @@ -257,14 +259,18 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { this.resetPartialState() // Process any queued messages after file edit completes - task.processQueuedMessages() + void task.processQueuedMessages().catch((error) => { + console.error("[ApplyDiffTool] Failed to process queued messages:", error) + }) return } catch (error) { await handleError("applying diff", error as Error) await task.diffViewProvider.reset() this.resetPartialState() - task.processQueuedMessages() + void task.processQueuedMessages().catch((error) => { + console.error("[ApplyDiffTool] Failed to process queued messages:", error) + }) return } } diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts index 56b2bf8909..9bec4c609c 100644 --- a/src/core/tools/ApplyPatchTool.ts +++ b/src/core/tools/ApplyPatchTool.ts @@ -226,7 +226,9 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, true) pushToolResult(message) await task.diffViewProvider.reset() - task.processQueuedMessages() + void task.processQueuedMessages().catch((error) => { + console.error("[ApplyPatchTool] Failed to process queued messages:", error) + }) } private async handleDeleteFile( @@ -283,7 +285,9 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task.didEditFile = true pushToolResult(`Successfully deleted ${relPath}`) - task.processQueuedMessages() + void task.processQueuedMessages().catch((error) => { + console.error("[ApplyPatchTool] Failed to process queued messages:", error) + }) } private async handleUpdateFile( @@ -446,7 +450,9 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, false) pushToolResult(message) await task.diffViewProvider.reset() - task.processQueuedMessages() + void task.processQueuedMessages().catch((error) => { + console.error("[ApplyPatchTool] Failed to process queued messages:", error) + }) } override async handlePartial(task: Task, block: ToolUse<"apply_patch">): Promise { diff --git a/src/core/tools/AskFollowupQuestionTool.ts b/src/core/tools/AskFollowupQuestionTool.ts index 3baea7ed52..3371321e78 100644 --- a/src/core/tools/AskFollowupQuestionTool.ts +++ b/src/core/tools/AskFollowupQuestionTool.ts @@ -70,9 +70,18 @@ export class AskFollowupQuestionTool extends BaseTool<"ask_followup_question"> { } task.consecutiveMistakeCount = 0 - const { text, images } = await task.ask("followup", JSON.stringify(follow_up_json), false) + const { text, images, queuedMessageId } = await task.ask("followup", JSON.stringify(follow_up_json), false) const safeText = text ?? "" - await task.say("user_feedback", safeText, images) + if (queuedMessageId) { + // The answer came from the durable queue: persist it and remove the + // queue entry only after the history write succeeds. + const persisted = await task.persistQueuedFeedbackAndAcknowledge(queuedMessageId, text, images) + if (!persisted) { + throw new Error(`Failed to persist queued follow-up feedback ${queuedMessageId}`) + } + } else { + await task.say("user_feedback", safeText, images) + } pushToolResult(formatResponse.toolResult(`\n${safeText}\n`, images)) } catch (error) { await handleError("asking question", error as Error) diff --git a/src/core/tools/EditFileTool.ts b/src/core/tools/EditFileTool.ts index a7301e2ac9..18644de710 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -167,7 +167,10 @@ export class EditFileTool extends BaseTool<"edit_file"> { } // Finalize the existing partial tool ask row so the UI doesn't get stuck in a spinner state. - await task.ask("tool", JSON.stringify(sharedMessageProps), false).catch(() => {}) + const result = await task.ask("tool", JSON.stringify(sharedMessageProps), false).catch(() => undefined) + // The result is only used to finalize the row; a consumed queued + // message is dropped without inventing a history write. + task.discardConsumedQueuedMessage(result?.queuedMessageId) } const recordFailureForPathAndMaybeEscalate = async (relPath: string, formattedError: string): Promise => { @@ -467,7 +470,9 @@ export class EditFileTool extends BaseTool<"edit_file"> { this.resetPartialState() // Process any queued messages after file edit completes - task.processQueuedMessages() + void task.processQueuedMessages().catch((error) => { + console.error("[EditFileTool] Failed to process queued messages:", error) + }) } catch (error) { if (relPathForErrorHandling) { await finalizePartialToolAskIfNeeded(relPathForErrorHandling) diff --git a/src/core/tools/EditTool.ts b/src/core/tools/EditTool.ts index 2ae8bf4ed0..29eb9a91ff 100644 --- a/src/core/tools/EditTool.ts +++ b/src/core/tools/EditTool.ts @@ -233,7 +233,9 @@ export class EditTool extends BaseTool<"edit"> { this.resetPartialState() // Process any queued messages after file edit completes - task.processQueuedMessages() + void task.processQueuedMessages().catch((error) => { + console.error("[EditTool] Failed to process queued messages:", error) + }) } catch (error) { await handleError("edit", error as Error) await task.diffViewProvider.reset() diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index a6034e4f4d..e4b935ed16 100644 --- a/src/core/tools/ExecuteCommandTool.ts +++ b/src/core/tools/ExecuteCommandTool.ts @@ -293,6 +293,27 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { // Convert agent-specified timeout from seconds to milliseconds const agentTimeout = resolveAgentTimeoutMs(timeoutSeconds) + // The background-completion drain inside executeCommandInTerminal + // must not run before this command's tool result is published: a + // background command can finish during the settle delay while the + // result is still pending. Settle at each pushToolResult site with + // the publication outcome so the drain waits on a structural signal + // and skips draining after a failed publication (handleError + // reports first; the queued message waits for a later turn). + let settleToolResultPublished: ((published: boolean) => void) | undefined + const toolResultPublished = new Promise((resolve) => { + settleToolResultPublished = resolve + }) + const publishToolResult = (result: ToolResponse): void => { + try { + pushToolResult(result) + } catch (error) { + settleToolResultPublished?.(false) + throw error + } + settleToolResultPublished?.(true) + } + const options: ExecuteCommandOptions = { executionId, command: canonicalCommand, @@ -300,16 +321,22 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { terminalShellIntegrationDisabled, commandExecutionTimeout, agentTimeout, + toolResultPublished, } + let shouldDrainQueuedMessages = false try { - const [rejected, result] = await executeCommandInTerminal(task, options) + const [rejected, result, commandSubmitted] = await executeCommandInTerminal(task, options) if (rejected) { task.didRejectTool = true } - pushToolResult(result) + publishToolResult(result) + // Only drain queued messages when the command actually ran + // (early validation failures end the turn without an execution, + // matching file tools' error-path behavior). + shouldDrainQueuedMessages = commandSubmitted } catch (error: unknown) { // Invalidate pending ask from first execution to prevent race condition task.supersedePendingAsk() @@ -319,27 +346,65 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { const status: CommandExecutionStatus = { executionId, status: "fallback" } postCommandExecutionStatus(provider, status) - const [rejected, result] = await executeCommandInTerminal(task, { - ...options, - terminalShellIntegrationDisabled: true, - }) - - if (rejected) { - task.didRejectTool = true + try { + const [rejected, result, commandSubmitted] = await executeCommandInTerminal(task, { + ...options, + terminalShellIntegrationDisabled: true, + }) + + if (rejected) { + task.didRejectTool = true + } + + publishToolResult(result) + shouldDrainQueuedMessages = commandSubmitted + } catch (fallbackError) { + // The retry never published: settle the signal (and + // rethrow) so the background-completion drain chain + // awaiting toolResultPublished cannot hang forever and + // hold task references. Settling is idempotent. + settleToolResultPublished?.(false) + throw fallbackError } - - pushToolResult(result) - } else { + } else if (error instanceof ShellIntegrationError) { // Command was submitted but shell integration lost track of it — show warning. - await task.say("shell_integration_warning") - - if (error instanceof ShellIntegrationError) { - pushToolResult( + try { + await task.say("shell_integration_warning") + publishToolResult( "Command was submitted in the VS Code terminal, but shell integration did not report its output or completion status. Do not run the command again automatically.", ) - } else { - pushToolResult(`Command failed to execute in terminal due to a shell integration error.`) + } catch (warningError) { + // Same hang hazard as the fallback arm: a throw before + // publishToolResult leaves the signal unsettled. + settleToolResultPublished?.(false) + throw warningError } + } else { + // Ordinary execution error (e.g. the terminal failed to start) — + // not a shell-integration failure, so it must not emit the + // shell-integration warning; surface it through the tool's + // error path instead. + // Settle the publication signal before the error bubbles: the + // background-completion drain chain captured toolResultPublished + // and awaits it, so an unresolved promise would hang that chain + // (and hold task references) forever. The outcome is false, so + // the chain never drains after a failed publication. Settling + // is idempotent. + settleToolResultPublished?.(false) + throw error + } + } + + // The drain runs outside the execution catch above and is awaited so a + // failed queued-message submission is observable. A drain failure is + // logged and the message stays claimed-released for a later drain, so + // it must never be misattributed as a shell-integration error after + // the tool result was already published. + if (shouldDrainQueuedMessages) { + try { + await task.processQueuedMessages() + } catch (error) { + console.error("[ExecuteCommandTool] Failed to process queued messages:", error) } } @@ -363,6 +428,14 @@ export type ExecuteCommandOptions = { terminalShellIntegrationDisabled?: boolean commandExecutionTimeout?: number agentTimeout?: number + /** + * Resolves once the tool result for this command has been published. + * Resolves true on successful publication and false when publication + * failed; the background-completion drain awaits this before processing + * queued messages so they are never submitted ahead of the current tool + * result, and skips draining entirely after a failed publication. + */ + toolResultPublished?: Promise } export async function executeCommandInTerminal( @@ -374,8 +447,10 @@ export async function executeCommandInTerminal( terminalShellIntegrationDisabled = true, commandExecutionTimeout = 0, agentTimeout = 0, + // Direct callers without a tool-result lifecycle drain immediately. + toolResultPublished = Promise.resolve(true), }: ExecuteCommandOptions, -): Promise<[boolean, ToolResponse]> { +): Promise<[boolean, ToolResponse, boolean]> { // Convert milliseconds back to seconds for display purposes. const commandExecutionTimeoutSeconds = commandExecutionTimeout / 1000 let workingDir: string @@ -391,7 +466,8 @@ export async function executeCommandInTerminal( try { await fs.access(workingDir) } catch (error) { - return [false, `Working directory '${workingDir}' does not exist.`] + // The command never ran (working directory validation failed). + return [false, `Working directory '${workingDir}' does not exist.`, false] } let runInBackground = false @@ -541,6 +617,35 @@ export async function executeCommandInTerminal( // errors here are UI-only and must not surface to the tool result. commandOutputSayChain .then(() => queueCommandOutputMessage(result, false, true)) + .then(async () => { + // The tool returned a "still running" result and drained when the + // agent timeout moved the command to the background; process + // messages queued since then so they are not held until an + // unrelated path drains them. The tool result for this command + // must be published first: completion can land inside the + // settle delay while the result is still pending, so wait on + // the per-invocation signal instead of timer ordering. + if (!runInBackground) { + return + } + const published = await toolResultPublished + // A task cancelled while the command finished in the + // background must not drain: the guards inside + // processQueuedMessages would no-op anyway, and skipping here + // avoids pointless work plus error-log noise on the dying task. + if (task.abort || task.abandoned) { + return + } + // A failed publication must not drain either: handleError + // reports the command error first, and the queued message + // waits for a later turn. + if (!published) { + return + } + return task.processQueuedMessages().catch((error) => { + console.error("[ExecuteCommandTool] Failed to process queued messages:", error) + }) + }) .catch((error) => { console.error("[ExecuteCommandTool] Failed to flush final command_output:", error) }) @@ -628,6 +733,8 @@ export async function executeCommandInTerminal( return [ false, `The command was terminated after exceeding a user-configured ${commandExecutionTimeoutSeconds}s timeout. Do not try to re-run the command.`, + + true, ] } throw error @@ -663,7 +770,7 @@ export async function executeCommandInTerminal( // Use persisted output format when output was truncated and spilled to disk if (persistedResult?.truncated) { - return [false, formatPersistedOutput(persistedResult, exitDetails, currentWorkingDir)] + return [false, formatPersistedOutput(persistedResult, exitDetails, currentWorkingDir), true] } // Use inline format for small outputs (original behavior with exit status). @@ -678,6 +785,7 @@ export async function executeCommandInTerminal( return [ false, `Command executed in terminal within working directory '${currentWorkingDir}'. ${exitStatus}\nOutput:\n${result}`, + true, ] } else { return [ @@ -687,6 +795,8 @@ export async function executeCommandInTerminal( result.length > 0 ? `Here's the output so far:\n${result}\n` : "\n", "You will be updated on the terminal status and new output in the future.", ].join("\n"), + + true, ] } } diff --git a/src/core/tools/ReadFileTool.ts b/src/core/tools/ReadFileTool.ts index 2107cfe21b..3d231467b5 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/src/core/tools/ReadFileTool.ts @@ -446,15 +446,15 @@ export class ReadFileTool extends BaseTool<"read_file"> { }) const completeMessage = JSON.stringify({ tool: "readFile", batchFiles } satisfies ClineSayTool) - const { response, text, images } = await task.ask("tool", completeMessage, false) + const { response, text, images, queuedMessageId } = await task.ask("tool", completeMessage, false) if (response === "yesButtonClicked") { - if (text) await task.say("user_feedback", text, images) + await task.sayUserFeedbackAndAckQueued(text, images, queuedMessageId) filesToApprove.forEach((fr) => { updateFileResult(fr.path, { status: "approved", feedbackText: text, feedbackImages: images }) }) } else if (response === "noButtonClicked") { - if (text) await task.say("user_feedback", text, images) + await task.sayUserFeedbackAndAckQueued(text, images, queuedMessageId) task.didRejectTool = true filesToApprove.forEach((fr) => { updateFileResult(fr.path, { @@ -465,7 +465,9 @@ export class ReadFileTool extends BaseTool<"read_file"> { }) }) } else { - // Individual permissions + // Individual permissions. A queued message consumed here is free + // text, not a permissions payload, so drop it without a write. + task.discardConsumedQueuedMessage(queuedMessageId) try { const individualPermissions = JSON.parse(text || "{}") let hasAnyDenial = false @@ -515,10 +517,10 @@ export class ReadFileTool extends BaseTool<"read_file"> { startLine, } satisfies ClineSayTool) - const { response, text, images } = await task.ask("tool", completeMessage, false) + const { response, text, images, queuedMessageId } = await task.ask("tool", completeMessage, false) if (response !== "yesButtonClicked") { - if (text) await task.say("user_feedback", text, images) + await task.sayUserFeedbackAndAckQueued(text, images, queuedMessageId) task.didRejectTool = true updateFileResult(relPath, { status: "denied", @@ -527,7 +529,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { feedbackImages: images, }) } else { - if (text) await task.say("user_feedback", text, images) + await task.sayUserFeedbackAndAckQueued(text, images, queuedMessageId) updateFileResult(relPath, { status: "approved", feedbackText: text, feedbackImages: images }) } } @@ -712,16 +714,16 @@ export class ReadFileTool extends BaseTool<"read_file"> { reason: lineSnippet || undefined, } satisfies ClineSayTool) - const { response, text, images } = await task.ask("tool", completeMessage, false) + const { response, text, images, queuedMessageId } = await task.ask("tool", completeMessage, false) if (response !== "yesButtonClicked") { - if (text) await task.say("user_feedback", text, images) + await task.sayUserFeedbackAndAckQueued(text, images, queuedMessageId) task.didRejectTool = true results.push(`File: ${relPath}\nStatus: Denied by user`) continue } - if (text) await task.say("user_feedback", text, images) + await task.sayUserFeedbackAndAckQueued(text, images, queuedMessageId) try { // Check if the path is a directory diff --git a/src/core/tools/SearchReplaceTool.ts b/src/core/tools/SearchReplaceTool.ts index e29b124010..2ba1dc7aa4 100644 --- a/src/core/tools/SearchReplaceTool.ts +++ b/src/core/tools/SearchReplaceTool.ts @@ -229,7 +229,9 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { this.resetPartialState() // Process any queued messages after file edit completes - task.processQueuedMessages() + void task.processQueuedMessages().catch((error) => { + console.error("[SearchReplaceTool] Failed to process queued messages:", error) + }) } catch (error) { await handleError("search and replace", error as Error) await task.diffViewProvider.reset() diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 0c5c80abb9..95e361208d 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -180,7 +180,9 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await task.diffViewProvider.reset() this.resetPartialState() - task.processQueuedMessages() + void task.processQueuedMessages().catch((error) => { + console.error("[WriteToFileTool] Failed to process queued messages:", error) + }) return } catch (error) { diff --git a/src/core/tools/__tests__/CodebaseSearchTool.spec.ts b/src/core/tools/__tests__/CodebaseSearchTool.spec.ts index bc88d44ec1..651c2aa1aa 100644 --- a/src/core/tools/__tests__/CodebaseSearchTool.spec.ts +++ b/src/core/tools/__tests__/CodebaseSearchTool.spec.ts @@ -59,7 +59,7 @@ describe("CodebaseSearchTool", () => { sayAndCreateMissingParamError: vi .fn() .mockResolvedValue("missing query"), - say: vi.fn().mockResolvedValue(undefined), + say: vi.fn().mockResolvedValue(true), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), } task = taskStub as Task diff --git a/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts b/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts index f2c4cc2aed..7ed0309460 100644 --- a/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts +++ b/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts @@ -79,7 +79,7 @@ describe("CodebaseSearchTool workspace selection", () => { cwd: second.uri.fsPath, providerRef: new WeakRef(provider), consecutiveMistakeCount: 0, - say: vi.fn().mockResolvedValue(undefined), + say: vi.fn().mockResolvedValue(true), } task = taskStub as Task callbacks = { diff --git a/src/core/tools/__tests__/applyDiffTool.spec.ts b/src/core/tools/__tests__/applyDiffTool.spec.ts new file mode 100644 index 0000000000..cb93ff5b3c --- /dev/null +++ b/src/core/tools/__tests__/applyDiffTool.spec.ts @@ -0,0 +1,192 @@ +// npx vitest run core/tools/__tests__/applyDiffTool.spec.ts + +import type { MockedFunction } from "vitest" + +import { fileExistsAtPath } from "../../../utils/fs" +import type { Task } from "../../task/Task" +import { ApplyDiffTool } from "../ApplyDiffTool" + +vi.mock("fs/promises", () => ({ + default: { + readFile: vi.fn().mockResolvedValue("original file content\n"), + }, +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), +})) + +describe("ApplyDiffTool.execute - queued message drain failures", () => { + const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction + + let tool: ApplyDiffTool + let mockTask: Pick< + Task, + | "cwd" + | "api" + | "consecutiveMistakeCount" + | "consecutiveMistakeCountForApplyDiff" + | "recordToolError" + | "rooIgnoreController" + | "rooProtectedController" + | "say" + | "sayAndCreateMissingParamError" + | "processQueuedMessages" + | "didEditFile" + | "providerRef" + | "diffViewProvider" + | "diffStrategy" + | "fileContextTracker" + > + let mockProcessQueuedMessages: MockedFunction<() => Promise> + let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise> + let mockHandleError: MockedFunction<(...args: unknown[]) => Promise> + let mockPushToolResult: MockedFunction<(...args: unknown[]) => void> + + beforeEach(() => { + vi.clearAllMocks() + + mockedFileExistsAtPath.mockResolvedValue(true) + mockProcessQueuedMessages = vi.fn().mockResolvedValue(true) + + mockTask = { + cwd: "/workspace/project", + api: { + getModel: vi.fn().mockReturnValue({ id: "claude-3" }), + } as unknown as Task["api"], + consecutiveMistakeCount: 0, + consecutiveMistakeCountForApplyDiff: new Map(), + recordToolError: vi.fn(), + rooIgnoreController: { + validateAccess: vi.fn().mockReturnValue(true), + } as unknown as Task["rooIgnoreController"], + rooProtectedController: { + isWriteProtected: vi.fn().mockReturnValue(false), + } as unknown as Task["rooProtectedController"], + say: vi.fn().mockResolvedValue(undefined), + sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing param error"), + processQueuedMessages: mockProcessQueuedMessages, + didEditFile: false, + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + }), + }), + } as unknown as Task["providerRef"], + diffViewProvider: { + editType: undefined, + isEditing: false, + originalContent: "", + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + saveChanges: vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: null, + finalContent: "final content", + }), + saveDirectly: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + pushToolWriteResult: vi.fn().mockResolvedValue("Tool result message"), + } as unknown as Task["diffViewProvider"], + diffStrategy: { + applyDiff: vi.fn().mockResolvedValue({ success: true, content: "updated file content" }), + } as unknown as Task["diffStrategy"], + fileContextTracker: { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } as unknown as Task["fileContextTracker"], + } + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + mockPushToolResult = vi.fn() + + tool = new ApplyDiffTool() + }) + + it("logs a drain failure when the user rejects the diff", async () => { + mockAskApproval.mockResolvedValue(false) + const drainError = new Error("queued submission failed") + mockProcessQueuedMessages.mockRejectedValue(drainError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await tool.execute({ path: "src/existing.ts", diff: "updated file content" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // Flush the fire-and-forget drain promise so its rejection is logged. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockTask.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockPushToolResult).not.toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[ApplyDiffTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("logs a drain failure after a successful diff edit without changing the tool result", async () => { + const drainError = new Error("queued submission failed") + mockProcessQueuedMessages.mockRejectedValue(drainError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await tool.execute({ path: "src/existing.ts", diff: "updated file content" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // Flush the fire-and-forget drain promise so its rejection is logged. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockPushToolResult).toHaveBeenCalledWith("Tool result message") + expect(mockTask.didEditFile).toBe(true) + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[ApplyDiffTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("logs a drain failure when the edit fails after the diff view reset", async () => { + const saveError = new Error("save failed") + ;( + mockTask.diffViewProvider.saveChanges as MockedFunction<(...args: unknown[]) => Promise> + ).mockRejectedValue(saveError) + const drainError = new Error("queued submission failed") + mockProcessQueuedMessages.mockRejectedValue(drainError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await tool.execute({ path: "src/existing.ts", diff: "updated file content" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // Flush the fire-and-forget drain promise so its rejection is logged. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockHandleError).toHaveBeenCalledWith("applying diff", saveError) + expect(mockTask.diffViewProvider.reset).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[ApplyDiffTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) +}) diff --git a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts index 72ffb112bc..5608d52c27 100644 --- a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts +++ b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts @@ -2,6 +2,8 @@ import type { MockedFunction } from "vitest" +import fs from "fs/promises" + import { fileExistsAtPath } from "../../../utils/fs" import { isPathOutsideWorkspace } from "../../../utils/pathUtils" import type { Task } from "../../task/Task" @@ -61,7 +63,7 @@ describe("ApplyPatchTool.execute - delete file success path", () => { isWriteProtected: vi.fn().mockReturnValue(false), } as unknown as Task["rooProtectedController"], say: vi.fn().mockResolvedValue(undefined), - processQueuedMessages: vi.fn(), + processQueuedMessages: vi.fn().mockResolvedValue(true), didEditFile: false, } @@ -94,3 +96,184 @@ describe("ApplyPatchTool.execute - delete file success path", () => { expect(mockTask.recordToolError).not.toHaveBeenCalled() }) }) + +describe("ApplyPatchTool.execute - queued message drain failures", () => { + const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction + const mockedIsPathOutsideWorkspace = isPathOutsideWorkspace as MockedFunction + const mockedReadFile = fs.readFile as MockedFunction + + let tool: ApplyPatchTool + let mockTask: Pick< + Task, + | "cwd" + | "consecutiveMistakeCount" + | "recordToolUsage" + | "recordToolError" + | "rooIgnoreController" + | "rooProtectedController" + | "say" + | "processQueuedMessages" + | "didEditFile" + | "providerRef" + | "diffViewProvider" + | "fileContextTracker" + > + let mockProcessQueuedMessages: MockedFunction<() => Promise> + let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise> + let mockHandleError: MockedFunction<(...args: unknown[]) => Promise> + let mockPushToolResult: MockedFunction<(...args: unknown[]) => void> + + beforeEach(() => { + vi.clearAllMocks() + + mockedFileExistsAtPath.mockResolvedValue(true) + mockedIsPathOutsideWorkspace.mockReturnValue(false) + mockedReadFile.mockResolvedValue("original file content\n") + mockProcessQueuedMessages = vi.fn().mockResolvedValue(true) + + mockTask = { + cwd: "/workspace/project", + consecutiveMistakeCount: 0, + recordToolUsage: vi.fn(), + recordToolError: vi.fn(), + rooIgnoreController: { + validateAccess: vi.fn().mockReturnValue(true), + } as unknown as Task["rooIgnoreController"], + rooProtectedController: { + isWriteProtected: vi.fn().mockReturnValue(false), + } as unknown as Task["rooProtectedController"], + say: vi.fn().mockResolvedValue(undefined), + processQueuedMessages: mockProcessQueuedMessages, + didEditFile: false, + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + }), + }), + } as unknown as Task["providerRef"], + diffViewProvider: { + editType: undefined, + isEditing: false, + originalContent: "", + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + saveChanges: vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: null, + finalContent: "final content", + }), + saveDirectly: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + pushToolWriteResult: vi.fn().mockResolvedValue("Tool result message"), + } as unknown as Task["diffViewProvider"], + fileContextTracker: { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } as unknown as Task["fileContextTracker"], + } + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + mockPushToolResult = vi.fn() + + tool = new ApplyPatchTool() + }) + + it("logs a drain failure after deleting a file without changing the tool result", async () => { + const drainError = new Error("queued submission failed") + mockProcessQueuedMessages.mockRejectedValue(drainError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + const patch = `*** Begin Patch +*** Delete File: src/obsolete.ts +*** End Patch` + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // Flush the fire-and-forget drain promise so its rejection is logged. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully deleted")) + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[ApplyPatchTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("logs a drain failure after writing a new file without changing the tool result", async () => { + mockedFileExistsAtPath.mockResolvedValue(false) + const drainError = new Error("queued submission failed") + mockProcessQueuedMessages.mockRejectedValue(drainError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + const patch = `*** Begin Patch +*** Add File: src/created.ts ++created content +*** End Patch` + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // Flush the fire-and-forget drain promise so its rejection is logged. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockPushToolResult).toHaveBeenCalledWith("Tool result message") + expect(mockTask.didEditFile).toBe(true) + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[ApplyPatchTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("logs a drain failure after updating a file without changing the tool result", async () => { + const drainError = new Error("queued submission failed") + mockProcessQueuedMessages.mockRejectedValue(drainError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + const patch = `*** Begin Patch +*** Update File: src/existing.ts +@@ +-original file content ++updated file content +*** End Patch` + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // Flush the fire-and-forget drain promise so its rejection is logged. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockPushToolResult).toHaveBeenCalledWith("Tool result message") + expect(mockTask.didEditFile).toBe(true) + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[ApplyPatchTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) +}) diff --git a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts index 9f081279ca..b9772d3368 100644 --- a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts +++ b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts @@ -23,7 +23,8 @@ describe("AskFollowupQuestionTool", () => { didToolFailInCurrentTurn: false, sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"), ask: vi.fn().mockResolvedValue({ text: "User answer", images: [] }), - say: vi.fn().mockResolvedValue(undefined), + say: vi.fn().mockResolvedValue(true), + persistQueuedFeedbackAndAcknowledge: vi.fn().mockResolvedValue(true), } as unknown as Task mockCallbacks = { @@ -261,6 +262,46 @@ describe("AskFollowupQuestionTool", () => { expect(mockCallbacks.handleError).toHaveBeenCalledWith("asking question", error) }) + // ===== Queued answer ack tests ===== + + it("should persist and ack a queued answer instead of saying it", async () => { + const params = { question: "Which approach?", follow_up: [{ text: "Approach 1" }] } + vi.mocked(mockTask.ask).mockResolvedValue({ + response: "messageResponse", + text: "queued words", + images: [], + queuedMessageId: "queued-1", + }) + + await tool.execute(params, mockTask, mockCallbacks) + + expect(mockTask.persistQueuedFeedbackAndAcknowledge).toHaveBeenCalledWith("queued-1", "queued words", []) + expect(mockTask.say).not.toHaveBeenCalled() + expect(mockCallbacks.pushToolResult).toHaveBeenCalledWith( + formatResponse.toolResult("\nqueued words\n", []), + ) + expect(mockCallbacks.handleError).not.toHaveBeenCalled() + }) + + it("should call handleError without pushing a tool result when the queued ack fails", async () => { + const params = { question: "Which approach?", follow_up: [{ text: "Approach 1" }] } + vi.mocked(mockTask.ask).mockResolvedValue({ + response: "messageResponse", + text: "queued words", + images: [], + queuedMessageId: "queued-1", + }) + vi.mocked(mockTask.persistQueuedFeedbackAndAcknowledge).mockResolvedValue(false) + + await tool.execute(params, mockTask, mockCallbacks) + + expect(mockCallbacks.handleError).toHaveBeenCalledWith( + "asking question", + new Error("Failed to persist queued follow-up feedback queued-1"), + ) + expect(mockCallbacks.pushToolResult).not.toHaveBeenCalled() + }) + // ===== handlePartial tests ===== it("should show question during partial streaming via handlePartial", async () => { diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 1ff8d52a8d..2a3a55cd17 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -147,9 +147,10 @@ describe("editFileTool", () => { } mockTask.say = vi.fn().mockResolvedValue(undefined) mockTask.ask = vi.fn().mockResolvedValue(undefined) + mockTask.discardConsumedQueuedMessage = vi.fn() mockTask.recordToolError = vi.fn() mockTask.recordToolUsage = vi.fn() - mockTask.processQueuedMessages = vi.fn() + mockTask.processQueuedMessages = vi.fn().mockResolvedValue(undefined) mockTask.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") mockAskApproval = vi.fn().mockResolvedValue(true) @@ -599,6 +600,7 @@ describe("editFileTool", () => { await executeEditFileTool({ old_string: "NonExistent" }, { isPartial: true }) await executeEditFileTool({ old_string: "NonExistent" }, { isPartial: true }) + mockTask.ask.mockResolvedValue({ response: "yesButtonClicked", queuedMessageId: "queued-finalize" }) await executeEditFileTool( { old_string: "NonExistent" }, { isPartial: false, fileContent: "Line 1\nLine 2\nLine 3" }, @@ -607,6 +609,8 @@ describe("editFileTool", () => { const askCalls = mockTask.ask.mock.calls const hasFinalToolAsk = askCalls.some((call: any[]) => call[0] === "tool" && call[2] === false) expect(hasFinalToolAsk).toBe(true) + // The finalize ask consumed a queued message that the tool drops. + expect(mockTask.discardConsumedQueuedMessage).toHaveBeenCalledExactlyOnceWith("queued-finalize") }) it("finalizes a partial tool preview row on no-op success (no changes needed)", async () => { @@ -620,6 +624,7 @@ describe("editFileTool", () => { { isPartial: true, fileContent: "Line 1\nLine 2\nLine 3" }, ) + mockTask.ask.mockResolvedValue({ response: "yesButtonClicked", queuedMessageId: "queued-finalize" }) const result = await executeEditFileTool( { old_string: " Line 2", new_string: "Line 2" }, { isPartial: false, fileContent: "Line 1\nLine 2\nLine 3" }, @@ -629,6 +634,20 @@ describe("editFileTool", () => { const askCalls = mockTask.ask.mock.calls const hasFinalToolAsk = askCalls.some((call: any[]) => call[0] === "tool" && call[2] === false) expect(hasFinalToolAsk).toBe(true) + expect(mockTask.discardConsumedQueuedMessage).toHaveBeenCalledExactlyOnceWith("queued-finalize") + }) + + it("drops nothing when the finalize ask itself rejects", async () => { + await executeEditFileTool({ old_string: "NonExistent" }, { isPartial: true }) + await executeEditFileTool({ old_string: "NonExistent" }, { isPartial: true }) + + mockTask.ask.mockRejectedValue(new Error("superseded partial message")) + await executeEditFileTool( + { old_string: "NonExistent" }, + { isPartial: false, fileContent: "Line 1\nLine 2\nLine 3" }, + ) + + expect(mockTask.discardConsumedQueuedMessage).toHaveBeenCalledExactlyOnceWith(undefined) }) }) @@ -677,6 +696,27 @@ describe("editFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("edit_file", expect.any(Error)) expect(mockTask.diffViewProvider.reset).toHaveBeenCalled() }) + + it("logs a queued-message drain failure after a successful edit without changing the tool result", async () => { + const drainError = new Error("queued submission failed") + mockTask.processQueuedMessages.mockRejectedValue(drainError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + const result = await executeEditFileTool() + + // Flush the fire-and-forget drain promise so its rejection is logged. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(result).toBe("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[EditFileTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) describe("file tracking", () => { diff --git a/src/core/tools/__tests__/editTool.spec.ts b/src/core/tools/__tests__/editTool.spec.ts index a5f665b9e5..29cf6d4a46 100644 --- a/src/core/tools/__tests__/editTool.spec.ts +++ b/src/core/tools/__tests__/editTool.spec.ts @@ -145,7 +145,7 @@ describe("editTool", () => { mockTask.ask = vi.fn().mockResolvedValue(undefined) mockTask.recordToolError = vi.fn() mockTask.recordToolUsage = vi.fn() - mockTask.processQueuedMessages = vi.fn() + mockTask.processQueuedMessages = vi.fn().mockResolvedValue(undefined) mockTask.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") mockAskApproval = vi.fn().mockResolvedValue(true) @@ -415,6 +415,27 @@ describe("editTool", () => { expect(mockHandleError).toHaveBeenCalledWith("edit", expect.any(Error)) expect(mockTask.diffViewProvider.reset).toHaveBeenCalled() }) + + it("logs a queued-message drain failure after a successful edit without changing the tool result", async () => { + const drainError = new Error("queued submission failed") + mockTask.processQueuedMessages.mockRejectedValue(drainError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + const result = await executeEditTool() + + // Flush the fire-and-forget drain promise so its rejection is logged. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(result).toBe("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[EditTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) describe("file tracking", () => { diff --git a/src/core/tools/__tests__/executeCommand.spec.ts b/src/core/tools/__tests__/executeCommand.spec.ts index 7146fa930b..ab4c07ffa0 100644 --- a/src/core/tools/__tests__/executeCommand.spec.ts +++ b/src/core/tools/__tests__/executeCommand.spec.ts @@ -261,10 +261,11 @@ describe("executeCommand", () => { } // Execute - const [rejected, result] = await executeCommandInTerminal(mockTask, options) + const [rejected, result, commandSubmitted] = await executeCommandInTerminal(mockTask, options) // Verify expect(rejected).toBe(false) + expect(commandSubmitted).toBe(false) expect(result).toBe(`Working directory '${nonExistentCwd}' does not exist.`) expect(TerminalRegistry.getOrCreateTerminal).not.toHaveBeenCalled() }) @@ -356,9 +357,10 @@ describe("executeCommand", () => { commandExecutionTimeout: 1_000, }) await vitest.advanceTimersByTimeAsync(1_000) - const [rejected, result] = await executionPromise + const [rejected, result, commandSubmitted] = await executionPromise expect(rejected).toBe(false) + expect(commandSubmitted).toBe(true) expect(result).toContain("terminated after exceeding") expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith( expect.objectContaining({ @@ -386,10 +388,11 @@ describe("executeCommand", () => { } // Execute - const [rejected, result] = await executeCommandInTerminal(mockTask, options) + const [rejected, result, commandSubmitted] = await executeCommandInTerminal(mockTask, options) // Verify expect(rejected).toBe(false) + expect(commandSubmitted).toBe(true) expect(result).toContain("Exit code: 0") expect(result).toContain("within working directory '/test/project'") }) @@ -444,10 +447,11 @@ describe("executeCommand", () => { } // Execute - const [rejected, result] = await executeCommandInTerminal(mockTask, options) + const [rejected, result, commandSubmitted] = await executeCommandInTerminal(mockTask, options) // Verify expect(rejected).toBe(false) + expect(commandSubmitted).toBe(true) expect(result).toContain("Process terminated by signal SIGINT") expect(result).toContain("within working directory '/test/project'") }) diff --git a/src/core/tools/__tests__/executeCommandTool.spec.ts b/src/core/tools/__tests__/executeCommandTool.spec.ts index 98640fe969..b1554a3ddb 100644 --- a/src/core/tools/__tests__/executeCommandTool.spec.ts +++ b/src/core/tools/__tests__/executeCommandTool.spec.ts @@ -2,6 +2,7 @@ import type { ToolUsage } from "@roo-code/types" import * as vscode from "vscode" +import fs from "fs/promises" import { Task } from "../../task/Task" import * as autoApprovalModule from "../../auto-approval" @@ -49,6 +50,40 @@ vitest.mock("../../../integrations/terminal/TerminalRegistry", () => ({ }, })) +const mockInterceptorInstances: Array<{ write: ReturnType; finalize: ReturnType }> = + vitest.hoisted(() => []) + +vitest.mock("../../../integrations/terminal/OutputInterceptor", () => ({ + // vitest 4 mocks used with `new` must be function/class implementations. + OutputInterceptor: class { + write = vitest.fn() + finalize = vitest.fn().mockResolvedValue({ truncated: false }) + constructor(..._args: unknown[]) { + mockInterceptorInstances.push(this) + } + }, +})) + +vitest.mock("../../../utils/storage", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + getTaskDirectoryPath: vitest.fn().mockResolvedValue("/test/storage/task-1/command-output"), + } +}) + +vitest.mock("@roo-code/telemetry", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + TelemetryService: class { + static instance = { + captureShellIntegrationError: vitest.fn(), + } + }, + } +}) + // The handler calls `isBlanketDenyEngaged` and reads // `BLANKET_DENY_AUTO_DENY_KINDS` from the Task module, so the blanket-policy // helpers must stay real while the Task class itself stays a stub. @@ -90,7 +125,9 @@ describe("executeCommandTool", () => { vitest.useRealTimers() // Spy on executeCommandInTerminal and mock its return value - vitest.spyOn(executeCommandModule, "executeCommandInTerminal").mockResolvedValue([false, "Command executed"]) + vitest + .spyOn(executeCommandModule, "executeCommandInTerminal") + .mockResolvedValue([false, "Command executed", true]) // Create mock implementations with eslint directives to handle the type issues mockCline = { @@ -105,6 +142,7 @@ describe("executeCommandTool", () => { recordToolUsage: vitest.fn().mockReturnValue({} as ToolUsage), recordToolError: vitest.fn(), supersedePendingAsk: vitest.fn(), + processQueuedMessages: vitest.fn(), recordBlanketCommandDenial: vitest.fn(), providerRef: { deref: vitest.fn().mockResolvedValue({ @@ -862,6 +900,32 @@ describe("executeCommandTool", () => { expect(executeCommandModule.canRetryShellIntegrationError(error)).toBe(false) }) + it("routes a generic terminal-start error to the execution error path without draining", async () => { + const runError = new Error("terminal process failed to start") + vitest.mocked(TerminalRegistry.getOrCreateTerminal).mockResolvedValueOnce({ + runCommand: vitest.fn().mockImplementation(() => { + throw runError + }), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + // A generic terminal failure is an ordinary execution error, not a + // shell-integration failure: the shell-integration warning and its + // dedicated result must not appear (see the ShellIntegrationError + // test for that distinct path). + expect(mockHandleError).toHaveBeenCalledWith("executing command", runError) + expect(mockCline.say).not.toHaveBeenCalledWith("shell_integration_warning") + expect(mockPushToolResult).not.toHaveBeenCalled() + // The command never ran, so queued messages must not be drained. + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + }) + it("selects the Execa fallback provider for cmd.exe shell integration", () => { vitest.spyOn(Terminal, "isActiveShellCmdExe").mockReturnValue(true) @@ -872,6 +936,374 @@ describe("executeCommandTool", () => { }) }) + describe("Queued message processing", () => { + it("processes queued messages after the command completes", async () => { + mockToolUse.params.command = "echo test" + mockToolUse.nativeArgs = { command: "echo test" } + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + // The tool result must be published before queued messages are processed. + expect(mockPushToolResult.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.processQueuedMessages.mock.invocationCallOrder[0], + ) + }) + + it("processes queued messages after the execa fallback retry completes", async () => { + const shellError = new executeCommandModule.ShellIntegrationError("startup failed", false) + const failedProcess = Object.assign(Promise.reject(shellError), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + const successfulProcess = Object.assign(Promise.resolve(), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + const successfulTerminalProcess = successfulProcess as unknown as RooTerminalProcess + + vitest + .mocked(TerminalRegistry.getOrCreateTerminal) + .mockResolvedValueOnce({ + runCommand: vitest.fn().mockReturnValue(failedProcess), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + .mockResolvedValueOnce({ + runCommand: vitest.fn().mockImplementation((_command: string, callbacks: RooTerminalCallbacks) => { + void callbacks.onCompleted?.("", successfulTerminalProcess) + callbacks.onShellExecutionComplete?.({ exitCode: 0 }, successfulTerminalProcess) + return successfulProcess + }), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + expect(mockPushToolResult.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.processQueuedMessages.mock.invocationCallOrder[0], + ) + }) + + it("processes queued messages once after a command terminated by an interruption", async () => { + mockToolUse.params.command = "long-running-command" + mockToolUse.nativeArgs = { command: "long-running-command" } + + vitest.mocked(TerminalRegistry.getOrCreateTerminal).mockResolvedValue({ + runCommand: vitest.fn().mockImplementation((_command: string, callbacks: RooTerminalCallbacks) => { + const interruptedProcess = Object.assign(Promise.resolve(), { + continue: vitest.fn(), + abort: vitest.fn(), + }) as unknown as RooTerminalProcess + void callbacks.onCompleted?.("Command interrupted", interruptedProcess) + callbacks.onShellExecutionComplete?.( + { exitCode: undefined, signalName: "SIGINT", coreDumpPossible: false }, + interruptedProcess, + ) + return interruptedProcess + }), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("Process terminated by signal SIGINT"), + ) + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + // The tool result must be published before queued messages are processed. + expect(mockPushToolResult.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.processQueuedMessages.mock.invocationCallOrder[0], + ) + }) + + it("processes queued messages once after a user-configured execution timeout", async () => { + mockToolUse.params.command = "sleep 10" + mockToolUse.nativeArgs = { command: "sleep 10" } + // 0.1s user timeout keeps the test fast while exercising the real abort path. + vitest.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vitest + .fn() + .mockImplementation((key: string, defaultValue: unknown) => + key === "commandExecutionTimeout" ? 0.1 : defaultValue, + ), + } as unknown as vscode.WorkspaceConfiguration) + + const pendingProcess = Object.assign(new Promise(() => {}), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + vitest.mocked(TerminalRegistry.getOrCreateTerminal).mockResolvedValue({ + runCommand: vitest.fn().mockReturnValue(pendingProcess), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("terminated after exceeding a user-configured"), + ) + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + expect(mockPushToolResult.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.processQueuedMessages.mock.invocationCallOrder[0], + ) + }) + + it("does not process queued messages when the user rejects the command", async () => { + mockAskApproval.mockResolvedValue(false) + mockToolUse.params.command = "echo test" + mockToolUse.nativeArgs = { command: "echo test" } + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + expect(mockPushToolResult).not.toHaveBeenCalled() + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + }) + + it("does not process queued messages when the working directory does not exist", async () => { + mockToolUse.params.command = "echo test" + mockToolUse.params.cwd = "/nonexistent/working/dir" + mockToolUse.nativeArgs = { command: "echo test", cwd: "/nonexistent/working/dir" } + vitest.mocked(fs.access).mockRejectedValueOnce(new Error("ENOENT")) + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + "Working directory '/nonexistent/working/dir' does not exist.", + ) + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + }) + + it("logs a queued-message drain failure after a successful result without a shell-integration warning or a second result", async () => { + mockToolUse.params.command = "echo test" + mockToolUse.nativeArgs = { command: "echo test" } + const successfulProcess = Object.assign(Promise.resolve(), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + const successfulTerminalProcess = successfulProcess as unknown as RooTerminalProcess + vitest.mocked(TerminalRegistry.getOrCreateTerminal).mockResolvedValue({ + runCommand: vitest.fn().mockImplementation((_command: string, callbacks: RooTerminalCallbacks) => { + void callbacks.onCompleted?.("", successfulTerminalProcess) + callbacks.onShellExecutionComplete?.({ exitCode: 0 }, successfulTerminalProcess) + return successfulProcess + }), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + const drainError = new Error("queued submission failed") + mockCline.processQueuedMessages.mockRejectedValueOnce(drainError) + const consoleErrorSpy = vitest.spyOn(console, "error").mockImplementation(() => {}) + try { + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + // The successful result is published exactly once: the drain failure + // must not emit a shell-integration warning or push a contradictory + // command-failure result on top of it. + expect(mockPushToolResult).toHaveBeenCalledTimes(1) + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Command executed in terminal")) + expect(mockCline.say).not.toHaveBeenCalledWith("shell_integration_warning") + expect(mockHandleError).not.toHaveBeenCalled() + // The failure is logged, not swallowed silently. + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[ExecuteCommandTool] Failed to process queued messages:", + drainError, + ) + + // The message stays queued for a later drain: the next command's + // drain still runs. + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(2) + expect(mockPushToolResult).toHaveBeenCalledTimes(2) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("logs a queued-message drain failure after a successful execa fallback retry without a shell-integration warning or a second result", async () => { + const shellError = new executeCommandModule.ShellIntegrationError("startup failed", false) + const failedProcess = Object.assign(Promise.reject(shellError), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + const successfulProcess = Object.assign(Promise.resolve(), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + const successfulTerminalProcess = successfulProcess as unknown as RooTerminalProcess + + vitest + .mocked(TerminalRegistry.getOrCreateTerminal) + .mockResolvedValueOnce({ + runCommand: vitest.fn().mockReturnValue(failedProcess), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + .mockResolvedValueOnce({ + runCommand: vitest.fn().mockImplementation((_command: string, callbacks: RooTerminalCallbacks) => { + void callbacks.onCompleted?.("", successfulTerminalProcess) + callbacks.onShellExecutionComplete?.({ exitCode: 0 }, successfulTerminalProcess) + return successfulProcess + }), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + + const drainError = new Error("queued submission failed") + mockCline.processQueuedMessages.mockRejectedValueOnce(drainError) + const consoleErrorSpy = vitest.spyOn(console, "error").mockImplementation(() => {}) + try { + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + expect(mockPushToolResult).toHaveBeenCalledTimes(1) + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Command executed in terminal")) + expect(mockCline.say).not.toHaveBeenCalledWith("shell_integration_warning") + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[ExecuteCommandTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("does not drain when the execa fallback retry hits a working directory failure", async () => { + mockToolUse.params.command = "echo test" + mockToolUse.params.cwd = "/nonexistent/working/dir" + mockToolUse.nativeArgs = { command: "echo test", cwd: "/nonexistent/working/dir" } + // First attempt passes validation but fails shell integration startup + // (retryable); the retry then fails the working directory check. + vitest.mocked(fs.access).mockResolvedValueOnce(undefined).mockRejectedValueOnce(new Error("ENOENT")) + const shellError = new executeCommandModule.ShellIntegrationError("startup failed", false) + const failedProcess = Object.assign(Promise.reject(shellError), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + vitest.mocked(TerminalRegistry.getOrCreateTerminal).mockResolvedValueOnce({ + runCommand: vitest.fn().mockReturnValue(failedProcess), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + "Working directory '/nonexistent/working/dir' does not exist.", + ) + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + }) + + it("settles the publication signal and reports when the execa fallback retry throws", async () => { + mockToolUse.params.command = "npm test" + mockToolUse.nativeArgs = { command: "npm test" } + // First attempt fails shell integration startup (retryable); the + // execa retry then fails with an ordinary terminal error before any + // result is published. + const shellError = new executeCommandModule.ShellIntegrationError("startup failed", false) + const retryError = new Error("terminal crashed during retry") + const failedProcess = Object.assign(Promise.reject(shellError), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + const retryProcess = Object.assign(Promise.reject(retryError), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + vitest + .mocked(TerminalRegistry.getOrCreateTerminal) + .mockResolvedValueOnce({ + runCommand: vitest.fn().mockReturnValue(failedProcess), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + .mockResolvedValueOnce({ + runCommand: vitest.fn().mockReturnValue(retryProcess), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + // The retry error is reported through the tool error path and no + // result is published; the catch arm also settles the shared + // toolResultPublished signal (false) so the background-completion + // drain chain awaiting it cannot hang forever and hold task refs. + expect(mockHandleError).toHaveBeenCalledWith("executing command", retryError) + expect(mockPushToolResult).not.toHaveBeenCalled() + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + }) + + it("settles the publication signal and reports when the shell-integration warning say throws", async () => { + mockToolUse.params.command = "npm test" + mockToolUse.nativeArgs = { command: "npm test" } + // Command was submitted but shell integration lost track of it + // (non-retryable); the warning say then fails before publication. + const shellError = new executeCommandModule.ShellIntegrationError("lost track", true) + const failedProcess = Object.assign(Promise.reject(shellError), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + vitest.mocked(TerminalRegistry.getOrCreateTerminal).mockResolvedValueOnce({ + runCommand: vitest.fn().mockReturnValue(failedProcess), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + const sayError = new Error("webview gone") + mockCline.say.mockRejectedValueOnce(sayError) + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledWith("executing command", sayError) + expect(mockPushToolResult).not.toHaveBeenCalled() + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + }) + }) + describe("Command execution timeout configuration", () => { it("should include timeout parameter in ExecuteCommandOptions", () => { // This test verifies that the timeout configuration is properly typed @@ -1106,5 +1538,395 @@ describe("executeCommandTool", () => { expect(mockPushToolResult).toHaveBeenCalled() expect(mockPushToolResult.mock.calls[0][0]).toContain("still running") }) + + it("drains messages queued during a background run after publishing the final command_output update", async () => { + vitest.useFakeTimers() + mockCline.processQueuedMessages.mockResolvedValue(true) + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("npm run dev", 2) + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("server starting...\n", proc) + + // The agent timeout moves the command to the background; the tool + // returns its "still running" result and drains immediately. + await vitest.advanceTimersByTimeAsync(2_000) + await handlePromise + + expect(mockPushToolResult.mock.calls[0][0]).toContain("still running") + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + + // When the background command later completes, the messages queued + // since the immediate drain are processed after the final + // non-partial command_output update is published. + await callbacks.onCompleted!("server exited\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + await vitest.advanceTimersByTimeAsync(100) + + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(2) + + const finalOutputCallIndex = mockCline.say.mock.calls.findIndex( + (call: unknown[]) => call[0] === "command_output" && call[3] === false, + ) + expect(finalOutputCallIndex).not.toBe(-1) + expect(mockCline.say.mock.invocationCallOrder[finalOutputCallIndex]).toBeLessThan( + mockCline.processQueuedMessages.mock.invocationCallOrder[1], + ) + }) + + it("publishes the tool result before draining when a background command completes during the settle delay", async () => { + vitest.useFakeTimers() + mockCline.processQueuedMessages.mockResolvedValue(true) + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("npm run dev", 2) + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("server starting...\n", proc) + + // The agent timeout moves the command to the background. Advance in + // small steps and stop as soon as the transition happens: the tool + // then sits in the 50 ms settle delay with the tool result still + // pending publication. + for (let i = 0; terminal.proc.continue.mock.calls.length === 0 && i < 200; i++) { + await vitest.advanceTimersByTimeAsync(25) + } + expect(terminal.proc.continue).toHaveBeenCalled() + expect(mockPushToolResult).not.toHaveBeenCalled() + + // Completion lands inside the settle delay, while the tool result + // is still pending publication. + await callbacks.onCompleted!("server exited\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + expect(mockPushToolResult).not.toHaveBeenCalled() + // The background-completion drain is gated on the tool result. + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + + await vitest.advanceTimersByTimeAsync(100) + await handlePromise + await vitest.advanceTimersByTimeAsync(0) + + expect(mockPushToolResult).toHaveBeenCalledTimes(1) + // Completion already landed, so the tool returns the completed + // result rather than a "still running" one. + expect(mockPushToolResult.mock.calls[0][0]).toContain("Command executed in terminal") + expect(mockPushToolResult.mock.calls[0][0]).toContain("Exit code: 0") + // The immediate post-result drain plus the background-completion drain. + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(2) + // Queued messages must never be processed before the tool result. + expect(mockPushToolResult.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.processQueuedMessages.mock.invocationCallOrder[0], + ) + expect(mockPushToolResult.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.processQueuedMessages.mock.invocationCallOrder[1], + ) + }) + + it("warns without draining when a submitted command loses shell integration", async () => { + vitest.useFakeTimers() + mockCline.providerRef.deref.mockResolvedValue({ + contextProxy: { getValue: vitest.fn().mockReturnValue(false) }, + getState: vitest.fn().mockResolvedValue({ terminalShellIntegrationDisabled: false }), + postMessageToWebview: vitest.fn().mockResolvedValue(undefined), + }) + vitest.spyOn(Terminal, "isActiveShellCmdExe").mockReturnValue(false) + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("Write-Output hello") + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + expect(terminal.provider).toBe("vscode") + callbacks.onShellExecutionStarted!(1234, proc) + // The command was submitted, but shell integration cannot report + // its completion, so the retry path must not run. + callbacks.onNoShellIntegration!({ message: "exit code unknown", commandSubmitted: true }, proc) + terminal.resolveProcess() + await vitest.advanceTimersByTimeAsync(100) + await handlePromise + + expect(mockCline.say).toHaveBeenCalledWith("shell_integration_warning") + expect(mockPushToolResult).toHaveBeenCalledTimes(1) + expect(mockPushToolResult).toHaveBeenCalledWith( + "Command was submitted in the VS Code terminal, but shell integration did not report its output or completion status. Do not run the command again automatically.", + ) + // A submitted command that loses shell integration did complete its + // execution path, but the tool must not drain queued messages here. + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("logs a background-completion drain failure after the final command_output update", async () => { + vitest.useFakeTimers() + const drainError = new Error("queued submission failed") + mockCline.processQueuedMessages.mockResolvedValueOnce(true).mockRejectedValueOnce(drainError) + const consoleErrorSpy = vitest.spyOn(console, "error").mockImplementation(() => {}) + try { + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("npm run dev", 2) + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("server starting...\n", proc) + + // The agent timeout moves the command to the background; the tool + // returns its "still running" result and drains immediately. + await vitest.advanceTimersByTimeAsync(2_000) + await handlePromise + + expect(mockPushToolResult.mock.calls[0][0]).toContain("still running") + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + + // When the background command later completes, the drain failure + // is logged and never surfaces as a second tool result. + await callbacks.onCompleted!("server exited\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + await vitest.advanceTimersByTimeAsync(100) + + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(2) + // The final non-partial command_output update is published before + // the background-completion drain runs. + const finalOutputCallIndex = mockCline.say.mock.calls.findIndex( + (call: unknown[]) => call[0] === "command_output" && call[3] === false, + ) + expect(finalOutputCallIndex).not.toBe(-1) + expect(mockCline.say.mock.invocationCallOrder[finalOutputCallIndex]).toBeLessThan( + mockCline.processQueuedMessages.mock.invocationCallOrder[1], + ) + expect(mockPushToolResult).toHaveBeenCalledTimes(1) + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[ExecuteCommandTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("skips the background-completion drain when the task was abandoned but still publishes the result", async () => { + vitest.useFakeTimers() + mockCline.processQueuedMessages.mockResolvedValue(true) + mockCline.abandoned = true + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("npm run dev", 2) + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("server starting...\n", proc) + + // The agent timeout moves the command to the background; the tool + // returns its "still running" result and runs the post-result drain. + await vitest.advanceTimersByTimeAsync(2_000) + await handlePromise + + expect(mockPushToolResult.mock.calls[0][0]).toContain("still running") + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + + // When the background command later completes, the + // background-completion drain is skipped on the abandoned task. + await callbacks.onCompleted!("server exited\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + await vitest.advanceTimersByTimeAsync(100) + + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + expect(mockPushToolResult).toHaveBeenCalledTimes(1) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("does not drain the background chain when publication fails on a backgrounded command", async () => { + vitest.useFakeTimers() + const consoleErrorSpy = vitest.spyOn(console, "error").mockImplementation(() => {}) + try { + // A process that never settles: the agent-timeout racer moves the + // command to background while the process stays pending. + const pendingProcess = Object.assign(new Promise(() => {}), { + continue: vitest.fn(), + abort: vitest.fn(), + }) as unknown as RooTerminalProcess + let capturedCallbacks: RooTerminalCallbacks | undefined + vitest.mocked(TerminalRegistry.getOrCreateTerminal).mockResolvedValue({ + runCommand: vitest.fn().mockImplementation((_command: string, callbacks: RooTerminalCallbacks) => { + capturedCallbacks = callbacks + return pendingProcess + }), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + + // The tool-result publication itself throws: a generic + // (non-shell-integration) error that reaches the else branch. + const publishError = new Error("tool result publication failed") + mockPushToolResult.mockImplementationOnce(() => { + throw publishError + }) + + const handlePromise = handleCommand("npm test", 2) + + await vitest.waitFor(() => expect(capturedCallbacks).toBeDefined()) + const callbacks = capturedCallbacks! + callbacks.onShellExecutionStarted!(1234, pendingProcess) + await callbacks.onLine!("partial output\n", pendingProcess) + + // The agent timeout fires while the tool sits in the settle + // delay: runInBackground is set, so the completion chain created + // below really awaits the publication signal. + await vitest.advanceTimersByTimeAsync(2_000) + + await callbacks.onCompleted!("partial output\n", pendingProcess) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, pendingProcess) + await vitest.advanceTimersByTimeAsync(0) + + // The settle delay elapses, the tool tries to publish, and the + // publication error rethrows through the generic error path. The + // signal settles with a failure outcome, so the awaiting chain + // releases without draining: handleError reports the error first + // and the queued message waits for a later turn. + await vitest.advanceTimersByTimeAsync(100) + await handlePromise + await vitest.advanceTimersByTimeAsync(0) + + expect(mockHandleError).toHaveBeenCalledWith("executing command", publishError) + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("settles the chain without draining when the retry publication throws", async () => { + vitest.useFakeTimers() + const consoleErrorSpy = vitest.spyOn(console, "error").mockImplementation(() => {}) + try { + // Attempt 1 (vscode): shell-integration startup race, command not + // submitted, so the tool retries through execa. + const shellError = new executeCommandModule.ShellIntegrationError("startup failed", false) + const failedProcess = Object.assign(Promise.reject(shellError), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + // Attempt 2 (execa): a process that never settles; the agent + // timeout moves the retried command to background. + const pendingProcess = Object.assign(new Promise(() => {}), { + continue: vitest.fn(), + abort: vitest.fn(), + }) as unknown as RooTerminalProcess + let retryCallbacks: RooTerminalCallbacks | undefined + vitest + .mocked(TerminalRegistry.getOrCreateTerminal) + .mockResolvedValueOnce({ + runCommand: vitest.fn().mockReturnValue(failedProcess), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + .mockResolvedValueOnce({ + runCommand: vitest + .fn() + .mockImplementation((_command: string, callbacks: RooTerminalCallbacks) => { + retryCallbacks = callbacks + return pendingProcess + }), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + } as never) + + // The retry's own publication throws: the fallback publish call + // must also settle the signal (success=false) instead of leaving + // the completion chain waiting. + const publishError = new Error("retry publication failed") + mockPushToolResult.mockImplementationOnce(() => { + throw publishError + }) + + const handlePromise = handleCommand("npm test", 2) + + await vitest.waitFor(() => expect(retryCallbacks).toBeDefined()) + const callbacks = retryCallbacks! + callbacks.onShellExecutionStarted!(1234, pendingProcess) + await callbacks.onLine!("retry output\n", pendingProcess) + + await vitest.advanceTimersByTimeAsync(2_000) + + await callbacks.onCompleted!("retry output\n", pendingProcess) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, pendingProcess) + await vitest.advanceTimersByTimeAsync(100) + await handlePromise + await vitest.advanceTimersByTimeAsync(0) + + expect(mockHandleError).toHaveBeenCalledWith("executing command", publishError) + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("returns the persisted output format when the interceptor reports truncated output", async () => { + vitest.useFakeTimers() + mockCline.providerRef.deref.mockResolvedValue({ + context: { globalStorageUri: { fsPath: "/test/storage" } }, + contextProxy: { getValue: vitest.fn().mockReturnValue(false) }, + getState: vitest.fn().mockResolvedValue({ + terminalOutputLineLimit: 500, + terminalOutputCharacterLimit: 100000, + terminalShellIntegrationDisabled: true, + }), + postMessageToWebview: vitest.fn().mockResolvedValue(undefined), + }) + const persisted = { + truncated: true, + totalBytes: 200_000, + artifactPath: "/test/storage/task-1/command-output/exec-1.txt", + preview: "head\n...[omitted middle content]...\ntail", + } + const terminal = await setupControllableTerminal() + // Other tests (DCG approval flows) also construct interceptors, so + // index relative to the instances that exist before this command. + const interceptorIndex = mockInterceptorInstances.length + + const handlePromise = handleCommand("cat big.log") + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + expect(mockInterceptorInstances.length).toBe(interceptorIndex + 1) + mockInterceptorInstances[interceptorIndex].finalize.mockResolvedValue(persisted) + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("line\n", proc) + await callbacks.onCompleted!("line\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + terminal.resolveProcess() + await vitest.advanceTimersByTimeAsync(100) + await handlePromise + + expect(mockPushToolResult).toHaveBeenCalledTimes(1) + const result = mockPushToolResult.mock.calls[0][0] + expect(result).toContain("Command executed in '/test/workspace'. Exit code: 0") + expect(result).toContain("Output (195.3KB) persisted. Artifact ID: exec-1.txt") + expect(result).toContain(persisted.preview) + expect(result).toContain("Use read_command_output tool to view full output if needed.") + // Truncated persisted output reports commandSubmitted: true, so the + // post-result drain runs exactly once, after the tool result. + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + expect(mockPushToolResult.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.processQueuedMessages.mock.invocationCallOrder[0], + ) + }) }) }) diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 6c9e177d38..2778e8b9ed 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -18,6 +18,7 @@ import { isBinaryFile } from "isbinaryfile" import { readFileTool, ReadFileTool } from "../ReadFileTool" import { formatResponse } from "../../prompts/responses" +import type { Task } from "../../task/Task" import { validateImageForProcessing, processImageFile, @@ -153,6 +154,8 @@ function createMockTask(options: MockTaskOptions = {}) { didRejectTool: false, ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: undefined, images: undefined }), say: vi.fn().mockResolvedValue(undefined), + sayUserFeedbackAndAckQueued: vi.fn().mockResolvedValue(undefined), + discardConsumedQueuedMessage: vi.fn(), sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing required parameter: path"), recordToolError: vi.fn(), rooIgnoreController: { @@ -630,7 +633,12 @@ describe("ReadFileTool", () => { await readFileTool.execute({ path: "test.ts" }, mockTask as any, callbacks) - expect(mockTask.say).toHaveBeenCalledWith("user_feedback", "Please be careful with this file", undefined) + expect(mockTask.sayUserFeedbackAndAckQueued).toHaveBeenCalledWith( + "Please be careful with this file", + undefined, + undefined, + ) + expect(mockTask.say).not.toHaveBeenCalled() expect(formatResponse.toolApprovedWithFeedback).toHaveBeenCalledWith("Please be careful with this file") }) @@ -646,9 +654,122 @@ describe("ReadFileTool", () => { await readFileTool.execute({ path: "secrets.env" }, mockTask as any, callbacks) - expect(mockTask.say).toHaveBeenCalledWith("user_feedback", "This file contains secrets", undefined) + expect(mockTask.sayUserFeedbackAndAckQueued).toHaveBeenCalledWith( + "This file contains secrets", + undefined, + undefined, + ) + expect(mockTask.say).not.toHaveBeenCalled() expect(formatResponse.toolDeniedWithFeedback).toHaveBeenCalledWith("This file contains secrets") }) + + it("acks a queued message intercepted by the approval ask instead of leaving it queued", async () => { + const mockTask = createMockTask() + const callbacks = createMockCallbacks() + + mockTask.ask.mockResolvedValue({ + response: "noButtonClicked", + text: "Do not read it", + images: undefined, + queuedMessageId: "queued-1", + }) + + await readFileTool.execute( + { path: "secrets.env" }, + // Double assertion: the file's `as any` budget is capped by + // eslint-suppressions.json and must not grow. + mockTask as unknown as Task, + callbacks, + ) + + expect(mockTask.sayUserFeedbackAndAckQueued).toHaveBeenCalledExactlyOnceWith( + "Do not read it", + undefined, + "queued-1", + ) + expect(mockTask.say).not.toHaveBeenCalled() + expect(mockTask.discardConsumedQueuedMessage).not.toHaveBeenCalled() + }) + + it("acks queued feedback on a batch approval", async () => { + const mockTask = createMockTask() + const callbacks = createMockCallbacks() + + mockTask.ask.mockResolvedValue({ + response: "yesButtonClicked", + text: "ok to proceed", + images: undefined, + queuedMessageId: "queued-batch-yes", + }) + + await readFileTool["requestApproval"]( + mockTask as unknown as Task, + [ + { path: "a.ts", status: "pending", entry: { path: "a.ts", mode: "slice", offset: 1 } }, + { path: "b.ts", status: "pending", entry: { path: "b.ts", mode: "slice", offset: 1 } }, + ], + () => {}, + ) + + expect(mockTask.sayUserFeedbackAndAckQueued).toHaveBeenCalledExactlyOnceWith( + "ok to proceed", + undefined, + "queued-batch-yes", + ) + expect(mockTask.say).not.toHaveBeenCalled() + }) + + it("discards a queued message consumed by the batch permissions branch", async () => { + const mockTask = createMockTask() + const callbacks = createMockCallbacks() + + mockTask.ask.mockResolvedValue({ + response: "messageResponse", + text: "free-form note", + images: undefined, + queuedMessageId: "queued-batch", + }) + + await readFileTool["requestApproval"]( + mockTask as unknown as Task, + [ + { path: "a.ts", status: "pending", entry: { path: "a.ts", mode: "slice", offset: 1 } }, + { path: "b.ts", status: "pending", entry: { path: "b.ts", mode: "slice", offset: 1 } }, + ], + () => {}, + ) + + expect(mockTask.discardConsumedQueuedMessage).toHaveBeenCalledExactlyOnceWith("queued-batch") + expect(mockTask.sayUserFeedbackAndAckQueued).not.toHaveBeenCalled() + expect(mockTask.say).not.toHaveBeenCalledWith("user_feedback", expect.anything(), expect.anything()) + // Free text is not a permissions payload, so both files are denied. + expect(mockTask.didRejectTool).toBe(true) + }) + + it("acks queued feedback on a legacy per-file denial", async () => { + const mockTask = createMockTask() + const callbacks = createMockCallbacks() + + mockTask.ask.mockResolvedValue({ + response: "noButtonClicked", + text: "Do not read it", + images: undefined, + queuedMessageId: "queued-legacy", + }) + + await readFileTool.execute( + { files: [{ path: "legacy-secret.ts" }] } as unknown as Parameters[0], + mockTask as unknown as Task, + callbacks, + ) + + expect(mockTask.sayUserFeedbackAndAckQueued).toHaveBeenCalledExactlyOnceWith( + "Do not read it", + undefined, + "queued-legacy", + ) + expect(mockTask.discardConsumedQueuedMessage).not.toHaveBeenCalled() + }) }) describe("output structure", () => { @@ -936,7 +1057,8 @@ describe("ReadFileTool", () => { await readFileTool.execute({ files: [{ path: "test.ts" }] } as any, mockTask as any, callbacks) - expect(mockTask.say).toHaveBeenCalledWith("user_feedback", "Read carefully", undefined) + expect(mockTask.sayUserFeedbackAndAckQueued).toHaveBeenCalledWith("Read carefully", undefined, undefined) + expect(mockTask.say).not.toHaveBeenCalled() }) it("should handle user feedback on denial in legacy format", async () => { @@ -951,7 +1073,8 @@ describe("ReadFileTool", () => { await readFileTool.execute({ files: [{ path: "secret.ts" }] } as any, mockTask as any, callbacks) - expect(mockTask.say).toHaveBeenCalledWith("user_feedback", "Not allowed", undefined) + expect(mockTask.sayUserFeedbackAndAckQueued).toHaveBeenCalledWith("Not allowed", undefined, undefined) + expect(mockTask.say).not.toHaveBeenCalled() }) it("should handle truncation in legacy format when no line ranges", async () => { diff --git a/src/core/tools/__tests__/searchReplaceTool.spec.ts b/src/core/tools/__tests__/searchReplaceTool.spec.ts index 5cf10790d4..6c00abdad9 100644 --- a/src/core/tools/__tests__/searchReplaceTool.spec.ts +++ b/src/core/tools/__tests__/searchReplaceTool.spec.ts @@ -147,7 +147,7 @@ describe("searchReplaceTool", () => { mockCline.ask = vi.fn().mockResolvedValue(undefined) mockCline.recordToolError = vi.fn() mockCline.recordToolUsage = vi.fn() - mockCline.processQueuedMessages = vi.fn() + mockCline.processQueuedMessages = vi.fn().mockResolvedValue(undefined) mockCline.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") mockAskApproval = vi.fn().mockResolvedValue(true) @@ -386,6 +386,27 @@ describe("searchReplaceTool", () => { expect(mockHandleError).toHaveBeenCalledWith("search and replace", expect.any(Error)) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) + + it("logs a queued-message drain failure after a successful replace without changing the tool result", async () => { + const drainError = new Error("queued submission failed") + mockCline.processQueuedMessages.mockRejectedValue(drainError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + const result = await executeSearchReplaceTool() + + // Flush the fire-and-forget drain promise so its rejection is logged. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(result).toBe("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[SearchReplaceTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) describe("file tracking", () => { diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..43f46cd003 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -187,6 +187,7 @@ describe("writeToFileTool", () => { mockCline.say = vi.fn().mockResolvedValue(undefined) mockCline.ask = vi.fn().mockResolvedValue(undefined) mockCline.recordToolError = vi.fn() + mockCline.processQueuedMessages = vi.fn().mockResolvedValue(undefined) mockCline.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") mockAskApproval = vi.fn().mockResolvedValue(true) @@ -392,6 +393,27 @@ describe("writeToFileTool", () => { // Should process normally without issues expect(mockCline.consecutiveMistakeCount).toBe(0) }) + + it("logs a queued-message drain failure after a successful write without changing the tool result", async () => { + const drainError = new Error("queued submission failed") + mockCline.processQueuedMessages.mockRejectedValue(drainError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + const result = await executeWriteFileTool({}, { fileExists: false }) + + // Flush the fire-and-forget drain promise so its rejection is logged. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(result).toBe("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[WriteToFileTool] Failed to process queued messages:", + drainError, + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) describe("partial block handling", () => { diff --git a/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts index 523f03e1c2..f84811cfcf 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts @@ -1,5 +1,6 @@ import type { Mock } from "vitest" import { describe, it, expect, vi, beforeEach } from "vitest" +import * as vscode from "vscode" // Mock dependencies first vi.mock("vscode", () => ({ @@ -43,6 +44,7 @@ import type { ClineProvider } from "../ClineProvider" import type { ClineMessage } from "@roo-code/types" import type { ApiMessage } from "../../task-persistence/apiMessages" import { MessageManager } from "../../message-manager" +import { Task } from "../../task/Task" describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => { let mockClineProvider: ClineProvider @@ -396,3 +398,79 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => { expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([]) }) }) + +describe("webviewMessageHandler - edit resubmission refusal", () => { + let mockClineProvider: ClineProvider + let mockCurrentTask: { + taskId: string + clineMessages: ClineMessage[] + apiConversationHistory: ApiMessage[] + overwriteClineMessages: ReturnType + overwriteApiConversationHistory: ReturnType + handleWebviewAskResponse: ReturnType + submitUserMessage: ReturnType + messageManager: MessageManager + } + + beforeEach(() => { + vi.clearAllMocks() + + const taskBase = { + taskId: "test-task-id", + clineMessages: [] as ClineMessage[], + apiConversationHistory: [] as ApiMessage[], + overwriteClineMessages: vi.fn(), + overwriteApiConversationHistory: vi.fn(), + handleWebviewAskResponse: vi.fn(), + // The task refuses the slot write: an approval ask is in flight. + submitUserMessage: vi.fn().mockResolvedValue(false), + } + mockCurrentTask = { + ...taskBase, + // MessageManager operates on the task's message arrays; the + // structural cast stays inside the helper per the test-utils rule. + messageManager: new MessageManager(taskBase as unknown as Task), + } + + mockClineProvider = { + getCurrentTask: vi.fn().mockReturnValue(mockCurrentTask), + postMessageToWebview: vi.fn(), + postStateToWebview: vi.fn(), + contextProxy: { + getValue: vi.fn(), + setValue: vi.fn(), + globalStorageUri: { fsPath: "/mock/storage" }, + }, + log: vi.fn(), + getState: vi.fn().mockResolvedValue({ + maxImageFileSize: 5, + maxTotalImageSize: 20, + }), + } as unknown as ClineProvider + }) + + it("surfaces a refused edit resubmission instead of dropping the edited message", async () => { + const userMessageTs = 1000 + // Mutate the shared array: the MessageManager was constructed against + // it, and property reassignment here would not be visible to the + // manager's task reference. + mockCurrentTask.clineMessages.push({ + ts: userMessageTs, + type: "say", + say: "user_feedback", + text: "Hello", + } as ClineMessage) + + await webviewMessageHandler(mockClineProvider, { + type: "editMessageConfirm", + messageTs: userMessageTs, + text: "Hello World", + restoreCheckpoint: false, + }) + + // The rewind already happened; the refusal must reach the user as a + // visible error instead of vanishing silently. + expect(mockCurrentTask.submitUserMessage).toHaveBeenCalledWith("Hello World", []) + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith(expect.stringContaining("error_editing_message")) + }) +}) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index dcd70f92f1..19e673e70e 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -2307,3 +2307,61 @@ describe("webviewMessageHandler - telemetrySetting", () => { expect(TelemetryService.instance.updateTelemetryState).not.toHaveBeenCalled() }) }) + +describe("webviewMessageHandler - chat message queue", () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(mockClineProvider.getState).mockResolvedValue({} as never) + }) + + it("routes editQueuedMessage through the same image validation as fresh queued messages", async () => { + const { MessageQueueService } = await import("../../message-queue/MessageQueueService") + const queue = new MessageQueueService() + const added = queue.addMessage("original")! + const updateSpy = vi.spyOn(queue, "updateMessage") + const editQueuedMessage = vi.fn((id: string, text: string, images?: string[]) => + queue.updateMessage(id, text, images), + ) + vi.mocked(mockClineProvider.getCurrentTask).mockReturnValue({ + cwd: "/mock/workspace", + rooIgnoreController: undefined, + messageQueueService: queue, + editQueuedMessage, + } as unknown as ReturnType) + + await webviewMessageHandler(mockClineProvider, { + type: "editQueuedMessage", + payload: { id: added.id, text: "edited", images: [] }, + }) + + // resolveImageMentions is mocked to tag validated payloads; the edited + // message must carry the validated images, proving the edit path no + // longer bypasses the size/mention validation applied to queueMessage. + expect(resolveImageMentions).toHaveBeenCalled() + expect(editQueuedMessage).toHaveBeenCalledWith(added.id, "edited", ["data:image/png;base64,from-mention"]) + expect(updateSpy).toHaveBeenCalledWith(added.id, "edited", ["data:image/png;base64,from-mention"]) + }) + + it("logs, shows an error, and still posts the response when condenseTaskContext rejects", async () => { + const condenseError = new Error("Task with id task-1 not found in stack") + const providerWithCondense = mockClineProvider as unknown as { + condenseTaskContext: ReturnType + } + providerWithCondense.condenseTaskContext = vi.fn().mockRejectedValue(condenseError) + + await expect( + webviewMessageHandler(mockClineProvider, { type: "condenseTaskContextRequest", text: "task-1" }), + ).resolves.toBeUndefined() + + expect(mockClineProvider.log).toHaveBeenCalledWith( + "[condenseTaskContextRequest] Failed: Task with id task-1 not found in stack", + ) + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("common:errors.condense_failed") + // The response must still reach the webview so ChatView clears + // isCondensing/sendingDisabled when the task vanished mid-request. + expect(mockClineProvider.postMessageToWebview).toHaveBeenCalledWith({ + type: "condenseTaskContextResponse", + text: "task-1", + }) + }) +}) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 4ba94d454c..3e187d1359 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -542,7 +542,16 @@ export const webviewMessageHandler = async ( // Update the UI to reflect the deletion await provider.postStateToWebview() - await currentCline.submitUserMessage(editedContent, images) + const submitted = await currentCline.submitUserMessage(editedContent, images) + if (!submitted) { + // The rewind above is destructive: if the task refused the slot + // write (an approval ask is in flight or the task is stopping), + // the edited message would vanish silently — surface it through + // the shared error dialog instead. + throw new Error( + "Edited message could not be delivered: an approval ask is in flight or the task is stopping.", + ) + } } catch (error) { console.error("Error in edit message:", error) vscode.window.showErrorMessage( @@ -908,7 +917,22 @@ export const webviewMessageHandler = async ( await provider.showTaskWithId(message.text!) break case "condenseTaskContextRequest": - await provider.condenseTaskContext(message.text!) + try { + await provider.condenseTaskContext(message.text!) + } catch (error) { + // condenseContext drains queued messages after summarizing, and a + // failed submission rejects: log the details for support and show + // the triggering user a visible error like sibling handlers. + provider.log( + `[condenseTaskContextRequest] Failed: ${error instanceof Error ? error.message : String(error)}`, + ) + await vscode.window.showErrorMessage(t("common:errors.condense_failed")) + // Post the response even on failure: a request racing task removal + // (or a failed condense) must still clear ChatView's + // isCondensing/sendingDisabled, which only the response message + // releases. The shape matches the success path. + await provider.postMessageToWebview({ type: "condenseTaskContextResponse", text: message.text! }) + } break case "deleteTaskWithId": await provider.deleteTaskWithId(message.text!) @@ -3812,7 +3836,11 @@ export const webviewMessageHandler = async ( case "editQueuedMessage": { if (message.payload) { const { id, text, images } = message.payload as EditQueuedMessagePayload - provider.getCurrentTask()?.messageQueueService.updateMessage(id, text, images) + // Edits can attach images too, so they go through the same + // size/mention validation as fresh queued messages, and through + // the task so a pending drain submission is updated in place. + const resolved = await resolveIncomingImages({ text, images }) + provider.getCurrentTask()?.editQueuedMessage(id, resolved.text, resolved.images) } break diff --git a/src/extension/__tests__/api-send-message.spec.ts b/src/extension/__tests__/api-send-message.spec.ts index 5158a218e7..d897924a95 100644 --- a/src/extension/__tests__/api-send-message.spec.ts +++ b/src/extension/__tests__/api-send-message.spec.ts @@ -3,12 +3,33 @@ import * as vscode from "vscode" import { API } from "../api" import { ClineProvider } from "../../core/webview/ClineProvider" -import { TaskCommandName } from "@roo-code/types" +import { IpcMessageType, TaskCommandName } from "@roo-code/types" import { makeClineProviderFactory } from "../../test-utils/provider" vi.mock("vscode") vi.mock("../../core/webview/ClineProvider") +// Capture the registered IPC TaskCommand handler so a test can drive the +// dispatch directly. IpcServer dispatches with emit (no promise handling), so +// a rejected command must be contained inside the listener itself. +const ipcState = vi.hoisted(() => ({ + handlers: new Map unknown>(), + instances: [] as unknown[], +})) + +vi.mock("@roo-code/ipc", () => ({ + IpcServer: class { + listen = vi.fn() + send = vi.fn() + on = vi.fn((type: string, handler: (...args: unknown[]) => unknown) => { + ipcState.handlers.set(type, handler) + }) + constructor(...args: unknown[]) { + ipcState.instances.push(args) + } + }, +})) + describe("API - SendMessage Command", () => { let api: API let mockOutputChannel: vscode.OutputChannel @@ -154,4 +175,50 @@ describe("API - SendMessage Command", () => { }) expect(mockPostMessageToWebview).toHaveBeenCalledTimes(1) }) + + it("rejects in headless mode when the task refuses the delivery", async () => { + // Arrange: headless flow with an in-flight approval ask — the task + // refuses the slot write, and the API caller must see the failure. + const submitUserMessage = vi.fn().mockResolvedValue(false) + const headlessProvider = { + context: {} as vscode.ExtensionContext, + postMessageToWebview: mockPostMessageToWebview, + on: vi.fn(), + getCurrentTaskStack: vi.fn().mockReturnValue([]), + getCurrentTask: vi.fn().mockReturnValue({ submitUserMessage }), + viewLaunched: false, + } as unknown as ClineProvider + const headlessApi = new API(mockOutputChannel, headlessProvider, makeClineProviderFactory(), undefined, true) + + // Act + Assert + await expect(headlessApi.sendMessage("Hello from headless")).rejects.toThrow( + "[API#sendMessage] message was not delivered", + ) + expect(submitUserMessage).toHaveBeenCalledWith("Hello from headless", undefined) + expect(mockPostMessageToWebview).not.toHaveBeenCalled() + }) + + it("contains a rejected headless SendMessage inside the IPC dispatch boundary", async () => { + // The task refuses the delivery; the fire-and-forget IPC listener must + // swallow and log the rejection instead of leaking an unhandled one. + const submitUserMessage = vi.fn().mockResolvedValue(false) + const headlessProvider = { + context: {} as vscode.ExtensionContext, + postMessageToWebview: mockPostMessageToWebview, + on: vi.fn(), + getCurrentTaskStack: vi.fn().mockReturnValue([]), + getCurrentTask: vi.fn().mockReturnValue({ submitUserMessage }), + viewLaunched: false, + } as unknown as ClineProvider + new API(mockOutputChannel, headlessProvider, makeClineProviderFactory(), "/tmp/test-roo-code.sock", true) + const handler = ipcState.handlers.get(IpcMessageType.TaskCommand) + expect(handler).toBeDefined() + + await expect( + handler!("client-1", { commandName: TaskCommandName.SendMessage, data: { text: "hi" } }), + ).resolves.toBeUndefined() + + // The rejection is contained: logged, not rethrown. + expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(expect.stringContaining("[API] SendMessage failed")) + }) }) diff --git a/src/extension/api.ts b/src/extension/api.ts index 9ae696adb1..57b7a97935 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -109,7 +109,18 @@ export class API extends EventEmitter implements RooCodeAPI { break case TaskCommandName.SendMessage: this.log(`[API] SendMessage -> ${command.data.text}`) - await this.sendMessage(command.data.text, command.data.images) + try { + await this.sendMessage(command.data.text, command.data.images) + } catch (error) { + // Headless delivery can reject (no task, or the task + // refused the slot write while an approval ask is pending). + // The IPC dispatch is fire-and-forget, so an uncaught + // rejection here would be unhandled; log like the + // ResumeTask boundary and swallow — there is no typed + // SendMessage failure response. + const errorMessage = error instanceof Error ? error.message : String(error) + this.log(`[API] SendMessage failed: ${errorMessage}`) + } break case TaskCommandName.GetCommands: try { @@ -331,7 +342,15 @@ export class API extends EventEmitter implements RooCodeAPI { return } - await currentTask.submitUserMessage(text ?? "", images) + const submitted = await currentTask.submitUserMessage(text ?? "", images) + if (!submitted) { + // The task refused the write (it is stopping or an approval ask + // is in flight): reject so headless callers see the message was + // not delivered instead of it vanishing silently. + throw new Error( + "[API#sendMessage] message was not delivered to the task (task stopping or an approval ask is pending)", + ) + } return } diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index 9e722702fd..97b390f6f4 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -15,7 +15,10 @@ vi.mock("@src/utils/vscode", () => ({ vscode: { postMessage: vi.fn() } })) // save round trip can be observed without rendering the whole provider tree. vi.mock("../ApiOptions", () => ({ __esModule: true, - default: ({ apiConfiguration, setApiConfigurationField }: Pick) => ( + default: ({ + apiConfiguration, + setApiConfigurationField, + }: Pick) => (
{String(apiConfiguration?.openAiStrictToolSchemas)}