feat: add MCP server support (Phase 1 — HTTP/SSE and basic stdio)#113
Merged
alexeykazakov merged 3 commits intocodeready-toolchain:masterfrom May 9, 2026
Merged
Conversation
- Add McpServerSpec type with CEL validation (command xor url) - Add spec.mcpServers map field to ClawSpec - Inject mcp.servers config into operator.json at reconciliation - Auto-extract HTTP MCP URL domains as proxy passthrough routes - Add McpServersConfigured status condition (set when non-empty) - Mark builtin domains in coveredDomains to prevent MCP duplicates Signed-off-by: Alexey Kazakov <alkazako@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📜 Recent review details⏰ 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). (2)
WalkthroughAdds MCP server support: new ChangesMCP Server Support Implementation
Sequence DiagramsequenceDiagram
actor User
participant Controller as Claw Controller
participant GatewayCM as Gateway ConfigMap
participant ProxyCM as Proxy ConfigMap
User->>Controller: Create/Update Claw with spec.mcpServers
activate Controller
Controller->>Controller: Detect non-empty McpServers\nSet McpServersConfigured condition
Controller->>GatewayCM: Read operator.json
activate GatewayCM
GatewayCM-->>Controller: Return operator.json
deactivate GatewayCM
Controller->>Controller: Unmarshal JSON\nBuild MCP server configs
Controller->>GatewayCM: Write updated operator.json with mcp.servers
activate GatewayCM
GatewayCM-->>Controller: ConfigMap updated
deactivate GatewayCM
Controller->>Controller: Generate proxy config including MCP servers\nExtract domains from HTTP MCP URLs
Controller->>ProxyCM: Write proxy routes (includes MCP passthrough routes)
activate ProxyCM
ProxyCM-->>Controller: Proxy ConfigMap updated
deactivate ProxyCM
Controller-->>User: Reconciliation complete
deactivate Controller
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/controller/claw_mcp_test.go`:
- Around line 85-109: The test and code currently persist plaintext MCP env
values into the operator.json ConfigMap (via
injectMcpServersIntoConfigMap/getMcpConfig), which leaks secrets; change
injectMcpServersIntoConfigMap to not write env map entries into the ConfigMap
but instead create or reference a Kubernetes Secret for each MCP server that has
env values and store only a secret reference (e.g., secret name and key or an
envRef field) in the ConfigMap entry for that server; update related helpers
(makeMcpConfigMap, testClawWithMcpServers, and the test expectations in this
test) so the ConfigMap no longer contains the plaintext env map and tests assert
a secretRef is present and env is not persisted in operator.json.
In `@internal/controller/claw_proxy.go`:
- Around line 235-256: mcpPassthroughRoutes currently treats coveredDomains as
exact matches only, allowing an MCP host like "api.example.com" to bypass a
credential entry for the suffix ".example.com"; update mcpPassthroughRoutes to
treat coveredDomains keys that begin with '.' as suffix rules by checking for
any coveredKey where strings.HasPrefix(coveredKey, ".") &&
strings.HasSuffix(domain, coveredKey[1:]) (or the reverse exact match), and skip
adding the proxyRoute if such a suffix match exists; keep adding exact domain
keys to coveredDomains as before and reference the mcpPassthroughRoutes,
coveredDomains and proxyRoute symbols when making this change.
In `@internal/controller/claw_resource_controller.go`:
- Around line 486-489: The code only sets the MCP condition to True when
instance.Spec.McpServers is present; update the full MCP condition lifecycle by
(1) ensuring any MCP injection failure paths call setCondition(instance,
clawv1alpha1.ConditionTypeMcpServersConfigured, metav1.ConditionFalse,
<appropriateReason>, <error message>) so failures are recorded, and (2) when
instance.Spec.McpServers is empty/removed, clear or mark the condition as
False/Removed (e.g., setCondition(..., metav1.ConditionFalse,
clawv1alpha1.ConditionReasonRemoved, "MCP servers removed") or remove the
condition) instead of leaving a stale True; reference the existing setCondition
function and ConditionTypeMcpServersConfigured/ConditionReasonConfigured symbols
to implement these status updates in the MCP injection and reconciliation
branches.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a7071ca1-7e1a-455b-8ae0-87af5f447514
📒 Files selected for processing (10)
api/v1alpha1/claw_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/claw.sandbox.redhat.com_claws.yamldocs/proposals/mcp-support-design.mdinternal/controller/claw_channels_test.gointernal/controller/claw_mcp.gointernal/controller/claw_mcp_test.gointernal/controller/claw_proxy.gointernal/controller/claw_proxy_test.gointernal/controller/claw_resource_controller.go
📜 Review details
⏰ 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). (2)
- GitHub Check: E2E Tests
- GitHub Check: Unit Tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
internal/controller/claw_mcp.goapi/v1alpha1/zz_generated.deepcopy.gointernal/controller/claw_proxy.goconfig/crd/bases/claw.sandbox.redhat.com_claws.yamldocs/proposals/mcp-support-design.mdinternal/controller/claw_resource_controller.gointernal/controller/claw_mcp_test.gointernal/controller/claw_channels_test.goapi/v1alpha1/claw_types.gointernal/controller/claw_proxy_test.go
🔇 Additional comments (5)
internal/controller/claw_channels_test.go (1)
464-517: Signature update in tests looks correct.Passing
nilfor the new MCP argument keeps these channel-companion tests behaviorally equivalent while matching the updated API.api/v1alpha1/zz_generated.deepcopy.go (1)
209-215: Deep-copy coverage for MCP fields looks correct.
McpServers,Args, andEnvare copied into fresh allocations, preventing shared mutable state across object copies.Also applies to: 322-347
api/v1alpha1/claw_types.go (1)
218-244: MCP API shape and validation are well-scoped for Phase 1.The
command/urlXOR rules and map-basedmcpServersaddition are clean and consistent with the controller integration.Also applies to: 260-263
internal/controller/claw_proxy_test.go (1)
1288-1420: Strong MCP domain-extraction regression coverage.This test block meaningfully protects the new behavior (extraction, dedup against credentials/builtins, invalid URL handling, stdio exclusion, and case normalization).
internal/controller/claw_mcp_test.go (1)
393-466: CEL xor validation coverage is solid.The negative/positive matrix for
commandvsurlis clear and directly protects the CRD contract.
- Add domainCovered() to check suffix-match credential domains - Prevents MCP passthrough routes from shadowing auth injection when a suffix credential (e.g. ".googleapis.com") covers the domain Signed-off-by: Alexey Kazakov <alkazako@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Remove McpServersConfigured condition when spec.mcpServers is empty/removed instead of leaving a stale True - Add integration test verifying condition lifecycle across add-then-remove reconciliation cycles Signed-off-by: Alexey Kazakov <alkazako@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
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
1 participant
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.
Summary by CodeRabbit
New Features
Tests
Documentation