Unified mcp registration endpoints

asimurka · asimurka · commit 42bfa9d1adf4 · 2026-05-07T10:12:04.000+02:00
diff --git a/docs/openapi.json b/docs/openapi.json
@@ -1229,6 +1229,14 @@
                                                 "response": "Configuration is not loaded"
                                             }
                                         }
+                                    },
+                                    "mcp server registration": {
+                                        "value": {
+                                            "detail": {
+                                                "cause": "Could not register the MCP server with the remote service.",
+                                                "response": "Failed to register MCP server"
+                                            }
+                                        }
                                     }
                                 }
                             }
@@ -1397,30 +1405,18 @@
                                                 "response": "User does not have permission to access this endpoint"
                                             }
                                         }
-                                    }
-                                },
-                                "schema": {
-                                    "$ref": "#/components/schemas/ForbiddenResponse"
-                                }
-                            }
-                        }
-                    },
-                    "404": {
-                        "description": "Resource not found",
-                        "content": {
-                            "application/json": {
-                                "examples": {
-                                    "mcp server": {
+                                    },
+                                    "mcp server static": {
                                         "value": {
                                             "detail": {
-                                                "cause": "Mcp Server with ID test-mcp-server does not exist",
-                                                "response": "Mcp Server not found"
+                                                "cause": "MCP server 'my-mcp' is defined in configuration and cannot be removed via the API.",
+                                                "response": "Cannot delete statically configured MCP server"
                                             }
                                         }
                                     }
                                 },
                                 "schema": {
-                                    "$ref": "#/components/schemas/NotFoundResponse"
+                                    "$ref": "#/components/schemas/ForbiddenResponse"
                                 }
                             }
                         }
@@ -13221,6 +13217,13 @@
                             "response": "This instance does not permit overriding model/provider in the query request (missing permission: model_override). Please remove the model and provider fields from your request."
                         },
                         "label": "model override"
+                    },
+                    {
+                        "detail": {
+                            "cause": "MCP server 'my-mcp' is defined in configuration and cannot be removed via the API.",
+                            "response": "Cannot delete statically configured MCP server"
+                        },
+                        "label": "mcp server static"
                     }
                 ]
             },
@@ -13514,6 +13517,13 @@
                             "response": "Internal server error"
                         },
                         "label": "invalid cluster version"
+                    },
+                    {
+                        "detail": {
+                            "cause": "Could not register the MCP server with the remote service.",
+                            "response": "Failed to register MCP server"
+                        },
+                        "label": "mcp server registration"
                     }
                 ]
             },
diff --git a/src/app/endpoints/mcp_servers.py b/src/app/endpoints/mcp_servers.py
@@ -3,7 +3,8 @@
 from typing import Annotated, Any
 
 from fastapi import APIRouter, Depends, HTTPException, Request, status
-from llama_stack_client import APIConnectionError
+from llama_stack_api.common.errors import ToolGroupNotFoundError
+from llama_stack_client import APIConnectionError, APIStatusError
 
 from authentication import get_auth_dependency
 from authentication.interface import AuthTuple
@@ -17,7 +18,6 @@
     ConflictResponse,
     ForbiddenResponse,
     InternalServerErrorResponse,
-    NotFoundResponse,
     ServiceUnavailableResponse,
     UnauthorizedResponse,
 )
@@ -39,7 +39,9 @@
     401: UnauthorizedResponse.openapi_response(examples=UNAUTHORIZED_OPENAPI_EXAMPLES),
     403: ForbiddenResponse.openapi_response(examples=["endpoint"]),
     409: ConflictResponse.openapi_response(examples=["mcp server"]),
-    500: InternalServerErrorResponse.openapi_response(examples=["configuration"]),
+    500: InternalServerErrorResponse.openapi_response(
+        examples=["configuration", "mcp server registration"]
+    ),
     503: ServiceUnavailableResponse.openapi_response(
         examples=["llama stack", "kubernetes api"]
     ),
