Recover both cost figures on routed calls (#3691) - #3695
Conversation
Every routed agent call logged `cost: null` and `cost_estimated: null`, so
egg had no dollar figure at all for its LLM spend: 1252 of 1252 sampled
`cost_callback` lines on run 6. Two independent, egg-side causes, one patch
each. Both verified against the pinned litellm 1.86.2 the image builds on.
Patch 10 — the bill survives streaming. OpenRouter reports what it charged
on the final streamed usage chunk, and stock LiteLLM already asks for it
(`OpenrouterConfig.transform_request` sets `usage: {"include": true}`
unconditionally, so the issue's proposed `extra_body` change was not
needed). The number reaches litellm intact and is then dropped by
`ChunkProcessor.calculate_usage`, which rebuilds `Usage` field-by-field over
the token counts it enumerates and re-constructs it from its own
`model_dump()`. Claude Code streams every /v1/messages request, so that seam
was ~100% of routed traffic; the non-streaming path was never affected,
which is why this read as a property of the route rather than as a transport
bug. The new `stream_cost_preservation` module carries `cost` /
`cost_details` across the rebuild and interprets neither — a zero `cost` is
the literal BYOK truth with its fall-through partner alongside it, and
`cost_callback._extract_cost` reads the result unchanged.
Patch 11 — the estimate has a rate card. `cost_estimated` is LiteLLM's own
`response_cost`, null because `model_prices_and_context_window.json` carries
none of the slugs egg routes — the same lag that broke the reasoning-param
gate in patch 7. `openrouter_capabilities` grows a second entry point off
the roster it already fetches, hooked into `_get_model_info_helper` at the
"isn't mapped yet" raise so it runs only after every bundled lookup has
failed: a mapped slug keeps its bundled rate, and the live card can add a
model but never reprice one. Cost fields only — a `supports_*` flag through
that door would change parameter admission, which is patch 7's job.
A model whose rate card is tiered by prompt length is declined rather than
priced. OpenRouter publishes a long-context surcharge as `pricing.overrides`
keyed by an arbitrary `min_prompt_tokens` (qwen3-max: 32000 and 128000);
LiteLLM has slots for three fixed thresholds and drops the rest, so
registering the base tier would under-report by 2-2.5x on precisely the
long-prompt turns agent traffic is made of, silently, under a field an
operator would use to choose a model. Those stay null and log the reason
once; their provider-billed `cost` is exact and is the number to read.
The issue's remaining options are moot given the above: reading
/api/v1/generation post-hoc (fix 3) would cost an extra request per call for
the figure patch 10 now recovers for free, and the non-streaming calibration
sample (fix 4) has nothing left to calibrate.
Also pins config/litellm/ to `target-version = "py311"` for ruff. Those five
files are the only Python here that runs on another interpreter (the litellm
base image ships 3.11), and `ruff format` under the repo's py314 rewrites
`except (A, B):` into the PEP 758 unparenthesized form — a hard SyntaxError
there, with no `noqa` escape from a formatter rewrite. It bit this change;
a test now parses all five at `feature_version=(3, 11)` so the outcome is
asserted independently of the pin.
There was a problem hiding this comment.
Review: PR #3695 — Recover both cost figures on routed calls
Reviewed all 12 changed files against the pinned upstream sources (litellm==1.86.2
streaming_chunk_builder_utils.py, types/utils.py, utils.py, fetched and read
directly) and against a live pull of OpenRouter's GET /api/v1/models roster
(367 models). No blocking defect found. Both patches are correctly placed,
correctly needled, and correctly fail-safe. Nine non-blocking findings below,
ordered by how much they cost you in practice.
What I verified as correct (so you know what was actually checked)
- Patch 10's needle matches 1.86.2 verbatim.
ChunkProcessor.calculate_usage
(line 619 ofstreaming_chunk_builder_utils.py) does end with
returned_usage = Usage(**returned_usage.model_dump())/return returned_usage,
andchunksis the method's first parameter — in scope at the insertion point. - Patch 10's placement AFTER the rebuild is not stylistic, it is required.
Usage.__init__(types/utils.py:1545) ends withif cost is not None: self.cost = cost
/else: del self.cost. A pre-rebuild write would be deleted by the rebuild. The
testtest_patch10_sets_cost_after_the_rebuild_not_beforeis guarding a real hazard. setattr(usage, "cost_details", ...)works on the realUsage. I checked this
because a plain-Python test double can't prove it:Usage(SafeAttributeModel, CompletionUsage)
declares nomodel_config;SafeAttributeModelis a bare mixin that only overrides
__delattr__;CompletionUsageinherits openai'sBaseModel, which sets
ConfigDict(extra="allow")(openai/_models.py:128). Verified against pydantic 2.13.4
thatextra="allow"permitssetattrof an undeclared name and thatmodel_dump()
round-trips it — which matters becausecost_callback._coerce_usagereads via
model_dump(). Under the defaultextra, the samesetattrraises
ValueError: "X" object has no field .... See finding 6.- Patch 11's needle is genuinely disambiguated.
utils.pycarries "isn't mapped yet"
at 5332 (get_max_tokens, different f-string), 5791 (the target), and 6000 (outer
handler,.format(...)). Only 5791 is preceded byif _model_info is None or key is None:. - Patch 11 does not change any
supports_*answer. I traced this because turning a
raise into a partialModelInfoBaselooks like it should. It doesn't:
_supports_factory(2590) and_is_explicitly_disabled_factory(2646) both converge
on_supports_provider_info_factory(...)→return Falsewhether the helper raised or
returned an entry whosemodel_info.get(key)isNone.get_max_tokens(5274) reads
litellm.model_costdirectly and never touches the helper. - A minimal entry cannot crash the constructor. Every field in the
ModelInfoBase(...)
return atutils.py:5817-5900+is read with_model_info.get(k, None).input_cost_per_token
andoutput_cost_per_tokendefault to0with averbose_logger.debug.ModelInfoBase
is aTypedDict— no runtime validation. - The tiered-decline reasoning is factually right about the mechanism. LiteLLM has slots
for exactly three thresholds (*_above_128k_tokens,*_above_200k_tokens,
*_above_272k_tokens). The doc'sqwen3-max: 32000 and 128000example is accurate —
I confirmed it against the live roster. .ruff.tomlis load-bearing, not decoration.make lintruns$(RUFF) format --check .
(Makefile:263) and.pre-commit-config.yamlrunsruff-format. Without this file,
make lint-fixwould emit PEP 758except TypeError, ValueError:into files that run on
the image's Python 3.11. The rootper-file-ignorespatterns (sandbox/egg,tests/*,
integration_tests/*) can't match anything underconfig/litellm/, soextend's
relative-path resolution is a non-issue here._install_module's provenance guard, parse check, and idempotency all hold for the new
4th entry;litellm_core_utils/exists in the stock tree, so thedest_dircheck passes.
Findings
1. The tiered decline is ~4× broader than its own justification. (non-blocking, highest practical cost)
_cost_entry (openrouter_capabilities.py:331-338) declines any entry with a non-empty
pricing.overrides. The stated reason is that LiteLLM cannot express an arbitrary boundary.
That is true for some models and false for most of them.
Live roster, right now:
declined tiered: 49
... of which expressible with LiteLLM's 3 slots: 36
The 36 have a single boundary landing exactly on 128000, 200000, or 272000 — a slot LiteLLM
already has. They include openai/gpt-5.5, openai/gpt-5.5:batch, openai/gpt-5.4,
openai/gpt-5.4-pro, x-ai/grok-4.5, x-ai/grok-4.3, x-ai/grok-4.20,
qwen/qwen3.6-max-preview, ~openai/gpt-latest, ~google/gemini-pro-latest. Only 13 are
genuinely inexpressible (qwen/qwen3-max at 32000+128000, qwen/qwen3.7-flash at 32000+256000, etc.).
Consequence: cost_estimated stays null for ~10% of the roster including several of the most
likely routing targets, and the operator gets a warn line saying LiteLLM "cannot express those
boundaries" when for 36 of them it can.
This is conservative-safe (null, not wrong), which is why it isn't blocking. But it's a large
amount of the feature's value left on the table for a reason that doesn't apply. Suggested fix,
entirely inside _cost_entry:
_EXPRESSIBLE_TIERS = {128000: "128k", 200000: "200k", 272000: "272k"}and when every min_prompt_tokens in overrides is in that set, emit
input_cost_per_token_above_<N>k_tokens / output_cost_per_token_above_<N>k_tokens /
cache_read_input_token_cost_above_<N>k_tokens alongside the base rates, declining only the
genuinely arbitrary boundaries. If you'd rather not take that on in this PR, please at least
reword the warning so it doesn't assert an impossibility that isn't one, and file the follow-up.
2. Four published price components are silently dropped, three with exact LiteLLM slots. (non-blocking)
_PRICE_KEYS (openrouter_capabilities.py:274-279) maps 4 of the 12 pricing components
OpenRouter actually publishes. Roster counts of models with a non-zero value:
prompt 350 completion 350 input_cache_read 212 input_cache_write 68 <- mapped
web_search 129 input_cache_write_1h 30 internal_reasoning 28 image 27 <- dropped
audio 30 input_audio_cache 25 image_output 9 audio_output 2 <- dropped
Three of the dropped ones have a one-to-one destination in the very ModelInfoBase(...) call
patch 11 feeds (utils.py:5817+):
| OpenRouter | LiteLLM field that exists and is left None |
|---|---|
input_cache_write_1h |
cache_creation_input_token_cost_above_1hr |
internal_reasoning |
output_cost_per_reasoning_token |
web_search |
input_cost_per_query |
input_cache_write_1h is the one that matters for egg specifically — it's on every Anthropic
route and is 2× the 5m rate (anthropic/claude-opus-4.8: input_cache_write 0.00000625
vs input_cache_write_1h 0.00001; claude-sonnet-5: 0.0000025 vs 0.000004). Claude Code
defaults to 5m ephemeral cache control, so this doesn't fire today — which is why it's
PLAUSIBLE and non-blocking rather than confirmed. But the moment anything sets ttl: "1h",
cache writes get priced at 50% of actual with no signal.
The inconsistency is what I'd fix even if you skip the mappings: this module declines an entry
outright rather than under-report a tiered card, then emits an entry that under-reports for
these components without a word. Pick one discipline. Adding the three mappings above is three
lines in _PRICE_KEYS; at minimum, say in the docstring which components the estimate omits.
3. _WARNED_DECLINED_PRICING's comment states the opposite of its behaviour. (non-blocking)
openrouter_capabilities.py:124-127:
# Slugs whose tiered rate card has already been reported as declined. Bounded by
# the roster size, and reset with the cache so a pricing change upstream is
# reported again rather than muted for the pod's lifetime.
_WARNED_DECLINED_PRICING: set[str] = set()It is not reset with the cache. _get_cache() (459-487) refreshes _CACHE on TTL expiry and
never touches this set. The only writer that clears it is reset_cache() (637-645), whose own
docstring says "Intended for tests." So the warning is muted for the pod's lifetime — the
exact outcome the comment claims to prevent.
What makes this worth fixing rather than just reading past: the neighbouring latch 5 lines up
gets this right. _WARNED_FETCH_FAILURE's comment says "Cleared again by a successful fetch"
and line 435 actually does _WARNED_FETCH_FAILURE = False inside _fetch. So there's a working
pattern immediately adjacent. One line beside it:
_WARNED_FETCH_FAILURE = False
_WARNED_DECLINED_PRICING.clear() # add(_fetch would need the extra global, or just make the set a mutable module-level and call
.clear().) Alternatively, delete the second sentence of the comment. Either is fine; the
current state — a comment asserting a reset that doesn't exist — is the one option that isn't.
4. _lookup short-circuits on the first candidate with any record, not the first that answers the asked half. (non-blocking, PLAUSIBLE)
openrouter_capabilities.py:531-535:
for candidate in _candidate_slugs(model):
record = cache.get(candidate)
if record is not None:
return record
return NoneBefore this PR, _fetch skipped roster entries with no supported_parameters, so the candidate
loop fell through them and reached the base slug. Now those entries are retained for their
pricing, so a candidate that carries pricing-only terminates the loop and
get_supported_parameters returns None — even though a later candidate would have answered.
The failure that re-opens is patch 7's: reasoning_effort silently dropped for a variant slug
whose roster entry has pricing but no supported_parameters.
I tried to confirm this and could not. On the live roster exactly 3 models lack
supported_parameters (openrouter/fusion, openrouter/pareto-code, openrouter/bodybuilder),
none contains a :, so none has a base-slug candidate to fall through to. The intersection of
"missing supported_parameters" and "suffixed variant" is currently empty. So: real mechanism,
no trigger in today's data — advisory, not blocking.
Cheap structural fix that removes the dependence on upstream data shape — make _lookup take
the field it's answering for:
def _lookup(model: str, field: str) -> dict | None:
...
best = None
for candidate in _candidate_slugs(model):
record = cache.get(candidate)
if record is None:
continue
if record.get(field) is not None:
return record
best = best or record # keep for declined_thresholds reporting
return best5. _candidate_slugs' suffix stripping attributes a base model's paid rate card to a variant. (non-blocking, PLAUSIBLE)
_candidate_slugs (490-513) adds the pre-colon base for any :-bearing candidate. That's the
right call for parameters, where the module is explicitly union-only and never subtractive
(comment at 508-509). It is a different proposition for pricing, where the fallback produces an
authoritative-looking number rather than a permissive capability.
Live roster: 37 suffixed slugs whose base is also published, and the rates differ by design —
poolside/laguna-s-2.1:free prompt 0 vs base 0.0000001; google/gemini-3.6-flash:batch
0.00000075 vs base 0.0000015; anthropic/claude-sonnet-5:batch 0.000001 vs base 0.000002.
Today the variants are all published in their own right, so the first candidate hits and the
fallback never runs — hence PLAUSIBLE, not confirmed. It fires only when the exact variant is
absent from the roster (OpenRouter retires :free variants routinely) while the base remains,
and the result is a :free route reported at the base's paid rate, or a :batch route reported
at 2× actual. There's no marker distinguishing an inherited rate from a published one.
Suggest either restricting the base-slug candidate to the parameters half (falls out naturally
from finding 4's signature change), or recording the slug the rate came from — you already have
the hook, since entry["key"] is set to record["id"] and surfaces on /model/info. A one-line
note in _cost_entry's docstring that the key may not equal the requested model would do.
6. The stream_cost_preservation tests don't exercise the property they depend on. (non-blocking, test robustness)
tests/config/test_litellm_runtime_modules.py's streamcost fixture uses a plain-Python _Usage
class. carry_upstream_cost does setattr(usage, COST_DETAILS_FIELD, cost_details) where
cost_details is an undeclared name on the real Usage — it survives only as a pydantic extra.
On a plain class that always works; on a pydantic model it works if and only if extra="allow".
I verified it does hold today (see the top section). But carry_upstream_cost's
except Exception: pass # noqa: BLE001 means that if it ever stops holding — a litellm bump that
tightens the config, a Usage subclass with its own model_config — the cost_details half
becomes a completely silent no-op, and nothing in this suite would notice, because the double
can't express the failure. That is the "fixture bypasses the production code path" shape.
Two cheap closures:
- Add one test that constructs a real pydantic model with
ConfigDict(extra="allow")and a
declaredcost(mirroringUsage's shape without importing litellm) and asserts both fields
land and survivemodel_dump(). - Add the missing seam test: nothing currently runs
carry_upstream_costoutput through
cost_callback._extract_cost.test_cost_callback.py::_reassembled_usage_with_costis a
hand-built dict asserting what patch 10 is supposed to produce; the producer and consumer are
joined only by prose._extract_cost(cc._coerce_usage(carry_upstream_cost(chunks, usage)))is
one assertion and covers the whole contract.
7. Both patch bodies swallow their import failure with no log line. (non-blocking)
Patch 10:
except Exception:
passPatch 11:
except Exception:
_egg_entry = NoneIf _egg_stream_cost or _egg_capabilities fails to import at runtime — a partially-written
layer, a sys.path surprise, an exception in the module body — the symptom is exactly the
symptom the PR exists to remove: cost: null on every line, no explanation anywhere. The whole
rest of this build is fail-loud on drift (_install_module's provenance guard, _check_parses,
the missing-needle exit), and this is the one place a failure is indistinguishable from "the
provider didn't report a cost."
The swallow itself is right — a cost must never break a response. Just make it observable:
latch a one-time verbose_logger.warning inside the handler, same warn-once discipline the
module already uses elsewhere. It costs nothing on the happy path.
8. A string tier boundary degrades to "boundaries unparseable" rather than being parsed. (non-blocking, minor)
_cost_entry:336 requires isinstance(boundary, int). Today every min_prompt_tokens in the
roster is a JSON integer, so this is correct. But pricing.prompt right next to it arrives as a
decimal string and is parsed as one — so OpenRouter demonstrably does serialize numbers as
strings in this same block. If min_prompt_tokens ever follows, the operator gets
"tier boundaries unparseable" for a card whose boundaries are perfectly parseable. Reusing a
_price-style coercion here would make the two halves consistent.
9. Type comment on the cache record is wrong for one field. (nit)
openrouter_capabilities.py:108-109 documents
"declined_thresholds": tuple[int, ...], but _cost_entry returns None for it on every
non-tiered entry and _fetch:415 stores that None. Should be tuple[int, ...] | None. The
prose two lines down already says both payloads are optional, so this is just the type line.
Also: docs/development/STRUCTURE.md gains a line for stream_cost_preservation.py but not for
.ruff.toml. STRUCTURE.md mostly omits dotfiles so this is arguably consistent — but this
particular dotfile is the only thing standing between make lint-fix and a build-breaking
SyntaxError on the image, which is more load-bearing than most files that are listed.
Summary
Patches 10 and 11 are well-targeted, correctly ordered against Usage.__init__'s
del self.cost, correctly needled against 1.86.2, and correctly inert under failure. The
.ruff.toml addition is a real fix for a real formatter hazard, with the reasoning written
down where the next person will find it. The docstrings throughout carry the why, which made
this review tractable.
The two I'd actually act on before merge are #1 (36 of the 49 declined models are
expressible — that's most of the feature's reach) and #3 (a comment that asserts a reset
which does not exist, next to a neighbour that does it correctly). #2 and #7 are the
next tier. The rest are advisory.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Translate expressible OpenRouter prompt-length surcharges instead of declining every tiered card, carry two more published rate components, fix the parameter/pricing lookup asymmetry, and make both patch bodies' swallowed import failures observable.
Review response — all 9 findings addressed in-PR (commit
|
| boundary | prompt | completion | cache_read | cache_write |
|---|---|---|---|---|
| 128000 | ✅ | ✅ | ❌ | ❌ |
| 200000 | ✅ | ✅ | ✅ | ✅ |
| 272000 | ✅ | ✅ | ✅ | ❌ |
Emitting "input/output/cache_read tiers" as suggested would have shipped the exact failure this module declines a whole card to avoid. openai/gpt-5.6-luna-pro surcharges input_cache_write at 272000; there is no cache_creation_input_token_cost_above_272k_tokens, so _get_token_base_cost's if cache_creation_tiered_key in model_info gate falls back to the base rate above the boundary — a silent under-report on precisely the long-prompt turns the surcharge exists for.
So the rule implemented is all-or-nothing per model: every published boundary must have slots, and every priced component published in each override must have a slot at that boundary, or the card is declined whole. Components we don't price at base either (image, audio, …) are skipped rather than treated as fatal — declining over one would withhold a card no less complete above the boundary than below it. A tier publishing a completion surcharge but no prompt one is also declined: LiteLLM finds the applicable boundary by scanning for input_cost_per_token_above_* keys, so such a tier contributes a key nothing reads and bills the turn at base while looking translated.
Measured against the live roster (367 models, 49 tiered): 25 now priced, 24 declined — not the 36 in your count, and the 11 difference is entirely per-component coverage. Notably anthropic/claude-sonnet-4.5 stays declined: its 200k tier surcharges input_cache_write_1h, and cache_creation_input_token_cost_above_1hr_above_200k_tokens is read dynamically by _get_token_base_cost:276 but absent from the enumeration, so it never arrives.
The warning was reworded too — it no longer asserts an impossibility, it names the boundaries and says LiteLLM has slots only at 128000/200000/272000 and not for every component at each.
Tests: test_a_tier_landing_on_a_litellm_slot_is_translated_not_declined, test_a_tier_is_declined_when_one_published_component_has_no_slot, test_a_tier_publishing_only_a_component_we_never_price_is_still_translated, test_a_tier_with_no_prompt_surcharge_is_declined_as_unreachable, test_an_inexpressible_boundary_is_still_declined.
2. Four published price components silently dropped — fixed-in-PR (commit a4d555b) for two; disagree on the third
Agreed on input_cache_write_1h → cache_creation_input_token_cost_above_1hr and internal_reasoning → output_cost_per_reasoning_token. Both are per-token and both are genuinely consumed on the chat path (calculate_cache_writing_cost at llm_cost_calc/utils.py:416, generic_cost_per_token at :762-770). Added to _PRICE_KEYS. Your point about the inconsistency is the one I acted on: declining a whole card to avoid under-reporting, then under-reporting quietly elsewhere, is not one discipline.
Disagree on web_search → input_cost_per_query. Neither input_cost_per_query nor search_context_cost_per_query appears anywhere in llm_cost_calc/utils.py or cost_calculator.py's chat path in 1.86.2 — the field exists on the model-info shape but nothing reads it when pricing a completion. And the units don't match: OpenRouter's web_search is per-request (0.01 / 0.014), not per-token. Mapping it would put a per-request rate into a slot that is either ignored or, if a future release starts reading it, read per-token. Instead the module docstring now enumerates exactly what the estimate omits and why (web_search, request, image, audio, input_audio_cache, discount), and docs/guides/per-agent-models.md says the same to operators — which was your minimum ask.
Test: test_the_1h_cache_write_and_reasoning_rates_are_carried (asserts both new keys land and input_cost_per_query does not).
3. _WARNED_DECLINED_PRICING's comment states the opposite of its behaviour — fixed-in-PR (commit a4d555b)
Agreed, and confirmed. Took the first option: _fetch now calls _WARNED_DECLINED_PRICING.clear() beside _WARNED_FETCH_FAILURE = False, so the latch is re-armed against the roster it was derived from. The set is module-level and mutated in place, so no extra global was needed. Comment updated to describe what now happens. Test: test_the_decline_latch_is_rearmed_by_a_refetch_not_only_by_reset_cache.
4. _lookup short-circuits on the first candidate with any record — fixed-in-PR (commit a4d555b)
Agreed, and took your suggested signature nearly verbatim: _lookup(model, field) skips a record that cannot answer the asked half and keeps the first one seen as a fallback so get_model_cost_entry can still report why a model it has definitely seen goes unpriced. Your read that today's roster has no trigger matches mine; the structural fix removes the dependence on that staying true. Test: test_a_pricing_only_entry_does_not_shadow_a_parameter_answer.
5. _candidate_slugs' suffix stripping attributes a base model's paid rate to a variant — fixed-in-PR (commit a4d555b)
Agreed. Took the stronger of the two options rather than the docstring note: _candidate_slugs(model, *, strip_variant=True), with get_model_cost_entry passing strip_variant=False. The openrouter/ prefix strip is kept for both halves — that names the same model, not a different rate card. Consequence worth stating: entry["key"] is now always the slug the rates were published under, so /model/info can be read as naming the real source. Test: test_a_variant_slug_never_inherits_its_base_models_rate_card (asserts pricing does not inherit and parameters still do).
6. The stream_cost_preservation tests don't exercise the property they depend on — fixed-in-PR (commit a4d555b)
Agreed. Both closures added:
test_an_undeclared_field_survives_on_a_real_pydantic_usage— realpydantic.BaseModelwithConfigDict(extra="allow"), declaredcost, undeclaredcost_details; asserts both land and survivemodel_dump(), since that is how_coerce_usagereads them.test_the_carried_cost_is_readable_by_the_callback_that_consumes_it— the seam test, on the BYOK shape (cost0, money incost_details.upstream_inference_cost), which is the case where the two modules have to agree on a nested key name to produce a number at all:cc._extract_cost(cc._coerce_usage(carry_upstream_cost(chunks, usage))) == 0.0045.
7. Both patch bodies swallow their import failure with no log line — fixed-in-PR (commit a4d555b)
Agreed. Both handlers now latch a one-time verbose_logger.warning, using the same discipline as the module's other warn-once sites (latch set only after the emit returns). Patch 10 imports verbose_logger inside the handler — streaming_chunk_builder_utils.py does not carry it at module scope, unlike utils.py; patch 11's is still wrapped in its own try because raising from an except block would propagate into a live request.
Also added test_patch_bodies_parse_at_their_insertion_indentation, which ast.parses each replacement at its real insertion indentation with feature_version=(3, 11). These payloads are string literals assembled line by line and now carry a nested handler; without it a bad indent surfaces only at a real image build.
8. A string tier boundary degrades to "boundaries unparseable" — fixed-in-PR (commit a4d555b)
Agreed. New _boundary() mirrors _price's coercion: accepts int and decimal-string, refuses bool, non-finite, and non-integral floats (a fractional token boundary means the field is not what we think it is). Test: test_a_string_tier_boundary_is_parsed_like_every_other_number, plus the parametrized test_an_inexpressible_boundary_is_still_declined covering 128000.5 / True / None.
9. Type comment on the cache record is wrong for one field — fixed-in-PR (commit a4d555b)
Agreed. Now "declined_thresholds": tuple[int, ...] | None.
On the STRUCTURE.md sub-point — agreed, .ruff.toml is listed now, with the reason it is load-bearing rather than just its name.
Verification: make lint clean. .venv/bin/python -m pytest tests/config/ -q → 350 passed (was 326; +24 new). Full suite left to CI on this HEAD.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3695 — delta 939cfa2b → a4d555bc
Approving. All nine findings from the prior round are addressed in code, not
prose, and the delta introduces no blocking issue. What I re-verified rather
than took on faith:
_TIER_SLOTS is transcribed correctly. I pulled litellm/utils.py at
v1.86.2 and diffed the table against the ModelInfoBase(...) enumeration at
5833–5913. It matches exactly, including the two gaps the response leaned on:
there is no cache_read_input_token_cost_above_128k_tokens and no
cache_creation_input_token_cost_above_272k_tokens anywhere in that
enumeration, so those really are inexpressible and the per-component
all-or-nothing rule is load-bearing rather than defensive. My original finding
1 proposed a boundary-only set ({128000, 200000, 272000}); the response is
right that it would have shipped the exact silent under-report the module
declines a whole card to avoid, and the correction is the reason I'm not
re-raising it.
The translation is correct on real data. I ran _cost_entry over a live
pull of the 367-model roster and asserted, for every translated card, that
every published surcharge component the module prices at base has a matching
tier key emitted with the published value. 25 priced, 24 declined, zero
mismatches — the counts in the response reproduce. x-ai/grok-4.5 translates
its 200k tier across prompt/completion/cache_read; qwen/qwen3-max (32000)
still declines.
Partial-slot fallback does not under-report. The concern with emitting a
128k prompt/completion tier is what LiteLLM does with the cache components at
that boundary. _get_token_base_cost guards on if cache_creation_tiered_key in model_info, and ModelInfoBase always carries the enumerated keys with
None, so the guard passes on a None value — but _get_cost_per_unit
(llm_cost_calc/utils.py:349) returns its default_value when the value is
None, and the call site passes the base rate as that default. Cache costs
fall back to base, which is what OpenRouter charges when the override doesn't
surcharge them. No leak.
The two new _PRICE_KEYS mappings are safe, including the zero case. Both
land on fields the chat cost path actually reads
(calculate_cache_writing_cost:416, generic_cost_per_token:761-770). The
hazard I checked for is a published "0": generic_cost_per_token prices
reasoning tokens at output_cost_per_reasoning_token if present, else at
completion_base_cost, so emitting a literal 0.0 would make reasoning tokens
free rather than billed at the completion rate. On the live roster
internal_reasoning is absent on 339 models and non-zero on 28 — zero on
none — and input_cache_write_1h is absent on 337 and non-zero on 30, also
zero on none. _price returns None for an absent key so the field isn't
emitted and the fallback holds. The mappings add signal without a zero-valued
foot-gun.
Findings 3–9. _fetch clears _WARNED_DECLINED_PRICING only on the
success path, after the not capabilities bail — the latch is not re-armed by
a failed fetch. _lookup(model, field) returns the first record that answers
and keeps the first seen as fallback, so get_model_cost_entry can still
explain a decline; get_supported_parameters degrades to None on a
pricing-only fallback exactly as before. strip_variant=False on the pricing
half keeps the openrouter/ prefix strip, which the existing
openrouter/qwen/qwen3-max test confirms still resolves. _boundary refuses
bool, non-finite, and non-integral floats. Both patch handlers latch after the
emit, and I independently ast.parsed each replacement at its real insertion
indentation with feature_version=(3, 11) — both parse; verbose_logger is at
module scope in utils.py:394 for patch 11 and litellm._logging.verbose_logger
exists for patch 10's local import.
tests/config/test_litellm_runtime_modules.py and
tests/config/test_patch_litellm_cache.py: 151 passed. Per review policy I
did not run the full suite; CI covers it.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…cts in patch_litellm_cache.py, Dockerfile, STRUCTURE.md, per-agent-models.md, and the two litellm test modules main landed its own Patch 10 (OpenrouterConfig.transform_request assistant reasoning round-trip, #3698) while this branch independently added Patch 10 (streamed cost preservation) and Patch 11 (openrouter live pricing). Every conflict is a numbering collision on otherwise-additive work: the two sides patch different functions in different files with disjoint needles. Resolution keeps main's already-landed patch at 10 and renumbers this branch's to 11 (streamed cost preservation) and 12 (openrouter live pricing). Totals become 12 patches / 5 modules. Renumbering was carried through to every downstream reference, including files that had no conflict: cost_callback.py, stream_cost_preservation.py, and k8s/base/litellm-deployment.yaml. The runtime module test's py311 parse-coverage tuple gained main's new openrouter_reasoning_roundtrip.py.
Merge conflict resolutionMerged The two sides patch different functions in different files with disjoint needles — there is no behavioural overlap. Resolution keeps main's already-landed patch at 10 (renumbering committed content would churn it and its tests for no gain) and renumbers this branch's two to 11 and 12. Totals are now 12 patches / 5 modules.
Renumbering follow-through outside the conflictsRenumbering invalidated references in three files that had no conflict. Found by a post-resolution
Verification
For reviewers
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3695 — delta a4d555bc → ca7042e1
Approving. The delta is one commit — the origin/main merge at 664d2256 and its
conflict resolution. No new code, no behavioural change to anything I signed off on
last round. What I verified rather than took on faith:
main's Patch 10 survived byte-intact. git diff 664d2256 ca7042e -- config/litellm/patch_litellm_cache.py touches exactly one line inside main's
reasoning-round-trip block: "Patch 10/10" → "Patch 10/12". The needle, the
present marker, the whole replacement payload including its nested
verbose_logger warn-once handler are context lines. Same for main's five
test_patch10_* tests in tests/config/test_patch_litellm_cache.py — that file's
diff against main is a pure append (plus the import ast this branch needs).
The renumbering is internally consistent and the patch set is still coherent. I
loaded PATCHES and NEW_MODULES and asserted mechanically: every label's
denominator is /12, every module label is Module N/5 in order, all label and
all present markers are unique, no patch's needle is a substring of another's in
the same file, and no patch's replacement injects or clobbers another patch's
needle or present marker. The three patches now sharing
llms/openrouter/chat/transformation.py (1, 7, 10) are order-independent under that
check — 7 sits in get_supported_openai_params, 10 in transform_request.
The _IMAGE_SOURCES judgement call is right and is load-bearing.
_IMAGE_SOURCES did not exist on main — this branch introduced it — so adding main's
openrouter_reasoning_roundtrip.py to it is the correct follow-through, not scope
creep. It is not a free assertion either: that module is Python that runs on the
image's 3.11, config/litellm/.ruff.toml covers the directory, and without the entry
a ruff format PEP 758 rewrite in it would reach a build unnoticed — the exact
failure this branch added the pin and the test for.
Renumbering follow-through in the non-conflicted files checks out. Repo-wide grep
for Patch N references: stream_cost_preservation.py:53 → 11,
cost_callback.py (5 sites) → 11/12, k8s/base/litellm-deployment.yaml → 12 for the
pricing knob and 11 for the cost note with main's Patch 10 comment left alone,
STRUCTURE.md → 10/11/Patches 7 + 12, per-agent-models.md → 11/12 with main's
Patch 10 section untouched. Counts updated in lockstep: Dockerfile "twelve gaps" /
"all five", patch script docstring "Twelve" / "five new modules", Module 5/5 added
for stream_cost_preservation.py alongside the retained Module 4/5 roundtrip COPY.
Merge hygiene. git diff 664d2256 ca7042e --stat touches only this PR's twelve
files — main's orchestrator/gateway/shared work came across untouched. Zero conflict
markers. tests/config/test_litellm_runtime_modules.py +
tests/config/test_patch_litellm_cache.py: 208 passed. Per review policy I did
not run the full suite; CI covers it.
— Authored by egg
|
egg review completed. View run logs 6 previous review(s) hidden. |
Closes #3691.
Every routed agent call logged
cost: nullandcost_estimated: null— 1252 of 1252 sampledcost_callbacklines on run 6 — so egg had no dollar figure at all for its LLM spend. Two independent, egg-side causes, one patch each. Both traced and verified against the pinned litellm 1.86.2.Patch 10 — the bill survives streaming
costis what OpenRouter charged. It arrives on the final streamed usage chunk and reaches litellm intact:chunk_parserhands the raw block toModelResponseStream, whoseUsagekeepscostas a declared field andcost_detailsas a pydantic extra. ThenChunkProcessor.calculate_usagerebuildsUsagefield-by-field over the token counts it enumerates and re-constructs it from its ownmodel_dump()— and the bill is gone.Claude Code streams every
/v1/messagesrequest, so that seam was ~100% of routed traffic. The non-streaming path was never affected (original_responsethere carries the raw provider JSON), which is why this read as a property of the route rather than as a transport bug.The new
config/litellm/stream_cost_preservation.pycarriescost/cost_detailsacross the rebuild and interprets neither — a zerocostis the literal BYOK truth with its fall-through partner alongside it, andcost_callback._extract_costreads the result with no change of its own.Correction to the issue. Fix 2 proposed requesting usage accounting via
extra_body: {usage: {include: true}}. That is already unconditional in stockOpenrouterConfig.transform_request; the number was always arriving. No config change was needed.Correction to this PR's own framing, worth knowing before review: upstream litellm fixed the
costhalf of this in v1.94.0 (commit8a49423, 2026-07-06 — a port of BerriAI/litellm#16162). egg pins 1.86.2, which predates it, so patch 10 is effectively a backport. Two consequences:costbut notcost_details, so the BYOK fall-through (cost: 0+cost_details.upstream_inference_cost) stays broken there. On a bump, patch 10 should shrink to thecost_detailshalf rather than disappear. Tracking that separately.Patch 11 — the estimate has a rate card
cost_estimatedis litellm's ownresponse_cost, null becausemodel_prices_and_context_window.jsoncarries none of the slugs egg routes — the same lag that broke the reasoning-param gate in patch 7 (#3624).openrouter_capabilitiesgrows a second entry point off the roster it already fetches, hooked into_get_model_info_helperat the "isn't mapped yet" raise so it runs only after every stock lookup has failed: a mapped slug keeps its bundled rate, and the live card can add a model but never reprice one.I chose the live card over the issue's fix 1 (static
litellm_settingsentries) because the model list is per-operator and deliberately not in the repo, and the machinery for exactly this lag already exists one module over.Cost fields only — no context lengths, no
supports_*flags. Asupports_reasoning: truethrough that door would make stockget_supported_openai_paramsadmitthinking, which patch 2's notes explain would forward an Anthropic-shaped block to a provider expectingreasoning. Patch 7 stays the only path by which a parameter becomes admissible.Judgment call: tiered rate cards are declined, not approximated
OpenRouter publishes a long-context surcharge as
pricing.overrideskeyed by an arbitrarymin_prompt_tokens— qwen3-max charges 2x above 32000 and 2.5x above 128000. LiteLLM has slots for three fixed thresholds and drops the rest, so a faithful translation does not exist in general. Registering the base tier would under-report by 2-2.5x on precisely the long-prompt turns agent traffic is made of, silently, under a field an operator would use to choose a model.So those models stay
cost_estimated: nulland log the reason once, atwarning, naming the tiers. Their provider-billedcostis exact and is the number to read. 49 of 367 models on the roster are tiered; the other 87% — includinglaguna-s-2.1, the issue's exemplar — get a real estimate.Fixes 3 and 4 from the issue are moot: post-hoc
/api/v1/generationwould cost an extra request per call for the figure patch 10 now recovers for free, and the non-streaming calibration sample has nothing left to calibrate.Unplanned:
ruff formatwas emitting Python the image cannot importruff formatunder the repo'starget-version = "py314"rewroteexcept (TypeError, ValueError):into the PEP 758 unparenthesized form — a hard SyntaxError on the litellm base image's Python 3.11. It broke the module and only surfaced when I ran the patched tree against the real library;make lintandmake testwere both green over it, because everything in this repo exceptconfig/litellm/runs on 3.14.A formatter rewrite has no
noqaescape, soconfig/litellm/.ruff.tomlpins that directory topy311(inheriting the root rule set viaextend). A test parses all five image sources atfeature_version=(3, 11)so the outcome is asserted independently of the pin surviving a future config reshuffle.Verification
stream_chunk_builderyieldscost: 0.00123+cost_details;get_model_info("openrouter/poolside/laguna-s-2.1")returns a priced entry;completion_costcomputes$0.0023on a 100k-prompt / 90k-cached turn, matching hand arithmetic;qwen/qwen3-maxis declined with the tier warning; an unknown slug still raises the stock error.make lintclean. Full suite: 22787 passed, 3 failed —test_reap_stale_egg_images(x2) andtest_git_client::test_worktrees_parent_detected, all three reproducing identically on cleanmainat 38b029d. Pre-existing, unrelated.Docs
docs/guides/per-agent-models.mdgains a section on where each dollar figure comes from and when it is null, including the tiered-pricing caveat and ajqquery for reading the two fields together.LITELLM_OPENROUTER_PRICING=0is wired as a commented env on the Deployment alongside the existing patch-7/9 knobs.