From a2bc809436067bff0d7640f6350bb2e959fc9146 Mon Sep 17 00:00:00 2001 From: KT Date: Fri, 31 Jul 2026 23:38:42 +0800 Subject: [PATCH] fix(*): stop a static read from starting a login, and ask Azure for its endpoint Three LiteLLM drivers ship a device-flow authenticator, and every entry point that resolves a model reaches it. With no token file on disk, reading a Copilot model's context window printed six GitHub device codes to stdout and blocked for 410 seconds: two full three-attempt login cycles, because the bare and the openrouter-prefixed candidate reach the same driver. session.create runs that read before the first turn, so the symptom was a gateway that hung on opening the picker, and the agent loop runs it again every turn. The cost estimate, which runs after every call, hung the same way through cost_per_token. One function answers whether a model can be handed to LiteLLM at all, and both lookups consult it; a guard test fails if either decides for itself. It asks the installed package -- a driver either ships authenticator.py or it does not -- rather than carrying a list that would need regenerating on every LiteLLM bump and would bring the hang back for whatever a stale copy missed. The window lookup reads the price table before asking, which answers all 49 rows those three drivers have between them. The ask stays for everything that cannot prompt, because it does more than normalize keys: for an openrouter-prefixed candidate it derives OpenRouter's own numbers, which are in no row. Reading the table with a prefix-stripping fallback looked like a free replacement and was not -- it answered three MiniMax models with the direct figure where LiteLLM had reported OpenRouter's. Verified across the 132 candidates Raven offers: no window and no rate differs from before. Azure OpenAI could be picked from the curated list and never worked. Its endpoint contains the tenant's own resource name, so there is nothing to default to, and its client raises on an empty api_base -- but the wizard classified the need for an endpoint by name, matching only the self-hosted entry, and asked Azure for a key alone. The model picker knew better and kept its own list of the two, which is the same fact answered twice with one answer wrong. It is a registry field now, and both read it. Co-authored-by: Claude (claude-opus-5[1m]) --- raven/cli/onboard_commands.py | 6 +- raven/providers/registry.py | 9 ++ raven/token_wise/pricing.py | 99 +++++++++++++++-- raven/tui_rpc/methods/model.py | 14 ++- tests/test_cli_onboard_commands.py | 61 ++++++++++ tests/test_token_wise_pricing.py | 171 ++++++++++++++++++++++++++++- 6 files changed, 348 insertions(+), 12 deletions(-) diff --git a/raven/cli/onboard_commands.py b/raven/cli/onboard_commands.py index f75330cfe..64d324a61 100644 --- a/raven/cli/onboard_commands.py +++ b/raven/cli/onboard_commands.py @@ -952,7 +952,11 @@ def _credential_kind(provider: str, spec: Any) -> str: return CRED_OAUTH if spec is not None and spec.is_local: return CRED_LOCAL - if provider == "custom": + if spec is not None and spec.requires_api_base: + # Not a name check any more: Azure needs the same pair and was asked only + # for a key, so it was configured with no endpoint and could not be + # called. The TUI already knew -- it kept its own list of the two -- which + # is the same fact answered twice, once wrongly. return CRED_ENDPOINT return CRED_KEY diff --git a/raven/providers/registry.py b/raven/providers/registry.py index bae2dc935..52a72b1f4 100644 --- a/raven/providers/registry.py +++ b/raven/providers/registry.py @@ -86,6 +86,13 @@ class ProviderSpec: # over OAuth). They take no LiteLLM route prefix. bypasses_litellm: bool = False + # The endpoint is the user's to supply and there is no default that works: + # Azure gives every tenant its own resource URL, a self-hosted endpoint is + # wherever the user put it. Distinct from `default_api_base`, which is a + # working address the user may override, and from `is_local`, which needs an + # address but no key. + requires_api_base: bool = False + @property def label(self) -> str: return self.display_name or self.name.title() @@ -161,6 +168,7 @@ def claims(self, model: str) -> bool: # empty). via_driver="openai", is_gateway=True, + requires_api_base=True, default_api_base="http://localhost:8000/v1", ), # === Azure OpenAI ====================================================== @@ -173,6 +181,7 @@ def claims(self, model: str) -> bool: env_key="", display_name="Azure OpenAI", bypasses_litellm=True, + requires_api_base=True, ), # === Gateways (detected by api_key / api_base, not model name) ========= # Gateways can route any model, so they win in fallback. diff --git a/raven/token_wise/pricing.py b/raven/token_wise/pricing.py index 1433c7a86..eff099993 100644 --- a/raven/token_wise/pricing.py +++ b/raven/token_wise/pricing.py @@ -23,7 +23,9 @@ from __future__ import annotations +import pathlib import time +from functools import lru_cache import httpx from loguru import logger @@ -50,6 +52,80 @@ _OPENROUTER_CACHE_TIME: float = 0.0 +def _litellm_price_table() -> dict: + """LiteLLM's static price table, or an empty dict if it cannot be imported.""" + try: + from raven.providers.litellm_setup import import_litellm + + return getattr(import_litellm(), "model_cost", None) or {} + except Exception: + return {} + + +def _table_entry(model: str) -> dict | None: + """The table row keyed exactly by this model id, or None. + + Deliberately no prefix-stripping fallback. It looked like a free replacement + for asking LiteLLM, but the ask does more than key normalization: for an + "openrouter//" candidate it derives OpenRouter's own numbers, + which are in no table row -- and stripping to the direct row answered three + MiniMax models with the direct figure where LiteLLM had reported OpenRouter's. + The table is read here to skip the ask where the ask cannot be made; it does + not replace it. + """ + entry = _litellm_price_table().get(model) + return entry if isinstance(entry, dict) else None + + +@lru_cache(maxsize=1) +def _drivers_dir() -> pathlib.Path | None: + """Where the installed LiteLLM keeps its per-provider drivers.""" + try: + from raven.providers.litellm_setup import import_litellm + + return pathlib.Path(import_litellm().__file__).parent / "llms" + except Exception: + return None + + +def _may_prompt(model: str) -> bool: + """Would handing this model to LiteLLM start an interactive login? + + Three of its drivers ship a device-flow authenticator, and every entry point + that resolves a model reaches it -- ``get_model_info``, ``cost_per_token`` and + ``validate_environment`` alike. With no token file the call prints a device + code to stdout and blocks about a minute per attempt, three attempts per + candidate: one window lookup cost 410 seconds and six codes, because the bare + and the openrouter-prefixed candidate reach the same driver. That is why any + segment counts, not just the first. + + Asked of the installed package rather than a snapshot of it -- a driver is one + that ships ``authenticator.py``. A frozen list would have to be regenerated on + every LiteLLM bump, and a stale one brings the hang back for the vendor it + missed; this cannot go stale. The check is a stat, and the callers have already + paid for the import. + """ + drivers = _drivers_dir() + if drivers is None: + return False + return any((drivers / part / "authenticator.py").exists() for part in model.split("/") if part) + + +def _numeric(entry: dict | None, *fields: str) -> float | None: + """First numeric value among ``fields``, or None. + + The table ships a self-documenting sample row whose numeric-looking fields + hold prose, so the type check is load-bearing rather than defensive. + """ + if not isinstance(entry, dict): + return None + for field in fields: + value = entry.get(field) + if isinstance(value, (int, float)) and value: + return float(value) + return None + + def _try_litellm_rates(model: str, input_tokens: int, output_tokens: int) -> tuple[float, float] | None: """Ask LiteLLM for per-token rates. Returns (prompt_rate, completion_rate) or None.""" try: @@ -69,6 +145,13 @@ def _try_litellm_rates(model: str, input_tokens: int, output_tokens: int) -> tup probe_out = output_tokens if output_tokens else 1 for candidate in candidates: + if _may_prompt(candidate): + # Skipped, not read from the table: the rows these families have are + # priced at zero, which this function already treats as unknown, so + # reading them would add a branch that cannot fire. The caller falls + # through to the live OpenRouter catalogue and the manual table, which + # is what any model LiteLLM does not price already does. + continue try: prompt_cost, completion_cost = litellm.cost_per_token( model=candidate, prompt_tokens=probe_in, completion_tokens=probe_out @@ -179,18 +262,20 @@ def _try_litellm_context_window(model: str) -> int | None: candidates.insert(0, f"openrouter/{model}") for candidate in candidates: + # The table before the ask: it holds every model the interactive-login + # drivers are asked about in practice, and reading it cannot prompt. + window = _numeric(_table_entry(candidate), "max_input_tokens", "max_tokens") + if window: + return int(window) + if _may_prompt(candidate): + continue try: info = litellm.get_model_info(candidate) except Exception: continue - if not info: - continue - window = info.get("max_input_tokens") or info.get("max_tokens") + window = _numeric(info, "max_input_tokens", "max_tokens") if window: - try: - return int(window) - except (TypeError, ValueError): - continue + return int(window) return None diff --git a/raven/tui_rpc/methods/model.py b/raven/tui_rpc/methods/model.py index ef2226f38..565e271c6 100644 --- a/raven/tui_rpc/methods/model.py +++ b/raven/tui_rpc/methods/model.py @@ -48,7 +48,15 @@ from raven.tui_rpc.dispatcher import Dispatcher -_NEEDS_API_BASE = {"custom", "azure_openai"} +def _needs_api_base(slug: str) -> bool: + """Does this provider have no usable endpoint until the user gives one? + + Read from the registry rather than listed here: the wizard answered the same + question from the spec and the two drifted, leaving Azure configurable with no + endpoint at all. + """ + spec = find_by_name(slug) + return bool(spec is not None and spec.requires_api_base) def _parse(model_cls: type, params: dict) -> Any: @@ -104,7 +112,7 @@ def _build_provider_entry(slug: str, *, current_provider: str | None) -> dict[st "key_env": (spec.env_key or None) if spec else None, "models": models, "total_models": len(models), - "needs_api_base": slug in _NEEDS_API_BASE, + "needs_api_base": _needs_api_base(slug), "warning": warning, } @@ -177,7 +185,7 @@ async def model_save_key(params: dict) -> dict: f"{label} uses OAuth; run `raven provider login {parsed.slug.replace('_', '-')}`", data={"slug": parsed.slug}, ) - if parsed.slug in _NEEDS_API_BASE and not parsed.api_base: + if _needs_api_base(parsed.slug) and not parsed.api_base: raise ConfigValidationError( f"{label} requires an api_base", data={"slug": parsed.slug, "field": "api_base"}, diff --git a/tests/test_cli_onboard_commands.py b/tests/test_cli_onboard_commands.py index 2f4f35715..164b20d33 100644 --- a/tests/test_cli_onboard_commands.py +++ b/tests/test_cli_onboard_commands.py @@ -2836,3 +2836,64 @@ def _base_url(default: str = "https://", **kw: Any) -> Any: assert section.get("apiBase") == "http://new-box:9000/v1", section assert section.get("apiKey") == "sk-new", section assert seeded["default"] == "http://old-box:8000/v1", "the stored address was not offered back" + + +def test_a_provider_whose_endpoint_only_the_user_knows_is_asked_for_it() -> None: + """Azure gives every tenant its own resource URL, so there is nothing to default to. + + It was classified by name -- only "custom" counted -- so Azure was asked for a + key alone and stored with no endpoint, and its own client raises + "api_base is required" on the first call. Picking it from the curated list + could not produce a working provider. + """ + from raven.providers.registry import PROVIDERS, find_by_name + + assert ( + onboard_commands._credential_kind("azure_openai", find_by_name("azure_openai")) + == onboard_commands.CRED_ENDPOINT + ) + for spec in PROVIDERS: + expected = onboard_commands.CRED_ENDPOINT if spec.requires_api_base else None + if expected is not None: + assert onboard_commands._credential_kind(spec.name, spec) == expected, spec.name + + +def test_the_wizard_and_the_model_picker_agree_on_who_needs_an_endpoint() -> None: + """The same question, asked in two places, and one of them was wrong. + + The picker kept a literal set of the two providers; the wizard derived it from + the name. They drifted, and the wizard's answer was the one users hit. + """ + from raven.providers.registry import PROVIDERS + from raven.tui_rpc.methods.model import _needs_api_base + + for spec in PROVIDERS: + wizard = onboard_commands._credential_kind(spec.name, spec) == onboard_commands.CRED_ENDPOINT + assert wizard == _needs_api_base(spec.name), f"{spec.name}: wizard={wizard} picker={_needs_api_base(spec.name)}" + + +def test_configuring_azure_stores_the_endpoint_it_was_given(tmp_env: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """The consequence, read back off disk rather than from the prompt count.""" + monkeypatch.setattr( + onboard_commands, + "_collect_fields", + lambda prompts: ["sk-azure", "https://my-resource.openai.azure.com", "gpt-4o-deployment"], + ) + + returned = onboard_commands._collect_credentials( + "azure_openai", + is_oauth=False, + is_custom=True, + is_local=False, + api_key=None, + base_url=None, + model=None, + non_interactive=False, + ) + + section = json.loads(tmp_env.read_text())["providers"]["azure_openai"] + assert section["apiBase"] == "https://my-resource.openai.azure.com", section + assert section["apiKey"] == "sk-azure" + # Azure takes a deployment name where every other provider takes a model id, + # so the step locks it in rather than offering the picker. + assert returned == "gpt-4o-deployment" diff --git a/tests/test_token_wise_pricing.py b/tests/test_token_wise_pricing.py index b71b68655..bf704a435 100644 --- a/tests/test_token_wise_pricing.py +++ b/tests/test_token_wise_pricing.py @@ -241,6 +241,19 @@ def _litellm_miss(_model): raise Exception("This model isn't mapped yet") +def _patch_litellm_blind(monkeypatch): + """Make LiteLLM miss for real: the price table is consulted before the ask. + + Stubbing ``get_model_info`` alone stopped being enough once the table is read + first -- a model the table keys exactly is answered there and never reaches + the OpenRouter tier, which is the point of reading it first. + """ + import litellm + + monkeypatch.setattr(litellm, "model_cost", {}) + _patch_litellm_info(monkeypatch, _litellm_miss) + + def test_resolve_context_window_from_litellm_no_network(monkeypatch): """Tier 1: a LiteLLM-mapped model's window comes from LiteLLM, no OpenRouter hit.""" _patch_litellm_info(monkeypatch, lambda m: {"max_input_tokens": 200000}) @@ -261,7 +274,7 @@ def test_resolve_context_window_from_openrouter_when_litellm_misses(monkeypatch) def test_resolve_context_window_non_openrouter_via_catalog(monkeypatch): """Tier 2: a bare provider model LiteLLM misses resolves via the OpenRouter catalog.""" - _patch_litellm_info(monkeypatch, _litellm_miss) + _patch_litellm_blind(monkeypatch) _patch_openrouter(monkeypatch, lambda req: _models_response(_DEEPSEEK_MODELS)) assert resolve_context_window("deepseek/deepseek-v4-pro") == 163840 @@ -275,6 +288,162 @@ def test_resolve_context_window_unknown_returns_none(monkeypatch): assert resolve_context_window("openrouter/some/model-not-listed") is None +_COPILOT_MODELS = [ + { + "id": "github_copilot/gpt-4.1", + "pricing": {"prompt": "0.000002", "completion": "0.000008"}, + "context_length": 128000, + } +] + + +def _forbid(recorder: list): + """A stub that records the call and misses. + + Recorded rather than raised: both lookups swallow exceptions to move to the + next candidate, so a probe that raises is caught and proves nothing. That is + how the first version of these tests passed against the unfixed code. + """ + + def _stub(*args, **kwargs): + recorder.append(kwargs.get("model") or (args[0] if args else "?")) + raise Exception("unmapped") + + return _stub + + +def test_the_window_of_a_login_prompting_model_comes_from_the_table(monkeypatch): + """Asking LiteLLM about a Copilot model starts a GitHub device flow. + + It resolves the model's credentials on the way to its metadata, so with no + token file on disk one lookup printed six device codes to stdout and blocked + for 410 seconds -- two three-attempt login cycles, because the bare and the + openrouter-prefixed candidate reach the same driver. session.create runs this + before the first turn, so the symptom was a gateway that hung on opening. + """ + import litellm + + asked: list[str] = [] + monkeypatch.setattr(litellm, "get_model_info", _forbid(asked)) + counter = _patch_openrouter(monkeypatch, lambda req: _models_response(_DEEPSEEK_MODELS)) + + assert resolve_context_window("github_copilot/gpt-4.1") == 128000 + assert not asked, f"a login-prompting model was handed to LiteLLM: {asked}" + assert counter["calls"] == 0 + + +def test_the_rates_of_a_login_prompting_model_skip_litellm(monkeypatch): + """The same hang, on the path that runs after every single call. + + ``cost_per_token`` resolves credentials too, so the cost estimate blocked on + the same device flow. Fixing only the window lookup left this one, and + answering "can this be handed to LiteLLM" separately in each place is what + made the first attempt at this wrong. + + The estimate still lands: the live catalogue prices the model. Asking LiteLLM + is what is skipped, not estimating. + """ + import litellm + + asked: list[str] = [] + monkeypatch.setattr(litellm, "cost_per_token", _forbid(asked)) + _patch_openrouter(monkeypatch, lambda req: _models_response(_COPILOT_MODELS)) + + cost = estimate_cost_usd("github_copilot/gpt-4.1", 1000, 100) + assert cost is not None and cost > 0 + assert not asked, f"a login-prompting model was handed to LiteLLM: {asked}" + + +def test_a_login_prompting_model_the_table_does_not_price_is_never_asked(monkeypatch): + """No row and no safe way to ask: degrade, do not prompt. + + Both lookups fall through to what they already do for an unknown model -- the + caller keeps its configured window, and the cost estimate is None. A read that + runs every turn is not worth a login prompt. + """ + import litellm + + asked: list[str] = [] + monkeypatch.setattr(litellm, "get_model_info", _forbid(asked)) + monkeypatch.setattr(litellm, "cost_per_token", _forbid(asked)) + _patch_openrouter(monkeypatch, lambda req: _models_response(_DEEPSEEK_MODELS)) + + assert resolve_context_window("github_copilot/not-a-real-model") is None + assert estimate_cost_usd("github_copilot/not-a-real-model", 1000, 100) is None + assert not asked, f"asked anyway: {asked}" + + +def test_reading_the_table_does_not_replace_asking_litellm(monkeypatch): + """The ask does more than key normalization, so it stays for everything else. + + "anthropic/claude-sonnet-4-5" is absent from the table -- it keys that model + bare -- and prefixed ids are the form Raven stores. Stripping the prefix to + read the table instead looked free and was not: for an openrouter-prefixed + candidate LiteLLM derives OpenRouter's own numbers, which are in no row, and + stripping answered three MiniMax models with the direct figure instead. + """ + import litellm + + assert "anthropic/claude-sonnet-4-5" not in getattr(litellm, "model_cost", {}), ( + "premise changed; this test proves nothing" + ) + _patch_litellm_info(monkeypatch, lambda m: {"max_input_tokens": 200000}) + counter = _patch_openrouter(monkeypatch, lambda req: _models_response(_DEEPSEEK_MODELS)) + + assert resolve_context_window("anthropic/claude-sonnet-4-5") == 200000 + assert counter["calls"] == 0 + + +def test_prose_in_a_numeric_field_is_not_read_as_a_number(): + """LiteLLM ships a self-documenting row whose numeric fields hold sentences.""" + from raven.token_wise.pricing import _numeric + + prose = {"max_input_tokens": "max input tokens, if the provider specifies it"} + assert _numeric(prose, "max_input_tokens") is None + assert _numeric({"max_input_tokens": 128000}, "max_input_tokens") == 128000 + assert _numeric({"max_tokens": 8192}, "max_input_tokens", "max_tokens") == 8192 + assert _numeric(None, "max_tokens") is None + + +def test_which_drivers_can_prompt_is_read_from_the_installed_litellm(): + """Derived, not snapshotted, so a LiteLLM bump cannot make it stale. + + A frozen list would need regenerating on every bump, and a stale one brings + the hang back for the vendor it missed. The driver ships ``authenticator.py`` + or it does not. + """ + from raven.token_wise.pricing import _may_prompt + + assert _may_prompt("github_copilot/gpt-4.1") + assert _may_prompt("openrouter/github_copilot/gpt-4.1"), "any segment counts" + assert _may_prompt("chatgpt/gpt-5.1") + assert _may_prompt("gigachat/GigaChat-2-Max") + for safe in ("openai/gpt-4o", "anthropic/claude-sonnet-4-5", "deepseek/deepseek-v4-pro", "gpt-4o"): + assert not _may_prompt(safe), safe + + +def test_one_place_decides_whether_a_model_can_be_handed_to_litellm(): + """Both lookups reach the same authenticator, so both consult one answer. + + The first attempt at this guarded the metadata lookup only, and would have + needed the same decision again for the pricing call -- and again for + ``validate_environment``, which turned out to prompt as well. + """ + import ast + import pathlib + + source = (pathlib.Path(__file__).resolve().parents[1] / "raven" / "token_wise" / "pricing.py").read_text() + tree = ast.parse(source) + owner = next(node for node in ast.walk(tree) if isinstance(node, ast.FunctionDef) and node.name == "_may_prompt") + allowed = range(owner.lineno, (owner.end_lineno or owner.lineno) + 1) + offenders = [ + f"line {i}: {line.strip()}" + for i, line in enumerate(source.splitlines(), 1) + if "authenticator.py" in line and i not in allowed + ] + assert not offenders, "ask _may_prompt instead:\n" + "\n".join(offenders) + + # --- Disk persistence of the OpenRouter catalog --- _DEEPSEEK_PRICE = (0.0000005, 0.0000015)