mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-12 23:02:41 +00:00
fix(cli): preserve API error details
Some checks are pending
Rust / Format (push) Waiting to run
Rust / Clippy (push) Waiting to run
Rust / Generated Docs (push) Waiting to run
Rust / Test (Linux) (push) Waiting to run
Rust / Test (macOS) (push) Waiting to run
TypeScript / Typecheck (push) Waiting to run
TypeScript / Test (push) Waiting to run
TypeScript / Build (push) Waiting to run
Some checks are pending
Rust / Format (push) Waiting to run
Rust / Clippy (push) Waiting to run
Rust / Generated Docs (push) Waiting to run
Rust / Test (Linux) (push) Waiting to run
Rust / Test (macOS) (push) Waiting to run
TypeScript / Typecheck (push) Waiting to run
TypeScript / Test (push) Waiting to run
TypeScript / Build (push) Waiting to run
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.
This commit is contained in:
parent
3bfb012fab
commit
ce481ca154
8 changed files with 210 additions and 12 deletions
|
|
@ -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.
|
||||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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!();
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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::<serde_json::Value>::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();
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue