fix(acp): Update allow approval policy flow for ACP clients to fix config persistence and compatible with TUI (#23818)

packages/cli/src/acp/acpClient.test.ts

@@ -99,6 +99,8 @@ vi.mock(
     const actual = await importOriginal();
     return {
       ...actual,
+      updatePolicy: vi.fn(),
+      createPolicyUpdater: vi.fn(),
       ReadManyFilesTool: vi.fn().mockImplementation(() => ({
         name: 'read_many_files',
         kind: 'read',
@@ -181,6 +183,20 @@ describe('GeminiAgent', () => {
       getWorkspaceContext: vi.fn().mockReturnValue({
         addReadOnlyPath: vi.fn(),
       }),
+      getPolicyEngine: vi.fn().mockReturnValue({
+        addRule: vi.fn(),
+      }),
+      messageBus: {
+        publish: vi.fn(),
+        subscribe: vi.fn(),
+        unsubscribe: vi.fn(),
+      },
+      storage: {
+        getWorkspaceAutoSavedPolicyPath: vi.fn(),
+        getAutoSavedPolicyPath: vi.fn(),
+        setClientName: vi.fn(),
+      },
+      setClientName: vi.fn(),
       get config() {
         return this;
       },
@@ -201,7 +217,10 @@ describe('GeminiAgent', () => {
     (loadCliConfig as unknown as Mock).mockResolvedValue(mockConfig);
     (loadSettings as unknown as Mock).mockImplementation(() => ({
       merged: {
-        security: { auth: { selectedType: AuthType.LOGIN_WITH_GOOGLE } },
+        security: {
+          auth: { selectedType: AuthType.LOGIN_WITH_GOOGLE },
+          enablePermanentToolApproval: true,
+        },
         mcpServers: {},
       },
       setValue: vi.fn(),
@@ -687,7 +706,10 @@ describe('Session', () => {
       systemDefaults: { settings: {} },
       user: { settings: {} },
       workspace: { settings: {} },
-      merged: { settings: {} },
+      merged: {
+        security: { enablePermanentToolApproval: true },
+        mcpServers: {},
+      },
       errors: [],
     } as unknown as LoadedSettings);
   });
@@ -1026,6 +1048,166 @@ describe('Session', () => {
     );
   });
 
+  it('should exclude always allow and save permanent option when enablePermanentToolApproval is false', async () => {
+    mockConfig.getDisableAlwaysAllow = vi.fn().mockReturnValue(false);
+    const confirmationDetails = {
+      type: 'edit',
+      onConfirm: vi.fn(),
+    };
+    mockTool.build.mockReturnValue({
+      getDescription: () => 'Test Tool',
+      toolLocations: () => [],
+      shouldConfirmExecute: vi.fn().mockResolvedValue(confirmationDetails),
+      execute: vi.fn().mockResolvedValue({ llmContent: 'Tool Result' }),
+    });
+
+    const customSettings = {
+      system: { settings: {} },
+      systemDefaults: { settings: {} },
+      user: { settings: {} },
+      workspace: { settings: {} },
+      merged: {
+        security: { enablePermanentToolApproval: false },
+        mcpServers: {},
+      },
+      errors: [],
+    } as unknown as LoadedSettings;
+
+    const localSession = new Session(
+      'session-2',
+      mockChat,
+      mockConfig,
+      mockConnection,
+      customSettings,
+    );
+
+    mockConnection.requestPermission.mockResolvedValueOnce({
+      outcome: {
+        outcome: 'selected',
+        optionId: ToolConfirmationOutcome.ProceedOnce,
+      },
+    });
+
+    const stream1 = createMockStream([
+      {
+        type: StreamEventType.CHUNK,
+        value: {
+          functionCalls: [{ name: 'test_tool', args: {} }],
+        },
+      },
+    ]);
+    const stream2 = createMockStream([
+      {
+        type: StreamEventType.CHUNK,
+        value: { candidates: [] },
+      },
+    ]);
+
+    mockChat.sendMessageStream
+      .mockResolvedValueOnce(stream1)
+      .mockResolvedValueOnce(stream2);
+
+    await localSession.prompt({
+      sessionId: 'session-2',
+      prompt: [{ type: 'text', text: 'Call tool' }],
+    });
+
+    expect(mockConnection.requestPermission).toHaveBeenCalledWith(
+      expect.objectContaining({
+        options: expect.not.arrayContaining([
+          expect.objectContaining({
+            optionId: ToolConfirmationOutcome.ProceedAlwaysAndSave,
+          }),
+        ]),
+      }),
+    );
+    expect(mockConnection.requestPermission).toHaveBeenCalledWith(
+      expect.objectContaining({
+        options: expect.arrayContaining([
+          expect.objectContaining({
+            optionId: ToolConfirmationOutcome.ProceedAlways,
+          }),
+        ]),
+      }),
+    );
+  });
+
+  it('should include always allow and save permanent option when enablePermanentToolApproval is true', async () => {
+    mockConfig.getDisableAlwaysAllow = vi.fn().mockReturnValue(false);
+    const confirmationDetails = {
+      type: 'edit',
+      onConfirm: vi.fn(),
+    };
+    mockTool.build.mockReturnValue({
+      getDescription: () => 'Test Tool',
+      toolLocations: () => [],
+      shouldConfirmExecute: vi.fn().mockResolvedValue(confirmationDetails),
+      execute: vi.fn().mockResolvedValue({ llmContent: 'Tool Result' }),
+    });
+
+    const customSettings = {
+      system: { settings: {} },
+      systemDefaults: { settings: {} },
+      user: { settings: {} },
+      workspace: { settings: {} },
+      merged: {
+        security: { enablePermanentToolApproval: true },
+        mcpServers: {},
+      },
+      errors: [],
+    } as unknown as LoadedSettings;
+
+    const localSession = new Session(
+      'session-2',
+      mockChat,
+      mockConfig,
+      mockConnection,
+      customSettings,
+    );
+
+    mockConnection.requestPermission.mockResolvedValueOnce({
+      outcome: {
+        outcome: 'selected',
+        optionId: ToolConfirmationOutcome.ProceedOnce,
+      },
+    });
+
+    const stream1 = createMockStream([
+      {
+        type: StreamEventType.CHUNK,
+        value: {
+          functionCalls: [{ name: 'test_tool', args: {} }],
+        },
+      },
+    ]);
+    const stream2 = createMockStream([
+      {
+        type: StreamEventType.CHUNK,
+        value: { candidates: [] },
+      },
+    ]);
+
+    mockChat.sendMessageStream
+      .mockResolvedValueOnce(stream1)
+      .mockResolvedValueOnce(stream2);
+
+    await localSession.prompt({
+      sessionId: 'session-2',
+      prompt: [{ type: 'text', text: 'Call tool' }],
+    });
+
+    expect(mockConnection.requestPermission).toHaveBeenCalledWith(
+      expect.objectContaining({
+        options: expect.arrayContaining([
+          expect.objectContaining({
+            optionId: ToolConfirmationOutcome.ProceedAlwaysAndSave,
+            name: 'Allow for this file in all future sessions',
+          }),
+        ]),
+      }),
+    );
+  });
+
   it('should use filePath for ACP diff content in permission request', async () => {
     const confirmationDetails = {
       type: 'edit',
@@ -1154,6 +1336,56 @@ describe('Session', () => {
     );
   });
 
+  it('should call updatePolicy when tool permission triggers always allow', async () => {
+    const confirmationDetails = {
+      type: 'info',
+      onConfirm: vi.fn(),
+    };
+    mockTool.build.mockReturnValue({
+      getDescription: () => 'Test Tool',
+      toolLocations: () => [],
+      shouldConfirmExecute: vi.fn().mockResolvedValue(confirmationDetails),
+      execute: vi.fn().mockResolvedValue({ llmContent: 'Tool Result' }),
+    });
+
+    mockConnection.requestPermission.mockResolvedValue({
+      outcome: {
+        outcome: 'selected',
+        optionId: ToolConfirmationOutcome.ProceedAlways,
+      },
+    });
+
+    const stream1 = createMockStream([
+      {
+        type: StreamEventType.CHUNK,
+        value: {
+          functionCalls: [{ name: 'test_tool', args: {} }],
+        },
+      },
+    ]);
+    const stream2 = createMockStream([
+      {
+        type: StreamEventType.CHUNK,
+        value: { candidates: [] },
+      },
+    ]);
+
+    mockChat.sendMessageStream
+      .mockResolvedValueOnce(stream1)
+      .mockResolvedValueOnce(stream2);
+
+    const { updatePolicy } = await import('@google/gemini-cli-core');
+
+    await session.prompt({
+      sessionId: 'session-1',
+      prompt: [{ type: 'text', text: 'Call tool' }],
+    });
+
+    expect(confirmationDetails.onConfirm).toHaveBeenCalled();
+
+    expect(updatePolicy).toHaveBeenCalled();
+  });
+
   it('should use filePath for ACP diff content in tool result', async () => {
     mockTool.build.mockReturnValue({
       getDescription: () => 'Test Tool',

packages/cli/src/acp/acpClient.ts

@@ -49,6 +49,7 @@ import {
   getDisplayString,
   processSingleFileContent,
   type AgentLoopContext,
+  updatePolicy,
 } from '@google/gemini-cli-core';
 import * as acp from '@agentclientprotocol/sdk';
 import { AcpFileSystemService } from './fileSystemService.js';
@@ -64,6 +65,7 @@ import {
   loadSettings,
   type LoadedSettings,
 } from '../config/settings.js';
+import { createPolicyUpdater } from '../config/policy.js';
 import * as fs from 'node:fs/promises';
 import * as path from 'node:path';
 import { z } from 'zod';
@@ -133,6 +135,7 @@ export class GeminiAgent {
     args: acp.InitializeRequest,
   ): Promise<acp.InitializeResponse> {
     this.clientCapabilities = args.clientCapabilities;
+
     const authMethods = [
       {
         id: AuthType.LOGIN_WITH_GOOGLE,
@@ -322,6 +325,7 @@ export class GeminiAgent {
 
     const geminiClient = config.getGeminiClient();
     const chat = await geminiClient.startChat();
+
     const session = new Session(
       sessionId,
       chat,
@@ -512,6 +516,12 @@ export class GeminiAgent {
 
     const config = await loadCliConfig(settings, sessionId, this.argv, { cwd });
 
+    createPolicyUpdater(
+      config.getPolicyEngine(),
+      config.messageBus,
+      config.storage,
+    );
+
     return config;
   }
 
@@ -1012,6 +1022,7 @@ export class Session {
           options: toPermissionOptions(
             confirmationDetails,
             this.context.config,
+            this.settings.merged.security.enablePermanentToolApproval,
           ),
           toolCall: {
             toolCallId: callId,
@@ -1036,6 +1047,16 @@ export class Session {
 
         await confirmationDetails.onConfirm(outcome);
 
+        // Update policy to enable Always Allow persistence
+        await updatePolicy(
+          tool,
+          outcome,
+          confirmationDetails,
+          this.context,
+          this.context.messageBus,
+          invocation,
+        );
+
         switch (outcome) {
           case ToolConfirmationOutcome.Cancel:
             return errorResponse(
@@ -1785,6 +1806,7 @@ const basicPermissionOptions = [
 function toPermissionOptions(
   confirmation: ToolCallConfirmationDetails,
   config: Config,
+  enablePermanentToolApproval: boolean = false,
 ): acp.PermissionOption[] {
   const disableAlwaysAllow = config.getDisableAlwaysAllow();
   const options: acp.PermissionOption[] = [];
@@ -1794,37 +1816,65 @@ function toPermissionOptions(
       case 'edit':
         options.push({
           optionId: ToolConfirmationOutcome.ProceedAlways,
-          name: 'Allow All Edits',
+          name: 'Allow for this session',
           kind: 'allow_always',
         });
+        if (enablePermanentToolApproval) {
+          options.push({
+            optionId: ToolConfirmationOutcome.ProceedAlwaysAndSave,
+            name: 'Allow for this file in all future sessions',
+            kind: 'allow_always',
+          });
+        }
         break;
       case 'exec':
         options.push({
           optionId: ToolConfirmationOutcome.ProceedAlways,
-          name: `Always Allow ${confirmation.rootCommand}`,
+          name: 'Allow for this session',
           kind: 'allow_always',
         });
+        if (enablePermanentToolApproval) {
+          options.push({
+            optionId: ToolConfirmationOutcome.ProceedAlwaysAndSave,
+            name: 'Allow this command for all future sessions',
+            kind: 'allow_always',
+          });
+        }
         break;
       case 'mcp':
         options.push(
           {
             optionId: ToolConfirmationOutcome.ProceedAlwaysServer,
-            name: `Always Allow ${confirmation.serverName}`,
+            name: 'Allow all server tools for this session',
             kind: 'allow_always',
           },
           {
             optionId: ToolConfirmationOutcome.ProceedAlwaysTool,
-            name: `Always Allow ${confirmation.toolName}`,
+            name: 'Allow tool for this session',
             kind: 'allow_always',
           },
         );
+        if (enablePermanentToolApproval) {
+          options.push({
+            optionId: ToolConfirmationOutcome.ProceedAlwaysAndSave,
+            name: 'Allow tool for all future sessions',
+            kind: 'allow_always',
+          });
+        }
         break;
       case 'info':
         options.push({
           optionId: ToolConfirmationOutcome.ProceedAlways,
-          name: `Always Allow`,
+          name: 'Allow for this session',
           kind: 'allow_always',
         });
+        if (enablePermanentToolApproval) {
+          options.push({
+            optionId: ToolConfirmationOutcome.ProceedAlwaysAndSave,
+            name: 'Allow for all future sessions',
+            kind: 'allow_always',
+          });
+        }
         break;
       case 'ask_user':
       case 'exit_plan_mode':

packages/cli/src/acp/acpResume.test.ts

@@ -91,6 +91,14 @@ describe('GeminiAgent Session Resume', () => {
       storage: {
         getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'),
       },
+      getPolicyEngine: vi.fn().mockReturnValue({
+        addRule: vi.fn(),
+      }),
+      messageBus: {
+        publish: vi.fn(),
+        subscribe: vi.fn(),
+        unsubscribe: vi.fn(),
+      },
       getApprovalMode: vi.fn().mockReturnValue('default'),
       isPlanEnabled: vi.fn().mockReturnValue(true),
       getModel: vi.fn().mockReturnValue('gemini-pro'),