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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-19 13:43:57 -04:00
parent 985373cad4
commit ad7fdc8d13
No known key found for this signature in database
5 changed files with 40 additions and 22 deletions

View file

@ -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"),

View file

@ -81,8 +81,11 @@ export async function readInstallError(
fallback: string,
): Promise<string> {
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
}

View file

@ -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<InstallTokenQuery>,
) -> 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<String> = 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::<Vec<_>>(),
"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<Response> {
(!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<String>) -> 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> {

View file

@ -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"))
);

View file

@ -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"
);
}