Copilot commented on code in PR #40359:
URL: https://github.com/apache/superset/pull/40359#discussion_r3326666244


##########
superset/mcp_service/report/tool/create_report.py:
##########
@@ -0,0 +1,163 @@
+# 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 typing import Any
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.report.schemas import (
+    CreateReportRequest,
+    CreateReportResponse,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+    tags=["mutate"],
+    class_permission_name="ReportSchedule",
+    method_permission_name="write",
+    annotations=ToolAnnotations(
+        title="Create a scheduled report or alert",
+        readOnlyHint=False,
+        destructiveHint=False,
+    ),
+)
+async def create_report(
+    request: CreateReportRequest, ctx: Context
+) -> CreateReportResponse:
+    """Create a new scheduled report or alert in Superset.
+
+    Use this tool when the user wants to:
+    - Email a dashboard or chart snapshot on a recurring schedule 
(type='Report')
+    - Trigger a notification when a SQL query result meets a threshold 
(type='Alert')
+
+    Workflow:
+    1. Get dashboard_id from list_dashboards or chart_id from list_charts
+    2. For alerts, also get database_id from list_databases
+    3. Call this tool with the schedule config and at least one recipient
+    4. The returned id can be referenced when managing schedules
+    """
+    await ctx.info(
+        "Creating report schedule: name=%r, type=%s, crontab=%r"
+        % (request.name, request.type, request.crontab)
+    )
+
+    try:
+        # Deferred to avoid circular imports with the @tool decorator 
initialization
+        from superset.commands.report.create import CreateReportScheduleCommand

Review Comment:
   This tool can create alert/report schedules even when the `ALERT_REPORTS` 
feature flag is disabled. The existing REST API, report list view, logs API, 
and scheduler all gate this feature (`superset/reports/api.py:76-80`, 
`superset/views/alerts.py:37-39`, `superset/tasks/scheduler.py:86-87`), so the 
MCP mutation path should enforce the same gate before calling the command.



