fix(mcp-service): ensure Flask app context in auth hook and resolve Pydantic warnings

[aminghadersohi]

SUMMARY

This PR fixes two critical bugs and enhances documentation in the MCP service:

1. Flask App Context Error - Fixes health_check tool failing in deployment with "Working outside of application context" error
2. Pydantic Warning - Resolves warning about schema field shadowing BaseModel attribute
3. Auth Documentation - Replaces TODO comments with production-ready deployment guidance

Context:
The health_check tool was the only MCP tool that doesn't access Flask functionality directly. In stateless HTTP mode, the @mcp_auth_hook decorator attempted to access current_app.config without ensuring Flask app context was active, causing failures in deployment environments while working fine locally.

CHANGES

1. Flask App Context Fix (superset/mcp_service/auth.py)

Problem:

• health_check tool failed in deployment with "Working outside of application context"
• Root cause: mcp_auth_hook called current_app.config without active Flask context
• Only affected tools that don't access Flask functionality in their implementation

Solution:

@functools.wraps(tool_func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
    from superset.mcp_service.flask_singleton import get_flask_app
    
    app = get_flask_app()
    
    # Explicitly push Flask app context for stateless_http mode
    with app.app_context():
        user = get_user_from_request()
        g.user = user
        # ... tool execution

Impact:

• Ensures all MCP tools have guaranteed Flask app context
• Critical for stateless_http=True mode in production
• Makes auth system more robust and predictable

2. Pydantic Warning Fix (superset/mcp_service/common/error_schemas.py)

Problem:

UserWarning: Field name "schema" in "DatasetContext" shadows an attribute in parent "BaseModel"

Solution:

class DatasetContext(BaseModel):
    model_config = {"populate_by_name": True}
    
    schema_name: str | None = Field(None, alias="schema", description="Schema name")

Impact:

• Eliminates Pydantic v2 warning
• Maintains backward compatibility (accepts both schema and schema_name)
• Follows Pydantic best practices

3. Enhanced Auth Documentation (superset/mcp_service/auth.py)

Changes:

• Removed all TODO comments suggesting "future PR" work
• Added comprehensive deployment guidance with concrete examples
• Clarified hook pattern as production-ready extension point
• Provided actionable code examples for:
  • JWT authentication integration
  • Custom permission checks
  • Audit logging implementation
  • Rate limiting middleware

Impact:

• Deployers have clear guidance for production customization
• Emphasizes MCP auth is a flexible hook system, not incomplete code
• Reduces confusion about auth implementation status

TESTING

✅ All 178 MCP service unit tests pass
✅ Pre-commit checks pass (mypy, ruff, pylint)
✅ No Pydantic warnings when importing MCP service
✅ Backward compatibility verified for DatasetContext usage

BEFORE/AFTER

Before:

• health_check tool fails in deployment with Flask context error
• Pydantic warning appears on every MCP service import
• TODO comments suggest auth system is incomplete

After:

• All MCP tools work reliably in stateless HTTP mode
• No Pydantic warnings
• Clear production deployment guidance

ADDITIONAL INFORMATION

• Has associated issue: N/A (bug fix)
• Required feature flags: None
• Changes UI: No
• Includes DB Migration: No
• Introduces new feature or API: No
• Removes existing feature or API: No

[bito-code-review]

Code Review Agent Run #838990

Actionable Suggestions - 0

Additional Suggestions - 3

• superset/mcp_service/auth.py - 3

  • Missing blank line after docstring section · Line 87-87
    Missing blank line after the "Raises" section in the docstring. Add a blank line after the last docstring section for proper formatting.
    Code suggestion

     @@ -87,2 +87,3 @@
    -    Raises:
    -        ValueError: If user cannot be authenticated or found
    +    Raises:
    +        ValueError: If user cannot be authenticated or found
    +

  • Missing trailing comma in function call · Line 214-214
    Trailing comma missing in the `logger.debug` function call. Add a trailing comma after `tool_func.__name__` for consistency.
    Code suggestion

     @@ -213,3 +213,3 @@
    -                logger.debug(
    -                    "MCP tool call: user=%s, tool=%s", user.username, tool_func.__name__
    -                )
    +                logger.debug(
    +                    "MCP tool call: user=%s, tool=%s", user.username, tool_func.__name__,
    +                )

  • Unnecessary variable assignment before return · Line 217-217
    Unnecessary assignment to `result` before `return` statement. Consider returning `tool_func(*args, **kwargs)` directly or move the assignment to an `else` block.
    Code suggestion

     @@ -217,5 +217,3 @@
    -                result = tool_func(*args, **kwargs)
    -
    -                # Deployers: Add audit logging or post-execution hooks here
    -                # Example: Log successful tool execution, track metrics,
    -                # or trigger downstream workflows
    -
    -                return result
    +                # Deployers: Add audit logging or post-execution hooks here
    +                # Example: Log successful tool execution, track metrics,
    +                # or trigger downstream workflows
    +
    +                return tool_func(*args, **kwargs)

Review Details

• Files reviewed - 2 · Commit Range: 79bf966..79bf966

  • superset/mcp_service/auth.py
  • superset/mcp_service/common/error_schemas.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[korbit-ai]

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Incomplete alias configuration for schema field ▹ view
	Unnecessary eager loading of user relationships ▹ view

Files scanned

​

File Path	Reviewed
superset/mcp_service/common/error_schemas.py	✅
superset/mcp_service/auth.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

[codecov]

Codecov Report

❌ Patch coverage is 0% with 51 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.35%. Comparing base (008c7c6) to head (de35377).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/mcp_service/auth.py	0.00%	47 Missing ⚠️
superset/mcp_service/common/error_schemas.py	0.00%	2 Missing ⚠️
superset/mcp_service/system/tool/health_check.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #36013       +/-   ##
===========================================
+ Coverage        0   68.35%   +68.35%     
===========================================
  Files           0      631      +631     
  Lines           0    45991    +45991     
  Branches        0     4983     +4983     
===========================================
+ Hits            0    31439    +31439     
- Misses          0    13306    +13306     
- Partials        0     1246     +1246     

Flag	Coverage Δ
hive	43.94% <0.00%> (?)
mysql	67.44% <0.00%> (?)
postgres	67.49% <0.00%> (?)
presto	47.48% <0.00%> (?)
python	68.32% <0.00%> (?)
sqlite	67.10% <0.00%> (?)
unit	100.00% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aminghadersohi]

