fix(hermes): refresh model.base_url on provider switch#2760
Open
lihan3238 wants to merge 1 commit into
Open
Conversation
apply_switch_defaults uses struct-update (..current) to preserve user customizations (context_length / max_tokens / extra), but this also silently preserved base_url across provider switches. Auxiliary services (title generation, session search) then hit the previous provider and returned HTTP 404. Use the new providers settings_config.base_url when set, falling back to the existing value when the new providers settings omit base_url (so a user override survives a switch to a provider that does not declare one). Add two regression tests: - updates_base_url_to_new_provider: covers the issue farion1231#2757 reproduction - ignores_blank_base_url_in_settings: guards the user-override fallback The four pre-existing apply_switch_defaults tests continue to pass unchanged (none of them asserted base_url with a non-empty settings.base_url, so the new behavior does not regress them). Closes farion1231#2757
Owner
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2757.
apply_switch_defaultsinsrc-tauri/src/hermes_config.rsused..currentstruct-update to preserve user customizations (context_length,max_tokens,extra). But this also silently preservedbase_urlacross provider switches — so when the user switched from DeepSeek to Kimi For Coding, themodel.base_urlfield in~/.hermes/config.yamlkept pointing at DeepSeek. Auxiliary services (title generation, session search) then 404'd against the wrong endpoint.The issue reporter (@KaisvenZ) provided a complete root-cause analysis including DB-vs-yaml diff evidence and pseudocode for the fix.
Change
src-tauri/src/hermes_config.rsapply_switch_defaults:Use the new provider's
settings_config.base_urlwhen set, falling back to the existing value when the new provider's settings omitsbase_url(so a user override survives a switch to a provider that does not declare its own).Docstring updated to spell out the new precedence.
Tests
Added two regression tests:
apply_switch_defaults_updates_base_url_to_new_provider— direct reproduction of [Bug] Hermes model.base_url not updated when switching providers (HTTP 404 on auxiliary services) #2757 (DeepSeek → Kimi switch with both providers declaring their ownbase_url)apply_switch_defaults_ignores_blank_base_url_in_settings— guards the fallback path so a whitespace-onlybase_urlin settings does not blank out an existing overrideThe four pre-existing
apply_switch_defaultstests continue to pass unchanged. None of them assertedbase_urlwith a non-emptysettings.base_urlvalue — they covered:sets_default_and_provider) settings hadbase_urlbut didn't assert on it → still passes; new behavior just makes the assertion possiblepreserves_user_context_length) settings lackedbase_url, asserts user override is preserved → still works (falls back to current via.or())updates_provider_even_without_models) settings hadbase_urlbut didn't assert on it → still passeskeeps_old_default_when_first_model_id_is_blank) settings lackedbase_url→ still worksBehavior change note
This change refreshes
model.base_urlwhenever the new provider declares one, which is a user-visible behavior change. The previous code would have kept a manually-editedmodel.base_urleven across a provider switch. Two views are reasonable:base_urlshould refresh.Happy to defer if you prefer the alternative — the simplest revert is to drop the
base_url:line in the struct literal and remove the new test. I went with the issue-reporter's view because the breakage they describe (404s on auxiliary services) is consistent and affects real users, and because the previous "always preserve" behavior was implicit in the..currentrather than explicitly designed.Verification
cargo fmt --checkpasses locallycargo testlocally (Tauri build needspkg-config+libssl-devsystem packages I don't have sudo for); diff is small and identical in shape to surrounding tested code, relying on the backend CI job to verify.