From 0d07da529a78a9928546430d1c3d8cc953652929 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sun, 12 Jul 2026 16:27:12 +0200 Subject: [PATCH] feat(coding-agent-tools): implemented browser execution safety controls - Implemented cell budget clamping for timeouts to prevent stalled browser operations from exceeding execution limits. - Added `recover` capabilities for tab workers to clear blocking dialogs and safely terminate hung navigations. - Introduced `//!world=main` support for `tab.evaluate` via Puppeteer patch to allow execution in main execution contexts. - Improved failure attribution for tab terminations by tracking dialogs, stalled operations, and specific termination reasons. --- packages/coding-agent/CHANGELOG.md | 11 ++ .../coding-agent/src/prompts/tools/browser.md | 8 +- .../src/tools/browser/cmux/cmux-tab.ts | 20 ++- .../src/tools/browser/run-cancellation.ts | 25 ++- .../src/tools/browser/tab-protocol.ts | 6 + .../src/tools/browser/tab-supervisor.ts | 18 ++- .../src/tools/browser/tab-worker.ts | 148 +++++++++++++++--- .../test/tools/browser-tab-timeouts.test.ts | 26 +++ patches/puppeteer-core@25.3.0.patch | 47 ++++-- 9 files changed, 262 insertions(+), 47 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e47320413..0df0c3212 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -3,12 +3,23 @@ ## [Unreleased] ## [16.4.8] - 2026-07-12 +### Added + +- Added a predicate form to the browser run's `wait()` helper: `wait(fn, { timeout?, interval? })` polls the function (sync or async) until truthy and resolves with that value, failing with a named timeout error (deadline clamped under the cell budget so it always beats the opaque whole-cell timeout) instead of Bun's `sleep expects a number` or a whole-cell stall from in-page polling Promises; both `wait` forms now register in the stall diagnosis of cell timeouts + ### Changed +- Improved tab recovery after timeouts by automatically clearing pending navigation and JS dialogs +- Made `tab.goto` navigation failures catchable with a named error instead of triggering a whole-cell timeout +- Added support for `//!world=main` in `tab.evaluate` to access page-level globals +- Enhanced cell timeout messages to include identification of stalled operations and blocking JS dialogs +- Browser `run` on a tab the supervisor force-killed now reports the kill reason instead of a bare "not alive" - Refined agent workflow to prioritize smoke testing and reduce mandatory upfront test generation ### Fixed +- Fixed the `//!world=main` evaluate directive being silently ignored for string expressions passed through `page.evaluate`/`tab.evaluate`: puppeteer's source-URL tagging boxed the string into a `String` object before the world check +- Fixed tab reuse issues where hung navigation or unhandled modals would cause initialization to stall and trigger a force-kill - Improved search reliability for Perplexity provider by forcing retrieval for all queries - Fixed JS eval cells losing top-level `function` and `var` declarations across cells when the defining cell contained top-level `await` — the async wrapper scoped them to the cell's IIFE instead of publishing them to the worker global diff --git a/packages/coding-agent/src/prompts/tools/browser.md b/packages/coding-agent/src/prompts/tools/browser.md index 30d193b24..0e96346c9 100644 --- a/packages/coding-agent/src/prompts/tools/browser.md +++ b/packages/coding-agent/src/prompts/tools/browser.md @@ -5,7 +5,7 @@ Drives real Chromium tab; full puppeteer access via JS. - Three actions: - `open` — acquire/reuse named tab (`name` defaults `"main"`). Optional `url` (navigate once ready), `viewport`, `dialogs: "accept" | "dismiss"` (auto-handle `alert`/`confirm`/`beforeunload`; else page hangs till you wire `page.on('dialog', …)`). - `close` — release tab by `name`, or all with `all: true`. `kill: true` also kills spawned-app process trees. - - `run` — execute JS in existing tab. `code` = async function body; `page`, `browser`, `tab`, `display`, `assert`, `wait` in scope. Return value JSON-stringified into result; `display(value)` accumulates text/images. `wait(ms)` sleeps; `wait(fn, { timeout?, interval? })` polls `fn` (sync or async) until truthy and resolves with that value (default 30s timeout, 100ms interval; named error on timeout) — use it instead of in-page polling Promises inside `tab.evaluate`. + - `run` — execute JS in existing tab. `code` = async function body; `page`, `browser`, `tab`, `display`, `assert`, `wait` in scope. Return value JSON-stringified into result; `display(value)` accumulates text/images. `wait(ms)` sleeps; `wait(fn, { timeout?, interval? })` polls `fn` (sync or async) until truthy and resolves with that value (default 100ms interval; deadline min(30s, cell budget − 1s), named error on timeout) — use it instead of in-page polling Promises inside `tab.evaluate`. - Tabs survive `run` calls and in-process subagents — open once, reuse. - Browser kinds (`app` on `open`): - default (no `app`) → headless Chromium with stealth patches. @@ -13,7 +13,7 @@ Drives real Chromium tab; full puppeteer access via JS. - `app.cdp_url` → connect to existing CDP endpoint (e.g. `http://127.0.0.1:9222`). - `app.target` (with `path`/`cdp_url`) — substring on url+title picks BrowserWindow. - `tab` helpers; drop to raw puppeteer `page` for anything uncovered: - - `tab.goto(url, { waitUntil? })` — navigate. + - `tab.goto(url, { waitUntil? })` — navigate. A hung load fails ~1s before the cell budget with a named, catchable error and the pending navigation is stopped; for slow pages raise `timeout` or use `waitUntil: "domcontentloaded"`. - `tab.observe({ includeAll?, viewportOnly? })` — accessibility snapshot: `{ url, title, viewport, scroll, elements: [{ id, role, name, value, states, … }] }`. Ids stable until next observe/goto. - `tab.ariaSnapshot(selector?, { depth?, boxes? })` — Playwright-format ARIA-tree YAML (nested roles + accessible names + `/url`/`/placeholder`), scoped to `selector` or the whole document. Every node carries a `[ref=eN]` id; `[cursor=pointer]` flags clickables. Captures dense, hierarchical structure/text that `observe()`'s flat list flattens away. Refs renumber from e1 each call and stay valid until the next `ariaSnapshot()`. - `tab.ref("e5")` — `[ref=eN]` from the last ariaSnapshot → element handle with the common action methods (`.click()`, `.type()`, `.fill()`, `.hover()`, `.evaluate()`, …); the primary way to act on a ref. For convenience `aria-ref=e5` also works inline in `tab.click`/`type`/`fill`/`waitFor`/`scrollIntoView` (e.g. `tab.click("aria-ref=e5")`). @@ -27,10 +27,10 @@ Drives real Chromium tab; full puppeteer access via JS. - `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.evaluate(fn, …args)` — `page.evaluate` for ad-hoc DOM reads. Runs in an ISOLATED world: DOM, `document.title`, `document.fonts` are shared, but the page's OWN JS globals (`window.myFlag`, app state, framework internals) are NOT visible and reads return `undefined`. To touch page globals, start the function body with the main-world directive: `tab.evaluate(() => { //!world=main` `return window.__done; })` (or `/*!world=main*/`; also honored by string expressions and `page.waitForFunction`). - `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/…`. Playwright-only engines/pseudos (`:has-text()`, `:visible`, …) are rejected — use `text/…` or `aria/…`. A stalled action/wait fails fast with a named `tab.` error carrying a match-count diagnosis, never the whole-cell timeout; a selector matching nothing fails in ~2s (pass an explicit `{ timeout }` to `waitFor`/`waitForSelector` to wait out slow-appearing elements). +- 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.` error carrying a match-count diagnosis, never the whole-cell timeout; a selector matching nothing fails in ~2s (pass an explicit `{ timeout }` to `waitFor`/`waitForSelector` to wait out slow-appearing elements). A whole-cell timeout names the stalled op (including `wait(...)`) and any unhandled dialog blocking the page. 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 b4bcdbf70..950fceeb4 100644 --- a/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts +++ b/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts @@ -12,7 +12,12 @@ import { ToolAbortError, ToolError, throwIfAborted } from "../../tool-errors"; import { type AriaSnapshotOptions, buildAriaSnapshotScript } from "../aria/aria-snapshot"; import { DEFAULT_VIEWPORT } from "../launch"; import { extractReadableFromHtml, type ReadableFormat } from "../readable"; -import { bindBrowserRunFacade, type WaitPredicateOptions, waitForBrowserRun } from "../run-cancellation"; +import { + bindBrowserRunFacade, + resolvePredicateTimeout, + type WaitPredicateOptions, + waitForBrowserRun, +} from "../run-cancellation"; import { cloneSafe, RunOutput } from "../run-output"; import type { Observation, ReadyInfo, RunResultOk, ScreenshotResult, SessionSnapshot } from "../tab-protocol"; import { @@ -1352,8 +1357,17 @@ export async function runCmuxCode(tab: CmuxTab, opts: RunCmuxCodeOptions): Promi assert: (cond: unknown, text?: string): void => { if (!cond) throw new ToolError(text ?? "Assertion failed"); }, - wait: (msOrPredicate: number | (() => unknown), opts?: WaitPredicateOptions): Promise => - waitForBrowserRun(msOrPredicate, signal, opts), + wait: (msOrPredicate: number | (() => unknown), waitOpts?: WaitPredicateOptions): Promise => + waitForBrowserRun( + msOrPredicate, + signal, + typeof msOrPredicate === "number" + ? waitOpts + : { + timeout: resolvePredicateTimeout(opts.timeoutMs, waitOpts?.timeout), + interval: waitOpts?.interval, + }, + ), }); const hooks: RuntimeHooks = { diff --git a/packages/coding-agent/src/tools/browser/run-cancellation.ts b/packages/coding-agent/src/tools/browser/run-cancellation.ts index 41460524d..d2313af14 100644 --- a/packages/coding-agent/src/tools/browser/run-cancellation.ts +++ b/packages/coding-agent/src/tools/browser/run-cancellation.ts @@ -16,14 +16,33 @@ export function markHandled(promise: Promise): Promise { return promise; } +/** Headroom subtracted from the cell budget so an in-run deadline fires before the opaque whole-cell timeout. */ +export const CELL_BUDGET_SLACK_MS = 1_000; + +/** Default poll deadline for `wait(predicate)` before clamping to the cell budget. */ +export const DEFAULT_PREDICATE_TIMEOUT_MS = 30_000; + /** Options for the predicate form of the run-scoped `wait()` helper. */ export interface WaitPredicateOptions { - /** Max time to poll before failing, in ms (default 30_000). */ + /** Max time to poll before failing, in ms (default 30s, clamped to the cell budget). */ timeout?: number; /** Poll interval in ms (default 100, floor 10). */ interval?: number; } +/** + * Effective `wait(predicate)` deadline for a given cell budget. Always strictly below + * the cell budget so the named `wait(predicate) timed out` error wins the race against + * the opaque whole-cell "Browser code execution timed out". `0`/`Infinity` ("disable") + * map to the largest bounded deadline; negative/NaN garbage falls back to the default. + */ +export function resolvePredicateTimeout(cellTimeoutMs: number, explicit?: number): number { + const budgetBound = Math.max(1, cellTimeoutMs - CELL_BUDGET_SLACK_MS); + if (explicit === 0 || explicit === Number.POSITIVE_INFINITY) return budgetBound; + if (explicit !== undefined && Number.isFinite(explicit) && explicit > 0) return Math.min(explicit, budgetBound); + return Math.min(DEFAULT_PREDICATE_TIMEOUT_MS, budgetBound); +} + /** * Run-scoped `wait()` helper for evaluated browser code, honoring the owning run's * cancellation signal. @@ -49,7 +68,9 @@ export function waitForBrowserRun( throw new ToolError("wait(...) expects milliseconds (number) or a predicate function to poll"); } const timeout = - opts?.timeout !== undefined && Number.isFinite(opts.timeout) && opts.timeout > 0 ? opts.timeout : 30_000; + opts?.timeout !== undefined && Number.isFinite(opts.timeout) && opts.timeout > 0 + ? opts.timeout + : DEFAULT_PREDICATE_TIMEOUT_MS; const interval = Math.max(opts?.interval ?? 100, 10); const deadline = Date.now() + timeout; for (;;) { diff --git a/packages/coding-agent/src/tools/browser/tab-protocol.ts b/packages/coding-agent/src/tools/browser/tab-protocol.ts index d2f5d51bd..96c946c4a 100644 --- a/packages/coding-agent/src/tools/browser/tab-protocol.ts +++ b/packages/coding-agent/src/tools/browser/tab-protocol.ts @@ -59,6 +59,12 @@ export type WorkerInitPayload = safeDir: string; targetId: string; dialogs?: "accept" | "dismiss"; + /** + * Post-timeout recycle: before adopting the page, dismiss any open JS dialog and + * stop a pending navigation so a blocked target cannot stall worker init (which + * previously force-killed the tab). Never set for first-time Electron attach. + */ + recover?: boolean; }; export type ToolReply = { ok: true; value: unknown } | { ok: false; error: RunErrorPayload }; diff --git a/packages/coding-agent/src/tools/browser/tab-supervisor.ts b/packages/coding-agent/src/tools/browser/tab-supervisor.ts index 921fb23ff..0d0a7324a 100644 --- a/packages/coding-agent/src/tools/browser/tab-supervisor.ts +++ b/packages/coding-agent/src/tools/browser/tab-supervisor.ts @@ -131,6 +131,10 @@ const tabs = new Map(); // awaits) cannot interleave and leak a worker + browser refCount. const acquireChains = new Map>(); const GRACE_MS = 750; +// Names of tabs the supervisor force-killed (timeout past grace, failed recycle), +// mapped to the kill reason. Lets the next `run` on that name explain WHY the tab +// vanished instead of a bare "not alive". Cleared when the name is opened again. +const killedTabs = new Map(); export function getTab(name: string): TabSession | undefined { return tabs.get(name); @@ -161,6 +165,7 @@ async function acquireTabImpl( if (opts.signal?.aborted) { throw new ToolAbortError("Browser tab open aborted"); } + killedTabs.delete(name); // Temporary refCount hold so releasing an existing tab on the SAME browser // below cannot drop it to refCount 0 and dispose the instance we are about // to reuse (e.g. reopening the sole tab with a different dialogs policy). @@ -386,7 +391,14 @@ async function runInTabWithSnapshot( snapshot: SessionSnapshot, ): Promise { const tab = tabs.get(name); - if (!tab || tab.state === "dead") throw new ToolError(`Tab ${JSON.stringify(name)} is not alive. Reopen it.`); + if (!tab || tab.state === "dead") { + const killed = killedTabs.get(name); + throw new ToolError( + killed + ? `Tab ${JSON.stringify(name)} was killed: ${killed}. Reopen it.` + : `Tab ${JSON.stringify(name)} is not alive. Open it first with action:"open".`, + ); + } if (tab.pending.size > 0) throw new ToolError(`Tab ${JSON.stringify(name)} is busy`); const id = Snowflake.next(); const { promise, resolve, reject } = Promise.withResolvers(); @@ -712,6 +724,9 @@ async function recycleTimedOutWorkerTab(tab: WorkerTabSession, timeoutMs: number safeDir: getPuppeteerDir(), targetId: tab.targetId, dialogs: tab.dialogPolicy, + // Unblock a wedged page (open JS dialog, hung navigation) before adopting it — + // otherwise init stalls, times out, and the tab gets force-killed. + recover: true, }; let worker = await spawnTabWorker(); try { @@ -743,6 +758,7 @@ async function recycleTimedOutWorkerTab(tab: WorkerTabSession, timeoutMs: number async function forceKillTab(name: string, reason: string): Promise { const tab = tabs.get(name); if (!tab) return; + killedTabs.set(name, reason); tab.state = "dead"; const error = postmortem.markExpectedCleanupError(new ToolError(reason)); for (const pending of tab.pending.values()) pending.reject(error); diff --git a/packages/coding-agent/src/tools/browser/tab-worker.ts b/packages/coding-agent/src/tools/browser/tab-worker.ts index 973d624a2..b51734c1f 100644 --- a/packages/coding-agent/src/tools/browser/tab-worker.ts +++ b/packages/coding-agent/src/tools/browser/tab-worker.ts @@ -6,6 +6,7 @@ import { postmortem, Snowflake, untilAborted } from "@oh-my-pi/pi-utils"; import type { HTMLElement } from "linkedom"; import type { Browser, + CDPSession, Dialog, ElementHandle, ElementScreenshotOptions, @@ -35,7 +36,13 @@ import { loadPuppeteerInWorker, } from "./launch"; import { extractReadableFromHtml, type ReadableFormat } from "./readable"; -import { markHandled, type WaitPredicateOptions, waitForBrowserRun } from "./run-cancellation"; +import { + CELL_BUDGET_SLACK_MS, + markHandled, + resolvePredicateTimeout, + type WaitPredicateOptions, + waitForBrowserRun, +} from "./run-cancellation"; import { cloneSafe, RunOutput } from "./run-output"; import type { Observation, @@ -105,6 +112,11 @@ const PLAYWRIGHT_ONLY_SELECTOR_RE = 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 }; +/** Last JS dialog seen on the page; kept for timeout attribution until handled or navigation. */ +interface OpenDialogInfo { + type: string; + message: string; +} /** * Per-op fail-fast ceilings for `tab.*` helpers. All are kept strictly under the cell @@ -126,7 +138,7 @@ type ActionabilityResult = { ok: true; x: number; y: number } | { ok: false; rea const QUICK_OP_TIMEOUT_MS = 20_000; const ACTION_OP_TIMEOUT_MS = 8_000; /** Headroom subtracted from the cell budget so a per-op deadline fires before it. */ -const OP_DEADLINE_SLACK_MS = 1_000; +const OP_DEADLINE_SLACK_MS = CELL_BUDGET_SLACK_MS; /** * A selector op whose selector has matched nothing for this long fails fast with the * zero-match hint instead of burning the rest of its deadline: a wrong selector or a @@ -595,6 +607,7 @@ export class WorkerCore { #mode?: WorkerInitPayload["mode"]; #dialogPolicy?: DialogPolicy; #dialogHandler?: (dialog: Dialog) => void; + #openDialog?: OpenDialogInfo; constructor(transport: Transport) { this.#transport = transport; @@ -648,6 +661,7 @@ export class WorkerCore { }); if (payload.mode === "headless") { this.#page = await this.#browser.newPage(); + this.#observeDialogs(); await applyStealthPatches(this.#browser, this.#page, { browserSession: null, override: null }); await applyViewport(this.#page, payload.viewport); if (payload.dialogs) this.#applyDialogPolicy(payload.dialogs); @@ -659,7 +673,15 @@ export class WorkerCore { }); } } else { - this.#page = await this.#findAttachedPage(payload.targetId); + const target = await this.#findAttachedTarget(payload.targetId); + // Post-timeout recycle: unblock the target BEFORE adopting the page — an open + // modal dialog or hung navigation can stall `target.page()` / ready info, and a + // stalled init used to time out and force-kill the tab. + if (payload.recover) await this.#recoverAttachedTarget(target); + const page = await target.page(); + if (!page) throw new ToolError(`Target ${payload.targetId} is no longer available on the attached browser`); + this.#page = page; + this.#observeDialogs(); if (payload.dialogs) this.#applyDialogPolicy(payload.dialogs); } this.#targetId = await targetIdForPage(this.#page); @@ -669,17 +691,53 @@ export class WorkerCore { } } - async #findAttachedPage(targetId: string): Promise { + async #findAttachedTarget(targetId: string): Promise { if (!this.#browser) throw new ToolError("Browser is not connected"); for (const target of this.#browser.targets()) { if ((await targetIdForTarget(target).catch(() => "")) !== targetId) continue; - const page = await target.page(); - if (!page) break; - return page; + return target; } throw new ToolError(`Target ${targetId} is no longer available on the attached browser`); } + /** + * Best-effort unblocking of a wedged target during post-timeout recovery: dismiss any + * open JS dialog and stop a pending navigation over a raw CDP session (created on the + * target, not the page, so it works while the page itself is unresponsive). Every step + * tolerates "nothing to do". + */ + async #recoverAttachedTarget(target: Target): Promise { + let session: CDPSession | undefined; + try { + session = await target.createCDPSession(); + await session.send("Page.enable").catch(() => undefined); + await session.send("Page.handleJavaScriptDialog", { accept: false }).catch(() => undefined); + await session.send("Page.stopLoading").catch(() => undefined); + } catch (error) { + this.#log("debug", "Recovery CDP session failed; proceeding with attach", { + error: error instanceof Error ? error.message : String(error), + }); + } finally { + await session?.detach().catch(() => undefined); + } + } + + /** + * Record JS dialogs for timeout attribution without handling them (semantics of an + * unset `dialogs` policy are unchanged — the page stays blocked until user code or + * the policy handler acts). Cleared when the policy handler settles the dialog or a + * main-frame navigation proves the modal is gone. + */ + #observeDialogs(): void { + const page = this.#requirePage(); + page.on("dialog", dialog => { + this.#openDialog = { type: dialog.type(), message: dialog.message() }; + }); + page.on("framenavigated", frame => { + if (frame === page.mainFrame()) this.#openDialog = undefined; + }); + } + async #currentReadyInfo(): Promise { const page = this.#requirePage(); const targetId = this.#targetId ?? (await targetIdForPage(page)); @@ -698,11 +756,15 @@ export class WorkerCore { if (this.#dialogHandler) page.off("dialog", this.#dialogHandler); const handler = (dialog: Dialog): void => { const action = policy === "accept" ? dialog.accept() : dialog.dismiss(); - void action.catch(err => - this.#log("debug", "Dialog auto-handler failed", { - policy, - error: err instanceof Error ? err.message : String(err), - }), + void action.then( + () => { + this.#openDialog = undefined; + }, + err => + this.#log("debug", "Dialog auto-handler failed", { + policy, + error: err instanceof Error ? err.message : String(err), + }), ); }; page.on("dialog", handler); @@ -761,8 +823,20 @@ export class WorkerCore { assert: (cond: unknown, text?: string): void => { if (!cond) throw new ToolError(text ?? "Assertion failed"); }, - wait: (msOrPredicate: number | (() => unknown), opts?: WaitPredicateOptions): Promise => - waitForBrowserRun(msOrPredicate, signal, opts), + // Both wait forms register in the in-flight map so a cell that dies while + // sleeping/polling names the culprit instead of a bare whole-cell timeout. + wait: (msOrPredicate: number | (() => unknown), opts?: WaitPredicateOptions): Promise => { + const label = typeof msOrPredicate === "number" ? `wait(${msOrPredicate}ms)` : "wait(predicate)"; + const resolved = + typeof msOrPredicate === "number" + ? undefined + : { timeout: resolvePredicateTimeout(msg.timeoutMs, opts?.timeout), interval: opts?.interval }; + return markHandled( + this.#runOp(active, label, signal, Number.POSITIVE_INFINITY, sig => + waitForBrowserRun(msOrPredicate, sig, resolved), + ), + ); + }, }); const { promise: cancelRejection, reject: rejectCancel } = Promise.withResolvers(); const onCancel = (): void => { @@ -772,9 +846,13 @@ export class WorkerCore { : new ToolAbortError(undefined, { cause: signal.reason }); if (timeoutSignal.aborted) { const stalled = describeInflight(active.inflight); + const dialog = this.#openDialog; + const dialogNote = dialog + ? `; a ${dialog.type}(${JSON.stringify(dialog.message.slice(0, 80))}) dialog opened during this run and may still block the page — reopen the tab with dialogs:"accept"|"dismiss" or handle page.on('dialog')` + : ""; rejectCancel( new ToolError( - `Browser code execution timed out after ${msg.timeoutMs}ms${stalled ? ` (stalled on ${stalled})` : ""}`, + `Browser code execution timed out after ${msg.timeoutMs}ms${stalled ? ` (stalled on ${stalled})` : ""}${dialogNote}`, ), ); } else { @@ -986,7 +1064,7 @@ export class WorkerCore { active: ActiveRun, ): TabApi { const page = this.#requirePage(); - const { quickOpMs, actionOpMs } = resolveOpTimeouts(timeoutMs); + const { budgetBound, quickOpMs, actionOpMs } = resolveOpTimeouts(timeoutMs); const waitMs = (explicit?: number): number => resolveWaitTimeout(timeoutMs, explicit); const INF = Number.POSITIVE_INFINITY; const op = ( @@ -1004,10 +1082,24 @@ export class WorkerCore { goto: (url, opts) => op(`tab.goto(${JSON.stringify(url)})`, INF, async sig => { this.#clearElementCache(); - // Default to "load" because dev servers with HMR/WS never reach networkidle. - await untilAborted(sig, () => - page.goto(url, { waitUntil: opts?.waitUntil ?? "load", timeout: timeoutMs }), - ); + try { + // Default to "load" because dev servers with HMR/WS never reach networkidle. + // budgetBound (not the full cell) so a hung navigation fails named and + // catchable inside the run instead of dying with the whole cell. + await untilAborted(sig, () => + page.goto(url, { waitUntil: opts?.waitUntil ?? "load", timeout: budgetBound }), + ); + } catch (err) { + if (err instanceof Error && err.name === "TimeoutError") { + // Abandon the hung navigation NOW — a still-pending load stalls every + // later op on this page and cascades into more opaque timeouts. + await this.#stopLoading(); + throw new ToolError( + `tab.goto(${JSON.stringify(url)}) timed out after ${budgetBound}ms; pending navigation stopped — retry with a longer tool timeout or waitUntil:"domcontentloaded"`, + ); + } + throw err; + } }), observe: opts => op("tab.observe()", quickOpMs, sig => this.#collectObservation({ ...opts, signal: sig })), ariaSnapshot: (selector, opts) => @@ -1554,6 +1646,22 @@ export class WorkerCore { for (const handle of handles) void handle.dispose().catch(() => undefined); } + /** Best-effort `Page.stopLoading` so an abandoned navigation cannot stall later ops. */ + async #stopLoading(): Promise { + try { + const session = await this.#requirePage().createCDPSession(); + try { + await session.send("Page.stopLoading"); + } finally { + await session.detach().catch(() => undefined); + } + } catch (error) { + this.#log("debug", "Page.stopLoading failed", { + error: error instanceof Error ? error.message : String(error), + }); + } + } + async #close(): Promise { this.#unsub(); this.#clearElementCache(); diff --git a/packages/coding-agent/test/tools/browser-tab-timeouts.test.ts b/packages/coding-agent/test/tools/browser-tab-timeouts.test.ts index 57468b411..ed66b8fd9 100644 --- a/packages/coding-agent/test/tools/browser-tab-timeouts.test.ts +++ b/packages/coding-agent/test/tools/browser-tab-timeouts.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "bun:test"; +import { resolvePredicateTimeout } from "@oh-my-pi/pi-coding-agent/tools/browser/run-cancellation"; import { normalizeSelector, resolveOpTimeouts, @@ -77,6 +78,31 @@ describe("browser wait-helper timeout resolution", () => { }); }); +describe("browser wait(predicate) deadline resolution", () => { + it("keeps the default deadline strictly under the cell budget so the named error wins", () => { + // Default cell (30s): the old 30s predicate default tied the cell timer and lost the + // race, surfacing the opaque whole-cell timeout instead of the named wait error. + for (const cell of [5_000, 30_000, 120_000]) { + expect(resolvePredicateTimeout(cell)).toBeLessThan(cell); + } + expect(resolvePredicateTimeout(120_000)).toBe(30_000); + expect(resolvePredicateTimeout(5_000)).toBe(4_000); + }); + + it("honors an explicit deadline but clamps it under the cell budget", () => { + expect(resolvePredicateTimeout(30_000, 5_000)).toBe(5_000); + expect(resolvePredicateTimeout(30_000, 90_000)).toBe(29_000); + expect(resolvePredicateTimeout(120_000, 90_000)).toBe(90_000); + }); + + it("maps disable sentinels to the largest bounded deadline and garbage to the default", () => { + expect(resolvePredicateTimeout(30_000, 0)).toBe(29_000); + expect(resolvePredicateTimeout(30_000, Number.POSITIVE_INFINITY)).toBe(29_000); + expect(resolvePredicateTimeout(30_000, -5)).toBe(29_000); + expect(resolvePredicateTimeout(30_000, Number.NaN)).toBe(29_000); + }); +}); + describe("browser selector guard", () => { it("rejects Playwright-only selector engines with an actionable message", () => { expect(() => normalizeSelector('button:has-text("Allow all")')).toThrow(/Playwright-only/); diff --git a/patches/puppeteer-core@25.3.0.patch b/patches/puppeteer-core@25.3.0.patch index bc93ba914..b2ececd9f 100644 --- a/patches/puppeteer-core@25.3.0.patch +++ b/patches/puppeteer-core@25.3.0.patch @@ -1,16 +1,24 @@ +diff --git a/node_modules/puppeteer-core/.bun-tag-2e714b457f0bd8e8 b/.bun-tag-2e714b457f0bd8e8 +new file mode 100644 +index 0000000000000000000000000000000000000000..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 diff --git a/node_modules/puppeteer-core/.bun-tag-a797aeb3ca2bd69f b/.bun-tag-a797aeb3ca2bd69f new file mode 100644 index 0000000000000000000000000000000000000000..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 diff --git a/lib/puppeteer/api/ElementHandle.js b/lib/puppeteer/api/ElementHandle.js -index 81454fe2af73518f7e76f04cbe3a7f170d735fa5..6f9eedb9fc1344901705bc83cb3cf415e6271e28 100644 +index 81454fe2af73518f7e76f04cbe3a7f170d735fa5..ee6b042c87861f00d7fcbb6e3eb6e87af232ebbd 100644 --- a/lib/puppeteer/api/ElementHandle.js +++ b/lib/puppeteer/api/ElementHandle.js -@@ -102,6 +102,31 @@ import { throwIfDisposed } from '../util/decorators.js'; +@@ -102,6 +102,36 @@ import { throwIfDisposed } from '../util/decorators.js'; import { _isElementHandle } from './ElementHandleSymbol.js'; import { JSHandle } from './JSHandle.js'; import { NodeLocator } from './locators/locators.js'; +const MAIN_WORLD_DIRECTIVE = /^\s*(?:(?:\/\/!world=main(?=$|\s))|(?:\/\*!world=main\s*\*\/))/; +const shouldEvaluateInMainWorld = (pageFunction) => { ++ if (pageFunction instanceof String) { ++ // withSourcePuppeteerURLIfNone tags string page functions via Object.assign, ++ // which boxes the primitive; unbox so the directive check still sees a string. ++ pageFunction = pageFunction.toString(); ++ } + if (typeof pageFunction !== 'function' && typeof pageFunction !== 'string') { + return false; + } @@ -33,11 +41,11 @@ index 81454fe2af73518f7e76f04cbe3a7f170d735fa5..6f9eedb9fc1344901705bc83cb3cf415 + const arrowIndex = source.indexOf('=>'); + const bodyStart = source.indexOf('{', arrowIndex >= 0 ? arrowIndex : 0); + return bodyStart >= 0 && MAIN_WORLD_DIRECTIVE.test(source.slice(bodyStart + 1)); -+}; ++} /** * A given method will have it's `this` replaced with an isolated version of * `this` when decorated with this decorator. -@@ -299,6 +324,7 @@ let ElementHandle = (() => { +@@ -299,6 +329,7 @@ let ElementHandle = (() => { * trying to adopt it multiple times */ isolatedHandle = __runInitializers(this, _instanceExtraInitializers); @@ -45,7 +53,7 @@ index 81454fe2af73518f7e76f04cbe3a7f170d735fa5..6f9eedb9fc1344901705bc83cb3cf415 /** * @internal */ -@@ -335,19 +361,37 @@ let ElementHandle = (() => { +@@ -335,19 +366,37 @@ let ElementHandle = (() => { async getProperties() { return await this.handle.getProperties(); } @@ -85,7 +93,7 @@ index 81454fe2af73518f7e76f04cbe3a7f170d735fa5..6f9eedb9fc1344901705bc83cb3cf415 } /** * @internal -@@ -371,7 +415,7 @@ let ElementHandle = (() => { +@@ -371,7 +420,7 @@ let ElementHandle = (() => { * @internal */ async dispose() { @@ -94,7 +102,7 @@ index 81454fe2af73518f7e76f04cbe3a7f170d735fa5..6f9eedb9fc1344901705bc83cb3cf415 } /** * @internal -@@ -555,15 +599,27 @@ let ElementHandle = (() => { +@@ -555,15 +604,27 @@ let ElementHandle = (() => { async $$eval(selector, pageFunction, ...args) { const env_2 = { stack: [], error: void 0, hasError: false }; try { @@ -127,15 +135,20 @@ index 81454fe2af73518f7e76f04cbe3a7f170d735fa5..6f9eedb9fc1344901705bc83cb3cf415 ]); return result; diff --git a/lib/puppeteer/api/Frame.js b/lib/puppeteer/api/Frame.js -index 8698d2a5a976b277344fe42e90ffe985bdf0fbd7..01614143cba96055d0f0c22a1eb903d4265ca15f 100644 +index 8698d2a5a976b277344fe42e90ffe985bdf0fbd7..62c3b7ca8ec987cc62ceacec69acf6545bb59b86 100644 --- a/lib/puppeteer/api/Frame.js +++ b/lib/puppeteer/api/Frame.js -@@ -119,6 +119,31 @@ export var FrameEvent; +@@ -119,6 +119,36 @@ export var FrameEvent; export const throwIfDetached = throwIfDisposed(frame => { return `Attempted to use detached Frame '${frame._id}'.`; }); +const MAIN_WORLD_DIRECTIVE = /^\s*(?:(?:\/\/!world=main(?=$|\s))|(?:\/\*!world=main\s*\*\/))/; +const shouldEvaluateInMainWorld = (pageFunction) => { ++ if (pageFunction instanceof String) { ++ // withSourcePuppeteerURLIfNone tags string page functions via Object.assign, ++ // which boxes the primitive; unbox so the directive check still sees a string. ++ pageFunction = pageFunction.toString(); ++ } + if (typeof pageFunction !== 'function' && typeof pageFunction !== 'string') { + return false; + } @@ -158,11 +171,11 @@ index 8698d2a5a976b277344fe42e90ffe985bdf0fbd7..01614143cba96055d0f0c22a1eb903d4 + const arrowIndex = source.indexOf('=>'); + const bodyStart = source.indexOf('{', arrowIndex >= 0 ? arrowIndex : 0); + return bodyStart >= 0 && MAIN_WORLD_DIRECTIVE.test(source.slice(bodyStart + 1)); -+}; ++} /** * Represents a DOM frame. * -@@ -277,12 +302,21 @@ let Frame = (() => { +@@ -277,12 +307,21 @@ let Frame = (() => { super(); } #_document; @@ -186,7 +199,7 @@ index 8698d2a5a976b277344fe42e90ffe985bdf0fbd7..01614143cba96055d0f0c22a1eb903d4 return document; }); } -@@ -295,6 +329,7 @@ let Frame = (() => { +@@ -295,6 +334,7 @@ let Frame = (() => { */ clearDocumentHandle() { this.#_document = undefined; @@ -194,7 +207,7 @@ index 8698d2a5a976b277344fe42e90ffe985bdf0fbd7..01614143cba96055d0f0c22a1eb903d4 } /** * @returns The frame element associated with this frame (if any). -@@ -345,8 +380,9 @@ let Frame = (() => { +@@ -345,8 +385,9 @@ let Frame = (() => { * See {@link Page.evaluateHandle} for details. */ async evaluateHandle(pageFunction, ...args) { @@ -205,7 +218,7 @@ index 8698d2a5a976b277344fe42e90ffe985bdf0fbd7..01614143cba96055d0f0c22a1eb903d4 } /** * Behaves identically to {@link Page.evaluate} except it's run within -@@ -355,8 +391,9 @@ let Frame = (() => { +@@ -355,8 +396,9 @@ let Frame = (() => { * See {@link Page.evaluate} for details. */ async evaluate(pageFunction, ...args) { @@ -216,7 +229,7 @@ index 8698d2a5a976b277344fe42e90ffe985bdf0fbd7..01614143cba96055d0f0c22a1eb903d4 } /** * @internal -@@ -458,9 +495,10 @@ let Frame = (() => { +@@ -458,9 +500,10 @@ let Frame = (() => { * @returns A promise to the result of the function. */ async $eval(selector, pageFunction, ...args) { @@ -228,7 +241,7 @@ index 8698d2a5a976b277344fe42e90ffe985bdf0fbd7..01614143cba96055d0f0c22a1eb903d4 return await document.$eval(selector, pageFunction, ...args); } /** -@@ -498,9 +536,10 @@ let Frame = (() => { +@@ -498,9 +541,10 @@ let Frame = (() => { * @returns A promise to the result of the function. */ async $$eval(selector, pageFunction, ...args) { @@ -240,7 +253,7 @@ index 8698d2a5a976b277344fe42e90ffe985bdf0fbd7..01614143cba96055d0f0c22a1eb903d4 return await document.$$eval(selector, pageFunction, ...args); } /** -@@ -577,7 +616,8 @@ let Frame = (() => { +@@ -577,7 +621,8 @@ let Frame = (() => { * @returns the promise which resolve when the `pageFunction` returns a truthy value. */ async waitForFunction(pageFunction, options = {}, ...args) {