From 43ca00d3d5f2243d35ccf99e6defb698e4f995f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9=20Delion?= Date: Sat, 1 Nov 2025 15:35:38 +0100 Subject: [PATCH] fix(server): Fix missing FastMCP v2 migration, and add more non-regression tests --- DEVELOPMENT.md | 24 ++- .../src/secops_mcp_server/server.py | 2 +- tests/test_server_profiles.py | 179 ++++++++++++++++++ 3 files changed, 197 insertions(+), 8 deletions(-) create mode 100644 tests/test_server_profiles.py diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 5f2d3e9..b08ae44 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -22,18 +22,22 @@ This document provides instructions for developers who want to contribute to the ### Pre-commit Hooks -This project uses [pre-commit](https://pre-commit.com/) to ensure code quality and security standards. The hooks are configured in `.pre-commit-config.yaml` and include: +This project uses [pre-commit](https://pre-commit.com/) to ensure code quality and security standards. The hooks are +configured in `.pre-commit-config.yaml` and include: **On every commit:** + - **Ruff** - Automatically lints and formats Python code - **Commitizen** - Validates commit message format - **GitGuardian ggshield** - Scans for secrets in staged files **On every push:** + - **Commitizen-branch** - Validates branch naming conventions - **GitGuardian ggshield-push** - Scans all commits being pushed for secrets -The hooks will automatically run before commits/pushes and will block the operation if any issues are found. You can also run the hooks manually: +The hooks will automatically run before commits/pushes and will block the operation if any issues are found. You can +also run the hooks manually: ```bash # Run all hooks on all files @@ -79,6 +83,7 @@ To add a new tool to the MCP server: from fastmcp import Request, Response, Tool from typing import Dict, Any + class ExampleTool(Tool): """Example tool implementation.""" @@ -114,6 +119,7 @@ class ExampleTool(Tool): data={"result": result} ) + # List of tools to be exported tools = [ExampleTool()] ``` @@ -126,7 +132,7 @@ from example.tools import tools as example_tools # Register the tools for tool in example_tools: - mcp.add_tool(tool) + mcp.tool(tool) ``` ## Testing @@ -153,7 +159,8 @@ Create test files in the `tests/` directory that match the pattern `test_*.py`. ## Code Style -This project uses `ruff` for linting and formatting. While pre-commit hooks will automatically run ruff on your staged files, you can also run it manually: +This project uses `ruff` for linting and formatting. While pre-commit hooks will automatically run ruff on your staged +files, you can also run it manually: ```bash # Check for linting issues @@ -166,13 +173,15 @@ ruff check --fix src tests ruff format src tests ``` -**Note:** Pre-commit hooks will automatically run ruff on your staged files when you commit, so you usually don't need to run it manually. +**Note:** Pre-commit hooks will automatically run ruff on your staged files when you commit, so you usually don't need +to run it manually. ## Cursor Rules This project includes Cursor IDE rules in the `.cursor/rules` directory that enforce coding standards: -1. **Don't use uvicorn or fastapi with MCP** - MCP has its own server implementation, external web servers are not needed +1. **Don't use uvicorn or fastapi with MCP** - MCP has its own server implementation, external web servers are not + needed 2. **Use pyproject.toml with uv** - Modern Python projects should use pyproject.toml with uv for dependency management These rules help maintain consistent code quality and follow best practices for MCP development. @@ -198,7 +207,8 @@ When adding a new tool, please document it in the README.md following the same s 5. Push your changes (pre-push hooks will scan for secrets and validate branch names) 6. Submit a pull request with a clear description of your changes -**Note:** The pre-commit and pre-push hooks will automatically check your code quality, commit messages, and scan for secrets before allowing commits and pushes. +**Note:** The pre-commit and pre-push hooks will automatically check your code quality, commit messages, and scan for +secrets before allowing commits and pushes. ## Releasing diff --git a/packages/secops_mcp_server/src/secops_mcp_server/server.py b/packages/secops_mcp_server/src/secops_mcp_server/server.py index f3acc6d..0ce737f 100644 --- a/packages/secops_mcp_server/src/secops_mcp_server/server.py +++ b/packages/secops_mcp_server/src/secops_mcp_server/server.py @@ -197,7 +197,7 @@ async def get_current_token_info() -> dict[str, Any]: required_scopes=["incidents:write"], ) -mcp.add_tool( +mcp.tool( create_code_fix_request, description="Create code fix requests for multiple secret incidents with their locations. This will generate pull requests to automatically remediate the detected secrets.", required_scopes=["incidents:write"], diff --git a/tests/test_server_profiles.py b/tests/test_server_profiles.py new file mode 100644 index 0000000..ac92dbf --- /dev/null +++ b/tests/test_server_profiles.py @@ -0,0 +1,179 @@ +"""Test that server modules can be imported and initialized for both profiles. + +This test ensures that both developer and secops server modules can be +successfully imported and that their tool registration doesn't have syntax errors +(like using mcp.add_tool instead of mcp.tool). +""" + +import sys +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + + +@pytest.fixture +def mock_env_no_http(): + """Mock environment variables to prevent HTTP server from starting.""" + with patch.dict("os.environ", {"MCP_PORT": ""}, clear=False): + yield + + +@pytest.fixture +def mock_gitguardian_modules(): + """Mock GitGuardian API modules to avoid actual API calls during import.""" + with ( + patch("gg_api_core.mcp_server.get_client") as mock_get_client, + patch("gg_api_core.scopes.set_developer_scopes") as mock_set_dev_scopes, + patch("gg_api_core.scopes.set_secops_scopes") as mock_set_secops_scopes, + ): + # Mock client + mock_client = MagicMock() + mock_client.get_current_token_info = AsyncMock(return_value={"scopes": ["scan"]}) + mock_get_client.return_value = mock_client + + yield { + "get_client": mock_get_client, + "set_dev_scopes": mock_set_dev_scopes, + "set_secops_scopes": mock_set_secops_scopes, + } + + +def clean_module_imports(module_name: str): + """Remove a module and its submodules from sys.modules.""" + modules_to_remove = [key for key in sys.modules if key.startswith(module_name)] + for module in modules_to_remove: + del sys.modules[module] + + +class TestServerProfiles: + """Test that both server profiles can be initialized successfully.""" + + def test_developer_server_imports_successfully(self, mock_gitguardian_modules, mock_env_no_http): + """Test that the developer server module can be imported without errors. + + This test would catch issues like: + - Using mcp.add_tool instead of mcp.tool + - Syntax errors in tool registration + - Missing imports + """ + # Clean any previous imports + clean_module_imports("developer_mcp_server") + + try: + # Import the developer server module + import developer_mcp_server.server as dev_server + + # Verify the server was created + assert hasattr(dev_server, "mcp") + assert dev_server.mcp is not None + + # Verify it's a GitGuardianFastMCP instance + from gg_api_core.mcp_server import GitGuardianFastMCP + + assert isinstance(dev_server.mcp, GitGuardianFastMCP) + + # Verify the server has the expected name + assert dev_server.mcp.name == "GitGuardian Developer" + + except AttributeError as e: + if "add_tool" in str(e): + pytest.fail(f"Developer server is using mcp.add_tool instead of mcp.tool: {e}") + raise + except Exception as e: + pytest.fail(f"Failed to import developer server: {e}") + + def test_secops_server_imports_successfully(self, mock_gitguardian_modules, mock_env_no_http): + """Test that the secops server module can be imported without errors. + + This test would catch issues like: + - Using mcp.add_tool instead of mcp.tool + - Syntax errors in tool registration + - Missing imports + """ + # Clean any previous imports + clean_module_imports("secops_mcp_server") + + try: + # Import the secops server module + import secops_mcp_server.server as secops_server + + # Verify the server was created + assert hasattr(secops_server, "mcp") + assert secops_server.mcp is not None + + # Verify it's a GitGuardianFastMCP instance + from gg_api_core.mcp_server import GitGuardianFastMCP + + assert isinstance(secops_server.mcp, GitGuardianFastMCP) + + # Verify the server has the expected name + assert secops_server.mcp.name == "GitGuardian SecOps" + + except AttributeError as e: + if "add_tool" in str(e): + pytest.fail(f"SecOps server is using mcp.add_tool instead of mcp.tool: {e}") + raise + except Exception as e: + pytest.fail(f"Failed to import secops server: {e}") + + @pytest.mark.asyncio + async def test_developer_server_tools_registered(self, mock_gitguardian_modules, mock_env_no_http): + """Test that developer server has tools registered properly.""" + # Clean any previous imports + clean_module_imports("developer_mcp_server") + + import developer_mcp_server.server as dev_server + + # Mock the _fetch_token_scopes to avoid actual API calls + dev_server.mcp._fetch_token_scopes = AsyncMock() + dev_server.mcp._token_scopes = {"scan", "incidents:read"} + + # List tools - this would fail if any tool was registered incorrectly + tools = await dev_server.mcp.list_tools() + + # Verify we have some tools registered + assert len(tools) > 0, "Developer server should have tools registered" + + @pytest.mark.asyncio + async def test_secops_server_tools_registered(self, mock_gitguardian_modules, mock_env_no_http): + """Test that secops server has tools registered properly.""" + # Clean any previous imports + clean_module_imports("secops_mcp_server") + + import secops_mcp_server.server as secops_server + + # Mock the _fetch_token_scopes to avoid actual API calls + secops_server.mcp._fetch_token_scopes = AsyncMock() + secops_server.mcp._token_scopes = {"scan", "incidents:read", "incidents:write"} + + # List tools - this would fail if any tool was registered incorrectly + tools = await secops_server.mcp.list_tools() + + # Verify we have some tools registered + assert len(tools) > 0, "SecOps server should have tools registered" + + # Verify some expected secops-specific tools are present + tool_names = [tool.name for tool in tools] + # Check for a secops-specific tool that should be registered + assert "assign_incident" in tool_names, "SecOps server should have assign_incident tool" + assert "create_code_fix_request" in tool_names, "SecOps server should have create_code_fix_request tool" + + def test_both_servers_can_coexist(self, mock_gitguardian_modules, mock_env_no_http): + """Test that both server modules can be imported in the same test session. + + This ensures there are no naming conflicts or import issues. + """ + # Clean any previous imports + clean_module_imports("developer_mcp_server") + clean_module_imports("secops_mcp_server") + + try: + import developer_mcp_server.server as dev_server + import secops_mcp_server.server as secops_server + + # Both servers should be distinct instances + assert dev_server.mcp is not secops_server.mcp + assert dev_server.mcp.name != secops_server.mcp.name + + except Exception as e: + pytest.fail(f"Failed to import both servers: {e}")