feat(mcp): add list and get tools for action log and tasks - #40344
Conversation
Code Review Agent Run #b8d6d6Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
0f9b0ee to
d5b95da
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The PR comments file is empty (only contains the header row). I cannot analyze any review comments or suggestions. Please ensure the file contains actual comment data. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40344 +/- ##
==========================================
- Coverage 64.02% 64.00% -0.02%
==========================================
Files 2612 2620 +8
Lines 140615 140974 +359
Branches 32501 32527 +26
==========================================
+ Hits 90032 90237 +205
- Misses 49034 49186 +152
- Partials 1549 1551 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf -- this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left a few notes below -- the main things I would check before merge are the feature/config guards around these new MCP surfaces and the dttm filter value type. All line numbers verified against HEAD 28461c7.
Functional -- worth checking before merge
-
superset/mcp_service/action_log/tool/list_action_logs.py:87The action-log MCP tools query
LogDAOdirectly whenever the caller has theLogread permission, but the existing REST/model views also honorFAB_ADD_SECURITY_VIEWSandSUPERSET_LOG_VIEWthroughLogRestApi.is_enabled(). If an operator disables the action-log surface with those config flags, the REST API returns 404 while these MCP tools still register and can read logs.WDYT -- could we mirror the existing
LogRestApi.is_enabled()guard for bothlist_action_logsandget_action_log_info, so MCP follows the same disabled-state behavior as the rest of Superset? -
superset/mcp_service/app.py:677The task tools are imported and registered unconditionally, but the existing task REST API and
TaskManagerinitialization are both gated byGLOBAL_TASK_FRAMEWORK. Since that flag defaults off, this can advertise and execute task metadata tools in an instance where/api/v1/taskand task-manager behavior were intentionally disabled.WDYT -- should the MCP task tools be hidden or return a disabled-feature error unless
feature_flag_manager.is_feature_enabled("GLOBAL_TASK_FRAMEWORK")is true? -
superset/mcp_service/action_log/tool/list_action_logs.py:93The injected default
dttmfilter is an ISO string, and the schema also limits filter values to string/number/bool/list.BaseDAO.apply_column_operators()then comparesLog.dttm >= valuedirectly; with this string value SQLAlchemy infers the bind asVARCHAR, so on Postgres this looks likely to compile as a timestamp-to-varchar comparison rather than a timestamp bind.Could we keep the DAO filter value as a
datetimeand let response serialization turnfilters_appliedinto an ISO string, or otherwise parsedttmfilter values before they reachColumnOperatorEnum.gte?
Praise
-
superset/mcp_service/task/tool/list_tasks.py:90Nice reuse of
TaskDAO.base_filterfor task visibility. Keeping subscription scoping in the DAO path avoids duplicating a second, possibly divergent access filter in the MCP layer.
|
Thanks for the review @richardfogaca! All three issues addressed in 946bb37: Issue 1 — Config guard for action-log tools Issue 2 — GLOBAL_TASK_FRAMEWORK gate for task tools Issue 3 — dttm filter as datetime object Tests added for all three guards in |
Code Review Agent Run #ea15ecActionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
|
Integration test results
In a multi-tenant deployment where workspace admins are mapped to a custom FAB role, |
|
Thanks for the detailed staging report. After reviewing the tool files and comparing against
Action log access is intentionally admin-only in OSS — The fix is in the deployment's role config: add |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf — this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left one functional note below. The line number is verified against HEAD e3ba2d170e9c068d76b9e27a37e5a65a5127a28d.
Functional — worth checking before merge
-
superset/mcp_service/app.py:727This guard reads only
flask_app.config["FEATURE_FLAGS"], but Superset registersTaskRestApithroughfeature_flag_manager.is_feature_enabled("GLOBAL_TASK_FRAMEWORK"). That manager also respects supported enablement paths likeDEFAULT_FEATURE_FLAGS,SUPERSET_FEATURE_*,GET_FEATURE_FLAGS_FUNC, andIS_FEATURE_ENABLED_FUNC.In those configurations, GTF can be enabled for the normal task API while MCP still removes
list_tasksandget_task_info.WDYT — could we use
feature_flag_manager.is_feature_enabled("GLOBAL_TASK_FRAMEWORK")here too, so this mirrorsinitialization/__init__.py?
|
Good catch @richardfogaca — fixed in 8a84b91.
Test update: swapped the raw |
Code Review Agent Run #466863Actionable 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 |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf -- this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left two notes below. Line numbers verified against HEAD 8a84b91f89e021d8d0248542c9075eb152f25a25.
Functional -- worth checking before merge
-
superset/mcp_service/action_log/schemas.py:69Explicit
dttmfilters from MCP clients can still reach the DAO as strings. JSON callers will send values like"2020-01-01T00:00:00", and sinceActionLogFilter.valueincludesstrbeforedatetime, Pydantic keeps that value as a string. The test attest_action_log_tools.py:154currently asserts that exact behavior.That means caller-provided
dttmfilters still hitBaseDAO.apply_column_operators()as timestamp-vs-string comparisons, even though the injected default cutoff was fixed to use adatetime.WDYT -- could we normalize
ActionLogFilter.valueto adatetimewhenevercol == "dttm"before passing filters intoModelListCore? -
superset/mcp_service/app.py:125The default MCP instructions' “Available tools” section does not mention the new action-log or task tools. Tool discovery may still expose them, but these instructions are the curated guide agents see for how to use the service, so the new tools are effectively undocumented in the primary prompt surface.
WDYT -- should we add a short Action Logs / Tasks section here, and make sure config-guarded tools are omitted from the generated instructions when removed?
|
Thanks for the continued review, @richardfogaca! Round 3 fixes — eeb4f0f: Issue A — dttm string normalization in Issue B — action-log/task tools in generated instructions:
|
|
A few schema issues worth addressing before merge:
Naming inconsistency:
|
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf - this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left one data-boundary note below. Line number verified against HEAD 1fd7ecf4a67b725e9f3ca06cc7cc7e9a25dae8aa.
Functional - worth checking before merge
-
superset/mcp_service/action_log/schemas.py:266The new action-log serializer returns the stored log
jsonpayload directly. That payload is workspace/user-controlled data, and MCP results are later placed in an LLM context; other MCP serializers wrap user-controlled strings with the untrusted-content sanitizer so result content cannot masquerade as instructions. Here, a log payload containing prompt-like text would be returned raw by bothlist_action_logsandget_action_log_info.WDYT - could we run this field through the existing LLM-context sanitization helper before returning it? If we expect callers to inspect raw JSON, preserving the JSON shape and sanitizing/wrapping only string leaves would keep it usable while maintaining the MCP data boundary.
Three issues identified in sibling PR reviews (#40344, #40348) that apply equally to the tag tools: 1. DEFAULT_INSTRUCTIONS omitted list_tags/get_tag_info — adds a Tag Management section so agents see the tools in the curated guide. 2. serialize_tag_object returned name/description without wrapping in UNTRUSTED-CONTENT delimiters — user-controlled tag text could masquerade as instructions in LLM context. Adds _sanitize_tag_info_for_llm_context matching the pattern used by chart, dashboard, and dataset serializers. 3. ALL_TAG_COLUMNS omitted changed_on_humanized / created_on_humanized — TagInfo exposes these derived fields but columns_available didn't advertise them, so clients following schema discovery couldn't request them. ModelListCore already maps *_humanized → *_on for DB loading. Updates tests: value assertions now use `in` to tolerate the UNTRUSTED-CONTENT wrapper; adds test_get_tag_info_sanitizes_user_controlled_fields to verify the boundary is enforced.
Code Review Agent Run #e201e0Actionable 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 #b3e52aActionable 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 |
89b5644 to
18a7def
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The injected 7-day default filter used a datetime object as the value, but ActionLogFilter.value only allows str|int|float|bool|list. Pydantic rejects the datetime when building the filters_applied list in ActionLogList, causing a ValidationError on every call that triggered the default filter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add model_serializer to ActionLogInfo and TaskInfo that drops non-requested fields from output when select_columns context is set, matching the DatabaseInfo pattern - Switch list_action_logs and list_tasks to return model_dump with serialization context so only requested columns appear in responses - Add search field + search-XOR-filters validator to ListActionLogsRequest and ListTasksRequest - Pass search=request.search through to ModelListCore.run_tool() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rage - Add task_key and task_name fields to TaskInfo schema and ALL_TASK_COLUMNS; these are real Task model columns present in the REST API search_columns - Expand search_columns in list_tasks to include task_key and task_name - Strengthen test_list_action_logs_default_7day_filter_applied to also assert the injected filter appears in filters_applied with an ISO string value Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Pydantic v2 rejects a ColumnOperator instance when validating list[ActionLogFilter] — it requires an exact instance or dict, not a parent-class instance. The injected 7-day default dttm filter was created as a plain ColumnOperator, causing every test_list_action_logs_* call to fail with '1 validation error for ActionLogList'. Fix: construct the default filter as ActionLogFilter (which is a subclass of ColumnOperator), so it passes pydantic validation for ActionLogList.filters_applied: list[ActionLogFilter] and is still accepted everywhere ColumnOperator is expected.
…ests - Normalize changed_on/created_on naive datetimes in serialize_task_object (mirrors serialize_action_log_object pattern for dttm) - Add filter-forwarding assertion to test_list_tasks_with_status_filter - Add id_column="uuid" assertion to test_get_task_info_by_uuid Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…permissions The ADMIN-ONLY label was misleading — access is gated by the Log permission in Superset's RBAC, not a hard admin check. Updated both docstrings to describe the actual permission model.
…docs - Add model_validator to ActionLogFilter to parse ISO string dttm values to timezone-aware datetime objects, preventing VARCHAR bind mismatch on Postgres TIMESTAMP columns (Pydantic's left-to-right union keeps strings as str when str precedes datetime in the union) - Feed config-guard removed tools (action-log, task) into disabled_tools before calling get_default_instructions so removed tools are never advertised in LLM instructions - Add Action Logs and Task Management sections to get_default_instructions output; existing per-line filtering strips them when tools are disabled - Update test assertions and add new coverage for both behaviors
The stored log `json` field is user-controlled data. Parse it and run each string leaf through `sanitize_for_llm_context` so the payload cannot masquerade as instructions when placed in an LLM context. Preserves the JSON shape (dict/list structure) so callers can still inspect individual fields; only string leaves are wrapped in UNTRUSTED-CONTENT delimiters. Falls back to sanitizing the raw string when the payload is not valid JSON. Addresses review feedback from richardfogaca.
- Pass excluded_field_names=frozenset() to sanitize_for_llm_context in _sanitize_log_json so that all string leaves in the user-controlled json blob are wrapped in UNTRUSTED-CONTENT delimiters. Previously, fields like url, schema, and uuid were only escaped (not wrapped) because they appear in the default exclusion list intended for structured, trusted output fields. - Extend normalize_dttm_value to also normalize string elements inside list values so dttm IN (...) filters are also converted to datetime objects, preventing TIMESTAMP/VARCHAR bind mismatch on Postgres for that operator. - Update ActionLogFilter.value type annotation to include datetime in the list element union, matching the post-validation runtime type. - Add tests: url/schema wrapping and dttm list filter normalization.
…ion-log
Log JSON payloads are fully user-controlled, including field names. A
crafted key like {"ignore previous instructions": "..."} would previously
pass through _sanitize_log_json with only delimiter-token escaping on the
key, not the full UNTRUSTED-CONTENT wrapping applied to string values.
Add a wrap_dict_keys parameter to sanitize_for_llm_context so callers
that know the entire blob is untrusted can opt into key wrapping.
_sanitize_log_json now passes wrap_dict_keys=True alongside the existing
excluded_field_names=frozenset() to ensure both keys and values of the
log JSON are wrapped.
Add regression tests:
- test_get_action_log_info_malicious_json_key_wrapped
- test_list_action_logs_malicious_json_key_wrapped
…_schema scope - Remove wrap_dict_keys param from sanitize_for_llm_context: wrapping keys breaks dict shape (callers cannot navigate by original key name) while providing no meaningful safety gain — values are already fully wrapped via excluded_field_names=frozenset(). Keys are still delimiter-escaped to prevent forged UNTRUSTED-CONTENT tokens in key names. - Align TASK_SORTABLE_COLUMNS with TaskRestApi.order_columns: remove id (not sortable in REST), add task_type/scope/started_at/ended_at. Change default sort from changed_on to created_on to match REST base_order. - Clarify in instructions that get_schema only covers chart/dataset/dashboard/ database; action_log and task tools list their columns in their docstrings. - Update malicious-key tests to match new behavior: verify value is wrapped and key tokens are escaped rather than checking that keys are wrapped. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…inline imports Closes Richard's open review thread on dict-key injection: _sanitize_log_json now serializes the entire JSON blob as a canonical JSON string and wraps it in a single UNTRUSTED-CONTENT block, so keys and values alike are inside the trust boundary — no key can inject instructions into the LLM context. Also fixes Scan-1 inline-import BLOCKERs across all changed files: - DAO imports (LogDAO, TaskDAO) moved to module top level in tool files - Redundant `from datetime import timezone` removed from schema methods (timezone already imported at module top in both schemas.py files) - `from unittest.mock import Mock` moved to module top level in test fixtures - Redundant re-imports of `mcp` / `init_fastmcp_server` removed from test function bodies in test_mcp_tool_registration.py (already at module top) - `from superset.utils import json as json_utils` moved to module top in action_log/schemas.py (was inline inside _sanitize_log_json with noqa comment) Updated tests: all json-payload assertions now check isinstance(payload, str) and verify UNTRUSTED-CONTENT wrapping on the string blob; malicious-key tests verify that injecting keys are enclosed in the wrapper (not accessible as dict keys) and that embedded UNTRUSTED-CONTENT tokens are escaped inside it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…STED-CONTENT delimiter Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
18a7def to
3ce791b
Compare
…itization Sync patterns from sibling PRs apache#40344 and apache#40348: - Inline-import BLOCKERs: move RLSDAO and DynamicPluginDAO imports from inside try blocks to module top level in all four tool files (list/get for rls and plugin) - Remove redundant inline `from datetime import timezone` from RlsFilterError.create and PluginError.create (timezone already imported at module top after adding it to the top-level import) - Add @field_validator("error") to RlsFilterError and PluginError to wrap error text in UNTRUSTED-CONTENT delimiters before LLM exposure, matching the DashboardError/DatasetError/ReportError pattern Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
Adds 4 new MCP tools across two new domains:
Action Log domain (
superset/mcp_service/action_log/):list_action_logs: Lists audit log entries with filtering, pagination, and a default 7-day dttm window to prevent oversized result sets on large instances. Admin-only.get_action_log_info: Retrieves a single log entry by integer ID. Admin-only.Task domain (
superset/mcp_service/task/):list_tasks: Lists async tasks. Non-admin users are automatically scoped to tasks they are subscribed to viaTaskDAO.base_filter = TaskFilter. Admins see all tasks. Supportssearch(matched against task_type, task_key, task_name, status, scope) and column-levelfilters(mutually exclusive).get_task_info: Retrieves a single task by integer ID or UUID string.Both domains follow the established
ModelListCore/ModelGetInfoCorepatterns from the database domain. Response field filtering viaselect_columnsuses@model_serializercontext propagation, matching theDatabaseInfopattern.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only MCP tools.
TESTING INSTRUCTIONS
Run the new unit tests:
Key behaviors verified by tests:
list_action_logsinjects adttm >= now - 7 daysISO-string filter when nodttmfilter is providedlist_action_logsskips the default filter when the caller provides adttmfilterlist_action_logsdefaults toorder_column=dttm, order_direction=desclist_action_logsfilters_appliedechoes the injected filter as an ISO string (not datetime)list_tasksdelegates toTaskDAO.list()soTaskFilterscoping applies automaticallyget_task_inforesolves by both integer ID and UUID stringerror_type: not_foundresponsesselect_columnsfiltering drops non-requested fields from the response (not null)ADDITIONAL INFORMATION
Live Test Results
Tested on staging Preset workspace (build ID:
36a56752, MCP server:claude.ai preset stg).list_action_logsRequest:
{}Response:
{ "action_logs": [], "count": 0, "total_count": 0, "page": 1, "page_size": 10, "total_pages": 0, "has_previous": false, "has_next": false, "columns_requested": ["id", "action", "user_id", "dttm"], "columns_loaded": ["id", "action", "user_id", "dttm"], "columns_available": ["id", "action", "user_id", "dttm", "dashboard_id", "slice_id", "json"], "sortable_columns": ["id", "dttm"], "filters_applied": [{"col": "dttm", "opr": "gte", "value": "2026-05-22T17:13:21.220832Z"}], "pagination": {"page": 1, "page_size": 10, "total_count": 0, "total_pages": 0, "has_next": false, "has_previous": false}, "timestamp": "2026-05-29T17:13:21.225768Z" }✅ Empty list (expected — action logs not populated in Preset environments), default 7-day
dttmfilter injected automatically,columns_availablepresent, no permission error.get_action_log_infoRequest:
{"identifier": 1}Response:
{ "error": "ActionLogInfo with identifier '1' not found", "error_type": "not_found", "timestamp": "2026-05-29T17:13:22.416252Z" }✅ Structured
not_founderror, no crash, no permission error.list_tasksRequest:
{}Response:
{ "tasks": [], "count": 0, "total_count": 0, "page": 1, "page_size": 10, "total_pages": 0, "has_previous": false, "has_next": false, "columns_requested": ["id", "uuid", "task_type", "status", "changed_on"], "columns_loaded": ["id", "uuid", "task_type", "status", "changed_on"], "columns_available": ["id", "uuid", "task_type", "task_key", "task_name", "status", "scope", "changed_on", "created_on"], "sortable_columns": ["id", "changed_on", "created_on", "status"], "filters_applied": [], "pagination": {"page": 1, "page_size": 10, "total_count": 0, "total_pages": 0, "has_next": false, "has_previous": false}, "timestamp": "2026-05-29T17:13:23.542322Z" }✅ Empty list (no active tasks in this workspace), no error,
columns_availablepresent.get_task_infoRequest:
{"identifier": 9999}Response:
{ "error": "TaskInfo with identifier '9999' not found", "error_type": "not_found", "timestamp": "2026-05-29T17:13:24.900648Z" }✅ Structured
not_founderror, no crash.