Add configuration for MCP servers, updated documentation, created man… #972
Conversation
…ual testing support
|
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 header fields and resolution (including file-backed, Kubernetes, and client-forwarded tokens), updates models and the MCP query endpoint to resolve and skip servers with unresolved auth, and introduces a self-contained MCP mock server with docs, tests, and test configs. Minor tooling, lint, and .gitignore updates included. 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: read configured mcp_servers
ConfigModel->>AuthUtils: resolve_authorization_headers(authorization_headers)
AuthUtils-->>ConfigModel: resolved_authorization_headers
QueryEndpoint->>QueryEndpoint: iterate servers, build per-server headers
alt Server has resolved headers
QueryEndpoint->>MCPServer: POST / (MCP request with resolved headers)
MCPServer-->>QueryEndpoint: tool list / response
else Server auth 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.
| def resolve_authorization_headers( | ||
| authorization_headers: dict[str, str], | ||
| ) -> dict[str, str]: | ||
| """ | ||
| Resolve authorization headers by reading secret files or preserving special values. | ||
| 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. | ||
| - Otherwise: Treat as file path and read the secret from that file | ||
| Returns: | ||
| dict[str, str]: Map of header names to resolved header values or special keywords | ||
| Examples: | ||
| >>> # With file paths | ||
| >>> resolve_authorization_headers({"Authorization": "/var/secrets/token"}) | ||
| {"Authorization": "secret-value-from-file"} | ||
| >>> # With kubernetes special case (kept as-is) | ||
| >>> resolve_authorization_headers({"Authorization": "kubernetes"}) | ||
| {"Authorization": "kubernetes"} | ||
| >>> # With client special case (kept as-is) | ||
| >>> resolve_authorization_headers({"Authorization": "client"}) | ||
| {"Authorization": "client"} | ||
| """ | ||
| resolved: dict[str, str] = {} | ||
|
|
||
| for header_name, value in authorization_headers.items(): | ||
| try: | ||
| if value == "kubernetes": | ||
| # Special case: Keep kubernetes keyword for later substitution | ||
| resolved[header_name] = "kubernetes" | ||
| logger.debug( | ||
| "Header %s will use Kubernetes token (resolved at request time)", | ||
| header_name, | ||
| ) | ||
| elif value == "client": | ||
| # Special case: Keep client keyword for later substitution | ||
| resolved[header_name] = "client" | ||
| logger.debug( | ||
| "Header %s will use client-provided token (resolved at request time)", | ||
| header_name, | ||
| ) | ||
| else: | ||
| # Regular case: Read secret from file path | ||
| secret_path = Path(value) | ||
| if secret_path.exists() and secret_path.is_file(): | ||
| with open(secret_path, "r", encoding="utf-8") as secret_file: | ||
| secret_value = secret_file.read().strip() | ||
| resolved[header_name] = secret_value | ||
| logger.debug( | ||
| "Resolved header %s from secret file %s", | ||
| header_name, | ||
| secret_path, | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| "Secret file not found or not a file: %s for header %s", | ||
| secret_path, | ||
| header_name, | ||
| ) | ||
| except Exception as e: # pylint: disable=broad-except | ||
| logger.error( | ||
| "Failed to resolve authorization header %s with value %s: %s", | ||
| header_name, | ||
| value, | ||
| e, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Don’t log raw header values; treat empty secrets as unresolved.
Current error logging includes value, which can leak secrets if a literal token is ever passed (or ends up in exception text). Also, a whitespace-only secret file becomes "" and will be forwarded as a header value.
Proposed fix
def resolve_authorization_headers(
authorization_headers: dict[str, str],
) -> dict[str, str]:
@@
- for header_name, value in authorization_headers.items():
+ for header_name, value in authorization_headers.items():
try:
@@
else:
# Regular case: Read secret from file path
secret_path = Path(value)
if secret_path.exists() and secret_path.is_file():
- with open(secret_path, "r", encoding="utf-8") as secret_file:
- secret_value = secret_file.read().strip()
- resolved[header_name] = secret_value
+ secret_value = secret_path.read_text(encoding="utf-8").strip()
+ if not secret_value:
+ logger.warning(
+ "Secret file %s for header %s is empty after trimming; skipping",
+ secret_path,
+ header_name,
+ )
+ continue
+ resolved[header_name] = secret_value
logger.debug(
"Resolved header %s from secret file %s",
header_name,
secret_path,
)
else:
logger.warning(
"Secret file not found or not a file: %s for header %s",
secret_path,
header_name,
)
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 (source redacted): %s",
header_name,
- value,
e,
)🤖 Prompt for AI Agents
In @src/utils/mcp_auth_headers.py around lines 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.
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
| 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:
# Check for README and dependency documentation in dev-tools/mcp-mock-server/
find dev-tools/mcp-mock-server -maxdepth 2 -type f \( -name "README*" -o -name "setup.py" -o -name "pyproject.toml" -o -name "requirements*.txt" \) | head -20Repository: lightspeed-core/lightspeed-stack
Length of output: 111
🏁 Script executed:
# Check if openssl is mentioned anywhere in documentation or config
rg -i "openssl" dev-tools/mcp-mock-server/ --type-list | head -20Repository: lightspeed-core/lightspeed-stack
Length of output: 465
🏁 Script executed:
# Look at the broader context of the server.py file - imports and setup
head -50 dev-tools/mcp-mock-server/server.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 1566
🏁 Script executed:
# Check for any error handling documentation or comments about dependencies
rg -B5 -A5 "openssl" dev-tools/mcp-mock-server/server.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 466
🏁 Script executed:
# Check if there's a root-level or parent documentation about setup
ls -la dev-tools/mcp-mock-server/Repository: lightspeed-core/lightspeed-stack
Length of output: 383
🏁 Script executed:
cat dev-tools/mcp-mock-server/README.mdRepository: lightspeed-core/lightspeed-stack
Length of output: 6900
🏁 Script executed:
# Also check the test file to see how certificate generation is tested
grep -n "generate_self_signed_cert\|openssl" dev-tools/mcp-mock-server/test_mock_mcp_server.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# Check if there's any subprocess error handling or fallback mechanisms documented
grep -B10 -A10 "CalledProcessError" dev-tools/mcp-mock-server/server.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 671
Update the README to document the openssl binary requirement and improve error messaging.
The README claims "✅ Pure Python - No external dependencies (uses stdlib only)" but the certificate generation code (lines 205-250) requires the openssl binary to be installed and available in the system PATH. This is misleading for users.
Additionally, improve the error message to explicitly inform users when openssl is not found or fails, rather than the generic "Failed to generate certificate" output.
🤖 Prompt for AI Agents
In @dev-tools/mcp-mock-server/server.py around lines 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.
| 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.
| ``` | ||
| ====================================================================== | ||
| MCP Mock Server starting with HTTP and HTTPS | ||
| ====================================================================== | ||
| HTTP: http://localhost:3000 | ||
| HTTPS: https://localhost:3001 | ||
| ====================================================================== | ||
| Debug endpoints: | ||
| • /debug/headers - View captured headers | ||
| • /debug/requests - View request log | ||
| MCP endpoint: | ||
| • POST /mcp/v1/list_tools | ||
| ====================================================================== | ||
| Note: HTTPS uses a self-signed certificate (for testing only) | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to code block.
Static analysis correctly identified a fenced code block without a language specified. Add text or bash to improve rendering.
📝 Proposed fix
You should see:
-```
+```text
======================================================================
MCP Mock Server starting with HTTP and HTTPS
======================================================================🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @dev-tools/mcp-mock-server/README.md around lines 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.
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.
| ``` | ||
| ====================================================================== | ||
| MCP Mock Server starting with HTTP and HTTPS | ||
| ====================================================================== | ||
| HTTP: http://localhost:3000 | ||
| HTTPS: https://localhost:3001 | ||
| ====================================================================== | ||
| Debug endpoints: | ||
| • /debug/headers - View captured headers | ||
| • /debug/requests - View request log | ||
| MCP endpoint: | ||
| • POST /mcp/v1/list_tools | ||
| ====================================================================== | ||
| Note: HTTPS uses a self-signed certificate (for testing only) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Add a language to the fenced block to satisfy MD040.
Use text for the “You should see” console output block.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @dev-tools/mcp-mock-server/README.md around lines 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.
| 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).
| authorization_headers: dict[str, str] = Field( | ||
| default_factory=dict, | ||
| title="Authorization headers", | ||
| description=( | ||
| "Headers to send to the MCP server. " | ||
| "The map contains the header name and the path to a file containing " | ||
| "the header value (secret). " | ||
| "There are 2 special cases: " | ||
| "1. Usage of the kubernetes token in the header. " | ||
| "To specify this use a string 'kubernetes' instead of the file path. " | ||
| "2. Usage of the client provided token in the header. " | ||
| "To specify this use a string 'client' instead of the file path." | ||
| ), | ||
| ) | ||
|
|
||
| resolved_authorization_headers: dict[str, str] = Field( | ||
| default_factory=dict, | ||
| init=False, | ||
| exclude=True, | ||
| title="Resolved authorization headers", | ||
| description=( | ||
| "Resolved authorization headers with file paths converted to actual values. " | ||
| "This field is automatically populated from authorization_headers. " | ||
| "Special values 'kubernetes' and 'client' are preserved for runtime substitution." | ||
| ), | ||
| ) | ||
|
|
||
| timeout: int | 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." | ||
| ), | ||
| ) | ||
|
|
||
| @model_validator(mode="after") | ||
| def resolve_auth_headers(self) -> Self: | ||
| """ | ||
| Resolve authorization headers by reading secret files. | ||
| Automatically populates resolved_authorization_headers by reading | ||
| secret files specified in authorization_headers. Special values | ||
| 'kubernetes' and 'client' are preserved for later substitution. | ||
| Returns: | ||
| Self: The model instance with resolved_authorization_headers populated. | ||
| """ | ||
| if self.authorization_headers: | ||
| self.resolved_authorization_headers = resolve_authorization_headers( | ||
| self.authorization_headers | ||
| ) | ||
| return self | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Pydantic v2 BaseModel Field init=False behavior prevent user input initialization
💡 Result:
Short answer: Field(init=False) is not a way to block user-provided values on BaseModel fields in Pydantic v2. That behavior is for dataclass/private-attribute helpers — to make an attribute ignored by validation/constructor you should use a private attribute (PrivateAttr / leading underscore) or adjust model config (e.g. extra='forbid').
Key points and examples
-
Field(init=False) is relevant to dataclasses / private-attribute helpers and is not how BaseModel prevents user input. (docs.pydantic.dev)
-
To create an attribute that is not set/validated during model construction, use PrivateAttr (and name it with a leading underscore). PrivateAttrs are not validated or set via init/model_validate. Example:
from pydantic import BaseModel, PrivateAttrclass M(BaseModel):
_secret: int = PrivateAttr(default=1)M(secret=5) # _secret is not set from input
(docs.pydantic.dev) -
If your goal is to prevent callers from passing unknown/extra fields or to reject overrides, use model config (ConfigDict) with extra='forbid' or validate by name/alias as appropriate. (pydantic.dev)
If you want, tell me which exact behavior you need (reject input, ignore input, or make a read-only computed field) and I’ll show the minimal code.
Citations:
- 1: https://docs.pydantic.dev/dev/errors/usage_errors/?utm_source=openai
- 2: https://docs.pydantic.dev/2.5/concepts/models/?utm_source=openai
- 3: https://pydantic.dev/articles/pydantic-v2-11-release?utm_source=openai
🏁 Script executed:
head -20 src/models/config.py && echo "---" && sed -n '1,50p' src/models/config.py | grep -E "class ConfigurationBase|extra"Repository: lightspeed-core/lightspeed-stack
Length of output: 575
🏁 Script executed:
# Get more context on ConfigurationBase and the ModelContextProtocolServer class
rg -A 5 "class ConfigurationBase" src/models/config.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 239
🏁 Script executed:
# Check the ModelContextProtocolServer class definition
rg -B 5 -A 60 "class ModelContextProtocolServer" src/models/config.py | head -100Repository: lightspeed-core/lightspeed-stack
Length of output: 2447
🏁 Script executed:
# Get the timeout field and validator method
sed -n '459,513p' src/models/config.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 2289
🏁 Script executed:
# Check if there's a resolve_authorization_headers function imported or defined
rg -B 3 "resolve_authorization_headers" src/models/config.py | head -30Repository: lightspeed-core/lightspeed-stack
Length of output: 402
🏁 Script executed:
# Check imports to see what's imported from pydantic
sed -n '1,30p' src/models/config.py | grep -A 10 "from pydantic import"Repository: lightspeed-core/lightspeed-stack
Length of output: 245
🏁 Script executed:
# Verify the resolve_authorization_headers function signature
rg -A 5 "def resolve_authorization_headers" src/Repository: lightspeed-core/lightspeed-stack
Length of output: 451
Use PrivateAttr for resolved_authorization_headers to prevent user override; validate timeout as PositiveInt.
In Pydantic v2 BaseModel, Field(init=False) does not prevent a field from being set via input—even though ConfigurationBase uses extra='forbid', this only rejects unknown fields. A user could provide resolved_authorization_headers directly in YAML (e.g., if authorization_headers is empty), and the "after" validator won't overwrite it. Additionally, timeout should be typed as PositiveInt | None rather than int | None to enforce a positive integer constraint.
Suggested fix
from pydantic import (
BaseModel,
ConfigDict,
Field,
model_validator,
FilePath,
AnyHttpUrl,
PositiveInt,
NonNegativeInt,
SecretStr,
+ PrivateAttr,
)
class ModelContextProtocolServer(ConfigurationBase):
"""..."""
- resolved_authorization_headers: dict[str, str] = Field(
- default_factory=dict,
- init=False,
- exclude=True,
- title="Resolved authorization headers",
- description=(
- "Resolved authorization headers with file paths converted to actual values. "
- "This field is automatically populated from authorization_headers. "
- "Special values 'kubernetes' and 'client' are preserved for runtime substitution."
- ),
- )
+ _resolved_authorization_headers: dict[str, str] = PrivateAttr(default_factory=dict)
+
+ @property
+ def resolved_authorization_headers(self) -> dict[str, str]:
+ """Resolved authorization headers (computed from authorization_headers)."""
+ return self._resolved_authorization_headers
- 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."
),
)
@model_validator(mode="after")
def resolve_auth_headers(self) -> Self:
"""
Resolve authorization headers by reading secret files.
Automatically populates resolved_authorization_headers by reading
secret files specified in authorization_headers. Special values
'kubernetes' and 'client' are preserved for later substitution.
Returns:
Self: The model instance with resolved_authorization_headers populated.
"""
- if self.authorization_headers:
- self.resolved_authorization_headers = resolve_authorization_headers(
- self.authorization_headers
- )
+ # Always overwrite computed value (never trust input).
+ self._resolved_authorization_headers = resolve_authorization_headers(
+ self.authorization_headers or {}
+ )
return self🤖 Prompt for AI Agents
In @src/models/config.py around lines 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.
| for header_name, value in authorization_headers.items(): | ||
| try: | ||
| if value == "kubernetes": | ||
| # Special case: Keep kubernetes keyword for later substitution | ||
| resolved[header_name] = "kubernetes" | ||
| logger.debug( | ||
| "Header %s will use Kubernetes token (resolved at request time)", | ||
| header_name, | ||
| ) | ||
| elif value == "client": | ||
| # Special case: Keep client keyword for later substitution | ||
| resolved[header_name] = "client" | ||
| logger.debug( | ||
| "Header %s will use client-provided token (resolved at request time)", | ||
| header_name, | ||
| ) | ||
| else: | ||
| # Regular case: Read secret from file path | ||
| secret_path = Path(value) | ||
| if secret_path.exists() and secret_path.is_file(): | ||
| with open(secret_path, "r", encoding="utf-8") as secret_file: | ||
| secret_value = secret_file.read().strip() | ||
| resolved[header_name] = secret_value | ||
| logger.debug( | ||
| "Resolved header %s from secret file %s", | ||
| header_name, | ||
| secret_path, | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| "Secret file not found or not a file: %s for header %s", | ||
| secret_path, | ||
| header_name, | ||
| ) | ||
| except Exception as e: # pylint: disable=broad-except | ||
| logger.error( | ||
| "Failed to resolve authorization header %s with value %s: %s", | ||
| header_name, | ||
| value, | ||
| e, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Avoid logging raw header “value”; add basic normalization & file helpers.
If a user accidentally puts the literal token in authorization_headers (instead of a path), the except block logs it verbatim (Line 75-78). Also, trimming whitespace / expanding ~ would make this more robust.
Proposed patch
@@
def resolve_authorization_headers(
authorization_headers: dict[str, str],
) -> dict[str, str]:
@@
resolved: dict[str, str] = {}
for header_name, value in authorization_headers.items():
+ raw_value = value
+ value = value.strip()
try:
- if value == "kubernetes":
+ if value == "kubernetes":
# Special case: Keep kubernetes keyword for later substitution
resolved[header_name] = "kubernetes"
@@
- elif value == "client":
+ elif value == "client":
# Special case: Keep client keyword for later substitution
resolved[header_name] = "client"
@@
else:
# Regular case: Read secret from file path
- secret_path = Path(value)
+ secret_path = Path(value).expanduser()
if secret_path.exists() and secret_path.is_file():
- with open(secret_path, "r", encoding="utf-8") as secret_file:
- secret_value = secret_file.read().strip()
- resolved[header_name] = secret_value
+ secret_value = secret_path.read_text(encoding="utf-8").strip()
+ if not secret_value:
+ logger.warning(
+ "Secret file %s for header %s is empty",
+ secret_path,
+ header_name,
+ )
+ else:
+ resolved[header_name] = secret_value
logger.debug(
"Resolved header %s from secret file %s",
header_name,
secret_path,
)
else:
logger.warning(
"Secret file not found or not a file: %s for header %s",
secret_path,
header_name,
)
except Exception as e: # pylint: disable=broad-except
- logger.error(
- "Failed to resolve authorization header %s with value %s: %s",
- header_name,
- value,
- e,
- )
+ # Don't log raw_value: it might be a literal token.
+ logger.exception(
+ "Failed to resolve authorization header %s (value treated as path)",
+ header_name,
+ )
return resolved🤖 Prompt for AI Agents
In @src/utils/mcp_auth_headers.py around lines 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.
2 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.