diff --git a/CHANGELOG.md b/CHANGELOG.md index 6aed2bcb..9bfe9e8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,8 @@ - **The reasoning-effort chip is correct right after an empty startup.** On a normal boot with no active session restored, the top-bar reasoning chip wasn't refreshed for the hydrated default model, so it could show a stale value until you changed the model. It now refreshes on empty boot too. Thanks @rodboev. (#5967, #5954) +- **Custom/proxy providers keep the full vendor namespace on the model id (P0 routing fix).** When you run multiple upstream vendors behind one OpenAI-compatible proxy (`provider: custom` / `custom:`) and pick a vendor-namespaced model such as `x-ai/grok-4.5`, the WebUI was stripping the `x-ai/` namespace before sending, so the proxy received a bare `grok-4.5` it couldn't route and rejected the turn with `HTTP 400: Invalid model format`. The strip was a catalog-*family* guess that couldn't tell a namespace the proxy actually routes on (`x-ai/grok-4.5`, keep) apart from a stale cross-provider leftover on a relay that only serves bare ids (`openai/gpt-5.4` → `gpt-5.4`, strip). Resolution is now based on **what the endpoint actually advertised**: if the proxy's own catalog lists the full `vendor/model`, it is sent whole; if it lists only the bare id, the redundant prefix is stripped; and on a cold/unbuilt catalog the id is preserved verbatim (fail-safe toward the id you selected). First-party providers pointed at an OpenAI-compatible proxy still send the bare id as before. Regression was latent — it surfaced only once a model graduated into the first-party catalog. Thanks @b3nw for the report and root-cause analysis. (#5979) + - **Transparent Stream no longer bloats memory (or freezes the tab) on long tool-heavy chats.** With Transparent Stream enabled, opening a conversation with many prompts built a DOM node for every activity event of every past turn at load and eagerly rendered each tool row's full detail body — a long reasoning-heavy history could balloon to multiple GB and lock up the tab. Settled tool rows now render header-only and build their detail on first expand (identical to the old view once opened), and a turn with a very large number of steps shows its most recent 30 behind a "Show N earlier steps" control. Live turns are unchanged. Thanks @curtisszmania-cytocync for the report. (#5974, #5966) - **Dictation no longer wipes text you already typed.** When you typed part of a prompt and then used the microphone (server-side transcription / MediaRecorder path), the recognized text replaced your typed prefix instead of being appended to it. Dictation now appends to whatever is already in the composer, so you can type a partial sentence and dictate the rest. A Settings toggle lets you opt back into the old replace behavior. Thanks @Kopamed. (#5895) diff --git a/api/config.py b/api/config.py index 44e44463..8e6ac076 100644 --- a/api/config.py +++ b/api/config.py @@ -2354,6 +2354,57 @@ def _is_local_server_provider(provider_id: str) -> bool: return False +def _model_id_declared_in_config(model_id: str, config_provider: str | None) -> bool: + """True when the user's own config declares ``model_id`` verbatim (full form). + + This is the COLD-catalog provenance signal for #5979: when the live + ``/v1/models`` catalog is unbuilt (fresh process, headless client), a + vendor-namespaced id the user configured — ``model.default``, the + ``model.models`` allowlist, or the matching ``custom_providers[].models`` / + ``.model`` for a named ``custom:`` — is still authoritative provenance + that the full id is intentional and must be preserved. Config is the one + source available with zero network and no catalog dependency, so it survives + a cold restart (b3nw's ``model.default: x-ai/grok-4.5``). Checked ONLY for + custom providers; returns False for anything not verbatim-declared so the + caller falls through to the legacy family heuristic. + """ + model = str(model_id or "").strip() + if not model: + return False + model_cfg = cfg.get("model", {}) + if isinstance(model_cfg, dict): + if str(model_cfg.get("default") or "").strip() == model: + return True + _declared = model_cfg.get("models") + if isinstance(_declared, dict) and model in _declared: + return True + if isinstance(_declared, list) and any( + (str(m.get("id") or "").strip() if isinstance(m, dict) else str(m or "").strip()) == model + for m in _declared + ): + return True + # Named custom: — scan its custom_providers[] entry for a verbatim id. + prov = str(config_provider or "").strip().lower() + if prov.startswith("custom:"): + raw_suffix = prov.removeprefix("custom:") + for entry in _custom_provider_entries(): + slug = _custom_provider_slug_from_name(entry.get("name")) + entry_name = str(entry.get("name") or "").strip().lower() + if not (prov in {entry_name, slug} or (slug and raw_suffix == slug.removeprefix("custom:"))): + continue + if str(entry.get("model") or "").strip() == model: + return True + _em = entry.get("models") + if isinstance(_em, dict) and model in _em: + return True + if isinstance(_em, list) and any( + (str(m.get("id") or "").strip() if isinstance(m, dict) else str(m or "").strip()) == model + for m in _em + ): + return True + return False + + def _is_first_party_model(provider_id: str, model_id: str) -> bool: """True when ``model_id`` is listed in ``provider_id``'s own static catalog. @@ -2808,9 +2859,59 @@ def resolve_model_provider(model_id: str) -> tuple: # earlier by the ``prefix == config_provider`` branch. _cp_lower = (config_provider or "").strip().lower() _is_custom = _cp_lower == "custom" or _cp_lower.startswith("custom:") - if prefix in _PROVIDER_MODELS and ( - not _is_custom or _is_first_party_model(prefix, bare) - ): + if _is_custom: + # Vendor-routing proxy: the reliable signal for whether the + # endpoint wants the full ``vendor/model`` id or the bare id is + # what its own catalog actually advertised (the ids the user + # picked from the dropdown, populated by the endpoint's live + # ``/v1/models`` probe or a ``custom_providers[].models`` + # allowlist). The catalog-FAMILY heuristic (_is_first_party_model) + # is the wrong question: it answered "is this bare id a first- + # party model of the prefix's home vendor?" which is True for BOTH + # ``x-ai/grok-4.5`` (proxy advertised it whole — must preserve, + # #5979) and ``openai/gpt-5.4`` (a stale leftover on a relay that + # only serves bare ``gpt-5.4`` — must strip, #433). Those two are + # structurally identical to the family heuristic, so a model + # graduating into a first-party catalog (agent commit 62ada5175 + # adding grok-4.5) silently flipped a working custom-proxy id from + # preserved to stripped. Tri-state provenance tells them apart: + # + # (1) Config declares the full id verbatim (model.default / + # model.models / custom_providers[].models). Authoritative and + # network-free, so #5979 survives a cold restart — preserve. + if _model_id_declared_in_config(model_id, config_provider): + return model_id, config_provider, config_base_url + # (2) The endpoint's live/cached catalog advertised it. + _advertised = _endpoint_advertised_model_ids(config_provider) + if _advertised: + # Full id advertised → route on it verbatim (#5979/#3872/#548). + if model_id in _advertised: + return model_id, config_provider, config_base_url + # ONLY the bare id advertised → the prefix is a redundant + # leftover the relay rejects; strip it (#433). Keep the + # ``prefix in _PROVIDER_MODELS`` belt so an adversarial catalog + # advertising a bare id can't strip an unknown-vendor prefix. + if bare in _advertised and prefix in _PROVIDER_MODELS: + return bare, config_provider, config_base_url + # Advertised but neither exact shape matched → intrinsic / + # unknown prefix the proxy routes on; preserve it whole. + return model_id, config_provider, config_base_url + # (3) Provenance genuinely unavailable (cold/unbuilt or + # fingerprint-mismatched catalog AND not config-declared). + # Fall back to the LEGACY family heuristic so this narrow edge + # is never WORSE than the pre-fix behaviour: a first-party + # redundant prefix still strips (#433 relay works cold), while + # an intrinsic/unknown prefix (bedrock/opus-4-6 #3872, + # zai-org/GLM-5.1 #548) is preserved. The #5979 active-user + # path can't reach here — a selected id is either config- + # declared or in the catalog the dropdown was built from. + if prefix in _PROVIDER_MODELS and _is_first_party_model(prefix, bare): + return bare, config_provider, config_base_url + return model_id, config_provider, config_base_url + # Non-custom first-party provider pointed at an OpenAI-compatible + # proxy (e.g. provider=openai + base_url=litellm): the bare id is + # what it expects — "openai/gpt-5.4" → "gpt-5.4" (#433). + if prefix in _PROVIDER_MODELS: return bare, config_provider, config_base_url # Intrinsic / unknown prefix — pass the full model_id through unchanged. return model_id, config_provider, config_base_url @@ -4511,6 +4612,121 @@ _available_models_cache_lock = threading.RLock() # must be RLock: cold path ref _cache_build_cv = threading.Condition(_available_models_cache_lock) # shares underlying RLock so notify_all() is safe inside with _available_models_cache_lock _cache_build_in_progress = False # True while a cold path is actively building +# Memoized (snapshot_ref, {provider_slug: frozenset(model_ids)}) derived from +# the published models-catalog snapshot. Used by _endpoint_advertised_model_ids +# to answer "did this endpoint actually advertise this exact id?" in O(1) per +# send without rebuilding. Keyed on the snapshot object identity so it is +# recomputed exactly once per catalog publish (the cache is replaced wholesale, +# never mutated) and can never serve stale ids from a superseded catalog. +_advertised_model_ids_memo: tuple | None = None + +# Atomic provenance pair: an immutable (snapshot, publisher_fingerprint) tuple +# published together at every catalog publish/invalidate site via +# _sync_models_cache_provenance(). The resolver reads THIS single global with one +# lock-free load so it can never observe a torn snapshot/fingerprint pair (the +# two underlying globals are assigned as separate statements). Reading a tuple is +# atomic under the GIL and, crucially, acquires NO lock — so the per-send +# provenance check introduces no lock-ordering edge (avoids the _cfg_lock ↔ +# _available_models_cache_lock deadlock) and never waits behind a catalog rebuild. +_models_cache_provenance: tuple | None = None + + +def _sync_models_cache_provenance() -> None: + """Republish the atomic (snapshot, fingerprint) provenance pair. + + MUST be called at every site that assigns ``_available_models_cache`` and + ``_available_models_cache_source_fingerprint`` (publish and invalidate), + AFTER both have been set. It snapshots the current pair into one immutable + tuple so ``_endpoint_advertised_model_ids`` reads both consistently with a + single lock-free load. A reader that races between an underlying assignment + and this call sees the PREVIOUS consistent tuple (never a torn pair); once + this runs, readers see the new consistent pair. + """ + global _models_cache_provenance + snap = _available_models_cache + _models_cache_provenance = ( + (snap, _available_models_cache_source_fingerprint) if snap is not None else None + ) + + +def _endpoint_advertised_model_ids(provider_id: str | None) -> frozenset | None: + """Model ids the given provider's group advertised in the current catalog. + + Reads ONLY the already-published in-memory catalog snapshot + (``_available_models_cache``) — it never builds, live-probes, or touches + disk, so it is safe to call on the per-turn send hot path. Returns: + + * a ``frozenset`` of the ids advertised by ``provider_id``'s own group + (bare ids for the active provider, e.g. ``x-ai/grok-4.5``), or + * ``None`` when the catalog is cold/unbuilt OR the provider has no group. + + ``None`` means "no provenance signal available" — callers MUST treat that as + "preserve the model id verbatim" so a cache miss never silently strips a + vendor namespace off an id the user actively selected (#5979). Scoping to + the provider's OWN group prevents a same-named id in a sibling group (e.g. + an ``openai/gpt-5.4`` sitting in the OpenRouter group) from masquerading as + something this custom endpoint advertised. + """ + global _advertised_model_ids_memo + # Single lock-free atomic read of the immutable (snapshot, fingerprint) pair + # published by _sync_models_cache_provenance(). Reading one tuple can never + # tear, and acquiring no lock means this per-send check adds no lock-ordering + # edge (no _cfg_lock ↔ _available_models_cache_lock deadlock) and never waits + # behind a catalog rebuild. + provenance = _models_cache_provenance + if provenance is None: + return None + snapshot, published_fp = provenance + if snapshot is None: + return None + # Profile-isolation fail-safe (profiles are islands): the catalog cache is a + # process global, so a concurrently-active profile could have published the + # snapshot we're now reading. Only trust it for provenance when the + # fingerprint captured AT PUBLISH TIME still matches the current runtime + # fingerprint — the ``config_yaml`` axis of that fingerprint is the + # PROFILE-SPECIFIC config path (_get_config_path -> get_active_hermes_home), + # so a match guarantees the snapshot belongs to the profile asking. Any + # mismatch (foreign profile, config edit, stale) returns None so the caller + # preserves the id verbatim rather than stripping against another profile's + # catalog. + try: + if published_fp != _models_cache_source_fingerprint(): + return None + except Exception: + return None # fingerprint unavailable → no trustworthy provenance + memo = _advertised_model_ids_memo + # Identity check (``is``), not id(): holding the snapshot reference in the + # memo keeps it alive, so a freed-then-reused id() can't cause a false hit. + if memo is None or memo[0] is not snapshot: + by_slug: dict[str, frozenset] = {} + try: + groups = snapshot.get("groups", []) or [] + except AttributeError: + return None + for group in groups: + if not isinstance(group, dict): + continue + slug = str(group.get("provider_id") or "").strip().lower() + if not slug: + continue + # Union BOTH catalog buckets: a provider's models can be split across + # ``models`` (visible) and ``extra_models`` (overflow) by the picker, + # so an id the endpoint genuinely advertised may live in either. Only + # reading ``models`` would miss it and mis-resolve (e.g. leave the + # #433 bare id unstripped because it sits in extra_models). + ids = frozenset( + str(m.get("id")) + for bucket in ("models", "extra_models") + for m in (group.get(bucket) or []) + if isinstance(m, dict) and m.get("id") + ) + by_slug[slug] = by_slug.get(slug, frozenset()) | ids + memo = (snapshot, by_slug) + _advertised_model_ids_memo = memo + slug = str(provider_id or "").strip().lower() + return memo[1].get(slug) + + # Hard wall-clock budget for a COLD live provider-catalog rebuild when it is # run from a foreground request path. The live rebuild does one network probe # per detected provider (Copilot token-exchange HTTPS, OpenRouter /v1/models, @@ -5817,6 +6033,7 @@ def _get_fresh_memory_models_cache(now: float) -> dict | None: _available_models_cache_ts = 0.0 _available_models_live_rebuild_ts = 0.0 _available_models_cache_source_fingerprint = None + _sync_models_cache_provenance() return None if _is_valid_models_cache(_available_models_cache): return _annotate_fast_tier_model_groups(copy.deepcopy(_available_models_cache)) @@ -5824,6 +6041,7 @@ def _get_fresh_memory_models_cache(now: float) -> dict | None: _available_models_cache_ts = 0.0 _available_models_live_rebuild_ts = 0.0 _available_models_cache_source_fingerprint = None + _sync_models_cache_provenance() return None @@ -5848,6 +6066,7 @@ def invalidate_models_cache(): _available_models_cache_ts = 0.0 _available_models_live_rebuild_ts = 0.0 _available_models_cache_source_fingerprint = None + _sync_models_cache_provenance() _cache_build_in_progress = False _cache_build_cv.notify_all() # Clear the credential pool cache too (all profiles). Without this, @@ -5907,6 +6126,7 @@ def invalidate_provider_models_cache(provider_id: str): _available_models_cache_ts = 0.0 _available_models_live_rebuild_ts = 0.0 _available_models_cache_source_fingerprint = None + _sync_models_cache_provenance() _provider_models_invalidated_ts[provider_id] = time.time() # Also evict the credential pool so the next cold path re-loads it. # Must evict both the original key and its canonical form (load_pool @@ -7654,6 +7874,7 @@ def get_available_models(*, prefer_cache: bool = False, force_refresh: bool = Fa _available_models_cache_ts = 0.0 _available_models_live_rebuild_ts = 0.0 _available_models_cache_source_fingerprint = None + _sync_models_cache_provenance() disk_groups = None stale_disk_groups = None @@ -7704,6 +7925,7 @@ def get_available_models(*, prefer_cache: bool = False, force_refresh: bool = Fa _available_models_cache = disk_groups _available_models_cache_ts = now _available_models_cache_source_fingerprint = _models_cache_source_fingerprint() + _sync_models_cache_provenance() return copy.deepcopy(disk_groups) # ── prefer_cache: NEVER run the live provider rebuild ──────────────── @@ -7780,6 +8002,7 @@ def get_available_models(*, prefer_cache: bool = False, force_refresh: bool = Fa _available_models_cache_ts = published_at _available_models_live_rebuild_ts = published_at _available_models_cache_source_fingerprint = _models_cache_source_fingerprint() + _sync_models_cache_provenance() try: _save_models_cache_to_disk(result) finally: @@ -7829,6 +8052,7 @@ def get_available_models(*, prefer_cache: bool = False, force_refresh: bool = Fa _available_models_cache_source_fingerprint = ( _models_cache_source_fingerprint() ) + _sync_models_cache_provenance() try: _save_models_cache_to_disk(result) except Exception: @@ -7993,6 +8217,7 @@ def get_available_models_for_session_visit() -> dict: _available_models_cache = copy.deepcopy(disk_cached) _available_models_cache_ts = time.monotonic() _available_models_cache_source_fingerprint = _models_cache_source_fingerprint() + _sync_models_cache_provenance() _mark("disk_cache_returned") _maybe_log_slow_stages(_logger, _stagelog, _slow_threshold_ms, "models.session_visit") return copy.deepcopy(disk_cached) diff --git a/tests/test_model_resolver.py b/tests/test_model_resolver.py index 0f6f5ee5..5c67d1e8 100644 --- a/tests/test_model_resolver.py +++ b/tests/test_model_resolver.py @@ -27,6 +27,44 @@ def _resolve_with_config(model_id, provider=None, base_url=None, default=None, c config.cfg.update(old_cfg) +def _resolve_with_catalog(model_id, advertised_ids, *, provider=None, base_url=None, + provider_id='custom', default=None): + """Resolve with a seeded models-catalog snapshot (#5979 provenance). + + ``advertised_ids`` is the list of model ids the endpoint's own group + advertised (what the user could have picked from the dropdown). Pass ``None`` + to simulate a COLD/unbuilt catalog. Seeds ``config._available_models_cache`` + (the snapshot ``_endpoint_advertised_model_ids`` reads) for the duration of + the call, resets the derivation memo, and restores both afterwards so tests + don't leak catalog state into each other. + """ + old_cache = config._available_models_cache + old_memo = config._advertised_model_ids_memo + old_fp = config._available_models_cache_source_fingerprint + old_prov = config._models_cache_provenance + if advertised_ids is None: + config._available_models_cache = None + else: + config._available_models_cache = { + 'groups': [{ + 'provider_id': provider_id, + 'models': [{'id': mid, 'label': mid} for mid in advertised_ids], + }] + } + # Stamp the source fingerprint exactly as the real publish sites do, so + # the accessor's profile-isolation guard trusts this seeded snapshot. + config._available_models_cache_source_fingerprint = config._models_cache_source_fingerprint() + config._advertised_model_ids_memo = None # force recompute against the seeded snapshot + config._sync_models_cache_provenance() # publish the atomic (snapshot, fingerprint) pair + try: + return _resolve_with_config(model_id, provider=provider, base_url=base_url, default=default) + finally: + config._available_models_cache = old_cache + config._advertised_model_ids_memo = old_memo + config._available_models_cache_source_fingerprint = old_fp + config._models_cache_provenance = old_prov + + # ── OpenRouter prefix handling ──────────────────────────────────────────── def test_openrouter_free_keeps_full_path(): @@ -170,10 +208,12 @@ def test_custom_remote_preserves_intrinsic_vendor_prefix_3872(): A bare ``custom`` provider with a remote base_url is a vendor-routing proxy (LiteLLM, Bedrock gateway). ``bedrock/`` is an intrinsic routing segment the proxy needs whole; stripping it to ``opus-4-6`` makes the proxy return 403 - "model not allowed for your group". + "model not allowed for your group". The proxy advertised the full id (the + user picked it from the dropdown), so provenance preserves it. """ - model, provider, base_url = _resolve_with_config( + model, provider, base_url = _resolve_with_catalog( 'bedrock/opus-4-6', + advertised_ids=['bedrock/opus-4-6'], provider='custom', base_url='https://router.example.com/v1', ) @@ -183,15 +223,20 @@ def test_custom_remote_preserves_intrinsic_vendor_prefix_3872(): def test_custom_remote_strips_redundant_first_party_prefix_433(): - """#433: bare-custom remote proxy still strips a REDUNDANT first-party prefix. + """#433: bare-custom remote proxy strips a prefix ONLY when the endpoint + advertised just the BARE id (not the full ``vendor/model``). - ``gpt-5.4`` IS a first-party OpenAI model, so ``openai/`` is a redundant - leftover and the proxy expects the bare id. This is the behaviour pinned by - test_sprint40_ui_polish.py::test_prefixed_model_stripped_for_custom_endpoint; - the #3872 fix must keep it while preserving intrinsic vendor prefixes. + #433 is a verified real relay whose ``/v1/models`` returned bare ``gpt-5.4`` + and rejected ``openai/gpt-5.4`` (a stale cross-provider leftover). The strip + is now justified by PROVENANCE — the catalog advertises exactly ``gpt-5.4`` + and NOT ``openai/gpt-5.4`` — instead of the old catalog-family guess that + couldn't tell this stale-leftover case apart from #5979's advertised-full-id + case. Behaviour is also pinned by + test_sprint40_ui_polish.py::test_prefixed_model_stripped_for_custom_endpoint. """ - model, provider, base_url = _resolve_with_config( + model, provider, base_url = _resolve_with_catalog( 'openai/gpt-5.4', + advertised_ids=['gpt-5.4'], # relay advertises ONLY the bare id provider='custom', base_url='https://router.example.com/v1', ) @@ -199,10 +244,255 @@ def test_custom_remote_strips_redundant_first_party_prefix_433(): assert provider == 'custom' +def test_custom_remote_preserves_advertised_full_id_5979(): + """#5979 (the P0 regression): a custom proxy that advertises the FULL + ``x-ai/grok-4.5`` must receive it whole — never the bare ``grok-4.5``. + + The old code stripped it because ``grok-4.5`` had graduated into the x-ai + first-party catalog (agent commit 62ada5175), flipping ``_is_first_party_model`` + to True for a model the proxy routes on by its full ``x-ai/`` namespace. This + is the exact HTTP 400 b3nw hit ("Invalid model format ... grok-4.5"). The + provenance rule preserves it because the endpoint advertised the full id. + """ + # Deterministically reproduce the data-driven trigger regardless of the + # hermes-agent catalog version CI happens to run against: ensure grok-4.5 is + # first-party of x-ai so _is_first_party_model('x-ai','grok-4.5') is True (the + # condition under which the OLD code stripped). We stub it into the catalog + # rather than asserting the live catalog already contains it. + xai_catalog = list(config._PROVIDER_MODELS.get('x-ai') or []) + had_grok = any(isinstance(m, dict) and m.get('id') == 'grok-4.5' for m in xai_catalog) + old_xai = config._PROVIDER_MODELS.get('x-ai') + if not had_grok: + config._PROVIDER_MODELS['x-ai'] = xai_catalog + [{'id': 'grok-4.5', 'label': 'Grok 4.5'}] + try: + assert config._is_first_party_model('x-ai', 'grok-4.5'), ( + "precondition: grok-4.5 must be first-party of x-ai for this regression" + ) + model, provider, base_url = _resolve_with_catalog( + 'x-ai/grok-4.5', + advertised_ids=['x-ai/grok-4.5'], # proxy advertises the FULL namespaced id + provider='custom', + base_url='https://proxy.example.com/v1', + ) + finally: + if not had_grok: + if old_xai is None: + config._PROVIDER_MODELS.pop('x-ai', None) + else: + config._PROVIDER_MODELS['x-ai'] = old_xai + assert model == 'x-ai/grok-4.5', ( + f"advertised full id must be preserved for routing, got {model!r}" + ) + assert provider == 'custom' + assert base_url == 'https://proxy.example.com/v1' + + +def test_named_custom_slug_preserves_advertised_full_id_5979(): + """#5979 (named-custom variant): provider=custom: proxy advertising the + full ``x-ai/grok-4.5`` also preserves it.""" + model, provider, base_url = _resolve_with_catalog( + 'x-ai/grok-4.5', + advertised_ids=['x-ai/grok-4.5'], + provider='custom:my-gateway', + provider_id='custom:my-gateway', + base_url='https://proxy.example.com/v1', + ) + assert model == 'x-ai/grok-4.5', f"full id must be preserved for custom:slug, got {model!r}" + + +def test_custom_remote_cold_catalog_falls_back_to_legacy_heuristic_5979(): + """#5979 tri-state: with a COLD/unbuilt catalog AND no config declaration + (no provenance at all), resolution falls back to the LEGACY family heuristic + so this narrow edge is never worse than the pre-fix behaviour. + + A first-party redundant prefix still strips (the #433 relay keeps working + cold), while an intrinsic/unknown prefix is preserved. The #5979 active-user + path never reaches this branch — a selected id is either config-declared + (see the config-declared test) or in the catalog the dropdown was built from. + """ + # first-party redundant prefix → strip (matches pre-fix / #433 cold path) + model_a, _, _ = _resolve_with_catalog( + 'openai/gpt-5.4', advertised_ids=None, + provider='custom', base_url='https://router.example.com/v1', + ) + assert model_a == 'gpt-5.4', f"cold first-party prefix must strip (legacy), got {model_a!r}" + # intrinsic/unknown prefix → preserve + model_b, _, _ = _resolve_with_catalog( + 'zai-org/GLM-5.1', advertised_ids=None, + provider='custom', base_url='https://api.deepinfra.com/v1/openai', + ) + assert model_b == 'zai-org/GLM-5.1', f"cold unknown prefix must preserve, got {model_b!r}" + + +def test_custom_remote_config_declared_full_id_preserved_cold_5979(): + """#5979 cold-restart survival: even with a COLD catalog, a full vendor id + the user DECLARED in config (model.default) is preserved — config is + network-free provenance that outlives a process restart. + """ + old_cache = config._available_models_cache + old_memo = config._advertised_model_ids_memo + old_fp = config._available_models_cache_source_fingerprint + old_prov = config._models_cache_provenance + old_cfg = dict(config.cfg) + config._available_models_cache = None # cold + config._advertised_model_ids_memo = None + config._sync_models_cache_provenance() # publish the cold (None) provenance + config.cfg['model'] = { + 'provider': 'custom', + 'default': 'x-ai/grok-4.5', # user-declared full id + 'base_url': 'https://proxy.example.com/v1', + } + try: + model, provider, _ = config.resolve_model_provider('x-ai/grok-4.5') + finally: + config._available_models_cache = old_cache + config._advertised_model_ids_memo = old_memo + config._available_models_cache_source_fingerprint = old_fp + config._models_cache_provenance = old_prov + config.cfg.clear() + config.cfg.update(old_cfg) + assert model == 'x-ai/grok-4.5', ( + f"config-declared full id must survive a cold catalog, got {model!r}" + ) + assert provider == 'custom' + + +def test_custom_remote_prefers_full_id_when_both_advertised_5979(): + """When a proxy advertises BOTH the full ``x-ai/grok-4.5`` and a bare + ``grok-4.5``, the exact full selection wins (preserve).""" + model, _, _ = _resolve_with_catalog( + 'x-ai/grok-4.5', + advertised_ids=['x-ai/grok-4.5', 'grok-4.5'], + provider='custom', + base_url='https://proxy.example.com/v1', + ) + assert model == 'x-ai/grok-4.5', f"exact full selection must win, got {model!r}" + + +def test_custom_remote_extra_models_bucket_counts_as_advertised_5979(): + """Provenance must read BOTH catalog buckets. A relay's bare id sitting in + ``extra_models`` (picker overflow) still counts as advertised, so the stale + ``openai/gpt-5.4`` prefix is stripped (#433) even when ``models`` is full of + OTHER ids and the bare id overflowed into ``extra_models``. + """ + old_cache = config._available_models_cache + old_memo = config._advertised_model_ids_memo + old_fp = config._available_models_cache_source_fingerprint + old_prov = config._models_cache_provenance + config._available_models_cache = { + 'groups': [{ + 'provider_id': 'custom', + 'models': [{'id': f'filler-{i}', 'label': f'filler-{i}'} for i in range(30)], + 'extra_models': [{'id': 'gpt-5.4', 'label': 'gpt-5.4'}], # bare id overflowed here + }] + } + config._available_models_cache_source_fingerprint = config._models_cache_source_fingerprint() + config._advertised_model_ids_memo = None + config._sync_models_cache_provenance() + try: + model, _, _ = _resolve_with_config( + 'openai/gpt-5.4', provider='custom', base_url='https://relay.example/v1', + ) + finally: + config._available_models_cache = old_cache + config._advertised_model_ids_memo = old_memo + config._available_models_cache_source_fingerprint = old_fp + config._models_cache_provenance = old_prov + assert model == 'gpt-5.4', f"bare id in extra_models must count as advertised, got {model!r}" + + +def test_custom_remote_foreign_profile_catalog_ignored_5979(): + """Profile-isolation fail-safe: when the catalog snapshot's source + fingerprint does NOT match the current runtime (a concurrently-active + foreign profile published it), that snapshot is NOT trusted for provenance — + resolution falls back to the legacy family heuristic instead of stripping + against another profile's catalog. + + Proof id: ``zai-org/GLM-5.1``. The foreign snapshot advertises a bare + ``GLM-5.1`` (which, if trusted, would strip the prefix), but ``zai-org`` is + NOT a first-party provider, so the legacy fallback preserves the full id. + A result of ``zai-org/GLM-5.1`` therefore proves the foreign catalog was + ignored (a trusted-catalog strip would have returned ``GLM-5.1``). + """ + old_cache = config._available_models_cache + old_memo = config._advertised_model_ids_memo + old_fp = config._available_models_cache_source_fingerprint + old_prov = config._models_cache_provenance + config._available_models_cache = { + 'groups': [{'provider_id': 'custom', 'models': [{'id': 'GLM-5.1', 'label': 'GLM-5.1'}]}] + } + config._available_models_cache_source_fingerprint = {'config_yaml': {'path': '/some/other/profile'}} + config._advertised_model_ids_memo = None + config._sync_models_cache_provenance() + try: + model, _, _ = _resolve_with_config( + 'zai-org/GLM-5.1', provider='custom', base_url='https://relay.example/v1', + ) + finally: + config._available_models_cache = old_cache + config._advertised_model_ids_memo = old_memo + config._available_models_cache_source_fingerprint = old_fp + config._models_cache_provenance = old_prov + assert model == 'zai-org/GLM-5.1', ( + f"foreign-profile catalog must be ignored (legacy fallback preserves), got {model!r}" + ) + + +def test_resolver_provenance_read_does_not_block_on_cache_lock_5979(): + """Regression: the resolver's per-send provenance read must be LOCK-FREE + with respect to ``_available_models_cache_lock``. + + Codex found a deadlock in an earlier cut where the accessor acquired that + lock: config-save (``_cfg_lock`` → cache lock) opposed catalog-refresh (cache + lock → ``_cfg_lock``). The fix publishes an atomic ``(snapshot, fingerprint)`` + tuple the resolver reads with one lock-free load. Proof: hold the cache lock + on one thread while another thread resolves — it must complete promptly, not + block behind the held lock. + """ + import threading + import time as _time + old_cfg = dict(config.cfg) + config.cfg.clear() + config.cfg.update({'model': { + 'provider': 'custom', 'default': 'x-ai/grok-4.5', + 'base_url': 'https://proxy.example/v1', 'models': {'x-ai/grok-4.5': {}}, + }}) + config.invalidate_models_cache() + config.get_available_models() # warm the catalog + publish provenance + try: + got = config._available_models_cache_lock.acquire(blocking=False) + assert got, "precondition: could not take cache lock non-blocking" + result = {} + def _worker(): + t0 = _time.time() + result['model'] = config.resolve_model_provider( + config.model_with_provider_context('x-ai/grok-4.5', 'custom') + )[0] + result['elapsed'] = _time.time() - t0 + th = threading.Thread(target=_worker) + th.start() + th.join(timeout=5) + blocked = th.is_alive() + config._available_models_cache_lock.release() + if blocked: + th.join(timeout=5) + assert not blocked, "DEADLOCK: resolver blocked on _available_models_cache_lock" + assert result.get('model') == 'x-ai/grok-4.5', f"got {result.get('model')!r}" + finally: + config.cfg.clear() + config.cfg.update(old_cfg) + config.invalidate_models_cache() + + def test_custom_remote_preserves_unknown_prefix_548(): - """#548: an unknown vendor prefix (zai-org/GLM-5.1) is always preserved.""" - model, provider, base_url = _resolve_with_config( + """#548: an unknown vendor prefix (zai-org/GLM-5.1) is always preserved. + + The proxy advertised the full id; ``zai-org`` isn't in _PROVIDER_MODELS so + even the bare-advertised belt could never strip it. + """ + model, provider, base_url = _resolve_with_catalog( 'zai-org/GLM-5.1', + advertised_ids=['zai-org/GLM-5.1'], provider='custom', base_url='https://api.deepinfra.com/v1/openai', ) @@ -212,15 +502,13 @@ def test_custom_remote_preserves_unknown_prefix_548(): def test_named_custom_slug_preserves_intrinsic_vendor_prefix_3872(): """#3872 (named-custom variant): provider=custom: + remote base_url also - preserves an intrinsic vendor prefix when the typed model isn't in the entry. - - A model typed/selected that is not listed in the custom_providers[] entry - falls through to the base_url branch; a bare id NOT first-party of the prefix - (bedrock/opus-4-6) must still be kept whole, same as bare ``custom``. + preserves an intrinsic vendor prefix the endpoint advertised whole. """ - model, provider, base_url = _resolve_with_config( + model, provider, base_url = _resolve_with_catalog( 'bedrock/opus-4-6', + advertised_ids=['bedrock/opus-4-6'], provider='custom:my-gateway', + provider_id='custom:my-gateway', base_url='https://router.example.com/v1', ) assert model == 'bedrock/opus-4-6', f"intrinsic prefix must be preserved for custom:slug, got {model!r}" diff --git a/tests/test_sprint40_ui_polish.py b/tests/test_sprint40_ui_polish.py index 5743ceba..8410f443 100644 --- a/tests/test_sprint40_ui_polish.py +++ b/tests/test_sprint40_ui_polish.py @@ -146,27 +146,55 @@ if __name__ == "__main__": class TestCustomEndpointModelStripping: """Tests for fix #433: strip provider prefix when custom base_url is set.""" - def _resolve(self, model_id, provider=None, base_url=None): - """Helper: set cfg directly (same pattern as test_model_resolver.py).""" + def _resolve(self, model_id, provider=None, base_url=None, advertised_ids=None): + """Helper: set cfg directly (same pattern as test_model_resolver.py). + + ``advertised_ids`` seeds the models-catalog snapshot that provenance + resolution reads (#5979). Pass the ids the endpoint's own group + advertised; None leaves the catalog cold (preserve-verbatim default). + """ old_cfg = dict(_api_config.cfg) + old_cache = _api_config._available_models_cache + old_memo = _api_config._advertised_model_ids_memo + old_fp = _api_config._available_models_cache_source_fingerprint + old_prov = _api_config._models_cache_provenance model_cfg = {} if provider: model_cfg['provider'] = provider if base_url: model_cfg['base_url'] = base_url _api_config.cfg['model'] = model_cfg + if advertised_ids is None: + _api_config._available_models_cache = None + else: + _api_config._available_models_cache = { + 'groups': [{ + 'provider_id': provider or 'custom', + 'models': [{'id': mid, 'label': mid} for mid in advertised_ids], + }] + } + _api_config._available_models_cache_source_fingerprint = _api_config._models_cache_source_fingerprint() + _api_config._advertised_model_ids_memo = None + _api_config._sync_models_cache_provenance() try: return _api_config.resolve_model_provider(model_id) finally: _api_config.cfg.clear() _api_config.cfg.update(old_cfg) + _api_config._available_models_cache = old_cache + _api_config._advertised_model_ids_memo = old_memo + _api_config._available_models_cache_source_fingerprint = old_fp + _api_config._models_cache_provenance = old_prov def test_prefixed_model_stripped_for_custom_endpoint(self): - """Issue #433: 'openai/gpt-5.4' with custom base_url returns bare 'gpt-5.4'.""" + """Issue #433: 'openai/gpt-5.4' with custom base_url returns bare 'gpt-5.4' + when the endpoint advertised ONLY the bare id (provenance strip). + """ model, provider, base_url = self._resolve( 'openai/gpt-5.4', provider='custom', base_url='http://my-proxy.local:8080/v1', + advertised_ids=['gpt-5.4'], # relay serves the bare id only ) assert model == 'gpt-5.4', ( "Expected bare 'gpt-5.4' for custom endpoint, got '{}'."