From 96eaca552d1f24d962c6e0d88b4d2de50403fd6c Mon Sep 17 00:00:00 2001 From: Pavel Tiunov Date: Wed, 19 Aug 2026 18:10:02 -0700 Subject: [PATCH] feat(cube-cli): suggest updating the CLI when a request fails on the API (#11592) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(cli): suggest updating the CLI on API errors API errors are frequently caused by an outdated CLI talking to a newer API, so the error output now suggests updating the CLI as a possible fix. - add a shared `ApiError` (with status, url and response body) plus an `isApiError` marker-based type guard - throw it from `CubeCloudClient` requests and from `cubejs typegen` instead of a bare `Error`, and include the response body in the message - pass the caught error into `displayError` from every command so the update suggestion is printed under "Need some help?" for API errors - fix `cubejs typegen` crashing inside its own catch block when the failure isn't shaped like `{ error: { error } }` (e.g. network errors) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jv8Ykk5wWXab2KjCsK58vi * Revert "feat(cli): suggest updating the CLI on API errors" This reverts commit b2cddfe413b59e223be32dbc099b09fda1f01e06. * feat(cube-cli): suggest updating the CLI when a request fails on the API A CLI that lags the API is a common cause of otherwise puzzling API errors, so point at `cube update` before the user starts digging. - add `ApiError`, a marker error type for failures that came from an API response (unsuccessful status, or a body the CLI can't make sense of), and `is_api_error` to spot it anywhere in an error chain - return it from the API client and from the OAuth device/refresh flows; transport failures (DNS, TLS, connection refused) stay plain errors, since an update won't fix those - print a hint under the error when the failure is an API error, pointing at the update notice instead of repeating it when one was just shown - honor `CUBE_NO_UPDATE_CHECK` for the hint too, and document it Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jv8Ykk5wWXab2KjCsK58vi * fix(cube-cli): only suggest an update when the CLI is actually behind Telling someone already on the latest release to update is noise that teaches them to ignore the hint, so consult the background release check before offering it. - turn the check's result into an `UpdateCheck` outcome (newer / up to date / disabled / undetermined) instead of a pre-rendered notice, and resolve it once for both the notice and the hint - print no hint when the CLI is current or checks are opted out; name the available release when it is behind; suggest looking when the check could not complete - color the hint on a terminal, matching the notice it sits under, and keep it plain in piped output - cover the policy with tests that don't touch env or terminal state Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jv8Ykk5wWXab2KjCsK58vi * fix(cube-cli): don't make piped runs wait on the release check Resolving the check moved ahead of both its consumers, so every non-interactive run paid up to 1.5s for an answer it then discarded: the notice is interactive-only, and the hint only runs on an API failure. A piped successful command took 1526ms against an unresponsive release endpoint, against 28ms with checks opted out. Await the handle only when something will read it — 24ms on that same path now, with the notice and all four hint outcomes unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jv8Ykk5wWXab2KjCsK58vi * fix(cube-cli): print the error before waiting on the release check Only the hint needs the check's answer, so a slow or blocked GitHub sat between the user and the reason their command failed — 1521ms of silence against an unresponsive release endpoint, now 20ms. The notice moves below the error line on a failing run as a result, which also puts it directly above the hint that refers back to it. Successful runs are unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jv8Ykk5wWXab2KjCsK58vi --------- Co-authored-by: Claude --- rust/cube-cli/README.md | 13 ++- rust/cube-cli/src/client.rs | 65 +++++++++++- rust/cube-cli/src/error.rs | 72 ++++++++++++++ rust/cube-cli/src/main.rs | 21 +++- rust/cube-cli/src/oauth.rs | 21 ++-- rust/cube-cli/src/update.rs | 192 +++++++++++++++++++++++++++++++----- 6 files changed, 341 insertions(+), 43 deletions(-) create mode 100644 rust/cube-cli/src/error.rs diff --git a/rust/cube-cli/README.md b/rust/cube-cli/README.md index df7843f5577b8..015090e438c39 100644 --- a/rust/cube-cli/README.md +++ b/rust/cube-cli/README.md @@ -28,8 +28,17 @@ pins a specific release tag). Every run checks GitHub for a newer release in the background and prints a notice when one is available (set `CUBE_NO_UPDATE_CHECK=1` to disable, e.g. -in CI; the notice only goes to interactive terminals, on stderr). Update -in place any time with: +in CI; the notice only goes to interactive terminals, on stderr). + +The same check feeds a hint under API errors: when a request fails on the API +side *and* this binary is behind, the error is followed by a line suggesting +`cube update`, since a CLI that lags the API is a common cause of otherwise +puzzling failures. A CLI already on the latest release is never told to +update, and `CUBE_NO_UPDATE_CHECK=1` silences the hint along with the notice. +Unlike the notice, the hint is not limited to interactive terminals — it is +attached to a failure, and a stale pinned CLI in CI is where it pays off. + +Update in place any time with: ```bash cube update # download the latest release and replace this binary diff --git a/rust/cube-cli/src/client.rs b/rust/cube-cli/src/client.rs index f5a8475c47edc..dfba417ec81a1 100644 --- a/rust/cube-cli/src/client.rs +++ b/rust/cube-cli/src/client.rs @@ -4,6 +4,7 @@ use anyhow::{anyhow, bail, Result}; use reqwest::{Method, StatusCode}; use serde_json::Value; +use crate::error::api_bail; use crate::oauth; /// Thin HTTP client over the Cube Cloud public REST API. @@ -196,12 +197,12 @@ impl Client { }) .unwrap_or_else(|| text.clone()); match status { - StatusCode::UNAUTHORIZED => bail!( + StatusCode::UNAUTHORIZED => api_bail!( "unauthorized (401): session expired — run `cube login` (or set CUBE_API_KEY). {detail}" ), - StatusCode::FORBIDDEN => bail!("forbidden (403): {detail}"), - StatusCode::NOT_FOUND => bail!("not found (404): {method} {path}. {detail}"), - _ => bail!("{method} {path} failed with {status}: {detail}"), + StatusCode::FORBIDDEN => api_bail!("forbidden (403): {detail}"), + StatusCode::NOT_FOUND => api_bail!("not found (404): {method} {path}. {detail}"), + _ => api_bail!("{method} {path} failed with {status}: {detail}"), } } @@ -215,7 +216,7 @@ impl Client { let looks_like_html = trimmed.len() >= 2 && trimmed.starts_with('<') && !trimmed.starts_with(") let _ = config.save(); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::error::is_api_error; + + fn client() -> Client { + Client::new("https://example.cubecloud.dev", "sk-test").unwrap() + } + + fn finish(status: StatusCode, body: &str) -> Result { + client().finish_response( + &Method::GET, + "/api/v1/deployments", + status, + body.to_string(), + ) + } + + #[test] + fn unsuccessful_status_is_an_api_error() { + let err = finish(StatusCode::BAD_REQUEST, r#"{"message":"unknown field"}"#).unwrap_err(); + assert!(is_api_error(&err)); + assert!(err.to_string().contains("unknown field"), "{err}"); + } + + #[test] + fn every_mapped_status_is_an_api_error() { + for status in [ + StatusCode::UNAUTHORIZED, + StatusCode::FORBIDDEN, + StatusCode::NOT_FOUND, + StatusCode::INTERNAL_SERVER_ERROR, + ] { + let err = finish(status, "").unwrap_err(); + assert!(is_api_error(&err), "{status} should be an API error"); + } + } + + #[test] + fn web_app_html_instead_of_json_is_an_api_error() { + let err = finish(StatusCode::OK, "").unwrap_err(); + assert!(is_api_error(&err)); + } + + #[test] + fn successful_responses_still_parse() { + assert_eq!( + finish(StatusCode::OK, r#"{"items":[]}"#).unwrap(), + serde_json::json!({"items": []}) + ); + assert_eq!(finish(StatusCode::NO_CONTENT, "").unwrap(), Value::Null); + } +} diff --git a/rust/cube-cli/src/error.rs b/rust/cube-cli/src/error.rs new file mode 100644 index 0000000000000..35503e864ab6f --- /dev/null +++ b/rust/cube-cli/src/error.rs @@ -0,0 +1,72 @@ +/// An error that came from an API response — an unsuccessful status, or a +/// body the CLI could not make sense of — as opposed to a local failure or a +/// transport error (DNS, TLS, connection refused). +/// +/// It is a distinct error type so `main` can recognize it in the error chain +/// and suggest an update: skew between an older CLI and a newer API is a +/// common cause of otherwise puzzling API errors. +#[derive(Debug)] +pub struct ApiError(String); + +impl std::fmt::Display for ApiError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +impl std::error::Error for ApiError {} + +/// Build an [`ApiError`] as an `anyhow::Error`, ready to return or wrap in +/// further context. +pub fn api_error(message: impl Into) -> anyhow::Error { + anyhow::Error::new(ApiError(message.into())) +} + +/// `bail!` for API failures: returns an [`ApiError`] instead of a plain one. +macro_rules! api_bail { + ($($arg:tt)*) => { + return Err($crate::error::api_error(format!($($arg)*))) + }; +} + +pub(crate) use api_bail; + +/// True when `err`, or anything it wraps, came from an API response. +pub fn is_api_error(err: &anyhow::Error) -> bool { + err.chain().any(|e| e.is::()) +} + +#[cfg(test)] +mod tests { + use super::*; + use anyhow::{anyhow, Context as _}; + + #[test] + fn recognizes_api_errors() { + assert!(is_api_error(&api_error("GET /x failed with 400"))); + } + + #[test] + fn recognizes_api_errors_wrapped_in_context() { + let err = Err::<(), _>(api_error("GET /x failed with 400")) + .context("while listing deployments") + .unwrap_err(); + assert!(is_api_error(&err)); + } + + #[test] + fn ignores_other_errors() { + assert!(!is_api_error(&anyhow!("no config file found"))); + assert!(!is_api_error( + &anyhow!("connection refused").context("request to https://x failed") + )); + } + + #[test] + fn displays_the_message_it_was_built_with() { + assert_eq!( + api_error("GET /x failed with 400").to_string(), + "GET /x failed with 400" + ); + } +} diff --git a/rust/cube-cli/src/main.rs b/rust/cube-cli/src/main.rs index a6bd3181cddc5..e6cf069e8c10f 100644 --- a/rust/cube-cli/src/main.rs +++ b/rust/cube-cli/src/main.rs @@ -1,6 +1,7 @@ mod client; mod commands; mod config; +mod error; mod oauth; mod output; mod telemetry; @@ -281,6 +282,9 @@ async fn main() { let command_name = command.name(); let result = run(global, command).await; + // API failures are commonly caused by a CLI that lags the API, so they + // get an extra "try updating" hint below the error. + let api_error = result.as_ref().err().is_some_and(error::is_api_error); if !is_completion { let mut props = serde_json::Map::new(); @@ -295,11 +299,20 @@ async fn main() { } telemetry::flush().await; } - if let Some(check) = check { - update::print_notice(check).await; - } - if let Err(err) = result { + // Print the failure before consulting the release check: only the hint + // needs that answer, and a slow or unreachable GitHub must not sit between + // the user and the error they are waiting for. + if let Err(err) = &result { eprintln!("error: {err:#}"); + } + let outcome = update::resolve(check, api_error).await; + let announced = update::print_notice(&outcome); + if result.is_err() { + if api_error { + if let Some(hint) = update::api_error_hint(&outcome, announced) { + eprintln!("{hint}"); + } + } std::process::exit(1); } } diff --git a/rust/cube-cli/src/oauth.rs b/rust/cube-cli/src/oauth.rs index 812feafe642c0..27b3dbadb2d30 100644 --- a/rust/cube-cli/src/oauth.rs +++ b/rust/cube-cli/src/oauth.rs @@ -1,8 +1,10 @@ use std::time::{Duration, Instant}; -use anyhow::{anyhow, bail, Result}; +use anyhow::{bail, Result}; use serde::Deserialize; +use crate::error::{api_bail, api_error}; + /// OAuth 2.0 Device Authorization Grant (RFC 8628). /// /// Cube Cloud exposes a confidential authorization-code + device server under @@ -99,13 +101,16 @@ pub async fn request_device_code( let status = res.status(); let text = res.text().await.unwrap_or_default(); if !status.is_success() { - bail!( + api_bail!( "device authorization request failed ({status}) at {endpoint}: {}", text.trim() ); } - serde_json::from_str(&text) - .map_err(|e| anyhow!("could not parse device authorization response: {e}\n{text}")) + serde_json::from_str(&text).map_err(|e| { + api_error(format!( + "could not parse device authorization response: {e}\n{text}" + )) + }) } /// Step 3 — poll the token endpoint until the user approves (or it fails). @@ -144,7 +149,7 @@ pub async fn poll_for_token( if status.is_success() { return serde_json::from_str(&text) - .map_err(|e| anyhow!("could not parse token response: {e}\n{text}")); + .map_err(|e| api_error(format!("could not parse token response: {e}\n{text}"))); } // RFC 8628 §3.5: pending/slow_down keep polling; anything else is fatal. @@ -166,7 +171,7 @@ pub async fn poll_for_token( .unwrap_or_default() ), }, - Err(_) => bail!( + Err(_) => api_bail!( "token poll failed ({status}) at {endpoint}: {}", text.trim() ), @@ -196,10 +201,10 @@ pub async fn refresh( let status = res.status(); let text = res.text().await.unwrap_or_default(); if !status.is_success() { - bail!("token refresh failed ({status}): {}", text.trim()); + api_bail!("token refresh failed ({status}): {}", text.trim()); } serde_json::from_str(&text) - .map_err(|e| anyhow!("could not parse refresh response: {e}\n{text}")) + .map_err(|e| api_error(format!("could not parse refresh response: {e}\n{text}"))) } /// Best-effort attempt to open a URL in the user's browser (no extra deps). diff --git a/rust/cube-cli/src/update.rs b/rust/cube-cli/src/update.rs index bd6bf09679b3c..869f5dd8861e6 100644 --- a/rust/cube-cli/src/update.rs +++ b/rust/cube-cli/src/update.rs @@ -2,7 +2,7 @@ use std::io::IsTerminal as _; use std::time::Duration; use anyhow::{anyhow, bail, Result}; -use owo_colors::OwoColorize; +use owo_colors::{OwoColorize, Style}; use serde::Deserialize; /// GitHub repository that hosts CLI release assets. Overridable for testing. @@ -93,43 +93,138 @@ fn newer_than(candidate: &str, current: &str) -> bool { a > b } -/// Spawn a background check for a newer release. Await the returned handle -/// after the command finishes; it resolves to a printable notice, or `None`. -/// Failures (offline, rate limit) resolve silently to `None`. -pub fn spawn_check() -> tokio::task::JoinHandle> { +/// Outcome of the background release check. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum UpdateCheck { + /// A release newer than this binary is available. + Newer(String), + /// This binary is already on the latest release. + UpToDate, + /// Checks are opted out of via `CUBE_NO_UPDATE_CHECK`. + Disabled, + /// The check could not be completed — offline, rate limited, or slower + /// than the command it ran alongside. + Unknown, +} + +/// Spawn a background check for a newer release. Resolve the returned handle +/// with [`resolve`] once the command finishes. Failures (offline, rate limit) +/// resolve to [`UpdateCheck::Unknown`] rather than surfacing an error. +pub fn spawn_check() -> tokio::task::JoinHandle { tokio::spawn(async { if std::env::var_os("CUBE_NO_UPDATE_CHECK").is_some() { - return None; + return UpdateCheck::Disabled; } - let http = reqwest::Client::builder() + let Ok(http) = reqwest::Client::builder() .user_agent(concat!("cube-cli/", env!("CUBE_CLI_VERSION"))) .build() - .ok()?; - let release = latest_release(&http).await.ok()?; + else { + return UpdateCheck::Unknown; + }; + let Ok(release) = latest_release(&http).await else { + return UpdateCheck::Unknown; + }; let latest = release.version().to_string(); if newer_than(&latest, CURRENT_VERSION) { - Some(format!( - "\n{} {} → {}\nRun {} to install it.", - "A new release of Cube CLI is available:".yellow(), - CURRENT_VERSION.dimmed(), - latest.bold().green(), - "cube update".bold().cyan(), - )) + UpdateCheck::Newer(latest) } else { - None + UpdateCheck::UpToDate } }) } -/// Print a pending update notice (best effort, never blocks long). -pub async fn print_notice(handle: tokio::task::JoinHandle>) { - if !std::io::stderr().is_terminal() { - return; +/// Await the background check, but only when its answer will be used: the +/// notice is interactive-only and the hint only runs on an API failure, so a +/// piped command that succeeded must not pay for a GitHub round trip it will +/// throw away. +pub async fn resolve( + handle: Option>, + api_error: bool, +) -> UpdateCheck { + let Some(handle) = handle else { + return UpdateCheck::Unknown; + }; + if !wanted(api_error, std::io::stderr().is_terminal()) { + return UpdateCheck::Unknown; } - // The check runs concurrently with the command; give a short grace + // The check runs concurrently with the command; give it a short grace // period in case the command finished faster than the API call. - if let Ok(Ok(Some(notice))) = tokio::time::timeout(Duration::from_millis(1500), handle).await { - eprintln!("{notice}"); + match tokio::time::timeout(Duration::from_millis(1500), handle).await { + Ok(Ok(outcome)) => outcome, + _ => UpdateCheck::Unknown, + } +} + +/// Whether anything will read the check's answer — [`print_notice`] on a +/// terminal, or [`api_error_hint`] under an API failure. +fn wanted(api_error: bool, interactive: bool) -> bool { + api_error || interactive +} + +/// Print the "new release available" notice, if there is one to print. +/// Returns whether it was printed. Interactive terminals only — an unprompted +/// nag has no place in piped or logged output. +pub fn print_notice(outcome: &UpdateCheck) -> bool { + if !std::io::stderr().is_terminal() { + return false; + } + let UpdateCheck::Newer(latest) = outcome else { + return false; + }; + eprintln!( + "\n{} {} → {}\nRun {} to install it.", + "A new release of Cube CLI is available:".yellow(), + CURRENT_VERSION.dimmed(), + latest.bold().green(), + "cube update".bold().cyan(), + ); + true +} + +/// Hint printed under an API error. A CLI that lags the API is a common cause +/// of otherwise puzzling API errors, so point at `cube update` — but only when +/// this binary really is behind: telling someone already on the latest release +/// to update is noise that teaches them to ignore the hint. +/// +/// `announced` says whether [`print_notice`] just reported the same release, so +/// the hint can point back at it instead of repeating the version and command. +/// +/// Unlike the notice, this is not limited to interactive terminals: it is +/// attached to a failure the user asked for rather than an unprompted nag, and +/// a stale pinned CLI in CI is exactly where the advice pays off. +pub fn api_error_hint(outcome: &UpdateCheck, announced: bool) -> Option { + hint_for(outcome, announced, std::io::stderr().is_terminal()) +} + +/// Split out from [`api_error_hint`] so the policy can be tested without +/// touching process-wide environment or terminal state. +fn hint_for(outcome: &UpdateCheck, announced: bool, color: bool) -> Option { + let (label, emphasis) = if color { + (Style::new().yellow(), Style::new().bold().cyan()) + } else { + (Style::new(), Style::new()) + }; + let hint = label.style("hint:"); + match outcome { + // Already current, or the user opted out of update checks: nothing to + // suggest that could plausibly resolve the error. + UpdateCheck::UpToDate | UpdateCheck::Disabled => None, + UpdateCheck::Newer(_) if announced => Some(format!( + "{hint} this request failed on the API side — the newer release above may already fix it" + )), + UpdateCheck::Newer(latest) => Some(format!( + "{hint} this request failed on the API side, and Cube CLI {} is available — run {} \ + to upgrade from {CURRENT_VERSION}, then try again", + emphasis.style(latest), + emphasis.style("cube update"), + )), + // Could not find out whether a newer release exists, so the advice is + // worth giving but not worth asserting. + UpdateCheck::Unknown => Some(format!( + "{hint} this request failed on the API side; a newer release may already fix it — run \ + {} to check, then try again", + emphasis.style("cube update"), + )), } } @@ -137,6 +232,55 @@ pub async fn print_notice(handle: tokio::task::JoinHandle>) { mod tests { use super::*; + fn hint(outcome: &UpdateCheck, announced: bool) -> Option { + hint_for(outcome, announced, false) + } + + #[test] + fn a_piped_command_that_succeeded_does_not_wait_on_the_check() { + // Neither consumer would read the answer, so there is nothing to await. + assert!(!wanted(false, false)); + // A terminal gets the notice; an API failure gets the hint. + assert!(wanted(false, true)); + assert!(wanted(true, false)); + } + + #[test] + fn a_current_cli_is_never_told_to_update() { + assert_eq!(hint(&UpdateCheck::UpToDate, false), None); + assert_eq!(hint(&UpdateCheck::Disabled, false), None); + } + + #[test] + fn a_stale_cli_is_pointed_at_the_new_release() { + let hint = hint(&UpdateCheck::Newer("9.9.9".into()), false).unwrap(); + assert!(hint.contains("cube update"), "{hint}"); + assert!(hint.contains("9.9.9"), "{hint}"); + assert!(hint.contains(CURRENT_VERSION), "{hint}"); + } + + #[test] + fn an_announced_release_is_referenced_rather_than_repeated() { + let hint = hint(&UpdateCheck::Newer("9.9.9".into()), true).unwrap(); + assert!(!hint.contains("cube update"), "{hint}"); + assert!(hint.contains("above"), "{hint}"); + } + + #[test] + fn an_undetermined_check_suggests_looking_rather_than_asserting() { + let hint = hint(&UpdateCheck::Unknown, false).unwrap(); + assert!(hint.contains("cube update"), "{hint}"); + assert!(hint.contains("may"), "{hint}"); + } + + #[test] + fn color_is_applied_only_when_asked_for() { + let plain = hint_for(&UpdateCheck::Unknown, false, false).unwrap(); + let colored = hint_for(&UpdateCheck::Unknown, false, true).unwrap(); + assert!(!plain.contains('\u{1b}'), "{plain}"); + assert!(colored.contains('\u{1b}'), "{colored}"); + } + #[test] fn newer_than_compares_numerically() { assert!(newer_than("1.7.10", "1.7.2"));