diff --git a/api/config.py b/api/config.py index b9d29136..7bba9db8 100644 --- a/api/config.py +++ b/api/config.py @@ -2560,7 +2560,7 @@ def _get_provider_cfg(provider_id) -> dict: return provider_cfg if isinstance(provider_cfg, dict) else {} -def resolve_model_provider(model_id: str) -> tuple: +def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> tuple: """Resolve model name, provider, and base_url for AIAgent. Model IDs from the dropdown can be in several formats: @@ -2580,6 +2580,16 @@ def resolve_model_provider(model_id: str) -> tuple: provider. Returns (model, provider, base_url) where provider and base_url may be None. + + ``explicitly_picked``: True when the caller knows the user DELIBERATELY + selected ``model_id`` this session (persisted from an ``explicit_model_pick`` + UI action), as opposed to it being a stale session leftover. Used ONLY for + the custom-proxy COLD-catalog decision (#5979): with no provenance available, + a deliberately-picked ``vendor/model`` is preserved verbatim (the user chose + it, the proxy routes on it), while an UNMARKED id (a stale cross-provider + leftover, e.g. #433's ``openai/gpt-5.4`` on a bare-only relay) still gets the + legacy redundant-prefix strip so it keeps routing when cold. Warm provenance + (endpoint-advertised ids) always takes precedence over this flag. """ config_provider = None config_base_url = None @@ -2898,24 +2908,32 @@ def resolve_model_provider(model_id: str) -> tuple: return model_id, config_provider, config_base_url # (3) Provenance genuinely unavailable (cold/unbuilt or # fingerprint-mismatched catalog AND not config-declared). - # PRESERVE the id verbatim. For an EXPLICIT ``custom`` / - # ``custom:`` proxy the safe default is to send exactly - # what the user selected — a wrong strip destroys a namespace - # the proxy routes on (recurs every turn, unrepairable by the - # user short of declaring every model in config), while a wrong - # preserve only mis-sends a stale cross-provider leftover that - # fails LOUDLY (the relay rejects it; #5940 surfaces the real - # error) and self-heals the instant the catalog warms. The strip - # (#433) now requires POSITIVE provenance that the endpoint - # advertises ONLY the bare id (case 2 above) — it is no longer - # the cold default. This also honours the documented contract - # of _endpoint_advertised_model_ids ("None → preserve verbatim") - # and removes the data-driven flaw where a model graduating into + # Distinguish a DELIBERATE selection from a stale leftover: + # + # * explicitly_picked → PRESERVE verbatim. The user chose this + # exact ``vendor/model`` in the UI this session; the proxy + # routes on it. A wrong strip destroys a namespace the proxy + # needs (recurs every turn, unrepairable short of declaring + # every model in config) — this is b3nw's #5979 case: a + # non-default pick on a custom: proxy, cold catalog. + # * NOT explicitly_picked → legacy redundant-prefix strip. An + # unmarked id here is a stale cross-provider leftover (the + # user switched providers and the old session model lingers, + # e.g. #433's ``openai/gpt-5.4`` on a relay that only serves + # bare ``gpt-5.4``); stripping keeps it routing while cold. + # + # Warm provenance (case 2, endpoint-advertised ids) always + # wins over this flag; the send path also warms provenance + # network-free from the disk cache first + # (warm_models_catalog_provenance_if_cold), so this branch is + # reached only in the narrow no-disk-cache window. The flag + # removes the data-driven flaw where a model graduating into # the static first-party catalog silently flipped routing - # (exactly how #5979 regressed). The send path additionally - # warms provenance network-free from the disk cache - # (warm_models_catalog_provenance_if_cold) so this cold branch - # is reached only in the narrow no-disk-cache window. + # (exactly how #5979 regressed). + if explicitly_picked: + return model_id, config_provider, config_base_url + 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 diff --git a/api/models.py b/api/models.py index cd78fa13..9187f0f0 100644 --- a/api/models.py +++ b/api/models.py @@ -1136,6 +1136,11 @@ class Session: self.workspace = str(Path(workspace).expanduser().resolve()) self.model = model self.model_provider = str(model_provider).strip().lower() if model_provider else None + # #5979: True when the user deliberately picked this session's model in + # the UI (persisted so the streaming resolver can preserve a custom-proxy + # vendor namespace on a cold catalog vs. stripping a stale leftover). + # Restored from the persisted metadata on load (arrives via **kwargs). + self.model_explicitly_picked = bool(kwargs.get('model_explicitly_picked', False)) self.messages = messages or [] self.tool_calls = tool_calls or [] self.created_at = created_at or time.time() @@ -1255,7 +1260,7 @@ class Session: # without parsing the full messages array (which may be 400KB+). # Fields are listed in the order they should appear in the JSON file. METADATA_FIELDS = [ - 'session_id', 'title', 'workspace', 'model', 'model_provider', 'created_at', 'updated_at', + 'session_id', 'title', 'workspace', 'model', 'model_provider', 'model_explicitly_picked', 'created_at', 'updated_at', 'pinned', 'archived', 'project_id', 'profile', 'input_tokens', 'output_tokens', 'estimated_cost', 'cache_read_tokens', 'cache_write_tokens', diff --git a/api/routes.py b/api/routes.py index 999b8caa..d48d348c 100644 --- a/api/routes.py +++ b/api/routes.py @@ -21680,6 +21680,32 @@ def _handle_chat_start(handler, body, diag=None): profile_config=_pp_cfg, explicit_model_pick=explicit_model_pick, ) + # #5979: persist whether the user DELIBERATELY selected this session's + # model, so the streaming worker's resolve_model_provider() can tell a + # deliberate custom-proxy pick (preserve the vendor namespace even on a + # cold catalog) from a stale cross-provider leftover (#433, strip). The + # resolve happens on a detached worker thread that only has the session + # in scope, so the intent is stamped here on the session. + # + # DURABILITY: the flag must survive follow-up sends that don't re-carry + # explicit_model_pick (the onchange marker is one-shot — messages.js + # consumes it after the first send). So we only set the flag from THIS + # send's explicit_model_pick when it's True, and otherwise CLEAR it only + # when the resolved model actually CHANGED from what was stored (a real + # switch away from the deliberately-picked model). A plain follow-up send + # of the same model keeps the prior intent. + try: + _prev_model = str(getattr(s, "model", "") or "").strip() + _new_model = str(model or "").strip() + if explicit_model_pick: + s.model_explicitly_picked = True + elif _new_model != _prev_model: + # model changed without a fresh pick → no longer a deliberate + # selection (provider repair / default fallback / stale switch) + s.model_explicitly_picked = False + # else: unchanged model, no fresh pick → preserve prior intent + except Exception: + pass catalog_profile_provider = _pp_provider if catalog_profile_provider is None and isinstance(_pp_cfg, dict): profile_model_config = _pp_cfg.get("model") or {} diff --git a/api/streaming.py b/api/streaming.py index f54a193b..821df294 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -7903,12 +7903,18 @@ def _run_agent_streaming( # owning-session profile across warm + resolve so both see the right # profile (no-op for the default/root profile). from api import profiles as profiles_api + # #5979: whether the user deliberately picked this session's model + # (persisted by /api/chat/start). Lets the cold-catalog custom-proxy + # branch preserve a deliberate vendor-namespaced pick while still + # stripping a stale cross-provider leftover (#433). + _explicitly_picked = bool(getattr(s, "model_explicitly_picked", False)) with profiles_api.profile_scope_for_detached_worker( _resolved_profile_name, "model resolution", logger_override=logger ): warm_models_catalog_provenance_if_cold() resolved_model, resolved_provider, resolved_base_url = resolve_model_provider( - model_with_provider_context(model, provider_context) + model_with_provider_context(model, provider_context), + explicitly_picked=_explicitly_picked, ) configured_base_url = resolved_base_url diff --git a/tests/test_model_resolver.py b/tests/test_model_resolver.py index bcafcbba..a3316d5d 100644 --- a/tests/test_model_resolver.py +++ b/tests/test_model_resolver.py @@ -7,7 +7,8 @@ import pytest import api.config as config -def _resolve_with_config(model_id, provider=None, base_url=None, default=None, custom_providers=None): +def _resolve_with_config(model_id, provider=None, base_url=None, default=None, custom_providers=None, + explicitly_picked=False): """Helper: temporarily set config.cfg model/custom provider sections, call resolve, restore.""" old_cfg = dict(config.cfg) model_cfg = {} @@ -21,14 +22,14 @@ def _resolve_with_config(model_id, provider=None, base_url=None, default=None, c if custom_providers is not None: config.cfg['custom_providers'] = custom_providers try: - return config.resolve_model_provider(model_id) + return config.resolve_model_provider(model_id, explicitly_picked=explicitly_picked) finally: config.cfg.clear() config.cfg.update(old_cfg) def _resolve_with_catalog(model_id, advertised_ids, *, provider=None, base_url=None, - provider_id='custom', default=None): + provider_id='custom', default=None, explicitly_picked=False): """Resolve with a seeded models-catalog snapshot (#5979 provenance). ``advertised_ids`` is the list of model ids the endpoint's own group @@ -57,7 +58,8 @@ def _resolve_with_catalog(model_id, advertised_ids, *, provider=None, base_url=N 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) + return _resolve_with_config(model_id, provider=provider, base_url=base_url, default=default, + explicitly_picked=explicitly_picked) finally: config._available_models_cache = old_cache config._advertised_model_ids_memo = old_memo @@ -300,32 +302,36 @@ 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 (reopened): with a COLD/unbuilt catalog AND no config declaration, - an explicit custom proxy PRESERVES the id verbatim — the strip now requires - positive provenance (endpoint advertises only the bare id), it is no longer - the cold default. +def test_custom_remote_cold_catalog_explicit_pick_preserves_5979(): + """#5979 (reopened): with a COLD catalog and no config declaration, an + EXPLICITLY-PICKED custom-proxy id is preserved verbatim (the user chose it; + the proxy routes on it), while an UNMARKED id gets the legacy strip. - This is the fix for b3nw's reopened case: a user's actively-selected - NON-default model on a ``custom:`` proxy was truncated on every cold - send because the old cold fallback ran the legacy family heuristic. A wrong - strip destroys a namespace the proxy routes on (recurs every turn, - unrepairable); a wrong preserve only mis-sends a stale leftover that fails - loudly and self-heals on the next catalog warm. So cold → preserve for BOTH - a first-party-looking prefix and an unknown one. + This is the persisted-explicit-pick resolution (Codex's mechanism, Nathan's + call): the cold decision is no longer an unconditional preserve — it is + gated on whether the user deliberately selected the model this session. """ - # first-party-looking prefix → PRESERVE (was: strip). This is the flip. - model_a, _, _ = _resolve_with_catalog( + # explicitly picked → PRESERVE even though grok graduated into first-party x-ai + picked, _, _ = _resolve_with_catalog( 'openai/gpt-5.4', advertised_ids=None, provider='custom', base_url='https://router.example.com/v1', + explicitly_picked=True, ) - assert model_a == 'openai/gpt-5.4', f"cold catalog must preserve verbatim, got {model_a!r}" - # intrinsic/unknown prefix → preserve (unchanged) - model_b, _, _ = _resolve_with_catalog( + assert picked == 'openai/gpt-5.4', f"explicit pick must preserve cold, got {picked!r}" + # NOT picked (stale leftover) → legacy strip keeps #433 relay routing cold + stale, _, _ = _resolve_with_catalog( + 'openai/gpt-5.4', advertised_ids=None, + provider='custom', base_url='https://router.example.com/v1', + explicitly_picked=False, + ) + assert stale == 'gpt-5.4', f"unmarked stale id must strip cold (legacy #433), got {stale!r}" + # intrinsic/unknown prefix → preserve regardless (no first-party family match) + unknown, _, _ = _resolve_with_catalog( 'zai-org/GLM-5.1', advertised_ids=None, provider='custom', base_url='https://api.deepinfra.com/v1/openai', + explicitly_picked=False, ) - assert model_b == 'zai-org/GLM-5.1', f"cold unknown prefix must preserve, got {model_b!r}" + assert unknown == 'zai-org/GLM-5.1', f"cold unknown prefix must preserve, got {unknown!r}" def test_custom_remote_config_declared_full_id_preserved_cold_5979(): @@ -413,12 +419,13 @@ def test_custom_remote_foreign_profile_catalog_ignored_5979(): catalog. Proof id: ``openai/gpt-5.4`` with the FOREIGN catalog advertising ONLY the - bare ``gpt-5.4``. If the foreign catalog were (wrongly) trusted, the - bare-only-advertised rule would strip → ``gpt-5.4``. Because the fingerprint - mismatches, the snapshot is ignored and resolution takes the cold-preserve - default → ``openai/gpt-5.4``. The two outcomes genuinely diverge, so a result - of ``openai/gpt-5.4`` proves the foreign catalog was ignored (not merely that - the prefix was unknown). + bare ``gpt-5.4``, resolved as an EXPLICIT pick. If the foreign catalog were + (wrongly) trusted, the bare-only-advertised rule (warm provenance, which + wins over the pick flag) would strip → ``gpt-5.4``. Because the fingerprint + mismatches, the snapshot is ignored and resolution takes the cold branch; + with the explicit-pick flag set that preserves → ``openai/gpt-5.4``. The two + outcomes genuinely diverge, so ``openai/gpt-5.4`` proves the foreign catalog + was ignored. """ old_cache = config._available_models_cache old_memo = config._advertised_model_ids_memo @@ -433,6 +440,7 @@ def test_custom_remote_foreign_profile_catalog_ignored_5979(): try: model, _, _ = _resolve_with_config( 'openai/gpt-5.4', provider='custom', base_url='https://relay.example/v1', + explicitly_picked=True, ) finally: config._available_models_cache = old_cache @@ -440,7 +448,7 @@ def test_custom_remote_foreign_profile_catalog_ignored_5979(): config._available_models_cache_source_fingerprint = old_fp config._models_cache_provenance = old_prov assert model == 'openai/gpt-5.4', ( - f"foreign-profile catalog must be ignored (cold-preserve default), got {model!r}" + f"foreign-profile catalog must be ignored (cold explicit-pick preserve), got {model!r}" ) @@ -526,7 +534,7 @@ def test_b3nw_cold_nondeclared_custom_slug_preserves_full_id_5979(): config._sync_models_cache_provenance() try: assert config._is_first_party_model('x-ai', bare), "precondition: stub failed" - model, provider, _ = config.resolve_model_provider(mid) + model, provider, _ = config.resolve_model_provider(mid, explicitly_picked=True) finally: if not had: if old_xai is None: @@ -541,6 +549,47 @@ def test_b3nw_cold_nondeclared_custom_slug_preserves_full_id_5979(): assert provider == 'custom:llm-proxy' +def test_custom_slug_cold_stale_not_picked_still_strips_5979(): + """Companion to b3nw's case: the SAME cold custom: shape, but the id is + NOT explicitly picked (a stale first-party leftover), still gets the legacy + strip — so #433-style stale sessions keep routing when cold and only + deliberate picks are preserved verbatim. + """ + mid = 'x-ai/grok-composer-2.5-fast' + bare = 'grok-composer-2.5-fast' + xai = list(config._PROVIDER_MODELS.get('x-ai') or []) + had = any(isinstance(m, dict) and m.get('id') == bare for m in xai) + old_xai = config._PROVIDER_MODELS.get('x-ai') + old_cfg = dict(config.cfg) + old_prov = config._models_cache_provenance + old_cache = config._available_models_cache + if not had: + config._PROVIDER_MODELS['x-ai'] = xai + [{'id': bare, 'label': 'Grok Composer 2.5 Fast'}] + config.cfg.clear() + config.cfg.update({ + 'model': {'default': 'x-ai/grok-4.5', 'provider': 'custom:llm-proxy', + 'base_url': 'https://proxy.example/v1'}, + 'custom_providers': [{'name': 'llm-proxy', 'base_url': 'https://proxy.example/v1', + 'key_env': 'LLM_PROXY_API_KEY'}], + }) + config._available_models_cache = None # COLD + config._advertised_model_ids_memo = None + config._sync_models_cache_provenance() + try: + model, _, _ = config.resolve_model_provider(mid, explicitly_picked=False) + finally: + if not had: + if old_xai is None: + config._PROVIDER_MODELS.pop('x-ai', None) + else: + config._PROVIDER_MODELS['x-ai'] = old_xai + config.cfg.clear() + config.cfg.update(old_cfg) + config._models_cache_provenance = old_prov + config._available_models_cache = old_cache + assert model == bare, f"unmarked stale cold custom:slug id must strip (legacy), got {model!r}" + + def test_warm_models_catalog_provenance_if_cold_publishes_from_disk_5979(): """The send-path warm helper publishes provenance from a valid disk cache when memory is cold — restoring the endpoint-advertised signal so #433