Repository navigation
fix(mcp): stop Sentry noise for unknown MCP server names - #959
Conversation
resolveMcpServerSetting threw plain Error for missing/blank server identifiers, so routine agent typos (e.g. reconnecting a removed server) opened Sentry issues. Throw McpCallerError so observability keeps them on mcp-event logs only.
📝 WalkthroughWalkthrough
ChangesMCP caller error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
🔎 Preview deployed: https://kody-pr-959.kody-a99.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/mcp/capabilities/mcp-servers/shared.node.test.ts (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert that the list lookup is skipped for blank input.
The test guards
getMcpServerSettingById, but the blank-input path should avoid both service calls.Suggested assertion
expect(mockModule.getMcpServerSettingById).not.toHaveBeenCalled() + expect(mockModule.listMcpServerSettings).not.toHaveBeenCalled()🤖 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/capabilities/mcp-servers/shared.node.test.ts` at line 65, Update the blank-input test alongside the existing getMcpServerSettingById assertion to also verify that the list lookup service is not called. Keep the test focused on confirming both service calls are skipped when the input is blank.
🤖 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/capabilities/mcp-servers/shared.node.test.ts`:
- Line 65: Update the blank-input test alongside the existing
getMcpServerSettingById assertion to also verify that the list lookup service is
not called. Keep the test focused on confirming both service calls are skipped
when the input is blank.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dfd1ae9f-17d6-4d66-becd-74d6ee68c578
📒 Files selected for processing (2)
packages/worker/src/mcp/capabilities/mcp-servers/shared.node.test.tspackages/worker/src/mcp/capabilities/mcp-servers/shared.ts
Address CodeRabbit feedback on the blank-identifier path.
Summary
Sentry KODY-CLOUDFLARE-29 fired when
mcp_server_reconnectwas called with a server name that is not saved (recipe-keeper; onlyhaexisted). That is a routine caller mistake (stale name / typo), not a platform defect.resolveMcpServerSettingthrew a plainError, so MCP observability reported it to Sentry. This PR throwsMcpCallerErrorinstead (same pattern as missing packages/runs), so these stay onmcp-eventlogs and out of Sentry.Changes
resolveMcpServerSetting→McpCallerErrorTest plan
shared.node.test.ts(3 tests)validategreenSystem recap — composes existing primitives (low risk)
Mode: recap · Base:
main@405751b2· Head:cursor/sentry-triage-kody-cloudflare-29-215aClassification: composes — reuses existing
McpCallerErrorso caller-resolvable MCP server lookup failures stay off Sentry; no new primitives or contracts.Primitives touched
mcp-client-serversMcpCallerErrorSystem map
Unknown MCP server names from reconnect/refresh/remove/set-enabled now classify as caller errors instead of platform exceptions.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
serverinresolveMcpServerSettingError→ Sentry issueMcpCallerError→ mcp-event onlySummary by CodeRabbit