Conversation
`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.
|
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:
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.
|
Thanks for the review — all three points are addressed in 1. Integration test for the real selector-miss path. Added 2. Scope of the list. The
It renders between the table and the centralised 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":
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 Local checks on Windows: |
What
When
session.startis given a--browserselector that matches no connected browser, the daemon returnsnot_found/requested browser is not connectedwith 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:
Why this shape
not_foundis one of three outcomes of the same selector resolution, and the only one that carries nothing:multiple_browsers_online{ "browsers": [...] }invalid_params){ "instance_ids": [...] }not_foundThe
RenderExtrashook incli/session.rsenumerates 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 browsersThe
hint:line already tells the reader to runbsk browsersto see current state, anddocs/browser-profiles.mddocuments 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, thehint:line and the ordering contract are unchanged; a new test asserts the summary → extras → hint order explicitly.Backward compatibility
BrowserNotFoundgains 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.datapayload is emitted at all and that wire shape is unchanged. A test assertsdata.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
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:
skill_install::harness::tests::{detects_hermes_from_home_layout, skills_dirs_match_harness_spec}fail on Windows. Filed separately as Two Hermes skills-dir tests fail on native Windows: they assume ~/.hermes, but the resolver documents %LOCALAPPDATA%\hermes #312.cargo clippy --workspace --all-targets --locked -- -D warningsreportsdaemon/file_transfer.rs:417,daemon/file_transfer.rs:426,skill_install/harness.rs:339anddaemon/ipc.rs:743. The first three are in files this change does not touch, and the last is a function that this change neither added nor removed a caller for.Both are Windows-only and CI's
rustjob runs onubuntu-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.