Skip to content

feat(cli): list connected browsers when a selector matches nothing - #313

Open
PerryLink wants to merge 2 commits into
Tencent:mainfrom
PerryLink:feat/selector-miss-lists-candidates
Open

PerryLink wants to merge 2 commits into
Tencent:mainfrom
PerryLink:feat/selector-miss-lists-candidates

Conversation

@PerryLink

Copy link
Copy Markdown

What

When session.start is given a --browser selector that matches no connected browser, the daemon returns not_found / requested browser is not connected with no payload, so the caller learns only that the selector was wrong — not what would have been right, and not whether the target is offline or the selector was a typo.

This attaches the same connected-browser snapshot that a sibling error already carries, and renders it the same way.

Output below is copied from the new test's rendered stderr, not hand-written:

error: requested resource does not exist
no online browser matches the requested selector; connected browsers:
  INSTANCE  BROWSER     LABEL     SESSIONS
  alpha     chrome 131  Personal  0
  beta      edge 130    -         1
hint: the session, tab, or browser may have stopped; run `bsk session list` / `bsk browsers` to see current state
details: requested browser is not connected

Why this shape

not_found is one of three outcomes of the same selector resolution, and the only one that carries nothing:

outcome payload CLI rendering
multiple_browsers_online { "browsers": [...] } connected-browsers table
ambiguous label (invalid_params) { "instance_ids": [...] } candidate bullet list
not_found (none) (none)

The RenderExtras hook in cli/session.rs enumerates the first two, so the third reads as a case that was never considered rather than one deliberately left bare. This makes it match the two that already exist, reusing the same snapshot helper (snapshot_status_entries) and the same table renderer.

On bsk browsers

The hint: line already tells the reader to run bsk browsers to see current state, and docs/browser-profiles.md documents the same discovery step, so this adds no new mechanism and does not replace either. What it changes is only that the information arrives at the point of the error instead of one command later — which matters for the two cases that doc calls out, verifying a mapping and dealing with an offline target, because at the moment of this error neither can be done without first running something else. It also makes an interactive typo (alhpa) distinguishable from an offline-but-correct instance, which it currently is not.

The message, the details: line, the hint: line and the ordering contract are unchanged; a new test asserts the summary → extras → hint order explicitly.

Backward compatibility

  • The human-readable message is byte-for-byte unchanged.
  • BrowserNotFound gains a field, so it changes from a unit variant to a struct variant. That is a source-level change for anyone matching on it exhaustively, though this is the CLI binary rather than a published library.
  • With nothing connected there is nothing to suggest, so no data payload is emitted at all and that wire shape is unchanged. A test asserts data.is_none() for exactly that case.

Tests

Four new tests, all green locally:

  • daemon::ipc::selector_miss_payload_tests::browser_selector_miss_lists_connected_candidates — the payload, and that the message is unchanged.
  • daemon::ipc::selector_miss_payload_tests::browser_selector_miss_without_browsers_keeps_message_only_payload — the empty-registry shape.
  • cli::session::i3_tests::selector_miss_extras_render_candidate_table — the rendered table, including the summary → extras → hint order.
  • cli::session::i3_tests::selector_miss_with_empty_candidates_renders_no_extras — additive for existing output.

The daemon-side module is deliberately placed outside that file's #[cfg(all(test, unix))] transport module, so it runs on Windows as well.

Local verification, and a caveat about Windows

cargo fmt --all -- --check                    # clean
cargo test -p bsk --lib --locked              # 341 passed; 2 failed, both pre-existing, see below

Two pre-existing failures and four pre-existing clippy errors show up on native Windows and are not from this change — I confirmed the test failures reproduce with this commit stashed, and none of the clippy sites are in this diff:

Both are Windows-only and CI's rust job runs on ubuntu-latest, so neither is visible upstream. I left them alone rather than widening this diff to unrelated code.

Related

Refs #213 — that report describes having to try profiles one at a time until one works. This does not fix an internal retry loop; selection is a single match and I could not find a loop to fix. It removes the need to guess, by naming the candidates at the miss.

Prepared with AI assistance; the verification above was run locally and the failures reported are real observed output.

`session.start --browser` takes an extension instance id or a unique
label, and rejects a selector that matches nothing with `not_found` /
"requested browser is not connected". That error carried no payload, so
the caller could not tell a typo from an offline target, and could not
see which instances were actually connected.