I've had claude address the reviewer feedback with the latest commit:

Changes Made

1. Replaced _ Variable Names (@eschutho)

• Changed _ = user.roles to user_roles = user.roles # noqa: F841
• Changed _ = user.groups to user_groups = user.groups # noqa: F841
• This avoids confusion with the translation function _() commonly used in Superset
• Added noqa: F841 because these variables are intentionally unused (they trigger eager loading)

2. Fixed Pydantic Alias Configuration (@korbit-ai)

• Added serialization_alias="schema" to DatasetContext.schema_name field
• This ensures the field serializes correctly as "schema" in output, not "schema_name"
• The field can now accept input as either "schema" or "schema_name" (via alias)

3. Improved Exception Handling Comments (@eschutho)

• Added clarifying comments to explain why we log cleanup errors but re-raise the original exception
• Renamed e to cleanup_error for clarity
• The pattern preserves the original tool error while logging any session cleanup failures

About the Performance Concern (@korbit-ai)

The eager loading of user relationships (user.roles, user.groups) is intentional and necessary in this context:

• Why: SQLAlchemy sessions close after the tool execution, causing lazy-loading errors
• When: Only happens once per tool call, not repeatedly
• Cost: Minimal - two simple relationship queries per request
• Benefit: Prevents runtime errors when accessing user permissions later

This is a session lifecycle management pattern, not a performance issue. The relationships are needed by Superset's security layer.

[Antonio-RiveroMartnez]

LGTM