diff --git a/packages/types/src/task.ts b/packages/types/src/task.ts index d243bffbb1..91f71a592e 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/message-queue/MessageQueueService.ts b/src/core/message-queue/MessageQueueService.ts index 340fad6087..6790edaaca 100644 --- a/src/core/message-queue/MessageQueueService.ts +++ b/src/core/message-queue/MessageQueueService.ts @@ -73,18 +73,18 @@ export class MessageQueueService extends EventEmitter { return true } - public dequeueMessage(): QueuedMessage | undefined { - const index = this._messages.findIndex((message) => !this.claimedMessageIds.has(message.id)) - if (index === -1) { - return undefined - } - const [message] = this._messages.splice(index, 1) - this.emit("stateChanged", this._messages) - return message - } - - public claimNextMessage(): QueuedMessage | undefined { - const message = this._messages.find((candidate) => !this.claimedMessageIds.has(candidate.id)) + /** + * Claims the first message that is neither already claimed nor listed in + * `excludeIds`. + * + * `excludeIds` lets a consumer that must not take certain messages skip them + * without removing them: an execution-gating ask passes the IDs a drain has + * already submitted so stale feedback can never stand in for an approval. + */ + public claimNextMessage(excludeIds?: ReadonlySet): QueuedMessage | undefined { + const message = this._messages.find( + (candidate) => !this.claimedMessageIds.has(candidate.id) && !excludeIds?.has(candidate.id), + ) if (message) { this.claimedMessageIds.add(message.id) } @@ -115,9 +115,13 @@ export class MessageQueueService extends EventEmitter { * report a non-empty queue. Consumers that are about to take a message need * this instead: it answers "is there anything I may take", which is false * when every remaining message is already spoken for. + * + * `excludeIds` mirrors `claimNextMessage`: a message the caller may not take + * is not available, so an execution-gating ask does not report availability + * for a message only a conversational ask may consume. */ - public hasUnclaimed(): boolean { - return this._messages.some((message) => !this.claimedMessageIds.has(message.id)) + public hasUnclaimed(excludeIds?: ReadonlySet): boolean { + return this._messages.some((message) => !this.claimedMessageIds.has(message.id) && !excludeIds?.has(message.id)) } public dispose(): void { diff --git a/src/core/message-queue/__tests__/MessageQueueService.spec.ts b/src/core/message-queue/__tests__/MessageQueueService.spec.ts index 567dca314b..68ef21a6ff 100644 --- a/src/core/message-queue/__tests__/MessageQueueService.spec.ts +++ b/src/core/message-queue/__tests__/MessageQueueService.spec.ts @@ -8,7 +8,7 @@ describe("MessageQueueService claims", () => { expect(queue.claimNextMessage()).toEqual(first) expect(queue.claimNextMessage()).toEqual(second) - expect(queue.dequeueMessage()).toBeUndefined() + expect(queue.claimNextMessage()).toBeUndefined() expect(queue.messages).toEqual([first, second]) }) @@ -67,4 +67,32 @@ describe("MessageQueueService claims", () => { queue.releaseMessage(message.id) expect(queue.hasUnclaimed()).toBe(true) }) + + it("skips excluded messages without claiming or removing them", () => { + const queue = new MessageQueueService() + const retained = queue.addMessage("retained by a drain")! + const fresh = queue.addMessage("fresh")! + const excluded = new Set([retained.id]) + + // Availability must agree with the claim: the excluded message is not + // available to this consumer even though the queue holds it. + expect(queue.hasUnclaimed(excluded)).toBe(true) + expect(queue.claimNextMessage(excluded)).toEqual(fresh) + + // The excluded message stays queued and unclaimed, so a consumer allowed + // to take it (a conversational ask) still can. + expect(queue.messages).toEqual([retained, fresh]) + expect(queue.hasUnclaimed(excluded)).toBe(false) + expect(queue.claimNextMessage()).toEqual(retained) + }) + + it("reports no availability when every message is excluded", () => { + const queue = new MessageQueueService() + const only = queue.addMessage("only")! + const excluded = new Set([only.id]) + + expect(queue.hasUnclaimed(excluded)).toBe(false) + expect(queue.claimNextMessage(excluded)).toBeUndefined() + expect(queue.messages).toEqual([only]) + }) }) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index e573a7f107..6d6e3a881f 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -509,6 +509,21 @@ export class Task extends EventEmitter implements TaskLike { // Message Queue Service public readonly messageQueueService: MessageQueueService private messageQueueStateChangedHandler: (() => void) | undefined + // Serializes drains so two of them cannot submit into the single ask-response + // slot and drop the earlier response. + private queuedMessageDrainChain: Promise = Promise.resolve() + // IDs a drain submitted into the ask-response slot that no ask has consumed + // yet. They stay queued on purpose: a later drain must not re-post them, and + // an execution-gating ask must not let between-turn feedback stand in for an + // approval. Cleared when a consume settles, or when the queue no longer holds + // the id (see `queuedClaimExclusionsForAsk`). + private drainRetainedQueuedMessageIds = new Set() + // `ask()` calls in flight. A drain must not submit while one is pending: the + // submission would answer that ask with text written for another turn. + private pendingAskCount = 0 + // Message id of the queued feedback row already in `clineMessages`, so a + // reclaim re-saves that row instead of appending a duplicate. + private queuedFeedbackRowWritten = new Map() // Streaming isWaitingForFirstChunk = false @@ -1104,14 +1119,40 @@ export class Task extends EventEmitter implements TaskLike { text?: string, images?: string[], ): Promise { - await this.say("user_feedback", text ?? "", images) + // Append the row once, before the retry loop, so a retry only re-attempts the + // durable save. `say` can throw after `addToClineMessages` pushed the row + // (its `Message` emit runs post-push) and a rewind can remove it later, so + // the record is trusted only while that row is still present. + const writtenRowId = this.queuedFeedbackRowWritten.get(messageId) + if (writtenRowId === undefined || !this.clineMessages.some((message) => message.messageId === writtenRowId)) { + this.queuedFeedbackRowWritten.delete(messageId) + const appended = this.clineMessages.length + try { + await this.say("user_feedback", text ?? "", images) + } catch (error) { + // Released like the exhausted-retry path: the row's durability is + // unknown, so the message stays reclaimable. + this.releaseUnconsumedQueuedMessage(messageId) + throw error + } finally { + // Recorded whenever the row landed, including a throw after the push. + const row = this.clineMessages[appended] + if (row?.messageId) { + this.queuedFeedbackRowWritten.set(messageId, row.messageId) + } + } + } for (let attempt = 0; attempt <= QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS.length; attempt++) { if (this.abort) { - this.messageQueueService.releaseMessage(messageId) + this.releaseUnconsumedQueuedMessage(messageId) return false } if (await this.saveClineMessages()) { - return this.messageQueueService.removeMessage(messageId) + // Durable, so the consume happened: report success even when the queue + // entry is already gone (deleted mid-ack), or a saved row would be + // reported as a failure. + this.consumeQueuedMessage(messageId) + return true } if (attempt < QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS.length) { await delay(QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS[attempt]) @@ -1120,7 +1161,7 @@ export class Task extends EventEmitter implements TaskLike { 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) + this.releaseUnconsumedQueuedMessage(messageId) return false } @@ -1234,12 +1275,31 @@ 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) { + // The consume is not settled yet: the caller's persistence write can + // still fail, and the retention record must survive that unwind. return message.id } - this.messageQueueService.removeMessage(message.id) + this.consumeQueuedMessage(message.id) return undefined } + /** Settle a consumed queued message on this one edge, so no record outlives it. */ + private consumeQueuedMessage(messageId: string): void { + this.messageQueueService.removeMessage(messageId) + this.drainRetainedQueuedMessageIds.delete(messageId) + this.queuedFeedbackRowWritten.delete(messageId) + } + + /** + * Undo a claim whose consume did not happen: release it for a later consumer + * and re-arm the retention record, so it cannot look like fresh input for the + * next command ask to approve. + */ + private releaseUnconsumedQueuedMessage(messageId: string): void { + this.drainRetainedQueuedMessageIds.add(messageId) + this.messageQueueService.releaseMessage(messageId) + } + /** * Re-read the auto-approval policy immediately before a queued message would * stand in for a command ask's approval, and consult the full command policy @@ -1363,9 +1423,33 @@ export class Task extends EventEmitter implements TaskLike { * `isMessageQueued`/`isStatusMutable` keep `isEmpty()` semantics on purpose: * flipping those would re-enable interactive prompt timers whenever a claim * is outstanding. + * + * `excludeIds` carries the messages this ask may not consume at all. + */ + private mayDrainQueuedMessageForAsk(excludeIds?: ReadonlySet): boolean { + return !this.blanketDeniedCommandThisTurn && this.messageQueueService.hasUnclaimed(excludeIds) + } + + /** + * Queued messages this ask must not claim: `queuedResponseForAsk` answers a + * command, tool, or use_mcp_server ask with `yesButtonClicked`, so claiming a + * drain-retained message would turn between-turn feedback into an approval + * with `checkAutoApproval` skipped. */ - private mayDrainQueuedMessageForAsk(): boolean { - return !this.blanketDeniedCommandThisTurn && this.messageQueueService.hasUnclaimed() + private queuedClaimExclusionsForAsk(type: ClineAsk, text?: string): ReadonlySet | undefined { + // Drop IDs the queue no longer holds (deleted from the webview or the API) + // so neither record can outlive its message. + const queuedIds = new Set(this.messageQueueService.messages.map((message) => message.id)) + for (const id of this.drainRetainedQueuedMessageIds) { + if (queuedIds.has(id)) continue + this.drainRetainedQueuedMessageIds.delete(id) + this.queuedFeedbackRowWritten.delete(id) + } + + return this.drainRetainedQueuedMessageIds.size > 0 && + queuedResponseForAsk(type, text)?.response === "yesButtonClicked" + ? this.drainRetainedQueuedMessageIds + : undefined } /** @@ -1844,10 +1928,34 @@ export class Task extends EventEmitter implements TaskLike { 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. + * rather than a user click: not a user rejection, and scoped to its own + * tool call. */ autoDenyDetail?: AutoDenyDetail + }> { + // Bracket the whole ask, not just the wait: a submission during any await + // below would answer this ask with text written for another turn. + this.pendingAskCount++ + try { + return await this.askWithPendingCount(type, text, partial, progressStatus, isProtected, autoApprovalContext) + } finally { + this.pendingAskCount-- + } + } + + private async askWithPendingCount( + type: ClineAsk, + text?: string, + partial?: boolean, + progressStatus?: ToolProgressStatus, + isProtected?: boolean, + autoApprovalContext?: AutoApprovalContext, + ): Promise<{ + response: ClineAskResponse + text?: string + images?: string[] + queuedMessageId?: string + 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, @@ -1886,6 +1994,9 @@ 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") + // Retained drain submissions are off-limits to any ask that would answer + // with `yesButtonClicked`; see `queuedClaimExclusionsForAsk`. + const queuedClaimExclusions = this.queuedClaimExclusionsForAsk(type, text) // A queued message must not short-circuit protected asks (e.g. // DCG-blocked commands): claiming one here skips checkAutoApproval, and // the drain below would auto-approve what protection intentionally @@ -1895,9 +2006,9 @@ export class Task extends EventEmitter implements TaskLike { type === "command_output" || isProtected || !queueMayAnswerThisAsk || - !this.mayDrainQueuedMessageForAsk() + !this.mayDrainQueuedMessageForAsk(queuedClaimExclusions) ? undefined - : this.messageQueueService.claimNextMessage() + : this.messageQueueService.claimNextMessage(queuedClaimExclusions) const queuedAskResolution = queuedMessage ? queuedResponseForAsk(type, text) : undefined // `this.cwd`, not `provider.cwd`: // The path inside `text` was made relative to this task's workspace, @@ -2302,9 +2413,9 @@ export class Task extends EventEmitter implements TaskLike { shouldDrainQueuedMessageForAsk && !isProtected && !queuedCommandPolicyCheck && - this.mayDrainQueuedMessageForAsk() + this.mayDrainQueuedMessageForAsk(queuedClaimExclusions) ) { - const message = this.messageQueueService.claimNextMessage() + const message = this.messageQueueService.claimNextMessage(queuedClaimExclusions) const resolution = message ? queuedResponseForAsk(type, text) : undefined if (message && resolution) { if (type === "command") { @@ -2346,7 +2457,7 @@ export class Task extends EventEmitter implements TaskLike { /* v8 ignore next 3 -- abort-while-waiting path; covered by e2e standalone-resume test */ if (this.abort) { if (queuedMessageId) { - this.messageQueueService.releaseMessage(queuedMessageId) + this.releaseUnconsumedQueuedMessage(queuedMessageId) } throw new Error(`[ZooCode#ask] task ${this.taskId}.${this.instanceId} aborted`) } @@ -2356,7 +2467,7 @@ export class Task extends EventEmitter implements TaskLike { // command_output. It's important that when we know an ask could // fail, it is handled gracefully. if (queuedMessageId) { - this.messageQueueService.releaseMessage(queuedMessageId) + this.releaseUnconsumedQueuedMessage(queuedMessageId) } throw new AskIgnoredError("superseded") } @@ -2400,6 +2511,9 @@ export class Task extends EventEmitter implements TaskLike { this.askResponse = askResponse this.askResponseText = text this.askResponseImages = images + // No retention bookkeeping here: a direct response overwrites the slot a + // drain submitted into, so that submission was NOT consumed: the message + // stays queued and excluded from approval asks. // 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. @@ -2535,18 +2649,24 @@ export class Task extends EventEmitter implements TaskLike { } } + /** + * Submit a user message through the ask-response channel. + * + * @returns false when there was nothing to submit, the task is stopping, or + * the provider reference was lost. Draining callers must check it. + */ public async submitUserMessage( text: string, images?: string[], mode?: string, providerProfile?: string, - ): Promise { + ): Promise { try { text = (text ?? "").trim() images = images ?? [] if (text.length === 0 && images.length === 0) { - return + return false } const provider = this.providerRef.deref() @@ -2569,17 +2689,28 @@ export class Task extends EventEmitter implements TaskLike { } } + // Cancellation guard adjacent to the slot write: abort and dispose + // both set this flag, and the side effects below must never run on + // a cancelled task. Single-threaded, so nothing interleaves. + if (this.abort || this.abandoned) { + console.error("[Task#submitUserMessage] Task aborted, dropping user message submission") + 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) + 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 } } @@ -2745,8 +2876,9 @@ export class Task extends EventEmitter implements TaskLike { contextCondense, ) - // Process any queued messages after condensing completes - this.processQueuedMessages() + // Awaited so a failed submission rejects this call instead of vanishing; + // the caller (ClineProvider.condenseTaskContext) surfaces it. + await this.processQueuedMessages() } async say( @@ -6431,26 +6563,78 @@ 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. * - * @param context - Context string for logging (e.g., the calling tool name) + * The message is claimed, not dequeued, and the claim is released after the + * submission handoff so the message STAYS queued: removal is tied to + * ask-consumption, not to submit-return, so a message submitted between turns + * survives a failed turn or restart. A throw releases the claim and + * propagates; a handoff that delivered nothing releases it and resolves false. + * + * Retention is restricted to conversational consumption: a retained ID is + * recorded in `drainRetainedQueuedMessageIds`, and every ask whose + * `queuedResponseForAsk` mapping answers `yesButtonClicked` excludes those + * IDs, so the retained text cannot silently approve a later execution ask. + * + * A drain never submits while an `ask()` is pending, and drains are + * serialized per task, so two of them cannot interleave into the slot. + * + * @returns true only when this call submitted a message. A retained queue + * head reports false: it was already submitted, and the single ask-response + * slot cannot take a second message without losing the first. */ - 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 post into a dying task. + if (this.abort || this.abandoned) { + return false + } + // An ask is waiting for its response. Submitting now would resolve that + // prompt with text written for another turn, bypassing the ask's claim gate, + // blanket-deny latch, and protected-ask exemption. + if (this.pendingAskCount > 0) { + 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 report false with the message still queued. + if (this.drainRetainedQueuedMessageIds.has(queued.id)) { + this.messageQueueService.releaseMessage(queued.id) + return false + } 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) + if (!submitted) { + // Cancelled mid-handoff: the guard in submitUserMessage dropped the + // write, so release quietly instead of failing a stopping task. + 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 } + // Record the ID before releasing the claim: a later drain must tell + // "already submitted, still pending" from "not submitted yet". + this.drainRetainedQueuedMessageIds.add(queued.id) + this.messageQueueService.releaseMessage(queued.id) + return true } } diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index ad07230204..f14cef5f4e 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -53,12 +53,16 @@ type TaskTestAccess = { addToClineMessages: (message: import("@roo-code/types").ClineMessage) => Promise updateClineMessage: (message: import("@roo-code/types").ClineMessage) => Promise saveClineMessages: () => Promise + lastMessageTs?: number + pendingAskCount: number + drainRetainedQueuedMessageIds: Set safeEnsureModelFetched: () => Promise backoffAndAnnounce: (retryAttempt: number, error: unknown) => Promise requestPolicySnapshot?: RequestPolicySnapshot getFilesReadByRooSafely: (context: string) => Promise addToApiConversationHistory: (message: unknown, reasoning?: string) => Promise resetAssistantMessagePersistence: () => void + checkpointSave: (force?: boolean, suppressMessage?: boolean) => Promise buildCleanConversationHistory: ( messages: ApiMessage[], requestModelInfo: ModelInfo, @@ -2475,7 +2479,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 @@ -2571,7 +2575,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() @@ -4201,6 +4205,98 @@ 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"]) + }) + + 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"]) + }) }) }) @@ -5914,7 +6010,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 = [ @@ -5962,7 +6058,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 @@ -5993,7 +6089,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() }, @@ -6049,7 +6145,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() }, @@ -6102,7 +6198,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() }, @@ -6149,7 +6245,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() }, @@ -6630,7 +6726,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() @@ -7698,6 +7794,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: { @@ -7760,20 +7861,19 @@ 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"]) + // 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) }) @@ -7797,30 +7897,343 @@ 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(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) + // 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, + }) + + // This suite mocks `p-wait-for` to resolve immediately, which is fine for + // asks that a queued message or an explicit response answers. A test that + // must observe a genuinely pending ask needs the real wait, so it runs + // inside this bracket and the mock is restored afterwards. + const withRealPWaitFor = async (run: () => Promise): Promise => { + const { default: realPWaitFor } = await vi.importActual("p-wait-for") + vi.mocked(pWaitFor).mockImplementation((condition) => + realPWaitFor(condition, { interval: 1, timeout: 2_000 }), + ) + try { + return await run() + } finally { + vi.mocked(pWaitFor).mockImplementation(async () => {}) + } + } + + // The ask row is appended after the ask clears its response slot and before + // it starts waiting, so its presence is the deterministic "this prompt is + // now pending" signal. Waiting on `pendingAskCount` alone would race the + // clear and let the response be discarded. + const waitForPendingAsk = (task: Task, ask: string) => + vi.waitFor(() => + expect(task.clineMessages.some((message) => message.type === "ask" && message.ask === ask)).toBe(true), + ) + + it("submits the next queued message and retains it until a conversational 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"]) + + // A retained drain submission must NOT answer an execution-gating ask. + // `queuedResponseForAsk` maps a tool ask to `yesButtonClicked`, so a + // claim here would run the tool with no prompt and with the stale + // queued text as "approval feedback". The ask excludes the retained + // ID and waits for a real response instead. + const { approval, retainedDuringAsk } = await withRealPWaitFor(async () => { + const approvalPromise = task.ask("tool", JSON.stringify({ tool: "readFile" }), false) + await waitForPendingAsk(task, "tool") + const retained = task.messageQueueService.messages.map((message) => message.text) + task.approveAsk() + return { approval: await approvalPromise, retainedDuringAsk: retained } + }) + + expect(retainedDuringAsk).toEqual(["queued text"]) + expect(approval).toMatchObject({ response: "yesButtonClicked", text: undefined }) + expect(approval.queuedMessageId).toBeUndefined() + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["queued text"]) + + // The retained message is still deliverable: the next conversational + // ask is the only consumer allowed to carry its text into the model. + const followup = await task.ask("followup", "Anything else?", false) + expect(followup).toMatchObject({ response: "messageResponse", text: "queued text", images: ["img1.png"] }) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("never lets a retained drain submission approve a command ask", async () => { + const task = createQueueTask() + vi.spyOn(getTaskTestAccess(task), "checkpointSave").mockResolvedValue(undefined) + task.messageQueueService.addMessage("ship it") + + await expect(task.processQueuedMessages()).resolves.toBe(true) + + // Stale between-turn feedback must not stand in for an approval: the + // command ask gets a real prompt even though the queue is non-empty. + const { denied, retainedDuringAsk } = await withRealPWaitFor(async () => { + const commandPromise = task.ask("command", "rm -rf build", false) + await waitForPendingAsk(task, "command") + const retained = task.messageQueueService.messages.map((message) => message.text) + task.denyAsk() + return { denied: await commandPromise, retainedDuringAsk: retained } + }) + + expect(retainedDuringAsk).toEqual(["ship it"]) + expect(denied).toMatchObject({ response: "noButtonClicked" }) + expect(denied.queuedMessageId).toBeUndefined() + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["ship it"]) + }) + + 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 when the submission handoff reports nothing delivered", async () => { + const task = createQueueTask() + vi.spyOn(task, "submitUserMessage").mockResolvedValue(false) + task.messageQueueService.addMessage("retry me") + + await expect(task.processQueuedMessages()).resolves.toBe(false) + + // 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) + // The second drain saw "first" still pending consumption, so it + // submitted nothing and reports that honestly. + await expect(secondDrain).resolves.toBe(false) + + // 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) + // "retry me" is now retained, so this drain submits nothing. + await expect(task.processQueuedMessages()).resolves.toBe(false) + + // The retried submission stays queued until an ask consumes it; the + // head blocks the drain, so "still deliverable" waits for its turn. + 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) + }) + + it.each([ + { state: "aborted", apply: (task: Task) => (task.abort = true) }, + { state: "abandoned", apply: (task: Task) => (task.abandoned = true) }, + ])("does not submit a queued message when the task is $state", async ({ apply }) => { + const task = createQueueTask() + const submitSpy = vi.spyOn(task, "submitUserMessage").mockResolvedValue(true) + task.messageQueueService.addMessage("must wait for a live task") + + apply(task) + + await expect(task.processQueuedMessages()).resolves.toBe(false) + + // The guard must reject before the claim, so no submission happens and + // the message is still there for a task that is not stopping. + expect(submitSpy).not.toHaveBeenCalled() + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual([ + "must wait for a live task", + ]) + expect(task.messageQueueService.hasUnclaimed()).toBe(true) + }) + + it("defers the drain while an ask is pending and submits after it settles", async () => { + const task = createQueueTask() + vi.spyOn(getTaskTestAccess(task), "checkpointSave").mockResolvedValue(undefined) + + const { duringAsk, afterAsk } = await withRealPWaitFor(async () => { + // A protected ask blocks on explicit user approval and refuses to + // consume a queued message at either claim site, so it is the ask + // that stays pending while the queue fills. That is the F1711 + // window: a background completion can land while the model's next + // approval prompt waits. + const askPromise = task.ask("tool", JSON.stringify({ tool: "readFile" }), false, undefined, true) + await waitForPendingAsk(task, "tool") + + // The message arrives while the prompt is pending. + task.messageQueueService.addMessage("written during the prompt") + + // A background completion landing here must not answer the pending + // ask with text written for another turn. + const deferred = await task.processQueuedMessages() + const queued = task.messageQueueService.messages.map((message) => message.text) + + task.approveAsk() + await askPromise + return { duringAsk: { deferred, queued }, afterAsk: await task.processQueuedMessages() } + }) + + expect(duringAsk.deferred).toBe(false) + expect(duringAsk.queued).toEqual(["written during the prompt"]) + // The deferred message is not lost: the next drain submits it. + expect(afterAsk).toBe(true) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual([ + "written during the prompt", + ]) + }) + + it("consumes a retained drain submission exactly once", async () => { + const task = createQueueTask() + vi.spyOn(getTaskTestAccess(task), "checkpointSave").mockResolvedValue(undefined) + task.messageQueueService.addMessage("exactly once") + + await expect(task.processQueuedMessages()).resolves.toBe(true) + // A second drain must not re-post the same message into the slot, so + // it submits nothing and reports that. + await expect(task.processQueuedMessages()).resolves.toBe(false) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["exactly once"]) + + const consumed = await task.ask("followup", "Anything else?", false) + expect(consumed).toMatchObject({ response: "messageResponse", text: "exactly once" }) + expect(task.messageQueueService.isEmpty()).toBe(true) + // Consumption clears the retention record, so the exclusion set no + // longer names a message the queue does not hold. + expect(getTaskTestAccess(task).drainRetainedQueuedMessageIds.size).toBe(0) + + // A consumed message is not resubmitted by a later drain. + await expect(task.processQueuedMessages()).resolves.toBe(false) + }) + + it("keeps a direct user response when a drain re-posts a retained message", async () => { + const task = createQueueTask() + vi.spyOn(getTaskTestAccess(task), "checkpointSave").mockResolvedValue(undefined) + task.messageQueueService.addMessage("retained feedback") + + await expect(task.processQueuedMessages()).resolves.toBe(true) + + // The user answers the pending prompt directly, overwriting the slot + // the drain submitted into. The message was NOT consumed, so it stays + // retained and a later drain must not re-post over the direct answer. + task.handleWebviewAskResponse("noButtonClicked", "typed by the user") + await expect(task.processQueuedMessages()).resolves.toBe(false) + + expect(task["askResponse"]).toBe("noButtonClicked") + expect(task["askResponseText"]).toBe("typed by the user") + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["retained feedback"]) + expect(getTaskTestAccess(task).drainRetainedQueuedMessageIds.size).toBe(1) + }) }) }) diff --git a/src/core/task/__tests__/ask-abort-race.spec.ts b/src/core/task/__tests__/ask-abort-race.spec.ts index aa5dbe4a82..e36c07213f 100644 --- a/src/core/task/__tests__/ask-abort-race.spec.ts +++ b/src/core/task/__tests__/ask-abort-race.spec.ts @@ -14,6 +14,8 @@ function buildTask(getState: () => Promise>) { task["abort"] = false task["clineMessages"] = [] task["lastMessageTs"] = undefined + task["pendingAskCount"] = 0 + task["drainRetainedQueuedMessageIds"] = new Set() task["addToClineMessages"] = vi.fn(async () => {}) task["saveClineMessages"] = vi.fn(async () => true) task["updateClineMessage"] = vi.fn(async () => {}) diff --git a/src/core/task/__tests__/ask-allowlist-cwd.spec.ts b/src/core/task/__tests__/ask-allowlist-cwd.spec.ts index 3b3682f969..d52b042b8c 100644 --- a/src/core/task/__tests__/ask-allowlist-cwd.spec.ts +++ b/src/core/task/__tests__/ask-allowlist-cwd.spec.ts @@ -29,6 +29,8 @@ function buildTask(provider: ProviderStub, taskCwd: string) { task["askResponseText"] = undefined task["askResponseImages"] = undefined task["lastMessageTs"] = undefined + task["pendingAskCount"] = 0 + task["drainRetainedQueuedMessageIds"] = new Set() task["addToClineMessages"] = vi.fn(async () => {}) task["saveClineMessages"] = vi.fn(async () => true) task["updateClineMessage"] = vi.fn(async () => {}) diff --git a/src/core/task/__tests__/ask-auto-deny.spec.ts b/src/core/task/__tests__/ask-auto-deny.spec.ts index 4930668c46..5873f9dce1 100644 --- a/src/core/task/__tests__/ask-auto-deny.spec.ts +++ b/src/core/task/__tests__/ask-auto-deny.spec.ts @@ -37,6 +37,9 @@ function buildTask(provider: ProviderStub, taskCwd: string) { task["askResponseText"] = undefined task["askResponseImages"] = undefined task["lastMessageTs"] = undefined + task["pendingAskCount"] = 0 + task["drainRetainedQueuedMessageIds"] = new Set() + task["queuedFeedbackRowWritten"] = new Map() task["addToClineMessages"] = vi.fn(async () => {}) task["saveClineMessages"] = vi.fn(async () => true) task["updateClineMessage"] = vi.fn(async () => {}) diff --git a/src/core/task/__tests__/ask-clear-approval-buttons.spec.ts b/src/core/task/__tests__/ask-clear-approval-buttons.spec.ts index 240dc9f22a..1a5b9d941f 100644 --- a/src/core/task/__tests__/ask-clear-approval-buttons.spec.ts +++ b/src/core/task/__tests__/ask-clear-approval-buttons.spec.ts @@ -18,6 +18,8 @@ function buildTask(provider: ProviderStub | undefined) { ;(task as any).askResponseText = undefined ;(task as any).askResponseImages = undefined ;(task as any).lastMessageTs = undefined + task["pendingAskCount"] = 0 + task["drainRetainedQueuedMessageIds"] = new Set() ;(task as any).addToClineMessages = vi.fn(async () => {}) ;(task as any).saveClineMessages = vi.fn(async () => {}) ;(task as any).updateClineMessage = vi.fn(async () => {}) 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 bb74531d45..b5c5c30f71 100644 --- a/src/core/task/__tests__/ask-queued-message-drain.spec.ts +++ b/src/core/task/__tests__/ask-queued-message-drain.spec.ts @@ -3,7 +3,7 @@ import { Task } from "../Task" type QueueTaskTestAccess = { say: Task["say"] saveClineMessages: () => Promise - addToClineMessages: () => Promise + addToClineMessages: (message: import("@roo-code/types").ClineMessage) => Promise lastMessageTs?: number abort: boolean } @@ -22,9 +22,17 @@ describe("Task.ask queued message drain", () => { ;(task as any).askResponseText = undefined ;(task as any).askResponseImages = undefined ;(task as any).lastMessageTs = undefined + task["pendingAskCount"] = 0 + task["drainRetainedQueuedMessageIds"] = new Set() + task["queuedFeedbackRowWritten"] = new Map() return import("../../message-queue/MessageQueueService").then(({ MessageQueueService }) => { ;(task as any).messageQueueService = new MessageQueueService() - ;(task as any).addToClineMessages = vi.fn(async () => {}) + // Mirrors the real `addToClineMessages`: it stamps a messageId and pushes + // synchronously before its own awaits, and the tests here depend on both. + ;(task as any).addToClineMessages = vi.fn(async (message: import("@roo-code/types").ClineMessage) => { + message.messageId ??= crypto.randomUUID() + task["clineMessages"].push(message) + }) ;(task as any).saveClineMessages = vi.fn(async () => {}) ;(task as any).updateClineMessage = vi.fn(async () => {}) ;(task as any).cancelAutoApprovalTimeout = vi.fn(() => {}) @@ -380,6 +388,60 @@ describe("Task.ask queued message drain", () => { } }) + it("keeps failed durable feedback out of the next command ask", async () => { + const task = await createTask() + // A drain submitted this message, so it is retained while it waits for a + // conversational ask to consume it. + task["drainRetainedQueuedMessageIds"].add(task.messageQueueService.addMessage("ship it")!.id) + const result = await task.ask("tool", JSON.stringify({ tool: "finishTask" }), false) + 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() + } + + // The consume was undone, so the message must stay retained: a command + // ask that claimed it would skip checkAutoApproval and execute on text + // never answered to that command. + expect(task["drainRetainedQueuedMessageIds"].has(result.queuedMessageId!)).toBe(true) + expect(task.messageQueueService.claimNextMessage(task["drainRetainedQueuedMessageIds"])).toBeUndefined() + + // A command ask gets a real prompt and leaves the message queued. + const commandAsk = task.ask("command", "rm -rf build", false) + setTimeout(() => task.denyAsk(), 0) + const denied = await commandAsk + expect(denied).toMatchObject({ response: "noButtonClicked" }) + expect(denied.queuedMessageId).toBeUndefined() + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["ship it"]) + }) + + it("reports success when the durable save lands but the queue entry is gone", async () => { + const task = await createTask() + task.messageQueueService.addMessage("Deleted mid-ack") + const result = await task.ask("completion_result", "Done", false) + const access = getQueueTaskTestAccess(task) + access.say = vi.fn().mockResolvedValue(undefined) + access.saveClineMessages = vi.fn().mockResolvedValue(true) + // The user removed the entry from the webview while the ack was in flight. + task.messageQueueService.removeMessage(result.queuedMessageId!) + + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).resolves.toBe(true) + expect(task["drainRetainedQueuedMessageIds"].size).toBe(0) + }) + it("releases durable queued feedback when the task aborts during retry backoff", async () => { vi.useFakeTimers() try { @@ -409,4 +471,165 @@ describe("Task.ask queued message drain", () => { vi.useRealTimers() } }) + + it("does not append a second user_feedback row when a reclaimed message is persisted again", async () => { + const task = await createTask() + task.messageQueueService.addMessage("Persist me twice") + const result = await task.ask("completion_result", "Done", false) + const access = getQueueTaskTestAccess(task) + const rowsBefore = task["clineMessages"].length + + // First attempt appends the row, then fails every save and releases the + // claim, so the message is reclaimed as fresh input for a later turn. + access.saveClineMessages = vi.fn().mockResolvedValue(false) + vi.useFakeTimers() + try { + const first = task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images) + await vi.runAllTimersAsync() + await expect(first).resolves.toBe(false) + } finally { + vi.useRealTimers() + } + expect(task["clineMessages"]).toHaveLength(rowsBefore + 1) + + // The retry succeeds, but the row it would append is already in history. + access.saveClineMessages = vi.fn().mockResolvedValue(true) + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).resolves.toBe(true) + expect(task["clineMessages"]).toHaveLength(rowsBefore + 1) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("keeps a message reclaimable and appends once when say throws before the row lands", async () => { + const task = await createTask() + task.messageQueueService.addMessage("Row never landed") + const result = await task.ask("completion_result", "Done", false) + const access = getQueueTaskTestAccess(task) + access.saveClineMessages = vi.fn().mockResolvedValue(true) + // The ask above left its own row; only rows added from here count. + const rowsBefore = task["clineMessages"].length + + // The real `say` guard rejects before `addToClineMessages` runs, so no row + // exists and the message must stay reclaimable. + access.abort = true + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).rejects.toThrow("aborted") + expect(task["clineMessages"]).toHaveLength(rowsBefore) + expect(task.messageQueueService.messages.map((message) => message.text)).toEqual(["Row never landed"]) + // Released, not left claimed: a later consumer can still claim it. + expect(task.messageQueueService.claimNextMessage()?.text).toBe("Row never landed") + task.messageQueueService.releaseMessage(result.queuedMessageId!) + + // A later reclaim appends exactly one row, because the failed attempt left + // no record behind. + access.abort = false + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).resolves.toBe(true) + expect(task["clineMessages"]).toHaveLength(rowsBefore + 1) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("re-appends the row when a rewind removed it before the reclaim", async () => { + const task = await createTask() + task.messageQueueService.addMessage("Rewound row") + const result = await task.ask("completion_result", "Done", false) + const access = getQueueTaskTestAccess(task) + access.saveClineMessages = vi.fn().mockResolvedValue(false) + const feedbackRows = () => task["clineMessages"].filter((message) => message.say === "user_feedback") + + // The row lands, then every save fails and the claim is released. + vi.useFakeTimers() + try { + const first = task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images) + await vi.runAllTimersAsync() + await expect(first).resolves.toBe(false) + } finally { + vi.useRealTimers() + } + expect(feedbackRows()).toHaveLength(1) + + // A rewind truncates the conversation, so the recorded row is gone while + // the message is still queued and reclaimable. + task["clineMessages"] = task["clineMessages"].filter((message) => message.say !== "user_feedback") + expect(feedbackRows()).toHaveLength(0) + + // The reclaim must re-append: consuming without the row would drop the + // feedback from the conversation entirely. + access.saveClineMessages = vi.fn().mockResolvedValue(true) + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).resolves.toBe(true) + expect(feedbackRows()).toHaveLength(1) + expect(task.messageQueueService.isEmpty()).toBe(true) + }) + + it("keys the row record by message id, not by timestamp", async () => { + const task = await createTask() + task.messageQueueService.addMessage("Same millisecond") + const result = await task.ask("completion_result", "Done", false) + const access = getQueueTaskTestAccess(task) + access.saveClineMessages = vi.fn().mockResolvedValue(false) + const feedbackRows = () => task["clineMessages"].filter((message) => message.say === "user_feedback") + + vi.useFakeTimers() + try { + const first = task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images) + await vi.runAllTimersAsync() + await expect(first).resolves.toBe(false) + } finally { + vi.useRealTimers() + } + expect(feedbackRows()).toHaveLength(1) + + // A rewind drops the feedback row but leaves a different row carrying the + // same `ts`: `say` stamps `Date.now()`, so two rows written in the same + // millisecond collide. Keying the record by `ts` would read this survivor as + // the feedback row and skip the re-append, dropping the text entirely. + const feedbackTs = feedbackRows()[0]!.ts + task["clineMessages"] = task["clineMessages"].filter((message) => message.say !== "user_feedback") + task["clineMessages"].push({ ts: feedbackTs, type: "say", say: "text", text: "Unrelated row" }) + expect(feedbackRows()).toHaveLength(0) + + access.saveClineMessages = vi.fn().mockResolvedValue(true) + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).resolves.toBe(true) + expect(feedbackRows()).toHaveLength(1) + expect(feedbackRows()[0]!.text).toBe("Same millisecond") + }) + + it("does not double-append when say throws after the row landed", async () => { + const task = await createTask() + task.messageQueueService.addMessage("Row landed then threw") + const result = await task.ask("completion_result", "Done", false) + const access = getQueueTaskTestAccess(task) + access.saveClineMessages = vi.fn().mockResolvedValue(true) + const rowsBefore = task["clineMessages"].length + // The row is pushed the way `addToClineMessages` pushes it (synchronously, + // before its own awaits), then the call rejects the way a throwing + // `Message` listener makes `say` reject. + access.addToClineMessages = vi.fn(async (message: import("@roo-code/types").ClineMessage) => { + message.messageId ??= crypto.randomUUID() + task["clineMessages"].push(message) + throw new Error("listener threw") + }) + + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).rejects.toThrow("listener threw") + expect(task["clineMessages"]).toHaveLength(rowsBefore + 1) + + // The row is retained in history, so the reclaim must not append a second + // one, and the release must leave the message reclaimable rather than + // leaked as claimed. + await expect( + task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images), + ).resolves.toBe(true) + expect(task["clineMessages"]).toHaveLength(rowsBefore + 1) + expect(task.messageQueueService.isEmpty()).toBe(true) + expect(task["drainRetainedQueuedMessageIds"].size).toBe(0) + }) }) diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 4011f802e9..96a66ac964 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -217,7 +217,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 } @@ -268,14 +270,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 2713e08ffe..8cc1205205 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/EditFileTool.ts b/src/core/tools/EditFileTool.ts index a7301e2ac9..1f6eb188a4 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -467,7 +467,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 895b929ed2..0eedd74b05 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 9855e6e417..99edf8ee6b 100644 --- a/src/core/tools/ExecuteCommandTool.ts +++ b/src/core/tools/ExecuteCommandTool.ts @@ -326,6 +326,26 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { // Convert agent-specified timeout from seconds to milliseconds const agentTimeout = resolveAgentTimeoutMs(timeoutSeconds) + // The background-completion drain 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. Every site publishes + // through `publishToolResult`, which settles the signal with the + // publication outcome, so the drain skips after a failed publication. + // Every throw path must settle it too, or the awaiting drain hangs. + 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, @@ -333,17 +353,22 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { terminalShellIntegrationDisabled, commandExecutionTimeout, agentTimeout, + toolResultPublished, toolCallId, } + let shouldDrainQueuedMessages = false try { - const [rejected, result] = await executeCommandInTerminal(task, options) + const [rejected, result, commandSubmitted] = await executeCommandInTerminal(task, options) if (rejected) { markToolRejected(task, toolCallId) } - pushToolResult(result) + publishToolResult(result) + // Only drain when the command actually ran: early validation + // failures end the turn without an execution. + shouldDrainQueuedMessages = commandSubmitted } catch (error: unknown) { // Invalidate pending ask from first execution to prevent race condition task.supersedePendingAsk() @@ -353,27 +378,54 @@ 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) { - markToolRejected(task, toolCallId) + try { + const [rejected, result, commandSubmitted] = await executeCommandInTerminal(task, { + ...options, + terminalShellIntegrationDisabled: true, + }) + + if (rejected) { + markToolRejected(task, toolCallId) + } + + publishToolResult(result) + shouldDrainQueuedMessages = commandSubmitted + } catch (fallbackError) { + // The retry never published: settle before rethrowing. + 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) { + settleToolResultPublished?.(false) + throw warningError } + // This arm owes the same drain the normal and fallback arms give, + // set only after the warning result published. + shouldDrainQueuedMessages = error.commandSubmitted + } 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 first: false skips the drain. + settleToolResultPublished?.(false) + throw error + } + } + + // Awaited so a failed submission is observable, and never misattributed + // as a shell-integration error after the result was already published. + if (shouldDrainQueuedMessages) { + try { + await task.processQueuedMessages() + } catch (error) { + console.error("[ExecuteCommandTool] Failed to process queued messages:", error) } } @@ -397,6 +449,12 @@ export type ExecuteCommandOptions = { terminalShellIntegrationDisabled?: boolean commandExecutionTimeout?: number agentTimeout?: number + /** + * Resolves true on successful publication, false when it failed. The + * background-completion drain awaits this so queued messages are never + * submitted ahead of the tool result, and skips after a failure. + */ + toolResultPublished?: Promise toolCallId: string } @@ -409,9 +467,11 @@ export async function executeCommandInTerminal( terminalShellIntegrationDisabled = true, commandExecutionTimeout = 0, agentTimeout = 0, + // Direct callers without a tool-result lifecycle drain immediately. + toolResultPublished = Promise.resolve(true), toolCallId, }: ExecuteCommandOptions, -): Promise<[boolean, ToolResponse]> { +): Promise<[boolean, ToolResponse, boolean]> { // Convert milliseconds back to seconds for display purposes. const commandExecutionTimeoutSeconds = commandExecutionTimeout / 1000 let workingDir: string @@ -425,7 +485,8 @@ export async function executeCommandInTerminal( const workingDirectoryError = await commandWorkingDirectoryError(workingDir) if (workingDirectoryError) { - return [false, workingDirectoryError] + // The command never ran (working directory validation failed). + return [false, workingDirectoryError, false] } let runInBackground = false @@ -434,7 +495,10 @@ export async function executeCommandInTerminal( let persistedResult: PersistedCommandOutput | undefined let exitDetails: ExitCodeDetails | undefined let shellIntegrationError: ShellIntegrationError | undefined - + // Whether the command text actually reached the terminal. `shell_execution_started` + // fires only after `TerminalProcess.run()` called `sendText`/`executeCommand`, + // so it proves a timeout did not beat submission. + let commandSubmitted = false const { terminalProvider, isCmdExeFallback } = getTerminalProviderForExecution(terminalShellIntegrationDisabled) const provider = await task.providerRef.deref() @@ -476,6 +540,10 @@ export async function executeCommandInTerminal( let lastCommandOutputEmitAt = 0 let pendingCommandOutputEmitTimer: NodeJS.Timeout | undefined let commandOutputSayChain: Promise = Promise.resolve() + // Outcome of the most recent `command_output` publication: the final forced + // one is the last queued before the background drain, so it reports whether + // that output reached the webview. A later success clears an earlier failure. + let commandOutputPublishFailed = false const queueCommandOutputMessage = (text: string, partial: boolean, force = false): Promise => { if (!force && text === lastQueuedCommandOutput) { @@ -488,8 +556,10 @@ export async function executeCommandInTerminal( await task.say("command_output", text, undefined, partial, undefined, undefined, { isNonInteractive: true, }) + commandOutputPublishFailed = false }) .catch((error) => { + commandOutputPublishFailed = true console.error("[ExecuteCommandTool] Failed to publish command output:", error) }) @@ -575,11 +645,43 @@ 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. Wait on the per-invocation + // publication signal: completion can land inside the settle delay. + if (!runInBackground) { + return + } + const published = await toolResultPublished + // A cancelled task must not drain (the guards inside + // processQueuedMessages would no-op anyway, and skipping avoids + // error-log noise on a dying task), and neither must a failed + // publication: handleError reports that error first. + if (task.abort || task.abandoned || !published) { + return + } + // The final `command_output` say is part of what the drain depends + // on: #937 requires queued feedback to be considered together with + // the finished command's output. A rejected say is swallowed by the + // chain's `.catch`, so read the recorded outcome instead. + if (commandOutputPublishFailed) { + 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) }) }, onShellExecutionStarted: (pid: number | undefined) => { + // The terminal reported that execution began, which only happens after + // `TerminalProcess.run()` handed the command to the shell. This is the + // submission proof the timeout arm reads; a pre-start timeout leaves it + // false because `run()` never reached `sendText`/`executeCommand`. + commandSubmitted = true const status: CommandExecutionStatus = { executionId, status: "started", pid, command } postCommandExecutionStatus(provider, status) }, @@ -657,9 +759,15 @@ export async function executeCommandInTerminal( postCommandExecutionStatus(provider, status) await task.say("error", t("common:errors:command_timeout", { seconds: commandExecutionTimeoutSeconds })) task.didToolFailInCurrentTurn = true + // Report the tracked submission, not an unconditional true: a timeout + // that beat shell integration means `TerminalProcess.run()` never + // called `sendText`/`executeCommand`, so nothing ran and the caller + // must not drain queued feedback against a command that never started. return [ false, `The command was terminated after exceeding a user-configured ${commandExecutionTimeoutSeconds}s timeout. Do not try to re-run the command.`, + + commandSubmitted, ] } throw error @@ -695,7 +803,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). @@ -710,6 +818,7 @@ export async function executeCommandInTerminal( return [ false, `Command executed in terminal within working directory '${currentWorkingDir}'. ${exitStatus}\nOutput:\n${result}`, + true, ] } else { return [ @@ -719,6 +828,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/SearchReplaceTool.ts b/src/core/tools/SearchReplaceTool.ts index a0f2dadfcc..edd12cd3a4 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__/applyDiffTool.failureReporting.spec.ts b/src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts index bf7b3f5351..28bee7f717 100644 --- a/src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts +++ b/src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts @@ -99,7 +99,7 @@ describe("ApplyDiffTool failure reporting", () => { ask: vi.fn().mockResolvedValue(undefined), recordToolError: vi.fn(), recordToolUsage: vi.fn(), - processQueuedMessages: vi.fn(), + processQueuedMessages: vi.fn().mockResolvedValue(true), sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing param error"), diffStrategy: undefined, } as unknown as Task diff --git a/src/core/tools/__tests__/applyDiffTool.spec.ts b/src/core/tools/__tests__/applyDiffTool.spec.ts new file mode 100644 index 0000000000..d128fa4ca4 --- /dev/null +++ b/src/core/tools/__tests__/applyDiffTool.spec.ts @@ -0,0 +1,195 @@ +// 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, + toolCallId: "test-tool-call-id", + }) + + // 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, + toolCallId: "test-tool-call-id", + }) + + // 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, + toolCallId: "test-tool-call-id", + }) + + // 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 eb89838cad..e9888462e5 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, } @@ -95,3 +97,187 @@ 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, + toolCallId: "test-tool-call-id", + }) + + // 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, + toolCallId: "test-tool-call-id", + }) + + // 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, + toolCallId: "test-tool-call-id", + }) + + // 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__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 95a67f04d7..4e40513a5e 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -149,7 +149,7 @@ describe("editFileTool", () => { mockTask.ask = vi.fn().mockResolvedValue(undefined) mockTask.recordToolError = vi.fn() mockTask.recordToolUsage = vi.fn() - mockTask.processQueuedMessages = vi.fn() + mockTask.processQueuedMessages = vi.fn().mockResolvedValue(true) mockTask.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") mockAskApproval = vi.fn().mockResolvedValue(true) diff --git a/src/core/tools/__tests__/editTool.spec.ts b/src/core/tools/__tests__/editTool.spec.ts index fbb14d7ed2..b7b65bef54 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) @@ -417,6 +417,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 774601c5ca..a75ebf4884 100644 --- a/src/core/tools/__tests__/executeCommand.spec.ts +++ b/src/core/tools/__tests__/executeCommand.spec.ts @@ -275,10 +275,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() }) @@ -312,7 +313,7 @@ describe("executeCommand", () => { customCwd: "/existing/file.txt/child", }) - expect(result).toEqual([false, "Working directory '/existing/file.txt/child' is not a directory."]) + expect(result).toEqual([false, "Working directory '/existing/file.txt/child' is not a directory.", false]) expect(TerminalRegistry.getOrCreateTerminal).not.toHaveBeenCalled() }) }) @@ -433,8 +434,10 @@ describe("executeCommand", () => { expect(mockTask.terminalProcesses.size).toBe(0) }) - it("posts timeout status when command execution exceeds the user limit", async () => { + it("reports no submission when the user timeout fires before the command starts", async () => { vitest.useFakeTimers() + // A never-starting process: `TerminalProcess.run()` never reached + // `sendText`/`executeCommand`, so no `shell_execution_started` fires. const pendingProcess = Object.assign(new Promise(() => {}), { continue: vitest.fn(), abort: vitest.fn(), @@ -449,9 +452,11 @@ 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) + // Nothing ran, so the caller must not drain queued feedback. + expect(commandSubmitted).toBe(false) expect(result).toContain("terminated after exceeding") expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith( expect.objectContaining({ @@ -462,6 +467,35 @@ describe("executeCommand", () => { expect(pendingProcess.abort).toHaveBeenCalled() }) + it("reports submission when the user timeout fires after the command started", async () => { + vitest.useFakeTimers() + const pendingProcess = Object.assign(new Promise(() => {}), { + continue: vitest.fn(), + abort: vitest.fn(), + }) + mockTerminal.runCommand.mockImplementation((_command: string, callbacks: RooTerminalCallbacks) => { + // The shell reported execution start, so the command really ran. + setTimeout(() => callbacks.onShellExecutionStarted(4242, mockProcess), 0) + return pendingProcess + }) + + const executionPromise = executeCommandInTerminal(mockTask, { + executionId: "test-123", + toolCallId: "test-tool-call-id", + command: "sleep 10", + terminalShellIntegrationDisabled: false, + commandExecutionTimeout: 1_000, + }) + await vitest.advanceTimersByTimeAsync(1_000) + const [rejected, result, commandSubmitted] = await executionPromise + + expect(rejected).toBe(false) + // The command did run, so the drain is owed. + expect(commandSubmitted).toBe(true) + expect(result).toContain("terminated after exceeding") + expect(pendingProcess.abort).toHaveBeenCalled() + }) + it("should handle completed command with exit code 0", async () => { mockTerminal.getCurrentWorkingDirectory.mockReturnValue("/test/project") mockTerminal.runCommand.mockImplementation((_command: string, callbacks: RooTerminalCallbacks) => { @@ -480,10 +514,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'") }) @@ -540,10 +575,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 c580b55102..713cbbb618 100644 --- a/src/core/tools/__tests__/executeCommandTool.spec.ts +++ b/src/core/tools/__tests__/executeCommandTool.spec.ts @@ -52,6 +52,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. @@ -93,7 +127,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 = { @@ -108,6 +144,7 @@ describe("executeCommandTool", () => { recordToolUsage: vitest.fn().mockReturnValue({} as ToolUsage), recordToolError: vitest.fn(), supersedePendingAsk: vitest.fn(), + processQueuedMessages: vitest.fn().mockResolvedValue(true), terminalProcesses: new Map(), recordBlanketCommandDenial: vitest.fn(), providerRef: { @@ -1008,6 +1045,33 @@ 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, + toolCallId: "test-tool-call-id", + }) + + // 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) @@ -1018,6 +1082,428 @@ 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, + toolCallId: "test-tool-call-id", + }) + + 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, + toolCallId: "test-tool-call-id", + }) + + 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, + toolCallId: "test-tool-call-id", + }) + + 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("does not process queued messages when the user timeout fires before the command starts", 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) + + // A never-starting process: `TerminalProcess.run()` never reached + // `sendText`/`executeCommand`, so the command did not run. + 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, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("terminated after exceeding a user-configured"), + ) + // Nothing ran, so no queued feedback may be consumed against it. + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + }) + + it("processes queued messages once after a user-configured execution timeout that started", async () => { + mockToolUse.params.command = "sleep 10" + mockToolUse.nativeArgs = { command: "sleep 10" } + 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(), + }) + const pendingTerminalProcess = pendingProcess as unknown as RooTerminalProcess + vitest.mocked(TerminalRegistry.getOrCreateTerminal).mockResolvedValue({ + runCommand: vitest.fn().mockImplementation((_command: string, callbacks: RooTerminalCallbacks) => { + // The shell reported execution start, so the command really ran + // and the drain is owed. + setTimeout(() => callbacks.onShellExecutionStarted(4242, pendingTerminalProcess), 0) + return 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, + toolCallId: "test-tool-call-id", + }) + + 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, + toolCallId: "test-tool-call-id", + }) + + 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, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + "Unable to access working directory '/nonexistent/working/dir': ENOENT", + ) + 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, + toolCallId: "test-tool-call-id", + }) + + // 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, + toolCallId: "test-tool-call-id", + }) + + 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, + toolCallId: "test-tool-call-id", + }) + + 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, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + "Unable to access working directory '/nonexistent/working/dir': ENOENT", + ) + 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, + toolCallId: "test-tool-call-id", + }) + + // 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, + toolCallId: "test-tool-call-id", + }) + + 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 @@ -1253,5 +1739,474 @@ 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("drains after publishing the warning 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.", + ) + // The command ran, so this arm owes the same drain as the normal and + // fallback arms — and only after the warning result is published. + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + expect(mockPushToolResult.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.processQueuedMessages.mock.invocationCallOrder[0], + ) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("skips the drain when the shell-integration warning result cannot be published", 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) + mockPushToolResult.mockImplementationOnce(() => { + throw new Error("publication failed") + }) + 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 + + callbacks.onShellExecutionStarted!(1234, proc) + callbacks.onNoShellIntegration!({ message: "exit code unknown", commandSubmitted: true }, proc) + terminal.resolveProcess() + await vitest.advanceTimersByTimeAsync(100) + await handlePromise + + // The warning could not be delivered, so the drain must not consume + // queued feedback against a result the model never saw. + expect(mockCline.processQueuedMessages).not.toHaveBeenCalled() + expect(mockHandleError).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("skips the background-completion drain when the final command_output say fails", async () => { + vitest.useFakeTimers() + 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 runs the post-result drain. + await vitest.advanceTimersByTimeAsync(2_000) + await handlePromise + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + + // The final non-partial command_output publication rejects. The + // chain's `.catch` swallows it, so only the recorded outcome can + // tell the drain that the output the model must see never landed. + mockCline.say.mockImplementation( + async (type: string, _text?: string, _images?: string[], partial?: boolean) => { + if (type === "command_output" && partial === false) { + throw new Error("final output publication failed") + } + return undefined + }, + ) + + await callbacks.onCompleted!("server exited\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + await vitest.advanceTimersByTimeAsync(100) + + // No second drain: queued feedback must not be consumed while the + // final output the model needs is missing. + expect(mockCline.processQueuedMessages).toHaveBeenCalledTimes(1) + } 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__/searchReplaceTool.spec.ts b/src/core/tools/__tests__/searchReplaceTool.spec.ts index 65311462cb..dbf5007cc7 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) @@ -388,6 +388,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 7cb9d01585..ac4c38a3ae 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) @@ -393,6 +394,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.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 685a7b11c1..c852d1ec92 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -966,6 +966,88 @@ describe("webviewMessageHandler - image mentions", () => { "data:image/png;base64,from-mention", ]) }) + + // A task switch can land while the image mentions resolve. The message was + // addressed to the task that was current when the webview sent it, so the + // post-await re-read would hand it to whichever task is current afterwards: + // a queued message would land in B's queue and B's command ask could consume + // it as approval, and an ask response would answer B's pending prompt. + it.each([ + ["queueMessage", "queueMessage"], + ["askResponse", "askResponse"], + ] as const)("delivers %s to the task that was current before the await", async (_label, type) => { + const taskA = { + cwd: "/mock/workspace", + rooIgnoreController: undefined, + messageQueueService: { addMessage: vi.fn() }, + handleWebviewAskResponse: vi.fn(), + } + const taskB = { + cwd: "/mock/workspace", + rooIgnoreController: undefined, + messageQueueService: { addMessage: vi.fn() }, + handleWebviewAskResponse: vi.fn(), + } + + // The switch happens while the handler is suspended at the await. + let currentTask = taskA + vi.mocked(mockClineProvider.getCurrentTask).mockImplementation(() => currentTask as never) + vi.mocked(resolveImageMentions).mockImplementationOnce(async ({ text, images }) => { + currentTask = taskB + return { text, images: images ?? [] } + }) + + await webviewMessageHandler(mockClineProvider, { + type, + askResponse: "messageResponse", + text: "for task A", + images: [], + }) + + expect(taskA.messageQueueService.addMessage).toHaveBeenCalledTimes(type === "queueMessage" ? 1 : 0) + expect(taskB.messageQueueService.addMessage).not.toHaveBeenCalled() + expect(taskA.handleWebviewAskResponse).toHaveBeenCalledTimes(type === "askResponse" ? 1 : 0) + expect(taskB.handleWebviewAskResponse).not.toHaveBeenCalled() + }) + + // Mentions must resolve against the workspace of the task the message is + // delivered to. The cwd is read before the handler's own awaits, so a task + // switch during the state read cannot move resolution to another workspace. + it.each([ + ["queueMessage", "queueMessage"], + ["askResponse", "askResponse"], + ] as const)("resolves %s mentions against the workspace captured before the state await", async (_label, type) => { + const taskA = { + cwd: "/workspace-a", + rooIgnoreController: undefined, + messageQueueService: { addMessage: vi.fn() }, + handleWebviewAskResponse: vi.fn(), + } + const taskB = { + cwd: "/workspace-b", + rooIgnoreController: undefined, + messageQueueService: { addMessage: vi.fn() }, + handleWebviewAskResponse: vi.fn(), + } + + let currentTask = taskA + vi.mocked(mockClineProvider.getCurrentTask).mockImplementation(() => currentTask as never) + // The switch lands while the handler is suspended on the state read, before + // mention resolution starts. + mockClineProvider.getState = vi.fn().mockImplementation(async () => { + currentTask = taskB + return { maxImageFileSize: 5, maxTotalImageSize: 20 } + }) + + await webviewMessageHandler(mockClineProvider, { + type, + askResponse: "messageResponse", + text: "see @/img.png", + images: [], + }) + + expect(vi.mocked(resolveImageMentions)).toHaveBeenCalledWith(expect.objectContaining({ cwd: "/workspace-a" })) + }) }) describe("webviewMessageHandler - requestOllamaModels", () => { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index bd7c589958..38ee74fca9 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -223,16 +223,20 @@ export const webviewMessageHandler = async ( /** * Resolves image file mentions in incoming messages. * Matches read_file behavior: respects size limits and model capabilities. + * + * The cwd is read before the awaits below: it must be the workspace of the + * task this message belongs to, which callers capture before their own await. */ const resolveIncomingImages = async (payload: { text?: string; images?: string[] }) => { const text = payload.text ?? "" const images = payload.images const currentTask = provider.getCurrentTask() + const cwd = getCurrentCwd() const state = await provider.getState() const resolved = await resolveImageMentions({ text, images, - cwd: getCurrentCwd(), + cwd, rooIgnoreController: currentTask?.rooIgnoreController, maxImageFileSize: state.maxImageFileSize, maxTotalImageSize: state.maxTotalImageSize, @@ -730,10 +734,10 @@ export const webviewMessageHandler = async ( case "askResponse": { + // Same pre-await binding as `queueMessage`. + const targetTask = provider.getCurrentTask() const resolved = await resolveIncomingImages({ text: message.text, images: message.images }) - provider - .getCurrentTask() - ?.handleWebviewAskResponse(message.askResponse!, resolved.text, resolved.images) + targetTask?.handleWebviewAskResponse(message.askResponse!, resolved.text, resolved.images) } break @@ -3962,8 +3966,12 @@ export const webviewMessageHandler = async ( */ case "queueMessage": { + // Bind before the await: a task switch during image resolution would + // route this message into the new task's queue, where its text can + // answer that task's command ask. + const targetTask = provider.getCurrentTask() const resolved = await resolveIncomingImages({ text: message.text, images: message.images }) - provider.getCurrentTask()?.messageQueueService.addMessage(resolved.text, resolved.images) + targetTask?.messageQueueService.addMessage(resolved.text, resolved.images) break } case "removeQueuedMessage": {