Bump litellm 1.86.2 -> 1.94.0, retire four absorbed patches (#3697) - #3699
Conversation
Four of the twelve patch entries are now dead weight: upstream absorbed the
fixes. Retired, with the upstream equivalent verified in the 1.94.0 source
rather than inferred from a needle miss:
- the streaming `reasoning_content` -> thinking-block branch
- the first-delta requeue on block transitions, both the sync and async
entries, now generalized upstream behind `_delta_has_content` so it
covers text and thinking rather than only `input_json_delta`
- the `prompt_tokens_details.cached_tokens` fallback
A fifth narrows rather than dies. 1.94.0 carries streamed `cost` natively
(upstream 8a49423, a port of BerriAI/litellm#16162) and feeds it to the cost
calculator, which is more than egg's patch did. It still drops `cost_details`,
where the BYOK number lives — OpenRouter's top-level `cost` is 0 when billing
routes past them. So patch 7 is now `cost_details` only. That needed no code
change: the helper never overwrote a value litellm already set, so it no-ops
on `cost` and supplies the missing half.
Twelve entries -> eight.
TWO patches had to be REPOINTED, and this is the part worth reading before the
next bump. Patch 2 (the cache_control gate) and patch 5 (the drop_params
warning) both stopped matching at 1.94.0 purely because upstream reflowed the
surrounding code — the gate collapsed onto one line and gained
`is_bedrock_arn_model`, the drop site lost a line break. Neither fix was
absorbed. Retiring them on the strength of the miss would have shipped an
image paying full input rate on every Qwen turn, silently.
Verified against the stock 1.94.0 wheel, which is what makes this a finding
rather than a worry: on stock, qwen and deepseek lose `cache_control` and the
`x-anthropic-billing-header` block leaks into every request. On the patched
tree both hold, and gpt-4o-mini still correctly does NOT get `cache_control`
— the gate is broadened, not widened.
A needle miss means "look at this", not "upstream fixed it". Both the script
docstring and the Dockerfile now say so.
Also fixes a live false pass in the patch tests. They looked patches up by
number prefix (`"Patch 4/"`), and numbers are positional: retiring patch 4
silently re-bound that test to the openrouter capability patch, where it
applied an unrelated needle to a fixture built from it and kept reading green.
Lookups are now by the descriptive tail of the label, which is stable across
renumbering, and assert exactly one match so a retired patch fails loudly
instead. The test for the retired patch 4 is deleted rather than repointed.
Verification, all against the real 1.94.0 wheel with the patch script applied:
- all 8 patches apply cleanly and idempotently
- `cost` and `cost_details` both survive stream reassembly
- laguna-s-2.1 prices from the live card; cost math matches hand arithmetic
- cache_control survives for qwen/deepseek and not for gpt-4o-mini; the
billing header is filtered on every route
- the drop_params warning fires; the synthesized reasoning_effort stays
suppressed by default
Still outstanding, and not doable from here: the live re-measurement #3697
lists — cache hit rate on real traffic, and the #3624 reasoning-depth numbers.
Both need a cluster and a model budget. The static behaviour they depend on is
verified above, but the numbers themselves are unconfirmed on 1.94.0.
This comment has been minimized.
This comment has been minimized.
Merge order with #3698: land that one first, then this#3698 (the assistant reasoning round-trip, Patch 10) is branched from Proposed order: #3698, then #3695, then this PR. #3698 is mergeable against Scope of the conflictSeven files are touched by both PRs: A three-way merge of Patch 10 re-verified against the merged tree, not a stock tagBecause a needle miss cannot distinguish "code moved" from "fix landed" (this PR's own point), the
The needle survives because it spans the The premise also still holds on 1.94.0: the Anthropic adapter still parks prior-turn reasoning on What this PR needs to add when it rebases
|
…ch_litellm_cache.py, STRUCTURE.md
Both sides edit the same LiteLLM patch documentation from opposite ends: this branch renumbers the patch set 12 -> 8 for the 1.94.0 bump, while the base rewrites what patch 11 (now 8) does with a tiered rate card — translating an expressible prompt-length surcharge instead of declining every tiered card.
config/litellm/patch_litellm_cache.py — docstring items 7/8. Kept this branch's renumbering and its narrowed item 7 (1.94.0 carries cost natively; the patch is down to cost_details), with the base's updated surcharge-translation wording folded into item 8. Both sides' code changes to the patch bodies auto-merged; only the prose collided.
docs/development/STRUCTURE.md — same shape. This branch's numbering (4 + 8 / 5 / 6 / 7) plus the base's added surcharge clause on openrouter_capabilities.py, and this branch's stream_cost_preservation line, which is the accurate one on 1.94.0.
Three follow-on fixes the textual merge could not make on its own, each a base-side reference to a number this branch reassigned: patch_litellm_cache.py 'patch 10's handler' -> 'patch 7's handler'; docs/guides/per-agent-models.md 'Patch 11 translates' -> 'Patch 8'; and tests/config/test_patch_litellm_cache.py, where the base's new _BODY_CONTEXTS parse test looks patches up by number prefix ('Patch 10/', 'Patch 11/') that no longer exist — StopIteration on every run. Repointed to _patch_by_description(), the number-independent lookup this branch introduced for exactly this failure mode.
tests/config/: 346 passed. make lint clean except three pre-existing mypy errors in shared/egg_agent/client.py, a file neither side touches.
Conflict Resolution SummaryMerged
Three fixes the textual merge could not make on its ownEach is a base-side reference to a patch number this branch reassigned. Git merged them cleanly and wrongly.
Verification
Please review
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed against the real litellm==1.94.0 wheel (extracted and grepped, not the fork). The core of this PR holds up better than most bumps of its size:
- All 8 needles match exactly once in stock 1.94.0. I checked each one by string count against the extracted tree.
- All four retirement claims are genuine upstream absorptions.
getattr(choice.delta, "reasoning_content", None)is now handled inline;_delta_has_contentis used in both__next__and__anext__;_get_cache_read_input_tokens/_get_cache_creation_input_tokenscarry thecached_tokensfallback. - The patch 7 narrowing is correct.
_usage_chunk_calculation_helperreadscostand nevercost_details, and the rebuiltUsagecarries no pydantic extras, socost_detailsis genuinely still dropped.carry_upstream_costnever overwrites, so "no code change needed" checks out. - The "a needle miss means look at this, not upstream fixed it" correction on the Dockerfile and the script docstring is the right lesson from the two repointed patches, and the
_patch_by_description+ exact-count assertion is a real fix for a real false pass.
Two blocking findings below. The first is not in the diff — it is a behaviour change the bump brings in, and it silently defeats the patch the stacked PR #3695 exists to add.
Blocking
1. 1.94.0 short-circuits the cost calculator on the streaming path — cost_estimated goes null on every streamed BYOK call, and patch 8 becomes dead code there
1.94.0 adds a static method that did not exist in 1.86.2 (I downloaded both wheels; grep -rn "_propagate_usage_cost_to_hidden_params" litellm/ returns nothing in 1.86.2):
# litellm_core_utils/streaming_handler.py:1694
@staticmethod
def _propagate_usage_cost_to_hidden_params(response: "ModelResponse") -> None:
_usage = getattr(response, "usage", None)
if _usage is not None and hasattr(_usage, "cost") and _usage.cost is not None:
...
response._hidden_params["additional_headers"]["llm_provider-x-litellm-response-cost"] = float(_usage.cost)It is called on complete_streaming_response at lines 1816 (sync) and 2051 (async), immediately before logging_obj.success_handler gets the copy. In 1.86.2 that header was only ever set by OpenrouterConfig.transform_response — the non-streaming path. This bump extends it to streaming, which per stream_cost_preservation.py's own docstring is "~100% of agent traffic".
Downstream:
# cost_calculator.py:1771
provider_response_cost = get_response_cost_from_hidden_params(response_object._hidden_params)
if provider_response_cost is not None:
return provider_response_cost # <-- completion_cost() at 1775 never runsand get_response_cost_from_hidden_params only returns None when the header value is literally None — 0.0 is returned as 0.0.
Now apply this PR's own stated premise, from stream_cost_preservation.py:10-13:
cost_details.upstream_inference_costis what the upstream provider charges under BYOK (where the former is 0 because billing routes past OpenRouter)
So on egg's route: usage.cost == 0.0 → _usage.cost is not None passes → header is 0.0 → response_cost_calculator returns 0.0 without calling completion_cost().
Two consequences, both of which contradict claims this PR makes:
(a) cost_estimated is null on every streamed call. _extract_estimated_cost (cost_callback.py:353-357) gates both reads on _positive; 0.0 fails both, so it returns None. But this PR's diff writes into that function's docstring:
On the egg-litellm image patch 8 supplies OpenRouter's published rates for slugs the bundled map does not carry, so a None here now means a genuinely unpriceable model
That is now false on the streaming path. It means "OpenRouter billed us zero because this is BYOK."
(b) Patch 8 never fires on the streaming path at all. Its needle sits in _get_model_info_helper's unmapped branch, reachable only via completion_cost() — which is exactly what line 1773 returns before. On 1.86.2 nothing set the header on the streaming path, so completion_cost() ran and patch 8 got its chance. The bump removes that. The PR's verification list says "laguna-s-2.1 prices from the live rate card" — that is a direct _get_model_info_helper call, not the streaming logging path, so it would pass while the live path is dead.
There is a third, quieter facet on non-BYOK routes: usage.cost there is the real OpenRouter charge, so cost_estimated becomes a verbatim copy of cost. cost_callback.py:48-50 states the invariant this breaks: "It is kept strictly separate from cost — an estimate from a possibly-stale rate card must never be mistaken for a bill." After this bump the two fields are the same number, and the cross-check they exist to provide is gone.
I don't think the fix belongs in cost_callback.py — reading a 0.0 there and guessing why is exactly the ambiguity the null-not-zero discipline avoids. The natural place is a ninth patch on _propagate_usage_cost_to_hidden_params that declines to propagate a non-positive cost, which restores completion_cost() (and therefore patch 8) for the BYOK case and leaves the real-charge case alone. Whichever way it goes, it needs to be settled before this ships, and the two docstring claims above need to match whatever lands.
2. Three unrelated tests deleted with no replacement, and the PR description says one
The PR body says:
The test for the retired patch 4 is deleted rather than repointed.
Singular. Four tests are actually gone. test_patch4_needle_anchors_on_content_block_function is the justified one. The other three are not about patch 4, not about numbering, and not affected by the version bump:
test_new_modules_are_installed_into_each_roottest_new_module_install_is_idempotenttest_missing_staged_module_fails_loud
These are the only tests covering NEW_MODULES — the four _egg_* companion modules that patches 4, 5, 6, 7 and 8 all import at runtime. Nothing in the new file calls plc._patch_root at all (base had three call sites; head has zero).
test_missing_staged_module_fails_loud is the one that matters most. Every companion-module import in the patch replacements is wrapped in except Exception: pass (patch 4) or a warn-once handler (patches 7, 8). If a module fails to stage, patches 4 and 6 degrade to stock silently — no warning at all in patch 4's handler. That build-time loud failure was the backstop, and it is the same "green build, unpatched image" failure mode this PR's own Dockerfile comment was rewritten to guard against.
Deleting them may well have been an accident of the rewrite. Either restore them or say in the PR body which coverage is being dropped and why — the current description reads as though only the retired patch's test went.
Non-blocking
3. The two "verbatim from 1.86.2" stock fixtures no longer match 1.94.0, and nothing catches it
test_capability_gate_is_additive_not_substitutive builds its input as patch7["needle"] + stock_gate — a synthetic file, never the real source. So when upstream reflows the tail, the fixture silently becomes a snapshot of a version the image no longer runs. Both fixtures have already drifted:
tests/config/test_patch_litellm_cache.py:297-306 asserts the gate spans three lines:
" if litellm.supports_reasoning(\n"
' model=model, custom_llm_provider="openrouter"\n'
" ) or litellm.supports_reasoning(model=model):\n"1.94.0 llms/openrouter/chat/transformation.py:42-44 has it reflowed onto a different split. Not in the file.
_STOCK_THINKING_TAIL_HEAD (line 341-353) asserts if self.is_anthropic_claude_model(model):. 1.94.0 line 971 is if self.is_anthropic_claude_model(model) or self.is_bedrock_arn_model(model): — the bedrock branch was added, and the translate_anthropic_thinking_to_reasoning_effort call below it collapsed from three lines to one.
Neither breaks the real patch (both needles sit elsewhere and match). But test_thinking_gate_leaves_the_claude_branch_alone now asserts "patch 9 must leave the Claude path alone" against a Claude path that no longer exists, and reads green. This is the same class of false pass as the "Patch 4/" prefix lookup — the number-keyed lookup got fixed, the hand-built fixture underneath it did not. Comparing against the real installed tree (or at minimum a comment noting the fixtures are pinned snapshots that need re-verifying on every bump) would close it.
4. Nine stale patch cross-references in patch_litellm_cache.py; one ships into the built image
Self-references were renumbered (see the patch 9 notes → patch 6, patch 10's handler → patch 7's, see patch 11 notes → patch 8). Cross-references to the old 7 (capabilities) and 8 (drop_params) were not, and they still parse as sensible prose while pointing at the wrong patch:
| line | text | should be |
|---|---|---|
| 421 | # never subtractive — see patch 7 notes |
4 — and this one is inside a replacement string, so it lands in the built image |
| 439 | # Patch 7 removes the OpenRouter false-negative |
4 |
| 491 | # This is the other half of patch 7, and without it patch 7 is a |
4 |
| 504, 509, 513 | Patch 7 unblocks the param / patch 7 alone would cut reasoning ~9x / patch 7's actual goal |
4 |
| 510 | patch 8 only fires on drops |
5 |
| 669 | # Same root cause as patch 7, second symptom |
4 |
Line 669 is the worst of them: under the old numbering it correctly linked pricing-map staleness to capability-map staleness; under the new one it points at the cost patch and still reads plausibly.
5. Every renamed test still carries the old number in its docstring, locals, and assertion messages
The lookup is number-independent now, which was the point. The prose is not:
test_capability_gate_is_additive_not_substitutive—patch7 = _patch_by_description(...), docstring "Patch 7 must UNION…","patch 7 must not replace the stock model-map branch","patch 7 must not short-circuit the stock branch"test_thinking_gate_leaves_the_claude_branch_alone—patch9 = …, "Patch 9 must stop the adapter…", "so Patch 7 alone would silently shallow every agent turn"test_drop_params_needle_disambiguates_the_two_drop_sites—patch8 = …,"patch 8 rewrote the embeddings-path drop site", and "litellm 1.86.2 has TWOdrop_paramsbranches in utils.py" as the docstring's first line in a PR that moves off 1.86.2test_cost_details_carry_runs_after_the_rebuild_not_before—patch10 = …, "Patch 10 must land on the far side…", and a rationale entirely aboutcost, which is no longer this patch's jobtest_pricing_hooks_the_unmapped_branch_…/test_pricing_needle_disambiguates_…—patch11 = …, "Patch 11 must be a fallback", "same needle-uniqueness trap as Patches 4 and 8" (4 is retired; 8 is now the patch under test)
The hazard the PR set out to remove got moved from the lookup key into the prose, where a _patch_by_description assertion can't catch it and a reader debugging a failure gets sent to the wrong patch.
6. Patch 7's operator-facing warning names the wrong field after the narrowing
patch_litellm_cache.py:651-653:
'egg cost patch: streamed cost preservation is inactive (%s: %s); '
'the provider-billed `cost` will read null on every streamed call.'
After the narrowing, cost is carried by upstream. If this import fails, cost is fine and cost_details is what's lost — so on BYOK the operator gets told to look at a field that reads correctly. Same for the test's assertion comment at line 522 ("cost would still read null on every streamed call").
7. Prose reflow leftovers in cost_callback.py
The docstring edits left three lines that don't match the file's ~79-col wrapping: the ragged the streaming path too / and this module needs no change break around line 34, and the long lines at ~41 (BYOK route on an unpatched image, or a provider that does not report one. It is still emitted as null rather) and ~382 (cost read null on every line before 1.94.0 and patch 7 — worth having next to the`).
Checked and clear
For the record, since this is the image every agent request goes through — I traced these and found nothing wrong: needle uniqueness for all 8 against real 1.94.0; the four retirements against their upstream replacements; _delta_has_content covering the requeue case for text and thinking; carry_upstream_cost's never-overwrite and non-finite guards; patch 6's placement after the adaptive override; patch 2's substring gate not admitting gpt-4o-mini; and the wheel being pure Python, so .py string-patching is valid. translate_thinking_for_model at adapters/transformation.py:657 duplicates the synthesis logic patch 6 gates and is not gated — but it has no callers anywhere in the wheel, so it's dead code, not a gap.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… tests, refresh stock snapshots - Add patch 9 (billable-cost gate on hidden params): 1.94.0's _propagate_usage_cost_to_hidden_params short-circuits response_cost_calculator on any non-None usage.cost, including a BYOK 0.0 — which suppressed cost_estimated and made the live pricing patch dead code on the streaming path. Narrowed to a positive, finite charge; a real bill propagates unchanged. - Restore the three NEW_MODULES tests dropped in the rewrite, plus a functional test for the new gate. - Re-transcribe the hand-written stock snapshots from the real 1.94.0 wheel (both had drifted) and pin them with a Dockerfile FROM assertion. - Fix stale patch cross-references, the patch 7 warn message naming the wrong field, test prose/locals still keyed on old numbers, and cost_callback prose.
Review response — all 7 findings addressed in
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-reviewed the delta (b2fb977d) against a real litellm==1.94.0 wheel, extracted and patched, not against the fork. Both blocking findings are genuinely fixed, and I verified the fix end-to-end rather than taking the response at its word.
Finding 1 — _propagate_usage_cost_to_hidden_params short-circuit: fixed
Patch 9 is correct at every link in the chain I traced:
- Needle matches.
streaming_handler.py:1703-1704in the real wheel is byte-identical to the needle, and I counted every one of the nine needles against the extracted tree: all exactly 1. - The gate is right. I applied all 9 patches + 4 modules to a real 1.94.0 tree — clean apply, idempotent on a second pass, every patched file
ast.parses atfeature_version=(3, 11), every present-marker count is 1. Reading the patchedstreaming_handler.py:1703-1723:0.0/0/-1.0/-inffail> 0;NaNfails> 0(all comparisons false);+inffails< float("inf");Trueis excluded by theboolcheck; a missingcostyieldsNonefrom thegetattr.0.0031propagates unchanged. - The restoration is real, not assumed.
get_response_cost_from_hidden_params(cost_calculator.py:1691-1705) returnsNonewhen the header key is absent — not just when its value isNone— so with the gate closed, line 1771'sprovider_response_cost is not Noneis false andcompletion_cost()runs. That is what puts patch 8's_get_model_info_helperhook back on the streaming path and lets_extract_estimated_costsee a positive number.
The narrow form is the right call: a route reporting a genuine charge still short-circuits, and the docstrings now say so instead of claiming independence unconditionally. The updated text in cost_callback.py (module docstring and _extract_estimated_cost), docs/guides/per-agent-models.md, and the patch-8 block's "PATCH 9 IS LOAD-BEARING FOR THIS ONE" note all match the behaviour I executed.
Finding 2 — deleted NEW_MODULES tests: fixed
test_new_modules_are_installed_into_each_root, test_new_module_install_is_idempotent, and test_missing_staged_module_fails_loud are back and exercise plc._patch_root / plc._module_source for real. _build_fixture_root derives from PATCHES, so the new F6 file is created generically and the restored tests cover the 9-patch set without special-casing.
Finding 3 — drifted stock fixtures: fixed properly
I checked all four hand-written snapshots against the extracted 1.94.0 tree by substring count: _STOCK_CAPABILITY_GATE, _STOCK_THINKING_TAIL_HEAD, _STOCK_THINKING_TAIL_FOOT, and the new _STOCK_HIDDEN_PARAMS_BODY each occur exactly once. They are verbatim, not approximations — so test_thinking_gate_leaves_the_claude_branch_alone is now asserting against a Claude path that actually exists. test_stock_snapshots_are_pinned_to_the_image_litellm correctly parses the single FROM ghcr.io/berriai/litellm:v1.94.0 at Dockerfile:32.
The new patch-9 tests execute the patched body through plc._apply rather than substring-matching the replacement, which is the right shape — a broken gate would fail them.
Verification run
tests/config/: 359 passed (35 intest_patch_litellm_cache.py).ruff check+ruff format --checkonconfig/litellm/and the test file: clean.- Patch-count and module-count consistency:
nine gaps/nine patchesin the Dockerfile andper-agent-models.md, four staged modules in both the DockerfileCOPYlist and the script docstring — patch 9 is inline and correctly needs none. Renumbered cross-references inpatch_litellm_cache.py,anthropic_thinking_policy.py:34, andopenrouter_capabilities.py:587all point at the right patch under the new numbering.
No blocking issues in the delta. Approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…lm-bump: resolve conflicts in patch_litellm_cache.py, its tests, Dockerfile, STRUCTURE.md, per-agent-models.md, litellm-deployment.yaml, stream_cost_preservation.py The conflict is renumbering, not semantics. The base branch carries 12 patches; this branch retired four that 1.94.0 absorbed and added one, leaving 9. The base independently added one genuinely new patch (the prior-turn assistant reasoning round-trip, from #3698). Target: 10. Every conflict is a patch number that moved. The one substantive decision was the round-trip patch, which was authored against 1.86.2: rather than assume it survived the bump, its needle was checked against the real 1.94.0 wheel. It matches, once, unmodified — and the anchoring comment inherited from the base was stale, since _supports_cache_control_in_content appears three times in 1.94.0, not twice. The comment is corrected here. The absorbed round-trip tests are converted off the number-prefix lookup (startswith 'Patch 10/') onto _patch_by_description(), the renumbering-resilient helper this PR introduced after a prefix lookup silently re-bound to a different patch and produced a false pass. One duplicate test the merge produced — the base's test_patch11_sets_cost_after_the_rebuild_not_before, which this branch had already renamed to test_cost_details_carry_runs_after_the_rebuild_not_before — is dropped. Three stale patch numbers that git auto-merged without flagging a conflict were found by sweeping the resolved files for patch references and corrected: litellm-deployment.yaml:134, and two in per-agent-models.md. Verified: patch script applies all 10 patches plus 5 modules to a real litellm==1.94.0 wheel, second pass fully idempotent, all patched files compile, Patch 7 confirmed by AST to land inside transform_request exactly once. 217 targeted tests pass; make lint clean.
Merge conflicts resolvedMerged The conflict is renumbering, not semantics. Base carries 12 patches. This branch retired four that 1.94.0 absorbed and added one, leaving 9. Base independently added one genuinely new patch — the prior-turn assistant reasoning round-trip, from #3698. Target: 10. Nearly every conflict hunk is a patch number that moved. Per-file
Two things worth a reviewer's eye1. The round-trip patch was re-verified against the wheel, not assumed. It was authored against 1.86.2; this PR bumps to 1.94.0. That is precisely the situation this PR exists to warn about — a needle miss cannot distinguish "the code moved" from "the fix landed." So rather than assume it survived, I downloaded the real 2. Three stale patch numbers that git auto-merged without flagging a conflict. Found by sweeping every resolved file for patch references after resolving, not by git: I also converted base's four round-trip tests off Verification
Full On merge order@jwbron — noted that #3698 should land first. It already has: base ( — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-reviewed the delta since b2fb977d — the conflict-resolution merge a7975aa0.
Scope note. The delta is a single merge commit, so git log b2fb977d..HEAD --not origin/egg/issue-3691-cost-visibility -p renders no patch text. I reviewed the resolution by diffing the result against both parents (b2fb977d = PR side, ca7042e1 = base side) and against their merge base a4d555bc, which is the only way to see what the resolution kept, dropped, and renumbered. I also re-verified the patch set end-to-end against a real litellm==1.94.0 wheel, because the merge grafts in a patch (#3698's round-trip) that was authored against 1.86.2 — exactly the situation this PR exists to warn about.
The grafted patch 7 was verified, not assumed
I downloaded and extracted the real litellm-1.94.0 wheel and counted every needle in the stock tree:
Patch 1/10 … Patch 10/10 needle_count=1 present_in_stock=0 (all ten)
All ten match exactly once, including Patch 7/10 (assistant reasoning round-trip). Running _patch_root against that tree: 5 modules installed, 10 patches applied, second pass byte-identical (zero drifted files), every patched file ast.parses at feature_version=(3, 11).
Placement is right too — walking the patched llms/openrouter/chat/transformation.py AST, _egg_map_reasoning(messages, model) occurs once in the whole file and its enclosing function is transform_request, not the sibling remove_cache_control_flag_from_messages_and_tools the needle-anchoring comment calls out. Loading the installed _egg_reasoning_roundtrip.py standalone and exercising it: thinking blocks join on \n into reasoning_content and thinking_blocks is stripped (including the None sentinel), openrouter/anthropic/* is declined untouched, and a caller-set reasoning_content is not overwritten. Matches the documented contract.
I diffed the patch-7 spec against the base's version: file, present, and needle are byte-identical, replacement differs only in two comment renumbers. The resolution did not alter its semantics, and openrouter_reasoning_roundtrip.py is unchanged from base.
Both prior blocking items survived the merge intact
- Patch 10 (billable-cost gate) — the gate body is unchanged from
b2fb977d; only the label moved9/9→10/10. The positive-and-finite,bool-excluding guard is still there. - The
NEW_MODULEStests —test_new_modules_are_installed_into_each_root,test_new_module_install_is_idempotent, andtest_missing_staged_module_fails_loudare all present in HEAD.
No base-side work was dropped by "took ours"
This was the resolution's main risk, so I checked it rather than trusting the summary. Comparing base-side changes since the merge base:
cost_callback.py— base's only changes were patch renumbers (10→11,11→12) under its 12-patch scheme. Taking the PR side, which is keyed on descriptive names, loses nothing.test_patch_litellm_cache.py— every base-side test name absent from HEAD has a renamed equivalent (test_patch10_*→test_roundtrip_*,test_patch12_*→test_pricing_*,test_patch9_*→test_thinking_gate_*,test_patch11_sets_cost…→test_cost_details_carry_runs…). The only genuine deletion istest_patch4_needle_anchors_on_content_block_function, the retired patch's test. 25 → 31 test functions.test_litellm_runtime_modules.py— came through whole (+429).
Wiring and consistency
_patch_by_description still resolves all seven lookups to exactly one label each with the eleventh patch added. The four pinned stock snapshots (_STOCK_CAPABILITY_GATE, _STOCK_THINKING_TAIL_HEAD, _STOCK_THINKING_TAIL_FOOT, _STOCK_HIDDEN_PARAMS_BODY) each still occur exactly once in the real 1.94.0 tree. The Dockerfile COPYs all five staged modules and still pins v1.94.0, so test_stock_snapshots_are_pinned_to_the_image_litellm holds. LITELLM_OPENROUTER_REASONING_ROUNDTRIP in litellm-deployment.yaml:142 matches the module's ENV_VAR. No conflict markers anywhere in the resolved files.
Tests
tests/config/test_patch_litellm_cache.py — 39 passed. tests/config/test_litellm_runtime_modules.py — 178 passed. (Per this workflow's constraints I did not run the full suite.)
No blocking issues in the delta. Approving.
— Authored by egg
|
egg review completed. View run logs 8 previous review(s) hidden. |
Post-merge: the needle verification re-run against
|
| check | result |
|---|---|
| round-trip needle in stock 1.94.0 | 1 match |
| after the full set applies | consumed, marker present exactly once |
| all 10 patches and 5 modules | apply cleanly |
| second pass | no-op, file unchanged |
| patched file | parses |
transform_request on poolside/laguna-s-2.1 |
thinking_blocks becomes reasoning_content |
transform_request on anthropic/* |
still declined, thinking_blocks left intact |
So the needle survived both the version bump this PR performed and the three-way merge around it,
and it survived for the reason given at the time: it spans the cache_control pair plus the
following extra_body pop, and 1.94.0's reflow of the super().transform_request(...) call falls
one line below the needle's end.
One caveat on my earlier comment
It asserted the merged set was verified "against the actual merged tree". That was true of the tree I
could construct at the time, by three-way merging this branch with #3698. It was not the tree that
actually landed, which also contains #3700. The table above is against what landed.
1 participant
Closes #3697. Stacked on #3695 — it needs the two patches that PR adds. Review that one first; this targets its branch.
Bumps
ghcr.io/berriai/litellmfrom v1.86.2 to v1.94.0 (the newest stable published tag) and takes the patch set from twelve entries to eight.Retired — four, each verified in the 1.94.0 source
reasoning_content→ thinking-block branch_delta_has_content, so it now covers text and thinking rather than onlyinput_json_deltaprompt_tokens_details.cached_tokensfallbackNarrowed — one
1.94.0 carries streamed
costnatively (upstream8a49423, a port of BerriAI/litellm#16162) and goes further than egg's patch did, feeding the figure to litellm's own cost calculator. It still dropscost_details, which is where the BYOK number lives — OpenRouter's top-levelcostis0when billing routes past them.So patch 7 is now
cost_detailsonly. That needed no code change: the helper never overwrote a value litellm had already set, so it no-ops oncostand supplies the missing half. The scope narrowing is entirely documentation.Repointed — two, and this is the part to read
Patch 2 (the cache_control gate) and patch 5 (the drop_params warning) both stopped matching at 1.94.0 — purely because upstream reflowed the surrounding code. The gate collapsed onto one line and gained
is_bedrock_arn_model; the drop site lost a line break. Neither fix was absorbed.That matters because #3697 originally told the reader to treat a needle miss as the signal to delete. It isn't. A miss means the code moved or the fix landed, and the miss alone cannot tell you which.
Verified against the stock 1.94.0 wheel:
Deleting patch 2 on the needle miss would have shipped an image paying full input rate on every Qwen turn — the ~10x regression the image exists to prevent — with a green build and no log line. Note the last row: gpt-4o-mini still correctly does not get
cache_control. The gate is broadened, not widened.The script docstring and the Dockerfile now both say a needle miss means "look at this", not "upstream fixed it".
I've posted a correction on #3697: my original audit was built by diffing against the fork, which carries its own local patches, so three fork-local fixes read as "upstream absorbed it". Redone against the stock wheel.
Fixes a live false pass in the patch tests
The tests looked patches up by number prefix (
"Patch 4/"). Numbers are positional, so retiring patch 4 silently re-bound that test to the openrouter capability patch — where it applied an unrelated needle to a fixture built from that same needle, and kept reading green while testing nothing.Lookups are now by the descriptive tail of the label, which is stable across renumbering, asserting exactly one match so a retired patch fails loudly instead. The test for the retired patch 4 is deleted rather than repointed.
Verification
All against the real 1.94.0 wheel with the patch script applied:
costandcost_detailssurvive stream reassemblylaguna-s-2.1prices from the live rate card;cost_per_tokenmatches hand arithmetic on a 100k-prompt / 90k-cached turncache_controlsurvives for qwen/deepseek, not for gpt-4o-mini; the billing header is filtered on every route (table above)drop_paramswarning fires with the full remedy textreasoning_effortstays suppressed by defaultmake lintclean;tests/config/328 passed.What is NOT done
The two live measurements #3697 lists need a cluster and a model budget, and I could not run them:
The static behaviour both rest on is verified above —
cache_controlreaches the wire, the synthesis stays gated — but the numbers themselves are unconfirmed on the new base. Worth a smoke run before this rolls to anything that matters.