From 4357cf4e37ddf8d887497cdc3532b565ef3c6e8b Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 13 Mar 2026 23:56:39 +0000 Subject: [PATCH] fix: harden browser existing-session flows --- docs/tools/browser.md | 4 + src/browser/chrome-mcp.test.ts | 60 ++++++++++++ src/browser/chrome-mcp.ts | 32 ++++++- src/browser/pw-tools-core.responses.ts | 19 +--- ...-core.waits-next-download-saves-it.test.ts | 2 +- src/browser/routes/agent.act.ts | 30 ++++-- .../routes/agent.existing-session.test.ts | 33 +++++++ .../routes/basic.existing-session.test.ts | 80 ++++++++++++++++ src/browser/routes/basic.ts | 92 ++++++++++--------- ....agent-contract-snapshot-endpoints.test.ts | 12 ++- src/browser/url-pattern.ts | 15 +++ 11 files changed, 302 insertions(+), 77 deletions(-) create mode 100644 src/browser/routes/basic.existing-session.test.ts create mode 100644 src/browser/url-pattern.ts diff --git a/docs/tools/browser.md b/docs/tools/browser.md index 8a7abe93209..15c0b4b0067 100644 --- a/docs/tools/browser.md +++ b/docs/tools/browser.md @@ -398,6 +398,10 @@ Notes: session only. - OpenClaw uses the official Chrome DevTools MCP `--autoConnect` flow here, not the legacy default-profile remote debugging port workflow. +- Existing-session screenshots support page captures and `--ref` element + captures from snapshots, but not CSS `--element` selectors. +- Existing-session `wait --url` supports exact, substring, and glob patterns + like other browser drivers. `wait --load networkidle` is not supported yet. - Some features still require the extension relay or managed browser path, such as PDF export and download interception. - Leave the relay loopback-only by default. If the relay must be reachable from a different network namespace (for example Gateway in WSL2, Chrome on Windows), set `browser.relayBindHost` to an explicit bind address such as `0.0.0.0` while keeping the surrounding network private and authenticated. diff --git a/src/browser/chrome-mcp.test.ts b/src/browser/chrome-mcp.test.ts index ec6f21339ed..a9571f31205 100644 --- a/src/browser/chrome-mcp.test.ts +++ b/src/browser/chrome-mcp.test.ts @@ -129,4 +129,64 @@ describe("chrome MCP page parsing", () => { expect(result).toBe(123); }); + + it("surfaces MCP tool errors instead of JSON parse noise", async () => { + const factory: ChromeMcpSessionFactory = async () => { + const session = createFakeSession(); + const callTool = vi.fn(async ({ name }: ToolCall) => { + if (name === "evaluate_script") { + return { + content: [ + { + type: "text", + text: "Cannot read properties of null (reading 'value')", + }, + ], + isError: true, + }; + } + throw new Error(`unexpected tool ${name}`); + }); + session.client.callTool = callTool as typeof session.client.callTool; + return session; + }; + setChromeMcpSessionFactoryForTest(factory); + + await expect( + evaluateChromeMcpScript({ + profileName: "chrome-live", + targetId: "1", + fn: "() => document.getElementById('missing').value", + }), + ).rejects.toThrow(/Cannot read properties of null/); + }); + + it("reuses a single pending session for concurrent requests", async () => { + let factoryCalls = 0; + let releaseFactory!: () => void; + const factoryGate = new Promise((resolve) => { + releaseFactory = resolve; + }); + + const factory: ChromeMcpSessionFactory = async () => { + factoryCalls += 1; + await factoryGate; + return createFakeSession(); + }; + setChromeMcpSessionFactoryForTest(factory); + + const tabsPromise = listChromeMcpTabs("chrome-live"); + const evalPromise = evaluateChromeMcpScript({ + profileName: "chrome-live", + targetId: "1", + fn: "() => 123", + }); + + releaseFactory(); + const [tabs, result] = await Promise.all([tabsPromise, evalPromise]); + + expect(factoryCalls).toBe(1); + expect(tabs).toHaveLength(2); + expect(result).toBe(123); + }); }); diff --git a/src/browser/chrome-mcp.ts b/src/browser/chrome-mcp.ts index ecd196547d3..e410cf886e9 100644 --- a/src/browser/chrome-mcp.ts +++ b/src/browser/chrome-mcp.ts @@ -39,6 +39,7 @@ const DEFAULT_CHROME_MCP_ARGS = [ ]; const sessions = new Map(); +const pendingSessions = new Map>(); let sessionFactory: ChromeMcpSessionFactory | null = null; function asRecord(value: unknown): Record | null { @@ -144,6 +145,11 @@ function extractMessageText(result: ChromeMcpToolResult): string { return blocks.find((block) => block.trim()) ?? ""; } +function extractToolErrorMessage(result: ChromeMcpToolResult, name: string): string { + const message = extractMessageText(result).trim(); + return message || `Chrome MCP tool "${name}" failed.`; +} + function extractJsonMessage(result: ChromeMcpToolResult): unknown { const candidates = [extractMessageText(result), ...extractTextContent(result)].filter((text) => text.trim(), @@ -207,8 +213,22 @@ async function getSession(profileName: string): Promise { session = undefined; } if (!session) { - session = await (sessionFactory ?? createRealSession)(profileName); - sessions.set(profileName, session); + let pending = pendingSessions.get(profileName); + if (!pending) { + pending = (async () => { + const created = await (sessionFactory ?? createRealSession)(profileName); + sessions.set(profileName, created); + return created; + })(); + pendingSessions.set(profileName, pending); + } + try { + session = await pending; + } finally { + if (pendingSessions.get(profileName) === pending) { + pendingSessions.delete(profileName); + } + } } try { await session.ready; @@ -229,10 +249,14 @@ async function callTool( ): Promise { const session = await getSession(profileName); try { - return (await session.client.callTool({ + const result = (await session.client.callTool({ name, arguments: args, })) as ChromeMcpToolResult; + if (result.isError) { + throw new Error(extractToolErrorMessage(result, name)); + } + return result; } catch (err) { sessions.delete(profileName); await session.client.close().catch(() => {}); @@ -268,6 +292,7 @@ export function getChromeMcpPid(profileName: string): number | null { } export async function closeChromeMcpSession(profileName: string): Promise { + pendingSessions.delete(profileName); const session = sessions.get(profileName); if (!session) { return false; @@ -508,5 +533,6 @@ export function setChromeMcpSessionFactoryForTest(factory: ChromeMcpSessionFacto export async function resetChromeMcpSessionsForTest(): Promise { sessionFactory = null; + pendingSessions.clear(); await stopAllChromeMcpSessions(); } diff --git a/src/browser/pw-tools-core.responses.ts b/src/browser/pw-tools-core.responses.ts index 5a6ddc1818c..4b153692a20 100644 --- a/src/browser/pw-tools-core.responses.ts +++ b/src/browser/pw-tools-core.responses.ts @@ -1,22 +1,7 @@ import { formatCliCommand } from "../cli/command-format.js"; import { ensurePageState, getPageForTargetId } from "./pw-session.js"; import { normalizeTimeoutMs } from "./pw-tools-core.shared.js"; - -function matchUrlPattern(pattern: string, url: string): boolean { - const p = pattern.trim(); - if (!p) { - return false; - } - if (p === url) { - return true; - } - if (p.includes("*")) { - const escaped = p.replace(/[|\\{}()[\]^$+?.]/g, "\\$&"); - const regex = new RegExp(`^${escaped.replace(/\*\*/g, ".*").replace(/\*/g, ".*")}$`); - return regex.test(url); - } - return url.includes(p); -} +import { matchBrowserUrlPattern } from "./url-pattern.js"; export async function responseBodyViaPlaywright(opts: { cdpUrl: string; @@ -65,7 +50,7 @@ export async function responseBodyViaPlaywright(opts: { } const r = resp as { url?: () => string }; const u = r.url?.() || ""; - if (!matchUrlPattern(pattern, u)) { + if (!matchBrowserUrlPattern(pattern, u)) { return; } done = true; diff --git a/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts b/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts index d976f7d7fb8..e5aa5bac2e0 100644 --- a/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts +++ b/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts @@ -291,6 +291,6 @@ describe("pw-tools-core", () => { targetId: "T1", ref: " ", }), - ).rejects.toThrow(/ref is required/i); + ).rejects.toThrow(/ref or selector is required/i); }); }); diff --git a/src/browser/routes/agent.act.ts b/src/browser/routes/agent.act.ts index a8d3a09ce83..ae18c044265 100644 --- a/src/browser/routes/agent.act.ts +++ b/src/browser/routes/agent.act.ts @@ -12,6 +12,7 @@ import { import type { BrowserActRequest, BrowserFormField } from "../client-actions-core.js"; import { normalizeBrowserFormField } from "../form-fields.js"; import type { BrowserRouteContext } from "../server-context.js"; +import { matchBrowserUrlPattern } from "../url-pattern.js"; import { registerBrowserAgentActDownloadRoutes } from "./agent.act.download.js"; import { registerBrowserAgentActHookRoutes } from "./agent.act.hooks.js"; import { @@ -47,7 +48,6 @@ function buildExistingSessionWaitPredicate(params: { text?: string; textGone?: string; selector?: string; - url?: string; loadState?: "load" | "domcontentloaded" | "networkidle"; fn?: string; }): string | null { @@ -61,9 +61,6 @@ function buildExistingSessionWaitPredicate(params: { if (params.selector) { checks.push(`Boolean(document.querySelector(${JSON.stringify(params.selector)}))`); } - if (params.url) { - checks.push(`window.location.href === ${JSON.stringify(params.url)}`); - } if (params.loadState === "domcontentloaded") { checks.push(`document.readyState === "interactive" || document.readyState === "complete"`); } else if (params.loadState === "load") { @@ -94,17 +91,30 @@ async function waitForExistingSessionCondition(params: { await sleep(params.timeMs); } const predicate = buildExistingSessionWaitPredicate(params); - if (!predicate) { + if (!predicate && !params.url) { return; } const timeoutMs = Math.max(250, params.timeoutMs ?? 10_000); const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { - const ready = await evaluateChromeMcpScript({ - profileName: params.profileName, - targetId: params.targetId, - fn: `async () => ${predicate}`, - }); + let ready = true; + if (predicate) { + ready = Boolean( + await evaluateChromeMcpScript({ + profileName: params.profileName, + targetId: params.targetId, + fn: `async () => ${predicate}`, + }), + ); + } + if (ready && params.url) { + const currentUrl = await evaluateChromeMcpScript({ + profileName: params.profileName, + targetId: params.targetId, + fn: "() => window.location.href", + }); + ready = typeof currentUrl === "string" && matchBrowserUrlPattern(params.url, currentUrl); + } if (ready) { return; } diff --git a/src/browser/routes/agent.existing-session.test.ts b/src/browser/routes/agent.existing-session.test.ts index 077889d16f9..070a0ed41d7 100644 --- a/src/browser/routes/agent.existing-session.test.ts +++ b/src/browser/routes/agent.existing-session.test.ts @@ -195,4 +195,37 @@ describe("existing-session browser routes", () => { }); expect(chromeMcpMocks.evaluateChromeMcpScript).not.toHaveBeenCalled(); }); + + it("supports glob URL waits for existing-session profiles", async () => { + chromeMcpMocks.evaluateChromeMcpScript.mockReset(); + chromeMcpMocks.evaluateChromeMcpScript.mockImplementation( + async ({ fn }: { fn: string }) => + (fn === "() => window.location.href" ? "https://example.com/" : true) as never, + ); + + const { app, postHandlers } = createApp(); + registerBrowserAgentActRoutes(app, { + state: () => ({ resolved: { evaluateEnabled: true } }), + } as never); + const handler = postHandlers.get("/act"); + expect(handler).toBeTypeOf("function"); + + const response = createResponse(); + await handler?.( + { + params: {}, + query: {}, + body: { kind: "wait", url: "**/example.com/" }, + }, + response.res, + ); + + expect(response.statusCode).toBe(200); + expect(response.body).toMatchObject({ ok: true, targetId: "7" }); + expect(chromeMcpMocks.evaluateChromeMcpScript).toHaveBeenCalledWith({ + profileName: "chrome-live", + targetId: "7", + fn: "() => window.location.href", + }); + }); }); diff --git a/src/browser/routes/basic.existing-session.test.ts b/src/browser/routes/basic.existing-session.test.ts new file mode 100644 index 00000000000..77366815ca8 --- /dev/null +++ b/src/browser/routes/basic.existing-session.test.ts @@ -0,0 +1,80 @@ +import { describe, expect, it } from "vitest"; +import { BrowserProfileUnavailableError } from "../errors.js"; +import { registerBrowserBasicRoutes } from "./basic.js"; +import type { BrowserResponse, BrowserRouteHandler, BrowserRouteRegistrar } from "./types.js"; + +function createApp() { + const getHandlers = new Map(); + const postHandlers = new Map(); + const deleteHandlers = new Map(); + const app: BrowserRouteRegistrar = { + get: (path, handler) => void getHandlers.set(path, handler), + post: (path, handler) => void postHandlers.set(path, handler), + delete: (path, handler) => void deleteHandlers.set(path, handler), + }; + return { app, getHandlers }; +} + +function createResponse() { + let statusCode = 200; + let jsonBody: unknown; + const res: BrowserResponse = { + status(code) { + statusCode = code; + return res; + }, + json(body) { + jsonBody = body; + }, + }; + return { + res, + get statusCode() { + return statusCode; + }, + get body() { + return jsonBody; + }, + }; +} + +describe("basic browser routes", () => { + it("maps existing-session status failures to JSON browser errors", async () => { + const { app, getHandlers } = createApp(); + registerBrowserBasicRoutes(app, { + state: () => ({ + resolved: { + enabled: true, + headless: false, + noSandbox: false, + executablePath: undefined, + }, + profiles: new Map(), + }), + forProfile: () => + ({ + profile: { + name: "chrome-live", + driver: "existing-session", + cdpPort: 18802, + cdpUrl: "http://127.0.0.1:18802", + color: "#00AA00", + attachOnly: true, + }, + isHttpReachable: async () => { + throw new BrowserProfileUnavailableError("attach failed"); + }, + isReachable: async () => true, + }) as never, + } as never); + + const handler = getHandlers.get("/"); + expect(handler).toBeTypeOf("function"); + + const response = createResponse(); + await handler?.({ params: {}, query: { profile: "chrome-live" } }, response.res); + + expect(response.statusCode).toBe(409); + expect(response.body).toMatchObject({ error: "attach failed" }); + }); +}); diff --git a/src/browser/routes/basic.ts b/src/browser/routes/basic.ts index 9991744107d..abb56d5af21 100644 --- a/src/browser/routes/basic.ts +++ b/src/browser/routes/basic.ts @@ -54,50 +54,58 @@ export function registerBrowserBasicRoutes(app: BrowserRouteRegistrar, ctx: Brow return jsonError(res, profileCtx.status, profileCtx.error); } - const [cdpHttp, cdpReady] = await Promise.all([ - profileCtx.isHttpReachable(300), - profileCtx.isReachable(600), - ]); - - const profileState = current.profiles.get(profileCtx.profile.name); - let detectedBrowser: string | null = null; - let detectedExecutablePath: string | null = null; - let detectError: string | null = null; - try { - const detected = resolveBrowserExecutableForPlatform(current.resolved, process.platform); - if (detected) { - detectedBrowser = detected.kind; - detectedExecutablePath = detected.path; - } - } catch (err) { - detectError = String(err); - } + const [cdpHttp, cdpReady] = await Promise.all([ + profileCtx.isHttpReachable(300), + profileCtx.isReachable(600), + ]); - res.json({ - enabled: current.resolved.enabled, - profile: profileCtx.profile.name, - driver: profileCtx.profile.driver, - running: cdpReady, - cdpReady, - cdpHttp, - pid: - profileCtx.profile.driver === "existing-session" - ? getChromeMcpPid(profileCtx.profile.name) - : (profileState?.running?.pid ?? null), - cdpPort: profileCtx.profile.cdpPort, - cdpUrl: profileCtx.profile.cdpUrl, - chosenBrowser: profileState?.running?.exe.kind ?? null, - detectedBrowser, - detectedExecutablePath, - detectError, - userDataDir: profileState?.running?.userDataDir ?? null, - color: profileCtx.profile.color, - headless: current.resolved.headless, - noSandbox: current.resolved.noSandbox, - executablePath: current.resolved.executablePath ?? null, - attachOnly: profileCtx.profile.attachOnly, - }); + const profileState = current.profiles.get(profileCtx.profile.name); + let detectedBrowser: string | null = null; + let detectedExecutablePath: string | null = null; + let detectError: string | null = null; + + try { + const detected = resolveBrowserExecutableForPlatform(current.resolved, process.platform); + if (detected) { + detectedBrowser = detected.kind; + detectedExecutablePath = detected.path; + } + } catch (err) { + detectError = String(err); + } + + res.json({ + enabled: current.resolved.enabled, + profile: profileCtx.profile.name, + driver: profileCtx.profile.driver, + running: cdpReady, + cdpReady, + cdpHttp, + pid: + profileCtx.profile.driver === "existing-session" + ? getChromeMcpPid(profileCtx.profile.name) + : (profileState?.running?.pid ?? null), + cdpPort: profileCtx.profile.cdpPort, + cdpUrl: profileCtx.profile.cdpUrl, + chosenBrowser: profileState?.running?.exe.kind ?? null, + detectedBrowser, + detectedExecutablePath, + detectError, + userDataDir: profileState?.running?.userDataDir ?? null, + color: profileCtx.profile.color, + headless: current.resolved.headless, + noSandbox: current.resolved.noSandbox, + executablePath: current.resolved.executablePath ?? null, + attachOnly: profileCtx.profile.attachOnly, + }); + } catch (err) { + const mapped = toBrowserErrorResponse(err); + if (mapped) { + return jsonError(res, mapped.status, mapped.message); + } + jsonError(res, 500, String(err)); + } }); // Start browser (profile-aware) diff --git a/src/browser/server.agent-contract-snapshot-endpoints.test.ts b/src/browser/server.agent-contract-snapshot-endpoints.test.ts index 7e300fe5aee..837a122becd 100644 --- a/src/browser/server.agent-contract-snapshot-endpoints.test.ts +++ b/src/browser/server.agent-contract-snapshot-endpoints.test.ts @@ -96,10 +96,14 @@ describe("browser control server", () => { headers: { "Content-Type": "application/json" }, body: JSON.stringify({ kind: "click", selector: "button.save" }), }); - expect(clickSelector.status).toBe(400); - expect(((await clickSelector.json()) as { error?: string }).error).toMatch( - /'selector' is not supported/i, - ); + expect(clickSelector.status).toBe(200); + expect(((await clickSelector.json()) as { ok?: boolean }).ok).toBe(true); + expect(pwMocks.clickViaPlaywright).toHaveBeenNthCalledWith(2, { + cdpUrl: state.cdpBaseUrl, + targetId: "abcd1234", + selector: "button.save", + doubleClick: false, + }); const type = await postJson<{ ok: boolean }>(`${base}/act`, { kind: "type", diff --git a/src/browser/url-pattern.ts b/src/browser/url-pattern.ts new file mode 100644 index 00000000000..2ff99657d26 --- /dev/null +++ b/src/browser/url-pattern.ts @@ -0,0 +1,15 @@ +export function matchBrowserUrlPattern(pattern: string, url: string): boolean { + const trimmedPattern = pattern.trim(); + if (!trimmedPattern) { + return false; + } + if (trimmedPattern === url) { + return true; + } + if (trimmedPattern.includes("*")) { + const escaped = trimmedPattern.replace(/[|\\{}()[\]^$+?.]/g, "\\$&"); + const regex = new RegExp(`^${escaped.replace(/\*\*/g, ".*").replace(/\*/g, ".*")}$`); + return regex.test(url); + } + return url.includes(trimmedPattern); +}