From c991e5b3dc4436708143f54f9f2b770500030916 Mon Sep 17 00:00:00 2001 From: Gaurav Ghosh Date: Mon, 23 Feb 2026 23:01:56 -0800 Subject: [PATCH] fix: address PR #19284 review comments - Remove redundant Promise.race in McpToolInvocation.execute (event listener leak) - Propagate AbortSignal to all press_key calls (submitKey + typeCharByChar) - Call this.close() on connectMcp failure (zombie process leak) - Set showInDialog: false for all browser settings - Remove debug log truncation in analyzeScreenshot - Fix misleading --experimental-vision error message - Replace any casts with typed TestableConfirmation interface in tests - Update license year to 2026 in all browser agent files - Merge duplicate imports in mcpToolWrapper - Add sync comment to BrowserAgentCustomConfig - Update subagents.md Chrome requirement wording - Regenerate settings docs --- docs/cli/settings.md | 9 ----- docs/core/subagents.md | 5 ++- packages/cli/src/config/settingsSchema.ts | 8 ++-- .../cli/src/ui/components/SettingsDialog.tsx | 3 +- .../agents/browser/analyzeScreenshot.test.ts | 2 +- .../src/agents/browser/analyzeScreenshot.ts | 12 ++---- .../agents/browser/browserAgentDefinition.ts | 2 +- .../browser/browserAgentFactory.test.ts | 2 +- .../src/agents/browser/browserAgentFactory.ts | 4 +- .../browser/browserAgentInvocation.test.ts | 2 +- .../agents/browser/browserAgentInvocation.ts | 2 +- .../src/agents/browser/browserManager.test.ts | 2 +- .../core/src/agents/browser/browserManager.ts | 4 +- .../src/agents/browser/mcpToolWrapper.test.ts | 2 +- .../core/src/agents/browser/mcpToolWrapper.ts | 31 +++++++--------- .../mcpToolWrapperConfirmation.test.ts | 37 ++++++++++++------- .../src/agents/browser/modelAvailability.ts | 2 +- packages/core/src/config/config.ts | 6 ++- 18 files changed, 65 insertions(+), 70 deletions(-) diff --git a/docs/cli/settings.md b/docs/cli/settings.md index 3b754682bf..5011f55b2c 100644 --- a/docs/cli/settings.md +++ b/docs/cli/settings.md @@ -87,15 +87,6 @@ they appear in the UI. | Disable Loop Detection | `model.disableLoopDetection` | Disable automatic detection and prevention of infinite loops. | `false` | | Skip Next Speaker Check | `model.skipNextSpeakerCheck` | Skip the next speaker check. | `true` | -### Agents - -| UI Label | Setting | Description | Default | -| -------------------- | ---------------------------- | ---------------------------------------------------------- | -------------- | -| Browser Session Mode | `agents.browser.sessionMode` | Session mode: 'persistent', 'isolated', or 'existing'. | `"persistent"` | -| Browser Headless | `agents.browser.headless` | Run browser in headless mode. | `false` | -| Browser Profile Path | `agents.browser.profilePath` | Path to browser profile directory for session persistence. | `undefined` | -| Browser Visual Model | `agents.browser.visualModel` | Model override for the visual agent. | `undefined` | - ### Context | UI Label | Setting | Description | Default | diff --git a/docs/core/subagents.md b/docs/core/subagents.md index ae0302e231..e84f46dd8c 100644 --- a/docs/core/subagents.md +++ b/docs/core/subagents.md @@ -96,7 +96,7 @@ Gemini CLI comes with the following built-in subagents: The browser agent requires: -- **Chrome** version 144 or later installed on your system. +- **Chrome** version 144 or later (any recent stable release will work). - **Node.js** with `npx` available (used to launch the [`chrome-devtools-mcp`](https://www.npmjs.com/package/chrome-devtools-mcp) server). @@ -193,6 +193,9 @@ captures a screenshot and sends it to the vision model for analysis. The model returns coordinates and element descriptions that the browser agent uses with the `click_at` tool for precise, coordinate-based interactions. +> **Note:** The visual agent requires API key or Vertex AI authentication. It is +> not available when using Google Login. + ## Creating custom subagents You can create your own subagents to automate specific workflows or enforce diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 3a8703ec24..ff238672ff 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -981,7 +981,7 @@ const SETTINGS_SCHEMA = { default: 'persistent', description: "Session mode: 'persistent', 'isolated', or 'existing'.", - showInDialog: true, + showInDialog: false, options: [ { value: 'persistent', label: 'Persistent' }, { value: 'isolated', label: 'Isolated' }, @@ -995,7 +995,7 @@ const SETTINGS_SCHEMA = { requiresRestart: true, default: false, description: 'Run browser in headless mode.', - showInDialog: true, + showInDialog: false, }, profilePath: { type: 'string', @@ -1005,7 +1005,7 @@ const SETTINGS_SCHEMA = { default: undefined as string | undefined, description: 'Path to browser profile directory for session persistence.', - showInDialog: true, + showInDialog: false, }, visualModel: { type: 'string', @@ -1014,7 +1014,7 @@ const SETTINGS_SCHEMA = { requiresRestart: true, default: undefined as string | undefined, description: 'Model override for the visual agent.', - showInDialog: true, + showInDialog: false, }, }, }, diff --git a/packages/cli/src/ui/components/SettingsDialog.tsx b/packages/cli/src/ui/components/SettingsDialog.tsx index 4b2c164d3b..e426e9bbe3 100644 --- a/packages/cli/src/ui/components/SettingsDialog.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.tsx @@ -170,8 +170,7 @@ export function SettingsDialog({ updated = setPendingSettingValue(key, value, updated); } else if ( (def?.type === 'number' && typeof value === 'number') || - (def?.type === 'string' && typeof value === 'string') || - (def?.type === 'enum' && typeof value === 'string') + (def?.type === 'string' && typeof value === 'string') ) { updated = setPendingSettingValueAny(key, value, updated); } diff --git a/packages/core/src/agents/browser/analyzeScreenshot.test.ts b/packages/core/src/agents/browser/analyzeScreenshot.test.ts index 34c8148668..71e082b75d 100644 --- a/packages/core/src/agents/browser/analyzeScreenshot.test.ts +++ b/packages/core/src/agents/browser/analyzeScreenshot.test.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ diff --git a/packages/core/src/agents/browser/analyzeScreenshot.ts b/packages/core/src/agents/browser/analyzeScreenshot.ts index 0aa7971063..c269b71bfb 100644 --- a/packages/core/src/agents/browser/analyzeScreenshot.ts +++ b/packages/core/src/agents/browser/analyzeScreenshot.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ @@ -73,11 +73,7 @@ class AnalyzeScreenshotInvocation extends BaseToolInvocation< getDescription(): string { const instruction = String(this.params['instruction'] ?? ''); - const preview = - instruction.length > 50 - ? instruction.substring(0, 50) + '...' - : instruction; - return `Visual analysis: "${preview}"`; + return `Visual analysis: "${instruction}"`; } async execute(signal: AbortSignal): Promise { @@ -166,9 +162,7 @@ class AnalyzeScreenshotInvocation extends BaseToolInvocation< }; } - debugLogger.log( - `Visual analysis complete: ${responseText.slice(0, 100)}`, - ); + debugLogger.log(`Visual analysis complete: ${responseText}`); return { llmContent: `Visual Analysis Result:\n${responseText}`, diff --git a/packages/core/src/agents/browser/browserAgentDefinition.ts b/packages/core/src/agents/browser/browserAgentDefinition.ts index e1f733d17e..2703f53930 100644 --- a/packages/core/src/agents/browser/browserAgentDefinition.ts +++ b/packages/core/src/agents/browser/browserAgentDefinition.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ diff --git a/packages/core/src/agents/browser/browserAgentFactory.test.ts b/packages/core/src/agents/browser/browserAgentFactory.test.ts index e5e48f13a5..a317f3a9ed 100644 --- a/packages/core/src/agents/browser/browserAgentFactory.test.ts +++ b/packages/core/src/agents/browser/browserAgentFactory.test.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ diff --git a/packages/core/src/agents/browser/browserAgentFactory.ts b/packages/core/src/agents/browser/browserAgentFactory.ts index 8b6fabf45f..a8a3b0f338 100644 --- a/packages/core/src/agents/browser/browserAgentFactory.ts +++ b/packages/core/src/agents/browser/browserAgentFactory.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ @@ -98,7 +98,7 @@ export async function createBrowserAgentDefinition( if (missingVisualTools.length > 0) { return ( `Visual tools missing (${missingVisualTools.join(', ')}). ` + - `Ensure chrome-devtools-mcp is started with --experimental-vision.` + `The installed chrome-devtools-mcp version may be too old.` ); } const authType = config.getContentGeneratorConfig()?.authType; diff --git a/packages/core/src/agents/browser/browserAgentInvocation.test.ts b/packages/core/src/agents/browser/browserAgentInvocation.test.ts index 3f2ebad7c4..b58a9c409e 100644 --- a/packages/core/src/agents/browser/browserAgentInvocation.test.ts +++ b/packages/core/src/agents/browser/browserAgentInvocation.test.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ diff --git a/packages/core/src/agents/browser/browserAgentInvocation.ts b/packages/core/src/agents/browser/browserAgentInvocation.ts index 2a7ea3e331..0de9564c39 100644 --- a/packages/core/src/agents/browser/browserAgentInvocation.ts +++ b/packages/core/src/agents/browser/browserAgentInvocation.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ diff --git a/packages/core/src/agents/browser/browserManager.test.ts b/packages/core/src/agents/browser/browserManager.test.ts index da01928662..d60f6ab982 100644 --- a/packages/core/src/agents/browser/browserManager.test.ts +++ b/packages/core/src/agents/browser/browserManager.test.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ diff --git a/packages/core/src/agents/browser/browserManager.ts b/packages/core/src/agents/browser/browserManager.ts index 2f17a0db2c..6abaa6b233 100644 --- a/packages/core/src/agents/browser/browserManager.ts +++ b/packages/core/src/agents/browser/browserManager.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ @@ -324,6 +324,8 @@ export class BrowserManager { }), ]); } catch (error) { + await this.close(); + // Provide error-specific, session-mode-aware remediation throw this.createConnectionError( error instanceof Error ? error.message : String(error), diff --git a/packages/core/src/agents/browser/mcpToolWrapper.test.ts b/packages/core/src/agents/browser/mcpToolWrapper.test.ts index 49b5accc0c..a99ff4943c 100644 --- a/packages/core/src/agents/browser/mcpToolWrapper.test.ts +++ b/packages/core/src/agents/browser/mcpToolWrapper.test.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ diff --git a/packages/core/src/agents/browser/mcpToolWrapper.ts b/packages/core/src/agents/browser/mcpToolWrapper.ts index 8572ca9d21..1838a01b42 100644 --- a/packages/core/src/agents/browser/mcpToolWrapper.ts +++ b/packages/core/src/agents/browser/mcpToolWrapper.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ @@ -17,8 +17,8 @@ import type { FunctionDeclaration } from '@google/genai'; import type { Tool as McpTool } from '@modelcontextprotocol/sdk/types.js'; -import type { ToolConfirmationOutcome } from '../../tools/tools.js'; import { + type ToolConfirmationOutcome, DeclarativeTool, BaseToolInvocation, Kind, @@ -86,18 +86,7 @@ class McpToolInvocation extends BaseToolInvocation< signal, ); - const result: McpToolCallResult = await (signal.aborted - ? Promise.reject(signal.reason ?? new Error('Operation cancelled')) - : Promise.race([ - callToolPromise, - new Promise((_resolve, reject) => { - signal.addEventListener( - 'abort', - () => reject(signal.reason ?? new Error('Operation cancelled')), - { once: true }, - ); - }), - ])); + const result: McpToolCallResult = await callToolPromise; // Extract text content from MCP response let textContent = ''; @@ -210,9 +199,11 @@ class TypeTextInvocation extends BaseToolInvocation< // Optionally press a submit key (Enter, Tab, etc.) after typing if (this.submitKey && !signal.aborted) { - const keyResult = await this.browserManager.callTool('press_key', { - key: this.submitKey, - }); + const keyResult = await this.browserManager.callTool( + 'press_key', + { key: this.submitKey }, + signal, + ); if (keyResult.isError) { const errText = this.extractErrorText(keyResult); debugLogger.warn( @@ -254,7 +245,11 @@ class TypeTextInvocation extends BaseToolInvocation< // Map special characters to key names const key = char === ' ' ? 'Space' : char; - const result = await this.browserManager.callTool('press_key', { key }); + const result = await this.browserManager.callTool( + 'press_key', + { key }, + signal, + ); if (result.isError) { debugLogger.warn( diff --git a/packages/core/src/agents/browser/mcpToolWrapperConfirmation.test.ts b/packages/core/src/agents/browser/mcpToolWrapperConfirmation.test.ts index c108cdb08d..c2baf9e77d 100644 --- a/packages/core/src/agents/browser/mcpToolWrapperConfirmation.test.ts +++ b/packages/core/src/agents/browser/mcpToolWrapperConfirmation.test.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ @@ -9,7 +9,20 @@ import { createMcpDeclarativeTools } from './mcpToolWrapper.js'; import type { BrowserManager } from './browserManager.js'; import type { MessageBus } from '../../confirmation-bus/message-bus.js'; import { MessageBusType } from '../../confirmation-bus/types.js'; -import { ToolConfirmationOutcome } from '../../tools/tools.js'; +import { + ToolConfirmationOutcome, + type ToolCallConfirmationDetails, + type PolicyUpdateOptions, +} from '../../tools/tools.js'; + +interface TestableConfirmation { + getConfirmationDetails( + signal: AbortSignal, + ): Promise; + getPolicyUpdateOptions( + outcome: ToolConfirmationOutcome, + ): PolicyUpdateOptions | undefined; +} describe('mcpToolWrapper Confirmation', () => { let mockBrowserManager: BrowserManager; @@ -25,7 +38,6 @@ describe('mcpToolWrapper Confirmation', () => { callTool: vi.fn(), } as unknown as BrowserManager; - // We accept any cast here because we are mocking the interface mockMessageBus = { publish: vi.fn().mockResolvedValue(undefined), subscribe: vi.fn(), @@ -38,11 +50,9 @@ describe('mcpToolWrapper Confirmation', () => { mockBrowserManager, mockMessageBus, ); - const invocation = tools[0].build({}); + const invocation = tools[0].build({}) as unknown as TestableConfirmation; - // Use "any" to access protected method - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const details = await (invocation as any).getConfirmationDetails( + const details = await invocation.getConfirmationDetails( new AbortController().signal, ); @@ -57,15 +67,15 @@ describe('mcpToolWrapper Confirmation', () => { // Verify onConfirm publishes policy update const outcome = ToolConfirmationOutcome.ProceedAlways; - await details.onConfirm(outcome); + if (details && typeof details === 'object' && 'onConfirm' in details) { + await details.onConfirm(outcome); + } expect(mockMessageBus.publish).toHaveBeenCalledWith( expect.objectContaining({ type: MessageBusType.UPDATE_POLICY, mcpName: 'browser-agent', - persist: false, // ProceedAlwaysServer doesn't persist by default unless specified otherwise in logic? - // Wait, BaseToolInvocation.publishPolicyUpdate handles logic. - // If outcome is ProceedAlwaysServer, BaseToolInvocation doesn't do anything by default! + persist: false, }), ); }); @@ -75,10 +85,9 @@ describe('mcpToolWrapper Confirmation', () => { mockBrowserManager, mockMessageBus, ); - const invocation = tools[0].build({}); + const invocation = tools[0].build({}) as unknown as TestableConfirmation; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const options = (invocation as any).getPolicyUpdateOptions( + const options = invocation.getPolicyUpdateOptions( ToolConfirmationOutcome.ProceedAlways, ); diff --git a/packages/core/src/agents/browser/modelAvailability.ts b/packages/core/src/agents/browser/modelAvailability.ts index 1a30aae89a..358d498aa4 100644 --- a/packages/core/src/agents/browser/modelAvailability.ts +++ b/packages/core/src/agents/browser/modelAvailability.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 511642d76b..ba9d212644 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -196,7 +196,6 @@ export interface AgentRunConfig { /** * Override configuration for a specific agent. * Generic fields (modelConfig, runConfig, enabled) are standard across all agents. - * Agent-specific settings go in customConfig - each agent defines its own type. */ export interface AgentOverride { modelConfig?: ModelConfig; @@ -262,7 +261,10 @@ export interface CustomTheme { /** * Browser agent custom configuration. - * Used in agents.overrides.browser_agent.customConfig + * Used in agents.browser + * + * IMPORTANT: Keep in sync with the browser settings schema in + * packages/cli/src/config/settingsSchema.ts (agents.browser.properties). */ export interface BrowserAgentCustomConfig { /**