SANDBOX-1981: MCP hot-path contract

README.md

@@ -13,7 +13,7 @@ Designed to run against a Kubernetes (or OpenShift) cluster — the server manag
 - **Per-session agent auth** — MCP server → sandbox `/exec` uses an HMAC-derived bearer token (defense in depth beyond NetworkPolicy)
 - **Customizable CLIs** — available tools are whatever is in the sandbox agent image (the default image includes `oc`/`kubectl`)
 - **Stateless, multi-replica ready** — any server replica can handle any request; Kubernetes is the source of truth
-- **Optional warm pool** — pre-warmed pods cut cold-start latency when enabled
+- **Claim then create** — MCP claims an instance-labeled unassigned pod if one exists, otherwise creates on demand
 
 ## Usage
 
@@ -51,7 +51,7 @@ Typical client practice (agent harness / orchestrator — not the LLM):
 2. Send that same ID on every follow-up `bash` call to reuse the persistent shell and `/workspace`
 3. Call `DELETE /sessions/{id}` when finished (HTTP transport only) — removes the sandbox pod, auth secret, and cache entry
 
-If the client never deletes the session, the sandbox is garbage-collected after `--idle-timeout` (default 30m).
+If the client never deletes the session, the sandbox pod remains until something else deletes it (operator idle GC, or a manual `DELETE`).
 
 ## Configuration
 
@@ -62,12 +62,21 @@ If the client never deletes the session, the sandbox is garbage-collected after
 | `--transport` | `stdio` | `stdio` or `http` (`http` requires `--stateless`) |
 | `--address` | `localhost:8080` | Listen address (HTTP; must be loopback) |
 | `--stateless` | `false` | Required for HTTP / multi-replica |
-| `--namespace` | `tarsy` | Namespace for sandbox pods |
+| `--namespace` | _(required)_ | Namespace for sandbox pods |
+| `--instance-name` | _(required)_ | Instance id on sandbox labels (`cli-mcp.redhat.com/instance`) |
 | `--sandbox-image` | _(required)_ | Container image for sandbox pods |
 | `--hmac-key-file` | _(required)_ | Path to shared HMAC secret |
-| `--kubeconfig` | _(in-cluster)_ | Kubeconfig for managing sandbox pods |
-| `--idle-timeout` | `30m` | Delete idle sandbox pods after this duration |
-| `--warm-pool-size` | `0` | Pre-warmed pods (`0` = create on demand) |
+| `--kubeconfig-secret` | _(required)_ | Investigation kubeconfig Secret mounted into sandbox pods |
+| `--sandbox-service-account` | _(required)_ | ServiceAccount name for sandbox pods |
+| `--kubeconfig` | _(in-cluster)_ | Kubeconfig for the MCP process's Kubernetes client (not the investigation Secret) |
+| `--sandbox-cpu-request` | `100m` | Sandbox CPU request |
+| `--sandbox-cpu-limit` | `500m` | Sandbox CPU limit |
+| `--sandbox-memory-request` | `128Mi` | Sandbox memory request |
+| `--sandbox-memory-limit` | `512Mi` | Sandbox memory limit |
+| `--sandbox-image-pull-policy` | _(empty)_ | Sandbox `imagePullPolicy` (`Always`, `Never`, `IfNotPresent`) |
+| `--sandbox-env` | _(empty)_ | JSON `[]corev1.EnvVar` overlay for sandbox pods |
+| `--idle-timeout` | `30m` | Parsed for CLI compatibility; MCP does not idle-GC |
+| `--warm-pool-size` | `0` | Parsed for CLI compatibility; MCP does not replenish a pool |
 
 ### Sandbox image (available CLIs)
 
@@ -89,7 +98,7 @@ No MCP server code changes are required. Tell the LLM what is available via your
 flowchart TB
   Client[MCP Client]
   Server["cli-mcp-server<br/>stateless · N replicas"]
