diff --git a/api/config.py b/api/config.py index 5c2d5a48..f8a598a0 100644 --- a/api/config.py +++ b/api/config.py @@ -2616,6 +2616,85 @@ def _get_provider_cfg(provider_id) -> dict: return provider_cfg if isinstance(provider_cfg, dict) else {} +class AmbiguousCustomProviderError(ValueError): + """Raised when two+ custom_providers[] entries normalize to the same slug. + + A custom provider is identified downstream by a SLUG (``custom:``): + ``resolve_model_provider()`` returns it, and the credential lookup + (``resolve_custom_provider_connection``) resolves the API key + base_url by + scanning ``custom_providers[]`` for the FIRST entry whose name normalizes to + that slug — independent of model ownership or the endpoint chosen earlier. So + when two distinct provider names normalize to the same slug (e.g. ``Foo Bar`` + and ``foo-bar`` both -> ``custom:foo-bar``), consuming the slug on ANY path + could pair one entry's endpoint with another entry's credential — including + the asymmetric case where only one of the colliding entries lists the + requested model. Rather than guess, every slug-only boundary fails closed and + surfaces the collision so the user can rename one provider. Subclasses + ``ValueError`` so existing ``except ValueError`` / broad-``except`` fallbacks + continue to catch it. + """ + + def __init__(self, message: str): + super().__init__(message) + # Expose the actionable rename text as ``.message`` so HTTP handlers can + # forward it verbatim (as the JSON ``error``) without ``str(e)`` casts, + # and it reaches the user on the handoff + save paths instead of being + # swallowed into a generic fallback. + self.message = message + + +def _custom_provider_slug_key(value: object) -> str: + """Canonical bare slug for custom-provider identity + collision detection. + + Derived from the SINGLE authoritative slug PRODUCER + ``_custom_provider_slug_from_name()`` — the same function that mints the + ``custom:`` ids resolve_model_provider() actually returns, persists, + and routes on. Using the producer's normalization (not a private variant) + everywhere means every slug-only boundary — bare + qualified resolution, + credential lookup, auxiliary persistence — compares against ONE identity, so + names that genuinely collide at the producer level (e.g. ``Foo (Bar)`` and + ``foo-bar`` both -> ``custom:foo-bar``) are detected as collisions instead of + slipping through a looser key. Returns the bare slug (no ``custom:`` prefix). + Accepts a bare provider name or a ``custom:`` id. + """ + produced = _custom_provider_slug_from_name(value) + return produced.split(":", 1)[1] if produced.startswith("custom:") else produced + + +def _unique_custom_provider_entry(custom_providers: object, slug_key: str) -> dict | None: + """Return the single named ``custom_providers`` entry matching ``slug_key``. + + Pure and lock-safe: operates only on the passed-in list, so it can be called + while holding ``_cfg_lock`` (unlike ``get_config()``-based resolvers). + + Membership is built from ALL named entries, INDEPENDENT of model ownership, + because slug-only credential resolution scans every same-slug entry and + returns the first match. Raises ``AmbiguousCustomProviderError`` when 2+ + entries share the key so an endpoint and an API key can never be resolved + from different entries on any path. Returns the matching entry, or ``None`` + when no entry matches. + """ + if not slug_key or not isinstance(custom_providers, list): + return None + matches: list[dict] = [] + for entry in custom_providers: + if not isinstance(entry, dict): + continue + name = str(entry.get("name") or "").strip() + if not name: + continue + if _custom_provider_slug_key(name) == slug_key: + matches.append(entry) + if len(matches) >= 2: + names = [str(e.get("name") or "").strip() for e in matches] + raise AmbiguousCustomProviderError( + f"Custom providers {names!r} all normalize to the same provider slug " + f"{slug_key!r}; an endpoint and API key could be resolved from " + f"different entries. Rename one so each custom provider has a unique slug." + ) + return matches[0] if matches else None + + def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> tuple: """Resolve model name, provider, and base_url for AIAgent. @@ -2669,9 +2748,30 @@ def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> if isinstance(config_provider, str) and config_provider.strip().lower() == "local": config_provider = "custom" + def _finalize(model: object, provider: object, base_url: object) -> tuple: + """Point-of-return collision guard. + + Fail closed HERE — immediately before handing a ``custom:`` back — + never up front. A slug collision on one custom pair must not block a + request that resolves to a DIFFERENT provider (an unrelated + ``@openrouter:...`` / ``@custom:safe-provider:...`` lane, or the bare + ``custom`` proxy), so the ambiguity check runs only on the slug actually + being returned. ``_unique_custom_provider_entry`` raises + AmbiguousCustomProviderError when >=2 config entries share the slug, so a + downstream credential lookup can never first-match a different entry than + the one whose endpoint we resolved. No-op for bare ``custom`` and every + non-custom provider. + """ + if isinstance(provider, str) and provider.startswith("custom:"): + _unique_custom_provider_entry( + cfg.get('custom_providers', []), + _custom_provider_slug_key(provider), + ) + return model, provider, base_url + model_id = (model_id or "").strip() if not model_id: - return model_id, config_provider, config_base_url + return _finalize(model_id, config_provider, config_base_url) # Custom providers declared in config.yaml should win over slash-based # OpenRouter heuristics. Their model IDs commonly contain '/' too. @@ -2732,6 +2832,57 @@ def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> ) custom_providers = cfg.get('custom_providers', []) if isinstance(custom_providers, list) and not _skip_custom_providers: + # Disambiguation guard: when two custom_providers[] entries both list the + # same bare model id (e.g. dogapi and packyapi both advertise + # 'claude-sonnet-5'), a plain first-match scan routes on config WRITE + # ORDER — silently hijacking the model to whichever entry appears first, + # regardless of the active provider the user actually configured. If the + # ACTIVE provider is itself a named custom provider (config_provider + # resolved to 'custom:', including via model.base_url → named-slug + # matching), prefer THAT entry when it also owns the model, so an explicit + # active endpoint wins over an overlapping earlier entry. Falls through to + # the ordered scan below when the active provider is bare 'custom' / not a + # named custom entry, or when it doesn't list this model. + _active_custom_slug = '' + if isinstance(config_provider, str) and config_provider.startswith('custom:'): + _active_custom_slug = config_provider + + def _entry_owns_model(entry: dict) -> bool: + entry_model = (entry.get('model') or '').strip() + ids = set() + if entry_model: + ids.add(entry_model) + ids.update(_configured_model_ids(entry.get('models'))) + return model_id in ids + + # Explicit active named provider wins over config order when it owns the + # model. Only CONSUME the active slug (and thus let _finalize fail closed + # on a collision) when the active provider actually lists this model: if + # it doesn't, fall through so an unrelated collision on the active slug + # never blocks a request that resolves to a different provider. + if _active_custom_slug: + _active_key = _custom_provider_slug_key(_active_custom_slug) + _active_owner = next( + ( + e for e in custom_providers + if isinstance(e, dict) + and _entry_owns_model(e) + and _custom_provider_slug_key(e.get('name')) == _active_key + ), + None, + ) + # Exactly one entry carries the active slug AND owns the model -> + # authoritative, even when model.base_url is stale/absent or points at + # a different endpoint. An explicit, unambiguous named provider must + # never lose to config order: a stale URL is not evidence to discard + # it. _finalize() fails closed if the active slug is shared by >=2 + # entries (endpoint + credential could then split). + if _active_owner is not None: + return _finalize( + model_id, + _active_custom_slug, + (_active_owner.get('base_url') or '').strip() or None, + ) for entry in custom_providers: if not isinstance(entry, dict): continue @@ -2744,7 +2895,9 @@ def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> entry_model_ids.update(_configured_model_ids(entry.get('models'))) if entry_name and model_id in entry_model_ids: provider_hint = _custom_provider_slug_from_name(entry_name) - return model_id, provider_hint, entry_base_url or None + # _finalize() applies the all-entry collision guard on this + # bare-'custom' / fall-through path before returning the slug. + return _finalize(model_id, provider_hint, entry_base_url or None) # Check user-defined providers (config.yaml → providers:). # Mirrors the custom_providers scan above — exact match against each @@ -2803,14 +2956,21 @@ def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> parsed_provider_hint = _parse_provider_qualified_model_id(model_id) if parsed_provider_hint is not None: bare_model, provider_hint = parsed_provider_hint + # Session/send/handoff shapes encode the provider as @custom::model + # and reach here after the ownership scan only saw the ENCODED string. + # _finalize() applies the all-entry uniqueness check before returning the + # slug so the downstream credential lookup can't first-match a different + # colliding entry — but ONLY on the custom: actually returned, so an + # unrelated collision never blocks this explicit hint (@openrouter, a + # non-colliding @custom:other, ...). if ( provider_hint.startswith("custom:") and config_base_url and _is_local_server_provider(config_provider) and provider_hint.lower() in _custom_endpoint_slugs_for_base_url(config_base_url) ): - return bare_model, config_provider, config_base_url - return bare_model, provider_hint, _get_provider_base_url(provider_hint) + return _finalize(bare_model, config_provider, config_base_url) + return _finalize(bare_model, provider_hint, _get_provider_base_url(provider_hint)) if "/" in model_id: prefix, bare = model_id.split("/", 1) @@ -2828,11 +2988,11 @@ def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> # #854 / #894 for Nous, where this guard was originally added). _PORTAL_PROVIDERS = {"nous", "opencode-zen", "opencode-go", "nvidia"} if config_provider in _PORTAL_PROVIDERS: - return model_id, config_provider, config_base_url + return _finalize(model_id, config_provider, config_base_url) # If prefix matches config provider exactly, strip it and use that provider directly. # e.g. config=anthropic, model=anthropic/claude-... → bare name to anthropic API if config_provider and prefix == config_provider: - return bare, config_provider, config_base_url + return _finalize(bare, config_provider, config_base_url) # The OpenAI Codex provider uses a real base_url, but its default # ChatGPT endpoint cannot serve OpenRouter-style provider/model IDs. # Keep that narrow exception before the custom endpoint protection so @@ -2858,7 +3018,7 @@ def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> if isinstance(_entry, dict) and _entry.get("name", "").strip() == prefix: _slug = _custom_provider_slug_from_name(prefix) _base = (_entry.get("base_url") or "").strip() - return model_id, _slug, _base or None + return _finalize(model_id, _slug, _base or None) # If a custom endpoint base_url is configured, don't reroute through OpenRouter # just because the model name contains a slash (e.g. google/gemma-4-26b-a4b). @@ -2872,7 +3032,7 @@ def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> # pointing at a loopback/private host. if (_is_local_server_provider(config_provider) or _base_url_points_at_local_server(config_base_url)): - return model_id, config_provider, config_base_url + return _finalize(model_id, config_provider, config_base_url) # Strip the provider prefix only when it's a known provider namespace # AND stripping is the right call for this configured provider: # @@ -2920,22 +3080,22 @@ def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> # 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 + return _finalize(model_id, config_provider, config_base_url) # (2) The endpoint's live/cached catalog advertised it. _advertised = _endpoint_advertised_model_ids(config_provider) if _advertised: # Full id advertised → route on it verbatim (#5979/#3872/#548). if model_id in _advertised: - return model_id, config_provider, config_base_url + return _finalize(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 + return _finalize(bare, config_provider, config_base_url) # Advertised but neither exact shape matched → intrinsic / # unknown prefix the proxy routes on; preserve it whole. - return model_id, config_provider, config_base_url + return _finalize(model_id, config_provider, config_base_url) # (3) Provenance genuinely unavailable (cold/unbuilt or # fingerprint-mismatched catalog AND not config-declared). # Distinguish a DELIBERATE selection from a stale leftover: @@ -2961,17 +3121,17 @@ def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> # the static first-party catalog silently flipped routing # (exactly how #5979 regressed). if explicitly_picked: - return model_id, config_provider, config_base_url + return _finalize(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 + return _finalize(bare, config_provider, config_base_url) + return _finalize(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 + return _finalize(bare, config_provider, config_base_url) # Intrinsic / unknown prefix — pass the full model_id through unchanged. - return model_id, config_provider, config_base_url + return _finalize(model_id, config_provider, config_base_url) # If prefix does NOT match config provider, the user picked a cross-provider model # from the OpenRouter dropdown (e.g. config=anthropic but picked openai/gpt-5.4-mini). @@ -2993,7 +3153,11 @@ def resolve_model_provider(model_id: str, *, explicitly_picked: bool = False) -> ): return model_id, "openrouter", None - return model_id, config_provider, config_base_url + # Final active-provider fallback: when nothing more specific matched, route + # on the configured provider. _finalize() fails closed here too if that + # provider is a collision-shared custom: (per finding #2 — the check + # must reach this last fallback, not just the earlier explicit paths). + return _finalize(model_id, config_provider, config_base_url) def resolve_custom_provider_connection(provider_id: str) -> tuple[str | None, str | None]: @@ -3007,13 +3171,7 @@ def resolve_custom_provider_connection(provider_id: str) -> tuple[str | None, st if not pid.startswith("custom:"): return None, None - def _slugify(value: str) -> str: - s = str(value or "").strip().lower().replace("_", "-").replace(" ", "-") - while "--" in s: - s = s.replace("--", "-") - return s.strip("-") - - slug = _slugify(pid.split(":", 1)[1].strip()) + slug = _custom_provider_slug_key(pid) if not slug: return None, None @@ -3041,18 +3199,13 @@ def resolve_custom_provider_connection(provider_id: str) -> tuple[str | None, st if not isinstance(custom_providers, list): custom_providers = [] - for entry in custom_providers: - if not isinstance(entry, dict): - continue - name = str(entry.get("name") or "").strip() - if not name: - continue - entry_slug = _slugify(name) - if entry_slug != slug: - continue - - base_url = str(entry.get("base_url") or "").strip() or None - api_key = _resolve_key(entry.get("api_key"), entry.get("key_env"), pid) + # Fail closed when the slug maps to multiple entries (raises); otherwise use + # the single matching entry. Shared with resolve_model_provider so endpoint + # and credential are always resolved from the SAME entry. + matched_entry = _unique_custom_provider_entry(custom_providers, slug) + if matched_entry is not None: + base_url = str(matched_entry.get("base_url") or "").strip() or None + api_key = _resolve_key(matched_entry.get("api_key"), matched_entry.get("key_env"), pid) return api_key, base_url # If exactly one custom provider is configured, use it as a pragmatic @@ -4849,12 +5002,51 @@ def set_auxiliary_model(task: str, provider: str, model: str, advanced: dict | N slot_cfg["provider"] = provider or "auto" slot_cfg["model"] = model or "" if provider and (provider.startswith("custom:") or provider == "custom"): - try: - _, _, resolved_base_url = resolve_model_provider(model) - if resolved_base_url: - slot_cfg["base_url"] = str(resolved_base_url).strip().rstrip("/") - except Exception: - pass + # Resolve the auxiliary slot's base_url against the SELECTED + # provider, not the active main provider. A bare + # resolve_model_provider(model) ignores `provider` and routes the + # model through whatever main provider is active — so when the + # selected auxiliary provider (custom:A) and the active main + # provider (custom:B) both list the same model id, the slot was + # persisted with provider=custom:A but base_url=B's endpoint + # (overlapping-id misroute, sibling of the resolve_model_provider + # fix). For a named custom: selection, look up that + # provider's OWN custom_providers[] entry directly. Note we do + # NOT route through model_with_provider_context here: the + # @custom::model form resolves base_url to None (the + # @provider path doesn't carry a custom entry's base_url), which + # would drop the base_url entirely. Fall back to the bare resolve + # only for the unnamed `custom` case, which has no own entry. + resolved_base_url = None + if provider.startswith("custom:"): + # Resolve the selected provider's base_url from the + # config_data already loaded under _cfg_lock above. Do NOT + # call resolve_custom_provider_connection() / get_config() + # here: they re-acquire the non-reentrant _cfg_lock we + # already hold, self-deadlocking whenever the cache is stale + # or the profile path changed. Use the shared uniqueness + # helper on the in-scope dict so this slug-only save fails + # closed on a collision (raises AmbiguousCustomProviderError) + # exactly like every other path — otherwise the ambiguity + # would be swallowed and the wrong endpoint persisted. + _cp_match = _unique_custom_provider_entry( + config_data.get("custom_providers", []), + _custom_provider_slug_key(provider), + ) + if _cp_match is not None: + resolved_base_url = str(_cp_match.get("base_url") or "").strip() or None + if not resolved_base_url: + # Best-effort fallback for the unnamed `custom` case (no own + # entry). Keep it non-fatal for unexpected errors, but let a + # genuine ambiguity propagate so the save fails closed. + try: + _, _, resolved_base_url = resolve_model_provider(model) + except AmbiguousCustomProviderError: + raise + except Exception: + resolved_base_url = None + if resolved_base_url: + slot_cfg["base_url"] = str(resolved_base_url).strip().rstrip("/") if advanced is not None: try: _apply_advanced_model_options(slot_cfg, advanced) diff --git a/api/routes.py b/api/routes.py index 1937725d..b0c69425 100644 --- a/api/routes.py +++ b/api/routes.py @@ -27051,14 +27051,28 @@ def _handle_handoff_summary(handler, body): resolved_model = None resolved_provider = None resolved_base_url = None + session_model_provider = None try: from api.models import get_session s_obj = get_session(sid) resolved_model = getattr(s_obj, "model", None) + # Carry the session's OWN selected provider into resolution. Without + # it, a bare resolve_model_provider(model) routes the summary through + # whatever main provider is active — so a session pinned to custom:A + # gets its handoff summary rerouted to the active custom:B when both + # providers list the same model id (overlapping-id misroute, sibling + # of the resolve_model_provider fix). model_with_provider_context + # encodes it as @custom:A:model so the resolver honors the session's + # endpoint; base_url is backfilled from that provider's own custom + # entry by the resolve_custom_provider_connection block below. + session_model_provider = getattr(s_obj, "model_provider", None) except Exception: pass - resolved_model, resolved_provider, resolved_base_url = _cfg.resolve_model_provider(resolved_model) + model_for_resolution = _cfg.model_with_provider_context( + resolved_model, session_model_provider + ) + resolved_model, resolved_provider, resolved_base_url = _cfg.resolve_model_provider(model_for_resolution) resolved_api_key = None try: @@ -27187,6 +27201,16 @@ def _handle_handoff_summary(handler, body): "type": "agent_runtime_stale", "retryable": True, }, status=409) + except api_config.AmbiguousCustomProviderError as e: + # A custom-provider slug collision is a user-fixable misconfiguration, + # not a transient summary failure. Return 400 with the actionable rename + # message so the UI shows it, instead of degrading to a 200 local + # fallback that the client treats as success and that hides the fix. + logger.warning("Handoff summary blocked by ambiguous custom provider: %s", e.message) + return j(handler, { + "error": e.message, + "type": "custom_provider_ambiguous", + }, status=400) except Exception as e: logger.warning("Handoff summary generation failed: %s", e) summary_text = _fallback_handoff_summary(msgs) diff --git a/api/streaming.py b/api/streaming.py index b3c10da9..57105d89 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -549,6 +549,7 @@ def _resolve_custom_provider_runtime_overrides( resolved_provider: str | None, resolved_api_key: str | None, resolved_base_url: str | None, + profile_name: str | None = None, ) -> tuple[str | None, str | None, str | None]: """Return provider/key/base_url overrides for ``custom:*`` endpoints. @@ -557,11 +558,24 @@ def _resolve_custom_provider_runtime_overrides( without authentication, so a missing key should not fail before the first request; pass a harmless placeholder to the SDK and let the endpoint accept it or return its own auth error. + + ``profile_name`` binds the SESSION's own profile while + ``resolve_custom_provider_connection`` reads the endpoint AND the credential. + That helper resolves both from ONE ``get_config()``/env snapshot, so binding + the profile here keeps them from splitting across profiles: on a detached + worker thread (which doesn't inherit the request-profile TLS/env) an unbound + call would read the DEFAULT profile and pair a named profile's endpoint with + the default profile's API key (finding #3). No-op for the default/root + profile; when ``profile_name`` is None the ambient scope (if any) is used. """ if not (isinstance(resolved_provider, str) and resolved_provider.startswith("custom:")): return resolved_provider, resolved_api_key, resolved_base_url - _cp_key, _cp_base = resolve_custom_provider_connection(resolved_provider) + from api import profiles as _profiles_api + with _profiles_api.profile_scope_for_detached_worker( + profile_name, "custom provider connection", logger_override=logger + ): + _cp_key, _cp_base = resolve_custom_provider_connection(resolved_provider) if not resolved_api_key and _cp_key: resolved_api_key = _cp_key if not resolved_base_url and _cp_base: @@ -9433,46 +9447,54 @@ def _run_agent_streaming( _sig_provider = getattr(s, "model_provider", None) or provider_context _current_sig = _mk_sig(_sig_model, _sig_provider) _explicitly_picked = bool(_picked_sig) and _picked_sig == _current_sig + # Resolve the endpoint AND the credential inside ONE profile scope so + # they always come from the same profile-owned snapshot. Previously + # only resolve_model_provider() ran under the scope; the runtime-key + # and custom-provider resolution below ran after it closed, so a + # detached worker for a NAMED profile paired that profile's endpoint + # with the DEFAULT profile's API key (finding #3). No-op for the + # default/root profile. with profiles_api.profile_scope_for_detached_worker( - _resolved_profile_name, "model resolution", logger_override=logger + _resolved_profile_name, "model + credential 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), explicitly_picked=_explicitly_picked, ) - configured_base_url = resolved_base_url + configured_base_url = resolved_base_url - # Resolve API key via Hermes runtime provider (matches gateway behaviour). - # Pass the resolved provider so non-default providers get their own credentials. - resolved_api_key = None - try: - from api.oauth import resolve_runtime_provider_with_anthropic_env_lock - from hermes_cli.runtime_provider import resolve_runtime_provider - _rt = resolve_runtime_provider_with_anthropic_env_lock( - resolve_runtime_provider, - requested=resolved_provider, - target_model=resolved_model, - ) - resolved_api_key = _rt.get("api_key") - if not resolved_provider: - resolved_provider = _rt.get("provider") - resolved_base_url = _runtime_preferred_base_url( - _rt, resolved_provider, configured_base_url - ) - except Exception as _e: - print(f"[webui] WARNING: resolve_runtime_provider failed: {_e}", flush=True) + # Resolve API key via Hermes runtime provider (matches gateway behaviour). + # Pass the resolved provider so non-default providers get their own credentials. + resolved_api_key = None + try: + from api.oauth import resolve_runtime_provider_with_anthropic_env_lock + from hermes_cli.runtime_provider import resolve_runtime_provider + _rt = resolve_runtime_provider_with_anthropic_env_lock( + resolve_runtime_provider, + requested=resolved_provider, + target_model=resolved_model, + ) + resolved_api_key = _rt.get("api_key") + if not resolved_provider: + resolved_provider = _rt.get("provider") + resolved_base_url = _runtime_preferred_base_url( + _rt, resolved_provider, configured_base_url + ) + except Exception as _e: + print(f"[webui] WARNING: resolve_runtime_provider failed: {_e}", flush=True) - # Named custom providers (custom:slug) may not be resolvable by - # hermes_cli.runtime_provider directly. Fall back to config.yaml - # custom_providers[] so WebUI can pass explicit creds/base_url. - # Preserve the pre-canonicalization identity so image routing can - # still select the exact custom_providers entry after the rewrite - # to "custom" below. - _session_requested_provider = resolved_provider - resolved_provider, resolved_api_key, resolved_base_url = _resolve_custom_provider_runtime_overrides( - resolved_provider, resolved_api_key, resolved_base_url - ) + # Named custom providers (custom:slug) may not be resolvable by + # hermes_cli.runtime_provider directly. Fall back to config.yaml + # custom_providers[] so WebUI can pass explicit creds/base_url. + # Preserve the pre-canonicalization identity so image routing can + # still select the exact custom_providers entry after the rewrite + # to "custom" below. + _session_requested_provider = resolved_provider + resolved_provider, resolved_api_key, resolved_base_url = _resolve_custom_provider_runtime_overrides( + resolved_provider, resolved_api_key, resolved_base_url, + profile_name=_resolved_profile_name, + ) # Read per-profile config at call time (not module-level snapshot). # The streaming worker is a detached thread that does NOT inherit the @@ -10508,10 +10530,18 @@ def _run_agent_streaming( # Before emitting the error, try re-reading credentials # and retrying once with a fresh agent. _heal_result = None - _heal_rt = _attempt_credential_self_heal( - resolved_provider or '', session_id, _agent_lock, - target_model=resolved_model, - ) + # Bind the session's profile so the self-heal re-resolve + # AND the custom-provider override below read one + # profile-owned snapshot (finding #3): otherwise a named + # profile's endpoint pairs with the default profile's key. + from api import profiles as _profiles_api + with _profiles_api.profile_scope_for_detached_worker( + _resolved_profile_name, "credential self-heal", logger_override=logger + ): + _heal_rt = _attempt_credential_self_heal( + resolved_provider or '', session_id, _agent_lock, + target_model=resolved_model, + ) if _heal_rt is not None: logger.info('[webui] self-heal: retrying stream after credential refresh') # Rebuild runtime variables from the refreshed resolve @@ -10530,7 +10560,8 @@ def _run_agent_streaming( if not _session_requested_provider: _session_requested_provider = resolved_provider resolved_provider, resolved_api_key, resolved_base_url = _resolve_custom_provider_runtime_overrides( - resolved_provider, resolved_api_key, resolved_base_url + resolved_provider, resolved_api_key, resolved_base_url, + profile_name=_resolved_profile_name, ) # Rebuild agent kwargs and create a fresh agent _agent_kwargs['api_key'] = resolved_api_key @@ -11749,10 +11780,18 @@ def _run_agent_streaming( elif _exc_is_auth: if not _self_healed: # ── Credential self-heal on 401 (#1401) ── - _heal_rt = _attempt_credential_self_heal( - resolved_provider or '', session_id, _agent_lock, - target_model=resolved_model, - ) + # Bind the session's profile so the self-heal re-resolve AND the + # custom-provider override below read one profile-owned snapshot + # (finding #3): otherwise a named profile's endpoint pairs with + # the default profile's key. + from api import profiles as _profiles_api + with _profiles_api.profile_scope_for_detached_worker( + _resolved_profile_name, "credential self-heal", logger_override=logger + ): + _heal_rt = _attempt_credential_self_heal( + resolved_provider or '', session_id, _agent_lock, + target_model=resolved_model, + ) if _heal_rt is not None: logger.info('[webui] self-heal (except path): retrying stream after credential refresh') _self_healed = True @@ -11771,7 +11810,8 @@ def _run_agent_streaming( if not _session_requested_provider: _session_requested_provider = resolved_provider resolved_provider, resolved_api_key, resolved_base_url = _resolve_custom_provider_runtime_overrides( - resolved_provider, resolved_api_key, resolved_base_url + resolved_provider, resolved_api_key, resolved_base_url, + profile_name=_resolved_profile_name, ) # Build a fresh agent with the new credentials _heal_kwargs = dict(_agent_kwargs) if '_agent_kwargs' in dir() else {} diff --git a/static/panels.js b/static/panels.js index 95b6ce7c..2e9acca4 100644 --- a/static/panels.js +++ b/static/panels.js @@ -12690,7 +12690,13 @@ async function _applyAuxModels(){ saved++; }catch(e){ console.warn('[settings] failed to save aux task',task.task,e); - if(typeof showToast==='function') showToast(t('settings_aux_save_failed')||'Failed to save auxiliary model'); + // Surface the server's actionable message (e.g. an ambiguous custom-provider + // slug collision: rename one provider so its slug is unique) instead of a + // generic failure, and abort the loop so the dirty selection is retained for + // the user to fix and retry — the reload that would clear it is skipped. + const _msg=(e&&e.message)?e.message:''; + const _base=t('settings_aux_save_failed')||'Failed to save auxiliary model'; + if(typeof showToast==='function') showToast(_msg?(_base+': '+_msg):_base,6000,'error'); return; } } @@ -12804,11 +12810,17 @@ async function saveSettings(andClose){ const saved=await _enqueueSettingsPost({method:'POST',body:JSON.stringify(payload)}); if(modelChanged && model){ try{ - await api('/api/default-model',{method:'POST',body:JSON.stringify({model,provider:modelState.model_provider||null})}); - body.default_model=model; - body.default_model_provider=(modelState&&modelState.model===model)?(modelState.model_provider||null):null; + await api('/api/default-model',{method:'POST',body:JSON.stringify({model,provider:modelState.model_provider||null})}); + body.default_model=model; + body.default_model_provider=(modelState&&modelState.model===model)?(modelState.model_provider||null):null; }catch(_modelErr){ - if(typeof showToast==='function') showToast('Failed to update default model — settings saved'); + // A 400 here (e.g. an ambiguous custom-provider slug collision: rename + // one provider) is user-fixable, not a partial success. Surface the + // message, abort before "settings saved", and retain dirty state so the + // user can fix and retry instead of the error being swallowed. + const _msg=(_modelErr&&_modelErr.message)?_modelErr.message:''; + if(typeof showToast==='function') showToast('Failed to update default model'+(_msg?(': '+_msg):''),6000,'error'); + return; } } _applySavedSettingsUi(saved, body, {sendKey,showTokenUsage,showQuotaChip,showConversationOutline,showBusyPlaceholderHint,showTps,fadeTextEffect,showCliSessions,theme,skin,language,sidebarDensity,fontSize}); @@ -12837,9 +12849,15 @@ async function saveSettings(andClose){ await api('/api/default-model',{method:'POST',body:JSON.stringify({model,provider:modelState.model_provider||null})}); body.default_model=model; body.default_model_provider=(modelState&&modelState.model===model)?(modelState.model_provider||null):null; - }catch(_modelErr){ - if(typeof showToast==='function') showToast('Failed to update default model — settings saved'); - } + }catch(_modelErr){ + // A 400 here (e.g. an ambiguous custom-provider slug collision: rename + // one provider) is user-fixable, not a partial success. Surface the + // message, abort before "settings saved", and retain dirty state so the + // user can fix and retry instead of the error being swallowed. + const _msg=(_modelErr&&_modelErr.message)?_modelErr.message:''; + if(typeof showToast==='function') showToast('Failed to update default model'+(_msg?(': '+_msg):''),6000,'error'); + return; + } } _applySavedSettingsUi(saved, body, {sendKey,showTokenUsage,showQuotaChip,showConversationOutline,showBusyPlaceholderHint,showTps,fadeTextEffect,showCliSessions,theme,skin,language,sidebarDensity,fontSize}); showToast(t('settings_saved')); diff --git a/static/sessions.js b/static/sessions.js index c73131f4..8cf39905 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -2918,12 +2918,20 @@ async function _generateHandoffSummary(sid, rounds) { } catch (e) { console.warn('Handoff summary failed:', e); if (S.session && S.session.session_id === sid && typeof setHandoffUi === 'function') { + // A 400 carries an actionable, user-fixable message (e.g. an ambiguous + // custom-provider slug collision: rename one provider so its slug is + // unique). Surface it verbatim rather than degrading to the generic + // "try again" card — previously the server answered 200 with a warning + // that this handler ignored, hiding the fix from the user. + const errorText = (e && e.status === 400 && e.message) + ? e.message + : ('Summary generation failed: ' + (e && e.message ? e.message : 'unknown error')); setHandoffUi({ sessionId: sid, phase: 'error', channel, rounds, - errorText: 'Summary generation failed: ' + e.message, + errorText, }); } } diff --git a/tests/test_auxiliary_models_settings.py b/tests/test_auxiliary_models_settings.py index 0456ac23..98c73579 100644 --- a/tests/test_auxiliary_models_settings.py +++ b/tests/test_auxiliary_models_settings.py @@ -937,3 +937,193 @@ class TestAuxiliaryModelsBackend: assert result["status"] == 400 assert "Unknown auxiliary task slot" in result["error"] + + def test_aux_slot_base_url_uses_selected_provider_not_active_endpoint( + self, monkeypatch, tmp_path + ): + """Overlapping-id sibling fix: persisting an auxiliary slot for a named + custom provider must record THAT provider's own base_url, not the active + main provider's endpoint. + + Repro: main provider is custom:dogapi (base_url dogapi), and both dogapi + and packyapi list 'shared-model'. Selecting custom:packyapi for the vision + slot previously persisted {provider: custom:packyapi, base_url: dogapi's} + because the base_url was resolved with a bare resolve_model_provider(model) + that ignores the selected provider. It must persist packyapi's base_url. + """ + from api import config + + shared_cfg = { + "model": { + "default": "shared-model", + "provider": "custom", + "base_url": "https://www.dogapi.cc/v1", + }, + "custom_providers": [ + {"name": "dogapi", "base_url": "https://www.dogapi.cc/v1", + "models": ["shared-model"]}, + {"name": "packyapi", "base_url": "https://www.packyapi.ai/v1", + "models": ["shared-model"]}, + ], + } + + config_path = tmp_path / "config.yaml" + import yaml + config_path.write_text(yaml.safe_dump(shared_cfg), encoding="utf-8") + monkeypatch.setattr(config, "_get_config_path", lambda: config_path) + monkeypatch.setattr(config, "reload_config", lambda: None) + # base_url resolution reads the in-memory cfg / get_config snapshot. + monkeypatch.setattr(config, "cfg", dict(shared_cfg)) + monkeypatch.setattr(config, "get_config", lambda: dict(shared_cfg)) + + config.set_auxiliary_model("vision", "custom:packyapi", "shared-model") + + saved = config._load_yaml_config_file(config_path)["auxiliary"]["vision"] + assert saved["provider"] == "custom:packyapi" + assert saved["base_url"] == "https://www.packyapi.ai/v1", ( + f"aux slot must persist the SELECTED provider's base_url, got " + f"{saved.get('base_url')!r}" + ) + + def test_aux_slot_custom_provider_base_url_no_deadlock(self, monkeypatch, tmp_path): + """Regression: saving a named custom auxiliary model must not self-deadlock. + + set_auxiliary_model() holds the non-reentrant _cfg_lock while resolving + the selected custom provider's base_url. The pre-fix code called + resolve_custom_provider_connection() -> get_config() -> + reload_config_if_stale(), which re-acquires _cfg_lock and hangs forever + whenever the config cache is stale or the profile path changed. This test + uses the REAL get_config (only reload_config, which runs AFTER the lock is + released, is stubbed) and forces get_config()'s reload branch, then runs + the call on a watchdog thread that fails the test if it hangs. + """ + import threading + + import yaml + + from api import config + + shared_cfg = { + "model": { + "default": "shared-model", + "provider": "custom", + "base_url": "https://www.dogapi.cc/v1", + }, + "custom_providers": [ + {"name": "dogapi", "base_url": "https://www.dogapi.cc/v1", + "models": ["shared-model"]}, + {"name": "packyapi", "base_url": "https://www.packyapi.ai/v1", + "models": ["shared-model"]}, + ], + } + + config_path = tmp_path / "config.yaml" + config_path.write_text(yaml.safe_dump(shared_cfg), encoding="utf-8") + monkeypatch.setattr(config, "_get_config_path", lambda: config_path) + # reload_config runs AFTER _cfg_lock is released; stub it so the test + # doesn't mutate global module state. get_config stays REAL — that is the + # path the pre-fix code re-entered while holding the lock. + monkeypatch.setattr(config, "reload_config", lambda: None) + # Force the reload branch inside the real get_config the pre-fix code + # called: a mismatched cached path makes path_changed True. + monkeypatch.setattr(config, "_cfg_path", None, raising=False) + + result_box: dict = {} + error_box: dict = {} + + def _run(): + try: + result_box["r"] = config.set_auxiliary_model( + "vision", "custom:packyapi", "shared-model" + ) + except Exception as exc: # pragma: no cover - surfaced via join + error_box["e"] = exc + + worker = threading.Thread(target=_run, daemon=True) + worker.start() + worker.join(timeout=10) + assert not worker.is_alive(), ( + "set_auxiliary_model deadlocked while resolving a custom provider " + "base_url under _cfg_lock" + ) + if "e" in error_box: + raise error_box["e"] + + saved = config._load_yaml_config_file(config_path)["auxiliary"]["vision"] + assert saved["provider"] == "custom:packyapi" + assert saved["base_url"] == "https://www.packyapi.ai/v1", ( + f"aux slot must persist the SELECTED provider's base_url, got " + f"{saved.get('base_url')!r}" + ) + + def test_aux_slot_custom_provider_slug_collision_fails_closed(self, monkeypatch, tmp_path): + """Saving a named custom auxiliary model whose slug collides with another + config entry must fail closed, not silently persist the wrong endpoint. + + The inline base_url resolution added for the deadlock fix shares the same + all-entry uniqueness helper, so a custom:foo-bar save with colliding + 'Foo Bar' + 'foo-bar' entries raises AmbiguousCustomProviderError and + writes nothing (the slot stays 'auto'). Operates on the in-scope + config_data, so it remains lock-safe. + """ + import yaml + + from api import config + + shared_cfg = { + "auxiliary": {"vision": {"provider": "auto", "model": ""}}, + "custom_providers": [ + {"name": "Foo Bar", "base_url": "https://a.example/v1"}, + {"name": "foo-bar", "base_url": "https://b.example/v1", + "models": ["shared-model"]}, + ], + } + + config_path = tmp_path / "config.yaml" + config_path.write_text(yaml.safe_dump(shared_cfg), encoding="utf-8") + monkeypatch.setattr(config, "_get_config_path", lambda: config_path) + monkeypatch.setattr(config, "reload_config", lambda: None) + + with pytest.raises(config.AmbiguousCustomProviderError): + config.set_auxiliary_model("vision", "custom:foo-bar", "shared-model") + + # The ambiguous save must not have persisted: the slot stays 'auto'. + saved = config._load_yaml_config_file(config_path)["auxiliary"]["vision"] + assert saved.get("provider") == "auto", ( + f"ambiguous aux save must not persist, got {saved!r}" + ) + + def test_aux_slot_parenthesized_name_collision_fails_closed(self, monkeypatch, tmp_path): + """Finding #1 on the aux persistence path: a parenthesized-name collision + that a looser slug key MISSED ('Foo (Bar)' vs 'foo-bar', both producing + custom:foo-bar) must also fail closed on save. + + This is the case the earlier collision key got wrong: it normalized + 'Foo (Bar)' to 'foo-(bar)' and never saw the collision, so the aux save + could persist endpoint A while the credential lookup later returned B. + """ + import yaml + + from api import config + + shared_cfg = { + "auxiliary": {"vision": {"provider": "auto", "model": ""}}, + "custom_providers": [ + {"name": "Foo (Bar)", "base_url": "https://a.example/v1"}, + {"name": "foo-bar", "base_url": "https://b.example/v1", + "models": ["shared-model"]}, + ], + } + + config_path = tmp_path / "config.yaml" + config_path.write_text(yaml.safe_dump(shared_cfg), encoding="utf-8") + monkeypatch.setattr(config, "_get_config_path", lambda: config_path) + monkeypatch.setattr(config, "reload_config", lambda: None) + + with pytest.raises(config.AmbiguousCustomProviderError): + config.set_auxiliary_model("vision", "custom:foo-bar", "shared-model") + + saved = config._load_yaml_config_file(config_path)["auxiliary"]["vision"] + assert saved.get("provider") == "auto", ( + f"ambiguous aux save must not persist, got {saved!r}" + ) diff --git a/tests/test_issue1013_handoff_dock.py b/tests/test_issue1013_handoff_dock.py index 87e4e5d0..59ed12ca 100644 --- a/tests/test_issue1013_handoff_dock.py +++ b/tests/test_issue1013_handoff_dock.py @@ -108,6 +108,28 @@ def test_handoff_delete_clears_local_storage_markers(): assert "ids.forEach(_clearHandoffStorageForSession);" in SESSIONS_JS +def test_handoff_catch_surfaces_400_message_verbatim(): + """Finding #5 (handoff UI): a 400 from the summary endpoint carries an + actionable message (e.g. an ambiguous custom-provider slug collision). The + catch must surface it verbatim instead of the generic 'try again' text.""" + assert "e && e.status === 400 && e.message" in SESSIONS_JS + # The generic 'try again' text must no longer be the sole error message. + assert "errorText," in SESSIONS_JS + + +def test_model_save_catches_surface_error_and_abort(): + """Finding #5 (settings UI): aux + default model saves must surface the + server's actionable message and abort (retain dirty state) rather than + swallowing it into a generic failure / 'settings saved'.""" + panels_js = (ROOT / "static" / "panels.js").read_text(encoding="utf-8") + # Default-model save no longer degrades to "settings saved" after a failure. + assert "Failed to update default model — settings saved" not in panels_js + # It surfaces the server message and aborts. + assert "const _msg=(_modelErr&&_modelErr.message)?_modelErr.message:''" in panels_js + # Aux save surfaces the server message too. + assert "const _base=t('settings_aux_save_failed')||'Failed to save auxiliary model';" in panels_js + + def test_handoff_summary_renders_as_transcript_card_not_dock_card(): assert "function setHandoffUi" in SESSIONS_JS or "function setHandoffUi" in (ROOT / "static" / "ui.js").read_text(encoding="utf-8") ui_js = (ROOT / "static" / "ui.js").read_text(encoding="utf-8") @@ -396,6 +418,83 @@ def test_exception_handoff_summary_persists_fallback_summary(monkeypatch): assert persisted[0]["rounds"] == models.CONVERSATION_ROUND_THRESHOLD +def test_ambiguous_custom_provider_handoff_returns_400_not_fallback(monkeypatch): + """Finding #4: a custom-provider slug collision on a handoff must surface as a + 400 with the actionable rename message — NOT a 200 local fallback. + + Previously AmbiguousCustomProviderError fell into the generic ``except + Exception`` handler that returns 200 with a ``warning`` the UI ignores, + marking the handoff handled and hiding the fix from the user. + """ + import api.routes as routes + import api.config as cfg + import api.models as models + + monkeypatch.setattr(routes, "require", lambda body, *keys: None) + monkeypatch.setattr( + routes, "j", + lambda _handler, payload, status=200, extra_headers=None: {**payload, "_status": status}, + ) + monkeypatch.setattr( + routes, "bad", + lambda _handler, msg, status=400: {"ok": False, "error": msg, "_status": status}, + ) + + persisted = [] + monkeypatch.setattr( + routes, "_persist_handoff_summary", + lambda sid, summary, channel, rounds, fallback=False: persisted.append(sid) or {"ok": True}, + ) + + monkeypatch.setattr(models, "count_conversation_rounds", lambda sid, since=None: models.CONVERSATION_ROUND_THRESHOLD) + monkeypatch.setattr( + models, "get_cli_session_messages", + lambda sid: [ + {"role": "user", "content": "Could you check this?", "timestamp": 1.0}, + {"role": "assistant", "content": "Sure, I can help", "timestamp": 2.0}, + ], + ) + + # require_ai_agent_class() imports run_agent and hermes_cli.runtime_provider + # before model resolution; stub them so execution reaches resolve_model_provider. + fake_runtime_module = types.ModuleType("hermes_cli.runtime_provider") + fake_runtime_module.resolve_runtime_provider = lambda requested=None: { + "api_key": "x", "provider": "openrouter", "base_url": None, + } + fake_hermes_cli = types.ModuleType("hermes_cli") + fake_hermes_cli.__path__ = [] + fake_hermes_cli.runtime_provider = fake_runtime_module + monkeypatch.setitem(sys.modules, "hermes_cli", fake_hermes_cli) + monkeypatch.setitem(sys.modules, "hermes_cli.runtime_provider", fake_runtime_module) + + class _DummyAgent: + def __init__(self, *args, **kwargs): + pass + + fake_run_agent = types.ModuleType("run_agent") + fake_run_agent.AIAgent = _DummyAgent + monkeypatch.setitem(sys.modules, "run_agent", fake_run_agent) + + rename_msg = ( + "Custom providers ['Foo (Bar)', 'foo-bar'] all normalize to the same " + "provider slug 'foo-bar'; rename one so each has a unique slug." + ) + + def _raise_ambiguous(*args, **kwargs): + raise cfg.AmbiguousCustomProviderError(rename_msg) + + monkeypatch.setattr(cfg, "resolve_model_provider", _raise_ambiguous) + + response = routes._handle_handoff_summary(object(), {"session_id": "session-ambiguous"}) + + assert response.get("_status") == 400, response + assert response.get("type") == "custom_provider_ambiguous", response + assert response.get("error") == rename_msg + # Must NOT have silently degraded to a persisted fallback summary. + assert persisted == [], "ambiguity must not persist a 200 fallback summary" + assert "summary" not in response + + def test_handoff_summary_retries_once_when_length_limit_reached(monkeypatch): """finish_reason='length' should trigger one retry with larger budget.""" import api.routes as routes diff --git a/tests/test_issue2271_keyless_custom_provider.py b/tests/test_issue2271_keyless_custom_provider.py index 93aaee9e..875bf9ee 100644 --- a/tests/test_issue2271_keyless_custom_provider.py +++ b/tests/test_issue2271_keyless_custom_provider.py @@ -55,6 +55,54 @@ def test_named_custom_provider_keeps_existing_runtime_base_url(monkeypatch): assert base_url == "http://runtime.example/v1" +def test_custom_provider_overrides_resolve_connection_within_profile_scope(monkeypatch): + """Finding #3: the custom endpoint AND credential are resolved inside the + session's OWN profile scope. + + ``resolve_custom_provider_connection`` reads both the base_url and the API key + from one ``get_config()``/env snapshot. On a detached worker thread (no + inherited request-profile TLS/env) an unbound call reads the DEFAULT profile, + pairing a named profile's endpoint with the default profile's key. The + overrides helper must run that lookup under the passed profile scope. + """ + import contextlib + import api.streaming as streaming + import api.profiles as profiles + + scope_state = {"active": False, "name": None} + + @contextlib.contextmanager + def _fake_scope(name, purpose="", logger_override=None): + scope_state["active"] = True + scope_state["name"] = name + try: + yield + finally: + scope_state["active"] = False + + monkeypatch.setattr(profiles, "profile_scope_for_detached_worker", _fake_scope) + + seen = {} + + def _fake_conn(provider): + # Endpoint + credential must both be read WHILE the profile scope is active. + seen["scope_active"] = scope_state["active"] + seen["scope_name"] = scope_state["name"] + return ("team-key", "https://team.example/v1") + + monkeypatch.setattr(streaming, "resolve_custom_provider_connection", _fake_conn) + + provider, api_key, base_url = streaming._resolve_custom_provider_runtime_overrides( + "custom:team", None, None, profile_name="team", + ) + + assert seen["scope_active"] is True, "connection resolved OUTSIDE the profile scope" + assert seen["scope_name"] == "team" + assert provider == "custom" + assert api_key == "team-key" + assert base_url == "https://team.example/v1" + + def test_non_custom_provider_is_unchanged(monkeypatch): import api.streaming as streaming diff --git a/tests/test_model_resolver.py b/tests/test_model_resolver.py index 0dc559f4..51855264 100644 --- a/tests/test_model_resolver.py +++ b/tests/test_model_resolver.py @@ -253,6 +253,405 @@ def test_custom_provider_model_with_slash_routes_to_named_custom_provider(): assert base_url == 'http://lmstudio.local:1234/v1' +# ── Overlapping custom_providers[] model ids — active endpoint wins ───────── + +def test_overlapping_custom_providers_active_base_url_wins_not_config_order(): + """When two custom_providers[] list the SAME bare model id, routing must + follow the ACTIVE provider (resolved from model.base_url → named slug), NOT + config write order. + + Real-world repro: dogapi and packyapi both advertise 'claude-sonnet-5'; + dogapi is written FIRST in config. A plain first-match scan hijacked the + model to dogapi even though model.base_url pointed at packyapi. The active + endpoint the user configured must win. + """ + custom_providers = [ + {'name': 'dogapi', 'base_url': 'https://www.dogapi.cc/v1', + 'models': ['claude-sonnet-5', 'gpt-5.6-sol', 'musk-4.5']}, + {'name': 'packyapi', 'base_url': 'https://www.packyapi.ai/v1', + 'models': ['claude-sonnet-5', 'claude-opus-5']}, + ] + # Active endpoint is packyapi (bare 'custom' + base_url resolves to the slug). + model, provider, base_url = _resolve_with_config( + 'claude-sonnet-5', + provider='custom', + base_url='https://www.packyapi.ai/v1', + custom_providers=custom_providers, + ) + assert provider == 'custom:packyapi', ( + f"shared model must route to the ACTIVE packyapi endpoint, got {provider!r}" + ) + assert base_url == 'https://www.packyapi.ai/v1' + + +def test_overlapping_custom_providers_unique_model_still_routes_by_ownership(): + """A model that only ONE overlapping provider lists still routes to that + provider even when the active endpoint is the other one — the active-slug + guard only claims models the active provider actually owns, then falls + through to the ordered ownership scan. + """ + custom_providers = [ + {'name': 'dogapi', 'base_url': 'https://www.dogapi.cc/v1', + 'models': ['claude-sonnet-5', 'gpt-5.6-sol', 'musk-4.5']}, + {'name': 'packyapi', 'base_url': 'https://www.packyapi.ai/v1', + 'models': ['claude-sonnet-5', 'claude-opus-5']}, + ] + # Active endpoint packyapi, but 'gpt-5.6-sol' is dogapi-only → must go dogapi. + model, provider, base_url = _resolve_with_config( + 'gpt-5.6-sol', + provider='custom', + base_url='https://www.packyapi.ai/v1', + custom_providers=custom_providers, + ) + assert provider == 'custom:dogapi', ( + f"dogapi-only model must still route to dogapi, got {provider!r}" + ) + assert base_url == 'https://www.dogapi.cc/v1' + + +def test_overlapping_custom_providers_bare_custom_no_base_url_keeps_order(): + """With a bare 'custom' provider and NO base_url to disambiguate, the active + slug can't be resolved, so the legacy config-order first-match behaviour is + preserved (no regression for users who never set a base_url). + """ + custom_providers = [ + {'name': 'dogapi', 'base_url': 'https://www.dogapi.cc/v1', + 'models': ['claude-sonnet-5']}, + {'name': 'packyapi', 'base_url': 'https://www.packyapi.ai/v1', + 'models': ['claude-sonnet-5']}, + ] + model, provider, base_url = _resolve_with_config( + 'claude-sonnet-5', + provider='custom', + custom_providers=custom_providers, + ) + assert provider == 'custom:dogapi', ( + f"with no base_url to disambiguate, first-match order is preserved, got {provider!r}" + ) + + +def test_overlapping_custom_providers_normalized_slug_collision_fails_closed(): + """Two DISTINCT provider names that normalize to the SAME slug must fail + closed, even when the active base_url pins one exact entry. + + 'Foo Bar' and 'foo-bar' both normalize to slug custom:foo-bar. Even with the + active base_url pointing at the SECOND entry (B), resolve_model_provider can + only return the shared slug custom:foo-bar. The downstream credential lookup + (resolve_custom_provider_connection) then resolves the API key from the FIRST + same-slug entry (A) regardless of base_url — so endpoint B would be paired + with credential A. Rather than emit that broken pairing, resolution must + raise so the collision surfaces to the user. + """ + custom_providers = [ + {'name': 'Foo Bar', 'base_url': 'https://a.example/v1', 'models': ['shared-model']}, + {'name': 'foo-bar', 'base_url': 'https://b.example/v1', 'models': ['shared-model']}, + ] + with pytest.raises(config.AmbiguousCustomProviderError): + _resolve_with_config( + 'shared-model', + provider='custom', + base_url='https://b.example/v1', + custom_providers=custom_providers, + ) + + +def test_overlapping_custom_providers_slug_collision_no_base_url_fails_closed(): + """When two same-slug entries exist and there is NO base_url to disambiguate, + resolution must fail closed rather than silently guessing the first entry. + The ordered scan would return the shared slug custom:foo-bar, whose credential + lookup can't be pinned to a single entry — so it raises instead. + """ + custom_providers = [ + {'name': 'Foo Bar', 'base_url': 'https://a.example/v1', 'models': ['shared-model']}, + {'name': 'foo-bar', 'base_url': 'https://b.example/v1', 'models': ['shared-model']}, + ] + # Active provider is bare 'custom' with no base_url: nothing disambiguates. + with pytest.raises(config.AmbiguousCustomProviderError): + _resolve_with_config( + 'shared-model', + provider='custom', + custom_providers=custom_providers, + ) + + +def test_overlapping_custom_providers_explicit_slug_wins_over_stale_base_url(): + """An explicitly selected named custom provider with a UNIQUE slug must win + even when model.base_url is stale and points at neither entry. + + A (dogapi) is written first; B (packyapi) is explicitly selected. Both own + 'shared-model'. model.base_url is set to a THIRD, stale URL that matches no + entry. The explicit, unambiguous provider B must be authoritative — a stale + URL is not evidence to demote it to config order (which would return A). + """ + custom_providers = [ + {'name': 'dogapi', 'base_url': 'https://www.dogapi.cc/v1', + 'models': ['shared-model']}, + {'name': 'packyapi', 'base_url': 'https://www.packyapi.ai/v1', + 'models': ['shared-model']}, + ] + model, provider, base_url = _resolve_with_config( + 'shared-model', + provider='custom:packyapi', + base_url='https://stale.example/v1', # stale: matches neither entry + custom_providers=custom_providers, + ) + assert provider == 'custom:packyapi', ( + f"explicit provider must win over stale base_url + config order, got {provider!r}" + ) + assert base_url == 'https://www.packyapi.ai/v1', ( + f"must use the selected provider's own base_url, got {base_url!r}" + ) + + +def test_session_provider_context_routes_handoff_to_session_endpoint_not_active(): + """Handoff-summary sibling fix: a session pinned to custom:A must resolve + through A, not the active custom:B, when both list the same model id. + + The handoff summary path reads s_obj.model + s_obj.model_provider and passes + model_with_provider_context(model, model_provider) into resolve_model_provider. + This pins the routing behavior that fix relies on: encoding the SESSION's own + provider overrides the active endpoint. (The handoff path then backfills + base_url from resolve_custom_provider_connection, so base_url=None here is + expected and fine.) + """ + custom_providers = [ + {'name': 'dogapi', 'base_url': 'https://www.dogapi.cc/v1', + 'models': ['shared-model']}, + {'name': 'packyapi', 'base_url': 'https://www.packyapi.ai/v1', + 'models': ['shared-model']}, + ] + old_cfg = dict(config.cfg) + config.cfg['model'] = { + 'default': 'shared-model', 'provider': 'custom', + 'base_url': 'https://www.dogapi.cc/v1', # active endpoint = dogapi + } + config.cfg['custom_providers'] = custom_providers + try: + # Session is pinned to packyapi even though dogapi is active. + encoded = config.model_with_provider_context('shared-model', 'custom:packyapi') + model, provider, _base = config.resolve_model_provider(encoded) + finally: + config.cfg.clear() + config.cfg.update(old_cfg) + assert provider == 'custom:packyapi', ( + f"session's own provider must win over the active endpoint, got {provider!r}" + ) + + +def test_overlapping_custom_providers_asymmetric_collision_fails_closed(): + """ASYMMETRIC normalized-slug collision must fail closed too. + + Reviewer-reproduced shape: entry A ('Foo Bar') appears FIRST and does NOT + list 'shared-model'; entry B ('foo-bar') lists it and is the active endpoint. + Both normalize to custom:foo-bar. Ownership-only collision detection would + see just B and happily return B's endpoint + custom:foo-bar — but the + credential lookup scans by slug and first-matches A, pairing B's endpoint + with A's credential. Membership must be built from ALL named entries + (ownership-independent), so this raises. + """ + custom_providers = [ + {'name': 'Foo Bar', 'base_url': 'https://a.example/v1'}, # first, NON-owner + {'name': 'foo-bar', 'base_url': 'https://b.example/v1', 'models': ['shared-model']}, + ] + with pytest.raises(config.AmbiguousCustomProviderError): + _resolve_with_config( + 'shared-model', + provider='custom', + base_url='https://b.example/v1', # active endpoint = B + custom_providers=custom_providers, + ) + + +def test_overlapping_custom_providers_asymmetric_collision_bare_custom_fails_closed(): + """Same asymmetric collision on the bare-'custom', no-base_url ordered-scan + path: the owning entry B is returned as custom:foo-bar, but non-owner A + shares the slug, so the ordered scan must also fail closed. + """ + custom_providers = [ + {'name': 'Foo Bar', 'base_url': 'https://a.example/v1'}, # first, NON-owner + {'name': 'foo-bar', 'base_url': 'https://b.example/v1', 'models': ['shared-model']}, + ] + with pytest.raises(config.AmbiguousCustomProviderError): + _resolve_with_config( + 'shared-model', + provider='custom', + custom_providers=custom_providers, + ) + + +def test_provider_qualified_custom_hint_collision_fails_closed(): + """The @custom::model qualified path (session/send/handoff shape via + model_with_provider_context) must apply the same all-entry uniqueness check. + + Without it, the qualified return hands back custom:foo-bar and the credential + backfill first-matches the wrong entry. Uses the asymmetric shape so the + check cannot rely on model ownership of the encoded string. + """ + custom_providers = [ + {'name': 'Foo Bar', 'base_url': 'https://a.example/v1'}, # first, NON-owner + {'name': 'foo-bar', 'base_url': 'https://b.example/v1', 'models': ['shared-model']}, + ] + old_cfg = dict(config.cfg) + config.cfg['model'] = { + 'default': 'shared-model', 'provider': 'custom', + 'base_url': 'https://a.example/v1', + } + config.cfg['custom_providers'] = custom_providers + try: + encoded = config.model_with_provider_context('shared-model', 'custom:foo-bar') + with pytest.raises(config.AmbiguousCustomProviderError): + config.resolve_model_provider(encoded) + finally: + config.cfg.clear() + config.cfg.update(old_cfg) + + +def test_resolve_custom_provider_connection_collision_fails_closed(monkeypatch): + """The credential boundary itself must fail closed on a slug collision so an + endpoint and API key can never be resolved from different entries — even when + called directly (e.g. the handoff/context-length credential backfill). + """ + custom_providers = [ + {'name': 'Foo Bar', 'base_url': 'https://a.example/v1', 'api_key': 'key-A'}, + {'name': 'foo-bar', 'base_url': 'https://b.example/v1', 'api_key': 'key-B'}, + ] + monkeypatch.setattr(config, 'get_config', lambda: {'custom_providers': custom_providers}) + with pytest.raises(config.AmbiguousCustomProviderError): + config.resolve_custom_provider_connection('custom:foo-bar') + + +# ── Finding #1: one canonical slug identity (_custom_provider_slug_from_name) ── +# The collision key MUST match the slug PRODUCER. A parenthesized / punctuated +# name is where a looser key diverged: 'Foo (Bar)' is produced as custom:foo-bar +# but a naive lower+space/underscore key yields 'foo-(bar)' — so the collision +# with a literal 'foo-bar' entry was MISSED, reintroducing the original bug +# (endpoint A + credential B) for these names. These regressions collide two +# names ONLY via the producer. + +def test_slug_key_matches_producer_for_parenthesized_name(): + """The collision key derives from the SAME producer that mints the returned + slug, so genuinely-colliding names are seen as identical.""" + assert config._custom_provider_slug_from_name('Foo (Bar)') == 'custom:foo-bar' + assert config._custom_provider_slug_from_name('foo-bar') == 'custom:foo-bar' + # Both map to the same bare collision key. + assert config._custom_provider_slug_key('Foo (Bar)') == config._custom_provider_slug_key('foo-bar') + assert config._custom_provider_slug_key('custom:foo-bar') == 'foo-bar' + + +def _parenthesized_collision_providers(): + # 'Foo (Bar)' and 'foo-bar' BOTH produce custom:foo-bar, but only collide + # under the producer's normalization (a looser key would miss them). + return [ + {'name': 'Foo (Bar)', 'base_url': 'https://a.example/v1', 'api_key': 'key-A', + 'models': ['shared-model']}, + {'name': 'foo-bar', 'base_url': 'https://b.example/v1', 'api_key': 'key-B', + 'models': ['shared-model']}, + ] + + +def test_parenthesized_name_collision_fails_closed_bare_resolution(): + """Finding #1: bare resolution must fail closed on a producer-level collision + that a looser key missed ('Foo (Bar)' vs 'foo-bar').""" + with pytest.raises(config.AmbiguousCustomProviderError): + _resolve_with_config( + 'shared-model', + provider='custom', + base_url='https://b.example/v1', + custom_providers=_parenthesized_collision_providers(), + ) + + +def test_parenthesized_name_collision_fails_closed_qualified_hint(): + """Finding #1: the @custom::model qualified path must also catch the + parenthesized-name collision.""" + old_cfg = dict(config.cfg) + config.cfg['model'] = {'default': 'shared-model', 'provider': 'custom', + 'base_url': 'https://a.example/v1'} + config.cfg['custom_providers'] = _parenthesized_collision_providers() + try: + encoded = config.model_with_provider_context('shared-model', 'custom:foo-bar') + with pytest.raises(config.AmbiguousCustomProviderError): + config.resolve_model_provider(encoded) + finally: + config.cfg.clear() + config.cfg.update(old_cfg) + + +def test_parenthesized_name_collision_fails_closed_credential_boundary(monkeypatch): + """Finding #1: the credential lookup must fail closed on the parenthesized + collision so endpoint and key can never split.""" + monkeypatch.setattr( + config, 'get_config', + lambda: {'custom_providers': _parenthesized_collision_providers()}, + ) + with pytest.raises(config.AmbiguousCustomProviderError): + config.resolve_custom_provider_connection('custom:foo-bar') + + +# ── Finding #2: the ambiguity check runs only at the point of return, so an +# unrelated collision never blocks a request that resolves to a DIFFERENT +# provider. With a colliding slug ACTIVE, unrelated lanes must still route. + +def _collision_plus_safe_lane_config(): + old_cfg = dict(config.cfg) + config.cfg['model'] = { + 'default': 'shared-model', + 'provider': 'custom:foo-bar', # the ACTIVE provider is the colliding slug + 'base_url': 'https://b.example/v1', + } + # 'Foo Bar' + 'foo-bar' collide under BOTH the old and new normalizers, so + # the PRE-FIX up-front check reproduces the over-block (every lane raises); + # the fix moves the check to the point of return so only the colliding lane + # fails. (Parenthesized-name collisions are covered by the finding #1 tests.) + config.cfg['custom_providers'] = [ + {'name': 'Foo Bar', 'base_url': 'https://a.example/v1', 'models': ['shared-model']}, + {'name': 'foo-bar', 'base_url': 'https://b.example/v1', 'models': ['shared-model']}, + {'name': 'safe-provider', 'base_url': 'https://safe.example/v1', + 'models': ['safe-model']}, + ] + return old_cfg + + +def test_unrelated_openrouter_lane_not_blocked_by_active_collision(): + """Finding #2: an @openrouter hint must resolve even though the ACTIVE custom + slug (custom:foo-bar) is ambiguous — the check must not run up front.""" + old_cfg = _collision_plus_safe_lane_config() + try: + model, provider, _base = config.resolve_model_provider('@openrouter:gpt-x') + assert provider == 'openrouter', provider + assert model == 'gpt-x', model + finally: + config.cfg.clear() + config.cfg.update(old_cfg) + + +def test_unrelated_custom_lane_not_blocked_by_active_collision(): + """Finding #2: a DIFFERENT, non-colliding @custom:safe-provider hint must + resolve even though the active slug collides.""" + old_cfg = _collision_plus_safe_lane_config() + try: + encoded = config.model_with_provider_context('safe-model', 'custom:safe-provider') + model, provider, base_url = config.resolve_model_provider(encoded) + assert provider == 'custom:safe-provider', provider + assert model == 'safe-model', model + finally: + config.cfg.clear() + config.cfg.update(old_cfg) + + +def test_active_collision_still_fails_closed_for_owned_model(): + """Finding #2 counterpart: when the request DOES resolve to the colliding + active slug (its owned default model), it must still fail closed — the + point-of-return check fires exactly when the slug is consumed.""" + old_cfg = _collision_plus_safe_lane_config() + try: + with pytest.raises(config.AmbiguousCustomProviderError): + config.resolve_model_provider('shared-model') + finally: + config.cfg.clear() + config.cfg.update(old_cfg) + + # ── #3872: bare ``custom`` provider is a vendor-routing proxy — preserve the # full model id (the prefix is intrinsic). #433's redundant-prefix strip is # scoped to real first-party providers (provider=openai + proxy base_url),