litellm: read OpenRouter capabilities live, and make drop_params visible - #3625
Conversation
…el map Closes #3624. OpenrouterConfig.get_supported_openai_params gates reasoning_effort and thinking on litellm.supports_reasoning, which reads the bundled model-cost map. For OpenRouter that map is wrong by construction: OpenRouter ships new slugs continuously and the map lags, so a current model answers False. The gate is a bare if, so it fails closed, and drop_params: true then discards the parameter with no exception and no log line. Every OpenRouter slug egg routes is absent from the 1.86.2 map (kimi-k3, glm-5.2, laguna-s-2.1, deepseek-v4-pro/flash, kimi-k2.7-code), so any reasoning knob set on them never reached the wire. Patch 7 plus a new installed module (openrouter_capabilities.py) reads OpenRouter's unauthenticated /api/v1/models and unions the answer with the existing map result. The union is deliberate: supported_parameters under-reports reasoning_effort (deepseek-r1 is flagged supports_reasoning in the map and is plainly a reasoning model, yet OpenRouter advertises only reasoning for it, treating the OpenAI spelling as an alias), so reading its absence as a denial would drop a working param. The lookup can admit a knob the map does not know but never withholds one the map allows. Fails soft throughout: any error, timeout, non-200 or malformed payload yields no opinion and the stock path runs. A failed fetch is cached for the TTL so an offline deployment costs one attempt per hour rather than one per request, and a lock prevents a stampede. LITELLM_OPENROUTER_CAPABILITY_FETCH=0 restores previous behaviour exactly. Verified in the built image against the live endpoint: slug stock patched fetch-disabled moonshotai/kimi-k3 False True False z-ai/glm-5.2 False True False poolside/laguna-s-2.1 False False False (advertises reasoning, moonshotai/kimi-k2.7-code False False False not reasoning_effort) deepseek/deepseek-r1 True True True (union keeps the map) Mirrors jwbron/litellm#8. NOTE: the operator overlay must not set reasoning_effort when this ships. Measured against OpenRouter, the knob caps reasoning below the model default rather than raising it (kimi-k3: 3130 mean reasoning tokens with no param vs 340 at high, non-overlapping); it has been removed from the overlay accordingly. New module install mechanism (NEW_MODULES) with fail-loud source resolution, plus 4 tests covering install, idempotency, missing-source, and that patch 7 stays additive rather than short-circuiting the stock branch.
Stock 1.86.2 pops unsupported params in a bare loop with no logging, so a param set in a proxy config that never reaches the provider is a real behavioural difference with no signal attached. That is the condition that made patch 7's bug take a full investigation to find rather than a log grep. Patch 7 removes the OpenRouter false-negative. This covers the rest, including drops that are CORRECT and still worth announcing: poolside/laguna-s-2.1 genuinely does not accept reasoning_effort, so the knob is dropped on purpose and the operator otherwise has no way to learn why their config line does nothing. The remedy in the message names the proxy-config form (allowed_openai_params in a model_list entry's litellm_params) rather than only the per-request kwarg, because the affected audience reads config.yaml. Verified that route works: LiteLLM_Params is ConfigDict(extra="allow") and the value reaches get_optional_params from there. NEEDLE NOTE: 1.86.2 has two drop_params branches in utils.py sharing an identical condition; the embeddings path is followed by a bare pass, this one by the pop loop. The needle includes the loop, and a test asserts the sibling site is left byte-for-byte untouched (the Patch 4 trap, one file over). Deduped per (provider, model, param-set) and bounded at 1000 entries, past which it stops recording rather than stops warning. Wrapped so a diagnostic can never break a request. Verified in the built image: laguna-s-2.1 + reasoning_effort warns once, names the param and the config remedy, stays silent on repeat calls, and glm-5.2 does not warn at all now that patch 7 admits the knob there. Mirrors jwbron/litellm#7 (merged into the fork the host proxy runs; the cluster image pins stock 1.86.2, which predates it).
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: egg-litellm patches 7 & 8 (OpenRouter live capabilities + drop_params visibility)
I verified the patch mechanics against the real litellm==1.86.2 sources (fetched from raw.githubusercontent.com/BerriAI/litellm/v1.86.2/…, not the fixtures): all three touched needles (Patch 1, 7, 8) occur exactly once in the genuine files, and applying them via importlib-loaded _apply produces source that passes ast.parse. The Patch 8 needle-disambiguation claim is correct — 1.86.2 really does have two if litellm.drop_params is True or (...) sites (utils.py:3303, a bare pass in the embeddings path, and utils.py:4079, the pop loop inside _check_valid_arg), and including the loop line is what selects the right one. _install_module's missing-destination-directory check is a genuine drift guard. The engineering here is careful and the diagnosis in the Patch 7 comment block is accurate.
The problem is what happens after the drop stops. Patch 7 opens a path that was closed, and on egg's primary route the value that flows through it is not the one the config author chose.
I also actively refuted four candidate findings so they don't waste your time: duplicate supported_params entries (stock gate ends return list(dict.fromkeys(supported_params)) — deduped), an HTTPHandler socket leak (__del__ closes the client), event-loop blocking from the sync fetch (acompletion runs completion via loop.run_in_executor(None, …), so it's a threadpool worker, not the loop), and reasoning_effort arriving as a dict (is_reasoning_auto_summary_enabled() defaults False).
Blocking
1. Patch 7 silently collapses reasoning depth on /v1/messages — egg's primary route
Confirmed. config/litellm/patch_litellm_cache.py:640-654.
The Patch 7 comment frames the pre-PR state as a loss: "any reasoning knob set on them never reached the wire" (patch_litellm_cache.py:604). On the /chat/completions route with an explicit config value, that's right. On the /v1/messages route that Claude Code actually uses, reasoning_effort is not something an operator sets — litellm synthesizes it per-request, and unblocking it replaces a deep default with a shallow instruction.
Trace, end to end:
-
Claude Code (sandbox) → egg-gateway
/v1/messages→ litellm (gateway/upstream_registry.py:66). Anthropic-shaped body carryingthinking: {"type": "enabled", "budget_tokens": N}. -
llms/anthropic/experimental_pass_through/adapters/transformation.py→_translate_thinking_to_openai.is_anthropic_claude_model("moonshotai/kimi-k3")is False (it's a substring test for"anthropic"/"claude"), so the branch that would preservethinkingis skipped and it sets:new_kwargs["reasoning_effort"] = self.translate_anthropic_thinking_to_reasoning_effort(...)
Bucketed by budget:
≥10000 → "high",≥5000 → "medium",≥2000 → "low"."thinking"is intranslatable_anthropic_params(), so the raw field is never copied through separately. -
Pre-PR:
supports_reasoning("moonshotai/kimi-k3")is False (your own comment,patch_litellm_cache.py:602-604: every slug egg routes is absent from the 1.86.2 map), soreasoning_effortis not insupported_openai_params,_check_valid_argpops it, and OpenRouter receives no reasoning param → the model runs at its own default depth. -
Post-PR: OpenRouter's
/api/v1/modelsadvertisesreasoning_effortformoonshotai/kimi-k3,z-ai/glm-5.2, anddeepseek/deepseek-v4-pro(I queried it live; it matches your tables). Patch 7 appends it,_check_valid_argkeeps it,gpt_transformation._map_openai_paramscopies any param insupported_openai_paramsverbatim, and OpenRouter receivesreasoning_effort: "high".
Per your own measurements in this PR, that is a large regression, not a fix: kimi-k3 mean reasoning tokens 3130 with no param → 340 with a flat reasoning_effort: high, with non-overlapping distributions. The PR ships a change whose measured effect on the deployed route is ~9x less reasoning.
The PR description's safety argument — that reasoning_effort has been removed from every row of the deployed litellm-models.yaml, so the image is safe to roll — does not cover this route. It reasons about config-supplied params. The param on this path is adapter-supplied, per request, and is unaffected by anything in litellm-models.yaml.
This is already documented in this repo, on main, in a file you own — config/litellm/cost_callback.py:818-819:
LiteLLM rewrites
thinkinginto areasoning_effortbucket, so the effective effort tracks the per-turn thinking budget.
Failure scenario: roll this image; every agent turn from Claude Code through the gateway to any of kimi-k3 / glm-5.2 / deepseek-v4-pro carries reasoning_effort: "high" synthesized from the thinking budget; per your own table those models drop from ~3130 to ~340 reasoning tokens per turn. Nothing logs it (Patch 8 only fires on drops, and this param is no longer dropped), no config file mentions it, and the symptom is degraded agent output — the hardest possible thing to attribute back to a proxy image bump.
Fix — pick one, but don't roll without one:
- Add a Patch 9 that makes
_translate_thinking_to_openaia no-op for non-Claude models, sothinkingstays out of the OpenAI body and only explicitly configuredreasoning_effortreaches the wire. This preserves exactly the property Patch 7 is trying to restore (config knobs work) without letting the adapter's bucket become the effective setting. - Or ship Patch 7 with
LITELLM_OPENROUTER_CAPABILITY_FETCH=0set ink8s/base/litellm-deployment.yaml, and flip it on only after measuring the/v1/messagespath specifically. The current tables measure the/chat/completionspath. - Or, if flat-high genuinely is the intent for these models, say so explicitly in the PR body and in
config/litellm-models.template.yaml, because right now the change reads as "restore a dropped knob" while its deployed effect is "impose a new one."
Whichever you choose, the PR description needs the /v1/messages interaction spelled out — a future reader will otherwise repeat the config-only reasoning.
2. _env_float silently discards deliberate operator input
Confirmed. config/litellm/openrouter_capabilities.py:64-72.
try:
value = float(raw)
except ValueError:
return default
return value if value > 0 else defaultBoth fallbacks are silent. LITELLM_OPENROUTER_CAPABILITY_TTL=0 — the obvious spelling for "never cache, always re-fetch" — becomes 3600 seconds with no signal. LITELLM_OPENROUTER_CAPABILITY_TIMEOUT=5s (or 5 with a stray character, or a value pasted from a Helm chart as "5.0s") becomes 5.0 with no signal. An operator debugging stale capability data sets the TTL, restarts, observes no change, and has no way to learn the proxy ignored them.
These are the only two tuning knobs the module exposes, and one of them (FETCH) is the kill switch for finding 1 — so an operator reaching for these env vars is very likely someone already in an incident.
Fix: verbose_logger is already imported at line 37. One line in each branch:
except ValueError:
verbose_logger.warning(
"openrouter capabilities: %s=%r is not a number; using %s", name, raw, default
)
return default
if value <= 0:
verbose_logger.warning(
"openrouter capabilities: %s=%r must be > 0; using %s", name, raw, default
)
return default
return valueAlso worth reconsidering whether TTL=0 should mean "disable caching" rather than being rejected — it's the natural reading and currently the least discoverable failure of the three.
3. Patch 8's remedy text is wrong in the case it will fire most often
Confirmed. config/litellm/drop_params_visibility.py (the verbose_logger.warning body).
The message tells the operator:
To send them anyway, add
allowed_openai_params: ['reasoning_effort']to that model's litellm_params in config.yaml.
On this deployment, the param most likely to be dropped is reasoning_effort — and per finding 1 it is not in any config file. It was synthesized by litellm's own Anthropic adapter from the caller's thinking block. An operator following this advice searches litellm-models.yaml for reasoning_effort, finds nothing (the PR description confirms it was removed from every row), and is left more confused than before the warning existed.
Worse, for a model like qwen/qwen3-max — which advertises no reasoning parameters at all, so the drop is correct — following the advice forces an unsupported param onto the wire and converts a correct silent drop into a provider-side error or an ignored field. The message asserts the drop is a problem; frequently it is the system working.
This matters because the entire premise of Patch 8 is that operators deserve accurate signal. A confidently-worded, wrong remediation is worse than the silence it replaces.
Fix: state what is known and stop short of prescribing:
litellm.drop_params: dropped %s for model=%s provider=%s — the provider does not
advertise support, so these did not reach it. If they came from this model's
litellm_params in config.yaml, remove them or override with
`allowed_openai_params: %s`. If they were synthesized from the request (e.g.
reasoning_effort derived from an Anthropic `thinking` block), the drop is expected.
Non-blocking
"reasoning" in the advertised set appends "thinking" — patch_litellm_cache.py:652-653. OpenRouter's reasoning is its own request field ({"effort": …} / {"max_tokens": …}); Anthropic's thinking is {"type", "budget_tokens"}. Different wire shapes, and the mapping is asserted by name similarity. It's inert on /v1/messages (the adapter strips thinking before this point), but live on /chat/completions, where it would let a raw Anthropic-shaped thinking dict through to a non-Anthropic provider. Your own Patch 2 notes in this file are careful about exactly this class of spelling conflation. Either drop the thinking append or add a shape translation.
~277 lines of new module logic with zero unit tests, in a PR whose test additions are all about the patch script. openrouter_capabilities.py has the highest branch density in the diff — TTL expiry, double-checked locking, negative caching, three slug-candidate spellings, four malformed-payload guards — and none of it is exercised. It is also made untestable by from litellm._logging import verbose_logger at module scope (line 37), since litellm isn't installable in this repo's env; a deferred import inside the functions that log, matching the pattern already used at line 84 for HTTPHandler, would make the whole module importable and testable. reset_cache() at line 198 is documented "Intended for tests" and no test calls it. The Dockerfile comment (config/litellm/Dockerfile:25-26) says keeping these as real files "means it stays lintable and testable in the egg repo" — the module-scope import is what prevents that.
test_missing_file_fails_loud no longer tests what it says. tests/config/test_patch_litellm_cache.py:105-109 passes an empty root and asserts SystemExit. It still passes — but _patch_root now runs NEW_MODULES first (patch_litellm_cache.py:762-765), so it aborts in _install_module's "destination package missing" branch and never reaches _apply. _apply's missing-file guard is now uncovered while the test name claims otherwise. Give it a root containing the llms/openrouter/ and top-level dirs so the module install succeeds and _apply is actually reached.
test_patch7_gate_is_additive_not_substitutive is close to vacuous. Same file, lines 195-214. Every assertion is a substring check against the replacement string; the patch is never applied and no behaviour is observed. assert "return" not in replacement is the fragile one — it passes today only by luck, and would break on any comment containing the word "returns". It also takes an unused tmp_path fixture. Given the test docstring claims to protect a real semantic invariant (union, not replace), consider applying the patch to a fixture and asserting the resulting source still contains the stock supports_reasoning branch after the inserted block.
_install_module silently overwrites a pre-existing stock file — patch_litellm_cache.py:741-759. Every other operation in this script is fail-loud on drift; this one content-compares and, on mismatch, overwrites without comment. litellm/llms/openrouter/ in 1.86.2 contains ['chat', 'common_utils.py', 'embedding', 'image_edit', 'image_generation', 'responses'], so there's no collision today — but the destination name is capabilities.py, a name upstream might plausibly take, and the failure mode would be a silently clobbered upstream module. The sibling module uses the _egg_ prefix (_egg_drop_params_visibility.py); using _egg_capabilities.py for both consistency and collision-avoidance costs nothing. At minimum, raise SystemExit when the destination exists with different content and lacks an egg marker.
Lock-held blocking HTTP on the request path. openrouter_capabilities.py:135-145 holds _LOCK across _fetch(). On first request after startup or TTL expiry, every concurrent worker thread blocks behind one HTTP call. The 5s httpx timeout is per-phase (connect, read, write, pool), not total, so a pathological connection can exceed 5s wall-clock. Bounded by the negative cache (one attempt per TTL) and it's a threadpool worker not the event loop — but the tail latency lands on real requests once an hour. Consider serving the stale cache while a background refresh runs, or a total-deadline httpx.Timeout.
Fetch failures are logged at debug. openrouter_capabilities.py:90 and :98. Default litellm log level is INFO, so a permanently unreachable endpoint — the exact case where behaviour silently reverts to the model-map — produces no visible line. This is the same failure mode Patch 8 exists to eliminate, one file over. First failure at warning, subsequent at debug, would fix it without noise.
New unauthenticated outbound trust input. openrouter.ai/api/v1/models now influences which params reach the provider. Blast radius is genuinely small (a set of parameter names, and _map_openai_params only copies params litellm already knows), TLS is verified by default, and there's no egress NetworkPolicy on egg-system to work around (k8s/base/network-policies.yaml scopes all six policies to egg-agents). Noting for the record, not asking for a change: follow_redirects=True is set by HTTPHandler, so the effective host is not pinned.
_MAX_WARNINGS doesn't cap warnings. drop_params_visibility.py:_SEEN stops growing at 1000 keys, but the verbose_logger.warning call sits outside that guard — so past 1000 distinct keys, every subsequent request re-warns unboundedly. Move the warning inside the cap, or _SEEN.clear() on overflow.
Stale Dockerfile header. config/litellm/Dockerfile:7 still says "closes the five gaps at build time" — it's eight now (the script's own docstring was correctly updated 6→8). Line 24 also says "this is a whole module" (singular) directly above two COPY lines.
config/litellm-models.template.yaml's reasoning comment is contradicted by this PR's data. The commented-out extra_body.reasoning.effort: "high" is labelled "Closest knob to Opus xhigh — uncomment to opt into the deepest reasoning." Your measurements show flat high produces substantially less reasoning than no param on these models. Someone will uncomment that line on the strength of the comment. Worth updating in the same PR that produced the counter-evidence.
No operator-facing docs for the three new env vars. LITELLM_OPENROUTER_CAPABILITY_FETCH / _TTL / _TIMEOUT exist only in the module docstring. FETCH=0 is the kill switch for finding 1; it should be discoverable from docs/guides/per-agent-models.md (which already discusses this patch script at line 597) and ideally present-but-commented in k8s/base/litellm-deployment.yaml.
Minor: _candidate_slugs is annotated -> list rather than -> list[str] (openrouter_capabilities.py:148), and get_supported_parameters returns the mutable cached set directly (line 194) — a caller doing params.add(...) would poison the process-wide cache. Neither is reachable from current call sites.
Findings 1-3 are all instances of the same theme this PR is otherwise fighting: a value that changes behaviour without any operator-visible signal. Finding 1 is the one that needs resolving before this rolls — the rest are fixable in follow-up if you'd rather land the mechanics first.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Patch 7 unblocked reasoning_effort on OpenRouter routes, which is right for a value an operator configured and wrong for the one litellm's Anthropic adapter synthesizes per request from the caller's thinking budget on /v1/messages. That synthesized bucket is a cap BELOW the model default (kimi-k3: 3130 reasoning tokens with no param vs 340 with "high"), so patch 7 alone would have cut reasoning ~9x on egg's primary route with nothing logged and no config file to point at. Patch 9 gates the synthesis, off by default, so only a configured knob reaches the wire. LITELLM_ANTHROPIC_THINKING_TO_REASONING_EFFORT=1 restores stock behaviour. Also from review: - patch 7 no longer maps OpenRouter's reasoning onto Anthropic's thinking — different wire shapes, not spellings of each other - drop_params warning states what is known instead of prescribing a config edit for a param litellm synthesized itself - _env_float warns instead of silently discarding operator input; TTL=0 now means never-cache rather than being rejected - capability fetch serves stale rather than queueing behind the lock, and warns on first failure instead of debug-only - staged modules defer their litellm imports, which is what makes them importable in-repo; 33 new unit tests cover all three - module destinations carry the _egg_ prefix and refuse to clobber a foreign file - test_missing_file_fails_loud and the patch-7 additivity test now exercise what their names claim - operator docs for the four env vars, and a measure-first warning on the template's extra_body.reasoning.effort comment
|
Thanks — finding 1 is a real catch and the right one to block on. I had reasoned about Everything below is in 5979951. Blocking1. Patch 7 collapses reasoning depth on 2. 3. Patch 8's remedy text is wrong in its most common case — Non-blocking
~277 lines of new module logic with zero tests, made untestable by a module-scope litellm import —
Lock-held blocking HTTP on the request path — Fetch failures logged at New unauthenticated outbound trust input —
Stale Dockerfile header —
No operator-facing docs for the new env vars — Minor: PR descriptionYou asked for the — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review — delta f70123b..5979951 (1 commit, +980/−81 across 10 files)
I re-reviewed only the delta and re-verified the three blocking findings from the previous round. All three are genuinely fixed, not papered over, and every non-blocking item from the last review was addressed. Verification I ran against a real litellm==1.86.2 tree (read-only scratch, nothing mutated in the worktree):
- All 10 patch needles occur exactly once in genuine 1.86.2; all
presentidempotency markers are absent from stock. _patch_rootapplied cleanly to a real copy, was a no-op on second run, and all four touched filesast.parseclean afterwards.- Inspected the patched output byte-for-byte: Patch 9's gate sits between the untouched
is_anthropic_claude_modelbranch and the synthesis, so the Claude path is unchanged. Patch 7's added block precedes an intact stocksupports_reasoningbranch and no longer appends"thinking"— the substitutive-gate bug from last round is gone, andtest_patch7_gate_is_additive_not_substitutivenow actually applies the patch to a verbatim-stock fixture instead of asserting on a hand-built string. translate_thinking_for_model— the sibling synthesis helper — has no production caller in 1.86.2 (tests only), so Patch 9's single call site is sufficient coverage. Downstream (_normalize_reasoning_effort,_route_openai_thinking_to_responses_api_if_needed) readscompletion_kwargs["reasoning_effort"]and no-ops when absent, andthinkingis a named parameter so it is not re-merged fromextra_kwargs— the suppression is clean on both sides.python3 -m pytest tests/config/test_litellm_runtime_modules.py tests/config/test_patch_litellm_cache.py -q→ 51 passed in 0.71s.ruff check config/litellm/ tests/config/→ clean. (Did not run the full suite.)
Patch 9 is the right fix for the previous blocking finding: the measurement table in anthropic_thinking_policy.py's docstring makes the case, and defaulting the synthesis off rather than trying to pick a better bucket is the correct call given the numbers.
One new blocking issue below.
Blocking
1. _ttl_seconds() emits an unbounded warning per proxied request when the TTL env var is malformed
config/litellm/openrouter_capabilities.py:98-138
_env_float is the right idea, but it is on the per-request hot path and warns unconditionally on every call:
get_supported_openai_params() → get_supported_parameters() → _get_cache() → _ttl_seconds() → _env_float()
_get_cache calls ttl = _ttl_seconds() on every lookup — before the freshness check, so caching does not shield it. A malformed LITELLM_OPENROUTER_CAPABILITY_TTL therefore produces one WARNING line per request, forever, with no dedup.
Reproduced directly (stubbed litellm._logging + HTTPHandler, LITELLM_OPENROUTER_CAPABILITY_TTL="1h", 500 simulated lookups):
warnings emitted for 500 simulated requests: 500
last: openrouter capabilities: LITELLM_OPENROUTER_CAPABILITY_TTL='1h' is not a number; using the default 3600.0
"1h" is not a strawman — it is exactly the mistake the docstring's own "5s"-style phrasing invites, and it is the class of typo this warning was added to catch.
Why it matters: this floods the LiteLLM pod log stream that egg's cost_callback per-call JSON observability depends on. A misconfigured TTL degrades from "capability lookups run at the default interval" (harmless) to "the log stream every incident query reads is buried in repeated noise" — and it does so silently at deploy time, only manifesting under load. It also contradicts the bounded-warning discipline this same commit applies in two other places: _SEEN dedup in drop_params_visibility.py, and _WARNED_FETCH_FAILURE in this very module. The rule is already established here; _env_float just doesn't follow it.
Note LITELLM_OPENROUTER_CAPABILITY_TIMEOUT is not affected — it is only read inside _fetch, which is bounded by the TTL.
Fix: memoize the parsed value (the env var cannot change mid-process), or dedupe the warning per (name, raw) the way _WARNED_FETCH_FAILURE does. Either is a few lines.
Non-blocking
2. Patch 9's return suppresses two paths that are explicit caller intent, not a manufactured cap
config/litellm/patch_litellm_cache.py (Patch 9 replacement)
The gate is inserted before three behaviours in _translate_thinking_to_openai, not one:
- the
budget_tokens→ bucket derivation — this is the manufactured ceiling the patch exists to kill; suppressing it is correct; - the adaptive-thinking
output_config.effortoverride; - the
thinking.summary→{"effort": …, "summary": …}handling.
(2) and (3) are cases where the caller explicitly stated an effort. With thinking={"type":"adaptive"} + output_config={"effort":"high"}, stock sends reasoning_effort="high"; with this patch and the env var unset, nothing is sent. That is not "stop inventing a cap", it is "discard what the caller asked for."
Not reachable on egg's route today — Claude Code sends thinking.type == "enabled" — so this is advisory, not blocking. But the patch's stated contract is "stop manufacturing a ceiling from a budget," and the gate is broader than that contract. Narrowing it to the pure budget→bucket derivation would make the code match its own docstring and remove a trap for any future caller shape.
3. _WARNED_FETCH_FAILURE is never reset on a successful fetch
config/litellm/openrouter_capabilities.py
The flag latches for process lifetime. A single transient blip during pod startup permanently demotes every subsequent outage to debug — which is precisely the silence _log_fetch_failure was added to prevent. A real, sustained OpenRouter outage hours later produces no operator-visible signal at all. Clear it in the success path of _fetch (one line, next to the _CACHE_STAMP update).
4. The EGG_MODULE_MARKER clobber guard is structurally unreachable in production
config/litellm/patch_litellm_cache.py, _install_module:
if EGG_MODULE_MARKER not in os.path.basename(spec["dest"]):
raise SystemExit(...)It inspects spec["dest"] — our own hardcoded literal in NEW_MODULES, every entry of which contains _egg_ — not the on-disk file. So it can only ever fire if someone edits NEW_MODULES to a non-_egg_ path. That is a lint of this file's own constants, not upstream-drift detection. test_new_module_refuses_to_clobber_a_foreign_file reaches it only by constructing a synthetic spec that isn't in NEW_MODULES, which confirms the reachability gap rather than closing it.
The _egg_ rename itself is a genuine improvement and does the real defensive work (no collision with an upstream path is possible). If you want the guard to mean something, key it on the existing file's content — e.g. refuse when the on-disk file lacks an egg provenance header. Otherwise it reads as protection that isn't there.
5. Stale module path in a test docstring
tests/config/test_patch_litellm_cache.py:200 still says:
Patch 7's gate imports
litellm.llms.openrouter.capabilities
The module was renamed to _egg_capabilities in this commit. Anyone tracing the import from that docstring lands on a path that no longer exists.
6. cost_callback.py's explanation of request_params is now wrong
config/litellm/cost_callback.py:817-819 (not touched by this PR, but invalidated by it):
Per line rather than once per session because it is NOT session-stable: LiteLLM rewrites
thinkinginto areasoning_effortbucket, so the effective effort tracks the per-turn thinking budget.
With Patch 9 default-off, that rewrite no longer happens on egg's route — reasoning_effort will simply be absent from the line. The per-line emission is still fine, but its stated justification is now false, and it is exactly the comment a future reader would consult when asking "why is reasoning_effort missing from my log lines?" Worth a one-sentence update in this PR since this PR is what changes the behaviour.
7. PR description does not mention Patch 9
gh pr view 3625 --json body has no occurrence of "Patch 9". The body still says "Two build-time patches" and "11 tests … (was 6)", and its safety argument — "reasoning_effort removed from all 9 rows, so this image is safe to roll" — covers only the config-supplied parameter, not the adapter-synthesized one that Patch 9 addresses. That synthesized path is the larger of the two behaviour changes and is invisible to anyone reading the description.
You noted gh pr edit is gateway-blocked for you on this PR. Understood — this is a merge-time obligation for whoever merges, not a code defect. Flagging it so it doesn't get lost.
8. Nit: commit message test count
5979951 says "33 new unit tests"; the actual count is 38 (your own PR comment has the right number).
Context, not a finding
With Patch 9 default-off and reasoning_effort removed from every deployed row, Patch 7 (the OpenRouter capability gate) currently has no observable effect on the deployed route. That is fine — its value is removing the silent-drop trap for the next operator who sets the knob, which is legitimate latent infrastructure. Noting it so the "does this do anything?" question is answered in the record rather than re-litigated later.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…guard real Review round 2 on #3625. _env_float sits on the per-request path via _ttl_seconds, which _get_cache calls ahead of the freshness check, so a malformed LITELLM_OPENROUTER_CAPABILITY_TTL emitted one WARNING per proxied request forever -- burying the log stream egg's per-call cost observability reads. The warning is now deduplicated per (name, value), the same discipline _log_fetch_failure and drop_params_visibility._SEEN already follow here. Patch 9's gate was broader than its own contract. Stock reaches the reasoning_effort assignment two ways: derived from budget_tokens (the manufactured ceiling the patch exists to stop) and stated outright as output_config.effort on an adaptive request. The gate now sits after the adaptive override and exempts an explicitly requested effort, so the patch declines to invent a value rather than discarding an instruction. A thinking.summary request stays suppressed alongside the derived effort and that is now documented: stock carries the summary only as a field of the reasoning_effort dict, so honouring it would mean sending the ceiling. _install_module's clobber guard read our own NEW_MODULES literal, so it could only fire if someone edited this file -- a lint of its own constants dressed as drift detection. Installed modules now carry a provenance header and the guard reads the file on disk, which is reachable in production. The _egg_ prefix requirement is kept and labelled as the lint it is. Also: a successful fetch re-arms _WARNED_FETCH_FAILURE, so one blip at pod startup no longer permanently demotes every later outage to debug; the cost_callback comment explaining request_params no longer describes a rewrite patch 9 turns off; stale _egg_capabilities path in a test docstring. Verified against genuine litellm 1.86.2 sources: all 10 needles unique, patches apply, second pass is a no-op, every touched file ast.parse clean. Executed the patched _translate_thinking_to_openai directly: Claude passthrough unchanged, budget-derived effort suppressed, adaptive output_config.effort still sent, opt-in restores stock. 60 unit tests pass; make lint clean apart from 3 mypy errors pre-existing on main in shared/egg_agent/client.py, a file this PR does not touch.
|
Re-review addressed in Blocking1. Confirmed and reproduced your 500/500 result before fixing. I chose dedup over memoizing the parsed value so a test (or a future caller) that changes the environment still gets the current answer. Non-blocking2. Patch 9's Agreed, and this is the item I'd have gotten wrong twice. The gate now sits after the adaptive override rather than before the whole block, and sets On (3), the I executed the patched
3. Cleared in 4. Clobber guard structurally unreachable — You're right that it was a lint of my own constants wearing drift-detection clothes, and that the synthetic-spec test confirmed the gap rather than closing it. Installed modules now carry a provenance header and the guard reads the file on disk: an unrecognised file at one of our real destinations aborts the build, with no synthetic spec needed. The 5. Stale module path in a test docstring — 6. Rewritten. It now says the per-line emission is for per-request values generally, and adds the thing a future reader actually needs: 7. PR description does not mention Patch 9 — Not a deferral and not a disagreement — I tried. 8. Commit message test count — Recording it here instead: the current count is 50 new tests — Context itemAgreed on the record: with Patch 9 default-off and no configured VerificationAgainst genuine Replacement PR body for whoever merges (item 7)Closes #3624. Three build-time patches to the pinned Patch 7 — read OpenRouter capabilities live
Every OpenRouter slug egg routes is absent from the 1.86.2 map:
OpenRouter publishes the answer itself: Why union and not "live data wins." I built it the other way first and litellm's own test suite caught it. Fails soft throughout: any error, timeout, non-200 or malformed payload yields no opinion. A failed fetch is cached for the TTL too, so an offline deployment costs one attempt per hour rather than one per request, and a lock prevents a stampede. Verified in the built image against the live endpoint:
laguna and k2.7-code stay Patch 9 — stop the adapter manufacturing a reasoning ceilingThis is the larger behaviour change of the two, and patch 7 is a regression on egg's primary route without it. It came out of review: I had reasoned about On that route (Claude Code → egg-gateway → litellm → OpenRouter) the body carries Historically that synthesized param was silently dropped, because the model-cost map does not carry these slugs — which is exactly why these models have been running at full depth. Patch 7 unblocks it. Per the measurements below that would be a ~9x reduction in reasoning on every agent turn, with nothing logged (patch 8 only fires on drops) and no config file to point at. So patch 9 gates the synthesis, off by default. Patch 8 — make
|
| Model | no parameter | flat reasoning_effort: high |
extra_body.reasoning.effort: high |
|---|---|---|---|
moonshotai/kimi-k3 |
3130 | 340 | 86 |
z-ai/glm-5.2 |
1689 | 1090 | 1516 |
On kimi-k3 the distributions do not overlap in either form (lowest no-param sample 1860; highest extra_body sample 137). The knob is a cap below the model default, not a ceiling above it. Today's silent drop is what was giving full depth — which is why patch 9 defaults the synthesis off rather than trying to pick a better bucket.
Operator sequencing (already done)
reasoning_effort has been removed from all 9 rows of ~/.config/egg/litellm-models.yaml, make litellm-config has been run, and the in-cluster ConfigMap and running pod are confirmed to carry zero reasoning_effort rows. That covers the config-supplied param; patch 9 covers the adapter-synthesized one. So this image is safe to roll.
New mechanism
NEW_MODULES installs whole files that have no stock counterpart, with fail-loud source resolution (it caught a missing Dockerfile COPY during development, exactly as intended). Destinations carry an _egg_ prefix so upstream can never occupy one, and each installed file carries a provenance header that _install_module reads before overwriting anything.
Testing
- 56 tests across
tests/config/test_patch_litellm_cache.py(16, was 6) and the newtests/config/test_litellm_runtime_modules.py(40). The patch-script tests cover module install, idempotency, fail-loud source resolution and clobber refusal, patch 7 staying additive rather than short-circuiting the stock branch, patch 9 gating the synthesis without touching the Claude branch or the explicit-effort path, and patch 8 needle disambiguation — 1.86.2 has twodrop_paramsbranches inutils.pysharing an identical condition, so the needle includes the pop loop and the test asserts the sibling (embeddings, barepass) site is left byte-for-byte untouched. Same trap as Patch 4, one file over. The runtime-module tests cover the three installed modules directly, which is what the deferred litellm imports exist to make possible. - Verified against genuine litellm 1.86.2 sources fetched from
raw.githubusercontent.com: all 10 needles occur exactly once,_patch_rootapplies cleanly, a second pass is a no-op, and every touched fileast.parses. The patched_translate_thinking_to_openaiwas then executed directly: Claude passthrough unchanged, budget-derived effort suppressed,output_config.effortstill sent, opt-in restores stock. make lintclean apart from 3 mypy errors pre-existing onmaininshared/egg_agent/client.py, a file this PR does not touch.
Companion PRs in the litellm fork: jwbron/litellm#7 (merged) and jwbron/litellm#8.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of PR #3625 (delta 5979951..b083445)
Verdict: approve. All six findings from the previous round are genuinely fixed — I verified each by execution, not by reading the diff. No blocking issues in the delta. Five non-blocking items below; item 1 is a merge-time obligation for whoever merges.
What I actually ran
The grafted worktree has no reachable parent, so the delta came from gh api repos/jwbron/egg/compare/5979951...b083445. Verification against a genuine litellm==1.86.2 wheel (downloaded with --python-version 3.13, unpacked to a scratch tree — no working-tree mutation):
- All 10 needles occur exactly once in the real 1.86.2 sources, and every
present-marker string is absent from stock. No needle is ambiguous — including the patch 8drop_paramsneedle, which had to be disambiguated from its embeddings sibling. _patch_rootapplies cleanly; a second pass is a complete no-op (idempotency holds).- All 7 touched/installed files
ast.parseclean after patching. - Stock
_translate_thinking_to_openaiin the wheel matches_STOCK_THINKING_TAIL_HEAD/_STOCK_THINKING_TAIL_FOOTintests/config/test_patch_litellm_cache.pybyte-for-byte, so the test's hand-authored stock fixture is not drifting from the pinned upstream.translate_anthropic_thinking_to_reasoning_effortis pure, so relocating the gate after it is behaviour-preserving. - Direct execution of the patched function across all seven quadrants (Claude passthrough; enabled/non-Claude/off; enabled/on; adaptive+
output_config.effort/off; adaptive-without-config/off; summary/off; summary/on) — every result matches the documented contract, including the key one:adaptive+output_config: {effort: "high"}with the gate OFF still yields{'reasoning_effort': 'high'}. The narrowing is real. - 56 targeted tests pass (0.61s).
ruff checkandruff format --check: clean across 17 files.
Prior findings — verified fixed
- Unbounded per-request WARNING from
_env_float— fixed properly._warn_env_once(openrouter_capabilities.py:100) keys on(name, raw), so a different bad value still warns; it isn't a blanket latch.test_env_warning_is_deduplicated_not_emitted_per_requestproves 200 lookups → 1 record, and a second distinct bad value → 2. - Patch 9 clobbering an explicitly requested effort — fixed at the right layer. The needle now sits after the stock adaptive override and
_egg_effort_is_explicitgates only the derived path. This is the correct fix, not a special-case guard: the distinction is now structural (derived vs. caller-supplied), which is exactly what the policy module docstring anddocs/guides/per-agent-models.mdnow claim. - Latched
_WARNED_FETCH_FAILURE— fixed; re-armed on successful fetch (openrouter_capabilities.py:224), covered bytest_successful_fetch_rearms_the_failure_warning. See item 2 below for one path that re-arms without a successful fetch. _install_moduleclobber guard — fixed and hardened beyond what I asked. The prefix check now raises unconditionally at the top of_install_modulerather than being a property of the call sites, the payload carriesEGG_MODULE_HEADER, and overwrite requiresEGG_MODULE_MARKER in existing.test_new_module_refuses_to_clobber_a_foreign_fileis now reachable without a synthetic spec, which was the actual gap.- Stale docstring module path — fixed to
litellm.llms.openrouter._egg_capabilities. cost_callback.pycomment blaming the wrong mechanism — fixed, and the replacement correctly notesreasoning_effortis now normally absent on/v1/messages.
Non-blocking
1. The PR body is materially wrong — second round. It still says "Two build-time patches" (there are nine plus a module-install mechanism), still contains zero occurrences of "Patch 9" — the single highest-impact change here, a ~9x reasoning-token behaviour change — and still claims "11 tests … (was 6)" against an actual 16 plus a new 40-test file. I accept that gh pr edit is gateway-blocked from the sandbox. That makes it a merge-time obligation for the human merger: the squash-merge commit message defaults to the PR body, so as written the permanent git record of this change omits the behaviour change entirely. Someone bisecting a reasoning-quality regression six months out will read "Two build-time patches" and move on. Please rewrite the body before merging.
2. A 200 response with {"data": []} degrades silently and clears a prior failure warning. Reproduced: _fetch() on {"data": []} returns {}, emits zero log records, and executes _WARNED_FETCH_FAILURE = False at openrouter_capabilities.py:224. Two consequences:
- The module docstring at
openrouter_capabilities.py:62-65states an empty dict "records a failed fetch". After this path it records a successful fetch that happened to yield nothing — the two are now indistinguishable downstream, contradicting the comment. - If a real outage already warned, this un-arms the dedup flag without a real recovery, so the next genuine failure re-warns. That direction is benign (extra signal, not lost signal), but it isn't what the re-arm comment describes.
The same applies if every entry is malformed and the parse loop drops them all. Suggest: only re-arm whencapabilitiesis non-empty, and_log_fetch_failure("datalist contained no usable entries; …")when a 200 parses to zero capabilities. That preserves the docstring's invariant and makes an OpenRouter schema change visible instead of looking like a normal fallback.
3. Nothing validates that the patched sources parse. _apply is a bare src.replace(needle, replacement, 1); no test ast.parses the patched output; the Dockerfile runs RUN python3 /egg/patch_litellm_cache.py with no compileall/py_compile (confirmed absent from config/litellm/, scripts/, and .github/workflows/). A replacement with wrong indentation therefore applies cleanly, passes the build, ships in the image, and surfaces as a pod CrashLoopBackOff at import time. This directly undercuts the script's own stated fail-loud discipline — needle misses SystemExit at build, but a syntactically broken result does not. One line in _apply (ast.parse(out) before returning) or RUN python3 -m compileall -q <site-packages>/litellm in the Dockerfile converts a deploy-time crashloop into a build-time failure. Cheap, and this file is going to keep growing patches.
4. Dedup keys are recorded before the log call, and _log swallows exceptions. _warn_env_once adds to _WARNED_ENV before calling _log (openrouter_capabilities.py:100); _log_fetch_failure sets _WARNED_FETCH_FAILURE = True before calling _log (:240). Since _log deliberately never propagates, a failure on the first call permanently loses that signal — subsequent calls see the key already recorded. Theoretical (litellm's logger is in place by then), so I'm not treating it as blocking, but recording the key only after a successful emit costs nothing.
5. Docstring item 9 omits the output_config.effort exemption. The module docstring at patch_litellm_cache.py:121-141 describes patch 9 as gating the synthesis wholesale. The inline PATCHES comment and anthropic_thinking_policy.py's docstring both document that an explicitly requested effort still reaches the provider — the top-of-file summary is the one place a reader looks first, and it's the one place that doesn't say so.
One thing I checked and am satisfied with
The decision to keep thinking.summary suppressed when the gate is off is correct and correctly justified. Stock carries the summary only as a field of the reasoning_effort dict, so there is genuinely no wire shape for "summary, no effort" — passing the summary through would require inventing one. The docstring now says this explicitly rather than leaving it as an unexplained side effect, which is the right resolution.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…etch Addresses the non-blocking items from the third review round. _apply now ast.parse-checks the result before writing it. A needle miss already exits non-zero, but a replacement with wrong indentation applies cleanly — it would pass the build, ship in the image, and surface as a pod CrashLoopBackOff at litellm import. Checked only when the input parsed, so the concatenated-needle fixtures still work; _install_module gets the same guard for a truncated COPY. A 200 that parses to zero capabilities is now reported and treated as a failure rather than re-arming the fetch-failure warning. It was indistinguishable from a fetch that had no opinion, contradicting the _CACHE comment and claiming a recovery that never happened. _log now returns whether it emitted, and both warn-once latches record their key only after the line is out — _log swallows its own failures, so recording first let one failure suppress the signal for the process. Patch 9's top-of-file summary now states the output_config.effort exemption that the inline comment and policy docstring already carried. 9 new tests (65 in tests/config, was 56). Verified end-to-end against a genuine litellm 1.86.2 wheel: 10 needles unique, _patch_root applies and is a no-op on the second pass, all 7 touched files ast.parse clean, and a deliberately de-indented replacement now exits non-zero and leaves the file untouched.
|
Third round addressed in Non-blocking1. PR body is materially wrong — second round — Not a disagreement with the substance: you're right on every count. The body says "Two build-time patches" against nine plus the module-install mechanism, contains zero occurrences of "Patch 9" — the change that actually moves reasoning depth ~9x — and cites "11 tests (was 6)" against an actual 20 + 45. And you're right about why it matters: squash-merge defaults the commit message to the body, so as written the permanent record omits the behaviour change. I re-probed rather than assuming, and the block is real: So I've written the corrected body out in full below. Pasting it into the squash-merge message is the whole fix. 2. Both consequences fixed at the source rather than at the re-arm. Two tests: 3. Nothing validates that the patched sources parse —
One wrinkle worth recording: the check runs only when the pre-patch source parsed. The fixtures in Verified against a genuine
Three tests on the 4. Dedup keys recorded before the log call, and
New 5. Docstring item 9 omits the Fixed, and in your framing rather than as a caveat: the top-of-file summary now says the Claude branch is untouched and so is an effort the caller stated outright, that the gate sits after stock's override so an adaptive request's Verification
Corrected PR body — for the squash-merge messageClick to expand, paste as-isCloses #3624.
Nine build-time patches to the pinned `egg-litellm` image (stock litellm
1.86.2), plus a mechanism for installing whole modules that have no stock
counterpart. Patches 1-6 predate this PR; this one adds 7, 8 and 9.
The three are one story. Patch 7 stops the OpenRouter param gate from failing
closed on current models. Patch 9 stops litellm manufacturing the parameter
that gate would then let through. Patch 8 makes any remaining drop visible.
**The behaviour change that matters is patch 9**, not patch 7.
## Patch 9 — stop synthesizing `reasoning_effort` from `thinking`
On `/v1/messages` — egg's primary route, Claude Code -> gateway -> litellm ->
OpenRouter — litellm's Anthropic adapter (`_translate_thinking_to_openai`)
*replaces* the caller's `thinking: {"type": "enabled", "budget_tokens": N}`
with a bucketed `reasoning_effort` for any non-Claude model
(`is_anthropic_claude_model` is a substring test for `anthropic`/`claude`, so
every OpenRouter slug egg routes takes that branch). Nothing in
`litellm-models.yaml` is involved: the value is manufactured per request.
That bucket is a **cap below the model default**, not a ceiling above it.
Measured directly against OpenRouter (`max_tokens: 16000`, n=4, mean reasoning
tokens):
| Model | no parameter | flat `reasoning_effort: high` | `extra_body.reasoning.effort: high` |
|---|---|---|---|
| `moonshotai/kimi-k3` | 3130 | 340 | 86 |
| `z-ai/glm-5.2` | 1689 | 1090 | 1516 |
On kimi-k3 the distributions do not overlap (lowest no-param sample 1860;
highest `extra_body` sample 137).
This never mattered because patch 7's bug was dropping the synthesized value —
which is why these models have been running at full depth. Shipping patch 7
alone would therefore have cut reasoning ~9x per agent turn, with no config
file to point at and nothing logged (patch 8 fires only on drops, and this
param would no longer be dropped).
So patch 9 gates the synthesis, **default off**. `LITELLM_ANTHROPIC_THINKING_TO_REASONING_EFFORT=1`
restores stock. The Claude branch is untouched, and so is an effort the caller
stated outright: on an adaptive request (`thinking: {"type": "adaptive"}` plus
`output_config: {"effort": ...}`) the gate sits after stock's override, so that
value still reaches the provider. Only the derived bucket is suppressed.
## Patch 7 — read OpenRouter capabilities live
`OpenrouterConfig.get_supported_openai_params` gates `reasoning_effort` on
`litellm.supports_reasoning`, which reads the bundled model-cost map. For
OpenRouter that map is wrong by construction: OpenRouter ships new slugs
continuously and the map lags. The gate is a bare `if`, so it fails **closed**,
and `drop_params: true` then discards the parameter with no exception and no
log line. Every OpenRouter slug egg routes is absent from the 1.86.2 map.
OpenRouter publishes the answer itself: `GET /api/v1/models` returns per-model
`supported_parameters` and needs no API key. The new `openrouter_capabilities.py`
reads it, caches per process, and **unions** it with the map answer.
Union rather than "live data wins" because `supported_parameters` under-reports:
`deepseek/deepseek-r1` is flagged `supports_reasoning: true` in the map and is
plainly a reasoning model, yet OpenRouter advertises only `reasoning` for it.
Reading that absence as a denial would trade one silent drop for another. The
lookup can admit a knob the map does not know, never withhold one it allows, so
the worst case is exactly the unpatched behaviour.
Fails soft throughout: any error, timeout, non-200 or malformed payload yields
no opinion, and a failed fetch is cached for the TTL so an offline deployment
costs one attempt per hour. `LITELLM_OPENROUTER_CAPABILITY_FETCH=0` restores
previous behaviour exactly.
With patch 9 default-off and `reasoning_effort` removed from every deployed
row, patch 7 has no observable effect on the deployed route today. That is
intended: its value is removing the silent-drop trap for the next operator who
sets the knob.
## Patch 8 — make `drop_params` say what it dropped
Stock pops unsupported params in a bare loop with no logging. That silence is
what turned patch 7's bug into a full investigation instead of a log grep. This
covers drops that are **correct** too: `poolside/laguna-s-2.1` genuinely does
not accept `reasoning_effort`, so it is dropped on purpose and the operator
otherwise cannot learn why their config line does nothing.
The message states what is known and stops short of prescribing a config edit —
the param most often dropped here is one litellm synthesized itself, so
directing the operator to `config.yaml` would send them looking for a line that
does not exist. Deduped per `(provider, model, param-set)`, bounded, and
wrapped so a diagnostic can never break a request.
## Operator sequencing (already done)
`reasoning_effort` has been removed from all 9 rows of
`~/.config/egg/litellm-models.yaml`, `make litellm-config` has been run, and the
in-cluster ConfigMap and running pod carry zero `reasoning_effort` rows. Note
this covers only the config-supplied parameter — the adapter-synthesized one is
what patch 9 handles.
## Build-time safety
`_apply` fails loudly if a needle is missing (version drift) and now also if the
patched result does not `ast.parse` — a replacement with wrong indentation would
otherwise apply cleanly, ship, and surface as a pod CrashLoopBackOff at import.
`NEW_MODULES` installs whole files with fail-loud source resolution, an `_egg_`
destination prefix, and a provenance header it refuses to clobber without.
## Testing
- 65 tests across `tests/config/test_patch_litellm_cache.py` (20) and
`tests/config/test_litellm_runtime_modules.py` (45, new): needle
disambiguation, patch-7 additivity against a verbatim-stock fixture, patch-9
behaviour across all seven request quadrants, capability caching/negative
caching/env handling, and drop_params dedup.
- Verified against a genuine `litellm==1.86.2` wheel: 10 needles unique,
`_patch_root` idempotent, all 7 touched files parse.
- `make lint` clean (3 pre-existing mypy errors in `shared/egg_agent/client.py`,
untouched here).
Companion PRs in the litellm fork: jwbron/litellm#7 (merged) and jwbron/litellm#8.
Authored-by: egg— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of PR #3625 (delta b083445..3481a4d)
Verdict: approve. All four code items from round 3 are genuinely fixed — I verified each by execution, not by reading the diff. No blocking issues in the delta. Four non-blocking items below; item 1 is the same merge-time obligation as last round.
What I actually ran
The grafted worktree has no reachable parent, so the delta came from gh api repos/jwbron/egg/compare/b083445...3481a4d.
PATCHES/NEW_MODULESare byte-identical to the previously-verified state. Every+/-line inpatch_litellm_cache.pylands in the module docstring,import ast,_parses,_check_parses,_apply, or_install_module. No needle, replacement, or present-marker moved, so last round's verification against a genuinelitellm==1.86.2wheel (10 needles unique,_patch_rootidempotent, all 7 files parse) still holds without re-running it.- 65 tests pass in
tests/config/(0.80s).ruff check+ruff format --check: clean across 17 files. - Item 2 fixed — executed. Real outage → 1 warning,
_WARNED_FETCH_FAILURE=True. Then{"data": []}on a fresh cache → returnsNone, latch staysTrue, the "datalist contained no usable entries" line goes to debug as a repeat. Then a real 200 with capabilities →{'reasoning_effort'}and the latch re-arms toFalse. Exactly the invariant the_CACHEcomment claims:{}now means one thing. - Item 3 fixed — executed. Fed
_install_modulea truncated staged file:SystemExit: … patched source does not parse (…:6: '(' was never closed), and the destination was not created._apply's guard is covered bytest_malformed_replacement_fails_at_build_time, which also asserts the target file is left byte-for-byte unchanged — the half that matters as much as the exit code. Confirmed the guard is enforced pre-merge, not just at manual build:test-integration.yml:85runsmake build, which runsconfig/litellm/Dockerfile'sRUN python3 /egg/patch_litellm_cache.py. - Item 4 fixed — executed. With a logger that raises on its first emit: call 1 → 0 emitted,
_WARNED_ENVempty; call 2 → 1 emitted, key recorded; calls 3–4 → still 1. The signal survives the failure and dedup still holds after. - Item 5 fixed —
patch_litellm_cache.py:138-150now carries theoutput_config.effortexemption and thethinking.summaryrationale, matchinganthropic_thinking_policy.py:42-49. - The
if _parses(src)escape hatch in_applyis load-bearing, not a hedge:_build_fixture_rootwrites"# fixture head\n\n" + "\n\n".join(needles), and the needles are class-body/function-body fragments (ZAI = "z-ai"\n), so every fixture is anIndentationErrorby construction. Without the gate, every existing fixture test would fail on the fixture rather than the patch.
Non-blocking
1. The PR body is still materially wrong — third round. gh pr edit is genuinely blocked for a PR owned by jwbron, and you supplied a full corrected body in the last comment, so there is nothing further you can do. Restating it here so it does not get lost at merge time: the squash-merge message defaults to the PR body, and as written the permanent git record says "Two build-time patches", contains zero occurrences of "Patch 9" — the ~9x reasoning-depth change — and cites "11 tests (was 6)" against an actual 20 + 45. Whoever merges: paste the corrected body from the 3481a4d follow-up comment into the squash message.
2. drop_params_visibility.py has the identical record-before-emit defect item 4 just fixed, and was left out of the fix. _SEEN.add(key) (drop_params_visibility.py:72) runs before _log_warning (:83), and _log_warning does not swallow — the outer except Exception in warn_dropped_params does. Reproduced:
5 calls, same (provider, model, param-set), logger raises on first emit
→ emit attempts: 1 | lines actually logged: 0
→ _SEEN: {('openrouter', 'poolside/laguna-s-2.1', ('reasoning_effort',))}
One failed emit and that route never warns again for the life of the process. Same theoretical trigger as item 4 (litellm's logger is up by the time get_optional_params runs), so same severity — but it is the third of three warn-once latches in this changeset and the only one still recording before the line is out. It is also the one where the failure mode is thematically worst: this module exists specifically so a drop is not silent, and this makes it silent. Fix mirrors the other two: _log_warning returns a bool, _SEEN.add(key) only on True. (Note _log_warning currently lets the import error propagate to the outer handler, so it needs its own try to return False rather than skipping the add by exception — otherwise the key is simply never recorded and you get one warning attempt per request.)
3. _install_module's parse error points one line past the real error and blames a replacement that does not exist on that path. payload = EGG_MODULE_HEADER + fh.read() (patch_litellm_cache.py:946) shifts every line by one, and _check_parses reports exc.lineno against source — the un-headered file on disk. Reproduced with a staged module whose syntax error is on line 5:
Module 1/3 (openrouter capabilities): patched source does not parse
(/tmp/.../openrouter_capabilities.py:6: '(' was never closed) — the replacement is malformed
Two small things: :6 sends the operator to the wrong line, and "the replacement is malformed" is _apply's vocabulary — on the install path there is no replacement, the staged source itself is broken. Suggest parsing the un-headered text (the header is a comment; it cannot change whether the rest parses) and taking the trailing clause as a parameter so each call site says what actually went wrong.
4. _log's new docstring overstates what the bool means. "Returns whether the record was actually emitted" (openrouter_capabilities.py:92) — it returns whether the call did not raise. verbose_logger.debug(...) on a logger at INFO returns normally, so _log returns True with nothing emitted. No behavioural consequence today: _log_fetch_failure only selects debug when _WARNED_FETCH_FAILURE is already True, so the latch assignment is a no-op there. But both latches' correctness comments are argued in terms of "once the line is out", and that is not the property being measured. Either narrow the docstring to "the call did not raise" or note the level-filtering caveat.
Coverage note (not a request)
Nothing in tests/config/ exercises the real PATCHES replacements through the new parse guard — the fixtures are unparseable by construction, so if _parses(src) skips _check_parses for every fixture-based test, and the four new _apply tests use toy patches. That is fine as shipped, because make build runs in CI and the guard is fail-loud there. Worth recording only because the test file's own docstring frames itself as "the patch-script regression the CI image build doesn't give us" — for this one class of defect, the build is still the only gate. The fixture design makes closing it awkward (each needle would need a per-needle scaffold), which is why I am not asking for it here.
One thing I checked and am satisfied with
Ordering _check_parses before the dest_dir existence check and the existing == payload idempotency short-circuit is harmless — our own modules always parse, so the reordering never masks the "destination package missing — LiteLLM version drift?" signal in practice, and re-parsing three small modules per root on an already-patched tree costs nothing. Likewise ast.parse running twice per _apply on utils.py (~500KB, two patches, two roots) is single-digit seconds of build time for a guard that converts a pod CrashLoopBackOff into a build failure. Right trade.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Round-4 review follow-ups. drop_params_visibility recorded its dedup key before the emit, so one logging failure on the first call muted that route for the life of the process — in the module whose entire purpose is to stop a drop being silent. `_log_warning` now returns whether the emit raised and the key is recorded only on success, matching the two latches already fixed in openrouter_capabilities. `_install_module` parsed the headered payload, so a broken staged module reported a line one past the real error and borrowed `_apply`'s "the replacement is malformed" wording on a path with no replacement in it. It now parses the un-headered text and `_check_parses` takes the trailing clause as a parameter. `_log`'s docstring claimed the bool meant "emitted"; it means the call did not raise, since a level-filtered call also returns normally. Two new tests (67 in tests/config/). make lint clean; the 3 mypy errors in shared/egg_agent/client.py are byte-identical to origin/main.
Round-4 feedback addressed —
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of PR #3625 (delta 3481a4d..272b08c)
Verdict: request changes. The code delta is clean — all three round-4 items are genuinely fixed and I verified each by execution, not by reading the diff. The one blocker is not on the branch: the corrected PR body you supplied for the squash message regressed this round and now names an env var that does not exist.
What I actually ran
The grafted worktree has no reachable parent, so the delta came from gh api repos/jwbron/egg/compare/3481a4d...272b08c. 5 files, 112+/23-.
-
67 tests pass in
tests/config/(0.77s, 21 + 46).ruff check+ruff format --checkclean across 17 files. -
Item 2 fixed — executed.
_FlakyLoggerequivalent, 5 identical calls, logger raises on the first emit:call 1: attempts=1 emitted=0 _SEEN=0 call 2: attempts=2 emitted=1 _SEEN=1 call 3-5: attempts=2 emitted=1 _SEEN=1The signal survives the failure, the retry costs exactly one extra attempt (not one per request forever), and dedup holds once the line is out. I also checked the wrinkle I flagged last round:
_log_warningnow carries its owntry/except(drop_params_visibility.py:54-60) and returnsFalse, so the key is skipped by return value rather than by exception — which is what makes the retry bounded. -
The
_MAX_WARNINGSbound still holds after moving the cap insideif emitted:. Drove 2500 distinct(provider, model, param-set)keys:peak _SEEN: 1000, cap: 1000. The clear-on-overflow moved with theadd, so both invariants survive. -
Item 3 fixed — executed. Staged a module with the syntax error deliberately on line 5:
Module 1/3 (openrouter capabilities): staged module source does not parse (/tmp/.../openrouter_capabilities.py:5: '(' was never closed)Real line, install-path vocabulary, no "replacement", and the destination was not created. The
detailparameter is threaded to both call sites and there are only two (patch_litellm_cache.py:241,:962) — no caller left on the old signature. -
Verified the header-prepend argument rather than taking it.
_check_parsesnow validatesbodywhilepayload = EGG_MODULE_HEADER + bodyis what gets written, so the guard is only sound if a leading comment cannot change parseability. Confirmed for all three real modules:body_parses=True payload_parses=True docstring_preserved=True—ast.get_docstringstill finds the module docstring, so the comment's "nothing about the module changes" claim holds. -
Item 4 fixed —
openrouter_capabilities.py:92-99now says "completed without raising" with the level-filtering caveat spelled out, and the two latch comments (:120,:267) were re-argued in the same terms. No behaviour change; the diff on this file is docstrings and comments only. -
No fourth latch. Grepped the three staged modules:
_WARNED_ENV,_WARNED_FETCH_FAILURE,_SEENare all of them, and all three now record after the emit.anthropic_thinking_policy.pydoes no logging at all.
Blocking
1. The corrected PR body now names an env var that does not exist — and it regressed this round
The round-4 comment's squash body says:
The synthesis is now suppressed by default (
LITELLM_ANTHROPIC_THINKING_SYNTHESIS=1restores it).
There is no such variable. The knob is LITELLM_ANTHROPIC_THINKING_TO_REASONING_EFFORT:
config/litellm/anthropic_thinking_policy.py:58—ENV_VAR = "LITELLM_ANTHROPIC_THINKING_TO_REASONING_EFFORT"docs/guides/per-agent-models.md:641,k8s/base/litellm-deployment.yaml:122,patch_litellm_cache.py:138and:820— all correct.grep -rn "LITELLM_ANTHROPIC_THINKING_SYNTHESIS"over the repo: zero hits.
This is new. I traced the name through every comment on the PR: the round-3 body (23:36:26Z) said LITELLM_ANTHROPIC_THINKING_TO_REASONING_EFFORT, correctly. The round-4 body (23:49:21Z) is the first appearance of ..._SYNTHESIS.
Why it blocks rather than being another "the body is wrong" note. The last two rounds' body defects were stale — wrong counts, a missing patch — and you had no tool to fix them. This one is different on both axes:
- It is wrong in the direction that costs an operator the most. Patch 9 is the change with a measured ~9x effect on reasoning depth, and this line is the only instruction anywhere for undoing it. An operator who reads the squash message, sets
LITELLM_ANTHROPIC_THINKING_SYNTHESIS=1, and restarts getsos.getenv→None→should_synthesize_reasoning_effort()→False(anthropic_thinking_policy.py:69-72). Synthesis stays off, nothing warns, and the knob they set does nothing — the exact silent-ignored-operator-input failure mode this PR spent nine patches and four review rounds building machinery to eliminate. - It is fixable from here.
gh pr editis still denied, and I am not asking you to re-probe that. But the body the merger pastes is one you author in a comment, and reposting a corrected one is entirely within reach.
Fix: repost the squash body with LITELLM_ANTHROPIC_THINKING_TO_REASONING_EFFORT, and say plainly in the comment that it supersedes the 272b08c version, so the merger does not paste the wrong one from higher in the thread. While you are in there, the rest of that body checks out — I verified the 67 tests / 21 + 46 split against the actual run.
Non-blocking
2. _env_flag silently resolves an unrecognized disable spelling to enabled, and the module docstring promises otherwise. openrouter_capabilities.py:43-45 claims, of all three knobs it has just listed:
An unparseable or out-of-range value for any of these is logged and ignored rather than silently swallowed: an operator reaching for these vars is very likely already debugging something.
That is true for TTL and TIMEOUT — _env_float routes both through _warn_env_once. It is false for FETCH, which goes through _env_flag (:128-132) and has no warn path at all. Executed:
FETCH='0' -> enabled=False warnings=[]
FETCH='off' -> enabled=False warnings=[]
FETCH='disabled' -> enabled=True warnings=[] <-- opposite of intent
FETCH='n' -> enabled=True warnings=[] <-- opposite of intent
not in ("0","false","no","off") means anything unrecognized is read as enable, so a near-miss disable spelling does not fall back to the default — it inverts the operator's instruction, silently. Low real-world risk (the documented spelling is =0, and the consequence is the fail-soft default staying on), which is why this is advisory rather than blocking. But the docstring is the thing that would make an operator trust a typo would be caught, and it currently overclaims. Either narrow the docstring to name TTL/TIMEOUT, or give _env_flag the same _warn_env_once treatment for values matching neither list.
3. should_synthesize_reasoning_effort has the same shape, without the docstring overclaim. raw.strip().lower() in _TRUTHY (anthropic_thinking_policy.py:72) — an operator who writes =enabled or =y gets False with no signal, and since False is also the default they cannot tell the difference between "ignored" and "working as configured". Same discipline gap as item 2, on the highest-impact knob in the PR. Worth a warn-once for a value that is neither truthy nor falsy, given this changeset already built that machinery one file over.
4. drop_params_visibility's comment and its message disagree about prescribing. The comment at :81-90 opens "Deliberately states what is known and stops short of prescribing" and then argues at length that pointing the operator at allowed_openai_params would be wrong. The message at :91-104 does prescribe it — conditionally, gated on "If they came from this model's litellm_params in config.yaml". The conditional framing is fine and I would not change the message; the comment reads as stale from the revision that removed the unconditional remedy. Pre-existing, not in this delta.
Coverage note (not a request)
Unchanged from last round and still accurate: the fixtures are unparseable by construction, so if _parses(src) (patch_litellm_cache.py:240) skips _check_parses for every fixture-based test, and the real PATCHES replacements are only exercised through the guard by make build in test-integration.yml:85. Both new tests this round exercise production entry points properly — warn_dropped_params and _install_module, with only _module_source (a locator, not the logic under test) stubbed, and NEW_MODULES[0] taken from the real constant rather than hand-built. No self-seeding, no bypass.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
_env_flag read anything outside its falsy list as *enable*, so a near-miss disable spelling (FETCH=disabled, FETCH=n) inverted the operator's instruction silently — and the module docstring promised all three knobs were checked. Both knobs now recognise an on-list and an off-list and warn once on anything else, taking the caller's default. should_synthesize_reasoning_effort gets the same treatment: False is also its default, so an operator who typed =enabled could not tell 'ignored' from 'working as configured' on the knob with the ~9x measured effect on reasoning depth. The new latch records after the emit, like the other three. Also refreshes drop_params_visibility's stale 'stops short of prescribing' comment (and its twin in patch_litellm_cache) to describe the conditional remedy the message actually carries.
Round-5 feedback addressed —
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of PR #3625 (delta 272b08c..24a3441)
Verdict: approve with suggestions. My round-5 blocker is resolved, and I checked it the way the last two rounds went wrong — by grepping every identifier in the reposted body against the tree rather than reading it for plausibility. The three code items are real fixes, verified by execution. One non-blocking finding below, which is new in this delta.
Delta came from gh api repos/jwbron/egg/compare/272b08c...24a3441 — the grafted worktree has no reachable parent. 5 files, 185+/20-, one commit.
What I ran
-
85 tests pass in the two litellm files, and the split matches the reposted body exactly:
test_patch_litellm_cache.py→ 21,test_litellm_runtime_modules.py→ 64.ruff check+ruff format --checkclean overconfig/litellm/and the test file (6 files). -
Round-5 blocker fixed. The reposted body says
LITELLM_ANTHROPIC_THINKING_TO_REASONING_EFFORT=1, which is the real name (anthropic_thinking_policy.py:62), and the supersedes note is at the top of the comment where a merger picking between two<details>blocks will actually hit it. I re-grepped every identifier the new body names rather than spot-checking the one I flagged:Claim in reposted body Verified LITELLM_ANTHROPIC_THINKING_TO_REASONING_EFFORT✓ anthropic_thinking_policy.py:62LITELLM_OPENROUTER_CAPABILITY_FETCH✓ openrouter_capabilities.py:376LITELLM_ANTHROPIC_THINKING_SYNTHESIS✓ 0 hits — gone _egg_capabilitiesmodule name✓ patch_litellm_cache.py:915dest: llms/openrouter/_egg_capabilities.pyoutput_config.effort,allowed_openai_params,litellm_params✓ all present in the installed replacements "85 tests … (21) and (64)" ✓ by execution "Patches 1-6 predate this PR" ✓ git show origin/main:…patch_litellm_cache.pyhas exactly 7 specs labelledPatch 1/6…6/6and noNEW_MODULES" make buildintest-integration.ymlruns the patch script"✓ test-integration.yml:85→make build;Makefile:538includesbuild-litellm;Makefile:562buildsconfig/litellm/Dockerfile, whichRUNs the script -
Item 2 fixed — executed, not read. Both boolean knobs now behave as claimed:
openrouter FETCH '0'->False warn=0 'off'->False warn=0 'disabled'->True warn=1 'n'->True warn=1 policy ENV_VAR 'disabled'->False warn=1 'enabled'->False warn=1 '0'/'off'->False warn=0 '1'->True warn=0disabledandnno longer invert the instruction, the caller's default is returned rather than a guess (the test asserts this fordefault=Trueanddefault=False, which is the half that actually proves "default, not guess"), and the warning is deduped through the existing_WARNED_ENV. -
Item 3 fixed, and the fourth latch holds the same discipline.
anthropic_thinking_policy.py:105-121records_WARNED_VALUESonly insideif _log(...), matching the other three sites. I re-grepped the three staged modules:_WARNED_ENV,_WARNED_FETCH_FAILURE,_SEEN,_WARNED_VALUES— four latches, all record-after-emit, none left on the old ordering. -
Item 4 fixed, and the same stale claim was chased one file over.
drop_params_visibility.py:81-93now says the message "prescribes only under a condition the operator can check," which matches:98-99(If they came from this model's litellm_params in config.yaml…).patch_litellm_cache.py:118-124carried the identical contradiction in the patch-8 header (does NOT prescribe a config edit) and was corrected in the same commit — right call, a header comment that contradicts the code it installs is the same defect at higher visibility. -
Test quality. All five new/extended tests drive production entry points (
_env_flag,should_synthesize_reasoning_effort) with onlyverbose_loggerstubbed. No hand-built fixtures, no self-seeded goldens, and no name-vs-behaviour contradiction —test_env_flag_warns_rather_than_inverting_a_near_missasserts exactly what its name says. The fixtures reload each module per test (_load→ freshspec.loader.exec_module), so the four parametrizedenabled/y/maybe/2cases cannot leak_WARNED_VALUESintotest_unrecognised_value_warning_survives_a_swallowed_emit_failure, which also usesenabled. I checked that specifically because shared module state plus an overlapping parametrize value is how that test would silently stop testing anything. -
Angle B (removed behaviour):
_env_flag's deletedreturn raw.strip().lower() not in (...)is the only removed line with an invariant attached, and it is only ever called withdefault=True(openrouter_capabilities.py:376), so the return value is unchanged for every input in production. This delta is pure added signal, no behaviour change on either knob.
Non-blocking
1. The two modules now disagree about the empty value, and the openrouter side emits a misleading complaint for it. New in this delta. anthropic_thinking_policy.py:65 puts "" in _FALSY; openrouter_capabilities.py:132 does not. Executed:
policy '' -> False warn=0 ' ' -> False warn=0
openrouter '' -> True warn=1 ' ' -> True warn=1
"LITELLM_OPENROUTER_CAPABILITY_FETCH='' is not a boolean
(expected one of 1, true, yes, on or 0, false, no, off)"
Blanking a var is a normal way to spell "unset" — docker run -e VAR=, export VAR=, a k8s value: "". The behaviour is right in both modules (the default is taken), so this is advisory, not blocking. What is wrong is the signal: the openrouter message tells an operator their value "is not a boolean" and then lists eight spellings, none of which is the empty string, for an input the code handled correctly. That is a misleading diagnostic in a changeset whose thesis is that operator input must never resolve silently or misleadingly — and the same gesture on the sibling knob produces nothing at all.
The code already knows this. anthropic_thinking_policy.py:118 writes ", ".join(v for v in _FALSY if v) — filtering "" out of the message precisely because it is not presentable as a spelling. It is in _FALSY to suppress a warning, not because anyone thinks it is an off-spelling.
Cleanest fix is one line in each module: treat raw.strip() == "" as unset and return default silently, before the truthy/falsy tests. That makes both knobs agree, drops "" from _FALSY (and the if v filter with it), and preserves every current return value — "" already resolves to the default in both modules today, so nothing observable changes except the spurious warning going away.
2. _WARNED_VALUES' bound rests on an assumption about litellm, not a property of this module. anthropic_thinking_policy.py:67-70 says "Bounded by construction: the environment does not change mid-process." That is a claim about every other component in the proxy process, and it is the one latch of the four with no hard ceiling — drop_params_visibility.py:35 carries _MAX_WARNINGS = 1000 with cap-and-clear for exactly this reason. The set is keyed on distinct env values, so the realistic worst case is small either way; a _MAX-style cap would make the bound unconditional for roughly zero cost, and would stop the comment having to argue for it.
3. Not this PR, but it bears on the coverage note. test-integration.yml:80-82's comment describes make build as "same image set (gateway/orchestrator/sandbox)" — stale since build-litellm and build-redis were added (Makefile:538). The coverage argument carried across the last three rounds ("make build runs the patch script against the real 1.86.2 tree") is correct, but a reader who checks it against that comment would conclude the opposite. Untouched file, so genuinely out of scope here — flagging it so it does not keep costing a verification step every round.
Merge-time note (not a request)
The PR body field on GitHub is still the round-1 text: it describes only patches 7 and 8 (no patch 9 — the change with the measured ~9x effect), says "11 tests … (was 6)", and names openrouter_capabilities.py rather than the installed _egg_capabilities. gh pr edit is denied to the producer (PR #3625 is not owned by james-in-a-box), re-probed this round, so this is not something the branch can fix. It is a squash-message defect, not a code defect — hence not blocking — but the merger must paste the corrected body from the 24a3441 comment, not the one higher in the thread. The two <details> blocks look identical at a glance and only one names a real env var.
Coverage note (unchanged, recorded not re-litigated)
The fixtures are unparseable by construction (_build_fixture_root concatenates class-body and function-body needle fragments), so if _parses(src) at patch_litellm_cache.py:240 skips _check_parses for every fixture-based test; the real PATCHES replacements reach the guard only via make build in CI, which is fail-loud. Nothing in this delta touches that path.
— Authored by egg
|
egg review completed. View run logs 15 previous review(s) hidden. |
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
1 participant
Closes #3624.
Two build-time patches to the pinned
egg-litellmimage (stock litellm 1.86.2): stop the OpenRouter param gate from failing closed on current models, and make any remaining drop visible.Patch 7 — read OpenRouter capabilities live
OpenrouterConfig.get_supported_openai_paramsgatesreasoning_effort/thinkingonlitellm.supports_reasoning, which reads the bundled model-cost map. For OpenRouter that map is wrong by construction: OpenRouter ships new slugs continuously and the map lags. The gate is a bareif, so it fails closed, anddrop_params: truethen discards the parameter with no exception and no log line.Every OpenRouter slug egg routes is absent from the 1.86.2 map:
model_costsupports_reasoningopenrouter/moonshotai/kimi-k3Falseopenrouter/z-ai/glm-5.2Falseopenrouter/poolside/laguna-s-2.1Falseopenrouter/deepseek/deepseek-v4-proFalseopenrouter/moonshotai/kimi-k2.7-codeFalseopenrouter/deepseek/deepseek-r1(control)TrueOpenRouter publishes the answer itself:
GET /api/v1/modelsreturns per-modelsupported_parametersand needs no API key (verified: HTTP 200 unauthenticated, 345 models). The newopenrouter_capabilities.pymodule reads it, caches per process, and is unioned with the map answer.Why union and not "live data wins." I built it the other way first and litellm's own test suite caught it.
supported_parametersunder-reportsreasoning_effort:deepseek/deepseek-r1is flaggedsupports_reasoning: truein the map and is plainly a reasoning model, yet OpenRouter advertises onlyreasoningfor it, treating the OpenAI spelling as an alias. Reading that absence as a denial would drop a working param, trading one silent drop for another. The lookup can admit a knob the map does not know, but never withholds one the map allows, so the worst case is exactly the unpatched behaviour.Fails soft throughout: any error, timeout, non-200 or malformed payload yields no opinion. A failed fetch is cached for the TTL too, so an offline deployment costs one attempt per hour rather than one per request, and a lock prevents a stampede.
LITELLM_OPENROUTER_CAPABILITY_FETCH=0restores previous behaviour exactly.Verified in the built image against the live endpoint:
moonshotai/kimi-k3FalseTrueFalsez-ai/glm-5.2FalseTrueFalsepoolside/laguna-s-2.1FalseFalseFalsemoonshotai/kimi-k2.7-codeFalseFalseFalsedeepseek/deepseek-r1TrueTrueTrueopenai/gpt-4o-miniFalseFalseFalselaguna and k2.7-code stay
Falsecorrectly: OpenRouter advertisesreasoning/include_reasoningfor them but notreasoning_effort, on every endpoint.Patch 8 — make
drop_paramssay what it droppedStock pops unsupported params in a bare loop with no logging. That silence is what turned patch 7's bug into a full investigation instead of a log grep. Patch 7 removes the OpenRouter false-negative; this covers the rest, including drops that are correct: laguna genuinely does not accept
reasoning_effort, so it is dropped on purpose and the operator otherwise cannot learn why their config line does nothing.The message names the proxy-config remedy (
allowed_openai_paramsin amodel_listentry'slitellm_params), not just the per-request kwarg, since the audience readsconfig.yaml. That route is verified to work (LiteLLM_ParamsisConfigDict(extra="allow"), and the value reachesget_optional_paramsfrom there).Deduped per
(provider, model, param-set), bounded at 1000 entries, past which it stops recording rather than stops warning. Wrapped so a diagnostic can never break a request.Verified in the built image: laguna +
reasoning_effortwarns once naming both the param and the remedy, stays silent on repeat calls, and glm-5.2 does not warn at all now that patch 7 admits the knob there.Operator sequencing (already done)
The overlay must not set
reasoning_effortwhen this ships. Measured directly against OpenRouter (max_tokens: 16000, n=4, mean reasoning tokens):reasoning_effort: highextra_body.reasoning.effort: highmoonshotai/kimi-k3z-ai/glm-5.2On kimi-k3 the distributions do not overlap in either form (lowest no-param sample 1860; highest
extra_bodysample 137). The knob is a cap below the model default, not a ceiling above it: sending it cuts reasoning 9x flat, 34x viaextra_body. Today's silent drop is what was giving full depth.reasoning_efforthas been removed from all 9 rows of~/.config/egg/litellm-models.yaml,make litellm-confighas been run, and the in-cluster ConfigMap and running pod are confirmed to carry zeroreasoning_effortrows. So this image is safe to roll.New mechanism
NEW_MODULESinstalls whole files that have no stock counterpart, with fail-loud source resolution (it caught a missing DockerfileCOPYduring development, exactly as intended).Testing
tests/config/test_patch_litellm_cache.py(was 6): module install, install idempotency, missing-source fail-loud, patch 7 staying additive rather than short-circuiting the stock branch, and patch 8 needle disambiguation. That last one matters: 1.86.2 has twodrop_paramsbranches inutils.pysharing an identical condition, so the needle includes the pop loop and the test asserts the sibling (embeddings, barepass) site is left byte-for-byte untouched. Same trap as Patch 4, one file over.make lintclean.make test: 4 pre-existing failures onmain(.egg-worktreesallowlist, unrelated), no new ones.Companion PRs in the litellm fork: jwbron/litellm#7 (merged) and jwbron/litellm#8.