Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-05-14
136 changes: 136 additions & 0 deletions openspec/changes/archive/2026-05-14-agent-http-handlers/design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
## Context

The CLI MCP Server architecture has two binaries: the MCP server (control plane) and the sandbox agent (data plane). SANDBOX-1806 delivered the shared HTTP contract types and the persistent bash session. SANDBOX-1807 adds the HTTP layer and binary entry point — the glue between the network and the bash session.

The sandbox agent exposes three HTTP endpoints on port 8090. The MCP server calls `/exec` to run commands and `/assign` to deliver auth tokens to warm-pool pods. Kubernetes kubelet calls `/health` for readiness probes.

## Goals / Non-Goals

**Goals:**
- Expose POST /exec with bearer token auth (constant-time via `hmac.Equal()`)
- Expose POST /assign for one-time token delivery to warm-pool pods
- Expose GET /health as an unauthenticated readiness probe
- Implement agent state machine: unassigned (warm pool) → assigned (has token)
- Wire cmd/agent/main.go with HTTP server, signal handling, graceful shutdown
- Keep the handler as a thin HTTP layer — no business logic beyond parsing, auth, and serialization

**Non-Goals:**
- Container image (SANDBOX-1808)
- Unit tests (SANDBOX-1809)
- MCP server-side agent HTTP client (SANDBOX-1812)
- HMAC key derivation / per-session token computation (MCP server's responsibility)
- TLS (handled by kube-rbac-proxy on the MCP server side)
- Request logging or metrics (observability is a server-side concern)
- Content-Type validation or body size limits (network isolation + pod memory limits suffice)

## Decisions

### Decision 1: Mutex-protected AgentState struct
Use a struct with `sync.Mutex` grouping the `assigned` flag and `token` string together.

**Rationale:**
- The state machine has two coupled fields (assigned ↔ token non-empty) — a mutex groups them in one critical section
- Matches internal patterns (e.g. `VMShutdownTracker` with `RWMutex` in `mcp-server-devsandbox`)
- Negligible mutex cost for a per-pod agent serving one investigation

**Alternative considered:** Atomic operations (`atomic.Bool` + `atomic.Value`)
- Rejected — requires coordinating two atomic fields for a compound invariant; lock-free performance advantage is irrelevant at per-pod scale

**Alternative considered:** `sync.Once` for assignment
- Rejected — can't distinguish "first call" from "already called" without additional state

### Decision 2: `hmac.Equal()` for token comparison
Use `crypto/hmac.Equal()` for constant-time bearer token comparison.

**Rationale:**
- Returns clean `bool` (vs `subtle.ConstantTimeCompare` returning `int`)
- Functionally identical — `hmac.Equal()` is a thin wrapper around `subtle.ConstantTimeCompare()`
- Semantically appropriate since the token is HMAC-derived by the MCP server (`HMAC-SHA256(HMACKey, sessionID)`)

**Alternative considered:** `crypto/subtle.ConstantTimeCompare()`
- Rejected — awkward `int` return; no functional difference

### Decision 3: Handler struct with method handlers
Use a `Handler` struct holding `*BashSession` and `*AgentState`, with `HandleExec`, `HandleAssign`, `HandleHealth` as methods.

**Rationale:**
- Three handlers share the same two dependencies — a struct groups them once
- Easy to test: construct with real or mock dependencies
- Standard Go HTTP handler pattern

**Alternative considered:** Closures over shared state
- Rejected — would repeat `(session, state)` parameter lists three times; closures are better when handlers have different dependencies

### Decision 4: `net/http.Server` with `Shutdown()` for graceful drain
Use `http.Server.Shutdown()` with a 310-second timeout (MaxTimeout 300s + 10s buffer).

**Rationale:**
- In-flight `/exec` requests can run up to 300s and must drain before exit
- `Shutdown()` stops accepting new connections, closes idle keepalives, waits for active requests
- 310s ensures even the longest command finishes with buffer for HTTP overhead
- Kubernetes pod spec must set `terminationGracePeriodSeconds: 310` to match

**Alternative considered:** Bare `http.ListenAndServe` + immediate exit
- Rejected — kills in-flight `/exec` requests mid-execution

### Decision 5: Go 1.22+ method-based ServeMux routing
Use `mux.HandleFunc("POST /exec", ...)` instead of manual method checks.

**Rationale:**
- Repo is Go 1.24+; `use-modern-go` skill says to use modern idioms

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this design but let's switch to Go 1.25, as a separate task.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — tracked as a separate task. The Go 1.22+ routing features we reference here work fine on 1.24 and will continue to work on 1.25

- Automatic 405 Method Not Allowed for wrong methods — correct HTTP semantics for free
- Less boilerplate in handlers

**Alternative considered:** Plain paths with manual method checks
- Rejected — boilerplate in every handler, pre-1.22 pattern in a 1.24+ repo

### Decision 6: Timeout clamping in BashSession only
The handler converts `int` seconds to `time.Duration` and passes it through. BashSession applies defaults and caps internally (lines 135-139 of `bash.go`).

**Rationale:**
- BashSession already clamps correctly — no duplication needed
- No change to existing SANDBOX-1806 code
- Follows "parse at boundaries, trust internals" — BashSession is the authority on its own limits

**Alternative considered:** Handler clamps too (defense in depth)
- Rejected — duplicate logic, two places to update if limits change

### Decision 7: Handler lives in `pkg/sandbox/handler.go`
Co-located with `bash.go` since the handler is the sole consumer of `BashSession`.

**Rationale:**
- Matches the parent design doc, implementation plan, and stories
- Cross-package import on `pkg/agent` is a one-way dependency on pure data types
- Handler is ~100 lines — doesn't warrant its own package

### Decision 8: POST /assign returns 200 with empty body
Simple, consistent with design docs. No response body needed.

**Alternative considered:** 204 No Content
- Rejected — minor deviation from design docs with no practical benefit

### Decision 9: No input validation (Content-Type, body size)
Agent runs in an isolated pod network (NetworkPolicy restricts ingress to MCP server only). Pod memory limits (512Mi) are the backstop.

**Rationale:**
- The MCP server is our code sending tiny well-formed payloads
- Body size limits protect against a scenario requiring a bug in our own code, in an isolated network, with existing safeguards
- Simplicity first

## Risks / Trade-offs

**Risk:** `/assign` is unauthenticated — anyone who can reach the agent can deliver a token
→ **Mitigation:** NetworkPolicy restricts ingress to pods labeled `app: cli-mcp-server` only. The warm-pool pod has no token yet, so there's nothing to authenticate with.

**Risk:** 310s shutdown timeout means pod deletion takes up to ~5 minutes worst case
→ **Mitigation:** Only occurs during active investigation cleanup or node drain. The alternative (killing in-flight commands) is worse.

**Risk:** `writeError` ignores `json.Encode` error return
→ **Mitigation:** `ErrorResponse` is a simple struct that can't fail to serialize. Match `mcp-server-devsandbox` pattern with `_ = json.NewEncoder(w).Encode(...)` to satisfy `errcheck` linter.

**Trade-off:** Handler doesn't clamp timeout — relies entirely on BashSession
→ **Accepted:** BashSession already handles this correctly. A negative or zero timeout from the request is treated as "use default" by BashSession's `<= 0` check.

## Open Questions

None — all design decisions are resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
## Why

The sandbox agent has a working persistent bash session (SANDBOX-1806) but no way to receive commands from the network. The MCP server needs to send commands to the agent over HTTP, deliver auth tokens to warm-pool pods, and probe health for Kubernetes readiness. Without HTTP handlers and an entry point, the bash session is a library with no runtime — nothing starts it, nothing talks to it, nothing shuts it down.

## What Changes

- Add `pkg/sandbox/handler.go` with:
- `AgentState` struct (mutex-protected) tracking unassigned/assigned state and bearer token
- `Handler` struct with method handlers for three HTTP endpoints
- `HandleExec` — bearer token auth via `hmac.Equal()`, parses `ExecRequest`, calls `BashSession.Execute()`, maps `ExecResult` to `ExecResponse`
- `HandleAssign` — one-time token delivery for warm-pool pods, transitions state from unassigned to assigned
- `HandleHealth` — unauthenticated readiness probe, returns 200 if bash alive, 503 if dead
- `ErrorResponse` type and `writeError` helper for consistent JSON error responses
- Update `cmd/agent/main.go` with:
- `SANDBOX_AUTH_TOKEN` env var reading for initial state
- `BashSession` creation with eager initialization
- HTTP mux with Go 1.22+ method-based routing (`POST /exec`, `POST /assign`, `GET /health`)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered using some lightweight frameworks like https://github.com/labstack/echo (v. 5+)? This is what we use in tarsy, GC, reg-service. I don't know if it gives us anything in the agent context though. The agent needs are pretty minimal.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexeykazakov i did consider it — Echo is a good fit for services with rich routing, middleware stacks, and request binding (tarsy, reg-service). The sandbox agent has 3 endpoints, no middleware, and the stdlib net/http with Go 1.22+ method routing covers everything we need (automatic 405, no boilerplate method checks). Adding Echo would be a dependency with no functional benefit here. mcp-server-devsandbox also uses stdlib net/http for the same reason.
WDYT?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with sticking with plain Go for now.

- `net/http.Server` on `:8090` with graceful shutdown via `Shutdown()` (310s timeout)
- Signal handling for `SIGTERM`/`SIGINT` via `signal.NotifyContext()`

## Capabilities

### New Capabilities
- `agent-http-handlers`: Defines the HTTP endpoints (`POST /exec`, `POST /assign`, `GET /health`), authentication via bearer token with constant-time comparison, agent state machine (unassigned → assigned), and error response format
- `agent-entry-point`: Defines the agent binary entry point — HTTP server lifecycle, signal handling, graceful shutdown with 310s timeout coordinated with Kubernetes `terminationGracePeriodSeconds`

### Modified Capabilities
<!-- None — these build on SANDBOX-1806 without modifying it -->

## Impact

- **Affected code**:
- `pkg/sandbox/handler.go` — New file with AgentState, Handler, and HTTP endpoint implementations
- `cmd/agent/main.go` — Updated from scaffold placeholder to full agent entry point
- **API changes**: New HTTP API exposed on port 8090 (POST /exec, POST /assign, GET /health). No changes to existing Go APIs — `BashSession` and shared types from SANDBOX-1806 are consumed as-is.
- **Dependencies**: Depends on SANDBOX-1806 (shared types + BashSession). Depended on by SANDBOX-1808 (agent container image), SANDBOX-1809 (agent unit tests), SANDBOX-1812 (agent HTTP client).
- **No breaking changes** — `cmd/agent/main.go` is updated from a placeholder, all other code is new.
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
## ADDED Requirements

### Requirement: Agent reads initial state from environment
The agent binary SHALL read `SANDBOX_AUTH_TOKEN` from the environment to determine its initial state.

#### Scenario: Token present starts agent in assigned state
- **WHEN** `SANDBOX_AUTH_TOKEN` environment variable is set to a non-empty value
- **THEN** the agent SHALL start with `AgentState` in assigned mode and log "starting in assigned state"

#### Scenario: Token absent starts agent in unassigned state
- **WHEN** `SANDBOX_AUTH_TOKEN` environment variable is empty or not set
- **THEN** the agent SHALL start with `AgentState` in unassigned mode and log "starting in unassigned state (warm pool)"

### Requirement: Agent creates BashSession eagerly
The agent SHALL create a `BashSession` at startup using `NewBashSession(NewDefaultBashConfig())` and exit fatally if bash fails to start.

#### Scenario: Bash starts successfully
- **WHEN** the agent starts and bash is available in PATH
- **THEN** a BashSession SHALL be created and the agent SHALL proceed to start the HTTP server

#### Scenario: Bash unavailable causes fatal exit
- **WHEN** the agent starts but bash is not available
- **THEN** the agent SHALL log the error and exit with a non-zero status code

### Requirement: Agent starts HTTP server on port 8090
The agent SHALL start an `http.Server` listening on `:8090` with routes registered using Go 1.22+ method-based routing.

#### Scenario: Server starts and listens
- **WHEN** the agent starts successfully
- **THEN** an HTTP server SHALL listen on `:8090` and log "listening on :8090"

#### Scenario: Routes are registered with method patterns
- **WHEN** the HTTP mux is configured
- **THEN** it SHALL register `"POST /exec"`, `"POST /assign"`, and `"GET /health"` using Go 1.22+ method-based patterns

### Requirement: Agent shuts down gracefully on SIGTERM
The agent SHALL handle `SIGTERM` and `SIGINT` signals, drain in-flight HTTP requests via `http.Server.Shutdown()`, and close the BashSession before exiting.

#### Scenario: SIGTERM triggers graceful shutdown
- **WHEN** the agent receives SIGTERM
- **THEN** it SHALL call `srv.Shutdown()` with a 310-second timeout context, then call `session.Close()`, then exit 0

#### Scenario: In-flight exec request completes during shutdown
- **WHEN** SIGTERM is received while a POST /exec request is in progress
- **THEN** the server SHALL wait for the in-flight request to complete (up to 310 seconds) before closing

#### Scenario: Shutdown timeout exceeded
- **WHEN** `srv.Shutdown()` exceeds the 310-second timeout
- **THEN** the agent SHALL log the shutdown error, close the BashSession, and exit

#### Scenario: SIGINT also triggers shutdown
- **WHEN** the agent receives SIGINT
- **THEN** it SHALL follow the same graceful shutdown sequence as SIGTERM

### Requirement: Shutdown timeout accommodates maximum command duration
The shutdown timeout SHALL be 310 seconds (`MaxTimeout` 300s + 10s buffer) to ensure the longest possible in-flight command completes.

#### Scenario: Kubernetes terminationGracePeriodSeconds coordination
- **WHEN** the agent pod spec is configured
- **THEN** `terminationGracePeriodSeconds` SHALL be set to 310 to match the agent's shutdown timeout

### Requirement: Agent logs version at startup
The agent SHALL print version information to stderr at startup.

#### Scenario: Version logged on start
- **WHEN** the agent starts
- **THEN** it SHALL print `sandbox-agent <commit> (built <time>)` to stderr using the version package
Loading
Loading