SANDBOX-1812: add openspec for agent HTTP client

openspec/changes/2026-07-08-agent-http-client/.openspec.yaml

@@ -0,0 +1,2 @@
+schema: spec-driven
+created: 2026-07-08

openspec/changes/2026-07-08-agent-http-client/design.md

@@ -0,0 +1,169 @@
+## Context
+
+The CLI MCP Server communicates with sandbox agents over HTTP within the Kubernetes pod network. Two existing call sites — `SessionManager.ExecuteCommand` in `manager.go` and `WarmPool.assignToken` in `pool.go` — construct HTTP requests inline, each with slightly different error handling patterns. The session manager invalidates its pod cache on transport errors; the warm pool does not (it rolls back the entire claim). Neither call site distinguishes between "agent returned 500" and "agent's response was garbage JSON" — both are opaque `fmt.Errorf` strings.
+
+SANDBOX-1813 (bash tool handler) needs to call `Execute` with clean error semantics: transport errors should trigger cache invalidation, status errors should be reported with the HTTP code, and decode errors should surface the malformed body for debugging. Building this into each call site would mean triplicate error classification logic.
+
+`AgentClient` centralizes agent HTTP communication in one place with typed errors, configurable timeouts, and a consistent API surface. The session manager and warm pool will be refactored to use it (reducing their inline HTTP code to single-method calls), and the bash tool handler will depend on it directly.
+
+## Goals / Non-Goals
+
+**Goals:**
+- `AgentClient` struct with `Execute`, `Assign`, `HealthCheck` methods
+- Functional options pattern for configuration (timeout, HTTP client, agent port)
+- Typed error hierarchy: `*NetworkError`, `*StatusError`, `*DecodeError` — each implementing `error` and `Unwrap()`
+- Thread-safe: `AgentClient` holds only immutable config and a shared `*http.Client` (which is documented as safe for concurrent use). Multiple goroutines can call `Execute`, `Assign`, `HealthCheck` concurrently without external synchronization.
+- Per-request context support: all methods accept `context.Context` for cancellation and deadline propagation
+- Trust boundary documentation: HTTP (not HTTPS) within cluster, with rationale
+
+**Non-Goals:**
+- Retry logic (callers decide whether/how to retry based on error type)
+- Circuit breaker or connection pooling configuration (stdlib defaults are sufficient)
+- Refactoring `manager.go` and `pool.go` to use `AgentClient` (separate change, reduces risk)
+- Metrics or tracing instrumentation (SANDBOX-1820)
+- Streaming/chunked response delivery to callers (callers receive the full decoded response)
+
+## Decisions
+
+### Decision 1: Separate `AgentClient` struct in `pkg/agent/` alongside `types.go`
+
+Place the client in the same package as the shared types it uses (`ExecRequest`, `ExecResponse`, `AssignRequest`).
+
+**Rationale:**
+- The `pkg/agent` package is already the home for the agent wire contract (`types.go`). Adding the client here keeps all agent-facing code together.
+- Avoids a circular dependency — `pkg/session` imports `pkg/agent`, not the other way around
+- The client is a pure HTTP wrapper over the types defined in the same package — they are a natural unit
+
+**Alternative considered:** `pkg/agent/client/` sub-package
+- Rejected — introduces an extra package for a single struct; the `pkg/agent` package is small (one file today) and benefits from co-location
+
+### Decision 2: Functional options for `NewAgentClient`
+
+```go
+type Option func(*AgentClient)
+
+func WithTimeout(d time.Duration) Option
+func WithHTTPClient(c *http.Client) Option
+func WithPort(port int) Option
+func WithMaxResponseSize(n int64) Option
+```
+
+**Rationale:**
+- Follows idiomatic Go patterns for optional configuration
+- Defaults are sensible (30s timeout, `http.DefaultClient`-like behavior, port 8090, 10 MB max response size)
+- Easy to extend with new options (e.g., `WithLogger`) without breaking existing callers
+- Testing injects a custom `*http.Client` via `WithHTTPClient`
+
+**Alternative considered:** Config struct parameter
+- Acceptable but less ergonomic for the common case where defaults suffice. Functional options are the established pattern in this codebase (see `mcp-common` middleware options).
+
+### Decision 3: Typed error hierarchy with `Kind` enum
+
+Three concrete error types, each carrying domain-relevant context:
+
+```go
+type NetworkError struct {
+    Op  string    // "execute", "assign", "health_check"
+    URL string
+    Err error     // underlying net error
+}
+
+type StatusError struct {
+    Op         string
+    URL        string
+    StatusCode int
+    Body       string  // first 512 bytes of response body for debugging
+}
+
+type DecodeError struct {
+    Op         string
+    URL        string
+    StatusCode int
+    Err        error   // json.Unmarshal error
+    Body       string  // first 512 bytes of raw body
+}
+```
+
+**Rationale:**
+- Callers can use `errors.As` to branch on error type: `*NetworkError` → invalidate cache + retry candidate; `*StatusError` → report HTTP code to user; `*DecodeError` → log body for debugging
+- Each type carries the operation name and URL for structured logging
+- `StatusError.Body` is truncated to 512 bytes to prevent log pollution from large error pages
+- All types implement `Unwrap()` so `errors.Is(err, context.DeadlineExceeded)` works through the chain
+
+**Alternative considered:** Single error type with a `Kind` field
+- Rejected — Go's `errors.As` type-switch pattern works better with distinct types; a `Kind` enum requires manual switch statements and provides no compile-time exhaustiveness
+
+**Alternative considered:** Return `(result, statusCode, error)` tuples
+- Rejected — makes every call site handle three values; typed errors encode the status code for callers that need it while keeping the common `(result, error)` signature clean
+
+### Decision 4: HTTP (not HTTPS) for intra-cluster agent communication
+
+The `AgentClient` constructs URLs with `http://` scheme.
+
+**Rationale:**
+- The MCP server and sandbox agents communicate over the Kubernetes pod network within the same cluster and namespace
+- NetworkPolicy restricts ingress to sandbox pods: only pods with `app: cli-mcp-server` label can reach port 8090
+- The bearer token (HMAC-SHA256 derived) authenticates the caller — it guards against unauthorized requests from within the cluster network, not against network-level eavesdropping
+- TLS between pods in the same cluster adds certificate management complexity (cert-manager, Secret rotation, mTLS handshake latency) for marginal benefit — the threat model assumes the intra-cluster network is trusted
+- If cross-cluster or external-network communication is needed in future, upgrading to TLS/mTLS is a localized change in `AgentClient` (change URL scheme + configure TLS transport)
+
+This is consistent with how the session manager and warm pool already communicate with agents (plain HTTP) and matches the parent design doc's architecture diagram.
+
+### Decision 5: `Execute` accepts token as a parameter, not a stored field
+
+```go
+func (c *AgentClient) Execute(ctx context.Context, podIP, token string, req ExecRequest) (*ExecResponse, error)
+```
+
+The bearer token is passed per-call rather than stored in the `AgentClient`.
+
+**Rationale:**
+- Tokens are per-session (HMAC-SHA256 of session ID). A single `AgentClient` instance is shared across all sessions — storing a token would require either one client per session (wasteful) or a token-lookup function (over-engineered).
+- The session manager already computes the token from `computeToken(hmacKey, sessionID)` — passing it through is natural.
+- `Assign` does not use a bearer token (the `/assign` endpoint is unauthenticated, callable once). Storing a token on the client would be misleading for `Assign` calls.
+
+### Decision 6: `HealthCheck` is a simple GET with no auth
+
+```go
+func (c *AgentClient) HealthCheck(ctx context.Context, podIP string) error
+```
+
+**Rationale:**
+- The agent's `GET /health` is unauthenticated — it's used by Kubernetes readiness probes which can't inject bearer tokens
+- Returns `nil` for 200, `*StatusError` for non-200, `*NetworkError` for transport failure
+- Used by the session manager to verify pod readiness after warm pool claim, and potentially by the MCP server's own health check
+
+### Decision 7: Request body and response body size safety
+
+- Request bodies (`ExecRequest`, `AssignRequest`) are small by design — the largest field is `ExecRequest.Command` which is bounded by the bash tool handler's input validation
+- **Successful response bodies** are read into memory for JSON decoding via `io.LimitReader` with a default cap of **10 MB** (`DefaultMaxResponseSize`). This prevents a single large `Stdout`/`Stderr` response from exhausting process memory during decode. The limit is configurable via `WithMaxResponseSize(n int64) Option`. If the response exceeds the limit, the client returns a `*DecodeError` (the body is valid but too large to safely decode in-process). The 10 MB default is generous for CLI command output — commands producing more output than this are pathological and should be bounded upstream (at the agent or tool handler level).
+- **Error response bodies** (for `StatusError` and `DecodeError`) are truncated to 512 bytes to prevent log pollution
+- **Response body lifecycle:** All methods (`Execute`, `Assign`, `HealthCheck`) close `resp.Body` on every path — both success and error — via `defer resp.Body.Close()` immediately after `httpClient.Do` returns. On success paths where the body is not fully consumed (e.g., `Assign`, `HealthCheck`), the body is drained with `io.Copy(io.Discard, resp.Body)` before closing to enable HTTP connection reuse.
+
+### Decision 8: No automatic retries
+
+The `AgentClient` does not retry failed requests. Each method makes exactly one HTTP call.
+
+**Rationale:**
+- Retry semantics depend on context: `Execute` is not idempotent (running a command twice has side effects), `Assign` returns 409 on replay (safe to retry but pointless), `HealthCheck` is idempotent (safe to retry)
+- The session manager already has its own retry/fallback logic (cache invalidation → rediscovery → on-demand creation)
+- Adding retries in the client would conflict with context deadlines and make timeout behavior harder to reason about
+- Callers are best positioned to decide retry strategy based on the typed error they receive
+
+## Risks / Trade-offs
+
+**Risk:** `AgentClient` and inline HTTP code coexist temporarily until `manager.go` and `pool.go` are refactored
+→ **Mitigation:** The refactoring is a mechanical change — replace inline HTTP blocks with `AgentClient` method calls. It can be done in SANDBOX-1813 or as a standalone cleanup. The typed errors make the refactoring straightforward since the existing `isTransportError` logic maps directly to `errors.As(err, &NetworkError{})`.
+
+**Risk:** `StatusError.Body` truncation at 512 bytes may lose diagnostic information for large error responses
+→ **Mitigation:** 512 bytes is sufficient for typical error messages ("unauthorized", "already assigned", JSON error objects). If a specific endpoint returns larger error payloads, the limit can be increased per-endpoint later.
+
+**Trade-off:** 10 MB default cap on successful `Execute` response bodies
+→ **Accepted:** The cap prevents unbounded memory allocation during JSON decode while being generous enough for all realistic CLI command output. Commands producing >10 MB of stdout/stderr are pathological (e.g., `cat /dev/urandom | base64`). The limit is configurable via `WithMaxResponseSize` for callers with known-large payloads. When exceeded, the client returns a `*DecodeError` with a clear message — callers can distinguish "response too large" from "malformed JSON" via the error text.
+
+**Trade-off:** Single shared `*http.Client` across all methods and sessions
+→ **Accepted:** `http.Client` is documented as safe for concurrent use and internally manages a connection pool. Per-session clients would waste connections and prevent connection reuse between calls to the same agent pod.
+
+## Open Questions
+
+None — all design decisions are resolved based on the existing patterns in `manager.go`, `pool.go`, and the parent design doc.

