Skip to content

litellm: carry prior-turn assistant reasoning to OpenRouter (Patch 10) - #3698

Merged
jwbron merged 2 commits into
mainfrom
egg/litellm-reasoning-roundtrip
Jul 29, 2026
Merged

litellm: carry prior-turn assistant reasoning to OpenRouter (Patch 10)#3698
jwbron merged 2 commits into
mainfrom
egg/litellm-reasoning-roundtrip

Conversation

@jwbron

@jwbron jwbron commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Correction after merge: the "keeps the seed alive" framing below is too strong. See
#3698 (comment)

The defect

On egg's primary route (Claude Code -> gateway -> LiteLLM /v1/messages -> OpenRouter),
assistant reasoning from prior turns was never sent back to the model.

Verified against pristine BerriAI/litellm at v1.86.2, the version the image pins:

  1. LiteLLM's Anthropic adapter converts incoming thinking content blocks into
    assistant_message["thinking_blocks"]
    (llms/anthropic/experimental_pass_through/adapters/transformation.py:662-663).
  2. llms/openrouter/chat/transformation.py has no request-path consumer for that field. Its
    only reasoning line is response-path (reasoning -> reasoning_content on streaming deltas).
  3. reasoning_details appears nowhere under llms/ or litellm_core_utils/.

OpenAIGPTConfig.transform_request puts messages straight into the request body, so
thinking_blocks reached OpenRouter as a field no one reads and every historical assistant turn
arrived with its reasoning missing.

For a model whose chat template re-renders prior thinking this is a malformed history rather than
a lost optimisation. Poolside Laguna renders
'<think>' + message.reasoning|message.reasoning_content + '</think>' for every previous
assistant turn, so each one became a literal empty <think></think>; Poolside's model card warns
this degrades follow-up behaviour.

