fix(cua-driver): target page JavaScript exactly - #2166
Conversation
adef3e8 to
68a42c0
Compare
|
@f-trycua Would you mind reviewing this targeted page-selection change? You are the primary recent author for the affected cua-driver page/CDP paths. I rebased it onto current |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68a42c06bf
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| port: u16, | ||
| target_url_contains: Option<&str>, | ||
| ) -> anyhow::Result<String> { | ||
| let mut session = CdpSession::connect(port, target_url_contains).await?; |
There was a problem hiding this comment.
Reuse the CDP session for targeted evaluation
When execute_javascript is called with an explicit cdp_port for Chrome's in-browser remote-debugging toggle, this creates a new WebSocket for every evaluation. The CdpSessionCache in this same module exists because that endpoint prompts on each new connection, and insert_text/type_keystrokes already reuse it; targeted execute_javascript will therefore reprompt or stall automation on repeated calls instead of reusing the approved session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5c2b402d. Explicit targeted execute_javascript now goes through the existing ToolState.cdp_sessions cache, with the same stale-session eviction and one retry used by insert_text / type_keystrokes. The original untargeted Electron inspector path remains one-shot.
I added targeted_evaluate_reuses_browser_websocket, backed by a local fake CDP browser endpoint. Two targeted evaluations perform one HTTP discovery probe and share exactly one browser WebSocket connection while reattaching on that socket. The full macOS CDP test set is now 5/5 and the release check passes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c2b402d3d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.evict(port).await; | ||
| let arc = self.get_or_connect(port, target_url_contains).await?; | ||
| let mut session = arc.lock().await; | ||
| session.ensure_target(port, target_url_contains).await?; | ||
| do_evaluate(&mut session, javascript).await |
There was a problem hiding this comment.
Avoid replaying timed-out JavaScript
For targeted execute_javascript, do_evaluate has already sent the caller's script before it waits for the response, but any error from that wait now falls through to this retry path. If a slow or side-effectful snippet times out, or the socket drops after Chrome accepted Runtime.evaluate, the same JavaScript is executed a second time; the previous execute path surfaced the error without replaying the script. Please limit the retry to connection/retarget failures that happen before sending the user script, or return evaluate errors directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0e9a8294. The cache now retries only when ensure_target fails before Runtime.evaluate is sent. Once the caller's JavaScript is dispatched, do_evaluate returns its success or error directly, so timeouts / response loss / disconnects cannot replay a side-effectful script.
Added targeted_evaluate_does_not_replay_after_disconnect: the fake CDP endpoint closes immediately after receiving Runtime.evaluate, then watches for a reconnect. The call errors and the accepted connection count remains exactly 2 (one HTTP probe + one browser WebSocket), proving the script was sent once. macOS CDP tests are now 6/6; core page tests 3/3 and release check pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e9a8294c8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if cdp_port.is_some() || target_url_contains.is_some() { | ||
| anyhow::bail!( | ||
| "targeted execute_javascript is not implemented on this platform's page backend" | ||
| ); |
There was a problem hiding this comment.
Implement cdp_port for non-macOS execute_javascript
On Windows and Linux, PageTool is registered with WindowsPageBackend/LinuxPageBackend, and those impls still only implement execute_javascript while their CDP paths read CUA_DRIVER_CDP_PORT. Because the new dispatcher always calls execute_javascript_targeted and this default bails whenever cdp_port is present, a call using the newly advertised execute_javascript cdp_port never reaches those CDP fallbacks; it fails with “not implemented” even though the platform can evaluate JS via CDP. Please either override this method in those backends to consume cdp_port or avoid advertising/accepting the option there.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 35fa5658. The shared cua_driver_core::cdp helper now supports unique case-insensitive URL targeting, and both Windows and Linux override execute_javascript_targeted to consume explicit cdp_port / target_url_contains. Calls without explicit targeting still use the existing bookmark/env behavior.
Validation:
- core CDP selection tests: 2/2
- core page tests: 3/3
- macOS CDP tests: 6/6
- macOS release check: passed
cargo check -p platform-windows --target x86_64-pc-windows-gnu: passedcargo check -p platform-linux: passed in a clean Debian bookworm / Rust 1.92 container with X11/Wayland build dependencies.
The local macOS-to-Linux cross-check was initially blocked before our code by missing pkg-config sysroot; the native Linux container check above covers the actual backend.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@f-trycua The PR workflows are currently blocked before job creation: all 8 runs report For reference, I have already run the relevant checks independently: core page 3/3, core CDP 2/2, macOS CDP 6/6, macOS release check, Windows cross-check, and native Linux check in Debian Bookworm / Rust 1.92 all pass. |
|
Thanks @hqhq1025 for the careful implementation, thorough validation, and quick follow-through on the review feedback. I also checked this against our current cua-driver E2E work: the branches merge cleanly, and the combined CDP code preserves both the readiness polling and exact target selection. Merged! |
Salvaged from #2166 Co-authored-by: Haoqing Wang <78337154+hqhq1025@users.noreply.github.com>
Represent Linux Chromium JavaScript-dialog resolution as an explicit foreground capability with a side-effect-free background refusal. Stabilize prepared-browser binding and keep the canonical matrix and public docs aligned. Salvaged from #2166 Co-authored-by: Haoqing Wang <78337154+hqhq1025@users.noreply.github.com>
Represent Linux Chromium JavaScript-dialog resolution as an explicit foreground capability with a side-effect-free background refusal. Stabilize prepared-browser binding and keep the canonical matrix and public docs aligned. Salvaged from #2166 Co-authored-by: Haoqing Wang <78337154+hqhq1025@users.noreply.github.com>
Complete exact browser tab state, typed JavaScript dialogs, file uploads, pointer-action parity, approval-gated downloads, telemetry, documentation, and canonical cross-platform evidence. Salvaged from #2166 Co-authored-by: Haoqing Wang <78337154+hqhq1025@users.noreply.github.com>
Add exact-tab JavaScript dialog capabilities and ref-targeted CDP file input assignment. Dialog state retains only bounded kind and generation; uploads validate direct regular files and omit paths from results.\n\nSalvaged from trycua#2166\nRefs trycua#2240 trycua#2320\n\nCo-authored-by: Haoqing Wang <78337154+hqhq1025@users.noreply.github.com>
Summary
page.execute_javascriptto accept explicitcdp_portandtarget_url_containsWhy
Multi-window Electron processes expose multiple page targets on one CDP port.
execute_javascriptpreviously discarded the caller's resolved page identity and selected the first page target. An explicit URL hint also fell back to the first page when no target matched, so a successful RPC could execute against the wrong renderer.The lower-level CDP session code already supported selecting a target by URL. This change threads that identity through the
pagetool and makes explicit hints fail closed when absent or ambiguous.Validation on latest upstream main
Rebased without conflict onto
trycua/cua@8c921b2b3bf13494724ead4f0a814d80c56a7e8b.The targeted-page commit is
68a42c06bf3fdfa429d65e9c011efa13c2414548; review feedback is addressed by5c2b402d, which reuses the existing CDP session cache, and0e9a8294, which prevents replay after a script has been sent.35fa5658completes explicit targeted routing on Windows and Linux.git range-diffreports it equivalent to the originally tested patch commitadef3e87405986cc82df52ae59aef4c32e08a082.Real multi-window downstream validation
Maka uses cua-driver as the sole page executor. Its real-machine harness runs two independent Electron
BrowserWindowpages on one CDP port and passes the exact port + unique URL hint through cua-driver for pointer actions, element inspection,Input.insertText, and post-action readback.Final batch
semantic-targeting-v5:Per-action results:
page.execute_javascriptpage.execute_javascriptpage.execute_javascriptpage.execute_javascriptpage.execute_javascriptThe downstream repo also passed its full typecheck, build, tests, bundle provenance gate, and final 39/39 real-machine E2E after integrating the patched driver.
Compatibility
Runtime.evaluate; a sent script is never replayed after timeout or disconnect.cdp_portand unique URL hints through the shared CDP helper; calls without explicit targeting retain the existing backend behavior.insert_textandtype_keystrokesnow share the checked CDP port parser, eliminating the previousu16truncation behavior.