diff --git a/integration-tests/hooks-system.test.ts b/integration-tests/hooks-system.test.ts index a4be2880e0..0840b2cdb7 100644 --- a/integration-tests/hooks-system.test.ts +++ b/integration-tests/hooks-system.test.ts @@ -7,7 +7,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { TestRig, poll, normalizePath } from './test-helper.js'; import { join } from 'node:path'; -import { writeFileSync } from 'node:fs'; +import { writeFileSync, existsSync, mkdirSync } from 'node:fs'; import os from 'node:os'; describe('Hooks System Integration', () => { @@ -2247,10 +2247,10 @@ console.log(JSON.stringify({ describe('Hooks "ask" Decision Integration', () => { it( 'should force confirmation prompt when hook returns "ask" decision even in YOLO mode', - { timeout: 20000 }, + { timeout: 60000 }, async () => { const testName = - 'should force confirmation prompt when hook returns "ask" decision'; + 'should force confirmation prompt when hook returns "ask" decision even in YOLO mode'; // 1. Setup hook script that returns 'ask' decision const hookOutput = { @@ -2280,6 +2280,9 @@ console.log(JSON.stringify({ tools: { approval: 'yolo', }, + general: { + enableAutoUpdateNotification: false, + }, hooksConfig: { enabled: true, }, @@ -2300,16 +2303,31 @@ console.log(JSON.stringify({ }, }); + // Bypass terminal setup prompt and other startup banners + const stateDir = join(rig.homeDir!, '.gemini'); + if (!existsSync(stateDir)) mkdirSync(stateDir, { recursive: true }); + writeFileSync( + join(stateDir, 'state.json'), + JSON.stringify({ + terminalSetupPromptShown: true, + hasSeenScreenReaderNudge: true, + tipsShown: 100, + }), + ); + // 3. Run interactive and verify prompt appears despite YOLO mode const run = await rig.runInteractive(); + // Wait for prompt to appear + await run.expectText('Type your message', 30000); + // Send prompt that will trigger write_file await run.type('Create a file called ask-test.txt with content "test"'); await run.type('\r'); // Wait for the FORCED confirmation prompt to appear // It should contain the system message from the hook - await run.expectText('Confirmation forced by security hook', 15000); + await run.expectText('Confirmation forced by security hook', 30000); await run.expectText('Allow', 5000); // 4. Approve the permission @@ -2317,7 +2335,7 @@ console.log(JSON.stringify({ await run.type('\r'); // Wait for command to execute - await run.expectText('approved.txt', 15000); + await run.expectText('approved.txt', 30000); // Should find the tool call const foundWriteFile = await rig.waitForToolCall('write_file'); @@ -2329,80 +2347,106 @@ console.log(JSON.stringify({ }, ); - it('should allow cancelling when hook forces "ask" decision', async () => { - const testName = - 'should allow cancelling when hook forces "ask" decision'; - const hookOutput = { - decision: 'ask', - systemMessage: 'Confirmation forced for cancellation test', - hookSpecificOutput: { - hookEventName: 'BeforeTool', - }, - }; - - const hookScript = `console.log(JSON.stringify(${JSON.stringify( - hookOutput, - )}));`; - - const scriptPath = join( - os.tmpdir(), - 'gemini-cli-tests-ask-cancel-hook.js', - ); - writeFileSync(scriptPath, hookScript); - - rig.setup(testName, { - fakeResponsesPath: join( - import.meta.dirname, - 'hooks-system.allow-tool.responses', - ), - settings: { - debugMode: true, - tools: { - approval: 'yolo', + it( + 'should allow cancelling when hook forces "ask" decision', + { timeout: 60000 }, + async () => { + const testName = + 'should allow cancelling when hook forces "ask" decision'; + const hookOutput = { + decision: 'ask', + systemMessage: 'Confirmation forced for cancellation test', + hookSpecificOutput: { + hookEventName: 'BeforeTool', }, - hooksConfig: { - enabled: true, + }; + + const hookScript = `console.log(JSON.stringify(${JSON.stringify( + hookOutput, + )}));`; + + const scriptPath = join( + os.tmpdir(), + 'gemini-cli-tests-ask-cancel-hook.js', + ); + writeFileSync(scriptPath, hookScript); + + rig.setup(testName, { + fakeResponsesPath: join( + import.meta.dirname, + 'hooks-system.allow-tool.responses', + ), + settings: { + debugMode: true, + tools: { + approval: 'yolo', + }, + general: { + enableAutoUpdateNotification: false, + }, + hooksConfig: { + enabled: true, + }, + hooks: { + BeforeTool: [ + { + matcher: 'write_file', + hooks: [ + { + type: 'command', + command: `node "${scriptPath}"`, + timeout: 5000, + }, + ], + }, + ], + }, }, - hooks: { - BeforeTool: [ - { - matcher: 'write_file', - hooks: [ - { - type: 'command', - command: `node "${scriptPath}"`, - timeout: 5000, - }, - ], - }, - ], - }, - }, - }); + }); - const run = await rig.runInteractive(); + // Bypass terminal setup prompt and other startup banners + const stateDir = join(rig.homeDir!, '.gemini'); + if (!existsSync(stateDir)) mkdirSync(stateDir, { recursive: true }); + writeFileSync( + join(stateDir, 'state.json'), + JSON.stringify({ + terminalSetupPromptShown: true, + hasSeenScreenReaderNudge: true, + tipsShown: 100, + }), + ); - await run.type( - 'Create a file called cancel-test.txt with content "test"', - ); - await run.type('\r'); + const run = await rig.runInteractive(); - await run.expectText('Confirmation forced for cancellation test', 15000); + // Wait for prompt to appear + await run.expectText('Type your message', 30000); - // 4. Deny the permission using option 4 - await run.type('4'); - await run.type('\r'); + await run.type( + 'Create a file called cancel-test.txt with content "test"', + ); + await run.type('\r'); - // Wait for cancellation message - await run.expectText('Cancelled', 10000); + await run.expectText( + 'Confirmation forced for cancellation test', + 30000, + ); - // Tool should NOT be called successfully - const toolLogs = rig.readToolLogs(); - const writeFileCalls = toolLogs.filter( - (t) => - t.toolRequest.name === 'write_file' && t.toolRequest.success === true, - ); - expect(writeFileCalls).toHaveLength(0); - }); + // 4. Deny the permission using option 4 + await run.type('4'); + await run.type('\r'); + + // Wait for cancellation message + await run.expectText('Cancelled', 15000); + + // Tool should NOT be called successfully + const toolLogs = rig.readToolLogs(); + const writeFileCalls = toolLogs.filter( + (t) => + t.toolRequest.name === 'write_file' && + t.toolRequest.success === true, + ); + expect(writeFileCalls).toHaveLength(0); + }, + ); }); }); diff --git a/packages/core/src/agents/remote-invocation.ts b/packages/core/src/agents/remote-invocation.ts index 53d582e92c..40dd142638 100644 --- a/packages/core/src/agents/remote-invocation.ts +++ b/packages/core/src/agents/remote-invocation.ts @@ -6,7 +6,6 @@ import { BaseToolInvocation, - type ForcedToolDecision, type ToolConfirmationOutcome, type ToolResult, type ToolCallConfirmationDetails, @@ -135,7 +134,6 @@ export class RemoteAgentInvocation extends BaseToolInvocation< protected override async getConfirmationDetails( _abortSignal: AbortSignal, - _forcedDecision?: ForcedToolDecision, ): Promise { // For now, always require confirmation for remote agents until we have a policy system for them. return { diff --git a/packages/core/src/agents/subagent-tool.test.ts b/packages/core/src/agents/subagent-tool.test.ts index 8664e3a959..622fd054f0 100644 --- a/packages/core/src/agents/subagent-tool.test.ts +++ b/packages/core/src/agents/subagent-tool.test.ts @@ -112,7 +112,6 @@ describe('SubAgentInvocation', () => { expect(result).toBe(false); expect(mockInnerInvocation.shouldConfirmExecute).toHaveBeenCalledWith( abortSignal, - undefined, ); expect(MockSubagentToolWrapper).toHaveBeenCalledWith( testDefinition, @@ -157,7 +156,6 @@ describe('SubAgentInvocation', () => { expect(result).toBe(confirmationDetails); expect(mockInnerInvocation.shouldConfirmExecute).toHaveBeenCalledWith( abortSignal, - undefined, ); expect(MockSubagentToolWrapper).toHaveBeenCalledWith( testRemoteDefinition, diff --git a/packages/core/src/agents/subagent-tool.ts b/packages/core/src/agents/subagent-tool.ts index c580576ac0..21a3864160 100644 --- a/packages/core/src/agents/subagent-tool.ts +++ b/packages/core/src/agents/subagent-tool.ts @@ -9,7 +9,6 @@ import { Kind, type ToolInvocation, type ToolResult, - type ForcedToolDecision, BaseToolInvocation, type ToolCallConfirmationDetails, isTool, @@ -146,13 +145,12 @@ class SubAgentInvocation extends BaseToolInvocation { override async shouldConfirmExecute( abortSignal: AbortSignal, - forcedDecision?: ForcedToolDecision, ): Promise { const invocation = this.buildSubInvocation( this.definition, this.withUserHints(this.params), ); - return invocation.shouldConfirmExecute(abortSignal, forcedDecision); + return invocation.shouldConfirmExecute(abortSignal); } async execute( diff --git a/packages/core/src/confirmation-bus/message-bus.ts b/packages/core/src/confirmation-bus/message-bus.ts index 33aa10355b..2d66ec681c 100644 --- a/packages/core/src/confirmation-bus/message-bus.ts +++ b/packages/core/src/confirmation-bus/message-bus.ts @@ -52,15 +52,18 @@ export class MessageBus extends EventEmitter { } if (message.type === MessageBusType.TOOL_CONFIRMATION_REQUEST) { - const { decision } = await this.policyEngine.check( + const { decision: policyDecision } = await this.policyEngine.check( message.toolCall, message.serverName, message.toolAnnotations, message.subagent, ); + const decision = message.forcedDecision ?? policyDecision; + switch (decision) { case PolicyDecision.ALLOW: + case 'allow': // Directly emit the response instead of recursive publish this.emitMessage({ type: MessageBusType.TOOL_CONFIRMATION_RESPONSE, @@ -69,6 +72,7 @@ export class MessageBus extends EventEmitter { }); break; case PolicyDecision.DENY: + case 'deny': // Emit both rejection and response messages this.emitMessage({ type: MessageBusType.TOOL_POLICY_REJECTION, @@ -81,6 +85,7 @@ export class MessageBus extends EventEmitter { }); break; case PolicyDecision.ASK_USER: + case 'ask_user': // Pass through to UI for user confirmation if any listeners exist. // If no listeners are registered (e.g., headless/ACP flows), // immediately request user confirmation to avoid long timeouts. diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index 62faca75a4..f4978a0c46 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -46,6 +46,10 @@ export interface ToolConfirmationRequest { * Optional rich details for the confirmation UI (diffs, counts, etc.) */ details?: SerializableConfirmationDetails; + /** + * Optional decision to force for this tool call, bypassing the policy engine. + */ + forcedDecision?: 'allow' | 'deny' | 'ask_user'; } export interface ToolConfirmationResponse { diff --git a/packages/core/src/core/turn.ts b/packages/core/src/core/turn.ts index 6b6acf37ea..9c0e536c48 100644 --- a/packages/core/src/core/turn.ts +++ b/packages/core/src/core/turn.ts @@ -16,7 +16,6 @@ import { import type { ToolCallConfirmationDetails, ToolResult, - ForcedToolDecision, } from '../tools/tools.js'; import { getResponseText } from '../utils/partUtils.js'; import { reportError } from '../utils/errorReporting.js'; @@ -47,7 +46,6 @@ export interface ServerTool { shouldConfirmExecute( params: Record, abortSignal: AbortSignal, - forcedDecision?: ForcedToolDecision, ): Promise; } diff --git a/packages/core/src/scheduler/scheduler.ts b/packages/core/src/scheduler/scheduler.ts index cde75beda7..8b841ee69e 100644 --- a/packages/core/src/scheduler/scheduler.ts +++ b/packages/core/src/scheduler/scheduler.ts @@ -616,6 +616,7 @@ export class Scheduler { if (beforeOutput.isAskDecision()) { hookDecision = 'ask'; hookSystemMessage = beforeOutput.systemMessage; + toolCall.request.forcedAsk = true; } if (beforeOutput instanceof BeforeToolHookOutput) { diff --git a/packages/core/src/test-utils/mock-tool.ts b/packages/core/src/test-utils/mock-tool.ts index 0d49dfad29..97f41a4fb0 100644 --- a/packages/core/src/test-utils/mock-tool.ts +++ b/packages/core/src/test-utils/mock-tool.ts @@ -12,7 +12,6 @@ import { BaseDeclarativeTool, BaseToolInvocation, Kind, - type ForcedToolDecision, type ToolCallConfirmationDetails, type ToolInvocation, type ToolLiveOutput, @@ -31,7 +30,6 @@ interface MockToolOptions { shouldConfirmExecute?: ( params: { [key: string]: unknown }, signal: AbortSignal, - forcedDecision?: ForcedToolDecision, ) => Promise; execute?: ( params: { [key: string]: unknown }, @@ -70,13 +68,8 @@ class MockToolInvocation extends BaseToolInvocation< override shouldConfirmExecute( abortSignal: AbortSignal, - forcedDecision?: ForcedToolDecision, ): Promise { - return this.tool.shouldConfirmExecute( - this.params, - abortSignal, - forcedDecision, - ); + return this.tool.shouldConfirmExecute(this.params, abortSignal); } getDescription(): string { @@ -94,7 +87,6 @@ export class MockTool extends BaseDeclarativeTool< readonly shouldConfirmExecute: ( params: { [key: string]: unknown }, signal: AbortSignal, - forcedDecision?: ForcedToolDecision, ) => Promise; readonly execute: ( @@ -177,7 +169,6 @@ export class MockModifiableToolInvocation extends BaseToolInvocation< override async shouldConfirmExecute( _abortSignal: AbortSignal, - _forcedDecision?: ForcedToolDecision, ): Promise { if (this.tool.shouldConfirm) { return { diff --git a/packages/core/src/tools/ask-user.ts b/packages/core/src/tools/ask-user.ts index cba0a4f6c8..621d4c10d1 100644 --- a/packages/core/src/tools/ask-user.ts +++ b/packages/core/src/tools/ask-user.ts @@ -7,7 +7,6 @@ import { BaseDeclarativeTool, BaseToolInvocation, - type ForcedToolDecision, type ToolResult, Kind, type ToolAskUserConfirmationDetails, @@ -127,7 +126,6 @@ export class AskUserInvocation extends BaseToolInvocation< override async shouldConfirmExecute( _abortSignal: AbortSignal, - _forcedDecision?: ForcedToolDecision, ): Promise { const normalizedQuestions = this.params.questions.map((q) => ({ ...q, diff --git a/packages/core/src/tools/confirmation-policy.test.ts b/packages/core/src/tools/confirmation-policy.test.ts index 361ab3d689..54ba4de5a8 100644 --- a/packages/core/src/tools/confirmation-policy.test.ts +++ b/packages/core/src/tools/confirmation-policy.test.ts @@ -191,5 +191,39 @@ describe('Tool Confirmation Policy Updates', () => { } }, ); + + it('should skip confirmation in AUTO_EDIT mode', async () => { + vi.spyOn(mockConfig, 'getApprovalMode').mockReturnValue( + ApprovalMode.AUTO_EDIT, + ); + const tool = create(mockConfig, mockMessageBus); + const invocation = tool.build(params as any); + + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + + expect(confirmation).toBe(false); + }); + + it('should NOT skip confirmation in AUTO_EDIT mode if forcedDecision is ask_user', async () => { + vi.spyOn(mockConfig, 'getApprovalMode').mockReturnValue( + ApprovalMode.AUTO_EDIT, + ); + const tool = create(mockConfig, mockMessageBus); + const invocation = tool.build(params as any); + + // Mock getMessageBusDecision to return ask_user + vi.spyOn(invocation as any, 'getMessageBusDecision').mockResolvedValue( + 'ask_user', + ); + + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + 'ask_user', + ); + + expect(confirmation).not.toBe(false); + }); }); }); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 8810a07330..5918c994af 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -13,7 +13,6 @@ import { BaseDeclarativeTool, BaseToolInvocation, Kind, - type ForcedToolDecision, type ToolCallConfirmationDetails, type ToolConfirmationOutcome, type ToolEditConfirmationDetails, @@ -27,7 +26,7 @@ import { ToolErrorType } from './tool-error.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { isNodeError } from '../utils/errors.js'; import type { Config } from '../config/config.js'; -import { ApprovalMode } from '../policy/types.js'; +import { type ApprovalMode } from '../policy/types.js'; import { CoreToolCallStatus } from '../scheduler/types.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; @@ -700,26 +699,26 @@ class EditToolInvocation ); } + protected override get respectsAutoEdit(): boolean { + return true; + } + + protected override getApprovalMode(): ApprovalMode { + return this.config.getApprovalMode(); + } + /** * Handles the confirmation prompt for the Edit tool in the CLI. * It needs to calculate the diff to show the user. */ protected override async getConfirmationDetails( - abortSignal: AbortSignal, - forcedDecision?: ForcedToolDecision, + _abortSignal: AbortSignal, ): Promise { - if ( - this.config.getApprovalMode() === ApprovalMode.AUTO_EDIT && - forcedDecision !== 'ask_user' - ) { - return false; - } - let editData: CalculatedEdit; try { - editData = await this.calculateEdit(this.params, abortSignal); + editData = await this.calculateEdit(this.params, _abortSignal); } catch (error) { - if (abortSignal.aborted) { + if (_abortSignal.aborted) { throw error; } const errorMsg = error instanceof Error ? error.message : String(error); diff --git a/packages/core/src/tools/enter-plan-mode.ts b/packages/core/src/tools/enter-plan-mode.ts index 6a6b03f5e6..dee8569669 100644 --- a/packages/core/src/tools/enter-plan-mode.ts +++ b/packages/core/src/tools/enter-plan-mode.ts @@ -7,7 +7,6 @@ import { BaseDeclarativeTool, BaseToolInvocation, - type ForcedToolDecision, type ToolResult, Kind, type ToolInfoConfirmationDetails, @@ -86,10 +85,8 @@ export class EnterPlanModeInvocation extends BaseToolInvocation< override async shouldConfirmExecute( abortSignal: AbortSignal, - forcedDecision?: ForcedToolDecision, ): Promise { - const decision = - forcedDecision ?? (await this.getMessageBusDecision(abortSignal)); + const decision = await this.getMessageBusDecision(abortSignal); if (decision === 'allow') { return false; } diff --git a/packages/core/src/tools/exit-plan-mode.ts b/packages/core/src/tools/exit-plan-mode.ts index a1d5e81472..4eec5124ac 100644 --- a/packages/core/src/tools/exit-plan-mode.ts +++ b/packages/core/src/tools/exit-plan-mode.ts @@ -7,7 +7,6 @@ import { BaseDeclarativeTool, BaseToolInvocation, - type ForcedToolDecision, type ToolResult, Kind, type ToolExitPlanModeConfirmationDetails, @@ -119,7 +118,6 @@ export class ExitPlanModeInvocation extends BaseToolInvocation< override async shouldConfirmExecute( abortSignal: AbortSignal, - forcedDecision?: ForcedToolDecision, ): Promise { const resolvedPlanPath = this.getResolvedPlanPath(); @@ -139,8 +137,7 @@ export class ExitPlanModeInvocation extends BaseToolInvocation< return false; } - const decision = - forcedDecision ?? (await this.getMessageBusDecision(abortSignal)); + const decision = await this.getMessageBusDecision(abortSignal); if (decision === 'deny') { throw new Error( `Tool execution for "${ diff --git a/packages/core/src/tools/get-internal-docs.ts b/packages/core/src/tools/get-internal-docs.ts index b185b24ae6..23bda8f4dd 100644 --- a/packages/core/src/tools/get-internal-docs.ts +++ b/packages/core/src/tools/get-internal-docs.ts @@ -10,7 +10,6 @@ import { Kind, type ToolInvocation, type ToolResult, - type ForcedToolDecision, type ToolCallConfirmationDetails, } from './tools.js'; import { GET_INTERNAL_DOCS_TOOL_NAME } from './tool-names.js'; @@ -86,7 +85,6 @@ class GetInternalDocsInvocation extends BaseToolInvocation< override async shouldConfirmExecute( _abortSignal: AbortSignal, - _forcedDecision?: ForcedToolDecision, ): Promise { return false; } diff --git a/packages/core/src/tools/mcp-tool.ts b/packages/core/src/tools/mcp-tool.ts index 4e93835a2d..f67d1f9bea 100644 --- a/packages/core/src/tools/mcp-tool.ts +++ b/packages/core/src/tools/mcp-tool.ts @@ -11,7 +11,6 @@ import { BaseToolInvocation, Kind, ToolConfirmationOutcome, - type ForcedToolDecision, type ToolCallConfirmationDetails, type ToolInvocation, type ToolMcpConfirmationDetails, @@ -193,7 +192,6 @@ export class DiscoveredMCPToolInvocation extends BaseToolInvocation< protected override async getConfirmationDetails( _abortSignal: AbortSignal, - _forcedDecision?: ForcedToolDecision, ): Promise { const serverAllowListKey = this.serverName; const toolAllowListKey = `${this.serverName}.${this.serverToolName}`; diff --git a/packages/core/src/tools/memoryTool.ts b/packages/core/src/tools/memoryTool.ts index 41b1572623..68a0942a53 100644 --- a/packages/core/src/tools/memoryTool.ts +++ b/packages/core/src/tools/memoryTool.ts @@ -9,7 +9,6 @@ import { BaseToolInvocation, Kind, ToolConfirmationOutcome, - type ForcedToolDecision, type ToolEditConfirmationDetails, type ToolResult, } from './tools.js'; @@ -164,7 +163,6 @@ class MemoryToolInvocation extends BaseToolInvocation< protected override async getConfirmationDetails( _abortSignal: AbortSignal, - _forcedDecision?: ForcedToolDecision, ): Promise { const memoryFilePath = getGlobalMemoryFilePath(); const allowlistKey = memoryFilePath; diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 778f4b8f0f..4ea83b0af4 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -16,7 +16,6 @@ import { BaseToolInvocation, ToolConfirmationOutcome, Kind, - type ForcedToolDecision, type ToolInvocation, type ToolResult, type ToolCallConfirmationDetails, @@ -110,7 +109,6 @@ export class ShellToolInvocation extends BaseToolInvocation< protected override async getConfirmationDetails( _abortSignal: AbortSignal, - _forcedDecision?: ForcedToolDecision, ): Promise { const command = stripShellWrapper(this.params.command); diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index b68579d6d7..e224c31285 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -18,7 +18,7 @@ import { type ToolConfirmationResponse, type Question, } from '../confirmation-bus/types.js'; -import { type ApprovalMode } from '../policy/types.js'; +import { ApprovalMode } from '../policy/types.js'; import type { SubagentProgress } from '../agents/types.js'; /** @@ -58,10 +58,10 @@ export interface ToolInvocation< * @param abortSignal An AbortSignal that can be used to cancel the confirmation request. * @returns A ToolCallConfirmationDetails object if confirmation is required, or false if not. */ - shouldConfirmExecute: ( + shouldConfirmExecute( abortSignal: AbortSignal, forcedDecision?: ForcedToolDecision, - ) => Promise; + ): Promise; /** * Executes the tool with the validated parameters. @@ -111,6 +111,14 @@ export abstract class BaseToolInvocation< abortSignal: AbortSignal, forcedDecision?: ForcedToolDecision, ): Promise { + if ( + this.respectsAutoEdit && + this.getApprovalMode() === ApprovalMode.AUTO_EDIT && + forcedDecision !== 'ask_user' + ) { + return false; + } + const decision = forcedDecision ?? (await this.getMessageBusDecision(abortSignal)); if (decision === 'allow') { @@ -126,11 +134,28 @@ export abstract class BaseToolInvocation< } if (decision === 'ask_user') { - return this.getConfirmationDetails(abortSignal, forcedDecision); + return this.getConfirmationDetails(abortSignal); } // Default to confirmation details if decision is unknown (should not happen with exhaustive policy) - return this.getConfirmationDetails(abortSignal, forcedDecision); + return this.getConfirmationDetails(abortSignal); + } + + /** + * Whether this tool respects the AUTO_EDIT approval mode. + * Subclasses should override this and return true if they want to skip + * confirmation when the session is in AUTO_EDIT mode. + */ + protected get respectsAutoEdit(): boolean { + return false; + } + + /** + * Returns the current approval mode from the tool configuration. + * Subclasses should override this and return the actual approval mode. + */ + protected getApprovalMode(): ApprovalMode { + return ApprovalMode.DEFAULT; } /** @@ -174,7 +199,6 @@ export abstract class BaseToolInvocation< */ protected async getConfirmationDetails( _abortSignal: AbortSignal, - _forcedDecision?: ForcedToolDecision, ): Promise { if (!this.messageBus) { return false; @@ -193,6 +217,7 @@ export abstract class BaseToolInvocation< protected getMessageBusDecision( abortSignal: AbortSignal, + forcedDecision?: ForcedToolDecision, ): Promise { if (!this.messageBus || !this._toolName) { // If there's no message bus, we can't make a decision, so we allow. @@ -211,6 +236,7 @@ export abstract class BaseToolInvocation< }, serverName: this._serverName, toolAnnotations: this._toolAnnotations, + forcedDecision, }; return new Promise((resolve) => { diff --git a/packages/core/src/tools/web-fetch.ts b/packages/core/src/tools/web-fetch.ts index fb196794c4..f69d51b2f7 100644 --- a/packages/core/src/tools/web-fetch.ts +++ b/packages/core/src/tools/web-fetch.ts @@ -8,7 +8,6 @@ import { BaseDeclarativeTool, BaseToolInvocation, Kind, - type ForcedToolDecision, type ToolCallConfirmationDetails, type ToolInvocation, type ToolResult, @@ -18,7 +17,7 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { ToolErrorType } from './tool-error.js'; import { getErrorMessage } from '../utils/errors.js'; import type { Config } from '../config/config.js'; -import { ApprovalMode } from '../policy/types.js'; +import { type ApprovalMode } from '../policy/types.js'; import { getResponseText } from '../utils/partUtils.js'; import { fetchWithTimeout, isPrivateIp } from '../utils/fetch.js'; import { truncateString } from '../utils/textUtils.js'; @@ -292,16 +291,17 @@ ${textContent} return `Processing URLs and instructions from prompt: "${displayPrompt}"`; } + protected override get respectsAutoEdit(): boolean { + return true; + } + + protected override getApprovalMode(): ApprovalMode { + return this.config.getApprovalMode(); + } + protected override async getConfirmationDetails( _abortSignal: AbortSignal, - _forcedDecision?: ForcedToolDecision, ): Promise { - // Check for AUTO_EDIT approval mode. This tool has a specific behavior - // where ProceedAlways switches the entire session to AUTO_EDIT. - if (this.config.getApprovalMode() === ApprovalMode.AUTO_EDIT) { - return false; - } - let urls: string[] = []; let prompt = this.params.prompt || ''; diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 4aa1cf48a0..85f46089ce 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -11,13 +11,12 @@ import os from 'node:os'; import * as Diff from 'diff'; import { WRITE_FILE_TOOL_NAME, WRITE_FILE_DISPLAY_NAME } from './tool-names.js'; import type { Config } from '../config/config.js'; -import { ApprovalMode } from '../policy/types.js'; +import { type ApprovalMode } from '../policy/types.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind, - type ForcedToolDecision, type FileDiff, type ToolCallConfirmationDetails, type ToolEditConfirmationDetails, @@ -173,22 +172,22 @@ class WriteFileToolInvocation extends BaseToolInvocation< return `Writing to ${shortenPath(relativePath)}`; } - protected override async getConfirmationDetails( - abortSignal: AbortSignal, - forcedDecision?: ForcedToolDecision, - ): Promise { - if ( - this.config.getApprovalMode() === ApprovalMode.AUTO_EDIT && - forcedDecision !== 'ask_user' - ) { - return false; - } + protected override get respectsAutoEdit(): boolean { + return true; + } + protected override getApprovalMode(): ApprovalMode { + return this.config.getApprovalMode(); + } + + protected override async getConfirmationDetails( + _abortSignal: AbortSignal, + ): Promise { const correctedContentResult = await getCorrectedFileContent( this.config, this.resolvedPath, this.params.content, - abortSignal, + _abortSignal, ); if (correctedContentResult.error) {