@@ -109,10 +111,7 @@ async def register_mcp_server_handler(
     except Exception as e:  # pylint: disable=broad-exception-caught
         configuration.remove_mcp_server(body.name)
         logger.error("Failed to register MCP toolgroup: %s", e)
-        error_response = InternalServerErrorResponse(
-            response="Failed to register MCP server",
-            cause=str(e),
-        )
+        error_response = InternalServerErrorResponse.mcp_server_registration_failed()
         raise HTTPException(**error_response.model_dump()) from e
 
     logger.info("Dynamically registered MCP server: %s at %s", body.name, body.url)
@@ -178,8 +177,7 @@ async def list_mcp_servers_handler(
 delete_responses: dict[int | str, dict[str, Any]] = {
     200: MCPServerDeleteResponse.openapi_response(),
     401: UnauthorizedResponse.openapi_response(examples=UNAUTHORIZED_OPENAPI_EXAMPLES),
-    403: ForbiddenResponse.openapi_response(examples=["endpoint"]),
-    404: NotFoundResponse.openapi_response(examples=["mcp server"]),
+    403: ForbiddenResponse.openapi_response(examples=["endpoint", "mcp server static"]),
     500: InternalServerErrorResponse.openapi_response(examples=["configuration"]),
     503: ServiceUnavailableResponse.openapi_response(
         examples=["llama stack", "kubernetes api"]
@@ -218,46 +216,30 @@ async def delete_mcp_server_handler(
     check_configuration_loaded(configuration)
 
     if not configuration.is_dynamic_mcp_server(name):
-        found = any(s.name == name for s in configuration.mcp_servers)
-        if found:
-            response = ForbiddenResponse(
-                response="Cannot delete statically configured MCP server",
-                cause=f"MCP server '{name}' was configured in lightspeed-stack.yaml "
-                "and cannot be removed via the API.",
-            )
-        else:
-            response = NotFoundResponse(resource="MCP server", resource_id=name)
-        raise HTTPException(**response.model_dump())
+        static_mcp_names = {s.name for s in configuration.mcp_servers}
+        if name in static_mcp_names:
+            response = ForbiddenResponse.mcp_server_static_config(name)
+            raise HTTPException(**response.model_dump())
+
+    try:
+        configuration.remove_mcp_server(name)
+        local_deleted = True
+    except ValueError as e:
+        logger.error("Failed to remove MCP server from configuration: %s", e)
+        local_deleted = False
 
     try:
         client = AsyncLlamaStackClientHolder().get_client()
         await client.toolgroups.unregister(  # pyright: ignore[reportDeprecated]
             toolgroup_id=name
         )
     except APIConnectionError as e:
-        logger.error("Failed to unregister MCP toolgroup from Llama Stack: %s", e)
+        logger.error("Failed to connect to Llama Stack: %s", e)
         svc_response = ServiceUnavailableResponse(
             backend_name="Llama Stack", cause=str(e)
         )
         raise HTTPException(**svc_response.model_dump()) from e
-    except Exception as e:  # pylint: disable=broad-exception-caught
-        logger.warning(
-            "Llama Stack toolgroup unregister failed for '%s', "
-            "proceeding with local removal: %s",
-            name,
-            e,
-        )
-
-    try:
-        configuration.remove_mcp_server(name)
-    except ValueError as e:
-        logger.error("Failed to remove MCP server from configuration: %s", e)
-        response = NotFoundResponse(resource="MCP server", resource_id=name)
-        raise HTTPException(**response.model_dump()) from e
+    except (APIStatusError, ToolGroupNotFoundError):
+        logger.warning("MCP server not found, treating as already deleted.")
 
-    logger.info("Dynamically unregistered MCP server: %s", name)
-
-    return MCPServerDeleteResponse(
-        name=name,
-        message=f"MCP server '{name}' unregistered successfully",
-    )
+    return MCPServerDeleteResponse(deleted=local_deleted, name=name)
diff --git a/src/models/api/responses/error/forbidden.py b/src/models/api/responses/error/forbidden.py
@@ -85,6 +85,16 @@ class ForbiddenResponse(AbstractErrorResponse):
                         ),
                     },
                 },
+                {
+                    "label": "mcp server static",
+                    "detail": {
+                        "response": "Cannot delete statically configured MCP server",
+                        "cause": (
+                            "MCP server 'my-mcp' is defined in configuration "
+                            "and cannot be removed via the API."
+                        ),
+                    },
+                },
             ]
         }
     }
@@ -156,6 +166,25 @@ def model_override(cls) -> Self:
                 "to override model/provider."
             ),
         )
