fix(picker): keep max_models=0 distinct from unlimited; lock cap semantics
Follow-up to the cap-removal salvage. The contributor guarded the new unlimited default with `[:max_models] if max_models else ...`, which conflates max_models=0 (used by slug-only callers that want an empty model list) with None (unlimited). Tighten to `is not None` at all five slicing sites in list_authenticated_providers / list_picker_providers, and add a regression test asserting the three-way contract: None=full, 0=empty, N=first N.
This commit is contained in:
parent
9705e7944a
commit
3042045540
@ -1425,7 +1425,7 @@ def list_authenticated_providers(
|
|||||||
if hermes_id in _MODELS_DEV_PREFERRED:
|
if hermes_id in _MODELS_DEV_PREFERRED:
|
||||||
model_ids = _merge_with_models_dev(hermes_id, model_ids)
|
model_ids = _merge_with_models_dev(hermes_id, model_ids)
|
||||||
total = len(model_ids)
|
total = len(model_ids)
|
||||||
top = model_ids[:max_models] if max_models else model_ids
|
top = model_ids[:max_models] if max_models is not None else model_ids
|
||||||
|
|
||||||
slug = hermes_id
|
slug = hermes_id
|
||||||
pinfo = _mdev_pinfo(mdev_id)
|
pinfo = _mdev_pinfo(mdev_id)
|
||||||
@ -1588,7 +1588,7 @@ def list_authenticated_providers(
|
|||||||
if hermes_slug in _MODELS_DEV_PREFERRED:
|
if hermes_slug in _MODELS_DEV_PREFERRED:
|
||||||
model_ids = _merge_with_models_dev(hermes_slug, model_ids)
|
model_ids = _merge_with_models_dev(hermes_slug, model_ids)
|
||||||
total = len(model_ids)
|
total = len(model_ids)
|
||||||
top = model_ids[:max_models] if max_models else model_ids
|
top = model_ids[:max_models] if max_models is not None else model_ids
|
||||||
|
|
||||||
results.append({
|
results.append({
|
||||||
"slug": hermes_slug,
|
"slug": hermes_slug,
|
||||||
@ -1663,7 +1663,7 @@ def list_authenticated_providers(
|
|||||||
if not _cp_model_ids:
|
if not _cp_model_ids:
|
||||||
_cp_model_ids = curated.get(_cp.slug, [])
|
_cp_model_ids = curated.get(_cp.slug, [])
|
||||||
_cp_total = len(_cp_model_ids)
|
_cp_total = len(_cp_model_ids)
|
||||||
_cp_top = _cp_model_ids[:max_models] if max_models else _cp_model_ids
|
_cp_top = _cp_model_ids[:max_models] if max_models is not None else _cp_model_ids
|
||||||
|
|
||||||
results.append({
|
results.append({
|
||||||
"slug": _cp.slug,
|
"slug": _cp.slug,
|
||||||
@ -1812,7 +1812,7 @@ def list_authenticated_providers(
|
|||||||
"name": "Custom endpoint",
|
"name": "Custom endpoint",
|
||||||
"is_current": True,
|
"is_current": True,
|
||||||
"is_user_defined": True,
|
"is_user_defined": True,
|
||||||
"models": _models[:max_models] if max_models else _models,
|
"models": _models[:max_models] if max_models is not None else _models,
|
||||||
"total_models": len(_models),
|
"total_models": len(_models),
|
||||||
"source": "model-config",
|
"source": "model-config",
|
||||||
"api_url": str(current_base_url).strip().rstrip("/"),
|
"api_url": str(current_base_url).strip().rstrip("/"),
|
||||||
@ -2082,7 +2082,7 @@ def list_picker_providers(
|
|||||||
except Exception:
|
except Exception:
|
||||||
live_ids = list(p.get("models", []))
|
live_ids = list(p.get("models", []))
|
||||||
p = dict(p)
|
p = dict(p)
|
||||||
p["models"] = live_ids[:max_models] if max_models else live_ids
|
p["models"] = live_ids[:max_models] if max_models is not None else live_ids
|
||||||
p["total_models"] = len(live_ids)
|
p["total_models"] = len(live_ids)
|
||||||
|
|
||||||
has_models = bool(p.get("models"))
|
has_models = bool(p.get("models"))
|
||||||
|
|||||||
@ -423,6 +423,71 @@ class TestIntegrationWithModelsModule:
|
|||||||
assert nous_row is not None, "nous row must appear when authed"
|
assert nous_row is not None, "nous row must appear when authed"
|
||||||
assert nous_row["models"] == expected
|
assert nous_row["models"] == expected
|
||||||
|
|
||||||
|
def test_picker_max_models_cap_semantics(self, tmp_path, monkeypatch):
|
||||||
|
"""The cap argument has three distinct meanings on the real slicing
|
||||||
|
path: ``None`` = unlimited (the cap-removal fix, #48297), ``0`` = no
|
||||||
|
models (preserved for slug-only callers), an int N = first N. Guards
|
||||||
|
the ``is not None`` distinction the cap-removal follow-up introduced —
|
||||||
|
a ``if max_models`` (falsy) check would conflate ``0`` with unlimited.
|
||||||
|
"""
|
||||||
|
import importlib
|
||||||
|
from hermes_cli import model_catalog
|
||||||
|
from hermes_cli.models import get_curated_nous_model_ids
|
||||||
|
importlib.reload(model_catalog)
|
||||||
|
try:
|
||||||
|
from hermes_cli.model_switch import (
|
||||||
|
list_authenticated_providers,
|
||||||
|
list_picker_providers,
|
||||||
|
)
|
||||||
|
|
||||||
|
active_home = Path(os.environ["HERMES_HOME"])
|
||||||
|
(active_home / "auth.json").write_text(
|
||||||
|
json.dumps(
|
||||||
|
{
|
||||||
|
"providers": {"nous": {"access_token": "fake"}},
|
||||||
|
"credential_pool": {},
|
||||||
|
}
|
||||||
|
)
|
||||||
|
)
|
||||||
|
with patch.object(
|
||||||
|
model_catalog, "_fetch_manifest", return_value=_valid_manifest()
|
||||||
|
), patch("hermes_cli.models.check_nous_free_tier", return_value=False), patch(
|
||||||
|
"hermes_cli.models.union_with_portal_free_recommendations",
|
||||||
|
side_effect=lambda ids, *a, **k: (ids, {}),
|
||||||
|
), patch(
|
||||||
|
"hermes_cli.models.union_with_portal_paid_recommendations",
|
||||||
|
side_effect=lambda ids, *a, **k: (ids, {}),
|
||||||
|
):
|
||||||
|
expected = get_curated_nous_model_ids()
|
||||||
|
full = list_picker_providers(current_provider="nous", max_models=None)
|
||||||
|
one = list_picker_providers(current_provider="nous", max_models=1)
|
||||||
|
# 0 is exercised on list_authenticated_providers (the slug-only
|
||||||
|
# path); the picker variant drops empty-model rows entirely, so
|
||||||
|
# the empty-list contract lives on the auth-providers call.
|
||||||
|
zero = list_authenticated_providers(
|
||||||
|
current_provider="nous", max_models=0
|
||||||
|
)
|
||||||
|
finally:
|
||||||
|
model_catalog.reset_cache()
|
||||||
|
|
||||||
|
def _nous(rows):
|
||||||
|
return next((r for r in rows if r["slug"] == "nous"), None)
|
||||||
|
|
||||||
|
# Only meaningful when the curated list actually exceeds 1 entry.
|
||||||
|
assert len(expected) > 1, "test needs a multi-model curated nous list"
|
||||||
|
|
||||||
|
full_row = _nous(full)
|
||||||
|
assert full_row is not None and full_row["models"] == expected
|
||||||
|
|
||||||
|
one_row = _nous(one)
|
||||||
|
assert one_row is not None and one_row["models"] == expected[:1]
|
||||||
|
|
||||||
|
zero_row = _nous(zero)
|
||||||
|
# 0 means an empty model list — NOT unlimited. total_models still real.
|
||||||
|
assert zero_row is not None
|
||||||
|
assert zero_row["models"] == []
|
||||||
|
assert zero_row["total_models"] == len(expected)
|
||||||
|
|
||||||
|
|
||||||
# -----------------------------------------------------------------------------
|
# -----------------------------------------------------------------------------
|
||||||
# Drift guard — prevent the in-repo curated lists from going out of sync with
|
# Drift guard — prevent the in-repo curated lists from going out of sync with
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user