feat(coding-agent): added browser navigation helpers and improved timeout management
- Implemented `waitForSelector` and `waitForNavigation` methods in the browser tool API. - Introduced per-operation fail-fast budget management with dynamic timeout clamping. - Added validation for selector engines to reject unsupported Playwright-only platform features. - Standardized error handling to provide descriptive, named timeouts for stalled browser operations.
This commit is contained in:
@@ -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.<op> timed out after <ms>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-`<select>` for `tab.select()`, non-file-input for `tab.uploadFile()`, and screenshot selector misses.
|
||||
- On run timeout, the worker reports `Browser code execution timed out after <ms>ms`; the supervisor may escalate to `Browser code execution hung past grace; tab killed` if the worker does not respond after the grace window.
|
||||
- On run timeout, the worker reports `Browser code execution timed out after <ms>ms` (with `(stalled on <op>)` naming the still-running helper); a single stalled per-op helper instead rejects with `tab.<op>(...) timed out after <ms>ms` before the cell budget is reached. The supervisor may escalate to `Browser code execution hung past grace; tab killed` if the worker does not respond after the grace window.
|
||||
|
||||
## Notes
|
||||
- `loadPuppeteer()` and `loadPuppeteerInWorker()` temporarily redirect `cwd` to a safe Puppeteer directory before importing `puppeteer-core`, because Puppeteer probes the current working directory during module load.
|
||||
|
||||
@@ -1,11 +1,19 @@
|
||||
# Changelog
|
||||
|
||||
## [Unreleased]
|
||||
### Added
|
||||
|
||||
- Added `tab.waitForSelector` for more robust element-wait behavior
|
||||
- Added `tab.waitForNavigation` to monitor and await page transitions
|
||||
|
||||
### Changed
|
||||
|
||||
- Clamped browser tool timeouts to fail fast with named errors instead of opaque cell timeouts
|
||||
- Added strict validation to block unsupported Playwright-only selector engines in browser tools
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed Codex image reads re-encoding to WebP and then failing the next request with an invalid `input_image.image_url`; Codex-bound images now stay in PNG/JPEG-compatible formats.
|
||||
|
||||
- Fixed the `/model` thinking picker labeling the OpenAI GPT-5.5 top effort as `max` instead of the catalog-declared `xhigh` ([#3194](https://github.com/can1357/oh-my-pi/issues/3194)).
|
||||
- Fixed session-title generation silently falling back to the online `smol` model (and billing whatever provider held the resolved API key — OpenRouter in the reporter's case) when the user had explicitly configured a **local** `providers.tinyModel`: `generateSessionTitle` raced local against online with a 10s timeout and fired the online request immediately whenever the local worker returned `null` (unknown key, model not downloaded, transformers.js failure). Now an explicit local-model choice is honored end-to-end — on local failure the session is left untitled with a `logger.warn` instead of billing the smol fallback ([#3187](https://github.com/can1357/oh-my-pi/issues/3187))
|
||||
- Fixed OpenCode MCP discovery so array commands are normalized into a stdio executable plus arguments, `environment` is accepted as the OpenCode env key, and argument lists are omitted when empty. ([#3180](https://github.com/can1357/oh-my-pi/issues/3180))
|
||||
@@ -12264,4 +12272,4 @@ Initial public release.
|
||||
|
||||
## [0.7.6] - 2025-11-13
|
||||
|
||||
Previous releases did not maintain a changelog.
|
||||
Previous releases did not maintain a changelog.
|
||||
@@ -19,17 +19,18 @@ Drives real Chromium tab; full puppeteer access via JS.
|
||||
- `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")`).
|
||||
- `tab.id(n)` — id from last observe → `ElementHandle` (`.click()`, `.type()`, …).
|
||||
- `tab.click(selector)` / `tab.type(selector, text)` / `tab.fill(selector, value)` / `tab.press(key, { selector? })` / `tab.scroll(dx, dy)`.
|
||||
- `tab.waitFor(selector)` — wait until attached; returns `ElementHandle`.
|
||||
- `tab.waitFor(selector, { timeout? })` / `tab.waitForSelector(selector, { timeout?, visible?, hidden? })` — wait until attached (optionally visible/hidden); returns the `ElementHandle`.
|
||||
- `tab.drag(from, to)` — endpoints: selector (center-to-center) or `{ x, y }` viewport point (canvases, sliders).
|
||||
- `tab.scrollIntoView(selector)` — center in viewport; before clicking off-screen elements.
|
||||
- `tab.select(selector, …values)` — set `<select>` option(s); returns selection. `tab.fill` NEVER works for selects.
|
||||
- `tab.uploadFile(selector, …filePaths)` — attach files to `<input type="file">`; 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.<op> timed out` error, never the whole-cell timeout.
|
||||
</instruction>
|
||||
|
||||
<critical>
|
||||
|
||||
@@ -434,6 +434,12 @@ export class CmuxTab {
|
||||
return new CmuxElementHandle(this, selector);
|
||||
}
|
||||
|
||||
async waitForSelector(selector: string, opts?: { timeout?: number }): Promise<CmuxElementHandle> {
|
||||
const timeoutMs = opts?.timeout ?? this.#runContext?.timeoutMs ?? 30_000;
|
||||
await this.#waitForSelector(selector, timeoutMs);
|
||||
return new CmuxElementHandle(this, selector);
|
||||
}
|
||||
|
||||
async evaluate<TResult, TArgs extends unknown[]>(
|
||||
fn: string | ((...args: TArgs) => TResult | Promise<TResult>),
|
||||
...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<null> {
|
||||
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<void> {
|
||||
const start = await this.#dragPoint(from);
|
||||
const end = await this.#dragPoint(to);
|
||||
|
||||
@@ -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<void>;
|
||||
scroll(deltaX: number, deltaY: number): Promise<void>;
|
||||
drag(from: DragTarget, to: DragTarget): Promise<void>;
|
||||
waitFor(selector: string): Promise<ElementHandle>;
|
||||
waitFor(selector: string, opts?: { timeout?: number }): Promise<ElementHandle>;
|
||||
evaluate<TResult, TArgs extends unknown[]>(
|
||||
fn: string | ((...args: TArgs) => TResult | Promise<TResult>),
|
||||
...args: TArgs
|
||||
@@ -134,12 +202,29 @@ interface TabApi {
|
||||
pattern: string | RegExp | ((response: HTTPResponse) => boolean | Promise<boolean>),
|
||||
opts?: { timeout?: number },
|
||||
): Promise<HTTPResponse>;
|
||||
waitForSelector(
|
||||
selector: string,
|
||||
opts?: { timeout?: number; visible?: boolean; hidden?: boolean },
|
||||
): Promise<ElementHandle | null>;
|
||||
waitForNavigation(opts?: {
|
||||
waitUntil?: "load" | "domcontentloaded" | "networkidle0" | "networkidle2";
|
||||
timeout?: number;
|
||||
}): Promise<HTTPResponse | null>;
|
||||
id(n: number): Promise<ElementHandle>;
|
||||
ref(id: string): Promise<ElementHandle>;
|
||||
}
|
||||
|
||||
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 = <T>(label: string, perOpMs: number, fn: (sig: AbortSignal) => Promise<T>): Promise<T> =>
|
||||
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<string[]> {
|
||||
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<ElementHandle> {
|
||||
@@ -1257,7 +1380,7 @@ export class WorkerCore {
|
||||
async #resolveActionHandle(selector: string, timeoutMs: number, sig: AbortSignal): Promise<ElementHandle> {
|
||||
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 {
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user