mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-05-26 11:43:24 -07:00
fix(browser): correct session mode CLI flags and add connection validation
Fix chrome-devtools-mcp CLI flags: - --existing (invalid) → --autoConnect for existing session mode - --profile-path (invalid) → --userDataDir for custom profile path - Default session mode changed from 'isolated' to 'persistent' Add 'persistent' session mode (new default) which uses a persistent Chrome profile at ~/.cache/chrome-devtools-mcp/chrome-profile. Add connection timeout and actionable error for 'existing' mode when Chrome remote debugging is not enabled.
This commit is contained in:
@@ -138,7 +138,7 @@ describe('BrowserManager', () => {
|
||||
});
|
||||
|
||||
describe('MCP connection', () => {
|
||||
it('should spawn npx chrome-devtools-mcp with --isolated and --experimental-vision', async () => {
|
||||
it('should spawn npx chrome-devtools-mcp with --experimental-vision (persistent mode by default)', async () => {
|
||||
const manager = new BrowserManager(mockConfig);
|
||||
await manager.ensureConnection();
|
||||
|
||||
@@ -148,10 +148,13 @@ describe('BrowserManager', () => {
|
||||
args: expect.arrayContaining([
|
||||
'-y',
|
||||
expect.stringMatching(/chrome-devtools-mcp@/),
|
||||
'--isolated',
|
||||
'--experimental-vision',
|
||||
]),
|
||||
});
|
||||
// Persistent mode should NOT include --isolated or --autoConnect
|
||||
const args = vi.mocked(StdioClientTransport).mock.calls[0]?.[0]?.args as string[];
|
||||
expect(args).not.toContain('--isolated');
|
||||
expect(args).not.toContain('--autoConnect');
|
||||
});
|
||||
|
||||
it('should pass headless flag when configured', async () => {
|
||||
@@ -177,7 +180,7 @@ describe('BrowserManager', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('should pass chromeProfilePath when configured', async () => {
|
||||
it('should pass chromeProfilePath as --userDataDir when configured', async () => {
|
||||
const profileConfig = makeFakeConfig({
|
||||
agents: {
|
||||
overrides: {
|
||||
@@ -196,9 +199,90 @@ describe('BrowserManager', () => {
|
||||
|
||||
expect(StdioClientTransport).toHaveBeenCalledWith({
|
||||
command: 'npx',
|
||||
args: expect.arrayContaining(['--profile-path', '/path/to/profile']),
|
||||
args: expect.arrayContaining(['--userDataDir', '/path/to/profile']),
|
||||
});
|
||||
});
|
||||
|
||||
it('should pass --isolated when sessionMode is isolated', async () => {
|
||||
const isolatedConfig = makeFakeConfig({
|
||||
agents: {
|
||||
overrides: {
|
||||
browser_agent: {
|
||||
enabled: true,
|
||||
customConfig: {
|
||||
sessionMode: 'isolated',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const manager = new BrowserManager(isolatedConfig);
|
||||
await manager.ensureConnection();
|
||||
|
||||
const args = vi.mocked(StdioClientTransport).mock.calls[0]?.[0]?.args as string[];
|
||||
expect(args).toContain('--isolated');
|
||||
expect(args).not.toContain('--autoConnect');
|
||||
});
|
||||
|
||||
it('should pass --autoConnect when sessionMode is existing', async () => {
|
||||
const existingConfig = makeFakeConfig({
|
||||
agents: {
|
||||
overrides: {
|
||||
browser_agent: {
|
||||
enabled: true,
|
||||
customConfig: {
|
||||
sessionMode: 'existing',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const manager = new BrowserManager(existingConfig);
|
||||
await manager.ensureConnection();
|
||||
|
||||
const args = vi.mocked(StdioClientTransport).mock.calls[0]?.[0]?.args as string[];
|
||||
expect(args).toContain('--autoConnect');
|
||||
expect(args).not.toContain('--isolated');
|
||||
});
|
||||
|
||||
it('should throw actionable error when existing mode connection fails', async () => {
|
||||
// Make the Client mock's connect method reject
|
||||
vi.mocked(Client).mockImplementation(
|
||||
() =>
|
||||
({
|
||||
connect: vi
|
||||
.fn()
|
||||
.mockRejectedValue(new Error('Connection refused')),
|
||||
close: vi.fn().mockResolvedValue(undefined),
|
||||
listTools: vi.fn(),
|
||||
callTool: vi.fn(),
|
||||
}) as unknown as InstanceType<typeof Client>,
|
||||
);
|
||||
|
||||
const existingConfig = makeFakeConfig({
|
||||
agents: {
|
||||
overrides: {
|
||||
browser_agent: {
|
||||
enabled: true,
|
||||
customConfig: {
|
||||
sessionMode: 'existing',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const manager = new BrowserManager(existingConfig);
|
||||
|
||||
await expect(manager.ensureConnection()).rejects.toThrow(
|
||||
/Failed to connect to existing Chrome instance/,
|
||||
);
|
||||
await expect(manager.ensureConnection()).rejects.toThrow(
|
||||
/chrome:\/\/inspect\/#remote-debugging/,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('MCP isolation', () => {
|
||||
|
||||
@@ -237,22 +237,32 @@ export class BrowserManager {
|
||||
|
||||
// Build args for chrome-devtools-mcp
|
||||
const browserConfig = this.config.getBrowserAgentConfig();
|
||||
const sessionMode = browserConfig.customConfig.sessionMode ?? 'isolated';
|
||||
const sessionMode = browserConfig.customConfig.sessionMode ?? 'persistent';
|
||||
|
||||
const mcpArgs = [
|
||||
'-y',
|
||||
`chrome-devtools-mcp@${CHROME_DEVTOOLS_MCP_VERSION}`,
|
||||
sessionMode === 'existing' ? '--existing' : '--isolated',
|
||||
'--experimental-vision',
|
||||
];
|
||||
|
||||
// Session mode determines how the browser is managed:
|
||||
// - "isolated": Temp profile, cleaned up after session (--isolated)
|
||||
// - "persistent": Persistent profile at ~/.cache/chrome-devtools-mcp/ (default)
|
||||
// - "existing": Connect to already-running Chrome (--autoConnect, requires
|
||||
// remote debugging enabled at chrome://inspect/#remote-debugging)
|
||||
if (sessionMode === 'isolated') {
|
||||
mcpArgs.push('--isolated');
|
||||
} else if (sessionMode === 'existing') {
|
||||
mcpArgs.push('--autoConnect');
|
||||
}
|
||||
|
||||
// Add optional settings from config
|
||||
if (browserConfig.customConfig.headless) {
|
||||
mcpArgs.push('--headless');
|
||||
}
|
||||
if (browserConfig.customConfig.chromeProfilePath) {
|
||||
mcpArgs.push(
|
||||
'--profile-path',
|
||||
'--userDataDir',
|
||||
browserConfig.customConfig.chromeProfilePath,
|
||||
);
|
||||
}
|
||||
@@ -267,12 +277,50 @@ export class BrowserManager {
|
||||
args: mcpArgs,
|
||||
});
|
||||
|
||||
// Connect to MCP server
|
||||
await this.rawMcpClient.connect(this.mcpTransport);
|
||||
debugLogger.log('MCP client connected to chrome-devtools-mcp');
|
||||
// Connect to MCP server — use a shorter timeout for 'existing' mode
|
||||
// since it should connect quickly if remote debugging is enabled.
|
||||
const connectTimeoutMs =
|
||||
sessionMode === 'existing' ? 15_000 : MCP_TIMEOUT_MS;
|
||||
|
||||
// Discover tools from the MCP server
|
||||
await this.discoverTools();
|
||||
let timeoutId: ReturnType<typeof setTimeout> | undefined;
|
||||
try {
|
||||
await Promise.race([
|
||||
(async () => {
|
||||
await this.rawMcpClient!.connect(this.mcpTransport!);
|
||||
debugLogger.log('MCP client connected to chrome-devtools-mcp');
|
||||
await this.discoverTools();
|
||||
})(),
|
||||
new Promise<never>((_, reject) => {
|
||||
timeoutId = setTimeout(
|
||||
() =>
|
||||
reject(
|
||||
new Error(
|
||||
`Timed out connecting to chrome-devtools-mcp (${connectTimeoutMs}ms)`,
|
||||
),
|
||||
),
|
||||
connectTimeoutMs,
|
||||
);
|
||||
}),
|
||||
]);
|
||||
} catch (error) {
|
||||
// Provide actionable error for 'existing' mode failures
|
||||
if (sessionMode === 'existing') {
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
throw new Error(
|
||||
`Failed to connect to existing Chrome instance: ${message}\n\n` +
|
||||
`To use sessionMode "existing", you must:\n` +
|
||||
` 1. Open Chrome (version 144+)\n` +
|
||||
` 2. Navigate to chrome://inspect/#remote-debugging\n` +
|
||||
` 3. Enable remote debugging\n\n` +
|
||||
`Alternatively, use sessionMode "persistent" (default) to launch a dedicated browser.`,
|
||||
);
|
||||
}
|
||||
throw error;
|
||||
} finally {
|
||||
if (timeoutId !== undefined) {
|
||||
clearTimeout(timeoutId);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -1331,7 +1331,7 @@ describe('Server Config (config.ts)', () => {
|
||||
|
||||
expect(browserConfig.enabled).toBe(false);
|
||||
expect(browserConfig.model).toBeUndefined();
|
||||
expect(browserConfig.customConfig.sessionMode).toBe('isolated');
|
||||
expect(browserConfig.customConfig.sessionMode).toBe('persistent');
|
||||
expect(browserConfig.customConfig.headless).toBe(false);
|
||||
expect(browserConfig.customConfig.chromeProfilePath).toBeUndefined();
|
||||
expect(browserConfig.customConfig.visualModel).toBeUndefined();
|
||||
@@ -1390,7 +1390,7 @@ describe('Server Config (config.ts)', () => {
|
||||
expect(browserConfig.enabled).toBe(true);
|
||||
expect(browserConfig.customConfig.headless).toBe(true);
|
||||
// Defaults for unspecified fields
|
||||
expect(browserConfig.customConfig.sessionMode).toBe('isolated');
|
||||
expect(browserConfig.customConfig.sessionMode).toBe('persistent');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -270,8 +270,14 @@ export interface CustomTheme {
|
||||
* Used in agents.overrides.browser_agent.customConfig
|
||||
*/
|
||||
export interface BrowserAgentCustomConfig {
|
||||
/** Session mode: 'isolated' (launch new browser) or 'existing' (attach to Chrome M144+). Default: 'isolated' */
|
||||
sessionMode?: 'isolated' | 'existing';
|
||||
/**
|
||||
* Session mode:
|
||||
* - 'persistent': Launch Chrome with a persistent profile at ~/.cache/chrome-devtools-mcp/ (default)
|
||||
* - 'isolated': Launch Chrome with a temporary profile, cleaned up after session
|
||||
* - 'existing': Attach to an already-running Chrome instance (requires remote debugging
|
||||
* enabled at chrome://inspect/#remote-debugging)
|
||||
*/
|
||||
sessionMode?: 'isolated' | 'persistent' | 'existing';
|
||||
/** Run browser in headless mode. Default: false */
|
||||
headless?: boolean;
|
||||
/** Path to Chrome profile directory for session persistence. */
|
||||
@@ -2539,7 +2545,7 @@ export class Config {
|
||||
enabled: override?.enabled ?? false,
|
||||
model: override?.modelConfig?.model,
|
||||
customConfig: {
|
||||
sessionMode: customConfig.sessionMode ?? 'isolated',
|
||||
sessionMode: customConfig.sessionMode ?? 'persistent',
|
||||
headless: customConfig.headless ?? false,
|
||||
chromeProfilePath: customConfig.chromeProfilePath,
|
||||
visualModel: customConfig.visualModel,
|
||||
|
||||
Reference in New Issue
Block a user