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
This commit is contained in:
Ara
2026-06-17 10:09:56 +09:00
committed by Dominic Cooney
parent 23b33c1acf
commit 622803b152
7 changed files with 349 additions and 7 deletions
+6
View File
@@ -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",
@@ -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)
})
})
+48 -3
View File
@@ -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<string, number>()
/** Tool calls rejected by the user; they should not render as red tool failures. */
private deniedToolApprovalsByCallId = new Map<string, { toolName: string; reason: string }>()
/**
* 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
}
}
@@ -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 = {
@@ -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)
@@ -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<void>
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
}
@@ -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
}