From ad7fdc8d1305380bb8c49ea81ba218558edc3e6e Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 19 Apr 2026 13:43:57 -0400 Subject: [PATCH] refactor(install): return spec-conformant ApiError shape Install handlers returned `{"error": "..."}` while the OpenAPI paths referenced the repo-wide `ErrorResponse` schema (`{"errors":[{status,title,detail}]}`). Funnel the install helper through `ApiError::into_response`, switch the invalid-token 401 and the persistence-failure INTERNAL_SERVER_ERROR to the same shape, and update the TS `readInstallError` helper + test fixtures to read `body.errors[0].detail`. The install-finish failure path still carries `leftover_env_keys` alongside the error envelope so the rollback integration tests retain their diagnostic field. Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/fabro-web/app/install-api.test.ts | 13 +++++--- apps/fabro-web/app/install-api.ts | 7 ++-- lib/crates/fabro-server/src/install.rs | 32 ++++++++++++------- .../fabro-server/tests/it/api/install.rs | 6 ++-- .../tests/it/api/install_openai_compatible.rs | 4 +-- 5 files changed, 40 insertions(+), 22 deletions(-) diff --git a/apps/fabro-web/app/install-api.test.ts b/apps/fabro-web/app/install-api.test.ts index 8ead4355a..d920beee9 100644 --- a/apps/fabro-web/app/install-api.test.ts +++ b/apps/fabro-web/app/install-api.test.ts @@ -4,10 +4,15 @@ import { buildGithubOwnerValue, readInstallError } from "./install-api"; describe("readInstallError", () => { test("prefers the structured install error payload", async () => { - const response = new Response(JSON.stringify({ error: "invalid token" }), { - status: 422, - headers: { "Content-Type": "application/json" }, - }); + const response = new Response( + JSON.stringify({ + errors: [{ status: "422", title: "Unprocessable Entity", detail: "invalid token" }], + }), + { + status: 422, + headers: { "Content-Type": "application/json" }, + }, + ); await expect( readInstallError(response, "install request failed"), diff --git a/apps/fabro-web/app/install-api.ts b/apps/fabro-web/app/install-api.ts index 4875fd444..328080d29 100644 --- a/apps/fabro-web/app/install-api.ts +++ b/apps/fabro-web/app/install-api.ts @@ -81,8 +81,11 @@ export async function readInstallError( fallback: string, ): Promise { try { - const body = (await response.clone().json()) as { error?: string }; - if (body.error) return body.error; + const body = (await response.clone().json()) as { + errors?: Array<{ detail?: string }>; + }; + const detail = body.errors?.[0]?.detail; + if (detail) return detail; } catch { // fall through to the default message } diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index 5a5e6259c..36c3f6124 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -31,6 +31,7 @@ use tower::service_fn; use tracing::{error, info, warn}; use crate::bind::{Bind, BindRequest}; +use crate::error::ApiError; use crate::serve::{self, DEFAULT_TCP_PORT}; use crate::{security_headers, static_files}; @@ -426,7 +427,7 @@ async fn get_install_session( Query(query): Query, ) -> Response { if !token_is_valid(&state, &headers, query.token.as_deref()) { - return (StatusCode::UNAUTHORIZED, "invalid install token").into_response(); + return ApiError::new(StatusCode::UNAUTHORIZED, "invalid install token").into_response(); } observe_operator(&state, &headers); @@ -890,11 +891,22 @@ async fn post_install_finish( }), ) { error!(error = %err, "install persistence failed"); + let status = StatusCode::INTERNAL_SERVER_ERROR; + let detail = err.to_string(); + let title = status.canonical_reason().unwrap_or("Unknown").to_string(); + let leftover_env_keys: Vec = server_env_secrets + .iter() + .map(|(key, _)| key.clone()) + .collect(); return ( - StatusCode::INTERNAL_SERVER_ERROR, + status, Json(serde_json::json!({ - "error": err.to_string(), - "leftover_env_keys": server_env_secrets.iter().map(|(key, _)| key.clone()).collect::>(), + "errors": [{ + "status": status.as_u16().to_string(), + "title": title, + "detail": detail, + }], + "leftover_env_keys": leftover_env_keys, })), ) .into_response(); @@ -956,7 +968,7 @@ fn require_valid_token( query_token: Option<&str>, ) -> Option { (!token_is_valid(state, headers, query_token)) - .then(|| (StatusCode::UNAUTHORIZED, "invalid install token").into_response()) + .then(|| ApiError::new(StatusCode::UNAUTHORIZED, "invalid install token").into_response()) } fn observe_operator(state: &InstallAppState, headers: &HeaderMap) { @@ -1077,17 +1089,15 @@ fn redacted_github(pending_install: &PendingInstall) -> serde_json::Value { } fn missing_step_response(step: &str) -> Response { - ( + ApiError::new( StatusCode::UNPROCESSABLE_ENTITY, - Json(serde_json::json!({ - "error": format!("install step '{step}' is incomplete"), - })), + format!("install step '{step}' is incomplete"), ) - .into_response() + .into_response() } fn install_error_response(status: StatusCode, message: impl Into) -> Response { - (status, Json(serde_json::json!({ "error": message.into() }))).into_response() + ApiError::new(status, message).into_response() } fn validate_canonical_url(value: &str) -> Result<(), String> { diff --git a/lib/crates/fabro-server/tests/it/api/install.rs b/lib/crates/fabro-server/tests/it/api/install.rs index c00d39560..f0f21b739 100644 --- a/lib/crates/fabro-server/tests/it/api/install.rs +++ b/lib/crates/fabro-server/tests/it/api/install.rs @@ -710,7 +710,7 @@ async fn github_app_manifest_rejects_retry_while_pending_and_preserves_prior_tok assert_eq!(retry_response.status(), StatusCode::CONFLICT); let retry_body = body_json(retry_response.into_body()).await; assert_eq!( - retry_body["error"], + retry_body["errors"][0]["detail"], "GitHub App setup is already pending; finish it or wait for it to expire." ); } @@ -988,7 +988,7 @@ async fn install_server_rejects_trailing_slash_canonical_urls() { assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY); let body = body_json(response.into_body()).await; assert_eq!( - body["error"], + body["errors"][0]["detail"], "canonical_url must not end with a trailing slash" ); } @@ -1032,7 +1032,7 @@ async fn install_finish_failure_restores_settings_and_vault_but_leaves_env_keys( assert_eq!(finish_response.status(), StatusCode::INTERNAL_SERVER_ERROR); let finish_body = body_json(finish_response.into_body()).await; assert!( - finish_body["error"] + finish_body["errors"][0]["detail"] .as_str() .is_some_and(|value| value.contains("persisting install outputs directly")) ); diff --git a/lib/crates/fabro-server/tests/it/api/install_openai_compatible.rs b/lib/crates/fabro-server/tests/it/api/install_openai_compatible.rs index 6b3f5b67b..9a1bd7214 100644 --- a/lib/crates/fabro-server/tests/it/api/install_openai_compatible.rs +++ b/lib/crates/fabro-server/tests/it/api/install_openai_compatible.rs @@ -27,7 +27,7 @@ async fn install_llm_endpoints_reject_openai_compatible_in_v1() { assert_eq!(test_response.status(), StatusCode::UNPROCESSABLE_ENTITY); let test_body = body_json(test_response.into_body()).await; assert_eq!( - test_body["error"], + test_body["errors"][0]["detail"], "openai_compatible is not supported by install in v1" ); @@ -48,7 +48,7 @@ async fn install_llm_endpoints_reject_openai_compatible_in_v1() { assert_eq!(put_response.status(), StatusCode::UNPROCESSABLE_ENTITY); let put_body = body_json(put_response.into_body()).await; assert_eq!( - put_body["error"], + put_body["errors"][0]["detail"], "openai_compatible is not supported by install in v1" ); }