The gap is upstream's. It predates every egg and jwbron/litellm change. The fork's OpenRouter
work (jwbron/litellm#8) is confined to get_supported_openai_params, which acts on
optional_params and cannot reach a message field.

Precondition check

The fix is only worth making if Claude Code actually returns thinking blocks in the /v1/messages
request body for a non-Anthropic model, so that was confirmed first, by capturing real request
bodies through a logging proxy in front of the LiteLLM proxy.

It does, under egg's configuration. A two-turn tool-calling exchange against
deepseek/deepseek-v4-pro produced a follow-up request whose assistant message carried
['text', 'thinking', 'tool_use'], with the block shaped
{"type": "thinking", "thinking": "...", "signature": ""}.

Two findings worth recording:

  • On poolside/laguna-s-2.1 the blocks do not come back, and the reason is not what this PR
    originally said.
    An earlier version of this body said that route "returned no reasoning content
    to begin with". The outcome was right and the cause was wrong, and the difference decides whether
    this patch is worth landing. See the section below.
  • The host cllm launcher is not representative of egg. It sets
    ANTHROPIC_CUSTOM_MODEL_OPTION_SUPPORTED_CAPABILITIES="", which egg deliberately does not set
    (docs/guides/per-agent-models.md:416-425). The precondition was therefore re-run under
    egg-equivalent environment before being accepted.

Laguna: the patch is load-bearing, and it still cannot fire today

This section supersedes the original claim that Laguna "returns no reasoning at all". It does
reason. What it does not do, on egg's traffic, is ever start.

The patch does real work, and the control is what proves it

Three arms, identical history, max_tokens 800, tools present in every arm. The only thing that
moves is what the prior assistant message carries. Full message sequences are in the reproduction
record on #3595.

arm prior assistant message carries turns that reasoned median reasoning_tokens
pre-patch nothing 0/4 0
post-patch reasoning_content: <prior reasoning> 4/4 105
control the same text as plain content 0/4 0

The control is the load-bearing cell. Identical text, identical token count, attached to a
different field: 4/4 against 0/4. It is specifically reasoning_content, which is exactly what this
patch produces, and not extra context, that restores the channel. The same result holds when the
next turn also emits a tool call (0/3 against 3/3, both arms calling the tool in every draw), so it
sustains through an agent loop rather than only on final answer turns.

Traced end to end through a real patched v1.86.2: a reasoning-bearing response comes out of the
adapter as ['thinking', 'text', 'tool_use'], so Claude Code echoes a thinking block back, and this
patch maps it to reasoning_content on the next request. A response with no reasoning channel comes
out as ['text', 'tool_use'], and there is nothing to map.

Why it is nonetheless inert on egg's Laguna traffic right now

Because nothing seeds the channel. A tool-bearing opening turn on an errand-shaped prompt does not
reason, so there is no first reasoning to carry forward, and a patch that replays reasoning has
nothing to replay. Four-turn sessions, two draws each:

arm opener replay reasoning_tokens per turn
A errand, tools no [0,0,0,0] [0,0,0,0]
B errand, tools yes [0,0,0,0] [0,0,0,0]
C tool-free yes [1601,746,309,70] [1448,490,368,237]
D tool-free no [1198,0,0,0] [1720,0,0,0]

Arm B is this patch alone: worthless without a seed. Arm D is a seed alone: it fires once and is
gone by the very next turn. Arm C is both, and it is the only arm that works. Arm D is also exactly
the shape production shows, a session total of reasoning tokens against otherwise uniform per-call
zeros.

So the original "inert on this route" was right about the outcome and wrong about the cause. Not
"the model does not reason", but "nothing ever primes it to".

The seed is now known, and it is a per-request parameter

Run round-robin with cells interleaved, because the unprimed rate drifts over time and blocking the
cells would confound drift with cell. n=4 per cell, max_tokens 2500, fresh opening turn.

opening turn reasoned reasoning_tokens
tools callable + errand prompt 0/4 [0,0,0,0]
tools + tool_choice: "none" + errand 3/4 [236,0,485,1135]
tools callable + reasoning-shaped 3/4 [0,527,696,522]
tools + tool_choice: "none" + reasoning-shaped 4/4 [1334,1584,1261,2105]
no tools + errand 4/4 [246,638,821,105]
no tools + reasoning-shaped 4/4 [1791,1646,1311,1647]

Callable tools are the strong lever and tool_choice defeats them: 0/4 to 3/4 on a byte-identical
errand prompt with only tool_choice moved. Prompt shape is the weaker lever and mostly drives
magnitude.

Recipe: open every agent session with one turn that sets tool_choice: "none" and is
reasoning-shaped.
4/4 at 1261-2105 seed tokens, indistinguishable from dropping the tools array,
and it is a per-request parameter rather than a change to how egg assembles the tools array, which
makes it far cheaper to ship. That recipe is not in this PR; this PR is the half that keeps the seed
alive once it exists.

Acceptance signal, per turn type, untested

  • On follow-up turns after a tool result, per-call reasoning_tokens should be non-zero instead
    of uniformly zero.
  • On turns that emit a tool call, the same, which is the stronger claim and the one that decides
    whether reasoning survives an agent loop at all.
  • Session-wide averages are the wrong instrument here and should not be used: the pre-patch pathology
    is a per-turn zero, not a lower mean.

Nothing in this PR measures any of that on a real pipeline. It needs a run on the patched image with
the seeding recipe applied.

What is untested, explicitly

The change

Patch 10 maps assistant thinking_blocks onto reasoning_content in
OpenrouterConfig.transform_request and removes thinking_blocks so no unknown field is
transmitted.

OpenRouter accepts reasoning, reasoning_content and reasoning_details interchangeably on an
assistant message and documents this for exactly this multi-turn tool-calling case; Poolside's
template reads message.reasoning / message.reasoning_content. The plain string form is used
because reasoning_details exists to carry encrypted or summarised blocks and the adapter
produces neither.

The logic lives in a fourth staged module (config/litellm/openrouter_reasoning_roundtrip.py
-> llms/openrouter/_egg_reasoning_roundtrip.py), following the same convention as Modules 1-3,
so it stays lintable and unit-testable in this repo rather than living as a string literal. Module
labels renumbered N/3 -> N/4 and patch labels N/9 -> N/10; the Dockerfile stages the new
file; the module docstring gains the tenth entry.

Edge cases, all covered by tests: multiple blocks concatenate in order; redacted_thinking carries
opaque data and contributes nothing; a whitespace-only or empty result emits no field rather than
re-creating the empty <think></think>; signature is not forwarded; assistant messages only;
input is not mutated; fail soft, so a block that cannot be parsed leaves its message exactly as it
arrived; idempotent.

Needle anchoring. _supports_cache_control_in_content and _move_cache_control_to_content are
each named twice in 1.86.2, so neither call alone is unique. The needle spans the cache_control
pair and the following extra_body pop, a sequence that occurs only in transform_request.
Confirmed to match real stock v1.86.2 exactly once, with a test that pins the anchoring against a
sibling-call-site fixture (same discipline as Patches 4 and 8).

Direction. This is request-path (client -> provider). Patches 4, 5a and 5b are response-path
(provider -> client). Adjacent, not the same thing; a test asserts this patch never touches
transform_response.

Validation

Applied the full script to a real stock litellm==1.86.2 install: all 10 patches and 4 modules
applied, and a second run reported all 15 already applied (idempotent).

Replayed a real captured Claude Code /v1/messages body through that patched proxy with the
provider endpoint captured:

outgoing assistant message
before ['role', 'thinking_blocks', 'tool_calls'], no reasoning field
after ['reasoning_content', 'role', 'tool_calls'], no thinking_blocks

Live two-turn tool-calling exchange against deepseek/deepseek-v4-pro through the patched proxy
completed normally. Per-turn growth of the assistant history on the follow-up turn: 118 tokens
before, 146 after (+28, 1.24x)
— the restored reasoning, exactly. The magnitude scales with how
much the model reasons; these probe tasks were small, so this is a floor rather than a typical
figure.

Note on a measurement that looks contradictory: total prompt tokens for the whole replayed body
went down (1992 -> 1982). That is Patch 3's billing-header filter also being active in the
patched build, not Patch 10; the isolated figure is the assistant-turn delta above.

make lint passes. make test has 4 failures, all pre-existing: verified by re-running them at
pristine HEAD with these changes set aside, where 3 fail identically
(test_reap_stale_egg_images.py::TestReapScriptSafetyGuard x2,
test_git_client.py::test_worktrees_parent_detected) and the 4th
(test_error_paths.py::test_session_expires_exactly_at_boundary) passes, i.e. a timing flake.
None are in tests/config/. The 114 tests in tests/config/test_litellm_runtime_modules.py and
tests/config/test_patch_litellm_cache.py pass.

Fork PR

jwbron/litellm#11 implements the same change as ordinary source (llms/openrouter/reasoning.py
plus the transform_request call), with 27 tests in the fork's own layout; its full
tests/test_litellm/llms/openrouter/ suite passes (154 tests). The two implementations are
AST-identical modulo docstrings and comments, checked mechanically, since the patch literal here is
what actually runs in production and divergence would be a bug. This PR does not make egg's
image build from the fork. A separate upstream BerriAI/litellm PR is a follow-up.

…uter

On egg's primary route (Claude Code -> gateway -> LiteLLM /v1/messages ->
OpenRouter), assistant reasoning from prior turns was never sent back to
the model.

LiteLLM's Anthropic adapter converts incoming `thinking` content blocks
into `assistant_message["thinking_blocks"]`, but nothing on the OpenRouter
request path consumes that field: stock openrouter/chat/transformation.py
names reasoning only on the response side, and `reasoning_details` appears
nowhere under llms/ or litellm_core_utils/. The parent `transform_request`
puts `messages` straight into the body, so `thinking_blocks` reached
OpenRouter as a field no one reads and every historical assistant turn
arrived with no reasoning.

For a model whose chat template re-renders prior thinking that is a
malformed history, not a lost optimisation: Poolside Laguna renders
`'<think>' + message.reasoning|message.reasoning_content + '</think>'` per
previous assistant turn, so each one became a literal empty
`<think></think>`, which Poolside's model card warns degrades follow-up
behaviour.

The gap is upstream's and predates every egg and fork change; jwbron/litellm#8
touches only `get_supported_openai_params`, which acts on `optional_params`
and cannot reach a message field.

Patch 10 maps the blocks onto `reasoning_content` in
`OpenrouterConfig.transform_request` and removes `thinking_blocks`. The
logic lives in a fourth staged module (`openrouter_reasoning_roundtrip.py`
-> `llms/openrouter/_egg_reasoning_roundtrip.py`), following the same
convention as Modules 1-3 so it stays lintable and unit-testable here.
Multiple blocks concatenate in order; `redacted_thinking` carries no
plaintext and contributes nothing; a whitespace-only result emits no field
rather than re-creating the empty `<think></think>`; Anthropic `signature`
values are not forwarded; assistant messages only; fail soft, so a block
that cannot be parsed leaves its message untouched.

Note the direction: this is request-path (client -> provider). Patches 4,
5a and 5b are response-path (provider -> client). Adjacent, not the same.

Verified by applying the full script to a real stock litellm 1.86.2 install
(all 10 patches and 4 modules apply, and re-applying is a no-op) and
replaying a real Claude Code /v1/messages body through it with the provider
endpoint captured: before, the outgoing assistant message was
`['role', 'thinking_blocks', 'tool_calls']` with no reasoning field; after,
`['reasoning_content', 'role', 'tool_calls']`.

Mirrors jwbron/litellm#11.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — Patch 10 (assistant reasoning round-trip)

I verified the core mechanism against real stock litellm==1.86.2 rather than the fixtures, and it holds up:

  • The needle matches llms/openrouter/chat/transformation.py exactly once in stock v1.86.2, and the patched source parses (ast.parse). _supports_cache_control_in_content appears 3× and _move_cache_control_to_content 2× in that file, so the extended needle spanning the extra_body pop is genuinely necessary — the anchoring rationale is correct.
  • The messages rebind is consumed: super().transform_request(model, messages, ...) at transformation.py:166 uses it, and OpenAIGPTConfig._transform_messages (the only later message rewrite) touches user-role content lists only, so reasoning_content survives to the body. Nothing else under llms/ or litellm_core_utils/ strips it — strip_none_values_from_message is Mistral-only.
  • The async path reaches the patch: OpenAIGPTConfig.async_transform_request delegates to self.transform_request(...) when _is_base_class is False, which __init__ sets for OpenrouterConfig.
  • OpenRouter does document reasoning_content as a request-side alias for message.reasoning ("You can also use reasoning_content as an alias - it functions identically to reasoning"), so the target field is right.

The diagnosis and the anchoring are sound. One blocking defect in the new module, plus advisories.


Blocking

1. thinking_blocks: None is the common case, and it lands in the fail-soft branch — the field is never stripped and the exception path becomes the hot path