openspec/changes/2026-07-08-agent-http-client/proposal.md

@@ -0,0 +1,44 @@
+## Why
+
+The session manager (`pkg/session/manager.go`) and warm pool (`pkg/session/pool.go`) both make HTTP calls to the sandbox agent — `POST /exec` with a bearer token, `POST /assign` with a token payload, and readiness checks via `GET /health`. Today each call site constructs `http.Request` objects inline, marshals JSON manually, checks status codes, and decodes responses with duplicated error handling. This makes the agent HTTP contract hard to reason about and easy to break in subtle ways (e.g., one call site invalidates the cache on transport errors while another doesn't, or a new endpoint is added without consistent timeout handling).
+
+SANDBOX-1812 introduces `pkg/agent/client.go` — a typed HTTP client that encapsulates all agent communication behind three methods: `Execute`, `Assign`, and `HealthCheck`. The client owns JSON encoding/decoding, bearer token injection, timeout enforcement, and a structured error type hierarchy that distinguishes network failures from non-200 responses from JSON decode errors. Once in place, the session manager and warm pool can replace their inline HTTP code with single-method calls, and the downstream bash tool handler (SANDBOX-1813) gets a clean interface to build on.
+
+SANDBOX-1806 delivered the shared types (`ExecRequest`, `ExecResponse`, `AssignRequest`) that define the wire contract. This story builds the Go HTTP client that speaks that contract.
+
+## What Changes
+
+- Add `pkg/agent/client.go` with:
+  - `AgentClient` struct holding an `*http.Client`, default timeout, and agent port
+  - `NewAgentClient(opts ...Option)` constructor with functional options for timeout, HTTP client, and port
+  - `Execute(ctx, podIP, token, req)` — POST `/exec` with bearer auth, returns `*ExecResponse` or typed error
+  - `Assign(ctx, podIP, req)` — POST `/assign` with JSON body, returns typed error
+  - `HealthCheck(ctx, podIP)` — GET `/health`, returns typed error
+
+- Add `pkg/agent/errors.go` with:
+  - `AgentError` base type with `Kind` enum (`NetworkError`, `StatusError`, `DecodeError`)
+  - `NetworkError` — wraps transport-level failures (connection refused, timeout, DNS)
+  - `StatusError` — non-200 HTTP status with status code and optional body
+  - `DecodeError` — JSON decode failure with the raw body that couldn't be parsed
+  - All error types implement `error` and `Unwrap()` for `errors.Is`/`errors.As` compatibility
+
+- Add `pkg/agent/client_test.go` with tests against `httptest.NewServer`
+
+## Capabilities
+
+### New Capabilities
+- `agent-client`: Typed HTTP client for the sandbox agent API (`/exec`, `/assign`, `/health`) with structured error handling and configurable timeouts
+
+### Modified Capabilities
+<!-- None — existing code is not modified in this story. The session manager and warm pool will be refactored to use AgentClient in a follow-up or as part of SANDBOX-1813 integration. -->
+
+## Impact
+
+- **Affected code**:
+  - `pkg/agent/client.go` — New file with AgentClient
+  - `pkg/agent/errors.go` — New file with typed error types
+  - `pkg/agent/client_test.go` — New file with unit tests
+- **API changes**: New Go API in `pkg/agent` package. No external HTTP endpoints added.
+- **Dependencies**: Depends on SANDBOX-1806 (`pkg/agent/types.go` — shared request/response types). Depended on by SANDBOX-1813 (bash tool handler).
+- **No new Go dependencies** — uses `net/http`, `encoding/json`, `context`, `fmt`, `errors` from stdlib.
+- **No breaking changes** — entirely new files. Existing inline HTTP code in `manager.go` and `pool.go` continues to work and will be refactored separately.