Repository navigation
Instrument MCP server with Sentry Insights spans - #441
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughExtracts MCP server instantiation into a new factory that wraps the server with Sentry (payload recording disabled), updates MCP initialization to use the factory, adds Sentry span attribution in a generated UI resource, and adds test stubs plus a unit test verifying the wrapping. ChangesMCP Server Factory with Sentry Wrapping
Sequence Diagram(s) sequenceDiagram
participant MCPCaller
participant createKodyMcpServer
participant McpServer
participant SentryCloudflare
MCPCaller->>createKodyMcpServer: instantiate with options
createKodyMcpServer->>McpServer: new McpServer(implementation, options)
McpServer-->>createKodyMcpServer: returns server
createKodyMcpServer->>SentryCloudflare: wrapMcpServerWithSentry(server, {recordInputs:false,recordOutputs:false})
SentryCloudflare-->>createKodyMcpServer: wrapped server
createKodyMcpServer-->>MCPCaller: returns wrapped server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kentcdodds
marked this pull request as ready for review
May 12, 2026 01:16
Contributor
|
🔎 Preview deployed: https://kody-pr-441.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/worker/src/mcp/index.node.test.ts (1)
24-28: 💤 Low valueConsider improving test clarity for the assertion.
The test currently asserts that
wrapMcpServerWithSentrywas called withserver(the return value), which works because the mock returns its input unchanged. This creates a circular reference that may confuse future maintainers. Consider usingexpect.any(McpServer)or similar for the first argument to make the test's intent clearer.📝 Alternative assertion approach
expect(sentryMock.wrapMcpServerWithSentry).toHaveBeenCalledTimes(1) - expect(sentryMock.wrapMcpServerWithSentry).toHaveBeenCalledWith(server, { + expect(sentryMock.wrapMcpServerWithSentry).toHaveBeenCalledWith( + expect.any(Object), + { recordInputs: false, recordOutputs: false, - }) + }, + )Or verify the first argument separately:
+ const [[calledServer, calledOptions]] = sentryMock.wrapMcpServerWithSentry.mock.calls + expect(calledServer).toBe(server) + expect(calledOptions).toEqual({ + recordInputs: false, + recordOutputs: false, + })🤖 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 `@packages/worker/src/mcp/index.node.test.ts` around lines 24 - 28, The assertion currently checks wrapMcpServerWithSentry was called with the concrete return value `server`, creating a circular/ambiguous test; update the assertion to assert the first argument type instead (e.g. use expect.any(McpServer)) so intent is clear: replace the toHaveBeenCalledWith(server, {...}) check with toHaveBeenCalledWith(expect.any(McpServer), { recordInputs: false, recordOutputs: false }) or alternatively assert the call count and verify the first call's first argument separately via expect(sentryMock.wrapMcpServerWithSentry.mock.calls[0][0]).toEqual(expect.any(McpServer)) and then assert the options object.packages/worker/src/mcp/sentry-mcp-server.ts (1)
9-11: 💤 Low valueConsider adding an explicit return type annotation.
While TypeScript will infer the return type, adding an explicit annotation would improve code clarity and catch potential type mismatches earlier.
📝 Suggested return type annotation
export function createKodyMcpServer( options: ConstructorParameters<typeof McpServer>[1], -) { +): McpServer { return Sentry.wrapMcpServerWithSentry(🤖 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 `@packages/worker/src/mcp/sentry-mcp-server.ts` around lines 9 - 11, Add an explicit return type to the createKodyMcpServer function signature (e.g., : McpServer) to make the API clear and catch mismatches; update the signature of createKodyMcpServer to return the concrete McpServer type (or the appropriate Promise<McpServer> if the implementation is async) and import/fully qualify the McpServer type where needed so the compiler verifies the returned value matches the annotated type.
🤖 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.
Nitpick comments:
In `@packages/worker/src/mcp/index.node.test.ts`:
- Around line 24-28: The assertion currently checks wrapMcpServerWithSentry was
called with the concrete return value `server`, creating a circular/ambiguous
test; update the assertion to assert the first argument type instead (e.g. use
expect.any(McpServer)) so intent is clear: replace the
toHaveBeenCalledWith(server, {...}) check with
toHaveBeenCalledWith(expect.any(McpServer), { recordInputs: false,
recordOutputs: false }) or alternatively assert the call count and verify the
first call's first argument separately via
expect(sentryMock.wrapMcpServerWithSentry.mock.calls[0][0]).toEqual(expect.any(McpServer))
and then assert the options object.
In `@packages/worker/src/mcp/sentry-mcp-server.ts`:
- Around line 9-11: Add an explicit return type to the createKodyMcpServer
function signature (e.g., : McpServer) to make the API clear and catch
mismatches; update the signature of createKodyMcpServer to return the concrete
McpServer type (or the appropriate Promise<McpServer> if the implementation is
async) and import/fully qualify the McpServer type where needed so the compiler
verifies the returned value matches the annotated type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 20415a2c-d3db-4e92-b8ac-ab3dad2f7a12
📒 Files selected for processing (5)
packages/worker/src/mcp/index.node.test.tspackages/worker/src/mcp/index.tspackages/worker/src/mcp/resources/generated-ui-app-resource.tspackages/worker/src/mcp/sentry-mcp-server.tspackages/worker/src/test-support/sentry-cloudflare-stub.ts
kody-bot
marked this pull request as draft
May 12, 2026 01:35
kentcdodds
marked this pull request as ready for review
May 12, 2026 01:55
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Summary
Validation
node --input-type=module -e "import('@sentry/cloudflare').then(m=>console.log(typeof m.wrapMcpServerWithSentry))"npx vitest run --config vitest.node.config.ts packages/worker/src/mcp/index.node.test.tsnpm run typechecknpm run lint(passes with existing warnings)npm testSummary by CodeRabbit
Tests
Chores