OLS-3185: spec changes for sandbox MCP server support#94
Conversation
Add behavioral rules for parsing LIGHTSPEED_MCP_SERVERS env var, resolving MCP headers (ServiceAccountToken, Secret), and wiring MCP servers into all three provider SDKs via Streamable HTTP transport. Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@onmete: This pull request references OLS-3185 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature request to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThree specification documents are updated to describe MCP (Model Context Protocol) server integration: a new ChangesMCP Specification Documentation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ai/spec/what/configuration.md:
- Around line 79-87: The MCP header schema and resolution rules for
LIGHTSPEED_MCP_SERVERS allow Secret headers without a secretName, but the
sandbox cannot resolve them reliably. Update the configuration spec around the
MCP server/header definitions so secretName is mandatory whenever header.source
is Secret, and adjust the MCP header resolution guidance to use a deterministic
secret file/key path instead of “first file” in the Secret case. Reference the
MCP server config and header resolution sections so the schema and behavior stay
aligned.
In @.ai/spec/what/provider-contract.md:
- Line 61: The Claude MCP handling in the provider contract needs to avoid
unsafe wildcard expansion for `allowed_tools`. In the MCP server processing for
`ClaudeAgentOptions(mcp_servers={...})`, make sure each server name used in the
`mcp__<name>__*` pattern is validated or escaped so glob metacharacters cannot
over-match; apply this where the adapter builds `allowed_tools` for configured
MCP servers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 57937998-f688-432c-b725-c88230b1f0eb
📒 Files selected for processing (3)
.ai/spec/how/provider-architecture.md.ai/spec/what/configuration.md.ai/spec/what/provider-contract.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-operator(manual)
| 20. **MCP server configuration.** When `LIGHTSPEED_MCP_SERVERS` is set, the sandbox MUST parse it as a JSON array of MCP server entries. Each entry has the shape `{"name": string, "url": string, "timeout": int, "headers": [{"name": string, "source": string, "secretName"?: string}]}`. The sandbox MUST build SDK-native MCP client configs from this array and pass them into provider adapters via `ProviderQueryOptions.mcp_servers` (see `provider-contract.md`). When the env var is absent or empty, no MCP servers are configured. | ||
|
|
||
| 21. **MCP header resolution.** For each header in an MCP server entry, the sandbox MUST resolve the value based on the `source` field: | ||
|
|
||
| | `source` | Resolution | | ||
| |---|---| | ||
| | `ServiceAccountToken` | Read the projected SA token from `/var/run/secrets/kubernetes.io/serviceaccount/token` and format as `Bearer <token>`. | | ||
| | `Secret` | Read the first file from `/var/secrets/mcp/<secretName>/` where `secretName` is the `secretName` field on the header entry. | | ||
| | `Client` | Skip — not resolved by the sandbox. Reserved for future client-passthrough flows. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make secretName required for Secret headers.
The schema currently allows secretName to be omitted, but Secret resolution depends on it. Also, resolving the "first file" from a mount is nondeterministic if the secret has multiple keys. Require the field for source=Secret and pin a deterministic file/key path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ai/spec/what/configuration.md around lines 79 - 87, The MCP header schema
and resolution rules for LIGHTSPEED_MCP_SERVERS allow Secret headers without a
secretName, but the sandbox cannot resolve them reliably. Update the
configuration spec around the MCP server/header definitions so secretName is
mandatory whenever header.source is Secret, and adjust the MCP header resolution
guidance to use a deterministic secret file/key path instead of “first file” in
the Secret case. Reference the MCP server config and header resolution sections
so the schema and behavior stay aligned.
|
|
||
| 26. **OpenAI client.** The OpenAI adapter constructs an async OpenAI client with optional base URL override from environment (see `configuration.md`). | ||
|
|
||
| 27. **MCP — Claude.** When `mcp_servers` is non-empty, the Claude adapter MUST pass them as `ClaudeAgentOptions(mcp_servers={...})` using `"type": "http"` (Streamable HTTP) entries with `url` and `headers` from the resolved config. The adapter MUST add `mcp__<name>__*` wildcard patterns to `allowed_tools` for each configured MCP server so the SDK can invoke discovered tools. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape MCP server names before expanding allowed_tools.
mcp__<name>__* is a glob pattern. If name contains wildcard chars, the pattern can over-match and expose tools from the wrong server. Constrain the name format or escape metacharacters before interpolation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ai/spec/what/provider-contract.md at line 61, The Claude MCP handling in
the provider contract needs to avoid unsafe wildcard expansion for
`allowed_tools`. In the MCP server processing for
`ClaudeAgentOptions(mcp_servers={...})`, make sure each server name used in the
`mcp__<name>__*` pattern is validated or escaped so glob metacharacters cannot
over-match; apply this where the adapter builds `allowed_tools` for configured
MCP servers.
|
@onmete: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
2 participants
Summary
LIGHTSPEED_MCP_SERVERSenv var to the operator contract inconfiguration.mdmcp_serversfield toProviderQueryOptionsinprovider-contract.mdmcp.pymodule to architecture module mapDesign decisions
From OLS-3185 design interview:
Test plan
Made with Cursor