refactor: deprecate legacy confirmation settings and enforce Policy Engine (#15626)

This commit is contained in:
Abhi
2025-12-29 14:22:42 -05:00
committed by GitHub
parent a3d214f8d7
commit dcd2449b1a
17 changed files with 31 additions and 113 deletions
-1
View File
@@ -94,7 +94,6 @@ they appear in the UI.
| Enable Tool Output Truncation | `tools.enableToolOutputTruncation` | Enable truncation of large tool outputs. | `true` | | Enable Tool Output Truncation | `tools.enableToolOutputTruncation` | Enable truncation of large tool outputs. | `true` |
| Tool Output Truncation Threshold | `tools.truncateToolOutputThreshold` | Truncate tool output if it is larger than this many characters. Set to -1 to disable. | `10000` | | Tool Output Truncation Threshold | `tools.truncateToolOutputThreshold` | Truncate tool output if it is larger than this many characters. Set to -1 to disable. | `10000` |
| Tool Output Truncation Lines | `tools.truncateToolOutputLines` | The number of lines to keep when truncating tool output. | `100` | | Tool Output Truncation Lines | `tools.truncateToolOutputLines` | The number of lines to keep when truncating tool output. | `100` |
| Enable Message Bus Integration | `tools.enableMessageBusIntegration` | Enable policy-based tool confirmation via message bus integration. | `true` |
### Security ### Security
-8
View File
@@ -684,14 +684,6 @@ their corresponding top-level category object in your `settings.json` file.
- **Default:** `1000` - **Default:** `1000`
- **Requires restart:** Yes - **Requires restart:** Yes
- **`tools.enableMessageBusIntegration`** (boolean):
- **Description:** Enable policy-based tool confirmation via message bus
integration. When enabled, tools automatically respect policy engine
decisions (ALLOW/DENY/ASK_USER) without requiring individual tool
implementations.
- **Default:** `true`
- **Requires restart:** Yes
- **`tools.enableHooks`** (boolean): - **`tools.enableHooks`** (boolean):
- **Description:** Enable the hooks system for intercepting and customizing - **Description:** Enable the hooks system for intercepting and customizing
Gemini CLI behavior. When enabled, hooks configured in settings will execute Gemini CLI behavior. When enabled, hooks configured in settings will execute
@@ -59,7 +59,6 @@ export function createMockConfig(
getEmbeddingModel: vi.fn().mockReturnValue('text-embedding-004'), getEmbeddingModel: vi.fn().mockReturnValue('text-embedding-004'),
getSessionId: vi.fn().mockReturnValue('test-session-id'), getSessionId: vi.fn().mockReturnValue('test-session-id'),
getUserTier: vi.fn(), getUserTier: vi.fn(),
getEnableMessageBusIntegration: vi.fn().mockReturnValue(false),
getMessageBus: vi.fn(), getMessageBus: vi.fn(),
getPolicyEngine: vi.fn(), getPolicyEngine: vi.fn(),
getEnableExtensionReloading: vi.fn().mockReturnValue(false), getEnableExtensionReloading: vi.fn().mockReturnValue(false),
+2 -8
View File
@@ -16,6 +16,7 @@ import {
WEB_FETCH_TOOL_NAME, WEB_FETCH_TOOL_NAME,
type ExtensionLoader, type ExtensionLoader,
debugLogger, debugLogger,
ApprovalMode,
} from '@google/gemini-cli-core'; } from '@google/gemini-cli-core';
import { loadCliConfig, parseArguments, type CliArgs } from './config.js'; import { loadCliConfig, parseArguments, type CliArgs } from './config.js';
import type { Settings } from './settings.js'; import type { Settings } from './settings.js';
@@ -629,14 +630,7 @@ describe('loadCliConfig', () => {
expect(config.getFileFilteringRespectGeminiIgnore()).toBe( expect(config.getFileFilteringRespectGeminiIgnore()).toBe(
DEFAULT_FILE_FILTERING_OPTIONS.respectGeminiIgnore, DEFAULT_FILE_FILTERING_OPTIONS.respectGeminiIgnore,
); );
}); expect(config.getApprovalMode()).toBe(ApprovalMode.DEFAULT);
it('should default enableMessageBusIntegration to true when unconfigured', async () => {
process.argv = ['node', 'script.js'];
const argv = await parseArguments({} as Settings);
const settings: Settings = {};
const config = await loadCliConfig(settings, 'test-session', argv);
expect(config['enableMessageBusIntegration']).toBe(true);
}); });
}); });
-4
View File
@@ -542,9 +542,6 @@ export async function loadCliConfig(
approvalMode, approvalMode,
); );
const enableMessageBusIntegration =
settings.tools?.enableMessageBusIntegration ?? true;
const allowedTools = argv.allowedTools || settings.tools?.allowed || []; const allowedTools = argv.allowedTools || settings.tools?.allowed || [];
const allowedToolsSet = new Set(allowedTools); const allowedToolsSet = new Set(allowedTools);
@@ -695,7 +692,6 @@ export async function loadCliConfig(
output: { output: {
format: (argv.outputFormat ?? settings.output?.format) as OutputFormat, format: (argv.outputFormat ?? settings.output?.format) as OutputFormat,
}, },
enableMessageBusIntegration,
codebaseInvestigatorSettings: codebaseInvestigatorSettings:
settings.experimental?.codebaseInvestigatorSettings, settings.experimental?.codebaseInvestigatorSettings,
introspectionAgentSettings: introspectionAgentSettings:
-1
View File
@@ -85,7 +85,6 @@ const MIGRATION_MAP: Record<string, string> = {
disableAutoUpdate: 'general.disableAutoUpdate', disableAutoUpdate: 'general.disableAutoUpdate',
disableUpdateNag: 'general.disableUpdateNag', disableUpdateNag: 'general.disableUpdateNag',
dnsResolutionOrder: 'advanced.dnsResolutionOrder', dnsResolutionOrder: 'advanced.dnsResolutionOrder',
enableMessageBusIntegration: 'tools.enableMessageBusIntegration',
enableHooks: 'tools.enableHooks', enableHooks: 'tools.enableHooks',
enablePromptCompletion: 'general.enablePromptCompletion', enablePromptCompletion: 'general.enablePromptCompletion',
enforcedAuthType: 'security.auth.enforcedType', enforcedAuthType: 'security.auth.enforcedType',
-12
View File
@@ -1073,18 +1073,6 @@ const SETTINGS_SCHEMA = {
description: 'The number of lines to keep when truncating tool output.', description: 'The number of lines to keep when truncating tool output.',
showInDialog: true, showInDialog: true,
}, },
enableMessageBusIntegration: {
type: 'boolean',
label: 'Enable Message Bus Integration',
category: 'Tools',
requiresRestart: true,
default: true,
description: oneLine`
Enable policy-based tool confirmation via message bus integration.
When enabled, tools automatically respect policy engine decisions (ALLOW/DENY/ASK_USER) without requiring individual tool implementations.
`,
showInDialog: true,
},
enableHooks: { enableHooks: {
type: 'boolean', type: 'boolean',
label: 'Enable Hooks System', label: 'Enable Hooks System',
@@ -150,7 +150,7 @@ describe('Settings Repro', () => {
showColor: true, showColor: true,
enableInteractiveShell: true, enableInteractiveShell: true,
}, },
enableMessageBusIntegration: true, truncateToolOutputLines: 100,
}, },
experimental: { experimental: {
useModelRouter: false, useModelRouter: false,
@@ -96,7 +96,6 @@ describe('BuiltinCommandLoader', () => {
vi.clearAllMocks(); vi.clearAllMocks();
mockConfig = { mockConfig = {
getFolderTrust: vi.fn().mockReturnValue(true), getFolderTrust: vi.fn().mockReturnValue(true),
getEnableMessageBusIntegration: () => false,
getEnableExtensionReloading: () => false, getEnableExtensionReloading: () => false,
getEnableHooks: () => false, getEnableHooks: () => false,
} as unknown as Config; } as unknown as Config;
@@ -172,7 +171,6 @@ describe('BuiltinCommandLoader', () => {
it('should include policies command when message bus integration is enabled', async () => { it('should include policies command when message bus integration is enabled', async () => {
const mockConfigWithMessageBus = { const mockConfigWithMessageBus = {
...mockConfig, ...mockConfig,
getEnableMessageBusIntegration: () => true,
getEnableHooks: () => false, getEnableHooks: () => false,
} as unknown as Config; } as unknown as Config;
const loader = new BuiltinCommandLoader(mockConfigWithMessageBus); const loader = new BuiltinCommandLoader(mockConfigWithMessageBus);
@@ -180,18 +178,6 @@ describe('BuiltinCommandLoader', () => {
const policiesCmd = commands.find((c) => c.name === 'policies'); const policiesCmd = commands.find((c) => c.name === 'policies');
expect(policiesCmd).toBeDefined(); expect(policiesCmd).toBeDefined();
}); });
it('should exclude policies command when message bus integration is disabled', async () => {
const mockConfigWithoutMessageBus = {
...mockConfig,
getEnableMessageBusIntegration: () => false,
getEnableHooks: () => false,
} as unknown as Config;
const loader = new BuiltinCommandLoader(mockConfigWithoutMessageBus);
const commands = await loader.loadCommands(new AbortController().signal);
const policiesCmd = commands.find((c) => c.name === 'policies');
expect(policiesCmd).toBeUndefined();
});
}); });
describe('BuiltinCommandLoader profile', () => { describe('BuiltinCommandLoader profile', () => {
@@ -202,7 +188,6 @@ describe('BuiltinCommandLoader profile', () => {
mockConfig = { mockConfig = {
getFolderTrust: vi.fn().mockReturnValue(false), getFolderTrust: vi.fn().mockReturnValue(false),
getCheckpointingEnabled: () => false, getCheckpointingEnabled: () => false,
getEnableMessageBusIntegration: () => false,
getEnableExtensionReloading: () => false, getEnableExtensionReloading: () => false,
getEnableHooks: () => false, getEnableHooks: () => false,
} as unknown as Config; } as unknown as Config;
@@ -81,9 +81,7 @@ export class BuiltinCommandLoader implements ICommandLoader {
modelCommand, modelCommand,
...(this.config?.getFolderTrust() ? [permissionsCommand] : []), ...(this.config?.getFolderTrust() ? [permissionsCommand] : []),
privacyCommand, privacyCommand,
...(this.config?.getEnableMessageBusIntegration() policiesCommand,
? [policiesCommand]
: []),
...(isDevelopment ? [profileCommand] : []), ...(isDevelopment ? [profileCommand] : []),
quitCommand, quitCommand,
restoreCommand(this.config), restoreCommand(this.config),
@@ -80,7 +80,6 @@ const mockConfig = {
getUseSmartEdit: () => false, getUseSmartEdit: () => false,
getGeminiClient: () => null, // No client needed for these tests getGeminiClient: () => null, // No client needed for these tests
getShellExecutionConfig: () => ({ terminalWidth: 80, terminalHeight: 24 }), getShellExecutionConfig: () => ({ terminalWidth: 80, terminalHeight: 24 }),
getEnableMessageBusIntegration: () => false,
getMessageBus: () => null, getMessageBus: () => null,
getPolicyEngine: () => null, getPolicyEngine: () => null,
isInteractive: () => false, isInteractive: () => false,
+2 -1
View File
@@ -61,6 +61,7 @@ vi.mock('../tools/tool-registry', () => {
ToolRegistryMock.prototype.sortTools = vi.fn(); ToolRegistryMock.prototype.sortTools = vi.fn();
ToolRegistryMock.prototype.getAllTools = vi.fn(() => []); // Mock methods if needed ToolRegistryMock.prototype.getAllTools = vi.fn(() => []); // Mock methods if needed
ToolRegistryMock.prototype.getTool = vi.fn(); ToolRegistryMock.prototype.getTool = vi.fn();
ToolRegistryMock.prototype.setMessageBus = vi.fn();
ToolRegistryMock.prototype.getFunctionDeclarations = vi.fn(() => []); ToolRegistryMock.prototype.getFunctionDeclarations = vi.fn(() => []);
return { ToolRegistry: ToolRegistryMock }; return { ToolRegistry: ToolRegistryMock };
}); });
@@ -923,7 +924,7 @@ describe('Server Config (config.ts)', () => {
expect(DelegateToAgentToolMock).toHaveBeenCalledWith( expect(DelegateToAgentToolMock).toHaveBeenCalledWith(
expect.anything(), // AgentRegistry expect.anything(), // AgentRegistry
config, config,
undefined, expect.anything(), // MessageBus
); );
const calls = registerToolMock.mock.calls; const calls = registerToolMock.mock.calls;
+3 -24
View File
@@ -312,7 +312,6 @@ export interface ConfigParameters {
useWriteTodos?: boolean; useWriteTodos?: boolean;
policyEngineConfig?: PolicyEngineConfig; policyEngineConfig?: PolicyEngineConfig;
output?: OutputSettings; output?: OutputSettings;
enableMessageBusIntegration?: boolean;
disableModelRouterForAuth?: AuthType[]; disableModelRouterForAuth?: AuthType[];
codebaseInvestigatorSettings?: CodebaseInvestigatorSettings; codebaseInvestigatorSettings?: CodebaseInvestigatorSettings;
introspectionAgentSettings?: IntrospectionAgentSettings; introspectionAgentSettings?: IntrospectionAgentSettings;
@@ -436,7 +435,6 @@ export class Config {
private readonly messageBus: MessageBus; private readonly messageBus: MessageBus;
private readonly policyEngine: PolicyEngine; private readonly policyEngine: PolicyEngine;
private readonly outputSettings: OutputSettings; private readonly outputSettings: OutputSettings;
private readonly enableMessageBusIntegration: boolean;
private readonly codebaseInvestigatorSettings: CodebaseInvestigatorSettings; private readonly codebaseInvestigatorSettings: CodebaseInvestigatorSettings;
private readonly introspectionAgentSettings: IntrospectionAgentSettings; private readonly introspectionAgentSettings: IntrospectionAgentSettings;
private readonly continueOnFailedApiCall: boolean; private readonly continueOnFailedApiCall: boolean;
@@ -579,14 +577,6 @@ export class Config {
? params.hooks.disabled ? params.hooks.disabled
: undefined) ?? []; : undefined) ?? [];
// Enable MessageBus integration if:
// 1. Explicitly enabled via setting, OR
// 2. Hooks are enabled and hooks are configured
const hasHooks = params.hooks && Object.keys(params.hooks).length > 0;
const hooksNeedMessageBus = this.enableHooks && hasHooks;
this.enableMessageBusIntegration =
params.enableMessageBusIntegration ??
(hooksNeedMessageBus ? true : false);
this.codebaseInvestigatorSettings = { this.codebaseInvestigatorSettings = {
enabled: params.codebaseInvestigatorSettings?.enabled ?? true, enabled: params.codebaseInvestigatorSettings?.enabled ?? true,
maxNumTurns: params.codebaseInvestigatorSettings?.maxNumTurns ?? 10, maxNumTurns: params.codebaseInvestigatorSettings?.maxNumTurns ?? 10,
@@ -1580,10 +1570,6 @@ export class Config {
return this.policyEngine; return this.policyEngine;
} }
getEnableMessageBusIntegration(): boolean {
return this.enableMessageBusIntegration;
}
getEnableHooks(): boolean { getEnableHooks(): boolean {
return this.enableHooks; return this.enableHooks;
} }
@@ -1600,9 +1586,7 @@ export class Config {
const registry = new ToolRegistry(this); const registry = new ToolRegistry(this);
// Set message bus on tool registry before discovery so MCP tools can access it // Set message bus on tool registry before discovery so MCP tools can access it
if (this.getEnableMessageBusIntegration()) { registry.setMessageBus(this.messageBus);
registry.setMessageBus(this.messageBus);
}
// helper to create & register core tools that are enabled // helper to create & register core tools that are enabled
// eslint-disable-next-line @typescript-eslint/no-explicit-any // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -1628,11 +1612,7 @@ export class Config {
// Pass message bus to tools when feature flag is enabled // Pass message bus to tools when feature flag is enabled
// This first implementation is only focused on the general case of // This first implementation is only focused on the general case of
// the tool registry. // the tool registry.
const messageBusEnabled = this.getEnableMessageBusIntegration(); const toolArgs = [...args, this.getMessageBus()];
const toolArgs = messageBusEnabled
? [...args, this.getMessageBus()]
: args;
registry.registerTool(new ToolClass(...toolArgs)); registry.registerTool(new ToolClass(...toolArgs));
} }
@@ -1686,11 +1666,10 @@ export class Config {
!allowedTools || allowedTools.includes(DELEGATE_TO_AGENT_TOOL_NAME); !allowedTools || allowedTools.includes(DELEGATE_TO_AGENT_TOOL_NAME);
if (isAllowed) { if (isAllowed) {
const messageBusEnabled = this.getEnableMessageBusIntegration();
const delegateTool = new DelegateToAgentTool( const delegateTool = new DelegateToAgentTool(
this.agentRegistry, this.agentRegistry,
this, this,
messageBusEnabled ? this.getMessageBus() : undefined, this.getMessageBus(),
); );
registry.registerTool(delegateTool); registry.registerTool(delegateTool);
} }
@@ -257,8 +257,7 @@ function createMockConfig(overrides: Partial<Config> = {}): Config {
getActiveModel: () => DEFAULT_GEMINI_MODEL, getActiveModel: () => DEFAULT_GEMINI_MODEL,
getUseSmartEdit: () => false, getUseSmartEdit: () => false,
getGeminiClient: () => null, getGeminiClient: () => null,
getEnableMessageBusIntegration: () => false, getMessageBus: () => createMockMessageBus(),
getMessageBus: () => null,
getEnableHooks: () => false, getEnableHooks: () => false,
getPolicyEngine: () => null, getPolicyEngine: () => null,
getExperiments: () => {}, getExperiments: () => {},
+21 -23
View File
@@ -148,32 +148,30 @@ export class CoreToolScheduler {
// Use a static WeakMap to ensure we only subscribe ONCE per MessageBus instance // Use a static WeakMap to ensure we only subscribe ONCE per MessageBus instance
// This prevents memory leaks when multiple CoreToolScheduler instances are created // This prevents memory leaks when multiple CoreToolScheduler instances are created
// (e.g., on every React render, or for each non-interactive tool call) // (e.g., on every React render, or for each non-interactive tool call)
if (this.config.getEnableMessageBusIntegration()) { const messageBus = this.config.getMessageBus();
const messageBus = this.config.getMessageBus();
// Check if we've already subscribed a handler to this message bus // Check if we've already subscribed a handler to this message bus
if (!CoreToolScheduler.subscribedMessageBuses.has(messageBus)) { if (!CoreToolScheduler.subscribedMessageBuses.has(messageBus)) {
// Create a shared handler that will be used for this message bus // Create a shared handler that will be used for this message bus
const sharedHandler = (request: ToolConfirmationRequest) => { const sharedHandler = (request: ToolConfirmationRequest) => {
// When ASK_USER policy decision is made, respond with requiresUserConfirmation=true // When ASK_USER policy decision is made, respond with requiresUserConfirmation=true
// to tell tools to use their legacy confirmation flow // to tell tools to use their legacy confirmation flow
// eslint-disable-next-line @typescript-eslint/no-floating-promises // eslint-disable-next-line @typescript-eslint/no-floating-promises
messageBus.publish({ messageBus.publish({
type: MessageBusType.TOOL_CONFIRMATION_RESPONSE, type: MessageBusType.TOOL_CONFIRMATION_RESPONSE,
correlationId: request.correlationId, correlationId: request.correlationId,
confirmed: false, confirmed: false,
requiresUserConfirmation: true, requiresUserConfirmation: true,
}); });
}; };
messageBus.subscribe( messageBus.subscribe(
MessageBusType.TOOL_CONFIRMATION_REQUEST, MessageBusType.TOOL_CONFIRMATION_REQUEST,
sharedHandler, sharedHandler,
); );
// Store the handler in the WeakMap so we don't subscribe again // Store the handler in the WeakMap so we don't subscribe again
CoreToolScheduler.subscribedMessageBuses.set(messageBus, sharedHandler); CoreToolScheduler.subscribedMessageBuses.set(messageBus, sharedHandler);
}
} }
} }
@@ -65,7 +65,6 @@ describe('executeToolCall', () => {
getActiveModel: () => PREVIEW_GEMINI_MODEL, getActiveModel: () => PREVIEW_GEMINI_MODEL,
getUseSmartEdit: () => false, getUseSmartEdit: () => false,
getGeminiClient: () => null, // No client needed for these tests getGeminiClient: () => null, // No client needed for these tests
getEnableMessageBusIntegration: () => false,
getMessageBus: () => null, getMessageBus: () => null,
getPolicyEngine: () => null, getPolicyEngine: () => null,
isInteractive: () => false, isInteractive: () => false,
-7
View File
@@ -1128,13 +1128,6 @@
"default": 1000, "default": 1000,
"type": "number" "type": "number"
}, },
"enableMessageBusIntegration": {
"title": "Enable Message Bus Integration",
"description": "Enable policy-based tool confirmation via message bus integration. When enabled, tools automatically respect policy engine decisions (ALLOW/DENY/ASK_USER) without requiring individual tool implementations.",
"markdownDescription": "Enable policy-based tool confirmation via message bus integration. When enabled, tools automatically respect policy engine decisions (ALLOW/DENY/ASK_USER) without requiring individual tool implementations.\n\n- Category: `Tools`\n- Requires restart: `yes`\n- Default: `true`",
"default": true,
"type": "boolean"
},
"enableHooks": { "enableHooks": {
"title": "Enable Hooks System", "title": "Enable Hooks System",
"description": "Enable the hooks system for intercepting and customizing Gemini CLI behavior. When enabled, hooks configured in settings will execute at appropriate lifecycle events (BeforeTool, AfterTool, BeforeModel, etc.). Requires MessageBus integration.", "description": "Enable the hooks system for intercepting and customizing Gemini CLI behavior. When enabled, hooks configured in settings will execute at appropriate lifecycle events (BeforeTool, AfterTool, BeforeModel, etc.). Requires MessageBus integration.",