feat(mcp): report every tool failure as an MCP error - #12429
Conversation
Adds ProwlerMCP, the FastMCP subclass every sub-server is now built
from. Its tool() wraps whatever it registers -- the decorator forms and
the direct call BaseTool uses -- so a failure leaves any tool as a
ToolError, which the client reads as isError: true.
Applied at the base class rather than by hand because forgetting it is
silent: every server now sets mask_error_details=True, so an unwrapped
tool would answer "Error calling tool 'x'" and nothing else. ToolError
bypasses that masking, which is what lets the servers mask by default
and still say something useful.
No tool changes yet. Tools that still return {"error": ...} keep working
exactly as before; they are converted surface by surface in the PRs
above this one. What changes here is that a failure which used to escape
as a raw exception is now described by render_tool_error.
The rules this establishes are in AGENTS.md and the developer guide,
so the conversions have something to be checked against.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
A changelog fragment is a small Markdown file named If this PR does not need a changelog entry, add the |
|
✅ No Conflicts No conflict markers, and the branch merges cleanly into its base. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/mcp-error-rendering #12429 +/- ##
=========================================================
Coverage 92.16% 92.17%
=========================================================
Files 311 312 +1
Lines 44952 44994 +42
=========================================================
+ Hits 41431 41472 +41
- Misses 3521 3522 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🔒 Container Security ScanImage: ✅ No Vulnerabilities DetectedThe container image passed all security checks. No known CVEs were found.📋 Resources:
|
🔎 Container Security Scan (Grype)Image: ✅ Nothing BlockingNo findings at critical or high severity. Not blocking at this cutoff — medium: 5, low: 2. 5 finding(s) excluded by 📋 Resources:
|
1 participant
Context
Stack 2 of N — based on #12428. Review that one first; this PR's diff is only what sits on top of it.
#12428 added the vocabulary for describing a failure. This PR is what makes every tool use it, and the rulebook the remaining PRs in the stack are checked against.
Description
ProwlerMCP(lib/server.py) is aFastMCPsubclass that overridestool()to wrap whatever it registers intool_errors.FastMCP.tool()is the single funnel every registration goes through — the@server.tool()and bare@server.tooldecorator forms, and the directmcp.tool(fn)callBaseTooluses to auto-register — so overriding it covers all of them at once. All four servers are now built from it.It is applied at the base class rather than by hand because forgetting it is silent and expensive: this PR also sets
mask_error_details=Trueeverywhere, so an unwrapped tool answersError calling tool 'x'and nothing else, while its name, schema and description all still look correct.ToolErroris aFastMCPErrorandFastMCP._call_toolre-raises those untouched, so a rendered message reaches the client verbatim, past every mount and past the masking. That is what lets the servers mask by default and still say something useful.tool_errorswraps the callable handed tomcp.tool(), not the class attribute — so only the MCP boundary is normalised. A tool calling another tool internally still sees the real, typed exception and can branch on it.Nothing about the tools themselves changes here. Every tool that returns
{"error": ...}today keeps doing exactly that; those are converted surface by surface in the PRs above. What changes is that a failure which used to escape as a raw exception is now described byrender_tool_errorinstead.The rulebook.
AGENTS.mdgains an### Errorssection and the developer guide's### Error Handlingsection is rewritten. Those exist in this PR rather than at the end of the stack for a specific reason: without them the guide would spend the next several PRs telling contributors to "return structured error responses instead of raising exceptions", which is now exactly wrong. The parts of the guide that describe things not yet built (the Hub helpers,ScanCreationResult(status="created_unconfirmed")) are held back.Steps to review
Start with
lib/server.py— it is 37 lines and it is the whole mechanism.Then
mcp_server/tests/lib/test_server.py, which covers the four ways a tool can be registered and asserts the wrapper is invisible to FastMCP's schema generation. A wrapper that loses the signature takes the parameters with it, leaving a tool that looks registered and is unusable;test_wrapping_does_not_disturb_the_published_toolis the guard.tests/test_server.pygainstest_no_tool_disappears_between_registration_and_the_client, which compares each sub-server's tool count against what the mount actually publishes. That is the check that would catch a wrapper silently dropping a tool.Worth knowing before changing
tool_errors:exclude_args=. That path rebuilds the function from__code__, which on a wrapper is the wrapper's own. Nothing passes it today.inspect.iscoroutinefunction, neverasyncio.iscoroutinefunction. The asyncio one is deprecated from Python 3.14 and removed in 3.16; sincerequires-pythonhas no upper bound and the suite runs withfilterwarnings = ["error"], swapping it in would fail the tests on any 3.14 interpreter — which CI does not currently cover.Checklist
MCP Server
no-changelog.License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.