fix: merge websearch tool params - #32162
Conversation
|
|
Greptile SummaryThis PR fixes websearch tool parameter forwarding and router synchronization for DB-backed proxy deployments. Search tools created via the UI were not being merged with config-defined tools in the router, so web-search requests fell back to Perplexity even when a different provider was configured in the database. The fix also properly forwards the full
Confidence Score: 5/5Safe to merge — the fix is narrow and well-tested, the merge logic is straightforward, and the new authorization layer follows established proxy helper patterns. All three changes (tool merging, credential forwarding, auth checks) have focused unit tests using mocks, no real network calls are introduced, and the No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/integrations/websearch_interception/handler.py | Refactored _execute_search to forward full litellm_params, added _authorize_search_tool for key/team permission checks, and extracted selection logic into _select_search_tool_from_list/_select_search_tool_from_router. Logic is sound; the redundant if kwargs is None: guard in try_short_circuit_search can be simplified. |
| litellm/llms/anthropic/experimental_pass_through/messages/handler.py | Threads kwargs (merged with metadata) into _try_websearch_short_circuit so auth context and litellm_params are available to the short-circuit path. Change is minimal and correct. |
| litellm/proxy/proxy_server.py | Replaced the DB-only router update with a merged config+DB list via the new _merge_config_and_db_search_tools static method. Uses get_config_state() (read-only, no I/O) to retrieve config tools. Logic is correct and the merge semantics (DB wins on duplicates) match the documented behavior. |
| tests/test_litellm/integrations/websearch_interception/test_websearch_interception_handler.py | Adds focused regression tests for litellm_params forwarding, key allowlist enforcement, and team allowlist enforcement. Tests use mocks and monkeypatching correctly; no real network calls introduced. |
| tests/test_litellm/proxy/proxy_server/test_proxy_config.py | Adds three new tests covering the merge logic (superset, duplicate/DB-wins, and end-to-end startup sync). Pre-existing tests reformatted for line length only — assertions are unchanged. |
Reviews (4): Last reviewed commit: "fix: preserve search tools on empty sync" | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6fefff3db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except Exception as e: | ||
| verbose_proxy_logger.debug(f"Could not get config-defined search tools: {e}") |
There was a problem hiding this comment.
Preserve config search tools when config reload fails
In the DB-enabled sync path (add_deployment -> _init_non_llm_objects_in_db), if get_config() raises here, config_search_tools stays empty and the later update_router_search_tools(...) call overwrites llm_router.search_tools with only DB tools, or an empty list when the DB has none. For proxies that already loaded search tools from config, a transient remote config read failure or missing config file during this periodic sync will evict the working config-defined search tools instead of preserving the last known router state.
Useful? React with 👍 / 👎.
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@greptile review |
|
@greptileai review |
| try: | ||
| search_result_text, structured = await self._execute_search(query) | ||
| if kwargs is None: | ||
| search_result_text, structured = await self._execute_search(query) | ||
| else: | ||
| search_result_text, structured = await self._execute_search(query, kwargs=kwargs) | ||
| except Exception as e: | ||
| verbose_logger.error(f"WebSearchInterception: Short-circuit search failed: {e}") | ||
| search_result_text, structured = f"Search failed: {e}", None |
There was a problem hiding this comment.
Auth exception swallowed in short-circuit path
_execute_search can now raise ProxyException from _authorize_search_tool when a key or team lacks permission for a search tool. That exception is a subclass of Exception and therefore falls into the existing catch-all block, which converts it into a "Search failed: ..." text string. The client receives a 200 OK with a failed search message instead of a proper authorization error — and the proxy logs the rejection at error level rather than surfacing it to the caller. The agentic-loop path (gathered search_tasks) lets the exception propagate normally, so there is already an inconsistency between the two paths.
|
bugbot please review? |
There was a problem hiding this comment.
Left a qq
Also, I'm seeing: FAILED tests/test_litellm/proxy/common_utils/test_key_rotation_e2e.py::TestDeprecatedKeyLookupDbE2E::test_deprecated_key_grace_period_cache_hit_path - httpx.ConnectError: All connection attempts failed. Deterministic failure or flake?
|
|
||
| async def _execute_search(self, query: str) -> Tuple[str, Optional[SearchResponse]]: | ||
| async def _execute_search( | ||
| self, query: str, kwargs: Optional[dict[str, Any]] = None |
There was a problem hiding this comment.
I'm noticing many introduced kwargs. Are these necessary? Can we just introduce named params?
There was a problem hiding this comment.
yes - this fixes the issue where litellm params were not being passed through. since litellm params can be a broad dict, this is appropriate imo
There was a problem hiding this comment.
Left a qq
Also, I'm seeing: FAILED tests/test_litellm/proxy/common_utils/test_key_rotation_e2e.py::TestDeprecatedKeyLookupDbE2E::test_deprecated_key_grace_period_cache_hit_path - httpx.ConnectError: All connection attempts failed. Deterministic failure or flake?
this seems like flake, since it's unrelated to the change here
There was a problem hiding this comment.
I see the issue _build_chat_completion_request_patch brings it in as kwargs.
if the tests pass, LGTM
2967bc9
into
litellm_internal_staging
* fix: pass websearch tool params * fix: load db websearch tool params * fix: merge search tools in proxy * fix: satisfy websearch lint budget * fix: enforce websearch tool auth * fix: preserve search tools on empty sync * chore: rerun circleci
Relevant issues
Linear ticket
Pre-Submission checklist
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review)
Screenshots / Proof of Fix
I reproduced this against a DB-backed proxy with
websearch_interceptionenabled and nosearch_toolsin config. The search tool existed only inLiteLLM_SearchToolsTable, created through the UI, withsearch_provider=exa_aiand a stored API key.EXA_API_KEYwas unset in the proxy processThe pre-fix
/v1/messagesrequest failed by falling back to Perplexity becausellm_router.search_toolsdid not include the DB/UI tool:After this fix, proxy search-tool sync builds the router list as config tools plus DB tools, with DB tools taking precedence on duplicate
search_tool_name. The same DB-backed/v1/messagesrequest succeeds:{ "stop_reason": "end_turn", "error": null, "content_types": ["text"] }Relevant proxy log lines:
Type
Bug Fix
Test
Changes
Proxy search-tool sync now updates
llm_router.search_toolsto the merged view of config search tools plus DB/UI search tools. DB tools take precedence when the samesearch_tool_nameexists in both places, matching the/search_tools/listbehavior. Empty config plus empty DB syncs now skip the router update instead of clearing existing search toolsWebsearch interception continues to select from
llm_router.search_tools, but now preserves and forwards the selected tool's fulllitellm_paramsintolitellm.asearch, withsearch_providerkept as the explicit argument and remaining non-null params forwarded as kwargsWebsearch interception now runs the same key and team search-tool authorization checks as the direct
/searchroute before forwarding stored search-tool credentials tolitellm.asearch. The check is applied to the normal agentic-loop path and the/v1/messagesshort-circuit pathAdded focused regression coverage for router-backed UI-style search tools passing all params, DB/config search-tool merging, startup router sync, and key/team search-tool allowlist enforcement before
litellm.asearchis calledLocal checks run:
make pre-commitwas retried, butuv sync --group proxy-devfailed while buildinggrpcio==1.78.0with an incompatible macOS x86_64 wheel. Because the sync did not complete, earlier runs also failed the later Prisma generation step with missingprismapackage metadataReplacement for fork-head PR #32157. This PR uses an upstream branch in BerriAI/litellm