From 97a2bd750765212441e2b8fb5c209d5432a3407d Mon Sep 17 00:00:00 2001 From: Sri Pasumarthi <111310667+sripasg@users.noreply.github.com> Date: Wed, 6 May 2026 08:42:01 -0700 Subject: [PATCH] fix(acp): move tool explanation from thought stream to tool call content (#26554) --- packages/cli/src/acp/acpSession.test.ts | 121 ++++++++++++++++++++++++ packages/cli/src/acp/acpSession.ts | 21 ++-- 2 files changed, 135 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/acp/acpSession.test.ts b/packages/cli/src/acp/acpSession.test.ts index 14f04ba7c5..482254f3c3 100644 --- a/packages/cli/src/acp/acpSession.test.ts +++ b/packages/cli/src/acp/acpSession.test.ts @@ -586,4 +586,125 @@ describe('Session', () => { }, }); }); + + it('should add explanation to tool call content instead of thought chunk', async () => { + mockTool.build.mockReturnValue({ + getDescription: () => 'Test Tool', + getExplanation: () => 'Test Explanation', + toolLocations: () => [], + shouldConfirmExecute: vi + .fn() + .mockResolvedValue({ type: 'info', onConfirm: vi.fn() }), + execute: vi.fn().mockResolvedValue({ llmContent: 'Tool Result' }), + }); + + mockConnection.requestPermission.mockResolvedValue({ + outcome: { + outcome: 'selected', + optionId: 'proceed_once', + }, + }); + + const stream1 = createMockStream([ + { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'call-1', + name: 'test_tool', + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-1', + }, + }, + ]); + const stream2 = createMockStream([ + { + type: GeminiEventType.Content, + value: '', + }, + ]); + + mockSendMessageStream + .mockReturnValueOnce(stream1) + .mockReturnValueOnce(stream2); + + await session.prompt({ + sessionId: 'session-1', + prompt: [{ type: 'text', text: 'Call tool' }], + }); + + expect(mockConnection.sessionUpdate).not.toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_thought_chunk', + content: { type: 'text', text: 'Test Explanation' }, + }), + }), + ); + + expect(mockConnection.requestPermission).toHaveBeenCalledWith( + expect.objectContaining({ + toolCall: expect.objectContaining({ + content: expect.arrayContaining([ + { + type: 'content', + content: { type: 'text', text: 'Test Explanation' }, + }, + ]), + }), + }), + ); + }); + + it('should add explanation to tool_call update content instead of thought chunk when no permission required', async () => { + mockTool.build.mockReturnValue({ + getDescription: () => 'Test Tool', + getExplanation: () => 'Test Explanation', + toolLocations: () => [], + shouldConfirmExecute: vi.fn().mockResolvedValue(null), + execute: vi.fn().mockResolvedValue({ llmContent: 'Tool Result' }), + }); + + const stream1 = createMockStream([ + { + type: GeminiEventType.ToolCallRequest, + value: { + callId: 'call-1', + name: 'test_tool', + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-1', + }, + }, + ]); + const stream2 = createMockStream([ + { + type: GeminiEventType.Content, + value: '', + }, + ]); + + mockSendMessageStream + .mockReturnValueOnce(stream1) + .mockReturnValueOnce(stream2); + + await session.prompt({ + sessionId: 'session-1', + prompt: [{ type: 'text', text: 'Call tool' }], + }); + + expect(mockConnection.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'tool_call', + content: expect.arrayContaining([ + { + type: 'content', + content: { type: 'text', text: 'Test Explanation' }, + }, + ]), + }), + }), + ); + }); }); diff --git a/packages/cli/src/acp/acpSession.ts b/packages/cli/src/acp/acpSession.ts index da7401cba1..d3c0aa3c9b 100644 --- a/packages/cli/src/acp/acpSession.ts +++ b/packages/cli/src/acp/acpSession.ts @@ -619,13 +619,6 @@ export class Session { ? invocation.getExplanation() : ''; - if (explanation) { - await this.sendUpdate({ - sessionUpdate: 'agent_thought_chunk', - content: { type: 'text', text: explanation }, - }); - } - const confirmationDetails = await invocation.shouldConfirmExecute(abortSignal); @@ -648,6 +641,13 @@ export class Session { }); } + if (content.length === 0 && explanation) { + content.push({ + type: 'content', + content: { type: 'text', text: explanation }, + }); + } + const params: acp.RequestPermissionRequest = { sessionId: this.id, options: toPermissionOptions( @@ -708,6 +708,13 @@ export class Session { } else { const content: acp.ToolCallContent[] = []; + if (explanation) { + content.push({ + type: 'content', + content: { type: 'text', text: explanation }, + }); + } + await this.sendUpdate({ sessionUpdate: 'tool_call', toolCallId: callId,