From 948877fa0bd4284119ea2bffb01b98f0bc01db96 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Fri, 21 Aug 2026 23:57:33 +1000 Subject: [PATCH] feat(cli): clearer remote errors and a live status line Transport failures now say whether DNS, the connection, TLS, or a timeout failed, name the host, and state the recovery step. Server 5xx fallbacks say to retry in a minute. Final rate limits state the Retry-After wait. Network commands show a TTY-only status line on stderr with elapsed time. It starts after output-mode detection, erases before any command output, and stays silent for Agents, CI, pipes, and --json. --- crates/skilld-command/src/remote.rs | 97 +++++++++-- crates/skilld-native/src/lib.rs | 94 ++++++++++- crates/skilld-native/src/main.rs | 13 +- crates/skilld-native/src/status.rs | 248 ++++++++++++++++++++++++++++ 4 files changed, 432 insertions(+), 20 deletions(-) create mode 100644 crates/skilld-native/src/status.rs diff --git a/crates/skilld-command/src/remote.rs b/crates/skilld-command/src/remote.rs index 6ad7b9aa..462be0b0 100644 --- a/crates/skilld-command/src/remote.rs +++ b/crates/skilld-command/src/remote.rs @@ -2109,12 +2109,7 @@ fn problem_error(response: &HttpResponse) -> RemoteError { instance: Option, } serde_json::from_slice::(&response.body).map_or_else( - |_| { - RemoteError::new( - "SERVICE_UNAVAILABLE", - format!("the remote service returned HTTP {}", response.status), - ) - }, + |_| service_unavailable_error(response), |problem| { let _ = (&problem.r#type, &problem.instance); if problem.status != response.status { @@ -2123,14 +2118,45 @@ fn problem_error(response: &HttpResponse) -> RemoteError { "the remote problem status does not match HTTP", ); } - RemoteError::new( - problem_code(&problem.code), - problem.detail.unwrap_or(problem.title), - ) + let detail = match (response.status, retry_after_seconds(response)) { + (429, Some(seconds)) => { + let detail = problem.detail.unwrap_or(problem.title); + let detail = detail.trim_end_matches('.'); + format!("{detail}. Retry in {seconds}s.") + } + _ => problem.detail.unwrap_or(problem.title), + }; + RemoteError::new(problem_code(&problem.code), detail) }, ) } +fn service_unavailable_error(response: &HttpResponse) -> RemoteError { + match (response.status, retry_after_seconds(response)) { + (429, Some(seconds)) => RemoteError::new( + "SERVICE_UNAVAILABLE", + format!("the remote service rate limited the request. Retry in {seconds}s."), + ), + (status, _) if (500..600).contains(&status) => RemoteError::new( + "SERVICE_UNAVAILABLE", + format!( + "the remote service returned HTTP {status}. Retry in a minute. If it keeps failing, the service may be down." + ), + ), + (status, _) => RemoteError::new( + "SERVICE_UNAVAILABLE", + format!("the remote service returned HTTP {status}"), + ), + } +} + +fn retry_after_seconds(response: &HttpResponse) -> Option { + response + .header("retry-after") + .and_then(|value| value.parse::().ok()) + .filter(|value| *value <= 3600) +} + fn problem_code(value: &str) -> &'static str { match value { "AUTH_REQUIRED" => "AUTH_REQUIRED", @@ -2341,3 +2367,54 @@ struct GithubBlob { encoding: String, size: u64, } + +#[cfg(test)] +mod problem_tests { + use super::{HttpResponse, problem_error}; + + fn response(status: u16, headers: &[(&str, &str)], body: &str) -> HttpResponse { + HttpResponse { + status, + headers: headers + .iter() + .map(|(name, value)| ((*name).to_owned(), (*value).to_owned())) + .collect(), + body: body.as_bytes().to_vec(), + } + } + + #[test] + fn server_errors_state_the_next_step() { + let error = problem_error(&response(500, &[], "not json")); + assert_eq!(error.code, "SERVICE_UNAVAILABLE"); + assert_eq!( + error.message, + "the remote service returned HTTP 500. Retry in a minute. If it keeps failing, the service may be down." + ); + } + + #[test] + fn rate_limits_without_a_body_state_the_wait() { + let error = problem_error(&response(429, &[("retry-after", "7")], "not json")); + assert_eq!(error.code, "SERVICE_UNAVAILABLE"); + assert_eq!( + error.message, + "the remote service rate limited the request. Retry in 7s." + ); + } + + #[test] + fn rate_limits_with_a_body_append_the_wait() { + let body = r#"{"code":"RATE_LIMITED","title":"Too many requests","status":429,"type":"about:blank"}"#; + let error = problem_error(&response(429, &[("retry-after", "12")], body)); + assert_eq!(error.code, "RATE_LIMITED"); + assert_eq!(error.message, "Too many requests. Retry in 12s."); + } + + #[test] + fn client_errors_keep_the_plain_message() { + let error = problem_error(&response(404, &[], "not json")); + assert_eq!(error.code, "SERVICE_UNAVAILABLE"); + assert_eq!(error.message, "the remote service returned HTTP 404"); + } +} diff --git a/crates/skilld-native/src/lib.rs b/crates/skilld-native/src/lib.rs index c664a946..f7c06b7f 100644 --- a/crates/skilld-native/src/lib.rs +++ b/crates/skilld-native/src/lib.rs @@ -115,12 +115,7 @@ impl HttpAdapter for NativeHttpAdapter { builder.send(request.body.as_slice()) } } - .map_err(|_| { - RemoteError::new( - "HTTP_TRANSPORT", - "the remote request could not be completed", - ) - })?; + .map_err(|error| transport_error(&error, &request.url))?; let status = response.status().as_u16(); let headers = response .headers() @@ -153,8 +148,15 @@ impl HttpAdapter for NativeHttpAdapter { "the remote operation was cancelled", )); } - let read = reader.read(&mut buffer).map_err(|_| { - RemoteError::new("HTTP_TRANSPORT", "the remote response could not be read") + let read = reader.read(&mut buffer).map_err(|error| { + let reason = match error.kind() { + std::io::ErrorKind::TimedOut | std::io::ErrorKind::WouldBlock => "timed out", + _ => "could not be read", + }; + RemoteError::new( + "HTTP_TRANSPORT", + format!("the remote response {reason}. Retry the command."), + ) })?; if read == 0 { break; @@ -174,3 +176,79 @@ impl HttpAdapter for NativeHttpAdapter { }) } } + +fn transport_error(error: &ureq::Error, request_url: &str) -> RemoteError { + let host = Url::parse(request_url) + .ok() + .and_then(|url| url.host_str().map(str::to_owned)) + .unwrap_or_else(|| "the remote service".to_owned()); + let reason = match error { + ureq::Error::HostNotFound => format!("the {host} address could not be resolved"), + ureq::Error::ConnectionFailed => format!("the connection to {host} failed"), + ureq::Error::Timeout(_) => format!("the request to {host} timed out"), + ureq::Error::Tls(_) | ureq::Error::Rustls(_) | ureq::Error::Pem(_) => { + format!("the secure connection to {host} failed") + } + ureq::Error::Io(io) => match io.kind() { + std::io::ErrorKind::TimedOut | std::io::ErrorKind::WouldBlock => { + format!("the request to {host} timed out") + } + std::io::ErrorKind::ConnectionRefused | std::io::ErrorKind::ConnectionReset => { + format!("the connection to {host} failed") + } + std::io::ErrorKind::NotFound => format!("the {host} address could not be resolved"), + _ => "the remote request could not be completed".to_owned(), + }, + _ => "the remote request could not be completed".to_owned(), + }; + let timed_out = matches!(error, ureq::Error::Timeout(_)) + || matches!(error, ureq::Error::Io(io) if io.kind() == std::io::ErrorKind::TimedOut); + let secure = matches!( + error, + ureq::Error::Tls(_) | ureq::Error::Rustls(_) | ureq::Error::Pem(_) + ); + let recovery = if secure { + "Check the system clock and certificates, then retry." + } else if timed_out { + "Retry the command. A slow network can cause this." + } else { + "Check the network connection, then retry the command." + }; + RemoteError::new("HTTP_TRANSPORT", format!("{reason}. {recovery}")) +} + +#[cfg(test)] +mod tests { + use super::transport_error; + + fn message(error: ureq::Error) -> String { + transport_error(&error, "https://skilld.dev/api/v1/skills").message + } + + #[test] + fn dns_failures_name_the_host_and_a_recovery_step() { + assert_eq!( + message(ureq::Error::HostNotFound), + "the skilld.dev address could not be resolved. Check the network connection, then retry the command." + ); + } + + #[test] + fn connection_failures_name_the_host() { + assert_eq!( + message(ureq::Error::ConnectionFailed), + "the connection to skilld.dev failed. Check the network connection, then retry the command." + ); + } + + #[test] + fn timeouts_say_so() { + assert_eq!( + message(ureq::Error::Io(std::io::Error::new( + std::io::ErrorKind::TimedOut, + "timed out" + ))), + "the request to skilld.dev timed out. Retry the command. A slow network can cause this." + ); + } +} diff --git a/crates/skilld-native/src/main.rs b/crates/skilld-native/src/main.rs index 5426af36..8c8a7e2e 100644 --- a/crates/skilld-native/src/main.rs +++ b/crates/skilld-native/src/main.rs @@ -1,5 +1,6 @@ mod embedded_skill; mod native_auth; +mod status; use std::env; use std::io::{IsTerminal, Write}; @@ -22,6 +23,7 @@ use skilld_native::update_ui::{ CommandInteractiveUpdateHost, require_interactive_tty, run_interactive_update, write_static_summary, }; +use status::StatusLine; use terminal_size::Width; fn main() -> ExitCode { @@ -97,7 +99,7 @@ fn main() -> ExitCode { } let mut stdout = std::io::stdout().lock(); - let mut stderr = std::io::stderr().lock(); + let mut stderr = std::io::stderr(); let output = OutputContext::auto( stdout.is_terminal(), active_agent_detected(), @@ -106,7 +108,14 @@ fn main() -> ExitCode { env::var("TERM").is_ok_and(|term| term.eq_ignore_ascii_case("dumb")), terminal_width(), ); - let result = run_with_output(args, host.as_ref(), output, &mut stdout, &mut stderr); + let label = status::status_label(args.iter().map(|arg| arg.to_string_lossy())); + let status = match label { + Some(label) => StatusLine::for_terminal(label, output), + None => StatusLine::disabled(), + }; + let mut gated = status::GatedStderr::new(&mut stderr, status); + let result = run_with_output(args, host.as_ref(), output, &mut stdout, &mut gated); + gated.finish_status(); ExitCode::from(result.exit_code) } diff --git a/crates/skilld-native/src/status.rs b/crates/skilld-native/src/status.rs new file mode 100644 index 00000000..74ea7e50 --- /dev/null +++ b/crates/skilld-native/src/status.rs @@ -0,0 +1,248 @@ +use std::io::Write; +use std::sync::Arc; +use std::sync::Mutex; +use std::thread; +use std::thread::JoinHandle; +use std::time::Duration; +use std::time::Instant; + +use skilld_command::OutputContext; + +const ERASE_LINE: &[u8] = b"\r\x1b[2K"; + +struct StatusState { + stopped: bool, + out: Box, + label: String, + started: Instant, +} + +pub struct StatusLine { + shared: Option>>, + thread: Option>, +} + +impl StatusLine { + pub fn disabled() -> Self { + Self { + shared: None, + thread: None, + } + } + + #[cfg(test)] + pub fn is_disabled(&self) -> bool { + self.shared.is_none() + } + + pub fn begin(label: &str, tick: Duration, out: Box) -> Self { + let shared = Arc::new(Mutex::new(StatusState { + stopped: false, + out, + label: label.to_owned(), + started: Instant::now(), + })); + let worker = Arc::clone(&shared); + let thread = thread::spawn(move || { + loop { + let mut state = match worker.lock() { + Ok(state) => state, + Err(_) => return, + }; + if state.stopped { + return; + } + let seconds = state.started.elapsed().as_secs(); + let line = format!("\r\x1b[2K{}… {seconds}s", state.label); + let _ = state.out.write_all(line.as_bytes()); + let _ = state.out.flush(); + drop(state); + thread::sleep(tick); + } + }); + Self { + shared: Some(shared), + thread: Some(thread), + } + } + + pub fn for_terminal(label: &str, context: OutputContext) -> Self { + if !matches!(context, OutputContext::HumanTerminal { .. }) { + return Self::disabled(); + } + Self::begin( + label, + Duration::from_millis(500), + Box::new(std::io::stderr()), + ) + } + + pub fn stop(&mut self) { + let Some(shared) = &self.shared else { + return; + }; + let mut state = match shared.lock() { + Ok(state) => state, + Err(_) => return, + }; + if !state.stopped { + state.stopped = true; + let _ = state.out.write_all(ERASE_LINE); + let _ = state.out.flush(); + } + } + + pub fn finish(mut self) { + self.stop(); + if let Some(thread) = self.thread.take() { + let _ = thread.join(); + } + } +} + +pub struct GatedStderr<'a, W: Write> { + inner: &'a mut W, + status: Option, +} + +impl<'a, W: Write> GatedStderr<'a, W> { + pub fn new(inner: &'a mut W, status: StatusLine) -> Self { + Self { + inner, + status: Some(status), + } + } + + pub fn finish_status(&mut self) { + if let Some(status) = self.status.take() { + status.finish(); + } + } +} + +impl Write for GatedStderr<'_, W> { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.finish_status(); + self.inner.write(buf) + } + + fn flush(&mut self) -> std::io::Result<()> { + self.finish_status(); + self.inner.flush() + } +} + +pub fn status_label(args: I) -> Option<&'static str> +where + I: IntoIterator, + S: AsRef, +{ + let mut args = args.into_iter(); + let _binary = args.next()?; + let mut subcommand = None; + for arg in args { + let arg = arg.as_ref(); + if arg == "--json" { + return None; + } + if arg.starts_with('-') { + continue; + } + subcommand = Some(arg.to_owned()); + break; + } + match subcommand.as_deref() { + Some("search") => Some("Searching"), + Some("install") => Some("Installing"), + Some("view") => Some("Loading"), + Some("verify") => Some("Verifying"), + Some("update") => Some("Updating"), + _ => None, + } +} + +#[cfg(test)] +mod tests { + use super::{GatedStderr, OutputContext, StatusLine, status_label}; + use std::io::Write; + use std::sync::Mutex; + use std::time::Duration; + + fn never() -> Duration { + Duration::MAX + } + + fn output(buffer: &Mutex>) -> String { + String::from_utf8(buffer.lock().unwrap().clone()).unwrap() + } + + #[test] + fn a_disabled_line_writes_nothing() { + StatusLine::disabled().finish(); + } + + #[test] + fn finish_erases_the_line() { + let buffer = std::sync::Arc::new(Mutex::new(Vec::new())); + let status = StatusLine::begin("Searching", never(), Box::new(Writer(buffer.clone()))); + status.finish(); + assert_eq!(output(&buffer), "\r\x1b[2K"); + } + + #[test] + fn stop_erases_once_and_is_idempotent() { + let buffer = std::sync::Arc::new(Mutex::new(Vec::new())); + let mut status = StatusLine::begin("Searching", never(), Box::new(Writer(buffer.clone()))); + status.stop(); + status.stop(); + status.finish(); + assert_eq!(output(&buffer), "\r\x1b[2K"); + } + + #[test] + fn a_gated_writer_stops_the_line_before_forwarding() { + let buffer = std::sync::Arc::new(Mutex::new(Vec::new())); + let status = StatusLine::begin("Searching", never(), Box::new(Writer(buffer.clone()))); + let mut sink = Vec::new(); + let mut gated = GatedStderr::new(&mut sink, status); + gated.write_all(b"done").unwrap(); + gated.finish_status(); + assert_eq!(output(&buffer), "\r\x1b[2K"); + assert_eq!(sink, b"done"); + } + + #[test] + fn labels_map_to_network_commands() { + let args = ["skilld", "--json", "search", "hi"]; + assert_eq!(status_label(args), None); + let args = ["skilld", "search", "hi"]; + assert_eq!(status_label(args), Some("Searching")); + let args = ["skilld", "install", "skilld:owner/repo/skill"]; + assert_eq!(status_label(args), Some("Installing")); + let args = ["skilld", "list"]; + assert_eq!(status_label(args), None); + let args = ["skilld"]; + assert_eq!(status_label(args), None); + } + + #[test] + fn plain_contexts_get_no_status_line() { + assert!(matches!( + StatusLine::for_terminal("Searching", OutputContext::Plain), + line if line.is_disabled() + )); + } + + struct Writer(std::sync::Arc>>); + + impl Write for Writer { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } +}