feat(core): support registerTool/registerResource/registerPrompt in MCP integration#20071
feat(core): support registerTool/registerResource/registerPrompt in MCP integration#20071
Conversation
…CP integration The @modelcontextprotocol/sdk introduced register* methods alongside the legacy tool/resource/prompt API in 1.x, and made them the only option in 2.x. - MCPServerInstance now accepts both old and new method names - validateMcpServerInstance accepts servers with either API set - wrapAllMCPHandlers instruments whichever methods are present - captureHandlerError maps register* names to the same error categories Co-Authored-By: claude-sonnet-4-6 <noreply@anthropic.com>
- Add createMockMcpServerWithRegisterApi() to test utilities - Test validation accepts register*-only servers and rejects invalid ones - Test that registerTool/registerResource/registerPrompt get wrapped - Add registerTool handler to node-express, node-express-v5, tsx-express e2e apps Co-Authored-By: claude-sonnet-4-6 <noreply@anthropic.com>
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Core
Deps
Other
Bug Fixes 🐛
Internal Changes 🔧Core
Deps
Other
🤖 This preview updates automatically when you update the PR. |
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Call echo-register tool in SSE transport tests across node-express, node-express-v5, and tsx-express, verifying that tools/call transactions are recorded for handlers registered via registerTool. Co-Authored-By: claude-sonnet-4-6 <noreply@anthropic.com>
…ror response When a tool handler threw an error, completeSpanWithResults() was ending the span without setting its status to error. This caused all MCP tool spans to appear with span.status=ok in Sentry, breaking the failure_rate() metric in the MCP insights dashboard. The fix passes hasError=true to completeSpanWithResults() when the outgoing JSON-RPC response contains an error object, setting the span status to internal_error directly on the stored span (bypassing getActiveSpan() which doesn't return the right span at send() time). Co-Authored-By: claude-sonnet-4-6 <noreply@anthropic.com>
68356d4 to
8984639
Compare
JPeer264
left a comment
There was a problem hiding this comment.
Nice addition. It would be nice if there would be an additional e2e test that has the v2 of the @modelcontextprotocol/sdk as well. Right now we only test against v1
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Potential double-wrapping when both API sets exist
- Added sentry_wrapped marker to prevent handlers from being wrapped twice when legacy API delegates to new API.
Or push these changes by commenting:
@cursor push 826164bfd2
Preview (826164bfd2)
diff --git a/packages/core/src/integrations/mcp-server/handlers.ts b/packages/core/src/integrations/mcp-server/handlers.ts
--- a/packages/core/src/integrations/mcp-server/handlers.ts
+++ b/packages/core/src/integrations/mcp-server/handlers.ts
@@ -41,7 +41,13 @@
* @returns Wrapped handler function
*/
function createWrappedHandler(originalHandler: MCPHandler, methodName: keyof MCPServerInstance, handlerName: string) {
- return function (this: unknown, ...handlerArgs: unknown[]): unknown {
+ // Check if handler is already wrapped to prevent double-wrapping
+ // when both legacy and new API methods exist and one delegates to the other
+ if ((originalHandler as { __sentry_wrapped__?: boolean }).__sentry_wrapped__) {
+ return originalHandler;
+ }
+
+ const wrappedHandler = function (this: unknown, ...handlerArgs: unknown[]): unknown {
try {
return createErrorCapturingHandler.call(this, originalHandler, methodName, handlerName, handlerArgs);
} catch (error) {
@@ -49,6 +55,11 @@
return originalHandler.apply(this, handlerArgs);
}
};
+
+ // Mark the handler as wrapped
+ (wrappedHandler as { __sentry_wrapped__?: boolean }).__sentry_wrapped__ = true;
+
+ return wrappedHandler;
}
/**
diff --git a/packages/core/test/lib/integrations/mcp-server/mcpServerWrapper.test.ts b/packages/core/test/lib/integrations/mcp-server/mcpServerWrapper.test.ts
--- a/packages/core/test/lib/integrations/mcp-server/mcpServerWrapper.test.ts
+++ b/packages/core/test/lib/integrations/mcp-server/mcpServerWrapper.test.ts
@@ -178,4 +178,45 @@
}).not.toThrow();
});
});
+
+ describe('Double-wrapping prevention', () => {
+ it('should not double-wrap handlers when both tool() and registerTool() exist and one delegates to the other', () => {
+ // Create a mock server with both APIs where tool() delegates to registerTool()
+ const registeredHandlers = new Map<string, any>();
+ const mockServerWithBothApis = {
+ registerTool: vi.fn((name: string, ...args: any[]) => {
+ const handler = args[args.length - 1];
+ registeredHandlers.set(name, handler);
+ }),
+ tool: vi.fn(function (this: any, name: string, handler: any) {
+ // Simulate legacy tool() delegating to registerTool()
+ return this.registerTool(name, {}, handler);
+ }),
+ resource: vi.fn(),
+ prompt: vi.fn(),
+ connect: vi.fn().mockResolvedValue(undefined),
+ server: {
+ setRequestHandler: vi.fn(),
+ },
+ };
+
+ const wrapped = wrapMcpServerWithSentry(mockServerWithBothApis);
+
+ // Register a handler via the legacy tool() method
+ const originalHandler = vi.fn();
+ wrapped.tool('test-tool', originalHandler);
+
+ // Get the registered handler
+ const registeredHandler = registeredHandlers.get('test-tool');
+
+ // The handler should be wrapped (have the __sentry_wrapped__ marker)
+ expect((registeredHandler as any).__sentry_wrapped__).toBe(true);
+
+ // Verify that calling tool() only wraps once, not twice
+ // If double-wrapped, the handler would have nested wrappers
+ // We can verify this by checking that the registered handler is the wrapped version
+ // and not a double-wrapped version
+ expect(registeredHandler).not.toBe(originalHandler);
+ });
+ });
});This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Adds a new optional e2e test application that pins @modelcontextprotocol/sdk v2 (split into @modelcontextprotocol/server + @modelcontextprotocol/node) and exercises only the register* API (registerTool, registerResource, registerPrompt), which is the only API supported in v2. The app is marked as optional in CI since v2 is still in alpha. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dev-packages/e2e-tests/test-applications/node-express-mcp-v2/playwright.config.mjs
Show resolved
Hide resolved
…rrelation Add mcp.transport assertion (NodeStreamableHTTPServerTransport) and mcp.tool.result.content_count check to prove span correlation completes with results end-to-end, matching the coverage level of the v1 tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
getPlaywrightConfig starts 'node start-event-proxy.mjs' on port 3031 unconditionally. Without it the event proxy never starts and all waitForTransaction calls hang. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit effc0c9. Configure here.
dev-packages/e2e-tests/test-applications/node-express-mcp-v2/start-event-proxy.mjs
Outdated
Show resolved
Hide resolved
Was copied from node-express but not updated. waitForTransaction keyed on 'node-express-mcp-v2' would never match events from a proxy advertising 'node-express', causing all tests to hang. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@modelcontextprotocol/server declares it as an optional peer dependency, so pnpm doesn't install it automatically. Without it the app fails at startup with ERR_MODULE_NOT_FOUND. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The app runs as ESM ("type": "module") because the MCP SDK v2 packages
are ESM-only. Sentry must be loaded via --import before the app module
to instrument Express correctly; importing it inside app.ts is too late.
Extracts Sentry.init() into instrument.mjs and starts the app with
node --import ./instrument.mjs dist/app.js, matching the pattern used
by tsx-express.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>


The
@modelcontextprotocol/sdkintroducedregisterTool,registerResource, andregisterPromptas a new API in 1.x, and in 2.x these are the only methods available — the oldtool/resource/promptnames are gone.Before this change, servers using the new API would silently get no instrumentation:
validateMcpServerInstancewould reject them (it only checked for the old names), sowrapMcpServerWithSentrywould return the unwrapped instance. The cloudflare-mcp e2e app already usedregisterTooland was affected by this.Changes
MCPServerInstancetype now includes optionalregisterTool?,registerResource?,registerPrompt?alongside the legacy methods (also made legacy ones optional with@deprecatedtags since 2.x removed them)validateMcpServerInstancenow accepts instances with eithertool+resource+prompt+connectorregisterTool+registerResource+registerPrompt+connectwrapAllMCPHandlersconditionally wraps whichever set of methods exists on the instancecaptureHandlerErrormapsregisterTool→tool_execution,registerResource→resource_execution,registerPrompt→prompt_executionregisterToolhandlers added to the node-express, node-express-v5, and tsx-express e2e appsThe existing
wrapMethodHandlerlogic (intercepts the last argument as the callback) works identically for both old and new signatures, so no changes were needed there.Closes #16666