From 622803b1529a50e4837c82691425596724ee0ff6 Mon Sep 17 00:00:00 2001 From: Ara Date: Wed, 3 Jun 2026 13:47:03 -0700 Subject: [PATCH] Fix approval chat replies rendering as tool errors (#11246) * fix(vscode): route approval chat replies as user feedback * fix(vscode): suppress approval reply denial errors * fix(vscode): hide rejected approval tool failures * chore(vscode): clarify denied approval suppression helper --- apps/vscode/src/sdk/SdkController.ts | 6 + ...message-translator-approval-denial.test.ts | 113 ++++++++++++++++++ apps/vscode/src/sdk/message-translator.ts | 51 +++++++- .../src/sdk/sdk-followup-coordinator.test.ts | 28 +++++ .../sdk/sdk-interaction-coordinator.test.ts | 73 +++++++++++ .../src/sdk/sdk-interaction-coordinator.ts | 35 +++++- apps/vscode/src/sdk/tool-approval-denial.ts | 50 ++++++++ 7 files changed, 349 insertions(+), 7 deletions(-) create mode 100644 apps/vscode/src/sdk/message-translator-approval-denial.test.ts create mode 100644 apps/vscode/src/sdk/tool-approval-denial.ts diff --git a/apps/vscode/src/sdk/SdkController.ts b/apps/vscode/src/sdk/SdkController.ts index 67f1aaf482..a63cea25c0 100644 --- a/apps/vscode/src/sdk/SdkController.ts +++ b/apps/vscode/src/sdk/SdkController.ts @@ -300,6 +300,12 @@ export class Controller { toolCallId, messageTs, ), + recordDeniedToolApproval: (toolCallId, toolName, reason) => + this.messageTranslatorState.recordDeniedToolApproval( + toolCallId, + toolName, + reason, + ), shouldAutoApproveTool: (request) => { const autoApprovalSettings = this.stateManager.getGlobalSettingsKey( "autoApprovalSettings", diff --git a/apps/vscode/src/sdk/message-translator-approval-denial.test.ts b/apps/vscode/src/sdk/message-translator-approval-denial.test.ts new file mode 100644 index 0000000000..145730ded7 --- /dev/null +++ b/apps/vscode/src/sdk/message-translator-approval-denial.test.ts @@ -0,0 +1,113 @@ +import type { CoreSessionEvent } from "@cline/core" +import type { AgentEvent } from "@cline/shared" +import { describe, expect, it } from "vitest" +import { MessageTranslatorState, translateSessionEvent } from "./message-translator" +import { DEFAULT_TOOL_APPROVAL_DENIAL_REASON, USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON } from "./tool-approval-denial" + +describe("translateSessionEvent - user-message tool approval denial", () => { + it("suppresses tool lifecycle events for approval replies routed as user feedback", () => { + const state = new MessageTranslatorState() + state.recordDeniedToolApproval("call-1", "fetch_web_content", USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON) + + const startEvent: CoreSessionEvent = { + type: "agent_event", + payload: { + sessionId: "session-1", + event: { + type: "content_start", + contentType: "tool", + toolName: "fetch_web_content", + toolCallId: "call-1", + input: { + requests: [{ url: "https://example.com", prompt: "Read it" }], + }, + } as AgentEvent, + }, + } + const endEvent: CoreSessionEvent = { + type: "agent_event", + payload: { + sessionId: "session-1", + event: { + type: "content_end", + contentType: "tool", + toolName: "fetch_web_content", + toolCallId: "call-1", + error: USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON, + } as AgentEvent, + }, + } + + const startResult = translateSessionEvent(startEvent, state) + const endResult = translateSessionEvent(endEvent, state) + + expect(startResult.messages).toHaveLength(0) + expect(endResult.messages).toHaveLength(0) + expect(endResult.toolError).toBeUndefined() + expect(endResult.toolSuccess).toBeUndefined() + }) + + it("suppresses generic no-button approval denials", () => { + const state = new MessageTranslatorState() + state.recordDeniedToolApproval("call-1", "fetch_web_content", DEFAULT_TOOL_APPROVAL_DENIAL_REASON) + + const endEvent: CoreSessionEvent = { + type: "agent_event", + payload: { + sessionId: "session-1", + event: { + type: "content_end", + contentType: "tool", + toolName: "fetch_web_content", + toolCallId: "call-1", + error: `{"error":"${DEFAULT_TOOL_APPROVAL_DENIAL_REASON}"}`, + } as AgentEvent, + }, + } + const mistakeEvent: CoreSessionEvent = { + type: "agent_event", + payload: { + sessionId: "session-1", + event: { + type: "error", + error: new Error( + `1 tool call(s) failed: [fetch_web_content] {"error":"${DEFAULT_TOOL_APPROVAL_DENIAL_REASON}"}`, + ), + recoverable: true, + iteration: 1, + } as AgentEvent, + }, + } + + const endResult = translateSessionEvent(endEvent, state) + const mistakeResult = translateSessionEvent(mistakeEvent, state) + + expect(endResult.messages).toHaveLength(0) + expect(endResult.toolError).toBeUndefined() + expect(mistakeResult.messages).toHaveLength(0) + expect(mistakeResult.turnComplete).toBe(false) + }) + + it("suppresses mistake errors caused by approval replies routed as user feedback", () => { + const state = new MessageTranslatorState() + const event: CoreSessionEvent = { + type: "agent_event", + payload: { + sessionId: "session-1", + event: { + type: "error", + error: new Error( + `1 tool call(s) failed: [fetch_web_content] {"error":"${USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON}"}`, + ), + recoverable: true, + iteration: 1, + } as AgentEvent, + }, + } + + const result = translateSessionEvent(event, state) + + expect(result.messages).toHaveLength(0) + expect(result.turnComplete).toBe(false) + }) +}) diff --git a/apps/vscode/src/sdk/message-translator.ts b/apps/vscode/src/sdk/message-translator.ts index ae811f3dcb..969c61bce4 100644 --- a/apps/vscode/src/sdk/message-translator.ts +++ b/apps/vscode/src/sdk/message-translator.ts @@ -42,6 +42,7 @@ import type { } from "@shared/ExtensionMessage" import { Logger } from "@shared/services/Logger" import { MessageIdMinter } from "./message-id-minter" +import { isDeniedToolApprovalMistake, isKnownToolApprovalDenial } from "./tool-approval-denial" // --------------------------------------------------------------------------- // Translation result @@ -123,6 +124,8 @@ export class MessageTranslatorState { private streamingToolName: string | undefined /** Approved tool-call ids mapped to the approval row that should be updated in place. */ private approvedToolMessageTsByCallId = new Map() + /** Tool calls rejected by the user; they should not render as red tool failures. */ + private deniedToolApprovalsByCallId = new Map() /** * Process-wide id/seq/epoch authority. Shared with the interaction coordinator and history * rendering so that message ids never collide across generators. See message-id-minter.ts. @@ -204,6 +207,30 @@ export class MessageTranslatorState { this.approvedToolMessageTsByCallId.clear() } + recordDeniedToolApproval(toolCallId: string, toolName: string, reason: string): void { + this.deniedToolApprovalsByCallId.set(toolCallId, { toolName, reason }) + } + + isToolApprovalDenied(toolCallId: string | undefined): boolean { + return toolCallId !== undefined && this.deniedToolApprovalsByCallId.has(toolCallId) + } + + /** + * Returns true when the given toolCallId was previously denied and its events should be + * suppressed. This intentionally does not remove the entry because the denial must persist + * past content_end so the follow-on error event can also be suppressed. + */ + checkDeniedToolApproval(toolCallId: string | undefined): boolean { + if (toolCallId === undefined || !this.deniedToolApprovalsByCallId.has(toolCallId)) { + return false + } + return true + } + + isSuppressedToolApprovalDenial(value: unknown): boolean { + return isDeniedToolApprovalMistake(value, this.deniedToolApprovalsByCallId.values()) + } + /** Reuse and remove a previously-approved prompt row for the matching tool event. */ consumeApprovedToolMessageTs(toolCallId: string | undefined): number | undefined { if (!toolCallId) { @@ -369,6 +396,7 @@ export class MessageTranslatorState { this.streamingToolInput = undefined this.streamingToolName = undefined this.clearApprovedToolMessageTs() + this.deniedToolApprovalsByCallId.clear() this.clearSpawnAgents() } @@ -873,6 +901,10 @@ function translateAgentEvent(event: AgentEvent, state: MessageTranslatorState): const toolName = event.toolName ?? "unknown" const input = event.input + if (state.isToolApprovalDenied(event.toolCallId)) { + break + } + // Store tool context so content_end can use it // (content_end doesn't carry the input) state.setStreamingToolContext(toolName, input) @@ -1051,6 +1083,11 @@ function translateAgentEvent(event: AgentEvent, state: MessageTranslatorState): case "tool": { const toolName = event.toolName ?? "unknown" + if (state.checkDeniedToolApproval(event.toolCallId) || isKnownToolApprovalDenial(event.error)) { + state.clearStreamingTool() + break + } + // ask_question is serviced by the interaction coordinator (see content_start); // it produces no transcript row of its own, so its content_end is a no-op. if (toolName === "ask_question" || toolName === "ask_followup_question") { @@ -1332,6 +1369,10 @@ function translateAgentEvent(event: AgentEvent, state: MessageTranslatorState): } case "error": { + if (state.isSuppressedToolApprovalDenial(event.error)) { + break + } + // Serialize the error message for the webview's ErrorRow to parse. // The webview uses ClineError.parse() on the `api_req_failed` text to // detect special error types (insufficient credits, spend limit, auth, @@ -1441,7 +1482,7 @@ export function translateSessionEvent(event: CoreSessionEvent, state: MessageTra if (agentEvent.type === "done") { result.turnComplete = true } - if (agentEvent.type === "error") { + if (agentEvent.type === "error" && !state.isSuppressedToolApprovalDenial(agentEvent.error)) { result.turnComplete = true } @@ -1449,9 +1490,13 @@ export function translateSessionEvent(event: CoreSessionEvent, state: MessageTra // A content_end event with contentType "tool" signals a completed // tool call — if event.error is set, the tool failed. if (agentEvent.type === "content_end" && agentEvent.contentType === "tool") { - if (agentEvent.error) { + if ( + agentEvent.error && + !isKnownToolApprovalDenial(agentEvent.error) && + !state.isToolApprovalDenied(agentEvent.toolCallId) + ) { result.toolError = true - } else { + } else if (!agentEvent.error) { result.toolSuccess = true } } diff --git a/apps/vscode/src/sdk/sdk-followup-coordinator.test.ts b/apps/vscode/src/sdk/sdk-followup-coordinator.test.ts index 05efba5fcb..7e43de2dfc 100644 --- a/apps/vscode/src/sdk/sdk-followup-coordinator.test.ts +++ b/apps/vscode/src/sdk/sdk-followup-coordinator.test.ts @@ -83,6 +83,34 @@ describe("SdkFollowupCoordinator", () => { ) }) + it("queues a message response after a pending tool approval is rejected", async () => { + const activeSession = makeActiveSession({ isRunning: true }) + const { coordinator, options } = makeCoordinator({ activeSession }) + options.interactions.resolvePendingToolApproval.mockReturnValue(false) + + await coordinator.askResponse("just give me an answer", undefined, undefined, "messageResponse") + + expect(options.interactions.resolvePendingToolApproval).toHaveBeenCalledWith("just give me an answer", "messageResponse") + expect(options.messages.appendAndEmit).toHaveBeenCalledWith( + [ + expect.objectContaining({ + type: "say", + say: "user_feedback", + text: "just give me an answer", + }), + ], + { type: "status", payload: { sessionId: "session-123", status: "running" } }, + ) + expect(options.sessions.fireAndForgetSend).toHaveBeenCalledWith( + activeSession.sdkHost, + "session-123", + "resolved: just give me an answer", + undefined, + undefined, + "queue", + ) + }) + it("resumes a displayed task before sending a follow-up when there is no live session", async () => { const task = makeTask("task-1") const historyItem = { diff --git a/apps/vscode/src/sdk/sdk-interaction-coordinator.test.ts b/apps/vscode/src/sdk/sdk-interaction-coordinator.test.ts index c0383b009c..3ccdb9ddd8 100644 --- a/apps/vscode/src/sdk/sdk-interaction-coordinator.test.ts +++ b/apps/vscode/src/sdk/sdk-interaction-coordinator.test.ts @@ -4,6 +4,7 @@ import { MessageTranslatorState, translateSessionEvent } from "./message-transla import { SdkInteractionCoordinator } from "./sdk-interaction-coordinator" import { SdkMessageCoordinator } from "./sdk-message-coordinator" import { createTaskProxy } from "./task-proxy" +import { DEFAULT_TOOL_APPROVAL_DENIAL_REASON, USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON } from "./tool-approval-denial" vi.mock("./webview-grpc-bridge", () => ({ pushMessageToWebview: vi.fn().mockResolvedValue(undefined), @@ -101,11 +102,13 @@ describe("SdkInteractionCoordinator", () => { it("resolves denied tool approval with the user reason", async () => { const task = createTaskProxy("session-123", vi.fn(), vi.fn()) const recordApprovedToolMessage = vi.fn() + const recordDeniedToolApproval = vi.fn() const coordinator = new SdkInteractionCoordinator({ messages: new SdkMessageCoordinator({ getTask: () => task }), getSessionId: () => "session-123", postStateToWebview: vi.fn().mockResolvedValue(undefined), recordApprovedToolMessage, + recordDeniedToolApproval, }) const approvalPromise = coordinator.handleRequestToolApproval({ @@ -124,9 +127,79 @@ describe("SdkInteractionCoordinator", () => { expect(coordinator.resolvePendingToolApproval("too risky", "noButtonClicked")).toBe(true) expect(recordApprovedToolMessage).not.toHaveBeenCalled() + expect(recordDeniedToolApproval).toHaveBeenCalledWith("tool-call", "execute_command", "too risky") await expect(approvalPromise).resolves.toEqual({ approved: false, reason: "too risky" }) }) + it("routes message responses as follow-ups instead of tool denial text", async () => { + const task = createTaskProxy("session-123", vi.fn(), vi.fn()) + const setTurnPhase = vi.fn() + const recordDeniedToolApproval = vi.fn() + const coordinator = new SdkInteractionCoordinator({ + messages: new SdkMessageCoordinator({ getTask: () => task }), + getSessionId: () => "session-123", + postStateToWebview: vi.fn().mockResolvedValue(undefined), + setTurnPhase, + recordDeniedToolApproval, + }) + + const approvalPromise = coordinator.handleRequestToolApproval({ + agentId: "agent", + conversationId: "conversation", + iteration: 1, + toolCallId: "tool-call", + toolName: "fetch_web_content", + input: { requests: [{ url: "https://example.com", prompt: "read it" }] }, + policy: { autoApprove: false }, + }) + await vi.waitFor(() => expect(task.messageStateHandler.getClineMessages()).toHaveLength(1)) + + expect(coordinator.resolvePendingToolApproval("just give me an answer", "messageResponse")).toBe(false) + await expect(approvalPromise).resolves.toEqual({ + approved: false, + reason: USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON, + }) + expect(recordDeniedToolApproval).toHaveBeenCalledWith( + "tool-call", + "fetch_web_content", + USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON, + ) + expect(setTurnPhase).toHaveBeenLastCalledWith("streaming") + }) + + it("records generic no-button approval denials for UI suppression", async () => { + const task = createTaskProxy("session-123", vi.fn(), vi.fn()) + const recordDeniedToolApproval = vi.fn() + const coordinator = new SdkInteractionCoordinator({ + messages: new SdkMessageCoordinator({ getTask: () => task }), + getSessionId: () => "session-123", + postStateToWebview: vi.fn().mockResolvedValue(undefined), + recordDeniedToolApproval, + }) + + const approvalPromise = coordinator.handleRequestToolApproval({ + agentId: "agent", + conversationId: "conversation", + iteration: 1, + toolCallId: "tool-call", + toolName: "fetch_web_content", + input: { requests: [{ url: "https://example.com", prompt: "read it" }] }, + policy: { autoApprove: false }, + }) + await vi.waitFor(() => expect(task.messageStateHandler.getClineMessages()).toHaveLength(1)) + + expect(coordinator.resolvePendingToolApproval(undefined, "noButtonClicked")).toBe(true) + await expect(approvalPromise).resolves.toEqual({ + approved: false, + reason: DEFAULT_TOOL_APPROVAL_DENIAL_REASON, + }) + expect(recordDeniedToolApproval).toHaveBeenCalledWith( + "tool-call", + "fetch_web_content", + DEFAULT_TOOL_APPROVAL_DENIAL_REASON, + ) + }) + it("auto-approves without emitting UI when the live settings allow the tool", async () => { const task = createTaskProxy("session-123", vi.fn(), vi.fn()) const postStateToWebview = vi.fn().mockResolvedValue(undefined) diff --git a/apps/vscode/src/sdk/sdk-interaction-coordinator.ts b/apps/vscode/src/sdk/sdk-interaction-coordinator.ts index a39360e83e..03e0c7b5a4 100644 --- a/apps/vscode/src/sdk/sdk-interaction-coordinator.ts +++ b/apps/vscode/src/sdk/sdk-interaction-coordinator.ts @@ -4,6 +4,7 @@ import { Logger } from "@/shared/services/Logger" import { MessageIdMinter } from "./message-id-minter" import { buildToolApprovalAskMessage } from "./message-translator" import type { SdkMessageCoordinator } from "./sdk-message-coordinator" +import { DEFAULT_TOOL_APPROVAL_DENIAL_REASON, USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON } from "./tool-approval-denial" export interface ToolApprovalRequest { agentId: string @@ -21,6 +22,7 @@ export interface SdkInteractionCoordinatorOptions { postStateToWebview: () => Promise shouldAutoApproveTool?: (request: ToolApprovalRequest) => boolean recordApprovedToolMessage?: (toolCallId: string, messageTs: number) => void + recordDeniedToolApproval?: (toolCallId: string, toolName: string, reason: string) => void /** * The process-wide id/seq/epoch authority, shared with the message translator. Optional so * existing tests that don't need cross-generator id uniqueness keep working; when omitted a @@ -42,6 +44,7 @@ export class SdkInteractionCoordinator { | { toolCallId: string messageTs: number + toolName: string } | undefined @@ -64,7 +67,11 @@ export class SdkInteractionCoordinator { return new Promise<{ approved: boolean; reason?: string }>((resolve) => { this.pendingToolApprovalResolve = resolve - this.pendingToolApprovalMessage = { toolCallId: request.toolCallId, messageTs: toolAskMessage.ts } + this.pendingToolApprovalMessage = { + toolCallId: request.toolCallId, + messageTs: toolAskMessage.ts, + toolName: request.toolName, + } }) } @@ -102,18 +109,38 @@ export class SdkInteractionCoordinator { const pendingMessage = this.pendingToolApprovalMessage this.pendingToolApprovalResolve = undefined this.pendingToolApprovalMessage = undefined + + if (responseType === "messageResponse") { + Logger.log("[SdkController] Rejecting pending tool approval from user message and routing message as follow-up") + this.options.setTurnPhase?.("streaming") + if (pendingMessage) { + this.options.recordDeniedToolApproval?.( + pendingMessage.toolCallId, + pendingMessage.toolName, + USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON, + ) + } + resolve({ approved: false, reason: USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON }) + // The approval was resolved, but the chat message still needs normal follow-up routing. + return false + } + const approved = responseType === "yesButtonClicked" Logger.log(`[SdkController] Resolving pending tool approval: approved=${approved} (responseType=${responseType})`) if (approved && pendingMessage) { this.options.recordApprovedToolMessage?.(pendingMessage.toolCallId, pendingMessage.messageTs) } - // Approved or rejected, the agent resumes its turn — back to streaming. (On rejection - // the agent receives the denial and continues; the SDK drives the next phase.) + // Approved or rejected by approval controls, the agent resumes its turn and returns to streaming. + // On rejection the agent receives the denial and continues; the SDK drives the next phase. this.options.setTurnPhase?.("streaming") + const denialReason = prompt || DEFAULT_TOOL_APPROVAL_DENIAL_REASON + if (!approved && pendingMessage) { + this.options.recordDeniedToolApproval?.(pendingMessage.toolCallId, pendingMessage.toolName, denialReason) + } resolve({ approved, - ...(approved ? {} : { reason: prompt || "User denied the tool execution" }), + ...(approved ? {} : { reason: denialReason }), }) return true } diff --git a/apps/vscode/src/sdk/tool-approval-denial.ts b/apps/vscode/src/sdk/tool-approval-denial.ts new file mode 100644 index 0000000000..9420769a6c --- /dev/null +++ b/apps/vscode/src/sdk/tool-approval-denial.ts @@ -0,0 +1,50 @@ +export const DEFAULT_TOOL_APPROVAL_DENIAL_REASON = "User denied the tool execution" +export const USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON = "Tool execution was cancelled because the user sent a follow-up message." + +function getMessage(value: unknown): string | undefined { + if (typeof value === "string") { + return value + } + + if (value instanceof Error) { + return value.message + } + + if (value && typeof value === "object" && "message" in value) { + const message = (value as { message?: unknown }).message + return typeof message === "string" ? message : undefined + } + + return undefined +} + +export function isKnownToolApprovalDenial(value: unknown): boolean { + const message = getMessage(value) + if (!message) { + return false + } + + return message.includes(USER_MESSAGE_TOOL_APPROVAL_DENIAL_REASON) || message.includes(DEFAULT_TOOL_APPROVAL_DENIAL_REASON) +} + +export function isDeniedToolApprovalMistake( + value: unknown, + deniedApprovals: Iterable<{ toolName: string; reason: string }>, +): boolean { + const message = getMessage(value) + if (!message) { + return false + } + + if (isKnownToolApprovalDenial(message)) { + return true + } + + for (const { toolName, reason } of deniedApprovals) { + if (message.includes(`[${toolName}]`) && message.includes(reason)) { + return true + } + } + + return false +}