From 2c4a0aef01ffd728f0876835e54e76091de0155a Mon Sep 17 00:00:00 2001 From: hojinyoo Date: Sun, 7 Jun 2026 11:02:36 +0900 Subject: [PATCH 1/5] fix(cli): return non-zero exit code on command errors Some handlers printed an error then returned Ok(()), so the process exited 0 on failure -- unsafe for orchestrators trusting $?. The central plumbing in main.rs already maps a handler Err to a non-zero code and emits the {"error":true,...} JSON body on stderr; the fix is to stop swallowing: - bulk.rs resolution failures (user/label) now return Err, preserving the underlying CliError kind/retry hint. - The four bulk handlers, the issues.rs multi-get, and the comments.rs multi-issue listing return Err if any item failed (any-item-failure => non-zero), while still printing per-item results on stdout. - Real fetch errors keep their ErrorKind: a rate-limited (429) failure stays exit 4 instead of being collapsed to NotFound (exit 2), so retry logic works. Adds offline exit-code regression tests (run_cli_isolated) and unit tests for the pure aggregation helpers (bulk_exit_status / multi_get_status / comments_status). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/commands/bulk.rs | 145 +++++++++++++++++++++++++++++---------- src/commands/comments.rs | 68 ++++++++++++++++-- src/commands/issues.rs | 88 +++++++++++++++++++++++- tests/cli_tests.rs | 75 ++++++++++++++++++++ 4 files changed, 331 insertions(+), 45 deletions(-) diff --git a/src/commands/bulk.rs b/src/commands/bulk.rs index ab1fa47..ec84e61 100644 --- a/src/commands/bulk.rs +++ b/src/commands/bulk.rs @@ -10,6 +10,7 @@ use tokio::sync::Mutex; use crate::api::{resolve_label_id, resolve_state_id, resolve_user_id, LinearClient}; use crate::display_options; +use crate::error::CliError; use crate::output::{print_json_owned, OutputOptions}; use crate::text::truncate; @@ -148,8 +149,7 @@ async fn bulk_update_state(state: &str, issues: Vec, output: &OutputOpti .collect() .await; print_summary(&results, "state updated", output); - - Ok(()) + bulk_exit_status(&results) } async fn bulk_assign(user: &str, issues: Vec, output: &OutputOptions) -> Result<()> { @@ -168,21 +168,13 @@ async fn bulk_assign(user: &str, issues: Vec, output: &OutputOptions) -> let client = LinearClient::new()?; - // Resolve the user ID once upfront - let user_id = match resolve_user_id(&client, user, &output.cache).await { - Ok(id) => id, - Err(e) => { - if output.is_json() || output.has_template() { - print_json_owned( - json!({ "error": format!("Failed to resolve user '{}': {}", user, e), "results": [] }), - output, - )?; - } else { - println!("{} Failed to resolve user '{}': {}", "x".red(), user, e); - } - return Ok(()); - } - }; + // Resolve the user ID once upfront. Resolution failure is a hard error + // (no issues can be processed), so it must return Err — the central handler + // in main.rs prints the message / JSON error body on stderr and sets a + // non-zero exit code. + let user_id = resolve_user_id(&client, user, &output.cache) + .await + .map_err(|e| wrap_resolve_error("user", user, e))?; let results: Vec<_> = stream::iter(issues.iter()) .map(|issue_id| { @@ -195,8 +187,7 @@ async fn bulk_assign(user: &str, issues: Vec, output: &OutputOptions) -> .collect() .await; print_summary(&results, "assigned", output); - - Ok(()) + bulk_exit_status(&results) } async fn bulk_label(label: &str, issues: Vec, output: &OutputOptions) -> Result<()> { @@ -215,21 +206,13 @@ async fn bulk_label(label: &str, issues: Vec, output: &OutputOptions) -> let client = LinearClient::new()?; - // Resolve the label ID once upfront - let label_id = match resolve_label_id(&client, label, &output.cache).await { - Ok(id) => id, - Err(e) => { - if output.is_json() || output.has_template() { - print_json_owned( - json!({ "error": format!("Failed to resolve label '{}': {}", label, e), "results": [] }), - output, - )?; - } else { - println!("{} Failed to resolve label '{}': {}", "x".red(), label, e); - } - return Ok(()); - } - }; + // Resolve the label ID once upfront. Resolution failure is a hard error + // (no issues can be processed), so it must return Err — the central handler + // in main.rs prints the message / JSON error body on stderr and sets a + // non-zero exit code. + let label_id = resolve_label_id(&client, label, &output.cache) + .await + .map_err(|e| wrap_resolve_error("label", label, e))?; let results: Vec<_> = stream::iter(issues.iter()) .map(|issue_id| { @@ -242,8 +225,7 @@ async fn bulk_label(label: &str, issues: Vec, output: &OutputOptions) -> .collect() .await; print_summary(&results, "labeled", output); - - Ok(()) + bulk_exit_status(&results) } async fn bulk_unassign(issues: Vec, output: &OutputOptions) -> Result<()> { @@ -267,8 +249,7 @@ async fn bulk_unassign(issues: Vec, output: &OutputOptions) -> Result<() .collect() .await; print_summary(&results, "unassigned", output); - - Ok(()) + bulk_exit_status(&results) } fn missing_bulk_issues(example: &str) -> Result<()> { @@ -620,3 +601,91 @@ fn print_summary(results: &[BulkResult], action: &str, output: &OutputOptions) { } ); } + +/// Exit status for a bulk operation: `Err` if any item failed, so the process +/// exits non-zero. Per-item results are printed by `print_summary` first +/// (stdout); the aggregate error is emitted by `main.rs` on stderr. Any single +/// failed item is enough to fail the command — partial success is failure for +/// an orchestrator trusting `$?`. +fn bulk_exit_status(results: &[BulkResult]) -> Result<()> { + let failed = results.iter().filter(|r| !r.success).count(); + if failed > 0 { + return Err(CliError::general(format!( + "{} of {} operations failed", + failed, + results.len() + )) + .into()); + } + Ok(()) +} + +/// Wrap a resolution failure, preserving the original `CliError` kind/retry hint +/// where present so the exit code stays accurate (e.g. a rate-limited resolution +/// keeps code 4 + retry_after instead of degrading to a string-matched code). +fn wrap_resolve_error(kind: &str, name: &str, e: anyhow::Error) -> anyhow::Error { + if let Some(cli) = e.downcast_ref::() { + let mut wrapped = CliError::new(cli.kind, format!("Failed to resolve {} '{}': {}", kind, name, e)) + .with_retry_after(cli.retry_after); + if let Some(details) = &cli.details { + wrapped = wrapped.with_details(details.clone()); + } + wrapped.into() + } else { + anyhow::anyhow!("Failed to resolve {} '{}': {}", kind, name, e) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn result(success: bool) -> BulkResult { + BulkResult { + issue_id: "LIN-1".to_string(), + success, + identifier: None, + error: if success { + None + } else { + Some("boom".to_string()) + }, + } + } + + #[test] + fn bulk_exit_status_ok_when_all_succeed_or_empty() { + assert!(bulk_exit_status(&[]).is_ok()); + assert!(bulk_exit_status(&[result(true), result(true)]).is_ok()); + } + + #[test] + fn bulk_exit_status_errs_on_any_failure() { + let err = bulk_exit_status(&[result(true), result(false)]).unwrap_err(); + assert_eq!( + err.downcast_ref::().expect("CliError").code(), + 1 + ); + } + + #[test] + fn bulk_exit_status_errs_when_all_fail() { + assert!(bulk_exit_status(&[result(false), result(false)]).is_err()); + } + + #[test] + fn wrap_resolve_error_preserves_rate_limited_kind() { + let original = CliError::rate_limited("429").with_retry_after(Some(7)); + let wrapped = wrap_resolve_error("label", "Bug", original.into()); + let cli = wrapped.downcast_ref::().expect("CliError"); + assert_eq!(cli.code(), 4); + assert_eq!(cli.retry_after, Some(7)); + } + + #[test] + fn wrap_resolve_error_keeps_message_for_plain_error() { + let wrapped = wrap_resolve_error("user", "nobody", anyhow::anyhow!("not found")); + assert!(wrapped.to_string().contains("nobody")); + assert!(wrapped.to_string().contains("not found")); + } +} diff --git a/src/commands/comments.rs b/src/commands/comments.rs index 8e64519..2a424f9 100644 --- a/src/commands/comments.rs +++ b/src/commands/comments.rs @@ -7,6 +7,7 @@ use tabled::{Table, Tabled}; use crate::api::LinearClient; use crate::display_options; +use crate::error::{CliError, ErrorKind}; use crate::input::read_ids_from_stdin; use crate::output::{ ensure_non_empty, filter_values, print_json, print_json_owned, sort_values, OutputOptions, @@ -109,7 +110,7 @@ async fn list_comments(issue_ids: &[String], output: &OutputOptions) -> Result<( Err(e) => Err((id, e)), } } - Ok(_) => Err((id, anyhow::anyhow!("Issue not found"))), + Ok(_) => Err((id.clone(), CliError::not_found(format!("Issue not found: {id}")).into())), Err(e) => Err((id, e)), } } @@ -119,12 +120,29 @@ async fn list_comments(issue_ids: &[String], output: &OutputOptions) -> Result<( .await; let mut issues = Vec::new(); + let mut missing: Vec = Vec::new(); + // A real fetch error (network/auth/rate-limit) takes priority for the exit + // code and keeps its original kind; genuinely-missing issues map to NotFound. + let mut first_error: Option = None; for result in fetched { match result { Ok((_id, issue)) => issues.push(issue), - Err((id, _e)) => { + Err((id, e)) => { + let is_missing = e + .downcast_ref::() + .map(|c| c.kind == ErrorKind::NotFound) + .unwrap_or(false); if !output.is_json() && !output.has_template() { - eprintln!("{} Issue not found: {}", "!".yellow(), id); + if is_missing { + eprintln!("{} Issue not found: {}", "!".yellow(), id); + } else { + eprintln!("{} Error fetching {}: {}", "!".red(), id, e); + } + } + if is_missing { + missing.push(id); + } else if first_error.is_none() { + first_error = Some(e); } } } @@ -137,7 +155,7 @@ async fn list_comments(issue_ids: &[String], output: &OutputOptions) -> Result<( } else { print_json_owned(serde_json::json!(issues), output)?; } - return Ok(()); + return comments_status(first_error, &missing); } for (idx, issue) in issues.iter().enumerate() { @@ -202,7 +220,23 @@ async fn list_comments(issue_ids: &[String], output: &OutputOptions) -> Result<( println!("\n{} comments", comments.len()); } - Ok(()) + comments_status(first_error, &missing) +} + +/// Aggregate exit status for a multi-issue comment listing. +/// +/// A real fetch error (passed through unchanged) takes priority so its exit +/// code/kind survives; otherwise any genuinely-missing issues map to NotFound, +/// and an all-found request returns Ok. +fn comments_status(first_error: Option, missing: &[String]) -> Result<()> { + if let Some(e) = first_error { + return Err(e); + } + if missing.is_empty() { + Ok(()) + } else { + Err(CliError::not_found(format!("Issue(s) not found: {}", missing.join(", "))).into()) + } } async fn fetch_issue_meta(client: &LinearClient, issue_id: &str) -> Result { @@ -335,6 +369,30 @@ mod tests { "badtitle" ); } + + #[test] + fn comments_status_ok_when_nothing_failed() { + assert!(comments_status(None, &[]).is_ok()); + } + + #[test] + fn comments_status_notfound_when_only_missing() { + let err = comments_status(None, &["LIN-9".to_string()]).unwrap_err(); + assert_eq!(err.downcast_ref::().expect("CliError").code(), 2); + } + + #[test] + fn comments_status_preserves_real_error_over_missing() { + // A real fetch error keeps its kind (rate-limited => 4), not NotFound(2). + let err = comments_status( + Some(CliError::rate_limited("429").with_retry_after(Some(5)).into()), + &["LIN-9".to_string()], + ) + .unwrap_err(); + let cli = err.downcast_ref::().expect("CliError"); + assert_eq!(cli.code(), 4); + assert_eq!(cli.retry_after, Some(5)); + } } async fn update_comment(id: &str, body: &str, output: &OutputOptions) -> Result<()> { diff --git a/src/commands/issues.rs b/src/commands/issues.rs index ff84fe9..593919d 100644 --- a/src/commands/issues.rs +++ b/src/commands/issues.rs @@ -11,6 +11,7 @@ use crate::api::{ }; use crate::cache::CacheOptions; use crate::display_options; +use crate::error::CliError; use crate::input::read_ids_from_stdin; use crate::output::{ ensure_non_empty, filter_values, print_json, print_json_owned, sort_values, OutputOptions, @@ -804,6 +805,12 @@ async fn get_issues( .collect() .await; + // Exit status: any missing or errored issue makes the command exit + // non-zero, while the issues that WERE found are still printed. Computed + // before printing so `results` can be consumed by the table path; the + // returned error owns its message so it does not borrow `results`. + let status = multi_get_status(&results); + // JSON output: array of issues if output.is_json() || output.has_template() { let issues: Vec<_> = results @@ -820,7 +827,7 @@ async fn get_issues( }) .collect(); print_json_owned(serde_json::json!(issues), output)?; - return Ok(()); + return status; } // Table output @@ -845,7 +852,50 @@ async fn get_issues( } } - Ok(()) + status +} + +/// Aggregate exit status for a multi-issue fetch. +/// +/// A real fetch error (network, auth, rate-limit) takes priority and keeps its +/// original error kind, so the exit code stays accurate (e.g. 4/RateLimited +/// remains retryable for an orchestrator). Only a genuinely missing issue — a +/// `null` issue node in an otherwise-successful response — maps to NotFound. +fn multi_get_status(results: &[(String, Result)]) -> Result<()> { + // Real errors win over plain not-found, and preserve their kind/retry hint. + if let Some((id, e)) = results.iter().find_map(|(id, r)| match r { + Err(e) => Some((id, e)), + Ok(_) => None, + }) { + return Err(wrap_fetch_error(id, e)); + } + + let missing: Vec<&str> = results + .iter() + .filter(|(_, r)| matches!(r, Ok(data) if data["data"]["issue"].is_null())) + .map(|(id, _)| id.as_str()) + .collect(); + + if missing.is_empty() { + Ok(()) + } else { + Err(CliError::not_found(format!("Issue(s) not found: {}", missing.join(", "))).into()) + } +} + +/// Wrap a per-item fetch error for the aggregate exit, preserving the original +/// `CliError` kind/retry hint where present so the exit code is accurate. +fn wrap_fetch_error(id: &str, e: &anyhow::Error) -> anyhow::Error { + if let Some(cli) = e.downcast_ref::() { + let mut wrapped = + CliError::new(cli.kind, format!("Error fetching {}: {}", id, e)).with_retry_after(cli.retry_after); + if let Some(details) = &cli.details { + wrapped = wrapped.with_details(details.clone()); + } + wrapped.into() + } else { + anyhow::anyhow!("Error fetching {}: {}", id, e) + } } async fn open_issue(id: &str) -> Result<()> { @@ -2266,6 +2316,40 @@ async fn transfer_issue(id: &str, team: &str) -> Result<()> { mod tests { use super::*; + #[test] + fn multi_get_status_ok_when_all_found() { + let results = vec![( + "LIN-1".to_string(), + Ok(json!({ "data": { "issue": { "id": "x" } } })), + )]; + assert!(multi_get_status(&results).is_ok()); + assert!(multi_get_status(&[]).is_ok()); + } + + #[test] + fn multi_get_status_notfound_when_issue_null() { + let results = vec![("LIN-9".to_string(), Ok(json!({ "data": { "issue": null } })))]; + let err = multi_get_status(&results).unwrap_err(); + assert_eq!(err.downcast_ref::().expect("CliError").code(), 2); + } + + #[test] + fn multi_get_status_preserves_rate_limited_kind_over_notfound() { + // A real fetch error must NOT be collapsed to NotFound(2): a 429 stays + // RateLimited(4) so an orchestrator keeps retrying. + let results: Vec<(String, Result)> = vec![ + ("LIN-1".to_string(), Ok(json!({ "data": { "issue": null } }))), + ( + "LIN-2".to_string(), + Err(CliError::rate_limited("429").with_retry_after(Some(3)).into()), + ), + ]; + let err = multi_get_status(&results).unwrap_err(); + let cli = err.downcast_ref::().expect("CliError"); + assert_eq!(cli.code(), 4); + assert_eq!(cli.retry_after, Some(3)); + } + #[test] fn test_format_history_state_change() { let entry = serde_json::json!({ diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index a2aa9ce..204c097 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -2142,3 +2142,78 @@ fn test_yes_flag_works_with_subcommand() { "--yes flag should not interfere with subcommand parsing" ); } + +// ---- Exit-code / JSON-error contract (agent ergonomics) ---- +// +// These verify the central contract offline: a failing command exits non-zero, +// and under `--output json` the `{"error":true,...}` body is emitted on stderr +// while stdout stays clean. They use the `bulk label` empty-issues path, which +// is a logical failure reached before any network/keyring/config access, so the +// assertions are deterministic with no credentials. The credential-dependent +// handler-swallow paths (bulk resolution returning Err, mixed batch failures) +// are verified end-to-end with live keys, since the harness does not mock the +// Linear API. + +/// Run the CLI with an isolated, empty config home and no ambient credentials, +/// so error paths are deterministic and fully offline. Each call gets a unique +/// HOME so parallel tests never share config/cache state. +fn run_cli_isolated(args: &[&str]) -> (i32, String, String) { + use std::sync::atomic::{AtomicUsize, Ordering}; + static COUNTER: AtomicUsize = AtomicUsize::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let tmp = std::env::temp_dir().join(format!("linear-cli-isolated-{}-{}", std::process::id(), n)); + let _ = std::fs::create_dir_all(&tmp); + let output = Command::new(env!("CARGO_BIN_EXE_linear-cli")) + .args(args) + .env("HOME", &tmp) + .env("XDG_CONFIG_HOME", &tmp) + .env_remove("LINEAR_API_KEY") + .env_remove("LINEAR_CLI_PROFILE") + .output() + .expect("Failed to execute command"); + + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + let code = output.status.code().unwrap_or(-1); + + (code, stdout, stderr) +} + +#[test] +fn test_failing_command_exits_nonzero() { + // `bulk label ` with no issues is a logical failure; it must NOT + // exit 0 (the agent-ergonomics regression: errors used to exit 0). + let (code, _stdout, stderr) = run_cli_isolated(&["bulk", "label", "Bug"]); + assert_ne!(code, 0, "a failing command must exit non-zero; stderr={stderr}"); + assert!( + (1..=4).contains(&code), + "exit code should be a documented error code (1-4), got {code}" + ); +} + +#[test] +fn test_json_error_body_emitted_on_stderr_not_stdout() { + // On the empty-issues path stdout is necessarily empty, so the stdout-clean + // assertion below is trivially satisfied here; the meaningful "results on + // stdout + error on stderr" stream split and the any-item-failure + // aggregation are unit-tested in src/commands/{bulk,issues,comments}.rs + // (bulk_exit_status / multi_get_status / comments_status). + let (code, stdout, stderr) = + run_cli_isolated(&["bulk", "label", "Bug", "--output", "json"]); + assert_ne!(code, 0, "failing command must exit non-zero"); + assert!( + stderr.contains("\"error\":true"), + "stderr should carry the JSON error body, got: {stderr}" + ); + assert!( + !stdout.contains("\"error\":true"), + "stdout must not carry a second JSON error body, got: {stdout}" + ); +} + +#[test] +fn test_successful_command_exits_zero() { + let (code, stdout, _stderr) = run_cli_isolated(&["--version"]); + assert_eq!(code, 0, "a successful command must exit 0"); + assert!(!stdout.is_empty()); +} From 3f12c1e70beb9a7a5e140c507c90177c0dc0ff53 Mon Sep 17 00:00:00 2001 From: hojinyoo Date: Tue, 9 Jun 2026 00:30:09 +0900 Subject: [PATCH 2/5] refactor(cli): consolidate CliError re-wrapping into error::rewrap_with_message Address review feedback on #31: - Move the duplicated CliError preservation logic (same kind/details/retry_after, new message) out of bulk.rs::wrap_resolve_error and issues.rs::wrap_fetch_error into one canonical `error::rewrap_with_message` helper at the error boundary. Command modules now just collect results, print them, and return the aggregate status; the helper keeps the exit code accurate (e.g. a 429 stays exit 4). - Run `cargo fmt` on the changed files. - Trim verbose comments. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/commands/bulk.rs | 63 ++++++-------------------------- src/commands/comments.rs | 21 ++++++----- src/commands/issues.rs | 55 ++++++++++++---------------- src/error.rs | 77 ++++++++++++++++++++++++++++++++++++++++ tests/cli_tests.rs | 40 +++++++++------------ 5 files changed, 138 insertions(+), 118 deletions(-) diff --git a/src/commands/bulk.rs b/src/commands/bulk.rs index ec84e61..0a7f402 100644 --- a/src/commands/bulk.rs +++ b/src/commands/bulk.rs @@ -10,7 +10,7 @@ use tokio::sync::Mutex; use crate::api::{resolve_label_id, resolve_state_id, resolve_user_id, LinearClient}; use crate::display_options; -use crate::error::CliError; +use crate::error::{self, CliError}; use crate::output::{print_json_owned, OutputOptions}; use crate::text::truncate; @@ -168,13 +168,12 @@ async fn bulk_assign(user: &str, issues: Vec, output: &OutputOptions) -> let client = LinearClient::new()?; - // Resolve the user ID once upfront. Resolution failure is a hard error - // (no issues can be processed), so it must return Err — the central handler - // in main.rs prints the message / JSON error body on stderr and sets a - // non-zero exit code. + // Resolution failure is a hard error: return Err, preserving the kind. let user_id = resolve_user_id(&client, user, &output.cache) .await - .map_err(|e| wrap_resolve_error("user", user, e))?; + .map_err(|e| { + error::rewrap_with_message(&e, format!("Failed to resolve user '{user}': {e}")) + })?; let results: Vec<_> = stream::iter(issues.iter()) .map(|issue_id| { @@ -206,13 +205,12 @@ async fn bulk_label(label: &str, issues: Vec, output: &OutputOptions) -> let client = LinearClient::new()?; - // Resolve the label ID once upfront. Resolution failure is a hard error - // (no issues can be processed), so it must return Err — the central handler - // in main.rs prints the message / JSON error body on stderr and sets a - // non-zero exit code. + // Resolution failure is a hard error: return Err, preserving the kind. let label_id = resolve_label_id(&client, label, &output.cache) .await - .map_err(|e| wrap_resolve_error("label", label, e))?; + .map_err(|e| { + error::rewrap_with_message(&e, format!("Failed to resolve label '{label}': {e}")) + })?; let results: Vec<_> = stream::iter(issues.iter()) .map(|issue_id| { @@ -602,11 +600,7 @@ fn print_summary(results: &[BulkResult], action: &str, output: &OutputOptions) { ); } -/// Exit status for a bulk operation: `Err` if any item failed, so the process -/// exits non-zero. Per-item results are printed by `print_summary` first -/// (stdout); the aggregate error is emitted by `main.rs` on stderr. Any single -/// failed item is enough to fail the command — partial success is failure for -/// an orchestrator trusting `$?`. +/// `Err` if any item failed, so partial failures still exit non-zero. fn bulk_exit_status(results: &[BulkResult]) -> Result<()> { let failed = results.iter().filter(|r| !r.success).count(); if failed > 0 { @@ -620,22 +614,6 @@ fn bulk_exit_status(results: &[BulkResult]) -> Result<()> { Ok(()) } -/// Wrap a resolution failure, preserving the original `CliError` kind/retry hint -/// where present so the exit code stays accurate (e.g. a rate-limited resolution -/// keeps code 4 + retry_after instead of degrading to a string-matched code). -fn wrap_resolve_error(kind: &str, name: &str, e: anyhow::Error) -> anyhow::Error { - if let Some(cli) = e.downcast_ref::() { - let mut wrapped = CliError::new(cli.kind, format!("Failed to resolve {} '{}': {}", kind, name, e)) - .with_retry_after(cli.retry_after); - if let Some(details) = &cli.details { - wrapped = wrapped.with_details(details.clone()); - } - wrapped.into() - } else { - anyhow::anyhow!("Failed to resolve {} '{}': {}", kind, name, e) - } -} - #[cfg(test)] mod tests { use super::*; @@ -662,30 +640,11 @@ mod tests { #[test] fn bulk_exit_status_errs_on_any_failure() { let err = bulk_exit_status(&[result(true), result(false)]).unwrap_err(); - assert_eq!( - err.downcast_ref::().expect("CliError").code(), - 1 - ); + assert_eq!(err.downcast_ref::().expect("CliError").code(), 1); } #[test] fn bulk_exit_status_errs_when_all_fail() { assert!(bulk_exit_status(&[result(false), result(false)]).is_err()); } - - #[test] - fn wrap_resolve_error_preserves_rate_limited_kind() { - let original = CliError::rate_limited("429").with_retry_after(Some(7)); - let wrapped = wrap_resolve_error("label", "Bug", original.into()); - let cli = wrapped.downcast_ref::().expect("CliError"); - assert_eq!(cli.code(), 4); - assert_eq!(cli.retry_after, Some(7)); - } - - #[test] - fn wrap_resolve_error_keeps_message_for_plain_error() { - let wrapped = wrap_resolve_error("user", "nobody", anyhow::anyhow!("not found")); - assert!(wrapped.to_string().contains("nobody")); - assert!(wrapped.to_string().contains("not found")); - } } diff --git a/src/commands/comments.rs b/src/commands/comments.rs index 2a424f9..504f51a 100644 --- a/src/commands/comments.rs +++ b/src/commands/comments.rs @@ -110,7 +110,10 @@ async fn list_comments(issue_ids: &[String], output: &OutputOptions) -> Result<( Err(e) => Err((id, e)), } } - Ok(_) => Err((id.clone(), CliError::not_found(format!("Issue not found: {id}")).into())), + Ok(_) => Err(( + id.clone(), + CliError::not_found(format!("Issue not found: {id}")).into(), + )), Err(e) => Err((id, e)), } } @@ -121,8 +124,7 @@ async fn list_comments(issue_ids: &[String], output: &OutputOptions) -> Result<( let mut issues = Vec::new(); let mut missing: Vec = Vec::new(); - // A real fetch error (network/auth/rate-limit) takes priority for the exit - // code and keeps its original kind; genuinely-missing issues map to NotFound. + // Real fetch errors keep their kind; missing issues map to NotFound. let mut first_error: Option = None; for result in fetched { match result { @@ -223,11 +225,8 @@ async fn list_comments(issue_ids: &[String], output: &OutputOptions) -> Result<( comments_status(first_error, &missing) } -/// Aggregate exit status for a multi-issue comment listing. -/// -/// A real fetch error (passed through unchanged) takes priority so its exit -/// code/kind survives; otherwise any genuinely-missing issues map to NotFound, -/// and an all-found request returns Ok. +/// Aggregate exit status: a real fetch error (passed through unchanged) wins so +/// its kind survives; otherwise missing issues map to NotFound. fn comments_status(first_error: Option, missing: &[String]) -> Result<()> { if let Some(e) = first_error { return Err(e); @@ -385,7 +384,11 @@ mod tests { fn comments_status_preserves_real_error_over_missing() { // A real fetch error keeps its kind (rate-limited => 4), not NotFound(2). let err = comments_status( - Some(CliError::rate_limited("429").with_retry_after(Some(5)).into()), + Some( + CliError::rate_limited("429") + .with_retry_after(Some(5)) + .into(), + ), &["LIN-9".to_string()], ) .unwrap_err(); diff --git a/src/commands/issues.rs b/src/commands/issues.rs index 593919d..b092ecc 100644 --- a/src/commands/issues.rs +++ b/src/commands/issues.rs @@ -11,7 +11,7 @@ use crate::api::{ }; use crate::cache::CacheOptions; use crate::display_options; -use crate::error::CliError; +use crate::error::{self, CliError}; use crate::input::read_ids_from_stdin; use crate::output::{ ensure_non_empty, filter_values, print_json, print_json_owned, sort_values, OutputOptions, @@ -805,10 +805,8 @@ async fn get_issues( .collect() .await; - // Exit status: any missing or errored issue makes the command exit - // non-zero, while the issues that WERE found are still printed. Computed - // before printing so `results` can be consumed by the table path; the - // returned error owns its message so it does not borrow `results`. + // Any missing/errored issue exits non-zero; the issues found are still + // printed. Computed before printing so `results` can be consumed below. let status = multi_get_status(&results); // JSON output: array of issues @@ -855,19 +853,18 @@ async fn get_issues( status } -/// Aggregate exit status for a multi-issue fetch. -/// -/// A real fetch error (network, auth, rate-limit) takes priority and keeps its -/// original error kind, so the exit code stays accurate (e.g. 4/RateLimited -/// remains retryable for an orchestrator). Only a genuinely missing issue — a -/// `null` issue node in an otherwise-successful response — maps to NotFound. +/// Aggregate exit status for a multi-issue fetch. A real fetch error takes +/// priority and keeps its kind (e.g. RateLimited stays retryable); only a `null` +/// issue node maps to NotFound. fn multi_get_status(results: &[(String, Result)]) -> Result<()> { - // Real errors win over plain not-found, and preserve their kind/retry hint. if let Some((id, e)) = results.iter().find_map(|(id, r)| match r { Err(e) => Some((id, e)), Ok(_) => None, }) { - return Err(wrap_fetch_error(id, e)); + return Err(error::rewrap_with_message( + e, + format!("Error fetching {id}: {e}"), + )); } let missing: Vec<&str> = results @@ -883,21 +880,6 @@ fn multi_get_status(results: &[(String, Result)]) -> Result<( } } -/// Wrap a per-item fetch error for the aggregate exit, preserving the original -/// `CliError` kind/retry hint where present so the exit code is accurate. -fn wrap_fetch_error(id: &str, e: &anyhow::Error) -> anyhow::Error { - if let Some(cli) = e.downcast_ref::() { - let mut wrapped = - CliError::new(cli.kind, format!("Error fetching {}: {}", id, e)).with_retry_after(cli.retry_after); - if let Some(details) = &cli.details { - wrapped = wrapped.with_details(details.clone()); - } - wrapped.into() - } else { - anyhow::anyhow!("Error fetching {}: {}", id, e) - } -} - async fn open_issue(id: &str) -> Result<()> { let client = LinearClient::new()?; let query = r#" @@ -2328,20 +2310,27 @@ mod tests { #[test] fn multi_get_status_notfound_when_issue_null() { - let results = vec![("LIN-9".to_string(), Ok(json!({ "data": { "issue": null } })))]; + let results = vec![( + "LIN-9".to_string(), + Ok(json!({ "data": { "issue": null } })), + )]; let err = multi_get_status(&results).unwrap_err(); assert_eq!(err.downcast_ref::().expect("CliError").code(), 2); } #[test] fn multi_get_status_preserves_rate_limited_kind_over_notfound() { - // A real fetch error must NOT be collapsed to NotFound(2): a 429 stays - // RateLimited(4) so an orchestrator keeps retrying. + // A 429 must stay RateLimited(4), not collapse to NotFound(2). let results: Vec<(String, Result)> = vec![ - ("LIN-1".to_string(), Ok(json!({ "data": { "issue": null } }))), + ( + "LIN-1".to_string(), + Ok(json!({ "data": { "issue": null } })), + ), ( "LIN-2".to_string(), - Err(CliError::rate_limited("429").with_retry_after(Some(3)).into()), + Err(CliError::rate_limited("429") + .with_retry_after(Some(3)) + .into()), ), ]; let err = multi_get_status(&results).unwrap_err(); diff --git a/src/error.rs b/src/error.rs index f48def1..f37f5d6 100644 --- a/src/error.rs +++ b/src/error.rs @@ -110,6 +110,30 @@ impl fmt::Display for CliError { impl Error for CliError {} +/// Re-wrap `source` with a new human-readable `message`, preserving the original +/// [`CliError`]'s `kind` / `details` / `retry_after` when `source` is (or wraps, +/// via `anyhow` context) a `CliError`. +/// +/// This is the canonical place commands re-message an error without losing its +/// exit-code semantics: a wrapped rate-limited error stays [`ErrorKind::RateLimited`] +/// (exit 4) with its `retry_after`, instead of being flattened to a generic +/// error. When `source` is not a `CliError`, a generic error carrying `message` +/// is returned — callers should embed the original cause in `message` so the +/// string-based exit-code heuristics still apply. +pub fn rewrap_with_message(source: &anyhow::Error, message: impl Into) -> anyhow::Error { + let message = message.into(); + match source.downcast_ref::() { + Some(cli) => { + let mut wrapped = CliError::new(cli.kind, message).with_retry_after(cli.retry_after); + if let Some(details) = &cli.details { + wrapped = wrapped.with_details(details.clone()); + } + wrapped.into() + } + None => anyhow::anyhow!("{message}"), + } +} + #[cfg(test)] mod tests { use super::*; @@ -199,4 +223,57 @@ mod tests { assert_eq!(err.code(), 2); assert_eq!(err.kind, ErrorKind::NotFound); } + + #[test] + fn rewrap_preserves_kind_and_retry_after() { + let source: anyhow::Error = CliError::rate_limited("429") + .with_retry_after(Some(9)) + .into(); + let wrapped = rewrap_with_message(&source, "while fetching LIN-1"); + let cli = wrapped + .downcast_ref::() + .expect("CliError preserved"); + assert_eq!(cli.kind, ErrorKind::RateLimited); + assert_eq!(cli.code(), 4); + assert_eq!(cli.retry_after, Some(9)); + assert_eq!(cli.message, "while fetching LIN-1"); + } + + #[test] + fn rewrap_preserves_details() { + let source: anyhow::Error = CliError::general("boom") + .with_details(json!({ "field": "value" })) + .into(); + let wrapped = rewrap_with_message(&source, "new message"); + let cli = wrapped + .downcast_ref::() + .expect("CliError preserved"); + assert_eq!(cli.details, Some(json!({ "field": "value" }))); + } + + #[test] + fn rewrap_plain_error_keeps_message() { + let source = anyhow::anyhow!("not found"); + let wrapped = rewrap_with_message(&source, "Failed to resolve 'x': not found"); + assert!(wrapped.downcast_ref::().is_none()); + assert_eq!(wrapped.to_string(), "Failed to resolve 'x': not found"); + } + + #[test] + fn rewrap_finds_cli_error_behind_anyhow_context() { + use anyhow::Context; + // A CliError wrapped in an anyhow context layer must still be found, + // so re-messaging across command boundaries preserves the exit code. + let source = Err::<(), _>(CliError::auth("401")) + .context("resolving user") + .unwrap_err(); + let wrapped = rewrap_with_message(&source, "Failed to resolve user 'me'"); + assert_eq!( + wrapped + .downcast_ref::() + .expect("CliError preserved") + .code(), + 3 + ); + } } diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index 204c097..a10f033 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -2144,24 +2144,18 @@ fn test_yes_flag_works_with_subcommand() { } // ---- Exit-code / JSON-error contract (agent ergonomics) ---- -// -// These verify the central contract offline: a failing command exits non-zero, -// and under `--output json` the `{"error":true,...}` body is emitted on stderr -// while stdout stays clean. They use the `bulk label` empty-issues path, which -// is a logical failure reached before any network/keyring/config access, so the -// assertions are deterministic with no credentials. The credential-dependent -// handler-swallow paths (bulk resolution returning Err, mixed batch failures) -// are verified end-to-end with live keys, since the harness does not mock the -// Linear API. - -/// Run the CLI with an isolated, empty config home and no ambient credentials, -/// so error paths are deterministic and fully offline. Each call gets a unique -/// HOME so parallel tests never share config/cache state. +// Offline checks via the `bulk label` empty-issues path (a failure reached +// before any network/config access). Credential-dependent paths are covered by +// the unit tests in src/commands/* and by live E2E. + +/// Run the CLI with an isolated, empty config home and no ambient credentials. +/// Each call gets a unique HOME so parallel tests never share state. fn run_cli_isolated(args: &[&str]) -> (i32, String, String) { use std::sync::atomic::{AtomicUsize, Ordering}; static COUNTER: AtomicUsize = AtomicUsize::new(0); let n = COUNTER.fetch_add(1, Ordering::Relaxed); - let tmp = std::env::temp_dir().join(format!("linear-cli-isolated-{}-{}", std::process::id(), n)); + let tmp = + std::env::temp_dir().join(format!("linear-cli-isolated-{}-{}", std::process::id(), n)); let _ = std::fs::create_dir_all(&tmp); let output = Command::new(env!("CARGO_BIN_EXE_linear-cli")) .args(args) @@ -2181,10 +2175,12 @@ fn run_cli_isolated(args: &[&str]) -> (i32, String, String) { #[test] fn test_failing_command_exits_nonzero() { - // `bulk label ` with no issues is a logical failure; it must NOT - // exit 0 (the agent-ergonomics regression: errors used to exit 0). + // `bulk label` with no issues is a logical failure: must not exit 0. let (code, _stdout, stderr) = run_cli_isolated(&["bulk", "label", "Bug"]); - assert_ne!(code, 0, "a failing command must exit non-zero; stderr={stderr}"); + assert_ne!( + code, 0, + "a failing command must exit non-zero; stderr={stderr}" + ); assert!( (1..=4).contains(&code), "exit code should be a documented error code (1-4), got {code}" @@ -2193,13 +2189,9 @@ fn test_failing_command_exits_nonzero() { #[test] fn test_json_error_body_emitted_on_stderr_not_stdout() { - // On the empty-issues path stdout is necessarily empty, so the stdout-clean - // assertion below is trivially satisfied here; the meaningful "results on - // stdout + error on stderr" stream split and the any-item-failure - // aggregation are unit-tested in src/commands/{bulk,issues,comments}.rs - // (bulk_exit_status / multi_get_status / comments_status). - let (code, stdout, stderr) = - run_cli_isolated(&["bulk", "label", "Bug", "--output", "json"]); + // stdout is empty on this path; the results-vs-error stream split is + // unit-tested in src/commands/{bulk,issues,comments}.rs. + let (code, stdout, stderr) = run_cli_isolated(&["bulk", "label", "Bug", "--output", "json"]); assert_ne!(code, 0, "failing command must exit non-zero"); assert!( stderr.contains("\"error\":true"), From 129c071dc613cdee536aefb90331182711460d2a Mon Sep 17 00:00:00 2001 From: hojinyoo Date: Tue, 9 Jun 2026 23:19:12 +0900 Subject: [PATCH 3/5] fix(cli): map GraphQL-payload errors to precise exit codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Linear returns application-level failures (not-found, auth, rate-limit) as HTTP 200 with a GraphQL `errors` array, so the transport-level status mapping in `http_error` never saw them and every such failure collapsed to exit code 1. The most common case — `i get ` — returned 1 instead of the documented 2 (not found), defeating agents that branch on exit codes. Classify the first GraphQL error via its `extensions.statusCode` / `extensions.code` / message text into NotFound (2) / Auth (3) / RateLimited (4), falling back to General (1). Note "Entity not found" arrives with statusCode 400 (not 404), so message-text is the fallback that catches it. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/api.rs | 4 +- src/error.rs | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 3 deletions(-) diff --git a/src/api.rs b/src/api.rs index 9449f5b..59dc3bc 100644 --- a/src/api.rs +++ b/src/api.rs @@ -695,9 +695,7 @@ impl LinearClient { let result: Value = response.json().await?; if let Some(errors) = result.get("errors") { - return Err(CliError::general("GraphQL error") - .with_details(errors.clone()) - .into()); + return Err(CliError::from_graphql_errors(errors).into()); } Ok(result) diff --git a/src/error.rs b/src/error.rs index f37f5d6..0de2bed 100644 --- a/src/error.rs +++ b/src/error.rs @@ -72,6 +72,71 @@ impl CliError { self.retry_after = retry_after; self } + + /// Classify a GraphQL `errors` array into a `CliError` with a precise exit code. + /// + /// Linear reports application-level failures as HTTP 200 with an `errors` + /// array, so the real signal lives in the first error's `extensions` + /// (`statusCode` / `code`) or its message text rather than the transport + /// status. The message stays "GraphQL error" so `Display` still appends the + /// underlying error messages. + pub fn from_graphql_errors(errors: &Value) -> Self { + let first = errors.as_array().and_then(|arr| arr.first()); + let kind = first.map_or(ErrorKind::General, Self::classify_graphql_error); + let mut err = Self::new(kind, "GraphQL error").with_details(errors.clone()); + if kind == ErrorKind::RateLimited { + let retry_after = first + .and_then(|e| e.get("extensions")) + .and_then(|ext| ext.get("retryAfter")) + .and_then(Value::as_u64); + err = err.with_retry_after(retry_after); + } + err + } + + fn classify_graphql_error(error: &Value) -> ErrorKind { + let ext = error.get("extensions"); + + // 1. Numeric status code carried in extensions, when present. + if let Some(status) = ext.and_then(|e| e.get("statusCode")).and_then(Value::as_u64) { + match status { + 401 | 403 => return ErrorKind::Auth, + 404 => return ErrorKind::NotFound, + 429 => return ErrorKind::RateLimited, + _ => {} + } + } + + // 2. Symbolic code string (e.g. AUTHENTICATION_ERROR, RATELIMITED). + if let Some(code) = ext.and_then(|e| e.get("code")).and_then(Value::as_str) { + let code = code.to_ascii_uppercase(); + if code.contains("AUTH") { + return ErrorKind::Auth; + } + if code.contains("RATELIM") { + return ErrorKind::RateLimited; + } + if code.contains("NOT_FOUND") { + return ErrorKind::NotFound; + } + } + + // 3. Message text — Linear reports "Entity not found" with statusCode 400. + let msg = error + .get("message") + .and_then(Value::as_str) + .or_else(|| { + ext.and_then(|e| e.get("userPresentableMessage")) + .and_then(Value::as_str) + }) + .unwrap_or(""); + let lower = msg.to_ascii_lowercase(); + if lower.contains("not found") || lower.contains("could not find") { + return ErrorKind::NotFound; + } + + ErrorKind::General + } } impl fmt::Display for CliError { @@ -276,4 +341,72 @@ mod tests { 3 ); } + + #[test] + fn graphql_entity_not_found_maps_to_not_found() { + // Linear returns "Entity not found" as HTTP 200 + errors array with + // statusCode 400 (NOT 404), so classification must fall through to the + // message-text check. This is the common `i get ` path. + let errors = json!([{ + "message": "Entity not found: Issue", + "extensions": { + "code": "INPUT_ERROR", + "statusCode": 400, + "type": "invalid input", + "userError": true, + "userPresentableMessage": "Could not find referenced Issue." + } + }]); + let err = CliError::from_graphql_errors(&errors); + assert_eq!(err.kind, ErrorKind::NotFound); + assert_eq!(err.code(), 2); + assert_eq!(err.to_string(), "GraphQL error: Entity not found: Issue"); + } + + #[test] + fn graphql_auth_error_maps_to_auth() { + let errors = json!([{ + "message": "Authentication required", + "extensions": { "code": "AUTHENTICATION_ERROR" } + }]); + let err = CliError::from_graphql_errors(&errors); + assert_eq!(err.kind, ErrorKind::Auth); + assert_eq!(err.code(), 3); + } + + #[test] + fn graphql_rate_limit_maps_to_rate_limited_with_retry_after() { + let errors = json!([{ + "message": "Too many requests", + "extensions": { "code": "RATELIMITED", "retryAfter": 30 } + }]); + let err = CliError::from_graphql_errors(&errors); + assert_eq!(err.kind, ErrorKind::RateLimited); + assert_eq!(err.code(), 4); + assert_eq!(err.retry_after, Some(30)); + } + + #[test] + fn graphql_status_code_takes_precedence() { + // A 403 in extensions classifies as Auth even without a code string. + let errors = json!([{ + "message": "denied", + "extensions": { "statusCode": 403 } + }]); + assert_eq!(CliError::from_graphql_errors(&errors).code(), 3); + } + + #[test] + fn graphql_generic_error_stays_general() { + let errors = json!([{ "message": "Field 'foo' is invalid" }]); + let err = CliError::from_graphql_errors(&errors); + assert_eq!(err.kind, ErrorKind::General); + assert_eq!(err.code(), 1); + } + + #[test] + fn graphql_empty_errors_stays_general() { + let errors = json!([]); + assert_eq!(CliError::from_graphql_errors(&errors).kind, ErrorKind::General); + } } From 7c64ca291de9b5745d2708cacd028f844d098660 Mon Sep 17 00:00:00 2001 From: hojinyoo Date: Tue, 9 Jun 2026 23:25:19 +0900 Subject: [PATCH 4/5] fix(cli): classify GraphQL errors behind non-2xx HTTP status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit from_graphql_errors only ran on the 2xx response branch, so any application error returned with a non-2xx status went through http_error, which maps only HTTP 429 to RateLimited. Per Linear's docs GraphQL rate limits arrive as HTTP 400 + extensions.code RATELIMITED, so they exited 1 (General) instead of 4 (RateLimited) — defeating agent backoff that keys on exit code 4. Add refine_http_error: when a non-2xx body carries a GraphQL `errors` array that classifies more specifically than the transport status, prefer the payload kind (keeping the status message and merging retry_after). Generic payloads leave the status kind intact, so a 401 with an unclassifiable body still exits 3. Unit-tested without HTTP mocking. Found via Codex PR review. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/api.rs | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/src/api.rs b/src/api.rs index 59dc3bc..4eae4b1 100644 --- a/src/api.rs +++ b/src/api.rs @@ -10,7 +10,7 @@ use tokio::sync::RwLock; use crate::cache::{Cache, CacheOptions, CacheType}; use crate::config; -use crate::error::CliError; +use crate::error::{CliError, ErrorKind}; use crate::pagination::{paginate_nodes, PaginationOptions}; use crate::retry::{with_retry, RetryConfig}; use crate::text::is_uuid; @@ -137,6 +137,25 @@ fn http_error(status: StatusCode, headers: &HeaderMap, context: &str) -> CliErro err.with_details(details) } +/// Refine a transport-level HTTP error using its GraphQL payload. +/// +/// Linear can return application errors (notably RATELIMITED) as a non-2xx +/// status carrying a GraphQL `errors` array, so the transport status alone +/// under-classifies them. When the payload resolves to a more specific kind +/// than the status, prefer it so the exit code stays precise (e.g. 4 for a +/// rate limit behind HTTP 400). The status-derived message is kept. +fn refine_http_error(http_err: CliError, body: &Value) -> CliError { + let Some(errors) = body.get("errors") else { + return http_err; + }; + let payload = CliError::from_graphql_errors(errors); + if payload.kind == ErrorKind::General { + return http_err; + } + CliError::new(payload.kind, http_err.message.clone()) + .with_retry_after(http_err.retry_after.or(payload.retry_after)) +} + pub fn parse_linear_upload_url(raw: &str) -> Result { let url = Url::parse(raw).context("Failed to parse upload URL")?; @@ -685,7 +704,7 @@ impl LinearClient { } else { json!({ "body": body }) }; - let mut err = http_error(status, &headers, "resource"); + let mut err = refine_http_error(http_error(status, &headers, "resource"), &details); if !body.is_empty() { err = err.with_details(details); } @@ -869,6 +888,41 @@ fn default_retry_config() -> RetryConfig { mod tests { use super::*; + #[test] + fn refine_http_error_prefers_ratelimited_payload_behind_http_400() { + // Linear returns GraphQL rate limits as HTTP 400 + extensions.code + // RATELIMITED; the transport status alone would yield General(1). + let http_err = CliError::general("HTTP 400 Bad Request"); + let body = json!({ + "errors": [{ + "message": "Rate limit exceeded", + "extensions": { "code": "RATELIMITED", "retryAfter": 12 } + }] + }); + let refined = refine_http_error(http_err, &body); + assert_eq!(refined.kind, ErrorKind::RateLimited); + assert_eq!(refined.code(), 4); + assert_eq!(refined.retry_after, Some(12)); + } + + #[test] + fn refine_http_error_keeps_status_kind_when_payload_is_generic() { + // A 401 whose payload doesn't classify must stay Auth(3), not regress to General. + let http_err = CliError::auth("Authentication failed"); + let body = json!({ "errors": [{ "message": "Unauthorized" }] }); + let refined = refine_http_error(http_err, &body); + assert_eq!(refined.kind, ErrorKind::Auth); + assert_eq!(refined.code(), 3); + } + + #[test] + fn refine_http_error_passes_through_without_errors_array() { + let http_err = CliError::not_found("resource not found"); + let body = json!({ "body": "502" }); + let refined = refine_http_error(http_err, &body); + assert_eq!(refined.kind, ErrorKind::NotFound); + } + #[test] fn test_auth_state_api_key_header() { let state = AuthState::ApiKey("lin_api_key123".to_string()); From 923ccf3b1a92b74b4f2c9014d216241b516fa469 Mon Sep 17 00:00:00 2001 From: Finesssee <90105158+Finesssee@users.noreply.github.com> Date: Thu, 11 Jun 2026 19:12:54 +0700 Subject: [PATCH 5/5] style: format error tests --- src/error.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/error.rs b/src/error.rs index 0de2bed..0b59052 100644 --- a/src/error.rs +++ b/src/error.rs @@ -98,7 +98,10 @@ impl CliError { let ext = error.get("extensions"); // 1. Numeric status code carried in extensions, when present. - if let Some(status) = ext.and_then(|e| e.get("statusCode")).and_then(Value::as_u64) { + if let Some(status) = ext + .and_then(|e| e.get("statusCode")) + .and_then(Value::as_u64) + { match status { 401 | 403 => return ErrorKind::Auth, 404 => return ErrorKind::NotFound, @@ -407,6 +410,9 @@ mod tests { #[test] fn graphql_empty_errors_stays_general() { let errors = json!([]); - assert_eq!(CliError::from_graphql_errors(&errors).kind, ErrorKind::General); + assert_eq!( + CliError::from_graphql_errors(&errors).kind, + ErrorKind::General + ); } }