diff --git a/docs/tools/browser.md b/docs/tools/browser.md index bafc4a224..402ef3ca7 100644 --- a/docs/tools/browser.md +++ b/docs/tools/browser.md @@ -133,22 +133,24 @@ The tool returns one result per call; no streaming partial output is emitted fro - `tab.press(key, { selector? })` - `tab.scroll(deltaX, deltaY)` - `tab.drag(from, to)` - - `tab.waitFor(selector)` + - `tab.waitFor(selector, { timeout? })` - `tab.evaluate(fn, ...args)` - `tab.scrollIntoView(selector)` - `tab.select(selector, ...values)` - `tab.uploadFile(selector, ...filePaths)` - `tab.waitForUrl(pattern, { timeout? })` - `tab.waitForResponse(pattern, { timeout? })` + - `tab.waitForSelector(selector, { timeout?, visible?, hidden? })` + - `tab.waitForNavigation({ waitUntil?, timeout? })` - `tab.id(n)` - `tab.ref(id)` -14. Selector handling in `normalizeSelector()` accepts plain CSS and Puppeteer query handlers, and rewrites legacy Playwright-style prefixes `p-text/`, `p-xpath/`, `p-pierce/`, `p-aria/`; other `p-*` prefixes throw a `ToolError`. +14. Selector handling in `normalizeSelector()` accepts plain CSS and Puppeteer query handlers, and rewrites legacy Playwright-style prefixes `p-text/`, `p-xpath/`, `p-pierce/`, `p-aria/`; other `p-*` prefixes throw a `ToolError`. Playwright-only engines/pseudos (`:has-text()`, `:text()`, `:visible`, `:nth-match()`, `:near()`/`:above()`/…) on a CSS selector throw a `ToolError` pointing at the `text/`/`aria/` equivalents instead of stalling the action timeout. 15. `tab.observe()` clears the element cache, takes a Puppeteer accessibility snapshot, filters to interactive nodes unless `includeAll`, optionally filters to viewport-visible nodes, assigns numeric ids, caches `ElementHandle`s, and returns URL/title/viewport/scroll metadata plus `elements`. 15a. `tab.ariaSnapshot()` resolves the optional `selector` (via `normalizeSelector()` → `page.$`, defaulting to the whole document) and runs the generated Playwright ARIA-snapshot bundle (`src/tools/browser/aria/aria-snapshot.bundle.txt`) via `captureAriaSnapshot()`. The bundle is wrapped in a `new Function` built worker-side (so page CSP never applies) and serialized to a CDP `page.evaluate` in the page's **main world**, returning Playwright-format YAML. It always runs in `ai` mode: every node gets a `[ref=eN]` id, clickables get `[cursor=pointer]`, and matched DOM nodes are tagged with an `_ariaRef` expando. Existing `_ariaRef` expandos are cleared before each snapshot so ids renumber deterministically from e1 (the fresh module's counter resets each call); refs stay valid until the next snapshot. The cmux backend uses `buildAriaSnapshotScript()` over `browser.eval` instead (no `ElementHandle`; CSS selectors only for the root). 16. `tab.id(n)` resolves the cached `ElementHandle`, verifies `el.isConnected`, and throws a stale-id error after cache invalidation if the DOM changed or the cache was cleared. 16a. `tab.ref(id)` resolves a `[ref=eN]` id from the latest `ariaSnapshot()` to a live `ElementHandle` via `resolveAriaRefHandle()` (`page.evaluateHandle` in the main world, walking the document + shadow roots for the matching `_ariaRef`), throwing if no element matches; it accepts a bare `eN` or a prefixed form. For inline selector use, `parseAriaRefSelector()` recognizes only the explicit `aria-ref=eN` / `aria-ref/eN` / `ariaref/eN` forms inside `tab.click/type/fill/waitFor/scrollIntoView` — a bare `eN` is intentionally rejected there so it does not collide with cmux's native observe ids. The cmux backend resolves the same explicit forms through its `aria-ref` `SelectorSpec` kind in `findElement`. 17. `tab.goto()` clears the cached element ids before navigating. Any new `tab.observe()` also clears and rebuilds the cache. -18. `tab.click()` uses a custom retry loop for `text/...` selectors to find an actionable visible match; other selectors use `page.locator(...).click()` with the run timeout. +18. `tab.click()` uses a custom retry loop for `text/...` selectors to find an actionable visible match; other selectors use `page.locator(...).click()`. Interactive actions (`click`/`fill`/`type`/`press`/`scroll`/`drag`/`scrollIntoView`/`select`/`uploadFile`) and the `waitFor*` helpers run under a per-op deadline (`min(cellBudget − slack, ceiling)`) threaded into both the puppeteer `signal` and `.setTimeout()`, so a stalled helper aborts the CDP action and rejects with a named `tab. timed out after ms` that leaves cell budget — never the opaque whole-cell timeout. `goto`/`evaluate` stay uncapped. 19. `tab.screenshot()` captures either the whole page or a selector PNG, downsizes a copy for model output, chooses a persistence path, writes the image to disk, records metadata, and optionally emits text + image display entries. 20. `display()` calls accumulate in an array. After code finishes, the worker posts `{ displays, returnValue, screenshots }`; `BrowserTool.#run()` appends the return value as trailing text content when not `undefined`. 21. `close` releases one tab or all tabs via `releaseTab()` / `releaseAllTabs()`. Each tab aborts pending runs, asks the worker to close, waits up to `750` ms for a `closed` ack, terminates the worker, decrements browser refcount, and disposes the browser handle when refcount reaches zero. @@ -216,6 +218,7 @@ The tool returns one result per call; no streaming partial output is emitted fro - Screenshot model-attachment resize cap: `maxWidth 1024`, `maxHeight 1024`, `maxBytes 150 * 1024`, `jpegQuality 70` (`packages/coding-agent/src/tools/browser/tab-worker.ts`). - `tab.waitForUrl()` polling interval: `200` ms (`packages/coding-agent/src/tools/browser/tab-worker.ts`). - Drag simulation uses `12` mouse-move steps (`packages/coding-agent/src/tools/browser/tab-worker.ts`). +- Per-op fail-fast ceilings (`packages/coding-agent/src/tools/browser/tab-worker.ts`): quick page reads (`observe`/`screenshot`/`extract`/`ariaSnapshot`) `min(cellBudget − 1s, 20s)`; interactive actions + default waits `min(cellBudget − 1s, 15s)`; an explicit `{ timeout }` on a `waitFor*` is clamped to `cellBudget − 1s` (`0`/`Infinity` → that bound). See `resolveOpTimeouts()` / `resolveWaitTimeout()`. ## Errors - `BrowserTool.execute()` converts DOM-style `AbortError` into `ToolAbortError`; other errors propagate. @@ -231,7 +234,7 @@ The tool returns one result per call; no streaming partial output is emitted fro - Spawn/attach failures are wrapped into `ToolError`s such as `Timed out waiting for CDP endpoint ...`, `Failed to attach to ...`, or `Connected to ... but puppeteer.connect failed: ...`. - `app.cdp_url` must be the HTTP CDP discovery endpoint, not a `ws://` URL; otherwise `normalizeConnectedCdpUrl()` throws `browser app.cdp_url must be the HTTP CDP discovery endpoint ...`. - `tab` helper errors are user-visible `ToolError`s, including unsupported selector prefix, stale/unknown element id, invalid drag target, missing upload files, non-`` option(s); returns selection. `tab.fill` NEVER works for selects. - `tab.uploadFile(selector, …filePaths)` — attach files to ``; paths relative to cwd. - `tab.waitForUrl(pattern, { timeout? })` — substring or `RegExp` (matches SPA pushState nav); returns matched URL. - `tab.waitForResponse(pattern, { timeout? })` — substring, `RegExp`, or `(response) => boolean`; returns puppeteer `HTTPResponse` (`.text()`/`.json()`/`.status()`/`.headers()`). + - `tab.waitForNavigation({ waitUntil?, timeout? })` — resolves on the next navigation. Start it BEFORE the click/submit that triggers it; after `tab.goto` (which already waits) use `tab.waitForUrl`/`tab.waitForSelector` instead. - `tab.evaluate(fn, …args)` — `page.evaluate` for ad-hoc DOM reads. - `tab.screenshot({ selector?, fullPage?, save?, silent? })` — capture + attach for viewing (`silent: true` skips). Pass `save` only when a later step needs the file. - `tab.extract(format = "markdown")` — readable page content (`"markdown"` | `"text"`); throws when nothing readable. -- Selectors: CSS + puppeteer handlers `aria/Sign in`, `text/Continue`, `xpath/…`, `pierce/…`; also Playwright-style `p-aria/…`, `p-text/…`. +- Selectors: CSS + puppeteer handlers `aria/Sign in`, `text/Continue`, `xpath/…`, `pierce/…`; also Playwright-style `p-aria/…`, `p-text/…`. Playwright-only engines/pseudos (`:has-text()`, `:visible`, …) are rejected — use `text/…` or `aria/…`. A stalled action/wait fails fast with a named `tab. timed out` error, never the whole-cell timeout. diff --git a/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts b/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts index 4a3a9cbf5..6e7be2faf 100644 --- a/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts +++ b/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts @@ -434,6 +434,12 @@ export class CmuxTab { return new CmuxElementHandle(this, selector); } + async waitForSelector(selector: string, opts?: { timeout?: number }): Promise { + const timeoutMs = opts?.timeout ?? this.#runContext?.timeoutMs ?? 30_000; + await this.#waitForSelector(selector, timeoutMs); + return new CmuxElementHandle(this, selector); + } + async evaluate( fn: string | ((...args: TArgs) => TResult | Promise), ...args: TArgs @@ -549,6 +555,37 @@ export class CmuxTab { throw new ToolError(`tab.waitForUrl() timed out after ${timeoutMs}ms`); } + async waitForNavigation(opts?: { waitUntil?: WaitUntil; timeout?: number }): Promise { + const timeoutMs = opts?.timeout ?? this.#runContext?.timeoutMs ?? 30_000; + // Cmux has no native "next navigation" wait — snapshot the current URL via a fresh + // `browser.url.get` (never the possibly-stale `#lastUrl`), then poll for a change + // from it (mirroring headless `page.waitForNavigation` intent) and optionally settle + // on the requested load state. Start it BEFORE the click/submit that navigates; after + // a completed nav it times out like puppeteer does. + const baseline = (await this.#request("browser.url.get", {}, Math.min(timeoutMs, 5_000))) as CmuxUrlGetResult; + const startUrl = typeof baseline.url === "string" && baseline.url.length > 0 ? baseline.url : this.#lastUrl; + if (typeof baseline.url === "string" && baseline.url.length > 0) this.#lastUrl = baseline.url; + const deadline = Date.now() + timeoutMs; + while (Date.now() <= deadline) { + const result = (await this.#request("browser.url.get", {}, Math.min(timeoutMs, 5_000))) as CmuxUrlGetResult; + if (typeof result.url === "string" && result.url.length > 0) { + this.#lastUrl = result.url; + if (result.url !== startUrl) { + if (opts?.waitUntil) { + await this.#request( + "browser.wait", + { load_state: mapWaitUntil(opts.waitUntil), timeout_ms: timeoutMs }, + timeoutMs, + ); + } + return null; + } + } + await Bun.sleep(200); + } + throw new ToolError(`tab.waitForNavigation() timed out after ${timeoutMs}ms`); + } + async drag(from: DragTarget, to: DragTarget): Promise { const start = await this.#dragPoint(from); const end = await this.#dragPoint(to); diff --git a/packages/coding-agent/src/tools/browser/tab-worker.ts b/packages/coding-agent/src/tools/browser/tab-worker.ts index 691d5748b..3cfbe1b63 100644 --- a/packages/coding-agent/src/tools/browser/tab-worker.ts +++ b/packages/coding-agent/src/tools/browser/tab-worker.ts @@ -82,17 +82,85 @@ const INTERACTIVE_AX_ROLES = new Set([ const LEGACY_SELECTOR_PREFIXES = ["p-aria/", "p-text/", "p-xpath/", "p-pierce/"] as const; +const SELECTOR_HANDLER_PREFIXES = [ + "aria/", + "text/", + "xpath/", + "pierce/", + "aria-ref=", + "aria-ref/", + "ariaref/", + "p-", +] as const; + +/** + * Playwright-only selector engines/pseudos puppeteer cannot parse. Without this guard a + * `tab.click(":has-text(...)")` would wait the full action timeout and fail opaquely; + * fail fast instead with a pointer to the puppeteer-native alternative. Skipped for + * explicit query-handler prefixes (`text/`, `aria/`, …) whose payload is literal text. + */ +const PLAYWRIGHT_ONLY_SELECTOR_RE = + /:has-text\(|:text\(|:text-is\(|:text-matches\(|:visible\b|:hidden\b|:nth-match\(|:near\(|:above\(|:below\(|:right-of\(|:left-of\(/; + type DialogPolicy = "accept" | "dismiss"; type DragTarget = string | { readonly x: number; readonly y: number }; type ActionabilityResult = { ok: true; x: number; y: number } | { ok: false; reason: string }; /** - * Per-op ceiling for puppeteer-internal helpers that should resolve quickly - * (`observe`, `screenshot`, `extract`). Kept below the default 30s cell budget so a - * single stalled helper fails fast with a named error and leaves budget for the rest - * of the cell. Effective cap is `min(cellBudget, QUICK_OP_TIMEOUT_MS)`. + * Per-op fail-fast ceilings for `tab.*` helpers. All are kept strictly under the cell + * budget (`timeoutMs - OP_DEADLINE_SLACK_MS`) so a stalled helper rejects with a named, + * attributable error that leaves recovery budget — never the opaque whole-cell + * "Browser code execution timed out" path that consumed the entire run. + * + * - `QUICK_OP_TIMEOUT_MS`: page-coupled reads that should resolve fast (`observe`, + * `screenshot`, `extract`, `ariaSnapshot`). + * - `ACTION_OP_TIMEOUT_MS`: interactive point actions (`click`, `fill`, `type`, …) and + * the default for wait helpers when no explicit `{ timeout }` is given. + * + * `goto` and `evaluate` stay uncapped (`Number.POSITIVE_INFINITY`): navigation and user + * code legitimately use the full cell budget. */ const QUICK_OP_TIMEOUT_MS = 20_000; +const ACTION_OP_TIMEOUT_MS = 15_000; +/** Headroom subtracted from the cell budget so a per-op deadline fires before it. */ +const OP_DEADLINE_SLACK_MS = 1_000; + +export interface OpTimeouts { + /** Largest per-op deadline allowed — strictly below the cell budget. */ + budgetBound: number; + /** Ceiling for quick page reads. */ + quickOpMs: number; + /** Ceiling for interactive actions + default for waits. */ + actionOpMs: number; +} + +/** Resolve the per-op fail-fast ceilings for a given cell budget. */ +export function resolveOpTimeouts(cellTimeoutMs: number): OpTimeouts { + const budgetBound = Math.max(1, cellTimeoutMs - OP_DEADLINE_SLACK_MS); + return { + budgetBound, + quickOpMs: Math.min(budgetBound, QUICK_OP_TIMEOUT_MS), + actionOpMs: Math.min(budgetBound, ACTION_OP_TIMEOUT_MS), + }; +} + +/** + * Effective timeout for a wait helper (`waitFor*`). A positive explicit `{ timeout }` is + * honored but clamped to the cell budget so it still fails fast + named; raising the tool + * `timeout` raises that cap, so a longer budget stays meaningful. No `{ timeout }` → the + * action ceiling. Puppeteer's `{ timeout: 0 }` / `Infinity` ("disable") maps to the largest + * bounded wait (`budgetBound`) — the harness never permits an unbounded wait. Garbage input + * (negative, `NaN`) falls back to the action ceiling rather than the longest wait. + */ +export function resolveWaitTimeout(cellTimeoutMs: number, explicit?: number): number { + const { budgetBound, actionOpMs } = resolveOpTimeouts(cellTimeoutMs); + if (explicit === undefined) return actionOpMs; + // Puppeteer "disable" sentinels — still bounded by the budget here. + if (explicit === 0 || explicit === Number.POSITIVE_INFINITY) return budgetBound; + // Positive finite → honored + clamped. Negative/NaN garbage → default, not the longest wait. + if (Number.isFinite(explicit) && explicit > 0) return Math.min(explicit, budgetBound); + return actionOpMs; +} interface ScreenshotOptions { selector?: string; @@ -121,7 +189,7 @@ interface TabApi { press(key: KeyInput, opts?: { selector?: string }): Promise; scroll(deltaX: number, deltaY: number): Promise; drag(from: DragTarget, to: DragTarget): Promise; - waitFor(selector: string): Promise; + waitFor(selector: string, opts?: { timeout?: number }): Promise; evaluate( fn: string | ((...args: TArgs) => TResult | Promise), ...args: TArgs @@ -134,12 +202,29 @@ interface TabApi { pattern: string | RegExp | ((response: HTTPResponse) => boolean | Promise), opts?: { timeout?: number }, ): Promise; + waitForSelector( + selector: string, + opts?: { timeout?: number; visible?: boolean; hidden?: boolean }, + ): Promise; + waitForNavigation(opts?: { + waitUntil?: "load" | "domcontentloaded" | "networkidle0" | "networkidle2"; + timeout?: number; + }): Promise; id(n: number): Promise; ref(id: string): Promise; } -function normalizeSelector(selector: string): string { +export function normalizeSelector(selector: string): string { if (!selector) return selector; + if ( + !SELECTOR_HANDLER_PREFIXES.some(prefix => selector.startsWith(prefix)) && + PLAYWRIGHT_ONLY_SELECTOR_RE.test(selector) + ) { + throw new ToolError( + `Playwright-only selector ${JSON.stringify(selector)} is not supported by the browser tool. ` + + `Use a puppeteer text selector ("text/Allow all"), an aria selector ("aria/Name"), CSS, or "xpath/...".`, + ); + } if (selector.startsWith("p-") && !LEGACY_SELECTOR_PREFIXES.some(prefix => selector.startsWith(prefix))) { throw new ToolError( `Unsupported selector prefix. Use CSS or puppeteer query handlers (aria/, text/, xpath/, pierce/). Got: ${selector}`, @@ -761,8 +846,15 @@ export class WorkerCore { try { return await fn(opSignal); } catch (err) { - // Per-op deadline fired (not the cell budget, not an explicit abort) → named, actionable error. - if (opTimeout?.aborted && !cellSignal.aborted) { + // Fail fast with a named, attributable error instead of the opaque whole-cell timeout: + // our per-op deadline fired, or puppeteer's own (equal) timeout fired first — having + // already torn down the CDP action via the op signal, so no work is left dangling. + // Cell-budget aborts and uncapped helpers (goto/evaluate) keep their native errors. + if ( + capped && + !cellSignal.aborted && + (opTimeout?.aborted || (err instanceof Error && err.name === "TimeoutError")) + ) { throw new ToolError(`${label} timed out after ${perOpTimeoutMs}ms`); } throw err; @@ -781,7 +873,8 @@ export class WorkerCore { active: ActiveRun, ): TabApi { const page = this.#requirePage(); - const quickOpMs = Math.min(timeoutMs, QUICK_OP_TIMEOUT_MS); + const { quickOpMs, actionOpMs } = resolveOpTimeouts(timeoutMs); + const waitMs = (explicit?: number): number => resolveWaitTimeout(timeoutMs, explicit); const INF = Number.POSITIVE_INFINITY; const op = (label: string, perOpMs: number, fn: (sig: AbortSignal) => Promise): Promise => this.#runOp(active, label, signal, perOpMs, fn); @@ -844,7 +937,7 @@ export class WorkerCore { return content; }), click: selector => - op(`tab.click(${JSON.stringify(selector)})`, INF, async sig => { + op(`tab.click(${JSON.stringify(selector)})`, actionOpMs, async sig => { if (parseAriaRefSelector(selector) !== null) { const handle = await this.#resolveAriaRef(selector); try { @@ -855,12 +948,12 @@ export class WorkerCore { return; } const resolved = normalizeSelector(selector); - if (resolved.startsWith("text/")) await clickQueryHandlerText(page, resolved, timeoutMs, sig); - else await untilAborted(sig, () => page.locator(resolved).setTimeout(timeoutMs).click()); + if (resolved.startsWith("text/")) await clickQueryHandlerText(page, resolved, actionOpMs, sig); + else await untilAborted(sig, () => page.locator(resolved).setTimeout(actionOpMs).click({ signal: sig })); }), type: (selector, text) => - op(`tab.type(${JSON.stringify(selector)})`, INF, async sig => { - const handle = await this.#resolveActionHandle(selector, timeoutMs, sig); + op(`tab.type(${JSON.stringify(selector)})`, actionOpMs, async sig => { + const handle = await this.#resolveActionHandle(selector, actionOpMs, sig); try { await untilAborted(sig, () => handle.type(text, { delay: 0 })); } finally { @@ -868,7 +961,7 @@ export class WorkerCore { } }), fill: (selector, value) => - op(`tab.fill(${JSON.stringify(selector)})`, INF, async sig => { + op(`tab.fill(${JSON.stringify(selector)})`, actionOpMs, async sig => { if (parseAriaRefSelector(selector) !== null) { const handle = await this.#resolveAriaRef(selector); try { @@ -886,22 +979,46 @@ export class WorkerCore { return; } await untilAborted(sig, () => - page.locator(normalizeSelector(selector)).setTimeout(timeoutMs).fill(value), + page.locator(normalizeSelector(selector)).setTimeout(actionOpMs).fill(value, { signal: sig }), ); }), press: (key, opts) => - op(`tab.press(${JSON.stringify(key)})`, INF, async sig => { + op(`tab.press(${JSON.stringify(key)})`, actionOpMs, async sig => { const selector = opts?.selector; if (selector) await untilAborted(sig, () => page.focus(normalizeSelector(selector))); await untilAborted(sig, () => page.keyboard.press(key)); }), scroll: (deltaX, deltaY) => - op("tab.scroll()", INF, sig => untilAborted(sig, () => page.mouse.wheel({ deltaX, deltaY }))), - drag: (from, to) => op("tab.drag()", INF, sig => this.#drag(from, to, sig)), - waitFor: selector => - op(`tab.waitFor(${JSON.stringify(selector)})`, INF, sig => - this.#resolveActionHandle(selector, timeoutMs, sig), - ), + op("tab.scroll()", actionOpMs, sig => untilAborted(sig, () => page.mouse.wheel({ deltaX, deltaY }))), + drag: (from, to) => op("tab.drag()", actionOpMs, sig => this.#drag(from, to, sig)), + waitFor: (selector, opts) => { + const w = waitMs(opts?.timeout); + return op(`tab.waitFor(${JSON.stringify(selector)})`, w, sig => + this.#resolveActionHandle(selector, w, sig), + ); + }, + waitForSelector: (selector, opts) => { + const w = waitMs(opts?.timeout); + return op(`tab.waitForSelector(${JSON.stringify(selector)})`, w, async sig => { + if (parseAriaRefSelector(selector) !== null) return this.#resolveAriaRef(selector); + return (await untilAborted(sig, () => + page.waitForSelector(normalizeSelector(selector), { + timeout: w, + visible: opts?.visible, + hidden: opts?.hidden, + signal: sig, + }), + )) as ElementHandle | null; + }); + }, + waitForNavigation: opts => { + const w = waitMs(opts?.timeout); + return op("tab.waitForNavigation()", w, sig => + untilAborted(sig, () => + page.waitForNavigation({ waitUntil: opts?.waitUntil ?? "load", timeout: w, signal: sig }), + ), + ); + }, evaluate: (fn, ...args) => op("tab.evaluate()", INF, sig => untilAborted(sig, () => @@ -911,8 +1028,8 @@ export class WorkerCore { ), ) as never, scrollIntoView: selector => - op(`tab.scrollIntoView(${JSON.stringify(selector)})`, INF, async sig => { - const handle = await this.#resolveActionHandle(selector, timeoutMs, sig); + op(`tab.scrollIntoView(${JSON.stringify(selector)})`, actionOpMs, async sig => { + const handle = await this.#resolveActionHandle(selector, actionOpMs, sig); try { await untilAborted(sig, () => handle.evaluate(el => { @@ -927,15 +1044,21 @@ export class WorkerCore { } }), select: (selector, ...values) => - op(`tab.select(${JSON.stringify(selector)})`, INF, sig => this.#select(selector, values, timeoutMs, sig)), - uploadFile: (selector, ...filePaths) => - op(`tab.uploadFile(${JSON.stringify(selector)})`, INF, sig => - this.#uploadFile(selector, filePaths, timeoutMs, sig, session), + op(`tab.select(${JSON.stringify(selector)})`, actionOpMs, sig => + this.#select(selector, values, actionOpMs, sig), ), - waitForUrl: (pattern, opts) => - op("tab.waitForUrl()", INF, sig => this.#waitForUrl(pattern, opts?.timeout ?? timeoutMs, sig)), - waitForResponse: (pattern, opts) => - op("tab.waitForResponse()", INF, sig => this.#waitForResponse(pattern, opts?.timeout ?? timeoutMs, sig)), + uploadFile: (selector, ...filePaths) => + op(`tab.uploadFile(${JSON.stringify(selector)})`, actionOpMs, sig => + this.#uploadFile(selector, filePaths, actionOpMs, sig, session), + ), + waitForUrl: (pattern, opts) => { + const w = waitMs(opts?.timeout); + return op("tab.waitForUrl()", w, sig => this.#waitForUrl(pattern, w, sig)); + }, + waitForResponse: (pattern, opts) => { + const w = waitMs(opts?.timeout); + return op("tab.waitForResponse()", w, sig => this.#waitForResponse(pattern, w, sig)); + }, id: id => this.#resolveCachedHandle(id), ref: id => this.#resolveAriaRef(id), }; @@ -1122,7 +1245,7 @@ export class WorkerCore { async #select(selector: string, values: string[], timeoutMs: number, signal: AbortSignal): Promise { const page = this.#requirePage(); const handle = (await untilAborted(signal, () => - page.locator(normalizeSelector(selector)).setTimeout(timeoutMs).waitHandle(), + page.locator(normalizeSelector(selector)).setTimeout(timeoutMs).waitHandle({ signal }), )) as ElementHandle; try { return (await untilAborted(signal, () => @@ -1168,7 +1291,7 @@ export class WorkerCore { if (!filePaths.length) throw new ToolError("tab.uploadFile() requires at least one file path"); const page = this.#requirePage(); const handle = (await untilAborted(signal, () => - page.locator(normalizeSelector(selector)).setTimeout(timeoutMs).waitHandle(), + page.locator(normalizeSelector(selector)).setTimeout(timeoutMs).waitHandle({ signal }), )) as ElementHandle; try { const absolute = filePaths.map(filePath => resolveToCwd(filePath, session.cwd)); @@ -1197,7 +1320,7 @@ export class WorkerCore { const url = (globalThis as unknown as { location: { href: string } }).location.href; return isRe ? new RegExp(m, fl).test(url) : url.includes(m); }, - { timeout, polling: 200 }, + { timeout, polling: 200, signal }, matcher, isRegex, flags, @@ -1218,7 +1341,7 @@ export class WorkerCore { : pattern instanceof RegExp ? response => pattern.test(response.url()) : response => response.url().includes(pattern); - return (await untilAborted(signal, () => page.waitForResponse(predicate, { timeout }))) as HTTPResponse; + return (await untilAborted(signal, () => page.waitForResponse(predicate, { timeout, signal }))) as HTTPResponse; } async #resolveCachedHandle(id: number): Promise { @@ -1257,7 +1380,7 @@ export class WorkerCore { async #resolveActionHandle(selector: string, timeoutMs: number, sig: AbortSignal): Promise { if (parseAriaRefSelector(selector) !== null) return this.#resolveAriaRef(selector); return (await untilAborted(sig, () => - this.#requirePage().locator(normalizeSelector(selector)).setTimeout(timeoutMs).waitHandle(), + this.#requirePage().locator(normalizeSelector(selector)).setTimeout(timeoutMs).waitHandle({ signal: sig }), )) as ElementHandle; } #clearElementCache(): void { diff --git a/packages/coding-agent/test/tools/browser-tab-timeouts.test.ts b/packages/coding-agent/test/tools/browser-tab-timeouts.test.ts new file mode 100644 index 000000000..57468b411 --- /dev/null +++ b/packages/coding-agent/test/tools/browser-tab-timeouts.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, it } from "bun:test"; +import { + normalizeSelector, + resolveOpTimeouts, + resolveWaitTimeout, +} from "@oh-my-pi/pi-coding-agent/tools/browser/tab-worker"; + +// Regression coverage for the "weird timeouts" failure mode: interactive `tab.*` helpers +// used to run with the full cell budget as their internal puppeteer timeout, so a stalled +// click/fill/waitForUrl raced (and lost to) the cell budget and died with the opaque +// "Browser code execution timed out … stalled on …" instead of a fast, named, recoverable +// error. The contracts below pin the fail-fast bounds. + +describe("browser per-op fail-fast ceilings", () => { + it("keeps every per-op deadline strictly under the cell budget so a stall leaves recovery room", () => { + for (const cell of [5_000, 30_000, 120_000]) { + const { budgetBound, quickOpMs, actionOpMs } = resolveOpTimeouts(cell); + expect(budgetBound).toBeLessThan(cell); + expect(quickOpMs).toBeLessThanOrEqual(budgetBound); + expect(actionOpMs).toBeLessThanOrEqual(budgetBound); + expect(actionOpMs).toBeGreaterThan(0); + expect(quickOpMs).toBeGreaterThan(0); + } + }); + + it("caps action/quick ceilings instead of scaling with an inflated cell budget", () => { + // A 5-minute tool timeout must not let a single click block for ~5 minutes. + const big = resolveOpTimeouts(300_000); + const mid = resolveOpTimeouts(60_000); + expect(big.actionOpMs).toBe(mid.actionOpMs); + expect(big.quickOpMs).toBe(mid.quickOpMs); + expect(big.actionOpMs).toBeLessThan(big.budgetBound); + }); + + it("never yields a non-positive deadline for a tiny budget", () => { + const { budgetBound, actionOpMs, quickOpMs } = resolveOpTimeouts(1_000); + expect(budgetBound).toBeGreaterThanOrEqual(1); + expect(actionOpMs).toBeGreaterThanOrEqual(1); + expect(quickOpMs).toBeGreaterThanOrEqual(1); + expect(actionOpMs).toBeLessThanOrEqual(budgetBound); + }); +}); + +describe("browser wait-helper timeout resolution", () => { + it("defaults a wait to the action ceiling when no explicit timeout is given", () => { + const cell = 30_000; + expect(resolveWaitTimeout(cell)).toBe(resolveOpTimeouts(cell).actionOpMs); + }); + + it("honors a positive explicit timeout but clamps it under the cell budget", () => { + const cell = 30_000; + const { budgetBound } = resolveOpTimeouts(cell); + expect(resolveWaitTimeout(cell, 5_000)).toBe(5_000); + expect(resolveWaitTimeout(cell, 120_000)).toBe(budgetBound); + }); + + it("lets a larger tool budget raise the explicit-wait ceiling", () => { + // Raising the tool `timeout` must stay meaningful for explicit waits. + expect(resolveWaitTimeout(120_000, 90_000)).toBe(90_000); + expect(resolveWaitTimeout(30_000, 90_000)).toBe(resolveOpTimeouts(30_000).budgetBound); + }); + + it("maps puppeteer's disable sentinels (0 / Infinity) to the largest bounded wait", () => { + const cell = 30_000; + const { budgetBound } = resolveOpTimeouts(cell); + expect(resolveWaitTimeout(cell, 0)).toBe(budgetBound); + expect(resolveWaitTimeout(cell, Number.POSITIVE_INFINITY)).toBe(budgetBound); + // Crucially, "disable" is still bounded — never the full cell budget. + expect(resolveWaitTimeout(cell, 0)).toBeLessThan(cell); + }); + + it("treats a garbage (negative) timeout as the default, not the longest wait", () => { + const cell = 30_000; + const { budgetBound, actionOpMs } = resolveOpTimeouts(cell); + expect(resolveWaitTimeout(cell, -5_000)).toBe(actionOpMs); + expect(resolveWaitTimeout(cell, -5_000)).not.toBe(budgetBound); + }); +}); + +describe("browser selector guard", () => { + it("rejects Playwright-only selector engines with an actionable message", () => { + expect(() => normalizeSelector('button:has-text("Allow all")')).toThrow(/Playwright-only/); + expect(() => normalizeSelector("div:visible")).toThrow(/not supported/); + expect(() => normalizeSelector(':text("Login")')).toThrow(/Playwright-only/); + }); + + it("passes puppeteer-native and plain CSS selectors through untouched", () => { + expect(normalizeSelector("text/Allow all")).toBe("text/Allow all"); + expect(normalizeSelector("aria/Sign in")).toBe("aria/Sign in"); + expect(normalizeSelector("button.cookie-accept")).toBe("button.cookie-accept"); + // `:has()` is valid modern CSS and must not be mistaken for Playwright `:has-text()`. + expect(normalizeSelector("div:has(> img)")).toBe("div:has(> img)"); + }); + + it("still rewrites legacy p- prefixes", () => { + expect(normalizeSelector("p-text/Continue")).toBe("text/Continue"); + }); +});