mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix: don't inject empty tools:[] when client omitted the tools field (#772)
Fixes #728 ## Summary - `apply_session_sticky_ccr_tool` and `apply_session_sticky_memory_tools` always return a list — returning `[]` when `existing_tools=None` and nothing was injected - The old handler guard `if tools is not None:` evaluated `True` for `[]`, causing `body["tools"] = []` to be sent upstream on every request - vLLM-based providers (Venice.ai, etc.) strictly reject empty `tools` arrays with a 400 error **Fix:** Change the guard in both the OpenAI and Anthropic handlers from: ```python if tools is not None: body["tools"] = tools ``` to: ```python if tools or _original_tools is not None: body["tools"] = tools ``` The `_original_tools` variable is already defined in both handlers (`_original_tools = body.get("tools")`). This condition correctly handles all four cases: | Scenario | `tools` | `_original_tools` | Result | |---|---|---|---| | No client tools, no injection | `[]` | `None` | `False` → don't inject ✅ | | No client tools, CCR injected | `[ccr_tool]` | `None` | `True` → inject ✅ | | Client sent `tools: []` | `[]` | `[]` | `True` → preserve ✅ | | Client sent tools | `[A, ...]` | `[A, ...]` | `True` → preserve ✅ | ## Test plan - [x] New test file `tests/test_issue_728_empty_tools_injection.py` with 7 tests covering the guard condition and helper behavior - [x] All 51 existing CCR/golden-bytes tests still pass - [x] Zero changes to helper function return types or signatures ## Real behavior proof Tested against the helpers directly: ``` tests/test_issue_728_empty_tools_injection.py::TestHandlerGuardCondition::test_no_tools_no_injection_does_not_inject PASSED tests/test_issue_728_empty_tools_injection.py::TestHandlerGuardCondition::test_client_sent_empty_tools_is_preserved PASSED tests/test_issue_728_empty_tools_injection.py::TestHandlerGuardCondition::test_ccr_injection_sets_body_tools PASSED tests/test_issue_728_empty_tools_injection.py::TestHandlerGuardCondition::test_client_tools_always_set PASSED tests/test_issue_728_empty_tools_injection.py::TestCCRHelperNoToolsNoCompression::test_returns_empty_list_and_false_when_no_session_ccr PASSED tests/test_issue_728_empty_tools_injection.py::TestCCRHelperNoToolsNoCompression::test_returns_tool_list_when_compression_occurred PASSED tests/test_issue_728_empty_tools_injection.py::TestCCRHelperNoToolsNoCompression::test_no_double_injection_when_client_pre_registered_ccr_tool PASSED 7 passed in 1.81s ``` **What I did not test:** end-to-end against a live Venice.ai endpoint (no API key available), or passthrough mode with a real vLLM backend. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Tejas Chopra <chopratejas@gmail.com>
This commit is contained in:
parent
199d693f98
commit
574bbae2cb
3 changed files with 136 additions and 2 deletions
|
|
@ -1646,7 +1646,7 @@ class AnthropicHandlerMixin:
|
|||
|
||||
# Update body
|
||||
body["messages"] = optimized_messages
|
||||
if tools is not None:
|
||||
if tools or _original_tools is not None:
|
||||
tools = self._sort_tools_deterministically(tools)
|
||||
body["tools"] = tools
|
||||
|
||||
|
|
|
|||
|
|
@ -1898,7 +1898,7 @@ class OpenAIHandlerMixin:
|
|||
tools = remembered_event.tools
|
||||
|
||||
body["messages"] = optimized_messages
|
||||
if tools is not None:
|
||||
if tools or _original_tools is not None:
|
||||
body["tools"] = tools
|
||||
|
||||
presend_event = self.pipeline_extensions.emit(
|
||||
|
|
|
|||
134
tests/test_issue_728_empty_tools_injection.py
Normal file
134
tests/test_issue_728_empty_tools_injection.py
Normal file
|
|
@ -0,0 +1,134 @@
|
|||
"""Issue #728: proxy must not inject ``tools: []`` when the client omitted the tools field.
|
||||
|
||||
vLLM-based providers (Venice.ai, etc.) reject requests containing an empty ``tools``
|
||||
array. The bug: ``apply_session_sticky_ccr_tool`` / ``apply_session_sticky_memory_tools``
|
||||
always return a list (empty when no tools exist and none were injected), and the
|
||||
old handler guard ``if tools is not None`` evaluated True for ``[]``, causing
|
||||
``body["tools"] = []`` to be sent upstream unconditionally.
|
||||
|
||||
Fix: the guard was changed to ``if tools or _original_tools is not None`` in both
|
||||
the OpenAI and Anthropic handlers so that an empty result list only reaches the
|
||||
outgoing body when the original request already carried a ``tools`` field.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from headroom.ccr.tool_injection import CCR_TOOL_NAME
|
||||
from headroom.proxy.helpers import (
|
||||
_reset_session_ccr_tracker_for_test,
|
||||
apply_session_sticky_ccr_tool,
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _reset_tracker():
|
||||
_reset_session_ccr_tracker_for_test()
|
||||
yield
|
||||
_reset_session_ccr_tracker_for_test()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Guard-condition logic (the actual fix)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _should_set_body_tools(tools: list | None, original_tools: list | None) -> bool:
|
||||
"""Mirror the fixed handler condition: ``if tools or _original_tools is not None``."""
|
||||
return bool(tools or original_tools is not None)
|
||||
|
||||
|
||||
class TestHandlerGuardCondition:
|
||||
"""Verify the guard condition that decides whether to write body['tools']."""
|
||||
|
||||
def test_no_tools_no_injection_does_not_inject(self):
|
||||
"""Client sent no tools and nothing was injected → body must stay tools-free."""
|
||||
original_tools = None # client did not send tools
|
||||
tools_after_helpers = [] # helpers return [] when existing_tools=None and no inject
|
||||
|
||||
assert not _should_set_body_tools(tools_after_helpers, original_tools), (
|
||||
"Empty tools from helpers + no original tools must NOT write body['tools']"
|
||||
)
|
||||
|
||||
def test_client_sent_empty_tools_is_preserved(self):
|
||||
"""Client explicitly sent ``tools: []`` → preserve that field (their choice)."""
|
||||
original_tools = [] # client explicitly sent an empty array
|
||||
tools_after_helpers = [] # nothing injected
|
||||
|
||||
assert _should_set_body_tools(tools_after_helpers, original_tools), (
|
||||
"Client's explicit tools:[] should be preserved in body"
|
||||
)
|
||||
|
||||
def test_ccr_injection_sets_body_tools(self):
|
||||
"""When CCR injects a tool into an originally tool-free request → set body."""
|
||||
original_tools = None
|
||||
from headroom.ccr.tool_injection import create_ccr_tool_definition
|
||||
|
||||
tools_after_helpers = [create_ccr_tool_definition("openai")]
|
||||
|
||||
assert _should_set_body_tools(tools_after_helpers, original_tools), (
|
||||
"Injected CCR tool must reach body['tools']"
|
||||
)
|
||||
|
||||
def test_client_tools_always_set(self):
|
||||
"""Client provided real tools → always write body['tools']."""
|
||||
original_tools = [{"type": "function", "function": {"name": "my_tool"}}]
|
||||
tools_after_helpers = original_tools[:]
|
||||
|
||||
assert _should_set_body_tools(tools_after_helpers, original_tools)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# apply_session_sticky_ccr_tool behaviour with no existing tools
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestCCRHelperNoToolsNoCompression:
|
||||
"""Verify what the helper returns when there are no tools and no CCR happened."""
|
||||
|
||||
def test_returns_empty_list_and_false_when_no_session_ccr(self):
|
||||
"""No session CCR history + no compression this turn → ([], False)."""
|
||||
tools_out, was_injected = apply_session_sticky_ccr_tool(
|
||||
provider="openai",
|
||||
session_id="fresh-session-728",
|
||||
request_id="req-1",
|
||||
existing_tools=None,
|
||||
has_compressed_content_this_turn=False,
|
||||
)
|
||||
assert was_injected is False
|
||||
# Helper still returns [] — the guard in the handler is what prevents injection.
|
||||
assert tools_out == []
|
||||
|
||||
def test_returns_tool_list_when_compression_occurred(self):
|
||||
"""First turn with CCR → helper returns the CCR tool definition."""
|
||||
tools_out, was_injected = apply_session_sticky_ccr_tool(
|
||||
provider="openai",
|
||||
session_id="ccr-session-728",
|
||||
request_id="req-1",
|
||||
existing_tools=None,
|
||||
has_compressed_content_this_turn=True,
|
||||
)
|
||||
assert was_injected is True
|
||||
tool_names = [t.get("function", {}).get("name") or t.get("name") for t in tools_out]
|
||||
assert CCR_TOOL_NAME in tool_names
|
||||
|
||||
def test_no_double_injection_when_client_pre_registered_ccr_tool(self):
|
||||
"""If the client already included the CCR tool, the helper must not duplicate it."""
|
||||
from headroom.ccr.tool_injection import create_ccr_tool_definition
|
||||
|
||||
existing = [create_ccr_tool_definition("openai")]
|
||||
tools_out, was_injected = apply_session_sticky_ccr_tool(
|
||||
provider="openai",
|
||||
session_id="pre-reg-session-728",
|
||||
request_id="req-1",
|
||||
existing_tools=existing,
|
||||
has_compressed_content_this_turn=True,
|
||||
)
|
||||
assert was_injected is False
|
||||
ccr_count = sum(
|
||||
1
|
||||
for t in tools_out
|
||||
if (t.get("function", {}).get("name") or t.get("name")) == CCR_TOOL_NAME
|
||||
)
|
||||
assert ccr_count == 1, "CCR tool should appear exactly once"
|
||||
Loading…
Add table
Add a link
Reference in a new issue