+    
+    @classmethod
+    def mcp_server_static_config(cls, server_name: str) -> Self:
+        """Create a ForbiddenResponse indicating the client tried to delete a statically configured MCP server.
+
+        Args:
+            server_name: The name of the MCP server that is statically configured.
+
+        Returns:
+            Error response with response "Cannot delete statically configured MCP server" and cause
+            "MCP server 'server_name' is defined in configuration and cannot be removed via the API."
+        """
+        return cls(
+            response="Cannot delete statically configured MCP server",
+            cause=(
+                f"MCP server '{server_name}' is defined in configuration "
+                "and cannot be removed via the API."
+            ),
+        )
 
     def __init__(self, *, response: str, cause: str) -> None:
         """Construct a ForbiddenResponse with a public message and an internal cause.
diff --git a/src/models/api/responses/error/internal.py b/src/models/api/responses/error/internal.py
@@ -80,6 +80,15 @@ class InternalServerErrorResponse(AbstractErrorResponse):
                         "cause": "Missing or invalid 'clusterID' in ClusterVersion",
                     },
                 },
+                {
+                    "label": "mcp server registration",
+                    "detail": {
+                        "response": "Failed to register MCP server",
+                        "cause": (
+                            "Could not register the MCP server with the remote service."
+                        ),
+                    },
+                },
             ]
         }
     }
@@ -173,6 +182,20 @@ def database_error(cls) -> Self:
             response="Database query failed",
             cause="Failed to query the database",
         )
+    
+    @classmethod
+    def mcp_server_registration_failed(cls) -> Self:
+        """
+        Create an InternalServerErrorResponse representing a failed MCP server registration.
+
+        Returns:
+            Error response with response "Failed to register MCP server" and cause
+            "Could not register the MCP server with the remote service."
+        """
+        return cls(
+            response="Failed to register MCP server",
+            cause=("Could not register the MCP server with the remote service."),
+        )
 
     def __init__(self, *, response: str, cause: str) -> None:
         """Initialize the error response for internal server errors and set the HTTP status code.
diff --git a/tests/e2e/features/mcp_servers_api.feature b/tests/e2e/features/mcp_servers_api.feature
@@ -48,10 +48,18 @@ Feature: MCP Server Management API tests
     Then The status code of the response is 403
     And The body of the response contains statically configured
 
-  Scenario: Delete non-existent MCP server returns 404
+  Scenario: Delete non-existent MCP server returns 200 with not-found outcome
     When I access REST API endpoint "mcp-servers/non-existent-server" using HTTP DELETE method
-    Then The status code of the response is 404
-    And The body of the response contains Mcp Server not found
+    Then The status code of the response is 200
+    And The body of the response is the following
+    """
+    {
+        "deleted": false,
+        "name": "non-existent-server",
+        "response": "MCP server not found",
+        "success": true
+    }
+    """
 
   Scenario: Register MCP server with missing required fields returns 422
     When I access REST API endpoint "mcp-servers" using HTTP POST method
@@ -100,9 +108,18 @@ Feature: MCP Server Management API tests
     When I access REST API endpoint "mcp-servers/e2e-lifecycle-server" using HTTP DELETE method
     Then The status code of the response is 200
     And The body of the response contains e2e-lifecycle-server
-    And The body of the response contains unregistered successfully
+    And The body of the response contains deleted successfully
     When I access REST API endpoint "mcp-servers/e2e-lifecycle-server" using HTTP DELETE method
-    Then The status code of the response is 404
+    Then The status code of the response is 200
+    And The body of the response is the following
+    """
+    {
+        "deleted": false,
+        "name": "e2e-lifecycle-server",
+        "response": "MCP server not found",
+        "success": true
+    }
+    """
     When I access REST API endpoint "mcp-servers" using HTTP GET method
     Then The status code of the response is 200
     And the body of the response has the following structure