-  Sandbox["Sandbox pods<br/>assigned sessions · optional warm pool"]
+  Sandbox["Sandbox pods<br/>assigned sessions · claim or create"]
   Target["Target infrastructure<br/>e.g. Kubernetes API"]
 
   Client -->|"bash + X-Session-ID"| Server
@@ -103,14 +112,14 @@ Bash commands run in the sandbox pods. What they can reach (for example a Kubern
 
 The MCP server holds no durable session state. Pod identity is stored in Kubernetes labels; an in-memory cache speeds up routing. Any replica can serve any request, so you can scale the Deployment horizontally behind a load balancer with no sticky sessions.
 
-Optional `--warm-pool-size` keeps ready pods on hand so new sessions skip cold start (image pull + container boot).
+MCP always claims an instance-labeled unassigned pod if one exists, otherwise creates on demand. Pool replenishment and idle GC are owned by the operator, not this process.
 
 ### Sandboxing and security
 
 Each session gets its own pod. That pod is the security boundary:
 
 - **Isolation** — non-root (runAsNonRoot), no privilege escalation, all capabilities dropped, resource limits
-- **Credentials** — read-only kubeconfig mounted from a dedicated investigation ServiceAccount (typically view/read-only RBAC)
+- **Credentials** — investigation kubeconfig Secret mounted into the sandbox; dedicated sandbox SA with `automountServiceAccountToken: false`
 - **Network** — NetworkPolicy can restrict ingress to the MCP server and egress to intended APIs
 - **Agent auth** — per-session HMAC bearer token; unauthenticated `/exec` calls are rejected
 - **Ephemeral workspace** — `/workspace` is an `emptyDir`; destroyed with the pod
@@ -139,7 +148,7 @@ make build-prod     # Production build (static, CGO disabled)
 cli-mcp-server/
 ├── cmd/server/     # MCP server entry point
 ├── cmd/agent/      # Sandbox agent entry point
-├── pkg/session/    # Pod lifecycle, warm pool, cache
+├── pkg/session/    # Pod lifecycle, claim, cache
 ├── pkg/sandbox/    # Bash session + agent HTTP handlers
 ├── pkg/tools/      # MCP tool handlers
 ├── pkg/server/     # MCP server + HTTP mux

cmd/server/flags.go

@@ -0,0 +1,101 @@
+package main
+
+import (
+	"encoding/json"
+	"fmt"
+	"strings"
+
+	"github.com/codeready-toolchain/cli-mcp-operator/pkg/session"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+)
+
+func buildSandboxConfig(cfg runConfig, hmacKey string) (session.SandboxConfig, error) {
+	if cfg.sandboxImage == "" {
+		return session.SandboxConfig{}, fmt.Errorf("--sandbox-image is required")
+	}
+	if cfg.namespace == "" {
+		return session.SandboxConfig{}, fmt.Errorf("--namespace is required")
+	}
+	if cfg.instanceName == "" {
+		return session.SandboxConfig{}, fmt.Errorf("--instance-name is required")
+	}
+	if cfg.kubeconfigSecret == "" {
+		return session.SandboxConfig{}, fmt.Errorf("--kubeconfig-secret is required")
+	}
+	if cfg.sandboxServiceAccount == "" {
+		return session.SandboxConfig{}, fmt.Errorf("--sandbox-service-account is required")
+	}
+
+	defaults := session.DefaultConfig()
+	cpuRequest, err := parseQuantityFlag("--sandbox-cpu-request", cfg.cpuRequest, defaults.CPURequest)
+	if err != nil {
+		return session.SandboxConfig{}, err
+	}
+	cpuLimit, err := parseQuantityFlag("--sandbox-cpu-limit", cfg.cpuLimit, defaults.CPULimit)
+	if err != nil {
+		return session.SandboxConfig{}, err
+	}
+	memoryRequest, err := parseQuantityFlag("--sandbox-memory-request", cfg.memoryRequest, defaults.MemoryRequest)
+	if err != nil {
+		return session.SandboxConfig{}, err
+	}
+	memoryLimit, err := parseQuantityFlag("--sandbox-memory-limit", cfg.memoryLimit, defaults.MemoryLimit)
+	if err != nil {
+		return session.SandboxConfig{}, err
+	}
+	pullPolicy, err := parseImagePullPolicy(cfg.imagePullPolicy)
+	if err != nil {
+		return session.SandboxConfig{}, err
+	}
+	env, err := parseSandboxEnv(cfg.sandboxEnv)
+	if err != nil {
+		return session.SandboxConfig{}, err
+	}
+
+	return session.SandboxConfig{
+		Image:              cfg.sandboxImage,
+		HMACKey:            hmacKey,
+		Namespace:          cfg.namespace,
+		InstanceName:       cfg.instanceName,
+		ServiceAccountName: cfg.sandboxServiceAccount,
+		KubeconfigSecret:   cfg.kubeconfigSecret,
+		CPURequest:         cpuRequest,
+		CPULimit:           cpuLimit,
+		MemoryRequest:      memoryRequest,
+		MemoryLimit:        memoryLimit,
+		ImagePullPolicy:    pullPolicy,
+		Env:                env,
+		AgentPort:          defaults.AgentPort,
+	}, nil
+}
+
+func parseQuantityFlag(flag, value, fallback string) (string, error) {
+	if value == "" {
+		value = fallback
+	}
+	if _, err := resource.ParseQuantity(value); err != nil {
+		return "", fmt.Errorf("%s: invalid quantity %q: %w", flag, value, err)
+	}
+	return value, nil
+}
+
+func parseImagePullPolicy(s string) (corev1.PullPolicy, error) {
+	switch corev1.PullPolicy(s) {
+	case "", corev1.PullAlways, corev1.PullNever, corev1.PullIfNotPresent:
+		return corev1.PullPolicy(s), nil
+	default:
+		return "", fmt.Errorf("--sandbox-image-pull-policy must be Always, Never, or IfNotPresent")
+	}
+}
+
+func parseSandboxEnv(raw string) ([]corev1.EnvVar, error) {
+	if strings.TrimSpace(raw) == "" {
+		return nil, nil
+	}
+	var env []corev1.EnvVar
+	if err := json.Unmarshal([]byte(raw), &env); err != nil {
+		return nil, fmt.Errorf("--sandbox-env must be JSON []corev1.EnvVar: %w", err)
+	}
+	return env, nil
+}

cmd/server/flags_test.go

@@ -0,0 +1,148 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/codeready-toolchain/cli-mcp-operator/pkg/session"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+)
+
+func validRunConfig() runConfig {
+	return runConfig{ //nolint:gosec // G101: K8s Secret resource name, not a credential
+		sandboxImage:          "quay.io/example/sandbox:test",
+		namespace:             "cli-mcp",
+		instanceName:          "oc",
+		kubeconfigSecret:      "cli-mcp-oc-kubeconfig",
+		sandboxServiceAccount: "cli-mcp-oc-sandbox",
+	}
+}
+
+func TestBuildSandboxConfig(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name    string
+		mutate  func(*runConfig)
+		wantErr string
+		check   func(*testing.T, session.SandboxConfig)
+	}{
+		{
+			name: "requires instance-name",
+			mutate: func(c *runConfig) {
+				c.instanceName = ""
+			},
+			wantErr: "--instance-name is required",
+		},
+		{
+			name: "requires kubeconfig-secret",
+			mutate: func(c *runConfig) {
+				c.kubeconfigSecret = ""
+			},
+			wantErr: "--kubeconfig-secret is required",
+		},
+		{
+			name: "requires sandbox-service-account",
+			mutate: func(c *runConfig) {
+				c.sandboxServiceAccount = ""
+			},
+			wantErr: "--sandbox-service-account is required",
+		},
+		{
+			name: "requires namespace",
+			mutate: func(c *runConfig) {
+				c.namespace = ""
+			},
+			wantErr: "--namespace is required",
+		},
+		{
+			name: "requires sandbox-image",
+			mutate: func(c *runConfig) {
+				c.sandboxImage = ""
+			},
+			wantErr: "--sandbox-image is required",
+		},
+		{
+			name: "uses DefaultConfig resources when quantity flags are empty",
+			check: func(t *testing.T, cfg session.SandboxConfig) {
+				t.Helper()
+				defaults := session.DefaultConfig()
+				assert.Equal(t, defaults.CPURequest, cfg.CPURequest)
+				assert.Equal(t, defaults.CPULimit, cfg.CPULimit)
+				assert.Equal(t, defaults.MemoryRequest, cfg.MemoryRequest)
+				assert.Equal(t, defaults.MemoryLimit, cfg.MemoryLimit)
+				assert.Empty(t, cfg.ImagePullPolicy)
+				assert.Empty(t, cfg.Env)
+			},
+		},
+		{
+			name: "parses overlay flags",
+			mutate: func(c *runConfig) {
+				c.cpuRequest = "200m"
+				c.cpuLimit = "1"
+				c.memoryRequest = "256Mi"
+				c.memoryLimit = "1Gi"
+				c.imagePullPolicy = "IfNotPresent"
+				c.sandboxEnv = `[{"name":"AWS_REGION","value":"us-east-1"}]`
+			},
+			check: func(t *testing.T, cfg session.SandboxConfig) {
+				t.Helper()
+				assert.Equal(t, "200m", cfg.CPURequest)
+				assert.Equal(t, "1", cfg.CPULimit)
+				assert.Equal(t, "256Mi", cfg.MemoryRequest)
+				assert.Equal(t, "1Gi", cfg.MemoryLimit)
+				assert.Equal(t, corev1.PullIfNotPresent, cfg.ImagePullPolicy)
+				require.Len(t, cfg.Env, 1)
+				assert.Equal(t, "AWS_REGION", cfg.Env[0].Name)
+				assert.Equal(t, "us-east-1", cfg.Env[0].Value)
+			},
+		},
+		{
+			name: "rejects invalid pull policy",
+			mutate: func(c *runConfig) {
+				c.imagePullPolicy = "Sometimes"
+			},
+			wantErr: "--sandbox-image-pull-policy",
+		},
+		{
+			name: "rejects invalid quantity",
+			mutate: func(c *runConfig) {
+				c.cpuRequest = "not-a-quantity"
+			},
+			wantErr: "--sandbox-cpu-request",
+		},
+		{
+			name: "rejects invalid sandbox-env JSON",
+			mutate: func(c *runConfig) {
+				c.sandboxEnv = "{not json"
+			},
+			wantErr: "--sandbox-env",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			cfg := validRunConfig()
+			if tt.mutate != nil {
+				tt.mutate(&cfg)
+			}
+
+			got, err := buildSandboxConfig(cfg, "hmac")
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, "oc", got.InstanceName)
+			assert.Equal(t, "cli-mcp", got.Namespace)
+			assert.Equal(t, "cli-mcp-oc-sandbox", got.ServiceAccountName)
+			assert.Equal(t, "cli-mcp-oc-kubeconfig", got.KubeconfigSecret)
+			if tt.check != nil {
+				tt.check(t, got)
+			}
+		})
+	}
+}

cmd/server/health_test.go

@@ -0,0 +1,23 @@
+package main
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/fake"
+	k8stesting "k8s.io/client-go/testing"
+)
+
+func TestK8sHealthCheckerListsPodsInNamespace(t *testing.T) {
+	t.Parallel()
+
+	client := fake.NewSimpleClientset()
+	client.PrependReactor("get", "namespaces", func(_ k8stesting.Action) (bool, runtime.Object, error) {
+		return true, nil, fmt.Errorf("cluster-scoped namespace get must not be used")
+	})
+	checker := &k8sHealthChecker{clientset: client, namespace: "cli-mcp"}
+
+	require.NoError(t, checker.CheckHealth(t.Context()))
+}