Skip to content

RHIDP-12707: Update docs and add E2E tests for MCP APIs#1342

Open
maysunfaisal wants to merge 3 commits intolightspeed-core:mainfrom
maysunfaisal:RHIDP-12707-1
Open

RHIDP-12707: Update docs and add E2E tests for MCP APIs#1342
maysunfaisal wants to merge 3 commits intolightspeed-core:mainfrom
maysunfaisal:RHIDP-12707-1

Conversation

@maysunfaisal
Copy link
Contributor

@maysunfaisal maysunfaisal commented Mar 17, 2026

Description

The PR #1330 introduced new API endpoints for registering and unregistering MCP Servers dynamically:

  1. POST http://localhost:8080/v1/mcp-servers to register MCP Servers
  2. GET http://localhost:8080/v1/mcp-servers to get a list of registered MCP Servers
  3. DELETE http://localhost:8080/v1/mcp-servers/ to unregister MCP Server

This PR:

  • adds some E2E tests for the above APIs
  • Updates docs and openai schema whereever necesssary to document the above APIs

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: Claude Opus 4.6
  • Generated by: Cursor

Related Tickets & Documents

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

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

    • Dynamic MCP server management via REST API: register servers at runtime, list servers (shows source: config vs API), and delete API-registered servers (deletion disallowed for static entries; API-registered servers do not persist across restarts).
  • API / OpenAPI

    • New endpoints: POST /v1/mcp-servers, GET /v1/mcp-servers, DELETE /v1/mcp-servers/{name}; OpenAPI schemas/responses added; RBAC actions for register/list/delete.
  • Documentation

    • Config and getting-started updated with workflows, examples, per-request header guidance, and lifecycle notes.
  • Tests

    • Added end-to-end and OpenAPI integration tests covering auth, validation, lifecycle, and error cases.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f6a6f900-fa1c-4c62-9f02-bc17ac7fb158

📥 Commits

Reviewing files that changed from the base of the PR and between 1aac7b4 and d3f497c.

📒 Files selected for processing (4)
  • tests/e2e/configuration/library-mode/lightspeed-stack-no-mcp.yaml
  • tests/e2e/configuration/server-mode/lightspeed-stack-no-mcp.yaml
  • tests/e2e/features/environment.py
  • tests/e2e/features/mcp_servers_api_no_config.feature
✅ Files skipped from review due to trivial changes (3)
  • tests/e2e/configuration/server-mode/lightspeed-stack-no-mcp.yaml
  • tests/e2e/configuration/library-mode/lightspeed-stack-no-mcp.yaml
  • tests/e2e/features/mcp_servers_api_no_config.feature
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/e2e/features/environment.py

Walkthrough

Adds 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

Cohort / File(s) Summary
Documentation
docs/ARCHITECTURE.md, docs/config.md, docs/getting_started.md
Document dynamic runtime MCP server registration/management via API, updated MCP availability/config wording, added API examples and new authorization actions for MCP server management.
OpenAPI / Public API
docs/openapi.json
Added POST /v1/mcp-servers, GET /v1/mcp-servers, DELETE /v1/mcp-servers/{name}; new schemas (MCPServerRegistrationRequest/Response, MCPServerInfo, MCPServerListResponse, MCPServerDeleteResponse, ConflictResponse); added RBAC actions (register_mcp_server, list_mcp_servers, delete_mcp_server); updated two A2A operationIds.
End-to-End Tests & Configs
tests/e2e/features/mcp_servers_api.feature, tests/e2e/features/mcp_servers_api_auth.feature, tests/e2e/features/mcp_servers_api_no_config.feature, tests/e2e/test_list.txt, tests/e2e/configuration/*/lightspeed-stack-mcp-auth.yaml
Added BDD scenarios covering list/register/delete flows and error cases (401, 403, 404, 409, 422); added new test configs for MCP auth in library and server modes; registered features in test discovery.
Test Helpers & Environment
tests/e2e/features/steps/common_http.py, tests/e2e/features/environment.py
Added HTTP DELETE step helper; extended environment setup/teardown to support MCPServerAPIAuth and MCPNoConfig tags with config swaps and container restarts.
Integration Tests
tests/integration/test_openapi_json.py
Extended OpenAPI expectation tests to include the three new MCP endpoints and their expected response codes for file- and URL-based assertions.
Test Configs (no-mcp)
tests/e2e/configuration/*/lightspeed-stack-no-mcp.yaml
Added configurations for running tests with no MCP servers configured in both library and server modes.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

ok-to-test

Suggested reviewers

  • tisnik
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: documentation updates and E2E tests for MCP (Model Context Protocol) API endpoints, directly reflecting the primary additions across docs and test files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Use unique operationId values 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 each operationId must 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 the ModelContextProtocolServer section to avoid contradictory behavior docs.

This line correctly documents runtime registration, but the later ModelContextProtocolServer description 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-server is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 80d5b9e and 60cd540.

📒 Files selected for processing (9)
  • docs/ARCHITECTURE.md
  • docs/config.md
  • docs/getting_started.md
  • docs/openapi.json
  • docs/openapi.md
  • tests/e2e/features/mcp_servers_api.feature
  • tests/e2e/features/steps/common_http.py
  • tests/e2e/test_list.txt
  • tests/integration/test_openapi_json.py

Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove docs/openapi.md from this PR. The generation script is broken. I'll update it later.

Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from my perspective. Also @radofuchs PTAL (e2e part).

@tisnik tisnik requested a review from radofuchs March 17, 2026 22:48
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Use unique operationId values for /a2a GET 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 to handle_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

📥 Commits

Reviewing files that changed from the base of the PR and between 60cd540 and 402d2a8.

📒 Files selected for processing (8)
  • docs/ARCHITECTURE.md
  • docs/config.md
  • docs/getting_started.md
  • docs/openapi.json
  • tests/e2e/features/mcp_servers_api.feature
  • tests/e2e/features/steps/common_http.py
  • tests/e2e/test_list.txt
  • tests/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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@radofuchs radofuchs Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Given The service is started locally
And REST API service prefix is /v1

Scenario: List MCP servers returns pre-configured servers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also add another test when no mcp server is configured

@@ -0,0 +1,90 @@
@MCP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@maysunfaisal
Copy link
Contributor Author

@radofuchs Please take a look, thank you!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Fix duplicate operationId on /a2a GET and POST endpoints.

Both GET and POST /a2a endpoints use handle_a2a_jsonrpc_a2a_post as 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

📥 Commits

Reviewing files that changed from the base of the PR and between 402d2a8 and 1aac7b4.

📒 Files selected for processing (13)
  • docs/ARCHITECTURE.md
  • docs/config.md
  • docs/getting_started.md
  • docs/openapi.json
  • tests/e2e/configuration/library-mode/lightspeed-stack-mcp-auth.yaml
  • tests/e2e/configuration/server-mode/lightspeed-stack-mcp-auth.yaml
  • tests/e2e/features/environment.py
  • tests/e2e/features/mcp_servers_api.feature
  • tests/e2e/features/mcp_servers_api_auth.feature
  • tests/e2e/features/mcp_servers_api_no_config.feature
  • tests/e2e/features/steps/common_http.py
  • tests/e2e/test_list.txt
  • tests/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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants