From 499f32c8ef16c0cf839833bbe5bf86f64cc9b0f6 Mon Sep 17 00:00:00 2001 From: Stephen Hellicar Date: Tue, 7 Apr 2026 02:47:06 +1000 Subject: [PATCH 1/2] Record tool approval decision in conversation history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a tool was approved or denied, the decision left no trace — the tool name appeared in the tools block before the decision, then the approval UI disappeared without updating that record. The previous code appended the tool summary synchronously in handle() when the tool_approval_request arrived, before the user had decided anything. With batched tools (multiple tool_use blocks in one assistant message), all tool names were appended before any decision was recorded, making it impossible to annotate each line with its decision inline. Fix: move the appendStreaming call into #toolApprovalRequest(), writing "ToolName ✓\n" or "ToolName ✗\n" after respond() and removePendingTool(). Tools appear in the block only once decided, in FIFO order which matches the order they were requested — so single-tool and batch cases both look clean. The catch path appends ✗ for exceptions too. Tests: new describe block covering transitions, auto-denied (unknown tool, Deny path, synchronous), auto-approved (read tool, Approve path, synchronous), manual approval and manual denial (delete tool, Ask path, async — flushed with Promise.resolve()). --- .../claude-sdk-cli/src/AgentMessageHandler.ts | 5 +- .../test/AgentMessageHandler.spec.ts | 62 +++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/apps/claude-sdk-cli/src/AgentMessageHandler.ts b/apps/claude-sdk-cli/src/AgentMessageHandler.ts index 29fff60..db7ee04 100644 --- a/apps/claude-sdk-cli/src/AgentMessageHandler.ts +++ b/apps/claude-sdk-cli/src/AgentMessageHandler.ts @@ -144,7 +144,6 @@ export class AgentMessageHandler { } case 'tool_approval_request': this.#layout.transitionBlock('tools'); - this.#layout.appendStreaming(`${formatToolSummary(msg.name, msg.input, this.#cwd, this.#store)}\n`); if (!this.#usageBeforeTools) { this.#usageBeforeTools = this.#lastUsage; } @@ -213,10 +212,14 @@ export class AgentMessageHandler { } this.#respond(msg.requestId, approved); this.#layout.removePendingTool(msg.requestId); + const summary = formatToolSummary(msg.name, msg.input, this.#cwd, this.#store); + this.#layout.appendStreaming(`${summary} ${approved ? '✓' : '✗'}\n`); } catch (err) { this.#logger.error('Error', err); this.#respond(msg.requestId, false); this.#layout.removePendingTool(msg.requestId); + const catchSummary = formatToolSummary(msg.name, msg.input, this.#cwd, this.#store); + this.#layout.appendStreaming(`${catchSummary} ✗\n`); } } } diff --git a/apps/claude-sdk-cli/test/AgentMessageHandler.spec.ts b/apps/claude-sdk-cli/test/AgentMessageHandler.spec.ts index 5018ac7..f9f3dbb 100644 --- a/apps/claude-sdk-cli/test/AgentMessageHandler.spec.ts +++ b/apps/claude-sdk-cli/test/AgentMessageHandler.spec.ts @@ -1,4 +1,6 @@ +import type { AnyToolDefinition } from '@shellicar/claude-sdk'; import { describe, expect, it, vi } from 'vitest'; +import { z } from 'zod'; import { AgentMessageHandler, type AgentMessageHandlerOptions } from '../src/AgentMessageHandler.js'; import type { AppLayout } from '../src/AppLayout.js'; import { logger } from '../src/logger.js'; @@ -235,6 +237,66 @@ describe('AgentMessageHandler — tool_error', () => { }); }); +// --------------------------------------------------------------------------- +// tool_approval_request +// --------------------------------------------------------------------------- + +function makeTool(name: string, operation: AnyToolDefinition['operation']): AnyToolDefinition { + return { + name, + description: 'test', + operation, + input_schema: z.object({}), + input_examples: [], + handler: async () => ({}), + } as unknown as AnyToolDefinition; +} + +describe('AgentMessageHandler — tool_approval_request', () => { + it('transitions to tools block', () => { + const layout = makeLayout(); + makeHandler(layout).handle({ type: 'tool_approval_request', requestId: 'r1', name: 'Unknown', input: {} }); + const expected = 'tools'; + const actual = vi.mocked(layout.transitionBlock).mock.calls[0]?.[0]; + expect(actual).toBe(expected); + }); + + it('records auto-denied decision synchronously when tool is not registered', () => { + const layout = makeLayout(); + // empty tools → getPermission returns Deny for any unknown tool + makeHandler(layout).handle({ type: 'tool_approval_request', requestId: 'r1', name: 'Unknown', input: {} }); + const text = vi.mocked(layout.appendStreaming).mock.calls[0]?.[0] ?? ''; + expect(text).toContain('✗'); + }); + + it('records auto-approved decision synchronously for a read tool', () => { + const layout = makeLayout(); + const handler = makeHandler(layout, { tools: [makeTool('Find', 'read')] }); + handler.handle({ type: 'tool_approval_request', requestId: 'r1', name: 'Find', input: {} }); + const text = vi.mocked(layout.appendStreaming).mock.calls[0]?.[0] ?? ''; + expect(text).toContain('✓'); + }); + + it('records manual approval after user input for a delete tool', async () => { + const layout = makeLayout(); // requestApproval resolves true by default + const handler = makeHandler(layout, { tools: [makeTool('DeleteFile', 'delete')] }); + handler.handle({ type: 'tool_approval_request', requestId: 'r1', name: 'DeleteFile', input: {} }); + await Promise.resolve(); + const text = vi.mocked(layout.appendStreaming).mock.calls[0]?.[0] ?? ''; + expect(text).toContain('✓'); + }); + + it('records manual denial after user input for a delete tool', async () => { + const layout = makeLayout(); + vi.mocked(layout.requestApproval).mockResolvedValue(false); + const handler = makeHandler(layout, { tools: [makeTool('DeleteFile', 'delete')] }); + handler.handle({ type: 'tool_approval_request', requestId: 'r1', name: 'DeleteFile', input: {} }); + await Promise.resolve(); + const text = vi.mocked(layout.appendStreaming).mock.calls[0]?.[0] ?? ''; + expect(text).toContain('✗'); + }); +}); + // --------------------------------------------------------------------------- // message_usage // --------------------------------------------------------------------------- From 437916536bed4215bc3e02e8e4660cb3dd69287b Mon Sep 17 00:00:00 2001 From: Stephen Hellicar Date: Tue, 7 Apr 2026 04:08:10 +1000 Subject: [PATCH 2/2] Use emoji for tool approval decision markers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces ✓/✗ with ✅/❌ (and 💥 for exceptions) — more visually distinct in the conversation history at a glance. --- apps/claude-sdk-cli/src/AgentMessageHandler.ts | 4 ++-- apps/claude-sdk-cli/test/AgentMessageHandler.spec.ts | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/claude-sdk-cli/src/AgentMessageHandler.ts b/apps/claude-sdk-cli/src/AgentMessageHandler.ts index db7ee04..6661095 100644 --- a/apps/claude-sdk-cli/src/AgentMessageHandler.ts +++ b/apps/claude-sdk-cli/src/AgentMessageHandler.ts @@ -213,13 +213,13 @@ export class AgentMessageHandler { this.#respond(msg.requestId, approved); this.#layout.removePendingTool(msg.requestId); const summary = formatToolSummary(msg.name, msg.input, this.#cwd, this.#store); - this.#layout.appendStreaming(`${summary} ${approved ? '✓' : '✗'}\n`); + this.#layout.appendStreaming(`${summary} ${approved ? '✅' : '❌'}\n`); } catch (err) { this.#logger.error('Error', err); this.#respond(msg.requestId, false); this.#layout.removePendingTool(msg.requestId); const catchSummary = formatToolSummary(msg.name, msg.input, this.#cwd, this.#store); - this.#layout.appendStreaming(`${catchSummary} ✗\n`); + this.#layout.appendStreaming(`${catchSummary} 💥\n`); } } } diff --git a/apps/claude-sdk-cli/test/AgentMessageHandler.spec.ts b/apps/claude-sdk-cli/test/AgentMessageHandler.spec.ts index f9f3dbb..b3cc76f 100644 --- a/apps/claude-sdk-cli/test/AgentMessageHandler.spec.ts +++ b/apps/claude-sdk-cli/test/AgentMessageHandler.spec.ts @@ -266,7 +266,7 @@ describe('AgentMessageHandler — tool_approval_request', () => { // empty tools → getPermission returns Deny for any unknown tool makeHandler(layout).handle({ type: 'tool_approval_request', requestId: 'r1', name: 'Unknown', input: {} }); const text = vi.mocked(layout.appendStreaming).mock.calls[0]?.[0] ?? ''; - expect(text).toContain('✗'); + expect(text).toContain('❌'); }); it('records auto-approved decision synchronously for a read tool', () => { @@ -274,7 +274,7 @@ describe('AgentMessageHandler — tool_approval_request', () => { const handler = makeHandler(layout, { tools: [makeTool('Find', 'read')] }); handler.handle({ type: 'tool_approval_request', requestId: 'r1', name: 'Find', input: {} }); const text = vi.mocked(layout.appendStreaming).mock.calls[0]?.[0] ?? ''; - expect(text).toContain('✓'); + expect(text).toContain('✅'); }); it('records manual approval after user input for a delete tool', async () => { @@ -283,7 +283,7 @@ describe('AgentMessageHandler — tool_approval_request', () => { handler.handle({ type: 'tool_approval_request', requestId: 'r1', name: 'DeleteFile', input: {} }); await Promise.resolve(); const text = vi.mocked(layout.appendStreaming).mock.calls[0]?.[0] ?? ''; - expect(text).toContain('✓'); + expect(text).toContain('✅'); }); it('records manual denial after user input for a delete tool', async () => { @@ -293,7 +293,7 @@ describe('AgentMessageHandler — tool_approval_request', () => { handler.handle({ type: 'tool_approval_request', requestId: 'r1', name: 'DeleteFile', input: {} }); await Promise.resolve(); const text = vi.mocked(layout.appendStreaming).mock.calls[0]?.[0] ?? ''; - expect(text).toContain('✗'); + expect(text).toContain('❌'); }); });