AurimasNav commented on code in PR #43374:
URL: https://github.com/apache/superset/pull/43374#discussion_r3860045790
##########
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:
> _Drafted with AI assistance._
Agreed, and thank you for the precise correction — I had verified
`to_mcp_result()` yields `CallToolResult(isError=True)` earlier and then wrote
a comment that implied the opposite. Applied your suggested wording verbatim in
dbb60b7.
##########
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:
> _Drafted with AI assistance._
Confirmed against the chain: `StructuredContentStripperMiddleware` is
registered first (`server.py:885`), and `LoggingMiddleware` (inner) rebuilds
the `ToolResult` without `is_error` on the success path, so the value at the
strip site was always `False`. No tool sets `is_error` today either.
Took your second option in dbb60b7: removed the strip-path preservation and
its test, so the PR is scoped to the catch-all return — which is reachable,
since this middleware is outermost and its own return goes straight to the SDK.
Preserving the flag end-to-end would mean touching the `LoggingMiddleware` and
`ResponseSizeGuardMiddleware` rebuild sites together; that belongs in its own
change if a tool ever sets the flag.
--
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]
