From 8d5db4f712cf94eeacee130eb3557b910155096d Mon Sep 17 00:00:00 2001 From: jtsaw <166962251+jtsaw@users.noreply.github.com> Date: Tue, 17 Feb 2026 21:10:50 -0800 Subject: [PATCH 1/3] fix handling of ResponseApplyPatchToolCall in completion bridge (#20913) * fix handling of ResponseApplyPatchToolCall in completion bridge * refactor * style: fix black formatting * fix: clean up lint errors in test file (unused imports, print statements, formatting) * refactor: extract _map_optional_params_to_responses_api to fix PLR0915 * what * this linter cannot be me * revert cause idk what's going on * weird * idk why this got removed * revert more stuff * revert pt 3 --- .../transformation.py | 19 +- .../transformation.py | 77 +++++--- ...responses_transformation_transformation.py | 174 ++++++++++++++++-- 3 files changed, 225 insertions(+), 45 deletions(-) diff --git a/litellm/completion_extras/litellm_responses_transformation/transformation.py b/litellm/completion_extras/litellm_responses_transformation/transformation.py index e546a0dbb0..5de9a48985 100644 --- a/litellm/completion_extras/litellm_responses_transformation/transformation.py +++ b/litellm/completion_extras/litellm_responses_transformation/transformation.py @@ -401,6 +401,7 @@ class LiteLLMResponsesTransformationHandler(CompletionTransformationBridge): ResponseOutputMessage, ResponseReasoningItem, ) + from openai.types.responses.response_output_item import ResponseApplyPatchToolCall from litellm.types.utils import Choices, Message @@ -457,6 +458,18 @@ class LiteLLMResponsesTransformationHandler(CompletionTransformationBridge): accumulated_tool_calls.append(tool_call_dict) tool_call_index += 1 + elif isinstance(item, ResponseApplyPatchToolCall): + from litellm.responses.litellm_completion_transformation.transformation import ( + LiteLLMCompletionResponsesConfig, + ) + + tool_call_dict = LiteLLMCompletionResponsesConfig.convert_apply_patch_tool_call_to_chat_completion_tool_call( + tool_call_item=item, + index=tool_call_index, + ) + accumulated_tool_calls.append(tool_call_dict) + tool_call_index += 1 + elif isinstance(item, dict) and handle_raw_dict_callback is not None: # Handle raw dict responses (e.g., from GPT-5 Codex) choice, index = handle_raw_dict_callback(item=item, index=index) @@ -533,7 +546,7 @@ class LiteLLMResponsesTransformationHandler(CompletionTransformationBridge): raw_response.usage ), ) - + # Preserve hidden params from the ResponsesAPIResponse, especially the headers # which contain important provider information like x-request-id raw_response_hidden_params = getattr(raw_response, "_hidden_params", {}) @@ -550,7 +563,7 @@ class LiteLLMResponsesTransformationHandler(CompletionTransformationBridge): model_response._hidden_params[key] = merged_headers else: model_response._hidden_params[key] = value - + return model_response def get_model_response_iterator( @@ -855,7 +868,7 @@ class LiteLLMResponsesTransformationHandler(CompletionTransformationBridge): return {"format": {"type": "text"}} return None - + @staticmethod def _convert_annotations_to_chat_format( annotations: Optional[List[Any]], diff --git a/litellm/responses/litellm_completion_transformation/transformation.py b/litellm/responses/litellm_completion_transformation/transformation.py index b8379b28c3..8daa8e49d1 100644 --- a/litellm/responses/litellm_completion_transformation/transformation.py +++ b/litellm/responses/litellm_completion_transformation/transformation.py @@ -291,14 +291,14 @@ class LiteLLMCompletionResponsesConfig: ) _messages = litellm_completion_request.get("messages") or [] session_messages = chat_completion_session.get("messages") or [] - + # If session messages are empty (e.g., no database in test environment), # we still need to process the new input messages # Store original _messages before combining for safety check original_new_messages = _messages.copy() if _messages else [] - + combined_messages = session_messages + _messages - + # Fix: Ensure tool_results have corresponding tool_calls in previous assistant message # Pass tools parameter to help reconstruct tool_calls if not in cache tools = litellm_completion_request.get("tools") or [] @@ -306,7 +306,7 @@ class LiteLLMCompletionResponsesConfig: messages=combined_messages, tools=tools ) - + # Safety check: Ensure we don't end up with empty messages # This can happen when using previous_response_id without a database (e.g., in tests) # and session messages are empty but new input messages exist @@ -337,7 +337,7 @@ class LiteLLMCompletionResponsesConfig: model=litellm_completion_request.get("model", ""), llm_provider=litellm_completion_request.get("custom_llm_provider", ""), ) - + litellm_completion_request["messages"] = combined_messages litellm_completion_request["litellm_trace_id"] = chat_completion_session.get( "litellm_session_id" @@ -385,8 +385,8 @@ class LiteLLMCompletionResponsesConfig: ######################################################### # If Input Item is a Tool Call Output, add it to the tool_call_output_messages list - # preserving the ordering of tool call outputs. Some models require the tool - # result to immediately follow the assistant tool call. + # preserving the ordering of tool call outputs. Some models require the tool + # result to immediately follow the assistant tool call. ######################################################### if LiteLLMCompletionResponsesConfig._is_input_item_tool_call_output( input_item=_input @@ -743,47 +743,47 @@ class LiteLLMCompletionResponsesConfig: ) -> List[Union[AllMessageValues, GenericChatCompletionMessage, ChatCompletionResponseMessage]]: """ Ensure that tool_result messages have corresponding tool_calls in the previous assistant message. - + This is critical for Anthropic API which requires that each tool_result block has a corresponding tool_use block in the previous assistant message. - + Args: messages: List of messages that may include tool_result messages tools: Optional list of tools that can be used to reconstruct tool_calls if not in cache - + Returns: List of messages with tool_calls added to assistant messages when needed """ if not messages: return messages - + # Create a deep copy to avoid modifying the original import copy fixed_messages = copy.deepcopy(messages) messages_to_remove = [] - + # Count non-tool messages to avoid removing all messages # This prevents empty messages list when using previous_response_id without a database non_tool_messages_count = sum( 1 for msg in fixed_messages if msg.get("role") != "tool" ) - + for i, message in enumerate(fixed_messages): # Only process tool messages - check role first to narrow the type if message.get("role") != "tool": continue - + # At this point, we know it's a tool message, so it should have tool_call_id # Use get() with default to safely access tool_call_id tool_call_id_raw = message.get("tool_call_id") if isinstance(message, dict) else getattr(message, "tool_call_id", None) tool_call_id: str = ( str(tool_call_id_raw) if tool_call_id_raw is not None else "" ) - + prev_assistant_idx = LiteLLMCompletionResponsesConfig._find_previous_assistant_idx( fixed_messages, i ) - + # Try to recover empty tool_call_id from previous assistant message if not tool_call_id and prev_assistant_idx is not None: prev_assistant = fixed_messages[prev_assistant_idx] @@ -798,7 +798,7 @@ class LiteLLMCompletionResponsesConfig: message_dict["tool_call_id"] = tool_call_id elif hasattr(message, "tool_call_id"): setattr(message, "tool_call_id", tool_call_id) - + # Only remove messages with empty tool_call_id if we have other non-tool messages # This prevents ending up with an empty messages list when using previous_response_id # without a database (e.g., in tests where session messages are empty) @@ -810,7 +810,7 @@ class LiteLLMCompletionResponsesConfig: # If no non-tool messages, keep the tool message even with empty call_id # The API will return a proper error message about the missing tool_use block continue - + # Check if the previous assistant message has the corresponding tool_call # This needs to run for ALL tool messages with a valid tool_call_id, # not just those that had an empty tool_call_id initially @@ -819,12 +819,12 @@ class LiteLLMCompletionResponsesConfig: tool_calls = LiteLLMCompletionResponsesConfig._get_tool_calls_list( prev_assistant ) - + if not LiteLLMCompletionResponsesConfig._check_tool_call_exists( tool_calls, tool_call_id ): _tool_use_definition = TOOL_CALLS_CACHE.get_cache(key=tool_call_id) - + if not _tool_use_definition and tools: _tool_use_definition = ( LiteLLMCompletionResponsesConfig._reconstruct_tool_call_from_tools( @@ -849,11 +849,11 @@ class LiteLLMCompletionResponsesConfig: LiteLLMCompletionResponsesConfig._add_tool_call_to_assistant( prev_assistant, tool_call_chunk ) - + # Remove messages with empty tool_call_id that couldn't be fixed for idx in reversed(messages_to_remove): fixed_messages.pop(idx) - + return fixed_messages @staticmethod @@ -1454,6 +1454,39 @@ class LiteLLMCompletionResponsesConfig: return tool_call_dict + @staticmethod + def convert_apply_patch_tool_call_to_chat_completion_tool_call( + tool_call_item: Any, + index: int = 0, + ) -> Dict[str, Any]: + """ + Convert ResponseApplyPatchToolCall to ChatCompletionToolCallChunk format. + + The operation (create_file / update_file / delete_file) is serialised + as JSON so it appears in function.arguments, just like any other + tool call. + + Args: + tool_call_item: ResponseApplyPatchToolCall object with call_id and operation + index: The index of this tool call + + Returns: + Dictionary in ChatCompletionToolCallChunk format + """ + import json + + operation_dict = tool_call_item.operation.model_dump() + tool_call_dict: Dict[str, Any] = { + "id": tool_call_item.call_id, + "function": { + "name": "apply_patch", + "arguments": json.dumps(operation_dict), + }, + "type": "function", + "index": index, + } + return tool_call_dict + @staticmethod def transform_chat_completion_response_to_responses_api_response( request_input: Union[str, ResponseInputParam], diff --git a/tests/test_litellm/completion_extras/litellm_responses_transformation/test_completion_extras_litellm_responses_transformation_transformation.py b/tests/test_litellm/completion_extras/litellm_responses_transformation/test_completion_extras_litellm_responses_transformation_transformation.py index f8a082ee30..25e8a1f330 100644 --- a/tests/test_litellm/completion_extras/litellm_responses_transformation/test_completion_extras_litellm_responses_transformation_transformation.py +++ b/tests/test_litellm/completion_extras/litellm_responses_transformation/test_completion_extras_litellm_responses_transformation_transformation.py @@ -1012,11 +1012,11 @@ def test_multiple_tool_calls_in_single_choice(): def test_map_reasoning_effort_adds_summary_detailed(): """ Test that _map_reasoning_effort behavior with reasoning_auto_summary flag. - + By default (flag=False), summary should NOT be added to avoid: 1. Breaking for users without verified OpenAI orgs (400 errors) 2. Making requests more expensive by including summary reasoning tokens - + When flag is enabled (flag=True or env var), summary="detailed" is added. """ import os @@ -1030,64 +1030,64 @@ def test_map_reasoning_effort_adds_summary_detailed(): # Test all string effort levels - DEFAULT BEHAVIOR (no summary) effort_levels = ["none", "low", "medium", "high", "xhigh", "minimal"] - + # Save original flag value original_flag = litellm.reasoning_auto_summary original_env = os.environ.get("LITELLM_REASONING_AUTO_SUMMARY") - + try: # Test 1: Default behavior (flag=False, no env var) - NO summary litellm.reasoning_auto_summary = False if "LITELLM_REASONING_AUTO_SUMMARY" in os.environ: del os.environ["LITELLM_REASONING_AUTO_SUMMARY"] - + for effort in effort_levels: result = handler._map_reasoning_effort(effort) - + assert result is not None, f"Result should not be None for effort={effort}" assert result["effort"] == effort, f"Effort should be {effort}" assert "summary" not in result, f"Summary should NOT be present by default for effort={effort}" - + print(f"✓ reasoning_effort='{effort}' correctly maps to effort='{effort}' (no summary by default)") - + # Test 2: With flag enabled - summary IS added litellm.reasoning_auto_summary = True - + for effort in effort_levels: result = handler._map_reasoning_effort(effort) - + assert result is not None, f"Result should not be None for effort={effort}" assert result["effort"] == effort, f"Effort should be {effort}" assert result["summary"] == "detailed", f"Summary should be 'detailed' when flag is enabled for effort={effort}" - + print(f"✓ reasoning_effort='{effort}' correctly maps to effort='{effort}', summary='detailed' (flag enabled)") - + # Test 3: With env var enabled (flag disabled) - summary IS added litellm.reasoning_auto_summary = False os.environ["LITELLM_REASONING_AUTO_SUMMARY"] = "true" - + result = handler._map_reasoning_effort("high") assert result["summary"] == "detailed", "Summary should be 'detailed' when env var is enabled" print("✓ LITELLM_REASONING_AUTO_SUMMARY env var works correctly") - + # Test 4: Dict input is passed through as-is (no modification) litellm.reasoning_auto_summary = False if "LITELLM_REASONING_AUTO_SUMMARY" in os.environ: del os.environ["LITELLM_REASONING_AUTO_SUMMARY"] - + dict_input = {"effort": "high", "summary": "custom_summary"} result_dict = handler._map_reasoning_effort(dict_input) assert result_dict["effort"] == "high" assert result_dict["summary"] == "custom_summary" print("✓ Dict input is passed through without modification") - + # Test 5: None/unknown values return None result_unknown = handler._map_reasoning_effort("unknown_value") assert result_unknown is None print("✓ Unknown reasoning_effort values return None") - + print("✓ All reasoning_effort behaviors work correctly with flag/env var control") - + finally: # Restore original values litellm.reasoning_auto_summary = original_flag @@ -1100,10 +1100,10 @@ def test_map_reasoning_effort_adds_summary_detailed(): def test_transform_response_preserves_annotations(): """ Test that annotations from Responses API are preserved when transforming to Chat Completions format. - + This is a regression test for the bug where annotations (like url_citation) were being dropped during the transformation from ResponsesAPIResponse to ModelResponse. - + The fix ensures annotations are extracted from ResponseOutputText content items and passed through to the Message object in the Chat Completions response. """ @@ -1278,3 +1278,137 @@ def test_transform_response_preserves_annotations(): assert result.usage.total_tokens == 30 print("✓ Annotations from Responses API are correctly preserved in Chat Completions format") + + +def test_apply_patch_tool_call_converted_to_chat_completion_tool_call(): + """ + Test that ResponseApplyPatchToolCall items from the Responses API are + correctly converted to ChatCompletions-style tool calls by the bridge. + + This is a regression test for a bug where litellm.completion() with a + responses/ model prefix crashed when the model returned an + apply_patch_call, because _convert_response_output_to_choices did not + handle ResponseApplyPatchToolCall items. The model DID use the tool, + but the bridge silently dropped it (or raised an error), while the + native litellm.responses() path worked correctly. + """ + import json + from unittest.mock import Mock + + from openai.types.responses.response_apply_patch_tool_call import ( + OperationCreateFile, + ) + from openai.types.responses.response_output_item import ( + ResponseApplyPatchToolCall, + ) + + from litellm.completion_extras.litellm_responses_transformation.transformation import ( + LiteLLMResponsesTransformationHandler, + ) + from litellm.types.llms.openai import ( + InputTokensDetails, + OutputTokensDetails, + ResponseAPIUsage, + ResponsesAPIResponse, + ) + from litellm.types.utils import ModelResponse, Usage + + handler = LiteLLMResponsesTransformationHandler() + + # Build an apply_patch_call item like the model would return + operation = OperationCreateFile( + diff="--- /dev/null\n+++ b/hello.py\n@@ -0,0 +1 @@\n+print('hello world')\n", + path="hello.py", + type="create_file", + ) + apply_patch_item = ResponseApplyPatchToolCall( + id="apc_001", + call_id="call_patch_hello", + operation=operation, + status="completed", + type="apply_patch_call", + ) + + # Minimal usage + usage = ResponseAPIUsage( + input_tokens=30, + input_tokens_details=InputTokensDetails(cached_tokens=0), + output_tokens=40, + output_tokens_details=OutputTokensDetails(reasoning_tokens=0), + total_tokens=70, + ) + + raw_response = ResponsesAPIResponse( + id="resp_apply_patch_test", + created_at=1234567890, + error=None, + incomplete_details=None, + instructions=None, + metadata={}, + model="gpt-5.2-codex", + object="response", + output=[apply_patch_item], + parallel_tool_calls=True, + temperature=1.0, + tool_choice="auto", + tools=[], + top_p=1.0, + max_output_tokens=None, + previous_response_id=None, + reasoning=None, + status="completed", + text=None, + truncation="disabled", + usage=usage, + user=None, + store=True, + background=False, + ) + + model_response = ModelResponse( + id="chatcmpl-apply-patch", + created=1234567890, + model=None, + object="chat.completion", + choices=[], + usage=Usage(completion_tokens=0, prompt_tokens=0, total_tokens=0), + ) + + logging_obj = Mock() + + result = handler.transform_response( + model="gpt-5.2-codex", + raw_response=raw_response, + model_response=model_response, + logging_obj=logging_obj, + request_data={"model": "gpt-5.2-codex"}, + messages=[ + {"role": "system", "content": "You are a coding assistant."}, + {"role": "user", "content": "Create hello.py"}, + ], + optional_params={}, + litellm_params={}, + encoding=Mock(), + ) + + # Should have exactly one choice with finish_reason="tool_calls" + assert len(result.choices) == 1, f"Expected 1 choice, got {len(result.choices)}" + + choice = result.choices[0] + assert choice.finish_reason == "tool_calls" + + # The choice should contain one tool call for apply_patch + tool_calls = choice.message.tool_calls + assert tool_calls is not None, "tool_calls should not be None" + assert len(tool_calls) == 1, f"Expected 1 tool_call, got {len(tool_calls)}" + + tc = tool_calls[0] + assert tc["id"] == "call_patch_hello" + assert tc["type"] == "function" + assert tc["function"]["name"] == "apply_patch" + + # The operation should be serialised as JSON in arguments + args = json.loads(tc["function"]["arguments"]) + assert args["type"] == "create_file" + assert args["path"] == "hello.py" + assert "print('hello world')" in args["diff"] From ae613b2d36f92a700077884234b0076af67cfb85 Mon Sep 17 00:00:00 2001 From: Atharva Jaiswal <92455570+AtharvaJaiswal005@users.noreply.github.com> Date: Wed, 18 Feb 2026 12:28:08 +0530 Subject: [PATCH 2/3] fix(router): break retry loop on non-retryable errors (#21370) The retry loop in async_function_with_retries catches all exceptions blindly and continues retrying even for non-retryable errors like 400 ContextWindowExceeded or 404 NotFoundError. This causes the original retryable error to be raised instead of the actual non-retryable one. Changes: - Update original_exception to latest error on each retry attempt - Add should_retry_this_error() check inside the retry loop to break out immediately on non-retryable errors - Respect _retry_policy_applies precedence Fixes #21343 --- litellm/router.py | 22 ++ .../test_router_retry_non_retryable_errors.py | 251 ++++++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 tests/test_litellm/test_router_retry_non_retryable_errors.py diff --git a/litellm/router.py b/litellm/router.py index 888c97ca0b..3fac761ce6 100644 --- a/litellm/router.py +++ b/litellm/router.py @@ -5149,6 +5149,10 @@ class Router: return response except Exception as e: + # Always track the latest error so we raise the most + # recent exception instead of the first one. + original_exception = e + ## LOGGING kwargs = self.log_retry(kwargs=kwargs, e=e) remaining_retries = num_retries - current_attempt - 1 @@ -5163,6 +5167,24 @@ class Router: ) else: _healthy_deployments = [] + + # Check if this error is non-retryable (e.g., 400 context + # window exceeded). If so, raise immediately instead of + # continuing the retry loop. Respect retry policy + # precedence - only check when no retry policy applies. + if not _retry_policy_applies: + try: + self.should_retry_this_error( + error=e, + healthy_deployments=_healthy_deployments, + all_deployments=_all_deployments, + context_window_fallbacks=context_window_fallbacks, + regular_fallbacks=fallbacks, + content_policy_fallbacks=content_policy_fallbacks, + ) + except Exception: + raise e + _timeout = self._time_to_sleep_before_retry( e=e, remaining_retries=remaining_retries, diff --git a/tests/test_litellm/test_router_retry_non_retryable_errors.py b/tests/test_litellm/test_router_retry_non_retryable_errors.py new file mode 100644 index 0000000000..20a1c979a0 --- /dev/null +++ b/tests/test_litellm/test_router_retry_non_retryable_errors.py @@ -0,0 +1,251 @@ +""" +Test that the Router retry loop correctly handles non-retryable errors. + +Verifies that: +1. Non-retryable errors (e.g., 400 ContextWindowExceeded) inside the retry loop + break out immediately instead of being swallowed. +2. original_exception is updated to the latest error, not stuck on the first. +3. Retryable errors (e.g., 429 RateLimitError) still retry normally. + +Regression tests for https://github.com/BerriAI/litellm/issues/21343 +""" + +from unittest.mock import AsyncMock, patch + +import pytest + +import litellm +from litellm import Router + + +def _make_rate_limit_error(message="Rate limited"): + """Create a RateLimitError for testing.""" + return litellm.RateLimitError( + message=message, + llm_provider="bedrock", + model="anthropic.claude-v2", + ) + + +def _make_context_window_error(message="prompt is too long: 1205821 tokens > 200000"): + """Create a ContextWindowExceededError for testing.""" + return litellm.ContextWindowExceededError( + message=message, + llm_provider="vertex_ai", + model="claude-3-opus", + ) + + +def _make_bad_request_error(message="Invalid request"): + """Create a BadRequestError for testing.""" + return litellm.BadRequestError( + message=message, + llm_provider="openai", + model="gpt-4", + ) + + +def _make_not_found_error(message="Model not found"): + """Create a NotFoundError for testing.""" + return litellm.NotFoundError( + message=message, + llm_provider="openai", + model="gpt-99", + ) + + +def _create_router(num_retries=2): + """Create a Router with two deployments for testing.""" + return Router( + model_list=[ + { + "model_name": "test-model", + "litellm_params": { + "model": "openai/gpt-4", + "api_key": "fake-key-1", + }, + }, + { + "model_name": "test-model", + "litellm_params": { + "model": "openai/gpt-4", + "api_key": "fake-key-2", + }, + }, + ], + num_retries=num_retries, + ) + + +def _base_kwargs(): + """Return kwargs required by async_function_with_retries.""" + return { + "model": "test-model", + "messages": [{"role": "user", "content": "test"}], + "original_function": AsyncMock(), + "metadata": {}, + } + + +@pytest.mark.asyncio +async def test_non_retryable_error_in_retry_loop_raises_immediately(): + """ + When a non-retryable error (400 ContextWindowExceeded) occurs inside the + retry loop, the router should raise it immediately instead of swallowing it + and raising the original error. + + Scenario: First call -> 429, Retry -> 400 (non-retryable) + Expected: ContextWindowExceededError is raised, NOT RateLimitError + """ + router = _create_router(num_retries=2) + + rate_limit_error = _make_rate_limit_error() + context_window_error = _make_context_window_error() + + call_count = 0 + + async def mock_make_call(*args, **kwargs): + nonlocal call_count + call_count += 1 + if call_count == 1: + raise rate_limit_error + else: + raise context_window_error + + with patch.object(router, "make_call", side_effect=mock_make_call), \ + patch.object(router, "_async_get_healthy_deployments", + return_value=(["d1", "d2"], ["d1", "d2"])), \ + patch.object(router, "_time_to_sleep_before_retry", return_value=0), \ + patch.object(router, "log_retry", side_effect=lambda kwargs, e: kwargs): + with pytest.raises(litellm.ContextWindowExceededError): + await router.async_function_with_retries( + num_retries=2, + **_base_kwargs(), + ) + + +@pytest.mark.asyncio +async def test_bad_request_error_in_retry_loop_raises_immediately(): + """ + A generic 400 BadRequestError inside the retry loop should also break out + immediately since 400 is not retryable. + """ + router = _create_router(num_retries=2) + + rate_limit_error = _make_rate_limit_error() + bad_request_error = _make_bad_request_error() + + call_count = 0 + + async def mock_make_call(*args, **kwargs): + nonlocal call_count + call_count += 1 + if call_count == 1: + raise rate_limit_error + else: + raise bad_request_error + + with patch.object(router, "make_call", side_effect=mock_make_call), \ + patch.object(router, "_async_get_healthy_deployments", + return_value=(["d1", "d2"], ["d1", "d2"])), \ + patch.object(router, "_time_to_sleep_before_retry", return_value=0), \ + patch.object(router, "log_retry", side_effect=lambda kwargs, e: kwargs): + with pytest.raises(litellm.BadRequestError): + await router.async_function_with_retries( + num_retries=2, + **_base_kwargs(), + ) + + +@pytest.mark.asyncio +async def test_original_exception_updated_to_latest_error(): + """ + When all retries are exhausted with retryable errors, the LAST error + should be raised, not the first one. + """ + router = _create_router(num_retries=2) + + call_count = 0 + + async def mock_make_call(*args, **kwargs): + nonlocal call_count + call_count += 1 + raise _make_rate_limit_error(f"Rate limit attempt {call_count}") + + with patch.object(router, "make_call", side_effect=mock_make_call), \ + patch.object(router, "_async_get_healthy_deployments", + return_value=(["d1", "d2"], ["d1", "d2"])), \ + patch.object(router, "_time_to_sleep_before_retry", return_value=0), \ + patch.object(router, "log_retry", side_effect=lambda kwargs, e: kwargs): + with pytest.raises(litellm.RateLimitError) as exc_info: + await router.async_function_with_retries( + num_retries=2, + **_base_kwargs(), + ) + # Should be the LAST error, not the first + assert "Rate limit attempt 3" in str(exc_info.value) + + +@pytest.mark.asyncio +async def test_retryable_errors_still_retry_normally(): + """ + Retryable errors (429 RateLimitError) should still be retried the + configured number of times before raising. + """ + router = _create_router(num_retries=3) + + call_count = 0 + + async def mock_make_call(*args, **kwargs): + nonlocal call_count + call_count += 1 + raise _make_rate_limit_error(f"Rate limit attempt {call_count}") + + with patch.object(router, "make_call", side_effect=mock_make_call), \ + patch.object(router, "_async_get_healthy_deployments", + return_value=(["d1", "d2"], ["d1", "d2"])), \ + patch.object(router, "_time_to_sleep_before_retry", return_value=0), \ + patch.object(router, "log_retry", side_effect=lambda kwargs, e: kwargs): + with pytest.raises(litellm.RateLimitError): + await router.async_function_with_retries( + num_retries=3, + **_base_kwargs(), + ) + + # Initial call + 3 retries = 4 total calls + assert call_count == 4 + + +@pytest.mark.asyncio +async def test_not_found_error_in_retry_loop_raises_immediately(): + """ + A 404 NotFoundError inside the retry loop should break out immediately. + """ + router = _create_router(num_retries=2) + + rate_limit_error = _make_rate_limit_error() + not_found_error = _make_not_found_error() + + call_count = 0 + + async def mock_make_call(*args, **kwargs): + nonlocal call_count + call_count += 1 + if call_count == 1: + raise rate_limit_error + else: + raise not_found_error + + with patch.object(router, "make_call", side_effect=mock_make_call), \ + patch.object(router, "_async_get_healthy_deployments", + return_value=(["d1", "d2"], ["d1", "d2"])), \ + patch.object(router, "_time_to_sleep_before_retry", return_value=0), \ + patch.object(router, "log_retry", side_effect=lambda kwargs, e: kwargs): + with pytest.raises(litellm.NotFoundError): + await router.async_function_with_retries( + num_retries=2, + **_base_kwargs(), + ) + + # Only 2 calls: initial + first retry that hits non-retryable + assert call_count == 2 From 42afba9cdd3ec78270c84da0e6e915d158105434 Mon Sep 17 00:00:00 2001 From: Atharva Jaiswal <92455570+AtharvaJaiswal005@users.noreply.github.com> Date: Wed, 18 Feb 2026 12:29:01 +0530 Subject: [PATCH 3/3] Fix invalid OpenAPI schema for /spend/calculate and /credentials endpoints (#21369) - /spend/calculate: wrap response in proper OpenAPI 3.x content structure - /credentials: split stacked route decorators into separate handlers to eliminate path parameter conflict between by_name and by_model routes --- .../proxy/credential_endpoints/endpoints.py | 97 ++++++------ .../spend_management_endpoints.py | 20 ++- .../proxy/test_openapi_schema_validation.py | 142 ++++++++++++++++++ 3 files changed, 209 insertions(+), 50 deletions(-) create mode 100644 tests/test_litellm/proxy/test_openapi_schema_validation.py diff --git a/litellm/proxy/credential_endpoints/endpoints.py b/litellm/proxy/credential_endpoints/endpoints.py index 9f228bb118..5fa9546e00 100644 --- a/litellm/proxy/credential_endpoints/endpoints.py +++ b/litellm/proxy/credential_endpoints/endpoints.py @@ -142,17 +142,47 @@ async def get_credentials( tags=["credential management"], response_model=CredentialItem, ) +async def get_credential_by_name( + request: Request, + fastapi_response: Response, + credential_name: str = Path(..., description="The credential name, percent-decoded; may contain slashes"), + user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth), +): + """ + [BETA] endpoint. This might change unexpectedly. + """ + try: + for credential in litellm.credential_list: + if credential.credential_name == credential_name: + masked_credential = CredentialItem( + credential_name=credential.credential_name, + credential_values=_get_masked_values( + credential.credential_values, + unmasked_length=4, + number_of_asterisks=4, + ), + credential_info=credential.credential_info, + ) + return masked_credential + raise HTTPException( + status_code=404, + detail="Credential not found. Got credential name: " + credential_name, + ) + except Exception as e: + verbose_proxy_logger.exception(e) + raise handle_exception_on_proxy(e) + + @router.get( "/credentials/by_model/{model_id}", dependencies=[Depends(user_api_key_auth)], tags=["credential management"], response_model=CredentialItem, ) -async def get_credential( +async def get_credential_by_model( request: Request, fastapi_response: Response, - credential_name: str = Path(..., description="The credential name, percent-decoded; may contain slashes"), - model_id: Optional[str] = None, + model_id: str = Path(..., description="The model ID to look up credentials for"), user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth), ): """ @@ -161,48 +191,25 @@ async def get_credential( from litellm.proxy.proxy_server import llm_router try: - if model_id: - if llm_router is None: - raise HTTPException(status_code=500, detail="LLM router not found") - model = llm_router.get_deployment(model_id) - if model is None: - raise HTTPException(status_code=404, detail="Model not found") - credential_values = llm_router.get_deployment_credentials(model_id) - if credential_values is None: - raise HTTPException(status_code=404, detail="Model not found") - masked_credential_values = _get_masked_values( - credential_values, - unmasked_length=4, - number_of_asterisks=4, - ) - credential = CredentialItem( - credential_name="{}-credential-{}".format(model.model_name, model_id), - credential_values=masked_credential_values, - credential_info={}, - ) - # return credential object - return credential - elif credential_name: - for credential in litellm.credential_list: - if credential.credential_name == credential_name: - masked_credential = CredentialItem( - credential_name=credential.credential_name, - credential_values=_get_masked_values( - credential.credential_values, - unmasked_length=4, - number_of_asterisks=4, - ), - credential_info=credential.credential_info, - ) - return masked_credential - raise HTTPException( - status_code=404, - detail="Credential not found. Got credential name: " + credential_name, - ) - else: - raise HTTPException( - status_code=404, detail="Credential name or model ID required" - ) + if llm_router is None: + raise HTTPException(status_code=500, detail="LLM router not found") + model = llm_router.get_deployment(model_id) + if model is None: + raise HTTPException(status_code=404, detail="Model not found") + credential_values = llm_router.get_deployment_credentials(model_id) + if credential_values is None: + raise HTTPException(status_code=404, detail="Model not found") + masked_credential_values = _get_masked_values( + credential_values, + unmasked_length=4, + number_of_asterisks=4, + ) + credential = CredentialItem( + credential_name="{}-credential-{}".format(model.model_name, model_id), + credential_values=masked_credential_values, + credential_info={}, + ) + return credential except Exception as e: verbose_proxy_logger.exception(e) raise handle_exception_on_proxy(e) diff --git a/litellm/proxy/spend_tracking/spend_management_endpoints.py b/litellm/proxy/spend_tracking/spend_management_endpoints.py index 08aaa85169..92770a5c80 100644 --- a/litellm/proxy/spend_tracking/spend_management_endpoints.py +++ b/litellm/proxy/spend_tracking/spend_management_endpoints.py @@ -1461,11 +1461,21 @@ async def _get_spend_report_for_time_range( dependencies=[Depends(user_api_key_auth)], responses={ 200: { - "cost": { - "description": "The calculated cost", - "example": 0.0, - "type": "float", - } + "description": "The calculated cost", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "cost": { + "type": "number", + "description": "The calculated cost", + "example": 0.0, + } + }, + } + } + }, } }, ) diff --git a/tests/test_litellm/proxy/test_openapi_schema_validation.py b/tests/test_litellm/proxy/test_openapi_schema_validation.py new file mode 100644 index 0000000000..aafe08f303 --- /dev/null +++ b/tests/test_litellm/proxy/test_openapi_schema_validation.py @@ -0,0 +1,142 @@ +""" +Test that the OpenAPI schema generated by FastAPI is valid for specific endpoints. + +Validates fixes for: +- /spend/calculate response schema (must use proper OpenAPI 3.x content wrapper) +- /credentials/by_model/{model_id} path parameter (must not leak credential_name) + +Related issue: https://github.com/BerriAI/litellm/issues/21305 +""" + +import pytest + + +class TestSpendCalculateOpenAPISchema: + """Test /spend/calculate response schema is valid OpenAPI 3.x.""" + + def test_response_schema_has_description(self): + """The 200 response must have a 'description' field per OpenAPI 3.x spec.""" + from litellm.proxy.spend_tracking.spend_management_endpoints import router + + for route in router.routes: + if hasattr(route, "path") and route.path == "/spend/calculate": + responses = route.responses or {} + response_200 = responses.get(200, {}) + assert "description" in response_200, ( + "/spend/calculate 200 response must have a 'description' field" + ) + break + else: + pytest.fail("/spend/calculate route not found in router") + + def test_response_schema_has_content_wrapper(self): + """The 200 response must use 'content' wrapper, not bare properties.""" + from litellm.proxy.spend_tracking.spend_management_endpoints import router + + for route in router.routes: + if hasattr(route, "path") and route.path == "/spend/calculate": + responses = route.responses or {} + response_200 = responses.get(200, {}) + # Must NOT have 'cost' as a top-level key (invalid OpenAPI) + assert "cost" not in response_200, ( + "/spend/calculate 200 response must not have 'cost' as a " + "top-level property - use 'content' wrapper instead" + ) + # Must have 'content' wrapper + assert "content" in response_200, ( + "/spend/calculate 200 response must have a 'content' field" + ) + content = response_200["content"] + assert "application/json" in content + assert "schema" in content["application/json"] + break + else: + pytest.fail("/spend/calculate route not found in router") + + +class TestCredentialEndpointsOpenAPISchema: + """Test /credentials endpoints have correct path parameters.""" + + def test_by_name_and_by_model_are_separate_handlers(self): + """ + /credentials/by_name/{credential_name} and /credentials/by_model/{model_id} + must be separate handler functions so each only declares its own path params. + """ + from litellm.proxy.credential_endpoints.endpoints import router + + by_name_routes = [] + by_model_routes = [] + for route in router.routes: + if not hasattr(route, "path"): + continue + if "by_name" in route.path: + by_name_routes.append(route) + elif "by_model" in route.path: + by_model_routes.append(route) + + assert len(by_name_routes) == 1, "Expected exactly one by_name route" + assert len(by_model_routes) == 1, "Expected exactly one by_model route" + + # They must be different endpoint functions + by_name_endpoint = by_name_routes[0].endpoint + by_model_endpoint = by_model_routes[0].endpoint + assert by_name_endpoint is not by_model_endpoint, ( + "by_name and by_model must be separate handler functions " + "to avoid path parameter conflicts in OpenAPI spec" + ) + + def test_by_model_route_does_not_require_credential_name(self): + """ + The /credentials/by_model/{model_id} route must NOT have + credential_name as a parameter. + """ + import inspect + from litellm.proxy.credential_endpoints.endpoints import ( + get_credential_by_model, + ) + + sig = inspect.signature(get_credential_by_model) + param_names = list(sig.parameters.keys()) + assert "credential_name" not in param_names, ( + "get_credential_by_model must not have a credential_name parameter" + ) + + def test_by_name_route_does_not_require_model_id(self): + """ + The /credentials/by_name/{credential_name} route must NOT have + model_id as a parameter. + """ + import inspect + from litellm.proxy.credential_endpoints.endpoints import ( + get_credential_by_name, + ) + + sig = inspect.signature(get_credential_by_name) + param_names = list(sig.parameters.keys()) + assert "model_id" not in param_names, ( + "get_credential_by_name must not have a model_id parameter" + ) + + def test_by_model_has_model_id_path_param(self): + """The by_model handler must accept model_id as a path parameter.""" + import inspect + from litellm.proxy.credential_endpoints.endpoints import ( + get_credential_by_model, + ) + + sig = inspect.signature(get_credential_by_model) + assert "model_id" in sig.parameters, ( + "get_credential_by_model must have a model_id parameter" + ) + + def test_by_name_has_credential_name_path_param(self): + """The by_name handler must accept credential_name as a path parameter.""" + import inspect + from litellm.proxy.credential_endpoints.endpoints import ( + get_credential_by_name, + ) + + sig = inspect.signature(get_credential_by_name) + assert "credential_name" in sig.parameters, ( + "get_credential_by_name must have a credential_name parameter" + )