From 7281eb58fc9f5461f3aeface5102e2d6483733b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=A1=BE=E7=9B=BC?= Date: Mon, 6 Jul 2026 22:35:38 +0800 Subject: [PATCH] feat(core): surface PreToolUse hook 'ask' as a TUI confirmation (#5629) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(core): surface PreToolUse hook 'ask' as a TUI confirmation A PreToolUse hook returning permissionDecision:'ask' was treated the same as 'deny'. The hook fires in the execution phase (_executeToolCallBody), after the confirmation flow in _schedule has finished, so an 'ask' could only block the tool as EXECUTION_DENIED instead of prompting the user. Bounce the tool from the execution phase back to awaiting_approval when a hook asks: build a synthetic 'info' confirmation whose onConfirm routes through handleConfirmationResponse (ProceedOnce re-executes, Cancel cancels). PreToolUse keeps its "before execution" timing — only the 'ask' branch is new; 'denied'/'stop' keep deny-as-error. A non-interactive CLI or background agent cannot prompt, so 'ask' falls back to deny there. The re-execution after approval skips both the hook re-fire (no infinite re-ask loop) and the non-idempotent path-unescape prelude. A walk-away abort sets a terminal status so the turn cannot hang, and the tool span survives the bounce so it is finalized exactly once. Tests: add coverage for ask->awaiting_approval, approve->execute-once (no re-ask loop), decline->cancelled, non-interactive/background deny, walk-away abort, single span finalize, and no double path-unescape. * fix(core): handle PreToolUse 'ask' bounce edge cases from review Round 1 review of #5629 surfaced edge cases in the bounce mechanism: - Multi-tool batch hang: a bounced tool approved while a sibling was still executing stayed stuck in 'scheduled'. attemptExecutionOfScheduledCalls now loops, re-checking for newly-scheduled bounce-approved calls after each batch drains. - Orphaned hook events: the post-approval re-execution generated a fresh tool_use_id, leaving PreToolUse(old)/PostToolUse(new) unpaired. Preserve and reuse the original id across the bounce. - ModifyWithEditor double-unescape: request.args is unescaped in place before the hook fires, so the ModifyWithEditor branch must skip its own unescape for a bounced tool (it would double-strip escaped metacharacters). - Missing signal.aborted re-check before bouncing: mirror the confirmation-phase guard so an aborted signal falls through to deny instead of flashing a confirmation nobody can answer. Tests: multi-tool-hang regression (RED before the loop fix), non-interactive STREAM_JSON and Zed bounce paths, and span-finalize assertions on the walk-away abort test. * fix(core): keep PreToolUse 'ask' gate when a sibling is auto-approved Round 2 review: autoApproveCompatiblePendingTools auto-approved every awaiting_approval tool when a sibling was approved with ProceedAlways — including tools bounced by a PreToolUse 'ask'. The bounced tool would be auto-approved and re-executed with the hook skipped (isPostAskReexecution), silently defeating the hook's confirmation gate. Exclude bounced callIds from the auto-approve filter so a hook 'ask' always requires explicit confirmation. Test: a sibling's ProceedAlways no longer auto-approves a bounced ask (RED before the filter guard). * fix(cli): preserve hook ask prompts on approval mode change * fix(core): handle PreToolUse ask edge cases * fix(core): cancel scheduled calls during ask abort drain --------- Co-authored-by: qwen-code-dev-bot Co-authored-by: Qwen-Coder --- .../cli/src/ui/hooks/useGeminiStream.test.tsx | 41 + packages/cli/src/ui/hooks/useGeminiStream.ts | 13 +- .../core/src/core/coreToolScheduler.test.ts | 850 +++++++++++++++++- packages/core/src/core/coreToolScheduler.ts | 286 +++++- 4 files changed, 1139 insertions(+), 51 deletions(-) diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index 19961ba87c..191189f52f 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -6010,6 +6010,47 @@ describe('useGeminiStream', () => { ); }); + it('should not auto-approve prompts that hide always allow', async () => { + const mockOnConfirm = vi.fn().mockResolvedValue(undefined); + const awaitingApprovalToolCalls: TrackedToolCall[] = [ + { + request: { + callId: 'call1', + name: 'replace', + args: { old_string: 'old', new_string: 'new' }, + isClientInitiated: false, + prompt_id: 'prompt-id-1', + }, + status: 'awaiting_approval', + responseSubmittedToGemini: false, + confirmationDetails: { + onConfirm: mockOnConfirm, + onCancel: vi.fn(), + message: 'Confirm hook ask?', + displayedText: 'Hook requested confirmation', + hideAlwaysAllow: true, + }, + tool: { + name: 'replace', + displayName: 'replace', + description: 'Replace text', + build: vi.fn(), + } as any, + invocation: { + getDescription: () => 'Mock description', + } as unknown as AnyToolInvocation, + } as unknown as TrackedWaitingToolCall, + ]; + + const { result } = renderTestHook(awaitingApprovalToolCalls); + + await act(async () => { + await result.current.handleApprovalModeChange(ApprovalMode.YOLO); + }); + + expect(mockOnConfirm).not.toHaveBeenCalled(); + }); + it('should only auto-approve edit tools when switching to AUTO_EDIT mode', async () => { const mockOnConfirmReplace = vi.fn().mockResolvedValue(undefined); const mockOnConfirmWrite = vi.fn().mockResolvedValue(undefined); diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index c83d9a018d..c1836137d0 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -2758,8 +2758,17 @@ export const useGeminiStream = ( newApprovalMode === ApprovalMode.AUTO_EDIT ) { let awaitingApprovalCalls = toolCalls.filter( - (call): call is TrackedWaitingToolCall => - call.status === 'awaiting_approval', + (call): call is TrackedWaitingToolCall => { + if (call.status !== 'awaiting_approval') { + return false; + } + const { confirmationDetails } = call; + return !( + confirmationDetails && + 'hideAlwaysAllow' in confirmationDetails && + confirmationDetails.hideAlwaysAllow === true + ); + }, ); // For AUTO_EDIT mode, only approve edit tools (edit/replace, write_file, notebook_edit) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index f11acaec02..36df456f9b 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -54,6 +54,8 @@ import { import { MessageBusType } from '../confirmation-bus/types.js'; import type { HookExecutionResponse } from '../confirmation-bus/types.js'; import { type NotificationType } from '../hooks/types.js'; +import { InputFormat } from '../output/types.js'; +import { unescapePath } from '../utils/paths.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { IdeClient } from '../ide/ide-client.js'; import { WriteFileTool } from '../tools/write-file.js'; @@ -1051,7 +1053,6 @@ describe('CoreToolScheduler', () => { } }); - it('executes only the first request for duplicate callIds in one batch', async () => { const execute = vi.fn().mockResolvedValue({ llmContent: 'first result', @@ -6842,37 +6843,48 @@ describe('CoreToolScheduler telemetry spans', () => { signal?: AbortSignal, updateOutput?: (output: string) => void, ) => Promise; + tools?: MockTool[]; messageBus?: { request: ReturnType }; disableHooks?: boolean; canUpdateOutput?: boolean; + isInteractive?: boolean; + inputFormat?: InputFormat; + shouldAvoidPermissionPrompts?: boolean; + experimentalZedIntegration?: boolean; includeSensitiveSpanAttributes?: boolean; sensitiveSpanAttributeMaxLength?: number; }): { scheduler: CoreToolScheduler; onAllToolCallsComplete: ReturnType; + onToolCallsUpdate: ReturnType; } { - const mockTool = new MockTool({ - name: 'mockTool', - canUpdateOutput: options.canUpdateOutput, - execute: - options.execute ?? - vi.fn().mockResolvedValue({ - llmContent: 'ok', - returnDisplay: 'ok', - }), - }); + const tools = options.tools ?? [ + new MockTool({ + name: 'mockTool', + canUpdateOutput: options.canUpdateOutput, + execute: + options.execute ?? + vi.fn().mockResolvedValue({ + llmContent: 'ok', + returnDisplay: 'ok', + }), + }), + ]; + const toolsByName = new Map(tools.map((t) => [t.name, t])); + const lookup = (name?: string) => + (name ? toolsByName.get(name) : undefined) ?? tools[0]; const mockToolRegistry = { - getTool: () => mockTool, - ensureTool: async () => mockTool, + getTool: (n?: string) => lookup(n), + ensureTool: async (n?: string) => lookup(n), getFunctionDeclarations: () => [], tools: new Map(), discovery: {}, registerTool: () => {}, - getToolByName: () => mockTool, - getToolByDisplayName: () => mockTool, - getTools: () => [], + getToolByName: (n?: string) => lookup(n), + getToolByDisplayName: (n?: string) => lookup(n), + getTools: () => tools, discoverTools: async () => {}, - getAllTools: () => [], + getAllTools: () => tools, getToolsByServer: () => [], } as unknown as ToolRegistry; @@ -6900,6 +6912,14 @@ describe('CoreToolScheduler telemetry spans', () => { getChatRecordingService: () => undefined, getMessageBus: vi.fn().mockReturnValue(options.messageBus), getDisableAllHooks: vi.fn().mockReturnValue(options.disableHooks ?? true), + // Confirmation-prompt capability stubs — consumed by + // canPromptForAskBounce when a PreToolUse hook returns 'ask'. + isInteractive: () => options.isInteractive ?? true, + getInputFormat: () => options.inputFormat ?? InputFormat.TEXT, + getExperimentalZedIntegration: () => + options.experimentalZedIntegration ?? false, + getShouldAvoidPermissionPrompts: () => + options.shouldAvoidPermissionPrompts ?? false, getTelemetryIncludeSensitiveSpanAttributes: () => options.includeSensitiveSpanAttributes ?? false, getTelemetrySensitiveSpanAttributeMaxLength: () => @@ -6907,14 +6927,15 @@ describe('CoreToolScheduler telemetry spans', () => { } as unknown as Config; const onAllToolCallsComplete = vi.fn(); + const onToolCallsUpdate = vi.fn(); const scheduler = new CoreToolScheduler({ config: mockConfig, onAllToolCallsComplete, - onToolCallsUpdate: vi.fn(), + onToolCallsUpdate, getPreferredEditor: () => 'vscode', onEditorClose: vi.fn(), }); - return { scheduler, onAllToolCallsComplete }; + return { scheduler, onAllToolCallsComplete, onToolCallsUpdate }; } async function runSingleTool( @@ -7726,6 +7747,797 @@ describe('CoreToolScheduler telemetry spans', () => { expect(getBlockedSpans()).toHaveLength(0); }); + // ------------------------------------------------------------------- + // PreToolUse hook permissionDecision:'ask' — bounce the tool from the + // EXECUTION phase back to awaiting_approval for a native TUI confirmation + // instead of denying it (the historical behavior). When we cannot prompt + // (non-interactive / background agent) it still falls back to deny. + // ------------------------------------------------------------------- + + function askMessageBus(reason = 'please confirm'): { + request: ReturnType; + } { + return { + request: vi.fn().mockResolvedValue({ + type: MessageBusType.HOOK_EXECUTION_RESPONSE, + correlationId: 'pre-hook', + success: true, + output: { decision: 'ask', reason }, + }), + }; + } + + async function scheduleWithAsk(options: { + messageBus: { request: ReturnType }; + execute?: () => Promise; + isInteractive?: boolean; + inputFormat?: InputFormat; + shouldAvoidPermissionPrompts?: boolean; + experimentalZedIntegration?: boolean; + args?: Record; + abortController?: AbortController; + tools?: MockTool[]; + }): Promise<{ + scheduler: CoreToolScheduler; + onAllToolCallsComplete: ReturnType; + onToolCallsUpdate: ReturnType; + abortController: AbortController; + }> { + toolSpanRecords.length = 0; + const built = buildScheduler({ disableHooks: false, ...options }); + const abortController = options.abortController ?? new AbortController(); + await built.scheduler.schedule( + [ + { + callId: 'ask-call', + name: 'mockTool', + args: options.args ?? { input: 'x' }, + isClientInitiated: false, + prompt_id: 'prompt-ask', + }, + ], + abortController.signal, + ); + return { ...built, abortController }; + } + + // Count only PreToolUse fires — the same messageBus mock also serves + // PostToolUse/PostToolBatch, so a raw call count would be ambiguous. + function preToolUseCallCount(messageBus: { + request: ReturnType; + }): number { + return messageBus.request.mock.calls.filter( + (call) => (call[0] as { eventName?: string })?.eventName === 'PreToolUse', + ).length; + } + + it('bounces a PreToolUse ask to awaiting_approval with an info confirmation', async () => { + const messageBus = askMessageBus('confirm deploy 38111'); + const { onToolCallsUpdate } = await scheduleWithAsk({ messageBus }); + + const waiting = (await waitForStatus( + onToolCallsUpdate, + 'awaiting_approval', + )) as WaitingToolCall; + + expect(waiting.confirmationDetails.type).toBe('info'); + // The hook re-evaluates on every call, so "always allow" is hidden. + expect( + (waiting.confirmationDetails as { hideAlwaysAllow?: boolean }) + .hideAlwaysAllow, + ).toBe(true); + expect( + (waiting.confirmationDetails as { prompt: string }).prompt, + ).toContain('confirm deploy 38111'); + // One open blocked_on_user span; the tool span stays open across the + // bounce (it is NOT finalized until the confirmation resolves). + const blocked = getBlockedSpans(); + expect(blocked).toHaveLength(1); + expect(blocked[0].ended).toBe(false); + expect(getToolSpans()[0].ended).toBe(false); + }); + + it('executes the tool exactly once when the user approves an ask (no re-ask loop)', async () => { + const execute = vi.fn().mockResolvedValue({ + llmContent: 'ok', + returnDisplay: 'ok', + }); + const messageBus = askMessageBus(); + const { onToolCallsUpdate, onAllToolCallsComplete } = await scheduleWithAsk( + { + messageBus, + execute, + }, + ); + + const waiting = (await waitForStatus( + onToolCallsUpdate, + 'awaiting_approval', + )) as WaitingToolCall; + await waiting.confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedOnce, + ); + + await vi.waitFor(() => { + expect(onAllToolCallsComplete).toHaveBeenCalled(); + }); + const completed = onAllToolCallsComplete.mock.calls.at( + -1, + )?.[0] as ToolCall[]; + expect(completed[0].status).toBe('success'); + expect(execute).toHaveBeenCalledTimes(1); + // The re-execution skips the hook → PreToolUse fired exactly once. + expect(preToolUseCallCount(messageBus)).toBe(1); + + // Tool span finalized exactly once; blocked span ended. + const toolSpans = getToolSpans(); + expect(toolSpans).toHaveLength(1); + expect(toolSpans[0].ended).toBe(true); + const blocked = getBlockedSpans(); + expect(blocked).toHaveLength(1); + expect(blocked[0].ended).toBe(true); + }); + + it('cancels the tool without executing when the user declines an ask', async () => { + const execute = vi.fn(); + const messageBus = askMessageBus(); + const { onToolCallsUpdate, onAllToolCallsComplete } = await scheduleWithAsk( + { + messageBus, + execute, + }, + ); + + const waiting = (await waitForStatus( + onToolCallsUpdate, + 'awaiting_approval', + )) as WaitingToolCall; + await waiting.confirmationDetails.onConfirm(ToolConfirmationOutcome.Cancel); + + await vi.waitFor(() => { + expect(onAllToolCallsComplete).toHaveBeenCalled(); + }); + const completed = onAllToolCallsComplete.mock.calls.at( + -1, + )?.[0] as ToolCall[]; + expect(completed[0].status).toBe('cancelled'); + expect(execute).not.toHaveBeenCalled(); + }); + + it('denies a PreToolUse ask (no bounce) in non-interactive mode', async () => { + const execute = vi.fn(); + const messageBus = askMessageBus(); + const { onAllToolCallsComplete } = await scheduleWithAsk({ + messageBus, + execute, + isInteractive: false, + inputFormat: InputFormat.TEXT, + }); + + await vi.waitFor(() => { + expect(onAllToolCallsComplete).toHaveBeenCalled(); + }); + const completed = onAllToolCallsComplete.mock.calls.at( + -1, + )?.[0] as ToolCall[]; + expect(completed[0].status).toBe('error'); + expect(execute).not.toHaveBeenCalled(); + // Never bounced → no blocked span. + expect(getBlockedSpans()).toHaveLength(0); + }); + + it('denies a PreToolUse ask for background agents', async () => { + const execute = vi.fn(); + const messageBus = askMessageBus(); + const { onAllToolCallsComplete } = await scheduleWithAsk({ + messageBus, + execute, + shouldAvoidPermissionPrompts: true, + }); + + await vi.waitFor(() => { + expect(onAllToolCallsComplete).toHaveBeenCalled(); + }); + const completed = onAllToolCallsComplete.mock.calls.at( + -1, + )?.[0] as ToolCall[]; + expect(completed[0].status).toBe('error'); + expect(execute).not.toHaveBeenCalled(); + expect(getBlockedSpans()).toHaveLength(0); + }); + + it('cancels a pending ask (no hang) when the signal aborts', async () => { + const execute = vi.fn(); + const messageBus = askMessageBus(); + const abortController = new AbortController(); + const { onToolCallsUpdate, onAllToolCallsComplete } = await scheduleWithAsk( + { + messageBus, + execute, + abortController, + }, + ); + + await waitForStatus(onToolCallsUpdate, 'awaiting_approval'); + abortController.abort(); + + await vi.waitFor(() => { + expect(onAllToolCallsComplete).toHaveBeenCalled(); + }); + const completed = onAllToolCallsComplete.mock.calls.at( + -1, + )?.[0] as ToolCall[]; + expect(completed[0].status).toBe('cancelled'); + expect(execute).not.toHaveBeenCalled(); + // The drainSpansForBatch safety net must also END both spans (not just + // leave the tool stuck) — guards against accidental removal of the + // terminal setStatusInternal added to drainSpansForBatch. + expect(getBlockedSpans()[0]?.ended).toBe(true); + expect(getToolSpans()[0]?.ended).toBe(true); + }); + + it('does not double-unescape path args across an ask bounce', async () => { + const execute = vi.fn().mockResolvedValue({ + llmContent: 'ok', + returnDisplay: 'ok', + }); + const messageBus = askMessageBus(); + // Two backslashes before the space: unescaping once → `a\ b`, twice → + // `a b`. The re-execution must skip the unescape prelude so the path is + // unescaped exactly once. + const rawPath = 'a\\\\ b'; + const { onToolCallsUpdate, onAllToolCallsComplete } = await scheduleWithAsk( + { + messageBus, + execute, + args: { file_path: rawPath }, + }, + ); + + const waiting = (await waitForStatus( + onToolCallsUpdate, + 'awaiting_approval', + )) as WaitingToolCall; + await waiting.confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedOnce, + ); + + await vi.waitFor(() => { + expect(onAllToolCallsComplete).toHaveBeenCalled(); + }); + const completed = onAllToolCallsComplete.mock.calls.at( + -1, + )?.[0] as ToolCall[]; + expect(completed[0].status).toBe('success'); + expect( + (completed[0].request.args as Record)['file_path'], + ).toBe(unescapePath(rawPath)); + }); + + it('approving a bounced ask runs the tool even while a sibling is still executing', async () => { + toolSpanRecords.length = 0; + // toolA bounces (PreToolUse 'ask'); toolB's execute stays pending so it + // is still 'executing' when the user approves A. The + // attemptExecutionOfScheduledCalls guard fails on that pass — without a + // re-check after toolB drains, toolA would hang in 'scheduled' forever. + let resolveB!: (r: ToolResult) => void; + const bDone = new Promise((res) => { + resolveB = res; + }); + const aExecute = vi.fn().mockResolvedValue({ + llmContent: 'A ok', + returnDisplay: 'A ok', + }); + const bExecute = vi.fn().mockReturnValue(bDone); + const tools = [ + new MockTool({ name: 'toolA', kind: Kind.Read, execute: aExecute }), + new MockTool({ name: 'toolB', kind: Kind.Read, execute: bExecute }), + ]; + const messageBus = { + request: vi + .fn() + .mockImplementation( + async (req: { + eventName?: string; + input?: { tool_name?: string }; + }) => ({ + type: MessageBusType.HOOK_EXECUTION_RESPONSE, + correlationId: 'pre-hook', + success: true, + output: + req.eventName === 'PreToolUse' && req.input?.tool_name === 'toolA' + ? { decision: 'ask', reason: 'confirm A' } + : {}, + }), + ), + }; + const { scheduler, onAllToolCallsComplete, onToolCallsUpdate } = + buildScheduler({ tools, messageBus, disableHooks: false }); + + // Not awaited: schedule stays pending while toolB executes. + const schedulePromise = scheduler.schedule( + [ + { + callId: 'a', + name: 'toolA', + args: {}, + isClientInitiated: false, + prompt_id: 'p', + }, + { + callId: 'b', + name: 'toolB', + args: {}, + isClientInitiated: false, + prompt_id: 'p', + }, + ], + new AbortController().signal, + ); + + const waiting = (await waitForStatus( + onToolCallsUpdate, + 'awaiting_approval', + )) as WaitingToolCall; + // Approve A while toolB's execute is still pending. + await waiting.confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedOnce, + ); + // Let toolB finish — toolA must now run rather than stay stuck. + resolveB({ llmContent: 'B ok', returnDisplay: 'B ok' }); + + await vi.waitFor(() => { + expect(onAllToolCallsComplete).toHaveBeenCalled(); + }); + await schedulePromise; + const completed = onAllToolCallsComplete.mock.calls.at( + -1, + )?.[0] as ToolCall[]; + expect(aExecute).toHaveBeenCalledTimes(1); + expect(completed.find((c) => c.request.callId === 'a')?.status).toBe( + 'success', + ); + expect(completed.find((c) => c.request.callId === 'b')?.status).toBe( + 'success', + ); + }); + + it('bounces a non-interactive STREAM_JSON ask (client can answer control requests)', async () => { + const execute = vi.fn().mockResolvedValue({ + llmContent: 'ok', + returnDisplay: 'ok', + }); + const messageBus = askMessageBus(); + const { onToolCallsUpdate } = await scheduleWithAsk({ + messageBus, + execute, + isInteractive: false, + inputFormat: InputFormat.STREAM_JSON, + }); + const waiting = (await waitForStatus( + onToolCallsUpdate, + 'awaiting_approval', + )) as WaitingToolCall; + expect(waiting.confirmationDetails.type).toBe('info'); + expect(getBlockedSpans()).toHaveLength(1); + }); + + it('bounces a non-interactive ask under the Zed integration', async () => { + const execute = vi.fn().mockResolvedValue({ + llmContent: 'ok', + returnDisplay: 'ok', + }); + const messageBus = askMessageBus(); + const { onToolCallsUpdate } = await scheduleWithAsk({ + messageBus, + execute, + isInteractive: false, + experimentalZedIntegration: true, + }); + const waiting = (await waitForStatus( + onToolCallsUpdate, + 'awaiting_approval', + )) as WaitingToolCall; + expect(waiting.confirmationDetails.type).toBe('info'); + expect(getBlockedSpans()).toHaveLength(1); + }); + + it("a sibling's ProceedAlways must not auto-approve a bounced ask", async () => { + toolSpanRecords.length = 0; + // Both tools bounce on a PreToolUse 'ask'. Approving toolB with + // ProceedAlways runs autoApproveCompatiblePendingTools, which must NOT + // auto-approve the bounced toolA — its hook 'ask' requires explicit + // confirmation, otherwise the hook gate is silently defeated. + const aExecute = vi.fn().mockResolvedValue({ + llmContent: 'A', + returnDisplay: 'A', + }); + const bExecute = vi.fn().mockResolvedValue({ + llmContent: 'B', + returnDisplay: 'B', + }); + const tools = [ + new MockTool({ name: 'toolA', kind: Kind.Read, execute: aExecute }), + new MockTool({ name: 'toolB', kind: Kind.Read, execute: bExecute }), + ]; + const messageBus = { + request: vi + .fn() + .mockImplementation(async (req: { eventName?: string }) => ({ + type: MessageBusType.HOOK_EXECUTION_RESPONSE, + correlationId: 'pre-hook', + success: true, + output: + req.eventName === 'PreToolUse' + ? { decision: 'ask', reason: 'confirm' } + : {}, + })), + }; + const { scheduler, onToolCallsUpdate } = buildScheduler({ + tools, + messageBus, + disableHooks: false, + }); + + await scheduler.schedule( + [ + { + callId: 'a', + name: 'toolA', + args: {}, + isClientInitiated: false, + prompt_id: 'p', + }, + { + callId: 'b', + name: 'toolB', + args: {}, + isClientInitiated: false, + prompt_id: 'p', + }, + ], + new AbortController().signal, + ); + + // Both tools bounce to awaiting_approval. + await vi.waitFor(() => { + const awaiting = onToolCallsUpdate.mock.calls + .flatMap((c) => c[0] as ToolCall[]) + .filter((tc) => tc.status === 'awaiting_approval') + .map((tc) => tc.request.callId); + expect(awaiting).toContain('a'); + expect(awaiting).toContain('b'); + }); + + const toolBWaiting = onToolCallsUpdate.mock.calls + .flatMap((c) => c[0] as ToolCall[]) + .find( + (tc) => tc.request.callId === 'b' && tc.status === 'awaiting_approval', + ) as WaitingToolCall; + // Programmatic ProceedAlways on toolB runs autoApproveCompatiblePendingTools + // synchronously inside handleConfirmationResponse (awaited here). + await toolBWaiting.confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedAlways, + ); + + // toolA's hook 'ask' must still gate it: autoApprove skipped the bounced + // tool, so it never ran (and stays awaiting the user's own confirmation). + // toolB also doesn't run yet — the batch waits while toolA is non-terminal. + expect(aExecute).not.toHaveBeenCalled(); + const latestA = onToolCallsUpdate.mock.calls + .flatMap((c) => c[0] as ToolCall[]) + .filter((tc) => tc.request.callId === 'a') + .at(-1); + expect(latestA?.status).toBe('awaiting_approval'); + }); + + it('ignores ModifyWithEditor for a bounced ask info confirmation', async () => { + const execute = vi.fn().mockResolvedValue({ + llmContent: 'ok', + returnDisplay: 'ok', + }); + const getModifyContext = vi.fn(() => { + throw new Error('info confirmation must not enter editor modify flow'); + }); + const tool = Object.assign( + new MockTool({ + name: 'mockTool', + kind: Kind.Edit, + execute, + }), + { getModifyContext }, + ); + const messageBus = askMessageBus(); + const { onToolCallsUpdate } = await scheduleWithAsk({ + messageBus, + execute, + tools: [tool], + }); + + const waiting = (await waitForStatus( + onToolCallsUpdate, + 'awaiting_approval', + )) as WaitingToolCall; + await expect( + waiting.confirmationDetails.onConfirm( + ToolConfirmationOutcome.ModifyWithEditor, + ), + ).resolves.toBeUndefined(); + + expect(getModifyContext).not.toHaveBeenCalled(); + expect(execute).not.toHaveBeenCalled(); + const latest = onToolCallsUpdate.mock.calls + .flatMap((c) => c[0] as ToolCall[]) + .filter((tc) => tc.request.callId === 'ask-call') + .at(-1); + expect(latest?.status).toBe('awaiting_approval'); + }); + + it('pauses later unsafe batches while a bounced ask awaits approval', async () => { + const aExecute = vi.fn().mockResolvedValue({ + llmContent: 'A ok', + returnDisplay: 'A ok', + }); + const bExecute = vi.fn().mockResolvedValue({ + llmContent: 'B ok', + returnDisplay: 'B ok', + }); + const tools = [ + new MockTool({ name: 'toolA', kind: Kind.Edit, execute: aExecute }), + new MockTool({ name: 'toolB', kind: Kind.Edit, execute: bExecute }), + ]; + const messageBus = { + request: vi + .fn() + .mockImplementation( + async (req: { + eventName?: string; + input?: { tool_name?: string }; + }) => ({ + type: MessageBusType.HOOK_EXECUTION_RESPONSE, + correlationId: 'pre-hook', + success: true, + output: + req.eventName === 'PreToolUse' && req.input?.tool_name === 'toolA' + ? { decision: 'ask', reason: 'confirm A' } + : {}, + }), + ), + }; + const { scheduler, onAllToolCallsComplete, onToolCallsUpdate } = + buildScheduler({ tools, messageBus, disableHooks: false }); + + await scheduler.schedule( + [ + { + callId: 'a', + name: 'toolA', + args: {}, + isClientInitiated: false, + prompt_id: 'p', + }, + { + callId: 'b', + name: 'toolB', + args: {}, + isClientInitiated: false, + prompt_id: 'p', + }, + ], + new AbortController().signal, + ); + + expect(aExecute).not.toHaveBeenCalled(); + expect(bExecute).not.toHaveBeenCalled(); + + const waiting = onToolCallsUpdate.mock.calls + .flatMap((c) => c[0] as ToolCall[]) + .find( + (tc) => tc.request.callId === 'a' && tc.status === 'awaiting_approval', + ) as WaitingToolCall; + expect(waiting).toBeDefined(); + + await waiting.confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedOnce, + ); + + await vi.waitFor(() => { + expect(onAllToolCallsComplete).toHaveBeenCalled(); + }); + expect(aExecute).toHaveBeenCalledTimes(1); + expect(bExecute).toHaveBeenCalledTimes(1); + }); + + it('abort drain leaves executing siblings to finish their own abort path', async () => { + let resolveB!: (r: ToolResult) => void; + const bDone = new Promise((resolve) => { + resolveB = resolve; + }); + const bExecute = vi.fn().mockReturnValue(bDone); + const tools = [ + new MockTool({ + name: 'toolA', + kind: Kind.Read, + execute: vi.fn().mockResolvedValue({ + llmContent: 'A ok', + returnDisplay: 'A ok', + }), + }), + new MockTool({ name: 'toolB', kind: Kind.Read, execute: bExecute }), + ]; + const messageBus = { + request: vi + .fn() + .mockImplementation( + async (req: { + eventName?: string; + input?: { tool_name?: string }; + }) => ({ + type: MessageBusType.HOOK_EXECUTION_RESPONSE, + correlationId: 'pre-hook', + success: true, + output: + req.eventName === 'PreToolUse' && req.input?.tool_name === 'toolA' + ? { decision: 'ask', reason: 'confirm A' } + : {}, + }), + ), + }; + const abortController = new AbortController(); + const { scheduler, onToolCallsUpdate } = buildScheduler({ + tools, + messageBus, + disableHooks: false, + }); + + const schedulePromise = scheduler.schedule( + [ + { + callId: 'a', + name: 'toolA', + args: {}, + isClientInitiated: false, + prompt_id: 'p', + }, + { + callId: 'b', + name: 'toolB', + args: {}, + isClientInitiated: false, + prompt_id: 'p', + }, + ], + abortController.signal, + ); + + await waitForStatus(onToolCallsUpdate, 'awaiting_approval'); + await vi.waitFor(() => { + expect(bExecute).toHaveBeenCalled(); + }); + + abortController.abort(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + const latestB = onToolCallsUpdate.mock.calls + .flatMap((c) => c[0] as ToolCall[]) + .filter((tc) => tc.request.callId === 'b') + .at(-1); + expect(latestB?.status).toBe('executing'); + + resolveB({ llmContent: 'B done', returnDisplay: 'B done' }); + await schedulePromise; + }); + + it('abort drain cancels scheduled siblings behind a bounced ask', async () => { + const aExecute = vi.fn().mockResolvedValue({ + llmContent: 'A ok', + returnDisplay: 'A ok', + }); + const bExecute = vi.fn().mockResolvedValue({ + llmContent: 'B ok', + returnDisplay: 'B ok', + }); + const tools = [ + new MockTool({ name: 'toolA', kind: Kind.Edit, execute: aExecute }), + new MockTool({ name: 'toolB', kind: Kind.Edit, execute: bExecute }), + ]; + const messageBus = { + request: vi + .fn() + .mockImplementation( + async (req: { + eventName?: string; + input?: { tool_name?: string }; + }) => ({ + type: MessageBusType.HOOK_EXECUTION_RESPONSE, + correlationId: 'pre-hook', + success: true, + output: + req.eventName === 'PreToolUse' && req.input?.tool_name === 'toolA' + ? { decision: 'ask', reason: 'confirm A' } + : {}, + }), + ), + }; + const abortController = new AbortController(); + const { scheduler, onAllToolCallsComplete, onToolCallsUpdate } = + buildScheduler({ + tools, + messageBus, + disableHooks: false, + }); + + await scheduler.schedule( + [ + { + callId: 'a', + name: 'toolA', + args: {}, + isClientInitiated: false, + prompt_id: 'p', + }, + { + callId: 'b', + name: 'toolB', + args: {}, + isClientInitiated: false, + prompt_id: 'p', + }, + ], + abortController.signal, + ); + + await waitForStatus(onToolCallsUpdate, 'awaiting_approval'); + + abortController.abort(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + const latestB = onToolCallsUpdate.mock.calls + .flatMap((c) => c[0] as ToolCall[]) + .filter((tc) => tc.request.callId === 'b') + .at(-1); + expect(latestB?.status).toBe('cancelled'); + expect(onAllToolCallsComplete).toHaveBeenCalled(); + expect(aExecute).not.toHaveBeenCalled(); + expect(bExecute).not.toHaveBeenCalled(); + }); + + it('cleans bounce markers when post-ask re-execution fails before body runs', async () => { + const tracing = await import('../telemetry/session-tracing.js'); + const runInToolSpanContext = vi.mocked(tracing.runInToolSpanContext); + const messageBus = askMessageBus(); + const { scheduler, onToolCallsUpdate } = await scheduleWithAsk({ + messageBus, + }); + const waiting = (await waitForStatus( + onToolCallsUpdate, + 'awaiting_approval', + )) as WaitingToolCall; + + runInToolSpanContext.mockImplementationOnce(() => { + throw new Error('context failed before callback'); + }); + + await expect( + waiting.confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedOnce, + ), + ).rejects.toThrow('context failed before callback'); + + expect( + (scheduler as unknown as { bouncedAwaitingApproval: Set }) + .bouncedAwaitingApproval.size, + ).toBe(0); + expect( + (scheduler as unknown as { bouncedToolUseId: Map }) + .bouncedToolUseId.size, + ).toBe(0); + expect( + (scheduler as unknown as { toolSpans: Map }).toolSpans + .size, + ).toBe(0); + }); + it('blocked_on_user span ends with cancel when the user rejects (#3731 Phase 2)', async () => { // Reuses MockEditTool — same setup as the existing edit-cancellation // test in `CoreToolScheduler edit cancellation`, just instrumented for diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index b0a0c4ab87..1b7d0ec544 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -222,7 +222,6 @@ const TOOL_FAILURE_KIND_BACKGROUND_AGENT_DENIED = 'background_agent_denied'; const TOOL_SPAN_STATUS_PRE_HOOK_BLOCKED = 'Tool execution blocked by hook'; - const TOOL_SPAN_STATUS_POST_HOOK_STOPPED = 'Tool execution stopped by hook'; const TOOL_SPAN_STATUS_PERMISSION_DENIED = 'Permission denied for tool'; const TOOL_SPAN_STATUS_PERMISSION_HOOK_DENIED = @@ -1210,6 +1209,20 @@ export class CoreToolScheduler { // callIdToBatch is drained earlier when spans end, so it cannot be used // to recover the PostToolBatch AbortSignal reliably. private callIdToPostToolBatchSignal = new Map(); + // Tool calls that a PreToolUse 'ask' hook bounced from the EXECUTION + // phase back to awaiting_approval. Tracked so that, once the user + // approves, the re-execution skips BOTH the PreToolUse hook (otherwise + // the hook would return 'ask' again → infinite confirmation loop) and + // the path-unescape prelude (unescapePath is not idempotent — running + // it twice corrupts paths containing escaped metacharacters). Cleared + // on terminal state via finalizeToolSpan. + private readonly bouncedAwaitingApproval = new Set(); + // Original tool_use_id captured when a tool is bounced by a PreToolUse + // 'ask', keyed by callId. The first (bounced) attempt fires PreToolUse + // with this id; the post-approval re-execution skips PreToolUse but fires + // PostToolUse — reusing this id keeps the Pre/Post pair correlated instead + // of orphaning two events. Cleared on terminal state via finalizeToolSpan. + private readonly bouncedToolUseId = new Map(); private requestQueue: Array<{ request: ToolCallRequestInfo | ToolCallRequestInfo[]; signal: AbortSignal; @@ -1499,6 +1512,12 @@ export class CoreToolScheduler { * `setToolSpan{Failure,Cancelled,Ok}` before this call (#4321 review). */ private finalizeToolSpan(callId: string): void { + // Terminal-state cleanup: drop any PreToolUse 'ask' bounce markers so + // they never leak past the tool call's lifetime. Done unconditionally + // (before the span guard) so a bounced call is cleared even on the + // defensive no-span path. + this.bouncedAwaitingApproval.delete(callId); + this.bouncedToolUseId.delete(callId); const span = this.toolSpans.get(callId); if (!span) return; this.toolSpans.delete(callId); @@ -1587,6 +1606,22 @@ export class CoreToolScheduler { // remaining entries — the timer callback would otherwise surface // an unhandled exception (#4321 review-3 wenshao Suggestion). try { + const call = this.toolCalls.find((c) => c.request.callId === callId); + if ( + call?.status !== 'awaiting_approval' && + call?.status !== 'scheduled' + ) { + continue; + } + // Safety-net for a tool stuck awaiting approval, or still scheduled + // behind such a tool, at abort time. Do not force-terminalize + // executing siblings; their own abort path owns live output, failure + // hooks, and final status. + this.setStatusInternal( + callId, + 'cancelled', + 'Tool call cancelled by user.', + ); if (this.blockedSpans.has(callId)) { this.finalizeBlockedSpan(callId, 'aborted', 'system'); } @@ -2882,7 +2917,10 @@ export class CoreToolScheduler { this.finalizeToolSpan(callId); } else if (outcome === ToolConfirmationOutcome.ModifyWithEditor) { const waitingToolCall = toolCall as WaitingToolCall; - if (isModifiableDeclarativeTool(waitingToolCall.tool)) { + if ( + waitingToolCall.confirmationDetails.type === 'edit' && + isModifiableDeclarativeTool(waitingToolCall.tool) + ) { const modifyContext = waitingToolCall.tool.getModifyContext(signal); const editorType = this.getPreferredEditor(); if (!editorType) { @@ -2921,15 +2959,21 @@ export class CoreToolScheduler { // Normalize shell-escaped paths so the editor receives actual // filesystem paths (request.args may still hold escaped values - // since buildInvocation normalizes a structuredClone). + // since buildInvocation normalizes a structuredClone) — UNLESS this + // tool was bounced by a PreToolUse 'ask', in which case + // _executeToolCallBody already unescaped request.args in place + // before the hook fired. Unescaping again here would double-strip + // and corrupt paths containing escaped metacharacters. const normalizedArgs = { ...waitingToolCall.request.args, } as typeof waitingToolCall.request.args; - for (const key of PATH_ARG_KEYS) { - if (typeof normalizedArgs[key] === 'string') { - (normalizedArgs as Record)[key] = unescapePath( - String(normalizedArgs[key]).trim(), - ); + if (!this.bouncedAwaitingApproval.has(callId)) { + for (const key of PATH_ARG_KEYS) { + if (typeof normalizedArgs[key] === 'string') { + (normalizedArgs as Record)[key] = unescapePath( + String(normalizedArgs[key]).trim(), + ); + } } } const { updatedParams, updatedDiff } = await modifyWithEditor< @@ -3124,18 +3168,34 @@ export class CoreToolScheduler { private async attemptExecutionOfScheduledCalls( signal: AbortSignal, ): Promise { - const allCallsFinalOrScheduled = this.toolCalls.every( - (call) => - call.status === 'scheduled' || - call.status === 'cancelled' || - call.status === 'success' || - call.status === 'error', - ); + // Loop rather than execute once: a tool bounced to awaiting_approval by a + // PreToolUse 'ask' can be approved (→ 'scheduled') while a sibling in the + // same batch is still executing. The guard below fails on that pass, and + // nothing else retriggers execution once the sibling finishes — so after + // each batch drains, re-check for a newly-scheduled bounce-approved tool. + // Each iteration either drains ≥1 'scheduled' call or returns, so this + // cannot spin: a re-bounce lands back in awaiting_approval (guard fails → + // return), and a clean run leaves nothing 'scheduled' (length 0 → return). + while (true) { + const allCallsFinalOrScheduled = this.toolCalls.every( + (call) => + call.status === 'scheduled' || + call.status === 'cancelled' || + call.status === 'success' || + call.status === 'error', + ); + if (!allCallsFinalOrScheduled) { + // Something is still executing or awaiting approval; its own + // completion path (or handleConfirmationResponse) re-enters here. + return; + } - if (allCallsFinalOrScheduled) { const callsToExecute = this.toolCalls.filter( (call): call is ScheduledToolCall => call.status === 'scheduled', ); + if (callsToExecute.length === 0) { + return; + } // Partition tool calls into consecutive batches by concurrency safety. // Consecutive safe tools are grouped into parallel batches; unsafe @@ -3146,15 +3206,28 @@ export class CoreToolScheduler { for (const batch of batches) { if (batch.concurrent && batch.calls.length > 1) { await this.runConcurrently(batch.calls, signal); + if (this.hasExecutingOrAwaitingApprovalCall()) { + return; + } } else { for (const call of batch.calls) { await this.executeSingleToolCall(call, signal); + if (this.hasExecutingOrAwaitingApprovalCall()) { + return; + } } } } } } + private hasExecutingOrAwaitingApprovalCall(): boolean { + return this.toolCalls.some( + (call) => + call.status === 'executing' || call.status === 'awaiting_approval', + ); + } + /** * Execute multiple tool calls concurrently with a concurrency cap. */ @@ -3211,6 +3284,8 @@ export class CoreToolScheduler { this._executeToolCallBody(scheduledCall, signal, toolSpan), ); } catch (error) { + this.bouncedAwaitingApproval.delete(callId); + this.bouncedToolUseId.delete(callId); // _executeToolCallBody pre-sets span status (OK / FAILURE / // CANCELLED) only AFTER its main try/catch is entered. Throws // from the prelude — getMessageBus, @@ -3244,13 +3319,114 @@ export class CoreToolScheduler { ); throw error; } finally { - // _executeToolCallBody pre-sets status (OK / FAILURE / CANCELLED) via - // setToolSpan*; finalize without metadata to preserve that. - this.finalizeToolSpan(callId); + // A PreToolUse 'ask' hook can bounce this tool back to + // awaiting_approval (see bounceToAwaitingApprovalForAsk). The tool + // span must then stay open until handleConfirmationResponse resolves + // the confirmation — finalizing here would orphan it and the + // re-execution would open a second span. The re-execution consumes + // the marker before running, so the post-approval finally finalizes + // normally. Checking the marker (not a re-read of tool status) avoids + // a race where a STREAM_JSON client answers the confirmation + // synchronously and flips status to 'scheduled' before this runs. + if (!this.bouncedAwaitingApproval.has(callId)) { + // _executeToolCallBody pre-sets status (OK / FAILURE / CANCELLED) + // via setToolSpan*; finalize without metadata to preserve that. + this.finalizeToolSpan(callId); + } this.memoryMonitor?.scheduleCheck(); } } + /** + * Whether a PreToolUse 'ask' decision can be surfaced as an interactive + * TUI confirmation. Mirrors the confirmation-phase guards: a + * non-interactive CLI (unless STREAM_JSON, which can answer control + * requests) and background agents cannot prompt, so an 'ask' there must + * fall back to deny rather than hang forever in awaiting_approval. + */ + private canPromptForAskBounce(): boolean { + const isNonInteractive = + !this.config.isInteractive() && + !this.config.getExperimentalZedIntegration() && + this.config.getInputFormat() !== InputFormat.STREAM_JSON; + if (isNonInteractive) { + return false; + } + if (this.config.getShouldAvoidPermissionPrompts?.()) { + return false; + } + return true; + } + + /** + * Bounce a tool from the EXECUTION phase back to awaiting_approval so the + * user can confirm a PreToolUse 'ask' decision in the TUI. Reuses the + * standard confirmation machinery: a synthetic 'info' confirmation whose + * onConfirm routes through handleConfirmationResponse (ProceedOnce → + * re-execute, Cancel → cancelled). `hideAlwaysAllow` is set because the + * hook re-evaluates on every call, so an "always allow" rule is + * meaningless. The callId is added to `bouncedAwaitingApproval` BEFORE + * the status change so executeSingleToolCall's finally keeps the tool + * span open across the bounce and the re-execution skips the hook + + * prelude (see `_executeToolCallBody`). + */ + private bounceToAwaitingApprovalForAsk( + scheduledCall: ScheduledToolCall, + reason: string | undefined, + toolSpan: Span, + signal: AbortSignal, + ): void { + const { callId, name: toolName } = scheduledCall.request; + const canonicalName = canonicalToolName(toolName); + + this.bouncedAwaitingApproval.add(callId); + + const confirmationDetails: ToolCallConfirmationDetails = { + type: 'info', + title: `Hook requested confirmation to run ${toolName}`, + prompt: + reason || + `A PreToolUse hook requested confirmation before running ${toolName}.`, + hideAlwaysAllow: true, + onConfirm: (outcome, payload) => + this.handleConfirmationResponse( + callId, + // No real tool onConfirm — for this synthetic prompt all of the + // approve/deny handling lives in handleConfirmationResponse. + async () => {}, + outcome, + signal, + payload, + ), + }; + + this.setStatusInternal(callId, 'awaiting_approval', confirmationDetails); + + // blocked_on_user span as a child of the tool span — mirrors the + // confirmation-phase setup so walk-away aborts and finalize paths + // behave identically. + const blockedSpan = startToolBlockedOnUserSpan(toolSpan, { + tool_name: canonicalName, + call_id: callId, + }); + this.blockedSpans.set(callId, blockedSpan); + + // Surface the prompt the same way the confirmation phase does. + const messageBus = this.config.getMessageBus() as MessageBus | undefined; + if (!this.config.getDisableAllHooks() && messageBus) { + fireNotificationHook( + messageBus, + `Qwen Code needs your permission to use ${toolName}`, + NotificationType.PermissionPrompt, + 'Permission needed', + ).catch((error) => { + debugLogger.warn( + `Permission prompt notification hook failed: ${error instanceof Error ? error.message : String(error)}`, + ); + }); + } + } + private safelyAddToolInputAttributes( span: Span, toolName: string, @@ -3285,11 +3461,22 @@ export class CoreToolScheduler { const invocation = scheduledCall.invocation; const toolInput = scheduledCall.request.args as Record; - // Normalize shell-escaped path params so hooks operate on actual filesystem - // paths, matching the normalization done in tool validation. - for (const key of PATH_ARG_KEYS) { - if (typeof toolInput[key] === 'string') { - toolInput[key] = unescapePath(String(toolInput[key]).trim()); + // Re-execution after the user approved a PreToolUse 'ask' bounce: the + // hook already ran and the user already confirmed. Consuming the marker + // here (a) lets executeSingleToolCall's finally finalize the span + // normally for this run, and (b) signals that we must skip BOTH the + // hook re-fire below (otherwise the hook returns 'ask' again → infinite + // confirmation loop) and the path-unescape prelude (unescapePath is not + // idempotent — running it twice corrupts paths with escaped metachars). + const isPostAskReexecution = this.bouncedAwaitingApproval.delete(callId); + + if (!isPostAskReexecution) { + // Normalize shell-escaped path params so hooks operate on actual + // filesystem paths, matching the normalization done in tool validation. + for (const key of PATH_ARG_KEYS) { + if (typeof toolInput[key] === 'string') { + toolInput[key] = unescapePath(String(toolInput[key]).trim()); + } } } @@ -3304,15 +3491,22 @@ export class CoreToolScheduler { ); } - // Generate unique tool_use_id for hook tracking - const toolUseId = generateToolUseId(); + // Generate unique tool_use_id for hook tracking. On a post-'ask' + // re-execution, reuse the id from the first (bounced) attempt so the + // PreToolUse event fired then pairs with the PostToolUse event fired now + // — a fresh id would leave both events orphaned for consumers that + // correlate Pre/Post by tool_use_id (audit trails, metrics). + const toolUseId = isPostAskReexecution + ? (this.bouncedToolUseId.get(callId) ?? generateToolUseId()) + : generateToolUseId(); // Get MessageBus for hook execution const messageBus = this.config.getMessageBus() as MessageBus | undefined; const hooksEnabled = !this.config.getDisableAllHooks(); - // PreToolUse Hook - if (hooksEnabled && messageBus) { + // PreToolUse Hook — skipped on a post-'ask' re-execution (the hook + // already ran and the user already confirmed; re-firing would loop). + if (hooksEnabled && messageBus && !isPostAskReexecution) { // Convert ApprovalMode to permission_mode string for hooks const permissionMode = this.config.getApprovalMode(); const preHookResult = await this.withHookSpan( @@ -3349,7 +3543,33 @@ export class CoreToolScheduler { }, ); if (!preHookResult.shouldProceed) { - // Hook blocked the execution + // A PreToolUse hook returning permissionDecision:'ask' wants the + // user to confirm in the TUI before the tool runs. When we can + // prompt, bounce the tool into the existing awaiting_approval flow + // instead of denying it. 'denied'/'stop' (and 'ask' in a + // non-interactive/background context where we cannot prompt) keep + // the original deny-as-error behavior. + if ( + preHookResult.blockType === 'ask' && + !signal.aborted && + this.canPromptForAskBounce() + ) { + // Mirror the confirmation-phase abort re-check: never open a + // transient awaiting_approval (flashing a confirmation nobody can + // answer) on an already-aborted signal — fall through to deny. + // Preserve the tool_use_id so the post-approval re-execution + // reuses it (see the toolUseId comment above). + this.bouncedToolUseId.set(callId, toolUseId); + this.bounceToAwaitingApprovalForAsk( + scheduledCall, + preHookResult.blockReason, + span, + signal, + ); + return; + } + + // Hook blocked the execution. const blockMessage = preHookResult.blockReason || 'Tool execution blocked by hook'; const errorResponse = createErrorResponse( @@ -4485,7 +4705,13 @@ export class CoreToolScheduler { const pendingTools = this.toolCalls.filter( (call) => call.status === 'awaiting_approval' && - call.request.callId !== triggeringCallId, + call.request.callId !== triggeringCallId && + // A tool bounced by a PreToolUse 'ask' must NOT be auto-approved as a + // side effect of approving a sibling: the hook explicitly requested + // confirmation, and re-execution skips the hook — auto-approving here + // would silently defeat the hook's gate. It requires its own explicit + // user confirmation. + !this.bouncedAwaitingApproval.has(call.request.callId), ) as WaitingToolCall[]; for (const pendingTool of pendingTools) {