feat: add mcp abstractions to core #36151
Conversation
villebro
requested review from
eschutho,
geido,
kgabryje,
michael-s-molina and
rusackas
as code owners
Code Review Agent Run #7c88d9Actionable Suggestions - 0Additional Suggestions - 14
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-core/src/superset_core/mcp/init.py | ✅ |
| superset/core/mcp/core_mcp_injection.py | ✅ |
| superset/initialization/init.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #36151 +/- ##
==========================================
+ Coverage 60.48% 68.25% +7.76%
==========================================
Files 1931 638 -1293
Lines 76236 47005 -29231
Branches 8568 5060 -3508
==========================================
- Hits 46114 32081 -14033
+ Misses 28017 13655 -14362
+ Partials 2105 1269 -836
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:
|
| - Do NOT import `Optional` from typing | ||
| - Still import `List`, `Dict`, `Any`, etc. from typing (for now) | ||
| - Do NOT import `Optional`, `List`, `Dict` from typing, prefer `| None`, `list[]` etc |
There was a problem hiding this comment.
Bycatch, I think we've moved from List[] to list[] some time ago.
villebro
force-pushed
the
villebro/mcp-extensions
branch
from
e7caafa to
50a2ae8
Compare
villebro
changed the title
michael-s-molina
left a comment
michael-s-molina
left a comment
There was a problem hiding this comment.
Great work @villebro! I left some non-blocking comments that would be nice to address before merging but overall LGTM.
|
|
||
| @mcp.prompt("create_chart_guided") | ||
| @mcp_auth_hook | ||
| @prompt("create_chart_guided") |
There was a problem hiding this comment.
# Files still using @mcp.resource + @mcp_auth_hook:
- superset/mcp_service/system/resources/instance_metadata.py:30-31
- superset/mcp_service/chart/resources/chart_configs.py:30-31
There was a problem hiding this comment.
These are ok for now, as we don't currently need to expose the MCP resource to extension devs.
There was a problem hiding this comment.
Isn't the idea to always use annotations from the core package independently of extensions?
There was a problem hiding this comment.
@mcp_auth_hook is integrated into @tool and @prompt, but here it's needed here explicitly, as we haven't abstracted @mcp.resource into core as it's not needed yet. But if we do, then we'll replace these with a new @resource that introduces an optional param protect=True.
| def create_tool_decorator( | ||
| func_or_name: str | Callable[..., Any] | None = None, | ||
| *, | ||
| name: Optional[str] = None, | ||
| description: Optional[str] = None, | ||
| tags: Optional[list[str]] = None, | ||
| protect: bool = True, | ||
| ) -> Callable[[F], F] | F: |
There was a problem hiding this comment.
# Current:
def create_tool_decorator(...) -> Callable[[F], F] | F:
# Consider using TypeVar for better type inference:
_F = TypeVar("_F", bound=Callable[..., Any])
def create_tool_decorator(...) -> Callable[[_F], _F] | _F:
There was a problem hiding this comment.
I'm not sure I fully understand this change request. But we can improve this in future iterations as needed.
There was a problem hiding this comment.
Using overloads preserves the decorated function's exact type signature (parameters and return type), enabling IDEs to provide accurate autocomplete and type checkers to catch errors like passing wrong argument types or mismatching return types.
There was a problem hiding this comment.
Let's chat about this in the next sync. I can follow up on this when we agree on the exact nature of this change.
4 participants
SUMMARY
This moves the
@mcp.tool,@mcp.promptand@mcp_auth_hookdecorators intosuperset-core, and replaces them with unified@tooland@promptdecorators that can be used for both authed and un-authed tools/prompts. We roll out the new decorators on all current implementations, and make it possible to add new tools/prompts in extensions (these get discovered during startup by the MCP Server). We also add a new section for MCP tools in the Developer Portal and test the example locally to make sure it works (see screenshot).Some key changes:
@mcp.tooland@mcp_auth_hookinto a new@tooldecorator with an optionalprotect: boolparameter to align with the@protectdecorator used for REST API endpoints. This can be used both within the host application and extensions.@mcp.promptand@mcp_auth_hookinto a new@promptwith a similar signature as@tool.@mcp.tooland@mcp_auth_hookwith the new@toolfromsuperset-core@mcp.promptand@mcp_auth_hookwith the new@promptfromsuperset-coresuperset_core.apidependency injection, irrespective ofENABLE_EXTENSIONSfeature flag, as the public API is now being used in the host codebase.pydanticas a dependency tosuperset-coreto explicitly support MCP tool schema validation. Rebuilt therequirements/*.txtfiles as per the newsuperset-coredependencies.run_proxy.shfrom-rw-r--r--to-rwxr-xr-xto make it executableSCREENSHOTS
Here's a screenshot of one of the example tools from the docs in action. You can see the extension code (left) and using Roo Code to call the Superset MCP (right):
Here's the startup logs for the MCP Server (notice how it's registering the
example_extension.random_numbertool towards the end, and how it's showint that they are protected and not public):TESTING INSTRUCTIONS
ADDITIONAL INFORMATION