Skip to content

Commit eee5d7c

Browse files
odysseus0claude
odysseus0
and
claude
authored
fix(browser): harden existing-session driver validation and session lifecycle (openclaw#45682)
* fix(browser): harden existing-session driver validation, session lifecycle, and code quality Fix config validation rejecting existing-session profiles that lack cdpPort/cdpUrl (they use Chrome MCP auto-connect instead). Fix callTool tearing down the MCP session on tool-level errors (element not found, script error), which caused expensive npx re-spawns. Skip unnecessary CDP port allocation for existing-session profiles. Remove redundant ensureChromeMcpAvailable call in isReachable. Extract shared ARIA role sets (INTERACTIVE_ROLES, CONTENT_ROLES, STRUCTURAL_ROLES) into snapshot-roles.ts so both the Playwright and Chrome MCP snapshot paths stay in sync. Add usesChromeMcp capability flag and replace ~20 scattered driver === "existing-session" string checks with the centralized flag. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(browser): harden existing-session driver validation and session lifecycle (openclaw#45682) (thanks @odysseus0) --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
1 parent 01674c5 commit eee5d7c

21 files changed

+256
-157
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ Docs: https://docs.openclaw.ai
1414

1515
### Fixes
1616

17+
- Browser/existing-session: harden driver validation and session lifecycle so transport errors trigger reconnects while tool-level errors preserve the session, and extract shared ARIA role sets to deduplicate Playwright and Chrome MCP snapshot paths. (#45682) Thanks @odysseus0.
1718
- Dashboard/chat UI: stop reloading full chat history on every live tool result in dashboard v2 so tool-heavy runs no longer trigger UI freeze/re-render storms while the final event still refreshes persisted history. (#45541) Thanks @BunsDev.
1819
- Ollama/reasoning visibility: stop promoting native `thinking` and `reasoning` fields into final assistant text so local reasoning models no longer leak internal thoughts in normal replies. (#45330) Thanks @xi7ang.
1920
- Android/onboarding QR scan: switch setup QR scanning to Google Code Scanner so onboarding uses a more reliable scanner instead of the legacy embedded ZXing flow. (#45021) Thanks @obviyus.

‎src/browser/chrome-mcp.snapshot.ts‎

Lines changed: 1 addition & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
type RoleRefMap,
55
type RoleSnapshotOptions,
66
} from "./pw-role-snapshot.js";
7+
import { CONTENT_ROLES, INTERACTIVE_ROLES, STRUCTURAL_ROLES } from "./snapshot-roles.js";
78

89
export type ChromeMcpSnapshotNode = {
910
id?: string;
@@ -14,60 +15,6 @@ export type ChromeMcpSnapshotNode = {
1415
children?: ChromeMcpSnapshotNode[];
1516
};
1617

17-
const INTERACTIVE_ROLES = new Set([
18-
"button",
19-
"checkbox",
20-
"combobox",
21-
"link",
22-
"listbox",
23-
"menuitem",
24-
"menuitemcheckbox",
25-
"menuitemradio",
26-
"option",
27-
"radio",
28-
"searchbox",
29-
"slider",
30-
"spinbutton",
31-
"switch",
32-
"tab",
33-
"textbox",
34-
"treeitem",
35-
]);
36-
37-
const CONTENT_ROLES = new Set([
38-
"article",
39-
"cell",
40-
"columnheader",
41-
"gridcell",
42-
"heading",
43-
"listitem",
44-
"main",
45-
"navigation",
46-
"region",
47-
"rowheader",
48-
]);
49-
50-
const STRUCTURAL_ROLES = new Set([
51-
"application",
52-
"directory",
53-
"document",
54-
"generic",
55-
"group",
56-
"ignored",
57-
"list",
58-
"menu",
59-
"menubar",
60-
"none",
61-
"presentation",
62-
"row",
63-
"rowgroup",
64-
"tablist",
65-
"table",
66-
"toolbar",
67-
"tree",
68-
"treegrid",
69-
]);
70-
7118
function normalizeRole(node: ChromeMcpSnapshotNode): string {
7219
const role = typeof node.role === "string" ? node.role.trim().toLowerCase() : "";
7320
return role || "generic";

‎src/browser/chrome-mcp.test.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,66 @@ describe("chrome MCP page parsing", () => {
190190
expect(result).toBe(123);
191191
});
192192

193+
it("preserves session after tool-level errors (isError)", async () => {
194+
let factoryCalls = 0;
195+
const factory: ChromeMcpSessionFactory = async () => {
196+
factoryCalls += 1;
197+
const session = createFakeSession();
198+
const callTool = vi.fn(async ({ name }: ToolCall) => {
199+
if (name === "evaluate_script") {
200+
return {
201+
content: [{ type: "text", text: "element not found" }],
202+
isError: true,
203+
};
204+
}
205+
if (name === "list_pages") {
206+
return {
207+
content: [{ type: "text", text: "## Pages\n1: https://example.com [selected]" }],
208+
};
209+
}
210+
throw new Error(`unexpected tool ${name}`);
211+
});
212+
session.client.callTool = callTool as typeof session.client.callTool;
213+
return session;
214+
};
215+
setChromeMcpSessionFactoryForTest(factory);
216+
217+
// First call: tool error (isError: true) — should NOT destroy session
218+
await expect(
219+
evaluateChromeMcpScript({ profileName: "chrome-live", targetId: "1", fn: "() => null" }),
220+
).rejects.toThrow(/element not found/);
221+
222+
// Second call: should reuse the same session (factory called only once)
223+
const tabs = await listChromeMcpTabs("chrome-live");
224+
expect(factoryCalls).toBe(1);
225+
expect(tabs).toHaveLength(1);
226+
});
227+
228+
it("destroys session on transport errors so next call reconnects", async () => {
229+
let factoryCalls = 0;
230+
const factory: ChromeMcpSessionFactory = async () => {
231+
factoryCalls += 1;
232+
const session = createFakeSession();
233+
if (factoryCalls === 1) {
234+
// First session: transport error (callTool throws)
235+
const callTool = vi.fn(async () => {
236+
throw new Error("connection reset");
237+
});
238+
session.client.callTool = callTool as typeof session.client.callTool;
239+
}
240+
return session;
241+
};
242+
setChromeMcpSessionFactoryForTest(factory);
243+
244+
// First call: transport error — should destroy session
245+
await expect(listChromeMcpTabs("chrome-live")).rejects.toThrow(/connection reset/);
246+
247+
// Second call: should create a new session (factory called twice)
248+
const tabs = await listChromeMcpTabs("chrome-live");
249+
expect(factoryCalls).toBe(2);
250+
expect(tabs).toHaveLength(2);
251+
});
252+
193253
it("clears failed pending sessions so the next call can retry", async () => {
194254
let factoryCalls = 0;
195255
const factory: ChromeMcpSessionFactory = async () => {

‎src/browser/chrome-mcp.ts‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -248,20 +248,24 @@ async function callTool(
248248
args: Record<string, unknown> = {},
249249
): Promise<ChromeMcpToolResult> {
250250
const session = await getSession(profileName);
251+
let result: ChromeMcpToolResult;
251252
try {
252-
const result = (await session.client.callTool({
253+
result = (await session.client.callTool({
253254
name,
254255
arguments: args,
255256
})) as ChromeMcpToolResult;
256-
if (result.isError) {
257-
throw new Error(extractToolErrorMessage(result, name));
258-
}
259-
return result;
260257
} catch (err) {
258+
// Transport/connection error — tear down session so it reconnects on next call
261259
sessions.delete(profileName);
262260
await session.client.close().catch(() => {});
263261
throw err;
264262
}
263+
// Tool-level errors (element not found, script error, etc.) don't indicate a
264+
// broken connection — don't tear down the session for these.
265+
if (result.isError) {
266+
throw new Error(extractToolErrorMessage(result, name));
267+
}
268+
return result;
265269
}
266270

267271
async function withTempFile<T>(fn: (filePath: string) => Promise<T>): Promise<T> {

‎src/browser/config.test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { describe, expect, it } from "vitest";
22
import { withEnv } from "../test-utils/env.js";
33
import { resolveBrowserConfig, resolveProfile, shouldStartLocalBrowserServer } from "./config.js";
4+
import { getBrowserProfileCapabilities } from "./profile-capabilities.js";
45

56
describe("browser config", () => {
67
it("defaults to enabled with loopback defaults and lobster-orange color", () => {
@@ -278,6 +279,47 @@ describe("browser config", () => {
278279
expect(resolved.ssrfPolicy).toEqual({});
279280
});
280281

282+
it("resolves existing-session profiles without cdpPort or cdpUrl", () => {
283+
const resolved = resolveBrowserConfig({
284+
profiles: {
285+
"chrome-live": {
286+
driver: "existing-session",
287+
attachOnly: true,
288+
color: "#00AA00",
289+
},
290+
},
291+
});
292+
const profile = resolveProfile(resolved, "chrome-live");
293+
expect(profile).not.toBeNull();
294+
expect(profile?.driver).toBe("existing-session");
295+
expect(profile?.attachOnly).toBe(true);
296+
expect(profile?.cdpPort).toBe(0);
297+
expect(profile?.cdpUrl).toBe("");
298+
expect(profile?.cdpIsLoopback).toBe(true);
299+
expect(profile?.color).toBe("#00AA00");
300+
});
301+
302+
it("sets usesChromeMcp only for existing-session profiles", () => {
303+
const resolved = resolveBrowserConfig({
304+
profiles: {
305+
"chrome-live": { driver: "existing-session", attachOnly: true, color: "#00AA00" },
306+
work: { cdpPort: 18801, color: "#0066CC" },
307+
},
308+
});
309+
310+
const existingSession = resolveProfile(resolved, "chrome-live")!;
311+
expect(getBrowserProfileCapabilities(existingSession).usesChromeMcp).toBe(true);
312+
313+
const managed = resolveProfile(resolved, "openclaw")!;
314+
expect(getBrowserProfileCapabilities(managed).usesChromeMcp).toBe(false);
315+
316+
const extension = resolveProfile(resolved, "chrome")!;
317+
expect(getBrowserProfileCapabilities(extension).usesChromeMcp).toBe(false);
318+
319+
const work = resolveProfile(resolved, "work")!;
320+
expect(getBrowserProfileCapabilities(work).usesChromeMcp).toBe(false);
321+
});
322+
281323
describe("default profile preference", () => {
282324
it("defaults to openclaw profile when defaultProfile is not configured", () => {
283325
const resolved = resolveBrowserConfig({

‎src/browser/config.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,20 @@ export function resolveProfile(
342342
? "existing-session"
343343
: "openclaw";
344344

345+
if (driver === "existing-session") {
346+
// existing-session uses Chrome MCP auto-connect; no CDP port/URL needed
347+
return {
348+
name: profileName,
349+
cdpPort: 0,
350+
cdpUrl: "",
351+
cdpHost: "",
352+
cdpIsLoopback: true,
353+
color: profile.color,
354+
driver,
355+
attachOnly: true,
356+
};
357+
}
358+
345359
if (rawProfileUrl) {
346360
const parsed = parseHttpUrl(rawProfileUrl, `browser.profiles.${profileName}.cdpUrl`);
347361
cdpHost = parsed.parsed.hostname;
@@ -361,7 +375,7 @@ export function resolveProfile(
361375
cdpIsLoopback: isLoopbackHost(cdpHost),
362376
color: profile.color,
363377
driver,
364-
attachOnly: driver === "existing-session" ? true : (profile.attachOnly ?? resolved.attachOnly),
378+
attachOnly: profile.attachOnly ?? resolved.attachOnly,
365379
};
366380
}
367381

‎src/browser/profile-capabilities.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ export type BrowserProfileMode =
99
export type BrowserProfileCapabilities = {
1010
mode: BrowserProfileMode;
1111
isRemote: boolean;
12+
/** Profile uses the Chrome DevTools MCP server (existing-session driver). */
13+
usesChromeMcp: boolean;
1214
requiresRelay: boolean;
1315
requiresAttachedTab: boolean;
1416
usesPersistentPlaywright: boolean;
@@ -25,6 +27,7 @@ export function getBrowserProfileCapabilities(
2527
return {
2628
mode: "local-extension-relay",
2729
isRemote: false,
30+
usesChromeMcp: false,
2831
requiresRelay: true,
2932
requiresAttachedTab: true,
3033
usesPersistentPlaywright: false,
@@ -39,6 +42,7 @@ export function getBrowserProfileCapabilities(
3942
return {
4043
mode: "local-existing-session",
4144
isRemote: false,
45+
usesChromeMcp: true,
4246
requiresRelay: false,
4347
requiresAttachedTab: false,
4448
usesPersistentPlaywright: false,
@@ -53,6 +57,7 @@ export function getBrowserProfileCapabilities(
5357
return {
5458
mode: "remote-cdp",
5559
isRemote: true,
60+
usesChromeMcp: false,
5661
requiresRelay: false,
5762
requiresAttachedTab: false,
5863
usesPersistentPlaywright: true,
@@ -66,6 +71,7 @@ export function getBrowserProfileCapabilities(
6671
return {
6772
mode: "local-managed",
6873
isRemote: false,
74+
usesChromeMcp: false,
6975
requiresRelay: false,
7076
requiresAttachedTab: false,
7177
usesPersistentPlaywright: false,

‎src/browser/profiles-service.test.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -178,10 +178,9 @@ describe("BrowserProfilesService", () => {
178178
driver: "existing-session",
179179
});
180180

181-
expect(result.cdpPort).toBe(18801);
181+
expect(result.cdpPort).toBe(0);
182182
expect(result.isRemote).toBe(false);
183183
expect(state.resolved.profiles["chrome-live"]).toEqual({
184-
cdpPort: 18801,
185184
driver: "existing-session",
186185
attachOnly: true,
187186
color: expect.any(String),
@@ -191,7 +190,6 @@ describe("BrowserProfilesService", () => {
191190
browser: expect.objectContaining({
192191
profiles: expect.objectContaining({
193192
"chrome-live": expect.objectContaining({
194-
cdpPort: 18801,
195193
driver: "existing-session",
196194
attachOnly: true,
197195
}),

‎src/browser/profiles-service.ts‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -141,18 +141,26 @@ export function createBrowserProfilesService(ctx: BrowserRouteContext) {
141141
if (driver === "extension") {
142142
throw new BrowserValidationError("driver=extension requires an explicit loopback cdpUrl");
143143
}
144-
const usedPorts = getUsedPorts(resolvedProfiles);
145-
const range = cdpPortRange(state.resolved);
146-
const cdpPort = allocateCdpPort(usedPorts, range);
147-
if (cdpPort === null) {
148-
throw new BrowserResourceExhaustedError("no available CDP ports in range");
144+
if (driver === "existing-session") {
145+
// existing-session uses Chrome MCP auto-connect; no CDP port needed
146+
profileConfig = {
147+
driver,
148+
attachOnly: true,
149+
color: profileColor,
150+
};
151+
} else {
152+
const usedPorts = getUsedPorts(resolvedProfiles);
153+
const range = cdpPortRange(state.resolved);
154+
const cdpPort = allocateCdpPort(usedPorts, range);
155+
if (cdpPort === null) {
156+
throw new BrowserResourceExhaustedError("no available CDP ports in range");
157+
}
158+
profileConfig = {
159+
cdpPort,
160+
...(driver ? { driver } : {}),
161+
color: profileColor,
162+
};
149163
}
150-
profileConfig = {
151-
cdpPort,
152-
...(driver ? { driver } : {}),
153-
...(driver === "existing-session" ? { attachOnly: true } : {}),
154-
color: profileColor,
155-
};
156164
}
157165

158166
const nextConfig: OpenClawConfig = {

0 commit comments

Comments
 (0)