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

[YellowSnnowmann]

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 (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
• 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

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.

[coderabbitai]

📝 Walkthrough

Walkthrough

list_configured_models_from_config now reads the /models response body as text and parses with serde_json::from_str, returning parse errors that include a sanitized, truncated snippet of the raw body. Three async tests verify HTML, empty, and valid JSON responses.

Changes

Error Diagnostics for Model List Fetch

Layer / File(s)	Summary
Read body-as-text, parse JSON, and tests src/openhuman/inference/provider/ops.rs	Switches from response.json() to response.text() + serde_json::from_str; JSON parse failures include a sanitized/truncated snippet of the raw response body. Adds three async tests serving static /models responses (HTML 200, empty 200, valid JSON 200) to validate failure diagnostics and success behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• tinyhumansai/openhuman#2794: Modifies the same /models parsing path in list_configured_models_from_config and adds related tests.

Suggested labels

bug

Suggested reviewers

• graycyrus
• oxoxDev
• senamakel

Poem

🐰 I nibble logs and sniff the trace,

Raw body snippets show the place.
No more guesses in the night,
Diagnostics shine the debugging light.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: improving error diagnostics for JSON parsing failures in list_models by including body snippets.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

[oxoxDev]

Walkthrough

Diagnostic-only fix on list_configured_models_from_config (src/openhuman/inference/provider/ops.rs). Pre-change response.json() consumed the body during decode, so Sentry TAURI-RUST-12 (376 events / 14d) only saw error decoding response body with no payload context. Fix reads the body as text first, parses via serde_json::from_str, and appends a sanitized + truncated snippet to the error string. Same sanitize_api_error + truncate_with_ellipsis(_, 300) chain as the adjacent non-2xx branch (L114–122). Three new async tests cover HTML body, empty body, and a valid-JSON regression guard. CI green. 1 file, +131/-4.

Changes

File	Summary
src/openhuman/inference/provider/ops.rs	text-then-parse replaces response.json() on the success path; 3 axum-mock tests added

Actionable comments (0)

LGTM on the substance. Below are 2 polish nits and 1 question — none blocking.

Nitpicks (2)

• src/openhuman/inference/provider/ops.rs:29 — truncate_with_ellipsis(&sanitized, 300) is effectively a no-op cap. sanitize_api_error already truncates to MAX_API_ERROR_CHARS = 200 (L340), so the outer 300-char cap can never re-trim. Mirrors the same dead pattern at L116–117 in the non-2xx branch — keeping it here for consistency is fine, but worth a follow-up cleanup PR to drop both calls (and either rename MAX_API_ERROR_CHARS or lift it to 300 if the larger cap is actually wanted).
• src/openhuman/inference/provider/ops.rs:28-34 — empty-body case currently formats as (body: ) with nothing after the colon. Cosmetic only; consider:

  let snippet = crate::openhuman::util::truncate_with_ellipsis(&sanitized, 300);
  if snippet.is_empty() {
      format!("[providers][list_models] failed to parse JSON: {} (empty body)", e)
  } else {
      format!("[providers][list_models] failed to parse JSON: {} (body: {})", e, snippet)
  }

  Captures the empty-body case the list_models_empty_body_returns_diagnostic_error test already exercises.

Questions for the author (1)

• src/openhuman/inference/provider/ops.rs:70-74 — build_runtime_proxy_client_with_timeouts reads proxy config from env. Tests hit 127.0.0.1, which most proxies bypass via NO_PROXY defaults, but a CI runner with a global HTTPS_PROXY and no localhost exception could route through it and 502 the loopback. Did you confirm the test sandbox has a permissive NO_PROXY, or does the reqwest builder special-case loopback? If not, a .no_proxy() test helper would harden this.

Outside the diff

• Other response.json()-style decode sites in provider/ were greppable — only this one exists. Scope is complete; no other call sites need the same treatment.

Verified / looks good

• Canonical error prefix [providers][list_models] failed to parse JSON: preserved → existing log greps / Sentry classifiers continue matching.
• Sanitization chain identical to the adjacent non-2xx branch (no new redaction policy).
• The new (body: …) clause is suffix-only / append-only — existing string matchers won't break.
• is_openrouter_provider check correctly bypassed for generic-test slug + 127.0.0.1 host → tests hit /models directly, not the OR pre-validation path.
• AuthStyle::None test config avoids auth-header coupling.
• Body is only read on the success path; the non-2xx branch (its own text() + sanitize) is untouched.
• 3 tests cover: captive-portal HTML, empty body, valid-JSON regression guard.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/openhuman/inference/provider/ops.rs (1)

174-178: 💤 Low value

Consider unifying logging library usage across the file.

The file uses both log::info!/log::debug! (lines 47, 59, 174) and tracing::info!/tracing::warn! (lines 514, 670). While this works, standardizing on one logging facade would improve consistency.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/inference/provider/ops.rs` around lines 174 - 178, The file
mixes log::info!/log::debug! and tracing::info!/tracing::warn!, causing
inconsistent logging; pick one facade (prefer tracing for structured/contextual
logs) and replace the other throughout—e.g., change the occurrences of
log::info! in the list models path (the snippet using entry.slug and
models.len(), and other uses like the debug at the top of the file) to
tracing::info!/tracing::debug!, or vice versa if you prefer log; ensure imports
are updated (remove log::... or tracing::... as appropriate) and the log macros
inside functions such as the list_models-related code and the sites that call
tracing::warn!/tracing::info! are unified to the chosen facade.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/openhuman/inference/provider/ops.rs`:
- Around line 174-178: The file mixes log::info!/log::debug! and
tracing::info!/tracing::warn!, causing inconsistent logging; pick one facade
(prefer tracing for structured/contextual logs) and replace the other
throughout—e.g., change the occurrences of log::info! in the list models path
(the snippet using entry.slug and models.len(), and other uses like the debug at
the top of the file) to tracing::info!/tracing::debug!, or vice versa if you
prefer log; ensure imports are updated (remove log::... or tracing::... as
appropriate) and the log macros inside functions such as the list_models-related
code and the sites that call tracing::warn!/tracing::info! are unified to the
chosen facade.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: caee6408-b3ef-40f3-9c7e-7b7239e1e1bb

📥 Commits

Reviewing files that changed from the base of the PR and between 27259b7 and c42ccf6.

📒 Files selected for processing (1)

• src/openhuman/inference/provider/ops.rs

[graycyrus]

@YellowSnnowmann hey! the code looks good to me — clean fix for TAURI-RUST-12.

Quick scan:

• text-then-parse swap is correct and idiomatic; the sanitize + truncate chain mirrors the adjacent non-2xx branch exactly as intended
• three tests cover the right paths (HTML body, empty body, valid-JSON regression guard); test setup using ephemeral port binding and a non-openrouter slug to bypass OR pre-validation is solid
• no new exports or shared-type changes — callers are unaffected; the (body: ...) suffix is append-only so existing log greps and Sentry classifiers keep matching

One thing i noticed independently: failed to read response body is a new error prefix not covered by the existing Sentry classifier for this issue. That's fine — it's a distinct transport failure mode — but worth documenting so future grep/alert rules can catch both. Not blocking.

CI has one job still pending (Rust Core Tests + Quality). Once that's green i'll come back and approve.

[YellowSnnowmann]

@YellowSnnowmann hey! the code looks good to me — clean fix for TAURI-RUST-12.

Quick scan:

• text-then-parse swap is correct and idiomatic; the sanitize + truncate chain mirrors the adjacent non-2xx branch exactly as intended
• three tests cover the right paths (HTML body, empty body, valid-JSON regression guard); test setup using ephemeral port binding and a non-openrouter slug to bypass OR pre-validation is solid
• no new exports or shared-type changes — callers are unaffected; the (body: ...) suffix is append-only so existing log greps and Sentry classifiers keep matching

One thing i noticed independently: failed to read response body is a new error prefix not covered by the existing Sentry classifier for this issue. That's fine — it's a distinct transport failure mode — but worth documenting so future grep/alert rules can catch both. Not blocking.

CI has one job still pending (Rust Core Tests + Quality). Once that's green i'll come back and approve.

Now CI Tests fixed and all checks are green!

[M3gA-Mind]

CI is fully green including Rust Core Tests + Quality. Code review: clean diagnostic fix — the text-then-parse swap is idiomatic, sanitize+truncate chain mirrors the adjacent non-2xx branch correctly, and all three tests cover the right paths. Approving per graycyrus's signoff intent.