From 4349ff4302a2e18f87a44882e49b7566e31d052d Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sun, 12 Jul 2026 04:22:44 +0000 Subject: [PATCH 1/5] fix(config): custom/proxy providers keep the full vendor namespace (#5979) Resolve the model id for custom/proxy providers by ENDPOINT PROVENANCE instead of a catalog-family guess. The old gate stripped a vendor prefix whenever the bare id was first-party of the prefix's home vendor, which could not distinguish a namespace the proxy actually routes on (x-ai/grok-4.5 -> keep) from a stale cross-provider leftover on a relay that only serves bare ids (openai/gpt-5.4 -> strip). A model graduating into the first-party catalog (agent commit adding grok-4.5) silently flipped a working custom-proxy id from preserved to stripped -> HTTP 400. New _endpoint_advertised_model_ids() reads the already-published catalog snapshot (never builds/probes/touches disk; O(1) per send via a snapshot-identity memo) and answers what the endpoint advertised: - full vendor/model advertised -> preserve (#5979, #3872, #548) - only the bare id advertised -> strip the redundant prefix (#433) - cold/unbuilt catalog -> preserve verbatim (fail-safe toward the id the user actively selected) The non-custom first-party-proxy strip arm is unchanged. Rewrites the two pinned #433 tests to model provenance (bare-only advertised) and adds #5979 coverage: advertised-full preserve (bare + named custom), cold-catalog preserve-verbatim, and both-advertised prefers-full. Thanks @b3nw for the report and precise root-cause analysis. --- CHANGELOG.md | 2 + api/config.py | 101 ++++++++++++++++++++- tests/test_model_resolver.py | 150 +++++++++++++++++++++++++++---- tests/test_sprint40_ui_polish.py | 28 +++++- 4 files changed, 259 insertions(+), 22 deletions(-) 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..7fdb0ed4 100644 --- a/api/config.py +++ b/api/config.py @@ -2808,9 +2808,46 @@ 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 ONLY 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. Provenance tells them apart: + _advertised = _endpoint_advertised_model_ids(config_provider) + if _advertised: + # Full id advertised → the endpoint routes 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 + # Cold/unbuilt catalog OR neither shape advertised → preserve the + # id verbatim. This is the fail-SAFE default: a wrong strip would + # destroy a namespace the user actively selected this session + # (recurs every turn, unrepairable — the info is gone), while a + # wrong preserve only mis-sends a stale leftover that fails loudly + # and self-heals the moment the catalog builds. Matches the + # agent's own normalize_model_for_provider (custom = pass-through) + # and the #4210 no-base_url custom behaviour. + 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 +4548,64 @@ _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 + + +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 + snapshot = _available_models_cache # atomic reference read; publishes replace wholesale + if snapshot is None: + return None + 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 + ids = frozenset( + str(m.get("id")) + for m in (group.get("models") 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, diff --git a/tests/test_model_resolver.py b/tests/test_model_resolver.py index 0f6f5ee5..3e0826be 100644 --- a/tests/test_model_resolver.py +++ b/tests/test_model_resolver.py @@ -27,6 +27,36 @@ 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 + 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], + }] + } + config._advertised_model_ids_memo = None # force recompute against the seeded snapshot + 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 + + # ── OpenRouter prefix handling ──────────────────────────────────────────── def test_openrouter_free_keeps_full_path(): @@ -170,10 +200,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 +215,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 +236,93 @@ 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. + """ + # Reproduce the data-driven trigger: grok-4.5 present in the x-ai catalog. + xai_catalog = config._PROVIDER_MODELS.get('x-ai') + assert isinstance(xai_catalog, list) + assert any(isinstance(m, dict) and m.get('id') == 'grok-4.5' for m in xai_catalog), ( + "guard: grok-4.5 must be first-party for this regression test to be meaningful" + ) + 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', + ) + 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_preserves_verbatim_5979(): + """#5979 fail-safe: with a COLD/unbuilt catalog (no provenance signal), a + custom proxy preserves the model id verbatim — for BOTH the advertised-full + case and the stale-leftover case. + + Preserve is the correct cold default: a wrong strip destroys a namespace the + user actively selected this session (recurs every turn, unrepairable), while + a wrong preserve only mis-sends a stale leftover that fails loudly and heals + the instant the catalog builds. The active-user-selection case (#5979) is + cold-safe by construction — you can't select from a dropdown that was never + built. + """ + model_a, _, _ = _resolve_with_catalog( + 'x-ai/grok-4.5', advertised_ids=None, + provider='custom', base_url='https://proxy.example.com/v1', + ) + assert model_a == 'x-ai/grok-4.5', f"cold catalog must preserve verbatim, got {model_a!r}" + model_b, _, _ = _resolve_with_catalog( + 'openai/gpt-5.4', advertised_ids=None, + provider='custom', base_url='https://router.example.com/v1', + ) + assert model_b == 'openai/gpt-5.4', f"cold catalog must preserve verbatim, got {model_b!r}" + + +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_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 +332,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..c802af95 100644 --- a/tests/test_sprint40_ui_polish.py +++ b/tests/test_sprint40_ui_polish.py @@ -146,27 +146,49 @@ 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 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._advertised_model_ids_memo = None 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 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 '{}'." From 0bdf555779819ac69cc5a692e5c78765846aff7c Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sun, 12 Jul 2026 04:39:47 +0000 Subject: [PATCH 2/5] harden #5979 per Codex gate: union extra_models bucket + profile-isolation fail-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex regression gate found two real issues on the first cut: 1. extra_models bucket: a provider's advertised ids can be split across the catalog's 'models' (visible) and 'extra_models' (picker overflow) buckets. Reading only 'models' could miss a bare id that overflowed, leaving the #433 stale prefix unstripped. _endpoint_advertised_model_ids now unions both buckets. 2. Profile isolation (profiles are islands): _available_models_cache is a process global, so a concurrently-active foreign profile could publish the snapshot a different profile then reads for provenance — potentially stripping an advertised full id against another profile's catalog. The accessor now rejects the snapshot unless its published source fingerprint (whose config_yaml axis is the profile-specific config path) matches the current runtime fingerprint; any mismatch fails safe to verbatim preserve. Adds tests: extra_models-counts-as-advertised (#433 via overflow bucket) and foreign-profile-catalog-preserves-verbatim (isolation fail-safe). Test helpers now stamp the source fingerprint exactly as the real publish sites do. --- api/config.py | 22 ++++++++++- tests/test_model_resolver.py | 63 ++++++++++++++++++++++++++++++++ tests/test_sprint40_ui_polish.py | 3 ++ 3 files changed, 87 insertions(+), 1 deletion(-) diff --git a/api/config.py b/api/config.py index 7fdb0ed4..98894829 100644 --- a/api/config.py +++ b/api/config.py @@ -4579,6 +4579,20 @@ def _endpoint_advertised_model_ids(provider_id: str | None) -> frozenset | None: snapshot = _available_models_cache # atomic reference read; publishes replace wholesale 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 its published + # source fingerprint 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, cold fingerprint) returns None so the caller preserves the id + # verbatim rather than stripping against another profile's catalog. + try: + if _available_models_cache_source_fingerprint != _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. @@ -4594,9 +4608,15 @@ def _endpoint_advertised_model_ids(provider_id: str | None) -> frozenset | None: 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 m in (group.get("models") or []) + 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 diff --git a/tests/test_model_resolver.py b/tests/test_model_resolver.py index 3e0826be..12d87d2a 100644 --- a/tests/test_model_resolver.py +++ b/tests/test_model_resolver.py @@ -40,6 +40,7 @@ def _resolve_with_catalog(model_id, advertised_ids, *, provider=None, base_url=N """ old_cache = config._available_models_cache old_memo = config._advertised_model_ids_memo + old_fp = config._available_models_cache_source_fingerprint if advertised_ids is None: config._available_models_cache = None else: @@ -49,12 +50,16 @@ def _resolve_with_catalog(model_id, advertised_ids, *, provider=None, base_url=N '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 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 # ── OpenRouter prefix handling ──────────────────────────────────────────── @@ -314,6 +319,64 @@ def test_custom_remote_prefers_full_id_when_both_advertised_5979(): 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 + 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 + 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 + 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_preserves_verbatim_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), provenance is untrusted and the id is + preserved verbatim — never stripped against another profile's catalog. + """ + old_cache = config._available_models_cache + old_memo = config._advertised_model_ids_memo + old_fp = config._available_models_cache_source_fingerprint + # Snapshot advertises ONLY the bare id (would normally strip openai/gpt-5.4), + # but the fingerprint is a deliberate mismatch (a foreign profile's stamp). + config._available_models_cache = { + 'groups': [{'provider_id': 'custom', 'models': [{'id': 'gpt-5.4', 'label': 'gpt-5.4'}]}] + } + config._available_models_cache_source_fingerprint = {'config_yaml': {'path': '/some/other/profile'}} + config._advertised_model_ids_memo = None + 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 + assert model == 'openai/gpt-5.4', ( + f"foreign-profile catalog must not strip; expected verbatim, got {model!r}" + ) + + def test_custom_remote_preserves_unknown_prefix_548(): """#548: an unknown vendor prefix (zai-org/GLM-5.1) is always preserved. diff --git a/tests/test_sprint40_ui_polish.py b/tests/test_sprint40_ui_polish.py index c802af95..f3c5fe51 100644 --- a/tests/test_sprint40_ui_polish.py +++ b/tests/test_sprint40_ui_polish.py @@ -156,6 +156,7 @@ class TestCustomEndpointModelStripping: 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 model_cfg = {} if provider: model_cfg['provider'] = provider @@ -171,6 +172,7 @@ class TestCustomEndpointModelStripping: '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 try: return _api_config.resolve_model_provider(model_id) @@ -179,6 +181,7 @@ class TestCustomEndpointModelStripping: _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 def test_prefixed_model_stripped_for_custom_endpoint(self): """Issue #433: 'openai/gpt-5.4' with custom base_url returns bare 'gpt-5.4' From 036c671f14985534e8b93d132ef5610b6a5e18e0 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sun, 12 Jul 2026 04:51:13 +0000 Subject: [PATCH 3/5] harden #5979 round 2 (Codex): atomic snapshot+fingerprint read + tri-state cold fallback Two more real issues from the second Codex gate: 1. Atomic provenance read: _endpoint_advertised_model_ids read the global snapshot and its source fingerprint as two unlocked steps, while publishers assign the new snapshot BEFORE computing the fingerprint (streaming.py publish sites). In that window profile B's freshly-published catalog could validate against profile A's still-current fingerprint and strip against the wrong catalog. Fix: acquire _available_models_cache_lock (RLock) across the snapshot read + fingerprint check + memo build, so the pair is always consistent. The lock is held only for the two-assignment publish (the network rebuild runs before it), so the per-send acquisition never blocks on a probe. 2. Tri-state cold fallback (was: unconditional cold->preserve): an unconditional verbatim fallback newly broke the #433 relay cold path (openai/gpt-5.4 sent to a relay that only accepts gpt-5.4; master stripped it). Now: (1) config declares the full id verbatim (model.default / model.models / custom_providers[].models) -> preserve (network-free, survives a cold restart -> #5979 works cold via b3nw's model.default) (2) endpoint catalog advertised it -> full=preserve / bare-only=strip (3) genuinely unknown (cold + not declared) -> LEGACY family heuristic, so the edge is never worse than pre-fix: first-party redundant prefix still strips (#433 cold), intrinsic/unknown preserved (#3872/#548). The #5979 active-user path can't reach (3): a selected id is either config-declared or in the catalog the dropdown was built from. New _model_id_declared_in_config() provides the config-provenance signal. Tests updated: cold-fallback-to-legacy, config-declared-cold-preserve, foreign-profile-catalog-ignored (proves untrusted snapshot is not used). --- api/config.py | 192 ++++++++++++++++++++++++----------- tests/test_model_resolver.py | 85 +++++++++++----- 2 files changed, 195 insertions(+), 82 deletions(-) diff --git a/api/config.py b/api/config.py index 98894829..4c9e1eab 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. @@ -2809,7 +2860,7 @@ def resolve_model_provider(model_id: str) -> tuple: _cp_lower = (config_provider or "").strip().lower() _is_custom = _cp_lower == "custom" or _cp_lower.startswith("custom:") if _is_custom: - # Vendor-routing proxy: the ONLY reliable signal for whether the + # 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 @@ -2823,10 +2874,17 @@ def resolve_model_provider(model_id: str) -> tuple: # 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. Provenance tells them apart: + # 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 → the endpoint routes on it verbatim (#5979/#3872/#548). + # 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 @@ -2835,14 +2893,20 @@ def resolve_model_provider(model_id: str) -> tuple: # 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 - # Cold/unbuilt catalog OR neither shape advertised → preserve the - # id verbatim. This is the fail-SAFE default: a wrong strip would - # destroy a namespace the user actively selected this session - # (recurs every turn, unrepairable — the info is gone), while a - # wrong preserve only mis-sends a stale leftover that fails loudly - # and self-heals the moment the catalog builds. Matches the - # agent's own normalize_model_for_provider (custom = pass-through) - # and the #4210 no-base_url custom behaviour. + # 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 @@ -4576,54 +4640,68 @@ def _endpoint_advertised_model_ids(provider_id: str | None) -> frozenset | None: something this custom endpoint advertised. """ global _advertised_model_ids_memo - snapshot = _available_models_cache # atomic reference read; publishes replace wholesale - 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 its published - # source fingerprint 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, cold fingerprint) returns None so the caller preserves the id - # verbatim rather than stripping against another profile's catalog. - try: - if _available_models_cache_source_fingerprint != _models_cache_source_fingerprint(): + # Read the snapshot AND its source fingerprint together under the catalog + # lock so we can never observe a torn pair: publishes assign + # _available_models_cache and _available_models_cache_source_fingerprint as + # two separate statements (always inside this same lock), so an unlocked + # reader could otherwise catch a freshly-published FOREIGN snapshot still + # paired with the PREVIOUS profile's (matching) fingerprint and strip against + # the wrong catalog. The lock is an RLock and every publish holds it only for + # the two assignments — the expensive network rebuild happens BEFORE the + # publish block — so this per-send acquisition contends only with the atomic + # publish, never with a live provider probe. + with _available_models_cache_lock: + snapshot = _available_models_cache + published_fp = _available_models_cache_source_fingerprint + if snapshot is None: 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] = {} + # 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 its published source fingerprint 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, cold fingerprint) returns None + # so the caller preserves the id verbatim rather than stripping against + # another profile's catalog. 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) + 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 diff --git a/tests/test_model_resolver.py b/tests/test_model_resolver.py index 12d87d2a..8ee7d1b7 100644 --- a/tests/test_model_resolver.py +++ b/tests/test_model_resolver.py @@ -283,28 +283,58 @@ def test_named_custom_slug_preserves_advertised_full_id_5979(): 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_preserves_verbatim_5979(): - """#5979 fail-safe: with a COLD/unbuilt catalog (no provenance signal), a - custom proxy preserves the model id verbatim — for BOTH the advertised-full - case and the stale-leftover case. +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. - Preserve is the correct cold default: a wrong strip destroys a namespace the - user actively selected this session (recurs every turn, unrepairable), while - a wrong preserve only mis-sends a stale leftover that fails loudly and heals - the instant the catalog builds. The active-user-selection case (#5979) is - cold-safe by construction — you can't select from a dropdown that was never - built. + 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( - 'x-ai/grok-4.5', advertised_ids=None, - provider='custom', base_url='https://proxy.example.com/v1', - ) - assert model_a == 'x-ai/grok-4.5', f"cold catalog must preserve verbatim, got {model_a!r}" - model_b, _, _ = _resolve_with_catalog( 'openai/gpt-5.4', advertised_ids=None, provider='custom', base_url='https://router.example.com/v1', ) - assert model_b == 'openai/gpt-5.4', f"cold catalog must preserve verbatim, got {model_b!r}" + 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_cfg = dict(config.cfg) + config._available_models_cache = None # cold + config._advertised_model_ids_memo = None + 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.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(): @@ -348,32 +378,37 @@ def test_custom_remote_extra_models_bucket_counts_as_advertised_5979(): 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_preserves_verbatim_5979(): +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), provenance is untrusted and the id is - preserved verbatim — never stripped against another profile's catalog. + 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 - # Snapshot advertises ONLY the bare id (would normally strip openai/gpt-5.4), - # but the fingerprint is a deliberate mismatch (a foreign profile's stamp). config._available_models_cache = { - 'groups': [{'provider_id': 'custom', 'models': [{'id': 'gpt-5.4', 'label': 'gpt-5.4'}]}] + '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 try: model, _, _ = _resolve_with_config( - 'openai/gpt-5.4', provider='custom', base_url='https://relay.example/v1', + '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 - assert model == 'openai/gpt-5.4', ( - f"foreign-profile catalog must not strip; expected verbatim, got {model!r}" + assert model == 'zai-org/GLM-5.1', ( + f"foreign-profile catalog must be ignored (legacy fallback preserves), got {model!r}" ) From 1443288dee80e9defe0e9d1818f1b49353ee3a2b Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sun, 12 Jul 2026 05:14:04 +0000 Subject: [PATCH 4/5] harden #5979 round 3 (Codex): lock-free atomic (snapshot,fingerprint) provenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-2 fix acquired _available_models_cache_lock in the resolver to read the snapshot+fingerprint consistently. Codex found that introduces a DEADLOCK: config-save takes _cfg_lock -> _available_models_cache_lock while catalog-refresh takes _available_models_cache_lock -> _cfg_lock (lock-order inversion), and it also makes every send wait behind a catalog rebuild. Fix: publish an immutable (snapshot, publisher_fingerprint) tuple atomically via _sync_models_cache_provenance() at every cache publish/invalidate site (9 sites), and have _endpoint_advertised_model_ids read that single global with ONE lock-free load. Reading one tuple can't tear (GIL), acquires no lock (no ordering edge, no deadlock), and never blocks behind a rebuild. A reader racing a publish sees the previous consistent pair, never a torn snapshot/fingerprint. Verified: two-thread probe — resolver completes in 0.0001s while another thread holds _available_models_cache_lock (was the deadlock). Real catalog build publishes the provenance tuple and closes the P0 end-to-end. Adds a deadlock-regression test. --- api/config.py | 152 +++++++++++++++++++------------ tests/test_model_resolver.py | 58 ++++++++++++ tests/test_sprint40_ui_polish.py | 3 + 3 files changed, 153 insertions(+), 60 deletions(-) diff --git a/api/config.py b/api/config.py index 4c9e1eab..8e6ac076 100644 --- a/api/config.py +++ b/api/config.py @@ -4620,6 +4620,34 @@ _cache_build_in_progress = False # True while a cold path is actively building # 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. @@ -4640,68 +4668,63 @@ def _endpoint_advertised_model_ids(provider_id: str | None) -> frozenset | None: something this custom endpoint advertised. """ global _advertised_model_ids_memo - # Read the snapshot AND its source fingerprint together under the catalog - # lock so we can never observe a torn pair: publishes assign - # _available_models_cache and _available_models_cache_source_fingerprint as - # two separate statements (always inside this same lock), so an unlocked - # reader could otherwise catch a freshly-published FOREIGN snapshot still - # paired with the PREVIOUS profile's (matching) fingerprint and strip against - # the wrong catalog. The lock is an RLock and every publish holds it only for - # the two assignments — the expensive network rebuild happens BEFORE the - # publish block — so this per-send acquisition contends only with the atomic - # publish, never with a live provider probe. - with _available_models_cache_lock: - snapshot = _available_models_cache - published_fp = _available_models_cache_source_fingerprint - if snapshot is None: + # 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 - # 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 its published source fingerprint 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, cold fingerprint) returns None - # so the caller preserves the id verbatim rather than stripping against - # another profile's catalog. + 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: - 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) + 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 @@ -6010,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)) @@ -6017,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 @@ -6041,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, @@ -6100,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 @@ -7847,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 @@ -7897,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 ──────────────── @@ -7973,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: @@ -8022,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: @@ -8186,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 8ee7d1b7..3570bbea 100644 --- a/tests/test_model_resolver.py +++ b/tests/test_model_resolver.py @@ -41,6 +41,7 @@ def _resolve_with_catalog(model_id, advertised_ids, *, provider=None, base_url=N 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: @@ -54,12 +55,14 @@ def _resolve_with_catalog(model_id, advertised_ids, *, provider=None, base_url=N # 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 ──────────────────────────────────────────── @@ -315,9 +318,11 @@ def test_custom_remote_config_declared_full_id_preserved_cold_5979(): 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 @@ -329,6 +334,7 @@ def test_custom_remote_config_declared_full_id_preserved_cold_5979(): 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', ( @@ -358,6 +364,7 @@ def test_custom_remote_extra_models_bucket_counts_as_advertised_5979(): 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', @@ -367,6 +374,7 @@ def test_custom_remote_extra_models_bucket_counts_as_advertised_5979(): } 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', @@ -375,6 +383,7 @@ def test_custom_remote_extra_models_bucket_counts_as_advertised_5979(): 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}" @@ -394,11 +403,13 @@ def test_custom_remote_foreign_profile_catalog_ignored_5979(): 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', @@ -407,11 +418,58 @@ def test_custom_remote_foreign_profile_catalog_ignored_5979(): 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. diff --git a/tests/test_sprint40_ui_polish.py b/tests/test_sprint40_ui_polish.py index f3c5fe51..8410f443 100644 --- a/tests/test_sprint40_ui_polish.py +++ b/tests/test_sprint40_ui_polish.py @@ -157,6 +157,7 @@ class TestCustomEndpointModelStripping: 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 @@ -174,6 +175,7 @@ class TestCustomEndpointModelStripping: } _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: @@ -182,6 +184,7 @@ class TestCustomEndpointModelStripping: _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' From 12a68ba27dcf694e9cece77d17a2e4f0e106d3b9 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sun, 12 Jul 2026 05:37:06 +0000 Subject: [PATCH 5/5] test(#5979): stub grok-4.5 into x-ai catalog instead of asserting live presence CI's hermes-agent catalog version may not contain grok-4.5, so asserting the live _PROVIDER_MODELS['x-ai'] already lists it failed on CI (shard 4, all py versions). Reproduce the data-driven first-party trigger deterministically by stubbing grok-4.5 into the x-ai catalog for the test (restored in finally), regardless of the agent version CI runs against. The other #5979 tests preserve x-ai/grok-4.5 via the advertised-full / cold-unknown-prefix paths, which don't depend on x-ai being a known first-party prefix. --- tests/test_model_resolver.py | 38 ++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/tests/test_model_resolver.py b/tests/test_model_resolver.py index 3570bbea..5c67d1e8 100644 --- a/tests/test_model_resolver.py +++ b/tests/test_model_resolver.py @@ -254,18 +254,32 @@ def test_custom_remote_preserves_advertised_full_id_5979(): 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. """ - # Reproduce the data-driven trigger: grok-4.5 present in the x-ai catalog. - xai_catalog = config._PROVIDER_MODELS.get('x-ai') - assert isinstance(xai_catalog, list) - assert any(isinstance(m, dict) and m.get('id') == 'grok-4.5' for m in xai_catalog), ( - "guard: grok-4.5 must be first-party for this regression test to be meaningful" - ) - 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', - ) + # 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}" )