From 602a213b9c9bc2b310feac3f28ffccf74e0dfa63 Mon Sep 17 00:00:00 2001 From: Abhi Date: Wed, 24 Dec 2025 20:19:02 -0500 Subject: [PATCH] refactor: deprecate legacy confirmation settings and enforce Policy Engine Removes enableMessageBusIntegration from settings and internal configuration logic, effectively enforcing the Policy Engine as the default and only confirmation mechanism. Updates CLI and Core config handling to assume message bus integration is always enabled. Cleaned up tests to reflect this change. --- docs/cli/settings.md | 1 - docs/get-started/configuration.md | 8 ---- .../a2a-server/src/utils/testing_utils.ts | 1 - packages/cli/src/config/config.test.ts | 10 +--- packages/cli/src/config/config.ts | 4 -- packages/cli/src/config/settings.ts | 1 - packages/cli/src/config/settingsSchema.ts | 12 ----- .../cli/src/config/settings_repro.test.ts | 2 +- .../src/services/BuiltinCommandLoader.test.ts | 15 ------ .../cli/src/services/BuiltinCommandLoader.ts | 4 +- .../cli/src/ui/hooks/useToolScheduler.test.ts | 1 - packages/core/src/config/config.test.ts | 3 +- packages/core/src/config/config.ts | 27 ++--------- .../core/src/core/coreToolScheduler.test.ts | 3 +- packages/core/src/core/coreToolScheduler.ts | 46 +++++++++---------- .../core/nonInteractiveToolExecutor.test.ts | 1 - schemas/settings.schema.json | 7 --- 17 files changed, 32 insertions(+), 114 deletions(-) diff --git a/docs/cli/settings.md b/docs/cli/settings.md index ffe0e1e72d7..96912995d4e 100644 --- a/docs/cli/settings.md +++ b/docs/cli/settings.md @@ -94,7 +94,6 @@ they appear in the UI. | 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 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 diff --git a/docs/get-started/configuration.md b/docs/get-started/configuration.md index edc05667490..3b0d9a538f0 100644 --- a/docs/get-started/configuration.md +++ b/docs/get-started/configuration.md @@ -684,14 +684,6 @@ their corresponding top-level category object in your `settings.json` file. - **Default:** `1000` - **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): - **Description:** Enable the hooks system for intercepting and customizing Gemini CLI behavior. When enabled, hooks configured in settings will execute diff --git a/packages/a2a-server/src/utils/testing_utils.ts b/packages/a2a-server/src/utils/testing_utils.ts index 2f5a8847535..d472b4f995b 100644 --- a/packages/a2a-server/src/utils/testing_utils.ts +++ b/packages/a2a-server/src/utils/testing_utils.ts @@ -59,7 +59,6 @@ export function createMockConfig( getEmbeddingModel: vi.fn().mockReturnValue('text-embedding-004'), getSessionId: vi.fn().mockReturnValue('test-session-id'), getUserTier: vi.fn(), - getEnableMessageBusIntegration: vi.fn().mockReturnValue(false), getMessageBus: vi.fn(), getPolicyEngine: vi.fn(), getEnableExtensionReloading: vi.fn().mockReturnValue(false), diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 7748c1869af..32745d4c619 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -16,6 +16,7 @@ import { WEB_FETCH_TOOL_NAME, type ExtensionLoader, debugLogger, + ApprovalMode, } from '@google/gemini-cli-core'; import { loadCliConfig, parseArguments, type CliArgs } from './config.js'; import type { Settings } from './settings.js'; @@ -629,14 +630,7 @@ describe('loadCliConfig', () => { expect(config.getFileFilteringRespectGeminiIgnore()).toBe( DEFAULT_FILE_FILTERING_OPTIONS.respectGeminiIgnore, ); - }); - - 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); + expect(config.getApprovalMode()).toBe(ApprovalMode.DEFAULT); }); }); diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 2ae3713298e..52b718d938d 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -542,9 +542,6 @@ export async function loadCliConfig( approvalMode, ); - const enableMessageBusIntegration = - settings.tools?.enableMessageBusIntegration ?? true; - const allowedTools = argv.allowedTools || settings.tools?.allowed || []; const allowedToolsSet = new Set(allowedTools); @@ -695,7 +692,6 @@ export async function loadCliConfig( output: { format: (argv.outputFormat ?? settings.output?.format) as OutputFormat, }, - enableMessageBusIntegration, codebaseInvestigatorSettings: settings.experimental?.codebaseInvestigatorSettings, introspectionAgentSettings: diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 5c7294b6019..31d5f707334 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -85,7 +85,6 @@ const MIGRATION_MAP: Record = { disableAutoUpdate: 'general.disableAutoUpdate', disableUpdateNag: 'general.disableUpdateNag', dnsResolutionOrder: 'advanced.dnsResolutionOrder', - enableMessageBusIntegration: 'tools.enableMessageBusIntegration', enableHooks: 'tools.enableHooks', enablePromptCompletion: 'general.enablePromptCompletion', enforcedAuthType: 'security.auth.enforcedType', diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 30298672f0d..465f1087d38 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -1073,18 +1073,6 @@ const SETTINGS_SCHEMA = { description: 'The number of lines to keep when truncating tool output.', 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: { type: 'boolean', label: 'Enable Hooks System', diff --git a/packages/cli/src/config/settings_repro.test.ts b/packages/cli/src/config/settings_repro.test.ts index 1f7ab5b9439..404554ddbdc 100644 --- a/packages/cli/src/config/settings_repro.test.ts +++ b/packages/cli/src/config/settings_repro.test.ts @@ -150,7 +150,7 @@ describe('Settings Repro', () => { showColor: true, enableInteractiveShell: true, }, - enableMessageBusIntegration: true, + truncateToolOutputLines: 100, }, experimental: { useModelRouter: false, diff --git a/packages/cli/src/services/BuiltinCommandLoader.test.ts b/packages/cli/src/services/BuiltinCommandLoader.test.ts index d3ee0e8f0b7..f030a60215c 100644 --- a/packages/cli/src/services/BuiltinCommandLoader.test.ts +++ b/packages/cli/src/services/BuiltinCommandLoader.test.ts @@ -96,7 +96,6 @@ describe('BuiltinCommandLoader', () => { vi.clearAllMocks(); mockConfig = { getFolderTrust: vi.fn().mockReturnValue(true), - getEnableMessageBusIntegration: () => false, getEnableExtensionReloading: () => false, getEnableHooks: () => false, } as unknown as Config; @@ -172,7 +171,6 @@ describe('BuiltinCommandLoader', () => { it('should include policies command when message bus integration is enabled', async () => { const mockConfigWithMessageBus = { ...mockConfig, - getEnableMessageBusIntegration: () => true, getEnableHooks: () => false, } as unknown as Config; const loader = new BuiltinCommandLoader(mockConfigWithMessageBus); @@ -180,18 +178,6 @@ describe('BuiltinCommandLoader', () => { const policiesCmd = commands.find((c) => c.name === 'policies'); 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', () => { @@ -202,7 +188,6 @@ describe('BuiltinCommandLoader profile', () => { mockConfig = { getFolderTrust: vi.fn().mockReturnValue(false), getCheckpointingEnabled: () => false, - getEnableMessageBusIntegration: () => false, getEnableExtensionReloading: () => false, getEnableHooks: () => false, } as unknown as Config; diff --git a/packages/cli/src/services/BuiltinCommandLoader.ts b/packages/cli/src/services/BuiltinCommandLoader.ts index baccd7347ad..9c54afe54be 100644 --- a/packages/cli/src/services/BuiltinCommandLoader.ts +++ b/packages/cli/src/services/BuiltinCommandLoader.ts @@ -81,9 +81,7 @@ export class BuiltinCommandLoader implements ICommandLoader { modelCommand, ...(this.config?.getFolderTrust() ? [permissionsCommand] : []), privacyCommand, - ...(this.config?.getEnableMessageBusIntegration() - ? [policiesCommand] - : []), + policiesCommand, ...(isDevelopment ? [profileCommand] : []), quitCommand, restoreCommand(this.config), diff --git a/packages/cli/src/ui/hooks/useToolScheduler.test.ts b/packages/cli/src/ui/hooks/useToolScheduler.test.ts index b6c1ebd4fac..68dd1122a61 100644 --- a/packages/cli/src/ui/hooks/useToolScheduler.test.ts +++ b/packages/cli/src/ui/hooks/useToolScheduler.test.ts @@ -80,7 +80,6 @@ const mockConfig = { getUseSmartEdit: () => false, getGeminiClient: () => null, // No client needed for these tests getShellExecutionConfig: () => ({ terminalWidth: 80, terminalHeight: 24 }), - getEnableMessageBusIntegration: () => false, getMessageBus: () => null, getPolicyEngine: () => null, isInteractive: () => false, diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index c33fc1e3edf..28714db6173 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -61,6 +61,7 @@ vi.mock('../tools/tool-registry', () => { ToolRegistryMock.prototype.sortTools = vi.fn(); ToolRegistryMock.prototype.getAllTools = vi.fn(() => []); // Mock methods if needed ToolRegistryMock.prototype.getTool = vi.fn(); + ToolRegistryMock.prototype.setMessageBus = vi.fn(); ToolRegistryMock.prototype.getFunctionDeclarations = vi.fn(() => []); return { ToolRegistry: ToolRegistryMock }; }); @@ -923,7 +924,7 @@ describe('Server Config (config.ts)', () => { expect(DelegateToAgentToolMock).toHaveBeenCalledWith( expect.anything(), // AgentRegistry config, - undefined, + expect.anything(), // MessageBus ); const calls = registerToolMock.mock.calls; diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index d8a7e5b5415..e07d40d03d2 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -312,7 +312,6 @@ export interface ConfigParameters { useWriteTodos?: boolean; policyEngineConfig?: PolicyEngineConfig; output?: OutputSettings; - enableMessageBusIntegration?: boolean; disableModelRouterForAuth?: AuthType[]; codebaseInvestigatorSettings?: CodebaseInvestigatorSettings; introspectionAgentSettings?: IntrospectionAgentSettings; @@ -436,7 +435,6 @@ export class Config { private readonly messageBus: MessageBus; private readonly policyEngine: PolicyEngine; private readonly outputSettings: OutputSettings; - private readonly enableMessageBusIntegration: boolean; private readonly codebaseInvestigatorSettings: CodebaseInvestigatorSettings; private readonly introspectionAgentSettings: IntrospectionAgentSettings; private readonly continueOnFailedApiCall: boolean; @@ -579,14 +577,6 @@ export class Config { ? params.hooks.disabled : 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 = { enabled: params.codebaseInvestigatorSettings?.enabled ?? true, maxNumTurns: params.codebaseInvestigatorSettings?.maxNumTurns ?? 10, @@ -1580,10 +1570,6 @@ export class Config { return this.policyEngine; } - getEnableMessageBusIntegration(): boolean { - return this.enableMessageBusIntegration; - } - getEnableHooks(): boolean { return this.enableHooks; } @@ -1600,9 +1586,7 @@ export class Config { const registry = new ToolRegistry(this); // 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 // 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 // This first implementation is only focused on the general case of // the tool registry. - const messageBusEnabled = this.getEnableMessageBusIntegration(); - - const toolArgs = messageBusEnabled - ? [...args, this.getMessageBus()] - : args; + const toolArgs = [...args, this.getMessageBus()]; registry.registerTool(new ToolClass(...toolArgs)); } @@ -1686,11 +1666,10 @@ export class Config { !allowedTools || allowedTools.includes(DELEGATE_TO_AGENT_TOOL_NAME); if (isAllowed) { - const messageBusEnabled = this.getEnableMessageBusIntegration(); const delegateTool = new DelegateToAgentTool( this.agentRegistry, this, - messageBusEnabled ? this.getMessageBus() : undefined, + this.getMessageBus(), ); registry.registerTool(delegateTool); } diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index a1b1a90b796..a5f340eeebd 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -257,8 +257,7 @@ function createMockConfig(overrides: Partial = {}): Config { getActiveModel: () => DEFAULT_GEMINI_MODEL, getUseSmartEdit: () => false, getGeminiClient: () => null, - getEnableMessageBusIntegration: () => false, - getMessageBus: () => null, + getMessageBus: () => createMockMessageBus(), getEnableHooks: () => false, getPolicyEngine: () => null, getExperiments: () => {}, diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index cd660da40f4..ae064a148d5 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -148,32 +148,30 @@ export class CoreToolScheduler { // Use a static WeakMap to ensure we only subscribe ONCE per MessageBus instance // This prevents memory leaks when multiple CoreToolScheduler instances are created // (e.g., on every React render, or for each non-interactive tool call) - if (this.config.getEnableMessageBusIntegration()) { - const messageBus = this.config.getMessageBus(); - - // Check if we've already subscribed a handler to this message bus - if (!CoreToolScheduler.subscribedMessageBuses.has(messageBus)) { - // Create a shared handler that will be used for this message bus - const sharedHandler = (request: ToolConfirmationRequest) => { - // When ASK_USER policy decision is made, respond with requiresUserConfirmation=true - // to tell tools to use their legacy confirmation flow - // eslint-disable-next-line @typescript-eslint/no-floating-promises - messageBus.publish({ - type: MessageBusType.TOOL_CONFIRMATION_RESPONSE, - correlationId: request.correlationId, - confirmed: false, - requiresUserConfirmation: true, - }); - }; + const messageBus = this.config.getMessageBus(); + + // Check if we've already subscribed a handler to this message bus + if (!CoreToolScheduler.subscribedMessageBuses.has(messageBus)) { + // Create a shared handler that will be used for this message bus + const sharedHandler = (request: ToolConfirmationRequest) => { + // When ASK_USER policy decision is made, respond with requiresUserConfirmation=true + // to tell tools to use their legacy confirmation flow + // eslint-disable-next-line @typescript-eslint/no-floating-promises + messageBus.publish({ + type: MessageBusType.TOOL_CONFIRMATION_RESPONSE, + correlationId: request.correlationId, + confirmed: false, + requiresUserConfirmation: true, + }); + }; - messageBus.subscribe( - MessageBusType.TOOL_CONFIRMATION_REQUEST, - sharedHandler, - ); + messageBus.subscribe( + MessageBusType.TOOL_CONFIRMATION_REQUEST, + sharedHandler, + ); - // Store the handler in the WeakMap so we don't subscribe again - CoreToolScheduler.subscribedMessageBuses.set(messageBus, sharedHandler); - } + // Store the handler in the WeakMap so we don't subscribe again + CoreToolScheduler.subscribedMessageBuses.set(messageBus, sharedHandler); } } diff --git a/packages/core/src/core/nonInteractiveToolExecutor.test.ts b/packages/core/src/core/nonInteractiveToolExecutor.test.ts index 42bd93c6b1c..a4f9524ec78 100644 --- a/packages/core/src/core/nonInteractiveToolExecutor.test.ts +++ b/packages/core/src/core/nonInteractiveToolExecutor.test.ts @@ -65,7 +65,6 @@ describe('executeToolCall', () => { getActiveModel: () => PREVIEW_GEMINI_MODEL, getUseSmartEdit: () => false, getGeminiClient: () => null, // No client needed for these tests - getEnableMessageBusIntegration: () => false, getMessageBus: () => null, getPolicyEngine: () => null, isInteractive: () => false, diff --git a/schemas/settings.schema.json b/schemas/settings.schema.json index dc496d2cfeb..f934a66b503 100644 --- a/schemas/settings.schema.json +++ b/schemas/settings.schema.json @@ -1128,13 +1128,6 @@ "default": 1000, "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": { "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.",