diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index ebfd94324e..34eeff2a70 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -16,6 +16,8 @@ const SUBTASK_APPROVAL_RESTORE_CHILD_MARKER = "SUBTASK_CHILD_APPROVAL_RESTORE" const SUBTASK_XPROFILE_PARENT_MARKER = "SUBTASK_PARENT_CROSS_PROFILE" const SUBTASK_XPROFILE_SAME_CHILD_MARKER = "SUBTASK_CHILD_SAME_PROFILE" const SUBTASK_XPROFILE_DIFFERENT_CHILD_MARKER = "SUBTASK_CHILD_DIFFERENT_PROFILE" +export const SUBTASK_QUEUED_INPUT_PARENT_MARKER = "SUBTASK_PARENT_QUEUED_INPUT" +export const SUBTASK_QUEUED_INPUT_CHILD_MARKER = "SUBTASK_CHILD_QUEUED_INPUT" const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` export const SUBTASK_PARENT_PROMPT = `${SUBTASK_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_CHILD_PROMPT}" Do not answer directly.` @@ -59,6 +61,14 @@ export const SUBTASK_XPROFILE_SAME_CHILD_RESULT = "Same-profile child completed" export const SUBTASK_XPROFILE_DIFFERENT_CHILD_RESULT = "Different-profile child completed" export const SUBTASK_XPROFILE_PARENT_RESULT = "Sequential cross-profile parent resumed" +const SUBTASK_QUEUED_INPUT_INITIAL_RESULT = "Child completed before queued input" +export const SUBTASK_QUEUED_INPUT_MESSAGE = "Use the queued instruction before completing." +export const SUBTASK_QUEUED_INPUT_CHILD_RESULT = "Child processed queued input" +export const SUBTASK_QUEUED_INPUT_PARENT_RESULT = "Parent resumed after queued input" +const SUBTASK_QUEUED_INPUT_CHILD_PROMPT = `${SUBTASK_QUEUED_INPUT_CHILD_MARKER}: Complete immediately with the exact result "${SUBTASK_QUEUED_INPUT_INITIAL_RESULT}".` +export const SUBTASK_QUEUED_INPUT_PARENT_PROMPT = `${SUBTASK_QUEUED_INPUT_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_QUEUED_INPUT_CHILD_PROMPT}" Do not answer directly. When the subtask returns, complete with the exact result "${SUBTASK_QUEUED_INPUT_PARENT_RESULT}".` +export const SUBTASK_QUEUED_INPUT_RESPONSE_LATENCY_MS = 2_000 + // Scheduler regression tests — exercises TaskScheduler + run() dispatch post-CodeRabbit fix. // Separate markers to avoid collisions with the other subtask fixtures. const SCHED_STANDALONE_MARKER = "SCHED_STANDALONE_INTERRUPT_RESUME" @@ -179,6 +189,81 @@ export function addSubtaskFixtures(mock: InstanceType) { }, }) + mock.addFixture({ + match: { + userMessage: SUBTASK_QUEUED_INPUT_PARENT_MARKER, + sequenceIndex: 0, + }, + response: { + toolCalls: [ + { + name: "new_task", + arguments: JSON.stringify({ + mode: "ask", + message: SUBTASK_QUEUED_INPUT_CHILD_PROMPT, + }), + id: "call_queued_input_parent_new_task_001", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + lastUserMessageContains(req, SUBTASK_QUEUED_INPUT_CHILD_MARKER) && + !requestContains(req, [SUBTASK_QUEUED_INPUT_PARENT_MARKER]) && + !requestContains(req, [SUBTASK_QUEUED_INPUT_MESSAGE]), + }, + streamingProfile: { ttft: SUBTASK_QUEUED_INPUT_RESPONSE_LATENCY_MS }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: SUBTASK_QUEUED_INPUT_INITIAL_RESULT }), + id: "call_queued_input_child_initial_completion_002", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + requestContains(req, [SUBTASK_QUEUED_INPUT_CHILD_MARKER, SUBTASK_QUEUED_INPUT_MESSAGE]) && + !requestContains(req, [SUBTASK_QUEUED_INPUT_PARENT_MARKER]), + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: SUBTASK_QUEUED_INPUT_CHILD_RESULT }), + id: "call_queued_input_child_revised_completion_003", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + requestContains(req, [ + SUBTASK_QUEUED_INPUT_PARENT_MARKER, + SUBTASK_RESULT_INJECTION, + SUBTASK_QUEUED_INPUT_CHILD_RESULT, + ]), + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: SUBTASK_QUEUED_INPUT_PARENT_RESULT }), + id: "call_queued_input_parent_completion_004", + }, + ], + }, + }) + mock.addFixture({ match: { userMessage: new RegExp(SUBTASK_FAST_PARENT_MARKER), diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index 857c8accc5..0da28c1aca 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -27,6 +27,12 @@ import { SUBTASK_INTERRUPT_PARENT_PROMPT, SUBTASK_INTERRUPT_PARENT_RESULT, SUBTASK_PARENT_PROMPT, + SUBTASK_QUEUED_INPUT_CHILD_MARKER, + SUBTASK_QUEUED_INPUT_CHILD_RESULT, + SUBTASK_QUEUED_INPUT_MESSAGE, + SUBTASK_QUEUED_INPUT_PARENT_MARKER, + SUBTASK_QUEUED_INPUT_PARENT_PROMPT, + SUBTASK_QUEUED_INPUT_PARENT_RESULT, SUBTASK_XPROFILE_DIFFERENT_CHILD_RESULT, SUBTASK_XPROFILE_PARENT_PROMPT, SUBTASK_XPROFILE_PARENT_RESULT, @@ -260,6 +266,73 @@ suite("Roo Code Subtasks", function () { } }) + test("queued input interrupts child completion before the parent resumes", async () => { + const api = globalThis.api + const says: Record = {} + + const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { + if (message.type === "say" && message.partial !== true) { + says[taskId] = says[taskId] || [] + says[taskId].push(message) + } + } + + api.on(RooCodeEventName.Message, messageHandler) + + try { + const parentTaskId = await api.startNewTask({ + configuration: { + mode: "ask", + alwaysAllowModeSwitch: true, + alwaysAllowSubtasks: true, + autoApprovalEnabled: true, + enableCheckpoints: false, + }, + text: SUBTASK_QUEUED_INPUT_PARENT_PROMPT, + }) + + let childTaskId: string | undefined + await waitFor(() => { + const current = api.getCurrentTaskStack().at(-1) + if (current && current !== parentTaskId) { + childTaskId = current + return true + } + return false + }) + + await waitForAimockRequestContaining(SUBTASK_QUEUED_INPUT_CHILD_MARKER, SUBTASK_QUEUED_INPUT_PARENT_MARKER) + + const completedParentTaskId = await waitUntilCompleted({ + api, + start: async () => { + await api.sendMessage(SUBTASK_QUEUED_INPUT_MESSAGE) + return parentTaskId + }, + }) + + assert.strictEqual(completedParentTaskId, parentTaskId) + assert.ok( + says[childTaskId!]?.some( + ({ say, text }) => + say === "completion_result" && text?.trim() === SUBTASK_QUEUED_INPUT_CHILD_RESULT, + ), + "Child should process the queued instruction before returning to its parent", + ) + assert.strictEqual( + says[parentTaskId]?.find(({ say }) => say === "completion_result")?.text?.trim(), + SUBTASK_QUEUED_INPUT_PARENT_RESULT, + "Parent should resume only after the child processes the queued instruction", + ) + } finally { + api.off(RooCodeEventName.Message, messageHandler) + while (api.getCurrentTaskStack().length > 0) { + await api.clearCurrentTask() + } + await waitFor(() => api.getCurrentTaskStack().length === 0).catch(() => {}) + } + }) + // Smoke: child completing normally must resume the parent task. test("child task returns to parent after normal completion", async () => { const api = globalThis.api diff --git a/apps/vscode-e2e/src/suite/tools/use-mcp-tool.test.ts b/apps/vscode-e2e/src/suite/tools/use-mcp-tool.test.ts index 21e189d7c1..a413e37600 100644 --- a/apps/vscode-e2e/src/suite/tools/use-mcp-tool.test.ts +++ b/apps/vscode-e2e/src/suite/tools/use-mcp-tool.test.ts @@ -64,9 +64,10 @@ suite("Roo Code use_mcp_tool Tool", function () { { mcpServers: { [FILESYSTEM_SERVER_NAME]: { - command: process.env.npm_node_execpath ?? "node", + command: process.execPath, args: [path.join(__dirname, "fixtures", "filesystem-mcp-server.js"), workspaceDir], env: { + ELECTRON_RUN_AS_NODE: "1", MCP_TEST_READY_FILE: mcpServerReadyPath, }, alwaysAllow: [ diff --git a/src/core/mentions/__tests__/resolveImageMentions.spec.ts b/src/core/mentions/__tests__/resolveImageMentions.spec.ts index 9169f40ef7..4a3e2c204f 100644 --- a/src/core/mentions/__tests__/resolveImageMentions.spec.ts +++ b/src/core/mentions/__tests__/resolveImageMentions.spec.ts @@ -1,6 +1,6 @@ import * as path from "path" -import { resolveImageMentions } from "../resolveImageMentions" +import { normalizeSuppliedImages, resolveImageMentions } from "../resolveImageMentions" vi.mock("../../tools/helpers/imageHelpers", () => ({ isSupportedImageFormat: vi.fn((ext: string) => @@ -193,3 +193,50 @@ describe("resolveImageMentions", () => { expect(mockValidateImage).toHaveBeenCalledWith(expect.any(String), true, 10, 50, 0) }) }) + +describe("normalizeSuppliedImages", () => { + it("should accept supported image data URIs and reject malformed or unsupported values", () => { + const payload = Buffer.from("image").toString("base64") + + expect(normalizeSuppliedImages()).toEqual([]) + expect( + normalizeSuppliedImages([ + `data:image/svg+xml;base64,${payload}`, + `data:image/x-icon;base64,${payload}`, + `data:image/png;base64,${payload}`, + `prefix-data:image/png;base64,${payload}`, + `data:image/png;base64,${payload}-suffix`, + "data:image/png;base64,YQ=", + `data:image/unsupported;base64,${payload}`, + ]), + ).toEqual([ + `data:image/svg+xml;base64,${payload}`, + `data:image/x-icon;base64,${payload}`, + `data:image/png;base64,${payload}`, + ]) + }) + + it("should enforce per-image and total decoded size limits", () => { + const image = `data:image/png;base64,${Buffer.from("four bytes").toString("base64")}` + const secondImage = `data:image/png;base64,${Buffer.from("nine bytes").toString("base64")}` + const sizeInMB = Buffer.byteLength("four bytes") / (1024 * 1024) + + expect(normalizeSuppliedImages([image], { maxImageFileSize: sizeInMB / 2 })).toEqual([]) + expect(normalizeSuppliedImages([image], { maxImageFileSize: sizeInMB })).toEqual([image]) + expect(normalizeSuppliedImages([image, secondImage], { maxTotalImageSize: sizeInMB * 1.5 })).toEqual([image]) + expect(normalizeSuppliedImages([image], { maxTotalImageSize: sizeInMB })).toEqual([image]) + }) + + it("should apply supplied-image limits through resolveImageMentions", async () => { + const image = `data:image/png;base64,${Buffer.from("image").toString("base64")}` + + const result = await resolveImageMentions({ + text: "No mentions", + images: [image], + cwd: "/workspace", + maxImageFileSize: 0, + }) + + expect(result.images).toEqual([]) + }) +}) diff --git a/src/core/mentions/resolveImageMentions.ts b/src/core/mentions/resolveImageMentions.ts index 0a0344348f..bb657e0d05 100644 --- a/src/core/mentions/resolveImageMentions.ts +++ b/src/core/mentions/resolveImageMentions.ts @@ -12,6 +12,11 @@ import { const MAX_IMAGES_PER_MESSAGE = 20 +interface NormalizeSuppliedImagesOptions { + maxImageFileSize?: number + maxTotalImageSize?: number +} + export interface ResolveImageMentionsOptions { text: string images?: string[] @@ -46,6 +51,37 @@ function dedupePreserveOrder(values: string[]): string[] { return result } +export function normalizeSuppliedImages( + images?: string[], + { + maxImageFileSize = DEFAULT_MAX_IMAGE_FILE_SIZE_MB, + maxTotalImageSize = DEFAULT_MAX_TOTAL_IMAGE_SIZE_MB, + }: NormalizeSuppliedImagesOptions = {}, +): string[] { + if (!images?.length) return [] + + const normalized: string[] = [] + let totalSize = 0 + + for (const image of images) { + if (normalized.length >= MAX_IMAGES_PER_MESSAGE) break + + const match = image.match(/^data:image\/([a-zA-Z0-9.+-]+);base64,([A-Za-z0-9+/]+={0,2})$/) + if (!match || match[2].length % 4 !== 0) continue + + const extension = match[1] === "svg+xml" ? ".svg" : match[1] === "x-icon" ? ".ico" : `.${match[1]}` + if (!isSupportedImageFormat(extension)) continue + + const sizeInMB = Buffer.byteLength(match[2], "base64") / (1024 * 1024) + if (sizeInMB > maxImageFileSize || totalSize + sizeInMB > maxTotalImageSize) continue + + totalSize += sizeInMB + normalized.push(image) + } + + return dedupePreserveOrder(normalized) +} + /** * Resolves local image file mentions like `@/path/to/image.png` found in `text` into `data:image/...;base64,...` * and appends them to the outgoing `images` array. @@ -66,7 +102,7 @@ export async function resolveImageMentions({ maxImageFileSize = DEFAULT_MAX_IMAGE_FILE_SIZE_MB, maxTotalImageSize = DEFAULT_MAX_TOTAL_IMAGE_SIZE_MB, }: ResolveImageMentionsOptions): Promise { - const existingImages = Array.isArray(images) ? images : [] + const existingImages = normalizeSuppliedImages(images, { maxImageFileSize, maxTotalImageSize }) if (existingImages.length >= MAX_IMAGES_PER_MESSAGE) { return { text, images: existingImages.slice(0, MAX_IMAGES_PER_MESSAGE) } } diff --git a/src/core/task/__tests__/ask-queued-message-drain.spec.ts b/src/core/task/__tests__/ask-queued-message-drain.spec.ts index b137130174..44cbe6d06e 100644 --- a/src/core/task/__tests__/ask-queued-message-drain.spec.ts +++ b/src/core/task/__tests__/ask-queued-message-drain.spec.ts @@ -41,7 +41,7 @@ describe("Task.ask queued message drain", () => { const askPromise = task.ask("followup", "Q?", false) // Simulate webview queuing the user's selection text while the ask is pending. - ;(task as any).messageQueueService.addMessage("picked answer") + task.messageQueueService.addMessage("picked answer") const result = await askPromise expect(result.response).toBe("messageResponse") @@ -52,7 +52,7 @@ describe("Task.ask queued message drain", () => { const task = await createTask() const askPromise = task.ask("command_output", "command is still running...", false) - ;(task as any).messageQueueService.addMessage("1+1=?") + task.messageQueueService.addMessage("1+1=?") setTimeout(() => { task.approveAsk() @@ -62,8 +62,8 @@ describe("Task.ask queued message drain", () => { expect(result.response).toBe("yesButtonClicked") expect(result.text).toBeUndefined() - expect((task as any).messageQueueService.isEmpty()).toBe(false) - expect((task as any).messageQueueService.messages[0]?.text).toBe("1+1=?") + expect(task.messageQueueService.isEmpty()).toBe(false) + expect(task.messageQueueService.messages[0]?.text).toBe("1+1=?") }) it("does not consume a message already queued before a command_output ask", async () => { diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 381cf0c1e0..d8d45ed1f4 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -831,7 +831,7 @@ }, "core/task/__tests__/ask-queued-message-drain.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 18 + "count": 14 } }, "core/task/__tests__/flushPendingToolResultsToHistory.spec.ts": { @@ -1139,11 +1139,6 @@ "count": 1 } }, - "extension/__tests__/api-send-message.spec.ts": { - "@typescript-eslint/no-explicit-any": { - "count": 7 - } - }, "extension/api.ts": { "@typescript-eslint/no-explicit-any": { "count": 9 diff --git a/src/extension/__tests__/api-send-message.spec.ts b/src/extension/__tests__/api.spec.ts similarity index 54% rename from src/extension/__tests__/api-send-message.spec.ts rename to src/extension/__tests__/api.spec.ts index 23677b1218..6e8427e37e 100644 --- a/src/extension/__tests__/api-send-message.spec.ts +++ b/src/extension/__tests__/api.spec.ts @@ -12,8 +12,8 @@ describe("API - SendMessage Command", () => { let api: API let mockOutputChannel: vscode.OutputChannel let mockProvider: ClineProvider - let mockPostMessageToWebview: ReturnType any>> - let mockLog: ReturnType void>> + let mockPostMessageToWebview: ReturnType> + let mockLog: ReturnType void>> beforeEach(() => { // Setup mocks @@ -21,10 +21,11 @@ describe("API - SendMessage Command", () => { appendLine: vi.fn(), } as unknown as vscode.OutputChannel - mockPostMessageToWebview = vi.fn<(...args: any[]) => any>().mockResolvedValue(undefined) + mockPostMessageToWebview = vi.fn().mockResolvedValue(undefined) mockProvider = { context: {} as vscode.ExtensionContext, + contextProxy: { getValues: vi.fn().mockReturnValue({}) }, postMessageToWebview: mockPostMessageToWebview, on: vi.fn(), getCurrentTaskStack: vi.fn().mockReturnValue([]), @@ -32,12 +33,12 @@ describe("API - SendMessage Command", () => { viewLaunched: true, } as unknown as ClineProvider - mockLog = vi.fn<(...args: any[]) => void>() + mockLog = vi.fn<(message: string) => void>() // Create API instance with logging enabled for testing api = new API(mockOutputChannel, mockProvider, undefined, true) // Override the log method to use our mock - ;(api as any).log = mockLog + Object.defineProperty(api, "log", { value: mockLog }) }) it("should handle SendMessage command with text only", async () => { @@ -56,6 +57,97 @@ describe("API - SendMessage Command", () => { }) }) + it("should enqueue directly when the current task is streaming", async () => { + const addMessage = vi.fn() + const messageText = "Use this before completing" + const images = ["data:image/png;base64,aW1hZ2Ux"] + const currentTask = { + isStreaming: true, + messageQueueService: { addMessage }, + } + mockProvider.getCurrentTask = vi.fn().mockReturnValue(currentTask) + + await api.sendMessage(messageText, images) + + expect(addMessage).toHaveBeenCalledWith(messageText, images) + expect(mockPostMessageToWebview).not.toHaveBeenCalled() + }) + + it("should enqueue image-only input when the current task is streaming", async () => { + const addMessage = vi.fn() + const images = ["data:image/png;base64,aW1hZ2Ux"] + mockProvider.getCurrentTask = vi.fn().mockReturnValue({ + isStreaming: true, + messageQueueService: { addMessage }, + }) + + await api.sendMessage(undefined, images) + + expect(addMessage).toHaveBeenCalledWith("", images) + expect(mockPostMessageToWebview).not.toHaveBeenCalled() + }) + + it("should cap streaming input at 20 images before enqueueing", async () => { + const addMessage = vi.fn() + const images = Array.from( + { length: 21 }, + (_, index) => `data:image/png;base64,${Buffer.from(`image-${index}`).toString("base64")}`, + ) + mockProvider.getCurrentTask = vi.fn().mockReturnValue({ + isStreaming: true, + messageQueueService: { addMessage }, + }) + + await api.sendMessage("Review these", images) + + expect(addMessage).toHaveBeenCalledWith("Review these", images.slice(0, 20)) + }) + + it("should discard malformed streaming image data before enqueueing", async () => { + const addMessage = vi.fn() + mockProvider.getCurrentTask = vi.fn().mockReturnValue({ + isStreaming: true, + messageQueueService: { addMessage }, + }) + + await api.sendMessage("Continue safely", ["not-an-image", "data:image/png;base64,%%%"]) + + expect(addMessage).toHaveBeenCalledWith("Continue safely", []) + }) + + it("should apply configured image size limits before streaming enqueue", async () => { + const addMessage = vi.fn() + mockProvider.contextProxy.getValues = vi.fn().mockReturnValue({ maxImageFileSize: 0 }) + mockProvider.getCurrentTask = vi.fn().mockReturnValue({ + isStreaming: true, + messageQueueService: { addMessage }, + }) + + await api.sendMessage("Continue safely", ["data:image/png;base64,aW1hZ2U="]) + + expect(addMessage).toHaveBeenCalledWith("Continue safely", []) + }) + + it("should retain webview routing when the current task is not streaming", async () => { + const addMessage = vi.fn() + const messageText = "Answer the current ask" + const currentTask = { + isStreaming: false, + messageQueueService: { addMessage }, + } + mockProvider.getCurrentTask = vi.fn().mockReturnValue(currentTask) + + await api.sendMessage(messageText) + + expect(addMessage).not.toHaveBeenCalled() + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "invoke", + invoke: "sendMessage", + text: messageText, + images: undefined, + }) + }) + it("should handle SendMessage command with text and images", async () => { // Arrange const messageText = "Analyze this image" diff --git a/src/extension/api.ts b/src/extension/api.ts index 74ea2e7680..89fbff6aa4 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -26,6 +26,7 @@ import { IpcServer } from "@roo-code/ipc" import { Package } from "../shared/package" import type { Mode } from "../shared/modes" import { ClineProvider } from "../core/webview/ClineProvider" +import { normalizeSuppliedImages } from "../core/mentions/resolveImageMentions" import { Terminal } from "../integrations/terminal/Terminal" import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry" import { openClineInNewTab } from "../activate/registerCommands" @@ -272,6 +273,17 @@ export class API extends EventEmitter implements RooCodeAPI { public async sendMessage(text?: string, images?: string[]) { const currentTask = this.sidebarProvider.getCurrentTask() + // API callers need the returned promise to mean that sequencing-critical + // input has reached the active task. During a stream, the webview would + // only relay this message back as queueMessage asynchronously, so enqueue + // it in the extension host instead of racing task completion. + if (currentTask?.isStreaming) { + const { maxImageFileSize, maxTotalImageSize } = this.sidebarProvider.contextProxy.getValues() + const normalizedImages = normalizeSuppliedImages(images, { maxImageFileSize, maxTotalImageSize }) + currentTask.messageQueueService.addMessage(text ?? "", normalizedImages) + return + } + // In headless/sandbox flows the webview may not be launched, so routing // through invoke=sendMessage drops the message. Deliver directly to the // task ask-response channel instead.