Repository navigation
fix(mcp): report the raised exception on mcp 1.x low-level isError results - #1231
Open
lucasheriques wants to merge 3 commits into
Open
lucasheriques wants to merge 3 commits into
lucasheriques wants to merge 3 commits into
Conversation
Contributor
posthog-python Compliance ReportDate: 2026-11-11T20:01:21.746299+00:00 ✅ All Tests Passed!270/270 tests passed Capture_V1 Tests✅ 95/95 tests passed View Details
Capture_Ai Tests✅ 5/5 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
Feature_Flags_V4 Tests✅ 149/149 tests passed View Details
Feature_Flags_Local_Evaluation Tests✅ 4/4 tests passed View Details
|
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 565038f: 1 Should fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
| # except block, but with a prefixed message. Only the catch-all around | ||
| # the tool call passes an exception's bare text. | ||
| error = sys.exc_info()[1] | ||
| if isinstance(error, Exception) and error_message == str(error): |
Contributor
There was a problem hiding this comment.
P2 · Exclude validation results before formatting the exception
When validation rejects a large payload, str(error) pretty-prints the entire instance and schema even though the prefixed message excludes it from capture. This blocks the event loop (about 140 ms for 100,000 items); deeply nested inputs raise RecursionError, replacing the original validation response. Exclude validation results before rendering the exception.
Contributor
Author
There was a problem hiding this comment.
Fixed in afd7389. The wrapper now recognizes the decorator's validation results before rendering anything: the message has to equal one of the Input validation error: / Output validation error: prefixes plus the exception's own short .message. Only then does it compare against str(error), so a jsonschema ValidationError is never rendered. If inspecting the exception raises, the original result goes back untouched. A tool's own raise worded like a validation error is still recovered.
The parameterized test in test_lowlevel.py has two new rows. One sends a 400-deep payload that fails input validation. Before the fix, the response text was maximum recursion depth exceeded; now it keeps Input validation error: ... and the event stays Error. The other has the tool raise ValueError("Input validation error: quota exceeded"), which is still reported as ValueError. pytest posthog/test/mcp: 730 passed on mcp 1.30, 551 passed on mcp 2.3.
Contributor
SDK performance: posthog-pythonBenchmarks completed. No changes flagged for review. Flags require at least a 5% change in the median and all six pairs agreeing on direction. This is a noise filter, not proof of a performance change or a CI gate. View all results
Values are medians of paired measurements. Not flagged does not mean unchanged. Unavailable means a measurement could not be provided, not zero. Estimated allocation measurements retain their instrumentation uncertainty. RSS measures the whole process, not just SDK allocations; observed peaks are sampled checkpoints. Paired ranges in the diagnostics describe variability, not confidence intervals. Each shard uses its own runner; compare Before PR against With PR within a row, not across shards. Compared 102 metric rows; 102 rows shown in all results. Omitted or shortened data remain in |
dustinbyrne
approved these changes
Nov 11, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
The raised-exception recovery works in the exercised low-level MCP 1.x paths. Optional validation-path robustness feedback is inline.
AI-assisted review.
| # except block, but with a prefixed message. Only the catch-all around | ||
| # the tool call passes an exception's bare text. | ||
| error = sys.exc_info()[1] | ||
| if isinstance(error, Exception) and error_message == str(error): |
Contributor
There was a problem hiding this comment.
F1 — [P2] Skip validation results before formatting their exceptions
MINOR · OPTIONAL · introduced — posthog/mcp/_raised_tool_error.py:75 (RIGHT).
A low-level tool with an output schema requiring msg can return invalid, deeply nested output. Base returns the useful Output validation error: 'msg' is a required property; head instead returns maximum recursion depth exceeded while getting the repr of an object. Instrumentation should preserve the framework's validation response. This does not turn a successful call into a failure or crash the process: both results remain HTTP 200 with isError: true.
MCP 1.28.1 deliberately passes prefixed e.message for input/output validation (src/mcp/server/lowlevel/server.py:537–538,573–575, official revision 777b8d06710c140e3606b0d4598e2aa48546c266). The new comparison calls str(error) anyway; jsonschema 4.26.0 renders the entire failing instance and schema before the comparison rejects it. Besides the output-response regression, a transport-reachable invalid request containing 100,000 array items (~300 KB) now performs that unnecessary synchronous traversal. A like-for-like handler probe measured approximately 6 ms base / 120 ms head (illustrative single measurements, not a statistical benchmark); an actual HTTP test independently records zero full ValidationError renders on base versus one on head.
The governing MCP observer-only contract prohibits instrumentation from changing server responses or adding avoidable response latency.
Consider identifying framework validation-result paths before rendering the exception, and letting exception-inspection failures fall back to the original result. Preserve recovery for actual tool exceptions whose text happens to resemble a validation prefix.
Executed regression: Python 3.11.13, MCP 1.28.1, jsonschema 4.26.0, Pydantic 2.12.5, httpx 0.28.1. The following test uses public instrument(Server) and the real MCP Streamable HTTP ASGI transport; capture is a test sink. Normal empty arguments reach the tool, so incoming payload-depth limits do not explain the result: the deep value is server-produced.
import pytest
import httpx
import mcp.types as t
from mcp.server.lowlevel import Server
from mcp.server.streamable_http_manager import StreamableHTTPSessionManager
from posthog.mcp import instrument, MCPAnalyticsOptions
from posthog.test.mcp._helpers import FakeClient, flush_background
@pytest.mark.parametrize('depth', [0, 400])
async def test_instrumentation_preserves_invalid_output_response(depth):
server = Server('validation-regression')
@server.list_tools()
async def list_tools():
return [t.Tool(name='lookup', inputSchema={'type': 'object'}, outputSchema={
'type': 'object', 'required': ['msg'],
'properties': {'msg': {'type': 'string'}, 'payload': {}},
})]
@server.call_tool()
async def lookup(name, arguments):
payload = 0
for _ in range(depth):
payload = [payload]
return {'payload': payload}
instrument(server, FakeClient(), MCPAnalyticsOptions(
context=False, capture_model=False, enable_conversation_id=False,
))
manager = StreamableHTTPSessionManager(server, stateless=True, json_response=True)
async with manager.run():
async with httpx.AsyncClient(transport=httpx.ASGITransport(app=manager.handle_request),
base_url='http://localhost', headers={
'Accept': 'application/json, text/event-stream',
'MCP-Protocol-Version': '2025-06-18',
}) as client:
response = await client.post('/mcp', json={
'jsonrpc': '2.0', 'id': 1, 'method': 'tools/call',
'params': {'name': 'lookup', 'arguments': {}},
})
await flush_background()
assert response.status_code == 200
result = response.json()['result']
assert result['isError'] is True
assert result['content'][0]['text'] == "Output validation error: 'msg' is a required property"Save the test as test_output_validation_regression.py and run pytest -q test_output_validation_regression.py with the versions above. Equivalent isolated base/head executions produced: base ddeb000b6730befcfd5c3ca26fd0cba21193b5be: 2 passed. Head 565038fe9f50668b922f14dd0dcfc6e84e4e78f5: 1 passed, 1 failed, at the response-text assertion. The shallow-invalid-output control passes both. No fix was applied or tested.
The unusual output-depth trigger and already-failing call limit the severity; this is optional robustness/performance feedback, not a merge-blocking crash claim.
Contributor
Author
There was a problem hiding this comment.
Fixed in afd7389. The wrapper now recognizes the decorator's validation results before rendering anything: the message has to equal one of the Input validation error: / Output validation error: prefixes plus the exception's own short .message. Only then does it compare against str(error), so a jsonschema ValidationError is never rendered. If inspecting the exception raises, the original result goes back untouched. A tool's own raise worded like a validation error is still recovered.
The parameterized test in test_lowlevel.py has two new rows. One sends a 400-deep payload that fails input validation. Before the fix, the response text was maximum recursion depth exceeded; now it keeps Input validation error: ... and the event stays Error. The other has the tool raise ValueError("Input validation error: quota exceeded"), which is still reported as ValueError. pytest posthog/test/mcp: 730 passed on mcp 1.30, 551 passed on mcp 2.3.
lucasheriques
added a commit
that referenced
this pull request
Nov 11, 2026
Review on #1231 (PostHog Review, dustinbyrne): the message guard called str(error) on jsonschema ValidationErrors, which renders the whole instance and schema. On a deeply nested payload that raised RecursionError and replaced the SDK's validation response; on a large one it blocked the loop. A validation result is now matched by the prefix plus the error's own short .message first, and any failure while inspecting the exception leaves the original result untouched. A tool's own raise worded like a validation error is still recovered. Tested: parameterized test gains a 400-deep payload row (red before: response text 'maximum recursion depth exceeded') and a prefix-worded ValueError row. pytest posthog/test/mcp: mcp 1.30 730 passed / 11 skipped; mcp 2.3 bare 551 passed / 55 skipped. CI-exact mypy clean.
lucasheriques
force-pushed
the
lucas/mcp-error-status
branch
from
November 11, 2026 19:15
ddeb000 to
9f60b4a
Compare
lucasheriques
added a commit
that referenced
this pull request
Nov 11, 2026
Review on #1231 (PostHog Review, dustinbyrne): the message guard called str(error) on jsonschema ValidationErrors, which renders the whole instance and schema. On a deeply nested payload that raised RecursionError and replaced the SDK's validation response; on a large one it blocked the loop. A validation result is now matched by the prefix plus the error's own short .message first, and any failure while inspecting the exception leaves the original result untouched. A tool's own raise worded like a validation error is still recovered. Tested: parameterized test gains a 400-deep payload row (red before: response text 'maximum recursion depth exceeded') and a prefix-worded ValueError row. pytest posthog/test/mcp: mcp 1.30 730 passed / 11 skipped; mcp 2.3 bare 551 passed / 55 skipped. CI-exact mypy clean.
lucasheriques
force-pushed
the
lucas/mcp-lowlevel-raised-error
branch
from
November 11, 2026 19:16
afd7389 to
9e6d7cb
Compare
…sults On mcp 1.x the low-level @server.call_tool() decorator catches a tool's raise and returns CallToolResult(isError=True) before our handler sees it, so $mcp_tool_call had $mcp_error_type "Error", no frames and no $mcp_error_status (#1223). The decorator builds that result through server._make_error_result inside its except block, so an instance wrapper reads the live exception there and settles it into the same raised_tool_error slot standalone FastMCP uses. Validation failures also pass through that method inside an except block; only a message equal to str(exception) is the tool's own raise. mcp 2.x low-level needs no change: raw handlers raise through to our wrapper, and a scratch check showed RuntimeError, both frames and status 503. Tested: parameterized test in test_lowlevel.py (plain and wrapped 503, plain ValueError, input-validation guard). Red before on the three raising rows. Removing the message guard turns the validation row into ValidationError. pytest posthog/test/mcp: mcp 1.30 727 passed / 11 skipped; mcp 2.3 bare 551 passed / 55 skipped. CI-exact mypy: only the base branch's _exceptions.py:65.
…what it covers Pre-PR review: the message guard admits any exception the decorator's catch-all sees (tool body, list_tools cache refresh, result construction), not only the tool's own raise, and _make_error_result first ships in mcp 1.10. Comment and changeset now say so. Tested: pytest posthog/test/mcp on mcp 1.30, 727 passed.
Review on #1231 (PostHog Review, dustinbyrne): the message guard called str(error) on jsonschema ValidationErrors, which renders the whole instance and schema. On a deeply nested payload that raised RecursionError and replaced the SDK's validation response; on a large one it blocked the loop. A validation result is now matched by the prefix plus the error's own short .message first, and any failure while inspecting the exception leaves the original result untouched. A tool's own raise worded like a validation error is still recovered. Tested: parameterized test gains a 400-deep payload row (red before: response text 'maximum recursion depth exceeded') and a prefix-worded ValueError row. pytest posthog/test/mcp: mcp 1.30 730 passed / 11 skipped; mcp 2.3 bare 551 passed / 55 skipped. CI-exact mypy clean.
lucasheriques
force-pushed
the
lucas/mcp-error-status
branch
from
November 11, 2026 19:56
9f60b4a to
3d2ad57
Compare
lucasheriques
force-pushed
the
lucas/mcp-lowlevel-raised-error
branch
from
November 11, 2026 19:56
9e6d7cb to
2dc2abd
Compare
This branch has not been deployed
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
Part of #1223. On mcp 1.x, a tool registered with the low-level
Server's@server.call_tool()that raises now reports the exception it raised.$mcp_tool_callgets the raised class, the cause chain with frames, and$mcp_error_status. Before, the event said$mcp_error_type: "Error", with no frames and no status. #1229 listed this as an open gap.Stacked on #1229. Review only the top three commits (
68ff269,e18d71f,2dc2abd).The decorator catches the raise and builds
CallToolResult(isError=True)before our handler sees it. It builds that result throughserver._make_error_result(str(e)), inside itsexcept Exceptionblock. A wrapper on the server instance reads the live exception there withsys.exc_info(). It settles the exception into the sameraised_tool_errorslot that #1226 added for standalone FastMCP._make_error_resultinside anexcept, but with a prefixed message. The wrapper only takes an exception whosestr()equals the message, so validation failures stayError.instrument()runs before or after the tool is registered._make_error_resultfirst ships in mcp 1.10. On older 1.x the hook does nothing and the event is the same as today.RuntimeError, frames on both chain links, and status 503.httpx.HTTPStatusError(503)Error, no frames, no statusHTTPStatusError, frames, 503RuntimeErrorfrom a 503Error, no frames, no statusRuntimeError, chain with frames, 503ErrorError(unchanged)isErrorwith message💚 How did you test it?
test_lowlevel.pynow has a parameterized test: plainValueError, plain 503, wrapped 503, and an input-validation row. The three raising rows failed before the change. Without the message guard, the validation row reportsValidationError.pytest posthog/test/mcp: 728 passed, 11 skipped on mcp 1.30 + fastmcp 3.2. 551 passed, 55 skipped on mcp 2.3 without FastMCP.ruff check,ruff format --check, andmypy --no-site-packages --config-file mypy.ini .are clean.instrument(), unchanged client results, and frame redaction. All of it held. Findings:Falseafter the rebase.list_toolsraise during its cache refresh, an invalid tool schema, or a malformed return value. These are real server-side exceptions behind theisErrorresult, so naming their class is the intent.Not in this PR: on mcp 1.x, calling
instrument()before@server.call_tool()registers the handler drops every$mcp_tool_call. That gap already exists on the base branch.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
claude-opus-5-5), as a side session started from the feat(mcp): set $mcp_error_status from the raised exception's cause chain #1229 work._make_error_resultseam over wrapping the user's tool function orServer.call_tool. Both of those need to run before registration, andinstrument()usually runs after it./simplify(four Sonnet reviewers) suggested a shared wrap helper and shared test fixtures. I kept the duplication, because it is a few lines at two call sites.