diff options
| -rw-r--r-- | .gitignore | 2 | ||||
| -rw-r--r-- | skills/pi-browser-harness/SKILL.md | 41 | ||||
| -rw-r--r-- | src/client.ts | 6 | ||||
| -rw-r--r-- | src/domains/scroll.ts | 25 | ||||
| -rw-r--r-- | src/prompt.ts | 14 | ||||
| -rw-r--r-- | src/registry.ts | 41 | ||||
| -rw-r--r-- | src/util/mutex.ts | 25 | ||||
| -rw-r--r-- | src/util/tool.ts | 58 |
8 files changed, 140 insertions, 72 deletions
@@ -5,3 +5,5 @@ benchmark/ .DS_Store *.log docs/ +scripts/ +data/ diff --git a/skills/pi-browser-harness/SKILL.md b/skills/pi-browser-harness/SKILL.md index 60cd069..2116d4c 100644 --- a/skills/pi-browser-harness/SKILL.md +++ b/skills/pi-browser-harness/SKILL.md @@ -34,34 +34,23 @@ and cross-origin content. No selectors needed. ## Parallelization -**Always parallelize when possible.** Never sequence independent operations. +**Always parallelize when possible.** The harness automatically serializes mutation tools so they never race on shared state. Emit independent tools in the same turn for better performance. ### What can run in parallel -- **Opening result pages:** Use `browser_open_urls` to open multiple URLs in parallel tabs - instead of navigating one at a time. After opening, use `browser_list_tabs` to - see the new tabs and `browser_switch_tab` to visit each one. +- **Observation tools** (`browser_screenshot`, `browser_page_info`, `browser_execute_js`, `browser_list_tabs`, `browser_current_tab`, `browser_http_get`, `browser_print_to_pdf`) can run in parallel with each other and with mutation tools. -- **API calls:** `browser_http_get` calls are independent of browser state — fire - multiple GETs in parallel to fetch APIs, static pages, or search results. +- **Opening result pages:** Use `browser_open_urls` to open multiple URLs in parallel tabs. After opening, use `browser_list_tabs` to see the new tabs and `browser_switch_tab` to visit each one. -- **JS extraction across tabs:** Once tabs are loaded, extract data from each tab - with `browser_execute_js` — these can run in parallel across different tabs. +- **API calls:** `browser_http_get` calls are independent of browser state — fire multiple GETs in parallel. -- **Screenshots across tabs:** After opening multiple pages, capture screenshots - of each tab in parallel. Each `browser_screenshot` targets the currently active - tab, so switch tabs first, then screenshot. +- **Read + browser tools:** Use `read` and `browser_*` tools in the same turn — reading files or previous results is independent of browser actions. -- **Read + browser tools:** Use `read` and `browser_*` tools in the same turn — - reading files or previous results is independent of browser actions. +### What must be sequential (harness enforces this) -### What must be sequential +- **Mutation tools** (`click`, `type`, `scroll`, `navigate`, `switch_tab`, `press_key`, `drag_and_drop`, `upload_file`, `go_back`, `go_forward`, `reload`, `handle_dialog`, `viewport_resize`, `wait_for_load`, `download`) are automatically queued through a mutex. Even if you emit them in parallel, they execute serially in FIFO order. You do not need to worry about race conditions. -- **Page interactions:** Clicks, typing, scrolling, and form submissions on the - same page depend on prior state and must be sequential. - -- **Tab switching then acting:** `browser_switch_tab` must complete before acting - on the switched-to tab. +- **Tab switching then acting:** `browser_switch_tab` must complete before acting on the switched-to tab. This is enforced automatically. ### Pattern: Parallel research across tabs @@ -77,13 +66,14 @@ browser_switch_tab(targetId1) → browser_screenshot() # These are sequential browser_switch_tab(targetId2) → browser_execute_js(...) # but you batch switch+act ``` -### Pattern: Parallel API + browser +### Pattern: Parallel observation + mutation ``` -# Fire API calls while interacting with a page -browser_http_get("https://api.example.com/data") # parallel -browser_click(x, y) # parallel (different target) -browser_type("search term") # parallel (different target) +# Fire observation tools while a mutation is in progress +browser_click(x, y) # mutation (queued) +browser_screenshot() # observation (runs immediately in parallel) +browser_page_info() # observation (runs immediately in parallel) +browser_http_get("https://api.example.com") # external (runs immediately in parallel) ``` ## Available Tools @@ -140,8 +130,9 @@ browser_http_get("https://api.example.com/data") ### Scrolling ``` -browser_screenshot() → browser_scroll({ deltaY: -500 }) → browser_screenshot() +browser_screenshot() → browser_scroll({ deltaY: 500 }) → browser_screenshot() ``` +Note: deltaY follows W3C wheel-event convention: positive=down, negative=up. Default deltaY=300 scrolls down. ### Research (search engine + browser_open_urls) ``` diff --git a/src/client.ts b/src/client.ts index 1bf7ccc..94b6ac0 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,5 +1,6 @@ import { type Result, err, ok } from "./util/result"; import { safeJs } from "./util/js-template"; +import { type Mutex, createMutex } from "./util/mutex"; import { discoverWsUrl } from "./cdp/discovery"; import { type CdpError, cdpError } from "./cdp/errors"; import type { CdpTransport } from "./cdp/transport"; @@ -27,6 +28,9 @@ export type BrowserClient = { current(): { readonly sessionId: string; readonly targetId: string } | null; session(): CdpSession; transport(): CdpTransport; + /** Returns the shared async mutex that serialized browser tools must acquire + * before performing mutations. Observation tools should not use this. */ + mutationMutex(): Mutex; }; const HEALTH_TTL_MS = 30_000; @@ -63,6 +67,7 @@ const parsePageInfoPayload = (v: unknown): Result<PageInfo, CdpError> => { export const createBrowserClient = (opts: BrowserClientOptions): BrowserClient => { const transport = createCdpTransport(); const session = createCdpSession(transport); + const mutationMutex = createMutex(); let lastHealth = 0; let pageCache: { readonly info: PageInfo; readonly at: number } | null = null; let remote: BrowserClientOptions["remote"] | null = opts.remote ?? null; @@ -228,5 +233,6 @@ export const createBrowserClient = (opts: BrowserClientOptions): BrowserClient = current: () => session.current(), session: () => session, transport: () => transport, + mutationMutex: () => mutationMutex, }; }; diff --git a/src/domains/scroll.ts b/src/domains/scroll.ts index db80af7..2bd2ed1 100644 --- a/src/domains/scroll.ts +++ b/src/domains/scroll.ts @@ -7,8 +7,8 @@ const ScrollArgs = Type.Object({ y: Type.Optional(Type.Number({ description: "Y coordinate where to scroll. Default: viewport center" })), deltaX: Type.Optional(Type.Number({ default: 0, description: "Horizontal scroll delta (CSS pixels). Positive = right." })), deltaY: Type.Optional(Type.Number({ - default: -300, - description: "Vertical scroll delta (CSS pixels). Positive = up (matches W3C wheel events). Default: -300 (scroll down).", + default: 300, + description: "Vertical scroll delta (CSS pixels). Positive = down, negative = up (W3C wheel-event convention). Default: 300 (scroll down).", })), }); @@ -16,10 +16,10 @@ export const scrollTool = defineBrowserTool({ name: "browser_scroll", label: "Browser Scroll", description: - "Scroll the page at given coordinates. deltaY follows W3C wheel-event convention: positive = scroll up, negative = scroll down. Default scrolls down (deltaY = -300).", - promptSnippet: "Scroll the page (deltaY positive=up, negative=down)", + "Scroll the page at given coordinates. deltaY follows W3C wheel-event convention: positive = scroll down, negative = scroll up. Default scrolls down (deltaY = 300).", + promptSnippet: "Scroll the page (deltaY positive=down, negative=up)", promptGuidelines: [ - "Default behavior scrolls down 300px (deltaY=-300). Pass a positive deltaY to scroll up.", + "Default behavior scrolls down 300px (deltaY=300). Pass a negative deltaY to scroll up.", "Pass x/y to target a specific scrollable region (e.g., a div with overflow); otherwise scrolls the page at viewport center.", ], parameters: ScrollArgs, @@ -35,14 +35,21 @@ export const scrollTool = defineBrowserTool({ const cx = args.x ?? Math.round(info.data.width / 2); const cy = args.y ?? Math.round(info.data.height / 2); const dx = args.deltaX ?? 0; - const dy = args.deltaY ?? -300; + const dy = args.deltaY ?? 300; + // The page target must be focused or Input events are dropped silently + // (mouseWheel hangs; synthesizeScrollGesture returns ok but no scroll). + // Page.bringToFront makes the target the active one in the browser. + const front = await client.session().call("Page.bringToFront", {}); + if (!front.success) return err({ kind: "cdp_error", message: front.error.message }); // Establish compositor mouse position before mouseWheel; without this, - // CDP sometimes drops the wheel event with a timeout. Fix preserved - // from v0.2.0. - const moved = await client.session().call("Input.dispatchMouseEvent", { type: "mouseMoved", x: cx, y: cy }); + // CDP sometimes drops the wheel event with a timeout. + const moved = await client.session().call("Input.dispatchMouseEvent", { + type: "mouseMoved", x: cx, y: cy, button: "none", buttons: 0, + }); if (!moved.success) return err({ kind: "cdp_error", message: moved.error.message }); const wheel = await client.session().call("Input.dispatchMouseEvent", { type: "mouseWheel", x: cx, y: cy, deltaX: dx, deltaY: dy, + button: "none", buttons: 0, pointerType: "mouse", }); if (!wheel.success) return err({ kind: "cdp_error", message: wheel.error.message }); return ok({ diff --git a/src/prompt.ts b/src/prompt.ts index 6d2b2ab..1a2a4c1 100644 --- a/src/prompt.ts +++ b/src/prompt.ts @@ -63,6 +63,16 @@ browser_run_script("/tmp/scrape-pages.js", { urls: [...] }) Script bindings: params, daemon, require, signal, onUpdate, ctx, console, fetch, JSON, Buffer, setTimeout, clearTimeout. +### Parallel Execution + +Observation tools (browser_screenshot, browser_page_info, browser_execute_js, browser_list_tabs, browser_http_get, etc.) can run in parallel with each other and with mutation tools. The harness automatically serializes mutation tools (click, type, scroll, navigate, switch_tab, etc.) so they never race on shared state. When operations are independent, emit them in the same turn for better performance. + +**Examples of safe parallel calls:** +\`\`\` +browser_screenshot() + browser_page_info() + browser_execute_js("document.title") +browser_http_get("https://api.example.com/data") + browser_click(x, y) +\`\`\` + ### Common Patterns **Navigation:** @@ -85,9 +95,9 @@ browser_http_get("https://api.example.com/data") **Scrolling:** \`\`\` -browser_screenshot() → browser_scroll({ deltaY: -500 }) → browser_screenshot() +browser_screenshot() → browser_scroll({ deltaY: 500 }) → browser_screenshot() \`\`\` -Note: deltaY follows W3C wheel-event convention: positive=up, negative=down. Default deltaY=-300 scrolls down. +Note: deltaY follows W3C wheel-event convention: positive=down, negative=up. Default deltaY=300 scrolls down. **Research Workflow (search + browser):** \`\`\` diff --git a/src/registry.ts b/src/registry.ts index 5f9d7da..d9b7cea 100644 --- a/src/registry.ts +++ b/src/registry.ts @@ -16,31 +16,34 @@ import { dragAndDropTool } from "./domains/drag"; import { httpGetTool, getNetworkLogTool } from "./domains/network"; import { executeJsTool, runScriptTool } from "./domains/js"; +// Mutation tools that modify page / browser state are marked `serialized: true` +// so they run through a shared async mutex. Observation / read-only tools are +// left unmarked and can execute in parallel. const TOOLS: ReadonlyArray<AnyBrowserToolDefinition> = [ - clickTool, - typeTool, - pressKeyTool, - dispatchKeyTool, - scrollTool, + { ...clickTool, serialized: true }, + { ...typeTool, serialized: true }, + { ...pressKeyTool, serialized: true }, + { ...dispatchKeyTool, serialized: true }, + { ...scrollTool, serialized: true }, pageInfoTool, waitTool, - waitForLoadTool, - handleDialogTool, + { ...waitForLoadTool, serialized: true }, + { ...handleDialogTool, serialized: true }, screenshotTool, - navigateTool, - openUrlsTool, - goBackTool, - goForwardTool, - reloadTool, + { ...navigateTool, serialized: true }, + { ...openUrlsTool, serialized: true }, + { ...goBackTool, serialized: true }, + { ...goForwardTool, serialized: true }, + { ...reloadTool, serialized: true }, listTabsTool, currentTabTool, - switchTabTool, - newTabTool, - uploadFileTool, - downloadTool, - printToPdfTool, - viewportResizeTool, - dragAndDropTool, + { ...switchTabTool, serialized: true }, + { ...newTabTool, serialized: true }, + { ...uploadFileTool, serialized: true }, + { ...downloadTool, serialized: true }, + { ...printToPdfTool, serialized: true }, + { ...viewportResizeTool, serialized: true }, + { ...dragAndDropTool, serialized: true }, httpGetTool, getNetworkLogTool, executeJsTool, diff --git a/src/util/mutex.ts b/src/util/mutex.ts new file mode 100644 index 0000000..08efe76 --- /dev/null +++ b/src/util/mutex.ts @@ -0,0 +1,25 @@ +/** + * Lightweight async mutex (~10 lines). Acquire returns a release function. + * When the mutex is free, acquire() resolves immediately. When held, + * callers are queued and execute serially in FIFO order. + */ +export type Mutex = { + acquire(): Promise<() => void>; +}; + +export const createMutex = (): Mutex => { + let queue = Promise.resolve<unknown>(undefined); + + return { + async acquire(): Promise<() => void> { + let release = () => {}; + const next = new Promise<void>((resolve) => { + release = resolve; + }); + const prev = queue; + queue = prev.then(() => next); + await prev; + return release; + }, + }; +}; diff --git a/src/util/tool.ts b/src/util/tool.ts index d269ba2..c2bb3f8 100644 --- a/src/util/tool.ts +++ b/src/util/tool.ts @@ -53,6 +53,10 @@ export type BrowserToolDefinition<S extends TSchema> = { readonly handler: ToolHandler<S>; readonly renderCall?: (args: Static<S>, theme: Theme) => Component; readonly renderResult?: (result: AgentToolResult<unknown>, expanded: boolean, theme: Theme) => Component; + /** If true, the tool acquires the client's mutation mutex before executing. + * This ensures that two serialized tools never run concurrently, + * preventing race conditions on shared CDP session/page state. */ + readonly serialized?: boolean; readonly ensureAlive?: boolean; }; @@ -146,23 +150,43 @@ export const registerBrowserTool = ( ); } } - const result = await def.handler(args, { - client, - signal, - onUpdate: (u) => { - if (onUpdate) { - // AgentToolUpdateCallback expects AgentToolResult<TDetails>; we use unknown for TDetails. - const update: AgentToolResult<OkDetails> = { - content: [{ type: "text", text: u.text }], - details: { ok: true, ...(u.details ?? {}) }, - }; - // Cast needed: onUpdate is AgentToolUpdateCallback<unknown> (generic TDetails). - (onUpdate as AgentToolUpdateCallback<OkDetails>)(update); - } - }, - extensionCtx, - }); - return toToolResult(result, def.name); + // Serialized tools acquire the client's mutation mutex so that only one + // mutation tool runs at a time. Observation tools skip this and run freely + // in parallel. AbortSignal is checked after acquiring the lock so that a + // cancelled task doesn't hold the mutex forever. + let release: (() => void) | undefined; + if (def.serialized) { + const acquire = await client.mutationMutex().acquire(); + release = acquire; + if (signal?.aborted) { + release(); + return toToolResult( + { success: false, error: { kind: "invalid_state", message: "Tool execution aborted before entering serialized lane" } }, + def.name, + ); + } + } + try { + const result = await def.handler(args, { + client, + signal, + onUpdate: (u) => { + if (onUpdate) { + // AgentToolUpdateCallback expects AgentToolResult<TDetails>; we use unknown for TDetails. + const update: AgentToolResult<OkDetails> = { + content: [{ type: "text", text: u.text }], + details: { ok: true, ...(u.details ?? {}) }, + }; + // Cast needed: onUpdate is AgentToolUpdateCallback<unknown> (generic TDetails). + (onUpdate as AgentToolUpdateCallback<OkDetails>)(update); + } + }, + extensionCtx, + }); + return toToolResult(result, def.name); + } finally { + release?.(); + } }, }; pi.registerTool(td); |