feat(#5979): persisted explicit-pick provenance for cold custom-proxy resolution
Nathan's call: adopt Codex's complete mechanism instead of the unconditional cold-preserve. The cold-catalog custom-proxy decision is now gated on whether the user DELIBERATELY selected the model this session, closing the residual where a stale #433 leftover would preserve+400 on a cold+no-disk send. Mechanism: - resolve_model_provider(model_id, *, explicitly_picked=False): in the custom-arm cold branch (no warm provenance, not config-declared), explicitly_picked=True preserves the vendor namespace verbatim (deliberate pick, proxy routes on it); otherwise the legacy redundant-prefix strip runs (stale cross-provider leftover, #433). Warm endpoint-advertised provenance still wins over the flag. - Session.model_explicitly_picked: new persisted field (default False, restored from metadata via **kwargs, listed in METADATA_FIELDS so it survives a restart — b3nw's exact cold scenario). - /api/chat/start stamps it: True on a fresh explicit_model_pick; cleared only when the resolved model actually CHANGED without a pick (real switch away); preserved across plain follow-up sends of the same model (the onchange marker is one-shot, so per-send explicit_model_pick alone would drop the intent). - streaming.py reads s.model_explicitly_picked and passes explicitly_picked= into resolve_model_provider (inside the profile scope). Independent end-to-end matrix (all pass): ben default cold, ben picked non-default cold+warm, ben stale cold (strips), #433 warm+cold, #3872 bedrock, #548 zai-org. 67 resolver+sprint40 tests pass incl. new explicit-pick + stale-companion + persistence round-trip.
This commit is contained in:
parent
f42b4d14f5
commit
45cb5bd3e6
5 changed files with 154 additions and 50 deletions
|
|
@ -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:<slug>`` 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:<slug> 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
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -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 {}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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:<slug>`` 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:<slug> 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
|
||||
|
|
|
|||
Loading…
Reference in a new issue