fix(inference): suppress spurious Sentry events for Ollama tool-schema rejection 400s#2812
Conversation
…ols' streaming 400 The `stream_native_chat` error handler fired a Sentry event for every Ollama model that returns HTTP 400 "does not support tools", even though the retry loop in `chat()` already handles this case correctly by re-issuing the request without tool definitions. Root cause: the streaming error path checked `is_budget_exhausted_http_400`, `is_custom_openai_upstream_bad_request_http_400`, `is_provider_access_policy_denied_http_403`, and `is_provider_config_rejection_http` — but not `is_native_tool_schema_unsupported`, which already existed and matched the exact Ollama wire shape. The check was only present in the non-streaming `chat()` path (line 1871), so each streaming attempt produced a TAURI-RUST-4K7 Sentry event before the retry succeeded. Fix: add an `is_native_tool_schema_unsupported` check between the config-rejection and `should_report_provider_http_failure` arms in `stream_native_chat`'s non-2xx handler. Matched events are demoted to `log::info!` — the caller's retry loop handles the actual recovery. Tests: two new unit tests: - `is_native_tool_schema_unsupported_matches_ollama_does_not_support_tools` — pin detection of all known phrases, non-matching statuses, and unrelated 400 bodies - `err_supports_no_tools_retry_matches_streaming_error_format` — verify the retry predicate correctly parses the anyhow string produced by stream_native_chat Closes tinyhumansai#2787
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis change adds targeted error handling in ChangesTool Schema Unsupport Detection and Retry
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/inference/provider/compatible_tests.rs (1)
1588-1595: ⚡ Quick winConsider adding a positive test case for 422 UNPROCESSABLE_ENTITY.
The function
is_native_tool_schema_unsupported(line 879) accepts bothBAD_REQUEST(400) andUNPROCESSABLE_ENTITY(422) status codes. While the current test verifies that a 500 status doesn't match, it doesn't verify that a 422 status does match with a valid tool-rejection body.✅ Suggested test addition
// Non-400 status must not match even with the right body. assert!( !OpenAiCompatibleProvider::is_native_tool_schema_unsupported( StatusCode::INTERNAL_SERVER_ERROR, r#"{"error":"does not support tools"}"#, ), "500 with tool-schema body must not match" ); + + // 422 UNPROCESSABLE_ENTITY should also match. + assert!( + OpenAiCompatibleProvider::is_native_tool_schema_unsupported( + StatusCode::UNPROCESSABLE_ENTITY, + r#"{"error":"does not support tools"}"#, + ), + "422 with tool-schema body must match" + ); // Unrelated 400 bodies must not match.🤖 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/compatible_tests.rs` around lines 1588 - 1595, Add a positive test asserting that OpenAiCompatibleProvider::is_native_tool_schema_unsupported returns true for a 422 UNPROCESSABLE_ENTITY response with the same tool-rejection body; mirror the existing 400 test in compatible_tests.rs by calling is_native_tool_schema_unsupported(StatusCode::UNPROCESSABLE_ENTITY, r#"{"error":"does not support tools"}"#) and assert it is true with an explanatory message so the function's handling of 422 is covered.
🤖 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/compatible_tests.rs`:
- Around line 1588-1595: Add a positive test asserting that
OpenAiCompatibleProvider::is_native_tool_schema_unsupported returns true for a
422 UNPROCESSABLE_ENTITY response with the same tool-rejection body; mirror the
existing 400 test in compatible_tests.rs by calling
is_native_tool_schema_unsupported(StatusCode::UNPROCESSABLE_ENTITY,
r#"{"error":"does not support tools"}"#) and assert it is true with an
explanatory message so the function's handling of 422 is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 864616f5-2e99-4366-93c8-1e00adf70161
📒 Files selected for processing (2)
src/openhuman/inference/provider/compatible.rssrc/openhuman/inference/provider/compatible_tests.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@staimoorulhassan hey! the code looks good to me — the fix is targeted and the tests are solid. one thing before I approve: the "Rust Core Tests (Windows — secrets ACL)" job is failing. that one tends to need org secrets that aren't available on external contributor PRs, so it may not be related to your changes, but CI needs to be 100% green before I can sign off. can you check whether that failure is pre-existing or if there's something you can do to resolve it? once that's green, I'll come back and approve this.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
CI is green now — the prior Windows sccache flake sorted itself out with the retrigger.
Code itself was clean on first read: the is_native_tool_schema_unsupported guard slots in exactly where it belongs (between config-rejection and the Sentry gate), the log::info! demotion is the right severity, and both tests are well-structured — pinning the detection phrases, verifying the non-400 escape hatch, and confirming the retry predicate parses the streaming error format correctly.
Approved.
staimoorulhassan
dismissed stale reviews from graycyrus and coderabbitai[bot]
via
1140c8a
staimoorulhassan
left a comment
staimoorulhassan
left a comment
There was a problem hiding this comment.
PR ready to be merged @graycyrus
4 participants
Summary
Closes #2787.
Every Ollama model that doesn't support tool-calling generates a Sentry event on each agent turn that tries to use tools. The
chat()retry loop already handles these 400s by re-issuing without tools — the bug was thatstream_native_chatfired the Sentry event before the retry loop could handle it silently.Root cause: In
stream_native_chat, the non-2xx handler checkedis_provider_config_rejection_httpbut notis_native_tool_schema_unsupported. Tool-schema-rejection 400s therefore fell through toshould_report_provider_http_failure→ Sentry before the caller's retry loop could suppress them.Changes:
compatible.rs—stream_native_chat: Insertis_native_tool_schema_unsupportedcheck between the config-rejection guard and the Sentry gate. When matched, log atinfo!and return the error string (the retry loop inchat()catches it and re-issues without tools).compatible_tests.rs: Two new unit tests covering the Sentry-suppression path for the Ollama "does not support tools" wire body.Test plan
cargo test -p openhuman -- inference::provider::compatible_testspassesTAURI-RUST-4K7eventsSummary by CodeRabbit