Skip to content

feat(mcp): add list_reports and get_report_info tools - #40348

Merged
aminghadersohi merged 31 commits into
masterfrom
amin/mcp-list-reports
Jun 1, 2026
Merged

feat(mcp): add list_reports and get_report_info tools#40348
aminghadersohi merged 31 commits into
masterfrom
amin/mcp-list-reports

Conversation

@aminghadersohi

@aminghadersohi aminghadersohi commented May 22, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

Adds two new MCP tools for the Alerts & Reports domain:

  • list_reports: List alert and report schedules with filtering (by name, type, active, dashboard_id, chart_id), text search, sorting, select_columns, and 1-based pagination. Supports owned_by_me and created_by_me flags to filter by ownership/creator.
  • get_report_info: Fetch a single report/alert schedule by numeric ID, returning schedule configuration including type (Alert/Report), active status, cron expression, and associated dashboard or chart.

Schema discovery is wired in via get_schema(model_type="report").

Key design decisions:

  • RBAC handled automatically via ReportScheduleDAO.base_filter = ReportScheduleFilter
  • owners field stripped by USER_DIRECTORY_FIELDS privacy controls
  • No UUID support (ReportSchedule has no UUID column) — integer ID only
  • Column constants and get_report_columns() live in schema_discovery.py for DRY access across list/schema tools

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — backend-only MCP tools.

TESTING INSTRUCTIONS

  1. Start Superset with MCP service enabled
  2. Connect an MCP client and call list_reports — verify it returns reports accessible to the current user
  3. Call list_reports with {"owned_by_me": true} — verify only owned reports are returned
  4. Call get_report_info with a valid report ID — verify schedule config is returned
  5. Call get_report_info with an invalid ID — verify a not_found error is returned
  6. Call get_schema(model_type="report") — verify column/filter metadata is returned

Unit tests:

pytest tests/unit_tests/mcp_service/report/

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Live Test Results

Tested locally (superset-local MCP server, localhost:5008).

list_reports

Request:

{}

Response:

{
  "reports": [],
  "count": 0,
  "total_count": 0,
  "page": 1,
  "page_size": 10,
  "total_pages": 0,
  "has_previous": false,
  "has_next": false,
  "columns_requested": ["id", "name", "type", "active", "crontab"],
  "columns_loaded": ["id", "name", "type", "active", "crontab"],
  "columns_available": ["id", "name", "description", "type", "active", "crontab", "dashboard_id", "chart_id", "changed_on", "changed_on_humanized", "created_on", "created_on_humanized"],
  "sortable_columns": ["id", "name", "type", "active", "changed_on", "created_on"],
  "filters_applied": [],
  "pagination": {"page": 1, "page_size": 10, "total_count": 0, "total_pages": 0, "has_next": false, "has_previous": false},
  "timestamp": "2026-05-29T19:56:55.214692Z"
}

✅ Empty list (no reports in this workspace), no error, columns_available present.


get_report_info

Request:

{"identifier": 9999}

Response:

{
  "error": "ReportInfo with identifier '9999' not found",
  "error_type": "not_found",
  "timestamp": "2026-05-29T19:57:03.740908Z"
}

✅ Structured not_found error, no crash.


Live Test Results (Local — with seeded data)

Tested locally (superset-local MCP server, localhost:5008) with seeded report schedules.

list_reports

Request:

{}

Response:

{
  "reports": [
    {"id": 1, "name": "Weekly Dashboard Digest", "type": "Report", "active": true,  "crontab": "0 8 * * 1"},
    {"id": 2, "name": "High Value Alert",         "type": "Alert",  "active": false, "crontab": "*/15 * * * *"}
  ],
  "count": 2,
  "total_count": 2,
  "page": 1,
  "page_size": 10,
  "total_pages": 1,
  "has_previous": false,
  "has_next": false,
  "columns_requested": ["id", "name", "type", "active", "crontab"],
  "columns_loaded": ["id", "name", "type", "active", "crontab"],
  "columns_available": ["id", "name", "description", "type", "active", "crontab", "dashboard_id", "chart_id", "changed_on", "changed_on_humanized", "created_on", "created_on_humanized"],
  "sortable_columns": ["id", "name", "type", "active", "changed_on", "created_on"],
  "filters_applied": [],
  "pagination": {"page": 1, "page_size": 10, "total_count": 2, "total_pages": 1, "has_next": false, "has_previous": false},
  "timestamp": "2026-05-29T20:14:12.976217Z"
}

✅ 2 schedules returned (1 Report, 1 Alert), both type variants confirmed working, no error.


get_report_info

Request:

{"identifier": 1}

Response:

{
  "id": 1,
  "name": "Weekly Dashboard Digest",
  "description": "Sends weekly dashboard snapshot every Monday",
  "type": "Report",
  "active": true,
  "crontab": "0 8 * * 1",
  "dashboard_id": null,
  "chart_id": null,
  "changed_on": "2026-05-29T16:14:02.535472",
  "changed_on_humanized": "4 hours ago",
  "created_on": "2026-05-29T16:14:02.535472",
  "created_on_humanized": "4 hours ago"
}

✅ Full report detail returned including description, type, active status, and cron schedule, no error.

@aminghadersohi
aminghadersohi marked this pull request as ready for review May 22, 2026 04:07
@dosubot dosubot Bot added alert-reports Namespace | Anything related to the Alert & Reports feature api Related to the REST API labels May 22, 2026
@aminghadersohi
aminghadersohi requested a review from eschutho May 22, 2026 04:08

@bito-code-review bito-code-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Agent Run #07067a

Actionable Suggestions - 2
  • superset/mcp_service/report/schemas.py - 1
  • superset/mcp_service/report/tool/get_report_info.py - 1
Additional Suggestions - 2
  • superset/mcp_service/privacy.py - 1
    • Test gap for owners.id exclusion · Line 61-63
      The addition of `"owners.id"` to `SELF_REFERENCING_FILTER_COLUMNS` is correct and the comment on line 57 accurately documents its purpose. However, the existing schema-exclusion tests at test_get_schema.py lines 400, 430, 460 only assert that `"owner"` and `"created_by_fk_or_owner"` are absent from `filter_columns` — they do not verify `"owners.id"` is also excluded when the DAO returns it. The docstring on those tests states they exist to ensure synthetic server-generated columns are not advertised to LLM callers, which applies equally to `"owners.id"`.
  • superset/mcp_service/report/tool/list_reports.py - 1
    • Unused Serializer Parameter · Line 185-188
      The nested `_serialize_report` function ignores its `cols` parameter and unconditionally delegates to `serialize_report_object`. This is dead code in the diff's serializer path and mirrors `list_charts.py`'s identical pattern (which also ignores `cols`). Either use `serialize_report_object` directly or properly use the `cols` argument for field-level filtering.
      Code suggestion
      --- a/superset/mcp_service/report/tool/list_reports.py
      +++ b/superset/mcp_service/report/tool/list_reports.py
       @@ -182,15 +182,11 @@ async def list_reports(
            try:
                from superset.daos.report import ReportScheduleDAO
       
      -        def _serialize_report(
      -            obj: "ReportSchedule | None", cols: list[str] | None
      -        ) -> ReportInfo | None:
      -            return serialize_report_object(obj)
      -
                list_tool = ReportListCore(
                    dao_class=ReportScheduleDAO,
                    output_schema=ReportInfo,
      -            item_serializer=_serialize_report,
      +            item_serializer=serialize_report_object,
                    filter_type=ReportFilter,
                    default_columns=REPORT_DEFAULT_COLUMNS,
                    search_columns=REPORT_SEARCH_COLUMNS,
       @@ -199,6 +195,10 @@ async def list_reports(
                    all_columns=get_all_column_names(get_report_columns()),
                    sortable_columns=REPORT_SORTABLE_COLUMNS,
                    logger=logger,
      +            # Type: ignore needed because ModelListCore.item_serializer expects
      +            # Callable[[Any, list[str] | None], Any] but serialize_report_object
      +            # is Callable[[Any], ReportInfo | None]. The cols argument is handled
      +            # by model_dump context at the list level, not by individual serializers.
                )
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • superset/mcp_service/report/tool/list_reports.py - 1
Review Details
  • Files reviewed - 14 · Commit Range: 2e87c2c..2305683
    • superset/mcp_service/app.py
    • superset/mcp_service/common/schema_discovery.py
    • superset/mcp_service/constants.py
    • superset/mcp_service/mcp_core.py
    • superset/mcp_service/privacy.py
    • superset/mcp_service/report/__init__.py
    • superset/mcp_service/report/schemas.py
    • superset/mcp_service/report/tool/__init__.py
    • superset/mcp_service/report/tool/get_report_info.py
    • superset/mcp_service/report/tool/list_reports.py
    • superset/mcp_service/system/tool/get_schema.py
    • tests/unit_tests/mcp_service/report/__init__.py
    • tests/unit_tests/mcp_service/report/tool/__init__.py
    • tests/unit_tests/mcp_service/report/tool/test_report_tools.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Comment thread superset/mcp_service/report/schemas.py Outdated
Comment thread superset/mcp_service/report/tool/get_report_info.py Outdated
@aminghadersohi
aminghadersohi force-pushed the amin/mcp-list-reports branch from 2305683 to 394c20e Compare May 26, 2026 17:26
Comment thread superset/mcp_service/report/tool/list_reports.py Outdated
Comment thread superset/mcp_service/report/schemas.py
Comment thread superset/mcp_service/report/tool/list_reports.py
Comment thread superset/mcp_service/report/tool/list_reports.py Outdated
@github-actions github-actions Bot removed the api Related to the REST API label May 26, 2026
@codecov

codecov Bot commented May 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 51.23967% with 118 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.95%. Comparing base (041ecbc) to head (9726123).

Files with missing lines Patch % Lines
superset/mcp_service/common/schema_discovery.py 16.21% 31 Missing ⚠️
superset/mcp_service/report/tool/list_reports.py 37.14% 22 Missing ⚠️
superset/daos/report.py 17.39% 19 Missing ⚠️
superset/mcp_service/report/schemas.py 84.00% 15 Missing and 1 partial ⚠️
...uperset/mcp_service/report/tool/get_report_info.py 38.46% 16 Missing ⚠️
superset/mcp_service/mcp_core.py 11.11% 8 Missing ⚠️
superset/mcp_service/system/tool/get_schema.py 14.28% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #40348      +/-   ##
==========================================
- Coverage   63.96%   63.95%   -0.02%     
==========================================
  Files        2654     2658       +4     
  Lines      142774   143010     +236     
  Branches    32839    32866      +27     
==========================================
+ Hits        91332    91459     +127     
- Misses      49880    49988     +108     
- Partials     1562     1563       +1     
Flag Coverage Δ
hive 39.76% <51.23%> (+0.04%) ⬆️
mysql 58.40% <51.23%> (-0.03%) ⬇️
postgres 58.47% <51.23%> (-0.03%) ⬇️
presto 41.36% <51.23%> (+0.03%) ⬆️
python 59.96% <51.23%> (-0.04%) ⬇️
sqlite 58.13% <51.23%> (-0.03%) ⬇️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bito-code-review

bito-code-review Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #878667

Actionable Suggestions - 0
Additional Suggestions - 1
  • superset/mcp_service/report/tool/get_report_info.py - 1
    • Missing test coverage for exception path · Line 102-125
      The exception handler now correctly calls ctx.error with a single string argument and includes proper stack trace logging. No issues found. However, consider adding a unit test for the exception path to ensure the error handling works correctly (e.g., when ReportScheduleDAO.find_by_id raises an unexpected exception).
Review Details
  • Files reviewed - 2 · Commit Range: 2305683..394c20e
    • superset/mcp_service/report/schemas.py
    • superset/mcp_service/report/tool/get_report_info.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@bito-code-review bito-code-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Agent Run #3423a4

Actionable Suggestions - 2
  • superset/daos/report.py - 1
    • Missing test coverage for created_by_fk_or_owner · Line 48-91
  • tests/unit_tests/mcp_service/report/tool/test_report_tools.py - 1
Review Details
  • Files reviewed - 4 · Commit Range: 394c20e..28b8151
    • superset/daos/report.py
    • superset/mcp_service/report/schemas.py
    • superset/mcp_service/report/tool/list_reports.py
    • tests/unit_tests/mcp_service/report/tool/test_report_tools.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Comment thread superset/daos/report.py
Comment thread tests/unit_tests/mcp_service/report/tool/test_report_tools.py

@richardfogaca richardfogaca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. All line numbers verified against HEAD 63e75441.

Functional - worth fixing before merge

  • superset/mcp_service/report/tool/list_reports.py:164

    columns_available is populated from get_report_columns(), which is derived from the full ReportSchedule SQLAlchemy model. That advertises fields such as timezone, sql, email_subject, validator_config_json, etc., but ReportInfo only serializes the smaller set of fields defined in superset/mcp_service/report/schemas.py:83-105. If a client follows columns_available or get_schema(model_type="report") and asks for one of the advertised model-only columns, the DAO loads it but ReportInfo._filter_fields_by_context() drops it, so list_reports(select_columns=["timezone"]) returns report entries like {} instead of the requested value.

    WDYT - could we make the advertised report columns match the actual ReportInfo response fields, or add serializers for the additional fields we want to support?

@aminghadersohi

Copy link
Copy Markdown
Contributor Author

@richardfogaca Fixed in commit 1047c6a. Replaced get_all_column_names(get_report_columns()) with list(ReportInfo.model_fields.keys()) so columns_available only advertises fields that ReportInfo actually serializes: id, name, description, type, active, crontab, dashboard_id, chart_id, changed_on, changed_on_humanized, created_on, created_on_humanized. ORM-only fields like timezone, sql, email_subject, validator_config_json are no longer advertised.

@bito-code-review

bito-code-review Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #f4a1e1

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 28b8151..1047c6a
    • tests/unit_tests/mcp_service/report/tool/test_report_tools.py
    • superset/mcp_service/report/tool/list_reports.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@richardfogaca richardfogaca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. All line numbers verified against HEAD c5a3720.

Functional - worth fixing before merge

  • superset/mcp_service/system/tool/get_schema.py:156

    get_schema(model_type="report") builds its filter_columns from ReportScheduleDAO.get_filterable_columns_and_operators() via ModelGetSchemaCore, which discovers the full ReportSchedule SQLAlchemy model. That means the schema can advertise filters like timezone, sql, email_subject, validator_config_json, database_id, etc., but ReportFilter.col only accepts name, type, active, dashboard_id, chart_id, and created_by_fk.

    Since ReportFilter explicitly tells clients to use get_schema(model_type='report') for available filter columns, clients can follow schema discovery and then get rejected at request validation for advertised filters.

    WDYT - could we make report schema discovery expose the same filter whitelist that ReportFilter accepts, or otherwise derive both from one shared report filter constant?

@aminghadersohi

Copy link
Copy Markdown
Contributor Author

@richardfogaca Fixed in commit 2158b6a.

Added a REPORT_FILTER_COLUMNS constant (the exact whitelist from ReportFilter.col) to schema_discovery.py, and an include_filter_columns allowlist parameter to ModelGetSchemaCore. _get_report_schema_core now passes include_filter_columns=REPORT_FILTER_COLUMNS, so get_schema(model_type='report') only advertises the 6 filter columns that list_reports actually accepts (name, type, active, dashboard_id, chart_id, created_by_fk). ORM-only columns like timezone, sql, email_subject are no longer advertised as filterable.

@aminghadersohi

Copy link
Copy Markdown
Contributor Author

A few schema issues worth addressing before merge:

ReportInfo missing last_eval_dttm and last_state
These are the two most operationally important fields — when did the schedule last run, and what was the outcome. Without them, get_report_info can't answer basic monitoring questions. Both exist on the ReportSchedule model. Recommend adding to ReportInfo, ALL_REPORT_COLUMNS, and serialize_report_object.

ReportInfo.owners: List[Any] is untyped
The serializer sets owners directly from getattr(report, "owners", None) which returns raw FAB User ORM objects. With from_attributes=True, Pydantic will introspect these — potentially including fields that shouldn't be in the response. Recommend typing as List[OwnerInfo] (or reuse system.schemas.UserInfo / a slim OwnerInfo(id, first_name, last_name) shape) and serializing explicitly in serialize_report_object.

ReportFilter missing last_state and creation_method
Currently allows name, type, active, dashboard_id, chart_id, created_by_fk. The most useful missing filter is last_state — "show me reports that last failed" is a common operational query. creation_method is also useful (distinguish dashboard-created alerts from API-created ones).

_humanize_timestamp is duplicated from utils.response_utils
superset/mcp_service/report/schemas.py defines a local _humanize_timestamp even though utils.response_utils.humanize_timestamp already exists and is used by chart and tag schemas. The implementations differ slightly. Recommend using the shared helper to avoid divergence.

ReportList.filters_applied typed as List[ColumnOperator] instead of List[ReportFilter]
All other List* response schemas use the typed filter class for this field. Minor inconsistency.

@richardfogaca

Copy link
Copy Markdown
Contributor

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 that look worth checking before merge. All line numbers verified against HEAD 2158b6a.

Functional - worth investigating before merge

  • superset/mcp_service/report/tool/list_reports.py:145

    The report MCP surface does not appear to mirror the REST API's ALERT_REPORTS feature-flag guard. ReportScheduleRestApi returns 404 when ALERT_REPORTS is disabled, but list_reports, get_report_info, and get_schema(model_type="report") can still reach ReportScheduleDAO / advertise the schema if MCP is enabled and the user has the permission.

    WDYT - could we gate the report MCP entry points the same way as the REST API, or remove/disable these tools from MCP registration when ALERT_REPORTS is false?

  • superset/mcp_service/report/tool/get_report_info.py:122

    This returns str(exc) to the MCP caller for unexpected internal failures. Since this path wraps DAO/database/serialization exceptions, it can leak internal details while the server log already records the full exception with exc_info=True.

    Small suggestion: could we return a generic InternalError message to the client and keep the exception detail server-side only?

aminghadersohi added a commit that referenced this pull request May 28, 2026
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.
aminghadersohi added a commit that referenced this pull request May 28, 2026
Use QueryInfo.model_fields.keys() / SavedQueryInfo.model_fields.keys()
as the columns_available source in list_queries and list_saved_queries,
rather than manually maintained ALL_QUERY_COLUMNS / ALL_SAVED_QUERY_COLUMNS
constants. This ensures the advertised columns always exactly match what
the response schema can serialize, preventing future drift between the
constant and the schema definition.

Pattern mirrors the fix applied to list_reports in #40348 per reviewer
feedback from richardfogaca.

@bito-code-review bito-code-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Agent Run #b34f48

Actionable Suggestions - 1
  • superset/mcp_service/common/schema_discovery.py - 1
Additional Suggestions - 4
  • superset/mcp_service/report/tool/list_reports.py - 2
    • Inconsistent Tool Tags for Reports · Line 55-55
      The `list_reports` tool uses `tags=["core"]` while the companion `get_report_info` tool uses `tags=["discovery"]`. Both tools operate on the same `ReportSchedule` resource and should use consistent tagging for discoverability and API organization.
    • Unused Function Parameter · Line 109-112
      The `_serialize_report` function defines a `cols` parameter (line 110) that is never used in the function body (line 112 calls `serialize_report_object(obj)` without passing `cols`). This dead parameter increases cognitive load and could mislead future maintainers.
      Code suggestion
      --- a/superset/mcp_service/report/tool/list_reports.py
      +++ b/superset/mcp_service/report/tool/list_reports.py
       @@ -106,9 +106,9 @@ async def list_reports(
                    )
       
                def _serialize_report(
      -            obj: "ReportSchedule | None", cols: list[str] | None
      +            obj: "ReportSchedule | None"
                ) -> ReportInfo | None:
                    return serialize_report_object(obj)
  • superset/mcp_service/system/tool/get_schema.py - 1
    • Missing test coverage for ALERT_REPORTS gate · Line 308-314
      The `get_schema` tool now checks `ALERT_REPORTS` feature flag for the report model type, but no corresponding unit test exists to verify this behavior when the feature is disabled. Add test_get_schema_returns_error_when_alert_reports_disabled.
  • tests/unit_tests/mcp_service/system/tool/test_get_schema.py - 1
    • Incomplete test assertions · Line 499-499
      Per BITO.md rule [6262], add assertions verifying that `REPORT_FILTER_COLUMNS` (`"type"`, `"active"`) are advertised. Currently the test only checks exclusion but not inclusion, leaving the allowlist logic unverified.
      Code suggestion
      --- a/tests/unit_tests/mcp_service/system/tool/test_get_schema.py
      +++ b/tests/unit_tests/mcp_service/system/tool/test_get_schema.py
       @@ -496,6 +496,8 @@ class TestGetSchemaOmitSelfReferencingColumns:
                data = json.loads(result.content[0].text)
                info = data["schema_info"]
       
      +        assert "type" in info["filter_columns"]
      +        assert "active" in info["filter_columns"]
                assert "name" in info["filter_columns"]
                for field in ("owners.id", "created_by_fk_or_owner"):
                    assert field not in info["filter_columns"]
Review Details
  • Files reviewed - 17 · Commit Range: 9bd1c6d..784af4e
    • scripts/uv-pip-compile.sh
    • superset/daos/report.py
    • superset/mcp_service/app.py
    • superset/mcp_service/common/schema_discovery.py
    • superset/mcp_service/constants.py
    • superset/mcp_service/mcp_core.py
    • superset/mcp_service/privacy.py
    • superset/mcp_service/report/__init__.py
    • superset/mcp_service/report/schemas.py
    • superset/mcp_service/report/tool/__init__.py
    • superset/mcp_service/report/tool/get_report_info.py
    • superset/mcp_service/report/tool/list_reports.py
    • superset/mcp_service/system/tool/get_schema.py
    • tests/unit_tests/mcp_service/report/__init__.py
    • tests/unit_tests/mcp_service/report/tool/__init__.py
    • tests/unit_tests/mcp_service/report/tool/test_report_tools.py
    • tests/unit_tests/mcp_service/system/tool/test_get_schema.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Comment thread superset/mcp_service/common/schema_discovery.py Outdated
@bito-code-review

bito-code-review Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #e9dc56

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 784af4e..484d8c3
    • scripts/uv-pip-compile.sh
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds two new MCP tools — list_reports and get_report_info — for the Alerts & Reports domain, plus schema discovery support (get_schema(model_type="report")). RBAC is delegated to the existing ReportScheduleDAO.base_filter, owners are stripped via USER_DIRECTORY_FIELDS, and the DAO gains custom handling for owners.id and created_by_fk_or_owner to back the owned_by_me / created_by_me flags.

Changes:

  • New superset/mcp_service/report/ package (schemas + tools) and registration in app.py; runtime ALERT_REPORTS feature-flag guards in both tool bodies and get_schema.
  • Extends ModelListCore with a configurable owner_filter_column and a _call_dao_list hook, and ModelGetSchemaCore with an include_filter_columns allowlist; ReportScheduleDAO.apply_column_operators now resolves owners.id / created_by_fk_or_owner via report_schedule_user subqueries.
  • Adds "owners.id" to SELF_REFERENCING_FILTER_COLUMNS, registers "report" in ModelType / _SCHEMA_CORE_FACTORIES / _MODEL_TYPE_CLASS_PERMISSION, and ships extensive unit tests covering filters, sanitization, privacy, feature-flag gating, and self-referencing filter omission.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
superset/mcp_service/report/schemas.py New Pydantic schemas (ReportFilter, ReportInfo, ReportList, ListReportsRequest, GetReportInfoRequest, ReportError) and serialize_report_object with LLM-context sanitization.
superset/mcp_service/report/tool/list_reports.py New list_reports tool wired through ModelListCore with owner_filter_column="owners.id".
superset/mcp_service/report/tool/get_report_info.py New get_report_info tool using ModelGetInfoCore (integer ID only).
superset/mcp_service/report/{init.py,tool/init.py} Package init files exposing the new tools.
superset/mcp_service/app.py Registers the two tools and adds Alerts & Reports section to default instructions.
superset/mcp_service/system/tool/get_schema.py Adds _get_report_schema_core, registers "report" model type, and gates it on ALERT_REPORTS.
superset/mcp_service/common/schema_discovery.py Adds REPORT_* column constants, get_report_columns, get_report_info_columns, _annotation_to_type_str.
superset/mcp_service/mcp_core.py Adds owner_filter_column, _call_dao_list, and include_filter_columns extension points.
superset/mcp_service/privacy.py Adds "owners.id" to SELF_REFERENCING_FILTER_COLUMNS.
superset/mcp_service/constants.py Adds "report" to ModelType literal.
superset/daos/report.py Adds apply_column_operators override for owners.id and created_by_fk_or_owner.
tests/unit_tests/mcp_service/report/... New comprehensive unit tests for the report tools.
tests/unit_tests/mcp_service/system/tool/test_get_schema.py Adds report self-referencing filter omission test.

Comment on lines +751 to +760
def get_report_columns() -> list[ColumnMetadata]:
"""Get column metadata for ReportSchedule model dynamically."""
from superset.reports.models import ReportSchedule

return get_columns_from_model(
ReportSchedule,
REPORT_DEFAULT_COLUMNS,
REPORT_EXTRA_COLUMNS,
exclude_columns=set(USER_DIRECTORY_FIELDS),
)

@richardfogaca richardfogaca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posting on Richard's behalf — this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.

One functional gap below. Line numbers verified against HEAD 484d8c3.

Functional — worth checking before merge

  • superset/mcp_service/report/tool/list_reports.py:138

    created_by_me=True looks like it can turn a successful DAO result into an InternalError. ModelListCore injects a created_by_fk filter for that flag, but filters_applied only strips SELF_REFERENCING_FILTER_COLUMNS; created_by_fk is intentionally not in that set. The response then validates filters_applied as List[ReportFilter], and ReportFilter.col does not allow created_by_fk, so the report list schema should reject the server-generated filter.

    WDYT — could we hide the injected created_by_fk from filters_applied for this self-filter path, or otherwise keep the public ReportFilter allowlist from rejecting server-generated filters?

aminghadersohi and others added 3 commits June 1, 2026 16:46
- Replace blind Exception catches with re-raise in list_reports and
  get_report_info (BLE001); log via ctx.error then raise so the
  middleware handles conversion to a ToolError
- Remove unused get_report_columns() from schema_discovery (dead code
  flagged by copilot reviewer; get_report_info_columns() is the one
  wired into get_schema)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Restore return-ReportError pattern in list_reports and get_report_info
  (re-raise broke two tests that expect structured {"error_type": ...}
  JSON matching the established saved_query / dashboard pattern)
- Remove redundant logger.warning from get_report_info except block;
  ctx.error is the single log point, matching list_reports
- Fix ReportInfo.owners field description from "filtered by privacy
  controls" (misleading — implies a populated list gets filtered) to
  "always empty; excluded by privacy policy" (accurate)
- Rename test_list_reports_request_rejects_invalid_order_column to
  test_list_reports_request_schema_accepts_any_order_column; the test
  body already documented that the schema accepts anything and ModelListCore
  rejects, so the old name was the opposite of what it tested
- Strengthen feature-flag tests: assert DAO is never called when
  ALERT_REPORTS is disabled (verifies the early-return path)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… in report tools

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

@richardfogaca richardfogaca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aminghadersohi
aminghadersohi merged commit 5312d0a into master Jun 1, 2026
59 checks passed
@aminghadersohi
aminghadersohi deleted the amin/mcp-list-reports branch June 1, 2026 18:23
@bito-code-review

Copy link
Copy Markdown
Contributor

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

alert-reports Namespace | Anything related to the Alert & Reports feature size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants