MediaMetz/gemini-cli
1
0
Fork 0
mirror of https://github.com/google-gemini/gemini-cli.git synced 2026-06-10 11:12:35 -07:00
Code Issues Packages Projects Releases Wiki Activity

fix: address PR #19284 review comments

Browse Source 
- 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
This commit is contained in:
Gaurav Ghosh
2026-02-23 23:01:56 -08:00
parent c1560d99fd
commit c991e5b3dc
18 changed files with 65 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/cli/settings.md
-9
View File
@@ -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 |
docs/core/subagents.md
+4 -1
View File
@@ -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
packages/cli/src/config/settingsSchema.ts
+4 -4
View File
@@ -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,
},
},
},
packages/cli/src/ui/components/SettingsDialog.tsx
+1 -2
View File
@@ -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);
}
packages/core/src/agents/browser/analyzeScreenshot.test.ts
+1 -1
View File
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
packages/core/src/agents/browser/analyzeScreenshot.ts
+3 -9
View File
@@ -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<ToolResult> {
@@ -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}`,
packages/core/src/agents/browser/browserAgentDefinition.ts
+1 -1
View File
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
packages/core/src/agents/browser/browserAgentFactory.test.ts
+1 -1
View File
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
packages/core/src/agents/browser/browserAgentFactory.ts
+2 -2
View File
@@ -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;
packages/core/src/agents/browser/browserAgentInvocation.test.ts
+1 -1
View File
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
packages/core/src/agents/browser/browserAgentInvocation.ts
+1 -1
View File
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
packages/core/src/agents/browser/browserManager.test.ts
+1 -1
View File
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
packages/core/src/agents/browser/browserManager.ts
+3 -1
View File
@@ -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),
packages/core/src/agents/browser/mcpToolWrapper.test.ts
+1 -1
View File
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
packages/core/src/agents/browser/mcpToolWrapper.ts
+13 -18
View File
@@ -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<never>((_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(
packages/core/src/agents/browser/mcpToolWrapperConfirmation.test.ts
+23 -14
View File
@@ -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<ToolCallConfirmationDetails | false>;
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,
);
packages/core/src/agents/browser/modelAvailability.ts
+1 -1
View File
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
packages/core/src/config/config.ts
+4 -2
View File
@@ -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 {
/**