fix(mcp): report the exception a tool raised when the SDK masks it - #907
Merged
Conversation
The MCP SDK's tool dispatch re-raises whatever a tool raised as a ToolError whose message starts "Error executing tool <name>", and mcp 2.1 masks the original text out of that message entirely, keeping it only on __cause__. The $mcp_error_message and $mcp_error_type scalars read $exception_list[0], so on mcp 2.1 every unexpected tool failure reported the same masked string and the failures view lost the reason. The scalars now read the entry behind the dispatch wrapper, which exceptions_from_error_tuple already records from __cause__. The wrapper is matched by type name and message prefix because each SDK major ships its own ToolError class. A wrapper without a chained cause is kept as is, and the $exception sibling still carries the full chain. This broke CI without a repo change: the rolling exclude-newer = "7 days" quarantine made mcp 2.1.0 (published Aug 24) eligible on Aug 31 at 19:04 UTC, between two runs of the same commit. The unpinned mcp>=2,<3 leg resolved 2.0.0 in the morning and 2.1.0 in the evening. mcp 2.1.1 ages in on Sep 1; the fix is verified against 2.0.0, 2.1.0, and 2.1.1. Generated-By: PostHog Desktop Task-Id: ec0cc27f-fc40-4994-b153-2bfe74de1edf
5 tasks
Contributor
posthog-python Compliance ReportDate: 2026-09-01 10:03:17 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
marandaneto
reviewed
Sep 1, 2026
marandaneto
left a comment
Member
There was a problem hiding this comment.
Automated advisory code review.
Two review findings on the dispatch-wrapper unwrap, both reproduced before fixing. A tool that invokes a failing tool is wrapped once per dispatch, so stepping past a single entry landed on the still-masked inner wrapper. The unwrap now walks every consecutive wrapper to the first real exception; a nested-server test asserts both the inner and the outer event report the root cause. Matching by type name and message prefix alone also unwrapped an application's own exception that happened to be named ToolError with a matching prefix, replacing the message the application chose to surface. The match now additionally requires the entry's recorded module to come from an SDK namespace (mcp., fastmcp.), verified against mcp 1.28.1, mcp 2.0.0/2.1.0/2.1.1, and standalone fastmcp. The unit tests use the real per-major ToolError classes, and a new test pins that a same-named application exception is kept. Generated-By: PostHog Desktop Task-Id: ec0cc27f-fc40-4994-b153-2bfe74de1edf
marandaneto
approved these changes
Sep 1, 2026
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
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.
💡 Motivation and Context
The MCP SDK CI matrix went red on Aug 31 with no repo change. The cause is the repo's rolling
exclude-newer = "7 days"quarantine: mcp 2.1.0 (published Aug 24, 19:04 UTC) aged into eligibility at 19:04 UTC, between two runs of the same commit, so the unpinnedmcp>=2,<3leg resolved 2.0.0 in the morning and 2.1.0 in the evening. Three of the four red jobs are fail-fast cancellations of the one real failure.mcp 2.1 masks unexpected tool exceptions: the dispatch wrapper's message becomes
Error executing tool <name>with the original kept only on__cause__(documented in the SDK attools/base.py).$mcp_error_messageand$mcp_error_typeread$exception_list[0], so every unexpected tool failure reported the same masked string and the failures view lost the reason a call failed, which is the one thing these properties exist to carry.The scalars now read the entry behind the SDK's dispatch wrapper.
exceptions_from_error_tuplealready records the__cause__chain, so the original exception is sitting right behind the wrapper; the match requires the type name (ToolError/UnexpectedToolError), theError executing toolmessage prefix, and a module from an SDK namespace (mcp.,fastmcp.), and it walks every consecutive wrapper because a tool invoking a failing tool is wrapped once per dispatch. A wrapper without a chained cause is kept as is, an application's own exception that merely shares the wrapper's name is kept, and the$exceptionsibling still carries the full chain, so error-tracking grouping is unchanged.On mcp 1.x the wrapper includes the original text, so the scalars there change from
Error executing tool boom: explode/ToolErrortoexplode/ValueError. The tool name is already its own property on the event.💚 How did you test it?
test_instrumented_server_failure_carries_them) passes untouched under every mcp version CI can resolve: 2.0.0 (yesterday's resolution), 2.1.0 (today's), and 2.1.1 (ages in Sep 1, verified with an--exclude-neweroverride).$exceptionsibling keeps the wrapper; a wrapper without a cause is kept; an application exception sharing the wrapper's name is kept; and a nested-server test asserts both the inner and outer events report the root cause. The unit tests import the real per-majorToolErrorclasses.main, the gate test and the unwrap test fail; against the first revision of this PR, both review findings reproduce as failing tests (nested wrappers and the same-named application exception); with the final change, all pass.posthog/test/mcpsuite: 235 passed on the lockfile env (mcp 1.28.1 + fastmcp 3.2.0), 215 passed + 13 skipped on each of mcp 2.0.0, 2.1.0, and 2.1.1.ruff format,ruff check, and the CI mypy command (throughmypy-baseline filter) pass._sanitize_exception_valuesmaps over every chain entry, not just the first.📝 Checklist
If releasing new changes
.sampo/changesets/(hand-written in thesampo addformat)🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written by Claude Code in a PostHog Desktop session directed by @bernatixer, after tracing why the MCP SDK matrix went red on #906 with a snapshot-only diff. Rejected alternatives: pinning the v2 leg below 2.1 (defeats the gate's purpose and ships masked messages to customers anyway), version-gating the test assertion (accepts the masked string as the product), and raising
ToolErrorin the test (stops exercising the unexpected-exception path customers actually hit). The SDK documents__cause__as the channel for the original error, so the instrumentation reads it.The advisory review's two blocking findings (nested double-wrapping, and type-name matching catching application exceptions) were both reproduced first and are fixed in the second commit: the match is gated on the entry's recorded SDK module and traverses consecutive wrappers.
Skills invoked: writing-tests, writing-pr-descriptions.
Created with PostHog Desktop