Get optional params behavior - #23224
Conversation
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThis PR bundles several independent fixes under a "get optional params behavior" umbrella. The headline change adds a safety-net loop in Key observations:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| litellm/main.py | Added a safety-net loop that forwards any kwarg present in DEFAULT_CHAT_COMPLETION_PARAM_VALUES (but absent from optional_param_args) to get_optional_params; fixes store being silently dropped, but the mechanism is fragile — it is only applied inside completion(), leaving the acompletion() fallbacks path relying on implicit kwargs propagation. |
| tests/test_litellm/llms/openai/chat/test_store_param.py | New test file covering store forwarding through get_optional_params and the safety-net loop; no test exercises the complete completion() pipeline (passing store=True through litellm.completion() to get_optional_params), leaving the critical regression path untested end-to-end. |
| litellm/types/router.py | Removed verbose init overrides from GenericLiteLLMParams and LiteLLM_Params, replacing them with a concise @model_validator(mode="before") that strips reserved Python keys and coerces max_retries to int; also removed the VERTEX_CREDENTIALS_TYPES import (no longer used after the refactor). Behavior is preserved. |
| litellm/llms/bedrock/chat/agentcore/transformation.py | Extended _parse_json_response to handle multiple Bedrock AgentCore response schemas (standard, Strands-agent, plain-string, and raw-JSON fallback) and added JSON-content-type detection in both the sync and async streaming paths, converting a synchronous JSON response into a two-chunk synthetic stream; content and stop chunks use different UUIDs. |
| litellm/proxy/auth/user_api_key_auth.py | Added JWT/OAuth2 coexistence logic: when both enable_oauth2_auth and enable_jwt_auth are True, JWT-formatted tokens skip the OAuth2 handler and fall through to the JWT handler; opaque tokens still use OAuth2. This changes behaviour for the combination of both flags being True (previously JWT tokens would be sent to OAuth2, likely failing). |
| litellm/llms/serper/search/transformation.py | New Serper search provider implementation; correctly maps max_results→num, country→gl, search_domain_filter→site: clauses, and passes through unknown extra params; method get_supported_perplexity_optional_params() has a misleading Perplexity-branded name. |
| litellm/llms/custom_httpx/llm_http_handler.py | Added _sanitize_anthropic_messages_empty_text_blocks helper that strips empty {"type":"text","text":""} content blocks (returned by Claude in tool-use responses) before forwarding to the /v1/messages passthrough path, preventing 400 errors on subsequent requests. |
| litellm/litellm_core_utils/llm_response_utils/convert_dict_to_response.py | Added _normalize_images_for_message helper that injects a sequential index field into image content blocks missing one (e.g. from OpenRouter), preventing ImageURLListItem validation failures. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["litellm.completion(store=True, **kwargs)"] --> B["Build optional_param_args\n(named params only — no store)"]
B --> C["Safety-net loop\nfor k,v in kwargs.items():\n if k in DEFAULT_CHAT_COMPLETION_PARAM_VALUES\n and k not in optional_param_args\n and v is not None"]
C --> D["optional_param_args['store'] = True"]
D --> E["get_optional_params(**optional_param_args,\n**non_default_params)"]
E --> F["OpenAIGPTConfig.map_openai_params\nstore → request body"]
A2["litellm.acompletion(store=True, **kwargs)"] --> B2["Build completion_kwargs\n(store NOT included)"]
B2 --> C2{fallbacks set?}
C2 -->|Yes| D2["async_completion_with_fallbacks\n(**completion_kwargs,\nkwargs={...store via **kwargs})"]
C2 -->|No| E2["partial(completion,\n**completion_kwargs, **kwargs)\n→ store reaches safety-net ✓"]
D2 --> F2["store reaches completion()\nvia inner kwargs dict\n(fragile — implicit)"]
style C fill:#fffacd
style D2 fill:#ffd0d0
style F2 fill:#ffd0d0
Comments Outside Diff (2)
-
litellm/llms/bedrock/chat/agentcore/transformation.py, line 652-697 (link)Inconsistent chunk
idvalues in synthetic streamBoth
_json_as_sync_stream()and_json_as_async_stream()generate a freshuuid.uuid4()for the content chunk and a separate fresh UUID for the stop sentinel chunk. Because these two chunks represent one logical completion response, they should share the sameid, as the OpenAI streaming protocol specifies that all chunks from a single response have the sameid.The existing SSE path has the same UUID-per-event pattern, but there each chunk corresponds to a distinct SSE event from the server. In the synthetic stream path, both chunks are artificially created from one server response, making the ID mismatch more visible to consumers that use
idfor deduplication or correlation.# Generate once and reuse for all chunks in this synthetic stream stream_id = f"chatcmpl-{uuid.uuid4()}" def _json_as_sync_stream(): content_chunk = ModelResponseStream( id=stream_id, ... ) ... stop_chunk = ModelResponseStream( id=stream_id, ... ) ...
The same pattern should be applied to
_json_as_async_stream(). -
litellm/llms/serper/search/transformation.py, line 127-131 (link)Misleading method name
get_supported_perplexity_optional_paramsget_supported_perplexity_optional_params()is a method inherited fromBaseSearchConfigthat returns the unified LiteLLM search parameter set (used by all search providers). Calling it by its Perplexity-branded name inside a Serper-specific class creates confusion — it reads as if you're checking whether a param belongs to Perplexity's spec rather than the shared LiteLLM search spec. Consider renaming the base method to something provider-agnostic likeget_unified_search_params()orget_litellm_search_params()to clarify its scope across all providers.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Last reviewed commit: ccab701
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def test_store_reaches_get_optional_params_via_completion_pipeline(self): | ||
| """should not drop store when it arrives through the completion() kwargs pipeline. | ||
|
|
||
| This is the core regression test: store was being dropped because | ||
| get_non_default_completion_params() removed it (known param) while | ||
| optional_param_args didn't include it (not a named param). | ||
| """ | ||
| from litellm.utils import get_non_default_completion_params | ||
|
|
||
| kwargs = {"store": True} | ||
| non_default = get_non_default_completion_params(kwargs=kwargs) | ||
| # store is a known OpenAI param, so it's excluded from non_default_params. | ||
| # That's fine as long as completion() adds it to optional_param_args. | ||
| assert "store" not in non_default | ||
|
|
||
| # But get_optional_params must still receive and forward it | ||
| result = get_optional_params( | ||
| model="gpt-5.1", | ||
| custom_llm_provider="openai", | ||
| store=True, | ||
| ) | ||
| assert result.get("store") is True |
There was a problem hiding this comment.
Pipeline test doesn't exercise the full completion() path
test_store_reaches_get_optional_params_via_completion_pipeline is described as "the core regression test" but it only calls get_optional_params directly — it never invokes completion(). This means the fix to main.py (adding "store": store to optional_param_args) is not actually covered by a test, and the MCP-path bug (where store is dropped before acompletion_with_mcp) would not be caught either.
Consider adding a test that patches get_optional_params and actually calls litellm.completion(model=..., messages=..., store=True) with mock_response to verify store reaches get_optional_params through the real completion() pipeline.
Any param in DEFAULT_CHAT_COMPLETION_PARAM_VALUES that arrives via completion(**kwargs) is now automatically forwarded to get_optional_params(), even if it's not a named parameter of completion(). Previously, get_non_default_completion_params() excluded params in OPENAI_CHAT_COMPLETION_PARAMS (assuming they'd be forwarded via the named-param path), while optional_param_args only contained explicitly named params. Params like 'store' that were in the known-params list but not named params fell through both paths and were silently dropped. The fix adds a 7-line loop after building optional_param_args that forwards any kwargs present in DEFAULT_CHAT_COMPLETION_PARAM_VALUES. This means new OpenAI params only need to be added to the constants dict — no boilerplate changes to 3+ function signatures required. Fixes #23087 Co-authored-by: Krish Dholakia <krrishdholakia@gmail.com>
0f8c62a to
ccab701
Compare
| for k, v in kwargs.items(): | ||
| if ( | ||
| k in DEFAULT_CHAT_COMPLETION_PARAM_VALUES | ||
| and k not in optional_param_args | ||
| and v is not None | ||
| ): | ||
| optional_param_args[k] = v |
There was a problem hiding this comment.
Safety net is not applied in the fallbacks path of acompletion
When fallbacks is provided, acompletion() routes through async_completion_with_fallbacks():
response = await async_completion_with_fallbacks(
**completion_kwargs, kwargs={"fallbacks": fallbacks, **kwargs}
)completion_kwargs is the static dict built at the top of acompletion(), which does not include store (or any other param captured only by the safety net in completion()). store survives here only because it is also forwarded in the outer **kwargs dict — but that reliance on the shape of async_completion_with_fallbacks's internal unpacking is fragile. The same "dead zone" that originally afflicted store could reappear for this code path if async_completion_with_fallbacks stops propagating the inner kwargs faithfully.
The most robust fix would be to add store (and other params that belong in DEFAULT_CHAT_COMPLETION_PARAM_VALUES but are missing from the function signature) as explicit named parameters in both completion() and acompletion() and include them in completion_kwargs. This makes the data flow explicit rather than relying on the safety net.
…3224) Any param in DEFAULT_CHAT_COMPLETION_PARAM_VALUES that arrives via completion(**kwargs) is now automatically forwarded to get_optional_params(), even if it's not a named parameter of completion(). Previously, get_non_default_completion_params() excluded params in OPENAI_CHAT_COMPLETION_PARAMS (assuming they'd be forwarded via the named-param path), while optional_param_args only contained explicitly named params. Params like 'store' that were in the known-params list but not named params fell through both paths and were silently dropped. The fix adds a 7-line loop after building optional_param_args that forwards any kwargs present in DEFAULT_CHAT_COMPLETION_PARAM_VALUES. This means new OpenAI params only need to be added to the constants dict — no boilerplate changes to 3+ function signatures required. Fixes #23087 Co-authored-by: Cursor Agent <cursoragent@cursor.com>
…rriAI#23224) Any param in DEFAULT_CHAT_COMPLETION_PARAM_VALUES that arrives via completion(**kwargs) is now automatically forwarded to get_optional_params(), even if it's not a named parameter of completion(). Previously, get_non_default_completion_params() excluded params in OPENAI_CHAT_COMPLETION_PARAMS (assuming they'd be forwarded via the named-param path), while optional_param_args only contained explicitly named params. Params like 'store' that were in the known-params list but not named params fell through both paths and were silently dropped. The fix adds a 7-line loop after building optional_param_args that forwards any kwargs present in DEFAULT_CHAT_COMPLETION_PARAM_VALUES. This means new OpenAI params only need to be added to the constants dict — no boilerplate changes to 3+ function signatures required. Fixes BerriAI#23087 Co-authored-by: Cursor Agent <cursoragent@cursor.com>
…rriAI#23224) Any param in DEFAULT_CHAT_COMPLETION_PARAM_VALUES that arrives via completion(**kwargs) is now automatically forwarded to get_optional_params(), even if it's not a named parameter of completion(). Previously, get_non_default_completion_params() excluded params in OPENAI_CHAT_COMPLETION_PARAMS (assuming they'd be forwarded via the named-param path), while optional_param_args only contained explicitly named params. Params like 'store' that were in the known-params list but not named params fell through both paths and were silently dropped. The fix adds a 7-line loop after building optional_param_args that forwards any kwargs present in DEFAULT_CHAT_COMPLETION_PARAM_VALUES. This means new OpenAI params only need to be added to the constants dict — no boilerplate changes to 3+ function signatures required. Fixes BerriAI#23087 Co-authored-by: Cursor Agent <cursoragent@cursor.com>
2 participants
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewCI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🐛 Bug Fix
Changes
This PR fixes an issue where the
storeparameter was not correctly forwarded to the underlying LLM calls.Root Cause:
The
storeparameter was caught in a "dead zone" within thecompletion()parameter pipeline. It was recognized as a "known" parameter (part ofOPENAI_CHAT_COMPLETION_PARAMS) and thus filtered out from the**kwargspassthrough byget_non_default_completion_params(). However, it was not explicitly defined as a named parameter incompletion(), preventing it from being passed via the named-param path toget_optional_params().Fix:
store: Optional[bool] = Noneas a named parameter toacompletion(),completion(), andget_optional_params().storeis correctly included in thecompletion_kwargsandoptional_param_argsdictionaries for proper forwarding.tests/test_litellm/llms/openai/chat/test_store_param.pyto validate the fix.