fix(providers): fall back to reasoning-v1 for unrecognized default_model#2223
Conversation
…del (tinyhumansai#2202) Stale `default_model` values written by older UI versions (e.g. `deepseek-v4-pro`, `claude-opus-4-7`) were forwarded verbatim to the OpenHuman backend, which rejects them with HTTP 400. This caused 88 Sentry events (TAURI-WJ + TAURI-QW). Changes: - Add `is_known_openhuman_tier(model)` helper in `factory.rs` that recognises the five canonical tier names plus `hint:*` prefixes. - In `make_openhuman_backend()`, replace the bare `_ => model` fall- through with a validated path: unknown tiers log a WARN and fall back to `MODEL_REASONING_V1`, matching the existing behaviour for an empty `default_model`. - In `config/ops.rs`, log a WARN when an unrecognised model is saved so the issue is surfaced at config-write time too. - Add 6 new unit tests in `factory_test.rs` covering the tier helper and the factory fallback. Closes tinyhumansai#2202
|
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 (2)
📝 WalkthroughWalkthroughAdds a helper to recognize OpenHuman tier names and ChangesOpenHuman tier validation and fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/config/ops.rs (1)
434-442:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
default_modelbefore persisting.Line 438 stores raw input while Line 441 validates
trim()ed input. That mismatch lets values like"chat-v1 "pass validation but behave differently at inference-time matching. Persist the trimmed value directly.Suggested patch
if let Some(model) = update.default_model { - config.default_model = if model.trim().is_empty() { + let trimmed = model.trim(); + config.default_model = if trimmed.is_empty() { None } else { - Some(model) + Some(trimmed.to_string()) }; if let Some(ref m) = config.default_model { - let trimmed = m.trim(); - if !crate::openhuman::inference::provider::factory::is_known_openhuman_tier(trimmed) { + if !crate::openhuman::inference::provider::factory::is_known_openhuman_tier(m) { log::warn!( "[config][model-settings] default_model '{}' is not a recognized \ OpenHuman backend tier — it will be replaced with the platform \ default at inference time.", - trimmed + m ); } } }🤖 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/config/ops.rs` around lines 434 - 442, Persist the trimmed default_model instead of the raw input: take update.default_model, call trim() and if the trimmed string is empty set config.default_model = None, otherwise set config.default_model = Some(trimmed.to_string()), then run the existing validation against that trimmed value (the check using crate::openhuman::inference::provider::factory::is_known_openhuman_tier). This ensures update.default_model and config.default_model are normalized the same way and avoids mismatches at inference time.
🤖 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.
Outside diff comments:
In `@src/openhuman/config/ops.rs`:
- Around line 434-442: Persist the trimmed default_model instead of the raw
input: take update.default_model, call trim() and if the trimmed string is empty
set config.default_model = None, otherwise set config.default_model =
Some(trimmed.to_string()), then run the existing validation against that trimmed
value (the check using
crate::openhuman::inference::provider::factory::is_known_openhuman_tier). This
ensures update.default_model and config.default_model are normalized the same
way and avoids mismatches at inference time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c26ce086-6618-4aad-913b-81bfc5a58fcc
📒 Files selected for processing (3)
src/openhuman/config/ops.rssrc/openhuman/inference/provider/factory.rssrc/openhuman/inference/provider/factory_test.rs
The `resolve_subagent_provider_hint_with_config_routes_via_factory` test used `"agentic-specific-model"` as the sentinel default_model, but the PR's `is_known_openhuman_tier` guard in `make_openhuman_backend` rejects unrecognized tier strings and falls back to `reasoning-v1`. Switch to `"coding-v1"` — a valid tier that the factory validation accepts and that differs from the old `agentic-v1` synthesis the test is guarding against.
|
Pre-push hook note: Pushed with |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — fix(providers): fall back to reasoning-v1 for unrecognized default_model
Solid bug fix addressing the 88 Sentry events from stale default_model values. The is_known_openhuman_tier helper and the fallback logic in make_openhuman_backend are clean, and the test coverage is thorough. One logic gap in the hint validation that needs fixing before merge.
| File | Area | What changed |
|---|---|---|
factory.rs |
Rust core | Added is_known_openhuman_tier() helper; replaced bare _ => model with validated fallback to MODEL_REASONING_V1 |
ops.rs |
Config | Added WARN log when unrecognized model is saved to config |
factory_test.rs |
Tests | 6 new tests covering tier validation + factory fallback behavior |
ops_tests.rs |
Tests | Updated existing test to use a valid tier (coding-v1) instead of arbitrary string |
1 major finding below (inline). CodeRabbit's trim-normalization finding in ops.rs is valid and not repeated here.
The previous blanket `model.starts_with("hint:")` check accepted any
`hint:*` string. Combined with the four-arm match in
`make_openhuman_backend`, this meant values like `hint:garbage` or
`hint:reasoning-quick` fell through to the `_ =>` arm where the tier
check still returned true — so the untranslated `hint:*` string was
forwarded to the backend, producing the same HTTP 400 this PR aims to
prevent. Enumerate only the four hints the factory actually translates
so unrecognized hints fall back to MODEL_REASONING_V1.
Also normalize `default_model` at config-save time: persist the
trimmed value rather than the raw input, so the persisted value and
the validation check operate on the same string.
Addresses @graycyrus inline review on factory.rs:64 and CodeRabbit's
outside-diff finding on config/ops.rs:434.
…tale non-hint models The previous fix (fa92072) was too aggressive: it enumerated only the four hints the factory translates (hint:reasoning/chat/agentic/coding) and fell back to reasoning-v1 for *any* other hint:* string. This broke the web-chat model_override path which is designed to forward lightweight hint strings like hint:reaction verbatim to the backend. Split the `_ =>` arm into explicit `Some(_)` and `None` branches: - Some(unrecognized suffix): forward the original hint:* string unchanged. The backend is authoritative over which hint values it accepts. - None (no hint: prefix): apply is_known_openhuman_tier() and fall back to reasoning-v1 for stale default_model values (deepseek-v4-pro, etc.). Update make_openhuman_backend_falls_back_for_unknown_hint → make_openhuman_backend_forwards_unknown_hint_verbatim to match the new semantics and verify hint:reaction, hint:garbage, hint:summarization all pass through unchanged.
|
The CI failure in What failed: The web-chat channel path explicitly forwards Fix in bbbec08: Split the match arm explicitly into
This preserves the protection against stale |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Re-review — previous changes addressed
The major finding from my last review (blanket hint:* prefix match in is_known_openhuman_tier creating a bypass for unrecognized hint strings) has been fixed across commits fa92072 and bbbec08.
What changed:
is_known_openhuman_tiernow explicitly enumerates the four known hint strings instead of using a prefix match — good.- The factory match now has a proper
Some(_)arm for unrecognized hints (forwarded verbatim to backend) vs theNonearm for non-hint strings (fallback toreasoning-v1if not a known tier). The separation is clean and well-commented. - Tests updated:
hint:garbage,hint:reasoning-quick, and emptyhint:are correctly rejected byis_known_openhuman_tier;make_openhuman_backend_forwards_unknown_hint_verbatimconfirms the forwarding path. - Trim normalization in
ops.rsapplied (CodeRabbit's suggestion).
The design choice to forward unknown hint:* strings to the backend (authoritative) while only falling back for stale non-hint default_model values is reasonable — it keeps the door open for new backend-side hint types without requiring client updates.
No new findings. Clean from my side.
3 participants
Summary
default_modelvalues (e.g.deepseek-v4-pro,claude-opus-4-7) written by older UI versions surviving inconfig.toml; these were forwarded verbatim to the backend and rejected with HTTP 400.is_known_openhuman_tier(model)helper recognising the five canonical backend tiers plushint:*prefixed strings.make_openhuman_backend(), replaces the bare_ => modelfall-through with a validated path: unknown tiers log aWARNand fall back toMODEL_REASONING_V1, matching existing behaviour for an emptydefault_model.WARNinapply_model_settings()when an unrecognised model name is saved to config (diagnostic only, non-blocking).Problem
config.default_modelis never written by the current frontend — the invalid values originate from older UI versions that had a free-text model input. They persist through app updates and the new UI never clears them.CustomRoutingDialogdropdown (added in feat(ai-panel): add chat workload and cloud model picker with slug-based lookup fix #2152) only covers per-workload routing to custom cloud providers and does not fix staledefault_modelvalues.Solution
is_known_openhuman_tier()is a pure, allocation-free check using the existingMODEL_*constants fromsrc/openhuman/config/schema/types.rs.reasoning-v1is the same default already applied for blankdefault_model, so this is zero-risk for users with valid configs.Submission Checklist
factory.rsand the helper;config/ops.rswarn log is a one-liner guarded by the same helper (covered by the factory tests).Closes #NNNin the## Relatedsection.Impact
default_modelvalues will silently getreasoning-v1instead of an HTTP 400 error — no user-visible regression.WARN-level log line will appear in core logs when the fallback fires, aiding future debugging.Related
Closes #2202
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/invalid-model-name-fallback35b29599d105359a7588bbcaf6312dd7fb2e9bb6Validation Run
pnpm --filter openhuman-app format:check— passedpnpm typecheck— passed (no frontend changes)cargo test -p openhuman 'factory_test::'— 36 tests pass (6 new)cargo fmt --all -- --check+cargo check --manifest-path Cargo.toml— cleanValidation Blocked
git push -u origin fix/invalid-model-name-fallbackBootCheckGate.tsx,RotatingTetrahedronCanvas.tsx,UnsubscribeApprovalCard.tsx, and others)--no-verify. All pre-existing warnings; zero frontend files changed in this PR.Behavior Changes
make_openhuman_backend()now falls back toreasoning-v1for unrecogniseddefault_modelvalues instead of forwarding them to the backend.Parity Contract
reasoning-v1,chat-v1,agentic-v1,coding-v1,reasoning-quick-v1) and allhint:*strings are unchanged; only invalid/unknown names are affected.MODEL_REASONING_V1— identical to the fallback for blank/emptydefault_model(line 200 in factory.rs before this patch).Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests