LCORE-226: How to register MCP servers with LCORE #390
Conversation
WalkthroughREADME.md updated to document MCP server configuration via lightspeed-stack.yaml, per-request MCP-HEADERS usage, and reinstates a note about run.yaml being an implementation detail with a TOML project sample. Minor formatting adjustment adds a blank line before an image. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
tisnik
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (4)
118-137: Good addition: clear, actionable MCP server configuration sampleThe new section is helpful and concrete. Two follow-ups to tighten it:
- Ensure the
mcp_serversschema is documented in your configuration reference (keys, required/optional, validation).- Add a link to Model Context Protocol docs for readers unfamiliar with MCP.
Also, since this is a new third-level section under “Llama Stack as separate server,” consider regenerating the vim-markdown TOC so this header (and “Configuring MCP Headers”) appear in the TOC if that’s your convention.
Would you like me to add a short “Configuration reference” block enumerating
mcp_servers[*].{name,provider_id,url}and constraints?
139-139: Wording/consistency nit: prefer “inaccessible”; capitalize consistentlyMinor phrasing and capitalization polish.
Apply this diff:
-**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.
141-150: Clarify header format and security considerations for MCP-HEADERS
- Show how to set headers for multiple MCP servers and include multiple headers per server.
- Add a brief caution that request logs and reverse proxies may capture this header; recommend redaction.
Apply this diff to expand the example:
-curl -X POST "http://localhost:8080/v1/query" \ - -H "Content-Type: application/json" \ - -H "MCP-HEADERS: {\"filesystem-tools\": {\"Authorization\": \"Bearer token123\"}}" \ - -d '{"query": "List files in /tmp"}' +curl -X POST "http://localhost:8080/v1/query" \ + -H "Content-Type: application/json" \ + -H $'MCP-HEADERS: {"filesystem-tools":{"Authorization":"Bearer token123"},"git-tools":{"X-API-Key":"abc123","X-Org":"eng"}}' \ + -d '{"query":"List files in /tmp"}'Optionally add after the example:
- “Note: Ensure intermediaries (proxies, access logs) do not log sensitive MCP-HEADERS. Consider server-side redaction.”
155-155: Avoid repeating the same “run.yaml is an implementation detail” noteThis note appears earlier (Line 120). Consider keeping it once and cross-referencing here to reduce duplication.
Apply this diff:
-**Note**: The `run.yaml` configuration is currently an implementation detail. In the future, all configuration will be available directly from the lightspeed-core config. +See note above: `run.yaml` is currently an implementation detail; future configuration will be driven from lightspeed-core.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~139-~139: 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)
🔇 Additional comments (1)
README.md (1)
90-90: LGTM: spacing before image improves readabilityThe extra blank line before the image renders cleanly in most Markdown renderers.
| ```yaml | ||
| mcp_servers: | ||
| - name: "filesystem-tools" | ||
| provider_id: "model-context-protocol" |
There was a problem hiding this comment.
Is the provider_id: "model-context-protocol" necessary? As it is llama-stack "thing", can it be added automatically?
There was a problem hiding this comment.
But I wonder if you can have two MCPs with the same name and different providers ? If this is supported by llama-stack I fear we will need to expose it as well ?
There was a problem hiding this comment.
This is a bit complicated because the provider_id needs to match an id defined in tool_runtime. By default we just have the following defined in the run.yml:
tool_runtime:
- provider_id: model-context-protocol
provider_type: remote::model-context-protocol
config: {}The providers supported today are here
But name is toolgroup_id today which must be unique no matter the provider id (see: https://github.com/lightspeed-core/lightspeed-stack/blob/main/src/utils/common.py#L56 ).
If we want to allow for same name but different provider we can generate a toolgroup_id or something but that sounds messy.
There was a problem hiding this comment.
There is another MCP provider in lightspeed-providers. This is why we asked for the provider to be a parameter too.
| ```bash | ||
| curl -X POST "http://localhost:8080/v1/query" \ | ||
| -H "Content-Type: application/json" \ | ||
| -H "MCP-HEADERS: {\"filesystem-tools\": {\"Authorization\": \"Bearer token123\"}}" \ |
There was a problem hiding this comment.
Just as in HTTP/1.x, header field names are strings of ASCII characters that are compared in a case-insensitive fashion. However, header field names MUST be converted to lowercase prior to their encoding in HTTP/2. A request or response containing uppercase header field names MUST be treated as malformed -- RFC 7540, Hypertext Transfer Protocol Version 2 (HTTP/2), Section 8.1.2
Should MCP-HEADERS be uppercase?
There was a problem hiding this comment.
@ldjebran I believe we use upper case from Ansible Lightspeed (as, IIRC, the header was contributed by us). Changing case could break us? Unless other code in lightspeed-core looks for the header in a case insensitive way?
There was a problem hiding this comment.
@manstis we are already using uppercase when sending the header just as the curl command above, do not think this should affect anyone as soon as they can be converted to lowercase or looking in a case insensitive way.
forcing clients to have headers in lowercase will break us for sure.
There was a problem hiding this comment.
curl actually send Mcp-headers in request. And as HTTP/1.1 standard says, header names are case insensitive, so don't worry much ;)
| ```yaml | ||
| mcp_servers: | ||
| - name: "filesystem-tools" | ||
| provider_id: "model-context-protocol" |
There was a problem hiding this comment.
But I wonder if you can have two MCPs with the same name and different providers ? If this is supported by llama-stack I fear we will need to expose it as well ?
6 participants
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit