fix(mcp): flag failed tool calls with isError - #43374
Conversation
Code Review Agent Run #d444feActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
The fix looks correct and well-reasoned. The core change is minimal and safe. Adding The Test coverage is thorough. The four cases ( One open question you've already flagged: confirming the serialized The field name correction (
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
| assert result.structured_content is None | ||
| assert result.is_error is True | ||
| assert "[REDACTED]" in text |
There was a problem hiding this comment.
Suggestion: This assertion was left at the end of the new test, but text is local to the preceding test_client_facing_text_is_sanitized method and is undefined here. The test therefore raises NameError after its intended assertions pass; remove the misplaced assertion or move it back to the sanitization test. [possible bug]
Severity Level: Major ⚠️
- ❌ MCP middleware regression test fails with NameError.
- ❌ CI cannot pass the affected unit-test module.
- ⚠️ The production middleware behavior remains unaffected by this assertion.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_middleware.py
**Line:** 2180:2180
**Comment:**
*Possible Bug: This assertion was left at the end of the new test, but `text` is local to the preceding `test_client_facing_text_is_sanitized` method and is undefined here. The test therefore raises `NameError` after its intended assertions pass; remove the misplaced assertion or move it back to the sanitization test.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. In the test To resolve this, you should remove the misplaced assertion, as it appears to be a copy-paste error from the preceding assert result.structured_content is None
assert result.is_error is TrueI have validated the issue and proposed the fix. Would you like me to check the other comments on this PR and implement fixes for them as well? tests/unit_tests/mcp_service/test_middleware.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43374 +/- ##
==========================================
- Coverage 66.81% 66.77% -0.05%
==========================================
Files 2876 2876
Lines 164243 164196 -47
Branches 37921 37896 -25
==========================================
- Hits 109744 109639 -105
- Misses 52314 52372 +58
Partials 2185 2185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
StructuredContentStripperMiddleware catches every exception and returns a
ToolResult carrying the error text, but never sets is_error. Since is_error
defaults to False, the call serializes as isError: false — so a permission
denial or an unhandled crash is indistinguishable from a successful call to any
client that inspects the flag rather than parsing the message text.
The catch-all itself has to stay: letting exceptions reach the MCP SDK produces
CallToolResult(isError=True) responses that some transports cannot encode. But
is_error rides along in the serialized result as a plain boolean, so setting it
restores protocol conformance without reintroducing the unencodable response.
Also preserve the flag when structured_content is stripped. That path rebuilds
the ToolResult and previously dropped is_error, so a tool reporting failure
alongside structured output came back looking successful.
Verified against fastmcp 3.4.7, the pinned version: the result serializes to
{"content": [...], "isError": true}, and is_error still defaults to False so
successful calls are unaffected.
Closes apache#43358
25e1208 to
c1575b3
Compare
Code Review Agent Run #a0b088Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
Fixes #43358, which @sadpandajoe invited a PR for.
The problem
StructuredContentStripperMiddleware.on_call_toolcatches every exception and returns aToolResultcarrying the error text, but never setsis_error. It defaults toFalse, so the call serializes asisError: false— a permission denial or an unhandled crash is indistinguishable from a successful call to any client that inspects the flag rather than parsing message text.The fix
Set
is_error=Trueon that last-resort result. The catch-all itself stays exactly as it is — letting exceptions reach the MCP SDK producesCallToolResult(isError=True)responses that some transports can't encode, which is the whole reason the handler exists.is_errorrides along in the serialized result as a plain boolean, so this restores protocol conformance without reintroducing the unencodable response.This is option 1 from the issue discussion, which @dosu also identified as the lowest-risk path. One correction to the snippet suggested there: the field is
is_error, notisError— the latter raisesTypeError.The PR also preserves the flag through the
structured_contentstrip. That path rebuilds theToolResultand droppedis_error, so a tool reporting failure alongside structured output came back looking successful.Verification
Against fastmcp 3.4.7, the pinned version:
The default staying
Falseis what keeps successful calls unaffected.Four tests cover it: a
ToolErrordenial and an unexpected exception are both flagged, a successful result is not, and the flag survives structured-content stripping.On the encoding concern raised in the original workaround comment — a
ToolResultwithis_error=Trueserializes as a plain dict with a boolean field, not as the exception object that triggered the bridge failure. I don't have access to the Claude.ai bridge to test that leg directly, so if a maintainer can confirm against it before merging, that would close the last open question.