Skip to content

fix(providers): deliver the reasoning budget on OpenRouter, and never drop a param in silence - #369

Merged
MattJColes merged 2 commits into
mainfrom
claude/lgtmaybe-perf-reasoning-effort-3j8akh
Aug 3, 2026
Merged

fix(providers): deliver the reasoning budget on OpenRouter, and never drop a param in silence#369
MattJColes merged 2 commits into
mainfrom
claude/lgtmaybe-perf-reasoning-effort-3j8akh

Conversation

@MattJColes

Copy link
Copy Markdown
Owner

Closes the loop on #348. reasoning_effort has been set in this repo's .lgtmaybe.yml since 1.12.0 and was never reaching the model.

The bug

cli/__init__.py puts reasoning_effort into the provider opts correctly and ungated. The adapter sets litellm.drop_params = True (so one unsupported param can't fail a whole review). litellm's openrouter transformation adds reasoning_effort to its supported-params list only when litellm.supports_reasoning(model=...) is true — i.e. only for models already in litellm's capability map. Everything else is dropped with no warning.

Re-verified against the installed litellm (1.94.1), reading OpenrouterConfig.get_supported_openai_params and calling get_optional_params directly:

model supports_reasoning param forwarded
openrouter/~deepseek/deepseek-v4-flash-latest False no
openrouter/deepseek/deepseek-v4-flash-latest False no
openrouter/deepseek/deepseek-r1 True yes
openrouter/anthropic/claude-sonnet-4.5 True yes

It is not the ~ floating-alias prefix — the bare name is unmapped too. The models a reasoning budget gets configured for are precisely the newest ones, which are precisely the ones the map does not know yet.

Live consequence: #367's review, with reasoning_effort: low set in this repo's config, still had a lens spend 34,012 reasoning tokens against the 32,768 max_tokens ceiling and truncate.

Part 1 — never silently discard a configured param

providers/factory.dropped_params reads litellm's own two maps — get_supported_openai_params for the resolved model, and OPENAI_CHAT_COMPLETION_PARAMS for the vocabulary — and names every param the user configured that the model will not accept. One structured warning at startup, next in spirit to the existing "per-call timeout resolved" line:

configured params are not supported by this model and will be ignored
  provider=openai model=openai/gpt-4o ignored_params=['reasoning_effort']

It runs in the factory rather than the CLI because that is where the litellm model string the capability map keys on comes into existence, and it judges exactly the user-configured opts — before the factory adds its own timeout, credentials, and ollama options.

Deliberately general, deliberately small: keyed off the capability map, so a param added to ReviewConfig later is covered without touching this function. Only OpenAI-vocabulary params are judged — a provider-native passthrough like ollama's num_ctx never appears in the map and is not litellm's to drop, so flagging it would only train the reader to ignore the warning. A lookup failure reports nothing; this is instrumentation and must never be why a review doesn't run.

Part 2 — actually deliver the budget on OpenRouter

Verified against OpenRouter's current documentation (openrouter.ai/docs/guides/best-practices/reasoning-tokens and the ChatRequestReasoningEffort schema in its API reference, via Context7 — the docs site 403s direct fetches):

  • the field is a top-level reasoning object: {"effort": ..., "max_tokens": ..., "exclude": ..., "enabled": ...}
  • reasoning.effort accepts xhigh | high | medium | low | minimal | none
  • reasoning_effort exists as a flat shorthand, and the docs state it "cannot be used simultaneously with reasoning.effort if they differ"

So on the openrouter route only, when litellm has been observed to drop the flat param, the budget goes out as extra_body={"reasoning": {"effort": ...}} instead.

No double-send. OpenRouter rejects a request carrying both with 400 Only one of "reasoning" and "reasoning_effort" may be provided. The extra_body is added only on the branch where dropped_params already reported the flat param dropped, and the flat param is popped when it is. When litellm will forward natively (the deepseek-r1 case), nothing is injected.

Value mapping. ReviewConfig.reasoning_effort is litellm's normalised set: none | minimal | low | medium | high | xhigh | default. Six of the seven are in OpenRouter's enum verbatim and pass through unchanged. default has no equivalent — it is omitted and reported through Part 1's warning rather than translated into a nearby level, which would quietly buy a budget nobody asked for. (litellm's own openrouter transformation translating maxxhigh is the hint that this is their vocabulary, not OpenAI's.)

Scope. Every other route is byte-identical — asserted directly (test_no_other_route_is_reshaped), and the whole existing provider matrix still passes unchanged.

A deliberate exception, not a precedent

CLAUDE.md's "Key decisions (do not relitigate)" names litellm as the provider spine that "normalises … to one completion() call". This is a maintainer-approved exception to that, for one param on one route, because litellm's normalisation is keyed on a model list that structurally lags the models people point at. It is called out as such in _honour_param_support's docstring and in the spec. It is not licence for general per-provider plumbing.

Part 3 — correcting the record

.lgtmaybe.yml's comments concluded reasoning_effort was "the separate lever" and told the reader to step up to medium if findings looked shallow. That conclusion was reached against a knob that was never connected. The measured table stays — it is real, and it is still the evidence that max_tokens is the wrong lever — but the conclusion drawn from it is corrected, with a note to re-measure before moving the value now that the budget is actually enforced.

docs/how-to/reduce-review-cost.md gains a short subsection on the OpenRouter path and the new warning line; docs/llms-full.txt regenerated.

Verification — what is genuinely verified vs mock-asserted

Genuinely verified against real litellm (no mocks):

  • openrouter/vendor/unmapped-model drops reasoning_effort and openai/gpt-4o drops it — both read from litellm's real capability map at test time
  • num_ctx / think are never reported as dropped (real OPENAI_CHAT_COMPLETION_PARAMS)
  • the mechanism is not param-specific — logprobs on anthropic/claude-3-haiku is reported, and nobody wrote a case for it
  • the reasoning object survives litellm's own openrouter transformationtest_the_native_field_survives_litellms_own_transformation runs the built opts through the real litellm.get_optional_params. This matters: OpenrouterConfig.map_openai_params assigns mapped_openai_params["extra_body"] for its own transforms/models/route params, so a future bump that made that assignment clobber rather than merge would put us straight back to a budget that looks sent and never leaves. The test catches that.

Asserted against a mock:

  • the end-to-end CLI test mocks litellm.completion and asserts the kwargs handed to it (the existing tests/cli/test_provider_threading.py seam) — real CLI, real config load, real factory, real engine, fake transport
  • the "litellm forwards it natively, so don't inject" case stubs get_supported_openai_params rather than naming a specific model, so the no-double-send guarantee can't rot into a false pass when a model leaves litellm's map on a dependency bump

Not verified: no live OpenRouter call was made (no key). OpenRouter's acceptance of the field is from its published documentation, not from a 200 response. The wire format is documented in two independent places in their docs and corroborated by the 400 error text quoted above.

test_reasoning_effort_flag_reaches_litellm previously asserted the flat param reached litellm on openrouter/vendor/m — i.e. it asserted the broken behaviour and passed because it stopped one layer above where the drop happens. It now points at ollama, where litellm's map really does forward it.

Coordination

Stayed entirely out of providers/litellm_provider.py — this is request/param assembly, in the factory. #368 owns the response-mapping region (ProviderTruncated, reasoning_tokens) and its truncation message. Worth noting: that message names reasoning_effort as the lever to reach for, which becomes accurate once this lands — today it points at a knob that does nothing on the route this repo actually runs on.

Gate

uv run ruff check . · uv run ruff format --check . · uv run mypy · uv run pytest -q — all green (1838 passed, 3 skipped). uv run pytest tests/specs -q green; openspec validate --specs green. Spec: one new requirement under provider-gateway with a provider.param-support anchor bound to dropped_params.


🤖 Generated with Claude Code


Generated by Claude Code

claude and others added 2 commits August 3, 2026 02:29
… drop a param in silence

`reasoning_effort` was configured and discarded. litellm's openrouter
transformation only adds the param to its supported list when
`supports_reasoning()` is true — i.e. when the model is in litellm's capability
map — and with `drop_params = True` an unmapped model loses it with no warning.
The models a reasoning budget is set for are exactly the ones the map does not
know yet. Measured on this repo: one lens on #367 spent 34,012 reasoning tokens
against a 32,768 ceiling with `reasoning_effort: low` set, and truncated.

Two changes:

- `dropped_params` reads litellm's own capability map and OpenAI-param
  vocabulary to name, once at startup, every configured param the resolved model
  will not accept. Keyed off the map rather than a per-param special case, so a
  param added later is covered; provider-native options (ollama's `num_ctx`) are
  not judged, since they are not litellm's to drop.
- On openrouter only, a budget litellm will not forward is sent as OpenRouter's
  own top-level `reasoning` object via `extra_body`. Never beside the flat param
  — OpenRouter answers a request carrying both with a 400. `default` has no
  equivalent in OpenRouter's effort enum, so it is reported, not translated.

The openrouter branch is a deliberate, narrowly scoped exception to the
litellm-normalises-everything decision in CLAUDE.md, documented as such in the
code — not licence for general per-provider plumbing.

Also corrects `.lgtmaybe.yml`, whose comments concluded `reasoning_effort` was
"the separate lever" against a knob that was never connected. The measured table
stays; the conclusion drawn from it does not.

Refs #348

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@MattJColes
MattJColes merged commit afcc0b8 into main Aug 3, 2026
6 of 7 checks passed
@MattJColes
MattJColes deleted the claude/lgtmaybe-perf-reasoning-effort-3j8akh branch August 3, 2026 02:46

@lgtmaybe lgtmaybe 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.

⚠️ lgtmaybe review failed: review incomplete — every review call failed (ProviderTruncated: spent 33300 of the 32768-token max_tokens ceiling on reasoning, so the batch was not split — a smaller payload cannot shrink a thinking budget; lower reasoning_effort instead). Check the provider credentials/quota, model, and timeout (ollama: a larger model needs a longer --timeout), then retry.

lgtmaybe 1.12.2

@lgtmaybe

lgtmaybe Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

⚠️ lgtmaybe review failed: review incomplete — every review call failed (ProviderTruncated: spent 33300 of the 32768-token max_tokens ceiling on reasoning, so the batch was not split — a smaller payload cannot shrink a thinking budget; lower reasoning_effort instead). Check the provider credentials/quota, model, and timeout (ollama: a larger model needs a longer --timeout), then retry.

lgtmaybe 1.12.2

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.

2 participants