codeant-ai-for-open-source[bot] commented on code in PR #40357:
URL: https://github.com/apache/superset/pull/40357#discussion_r3315353892


##########
superset/mcp_service/plugin/schemas.py:
##########
@@ -0,0 +1,127 @@
+# 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 plugin-related MCP tool requests and responses."""
+
+from pydantic import BaseModel, Field, field_validator, model_validator
+
+
+class CreatePluginRequest(BaseModel):
+    """Request schema for create_plugin."""
+
+    name: str = Field(
+        ...,
+        min_length=1,
+        description="Human-friendly name for the plugin.",
+    )
+    key: str = Field(
+        ...,
+        min_length=1,
+        description=(
+            "Unique plugin key. Should match the package name from the 
plugin's "
+            "package.json (e.g. '@my-org/my-chart-plugin')."
+        ),
+    )
+    bundle_url: str = Field(
+        ...,
+        min_length=1,
+        description=(
+            "Full URL pointing to the built plugin bundle "
+            "(e.g. a CDN-hosted JavaScript file)."
+        ),
+    )
+
+    @field_validator("name", "key", "bundle_url")
+    @classmethod
+    def must_not_be_blank(cls, v: str) -> str:
+        if not v.strip():
+            raise ValueError("Field must not be blank")
+        return v.strip()
+
+
+class CreatePluginResponse(BaseModel):
+    """Response schema for create_plugin."""
+
+    id: int | None = Field(
+        None,
+        description="ID of the newly created plugin. None if creation failed.",
+    )
+    name: str | None = Field(None, description="Plugin name.")
+    key: str | None = Field(None, description="Plugin key.")
+    bundle_url: str | None = Field(None, description="Plugin bundle URL.")
+    error: str | None = Field(
+        None,
+        description="Error message if creation failed, otherwise null.",
+    )
+
+
+class UpdatePluginRequest(BaseModel):
+    """Request schema for update_plugin."""
+
+    id: int = Field(..., description="ID of the plugin to update.")
+    name: str | None = Field(
+        None,
+        min_length=1,
+        description="New human-friendly name for the plugin.",
+    )
+    key: str | None = Field(
+        None,
+        min_length=1,
+        description=(
+            "New unique plugin key. Should match the package name from the "
+            "plugin's package.json (e.g. '@my-org/my-chart-plugin')."
+        ),
+    )
+    bundle_url: str | None = Field(
+        None,
+        min_length=1,
+        description=(
+            "New full URL pointing to the built plugin bundle "
+            "(e.g. a CDN-hosted JavaScript file)."
+        ),
+    )

Review Comment:
   **Suggestion:** The update schema has the same gap as create: `bundle_url` 
is treated as any non-empty string, so an invalid URL can be saved and later 
break plugin loading in the client. Apply URL-format validation here as well so 
updates cannot store unusable bundle paths. [incomplete implementation]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ MCP update_plugin can corrupt existing plugin bundle URLs.
   - ⚠️ Updated plugins may disappear or fail to render.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Ensure MCP is running so `update_plugin` is imported and registered via 
`from
   superset.mcp_service.plugin.tool import ... update_plugin` at
   `superset/mcp_service/app.py:62-65`.
   
   2. Create a valid dynamic plugin row (for example via `create_plugin()` at
   `superset/mcp_service/plugin/tool/create_plugin.py:42-91`), then from an MCP 
client call
   the `update_plugin` tool (decorated at
   `superset/mcp_service/plugin/tool/update_plugin.py:32-42`) with a valid `id` 
and
   `bundle_url="invalid-path"` or similar non-URL string.
   
   3. The request is parsed into `UpdatePluginRequest` at
   `superset/mcp_service/plugin/schemas.py:72-111`; the `bundle_url` field at
   `schemas.py:89-96` plus the shared `must_not_be_blank` validator at 
`schemas.py:98-103`
   only enforce non-empty text, so the invalid URL string passes validation as 
long as it is
   non-blank.
   
   4. In `update_plugin()` at 
`superset/mcp_service/plugin/tool/update_plugin.py:80-85`,
   `plugin.bundle_url` is set to this value and committed at 
