fix(cli): probe /v1/models live when custom provider has no api_key - #40554
fix(cli): probe /v1/models live when custom provider has no api_key#40554Morad37 wants to merge 1 commit into
Conversation
The should_probe condition in list_authenticated_providers() Section 4 used "not grp["models"]" to decide whether to skip live discovery for endpoints without an api_key. But grp["models"] collects models from both the explicit "models:" dict (user intentionally narrowed) AND the singular "model:" field (just the active selection). For custom providers without api_key, the single "model:" value made the list non-empty, causing should_probe = False and live discovery to be skipped. The /model picker would then only show 1 model instead of the full catalog. Fix: use "len(grp["models"]) <= 1" instead of "not grp["models"]" so that a single entry from the "model:" field is not mistaken for an explicit narrowing and live /v1/models probing still occurs. Closes NousResearch#40542
alpindiay
left a comment
There was a problem hiding this comment.
PR Review: #40554 -- fix(cli): probe /v1/models live when custom provider has no api_key
Summary
This PR fixes model discovery for custom providers that use only the singular model: config field (1 entry). Previously, not grp["models"] only probed live /v1/models when the models list was empty -- but a single-entry list from the singular model: field was treated as "explicitly narrowed" and skipped live discovery. This change adjusts the condition to len(grp["models"]) <= 1, so single-model entries (which likely represent a bare endpoint, not an intentional narrow-down) also trigger live probing.
Analysis
Security: [PASS] No issues. No secrets, no injection vectors, no path traversal.
Bugs: [PASS] Fixes a real bug where local Ollama/llama.cpp servers with only a model: field would show a single model instead of the full catalog.
Logic: [PASS] The change is sound. The key insight -- that a single model: entry is more likely to be a user specifying a default model on a bare endpoint rather than intentionally narrowing -- is well-reasoned and matches CLI behavior.
Style: [PASS] Clean, minimal diff. Comments updated to explain the new behavior clearly.
Missing tests: [NOTE] No new tests added. While the change is small, a unit test covering the len(grp["models"]) <= 1 branch would be valuable to prevent regression.
Verdict: LGTM. The fix is correct, focused, and well-documented.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Looks Good
- Single line change: the probe threshold treats a one-entry model: list same as an empty list.
- Fix targets the exact scenario the bug report describes (local Ollama/llama.cpp with a single model entry).
- Comment block updated to match the new behavior.
Reviewed by Hermes Agent
|
Merged via #61928 using #55158's declaration-tracking implementation. Your PR was the first submitted fix for #40542 and correctly identified the singular-model probe gap; we credited that contribution in the salvage PR. The merged implementation avoids the one-item explicit-catalog regression in the |
The
should_probecondition inlist_authenticated_providers()Section 4 usednot grp[models]to decide whether to skip live discovery for endpoints without anapi_key. Butgrp[models]collects models from both the explicitmodels:dict (user intentionally narrowed) AND the singularmodel:field (just the active selection).For custom providers without
api_key, the singlemodel:value made the list non-empty, causingshould_probe = Falseand live discovery skipped. The/modelpicker would show only 1 model instead of the full catalog.Fix: Use
len(grp[models]) <= 1instead ofnot grp[models]so that a single entry from themodel:field is not mistaken for an explicit narrowing. When a provider has noapi_keyand only the singularmodel:field, live/v1/modelsprobing still occurs -- matching the CLI'shermes modelbehaviour.Also updated the surrounding comments to accurately describe the three distinct cases: (1) explicit
models:list (2+ entries) -- skip probe, (2) singularmodel:field (1 entry) -- probe, (3) no models -- probe.Closes #40542