From dff164b98031e257f228b6f84a2459449f7b0250 Mon Sep 17 00:00:00 2001 From: Jeroen Ubbink Date: Wed, 26 Aug 2026 15:44:29 +0200 Subject: [PATCH] fix(pricing): a rate spelled as a boolean is not a rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isinstance(True, int)` is True and `float(True)` is `1.0`, so a JSON `true` in a catalog was a finite, positive rate that passed every numeric guard: a dollar per token. It reached a stored price through the OpenRouter feed filter, the LiteLLM cost-map rung and the OpenRouter resolver rung. The write edge and the exchange feed already rejected booleans explicitly, which is the shape of the real problem: four readers were each parsing a rate for themselves and disagreeing about what a rate is. Give them one coercion — `coerce_rate` — and let it answer for all of them. A numeric string stays a rate, because feeds report prices as strings; that now holds on the LiteLLM rung too, which previously required a float outright. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014X8RZzzbAuQCbavhFjTvJ4 --- routstr/core/admin.py | 20 +++---- routstr/payment/cost_calculation.py | 18 ++---- routstr/payment/models.py | 21 +++---- routstr/payment/price.py | 38 ++----------- routstr/payment/rates.py | 23 ++++++++ routstr/upstream/pricing_resolver.py | 35 ++++-------- .../test_admin_pricing_rate_validation.py | 22 +++++++ tests/unit/test_pricing_rate_validation.py | 20 +++++++ tests/unit/test_upstream_generic.py | 57 +++++++++++++++++++ 9 files changed, 160 insertions(+), 94 deletions(-) diff --git a/routstr/core/admin.py b/routstr/core/admin.py index 1af941d0..f02ec441 100644 --- a/routstr/core/admin.py +++ b/routstr/core/admin.py @@ -15,7 +15,7 @@ from ..payment.models import ( _row_to_model, list_models, ) -from ..payment.rates import BILLABLE_PRICING_FIELDS, is_usable_rate +from ..payment.rates import BILLABLE_PRICING_FIELDS, coerce_rate from ..proxy import refresh_model_maps, reinitialize_upstreams from ..wallet import fetch_all_balances, send_token, token_mint_url from . import vault @@ -542,17 +542,13 @@ class ModelCreate(BaseModel): raw = value.get(field) if raw is None: continue - if isinstance(raw, bool) or not isinstance(raw, (int, float, str)): - raise ValueError(f"{field} must be a non-negative number") - try: - rate = float(raw) - except (ValueError, OverflowError): - # An integer too large for a float raises OverflowError, which - # pydantic does not convert into a validation error — unhandled - # it escapes as a 500 for what is still a bad client value. - raise ValueError(f"{field} must be a number, got {raw!r}") - if not is_usable_rate(rate): - raise ValueError(f"{field} must be a finite, non-negative number") + # The shared coercion also absorbs the OverflowError an oversized + # integer raises, which pydantic does not convert into a validation + # error — unhandled it escaped as a 500 for a bad client value. + if coerce_rate(raw) is None: + raise ValueError( + f"{field} must be a finite, non-negative number, got {raw!r}" + ) return value diff --git a/routstr/payment/cost_calculation.py b/routstr/payment/cost_calculation.py index ea45abe6..e7cfdb87 100644 --- a/routstr/payment/cost_calculation.py +++ b/routstr/payment/cost_calculation.py @@ -6,7 +6,7 @@ from pydantic.v1 import BaseModel from ..core import get_logger from ..core.settings import settings from .price import sats_usd_price -from .rates import is_usable_rate +from .rates import coerce_rate, is_usable_rate from .usage import normalize_usage, parse_token_count if TYPE_CHECKING: @@ -336,17 +336,11 @@ def _coerce_usd(value: object) -> float: ``0.0`` means "no usable figure" to every caller, which is the same thing an absent field means, so the caller's existing ``> 0`` checks handle it. """ - if value is None or isinstance(value, bool): - return 0.0 - if not isinstance(value, (int, float, str)): - return 0.0 - try: - # An oversized integer raises OverflowError, not ValueError. - amount = float(value) - except (TypeError, ValueError, OverflowError): - return 0.0 - # A negative is rejected here, where the previous `max(0.0, …)` clamped it. - return amount if is_usable_rate(amount) else 0.0 + # A cost figure is coerced exactly like a rate; only the way an unusable one + # is reported differs. A negative is rejected here, where the previous + # `max(0.0, …)` clamped it. + amount = coerce_rate(value) + return amount if amount is not None else 0.0 def _resolve_usd_cost(usage_data: dict, response_data: dict) -> float: diff --git a/routstr/payment/models.py b/routstr/payment/models.py index 62773371..49ca89b1 100644 --- a/routstr/payment/models.py +++ b/routstr/payment/models.py @@ -12,7 +12,7 @@ from ..core.db import ModelRow, UpstreamProviderRow, get_session from ..core.logging import get_logger from ..core.settings import settings from .price import sats_usd_price -from .rates import BILLABLE_PRICING_FIELDS, is_usable_rate +from .rates import BILLABLE_PRICING_FIELDS, coerce_rate, is_usable_rate logger = get_logger(__name__) @@ -163,19 +163,12 @@ def _has_valid_pricing(model: dict) -> bool: if not pricing: return False - try: - prompt = float(pricing.get("prompt", 0)) - completion = float(pricing.get("completion", 0)) - except (ValueError, TypeError, OverflowError): - # An integer too large for a float raises OverflowError, not - # ValueError, so it escaped this coercion guard and unwound the whole - # fetch — one junk entry cost the node the entire upstream catalog. - return False - - # `NaN`/`±inf` are not prices, and neither is caught by the checks below: - # every comparison with `NaN` is False, and `inf` reads as a large positive - # rate that would be advertised and billed on. - if not is_usable_rate(prompt) or not is_usable_rate(completion): + # Coercion runs before the both-zero test below, which `NaN` would defeat + # on its own — and one entry the coercion chokes on must not unwind the + # whole fetch, which once cost the node an entire upstream catalog. + prompt = coerce_rate(pricing.get("prompt", 0)) + completion = coerce_rate(pricing.get("completion", 0)) + if prompt is None or completion is None: return False if prompt == 0 and completion == 0: diff --git a/routstr/payment/price.py b/routstr/payment/price.py index 48fc5b65..ae162398 100644 --- a/routstr/payment/price.py +++ b/routstr/payment/price.py @@ -5,7 +5,7 @@ import httpx from ..core import get_logger from ..core.settings import settings -from .rates import is_usable_rate +from .rates import coerce_rate logger = get_logger(__name__) @@ -19,40 +19,14 @@ def _parse_quote(raw: object, exchange: str) -> float | None: Every quote passes through here because the aggregator takes the ``min()`` of what it collects: an unusable quote does not merely join the sample, it *wins* it, and the result is the rate every model and every request on the - node is priced at. A zero divides by zero on the USD cost path, ``NaN`` - raises out of the integer conversion in settlement, and a negative rate - produces a negative charge that is credited back to the caller. - - A quote is stricter than a billable rate: it must be positive, since a BTC - price of zero is a broken feed, not free money. + node is priced at. A quote is stricter than a billable rate — it must be + positive, since a BTC price of zero is a broken feed, not free money. """ - # `float(True)` is a finite, positive 1.0 that passes every guard below and - # would then win the `min()`, pricing the node at one dollar per bitcoin. - if isinstance(raw, bool): - logger.warning( - "Non-numeric price quote — ignoring this exchange", - extra={"exchange": exchange, "quote": repr(raw)}, - ) - return None - - try: - price = float(raw) # type: ignore[arg-type] - except (TypeError, ValueError, OverflowError) as e: - logger.warning( - "Unparseable price quote — ignoring this exchange", - extra={ - "error": str(e), - "error_type": type(e).__name__, - "exchange": exchange, - "quote": repr(raw), - }, - ) - return None - - if not is_usable_rate(price) or price <= 0: + price = coerce_rate(raw) + if price is None or price <= 0: logger.warning( "Unusable price quote — ignoring this exchange", - extra={"exchange": exchange, "quote": price}, + extra={"exchange": exchange, "quote": repr(raw)}, ) return None diff --git a/routstr/payment/rates.py b/routstr/payment/rates.py index 4ac74587..998f3ca5 100644 --- a/routstr/payment/rates.py +++ b/routstr/payment/rates.py @@ -46,3 +46,26 @@ def is_usable_rate(rate: float) -> bool: they do with the answer, not why the answer matters. """ return math.isfinite(rate) and rate >= 0.0 + + +def coerce_rate(value: object) -> float | None: + """Coerce a value from outside the node to a usable rate, or ``None``. + + The one coercion, shared by every reader of a rate the node did not compute + itself: an upstream catalog, the LiteLLM cost map, the exchange feed and the + admin write edge. Each of them was parsing for itself, and they disagreed — + which is how a boolean became a price on some paths and not others. + + A boolean is rejected outright: it is a change of shape, not a rate, and + Python would make ``True`` a finite, positive ``1.0`` that passes every + numeric guard downstream — a dollar per token. A numeric string is accepted, + because feeds report prices as strings. An oversized integer raises + ``OverflowError`` rather than ``ValueError``, so that is caught too. + """ + if isinstance(value, bool) or not isinstance(value, (int, float, str)): + return None + try: + rate = float(value) + except (TypeError, ValueError, OverflowError): + return None + return rate if is_usable_rate(rate) else None diff --git a/routstr/upstream/pricing_resolver.py b/routstr/upstream/pricing_resolver.py index b12a7f1e..8b58dddb 100644 --- a/routstr/upstream/pricing_resolver.py +++ b/routstr/upstream/pricing_resolver.py @@ -17,7 +17,7 @@ from __future__ import annotations from dataclasses import dataclass, field -from ..payment.rates import is_usable_rate +from ..payment.rates import coerce_rate @dataclass @@ -67,18 +67,8 @@ def estimate_context_length(model_id: str) -> int: def _as_float(value: object) -> float | None: - """OpenRouter reports prices as strings; coerce, ``None`` if not a real rate. - - Every caller reads a *price* out of a feed, so this asks the shared - billable-rate question rather than merely parsing: ``float("Infinity")``, - ``float("NaN")`` and a negative all parse cleanly from a feed string. An - oversized integer raises ``OverflowError`` rather than ``ValueError``. - """ - try: - parsed = float(value) # type: ignore[arg-type] - except (TypeError, ValueError, OverflowError): - return None - return parsed if is_usable_rate(parsed) else None + """OpenRouter reports prices as strings; coerce, ``None`` if not a real rate.""" + return coerce_rate(value) def _as_int(value: object) -> int | None: @@ -95,18 +85,15 @@ def _from_litellm(model_id: str) -> ResolvedPricing | None: if info is None: return None - prompt = info.get("input_cost_per_token") - completion = info.get("output_cost_per_token") - if not isinstance(prompt, (int, float)) or not isinstance(completion, (int, float)): + prompt = coerce_rate(info.get("input_cost_per_token")) + completion = coerce_rate(info.get("output_cost_per_token")) + if prompt is None or completion is None: return None # A both-zero entry is litellm listing a model without a real price (free # moderation/rerank tiers do this) — treating 0/0 as resolved would serve - # the model for free. Reject it (and any negative) so the caller falls - # through, mirroring async_fetch_openrouter_models' _has_valid_pricing. - # Checked before the both-zero guard below: `NaN` defeats that guard on its - # own, since every comparison against it is False. - if not is_usable_rate(prompt) or not is_usable_rate(completion): - return None + # the model for free. Reject it so the caller falls through, mirroring + # async_fetch_openrouter_models' _has_valid_pricing. Coercion runs first: + # `NaN` would defeat this guard on its own, every comparison being False. if prompt == 0 and completion == 0: return None @@ -115,8 +102,8 @@ def _from_litellm(model_id: str) -> ResolvedPricing | None: input_modalities.append("image") return ResolvedPricing( - prompt=float(prompt), - completion=float(completion), + prompt=prompt, + completion=completion, # max_input_tokens is the context window; max_tokens is litellm's # completion cap (it tracks max_output_tokens for ~94% of models), so # it is never a context source. A missing window falls to the id-based diff --git a/tests/integration/test_admin_pricing_rate_validation.py b/tests/integration/test_admin_pricing_rate_validation.py index 5b53b02a..5a3d06c4 100644 --- a/tests/integration/test_admin_pricing_rate_validation.py +++ b/tests/integration/test_admin_pricing_rate_validation.py @@ -148,6 +148,28 @@ async def test_malformed_price_string_is_rejected( assert await integration_session.get(ModelRow, ("bad-price", provider_id)) is None +@pytest.mark.integration +@pytest.mark.asyncio +async def test_boolean_price_is_rejected( + integration_client: AsyncClient, integration_session: AsyncSession +) -> None: + """A JSON ``true`` coerces to a finite, positive ``1.0`` — a dollar per + token — so it passes every numeric guard. The write edge asks the same + coercion the catalog readers do, and answers a 422.""" + provider_id = await _make_provider(integration_session) + + resp = await integration_client.post( + f"/admin/api/upstream-providers/{provider_id}/models", + headers=_admin_headers(), + json=_payload( + provider_id, model_id="bool-price", pricing=_pricing(prompt=True) + ), + ) + + assert resp.status_code == 422 + assert await integration_session.get(ModelRow, ("bool-price", provider_id)) is None + + @pytest.mark.integration @pytest.mark.asyncio async def test_numeric_string_price_is_still_accepted( diff --git a/tests/unit/test_pricing_rate_validation.py b/tests/unit/test_pricing_rate_validation.py index d3656d25..fab4e6d5 100644 --- a/tests/unit/test_pricing_rate_validation.py +++ b/tests/unit/test_pricing_rate_validation.py @@ -442,3 +442,23 @@ async def test_oversized_catalog_rate_does_not_empty_the_catalog() -> None: models = await async_fetch_openrouter_models() assert [m["id"] for m in models] == ["good"] + + +@pytest.mark.asyncio +async def test_boolean_catalog_rate_is_not_imported() -> None: + """A JSON ``true`` is a change of shape, not a price. + + Python coerces it to a finite, positive ``1.0`` — a dollar per token — so it + passes every numeric guard and must be rejected before coercion. + """ + with _patch_openrouter_catalog( + [ + _catalog_entry("bad", {"prompt": True, "completion": "0.000002"}), + _catalog_entry("good", {"prompt": "0.000001", "completion": "0.000002"}), + ] + ): + from routstr.payment.models import async_fetch_openrouter_models + + models = await async_fetch_openrouter_models() + + assert [m["id"] for m in models] == ["good"] diff --git a/tests/unit/test_upstream_generic.py b/tests/unit/test_upstream_generic.py index 04470a76..2e6fd56e 100644 --- a/tests/unit/test_upstream_generic.py +++ b/tests/unit/test_upstream_generic.py @@ -667,3 +667,60 @@ async def test_negative_openrouter_cache_rate_is_dropped_not_carried() -> None: assert model.enabled is True assert model.pricing.prompt == pytest.approx(1e-06) assert model.pricing.input_cache_read == 0.0 + + +@pytest.mark.asyncio +async def test_boolean_litellm_rate_is_not_a_resolved_price() -> None: + """``isinstance(True, int)`` is True, so a boolean passed the cost map's own + numeric check and resolved as a rate of ``1.0`` — a dollar per token.""" + payload = { + "data": [ + {"id": "bool-priced-model", "object": "model", "owned_by": "mystery"}, + ] + } + cost_entry = { + "input_cost_per_token": True, + "output_cost_per_token": 2e-06, + "max_input_tokens": 8192, + } + + with _patch_models_endpoint(payload): + or_feed = AsyncMock(return_value=[]) + with patch("routstr.payment.models.litellm_cost_entry", lambda _id: cost_entry): + with patch("routstr.payment.models.async_fetch_openrouter_models", or_feed): + models = await GenericUpstreamProvider( + base_url="http://x" + ).fetch_models() + + model = _model_by_id(models, "bool-priced-model") + assert model.enabled is False + assert model.pricing.prompt == 0.0 + assert model.pricing.completion == 0.0 + + +@pytest.mark.asyncio +async def test_boolean_openrouter_rate_is_not_a_resolved_price() -> None: + """The same coercion reads a feed's ``true`` as a rate of ``1.0``; the model + must import disabled rather than priced at a dollar per token.""" + payload = { + "data": [ + {"id": "or-bool-xyz", "object": "model", "owned_by": "mystery"}, + ] + } + feed = [ + { + "id": "or-bool-xyz", + "pricing": {"prompt": True, "completion": "0.000002"}, + "context_length": 8192, + } + ] + + with _patch_models_endpoint(payload): + or_feed = AsyncMock(return_value=feed) + with patch("routstr.payment.models.async_fetch_openrouter_models", or_feed): + models = await GenericUpstreamProvider(base_url="http://x").fetch_models() + + model = _model_by_id(models, "or-bool-xyz") + assert model.enabled is False + assert model.pricing.prompt == 0.0 + assert model.pricing.completion == 0.0