diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-auto-deny.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-auto-deny.spec.ts index 0f3ac24669..72c343c1eb 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-auto-deny.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-auto-deny.spec.ts @@ -77,6 +77,7 @@ interface MockTask { didCompleteReadingStream: boolean didRejectTool: boolean didAlreadyUseTool: boolean + activeToolContexts: Map consecutiveMistakeCount: number clineMessages: unknown[] api: { getModel: () => { id: string; info: Record } } @@ -109,6 +110,7 @@ function buildMockTask(): MockTask { didCompleteReadingStream: true, didRejectTool: false, didAlreadyUseTool: false, + activeToolContexts: new Map(), consecutiveMistakeCount: 0, clineMessages: [], api: { @@ -600,6 +602,7 @@ describe("presentAssistantMessage - automatic (policy) denials", () => { expect(payload.message).toBe("The user denied this operation.") expect(payload.type).toBeUndefined() expect(mockTask.didRejectTool).toBe(true) + expect(mockTask.activeToolContexts.get("call_mcp")).toEqual({ toolCallId: "call_mcp", didReject: true }) expect(mockTask.say).not.toHaveBeenCalledWith("user_feedback", expect.anything(), expect.anything()) }) @@ -664,6 +667,7 @@ describe("presentAssistantMessage - automatic (policy) denials", () => { expect(payload.rule_id).toBe("R-1") expect(mcpApproval).toBe(false) expect(mockTask.didRejectTool).toBe(false) + expect(mockTask.activeToolContexts.get("call_mcp")).toEqual({ toolCallId: "call_mcp", didReject: false }) }) it("denies dangerous_substitution with the fixed shell-expansion reason without aborting the turn", async () => { diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts index 340eb44dd5..ed5b0dd002 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts @@ -75,6 +75,7 @@ describe("presentAssistantMessage - Custom Tool Recording", () => { didCompleteReadingStream: false, didRejectTool: false, didAlreadyUseTool: false, + activeToolContexts: new Map(), consecutiveMistakeCount: 0, clineMessages: [], getTaskMode: vi.fn().mockResolvedValue("code"), diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts index 695c40818c..62f30be12f 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts @@ -50,6 +50,7 @@ describe("presentAssistantMessage - Image Handling in Native Tool Calling", () = didCompleteReadingStream: false, didRejectTool: false, didAlreadyUseTool: false, + activeToolContexts: new Map(), consecutiveMistakeCount: 0, getTaskMode: vi.fn().mockResolvedValue("code"), api: { diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-tool-repetition.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-tool-repetition.spec.ts index cbd3000e78..0121fed928 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-tool-repetition.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-tool-repetition.spec.ts @@ -161,6 +161,7 @@ describe("presentAssistantMessage - Tool Repetition Detection", () => { didCompleteReadingStream: false, didRejectTool: false, didAlreadyUseTool: false, + activeToolContexts: new Map(), consecutiveMistakeCount: 0, consecutiveMistakeLimit: HARD_LIMIT, clineMessages: [], diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts index 00350b79c3..d2e04b89a8 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts @@ -68,6 +68,7 @@ interface MockTask { didCompleteReadingStream: boolean didRejectTool: boolean didAlreadyUseTool: boolean + activeToolContexts: Map consecutiveMistakeCount: number clineMessages: unknown[] getTaskMode: Mock<() => Promise> @@ -107,6 +108,7 @@ describe("presentAssistantMessage - tool usage attribution", () => { didCompleteReadingStream: false, didRejectTool: false, didAlreadyUseTool: false, + activeToolContexts: new Map(), consecutiveMistakeCount: 0, clineMessages: [], getTaskMode: vi.fn().mockResolvedValue("code"), diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts index 75199dc635..157dade953 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts @@ -50,6 +50,7 @@ describe("presentAssistantMessage - Unknown Tool Handling", () => { didCompleteReadingStream: false, didRejectTool: false, didAlreadyUseTool: false, + activeToolContexts: new Map(), consecutiveMistakeCount: 0, clineMessages: [], getTaskMode: vi.fn().mockResolvedValue("code"), diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 5dd14e535b..7b1d2f5737 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -1,3 +1,4 @@ +import { markToolRejected } from "../task/toolExecutionContext" import { serializeError } from "serialize-error" import { Anthropic } from "@anthropic-ai/sdk" @@ -187,6 +188,9 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request // Track if we've already pushed a tool result let hasToolResult = false const toolCallId = mcpBlock.id + if (toolCallId && !cline.activeToolContexts.has(toolCallId)) { + cline.activeToolContexts.set(toolCallId, { toolCallId, didReject: false }) + } // Store approval feedback to merge into tool result (GitHub #10465) let approvalFeedback: { text: string; images?: string[] } | undefined @@ -286,7 +290,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request } else { pushToolResult(formatResponse.toolDenied()) } - cline.didRejectTool = true + markToolRejected(cline, toolCallId ?? "") return false } @@ -411,6 +415,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId: toolCallId ?? "", onValidated: mcpBlock.partial ? undefined : () => { @@ -586,6 +591,10 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request } } + if (!cline.activeToolContexts.has(toolCallId)) { + cline.activeToolContexts.set(toolCallId, { toolCallId, didReject: false }) + } + // Store approval feedback to merge into tool result (GitHub #10465) let approvalFeedback: { text: string; images?: string[] } | undefined @@ -689,7 +698,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request } else { pushToolResult(formatResponse.toolDenied()) } - cline.didRejectTool = true + markToolRejected(cline, toolCallId) return false } @@ -875,6 +884,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "update_todo_list": @@ -882,6 +892,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "apply_diff": @@ -890,6 +901,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "edit": @@ -899,6 +911,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "search_replace": @@ -907,6 +920,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "edit_file": @@ -915,6 +929,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "apply_patch": @@ -923,6 +938,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "read_file": @@ -931,6 +947,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "list_files": @@ -938,6 +955,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "codebase_search": @@ -945,6 +963,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "search_files": @@ -952,6 +971,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "execute_command": @@ -959,6 +979,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "read_command_output": @@ -966,6 +987,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "use_mcp_tool": @@ -973,6 +995,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "access_mcp_resource": @@ -980,6 +1003,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "ask_followup_question": @@ -987,6 +1011,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "switch_mode": @@ -994,6 +1019,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "new_task": @@ -1002,7 +1028,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, - toolCallId: block.id, + toolCallId, }) break case "attempt_completion": { @@ -1012,7 +1038,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request pushToolResult, askFinishSubTaskApproval, toolDescription, - toolCallId: block.id, + toolCallId, } await attemptCompletionTool.handle( cline, @@ -1026,6 +1052,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "skill": @@ -1033,6 +1060,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break case "generate_image": @@ -1041,6 +1069,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request askApproval, handleError, pushToolResult, + toolCallId, }) break default: { diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index b84a449c1d..e1e5720026 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -241,6 +241,11 @@ function queuedResponseForAsk(type: ClineAsk, text?: string): QueuedAskResolutio const FORCED_CONTEXT_REDUCTION_PERCENT = 75 // Keep 75% of context (remove 25%) on context window errors const MAX_CONTEXT_WINDOW_RETRIES = 3 // Maximum retries for context window errors +export interface ToolExecutionContext { + toolCallId: string + didReject: boolean +} + export interface TaskOptions extends CreateTaskOptions { provider: ClineProvider apiConfiguration: ProviderSettings @@ -420,7 +425,8 @@ export class Task extends EventEmitter implements TaskLike { rooIgnoreController?: RooIgnoreController rooProtectedController?: RooProtectedController fileContextTracker: FileContextTracker - terminalProcess?: RooTerminalProcess + terminalProcesses = new Map() + activeToolContexts = new Map() // Editing diffViewProvider: DiffViewProvider @@ -2535,9 +2541,9 @@ export class Task extends EventEmitter implements TaskLike { async handleTerminalOperation(terminalOperation: "continue" | "abort") { if (terminalOperation === "continue") { - this.terminalProcess?.continue() + for (const process of this.terminalProcesses.values()) process.continue() } else if (terminalOperation === "abort") { - this.terminalProcess?.abort() + for (const process of this.terminalProcesses.values()) process.abort() } } @@ -4021,6 +4027,7 @@ export class Task extends EventEmitter implements TaskLike { this.didCompleteReadingStream = false this.userMessageContent = [] this.userMessageContentReady = false + this.activeToolContexts.clear() this.didRejectTool = false this.didAlreadyUseTool = false this.assistantMessageSavedToHistory = false diff --git a/src/core/task/__tests__/Task.terminal-operation.spec.ts b/src/core/task/__tests__/Task.terminal-operation.spec.ts new file mode 100644 index 0000000000..d5ab64b0dc --- /dev/null +++ b/src/core/task/__tests__/Task.terminal-operation.spec.ts @@ -0,0 +1,38 @@ +import { Task } from "../Task" + +function buildTask() { + const task = Object.create(Task.prototype) as Task + task.terminalProcesses = new Map() + return task +} + +function stubProcess() { + return { continue: vi.fn(), abort: vi.fn() } +} + +describe("Task.handleTerminalOperation", () => { + it.each([ + ["continue", "abort"], + ["abort", "continue"], + ] as const)("%s reaches every running terminal process exactly once", async (operation, other) => { + const task = buildTask() + const first = stubProcess() + const second = stubProcess() + task.terminalProcesses.set("call_a", first as never) + task.terminalProcesses.set("call_b", second as never) + + await task.handleTerminalOperation(operation) + + expect(first[operation]).toHaveBeenCalledTimes(1) + expect(second[operation]).toHaveBeenCalledTimes(1) + expect(first[other]).not.toHaveBeenCalled() + expect(second[other]).not.toHaveBeenCalled() + }) + + it.each(["continue", "abort"] as const)( + "%s does nothing when no terminal process is running", + async (operation) => { + await expect(buildTask().handleTerminalOperation(operation)).resolves.toBeUndefined() + }, + ) +}) diff --git a/src/core/task/__tests__/ToolExecutionContext.spec.ts b/src/core/task/__tests__/ToolExecutionContext.spec.ts new file mode 100644 index 0000000000..f466c05596 --- /dev/null +++ b/src/core/task/__tests__/ToolExecutionContext.spec.ts @@ -0,0 +1,180 @@ +// npx vitest run core/task/__tests__/ToolExecutionContext.spec.ts + +import type { Anthropic } from "@anthropic-ai/sdk" +import { describe, it, expect, beforeEach, vi } from "vitest" +import { presentAssistantMessage } from "../../assistant-message/presentAssistantMessage" +import { validateToolUse } from "../../tools/validateToolUse" +import type { Task, ToolExecutionContext } from "../Task" +import type { RequestPolicySnapshot } from "../../prompts/tools/effective-tool-policy" + +vi.mock("../Task") +vi.mock("../../tools/validateToolUse", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, validateToolUse: vi.fn() } +}) + +vi.mock("@roo-code/core", () => ({ + customToolRegistry: { has: vi.fn(() => false), get: vi.fn() }, +})) + +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureToolUsage: vi.fn(), + captureConsecutiveMistakeError: vi.fn(), + captureException: vi.fn(), + captureEvent: vi.fn(), + }, + }, +})) + +const { listFilesHandle } = vi.hoisted(() => ({ listFilesHandle: vi.fn() })) + +vi.mock("../../tools/ListFilesTool", () => ({ + listFilesTool: { handle: listFilesHandle }, +})) + +type ApprovalCallbacks = { + askApproval: (type: string, message?: string) => Promise + toolCallId: string +} + +function buildMockTask() { + const mockTask = { + taskId: "test-task-id", + instanceId: "test-instance", + abort: false, + presentAssistantMessageLocked: false, + presentAssistantMessageHasPendingUpdates: false, + currentStreamingContentIndex: 0, + assistantMessageContent: [] as unknown[], + userMessageContent: [] as Anthropic.ToolResultBlockParam[], + didCompleteReadingStream: true, + didRejectTool: false, + didAlreadyUseTool: false, + activeToolContexts: new Map(), + consecutiveMistakeCount: 0, + clineMessages: [], + api: { getModel: () => ({ id: "test-model", info: {} }) }, + apiConfiguration: { apiProvider: "test" }, + recordToolUsage: vi.fn(), + recordToolError: vi.fn(), + toolRepetitionDetector: { check: vi.fn().mockReturnValue({ allowExecution: true }) }, + providerRef: { + deref: () => ({ getState: vi.fn().mockResolvedValue({ mode: "code", customModes: [] }) }), + }, + say: vi.fn().mockResolvedValue(undefined), + ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), + getTaskMode: vi.fn().mockResolvedValue("code"), + pushToolResultToUserContent: vi.fn(), + } + + mockTask.pushToolResultToUserContent.mockImplementation((toolResult: Anthropic.ToolResultBlockParam) => { + mockTask.userMessageContent.push(toolResult) + return true + }) + + return mockTask +} + +const listFilesBlock = (id: string) => ({ + type: "tool_use", + id, + name: "list_files", + params: { path: "src" }, + nativeArgs: { path: "src" }, + partial: false, +}) + +const baseSnapshot: RequestPolicySnapshot = { + disabledTools: [], + customModes: [], +} + +const asTask = (task: ReturnType): Task => task as unknown as Task + +describe("ToolExecutionContext", () => { + let mockTask: ReturnType + let contextsSeenDuringExecution: Map + + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(validateToolUse).mockImplementation(() => undefined) + mockTask = buildMockTask() + contextsSeenDuringExecution = new Map() + + listFilesHandle.mockImplementation( + async (task: Task, _block: unknown, { askApproval, toolCallId }: ApprovalCallbacks) => { + contextsSeenDuringExecution.set(toolCallId, task.activeToolContexts.get(toolCallId)) + await askApproval("tool", JSON.stringify({ tool: "listFilesTopLevel", path: "src" })) + }, + ) + }) + + it("creates a context keyed by the tool call id that stays readable after the tool pushes its result", async () => { + mockTask.assistantMessageContent = [listFilesBlock("call_a")] + listFilesHandle.mockImplementationOnce( + async ( + task: Task, + _block: unknown, + { + askApproval, + toolCallId, + pushToolResult, + }: ApprovalCallbacks & { pushToolResult: (c: string) => void }, + ) => { + contextsSeenDuringExecution.set(toolCallId, task.activeToolContexts.get(toolCallId)) + await askApproval("tool", "{}") + pushToolResult("done") + }, + ) + + await presentAssistantMessage(asTask(mockTask), baseSnapshot) + + expect(contextsSeenDuringExecution.get("call_a")).toBe(mockTask.activeToolContexts.get("call_a")) + expect(mockTask.activeToolContexts.get("call_a")).toEqual({ + toolCallId: "call_a", + didReject: false, + }) + expect(mockTask.didRejectTool).toBe(false) + }) + + it("reuses the context when a partial tool call is followed by its complete block", async () => { + mockTask.assistantMessageContent = [{ ...listFilesBlock("call_a"), partial: true }] + await presentAssistantMessage(asTask(mockTask), baseSnapshot) + const first = mockTask.activeToolContexts.get("call_a") + + mockTask.assistantMessageContent = [listFilesBlock("call_a")] + await presentAssistantMessage(asTask(mockTask), baseSnapshot) + + expect(first).toBeDefined() + expect(mockTask.activeToolContexts.get("call_a")).toBe(first) + }) + + it("sets didReject on the rejected tool's context only, and the serial flag keeps skipping the sibling", async () => { + mockTask.assistantMessageContent = [listFilesBlock("call_a"), listFilesBlock("call_b")] + mockTask.activeToolContexts.set("call_a", { toolCallId: "call_a", didReject: false }) + mockTask.activeToolContexts.set("call_b", { toolCallId: "call_b", didReject: false }) + mockTask.ask.mockResolvedValueOnce({ response: "noButtonClicked" }) + + await presentAssistantMessage(asTask(mockTask), baseSnapshot) + mockTask.presentAssistantMessageLocked = false + mockTask.currentStreamingContentIndex = 1 + await presentAssistantMessage(asTask(mockTask), baseSnapshot) + + expect(mockTask.didRejectTool).toBe(true) + expect(listFilesHandle).toHaveBeenCalledTimes(1) + expect(mockTask.activeToolContexts.get("call_a")?.didReject).toBe(true) + expect(mockTask.activeToolContexts.get("call_b")?.didReject).toBe(false) + }) + + it("keeps the serial didRejectTool flag in sync with a rejection", async () => { + mockTask.assistantMessageContent = [listFilesBlock("call_a")] + mockTask.ask.mockResolvedValue({ response: "noButtonClicked" }) + + await presentAssistantMessage(asTask(mockTask), baseSnapshot) + + expect(mockTask.didRejectTool).toBe(true) + expect(mockTask.activeToolContexts.get("call_a")?.didReject).toBe(true) + }) +}) diff --git a/src/core/task/__tests__/ask-auto-deny.spec.ts b/src/core/task/__tests__/ask-auto-deny.spec.ts index 4254791025..4930668c46 100644 --- a/src/core/task/__tests__/ask-auto-deny.spec.ts +++ b/src/core/task/__tests__/ask-auto-deny.spec.ts @@ -79,6 +79,7 @@ function attachTurnHarness(task: Task) { Object.defineProperty(task, "rateLimitClock", { value: createRateLimitClock() }) Object.defineProperty(task, "diffViewProvider", { value: { isEditing: false, reset: async () => {} } }) Object.defineProperty(task, "streamingToolCallIndices", { value: new Map() }) + Object.defineProperty(task, "activeToolContexts", { value: new Map() }) task["saveApiConversationHistory"] = vi.fn(async () => true) task["say"] = vi.fn(async () => undefined) // The loop rewrites the newest api_req_started row with cost data; the diff --git a/src/core/task/toolExecutionContext.ts b/src/core/task/toolExecutionContext.ts new file mode 100644 index 0000000000..492410c4a9 --- /dev/null +++ b/src/core/task/toolExecutionContext.ts @@ -0,0 +1,7 @@ +import type { Task } from "./Task" + +export function markToolRejected(task: Task, toolCallId: string): void { + task.didRejectTool = true + const context = task.activeToolContexts.get(toolCallId) + if (context) context.didReject = true +} diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 83a733c7b0..35e7f14f83 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -10,7 +10,7 @@ export interface ToolCallbacks { askApproval: AskApproval handleError: HandleError pushToolResult: PushToolResult - toolCallId?: string + toolCallId: string } /** diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index a6034e4f4d..863dd7d6c7 100644 --- a/src/core/tools/ExecuteCommandTool.ts +++ b/src/core/tools/ExecuteCommandTool.ts @@ -1,3 +1,4 @@ +import { markToolRejected } from "../task/toolExecutionContext" import fs from "fs/promises" import * as path from "path" import * as vscode from "vscode" @@ -89,7 +90,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { async execute(params: ExecuteCommandParams, task: Task, callbacks: ToolCallbacks): Promise { const { command, cwd: customCwd, timeout: timeoutSeconds } = params - const { handleError, pushToolResult, askApproval } = callbacks + const { handleError, pushToolResult, askApproval, toolCallId } = callbacks try { if (!command) { @@ -300,13 +301,14 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { terminalShellIntegrationDisabled, commandExecutionTimeout, agentTimeout, + toolCallId, } try { const [rejected, result] = await executeCommandInTerminal(task, options) if (rejected) { - task.didRejectTool = true + markToolRejected(task, toolCallId) } pushToolResult(result) @@ -325,7 +327,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { }) if (rejected) { - task.didRejectTool = true + markToolRejected(task, toolCallId) } pushToolResult(result) @@ -363,6 +365,7 @@ export type ExecuteCommandOptions = { terminalShellIntegrationDisabled?: boolean commandExecutionTimeout?: number agentTimeout?: number + toolCallId: string } export async function executeCommandInTerminal( @@ -374,6 +377,7 @@ export async function executeCommandInTerminal( terminalShellIntegrationDisabled = true, commandExecutionTimeout = 0, agentTimeout = 0, + toolCallId, }: ExecuteCommandOptions, ): Promise<[boolean, ToolResponse]> { // Convert milliseconds back to seconds for display purposes. @@ -575,7 +579,7 @@ export async function executeCommandInTerminal( } const process = terminal.runCommand(command, callbacks) - task.terminalProcess = process + task.terminalProcesses.set(toolCallId, process) // Dual-timeout logic: // - Agent timeout: transitions the command to background (continues running) @@ -609,7 +613,7 @@ export async function executeCommandInTerminal( new Promise((_, reject) => { userTimeoutId = setTimeout(() => { isUserTimedOut = true - task.terminalProcess?.abort() + process.abort() reject(new Error(`Command execution timed out after ${commandExecutionTimeout}ms`)) }, commandExecutionTimeout) }), @@ -623,8 +627,6 @@ export async function executeCommandInTerminal( postCommandExecutionStatus(provider, status) await task.say("error", t("common:errors:command_timeout", { seconds: commandExecutionTimeoutSeconds })) task.didToolFailInCurrentTurn = true - task.terminalProcess = undefined - return [ false, `The command was terminated after exceeding a user-configured ${commandExecutionTimeoutSeconds}s timeout. Do not try to re-run the command.`, @@ -635,7 +637,7 @@ export async function executeCommandInTerminal( clearTimeout(agentTimeoutId) clearTimeout(userTimeoutId) clearTimeout(pendingCommandOutputEmitTimer) - task.terminalProcess = undefined + task.terminalProcesses.delete(toolCallId) } if (shellIntegrationError) { diff --git a/src/core/tools/ReadFileTool.ts b/src/core/tools/ReadFileTool.ts index 00fc507692..cf558ad634 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/src/core/tools/ReadFileTool.ts @@ -8,6 +8,7 @@ * Also supports legacy format for backward compatibility: * - Legacy format: { files: [{ path: string, lineRanges?: [...] }] } */ +import { markToolRejected } from "../task/toolExecutionContext" import path from "path" import * as fs from "fs/promises" import { isBinaryFile } from "isbinaryfile" @@ -84,7 +85,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { * Execute new single-file format with slice/indentation mode support. */ private async executeNew(params: ReadFileParams, task: Task, callbacks: ToolCallbacks): Promise { - const { pushToolResult } = callbacks + const { pushToolResult, toolCallId } = callbacks const modelInfo = task.api.getModel().info const filePath = params.path @@ -163,7 +164,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { } // Phase 2: Request user approval - await this.requestApproval(task, filesToApprove, updateFileResult) + await this.requestApproval(task, toolCallId, filesToApprove, updateFileResult) // Phase 3: Process approved files const imageMemoryTracker = new ImageMemoryTracker() @@ -426,6 +427,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { */ private async requestApproval( task: Task, + toolCallId: string, filesToApprove: FileResult[], updateFileResult: (path: string, updates: Partial) => void, ): Promise { @@ -455,7 +457,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { }) } else if (response === "noButtonClicked") { if (text) await task.say("user_feedback", text, images) - task.didRejectTool = true + markToolRejected(task, toolCallId) filesToApprove.forEach((fr) => { updateFileResult(fr.path, { status: "denied", @@ -485,9 +487,9 @@ export class ReadFileTool extends BaseTool<"read_file"> { } }) - if (hasAnyDenial) task.didRejectTool = true + if (hasAnyDenial) markToolRejected(task, toolCallId) } catch { - task.didRejectTool = true + markToolRejected(task, toolCallId) filesToApprove.forEach((fr) => { updateFileResult(fr.path, { status: "denied", @@ -519,7 +521,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { if (response !== "yesButtonClicked") { if (text) await task.say("user_feedback", text, images) - task.didRejectTool = true + markToolRejected(task, toolCallId) updateFileResult(relPath, { status: "denied", nativeContent: `File: ${relPath}\nStatus: Denied by user`, @@ -665,7 +667,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { * This handles the old format: { files: [{ path: string, lineRanges?: [...] }] } */ private async executeLegacy(fileEntries: FileEntry[], task: Task, callbacks: ToolCallbacks): Promise { - const { pushToolResult } = callbacks + const { pushToolResult, toolCallId } = callbacks const modelInfo = task.api.getModel().info if (!fileEntries || fileEntries.length === 0) { @@ -716,7 +718,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { if (response !== "yesButtonClicked") { if (text) await task.say("user_feedback", text, images) - task.didRejectTool = true + markToolRejected(task, toolCallId) results.push(`File: ${relPath}\nStatus: Denied by user`) continue } diff --git a/src/core/tools/__tests__/CodebaseSearchTool.spec.ts b/src/core/tools/__tests__/CodebaseSearchTool.spec.ts index bc88d44ec1..eb0c27c1ae 100644 --- a/src/core/tools/__tests__/CodebaseSearchTool.spec.ts +++ b/src/core/tools/__tests__/CodebaseSearchTool.spec.ts @@ -67,6 +67,7 @@ describe("CodebaseSearchTool", () => { askApproval: vi.fn().mockResolvedValue(true), handleError: vi.fn().mockResolvedValue(undefined), pushToolResult: vi.fn(), + toolCallId: "test-tool-call-id", } manager = { isConfigurationLoaded: true, diff --git a/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts b/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts index f2c4cc2aed..f6f6c0f5cb 100644 --- a/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts +++ b/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts @@ -86,6 +86,7 @@ describe("CodebaseSearchTool workspace selection", () => { askApproval: vi.fn().mockResolvedValue(true), handleError: vi.fn().mockResolvedValue(undefined), pushToolResult: vi.fn(), + toolCallId: "test-tool-call-id", } }) diff --git a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts index 72ffb112bc..eb89838cad 100644 --- a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts +++ b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts @@ -81,6 +81,7 @@ describe("ApplyPatchTool.execute - delete file success path", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockAskApproval).toHaveBeenCalled() diff --git a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts index f7114eb3d6..965cb23e19 100644 --- a/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts +++ b/src/core/tools/__tests__/askFollowupQuestionTool.spec.ts @@ -30,6 +30,7 @@ describe("AskFollowupQuestionTool", () => { askApproval: vi.fn().mockResolvedValue(true), handleError: vi.fn(), pushToolResult: vi.fn(), + toolCallId: "test-tool-call-id", } }) diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 4bf3a89f34..c1c235fe7f 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -96,6 +96,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -121,6 +122,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -150,6 +152,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -189,6 +192,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -231,6 +235,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -274,6 +279,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -316,6 +322,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -359,6 +366,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -402,6 +410,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -432,6 +441,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -466,6 +476,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -805,6 +816,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -855,6 +867,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -907,6 +920,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -936,6 +950,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -972,6 +987,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, }) @@ -1001,6 +1017,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, }) @@ -1033,6 +1050,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, } @@ -1073,6 +1091,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, }) @@ -1104,6 +1123,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, }) @@ -1132,6 +1152,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, }) @@ -1173,6 +1194,7 @@ describe("attemptCompletionTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", askFinishSubTaskApproval: mockAskFinishSubTaskApproval, toolDescription: mockToolDescription, }) @@ -1238,6 +1260,7 @@ describe("attemptCompletionTool telemetry invariants", () => { askApproval: vi.fn(), handleError: vi.fn(), pushToolResult: vi.fn(), + toolCallId: "", askFinishSubTaskApproval: vi.fn(), toolDescription: vi.fn(), } as AttemptCompletionCallbacks) @@ -1273,6 +1296,7 @@ describe("attemptCompletionTool telemetry invariants", () => { askApproval: vi.fn(), handleError: vi.fn(), pushToolResult: vi.fn(), + toolCallId: "", askFinishSubTaskApproval: vi.fn(), toolDescription: vi.fn(), } as AttemptCompletionCallbacks) @@ -1302,6 +1326,7 @@ describe("attemptCompletionTool telemetry invariants", () => { askApproval: vi.fn(), handleError: vi.fn(), pushToolResult: vi.fn(), + toolCallId: "", askFinishSubTaskApproval: vi.fn(), toolDescription: vi.fn(), } as AttemptCompletionCallbacks) @@ -1333,6 +1358,7 @@ describe("attemptCompletionTool telemetry invariants", () => { askApproval: vi.fn(), handleError: vi.fn(), pushToolResult: vi.fn(), + toolCallId: "", askFinishSubTaskApproval: vi.fn(), toolDescription: vi.fn(), } as AttemptCompletionCallbacks) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 1ff8d52a8d..c8dbf44112 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -213,6 +213,7 @@ describe("editFileTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) return toolResult @@ -290,6 +291,7 @@ describe("editFileTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: localPushToolResult, + toolCallId: "test-tool-call-id", }) return capturedResult @@ -661,6 +663,7 @@ describe("editFileTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: localPushToolResult, + toolCallId: "test-tool-call-id", }) expect(capturedResult).toContain("Failed to read file") diff --git a/src/core/tools/__tests__/editTool.spec.ts b/src/core/tools/__tests__/editTool.spec.ts index a5f665b9e5..fbb14d7ed2 100644 --- a/src/core/tools/__tests__/editTool.spec.ts +++ b/src/core/tools/__tests__/editTool.spec.ts @@ -218,6 +218,7 @@ describe("editTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) return toolResult @@ -400,6 +401,7 @@ describe("editTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: localPushToolResult, + toolCallId: "test-tool-call-id", }) expect(capturedResult).toContain("Error:") diff --git a/src/core/tools/__tests__/executeCommand.spec.ts b/src/core/tools/__tests__/executeCommand.spec.ts index 7146fa930b..c288330887 100644 --- a/src/core/tools/__tests__/executeCommand.spec.ts +++ b/src/core/tools/__tests__/executeCommand.spec.ts @@ -52,7 +52,7 @@ describe("executeCommand", () => { deref: vitest.fn().mockResolvedValue(mockProvider), }, say: vitest.fn().mockResolvedValue(undefined), - terminalProcess: undefined, + terminalProcesses: new Map(), } // Create mock process that resolves immediately @@ -103,6 +103,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "echo test", terminalShellIntegrationDisabled: false, } @@ -143,6 +144,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "echo test", terminalShellIntegrationDisabled: false, } @@ -175,6 +177,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "echo test", terminalShellIntegrationDisabled: true, // Forces ExecaTerminal } @@ -204,6 +207,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "echo test", customCwd, terminalShellIntegrationDisabled: false, @@ -233,6 +237,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "echo test", customCwd: relativeCwd, terminalShellIntegrationDisabled: false, @@ -255,6 +260,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "echo test", customCwd: nonExistentCwd, terminalShellIntegrationDisabled: false, @@ -283,6 +289,7 @@ describe("executeCommand", () => { await executeCommandInTerminal(mockTask, { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "echo test", terminalShellIntegrationDisabled: false, }) @@ -306,6 +313,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "echo test", terminalShellIntegrationDisabled: false, } @@ -328,6 +336,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "echo test", terminalShellIntegrationDisabled: true, } @@ -341,6 +350,48 @@ describe("executeCommand", () => { }) describe("Command Execution States", () => { + it("keeps a separate terminal process per tool call id and aborts only the timed-out one", async () => { + vitest.useFakeTimers() + const makePendingProcess = () => + Object.assign(new Promise(() => {}), { continue: vitest.fn(), abort: vitest.fn() }) + const processA = makePendingProcess() + const processB = makePendingProcess() + mockTerminal.runCommand.mockReturnValueOnce(processA).mockReturnValueOnce(processB) + + const executionA = executeCommandInTerminal(mockTask, { + executionId: "exec-a", + toolCallId: "call-a", + command: "sleep 10", + terminalShellIntegrationDisabled: false, + commandExecutionTimeout: 1_000, + }) + await vitest.advanceTimersByTimeAsync(0) + const executionB = executeCommandInTerminal(mockTask, { + executionId: "exec-b", + toolCallId: "call-b", + command: "sleep 20", + terminalShellIntegrationDisabled: false, + commandExecutionTimeout: 5_000, + }) + await vitest.advanceTimersByTimeAsync(0) + + expect(mockTask.terminalProcesses.get("call-a")).toBe(processA) + expect(mockTask.terminalProcesses.get("call-b")).toBe(processB) + + await vitest.advanceTimersByTimeAsync(1_000) + await executionA + + expect(processA.abort).toHaveBeenCalled() + expect(processB.abort).not.toHaveBeenCalled() + expect(mockTask.terminalProcesses.has("call-a")).toBe(false) + expect(mockTask.terminalProcesses.get("call-b")).toBe(processB) + + await vitest.advanceTimersByTimeAsync(4_000) + await executionB + + expect(mockTask.terminalProcesses.size).toBe(0) + }) + it("posts timeout status when command execution exceeds the user limit", async () => { vitest.useFakeTimers() const pendingProcess = Object.assign(new Promise(() => {}), { @@ -351,6 +402,7 @@ describe("executeCommand", () => { const executionPromise = executeCommandInTerminal(mockTask, { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "sleep 10", terminalShellIntegrationDisabled: false, commandExecutionTimeout: 1_000, @@ -381,6 +433,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "echo success", terminalShellIntegrationDisabled: false, } @@ -406,6 +459,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "exit 1", terminalShellIntegrationDisabled: false, } @@ -439,6 +493,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "long-running-command", terminalShellIntegrationDisabled: false, } @@ -480,6 +535,7 @@ describe("executeCommand", () => { const options: ExecuteCommandOptions = { executionId: "test-123", + toolCallId: "test-tool-call-id", command: "cd src && pwd", terminalShellIntegrationDisabled: false, } diff --git a/src/core/tools/__tests__/executeCommandTool.spec.ts b/src/core/tools/__tests__/executeCommandTool.spec.ts index 98640fe969..6fd521b4eb 100644 --- a/src/core/tools/__tests__/executeCommandTool.spec.ts +++ b/src/core/tools/__tests__/executeCommandTool.spec.ts @@ -105,6 +105,7 @@ describe("executeCommandTool", () => { recordToolUsage: vitest.fn().mockReturnValue({} as ToolUsage), recordToolError: vitest.fn(), supersedePendingAsk: vitest.fn(), + terminalProcesses: new Map(), recordBlanketCommandDenial: vitest.fn(), providerRef: { deref: vitest.fn().mockResolvedValue({ @@ -198,6 +199,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) // Verify @@ -219,6 +221,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) // Verify - command approved, result pushed, and custom cwd passed to terminal @@ -240,6 +243,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) expect(provider.postMessageToWebview).toHaveBeenCalledWith( @@ -284,6 +288,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) expect(provider.postMessageToWebview).toHaveBeenCalledWith( @@ -327,6 +332,7 @@ describe("executeCommandTool", () => { 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.say).toHaveBeenCalledWith( @@ -350,6 +356,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) // The DCG verdict is forwarded so checkAutoApproval auto-approves from @@ -378,6 +385,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) // Blanket mode: not protected, so checkAutoApproval resolves the ask as @@ -405,6 +413,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockAskApproval).toHaveBeenCalledWith("command", "echo test", undefined, true) @@ -428,6 +437,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockAskApproval).toHaveBeenCalledWith("command", "echo test", undefined, true) @@ -455,6 +465,7 @@ describe("executeCommandTool", () => { handleError: mockHandleError as unknown as HandleError, // vi.fn stands in for the PushToolResult signature; every test in this block uses this identical cast. pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockAskApproval).toHaveBeenCalledWith("command", "echo test", undefined, true) @@ -495,6 +506,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // Position pin: the denial must arrive after an ask actually happened — @@ -545,6 +557,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // Same position pin as the sibling test: asked first, denied by the re-check. @@ -600,6 +613,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // The prompt shown was the protected one — the blanket setting was off @@ -632,6 +646,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(recheckSpy).toHaveBeenCalledTimes(1) @@ -654,6 +669,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(TerminalRegistry.getOrCreateTerminal).not.toHaveBeenCalled() @@ -691,6 +707,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // The re-check genuinely ran against the fresh state — without this @@ -725,6 +742,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockEnsureDcgInstalled).toHaveBeenCalledWith("/test/storage") @@ -745,6 +763,7 @@ describe("executeCommandTool", () => { 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( @@ -770,6 +789,7 @@ describe("executeCommandTool", () => { 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( @@ -792,6 +812,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) // Verify @@ -813,6 +834,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) // Verify @@ -839,6 +861,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) // Verify @@ -972,6 +995,7 @@ describe("executeCommandTool", () => { askApproval: mockAskApproval as unknown as AskApproval, handleError: mockHandleError as unknown as HandleError, pushToolResult: mockPushToolResult as unknown as PushToolResult, + toolCallId: "test-tool-call-id", }) } diff --git a/src/core/tools/__tests__/generateImageTool.test.ts b/src/core/tools/__tests__/generateImageTool.test.ts index d55d54c6e8..5c4a9d7f72 100644 --- a/src/core/tools/__tests__/generateImageTool.test.ts +++ b/src/core/tools/__tests__/generateImageTool.test.ts @@ -89,6 +89,7 @@ describe("generateImageTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // Should not process anything when partial @@ -118,6 +119,7 @@ describe("generateImageTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // Should not process anything when partial @@ -158,6 +160,7 @@ describe("generateImageTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // Should process the complete block @@ -204,6 +207,7 @@ describe("generateImageTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // Check that cline.say was called with image data containing cache-busting parameter @@ -242,6 +246,7 @@ describe("generateImageTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockCline.consecutiveMistakeCount).toBe(1) @@ -267,6 +272,7 @@ describe("generateImageTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockCline.consecutiveMistakeCount).toBe(1) @@ -303,6 +309,7 @@ describe("generateImageTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockPushToolResult).toHaveBeenCalledWith( @@ -337,6 +344,7 @@ describe("generateImageTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("Input image not found")) @@ -364,6 +372,7 @@ describe("generateImageTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("Unsupported image format")) @@ -399,6 +408,7 @@ describe("generateImageTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) const expectedMessage = t("tools:generateImage.openRouterApiKeyRequired") diff --git a/src/core/tools/__tests__/listFilesTool.spec.ts b/src/core/tools/__tests__/listFilesTool.spec.ts index 6306e170ca..b9854d3cf6 100644 --- a/src/core/tools/__tests__/listFilesTool.spec.ts +++ b/src/core/tools/__tests__/listFilesTool.spec.ts @@ -72,6 +72,7 @@ describe("ListFilesTool", () => { askApproval: vi.fn().mockResolvedValue(true), handleError: vi.fn(), pushToolResult: vi.fn(), + toolCallId: "test-tool-call-id", } // Default mock implementations diff --git a/src/core/tools/__tests__/newTaskTool.spec.ts b/src/core/tools/__tests__/newTaskTool.spec.ts index b7872cb714..3c4e502c90 100644 --- a/src/core/tools/__tests__/newTaskTool.spec.ts +++ b/src/core/tools/__tests__/newTaskTool.spec.ts @@ -157,6 +157,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) // Verify askApproval was called @@ -192,6 +193,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) expect(mockStartSubtask).toHaveBeenCalledWith( @@ -217,6 +219,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) expect(mockStartSubtask).toHaveBeenCalledWith( @@ -242,6 +245,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) expect(mockStartSubtask).toHaveBeenCalledWith( @@ -267,6 +271,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) // Should NOT error when todos is missing @@ -297,6 +302,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) // Should parse and include todos when provided @@ -328,6 +334,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) expect(mockSayAndCreateMissingParamError).toHaveBeenCalledWith("new_task", "mode") @@ -351,6 +358,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) expect(mockSayAndCreateMissingParamError).toHaveBeenCalledWith("new_task", "message") @@ -374,6 +382,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) expect(mockStartSubtask).toHaveBeenCalledWith( @@ -410,6 +419,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) // Should NOT error when todos is missing and setting is disabled @@ -446,6 +456,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) // Should error when todos is missing and setting is enabled @@ -482,6 +493,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) // Should NOT error when todos is provided and setting is enabled @@ -524,6 +536,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) // Should NOT error when todos is empty string and setting is enabled @@ -558,6 +571,7 @@ describe("newTaskTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "", }) // Verify that VSCode configuration was accessed with Package.name diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 5f9033eb7c..b16c18be97 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -15,7 +15,7 @@ import path from "path" import type { ToolUse } from "../../../shared/tools" -import type { Task } from "../../task/Task" +import type { Task, ToolExecutionContext } from "../../task/Task" import { isBinaryFile } from "isbinaryfile" @@ -154,6 +154,7 @@ function createMockTask(options: MockTaskOptions = {}) { consecutiveMistakeCount: 0, didToolFailInCurrentTurn: false, didRejectTool: false, + activeToolContexts: new Map(), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: undefined, images: undefined }), say: vi.fn().mockResolvedValue(undefined), sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing required parameter: path"), @@ -178,6 +179,7 @@ function createMockTask(options: MockTaskOptions = {}) { function createMockCallbacks() { return { pushToolResult: vi.fn(), + toolCallId: "test-tool-call-id", askApproval: vi.fn(), handleError: vi.fn(), } @@ -605,11 +607,14 @@ describe("ReadFileTool", () => { const mockTask = createMockTask() const callbacks = createMockCallbacks() + const context = { toolCallId: callbacks.toolCallId, didReject: false } + mockTask.activeToolContexts.set(callbacks.toolCallId, context) mockTask.ask.mockResolvedValue({ response: "noButtonClicked", text: undefined, images: undefined }) await readFileTool.execute({ path: "test.ts" }, mockTask as any, callbacks) expect(mockTask.didRejectTool).toBe(true) + expect(context.didReject).toBe(true) expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("Denied by user")) }) @@ -798,6 +803,73 @@ describe("ReadFileTool", () => { expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("file2 content")) }) + describe("batch approval denial", () => { + const requestBatchApproval = async (answer: { + response: string + text?: string + approvedIndexes?: number[] + }) => { + const mockTask = createMockTask() + const context = { toolCallId: "batch-call", didReject: false } + mockTask.activeToolContexts.set("batch-call", context) + mockTask.ask.mockImplementation(async (_type: string, message: string) => { + if (!answer.approvedIndexes) return { response: answer.response, text: answer.text } + const { batchFiles } = JSON.parse(message) as { batchFiles: { key: string }[] } + const permissions = Object.fromEntries( + batchFiles.map((file, index) => [file.key, answer.approvedIndexes!.includes(index)]), + ) + return { response: answer.response, text: JSON.stringify(permissions) } + }) + const updateFileResult = vi.fn() + const filesToApprove = ["file1.ts", "file2.ts"].map((path) => ({ + path, + status: "pending" as const, + entry: { path }, + })) + + await readFileTool["requestApproval"](mockTask as never, "batch-call", filesToApprove, updateFileResult) + + return { mockTask, context, updateFileResult } + } + + it.each([ + ["the user clicks no", { response: "noButtonClicked" }], + ["individual permissions deny every file", { response: "messageResponse", text: "{}" }], + ["individual permissions are not valid JSON", { response: "messageResponse", text: "not json" }], + ])("marks the tool call rejected and denies both files when %s", async (_name, askResult) => { + const { mockTask, context, updateFileResult } = await requestBatchApproval(askResult) + + expect(mockTask.didRejectTool).toBe(true) + expect(context.didReject).toBe(true) + expect(updateFileResult).toHaveBeenCalledWith("file1.ts", expect.objectContaining({ status: "denied" })) + expect(updateFileResult).toHaveBeenCalledWith("file2.ts", expect.objectContaining({ status: "denied" })) + }) + + it("marks the tool call rejected when individual permissions approve one file and deny the other", async () => { + const { mockTask, context, updateFileResult } = await requestBatchApproval({ + response: "messageResponse", + approvedIndexes: [0], + }) + + expect(mockTask.didRejectTool).toBe(true) + expect(context.didReject).toBe(true) + expect(updateFileResult).toHaveBeenCalledWith("file1.ts", { status: "approved" }) + expect(updateFileResult).toHaveBeenCalledWith("file2.ts", expect.objectContaining({ status: "denied" })) + }) + + it("leaves the tool call unrejected when individual permissions approve every file", async () => { + const { mockTask, context, updateFileResult } = await requestBatchApproval({ + response: "messageResponse", + approvedIndexes: [0, 1], + }) + + expect(mockTask.didRejectTool).toBe(false) + expect(context.didReject).toBe(false) + expect(updateFileResult).toHaveBeenCalledWith("file1.ts", { status: "approved" }) + expect(updateFileResult).toHaveBeenCalledWith("file2.ts", { status: "approved" }) + }) + }) + it("should block rooignore-protected files in legacy format", async () => { const mockTask = createMockTask({ rooIgnoreAllowed: false }) const callbacks = createMockCallbacks() diff --git a/src/core/tools/__tests__/searchReplaceTool.spec.ts b/src/core/tools/__tests__/searchReplaceTool.spec.ts index 5cf10790d4..c9de4fa69e 100644 --- a/src/core/tools/__tests__/searchReplaceTool.spec.ts +++ b/src/core/tools/__tests__/searchReplaceTool.spec.ts @@ -207,6 +207,7 @@ describe("searchReplaceTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) return toolResult @@ -371,6 +372,7 @@ describe("searchReplaceTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: localPushToolResult, + toolCallId: "test-tool-call-id", }) expect(capturedResult).toContain("Error:") diff --git a/src/core/tools/__tests__/switchModeTool.spec.ts b/src/core/tools/__tests__/switchModeTool.spec.ts index 88af75249b..fd8e939759 100644 --- a/src/core/tools/__tests__/switchModeTool.spec.ts +++ b/src/core/tools/__tests__/switchModeTool.spec.ts @@ -59,6 +59,7 @@ describe("SwitchModeTool", () => { askApproval: vi.fn().mockResolvedValue(true), handleError: vi.fn(), pushToolResult: vi.fn(), + toolCallId: "test-tool-call-id", } }) diff --git a/src/core/tools/__tests__/useMcpToolTool.spec.ts b/src/core/tools/__tests__/useMcpToolTool.spec.ts index 6af93be0f4..31cd5c0b33 100644 --- a/src/core/tools/__tests__/useMcpToolTool.spec.ts +++ b/src/core/tools/__tests__/useMcpToolTool.spec.ts @@ -99,6 +99,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.consecutiveMistakeCount).toBe(1) @@ -129,6 +130,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.consecutiveMistakeCount).toBe(1) @@ -175,6 +177,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.consecutiveMistakeCount).toBe(1) @@ -203,6 +206,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.ask).toHaveBeenCalledWith("use_mcp_server", expect.stringContaining("use_mcp_tool"), true) @@ -245,6 +249,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.consecutiveMistakeCount).toBe(0) @@ -294,6 +299,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.consecutiveMistakeCount).toBe(0) @@ -339,6 +345,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.say).not.toHaveBeenCalledWith("mcp_server_request_started") @@ -383,6 +390,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockHandleError).toHaveBeenCalledWith("executing MCP tool", error) @@ -430,6 +438,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.consecutiveMistakeCount).toBe(1) @@ -479,6 +488,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.consecutiveMistakeCount).toBe(1) @@ -532,6 +542,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.consecutiveMistakeCount).toBe(0) @@ -576,6 +587,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // Assert @@ -621,6 +633,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // Assert @@ -679,6 +692,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // Tool should be found and executed @@ -739,6 +753,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.say).toHaveBeenCalledWith("mcp_server_request_started") @@ -795,6 +810,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.say).toHaveBeenCalledWith("mcp_server_request_started") @@ -851,6 +867,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) // Should not double-prefix the data URL @@ -911,6 +928,7 @@ describe("useMcpToolTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) expect(mockTask.say).toHaveBeenCalledWith("mcp_server_response", "[2 image(s) received]", [ diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..1f04ecfc3a 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -238,6 +238,7 @@ describe("writeToFileTool", () => { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", }) return toolResult