Copilot commented on code in PR #40355:
URL: https://github.com/apache/superset/pull/40355#discussion_r3285752633
##########
superset/mcp_service/app.py:
##########
@@ -670,6 +670,9 @@ def create_mcp_app(
get_schema,
health_check,
)
+from superset.mcp_service.theme.tool import ( # noqa: F401, E402
+ create_theme,
+)
Review Comment:
`create_theme` is imported/registered, but it is not advertised in
`get_default_instructions()` (the “Available tools” section). Since these
instructions are what LLM clients see on connect, the new tool will be
effectively undiscoverable in practice. Please add a Theme/Themes section (or
similar) and include a `- create_theme:` bullet, and ensure it’s filtered out
when disabled via `disabled_tools` just like the other tool bullets.
##########
superset/mcp_service/theme/schemas.py:
##########
@@ -0,0 +1,48 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Pydantic schemas for theme-related MCP tool requests and responses.
+"""
+
+from __future__ import annotations
+
+from pydantic import BaseModel, ConfigDict, Field
+
+
+class CreateThemeRequest(BaseModel):
+ model_config = ConfigDict(populate_by_name=True)
+
+ theme_name: str = Field(
+ ...,
+ description="Name of the theme to create.",
+ )
+ json_data: str = Field(
+ ...,
+ description=(
+ "JSON string containing the theme configuration. "
+ "Must be valid Ant Design theme token JSON, e.g. "
+ '\'{"token": {"colorPrimary": "#1677ff"}}\'.'
+ ),
+ )
Review Comment:
The MCP service generally accepts structured objects for JSON-like inputs,
but `CreateThemeRequest.json_data` is typed as a plain string. This forces
clients/LLMs to double-escape JSON and makes it easy to accidentally send a
dict that gets stringified into invalid JSON. Consider accepting `json_data` as
either a dict (native object) or a JSON string and normalizing it before
passing to `ThemePostSchema` (the repo already has
`superset.mcp_service.utils.schema_utils.parse_json_or_passthrough` to support
this pattern).
##########
superset/mcp_service/theme/tool/create_theme.py:
##########
@@ -0,0 +1,113 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+
+from fastmcp import Context
+from marshmallow import ValidationError
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.theme.schemas import (
+ CreateThemeRequest,
+ CreateThemeResponse,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Theme",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Create theme",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def create_theme(
+ request: CreateThemeRequest, ctx: Context
+) -> CreateThemeResponse:
Review Comment:
This PR introduces a new mutate tool that writes to the database, but there
are no unit tests covering its request validation, sanitization behavior, or
successful/failed creation paths. There are existing MCP tool tests (e.g. for
`create_virtual_dataset`) that exercise both schema validation and tool
execution via the MCP client; adding similar tests for `create_theme` would
help prevent regressions and confirm permissions/validation behavior.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
