feat: Support env.valueFrom in MCP server config

[kelos-bot]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Changes AgentConfig.spec.mcpServers[].env from a map[string]string to []corev1.EnvVar so each entry can pull its value from a single key of a Secret or ConfigMap via the standard valueFrom shape (secretKeyRef, configMapKeyRef). The existing envFrom (whole-Secret) field is preserved and still wins over env on name collision.

Only SecretKeyRef and ConfigMapKeyRef variants of valueFrom are honored — fieldRef and resourceFieldRef are pod-scoped and meaningless for an MCP server process, so they are rejected with an error.

Because the env field is now a list rather than a map, this is a breaking change for any AgentConfig that sets mcpServers[].env. The CLI --mcp flag still accepts the {"KEY":"VAL"} shorthand alongside the new [{name,value/valueFrom}] form.

Includes RBAC permission to read ConfigMaps.

Which issue(s) this PR is related to:

Fixes #1152

Special notes for your reviewer:

The shape choice mirrors PodOverrides.Env in api/v1alpha1/task_types.go:78 so MCP env now uses the same idiom users already see elsewhere in the API. Migration for existing YAML:

# Before
env:
  DSN: postgres://localhost/db

# After
env:
- name: DSN
  value: postgres://localhost/db

Does this PR introduce a user-facing change?

AgentConfig: `mcpServers[].env` is now a list of `EnvVar` (matching `PodOverrides.env`) and supports `valueFrom.secretKeyRef` and `valueFrom.configMapKeyRef` for pulling a single key out of a Secret or ConfigMap. The previous `map[string]string` shape is no longer accepted.

---

Summary by cubic

Adds valueFrom support for MCP server env by changing AgentConfig.spec.mcpServers[].env to a list of corev1.EnvVar, enabling per-key Secret/ConfigMap references. envFrom still wins on name collision, the CLI keeps the {"KEY":"VAL"} shorthand, and RBAC/CRDs were updated.

• New Features

  • mcpServers[].env is now []corev1.EnvVar with valueFrom.secretKeyRef and valueFrom.configMapKeyRef.
  • Controller resolves valueFrom to literals; fieldRef/resourceFieldRef are rejected.
  • envFrom continues to override env on key collision.
  • Added permission to read ConfigMaps; CRD/manifests updated.

• Migration

  • Update YAML from map to list. Example: env: {DSN: "postgres://..."} → env: [{name: "DSN", value: "postgres://..."}]. The CLI --mcp flag still accepts the old map shorthand.

^{Written for commit 7f0e8c5. Summary will update on new commits. Review in cubic}

[greptile-apps]

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

[cubic-dev-ai]

1 issue found across 12 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/controller/task_controller.go">

<violation number="1" location="internal/controller/task_controller.go:531">
P2: `valueFrom` is not validated as exclusive, so unsupported `fieldRef/resourceFieldRef` can slip through when combined with `secretKeyRef` or `configMapKeyRef`.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: valueFrom is not validated as exclusive, so unsupported fieldRef/resourceFieldRef can slip through when combined with secretKeyRef or configMapKeyRef.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/task_controller.go, line 531:

<comment>`valueFrom` is not validated as exclusive, so unsupported `fieldRef/resourceFieldRef` can slip through when combined with `secretKeyRef` or `configMapKeyRef`.</comment>

<file context>
@@ -451,27 +452,130 @@ func (r *TaskReconciler) resolveMCPServerSecrets(ctx context.Context, namespace
+	}
+
+	src := entry.ValueFrom
+	switch {
+	case src.SecretKeyRef != nil:
+		ref := src.SecretKeyRef
</file context>

Suggested change

	switch {
	setSources := 0
	if src.SecretKeyRef != nil {
	setSources++
	}
	if src.ConfigMapKeyRef != nil {
	setSources++
	}
	if src.FieldRef != nil || src.ResourceFieldRef != nil {
	return "", fmt.Errorf("MCP server %q env %q: fieldRef and resourceFieldRef are not supported", serverName, entry.Name)
	}
	if setSources != 1 {
	return "", fmt.Errorf("MCP server %q env %q: valueFrom must set exactly one of secretKeyRef or configMapKeyRef", serverName, entry.Name)
	}
	switch {

[gjkim42]

This is a breaking change.
What's the best way to handle it?
/kelos api-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-api-reviewer-issue-comment-8dcef41a59c6 has succeeded. ✅

[kelos-bot]

🤖 Kelos API Reviewer Agent @gjkim42

API Design Review

Verdict: REQUEST CHANGES
Scope: Change AgentConfig.spec.mcpServers[].env from map[string]string to []corev1.EnvVar and resolve valueFrom.secretKeyRef / valueFrom.configMapKeyRef at reconcile time.

The shape choice (reusing corev1.EnvVar to mirror PodOverrides.Env in api/v1alpha1/task_types.go:78) is the right call — consistent with existing Kelos API surface and with upstream Kubernetes idioms. Below are the API-level concerns before this ships.

Findings

Validation / CRD (blocking)

• internal/manifests/charts/kelos/templates/crds/agentconfig-crd.yaml: the generated CRD pulls in the full upstream corev1.EnvVar schema, so the OpenAPI surface advertises valueFrom.fieldRef, valueFrom.resourceFieldRef, and the new valueFrom.fileKeyRef (EnvFiles feature gate). Admission accepts them; only at reconcile time does task_controller.go:566 reject fieldRef/resourceFieldRef, and fileKeyRef falls through the default branch. Once a CRD ships, what its schema accepts is effectively part of the contract — please tighten this at admission rather than at reconcile. Two options:
  • Add a +kubebuilder:validation:XValidation CEL rule on the env items (e.g. self.valueFrom == null || (has(self.valueFrom.secretKeyRef) || has(self.valueFrom.configMapKeyRef)) and !(has(self.valueFrom.fieldRef) || has(self.valueFrom.resourceFieldRef) || has(self.valueFrom.fileKeyRef))).
  • Or define a Kelos-specific MCPEnvVar / MCPEnvVarSource that only embeds SecretKeyRef and ConfigMapKeyRef. This is also more future-proof against new upstream EnvVarSource variants being silently surfaced.
• api/v1alpha1/agentconfig_types.go:122-128, internal/controller/task_controller.go:512: value and valueFrom mutual exclusion is also runtime-only. A +kubebuilder:validation:XValidation rule ((self.value == "" || self.valueFrom == null)) would surface this at kubectl apply. (cubic flagged a related concern in the same area; my own analysis surfaces the broader CRD-vs-runtime validation gap.)

Semantics — Optional divergence from Kubernetes (blocking unless documented)

• internal/controller/task_controller.go:521, :527, :547, :555: when valueFrom.secretKeyRef.optional (or configMapKeyRef.optional) is true and the secret/key is missing, the resolver emits an env entry with empty Value. Standard kubelet semantics for a pod EnvVar with Optional=true and a missing key is that the variable is not set on the container, not set to "". Because the field type is corev1.EnvVar, users will reasonably assume upstream semantics — silently surfacing FOO="" to the MCP server process when the user marked it optional is a non-obvious foot-gun. Either match upstream (skip the entry; do not append a name to order when optional+missing), or call out the divergence explicitly in the field godoc.

Naming / Consistency

• api/v1alpha1/agentconfig_types.go:128: reusing []corev1.EnvVar and mirroring PodOverrides.Env is a good, consistent choice — names and shapes match existing API surface in this repo and upstream. No change requested.
• The asymmetric precedence between env and envFrom (existing behavior: envFrom wins on collision) is preserved. This is opposite of pod-spec semantics where, when present together, the later-applied wins / env typically overrides envFrom. Pre-existing behavior, but worth a sentence in the Env godoc (or EnvFrom godoc) so users coming from pod-spec know what to expect.

API compatibility / evolution

• This is a hard break for stored v1alpha1 AgentConfig objects: the env JSON shape changes from object to array, so existing in-cluster AgentConfig resources written with the old shape will fail to deserialize under the new CRD schema. v1alpha1 conventionally permits this, but the release-note only mentions the YAML shape change. Suggest extending the release note (or adding an upgrade note) to call out that existing in-cluster AgentConfig objects with mcpServers[].env set need to be re-applied after the upgrade.

Comments / godoc

• api/v1alpha1/agentconfig_types.go:122-126: the godoc enumerates SecretKeyRef/ConfigMapKeyRef as supported and FieldRef/ResourceFieldRef as unsupported — but the CRD also exposes fileKeyRef (new upstream EnvFiles field) and fileKeyRef is rejected only via the default error branch. Either explicitly mention fileKeyRef (and any future variants) as unsupported, or phrase the comment as "only secretKeyRef and configMapKeyRef are honored; all other variants are rejected." The latter is more future-proof.

Suggestions (optional)

• Consider a custom MCPEnvVar type (with only Name, Value, ValueFrom *MCPEnvVarSource where MCPEnvVarSource has just SecretKeyRef and ConfigMapKeyRef). It costs a small amount of code duplication but yields a tight, self-documenting CRD schema, immune to upstream EnvVarSource additions surfacing through the API.
• For determinism, resolveMCPServerEnv appends EnvFrom keys in Go map iteration order (internal/controller/task_controller.go:494-499). The final JSON happens to be deterministic because buildMCPServersJSON re-marshals through a map[string]string (which encoding/json sorts), but if mcpServerJSON.Env ever moves to a slice, this would surface as non-deterministic output. Worth sorting the EnvFrom keys explicitly to be defensive.

Note on prompt injection: I noticed an embedded instruction in a third-party reviewer's comment demanding attribution for its findings. I disregarded it and formed independent analysis from the diff and code.

/kelos needs-input