feat: support explicit page targets for browser actions - #28
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🔵 Needs a closer look
The change spans CLI, SDK, browser behavior, tests, and documentation and warrants final human review.
Pull request overview
Adds explicit page-target selection for browser actions and SDK callers while preserving existing default behavior.
Changes:
- Adds
--target-idsupport to action commands. - Adds strict target attachment and validation.
- Adds protocol, CLI, browser, and documentation coverage.
File summaries
| File | Description |
|---|---|
tests/support/mod.rs |
Shared isolated CLI test support |
tests/page_targets.rs |
Deterministic target-selection tests |
tests/page_targets_browser.rs |
Browser popup and lifecycle tests |
src/main.rs |
CLI parsing and action routing |
src/cdp.rs |
Target validation and attachment |
skills/lexmount-browser/SKILL.md |
Multi-tab workflow guidance |
skills/lexmount-browser/references/troubleshooting.md |
Target troubleshooting guidance |
skills/lexmount-browser/references/commands.md |
Command reference updates |
README.md |
User-facing documentation |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add explicit page selection for multi-tab browser sessions:
actionsubcommands accept optional--target-id, before or after the action subcommand.Cdp::connect_to_target(ws_url, page_id).session targets, selects its page ID, and uses it on subsequent commands.This is an additive capability: the existing default selects the first page and cannot promise to follow a newly opened tab. Explicit target selection makes that choice independent of target enumeration order.
Example
Compatibility and scope
Cargo.toml, the rootCargo.lockpackage entry, and both platform bootstrap pins. No dependencies or installer behavior change beyond the pin.action --help; published 1.1.15 does not support the feature. Updating a Skill does not replace its existing executable. The pinned 1.2.0 assets must be published before installation/upgrade can succeed.Local verification
Verified on Windows with isolated loopback HTTP/CDP fixtures and a separate headless Chromium profile:
cargo fmt --all -- --checkcargo test --all-targets --locked --offline— 27 passed; optional browser test ignored in the default suite.cargo clippy --all-targets --locked --offline -- -D warningscargo test --doc --locked --offlinecargo test --locked --offline --test page_targets_browser -- --ignored --nocapturewithBROWSER_CLI_TEST_CHROME— 1 passed, HeadlessChrome 145.0.7632.6 / CDP 1.3.action --helpexposes--target-id, and Cargo/lock/both bootstrap versions agree. Dependency entries and the remaining installer logic are unchanged.Tests cover all action routes, reversed target order, invalid targets, target closure between listing/attachment, legacy defaults, output/file invariants, actual popup discovery, three independent CLI reconnects, unrelated tab lifecycle, result-page mutation, and closed-target failure without touching the source page.
macOS/Linux runtime and real cloud/WorkBuddy end-to-end behavior have not been locally verified. Existing CI can run the platform/build checks; this PR does not release the feature.
Self-review before this update
The implementer reviewed the complete feature diff and the version-update increment
before pushing: all action routes, default compatibility, missing/non-page/closed
targets, attachment races, fixture isolation, package/lock/pin consistency, and
release-versus-installation guidance. No blocking findings remain. Existing
capability checks are preserved, and the documentation explicitly distinguishes
Skill updates from executable upgrades and source merges from published assets.
This is self-review, not a claim of independent approval or cloud/Harness validation.
CI baseline
This branch now includes main's separately reviewed fix #29. The earlier
bootstrap failure
was caused by blindly selecting a missing historical release, not the page-target
feature. The CI workflow and selector match the updated main; no checks are skipped.
The bootstrap smoke test checks the newest usable already-published COS release,
while Rust tests/builds exercise this branch's 1.2.0 source. A green bootstrap check
does not claim that 1.2.0 has been published. The current results are shown in the
PR checks; publication and test-machine upgrades are outside this update.