fix(cli): accept available_models in custom provider config - #52287
harjothkhara wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Well-tested fix for custom provider model switching. The fix correctly recognizes available_models as a supported declaration and parses provider:model / custom:provider:model colon forms. Good refactoring of _match into a reusable _match_declared_model function.
Changes:
hermes_cli/config.py: Normalizesavailable_modelsinto custom provider model viewhermes_cli/model_switch.py: New_DECLARED_MODEL_KEYSand_match_declared_modelfunction
LGTM.
Reviewed by Hermes Agent
e1224a5 to
8e25689
Compare
|
Rebased onto current While rebasing I ran a cross-model review and hardened one real issue it surfaced:
Note on scope: the literal |
8e25689 to
460f0b0
Compare
|
Ping — approved Jun 25 and green. Let me know if anything's blocking the merge. |
528fe08 to
3e15e9a
Compare
|
Thanks for the focused fix. The current-main premise is verified: The PR head Automated hermes-sweeper review. |
3e15e9a to
0e33362
Compare
|
Thanks for the review. Rebased onto current main: the endpoint listing was restructured into grouped endpoints on main, so the |
0e33362 to
5c62552
Compare
|
Re-scoped against current Two things changed since this was last touched:
The diff went from four files to one plus tests. Worth flagging: the approval on this PR predates the rework and doesn't describe the current change. Happy to have it re-reviewed from scratch — or closed, if the narrower fix is better folded in elsewhere. |
|
@kshitijk4poor — could I get a maintainer review here? You've been in Status: green (27 checks), one file plus tests, fixes #52266. Flagging one thing so it isn't misread: the approval showing on this PR is from an external contributor, not a maintainer, and it predates a re-scope + force-push that replaced the diff. It shouldn't count toward merge — this needs a fresh look. The change is narrow. Worth noting the other half of the original issue (the Equally happy to close this if you'd rather fold the fix in elsewhere — #52266 would just need to stay open. |
|
@teknium1 — could you take a look, or point me at whoever owns Fixes #52266, still open and unassigned. I re-verified on current main: One file plus tests, green CI, no conflicts. The Jun 25 approval was dropped by a re-scope force-push, so it needs a fresh look rather than a re-approve. |
|
Status check-in: still green, no conflicts, one file plus tests, on today's |
Hand-written custom-provider entries declare their model list as
available_models, but _normalize_custom_provider_entry only knew models.
The alias fell through to the unknown-key warning and was dropped, so the
normalized entry carried no models at all: the picker listed the provider
with (0) models and every /model <id> against it failed to resolve, since
all routing paths read the normalized models key.
Merge both keys at the normalization chokepoint so every downstream
consumer (typed routing, picker list, validation fallback) sees them
without each having to learn the alias. models metadata wins for ids
declared in both. The list/mapping coercion moves to a helper so the
alias gets the same [{id: ...}] row handling models already had.
Fixes NousResearch#52266
5c62552 to
167a7e1
Compare
What does this PR do?
Makes
available_modelsa recognized model-declaration key for custom providers, so a provider that declares its models under that key is actually routable by/model.This PR has been re-scoped against current
main— see "What changed" below.Related Issue
Fixes #52266
The bug on current
main_normalize_custom_provider_entry()(hermes_cli/config.py) is the single chokepoint every custom-provider path runs through. It recognized onlymodels, so a hand-written entry like the one in #52266:normalizes to an entry with no
modelskey at all, and emits:Every downstream consumer reads the normalized
modelskey, so the provider lists (0) models in the picker and no/model <id>against it resolves.What changed since this PR was opened
The original version of this PR also added configured-provider colon parsing (
tokenplan:deepseek-v4-flash,custom:tokenplan:qwen3.7-max). That half is now fixed onmainby the "single-owner/modelrequest parsing" consolidation inmodel_switch.py— all three forms from the issue parse cleanly there today, including the reportedValueError: too many values to unpack (expected 3).So this PR is re-scoped to the one thing still broken: the unrecognized declaration key. The diff is now one file plus tests, instead of the previous four-file change to
model_switch.pyandconfig.py.Reviewers: the earlier approval predates this rework and no longer describes the current diff — fresh eyes welcome.
Changes Made
hermes_cli/config.pyavailable_modelsandmodelsin_normalize_custom_provider_entry(). Fixing it at the chokepoint means typed routing, the picker list, and validation fallback all pick it up without each learning the alias.modelsmetadata wins for ids declared under both keys._declared_models_to_mapping()so the alias inherits the same[{id: ...}]row handlingmodelsalready had, instead of duplicating it.available_modelsto_KNOWN_KEYSso it no longer trips the unknown-key warning.tests/hermes_cli/test_custom_provider_available_models.py(new)[{id: ...}]rows via the alias;models-wins-on-conflict;models-only entries unchanged; neither key leavesmodelsunset; and a no-mutation check (entries alias the shared read-only config cache).How to Test
available_modelswith hyphenated ids, as above./model tokenplan:deepseek-v4-flash(orcustom:tokenplan:qwen3.7-max, or/model deepseek-v4-flash --provider tokenplan).custom:tokenplanand the requested model; the picker lists all declared models rather than (0).Verified on this branch:
tests/hermes_cli/test_custom_provider_available_models.py→ 6 passedtest_config_validation.py,test_custom_provider_normalize_no_mutate.py,test_custom_provider_extra_headers.py,test_provider_config_validation.py,test_runtime_provider_resolution.py,test_config.py→ 142 passedtests/hermes_cli/ -k "model_switch or picker or custom_provider or inventory"→ 218 passedruff checkon both changed files → passedType of Change