dynamic mcp: harden client catalog discovery#772
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbdf599280
ℹ️ 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".
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces MCP dynamic tool loading with telemetry tracking, tool store enhancements with namespacing helpers, MCP client caching with concurrency-safe invalidation, and per-server slug-based tool naming. It propagates context throughout the MCP client lifecycle and adds tool retrieval override metadata. ChangesMCP Dynamic Tool Loading and Telemetry
Tool Store Enhancements and MCP Tool Namespacing
MCP Client Caching, Invalidation, and Tool Discovery
MCP Client Manager and User Client Integration
Test Updates and Cache Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp/client.go (1)
163-164:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against nil
pluginAPIbefore session validation.On Line 164,
c.pluginAPIis dereferenced whensessionID != "". If this client is constructed without a plugin API, this path panics instead of returning a typed error.Suggested fix
func (c *EmbeddedServerClient) CreateClient(ctx context.Context, userID, sessionID string) (*Client, error) { // Validate session exists before creating transport (unless empty for tool discovery) if sessionID != "" { + if c.pluginAPI == nil { + return nil, fmt.Errorf("plugin API is required when sessionID is provided") + } mmSession, err := c.pluginAPI.Session.Get(sessionID) if err != nil { return nil, fmt.Errorf("failed to get session: %w", err) }🤖 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/client.go` around lines 163 - 164, The code dereferences c.pluginAPI when sessionID != "" (calling c.pluginAPI.Session.Get) which will panic if c.pluginAPI is nil; add a nil check for c.pluginAPI before calling Session.Get and return a typed error (e.g., ErrPluginAPIUnavailable or a wrapped error) when it's nil, then only call c.pluginAPI.Session.Get when c.pluginAPI is non-nil and proceed to validate mmSession as before.
🤖 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 `@llm/tools.go`:
- Around line 665-668: The function toolArgsForLog should guard the argsGetter
before calling it: check if argsGetter == nil and return a safe string like "no
args getter" (or similar) instead of invoking it, then proceed to call
argsGetter(&raw) and handle its error as before; update the toolArgsForLog
function to perform this nil check (referencing the argsGetter parameter and the
toolArgsForLog function) so logging paths that pass a nil getter do not panic.
In `@mcp/client_manager.go`:
- Around line 205-213: The code currently persists transient connect errors by
calling userClient.appendInitialRemoteConnectError(connectErr) after
ConnectToPluginServer fails; instead, make plugin connect errors request-scoped
or cleared on successful connect: remove or avoid appending to the cached client
on transient failure and either collect connectErr in a local slice for this
request (return it with the response) or ensure you clear per-plugin entries on
subsequent successful ConnectToPluginServer calls (e.g., call a
clearInitialRemoteConnectError-like operation after a successful connect).
Update the logic around userClient.ConnectToPluginServer,
userClient.appendInitialRemoteConnectError and
userClient.InitialRemoteConnectErrors so the cached UserClient no longer
permanently stores transient plugin connect failures.
- Around line 184-188: Change GetToolsForUser to accept ctx context.Context as
its first parameter (i.e., func (m *ClientManager) GetToolsForUser(ctx
context.Context, userID string) ...) and remove the context.Background() call;
propagate this ctx into the downstream call to m.getClientForUser(ctx, userID)
and any other internal calls within GetToolsForUser so caller
cancellation/deadlines flow through the LLM entry path.
In `@mcp/user_clients.go`:
- Around line 371-377: The code currently creates a fallback callCtx :=
context.Background() before invoking client.CallToolWithMetadata which allows
tool calls to outlive request cancellation; instead, require a request-scoped
context: remove the context.Background() fallback, validate that llmContext and
llmContext.RequestContext are non-nil (or ensure a context is passed into the
resolver closure) and return an error immediately if missing, then call
client.CallToolWithMetadata using the verified llmContext.RequestContext along
with toolName, args and metadata.
---
Outside diff comments:
In `@mcp/client.go`:
- Around line 163-164: The code dereferences c.pluginAPI when sessionID != ""
(calling c.pluginAPI.Session.Get) which will panic if c.pluginAPI is nil; add a
nil check for c.pluginAPI before calling Session.Get and return a typed error
(e.g., ErrPluginAPIUnavailable or a wrapped error) when it's nil, then only call
c.pluginAPI.Session.Get when c.pluginAPI is non-nil and proceed to validate
mmSession as before.
🪄 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: 2d34a450-b143-49b9-abb0-256fdd5cd29c
📒 Files selected for processing (17)
llm/context.gollm/context_test.gollm/tools.gollm/tools_test.gomcp/client.gomcp/client_embedded_oauth_test.gomcp/client_manager.gomcp/client_manager_filter_test.gomcp/client_manager_test.gomcp/client_test.gomcp/retrieval_overrides.gomcp/tools_cache_test.gomcp/user_clients.gomcp/user_clients_test.gomcpserver/eval_helpers_test.gosearch/search_test.gotelemetry/integration_test.go
Co-authored-by: Cursor <cursoragent@cursor.com>
nickmisasi
force-pushed
the
dynamic-mcp-02-client-catalog
branch
from
dbdf599 to
eb5e85f
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
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 Actionable Findings
Reviewed the full diff (3014 additions, 424 deletions across 38 files) for this MCP client catalog hardening PR. The changes are security-positive overall.
Key areas examined:
-
channelAnalysisToolBottool expansion (api/api_channel.go): Overrides the bot's MCP tool allowlist for channel analysis. Initially appeared risky since channel analysis auto-approves all tool calls, but the downstreamAnalyzeChannelfunction correctly scopes the tool set to onlyread_channelandget_channel_infowith boundchannel_idparameters before the LLM sees them. The expanded catalog is only used to locate these two tools. -
Context propagation:
context.Background()removed from production MCP tool paths.GetToolsForUser,createAndStoreUserClient,ConnectToRemoteServers, andConnectToEmbeddedServerIfAvailablenow accept caller-scopedcontext.Context. Tool resolvers requireRequestContextand fail fast if missing. -
Tool namespacing and bare-name fallback (
llm/tools.go):lookupToolbare-name resolution correctly returns ambiguous results as not-found.mcpToolAllowedscopes allowlist checks byServerOrigin, preventing cross-server tool confusion. -
Concurrency safety: New mutex protection (
toolsMu,clientsMu,discoveryMu,notifyOwnerMu) onClientandUserClientsprevents race conditions during tool list invalidation and rediscovery. -
Plugin MCP client (
mcp/client.go):NewPluginClientcorrectly injects authenticated user ID viaheaderTransportfrom session-derived data, not user input. Tool list changed handlers properly invalidate caches. -
Trace logging (
llm/tools.go):TraceResolvedlogs full tool results whendoTraceis true, but this flag is not enabled in any production code path.
No medium, high, or critical vulnerabilities identified.
Sent by Cursor Automation: Find vulnerabilities
1 participant
Summary
Hardens MCP client catalog discovery with safer tool identity handling (prior to this, a tool registration with a name that was already registered would be dropped), plugin/embedded discovery refresh behavior, user-client synchronization, and retrieval override plumbing. All plumbing includes backwards compatibility support for existing tool policies that will not have the namespace plumbing setup by this branch.
Stack: 2/6
Ticket Link
N/A
Screenshots
N/A
Release Note
Made with Cursor
Summary by CodeRabbit
Release Notes
New Features
Improvements