You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/03/13 22:39:59 UTC
[PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
aminghadersohi opened a new pull request, #38641:
URL: https://github.com/apache/superset/pull/38641
### SUMMARY
Add `ToolAnnotations` (`title`, `readOnlyHint`, `destructiveHint`) to all 20 MCP tools so that MCP directory services and clients can surface human-readable titles and understand each tool's safety profile.
**What changed:**
1. **Abstract decorator** (`superset-core/src/superset_core/mcp/decorators.py`): Added `annotations: ToolAnnotations | None = None` parameter and re-exported `ToolAnnotations` from `mcp.types` in `__all__`.
2. **Concrete decorator** (`superset/core/mcp/core_mcp_injection.py`): Added `annotations` parameter that gets passed through to `Tool.from_function()`.
3. **All 20 tool files**: Added `ToolAnnotations` import and `annotations=ToolAnnotations(title=..., readOnlyHint=..., destructiveHint=False)` to each `@tool(...)` decorator:
- **14 read-only tools** (`readOnlyHint=True`): health_check, get_instance_info, get_schema, list_charts, get_chart_info, get_chart_preview, get_chart_data, list_dashboards, get_dashboard_info, list_datasets, get_dataset_info, execute_sql, open_sql_lab_with_context, generate_explore_link
- **6 mutating tools** (`readOnlyHint=False`): generate_chart, update_chart, update_chart_preview, generate_dashboard, add_chart_to_existing_dashboard, save_sql_query
**Why:**
The MCP spec defines `ToolAnnotations` to help clients and directory services understand tool behavior. Adding `title` provides human-readable display names, `readOnlyHint` tells clients which tools only read data, and `destructiveHint=False` signals that no tool performs irreversible destructive operations.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (backend-only change, no UI impact).
### TESTING INSTRUCTIONS
1. Verify all 20 tool files have `ToolAnnotations` in their `@tool()` decorator
2. Verify read-only tools have `readOnlyHint=True` and mutating tools have `readOnlyHint=False`
3. Verify all tools have `destructiveHint=False`
4. Run `ruff check` and `ruff format --check` on all changed files to confirm linting passes
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #38641:
URL: https://github.com/apache/superset/pull/38641#issuecomment-4058466890
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/38641?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 65.16%. Comparing base ([`1867336`](https://app.codecov.io/gh/apache/superset/commit/186733690761b16d4856e0a7fca09c6f5204b758?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8869ee4`](https://app.codecov.io/gh/apache/superset/commit/8869ee4053323ff2665a459130d13cef36099937?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 12 commits behind head on master.
:x: Your project check has failed because the head coverage (99.92%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #38641 +/- ##
==========================================
+ Coverage 65.01% 65.16% +0.15%
==========================================
Files 1817 1824 +7
Lines 72348 73036 +688
Branches 23044 23149 +105
==========================================
+ Hits 47038 47596 +558
- Misses 25310 25440 +130
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/38641/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset-extensions-cli](https://app.codecov.io/gh/apache/superset/pull/38641/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `89.82% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/38641?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #38641:
URL: https://github.com/apache/superset/pull/38641#discussion_r2943627434
##########
superset/core/mcp/core_mcp_injection.py:
##########
@@ -62,6 +67,7 @@ def create_tool_decorator(
protect: bool = True,
class_permission_name: Optional[str] = None,
method_permission_name: Optional[str] = None,
+ annotations: Optional[ToolAnnotations] = None,
Review Comment:
Fixed in a623352 — switched to `ToolAnnotations | None` syntax.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #38641:
URL: https://github.com/apache/superset/pull/38641#discussion_r2935253491
##########
superset-core/src/superset_core/mcp/decorators.py:
##########
@@ -37,6 +37,8 @@ def another_tool(value: int) -> str:
from typing import Any, Callable, TypeVar
+from mcp.types import ToolAnnotations
Review Comment:
Fixed - wrapped in try/except with fallback to `Any` so superset-core remains importable without MCP extras. Fixed in ec9aec2.
##########
superset/core/mcp/core_mcp_injection.py:
##########
@@ -25,6 +25,8 @@
import logging
from typing import Any, Callable, Optional, TypeVar
+from mcp.types import ToolAnnotations
Review Comment:
Fixed - same try/except pattern applied here. Fixed in ec9aec2.
##########
superset/mcp_service/explore/tool/generate_explore_link.py:
##########
@@ -39,7 +39,15 @@
from superset.mcp_service.utils.schema_utils import parse_request
-@tool(tags=["explore"], class_permission_name="Explore")
+@tool(
+ tags=["explore"],
+ class_permission_name="Explore",
+ annotations=ToolAnnotations(
+ title="Generate explore link",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
Review Comment:
Good catch - set `readOnlyHint=False` since it writes form_data_key cache entries via `MCPCreateFormDataCommand.run()`. Fixed in ec9aec2.
##########
superset/mcp_service/sql_lab/tool/save_sql_query.py:
##########
@@ -43,7 +43,14 @@
logger = logging.getLogger(__name__)
-@tool(tags=["mutate"])
+@tool(
+ tags=["mutate"],
+ annotations=ToolAnnotations(
Review Comment:
Fixed - added `class_permission_name="SQLLab"` and `method_permission_name="write"` for proper RBAC enforcement, consistent with `execute_sql`. Fixed in ec9aec2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38641:
URL: https://github.com/apache/superset/pull/38641#discussion_r2939144745
##########
superset-core/src/superset_core/mcp/decorators.py:
##########
@@ -37,6 +37,13 @@ def another_tool(value: int) -> str:
from typing import Any, Callable, TypeVar
+try:
+ from mcp.types import ToolAnnotations
+except (
+ ImportError
+): # MCP extras may not be installed in superset-core-only environments
+ ToolAnnotations = Any
Review Comment:
**Suggestion:** The fallback `ToolAnnotations = Any` is not callable, so in environments where `mcp.types` is unavailable, every `ToolAnnotations(...)` usage in tool modules raises `TypeError: Any cannot be instantiated` at import time. Replace the fallback with a callable lightweight type (for example `dict`) so annotation objects can still be constructed when MCP extras are missing. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ `superset mcp run` fails during module import.
- ❌ All annotated MCP tools fail registration path.
- ⚠️ Optional-dependency fallback does not degrade gracefully.
```
</details>
```suggestion
ToolAnnotations = dict
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use an environment where `mcp.types` is unavailable (the fallback path is explicitly
intended in `superset-core/src/superset_core/mcp/decorators.py:40-45`).
2. Start MCP via CLI: `superset mcp run`, which executes `run()` in
`superset/cli/mcp.py:32-37` and imports `superset.mcp_service.server` at
`superset/cli/mcp.py:35`.
3. `superset/mcp_service/server.py:32` imports `superset.mcp_service.app`, and
`superset/mcp_service/app.py:395-429` imports all MCP tool modules.
4. Tool modules instantiate `ToolAnnotations(...)` at import time (example:
`superset/mcp_service/system/tool/health_check.py:39`; same pattern found across all 20
tools), but fallback `ToolAnnotations = Any` from `decorators.py:45` raises `TypeError:
Any cannot be instantiated` (verified by runtime check), so module import and MCP startup
fail.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-core/src/superset_core/mcp/decorators.py
**Line:** 45:45
**Comment:**
*Type Error: The fallback `ToolAnnotations = Any` is not callable, so in environments where `mcp.types` is unavailable, every `ToolAnnotations(...)` usage in tool modules raises `TypeError: Any cannot be instantiated` at import time. Replace the fallback with a callable lightweight type (for example `dict`) so annotation objects can still be constructed when MCP extras are missing.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=43ef5dab124bc0dfb25874c189a83f857a02e4489bcdac068fd697399e6fbcbb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=43ef5dab124bc0dfb25874c189a83f857a02e4489bcdac068fd697399e6fbcbb&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #38641:
URL: https://github.com/apache/superset/pull/38641#discussion_r2943377267
##########
superset-core/src/superset_core/mcp/decorators.py:
##########
@@ -37,6 +37,13 @@ def another_tool(value: int) -> str:
from typing import Any, Callable, TypeVar
+try:
+ from mcp.types import ToolAnnotations
+except (
+ ImportError
+): # MCP extras may not be installed in superset-core-only environments
+ ToolAnnotations = Any
Review Comment:
Fixed in commit 3d27a32 — changed the fallback from `Any` to `dict` in both `superset-core/src/superset_core/mcp/decorators.py` and `superset/core/mcp/core_mcp_injection.py`. `dict(...)` is callable with keyword args, so `ToolAnnotations(readOnlyHint=True)` will work correctly even when `mcp.types` is not installed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38641:
URL: https://github.com/apache/superset/pull/38641#issuecomment-4071322973
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38641:
URL: https://github.com/apache/superset/pull/38641#issuecomment-4071645913
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #38641:
URL: https://github.com/apache/superset/pull/38641
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38641:
URL: https://github.com/apache/superset/pull/38641#issuecomment-4058461600
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #38641:
URL: https://github.com/apache/superset/pull/38641#discussion_r2934305745
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -44,7 +44,16 @@
logger = logging.getLogger(__name__)
-@tool(tags=["mutate"], class_permission_name="Chart", method_permission_name="write")
+@tool(
+ tags=["mutate"],
+ class_permission_name="Chart",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Update chart preview",
+ readOnlyHint=False,
+ destructiveHint=False,
Review Comment:
I believe an update would be considered a destructiveHint
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #38641:
URL: https://github.com/apache/superset/pull/38641#discussion_r2943576326
##########
superset/core/mcp/core_mcp_injection.py:
##########
@@ -62,6 +67,7 @@ def create_tool_decorator(
protect: bool = True,
class_permission_name: Optional[str] = None,
method_permission_name: Optional[str] = None,
+ annotations: Optional[ToolAnnotations] = None,
Review Comment:
nit:
```suggestion
annotations: ToolAnnotations | None = None,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38641:
URL: https://github.com/apache/superset/pull/38641#discussion_r2935237586
##########
superset-core/src/superset_core/mcp/decorators.py:
##########
@@ -37,6 +37,8 @@ def another_tool(value: int) -> str:
from typing import Any, Callable, TypeVar
+from mcp.types import ToolAnnotations
Review Comment:
**Suggestion:** This introduces a hard runtime dependency on `mcp` in `apache-superset-core`, but `superset-core/pyproject.toml` does not include `mcp`/`fastmcp`. Importing this module will raise `ModuleNotFoundError` in environments without the MCP extra. Make the import optional and provide a fallback alias so the abstract decorators remain importable. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ README MCP tool example crashes on import.
- ❌ `superset_core.mcp.decorators` unusable in core-only installs.
- ⚠️ Extension authors blocked without optional MCP extras.
```
</details>
```suggestion
try:
from mcp.types import ToolAnnotations
except ImportError: # MCP extras may not be installed in superset-core-only environments
ToolAnnotations = Any # type: ignore[assignment]
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Install only `apache-superset-core` as documented in `superset-core/README.md:30-32`
(`pip install apache-superset-core`), which uses dependencies declared in
`superset-core/pyproject.toml:44-51` and does not include `mcp`/`fastmcp`.
2. Follow the documented MCP quick-start import in `superset-core/README.md:82` (`from
superset_core.mcp.decorators import tool`) or `README.md:92` (`prompt`).
3. Python imports `superset-core/src/superset_core/mcp/decorators.py`, which executes
`from mcp.types import ToolAnnotations` at line 40 before any function call.
4. In environments without MCP extras, import fails immediately with `ModuleNotFoundError:
No module named 'mcp'`, preventing use of the abstract decorators that are intended to be
importable from `superset_core`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-core/src/superset_core/mcp/decorators.py
**Line:** 40:40
**Comment:**
*Possible Bug: This introduces a hard runtime dependency on `mcp` in `apache-superset-core`, but `superset-core/pyproject.toml` does not include `mcp`/`fastmcp`. Importing this module will raise `ModuleNotFoundError` in environments without the MCP extra. Make the import optional and provide a fallback alias so the abstract decorators remain importable.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=a2b8a2e6a9965ed27249066d4a12586307ebee49e8d2173e0f877094611e223b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=a2b8a2e6a9965ed27249066d4a12586307ebee49e8d2173e0f877094611e223b&reaction=dislike'>👎</a>
##########
superset/mcp_service/explore/tool/generate_explore_link.py:
##########
@@ -39,7 +39,15 @@
from superset.mcp_service.utils.schema_utils import parse_request
-@tool(tags=["explore"], class_permission_name="Explore")
+@tool(
+ tags=["explore"],
+ class_permission_name="Explore",
+ annotations=ToolAnnotations(
+ title="Generate explore link",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
Review Comment:
**Suggestion:** The tool is marked as read-only, but it calls logic that creates a new `form_data_key` cache entry (`MCPCreateFormDataCommand.run()`), which is a state mutation. This incorrect annotation can cause MCP clients/directories to treat a mutating tool as safe read-only behavior. Set `readOnlyHint` to `False` to match actual side effects. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP metadata mislabels mutating `generate_explore_link` behavior.
- ⚠️ Clients may treat cache-writing tool as read-only.
- ⚠️ Directory safety profile for explore tool becomes inaccurate.
```
</details>
```suggestion
annotations=ToolAnnotations(
title="Generate explore link",
readOnlyHint=False,
destructiveHint=False,
),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start MCP service and invoke `generate_explore_link` through MCP client exactly like
unit tests do at
`tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py:119-122`
(`client.call_tool("generate_explore_link", ...)`).
2. Request enters tool function `generate_explore_link()` in
`superset/mcp_service/explore/tool/generate_explore_link.py:52`; decorator metadata
currently marks `readOnlyHint=True` at `:47`.
3. The tool calls `generate_url(...)` at
`superset/mcp_service/explore/tool/generate_explore_link.py:141-143`, which resolves to
`chart_utils.generate_explore_link()` at `superset/mcp_service/chart/chart_utils.py:152`.
4. `chart_utils.generate_explore_link()` executes
`MCPCreateFormDataCommand(cmd_params).run()` at
`superset/mcp_service/chart/chart_utils.py:205`; underlying `CreateFormDataCommand.run()`
writes cache entries via `cache_manager.explore_form_data_cache.set(...)` at
`superset/commands/explore/form_data/create.py:67-68`, proving server-side state mutation
despite read-only annotation.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/explore/tool/generate_explore_link.py
**Line:** 45:49
**Comment:**
*Logic Error: The tool is marked as read-only, but it calls logic that creates a new `form_data_key` cache entry (`MCPCreateFormDataCommand.run()`), which is a state mutation. This incorrect annotation can cause MCP clients/directories to treat a mutating tool as safe read-only behavior. Set `readOnlyHint` to `False` to match actual side effects.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=254f0be6597885d9fb69f71d780904c652371b2f6b3c05f1118b0c9ca3f828df&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=254f0be6597885d9fb69f71d780904c652371b2f6b3c05f1118b0c9ca3f828df&reaction=dislike'>👎</a>
##########
superset/core/mcp/core_mcp_injection.py:
##########
@@ -25,6 +25,8 @@
import logging
from typing import Any, Callable, Optional, TypeVar
+from mcp.types import ToolAnnotations
Review Comment:
**Suggestion:** This unconditional import introduces a hard runtime dependency on `mcp` during normal Superset startup, but MCP support is optional (`fastmcp` extra). Since this module is imported from core initialization even when MCP extras are not installed, `ModuleNotFoundError` can crash app initialization before the existing fastmcp availability guard runs. Make the import optional with a fallback type alias so startup still works without MCP packages. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ create_app() fails without optional MCP dependencies.
- ❌ Celery bootstrap fails via superset/tasks/celery_app.py:32.
- ⚠️ fastmcp guard never executes due earlier import crash.
```
</details>
```suggestion
try:
from mcp.types import ToolAnnotations
except ImportError:
ToolAnnotations = Any # type: ignore[assignment]
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset through normal app bootstrap (`superset/app.py:47-55`), which always
calls `app_initializer.init_app()`.
2. During initialization, `SupersetAppInitializer.init_app()` enters app-context init
(`superset/initialization/__init__.py:728-740`) and calls
`init_all_dependencies_and_extensions()` (`:626`), which always calls
`init_core_dependencies()` (`:543-554`).
3. `init_core_dependencies()` unconditionally imports `initialize_core_mcp_dependencies`
from `superset.core.mcp.core_mcp_injection`
(`superset/initialization/__init__.py:547-549`), so Python evaluates module top-level
imports first.
4. In environments without MCP packages installed, top-level `from mcp.types import
ToolAnnotations` at `superset/core/mcp/core_mcp_injection.py:28` raises
`ModuleNotFoundError` before the internal `fastmcp` guard
(`core_mcp_injection.py:281-288`) can run, aborting startup.
5. This is realistic because MCP is optional (`pyproject.toml:147` defines `fastmcp` as an
extra), so non-MCP deployments can hit this on standard startup paths.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/core/mcp/core_mcp_injection.py
**Line:** 28:28
**Comment:**
*Possible Bug: This unconditional import introduces a hard runtime dependency on `mcp` during normal Superset startup, but MCP support is optional (`fastmcp` extra). Since this module is imported from core initialization even when MCP extras are not installed, `ModuleNotFoundError` can crash app initialization before the existing fastmcp availability guard runs. Make the import optional with a fallback type alias so startup still works without MCP packages.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=bb7d97f0b6037f45a6a6b0f5f5282635af7082eb8c054b5890932ffd3330a596&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=bb7d97f0b6037f45a6a6b0f5f5282635af7082eb8c054b5890932ffd3330a596&reaction=dislike'>👎</a>
##########
superset/mcp_service/sql_lab/tool/save_sql_query.py:
##########
@@ -43,7 +43,14 @@
logger = logging.getLogger(__name__)
-@tool(tags=["mutate"])
+@tool(
+ tags=["mutate"],
+ annotations=ToolAnnotations(
Review Comment:
**Suggestion:** This tool is registered without `class_permission_name`, so MCP RBAC defaults to allow and skips Superset view-level permission checks. As a result, any authenticated user who can access a database can invoke this mutating endpoint even if they do not have SQL Lab permissions. Add SQL Lab RBAC metadata to the decorator so permission checks are enforced consistently with other SQL Lab tools. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ SQL Lab RBAC can be bypassed for save_sql_query.
- ❌ Unauthorized users may create persisted SavedQuery records.
- ⚠️ Inconsistent authorization versus execute_sql SQLLab tool.
```
</details>
```suggestion
class_permission_name="SQLLab",
method_permission_name="write",
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start MCP server initialization; `superset/mcp_service/app.py:418-421` imports
`save_sql_query`, so the decorator in
`superset/mcp_service/sql_lab/tool/save_sql_query.py:46-53` is executed during tool
registration.
2. In `superset/core/mcp/core_mcp_injection.py:110-121`, RBAC attributes are only attached
when `class_permission_name` is provided; this tool omits it, so no
`_class_permission_name`/`_method_permission_name` gets set.
3. Invoke MCP `save_sql_query` (documented as a normal workflow tool in
`superset/mcp_service/app.py:73,109`); execution enters `mcp_auth_hook` at
`superset/mcp_service/auth.py:55`.
4. `check_tool_permission()` at `superset/mcp_service/auth.py:103-106` returns
allow-by-default when class permission metadata is missing (also validated by
`tests/unit_tests/mcp_service/test_auth_rbac.py:103-108`), so SQL Lab view-level RBAC is
skipped.
5. The function then only enforces database access (`save_sql_query.py:91`) and proceeds
to mutate state with `SavedQueryDAO.create(...)` at `save_sql_query.py:102-113`, allowing
saved-query creation without explicit SQLLab permission checks.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/sql_lab/tool/save_sql_query.py
**Line:** 48:48
**Comment:**
*Security: This tool is registered without `class_permission_name`, so MCP RBAC defaults to allow and skips Superset view-level permission checks. As a result, any authenticated user who can access a database can invoke this mutating endpoint even if they do not have SQL Lab permissions. Add SQL Lab RBAC metadata to the decorator so permission checks are enforced consistently with other SQL Lab tools.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=6fb90b4b5f82a067fad9c3564e6b7f8c596b663ab2358d809f35ef063be17ef6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38641&comment_hash=6fb90b4b5f82a067fad9c3564e6b7f8c596b663ab2358d809f35ef063be17ef6&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38641:
URL: https://github.com/apache/superset/pull/38641#issuecomment-4060559047
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): Add tool annotations for MCP directory compliance [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38641:
URL: https://github.com/apache/superset/pull/38641#issuecomment-4058865623
<!-- Bito Reply -->
In MCP tool annotations, 'destructiveHint' indicates if the tool performs operations that are hard to undo, like deletions. For update operations like this one, setting it to False is standard since updates can be reverted. If you meant something else by 'update', please clarify.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org