Skip to content

Commit 462a5cf

Browse files
asimurkaasimurka
committed
Addressed comments
1 parent 42bfa9d commit 462a5cf

7 files changed

Lines changed: 99 additions & 40 deletions

File tree

‎docs/openapi.json‎

Lines changed: 49 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1310,9 +1310,21 @@
13101310
"schema": {
13111311
"$ref": "#/components/schemas/MCPServerDeleteResponse"
13121312
},
1313-
"example": {
1314-
"message": "MCP server 'test-mcp-server' unregistered successfully",
1315-
"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+
}
13161328
}
13171329
}
13181330
}
@@ -13813,28 +13825,54 @@
1381313825
},
1381413826
"MCPServerDeleteResponse": {
1381513827
"properties": {
13828+
"deleted": {
13829+
"type": "boolean",
13830+
"title": "Deleted",
13831+
"description": "Whether the deletion was successful.",
13832+
"examples": [
13833+
true,
13834+
false
13835+
]
13836+
},
1381613837
"name": {
1381713838
"type": "string",
1381813839
"title": "Name",
13819-
"description": "Deleted MCP server name"
13840+
"description": "MCP server name that was passed to delete.",
13841+
"examples": [
13842+
"test-mcp-server"
13843+
]
1382013844
},
13821-
"message": {
13845+
"response": {
1382213846
"type": "string",
13823-
"title": "Message",
13824-
"description": "Status message"
13847+
"title": "Response",
13848+
"description": "Human-readable outcome of the delete operation.",
13849+
"readOnly": true
1382513850
}
1382613851
},
1382713852
"type": "object",
1382813853
"required": [
13854+
"deleted",
1382913855
"name",
13830-
"message"
13856+
"response"
1383113857
],
1383213858
"title": "MCPServerDeleteResponse",
13833-
"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\".",
1383413860
"examples": [
1383513861
{
13836-
"message": "MCP server 'test-mcp-server' unregistered successfully",
13837-
"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+
}
1383813876
}
1383913877
]
1384013878
},

‎src/app/endpoints/mcp_servers.py‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

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

99
from authentication import get_auth_dependency
1010
from authentication.interface import AuthTuple
@@ -221,13 +221,6 @@ async def delete_mcp_server_handler(
221221
response = ForbiddenResponse.mcp_server_static_config(name)
222222
raise HTTPException(**response.model_dump())
223223

224-
try:
225-
configuration.remove_mcp_server(name)
226-
local_deleted = True
227-
except ValueError as e:
228-
logger.error("Failed to remove MCP server from configuration: %s", e)
229-
local_deleted = False
230-
231224
try:
232225
client = AsyncLlamaStackClientHolder().get_client()
233226
await client.toolgroups.unregister( # pyright: ignore[reportDeprecated]
@@ -239,7 +232,14 @@ async def delete_mcp_server_handler(
239232
backend_name="Llama Stack", cause=str(e)
240233
)
241234
raise HTTPException(**svc_response.model_dump()) from e
242-
except (APIStatusError, ToolGroupNotFoundError):
235+
except (ToolGroupNotFoundError, NotFoundError):
243236
logger.warning("MCP server not found, treating as already deleted.")
244237

238+
try:
239+
configuration.remove_mcp_server(name)
240+
local_deleted = True
241+
except ValueError as e:
242+
logger.error("Failed to remove MCP server from configuration: %s", e)
243+
local_deleted = False
244+
245245
return MCPServerDeleteResponse(deleted=local_deleted, name=name)

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -166,17 +166,17 @@ def model_override(cls) -> Self:
166166
"to override model/provider."
167167
),
168168
)
169-
169+
170170
@classmethod
171171
def mcp_server_static_config(cls, server_name: str) -> Self:
172-
"""Create a ForbiddenResponse indicating the client tried to delete a statically configured MCP server.
172+
"""Create a response for an attempt to delete a statically configured MCP server.
173173
174174
Args:
175175
server_name: The name of the MCP server that is statically configured.
176176
177177
Returns:
178-
Error response with response "Cannot delete statically configured MCP server" and cause
179-
"MCP server 'server_name' is defined in configuration and cannot be removed via the API."
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.
180180
"""
181181
return cls(
182182
response="Cannot delete statically configured MCP server",

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ def database_error(cls) -> Self:
182182
response="Database query failed",
183183
cause="Failed to query the database",
184184
)
185-
185+
186186
@classmethod
187187
def mcp_server_registration_failed(cls) -> Self:
188188
"""

‎src/models/api/responses/successful/mcp_servers.py‎

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
11
"""Successful responses for MCP server registration and listing."""
22

3+
from typing import ClassVar
4+
35
from pydantic import Field
46

5-
from models.api.responses.successful.bases import AbstractSuccessfulResponse
7+
from models.api.responses.successful.bases import (
8+
AbstractDeleteResponse,
9+
AbstractSuccessfulResponse,
10+
)
611
from models.common.mcp import MCPServerAuthInfo, MCPServerInfo
712

813

@@ -103,24 +108,41 @@ class MCPServerListResponse(AbstractSuccessfulResponse):
103108
}
104109

105110

106-
class MCPServerDeleteResponse(AbstractSuccessfulResponse):
107-
"""Response for a successful MCP server deletion.
111+
class MCPServerDeleteResponse(AbstractDeleteResponse):
112+
"""Response indicating the outcome of an MCP server delete operation.
108113
109114
Attributes:
110-
name: Deleted MCP server name.
111-
message: Status message.
115+
name: Name of the MCP server targeted for deletion.
116+
deleted: Whether the server was successfully deleted (True) or not found (False).
117+
response: Description of the result, e.g. "MCP server deleted successfully".
112118
"""
113119

114-
name: str = Field(..., description="Deleted MCP server name")
115-
message: str = Field(..., description="Status message")
120+
resource_name: ClassVar[str] = "MCP server"
121+
name: str = Field(
122+
...,
123+
description="MCP server name that was passed to delete.",
124+
examples=["test-mcp-server"],
125+
)
116126

117127
model_config = {
118128
"json_schema_extra": {
119129
"examples": [
120130
{
121-
"name": "test-mcp-server",
122-
"message": "MCP server 'test-mcp-server' unregistered successfully",
123-
}
131+
"label": "deleted",
132+
"value": {
133+
"name": "mcp-server",
134+
"deleted": True,
135+
"response": "MCP server deleted successfully",
136+
},
137+
},
138+
{
139+
"label": "not found",
140+
"value": {
141+
"name": "mcp-server",
142+
"deleted": False,
143+
"response": "MCP server not found",
144+
},
145+
},
124146
]
125147
}
126148
}

‎tests/e2e/features/mcp_servers_api.feature‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,7 @@ Feature: MCP Server Management API tests
5656
{
5757
"deleted": false,
5858
"name": "non-existent-server",
59-
"response": "MCP server not found",
60-
"success": true
59+
"response": "MCP server not found"
6160
}
6261
"""
6362

@@ -116,8 +115,7 @@ Feature: MCP Server Management API tests
116115
{
117116
"deleted": false,
118117
"name": "e2e-lifecycle-server",
119-
"response": "MCP server not found",
120-
"success": true
118+
"response": "MCP server not found"
121119
}
122120
"""
123121
When I access REST API endpoint "mcp-servers" using HTTP GET method

‎tests/unit/models/responses/test_error_responses.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -518,7 +518,7 @@ def test_openapi_response(self) -> None:
518518

519519
# Verify example count matches schema examples count
520520
assert len(examples) == expected_count
521-
assert expected_count == 8
521+
assert expected_count == 9
522522

523523
# Verify all labeled examples are present
524524
assert "conversation" in examples
@@ -529,6 +529,7 @@ def test_openapi_response(self) -> None:
529529
assert "vector store" in examples
530530
assert "file" in examples
531531
assert "prompt" in examples
532+
assert "mcp server" in examples
532533

533534
# Verify example structure for one example
534535
conversation_example = examples["conversation"]

0 commit comments

Comments
 (0)