-
Notifications
You must be signed in to change notification settings - Fork 55
LCORE-321: MCP Server Documentation #549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LCORE-321: MCP Server Documentation #549
Conversation
WalkthroughAdds a new "Configuring MCP Servers" section to docs/getting_started.md describing MCP basics, LCS discovery via tools/list, YAML configuration (mcp_servers) with example servers/URLs, per-request metadata via the MCP-HEADERS HTTP header, and verification steps. No code or API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as AI Agent
participant L as LCS
participant M as MCP Server(s)
Note over U,A: User prompts agent to use external tools
U->>A: Request requiring tools
Note over L: LCS configured via lightspeed-stack.yaml (mcp_servers)
A->>L: Request tool-enabled action
alt Discovery needed
L->>M: tools/list
M-->>L: Tool metadata
end
Note over A,L: Per-request headers passed via MCP-HEADERS
L->>M: Tool call (with MCP-HEADERS)
M-->>L: Tool result
L-->>A: Aggregated response
A-->>U: Final answer
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/getting_started.md (5)
351-386: Fix heading levels (MD001) for Step 1–4.Headings jump from H2 to H4. Promote these to H3 for proper hierarchy.
Apply this diff:
-#### Step 1: Run your MCP servers +### Step 1: Run your MCP servers -#### Step 2: Configure Lightspeed-core to know about your MCP servers +### Step 2: Configure LCS to know about your MCP servers -#### Step 3: Pass authentication or metadata via MCP headers (optional) +### Step 3: Pass authentication or metadata via MCP headers (optional) -#### Step 4: Verify connectivity +### Step 4: Verify connectivity
354-371: Use consistent product naming.Replace “Lightspeed-core” with “Lightspeed Core Service (LCS)” for consistency with earlier sections.
Apply this diff:
-#### Step 2: Configure Lightspeed-core to know about your MCP servers +### Step 2: Configure LCS to know about your MCP servers @@ -**Important**: Only MCP servers defined in the `lightspeed-stack.yaml` configuration are available to the AI 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 AI agents. Tools configured in the Llama Stack `run.yaml` are not accessible to LCS agents.
378-383: Simplify curl quoting; avoid escaping JSON in header.Single‑quote the header to reduce errors.
Apply this diff:
-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"}}' \ + -d '{"query":"List files in /tmp"}'
385-386: Provide a concrete verification command.Add a quick smoke test so users see a successful tool call.
Apply this diff:
-After starting the MCP servers and updating `lightspeed-stack.yaml`, test by sending a prompt to the AI agent. LCS evaluates the prompt against available tools’ metadata, selects the appropriate tool, calls the corresponding MCP server, and uses the result to generate more accurate agent response. +After starting the MCP servers and updating `lightspeed-stack.yaml`, test by sending a prompt: + +```bash +curl -X POST 'http://localhost:8080/v1/query' \ + -H 'Content-Type: application/json' \ + -d '{"query":"Use filesystem-tools to list files in /tmp"}' +``` + +You should see a response that includes a tool invocation or content derived from the MCP server. Also check LCS logs for “Registering MCP servers” and successful tool routing.
358-369: Clarify thatprovider_idis optional (defaults to "model-context-protocol")ModelContextProtocolServer.provider_id has a default value "model-context-protocol" (src/models/config.py:165), so update the docs/example to either omit provider_id or explicitly state the default. Location: docs/getting_started.md (lines 358-369).
mcp_servers: - name: "filesystem-tools" - provider_id: "model-context-protocol" url: "http://localhost:3000" - name: "git-tools" - provider_id: "model-context-protocol" url: "http://localhost:3001" - name: "database-tools" - provider_id: "model-context-protocol" url: "http://localhost:3002"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/getting_started.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/getting_started.md
351-351: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e_tests
🔇 Additional comments (1)
docs/getting_started.md (1)
343-349: Good conceptual intro to MCP in LCS.Clear and accurate overview.
| #### Step 3: Pass authentication or metadata via MCP headers (optional) | ||
|
|
||
| Some MCP servers require authentication tokens, API keys, or other metadata. These can be passed **per request** using the `MCP-HEADERS` HTTP header. LCS will forward these headers when invoking the tool, allowing the MCP server to authenticate requests or receive additional context. | ||
| Example: | ||
|
|
||
| ```bash | ||
| 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"}' | ||
| ``` | ||
|
|
||
| #### Step 4: Verify connectivity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Warn about secrets in logs when using MCP-HEADERS.
With access_log: true (Line 258), reverse proxies/app servers may log request headers. Passing bearer tokens in MCP‑HEADERS can leak secrets.
Add a note after the curl example:
+> Note: If your deployment logs request headers (for example, `access_log: true` or upstream proxy defaults), MCP-HEADERS may expose credentials in logs. In production, disable access logging, ensure header redaction, or pass credentials via a secure store referenced by the MCP server.Optionally verify redaction in code:
🏁 Script executed:
#!/bin/bash
# Check for explicit redaction of MCP-HEADERS in request/response logging.
rg -n -C3 'MCP-HEADERS|access_log|Authorization' --type py --type ts --type goLength of output: 55265
Warn about secrets in logs when using MCP-HEADERS.
access_log is enabled by default (src/models/config.py) and the uvicorn runner sets access_log=True (src/runners/uvicorn.py); extract_mcp_headers reads MCP-HEADERS without redaction (src/utils/mcp_headers.py). Add the doc note below after the curl example and ensure header redaction or disable header logging in production.
+> Note: If your deployment logs request headers (for example, `access_log: true` or upstream proxy defaults), MCP-HEADERS may expose credentials in logs. In production, disable access logging, ensure header redaction, or pass credentials via a secure store referenced by the MCP server.Optional quick check:
#!/bin/bash
# Check for explicit redaction of MCP-HEADERS or Authorization in logging.
rg -n -C3 'MCP-HEADERS|access_log|Authorization|request.headers' --type py --type ts --type goFiles to review: src/models/config.py (access_log default), src/runners/uvicorn.py (access_log=True), src/utils/mcp_headers.py (extracts MCP-HEADERS), src/app/endpoints/query.py & src/app/endpoints/streaming_query.py (code injecting Authorization into MCP headers).
5c5a95c to
bc3c682
Compare
tisnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
LGTM! |
Description
Add step-by-step guide for configuring and using MCP servers
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit