dynamic mcp: add registry and loading tools#773
Conversation
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 7 failuresOPENAI1. TestReactEval/[openai]_react_cat_message
2. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[openai]_bot_dm_tool_introspection
Anthropic
❌ Failed EvaluationsShow 7 failuresANTHROPIC1. TestReactEval/[anthropic]_react_cat_message
2. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[anthropic]_bot_dm_tool_introspection
This comment was automatically generated by the eval CI pipeline. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR implements a complete MCP tool discovery and dynamic loading system. It introduces a BM25 text search index, an in-memory tool registry using BM25 for relevance matching with name-based fallback, and two LLM-callable meta tools ( ChangesMCP Tool Discovery and Loading System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
mcp/dynamic_registry_test.go (1)
63-70: ⚡ Quick winUse a table-driven structure for these two query cases.
This function exercises multiple test cases and should be table-driven per repository test rules.
As per coding guidelines, "
**/*_test.go: Go tests must be table-driven when there is more than one test case".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/dynamic_registry_test.go` around lines 63 - 70, The test TestToolRegistrySearchEmptyQuery currently asserts two cases inline; convert it to a table-driven test that iterates over cases for empty and whitespace-only queries. Create a slice of structs (e.g., name and query) inside TestToolRegistrySearchEmptyQuery, loop over them with t.Run(case.name, func(t *testing.T) { ... }), construct the registry with NewToolRegistry and testRegistryTool as before, call registry.Search(case.query, 10) and assert require.Nil(t, ...) for each case; this keeps the same assertions but follows the repository rule requiring table-driven tests.mcp/bm25_test.go (1)
54-61: ⚡ Quick winConvert this multi-case test to a table-driven test.
This test currently validates more than one case inline; please convert it to a table-driven shape to match test conventions.
As per coding guidelines, "
**/*_test.go: Go tests must be table-driven when there is more than one test case".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/bm25_test.go` around lines 54 - 61, Replace the inline multi-case assertions in TestBM25EmptyQueryReturnsNil with a table-driven subtest loop: define a slice of test cases (each with a name and query string for the empty and whitespace cases), construct the BM25 index once using NewBM25Index, then iterate over cases calling t.Run(case.name, func(t *testing.T) { require.Nil(t, idx.Search(case.query, 10)) }). Keep references to NewBM25Index and idx.Search and ensure each case has its own name for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/dynamic_registry.go`:
- Around line 152-153: The method searchWithIndex currently calls r.bm25.Search
(and later may call ClosestMatches) without checking that r.bm25 is non-nil,
which will panic for a zero-value ToolRegistry; update searchWithIndex in
ToolRegistry to first guard if r.bm25 == nil (or otherwise not initialized) and
return an empty []ToolSearchResult (or a safe default) instead of calling
r.bm25.Search/ClosestMatches, ensuring any callers of searchWithIndex receive a
valid empty result when the index is absent.
---
Nitpick comments:
In `@mcp/bm25_test.go`:
- Around line 54-61: Replace the inline multi-case assertions in
TestBM25EmptyQueryReturnsNil with a table-driven subtest loop: define a slice of
test cases (each with a name and query string for the empty and whitespace
cases), construct the BM25 index once using NewBM25Index, then iterate over
cases calling t.Run(case.name, func(t *testing.T) { require.Nil(t,
idx.Search(case.query, 10)) }). Keep references to NewBM25Index and idx.Search
and ensure each case has its own name for clarity.
In `@mcp/dynamic_registry_test.go`:
- Around line 63-70: The test TestToolRegistrySearchEmptyQuery currently asserts
two cases inline; convert it to a table-driven test that iterates over cases for
empty and whitespace-only queries. Create a slice of structs (e.g., name and
query) inside TestToolRegistrySearchEmptyQuery, loop over them with
t.Run(case.name, func(t *testing.T) { ... }), construct the registry with
NewToolRegistry and testRegistryTool as before, call registry.Search(case.query,
10) and assert require.Nil(t, ...) for each case; this keeps the same assertions
but follows the repository rule requiring table-driven tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ad1bb676-7703-4a3a-81e5-493bb9d8f01e
📒 Files selected for processing (6)
mcp/bm25.gomcp/bm25_test.gomcp/dynamic_registry.gomcp/dynamic_registry_test.gomcp/meta_tools.gomcp/meta_tools_test.go
nickmisasi
force-pushed
the
dynamic-mcp-02-client-catalog
branch
from
dbdf599 to
eb5e85f
Compare
nickmisasi
force-pushed
the
dynamic-mcp-03-registry-meta-tools
branch
from
e6c9846 to
e5ef694
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
nickmisasi
force-pushed
the
dynamic-mcp-03-registry-meta-tools
branch
from
e5ef694 to
0928d23
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Review: No Vulnerabilities Found
Reviewed the dynamic MCP tool registry, BM25 search, and search_tools/load_tool meta-tools for security concerns.
Analysis
Scope of this PR: Pure data-structure and search logic. Introduces an immutable ToolRegistry built from a pre-provided list of tools, a BM25 text-search index, and two meta-tools that let the LLM discover and load tools at runtime.
Key security invariant: The ToolRegistry must be constructed with only tools the requesting user is authorized to use. This is enforced upstream by Context.KeepMCPTool and Context.DisabledMCPServerOrigins (applied "before strict registry construction" per field docs), with the integration layer in the next PR in the stack.
Checked and verified:
- No permission bypass:
load_toolonly loads from the pre-filtered registry (exact name match required). No wildcard or fuzzy loading. The registry is immutable post-construction. - No data leakage:
search_toolsreturns only names and summaries from the registry scope. Error responses fromload_tool(closest matches) also only reference tools already in the user's filtered registry. - No secrets exposure: No credentials, tokens, or admin-only data in search/load responses. Tool schemas are MCP server-defined metadata, not sensitive.
- No injection vectors: Search queries are only used for BM25 token scoring against pre-loaded data. No SQL, no shell, no template rendering.
- No DoS risk: BM25 tokenization is O(n) with no backtracking. Index size is bounded by admin-configured MCP tool count.
- Proper null safety: All code paths handle nil context, nil registry, nil tool store, and empty inputs gracefully with structured error responses.
The code delegates the permission-filtering responsibility to the caller (registry constructor), which is a sound architectural pattern. The filtering fields (KeepMCPTool, DisabledMCPServerOrigins, PreloadedMCPTools) are already defined in llm.Context with clear documentation of when they're applied.
Sent by Cursor Automation: Find vulnerabilities
1 participant
Summary
Adds the dynamic MCP tool registry, BM25 search, and the
search_tools/load_toolmeta-tools used to discover and materialize MCP tools on demand.Stack: 3/6
Ticket Link
N/A
Screenshots
N/A
Release Note
Made with Cursor
Summary by CodeRabbit
Release Notes