korbit-ai[bot] commented on code in PR #35163: URL: https://github.com/apache/superset/pull/35163#discussion_r2353708161
########## superset/cli/mcp.py: ########## @@ -0,0 +1,46 @@ +# 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. +"""CLI module for MCP service""" + +import click +from flask.cli import with_appcontext + +from superset.mcp_service.scripts.setup import run_setup +from superset.mcp_service.server import run_server + + [email protected]() +def mcp() -> None: + """Model Context Protocol service commands""" + pass + + [email protected]() [email protected]("--host", default="127.0.0.1", help="Host to bind to") [email protected]("--port", default=5008, help="Port to bind to") Review Comment: ### Missing port number validation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The port parameter accepts any integer value without validation, potentially allowing invalid port numbers. ###### Why this matters Invalid port numbers (negative, zero, or above 65535) could cause runtime errors or unexpected behavior when the server attempts to bind to the port. ###### Suggested change โ *Feature Preview* Add click validation to ensure the port is within the valid range (1-65535): ```python @click.option("--port", default=5008, type=click.IntRange(1, 65535), help="Port to bind to") ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6d60ad32-1287-425b-bea8-01b52604dfb5/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6d60ad32-1287-425b-bea8-01b52604dfb5?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6d60ad32-1287-425b-bea8-01b52604dfb5?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6d60ad32-1287-425b-bea8-01b52604dfb5?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6d60ad32-1287-425b-bea8-01b52604dfb5) </details> <sub> ๐ฌ Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:eb14c942-e4bd-4d85-8880-2fda95fd800d --> [](eb14c942-e4bd-4d85-8880-2fda95fd800d) ########## .devcontainer/setup-dev.sh: ########## @@ -3,76 +3,30 @@ echo "๐ง Setting up Superset development environment..." -# System dependencies and uv are now pre-installed in the Docker image -# This speeds up Codespace creation significantly! - -# Create virtual environment using uv -echo "๐ Creating Python virtual environment..." -if ! uv venv; then - echo "โ Failed to create virtual environment" - exit 1 -fi - -# Install Python dependencies -echo "๐ฆ Installing Python dependencies..." -if ! uv pip install -r requirements/development.txt; then - echo "โ Failed to install Python dependencies" - echo "๐ก You may need to run this manually after the Codespace starts" - exit 1 -fi - -# Install pre-commit hooks -echo "๐ช Installing pre-commit hooks..." -if source .venv/bin/activate && pre-commit install; then - echo "โ Pre-commit hooks installed" -else - echo "โ ๏ธ Pre-commit hooks installation failed (non-critical)" -fi +# The universal image has most tools, just need Superset-specific libs +echo "๐ฆ Installing Superset-specific dependencies..." +sudo apt-get update +sudo apt-get install -y \ + libsasl2-dev \ + libldap2-dev \ + libpq-dev \ + tmux \ + gh Review Comment: ### Mixed Dependency Types <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? System dependencies are mixed with development tools in a single installation command. ###### Why this matters Mixing different types of dependencies reduces code clarity and makes it harder to maintain the list of required packages for different purposes. ###### Suggested change โ *Feature Preview* Separate system dependencies from development tools: ```bash # Install system dependencies sudo apt-get install -y \ libsasl2-dev \ libldap2-dev \ libpq-dev # Install development tools sudo apt-get install -y \ tmux \ gh ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3419326e-c919-42cf-b3e3-67ec06946ff0/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3419326e-c919-42cf-b3e3-67ec06946ff0?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3419326e-c919-42cf-b3e3-67ec06946ff0?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3419326e-c919-42cf-b3e3-67ec06946ff0?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3419326e-c919-42cf-b3e3-67ec06946ff0) </details> <sub> ๐ฌ Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:c42f2437-ce7e-419f-af00-8667f41a3454 --> [](c42f2437-ce7e-419f-af00-8667f41a3454) ########## superset/mcp_service/index.js: ########## @@ -0,0 +1,37 @@ +/** + * Apache Superset MCP Server + * + * Entry point for the MCP server when used as a Node.js module. + */ + +const { spawn } = require('child_process'); +const path = require('path'); + +class SupersetMCPServer { + constructor(options = {}) { + this.options = { + transport: options.transport || 'http', + host: options.host || '127.0.0.1', + port: options.port || 5008, + debug: options.debug || false, + pythonPath: options.pythonPath || null, + supersetRoot: options.supersetRoot || null, + configPath: options.configPath || null, + }; + this.process = null; + } + + start() { + const runner = require('./bin/superset-mcp.js'); + // The bin script handles the execution + } Review Comment: ### Non-functional start method <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The start() method requires the bin script but never executes it or stores a reference to control the process lifecycle. ###### Why this matters This makes the start() method non-functional as it doesn't actually start any server process, and the stop() method cannot work since this.process remains null. ###### Suggested change โ *Feature Preview* Execute the runner and store the process reference: ```javascript start() { if (this.process) { throw new Error('Server is already running'); } const runner = require('./bin/superset-mcp.js'); this.process = runner.start(this.options); return this.process; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1c500c59-9e23-4608-98fb-7c6341de4625/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1c500c59-9e23-4608-98fb-7c6341de4625?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1c500c59-9e23-4608-98fb-7c6341de4625?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1c500c59-9e23-4608-98fb-7c6341de4625?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1c500c59-9e23-4608-98fb-7c6341de4625) </details> <sub> ๐ฌ Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:329d0444-31c1-47ce-b47a-5245de5b8a28 --> [](329d0444-31c1-47ce-b47a-5245de5b8a28) ########## .devcontainer/start-superset.sh: ########## @@ -18,71 +18,32 @@ else echo "๐ Using current directory: $(pwd)" fi -# Wait for Docker to be available -echo "โณ Waiting for Docker to start..." -echo "[$(date)] Waiting for Docker..." >> "$LOG_FILE" -max_attempts=30 -attempt=0 -while ! docker info > /dev/null 2>&1; do - if [ $attempt -eq $max_attempts ]; then - echo "โ Docker failed to start after $max_attempts attempts" - echo "[$(date)] Docker failed to start after $max_attempts attempts" >> "$LOG_FILE" - echo "๐ Please restart the Codespace or run this script manually later" - exit 1 - fi - echo " Attempt $((attempt + 1))/$max_attempts..." - echo "[$(date)] Docker check attempt $((attempt + 1))/$max_attempts" >> "$LOG_FILE" - sleep 2 - attempt=$((attempt + 1)) -done -echo "โ Docker is ready!" -echo "[$(date)] Docker is ready" >> "$LOG_FILE" - -# Check if Superset containers are already running -if docker ps | grep -q "superset"; then - echo "โ Superset containers are already running!" - echo "" - echo "๐ To access Superset:" - echo " 1. Click the 'Ports' tab at the bottom of VS Code" - echo " 2. Find port 9001 and click the globe icon to open" - echo " 3. Wait 10-20 minutes for initial startup" - echo "" - echo "๐ Login credentials: admin/admin" - exit 0 +# Check if docker is running +if ! docker info > /dev/null 2>&1; then + echo "โณ Waiting for Docker to start..." + sleep 5 fi # Clean up any existing containers echo "๐งน Cleaning up existing containers..." -docker-compose -f docker-compose-light.yml down +docker-compose -f docker-compose-light.yml --profile mcp down Review Comment: ### Unconditional MCP profile cleanup <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Cleanup command always includes --profile mcp flag regardless of whether MCP is enabled. ###### Why this matters This could cause unnecessary overhead by attempting to stop MCP-related containers even when MCP is disabled, and may fail if the MCP profile doesn't exist in all environments. ###### Suggested change โ *Feature Preview* Make the cleanup command conditional based on ENABLE_MCP: ```bash if [ "$ENABLE_MCP" = "true" ]; then docker-compose -f docker-compose-light.yml --profile mcp down else docker-compose -f docker-compose-light.yml down fi ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5391b96-fbe5-442e-ba00-e4a76c6d78b5/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5391b96-fbe5-442e-ba00-e4a76c6d78b5?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5391b96-fbe5-442e-ba00-e4a76c6d78b5?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5391b96-fbe5-442e-ba00-e4a76c6d78b5?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5391b96-fbe5-442e-ba00-e4a76c6d78b5) </details> <sub> ๐ฌ Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:a4504091-8d1c-4b4f-9767-5bf42e027a41 --> [](a4504091-8d1c-4b4f-9767-5bf42e027a41) ########## .devcontainer/start-superset.sh: ########## @@ -18,71 +18,32 @@ echo "๐ Using current directory: $(pwd)" fi -# Wait for Docker to be available -echo "โณ Waiting for Docker to start..." -echo "[$(date)] Waiting for Docker..." >> "$LOG_FILE" -max_attempts=30 -attempt=0 -while ! docker info > /dev/null 2>&1; do - if [ $attempt -eq $max_attempts ]; then - echo "โ Docker failed to start after $max_attempts attempts" - echo "[$(date)] Docker failed to start after $max_attempts attempts" >> "$LOG_FILE" - echo "๐ Please restart the Codespace or run this script manually later" - exit 1 - fi - echo " Attempt $((attempt + 1))/$max_attempts..." - echo "[$(date)] Docker check attempt $((attempt + 1))/$max_attempts" >> "$LOG_FILE" - sleep 2 - attempt=$((attempt + 1)) -done -echo "โ Docker is ready!" -echo "[$(date)] Docker is ready" >> "$LOG_FILE" - -# Check if Superset containers are already running -if docker ps | grep -q "superset"; then - echo "โ Superset containers are already running!" - echo "" - echo "๐ To access Superset:" - echo " 1. Click the 'Ports' tab at the bottom of VS Code" - echo " 2. Find port 9001 and click the globe icon to open" - echo " 3. Wait 10-20 minutes for initial startup" - echo "" - echo "๐ Login credentials: admin/admin" - exit 0 +# Check if docker is running +if ! docker info > /dev/null 2>&1; then + echo "โณ Waiting for Docker to start..." + sleep 5 fi Review Comment: ### Insufficient Docker startup wait time <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The Docker availability check only waits 5 seconds once and doesn't retry, which may not be sufficient for Docker to become available in all environments. ###### Why this matters If Docker takes longer than 5 seconds to start, the script will proceed and likely fail when trying to run docker-compose commands, leading to confusing error messages for users. ###### Suggested change โ *Feature Preview* Implement a retry loop with a reasonable timeout: ```bash echo "โณ Waiting for Docker to start..." max_attempts=30 attempt=0 while ! docker info > /dev/null 2>&1; do if [ $attempt -eq $max_attempts ]; then echo "โ Docker failed to start after $max_attempts attempts" exit 1 fi echo " Attempt $((attempt + 1))/$max_attempts..." sleep 2 attempt=$((attempt + 1)) done echo "โ Docker is ready!" ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/63db28e2-29d8-42b0-8ffa-cb9495fbb04a/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/63db28e2-29d8-42b0-8ffa-cb9495fbb04a?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/63db28e2-29d8-42b0-8ffa-cb9495fbb04a?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/63db28e2-29d8-42b0-8ffa-cb9495fbb04a?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/63db28e2-29d8-42b0-8ffa-cb9495fbb04a) </details> <sub> ๐ฌ Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:6308bb16-77cf-4b57-86fe-7d63e78d22ef --> [](6308bb16-77cf-4b57-86fe-7d63e78d22ef) ########## superset/mcp_service/simple_proxy.py: ########## @@ -0,0 +1,74 @@ +#!/usr/bin/env python3 +""" +Simple MCP proxy server that connects to FastMCP server on localhost:5008 +""" + +import logging +import signal +import sys +import time +from typing import Any, Optional + +from fastmcp import FastMCP + +# Configure logging +logging.basicConfig( + level=logging.INFO, format="%(asctime)s - %(name)s - %(levelname)s - %(message)s" +) +logger = logging.getLogger(__name__) + +# Global proxy instance for cleanup +proxy: Optional[FastMCP] = None + + +def signal_handler(signum: int, frame: Any) -> None: + """Handle shutdown signals gracefully""" + logger.info("Received signal %s, shutting down gracefully...", signum) + if proxy: + try: + # Give the proxy a moment to clean up + time.sleep(0.1) + except Exception as e: + logger.warning("Error during proxy cleanup: %s", e) Review Comment: ### Incomplete proxy cleanup in signal handler <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The signal handler does not properly clean up the proxy instance, only sleeps for 0.1 seconds without calling any cleanup methods. ###### Why this matters Resources may not be properly released when the proxy is terminated, potentially leaving connections open or other cleanup tasks incomplete. ###### Suggested change โ *Feature Preview* Call the appropriate cleanup method on the proxy instance: ```python if proxy: try: # Properly close/cleanup the proxy if hasattr(proxy, 'close'): proxy.close() elif hasattr(proxy, 'shutdown'): proxy.shutdown() except Exception as e: logger.warning("Error during proxy cleanup: %s", e) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9ae5c3f3-61ec-425b-ac65-f8e783d489a9/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9ae5c3f3-61ec-425b-ac65-f8e783d489a9?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9ae5c3f3-61ec-425b-ac65-f8e783d489a9?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9ae5c3f3-61ec-425b-ac65-f8e783d489a9?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9ae5c3f3-61ec-425b-ac65-f8e783d489a9) </details> <sub> ๐ฌ Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:eb4e36ea-413d-4643-bfb0-69da6ddae11b --> [](eb4e36ea-413d-4643-bfb0-69da6ddae11b) ########## superset/mcp_service/simple_proxy.py: ########## @@ -0,0 +1,74 @@ +#!/usr/bin/env python3 +""" +Simple MCP proxy server that connects to FastMCP server on localhost:5008 +""" + +import logging +import signal +import sys +import time +from typing import Any, Optional + +from fastmcp import FastMCP + +# Configure logging +logging.basicConfig( + level=logging.INFO, format="%(asctime)s - %(name)s - %(levelname)s - %(message)s" +) +logger = logging.getLogger(__name__) + +# Global proxy instance for cleanup +proxy: Optional[FastMCP] = None + + +def signal_handler(signum: int, frame: Any) -> None: + """Handle shutdown signals gracefully""" + logger.info("Received signal %s, shutting down gracefully...", signum) + if proxy: + try: + # Give the proxy a moment to clean up + time.sleep(0.1) + except Exception as e: + logger.warning("Error during proxy cleanup: %s", e) + sys.exit(0) + + +def main() -> None: + """Main function to run the proxy""" + global proxy Review Comment: ### Global State Usage <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code uses a global variable for the proxy instance, which creates tight coupling and makes the code harder to test and maintain. ###### Why this matters Global state makes the code less modular, harder to test, and can lead to unexpected behavior in larger applications. It violates the principle of dependency injection and makes the code less flexible. ###### Suggested change โ *Feature Preview* Create a ProxyServer class to encapsulate the proxy functionality: ```python class ProxyServer: def __init__(self): self.proxy: Optional[FastMCP] = None def handle_signal(self, signum: int, frame: Any) -> None: # Signal handling logic def run(self) -> None: # Proxy setup and run logic ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb46033b-3b14-499e-b76a-82509ed62974/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb46033b-3b14-499e-b76a-82509ed62974?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb46033b-3b14-499e-b76a-82509ed62974?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb46033b-3b14-499e-b76a-82509ed62974?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb46033b-3b14-499e-b76a-82509ed62974) </details> <sub> ๐ฌ Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:38ed9bab-1da2-430c-b3dc-9d44232fe53e --> [](38ed9bab-1da2-430c-b3dc-9d44232fe53e) ########## .devcontainer/start-superset.sh: ########## @@ -18,71 +18,32 @@ echo "๐ Using current directory: $(pwd)" fi -# Wait for Docker to be available -echo "โณ Waiting for Docker to start..." -echo "[$(date)] Waiting for Docker..." >> "$LOG_FILE" -max_attempts=30 -attempt=0 -while ! docker info > /dev/null 2>&1; do - if [ $attempt -eq $max_attempts ]; then - echo "โ Docker failed to start after $max_attempts attempts" - echo "[$(date)] Docker failed to start after $max_attempts attempts" >> "$LOG_FILE" - echo "๐ Please restart the Codespace or run this script manually later" - exit 1 - fi - echo " Attempt $((attempt + 1))/$max_attempts..." - echo "[$(date)] Docker check attempt $((attempt + 1))/$max_attempts" >> "$LOG_FILE" - sleep 2 - attempt=$((attempt + 1)) -done -echo "โ Docker is ready!" -echo "[$(date)] Docker is ready" >> "$LOG_FILE" - -# Check if Superset containers are already running -if docker ps | grep -q "superset"; then - echo "โ Superset containers are already running!" - echo "" - echo "๐ To access Superset:" - echo " 1. Click the 'Ports' tab at the bottom of VS Code" - echo " 2. Find port 9001 and click the globe icon to open" - echo " 3. Wait 10-20 minutes for initial startup" - echo "" - echo "๐ Login credentials: admin/admin" - exit 0 +# Check if docker is running +if ! docker info > /dev/null 2>&1; then + echo "โณ Waiting for Docker to start..." + sleep 5 fi # Clean up any existing containers echo "๐งน Cleaning up existing containers..." -docker-compose -f docker-compose-light.yml down +docker-compose -f docker-compose-light.yml --profile mcp down Review Comment: ### Unconditional MCP profile usage in cleanup <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The cleanup command always uses the '--profile mcp' flag regardless of whether MCP is enabled, which may cause issues if the MCP profile doesn't exist or isn't properly configured. ###### Why this matters This could lead to docker-compose errors during cleanup, potentially preventing the script from starting fresh containers or leaving orphaned containers running. ###### Suggested change โ *Feature Preview* Make the cleanup command conditional based on MCP enablement: ```bash echo "๐งน Cleaning up existing containers..." if [ "$ENABLE_MCP" = "true" ]; then docker-compose -f docker-compose-light.yml --profile mcp down else docker-compose -f docker-compose-light.yml down fi ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b33b210-8804-4d84-ad81-5ab8f1ceda8f/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b33b210-8804-4d84-ad81-5ab8f1ceda8f?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b33b210-8804-4d84-ad81-5ab8f1ceda8f?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b33b210-8804-4d84-ad81-5ab8f1ceda8f?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b33b210-8804-4d84-ad81-5ab8f1ceda8f) </details> <sub> ๐ฌ Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:02996178-e391-45cb-9c1f-7df41fa7d10a --> [](02996178-e391-45cb-9c1f-7df41fa7d10a) ########## superset/mcp_service/simple_proxy.py: ########## @@ -0,0 +1,74 @@ +#!/usr/bin/env python3 +""" +Simple MCP proxy server that connects to FastMCP server on localhost:5008 +""" + +import logging +import signal +import sys +import time +from typing import Any, Optional + +from fastmcp import FastMCP Review Comment: ### Duplicate Module Import <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? FastMCP is imported twice - once at the module level and again inside the main function. This violates the DRY principle and creates confusion about import scope. ###### Why this matters Duplicate imports make the code harder to maintain and can lead to confusion about which import is actually being used. It also violates Python's idiom of keeping imports at the module level. ###### Suggested change โ *Feature Preview* Remove the duplicate import from the main function since it's already imported at the module level: ```python from fastmcp import FastMCP # Keep only the module-level import def main() -> None: global proxy try: # Set up signal handlers... ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5efc8428-e1af-42a1-bf0a-82cb57bf311e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5efc8428-e1af-42a1-bf0a-82cb57bf311e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5efc8428-e1af-42a1-bf0a-82cb57bf311e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5efc8428-e1af-42a1-bf0a-82cb57bf311e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5efc8428-e1af-42a1-bf0a-82cb57bf311e) </details> <sub> ๐ฌ Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:5c3763cb-3f25-4b92-b167-cda78b1f9319 --> [](5c3763cb-3f25-4b92-b167-cda78b1f9319) ########## .devcontainer/setup-dev.sh: ########## @@ -3,76 +3,30 @@ echo "๐ง Setting up Superset development environment..." -# System dependencies and uv are now pre-installed in the Docker image -# This speeds up Codespace creation significantly! - -# Create virtual environment using uv -echo "๐ Creating Python virtual environment..." -if ! uv venv; then - echo "โ Failed to create virtual environment" - exit 1 -fi - -# Install Python dependencies -echo "๐ฆ Installing Python dependencies..." -if ! uv pip install -r requirements/development.txt; then - echo "โ Failed to install Python dependencies" - echo "๐ก You may need to run this manually after the Codespace starts" - exit 1 -fi - -# Install pre-commit hooks -echo "๐ช Installing pre-commit hooks..." -if source .venv/bin/activate && pre-commit install; then - echo "โ Pre-commit hooks installed" -else - echo "โ ๏ธ Pre-commit hooks installation failed (non-critical)" -fi +# The universal image has most tools, just need Superset-specific libs +echo "๐ฆ Installing Superset-specific dependencies..." +sudo apt-get update +sudo apt-get install -y \ + libsasl2-dev \ + libldap2-dev \ + libpq-dev \ + tmux \ + gh + +# Install uv for fast Python package management +echo "๐ฆ Installing uv..." +curl -LsSf https://astral.sh/uv/install.sh | sh Review Comment: ### Uncached external tool installation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Installing uv via curl pipe to shell without caching or version pinning creates unnecessary network overhead on repeated runs. ###### Why this matters Each container rebuild or script re-execution downloads and installs uv again, wasting bandwidth and time when the same version could be reused. ###### Suggested change โ *Feature Preview* Check if uv is already installed before downloading: `if ! command -v uv >/dev/null 2>&1; then curl -LsSf https://astral.sh/uv/install.sh | sh; fi` or consider using a specific version and caching the binary. ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d2ff76f-a679-4c94-abd9-9bf7f52765d5/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d2ff76f-a679-4c94-abd9-9bf7f52765d5?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d2ff76f-a679-4c94-abd9-9bf7f52765d5?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d2ff76f-a679-4c94-abd9-9bf7f52765d5?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d2ff76f-a679-4c94-abd9-9bf7f52765d5) </details> <sub> ๐ฌ Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:3ac8d463-04c4-4434-9f57-d95fa3a99c0a --> [](3ac8d463-04c4-4434-9f57-d95fa3a99c0a) -- 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]
