From e64146914aee3e24d4f85c83a694ec4c10b7bb62 Mon Sep 17 00:00:00 2001 From: Jacob Richman Date: Fri, 19 Dec 2025 17:09:43 -0800 Subject: [PATCH] fix(policy): fix bug where accepting-edits continued after it was turned off (#15351) --- .../src/tools/confirmation-policy.test.ts | 188 ++++++++++++++++++ packages/core/src/tools/edit.ts | 5 +- packages/core/src/tools/smart-edit.ts | 5 +- packages/core/src/tools/web-fetch.ts | 5 +- packages/core/src/tools/write-file.ts | 5 +- 5 files changed, 204 insertions(+), 4 deletions(-) create mode 100644 packages/core/src/tools/confirmation-policy.test.ts diff --git a/packages/core/src/tools/confirmation-policy.test.ts b/packages/core/src/tools/confirmation-policy.test.ts new file mode 100644 index 0000000000..53e04f6dec --- /dev/null +++ b/packages/core/src/tools/confirmation-policy.test.ts @@ -0,0 +1,188 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/* eslint-disable @typescript-eslint/no-explicit-any */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { EditTool } from './edit.js'; +import { SmartEditTool } from './smart-edit.js'; +import { WriteFileTool } from './write-file.js'; +import { WebFetchTool } from './web-fetch.js'; +import { ToolConfirmationOutcome } from './tools.js'; +import { ApprovalMode } from '../policy/types.js'; +import { MessageBusType } from '../confirmation-bus/types.js'; +import type { MessageBus } from '../confirmation-bus/message-bus.js'; +import type { Config } from '../config/config.js'; +import path from 'node:path'; +import fs from 'node:fs'; +import os from 'node:os'; + +// Mock telemetry loggers to avoid failures +vi.mock('../telemetry/loggers.js', () => ({ + logSmartEditStrategy: vi.fn(), + logSmartEditCorrectionEvent: vi.fn(), + logFileOperation: vi.fn(), +})); + +describe('Tool Confirmation Policy Updates', () => { + let mockConfig: any; + let mockMessageBus: MessageBus; + const rootDir = path.join( + os.tmpdir(), + `gemini-cli-policy-test-${Date.now()}`, + ); + + beforeEach(() => { + if (!fs.existsSync(rootDir)) { + fs.mkdirSync(rootDir, { recursive: true }); + } + + mockMessageBus = { + publish: vi.fn(), + subscribe: vi.fn(), + unsubscribe: vi.fn(), + } as unknown as MessageBus; + + mockConfig = { + getTargetDir: () => rootDir, + getApprovalMode: vi.fn().mockReturnValue(ApprovalMode.DEFAULT), + setApprovalMode: vi.fn(), + getFileSystemService: () => ({ + readTextFile: vi.fn().mockImplementation((p) => { + if (fs.existsSync(p)) { + return fs.readFileSync(p, 'utf8'); + } + return 'existing content'; + }), + writeTextFile: vi.fn().mockImplementation((p, c) => { + fs.writeFileSync(p, c); + }), + }), + getFileService: () => ({}), + getFileFilteringOptions: () => ({}), + getGeminiClient: () => ({}), + getBaseLlmClient: () => ({}), + getIdeMode: () => false, + getWorkspaceContext: () => ({ + isPathWithinWorkspace: () => true, + getDirectories: () => [rootDir], + }), + }; + }); + + afterEach(() => { + if (fs.existsSync(rootDir)) { + fs.rmSync(rootDir, { recursive: true, force: true }); + } + vi.restoreAllMocks(); + }); + + const tools = [ + { + name: 'EditTool', + create: (config: Config, bus: MessageBus) => new EditTool(config, bus), + params: { + file_path: 'test.txt', + old_string: 'existing', + new_string: 'new', + }, + }, + { + name: 'SmartEditTool', + create: (config: Config, bus: MessageBus) => + new SmartEditTool(config, bus), + params: { + file_path: 'test.txt', + instruction: 'change content', + old_string: 'existing', + new_string: 'new', + }, + }, + { + name: 'WriteFileTool', + create: (config: Config, bus: MessageBus) => + new WriteFileTool(config, bus), + params: { + file_path: path.join(rootDir, 'test.txt'), + content: 'new content', + }, + }, + { + name: 'WebFetchTool', + create: (config: Config, bus: MessageBus) => + new WebFetchTool(config, bus), + params: { + prompt: 'fetch https://example.com', + }, + }, + ]; + + describe.each(tools)('$name policy updates', ({ create, params }) => { + it.each([ + { + outcome: ToolConfirmationOutcome.ProceedAlways, + shouldPublish: false, + expectedApprovalMode: ApprovalMode.AUTO_EDIT, + }, + { + outcome: ToolConfirmationOutcome.ProceedAlwaysAndSave, + shouldPublish: true, + persist: true, + }, + ])( + 'should handle $outcome correctly', + async ({ outcome, shouldPublish, persist, expectedApprovalMode }) => { + const tool = create(mockConfig, mockMessageBus); + + // For file-based tools, ensure the file exists if needed + if (params.file_path) { + const fullPath = path.isAbsolute(params.file_path) + ? params.file_path + : path.join(rootDir, params.file_path); + fs.writeFileSync(fullPath, 'existing content'); + } + + const invocation = tool.build(params as any); + + // Mock getMessageBusDecision to trigger ASK_USER flow + vi.spyOn(invocation as any, 'getMessageBusDecision').mockResolvedValue( + 'ASK_USER', + ); + + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + expect(confirmation).not.toBe(false); + + if (confirmation) { + await confirmation.onConfirm(outcome); + + if (shouldPublish) { + expect(mockMessageBus.publish).toHaveBeenCalledWith( + expect.objectContaining({ + type: MessageBusType.UPDATE_POLICY, + persist, + }), + ); + } else { + // Should not publish UPDATE_POLICY message for ProceedAlways + const publishCalls = (mockMessageBus.publish as any).mock.calls; + const hasUpdatePolicy = publishCalls.some( + (call: any) => call[0].type === MessageBusType.UPDATE_POLICY, + ); + expect(hasUpdatePolicy).toBe(false); + } + + if (expectedApprovalMode !== undefined) { + expect(mockConfig.setApprovalMode).toHaveBeenCalledWith( + expectedApprovalMode, + ); + } + } + }, + ); + }); +}); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 2ea6bed52d..475e8f2745 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -311,9 +311,12 @@ class EditToolInvocation newContent: editData.newContent, onConfirm: async (outcome: ToolConfirmationOutcome) => { if (outcome === ToolConfirmationOutcome.ProceedAlways) { + // No need to publish a policy update as the default policy for + // AUTO_EDIT already reflects always approving edit. this.config.setApprovalMode(ApprovalMode.AUTO_EDIT); + } else { + await this.publishPolicyUpdate(outcome); } - await this.publishPolicyUpdate(outcome); if (ideConfirmation) { const result = await ideConfirmation; diff --git a/packages/core/src/tools/smart-edit.ts b/packages/core/src/tools/smart-edit.ts index 5c1e776db6..cd1a563863 100644 --- a/packages/core/src/tools/smart-edit.ts +++ b/packages/core/src/tools/smart-edit.ts @@ -681,9 +681,12 @@ class EditToolInvocation newContent: editData.newContent, onConfirm: async (outcome: ToolConfirmationOutcome) => { if (outcome === ToolConfirmationOutcome.ProceedAlways) { + // No need to publish a policy update as the default policy for + // AUTO_EDIT already reflects always approving smart-edit. this.config.setApprovalMode(ApprovalMode.AUTO_EDIT); + } else { + await this.publishPolicyUpdate(outcome); } - await this.publishPolicyUpdate(outcome); if (ideConfirmation) { const result = await ideConfirmation; diff --git a/packages/core/src/tools/web-fetch.ts b/packages/core/src/tools/web-fetch.ts index 57591343f3..92c0ae9fea 100644 --- a/packages/core/src/tools/web-fetch.ts +++ b/packages/core/src/tools/web-fetch.ts @@ -242,9 +242,12 @@ ${textContent} urls, onConfirm: async (outcome: ToolConfirmationOutcome) => { if (outcome === ToolConfirmationOutcome.ProceedAlways) { + // No need to publish a policy update as the default policy for + // AUTO_EDIT already reflects always approving web-fetch. this.config.setApprovalMode(ApprovalMode.AUTO_EDIT); + } else { + await this.publishPolicyUpdate(outcome); } - await this.publishPolicyUpdate(outcome); }, }; return confirmationDetails; diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index f9a6d2559d..4749970977 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -222,9 +222,12 @@ class WriteFileToolInvocation extends BaseToolInvocation< newContent: correctedContent, onConfirm: async (outcome: ToolConfirmationOutcome) => { if (outcome === ToolConfirmationOutcome.ProceedAlways) { + // No need to publish a policy update as the default policy for + // AUTO_EDIT already reflects always approving write-file. this.config.setApprovalMode(ApprovalMode.AUTO_EDIT); + } else { + await this.publishPolicyUpdate(outcome); } - await this.publishPolicyUpdate(outcome); if (ideConfirmation) { const result = await ideConfirmation;