fix(inference): fail-fast on empty model for cloud providers, demote nvidia-nim Sentry noise#2811
Conversation
…nvidia-nim Sentry noise Fixes tinyhumansai#2784. `make_cloud_provider_by_slug` now rejects a zero-length effective model (provider string `nvidia-nim:` with no inline model AND no `default_model` entry) at factory construction time instead of silently forwarding `model=""` to the upstream API. Providers such as nvidia-nim respond with `{"error":{"message":"model field is required",...}}` (400) and Sentry accumulates one event per agent turn (TAURI-RUST-4NM). Changes: - `factory.rs`: bail with a clear, actionable message when `effective_model.trim().is_empty()` after resolution. Actionable path: either supply the model in the provider string (`nvidia-nim:<model-id>`) or set `default_model` in the `[[cloud_providers]]` config entry. - `config_rejection.rs`: add "model field is required" to the `PHRASES` list as defense-in-depth so any body that still reaches the HTTP layer is demoted to `log::info!` rather than Sentry. - `factory_test.rs`: two new unit tests — `cloud_provider_with_no_model_and_no_default_rejected` asserts the early-error path fires with a message naming the slug, and `cloud_provider_default_model_used_when_model_part_is_empty` asserts the happy-path fallback to `default_model` is preserved. - `config_rejection.rs` tests: `detects_nvidia_nim_missing_model_body` pins the "model field is required" classifier.
|
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 skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds early validation to fail provider creation when the resolved model is empty (except for OpenhumanJwt auth) and expands the provider error classifier to recognize case-insensitive "model field is required" messages; includes regression/unit tests for both behaviors. ChangesCloud Provider Model Validation and Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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}
Three lines were formatted in a way cargo fmt rejects: - config_rejection.rs: wrap bare assert! arg onto its own line - factory_test.rs: inline `let err = match` (no line-break before match) - factory_test.rs: inline `let (_, model) = call()` method chain
The empty-model bail! added in TAURI-RUST-4NM fired for openhuman: (auth_style=OpenhumanJwt) slugs, breaking openhuman_slug_routes_to_backend. OpenhumanJwt entries route to the OpenHuman backend and never forward the effective_model to an upstream API, so an empty model is valid for that auth style. Guard the check with auth_style != OpenhumanJwt.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@staimoorulhassan hey! the code looks good to me — solid fix for the nvidia-nim Sentry noise. the fail-fast in make_cloud_provider_by_slug is the right call, and the tests cover both cases cleanly. the config_rejection.rs defense-in-depth phrase is a nice touch too.
however, the Rust Core Coverage (cargo-llvm-cov) CI job is failing (the Run cargo llvm-cov for openhuman core step itself, not a coverage threshold — so this may be an infra flake, but it's blocking the coverage gate from running entirely). once that's green, i'll come back and approve this. let me know if you need any help!
|
Hi @graycyrus, thanks for the thorough review! The Our PR only touches |
staimoorulhassan
left a comment
staimoorulhassan
left a comment
There was a problem hiding this comment.
Commented in CodeRabbit Change Stack
|
Hi @graycyrus — the |
3 participants
Summary
Closes #2784.
make_cloud_provider_by_slugnow rejects a zero-length effective model at factory construction time instead of silently forwardingmodel=""to the upstream API. Providers like nvidia-nim respond with{"error":{"message":"model field is required",...}}(400), and each agent turn generates a Sentry event (TAURI-RUST-4NM).Root cause: In
make_cloud_provider_by_slug, when the provider string isnvidia-nim:(empty model component) and the[[cloud_providers]]entry has nodefault_model,effective_modelresolved to"". Nothing validated this before forwarding it in the API request body.Changes:
factory.rs: After resolvingeffective_model, bail with a clear, actionable message if it's still empty (and the entry is notOpenhumanJwt— JWT entries route to the backend and never forward the model).config_rejection.rs: Add"model field is required"to thePHRASESlist as defense-in-depth.factory_test.rs: Two new unit tests:cloud_provider_with_no_model_and_no_default_rejected— empty provider string + no default_model → clear error naming the slugcloud_provider_default_model_used_when_model_part_is_empty— empty provider string + has default_model → factory succeeds, resolves to default_modelconfig_rejection.rstests:detects_nvidia_nim_missing_model_bodypins the classifier against the real wire body.Test plan
cargo test -p openhuman -- inference::provider::factory_testpasses (two new tests + existing suite unchanged)cargo test -p openhuman -- inference::provider::config_rejection::testspasses[[cloud_providers]]entry withslug = "nvidia-nim"and nodefault_model; attempting to usenvidia-nim:shows the actionable error instead of a provider 400default_model = "meta/llama-3.1-8b-instruct"and provider stringnvidia-nim:→ factory uses the default modelSummary by CodeRabbit
Bug Fixes
Tests