mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-04-02 17:31:05 -07:00
fix(core): ensure blue border overlay and input blocker to act correctly depending on browser agent activities (#24385)
This commit is contained in:
@@ -88,8 +88,6 @@ export async function injectAutomationOverlay(
|
||||
signal?: AbortSignal,
|
||||
): Promise<void> {
|
||||
try {
|
||||
debugLogger.log('Injecting automation overlay...');
|
||||
|
||||
const result = await browserManager.callTool(
|
||||
'evaluate_script',
|
||||
{ function: buildInjectionScript() },
|
||||
@@ -99,8 +97,6 @@ export async function injectAutomationOverlay(
|
||||
|
||||
if (result.isError) {
|
||||
debugLogger.warn('Failed to inject automation overlay:', result);
|
||||
} else {
|
||||
debugLogger.log('Automation overlay injected successfully');
|
||||
}
|
||||
} catch (error) {
|
||||
debugLogger.warn('Error injecting automation overlay:', error);
|
||||
@@ -115,8 +111,6 @@ export async function removeAutomationOverlay(
|
||||
signal?: AbortSignal,
|
||||
): Promise<void> {
|
||||
try {
|
||||
debugLogger.log('Removing automation overlay...');
|
||||
|
||||
const result = await browserManager.callTool(
|
||||
'evaluate_script',
|
||||
{ function: buildRemovalScript() },
|
||||
@@ -126,8 +120,6 @@ export async function removeAutomationOverlay(
|
||||
|
||||
if (result.isError) {
|
||||
debugLogger.warn('Failed to remove automation overlay:', result);
|
||||
} else {
|
||||
debugLogger.log('Automation overlay removed successfully');
|
||||
}
|
||||
} catch (error) {
|
||||
debugLogger.warn('Error removing automation overlay:', error);
|
||||
|
||||
@@ -32,6 +32,10 @@ vi.mock('./inputBlocker.js', () => ({
|
||||
removeInputBlocker: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock('./automationOverlay.js', () => ({
|
||||
removeAutomationOverlay: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock('../local-executor.js', () => ({
|
||||
LocalAgentExecutor: {
|
||||
create: vi.fn(),
|
||||
@@ -40,6 +44,7 @@ vi.mock('../local-executor.js', () => ({
|
||||
|
||||
import { createBrowserAgentDefinition } from './browserAgentFactory.js';
|
||||
import { removeInputBlocker } from './inputBlocker.js';
|
||||
import { removeAutomationOverlay } from './automationOverlay.js';
|
||||
import { LocalAgentExecutor } from '../local-executor.js';
|
||||
import type { ToolLiveOutput } from '../../tools/tools.js';
|
||||
|
||||
@@ -677,4 +682,81 @@ describe('BrowserAgentInvocation', () => {
|
||||
expect(toolB?.status).toBe('error');
|
||||
});
|
||||
});
|
||||
|
||||
describe('cleanup', () => {
|
||||
it('should clean up all pages on finally', async () => {
|
||||
const mockBrowserManager = {
|
||||
callTool: vi.fn().mockImplementation(async (toolName: string) => {
|
||||
if (toolName === 'list_pages') {
|
||||
return {
|
||||
content: [{ type: 'text', text: '0: Page 1\n1: Page 2\n' }],
|
||||
isError: false,
|
||||
};
|
||||
}
|
||||
return { isError: false };
|
||||
}),
|
||||
};
|
||||
|
||||
vi.mocked(createBrowserAgentDefinition).mockResolvedValue({
|
||||
definition: {
|
||||
name: 'browser_agent',
|
||||
description: 'mock definition',
|
||||
kind: 'local',
|
||||
inputConfig: {} as never,
|
||||
outputConfig: {} as never,
|
||||
processOutput: () => '',
|
||||
modelConfig: { model: 'test' },
|
||||
runConfig: {},
|
||||
promptConfig: { query: '', systemPrompt: '' },
|
||||
toolConfig: { tools: [] },
|
||||
},
|
||||
browserManager: mockBrowserManager as never,
|
||||
});
|
||||
|
||||
const mockExecutor = {
|
||||
run: vi.fn().mockResolvedValue({
|
||||
result: JSON.stringify({ success: true }),
|
||||
terminate_reason: 'GOAL',
|
||||
}),
|
||||
};
|
||||
|
||||
vi.mocked(LocalAgentExecutor.create).mockResolvedValue(
|
||||
mockExecutor as never,
|
||||
);
|
||||
|
||||
const invocation = new BrowserAgentInvocation(
|
||||
mockConfig,
|
||||
{ task: 'test' },
|
||||
mockMessageBus,
|
||||
);
|
||||
|
||||
await invocation.execute(new AbortController().signal);
|
||||
|
||||
// Verify list_pages was called
|
||||
expect(mockBrowserManager.callTool).toHaveBeenCalledWith(
|
||||
'list_pages',
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
true,
|
||||
);
|
||||
|
||||
// Verify select_page was called for each page
|
||||
expect(mockBrowserManager.callTool).toHaveBeenCalledWith(
|
||||
'select_page',
|
||||
{ pageId: 0, bringToFront: false },
|
||||
expect.anything(),
|
||||
true,
|
||||
);
|
||||
expect(mockBrowserManager.callTool).toHaveBeenCalledWith(
|
||||
'select_page',
|
||||
{ pageId: 1, bringToFront: false },
|
||||
expect.anything(),
|
||||
true,
|
||||
);
|
||||
|
||||
// Verify removeInputBlocker and removeAutomationOverlay were called for each page + initial cleanup
|
||||
expect(removeInputBlocker).toHaveBeenCalledTimes(3);
|
||||
expect(removeAutomationOverlay).toHaveBeenCalledTimes(3);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -40,6 +40,7 @@ import {
|
||||
sanitizeToolArgs,
|
||||
sanitizeErrorMessage,
|
||||
} from '../../utils/agent-sanitization-utils.js';
|
||||
import { removeAutomationOverlay } from './automationOverlay.js';
|
||||
|
||||
const INPUT_PREVIEW_MAX_LENGTH = 50;
|
||||
const DESCRIPTION_MAX_LENGTH = 200;
|
||||
@@ -377,7 +378,40 @@ ${output.result}`;
|
||||
} finally {
|
||||
// Clean up input blocker, but keep browserManager alive for persistent sessions
|
||||
if (browserManager) {
|
||||
await removeInputBlocker(browserManager);
|
||||
await removeInputBlocker(browserManager, signal);
|
||||
await removeAutomationOverlay(browserManager, signal);
|
||||
|
||||
// try cleaning up overlays in previous opened pages if any
|
||||
try {
|
||||
const listResult = await browserManager.callTool(
|
||||
'list_pages',
|
||||
{},
|
||||
signal,
|
||||
true,
|
||||
);
|
||||
const pagesText =
|
||||
listResult.content?.find((c) => c.type === 'text')?.text || '';
|
||||
const pageMatches = Array.from(pagesText.matchAll(/^(\d+):/gm));
|
||||
const pageIds = pageMatches.map((m) => parseInt(m[1], 10));
|
||||
if (pageIds.length > 1) {
|
||||
for (const pageId of pageIds) {
|
||||
try {
|
||||
await browserManager.callTool(
|
||||
'select_page',
|
||||
{ pageId, bringToFront: false },
|
||||
signal,
|
||||
true,
|
||||
);
|
||||
await removeInputBlocker(browserManager, signal);
|
||||
await removeAutomationOverlay(browserManager, signal);
|
||||
} catch (_err) {
|
||||
// Ignore errors for individual pages
|
||||
}
|
||||
}
|
||||
}
|
||||
} catch (_) {
|
||||
// Ignore errors for removing the overlays.
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -996,6 +996,28 @@ describe('BrowserManager', () => {
|
||||
const manager = new BrowserManager(mockConfig);
|
||||
await manager.callTool('click', { uid: 'bad' });
|
||||
});
|
||||
|
||||
it('should NOT re-inject overlay if select_page is called with bringToFront: false', async () => {
|
||||
mockConfig = makeFakeConfig({
|
||||
agents: {
|
||||
overrides: {
|
||||
browser_agent: {
|
||||
enabled: true,
|
||||
},
|
||||
},
|
||||
browser: {
|
||||
headless: false,
|
||||
disableUserInput: true,
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const manager = new BrowserManager(mockConfig);
|
||||
await manager.callTool('select_page', { pageId: 1, bringToFront: false });
|
||||
|
||||
expect(injectAutomationOverlay).not.toHaveBeenCalled();
|
||||
expect(injectInputBlocker).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('Rate limiting', () => {
|
||||
|
||||
@@ -302,6 +302,11 @@ export class BrowserManager {
|
||||
POTENTIALLY_NAVIGATING_TOOLS.has(toolName) &&
|
||||
!signal?.aborted
|
||||
) {
|
||||
// Don't re-inject if explicitly switching to a page in the background
|
||||
if (toolName === 'select_page' && args['bringToFront'] === false) {
|
||||
return result;
|
||||
}
|
||||
|
||||
try {
|
||||
if (this.shouldInjectOverlay) {
|
||||
await injectAutomationOverlay(this, signal);
|
||||
@@ -601,7 +606,6 @@ export class BrowserManager {
|
||||
await this.rawMcpClient!.connect(this.mcpTransport!);
|
||||
debugLogger.log('MCP client connected to chrome-devtools-mcp');
|
||||
await this.discoverTools();
|
||||
this.registerInputBlockerHandler();
|
||||
// clear the action counter for each connection
|
||||
this.actionCounter = 0;
|
||||
})(),
|
||||
@@ -805,45 +809,4 @@ export class BrowserManager {
|
||||
// If none matched, then deny
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Registers a fallback notification handler on the MCP client to
|
||||
* automatically re-inject the input blocker after any server-side
|
||||
* notification (e.g. page navigation, resource updates).
|
||||
*
|
||||
* This covers ALL navigation types (link clicks, form submissions,
|
||||
* history navigation) — not just explicit navigate_page tool calls.
|
||||
*/
|
||||
private registerInputBlockerHandler(): void {
|
||||
if (!this.rawMcpClient) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (!this.config.shouldDisableBrowserUserInput()) {
|
||||
return;
|
||||
}
|
||||
|
||||
const existingHandler = this.rawMcpClient.fallbackNotificationHandler;
|
||||
this.rawMcpClient.fallbackNotificationHandler = async (notification: {
|
||||
method: string;
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
params?: any;
|
||||
}) => {
|
||||
// Chain with any existing handler first.
|
||||
if (existingHandler) {
|
||||
await existingHandler(notification);
|
||||
}
|
||||
|
||||
// Only re-inject on resource update notifications which indicate
|
||||
// page content has changed (navigation, new page, etc.)
|
||||
if (notification.method === 'notifications/resources/updated') {
|
||||
debugLogger.log('Page content changed, re-injecting input blocker...');
|
||||
void injectInputBlocker(this);
|
||||
}
|
||||
};
|
||||
|
||||
debugLogger.log(
|
||||
'Registered global notification handler for input blocker re-injection',
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -207,7 +207,6 @@ export async function injectInputBlocker(
|
||||
signal,
|
||||
true,
|
||||
);
|
||||
debugLogger.log('Input blocker injected successfully');
|
||||
} catch (error) {
|
||||
// Log but don't throw - input blocker is a UX enhancement, not critical functionality
|
||||
debugLogger.warn(
|
||||
@@ -235,7 +234,6 @@ export async function removeInputBlocker(
|
||||
signal,
|
||||
true,
|
||||
);
|
||||
debugLogger.log('Input blocker removed successfully');
|
||||
} catch (error) {
|
||||
// Log but don't throw - removal failure is not critical
|
||||
debugLogger.warn(
|
||||
|
||||
Reference in New Issue
Block a user