feat(mcp): add manage_native_filters tool#40960
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40960 +/- ##
==========================================
- Coverage 64.54% 64.49% -0.05%
==========================================
Files 2665 2666 +1
Lines 146641 146875 +234
Branches 33880 33912 +32
==========================================
+ Hits 94652 94733 +81
- Misses 50271 50425 +154
+ Partials 1718 1717 -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:
|
Code Review Agent Run #13a2f1Actionable 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 |
5bd5c9a to
274f0f3
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #739ec1Actionable 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 |
c76ff7d to
45e1fc2
Compare
|
Addressed in the latest push (now |
Code Review Agent Run #51f943Actionable Suggestions - 0Additional Suggestions - 3
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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
0dbe17c to
dc721df
Compare
dc721df to
582ebf8
Compare
|
Addressing bito review findings from runs #13a2f1 and #51f943 (commit 582ebf8): Dead type cast (run #51f943): Fixed — removed the runtime-noop Missing reorder duplicate test (run #51f943): Fixed — added Incomplete test stub (run #51f943, Dead exception handler (run #13a2f1, |
|
Fixed pre-commit CI failures in 49c78fb:
|
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
UpdateDashboardNativeFiltersCommand doesn't process tags, so TagForbiddenError can't be raised on this path. This except looks copied from a general dashboard-update handler — safe to drop.
|
Also, the current_config snapshot read here (T1) and the re-read inside UpdateDashboardNativeFiltersCommand (T2) aren't covered by a shared lock or a version check on json_metadata. If another writer commits in between, a concurrently added filter gets silently dropped when this request also does a reorder (its id is in neither reordered nor modified, so the DAO's rebuild discards it). This mirrors existing REST behavior, but since this tool may be driven by an agent issuing frequent reorders, worth at least a docstring note — ideally re-read inside the command's transaction and reject if changed. |
|
Thank you @rusackas I will address your concerns and test end to end |
Code Review Agent Run #1c0c2bActionable 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 |
Address PR review feedback on manage_native_filters: - Wrap the user-controlled filter name and target column names as untrusted content before returning them in the tool response, mirroring the get_dashboard_info read path (prevents prompt-injection via filter metadata). - Reject duplicate filter IDs in the update list so later updates for the same filter are no longer silently dropped by the DAO's first-match resolution. - Add docstrings to helper functions and type annotations to test helpers.
Add test_update_and_remove_same_filter_rejected and refresh the module docstring coverage list (update-validation + sanitization cases). The update+remove conflict guard was previously uncovered.
CodeAnt review: json_metadata can parse to a non-dict (e.g. a legacy '[]' payload), which made metadata.get(...) raise AttributeError and escape the structured error path. Extract _current_native_filter_config to parse defensively (invalid JSON or non-dict -> empty list). Add a regression test plus docstrings/type annotations flagged in review.
CodeAnt review:
- The request validator rejected an explicit empty reorder list because it
used a falsy check; the payload builder already treats reorder is not None
as an operation. Align the validator (check reorder is None) so
{"reorder": []} is a valid no-op operation.
- native_filter_configuration could be a non-list or contain non-dict items,
crashing payload building on conf["id"]/conf.get(...). Filter to dict
entries in _current_native_filter_config.
Add regression tests and two helper docstrings flagged in review.
…ite path - _filter_summary now applies escape_llm_context_delimiters to the operational id and filter_type fields so embedded delimiter tokens cannot disrupt outer LLM-context wrappers, while keeping the values usable verbatim as identifiers in subsequent tool calls. Added test_filter_summary_escapes_delimiter_tokens_in_operational_fields. - DashboardDAO.update_native_filters_config now guards against non-dict json_metadata in the write path (e.g. legacy '[]' payloads), matching the robustness already in _current_native_filter_config for the read path.
Extract tampered_id into a local variable so the inline string literal in the update request stays within the 88-char line limit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Drop the runtime-noop `cast(list[dict[str,Any]], ...)` wrapper around
`sanitize_for_llm_context` in `_filter_summary` and remove the now-
unused `cast` import.
- Wrap `json.loads` in `DashboardDAO.update_native_filters_config` with a
`(json.JSONDecodeError, TypeError)` guard so corrupt `json_metadata`
degrades to `{}` instead of crashing the write path with an unhandled
exception (mirrors the existing guard in `_current_native_filter_config`).
- Add `test_reorder_duplicate_filter_ids_rejected` to cover the duplicate-
ID check in `_build_native_filters_payload`, which was previously
exercised only by source inspection.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ply ruff formatting `update_native_filters_config` hit complexity 11 (limit 10) after the json.loads guard was added in the previous commit. Pre-build `deleted_ids` set and `modified_map` dict to replace the two generator-if expressions and the conditional append, dropping McCabe complexity from 11 to 8. No behaviour change — tests confirm identical output. Also apply ruff auto-format to test file (unnecessary parentheses removal).
49c78fb to
4b4db14
Compare
|
Rebased onto master in 4b4db14 (was 38 commits behind; now clean on top of 805c12e). Conflict in superset/mcp_service/app.py: master had added duplicate_dashboard to the write-tool list; our branch adds manage_native_filters. Merged both. All 22 unit tests pass. All 38 review threads have replies; the one remaining unresolved thread (type-annotation nit, already declined + CodeAnt custom instruction saved) resolved via API. |
A reorder validated against the pre-transaction snapshot can silently drop a filter added by a concurrent writer between the snapshot read (T1) and the DAO write (T2). This mirrors existing REST behaviour. Document the limitation and the recommended retry pattern in the tool docstring so callers (including LLM agents) are aware.
|
@rusackas — you're right, and thank you for the detailed write-up. The race is real: a This mirrors the existing REST write behavior (the REST update endpoint has the same gap), so it's not a regression, but it's worth calling out explicitly since an LLM-driven agent could issue frequent reorders. For v1, I've added a concurrency note to the tool docstring in f255d21 documenting the limitation and recommending the read-then-compare retry pattern. A proper server-side guard (e.g. compare a hash of |
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
Adds a new MCP tool,
manage_native_filters, to the Superset MCP service. Native filters are what make dashboards interactive; the MCP server could previously read them (viaget_dashboard_info) but not write them.The tool exposes four operations on a dashboard's native filters, translated into the
deleted/modified/reorderedpayload consumed by the existingUpdateDashboardNativeFiltersCommand(the same command behind thePUT /api/v1/dashboard/{pk}/filtersendpoint):filter_select(dropdown: dataset + column target,multiSelect,defaultToFirstItem,enableEmptyFilter,sortAscending,searchAllOptions) andfilter_time(time range with optional default). Other filter types (numerical range, time column, time grain) are documented as not yet supported.native_filter_configurationand merges changes into FULL filter configs. Type-incompatible fields (e.g.default_time_rangeon afilter_select) are rejected.Design notes:
NATIVE_FILTER-<random>, matching the frontend'sgenerateFilterIdconvention, and returned in the response (added_filter_ids).Filtertype shape:{id, type: "NATIVE_FILTER", filterType, name, description, scope, targets, controlValues, defaultDataMask, cascadeParentIds}.scope_chart_idsper filter is translated into the frontend's exclusion-list scope ({rootPath: ["ROOT_ID"], excluded: [...]}); IDs not on the dashboard are rejected. When omitted, the filter applies to all charts.DatasetDAOwith an error that lists available columns so LLM callers can self-correct.tags=["mutate"],class_permission_name="Dashboard",method_permission_name="write", andToolAnnotations(readOnlyHint=False, destructiveHint=True)sinceremovedeletes filters.DashboardForbiddenError/TagForbiddenErrorsurface as structuredpermission_deniedresponses.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — MCP service only, no UI changes.
TESTING INSTRUCTIONS
pytest tests/unit_tests/mcp_service/dashboard/tool/test_manage_native_filters.pymanage_native_filterswith e.g.{"dashboard_id": 1, "add": [{"filter_type": "filter_select", "name": "Region", "dataset_id": 5, "column": "region"}]}and verify the filter appears on the dashboard's filter bar; exerciseupdate,remove, andreordersimilarly.ADDITIONAL INFORMATION