codeant-ai-for-open-source[bot] commented on PR #37023: URL: https://github.com/apache/superset/pull/37023#issuecomment-3730787994
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/37023/files#diff-9ebe980fc09c4554a65355be83dcd8cadac049bc64134a09aaab14831f0e6d51R121-R123'><strong>Behavior Change</strong></a><br>Passing `stateless_http=True` at `mcp_instance.run(...)` can change request/session semantics compared to the previous placement. Middleware or auth providers that rely on stateful HTTP (cookies, session-backed auth) may break or behave differently. Verify compatibility of all auth/middleware with stateless mode and add targeted tests.<br> - [ ] <a href='https://github.com/apache/superset/pull/37023/files#diff-e408acc5e688745068830ee2c9a6c8d55bdbaff37663e2203e303ca91baa958fR125-R126'><strong>Environment parsing</strong></a><br>`port = int(os.environ.get("FASTMCP_PORT", "5008"))` will raise a ValueError if `FASTMCP_PORT` is set to a non-integer string, causing the process to crash on startup. This block lacks robust validation and a safe fallback path.<br> - [ ] <a href='https://github.com/apache/superset/pull/37023/files#diff-e408acc5e688745068830ee2c9a6c8d55bdbaff37663e2203e303ca91baa958fR126-R126'><strong>Compatibility Risk</strong></a><br>The new call adds the `stateless_http=True` keyword to `mcp.run(...)`. If deployed with an older FastMCP/mcp implementation that does not accept this keyword, the call will raise a TypeError at startup and the service will fail to start. Consider a compatibility fallback or runtime feature detection.<br> - [ ] <a href='https://github.com/apache/superset/pull/37023/files#diff-0b1268c08fa98c96adafc6abfc3d24dcf863c08e1959c49410c48bc78e74ae9dR288-R288'><strong>Dependency injection failure fallout</strong></a><br>The call to `initialize_core_mcp_dependencies()` runs at import time and will re-raise non-ImportError exceptions from the injection routine. That can cause the entire module import to fail and block application startup. Evaluate making the call resilient or failing more gracefully so a non-fatal problem in injection doesn't break imports.<br> - [ ] <a href='https://github.com/apache/superset/pull/37023/files#diff-9ebe980fc09c4554a65355be83dcd8cadac049bc64134a09aaab14831f0e6d51R111-R114'><strong>Run Guard</strong></a><br>There is no defensive check that `mcp_instance` is non-None and actually exposes a `run` method before calling it. If `init_fastmcp_server` failed or returned an unexpected object, this will raise. Add validation and ensure the environment marker is cleaned up on precondition failures.<br> </td></tr> </table> -- 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]
