fix(mcp): prevent encoding error on tools/list when middleware raises#40446
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
aminghadersohi
force-pushed
the
amin/fix-mcp-list-tools-encoding
branch
from
541fe01 to
df9b623
Compare
StructuredContentStripperMiddleware.on_list_tools had no exception handling. When inner middleware (GlobalErrorHandlerMiddleware.on_message) raises ToolError, it propagated uncaught to the MCP SDK layer. The SDK cannot encode a ToolError as a tools/list response, producing the cryptic "encoding without a string argument" error visible to Claude.ai users. Fix: wrap the inner call in try/except and return [] on any exception. GlobalErrorHandlerMiddleware already logs and records event_logger entries for the underlying error, so no observability is lost. Also adds a unit test that exercises the full middleware chain and asserts the empty-list fallback.
aminghadersohi
force-pushed
the
amin/fix-mcp-list-tools-encoding
branch
from
df9b623 to
e8643a2
Compare
The duckdb 1.4.2→1.5.3 change in requirements/development.txt is an upstream maintenance task unrelated to this fix. It belongs in a separate PR.
There was a problem hiding this comment.
Pull request overview
This PR hardens Superset’s MCP middleware stack to avoid MCP SDK transport encoding failures when tools/list encounters an exception (notably when inner middleware raises ToolError), by ensuring tools/list always returns a list-shaped response.
Changes:
- Add exception handling to
StructuredContentStripperMiddleware.on_list_toolsto return[]whencall_nextraises. - Add a unit test exercising the real middleware chain to confirm exceptions during
tools/listproduce[]. - Update the pinned dev dependency
duckdbversion inrequirements/development.txt.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
superset/mcp_service/middleware.py |
Wrap on_list_tools middleware call with try/except and return an empty list on failure to prevent transport encoding errors. |
tests/unit_tests/mcp_service/test_middleware_logging.py |
Add test covering the tools/list exception path through the real middleware chain. |
requirements/development.txt |
Bump duckdb pinned version in dev requirements. |
|
The comment in the PR suggests that the |
pyproject.toml was missing from the change-detector python patterns, so check-python-deps was silently skipped on pyproject.toml-only PRs. This means dependency bumps in pyproject.toml could land without regenerating requirements/development.txt, leaving them out of sync.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40446 +/- ##
==========================================
- Coverage 64.20% 64.19% -0.01%
==========================================
Files 2592 2592
Lines 139226 139229 +3
Branches 32326 32326
==========================================
Hits 89385 89385
- Misses 48306 48309 +3
Partials 1535 1535
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
risk:ci-script
Runs uv-pip-compile.sh to sync requirements/development.txt with pyproject.toml after duckdb was bumped from 1.4.2 to 1.5.2 in apache#40381 but requirements were not regenerated.
aminghadersohi
requested review from
betodealmeida,
craig-rueda,
dpgaspar,
eschutho,
geido,
hainenber,
kgabryje,
mistercrunch,
nytai,
rusackas,
sadpandajoe and
villebro
as code owners
|
Bito Automatic Review Skipped – PR Already Merged |
Summary
StructuredContentStripperMiddleware.on_list_toolshad no exception handling. When inner middleware raisesToolError, it propagated to the MCP SDK layer which cannot encode aToolErroras atools/listresponse — producing the cryptic"encoding without a string argument"error visible to Claude.ai users across all MCP tools simultaneously.Fix: wrap the
call_nextcall inon_list_toolswith try/except and return[]on any exception.GlobalErrorHandlerMiddlewarealready logs and records event_logger entries for the underlying error, so no observability is lost.Note:
on_call_toolalready had this protection —on_list_toolswas the remaining unguarded path.Changes
superset/mcp_service/middleware.py— add try/except toStructuredContentStripperMiddleware.on_list_tools, return[]on exceptiontests/unit_tests/mcp_service/test_middleware_logging.py— new testtest_list_tools_exception_returns_empty_listexercises the full middleware chainTesting
All 15 middleware tests pass including the new one.