[LCORE-353] Allow mcp headers to contain mcp servers names #221
Conversation
|
""" WalkthroughA new function, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint (Query/Streaming)
participant MCP Header Handler
participant AppConfig
Client->>Endpoint (Query/Streaming): Send request with mcp_headers
Endpoint (Query/Streaming)->>MCP Header Handler: handle_mcp_headers_with_toolgroups(mcp_headers, AppConfig)
MCP Header Handler->>AppConfig: Lookup server URLs for toolgroup names
MCP Header Handler-->>Endpoint (Query/Streaming): Return processed mcp_headers (URLs as keys)
Endpoint (Query/Streaming)-->>Client: Continue processing with updated mcp_headers
Poem
""" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/utils/mcp_headers.py(2 hunks)tests/unit/app/endpoints/test_query.py(2 hunks)tests/unit/app/endpoints/test_streaming_query.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/app/endpoints/streaming_query.py (1)
src/utils/mcp_headers.py (1)
handle_mcp_headers_with_toolgroups(55-74)
src/app/endpoints/query.py (1)
src/utils/mcp_headers.py (1)
handle_mcp_headers_with_toolgroups(55-74)
src/utils/mcp_headers.py (1)
src/configuration.py (3)
configuration(47-52)AppConfig(20-100)mcp_servers(79-84)
🪛 GitHub Actions: Pydocstyle
src/utils/mcp_headers.py
[error] 55-55: pydocstyle: Missing docstring in public function handle_mcp_headers_with_toolgroups (D103)
🪛 GitHub Actions: Python linter
src/utils/mcp_headers.py
[warning] 55-55: pylint: Missing function or method docstring (missing-function-docstring)
[warning] 9-9: pylint: third party import "fastapi.Request" should be placed before first party import "configuration.AppConfig" (wrong-import-order)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-pr
🔇 Additional comments (8)
src/app/endpoints/query.py (2)
32-32: LGTM: Clean import addition.The import is properly added and aligns with the new MCP headers processing functionality.
234-234: LGTM: Proper integration of header processing.The function is correctly placed in the flow - after ensuring
mcp_headersis not None and before using the headers for authorization setup. This ensures that toolgroup names are properly converted to URLs before being used.src/app/endpoints/streaming_query.py (2)
25-25: LGTM: Consistent import pattern.The import matches the pattern used in
query.py, maintaining consistency across both endpoint files.
294-294: LGTM: Consistent header processing implementation.The function integration is identical to the synchronous query endpoint, ensuring consistent behavior across both streaming and non-streaming queries. The placement in the function flow is appropriate.
tests/unit/app/endpoints/test_query.py (2)
696-705: LGTM: Comprehensive test input covering all scenarios.The test input properly covers all the scenarios that the new function should handle:
- Server names that map to known MCP servers
- Valid URLs that should pass through unchanged
- Unknown server names that should be filtered out
This provides good coverage for the new functionality.
727-734: LGTM: Correct expected output validation.The expected headers correctly demonstrate the mapping behavior:
- Server names converted to their corresponding URLs
- Direct URLs passed through unchanged
- Unknown servers filtered out as documented in the comment
This properly validates the
handle_mcp_headers_with_toolgroupsfunction behavior.tests/unit/app/endpoints/test_streaming_query.py (2)
764-773: LGTM: Consistent test input with synchronous query tests.The test input is identical to the synchronous query test, ensuring consistent validation across both streaming and non-streaming endpoints. The comprehensive scenario coverage is maintained.
795-802: LGTM: Consistent expected output validation.The expected output validation matches the synchronous query test, ensuring both endpoints handle MCP headers identically. The filtering and mapping behavior is properly verified.
https://issues.redhat.com/browse/LCORE-353 this PR allow the client to provide mcp headers with mcp servers names mcp_headers = { "mcp-server-url-1": {"HEADER-1": "HEADER-1-VALUE-1", "HEADER-1": "HEADER-1-VALUE-2"}, "mcp-server-name": {"HEADER-2": "HEADER-2-VALUE"} } the header with "mcp-server-name" will be replaced with it corresponding url using the configuration.mcp_servers, if it does exit it's getting ignored. Signed-off-by: Djebran Lezzoum <ldjebran@gmail.com>
ldjebran
force-pushed
the
allow-mcp-headers-with-mcp-servers-name
branch
from
df64e8c to
8dd0435
Compare
3 participants
Description
In some configurations it's very hard for the application to be configured using the urls of mcp servers as the urls may be generated dynamically, this implementation simplify a lot the application deployments, as mcp servers are defined in earlier stages.
This PR allow the client to provide mcp headers with mcp servers names mcp_headers = {
"mcp-server-url-1": {"HEADER-1": "HEADER-1-VALUE-1", "HEADER-1": "HEADER-1-VALUE-2"},
"mcp-server-name": {"HEADER-2": "HEADER-2-VALUE"}
}
the header with "mcp-server-name" will be replaced with it corresponding url using the configuration.mcp_servers, if it does exit it's getting ignored.
for url validation we use urllib urlpase the same used by llama-stack
Type of change
Related Tickets & Documents
Checklist before requesting a review
Summary by CodeRabbit
New Features
Bug Fixes
Tests