gabotorresruiz commented on code in PR #44558:
URL: https://github.com/apache/superset/pull/44558#discussion_r4160548881
##########
superset/mcp_service/middleware.py:
##########
@@ -2075,6 +2079,26 @@ async def on_call_tool(
# the oversized path below rather than slipping through unmeasured.
actual_bytes = get_response_size_bytes(estimation_target)
+ # Choose the image budget from the returned payload, never request
args.
+ # Other formats of this tool retain the normal text-response budget.
+ content = (
+ estimation_target.get("content")
+ if isinstance(estimation_target, dict)
+ else None
+ )
+ if (
+ tool_name == "get_chart_preview"
+ and isinstance(content, dict)
+ and content.get("type") == "png"
+ ):
+ if actual_bytes > self.png_max_bytes:
+ raise ToolError(
+ f"PNG preview is {actual_bytes} bytes, exceeding the "
+ f"{self.png_max_bytes}-byte limit. Request smaller width
and "
+ "height, or use the URL preview format."
+ )
+ return response
Review Comment:
Just a small NIT, not worth another round on its own.
This branch returns before the warn log and raises without the
`event_logger.log(action="mcp_response_size_exceeded", ...)` that the text path
uses at line 2031, so a rejected PNG is completely silent. Same harness, both
paths:
```text
PNG over png_max_bytes -> is_error=True, middleware size/block log lines:
NONE
ascii over max_bytes -> "Response size warning for get_chart_preview:
200588 bytes (401% of 50000 limit)"
"Response blocked for get_chart_preview: 200588
bytes exceeds limit of 50000"
```
Every rejection costs a full Chromium capture, so this is the one case most
worth a counter, and there is no 80 percent warning against `png_max_bytes` the
way `warn_threshold` gives one against `max_bytes`. A `logger.warning` plus the
same `event_logger.log` call would cover both.
Tiny second thing while you are in here: `"get_chart_preview"` is the only
tool name hardcoded in this middleware, while every other grouping lives as a
named frozenset in `response_size_utils.py`. Nothing is wrong today,
`PNGPreview` is only reachable from this tool, but a constant there would match
the house style.
--
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]
