Add configuration for MCP servers, updated documentation, created man… #972
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 per-server MCP authorization (file, Kubernetes, client) with header resolution and skipping behavior, a new auth utility, model/schema updates, endpoint changes to assemble per-server headers, a self-contained MCP mock server plus tests and docs, new test configs/examples, Makefile/.gitignore updates, and extensive unit test additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryEndpoint as Query Endpoint
participant ConfigModel as ModelContextProtocolServer
participant AuthUtils as resolve_authorization_headers
participant MCPServer as MCP Server
Client->>QueryEndpoint: Request for MCP tools
QueryEndpoint->>ConfigModel: access configured mcp_servers
ConfigModel->>AuthUtils: resolve_authorization_headers(authorization_headers)
AuthUtils-->>ConfigModel: resolved_authorization_headers
QueryEndpoint->>QueryEndpoint: iterate servers, resolve per-server headers
alt Headers resolved for server
QueryEndpoint->>MCPServer: POST MCP request with resolved headers
MCPServer-->>QueryEndpoint: tool list / response
else Required headers unresolved
QueryEndpoint-->>QueryEndpoint: skip server and log warning
end
QueryEndpoint-->>Client: return aggregated tools
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 3
🤖 Fix all issues with AI agents
In @dev-tools/MANUAL_TESTING.md:
- Around line 94-96: Update the fenced code block that currently contains "DEBUG
Configured 3 MCP tools: ['mock-file-auth', 'mock-k8s-auth', 'mock-client-auth']"
to include a language specifier (use "text") so the block starts with ```text
and ends with ```, enabling proper syntax highlighting.
In @src/utils/mcp_auth_headers.py:
- Around line 15-19: Fix the typos in the docstring for the
authorization_headers parameter in src/utils/mcp_auth_headers.py: change both
occurrences of "live is unchanged" and "live it unchanged." to "leave it
unchanged" so the descriptions for the "kubernetes" and "client" special
keywords read correctly; keep the rest of the wording intact and preserve the
bullet formatting under the Parameters section.
In @tests/unit/models/config/test_model_context_protocol_server.py:
- Around line 140-157: The test passes literal secrets but
ModelContextProtocolServer.resolve_auth_headers() treats
non-"client"/"kubernetes" values as file paths, so update
test_model_context_protocol_server_with_authorization_headers to create
tmp_path-backed files containing "my-secret" and "api-key-secret", set
ModelContextProtocolServer.authorization_headers to the file paths, then assert
resolved headers (after calling mcp.resolve_auth_headers() if needed) match the
expected secret strings; reference
ModelContextProtocolServer.resolve_auth_headers and
resolve_authorization_headers to locate the resolution behavior and the test
function name to update.
🧹 Nitpick comments (12)
dev-tools/mcp-mock-server/README.md (1)
36-51: Add language specification to code block.The code block starting at line 36 lacks a language identifier. Add a language specification (e.g.,
```textor```plaintext) for proper markdown linting.You should see: -``` +```text ====================================================================== MCP Mock Server starting with HTTP and HTTPS ======================================================================examples/lightspeed-stack-mcp-servers.yaml (1)
30-49: Clarify token formatting + “timeout” behavior in the examplesThese examples are clear, but two small clarifications would prevent confusion:
- If the upstream expects
Authorization: Bearer <token>, the file needs to contain the full header value, not just the raw token.timeoutis marked “future feature”; consider noting it may be ignored at runtime until Llama Stack supports it.tests/unit/models/config/test_model_context_protocol_server.py (2)
3-4: Avoid globalpyrightsuppression; prefer local ignores
# pyright: reportCallIssue=falsecan mask real issues across the whole module; the file already uses targeted# pyright: ignorewhere needed.Proposed diff
-# pyright: reportCallIssue=false - - import pytest
257-260: Add a type fortmp_pathfor consistencyOther tests already annotate
tmp_path: Path; doing the same here keeps typing consistent.Proposed diff
-def test_model_context_protocol_server_resolved_headers_with_file( - tmp_path, -) -> None: +from pathlib import Path + +def test_model_context_protocol_server_resolved_headers_with_file( + tmp_path: Path, +) -> None:tests/unit/utils/test_mcp_auth_headers.py (1)
1-116: Nice coverage of file-based resolution + special keywordsTests hit the key cases (file read/strip, missing file, mixed headers, preserving
client/kubernetes) and keep assertions focused on returned values.src/utils/mcp_auth_headers.py (1)
73-79: Consider not logging the raw value for security.The error log includes the raw
valuewhich could be a file path containing sensitive information in its name. Consider masking or omitting it.🔒 Proposed fix
except Exception as e: # pylint: disable=broad-except logger.error( - "Failed to resolve authorization header %s with value %s: %s", + "Failed to resolve authorization header %s: %s", header_name, - value, e, )src/models/config.py (1)
485-493: Consider usingPositiveIntfor timeout validation.The
timeoutfield accepts any integer including negative values. If timeout should always be positive when specified, consider usingPositiveInt | Nonefor automatic validation.♻️ Proposed fix
- timeout: int | None = Field( + timeout: PositiveInt | None = Field( default=None, title="Request timeout", description=( "Timeout in seconds for requests to the MCP server. " "If not specified, the default timeout from Llama Stack will be used. " "Note: This field is reserved for future use when Llama Stack adds timeout support." ), )src/app/endpoints/query_v2.py (1)
709-727: Nested helper function captures outer scope variable.The
_get_token_valuefunction referencesmcp_serverfrom the enclosingforloop scope. While this works correctly in Python, it could be clearer to passmcp_server.nameas an explicit parameter.♻️ Proposed refactor for clarity
- def _get_token_value(original: str, header: str) -> str | None: + def _get_token_value(original: str, header: str, server_name: str) -> str | None: """convert to header value""" match original: case "kubernetes": # use k8s token if token is None or token == "": return None return f"Bearer {token}" case "client": # use client provided token if mcp_headers is None: return None - c_headers = mcp_headers.get(mcp_server.name, None) + c_headers = mcp_headers.get(server_name, None) if c_headers is None: return None return c_headers.get(header, None) case _: # use provided return originalThen call it as
_get_token_value(value, name, mcp_server.name)on line 743.dev-tools/mcp-mock-server/test_mock_mcp_server.py (1)
35-41: Consider polling for server readiness instead of fixed sleep.The fixed 2-second sleep may be insufficient on slower systems or excessive on fast ones. Consider polling the server with a health check.
♻️ Proposed improvement
# Wait for server to start - time.sleep(2) - - # Check if server started successfully - if process.poll() is not None: - _, stderr = process.communicate() - pytest.fail(f"Server failed to start: {stderr.decode('utf-8')}") + max_retries = 10 + for i in range(max_retries): + if process.poll() is not None: + _, stderr = process.communicate() + pytest.fail(f"Server failed to start: {stderr.decode('utf-8')}") + try: + req = urllib.request.Request(f"http://localhost:{http_port}/debug/headers") + with urllib.request.urlopen(req, timeout=1): + break # Server is ready + except (urllib.error.URLError, ConnectionRefusedError): + time.sleep(0.5) + else: + process.terminate() + pytest.fail("Server did not become ready in time")dev-tools/mcp-mock-server/server.py (3)
26-31: Use modern type annotations.As per coding guidelines, prefer modern type syntax. The
Dictimport is legacy style (Python 3.9+).♻️ Suggested fix
-from typing import Dict - - -# Global storage for captured headers (last request) -last_headers: Dict[str, str] = {} -request_log: list = [] +# Global storage for captured headers (last request) +last_headers: dict[str, str] = {} +request_log: list[dict[str, str | dict[str, str]]] = []
34-35: Enhance class docstring.As per coding guidelines, all classes require descriptive docstrings explaining purpose.
📝 Suggested enhancement
class MCPMockHandler(BaseHTTPRequestHandler): - """HTTP request handler for mock MCP server.""" + """HTTP request handler for mock MCP server. + + Implements basic MCP protocol endpoints for testing authorization headers. + Captures and logs request headers for debugging and validation purposes. + + Attributes: + Headers are captured to global last_headers and request_log variables. + """
266-266: Add return type annotation to main function.As per coding guidelines, complete type annotations are required for all function parameters and return types.
♻️ Suggested fix
-def main(): +def main() -> None:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.gitignoreREADME.mddev-tools/MANUAL_TESTING.mddev-tools/README.mddev-tools/mcp-mock-server/README.mddev-tools/mcp-mock-server/server.pydev-tools/mcp-mock-server/test_mock_mcp_server.pydev-tools/test-configs/llama-stack-mcp-test.yamldev-tools/test-configs/mcp-mock-test.yamlexamples/lightspeed-stack-mcp-servers.yamlsrc/app/endpoints/query_v2.pysrc/configuration.pysrc/models/config.pysrc/utils/mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.py
🧰 Additional context used
📓 Path-based instructions (7)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from auth import get_auth_dependency
Llama Stack imports should use:from llama_stack_client import AsyncLlamaStackClient
All modules must start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases must be defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations required for all function parameters and return types
Usetyping_extensions.Selffor model validators in Pydantic models
Use modern union type syntaxstr | intinstead ofUnion[str, int]
UseOptional[Type]orType | Nonefor optional types
Use snake_case naming for functions with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying parameters
Useasync deffor functions performing I/O operations and external API calls
Use standard log levels with clear purposes: debug for diagnostic info, info for general execution, warning for unexpected issues, error for serious problems
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Complete type annotations required for all class attributes
Docstrings must follow Google Python docstring conventions with sections: Args, Returns, Raises, Attributes as applicable
Files:
tests/unit/models/config/test_model_context_protocol_server.pytests/unit/app/endpoints/test_streaming_query_v2.pydev-tools/mcp-mock-server/server.pysrc/configuration.pydev-tools/mcp-mock-server/test_mock_mcp_server.pysrc/models/config.pytests/unit/utils/test_mcp_auth_headers.pysrc/utils/mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pysrc/app/endpoints/query_v2.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest framework for all unit and integration tests; do not use unittest
Files:
tests/unit/models/config/test_model_context_protocol_server.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/utils/test_mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockfor AsyncMock objects in tests
Files:
tests/unit/models/config/test_model_context_protocol_server.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/utils/test_mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All config classes must use Pydantic models extendingConfigurationBasewithextra="forbid"to reject unknown fields
Configuration type hints must use:Optional[FilePath],PositiveInt,SecretStr
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic config classes must extendConfigurationBase; data models must extendBaseModel
Pydantic models must use@model_validatorand@field_validatorfor validation
Files:
src/models/config.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
FastAPI dependencies must be imported from:
from fastapi import APIRouter, HTTPException, Request, status, Depends
Files:
src/app/endpoints/query_v2.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoints and handleAPIConnectionErrorfrom Llama Stack
Files:
src/app/endpoints/query_v2.py
🧠 Learnings (5)
📚 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:
dev-tools/test-configs/mcp-mock-test.yaml
📚 Learning: 2026-01-09T09:40:04.683Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T09:40:04.683Z
Learning: Applies to tests/**/*.py : Use `pytest-mock` for AsyncMock objects in tests
Applied to files:
dev-tools/mcp-mock-server/test_mock_mcp_server.py
📚 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:
dev-tools/test-configs/llama-stack-mcp-test.yamlREADME.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:
README.md
📚 Learning: 2026-01-09T09:40:04.683Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T09:40:04.683Z
Learning: Applies to src/app/endpoints/**/*.py : Use FastAPI `HTTPException` with appropriate status codes for API endpoints and handle `APIConnectionError` from Llama Stack
Applied to files:
tests/unit/app/endpoints/test_query_v2.py
🧬 Code graph analysis (6)
tests/unit/models/config/test_model_context_protocol_server.py (2)
src/models/config.py (3)
config(325-342)ModelContextProtocolServer(425-511)Configuration(1440-1554)src/configuration.py (1)
mcp_servers(151-162)
tests/unit/app/endpoints/test_streaming_query_v2.py (1)
src/models/config.py (1)
ModelContextProtocolServer(425-511)
src/models/config.py (1)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-81)
tests/unit/utils/test_mcp_auth_headers.py (1)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-81)
tests/unit/app/endpoints/test_query_v2.py (2)
src/models/config.py (1)
ModelContextProtocolServer(425-511)src/app/endpoints/query_v2.py (1)
get_mcp_tools(675-765)
src/app/endpoints/query_v2.py (2)
src/models/config.py (3)
config(325-342)Action(810-863)ModelContextProtocolServer(425-511)src/configuration.py (1)
mcp_servers(151-162)
🪛 Gitleaks (8.30.0)
dev-tools/MANUAL_TESTING.md
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 115-117: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
README.md
[style] ~297-~297: Consider using “inaccessible” to avoid wordiness.
Context: ...gured in the llama-stack run.yaml are not accessible to lightspeed-core agents. #### Config...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
dev-tools/MANUAL_TESTING.md
94-94: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
dev-tools/mcp-mock-server/README.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (29)
.gitignore (1)
183-184: LGTM!Appropriate addition to ignore auto-generated mock server certificates. The placement within the "Dev stuff" section, specific path targeting, and clear comment make this a well-structured change. Self-signed certificates should never be committed to version control.
src/configuration.py (2)
327-334: Clarify the purpose of the pylint disable comment.Line 329 suppresses a
no-memberwarning onenable_token_historywithout explanation. This suggests the attribute may not be reliably available on the quota_handlers object.Can you clarify:
- Why does
enable_token_historyrequire a pylint suppression?- Is this attribute dynamically added, optional, or conditionally present?
- Should there be defensive code (e.g.,
hasattr()check) instead of suppressing the warning?
3-31: The imports in this file are correct and already follow the coding guidelines.The current import pattern (
from models.config,from cache.cache,from quota.quota_limiter) uses absolute imports relative to the configured Python path, which is explicitly set inpyproject.tomlwithpythonpath = ["src"]. This matches the guideline exactly—the example provided (from auth import get_auth_dependency) uses the same pattern without thesrc.prefix.Adding
src.to the import paths as suggested would break the imports, sincesrcis already included in the configured Python path.No changes are needed.
dev-tools/README.md (1)
1-32: LGTM!Clear documentation structure guiding developers to the MCP Mock Server and manual testing resources. Well-organized.
dev-tools/mcp-mock-server/README.md (1)
1-275: Comprehensive and well-structured documentation.The README provides clear setup instructions, configuration examples for multiple authentication methods, debug endpoints, test integration guidance, and troubleshooting. Good coverage of use cases and limitations. The examples are practical and easy to follow.
tests/unit/app/endpoints/test_streaming_query_v2.py (1)
61-66: LGTM: test setup now matches per-server auth header configAdding
authorization_headers={"Authorization": "kubernetes"}keeps the endpoint test aligned with the updatedModelContextProtocolServerconfiguration contract.dev-tools/test-configs/llama-stack-mcp-test.yaml (1)
1-98: Looks useful for local MCP testing; please verify schema/version compatibilityThis is a solid minimal dev config, but the exact YAML schema (e.g.,
${env.OPENAI_API_KEY}interpolation andprovider_typestrings likeremote::model-context-protocol) is version-sensitive in Llama Stack—worth validating against the version used by this repo.dev-tools/test-configs/mcp-mock-test.yaml (1)
1-38: Well-structured test configuration covering all three authorization types.The configuration correctly defines three MCP servers to test file-based, Kubernetes, and client-provided authentication methods. This aligns with the authorization header resolution logic in
src/utils/mcp_auth_headers.py.dev-tools/MANUAL_TESTING.md (1)
1-163: Comprehensive manual testing guide for MCP authorization.The documentation thoroughly covers prerequisites, test scenarios, verification steps, and troubleshooting. The example tokens (like
Bearer my-k8s-token) are clearly for testing purposes only.src/utils/mcp_auth_headers.py (1)
1-81: Clean implementation of authorization header resolution.The function correctly handles the three authorization scenarios (kubernetes, client, file-based) with appropriate logging and error handling. The use of
Pathfor file operations and context managers for file reading follows best practices.src/models/config.py (1)
459-511: Well-designed per-server authorization configuration.The new fields and validator correctly implement the per-server authorization pattern. Using
init=Falseforresolved_authorization_headersensures it's only populated by the validator, and themodel_validator(mode="after")runs after all fields are set.src/app/endpoints/query_v2.py (2)
466-468: Good defensive check for None response.Adding the None check for
responseprevents potentialAttributeErrorwhen the agent fails and returns None.
748-758: Appropriate server skipping with warning.The logic correctly skips MCP servers when authorization headers are configured but cannot be fully resolved, with an informative warning log. This prevents authentication failures at runtime.
README.md (1)
297-438: Comprehensive MCP authentication documentation.The documentation clearly explains all three authentication methods with practical examples, comparison table, and important notes about automatic server skipping. This will be valuable for users configuring MCP server authentication.
dev-tools/mcp-mock-server/test_mock_mcp_server.py (2)
78-84: Using unverified SSL context is acceptable for testing.The use of
ssl._create_unverified_context()is appropriate here since the mock server uses self-signed certificates. The# pylint: disable=protected-accesscomment correctly acknowledges this.
103-267: Good test coverage for mock server functionality.The tests comprehensively cover HTTP/HTTPS endpoints, header capture, request logging, and counter increments. The assertions are specific and include helpful error messages.
tests/unit/app/endpoints/test_query_v2.py (9)
1-2: LGTM on the pyright directive.The
# pyright: reportCallIssue=falsedirective is appropriate for test files where mocks may not have perfect type signatures.
56-82: LGTM! Good coverage of kubernetes auth flow.The test correctly validates:
- Servers without auth requirements don't include headers
- Kubernetes auth resolves to
Bearer {token}format
84-111: LGTM! Comprehensive client auth testing.Good verification that servers requiring client-provided headers are correctly skipped when
mcp_headers=None.
114-130: LGTM! Validates file-based secret resolution.Good integration test that verifies the full flow from file path in
authorization_headersto resolved secret value in the tool definition.
133-163: LGTM! Excellent mixed header scenario coverage.This test validates the most complex use case where a single server uses all three auth mechanisms simultaneously.
166-211: LGTM! Thorough testing of server skipping logic.Excellent coverage of edge cases where servers are skipped due to incomplete auth header resolution. The partial auth test (Line 181-189) is particularly valuable.
214-229: LGTM! Public server scenario validated.Correctly verifies that servers with no auth requirements are always included regardless of token/mcp_headers availability.
298-303: LGTM! Fixture updated to use new authorization_headers field.The test correctly uses the updated
ModelContextProtocolServerconstructor withauthorization_headers.
670-671: LGTM on the type ignore comment.The
# type: ignore[index]is appropriate here since we've already assertedisinstance(detail, dict)on Line 669, but the type checker doesn't narrow the type.dev-tools/mcp-mock-server/server.py (4)
42-62: Thread safety note for concurrent requests.The global
last_headersandrequest_logmodifications are not thread-safe. With concurrent HTTP and HTTPS servers, requests could interleave. For a dev/testing tool this is acceptable, but worth noting.Consider using
collections.deque(maxlen=10)instead of manual list management for better performance and simplicity:from collections import deque request_log: deque = deque(maxlen=10)
64-149: LGTM! Well-implemented MCP protocol mock.The handler correctly:
- Parses JSON-RPC requests with error handling
- Varies tool names based on auth header (useful for testing multiple auth scenarios)
- Implements required MCP methods (
initialize,tools/list)
200-245: LGTM! Certificate generation is well-implemented.Good practices:
- Reuses existing certificates
- Uses
check=Truefor subprocess error handling- Captures output to avoid cluttering console
314-321: Thread join behavior with daemon threads.The
join()calls on daemon threads will block indefinitely in normal operation (since daemon threads only terminate when the main program exits). This works because the servers runserve_forever(), but the shutdown is only triggered onKeyboardInterrupt.This is acceptable for a dev tool, but if you want cleaner shutdown behavior, consider adding a signal handler or using
threading.Eventfor coordinated shutdown.
| @@ -0,0 +1,32 @@ | |||
| # Development Tools | |||
|
|
|||
| This directory contains utilities and tools for local development and testing of Lightspeed Core Stack. | |||
There was a problem hiding this comment.
These are not scripts its a mock MCP server
dev-tools/mcp-mock-server/server.py
Outdated
|
|
||
|
|
||
| # Global storage for captured headers (last request) | ||
| last_headers: Dict[str, str] = {} |
|
|
||
| def _capture_headers(self) -> None: | ||
| """Capture all headers from the request.""" | ||
| last_headers.clear() |
There was a problem hiding this comment.
will it work for concurrent calls?
There was a problem hiding this comment.
No, its just a simple mock for manual testing
There was a problem hiding this comment.
but the issue is still there, right?
There was a problem hiding this comment.
No. Its test only. Not a full-fledged MCP server
dev-tools/mcp-mock-server/server.py
Outdated
|
|
||
| # Determine tool name based on authorization header to avoid collisions | ||
| auth_header = self.headers.get("Authorization", "") | ||
| if "test-secret-token" in auth_header: |
There was a problem hiding this comment.
match-case might be better there
dev-tools/mcp-mock-server/server.py
Outdated
| def do_GET(self) -> None: # pylint: disable=invalid-name | ||
| """Handle GET requests (debug endpoints).""" | ||
| # Debug endpoint to view captured headers | ||
| if self.path == "/debug/headers": |
There was a problem hiding this comment.
match-case (if there are no better way how to structure it)
|
|
||
| def run_https_server(port: int, httpd: HTTPServer) -> None: | ||
| """Run HTTPS server in a thread.""" | ||
| print(f"HTTPS server started on https://localhost:{port}") |
There was a problem hiding this comment.
probably worth to use logger instead of plain prints
There was a problem hiding this comment.
This is not the main code, its a mock server. print here is fine - it's simpler and fits the tool's purpose.
| url: "http://localhost:3002" | ||
| url: "http://api-service:8080" | ||
| authorization_headers: | ||
| Authorization: "/var/secrets/api-token" # Path to file containing token |
There was a problem hiding this comment.
how to pass user token into MCP? IMHO this is really needed feature, right?
There was a problem hiding this comment.
This was implemented before, but added this as well
| create a tool definitions for each of them. If MCP server definition | ||
| has a non-empty resolved_authorization_headers we create invocation | ||
| headers, following the algorithm: | ||
| 1. If the header value is 'kubernetes' the header value is a k8s token |
There was a problem hiding this comment.
is it tied only to k8s now? LCORE supports multiple auth mechanisms, so probably other mechanisms will be needed too
There was a problem hiding this comment.
For this PR, I think it's fine, but you are correct. We need to consider this
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @dev-tools/mcp-mock-server/test_mock_mcp_server.py:
- Around line 255-271: The test_request_count_increments is flaky because it
relies on shared module-scoped mock_server state (globals like last_headers and
request_log); modify the test to either reset the server state before measuring
(e.g., call a reset endpoint or clear request_log/last_headers on the
mock_server fixture) or assert relative change by capturing the current
request_count via make_request(f"{mock_server['http_url']}/debug/headers") then
performing the POST to /mcp/v1/list_tools and asserting the new count equals
previous_count + 1; reference make_request, mock_server, /debug/headers and
/mcp/v1/list_tools when implementing the state-reset or relative-assertion fix.
🧹 Nitpick comments (6)
dev-tools/mcp-mock-server/test_mock_mcp_server.py (1)
22-56: Consider polling for server readiness instead of fixed sleep.The 2-second
time.sleep(36)is brittle—it may be too short on slow CI runners or unnecessarily long on fast machines. A polling approach with a timeout would be more reliable.♻️ Suggested improvement
- # Wait for server to start - time.sleep(2) - - # Check if server started successfully - if process.poll() is not None: - _, stderr = process.communicate() - pytest.fail(f"Server failed to start: {stderr.decode('utf-8')}") + # Poll for server readiness with timeout + max_wait = 10 # seconds + poll_interval = 0.2 + start_time = time.time() + server_ready = False + + while time.time() - start_time < max_wait: + if process.poll() is not None: + _, stderr = process.communicate() + pytest.fail(f"Server failed to start: {stderr.decode('utf-8')}") + try: + with urllib.request.urlopen(f"http://localhost:{http_port}/", timeout=1): + server_ready = True + break + except (urllib.error.URLError, ConnectionRefusedError): + time.sleep(poll_interval) + + if not server_ready: + process.terminate() + pytest.fail("Server did not become ready within timeout")dev-tools/mcp-mock-server/server.py (2)
28-30: Thread-safety concern with global mutable state.
last_headersandrequest_logare global dicts/lists modified by multiple request threads. While Python's GIL provides some protection, operations likelast_headers.clear()followed by updates in_capture_headerscould result in inconsistent state if requests interleave. For a dev-only tool this is acceptable, but consider adding a note.📝 Optional: Add thread-safety note
# Global storage for captured headers (last request) +# Note: Not thread-safe; intended for single-client testing scenarios only. last_headers: dict[str, str] = {} request_log: list = []
307-326: Daemon threads may not allow graceful shutdown to complete.Since
daemon=Trueis set, if the main thread exits (e.g., due to an unhandled exception), the server threads will be abruptly killed. Also, thejoin()calls on daemon threads will block indefinitely sinceserve_forever()never returns unlessshutdown()is called. The current code works becauseKeyboardInterrupttriggersshutdown(), but consider the edge case.dev-tools/mcp-mock-server/README.md (1)
38-53: Add language specifier to fenced code block.The output example block is missing a language specifier. While it's plain text output, adding
textor leaving it empty with explicit specifier helps consistency.📝 Fix markdown lint warning
You should see: -``` +```text ====================================================================== MCP Mock Server starting with HTTP and HTTPSdev-tools/MANUAL_TESTING.md (1)
22-30: Consider extracting dependency list to a requirements file.The inline dependency list on line 27 is long and may become out of sync. Consider maintaining these in a dedicated requirements file (e.g.,
dev-tools/test-configs/library-mode-requirements.txt) and referencing it.# Alternative approach: uv pip install -r dev-tools/test-configs/library-mode-requirements.txttests/unit/models/config/test_model_context_protocol_server.py (1)
274-276: Add type annotation for consistency.The
tmp_pathparameter is missing its type annotation, unlike the similar test on line 143.📝 Add type annotation
def test_model_context_protocol_server_resolved_headers_with_file( - tmp_path, + tmp_path: Path, ) -> None:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mddev-tools/MANUAL_TESTING.mddev-tools/mcp-mock-server/README.mddev-tools/mcp-mock-server/server.pydev-tools/mcp-mock-server/test_mock_mcp_server.pysrc/utils/mcp_auth_headers.pytests/unit/models/config/test_model_context_protocol_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/mcp_auth_headers.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all unit and integration tests
Files:
tests/unit/models/config/test_model_context_protocol_server.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Do not use unittest - pytest is the standard for this project
Usepytest-mockfor AsyncMock objects in tests
Files:
tests/unit/models/config/test_model_context_protocol_server.py
🧠 Learnings (4)
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to tests/**/*.py : Use `pytest-mock` for AsyncMock objects in tests
Applied to files:
dev-tools/mcp-mock-server/test_mock_mcp_server.py
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use standard log levels: `logger.debug()` for detailed diagnostic info, `logger.info()` for general execution info, `logger.warning()` for unexpected events, `logger.error()` for serious problems
Applied to files:
dev-tools/mcp-mock-server/server.py
📚 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:
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:
README.md
🧬 Code graph analysis (1)
tests/unit/models/config/test_model_context_protocol_server.py (2)
src/models/config.py (3)
config(325-342)ModelContextProtocolServer(425-511)Configuration(1440-1554)src/configuration.py (1)
mcp_servers(151-162)
🪛 Gitleaks (8.30.0)
dev-tools/MANUAL_TESTING.md
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 115-117: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
dev-tools/mcp-mock-server/README.md
[high] 96-98: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 208-210: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
README.md
[style] ~297-~297: Consider using “inaccessible” to avoid wordiness.
Context: ...gured in the llama-stack run.yaml are not accessible to lightspeed-core agents. #### Config...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
dev-tools/mcp-mock-server/README.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (16)
dev-tools/mcp-mock-server/test_mock_mcp_server.py (4)
103-121: LGTM!The test correctly validates HTTP POST to the MCP
tools/listendpoint, verifies JSON-RPC response structure, and checks that the mock tool is returned.
124-138: LGTM!Good coverage of HTTPS endpoint with self-signed certificate handling.
140-168: LGTM!Properly validates that the debug endpoint captures both standard and custom headers.
170-198: LGTM!Request logging validation is thorough, checking all expected fields in the request log.
dev-tools/mcp-mock-server/server.py (3)
63-78: LGTM!POST handler correctly parses JSON-RPC requests with appropriate fallback for malformed input.
79-97: LGTM!The match statement cleanly maps authorization tokens to tool identities, which is useful for testing different auth scenarios.
204-249: LGTM!Self-signed certificate generation is well-implemented with appropriate caching to avoid regeneration on each run.
dev-tools/mcp-mock-server/README.md (1)
1-276: LGTM!The documentation is comprehensive, covering quick start, configuration examples, debug endpoints, testing methods, and troubleshooting. The Gitleaks warnings about authorization tokens are false positives—these are clearly example/placeholder values in documentation.
dev-tools/MANUAL_TESTING.md (1)
1-163: LGTM!The manual testing guide is well-structured with clear prerequisites, step-by-step instructions, expected results, and troubleshooting. The Gitleaks warnings are false positives—these are example tokens for local testing.
tests/unit/models/config/test_model_context_protocol_server.py (4)
142-169: LGTM!Excellent test coverage for file-based authorization header resolution, properly verifying both raw and resolved header values.
172-191: LGTM!Good coverage of the special
kubernetesandclientkeyword handling.
194-253: LGTM!Comprehensive test for mixed authorization scenarios, validating backward compatibility (no-auth servers) alongside the new auth header functionality.
256-271: LGTM!Properly validates that special values (
kubernetes,client) are preserved through resolution rather than being treated as file paths.README.md (3)
1108-1120: YAML block inside Markdown code fence may not render correctly.The YAML configuration example starting at line 1115 appears to be outside the bash code fence that ends at line 1111, but the indentation suggests it should be part of the configuration snippet. Verify the rendering.
297-434: LGTM!The MCP Server Authentication documentation is comprehensive and well-structured. The three authentication methods (file-based, Kubernetes, client-provided) are clearly explained with practical examples. The comparison table and automatic server skipping behavior documentation are particularly helpful.
1100-1128: LGTM!Good addition of the Development Tools section. The MCP Mock Server quick start is concise and links to detailed documentation.
blublinsky
force-pushed
the
mcp-headers
branch
from
cc87f5b to
c702382
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/endpoints/query_v2.py (2)
450-468: Fix type contract: function checksresponse is Nonebut doesn’t acceptNone.
Either remove the None-branch or change the signature toOpenAIResponseObject | None(and drop the now-incorrectunused-argumentpylint disable).Proposed fix
def parse_referenced_documents_from_responses_api( - response: OpenAIResponseObject, # pylint: disable=unused-argument + response: OpenAIResponseObject | None, ) -> list[ReferencedDocument]: @@ - if response is None or not response.output: + if response is None or not response.output: return documents
675-766:get_mcp_tools: doc mismatch + fragile closure overmcp_serverfor client headers.
- Docstring says
mcp_headersis keyed by server URL, but code usesmcp_server.name(and README/docs also describe “server name”)._get_token_value()readsmcp_server.namefrom the outer loop; passserver_nameexplicitly to avoid future footguns.Proposed fix
def get_mcp_tools( mcp_servers: list[ModelContextProtocolServer], token: str | None = None, - mcp_headers: dict[str, dict[str, str]] | None = None, + mcp_headers: dict[str, dict[str, str]] | None = None, ) -> list[dict[str, Any]]: @@ - mcp_headers: Optional per-request headers for MCP servers, keyed by server URL + mcp_headers: Optional per-request headers for MCP servers, keyed by server name @@ - def _get_token_value(original: str, header: str) -> str | None: + def _get_token_value(server_name: str, original: str, header: str) -> str | None: """Convert to header value.""" match original: case "kubernetes": # use k8s token if token is None or token == "": return None - return f"Bearer {token}" + return token if token.lower().startswith("bearer ") else f"Bearer {token}" case "client": # use client provided token if mcp_headers is None: return None - c_headers = mcp_headers.get(mcp_server.name, None) - if c_headers is None: - return None - return c_headers.get(header, None) + return (mcp_headers.get(server_name) or {}).get(header) case _: # use provided return original @@ for name, value in mcp_server.resolved_authorization_headers.items(): # for each defined header - h_value = _get_token_value(value, name) + h_value = _get_token_value(mcp_server.name, value, name)
🤖 Fix all issues with AI agents
In @dev-tools/MANUAL_TESTING.md:
- Around line 53-60: Replace the hardcoded bearer tokens in the curl example
with non-secret placeholders: change the header "Authorization: Bearer
my-k8s-token" to something like "Authorization: Bearer <K8S_TOKEN_PLACEHOLDER>"
and inside the MCP-HEADERS JSON replace "Authorization": "Bearer
my-client-token" with "Authorization": "Bearer <CLIENT_TOKEN_PLACEHOLDER>" (or
use environment variable syntax like $K8S_TOKEN and $CLIENT_TOKEN) so the curl
command and the MCP-HEADERS line no longer contain literal-looking secrets.
In @dev-tools/mcp-mock-server/server.py:
- Around line 194-198: The docs and startup message claim POSTs go to
/mcp/v1/list_tools but the HTTP handler do_POST accepts any path; either tighten
do_POST to validate self.path (in do_POST) against the documented endpoint
(e.g., require '/mcp/v1/list_tools' and return 404/405 for other paths with a
clear error log) or update the help page and the startup message to state that
the server accepts POSTs to any path; modify the startup output text accordingly
and ensure references to the endpoint in the help HTML are consistent with the
chosen behavior.
In @dev-tools/mcp-mock-server/test_mock_mcp_server.py:
- Around line 22-55: The mock_server pytest fixture is flaky because it uses
fixed ports (http_port/https_port), a blind time.sleep, and pipes stdout/stderr
without draining; make it robust by selecting free ephemeral ports (use a
temporary socket to bind and get a free port and set http_port/https_port),
start the subprocess via subprocess.Popen([... , str(http_port)]) but avoid
stdout=PIPE/stderr=PIPE (or if kept, spawn a thread to continuously read/drain
them), replace the fixed time.sleep with a readiness poll loop that repeatedly
attempts to connect to the started server URL (http_url) and checks
process.poll() to fail fast, and keep the cleanup logic
(process.terminate()/wait()/kill()) unchanged.
In @Makefile:
- Around line 34-36: The Makefile target security-check should invoke Bandit via
the uv-managed environment like the rest of the toolchain; replace the direct
call to "bandit -c pyproject.toml -r src tests dev-tools" in the
"security-check" target with "uv run bandit -c pyproject.toml -r src tests
dev-tools" (preserve the Makefile tab indentation and keep the target name
security-check unchanged).
In @README.md:
- Around line 1100-1120: The README and MANUAL_TESTING.md (and tests) use
inconsistent mock MCP server ports causing setup failures; pick a single port
(either 3000 or 9000), then update the dev-tools/mcp-mock-server/server.py
startup/default, the README.md Quick Start command and the mcp_servers.url
example (the Authorization key path can remain), and all references in
MANUAL_TESTING.md and any tests that hard-code ports 9000/9001 so they all use
the chosen port consistently (search for "dev-tools/mcp-mock-server/server.py",
"mcp_servers", "http://localhost:3000" / "http://localhost:9000", and test files
that mention 9000/9001 and change them to the selected port).
🧹 Nitpick comments (7)
dev-tools/MANUAL_TESTING.md (1)
22-30: Consider replacing the hugeuv pip install ...list with a repo-native install path.
If this must stay, it’d help to explain whyuv sync --group ...isn’t sufficient here (and ideally reference the exact group(s) needed). Based on learnings.tests/unit/utils/test_mcp_auth_headers.py (1)
40-47: If you want to assert “logs warning/error”, usecaploginstead of comments.
Right now these tests only assert the returned dict, not the promised log behavior.Also applies to: 106-115
README.md (1)
328-349: Use non-secret placeholders in auth/token examples to avoid scanner noise.
E.g., avoidBearer sk-...in docs; preferBearer <service-token>orBearer ${TOKEN}patterns.Also applies to: 380-385
dev-tools/mcp-mock-server/test_mock_mcp_server.py (1)
57-74: Avoid mutating caller-providedheadersinmake_request.
req_headers = headers or {}+ later writes will modify the input dict when provided.Proposed fix
- req_headers = headers or {} + req_headers = dict(headers or {})dev-tools/mcp-mock-server/server.py (3)
29-31: Consider thread-safety for concurrent requests.The global variables
last_headersandrequest_logare modified without synchronization in_capture_headers()(lines 44-62). Concurrent requests from HTTP and HTTPS servers could race when clearing/updatinglast_headersor appending/popping fromrequest_log, potentially causing data corruption or lost capture data.For a dev/test mock server, this may be acceptable. However, if you expect concurrent testing scenarios, consider adding a
threading.Lock()to protect these shared resources.🔒 Optional fix: Add thread safety
+import threading + +# Global storage for captured headers (last request) +last_headers: dict[str, str] = {} +request_log: list = [] +_lock = threading.Lock()Then wrap the modifications in
_capture_headers:def _capture_headers(self) -> None: """Capture all headers from the request.""" + with _lock: last_headers.clear() # Capture all headers for debugging for header_name, value in self.headers.items(): last_headers[header_name] = value # Log the request request_log.append( { "timestamp": datetime.now().isoformat(), "method": self.command, "path": self.path, "headers": dict(last_headers), } ) # Keep only last 10 requests if len(request_log) > 10: request_log.pop(0)
37-40: Consider using proper logging levels.The custom
log_messagemethod useslogger.info()for general execution info like request logging.For a simple dev tool, print statements are pragmatic. However, proper logging would enable better control over verbosity during testing.
253-268: Consider more specific exception handling.The broad
except Exceptionhandlers catch all errors but may hide bugs during development. While acceptable for a simple mock server, more specific exception types (e.g.,OSError,socket.error) would make troubleshooting easier.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
MakefileREADME.mddev-tools/MANUAL_TESTING.mddev-tools/mcp-mock-server/README.mddev-tools/mcp-mock-server/server.pydev-tools/mcp-mock-server/test_mock_mcp_server.pysrc/app/endpoints/query_v2.pysrc/utils/mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.py
🚧 Files skipped from review as they are similar to previous changes (3)
- dev-tools/mcp-mock-server/README.md
- src/utils/mcp_auth_headers.py
- tests/unit/models/config/test_model_context_protocol_server.py
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Usefrom llama_stack_client import AsyncLlamaStackClientfor Llama Stack integration
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
Use modern union syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]orType | Nonefor optional types
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification: return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in API calls
Useimport loggingand module logger pattern:logger = logging.getLogger(__name__)
Use standard log levels:logger.debug()for detailed diagnostic info,logger.info()for general execution info,logger.warning()for unexpected events,logger.error()for serious problems
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes:Configurationfor config,Error/Exceptionfor exceptions,Resolverfor strategy pattern,Interfacefor abstract base classes
Use ABC (Abstract Base Class) with@abstractmethoddecorators for interface definitions
Include complete type annotations for all class attributes
Follow Google Python docstring conventions: https://google.github.io/styleguide/pyguide.html with sections for Args, Returns, Raises, and Attributes
Use black for code formatting
Use pylint for static analysis with source-roots = "src"
Use pyright for type checking (excludessrc/authentication/k8s.py)
Use ruff for fast linting
Use pydocstyle for docstring style validation
U...
Files:
src/app/endpoints/query_v2.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI dependencies:
from fastapi import APIRouter, HTTPException, Request, status, Depends
Files:
src/app/endpoints/query_v2.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query_v2.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all unit and integration tests
Files:
tests/unit/utils/test_mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Do not use unittest - pytest is the standard for this project
Usepytest-mockfor AsyncMock objects in tests
Files:
tests/unit/utils/test_mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.py
🧠 Learnings (14)
📚 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:
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:
README.md
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use standard log levels: `logger.debug()` for detailed diagnostic info, `logger.info()` for general execution info, `logger.warning()` for unexpected events, `logger.error()` for serious problems
Applied to files:
dev-tools/mcp-mock-server/server.py
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to tests/**/*.py : Use `pytest-mock` for AsyncMock objects in tests
Applied to files:
dev-tools/mcp-mock-server/test_mock_mcp_server.py
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use pyright for type checking (excludes `src/authentication/k8s.py`)
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use bandit for security issue detection
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use black for code formatting
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Run `uv run make verify` to run all linters before completion
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Run `uv run make format` to auto-format code with black and ruff before completion
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use ruff for fast linting
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use pylint for static analysis with source-roots = "src"
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Always use `uv run` prefix for commands
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use pydocstyle for docstring style validation
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Handle `APIConnectionError` from Llama Stack in API calls
Applied to files:
tests/unit/app/endpoints/test_query_v2.py
🧬 Code graph analysis (3)
src/app/endpoints/query_v2.py (2)
src/models/config.py (2)
config(325-342)ModelContextProtocolServer(425-511)src/configuration.py (1)
mcp_servers(151-162)
tests/unit/utils/test_mcp_auth_headers.py (1)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-81)
tests/unit/app/endpoints/test_query_v2.py (2)
src/models/config.py (1)
ModelContextProtocolServer(425-511)src/app/endpoints/query_v2.py (1)
get_mcp_tools(675-765)
🪛 Gitleaks (8.30.0)
dev-tools/MANUAL_TESTING.md
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 115-117: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
README.md
[style] ~297-~297: Consider using “inaccessible” to avoid wordiness.
Context: ...gured in the llama-stack run.yaml are not accessible to lightspeed-core agents. #### Config...
(NOT_ABLE_PREMIUM)
⏰ 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). (2)
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (5)
Makefile (1)
31-33: Good: expanding type/lint/docstyle coverage todev-tools/.
This aligns with introducing executable tooling underdev-tools/and prevents bitrot.Also applies to: 84-92
tests/unit/utils/test_mcp_auth_headers.py (1)
1-115: Solid unit coverage for file-backed + keyword-preserved auth header resolution.tests/unit/app/endpoints/test_query_v2.py (1)
57-231: Nice: tests cover kubernetes/client/static/mixed headers + skip behavior.
This should prevent regressions inget_mcp_toolsandretrieve_responsetool assembly.Also applies to: 298-304, 671-672
dev-tools/mcp-mock-server/server.py (2)
205-250: LGTM: Certificate generation is secure and well-handled.The certificate generation logic correctly:
- Checks for existing certificates before regenerating
- Uses subprocess safely with hardcoded commands (no injection risk)
- Handles errors appropriately with
check=Trueand exception catching- Creates directories as needed
271-327: LGTM: Server lifecycle management is well-structured.The main function correctly:
- Sets up both HTTP and HTTPS servers on consecutive ports
- Uses daemon threads to run servers concurrently
- Handles graceful shutdown on KeyboardInterrupt
- Provides clear startup information to users
| @pytest.fixture(scope="module") | ||
| def mock_server() -> Any: | ||
| """Start mock server for testing and stop it after tests complete.""" | ||
| http_port = 9000 | ||
| https_port = 9001 | ||
|
|
||
| print(f"\n🚀 Starting mock server on ports {http_port}/{https_port}...") | ||
| with subprocess.Popen( | ||
| [sys.executable, "dev-tools/mcp-mock-server/server.py", str(http_port)], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| ) as process: | ||
| # Wait for server to start | ||
| time.sleep(2) | ||
|
|
||
| # Check if server started successfully | ||
| if process.poll() is not None: | ||
| _, stderr = process.communicate() | ||
| pytest.fail(f"Server failed to start: {stderr.decode('utf-8')}") | ||
|
|
||
| yield { | ||
| "process": process, | ||
| "http_url": f"http://localhost:{http_port}", | ||
| "https_url": f"https://localhost:{https_port}", | ||
| } | ||
|
|
||
| # Cleanup: stop server | ||
| print("\n🛑 Stopping mock server...") | ||
| process.terminate() | ||
| try: | ||
| process.wait(timeout=5) | ||
| except subprocess.TimeoutExpired: | ||
| process.kill() | ||
|
|
There was a problem hiding this comment.
Test fixture is prone to flakiness: fixed ports + sleep + piped output.
Recommend: pick free ports, poll for readiness, and avoid stdout/stderr=PIPE unless you actively drain.
Sketch of a more robust fixture approach
@@
def mock_server() -> Any:
@@
- http_port = 9000
- https_port = 9001
+ # Consider allocating free ports to avoid conflicts.
+ http_port = 9000
+ https_port = 9001
@@
with subprocess.Popen(
[sys.executable, "dev-tools/mcp-mock-server/server.py", str(http_port)],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
+ stdout=subprocess.DEVNULL,
+ stderr=subprocess.DEVNULL,
) as process:
- # Wait for server to start
- time.sleep(2)
+ # Prefer polling a health/debug endpoint instead of sleeping.
+ time.sleep(2)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @dev-tools/mcp-mock-server/test_mock_mcp_server.py around lines 22 - 55, The
mock_server pytest fixture is flaky because it uses fixed ports
(http_port/https_port), a blind time.sleep, and pipes stdout/stderr without
draining; make it robust by selecting free ephemeral ports (use a temporary
socket to bind and get a free port and set http_port/https_port), start the
subprocess via subprocess.Popen([... , str(http_port)]) but avoid
stdout=PIPE/stderr=PIPE (or if kept, spawn a thread to continuously read/drain
them), replace the fixed time.sleep with a readiness poll loop that repeatedly
attempts to connect to the started server URL (http_url) and checks
process.poll() to fail fast, and keep the cleanup logic
(process.terminate()/wait()/kill()) unchanged.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of the mock_server fixture in the test file
cat dev-tools/mcp-mock-server/test_mock_mcp_server.py | head -60
Length of output: 1590
@blublinsky I don't see the fix in the current version of the file. The code at dev-tools/mcp-mock-server/test_mock_mcp_server.py still shows:
- Fixed ports:
http_port = 9000andhttps_port = 9001(line 25-26) - Blind sleep:
time.sleep(2)(line 35) - Piped output:
stdout=subprocess.PIPE, stderr=subprocess.PIPE(line 31-32)
Could you verify that your changes were committed and pushed to the PR branch? The fixture would need updates like:
- Dynamic port allocation (e.g., binding to port 0 and getting the assigned port)
- Polling for server readiness instead of
time.sleep(2) - Using
subprocess.DEVNULLfor stdout/stderr or draining the pipes in a separate thread
🧠 Learnings used
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to tests/**/*.py : Use `pytest-mock` for AsyncMock objects in tests
blublinsky
force-pushed
the
mcp-headers
branch
2 times, most recently
from
8fa6a7d to
bf1a695
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
33-36: Fix ToC vs section heading mismatch (broken anchor risk).
The ToC entry “Configuring MCP Headers” doesn’t match the actual heading “Configuring MCP Server Authentication”, so the link likely won’t jump correctly.Also applies to: 324-325
🤖 Fix all issues with AI agents
In @dev-tools/MANUAL_TESTING.md:
- Around line 53-60: Replace hardcoded bearer tokens in the curl example by
using placeholders so secrets aren’t committed; update the Authorization header
value "Bearer my-k8s-token" to something like "Bearer <K8S_TOKEN>" (or
${K8S_TOKEN}) and update the MCP-HEADERS JSON value "Authorization": "Bearer
my-client-token" to "Authorization": "Bearer <CLIENT_TOKEN>" (or
${CLIENT_TOKEN}); apply the same placeholder changes to the other examples that
use these tokens (e.g., the second occurrence around the MCP-HEADERS example).
In @dev-tools/mcp-mock-server/README.md:
- Around line 96-99: Update the curl examples to avoid hardcoded-looking bearer
tokens by replacing "Authorization: Bearer user-token" with a placeholder such
as "Authorization: Bearer <TOKEN>" (or "Bearer REDACTED") in the shown curl
command and the other occurrences noted (around the second example at lines
208-213); ensure all README curl examples use the placeholder string instead of
any token-like literal.
- Around line 39-53: Add an explicit language specifier ("text") to the fenced
code block under the "You should see:" section so the Markdown linter MD040
passes; locate the "You should see:" fenced block in the README and change the
opening fence from ``` to ```text and ensure the ASCII server output content
remains inside that fenced block exactly as shown in the diff.
In @dev-tools/mcp-mock-server/test_mock_mcp_server.py:
- Around line 73-90: The make_request function mutates the caller-provided
headers dict when adding "Content-Type", which can leak across tests; fix by
copying the headers input before modification (e.g., set req_headers =
dict(headers) if headers is not None else {}) so you never modify the original,
then add "Content-Type" to req_headers when req_data is set and proceed to build
the urllib.request.Request as before.
In @src/utils/mcp_auth_headers.py:
- Around line 9-80: In resolve_authorization_headers, stop logging raw header
values and treat whitespace-only secrets as unresolved: after reading
secret_value = secret_file.read().strip(), only assign resolved[header_name] if
secret_value is non-empty; otherwise emit a warning (e.g., "Empty secret for
header %s, skipping") and do not set the header. Also change the exception
logging in the except block to avoid including the raw value variable; log only
the header_name and the exception (e.g., "Failed to resolve authorization header
%s: %s") to prevent secret leakage.
🧹 Nitpick comments (4)
README.md (2)
297-298: Tighten wording (“not accessible” → “inaccessible” / “unavailable”).
Keeps the sentence shorter and matches LanguageTool’s suggestion.
1109-1120: Doc example: consider marking the embedded YAML as YAML (readability + consistency).
The “Quick Start” block mixes bash + YAML; adding a fenced ```yaml block around the config snippet makes it easier to copy/paste.src/utils/mcp_auth_headers.py (1)
12-36: Align docstring with Google style (Args/Returns/Raises).
This repo’s guideline explicitly calls for Google Python docstrings.src/app/endpoints/query_v2.py (1)
709-727: Consider passingmcp_serveras a parameter to improve clarity.The nested function
_get_token_valueaccessesmcp_serverfrom the outer loop scope via closure (line 721). While valid Python, this implicit coupling can make the code harder to understand and test.♻️ Refactor to make the dependency explicit
- def _get_token_value(original: str, header: str) -> str | None: + def _get_token_value(original: str, header: str, server_name: str) -> str | None: """Convert to header value.""" match original: case "kubernetes": # use k8s token if token is None or token == "": return None return f"Bearer {token}" case "client": # use client provided token if mcp_headers is None: return None - c_headers = mcp_headers.get(mcp_server.name, None) + c_headers = mcp_headers.get(server_name, None) if c_headers is None: return None return c_headers.get(header, None) case _: # use provided return originalThen update the call site:
for name, value in mcp_server.resolved_authorization_headers.items(): # for each defined header - h_value = _get_token_value(value, name) + h_value = _get_token_value(value, name, mcp_server.name) # only add the header if we got value if h_value is not None: headers[name] = h_value
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
MakefileREADME.mddev-tools/MANUAL_TESTING.mddev-tools/mcp-mock-server/README.mddev-tools/mcp-mock-server/server.pydev-tools/mcp-mock-server/test_mock_mcp_server.pysrc/app/endpoints/query_v2.pysrc/utils/mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.py
🚧 Files skipped from review as they are similar to previous changes (3)
- dev-tools/mcp-mock-server/server.py
- tests/unit/utils/test_mcp_auth_headers.py
- tests/unit/models/config/test_model_context_protocol_server.py
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Usefrom llama_stack_client import AsyncLlamaStackClientfor Llama Stack integration
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
Use modern union syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]orType | Nonefor optional types
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification: return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in API calls
Useimport loggingand module logger pattern:logger = logging.getLogger(__name__)
Use standard log levels:logger.debug()for detailed diagnostic info,logger.info()for general execution info,logger.warning()for unexpected events,logger.error()for serious problems
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes:Configurationfor config,Error/Exceptionfor exceptions,Resolverfor strategy pattern,Interfacefor abstract base classes
Use ABC (Abstract Base Class) with@abstractmethoddecorators for interface definitions
Include complete type annotations for all class attributes
Follow Google Python docstring conventions: https://google.github.io/styleguide/pyguide.html with sections for Args, Returns, Raises, and Attributes
Use black for code formatting
Use pylint for static analysis with source-roots = "src"
Use pyright for type checking (excludessrc/authentication/k8s.py)
Use ruff for fast linting
Use pydocstyle for docstring style validation
U...
Files:
src/app/endpoints/query_v2.pysrc/utils/mcp_auth_headers.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI dependencies:
from fastapi import APIRouter, HTTPException, Request, status, Depends
Files:
src/app/endpoints/query_v2.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query_v2.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all unit and integration tests
Files:
tests/unit/app/endpoints/test_query_v2.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Do not use unittest - pytest is the standard for this project
Usepytest-mockfor AsyncMock objects in tests
Files:
tests/unit/app/endpoints/test_query_v2.py
🧠 Learnings (13)
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use pyright for type checking (excludes `src/authentication/k8s.py`)
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use bandit for security issue detection
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Always use `uv run` prefix for commands
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Run `uv run make verify` to run all linters before completion
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Run `uv run make format` to auto-format code with black and ruff before completion
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use black for code formatting
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use ruff for fast linting
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use pylint for static analysis with source-roots = "src"
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use pydocstyle for docstring style validation
Applied to files:
Makefile
📚 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:
dev-tools/MANUAL_TESTING.md
📚 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:
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:
README.md
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Handle `APIConnectionError` from Llama Stack in API calls
Applied to files:
tests/unit/app/endpoints/test_query_v2.py
🧬 Code graph analysis (2)
src/app/endpoints/query_v2.py (2)
src/models/config.py (2)
config(325-342)ModelContextProtocolServer(425-511)src/configuration.py (1)
mcp_servers(151-162)
tests/unit/app/endpoints/test_query_v2.py (2)
src/models/config.py (1)
ModelContextProtocolServer(425-511)src/app/endpoints/query_v2.py (1)
get_mcp_tools(675-765)
🪛 Gitleaks (8.30.0)
dev-tools/MANUAL_TESTING.md
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 139-141: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
dev-tools/mcp-mock-server/README.md
[high] 96-98: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 208-210: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
README.md
[style] ~297-~297: Consider using “inaccessible” to avoid wordiness.
Context: ...gured in the llama-stack run.yaml are not accessible to lightspeed-core agents. #### Config...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
dev-tools/mcp-mock-server/README.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (12)
Makefile (1)
32-32: LGTM! Consistent quality tool coverage for dev-tools.The additions systematically extend static analysis, type checking, security scanning, and docstring validation to the new
dev-toolsdirectory. The path formats are consistent within each target (trailing slash for mypy, no slash for others), and the changes align with the PR's introduction of new development utilities.dev-tools/mcp-mock-server/test_mock_mcp_server.py (1)
119-155: No issue here. The mock server'sdo_POST()handler accepts POST requests to any path and routes based on the JSON-RPCmethodfield in the request body, not the URL path. Both test cases POST to/and the server handles them correctly. The implementation explicitly supports this flexibility (documented as "POST to any path"), so there's no risk of intermittent breakage.dev-tools/MANUAL_TESTING.md (1)
34-39: The documented startup commanduv run src/lightspeed_stack.py --config dev-tools/test-configs/mcp-mock-test.yamlis correct and consistent with the Makefile and other documentation. This matches the pattern used in the Makefile (uv run src/lightspeed_stack.py) and docs/getting_started.md. No changes needed.Likely an incorrect or invalid review comment.
README.md (1)
425-435: The server skipping warning is already secure. At query_v2.py line 753, the log only includes the server name and header counts—no tokens or secrets are exposed.However, there is a secondary logging concern in
src/utils/mcp_auth_headers.py(lines 62–71): secret file paths are logged during resolution. While these are not the secrets themselves, full secret file paths can reveal infrastructure details. Consider masking or abbreviating paths in debug/warning messages, e.g., logging only the filename or a generic reference rather than the full path.src/app/endpoints/query_v2.py (4)
25-25: LGTM!The import addition is necessary for the updated
get_mcp_toolssignature and follows the project's absolute import pattern.
80-80: LGTM!Minor docstring punctuation fix improves documentation quality.
466-468: LGTM!The None-check is a good defensive programming practice that prevents potential
AttributeErrorwhen the response is missing, improving robustness.
749-758: LGTM!The skip logic correctly handles servers with incomplete authentication headers by logging a warning and excluding them from the tool list. This is important for security and prevents misconfigured servers from being used.
tests/unit/app/endpoints/test_query_v2.py (4)
1-2: Verify that broad linter suppressions are necessary.The
import-errorandreportCallIssue=falsedirectives suppress potentially useful warnings. Ensure these are actually needed and not masking real issues that should be addressed.If these are temporary workarounds, consider adding comments explaining why they're needed or tracking them for future removal.
57-230: LGTM!Excellent test coverage for the per-server authorization header functionality. The tests cover:
- Servers without authentication
- Kubernetes token-based auth
- Client-provided headers
- Static headers from configuration files
- Mixed header types
- Skip behavior for incomplete authentication
The tests are well-structured, use appropriate pytest fixtures, and validate both positive and negative scenarios.
299-327: LGTM!The test correctly validates that MCP servers with kubernetes authentication receive the properly formatted Bearer token in their headers when a token is provided.
671-671: LGTM!The assertion correctly validates the user-facing error message for Llama Stack connection failures.
| ```bash | ||
| # Terminal 3: Make test query with all required headers | ||
| curl -X POST http://localhost:8080/v1/streaming_query \ | ||
| -H "Content-Type: application/json" \ | ||
| -H "Authorization: Bearer my-k8s-token" \ | ||
| -H 'MCP-HEADERS: {"mock-client-auth": {"Authorization": "Bearer my-client-token"}}' \ | ||
| -d '{"query": "Test all MCP auth types"}' | ||
| ``` |
There was a problem hiding this comment.
Replace bearer tokens in curl examples with placeholders (gitleaks).
Proposed fix
curl -X POST http://localhost:8080/v1/streaming_query \
-H "Content-Type: application/json" \
- -H "Authorization: Bearer my-k8s-token" \
- -H 'MCP-HEADERS: {"mock-client-auth": {"Authorization": "Bearer my-client-token"}}' \
+ -H "Authorization: Bearer <K8S_TOKEN>" \
+ -H 'MCP-HEADERS: {"mock-client-auth": {"Authorization": "Bearer <CLIENT_TOKEN>"}}' \
-d '{"query": "Test all MCP auth types"}'
@@
curl -X POST http://localhost:8080/v1/streaming_query \
-H "Content-Type: application/json" \
- -H "Authorization: Bearer my-k8s-token" \
+ -H "Authorization: Bearer <K8S_TOKEN>" \
-d '{"query": "Test without client headers"}'Also applies to: 138-143
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🤖 Prompt for AI Agents
In @dev-tools/MANUAL_TESTING.md around lines 53 - 60, Replace hardcoded bearer
tokens in the curl example by using placeholders so secrets aren’t committed;
update the Authorization header value "Bearer my-k8s-token" to something like
"Bearer <K8S_TOKEN>" (or ${K8S_TOKEN}) and update the MCP-HEADERS JSON value
"Authorization": "Bearer my-client-token" to "Authorization": "Bearer
<CLIENT_TOKEN>" (or ${CLIENT_TOKEN}); apply the same placeholder changes to the
other examples that use these tokens (e.g., the second occurrence around the
MCP-HEADERS example).
| curl -X POST http://localhost:8080/v1/query \ | ||
| -H "Content-Type: application/json" \ | ||
| -H "Authorization: Bearer user-token" \ | ||
| -d '{"query": "Test MCP tools"}' |
There was a problem hiding this comment.
Replace bearer tokens in curl examples with placeholders (gitleaks).
Even fake-looking tokens can trip secret scanners; prefer <TOKEN>/REDACTED.
Proposed fix
curl -X POST http://localhost:8080/v1/query \
-H "Content-Type: application/json" \
- -H "Authorization: Bearer user-token" \
+ -H "Authorization: Bearer <USER_TOKEN>" \
-d '{"query": "Test MCP tools"}'
@@
curl -X POST http://localhost:8080/v1/query \
-H "Content-Type: application/json" \
- -H "Authorization: Bearer user-k8s-token" \
+ -H "Authorization: Bearer <K8S_TOKEN>" \
-H 'MCP-HEADERS: {"client-auth-test": {"Authorization": "Bearer client-custom-token"}}' \
-d '{"query": "Test"}'Also applies to: 208-213
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 96-98: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🤖 Prompt for AI Agents
In @dev-tools/mcp-mock-server/README.md around lines 96 - 99, Update the curl
examples to avoid hardcoded-looking bearer tokens by replacing "Authorization:
Bearer user-token" with a placeholder such as "Authorization: Bearer <TOKEN>"
(or "Bearer REDACTED") in the shown curl command and the other occurrences noted
(around the second example at lines 208-213); ensure all README curl examples
use the placeholder string instead of any token-like literal.
| @pytest.fixture(scope="module") | ||
| def mock_server() -> Any: | ||
| """Start mock server for testing and stop it after tests complete.""" | ||
| # Using fixed ports for simplicity. For parallel test execution, | ||
| # consider using dynamic port allocation (e.g., bind to port 0). | ||
| http_port = 9000 | ||
| https_port = 9001 | ||
|
|
||
| print(f"\n🚀 Starting mock server on ports {http_port}/{https_port}...") | ||
| # Keep stdout/stderr as PIPE to capture errors if startup fails | ||
| with subprocess.Popen( | ||
| [sys.executable, "dev-tools/mcp-mock-server/server.py", str(http_port)], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| ) as process: | ||
| # Poll server health endpoint instead of blind sleep | ||
| max_attempts = 10 | ||
| for attempt in range(max_attempts): | ||
| if process.poll() is not None: | ||
| # Server crashed during startup | ||
| _, stderr = process.communicate() | ||
| pytest.fail(f"Server failed to start: {stderr.decode('utf-8')}") | ||
|
|
||
| # Try to connect to health endpoint | ||
| try: | ||
| status, _ = make_request(f"http://localhost:{http_port}/", method="GET") | ||
| if status == 200: | ||
| print(f"✅ Server ready after {attempt + 1} attempt(s)") | ||
| break | ||
| except Exception: # pylint: disable=broad-except | ||
| time.sleep(0.5) | ||
| else: | ||
| # Timeout waiting for server | ||
| process.terminate() | ||
| pytest.fail(f"Server did not respond after {max_attempts} attempts") | ||
|
|
||
| yield { | ||
| "process": process, | ||
| "http_url": f"http://localhost:{http_port}", | ||
| "https_url": f"https://localhost:{https_port}", | ||
| } | ||
|
|
||
| # Cleanup: stop server | ||
| print("\n🛑 Stopping mock server...") | ||
| process.terminate() | ||
| try: | ||
| process.wait(timeout=5) | ||
| except subprocess.TimeoutExpired: | ||
| process.kill() | ||
|
|
There was a problem hiding this comment.
Avoid fixed ports + unconsumed subprocess PIPEs (flaky tests).
Using hard-coded 9000/9001 and leaving stdout/stderr unconsumed can fail under parallel runs or if the server prints enough output to fill buffers.
Proposed change (pick free ports + don’t risk PIPE deadlock)
@pytest.fixture(scope="module")
def mock_server() -> Any:
@@
- http_port = 9000
- https_port = 9001
+ # Pick free ports to reduce CI flakiness.
+ import socket # pylint: disable=import-outside-toplevel
+
+ def _free_port() -> int:
+ with socket.socket() as s:
+ s.bind(("127.0.0.1", 0))
+ return int(s.getsockname()[1])
+
+ http_port = _free_port()
+ https_port = _free_port()
@@
with subprocess.Popen(
[sys.executable, "dev-tools/mcp-mock-server/server.py", str(http_port)],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
+ # Avoid PIPE deadlocks; tests can still fail with a useful message via return code.
+ stdout=subprocess.DEVNULL,
+ stderr=subprocess.STDOUT,
) as process:Committable suggestion skipped: line range outside the PR's diff.
| def make_request( | ||
| url: str, | ||
| method: str = "GET", | ||
| data: dict[str, Any] | None = None, | ||
| headers: dict[str, str] | None = None, | ||
| use_https: bool = False, | ||
| ) -> tuple[int, dict[str, Any] | str]: | ||
| """Make HTTP request and return status code and response.""" | ||
| req_headers = headers or {} | ||
| req_data = None | ||
| if data: | ||
| req_data = json.dumps(data).encode("utf-8") | ||
| req_headers["Content-Type"] = "application/json" | ||
|
|
||
| request = urllib.request.Request( | ||
| url, data=req_data, headers=req_headers, method=method | ||
| ) | ||
|
|
There was a problem hiding this comment.
Don’t mutate caller-provided headers dict in make_request().
Right now adding Content-Type can leak across tests if the same dict instance is reused.
Proposed fix
def make_request(
@@
) -> tuple[int, dict[str, Any] | str]:
@@
- req_headers = headers or {}
+ req_headers = dict(headers) if headers else {}🤖 Prompt for AI Agents
In @dev-tools/mcp-mock-server/test_mock_mcp_server.py around lines 73 - 90, The
make_request function mutates the caller-provided headers dict when adding
"Content-Type", which can leak across tests; fix by copying the headers input
before modification (e.g., set req_headers = dict(headers) if headers is not
None else {}) so you never modify the original, then add "Content-Type" to
req_headers when req_data is set and proceed to build the urllib.request.Request
as before.
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/models/config/test_model_context_protocol_server.py (1)
11-17: Use absolute imports for internal modules.As per coding guidelines, internal module imports should use absolute import paths starting from the project root, not relative imports with type ignore comments.
♻️ Proposed fix
-from models.config import ( # type: ignore[import-not-found] +from src.models.config import ( ModelContextProtocolServer, LlamaStackConfiguration, UserDataCollection, Configuration, ServiceConfiguration, )Based on coding guidelines, internal modules should use absolute imports.
🤖 Fix all issues with AI agents
In @dev-tools/mcp-mock-server/server.py:
- Around line 205-250: The README incorrectly claims no external deps while
generate_self_signed_cert uses the openssl binary; update the README to document
that an OpenSSL executable must be installed and on PATH. In
generate_self_signed_cert, improve error handling by catching FileNotFoundError
to emit a clear message that the openssl binary was not found and by catching
subprocess.CalledProcessError and including the process stderr/stdout in the
error log for cert_file/key_file generation failures (refer to subprocess.run,
cert_file, key_file), then re-raise or raise a more descriptive RuntimeError so
callers get actionable feedback.
In @tests/unit/utils/test_mcp_auth_headers.py:
- Line 6: The test imports the internal util using a relative/short module path;
update the import to the project's absolute module path so internal modules use
absolute imports (replace the current "from utils.mcp_auth_headers import
resolve_authorization_headers" with the project-root absolute import that
exposes resolve_authorization_headers, e.g. using the package name followed by
.utils.mcp_auth_headers).
🧹 Nitpick comments (4)
dev-tools/mcp-mock-server/README.md (1)
39-53: Add language specification to fenced code block.The code block showing expected server output should specify a language for proper syntax highlighting and linting compliance.
📝 Proposed fix
-``` +```text ====================================================================== MCP Mock Server starting with HTTP and HTTPS ======================================================================src/utils/mcp_auth_headers.py (1)
17-18: Fix typos in docstring.Two minor typos in the parameter documentation:
- Line 17: "leave is unchanged" should be "leave it unchanged"
- Line 18: Double period "unchanged. ." should be "unchanged."
📝 Proposed fix
Parameters: authorization_headers: Map of header names to secret locations or special keywords. - - If value is "kubernetes": leave is unchanged. We substitute it during request. - - If value is "client": leave it unchanged. . We substitute it during request. + - If value is "kubernetes": leave it unchanged. We substitute it during request. + - If value is "client": leave it unchanged. We substitute it during request. - Otherwise: Treat as file path and read the secret from that filedev-tools/mcp-mock-server/test_mock_mcp_server.py (1)
91-98: Consider movingsslimport to the top of the file.The
sslimport inside the function works but is unconventional. Since this is a dev tool and the import is only used whenuse_https=True, this is acceptable, but moving it to the top would improve readability.dev-tools/mcp-mock-server/server.py (1)
29-31: Consider thread-safety for global state.The
last_headersandrequest_logare shared mutable state accessed by both HTTP and HTTPS server threads. For a dev tool, this is acceptable, but be aware that concurrent requests may cause race conditions when reading debug endpoints.If thread-safety becomes a concern, consider using
threading.Lock:🔒 Optional thread-safe implementation
+import threading + # Global storage for captured headers (last request) +_lock = threading.Lock() last_headers: dict[str, str] = {} request_log: list = []Then wrap modifications and reads with
with _lock:.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
MakefileREADME.mddev-tools/MANUAL_TESTING.mddev-tools/mcp-mock-server/README.mddev-tools/mcp-mock-server/server.pydev-tools/mcp-mock-server/test_mock_mcp_server.pysrc/app/endpoints/query_v2.pysrc/utils/mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
📓 Path-based instructions (5)
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all unit and integration tests
Files:
tests/unit/app/endpoints/test_query_v2.pytests/unit/utils/test_mcp_auth_headers.pytests/unit/models/config/test_model_context_protocol_server.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Do not use unittest - pytest is the standard for this project
Usepytest-mockfor AsyncMock objects in tests
Files:
tests/unit/app/endpoints/test_query_v2.pytests/unit/utils/test_mcp_auth_headers.pytests/unit/models/config/test_model_context_protocol_server.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Usefrom llama_stack_client import AsyncLlamaStackClientfor Llama Stack integration
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
Use modern union syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]orType | Nonefor optional types
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification: return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in API calls
Useimport loggingand module logger pattern:logger = logging.getLogger(__name__)
Use standard log levels:logger.debug()for detailed diagnostic info,logger.info()for general execution info,logger.warning()for unexpected events,logger.error()for serious problems
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes:Configurationfor config,Error/Exceptionfor exceptions,Resolverfor strategy pattern,Interfacefor abstract base classes
Use ABC (Abstract Base Class) with@abstractmethoddecorators for interface definitions
Include complete type annotations for all class attributes
Follow Google Python docstring conventions: https://google.github.io/styleguide/pyguide.html with sections for Args, Returns, Raises, and Attributes
Use black for code formatting
Use pylint for static analysis with source-roots = "src"
Use pyright for type checking (excludessrc/authentication/k8s.py)
Use ruff for fast linting
Use pydocstyle for docstring style validation
U...
Files:
src/app/endpoints/query_v2.pysrc/utils/mcp_auth_headers.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI dependencies:
from fastapi import APIRouter, HTTPException, Request, status, Depends
Files:
src/app/endpoints/query_v2.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query_v2.py
🧠 Learnings (4)
📚 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:
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:
README.md
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Handle `APIConnectionError` from Llama Stack in API calls
Applied to files:
tests/unit/app/endpoints/test_query_v2.py
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use standard log levels: `logger.debug()` for detailed diagnostic info, `logger.info()` for general execution info, `logger.warning()` for unexpected events, `logger.error()` for serious problems
Applied to files:
dev-tools/mcp-mock-server/server.py
🧬 Code graph analysis (4)
tests/unit/app/endpoints/test_query_v2.py (2)
src/models/config.py (1)
ModelContextProtocolServer(425-511)src/app/endpoints/query_v2.py (1)
get_mcp_tools(675-765)
tests/unit/utils/test_mcp_auth_headers.py (1)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-81)
tests/unit/models/config/test_model_context_protocol_server.py (1)
src/models/config.py (3)
config(325-342)ModelContextProtocolServer(425-511)Configuration(1440-1554)
src/app/endpoints/query_v2.py (2)
src/models/config.py (3)
config(325-342)Action(810-863)ModelContextProtocolServer(425-511)src/configuration.py (1)
mcp_servers(151-162)
🪛 Gitleaks (8.30.0)
dev-tools/MANUAL_TESTING.md
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 139-141: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
dev-tools/mcp-mock-server/README.md
[high] 96-98: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 208-210: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
README.md
[style] ~297-~297: Consider using “inaccessible” to avoid wordiness.
Context: ...gured in the llama-stack run.yaml are not accessible to lightspeed-core agents. #### Config...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
dev-tools/mcp-mock-server/README.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (16)
dev-tools/mcp-mock-server/test_mock_mcp_server.py (2)
22-70: Well-structured test fixture with proper health-check polling.The
mock_serverfixture correctly implements server lifecycle management with health-check polling instead of blind sleep, proper error handling for startup failures, and graceful cleanup with timeout handling.
119-297: Good test coverage for mock server functionality.The tests comprehensively cover HTTP/HTTPS endpoints, header capturing, request logging, and counting. The assertions include descriptive error messages which aids debugging.
README.md (3)
297-298: Documentation is clear and comprehensive.The updated MCP server configuration documentation clearly explains the three authentication methods, provides practical examples, and includes a helpful comparison table. The automatic server skipping behavior is well-documented.
378-389: Clear explanation of MCP-HEADERS structure.The note clarifying that
MCP-HEADERSis keyed by server name (not URL) is particularly helpful for avoiding common misconfiguration.
1100-1129: Helpful developer tooling documentation.The Development Tools section provides a clear quick start for the MCP mock server with practical configuration examples.
tests/unit/app/endpoints/test_query_v2.py (4)
57-83: Good test coverage for MCP tools authorization scenarios.The tests properly cover both servers without authorization and servers with kubernetes-style authentication, verifying the correct header format (
Bearer user-k8s-token).
115-131: Good use oftmp_pathfixture for file-based auth testing.The test correctly creates a temporary secret file and verifies that the resolved header value matches the file contents.
167-212: Comprehensive test coverage for server skipping scenarios.The test covers multiple edge cases: missing kubernetes token, missing client headers, and partial auth resolution. This thoroughly validates the server skipping behavior documented in the README.
298-327: Test properly exercises per-server authorization header resolution.The test correctly configures
ModelContextProtocolServerwithauthorization_headersand verifies that the resolved headers are included in the MCP tool definition.src/app/endpoints/query_v2.py (5)
25-25: Correct import for ModelContextProtocolServer.The import is added to support the new type annotation in
get_mcp_tools.
709-727: Well-implemented helper for token value resolution.The
_get_token_valuehelper uses a clear match statement to handle the three authentication methods. The logic correctly:
- Returns
Nonewhen kubernetes token is unavailable- Looks up client headers by server name (not URL)
- Returns static values as-is
748-758: Correct server skipping logic for incomplete auth resolution.The check
len(headers) != len(mcp_server.authorization_headers)correctly identifies when not all configured headers could be resolved, triggering the skip with an informative warning log.
466-468: Good defensive check for None response.This null check prevents potential
AttributeErrorwhen the agent fails and returns no response.
691-707: Excellent documentation of the header resolution algorithm.The docstring clearly explains the three-step resolution process and the use cases each method supports, making the code self-documenting for future maintainers.
dev-tools/mcp-mock-server/server.py (2)
64-150: Clean MCP protocol implementation.The
do_POSThandler correctly:
- Captures all headers before processing
- Parses JSON-RPC requests with error handling
- Returns appropriate responses for
initializeandtools/listmethods- Derives unique tool names based on auth header content to avoid collisions in tests
271-327: Well-structured server lifecycle management.The
mainfunction correctly:
- Runs HTTP and HTTPS servers on consecutive ports
- Uses daemon threads for clean shutdown
- Provides informative startup messages with endpoints
- Handles
KeyboardInterruptfor graceful shutdown
| from pathlib import Path | ||
|
|
||
|
|
||
| from utils.mcp_auth_headers import resolve_authorization_headers |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use absolute imports for internal modules.
As per coding guidelines, internal module imports should use absolute import paths starting from the project root.
♻️ Proposed fix
-from utils.mcp_auth_headers import resolve_authorization_headers
+from src.utils.mcp_auth_headers import resolve_authorization_headersBased on coding guidelines, internal modules should use absolute imports.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from utils.mcp_auth_headers import resolve_authorization_headers | |
| from src.utils.mcp_auth_headers import resolve_authorization_headers |
🤖 Prompt for AI Agents
In @tests/unit/utils/test_mcp_auth_headers.py at line 6, The test imports the
internal util using a relative/short module path; update the import to the
project's absolute module path so internal modules use absolute imports (replace
the current "from utils.mcp_auth_headers import resolve_authorization_headers"
with the project-root absolute import that exposes
resolve_authorization_headers, e.g. using the package name followed by
.utils.mcp_auth_headers).
bf1a695 to
89f443e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @dev-tools/mcp-mock-server/README.md:
- Around line 39-53: Update the fenced code block in README.md that starts with
the "MCP Mock Server starting with HTTP and HTTPS" banner to include a language
identifier (e.g., add ```text or ```bash) on the opening fence so the block is
rendered correctly; keep the existing block contents and closing ``` intact.
🧹 Nitpick comments (1)
dev-tools/mcp-mock-server/server.py (1)
84-97: Consider refactoring the token matching logic.The current implementation uses match/case with string content checks, which works but is somewhat unconventional. Consider extracting the token value first for clearer logic:
♻️ Optional refactor for clarity
- # Match based on token content - match auth_header: - case _ if "test-secret-token" in auth_header: - tool_name = "mock_tool_file" - tool_desc = "Mock tool with file-based auth" - case _ if "my-k8s-token" in auth_header: - tool_name = "mock_tool_k8s" - tool_desc = "Mock tool with Kubernetes token" - case _ if "my-client-token" in auth_header: - tool_name = "mock_tool_client" - tool_desc = "Mock tool with client-provided token" - case _: - # No auth header or unrecognized token - tool_name = "mock_tool_no_auth" - tool_desc = "Mock tool with no authorization" + # Match based on token content + if "test-secret-token" in auth_header: + tool_name = "mock_tool_file" + tool_desc = "Mock tool with file-based auth" + elif "my-k8s-token" in auth_header: + tool_name = "mock_tool_k8s" + tool_desc = "Mock tool with Kubernetes token" + elif "my-client-token" in auth_header: + tool_name = "mock_tool_client" + tool_desc = "Mock tool with client-provided token" + else: + # No auth header or unrecognized token + tool_name = "mock_tool_no_auth" + tool_desc = "Mock tool with no authorization"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
MakefileREADME.mddev-tools/MANUAL_TESTING.mddev-tools/mcp-mock-server/README.mddev-tools/mcp-mock-server/server.pydev-tools/mcp-mock-server/test_mock_mcp_server.pysrc/app/endpoints/query_v2.pysrc/utils/mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/mcp_auth_headers.py
🧰 Additional context used
📓 Path-based instructions (5)
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all unit and integration tests
Files:
tests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_dump_configuration.pytests/unit/utils/test_mcp_auth_headers.pytests/unit/models/config/test_model_context_protocol_server.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Do not use unittest - pytest is the standard for this project
Usepytest-mockfor AsyncMock objects in tests
Files:
tests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_dump_configuration.pytests/unit/utils/test_mcp_auth_headers.pytests/unit/models/config/test_model_context_protocol_server.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Usefrom llama_stack_client import AsyncLlamaStackClientfor Llama Stack integration
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
Use modern union syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]orType | Nonefor optional types
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification: return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in API calls
Useimport loggingand module logger pattern:logger = logging.getLogger(__name__)
Use standard log levels:logger.debug()for detailed diagnostic info,logger.info()for general execution info,logger.warning()for unexpected events,logger.error()for serious problems
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes:Configurationfor config,Error/Exceptionfor exceptions,Resolverfor strategy pattern,Interfacefor abstract base classes
Use ABC (Abstract Base Class) with@abstractmethoddecorators for interface definitions
Include complete type annotations for all class attributes
Follow Google Python docstring conventions: https://google.github.io/styleguide/pyguide.html with sections for Args, Returns, Raises, and Attributes
Use black for code formatting
Use pylint for static analysis with source-roots = "src"
Use pyright for type checking (excludessrc/authentication/k8s.py)
Use ruff for fast linting
Use pydocstyle for docstring style validation
U...
Files:
src/app/endpoints/query_v2.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI dependencies:
from fastapi import APIRouter, HTTPException, Request, status, Depends
Files:
src/app/endpoints/query_v2.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query_v2.py
🧠 Learnings (16)
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Handle `APIConnectionError` from Llama Stack in API calls
Applied to files:
tests/unit/app/endpoints/test_query_v2.py
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to tests/**/*.py : Use `pytest-mock` for AsyncMock objects in tests
Applied to files:
dev-tools/mcp-mock-server/test_mock_mcp_server.py
📚 Learning: 2026-01-09T15:39:01.298Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.298Z
Learning: Applies to src/**/*.py : Use absolute imports for internal modules: `from authentication import get_auth_dependency`
Applied to files:
tests/unit/utils/test_mcp_auth_headers.py
📚 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:
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:
README.md
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use pyright for type checking (excludes `src/authentication/k8s.py`)
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use bandit for security issue detection
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Always use `uv run` prefix for commands
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Run `uv run make verify` to run all linters before completion
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Run `uv run make format` to auto-format code with black and ruff before completion
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use black for code formatting
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use ruff for fast linting
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use pylint for static analysis with source-roots = "src"
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use pydocstyle for docstring style validation
Applied to files:
Makefile
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use standard log levels: `logger.debug()` for detailed diagnostic info, `logger.info()` for general execution info, `logger.warning()` for unexpected events, `logger.error()` for serious problems
Applied to files:
dev-tools/mcp-mock-server/server.py
📚 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:
dev-tools/MANUAL_TESTING.md
🧬 Code graph analysis (4)
tests/unit/app/endpoints/test_query_v2.py (2)
src/models/config.py (1)
ModelContextProtocolServer(425-511)src/app/endpoints/query_v2.py (1)
get_mcp_tools(675-765)
tests/unit/utils/test_mcp_auth_headers.py (1)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-81)
tests/unit/models/config/test_model_context_protocol_server.py (2)
src/models/config.py (6)
config(325-342)ModelContextProtocolServer(425-511)Configuration(1440-1554)ServiceConfiguration(345-422)LlamaStackConfiguration(514-605)UserDataCollection(608-673)src/configuration.py (2)
mcp_servers(151-162)customization(201-213)
src/app/endpoints/query_v2.py (2)
src/models/config.py (2)
config(325-342)ModelContextProtocolServer(425-511)src/configuration.py (1)
mcp_servers(151-162)
🪛 Gitleaks (8.30.0)
dev-tools/MANUAL_TESTING.md
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 139-141: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
dev-tools/mcp-mock-server/README.md
[high] 96-98: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 208-210: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
README.md
[style] ~297-~297: Consider using “inaccessible” to avoid wordiness.
Context: ...gured in the llama-stack run.yaml are not accessible to lightspeed-core agents. #### Config...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
dev-tools/mcp-mock-server/README.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (39)
Makefile (1)
32-32: LGTM! Quality checks correctly extended to dev-tools directory.The additions of
dev-toolsto all linting, type-checking, security, and documentation style targets are appropriate and consistent. This ensures the new MCP mock server code and development utilities receive the same quality checks as the rest of the codebase.Also applies to: 35-35, 85-85, 88-88, 91-91
dev-tools/MANUAL_TESTING.md (1)
1-184: LGTM! Comprehensive manual testing guide.The documentation is well-structured and provides clear, step-by-step instructions for testing MCP server authorization. The separation of basic testing from optional real-token testing is good practice, and the troubleshooting section adds helpful guidance.
Note: Static analysis flagged the Authorization headers in curl examples as potential security issues, but these are clearly example/mock tokens for documentation purposes, not real credentials.
README.md (2)
297-435: Excellent MCP authentication documentation.The documentation clearly explains all three authentication methods (static files, Kubernetes tokens, client-provided) with practical examples and a comparison table. The warning about automatic server skipping (lines 426-434) is prominently placed and helps prevent confusion.
1100-1129: Good addition of Development Tools section.The MCP Mock Server documentation is well-placed and provides a clear quick start guide. The feature list helps developers quickly understand what the tool offers.
dev-tools/mcp-mock-server/server.py (2)
225-248: Certificate generation implementation is solid.The subprocess call to openssl is properly configured with error handling. Good practice checking for existing certificates before generating new ones.
253-269: Appropriate exception handling for a mock server.The broad exception catching with
Exceptionis acceptable here since this is a development/testing tool where graceful degradation and clear error messages are more important than strict error handling.dev-tools/mcp-mock-server/README.md (1)
1-277: Excellent documentation for the mock server.The README is comprehensive, well-structured, and provides clear guidance for all use cases. The separation of HTTP/HTTPS examples, debug endpoints documentation, and troubleshooting section are particularly helpful.
tests/unit/models/config/test_dump_configuration.py (2)
240-249: Test expectations correctly updated for new MCP server fields.The three new fields (
authorization_headers,resolved_authorization_headers,timeout) are consistently added to the expected output, matching the ModelContextProtocolServer model changes.
300-325: Multi-server test expectations are consistent.All three MCP servers in the test have the same new fields properly configured, ensuring comprehensive test coverage.
tests/unit/utils/test_mcp_auth_headers.py (6)
1-12: LGTM! Well-structured test module setup.The module docstring, imports, and initial test for empty input are correct. The absolute import
from utils.mcp_auth_headersaligns with coding guidelines.
15-37: LGTM! File-based resolution tests are thorough.These tests correctly verify file reading and whitespace stripping behavior, aligning with the
.strip()call in the productionresolve_authorization_headersfunction.
40-46: LGTM! Nonexistent file handling test is correct.The test verifies the expected behavior where missing files result in the header being omitted from the result.
49-64: LGTM! Special keyword tests correctly verify preservation behavior.Both
clientandkuberneteskeywords are properly tested to ensure they pass through unchanged for runtime substitution.
67-103: LGTM! Comprehensive multi-header and mixed-type tests.These tests effectively validate the function's ability to handle multiple headers simultaneously with different resolution strategies.
106-116: LGTM! Directory path error handling test is appropriate.This test verifies that providing a directory path (instead of a file) results in the header being omitted, matching the
is_file()check in production code.tests/unit/models/config/test_model_context_protocol_server.py (6)
3-5: LGTM! Appropriate pragma and import additions.The
pyright: reportCallIssue=falsepragma andPathimport are necessary for the new file-based authorization header tests.
27-27: LGTM! Verifies default empty authorization headers.This assertion confirms the
default_factory=dictbehavior defined in the model.
142-169: LGTM! Comprehensive test for file-based header resolution.This test correctly verifies that:
authorization_headerspreserves the original file pathsresolved_authorization_headerscontains the actual secret values read from filesThis aligns with the
resolve_auth_headersmodel validator behavior.
172-191: LGTM! Special case keyword tests.These tests verify that
kubernetesandclientkeywords are accepted inauthorization_headers. Theresolved_authorization_headerspreservation is covered bytest_model_context_protocol_server_resolved_headers_with_special_values.
194-257: LGTM! Excellent integration test for mixed authorization configurations.This test validates backward compatibility (servers without auth headers) alongside new functionality (servers with various auth configurations), ensuring the Configuration model works correctly with heterogeneous MCP server definitions.
260-303: LGTM! Complete coverage of resolved_authorization_headers behavior.These tests effectively verify:
- Special keywords (
kubernetes,client) are preserved in resolved headers- File contents are correctly read into resolved headers
- Empty input results in empty resolved headers
dev-tools/mcp-mock-server/test_mock_mcp_server.py (5)
22-74: Well-structured fixture with proper startup validation and cleanup.The fixture correctly:
- Uses polling instead of blind sleep for server readiness
- Handles server crash during startup
- Provides proper cleanup with termination timeout
The fixed port allocation (acknowledged in comments) is acceptable for this dev-tools context. Consider using dynamic port allocation in the future if parallel test execution becomes necessary.
76-119: LGTM! Flexible request helper for test scenarios.The helper appropriately handles both HTTP and HTTPS (with self-signed cert bypass), JSON/non-JSON responses, and error conditions. The pylint disables for import-outside-toplevel and protected-access are justified for this test utility.
122-157: LGTM! HTTP and HTTPS endpoint tests verify core functionality.Both tests correctly validate the JSON-RPC
tools/listmethod response structure over HTTP and HTTPS protocols.
159-217: LGTM! Debug endpoint tests verify header and request logging.These tests effectively validate that the mock server captures and exposes authorization headers and request history through debug endpoints.
220-300: LGTM! Comprehensive header capture and request counting tests.These tests verify:
- Multiple authorization headers are captured simultaneously
- Arbitrary headers (not just predefined ones) are captured
- Request count increments correctly for POST requests
The comment at line 278-279 clarifying that only POST requests are logged is helpful for understanding the test logic.
src/app/endpoints/query_v2.py (6)
25-25: LGTM! Appropriate import addition.The
ModelContextProtocolServerimport supports the type annotation change inget_mcp_tools.
80-81: LGTM! Minor docstring improvement.Added comma before dependent clause improves readability.
466-468: LGTM! Defensive null check for response.This guards against
Noneresponse or empty output, preventing potentialAttributeErrorwhen the agent fails.
675-707: LGTM! Well-documented function with clear algorithm description.The updated docstring clearly explains the three-tier header resolution algorithm:
kubernetes→ use k8s token from requestclient→ use client-provided headers frommcp_headers- Otherwise → use literal value from configuration
This provides good clarity for maintainers.
709-727: LGTM! Clean pattern matching for header value resolution.The
_get_token_valuehelper correctly implements the resolution algorithm:
- Returns
Nonewhen required token/headers are unavailable (enabling server skipping)- Prefixes kubernetes token with
Bearerfor proper authorization header format- Uses closure to access
mcp_server.namefor looking up client headers
748-764: LGTM! Robust server skipping logic ensures auth integrity.The condition
mcp_server.authorization_headers and len(headers) != len(mcp_server.authorization_headers)correctly:
- Allows servers without auth requirements to always be included
- Skips servers where any required header couldn't be resolved
- Logs a clear warning explaining why a server was skipped
This fail-safe approach prevents making requests with incomplete authorization.
tests/unit/app/endpoints/test_query_v2.py (7)
1-5: LGTM! Updated pragmas and imports for new test requirements.The
Pathimport is needed for the new file-based header tests.
57-83: LGTM! Comprehensive test for token-based header generation.The test correctly verifies:
- Servers without
authorization_headersproduce tools without headers- Servers with
kubernetesauth produce tools withBearer {token}format
85-113: LGTM! Client header resolution test with skip behavior.The test effectively validates:
- Headers with
clientvalue are resolved frommcp_headers- Servers requiring client auth are skipped when
mcp_headers=None
115-164: LGTM! Excellent coverage of static and mixed header scenarios.These tests validate:
- Static headers from config files are resolved correctly
- Mixed header types (kubernetes + file + client) coexist and resolve independently
167-230: LGTM! Thorough coverage of server skipping and inclusion logic.These tests comprehensively validate the fail-safe behavior:
- Servers missing required kubernetes tokens are skipped
- Servers missing required client headers are skipped
- Servers with partial header resolution are skipped
- Servers without auth requirements are always included
299-304: LGTM! Updated mock to use new authorization_headers parameter.The mock MCP server correctly uses the new
authorization_headers={"Authorization": "kubernetes"}format to test the integrated flow.
671-671: LGTM! Assertion string unchanged.The assertion message remains consistent with the expected error response.
blublinsky
force-pushed
the
mcp-headers
branch
from
89f443e to
56bdd35
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @dev-tools/MANUAL_TESTING.md:
- Around line 55-60: Replace hard-coded token literals in the curl examples with
clear placeholders: change the Authorization header value "Bearer my-k8s-token"
to "Bearer <K8S_TOKEN>" and inside the MCP-HEADERS JSON change "Authorization":
"Bearer my-client-token" to "Authorization": "Bearer <CLIENT_TOKEN>"; apply the
same replacement for the other occurrence mentioned (lines 139-143) so examples
use placeholders instead of real-looking tokens for gitleaks/safety compliance.
In @dev-tools/mcp-mock-server/README.md:
- Around line 96-100: Replace hard-coded token-like examples in the README curl
snippets (the Authorization header value "Bearer user-token") with a placeholder
pattern to avoid gitleaks and accidental copy/paste of real tokens; update the
curl example Authorization header to use a placeholder such as "Bearer
<USER_TOKEN>" or "Bearer ${TOKEN}" in the snippet(s) around the shown block and
the other occurrences mentioned (lines ~208-213).
- Around line 39-54: The fenced code block showing the MCP Mock Server startup
console output is missing a language tag; update the triple-backtick fence to
use "text" (i.e., ```text) for the block that begins with "MCP Mock Server
starting with HTTP and HTTPS" so the markdown linter rule MD040 is satisfied.
In @src/models/config.py:
- Around line 459-513: Make resolved_authorization_headers a PrivateAttr so
users cannot override it from input (replace the Field(...) declaration with a
pydantic.PrivateAttr on the model and remove it from model fields), keep
resolve_auth_headers to populate that PrivateAttr from authorization_headers
using resolve_authorization_headers, and change the timeout annotation from int
| None to pydantic.types.PositiveInt | None (or import PositiveInt) so timeout
is validated as a positive integer.
In @src/utils/mcp_auth_headers.py:
- Around line 39-80: The code currently trims and reads paths directly and logs
the raw incoming header value in the exception/warning paths; update the
resolution logic in the loop over authorization_headers to normalize inputs and
avoid leaking secrets: for each value, call value = value.strip() and expand
user/home (Path(value).expanduser()) before creating secret_path, treat values
that are clearly file paths (exists and is_file) as before, but if a literal
token is detected (no path separators or the path does not exist) do not log the
raw value—instead log a redacted placeholder or masked form (e.g. "<REDACTED>"
or show only last 4 chars) in logger.warning and logger.error; also centralize
file-read into a small helper (e.g., read_secret_from_file) and replace the
inline open logic, and update the except block and warning messages to reference
header_name and the normalized secret_path or redacted value rather than the raw
value variable.
🧹 Nitpick comments (8)
src/utils/mcp_auth_headers.py (1)
12-36: Tighten docstring + fix minor typos (and align with Google style).Docstring has a typo (“leave is unchanged”, double “.”) and uses “Parameters” instead of “Args/Returns” per the repo’s Python guidelines.
src/models/config.py (1)
31-32: Import location is fine; consider centralizing sentinel strings in constants.Based on learnings, check
constants.pybefore adding new shared strings like"kubernetes"/"client".dev-tools/MANUAL_TESTING.md (1)
27-29: Consider moving the longuv pip install ...list into a requirements file.This will be easier to update/reuse (e.g.,
uv pip install -r dev-tools/test-configs/llama-stack-library-requirements.txt).tests/unit/models/config/test_dump_configuration.py (1)
4-4: Consider adding a brief rationale comment for the pyright suppression, or narrow it to specific lines.The blanket
# pyright: reportCallIssue=falsecan hide real type issues. If the suppression is needed due to Pydantic'sSecretStrconstructor (which appears 10 times across multiple test functions), add a comment explaining this. Alternatively, if issues are clustered in specific functions, apply line-level# pyright: ignore[reportCallIssue]comments instead.dev-tools/mcp-mock-server/test_mock_mcp_server.py (1)
117-119: Unreachable return statement afterpytest.fail.The
return 500, ""on line 119 is dead code sincepytest.fail()raises an exception. While it satisfies static analysis, consider removing the comment or usingtyping.NoReturnannotation.♻️ Suggested improvement
except Exception as e: # pylint: disable=broad-except pytest.fail(f"Request failed: {e}") - return 500, "" # Never reached, but makes pylint happy + # pytest.fail() raises - this line is unreachable + raise AssertionError("Unreachable") # For type checkerREADME.md (1)
297-298: Minor wording improvement suggested by static analysis.Consider the style suggestion to use "inaccessible" for conciseness.
📝 Suggested wording
-**Important**: Only MCP servers defined in the `lightspeed-stack.yaml` configuration are available to the agents. Tools configured in the llama-stack `run.yaml` are not accessible to lightspeed-core agents. +**Important**: Only MCP servers defined in the `lightspeed-stack.yaml` configuration are available to the agents. Tools configured in the llama-stack `run.yaml` are inaccessible to lightspeed-core agents.dev-tools/mcp-mock-server/server.py (2)
29-32: Consider adding type annotation for request_log.For consistency with
last_headers, consider adding a type annotation torequest_log.♻️ Suggested improvement
# Global storage for captured headers (last request) last_headers: dict[str, str] = {} -request_log: list = [] +request_log: list[dict[str, Any]] = []
42-62: Thread safety concern with global state mutation.The
_capture_headersmethod modifies globallast_headersandrequest_logwithout synchronization. With concurrent HTTP/HTTPS servers, this could cause race conditions. For a dev/test tool this is likely acceptable, but worth noting.For production-grade tooling, consider using
threading.Lockto protect shared state:import threading _lock = threading.Lock() def _capture_headers(self) -> None: with _lock: last_headers.clear() # ... rest of capture logic
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
MakefileREADME.mddev-tools/MANUAL_TESTING.mddev-tools/mcp-mock-server/README.mddev-tools/mcp-mock-server/server.pydev-tools/mcp-mock-server/test_mock_mcp_server.pysrc/app/endpoints/query_v2.pysrc/models/config.pysrc/utils/mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
📓 Path-based instructions (7)
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all unit and integration tests
Files:
tests/unit/models/config/test_dump_configuration.pytests/unit/utils/test_mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_model_context_protocol_server.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Do not use unittest - pytest is the standard for this project
Usepytest-mockfor AsyncMock objects in tests
Files:
tests/unit/models/config/test_dump_configuration.pytests/unit/utils/test_mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_model_context_protocol_server.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Usefrom llama_stack_client import AsyncLlamaStackClientfor Llama Stack integration
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
Use modern union syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]orType | Nonefor optional types
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification: return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in API calls
Useimport loggingand module logger pattern:logger = logging.getLogger(__name__)
Use standard log levels:logger.debug()for detailed diagnostic info,logger.info()for general execution info,logger.warning()for unexpected events,logger.error()for serious problems
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes:Configurationfor config,Error/Exceptionfor exceptions,Resolverfor strategy pattern,Interfacefor abstract base classes
Use ABC (Abstract Base Class) with@abstractmethoddecorators for interface definitions
Include complete type annotations for all class attributes
Follow Google Python docstring conventions: https://google.github.io/styleguide/pyguide.html with sections for Args, Returns, Raises, and Attributes
Use black for code formatting
Use pylint for static analysis with source-roots = "src"
Use pyright for type checking (excludessrc/authentication/k8s.py)
Use ruff for fast linting
Use pydocstyle for docstring style validation
U...
Files:
src/app/endpoints/query_v2.pysrc/utils/mcp_auth_headers.pysrc/models/config.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI dependencies:
from fastapi import APIRouter, HTTPException, Request, status, Depends
Files:
src/app/endpoints/query_v2.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query_v2.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All config uses Pydantic models extendingConfigurationBase
Base classConfigurationBasesetsextra="forbid"to reject unknown fields
Use type hints:Optional[FilePath],PositiveInt,SecretStrfor configuration fields
ExtendConfigurationBasefor configuration Pydantic models
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Usetyping_extensions.Selffor model validators in type annotations
ExtendBaseModelfor data Pydantic models
Use@model_validatorand@field_validatorfor Pydantic model validation
Files:
src/models/config.py
🧠 Learnings (9)
📚 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:
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:
README.md
📚 Learning: 2026-01-09T15:39:01.298Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.298Z
Learning: Applies to src/**/*.py : Use absolute imports for internal modules: `from authentication import get_auth_dependency`
Applied to files:
tests/unit/utils/test_mcp_auth_headers.py
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Use standard log levels: `logger.debug()` for detailed diagnostic info, `logger.info()` for general execution info, `logger.warning()` for unexpected events, `logger.error()` for serious problems
Applied to files:
dev-tools/mcp-mock-server/server.py
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to tests/**/*.py : Use `pytest-mock` for AsyncMock objects in tests
Applied to files:
dev-tools/mcp-mock-server/test_mock_mcp_server.py
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Applies to src/**/*.py : Handle `APIConnectionError` from Llama Stack in API calls
Applied to files:
tests/unit/app/endpoints/test_query_v2.py
📚 Learning: 2026-01-09T15:39:01.298Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.298Z
Learning: Applies to src/models/config.py : All config uses Pydantic models extending `ConfigurationBase`
Applied to files:
src/models/config.py
📚 Learning: 2026-01-09T15:39:01.299Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.299Z
Learning: Check `constants.py` for shared constants before defining new ones
Applied to files:
src/models/config.py
📚 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:
dev-tools/MANUAL_TESTING.md
🧬 Code graph analysis (5)
src/app/endpoints/query_v2.py (2)
src/models/config.py (2)
config(325-342)ModelContextProtocolServer(425-512)src/configuration.py (1)
mcp_servers(151-162)
tests/unit/utils/test_mcp_auth_headers.py (1)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-81)
tests/unit/app/endpoints/test_query_v2.py (2)
src/models/config.py (1)
ModelContextProtocolServer(425-512)src/app/endpoints/query_v2.py (1)
get_mcp_tools(675-765)
src/models/config.py (1)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-81)
tests/unit/models/config/test_model_context_protocol_server.py (2)
src/models/config.py (3)
config(325-342)ModelContextProtocolServer(425-512)Configuration(1441-1555)src/configuration.py (1)
mcp_servers(151-162)
🪛 Gitleaks (8.30.0)
dev-tools/MANUAL_TESTING.md
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 139-141: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
dev-tools/mcp-mock-server/README.md
[high] 96-98: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 208-210: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
README.md
[style] ~297-~297: Consider using “inaccessible” to avoid wordiness.
Context: ...gured in the llama-stack run.yaml are not accessible to lightspeed-core agents. #### Config...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
dev-tools/mcp-mock-server/README.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (20)
src/models/config.py (1)
1543-1555:dump(self, filename: str | Path)update looks good.tests/unit/models/config/test_dump_configuration.py (1)
240-249: Updated dumped MCP server expectations look consistent.Also applies to: 299-322
tests/unit/utils/test_mcp_auth_headers.py (1)
1-116: LGTM! Comprehensive test coverage for authorization header resolution.The tests thoroughly cover all resolution scenarios: empty input, file-backed secrets, whitespace stripping, nonexistent files, special keywords (
kubernetes,client), multiple headers, and mixed types. Good use of pytest'stmp_pathfixture for file-based tests.dev-tools/mcp-mock-server/test_mock_mcp_server.py (2)
22-74: Well-structured server fixture with proper lifecycle management.Good practices: health-check polling instead of blind sleep, proper process cleanup on failure, graceful shutdown with timeout fallback to kill. The comment about fixed ports for parallel test execution is helpful.
76-119: LGTM! Helper function covers HTTP/HTTPS request handling well.The
make_requesthelper handles JSON encoding, SSL context for self-signed certs, and proper error response parsing. Good reuse across tests.tests/unit/models/config/test_model_context_protocol_server.py (3)
142-169: Thorough test for file-based header resolution.This test verifies that
authorization_headerswith file paths correctly populatesresolved_authorization_headerswith the file contents. Good coverage of the model validator behavior.
172-191: Good coverage of special keyword preservation.Tests verify that
kubernetesandclientvalues are preserved as-is for later runtime substitution, which aligns with the resolution algorithm documented in the production code.
194-258: Excellent mixed configuration and backward compatibility testing.This test validates that MCP servers with different header configurations (no auth, file-backed, kubernetes, client) work correctly together, ensuring backward compatibility for existing configurations without authorization headers.
README.md (2)
324-434: Comprehensive authentication documentation with clear examples.The three authentication methods are well-documented with practical YAML examples, the comparison table is helpful for choosing the right approach, and the automatic server skipping behavior is clearly explained. This documentation will significantly help users configure MCP server authentication correctly.
1100-1128: Good addition of Development Tools section.The MCP Mock Server quick start and feature list provide clear guidance for local testing. Cross-reference to the detailed README is appropriate.
src/app/endpoints/query_v2.py (4)
675-707: Clear and well-documented header resolution algorithm.The docstring comprehensively explains the three-tier header resolution: kubernetes token, client-provided headers via
MCP-HEADERS, and static values from configuration. This documentation will help maintainers understand the logic.
709-727: Nested function correctly captures outer scope for server context.The
_get_token_valueclosure appropriately accessesmcp_server.namefrom the enclosing loop scope for client header lookup. The match-case pattern is clean and readable.
749-758: Robust server skipping logic with informative warning.Servers with incomplete header resolution are correctly skipped with a descriptive warning log. This prevents authentication failures at runtime while providing visibility into why servers were excluded.
466-468: Good defensive handling for None responses.This robustness improvement prevents errors when an agent fails and returns
None, aligning with the AI summary's note about handling agent failures gracefully.dev-tools/mcp-mock-server/server.py (2)
64-150: Well-structured MCP protocol handling with token-based tool identification.The match-case pattern for determining tool identity based on authorization tokens is a clean approach for testing different auth scenarios. The JSON-RPC protocol handling is correct.
271-327: Clean server lifecycle management.Good use of daemon threads for server processes, graceful shutdown handling on KeyboardInterrupt, and informative startup banner. The self-signed certificate generation for HTTPS testing is practical for local development.
tests/unit/app/endpoints/test_query_v2.py (4)
57-83: Good coverage of basic MCP tools scenarios.Tests correctly verify that servers without authorization headers work without token, and servers with
kubernetesauth correctly receive the Bearer token. The assertions validate both inclusion and header content.
85-112: Excellent test for client-provided headers and skipping behavior.This test verifies the critical behavior that servers requiring client auth are skipped when
mcp_headers=None, which prevents authentication failures at runtime. The assertionlen(tools_no_headers) == 0correctly validates the skipping logic.
167-212: Comprehensive server skipping edge case coverage.This test thoroughly validates the skipping logic across multiple scenarios:
- All servers skipped when no token/headers provided
- Partial resolution (kubernetes works, client doesn't)
- Incomplete header sets for multi-header configurations
This is excellent coverage for the security-critical behavior of not calling servers with incomplete authentication.
298-327: Test correctly validates MCP tool construction with kubernetes auth.The mock configuration now properly uses
ModelContextProtocolServerwithauthorization_headers, and the assertion verifies the Bearer token format is correctly applied.
| curl -X POST http://localhost:8080/v1/streaming_query \ | ||
| -H "Content-Type: application/json" \ | ||
| -H "Authorization: Bearer my-k8s-token" \ | ||
| -H 'MCP-HEADERS: {"mock-client-auth": {"Authorization": "Bearer my-client-token"}}' \ | ||
| -d '{"query": "Test all MCP auth types"}' | ||
| ``` |
There was a problem hiding this comment.
Use token placeholders in curl examples (gitleaks + safety).
E.g. Authorization: Bearer <K8S_TOKEN> and Bearer <CLIENT_TOKEN> instead of token-like literals.
Also applies to: 139-143
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🤖 Prompt for AI Agents
In @dev-tools/MANUAL_TESTING.md around lines 55 - 60, Replace hard-coded token
literals in the curl examples with clear placeholders: change the Authorization
header value "Bearer my-k8s-token" to "Bearer <K8S_TOKEN>" and inside the
MCP-HEADERS JSON change "Authorization": "Bearer my-client-token" to
"Authorization": "Bearer <CLIENT_TOKEN>"; apply the same replacement for the
other occurrence mentioned (lines 139-143) so examples use placeholders instead
of real-looking tokens for gitleaks/safety compliance.
| curl -X POST http://localhost:8080/v1/query \ | ||
| -H "Content-Type: application/json" \ | ||
| -H "Authorization: Bearer user-token" \ | ||
| -d '{"query": "Test MCP tools"}' | ||
| ``` |
There was a problem hiding this comment.
Replace token-like curl examples with placeholders to avoid gitleaks + copy/paste accidents.
Even though these are “fake”, gitleaks flags them and users may paste real tokens into docs. Prefer Bearer <USER_TOKEN> / Bearer ${TOKEN} patterns.
Also applies to: 208-213
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 96-98: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🤖 Prompt for AI Agents
In @dev-tools/mcp-mock-server/README.md around lines 96 - 100, Replace
hard-coded token-like examples in the README curl snippets (the Authorization
header value "Bearer user-token") with a placeholder pattern to avoid gitleaks
and accidental copy/paste of real tokens; update the curl example Authorization
header to use a placeholder such as "Bearer <USER_TOKEN>" or "Bearer ${TOKEN}"
in the snippet(s) around the shown block and the other occurrences mentioned
(lines ~208-213).
src/models/config.py
Outdated
| ), | ||
| ) | ||
|
|
||
| timeout: int | None = Field( |
There was a problem hiding this comment.
CodeRabbit is right, should be Optional[PositiveInt]
|
@tisnik @gallettilance This PR sets a dependency between the |
@onmete your original proposal was a bit different, right? From my perspective, the ability to enable headers (and not vice versa) is ok from sec.perspective etc. Also @jrobertboos WDYT? |
I think it is the right thing to do as it makes execution more predictable. Allowing user to overwrite anything in MCP configuration can lead to unpredictable results |
|
I don't have an opinion here, I'm just raising it for awareness, because it changes how it works now - the client can set and send whatever it needs without service configuration changes. |
blublinsky
force-pushed
the
mcp-headers
branch
from
56bdd35 to
2a93288
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Makefile (1)
93-94: Adddev-toolsto ruff per-file-ignores for consistency.The
dev-tools/mcp-mock-server/test_mock_mcp_server.pycontains multipleassertstatements that will be flagged by S101. Adding--per-file-ignores=dev-tools/*:S101will treat test utilities consistently withtests/*andscripts/*.src/utils/mcp_auth_headers.py (1)
12-36: Fix typos and align docstring format with Google style conventions.The docstring has a few issues:
- Line 17: "leave is unchanged" → "leave it unchanged"
- Line 18: Extra period before "We substitute"
- Per coding guidelines, use "Args:" instead of "Parameters:" for Google Python docstring conventions
📝 Suggested fix
""" Resolve authorization headers by reading secret files or preserving special values. - Parameters: + Args: authorization_headers: Map of header names to secret locations or special keywords. - - If value is "kubernetes": leave is unchanged. We substitute it during request. - - If value is "client": leave it unchanged. . We substitute it during request. + - If value is "kubernetes": leave it unchanged. We substitute it during request. + - If value is "client": leave it unchanged. We substitute it during request. - Otherwise: Treat as file path and read the secret from that file Returns:src/app/endpoints/query_v2.py (1)
709-727: Consider passingmcp_serveras parameter instead of relying on closure.The
_get_token_valuehelper referencesmcp_serverfrom the enclosing loop scope (line 721). While this works correctly, explicitly passing it as a parameter would make the data flow clearer:♻️ Optional refactor for clarity
- def _get_token_value(original: str, header: str) -> str | None: + def _get_token_value(original: str, header: str, server: ModelContextProtocolServer) -> str | None: """Convert to header value.""" match original: case "kubernetes": # use k8s token if token is None or token == "": return None return f"Bearer {token}" case "client": # use client provided token if mcp_headers is None: return None - c_headers = mcp_headers.get(mcp_server.name, None) + c_headers = mcp_headers.get(server.name, None) if c_headers is None: return None return c_headers.get(header, None) case _: # use provided return originalThen update the call site:
- h_value = _get_token_value(value, name) + h_value = _get_token_value(value, name, mcp_server)dev-tools/MANUAL_TESTING.md (1)
22-30: Consider moving library-mode dependencies to a requirements file.The inline
uv pip installcommand with 20+ packages is hard to maintain. Consider creating adev-tools/test-configs/requirements-library-mode.txtfile for easier updates:♻️ Suggested improvement
Create
dev-tools/test-configs/requirements-library-mode.txt:emoji langdetect aiosqlite pythainlp asyncpg nltk mcp>=1.23.0 matplotlib sqlalchemy[asyncio] chardet scikit-learn faiss-cpu pillow datasets>=4.0.0 psycopg2-binary pandas pypdf pymongo redis tree_sitter requestsThen update the documentation:
-uv pip install emoji langdetect aiosqlite pythainlp asyncpg nltk 'mcp>=1.23.0' matplotlib 'sqlalchemy[asyncio]' chardet scikit-learn faiss-cpu pillow 'datasets>=4.0.0' psycopg2-binary pandas pypdf pymongo redis tree_sitter requests +uv pip install -r dev-tools/test-configs/requirements-library-mode.txtdev-tools/mcp-mock-server/server.py (1)
205-250: Consider documenting theopenssldependency.The certificate generation relies on the
opensslCLI being available. While this is common on developer machines, consider adding a note about this requirement in the README or a helpful error message.💡 Optional: Add helpful error message for missing openssl
except subprocess.CalledProcessError as e: - print(f"Failed to generate certificate: {e}") + print(f"Failed to generate certificate: {e}") + print("Hint: Make sure 'openssl' is installed and available in PATH") raise
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
MakefileREADME.mddev-tools/MANUAL_TESTING.mddev-tools/mcp-mock-server/README.mddev-tools/mcp-mock-server/server.pydev-tools/mcp-mock-server/test_mock_mcp_server.pysrc/app/endpoints/query_v2.pysrc/models/config.pysrc/utils/mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/models/config/test_dump_configuration.py
- dev-tools/mcp-mock-server/test_mock_mcp_server.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:
tests/unit/utils/test_mcp_auth_headers.pysrc/utils/mcp_auth_headers.pytests/unit/models/config/test_model_context_protocol_server.pydev-tools/mcp-mock-server/server.pysrc/app/endpoints/query_v2.pytests/unit/app/endpoints/test_query_v2.pysrc/models/config.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/utils/test_mcp_auth_headers.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/app/endpoints/test_query_v2.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/utils/test_mcp_auth_headers.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/app/endpoints/test_query_v2.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query_v2.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All config uses Pydantic models extendingConfigurationBase
Configuration base class setsextra="forbid"to reject unknown fields
Use type hints:Optional[FilePath],PositiveInt,SecretStrfor configuration fields
ExtendConfigurationBasefor configuration Pydantic models
Files:
src/models/config.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/config.py
🧠 Learnings (20)
📚 Learning: 2026-01-11T16:30:41.767Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.767Z
Learning: Applies to **/*.py : Use absolute imports for internal modules: `from authentication import get_auth_dependency`
Applied to files:
tests/unit/utils/test_mcp_auth_headers.py
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
Learning: Applies to **/*.py : Use `logger.debug()` for detailed diagnostic information
Applied to files:
dev-tools/mcp-mock-server/server.py
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
Learning: Applies to **/*.py : Use `logger.info()` for general information about program execution
Applied to files:
dev-tools/mcp-mock-server/server.py
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
Learning: Applies to **/*.py : Use `logger.warning()` for unexpected events or potential problems
Applied to files:
dev-tools/mcp-mock-server/server.py
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
Learning: Applies to **/*.py : Use `logger.error()` for serious problems that prevented function execution
Applied to files:
dev-tools/mcp-mock-server/server.py
📚 Learning: 2026-01-11T16:30:41.767Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.767Z
Learning: Applies to **/*.py : Use `logger = logging.getLogger(__name__)` pattern for module logging
Applied to files:
dev-tools/mcp-mock-server/server.py
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
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_query_v2.py
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
Learning: Applies to **/*.py : Handle `APIConnectionError` from Llama Stack in error handling
Applied to files:
tests/unit/app/endpoints/test_query_v2.py
📚 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:
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:
README.md
📚 Learning: 2026-01-11T16:30:41.767Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.767Z
Learning: Applies to src/models/config.py : Configuration base class sets `extra="forbid"` to reject unknown fields
Applied to files:
src/models/config.py
📚 Learning: 2026-01-11T16:30:41.767Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.767Z
Learning: Applies to src/models/config.py : All config uses Pydantic models extending `ConfigurationBase`
Applied to files:
src/models/config.py
📚 Learning: 2026-01-11T16:30:41.767Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.767Z
Learning: Applies to src/models/config.py : Use type hints: `Optional[FilePath]`, `PositiveInt`, `SecretStr` for configuration fields
Applied to files:
src/models/config.py
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
Learning: Applies to src/models/config.py : Extend `ConfigurationBase` for configuration Pydantic models
Applied to files:
src/models/config.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/config.py
📚 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:
dev-tools/MANUAL_TESTING.md
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
Learning: Applies to **/*.py : Run `uv run make verify` to run all linters (black, pylint, pyright, ruff, docstyle, check-types) before completion
Applied to files:
Makefile
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
Learning: Applies to **/*.py : Run `uv run make format` to auto-format code with black and ruff before completion
Applied to files:
Makefile
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
Learning: Always use `uv run` prefix for all commands
Applied to files:
Makefile
📚 Learning: 2026-01-11T16:30:41.768Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.768Z
Learning: Use `uv sync --group dev --group llslibdev` to install development dependencies
Applied to files:
Makefile
🧬 Code graph analysis (5)
tests/unit/utils/test_mcp_auth_headers.py (1)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-87)
tests/unit/models/config/test_model_context_protocol_server.py (2)
src/models/config.py (4)
config(326-343)ModelContextProtocolServer(426-508)resolved_authorization_headers(478-480)Configuration(1437-1551)src/configuration.py (1)
mcp_servers(151-162)
src/app/endpoints/query_v2.py (1)
src/configuration.py (1)
mcp_servers(151-162)
tests/unit/app/endpoints/test_query_v2.py (2)
src/models/config.py (1)
ModelContextProtocolServer(426-508)src/app/endpoints/query_v2.py (1)
get_mcp_tools(675-765)
src/models/config.py (1)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-87)
🪛 Gitleaks (8.30.0)
dev-tools/MANUAL_TESTING.md
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 139-141: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
dev-tools/mcp-mock-server/README.md
[high] 96-98: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 208-210: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
README.md
[style] ~297-~297: Consider using “inaccessible” to avoid wordiness.
Context: ...gured in the llama-stack run.yaml are not accessible to lightspeed-core agents. #### Config...
(NOT_ABLE_PREMIUM)
⏰ 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). (2)
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (34)
Makefile (5)
32-32: LGTM!The
dev-tools/addition to mypy is consistent with the existing directory patterns.
35-35: LGTM!Security scanning coverage appropriately extended to include the new dev-tools directory.
85-85: LGTM!
88-88: LGTM!
91-91: LGTM!src/utils/mcp_auth_headers.py (1)
39-87: LGTM!The implementation correctly handles the three authorization header types (kubernetes, client, file-based) with appropriate logging for warnings and debug messages. The broad exception handling with
logger.exceptionensures individual header failures don't prevent other headers from being resolved, which is a good resilience pattern.src/models/config.py (2)
460-508: Well-structured addition of per-server authorization headers.The implementation correctly:
- Uses
PrivateAttrfor computed internal state- Provides read-only property access to resolved headers
- Uses
model_validator(mode="after")to resolve headers post-construction- Properly documents the
timeoutfield as reserved for future useOne minor observation: the
resolved_authorization_headersproperty docstring could include a "Returns:" section for consistency with Google docstring conventions.
1539-1550: Good improvement to accept Path-like arguments.The signature update to
str | Pathfollows the coding guidelines for modern union syntax and improves API flexibility.src/app/endpoints/query_v2.py (2)
466-468: Good defensive handling for None responses.Adding the None check prevents errors when an agent fails and returns no response. This improves robustness.
748-764: Important security behavior: skip servers with unresolved auth headers.This logic correctly implements the behavioral change discussed in the PR - servers with configured authorization requirements that cannot be fully resolved are skipped rather than called with partial/missing authentication. The warning log provides good visibility for debugging.
dev-tools/mcp-mock-server/README.md (2)
1-56: Comprehensive documentation for the mock MCP server.Well-structured documentation covering purpose, features, and quick start. The warning about single-threaded nature (line 14) appropriately sets expectations for testing-only use.
Note: The static analysis warnings about authorization tokens in curl commands (lines 96-98, 208-210) are false positives - these are example/placeholder tokens for documentation purposes, not actual secrets.
174-214: Good coverage of all authentication methods.The examples clearly demonstrate the three authorization header types (file-based, kubernetes, client-provided) that align with the implementation in
src/utils/mcp_auth_headers.py.dev-tools/MANUAL_TESTING.md (2)
47-129: Well-designed test scenario covering all authorization types.The unified test scenario effectively exercises all three authorization mechanisms (file-based, kubernetes, client-provided) in a single request, making it easy to verify the complete feature. The expected results and verification steps are clearly documented.
Note: Static analysis warnings about curl auth headers (lines 55-57, 139-141) are false positives - these are example tokens for testing purposes.
132-148: Good documentation of the server-skip behavior.This test scenario clearly demonstrates the behavioral change where servers with unresolved authorization headers (like
mock-client-authwithoutMCP-HEADERS) are skipped rather than called with missing authentication. This aligns with the security-conscious approach discussed in the PR.tests/unit/utils/test_mcp_auth_headers.py (3)
1-12: LGTM! Good test setup and empty input handling.The module docstring, imports (using absolute import as per guidelines), and the empty input test are correctly implemented.
15-65: LGTM! Comprehensive coverage of file-based and keyword scenarios.Tests correctly validate:
- File content reading with
tmp_pathfixture- Whitespace stripping behavior
- Graceful handling of nonexistent files
- Preservation of special keywords ("client", "kubernetes")
67-116: LGTM! Good coverage of complex and error scenarios.The tests for multiple headers, mixed types, and file read errors are well-structured. The directory-as-path test is a clever way to trigger the "not a file" branch without needing permission manipulation.
tests/unit/models/config/test_model_context_protocol_server.py (5)
3-17: LGTM! Test configuration and imports are appropriate.The pyright directive and type ignore comment are reasonable for test files where import paths may differ from production. The
Pathimport supports the new tests usingtmp_pathfixtures.
142-169: LGTM! Good integration test for file-based header resolution.This test validates the end-to-end flow: file paths in
authorization_headers→ resolved secrets inresolved_authorization_headers. The model validator behavior is correctly exercised.
172-191: Consider adding assertions forresolved_authorization_headersin special case tests.These tests verify
authorization_headerscontains the keyword, but don't assert thatresolved_authorization_headersalso preserves the keyword. The test at lines 260-275 does cover this, so this is fine as-is.
194-257: LGTM! Excellent comprehensive test for mixed authorization scenarios.This test effectively validates:
- Backward compatibility (servers without auth headers)
- File-based secret resolution
- Special keyword preservation
- All scenarios working together in a single Configuration
260-303: LGTM! Complete coverage ofresolved_authorization_headersproperty.Tests correctly verify:
- Special values preserved in resolved headers
- File-based secrets properly resolved
- Empty headers when no authorization configured
README.md (3)
297-323: LGTM! Clear introduction to MCP server configuration.The documentation provides a good overview of the basic configuration structure with clear field descriptions and a minimal example.
324-435: LGTM! Comprehensive authentication documentation.The documentation thoroughly covers:
- All three authentication methods with clear examples
- MCP-HEADERS JSON structure and per-server keying
- Combined authentication scenarios
- Important: The automatic server skipping behavior (lines 425-434) is well-documented, addressing the behavioral change discussed in the PR comments
This documentation aligns well with the implementation and provides users with clear guidance.
1100-1128: LGTM! Helpful developer tools documentation.Good addition providing quick start instructions for the MCP Mock Server. The features list clearly explains the testing capabilities, and the link to detailed documentation is appropriate.
dev-tools/mcp-mock-server/server.py (3)
1-31: LGTM! Good module structure for development tool.The docstring clearly explains the purpose and usage. Using stdlib-only dependencies (
http.server,ssl,subprocess) keeps this lightweight and self-contained.
34-98: LGTM! Well-structured request handler with useful debug features.The handler effectively:
- Captures and logs all headers for debugging
- Implements MCP protocol basics (initialize, tools/list)
- Uses match/case for clean authorization detection
- Maintains a rolling log limited to 10 entries
308-327: Thread joining after daemon threads will block forever.Since both threads are started as
daemon=True, callingjoin()without a timeout will block indefinitely because daemon threads only terminate when the main thread exits. TheKeyboardInterruptis never caught unless the user presses Ctrl+C, which is fine for this use case.However, this works correctly for the intended purpose (waiting for Ctrl+C). No change needed.
tests/unit/app/endpoints/test_query_v2.py (6)
1-29: LGTM! Test setup follows coding guidelines.The imports, mock authentication pattern (
MOCK_AUTH), and fixture setup follow the project's testing conventions.
57-83: LGTM! Good coverage of basic MCP tools scenarios.The test correctly validates:
- Servers without auth headers have no
headerskey- Kubernetes auth produces
Bearer {token}format when token is provided
85-165: LGTM! Comprehensive header resolution testing.Tests thoroughly cover:
- Client-provided headers via
mcp_headers- Server skipping when required auth is unavailable (line 112)
- Static file-based headers
- Mixed header types (kubernetes + file + client) in single server
167-231: LGTM! Excellent coverage of server skipping behavior.These tests are critical for validating the behavioral change discussed in the PR:
- Servers requiring unavailable auth are skipped
- Servers with partial auth (some headers resolvable, some not) are skipped
- Servers without auth requirements are always included
This aligns with the documentation and addresses the behavioral change concern raised in PR comments.
299-327: LGTM! Integration test validates end-to-end header flow.The test correctly verifies that:
- MCP server with kubernetes auth is configured
- Token is passed through
retrieve_response- Resulting MCP tool has
Bearer {token}header
668-672: LGTM! No issues with error handling test.The assertion and type comment are appropriate for validating the error response structure.
jrobertboos
left a comment
jrobertboos
left a comment
There was a problem hiding this comment.
I am a little concerned that this solution, while working well with OLS's needs, does not support the standard MCP auth spec. We are not only not allowing for the oAuth 2.0 flow to be implemented but we are also passing through tokens to services behind the MCP server which is expressly forbidden according to the MCP security best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/models/config/test_model_context_protocol_server.py (1)
173-192: Consider addingresolved_authorization_headersassertions.These tests verify the raw
authorization_headersvalue but don't assert onresolved_authorization_headers. Whiletest_model_context_protocol_server_resolved_headers_with_special_values(Line 262) covers this behavior, adding assertions here would make these tests more self-contained.♻️ Suggested enhancement
def test_model_context_protocol_server_kubernetes_special_case() -> None: """Test ModelContextProtocolServer with kubernetes special case.""" mcp = ModelContextProtocolServer( name="k8s-server", url="http://localhost:8080", authorization_headers={"Authorization": "kubernetes"}, ) assert mcp is not None assert mcp.authorization_headers == {"Authorization": "kubernetes"} + # Special value should be preserved in resolved headers + assert mcp.resolved_authorization_headers == {"Authorization": "kubernetes"} def test_model_context_protocol_server_client_special_case() -> None: """Test ModelContextProtocolServer with client special case.""" mcp = ModelContextProtocolServer( name="client-server", url="http://localhost:8080", authorization_headers={"Authorization": "client"}, ) assert mcp is not None assert mcp.authorization_headers == {"Authorization": "client"} + # Special value should be preserved in resolved headers + assert mcp.resolved_authorization_headers == {"Authorization": "client"}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/models/config.pytests/unit/app/endpoints/test_query_v2.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/models/config/test_model_context_protocol_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/app/endpoints/test_streaming_query_v2.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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_query_v2.pytests/unit/models/config/test_model_context_protocol_server.pysrc/models/config.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_query_v2.pytests/unit/models/config/test_model_context_protocol_server.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_query_v2.pytests/unit/models/config/test_model_context_protocol_server.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All config uses Pydantic models extendingConfigurationBase
Configuration base class setsextra="forbid"to reject unknown fields
Use type hints:Optional[FilePath],PositiveInt,SecretStrfor configuration fields
ExtendConfigurationBasefor configuration Pydantic models
Files:
src/models/config.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/config.py
🧠 Learnings (6)
📚 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_query_v2.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 : Handle `APIConnectionError` from Llama Stack in error handling
Applied to files:
tests/unit/app/endpoints/test_query_v2.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 src/models/config.py : All config uses Pydantic models extending `ConfigurationBase`
Applied to files:
tests/unit/models/config/test_model_context_protocol_server.pysrc/models/config.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 src/models/config.py : Configuration base class sets `extra="forbid"` to reject unknown fields
Applied to files:
src/models/config.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 src/models/config.py : Extend `ConfigurationBase` for configuration Pydantic models
Applied to files:
src/models/config.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/config.py
🧬 Code graph analysis (3)
tests/unit/app/endpoints/test_query_v2.py (2)
src/models/config.py (1)
ModelContextProtocolServer(429-511)src/app/endpoints/query_v2.py (1)
get_mcp_tools(675-765)
tests/unit/models/config/test_model_context_protocol_server.py (2)
src/models/config.py (8)
config(329-346)AuthenticationConfiguration(982-1091)Configuration(1440-1593)LlamaStackConfiguration(514-605)ModelContextProtocolServer(429-511)ServiceConfiguration(349-426)UserDataCollection(608-673)resolved_authorization_headers(481-483)src/configuration.py (2)
mcp_servers(151-162)customization(201-213)
src/models/config.py (2)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-87)src/configuration.py (1)
mcp_servers(151-162)
⏰ 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). (2)
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (16)
src/models/config.py (4)
5-5: LGTM!The imports and logger initialization follow the coding guidelines:
loggingimport,PrivateAttrfrom Pydantic, absolute import forresolve_authorization_headers, and the standardlogger = logging.getLogger(__name__)pattern.Also applies to: 24-24, 33-35
463-511: LGTM!The new
authorization_headersfield, private_resolved_authorization_headersattribute withPrivateAttr, and theresolved_authorization_headersproperty follow Pydantic best practices. Theresolve_auth_headersmodel validator correctly delegates to theresolve_authorization_headersutility for consistent header resolution logic. Thetimeoutfield is appropriately documented as reserved for future use.
1542-1579: LGTM!The validator correctly prevents misconfigured MCP servers from being used at runtime by filtering those with
"kubernetes"auth headers when the authentication module is not"k8s". The warning log provides clear, actionable guidance for operators. This aligns with the PR discussion about making execution more predictable.One observation: this is a behavioral change that silently removes servers rather than failing fast. The warning log mitigates this, but operators should monitor logs during deployment.
1581-1591: LGTM!Good improvement to accept both
strandPathfor the filename parameter, using the modern union syntaxstr | Pathper coding guidelines.tests/unit/app/endpoints/test_query_v2.py (7)
1-5: LGTM!Appropriate pragma additions for pylint and pyright, and the
Pathimport is needed for the new file-based header tests.
57-83: LGTM!Good test coverage for basic MCP tools scenarios: servers without authorization headers (verifying no
"headers"key) and servers with kubernetes auth (verifying proper"Bearer"token formatting).
85-113: LGTM!Important test verifying client-provided header resolution and the skip behavior when required auth is unavailable (Lines 110-112). This correctly validates that servers requiring
"client"auth are omitted whenmcp_headers=None.
115-165: LGTM!Excellent test coverage for file-based static headers and mixed header scenarios. The
test_get_mcp_tools_with_mixed_headerstest comprehensively validates that kubernetes, file-based, and client headers can coexist and resolve correctly on a single server.
167-214: LGTM!Comprehensive edge case coverage: servers with missing kubernetes auth, missing client auth, partial auth (where only some headers can be resolved), and the important backward compatibility test for public servers without any auth requirements.
283-311: LGTM!The mock MCP server is correctly updated to include
authorization_headerswith the kubernetes keyword, and the assertion at Line 311 properly verifies that the token is formatted as"Bearer mytoken"in the final headers.
655-656: LGTM!The assertion correctly validates the expected error response for API connection failures.
tests/unit/models/config/test_model_context_protocol_server.py (5)
3-18: LGTM!Appropriate additions: pyright directive to suppress call issue warnings in tests,
Pathimport for tmp_path fixture usage, and additional model imports needed for Configuration test setup.
21-28: LGTM!Good addition of the assertion to verify
authorization_headersdefaults to an empty dict, documenting the expected default behavior.
143-170: LGTM!Comprehensive test for file-based authorization headers: verifies both the raw
authorization_headers(file paths) and theresolved_authorization_headers(file contents). Good use of thetmp_pathfixture.
195-259: LGTM!Excellent integration test covering all authorization header scenarios within a
Configuration. The use ofauthentication=AuthenticationConfiguration(module="k8s")correctly ensures kubernetes servers aren't filtered out by thevalidate_mcp_auth_headersvalidator. Good coverage of backward compatibility (server without auth headers).
262-305: LGTM!Good test coverage for
resolved_authorization_headersbehavior: preservation of special values (kubernetes,client), file content reading, and the empty case. The tests properly validate the resolution logic.
…ual testing support
blublinsky
force-pushed
the
mcp-headers
branch
from
29be23a to
b379d1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @dev-tools/mcp-mock-server/server.py:
- Around line 205-250: Update generate_self_signed_cert to use the logging
module instead of print: add "import logging" and "logger =
logging.getLogger(__name__)" near the top-level imports, replace the print on
success inside generate_self_signed_cert with logger.info including the
cert_file, and replace the print in the except block with logger.error including
exception details (use the caught exception variable e).
🧹 Nitpick comments (8)
Makefile (1)
34-34: LGTM! Dev-tools appropriately added to static analysis targets.The addition of
dev-toolsto mypy, bandit, pylint, pyright, and pydocstyle ensures the new MCP testing utilities receive the same code quality checks as the main codebase. This aligns with the learnings indicatinguv run make verifyshould run all linters before completion.Minor nit: Line 34 uses
dev-tools/(trailing slash) while lines 37, 87, 90, 93 usedev-toolswithout the slash. Both work, but consistency would be slightly cleaner.Optional: Remove trailing slash for consistency
- uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/ tests/unit tests/integration tests/e2e/ dev-tools/ + uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/ tests/unit tests/integration tests/e2e/ dev-toolsAlso applies to: 37-37, 87-87, 90-90, 93-93
dev-tools/mcp-mock-server/server.py (2)
29-62: Thread-safety concern with global mutable state.The
last_headersandrequest_logglobals are accessed by multiple threads (HTTP and HTTPS servers) without synchronization. While unlikely to cause issues in typical test scenarios with sequential requests, concurrent requests could lead to race conditions.Consider using
threading.Lockfor thread-safe access, or document that this is single-request testing only.🔧 Optional: Thread-safe implementation using Lock
+import threading + # Global storage for captured headers (last request) last_headers: dict[str, str] = {} request_log: list = [] +_state_lock = threading.Lock()Then wrap mutations in
_capture_headers:def _capture_headers(self) -> None: """Capture all headers from the request.""" - last_headers.clear() + with _state_lock: + last_headers.clear() - # Capture all headers for debugging - for header_name, value in self.headers.items(): - last_headers[header_name] = value + # Capture all headers for debugging + for header_name, value in self.headers.items(): + last_headers[header_name] = value - # Log the request - request_log.append(...) + # Log the request + request_log.append(...) - # Keep only last 10 requests - if len(request_log) > 10: - request_log.pop(0) + # Keep only last 10 requests + if len(request_log) > 10: + request_log.pop(0)
60-62: Consider usingcollections.dequefor O(1) bounded log.
request_log.pop(0)is O(n). For a test server with only 10 entries this is negligible, butdeque(maxlen=10)provides O(1) operations and automatic size management.♻️ Optional: Use deque for bounded log
+from collections import deque + # Global storage for captured headers (last request) last_headers: dict[str, str] = {} -request_log: list = [] +request_log: deque = deque(maxlen=10)Then simplify
_capture_headers:# Log the request request_log.append({...}) - - # Keep only last 10 requests - if len(request_log) > 10: - request_log.pop(0)dev-tools/mcp-mock-server/README.md (1)
49-52: Minor documentation inconsistency.Line 50 shows
POST /mcp/v1/list_toolsas the MCP endpoint, but line 194 correctly states "POST requests to any path with JSON-RPC format." The mock server accepts POST at any path. Consider updating line 50 toPOST to any path (e.g., / or /mcp/v1/list_tools)for consistency with the actual behavior.dev-tools/MANUAL_TESTING.md (1)
22-30: Consider moving dependencies to a requirements file.The inline
uv pip installcommand on line 27 is quite long and may become difficult to maintain. Consider creating adev-tools/test-configs/requirements-mcp-test.txtfile for these dependencies.The static analysis warnings about authorization tokens in curl commands are false positives—these are placeholder test tokens (
my-k8s-token,my-client-token) in documentation examples.📄 Optional: Create dedicated requirements file
Create
dev-tools/test-configs/requirements-mcp-test.txt:emoji langdetect aiosqlite pythainlp asyncpg nltk mcp>=1.23.0 matplotlib sqlalchemy[asyncio] chardet scikit-learn faiss-cpu pillow datasets>=4.0.0 psycopg2-binary pandas pypdf pymongo redis tree_sitter requestsThen update the docs:
uv pip install -r dev-tools/test-configs/requirements-mcp-test.txttests/unit/utils/test_mcp_auth_headers.py (1)
106-116: Consider adding test for empty file content.The directory-as-path test is good, but consider adding a test for when a file exists but is empty. Per the
resolve_authorization_headersimplementation, empty files should log a warning and not add the header to the result.🧪 Optional: Add test for empty file
def test_resolve_authorization_headers_with_empty_file(tmp_path: Path) -> None: """Test that empty file content logs warning and skips header.""" empty_file = tmp_path / "empty.txt" empty_file.write_text("") headers = {"Authorization": str(empty_file)} result = resolve_authorization_headers(headers) # Empty file should result in no header being set assert not resulttests/unit/models/config/test_model_context_protocol_server.py (1)
173-193: Special case keyword tests look good.Tests for
kubernetesandclientspecial cases verify that these keywords are preserved inauthorization_headers.However, these tests don't verify that
resolved_authorization_headersalso preserves these keywords. Consider adding assertions similar totest_model_context_protocol_server_resolved_headers_with_special_values(lines 262-277).🧪 Optional: Add resolved headers assertions
def test_model_context_protocol_server_kubernetes_special_case() -> None: """Test ModelContextProtocolServer with kubernetes special case.""" mcp = ModelContextProtocolServer( name="k8s-server", url="http://localhost:8080", authorization_headers={"Authorization": "kubernetes"}, ) assert mcp is not None assert mcp.authorization_headers == {"Authorization": "kubernetes"} + assert mcp.resolved_authorization_headers == {"Authorization": "kubernetes"} def test_model_context_protocol_server_client_special_case() -> None: """Test ModelContextProtocolServer with client special case.""" mcp = ModelContextProtocolServer( name="client-server", url="http://localhost:8080", authorization_headers={"Authorization": "client"}, ) assert mcp is not None assert mcp.authorization_headers == {"Authorization": "client"} + assert mcp.resolved_authorization_headers == {"Authorization": "client"}src/app/endpoints/query_v2.py (1)
752-771: Nested helper relies on outer loop variable — consider clarity.The
_get_token_valuefunction referencesmcp_serverfrom the enclosing loop scope (line 765). While this works correctly because the function is defined and called within the same loop iteration, it's a subtle pattern that could confuse future maintainers.Consider passing
mcp_server.nameas a parameter for explicitness:♻️ Optional refactor for clarity
- def _get_token_value(original: str, header: str) -> str | None: + def _get_token_value(original: str, header: str, server_name: str) -> str | None: """Convert to header value.""" match original: case "kubernetes": # use k8s token if token is None or token == "": return None return f"Bearer {token}" case "client": # use client provided token if mcp_headers is None: return None - c_headers = mcp_headers.get(mcp_server.name, None) + c_headers = mcp_headers.get(server_name, None) if c_headers is None: return None return c_headers.get(header, None) case _: # use provided return originalThen call as
_get_token_value(value, name, mcp_server.name).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.gitignoreMakefileREADME.mddev-tools/MANUAL_TESTING.mddev-tools/README.mddev-tools/mcp-mock-server/README.mddev-tools/mcp-mock-server/server.pydev-tools/mcp-mock-server/test_mock_mcp_server.pydev-tools/test-configs/llama-stack-mcp-test.yamldev-tools/test-configs/mcp-mock-test.yamlexamples/lightspeed-stack-mcp-servers.yamlsrc/app/endpoints/query_v2.pysrc/configuration.pysrc/models/config.pysrc/utils/mcp_auth_headers.pytests/unit/app/endpoints/test_query_v2.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.py
✅ Files skipped from review due to trivial changes (1)
- src/configuration.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/models/config/test_dump_configuration.py
- dev-tools/README.md
- tests/unit/app/endpoints/test_streaming_query_v2.py
- examples/lightspeed-stack-mcp-servers.yaml
- src/utils/mcp_auth_headers.py
- dev-tools/test-configs/mcp-mock-test.yaml
🧰 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/query_v2.pydev-tools/mcp-mock-server/server.pytests/unit/app/endpoints/test_query_v2.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.pydev-tools/mcp-mock-server/test_mock_mcp_server.pysrc/models/config.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query_v2.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_query_v2.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.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_query_v2.pytests/unit/models/config/test_model_context_protocol_server.pytests/unit/utils/test_mcp_auth_headers.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All config uses Pydantic models extendingConfigurationBase
Configuration base class setsextra="forbid"to reject unknown fields
Use type hints:Optional[FilePath],PositiveInt,SecretStrfor configuration fields
ExtendConfigurationBasefor configuration Pydantic models
Files:
src/models/config.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/config.py
🧠 Learnings (19)
📚 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 `logger.debug()` for detailed diagnostic information
Applied to files:
dev-tools/mcp-mock-server/server.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 `logger.info()` for general information about program execution
Applied to files:
dev-tools/mcp-mock-server/server.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 `logger.warning()` for unexpected events or potential problems
Applied to files:
dev-tools/mcp-mock-server/server.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 `logger.error()` for serious problems that prevented function execution
Applied to files:
dev-tools/mcp-mock-server/server.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 `logger = logging.getLogger(__name__)` pattern for module logging
Applied to files:
dev-tools/mcp-mock-server/server.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_query_v2.pydev-tools/mcp-mock-server/test_mock_mcp_server.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 : Handle `APIConnectionError` from Llama Stack in error handling
Applied to files:
tests/unit/app/endpoints/test_query_v2.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 : Run `uv run make verify` to run all linters (black, pylint, pyright, ruff, docstyle, check-types) before completion
Applied to files:
Makefile
📚 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: Always use `uv run` prefix for all commands
Applied to files:
Makefile
📚 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 : Run `uv run make format` to auto-format code with black and ruff before completion
Applied to files:
Makefile
📚 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: Use `uv sync --group dev --group llslibdev` to install development dependencies
Applied to files:
Makefile
📚 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 src/models/config.py : All config uses Pydantic models extending `ConfigurationBase`
Applied to files:
tests/unit/models/config/test_model_context_protocol_server.pysrc/models/config.py
📚 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:
dev-tools/test-configs/llama-stack-mcp-test.yamlREADME.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 **/*.py : Use absolute imports for internal modules: `from authentication import get_auth_dependency`
Applied to files:
tests/unit/utils/test_mcp_auth_headers.py
📚 Learning: 2026-01-12T10:58:50.251Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:50.251Z
Learning: In the lightspeed-stack repository, when a user claims a fix is done but the code still shows the original issue, verify the current state of the code before accepting the fix.
Applied to files:
dev-tools/mcp-mock-server/test_mock_mcp_server.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 src/models/config.py : Configuration base class sets `extra="forbid"` to reject unknown fields
Applied to files:
src/models/config.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 src/models/config.py : Extend `ConfigurationBase` for configuration Pydantic models
Applied to files:
src/models/config.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/config.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:
README.md
🧬 Code graph analysis (5)
src/app/endpoints/query_v2.py (2)
src/configuration.py (1)
mcp_servers(152-163)src/models/config.py (2)
ModelContextProtocolServer(436-518)resolved_authorization_headers(488-490)
tests/unit/app/endpoints/test_query_v2.py (2)
src/models/config.py (1)
ModelContextProtocolServer(436-518)src/app/endpoints/query_v2.py (1)
get_mcp_tools(719-809)
tests/unit/models/config/test_model_context_protocol_server.py (1)
src/models/config.py (5)
config(330-347)config(1361-1369)Configuration(1537-1698)ModelContextProtocolServer(436-518)resolved_authorization_headers(488-490)
tests/unit/utils/test_mcp_auth_headers.py (1)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-87)
src/models/config.py (2)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-87)src/configuration.py (1)
mcp_servers(152-163)
🪛 Gitleaks (8.30.0)
dev-tools/MANUAL_TESTING.md
[high] 55-57: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 139-141: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
dev-tools/mcp-mock-server/README.md
[high] 96-98: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 208-210: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
README.md
[style] ~300-~300: Consider using “inaccessible” to avoid wordiness.
Context: ...gured in the llama-stack run.yaml are not accessible to lightspeed-core agents. #### Config...
(NOT_ABLE_PREMIUM)
⏰ 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: E2E: server mode / ci
🔇 Additional comments (35)
.gitignore (1)
183-184: LGTM!The addition correctly ignores auto-generated certificates for the MCP mock server. The pattern is appropriately placed under the "Dev stuff" section, and the comment clearly explains what's being ignored.
dev-tools/test-configs/llama-stack-mcp-test.yaml (1)
1-98: LGTM! Well-structured test configuration.The configuration is appropriate for MCP testing purposes:
- Clean separation of concerns with minimal APIs needed for testing
- Environment-sourced API key follows security best practices
- Test-appropriate
/tmpstorage paths provide isolation- Provider configurations align with Llama Stack 0.3.x patterns (no explicit telemetry provider block needed per learnings)
Minor: The file is missing a trailing newline at the end (line 98).
dev-tools/mcp-mock-server/server.py (1)
271-327: LGTM! Clean server lifecycle management.The
main()function properly:
- Starts HTTP and HTTPS servers on consecutive ports
- Uses daemon threads for automatic cleanup
- Handles
KeyboardInterruptgracefully with proper shutdown sequence- Provides clear startup messages with endpoint information
dev-tools/mcp-mock-server/README.md (2)
1-25: Excellent documentation coverage.The README provides comprehensive documentation including:
- Clear purpose statement and feature list
- Quick start guide with both HTTP and HTTPS examples
- Debug endpoint documentation
- Testing scenarios for different authentication methods
- Troubleshooting section
The static analysis warnings about authorization tokens in curl commands (lines 96-98, 208-210) are false positives—these are placeholder test tokens in documentation examples, not actual secrets.
268-277: Good limitations section.Clear documentation of the server's constraints helps set appropriate expectations. The explicit "Testing Only" warnings throughout prevent misuse in production contexts.
dev-tools/MANUAL_TESTING.md (2)
86-116: Clear test verification steps.The documentation clearly explains:
- What each MCP server tests (file-based, Kubernetes, client auth)
- Expected request count (6 requests: 2 per server)
- How to verify via mock server logs and debug endpoints
- Expected tool names returned for each auth type
This makes manual testing straightforward and verifiable.
132-149: Good coverage of edge case behavior.The "Test Without Client Headers" scenario documents the important behavioral change mentioned in PR discussions—that
mock-client-authwill be skipped when required client headers are not provided. This aligns with the security-focused design decision to require explicit header configuration.tests/unit/utils/test_mcp_auth_headers.py (3)
1-12: Good test coverage for empty input handling.The test file properly validates the
resolve_authorization_headersfunction. The import patternfrom utils.mcp_auth_headers import resolve_authorization_headersfollows the project's test import conventions.The empty input test correctly verifies that an empty dict input returns an empty dict output.
15-46: Comprehensive file-based header tests.Good coverage of:
- Basic file reading
- Whitespace trimming (including trailing newlines)
- Nonexistent file handling (returns empty dict, logs warning)
49-104: Good special keyword preservation tests.Tests correctly verify that
"client"and"kubernetes"keywords are preserved as-is in the resolved headers for later substitution at request time. The mixed types test validates that these work alongside file-based headers.tests/unit/models/config/test_model_context_protocol_server.py (4)
21-28: Good baseline test with default verification.The constructor test now includes assertion for
authorization_headers == {}(line 28), verifying the default empty dict behavior. This is important for backward compatibility.
143-170: Comprehensive authorization headers test.Good test that verifies:
authorization_headersstores the raw file pathsresolved_authorization_headerscontains the actual secret values read from files- Multiple headers can be resolved simultaneously
195-260: Excellent mixed configuration test.The
test_configuration_mcp_servers_with_mixed_auth_headerstest is thorough:
- Tests backward compatibility with servers without auth headers
- Tests file-based secret resolution
- Tests special case keywords (kubernetes, client)
- Verifies both raw and resolved headers
Good use of
AuthenticationConfiguration(module="k8s")to ensure the test reflects realistic configuration.
262-305: Good edge case coverage.Tests properly cover:
- Special values preserved in resolved headers
- File content correctly read into resolved headers
- Empty resolved headers when no auth configured
This provides confidence in the header resolution behavior.
dev-tools/mcp-mock-server/test_mock_mcp_server.py (5)
22-74: Robust server fixture with proper lifecycle management.The
mock_serverfixture is well-implemented:
- Health check polling replaces blind sleep—more reliable
- Proper error handling if server crashes during startup
- Graceful shutdown with timeout fallback to kill
- Process handles captured for cleanup
The comment about fixed ports and parallel test execution is a good callout for future consideration.
76-119: Clean request helper with appropriate SSL handling.The
make_requesthelper properly:
- Handles both GET and POST methods
- Sets Content-Type for JSON payloads
- Uses
ssl._create_unverified_context()appropriately for self-signed cert testing- Handles both success and error responses
The unreachable return on line 119 after
pytest.fail()is acceptable as it satisfies the type checker whilepytest.fail()actually raises.
122-157: Good HTTP and HTTPS endpoint tests.Tests verify:
- Correct JSON-RPC response structure
- Tool listing functionality
- Both HTTP and HTTPS protocols work
The tool name validation (
startswith("mock_tool")) accommodates the dynamic naming based on auth header content.
159-218: Comprehensive debug endpoint tests.Good coverage of:
- Header capture functionality with custom headers
- Request logging with timestamps, method, path, headers
- Both Authorization and custom headers are captured
275-300: Useful request counting test with clear documentation.The docstring explains the important behavior that only POST requests increment the count—this matches the mock server implementation where only
do_POSTcalls_capture_headers. This prevents confusion about GET requests not being logged.src/app/endpoints/query_v2.py (2)
510-512: LGTM - Good defensive check for None response.Adding the
response is Nonecheck prevents potentialAttributeErrorwhen the agent fails and returns None. This defensive pattern improves reliability.
792-802: Server skip logic is correct and aligns with security-by-default approach.The logic properly ensures that MCP servers with incomplete authentication are excluded from tool invocations, preventing potential authentication failures or unauthorized access. The warning log provides good observability.
README.md (3)
300-301: Documentation clearly explains the MCP configuration behavior.The documentation accurately describes the new authentication mechanisms and the behavioral change where client-provided MCP-HEADERS are now ignored unless explicitly configured. This aligns with the PR discussion about making execution more predictable.
390-392: Excellent clarification on MCP-HEADERS structure.The explicit note that MCP-HEADERS is keyed by server name (not URL) is crucial for users to correctly configure client-provided tokens. This matches the implementation where
mcp_headers.get(mcp_server.name, ...)is used.
1107-1135: Helpful addition for development and testing workflows.The Development Tools section with the MCP Mock Server documentation will help developers test MCP integrations locally. The quick start example demonstrates the new authorization_headers configuration format.
tests/unit/app/endpoints/test_query_v2.py (7)
58-84: Tests correctly verify kubernetes token resolution.The test properly verifies that:
- Servers without auth headers don't include headers in tool definition
- Servers with
kubernetesauth include theBearer {token}format
86-114: Good test coverage for client-provided token resolution.The test verifies both the happy path (headers correctly applied) and the skip behavior when required client headers are not provided. This aligns with the server skipping logic in
get_mcp_tools.
116-133: Static header test properly verifies file-based secret resolution.Good use of
tmp_pathfixture for creating temporary secret files. The test verifies that the file content is correctly resolved and applied to headers.
135-166: Comprehensive test for mixed authentication methods.This test validates the critical scenario where a single MCP server uses multiple authentication methods simultaneously. It ensures all three resolution paths (kubernetes, file, client) work correctly together.
168-198: Good coverage of server skipping edge cases.The test validates that servers are correctly excluded when:
- Kubernetes token is required but not provided
- Client headers are required but not provided
- Multiple headers are required but only some are resolvable
This ensures the security-by-default behavior works as designed.
200-216: Ensures backward compatibility for unauthenticated MCP servers.This test confirms that MCP servers without authentication requirements continue to work, which is essential for backward compatibility with existing configurations.
286-314: Integration test correctly validates end-to-end MCP tool construction.The test verifies that
ModelContextProtocolServerwithauthorization_headersconfigured flows through correctly to produce the expectedBearer {token}header format in the tool definition.src/models/config.py (4)
5-6: Imports follow coding guidelines.The new imports are correctly organized:
loggingfor module-level loggingPrivateAttrfrom pydantic for private model attributesresolve_authorization_headersfrom utils moduleLogger initialization uses the
logger = logging.getLogger(__name__)pattern as specified in coding guidelines.Also applies to: 25-26, 34-36
470-518: Well-structured authorization header configuration.The implementation:
- Uses
dict[str, str]forauthorization_headerswith sensible default- Private attribute
_resolved_authorization_headerskeeps resolved values encapsulated- Property provides read-only access to resolved headers
- Model validator automatically resolves headers during construction
This follows Pydantic best practices and the coding guidelines for configuration models.
1647-1684: Validator correctly prevents kubernetes auth misconfiguration.The validator properly:
- Detects MCP servers with
kubernetesauth when auth module is notk8s- Removes misconfigured servers with a clear warning message
- Suggests remediation options (change auth module or update headers)
This fail-safe approach prevents runtime errors by filtering at configuration load time.
1686-1697: Good API improvement for dump method.Accepting
str | Pathprovides better ergonomics for callers usingpathlib.Pathobjects. The implementation correctly handles both types since Python'sopen()accepts either.
| def generate_self_signed_cert(cert_dir: Path) -> tuple[Path, Path]: | ||
| """Generate self-signed certificate for HTTPS testing. | ||
| Args: | ||
| cert_dir: Directory to store certificate files | ||
| Returns: | ||
| Tuple of (cert_file, key_file) paths | ||
| """ | ||
| cert_file = cert_dir / "cert.pem" | ||
| key_file = cert_dir / "key.pem" | ||
|
|
||
| # Only generate if files don't exist | ||
| if cert_file.exists() and key_file.exists(): | ||
| return cert_file, key_file | ||
|
|
||
| cert_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Generate self-signed certificate using openssl | ||
| try: | ||
| subprocess.run( | ||
| [ | ||
| "openssl", | ||
| "req", | ||
| "-x509", | ||
| "-newkey", | ||
| "rsa:4096", | ||
| "-keyout", | ||
| str(key_file), | ||
| "-out", | ||
| str(cert_file), | ||
| "-days", | ||
| "365", | ||
| "-nodes", | ||
| "-subj", | ||
| "/CN=localhost", | ||
| ], | ||
| check=True, | ||
| capture_output=True, | ||
| ) | ||
| print(f"Generated self-signed certificate: {cert_file}") | ||
| except subprocess.CalledProcessError as e: | ||
| print(f"Failed to generate certificate: {e}") | ||
| raise | ||
|
|
||
| return cert_file, key_file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the server.py file and view the relevant lines
find . -name "server.py" -path "*/mcp-mock-server/*" | head -5Repository: lightspeed-core/lightspeed-stack
Length of output: 113
🏁 Script executed:
# Check for .gitignore files
find . -name ".gitignore" | head -20Repository: lightspeed-core/lightspeed-stack
Length of output: 88
🏁 Script executed:
# Search for .certs references in .gitignore files
grep -r "\.certs\|certs" .gitignore 2>/dev/null || echo "No .certs entries found in root .gitignore"Repository: lightspeed-core/lightspeed-stack
Length of output: 109
🏁 Script executed:
cat -n ./dev-tools/mcp-mock-server/server.py | sed -n '205,250p'Repository: lightspeed-core/lightspeed-stack
Length of output: 1677
🏁 Script executed:
# Also check the imports and module structure at the beginning
head -50 ./dev-tools/mcp-mock-server/server.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 1566
🏁 Script executed:
# Check if logging is imported and how it's used in the file
head -100 ./dev-tools/mcp-mock-server/server.py | grep -E "^import|^from|logging"Repository: lightspeed-core/lightspeed-stack
Length of output: 281
🏁 Script executed:
# Check if logger is defined in the file
grep -n "logger\|logging" ./dev-tools/mcp-mock-server/server.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# View the actual .gitignore content to confirm .certs entry
cat .gitignoreRepository: lightspeed-core/lightspeed-stack
Length of output: 3822
Replace print() calls with logging statements per coding guidelines.
The function's error handling and certificate logic are solid. However, update lines 245 and 247 to use the standard logging pattern:
- Add to imports:
import logging - Add below imports:
logger = logging.getLogger(__name__) - Replace
print(f"Generated self-signed certificate: {cert_file}")withlogger.info(f"Generated self-signed certificate: {cert_file}") - Replace
print(f"Failed to generate certificate: {e}")withlogger.error(f"Failed to generate certificate: {e}")
The .certs/ directory is already properly gitignored in .gitignore.
🤖 Prompt for AI Agents
In @dev-tools/mcp-mock-server/server.py around lines 205 - 250, Update
generate_self_signed_cert to use the logging module instead of print: add
"import logging" and "logger = logging.getLogger(__name__)" near the top-level
imports, replace the print on success inside generate_self_signed_cert with
logger.info including the cert_file, and replace the print in the except block
with logger.error including exception details (use the caught exception variable
e).
b379d1c to
e3ed845
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/models/config.py (1)
1647-1684: Configuration validator correctly enforces kubernetes auth module requirement.The validator properly removes MCP servers that specify
"kubernetes"auth headers when the authentication module isn't"k8s". The warning log provides good diagnostics for operators.However, consider whether silently removing misconfigured servers is the right UX—an alternative would be to raise a
ValueErrorto make the misconfiguration explicit at startup. The current approach is more lenient but could mask configuration errors.💡 Alternative: Fail fast on misconfiguration
If stricter validation is preferred:
- logger.warning( - "Removing MCP server '%s': has authorization_headers with " - "value 'kubernetes' but authentication module is '%s' " - "(not 'k8s'). Either change authentication.module to 'k8s' " - "or update the MCP server's authorization_headers to use a " - "file path or 'client'.", - mcp_server.name, - auth_module, - ) - is_valid = False - break + raise ValueError( + f"MCP server '{mcp_server.name}' has authorization_headers with " + f"value 'kubernetes' but authentication module is '{auth_module}' " + "(not 'k8s'). Either change authentication.module to 'k8s' " + "or update the MCP server's authorization_headers to use a " + "file path or 'client'." + )src/app/endpoints/query_v2.py (1)
753-771: The_get_token_valueclosure capturesmcp_serverfrom outer loop—verify correctness.The nested function
_get_token_valuereferencesmcp_serverfrom the enclosing loop scope (line 765). This works correctly in Python because the function is called immediately within the same iteration, but it's a subtle coupling.Consider passing
mcp_server.nameas an explicit parameter for clarity:💡 Explicit parameter for clarity
- def _get_token_value(original: str, header: str) -> str | None: + def _get_token_value(original: str, header: str, server_name: str) -> str | None: """Convert to header value.""" match original: case "kubernetes": # use k8s token if token is None or token == "": return None return f"Bearer {token}" case "client": # use client provided token if mcp_headers is None: return None - c_headers = mcp_headers.get(mcp_server.name, None) + c_headers = mcp_headers.get(server_name, None) if c_headers is None: return None return c_headers.get(header, None) case _: # use provided return originalThen update the call site:
- h_value = _get_token_value(value, name) + h_value = _get_token_value(value, name, mcp_server.name)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/endpoints/query_v2.pysrc/models/config.pytests/unit/app/endpoints/test_query_v2.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/models/config/test_model_context_protocol_server.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:
tests/unit/models/config/test_model_context_protocol_server.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_query_v2.pysrc/models/config.pysrc/app/endpoints/query_v2.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/models/config/test_model_context_protocol_server.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_query_v2.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/models/config/test_model_context_protocol_server.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_query_v2.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All config uses Pydantic models extendingConfigurationBase
Configuration base class setsextra="forbid"to reject unknown fields
Use type hints:Optional[FilePath],PositiveInt,SecretStrfor configuration fields
ExtendConfigurationBasefor configuration Pydantic models
Files:
src/models/config.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/config.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query_v2.py
🧠 Learnings (6)
📚 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 src/models/config.py : All config uses Pydantic models extending `ConfigurationBase`
Applied to files:
tests/unit/models/config/test_model_context_protocol_server.pysrc/models/config.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_query_v2.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 : Handle `APIConnectionError` from Llama Stack in error handling
Applied to files:
tests/unit/app/endpoints/test_query_v2.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 src/models/config.py : Configuration base class sets `extra="forbid"` to reject unknown fields
Applied to files:
src/models/config.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 src/models/config.py : Extend `ConfigurationBase` for configuration Pydantic models
Applied to files:
src/models/config.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/config.py
🧬 Code graph analysis (5)
tests/unit/models/config/test_model_context_protocol_server.py (2)
src/models/config.py (5)
config(330-347)config(1361-1369)Configuration(1537-1698)ModelContextProtocolServer(436-518)resolved_authorization_headers(488-490)src/configuration.py (1)
mcp_servers(152-163)
tests/unit/app/endpoints/test_streaming_query_v2.py (1)
src/models/config.py (1)
ModelContextProtocolServer(436-518)
tests/unit/app/endpoints/test_query_v2.py (2)
src/models/config.py (1)
ModelContextProtocolServer(436-518)src/app/endpoints/query_v2.py (1)
get_mcp_tools(719-809)
src/models/config.py (2)
src/utils/mcp_auth_headers.py (1)
resolve_authorization_headers(9-87)src/configuration.py (1)
mcp_servers(152-163)
src/app/endpoints/query_v2.py (2)
src/models/config.py (4)
config(330-347)config(1361-1369)ModelContextProtocolServer(436-518)resolved_authorization_headers(488-490)src/configuration.py (1)
mcp_servers(152-163)
⏰ 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). (2)
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (20)
src/models/config.py (3)
5-5: LGTM! Import additions are appropriate.The new imports for
logging,PrivateAttr, andresolve_authorization_headersare necessary for the MCP authorization feature. Logger initialization follows the project's standard pattern.Also applies to: 25-25, 34-36
470-518: Well-structured authorization headers implementation.The design cleanly separates configuration (
authorization_headers) from resolved values (_resolved_authorization_headers). UsingPrivateAttrfor the resolved headers prevents serialization while the property provides read-only access. The@model_validator(mode="after")correctly triggers resolution after all fields are validated.One consideration: the validator silently continues when
authorization_headersis empty (line 514 condition). This is appropriate since servers without auth headers should work without resolution.
1686-1696: LGTM! Dump method signature update uses modern union syntax.The signature change from
strtostr | Pathfollows the coding guidelines for modern union syntax and improves API flexibility.src/app/endpoints/query_v2.py (4)
25-25: LGTM! Import addition is necessary.The
ModelContextProtocolServerimport is required for the updated type annotation inget_mcp_tools.
510-512: LGTM! Defensive None check added.The added check for
response is Nonebefore accessingresponse.outputimproves robustness against edge cases where the agent may fail.
793-802: Server skipping logic is correct and well-documented.The comparison
len(headers) != len(mcp_server.authorization_headers)correctly identifies when not all configured headers could be resolved. The warning log provides actionable information. This ensures servers requiring authentication aren't called without proper credentials.
719-809: Overall implementation is solid with clear header resolution algorithm.The three-tier header resolution (kubernetes token → client-provided → static file) is well-designed and documented. The algorithm correctly:
- Skips servers when required auth headers can't be resolved
- Includes servers without auth requirements unconditionally
- Attaches resolved headers to tool definitions
This aligns with the PR objective of making MCP execution more predictable by requiring explicit configuration.
tests/unit/app/endpoints/test_streaming_query_v2.py (1)
63-70: LGTM! Test setup correctly exercises new authorization_headers field.The test now includes
authorization_headers={"Authorization": "kubernetes"}to verify the new per-server header functionality. The additional patch at line 70 forapp.endpoints.query_v2.configurationis necessary becauseprepare_tools_for_responses_apiis imported fromquery_v2and used bystreaming_query_v2.tests/unit/models/config/test_model_context_protocol_server.py (6)
3-6: LGTM! Import additions support new test scenarios.The added imports (
Path,AuthenticationConfiguration,LlamaStackConfiguration,UserDataCollection) are necessary for the expanded test coverage.Also applies to: 11-18
28-28: LGTM! Default value assertion added.Adding
assert mcp.authorization_headers == {}verifies backward compatibility—servers without explicit auth headers default to an empty dict.
143-170: Good test coverage for file-based header resolution.The test correctly verifies that:
- Original
authorization_headerspreserves file pathsresolved_authorization_headerscontains the actual file contentsUsing
tmp_pathfixture follows pytest best practices for temporary file handling.
173-192: Tests verify special case preservation.Both
test_model_context_protocol_server_kubernetes_special_caseandtest_model_context_protocol_server_client_special_caseconfirm that"kubernetes"and"client"values are preserved inauthorization_headersfor later runtime substitution.
195-259: Comprehensive integration test for mixed auth headers.This test exercises the full Configuration model with multiple MCP servers using different header types (none, file-based, kubernetes, client). Setting
authentication=AuthenticationConfiguration(module="k8s")ensures the kubernetes-auth server isn't filtered out byvalidate_mcp_auth_headers.The assertions at lines 241-259 thoroughly verify each server's configuration.
262-305: LGTM! Additional edge case coverage.Tests for:
resolved_authorization_headerspreserving special values (lines 262-277)- File content being read into resolved headers (lines 280-295)
- Empty resolved headers when no auth configured (lines 298-305)
This provides good coverage of the resolution logic edge cases.
tests/unit/app/endpoints/test_query_v2.py (6)
1-5: LGTM! File-level directives and imports updated.The
Pathimport supports temporary file handling in new tests. The pylint/pyright directives are appropriate for this test file.
58-84: Good test restructuring for clarity.The test is now split into two logical sections:
- Servers without authorization headers (lines 61-71)
- Servers with kubernetes auth (lines 74-83)
The assertions verify both cases work correctly: no headers when auth isn't configured, and
Bearertoken added when kubernetes auth is used.
86-113: Thorough test for client-provided headers.The test verifies:
- Headers are correctly resolved from
mcp_headerswhen auth type is"client"(lines 104-109)- Servers are skipped when
mcp_headers=Nonebut client auth is required (lines 111-113)This matches the implementation's skipping behavior for unresolvable auth.
116-165: Excellent coverage for static and mixed header scenarios.
test_get_mcp_tools_with_static_headers(lines 116-132) verifies file-based secret resolution.
test_get_mcp_tools_with_mixed_headers(lines 135-165) tests the most complex scenario: a server with kubernetes, file-based, and client headers simultaneously. The assertion at lines 161-165 confirms all three header types resolve correctly when all inputs are provided.
168-215: Critical edge case coverage for server skipping.
test_get_mcp_tools_skips_server_with_missing_auth(lines 168-197) tests three scenarios:
- Kubernetes auth without token
- Client auth without mcp_headers
- Partial auth (2 required, 1 available)
All servers are correctly skipped, ensuring no unauthenticated requests are made.
test_get_mcp_tools_includes_server_without_auth(lines 200-215) confirms servers withauthorization_headers={}are always included—important for backward compatibility.
286-314: Integration test updated to exercise per-server headers.The
ModelContextProtocolServerconfiguration now includesauthorization_headers={"Authorization": "kubernetes"}, and the assertion at line 314 verifies the token is correctly resolved toBearer mytoken.
|
/ok-to-test |
4 participants
…ual testing support
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-1068
https://issues.redhat.com/browse/LCORE-799
https://issues.redhat.com/browse/LCORE-1068
https://issues.redhat.com/browse/LCORE-799
Checklist before requesting a review
Testing
unit testing, manual e2e testing
Summary by CodeRabbit
New Features
Documentation
Tools
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.