From 85d07112a5c0a36f6ce3794565f06b4a34f7ec24 Mon Sep 17 00:00:00 2001 From: PerryLink <255665900+PerryLink@users.noreply.github.com> Date: Tue, 22 Sep 2026 00:02:04 +0800 Subject: [PATCH 1/2] feat(cli): list connected browsers when a selector matches nothing `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. --- crates/bsk-cli/src/cli/session.rs | 90 +++++++++++++++++++++++++++ crates/bsk-cli/src/daemon/ipc.rs | 66 +++++++++++++++++++- crates/bsk-cli/src/daemon/sessions.rs | 22 ++++++- 3 files changed, 174 insertions(+), 4 deletions(-) diff --git a/crates/bsk-cli/src/cli/session.rs b/crates/bsk-cli/src/cli/session.rs index 9bf3ce07..f11f817b 100644 --- a/crates/bsk-cli/src/cli/session.rs +++ b/crates/bsk-cli/src/cli/session.rs @@ -393,6 +393,22 @@ impl RenderExtras for StartExtras<'_> { } Ok(()) } + ErrorCode::NotFound => { + // Same table as `multiple_browsers_online`, but for a + // selector that matched nothing. Absent / empty + // `browsers` data (an older daemon, or a miss against an + // empty registry) renders nothing, so unrelated + // `not_found` errors keep their current output. + let browsers = parse_browsers_data(data.as_ref()); + if browsers.is_empty() { + return Ok(()); + } + writeln!( + out, + "no online browser matches the requested selector; connected browsers:" + )?; + write_browser_table(out, &browsers) + } _ => Ok(()), } } @@ -767,4 +783,78 @@ mod i3_tests { assert!(stderr.contains("hint:")); assert!(stderr.contains("details: requested browser is not connected")); } + + /// A selector that matches nothing renders the connected-browser + /// table, so the user (or agent) can pick a real instance id or + /// label in one round trip instead of cycling through every + /// connected browser until one starts. + #[test] + fn selector_miss_extras_render_candidate_table() { + let data = serde_json::json!({ + "browsers": [ + { + "instance_id": "alpha", + "browser_name": "chrome", + "browser_version": "131", + "extension_version": "0.1.0-dev.0", + "label": "Personal", + "session_count": 0_u32, + "connected_at_ms": 1_i64, + "version_skew": false, + }, + { + "instance_id": "beta", + "browser_name": "edge", + "browser_version": "130", + "extension_version": "0.1.0-dev.0", + "label": "", + "session_count": 1_u32, + "connected_at_ms": 2_i64, + "version_skew": false, + }, + ] + }); + let cli = CliError::from_rpc(RpcError { + code: ErrorCode::NotFound, + message: "requested browser is not connected".into(), + data: Some(data), + }); + let extras = StartExtras::new(&cli); + let stderr = render_human_to_string(&cli, Some(&extras)); + assert!(stderr.contains("error: requested resource does not exist")); + assert!( + stderr.contains("no online browser matches the requested selector"), + "extras must explain the miss: {stderr}" + ); + assert!(stderr.contains("INSTANCE")); + assert!(stderr.contains("alpha")); + assert!(stderr.contains("beta")); + assert!(stderr.contains("Personal")); + // Ordering contract unchanged: summary → extras → hint. + let summary_idx = stderr.find("error:").expect("summary line missing"); + let table_idx = stderr + .find("connected browsers:") + .expect("candidate table missing"); + let hint_idx = stderr.find("hint:").expect("hint line missing"); + assert!( + summary_idx < table_idx && table_idx < hint_idx, + "stderr order must be summary → extras → hint, got:\n{stderr}" + ); + assert!(stderr.contains("details: requested browser is not connected")); + } + + /// An empty candidate list (miss against an empty registry) renders + /// no extras, so the change is additive for existing output. + #[test] + fn selector_miss_with_empty_candidates_renders_no_extras() { + let cli = CliError::from_rpc(RpcError { + code: ErrorCode::NotFound, + message: "requested browser is not connected".into(), + data: Some(serde_json::json!({ "browsers": [] })), + }); + let extras = StartExtras::new(&cli); + let stderr = render_human_to_string(&cli, Some(&extras)); + assert!(!stderr.contains("connected browsers:")); + assert!(stderr.contains("details: requested browser is not connected")); + } } diff --git a/crates/bsk-cli/src/daemon/ipc.rs b/crates/bsk-cli/src/daemon/ipc.rs index 3c479d16..acf8f605 100644 --- a/crates/bsk-cli/src/daemon/ipc.rs +++ b/crates/bsk-cli/src/daemon/ipc.rs @@ -1009,7 +1009,7 @@ fn map_start_error(err: StartSessionError) -> RpcError { let code = match &err { StartSessionError::NoBrowserConnected => ErrorCode::NoBrowserConnected, StartSessionError::MultipleBrowsersOnline { .. } => ErrorCode::MultipleBrowsersOnline, - StartSessionError::BrowserNotFound => ErrorCode::NotFound, + StartSessionError::BrowserNotFound { .. } => ErrorCode::NotFound, StartSessionError::AmbiguousBrowserLabel { .. } => ErrorCode::InvalidParams, StartSessionError::IdExhausted => ErrorCode::ProtocolError, StartSessionError::Timeout => ErrorCode::Timeout, @@ -1023,6 +1023,14 @@ fn map_start_error(err: StartSessionError) -> RpcError { StartSessionError::MultipleBrowsersOnline { browsers } => { Some(serde_json::json!({ "browsers": browsers })) } + // A selector miss lists the candidates it could have meant, so + // the caller can pick a connected instance or label instead of + // probing every browser in turn. Emitted only when at least one + // browser is online, so a miss against an empty registry keeps + // the historical `code` + `message`-only payload. + StartSessionError::BrowserNotFound { browsers } if !browsers.is_empty() => { + Some(serde_json::json!({ "browsers": browsers })) + } StartSessionError::AmbiguousBrowserLabel { label, instance_ids, @@ -2076,3 +2084,59 @@ mod tests { let _ = server.await; } } + +/// Platform-neutral coverage for the `session.start` selector-miss +/// payload. Kept out of the `#[cfg(all(test, unix))]` IPC transport +/// module above so it also runs on Windows. +#[cfg(test)] +mod selector_miss_payload_tests { + use super::*; + + fn status_entry(instance_id: &str, label: &str) -> BrowserStatusEntry { + BrowserStatusEntry { + instance_id: instance_id.into(), + browser_name: "chrome".into(), + browser_version: "131".into(), + extension_version: "0.1.0-dev.0".into(), + label: label.into(), + session_count: 0, + connected_at_ms: 1, + version_skew: false, + extension_protocol_version: String::new(), + } + } + + /// A selector miss must hand the caller the connected candidates so + /// it can pick a real instance id or label in one round trip, + /// instead of enumerating `bsk browsers` and retrying instances one + /// after another. + #[test] + fn browser_selector_miss_lists_connected_candidates() { + let err = map_start_error(StartSessionError::BrowserNotFound { + browsers: vec![status_entry("alpha", "Personal"), status_entry("beta", "")], + }); + assert_eq!(err.code, ErrorCode::NotFound); + // The human-readable message is unchanged (existing callers and + // the `details:` line depend on it). + assert_eq!(err.message, "requested browser is not connected"); + let data = err.data.expect("selector miss must carry candidates"); + let browsers = data["browsers"].as_array().expect("browsers array"); + assert_eq!(browsers.len(), 2); + assert_eq!(browsers[0]["instance_id"], "alpha"); + assert_eq!(browsers[0]["label"], "Personal"); + assert_eq!(browsers[1]["instance_id"], "beta"); + } + + /// Backward compatibility: with nothing connected there is nothing + /// to suggest, so the error keeps its previous message-only shape. + #[test] + fn browser_selector_miss_without_browsers_keeps_message_only_payload() { + let err = map_start_error(StartSessionError::BrowserNotFound { browsers: vec![] }); + assert_eq!(err.code, ErrorCode::NotFound); + assert_eq!(err.message, "requested browser is not connected"); + assert!( + err.data.is_none(), + "an empty registry must not invent a `browsers` payload" + ); + } +} diff --git a/crates/bsk-cli/src/daemon/sessions.rs b/crates/bsk-cli/src/daemon/sessions.rs index c4201026..45c4752a 100644 --- a/crates/bsk-cli/src/daemon/sessions.rs +++ b/crates/bsk-cli/src/daemon/sessions.rs @@ -378,7 +378,16 @@ pub enum StartSessionError { browsers: Vec, }, #[error("requested browser is not connected")] - BrowserNotFound, + BrowserNotFound { + /// Snapshot of every currently connected browser, attached to + /// the daemon's structured error so a selector miss can list + /// the candidates the caller could have meant instead of + /// making the caller run `bsk browsers` and retry one instance + /// after another. Empty when nothing is connected, in which + /// case no `browsers` payload is emitted at all and the error + /// keeps its pre-existing wire shape. + browsers: Vec, + }, #[error("label '{label}' matches {} connected browsers", instance_ids.len())] AmbiguousBrowserLabel { label: String, @@ -407,7 +416,7 @@ impl StartSessionError { match self { StartSessionError::NoBrowserConnected => "no_browser_connected", StartSessionError::MultipleBrowsersOnline { .. } => "multiple_browsers_online", - StartSessionError::BrowserNotFound => "not_found", + StartSessionError::BrowserNotFound { .. } => "not_found", StartSessionError::AmbiguousBrowserLabel { .. } => "invalid_params", StartSessionError::IdExhausted => "protocol_error", StartSessionError::Timeout => "timeout", @@ -518,7 +527,14 @@ pub(crate) async fn start_session_recoverable( SelectError::MultipleBrowsersOnline => StartSessionError::MultipleBrowsersOnline { browsers: snapshot_status_entries(registry, sessions), }, - SelectError::NotFound => StartSessionError::BrowserNotFound, + // A selector miss is the one selection failure that used to + // carry no candidates, which pushed callers into enumerating + // `bsk browsers` and retrying instances one by one. Attach the + // same snapshot `multiple_browsers_online` already uses so the + // CLI can answer the miss in a single round trip. + SelectError::NotFound => StartSessionError::BrowserNotFound { + browsers: snapshot_status_entries(registry, sessions), + }, SelectError::AmbiguousLabel { label, instance_ids, From 0a679b21f57896e185fcb0e72acaf6425512ed3e Mon Sep 17 00:00:00 2001 From: PerryLink <255665900+PerryLink@users.noreply.github.com> Date: Sun, 4 Oct 2026 21:46:33 +0800 Subject: [PATCH 2/2] test(session): exercise the selector-miss path end to end Review follow-up on #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. --- crates/bsk-cli/src/cli/session.rs | 23 ++++++- crates/bsk-cli/src/daemon/ipc.rs | 103 ++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 3 deletions(-) diff --git a/crates/bsk-cli/src/cli/session.rs b/crates/bsk-cli/src/cli/session.rs index f11f817b..d2a038cf 100644 --- a/crates/bsk-cli/src/cli/session.rs +++ b/crates/bsk-cli/src/cli/session.rs @@ -407,7 +407,15 @@ impl RenderExtras for StartExtras<'_> { out, "no online browser matches the requested selector; connected browsers:" )?; - write_browser_table(out, &browsers) + write_browser_table(out, &browsers)?; + // The table answers "what is connected", not "what did you + // mean": a mistyped selector and an offline browser look + // identical here, so name the question it cannot answer + // rather than let the caller switch instances by accident. + writeln!( + out, + "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" + ) } _ => Ok(()), } @@ -830,15 +838,24 @@ mod i3_tests { assert!(stderr.contains("alpha")); assert!(stderr.contains("beta")); assert!(stderr.contains("Personal")); + // The table must not read as "pick another instance": it cannot + // distinguish a mistyped selector from an offline browser. + assert!( + stderr.contains("cannot tell a mistyped selector from an offline browser"), + "extras must state what the list cannot tell: {stderr}" + ); // Ordering contract unchanged: summary → extras → hint. let summary_idx = stderr.find("error:").expect("summary line missing"); let table_idx = stderr .find("connected browsers:") .expect("candidate table missing"); + let caveat_idx = stderr + .find("cannot tell a mistyped selector") + .expect("scope caveat missing"); let hint_idx = stderr.find("hint:").expect("hint line missing"); assert!( - summary_idx < table_idx && table_idx < hint_idx, - "stderr order must be summary → extras → hint, got:\n{stderr}" + summary_idx < table_idx && table_idx < caveat_idx && caveat_idx < hint_idx, + "stderr order must be summary → table → caveat → hint, got:\n{stderr}" ); assert!(stderr.contains("details: requested browser is not connected")); } diff --git a/crates/bsk-cli/src/daemon/ipc.rs b/crates/bsk-cli/src/daemon/ipc.rs index acf8f605..5461420b 100644 --- a/crates/bsk-cli/src/daemon/ipc.rs +++ b/crates/bsk-cli/src/daemon/ipc.rs @@ -2091,6 +2091,13 @@ mod tests { #[cfg(test)] mod selector_miss_payload_tests { use super::*; + use crate::daemon::browsers::{ + BrowserClient, BrowserId, BrowserRegistry, BrowserSink, Pending, next_browser_generation, + }; + use crate::daemon::queue::ToolQueueRegistry; + use crate::daemon::sessions::{SessionRegistry, start_session_recoverable}; + use std::sync::Mutex; + use std::sync::atomic::AtomicBool; fn status_entry(instance_id: &str, label: &str) -> BrowserStatusEntry { BrowserStatusEntry { @@ -2139,4 +2146,100 @@ mod selector_miss_payload_tests { "an empty registry must not invent a `browsers` payload" ); } + + // ----------------------------------------------------------------- + // Integration: the real start path, not the pieces + // ----------------------------------------------------------------- + + /// A registered browser, built in-process. `BrowserClient`'s fields + /// are public, so this registers candidates through the same + /// `BrowserRegistry` the daemon fills from the extension handshake — + /// no transport, no extension, no model. + fn connected_browser(instance_id: &str, label: &str) -> Arc { + let (tx, _rx) = tokio::sync::mpsc::unbounded_channel(); + Arc::new(BrowserClient { + id: BrowserId(instance_id.into()), + browser_name: "chrome".into(), + browser_version: "131".into(), + extension_version: "0.1.0-dev.0".into(), + extension_protocol_version: "1.0".into(), + label: label.into(), + sink: BrowserSink { tx }, + pending: Mutex::new(Pending::default()), + generation: next_browser_generation(), + connected_at_ms: 0, + version_skew: false, + last_seen: Mutex::new(Instant::now()), + heartbeat_seen: AtomicBool::new(false), + }) + } + + /// Drives the function the daemon actually calls for + /// `session.start` against a populated registry. The two tests above + /// cover the error mapping and the CLI rendering in isolation; this + /// one pins both halves of the review ask on a single run: + /// + /// 1. the candidate list reaches the caller in the wire shape, and + /// 2. a failed selection creates **no** session. + /// + /// (2) is structural today — the selection `?` returns before any id + /// is reserved — and this test is what keeps it that way. + #[tokio::test] + async fn selector_miss_reaches_caller_and_creates_no_session() { + let registry = Arc::new(BrowserRegistry::new()); + registry.insert(connected_browser("alpha", "Personal")); + registry.insert(connected_browser("beta", "")); + let sessions = Arc::new(SessionRegistry::new()); + let queues = Arc::new(ToolQueueRegistry::new(registry.clone(), sessions.clone())); + + assert!(sessions.is_empty(), "fixture must start with no sessions"); + + // A selector that matches nothing, with two browsers online: + // this must fail immediately and start nothing. + let error = start_session_recoverable( + ®istry, + &sessions, + &queues, + Some("no-such-browser"), + AgentWindowOptions::default(), + Duration::from_secs(1), + Duration::from_secs(1), + None, + false, + ) + .await + .expect_err("a selector that matches nothing must not start a session"); + + // (2) no session was reserved, so the caller can retry with a + // real instance id from the list below. + assert!( + sessions.is_empty(), + "a selector miss must not leave a session behind" + ); + + // (1) the candidates reach the caller through the daemon's own + // error mapping. + let rpc = map_start_error(error); + assert_eq!(rpc.code, ErrorCode::NotFound); + let data = rpc + .data + .expect("a miss with browsers online must carry candidates"); + let browsers = data["browsers"].as_array().expect("browsers array"); + let seen: Vec<(&str, &str)> = browsers + .iter() + .map(|b| { + ( + b["instance_id"].as_str().expect("instance_id"), + b["label"].as_str().expect("label"), + ) + }) + .collect(); + assert_eq!( + seen.len(), + 2, + "both connected browsers must be listed, got {seen:?}" + ); + assert!(seen.contains(&("alpha", "Personal")), "got {seen:?}"); + assert!(seen.contains(&("beta", "")), "got {seen:?}"); + } }