feat(extensions): add client-side MCP tools API for chat - #43093
feat(extensions): add client-side MCP tools API for chat#43093justinpark wants to merge 1 commit into
Conversation
|
Bito Automatic Review Skipped - Branch Excluded |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dashboard-v2 #43093 +/- ##
===============================================
Coverage ? 65.35%
===============================================
Files ? 2800
Lines ? 158275
Branches ? 36053
===============================================
Hits ? 103440
Misses ? 52857
Partials ? 1978
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| export declare function getTools( | ||
| format: typeof McpToolsFormat.Claude, | ||
| ): ClaudeToolSpec[]; | ||
| export declare function getTools(format: McpToolsFormat): never; |
There was a problem hiding this comment.
Suggestion: The overload for a general McpToolsFormat incorrectly returns never, even though the union includes Claude, which has a valid return value. A caller storing chat.McpToolsFormat.Claude in a variable typed as McpToolsFormat will see the result of getTools(format) typed as unreachable and cannot use the returned tool specifications. Provide a return type that reflects the possible formats, or separate the throwing placeholder formats from the Claude overload. [type error]
Severity Level: Major ⚠️
- ⚠️ Dynamic Claude tool selection fails TypeScript compilation.
- ⚠️ Extension consumers cannot safely use union-typed format variables.
- ❌ Typed integrations cannot send returned tools to Claude APIs.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-core/src/chat/index.ts
**Line:** 260:260
**Comment:**
*Type Error: The overload for a general `McpToolsFormat` incorrectly returns `never`, even though the union includes `Claude`, which has a valid return value. A caller storing `chat.McpToolsFormat.Claude` in a variable typed as `McpToolsFormat` will see the result of `getTools(format)` typed as unreachable and cannot use the returned tool specifications. Provide a return type that reflects the possible formats, or separate the throwing placeholder formats from the Claude overload.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The issue is correct. The current To resolve this, you should update the superset-frontend/packages/superset-core/src/chat/index.ts |
| url: str = Field( | ||
| ..., | ||
| description=( | ||
| "Path to the module exporting the mcpTools factory, resolved " | ||
| "relative to frontend/src/ (e.g. './mcpTools.ts')" | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Suggestion: The schema accepts an empty url, but the webpack template only exposes ./mcpTools for a truthy URL while manifest generation marks any truthy mcpTools object as enabled. An extension using {"url": ""} therefore advertises MCP tools while its container does not expose ./mcpTools, causing extension initialization to fail. Require a non-empty URL in the schema or make manifest generation use the same truthiness condition as the webpack template. [api mismatch]
Severity Level: Major ⚠️
- ❌ Invalid extension configuration can prevent the extension entrypoint from loading.
- ⚠️ Generated metadata advertises an unavailable MCP module.
- ⚠️ Extension startup logs a failure instead of loading normal contributions.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-core/src/superset_core/extensions/types.py
**Line:** 112:118
**Comment:**
*Api Mismatch: The schema accepts an empty `url`, but the webpack template only exposes `./mcpTools` for a truthy URL while manifest generation marks any truthy `mcpTools` object as enabled. An extension using `{"url": ""}` therefore advertises MCP tools while its container does not expose `./mcpTools`, causing extension initialization to fail. Require a non-empty URL in the schema or make manifest generation use the same truthiness condition as the webpack template.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| function getDashboardId(): number | undefined { | ||
| return store.getState().dashboardInfo?.id; |
There was a problem hiding this comment.
Suggestion: dashboardInfo is a merge-based global reducer and is not cleared when navigating away from a classic dashboard, so this accessor returns the last visited dashboard ID on Explore, SQL Lab, or other non-dashboard pages instead of undefined as promised by the public API. Track the active dashboard page lifecycle or clear the reducer state on navigation before exposing this value. [stale reference]
Severity Level: Major ⚠️
- ❌ Chat tools receive the wrong active dashboard identifier.
- ⚠️ Extensions may target a dashboard after navigation.
- ⚠️ Non-dashboard pages expose stale dashboard context.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/core/dashboard/index.ts
**Line:** 50:51
**Comment:**
*Stale Reference: `dashboardInfo` is a merge-based global reducer and is not cleared when navigating away from a classic dashboard, so this accessor returns the last visited dashboard ID on Explore, SQL Lab, or other non-dashboard pages instead of `undefined` as promised by the public API. Track the active dashboard page lifecycle or clear the reducer state on navigation before exposing this value.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const mcpToolsFactory = await container.get('./mcpTools'); | ||
| const mcpToolsModule = mcpToolsFactory() as unknown as McpToolsModule; | ||
| const tools = mcpToolsModule.default(scopedCore.chat); | ||
| registerChatTools(extension.id, tools); |
There was a problem hiding this comment.
Suggestion: The tool registration occurs before the main ./index factory, but its returned Disposable is discarded. If the subsequent factory() throws, initialization reports failure while the extension's MCP tools remain registered and callable, leaving orphaned tools from an extension that was not successfully initialized. Retain the disposable and remove the tools when main-entry initialization fails. [missing cleanup]
Severity Level: Major ⚠️
- ⚠️ Failed extensions leave stale MCP tools callable.
- ⚠️ Chat can invoke handlers from an uninitialized extension.
- ⚠️ Tool listings disagree with extension initialization status.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/extensions/ExtensionsLoader.ts
**Line:** 271:271
**Comment:**
*Missing Cleanup: The tool registration occurs before the main `./index` factory, but its returned `Disposable` is discarded. If the subsequent `factory()` throws, initialization reports failure while the extension's MCP tools remain registered and callable, leaving orphaned tools from an extension that was not successfully initialized. Retain the disposable and remove the tools when main-entry initialization fails.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
Thanks for the PR, @justinpark! I'll go through it in detail and leave comments, but one initial request: could we align the tools registration approach with how other contributions are registered, using a code-first pattern? This mirrors a change we made in the extensions framework, discussed in Extension Contributions: Code-First vs Manifest-First. Concretely, rather than declaring an |
2 participants
SUMMARY
How to Use
Declare
mcpToolsinextension.json, pointing at a file resolved relative tofrontend/src/:{ "mcpTools": { "url": "./mcpTools.ts" } }That file's default export is a
chat.McpToolsFactory—(chat) => McpTool[]. Each tool'snameis unprefixed (surface__name, nocore./extension-id prefix — the host adds that automatically). Fromextensions/chat/frontend/src/mcpTools.ts:Consuming them (from
ChatPanel.tsx):BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION