Uh oh!
There was an error while loading. Please reload this page.
feat(core): support registerTool/registerResource/registerPrompt in MCP integration - #20071
Merged
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>
Contributor
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. |
Contributor
size-limit report 📦
|
Contributor
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.
|
betegon
marked this pull request as ready for review
April 1, 2026 12:31
Uh oh!
There was an error while loading. Please reload this page.
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>
betegonforce-pushed
the
feat/mcp-register-api-support
branch
from
April 1, 2026 19:37
68356d4 to
8984639CompareJPeer264
approved these changes
Apr 2, 2026
JPeer264
left a comment
Member
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.
Uh oh!
There was an error while loading. Please reload this page.
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>
Uh oh!
There was an error while loading. Please reload this page.
…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.
Uh oh!
There was an error while loading. Please reload this page.
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>…ess-mcp-v2 Without .npmrc, pnpm resolves @sentry/* from the public registry instead of Verdaccio, so the published validateMcpServerInstance (v1-only) rejects the v2 McpServer and no instrumentation is applied — causing the 30s timeout. Also fix the mcp.transport assertion: NodeStreamableHTTPServerTransport proxies onmessage to its inner WebStandardStreamableHTTPServerTransport, so constructor.name in the wrapper is the inner class. Use the broader /StreamableHTTPServerTransport/ regex (matching the v1 test pattern). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
s1gr1d added a commit
that referenced
this pull request
Apr 7, 2026
Nuxt 5 is still in development and currently in a nightly version. The E2E tests might fail due to changes but this should not block CI as long as v5 is in nightly. This adds the `optional` label to the test. E.g. unblocking this PR: #20071
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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