`update_plugin.py:87-88`,
   storing an unusable bundle URL in the `dynamic_plugin.bundle_url` column
   (`superset/models/dynamic_plugins.py:29`), which then causes subsequent 
client-side
   attempts to load the plugin bundle from this field to fail.
   ```
   </details>
   
   [Fix in 
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0d2669cc915046879edae81913f26dbb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 | [Fix in VSCode 
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0d2669cc915046879edae81913f26dbb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/mcp_service/plugin/schemas.py
   **Line:** 89:96
   **Comment:**
        *Incomplete Implementation: The update schema has the same gap as 
create: `bundle_url` is treated as any non-empty string, so an invalid URL can 
be saved and later break plugin loading in the client. Apply URL-format 
validation here as well so updates cannot store unusable bundle paths.
   
   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
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40357&comment_hash=9dfeaaea4ad06dc0d8b683c4c73190eb1478b6ccf5abed33510c82fd15cbc09d&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40357&comment_hash=9dfeaaea4ad06dc0d8b683c4c73190eb1478b6ccf5abed33510c82fd15cbc09d&reaction=dislike'>👎</a>



##########
superset/mcp_service/plugin/schemas.py:
##########
@@ -0,0 +1,127 @@
+# 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 plugin-related MCP tool requests and responses."""
+
+from pydantic import BaseModel, Field, field_validator, model_validator
+
+
+class CreatePluginRequest(BaseModel):
+    """Request schema for create_plugin."""
+
+    name: str = Field(
+        ...,
+        min_length=1,
+        description="Human-friendly name for the plugin.",
+    )
+    key: str = Field(
+        ...,
+        min_length=1,
+        description=(
+            "Unique plugin key. Should match the package name from the 
plugin's "
+            "package.json (e.g. '@my-org/my-chart-plugin')."
+        ),
+    )
+    bundle_url: str = Field(
+        ...,
+        min_length=1,
+        description=(
+            "Full URL pointing to the built plugin bundle "
+            "(e.g. a CDN-hosted JavaScript file)."
+        ),
+    )

Review Comment:
   **Suggestion:** `bundle_url` is only validated as a non-empty string, so 
invalid values (for example non-URL text) will be persisted successfully and 
then fail later when the frontend tries to dynamically import the plugin 
bundle. Validate this field as a real HTTP/HTTPS URL in the request schema to 
fail fast during creation. [incomplete implementation]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ MCP create_plugin can persist unusable plugin bundle URLs.
   - ⚠️ Dynamic plugin UI may show plugins that never load.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Start Superset with MCP enabled so `init_fastmcp_server()` in
   `superset/mcp_service/app.py:103-179` imports `create_plugin` from
   `superset.mcp_service.plugin.tool` at `app.py:62-65`, registering the tool 
with FastMCP.
   
   2. From an MCP client, call the `create_plugin` tool (decorated at
   `superset/mcp_service/plugin/tool/create_plugin.py:32-42`) with a payload 
where
   `bundle_url` is a non-empty but invalid string such as `"not a url"`.
   
   3. The payload is parsed into `CreatePluginRequest` at
   `superset/mcp_service/plugin/schemas.py:23-47`; `bundle_url` is only 
constrained by
   `min_length=1` and the `must_not_be_blank` validator at `schemas.py:48-53`, 
so any
   non-blank string is accepted and stripped, regardless of URL validity.
   
   4. In `create_plugin()` at `create_plugin.py:73-81`, a `DynamicPlugin` row 
is created with
   `bundle_url=request.bundle_url`, persisting the invalid value into
   `dynamic_plugin.bundle_url` (`superset/models/dynamic_plugins.py:23-30`); 
when the
   Superset UI or other components later load dynamic plugins via 
`DynamicPluginsView`
   (registered in `superset/initialization/__init__.py:205-389`) and attempt to 
fetch the
   bundle from this field, the HTTP request fails and the plugin cannot be 
loaded.
   ```
   </details>
   
   [Fix in 
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8c9b49a59e1741c5a208fc8e707b1877&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 | [Fix in VSCode 
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8c9b49a59e1741c5a208fc8e707b1877&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/mcp_service/plugin/schemas.py
   **Line:** 39:46
   **Comment:**
        *Incomplete Implementation: `bundle_url` is only validated as a 
non-empty string, so invalid values (for example non-URL text) will be 
persisted successfully and then fail later when the frontend tries to 
dynamically import the plugin bundle. Validate this field as a real HTTP/HTTPS 
URL in the request schema to fail fast during creation.
   
   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
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40357&comment_hash=f7790984e121d36e7630156856bfe405b520e959cd1640c4829a61a2c0376818&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40357&comment_hash=f7790984e121d36e7630156856bfe405b520e959cd1640c4829a61a2c0376818&reaction=dislike'>👎</a>



##########
superset/mcp_service/plugin/tool/update_plugin.py:
##########
@@ -0,0 +1,116 @@
+# 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 superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.plugin.schemas import (
+    UpdatePluginRequest,
+    UpdatePluginResponse,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+    tags=["mutate"],
+    class_permission_name="DynamicPlugin",
+    method_permission_name="write",
+    annotations=ToolAnnotations(
+        title="Update a dynamic plugin",
+        readOnlyHint=False,
+        destructiveHint=True,
+    ),
+)
+async def update_plugin(
+    request: UpdatePluginRequest, ctx: Context
+) -> UpdatePluginResponse:
+    """Update an existing dynamic plugin's name, key, or bundle URL.
+
+    Requires admin write access to DynamicPlugin and the DYNAMIC_PLUGINS
+    feature flag to be enabled. At least one of ``name``, ``key``, or
+    ``bundle_url`` must be provided; only the supplied fields are changed.
+
+    Use ``create_plugin`` to look up the plugin ID if you only know the key.
+    """
+    await ctx.info("Updating dynamic plugin: id=%s" % (request.id,))
+
+    try:
+        from sqlalchemy.exc import IntegrityError
+
+        from superset import is_feature_enabled
+        from superset.extensions import db
+        from superset.models.dynamic_plugins import DynamicPlugin
+
+        if not is_feature_enabled("DYNAMIC_PLUGINS"):
+            await ctx.warning("DYNAMIC_PLUGINS feature flag is not enabled")
+            return UpdatePluginResponse(
+                error=(
+                    "The DYNAMIC_PLUGINS feature flag is not enabled on this 
instance."
+                )
+            )
+
+        with event_logger.log_context(action="mcp.update_plugin.lookup"):
+            plugin = db.session.get(DynamicPlugin, request.id)
+
+        if plugin is None:
+            await ctx.warning("Plugin not found: id=%s" % (request.id,))
+            return UpdatePluginResponse(
+                error="No plugin found with id=%d. "
+                "Use the plugin ID returned by create_plugin." % request.id
+            )
+
+        if request.name is not None:
+            plugin.name = request.name
+        if request.key is not None:
+            plugin.key = request.key
+        if request.bundle_url is not None:
+            plugin.bundle_url = request.bundle_url
+
+        with event_logger.log_context(action="mcp.update_plugin.save"):
+            db.session.commit()
+
+        await ctx.info(
+            "Dynamic plugin updated: id=%s, key=%r" % (plugin.id, plugin.key)
+        )
+
+        return UpdatePluginResponse(
+            id=plugin.id,
+            name=plugin.name,
+            key=plugin.key,
+            bundle_url=plugin.bundle_url,
+        )
+
+    except IntegrityError as exc:
+        db.session.rollback()
+        msg = str(exc.orig) if exc.orig else str(exc)
+        await ctx.warning("Plugin update failed (duplicate field): %s" % 
(msg,))
+        return UpdatePluginResponse(
+            error=(
+                "A plugin with the same name, key, or bundle_url already 
exists. "
+                "Each field must be unique."
+            )
+        )
+    except Exception as exc:
+        db.session.rollback()
+        await ctx.error(
+            "Unexpected error updating plugin: %s: %s" % (type(exc).__name__, 
str(exc))
+        )
+        raise

Review Comment:
   **Suggestion:** This has the same error-path issue as `create_plugin`: `db` 
is defined inside the `try` but referenced in the generic `except`. Any failure 
before that import completes will trigger another exception during rollback and 
obscure the root cause. Move `db` import outside `try` or conditionally 
rollback only when available. [incorrect variable usage]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ update_plugin generic error handler can crash on rollback.
   - ⚠️ Root-cause exceptions during MCP updates may be obscured.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Start Superset with MCP so `init_fastmcp_server()` in
   `superset/mcp_service/app.py:103-179` imports and registers `update_plugin` 
via `from
   superset.mcp_service.plugin.tool import ... update_plugin` at 
`app.py:62-65`, binding the
   tool defined at `superset/mcp_service/plugin/tool/update_plugin.py:42-99`.
   
   2. Arrange for `from superset.extensions import db` at `update_plugin.py:59` 
(or `from
   superset import is_feature_enabled` at line 58) to fail with an 
`ImportError` or other
   exception before `db` is bound, while still allowing the module to import so 
the tool
   registration completes.
   
   3. Call the `update_plugin` MCP tool; the call enters the `try` block at
   `update_plugin.py:55`, hits the failing import before executing any 
statement that assigns
   `db`, and raises a non-`IntegrityError` exception that bypasses the `except
   IntegrityError` handler at `update_plugin.py:101-110`.
   
   4. Execution reaches the broad `except Exception as exc` handler at
   `update_plugin.py:111-116`, which immediately calls `db.session.rollback()` 
at line 112
   even though `db` is uninitialized in this code path, raising 
`UnboundLocalError` and
   obscuring the original configuration or import error that broke the tool.
   ```
   </details>
   
   [Fix in 
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fd1f4f31784549509cf61432d9c2d72b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 | [Fix in VSCode 
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fd1f4f31784549509cf61432d9c2d72b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/mcp_service/plugin/tool/update_plugin.py
   **Line:** 55:116
   **Comment:**
        *Incorrect Variable Usage: This has the same error-path issue as 
`create_plugin`: `db` is defined inside the `try` but referenced in the generic 
`except`. Any failure before that import completes will trigger another 
exception during rollback and obscure the root cause. Move `db` import outside 
`try` or conditionally rollback only when available.
   
   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
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40357&comment_hash=87a212bbdf7531909008a9737358cf3697d8fecbb1ba04edea7f6ce1b904d188&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40357&comment_hash=87a212bbdf7531909008a9737358cf3697d8fecbb1ba04edea7f6ce1b904d188&reaction=dislike'>👎</a>



##########
superset/mcp_service/plugin/tool/create_plugin.py:
##########
@@ -0,0 +1,108 @@
+# 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 superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.plugin.schemas import (
+    CreatePluginRequest,
+    CreatePluginResponse,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+    tags=["mutate"],
+    class_permission_name="DynamicPlugin",
+    method_permission_name="write",
+    annotations=ToolAnnotations(
+        title="Register a dynamic plugin",
+        readOnlyHint=False,
+        destructiveHint=False,
+    ),
+)
+async def create_plugin(
+    request: CreatePluginRequest, ctx: Context
+) -> CreatePluginResponse:
+    """Register a new dynamic (custom) plugin in Superset.
+
+    Requires the DYNAMIC_PLUGINS feature flag to be enabled and admin write
+    access to DynamicPlugin. The ``key`` must match the package name from the
+    plugin's package.json and be unique across all registered plugins.
+
+    After registration, Superset will load the plugin bundle from 
``bundle_url``
+    on the next page load.
+    """
+    await ctx.info(
+        "Registering dynamic plugin: name=%r, key=%r" % (request.name, 
request.key)
+    )
+
+    try:
+        from sqlalchemy.exc import IntegrityError
+
+        from superset import is_feature_enabled
+        from superset.extensions import db
+        from superset.models.dynamic_plugins import DynamicPlugin
+
+        if not is_feature_enabled("DYNAMIC_PLUGINS"):
+            await ctx.warning("DYNAMIC_PLUGINS feature flag is not enabled")
+            return CreatePluginResponse(
+                error=(
+                    "The DYNAMIC_PLUGINS feature flag is not enabled on this 
instance."
+                )
+            )
+
+        with event_logger.log_context(action="mcp.create_plugin.create"):
+            plugin = DynamicPlugin(
+                name=request.name,
+                key=request.key,
+                bundle_url=request.bundle_url,
+            )
+            db.session.add(plugin)
+            db.session.commit()
+
+        await ctx.info(
+            "Dynamic plugin registered: id=%s, key=%r" % (plugin.id, 
plugin.key)
+        )
+
+        return CreatePluginResponse(
+            id=plugin.id,
+            name=plugin.name,
+            key=plugin.key,
+            bundle_url=plugin.bundle_url,
+        )
+
+    except IntegrityError as exc:
+        db.session.rollback()
+        msg = str(exc.orig) if exc.orig else str(exc)
+        await ctx.warning("Plugin creation failed (duplicate field): %s" % 
(msg,))
+        return CreatePluginResponse(
+            error=(
+                "A plugin with the same name, key, or bundle_url already 
exists. "
+                "Each field must be unique."
+            )
+        )
+    except Exception as exc:
+        db.session.rollback()
+        await ctx.error(
+            "Unexpected error creating plugin: %s: %s" % (type(exc).__name__, 
str(exc))
+        )
+        raise

Review Comment:
   **Suggestion:** `db` is imported inside the `try` block but used 
unconditionally in the broad exception handler; if an earlier statement in the 
`try` fails before `db` is bound, the handler raises `UnboundLocalError` and 
masks the original failure. Import `db` outside the `try` (or guard rollback) 
so error handling cannot crash. [incorrect variable usage]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ create_plugin error handling can raise UnboundLocalError.
   - ⚠️ Original MCP failure reason hidden from operators and logs.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Run Superset with MCP enabled so `init_fastmcp_server()` in
   `superset/mcp_service/app.py:103-179` imports `create_plugin` from
   `superset.mcp_service.plugin.tool` at `app.py:62-65`, registering the 
`create_plugin()`
   tool defined at `superset/mcp_service/plugin/tool/create_plugin.py:42-91`.
   
   2. Simulate a misconfigured or partially broken installation where `from
   superset.extensions import db` at `create_plugin.py:62` raises an 
`ImportError` (for
   example, by having `superset.extensions` fail to import), while still 
allowing the module
   to load so the tool is registered.
   
   3. Call the `create_plugin` MCP tool; the function enters the `try` block at
   `create_plugin.py:58`, hits the failing import at line 62 before `db` is 
ever bound, and
   raises `ImportError`, which is not an `IntegrityError` so it bypasses the 
`except
   IntegrityError` handler at `create_plugin.py:93-102`.
   
   4. Control flows into the broad `except Exception as exc` handler at
   `create_plugin.py:103-108`, which executes `db.session.rollback()` at line 
104 even though
   `db` was never successfully assigned, causing an `UnboundLocalError: local 
variable 'db'
   referenced before assignment` that masks the original import failure and 
prevents a
   correct error message from being logged via `ctx.error`.
   ```
   </details>
   
   [Fix in 
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=271a768338f84b1a947670488d6635db&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 | [Fix in VSCode 
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=271a768338f84b1a947670488d6635db&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/mcp_service/plugin/tool/create_plugin.py
   **Line:** 58:108
   **Comment:**
        *Incorrect Variable Usage: `db` is imported inside the `try` block but 
used unconditionally in the broad exception handler; if an earlier statement in 
the `try` fails before `db` is bound, the handler raises `UnboundLocalError` 
and masks the original failure. Import `db` outside the `try` (or guard 
rollback) so error handling cannot crash.
   
   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
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40357&comment_hash=0d805ea3e8c193b48cc16f18fb91a41e80a5ffa579094e976bb9e95cdc184e7b&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40357&comment_hash=0d805ea3e8c193b48cc16f18fb91a41e80a5ffa579094e976bb9e95cdc184e7b&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: [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