feat(cli): clearer remote errors and a live status line (#117)

Author: harlan-zw

crates/skilld-command/src/remote.rs

@@ -2109,12 +2109,7 @@ fn problem_error(response: &HttpResponse) -> RemoteError {
         instance: Option<String>,
     }
     serde_json::from_slice::<Problem>(&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<u64> {
+    response
+        .header("retry-after")
+        .and_then(|value| value.parse::<u64>().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");
+    }
+}

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."
+        );
+    }
+}

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)
 }