config/litellm/openrouter_reasoning_roundtrip.py:100-107

The module guards on key presence:

if message.get("role") != "assistant" or _SOURCE_FIELD not in message:
    out.append(message)
    continue
text = _extract_reasoning_text(message.get(_SOURCE_FIELD))

But the adapter you cite as the producer sets the key unconditionally, including when there is no reasoning. llms/anthropic/experimental_pass_through/adapters/transformation.py:652-658 (v1.86.2):

assistant_message = ChatCompletionAssistantMessage(
    role="assistant",
    content=assistant_content,
    thinking_blocks=(thinking_blocks if len(thinking_blocks) > 0 else None),
)

ChatCompletionAssistantMessage is a plain TypedDict (types/llms/openai.py:745, deriving from OpenAIChatCompletionAssistantMessage(TypedDict, total=False) at :724), so calling it is just dict(**kwargs) — the key is present with value None, not absent.

So _SOURCE_FIELD not in message is False, _extract_reasoning_text(None) raises TypeError, the broad except Exception swallows it, and the original message is appended.

Reproduced against the committed module:

$ python3 -c "...map_thinking_blocks_to_reasoning_content([{'role':'assistant','content':'hi','thinking_blocks':None}])"
no-thinking assistant -> {'role': 'assistant', 'content': 'hi', 'thinking_blocks': None}
thinking_blocks still present: True

Two consequences:

  • The documented contract is false in the dominant case. patch_litellm_cache.py:180-182 states "thinking_blocks is removed either way so no unknown field is transmitted", and the module docstring says "drop that field so no unknown key is transmitted". Every assistant turn that emitted no reasoning — the majority of turns on any route, and all turns on poolside/laguna-s-2.1, which your own precondition probe found returns no reasoning at all — still ships "thinking_blocks": null to OpenRouter. The PR body's before/after table only exercises a message that had blocks, which is why this wasn't caught.
  • The last-resort branch is the routine branch. except Exception is documented as "a request must survive a bad block" — a shape you did not anticipate. It now fires on essentially every assistant message in every request. That destroys the branch's diagnostic value: if anyone later adds a log line or counter there (as all three sibling modules do), it fires on ~100% of traffic and the genuinely-malformed case is unfindable in the noise.

Fix is one line — treat the adapter's own sentinel as "nothing to map" rather than "unparseable", so the key is stripped cleanly:

blocks = message.get(_SOURCE_FIELD)
if blocks is None:
    blocks = []          # the adapter's "no thinking" sentinel, not a bad shape
text = _extract_reasoning_text(blocks)

or equivalently handle None inside _extract_reasoning_text before the isinstance(blocks, list) check.

Test gap that points straight at this. test_malformed_blocks_never_raise parametrizes over "not-a-list", {"type": "thinking"}, 17, [None], ["bare string"], [{}] — every shape except bare None, the one value the production upstream actually emits. And test_assistant_without_thinking_blocks_is_returned_as_is asserts on {"role": "assistant", "content": "plain"} with the key absent, a shape the /v1/messages adapter never produces. Please add None to the parametrize list with the stripping assertion, not the untouched one.


Non-blocking

