Repository navigation
fix(mcp): stop reporting sandbox execute failures to Sentry - #917
Conversation
Caller/module errors (bad Notion filters, thrown strings, syntax issues) already land on mcp-event logs. Sending them to Sentry as warnings opens noise issues that look like platform bugs and trip triage automation.
📝 WalkthroughWalkthroughMCP sandbox failures now bypass Sentry reporting, while non-sandbox failures are captured at error level. A new test verifies MCP logging, Sentry capture behavior, and scope configuration. ChangesMCP observability
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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-917.kody-a99.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/worker/src/mcp/observability.ts (1)
61-71: 🩺 Stability & Availability | 🔵 TrivialSandbox errors now have zero Sentry visibility — confirm log-based alerting covers the gap.
Since sandbox failures no longer reach Sentry, the only remaining signal is the
mcp-eventconsole log line. Worth confirming there's log-based monitoring/alerting onsandboxError: truespikes so regressions in sandbox execution aren't silently missed.🤖 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/observability.ts` around lines 61 - 71, Verify that the structured mcp-event logging path records sandboxError: true and has active log-based monitoring or alerting for spikes in those events. If coverage is missing, add the necessary alerting using the existing observability conventions while keeping sandbox errors excluded from Sentry.packages/worker/src/mcp/observability.node.test.ts (1)
63-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the
captureMessagefallback branch.
reportMcpFailureToSentryhas two capture paths:captureExceptionforErrorcauses andcaptureMessagewhencauseisn't anErrorbuterrorMessageis set. Only thecaptureExceptionpath is exercised here;captureMessageis only asserted as not called (sandbox case). Consider adding a third case (non-sandbox, non-Errorcause, non-emptyerrorMessage) to cover thecaptureMessagebranch.🤖 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/observability.node.test.ts` around lines 63 - 102, Extend the observability test around the existing MCP failure cases to add a non-sandbox failure with a non-Error cause and non-empty errorMessage, exercising reportMcpFailureToSentry’s captureMessage fallback. Assert that sentryMock.captureMessage is called with the expected message while preserving the existing captureException assertions for Error causes.
🤖 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/observability.node.test.ts`:
- Around line 63-102: Extend the observability test around the existing MCP
failure cases to add a non-sandbox failure with a non-Error cause and non-empty
errorMessage, exercising reportMcpFailureToSentry’s captureMessage fallback.
Assert that sentryMock.captureMessage is called with the expected message while
preserving the existing captureException assertions for Error causes.
In `@packages/worker/src/mcp/observability.ts`:
- Around line 61-71: Verify that the structured mcp-event logging path records
sandboxError: true and has active log-based monitoring or alerting for spikes in
those events. If coverage is missing, add the necessary alerting using the
existing observability conventions while keeping sandbox errors excluded from
Sentry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f70906b-7503-4789-9b38-1c036586201b
📒 Files selected for processing (2)
packages/worker/src/mcp/observability.node.test.tspackages/worker/src/mcp/observability.ts
* Keep expected MCP caller errors out of Sentry Capability handlers throw plain Errors for caller mistakes (missing arguments, ids that do not resolve, preconditions the caller must clear) and every one of them opened a Sentry issue that reads like a platform bug. Five of the fourteen open kody-cloudflare issues are this class. Adds McpCallerError so a failure site can say the caller caused it, skips Sentry for parse_input failures (arguments never matched the declared schema), and adds a callerError payload flag for the search paths that report a caller mistake without throwing. Extends the same carve-out #916 and #917 made for connector disconnects and sandbox execute failures. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * Stop reporting OpenAPI missing path params to Sentry Caller mistakes like omitting organization_id_or_slug were thrown as plain Errors from buildOperationUrl and opened Sentry issues that look like platform bugs (KODY-CLOUDFLARE-1S). Throw McpCallerError instead so observability keeps them on mcp-event logs only. * Only skip Sentry for caller-attributed search batch failures Review feedback: a total entity-batch failure could hide platform incidents when every lookup failed for DB/load reasons. Preserve per-entry McpCallerError provenance and set callerError only when all failures are caller mistakes. Also mark search-detail not-found paths as McpCallerError. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Summary
Sentry issue 7631441133 (
Unknown: path must be an absolute Notion API path…) is a warning with tagsmcp.sandbox_error=true/mcp.tool=execute: a caller execute module passed a non-absolute Notion API path. That is a user/module error, not a platform defect.logMcpEventalready records these on the structuredmcp-eventlog line. Forwarding them to Sentry opens warning issues that look like platform bugs and spawn triage agents. This PR skips Sentry capture whensandboxErroris true; real MCP/platform failures still report at error level.Same root-cause fix as #915 (cherry-picked). Many sibling unresolved
/mcpwarnings sharemcp.sandbox_error=trueand will stop recurring once this deploys.Test plan
observability.node.test.tsasserts sandbox failures still emitmcp-eventbut do not callcaptureMessage/captureException, while non-sandbox failures still reportnpm run validate/ CI greenSystem recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@49893e20· Head:02ce0e6fClassification: extends — changes MCP failure reporting so sandbox/user-module errors stay on logs only and no longer open Sentry issues.
Primitives touched
mcp-serverSystem map
Sandbox execute failures used to become Sentry warnings via MCP observability; they now stay on the
mcp-eventlog path only.Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context.
Before / after
Before: execute sandbox failures (
sandboxError: true) were captured in Sentry as warnings (Unknown: …), including caller Notion path validation errors.After: sandbox failures remain on
mcp-eventlogs; only non-sandbox MCP failures are sent to Sentry at error level.Risk and invariants
Docs
No doc updates; behavior is an observability policy change covered by unit tests.
Summary by CodeRabbit