Skip to content

Commit f7b927a

Browse files
tisniktisnik
authored
Merge pull request #1627 from asimurka/refactor_provider_response
LCORE-2034: Unified mcp registration endpoints
2 parents d0b65f5 + 462a5cf commit f7b927a

9 files changed

Lines changed: 258 additions & 91 deletions

File tree

‎docs/openapi.json‎

Lines changed: 76 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1229,6 +1229,14 @@
12291229
"response": "Configuration is not loaded"
12301230
}
12311231
}
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+
}
12321240
}
12331241
}
12341242
}
@@ -1302,9 +1310,21 @@
13021310
"schema": {
13031311
"$ref": "#/components/schemas/MCPServerDeleteResponse"
13041312
},
1305-
"example": {
1306-
"message": "MCP server 'test-mcp-server' unregistered successfully",
1307-
"name": "test-mcp-server"
1313+
"examples": {
1314+
"deleted": {
1315+
"value": {
1316+
"deleted": true,
1317+
"name": "mcp-server",
1318+
"response": "MCP server deleted successfully"
1319+
}
1320+
},
1321+
"not found": {
1322+
"value": {
1323+
"deleted": false,
1324+
"name": "mcp-server",
1325+
"response": "MCP server not found"
1326+
}
1327+
}
13081328
}
13091329
}
13101330
}
@@ -1397,30 +1417,18 @@
13971417
"response": "User does not have permission to access this endpoint"
13981418
}
13991419
}
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": {
1420+
},
1421+
"mcp server static": {
14141422
"value": {
14151423
"detail": {
1416-
"cause": "Mcp Server with ID test-mcp-server does not exist",
1417-
"response": "Mcp Server not found"
1424+
"cause": "MCP server 'my-mcp' is defined in configuration and cannot be removed via the API.",
1425+
"response": "Cannot delete statically configured MCP server"
14181426
}
14191427
}
14201428
}
14211429
},
14221430
"schema": {
1423-
"$ref": "#/components/schemas/NotFoundResponse"
1431+
"$ref": "#/components/schemas/ForbiddenResponse"
14241432
}
14251433
}
14261434
}
@@ -13221,6 +13229,13 @@
1322113229
"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."
1322213230
},
1322313231
"label": "model override"
13232+
},
13233+
{
13234+
"detail": {
13235+
"cause": "MCP server 'my-mcp' is defined in configuration and cannot be removed via the API.",
13236+
"response": "Cannot delete statically configured MCP server"
13237+
},
13238+
"label": "mcp server static"
1322413239
}
1322513240
]
1322613241
},
@@ -13514,6 +13529,13 @@
1351413529
"response": "Internal server error"
1351513530
},
1351613531
"label": "invalid cluster version"
13532+
},
13533+
{
13534+
"detail": {
13535+
"cause": "Could not register the MCP server with the remote service.",
13536+
"response": "Failed to register MCP server"
13537+
},
13538+
"label": "mcp server registration"
1351713539
}
1351813540
]
1351913541
},
@@ -13803,28 +13825,54 @@
1380313825
},
1380413826
"MCPServerDeleteResponse": {
1380513827
"properties": {
13828+
"deleted": {
13829+
"type": "boolean",
13830+
"title": "Deleted",
13831+
"description": "Whether the deletion was successful.",
13832+
"examples": [
13833+
true,
13834+
false
13835+
]
13836+
},
1380613837
"name": {
1380713838
"type": "string",
1380813839
"title": "Name",
13809-
"description": "Deleted MCP server name"
13840+
"description": "MCP server name that was passed to delete.",
13841+
"examples": [
13842+
"test-mcp-server"
13843+
]
1381013844
},
13811-
"message": {
13845+
"response": {
1381213846
"type": "string",
13813-
"title": "Message",
13814-
"description": "Status message"
13847+
"title": "Response",
13848+
"description": "Human-readable outcome of the delete operation.",
13849+
"readOnly": true
1381513850
}
1381613851
},
1381713852
"type": "object",
1381813853
"required": [
13854+
"deleted",
1381913855
"name",
13820-
"message"
13856+
"response"
1382113857
],
1382213858
"title": "MCPServerDeleteResponse",
13823-
"description": "Response for a successful MCP server deletion.\n\nAttributes:\n name: Deleted MCP server name.\n message: Status message.",
13859+
"description": "Response indicating the outcome of an MCP server delete operation.\n\nAttributes:\n name: Name of the MCP server targeted for deletion.\n deleted: Whether the server was successfully deleted (True) or not found (False).\n response: Description of the result, e.g. \"MCP server deleted successfully\".",
1382413860
"examples": [
1382513861
{
13826-
"message": "MCP server 'test-mcp-server' unregistered successfully",
13827-
"name": "test-mcp-server"
13862+
"label": "deleted",
13863+
"value": {
13864+
"deleted": true,
13865+
"name": "mcp-server",
13866+
"response": "MCP server deleted successfully"
13867+
}
13868+
},
13869+
{
13870+
"label": "not found",
13871+
"value": {
13872+
"deleted": false,
13873+
"name": "mcp-server",
13874+
"response": "MCP server not found"
13875+
}
1382813876
}
1382913877
]
1383013878
},

‎src/app/endpoints/mcp_servers.py‎

Lines changed: 17 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
from typing import Annotated, Any
44

55
from fastapi import APIRouter, Depends, HTTPException, Request, status
6-
from llama_stack_client import APIConnectionError
6+
from llama_stack_api.common.errors import ToolGroupNotFoundError
7+
from llama_stack_client import APIConnectionError, NotFoundError
78

89
from authentication import get_auth_dependency
910
from authentication.interface import AuthTuple
@@ -17,7 +18,6 @@
1718
ConflictResponse,
1819
ForbiddenResponse,
1920
InternalServerErrorResponse,
20-
NotFoundResponse,
2121
ServiceUnavailableResponse,
2222
UnauthorizedResponse,
2323
)
@@ -39,7 +39,9 @@
3939
401: UnauthorizedResponse.openapi_response(examples=UNAUTHORIZED_OPENAPI_EXAMPLES),
4040
403: ForbiddenResponse.openapi_response(examples=["endpoint"]),
4141
409: ConflictResponse.openapi_response(examples=["mcp server"]),
42-
500: InternalServerErrorResponse.openapi_response(examples=["configuration"]),
42+
500: InternalServerErrorResponse.openapi_response(
43+
examples=["configuration", "mcp server registration"]
44+
),
4345
503: ServiceUnavailableResponse.openapi_response(
4446
examples=["llama stack", "kubernetes api"]
4547
),
@@ -109,10 +111,7 @@ async def register_mcp_server_handler(
109111
except Exception as e: # pylint: disable=broad-exception-caught
110112
configuration.remove_mcp_server(body.name)
111113
logger.error("Failed to register MCP toolgroup: %s", e)
112-
error_response = InternalServerErrorResponse(
113-
response="Failed to register MCP server",
114-
cause=str(e),
115-
)
114+
error_response = InternalServerErrorResponse.mcp_server_registration_failed()
116115
raise HTTPException(**error_response.model_dump()) from e
117116

118117
logger.info("Dynamically registered MCP server: %s at %s", body.name, body.url)
@@ -178,8 +177,7 @@ async def list_mcp_servers_handler(
178177
delete_responses: dict[int | str, dict[str, Any]] = {
179178
200: MCPServerDeleteResponse.openapi_response(),
180179
401: UnauthorizedResponse.openapi_response(examples=UNAUTHORIZED_OPENAPI_EXAMPLES),
181-
403: ForbiddenResponse.openapi_response(examples=["endpoint"]),
182-
404: NotFoundResponse.openapi_response(examples=["mcp server"]),
180+
403: ForbiddenResponse.openapi_response(examples=["endpoint", "mcp server static"]),
183181
500: InternalServerErrorResponse.openapi_response(examples=["configuration"]),
184182
503: ServiceUnavailableResponse.openapi_response(
185183
examples=["llama stack", "kubernetes api"]
@@ -218,46 +216,30 @@ async def delete_mcp_server_handler(
218216
check_configuration_loaded(configuration)
219217

220218
if not configuration.is_dynamic_mcp_server(name):
221-
found = any(s.name == name for s in configuration.mcp_servers)
222-
if found:
223-
response = ForbiddenResponse(
224-
response="Cannot delete statically configured MCP server",
225-
cause=f"MCP server '{name}' was configured in lightspeed-stack.yaml "
226-
"and cannot be removed via the API.",
227-
)
228-
else:
229-
response = NotFoundResponse(resource="MCP server", resource_id=name)
230-
raise HTTPException(**response.model_dump())
219+
static_mcp_names = {s.name for s in configuration.mcp_servers}
220+
if name in static_mcp_names:
221+
response = ForbiddenResponse.mcp_server_static_config(name)
222+
raise HTTPException(**response.model_dump())
231223

232224
try:
233225
client = AsyncLlamaStackClientHolder().get_client()
234226
await client.toolgroups.unregister( # pyright: ignore[reportDeprecated]
235227
toolgroup_id=name
236228
)
237229
except APIConnectionError as e:
238-
logger.error("Failed to unregister MCP toolgroup from Llama Stack: %s", e)
230+
logger.error("Failed to connect to Llama Stack: %s", e)
239231
svc_response = ServiceUnavailableResponse(
240232
backend_name="Llama Stack", cause=str(e)
241233
)
242234
raise HTTPException(**svc_response.model_dump()) from e
243-
except Exception as e: # pylint: disable=broad-exception-caught
244-
logger.warning(
245-
"Llama Stack toolgroup unregister failed for '%s', "
246-
"proceeding with local removal: %s",
247-
name,
248-
e,
249-
)
235+
except (ToolGroupNotFoundError, NotFoundError):
236+
logger.warning("MCP server not found, treating as already deleted.")
250237

251238
try:
252239
configuration.remove_mcp_server(name)
240+
local_deleted = True
253241
except ValueError as e:
254242
logger.error("Failed to remove MCP server from configuration: %s", e)
255-
response = NotFoundResponse(resource="MCP server", resource_id=name)
256-
raise HTTPException(**response.model_dump()) from e
243+
local_deleted = False
257244

258-
logger.info("Dynamically unregistered MCP server: %s", name)
259-
260-
return MCPServerDeleteResponse(
261-
name=name,
262-
message=f"MCP server '{name}' unregistered successfully",
263-
)
245+
return MCPServerDeleteResponse(deleted=local_deleted, name=name)

‎src/models/api/responses/error/forbidden.py‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,16 @@ class ForbiddenResponse(AbstractErrorResponse):
8585
),
8686
},
8787
},
88+
{
89+
"label": "mcp server static",
90+
"detail": {
91+
"response": "Cannot delete statically configured MCP server",
92+
"cause": (
93+
"MCP server 'my-mcp' is defined in configuration "
94+
"and cannot be removed via the API."
95+
),
96+
},
97+
},
8898
]
8999
}
90100
}
@@ -157,6 +167,25 @@ def model_override(cls) -> Self:
157167
),
158168
)
159169

