fix(litellm): preserve reasoning_content for known reasoning model families#899
fix(litellm): preserve reasoning_content for known reasoning model families#899daewoongoh wants to merge 4 commits into
Conversation
…milies LiteLLM's /v1/model/info never reports reasoning capability flags, so infer preserveReasoning from the model alias/routed-model name and use convertToR1Format to keep reasoning_content intact across tool-call turns. Signed-off-by: daewoongoh <dw.oh@samsung.com>
…on branching Add unit coverage for LITELLM_PRESERVE_REASONING_PATTERN matching per model family, getLiteLLMModels setting preserveReasoning on matched models, and LiteLLMHandler.createMessage branching between convertToR1Format and convertToOpenAiMessages based on info.preserveReasoning. Signed-off-by: daewoongoh <dw.oh@samsung.com>
📝 WalkthroughWalkthroughLiteLLM model aliases are matched against an explicit reasoning-model allowlist. Matching models receive ChangesLiteLLM reasoning preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant getLiteLLMModels
participant LiteLLMHandler
participant convertToR1Format
participant LiteLLM_API
getLiteLLMModels->>getLiteLLMModels: Match model name and routed alias
getLiteLLMModels-->>LiteLLMHandler: Return preserveReasoning metadata
LiteLLMHandler->>convertToR1Format: Convert messages and merge tool results
convertToR1Format-->>LiteLLMHandler: Return reasoning-preserving messages
LiteLLMHandler->>LiteLLM_API: Send converted conversation
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
edelauna
left a comment
There was a problem hiding this comment.
Thanks for this contribution, had a small comment on test and the regex in use.
| String.raw`kimi-k2\.7-code`, // fireworks.ts (dotted alias variant) | ||
| String.raw`minimax[.-]?m[23](\.\d+)?(-highspeed|-stable)?\b`, // bedrock.ts, minimax.ts, opencode-go.ts | ||
| String.raw`glm-4\.7(?!-flash)\b`, // zai.ts (excludes glm-4.7-flash(x)) | ||
| String.raw`glm-5(\.[12])?(-turbo)?(?!-flash)\b`, // zai.ts, opencode-go.ts |
There was a problem hiding this comment.
Could this still match glm-5 as a prefix for names like glm-5.1-flash or glm-5.3? If those should stay excluded, it may be worth making this fragment consume the accepted suffix fully and adding negative cases for those variants.
There was a problem hiding this comment.
fixed in d57a97a by dropping the regex/prefix approach entirely. isLiteLLMPreserveReasoningModel now does an exact match against LITELLM_PRESERVE_REASONING_MODEL_IDS via Set.has() on the full (lowercased) model id, so glm-5 no longer matches glm-5.1-flash, glm-5.3, etc. Added negative test cases for exactly these variants (glm-5-flash, glm-4.7-flash, glm-4.7-flashx, glm-4.8, etc.) in lite-llm.test.ts.
| data: { | ||
| data: [ | ||
| { | ||
| model_name: "deepseek-reasoner-alias", |
There was a problem hiding this comment.
Could this alias be something neutral that does not match the preserve-reasoning pattern? As written, this still passes if the implementation only checks model_name, so it does not quite prove that litellm_params.model is used.
There was a problem hiding this comment.
fixed in d57a97a — the alias (model_name) is now a neutral string that doesn't match the allowlist on its own (my-deepseek-alias, my-kimi-alias), so this test only passes if litellm_params.model is actually checked. Added a companion test for the reverse case (alias matches, routed model doesn't) to confirm both fields are checked independently.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/providers/lite-llm.ts (1)
325-325: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThread
CompletePromptOptionsinto LiteLLM requests
completePromptdropsabortSignalandtimeoutMs, so callers can’t cancel or time-limit LiteLLM completions. Pass them through tothis.client.chat.completions.create(..., { signal, timeout })and add coverage for abort/timeout behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/lite-llm.ts` at line 325, Update LiteLLMProvider.completePrompt to propagate CompletePromptOptions.abortSignal and timeoutMs into this.client.chat.completions.create as the request signal and timeout, preserving existing completion behavior while enabling cancellation and time limits. Add coverage verifying both abort and timeout options reach the LiteLLM request.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/api/providers/lite-llm.ts`:
- Line 325: Update LiteLLMProvider.completePrompt to propagate
CompletePromptOptions.abortSignal and timeoutMs into
this.client.chat.completions.create as the request signal and timeout,
preserving existing completion behavior while enabling cancellation and time
limits. Add coverage verifying both abort and timeout options reach the LiteLLM
request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ef8d091-a315-4b0e-9a30-01ada5a94cc1
📒 Files selected for processing (1)
src/api/providers/lite-llm.ts
…odel id list Regex-based family matching could over-match unrelated aliases sharing a substring (e.g. glm-5-flash matching a glm-5 prefix), a gap flagged in PR Zoo-Code-Org#899 review. LITELLM_PRESERVE_REASONING_MODEL_IDS now lists the exact model ids that set preserveReasoning: true in their native provider config, matched via isLiteLLMPreserveReasoningModel() against the final segment of model_name/litellm_params.model. Also fixes a test in litellm.spec.ts where the model_name alias itself matched a known id, so the assertion didn't actually prove litellm_params.model was being checked. Signed-off-by: daewoongoh <dw.oh@samsung.com>
Related GitHub Issue
Closes: #891
Description
LiteLLM's
/v1/model/inforesponse never exposes reasoning-related capability flags, sopreserveReasoningwas always resolving tofalsefor reasoning/thinking models routed through a LiteLLM proxy (DeepSeek-Reasoner, GLM-4.7/5, Kimi-K2-Thinking, MiniMax-M2/M3, Qwen3-plus/max, etc.). As a result,LiteLLMHandler.createMessagealways usedconvertToOpenAiMessages, soreasoning_contentfrom a prior turn was never echoed back to the API after a tool-call turn. This differs from accessing the same underlying models directly through their native provider config (e.g.deepseek.tsalready setspreserveReasoning: true), and some providers reject the request with a 400 error when required reasoning history is missing.How:
packages/types/src/providers/lite-llm.ts: Collected the exact alias/routed-model-name ids for every model that requirespreserveReasoning: truein its native provider config (deepseek.ts,mimo.ts,moonshot.ts,bedrock.ts,fireworks.ts,zai.ts,minimax.ts,opencode-go.ts), and exported them asLITELLM_PRESERVE_REASONING_MODEL_IDS.isLiteLLMPreserveReasoningModel()checks a model name against this set with an exact (case-insensitive) match on the final/-delimited segment, so provider-prefixed routed names (e.g.deepseek/deepseek-reasoner,fireworks_ai/accounts/fireworks/models/kimi-k2p7-code) resolve correctly without over-matching unrelated aliases that merely contain a family fragment as a substring (e.g.glm-5-flashvsglm-5).src/api/providers/fetchers/litellm.ts:getLiteLLMModelsnow checks bothmodel_nameandlitellm_params.modelagainstisLiteLLMPreserveReasoningModel()independently while parsing each model, settingModelInfo.preserveReasoning: trueif either matches.src/api/providers/lite-llm.ts:createMessagenow branches oninfo.preserveReasoning—trueroutes throughconvertToR1Format(withmergeToolResultText: true), otherwise it keeps usingconvertToOpenAiMessages.convertToR1Formatfolds any text following a tool_result into the preceding tool message instead of inserting a separate user message, which prevents reasoning-mode providers from droppingreasoning_contentwhen a user message appears mid-turn.Trade-off: the matching is an explicit model-id allowlist, not a query against LiteLLM's actual capabilities (which it doesn't expose). It's exact rather than pattern-based to avoid false positives on similarly-named non-reasoning variants, but that also means unrecognized aliases or renamed deployments simply won't match (documented in the
lite-llm.tscomment).Test Procedure
pnpm vitest run(unit tests):packages/types/src/__tests__/lite-llm.test.ts: verifiesisLiteLLMPreserveReasoningModelmatches every listed model id (including provider-prefixed routed names and case-insensitivity), and does not match unrelated ids or same-family substrings (e.g.glm-5-flash,deepseek-v4-mini).src/api/providers/fetchers/__tests__/litellm.spec.ts: verifiesgetLiteLLMModelssetspreserveReasoning: truewhen either the alias (model_name) or the routed model name (litellm_params.model) matches — with each field tested independently so a match on one alone can't be mistaken for the other being checked — and omits the field otherwise.src/api/providers/__tests__/lite-llm.spec.ts: verifiesconvertToR1Formatis used andreasoning_contentsurvives on the outgoing request whenpreserveReasoning: true; and that the non-preserveReasoningbranch sends noreasoning_content(reflecting thatTask.buildCleanConversationHistoryalready strips the reasoning block upstream in that case).litellm_params.modelrouted to a reasoning model (e.g.deepseek/deepseek-reasoner), run a multi-turn task involving a tool call — previously the follow-up request droppedreasoning_content; it's now preserved.Pre-Submission Checklist
Documentation Updates
Additional Notes
If a reasoning model family or renamed alias isn't caught by this heuristic, additional patterns can be appended to
LITELLM_PRESERVE_REASONING_FRAGMENTS.Get in Touch
hehegwk_23849
Summary by CodeRabbit