diff --git a/tests/integration/test_openapi_json.py b/tests/integration/test_openapi_json.py
@@ -237,7 +237,7 @@ def test_servers_section_present_from_url(spec_from_url: dict[str, Any]) -> None
         (
             "/v1/mcp-servers/{name}",
             "delete",
-            {"200", "401", "403", "404", "500", "503"},
+            {"200", "401", "403", "500", "503"},
         ),
         ("/v1/query", "post", {"200", "401", "403", "404", "422", "429", "500", "503"}),
         (
@@ -341,7 +341,7 @@ def test_paths_and_responses_exist_from_file(
         (
             "/v1/mcp-servers/{name}",
             "delete",
-            {"200", "401", "403", "404", "500", "503"},
+            {"200", "401", "403", "500", "503"},
         ),
         ("/v1/query", "post", {"200", "401", "403", "404", "422", "429", "500", "503"}),
         (
diff --git a/tests/unit/app/endpoints/test_mcp_servers.py b/tests/unit/app/endpoints/test_mcp_servers.py
diff --git a/tests/unit/models/responses/test_error_responses.py b/tests/unit/models/responses/test_error_responses.py

Original file line number	Diff line number	Diff line change
`@@ -1229,6 +1229,14 @@`
`1229`	`1229`	`"response": "Configuration is not loaded"`
`1230`	`1230`	`}`
`1231`	`1231`	`}`
	`1232`	`+ },`
	`1233`	`+ "mcp server registration": {`
	`1234`	`+ "value": {`
	`1235`	`+ "detail": {`
	`1236`	`+ "cause": "Could not register the MCP server with the remote service.",`
	`1237`	`+ "response": "Failed to register MCP server"`
	`1238`	`+ }`
	`1239`	`+ }`
`1232`	`1240`	`}`
`1233`	`1241`	`}`
`1234`	`1242`	`}`
`@@ -1397,30 +1405,18 @@`
`1397`	`1405`	`"response": "User does not have permission to access this endpoint"`
`1398`	`1406`	`}`
`1399`	`1407`	`}`
`1400`		`- }`
`1401`		`- },`
`1402`		`- "schema": {`
`1403`		`- "$ref": "#/components/schemas/ForbiddenResponse"`
`1404`		`- }`
`1405`		`- }`
`1406`		`- }`
`1407`		`- },`
`1408`		`- "404": {`
`1409`		`- "description": "Resource not found",`
`1410`		`- "content": {`
`1411`		`- "application/json": {`
`1412`		`- "examples": {`
`1413`		`- "mcp server": {`
	`1408`	`+ },`
	`1409`	`+ "mcp server static": {`
`1414`	`1410`	`"value": {`
`1415`	`1411`	`"detail": {`
`1416`		`- "cause": "Mcp Server with ID test-mcp-server does not exist",`
`1417`		`- "response": "Mcp Server not found"`
	`1412`	`+ "cause": "MCP server 'my-mcp' is defined in configuration and cannot be removed via the API.",`
	`1413`	`+ "response": "Cannot delete statically configured MCP server"`
`1418`	`1414`	`}`
`1419`	`1415`	`}`
`1420`	`1416`	`}`
`1421`	`1417`	`},`
`1422`	`1418`	`"schema": {`
`1423`		`- "$ref": "#/components/schemas/NotFoundResponse"`
	`1419`	`+ "$ref": "#/components/schemas/ForbiddenResponse"`
`1424`	`1420`	`}`
`1425`	`1421`	`}`
`1426`	`1422`	`}`
`@@ -13221,6 +13217,13 @@`
`13221`	`13217`	`"response": "This instance does not permit overriding model/provider in the query request (missing permission: model_override). Please remove the model and provider fields from your request."`
`13222`	`13218`	`},`
`13223`	`13219`	`"label": "model override"`
	`13220`	`+ },`
	`13221`	`+ {`
	`13222`	`+ "detail": {`
	`13223`	`+ "cause": "MCP server 'my-mcp' is defined in configuration and cannot be removed via the API.",`
	`13224`	`+ "response": "Cannot delete statically configured MCP server"`
	`13225`	`+ },`
	`13226`	`+ "label": "mcp server static"`
`13224`	`13227`	`}`
`13225`	`13228`	`]`
`13226`	`13229`	`},`
`@@ -13514,6 +13517,13 @@`
`13514`	`13517`	`"response": "Internal server error"`
`13515`	`13518`	`},`
`13516`	`13519`	`"label": "invalid cluster version"`
	`13520`	`+ },`
	`13521`	`+ {`
	`13522`	`+ "detail": {`
	`13523`	`+ "cause": "Could not register the MCP server with the remote service.",`
	`13524`	`+ "response": "Failed to register MCP server"`
	`13525`	`+ },`
	`13526`	`+ "label": "mcp server registration"`
`13517`	`13527`	`}`
`13518`	`13528`	`]`
`13519`	`13529`	`},`
Original file line number	Diff line number	Diff line change
`@@ -237,7 +237,7 @@ def test_servers_section_present_from_url(spec_from_url: dict[str, Any]) -> None`
`237`	`237`	`(`
`238`	`238`	`"/v1/mcp-servers/{name}",`
`239`	`239`	`"delete",`
`240`		`- {"200", "401", "403", "404", "500", "503"},`
	`240`	`+ {"200", "401", "403", "500", "503"},`
`241`	`241`	`),`
`242`	`242`	`("/v1/query", "post", {"200", "401", "403", "404", "422", "429", "500", "503"}),`
`243`	`243`	`(`
`@@ -341,7 +341,7 @@ def test_paths_and_responses_exist_from_file(`
`341`	`341`	`(`
`342`	`342`	`"/v1/mcp-servers/{name}",`
`343`	`343`	`"delete",`
`344`		`- {"200", "401", "403", "404", "500", "503"},`
	`344`	`+ {"200", "401", "403", "500", "503"},`
`345`	`345`	`),`
`346`	`346`	`("/v1/query", "post", {"200", "401", "403", "404", "422", "429", "500", "503"}),`
`347`	`347`	`(`