fix(model_switch): filter /model picker for providers with no PROVIDER_REGISTRY entry - #57568
Closed
Ahmett101 wants to merge 1 commit into
Closed
Conversation
…ousResearch#57503) list_authenticated_providers() emits picker rows for every slug in PROVIDER_TO_MODELS_DEV that has any credential env-var set. Several of those slugs (notably 'mistral') have no PROVIDER_REGISTRY entry, so resolve_provider() rejects them as 'Unknown provider' once the user selects a model — leaving the picker showing rows that cannot actually be selected. Add a resolve-gate in section 1: if PROVIDER_REGISTRY.get(hermes_id) is None, skip the slug. The picker now only lists providers that can actually be switched to at runtime. This automatically resolves the duplicate-Mistral dedup symptom too: once the broken-from-models.dev row is filtered, the conflict between PROVIDER_TO_MODELS_DEV['mistral'] and a custom_providers 'Mistral' row is moot. Composes with NousResearch#50289 (which promotes mistral to first-class via the provider-plugin path): when that lands, PROVIDER_REGISTRY gains a 'mistral' entry and the gate becomes a no-op for it. No conflict. Tests (regression suite): - tests/hermes_cli/test_model_switch_filter_unresolved.py (new, 4 tests): Picker excludes 'mistral' when MISTRAL_API_KEY is set; 'deepseek' and 'xai' (PROVIDER_REGISTRY-backed) still appear; 'mistral' stays excluded when no key is set. Confirmed by reverting the fix and seeing the test fail with 'mistral leaked into /model picker'. Cross-checked against the existing 51 test_model_switch_* and test_custom_provider_* cases — 55/55 PASS, no regressions.
Contributor
Contributor
Contributor
|
Superseded by the improved consolidated salvage PRs prepared in this review batch; contributor credit is preserved in the replacement PR body/commits. |
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
list_authenticated_providers()(powers/modelin the CLI, gateway, and Telegram picker) emits a row for every slug inPROVIDER_TO_MODELS_DEVwhose credential env-var is set. Several of those slugs (notablymistral) have noPROVIDER_REGISTRYentry, soresolve_provider()rejects them withUnknown provider 'mistral'once the user selects a model. The picker shows rows that cannot actually be selected. This is #57503.Fix: a single resolve-gate in
list_authenticated_providers()Section 1 — ifPROVIDER_REGISTRY.get(hermes_id)is None, skip the slug. The picker now only lists providers that can actually be switched to. This also resolves the duplicate-Mistral dedup symptom: once the broken-from-models.dev row is filtered, the conflict betweenPROVIDER_TO_MODELS_DEV['mistral']and acustom_providersMistralrow becomes moot.Composes with #50289 (the in-flight first-class-providers plugin merge for mistral): when/if that lands,
PROVIDER_REGISTRYgains amistralentry and the new gate becomes a no-op for it. The two PRs are independent — neither blocks nor breaks the other.Changes
hermes_cli/model_switch.py: 9-line addition inlist_authenticated_providers()Section 1: aif not pconfig: continuegate after the existingpconfig.auth_type != "api_key"skip. Documented inline, references the issue.tests/hermes_cli/test_model_switch_filter_unresolved.py(new): 4 regression tests.test_mistral_filtered_when_unregistered_but_api_key_set— pin:MISTRAL_API_KEYset ⇒mistralMUST NOT appear in picker.test_resolveable_provider_still_appears— regression guard:deepseek(PROVIDER_REGISTRY-backed) with key set MUST still appear.test_resolve_gate_skips_models_dev_only_provider_without_creds— no-key case stays filtered.test_picker_skips_pconfig_none_does_not_break_other_section1_providers—xai(REGISTRY-backed) with key set still appears.How to Test
Checklist
fix(scope):)os.environread withmonkeypatch, no platform-specific code)Risk & Impact
Low. Single-file 9-line addition in the picker code path; behaviour change is strictly "drop a row from a list when that row would 100% fail on selection". Users who currently hit the bug get a fixed picker immediately; users with working environments see the picker unchanged (every other provider in
PROVIDER_TO_MODELS_DEVis also inPROVIDER_REGISTRY, so they keep appearing).Type: Bug fix
Closes #57503
Related findings (out of scope here, flagged for maintainers):
mistraland just keeps defending against the next unregistered models.dev provider that anyone adds toPROVIDER_TO_MODELS_DEV. The gate is intentionally defensive: same code path would have caught a follow-up like "provider X has Y key set but isn't in registry" without a second PR.PROVIDER_TO_MODELS_DEVagainstPROVIDER_REGISTRY(one-shot script, no runtime cost) would let us surface any other slugs that historically slipped through, instead of relying on users to file bugs one-by-one. Out of scope for this PR.