-
Notifications
You must be signed in to change notification settings - Fork 759
improvement: mcp server ui, auth, and rules #6659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| # Copyright 2025 Marimo. All rights reserved. | ||
| from __future__ import annotations | ||
|
|
||
| from dataclasses import dataclass | ||
| from typing import Optional | ||
|
|
||
| import marimo._utils.requests as requests | ||
| from marimo import _loggers | ||
| from marimo._ai._tools.base import ToolBase | ||
| from marimo._ai._tools.types import EmptyArgs, SuccessResult | ||
|
|
||
| LOGGER = _loggers.marimo_logger() | ||
|
|
||
| # We load the rules remotely, so we can update these without requiring a new release. | ||
| # If requested, we can bundle this into the library instead. | ||
| MARIMO_RULES_URL = "https://docs.marimo.io/CLAUDE.md" | ||
|
|
||
|
|
||
| @dataclass | ||
| class GetMarimoRulesOutput(SuccessResult): | ||
| rules_content: Optional[str] = None | ||
| source_url: str = MARIMO_RULES_URL | ||
|
|
||
|
|
||
| class GetMarimoRules(ToolBase[EmptyArgs, GetMarimoRulesOutput]): | ||
| """Get the official marimo rules and guidelines for AI assistants. | ||
| Returns: | ||
| The content of the rules file. | ||
| """ | ||
|
|
||
| def handle(self, args: EmptyArgs) -> GetMarimoRulesOutput: | ||
| del args | ||
|
|
||
| try: | ||
| response = requests.get(MARIMO_RULES_URL, timeout=10) | ||
| response.raise_for_status() | ||
|
|
||
| return GetMarimoRulesOutput( | ||
| rules_content=response.text(), | ||
| source_url=MARIMO_RULES_URL, | ||
| next_steps=[ | ||
| "Follow the guidelines in the rules when working with marimo notebooks", | ||
| ], | ||
| ) | ||
|
|
||
| except Exception as e: | ||
| LOGGER.warning( | ||
| "Failed to fetch marimo rules from %s: %s", | ||
| MARIMO_RULES_URL, | ||
| str(e), | ||
| ) | ||
|
|
||
| return GetMarimoRulesOutput( | ||
| status="error", | ||
| message=f"Failed to fetch marimo rules: {str(e)}", | ||
| source_url=MARIMO_RULES_URL, | ||
| next_steps=[ | ||
| "Check internet connectivity", | ||
| "Verify the rules URL is accessible", | ||
| "Try again later if the service is temporarily unavailable", | ||
| ], | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,6 @@ | |
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| from mcp.server.fastmcp import FastMCP | ||
|
|
||
| from marimo._ai._tools.base import ToolContext | ||
| from marimo._ai._tools.tools_registry import SUPPORTED_BACKEND_AND_MCP_TOOLS | ||
| from marimo._loggers import marimo_logger | ||
|
|
@@ -18,11 +16,10 @@ | |
|
|
||
|
|
||
| if TYPE_CHECKING: | ||
| from mcp.server.streamable_http_manager import StreamableHTTPSessionManager | ||
| from starlette.applications import Starlette | ||
|
|
||
|
|
||
| def setup_mcp_server(app: "Starlette") -> "StreamableHTTPSessionManager": | ||
| def setup_mcp_server(app: "Starlette") -> None: | ||
| """Create and configure MCP server for marimo integration. | ||
|
|
||
| Args: | ||
|
|
@@ -33,17 +30,20 @@ def setup_mcp_server(app: "Starlette") -> "StreamableHTTPSessionManager": | |
| Returns: | ||
| StreamableHTTPSessionManager: MCP session manager | ||
| """ | ||
| from mcp.server.fastmcp import FastMCP | ||
| from starlette.middleware.base import BaseHTTPMiddleware | ||
| from starlette.responses import JSONResponse | ||
| from starlette.routing import Mount | ||
| from starlette.types import Receive, Scope, Send | ||
|
|
||
| mcp = FastMCP( | ||
| "marimo-mcp-server", | ||
| stateless_http=True, | ||
| log_level="WARNING", | ||
| # Change base path from /mcp to /server | ||
| streamable_http_path="/server", | ||
| ) | ||
|
|
||
| # Change base path from /mcp to /server | ||
| mcp.settings.streamable_http_path = "/server" | ||
|
|
||
| # Register all tools | ||
| context = ToolContext(app=app) | ||
| for tool in SUPPORTED_BACKEND_AND_MCP_TOOLS: | ||
|
|
@@ -53,7 +53,23 @@ def setup_mcp_server(app: "Starlette") -> "StreamableHTTPSessionManager": | |
| # Initialize streamable HTTP app | ||
| mcp_app = mcp.streamable_http_app() | ||
|
|
||
| # Middleware to require edit scope | ||
| class RequiresEditMiddleware(BaseHTTPMiddleware): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is actually just for MCP. we already support auth on other endpoints with |
||
| async def __call__( | ||
| self, scope: Scope, receive: Receive, send: Send | ||
| ) -> None: | ||
| auth = scope.get("auth") | ||
| if auth is None or "edit" not in auth.scopes: | ||
| response = JSONResponse( | ||
| {"detail": "Forbidden"}, | ||
| status_code=403, | ||
| ) | ||
| return await response(scope, receive, send) | ||
|
|
||
| return await self.app(scope, receive, send) | ||
|
|
||
| mcp_app.add_middleware(RequiresEditMiddleware) | ||
|
|
||
| # Add to the top of the routes to avoid conflicts with other routes | ||
| app.routes.insert(0, Mount("/mcp", mcp_app)) | ||
|
|
||
| return mcp.session_manager | ||
| app.state.mcp = mcp | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,14 +163,18 @@ def print_mcp_server(mcp_url: str, server_token: str | None) -> None: | |
| """Print MCP server configuration when MCP is enabled.""" | ||
| print_() | ||
| print_tabbed( | ||
| f"{_utf8('🔗')} {green('Experimental MCP Server Configuration', bold=True)}" | ||
| f"{_utf8('🔗')} {green('Experimental MCP server configuration', bold=True)}" | ||
| ) | ||
| print_tabbed( | ||
| f"{_utf8('➜')} {green('MCP Server URL')}: {_colorized_url(mcp_url)}" | ||
| f"{_utf8('➜')} {green('MCP server URL')}: {_colorized_url(mcp_url)}" | ||
| ) | ||
| # Add to Claude code | ||
| print_tabbed( | ||
| f"{_utf8('➜')} {green('Add to Claude Code')}: claude mcp add --transport http marimo {mcp_url}" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also include print_tabbed(
f"{_utf8('➜')} {green('Add to Cursor')}: Add the following to ~/.cursor/mcp.json:\n"
f""" {{
"mcpServers": {{
"marimo": {{
"url": "{mcp_url}"
}}
}}
}}"""
)There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that seems a bit verbose for the CLI. we could look into making the CLI more interface in the future (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that makes sense |
||
| ) | ||
| if server_token is not None: | ||
| print_tabbed( | ||
| f"{_utf8('➜')} {green('Add Header')}: Marimo-Server-Token: {muted(server_token)}" | ||
| f"{_utf8('➜')} {green('Add header')}: Marimo-Server-Token: {muted(server_token)}" | ||
| ) | ||
| print_() | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| # Copyright 2025 Marimo. All rights reserved. | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from unittest.mock import Mock, patch | ||
|
|
||
| import pytest | ||
|
|
||
| from marimo._ai._tools.base import ToolContext | ||
| from marimo._ai._tools.tools.rules import GetMarimoRules | ||
| from marimo._ai._tools.types import EmptyArgs | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def tool() -> GetMarimoRules: | ||
| """Create a GetMarimoRules tool instance.""" | ||
| return GetMarimoRules(ToolContext()) | ||
|
|
||
|
|
||
| def test_get_rules_success(tool: GetMarimoRules) -> None: | ||
| """Test successfully fetching marimo rules.""" | ||
| mock_response = Mock() | ||
| mock_response.text.return_value = "# Marimo Rules\n\nTest content" | ||
| mock_response.raise_for_status = Mock() | ||
|
|
||
| with patch("marimo._utils.requests.get", return_value=mock_response): | ||
| result = tool.handle(EmptyArgs()) | ||
|
|
||
| assert result.status == "success" | ||
| assert result.rules_content == "# Marimo Rules\n\nTest content" | ||
| assert result.source_url == "https://docs.marimo.io/CLAUDE.md" | ||
| assert len(result.next_steps) == 1 | ||
| assert "Follow the guidelines" in result.next_steps[0] | ||
| mock_response.raise_for_status.assert_called_once() | ||
|
|
||
|
|
||
| def test_get_rules_http_error(tool: GetMarimoRules) -> None: | ||
| """Test handling HTTP errors when fetching rules.""" | ||
| mock_response = Mock() | ||
| mock_response.raise_for_status.side_effect = Exception("404 Not Found") | ||
|
|
||
| with patch("marimo._utils.requests.get", return_value=mock_response): | ||
| result = tool.handle(EmptyArgs()) | ||
|
|
||
| assert result.status == "error" | ||
| assert result.rules_content is None | ||
| assert "Failed to fetch marimo rules" in result.message | ||
| assert "404 Not Found" in result.message | ||
| assert result.source_url == "https://docs.marimo.io/CLAUDE.md" | ||
| assert len(result.next_steps) == 3 | ||
| assert "Check internet connectivity" in result.next_steps[0] | ||
|
|
||
|
|
||
| def test_get_rules_network_error(tool: GetMarimoRules) -> None: | ||
| """Test handling network errors when fetching rules.""" | ||
| with patch( | ||
| "marimo._utils.requests.get", | ||
| side_effect=Exception("Connection refused"), | ||
| ): | ||
| result = tool.handle(EmptyArgs()) | ||
|
|
||
| assert result.status == "error" | ||
| assert result.rules_content is None | ||
| assert "Failed to fetch marimo rules" in result.message | ||
| assert "Connection refused" in result.message | ||
| assert len(result.next_steps) == 3 | ||
|
|
||
|
|
||
| def test_get_rules_timeout(tool: GetMarimoRules) -> None: | ||
| """Test handling timeout when fetching rules.""" | ||
| with patch( | ||
| "marimo._utils.requests.get", | ||
| side_effect=Exception("Request timeout"), | ||
| ): | ||
| result = tool.handle(EmptyArgs()) | ||
|
|
||
| assert result.status == "error" | ||
| assert result.rules_content is None | ||
| assert "Request timeout" in result.message |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really good! Maybe we should add this to the
next_steps=suggestion for some of the other tools?My suggestions would be:
GetNotebookErrors()if there are any notebook specific or marimo specific errorsGetLightweightCellMap()andGetActiveNotebooks()because these are entry points.GetDatabaseTables()because the rules contain some SQL-specific guidelinesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can do this in a followup. it hard to tell why an AI would ask for rules so im not sure if the next steps would veer it off track
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds good. I'll keep this incase we decide to update it later. We can see first if the Chat LLMs either don't pull rules initially or pull it early on in the conversation then forget it.