diff --git a/CHANGELOG.md b/CHANGELOG.md index ebbba736..0c667863 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Fixed +- **Issue #2245** — Model picker provider lookup now canonicalizes configured provider keys before loading their configured models. Custom provider keys such as `CLIPpoxy` or `snake_case_provider` still display their configured model allowlists after canonicalization while preserving the original config key for provider settings. + - **PR #2244** by @franksong2702 (fixes #2243) — `Archive Session` no longer fails when the in-memory session cache contains a metadata-only stub for the target. Pre-fix, the route loaded via `get_session(sid)` which returned the cached `_loaded_metadata_only=True` instance, then `Session.save()` correctly refused to write because the metadata stub's `messages=[]` would have overwritten the full transcript (#1558 guard). Now the archive route reloads the full session from disk before mutating `archived` and refreshes the cache. Existing CLI/imported-session fallback unchanged. 47-line regression test pinning the route-level behaviour. - **PR #2249** by @franksong2702 (fixes #2248, follow-up to #2244) — Same metadata-only cache hit was happening at `/api/session/pin`, `/api/session/rename`, and `/api/personality/set`. Adds `_ensure_full_session_before_mutation()` helper in `api/routes.py` that reloads through `Session.load(sid)`, replaces the cached entry, preserves LRU ordering, and enforces `SESSIONS_MAX` eviction. Applied to all three routes. Parametrized regression coverage forces a metadata-only session into the cache for each route and verifies the saved session keeps its original messages while the cache is upgraded to a full session. (Archive Session in #2244 still uses an inline fix at the same site — a follow-up could refactor archive to use the helper too.) @@ -39,7 +41,6 @@ - **PR #2228** by @franksong2702 (refs #749) — Profile creation now exposes the same configured model/provider choices users already see in the composer/settings model picker. New profiles can be created with an explicit `model.default` and `model.provider` written into that profile's own `config.yaml`, while clone/base-url/API-key creation behavior remains unchanged. Backend validates the chosen model/provider before profile creation so invalid values do not leave a half-created profile on disk. Adds locale entries for English, Chinese, Japanese, Korean, Russian, and Spanish (parity-tested). 74-line regression test pinning the form, backend, and locale-key contract. ### Fixed - - **PR #2234** by @Jordan-SkyLF (refines #2207, original v0.51.61 portion) — Three update-banner improvements: (1) Update summaries no longer repeat the same bullet under both "What you'll notice" and "Worth knowing" — visible notice items keep priority, and the secondary section is omitted when there is no distinct detail to show. (2) Update summaries now cap large commit lists (24 + probe item) before sending them to the summarizer and disclose when the summary uses only the latest commit subjects, while keeping the full comparison link available — bounds summarizer cost on large update ranges while remaining honest about coverage. (3) Update banners now wrap generated-summary links and long update text on narrow/mobile screens inside the banner instead of pushing controls off-screen. 108-line regression coverage for short-target dedup, repeated Agent-summary bullets, large-range capped input, and responsive wrapping. (A follow-up commit pushed AFTER stage-354 merged is now shipped in stage-355.) - **PR #2236** by @jasonjcwu — Silent failure detection in `api/streaming.py` now scans only NEW messages, not the full conversation history. Pre-fix, the `_assistant_added` check at `_run_agent_streaming` scanned all messages in `result["messages"]` (including pre-turn history); if any prior turn contained an assistant response, `_assistant_added` was `True` and the apperror SSE event was silently skipped, leaving the user staring at a blank response after a provider 401/429/rate-limit error. Fix extracts a `_has_new_assistant_reply(all_messages, prev_count)` helper that only inspects messages beyond the pre-turn history offset (`_previous_context_messages`); applied to both the main detection path and the self-heal/retry `_heal_ok` check. 15-test regression suite covering empty/short/long-history scenarios, the heal path, and the `len < prev_count` edge-case fallback. Also includes a small alignment fix to `test_issue1857_usage_overwrite.py` so the FakeAgent message shape matches what the real agent produces. diff --git a/api/config.py b/api/config.py index d0c047bc..7d35977a 100644 --- a/api/config.py +++ b/api/config.py @@ -2872,11 +2872,16 @@ def get_available_models() -> dict: # The same applies to mixed-case ids like ``OpenCode-Go`` and to # legitimate aliases like ``z-ai`` → ``zai``. _cfg_providers = cfg.get("providers", {}) + # Map canonical provider IDs back to raw config keys so the + # generic-provider branch can preserve mixed-case/underscore + # provider_cfg values (#2245). + _canonical_to_raw_provider_key: dict[str, str] = {} if isinstance(_cfg_providers, dict): for _pid_key in _cfg_providers: _canonical = _canonicalise_provider_id(_pid_key) if not _canonical: continue + _canonical_to_raw_provider_key.setdefault(_canonical, _pid_key) if _canonical in _PROVIDER_MODELS or _canonical in _cfg_providers or _pid_key in _cfg_providers: detected_providers.add(_canonical) @@ -3502,8 +3507,14 @@ def get_available_models() -> dict: "models": models, } ) - elif pid in _PROVIDER_MODELS or pid in cfg.get("providers", {}): - provider_cfg = cfg.get("providers", {}).get(pid, {}) + elif pid in _PROVIDER_MODELS or pid in _canonical_to_raw_provider_key: + # Look up provider_cfg using the original raw key from + # config.yaml so that mixed-case / underscore keys like + # ``CLIPpoxy`` or ``snake_case_provider`` still resolve + # (#2245). Fall back to the canonical pid for providers + # that appear in _PROVIDER_MODELS but not in cfg. + _raw_key = _canonical_to_raw_provider_key.get(pid, pid) + provider_cfg = cfg.get("providers", {}).get(_raw_key, {}) raw_models = [] # User-configured model allowlists are explicit local diff --git a/tests/test_issue2245_mixed_case_provider_models.py b/tests/test_issue2245_mixed_case_provider_models.py new file mode 100644 index 00000000..8d22beb8 --- /dev/null +++ b/tests/test_issue2245_mixed_case_provider_models.py @@ -0,0 +1,242 @@ +"""Regression tests for #2245 — mixed-case custom provider keys lose picker models. + +When a user configures a provider key in ``config.yaml`` with mixed case +(e.g. ``CLIPpoxy``) or underscores (e.g. ``snake_case_provider``), the +WebUI model picker must still surface that provider's configured models. + +Root cause: ``_build_available_models_uncached()`` iterates over +*canonicalised* provider IDs (lowercase, hyphens) but looked up +``cfg["providers"]`` using the canonical key — which doesn't match the +raw mixed-case/underscore key in the config dict. The fix adds a +``_canonical_to_raw_provider_key`` map so the generic-provider branch +can resolve the original key and load ``provider_cfg`` correctly. +""" + +from __future__ import annotations + +import sys +import types + +import pytest + +import api.config as config + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + +@pytest.fixture(autouse=True) +def _isolate_models_cache(tmp_path, monkeypatch): + """Invalidate the models TTL cache before and after every test.""" + monkeypatch.setattr(config, "_models_cache_path", tmp_path / "models_cache.json") + config.invalidate_models_cache() + yield + config.invalidate_models_cache() + + +def _stub_hermes_cli(monkeypatch): + """Stub hermes_cli so no real CLI/agent calls happen.""" + fake_models = types.ModuleType("hermes_cli.models") + fake_models.list_available_providers = lambda: [] + fake_models.provider_model_ids = lambda _pid: [] + fake_auth = types.ModuleType("hermes_cli.auth") + fake_auth.get_auth_status = lambda _pid: {"key_source": "none"} + monkeypatch.setitem(sys.modules, "hermes_cli.models", fake_models) + monkeypatch.setitem(sys.modules, "hermes_cli.auth", fake_auth) + monkeypatch.setattr( + config, + "_get_auth_store_path", + lambda: config.Path("/tmp/does-not-exist-auth.json"), + ) + + +def _with_config(cfg_dict: dict): + """Replace ``config.cfg`` with *cfg_dict* and return a restore callable.""" + old_cfg = dict(config.cfg) + old_mtime = config._cfg_mtime + config.cfg.clear() + config.cfg.update(cfg_dict) + try: + config._cfg_mtime = config.Path(config._get_config_path()).stat().st_mtime + except Exception: + config._cfg_mtime = 0.0 + + def restore(): + config.cfg.clear() + config.cfg.update(old_cfg) + config._cfg_mtime = old_mtime + config.invalidate_models_cache() + + return restore + + +# --------------------------------------------------------------------------- +# Test case 1 — mixed-case provider key +# --------------------------------------------------------------------------- + +def test_mixed_case_provider_key_produces_configured_models(monkeypatch): + """A provider key like ``CLIPpoxy`` must feed its configured models into + the picker after canonicalisation to ``clippoxy``.""" + _stub_hermes_cli(monkeypatch) + monkeypatch.setattr("socket.getaddrinfo", lambda *a, **k: []) + + restore = _with_config( + { + "model": { + "default": "my-model", + "provider": "CLIPpoxy", + }, + "providers": { + "CLIPpoxy": { + "models": ["my-model", "another-model"], + }, + }, + } + ) + try: + result = config.get_available_models() + finally: + restore() + + # Find the group for the canonicalised provider id "clippoxy" + groups_by_pid = {g["provider_id"]: g for g in result["groups"]} + assert "clippoxy" in groups_by_pid, ( + f"Expected canonical provider id 'clippoxy' in groups, " + f"got: {list(groups_by_pid.keys())}" + ) + group = groups_by_pid["clippoxy"] + model_ids = [m["id"] for m in group["models"]] + # Both configured models must appear (possibly with @clippoxy: prefix) + # Strip any @provider: prefix for comparison + bare_ids = [] + for mid in model_ids: + if mid.startswith("@"): + bare_ids.append(mid.split(":", 1)[-1] if ":" in mid else mid) + else: + bare_ids.append(mid) + assert "my-model" in bare_ids, ( + f"Expected 'my-model' in model ids for clippoxy, got: {bare_ids}" + ) + assert "another-model" in bare_ids, ( + f"Expected 'another-model' in model ids for clippoxy, got: {bare_ids}" + ) + + +# --------------------------------------------------------------------------- +# Test case 2 — underscore provider key +# --------------------------------------------------------------------------- + +def test_underscore_provider_key_produces_configured_models(monkeypatch): + """A provider key like ``snake_case_provider`` must canonicalise to + ``snake-case-provider`` and still surface its configured models.""" + _stub_hermes_cli(monkeypatch) + monkeypatch.setattr("socket.getaddrinfo", lambda *a, **k: []) + + restore = _with_config( + { + "model": { + "default": "model-a", + "provider": "snake_case_provider", + }, + "providers": { + "snake_case_provider": { + "models": ["model-a", "model-b"], + }, + }, + } + ) + try: + result = config.get_available_models() + finally: + restore() + + groups_by_pid = {g["provider_id"]: g for g in result["groups"]} + canonical_pid = "snake-case-provider" + assert canonical_pid in groups_by_pid, ( + f"Expected canonical provider id '{canonical_pid}' in groups, " + f"got: {list(groups_by_pid.keys())}" + ) + group = groups_by_pid[canonical_pid] + model_ids = [m["id"] for m in group["models"]] + bare_ids = [] + for mid in model_ids: + if mid.startswith("@"): + bare_ids.append(mid.split(":", 1)[-1] if ":" in mid else mid) + else: + bare_ids.append(mid) + assert "model-a" in bare_ids, ( + f"Expected 'model-a' in model ids for {canonical_pid}, got: {bare_ids}" + ) + assert "model-b" in bare_ids, ( + f"Expected 'model-b' in model ids for {canonical_pid}, got: {bare_ids}" + ) + + +# --------------------------------------------------------------------------- +# Test case 3 — built-in provider with lowercase key still works +# --------------------------------------------------------------------------- + +def test_builtin_provider_still_resolves(monkeypatch): + """A built-in provider like ``anthropic`` must still resolve through the + same branch without regression.""" + _stub_hermes_cli(monkeypatch) + monkeypatch.setattr("socket.getaddrinfo", lambda *a, **k: []) + + restore = _with_config( + { + "model": { + "default": "claude-sonnet-4-5", + "provider": "anthropic", + }, + "providers": { + "anthropic": { + "api_key": "sk-test-key", + }, + }, + } + ) + try: + result = config.get_available_models() + finally: + restore() + + groups_by_pid = {g["provider_id"]: g for g in result["groups"]} + assert "anthropic" in groups_by_pid, ( + f"Expected 'anthropic' in groups, got: {list(groups_by_pid.keys())}" + ) + # Should have at least one model (from _PROVIDER_MODELS fallback) + group = groups_by_pid["anthropic"] + assert len(group["models"]) > 0, "anthropic group should have models" + + +# --------------------------------------------------------------------------- +# Test case 4 — _PROVIDER_MODELS fallback still works when no cfg key +# --------------------------------------------------------------------------- + +def test_provider_models_fallback_when_no_config_key(monkeypatch): + """A provider in _PROVIDER_MODELS but NOT in cfg["providers"] must + still fall back to the static model list.""" + _stub_hermes_cli(monkeypatch) + monkeypatch.setattr("socket.getaddrinfo", lambda *a, **k: []) + + restore = _with_config( + { + "model": { + "default": "deepseek-chat", + "provider": "deepseek", + }, + # No providers section at all + } + ) + try: + result = config.get_available_models() + finally: + restore() + + groups_by_pid = {g["provider_id"]: g for g in result["groups"]} + assert "deepseek" in groups_by_pid, ( + f"Expected 'deepseek' in groups, got: {list(groups_by_pid.keys())}" + ) + group = groups_by_pid["deepseek"] + assert len(group["models"]) > 0, "deepseek group should have models from _PROVIDER_MODELS" \ No newline at end of file