Repository navigation
feat(mcp): add caller-scoped PNG chart previews - #44558
Conversation
Code Review Agent Run #596c25Actionable Suggestions - 0Additional Suggestions - 1
Review 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44558 +/- ##
=======================================
Coverage 82.83% 82.84%
=======================================
Files 3017 3017
Lines 192709 192782 +73
Branches 44914 44923 +9
=======================================
+ Hits 159635 159706 +71
- Misses 30040 30041 +1
- Partials 3034 3035 +1
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| chart = find_chart_by_identifier(chart_id) | ||
| if chart is None: | ||
| return ChartError(error="Chart not found", error_type="NotFound") | ||
| security_manager.raise_for_access(chart=chart) |
There was a problem hiding this comment.
Suggestion: Denied chart access raises an authorization exception inside the broad handler, so callers receive RenderError instead of the expected Forbidden response.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_preview.py
**Line:** 1146:1146
**Comment:**
*Api Mismatch: Denied chart access raises an authorization exception inside the broad handler, so callers receive `RenderError` instead of the expected `Forbidden` response.
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 fixThere was a problem hiding this comment.
Valid catch. Fixed in 3522c87: SupersetSecurityException from raise_for_access is now caught explicitly around the render worker and mapped to Forbidden, instead of falling into the broad except Exception that returned RenderError.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 3522c87.
Added a dedicated except SupersetSecurityException handler around the render call, returning a ChartError with error_type="Forbidden" instead of treating the denial as a rendering failure.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1478574.
_run_png_render catches SupersetSecurityException from the render worker and returns a ChartError with error_type="Forbidden".
If that's not right, unresolve this thread and CodeAnt will leave it open.
| manager._cleanup() | ||
|
|
||
| try: | ||
| return await asyncio.to_thread(render) |
There was a problem hiding this comment.
Suggestion: Cancellation cannot stop render, leaving Chromium processes and worker threads occupied until screenshot timeouts expire; repeated cancelled requests can exhaust rendering capacity.
Assessment: 🔴 Critical · 🔁 Occurrence: Rarely · 🏷️ Resource leak
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_preview.py
**Line:** 1184:1184
**Comment:**
*Resource Leak: Cancellation cannot stop `render`, leaving Chromium processes and worker threads occupied until screenshot timeouts expire; repeated cancelled requests can exhaust rendering capacity.
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 fixThere was a problem hiding this comment.
Acknowledged as a known limitation. asyncio.to_thread cannot be cancelled mid-render, but occupation is bounded: the screenshot call has its own timeout and finally: manager._cleanup() always releases the browser. Killing it early would need process isolation, which is out of scope for this change.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag cancellation-related resource leaks for the bounded render operation in this module when the screenshot call has its own timeout and browser resources are released in a finally cleanup block; early process isolation is out of scope.
Applied to:
superset/mcp_service/chart/tool/get_chart_preview.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| content = ( | ||
| await _generate_png_preview(chart.id, request) | ||
| if request.format == "png" | ||
| else preview_generator.generate() | ||
| ) |
There was a problem hiding this comment.
Suggestion: PNG rejection occurs only after chart lookup and transient form-data handling, so invalid PNG requests can return NotFound or other errors instead of UnsupportedFormat.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Incomplete implementation
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_preview.py
**Line:** 1471:1475
**Comment:**
*Incomplete Implementation: PNG rejection occurs only after chart lookup and transient form-data handling, so invalid PNG requests can return `NotFound` or other errors instead of `UnsupportedFormat`.
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 fixThere was a problem hiding this comment.
Checked the ordering: every PNG-unsupported precondition (unsaved chart, guest, form_data_key, extra_form_data) still resolves to UnsupportedFormat, just after lookup. A nonexistent chart returns NotFound, which is the correct contract (the resource does not exist regardless of format). No wrong error type is reachable, so leaving the ordering as is.
| assert isinstance(result, ChartError) | ||
| assert "secret" not in result.error | ||
| browser.assert_not_called() |
There was a problem hiding this comment.
Suggestion: The denied case accepts any ChartError, so authorization failures converted to RenderError still pass and the test misses the required Forbidden contract.
Assessment: 🟠 Major · 🔁 Occurrence: Often · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
**Line:** 1955:1957
**Comment:**
*Api Mismatch: The denied case accepts any `ChartError`, so authorization failures converted to `RenderError` still pass and the test misses the required `Forbidden` contract.
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 fixThere was a problem hiding this comment.
Agreed. Fixed in 3522c87: the denied case now raises the real SupersetSecurityException and asserts error_type == "Forbidden", so a regression to RenderError would fail the test. The no-secret-leak assertion is kept.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 3522c87.
The denied-case test now explicitly asserts result.error_type == "Forbidden" while still verifying that sensitive error text is not exposed.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 2dde258.
The denied-case test now explicitly asserts that the returned ChartError has error_type equal to "Forbidden", while still checking that the authorization message is not exposed.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 71d8fc7.
The denied-case test now explicitly asserts that the returned ChartError has error_type equal to "Forbidden", while still checking that the authorization message is not exposed.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 3dbb1e1.
The denied-case test now explicitly asserts that the returned ChartError has error_type equal to "Forbidden", rather than accepting any ChartError.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 136e405.
The denied-case test now explicitly asserts that the returned ChartError has error_type equal to "Forbidden", rather than accepting any ChartError.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of f62453d.
The denied-case test now explicitly asserts that the returned ChartError has error_type equal to "Forbidden", rather than accepting any ChartError.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 5d6c555.
The denied-case test now explicitly asserts that the returned ChartError has error_type="Forbidden", while still verifying that the denial message is not leaked.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1478574.
The denied-case test now explicitly asserts result.error_type == "Forbidden".
If that's not right, unresolve this thread and CodeAnt will leave it open.
raise_for_access raises SupersetSecurityException inside the render worker, where the broad except Exception handler converted it to RenderError. Catch it explicitly so denied charts return Forbidden. Tighten test_png_preview_authorizes_before_browser: the denied case now raises the real SupersetSecurityException and asserts the Forbidden contract instead of accepting any ChartError. Addresses the codeant-ai findings on apache#44558.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #5adb1eActionable 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 |
|
Addressed the dimension-description suggestion from Bito review #596c25: PNGPreview.width and height now describe the rendered pixel dimensions in the generated tool schema. Both fields remain required integers. The schema assertion and 89 chart-preview tests pass. |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hi @deepujain, thanks for this, and thanks for a PR description that actually explains the design. I went at the "caller scoped" claim hard and I could not break it, so the headline is good news. I am leaving two comments on the code. One of them I think blocks: with the shipped MCP defaults I do not believe a real chart PNG can reach the caller at all.
What I ran, on 100ee4c:
pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py tests/unit_tests/utils/webdriver_test.py -q, 161 passed, which reproduces your number exactly. The wholetests/unit_tests/mcp_servicesuite is also green, 4911 passed and 2 skipped.- Mutation check on the
Forbiddenmapping you pushed: deleting theexcept SupersetSecurityExceptionclause makestest_png_preview_authorizes_before_browser[denied]fail, so that fix is genuinely pinned by a test rather than passing for free. - Authorization differential.
MachineAuthProvider.get_auth_cookiesfor two callers produces two different signed session cookies that decode to_user_id='11'and_user_id='29', so the browser really is the caller and not a service identity. AGuestUserreturnsForbiddenand never launches a browser even withEMBEDDED_SUPERSEToff, whereis_guest_user()isFalseand theguest_scopecheck does not fire, becauseGuestUsercarries noid. The worker also re-resolves the chart throughChartDAO.find_by_idwithChartFilterapplied, and/superset/slice/<id>/is still@has_accessprotected, so there are two independent gates on the way to the pixels. - I checked the MCP response cache too, since a shared cache in front of a caller scoped tool would quietly undo all of the above.
superset/mcp_service/caching.pyrefuses to enable caching unless an operator setsdangerously_share_cache_across_principals, so that is fine as shipped. - Response path: rendered a real chart in headless Chromium and pushed those exact bytes through a real
FastMCPclient with the realResponseSizeGuardMiddleware. Numbers are in the inline comment.
CI is fully green on this head, including unit-tests (current), test-postgres (current), test-mysql, test-sqlite and pre-commit (current).
I read the CodeAnt and Bito threads first and I am not repeating any of them. Two nits I am deliberately dropping so they do not dilute the above: the form_data_key lookup still runs before PNG rejects the request, and the PNGPreview docstring describes the caching strategy rather than the content.
Happy to dig into the size one with you if you want a hand.
| return PNGPreview( | ||
| data=base64.b64encode(image).decode("ascii"), | ||
| width=image_width, | ||
| height=image_height, | ||
| ) |
There was a problem hiding this comment.
This block worries me a bit. With the shipped defaults I do not think a real chart can come back through this path at all.
ResponseSizeGuardMiddleware runs on every tool call, and get_chart_preview is in none of INFO_TOOLS, DATA_QUERY_TOOLS, COMMITTED_WRITE_TOOLS or STRING_FIELD_TRUNCATION_TOOLS (superset/mcp_service/utils/response_size_utils.py), and it is not in excluded_tools in MCP_RESPONSE_SIZE_CONFIG (superset/mcp_service/mcp_config.py). So _handle_oversized_response falls all the way through to the hard ToolError. The default budget is DEFAULT_MAX_RESPONSE_BYTES = 50_000.
I verified it on this branch. I rendered an ECharts line chart in headless Chromium at the default 800x600 with device_scale_factor=1, put those exact bytes into a PNGPreview inside a real ChartPreview, and called a tool named get_chart_preview through a real FastMCP client with the real middleware attached:
raw PNG bytes : 96272
base64 chars : 128364
measured response bytes : 128944
guard max_bytes : 50000
is_error: True
"Response too large: 128,944 bytes (limit: 50,000) ... Reduction needed: ~61%"
The caller pays for the full Chromium launch and capture and then gets an error instead of the image, and that error suggests narrowing a query that has no narrowing knob. Sweeping the viewport on the same chart, 320x240 fit under the budget and 400x300 was already over, so there is no width/height inside your 64..4096 range that yields a useful preview.
Options, in the order I would pick them:
- add
get_chart_previewtoexcluded_toolsinMCP_RESPONSE_SIZE_CONFIG, and say in the README that a PNG response is not bounded bymax_bytes - or return the image as an MCP image content block instead of base64 inside the JSON payload, so it never counts against the text budget
- or keep the guard and bound the output (downscale, or JPEG/WebP) with a documented ceiling
Whichever you pick, could we get a test in test_get_chart_preview.py that pushes a realistically sized PNG through ResponseSizeGuardMiddleware.on_call_tool and asserts a PNGPreview comes back rather than a ToolError? Every current PNG test stops at _generate_png_preview, which is exactly why this is invisible today. EMBEDDED_DIMENSIONS_MAX_PAGE_SIZE in superset/mcp_service/semantic_layer/schemas.py is a nice in repo example of budgeting a response against this same limit.
Or am I missing a place where the guard is relaxed for this tool?
There was a problem hiding this comment.
Fixed in 3dbb1e1. The default response-size configuration now excludes get_chart_preview, and the README explicitly states that all preview formats are outside max_bytes unless the operator removes that exclusion. A real 800x600 PNG exceeding the default budget now traverses ResponseSizeGuardMiddleware.on_call_tool intact; the new test fails with ToolError on the previous code. The existing default-config assertion was updated to the new contract. All 308 preview, middleware and WebDriver tests and staged hooks pass.
| so those stay server-side. | ||
| """ | ||
| try: | ||
| return await asyncio.to_thread(render) |
There was a problem hiding this comment.
Not a blocker, and I am not reopening the cancellation point you already answered. This is a different property of the same call.
asyncio.to_thread submits to the loop's default executor (it is literally loop.run_in_executor(None, ...)), which is the same pool that superset/mcp_service/composite_token_verifier.py:184 and superset/mcp_service/guest_token_verifier.py:153 use for API key and guest token verification. That pool is min(32, cpu_count + 4) threads, and a render holds its thread for the whole capture, which by your own note is not released early when the request is cancelled. So a handful of concurrent format="png" calls can sit on the same threads every other request needs for transport auth.
Would a dedicated bounded executor, or an asyncio.Semaphore around the render, be worth it here? It also gives you a natural place to cap how many Chromium instances one process will launch at once, since _PlaywrightBrowserManager() is created per call.
There was a problem hiding this comment.
Fixed in 3dbb1e1 with a dedicated two-worker PNG executor. It preserves caller ContextVars and leaves the default authentication pool available. The regression holds two render workers, cancels one awaiting request, verifies the third capture cannot start early, and confirms default-pool work still completes. That test fails on the old shared executor. Running captures still finish under the existing screenshot timeouts; the README documents this limit.
Code Review Agent Run #e977bfActionable 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 |
Code Review Agent Run #9d3525Actionable Suggestions - 0Additional Suggestions - 2
Review 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 |
|
Addressed the process-lifetime suggestion from Bito review #9d3525 in 136e405. The README now states that each MCP worker starts after any process fork and that interpreter shutdown waits for active captures. It also makes explicit that request cancellation does not stop the browser operation or release its worker slot early; captures retain the configured screenshot timeouts. This documents the existing tradeoff without changing execution behavior. Staged pre-commit hooks and the Markdown render pass. The new CodeQL run failed before analysis in both languages because its init and analysis actions loaded different versions (4.38.0 and 4.38.1). This is an action-version mismatch, not a reported code finding; the workflow needs a rerun with consistent action versions. |
|
Fixed the current inventory-budget failure in f62453d. The PNG description keeps its saved-chart, non-guest caller and unsupported-filter constraints with less repeated wording. The existing budget and serializer are unchanged. The current-master integration reproduced the 3403 > 3400 failure before this patch; all 416 inventory, preview, schema and webdriver tests pass afterward. The original PR branch also passes 270 focused tests and staged pre-commit hooks. Only the two-line description change was pushed; the temporary upstream integration was used for validation. Hosted checks on the new head are pending. |
Code Review Agent Run #4c2c27Actionable 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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hi @deepujain, thanks for turning this around so quickly, and for doing both pieces properly rather than only the one I blocked on.
Clearing my original block: the PNG really does reach the caller now. I re-ran the exact round trip that failed last time and the caller gets a PNGPreview with byte identical data instead of a ToolError. The executor change is right too. I am leaving two comments, one of which I would still hold for: with the tool out of the size guard, nothing bounds the response in bytes any more, and the 4096 ceiling this PR allows reaches about two megabytes.
What I ran, on f62453d, in a clean uv venv:
pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py tests/unit_tests/utils/webdriver_test.py tests/unit_tests/mcp_service/test_middleware.py -q, 308 passed, which matches your number exactly. Fulltests/unit_tests/mcp_service: 4913 passed, 2 skipped.- Both new regression tests are genuinely load bearing, which I like. Reverting
_run_png_rendertoasyncio.to_threadfailstest_png_workers_do_not_starve_auth_or_release_on_cancellation, and droppingget_chart_previewback out ofexcluded_toolsfailstest_png_preview_passes_default_response_size_guardwithResponse blocked for get_chart_preview: 1921611 bytes exceeds limit of 50000. - Re-ran the authorization differential, since two new code paths on a caller scoped tool is where scoping tends to drift. It did not drift. A
GuestUseris still refused both ways,UnsupportedFormatwhenguest_scope.is_guest_read()fires andForbiddenwhenEMBEDDED_SUPERSETis off so it does not fire, with no browser launched in either case. Two different callers still get distinct signed session cookies decoding to_user_id='11'and_user_id='29'. On the new executor the render still runs off the caller thread (mcp-png_0), seesg.user.idas the caller, does not inherit the request'sg, callsraise_for_accessexactly once, and leaves the caller'sgintact afterwards. - Size behaviour: rendered real charts in headless Chromium and pushed each response through a real
fastmcp.Clientwith the realResponseSizeGuardMiddlewareand your new default config. Numbers are in the inline comments.
CI is green on this head, 47 passing and nothing failing, including unit-tests (current), test-postgres (current), test-mysql, test-sqlite and pre-commit (current).
| if not (64 <= width <= 4096 and 64 <= height <= 4096): | ||
| return ChartError( | ||
| error="PNG dimensions must be between 64 and 4096 pixels.", | ||
| error_type="ValidationError", |
There was a problem hiding this comment.
The PNG genuinely reaches the caller now, I confirmed that end to end, thank you. But with get_chart_preview out of the size guard, this clamp is the only thing left bounding the response, and it bounds pixels rather than bytes.
I rendered the same ECharts line chart at both corners of the range you allow here and pushed each result through a real fastmcp.Client with the real ResponseSizeGuardMiddleware and the new default excluded_tools:
viewport pixel_density response bytes delivered
800x600 1 128,944 yes
4096x4096 1 859,666 yes
4096x4096 2 2,014,190 yes
pixel_density is not caller controlled, it comes from WEBDRIVER_WINDOW in superset/config.py, but a deployment that sets it to 2 doubles each rendered dimension, so that last row is reachable. All three came back with is_error=False and a base64 payload byte identical to what went in. A single tool result of roughly two megabytes of base64 text is the exact shape ResponseSizeGuardMiddleware was written to stop (its class docstring in superset/mcp_service/middleware.py says so), and an agent asked for a large preview will pass 4096 because the README advertises it.
Could we clamp the encoded output rather than only the viewport? A configurable byte or megapixel ceiling that either downscales before encoding or returns a ValidationError naming the largest size that fits would do it, with the ceiling written into the README next to the 64..4096 line. A test next to test_png_preview_passes_default_response_size_guard that renders at 4096x4096 and asserts the response stays under that ceiling would lock it in.
This is the only thing I would still hold for, and I am happy to help if the clamp turns out to be awkward to place.
| "generate_explore_link", # Returns URLs | ||
| "open_sql_lab_with_context", # Returns URLs | ||
| "search_tools", # Returns tool schemas for discovery (intentionally large) | ||
| "get_chart_preview", # Rendered PNG previews exceed the text budget |
There was a problem hiding this comment.
Not a blocker, but worth on the record because it reaches past PNG.
The guard is keyed on the tool name, so this entry also turns it off for ascii, table and vega_lite. ascii and table are row capped at 50 and 20 so they are fine, but vega_lite runs at _preview_row_limit(form_data, 1000) and then embeds the whole result set inline as "data": {"values": data}, so it can get large. I built a 1000 row spec of plausible chart columns and ran it through the same real client twice:
1000 row vega_lite preview: 142,677 bytes
with master's excluded_tools -> is_error=True (blocked at 50,000)
with this branch's list -> is_error=False (130,618 bytes delivered)
Also worth knowing: #39719 removed get_chart_preview from this list on purpose and added test_default_config_checks_chart_preview to pin it, which is the assertion this PR inverts. That decision was about the non PNG formats, so it may be worth looping in whoever owns it before flipping it.
If you would rather keep that invariant, skipping the guard on the payload's content.type == "png" instead of on the tool name gives PNG its escape hatch and leaves the other three formats checked.
github-actions
Bot
added
the
requires:rebase
f62453d to
5d6c555
Compare
raise_for_access raises SupersetSecurityException inside the render worker, where the broad except Exception handler converted it to RenderError. Catch it explicitly so denied charts return Forbidden. Tighten test_png_preview_authorizes_before_browser: the denied case now raises the real SupersetSecurityException and asserts the Forbidden contract instead of accepting any ChartError. Addresses the codeant-ai findings on apache#44558.
github-actions
Bot
removed
the
requires:rebase
Code Review Agent Run #7621c3Actionable Suggestions - 0Additional Suggestions - 6
Review 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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hi @deepujain, this is the right fix, and it is a better one than I asked for. Taking the tool back out of excluded_tools and giving PNG its own png_max_bytes budget keyed on the returned content.type is exactly the narrow shape, and it restores the invariant the April change had pinned instead of leaving it inverted.
Approving. The response is bounded in bytes now, the whole documented 64..4096 range fits at the default pixel_density, and over the ceiling the caller gets an actionable error naming the actual size and two ways out. One NIT inline, which should not hold the PR.
What I ran, on 5d6c555, in a fresh uv venv I checked was genuinely isolated (include-system-site-packages = false, every package resolving from the venv, verified from outside the repo after being bitten by a fake one last round):
- The three affected suites, 319 passed. Full
tests/unit_tests/mcp_service, 6309 passed and 4 skipped. - Pushed real headless-Chromium renders of the same chart through a real
fastmcp.Clientwith the real middleware built from the real default config, sweeping to the boundary rather than trusting the documented range:
800x600 d1 128,944 bytes delivered, data intact
2048x2048 d1 416,158 delivered
3000x3000 d1 613,754 delivered
3500x3500 d1 729,678 delivered
4096x4096 d1 859,666 delivered, data intact
2048x2048 d2 962,026 delivered
2300x2300 d2 1,076,698 rejected
4096x4096 d2 2,014,190 rejected
The real ceiling at pixel_density: 2 for this chart sits between 2048 and 2300 square, inside the documented range, which is the case your new README paragraph already calls out. Every delivered payload decoded byte identical to what went in, so nothing is being silently rescaled.
vega_litegot its protection back. The same 1000 row spec, 142,677 bytes, is blocked again atmax_bytes, andtest_default_config_checks_chart_previewis restored to its original name, docstring and assertion.- Both halves are pinned. Deleting the PNG branch from the guard fails 4 tests; putting
get_chart_previewback intoexcluded_toolsfails 4 more, including the April one. - The new branch reads
contentoff the extracted payload, so I checked it both ways:structured_output_enabledTrueandFalseboth deliver, 128,904 bytes each. - Re-ran the authorization differential, fourth round, still no drift. Guest refused both ways with no browser launched, two callers still getting distinct signed sessions (
_user_id='11'and_user_id='29'), and the render still onmcp-png_0seeingg.user.idas the caller, not inheriting the request'sg, and callingraise_for_accessonce. Encoding is still inside the worker with the clamp downstream of it.
On CI: 44 passing, and the single red check is playwright-tests (chromium, /app/prefix), which itself reports 1 flaky, 89 passed and zero failures. The flake is a 422 on the fixture creating apiRequestContext.post in chart-list.spec.ts:49, it passed on retry inside that same job, and playwright-tests (chromium) passed on this commit. I am confident it has nothing to do with this change, but playwright-tests-required will need a re-run to go green before merge.
| 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 |
There was a problem hiding this 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:
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.
github-actions
Bot
added
the
requires:rebase
Signed-off-by: Deepak Jain <deepujain@gmail.com>
raise_for_access raises SupersetSecurityException inside the render worker, where the broad except Exception handler converted it to RenderError. Catch it explicitly so denied charts return Forbidden. Tighten test_png_preview_authorizes_before_browser: the denied case now raises the real SupersetSecurityException and asserts the Forbidden contract instead of accepting any ChartError. Addresses the codeant-ai findings on apache#44558. Signed-off-by: Deepak Jain <deepujain@gmail.com>
The Forbidden-mapping except branch pushed _generate_png_preview to complexity 11. Moving the asyncio.to_thread call plus its two except branches into _run_png_render keeps the error contract identical and restores the function under the limit. Signed-off-by: Deepak Jain <deepujain@gmail.com>
Upstream apache#44574 landed the messages.pot regeneration on master; the babel-extract and unit-tests (check_pot_drift_test.py) failures on this PR were base drift, not contributor code. Fresh CI will run against the new merge commit with the fixed base pot. Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
5d6c555 to
1478574
Compare
CodeAnt PR Risk: Low Risk
Assessed commit: |
github-actions
Bot
removed
the
requires:rebase
| from superset import is_feature_enabled | ||
| from superset.mcp_service.auth import _mcp_tool_call_context | ||
| from superset.utils.core import override_user | ||
| from superset.utils.screenshots import validate_screenshot_image |
There was a problem hiding this comment.
superset/utils/screenshots.py already defines PNG_MAGIC_BYTES, used by validate_screenshot_image right below this. Importing it here too avoids the two checks drifting if that byte sequence ever changes.
| from superset.utils.screenshots import validate_screenshot_image | |
| from superset.utils.screenshots import PNG_MAGIC_BYTES, validate_screenshot_image |
| image = driver.get_screenshot(url, "chart-container", user=user) | ||
| if ( | ||
| image is None | ||
| or not image.startswith(b"\x89PNG\r\n\x1a\n") |
There was a problem hiding this comment.
| or not image.startswith(b"\x89PNG\r\n\x1a\n") | |
| or not image.startswith(PNG_MAGIC_BYTES) |
| @@ -1447,7 +1575,11 @@ def __init__(self, form_data: Dict[str, Any]): | |||
| action="mcp.get_chart_preview.preview_generation" | |||
| ): | |||
| preview_generator = PreviewFormatGenerator(chart, request) | |||
There was a problem hiding this comment.
NIT: preview_generator gets built and then thrown away on the png branch. Not a big deal, but may as well only construct it where it's used.
| preview_generator = PreviewFormatGenerator(chart, request) | |
| content = ( | |
| await _generate_png_preview(chart.id, request) | |
| if request.format == "png" | |
| else PreviewFormatGenerator(chart, request).generate() |
|
Nothing to add on top of @gabotorresruiz's review, honestly, that was about as thorough a pass as this gets. Re-ran the suite myself and traced the executor, the A couple of small things that shouldn't hold this up, left as suggestions. One more that doesn't land in this diff so I can't suggest it inline: Approving. |
Master's PNG chart previews (#44558) grew the chart tool definitions without refreshing the native tool inventory, which fails the unit suite once merged here. Apply the generate_chart, get_chart_preview and update_chart entries from #45511 unchanged and recompute the catalog totals to include this branch's get_dashboard_info growth. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
SUMMARY
MCP clients need a chart image they can display inline. Existing preview formats provide text, data or a link. This adds
format: "png"toget_chart_preview, so a client can display the image rendered by Superset. The response includes base64 PNG bytes and the image dimensions, addressing the rendered-image part of #41114. Existing formats and the ASCII default stay unchanged.Chart and image-export checks run before the browser authenticates as the caller. Each capture owns its browser and MCP ORM session; no shared thumbnail cache or report executor is used. Guest users, unsaved chart state and extra dashboard filters return an explicit unsupported-format error for PNG. The additional metadata requested in #41114 remains separate work.
PNG previews use a separate 1,000,000-byte serialized payload limit, configurable with
MCP_RESPONSE_SIZE_CONFIG["png_max_bytes"]. It counts base64 data and preview metadata. Larger images return an error asking for smaller dimensions or a URL preview; images are not silently rescaled. ASCII, table and Vega-Lite previews retain the existingmax_byteslimit. As with the existing guard, this measures the payload, not the MCP transport envelope.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable to existing UI: this adds an MCP response format. A local real-Chromium fixture rendered two concurrent caller-specific SVG pages through the production machine-auth cookie path and verified distinct images and identities.
TESTING INSTRUCTIONS
With Playwright/Chromium installed on the MCP host and
WEBDRIVER_BASEURLpointing to Superset, callget_chart_previewwith{"identifier": <saved-chart-id>, "format": "png"}. Decodecontent.dataas base64 PNG. Check that it renders the saved chart as the calling user, and that a user without chart access cannot capture it. Try guest auth,form_data_key, extra filters, and viewport dimensions outside 64..4096; those requests must be rejected.Validation on the rebased head:
TZ=UTC .venv/bin/pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py tests/unit_tests/mcp_service/test_middleware.py tests/unit_tests/utils/webdriver_test.py -q, 349 passed.masterand this PR's PNG tests; the only manual conflict resolution retained both import groups.cb8ff2885a: three dateutil timezone-offset cases and a worker test missing the localtrinodependency. These are not repaired here.Earlier validation on
5d6c555:TZ=UTC. The affected time-range/query files were unchanged from the base.pre-commit runpassed, including mypy, Ruff and pylint. Direct pylint also passed for the changed middleware/configuration files.A live Superset browser capture with dataset permissions and RLS has not been rerun on the rebased head. Hosted CI must evaluate the new commit; the old prefixed Playwright fixture flake does not count as a pass.
Capture uses configured screenshot timeouts and a dedicated two-worker pool per MCP process. Cancelling a request does not interrupt an active capture or free its slot early; its browser closes when the capture finishes or fails. Start MCP workers after any process fork. Shutdown can wait for a running browser operation to finish or time out, as documented in the MCP README.
ADDITIONAL INFORMATION