diff --git a/crates/bsk-cli/src/cli/session.rs b/crates/bsk-cli/src/cli/session.rs index 9bf3ce07..d2a038cf 100644 --- a/crates/bsk-cli/src/cli/session.rs +++ b/crates/bsk-cli/src/cli/session.rs @@ -393,6 +393,30 @@ 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)?; + // 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(()), } } @@ -767,4 +791,87 @@ 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")); + // 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 < 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")); + } + + /// 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 7f15fb68..0c725217 100644 --- a/crates/bsk-cli/src/daemon/ipc.rs +++ b/crates/bsk-cli/src/daemon/ipc.rs @@ -1059,7 +1059,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, @@ -1074,6 +1074,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, @@ -2133,3 +2141,162 @@ 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::*; + use crate::daemon::browsers::{ + BrowserClient, BrowserId, BrowserRegistry, BrowserSink, Liveness, Pending, + next_browser_generation, + }; + use crate::daemon::queue::ToolQueueRegistry; + use crate::daemon::sessions::{SessionRegistry, start_session_recoverable}; + use std::sync::Mutex; + + 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(), + unresponsive: false, + } + } + + /// 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" + ); + } + + // ----------------------------------------------------------------- + // 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, + liveness: Liveness::default(), + }) + } + + /// 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:?}"); + } +} diff --git a/crates/bsk-cli/src/daemon/sessions.rs b/crates/bsk-cli/src/daemon/sessions.rs index c4174e01..766d7db5 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, @@ -409,7 +418,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", @@ -524,7 +533,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,