From 9f836712e88fd3ef3e9426a5534bd7ddf376d022 Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 17 Jul 2026 02:10:13 +0000 Subject: [PATCH 1/2] fix(browser): rejected non-string tab selectors with a named error tab.click/type/fill/waitFor*/scrollIntoView route their selector through parseAriaRefSelector (.trim) and normalizeSelector (.startsWith) before any validation, so passing the ElementHandle from tab.id(n)/tab.ref(...) (or an un-awaited Promise of one) crashed with the opaque minified "A.trim is not a function" instead of an actionable error. - Added assertSelectorString guard at both selector funnels; throws a ToolError naming the recovery ((await tab.id(n)).click() or a string selector) and distinguishing ElementHandle / Promise / primitive. - Corrected browser.md: handles are called directly, not fed to tab.click. - Regression tests in both selector suites. Fixes #5776 --- packages/coding-agent/CHANGELOG.md | 4 +++ .../coding-agent/src/prompts/tools/browser.md | 2 +- .../src/tools/browser/aria/aria-snapshot.ts | 25 +++++++++++++++++++ .../src/tools/browser/tab-worker.ts | 2 ++ .../test/tools/browser-aria-snapshot.test.ts | 16 ++++++++++++ .../test/tools/browser-tab-timeouts.test.ts | 13 ++++++++++ 6 files changed, 61 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 0e753ced7..bd616d821 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed browser `tab.click`/`type`/`fill`/`waitFor*`/`scrollIntoView` crashing with the opaque minified `A.trim is not a function` when handed the `ElementHandle` from `tab.id(n)`/`tab.ref(...)` (or an un-awaited `Promise` of one); the selector funnels now reject non-strings with a `ToolError` naming the recovery (`(await tab.id(n)).click()` or a string selector), and `browser.md` clarifies that handles are called directly rather than passed as selectors ([#5776](https://github.com/can1357/oh-my-pi/issues/5776)). + ## [17.0.1] - 2026-07-16 ### Changed diff --git a/packages/coding-agent/src/prompts/tools/browser.md b/packages/coding-agent/src/prompts/tools/browser.md index efc1b3f12..81fbbf26d 100644 --- a/packages/coding-agent/src/prompts/tools/browser.md +++ b/packages/coding-agent/src/prompts/tools/browser.md @@ -6,7 +6,7 @@ Drives real Chromium tab; full puppeteer access via JS. - `run` scope: `page`, `browser`, `tab`, `display`, `assert`, `wait` available. `wait(fn)` polls until truthy — use instead of polling inside `tab.evaluate`. - `tab` helpers (drop to raw puppeteer `page` for anything uncovered): - Element handles: `tab.ref("e5")` / `tab.id(n)`. Also `aria-ref=e5` inline. + Element handles: `tab.ref("e5")` / `tab.id(n)` return a handle you call methods on directly — `(await tab.id(n)).click()`, `(await tab.ref("e5")).fill(v)`. They are NOT selectors: `tab.click`/`type`/`fill`/`waitFor*`/`scrollIntoView` take STRING selectors only (pass `"aria-ref=e5"` inline to act on a ref by string). Simple: `tab.goto`, `tab.click`, `tab.type`, `tab.fill`, `tab.press`, `tab.scroll`, `tab.scrollIntoView`, `tab.drag`, `tab.uploadFile`, `tab.select`, `tab.screenshot`, `tab.extract`, `tab.evaluate`. Waits: `tab.waitFor`, `tab.waitForSelector`, `tab.waitForUrl`, `tab.waitForResponse`, `tab.waitForNavigation`. Snapshots: `tab.observe()` → accessibility tree; `tab.ariaSnapshot()` → ARIA YAML with `[ref=eN]`. diff --git a/packages/coding-agent/src/tools/browser/aria/aria-snapshot.ts b/packages/coding-agent/src/tools/browser/aria/aria-snapshot.ts index c37e30983..c497b4aa7 100644 --- a/packages/coding-agent/src/tools/browser/aria/aria-snapshot.ts +++ b/packages/coding-agent/src/tools/browser/aria/aria-snapshot.ts @@ -1,4 +1,5 @@ import type { ElementHandle, JSHandle, Page } from "puppeteer-core"; +import { ToolError } from "../../tool-errors"; import ariaBundle from "./aria-snapshot.bundle.txt" with { type: "text" }; // `aria-snapshot.bundle.txt` is a generated, committed artifact: Playwright's // injected ARIA-snapshot sources (pinned, Apache-2.0) bundled to a CJS module. @@ -68,6 +69,29 @@ export async function resolveAriaRefHandle(page: Page, ref: string): Promise selector.startsWith(prefix)) && diff --git a/packages/coding-agent/test/tools/browser-aria-snapshot.test.ts b/packages/coding-agent/test/tools/browser-aria-snapshot.test.ts index 3dbd22070..d38e04816 100644 --- a/packages/coding-agent/test/tools/browser-aria-snapshot.test.ts +++ b/packages/coding-agent/test/tools/browser-aria-snapshot.test.ts @@ -22,6 +22,22 @@ describe("parseAriaRefSelector", () => { expect(parseAriaRefSelector("aria-ref=button")).toBeNull(); // not an eN id expect(parseAriaRefSelector("aria-ref=")).toBeNull(); }); + + it("rejects non-string selectors (handle/Promise) with a recovery-naming ToolError", () => { + // Regression: tab.click(await tab.id(n)) / tab.click(tab.id(n)) used to reach + // `selector.trim()` and throw the opaque minified `A.trim is not a function`. + const handle = { + click: async () => {}, + asElement() { + return this; + }, + }; + expect(() => parseAriaRefSelector(handle as never)).toThrow(/must be a string; got an ElementHandle/); + expect(() => parseAriaRefSelector(handle as never)).toThrow(/\(await tab\.id\(n\)\)\.click\(\)/); + const promise = Promise.resolve(handle); + expect(() => parseAriaRefSelector(promise as never)).toThrow(/got a Promise \(missing await\?\)/); + promise.catch(() => {}); + }); }); describe("buildAriaSnapshotScript", () => { 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 ed66b8fd9..e5dbf1e14 100644 --- a/packages/coding-agent/test/tools/browser-tab-timeouts.test.ts +++ b/packages/coding-agent/test/tools/browser-tab-timeouts.test.ts @@ -121,4 +121,17 @@ describe("browser selector guard", () => { it("still rewrites legacy p- prefixes", () => { expect(normalizeSelector("p-text/Continue")).toBe("text/Continue"); }); + + it("rejects non-string selectors (handle/number) instead of crashing on .startsWith", () => { + // Regression: passing the ElementHandle from tab.id()/tab.ref() reached + // `selector.startsWith(...)` and threw the opaque `A.trim is not a function`. + const handle = { + click: async () => {}, + asElement() { + return this; + }, + }; + expect(() => normalizeSelector(handle as never)).toThrow(/must be a string; got an ElementHandle/); + expect(() => normalizeSelector(23 as never)).toThrow(/must be a string; got a number/); + }); }); From 7715132e71b293604dc98f065f6dacb22b8ad43a Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 17 Jul 2026 02:14:58 +0000 Subject: [PATCH 2/2] fix(browser): guard cmux selector funnel against non-string selectors CmuxTab.#selectorSpec bypassed the puppeteer-backend guards and called normalized.startsWith(...) directly, so tab.click(await tab.ref("e5")) on a cmux surface still threw the opaque TypeError instead of the named ToolError. Apply assertSelectorString at the cmux funnel too. Fixes #5776 --- packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 950fceeb4..1ded27294 100644 --- a/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts +++ b/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts @@ -9,7 +9,7 @@ import type { ToolSession } from "../../index"; import { resolveToCwd } from "../../path-utils"; import { formatScreenshot } from "../../render-utils"; import { ToolAbortError, ToolError, throwIfAborted } from "../../tool-errors"; -import { type AriaSnapshotOptions, buildAriaSnapshotScript } from "../aria/aria-snapshot"; +import { type AriaSnapshotOptions, assertSelectorString, buildAriaSnapshotScript } from "../aria/aria-snapshot"; import { DEFAULT_VIEWPORT } from "../launch"; import { extractReadableFromHtml, type ReadableFormat } from "../readable"; import { @@ -1015,6 +1015,7 @@ export class CmuxTab { } #selectorSpec(selector: string): SelectorSpec { + assertSelectorString(selector); const raw = selector; let normalized = selector; if (normalized.startsWith("p-text/")) normalized = `text/${normalized.slice("p-text/".length)}`;