feat: add MCP API integration tests and launcher backwards compatibility fixes

rokroskar · rokroskar · commit 578da49aefdc · 2026-06-08T17:25:35.000+02:00
- Integration tests against the real Sanic/DB/SpiceDB stack via SanicMCPDependencies
- Regression test: launcher_create without launcher_type must not fail (old API compat)
- launcher_type=None treated as interactive for backwards compatibility
- launcher_type optional in launcher_create, only sent when explicitly set
diff --git a/bases/renku_data_services/mcp_api/server.py b/bases/renku_data_services/mcp_api/server.py
@@ -531,10 +531,14 @@ async def launcher_create(
         name: Annotated[str, Field(description="Launcher name")],
         resource_class_id: Annotated[int, Field(description="Resource class ID (from resource_classes())")],
         environment: Annotated[dict[str, Any], Field(description="Environment definition dict")],
-        launcher_type: Annotated[str, Field(description="'interactive' for sessions, 'non_interactive' for jobs")] = "interactive",
+        launcher_type: Annotated[
+            str | None, Field(description="Set to 'non_interactive' for job launchers. Leave unset for interactive sessions (default).")
+        ] = None,
         description: Annotated[str, Field(description="Optional description")] = "",
     ) -> dict[str, Any]:
         """Create a session launcher. Always call resource_classes(cpu=..., memory=...) first.
+        Do NOT set launcher_type unless creating a non_interactive job launcher — leave it unset
+        for interactive sessions. Sending launcher_type='interactive' will fail on older deployments.
 
         Three ways to specify the environment:
 
@@ -558,8 +562,9 @@ async def launcher_create(
             "name": name,
             "resource_class_id": resource_class_id,
             "environment": environment,
-            "launcher_type": launcher_type,
         }
+        if launcher_type is not None:
+            body["launcher_type"] = launcher_type
         if description:
             body["description"] = description
         return _launcher_summary(await _api(ctx, "POST", "/session_launchers", body))
@@ -641,7 +646,8 @@ async def session_launch(
         After calling this, use session_wait(session_id) to wait for 'running' state —
         do not sleep or poll manually."""
         launcher = await _api(ctx, "GET", f"/session_launchers/{launcher_id}")
