feat(browser): migrate from puppeteer-core to patchright.#3141
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Preserve tab.evaluate variadic arguments
The public tab helper still accepts tab.evaluate(fn, ...args), but Playwright/Patchright's page.evaluate accepts only a single argument object, unlike Puppeteer's varargs. With this forwarding, calls such as await tab.evaluate((a, b) => a + b, 1, 2) only deliver the first argument and leave later parameters undefined, breaking existing browser snippets that worked before the migration.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const role = typeof value.role === "string" && value.role.length > 0 ? value.role : "generic"; | ||
| const name = typeof value.name === "string" && value.name.length > 0 ? value.name : undefined; | ||
| elements.push({ id, role, name, states: [] }); | ||
| elements.push({ id: normalizedRef, role, name, states: [] }); |
There was a problem hiding this comment.
In cmux mode the snapshot refs already have the e prefix (for example e2), but storing that full value as the public id here interacts with CmuxTab.#rememberObservedElements, which builds @e${element.id} and therefore caches @ee2. As a result, the documented const obs = await tab.observe(); await (await tab.id(obs.elements[0].id)).click() flow times out in cmux even though the element exists; either keep numeric ids or build cached refs as @${element.id}.
Useful? React with 👍 / 👎.
| const pathFormat = explicitPath ? imageFormatForPath(explicitPath) : "png"; | ||
| const captureType: "png" | "jpeg" = pathFormat === "jpeg" ? "jpeg" : "png"; |
There was a problem hiding this comment.
When a caller explicitly saves to *.webp, pathFormat becomes webp but captureType is forced to png; because explicit paths set saveFullRes, the code later writes the original PNG buffer to the requested .webp path instead of the resized WebP buffer mentioned in the comment. This regresses the previous contract that save: "shot.webp" writes WebP bytes, and consumers will get a mislabeled PNG file.
Useful? React with 👍 / 👎.
| }), | ||
| click: selector => | ||
| op(`tab.click(${JSON.stringify(selector)})`, INF, async sig => { | ||
| const resolved = normalizeSelector(selector); |
There was a problem hiding this comment.
For documented Puppeteer-style selectors such as p-aria/[name="Save"], p-xpath/..., or direct aria/..., normalizeSelector still returns slash-prefixed query-handler strings, but the new Playwright/Patchright page.locator(resolved) path does not understand Puppeteer's aria/, xpath/, or pierce/ syntax. Those selectors now fail in click/fill/type/wait helpers instead of resolving elements; translate them to Playwright selector syntax or keep a compatibility resolver before passing them to locator.
Useful? React with 👍 / 👎.
H4vC
force-pushed
the
patchright-migration
branch
2 times, most recently
from
f166456 to
84fa02e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84fa02e301
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| expectedPath: exe, | ||
| }); | ||
| return executablePath; | ||
| return exe; |
There was a problem hiding this comment.
On hosts without a system Chrome/Chromium and without PUPPETEER_EXECUTABLE_PATH, this returns chromium.executablePath() even when the file does not exist; fresh installs do not run npx patchright install chromium, while the previous implementation downloaded Chromium on first use. In that common server/CI setup the default browser.open path now proceeds to launchServer with a missing executable and fails instead of self-provisioning or giving an actionable install step before launch.
Useful? React with 👍 / 👎.
| return `aria/${rest}`; | ||
| // Playwright has no aria/ engine; use text= as the closest equivalent. | ||
| // For role-specific matching, agents should use page.getByRole() directly. | ||
| if (name) return `text=${name.trim()}`; |
There was a problem hiding this comment.
When a legacy aria/... or p-aria/... selector targets a control whose accessible name comes from aria-label, title, or an image alt rather than visible text, converting it to text= will not match the element even though the old Puppeteer aria handler did. The fresh evidence after the prior selector issue is this current translation to text selectors, so documented calls like tab.click('aria/[name="Save"]') still regress for icon-only accessible buttons.
Useful? React with 👍 / 👎.
| // Playwright can't capture webp natively. When a .webp save is requested, use | ||
| // the resized buffer (which resizeImage encodes as webp) even for full-res saves, | ||
| // so the file bytes match the extension. | ||
| const useResizedForSave = pathFormat === "webp"; |
There was a problem hiding this comment.
For explicit save: "shot.webp", this branch writes resized.buffer, but the resize call above honors session.excludeWebP; when WebP is excluded for the active model or environment, resizeImage can only return PNG/JPEG bytes. That leaves callers with a .webp path containing non-WebP data despite the extension-specific save request, so either force a WebP encode for explicit WebP saves or reject/rename when WebP output is disabled.
Useful? React with 👍 / 👎.
H4vC
force-pushed
the
patchright-migration
branch
from
84fa02e to
f3a1a0e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3a1a0e11c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const trimmed = name.trim(); | ||
| // Match by accessible name: aria-label (icon-only buttons) or visible text. | ||
| // Use CSS union so both sources match — the old Puppeteer aria handler did the same. | ||
| return `[aria-label="${trimmed}"], :has-text("${trimmed}")`; |
There was a problem hiding this comment.
When a legacy aria/[name="..."] selector targets visible text, this bare :has-text(...) branch matches every ancestor containing that text (often body, wrappers, and the button), and the later page.locator(resolved).click()/fill() path uses Playwright strict locators rather than the custom text resolver. The fresh issue in this version is the new CSS union, so an ordinary page with one visible “Save” button can still fail with a strict-mode violation even though the old aria handler resolved the control.
Useful? React with 👍 / 👎.
| reuseSteps.push( | ||
| `await page.setViewport({ width: ${opts.viewport.width}, height: ${opts.viewport.height}, deviceScaleFactor: ${dsf === undefined ? "undefined" : String(dsf)} });`, | ||
| `await page.setViewportSize({ width: ${opts.viewport.width}, height: ${opts.viewport.height} });`, | ||
| ); |
There was a problem hiding this comment.
When reopening an existing non-cmux tab with viewport.scale, this reuse path only calls page.setViewportSize(), while the first-open path applies deviceScaleFactor via applyViewport(). Reusing a tab after it was opened with a different scale (including the default 1.25) leaves the old DPR active, so screenshots, media queries, and coordinate-based interactions run with a viewport scale different from the requested one.
Useful? React with 👍 / 👎.
…Playwright)
Replace puppeteer-core with patchright — a patched, undetected Playwright
drop-in that provides built-in stealth (Runtime.enable avoidance, Console.enable
disable, command-flag leak fixes, closed shadow root support).
Core changes:
- launch.ts: chromium.launchServer() + connect(wsEndpoint) for headless mode,
connectOverCDP(cdpUrl) for Electron/external attach. CDP-based
Emulation.setDeviceMetricsOverride for deviceScaleFactor (DPR) support.
Removed all 14 custom stealth init scripts, patchSourceUrl CDP monkey-patch,
UA override logic, and @puppeteer/browsers dependency.
- registry.ts: BrowserServer|Browser union handle, browserAlive() helper,
connectOverCDP() replaces puppeteer.connect(), browser.isConnected()
replaces browser.connected.
- attach.ts: pickElectronTarget uses browser.contexts().flatMap(ctx => ctx.pages())
instead of browser.targets()/target.type()/target.page().
- tab-worker.ts: page.ariaSnapshot({mode:'ai',boxes:true}) replaces
page.accessibility.snapshot(). New parseAriaSnapshot()/parseAriaLine()
functions parse the YAML-like output into ObservationEntry[] with string
aria-ref IDs (e.g. 'e2'). tab.id(ref) returns page.locator('aria-ref='+ref).
CDP Accessibility.getFullAXTree enriches entries with description and
keyshortcuts (stealth-safe, does not trigger Runtime.enable). All locator
methods updated (.click({timeout}) instead of .setTimeout().click()),
setInputFiles replaces uploadFile, mouse.wheel(dx,dy) takes numbers.
normalizeSelector translates Puppeteer selectors (aria/X, text/X, xpath/X,
pierce/X, p-aria/X) to Playwright equivalents (text=X, xpath=, bare CSS).
WebP screenshot saves use resizeImage encoding since Playwright can't
capture webp natively.
- tab-supervisor.ts: buildInitPayload passes WS endpoint (headless) or CDP
URL (attach). pageTargetId() from launch.ts replaces internal _targetId
access. closeOrphanTarget uses context-based page discovery.
- tab-protocol.ts: WorkerInitPayload uses 'endpoint' instead of
'browserWSEndpoint'. ObservationEntry.id changed from number to string.
Observation.viewport removed deviceScaleFactor (DPR set via CDP instead).
- cmux/cmux-tab.ts: ref construction uses @${element.id} (not @e${id})
since ids already have the e prefix. selectorSpec parses eN refs correctly.
- bundle-dist.ts: RUNTIME_EXTERNAL updated from puppeteer-core to
patchright/patchright-core.
Removed:
- puppeteer-core, @puppeteer/browsers dependencies
- packages/coding-agent/src/tools/puppeteer/ (14 stealth scripts)
- browser-stealth-targets.test.ts (stealth functionality removed)
Added: patchright 1.60.2 dependency
H4vC
force-pushed
the
patchright-migration
branch
from
f3a1a0e to
6c4052e
Compare
There was a problem hiding this comment.
💡 Codex Review
Preserve variadic tab.evaluate arguments
Patchright is Playwright-based, and page.evaluate() forwards only one optional argument rather than Puppeteer's variadic ...args. This wrapper still spreads the browser-tool arguments, so documented calls like await tab.evaluate((a, b) => a + b, 1, 2) invoke the page function with only the first value and leave b undefined, breaking existing browser scripts after the migration; pack the arguments into a single value and unpack them in the page callback.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async function closeOrphanTarget(tab: WorkerTabSession): Promise<void> { | ||
| // Playwright: iterate pages across all contexts instead of targets. | ||
| const browser = tab.browser.browser as Browser; | ||
| const pages = browser.contexts().flatMap(ctx => ctx.pages()); |
There was a problem hiding this comment.
In headless mode tab.browser.browser is the BrowserServer returned by chromium.launchServer(), not a connected Browser. When a tab worker must be force-killed after a timeout or failed graceful close, this cleanup path calls browser.contexts() on the server object and throws before releaseBrowser() and tabs.delete() run, leaving the dead tab and browser server registered instead of cleaning up.
Useful? React with 👍 / 👎.
…adic evaluate args - closeOrphanTarget: check typeof browser.contexts before calling it — in headless mode, browser is a BrowserServer (no contexts()), so the orphan cleanup threw before releaseBrowser/tabs.delete could run. Wrapped in try/catch so cleanup never blocks. - tab.evaluate: Playwright's page.evaluate forwards only one optional arg, not variadic spread like Puppeteer's. For 0-1 args, pass directly. For 2+ args, pack into an array and reconstruct the callback inside the page via Function constructor (functions aren't serializable as arg values).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c77b0b0a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ); | ||
| } | ||
| }, | ||
| const child = Bun.spawn(["npx", "patchright", "install", "chromium"], { |
There was a problem hiding this comment.
On standalone binary installs or Bun-only hosts that do not have npm/npx on PATH, this first-use path throws before Patchright Chromium can be installed, so the default browser.open fails whenever no system Chrome and no PUPPETEER_EXECUTABLE_PATH are present. The previous downloader ran in-process, so use a bundled Patchright installer path/API or surface an explicit manual-install diagnostic instead of unconditionally spawning npx.
Useful? React with 👍 / 👎.
| // BrowserServer has no isConnected(); check process instead. | ||
| if ("isConnected" in browser && typeof browser.isConnected === "function") return browser.isConnected(); | ||
| const proc = "process" in browser && typeof browser.process === "function" ? browser.process() : null; | ||
| return proc !== null && !proc.killed; |
There was a problem hiding this comment.
When headless Chromium exits or crashes on its own, the child process's killed flag remains false unless this process sent a kill signal, so acquireBrowser can keep reusing a dead BrowserServer and hand out a stale wsEndpoint() instead of launching a replacement. Check process exit state (for example exitCode/signalCode) as well so the registry evicts crashed headless browsers.
Useful? React with 👍 / 👎.
…owserServer - ensureChromiumExecutable: replace npx-only provisioning with multi-fallback strategy: try npx first (npm installs), then node + patchright CLI module (binary installs with node available), then actionable error. Works on standalone binary and Bun-only hosts that lack npx. - browserAlive: check exitCode/signalCode in addition to killed flag. A BrowserServer child process that exited or crashed on its own has killed===false, causing acquireBrowser to reuse a dead server and hand out a stale wsEndpoint. Now correctly evicts crashed headless browsers.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24dae92ca8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| import stealthCodecsScript from "../puppeteer/12_stealth_codecs.txt" with { type: "text" }; | ||
| import stealthWorkerScript from "../puppeteer/13_stealth_worker.txt" with { type: "text" }; | ||
| import { $which, logger } from "@oh-my-pi/pi-utils"; | ||
| import { type Browser, type BrowserServer, type CDPSession, chromium, type Page } from "patchright"; |
There was a problem hiding this comment.
In normal sessions tools/index.ts statically imports BrowserTool, which reaches registry.ts and then this module, so this value import executes Patchright before the browser tool is ever used. That conflicts with the packaging contract in bundle-dist.ts that treats patchright, patchright-core, and chromium-bidi as heavy runtime externals; a missing/broken Patchright subtree in an npm or compiled-binary install can now crash omp at startup instead of producing a browser-tool error on first use. Keep the top level type-only and lazy-load chromium inside the browser launch/connect paths.
Useful? React with 👍 / 👎.
| const child = Bun.spawn(["npx", "patchright", "install", "chromium"], { | ||
| stdout: "inherit", | ||
| stderr: "inherit", | ||
| }); |
There was a problem hiding this comment.
The new fallback still does not run on hosts where npx is not on PATH: Bun.spawn(["npx", ...]) throws synchronously before child.exited is available, so the later node strategy and the actionable ToolError are skipped. This leaves standalone/Bun-only installs with no system Chrome failing first browser use with a raw executable-not-found error even though this function is meant to support that environment.
Useful? React with 👍 / 👎.
| const dpr = vp.deviceScaleFactor ?? DEFAULT_VIEWPORT.deviceScaleFactor; | ||
| if (dpr !== 1) { |
There was a problem hiding this comment.
When an existing tab was previously opened with a non-1 viewport.scale, reopening it with scale: 1 only calls setViewportSize() and skips the CDP metrics override, so Chromium keeps the old deviceScaleFactor. In that reuse path, screenshots, media queries, and coordinate-based clicks continue running at the prior DPR despite the caller explicitly requesting scale 1; send the override for scale 1 as well, or clear the device metrics override.
Useful? React with 👍 / 👎.
…euse P1: Lazy-load Patchright outside the startup graph launch.ts imported chromium as a value at module top level, executing patchright-core (Node-version guard + heavy coreBundle with chromium-bidi requires) during omp startup before the browser tool is ever used. A missing/broken patchright subtree in an npm or compiled-binary install would crash omp at startup instead of producing a browser-tool error on first use. Now a type-only import at top level; chromium is loaded lazily via require() inside launch/connect paths only. Verified: 0 patchright modules loaded when importing tools/index.ts. P2: Catch missing npx before trying fallbacks Bun.spawn(['npx', ...]) throws synchronously if npx is not on PATH, before child.exited is available, so the node fallback strategy and actionable ToolError were skipped. Wrapped both spawn strategies in try/catch so standalone/Bun-only hosts without npx fall through to the node strategy, then to the actionable error. P3: Reset DPR when reopening with scale 1 When an existing tab was previously opened with a non-1 viewport.scale, reopening it with scale: 1 only called setViewportSize() and skipped the CDP Emulation.setDeviceMetricsOverride, so Chromium kept the old deviceScaleFactor. Now always sends the override on the reuse path, even when dpr === 1, to clear any prior non-1 override.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc0488a556
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (this.#dialogHandler && page && !page.isClosed()) page.off("dialog", this.#dialogHandler); | ||
| if (this.#mode === "headless" && page && !page.isClosed()) await page.close().catch(() => undefined); | ||
| if (this.#browser?.connected) this.#browser.disconnect(); | ||
| if (this.#browser?.isConnected()) await this.#browser.close().catch(() => undefined); |
There was a problem hiding this comment.
When the tab was opened via app.path or app.cdp_url, this worker connection was created with connectOverCDP in #init; calling browser.close() here replaces the old Puppeteer disconnect() behavior. Playwright closes/clears connected browser contexts on close(), so a normal browser.close action without kill can tear down the user's attached app/pages; the registry dispose path has the same close-vs-disconnect problem, so attach/spawned cleanup needs disconnect semantics while headless still closes the server/page.
Useful? React with 👍 / 👎.
| const useResizedForSave = pathFormat === "webp"; | ||
| const savedBuffer = saveFullRes && !useResizedForSave ? buffer : resized.buffer; | ||
| const savedMimeType = saveFullRes && !useResizedForSave ? captureMime : resized.mimeType; |
There was a problem hiding this comment.
The fresh evidence in this version is that resizeImage still picks the smallest PNG/JPEG/WebP candidate (and can fast-path the original PNG), rather than honoring pathFormat. For save: "shot.webp", this branch writes resized.buffer to the explicit .webp path, so small screenshots or PNG-compressible UI can still produce a .webp file containing PNG/JPEG bytes; use an explicit WebP encode for the saved buffer or reject/rename when the encoder does not return WebP.
Useful? React with 👍 / 👎.
…saves P2: Disconnect attached browsers instead of closing them tab-worker.ts #close(): browser.close() was called unconditionally for all modes. For attach mode (connectOverCDP via app.path/app.cdp_url), Playwright's Browser has no disconnect(), and close() can tear down contexts/pages on the user's attached app. Now only calls browser.close() for headless mode; attach mode skips it — the CDP connection is cleaned up when the worker process exits. registry.ts disposeBrowserHandle: connected/spawned kinds no longer call browser.close() — same disconnect semantics. The CDP connection drops when the worker exits; kill is handled by gracefulKillTreeOnce. P2: Encode explicit WebP saves with a WebP-specific path resizeImage picks the smallest of PNG/JPEG/WebP and can fast-path the original PNG, so saving to 'shot.webp' could write PNG/JPEG bytes to a .webp file. Now when pathFormat === 'webp', explicitly encodes the saved buffer as WebP via Bun.Image.webp() so file bytes always match the extension.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32c959c232
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (pathFormat === "webp") { | ||
| // Explicit WebP encode for .webp save paths — don't let resizeImage's | ||
| // "pick smallest" fallback write PNG/JPEG bytes to a .webp file. | ||
| const webpBytes = await new Bun.Image(buffer).resize(1024, 1024).webp({ quality: 80 }).bytes(); |
There was a problem hiding this comment.
When a caller saves a non-square page or selector screenshot as *.webp, this branch encodes new Bun.Image(buffer).resize(1024, 1024), which upscales/squashes the captured pixels to a square instead of using the original dimensions or the aspect-preserving dimensions computed by resizeImage. This makes saved WebP artifacts visibly distorted, and the returned ScreenshotResult can report dimensions that do not match the file that was written.
Useful? React with 👍 / 👎.
| const firstBracket = rest.indexOf("["); | ||
| const roleAndName = (firstBracket >= 0 ? rest.slice(0, firstBracket) : rest).trim(); |
There was a problem hiding this comment.
For an accessible name containing brackets, such as Docs [beta], the aria snapshot still puts the bracketed text inside the quoted name before the real [ref=...] attributes. Splitting the role/name at the first [ in the whole line truncates the name before parsing, so tab.observe() returns missing or wrong names for controls with bracketed labels and agents cannot reliably find those observed elements by name.
Useful? React with 👍 / 👎.
… names
P2: Preserve aspect ratio for WebP saves
The WebP encode path called resize(1024, 1024), which forces a square
and squashes non-square page/selector screenshots. Now encodes at the
original dimensions (matching the non-webp full-res save path), so the
saved WebP file preserves the correct aspect ratio. ScreenshotResult
also reports originalWidth/originalHeight for the webp path instead of
the resized dimensions.
P2: Ignore quoted brackets while parsing aria lines
parseAriaLine extracted all [bracket] pairs from the full line, then
used indexOf('[') to find the role/name boundary. For accessible names
containing brackets (e.g. 'button "Docs [beta]"'), the [ inside the
quoted name was treated as an attribute bracket, truncating the name to
'Docs ' before the real [ref=eN] attributes. Now extracts the quoted
name first via regex, then parses attributes only from the text after
the closing quote — brackets inside quoted names are preserved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b37229e6e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - `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 + Playwright engines `text=Sign in`, `xpath=//a`, `role=button`; legacy `aria/X`, `text/X`, `xpath/X`, `pierce/X`, `p-aria/X`, `p-text/X` auto-translated. |
There was a problem hiding this comment.
When the default browser path uses cmux (browser.cmux with CMUX_SOCKET_PATH), this prompt now tells the agent to use text=..., xpath=..., and role=..., but the cmux selector parser still only recognizes slash-prefixed handlers (text/..., aria/..., xpath/...) and otherwise treats text=Continue as a CSS/native selector. That makes the documented selector form fail or time out in cmux sessions even though it works in the Patchright worker path; either add these engines to CmuxTab.#selectorSpec or avoid advertising them globally.
Useful? React with 👍 / 👎.
| if (description || keyshortcuts) { | ||
| cdpQueue.push({ role, name, description, keyshortcuts }); | ||
| } |
There was a problem hiding this comment.
When a page has duplicate controls with the same role/name and only a later duplicate has aria-description or aria-keyshortcuts, this queue drops the earlier CDP nodes without metadata, so the sequential matcher can apply the later node's metadata to the first observed entry. That corrupts tab.observe() output and can make the agent choose the wrong duplicate element; keep placeholder CDP nodes in order or match by a stable ref/DOM identity before enriching.
Useful? React with 👍 / 👎.
|
Posting as a standalone comment because the staged inline-review batch hit a GitHub 422 on line anchors that fell outside the diff hunks and the harness can't clear them; consolidating the findings here. Thanks @H4vC for taking this on — the patchright drop-in is a clean win on lines-of-code. Verdict: ─ Blocking1. Agent prompt is out of sync with the new API and now actively misleads the model — The prompt still describes a puppeteer surface, but the implementation switched to patchright/Playwright with breaking contract changes that aren't documented here:
Concrete failure mode: a model following the prompt today writes Please update the prompt to (a) drop the "puppeteer" wording, (b) state ─ Should-fix2. Explicit const webpBytes = await new Bun.Image(buffer).resize(1024, 1024).webp({ quality: 80 }).bytes();
3. Child stdio inherits the TUI's stdout/stderr during Chromium install — const child = Bun.spawn(["npx", "patchright", "install", "chromium"], {
stdout: "inherit", stderr: "inherit",
});
4. Puppeteer's 5. Eager-startup regression test has silently rotted — The test still greps for 6. No CHANGELOG entry — Per ─ Nit7. Load-bearing claim with no automated coverage — This PR deletes ─ Open question for the maintainerIs the API break to |
…nrichment duplicates P2: Support text=/xpath=/role= selectors in cmux The browser prompt advertises Playwright engine= syntax (text=Sign in, xpath=//a, role=button) globally, but the cmux #selectorSpec only recognized slash-prefixed legacy forms (text/X, aria/X, xpath/X). text=Continue fell through to kind:'css' and was treated as a CSS selector, causing failures/timeouts in cmux sessions. Now adds engine= parsing: text=X→kind:'text', xpath=X→kind:'xpath', role=button[name="X"]→kind:'aria' with accessible name extracted. P2: Keep all CDP nodes in enrichment queue to preserve DOM order enrichWithCdpAxMetadata only queued CDP nodes that had description or keyshortcuts, so when duplicate controls with the same role/name existed, the sequential matcher would skip earlier duplicates (no queue entry) and apply a later duplicate's metadata to the first observed entry. Now queues ALL CDP nodes (with metadata as optional fields), so the sequential match respects DOM order across duplicates and only enriches when the matched CDP node actually has metadata.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e23d62d90
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (prefix === "role") { | ||
| // role=button[name="Save"] → extract accessible name if present | ||
| const nameMatch = value.match(/\[\s*name\s*=\s*(?:"([^"]+)"|'([^']+)'|([^\]]+))\s*\]/); | ||
| const name = nameMatch?.[1] ?? nameMatch?.[2] ?? nameMatch?.[3]; | ||
| return { kind: "aria", value: (name ?? value).trim(), raw, name: (name ?? value).trim() }; |
There was a problem hiding this comment.
In cmux sessions this fresh role= parser converts role=button into an aria/name lookup with name button and no role, so the advertised selector fails for ordinary controls like <button>Continue</button> whose accessible name is not “button”; role=button[name="Continue"] also does not enforce the button role. Carry the parsed role separately into the selector spec (and match implicit roles) or avoid advertising role selectors for cmux.
Useful? React with 👍 / 👎.
| const child = Bun.spawn(["npx", "patchright", "install", "chromium"], { | ||
| stdout: "inherit", | ||
| stderr: "inherit", |
There was a problem hiding this comment.
When first browser use has to install Chromium, this runs npx patchright install chromium with stdout/stderr inherited, so npm prompts and download progress write directly into the interactive terminal instead of going through the browser tool output/logger; in coding-agent TUI sessions that corrupts rendering. Capture the child output and surface a sanitized status/log message instead.
Useful? React with 👍 / 👎.
…r, rule compliance Blocking can1357#1: Agent prompt out of sync with new API browser.md still described puppeteer surface (full puppeteer access, ElementHandle return, HTTPResponse type, numeric id). Updated to reflect patchright/Playwright: string ids ("e2"), Locator return type, Playwright Response, built-in stealth wording. Blocking can1357#4: aria/ semantics narrowed to aria-label-only normalizeSelector rewrote aria/X to [aria-label="X"] which only matched the literal attribute, not visible text or title. Now uses Playwright's role= engine: aria/role[name="X"] → role=role[name="X"], aria/X (no role) → text=X fallback. Matches the computed accessible name like Puppeteer's aria/ handler did. Should-fix can1357#3: Chromium install stdio corrupts TUI npx/node spawn used stdout/stderr: "inherit", writing download progress directly into the interactive terminal. Now pipes stdout/stderr, logs failures via logger.warn, and embeds stderr in the ToolError. Should-fix can1357#5: Startup import graph test rotted Test still grepped for puppeteer-core|@puppeteer/browsers, which are gone after the migration — passed vacuously. Updated regex to ^import\s+(?!type\b)...from ["'](patchright|patchright-core|chromium-bidi). Should-fix can1357#6: CHANGELOG entry Added Unreleased entry documenting the migration: dependency swap, stealth removal, id format change (number→string), Locator return, selector semantics, lazy loading, Chromium provisioning. P2: cmux role= selector carries role into SelectorSpec role=button was converted to kind:'aria' with name:'button' (treating the role as the accessible name), so role=button matched elements whose accessible name included 'button', not actual buttons. Now parses the role separately into spec.role. findElement checks both explicit role attributes and implicit ARIA roles via new implicitRole() helper. role=button (no name) matches any visible element with that role; role=button[name="Continue"] matches by both role and accessible name. Rule compliance: launch.ts lazy loader Replaced typeof import("patchright").chromium inline type annotations with top-level import type { BrowserType } from "patchright". Added comment documenting the ts-no-dynamic-import exception (static import would trigger the heavy module side effect we need to defer).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8830a65b67
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } | ||
| const child = Bun.spawn(["npx", "patchright", "install", "chromium"], { | ||
| stdout: "pipe", |
There was a problem hiding this comment.
When first browser use has to install Chromium, both installer strategies create a stdout pipe but never read it (only stderr is consumed before awaiting exit). If npx patchright install chromium or the node fallback writes download/progress output to stdout, the pipe can fill and block the child, leaving browser.open hung while waiting for stderr EOF instead of completing the install or trying the fallback; drain stdout as well or set it to ignore.
Useful? React with 👍 / 👎.
| // visible text, title). Use Playwright's role= engine with [name="..."] which | ||
| // also matches the accessible name. When no explicit role was given (aria/X | ||
| // with just a name), use a generic role=* match by omitting the role. | ||
| const roleMatch = rest.match(/^\s*(\w+)\s*(?:\[|$)/); |
There was a problem hiding this comment.
For a legacy selector whose accessible name is a single word, such as aria/Save or p-aria/Submit, this regex treats that word as a role because it is followed by end-of-string, so normalizeSelector emits role=Save[name="Save"] instead of a name lookup. Those documented selectors now fail for common one-word buttons/links; only infer a role when the syntax actually includes a role plus attributes (for example button[...]).
Useful? React with 👍 / 👎.
| if (role && element.getAttribute("role") !== role && implicitRole(element) !== role) return false; | ||
| if (!wanted) return true; |
There was a problem hiding this comment.
In cmux mode, role=... and legacy aria/... selectors now search all elements without the old isVisible guard. On pages with hidden/template controls before the visible one, tab.waitFor, tab.click, or tab.fill can resolve the hidden node and either fire a synthetic click on the wrong element or report success before the usable control is available; keep the visibility check before accepting aria/role matches.
Useful? React with 👍 / 👎.
…ux visibility guard Bug 1: Drain stdout pipe during Chromium install Both npx and node spawn strategies created a stdout pipe but only consumed stderr (via new Response(child.stderr).text()). If the child wrote download progress to stdout, the pipe buffer could fill and block the child on write, leaving browser.open hung waiting for stderr EOF. Now drains both stdout and stderr concurrently via Promise.all. Bug 2: aria/ bare word misidentified as role The roleMatch regex /^(\w+)\s*(?:\[|$)/ matched any word followed by end-of-string, so aria/Save was treated as role=Save[name="Save"] instead of a text=Save lookup. Now only infers a role when the word is followed by [name= (i.e., role[name="..."] syntax). Bare words like "Save" or "Submit" correctly fall through to text=. Bug 3: cmux findElement aria branch missing isVisible guard The role=/aria/ findElement branch lost the isVisible(element) check, so hidden/template controls could be matched before the visible one. On pages with hidden duplicates, tab.waitFor/tab.click/tab.fill could resolve the hidden node and fire actions on the wrong element. Restored isVisible as the first check in the aria match predicate.
…x waitFor return type
Bug 1: macOS app-bundle executables rejected by native pipe validation
validate_patchright_launch only allowlisted chrome/chromium/msedge-style
basenames. On macOS, system Chrome is at /Applications/Google Chrome.app/
Contents/MacOS/Google Chrome — basename 'Google Chrome', not in the list.
Added macOS app-bundle names: 'google chrome', 'google chrome beta',
'google chrome canary', 'microsoft edge', plus linux package names
'google-chrome-beta', 'google-chrome-canary'.
Bug 2: aria snapshot values with brackets truncated
parseAriaLine used lastIndexOf(']') to find the end of metadata, so a
value like 'Docs [beta]' (textbox "Title" [ref=e1]: Docs [beta]) had
its brackets treated as attribute delimiters and the value was lost.
Now finds the ': ' value separator first, parses attributes only from
the text before it, and preserves the full value including brackets.
Bug 3: cmux waitFor return type mismatch
Prompt said tab.waitFor() returns Locator, but cmux returns
CmuxElementHandle which lacks Locator-only APIs (count(), filter(),
chained locators). Qualified the prompt to state both return types.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3d82396e5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (process.versions.bun) { | ||
| return await chromium().launch(launchOptions); |
There was a problem hiding this comment.
When running under the normal Bun CLI, this branch returns a Browser from launch() instead of a BrowserServer; buildInitPayload() then marks the tab as headlessDirect, and acquireTabImpl() deliberately uses the inline worker for that mode. That means the default headless browser.run path now executes user JS on the main thread, so a synchronous loop or CPU-bound browser snippet cannot be interrupted by the browser timeout and can hang the whole agent/TUI; keep the default path on a worker-connectable browser server (or another isolated worker path) even under Bun.
Useful? React with 👍 / 👎.
| | "google chrome" | ||
| | "google chrome beta" | ||
| | "google chrome canary" | ||
| | "microsoft edge" |
There was a problem hiding this comment.
If the only detected macOS system browser is Google Chrome Dev, systemChromiumCandidates() can return .../Google Chrome Dev.app/Contents/MacOS/Google Chrome Dev, but this allowlist still rejects that basename before Chromium starts. The fresh evidence is that the current allowlist added the other app-bundle names but still omits google chrome dev, so default browser.open fails on that supported candidate.
Useful? React with 👍 / 👎.
| if (role) { | ||
| return `role=${role}[name="${accessibleName}"]`; |
There was a problem hiding this comment.
For explicit aria selectors whose accessible name contains a double quote or backslash, such as aria/button[name='Save "draft"'], this interpolation emits an invalid Playwright selector like role=button[name="Save "draft""]. Those selectors used to be valid Puppeteer aria queries, so quote the name for Playwright selector syntax instead of inserting it raw.
Useful? React with 👍 / 👎.
…ome Dev, escape aria names, remove .build artifacts Bug 1: headlessDirect inline worker runs user JS on main thread Under Bun CLI, launchHeadlessBrowser used chromium.launch() (returns Browser, no wsEndpoint) instead of launchServer() (returns BrowserServer). buildInitPayload then marked the tab as headlessDirect, which uses the inline worker — running user browser JS on the main thread where a synchronous loop or CPU-bound snippet can't be interrupted by the browser timeout and hangs the whole agent/TUI. Now always uses launchServer() so workers connect via wsEndpoint() in an isolated Bun Worker. Removed headlessDirect mode entirely from WorkerInitPayload, tab-worker init, and acquireTabImpl. Bug 2: Google Chrome Dev rejected by macOS allowlist Added 'google chrome dev' to the native pipe validation allowlist. Bug 3: aria names with quotes produce invalid Playwright selectors role=button[name="Save "draft""] was emitted for accessible names containing double quotes. Now escapes \ and " in the interpolated name for Playwright selector syntax. Cleanup: Remove 3,200 lines of generated .d.ts build artifacts packages/natives/native/.build/ was accidentally committed. Added to .gitignore and removed from tracking.
…x Chrome Dev, escape aria quotes Simplification pass: trimmed ~60 lines of verbose comments across launch.ts, tab-worker.ts, and tab-supervisor.ts. Removed redundant multi-line explanations where a single-line comment suffices. Bug fixes (from previous uncommitted edits): - Always use launchServer() so workers connect via wsEndpoint() in an isolated Bun Worker (removed headlessDirect inline worker path) - Added 'google chrome dev' to macOS allowlist in browser_pipe.rs - Escape \ and " in aria names for Playwright [name="..."] syntax
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98e200ae3c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let mut guard = inner.child.lock().ok()?; | ||
| let mut child = guard.take()?; | ||
| let status = child.wait().ok()?; |
There was a problem hiding this comment.
On Unix, NativeBrowserChildProcess registers onExit as soon as the wrapper is constructed, so this wait thread immediately locks inner.child and keeps the mutex through child.wait(). If a later browser shutdown or timeout path needs kill(), it tries to lock the same mutex and blocks until Chromium exits on its own, so forced cleanup of a Patchright browser can hang instead of terminating the process.
Useful? React with 👍 / 👎.
| // No role prefix — just match by accessible name via text= as a fallback. | ||
| return `text=${accessibleName}`; |
There was a problem hiding this comment.
For bare legacy ARIA selectors such as aria/Close against an icon button whose accessible name comes from aria-label rather than visible text, this converts the lookup to text=Close. Playwright text selectors do not match aria-label, so those selectors time out in the headless backend even though the previous Puppeteer ARIA query and the prompt's aria/ contract match accessible names; keep this on an ARIA/role lookup instead of visible text.
Useful? React with 👍 / 👎.
…l deadlock Two bugs from PR review: 1. Bare `aria/Name` selectors (e.g. `aria/Close`) against icon buttons whose accessible name comes from `aria-label` were converted to Playwright `text=Name`, which only matches visible text content. These selectors silently timed out in the headless backend. Fix: register a custom `aria=` selector engine (at first browser launch) that computes the accessible name from aria-label, aria-labelledby, title, visible text, and input value — the same sources Puppeteer's aria query handler used. `normalizeSelector` now maps bare `aria/Name` → `aria=Name` instead of `text=Name`. Both `text=` and `aria=` selectors route through `clickQueryHandler` for viewport-aware candidate selection. 2. On Unix, `PatchrightPipeProcess.kill()` deadlocked with the exit-wait thread. The wait thread held `inner.child`'s mutex through `child.wait()`, blocking `kill()` from acquiring the lock — forced browser cleanup hung instead of terminating Chromium. Fix: `kill()` sends `SIGTERM` directly via `pid` (no lock); `wait_exit()` takes the child out of the mutex before waiting. Also consolidates write/close_stdin #[cfg] branches into platform Inner methods, trimming ~65 lines of duplicated lock boilerplate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25ef983893
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| var lb=el.getAttribute&&el.getAttribute("aria-labelledby"); | ||
| if(lb){var p=lb.split(/\\s+/).map(function(id){var e=document.getElementById(id);return e&&e.textContent&&e.textContent.trim()}).filter(Boolean);if(p.length)return p.join(" ")} | ||
| var t=el.getAttribute&&el.getAttribute("title");if(t)return t; | ||
| var tx=el.textContent&&el.textContent.trim();if(tx)return tx; |
There was a problem hiding this comment.
For form controls whose accessible name comes from a <label> or wrapper label, such as <label for="email">Email</label><input id="email">, this custom aria= engine falls back to each element's own text and never computes label ownership. tab.fill("aria/Email", ...) can therefore resolve the <label> rather than the input and fail, whereas the previous Puppeteer ARIA query used the accessibility tree and matched the control; handle associated labels or use an AX-backed accessible-name computation before returning matches.
Useful? React with 👍 / 👎.
| "--external", | ||
| "chromium-bidi", |
There was a problem hiding this comment.
This flag is added only to the local/package binary builder. The release binaries are built by scripts/ci-release-build-binaries.ts with a separate bun build --compile argv that still lacks --external chromium-bidi; Bun's help defines --external as excluding a module from transpilation, so the release build will not get the same Patchright/Chromium-BiDi workaround that this script needs. Mirror this flag in the release builder or the published binaries can fail or differ from the tested local binary.
Useful? React with 👍 / 👎.
Windows Terminal sends bare \n (LF) for Ctrl+Enter and \r (CR) for plain Enter. Without Kitty keyboard protocol, \n was misidentified as iTerm2's Shift+Enter newline mapping, causing Ctrl+Enter to insert a newline instead of triggering the follow-up message handler. Editor (packages/tui): - On Windows Terminal without Kitty, bare \n no longer matches the newline block. It falls through to submit, where the app's ctrl+enter custom handler intercepts it. - On non-Windows terminals (iTerm2), bare \n still inserts a newline. CustomEditor (packages/coding-agent): - On Windows Terminal without Kitty, \n is remapped from 'enter' to 'ctrl+enter' in the canonical key id, so the app.message.followUp custom handler fires before reaching submit. Tests: - Added test verifying \n submits on Windows Terminal without Kitty - Added test verifying \n inserts newline on non-Windows terminals - Fixed umlaut line-break test to use insertText instead of handleInput This is a known Windows Terminal limitation: without Kitty keyboard protocol (not yet shipped by Microsoft), Shift+Enter is indistinguishable from plain Enter (both send \r). Users who need Shift+Enter for newlines should configure Windows Terminal keybindings to send \x1b[13;2u.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd8aea1252
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (_ariaEngineRegistered || !_chromium) return; | ||
| _ariaEngineRegistered = true; | ||
| const { selectors } = require("patchright") as typeof import("patchright"); | ||
| void selectors.register("aria", ARIA_SELECTOR_ENGINE_SOURCE, { contentScript: true }).catch(err => { |
There was a problem hiding this comment.
When the first browser operation immediately uses a legacy aria/... selector, this starts registering the custom aria= engine but returns before the registration promise settles, while normalizeSelector can already emit aria=... for tab.click, tab.fill, or tab.waitFor. In fast connectOverCDP/attach paths that can race into Playwright with an unknown or not-yet-installed selector engine; cache and await the registration before returning from the launch/connect path (and don't mark it registered until it succeeds).
Useful? React with 👍 / 👎.
| this.exitCode = exit.exitCode ?? null; | ||
| this.stdout.close(); | ||
| this.stderr.close(); | ||
| cdpRead.close(); | ||
| this.emit("exit", this.exitCode, this.signalCode); | ||
| this.emit("close", this.exitCode, this.signalCode); |
There was a problem hiding this comment.
If Chromium exits due to a signal under the new Bun pipe wrapper (for example SIGSEGV or an external SIGKILL), the native side returns no exit code and this wrapper leaves both exitCode and signalCode as null before emitting close. The fresh evidence is that registry.browserAlive() now depends on process.exitCode/signalCode to evict dead BrowserServers, so a signaled crash can still be treated as alive and reused with a stale endpoint; propagate the terminating signal or otherwise mark signaled exits as not alive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eaf5e1c0a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| fn pipe() -> Result<(OwnedFd, OwnedFd)> { | ||
| let mut fds = [0; 2]; | ||
| if unsafe { libc::pipe(fds.as_mut_ptr()) } != 0 { |
There was a problem hiding this comment.
On Unix, these CDP pipes are created without FD_CLOEXEC, and the pre_exec hook only dup2s the child ends to fds 3/4, so Chromium also inherits the parent pipe ends and the original child descriptors. When Playwright closes stdio[3] during BrowserServer.close()/disconnect, the browser process still owns a writer for that pipe, so EOF/EPIPE is never delivered and shutdown can hang or leave the Patchright transport half-open; create the pipes with O_CLOEXEC and only clear it on fds 3/4, or close the extra ends in pre_exec.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfa67cab60
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| kill(_signal?: NodeJS.Signals | number): boolean { | ||
| if (this.killed) return false; | ||
| this.killed = true; | ||
| this.process.kill(); |
There was a problem hiding this comment.
When a caller requests a hard cleanup such as kill('SIGKILL'), this wrapper discards the requested signal, marks the process as killed, and calls the native kill() with no signal; on Unix that native path sends only SIGTERM. If Chromium does not exit on TERM, any later escalation is suppressed by this.killed, so browser shutdown can hang or leave the process running. Forward the requested signal to the native layer and allow escalation attempts after a soft kill.
Useful? React with 👍 / 👎.
| ) || command.contains("\\patchright\\") | ||
| || command.contains("/patchright/") | ||
| || command.contains("ms-playwright"); |
There was a problem hiding this comment.
On Linux systems where systemChromiumCandidates() selects the Flatpak launchers (/var/lib/flatpak/exports/bin/com.google.Chrome or org.chromium.Chromium), exe_name becomes com.google.chrome or org.chromium.chromium. This validation does not accept either basename and those paths do not contain /patchright/ or ms-playwright, so the native pipe spawn rejects a supported detected browser before Chromium starts; add these basenames or validate against the known candidate paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2046cb8381
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!browserAlive(handle.browser)) return; | ||
| try { | ||
| handle.browser.disconnect(); | ||
| await (handle.browser as Browser | BrowserServer).close(); |
There was a problem hiding this comment.
When browser.open attaches via app.cdp_url (or reuses a spawned CDP app), releasing the last tab now reaches closeIfAlive, which calls Playwright browser.close(). Playwright documents close() for connected browsers as clearing the connected contexts before disconnecting, so a normal browser.close without kill can close the user's externally owned app windows/pages; the old Puppeteer path used disconnect semantics for these browser kinds. Keep close() limited to headless/owned servers and use a disconnect-only cleanup for attached CDP handles.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0c66ca186
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function labels(el,doc){var out=[];if(el.labels){for(var i=0;i<el.labels.length;i++){var t=txt(el.labels[i]);if(t)out.push(t)}}else{var id=el.getAttribute&&el.getAttribute("id");if(id){doc.querySelectorAll('label[for="'+esc(id)+'"]').forEach(function(l){var t=txt(l);if(t)out.push(t)})}var p=el.closest&&el.closest("label");if(p){var pt=txt(p);if(pt)out.push(pt)}}return out.join(" ")} | ||
| function gan(el,doc){var lb=el.getAttribute&&el.getAttribute("aria-labelledby");if(lb){var lbt=byIds(doc,lb);if(lbt)return lbt}var al=el.getAttribute&&el.getAttribute("aria-label");if(al)return al;var lt=labels(el,doc);if(lt)return lt;var t=el.getAttribute&&el.getAttribute("title");if(t)return t;var tx=txt(el);if(tx)return tx;if(el.tagName==="INPUT"&&el.value)return el.value;return""} | ||
| var n=selector.trim();var r=[];var doc=root.ownerDocument||root;var a=root.querySelectorAll("*"); | ||
| for(var i=0;i<a.length;i++){var el=a[i];if(el.tagName==="LABEL"&&(el.control||el.htmlFor||el.getAttribute("for")||el.querySelector("input,textarea,select,button")))continue;if(gan(el,doc)===n)r.push(el)}return r}})`; |
There was a problem hiding this comment.
When a bare legacy ARIA selector such as aria/Save is used on common markup like <div><button>Save</button></div>, this engine returns every element whose fallback text/name equals the selector, including generic ancestors and hidden/template nodes. tab.click can then choose and click the ancestor instead of the button, while tab.fill, tab.type, and tab.waitFor use the locator directly and can strict-violate or wait on a hidden match. The previous Puppeteer ARIA query came from the accessibility tree, so these generic/hidden elements were not candidates; filter to visible, target-like/AX-named elements before pushing matches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7210be5b43
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function byIds(doc,ids){var out=[];ids.split(/\\s+/).forEach(function(id){var e=doc.getElementById(id);var t=txt(e);if(t)out.push(t)});return out.join(" ")} | ||
| function esc(v){return typeof CSS!=="undefined"&&CSS.escape?CSS.escape(v):v.replace(/(["\\\\])/g,"\\\\$1")} | ||
| function labels(el,doc){var out=[];if(el.labels){for(var i=0;i<el.labels.length;i++){var t=txt(el.labels[i]);if(t)out.push(t)}}else{var id=el.getAttribute&&el.getAttribute("id");if(id){doc.querySelectorAll('label[for="'+esc(id)+'"]').forEach(function(l){var t=txt(l);if(t)out.push(t)})}var p=el.closest&&el.closest("label");if(p){var pt=txt(p);if(pt)out.push(pt)}}return out.join(" ")} | ||
| function role(el){var x=(el.getAttribute&&el.getAttribute("role")||"").trim();if(x)return /^(none|presentation)$/.test(x)?null:x.split(/s+/)[0];var t=el.tagName;if(t==="A"||t==="AREA")return el.hasAttribute("href")?"link":null;if(t==="BUTTON")return"button";if(t==="IMG")return el.getAttribute("alt")===""?null:"img";if(/^H[1-6]$/.test(t))return"heading";if(t==="SELECT")return el.multiple||el.size>1?"listbox":"combobox";if(t==="TEXTAREA")return"textbox";if(t==="INPUT"){var ty=(el.getAttribute("type")||"text").toLowerCase();return /^(button|submit|reset|image)$/.test(ty)?"button":ty==="checkbox"?"checkbox":ty==="radio"?"radio":ty==="range"?"slider":ty==="number"?"spinbutton":/^(hidden|password)$/.test(ty)?null:"textbox"}return null} |
There was a problem hiding this comment.
For bare aria/... selectors, the custom engine uses x.split(/s+/) when deciding whether an element with an explicit role is an ARIA candidate. Because that regex matches the literal letter s instead of whitespace, roles that start with s such as switch, slider, searchbox, or spinbutton produce an empty first token; a <div role="switch">Dark mode</div> with its name from text is then excluded and aria/Dark mode no longer matches, unlike the previous accessibility-tree query. Use /\s+/ here so explicit roles remain candidates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f570b7a5da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // macOS app-bundle executables (basename after the last /) | ||
| | "google chrome" | ||
| | "google chrome beta" | ||
| | "google chrome canary" | ||
| | "google chrome dev" |
There was a problem hiding this comment.
When macOS has only the supported Edge app candidate, systemChromiumCandidates() can select /Applications/Microsoft Edge.app/Contents/MacOS/Microsoft Edge, whose basename lowercases to microsoft edge; this allowlist now includes the Chrome app-bundle basenames but still omits that Edge basename, so the native pipe spawn rejects the detected browser before Patchright can launch it. The fresh evidence after the earlier macOS allowlist comment is that the current list added the Chrome variants while still lacking microsoft edge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27ef32cc4f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } | ||
| if (labels.length) return labels.join(" "); | ||
| return (element.getAttribute("alt") || element.getAttribute("title") || textOf(element)).trim(); |
There was a problem hiding this comment.
In cmux sessions, input buttons such as <input type="submit" value="Save"> are classified as button by implicitRole, but this accessible-name fallback never considers element.value. As a result the newly advertised role=button[name="Save"] and legacy aria/Save selectors fail in cmux for common form submit/reset/button controls even though the headless backend and the prompt's role/access-name contract match them; include the value for input button-like types before falling back to text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a155882d6a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * name, including associated form labels (`label[for]` and wrapper labels). | ||
| * Playwright's built-in `role=` engine requires a role; `text=` only matches | ||
| * visible text. This keeps bare `aria/Name` selectors compatible with the old | ||
| * Puppeteer ARIA query behavior for icon buttons and labelled controls. |
There was a problem hiding this comment.
When a legacy bare ARIA selector targets an accessible name whose DOM text is split across lines or nodes (for example <button>Save\n draft</button> or a wrapped <label>), the browser's accessible name is whitespace-collapsed to Save draft, but this custom aria= engine only trim()s the text before comparing it exactly. Those selectors worked through Puppeteer's accessibility query and now time out unless the caller guesses the raw DOM whitespace; normalize internal whitespace before storing/comparing names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c0aad898a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function vis(el){if(el.tagName==="TEMPLATE"||(el.closest&&el.closest("template,[hidden],[aria-hidden='true']")))return false;var s=typeof getComputedStyle==="function"&&getComputedStyle(el);if(s&&(s.visibility==="hidden"||s.display==="none"))return false;var r=el.getClientRects&&el.getClientRects();return !r||r.length>0} | ||
| function named(el){return !!(el.getAttribute&&(el.getAttribute("aria-label")||el.getAttribute("aria-labelledby")||el.getAttribute("title")||el.getAttribute("alt"))||el.labels&&el.labels.length)} | ||
| function cand(el){if(el.tagName==="LABEL"&&(el.control||el.htmlFor||el.getAttribute("for")||el.querySelector("input,textarea,select,button")))return false;return vis(el)&&(!!role(el)||named(el)||el.isContentEditable||el.hasAttribute&&el.hasAttribute("tabindex"))} | ||
| function gan(el,doc){var lb=el.getAttribute&&el.getAttribute("aria-labelledby");if(lb){var lbt=byIds(doc,lb);if(lbt)return lbt}var al=norm(el.getAttribute&&el.getAttribute("aria-label"));if(al)return al;var lt=labels(el,doc);if(lt)return lt;var alt=norm(el.getAttribute&&el.getAttribute("alt"));if(alt)return alt;var t=norm(el.getAttribute&&el.getAttribute("title"));if(t)return t;var tx=txt(el);if(tx)return tx;if(el.tagName==="INPUT"&&el.value)return norm(el.value);return""} |
There was a problem hiding this comment.
When a visible text-like input has no label, cand() still admits it because role() returns textbox, and gan() then treats any INPUT value as the ARIA name. In pages with prefilled fields, selectors like aria/Save can now match a textbox whose current value is Save, even though the value is not the control's accessible name; limit this fallback to button-like inputs (button/submit/reset/image).
Useful? React with 👍 / 👎.
| function registerAriaSelectorEngine(): void { | ||
| if (_ariaEngineRegistered || !_chromium) return; | ||
| _ariaEngineRegistered = true; | ||
| const { selectors } = require("patchright") as typeof import("patchright"); |
There was a problem hiding this comment.
AGENTS.md for this tree says “NEVER use inline imports — no import("pkg").Type in type positions,” but this new type assertion uses typeof import("patchright"). Use a top-level type import for the Patchright namespace instead so the browser startup code follows the repo's documented import contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7d4bd65fa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const npxStderr = await tryInstallPatchrightChromium(["npx", "patchright", "install", "chromium"], "npx"); | ||
| if (npxStderr === undefined) return; | ||
| const nodeStderr = await tryInstallPatchrightChromium( | ||
| ["node", "-e", "require('patchright/lib/program').program.parse(['node','patchright','install','chromium'])"], |
There was a problem hiding this comment.
When no system Chrome is available and omp is run from a project that does not itself depend on patchright (for example a global/npm install or a Bun-only install), these installer commands resolve from the user's cwd instead of this package: npx patchright may fetch/use a different Patchright version, and the node -e require('patchright/lib/program') fallback cannot see the bundled dependency at all. The later fs.existsSync(exe) check still expects this package's chromium().executablePath(), so first browser.open can fail to self-provision Chromium even though Patchright is installed with omp; run the local Patchright CLI/module from the package directory or use a bundled/Bun installer path.
Useful? React with 👍 / 👎.
What
Migrates the browser tool from puppeteer-core to patchright — a patched, undetected Playwright drop-in. Replaces all Puppeteer APIs (launch, connect, accessibility snapshot, element handles) with
Playwright/patchright equivalents. Removes the 14 custom stealth injection scripts and CDP monkey-patches in favor of patchright's built-in stealth (Runtime.enable avoidance, Console.enable disable,
command-flag leak fixes, closed shadow root support).
Why
The old implementation layered custom stealth scripts on top of a stock Puppeteer/Chromium — an approach that's increasingly detected by modern anti-bot systems and adds significant maintenance
surface. Patchright provides production-grade undetected automation out of the box (passes Cloudflare, Datadome, CreepJS, Sannysoft, etc.) as a drop-in Playwright replacement, eliminating the need for
custom injection while gaining Playwright's better DX and active maintenance.
Testing
session → Target.getTargetInfo → Accessibility.getFullAXTree enrichment (description + keyshortcuts, stealth-safe)
viewport dimensions, DPR applied via CDP Emulation.setDeviceMetricsOverride
bun checkpasses