fix(mcp): replace uuid with url and changed_on_humanized in default list columns#38566
Conversation
Code Review Agent Run #2f23b6Actionable 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #38566 +/- ##
==========================================
- Coverage 65.01% 64.40% -0.62%
==========================================
Files 1817 2529 +712
Lines 72318 128947 +56629
Branches 23032 29718 +6686
==========================================
+ Hits 47016 83042 +36026
- Misses 25302 44460 +19158
- Partials 0 1445 +1445
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:
|
…ist columns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5308c5e to
d49aacb
Compare
Sequence DiagramThis PR changes the default selected columns for list tools so responses return user-friendly fields like url and changed_on_humanized instead of uuid. It also updates get schema metadata so clients see the same new defaults in default_select. sequenceDiagram
participant User
participant ListTool
participant SchemaConfig
participant DataStore
participant SchemaTool
User->>ListTool: Request list dashboards with no select columns
ListTool->>SchemaConfig: Resolve default columns for dashboard
SchemaConfig-->>ListTool: id dashboard_title slug url changed_on_humanized
ListTool->>DataStore: Fetch dashboard rows with resolved columns
DataStore-->>ListTool: Dashboard records
ListTool-->>User: Return list without uuid by default
User->>SchemaTool: Request schema for dashboard
SchemaTool->>SchemaConfig: Build schema info
SchemaConfig-->>SchemaTool: default_select uses url and changed_on_humanized
SchemaTool-->>User: Return schema metadata with updated defaults
Generated by CodeAnt AI |
| "url", | ||
| "changed_on_humanized", |
There was a problem hiding this comment.
Suggestion: changed_on_humanized is a computed property, not a queryable SQLAlchemy column. In this flow, ModelListCore passes default columns to BaseDAO.list, which does column-based queries and returns row objects; computed fields are not loaded there, so changed_on_humanized will be None for every row. Use a queryable timestamp column in defaults (or compute humanized text during serialization) to avoid returning empty values. [logic error]
Severity Level: Major ⚠️
- ⚠️ list_charts default returns null changed_on_humanized values.
- ⚠️ MCP chart summaries lose human-readable modification context.
- ❌ PR's user-friendly default-column goal is partially broken.| "url", | |
| "changed_on_humanized", | |
| "url", | |
| "changed_on", |
Steps of Reproduction ✅
1. Invoke MCP tool `list_charts` without `select_columns` (default workflow documented in
`superset/mcp_service/app.py:62` and request default behavior in
`tests/unit_tests/mcp_service/chart/tool/test_list_charts.py:70-78`).
2. Execution enters `list_charts()` at
`superset/mcp_service/chart/tool/list_charts.py:67`, where `ModelListCore` is configured
with `default_columns=DEFAULT_CHART_COLUMNS` (`list_charts.py:109-115`), including
`"changed_on_humanized"` (`list_charts.py:46-52`).
3. `ModelListCore.run_tool()` (`superset/mcp_service/mcp_core.py:135`) uses defaults when
`select_columns` is empty (`mcp_core.py:41-43`) and calls `ChartDAO.list(...,
columns=columns_to_load)` (`mcp_core.py:46-55`).
4. In `BaseDAO.list()` (`superset/daos/base.py:605`), non-queryable attributes are
explicitly ignored (`base.py:38` comment). Since `changed_on_humanized` is a computed
`@property` (`superset/models/helpers.py:644-646`), it is not included in SQL-selected
columns (`base.py:29-47`), and rows are returned without that field (`base.py:84-86`).
5. `serialize_chart_object()` reads `changed_on_humanized` via `getattr(chart,
"changed_on_humanized", None)` (`superset/mcp_service/chart/schemas.py:268`), so default
list responses contain null/empty modification-humanized values even though
`columns_requested` includes that field.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/list_charts.py
**Line:** 50:51
**Comment:**
*Logic Error: `changed_on_humanized` is a computed property, not a queryable SQLAlchemy column. In this flow, `ModelListCore` passes default columns to `BaseDAO.list`, which does column-based queries and returns row objects; computed fields are not loaded there, so `changed_on_humanized` will be `None` for every row. Use a queryable timestamp column in defaults (or compute humanized text during serialization) to avoid returning empty values.
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.| "dashboard_title", | ||
| "slug", | ||
| "uuid", | ||
| "url", |
There was a problem hiding this comment.
Suggestion: url is a computed @property on the dashboard model, not a queryable SQLAlchemy column. In list mode, the DAO builds a column-only query from default_columns, so url is skipped during SELECT and the serializer returns null for this field. Use a real persisted column in defaults to avoid returning empty values. [logic error]
Severity Level: Critical 🚨
- ❌ list_dashboards default response returns null URL values.
- ⚠️ MCP clients cannot open dashboards from list output.
- ⚠️ Schema default_select promises field not actually populated.| "url", | |
| "uuid", |
Steps of Reproduction ✅
1. Start MCP service and invoke `list_dashboards` without `select_columns` (tool is
registered via `superset/mcp_service/app.py:402-407`, and exposed in MCP instructions at
`superset/mcp_service/app.py:52`).
2. Request enters `list_dashboards()` at
`superset/mcp_service/dashboard/tool/list_dashboards.py:68`, which calls
`tool.run_tool(...)` at `:130` using `DEFAULT_DASHBOARD_COLUMNS` from `:48-54` (includes
`"url"`).
3. `ModelListCore.run_tool()` sets `columns_to_load = self.default_columns` when no
`select_columns` are provided (`superset/mcp_service/mcp_core.py:161`) and passes them to
DAO list as `columns=columns_to_load` (`:165-173`).
4. `BaseDAO.list()` only keeps SQLAlchemy `ColumnProperty`/`RelationshipProperty` fields
(`superset/daos/base.py:633-636`) and explicitly ignores non-queryable properties
(`:637`), so `"url"` is skipped and query is built from remaining real columns (`:645`).
5. Serializer `serialize_dashboard_object()` reads `url=getattr(dashboard, "url", None)`
(`superset/mcp_service/dashboard/schemas.py:533`), but row objects from column query have
no `url`, so default response returns `url: null`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/list_dashboards.py
**Line:** 52:52
**Comment:**
*Logic Error: `url` is a computed `@property` on the dashboard model, not a queryable SQLAlchemy column. In list mode, the DAO builds a column-only query from `default_columns`, so `url` is skipped during SELECT and the serializer returns `null` for this field. Use a real persisted column in defaults to avoid returning empty values.
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.| "slug", | ||
| "uuid", | ||
| "url", | ||
| "changed_on_humanized", |
There was a problem hiding this comment.
Suggestion: changed_on_humanized is also a computed property, so it is ignored by the DAO's column selection path and will not be loaded from the database in list queries. This makes the default response include an empty field instead of the expected last-modified value; use the persisted timestamp column for reliable output. [logic error]
Severity Level: Major ⚠️
- ❌ Default dashboard list misses humanized last-modified value.
- ⚠️ Users lose quick recency context in MCP responses.
- ⚠️ Prompted field appears but carries empty payload.| "changed_on_humanized", | |
| "changed_on", |
Steps of Reproduction ✅
1. Call MCP `list_dashboards` with default parameters (tool imported/registered in
`superset/mcp_service/app.py:402-407`; default usage documented at `app.py:52`).
2. `list_dashboards()` uses `DEFAULT_DASHBOARD_COLUMNS` containing
`"changed_on_humanized"` (`superset/mcp_service/dashboard/tool/list_dashboards.py:48-54`)
and executes `tool.run_tool(...)` (`:130`).
3. `ModelListCore.run_tool()` forwards default columns directly to DAO
(`superset/mcp_service/mcp_core.py:161,165-173`).
4. In `BaseDAO.list()`, non-queryable properties are ignored
(`superset/daos/base.py:637`), so computed `"changed_on_humanized"` is not selected; only
real DB columns are queried (`:633-645`).
5. `serialize_dashboard_object()` tries `changed_on_humanized=getattr(dashboard,
"changed_on_humanized", None)` (`superset/mcp_service/dashboard/schemas.py:537`),
producing `null` in the returned dashboard list metadata.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/list_dashboards.py
**Line:** 53:53
**Comment:**
*Logic Error: `changed_on_humanized` is also a computed property, so it is ignored by the DAO's column selection path and will not be loaded from the database in list queries. This makes the default response include an empty field instead of the expected last-modified value; use the persisted timestamp column for reliable output.
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.| "table_name", | ||
| "schema", | ||
| "uuid", | ||
| "changed_on_humanized", |
There was a problem hiding this comment.
Suggestion: changed_on_humanized is a computed property, not a SQLAlchemy column, so BaseDAO.list(columns=...) does not load it. With this default, the query returns row objects without that attribute and the serializer emits null for the field. Use changed_on in defaults (or otherwise force full model loading) so the returned default field is actually populated. [logic error]
Severity Level: Major ⚠️
- ⚠️ list_datasets default modified-time field often returns null.
- ⚠️ MCP responses degrade quality for dataset discovery.
- ⚠️ Copilot summaries lose reliable recency information.| "changed_on_humanized", | |
| "changed_on", |
Steps of Reproduction ✅
1. Start MCP service where dataset tools are registered via
`superset/mcp_service/app.py:408-410` (`list_datasets` imported into app startup).
2. Call MCP tool `list_datasets` without `select_columns` (same entry path used in tests
at `tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py:187-190` via
`client.call_tool("list_datasets", ...)`).
3. `list_datasets` uses default columns containing `"changed_on_humanized"` at
`superset/mcp_service/dataset/tool/list_datasets.py:48-53`, then `ModelListCore.run_tool`
sets `columns_to_load=self.default_columns` (`superset/mcp_service/mcp_core.py:161`) and
passes `columns=columns_to_load` to DAO (`superset/mcp_service/mcp_core.py:165-173`).
4. `BaseDAO.list` only includes SQLAlchemy `ColumnProperty` fields
(`superset/daos/base.py:633`) and explicitly ignores non-queryable properties
(`superset/daos/base.py:637`), while `changed_on_humanized` is a Python `@property`
(`superset/models/helpers.py:644-645`), so it is dropped from SQL selection
(`superset/daos/base.py:645`).
5. Serializer reads `changed_on_humanized` with `getattr(dataset, "changed_on_humanized",
None)` (`superset/mcp_service/dataset/schemas.py:329`); for row results missing that
attribute, output becomes `null` for this default field.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/tool/list_datasets.py
**Line:** 52:52
**Comment:**
*Logic Error: `changed_on_humanized` is a computed property, not a SQLAlchemy column, so `BaseDAO.list(columns=...)` does not load it. With this default, the query returns row objects without that attribute and the serializer emits `null` for the field. Use `changed_on` in defaults (or otherwise force full model loading) so the returned default field is actually populated.
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.
Code Review Agent Run #35dab2Actionable 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 |
…ist columns (apache#38566) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> (cherry picked from commit fc156d0)
User description
Summary
When users ask the copilot chatbot "list my dashboards", the LLM displays all default columns from the MCP tool response — including
uuid. Despite system prompt instructions to hide raw technical fields, LLMs reliably display whatever data is in the tool response. Prompt-level instructions are suggestions; data-level changes are deterministic.This PR removes
uuidfrom the default columns in all three list tools and replaces it with user-friendly fields:uuid→url,changed_on_humanizeduuid→url,changed_on_humanizeduuid→changed_on_humanized(nourlsince datasets lack a direct user-facing URL)Changes applied to:
list_dashboards.py,list_charts.py,list_datasets.py(tool default columns)schema_discovery.py(schema metadata defaults +is_defaultflags on extra columns)uuidremains available viaselect_columnsparameter and stays insearch_columns— just no longer returned by default.BEFORE/AFTER
Before —
list_dashboardsdefault response includes:After —
list_dashboardsdefault response includes:TESTING INSTRUCTIONS
list_dashboards,list_charts,list_datasetswith noselect_columnsurl/changed_on_humanizedinstead ofuuidselect_columns=["id", "uuid"]and verifyuuidis still available on demandget_schemafor each model type and verifydefault_selectreflects new columnsADDITIONAL INFORMATION
🤖 Generated with Claude Code
CodeAnt-AI Description
Show URL and humanized last-modified time instead of UUID in default lists
What Changed
Impact
✅ Clearer Copilot list responses✅ Fewer raw UUIDs shown to users✅ Visible last-modified time in dashboard/chart/dataset lists💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.