fix(providers): include body snippet on list_models JSON parse failure (#2838)

## Summary - Replace `response.json()` with read-as-text + `serde_json::from_str` in `list_configured_models_from_config` so the response body is preserved when JSON decoding fails. - Append a sanitized + truncated body snippet to the `[providers][list_models] failed to parse JSON` error so the failure is diagnosable from the log/Sentry line alone. - Add three async unit tests: HTML body returns a diagnostic snippet, empty body still surfaces the parse error, and a valid `/models` response still lists models (regression guard for the new text-then-parse path). ## Problem Sentry issue **TAURI-RUST-12** — `[providers][list_models] failed to parse JSON: error decoding response body` — 376 events / 14d on the `tauri-rust` project. `response.json()` in [src/openhuman/inference/provider/ops.rs:125](src/openhuman/inference/provider/ops.rs:125) (pre-change) consumes the body in the process of decoding it. When the decode fails — typically because the server returned HTML from a captive portal / corporate proxy login page, an upstream load-balancer 502 served as HTML with `200 OK`, or a wrong-path endpoint returning a non-JSON response — the body is gone by the time we format the error, so Sentry receives `error decoding response body` with no payload context. We can't fix this server-side. We *can* stop discarding the diagnostic information at the call site so users and devs can identify the real cause from the error string instead of guessing. ## Solution `src/openhuman/inference/provider/ops.rs`: - After the `status.is_success()` check, call `response.text().await` instead of `response.json()`. The text path returns the raw body verbatim, which we can then both parse *and* embed in the diagnostic message. - `serde_json::from_str(&raw_body)` reproduces the previous decode behaviour exactly — same JSON parser, same `serde_json::Error` shape. On failure, the closure sanitizes the body via the existing `sanitize_api_error` helper and truncates it through the existing `crate::openhuman::util::truncate_with_ellipsis(_, 300)` before appending it as `(body: …)`. - Adds an explicit error for `response.text()` failure (`failed to read response body`) — a transport-layer concern distinct from JSON parsing. **Design choices** - Re-use the existing `sanitize_api_error` (strips ANSI / control chars, caps at `MAX_API_ERROR_CHARS`) and `truncate_with_ellipsis` helpers — same sanitization the non-2xx branch already applies a few lines above. No new redaction policy. - Keep the canonical error prefix `[providers][list_models] failed to parse JSON:` so any existing log greps / Sentry classifiers continue to match. - 300-character snippet cap matches the existing non-2xx branch's `truncated` cap and the codebase convention for "include enough for triage, not enough to flood logs." - No change to the JSON parser, no change to what counts as a valid `/models` response, and no change to error semantics — the new branch returns `Err(...)` in exactly the same shape and code path as before. Callers see one extra clause appended to the message string. - Body is only read on the success path (`status.is_success()`). The non-2xx branch already had its own `response.text()` + sanitize chain, untouched. ## Submission Checklist - Tests added or updated (happy path + at least one failure / edge case) per [Testing Strategy](../gitbooks/developing/testing-strategy.md#failure-path-requirement) - **Diff coverage ≥ 80%** — pending local `pnpm test:rust` run - Coverage matrix updated — `N/A: diagnostic-only change, no new feature row` - All affected feature IDs from the matrix are listed in the PR description under `## Related` - No new external network dependencies introduced (uses existing axum-based mock pattern from `spawn_openrouter_probe_server`) - Manual smoke checklist updated — `N/A: no release-cut surface touched` - Linked issue closed via `Closes #NNN` — `N/A: Sentry-tracked issue, no GitHub issue yet` ## Impact - **Runtime**: desktop (Rust core). No mobile / web / CLI surface change. - **Performance**: negligible — one extra `String` allocation for the body (length already bounded by reqwest's response size limits) and one extra `serde_json::from_str` instead of `response.json()`'s internal equivalent. Happy path serialization cost is identical. - **Security**: no new surface. Body is sanitized via the same helper the non-2xx branch already trusts; `truncate_with_ellipsis(_, 300)` caps the leak window. No PII redaction policy changes. - **Migration / compatibility**: none. RPC schema, return type, error-string prefix all preserved. Callers that previously matched on `"failed to parse JSON"` still match — only a `(body: …)` suffix is added. ## Related - Closes: Sentry [TAURI-RUST-12](https://sentry.tinyhumans.ai/organizations/tinyhumans/issues/100/?project=4&referrer=issue-list&statsPeriod=14d)  ## Summary by CodeRabbit * **Bug Fixes** * Improved model-listing response parsing and diagnostics: parsing now uses the raw response text and, on failure, error messages include a sanitized, truncated snippet of the body to aid troubleshooting. Non-2xx handling and subsequent response validation remain unchanged. * **Tests** * Added tests covering HTML responses, empty bodies, and valid model-listing payloads.  [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2838?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)   Co-authored-by: M3gA-Mind <megamind@mahadao.com>
2026-07-27 21:08:00 +00:00 · 2026-05-29 04:04:48 +05:30
parent 32dd0be19a
commit cc088d7e70
1 changed files with 131 additions and 4 deletions
@@ -122,10 +122,28 @@ async fn list_configured_models_from_config(
        ));
    }

-    let body: serde_json::Value = response
-        .json()
-        .await
-        .map_err(|e| format!("[providers][list_models] failed to parse JSON: {}", e))?;
+    // TAURI-RUST-12: `response.json()` discards the body when decoding fails,
+    // so Sentry just sees `error decoding response body` with no clue what the
+    // server actually sent. In practice the offending body is HTML from a
+    // captive portal / corporate proxy login page, an upstream load-balancer
+    // 502 served as HTML with a `200 OK`, or a JSON parser tripping on a
+    // wrong-path endpoint. Read the body as text first, then parse, and
+    // surface a sanitized + truncated snippet so the failure is diagnosable
+    // from the error string alone.
+    let raw_body = response.text().await.map_err(|e| {
+        format!(
+            "[providers][list_models] failed to read response body: {}",
+            e
+        )
+    })?;
+    let body: serde_json::Value = serde_json::from_str(&raw_body).map_err(|e| {
+        let sanitized = sanitize_api_error(&raw_body);
+        let snippet = crate::openhuman::util::truncate_with_ellipsis(&sanitized, 300);
+        format!(
+            "[providers][list_models] failed to parse JSON: {} (body: {})",
+            e, snippet
+        )
+    })?;

    // OpenAI-compatible servers occasionally return HTTP 200 with an error
    // payload instead of a 4xx (LM Studio does this for unknown paths like
@@ -1755,6 +1773,115 @@ mod tests {
        assert_eq!(crabs_count, MAX_API_ERROR_CHARS);
    }

+    // ── TAURI-RUST-12: list_models JSON parse error must surface body ──────
+    //
+    // `response.json()` previously dropped the body when decoding failed, so
+    // Sentry saw `[providers][list_models] failed to parse JSON: error decoding
+    // response body` with no clue what the server actually returned. The fix
+    // reads the body as text first, parses with `serde_json::from_str`, and
+    // appends a sanitized + truncated snippet to the error string so the
+    // failure is diagnosable from the log line alone.
+
+    #[derive(Clone)]
+    struct StaticResponse {
+        status: StatusCode,
+        body: &'static str,
+    }
+
+    async fn static_models_handler(State(s): State<StaticResponse>) -> Response {
+        (s.status, s.body).into_response()
+    }
+
+    async fn spawn_static_models_server(status: StatusCode, body: &'static str) -> String {
+        let listener = tokio::net::TcpListener::bind("127.0.0.1:0")
+            .await
+            .expect("bind");
+        let addr = listener.local_addr().expect("local_addr");
+        let app = Router::new()
+            .route("/models", get(static_models_handler))
+            .with_state(StaticResponse { status, body });
+        tokio::spawn(async move {
+            axum::serve(listener, app).await.expect("serve");
+        });
+        format!("http://{addr}")
+    }
+
+    async fn configure_generic_workspace(tmp: &TempDir, endpoint: String) -> Config {
+        // Non-`openrouter` slug so the OpenRouter pre-validation path is
+        // skipped and the test hits `/models` directly.
+        let mut config = Config {
+            config_path: tmp.path().join("config.toml"),
+            workspace_dir: tmp.path().join("workspace"),
+            ..Config::default()
+        };
+        config.secrets.encrypt = false;
+        config.cloud_providers.push(CloudProviderCreds {
+            id: "p_generic_test".to_string(),
+            slug: "generic-test".to_string(),
+            label: "Generic".to_string(),
+            endpoint,
+            auth_style: AuthStyle::None,
+            legacy_type: None,
+            default_model: None,
+        });
+        config.save().await.expect("save config");
+        config
+    }
+
+    #[tokio::test]
+    async fn list_models_html_body_returns_diagnostic_snippet() {
+        // Captive-portal / proxy-login wire shape: 200 OK with HTML.
+        let tmp = tempfile::tempdir().expect("tempdir");
+        let html = "<html><head><title>Sign in</title></head><body>captive portal</body></html>";
+        let endpoint = spawn_static_models_server(StatusCode::OK, html).await;
+        let config = configure_generic_workspace(&tmp, endpoint).await;
+
+        let err = list_configured_models_from_config("generic-test", &config)
+            .await
+            .expect_err("HTML body must not parse as JSON");
+
+        assert!(
+            err.contains("failed to parse JSON"),
+            "error must keep canonical prefix: {err}"
+        );
+        assert!(
+            err.contains("captive portal") || err.contains("Sign in") || err.contains("html"),
+            "error must include a body snippet for diagnosis: {err}"
+        );
+    }
+
+    #[tokio::test]
+    async fn list_models_empty_body_returns_diagnostic_error() {
+        // Some misconfigured load balancers return 200 with an empty body.
+        let tmp = tempfile::tempdir().expect("tempdir");
+        let endpoint = spawn_static_models_server(StatusCode::OK, "").await;
+        let config = configure_generic_workspace(&tmp, endpoint).await;
+
+        let err = list_configured_models_from_config("generic-test", &config)
+            .await
+            .expect_err("empty body must not parse as JSON");
+
+        assert!(
+            err.contains("failed to parse JSON"),
+            "error must keep canonical prefix: {err}"
+        );
+    }
+
+    #[tokio::test]
+    async fn list_models_valid_json_still_succeeds() {
+        // Regression guard: the new text-then-parse path must still accept
+        // a valid `/models` JSON response.
+        let tmp = tempfile::tempdir().expect("tempdir");
+        let body = r#"{"data":[{"id":"some-model","owned_by":"vendor","context_length":4096}]}"#;
+        let endpoint = spawn_static_models_server(StatusCode::OK, body).await;
+        let config = configure_generic_workspace(&tmp, endpoint).await;
+
+        let outcome = list_configured_models_from_config("generic-test", &config)
+            .await
+            .expect("valid JSON must list models");
+        assert_eq!(outcome.value["models"][0]["id"], "some-model");
+    }
+
    // ── parse_models_response (TAURI-RUST-4Y) ──────────────────────────────
    //
    // Before this fix the `/models` parser collapsed "no `data` field" and