##########
superset/mcp_service/report/tool/update_report.py:
##########
@@ -0,0 +1,158 @@
+# 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 typing import Any
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.report.schemas import (
+    UpdateReportRequest,
+    UpdateReportResponse,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+    tags=["mutate"],
+    class_permission_name="ReportSchedule",
+    method_permission_name="write",
+    annotations=ToolAnnotations(
+        title="Update a scheduled report or alert",
+        readOnlyHint=False,
+        destructiveHint=True,
+    ),
+)
+async def update_report(
+    request: UpdateReportRequest, ctx: Context
+) -> UpdateReportResponse:
+    """Update an existing scheduled report or alert in Superset.
+
+    Use this tool when the user wants to:
+    - Change the name, schedule, or recipients of an existing report or alert
+    - Enable or disable a schedule without deleting it
+    - Update the SQL condition or database for an existing alert
+
+    Only fields explicitly provided in the request are changed;
+    all omitted fields retain their current values.
+
+    Workflow:
+    1. Get the report id from a previous create_report call or from the 
Superset UI
+    2. Call this tool with the id and only the fields to change
+    3. The returned id confirms which schedule was updated
+    """
+    await ctx.info("Updating report schedule: id=%s" % (request.id,))
+
+    try:
+        # Deferred to avoid circular imports with the @tool decorator 
initialization
+        from superset.commands.report.exceptions import (

Review Comment:
   This tool can update alert/report schedules even when the `ALERT_REPORTS` 
feature flag is disabled. Other alert/report entry points return 404 or no-op 
when the flag is off (`superset/reports/api.py:76-80`, 
`superset/views/alerts.py:37-39`, `superset/tasks/scheduler.py:86-87`), so the 
MCP mutation path should enforce the same gate before running the update 
command.



##########
superset/mcp_service/report/schemas.py:
##########
@@ -0,0 +1,340 @@
+# 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.
+
+from pydantic import BaseModel, ConfigDict, Field, field_validator
+
+
+class RecipientConfig(BaseModel):
+    """Configuration for a single report recipient."""
+
+    model_config = ConfigDict(populate_by_name=True)
+
+    type: str = Field(
+        ...,
+        description=(
+            "Recipient channel type. One of: 'Email', 'Slack', 'SlackV2', 
'Webhook'."
+        ),
+    )
+    target: str = Field(
+        ...,
+        description=(
+            "Destination for the notification. "
+            "For Email: an email address. "
+            "For Slack/SlackV2: a channel name or ID. "
+            "For Webhook: the URL."
+        ),
+    )
+
+    @field_validator("type")
+    @classmethod
+    def type_must_be_valid(cls, v: str) -> str:
+        if v not in (valid_types := {"Email", "Slack", "SlackV2", "Webhook"}):
+            raise ValueError(
+                f"Invalid recipient type {v!r}. Must be one of: 
{sorted(valid_types)}"
+            )
+        return v
+
+    @field_validator("target")
+    @classmethod
+    def target_must_not_be_empty(cls, v: str) -> str:
+        if not v.strip():
+            raise ValueError("target must not be empty")
+        return v.strip()
+
+
+class CreateReportRequest(BaseModel):
+    """Request schema for create_report."""
+
+    model_config = ConfigDict(populate_by_name=True)
+
+    name: str = Field(
+        ...,
+        min_length=1,
+        max_length=150,
+        description="Name for the new report or alert schedule.",
+    )
+    type: str = Field(
+        ...,
+        description=(
+            "Schedule type: 'Report' to deliver a snapshot, "
+            "'Alert' to notify on a SQL condition."
+        ),
+    )
+    crontab: str = Field(
+        ...,
+        description=(
+            "Cron expression defining the execution schedule. "
+            "Examples: '0 9 * * 1' (every Monday at 9 AM), "
+            "'0 8 * * 1-5' (weekdays at 8 AM)."
+        ),
+    )
+    description: str | None = Field(
+        None,
+        description="Optional human-readable description of the schedule.",
+    )
+    active: bool = Field(
+        True,
+        description="Whether the schedule is active immediately after 
creation.",
+    )
+    timezone: str | None = Field(
+        None,
+        description=(
+            "Timezone for interpreting the cron schedule "
+            "(e.g., 'America/New_York', 'UTC'). Defaults to server timezone."
+        ),
+    )
+    recipients: list[RecipientConfig] = Field(
+        default_factory=list,
+        description=(
+            "List of notification recipients. "
+            "For reports/alerts created via this tool, provide at least one 
recipient. "
+            "Example: [{'type': 'Email', 'target': '[email protected]'}]"
+        ),
+    )
+    dashboard_id: int | None = Field(
+        None,
+        description=(
+            "ID of the dashboard to attach this report to. "
+            "Use list_dashboards to find valid IDs. "
+            "Provide either dashboard_id or chart_id, not both."
+        ),
+    )
+    chart_id: int | None = Field(
+        None,
+        description=(
+            "ID of the chart to attach this report to. "
+            "Use list_charts to find valid IDs. "
+            "Provide either chart_id or dashboard_id, not both."
+        ),
+    )
+    database_id: int | None = Field(
+        None,
+        description=(
+            "ID of the database connection for evaluating the alert condition. 
"
+            "Required when type is 'Alert'. Use list_databases to find valid 
IDs."
+        ),
+    )
+    sql: str | None = Field(
+        None,
+        description=(
+            "SQL query whose result is evaluated as the alert condition. "
+            "Used when type is 'Alert'."
+        ),
+    )
+
+    @field_validator("type")
+    @classmethod
+    def type_must_be_valid(cls, v: str) -> str:
+        if v not in (valid_types := {"Report", "Alert"}):
+            raise ValueError(
+                f"Invalid type {v!r}. Must be one of: {sorted(valid_types)}"
+            )
+        return v
+
+    @field_validator("name")
+    @classmethod
+    def name_must_not_be_empty(cls, v: str) -> str:
+        if not v.strip():
+            raise ValueError("name must not be empty")
+        return v.strip()
+
+    @field_validator("crontab")
+    @classmethod
+    def crontab_must_not_be_empty(cls, v: str) -> str:
+        if not v.strip():
+            raise ValueError("crontab must not be empty")
+        return v.strip()

Review Comment:
   `crontab` is only checked for non-empty text here, so invalid cron 
expressions can be passed into `CreateReportScheduleCommand` and persisted when 
the minimum-frequency check is disabled. The existing report API schema 
validates cron syntax with `validate_crontab` 
(`superset/reports/schemas.py:111-113`, `:208-210`); this MCP schema should 
reuse that validation.



##########
superset/mcp_service/report/schemas.py:
##########
@@ -0,0 +1,340 @@
+# 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.
+
+from pydantic import BaseModel, ConfigDict, Field, field_validator
+
+
+class RecipientConfig(BaseModel):
+    """Configuration for a single report recipient."""
+
+    model_config = ConfigDict(populate_by_name=True)
+
+    type: str = Field(
+        ...,
+        description=(
+            "Recipient channel type. One of: 'Email', 'Slack', 'SlackV2', 
'Webhook'."
+        ),
+    )
+    target: str = Field(
+        ...,
+        description=(
+            "Destination for the notification. "
+            "For Email: an email address. "
+            "For Slack/SlackV2: a channel name or ID. "
+            "For Webhook: the URL."
+        ),
+    )
+
+    @field_validator("type")
+    @classmethod
+    def type_must_be_valid(cls, v: str) -> str:
+        if v not in (valid_types := {"Email", "Slack", "SlackV2", "Webhook"}):
+            raise ValueError(
+                f"Invalid recipient type {v!r}. Must be one of: 
{sorted(valid_types)}"
+            )
+        return v
+
+    @field_validator("target")
+    @classmethod
+    def target_must_not_be_empty(cls, v: str) -> str:
+        if not v.strip():
+            raise ValueError("target must not be empty")
+        return v.strip()
+
+
+class CreateReportRequest(BaseModel):
+    """Request schema for create_report."""
+
+    model_config = ConfigDict(populate_by_name=True)
+
+    name: str = Field(
+        ...,
+        min_length=1,
+        max_length=150,
+        description="Name for the new report or alert schedule.",
+    )
+    type: str = Field(
+        ...,
+        description=(
+            "Schedule type: 'Report' to deliver a snapshot, "
+            "'Alert' to notify on a SQL condition."
+        ),
+    )
+    crontab: str = Field(
+        ...,
+        description=(
+            "Cron expression defining the execution schedule. "
+            "Examples: '0 9 * * 1' (every Monday at 9 AM), "
+            "'0 8 * * 1-5' (weekdays at 8 AM)."
+        ),
+    )
+    description: str | None = Field(
+        None,
+        description="Optional human-readable description of the schedule.",
+    )
+    active: bool = Field(
+        True,
+        description="Whether the schedule is active immediately after 
creation.",
+    )
+    timezone: str | None = Field(
+        None,
+        description=(
+            "Timezone for interpreting the cron schedule "
+            "(e.g., 'America/New_York', 'UTC'). Defaults to server timezone."
+        ),
+    )
+    recipients: list[RecipientConfig] = Field(
+        default_factory=list,
+        description=(
+            "List of notification recipients. "
+            "For reports/alerts created via this tool, provide at least one 
recipient. "
+            "Example: [{'type': 'Email', 'target': '[email protected]'}]"
+        ),
+    )
+    dashboard_id: int | None = Field(
+        None,
+        description=(
+            "ID of the dashboard to attach this report to. "
+            "Use list_dashboards to find valid IDs. "
+            "Provide either dashboard_id or chart_id, not both."
+        ),
+    )
+    chart_id: int | None = Field(
+        None,
+        description=(
+            "ID of the chart to attach this report to. "
+            "Use list_charts to find valid IDs. "
+            "Provide either chart_id or dashboard_id, not both."
+        ),
+    )
+    database_id: int | None = Field(
+        None,
+        description=(
+            "ID of the database connection for evaluating the alert condition. 
"
+            "Required when type is 'Alert'. Use list_databases to find valid 
IDs."
+        ),
+    )
+    sql: str | None = Field(
+        None,
+        description=(
+            "SQL query whose result is evaluated as the alert condition. "
+            "Used when type is 'Alert'."
+        ),
+    )
+
+    @field_validator("type")
+    @classmethod
+    def type_must_be_valid(cls, v: str) -> str:
+        if v not in (valid_types := {"Report", "Alert"}):
+            raise ValueError(
+                f"Invalid type {v!r}. Must be one of: {sorted(valid_types)}"
+            )
+        return v
+
+    @field_validator("name")
+    @classmethod
+    def name_must_not_be_empty(cls, v: str) -> str:
+        if not v.strip():
+            raise ValueError("name must not be empty")
+        return v.strip()
+
+    @field_validator("crontab")
+    @classmethod
+    def crontab_must_not_be_empty(cls, v: str) -> str:
+        if not v.strip():
+            raise ValueError("crontab must not be empty")
+        return v.strip()
+
+
+class CreateReportResponse(BaseModel):
+    """Response schema for create_report."""
+
+    id: int | None = Field(
+        None,
+        description="ID of the created report schedule. None if creation 
failed.",
+    )
+    name: str | None = Field(
+        None,
+        description="Name of the created schedule.",
+    )
+    type: str | None = Field(
+        None,
+        description="Type of the schedule ('Report' or 'Alert').",
+    )
+    crontab: str | None = Field(
+        None,
+        description="Cron expression for the schedule.",
+    )
+    active: bool | None = Field(
+        None,
+        description="Whether the schedule is active.",
+    )
+    url: str | None = Field(
+        None,
+        description=(
+            "URL to manage the report schedule in Superset. None if creation 
failed."
+        ),
+    )
+    error: str | None = Field(
+        None,
+        description="Error message if creation failed, otherwise null.",
+    )
+
+
+class UpdateReportRequest(BaseModel):
+    """Request schema for update_report. All fields except id are optional."""
+
+    model_config = ConfigDict(populate_by_name=True)
+
+    id: int = Field(
+        ...,
+        description=(
+            "ID of the report schedule to update. "
+            "Use list_reports or the id returned by create_report."
+        ),
+    )
+    name: str | None = Field(
+        None,
+        max_length=150,
+        description="New name for the schedule. Omit to keep the current 
name.",
+    )
+    type: str | None = Field(
+        None,
+        description=(
+            "Change the schedule type: 'Report' or 'Alert'. "
+            "Omit to keep the current type."
+        ),
+    )
+    crontab: str | None = Field(
+        None,
+        description=(
+            "New cron expression for the schedule "
+            "(e.g., '0 9 * * 1' for every Monday at 9 AM). "
+            "Omit to keep the current schedule."
+        ),
+    )
+    description: str | None = Field(
+        None,
+        description="New description. Omit to keep the current description.",
+    )
+    active: bool | None = Field(
+        None,
+        description=(
+            "Set to True to enable or False to disable the schedule. "
+            "Omit to keep the current state."
+        ),
+    )
+    timezone: str | None = Field(
+        None,
+        description=(
+            "New timezone for interpreting the cron schedule "
+            "(e.g., 'America/New_York'). Omit to keep the current timezone."
+        ),
+    )
+    recipients: list[RecipientConfig] | None = Field(
+        None,
+        description=(
+            "Replacement list of notification recipients. "
+            "Replaces the entire recipient list when provided. "
+            "Omit to keep the current recipients."
+        ),
+    )
+    dashboard_id: int | None = Field(
+        None,
+        description=(
+            "ID of the dashboard to attach this schedule to. "
+            "Omit to keep the current dashboard association."
+        ),
+    )
+    chart_id: int | None = Field(
+        None,
+        description=(
+            "ID of the chart to attach this schedule to. "
+            "Omit to keep the current chart association."
+        ),
+    )
+    database_id: int | None = Field(
+        None,
+        description=(
+            "ID of the database for the alert condition. "
+            "Required when changing type to 'Alert'."
+        ),
+    )
+    sql: str | None = Field(
+        None,
+        description=(
+            "New SQL query for the alert condition. Omit to keep the current 
query."
+        ),
+    )
+
+    @field_validator("type")
+    @classmethod
+    def type_must_be_valid(cls, v: str | None) -> str | None:
+        if v is not None and v not in (valid_types := {"Report", "Alert"}):
+            raise ValueError(
+                f"Invalid type {v!r}. Must be one of: {sorted(valid_types)}"
+            )
+        return v
+
+    @field_validator("name")
+    @classmethod
+    def name_must_not_be_empty(cls, v: str | None) -> str | None:
+        if v is not None and not v.strip():
+            raise ValueError("name must not be empty")
+        return v.strip() if v is not None else v
+
+    @field_validator("crontab")
+    @classmethod
+    def crontab_must_not_be_empty(cls, v: str | None) -> str | None:
+        if v is not None and not v.strip():
+            raise ValueError("crontab must not be empty")
+        return v.strip() if v is not None else v

Review Comment:
   `crontab` updates are only checked for non-empty text, so invalid cron 
expressions can be saved through this tool when the command's minimum-frequency 
check is disabled. The REST update schema validates cron syntax with 
`validate_crontab` (`superset/reports/schemas.py:111-113`, `:365-367`); this 
MCP update schema should do the same before forwarding the value.



##########
superset/mcp_service/report/tool/update_report.py:
##########
@@ -0,0 +1,158 @@
+# 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 typing import Any
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.report.schemas import (
+    UpdateReportRequest,
+    UpdateReportResponse,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+    tags=["mutate"],
+    class_permission_name="ReportSchedule",
+    method_permission_name="write",
+    annotations=ToolAnnotations(
+        title="Update a scheduled report or alert",
+        readOnlyHint=False,
+        destructiveHint=True,
+    ),
+)
+async def update_report(
+    request: UpdateReportRequest, ctx: Context
+) -> UpdateReportResponse:
+    """Update an existing scheduled report or alert in Superset.
+
+    Use this tool when the user wants to:
+    - Change the name, schedule, or recipients of an existing report or alert
+    - Enable or disable a schedule without deleting it
+    - Update the SQL condition or database for an existing alert
+
+    Only fields explicitly provided in the request are changed;
+    all omitted fields retain their current values.
+
+    Workflow:
+    1. Get the report id from a previous create_report call or from the 
Superset UI
+    2. Call this tool with the id and only the fields to change
+    3. The returned id confirms which schedule was updated
+    """
+    await ctx.info("Updating report schedule: id=%s" % (request.id,))
+
+    try:
+        # Deferred to avoid circular imports with the @tool decorator 
initialization
+        from superset.commands.report.exceptions import (
+            ReportScheduleInvalidError,
+            ReportScheduleNotFoundError,
+            ReportScheduleUpdateFailedError,
+        )
+        from superset.commands.report.update import UpdateReportScheduleCommand
+        from superset.mcp_service.utils.url_utils import get_superset_base_url
+        from superset.reports.models import (
+            ReportRecipientType,
+            ReportScheduleType,
+        )
+
+        recipient_type_map = {
+            "Email": ReportRecipientType.EMAIL,
+            "Slack": ReportRecipientType.SLACK,
+            "SlackV2": ReportRecipientType.SLACKV2,
+            "Webhook": ReportRecipientType.WEBHOOK,
+        }
+
+        recipients = None
+        if request.recipients is not None:
+            recipients = [
+                {
+                    "type": recipient_type_map[r.type],
+                    "recipient_config_json": {"target": r.target},
+                }
+                for r in request.recipients
+            ]
+
+        all_props: dict[str, Any] = {
+            "name": request.name,
+            "crontab": request.crontab,
+            "active": request.active,
+            "description": request.description,
+            "timezone": request.timezone,
+            "dashboard": request.dashboard_id,
+            "chart": request.chart_id,
+            "database": request.database_id,
+            "sql": request.sql,
+        }
+        if request.type is not None:
+            all_props["type"] = (
+                ReportScheduleType.ALERT
+                if request.type == "Alert"
+                else ReportScheduleType.REPORT
+            )
+        if recipients is not None:
+            all_props["recipients"] = recipients
+        properties = {k: v for k, v in all_props.items() if v is not None}
+
+        with event_logger.log_context(action="mcp.update_report.update"):
+            schedule = UpdateReportScheduleCommand(request.id, 
properties).run()
+
+        report_url = f"{get_superset_base_url()}/report/list/"
+
+        await ctx.info(
+            "Report schedule updated: id=%s, name=%r, type=%s"
+            % (schedule.id, schedule.name, schedule.type)
+        )
+
+        return UpdateReportResponse(
+            id=schedule.id,
+            name=schedule.name,
+            type=schedule.type,
+            crontab=schedule.crontab,
+            active=schedule.active,
+            url=report_url,
+        )
+
+    except ReportScheduleNotFoundError:
+        await ctx.warning("Report schedule not found: id=%s" % (request.id,))
+        return UpdateReportResponse(
+            id=None,
+            error=f"Report schedule with id={request.id} not found.",
+        )
+    except ReportScheduleInvalidError as exc:
+        messages = exc.normalized_messages()
+        await ctx.warning("Report schedule validation failed: %s" % 
(messages,))
+        return UpdateReportResponse(
+            id=None,
+            error=str(messages),
+        )
+    except ReportScheduleUpdateFailedError as exc:
+        await ctx.error("Report schedule update failed: %s" % (str(exc),))
+        return UpdateReportResponse(
+            id=None,
+            error=f"Failed to update report schedule: {exc}",
+        )
+    except Exception as exc:
+        await ctx.error(
+            "Unexpected error updating report schedule: %s: %s"
+            % (type(exc).__name__, str(exc))
+        )
+        raise

Review Comment:
   `UpdateReportScheduleCommand` raises `ReportScheduleForbiddenError` when the 
caller does not own the schedule 
(`superset/commands/report/update.py:169-173`), but this tool doesn't catch it, 
so an authorization failure falls through to the generic exception path instead 
of returning a structured MCP error like not-found and validation failures do.



##########
superset/mcp_service/report/schemas.py:
##########
@@ -0,0 +1,340 @@
+# 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.
+
+from pydantic import BaseModel, ConfigDict, Field, field_validator
+
+
+class RecipientConfig(BaseModel):
+    """Configuration for a single report recipient."""
+
+    model_config = ConfigDict(populate_by_name=True)
+
+    type: str = Field(
+        ...,
+        description=(
+            "Recipient channel type. One of: 'Email', 'Slack', 'SlackV2', 
'Webhook'."
+        ),
+    )
+    target: str = Field(
+        ...,
+        description=(
+            "Destination for the notification. "
+            "For Email: an email address. "
+            "For Slack/SlackV2: a channel name or ID. "
+            "For Webhook: the URL."
+        ),
+    )
+
+    @field_validator("type")
+    @classmethod
+    def type_must_be_valid(cls, v: str) -> str:
+        if v not in (valid_types := {"Email", "Slack", "SlackV2", "Webhook"}):
+            raise ValueError(
+                f"Invalid recipient type {v!r}. Must be one of: 
{sorted(valid_types)}"
+            )
+        return v
+
+    @field_validator("target")
+    @classmethod
+    def target_must_not_be_empty(cls, v: str) -> str:
+        if not v.strip():
+            raise ValueError("target must not be empty")
+        return v.strip()

Review Comment:
   Email recipients only get a non-empty target check here, and the DAO writes 
the recipient JSON directly without running `ReportRecipientSchema` 
(`superset/daos/report.py:170-181`). The existing REST schema rejects malformed 
email addresses (`superset/reports/schemas.py:144-170`), so invalid email 
targets can be saved through MCP and later fail delivery; validate Email 
targets before building the command payload.



-- 
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]

Reply via email to