From ce481ca1547575afd69b7d9e5faa57c67e8df7c8 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 6 May 2026 14:03:12 -0400 Subject: [PATCH] fix(cli): preserve API error details Route async API failures through the body-preserving classifier and add run-create context so CLI output keeps server response details in the cause chain. --- ...026-05-06-improve-cli-api-error-display.md | 78 +++++++++++++++++++ .../fabro-cli/src/commands/run/command.rs | 4 +- .../fabro-cli/src/commands/run/create.rs | 5 +- .../fabro-cli/src/commands/run/resume.rs | 9 ++- lib/crates/fabro-cli/tests/it/cmd/logs.rs | 4 + lib/crates/fabro-cli/tests/it/cmd/run.rs | 49 ++++++++++++ lib/crates/fabro-client/src/client.rs | 15 ++-- lib/crates/fabro-client/src/error.rs | 58 +++++++++++++- 8 files changed, 210 insertions(+), 12 deletions(-) create mode 100644 docs/superpowers/plans/2026-05-06-improve-cli-api-error-display.md diff --git a/docs/superpowers/plans/2026-05-06-improve-cli-api-error-display.md b/docs/superpowers/plans/2026-05-06-improve-cli-api-error-display.md new file mode 100644 index 000000000..948f31ccf --- /dev/null +++ b/docs/superpowers/plans/2026-05-06-improve-cli-api-error-display.md @@ -0,0 +1,78 @@ +# Improve CLI API Error Display Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Preserve useful server error details in Fabro CLI output without adding a new error taxonomy or sprawling command-specific handling. + +**Architecture:** Keep error flow centered on existing `anyhow`, `source()`, `miette`, `TaggedFailure`, `ApiFailure`, `api_failure_for`, `classify_api_error`, and `raw_response_failure_error` abstractions. Preserve response details centrally in `fabro-client`, then add sparse user-action context at command boundaries. + +**Tech Stack:** Rust, `anyhow`, `miette`, `progenitor_client`, `httpmock`, `cargo nextest`. + +--- + +## Summary + +Improve failed `fabro run` and related API command output by preserving server response details through the existing error chain. The target display for run creation failures is: + +```text +x could not create run + caused by: missing field `dirty` at line 1 column 2834 +``` + +For non-JSON plain-text bodies, the CLI should retain the body instead of collapsing to status only: + +```text +x could not create run + caused by: request failed with status 422 Unprocessable Entity: Failed to deserialize ... +``` + +## Key Changes + +- [x] In `lib/crates/fabro-client/src/error.rs`, keep `map_api_error` synchronous. Do not make it read `UnexpectedResponse` bodies, because that requires async body consumption. +- [x] In async client paths that receive `progenitor_client::Error` after `.await`, use `classify_api_error(err).await` so `UnexpectedResponse` bodies are consumed and preserved. +- [x] Update the token-refresh retry path in `Client::send_api` so retry failures also go through the async classifier instead of `.map_err(map_api_error)`. +- [x] Update optional fetch paths that currently call `map_api_error` after `.await` (`get_run_logs`, `read_run_blob`) to use `classify_api_error(err).await.error`, preserving existing `is_not_found_error` behavior through `ApiFailure`. +- [x] In `lib/crates/fabro-cli/src/commands/run/create.rs`, wrap `client.create_run_from_manifest(built.manifest).await` with `context("could not create run")`. +- [x] Do not add new public error types, new CLI diagnostic enums, run-specific API error branches, or a new version-compatibility framework. +- [x] Do not change server wire behavior in this patch. + +## Interface Impact + +- No public Rust API additions. +- No OpenAPI/schema changes. +- CLI stderr output changes for failed API calls by showing action context plus the existing source chain. +- Exit codes remain unchanged because `ApiFailure` and existing `ExitClass` tagging stay in place. +- `--json` behavior remains unchanged; do not add new JSON error payloads. + +## Test Plan + +- [x] Add or adjust `fabro-client` unit tests in `lib/crates/fabro-client/src/error.rs` for structured JSON API errors: + - `errors[0].detail` remains the displayed message. + - `errors[0].code` remains discoverable through `api_failure_for`. + - Existing 401 `ExitClass::AuthRequired` behavior still passes. +- [x] Add a `fabro-client` async test for a plain-text `422` `UnexpectedResponse` through `classify_api_error`: + - The displayed error includes both the status and response body. + - `api_failure_for` reports status `422`. +- [x] Add a CLI integration test in `lib/crates/fabro-cli/tests/it/cmd/run.rs` with a mock server returning `422` from `POST /api/v1/runs`: + - `stderr` includes `could not create run`. + - `stderr` includes the response detail/body, such as `missing field \`dirty\``. + - `stderr` does not collapse to status-only output. +- [x] Run targeted tests: + +```bash +cargo nextest run -p fabro-client +cargo nextest run -p fabro-cli --test it run +``` + +- [x] Run workspace checks: + +```bash +cargo +nightly-2026-04-14 fmt --check --all +cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings +``` + +## Assumptions + +- The first fix should improve error presentation, not add backwards compatibility for the `git.dirty` manifest change. +- Version-skew-specific hints are out of scope unless they can reuse already-available metadata without extra probing or new error plumbing. +- Existing `miette` cause-chain rendering is the display mechanism; implementation should make the error chain better, not bypass it. diff --git a/lib/crates/fabro-cli/src/commands/run/command.rs b/lib/crates/fabro-cli/src/commands/run/command.rs index b6692081f..4c7f7a93b 100644 --- a/lib/crates/fabro-cli/src/commands/run/command.rs +++ b/lib/crates/fabro-cli/src/commands/run/command.rs @@ -54,12 +54,12 @@ pub(crate) async fn execute(mut args: RunArgs, base_ctx: &CommandContext) -> Res )) .await?; if !json { - super::output::print_run_summary_with_client( + Box::pin(super::output::print_run_summary_with_client( &client, &created_run.run_id, styles, printer, - ) + )) .await?; } if exit_code != std::process::ExitCode::SUCCESS { diff --git a/lib/crates/fabro-cli/src/commands/run/create.rs b/lib/crates/fabro-cli/src/commands/run/create.rs index 83120f57b..89397f70d 100644 --- a/lib/crates/fabro-cli/src/commands/run/create.rs +++ b/lib/crates/fabro-cli/src/commands/run/create.rs @@ -65,7 +65,10 @@ pub(crate) async fn create_run( } let client = ctx.server().await?; - let created_run_id = client.create_run_from_manifest(built.manifest).await?; + let created_run_id = client + .create_run_from_manifest(built.manifest) + .await + .context("could not create run")?; Ok(CreatedRun { run_id: created_run_id, diff --git a/lib/crates/fabro-cli/src/commands/run/resume.rs b/lib/crates/fabro-cli/src/commands/run/resume.rs index 22db1bc5d..2e28c8f8c 100644 --- a/lib/crates/fabro-cli/src/commands/run/resume.rs +++ b/lib/crates/fabro-cli/src/commands/run/resume.rs @@ -40,8 +40,13 @@ pub(crate) async fn resume_command( )) .await?; if !json { - super::output::print_run_summary_with_client(client.as_ref(), &run_id, styles, printer) - .await?; + Box::pin(super::output::print_run_summary_with_client( + client.as_ref(), + &run_id, + styles, + printer, + )) + .await?; } if exit_code != std::process::ExitCode::SUCCESS { std::process::exit(1); diff --git a/lib/crates/fabro-cli/tests/it/cmd/logs.rs b/lib/crates/fabro-cli/tests/it/cmd/logs.rs index 6b331651f..61ce799a0 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/logs.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/logs.rs @@ -4,6 +4,10 @@ use fabro_test::{fabro_snapshot, test_context}; use super::support::{output_stderr, setup_seeded_completed_dry_run}; +#[expect( + clippy::disallowed_methods, + reason = "CLI integration test seeds a deterministic runtime log fixture with synchronous fs writes." +)] fn seed_run_log(run_dir: &Path, contents: &[u8]) { // Raw logs are the CLI surface for this runtime-owned file; no public // command creates deterministic contents suitable for exact assertions. diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index cccd1eb21..21abe81ed 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -255,6 +255,55 @@ fn detach_uses_configured_server_target_without_server_flag() { ); } +#[test] +fn run_create_failure_shows_action_context_and_response_body() { + let context = test_context!(); + let server = MockServer::start(); + let create_mock = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(422) + .header("Content-Type", "text/plain") + .body("Failed to deserialize request: missing field `dirty` at line 1 column 2834"); + }); + + let workflow = context.install_fixture("simple.fabro"); + let output = context + .run_cmd() + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--detach", + "--dry-run", + "--auto-approve", + workflow.to_str().unwrap(), + ]) + .output() + .expect("command should execute"); + + assert!( + !output.status.success(), + "create failure should fail:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + create_mock.assert(); + + let stderr = output_stderr(&output); + assert!(stderr.contains("could not create run"), "{stderr}"); + assert!(stderr.contains("missing field `dirty`"), "{stderr}"); + assert!( + stderr.contains("422 Unprocessable Entity"), + "status should remain visible for plain-text API failures:\n{stderr}" + ); + assert!( + !stderr.lines().any(|line| { + line.trim_end() + .ends_with("request failed with status 422 Unprocessable Entity") + }), + "stderr should not collapse to status-only output:\n{stderr}" + ); +} + #[test] fn run_uses_vault_credentials_for_worker_execution() { let mut context = test_context!(); diff --git a/lib/crates/fabro-client/src/client.rs b/lib/crates/fabro-client/src/client.rs index e7c111e7f..576161ae3 100644 --- a/lib/crates/fabro-client/src/client.rs +++ b/lib/crates/fabro-client/src/client.rs @@ -26,7 +26,7 @@ use tokio_util::io::ReaderStream; use crate::credential::Credential; use crate::error::{ ApiError, ApiFailure, api_failure_for, classify_api_error, classify_http_response, - convert_type, is_not_found_error, map_api_error, raw_response_failure_error, + convert_type, is_not_found_error, raw_response_failure_error, }; use crate::session::OAuthSession; use crate::target::ServerTarget; @@ -359,10 +359,13 @@ impl Client { if let Some(failed_token) = state.bearer_token.as_deref() { self.refresh_access_token(failed_token).await?; let state = self.current_state(); - return self + let retry_response = self .with_request_timeout(Box::pin(request(state.client.clone()))) - .await? - .map_err(map_api_error); + .await?; + return match retry_response { + Ok(response) => Ok(response), + Err(err) => Err(classify_api_error(err).await.error), + }; } } Err(mapped.error) @@ -995,7 +998,7 @@ impl Client { Ok(Some(bytes)) } Err(err) => { - let err = map_api_error(err); + let err = classify_api_error(err).await.error; if is_not_found_error(&err) { Ok(None) } else { @@ -1234,7 +1237,7 @@ impl Client { Ok(Some(Bytes::from(bytes))) } Err(err) => { - let err = map_api_error(err); + let err = classify_api_error(err).await.error; if is_not_found_error(&err) { Ok(None) } else { diff --git a/lib/crates/fabro-client/src/error.rs b/lib/crates/fabro-client/src/error.rs index 9c48ff55e..b5b2c9a07 100644 --- a/lib/crates/fabro-client/src/error.rs +++ b/lib/crates/fabro-client/src/error.rs @@ -231,10 +231,14 @@ where #[cfg(test)] mod tests { use fabro_util::exit; + use httpmock::MockServer; use serde_json::json; use tokio::net::TcpListener; - use super::{ApiError, ApiFailure, map_api_error, raw_response_failure_error}; + use super::{ + ApiError, ApiFailure, api_failure_for, classify_api_error, map_api_error, + raw_response_failure_error, + }; fn error_response( status: fabro_http::StatusCode, @@ -292,6 +296,23 @@ mod tests { assert_eq!(exit::exit_code_for(&err), 1); } + #[test] + fn map_api_error_uses_structured_detail_and_preserves_code() { + let err = map_api_error(error_response( + fabro_http::StatusCode::UNPROCESSABLE_ENTITY, + "missing field `dirty` at line 1 column 2834", + "invalid_manifest", + )); + + assert_eq!( + err.to_string(), + "missing field `dirty` at line 1 column 2834" + ); + let failure = api_failure_for(&err).expect("error should carry API failure metadata"); + assert_eq!(failure.status, fabro_http::StatusCode::UNPROCESSABLE_ENTITY); + assert_eq!(failure.code.as_deref(), Some("invalid_manifest")); + } + #[test] fn raw_response_failure_error_marks_401_as_auth_required() { let err = raw_response_failure_error(&api_error( @@ -312,6 +333,41 @@ mod tests { assert_eq!(exit::exit_code_for(&err), 1); } + #[tokio::test] + async fn classify_api_error_preserves_plain_text_unexpected_response_body() { + let server = MockServer::start(); + server.mock(|when, then| { + when.method("GET").path("/plain-error"); + then.status(422) + .header("Content-Type", "text/plain") + .body("Failed to deserialize request: missing field `dirty` at line 1 column 2834"); + }); + + let response = fabro_http::test_http_client() + .unwrap() + .get(format!("{}/plain-error", server.base_url())) + .send() + .await + .expect("mock server should return a response"); + let err = classify_api_error( + progenitor_client::Error::::UnexpectedResponse(response), + ) + .await + .error; + + let message = err.to_string(); + assert!( + message.contains("request failed with status 422 Unprocessable Entity"), + "expected status in message, got: {message}" + ); + assert!( + message.contains("missing field `dirty`"), + "expected response body in message, got: {message}" + ); + let failure = api_failure_for(&err).expect("error should carry API failure metadata"); + assert_eq!(failure.status, fabro_http::StatusCode::UNPROCESSABLE_ENTITY); + } + #[tokio::test] async fn map_api_error_preserves_communication_error_source_chain() { let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();