rebenitez1802 commented on code in PR #43374:
URL: https://github.com/apache/superset/pull/43374#discussion_r3855820137
##########
superset/mcp_service/middleware.py:
##########
@@ -815,12 +815,27 @@ async def on_call_tool(
"duration_ms": None,
},
)
+ # Flag the failure so clients can distinguish it from a
+ # successful call. Returning a ToolResult here (rather than
+ # letting the exception reach the SDK) is what avoids the
+ # bridge encoding failure described above; is_error rides
+ # along in the serialized result as a plain boolean, so the
+ # protocol stays conformant without reintroducing the
+ # unencodable error response.
return ToolResult(
content=[mt.TextContent(type="text", text=error_text)],
meta={"mcp_call_id": mcp_call_id} if mcp_call_id else None,
+ is_error=True,
)
if isinstance(result, ToolResult) and result.structured_content is not
None:
- result = ToolResult(content=result.content, meta=result.meta)
+ # Rebuilding to drop structured_content must preserve is_error,
+ # or a tool that reported failure alongside structured output
+ # would come back looking successful.
+ result = ToolResult(
+ content=result.content,
+ meta=result.meta,
+ is_error=result.is_error,
+ )
Review Comment:
🟡 **Medium — this `is_error` preservation is dead code end-to-end.**
On the success return path, `LoggingMiddleware.on_call_tool` rebuilds the
`ToolResult` and drops `is_error` at `middleware.py:606-612` *before* the
result reaches this outer `StructuredContentStripperMiddleware` (chain order,
outer→inner: `Stripper → RBAC → Logging → GlobalErrorHandler →
ResponseSizeGuard → tool`). So `result.is_error` here can only ever read
`False`, and the "tool reporting failure alongside structured output" case is
unreachable — no tool sets `is_error=True` today either.
`test_is_error_survives_structured_content_stripping` passes only because it
drives this middleware in isolation with a mocked `call_next`, bypassing
`LoggingMiddleware`, so it doesn't prove the end-to-end behavior.
To make this reachable, also preserve the flag at the sibling rebuild sites:
`LoggingMiddleware` (`:608`) and
`ResponseSizeGuardMiddleware._rewrap_as_tool_result` (`:1161`) both drop
`is_error`. Otherwise, consider dropping this half plus its test and scoping
the PR to the catch-all fix.
##########
superset/mcp_service/middleware.py:
##########
@@ -815,12 +815,27 @@ async def on_call_tool(
"duration_ms": None,
},
)
+ # Flag the failure so clients can distinguish it from a
+ # successful call. Returning a ToolResult here (rather than
+ # letting the exception reach the SDK) is what avoids the
+ # bridge encoding failure described above; is_error rides
+ # along in the serialized result as a plain boolean, so the
+ # protocol stays conformant without reintroducing the
+ # unencodable error response.
Review Comment:
🟡 **Medium — this rationale is technically inaccurate.**
Per fastmcp 3.4.7 `ToolResult.to_mcp_result`, a result with `is_error=True`
(or `meta` set) *does* round-trip to `CallToolResult(isError=True)` — so
returning it here doesn't avoid that envelope, it produces it. The accurate
(and narrower) reason this stays encodable: `structured_content` remains `None`
and only the boolean flips `false→true`, so the un-encodable
`structuredContent` payload implicated in the bridge failure above isn't
reintroduced. As the PR notes, that leg is still unverified against the live
Claude.ai bridge. Suggested rewording:
```suggestion
# Flag the failure so clients can distinguish it from a
# successful call. This still serializes to
# CallToolResult(isError=True) (see ToolResult.to_mcp_result);
# what keeps it encodable is that structured_content stays None
# and only the boolean flips false->true, not the structured
# payload implicated in the bridge failure above. That leg is
# unverified against the live Claude.ai bridge.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
