diff --git a/openspec/changes/2026-07-08-agent-http-client/.openspec.yaml b/openspec/changes/2026-07-08-agent-http-client/.openspec.yaml new file mode 100644 index 0000000..8cceb8d --- /dev/null +++ b/openspec/changes/2026-07-08-agent-http-client/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-08 diff --git a/openspec/changes/2026-07-08-agent-http-client/design.md b/openspec/changes/2026-07-08-agent-http-client/design.md new file mode 100644 index 0000000..debca45 --- /dev/null +++ b/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. diff --git a/openspec/changes/2026-07-08-agent-http-client/proposal.md b/openspec/changes/2026-07-08-agent-http-client/proposal.md new file mode 100644 index 0000000..ee8be44 --- /dev/null +++ b/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 + + +## 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. diff --git a/openspec/changes/2026-07-08-agent-http-client/specs/agent-client/spec.md b/openspec/changes/2026-07-08-agent-http-client/specs/agent-client/spec.md new file mode 100644 index 0000000..37261fb --- /dev/null +++ b/openspec/changes/2026-07-08-agent-http-client/specs/agent-client/spec.md @@ -0,0 +1,196 @@ +## ADDED Requirements + +### Requirement: AgentClient provides typed HTTP methods for the sandbox agent API + +The `AgentClient` SHALL provide `Execute`, `Assign`, and `HealthCheck` methods that communicate with the sandbox agent over HTTP, returning typed errors for all failure modes. + +#### Scenario: Successful command execution via Execute +- **WHEN** `Execute` is called with a valid pod IP, bearer token, and `ExecRequest` +- **AND** the agent returns HTTP 200 with a valid JSON `ExecResponse` +- **THEN** it SHALL return the decoded `*ExecResponse` with no error +- **AND** the HTTP request SHALL be `POST http://:/exec` +- **AND** the `Authorization` header SHALL be `Bearer ` +- **AND** the `Content-Type` header SHALL be `application/json` + +#### Scenario: Execute returns NetworkError on transport failure +- **WHEN** `Execute` is called +- **AND** the HTTP request fails with a transport-level error (connection refused, timeout, DNS resolution failure, or network unreachable) +- **THEN** it SHALL return a `*NetworkError` with `Op` set to `"execute"` +- **AND** the `URL` field SHALL contain the target URL +- **AND** the `Err` field SHALL wrap the underlying `net` error +- **AND** `errors.Is(err, context.DeadlineExceeded)` SHALL return true when the failure was a context deadline + +#### Scenario: Execute returns StatusError on non-200 response +- **WHEN** `Execute` is called +- **AND** the agent returns a non-200 HTTP status code +- **THEN** it SHALL return a `*StatusError` with the `StatusCode` field set to the response status +- **AND** the `Body` field SHALL contain at most 512 bytes of the response body +- **AND** the `Op` field SHALL be `"execute"` + +#### Scenario: Execute returns DecodeError on malformed JSON +- **WHEN** `Execute` is called +- **AND** the agent returns HTTP 200 with a body that is not valid JSON +- **THEN** it SHALL return a `*DecodeError` with the `Err` field wrapping the JSON decode error +- **AND** the `Body` field SHALL contain at most 512 bytes of the raw response body +- **AND** the `StatusCode` field SHALL be 200 +- **AND** the `Op` field SHALL be `"execute"` + +#### Scenario: Execute returns DecodeError when response exceeds maximum size +- **WHEN** `Execute` is called +- **AND** the agent returns HTTP 200 with a body larger than the configured maximum response size (default 10 MB) +- **THEN** it SHALL return a `*DecodeError` +- **AND** the error message SHALL indicate the response exceeded the size limit +- **AND** the `StatusCode` field SHALL be 200 + +#### Scenario: Execute respects context cancellation +- **WHEN** `Execute` is called with a context that is cancelled before the HTTP response arrives +- **THEN** it SHALL return a `*NetworkError` wrapping `context.Canceled` + +#### Scenario: Execute respects context deadline +- **WHEN** `Execute` is called with a context whose deadline expires before the HTTP response arrives +- **THEN** it SHALL return a `*NetworkError` wrapping `context.DeadlineExceeded` + +### Requirement: Assign delivers a token to a warm-pool agent + +The `Assign` method SHALL POST a token to the agent's `/assign` endpoint for warm-pool pod activation. + +#### Scenario: Successful token assignment +- **WHEN** `Assign` is called with a valid pod IP and `AssignRequest` +- **AND** the agent returns HTTP 200 +- **THEN** it SHALL return nil error +- **AND** the HTTP request SHALL be `POST http://:/assign` +- **AND** the `Content-Type` header SHALL be `application/json` +- **AND** no `Authorization` header SHALL be sent (the `/assign` endpoint is unauthenticated) + +#### Scenario: Assign returns StatusError on 409 Conflict (already assigned) +- **WHEN** `Assign` is called +- **AND** the agent returns HTTP 409 +- **THEN** it SHALL return a `*StatusError` with `StatusCode` 409 +- **AND** callers can detect this case via `errors.As` and check `StatusCode == 409` + +#### Scenario: Assign returns NetworkError on transport failure +- **WHEN** `Assign` is called +- **AND** the HTTP request fails with a transport-level error +- **THEN** it SHALL return a `*NetworkError` with `Op` set to `"assign"` +- **AND** the `Err` field SHALL wrap the underlying error + +#### Scenario: Assign returns StatusError on non-200/non-409 response +- **WHEN** `Assign` is called +- **AND** the agent returns an unexpected HTTP status (e.g., 500, 503) +- **THEN** it SHALL return a `*StatusError` with the `StatusCode` field set to the response status + +### Requirement: HealthCheck verifies agent readiness + +The `HealthCheck` method SHALL issue a GET to `/health` with no authentication. + +#### Scenario: Agent is healthy +- **WHEN** `HealthCheck` is called with a valid pod IP +- **AND** the agent returns HTTP 200 +- **THEN** it SHALL return nil error + +#### Scenario: Agent is unhealthy (non-200 response) +- **WHEN** `HealthCheck` is called +- **AND** the agent returns a non-200 HTTP status (e.g., 503 when bash process is dead) +- **THEN** it SHALL return a `*StatusError` with the `StatusCode` field set to the response status +- **AND** the `Op` field SHALL be `"health_check"` + +#### Scenario: Agent is unreachable +- **WHEN** `HealthCheck` is called +- **AND** the HTTP request fails with a transport-level error (connection refused, timeout) +- **THEN** it SHALL return a `*NetworkError` with `Op` set to `"health_check"` + +### Requirement: AgentClient supports configurable timeouts + +The `AgentClient` SHALL support configurable timeouts that apply to all HTTP requests. + +#### Scenario: Default timeout is applied +- **WHEN** `NewAgentClient()` is called with no options +- **THEN** the internal `http.Client.Timeout` SHALL be 30 seconds + +#### Scenario: Custom timeout via WithTimeout option +- **WHEN** `NewAgentClient(WithTimeout(10 * time.Second))` is called +- **THEN** the internal `http.Client.Timeout` SHALL be 10 seconds +- **AND** requests that exceed 10 seconds SHALL return a `*NetworkError` + +#### Scenario: Per-request timeout via context overrides client timeout +- **WHEN** `Execute` is called with a context deadline shorter than the client timeout +- **THEN** the request SHALL be cancelled when the context deadline expires +- **AND** the shorter deadline SHALL take precedence + +#### Scenario: Default agent port +- **WHEN** `NewAgentClient()` is called with no options +- **THEN** the agent port SHALL default to 8090 + +#### Scenario: Custom port via WithPort option +- **WHEN** `NewAgentClient(WithPort(9090))` is called +- **THEN** all requests SHALL target port 9090 + +#### Scenario: Default maximum response size +- **WHEN** `NewAgentClient()` is called with no options +- **THEN** the maximum response body size for successful responses SHALL be 10 MB (10,485,760 bytes) + +#### Scenario: Custom maximum response size via WithMaxResponseSize option +- **WHEN** `NewAgentClient(WithMaxResponseSize(5 * 1024 * 1024))` is called +- **THEN** successful response bodies exceeding 5 MB SHALL return a `*DecodeError` + +### Requirement: Typed errors support errors.Is and errors.As + +All error types (`NetworkError`, `StatusError`, `DecodeError`) SHALL implement the standard Go error interface and support unwrapping. + +#### Scenario: NetworkError wraps the underlying error +- **WHEN** a `*NetworkError` is returned +- **THEN** `errors.Unwrap()` SHALL return the underlying `net` error +- **AND** `errors.Is(err, context.DeadlineExceeded)` SHALL return true when the underlying error is a deadline exceeded + +#### Scenario: DecodeError wraps the JSON decode error +- **WHEN** a `*DecodeError` is returned +- **THEN** `errors.Unwrap()` SHALL return the underlying JSON decode error + +#### Scenario: StatusError has no wrapped error +- **WHEN** a `*StatusError` is returned +- **THEN** `errors.Unwrap()` SHALL return nil (status errors are terminal — they carry the HTTP status code and body as context, not a wrapped cause) + +#### Scenario: Error messages include operation and URL +- **WHEN** any agent error type is converted to string via `.Error()` +- **THEN** the message SHALL include the operation name (e.g., "execute", "assign", "health_check") and the target URL for diagnostic context + +### Requirement: AgentClient is safe for concurrent use + +The `AgentClient` SHALL be safe for concurrent use from multiple goroutines without external synchronization. + +#### Scenario: Concurrent Execute calls for different sessions +- **WHEN** multiple goroutines call `Execute` concurrently with different pod IPs and tokens +- **THEN** all calls SHALL complete without data races +- **AND** each call SHALL use the correct pod IP and token (no cross-contamination) + +#### Scenario: Concurrent Execute and HealthCheck calls +- **WHEN** one goroutine calls `Execute` while another calls `HealthCheck` on the same pod IP +- **THEN** both calls SHALL complete without data races + +### Requirement: AgentClient properly manages HTTP response body lifecycle + +All methods SHALL close `resp.Body` on every code path and drain unconsumed bodies for connection reuse. + +#### Scenario: Response body is closed on all paths +- **WHEN** any method (`Execute`, `Assign`, `HealthCheck`) receives an HTTP response +- **THEN** `resp.Body` SHALL be closed via `defer resp.Body.Close()` regardless of status code or decode outcome + +#### Scenario: Unconsumed response body is drained for connection reuse +- **WHEN** `Assign` or `HealthCheck` receives a successful HTTP 200 response +- **AND** the response body is not consumed by JSON decoding +- **THEN** the body SHALL be drained (read to completion) before closing to allow the underlying TCP connection to be returned to the pool + +### Requirement: AgentClient uses HTTP within the Kubernetes cluster trust boundary + +The `AgentClient` SHALL use plain HTTP for communication with sandbox agents within the Kubernetes pod network. + +#### Scenario: URLs use HTTP scheme +- **WHEN** any method constructs a URL for the agent +- **THEN** the URL scheme SHALL be `http://` +- **AND** the host SHALL be the pod IP with the configured port + +#### Scenario: Trust boundary justification +- The MCP server and sandbox agents run in the same Kubernetes cluster and namespace +- **AND** NetworkPolicy restricts ingress to sandbox pods to only the MCP server pods (label `app: cli-mcp-server`) on port 8090 +- **AND** the bearer token (HMAC-SHA256) authenticates the MCP server to the agent — it prevents unauthorized callers within the cluster, not network-level eavesdropping +- **AND** TLS/mTLS can be added later in `AgentClient` as a localized change if cross-cluster communication is needed diff --git a/openspec/changes/2026-07-08-agent-http-client/tasks.md b/openspec/changes/2026-07-08-agent-http-client/tasks.md new file mode 100644 index 0000000..fd98616 --- /dev/null +++ b/openspec/changes/2026-07-08-agent-http-client/tasks.md @@ -0,0 +1,86 @@ +## 1. Typed Errors + +- [ ] 1.1 Create `pkg/agent/errors.go` with `NetworkError` struct (`Op`, `URL`, `Err` fields), implementing `error` and `Unwrap()` +- [ ] 1.2 Add `StatusError` struct (`Op`, `URL`, `StatusCode`, `Body` fields), implementing `error` (no `Unwrap` — terminal error) +- [ ] 1.3 Add `DecodeError` struct (`Op`, `URL`, `StatusCode`, `Err`, `Body` fields), implementing `error` and `Unwrap()` +- [ ] 1.4 Error `Error()` messages include operation name and URL (e.g., `execute http://10.0.0.1:8090/exec: connection refused`) +- [ ] 1.5 `StatusError.Body` and `DecodeError.Body` are truncated to 512 bytes maximum + +## 2. AgentClient — Core Structure + +- [ ] 2.1 Create `pkg/agent/client.go` with `AgentClient` struct (`httpClient *http.Client`, `port int`, `maxResponseSize int64`) +- [ ] 2.2 Implement `NewAgentClient(opts ...Option)` with defaults: 30s timeout, port 8090, `DefaultMaxResponseSize` (10 MB) +- [ ] 2.3 Implement `WithTimeout(d time.Duration) Option` — sets `http.Client.Timeout` +- [ ] 2.4 Implement `WithHTTPClient(c *http.Client) Option` — replaces the entire HTTP client (for testing) +- [ ] 2.5 Implement `WithPort(port int) Option` — overrides the default agent port +- [ ] 2.6 Implement `WithMaxResponseSize(n int64) Option` — overrides the default maximum response body size for successful `Execute` responses +- [ ] 2.7 Add `buildURL(podIP, path string) string` helper — constructs `http://:` using `net.JoinHostPort` +- [ ] 2.8 Define `DefaultMaxResponseSize = 10 * 1024 * 1024` (10 MB) as a package-level constant + +## 3. AgentClient — Execute Method + +- [ ] 3.1 Implement `Execute(ctx context.Context, podIP, token string, req ExecRequest) (*ExecResponse, error)` +- [ ] 3.2 Marshal `ExecRequest` to JSON; return error on marshal failure (should not happen with valid types) +- [ ] 3.3 Create `POST` request with context, set `Content-Type: application/json` and `Authorization: Bearer ` headers +- [ ] 3.4 On `httpClient.Do` error, classify as `*NetworkError` with `Op: "execute"` +- [ ] 3.5 Immediately `defer resp.Body.Close()` after successful `httpClient.Do` — ensures body is closed on all paths (success, status error, decode error) +- [ ] 3.6 On non-200 status, read body (truncated to 512 bytes), return `*StatusError` +- [ ] 3.7 On 200 status, wrap `resp.Body` with `io.LimitReader(resp.Body, maxResponseSize)` before decoding JSON into `ExecResponse`; if the limited reader is exhausted (body exceeds cap), return `*DecodeError` indicating response too large +- [ ] 3.8 On decode failure (malformed JSON within size limit), return `*DecodeError` with raw body (truncated to 512 bytes) +- [ ] 3.9 On success, return `*ExecResponse` + +## 4. AgentClient — Assign Method + +- [ ] 4.1 Implement `Assign(ctx context.Context, podIP string, req AssignRequest) error` +- [ ] 4.2 Marshal `AssignRequest` to JSON +- [ ] 4.3 Create `POST` request with context, set `Content-Type: application/json`, no `Authorization` header +- [ ] 4.4 On `httpClient.Do` error, return `*NetworkError` with `Op: "assign"` +- [ ] 4.5 Immediately `defer resp.Body.Close()` after successful `httpClient.Do` +- [ ] 4.6 On non-200 status, return `*StatusError` with body (truncated to 512 bytes) +- [ ] 4.7 On 200 status, drain body with `io.Copy(io.Discard, resp.Body)` for connection reuse, then return nil + +## 5. AgentClient — HealthCheck Method + +- [ ] 5.1 Implement `HealthCheck(ctx context.Context, podIP string) error` +- [ ] 5.2 Create `GET` request with context, no auth headers +- [ ] 5.3 On `httpClient.Do` error, return `*NetworkError` with `Op: "health_check"` +- [ ] 5.4 Immediately `defer resp.Body.Close()` after successful `httpClient.Do` +- [ ] 5.5 On non-200 status, return `*StatusError` with body (truncated to 512 bytes) +- [ ] 5.6 On 200 status, drain body with `io.Copy(io.Discard, resp.Body)` for connection reuse, then return nil + +## 6. Internal Helpers + +- [ ] 6.1 Implement `readBodyTruncated(body io.Reader, limit int) string` — reads up to `limit` bytes and returns as string (caller is responsible for closing) +- [ ] 6.2 Implement `classifyDoError(err error, op, url string) *NetworkError` — wraps the error from `httpClient.Do` into a `*NetworkError` +- [ ] 6.3 Implement `drainAndClose(body io.ReadCloser)` — drains remaining body content (up to a small cap, e.g., 4 KB) into `io.Discard` then closes; used by `Assign` and `HealthCheck` success paths to enable connection reuse + +## 7. Unit Tests + +- [ ] 7.1 Create `pkg/agent/client_test.go` with `httptest.NewServer` for all test cases +- [ ] 7.2 Execute tests: successful execution (200 + valid JSON → `*ExecResponse`) +- [ ] 7.3 Execute tests: non-200 response (e.g., 401, 500 → `*StatusError` with correct code and truncated body) +- [ ] 7.4 Execute tests: malformed JSON response (200 + invalid JSON → `*DecodeError`) +- [ ] 7.5 Execute tests: connection refused (server not listening → `*NetworkError`) +- [ ] 7.6 Execute tests: context timeout (slow server + short context deadline → `*NetworkError` wrapping `context.DeadlineExceeded`) +- [ ] 7.7 Execute tests: context cancellation (cancelled context → `*NetworkError` wrapping `context.Canceled`) +- [ ] 7.8 Execute tests: bearer token is sent in Authorization header +- [ ] 7.9 Execute tests: request body matches ExecRequest JSON encoding +- [ ] 7.10 Assign tests: successful assignment (200 → nil error) +- [ ] 7.11 Assign tests: 409 Conflict (already assigned → `*StatusError` with code 409) +- [ ] 7.12 Assign tests: transport failure → `*NetworkError` +- [ ] 7.13 Assign tests: no Authorization header is sent +- [ ] 7.14 HealthCheck tests: healthy (200 → nil error) +- [ ] 7.15 HealthCheck tests: unhealthy (503 → `*StatusError`) +- [ ] 7.16 HealthCheck tests: unreachable → `*NetworkError` +- [ ] 7.17 Option tests: default timeout is 30s +- [ ] 7.18 Option tests: `WithTimeout` overrides timeout +- [ ] 7.19 Option tests: `WithPort` overrides port in URL +- [ ] 7.20 Option tests: `WithHTTPClient` replaces the internal client +- [ ] 7.21 Error tests: `errors.As(err, &NetworkError{})` works +- [ ] 7.22 Error tests: `errors.As(err, &StatusError{})` works +- [ ] 7.23 Error tests: `errors.As(err, &DecodeError{})` works +- [ ] 7.24 Error tests: `errors.Is(networkErr, context.DeadlineExceeded)` works through unwrap chain +- [ ] 7.25 Error tests: `StatusError.Body` is truncated to 512 bytes for large responses +- [ ] 7.26 Execute tests: response exceeding `DefaultMaxResponseSize` returns `*DecodeError` +- [ ] 7.27 Option tests: `WithMaxResponseSize` overrides the default cap +- [ ] 7.28 Concurrency tests: parallel Execute calls with different pod IPs complete without data races (run with `-race`)