diff --git a/crates/client/src/error.rs b/crates/client/src/error.rs index 2cd5fc55..b96e05a1 100644 --- a/crates/client/src/error.rs +++ b/crates/client/src/error.rs @@ -34,3 +34,114 @@ impl From for ClientError { ClientError::Status(Box::new(status)) } } + +impl ClientError { + /// Parse the structured `{ code, message, meta }` object Guardian attaches + /// to a gRPC `Status.details` (feature `009-human-readable-errors`). + /// Returns `None` for non-Status errors or Status errors without details. + fn guardian_error_details(&self) -> Option { + match self { + ClientError::Status(status) => { + let details = status.details(); + if details.is_empty() { + return None; + } + serde_json::from_slice(details).ok() + } + _ => None, + } + } + + /// Stable, machine-readable Guardian error code (e.g. `account_paused`), + /// when this error originated from a Guardian gRPC `Status`. Branch on + /// this rather than on the message text. + pub fn guardian_code(&self) -> Option { + self.guardian_error_details()? + .get("code")? + .as_str() + .map(str::to_owned) + } + + /// Short, end-user-safe message safe to show in a wallet UI, when this + /// error originated from a Guardian gRPC `Status`. Falls back to the + /// `Status.message` (also the user-safe message) for Status errors + /// without a details object. + pub fn user_message(&self) -> Option { + if let Some(message) = self + .guardian_error_details() + .and_then(|d| d.get("message")?.as_str().map(str::to_owned)) + { + return Some(message); + } + match self { + ClientError::Status(status) => Some(status.message().to_owned()), + _ => None, + } + } + + /// The structured `meta` block (`retryable`, `retry_after_secs`, …) from a + /// Guardian gRPC `Status`, when present. + pub fn guardian_meta(&self) -> Option { + self.guardian_error_details()?.get("meta").cloned() + } + + /// Whether this error means "the requested record does not exist". Handles + /// both the gRPC `Status` path (gRPC `NotFound`, feature 009) and the + /// legacy in-band `ServerError` message. Callers use this instead of + /// substring-matching the message text. + pub fn is_not_found(&self) -> bool { + match self { + ClientError::Status(status) => status.code() == tonic::Code::NotFound, + ClientError::ServerError(msg) => msg.contains("not found"), + _ => false, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn guardian_status() -> tonic::Status { + // Mirrors the object the server attaches to Status.details (feature 009). + let details = serde_json::json!({ + "code": "account_paused", + "message": "This account is paused and can't approve transactions right now.", + "meta": { "retryable": false } + }) + .to_string() + .into_bytes(); + tonic::Status::with_details( + tonic::Code::FailedPrecondition, + "This account is paused and can't approve transactions right now.", + details.into(), + ) + } + + #[test] + fn accessors_parse_guardian_status_details() { + let err: ClientError = guardian_status().into(); + assert_eq!(err.guardian_code().as_deref(), Some("account_paused")); + assert_eq!( + err.user_message().as_deref(), + Some("This account is paused and can't approve transactions right now.") + ); + assert_eq!(err.guardian_meta().unwrap()["retryable"], false); + assert!(!err.is_not_found()); + } + + #[test] + fn user_message_falls_back_to_status_message_without_details() { + let err: ClientError = tonic::Status::new(tonic::Code::Unavailable, "fallback msg").into(); + assert_eq!(err.guardian_code(), None); + assert_eq!(err.user_message().as_deref(), Some("fallback msg")); + } + + #[test] + fn is_not_found_detects_grpc_not_found_and_legacy_message() { + let status: ClientError = tonic::Status::new(tonic::Code::NotFound, "x").into(); + assert!(status.is_not_found()); + assert!(ClientError::ServerError("Delta not found for account".into()).is_not_found()); + assert!(!ClientError::ServerError("boom".into()).is_not_found()); + } +} diff --git a/crates/miden-multisig-client/src/client/account.rs b/crates/miden-multisig-client/src/client/account.rs index 3781a59d..30d3dccd 100644 --- a/crates/miden-multisig-client/src/client/account.rs +++ b/crates/miden-multisig-client/src/client/account.rs @@ -7,8 +7,7 @@ use std::collections::HashSet; use base64::Engine; use guardian_client::{ - AuthConfig, ClientError as GuardianClientError, MidenEcdsaAuth, MidenFalconRpoAuth, - TryIntoTxSummary, auth_config::AuthType, + AuthConfig, MidenEcdsaAuth, MidenFalconRpoAuth, TryIntoTxSummary, auth_config::AuthType, }; use guardian_shared::SignatureScheme; use miden_client::account::Account; @@ -393,8 +392,10 @@ impl MultisigClient { .await { Ok(resp) => resp, - Err(GuardianClientError::ServerError(msg)) if msg.contains("not found") => { - // No new deltas since current nonce - this is not an error + // No new deltas since current nonce — not an error. Errors now + // arrive as a gRPC Status (feature 009); branch on the not-found + // signal rather than substring-matching the message. + Err(e) if e.is_not_found() => { return Ok(()); } Err(e) => { diff --git a/crates/server/src/api/dashboard.rs b/crates/server/src/api/dashboard.rs index 556a5d7c..2d0d5a1d 100644 --- a/crates/server/src/api/dashboard.rs +++ b/crates/server/src/api/dashboard.rs @@ -1275,11 +1275,11 @@ mod tests { "expected stable code on {path}", ); assert_eq!( - body["missing_permissions"], + body["meta"]["missing_permissions"], serde_json::json!(["dashboard:read"]), "expected missing_permissions on {path}", ); - assert_eq!(body["retryable"], serde_json::Value::Bool(false)); + assert_eq!(body["meta"]["retryable"], serde_json::Value::Bool(false)); } } @@ -1433,7 +1433,7 @@ mod tests { let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); assert_eq!(body["code"], "GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION"); assert_eq!( - body["missing_permissions"], + body["meta"]["missing_permissions"], serde_json::json!(["accounts:pause"]) ); @@ -1912,7 +1912,7 @@ mod tests { let body: serde_json::Value = read_json(response).await; assert_eq!(body["code"], "GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION"); assert_eq!( - body["missing_permissions"], + body["meta"]["missing_permissions"], serde_json::json!(["accounts:pause"]) ); } diff --git a/crates/server/src/api/grpc.rs b/crates/server/src/api/grpc.rs index cc719444..ef3c9fd5 100644 --- a/crates/server/src/api/grpc.rs +++ b/crates/server/src/api/grpc.rs @@ -72,13 +72,7 @@ impl Guardian for GuardianService { ack_commitment: response.ack_commitment, error_code: String::new(), })), - Err(e) => Ok(Response::new(ConfigureResponse { - success: false, - message: e.to_string(), - ack_pubkey: String::new(), - ack_commitment: String::new(), - error_code: e.code().to_string(), - })), + Err(e) => Err(Status::from(e)), } } @@ -122,13 +116,7 @@ impl Guardian for GuardianService { ack_sig: Some(response.delta.ack_sig), error_code: String::new(), })), - Err(e) => Ok(Response::new(PushDeltaResponse { - success: false, - message: e.to_string(), - delta: None, - ack_sig: None, - error_code: e.code().to_string(), - })), + Err(e) => Err(Status::from(e)), } } @@ -154,12 +142,7 @@ impl Guardian for GuardianService { delta: Some(delta_to_proto(&response.delta)), error_code: String::new(), })), - Err(e) => Ok(Response::new(GetDeltaResponse { - success: false, - message: e.to_string(), - delta: None, - error_code: e.code().to_string(), - })), + Err(e) => Err(Status::from(e)), } } @@ -185,12 +168,7 @@ impl Guardian for GuardianService { merged_delta: Some(delta_to_proto(&response.merged_delta)), error_code: String::new(), })), - Err(e) => Ok(Response::new(GetDeltaSinceResponse { - success: false, - message: e.to_string(), - merged_delta: None, - error_code: e.code().to_string(), - })), + Err(e) => Err(Status::from(e)), } } @@ -215,12 +193,7 @@ impl Guardian for GuardianService { state: Some(state_to_proto(&response.state)), error_code: String::new(), })), - Err(e) => Ok(Response::new(GetStateResponse { - success: false, - message: e.to_string(), - state: None, - error_code: e.code().to_string(), - })), + Err(e) => Err(Status::from(e)), } } @@ -268,13 +241,7 @@ impl Guardian for GuardianService { commitment: response.commitment, error_code: String::new(), })), - Err(e) => Ok(Response::new(PushDeltaProposalResponse { - success: false, - message: e.to_string(), - delta: None, - commitment: String::new(), - error_code: e.code().to_string(), - })), + Err(e) => Err(Status::from(e)), } } @@ -297,12 +264,7 @@ impl Guardian for GuardianService { proposals: response.proposals.iter().map(delta_to_proto).collect(), error_code: String::new(), })), - Err(e) => Ok(Response::new(GetDeltaProposalsResponse { - success: false, - message: e.to_string(), - proposals: vec![], - error_code: e.code().to_string(), - })), + Err(e) => Err(Status::from(e)), } } @@ -326,12 +288,7 @@ impl Guardian for GuardianService { proposal: Some(delta_to_proto(&response.proposal)), error_code: String::new(), })), - Err(e) => Ok(Response::new(GetDeltaProposalResponse { - success: false, - message: e.to_string(), - proposal: None, - error_code: e.code().to_string(), - })), + Err(e) => Err(Status::from(e)), } } @@ -360,12 +317,7 @@ impl Guardian for GuardianService { delta: Some(delta_to_proto(&response.delta)), error_code: String::new(), })), - Err(e) => Ok(Response::new(SignDeltaProposalResponse { - success: false, - message: e.to_string(), - delta: None, - error_code: e.code().to_string(), - })), + Err(e) => Err(Status::from(e)), } } diff --git a/crates/server/src/api/http.rs b/crates/server/src/api/http.rs index 688beb75..06bb7ec2 100644 --- a/crates/server/src/api/http.rs +++ b/crates/server/src/api/http.rs @@ -112,13 +112,6 @@ pub struct ConfigureResponse { pub code: Option<&'static str>, } -#[derive(Serialize, utoipa::ToSchema)] -pub struct ErrorResponse { - pub success: bool, - pub code: &'static str, - pub error: String, -} - /// Configure (register) an account with its authorization set and /// initial state. Requires the signed `x-pubkey` / `x-signature` / /// `x-timestamp` auth headers. @@ -141,14 +134,18 @@ pub async fn configure( let request_payload = match request_payload_from_serializable(&payload) { Ok(request_payload) => request_payload, Err(e) => { + // Log the raw detail; return only the user-safe message + stable + // code (feature 009-human-readable-errors). + let err = GuardianError::InvalidInput(e); + tracing::debug!(code = err.code(), detail = %err, "configure request invalid"); return ( StatusCode::BAD_REQUEST, Json(ConfigureResponse { success: false, - message: e, + message: err.user_message().to_string(), ack_pubkey: None, ack_commitment: None, - code: None, + code: Some(err.code()), }), ); } @@ -168,16 +165,25 @@ pub async fn configure( code: None, }), ), - Err(e) => ( - e.http_status(), - Json(ConfigureResponse { - success: false, - message: e.to_string(), - ack_pubkey: None, - ack_commitment: None, - code: Some(e.code()), - }), - ), + Err(e) => { + // The diagnostic Display string is logged, not returned; the wire + // carries only the user-safe message + stable code (feature 009). + if e.http_status().is_server_error() { + tracing::error!(code = e.code(), detail = %e, "configure failed (5xx)"); + } else { + tracing::debug!(code = e.code(), detail = %e, "configure rejected (4xx)"); + } + ( + e.http_status(), + Json(ConfigureResponse { + success: false, + message: e.user_message().to_string(), + ack_pubkey: None, + ack_commitment: None, + code: Some(e.code()), + }), + ) + } } } diff --git a/crates/server/src/error.rs b/crates/server/src/error.rs index dbb62bf0..ad76e378 100644 --- a/crates/server/src/error.rs +++ b/crates/server/src/error.rs @@ -248,6 +248,112 @@ impl GuardianError { GuardianError::AccountPaused { .. } => "GUARDIAN_ACCOUNT_PAUSED", } } + + /// Short, end-user-safe message for this error (feature + /// `009-human-readable-errors`). Unlike [`Display`], this is **safe to show + /// directly in a wallet UI**: it is a single plain sentence and is + /// safe-by-construction — it never interpolates account IDs, commitments, + /// nonces, signer IDs, file paths, URLs, or raw upstream/RPC text. Wording + /// is NOT part of the stable wire contract; only [`code`](Self::code) is + /// stable. Clients branch and localize on `code`, never on this text. + /// + /// Server-mapped connectivity faults (`network_error`, `rpc_unavailable`, + /// `rpc_validation_failed`) deliberately get a connectivity-style message + /// distinct from the single generic message used for pure internal faults + /// (`storage_error`, `signing_error`, `configuration_error`, + /// `data_unavailable`). + pub fn user_message(&self) -> &'static str { + match self { + // Not found — uniform, no info leak about which thing/account. + GuardianError::AccountNotFound(_) + | GuardianError::StateNotFound(_) + | GuardianError::DeltaNotFound { .. } + | GuardianError::ProposalNotFound { .. } => { + "We couldn't find that. It may have been completed or removed." + } + GuardianError::AccountAlreadyExists(_) => "This account already exists.", + // Validation — not user-actionable beyond retrying with valid input. + GuardianError::InvalidAccountId(_) + | GuardianError::InvalidDelta(_) + | GuardianError::InvalidCommitment(_) + | GuardianError::CommitmentMismatch { .. } + | GuardianError::InvalidProposalSignature(_) + | GuardianError::InvalidInput(_) + | GuardianError::InvalidNetworkConfig(_) + | GuardianError::InvalidEvmProposal(_) + | GuardianError::InvalidCursor(_) + | GuardianError::InvalidLimit(_) + | GuardianError::InvalidStatusFilter(_) => { + "That request couldn't be processed. Please check the details and try again." + } + // Pending-change conflicts. + GuardianError::ConflictPendingDelta + | GuardianError::ConflictPendingProposal + | GuardianError::PendingProposalsLimit { .. } => { + "There's already a pending change for this account. Finish or cancel it first." + } + GuardianError::AuthenticationFailed(_) => { + "Your session has expired. Please sign in again." + } + GuardianError::AuthorizationFailed(_) | GuardianError::SignerNotAuthorized(_) => { + "You're not an authorized signer for this account." + } + GuardianError::InsufficientOperatorPermission { .. } => { + "You don't have permission to do that." + } + GuardianError::ProposalAlreadySigned { .. } => { + "You've already signed this transaction." + } + GuardianError::InsufficientSignatures { .. } => { + "This transaction still needs more signatures." + } + GuardianError::UnsupportedForNetwork { .. } + | GuardianError::UnsupportedEvmChain { .. } => { + "That action isn't supported for this account's network." + } + GuardianError::RateLimitExceeded { .. } => { + "Too many requests — please try again shortly." + } + GuardianError::AccountPaused { .. } => { + "This account is paused and can't approve transactions right now." + } + GuardianError::AccountDataUnavailable(_) => { + "This account's data is temporarily unavailable. Please try again." + } + // Server-mapped connectivity (Guardian reached; the network behind + // it didn't answer) — distinct from internal faults below. + GuardianError::NetworkError(_) + | GuardianError::RpcUnavailable(_) + | GuardianError::RpcValidationFailed(_) => { + "Guardian can't reach the network right now. Please try again." + } + // Pure internal faults — single generic message, no internals. + GuardianError::StorageError(_) + | GuardianError::SigningError(_) + | GuardianError::ConfigurationError(_) + | GuardianError::DataUnavailable(_) => { + "Something went wrong on Guardian's side. Please try again." + } + } + } + + /// Whether retrying the same request could plausibly succeed. Surfaced in + /// `meta.retryable` on the error wire object so clients can drive retry UI + /// uniformly without branching on every `code`. + pub fn retryable(&self) -> bool { + matches!( + self, + GuardianError::AccountDataUnavailable(_) + | GuardianError::StorageError(_) + | GuardianError::NetworkError(_) + | GuardianError::SigningError(_) + | GuardianError::ConfigurationError(_) + | GuardianError::RpcUnavailable(_) + | GuardianError::RpcValidationFailed(_) + | GuardianError::RateLimitExceeded { .. } + | GuardianError::DataUnavailable(_) + ) + } } impl fmt::Display for GuardianError { @@ -379,66 +485,103 @@ impl From for GuardianError { } } +/// Structured machine-readable side-data on the error wire object. Carried +/// identically on the HTTP error body and the gRPC `Status.details` +/// (feature `009-human-readable-errors`). `retryable` is always present; the +/// remaining fields are omitted when they do not apply to the variant. #[derive(Serialize)] -struct ErrorResponse { - success: bool, - code: &'static str, - error: String, +struct ErrorMeta { + /// Whether retrying the same request could plausibly succeed. + retryable: bool, + /// Seconds to wait before retrying. Populated only for + /// `rate_limit_exceeded`; the `Retry-After` header carries the same value. #[serde(skip_serializing_if = "Option::is_none")] retry_after_secs: Option, - /// FR-016 / FR-017: lex-sorted permissions the operator lacks. - /// Populated only for `GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION`. + /// Lex-sorted permissions the operator lacks. Populated only for + /// `GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION`. #[serde(skip_serializing_if = "Option::is_none")] missing_permissions: Option>, - /// FR-016: `false` for permission denials and `GUARDIAN_ACCOUNT_PAUSED`, - /// absent elsewhere. - #[serde(skip_serializing_if = "Option::is_none")] - retryable: Option, - /// RFC 3339 UTC timestamp of the original pause. Populated only - /// for `GUARDIAN_ACCOUNT_PAUSED`. + /// RFC 3339 UTC timestamp of the original pause. Populated only for + /// `GUARDIAN_ACCOUNT_PAUSED`. #[serde(skip_serializing_if = "Option::is_none")] paused_at: Option, /// Reason captured at first pause. Populated only for - /// `GUARDIAN_ACCOUNT_PAUSED` (may be absent within that variant - /// if the reason field is null). + /// `GUARDIAN_ACCOUNT_PAUSED` (may itself be absent within that variant). #[serde(skip_serializing_if = "Option::is_none")] paused_reason: Option, } -impl IntoResponse for GuardianError { - fn into_response(self) -> Response { - let status = self.http_status(); - let retry_after_secs = match &self { +/// The single error object on the wire: `{ code, message, meta }`. Identical +/// shape on the HTTP error body and the gRPC `Status.details` (feature +/// `009-human-readable-errors`). The legacy `success`/`error` fields are +/// gone; the diagnostic [`Display`](GuardianError) string is logged +/// server-side only and never returned. +#[derive(Serialize)] +struct ErrorBody { + /// Stable machine-readable code; the client branch + i18n key. + code: &'static str, + /// User-safe sentence; safe to display verbatim. Wording is not stable. + message: &'static str, + meta: ErrorMeta, +} + +impl GuardianError { + /// Build the structured `meta` block for this error. + fn error_meta(&self) -> ErrorMeta { + let retry_after_secs = match self { GuardianError::RateLimitExceeded { retry_after_secs, .. } => Some(*retry_after_secs), _ => None, }; - let (missing_permissions, retryable, paused_at, paused_reason) = match &self { + let (missing_permissions, paused_at, paused_reason) = match self { GuardianError::InsufficientOperatorPermission { missing_permissions, - } => (Some(missing_permissions.clone()), Some(false), None, None), + } => (Some(missing_permissions.clone()), None, None), GuardianError::AccountPaused { paused_at, paused_reason, - } => ( - None, - Some(false), - Some(paused_at.to_rfc3339()), - paused_reason.clone(), - ), - _ => (None, None, None, None), + } => (None, Some(paused_at.to_rfc3339()), paused_reason.clone()), + _ => (None, None, None), }; - let body = Json(ErrorResponse { - success: false, - code: self.code(), - error: self.to_string(), + ErrorMeta { + retryable: self.retryable(), retry_after_secs, missing_permissions, - retryable, paused_at, paused_reason, - }); + } + } + + /// Build the full `{ code, message, meta }` wire object. Shared by the + /// HTTP `IntoResponse` and the gRPC `Status` conversion so the two + /// surfaces return a byte-identical object (Constitution II parity). + fn error_body(&self) -> ErrorBody { + ErrorBody { + code: self.code(), + message: self.user_message(), + meta: self.error_meta(), + } + } +} + +impl IntoResponse for GuardianError { + fn into_response(self) -> Response { + let status = self.http_status(); + // FR-003: the diagnostic Display string is logged for operators, never + // returned on the wire (removes the disclosure risk by construction). + if status.is_server_error() { + tracing::error!(code = self.code(), detail = %self, "guardian error (HTTP 5xx)"); + } else { + tracing::debug!(code = self.code(), detail = %self, "guardian error (HTTP 4xx)"); + } + let retry_after_secs = match &self { + GuardianError::RateLimitExceeded { + retry_after_secs, .. + } => Some(*retry_after_secs), + _ => None, + }; + let body = Json(self.error_body()); if let Some(retry_after_secs) = retry_after_secs { ( status, @@ -454,22 +597,16 @@ impl IntoResponse for GuardianError { impl From for tonic::Status { fn from(err: GuardianError) -> Self { - match &err { - GuardianError::AccountPaused { - paused_at, - paused_reason, - } => { - let details = serde_json::json!({ - "code": err.code(), - "paused_at": paused_at.to_rfc3339(), - "paused_reason": paused_reason, - }) - .to_string() - .into_bytes(); - tonic::Status::with_details(err.grpc_status(), err.to_string(), details.into()) - } - _ => tonic::Status::new(err.grpc_status(), err.to_string()), + // gRPC carries the same `{ code, message, meta }` object as HTTP, in + // `Status.details`, for every error. `Status.message` is the user-safe + // message; the diagnostic Display string is logged, not returned. + if err.http_status().is_server_error() { + tracing::error!(code = err.code(), detail = %err, "guardian error (gRPC internal)"); + } else { + tracing::debug!(code = err.code(), detail = %err, "guardian error (gRPC)"); } + let details = serde_json::to_vec(&err.error_body()).unwrap_or_default(); + tonic::Status::with_details(err.grpc_status(), err.user_message(), details.into()) } } @@ -901,10 +1038,20 @@ mod tests { #[test] fn into_tonic_status() { + // Status.message is now the user-safe message (not the Display detail + // "bad creds", which is logged, not returned); details carry the + // identical { code, message, meta } object as the HTTP body. let err = GuardianError::AuthenticationFailed("bad creds".into()); + let user_message = err.user_message(); let status: tonic::Status = err.into(); assert_eq!(status.code(), tonic::Code::Unauthenticated); - assert!(status.message().contains("bad creds")); + assert_eq!(status.message(), user_message); + assert!(!status.message().contains("bad creds")); + let details: serde_json::Value = + serde_json::from_slice(status.details()).expect("details are JSON"); + assert_eq!(details["code"], "authentication_failed"); + assert_eq!(details["message"], user_message); + assert_eq!(details["meta"]["retryable"], serde_json::Value::Bool(false)); } // --- Dashboard pagination error variants (FR-028) --- @@ -993,17 +1140,19 @@ mod tests { serde_json::from_slice(&body_bytes).expect("body is valid JSON"); assert_eq!(status, StatusCode::FORBIDDEN); - assert_eq!(parsed["success"], serde_json::Value::Bool(false)); + // New shape: no `success`/`error`; `code` + `message` at top level, + // structured side-data under `meta`. + assert!(parsed.get("success").is_none()); + assert!(parsed.get("error").is_none()); assert_eq!(parsed["code"], "GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION"); + assert!(parsed["message"].is_string()); assert_eq!( - parsed["missing_permissions"], + parsed["meta"]["missing_permissions"], serde_json::json!(["accounts:pause"]) ); - assert_eq!(parsed["retryable"], serde_json::Value::Bool(false)); - // The new fields are populated; the legacy `retry_after_secs` - // is absent for this code (additive extension preserves the - // existing envelope shape for every other code). - assert!(parsed.get("retry_after_secs").is_none()); + assert_eq!(parsed["meta"]["retryable"], serde_json::Value::Bool(false)); + // `retry_after_secs` does not apply to this code. + assert!(parsed["meta"].get("retry_after_secs").is_none()); } // -- Feature 001-account-pausing: AccountPaused -- @@ -1039,10 +1188,16 @@ mod tests { serde_json::from_slice(&body_bytes).expect("body is valid JSON"); assert_eq!(status, StatusCode::CONFLICT); + assert!(parsed.get("success").is_none()); + assert!(parsed.get("error").is_none()); assert_eq!(parsed["code"], "GUARDIAN_ACCOUNT_PAUSED"); - assert_eq!(parsed["paused_at"], "2026-05-19T14:23:00+00:00"); - assert_eq!(parsed["paused_reason"], "suspected cosigner compromise"); - assert_eq!(parsed["retryable"], serde_json::Value::Bool(false)); + assert!(parsed["message"].is_string()); + assert_eq!(parsed["meta"]["paused_at"], "2026-05-19T14:23:00+00:00"); + assert_eq!( + parsed["meta"]["paused_reason"], + "suspected cosigner compromise" + ); + assert_eq!(parsed["meta"]["retryable"], serde_json::Value::Bool(false)); } #[test] @@ -1058,23 +1213,203 @@ mod tests { let details: serde_json::Value = serde_json::from_slice(status.details()).expect("details are JSON"); assert_eq!(details["code"], "GUARDIAN_ACCOUNT_PAUSED"); - assert_eq!(details["paused_at"], "2026-05-19T14:23:00+00:00"); - assert_eq!(details["paused_reason"], "compromise"); + assert!(details["message"].is_string()); + assert_eq!(details["meta"]["paused_at"], "2026-05-19T14:23:00+00:00"); + assert_eq!(details["meta"]["paused_reason"], "compromise"); + assert_eq!(details["meta"]["retryable"], serde_json::Value::Bool(false)); } #[test] - fn other_errors_do_not_carry_missing_permissions_or_retryable() { + fn plain_error_body_has_code_message_meta_and_no_legacy_fields() { use axum::body::to_bytes; - // A non-permission error must NOT populate the new fields, so - // existing dashboard error parsers see no change (research.md - // Decision 1: additive extension). + // A plain error: { code, message, meta:{retryable} } — no `success`, + // no `error`, and no `meta` fields that don't apply to the variant. let err = GuardianError::AccountNotFound("0xabc".into()); let response = err.into_response(); let body_bytes = futures::executor::block_on(to_bytes(response.into_body(), usize::MAX)) .expect("body bytes"); let parsed: serde_json::Value = serde_json::from_slice(&body_bytes).expect("body is valid JSON"); - assert!(parsed.get("missing_permissions").is_none()); - assert!(parsed.get("retryable").is_none()); + assert!(parsed.get("success").is_none()); + assert!(parsed.get("error").is_none()); + assert_eq!(parsed["code"], "account_not_found"); + assert!(parsed["message"].is_string()); + assert_eq!(parsed["meta"]["retryable"], serde_json::Value::Bool(false)); + assert!(parsed["meta"].get("missing_permissions").is_none()); + assert!(parsed["meta"].get("paused_at").is_none()); + assert!(parsed["meta"].get("retry_after_secs").is_none()); + } + + // --- Feature 009-human-readable-errors: user_message() --- + + /// Every variant, built with deliberately sensitive-looking payloads so + /// the sanitization scan (SC-002) is meaningful. + fn all_variants_with_sensitive_payloads() -> Vec { + let paused_at = chrono::DateTime::parse_from_rfc3339("2026-05-19T14:23:00Z") + .unwrap() + .with_timezone(&Utc); + vec![ + GuardianError::AccountNotFound("0xDEADBEEFACCOUNT".into()), + GuardianError::AccountAlreadyExists("0xDEADBEEFACCOUNT".into()), + GuardianError::AccountDataUnavailable("0xDEADBEEFACCOUNT".into()), + GuardianError::InvalidAccountId("0xDEADBEEFACCOUNT".into()), + GuardianError::StateNotFound("0xDEADBEEFACCOUNT".into()), + GuardianError::DeltaNotFound { + account_id: "0xDEADBEEFACCOUNT".into(), + nonce: 42, + }, + GuardianError::InvalidDelta("postgres://secret@host/db".into()), + GuardianError::ConflictPendingDelta, + GuardianError::ConflictPendingProposal, + GuardianError::PendingProposalsLimit { limit: 7 }, + GuardianError::CommitmentMismatch { + expected: "0xAAAACOMMITMENT".into(), + actual: "0xBBBBCOMMITMENT".into(), + }, + GuardianError::InvalidCommitment("0xAAAACOMMITMENT".into()), + GuardianError::AuthenticationFailed("bad creds for 0xSIGNER".into()), + GuardianError::AuthorizationFailed("0xSIGNER not in policy".into()), + GuardianError::InvalidInput("/var/secret/path".into()), + GuardianError::StorageError("/var/lib/guardian/db: disk full".into()), + GuardianError::NetworkError("https://rpc.internal:8080 refused".into()), + GuardianError::SigningError("falcon: 0xPRIVATEKEYMATERIAL".into()), + GuardianError::ConfigurationError("postgres://u:p@h/db".into()), + GuardianError::ProposalNotFound { + account_id: "0xDEADBEEFACCOUNT".into(), + commitment: "0xAAAACOMMITMENT".into(), + }, + GuardianError::ProposalAlreadySigned { + signer_id: "0xSIGNER".into(), + }, + GuardianError::InvalidProposalSignature("0xSIGNATURE".into()), + GuardianError::UnsupportedForNetwork { + network: "evm".into(), + operation: "push_delta".into(), + }, + GuardianError::UnsupportedEvmChain { chain_id: 1 }, + GuardianError::InvalidNetworkConfig("https://rpc.internal".into()), + GuardianError::RpcUnavailable("https://rpc.internal:8080".into()), + GuardianError::RpcValidationFailed("https://rpc.internal".into()), + GuardianError::SignerNotAuthorized("0xSIGNER".into()), + GuardianError::InvalidEvmProposal("0xCALLDATA".into()), + GuardianError::InsufficientSignatures { + required: 3, + got: 1, + }, + GuardianError::RateLimitExceeded { + retry_after_secs: 30, + scope: "ip:10.0.0.1".into(), + }, + GuardianError::InvalidCursor("0xTAMPERED".into()), + GuardianError::InvalidLimit("9999".into()), + GuardianError::InvalidStatusFilter("'; DROP TABLE".into()), + GuardianError::InsufficientOperatorPermission { + missing_permissions: vec!["accounts:pause".into()], + }, + GuardianError::DataUnavailable("/var/lib/guardian unreadable".into()), + GuardianError::AccountPaused { + paused_at, + paused_reason: Some("0xSIGNER compromise".into()), + }, + ] + } + + #[test] + fn user_message_is_nonempty_for_every_variant() { + // SC-001: 100% of variants return a non-empty, single-sentence message. + for err in all_variants_with_sensitive_payloads() { + let msg = err.user_message(); + assert!(!msg.is_empty(), "empty user_message for {}", err.code()); + assert!( + msg.ends_with('.'), + "message for {} should be a sentence: {msg:?}", + err.code() + ); + } + } + + #[test] + fn user_message_never_leaks_sensitive_payload() { + // SC-002: the user-safe message must never echo identifiers, hashes, + // paths, URLs, or other payload values that the Display string carries. + let disallowed = [ + "0xDEADBEEFACCOUNT", + "0xAAAACOMMITMENT", + "0xBBBBCOMMITMENT", + "0xSIGNER", + "0xSIGNATURE", + "0xPRIVATEKEYMATERIAL", + "0xCALLDATA", + "0xTAMPERED", + "postgres://", + "https://", + "/var/", + "DROP TABLE", + "10.0.0.1", + ]; + for err in all_variants_with_sensitive_payloads() { + let msg = err.user_message(); + for needle in disallowed { + assert!( + !msg.contains(needle), + "user_message for {} leaked {needle:?}: {msg:?}", + err.code() + ); + } + } + } + + #[test] + fn connectivity_and_internal_messages_are_distinct() { + // SC-005: server-mapped connectivity faults vs pure internal faults + // must surface different copy. + let connectivity = GuardianError::RpcUnavailable("x".into()).user_message(); + assert_eq!( + GuardianError::NetworkError("x".into()).user_message(), + connectivity + ); + assert_eq!( + GuardianError::RpcValidationFailed("x".into()).user_message(), + connectivity + ); + let internal = GuardianError::StorageError("x".into()).user_message(); + assert_eq!( + GuardianError::SigningError("x".into()).user_message(), + internal + ); + assert_eq!( + GuardianError::ConfigurationError("x".into()).user_message(), + internal + ); + assert_eq!( + GuardianError::DataUnavailable("x".into()).user_message(), + internal + ); + assert_ne!(connectivity, internal); + } + + #[test] + fn rate_limit_meta_carries_retry_after_and_retryable_true() { + use axum::body::to_bytes; + let err = GuardianError::RateLimitExceeded { + retry_after_secs: 30, + scope: "ip".into(), + }; + let response = err.into_response(); + // Retry-After header preserved. + assert_eq!( + response + .headers() + .get("Retry-After") + .and_then(|v| v.to_str().ok()), + Some("30") + ); + let body_bytes = futures::executor::block_on(to_bytes(response.into_body(), usize::MAX)) + .expect("body bytes"); + let parsed: serde_json::Value = + serde_json::from_slice(&body_bytes).expect("body is valid JSON"); + assert_eq!(parsed["code"], "rate_limit_exceeded"); + assert_eq!(parsed["meta"]["retryable"], serde_json::Value::Bool(true)); + assert_eq!(parsed["meta"]["retry_after_secs"], serde_json::json!(30)); } } diff --git a/crates/server/src/middleware/rate_limit.rs b/crates/server/src/middleware/rate_limit.rs index 3b87548a..58a80db4 100644 --- a/crates/server/src/middleware/rate_limit.rs +++ b/crates/server/src/middleware/rate_limit.rs @@ -4,12 +4,10 @@ //! Uses two windows: burst (per second) and sustained (per minute). use axum::{ - Json, body::Body, - http::{Request, Response, StatusCode}, + http::{Request, Response}, response::IntoResponse, }; -use serde::Serialize; use std::{ collections::HashMap, env, @@ -236,14 +234,6 @@ impl RateLimitType { } } -/// Rate limit error response -#[derive(Debug, Serialize)] -pub struct RateLimitResponse { - pub success: bool, - pub error: String, - pub retry_after_secs: u32, -} - /// Tower layer for rate limiting #[derive(Debug, Clone)] pub struct RateLimitLayer { @@ -364,22 +354,14 @@ where "Request rate limited" ); - let response = RateLimitResponse { - success: false, - error: format!( - "Rate limit exceeded ({} limit). Retry after {} seconds.", - limit_type.as_str(), - retry_after - ), + // Reuse the canonical error object (feature 009): this + // yields `{ code, message, meta }` plus the `Retry-After` + // header, identical to every other Guardian error body. + Ok(crate::error::GuardianError::RateLimitExceeded { retry_after_secs: retry_after, - }; - - Ok(( - StatusCode::TOO_MANY_REQUESTS, - [("Retry-After", retry_after.to_string())], - Json(response), - ) - .into_response()) + scope: limit_type.as_str().to_string(), + } + .into_response()) } } }) @@ -599,18 +581,35 @@ mod tests { assert_eq!(original.as_str(), cloned.as_str()); } - #[test] - fn test_rate_limit_response_serialization() { - let response = RateLimitResponse { - success: false, - error: "Rate limit exceeded".to_string(), - retry_after_secs: 60, - }; + #[tokio::test] + async fn test_rate_limit_response_uses_canonical_error_envelope() { + use axum::body::to_bytes; + use axum::http::StatusCode; + use axum::response::IntoResponse; - let json = serde_json::to_string(&response).unwrap(); - assert!(json.contains("\"success\":false")); - assert!(json.contains("\"retry_after_secs\":60")); - assert!(json.contains("Rate limit exceeded")); + let response = crate::error::GuardianError::RateLimitExceeded { + retry_after_secs: 60, + scope: "sustained".to_string(), + } + .into_response(); + + assert_eq!(response.status(), StatusCode::TOO_MANY_REQUESTS); + assert_eq!( + response + .headers() + .get("Retry-After") + .and_then(|v| v.to_str().ok()), + Some("60") + ); + let body = to_bytes(response.into_body(), usize::MAX).await.unwrap(); + let parsed: serde_json::Value = serde_json::from_slice(&body).unwrap(); + // Canonical { code, message, meta } shape — no legacy success/error. + assert!(parsed.get("success").is_none()); + assert!(parsed.get("error").is_none()); + assert_eq!(parsed["code"], "rate_limit_exceeded"); + assert!(parsed["message"].is_string()); + assert_eq!(parsed["meta"]["retryable"], serde_json::Value::Bool(true)); + assert_eq!(parsed["meta"]["retry_after_secs"], serde_json::json!(60)); } #[test] diff --git a/crates/server/src/openapi.rs b/crates/server/src/openapi.rs index ba92c287..10fe5f52 100644 --- a/crates/server/src/openapi.rs +++ b/crates/server/src/openapi.rs @@ -20,31 +20,22 @@ use utoipa::{ openapi::security::{ApiKey, ApiKeyValue, SecurityScheme}, }; -/// Wire shape of a Guardian error response body. Mirrors the envelope -/// produced by [`crate::error::GuardianError`]'s `IntoResponse` impl. -/// Documented as the body of every non-2xx response. Optional fields -/// are populated only for the error codes that carry them. +/// Structured machine-readable side-data on the error wire object +/// (feature `009-human-readable-errors`). `retryable` is always present; +/// the rest appear only for the codes that carry them. #[derive(Debug, Serialize, utoipa::ToSchema)] -pub struct ApiErrorResponse { - /// Always `false` for error responses. - pub success: bool, - /// Stable, machine-readable error code (e.g. `account_not_found`). - pub code: String, - /// Human-readable error message. - pub error: String, +pub struct ApiErrorMeta { + /// Whether retrying the same request could plausibly succeed. + pub retryable: bool, /// Seconds to wait before retrying. Present only for - /// `rate_limit_exceeded`. + /// `rate_limit_exceeded` (the `Retry-After` header carries the same value). #[serde(skip_serializing_if = "Option::is_none")] pub retry_after_secs: Option, /// Lex-sorted permissions the operator lacks. Present only for /// `GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION`. #[serde(skip_serializing_if = "Option::is_none")] pub missing_permissions: Option>, - /// `false` for permission denials and `GUARDIAN_ACCOUNT_PAUSED`. - #[serde(skip_serializing_if = "Option::is_none")] - pub retryable: Option, - /// RFC 3339 pause timestamp. Present only for - /// `GUARDIAN_ACCOUNT_PAUSED`. + /// RFC 3339 pause timestamp. Present only for `GUARDIAN_ACCOUNT_PAUSED`. #[serde(skip_serializing_if = "Option::is_none")] pub paused_at: Option, /// Pause reason. Present only for `GUARDIAN_ACCOUNT_PAUSED`. @@ -52,6 +43,24 @@ pub struct ApiErrorResponse { pub paused_reason: Option, } +/// Wire shape of a Guardian error response body: `{ code, message, meta }` +/// (feature `009-human-readable-errors`). Mirrors the object produced by +/// [`crate::error::GuardianError`]'s `IntoResponse` impl and carried +/// identically on the gRPC `Status.details`. The legacy `success`/`error` +/// fields are gone; the diagnostic detail is logged server-side only. +/// Documented as the body of every non-2xx response. +#[derive(Debug, Serialize, utoipa::ToSchema)] +pub struct ApiErrorResponse { + /// Stable, machine-readable error code (e.g. `account_not_found`). The + /// client branch + i18n key. Branch on this, never on `message`. + pub code: String, + /// Short, user-safe message; safe to display verbatim. Wording is not + /// part of the stable contract. + pub message: String, + /// Structured machine-readable side-data. + pub meta: ApiErrorMeta, +} + /// Security scheme name for the signed-request public key header. pub const SEC_PUBKEY: &str = "x-pubkey"; /// Security scheme name for the signed-request signature header. @@ -186,7 +195,7 @@ impl Modify for CommonResponsesAddon { crate::api::http::get_delta_proposal, crate::api::http::sign_delta_proposal, ), - components(schemas(ApiErrorResponse, crate::services::StatusResponse)), + components(schemas(ApiErrorResponse, ApiErrorMeta, crate::services::StatusResponse)), modifiers(&ClientSecurityAddon, &CommonResponsesAddon), tags((name = "client", description = "Client-facing API consumed by SDKs and packages.")), )] @@ -217,7 +226,7 @@ pub struct ClientApiDoc; crate::api::dashboard_feeds::list_global_deltas_handler, crate::api::dashboard_feeds::list_global_proposals_handler, ), - components(schemas(ApiErrorResponse)), + components(schemas(ApiErrorResponse, ApiErrorMeta)), modifiers(&DashboardSecurityAddon, &CommonResponsesAddon), tags((name = "dashboard", description = "Operator dashboard API.")), )] @@ -244,7 +253,7 @@ pub struct DashboardApiDoc; crate::api::evm::get_executable_evm_proposal, crate::api::evm::cancel_evm_proposal, ), - components(schemas(ApiErrorResponse)), + components(schemas(ApiErrorResponse, ApiErrorMeta)), modifiers(&EvmSecurityAddon, &CommonResponsesAddon), tags((name = "evm", description = "EVM smart-account API.")), )] diff --git a/crates/server/src/testing/integration/auth_grpc.rs b/crates/server/src/testing/integration/auth_grpc.rs index a87ff1a2..1ea09479 100644 --- a/crates/server/src/testing/integration/auth_grpc.rs +++ b/crates/server/src/testing/integration/auth_grpc.rs @@ -103,17 +103,18 @@ async fn test_grpc_push_delta_unauthorized_cosigner() { let request = create_signed_request_with_auth(push_req, &account_id_hex, &unauthorized_signer); let push_response = service.push_delta(request).await; - // Should succeed as a gRPC call but return failure in response - assert!(push_response.is_ok(), "gRPC call should succeed"); - let push_response = push_response.unwrap().into_inner(); - assert!( - !push_response.success, - "Push should fail with unauthorized cosigner" - ); - assert!( - push_response.message.contains("not authorized"), - "Error message should mention authorization" - ); + // Errors now surface as a gRPC Status (feature 009), not an in-band + // success=false response. + let status = push_response.expect_err("unauthorized push must be a gRPC error status"); + assert_eq!(status.code(), tonic::Code::Unauthenticated); + // Status.message is the user-safe sentence; the raw "not authorized" + // detail is logged server-side, not returned. + assert!(!status.message().is_empty()); + let details: serde_json::Value = + serde_json::from_slice(status.details()).expect("Status.details is JSON"); + assert!(details["code"].is_string(), "details carry a stable code"); + assert!(details["message"].is_string()); + assert_eq!(details["meta"]["retryable"], serde_json::Value::Bool(false)); } #[tokio::test] diff --git a/crates/server/src/testing/integration/error_envelope_http.rs b/crates/server/src/testing/integration/error_envelope_http.rs index 0244c253..67619e4a 100644 --- a/crates/server/src/testing/integration/error_envelope_http.rs +++ b/crates/server/src/testing/integration/error_envelope_http.rs @@ -20,15 +20,24 @@ use serde::Deserialize; use serde_json::json; use tower::Service; +#[derive(Deserialize, Debug)] +struct ErrorMeta { + retryable: bool, + paused_at: Option, + paused_reason: Option, +} + +/// New error wire shape (feature 009): `{ code, message, meta }`. The legacy +/// `success`/`error` fields are gone; the diagnostic detail is logged only. #[derive(Deserialize, Debug)] struct ErrorEnvelope { - success: bool, code: String, - error: String, + message: String, + meta: ErrorMeta, } async fn parse_envelope(response: axum::http::Response) -> ErrorEnvelope { - let status = response.status(); + let _status = response.status(); let body_bytes = to_bytes(response.into_body(), usize::MAX).await.unwrap(); let value: serde_json::Value = serde_json::from_slice(&body_bytes).unwrap_or_else(|e| { panic!( @@ -47,12 +56,20 @@ async fn parse_envelope(response: axum::http::Response) -> ErrorEnvelope { !obj.contains_key("nonce"), "error body must not be a delta object; got: {value}" ); + // Legacy fields must be gone (breaking reshape). + assert!( + !obj.contains_key("success"), + "error body must not carry legacy `success`; got: {value}" + ); + assert!( + !obj.contains_key("error"), + "error body must not carry the diagnostic `error` field; got: {value}" + ); let envelope: ErrorEnvelope = serde_json::from_value(value.clone()) .unwrap_or_else(|e| panic!("body does not match ErrorEnvelope ({e}): {value}")); - assert!(!envelope.success, "success must be false ({status})"); assert!(!envelope.code.is_empty(), "code must be non-empty"); - assert!(!envelope.error.is_empty(), "error must be non-empty"); + assert!(!envelope.message.is_empty(), "message must be non-empty"); envelope } @@ -280,22 +297,30 @@ async fn push_delta_proposal_on_paused_account_returns_409_envelope() { "paused-account error body must not be shaped like a domain object; got: {value}" ); + // Legacy fields gone; pause context now lives under `meta`. + assert!(!obj.contains_key("success")); + assert!(!obj.contains_key("error")); + let envelope: ErrorEnvelope = serde_json::from_value(value.clone()) .unwrap_or_else(|e| panic!("body does not match ErrorEnvelope ({e}): {value}")); - assert!(!envelope.success); assert_eq!(envelope.code, "GUARDIAN_ACCOUNT_PAUSED"); - assert!(envelope.error.contains(pause_reason)); + // The user-safe message is sanitized: it must NOT echo the internal + // pause reason (that now travels only in `meta.paused_reason`). + assert!(!envelope.message.is_empty()); + assert!( + !envelope.message.contains(pause_reason), + "user-safe message must not leak the pause reason; got: {:?}", + envelope.message + ); - let paused_at = obj - .get("paused_at") - .and_then(|v| v.as_str()) - .expect("paused_at must be a string"); + let paused_at = envelope.meta.paused_at.expect("meta.paused_at must be set"); assert!(!paused_at.is_empty(), "paused_at must be non-empty"); - let paused_reason = obj - .get("paused_reason") - .and_then(|v| v.as_str()) - .expect("paused_reason must be a string"); - assert_eq!(paused_reason, pause_reason); + assert_eq!( + envelope.meta.paused_reason.as_deref(), + Some(pause_reason), + "meta.paused_reason must carry the internal reason" + ); + assert!(!envelope.meta.retryable, "paused account is not retryable"); } #[tokio::test] diff --git a/crates/server/src/testing/integration/proposals_grpc.rs b/crates/server/src/testing/integration/proposals_grpc.rs index a51b74cf..8809ae53 100644 --- a/crates/server/src/testing/integration/proposals_grpc.rs +++ b/crates/server/src/testing/integration/proposals_grpc.rs @@ -298,16 +298,14 @@ async fn test_grpc_sign_delta_proposal_not_found() { let request = create_signed_request_with_auth(sign_proposal_req, &account_id_hex, &signer); let sign_response = service.sign_delta_proposal(request).await; - assert!(sign_response.is_ok(), "gRPC call should succeed"); - let sign_response = sign_response.unwrap().into_inner(); - assert!( - !sign_response.success, - "Sign should fail for nonexistent proposal" - ); - assert!( - sign_response.message.contains("not found") || sign_response.message.contains("Proposal"), - "Error message should mention proposal not found" - ); + // Errors now surface as a gRPC Status (feature 009). + let status = sign_response.expect_err("signing a missing proposal must be a gRPC error"); + assert_eq!(status.code(), tonic::Code::NotFound); + assert!(!status.message().is_empty()); + let details: serde_json::Value = + serde_json::from_slice(status.details()).expect("Status.details is JSON"); + assert_eq!(details["code"], "proposal_not_found"); + assert!(details["message"].is_string()); } #[tokio::test] @@ -362,16 +360,14 @@ async fn test_grpc_push_delta_proposal_unauthorized() { create_signed_request_with_auth(push_proposal_req, &account_id_hex, &unauthorized_signer); let push_response = service.push_delta_proposal(request).await; - assert!(push_response.is_ok(), "gRPC call should succeed"); - let push_response = push_response.unwrap().into_inner(); - assert!( - !push_response.success, - "Push should fail with unauthorized cosigner" - ); - assert!( - push_response.message.contains("not authorized"), - "Error message should mention authorization" - ); + // Errors now surface as a gRPC Status (feature 009). + let status = push_response.expect_err("unauthorized proposal push must be a gRPC error"); + assert_eq!(status.code(), tonic::Code::Unauthenticated); + assert!(!status.message().is_empty()); + let details: serde_json::Value = + serde_json::from_slice(status.details()).expect("Status.details is JSON"); + assert!(details["code"].is_string()); + assert_eq!(details["meta"]["retryable"], serde_json::Value::Bool(false)); } #[tokio::test] diff --git a/docs/openapi-client.json b/docs/openapi-client.json index c925ce94..8143abe4 100644 --- a/docs/openapi-client.json +++ b/docs/openapi-client.json @@ -937,23 +937,13 @@ }, "components": { "schemas": { - "ApiErrorResponse": { + "ApiErrorMeta": { "type": "object", - "description": "Wire shape of a Guardian error response body. Mirrors the envelope\nproduced by [`crate::error::GuardianError`]'s `IntoResponse` impl.\nDocumented as the body of every non-2xx response. Optional fields\nare populated only for the error codes that carry them.", + "description": "Structured machine-readable side-data on the error wire object\n(feature `009-human-readable-errors`). `retryable` is always present;\nthe rest appear only for the codes that carry them.", "required": [ - "success", - "code", - "error" + "retryable" ], "properties": { - "code": { - "type": "string", - "description": "Stable, machine-readable error code (e.g. `account_not_found`)." - }, - "error": { - "type": "string", - "description": "Human-readable error message." - }, "missing_permissions": { "type": [ "array", @@ -969,7 +959,7 @@ "string", "null" ], - "description": "RFC 3339 pause timestamp. Present only for\n`GUARDIAN_ACCOUNT_PAUSED`." + "description": "RFC 3339 pause timestamp. Present only for `GUARDIAN_ACCOUNT_PAUSED`." }, "paused_reason": { "type": [ @@ -984,19 +974,35 @@ "null" ], "format": "int32", - "description": "Seconds to wait before retrying. Present only for\n`rate_limit_exceeded`.", + "description": "Seconds to wait before retrying. Present only for\n`rate_limit_exceeded` (the `Retry-After` header carries the same value).", "minimum": 0 }, "retryable": { - "type": [ - "boolean", - "null" - ], - "description": "`false` for permission denials and `GUARDIAN_ACCOUNT_PAUSED`." - }, - "success": { "type": "boolean", - "description": "Always `false` for error responses." + "description": "Whether retrying the same request could plausibly succeed." + } + } + }, + "ApiErrorResponse": { + "type": "object", + "description": "Wire shape of a Guardian error response body: `{ code, message, meta }`\n(feature `009-human-readable-errors`). Mirrors the object produced by\n[`crate::error::GuardianError`]'s `IntoResponse` impl and carried\nidentically on the gRPC `Status.details`. The legacy `success`/`error`\nfields are gone; the diagnostic detail is logged server-side only.\nDocumented as the body of every non-2xx response.", + "required": [ + "code", + "message", + "meta" + ], + "properties": { + "code": { + "type": "string", + "description": "Stable, machine-readable error code (e.g. `account_not_found`). The\nclient branch + i18n key. Branch on this, never on `message`." + }, + "message": { + "type": "string", + "description": "Short, user-safe message; safe to display verbatim. Wording is not\npart of the stable contract." + }, + "meta": { + "$ref": "#/components/schemas/ApiErrorMeta", + "description": "Structured machine-readable side-data." } } }, diff --git a/docs/openapi-dashboard.json b/docs/openapi-dashboard.json index ca4d5bf0..ba6f2221 100644 --- a/docs/openapi-dashboard.json +++ b/docs/openapi-dashboard.json @@ -1350,23 +1350,13 @@ "paused" ] }, - "ApiErrorResponse": { + "ApiErrorMeta": { "type": "object", - "description": "Wire shape of a Guardian error response body. Mirrors the envelope\nproduced by [`crate::error::GuardianError`]'s `IntoResponse` impl.\nDocumented as the body of every non-2xx response. Optional fields\nare populated only for the error codes that carry them.", + "description": "Structured machine-readable side-data on the error wire object\n(feature `009-human-readable-errors`). `retryable` is always present;\nthe rest appear only for the codes that carry them.", "required": [ - "success", - "code", - "error" + "retryable" ], "properties": { - "code": { - "type": "string", - "description": "Stable, machine-readable error code (e.g. `account_not_found`)." - }, - "error": { - "type": "string", - "description": "Human-readable error message." - }, "missing_permissions": { "type": [ "array", @@ -1382,7 +1372,7 @@ "string", "null" ], - "description": "RFC 3339 pause timestamp. Present only for\n`GUARDIAN_ACCOUNT_PAUSED`." + "description": "RFC 3339 pause timestamp. Present only for `GUARDIAN_ACCOUNT_PAUSED`." }, "paused_reason": { "type": [ @@ -1397,19 +1387,35 @@ "null" ], "format": "int32", - "description": "Seconds to wait before retrying. Present only for\n`rate_limit_exceeded`.", + "description": "Seconds to wait before retrying. Present only for\n`rate_limit_exceeded` (the `Retry-After` header carries the same value).", "minimum": 0 }, "retryable": { - "type": [ - "boolean", - "null" - ], - "description": "`false` for permission denials and `GUARDIAN_ACCOUNT_PAUSED`." - }, - "success": { "type": "boolean", - "description": "Always `false` for error responses." + "description": "Whether retrying the same request could plausibly succeed." + } + } + }, + "ApiErrorResponse": { + "type": "object", + "description": "Wire shape of a Guardian error response body: `{ code, message, meta }`\n(feature `009-human-readable-errors`). Mirrors the object produced by\n[`crate::error::GuardianError`]'s `IntoResponse` impl and carried\nidentically on the gRPC `Status.details`. The legacy `success`/`error`\nfields are gone; the diagnostic detail is logged server-side only.\nDocumented as the body of every non-2xx response.", + "required": [ + "code", + "message", + "meta" + ], + "properties": { + "code": { + "type": "string", + "description": "Stable, machine-readable error code (e.g. `account_not_found`). The\nclient branch + i18n key. Branch on this, never on `message`." + }, + "message": { + "type": "string", + "description": "Short, user-safe message; safe to display verbatim. Wording is not\npart of the stable contract." + }, + "meta": { + "$ref": "#/components/schemas/ApiErrorMeta", + "description": "Structured machine-readable side-data." } } }, diff --git a/docs/openapi-evm.json b/docs/openapi-evm.json index 98ff4d3e..2bb76f49 100644 --- a/docs/openapi-evm.json +++ b/docs/openapi-evm.json @@ -791,23 +791,13 @@ }, "components": { "schemas": { - "ApiErrorResponse": { + "ApiErrorMeta": { "type": "object", - "description": "Wire shape of a Guardian error response body. Mirrors the envelope\nproduced by [`crate::error::GuardianError`]'s `IntoResponse` impl.\nDocumented as the body of every non-2xx response. Optional fields\nare populated only for the error codes that carry them.", + "description": "Structured machine-readable side-data on the error wire object\n(feature `009-human-readable-errors`). `retryable` is always present;\nthe rest appear only for the codes that carry them.", "required": [ - "success", - "code", - "error" + "retryable" ], "properties": { - "code": { - "type": "string", - "description": "Stable, machine-readable error code (e.g. `account_not_found`)." - }, - "error": { - "type": "string", - "description": "Human-readable error message." - }, "missing_permissions": { "type": [ "array", @@ -823,7 +813,7 @@ "string", "null" ], - "description": "RFC 3339 pause timestamp. Present only for\n`GUARDIAN_ACCOUNT_PAUSED`." + "description": "RFC 3339 pause timestamp. Present only for `GUARDIAN_ACCOUNT_PAUSED`." }, "paused_reason": { "type": [ @@ -838,19 +828,35 @@ "null" ], "format": "int32", - "description": "Seconds to wait before retrying. Present only for\n`rate_limit_exceeded`.", + "description": "Seconds to wait before retrying. Present only for\n`rate_limit_exceeded` (the `Retry-After` header carries the same value).", "minimum": 0 }, "retryable": { - "type": [ - "boolean", - "null" - ], - "description": "`false` for permission denials and `GUARDIAN_ACCOUNT_PAUSED`." - }, - "success": { "type": "boolean", - "description": "Always `false` for error responses." + "description": "Whether retrying the same request could plausibly succeed." + } + } + }, + "ApiErrorResponse": { + "type": "object", + "description": "Wire shape of a Guardian error response body: `{ code, message, meta }`\n(feature `009-human-readable-errors`). Mirrors the object produced by\n[`crate::error::GuardianError`]'s `IntoResponse` impl and carried\nidentically on the gRPC `Status.details`. The legacy `success`/`error`\nfields are gone; the diagnostic detail is logged server-side only.\nDocumented as the body of every non-2xx response.", + "required": [ + "code", + "message", + "meta" + ], + "properties": { + "code": { + "type": "string", + "description": "Stable, machine-readable error code (e.g. `account_not_found`). The\nclient branch + i18n key. Branch on this, never on `message`." + }, + "message": { + "type": "string", + "description": "Short, user-safe message; safe to display verbatim. Wording is not\npart of the stable contract." + }, + "meta": { + "$ref": "#/components/schemas/ApiErrorMeta", + "description": "Structured machine-readable side-data." } } }, diff --git a/docs/openapi.json b/docs/openapi.json index b5b1b04d..99864ad0 100644 --- a/docs/openapi.json +++ b/docs/openapi.json @@ -3052,23 +3052,13 @@ "paused" ] }, - "ApiErrorResponse": { + "ApiErrorMeta": { "type": "object", - "description": "Wire shape of a Guardian error response body. Mirrors the envelope\nproduced by [`crate::error::GuardianError`]'s `IntoResponse` impl.\nDocumented as the body of every non-2xx response. Optional fields\nare populated only for the error codes that carry them.", + "description": "Structured machine-readable side-data on the error wire object\n(feature `009-human-readable-errors`). `retryable` is always present;\nthe rest appear only for the codes that carry them.", "required": [ - "success", - "code", - "error" + "retryable" ], "properties": { - "code": { - "type": "string", - "description": "Stable, machine-readable error code (e.g. `account_not_found`)." - }, - "error": { - "type": "string", - "description": "Human-readable error message." - }, "missing_permissions": { "type": [ "array", @@ -3084,7 +3074,7 @@ "string", "null" ], - "description": "RFC 3339 pause timestamp. Present only for\n`GUARDIAN_ACCOUNT_PAUSED`." + "description": "RFC 3339 pause timestamp. Present only for `GUARDIAN_ACCOUNT_PAUSED`." }, "paused_reason": { "type": [ @@ -3099,19 +3089,35 @@ "null" ], "format": "int32", - "description": "Seconds to wait before retrying. Present only for\n`rate_limit_exceeded`.", + "description": "Seconds to wait before retrying. Present only for\n`rate_limit_exceeded` (the `Retry-After` header carries the same value).", "minimum": 0 }, "retryable": { - "type": [ - "boolean", - "null" - ], - "description": "`false` for permission denials and `GUARDIAN_ACCOUNT_PAUSED`." - }, - "success": { "type": "boolean", - "description": "Always `false` for error responses." + "description": "Whether retrying the same request could plausibly succeed." + } + } + }, + "ApiErrorResponse": { + "type": "object", + "description": "Wire shape of a Guardian error response body: `{ code, message, meta }`\n(feature `009-human-readable-errors`). Mirrors the object produced by\n[`crate::error::GuardianError`]'s `IntoResponse` impl and carried\nidentically on the gRPC `Status.details`. The legacy `success`/`error`\nfields are gone; the diagnostic detail is logged server-side only.\nDocumented as the body of every non-2xx response.", + "required": [ + "code", + "message", + "meta" + ], + "properties": { + "code": { + "type": "string", + "description": "Stable, machine-readable error code (e.g. `account_not_found`). The\nclient branch + i18n key. Branch on this, never on `message`." + }, + "message": { + "type": "string", + "description": "Short, user-safe message; safe to display verbatim. Wording is not\npart of the stable contract." + }, + "meta": { + "$ref": "#/components/schemas/ApiErrorMeta", + "description": "Structured machine-readable side-data." } } }, diff --git a/examples/operator-smoke-web/src/App.tsx b/examples/operator-smoke-web/src/App.tsx index 3ff270cf..b15303e5 100644 --- a/examples/operator-smoke-web/src/App.tsx +++ b/examples/operator-smoke-web/src/App.tsx @@ -22,7 +22,8 @@ function formatJson(value: unknown): string { function normalizeError(error: unknown): string { if (error instanceof GuardianOperatorHttpError) { const data = error.data; - const base = data?.error ?? error.message; + // Feature 009: the user-safe message moved from `error` to `message`. + const base = data?.message ?? error.message; // Surface the normalized code + paused-specific details so the // smoke makes the wire→client mapping visible (e.g. server emits // `GUARDIAN_ACCOUNT_PAUSED`, client surfaces `account_paused` diff --git a/packages/guardian-client/src/http.test.ts b/packages/guardian-client/src/http.test.ts index 13ed892b..1b3001db 100644 --- a/packages/guardian-client/src/http.test.ts +++ b/packages/guardian-client/src/http.test.ts @@ -782,6 +782,26 @@ describe('GuardianHttpError', () => { expect(error.name).toBe('GuardianHttpError'); }); + it('parses a { code, message, meta } body into structured accessors (feature 009)', () => { + const body = JSON.stringify({ + code: 'rate_limit_exceeded', + message: 'Too many requests — please try again shortly.', + meta: { retryable: true, retry_after_secs: 30 }, + }); + const error = new GuardianHttpError(429, 'Too Many Requests', body); + expect(error.code).toBe('rate_limit_exceeded'); + expect(error.userMessage).toBe('Too many requests — please try again shortly.'); + expect(error.meta?.retryable).toBe(true); + expect(error.meta?.retryAfterSecs).toBe(30); // snake_case → camelCase + }); + + it('leaves accessors undefined for a non-JSON / non-conforming body', () => { + const plain = new GuardianHttpError(502, 'Bad Gateway', 'upstream exploded'); + expect(plain.code).toBeUndefined(); + expect(plain.userMessage).toBeUndefined(); + expect(plain.meta).toBeUndefined(); + }); + describe('error envelope contract (account-paused path)', () => { let client: GuardianHttpClient; beforeEach(() => { @@ -792,15 +812,18 @@ describe('GuardianHttpError', () => { it('surfaces 409 GUARDIAN_ACCOUNT_PAUSED with a parseable error envelope on pushDeltaProposal', async () => { client.setSigner(mockSigner); - // The server's GuardianError::AccountPaused → IntoResponse contract. - // Locks client/server in lockstep: a regression to the legacy + // The server's GuardianError::AccountPaused → IntoResponse contract, + // reshaped to { code, message, meta } (feature 009). Locks client/server + // in lockstep: a regression to the legacy { success, error, paused_* } or // "(400, {delta: {account_id: 'error text'}})" shape would break this. const envelope = { - success: false, code: 'GUARDIAN_ACCOUNT_PAUSED', - error: 'Account is paused: compliance review', - paused_at: '2026-05-20T10:00:00Z', - paused_reason: 'compliance review', + message: "This account is paused and can't approve transactions right now.", + meta: { + retryable: false, + paused_at: '2026-05-20T10:00:00Z', + paused_reason: 'compliance review', + }, }; mockFetch.mockResolvedValueOnce({ @@ -821,26 +844,36 @@ describe('GuardianHttpError', () => { .catch((e) => e as GuardianHttpError); expect(error).toBeInstanceOf(GuardianHttpError); - expect((error as GuardianHttpError).status).toBe(409); - - const parsed = JSON.parse((error as GuardianHttpError).body); - expect(parsed.success).toBe(false); - expect(parsed.code).toBe('GUARDIAN_ACCOUNT_PAUSED'); - expect(typeof parsed.error).toBe('string'); - expect(parsed.paused_at).toBe('2026-05-20T10:00:00Z'); - expect(parsed.paused_reason).toBe('compliance review'); - // Negative assertion: legacy "stuff error into delta.account_id" shape. + const e = error as GuardianHttpError; + expect(e.status).toBe(409); + + // Structured accessors parsed from { code, message, meta } (feature 009). + expect(e.code).toBe('GUARDIAN_ACCOUNT_PAUSED'); + expect(typeof e.userMessage).toBe('string'); + expect(e.userMessage).not.toContain('compliance review'); // sanitized + expect(e.meta?.retryable).toBe(false); + expect(e.meta?.pausedAt).toBe('2026-05-20T10:00:00Z'); + expect(e.meta?.pausedReason).toBe('compliance review'); + + const parsed = JSON.parse(e.body); + // Legacy fields gone; not a domain object. + expect(parsed.success).toBeUndefined(); + expect(parsed.error).toBeUndefined(); expect(parsed.delta).toBeUndefined(); }); it('surfaces 401 AUTHENTICATION_FAILED with a parseable error envelope', async () => { client.setSigner(mockSigner); const envelope = { - success: false, code: 'authentication_failed', - error: 'Invalid signature', + message: 'Your session has expired. Please sign in again.', + meta: { retryable: false }, }; - mockFetch.mockResolvedValueOnce({ + // `authentication_failed` triggers the replay-retry path (the specific + // "Replay attack" detail is sanitized off the wire in feature 009, so we + // retry on the auth code). Use a persistent mock so the retries resolve + // and the final attempt throws the typed error. + mockFetch.mockResolvedValue({ ok: false, status: 401, statusText: 'Unauthorized', @@ -856,10 +889,12 @@ describe('GuardianHttpError', () => { .catch((e) => e as GuardianHttpError); expect(error).toBeInstanceOf(GuardianHttpError); - expect((error as GuardianHttpError).status).toBe(401); - const parsed = JSON.parse((error as GuardianHttpError).body); - expect(parsed.success).toBe(false); - expect(parsed.code).toBe('authentication_failed'); + const e = error as GuardianHttpError; + expect(e.status).toBe(401); + expect(e.code).toBe('authentication_failed'); + expect(typeof e.userMessage).toBe('string'); + const parsed = JSON.parse(e.body); + expect(parsed.success).toBeUndefined(); expect(parsed.delta).toBeUndefined(); }); }); diff --git a/packages/guardian-client/src/http.ts b/packages/guardian-client/src/http.ts index 9ca85d30..9735c077 100644 --- a/packages/guardian-client/src/http.ts +++ b/packages/guardian-client/src/http.ts @@ -38,9 +38,67 @@ import { } from './conversion.js'; /** - * Error thrown by the GUARDIAN HTTP client. + * Structured machine-readable side-data on a GUARDIAN error + * (feature `009-human-readable-errors`). `retryable` is always present. + */ +export interface GuardianErrorMeta { + retryable: boolean; + retryAfterSecs?: number; + missingPermissions?: string[]; + pausedAt?: string; + pausedReason?: string | null; +} + +interface ParsedGuardianError { + code: string; + message: string; + meta: GuardianErrorMeta; +} + +/** + * Parse a GUARDIAN error body `{ code, message, meta }` (feature 009), + * mapping the server's snake_case `meta` fields to camelCase. Returns + * `undefined` for non-JSON or non-conforming bodies. + */ +function parseGuardianErrorBody(body: string): ParsedGuardianError | undefined { + let json: unknown; + try { + json = JSON.parse(body); + } catch { + return undefined; + } + if (typeof json !== 'object' || json === null) return undefined; + const obj = json as Record; + if (typeof obj.code !== 'string' || typeof obj.message !== 'string') return undefined; + + const rawMeta = + typeof obj.meta === 'object' && obj.meta !== null ? (obj.meta as Record) : {}; + const meta: GuardianErrorMeta = { retryable: rawMeta.retryable === true }; + if (typeof rawMeta.retry_after_secs === 'number') meta.retryAfterSecs = rawMeta.retry_after_secs; + if (Array.isArray(rawMeta.missing_permissions)) { + meta.missingPermissions = rawMeta.missing_permissions.filter( + (x): x is string => typeof x === 'string' + ); + } + if (typeof rawMeta.paused_at === 'string') meta.pausedAt = rawMeta.paused_at; + if (typeof rawMeta.paused_reason === 'string' || rawMeta.paused_reason === null) { + meta.pausedReason = rawMeta.paused_reason as string | null; + } + return { code: obj.code, message: obj.message, meta }; +} + +/** + * Error thrown by the GUARDIAN HTTP client. Parses the `{ code, message, meta }` + * error body (feature 009): branch on {@link code}, display {@link userMessage}. */ export class GuardianHttpError extends Error { + /** Stable, machine-readable error code (e.g. `account_paused`). */ + readonly code?: string; + /** Short, user-safe message — safe to display verbatim in a wallet UI. */ + readonly userMessage?: string; + /** Structured side-data (`retryable`, `retryAfterSecs`, …). */ + readonly meta?: GuardianErrorMeta; + constructor( public readonly status: number, public readonly statusText: string, @@ -48,6 +106,10 @@ export class GuardianHttpError extends Error { ) { super(`GUARDIAN HTTP error ${status}: ${statusText} - ${body}`); this.name = 'GuardianHttpError'; + const parsed = parseGuardianErrorBody(body); + this.code = parsed?.code; + this.userMessage = parsed?.message; + this.meta = parsed?.meta; } } @@ -318,7 +380,16 @@ export class GuardianHttpClient { }, }); } catch (err) { - if (retries > 0 && err instanceof GuardianHttpError && err.body.includes('Replay attack')) { + // Replay rejections (stale timestamp) are transient: retry once with a + // fresh timestamp. The specific "Replay attack" detail is now sanitized + // off the wire (feature 009), so branch on the stable auth code instead. + // Retrying a genuine auth failure is harmless — one extra attempt that + // also fails — and only happens when a retry budget remains. + if ( + retries > 0 && + err instanceof GuardianHttpError && + err.code === 'authentication_failed' + ) { await new Promise((resolve) => setTimeout(resolve, 50)); return this.fetchAuthenticated(path, init, accountId, requestPayload, retries - 1); } diff --git a/packages/guardian-client/src/index.ts b/packages/guardian-client/src/index.ts index 83e14353..0d590af0 100644 --- a/packages/guardian-client/src/index.ts +++ b/packages/guardian-client/src/index.ts @@ -1,4 +1,5 @@ export { GuardianHttpClient, GuardianHttpError } from './http.js'; +export type { GuardianErrorMeta } from './http.js'; export { RequestAuthPayload } from './auth-request.js'; export type { diff --git a/packages/guardian-operator-client/src/http.test.ts b/packages/guardian-operator-client/src/http.test.ts index 14ea81b1..10dd206c 100644 --- a/packages/guardian-operator-client/src/http.test.ts +++ b/packages/guardian-operator-client/src/http.test.ts @@ -160,9 +160,8 @@ describe('GuardianOperatorHttpClient', () => { status: 400, statusText: 'Bad Request', body: { - success: false, code: 'invalid_limit', - error: 'limit must be at most 500, got 9999', + message: 'limit must be at most 500, got 9999', }, }), ); @@ -481,9 +480,8 @@ describe('GuardianOperatorHttpClient', () => { status: 429, statusText: 'Too Many Requests', body: { - success: false, - error: 'Rate limit exceeded', - retry_after_secs: 60, + message: 'Rate limit exceeded', + meta: { retry_after_secs: 60 }, }, })); @@ -493,8 +491,7 @@ describe('GuardianOperatorHttpClient', () => { expect(error).toBeInstanceOf(GuardianOperatorHttpError); expect(error.status).toBe(429); expect(error.data).toEqual({ - success: false, - error: 'Rate limit exceeded', + message: 'Rate limit exceeded', retryAfterSecs: 60, }); expect(error.retryAfterSecs).toBe(60); @@ -669,9 +666,8 @@ describe('GuardianOperatorHttpClient — per-account history', () => { status: 400, statusText: 'Bad Request', body: { - success: false, code: 'invalid_limit', - error: 'limit must be at most 500, got 9999', + message: 'limit must be at most 500, got 9999', }, }), ); @@ -694,9 +690,8 @@ describe('GuardianOperatorHttpClient — per-account history', () => { status: 404, statusText: 'Not Found', body: { - success: false, code: 'account_not_found', - error: "Account '0xunknown' not found", + message: "Account '0xunknown' not found", }, }), ); @@ -718,9 +713,8 @@ describe('GuardianOperatorHttpClient — per-account history', () => { status: 503, statusText: 'Service Unavailable', body: { - success: false, code: 'data_unavailable', - error: 'delta store unreadable', + message: 'delta store unreadable', }, }), ); @@ -1194,7 +1188,7 @@ describe('parseErrorBody', () => { const response = errorResponse({ status: 400, statusText: 'Bad Request', - body: { success: false, code, error: message }, + body: { code, message: message }, }); const parsed = await parseErrorBody(response as unknown as Response); expect(parsed.code).toBe(code); @@ -1210,7 +1204,7 @@ describe('parseErrorBody', () => { const response = errorResponse({ status: 429, statusText: 'Too Many Requests', - body: { success: false, code: 'rate_limit_exceeded', error: 'slow down' }, + body: { code: 'rate_limit_exceeded', message: 'slow down' }, }); const parsed = await parseErrorBody(response as unknown as Response); expect(parsed.code).toBe('rate_limit_exceeded'); @@ -1222,10 +1216,9 @@ describe('parseErrorBody', () => { status: 429, statusText: 'Too Many Requests', body: { - success: false, code: 'rate_limit_exceeded', - error: 'slow down', - retry_after_secs: 7, + message: 'slow down', + meta: { retry_after_secs: 7 }, }, }); const parsed = await parseErrorBody(response as unknown as Response); @@ -1258,9 +1251,8 @@ describe('parseErrorBody', () => { it('accepts a pre-parsed object body', async () => { const parsed = await parseErrorBody({ - success: false, code: 'invalid_cursor', - error: 'tampered', + message: 'tampered', }); expect(parsed.code).toBe('invalid_cursor'); expect(parsed.message).toBe('tampered'); @@ -1351,9 +1343,8 @@ describe('GuardianOperatorHttpClient — global feeds (US6, US7)', () => { status: 400, statusText: 'Bad Request', body: { - success: false, code: 'invalid_status_filter', - error: "unknown status value 'foo'", + message: "unknown status value 'foo'", }, }), ); @@ -1477,7 +1468,7 @@ describe('GuardianOperatorHttpClient — error matrix (FR-028 / SC-012)', () => errorResponse({ status, statusText: code, - body: { success: false, code, error: `${code} message` }, + body: { code, message: `${code} message` }, }), ); const client = new GuardianOperatorHttpClient('https://guardian.example'); @@ -1529,11 +1520,9 @@ describe('parseErrorBody (feature 006-operator-authz)', () => { status: 403, statusText: 'Forbidden', body: { - success: false, code: 'GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION', - error: 'Operator lacks required permissions: accounts:pause', - missing_permissions: ['accounts:pause'], - retryable: false, + message: 'Operator lacks required permissions: accounts:pause', + meta: { missing_permissions: ['accounts:pause'], retryable: false }, }, }); const parsed = await parseErrorBody(response as unknown as Response); @@ -1548,9 +1537,8 @@ describe('parseErrorBody (feature 006-operator-authz)', () => { status: 404, statusText: 'Not Found', body: { - success: false, code: 'account_not_found', - error: "Account 'x' not found", + message: "Account 'x' not found", }, }); const parsed = await parseErrorBody(response as unknown as Response); @@ -1566,11 +1554,9 @@ describe('parseErrorBody (feature 006-operator-authz)', () => { status: 403, statusText: 'Forbidden', body: { - success: false, code: 'GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION', - error: 'multiple missing', - missing_permissions: ['accounts:pause', 'policies:write'], - retryable: false, + message: 'multiple missing', + meta: { missing_permissions: ['accounts:pause', 'policies:write'], retryable: false }, }, }); const parsed = await parseErrorBody(response as unknown as Response); @@ -1588,16 +1574,17 @@ describe('parseErrorBody (feature 006-operator-authz)', () => { status: 404, statusText: 'Not Found', body: { - success: false, code: 'account_not_found', - error: 'unrelated', - missing_permissions: ['accounts:pause'], - retryable: false, + message: 'unrelated', + meta: { missing_permissions: ['accounts:pause'], retryable: false }, }, }); const parsed = await parseErrorBody(response as unknown as Response); + // `missingPermissions` stays gated on the permission code (not surfaced + // here), but `meta.retryable` is surfaced universally on the new wire + // shape (feature 009), so it is read through as-is. expect(parsed.missingPermissions).toBeUndefined(); - expect(parsed.retryable).toBeUndefined(); + expect(parsed.retryable).toBe(false); }); }); @@ -1698,11 +1685,9 @@ describe('GuardianOperatorHttpClient — account pausing (feature 001)', () => { status: 409, statusText: 'Conflict', body: { - success: false, code: 'GUARDIAN_ACCOUNT_PAUSED', - error: 'account is paused', - paused_at: '2026-05-20T10:00:00Z', - paused_reason: 'compliance review', + message: 'account is paused', + meta: { paused_at: '2026-05-20T10:00:00Z', paused_reason: 'compliance review' }, }, }), ); @@ -1858,12 +1843,9 @@ describe('GuardianOperatorHttpClient — account pause/unpause', () => { status: 409, statusText: 'Conflict', body: { - success: false, code: 'GUARDIAN_ACCOUNT_PAUSED', - error: 'account is paused', - paused_at: '2026-05-19T14:30:00Z', - paused_reason: 'compliance review', - retryable: false, + message: 'account is paused', + meta: { paused_at: '2026-05-19T14:30:00Z', paused_reason: 'compliance review', retryable: false }, }, }), ); @@ -1886,11 +1868,9 @@ describe('GuardianOperatorHttpClient — account pause/unpause', () => { status: 409, statusText: 'Conflict', body: { - success: false, code: 'GUARDIAN_ACCOUNT_PAUSED', - error: 'account is paused', - paused_at: '2026-05-19T14:30:00Z', - paused_reason: null, + message: 'account is paused', + meta: { paused_at: '2026-05-19T14:30:00Z', paused_reason: null }, }, }), ); @@ -1912,9 +1892,8 @@ describe('GuardianOperatorHttpClient — account pause/unpause', () => { status: 409, statusText: 'Conflict', body: { - success: false, code: 'GUARDIAN_ACCOUNT_PAUSED', - error: 'account is paused', + message: 'account is paused', }, }), ); diff --git a/packages/guardian-operator-client/src/http.ts b/packages/guardian-operator-client/src/http.ts index 7ff79c9a..a163bd2e 100644 --- a/packages/guardian-operator-client/src/http.ts +++ b/packages/guardian-operator-client/src/http.ts @@ -199,47 +199,54 @@ export async function parseErrorBody( typeof codeRaw === 'string' ? codeRaw : null, ); - const messageRaw = record['error']; + // Feature 009-human-readable-errors reshaped the body to + // `{ code, message, meta }`: the user-safe message moved from `error` to + // `message`, and the structured side-data (retry/permissions/pause) moved + // from top-level fields into `meta`. + const messageRaw = record['message']; const message = typeof messageRaw === 'string' ? messageRaw : null; - const retryRaw = record['retry_after_secs']; + const meta = + typeof record['meta'] === 'object' && record['meta'] !== null + ? (record['meta'] as Record) + : {}; + + const retryRaw = meta['retry_after_secs']; const retryAfterSecs = typeof retryRaw === 'number' && Number.isInteger(retryRaw) ? retryRaw : undefined; - // Feature 006-operator-authz FR-016: populate `missingPermissions` - // and `retryable` only when the server emitted the permission-denial - // code. Every other code path leaves both fields undefined so the - // additive envelope extension is invisible to existing parsers. The - // contract pins `retryable=false` for this code; surface that - // unconditionally so a non-compliant server can't mislead retry - // policy code into retrying a permission denial. + // `meta.retryable` is always present on the new wire shape; surface it + // whenever the server sent a boolean (the contract still pins `false` for + // permission denials and account-paused rejections). + const retryable = + typeof meta['retryable'] === 'boolean' ? (meta['retryable'] as boolean) : undefined; + + // `missingPermissions` / pause fields are populated only for the codes + // that carry them, now read out of `meta`. let missingPermissions: readonly string[] | undefined; - let retryable: boolean | undefined; let pausedAt: string | undefined; let pausedReason: string | null | undefined; if (code === 'insufficient_operator_permission') { - const missingRaw = record['missing_permissions']; + const missingRaw = meta['missing_permissions']; if ( Array.isArray(missingRaw) && missingRaw.every((v): v is string => typeof v === 'string') ) { missingPermissions = missingRaw as readonly string[]; } - retryable = false; } else if (code === 'account_paused') { - const pausedAtRaw = record['paused_at']; + const pausedAtRaw = meta['paused_at']; if (typeof pausedAtRaw === 'string') { pausedAt = pausedAtRaw; } - const reasonRaw = record['paused_reason']; + const reasonRaw = meta['paused_reason']; if (typeof reasonRaw === 'string') { pausedReason = reasonRaw; } else if (reasonRaw === null) { pausedReason = null; } - retryable = false; } return { @@ -273,7 +280,7 @@ export class GuardianOperatorHttpError extends Error { ) { super( `Guardian operator HTTP error ${status}: ${statusText}${ - data ? ` - ${data.error}` : body ? ` - ${body}` : '' + data ? ` - ${data.message}` : body ? ` - ${body}` : '' }`, ); this.name = 'GuardianOperatorHttpError'; @@ -1043,52 +1050,69 @@ function parseUnpauseResponse(value: unknown): UnpauseAccountResponse { function parseErrorResponse(value: unknown): GuardianOperatorHttpErrorData { const record = asRecord(value, 'error response'); - const success = requireBoolean(record, 'success', 'error response'); - if (success) { - throw new GuardianOperatorContractError( - 'error response', - 'expected success to be false', - ); + + // Feature 009-human-readable-errors: the body is `{ code, message, meta }`. + // `success` is gone (HTTP status is the discriminator), the user-safe + // message moved from `error` to `message`, and the structured side-data + // moved into `meta`. + const codeValue = record.code; + let code: string | undefined; + if (codeValue !== undefined) { + if (typeof codeValue !== 'string') { + throw new GuardianOperatorContractError( + 'error response', + 'code must be a string when present', + ); + } + code = mapDashboardErrorCode(codeValue) ?? undefined; } - const retryAfterValue = record.retry_after_secs; + const message = requireString(record, 'message', 'error response'); + + const metaRaw = record.meta; + let meta: Record = {}; + if (metaRaw !== undefined) { + if (typeof metaRaw !== 'object' || metaRaw === null || Array.isArray(metaRaw)) { + throw new GuardianOperatorContractError( + 'error response', + 'meta must be an object when present', + ); + } + meta = metaRaw as Record; + } + + const retryAfterValue = meta.retry_after_secs; let retryAfterSecs: number | undefined; if (retryAfterValue !== undefined) { if (typeof retryAfterValue !== 'number' || !Number.isInteger(retryAfterValue)) { throw new GuardianOperatorContractError( 'error response', - 'retry_after_secs must be an integer when present', + 'meta.retry_after_secs must be an integer when present', ); } retryAfterSecs = retryAfterValue; } - // Optional stable machine-readable error code added by feature - // `005-operator-dashboard-metrics` and required for the dashboard - // error taxonomy (FR-028). Older servers may omit it; tolerate that - // by leaving the field undefined. - const codeValue = record.code; - let code: string | undefined; - if (codeValue !== undefined) { - if (typeof codeValue !== 'string') { + // `meta.retryable` is always present on the new wire shape; surface it + // whenever it is a boolean (the server pins `false` for permission denials + // and account-paused rejections). + let retryable: boolean | undefined; + const retryableRaw = meta.retryable; + if (retryableRaw !== undefined) { + if (typeof retryableRaw !== 'boolean') { throw new GuardianOperatorContractError( 'error response', - 'code must be a string when present', + 'meta.retryable must be a boolean when present', ); } - code = mapDashboardErrorCode(codeValue) ?? undefined; + retryable = retryableRaw; } - // Feature 006-operator-authz FR-016: populate `missingPermissions` - // and `retryable` only on the permission-denial code. Tolerate - // either ordering or omission on every other code so legacy 5xx / - // 4xx errors continue to parse byte-for-byte as before. let missingPermissions: readonly string[] | undefined; - let retryable: boolean | undefined; let pausedAt: string | undefined; let pausedReason: string | null | undefined; if (code === 'insufficient_operator_permission') { - const missingRaw = record.missing_permissions; + const missingRaw = meta.missing_permissions; if (missingRaw !== undefined) { if ( !Array.isArray(missingRaw) || @@ -1096,56 +1120,34 @@ function parseErrorResponse(value: unknown): GuardianOperatorHttpErrorData { ) { throw new GuardianOperatorContractError( 'error response', - 'missing_permissions must be an array of strings', + 'meta.missing_permissions must be an array of strings', ); } missingPermissions = missingRaw as readonly string[]; } - const retryableRaw = record.retryable; - if (retryableRaw !== undefined) { - if (retryableRaw !== false) { - // FR-016 pins `retryable: false` for permission denials. - // Surface server contract drift loudly rather than letting - // retry policy code retry an unretryable failure. - throw new GuardianOperatorContractError( - 'error response', - 'retryable must be false for insufficient_operator_permission', - ); - } - retryable = false; - } } else if (code === 'account_paused') { - const pausedAtRaw = record.paused_at; + const pausedAtRaw = meta.paused_at; if (typeof pausedAtRaw !== 'string') { throw new GuardianOperatorContractError( 'error response', - 'paused_at must be a string for account_paused', + 'meta.paused_at must be a string for account_paused', ); } pausedAt = pausedAtRaw; - const reasonRaw = record.paused_reason; + const reasonRaw = meta.paused_reason; if (typeof reasonRaw === 'string' || reasonRaw === null) { pausedReason = reasonRaw; } else if (reasonRaw !== undefined) { throw new GuardianOperatorContractError( 'error response', - 'paused_reason must be a string or null for account_paused', + 'meta.paused_reason must be a string or null for account_paused', ); } - const retryableRaw = record.retryable; - if (retryableRaw !== undefined && retryableRaw !== false) { - throw new GuardianOperatorContractError( - 'error response', - 'retryable must be false for account_paused', - ); - } - retryable = false; } return { - success: false, code, - error: requireString(record, 'error', 'error response'), + message, retryAfterSecs, missingPermissions, retryable, diff --git a/packages/guardian-operator-client/src/server-types.ts b/packages/guardian-operator-client/src/server-types.ts index 21f705ef..fca9db09 100644 --- a/packages/guardian-operator-client/src/server-types.ts +++ b/packages/guardian-operator-client/src/server-types.ts @@ -83,16 +83,28 @@ export interface GuardianDashboardAccountResponse { account: GuardianDashboardAccountDetail; } -export interface GuardianErrorResponse { - success: boolean; - error: string; +/** Structured machine-readable side-data on the wire (feature 009). */ +export interface GuardianErrorMetaWire { + /** Always present. */ + retryable: boolean; retry_after_secs?: number; - code?: string; - retryable?: boolean; + /** Populated only for `GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION`. */ + missing_permissions?: string[]; /** Populated only for `GUARDIAN_ACCOUNT_PAUSED`. */ paused_at?: string; /** Populated only for `GUARDIAN_ACCOUNT_PAUSED`. */ - paused_reason?: string; - /** Populated only for `GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION`. */ - missing_permissions?: string[]; + paused_reason?: string | null; +} + +/** + * Wire shape of a Guardian error body (feature `009-human-readable-errors`): + * `{ code, message, meta }`. The legacy `success`/`error` fields are gone; + * the diagnostic detail is logged server-side only. + */ +export interface GuardianErrorResponse { + /** Stable machine-readable code; branch on this. */ + code?: string; + /** Short, user-safe message — safe to display verbatim. */ + message: string; + meta: GuardianErrorMetaWire; } diff --git a/packages/guardian-operator-client/src/types.ts b/packages/guardian-operator-client/src/types.ts index 4cd4fad2..aab59353 100644 --- a/packages/guardian-operator-client/src/types.ts +++ b/packages/guardian-operator-client/src/types.ts @@ -3,17 +3,24 @@ import type { OperatorPermission } from './permissions.js'; export type DashboardAccountStateStatus = 'available' | 'unavailable'; export interface GuardianOperatorHttpErrorData { - success: false; /** * Stable, machine-readable error code emitted by the server. Clients - * SHOULD branch on this rather than on `error` (the human message) or + * SHOULD branch on this rather than on `message` (the human text) or * the HTTP status alone. Codes added by feature * `005-operator-dashboard-metrics` are typed via {@link DashboardErrorCode}; * other codes (e.g. `account_not_found`, `authentication_failed`) are * forwarded as raw strings. */ code?: string; - error: string; + /** + * Short, user-safe message (feature `009-human-readable-errors`) — safe to + * display verbatim. Wording is not part of the stable contract; branch on + * `code`, not on this text. Replaces the former diagnostic `error` field, + * which is no longer on the wire (logged server-side only). Parsed out of + * the wire `{ code, message, meta }` object; the structured `meta` fields + * below are flattened here for ergonomics. + */ + message: string; retryAfterSecs?: number; /** * Feature 006-operator-authz FR-016 / FR-017: populated only for diff --git a/packages/miden-multisig-client/src/connectivity.test.ts b/packages/miden-multisig-client/src/connectivity.test.ts new file mode 100644 index 00000000..a03b9007 --- /dev/null +++ b/packages/miden-multisig-client/src/connectivity.test.ts @@ -0,0 +1,59 @@ +import { describe, it, expect } from 'vitest'; +import { GuardianHttpError } from '@openzeppelin/guardian-client'; +import { isLikelyNetworkError, toUserFacingError } from './connectivity.js'; + +describe('isLikelyNetworkError', () => { + it('flags codeless transport failures', () => { + for (const m of [ + 'Failed to fetch', + 'NetworkError when attempting to fetch resource', + 'Load failed', + 'The operation was aborted', + 'request timed out', + 'connection refused', + 'getaddrinfo ENOTFOUND guardian.example', + ]) { + expect(isLikelyNetworkError(new TypeError(m))).toBe(true); + } + }); + + it('does not flag semantic errors', () => { + expect(isLikelyNetworkError(new Error('account is paused'))).toBe(false); + expect(isLikelyNetworkError(new Error('insufficient signatures'))).toBe(false); + }); +}); + +describe('toUserFacingError', () => { + it('uses the server code + user-safe message when Guardian was reached', () => { + const body = JSON.stringify({ + code: 'account_paused', + message: "This account is paused and can't approve transactions right now.", + meta: { retryable: false }, + }); + const result = toUserFacingError(new GuardianHttpError(409, 'Conflict', body)); + expect(result.code).toBe('account_paused'); + expect(result.userMessage).toContain('paused'); + expect(result.category).toBeUndefined(); + }); + + it('classifies a codeless transport failure as connectivity', () => { + const result = toUserFacingError(new TypeError('Failed to fetch')); + expect(result.code).toBeUndefined(); + expect(result.category).toBe('unreachable'); + expect(result.userMessage).toContain("Can't reach Guardian"); + // The raw transport text is never the primary message. + expect(result.userMessage).not.toContain('Failed to fetch'); + }); + + it('treats a reachable proxy 5xx with no Guardian body as connectivity', () => { + const result = toUserFacingError(new GuardianHttpError(502, 'Bad Gateway', 'nope')); + expect(result.category).toBe('unreachable'); + expect(result.userMessage).toContain("Can't reach Guardian"); + }); + + it('falls back to a generic message for unknown non-Guardian errors', () => { + const result = toUserFacingError(new Error('totally unexpected')); + expect(result.userMessage).toBe('Something went wrong. Please try again.'); + expect(result.userMessage).not.toContain('totally unexpected'); + }); +}); diff --git a/packages/miden-multisig-client/src/connectivity.ts b/packages/miden-multisig-client/src/connectivity.ts new file mode 100644 index 00000000..a27c71d3 --- /dev/null +++ b/packages/miden-multisig-client/src/connectivity.ts @@ -0,0 +1,83 @@ +/** + * Classification for codeless transport failures (feature + * `009-human-readable-errors`, User Story 3). + * + * When Guardian *is* reached, errors arrive as a {@link GuardianHttpError} + * carrying the server's stable `code` and user-safe `message` — use those. + * When Guardian is *not* reached (connection refused, DNS, timeout, TLS, or a + * proxy 5xx with no Guardian body) no error object exists, so the wallet-facing + * client must classify the failure and supply a friendly connectivity message + * rather than surfacing raw `"Failed to fetch"` / `"NetworkError"` text. + * + * The detection is intentionally string-matching, mirroring the 0xMiden/wallet + * `connectivity-classify.ts` heuristic: fetch / DOMException / TypeError each + * surface failures with different shapes, so the message string is the only + * stable join key. + */ +import { GuardianHttpError } from '@openzeppelin/guardian-client'; + +/** Connectivity category for a codeless transport failure. */ +export type ConnectivityCategory = 'network' | 'unreachable' | 'timeout'; + +const CONNECTIVITY_MESSAGE = "Can't reach Guardian right now. Check your connection and try again."; +const GENERIC_MESSAGE = 'Something went wrong. Please try again.'; + +/** + * Does this error look like a codeless transport/connectivity failure (vs a + * semantic Guardian error)? String heuristic — see module docs. + */ +export function isLikelyNetworkError(err: unknown): boolean { + const message = (err as { message?: string } | null | undefined)?.message ?? String(err ?? ''); + const lower = message.toLowerCase(); + if (lower.includes('failed to fetch')) return true; + if (lower.includes('networkerror')) return true; + if (lower.includes('network error')) return true; + if (lower.includes('load failed')) return true; // Safari fetch failure + if (lower.includes('abort')) return true; // includes "aborted" + if (lower.includes('timeout') || lower.includes('timed out')) return true; + if (lower.includes('connection')) return true; + if (lower.includes('econnrefused') || lower.includes('enotfound')) return true; + if (lower.includes('dns')) return true; + return false; +} + +/** A normalized, wallet-displayable view of any error thrown by a Guardian call. */ +export interface UserFacingError { + /** Stable Guardian error code, when the server was reached. */ + code?: string; + /** Connectivity category, when this was a codeless transport failure. */ + category?: ConnectivityCategory; + /** Short message safe to display verbatim in a wallet UI. */ + userMessage: string; + /** The original error, for logging/diagnostics. */ + cause: unknown; +} + +/** + * Normalize any error thrown by a Guardian call into a user-facing shape. + * + * - A {@link GuardianHttpError} with a parsed `{ code, message }` → the + * server's stable code + user-safe message (feature 009). + * - A reachable host that returned no Guardian error object (e.g. a proxy 5xx + * with an HTML body) → treated as a connectivity failure. + * - A codeless transport rejection (server never reached) → classified and + * given the generic connectivity message; the raw transport text is never + * surfaced as the primary message. + */ +export function toUserFacingError(err: unknown): UserFacingError { + if (err instanceof GuardianHttpError) { + if (err.code && err.userMessage) { + return { code: err.code, userMessage: err.userMessage, cause: err }; + } + // Reached a host, but no Guardian error object (unparseable body). A 5xx + // here is an upstream/proxy connectivity problem, not a user-actionable one. + if (err.status >= 500) { + return { category: 'unreachable', userMessage: CONNECTIVITY_MESSAGE, cause: err }; + } + return { userMessage: GENERIC_MESSAGE, cause: err }; + } + if (isLikelyNetworkError(err)) { + return { category: 'unreachable', userMessage: CONNECTIVITY_MESSAGE, cause: err }; + } + return { userMessage: GENERIC_MESSAGE, cause: err }; +} diff --git a/packages/miden-multisig-client/src/index.ts b/packages/miden-multisig-client/src/index.ts index 20328ab0..0bbff88d 100644 --- a/packages/miden-multisig-client/src/index.ts +++ b/packages/miden-multisig-client/src/index.ts @@ -57,6 +57,11 @@ export { } from './transaction.js'; export { GuardianHttpClient, GuardianHttpError } from '@openzeppelin/guardian-client'; +export type { GuardianErrorMeta } from '@openzeppelin/guardian-client'; + +// Codeless transport-failure classification (feature 009, User Story 3). +export { isLikelyNetworkError, toUserFacingError } from './connectivity.js'; +export type { ConnectivityCategory, UserFacingError } from './connectivity.js'; export { FalconSigner, diff --git a/speckit/features/009-human-readable-errors/plan.md b/speckit/features/009-human-readable-errors/plan.md new file mode 100644 index 00000000..23a114cc --- /dev/null +++ b/speckit/features/009-human-readable-errors/plan.md @@ -0,0 +1,45 @@ +# Implementation Plan: Human-readable error messages for wallet users + +**Feature**: `009-human-readable-errors` · **Spec**: [spec.md](./spec.md) · **Branch**: `009-human-readable-errors-impl` + +Implements the error contract reshaped in PR [#292](https://github.com/OpenZeppelin/guardian/pull/292) review by @zeljkoX: a single `{ code, message, meta }` object, identical on the HTTP body and the gRPC `Status.details`; the diagnostic `Display` string is logged server-side only; gRPC errors ride on `tonic::Status`. + +## Wire contract (target) + +```json +{ "code": "authorization_failed", "message": "You're not an authorized signer for this account.", "meta": { "retryable": false } } +``` + +- `code` — stable machine code (existing vocabulary, unchanged). Branch + i18n key. +- `message` — short, user-safe sentence; always present; safe to display. Wording not stable. +- `meta` — `retryable` (always present) + `retry_after_secs` / `missing_permissions` / `paused_at` / `paused_reason` when they apply. +- **HTTP**: response body is this object. No `success`, no `error`. +- **gRPC**: `Status.code` unchanged; `Status.message` = `message`; `Status.details` = this object (JSON), for every error. + +## Layer-by-layer (bottom-up, Constitution I) + +1. **Server core** (`crates/server/src/error.rs`): `GuardianError::user_message()` (sanitized per-variant, connectivity-vs-internal split per FR-004) + `retryable()`; `ErrorBody`/`ErrorMeta` structs; `IntoResponse` logs `Display` then emits the object; `From for tonic::Status` attaches `{code,message,meta}` details for all variants with `Status.message = user_message`. +2. **Server edges**: `middleware/rate_limit.rs` reuses the canonical envelope; `api/http.rs` `configure` returns the user-safe message + code (in-band shape kept — *deferred follow-up*); dead `ErrorResponse` removed. +3. **gRPC handlers** (`api/grpc.rs`): all 9 service error arms return `Err(Status::from(e))` (was in-band `success=false`). +4. **OpenAPI**: `openapi.rs` `ApiErrorResponse` + `ApiErrorMeta`; regenerated `docs/openapi*.json`. +5. **Rust clients**: `crates/client` `ClientError::{guardian_code,user_message,guardian_meta,is_not_found}` parsed from `Status.details`; `crates/miden-multisig-client` `get_deltas` branches on `is_not_found()`. +6. **TS guardian-client** (wallet HTTP path): `GuardianHttpError` parses `{code,message,meta}` (snake→camel); replay-retry keyed on `authentication_failed` code. +7. **TS miden-multisig-client**: `toUserFacingError()` / `isLikelyNetworkError()` connectivity classifier (US3); re-exports the error types. +8. **TS guardian-operator-client**: `parseErrorBody`/`parseErrorResponse` read `message` + `meta.*`; types reshaped. +9. **Examples**: `operator-smoke-web` reads `data.message`. + +## Validation + +- Rust: `cargo check --workspace --all-targets` clean; `error::tests` (45), `error_envelope_http` (8), `dashboard` (73), `*_grpc` integration, `guardian-client` (41), `miden-multisig-client` (110). +- TS: `guardian-client` vitest (49), `guardian-operator-client` vitest (83), `miden-multisig-client` `connectivity` (6). Cross-package TS requires the local `guardian-client` linked/built (packages depend on published versions); CI uses the linked-PR mechanism. The full multisig vitest suite needs the miden-sdk WASM setup. + +## Deferred follow-ups (out of this change, noted in commits) + +- Strip the now-vestigial `success`/`error_code` from the gRPC **success** response messages (proto + handlers + Rust-client success parsing) and migrate the HTTP `configure` error path onto the `{code,message,meta}` envelope — completes a strict reading of Zeljko's "drop success / one object everywhere." +- Pre-service `Status::invalid_argument` auth-metadata guards (`api/grpc.rs`, `metadata/auth/credentials.rs`) still return developer-facing strings; route them through `GuardianError` for user-safe messages. + +## Constitution check + +- I (bottom-up): server → proto/OpenAPI → Rust clients → TS clients → examples all updated. +- II (parity): HTTP and gRPC carry the identical object; Rust + TS clients expose the same `code`/`message`/`meta`. +- IV (stable boundary errors): `code` vocabulary + HTTP/gRPC status mappings unchanged; diagnostic detail removed from the wire (disclosure fix by construction). diff --git a/speckit/features/009-human-readable-errors/tasks.md b/speckit/features/009-human-readable-errors/tasks.md new file mode 100644 index 00000000..59266e97 --- /dev/null +++ b/speckit/features/009-human-readable-errors/tasks.md @@ -0,0 +1,26 @@ +# Tasks: Human-readable error messages for wallet users + +Feature `009-human-readable-errors`. Status as of the implementation branch +`009-human-readable-errors-impl`. See [plan.md](./plan.md). + +## Done + +- [x] **T001** Server: `GuardianError::user_message()` (sanitized, connectivity-vs-internal split) + `retryable()`. *(FR-001..FR-004)* +- [x] **T002** Server: reshape error wire object to `{ code, message, meta }`; `Display` logged not returned; drop `success`/`error`. *(FR-005, FR-007)* +- [x] **T003** Server: generalize `From for tonic::Status` — `Status.message` = user message, `Status.details` = the object for every variant. *(FR-006, FR-008, FR-009)* +- [x] **T004** Server edges: rate-limit middleware + `configure` use the user-safe message; dead `ErrorResponse` removed. +- [x] **T005** gRPC handlers return `Err(Status::from(e))` (was in-band `success=false`). *(FR-006)* +- [x] **T006** OpenAPI `ApiErrorResponse`/`ApiErrorMeta` + regenerate `docs/openapi*.json`. *(FR-010)* +- [x] **T007** Rust `crates/client`: `guardian_code()`/`user_message()`/`guardian_meta()`/`is_not_found()`. *(FR-011)* +- [x] **T008** `crates/miden-multisig-client`: `get_deltas` branches on `is_not_found()` (not message text). *(FR-012, FR-014)* +- [x] **T009** TS `guardian-client`: `GuardianHttpError` parses `{ code, message, meta }`; replay-retry on `authentication_failed`. *(FR-010, FR-013)* +- [x] **T010** TS `miden-multisig-client`: `toUserFacingError()`/`isLikelyNetworkError()` connectivity classifier. *(FR-011, FR-013, US3)* +- [x] **T011** TS `guardian-operator-client`: parsers + types reshaped. *(FR-010)* +- [x] **T012** Examples: `operator-smoke-web` reads `data.message`. *(FR-014)* +- [x] **T013** Tests: SC-001 (every variant non-empty), SC-002 (sanitization + no `success`/`error`), SC-005 (connectivity vs internal split), parity + client/connectivity tests. *(SC-001..SC-007)* + +## Deferred follow-ups (separate change) + +- [ ] **T014** Strip vestigial `success`/`error_code` from gRPC success response messages (proto + handlers + Rust-client success parsing); migrate HTTP `configure` error path onto the envelope. *(strict "drop success" everywhere)* +- [ ] **T015** Route pre-service `Status::invalid_argument` auth-metadata guards through `GuardianError` for user-safe messages. +- [ ] **T016** CI: ensure cross-package TS builds link the in-repo `guardian-client` (the packages otherwise resolve published versions).