From cc088d7e70cc4ec37d6974896569db7f43e622dd Mon Sep 17 00:00:00 2001 From: YellowSnnowmann <167776381+YellowSnnowmann@users.noreply.github.com> Date: Fri, 29 May 2026 04:04:48 +0530 Subject: [PATCH] fix(providers): include body snippet on list_models JSON parse failure (#2838) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- src/openhuman/inference/provider/ops.rs | 135 +++++++++++++++++++++++- 1 file changed, 131 insertions(+), 4 deletions(-) diff --git a/src/openhuman/inference/provider/ops.rs b/src/openhuman/inference/provider/ops.rs index 2f3929a82..177b5735b 100644 --- a/src/openhuman/inference/provider/ops.rs +++ b/src/openhuman/inference/provider/ops.rs @@ -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) -> 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 = "Sign incaptive portal"; + 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