From 6dbbeefeab1119ee2e0e7cfa1a89a5db5a1ced3d Mon Sep 17 00:00:00 2001
From: Eddie A Tejeda <669988+eddietejeda@users.noreply.github.com>
Date: Fri, 18 Sep 2026 14:59:31 -0700
Subject: [PATCH] fix(client): render the error envelope's sentence in inline
 warnings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`ApiError::message` formatted the raw response body, so every warning
that folded an API error inline printed the JSON envelope instead of
the sentence inside it. `create --attach` showed it as

    warning: could not attach 'src': 409 Conflict: {"error":{"message":
    "this catalog is scoped to another database…","code":"CONFLICT"}}

and the query preview fallback did the same for a failed full-result
fetch ("could not fetch full result (404 Not Found: {"error":…})").

Fix it where it lives: `message` now runs the body through the same
unwrapping the fatal path already does, via a shared
`server_explanation`, so the inline and the fatal rendering of one
response cannot drift. The status stays in the line so a non-JSON body
still says what happened. Every `message` caller gets the fix at once;
nothing has to remember to pick a second accessor.
---
 src/client/sdk.rs     | 75 +++++++++++++++++++++++++++++++++++++------
 src/commands/query.rs |  8 ++++-
 2 files changed, 73 insertions(+), 10 deletions(-)

diff --git a/src/client/sdk.rs b/src/client/sdk.rs
index b7b82ef..6ec2544 100644
--- a/src/client/sdk.rs
+++ b/src/client/sdk.rs
@@ -276,11 +276,19 @@ impl ApiError {
 
     /// A printable, single-line description of the failure.
     ///
-    /// Used where the error is surfaced inline (e.g. folded into a query
-    /// `warning`) rather than printed-and-exited via [`exit`](Self::exit).
+    /// Used where the error is surfaced inline (folded into a query `warning`,
+    /// the `create --attach` warning) rather than printed-and-exited via
+    /// [`exit`](Self::exit). Carries the status so a non-JSON body still says
+    /// what happened, and runs the body through [`server_explanation`] so an
+    /// error envelope renders as the sentence inside it, never as raw JSON.
+    /// The exit path ([`format_fail_message`]) shares that base and adds the
+    /// credential-probe hints on top, which need a live `Api` this does not
+    /// have.
     pub fn message(&self) -> String {
         match self {
-            ApiError::Status { status, body } => format!("{status}: {body}"),
+            ApiError::Status { status, body } => {
+                format!("{status}: {}", server_explanation(*status, body))
+            }
             ApiError::Transport(msg) => msg.clone(),
         }
     }
@@ -1040,6 +1048,21 @@ impl Api {
     }
 }
 
+/// What the server said, as one line a person can read.
+///
+/// An error envelope (`{"error":{"message":…}}`, and the other shapes
+/// `util::api_error` knows) renders as the sentence inside it; a bodyless
+/// response says so instead of printing a blank. This is the one place that
+/// unwrapping lives — both `ApiError::message` (inline) and
+/// `format_fail_message` (fatal) build on it.
+fn server_explanation(status: reqwest::StatusCode, body: &str) -> String {
+    if body.trim().is_empty() {
+        format!("HTTP {status} (empty response body)")
+    } else {
+        util::api_error(body.to_string())
+    }
+}
+
 /// Decide what error text to print for a failed response. Pure function so the
 /// re-auth-hint heuristic is unit-testable without HTTP or `exit`.
 ///
@@ -1055,12 +1078,13 @@ pub fn format_fail_message(
     body: &str,
     auth_status: Option<&credentials::AuthStatus>,
 ) -> String {
-    // Base: the server's own explanation, or the status line when there is none.
-    let mut msg = if body.trim().is_empty() {
-        format!("error: HTTP {status} (empty response body)")
-    } else {
-        util::api_error(body.to_string())
-    };
+    // Base: the server's own explanation, or the status line when there is
+    // none. Shared with `ApiError::message` so the inline and the fatal
+    // rendering of one response never drift apart.
+    let mut msg = server_explanation(status, body);
+    if body.trim().is_empty() {
+        msg = format!("error: {msg}");
+    }
     // A 403 ACCESS_DENIED is the allow-list guard rejecting an operation the
     // credential can't perform — typically a database API token (which is
     // limited to create/query/upload) hitting a workspace-level endpoint. Keep
@@ -1089,6 +1113,39 @@ mod tests {
     use super::*;
     use credentials::AuthStatus;
 
+    /// An inline warning is read by a person, so the envelope must be gone.
+    /// `create --attach` used to print `409 Conflict: {"error":{"message":…}}`.
+    #[test]
+    fn api_error_message_unwraps_the_error_envelope() {
+        let err = ApiError::Status {
+            status: reqwest::StatusCode::CONFLICT,
+            body: r#"{"error":{"message":"this catalog is scoped to another database and cannot be attached here","code":"CONFLICT"}}"#.to_string(),
+        };
+        assert_eq!(
+            err.message(),
+            "409 Conflict: this catalog is scoped to another database and cannot be attached here"
+        );
+    }
+
+    /// A bodyless response names the status instead of printing a blank, and
+    /// says the same thing the fatal path says (minus its `error:` prefix).
+    #[test]
+    fn api_error_message_and_fail_message_agree_on_an_empty_body() {
+        let status = reqwest::StatusCode::BAD_GATEWAY;
+        let err = ApiError::Status {
+            status,
+            body: String::new(),
+        };
+        assert_eq!(
+            err.message(),
+            "502 Bad Gateway: HTTP 502 Bad Gateway (empty response body)"
+        );
+        assert_eq!(
+            format_fail_message(status, "", None),
+            "error: HTTP 502 Bad Gateway (empty response body)"
+        );
+    }
+
     #[test]
     fn api_error_message_formats_status_and_transport() {
         let status = ApiError::Status {
diff --git a/src/commands/query.rs b/src/commands/query.rs
index e809645..903faa2 100644
--- a/src/commands/query.rs
+++ b/src/commands/query.rs
@@ -1708,7 +1708,9 @@ mod tests {
                 "arrow".into(),
             ))
             .with_status(500)
-            .with_body("boom")
+            // An error envelope, the shape the server actually sends: the
+            // warning must carry the sentence inside it, not the JSON.
+            .with_body(r#"{"error":{"message":"result res_1 has expired","code":"NOT_FOUND"}}"#)
             .create();
 
         let api = Api::test_new_scoped(&server.url(), "test-jwt", Some("ws-1"), Some("db-1"));
@@ -1738,6 +1740,10 @@ mod tests {
         assert_eq!(resolved.total_row_count, None);
         let warning = resolved.warning.as_deref().unwrap_or("");
         assert!(warning.contains("truncated"), "warning: {warning:?}");
+        assert!(
+            warning.contains("result res_1 has expired") && !warning.contains("{\"error\""),
+            "the warning should quote the server's sentence, not its envelope: {warning}"
+        );
         assert!(
             warning.contains("could not fetch full result"),
             "warning: {warning:?}"