From 79517bc6282c43d0d844b6d3f7b2597e3c2735bd Mon Sep 17 00:00:00 2001 From: Darien Kindlund Date: Fri, 24 Apr 2026 12:03:55 -0400 Subject: [PATCH] fix: address Greptile review feedback on PR #26439 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three concerns raised by bot reviewers, all addressed: 1. CodeQL cyclic-import warning ``experimental_pass_through/transformation.py`` imported from the parent ``..transformation`` module, which CodeQL flagged as a potential cycle. Extracted the helper into a new leaf module ``vertex_ai_partner_models/anthropic/output_params_utils.py`` that has no heavy imports of its own. Both transformation files now import from it cleanly. Renamed the helper from the underscore- prefixed ``_sanitize_vertex_anthropic_output_params`` to the public ``sanitize_vertex_anthropic_output_params`` since it is now shared across modules. 2. Greptile P2: redundant ``None`` guard on ``extra_kwargs`` ``handler.py`` had two ``extra_kwargs = extra_kwargs if ... else {}`` coercions; the second was a no-op because line 220 already coerced. Removed the second one and added a NOTE comment so future readers understand ``extra_kwargs`` is guaranteed non-None at the point of use. 3. Greptile P2: misleading "already translated" docstring The docstring claimed the translator above mapped ``output_config.format`` to ``response_format``, but Greptile correctly traced the code and found that only the legacy top-level ``output_format`` was being translated — ``output_config.format`` was being silently dropped on the adapter path. Two-part fix: a. Code: extended ``_translate_output_format_to_openai`` to accept both shapes (top-level ``output_format`` AND ``output_config.format`` sub-key). Top-level still takes precedence when both are supplied. This means callers using the newer Anthropic Structured Outputs API now have their schema properly forwarded to non-Anthropic backends as ``response_format``. b. Tests: rewrote the misleading docstring to describe what actually happens, plus added two new tests: * ``test_output_format_top_level_still_translates`` — regression guard for the legacy path * ``test_output_format_takes_precedence_over_output_config_format`` — documents the precedence rule explicitly Tests: 28/28 pass (was 26/26 before; +2 for the new translation behavior + precedence). All run in ~0.5s, no real network calls. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../adapters/handler.py | 3 +- .../adapters/transformation.py | 23 +++-- .../transformation.py | 4 +- .../anthropic/output_params_utils.py | 50 +++++++++++ .../anthropic/transformation.py | 42 +-------- .../test_handler_output_config_passthrough.py | 85 ++++++++++++++++--- ...partner_models_anthropic_transformation.py | 14 +-- 7 files changed, 156 insertions(+), 65 deletions(-) create mode 100644 litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/output_params_utils.py diff --git a/litellm/llms/anthropic/experimental_pass_through/adapters/handler.py b/litellm/llms/anthropic/experimental_pass_through/adapters/handler.py index 10455825e41..ac55aac8062 100644 --- a/litellm/llms/anthropic/experimental_pass_through/adapters/handler.py +++ b/litellm/llms/anthropic/experimental_pass_through/adapters/handler.py @@ -254,7 +254,8 @@ class LiteLLMMessagesToCompletionTransformationHandler: # ``AnthropicMessagesRequestOptionalParams``, also extend # ``ANTHROPIC_ONLY_REQUEST_KEYS`` here so it doesn't silently leak. excluded_keys = ANTHROPIC_ONLY_REQUEST_KEYS | {"anthropic_messages"} - extra_kwargs = extra_kwargs if extra_kwargs is not None else {} + # NOTE: extra_kwargs was already coerced from None to {} at the top of + # this method (line ~220). It is guaranteed to be a dict here. for key, value in extra_kwargs.items(): if ( key == "litellm_logging_obj" diff --git a/litellm/llms/anthropic/experimental_pass_through/adapters/transformation.py b/litellm/llms/anthropic/experimental_pass_through/adapters/transformation.py index 20fa4f125de..f7bc67ccd3a 100644 --- a/litellm/llms/anthropic/experimental_pass_through/adapters/transformation.py +++ b/litellm/llms/anthropic/experimental_pass_through/adapters/transformation.py @@ -664,7 +664,7 @@ class LiteLLMAnthropicMessagesAdapter: @staticmethod def translate_anthropic_thinking_to_reasoning_effort( - thinking: Dict[str, Any] + thinking: Dict[str, Any], ) -> Optional[str]: """ Translate Anthropic's thinking parameter to OpenAI's reasoning_effort. @@ -1081,10 +1081,23 @@ class LiteLLMAnthropicMessagesAdapter: anthropic_message_request: AnthropicMessagesRequest, new_kwargs: ChatCompletionRequest, ) -> None: - """Translate output_format to response_format when applicable.""" - if "output_format" not in anthropic_message_request: - return - output_format = anthropic_message_request["output_format"] + """Translate Anthropic structured-output config to OpenAI ``response_format``. + + Accepts either the legacy top-level ``output_format`` field OR the + newer ``output_config.format`` (sub-key on ``output_config``) so that + both shapes flow through to non-Anthropic backends as + ``response_format``. Without the ``output_config.format`` branch, + callers using the new Anthropic Structured Outputs API would have + their schema silently dropped on the adapter path — only the legacy + top-level ``output_format`` was being mapped. + + ``output_format`` takes precedence when both are provided. + """ + output_format: Any = anthropic_message_request.get("output_format") + if not output_format: + output_config = anthropic_message_request.get("output_config") + if isinstance(output_config, dict): + output_format = output_config.get("format") if not output_format: return response_format = self.translate_anthropic_output_format_to_openai( diff --git a/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/experimental_pass_through/transformation.py b/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/experimental_pass_through/transformation.py index 9080ac02330..d450f7a4635 100644 --- a/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/experimental_pass_through/transformation.py +++ b/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/experimental_pass_through/transformation.py @@ -13,7 +13,7 @@ from litellm.types.llms.vertex_ai import VertexPartnerProvider from litellm.types.router import GenericLiteLLMParams from ....vertex_llm_base import VertexBase -from ..transformation import _sanitize_vertex_anthropic_output_params +from ..output_params_utils import sanitize_vertex_anthropic_output_params class VertexAIPartnerModelsAnthropicMessagesConfig(AnthropicMessagesConfig, VertexBase): @@ -163,6 +163,6 @@ class VertexAIPartnerModelsAnthropicMessagesConfig(AnthropicMessagesConfig, Vert # and ``output_format``, but rejects ``output_config.effort`` with 400 # "Extra inputs are not permitted". Sanitize in place so the supported # bits flow through. - _sanitize_vertex_anthropic_output_params(anthropic_messages_request) + sanitize_vertex_anthropic_output_params(anthropic_messages_request) return anthropic_messages_request diff --git a/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/output_params_utils.py b/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/output_params_utils.py new file mode 100644 index 00000000000..982d8edbf20 --- /dev/null +++ b/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/output_params_utils.py @@ -0,0 +1,50 @@ +""" +Shared sanitization for ``output_config`` / ``output_format`` on Vertex AI +Claude. Lives in its own module so both the chat-completion transformation +(``transformation.py``) and the Messages pass-through transformation +(``experimental_pass_through/transformation.py``) can import it without +forming a cycle through the parent module's heavier imports. + +CodeQL flagged the ``..transformation`` import path as a potential cyclic +import; extracting the helper into a leaf module resolves the warning and +keeps the parent module's import surface narrow. +""" + +# Keys inside ``output_config`` that Vertex AI Claude does not accept. +# Today only ``effort`` triggers "Extra inputs are not permitted"; add new +# entries here as Vertex parity drifts. Keep this list narrow — anything +# Vertex DOES accept (e.g. ``format`` for structured outputs) must be +# preserved so callers can rely on Anthropic-native features. +VERTEX_UNSUPPORTED_OUTPUT_CONFIG_KEYS: frozenset = frozenset({"effort"}) + + +def sanitize_vertex_anthropic_output_params(data: dict) -> None: + """ + Strip Vertex-unsupported keys from ``output_config`` / + ``output_format`` in-place; forward whatever remains. + + Behavior: + * ``output_config`` containing only unsupported keys (e.g. ``effort`` + alone) is removed entirely so the request body has no empty dict. + * ``output_config`` containing a mix of supported + unsupported keys + has the unsupported subset filtered out and the rest forwarded. + * ``output_config`` that is supported in full passes through unchanged. + * ``output_format`` is forwarded as-is (Vertex AI Claude accepts it). + * Non-dict values for ``output_config`` are dropped to avoid sending + malformed payloads downstream. + """ + output_config = data.get("output_config") + if output_config is None: + return + if not isinstance(output_config, dict): + data.pop("output_config", None) + return + sanitized = { + k: v + for k, v in output_config.items() + if k not in VERTEX_UNSUPPORTED_OUTPUT_CONFIG_KEYS + } + if sanitized: + data["output_config"] = sanitized + else: + data.pop("output_config", None) diff --git a/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/transformation.py b/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/transformation.py index a9dea6646ff..914c7e92e5e 100644 --- a/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/transformation.py +++ b/litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/transformation.py @@ -10,45 +10,7 @@ from litellm.types.llms.openai import AllMessageValues from litellm.types.utils import ModelResponse from ....anthropic.chat.transformation import AnthropicConfig - -# Keys inside ``output_config`` that Vertex AI Claude does not accept. -# Today only ``effort`` triggers "Extra inputs are not permitted"; add new -# entries here as Vertex parity drifts. Keep this list narrow — anything -# Vertex DOES accept (e.g. ``format`` for structured outputs) must be -# preserved so callers can rely on Anthropic-native features. -_VERTEX_UNSUPPORTED_OUTPUT_CONFIG_KEYS = frozenset({"effort"}) - - -def _sanitize_vertex_anthropic_output_params(data: dict) -> None: - """ - Strip Vertex-unsupported keys from ``output_config`` / ``output_format`` - in-place; forward whatever remains. - - Behavior: - * ``output_config`` containing only unsupported keys (e.g. ``effort`` - alone) is removed entirely so the request body has no empty dict. - * ``output_config`` containing a mix of supported + unsupported keys has - the unsupported subset filtered out and the rest forwarded. - * ``output_config`` that is supported in full passes through unchanged. - * ``output_format`` is forwarded as-is (Vertex AI Claude accepts it). - * Non-dict values for ``output_config`` are dropped to avoid sending - malformed payloads downstream. - """ - output_config = data.get("output_config") - if output_config is None: - return - if not isinstance(output_config, dict): - data.pop("output_config", None) - return - sanitized = { - k: v - for k, v in output_config.items() - if k not in _VERTEX_UNSUPPORTED_OUTPUT_CONFIG_KEYS - } - if sanitized: - data["output_config"] = sanitized - else: - data.pop("output_config", None) +from .output_params_utils import sanitize_vertex_anthropic_output_params class VertexAIError(Exception): @@ -149,7 +111,7 @@ class VertexAIAnthropicConfig(AnthropicConfig): # Vertex returns 400 "Extra inputs are not permitted". Sanitize in place: # forward the structured-output bits, drop the unsupported keys. # Same treatment for the legacy top-level ``output_format`` field. - _sanitize_vertex_anthropic_output_params(data) + sanitize_vertex_anthropic_output_params(data) tools = optional_params.get("tools") tool_search_used = self.is_tool_search_used(tools) diff --git a/tests/test_litellm/llms/anthropic/experimental_pass_through/adapters/test_handler_output_config_passthrough.py b/tests/test_litellm/llms/anthropic/experimental_pass_through/adapters/test_handler_output_config_passthrough.py index 50e5fd884e9..615dc5cfebc 100644 --- a/tests/test_litellm/llms/anthropic/experimental_pass_through/adapters/test_handler_output_config_passthrough.py +++ b/tests/test_litellm/llms/anthropic/experimental_pass_through/adapters/test_handler_output_config_passthrough.py @@ -46,10 +46,13 @@ from litellm.llms.anthropic.experimental_pass_through.adapters.handler import ( MESSAGES = [{"role": "user", "content": "hello"}] -def _call_prepare(extra_kwargs, model="gpt-4o", **overrides): +def _call_prepare(extra_kwargs, model="gpt-4o", output_format=None, **overrides): """ Drive ``_prepare_completion_kwargs`` with the minimum scaffolding needed. + ``output_format`` is a top-level parameter on the function, so callers + pass it explicitly here rather than tucking it into ``extra_kwargs``. + Uses an explicit-None check on ``extra_kwargs`` so callers can test the falsy-empty-dict path. The fallback ``or {}`` pattern PR #22727 used here masked the no-extra-kwargs case from ever exercising the test's intent. @@ -68,7 +71,7 @@ def _call_prepare(extra_kwargs, model="gpt-4o", **overrides): tools=None, top_k=None, top_p=None, - output_format=None, + output_format=output_format, extra_kwargs=extra_kwargs, ) @@ -107,22 +110,84 @@ class TestOutputConfigStrippedFromCompletionKwargs: "reject it with 400 'Extra inputs are not permitted'" ) - def test_output_config_with_format_is_stripped_format_already_translated(self): - """Even when ``output_config`` carries useful structured-output info, - the raw key must be excluded — the translator above has already mapped - ``output_config.format`` to ``response_format`` (the OpenAI-shaped key - the downstream backend understands).""" + def test_output_config_format_translated_to_response_format(self): + """When ``output_config`` carries structured-output ``format``, the + translator now maps it to OpenAI's ``response_format`` so non-Anthropic + backends see the schema in their native shape. The raw + ``output_config`` key is still stripped from ``completion_kwargs`` — + only the translated ``response_format`` survives. + + Before this PR, only the legacy top-level ``output_format`` was + translated; ``output_config.format`` was silently dropped on the + adapter path even when the schema was correctly supplied (issue + flagged by Greptile review of the initial fix). + """ + schema = { + "type": "object", + "additionalProperties": False, + "properties": {"name": {"type": "string"}}, + } extra_kwargs = { "custom_llm_provider": "azure", - "output_config": { - "format": {"type": "json_schema", "schema": {"type": "object"}} - }, + "output_config": {"format": {"type": "json_schema", "schema": schema}}, } result = _call_prepare(extra_kwargs=extra_kwargs) completion_kwargs = result[0] if isinstance(result, tuple) else result + # Raw Anthropic-shaped key is gone (would 400 on non-Anthropic backends). assert "output_config" not in completion_kwargs + # Translated OpenAI-shaped key is present so the schema actually + # reaches the downstream backend. + assert "response_format" in completion_kwargs, ( + "output_config.format must be translated to response_format — " + "without this, structured-output schemas are silently dropped on " + "the adapter path" + ) + + def test_output_format_top_level_still_translates(self): + """Regression guard: the legacy top-level ``output_format`` field must + continue to translate to ``response_format``. The new + ``output_config.format`` path must not break this existing behavior.""" + schema = {"type": "object", "properties": {"name": {"type": "string"}}} + result = _call_prepare( + extra_kwargs={"custom_llm_provider": "azure"}, + output_format={"type": "json_schema", "schema": schema}, + ) + completion_kwargs = result[0] if isinstance(result, tuple) else result + + assert "response_format" in completion_kwargs + + def test_output_format_takes_precedence_over_output_config_format(self): + """When both top-level ``output_format`` and ``output_config.format`` + are present, the legacy top-level ``output_format`` wins. Documents + which one the translator picks rather than leaving it implementation- + defined.""" + winning_schema = { + "type": "object", + "properties": {"top_level": {"type": "string"}}, + } + losing_schema = { + "type": "object", + "properties": {"nested": {"type": "string"}}, + } + result = _call_prepare( + extra_kwargs={ + "custom_llm_provider": "azure", + "output_config": { + "format": {"type": "json_schema", "schema": losing_schema} + }, + }, + output_format={"type": "json_schema", "schema": winning_schema}, + ) + completion_kwargs = result[0] if isinstance(result, tuple) else result + + assert "response_format" in completion_kwargs + # Verify the winning_schema (top-level output_format) was used, + # not the losing one nested under output_config. + rendered = str(completion_kwargs["response_format"]) + assert "top_level" in rendered + assert "nested" not in rendered def test_other_extra_kwargs_still_passed_through(self): """Regression guard: the strip must be narrow. Unrelated fields like diff --git a/tests/test_litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/test_vertex_ai_partner_models_anthropic_transformation.py b/tests/test_litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/test_vertex_ai_partner_models_anthropic_transformation.py index 376c48d9e95..d0be476d72e 100644 --- a/tests/test_litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/test_vertex_ai_partner_models_anthropic_transformation.py +++ b/tests/test_litellm/llms/vertex_ai/vertex_ai_partner_models/anthropic/test_vertex_ai_partner_models_anthropic_transformation.py @@ -693,32 +693,32 @@ def test_sanitize_vertex_anthropic_output_params_unit(): """Direct unit coverage for the helper itself (used by both Vertex Anthropic transformation paths). Mirrors the integration assertions above without going through the full ``transform_request`` stack.""" - from litellm.llms.vertex_ai.vertex_ai_partner_models.anthropic.transformation import ( - _sanitize_vertex_anthropic_output_params, + from litellm.llms.vertex_ai.vertex_ai_partner_models.anthropic.output_params_utils import ( + sanitize_vertex_anthropic_output_params, ) # No-op when output_config absent. data: dict = {"max_tokens": 8} - _sanitize_vertex_anthropic_output_params(data) + sanitize_vertex_anthropic_output_params(data) assert data == {"max_tokens": 8} # Effort-only → dropped entirely. data = {"output_config": {"effort": "high"}} - _sanitize_vertex_anthropic_output_params(data) + sanitize_vertex_anthropic_output_params(data) assert "output_config" not in data # Format-only → preserved unchanged. fmt = {"format": {"type": "json_schema", "schema": {"type": "object"}}} data = {"output_config": dict(fmt)} - _sanitize_vertex_anthropic_output_params(data) + sanitize_vertex_anthropic_output_params(data) assert data["output_config"] == fmt # Mixed → effort filtered, format kept. data = {"output_config": {"format": fmt["format"], "effort": "high"}} - _sanitize_vertex_anthropic_output_params(data) + sanitize_vertex_anthropic_output_params(data) assert data["output_config"] == fmt # Non-dict → dropped defensively. data = {"output_config": "garbage"} - _sanitize_vertex_anthropic_output_params(data) + sanitize_vertex_anthropic_output_params(data) assert "output_config" not in data