2. Signature discard + concatenation is unsafe for openrouter/anthropic/* and openrouter/google/* routes

The mapping is unconditional across every OpenRouter slug. For a Claude model routed through OpenRouter, the thinking signature is not decoration — it is what the Anthropic upstream verifies when prior thinking is replayed on a tool-calling turn, and OpenRouter's own docs place signatures inside reasoning_details, telling callers to "Pass back unmodified" and warning that "you cannot rearrange or modify the sequence of these blocks."

This patch does the opposite for those routes: drops signature, and flattens N ordered blocks into one string. That the shipped config/litellm-models.template.yaml only pins openrouter/qwen/qwen3-max today doesn't settle it — the template is operator-edited, and CacheControlSupportedModels in the very file being patched already carries CLAUDE and GEMINI, so Claude-via-OpenRouter is an anticipated route here.

I could not confirm a concrete failure (I have no Claude-via-OpenRouter route to probe), so this is advisory, not blocking. Suggest either gating the mapping to non-Anthropic slugs, or emitting reasoning_details (preserving signature and block boundaries) when any block carries a non-empty signature and falling back to the plain string otherwise. Worth at minimum a docstring note that Anthropic-via-OpenRouter is out of scope.

3. The module is completely silent, unlike all three of its siblings

Every other staged module treats silence as the bug and logs through verbose_logger with dedup — openrouter_capabilities._log / _log_fetch_failure (:88, :287), drop_params_visibility._log_warning (:39), and anthropic_thinking_policy (:82, which explicitly cross-references the convention). Patch 8 exists entirely because a silent drop was unfindable.

Module 4 has no logging at all, and the injected call site is except Exception: pass with nothing emitted. If _egg_reasoning_roundtrip ever fails to import, the feature is an invisible no-op with zero signal in the pod stream — and the only way to notice is a model quietly regressing. Build-time install is fail-loud so the risk is low, but a one-shot verbose_logger.warning on import failure would match the house style and costs nothing. This matters more once #1 is fixed, since the swallow branch will then genuinely mean "something is wrong."

4. Blocks concatenate with no separator

_extract_reasoning_text does "".join(parts). Two adjacent thinking blocks — which the adapter produces whenever Claude Code interleaves thinking with text/tool_use in one assistant turn — join as ...end of firstbeginning of second, running words together mid-sentence. The tests pass only because the fixtures carry explicit trailing spaces (_thinking("first ")). "\n" (or "\n\n") would be safer and is what a <think> re-render expects.

5. A pre-existing reasoning_content is silently overwritten, but only sometimes

If a message arrives with both thinking_blocks and reasoning_content — which litellm's own response objects carry (convert_dict_to_response.py:594-611, gpt_transformation.py:575-583), so any client echoing a litellm assistant message back will send both — the mapping clobbers reasoning_content when the blocks yield text, but preserves it when they yield only whitespace or redacted blocks. Two different rules for the same field. Usually the two agree in content so the impact is nil, but the asymmetry is unintentional; pick one and document it.

6. Stale docs — four places

  • docs/guides/per-agent-models.md:596 — "bakes in nine patches closing those gaps"; it's ten now.
  • docs/development/STRUCTURE.md:545-549 — the config/litellm/ tree lists each staged module with its patch number (openrouter_capabilities.py # Patch 7, drop_params_visibility.py # Patch 8, anthropic_thinking_policy.py # Patch 9). openrouter_reasoning_roundtrip.py is missing, and line 545's summary of patch_litellm_cache.py doesn't mention the round-trip.
  • tests/config/test_litellm_runtime_modules.py:1-5 — docstring still says "the three modules" and names only three; there are four.
  • Same file, test_module_imports_without_litellm (~:450) asserts on the three existing modules and wasn't extended. The new test_module_imports_without_litellm_installed is a textual grep for import litellm in the source, which is strictly weaker than the existing test's actual import under a purged sys.modules. Add the fourth module to the existing test rather than substituting the grep.

7. No runtime escape hatch

Patches 7 and 9 are both revertable without an image rebuild via commented-out env vars on k8s/base/litellm-deployment.yaml:102-122, and per-agent-models.md documents them as the knobs controlling reasoning on OpenRouter routes. Patch 10 changes the outgoing request body on every OpenRouter call and its correctness depends on provider-side behaviour verified against two models. Given the precedent for the reasoning-related patches specifically, a LITELLM_OPENROUTER_REASONING_ROUNDTRIP=0 guard would let an operator back it out of a live cluster. Judgment call — patches 1-6 and 8 have none either — but this one is closer to 7/9 in blast radius than to the cache patches.

8. Needle fixtures remain self-referential

_build_fixture_root constructs each fixture by concatenating the needles themselves, so test_each_needle_occurs_once_in_its_fixture and friends cannot detect a needle that doesn't match real upstream source. Pre-existing for all ten patches and mitigated by the fail-loud build, so not something to fix here — but it does mean the "confirmed to match real stock v1.86.2 exactly once" claim rests on a manual check with no automated guard. I re-ran that check by hand against the v1.86.2 tag and it holds (count 1, patched source parses); a checked-in trimmed excerpt of the real upstream region would make it durable.


Verdict: request changes on #1. It's a two-line fix, but the module ships a documented guarantee it does not honour in the majority case, and it routes ordinary traffic through the branch reserved for corruption. Everything else above is advisory.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

Review feedback on #3698.

The adapter sets thinking_blocks=None (key present, TypedDict) on every
assistant turn that produced no reasoning, so the key-presence guard sent
that value into _extract_reasoning_text, which raised, which routed the
dominant message shape through the fail-soft branch: "thinking_blocks":
null still reached OpenRouter, and the branch reserved for corruption
became the hot path. It is now read as the sentinel it is.

Also: decline anthropic/* and google/* slugs, whose upstream re-verifies
the block signature this plain-string form cannot carry; join blocks on a
newline rather than running them together; never overwrite a
reasoning_content the caller already set; log an unmappable shape and an
unavailable module once each; add LITELLM_OPENROUTER_REASONING_ROUNDTRIP=0
as a runtime escape hatch, as for Patches 7 and 9; refresh the four stale
doc/test references to the patch and module counts.
@james-in-a-box

Copy link
Copy Markdown
Contributor

Review response — all 8 items

Thanks for verifying the mechanism against real stock 1.86.2 rather than the fixtures; #1 is exactly right and was the kind of defect the fixtures could never have shown.

Pushed as 97feaff.


1. thinking_blocks: None lands in the fail-soft branch — fixed-in-PR (commit 97feaff)

Confirmed and fixed. None is now read as the adapter's own "this turn produced no thinking" sentinel, so the key is stripped cleanly and the message never reaches the except. Your diagnosis of the second consequence is the one that made me restructure rather than one-line it: the last-resort branch now means something, so it also logs (see #3).

The module docstring now states the sentinel as the dominant input rather than an edge case, and two tests pin it:

  • test_adapter_none_sentinel_is_stripped_not_treated_as_malformed — asserts the stripping, as you asked.
  • test_none_sentinel_does_not_fire_the_fail_soft_diagnostic — asserts the branch stays cold, which is the property that decays silently.

I put these beside the parametrize rather than inside it: None is now the one value in that list with a different expected outcome (stripped, not passed through), so folding it in would have weakened the shared assertion for the other seven. test_assistant_without_thinking_blocks_is_returned_as_is keeps the key-absent case and now says in its docstring that it is not the shape the adapter emits.

2. Signature discard on openrouter/anthropic/* and openrouter/google/*fixed-in-PR (commit 97feaff)

Took the first of your two suggestions. map_thinking_blocks_to_reasoning_content now takes model (the patch call site passes it) and declines any slug matching anthropic/claude/google/gemini, leaving the message byte-identical to stock — a known-working state, since stock has never mapped this field. Emitting a real reasoning_details for those routes needs a wire shape I have no route to verify against, and half-serving them is worse than not serving them.

Substring rather than prefix, because litellm may or may not have stripped its own openrouter/ prefix by the time transform_request runs; a non-string model is treated as ordinary. Tested both directions — the four slug spellings pass through untouched, and deepseek/qwen/laguna still map.

3. Module is silent, unlike all three siblings — fixed-in-PR (commit 97feaff)

Added _log / _warn_once in the house style (dedupe key recorded only after a successful emit, same reason as openrouter_capabilities._warn_env_once). Two sites: the fail-soft branch, which now genuinely means "unrecognised shape" once #1 is fixed, and the injected call site, which warns once via a verbose_logger sentinel attribute if _egg_reasoning_roundtrip cannot be imported. Verified end to end against the real staged module with the destination package removed: exactly one warning across repeated calls.

4. Blocks concatenate with no separator — fixed-in-PR (commit 97feaff)

"\n" now. Blank blocks are skipped so they cannot contribute a bare separator. You were right that the fixtures were carrying the fix: they had explicit trailing spaces, which are now gone, and test_blocks_are_separated_rather_than_run_together asserts on the run-together string directly.

5. Pre-existing reasoning_content overwritten only sometimes — fixed-in-PR (commit 97feaff)

Picked "the caller's own field wins" and documented it: a non-empty reasoning_content already on the message is never overwritten, whatever the blocks yield. That is the one rule for both branches, and it is the less destructive of the two. Blank ("" / whitespace) does not count as set.

6. Stale docs, four places — fixed-in-PR (commit 97feaff)

All four. per-agent-models.md nine→ten, plus a Patch 10 bullet in the reasoning-depth callout (now "five env vars", three patches); STRUCTURE.md gains the module line and mentions the round-trip in the patch_litellm_cache.py summary; the test docstring says four and names the fourth. On the last point specifically: added the module to the existing test_module_imports_without_litellm (real import under a purged sys.modules) and deleted the grep test rather than keeping the weaker duplicate.

7. No runtime escape hatch — fixed-in-PR (commit 97feaff)

Agreed on the blast-radius argument — this one changes the outgoing body on every OpenRouter call, which is why it belongs with 7/9 rather than with the cache patches. LITELLM_OPENROUTER_REASONING_ROUNDTRIP=0 added, defaulting on, with the same near-miss discipline as anthropic_thinking_policy (an unrecognised spelling warns once and takes the default, because off is also the default and an operator otherwise cannot tell "ignored" from "working"). Commented out on k8s/base/litellm-deployment.yaml beside the other two, and documented.

8. Needle fixtures remain self-referential — disagree (correct observation, but scoped out of this PR by your own read, and the durable version is a different change)

Not disputing the substance — the fixtures genuinely cannot detect a needle that does not match upstream, and you are right that the "matches stock exactly once" claim rests on a manual check. But it is pre-existing for all ten patches, you scoped it out yourself, and the fix you describe (a checked-in trimmed excerpt of real upstream source) is a vendoring decision that applies to the whole patch set rather than to Patch 10. Doing it here would put a partial version of it in the one place it is least needed. Leaving it as-is, with the fail-loud build as the mitigation it already is.


make lint — ruff check, ruff format, shellcheck, and all custom checks pass. mypy reports 3 errors in shared/egg_agent/client.py, which this PR does not touch (pre-existing on the base). Did not run make test; tests/config/ is 337 passing, up from 140.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Patch 10, delta 1b36232..97feaff

The blocking item is genuinely fixed, not papered over. Verified against real stock litellm==1.86.2 source rather than the repo's fixtures.

#1thinking_blocks: None sentinel. Confirmed the production shape at llms/anthropic/experimental_pass_through/adapters/transformation.py:653-663 in v1.86.2: ChatCompletionAssistantMessage(..., thinking_blocks=(thinking_blocks if len(thinking_blocks) > 0 else None)), with the re-assignment on :662 guarded by len(thinking_blocks) > 0 — so the key is present-and-None on every assistant turn that produced no reasoning, exactly as diagnosed. openrouter_reasoning_roundtrip.py:258-266 now reads it as the sentinel, and the message flows through the normal path:

sentinel -> [{'role': 'assistant', 'content': 'hi'}]      # thinking_blocks stripped, no field emitted

Both consequences are closed: "thinking_blocks": null no longer reaches OpenRouter on the dominant message shape, and the except Exception branch stays cold — test_none_sentinel_does_not_fire_the_fail_soft_diagnostic pins the second property, which is the one that would decay silently.

Patch application still holds after the replacement changed. Applied the current PATCHES entry to the real v1.86.2 llms/openrouter/chat/transformation.py: needle count 1, patched source ast.parses, the injected call lands inside transform_request (where model is a parameter at :150, so _egg_map_reasoning(messages, model) resolves), and _egg_map_reasoning appears nowhere after def transform_response.

The injected import-failure path works, not just its string literals. Executed the injected block against a fake litellm.llms.openrouter with the module absent: 4 calls, exactly 1 verbose_logger.warning, _egg_reasoning_roundtrip_warned set, messages returned unmodified. The nested try cannot break a request.

Module behaviour across the rest of the delta, exercised directly: blocks join on \n; an anthropic/google slug returns the message byte-identical; LITELLM_OPENROUTER_REASONING_ROUNDTRIP=0 returns stock; a caller-set reasoning_content survives while the blank one does not block the mapping; input is not mutated.

Two things I checked because the delta could have silently no-op'd the feature:

  • The adapter never sets reasoning_content on the assistant messages it builds (only role/content/tool_calls/thinking_blocks), so the new "caller's field wins" rule cannot short-circuit the mapping on egg's own /v1/messages path.
  • No slug in config/litellm-models.template.yaml (openrouter/qwen/qwen3-max) matches _SIGNATURE_VERIFYING_MARKERS, so the new decline gate does not disable the patch on a shipped route.

Removed-behaviour audit. Dropping test_module_imports_without_litellm_installed was required, not a loss — the module now carries a deferred from litellm._logging import ... inside _log, which that grep would have failed on. The replacement is strictly stronger: openrouter_reasoning_roundtrip was added to test_module_imports_without_litellm (:459), a real import under a purged sys.modules.

tests/config/test_litellm_runtime_modules.py + tests/config/test_patch_litellm_cache.py: 141 passed. Docs delta is consistent — three patches, five env vars, matching the three commented blocks on k8s/base/litellm-deployment.yaml.

No blocking issues in the delta.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor

egg review completed. View run logs

3 previous review(s) hidden.

@jwbron

jwbron commented Jul 29, 2026

Copy link
Copy Markdown
Owner Author

Merge order with #3699: land this one first

#3699 (the 1.94.0 bump) is branched from #3695, not from this PR, so Patch 10 is not in its tree.
The two are siblings editing the same files and whichever lands second conflicts.

Proposed order: this PR, then #3695, then #3699. Reasons:

The conflict is wider than the patch script

Seven files are touched by both:

config/litellm/Dockerfile
config/litellm/patch_litellm_cache.py
docs/development/STRUCTURE.md
docs/guides/per-agent-models.md
k8s/base/litellm-deployment.yaml
tests/config/test_litellm_runtime_modules.py
tests/config/test_patch_litellm_cache.py

A three-way merge of patch_litellm_cache.py alone produces 10 conflict hunks, including the whole
PATCHES list (8 entries vs 10).

What #3699 has to redo, and what is already verified

Re-verified against the actual merged tree rather than a stock tag (see #3699 for the same check
from the other side):

  • Patch 10's needle matches exactly once in stock 1.94.0, and still exactly once after
    Bump litellm 1.86.2 -> 1.94.0, retire four absorbed patches (#3697) #3699's eight patches have been applied
    . None of them disturbs the cache_control plus
    extra_body sequence the needle spans.
  • The merged set (9 patches, 5 modules) applies cleanly against a real litellm==1.94.0 install,
    is idempotent on a second pass, and the patched file parses.
  • Functionally on 1.94.0: an assistant message carrying thinking_blocks comes out as
    ['reasoning_content', 'role', 'tool_calls'] with thinking_blocks removed, and the
    anthropic/* route is still declined.
  • The premise still holds on 1.94.0: the Anthropic adapter still parks reasoning on
    assistant_message["thinking_blocks"], and OpenRouter's request path still has no consumer for
    it.

So what is left for #3699 is bookkeeping, not investigation:

  1. Renumber to Patch 9/9 and Module 5/5 in the patch list, the script docstring, and the
    Dockerfile's staged-file list and counts (8 patches to 9, 4 modules to 5).
  2. Extend the lookup-by-descriptive-tail fix to this patch's tests. Four sites in
    tests/config/test_patch_litellm_cache.py still match on startswith("Patch 10/"), which is
    exactly the positional lookup Bump litellm 1.86.2 -> 1.94.0, retire four absorbed patches (#3697) #3699 removes elsewhere; they become
    _patch_by_description("assistant reasoning round-trip").

@jwbron
jwbron merged commit 664d225 into main Jul 29, 2026
23 checks passed
james-in-a-box Bot pushed a commit that referenced this pull request Jul 29, 2026
…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.
james-in-a-box Bot pushed a commit that referenced this pull request Jul 29, 2026
…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.
@jwbron

jwbron commented Jul 29, 2026

Copy link
Copy Markdown
Owner Author

Post-merge record: what this patch does, what it cannot do alone, and one sentence above that is now too strong

Landing this as a comment because the body is history now. It also contains one claim that later
measurement weakened, corrected at the bottom.

The control, quoted rather than summarised

Three arms, identical history, tools present in every arm, max_tokens 800, n=4. The only
difference is the assistant message in the replayed history. Pre-patch it is:

{"role": "assistant", "content": null,
 "tool_calls": [{"id": "call_1", "type": "function",
                 "function": {"name": "run_shell", "arguments": "{\"command\": \"ls /etc | wc -l\"}"}}]}

The post-patch arm adds exactly one key, which is what this patch produces:

 "reasoning_content": "The user wants a count of files in /etc. I should run `ls /etc | wc -l` rather than listing everything, since only the count was asked for. After I get the number I will report it directly without re-running anything."

The control arm adds that same string under content instead of reasoning_content. Same text,
same token count, different field.

arm prior assistant message carries turns that reasoned median reasoning_tokens
pre-patch nothing 0/4 0
post-patch reasoning_content 4/4 105
control same text as plain content 0/4 0

The control is the load-bearing cell: it is the field, not the extra context. The same result holds
when the next turn also emits a tool call (0/3 against 3/3, both arms calling the tool in every
draw). Full message sequences are in the reproduction record on #3595.

The field is honoured on the request path

Verified separately from the cells above: reasoning_content primed 3/3 (398, 448, 82 tokens),
reasoning primed 3/3 (183, 275, 232), and neither field primed 0/2. So this patch targets a field
this route actually honours when it arrives on a request. Worth recording explicitly, because an
earlier observation that "this route's field is message.reasoning" was response-path only and
does not bear on what the request path accepts. A merged patch quietly writing to a dead field is
the kind of thing found six months later.

It still cannot fire on this traffic without a seed

Four-turn sessions, two draws each:

arm opener replay reasoning_tokens per turn
A errand, tools no [0,0,0,0] [0,0,0,0]
B errand, tools yes [0,0,0,0] [0,0,0,0]
C tool-free yes [1601,746,309,70] [1448,490,368,237]
D tool-free no [1198,0,0,0] [1720,0,0,0]

Arm B is this patch with nothing to replay. So the original "inert on this route" claim was right
about the outcome and wrong about the cause
: not that the model does not reason, but that nothing
ever primes it to.

The seed, and its limit, together

The seed is cheap: an opening turn with tool_choice: "none" and a reasoning-shaped prompt reasons
4/4, against 0/4 for the same errand prompt with callable tools. It is a per-request parameter,
not a change to how the tools array is assembled.

Do not take that half without this one. Replay-forward has since been measured at 3 of 6
per-turn continuation, with chains dropping out by turn 3
, and re-seed revival is unproven,
because a control chain revived unaided and so nothing attributes the recovery to the re-seed.
Budget two or three reasoning turns, not a sustained channel.

Correction to the body above

The body says: "this PR is the half that keeps the seed alive once it exists." That is too strong.
It extends a seed by a couple of turns; it does not keep it alive. The four-turn arm C figures above
were the basis for that sentence, and their monotone decay (1601, 746, 309, 70) was already pointing
at what the 3-of-6 measurement later confirmed.

Re-verified against current main, post-merge

Everything in the body was verified against pre-merge trees, so none of it transferred. Re-run
against main at b4f0c5c5f, which pins v1.94.0 and carries 10 patches and 5 modules, with this
one landing as Patch 7/10 rather than the 9/9 predicted:

  • needle matches exactly once in stock 1.94.0, is consumed by the patch, and the idempotency
    marker appears exactly once afterwards
  • all 10 patches and 5 modules apply, second pass is a clean no-op, patched file parses
  • functionally, an assistant message carrying thinking_blocks goes out as
    ['reasoning_content', 'role', 'tool_calls'] on poolside/laguna-s-2.1, and anthropic/* is
    still declined with thinking_blocks left intact

The renumber from 10 to 7 is also the case the positional test lookups would have failed silently;
the four tests here use _patch_by_description("assistant reasoning round-trip") and survived it.

Not claimed

Nothing here rests on the local-window account of the livelock, which is unsupported, nor on the
synthetic period sweep, which is retracted (its stimulus instructed the model to copy, so it
measured obedience). No synthetic reproduction of the production livelock exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant