implementation of the mcp-auth endpoint #1003
Conversation
|
Hi @blublinsky. Thanks for your PR. I'm waiting for a lightspeed-core member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
WalkthroughAdds a new authenticated endpoint GET /v1/mcp-auth/client-options to enumerate configured MCP servers that accept client-provided authentication headers; includes response models, router registration, unit and E2E tests, a mock MCP server and Docker config, updated compose files, test configs, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Lightspeed API
participant Auth as Authorization
participant Config as Configuration Store
Client->>API: GET /v1/mcp-auth/client-options (credentials)
API->>Auth: authorize(Action.GET_TOOLS)
Auth-->>API: authorized
API->>Config: load MCP server configurations
Config-->>API: return mcp_servers list
API->>API: filter servers where authorization_headers values == "client"
API-->>Client: 200 MCPClientAuthOptionsResponse (servers + client_auth_headers)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/e2e/features/steps/info.py`:
- Around line 186-242: The failing Black check is caused by trailing whitespace
in the new step implementations; open the functions
check_client_auth_options_structure and check_server_with_header in
tests/e2e/features/steps/info.py, remove any trailing spaces at line ends
(notably around the assertions and loops), then run the project formatter (uv
run make format or black info.py) to reformat and ensure Black passes before
committing.
🧹 Nitpick comments (3)
docker-compose.yaml (1)
2-17: Consider separating test services from production compose file.The
mcp-mock-serveris described as a mock server for testing. Having it in the maindocker-compose.yamlmeans it will be started alongside production services. Consider whether this should be in a separatedocker-compose.test.yamlordocker-compose.override.yamlto keep the production configuration lean.If this is intentional for development workflows, the current setup is acceptable.
docker-compose-library.yaml (1)
2-17: Consider reducing duplication between compose files.The
mcp-mock-serverservice definition is identical to the one indocker-compose.yaml. This duplication creates a maintenance burden where changes must be synchronized across both files.Consider using Docker Compose's
extendsfeature or extracting shared service definitions to a base file (e.g.,docker-compose.base.yaml) that both configurations can include.docs/ARCHITECTURE.md (1)
44-90: Consider adding a language identifier to the fenced code block.While ASCII diagrams don't have a standard language identifier, you could use
```textor```plaintextto satisfy the linter (MD040) and indicate this is intentional plain text content.📝 Suggested fix
-``` +```text ┌─────────────────────────────────────────────────────────┐ │ Client Applications │
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.mddocker-compose-library.yamldocker-compose.yamldocs/ARCHITECTURE.mddocs/auth.mdsrc/app/endpoints/mcp_auth.pysrc/app/routers.pysrc/models/responses.pytests/e2e/configuration/README.mdtests/e2e/configuration/library-mode/lightspeed-stack.yamltests/e2e/configuration/server-mode/lightspeed-stack.yamltests/e2e/features/info.featuretests/e2e/features/steps/info.pytests/unit/app/endpoints/test_mcp_auth.pytests/unit/app/test_routers.py
🧰 Additional context used
📓 Path-based instructions (7)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
tests/unit/app/test_routers.pysrc/models/responses.pysrc/app/endpoints/mcp_auth.pysrc/app/routers.pytests/e2e/features/steps/info.pytests/unit/app/endpoints/test_mcp_auth.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest for all unit and integration tests, not unittest
Usepytest-mockfor AsyncMock objects in tests
UseMOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")pattern for authentication mocks in tests
Files:
tests/unit/app/test_routers.pytests/e2e/features/steps/info.pytests/unit/app/endpoints/test_mcp_auth.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/**/*.py: Unit tests require 60% code coverage
Write unit tests covering new functionality before completion
Files:
tests/unit/app/test_routers.pytests/unit/app/endpoints/test_mcp_auth.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
ExtendBaseModelfor data Pydantic models
Use@model_validatorand@field_validatorfor Pydantic model validation
Files:
src/models/responses.py
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Use behave (BDD) framework with Gherkin feature files for end-to-end tests
Files:
tests/e2e/features/info.feature
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/mcp_auth.pysrc/app/routers.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define E2E test steps in
tests/e2e/features/steps/directory
Files:
tests/e2e/features/steps/info.py
🧠 Learnings (8)
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/e2e/configuration/library-mode/lightspeed-stack.yamltests/e2e/configuration/README.mdtests/e2e/configuration/server-mode/lightspeed-stack.yaml
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/responses.py
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/e2e/features/steps/**/*.py : Define E2E test steps in `tests/e2e/features/steps/` directory
Applied to files:
tests/e2e/configuration/README.md
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/e2e/test_list.txt : Maintain test list in `tests/e2e/test_list.txt`
Applied to files:
tests/e2e/configuration/README.md
📚 Learning: 2025-09-02T11:15:02.411Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.411Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.
Applied to files:
tests/e2e/configuration/README.md
📚 Learning: 2025-08-18T10:56:55.349Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:0-0
Timestamp: 2025-08-18T10:56:55.349Z
Learning: The lightspeed-stack project intentionally uses a "generic image" approach, bundling many dependencies directly in the base runtime image to work for everyone, rather than using lean base images with optional dependency groups.
Applied to files:
docker-compose-library.yaml
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/**/*.py : Use `MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")` pattern for authentication mocks in tests
Applied to files:
tests/unit/app/endpoints/test_mcp_auth.py
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to **/*.py : Use Llama Stack imports: `from llama_stack_client import AsyncLlamaStackClient`
Applied to files:
README.md
🧬 Code graph analysis (1)
src/app/endpoints/mcp_auth.py (6)
src/authentication/__init__.py (1)
get_auth_dependency(21-72)src/authorization/middleware.py (1)
authorize(122-133)src/configuration.py (2)
configuration(95-106)mcp_servers(152-163)src/models/config.py (1)
Action(817-876)src/models/responses.py (2)
InternalServerErrorResponse(1825-1983)UnauthorizedResponse(1340-1421)src/utils/endpoints.py (1)
check_configuration_loaded(124-138)
🪛 GitHub Actions: Black
tests/e2e/features/steps/info.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Run 'black --write' to fix code style issues.
🪛 markdownlint-cli2 (0.18.1)
docs/ARCHITECTURE.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
573-573: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
README.md
35-35: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-pr
🔇 Additional comments (23)
docker-compose.yaml (1)
79-83: LGTM!The dependency configuration correctly uses
condition: service_healthyto ensure proper startup ordering, consistent with the existingllama-stackdependency pattern.tests/e2e/configuration/README.md (1)
1-32: LGTM!The documentation clearly explains the e2e test configuration structure, MCP server setup for testing client-provided authentication, and special-purpose configurations. This aligns well with the new
/v1/mcp-auth/client-optionsendpoint and provides helpful context for test maintainers.docker-compose-library.yaml (1)
28-32: LGTM!The dependency configuration and network setup are correctly implemented, ensuring
mcp-mock-serveris healthy beforelightspeed-stackstarts, with proper bridge network connectivity.Also applies to: 72-74
src/models/responses.py (2)
130-137: LGTM!The
MCPServerAuthInfomodel is well-defined with proper type annotations and field descriptions. The model correctly usesField(...)for required fields with descriptive help text.
140-165: LGTM!The
MCPClientAuthOptionsResponsemodel follows the established patterns in this file:
- Extends
AbstractSuccessfulResponsefor OpenAPI integration- Uses
default_factory=listcorrectly to avoid mutable default issues- Includes comprehensive
json_schema_extraexamples for API documentationdocs/auth.md (1)
355-356: LGTM!The documentation correctly adds the new authorization actions:
get_toolsappropriately groups/toolsand/mcp-auth/client-optionsas they both relate to tool/MCP discoveryget_shieldsis properly documented as a separate actionThe descriptions are clear and follow the existing documentation format.
src/app/routers.py (1)
21-21: LGTM!The
mcp_authrouter is properly imported using absolute imports and registered with the/v1prefix, consistent with other API routers. The placement alongsidetools.routeris appropriate.Also applies to: 42-42
tests/e2e/configuration/library-mode/lightspeed-stack.yaml (1)
20-36: LGTM!The MCP server configuration provides good test coverage with:
- Two servers requiring different client-auth headers (
AuthorizationandX-API-Token)- One public server without auth for negative testing
This aligns well with the e2e test expectations in
info.feature.tests/e2e/configuration/server-mode/lightspeed-stack.yaml (1)
21-37: LGTM!The server-mode configuration mirrors the library-mode configuration, ensuring consistent test behavior across both operational modes. The MCP server definitions are correctly structured.
tests/e2e/features/info.feature (1)
131-137: LGTM!The new e2e scenario properly validates the MCP client auth options endpoint:
- Follows existing Gherkin patterns in the feature file
- Tests both positive cases (github-api, gitlab-api with their respective auth headers)
- Runs in both library and server modes (no skip tag), which is appropriate since both configurations include the same MCP servers
tests/unit/app/test_routers.py (1)
26-26: LGTM!The unit tests are correctly updated:
- Router count incremented to 19
mcp_auth.routerpresence assertion added- Prefix assertion correctly expects
/v1The changes maintain consistency with the router registration in
src/app/routers.py.Also applies to: 109-109, 114-114, 147-147, 152-152
tests/e2e/features/steps/info.py (2)
188-217: LGTM!The step definition thoroughly validates the MCP client auth options response structure, including type checking for nested fields and ensuring
client_auth_headersis a non-empty list of strings.
220-242: LGTM!The step definition correctly validates specific server/header presence with informative assertion messages that aid debugging.
docs/ARCHITECTURE.md (2)
488-491: LGTM!The API endpoint documentation for
GET /v1/mcp-auth/client-optionsis clear and accurately describes the endpoint's purpose.
1-6: LGTM!Comprehensive architecture documentation that effectively covers the system components, request pipeline, and integrates the new MCP client auth functionality.
README.md (1)
394-421: LGTM!Excellent documentation of the new MCP client auth discovery endpoint. The response example clearly shows the expected structure and the use case explanation helps developers understand when to use this endpoint.
tests/unit/app/endpoints/test_mcp_auth.py (4)
22-23: **Verify the MOCK_AUTH tuple structure.**TheMOCK_AUTHfollows the pattern from the retrieved learnings:("mock_user_id", "mock_username", False, "mock_token"). The 3rd element appears to be a boolean (not roles as a list). Based on the learnings, this pattern is correct for authentication mocks in this codebase. The comment on line 22 says "4 fields as expected by the application" which is consistent.
26-51: LGTM!The fixture provides a well-structured mock configuration for testing MCP servers with client authentication headers, including single and multiple header scenarios.
110-144: LGTM!Comprehensive test for the success case with proper assertions on response structure. Using
set()for comparinggitlab.client_auth_headersis appropriate since dictionary iteration order could vary.
243-289: Good edge case coverage for whitespace handling.This test validates that the endpoint correctly trims whitespace when matching
"client"values, ensuring" client "is recognized while"kubernetes "is not.src/app/endpoints/mcp_auth.py (3)
1-23: LGTM!Module structure follows coding guidelines: descriptive docstring, absolute imports, proper logging setup with
logging.getLogger(__name__), and appropriate FastAPI router configuration.
26-33: LGTM!The response mapping correctly covers the expected HTTP status codes with appropriate examples for authentication and authorization scenarios.
36-87: LGTM!The endpoint implementation is clean and correct:
- Appropriate authorization using
GET_TOOLSaction with clear rationale- Proper configuration validation before accessing MCP servers
- Correct filtering logic with whitespace handling via
strip()- Clear docstring following Google conventions
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
blublinsky
force-pushed
the
mcp-auth
branch
2 times, most recently
from
564cd9e to
3dfe0f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/app/test_routers.py (1)
133-142: Docstring mentions outdated router count.The docstring states "16 routers" but the test now asserts 19 routers. Consider updating the docstring to reflect the current count.
📝 Proposed fix
def test_check_prefixes() -> None: """Test the router prefixes. Verify that include_routers registers the expected routers with their configured URL prefixes. - Asserts that 16 routers are registered on a MockFastAPI instance and that + Asserts that 19 routers are registered on a MockFastAPI instance and that each router's prefix matches the expected value (e.g., root, health, authorized, metrics use an empty prefix; most API routers use "/v1"; conversations_v2 uses "/v2"). """
🤖 Fix all issues with AI agents
In `@dev-tools/mcp-mock-server/Dockerfile`:
- Around line 1-12: The Dockerfile uses python:3.12-slim but the docker-compose
healthcheck uses curl which is not present; update the Dockerfile to install
curl (e.g., run apt-get update && apt-get install -y curl
--no-install-recommends and clean apt caches) before the COPY/EXPOSE/CMD steps
so the healthcheck will succeed, or alternatively update the docker-compose
healthcheck to use a Python one-liner against the server started by CMD
["python", "server.py", "3000"] if you prefer not to install curl.
In `@src/app/endpoints/mcp_auth.py`:
- Around line 44-57: Update the function docstring to include an "Args" section
per Google Python docstring conventions documenting the request and auth
parameters: add lines under "Args:" that describe request (type, e.g., Request,
and note that it's provided by the framework/middleware) and auth (type, e.g.,
AuthCredentials or similar, and explain it's used by middleware for
authentication), keeping the existing summary and Returns intact; locate the
endpoint function in src/app/endpoints/mcp_auth.py that accepts request and auth
and insert the Args block there.
♻️ Duplicate comments (1)
tests/e2e/features/steps/info.py (1)
220-246: LGTM!The test step correctly finds a server by name and validates that the specified header exists in its
client_auth_headers. The error message helpfully includes the found headers for debugging.Ensure trailing whitespace has been removed as flagged in the previous review. Run
uv run make formatper coding guidelines if the formatter check still fails.
🧹 Nitpick comments (2)
src/app/endpoints/mcp_auth.py (1)
72-77: Document that keywords must be lowercase, or consider defensive case-insensitive comparison.The documentation for
authorization_headersexplicitly specifies lowercase keywords: "use a string 'kubernetes'" and "use a string 'client'". The current case-sensitive comparison aligns with this design. However, defensive programming could benefit from.lower()to gracefully handle misconfigured values.♻️ Optional: Use case-insensitive comparison for robustness
# Find headers with "client" value client_headers = [ header_name for header_name, header_value in mcp_server.authorization_headers.items() - if header_value.strip() == "client" + if header_value.strip().lower() == "client" ]tests/unit/app/endpoints/test_mcp_auth.py (1)
110-131: Consider extracting common test setup into a fixture.All six tests repeat the same pattern: creating
AppConfig, patching the configuration module, patching the authorize decorator, and setting up mock request/auth. A parameterized fixture or helper function could reduce duplication.♻️ Example fixture to reduce boilerplate
`@pytest.fixture` def setup_mcp_auth_test(mocker: MockerFixture): """Helper fixture for MCP auth endpoint tests.""" def _setup(config: Configuration): app_config = AppConfig() app_config._configuration = config mocker.patch("app.endpoints.mcp_auth.configuration", app_config) mocker.patch("app.endpoints.mcp_auth.authorize", lambda action: lambda func: func) mock_request = mocker.Mock() return mock_request, MOCK_AUTH return _setupThen use in tests:
async def test_get_mcp_client_auth_options_success( setup_mcp_auth_test, mock_configuration_with_client_auth: Configuration, ) -> None: mock_request, mock_auth = setup_mcp_auth_test(mock_configuration_with_client_auth) response = await mcp_auth.get_mcp_client_auth_options.__wrapped__(mock_request, mock_auth) # ... assertions
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
README.mddev-tools/mcp-mock-server/Dockerfiledocker-compose-library.yamldocker-compose.yamldocs/ARCHITECTURE.mddocs/auth.mdsrc/app/endpoints/mcp_auth.pysrc/app/routers.pysrc/models/responses.pytests/e2e/configuration/README.mdtests/e2e/configuration/library-mode/lightspeed-stack.yamltests/e2e/configuration/server-mode/lightspeed-stack.yamltests/e2e/features/info.featuretests/e2e/features/steps/info.pytests/unit/app/endpoints/test_mcp_auth.pytests/unit/app/test_routers.py
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/auth.md
- tests/e2e/configuration/README.md
- src/models/responses.py
- tests/e2e/configuration/server-mode/lightspeed-stack.yaml
🧰 Additional context used
📓 Path-based instructions (6)
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Use behave (BDD) framework with Gherkin feature files for end-to-end tests
Files:
tests/e2e/features/info.feature
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
tests/unit/app/endpoints/test_mcp_auth.pysrc/app/routers.pytests/e2e/features/steps/info.pysrc/app/endpoints/mcp_auth.pytests/unit/app/test_routers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest for all unit and integration tests, not unittest
Usepytest-mockfor AsyncMock objects in tests
UseMOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")pattern for authentication mocks in tests
Files:
tests/unit/app/endpoints/test_mcp_auth.pytests/e2e/features/steps/info.pytests/unit/app/test_routers.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/**/*.py: Unit tests require 60% code coverage
Write unit tests covering new functionality before completion
Files:
tests/unit/app/endpoints/test_mcp_auth.pytests/unit/app/test_routers.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/routers.pysrc/app/endpoints/mcp_auth.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define E2E test steps in
tests/e2e/features/steps/directory
Files:
tests/e2e/features/steps/info.py
🧠 Learnings (4)
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/e2e/configuration/library-mode/lightspeed-stack.yaml
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/**/*.py : Use `MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")` pattern for authentication mocks in tests
Applied to files:
tests/unit/app/endpoints/test_mcp_auth.py
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to **/*.py : Use Llama Stack imports: `from llama_stack_client import AsyncLlamaStackClient`
Applied to files:
README.md
📚 Learning: 2025-08-18T10:56:55.349Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:0-0
Timestamp: 2025-08-18T10:56:55.349Z
Learning: The lightspeed-stack project intentionally uses a "generic image" approach, bundling many dependencies directly in the base runtime image to work for everyone, rather than using lean base images with optional dependency groups.
Applied to files:
docker-compose-library.yaml
🧬 Code graph analysis (2)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(47-75)
src/app/endpoints/mcp_auth.py (4)
src/configuration.py (2)
configuration(95-106)mcp_servers(152-163)src/models/config.py (1)
Action(817-876)src/models/responses.py (4)
ForbiddenResponse(1424-1571)InternalServerErrorResponse(1825-1983)MCPClientAuthOptionsResponse(140-165)MCPServerAuthInfo(130-137)src/utils/endpoints.py (1)
check_configuration_loaded(124-138)
🪛 markdownlint-cli2 (0.18.1)
docs/ARCHITECTURE.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
573-573: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
README.md
35-35: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-pr
🔇 Additional comments (28)
docker-compose.yaml (2)
2-17: LGTM!The mcp-mock-server service configuration is well-structured with appropriate healthcheck settings and network attachment. The dependency chain ensures proper startup ordering.
Note: The healthcheck relies on
curlbeing available in the container - see the related comment on the Dockerfile.
82-83: LGTM!The dependency on mcp-mock-server with
service_healthycondition ensures the mock server is ready before lightspeed-stack starts, which is correct for testing scenarios.README.md (2)
394-420: LGTM!The documentation for the new
/v1/mcp-auth/client-optionsendpoint is clear and comprehensive. It accurately describes the response structure matching theMCPClientAuthOptionsResponsemodel, explains the use case, and clarifies which servers are included/excluded.
366-366: LGTM!Good clarification about the Kubernetes authentication prerequisite for MCP servers using
Authorization: "kubernetes".docker-compose-library.yaml (3)
2-17: LGTM!The mcp-mock-server service configuration mirrors
docker-compose.yamlappropriately for library mode testing. Same note applies: the healthcheck depends oncurlbeing available in the container.
28-32: LGTM!Proper dependency and network configuration for the library mode compose file.
72-74: LGTM!Bridge network definition is correct and consistent with the server mode compose file.
src/app/endpoints/mcp_auth.py (3)
1-23: LGTM!Module structure follows coding guidelines: descriptive docstring, absolute imports, proper logger pattern, and appropriate FastAPI router setup.
26-33: LGTM!OpenAPI response mapping is comprehensive, covering success (200), authentication (401), authorization (403), and configuration error (500) cases.
66-87: LGTM!The core logic correctly:
- Iterates over configured MCP servers
- Filters for servers with authorization headers containing the "client" keyword
- Builds the response with server names and their client-auth header names
The implementation aligns with the documented behavior in README.md.
src/app/routers.py (1)
21-21: LGTM!The
mcp_authmodule is correctly imported and the router is properly included with the/v1prefix, following the established pattern for other API endpoints in this file.Also applies to: 42-42
docs/ARCHITECTURE.md (3)
44-90: Well-documented architecture diagram.The ASCII diagram effectively illustrates the system overview. The markdownlint warning about missing language specification (MD040) is a false positive here since this is an ASCII diagram, not executable code.
339-347: Clear documentation of MCP client-provided authentication.The documentation correctly explains the
MCP-HEADERSmechanism and references the new/v1/mcp-auth/client-optionsdiscovery endpoint, which aligns with the implementation being added in this PR.
488-491: API endpoint documentation is accurate.The endpoint documentation for
GET /v1/mcp-auth/client-optionsaccurately describes the functionality implemented in themcp_authmodule.tests/e2e/features/info.feature (1)
131-137: Well-structured e2e test scenario.The test scenario follows the established BDD pattern in this feature file and validates both the response structure and specific server/header content. The expected servers (
github-apiwithAuthorizationandgitlab-apiwithX-API-Token) align with the configuration inlightspeed-stack.yaml.tests/unit/app/test_routers.py (3)
26-26: LGTM!Import correctly added following the established pattern.
109-114: Router count and presence assertions correctly updated.The test properly verifies that
mcp_auth.routeris registered, and the total router count is correctly incremented to 19.
147-152: Prefix verification correctly added.The test verifies that
mcp_auth.routeris registered with the/v1prefix, consistent with the implementation inrouters.py.tests/e2e/configuration/library-mode/lightspeed-stack.yaml (1)
20-36: Well-structured test configuration for MCP servers.The
mcp_serversconfiguration correctly sets up three test scenarios:
github-apiwithAuthorization: "client"- will appear in client-options responsegitlab-apiwithX-API-Token: "client"- will appear in client-options responsepublic-apiwith no auth headers - will NOT appear in responseThis aligns with the e2e test assertions in
info.featurethat expect exactly these two servers with their respective headers.tests/e2e/features/steps/info.py (1)
188-218: LGTM!The test step correctly validates the MCP client auth options response structure: checks for
serverslist, validates each server has anamestring and non-emptyclient_auth_headerslist with string entries. The implementation is thorough and aligns with the response model defined inMCPClientAuthOptionsResponse.tests/unit/app/endpoints/test_mcp_auth.py (8)
22-23: LGTM!The
MOCK_AUTHconstant correctly follows the expected pattern("mock_user_id", "mock_username", False, "mock_token")as specified in the coding guidelines for authentication mocks in tests. Based on learnings.
26-51: LGTM!The fixture correctly sets up two MCP servers with client-provided auth headers: one with a single
Authorizationheader and another with bothAuthorizationandX-API-Key. Thetype: ignore[call-arg]comments are acceptable for test fixtures.
54-107: LGTM!The
mock_configuration_mixed_authandmock_configuration_no_client_authfixtures appropriately test edge cases: filtering out servers with non-client auth values ("kubernetes") and servers without authorization headers. This provides good test coverage for the filtering logic.
110-144: LGTM!The test correctly validates the happy path: two servers with client auth are returned, with the expected header configurations. Using
__wrapped__to bypass decorators and directly test the endpoint logic is appropriate for unit testing.
146-178: LGTM!Good test coverage for the filtering logic: only servers with
"client"value in authorization headers should be returned. The assertions properly verify thatk8s_mgmtandpublic_serverare excluded whilegithubis included.
181-240: LGTM!Tests appropriately cover the empty result scenarios: when no servers have client auth and when no MCP servers are configured at all. Both should return an empty
serverslist.
243-288: LGTM!Good edge case test for whitespace handling. The test verifies that
" client "(with surrounding spaces) is correctly trimmed and recognized as client auth, while"kubernetes "(with trailing space) remains filtered out after trimming.
291-339: LGTM!Test correctly validates that a server with multiple client auth headers (
Authorization,X-API-Key,X-Custom-Token) returns all headers in the response.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
blublinsky
force-pushed
the
mcp-auth
branch
4 times, most recently
from
8094d39 to
19dc395
Compare
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)
tests/unit/app/test_routers.py (1)
133-141: Docstring contains outdated router count.The docstring states "16 routers" but the test now asserts 19 routers. Update the docstring to reflect the current count.
📝 Suggested fix
def test_check_prefixes() -> None: """Test the router prefixes. Verify that include_routers registers the expected routers with their configured URL prefixes. - Asserts that 16 routers are registered on a MockFastAPI instance and that + Asserts that 19 routers are registered on a MockFastAPI instance and that each router's prefix matches the expected value (e.g., root, health, authorized, metrics use an empty prefix; most API routers use "/v1"; conversations_v2 uses "/v2"). """
🤖 Fix all issues with AI agents
In `@docker-compose-library.yaml`:
- Around line 10-11: Replace every occurrence of the MCP server URL string
"http://localhost:3000" with "http://mcp-mock-server:3000" in the test
configuration YAMLs for library-mode and server-mode (the files containing the
MCP URL entries for lightspeed-stack). Locate the three URL entries in each
config (currently set to http://localhost:3000) and update them so
container-to-container DNS resolves to mcp-mock-server:3000.
♻️ Duplicate comments (1)
src/app/endpoints/mcp_auth.py (1)
44-57: Docstring missingArgssection.Per Google Python docstring conventions specified in coding guidelines, the function docstring should document the
requestandauthparameters.
🧹 Nitpick comments (2)
src/app/endpoints/mcp_auth.py (1)
72-77: Consider case-insensitive comparison for header value.The comparison
header_value.strip() == "client"is case-sensitive. If configuration values might vary in casing (e.g.,"Client"or"CLIENT"), this could silently exclude valid servers.♻️ Optional: case-insensitive comparison
client_headers = [ header_name for header_name, header_value in mcp_server.authorization_headers.items() - if header_value.strip() == "client" + if header_value.strip().lower() == "client" ]tests/unit/app/endpoints/test_mcp_auth.py (1)
1-339: Consider adding test for configuration-not-loaded error path.The endpoint calls
check_configuration_loaded(configuration)which raises an HTTPException if configuration isn't loaded. While not critical, a test validating this error behavior would improve coverage.💡 Optional test case for error handling
`@pytest.mark.asyncio` async def test_get_mcp_client_auth_options_config_not_loaded( mocker: MockerFixture, ) -> None: """Test that endpoint raises error when configuration is not loaded.""" # Mock configuration with no loaded config app_config = AppConfig() app_config._configuration = None mocker.patch("app.endpoints.mcp_auth.configuration", app_config) # Mock authorization decorator to bypass it mocker.patch("app.endpoints.mcp_auth.authorize", lambda action: lambda func: func) mock_request = mocker.Mock() mock_auth = MOCK_AUTH with pytest.raises(HTTPException) as exc_info: await mcp_auth.get_mcp_client_auth_options.__wrapped__( mock_request, mock_auth ) assert exc_info.value.status_code == 500
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
README.mddev-tools/mcp-mock-server/Dockerfiledocker-compose-library.yamldocker-compose.yamldocs/ARCHITECTURE.mddocs/auth.mdsrc/app/endpoints/mcp_auth.pysrc/app/routers.pysrc/models/responses.pytests/e2e/configs/run-ci.yamltests/e2e/configuration/README.mdtests/e2e/configuration/library-mode/lightspeed-stack.yamltests/e2e/configuration/server-mode/lightspeed-stack.yamltests/e2e/features/info.featuretests/e2e/features/steps/info.pytests/unit/app/endpoints/test_mcp_auth.pytests/unit/app/test_routers.py
🚧 Files skipped from review as they are similar to previous changes (8)
- README.md
- tests/e2e/configuration/server-mode/lightspeed-stack.yaml
- src/models/responses.py
- tests/e2e/features/info.feature
- dev-tools/mcp-mock-server/Dockerfile
- src/app/routers.py
- tests/e2e/features/steps/info.py
- docker-compose.yaml
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
tests/unit/app/test_routers.pytests/unit/app/endpoints/test_mcp_auth.pysrc/app/endpoints/mcp_auth.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest for all unit and integration tests, not unittest
Usepytest-mockfor AsyncMock objects in tests
UseMOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")pattern for authentication mocks in tests
Files:
tests/unit/app/test_routers.pytests/unit/app/endpoints/test_mcp_auth.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/**/*.py: Unit tests require 60% code coverage
Write unit tests covering new functionality before completion
Files:
tests/unit/app/test_routers.pytests/unit/app/endpoints/test_mcp_auth.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/mcp_auth.py
🧠 Learnings (6)
📚 Learning: 2025-08-18T10:56:55.349Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:0-0
Timestamp: 2025-08-18T10:56:55.349Z
Learning: The lightspeed-stack project intentionally uses a "generic image" approach, bundling many dependencies directly in the base runtime image to work for everyone, rather than using lean base images with optional dependency groups.
Applied to files:
docker-compose-library.yaml
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/e2e/configuration/library-mode/lightspeed-stack.yamltests/e2e/configuration/README.md
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/**/*.py : Use `MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")` pattern for authentication mocks in tests
Applied to files:
tests/unit/app/endpoints/test_mcp_auth.py
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/e2e/test_list.txt : Maintain test list in `tests/e2e/test_list.txt`
Applied to files:
tests/e2e/configuration/README.md
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/e2e/features/steps/**/*.py : Define E2E test steps in `tests/e2e/features/steps/` directory
Applied to files:
tests/e2e/configuration/README.md
📚 Learning: 2025-09-02T11:15:02.411Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.411Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.
Applied to files:
tests/e2e/configuration/README.md
🧬 Code graph analysis (2)
tests/unit/app/endpoints/test_mcp_auth.py (4)
src/configuration.py (3)
configuration(95-106)AppConfig(41-342)mcp_servers(152-163)src/models/config.py (3)
Configuration(1537-1698)ModelContextProtocolServer(436-518)ServiceConfiguration(350-433)src/models/responses.py (1)
MCPClientAuthOptionsResponse(140-165)src/app/endpoints/mcp_auth.py (1)
get_mcp_client_auth_options(40-87)
src/app/endpoints/mcp_auth.py (6)
src/authentication/__init__.py (1)
get_auth_dependency(21-72)src/authorization/middleware.py (1)
authorize(122-133)src/configuration.py (2)
configuration(95-106)mcp_servers(152-163)src/models/config.py (1)
Action(817-876)src/models/responses.py (9)
ForbiddenResponse(1424-1571)InternalServerErrorResponse(1825-1983)MCPClientAuthOptionsResponse(140-165)MCPServerAuthInfo(130-137)UnauthorizedResponse(1340-1421)openapi_response(47-60)openapi_response(501-523)openapi_response(905-944)openapi_response(1256-1301)src/utils/endpoints.py (1)
check_configuration_loaded(124-138)
🪛 markdownlint-cli2 (0.18.1)
docs/ARCHITECTURE.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
573-573: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (20)
src/app/endpoints/mcp_auth.py (2)
1-23: LGTM!Module setup follows coding guidelines: absolute imports, FastAPI dependencies, standard logger pattern, and descriptive module docstring.
26-33: LGTM!OpenAPI response mapping properly defines 200, 401, 403, and 500 responses with appropriate example selections.
docs/auth.md (1)
355-356: LGTM!Documentation accurately reflects the new action scopes. The
get_toolsaction correctly documents coverage of both/toolsand/mcp-auth/client-optionsendpoints, consistent with the@authorize(Action.GET_TOOLS)decorator in the implementation.tests/unit/app/test_routers.py (3)
26-26: LGTM!Import correctly added following the existing pattern.
109-114: LGTM!Router count and assertion correctly updated to include the new
mcp_authrouter.
147-152: LGTM!Router count and prefix assertion correctly updated for the new
mcp_authrouter with expected/v1prefix.tests/e2e/configs/run-ci.yaml (1)
68-70: LGTM!MCP provider configuration follows the existing pattern with appropriate provider type and empty config. The inline comment clearly documents the purpose.
docs/ARCHITECTURE.md (3)
44-90: ASCII diagram is appropriately formatted.The static analysis hint about missing language specifier (MD040) is a false positive here - ASCII art diagrams should not have a language identifier as they are plain text representations. The diagram clearly illustrates the system architecture.
338-343: Good documentation of the new MCP client-auth discovery feature.The documentation accurately describes the client-provided authentication mechanism and references the new endpoint
GET /v1/mcp-auth/client-optionsfor discovering which servers accept client authentication.
488-491: Endpoint documentation aligns with implementation.The documented endpoint path
/v1/mcp-auth/client-optionsand its described behavior match the implementation insrc/app/endpoints/mcp_auth.py.tests/e2e/configuration/library-mode/lightspeed-stack.yaml (1)
20-36: Well-structured test configuration for MCP client-auth discovery.The configuration effectively tests three scenarios:
- Server with
Authorization: "client"→ should appear in response- Server with
X-API-Token: "client"→ should appear with different header name- Server with no auth headers → should be filtered out
The inline comments clearly document expected behavior for each server.
tests/unit/app/endpoints/test_mcp_auth.py (6)
22-23: MOCK_AUTH follows the expected pattern.The mock authentication tuple correctly follows the project's testing pattern with 4 fields:
(user_id, username, roles, token). Based on learnings, this aligns with the expectedMOCK_AUTHpattern.
26-51: Fixture provides good test coverage for client-auth scenarios.The
mock_configuration_with_client_authfixture correctly sets up two servers:
githubwith singleAuthorization: "client"headergitlabwith multiple client auth headers (AuthorizationandX-API-Key)This enables testing both single and multiple header scenarios.
110-144: Comprehensive test for successful client-auth retrieval.The test correctly:
- Mocks the configuration with client-auth servers
- Bypasses the authorization decorator for unit testing
- Validates response type, server count, and header contents
- Uses
set()comparison for unordered header verification (line 143)
146-178: Good test for filtering logic with mixed authentication types.The test validates that only servers with
"client"auth value are returned, while"kubernetes"and servers without auth headers are correctly filtered out.
243-288: Important edge case: whitespace handling test.This test validates that the endpoint correctly trims whitespace from authorization header values (e.g.,
" client "matches"client"). This aligns with theheader_value.strip() == "client"logic in the endpoint implementation.
291-339: Good coverage for multiple client-auth headers per server.The test validates that a single server can expose multiple client-auth headers (
Authorization,X-API-Key,X-Custom-Token) and all are correctly returned in the response.docker-compose-library.yaml (3)
2-17: Well-configured mock MCP server service for testing.The service configuration follows good practices:
- Proper build context pointing to
dev-tools/mcp-mock-server/Dockerfile- Healthcheck with reasonable intervals (5s) and retries (3)
- Attached to dedicated network for container communication
28-32: Proper service dependency configuration.Using
condition: service_healthyensures the mock MCP server is fully ready beforelightspeed-stackstarts, preventing race conditions during initialization.
73-75: Bridge network enables proper service discovery.The
lightspeednetbridge network allowslightspeed-stackto communicate withmcp-mock-serverusing container names as hostnames.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/app/test_routers.py (1)
133-152: Docstring mentions outdated router count.The docstring on lines 138-139 states "16 routers are registered" but the actual assertion is for 19 routers. This pre-existing inconsistency should be corrected.
📝 Suggested fix
def test_check_prefixes() -> None: """Test the router prefixes. - Verify that include_routers registers the expected routers with their configured URL prefixes. - - Asserts that 16 routers are registered on a MockFastAPI instance and that + Verify that include_routers registers the expected routers with their + configured URL prefixes. + + Asserts that 19 routers are registered on a MockFastAPI instance and that each router's prefix matches the expected value (e.g., root, health, authorized, metrics use an empty prefix; most API routers use "/v1"; conversations_v2 uses "/v2"). """
♻️ Duplicate comments (1)
src/app/endpoints/mcp_auth.py (1)
44-62: Docstring missingArgssection.The docstring should include an
Argssection per Google Python docstring conventions. Additionally, consider using_directly in the function signature for unused parameters instead of explicit assignment.♻️ Suggested improvements
`@router.get`("/client-options", responses=mcp_auth_responses) `@authorize`( Action.GET_TOOLS ) # Uses GET_TOOLS: discovering client auth is related to tool discovery async def get_mcp_client_auth_options( - request: Request, - auth: Annotated[AuthTuple, Depends(get_auth_dependency())], + _request: Request, # Required by middleware + _auth: Annotated[AuthTuple, Depends(get_auth_dependency())], # Required by middleware ) -> MCPClientAuthOptionsResponse: """ Get MCP servers that accept client-provided authorization. Returns a list of MCP servers configured to accept client-provided authorization tokens, along with the header names where clients should provide these tokens. This endpoint helps clients discover which MCP servers they can authenticate with using their own tokens. + Args: + _request: The incoming HTTP request (used by middleware). + _auth: Authentication tuple from the auth dependency (used by middleware). + Returns: MCPClientAuthOptionsResponse: List of MCP servers and their accepted client authentication headers. """ - # Used only by the middleware - _ = auth - - # Nothing interesting in the request - _ = request - check_configuration_loaded(configuration)
🧹 Nitpick comments (1)
docs/ARCHITECTURE.md (1)
44-90: Consider adding a language specifier to the code block.The ASCII diagram is rendered correctly, but adding a language identifier (e.g.,
textorplaintext) would satisfy markdown linters and improve accessibility for screen readers.-``` +```text ┌─────────────────────────────────────────────────────────┐
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
README.mddev-tools/mcp-mock-server/Dockerfiledocker-compose-library.yamldocker-compose.yamldocs/ARCHITECTURE.mddocs/auth.mdsrc/app/endpoints/mcp_auth.pysrc/app/routers.pysrc/models/responses.pytests/e2e/configs/run-ci.yamltests/e2e/configuration/README.mdtests/e2e/configuration/library-mode/lightspeed-stack.yamltests/e2e/configuration/server-mode/lightspeed-stack.yamltests/e2e/features/info.featuretests/e2e/features/steps/info.pytests/unit/app/endpoints/test_mcp_auth.pytests/unit/app/test_routers.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/e2e/configuration/README.md
- tests/e2e/configuration/library-mode/lightspeed-stack.yaml
- tests/e2e/configuration/server-mode/lightspeed-stack.yaml
- src/models/responses.py
- dev-tools/mcp-mock-server/Dockerfile
🧰 Additional context used
📓 Path-based instructions (6)
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Use behave (BDD) framework with Gherkin feature files for end-to-end tests
Files:
tests/e2e/features/info.feature
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
tests/unit/app/endpoints/test_mcp_auth.pysrc/app/routers.pytests/e2e/features/steps/info.pytests/unit/app/test_routers.pysrc/app/endpoints/mcp_auth.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest for all unit and integration tests, not unittest
Usepytest-mockfor AsyncMock objects in tests
UseMOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")pattern for authentication mocks in tests
Files:
tests/unit/app/endpoints/test_mcp_auth.pytests/e2e/features/steps/info.pytests/unit/app/test_routers.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/**/*.py: Unit tests require 60% code coverage
Write unit tests covering new functionality before completion
Files:
tests/unit/app/endpoints/test_mcp_auth.pytests/unit/app/test_routers.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/routers.pysrc/app/endpoints/mcp_auth.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define E2E test steps in
tests/e2e/features/steps/directory
Files:
tests/e2e/features/steps/info.py
🧠 Learnings (3)
📚 Learning: 2025-12-18T10:21:09.038Z
Learnt from: are-ces
Repo: lightspeed-core/lightspeed-stack PR: 935
File: run.yaml:114-115
Timestamp: 2025-12-18T10:21:09.038Z
Learning: In Llama Stack version 0.3.x, telemetry provider configuration is not supported under the `providers` section in run.yaml configuration files. Telemetry can be enabled with just `telemetry.enabled: true` without requiring an explicit provider block.
Applied to files:
tests/e2e/configs/run-ci.yaml
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/**/*.py : Use `MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")` pattern for authentication mocks in tests
Applied to files:
tests/unit/app/endpoints/test_mcp_auth.py
📚 Learning: 2025-08-18T10:56:55.349Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:0-0
Timestamp: 2025-08-18T10:56:55.349Z
Learning: The lightspeed-stack project intentionally uses a "generic image" approach, bundling many dependencies directly in the base runtime image to work for everyone, rather than using lean base images with optional dependency groups.
Applied to files:
docker-compose-library.yaml
🧬 Code graph analysis (3)
tests/unit/app/endpoints/test_mcp_auth.py (4)
src/configuration.py (3)
configuration(95-106)AppConfig(41-342)mcp_servers(152-163)src/models/config.py (4)
Configuration(1537-1698)ModelContextProtocolServer(436-518)ServiceConfiguration(350-433)UserDataCollection(615-680)src/models/responses.py (1)
MCPClientAuthOptionsResponse(140-165)src/app/endpoints/mcp_auth.py (1)
get_mcp_client_auth_options(40-87)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(47-75)
src/app/endpoints/mcp_auth.py (5)
src/authentication/__init__.py (1)
get_auth_dependency(21-72)src/configuration.py (2)
configuration(95-106)mcp_servers(152-163)src/models/config.py (1)
Action(817-876)src/models/responses.py (5)
ForbiddenResponse(1424-1571)InternalServerErrorResponse(1825-1983)MCPClientAuthOptionsResponse(140-165)MCPServerAuthInfo(130-137)UnauthorizedResponse(1340-1421)src/utils/endpoints.py (1)
check_configuration_loaded(124-138)
🪛 markdownlint-cli2 (0.18.1)
docs/ARCHITECTURE.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
573-573: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (26)
tests/e2e/configs/run-ci.yaml (1)
68-70: LGTM!The new MCP provider configuration follows the established pattern used by other providers in this file (e.g.,
rag-runtime). The empty config block and provider naming convention are consistent.docs/auth.md (1)
355-356: LGTM!The documentation correctly reflects that
get_toolsnow also covers the new/mcp-auth/client-optionsendpoint, which aligns with the@authorize(Action.GET_TOOLS)decorator used inmcp_auth.py. The addition ofget_shieldscompletes the action catalog.tests/unit/app/test_routers.py (2)
26-26: LGTM!The import follows the established pattern for endpoint modules.
109-114: LGTM!Router count correctly updated and assertion for
mcp_auth.routerproperly added.docker-compose.yaml (2)
83-84: LGTM!The dependency ensures
lightspeed-stackwaits formcp-mock-serverto be healthy before starting, which is the correct startup ordering for testing the MCP auth functionality.
2-17: LGTM!The mock MCP server service follows the established patterns in this compose file. The healthcheck configuration with appropriate intervals and the network attachment are well-configured. Curl is explicitly installed in the Dockerfile for the healthcheck, and the service dependency ordering in
lightspeed-stackis correct withcondition: service_healthy.src/app/endpoints/mcp_auth.py (3)
1-23: LGTM!Module setup follows the coding guidelines correctly: descriptive docstring, absolute imports for internal modules, FastAPI dependencies, and the standard logger pattern.
26-33: LGTM!The OpenAPI response mapping follows the established pattern from other endpoints and correctly documents the expected HTTP responses (200, 401, 403, 500).
64-87: LGTM!The business logic is clear and correct:
- Validates configuration is loaded
- Iterates over MCP servers and filters those with
authorization_headers- Selects headers where value is
"client"(with appropriate whitespace handling viastrip())- Returns properly structured response
The implementation cleanly supports the MCP client-auth discovery use case.
src/app/routers.py (1)
21-21: LGTM!The
mcp_authimport and router registration follow the established patterns in this file. The router is correctly placed among other/v1prefixed endpoints.Also applies to: 42-42
docs/ARCHITECTURE.md (1)
339-347: LGTM!The MCP client-provided authentication documentation clearly explains the discovery endpoint and its purpose. The endpoint path
/v1/mcp-auth/client-optionsis consistent with the router registration.README.md (2)
421-448: LGTM!The Client-Authenticated MCP Servers Discovery section provides clear documentation with a practical example. The endpoint path and response structure are well-documented for API consumers.
393-393: LGTM!The clarification about Kubernetes authentication requirements for MCP servers is important for users to understand the automatic server filtering behavior.
docker-compose-library.yaml (2)
2-17: LGTM on the service definition!The mcp-mock-server service is well-configured with proper healthcheck, network attachment, and build context. The healthcheck correctly uses
localhost:3000since it runs inside the mcp-mock-server container itself.
28-32: Network configuration looks correct.The
depends_onwithservice_healthycondition ensures proper startup order, and the sharedlightspeednetbridge network enables DNS-based container-to-container communication using service names.Also applies to: 73-75
tests/e2e/features/info.feature (1)
131-137: LGTM on the test scenario structure!The BDD test scenario follows the established pattern in this feature file. The test validates the endpoint returns proper status and structure, and checks for specific MCP servers with their expected client auth headers.
The test assertions are confirmed by the test configuration files:
github-apiis configured withAuthorization: "client"in both library-mode and server-modegitlab-apiis configured withX-API-Token: "client"in both library-mode and server-modetests/e2e/features/steps/info.py (2)
188-218: LGTM! Well-structured validation for MCP client auth options response.The step function correctly validates the response structure by checking:
- Presence and type of
serversfield- Required fields (
name,client_auth_headers) on each server entry- Type validation for all nested fields including individual headers
The implementation follows the existing patterns in this file and aligns with the
MCPClientAuthOptionsResponsemodel defined insrc/models/responses.py.
220-246: LGTM! Clear and focused server/header lookup step.The function correctly:
- Locates a server by name using iteration
- Validates the presence of a specific header in
client_auth_headers- Provides informative assertion messages with context (found headers list)
This aligns well with the E2E test scenarios that validate specific server configurations like
github-apiwithAuthorizationheader.tests/unit/app/endpoints/test_mcp_auth.py (8)
1-24: LGTM! Proper module setup with appropriate suppressions.The module correctly:
- Disables
protected-accesslint for testing internal state- Suppresses
reportCallIssuefor Pydantic model instantiation withtype: ignore- Uses the
MOCK_AUTHpattern as specified in the coding guidelines- Has a descriptive module docstring
Based on learnings, the
MOCK_AUTHtuple follows the expected pattern.
26-51: LGTM! Well-structured fixture for client auth testing.The fixture creates a realistic configuration with two MCP servers demonstrating:
- Single client auth header (
githubwithAuthorization)- Multiple client auth headers (
gitlabwithAuthorizationandX-API-Key)This aligns with the JSON schema example in
src/models/responses.pywhich shows the same structure.
54-107: LGTM! Comprehensive fixtures covering edge cases.The
mock_configuration_mixed_authandmock_configuration_no_client_authfixtures properly test:
- Filtering behavior (only servers with
"client"value should be returned)- Servers with
"kubernetes"auth type being excluded- Servers without authorization headers being excluded
110-144: LGTM! Comprehensive success case test.The test correctly:
- Sets up mocked configuration via
AppConfig._configuration- Patches the
authorizedecorator to bypass authorization- Calls the endpoint via
__wrapped__to skip FastAPI dependency injection- Validates response type and content including both servers and their headers
146-178: LGTM! Good coverage of filtering behavior.The test validates that only servers with
"client"auth type are returned, properly excluding:
k8s_mgmt(uses"kubernetes")public_server(no authorization headers)
181-241: LGTM! Edge cases properly covered.Both
test_get_mcp_client_auth_options_no_client_authandtest_get_mcp_client_auth_options_empty_configcorrectly validate that an emptyserverslist is returned when no servers support client authentication.
243-289: LGTM! Whitespace handling test validates implementation detail.This test confirms the endpoint correctly handles authorization header values with surrounding whitespace (e.g.,
" client ") by stripping before comparison, as documented insrc/app/endpoints/mcp_auth.pywhich uses.strip().
291-339: LGTM! Multiple headers test validates complete header collection.The test confirms that all headers with
"client"value are collected and returned, validating the iteration logic in the endpoint implementation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/app/test_routers.py (1)
133-152: Stale docstring mentions incorrect router count.The docstring on lines 138-141 states "16 routers are registered" but the actual assertion on line 147 checks for 19 routers. Consider updating the docstring to reflect the current count.
📝 Suggested fix
def test_check_prefixes() -> None: """Test the router prefixes. Verify that include_routers registers the expected routers with their configured URL prefixes. - Asserts that 16 routers are registered on a MockFastAPI instance and that + Asserts that 19 routers are registered on a MockFastAPI instance and that each router's prefix matches the expected value (e.g., root, health, authorized, metrics use an empty prefix; most API routers use "/v1"; conversations_v2 uses "/v2"). """
♻️ Duplicate comments (1)
src/app/endpoints/mcp_auth.py (1)
44-57: Docstring missingArgssection.Per Google Python docstring conventions, the function docstring should include an
Argssection documenting therequestandauthparameters. Based on coding guidelines, all functions require docstrings with complete sections.
🧹 Nitpick comments (1)
src/app/endpoints/mcp_auth.py (1)
73-77: Consider case-insensitive comparison for header value.The
header_value.strip() == "client"comparison is case-sensitive. If configuration files use variations like"Client"or"CLIENT", they won't be matched. Consider using a case-insensitive comparison for robustness.♻️ Suggested fix
client_headers = [ header_name for header_name, header_value in mcp_server.authorization_headers.items() - if header_value.strip() == "client" + if header_value.strip().lower() == "client" ]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
README.mddev-tools/mcp-mock-server/Dockerfiledocker-compose-library.yamldocker-compose.yamldocs/ARCHITECTURE.mddocs/auth.mdsrc/app/endpoints/mcp_auth.pysrc/app/routers.pysrc/models/responses.pytests/e2e/configs/run-ci.yamltests/e2e/configuration/README.mdtests/e2e/configuration/library-mode/lightspeed-stack.yamltests/e2e/configuration/server-mode/lightspeed-stack.yamltests/e2e/features/info.featuretests/e2e/features/steps/info.pytests/unit/app/endpoints/test_mcp_auth.pytests/unit/app/test_routers.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/configuration/README.md
- tests/e2e/configuration/library-mode/lightspeed-stack.yaml
- tests/e2e/configs/run-ci.yaml
- tests/e2e/features/info.feature
- docs/auth.md
- src/app/routers.py
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
src/app/endpoints/mcp_auth.pytests/unit/app/test_routers.pysrc/models/responses.pytests/unit/app/endpoints/test_mcp_auth.pytests/e2e/features/steps/info.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/mcp_auth.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest for all unit and integration tests, not unittest
Usepytest-mockfor AsyncMock objects in tests
UseMOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")pattern for authentication mocks in tests
Files:
tests/unit/app/test_routers.pytests/unit/app/endpoints/test_mcp_auth.pytests/e2e/features/steps/info.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/**/*.py: Unit tests require 60% code coverage
Write unit tests covering new functionality before completion
Files:
tests/unit/app/test_routers.pytests/unit/app/endpoints/test_mcp_auth.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
ExtendBaseModelfor data Pydantic models
Use@model_validatorand@field_validatorfor Pydantic model validation
Files:
src/models/responses.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define E2E test steps in
tests/e2e/features/steps/directory
Files:
tests/e2e/features/steps/info.py
🧠 Learnings (4)
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/e2e/configuration/server-mode/lightspeed-stack.yaml
📚 Learning: 2025-08-18T10:56:55.349Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:0-0
Timestamp: 2025-08-18T10:56:55.349Z
Learning: The lightspeed-stack project intentionally uses a "generic image" approach, bundling many dependencies directly in the base runtime image to work for everyone, rather than using lean base images with optional dependency groups.
Applied to files:
docker-compose-library.yaml
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/responses.py
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/**/*.py : Use `MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")` pattern for authentication mocks in tests
Applied to files:
tests/unit/app/endpoints/test_mcp_auth.py
🧬 Code graph analysis (2)
src/app/endpoints/mcp_auth.py (5)
src/authentication/__init__.py (1)
get_auth_dependency(21-72)src/configuration.py (2)
configuration(95-106)mcp_servers(152-163)src/models/config.py (1)
Action(817-876)src/models/responses.py (4)
InternalServerErrorResponse(1825-1983)MCPClientAuthOptionsResponse(140-165)MCPServerAuthInfo(130-137)UnauthorizedResponse(1340-1421)src/utils/endpoints.py (1)
check_configuration_loaded(124-138)
tests/unit/app/endpoints/test_mcp_auth.py (4)
src/configuration.py (2)
configuration(95-106)mcp_servers(152-163)src/models/config.py (2)
Configuration(1551-1713)ModelContextProtocolServer(436-518)src/models/responses.py (1)
MCPClientAuthOptionsResponse(140-165)src/app/endpoints/mcp_auth.py (1)
get_mcp_client_auth_options(40-87)
🪛 markdownlint-cli2 (0.18.1)
docs/ARCHITECTURE.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
573-573: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (20)
dev-tools/mcp-mock-server/Dockerfile (1)
1-15: LGTM! Well-structured Dockerfile with proper cleanup.The Dockerfile correctly installs
curlfor healthcheck support with apt cache cleanup to minimize image size. The implementation addresses the previous review feedback.tests/e2e/configuration/server-mode/lightspeed-stack.yaml (1)
21-37: LGTM! Well-documented MCP server test configuration.The configuration correctly:
- Uses container DNS name (
mcp-mock-server:3000) for container-to-container communication- Tests three auth scenarios: standard
Authorizationheader, customX-API-Tokenheader, and no auth- Includes helpful inline comments explaining expected endpoint behavior
docker-compose-library.yaml (3)
2-17: LGTM! Properly configured mock MCP server service.The service configuration is well-structured with:
- Correct healthcheck using
localhost:3000(appropriate since it runs inside the container checking itself)- Proper network attachment for container-to-container communication
- Reasonable healthcheck intervals and retry settings
28-32: LGTM! Proper dependency configuration.The
depends_onwithcondition: service_healthyensureslightspeed-stackonly starts aftermcp-mock-serveris healthy, preventing race conditions during startup.
76-78: LGTM! Bridge network for service communication.The
lightspeednetbridge network enables DNS-based container-to-container communication.docker-compose.yaml (2)
2-17: LGTM! Consistent mock server configuration.The
mcp-mock-serverservice configuration mirrorsdocker-compose-library.yaml, maintaining consistency across environments.
89-90: LGTM! Extended dependency for lightspeed-stack.The service now correctly waits for both
llama-stackandmcp-mock-serverto be healthy before starting.tests/unit/app/test_routers.py (2)
26-26: LGTM! Proper import addition for mcp_auth module.
109-114: LGTM! Test correctly verifies mcp_auth router registration.The router count increment and assertion are consistent with the new endpoint addition.
docs/ARCHITECTURE.md (1)
1-573: LGTM! Comprehensive architecture documentation.This new architecture document provides excellent coverage of the system components, including the newly added MCP client-auth discovery endpoint at
/v1/mcp-auth/client-options(line 343, 488-491). The document is well-structured with clear explanations of request processing, authentication, and MCP integration.Regarding static analysis hints:
- MD040 (line 44): The fenced code block contains an ASCII diagram, not executable code. Adding a language specifier would be misleading—this is acceptable as-is.
- MD036 (line 573): The closing emphasis is intentional styling for document closure, not a structural heading.
tests/e2e/features/steps/info.py (1)
183-241: LGTM! Well-structured E2E test steps.The new BDD step definitions follow the existing patterns in the file and provide comprehensive validation:
check_client_auth_options_structurevalidates the response schema with proper type checks and ensuresclient_auth_headersis non-empty.check_server_with_headervalidates specific server/header presence with clear assertion messages.Both functions have proper type annotations, docstrings, and meaningful error messages that will help debug test failures.
README.md (1)
421-448: LGTM! Clear and comprehensive endpoint documentation.The new "Client-Authenticated MCP Servers Discovery" section effectively documents the
/v1/mcp-auth/client-optionsendpoint with:
- Clear endpoint specification
- Well-formatted JSON response example matching
MCPClientAuthOptionsResponse- Explanation of filtering behavior (only
authorization_headers: "client"servers)- Practical use-case guidance for client integration
This documentation aligns well with the implementation and architecture docs.
src/models/responses.py (1)
130-166: LGTM! Well-designed response models.The new models follow established patterns in the file:
MCPServerAuthInfocorrectly extendsBaseModelas a nested data structure with required fields and proper Field descriptions.MCPClientAuthOptionsResponseproperly extendsAbstractSuccessfulResponsewithdefault_factory=listfor safe initialization and comprehensivejson_schema_extraexamples for OpenAPI documentation.Both models have complete type annotations and descriptive docstrings as per coding guidelines.
tests/unit/app/endpoints/test_mcp_auth.py (2)
1-51: LGTM! Comprehensive test suite setup.The test module correctly follows coding guidelines:
- Uses
pytestwithpytest-mock(not unittest)MOCK_AUTHtuple follows the expected 4-field pattern as per coding guidelines- Fixtures create realistic
Configurationobjects with proper MCP server configurations# type: ignore[call-arg]comments appropriately handle Pydantic validation for test mocks
110-339: LGTM! Thorough test coverage.The test cases provide excellent coverage:
Test Scenario test_get_mcp_client_auth_options_successMultiple servers with client auth test_get_mcp_client_auth_options_mixed_authFilters out non-client auth servers test_get_mcp_client_auth_options_no_client_authReturns empty when no client auth test_get_mcp_client_auth_options_empty_configHandles empty MCP server list test_get_mcp_client_auth_options_whitespace_handlingTests " client "value strippingtest_get_mcp_client_auth_options_multiple_headers_single_serverMultiple headers per server The use of
__wrapped__to bypass decorators and consistent mocking patterns (AppConfig wrapper, configuration patch, authorize bypass) demonstrates good unit testing practices.src/app/endpoints/mcp_auth.py (5)
1-21: LGTM!Module docstring is present and descriptive. All imports use absolute paths as required, and FastAPI dependencies are correctly imported.
22-23: LGTM!Logger follows the required
logging.getLogger(__name__)pattern, and router configuration is appropriate.
26-33: LGTM!Response mapping properly covers success and error cases with appropriate OpenAPI response schemas. Type annotation uses modern union syntax as per guidelines.
58-62: Unused parameters pattern is acceptable but consider removing if not needed by middleware.The explicit
_ = authand_ = requestassignments clarify that these parameters are required by the framework but unused in the function body. This is acceptable, though you could also use an underscore prefix in the parameter names (_request,_auth) to signal intent more idiomatically.
64-87: LGTM!The configuration check and iteration logic are well-structured. The endpoint correctly:
- Validates configuration is loaded before proceeding
- Filters MCP servers to only those with client-provided authorization headers
- Returns a properly typed response
The logic cleanly identifies servers accepting client tokens by checking for
"client"values in authorization headers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jrobertboos
left a comment
jrobertboos
left a comment
There was a problem hiding this comment.
Overall looks good and should work for the flow that you previously implemented, still a little confused by the choice of direction for this auth implementation though, but I think as long as it works for now its fine as it can be modified at a later date.
| ``` | ||
|
|
||
| The user's Kubernetes token is extracted from the incoming request's `Authorization` header and forwarded to the MCP server. | ||
| **Note:** Kubernetes token-based MCP authorization only works when Lightspeed Core Stack is configured with Kubernetes authentication (`authentication.k8s`). For any other authentication types, MCP servers configured with `Authorization: "kubernetes"` are removed from the available MCP servers list. |
There was a problem hiding this comment.
I am not sure where this occurs? is this feature in this PR? Also if it is then these MCP servers that are invalid should be logged, also make sure that this reflected in the /tools endpoint.
There was a problem hiding this comment.
Oh, I implemented it in previous PR. Just added documentation here
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
README.mddev-tools/mcp-mock-server/Dockerfiledocker-compose-library.yamldocker-compose.yamldocs/ARCHITECTURE.mddocs/auth.mdsrc/app/endpoints/mcp_auth.pysrc/app/routers.pysrc/models/responses.pytests/e2e/configs/run-ci.yamltests/e2e/configuration/README.mdtests/e2e/configuration/library-mode/lightspeed-stack.yamltests/e2e/configuration/server-mode/lightspeed-stack.yamltests/e2e/features/info.featuretests/e2e/features/steps/info.pytests/unit/app/endpoints/test_mcp_auth.pytests/unit/app/test_routers.py
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/auth.md
- dev-tools/mcp-mock-server/Dockerfile
- tests/unit/app/test_routers.py
- tests/e2e/configuration/server-mode/lightspeed-stack.yaml
🧰 Additional context used
📓 Path-based instructions (7)
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Use behave (BDD) framework with Gherkin feature files for end-to-end tests
Files:
tests/e2e/features/info.feature
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
tests/unit/app/endpoints/test_mcp_auth.pytests/e2e/features/steps/info.pysrc/models/responses.pysrc/app/routers.pysrc/app/endpoints/mcp_auth.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest for all unit and integration tests, not unittest
Usepytest-mockfor AsyncMock objects in tests
UseMOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")pattern for authentication mocks in tests
Files:
tests/unit/app/endpoints/test_mcp_auth.pytests/e2e/features/steps/info.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/**/*.py: Unit tests require 60% code coverage
Write unit tests covering new functionality before completion
Files:
tests/unit/app/endpoints/test_mcp_auth.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define E2E test steps in
tests/e2e/features/steps/directory
Files:
tests/e2e/features/steps/info.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
ExtendBaseModelfor data Pydantic models
Use@model_validatorand@field_validatorfor Pydantic model validation
Files:
src/models/responses.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/routers.pysrc/app/endpoints/mcp_auth.py
🧠 Learnings (7)
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/e2e/test_list.txt : Maintain test list in `tests/e2e/test_list.txt`
Applied to files:
tests/e2e/configuration/README.md
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/e2e/configuration/README.mdtests/e2e/configuration/library-mode/lightspeed-stack.yaml
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/e2e/features/steps/**/*.py : Define E2E test steps in `tests/e2e/features/steps/` directory
Applied to files:
tests/e2e/configuration/README.md
📚 Learning: 2025-09-02T11:15:02.411Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.411Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.
Applied to files:
tests/e2e/configuration/README.mdtests/e2e/configuration/library-mode/lightspeed-stack.yaml
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/**/*.py : Use `MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")` pattern for authentication mocks in tests
Applied to files:
tests/unit/app/endpoints/test_mcp_auth.py
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/responses.py
📚 Learning: 2025-08-18T10:56:55.349Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:0-0
Timestamp: 2025-08-18T10:56:55.349Z
Learning: The lightspeed-stack project intentionally uses a "generic image" approach, bundling many dependencies directly in the base runtime image to work for everyone, rather than using lean base images with optional dependency groups.
Applied to files:
docker-compose-library.yaml
🧬 Code graph analysis (1)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(47-75)
🪛 LanguageTool
docs/ARCHITECTURE.md
[grammar] ~339-~339: Ensure spelling is correct
Context: ... Client-Provided Authentication (MCP-HEADERS): Clients can provide their own authentica...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/ARCHITECTURE.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
573-573: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (24)
tests/e2e/configuration/library-mode/lightspeed-stack.yaml (1)
20-36: LGTM!The MCP servers configuration is well-structured with clear inline comments explaining the expected behavior for each server type. The URLs correctly use the Docker container name (
mcp-mock-server:3000) for proper DNS resolution within the Docker bridge network.docs/ARCHITECTURE.md (1)
1-573: Comprehensive architecture documentation.This is an excellent addition that documents the system architecture including entry points, configuration, authentication/authorization, Llama Stack client, quota management, caching, metrics, MCP integration, and A2A protocol support. The document is well-structured with clear diagrams and explanations.
The static analysis hints for this file are false positives:
- Line 44: The ASCII diagram doesn't require a language identifier
- Line 573: The emphasis is intentional styling for document closure
- Line 339: "MCP-HEADERS" is a technical term (HTTP header name)
docker-compose.yaml (2)
2-17: LGTM!The
mcp-mock-serverservice is properly configured with appropriate healthcheck settings, network connectivity, and build context. The healthcheck intervals are reasonable for a mock server.
89-90: LGTM!The dependency on
mcp-mock-serverwithservice_healthycondition ensures the mock server is ready beforelightspeed-stackstarts, which is important since MCP servers must be accessible at startup time per the architecture documentation.docker-compose-library.yaml (3)
2-17: LGTM!The
mcp-mock-serverservice configuration is consistent withdocker-compose.yaml, maintaining parity between server-mode and library-mode setups.
28-32: LGTM!The dependency and network configuration ensures proper service orchestration for library mode testing.
76-78: LGTM!The bridge network definition enables container-to-container communication, which is essential for the
lightspeed-stackservice to reachmcp-mock-serverusing DNS name resolution.tests/e2e/configs/run-ci.yaml (1)
68-70: LGTM!The MCP provider configuration follows the established pattern for tool_runtime providers and correctly enables Model Context Protocol support for CI testing.
src/app/routers.py (2)
21-21: LGTM!The import follows the existing pattern and uses absolute imports as required by coding guidelines.
42-42: LGTM!The router registration follows the established pattern and is correctly placed with other
/v1prefixed routers.tests/e2e/features/steps/info.py (2)
183-212: LGTM!The step definition properly validates the MCP client auth options response structure with clear assertions and informative error messages. Type annotations and docstring follow the coding guidelines.
215-241: LGTM!The step definition correctly validates specific server presence and header configuration. The use of
.get()with defaults and detailed assertion messages makes debugging easier.README.md (2)
392-393: LGTM!The clarification about Kubernetes token handling is important for operators to understand when MCP servers will be available.
421-447: Well-documented new endpoint.The documentation clearly explains the MCP Client Auth Options endpoint purpose, response structure, and filtering behavior. The example response and use-case guidance will help developers integrate with this feature effectively.
tests/e2e/features/info.feature (1)
131-137: LGTM!The e2e test scenario follows the established Gherkin format and provides good coverage for the new MCP client auth options endpoint. The test validates both the response structure and specific server configurations. The endpoint requires no library-mode-specific handling since it simply reads and returns configured MCP servers, making this test valid in both server and library modes.
src/models/responses.py (1)
130-166: LGTM! Well-structured response models.The new
MCPServerAuthInfoandMCPClientAuthOptionsResponsemodels follow the established patterns in this file. The models are properly documented, have complete type annotations, and include appropriate OpenAPI examples.tests/unit/app/endpoints/test_mcp_auth.py (4)
1-24: LGTM! Proper test setup following coding guidelines.The test file correctly uses:
pytestandpytest-mockfor testing- The required
MOCK_AUTHtuple pattern with 4 fields as specified in the coding guidelines
26-108: Well-designed test fixtures covering key scenarios.The fixtures provide good coverage for different MCP server configurations:
- Servers with client auth headers
- Mixed auth types (client + kubernetes + none)
- No client auth servers
110-241: Comprehensive test coverage for the endpoint.The tests effectively cover:
- Success scenarios with client auth servers
- Filtering behavior for mixed auth types
- Empty results when no client auth is configured
- Empty MCP server list handling
243-339: Good edge case coverage for whitespace and multiple headers.The whitespace handling test validates that the endpoint correctly trims header values, and the multiple headers test ensures all client auth headers are properly surfaced.
src/app/endpoints/mcp_auth.py (4)
1-23: LGTM! Proper module setup following coding guidelines.The module correctly follows the required patterns:
- Descriptive module docstring
- Absolute imports for internal modules
logger = logging.getLogger(__name__)pattern- FastAPI APIRouter with appropriate prefix and tags
26-33: LGTM! Well-defined OpenAPI response specifications.The response mapping correctly documents all expected HTTP status codes with appropriate examples.
36-67: LGTM! Endpoint function properly documented and structured.The function signature and docstring are well-formed. The explicit discarding of
authandrequestparameters (used only by middleware) clearly communicates intent.
68-91: LGTM! Clean and correct filtering logic.The implementation correctly:
- Validates configuration is loaded before processing
- Handles servers without authorization headers
- Filters headers with "client" value (with whitespace handling via
.strip())- Constructs the response with only client-auth capable servers
Note:
configuration.mcp_serversis guaranteed to be a list (neverNone) since theConfigurationmodel defines it withdefault_factory=list, so the iteration at line 70 is safe without additional guards.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 1. **MCP Servers** - Used for testing MCP-related endpoints: | ||
| - `github-api` - Uses client-provided auth (Authorization header) | ||
| - `gitlab-api` - Uses client-provided auth (X-API-Token header) | ||
| - `k8s-service` - Uses kubernetes auth (not client-provided) | ||
| - `public-api` - No authentication (not client-provided) | ||
|
|
||
| These servers test the `/v1/mcp-auth/client-options` endpoint, which should return only servers accepting client-provided authentication (`github-api` and `gitlab-api`). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if k8s-service is defined in any test configuration file
echo "=== Searching for k8s-service in test configurations ==="
rg -n "k8s-service" tests/e2e/configuration/
echo ""
echo "=== List all MCP server names in test configs ==="
rg -n "name:.*-api|name:.*-service" tests/e2e/configuration/Repository: lightspeed-core/lightspeed-stack
Length of output: 791
Remove k8s-service from the documentation or add it to the configuration files.
The README lists k8s-service as one of four MCP servers used for testing, but this server is not defined in either library-mode/lightspeed-stack.yaml or server-mode/lightspeed-stack.yaml. Both configuration files only contain three servers: github-api, gitlab-api, and public-api. Update the documentation to reflect the actual available servers, or add the missing k8s-service configuration if it should be included.
2 participants
Description
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
https://issues.redhat.com/browse/LCORE-1171
https://issues.redhat.com/browse/LCORE-1171
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Infrastructure
Tests
Auth
✏️ Tip: You can customize this high-level summary in your review settings.