diff --git a/crates/bxctl/src/admin.rs b/crates/bxctl/src/admin.rs index cde00b0..6d38c81 100644 --- a/crates/bxctl/src/admin.rs +++ b/crates/bxctl/src/admin.rs @@ -40,7 +40,17 @@ pub fn request_with_timeout( msg, }; write_frame(&mut stream, &env).map_err(AdminError::Frame)?; - let answer = read_frame(&mut stream).map_err(AdminError::Frame)?; + let answer = read_frame(&mut stream).map_err(|e| match e { + FrameError::Io(io) + if matches!( + io.kind(), + io::ErrorKind::WouldBlock | io::ErrorKind::TimedOut + ) => + { + AdminError::NoAnswer(timeout) + } + other => AdminError::Frame(other), + })?; if answer.id != 1 || !answer.r#final { return Err(AdminError::Protocol( "expected an answer for request 1".to_string(), @@ -70,19 +80,27 @@ pub enum AdminError { Protocol(String), /// Writing the output failed; the caller stops rather than write again. Io(std::io::Error), + /// The request was sent and no answer came in time, so what brokerd did is not known. + NoAnswer(std::time::Duration), } impl std::fmt::Display for AdminError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { AdminError::Connect(p, e) => write!(f, "cannot reach brokerd at {}: {e}", p.display()), - AdminError::Frame(e) => write!(f, "{e}"), + // serde quotes the offending text of a bad frame, decoded: escape it. + AdminError::Frame(e) => write!(f, "{}", escape_json_text(&e.to_string())), // The detail may carry text from the inference server or a tool: escape it. AdminError::Refused(w) => { write!(f, "{}: {}", code_name(w.code), escape_json_text(&w.detail)) } AdminError::Protocol(s) => write!(f, "{s}"), AdminError::Io(e) => write!(f, "{e}"), + AdminError::NoAnswer(t) => write!( + f, + "brokerd did not answer within {} s; whether it acted is unknown: check `bxctl approvals` and `bxctl audit verify`", + t.as_secs_f64() + ), } } } diff --git a/crates/bxctl/src/chat.rs b/crates/bxctl/src/chat.rs index 11548a6..e05dab9 100644 --- a/crates/bxctl/src/chat.rs +++ b/crates/bxctl/src/chat.rs @@ -24,7 +24,8 @@ impl std::fmt::Display for ChatError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { ChatError::Connect(e) => write!(f, "{e}"), - ChatError::Frame(e) => write!(f, "{e}"), + // serde quotes the offending text of a bad frame, decoded: escape it. + ChatError::Frame(e) => write!(f, "{}", escape_json_text(&e.to_string())), // The detail may carry the inference server's body: escape it like model text. ChatError::Refused(w) => { write!(f, "{}: {}", code_name(w.code), escape_json_text(&w.detail)) diff --git a/crates/bxctl/tests/admin_timeout.rs b/crates/bxctl/tests/admin_timeout.rs index f716c0d..1b3e69e 100644 --- a/crates/bxctl/tests/admin_timeout.rs +++ b/crates/bxctl/tests/admin_timeout.rs @@ -31,7 +31,11 @@ fn a_silent_brokerd_is_an_error_after_the_timeout() { Duration::from_millis(200), ); let took = started.elapsed(); - assert!(got.is_err(), "{got:?}"); + let text = got.expect_err("a silent brokerd is an error").to_string(); + assert!( + text.contains("whether it acted is unknown"), + "brokerd may have approved and run the call before going quiet: {text}" + ); assert!(took < Duration::from_millis(1_500), "waited {took:?}"); held.join().unwrap(); let _ = std::fs::remove_dir_all(&dir); diff --git a/crates/bxctl/tests/escape_details.rs b/crates/bxctl/tests/escape_details.rs index 587c993..238ea48 100644 --- a/crates/bxctl/tests/escape_details.rs +++ b/crates/bxctl/tests/escape_details.rs @@ -55,3 +55,21 @@ fn an_error_detail_is_escaped_in_chat_and_admin_errors() { assert!(text.contains("\\u202e"), "{text}"); } } + +/// A frame that does not decode is reported with serde's message, which quotes the offending +/// text after JSON decoding: from a compromised peer that can be raw escape sequences. +#[test] +fn a_frame_error_is_escaped() { + let bad = r#"{"v":1,"id":1,"final":true,"msg":{"kind":"\u001b[2J\u202e"}}"#; + let error = serde_json::from_str::(bad).unwrap_err(); + for text in [ + ChatError::Frame(proto::FrameError::Json( + serde_json::from_str::(bad).unwrap_err(), + )) + .to_string(), + AdminError::Frame(proto::FrameError::Json(error)).to_string(), + ] { + clean(&text); + assert!(text.contains("\\u001b"), "{text}"); + } +}