The two sibling outcomes of the same selector resolution already carry
their candidates: `multiple_browsers_online` emits a `browsers` snapshot
and the ambiguous-label case emits `instance_ids`, and the CLI already
renders a connected-browsers table and a candidate bullet list for those
two. `not_found` was the third case, and the only one with nothing to
show.

Attach the same snapshot to `BrowserNotFound`, which becomes a struct
variant, and render the connected-browsers table for it. The message is
unchanged, and an empty registry still emits no `data` payload, so that
wire shape and the existing `details:` line are untouched.
@iuyo5678

Copy link
Copy Markdown
Collaborator

Thanks for the contribution. I reviewed the change against the latest main, and I don’t see any major implementation issues that should block merging, assuming the required CI checks pass. Reusing the existing browser snapshot and table renderer keeps this improvement focused and consistent with the current behavior.

A few suggestions to strengthen the PR:

  • Add an integration test for the actual selector-miss path. The new tests cover error mapping and rendering separately. Exercising a real session.start request against registered browsers would also verify that the candidate list reaches the caller and that a failed selection creates no session.
  • Clarify the scope of the improvement. The list helps users diagnose a missing selector, but it cannot determine whether the selector was mistyped or the intended browser is offline. It would be helpful to remind callers to verify or reconnect the intended browser before choosing another instance.
  • Note the current DSH limitation. The DSH error adapter currently drops data, so its users won’t see the new candidates yet. A brief note would keep expectations accurate; support there can be a follow-up.

Overall, I think this is a useful improvement and worth merging once CI is green. These suggestions are intended to strengthen coverage and clarify the user-facing behavior.

Review follow-up on Tencent#313.

- Add an integration test that drives `start_session_recoverable`
  against a registry holding two connected browsers, with a selector
  that matches nothing. It pins both halves the review asked for: the
  candidate list reaches the caller through the daemon's own error
  mapping, and the failed selection leaves the session registry empty.
  The existing tests covered the mapping and the CLI rendering
  separately, so neither could show the two together.
- Say what the candidate table cannot tell. A mistyped selector and an
  offline browser are indistinguishable in the list, so the extras now
  say so, and the ordering test covers the added line.
@PerryLink

Copy link
Copy Markdown
Author

Thanks for the review — all three points are addressed in 0a679b2, pushed on top of 85d0711.

1. Integration test for the real selector-miss path. Added selector_miss_reaches_caller_and_creates_no_session in daemon/ipc.rs. It builds a BrowserRegistry with two connected browsers and calls start_session_recoverable — the same function the daemon calls for session.start — then asserts both things you named: the candidate list reaches the caller through the daemon's own error mapping, and the failed selection leaves SessionRegistry empty. You were right that the two earlier tests could not show either, since they cover the mapping and the rendering separately. The emptiness assertion also pins the ordering that makes it true today: the selection ? returns before any session id is reserved.

2. Scope of the list. The not_found extras now end with:

the list shows what is connected now; it cannot tell a mistyped selector from an offline browser, so check or reconnect the intended browser before starting on another instance

It renders between the table and the centralised hint:, and the ordering test covers the new line.

3. The DSH limitation is one level above the CLI, and inside this repo. I re-checked this against the current tree instead of restating it, and the finding is narrower than "DSH drops the data":

  • the CLI already emits it — cli/error.rs serializes { code, message, hint, exit_code, data } and sets data: err.data().cloned(), so {"browsers": [...]} is on stdout under --json;
  • the drop happens here, in packages/dsh-plugin-browserskill/src/runner.ts: BskErrorBody already declares data?: unknown, but BskError has no data field and parseBskJson throws with only code, hint and exitCode;
  • and because the tool wrappers re-throw, the host only ever sees error.message — so even carrying data on the error object would not reach a DSH user. The candidates have to go into the message text.

So the follow-up is a small change in this repo rather than a DSH-side one. I kept it out of this PR to leave the diff on the CLI, and I am glad to send it separately if you want it.

One note on the base: this branch is some way behind main, so cargo test -p bsk --lib on native Windows still hits the two skill_install::harness failures we reported as #312, which main has since fixed in 47f5765. They are unrelated to this change and are not in any CI job this PR runs, so I left the base exactly as you reviewed it rather than rebasing. Say the word if you would rather have it refreshed.

Local checks on Windows: cargo fmt --all -- --check clean; cargo clippy --workspace --all-targets reports only pre-existing warnings (file_transfer.rs:417/:426, harness.rs:339, ipc.rs:743, tools_m9_ipc.rs:40), none of them in the added code; the five selector-miss tests pass.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants