OLS-3185: mcp servers wiring#82
Draft
tremes wants to merge 2 commits into
Draft
Conversation
Add external MCP server integration via streamable HTTP transport.
The operator configures endpoints through LIGHTSPEED_MCP_SERVERS env
var (JSON array of {name, url, headers?} objects).
Configuration:
- McpServerConfig dataclass and resolve_mcp_servers() in config.py
- mcp_servers field on ResolvedSDK and ProviderQueryOptions
- Flows from app startup through routing to provider query
Provider wiring:
- Claude: mcp_servers dict on ClaudeAgentOptions with mcp__*
allowed_tools patterns
- Gemini: McpToolset with StreamableHTTPConnectionParams in tools
- OpenAI: MCPServerStreamableHttp with connect/cleanup lifecycle
Health probes:
- R3 check_mcp_endpoints() probes each MCP server URL
- Readiness route returns 503 when any MCP endpoint unreachable
Signed-off-by: Tomáš Remeš <tremes@redhat.com>
Assisted-by: Claude Code:claude-opus-4-6
…n source
The operator injects LIGHTSPEED_MCP_SERVERS headers as an array of
{name, source/value} objects, but resolve_mcp_servers() only accepted
a dict. This caused a ValueError at startup, crash-looping the pod.
Accept both formats:
- dict: {"header-name": "static-value"} (existing)
- list: [{"name": "h", "source": "ServiceAccountToken"}] (operator)
ServiceAccountToken source reads from the mounted SA token file.
Co-Authored-By: Claude <noreply@anthropic.com>
|
@tremes: 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds end-to-end MCP (Model Context Protocol) server support via a new ChangesMCP Server Support
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
[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 |
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_config.py (1)
292-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for missing ServiceAccountToken file.
This suite validates unknown header sources, but not the explicit missing-token-file error path for
source: "ServiceAccountToken". Add one test that patches_SA_TOKEN_PATHto a non-existent file and asserts the raisedValueError.Proposed test addition
+def test_mcp_servers_headers_service_account_token_missing_file( + monkeypatch: pytest.MonkeyPatch, +) -> None: + _clean_env(monkeypatch) + import lightspeed_agentic.config as cfg + + monkeypatch.setattr(cfg, "_SA_TOKEN_PATH", "/tmp/does-not-exist") + monkeypatch.setenv( + "LIGHTSPEED_MCP_SERVERS", + json.dumps( + [ + { + "name": "k8s", + "url": "http://mcp.local/mcp", + "headers": [ + {"name": "kubernetes-authorization", "source": "ServiceAccountToken"} + ], + } + ] + ), + ) + with pytest.raises(ValueError, match="requires"): + resolve_mcp_servers()🤖 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 `@tests/test_config.py` around lines 292 - 319, Add a new test function to cover the error case when the ServiceAccountToken file does not exist. Create a test similar to test_mcp_servers_headers_list_with_source that sets up an MCP server configuration with a ServiceAccountToken header source, but patches _SA_TOKEN_PATH to point to a non-existent file path. Use pytest.raises to assert that calling resolve_mcp_servers() raises a ValueError when the token file cannot be found, ensuring the error handling for missing token files is properly tested.
🤖 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:
- Line 20: The `headers` field in the LIGHTSPEED_MCP_SERVERS configuration
documentation only describes the object format but the implementation supports
both object and list formats. Update the documentation for the `headers`
parameter at line 20 to include both supported formats: the object format
(existing documentation) and the list format with entries containing "name" and
either "value" or "source" fields. Also apply the same fix to the additional
occurrences of this documentation at lines 70 and 87 to ensure consistency
across the specification.
In `@src/lightspeed_agentic/config.py`:
- Around line 56-57: The dict path (returning raw) and the list-form path
(coercing with str()) validate headers inconsistently, allowing non-string
header names or values to pass through despite the type hint headers: dict[str,
str]. Add validation in both paths to ensure all header names and values are
strings before returning, rejecting non-string values at startup. Apply the same
validation logic to both the dict return statement around line 56-57 and the
list-form value coercion around line 70-73 to maintain consistency.
- Around line 92-103: Add validation to detect and reject duplicate MCP server
names during parsing. Initialize a set to track seen server names before the
loop that processes entries, then before appending each McpServerConfig to the
servers list, check if the current name already exists in the seen names set. If
a duplicate is found, raise a ValueError with a message indicating that
duplicate server names are not allowed. Add the name to the seen names set after
all validations pass but before appending the server configuration.
In `@src/lightspeed_agentic/providers/openai.py`:
- Around line 241-244: The MCP server connection loop (iterating over
mcp_server_instances and calling srv.connect()) is positioned before the
try/finally block, which means if a connection fails partway through, the
cleanup code in the finally block will never execute and leave open sessions
uncleaned. Move the entire for loop that connects to mcp_server_instances to be
inside the try block, after the try statement but before any other code, so that
all connection attempts are protected by the cleanup guard in the corresponding
finally block. This same pattern issue also appears at lines 289-291, so apply
the same fix there as well.
---
Nitpick comments:
In `@tests/test_config.py`:
- Around line 292-319: Add a new test function to cover the error case when the
ServiceAccountToken file does not exist. Create a test similar to
test_mcp_servers_headers_list_with_source that sets up an MCP server
configuration with a ServiceAccountToken header source, but patches
_SA_TOKEN_PATH to point to a non-existent file path. Use pytest.raises to assert
that calling resolve_mcp_servers() raises a ValueError when the token file
cannot be found, ensuring the error handling for missing token files is properly
tested.
🪄 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: 07a98632-5383-434e-b483-e7ad7d161880
📒 Files selected for processing (15)
.ai/spec/what/configuration.md.ai/spec/what/health-probes.md.ai/spec/what/provider-contract.mdAGENTS.mdsrc/lightspeed_agentic/app.pysrc/lightspeed_agentic/config.pysrc/lightspeed_agentic/health.pysrc/lightspeed_agentic/providers/claude.pysrc/lightspeed_agentic/providers/gemini.pysrc/lightspeed_agentic/providers/openai.pysrc/lightspeed_agentic/routes/__init__.pysrc/lightspeed_agentic/routes/query.pysrc/lightspeed_agentic/types.pytests/test_config.pytests/test_ready.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-operator(manual)
| | `LIGHTSPEED_PROVIDER_PROJECT` | When provider=`vertex` | Cloud project ID | | ||
| | `LIGHTSPEED_PROVIDER_REGION` | When provider=`vertex` or `bedrock` | Cloud region | | ||
| | `LIGHTSPEED_PROVIDER_API_VERSION` | When provider=`azure` | API version | | ||
| | `LIGHTSPEED_MCP_SERVERS` | No | JSON array of MCP server configs: `[{"name":"…","url":"…","headers":{}}]` | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fix MCP headers contract to include list-format entries.
The spec currently documents headers as object-only, but the implementation and tests support both object and list formats ([{ "name": "...", "value" | "source": ... }]). This mismatch can break operator-side integration expectations.
Suggested spec wording update
-| `LIGHTSPEED_MCP_SERVERS` | No | JSON array of MCP server configs: `[{"name":"…","url":"…","headers":{}}]` |
+| `LIGHTSPEED_MCP_SERVERS` | No | JSON array of MCP server configs: `[{"name":"…","url":"…","headers":{}}]` or `[{"name":"…","url":"…","headers":[{"name":"h","value":"v"}]}]` |
-19. **MCP server configuration.** ... Each entry MUST have `name` (string) and `url` (string); `headers` (object) is optional.
+19. **MCP server configuration.** ... Each entry MUST have `name` (string) and `url` (string); `headers` is optional and may be an object (`{"h":"v"}`) or an array of objects (`[{"name":"h","value":"v"}]` or `[{"name":"h","source":"ServiceAccountToken"}]`).
-| `LIGHTSPEED_MCP_SERVERS` | Optional JSON array of MCP server endpoint configs (see rule 19). |
+| `LIGHTSPEED_MCP_SERVERS` | Optional JSON array of MCP endpoint configs; `headers` supports object and list-entry formats (see rule 19). |Also applies to: 70-70, 87-87
🤖 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 at line 20, The `headers` field in the
LIGHTSPEED_MCP_SERVERS configuration documentation only describes the object
format but the implementation supports both object and list formats. Update the
documentation for the `headers` parameter at line 20 to include both supported
formats: the object format (existing documentation) and the list format with
entries containing "name" and either "value" or "source" fields. Also apply the
same fix to the additional occurrences of this documentation at lines 70 and 87
to ensure consistency across the specification.
Comment on lines
+56
to
+57
| if isinstance(raw, dict): | ||
| return raw |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate header names and values consistently.
The dict path returns raw unchanged, while list-form value is coerced with str(). Non-string header names/values can reach provider HTTP clients despite headers: dict[str, str]; reject them at startup in both formats.
Proposed validation
if isinstance(raw, dict):
- return raw
+ resolved: dict[str, str] = {}
+ for hdr_name, hdr_value in raw.items():
+ if not hdr_name or not isinstance(hdr_name, str):
+ raise ValueError(
+ f"LIGHTSPEED_MCP_SERVERS[{index}].headers has invalid header name"
+ )
+ if not isinstance(hdr_value, str):
+ raise ValueError(
+ f"LIGHTSPEED_MCP_SERVERS[{index}].headers[{hdr_name!r}] must be a string"
+ )
+ resolved[hdr_name] = hdr_value
+ return resolved
...
if "value" in item:
- resolved[hdr_name] = str(item["value"])
+ value = item["value"]
+ if not isinstance(value, str):
+ raise ValueError(
+ f"LIGHTSPEED_MCP_SERVERS[{index}].headers[{j}].value must be a string"
+ )
+ resolved[hdr_name] = valueAs per path instructions, “Validate at trust boundaries with allow-lists, not deny-lists.”
Also applies to: 70-73
🤖 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 `@src/lightspeed_agentic/config.py` around lines 56 - 57, The dict path
(returning raw) and the list-form path (coercing with str()) validate headers
inconsistently, allowing non-string header names or values to pass through
despite the type hint headers: dict[str, str]. Add validation in both paths to
ensure all header names and values are strings before returning, rejecting
non-string values at startup. Apply the same validation logic to both the dict
return statement around line 56-57 and the list-form value coercion around line
70-73 to maintain consistency.
Source: Path instructions
Comment on lines
+92
to
+103
| servers: list[McpServerConfig] = [] | ||
| for i, entry in enumerate(entries): | ||
| if not isinstance(entry, dict): | ||
| raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] must be an object") | ||
| name = entry.get("name") | ||
| url = entry.get("url") | ||
| if not name or not isinstance(name, str): | ||
| raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] missing required 'name' string") | ||
| if not url or not isinstance(url, str): | ||
| raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] missing required 'url' string") | ||
| headers = _resolve_headers(entry.get("headers", {}), i) | ||
| servers.append(McpServerConfig(name=name, url=url, headers=headers)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject duplicate MCP server names.
Duplicate name values are accepted here, but downstream consumers key by name: Claude’s dict keeps only one server and health results overwrite mcp_{name}. Fail fast during parsing.
Proposed duplicate check
servers: list[McpServerConfig] = []
+ seen_names: set[str] = set()
for i, entry in enumerate(entries):
if not isinstance(entry, dict):
raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] must be an object")
name = entry.get("name")
url = entry.get("url")
@@
if not url or not isinstance(url, str):
raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] missing required 'url' string")
+ if name in seen_names:
+ raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] duplicate server name {name!r}")
+ seen_names.add(name)
headers = _resolve_headers(entry.get("headers", {}), i)
servers.append(McpServerConfig(name=name, url=url, headers=headers))📝 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.
Suggested change
| servers: list[McpServerConfig] = [] | |
| for i, entry in enumerate(entries): | |
| if not isinstance(entry, dict): | |
| raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] must be an object") | |
| name = entry.get("name") | |
| url = entry.get("url") | |
| if not name or not isinstance(name, str): | |
| raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] missing required 'name' string") | |
| if not url or not isinstance(url, str): | |
| raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] missing required 'url' string") | |
| headers = _resolve_headers(entry.get("headers", {}), i) | |
| servers.append(McpServerConfig(name=name, url=url, headers=headers)) | |
| servers: list[McpServerConfig] = [] | |
| seen_names: set[str] = set() | |
| for i, entry in enumerate(entries): | |
| if not isinstance(entry, dict): | |
| raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] must be an object") | |
| name = entry.get("name") | |
| url = entry.get("url") | |
| if not name or not isinstance(name, str): | |
| raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] missing required 'name' string") | |
| if not url or not isinstance(url, str): | |
| raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] missing required 'url' string") | |
| if name in seen_names: | |
| raise ValueError(f"LIGHTSPEED_MCP_SERVERS[{i}] duplicate server name {name!r}") | |
| seen_names.add(name) | |
| headers = _resolve_headers(entry.get("headers", {}), i) | |
| servers.append(McpServerConfig(name=name, url=url, headers=headers)) |
🤖 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 `@src/lightspeed_agentic/config.py` around lines 92 - 103, Add validation to
detect and reject duplicate MCP server names during parsing. Initialize a set to
track seen server names before the loop that processes entries, then before
appending each McpServerConfig to the servers list, check if the current name
already exists in the seen names set. If a duplicate is found, raise a
ValueError with a message indicating that duplicate server names are not
allowed. Add the name to the seen names set after all validations pass but
before appending the server configuration.
Comment on lines
+241
to
+244
| for srv in mcp_server_instances: | ||
| await srv.connect() | ||
|
|
||
| try: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Put MCP connects inside the cleanup guard.
If one server connects and a later srv.connect() raises, execution never enters the try/finally, so already-open MCP sessions are not cleaned up.
Proposed lifecycle guard
- for srv in mcp_server_instances:
- await srv.connect()
-
+ connected_mcp_servers: list[Any] = []
try:
+ for srv in mcp_server_instances:
+ await srv.connect()
+ connected_mcp_servers.append(srv)
+
run_config = RunConfig(
sandbox=SandboxRunConfig(
client=UnixLocalSandboxClient(),
@@
)
finally:
- for srv in mcp_server_instances:
+ for srv in connected_mcp_servers:
await srv.cleanup()Also applies to: 289-291
🤖 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 `@src/lightspeed_agentic/providers/openai.py` around lines 241 - 244, The MCP
server connection loop (iterating over mcp_server_instances and calling
srv.connect()) is positioned before the try/finally block, which means if a
connection fails partway through, the cleanup code in the finally block will
never execute and leave open sessions uncleaned. Move the entire for loop that
connects to mcp_server_instances to be inside the try block, after the try
statement but before any other code, so that all connection attempts are
protected by the cleanup guard in the corresponding finally block. This same
pattern issue also appears at lines 289-291, so apply the same fix there as
well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.