From 2ac020c070c7fdd47ec912f5ae334923ec85356c Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Fri, 6 Nov 2026 02:09:06 -0800 Subject: [PATCH] fix(tool-choice): refuse an allowed_tools entry whose name is not a string An mcp or other non-function entry with a list or dict name passed the parser and crashed when the allow list was hashed. The parser now treats it as malformed on both layouts, so it answers the same 400 as an empty allow list, or drops under drop_params. --- .../allowed_tools_tool_choice.py | 8 ++++++-- .../test_allowed_tools_tool_choice.py | 16 ++++++++++++++++ .../mcp/test_aresponses_api_with_mcp.py | 11 +++++++++-- tests/unit/test_main.py | 14 ++++++++++++-- tests/unit/test_tool_loop.py | 10 ++++++++-- 5 files changed, 51 insertions(+), 8 deletions(-) diff --git a/litellm/litellm_core_utils/allowed_tools_tool_choice.py b/litellm/litellm_core_utils/allowed_tools_tool_choice.py index 0d836eb8aba..7ee53dcddab 100644 --- a/litellm/litellm_core_utils/allowed_tools_tool_choice.py +++ b/litellm/litellm_core_utils/allowed_tools_tool_choice.py @@ -92,6 +92,10 @@ def _has_name(selector: Mapping[str, object]) -> bool: return isinstance(name, str) and name != "" +def _names_on_other_tool_types_are_strings(tools: Iterable[Mapping[str, object]]) -> bool: + return all(tool.get("type") in ("function", "custom") or "name" not in tool or _has_name(tool) for tool in tools) + + def _chat_allowed_tool(tool: Mapping[str, object]) -> Mapping[str, object] | None: tool_type: Final = tool.get("type") if not isinstance(tool_type, str) or tool_type not in ("function", "custom"): @@ -120,9 +124,9 @@ def parse_allowed_tools_tool_choice(tool_choice: object) -> ChatAllowedToolsTool return None match parsed: case ChatAllowedToolsToolChoice(): - return parsed + return parsed if _names_on_other_tool_types_are_strings(parsed.allowed_tools.tools) else None case ResponsesAllowedToolsToolChoice(): - return _chat_layout(parsed) + return _chat_layout(parsed) if _names_on_other_tool_types_are_strings(parsed.tools) else None def azure_api_version_takes_tool_choice(api_version: str) -> bool: diff --git a/tests/unit/litellm_core_utils/test_allowed_tools_tool_choice.py b/tests/unit/litellm_core_utils/test_allowed_tools_tool_choice.py index a32ebe3e9b5..8b124818d6a 100644 --- a/tests/unit/litellm_core_utils/test_allowed_tools_tool_choice.py +++ b/tests/unit/litellm_core_utils/test_allowed_tools_tool_choice.py @@ -14,10 +14,20 @@ from litellm.litellm_core_utils.allowed_tools_tool_choice import ( parse_allowed_tools_tool_choice, refuse_a_malformed_allowed_tools_tool_choice, refuse_tool_calls_the_choice_excludes, + tool_calls_the_choice_excludes, unenforced_allowed_tools_tool_choice, responses_allowed_tools_tool_choice, ) +_CHOICES_WITH_A_NAME_THAT_IS_NOT_A_STRING: Final = ( + {"type": "allowed_tools", "mode": "auto", "tools": [{"type": "mcp", "name": ["get_time"]}]}, + {"type": "allowed_tools", "mode": "auto", "tools": [{"type": "mcp", "name": {"first": "get_time"}}]}, + {"type": "allowed_tools", "mode": "auto", "tools": [{"type": "mcp", "name": 7}]}, + {"type": "allowed_tools", "mode": "auto", "tools": [{"type": "mcp", "name": ""}]}, + {"type": "allowed_tools", "allowed_tools": {"mode": "auto", "tools": [{"type": "mcp", "name": ["get_time"]}]}}, + {"type": "allowed_tools", "allowed_tools": {"mode": "auto", "tools": [{"type": "mcp", "name": 7}]}}, +) + _CHAT_LAYOUT: Final[dict[str, object]] = { "type": "allowed_tools", "allowed_tools": { @@ -245,6 +255,7 @@ def test_a_choice_the_provider_cannot_take_answers_400_without_drop_params(): [ {"type": "allowed_tools", "mode": "required", "tools": []}, {"type": "allowed_tools", "tools": [{"type": "function", "name": "get_time"}]}, + *_CHOICES_WITH_A_NAME_THAT_IS_NOT_A_STRING, ], ) def test_a_malformed_choice_is_refused(tool_choice: object): @@ -254,6 +265,11 @@ def test_a_malformed_choice_is_refused(tool_choice: object): assert exc.value.status_code == 400 +@pytest.mark.parametrize("tool_choice", _CHOICES_WITH_A_NAME_THAT_IS_NOT_A_STRING) +def test_a_name_that_is_not_a_string_lists_no_tool(tool_choice: object): + assert tool_calls_the_choice_excludes(tool_choice, ("get_time",)) == ("get_time",) + + @pytest.mark.parametrize("tool_choice", [_FLAT_CHOICE, _CHAT_LAYOUT, "required", None]) def test_a_valid_choice_or_any_other_choice_is_not_called_malformed(tool_choice: object): assert refuse_a_malformed_allowed_tools_tool_choice(tool_choice, "the-model") is None diff --git a/tests/unit/responses/mcp/test_aresponses_api_with_mcp.py b/tests/unit/responses/mcp/test_aresponses_api_with_mcp.py index 28db003f4c1..01b223f5e26 100644 --- a/tests/unit/responses/mcp/test_aresponses_api_with_mcp.py +++ b/tests/unit/responses/mcp/test_aresponses_api_with_mcp.py @@ -1807,8 +1807,15 @@ def _anthropic_call_to_echo() -> httpx.Response: @pytest.mark.asyncio +@pytest.mark.parametrize( + "tool_choice", + [ + _ALLOWED_ONLY_ANOTHER_TOOL, + {"type": "allowed_tools", "mode": "auto", "tools": [{"type": "mcp", "name": ["echo"]}]}, + ], +) async def test_a_dropped_choice_hands_back_an_mcp_call_it_excludes( - respx_mock: respx.MockRouter, monkeypatch: pytest.MonkeyPatch + tool_choice: dict[str, object], respx_mock: respx.MockRouter, monkeypatch: pytest.MonkeyPatch ) -> None: monkeypatch.setattr(litellm, "disable_aiohttp_transport", True) monkeypatch.setenv("DISABLE_AIOHTTP_TRANSPORT", "True") @@ -1824,7 +1831,7 @@ async def test_a_dropped_choice_hands_back_an_mcp_call_it_excludes( model="anthropic/claude-haiku-4-5", input="Call echo with PING", tools=[dict(_ECHO_MCP_TOOL)], - tool_choice=_ALLOWED_ONLY_ANOTHER_TOOL, + tool_choice=tool_choice, drop_params=True, api_key="sk-ant-fake", ) diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index eac7ac8ed60..7313a1600f9 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -648,13 +648,23 @@ def test_completion_drop_params_drops_an_allowed_tools_tool_choice_it_cannot_sen @pytest.mark.asyncio -async def test_a_malformed_allowed_tools_tool_choice_stays_a_400_on_mcp_gateway_tools(respx_mock: respx.MockRouter): +@pytest.mark.parametrize( + "tool_choice", + [ + {"type": "allowed_tools", "mode": "required", "tools": []}, + {"type": "allowed_tools", "mode": "auto", "tools": [{"type": "mcp", "name": ["get_time"]}]}, + {"type": "allowed_tools", "allowed_tools": {"mode": "auto", "tools": [{"type": "mcp", "name": ["get_time"]}]}}, + ], +) +async def test_a_malformed_allowed_tools_tool_choice_stays_a_400_on_mcp_gateway_tools( + tool_choice: dict[str, object], respx_mock: respx.MockRouter +): with pytest.raises(litellm.BadRequestError, match="Invalid tool choice") as exc: await litellm.acompletion( model="openai/gpt-5.5", messages=[{"role": "user", "content": "What time is it?"}], tools=[{"type": "mcp", "server_url": "litellm_proxy/mcp/demo", "require_approval": "never"}], - tool_choice={"type": "allowed_tools", "mode": "required", "tools": []}, + tool_choice=tool_choice, drop_params=True, api_base="http://localhost:12346/v1", api_key="fake_api_key", diff --git a/tests/unit/test_tool_loop.py b/tests/unit/test_tool_loop.py index 9d0539c80d2..bf5c00ef53f 100644 --- a/tests/unit/test_tool_loop.py +++ b/tests/unit/test_tool_loop.py @@ -336,14 +336,20 @@ def test_a_tool_the_choice_excludes_never_runs_when_the_model_ignored_the_choice assert not _tools_run_before_the_refusal(_ALLOWED_WEATHER_ONLY, model="openai/gpt-5-mini") -def test_a_malformed_choice_is_refused_before_any_request(respx_mock: respx.MockRouter) -> None: +@pytest.mark.parametrize( + "tool_choice", + [_EMPTY_ALLOWED_TOOLS, {"type": "allowed_tools", "mode": "auto", "tools": [{"type": "mcp", "name": ["get_time"]}]}], +) +def test_a_malformed_choice_is_refused_before_any_request( + tool_choice: dict[str, object], respx_mock: respx.MockRouter +) -> None: with pytest.raises(litellm.BadRequestError, match="Invalid tool choice"): litellm.run_tool_loop( model="openai/gpt-5-mini", messages=[{"role": "user", "content": "hi"}], tools=WEATHER_TOOLS, execute_tool=_tool_result, - tool_choice=_EMPTY_ALLOWED_TOOLS, + tool_choice=tool_choice, drop_params=True, api_key="sk-test", )