-        if launcher.get("launcher_type") != "interactive":
+        # None means the API predates launcher_type — treat as interactive (the historical default).
+        if launcher.get("launcher_type") not in ("interactive", None):
             raise RuntimeError(
                 f"Launcher {launcher_id!r} has launcher_type={launcher.get('launcher_type')!r}. "
                 "Use job_run for non_interactive launchers."
@@ -770,9 +776,10 @@ async def job_run(
         a new one — always verify _created=true in the response. If _created=false, delete
         the returned session and retry."""
         launcher = await _api(ctx, "GET", f"/session_launchers/{launcher_id}")
+        # None means the API predates launcher_type — treat as interactive, so block job_run.
         if launcher.get("launcher_type") != "non_interactive":
             raise RuntimeError(
-                f"Launcher {launcher_id!r} has launcher_type={launcher.get('launcher_type')!r}. "
+                f"Launcher {launcher_id!r} has launcher_type={launcher.get('launcher_type') or 'interactive (default)'}. "
                 "Use session_launch for interactive launchers."
             )
         body: dict[str, Any] = {"launcher_id": launcher_id}
diff --git a/test/bases/renku_data_services/data_api/test_mcp_api_integration.py b/test/bases/renku_data_services/data_api/test_mcp_api_integration.py
@@ -0,0 +1,228 @@
+"""Integration tests for the MCP server against the real data service API.
+
+These tests verify that the API calls the MCP tools make are compatible with the
+current API definition in this repo. They use the same Sanic test infrastructure as
+the other data_api tests — a real DB, real SpiceDB, and a dummy authenticator.
+
+The MCP tools are exercised via the full MCP protocol (in-process), routing API calls
+through the Sanic test client instead of httpx.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+import pytest
+from sanic_testing.testing import SanicASGITestClient
+
+from renku_data_services.mcp_api.dependencies import MCPDependencies
+from renku_data_services.mcp_api.server import _admin_cache
+from test.bases.renku_data_services.mcp_api.conftest import (
+    mcp_session,
+    tool_result_dict,
+    tool_result_list,
+)
+
+
+class SanicMCPDependencies(MCPDependencies):
+    """MCPDependencies that routes api() calls through the Sanic test client."""
+
+    def __init__(self, sanic_client: SanicASGITestClient) -> None:
+        super().__init__(base_url="http://localhost")
+        self._client = sanic_client
+
+    async def api(
+        self,
+        method: str,
+        path: str,
+        token: str,
+        body: Any = None,
+        *,
+        query: dict[str, Any] | None = None,
+        extra_headers: dict[str, str] | None = None,
+        return_headers: bool = False,
+    ) -> Any:
+        headers: dict[str, str] = {"Authorization": f"Bearer {token}"}
+        if extra_headers:
+            headers.update(extra_headers)
+
+        _, response = await self._client.request(
+            method,
+            f"/api/data{path}",
+            headers=headers,
+            json=body,
+            params=query,
+        )
+
+        if response.status >= 400:
+            raise RuntimeError(f"HTTP {response.status}: {response.text}")
+
+        result = response.json if response.content_type and "json" in response.content_type else None
+        if return_headers:
+            return result, dict(response.headers)
+        return result
+
+
+@pytest.fixture(autouse=True)
+def clear_admin_cache_integration():
+    _admin_cache.clear()
+    yield
+    _admin_cache.clear()
+
+
+@pytest.fixture
+def mcp_deps(sanic_client: SanicASGITestClient) -> SanicMCPDependencies:
+    return SanicMCPDependencies(sanic_client)
+
+
+# ---------------------------------------------------------------------------
+# Platform / auth
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_auth_status(mcp_deps: SanicMCPDependencies, regular_user_access_token: str) -> None:
+    async with mcp_session(mcp_deps, token=regular_user_access_token) as (session, _):
+        result = await session.call_tool("auth_status", {})
+        data = tool_result_dict(result)
+        assert data["authenticated"] is True
+        assert data["is_admin"] is False
+
+
+@pytest.mark.asyncio
+async def test_resource_classes(mcp_deps: SanicMCPDependencies, regular_user_access_token: str) -> None:
+    async with mcp_session(mcp_deps, token=regular_user_access_token) as (session, _):
+        result = await session.call_tool("resource_classes", {})
+        classes = tool_result_list(result)
+        # Resource pools may not be seeded in all test environments
+        assert isinstance(classes, list)
+        if classes:
+            assert all("id" in c and "cpu" in c for c in classes)
+
+
+@pytest.mark.asyncio
+async def test_namespaces(mcp_deps: SanicMCPDependencies, regular_user_access_token: str) -> None:
+    async with mcp_session(mcp_deps, token=regular_user_access_token) as (session, _):
+        result = await session.call_tool("namespaces", {})
+        ns_list = tool_result_list(result)
+        assert len(ns_list) > 0
+
+
+# ---------------------------------------------------------------------------
+# Projects
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_project_list(mcp_deps: SanicMCPDependencies, regular_user_access_token: str) -> None:
+    async with mcp_session(mcp_deps, token=regular_user_access_token) as (session, _):
+        result = await session.call_tool("project_list", {})
+        assert result.isError is not True
+
+
+@pytest.mark.asyncio
+async def test_project_create_and_delete(
+    mcp_deps: SanicMCPDependencies, regular_user_access_token: str, regular_user: Any
+) -> None:
+    namespace = regular_user.namespace.path.serialize()
+    async with mcp_session(mcp_deps, token=regular_user_access_token) as (session, _):
+        created = await session.call_tool(
+            "project_create",
+            {"name": "mcp-integration-test", "namespace": namespace, "visibility": "private"},
+        )
+        assert created.isError is not True
+        project = tool_result_dict(created)
+        project_id = project["id"]
+
+        deleted = await session.call_tool("project_delete", {"project": project_id})
+        assert deleted.isError is not True
+
+
+# ---------------------------------------------------------------------------
+# Launcher create — API compatibility regression tests
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_launcher_create_without_launcher_type(
+    mcp_deps: SanicMCPDependencies,
+    regular_user_access_token: str,
+    regular_user: Any,
+    create_session_environment: Any,
+    create_resource_pool: Any,
+) -> None:
+    """Launcher creation without launcher_type must not return 422.
+
+    Regression test: launcher_type was added to POST /session_launchers in a recent
+    API change. Sending launcher_type='interactive' fails on older deployments because
+    their schema has extra='forbid'. The MCP server must omit launcher_type when not
+    explicitly set.
+    """
+    env = await create_session_environment("mcp-test-env")
+    pool = await create_resource_pool(admin=True)
+    resource_class_id = pool["classes"][0]["id"]
+
+    namespace = regular_user.namespace.path.serialize()
+    async with mcp_session(mcp_deps, token=regular_user_access_token) as (session, _):
+        project_result = await session.call_tool(
+            "project_create",
+            {"name": "mcp-launcher-test", "namespace": namespace, "visibility": "private"},
+        )
+        project_id = tool_result_dict(project_result)["id"]
+        try:
+            # Do NOT pass launcher_type — this is the key assertion
+            launcher_result = await session.call_tool(
+                "launcher_create",
+                {
+                    "project_id": project_id,
+                    "name": "test-launcher",
+                    "resource_class_id": resource_class_id,
+                    "environment": {"id": env["id"]},
+                },
+            )
+            assert launcher_result.isError is not True, (
+                f"launcher_create failed without launcher_type: {launcher_result.content[0].text}"
+            )
+            launcher_id = tool_result_dict(launcher_result)["id"]
+            await session.call_tool("launcher_delete", {"launcher_id": launcher_id})
+        finally:
+            await session.call_tool("project_delete", {"project": project_id})
+
+
+@pytest.mark.asyncio
+async def test_launcher_create_non_interactive(
+    mcp_deps: SanicMCPDependencies,
+    regular_user_access_token: str,
+    regular_user: Any,
+    create_session_environment: Any,
+    create_resource_pool: Any,
+) -> None:
+    """Launcher creation with launcher_type='non_interactive' must succeed on current API."""
+    env = await create_session_environment("mcp-job-env")
+    pool = await create_resource_pool(admin=True)
+    resource_class_id = pool["classes"][0]["id"]
+
+    namespace = regular_user.namespace.path.serialize()
+    async with mcp_session(mcp_deps, token=regular_user_access_token) as (session, _):
+        project_result = await session.call_tool(
+            "project_create",
+            {"name": "mcp-job-test", "namespace": namespace, "visibility": "private"},
+        )
+        project_id = tool_result_dict(project_result)["id"]
+        try:
+            launcher_result = await session.call_tool(
+                "launcher_create",
+                {
+                    "project_id": project_id,
+                    "name": "test-job-launcher",
+                    "resource_class_id": resource_class_id,
+                    "environment": {"id": env["id"]},
+                    "launcher_type": "non_interactive",
+                },
+            )
+            assert launcher_result.isError is not True
+            launcher = tool_result_dict(launcher_result)
+            assert launcher.get("launcher_type") == "non_interactive"
+            await session.call_tool("launcher_delete", {"launcher_id": launcher["id"]})
+        finally:
+            await session.call_tool("project_delete", {"project": project_id})
diff --git a/test/bases/renku_data_services/mcp_api/conftest.py b/test/bases/renku_data_services/mcp_api/conftest.py
@@ -26,10 +26,10 @@ def mock_deps() -> MCPDependencies:
 
 
 @contextlib.asynccontextmanager
-async def mcp_session(deps: MCPDependencies):
+async def mcp_session(deps: MCPDependencies, token: str = "test-token"):
     """Async context manager that runs the MCP server in-process.
     Must be used within a single asyncio task to keep anyio cancel scopes happy."""
-    set_current_token("test-token")
+    set_current_token(token)
     server = create_server(deps)
 
     async with create_client_server_memory_streams() as (client_streams, server_streams):
diff --git a/test/bases/renku_data_services/mcp_api/test_mcp_server.py b/test/bases/renku_data_services/mcp_api/test_mcp_server.py
@@ -186,6 +186,7 @@ async def test_api_returns_headers_when_requested(httpx_mock):
 @pytest.mark.asyncio
 async def test_admin_blocks_tool_calls(mock_deps):
     """All tools except auth_status are blocked for admin users."""
+
     async def fake_api(method: str, path: str, token: str, *args: Any, **kwargs: Any) -> Any:
         if path == "/user":
             return {"is_admin": True, "id": "admin"}
@@ -202,6 +203,7 @@ async def fake_api(method: str, path: str, token: str, *args: Any, **kwargs: Any
 @pytest.mark.asyncio
 async def test_non_admin_allowed(mock_deps):
     """Non-admin users can call tools normally."""
+
     async def fake_api(method: str, path: str, token: str, *args: Any, **kwargs: Any) -> Any:
         if path == "/user":
             return {"is_admin": False, "id": "user1"}
@@ -217,6 +219,7 @@ async def fake_api(method: str, path: str, token: str, *args: Any, **kwargs: Any
 @pytest.mark.asyncio
 async def test_admin_check_cached(mock_deps):
     """/user is only called once per token, even across multiple tool calls."""
+
     async def fake_api(method: str, path: str, token: str, *args: Any, **kwargs: Any) -> Any:
         if path == "/user":
             return {"is_admin": False, "id": "user1"}
@@ -261,10 +264,20 @@ async def test_list_tools_smoke(mock_deps):
         result = await session.list_tools()
         names = {t.name for t in result.tools}
         for expected in (
-            "auth_status", "project_list", "project_create", "project_update",
-            "connector_create", "launcher_create", "launcher_delete",
-            "session_launch", "session_wait", "job_run", "job_wait", "build_wait",
-            "global_environments", "renku_group_members",
+            "auth_status",
+            "project_list",
+            "project_create",
+            "project_update",
+            "connector_create",
+            "launcher_create",
+            "launcher_delete",
+            "session_launch",
+            "session_wait",
+            "job_run",
+            "job_wait",
+            "build_wait",
+            "global_environments",
+            "renku_group_members",
         ):
             assert expected in names, f"Missing tool: {expected}"
 
@@ -273,7 +286,7 @@ async def test_list_tools_smoke(mock_deps):
 async def test_session_list_filters_stale(mock_deps):
     mock_deps.api.return_value = [
         make_session("running"),
-        make_session("hibernated", will_delete_at=iso_ago(60)),    # stale
+        make_session("hibernated", will_delete_at=iso_ago(60)),  # stale
         make_session("running", will_delete_at=iso_future(3600)),  # not stale
     ]
 
@@ -406,9 +419,7 @@ async def fake_api(method: str, path: str, token: str, body: Any = None, **kwarg
     mock_deps.api.side_effect = fake_api
 
     async with mcp_session(mock_deps) as (session, deps):
-        await session.call_tool(
-            "project_repo_add", {"project": "proj-1", "repository_url": "https://github.com/x/y"}
-        )
+        await session.call_tool("project_repo_add", {"project": "proj-1", "repository_url": "https://github.com/x/y"})
 
     patch_calls = [c for c in deps.api.call_args_list if c.args[0] == "PATCH"]
     assert len(patch_calls) == 1
@@ -418,6 +429,7 @@ async def fake_api(method: str, path: str, token: str, body: Any = None, **kwarg
 @pytest.mark.asyncio
 async def test_project_repo_add_raises_without_etag(mock_deps):
     """project_repo_add raises when the project has no ETag."""
+
     async def fake_api(method: str, path: str, token: str, *args: Any, **kwargs: Any) -> Any:
         if path == "/user":
             return {"is_admin": False}