170+
@classmethod
171+
def mcp_server_static_config(cls, server_name: str) -> Self:
172+
"""Create a response for an attempt to delete a statically configured MCP server.
173+
174+
Args:
175+
server_name: The name of the MCP server that is statically configured.
176+
177+
Returns:
178+
A ``ForbiddenResponse`` with the static-delete user message and a cause
179+
that names ``server_name`` and states it cannot be removed via the API.
180+
"""
181+
return cls(
182+
response="Cannot delete statically configured MCP server",
183+
cause=(
184+
f"MCP server '{server_name}' is defined in configuration "
185+
"and cannot be removed via the API."
186+
),
187+
)
188+
160189
def __init__(self, *, response: str, cause: str) -> None:
161190
"""Construct a ForbiddenResponse with a public message and an internal cause.
162191

‎src/models/api/responses/error/internal.py‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,15 @@ class InternalServerErrorResponse(AbstractErrorResponse):
8080
"cause": "Missing or invalid 'clusterID' in ClusterVersion",
8181
},
8282
},
83+
{
84+
"label": "mcp server registration",
85+
"detail": {
86+
"response": "Failed to register MCP server",
87+
"cause": (
88+
"Could not register the MCP server with the remote service."
89+
),
90+
},
91+
},
8392
]
8493
}
8594
}
@@ -174,6 +183,20 @@ def database_error(cls) -> Self:
174183
cause="Failed to query the database",
175184
)
176185

186+
@classmethod
187+
def mcp_server_registration_failed(cls) -> Self:
188+
"""
189+
Create an InternalServerErrorResponse representing a failed MCP server registration.
190+
191+
Returns:
192+
Error response with response "Failed to register MCP server" and cause
193+
"Could not register the MCP server with the remote service."
194+
"""
195+
return cls(
196+
response="Failed to register MCP server",
197+
cause=("Could not register the MCP server with the remote service."),
198+
)
199+
177200
def __init__(self, *, response: str, cause: str) -> None:
178201
"""Initialize the error response for internal server errors and set the HTTP status code.
179202

0 commit comments

Comments
 (0)