fix: MCP semantic filter regression — top_k not limiting tools#20322
fix: MCP semantic filter regression — top_k not limiting tools#20322shin-bot-litellm wants to merge 1 commit intomainfrom
Conversation
Two bugs from PR #20296 (MCP Semantic Filtering): 1. filter_tools() returned all tools when tool_router was None instead of lazily building the router from available_tools. This caused top_k to never limit results. 2. async_pre_call_hook() crashed with KeyError when data dict had no metadata key, causing the exception handler to return None (no filtering applied).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Greptile OverviewGreptile SummaryThis PR fixes two regression bugs from PR #20296 that broke the MCP semantic filtering feature: Bug 1: Bug 2: Both fixes are logically sound and align with the PR description. All 7 semantic filter tests pass locally. Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/semantic_tool_filter.py | Adds lazy router initialization when tool_router is None, fixing regression where top_k parameter was not limiting tools. Performance concern: creates SemanticRouter on first request if not pre-built. |
| litellm/proxy/hooks/mcp_semantic_filter/hook.py | Safely initializes metadata dict if missing before writing filter stats, preventing KeyError crash. Clean defensive programming fix. |
Sequence Diagram
sequenceDiagram
participant Client
participant Hook as SemanticToolFilterHook
participant Filter as SemanticMCPToolFilter
participant Router as SemanticRouter
participant LLM as LiteLLM Router
Client->>Hook: async_pre_call_hook(data with tools)
Hook->>Hook: Check if tools present
Hook->>Hook: Check if metadata exists
Note over Hook: Fix: Initialize metadata dict if missing
Hook->>Filter: extract_user_query(messages)
Filter-->>Hook: user_query
Hook->>Filter: filter_tools(query, available_tools)
alt Router not initialized (first call)
Filter->>Filter: Lazy build router
Note over Filter: Regression fix: Build router from available_tools
Filter->>Router: SemanticRouter(routes, encoder)
Router->>LLM: Generate embeddings
LLM-->>Router: Embeddings
Router-->>Filter: Router initialized
else Router already initialized
Note over Filter: Router pre-built on startup
end
Filter->>Router: semantic_router(query, limit=top_k)
Router-->>Filter: Top K matched tool names
Filter->>Filter: Get tools by names from available_tools
Filter-->>Hook: Filtered tools (limited by top_k)
Hook->>Hook: Store filter stats in metadata
Hook-->>Client: Modified data with filtered tools
| # Lazy-build router from available_tools if not already initialized | ||
| if self.tool_router is None: | ||
| verbose_logger.warning("Router not initialized - was build_router_from_mcp_registry() called on startup?") | ||
| return available_tools | ||
| verbose_logger.info( | ||
| "Router not pre-built, building from available_tools on first call" | ||
| ) | ||
| try: | ||
| self._build_router(available_tools) | ||
| except Exception as e: | ||
| verbose_logger.error(f"Failed to lazy-build router: {e}") | ||
| return available_tools | ||
|
|
||
| if self.tool_router is None: | ||
| verbose_logger.warning("Router still None after build attempt") | ||
| return available_tools |
There was a problem hiding this comment.
Lazy router initialization happens in the critical request path on first call — creating SemanticRouter with embeddings can add significant latency. Consider pre-building the router during startup via build_router_from_mcp_registry() or add a startup check/warning if the router isn't initialized.
Context Used: Rule from dashboard - What: Avoid creating new database requests or Router objects in the critical request path.
Why: Cre... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: litellm/proxy/_experimental/mcp_server/semantic_tool_filter.py
Line: 175:188
Comment:
Lazy router initialization happens in the critical request path on first call — creating `SemanticRouter` with embeddings can add significant latency. Consider pre-building the router during startup via `build_router_from_mcp_registry()` or add a startup check/warning if the router isn't initialized.
**Context Used:** Rule from `dashboard` - What: Avoid creating new database requests or Router objects in the critical request path.
Why: Cre... ([source](https://app.greptile.com/review/custom-context?memory=0c2a17ad-5f29-423f-a48b-371852ac4169))
How can I resolve this? If you propose a fix, please make it concise.
Regression Fix
Failing Jobs:
mcp_testing,litellm_mapped_tests_proxyCaused By: PR #20296 (MCP Semantic Filtering)
What Broke
Semantic filter
top_knot limiting tools — all tools returned instead of top_k subset. Two bugs:filter_tools()never built the semantic router — whentool_routerwasNone(tools passed directly, not via MCP registry startup), the method returned all tools unfiltered instead of lazily building the router fromavailable_tools.async_pre_call_hook()KeyError on missing metadata — when the requestdatadict had nometadatakey, writing filter stats crashed withKeyError, causing the exception handler to returnNone(no filtering applied).This Fix
filter_tools()now builds the semantic router fromavailable_toolson first call if not pre-built viabuild_router_from_mcp_registry().async_pre_call_hook()initializes the metadata dict if missing before writing filter stats.Testing
All 7 semantic filter tests pass locally:
test_semantic_filter_basic_filtering✅test_semantic_filter_top_k_limiting✅test_semantic_filter_hook_triggers_on_completion✅test_semantic_filter_disabled✅test_semantic_filter_empty_tools✅test_semantic_filter_extract_user_query✅test_semantic_filter_hook_skips_no_tools✅