RHIDP-12707: Update docs and add E2E tests for MCP APIs#1342
RHIDP-12707: Update docs and add E2E tests for MCP APIs#1342maysunfaisal wants to merge 3 commits intolightspeed-core:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds runtime MCP server management: new REST endpoints to register, list, and delete MCP servers; OpenAPI schemas and RBAC action updates; documentation and getting-started updates; end-to-end and integration tests plus a DELETE test helper and environment config switches. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API Server
participant Auth as Authorization
participant Registry as MCP Registry
Client->>API: POST /v1/mcp-servers {name, url, ...}
API->>Auth: check action: register_mcp_server
Auth-->>API: allow / deny
alt allowed
API->>Registry: store server (source="api", runtime)
Registry-->>API: stored (201, server info)
API-->>Client: 201 Created {name, source:"api", ...}
else denied
API-->>Client: 403 Forbidden
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/openapi.json (1)
5212-5230:⚠️ Potential issue | 🟠 MajorUse unique
operationIdvalues for GET and POST on/a2a.Both the GET and POST operations use
handle_a2a_jsonrpc_a2a_post, which violates the OpenAPI specification requirement that eachoperationIdmust be unique. This can break SDK/codegen pipelines.🛠️ Suggested fix
"/a2a": { "get": { - "operationId": "handle_a2a_jsonrpc_a2a_post", + "operationId": "handle_a2a_jsonrpc_a2a_get", "post": { "operationId": "handle_a2a_jsonrpc_a2a_post",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi.json` around lines 5212 - 5230, The OpenAPI spec has duplicate operationId values for the /a2a path (both GET and POST use "handle_a2a_jsonrpc_a2a_post"), which violates uniqueness and breaks codegen; update the operationId for one of the operations so they are unique (e.g., change the GET to "handle_a2a_jsonrpc_a2a_get" or the POST to "handle_a2a_jsonrpc_a2a_post_jsonrpc") by editing the operationId fields under the /a2a path in docs/openapi.json so each HTTP method has a distinct identifier.
🧹 Nitpick comments (3)
docs/config.md (1)
178-178: Align this entry with theModelContextProtocolServersection to avoid contradictory behavior docs.This line correctly documents runtime registration, but the later
ModelContextProtocolServerdescription still states only YAML-defined servers are available. Please update that section too so readers don’t get conflicting guidance.📝 Suggested doc alignment
-Only MCP servers -defined in the lightspeed-stack.yaml configuration are available to the -agents. +MCP servers defined in lightspeed-stack.yaml and servers registered at +runtime via the MCP Server Management API are available to agents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/config.md` at line 178, The docs are inconsistent: the mcp_servers entry correctly states runtime registration via POST /v1/mcp-servers but the ModelContextProtocolServer section still claims only YAML-defined servers are available; update the ModelContextProtocolServer section to match mcp_servers by adding the same note about dynamic registration and noting that servers registered via the API (POST /v1/mcp-servers) are available at runtime, and clarify that tools in llama-stack run.yaml remain inaccessible to lightspeed-core agents; reference the symbols mcp_servers and ModelContextProtocolServer when editing to ensure both sections convey the same behavior.tests/integration/test_openapi_json.py (1)
203-285: Reduce duplicated path matrices to prevent drift.Both parametrize blocks maintain near-identical endpoint lists; future edits can easily update one and miss the other. Consider extracting shared cases into one helper and varying only the root endpoint codes.
Refactor sketch
+COMMON_PATH_CASES: list[tuple[str, str, set[str]]] = [ + ("/v1/info", "get", {"200", "401", "403", "503"}), + # ... all shared entries ... + ("/v1/mcp-servers/{name}", "delete", {"200", "401", "403", "404", "500", "503"}), +] + +def _cases(root_codes: set[str]) -> list[tuple[str, str, set[str]]]: + return [("/", "get", root_codes), *COMMON_PATH_CASES] + `@pytest.mark.parametrize`( "path,method,expected_codes", - [ - ("/", "get", {"200"}), - ... - ], + _cases({"200"}), ) def test_paths_and_responses_exist_from_file(...): ... `@pytest.mark.parametrize`( "path,method,expected_codes", - [ - ("/", "get", {"200", "401", "403"}), - ... - ], + _cases({"200", "401", "403"}), ) def test_paths_and_responses_exist_from_url(...): ...Also applies to: 306-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_openapi_json.py` around lines 203 - 285, The two pytest.mark.parametrize blocks both define the same "path,method,expected_codes" matrix and should be consolidated to avoid drift: extract the common list of endpoint tuples into a single constant (e.g., COMMON_ENDPOINT_CASES) and then build the two parametrizations from it by mapping/overriding only the differing root-level expected_codes (use pytest.param or a small helper that replaces expected_codes for "/" and "/v1/info" as needed). Update the test that currently uses the explicit list (the parametrize with "path,method,expected_codes") to import/iterate COMMON_ENDPOINT_CASES so both tests share the same source of truth (refer to the existing parametrize signature "path,method,expected_codes" and the tuple entries like ("/", "get", {"200"}) and ("/v1/info", "get", {...}) when replacing).tests/e2e/features/mcp_servers_api.feature (1)
86-90: Strengthen post-delete verification in lifecycle scenario.At Line 90, the final GET only confirms a 200 response. It doesn’t prove
e2e-lifecycle-serveris gone. Add an explicit absence check (or a second DELETE expecting 404) to validate actual unregistration.Suggested test hardening
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 unregistered 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 When I access REST API endpoint "mcp-servers" using HTTP GET method Then The status code of the response is 200🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/mcp_servers_api.feature` around lines 86 - 90, The final GET in the lifecycle scenario only asserts a 200 and doesn't verify that "e2e-lifecycle-server" was removed; update the scenario in tests/e2e/features/mcp_servers_api.feature so after DELETE to "mcp-servers/e2e-lifecycle-server" and the existing 200/assertion you either (a) perform a GET to "mcp-servers/e2e-lifecycle-server" and assert 404/Not Found, or (b) perform the GET to "mcp-servers" and assert the response body does not contain "e2e-lifecycle-server" (or both); ensure you reference the existing endpoints "mcp-servers/e2e-lifecycle-server" and "mcp-servers" when adding the new assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/openapi.json`:
- Around line 5212-5230: The OpenAPI spec has duplicate operationId values for
the /a2a path (both GET and POST use "handle_a2a_jsonrpc_a2a_post"), which
violates uniqueness and breaks codegen; update the operationId for one of the
operations so they are unique (e.g., change the GET to
"handle_a2a_jsonrpc_a2a_get" or the POST to
"handle_a2a_jsonrpc_a2a_post_jsonrpc") by editing the operationId fields under
the /a2a path in docs/openapi.json so each HTTP method has a distinct
identifier.
---
Nitpick comments:
In `@docs/config.md`:
- Line 178: The docs are inconsistent: the mcp_servers entry correctly states
runtime registration via POST /v1/mcp-servers but the ModelContextProtocolServer
section still claims only YAML-defined servers are available; update the
ModelContextProtocolServer section to match mcp_servers by adding the same note
about dynamic registration and noting that servers registered via the API (POST
/v1/mcp-servers) are available at runtime, and clarify that tools in llama-stack
run.yaml remain inaccessible to lightspeed-core agents; reference the symbols
mcp_servers and ModelContextProtocolServer when editing to ensure both sections
convey the same behavior.
In `@tests/e2e/features/mcp_servers_api.feature`:
- Around line 86-90: The final GET in the lifecycle scenario only asserts a 200
and doesn't verify that "e2e-lifecycle-server" was removed; update the scenario
in tests/e2e/features/mcp_servers_api.feature so after DELETE to
"mcp-servers/e2e-lifecycle-server" and the existing 200/assertion you either (a)
perform a GET to "mcp-servers/e2e-lifecycle-server" and assert 404/Not Found, or
(b) perform the GET to "mcp-servers" and assert the response body does not
contain "e2e-lifecycle-server" (or both); ensure you reference the existing
endpoints "mcp-servers/e2e-lifecycle-server" and "mcp-servers" when adding the
new assertion.
In `@tests/integration/test_openapi_json.py`:
- Around line 203-285: The two pytest.mark.parametrize blocks both define the
same "path,method,expected_codes" matrix and should be consolidated to avoid
drift: extract the common list of endpoint tuples into a single constant (e.g.,
COMMON_ENDPOINT_CASES) and then build the two parametrizations from it by
mapping/overriding only the differing root-level expected_codes (use
pytest.param or a small helper that replaces expected_codes for "/" and
"/v1/info" as needed). Update the test that currently uses the explicit list
(the parametrize with "path,method,expected_codes") to import/iterate
COMMON_ENDPOINT_CASES so both tests share the same source of truth (refer to the
existing parametrize signature "path,method,expected_codes" and the tuple
entries like ("/", "get", {"200"}) and ("/v1/info", "get", {...}) when
replacing).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 943c5eb3-ab8f-4139-9b84-a2f3ac10898f
📒 Files selected for processing (9)
docs/ARCHITECTURE.mddocs/config.mddocs/getting_started.mddocs/openapi.jsondocs/openapi.mdtests/e2e/features/mcp_servers_api.featuretests/e2e/features/steps/common_http.pytests/e2e/test_list.txttests/integration/test_openapi_json.py
tisnik
left a comment
There was a problem hiding this comment.
Please remove docs/openapi.md from this PR. The generation script is broken. I'll update it later.
60cd540 to
402d2a8
Compare
tisnik
left a comment
There was a problem hiding this comment.
LGTM from my perspective. Also @radofuchs PTAL (e2e part).
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/openapi.json (1)
5212-5230:⚠️ Potential issue | 🟠 MajorUse unique
operationIdvalues for/a2aGET and POST.Lines 5212 (GET) and 5230 (POST) both use
handle_a2a_jsonrpc_a2a_post, creating an operationId collision that violates the OpenAPI specification and breaks client generation. Change the GET endpoint's operationId tohandle_a2a_jsonrpc_a2a_get.Proposed fix
"operationId": "handle_a2a_jsonrpc_a2a_post", + "operationId": "handle_a2a_jsonrpc_a2a_get",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi.json` around lines 5212 - 5230, The OpenAPI spec currently has duplicate operationId "handle_a2a_jsonrpc_a2a_post" for both the GET and POST /a2a endpoints; locate the GET operation that currently uses operationId "handle_a2a_jsonrpc_a2a_post" and rename it to "handle_a2a_jsonrpc_a2a_get" so the GET and POST operationIds are unique (ensure any references to the old GET operationId are updated accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/config.md`:
- Line 178: The docs currently contradict about MCP server availability: update
the ModelContextProtocolServer section to match the mcp_servers table by stating
that MCP servers can be defined in lightspeed-stack.yaml and also registered
dynamically via the POST /v1/mcp-servers API (i.e., both YAML-defined and
API-registered ModelContextProtocolServer instances are available to agents);
keep the note that tools configured in llama-stack run.yaml are not accessible
to lightspeed-core agents and ensure the terminology uses the same symbols
(mcp_servers, ModelContextProtocolServer, lightspeed-stack.yaml, POST
/v1/mcp-servers, llama-stack run.yaml) so both sections read consistently.
In `@docs/getting_started.md`:
- Around line 260-333: Add a short RBAC prerequisites note explaining that when
authorization is enabled the MCP server API endpoints (POST /v1/mcp-servers, GET
/v1/mcp-servers, DELETE /v1/mcp-servers/{name}) require explicit permissions to
avoid 403 responses; list the needed actions (create/register, list/read,
delete/unregister) and give example permission names (e.g., mcp.servers.create,
mcp.servers.list, mcp.servers.delete) and a reminder that only servers with
source "api" can be deleted.
In `@tests/e2e/features/mcp_servers_api.feature`:
- Around line 64-70: The scenario "Register MCP server with invalid URL scheme
returns 422" is currently ambiguous because the 422 may come from missing
required fields rather than URL scheme validation; update the POST payload sent
to the "mcp-servers" endpoint in that scenario to include a valid provider_id
along with name and url (e.g., add "provider_id": "<valid-id>") so the request
only fails due to the invalid URL scheme; ensure the body contains
"provider_id", "name", and "url" to isolate scheme validation.
- Around line 86-90: The lifecycle test only asserts the DELETE returned 200 but
doesn't verify the server was actually removed; update the scenario in
tests/e2e/features/mcp_servers_api.feature to add a post-delete assertion for
the deleted resource "mcp-servers/e2e-lifecycle-server" (for example, perform a
subsequent GET to "mcp-servers/e2e-lifecycle-server" and assert a 404, or repeat
the DELETE and assert 404) so the side effect of deletion is verified rather
than just the success status.
---
Outside diff comments:
In `@docs/openapi.json`:
- Around line 5212-5230: The OpenAPI spec currently has duplicate operationId
"handle_a2a_jsonrpc_a2a_post" for both the GET and POST /a2a endpoints; locate
the GET operation that currently uses operationId "handle_a2a_jsonrpc_a2a_post"
and rename it to "handle_a2a_jsonrpc_a2a_get" so the GET and POST operationIds
are unique (ensure any references to the old GET operationId are updated
accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b91f451d-9c84-49eb-a1c8-fab8dc4fb35e
📒 Files selected for processing (8)
docs/ARCHITECTURE.mddocs/config.mddocs/getting_started.mddocs/openapi.jsontests/e2e/features/mcp_servers_api.featuretests/e2e/features/steps/common_http.pytests/e2e/test_list.txttests/integration/test_openapi_json.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/features/steps/common_http.py
- tests/integration/test_openapi_json.py
| And The body of the response contains api | ||
| 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 unregistered successfully |
There was a problem hiding this comment.
missing in the response that the proper mcp was unregistered successfully
| 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 contains e2e-lifecycle-server | ||
| And The body of the response contains api |
There was a problem hiding this comment.
I would rather see here the response assertion as in the tests List MCP servers response has expected structure
| """ | ||
| {"url": "http://mock-mcp:3001"} | ||
| """ | ||
| Then The status code of the response is 422 |
There was a problem hiding this comment.
also verify here the body of the response, no need for the whole structure, but need to make sure that the response contains proper fields missing description
| Scenario: Delete non-existent MCP server returns 404 | ||
| Given The system is in default state | ||
| When I access REST API endpoint "mcp-servers/non-existent-server" using HTTP DELETE method | ||
| Then The status code of the response is 404 |
There was a problem hiding this comment.
also verify here the response body as you do in the test above
| """ | ||
| {"name": "bad-url-server", "url": "ftp://mock-mcp:3001"} | ||
| """ | ||
| Then The status code of the response is 422 |
| Given The service is started locally | ||
| And REST API service prefix is /v1 | ||
|
|
||
| Scenario: List MCP servers returns pre-configured servers |
There was a problem hiding this comment.
also add another test when no mcp server is configured
| @@ -0,0 +1,90 @@ | |||
| @MCP | |||
There was a problem hiding this comment.
missing tests for 401and 503 status codes here
Assisted-by: Claude Opus 4.6 Generated-by: Cursor Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Assisted-by: Claude Opus 4.6 Generated-by: Cursor Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
402d2a8 to
1aac7b4
Compare
|
@radofuchs Please take a look, thank you! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/openapi.json (1)
5212-5230:⚠️ Potential issue | 🟠 MajorFix duplicate
operationIdon/a2aGET and POST endpoints.Both GET and POST
/a2aendpoints usehandle_a2a_jsonrpc_a2a_postas their operationId. OpenAPI requires operationId values to be unique per operation; duplicates break SDK generation and API tooling.🛠️ Proposed fix
- "operationId": "handle_a2a_jsonrpc_a2a_post", + "operationId": "handle_a2a_jsonrpc_a2a_get", ... "operationId": "handle_a2a_jsonrpc_a2a_post",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi.json` around lines 5212 - 5230, The GET and POST operations for the /a2a path share the same operationId "handle_a2a_jsonrpc_a2a_post", which must be unique; update one of them (preferably the GET operation) to a distinct id such as "handle_a2a_jsonrpc_a2a_get" so each operationId is unique, ensuring you change the operationId string in the OpenAPI document for the GET operation (search for "handle_a2a_jsonrpc_a2a_post" under the GET /a2a entry and replace it) and keep the POST operationId as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/features/mcp_servers_api.feature`:
- Line 55: The test assertion uses the wrong casing "Mcp Server not found" which
mismatches the API's resource label; update the step text (the Gherkin line "And
The body of the response contains Mcp Server not found") to use the canonical
message "MCP server not found" so the assertion matches the API response
exactly.
---
Outside diff comments:
In `@docs/openapi.json`:
- Around line 5212-5230: The GET and POST operations for the /a2a path share the
same operationId "handle_a2a_jsonrpc_a2a_post", which must be unique; update one
of them (preferably the GET operation) to a distinct id such as
"handle_a2a_jsonrpc_a2a_get" so each operationId is unique, ensuring you change
the operationId string in the OpenAPI document for the GET operation (search for
"handle_a2a_jsonrpc_a2a_post" under the GET /a2a entry and replace it) and keep
the POST operationId as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 220dc07e-ea1d-4043-ab29-20c390295f19
📒 Files selected for processing (13)
docs/ARCHITECTURE.mddocs/config.mddocs/getting_started.mddocs/openapi.jsontests/e2e/configuration/library-mode/lightspeed-stack-mcp-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-auth.yamltests/e2e/features/environment.pytests/e2e/features/mcp_servers_api.featuretests/e2e/features/mcp_servers_api_auth.featuretests/e2e/features/mcp_servers_api_no_config.featuretests/e2e/features/steps/common_http.pytests/e2e/test_list.txttests/integration/test_openapi_json.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration/test_openapi_json.py
- tests/e2e/features/steps/common_http.py
- docs/ARCHITECTURE.md
| Given The system is in default state | ||
| 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 |
There was a problem hiding this comment.
Use canonical not-found message casing in assertion.
Line 55 checks Mcp Server not found, but the API’s resource label is MCP server; this can make the assertion fail for casing rather than behavior.
✅ Proposed test fix
- And The body of the response contains Mcp Server not found
+ And The body of the response contains MCP server not found📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| And The body of the response contains Mcp Server not found | |
| And The body of the response contains MCP server not found |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/features/mcp_servers_api.feature` at line 55, The test assertion
uses the wrong casing "Mcp Server not found" which mismatches the API's resource
label; update the step text (the Gherkin line "And The body of the response
contains Mcp Server not found") to use the canonical message "MCP server not
found" so the assertion matches the API response exactly.
Assisted-by: Claude Opus 4.6 Generated-by: Cursor Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Description
The PR #1330 introduced new API endpoints for registering and unregistering MCP Servers dynamically:
This PR:
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
uv run make format: all clean
uv run make verify: all linters pass (pylint 10/10, pyright 0 errors, mypy success, pydocstyle pass, ruff pass)
uv run make test-unit: 1754 passed (1 pre-existing failure unrelated to our changes)
uv run make test-integration: 171 passed
Summary by CodeRabbit
New Features
API / OpenAPI
Documentation
Tests