diff --git a/openspec/changes/2026-07-13-bash-mcp-tool-handler/.openspec.yaml b/openspec/changes/2026-07-13-bash-mcp-tool-handler/.openspec.yaml new file mode 100644 index 0000000..b119b63 --- /dev/null +++ b/openspec/changes/2026-07-13-bash-mcp-tool-handler/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-13 diff --git a/openspec/changes/2026-07-13-bash-mcp-tool-handler/design.md b/openspec/changes/2026-07-13-bash-mcp-tool-handler/design.md new file mode 100644 index 0000000..e76e412 --- /dev/null +++ b/openspec/changes/2026-07-13-bash-mcp-tool-handler/design.md @@ -0,0 +1,192 @@ +## Context + +The CLI MCP Server architecture has two binaries: the MCP server (control plane) and the sandbox agent (data plane). SANDBOX-1806/1807 delivered the agent. SANDBOX-1810 delivered session management (`GetOrCreatePod`, `ExecuteCommand`). SANDBOX-1812 delivered the typed agent HTTP client. + +What is missing is the MCP tool surface: a `bash` tool that TARSy can call via the standard MCP `tools/call` interface. The parent design doc (`docs/proposals/cli-mcp-server-design.md`) already specifies the handler shape, input/output types, timeout clamping, and the critical error-model distinction between command failures and infrastructure failures. This change makes that design concrete in `pkg/tools`. + +Session identity is **not** a tool parameter. TARSy injects `X-Session-ID` on every HTTP request; the MCP SDK populates `req.Extra.Header` from the transport. The handler reads that header and rejects the call if it is missing. + +## Goals / Non-Goals + +**Goals:** +- `bash` MCP tool registered via `mcp.AddTool` with typed `BashInput` / `BashOutput` +- Extract `X-Session-ID` from `req.Extra.Header`; reject with a clear error if missing or if `req.Extra` is nil +- Clamp timeout: default 60s when omitted/non-positive; max 300s +- Call `CommandExecutor.ExecuteCommand` (satisfied by `*session.SessionManager`) +- Non-zero exit codes: return `CallToolResult{IsError: true}` **with** `BashOutput` still populated (stdout/stderr/exit_code/duration_ms) +- Infrastructure failures (pod resolve/create, agent unreachable, etc.): return a Go error from the handler (SDK packs as `IsError` tool result **without** structured `BashOutput`) +- Reject empty `command` with a clear validation error +- Unit tests with a mock `CommandExecutor` — no cluster or HTTP required + +**Non-Goals:** +- MCP server bootstrap, middleware, HTTP mux, Cobra flags (SANDBOX-1814) +- `DELETE /sessions/{id}`, `/health`, `/metrics` endpoints (SANDBOX-1814) +- Refactoring `SessionManager.ExecuteCommand` to use `AgentClient` (separate cleanup; not required for the tool to work) +- Output truncation, masking, or summarization (TARSy's responsibility per design doc) +- Command allowlisting / read-only enforcement (sandbox is intentionally full bash) +- Warm-pool or session-manager behavior changes + +## Decisions + +### Decision 1: Place the tool in `pkg/tools/` and follow the mcp-server-devsandbox registration pattern + +```go +type BashTool struct { + executor CommandExecutor + tool *mcp.Tool +} + +func (t *BashTool) RegisterWith(s *mcp.Server) { + mcp.AddTool(s, t.tool, t.handle) +} +``` + +**Rationale:** +- Matches `mcp-server-devsandbox/pkg/tools` (`RegisterWith` + `mcp.AddTool`) +- `mcp.AddTool` derives input/output JSON Schema from Go struct tags automatically +- Keeps tool code out of `pkg/session` and `pkg/server` — session manager stays infrastructure; tools stay MCP-facing +- SANDBOX-1814 will construct `NewBashTool(sessionManager)` and call `RegisterWith(server)` + +**Alternative considered:** Inline registration inside `pkg/server` +- Rejected — couples server bootstrap to tool logic and makes unit-testing the handler harder + +### Decision 2: Narrow `CommandExecutor` interface instead of depending on `*session.SessionManager` + +```go +type CommandExecutor interface { + ExecuteCommand(ctx context.Context, sessionID, command string, timeoutSec int) (*agent.ExecResponse, error) +} +``` + +**Rationale:** +- `*session.SessionManager` already implements this method — no adapter needed at wiring time +- Tests inject a mock without pulling in client-go fakes or HTTP servers +- Matches the implementation plan note: "mock SessionManager interface" +- Avoids an import cycle risk if `pkg/session` ever needed to reference tools (it should not) + +**Alternative considered:** Depend directly on `*session.SessionManager` +- Rejected — harder to unit-test; couples tools package to K8s session machinery + +### Decision 3: Use typed `mcp.AddTool` — infrastructure failures are tool `IsError`, not protocol errors (option A) + +The design-doc pseudocode uses a low-level handler that returns `(*CallToolResult, error)`, where a Go error becomes a **JSON-RPC protocol** error. The modern go-sdk `mcp.AddTool` path treats a regular Go error as a **tool** error (`IsError: true` with error text in content), and only `*jsonrpc.Error` becomes a protocol error. + +We deliberately choose **option A**: infrastructure/validation failures return a normal Go error from the typed handler, which the SDK packs as a tool-level `IsError` result (no structured `BashOutput`). We do **not** escalate them to JSON-RPC protocol errors. + +**Chosen mapping (AC-aligned):** + +| Scenario | Handler return | Client observation | +|---|---|---| +| Exit code 0 | `(nil, BashOutput{...}, nil)` | Success; structured `BashOutput` in content | +| Exit code ≠ 0 | `(&CallToolResult{IsError: true}, BashOutput{...}, nil)` | Tool error **with** structured output (useful for LLM) | +| Missing `X-Session-ID`, empty command, `ExecuteCommand` error | `(nil, BashOutput{}, fmt.Errorf(...))` | Tool error **without** structured `BashOutput` — infrastructure/validation failure | + +**Rationale:** +- Preserves the AC distinction: command failures still return stdout/stderr; infrastructure failures do not fabricate command output +- Matches mcp-server-devsandbox conventions (`AddTool` + typed args/results + `fmt.Errorf` → tool `IsError`) +- Same failure shape TARSy already handles for other MCP tools (“tool said it failed”) rather than a harsher protocol-error path +- SDK auto-populates `Content` with JSON text of `BashOutput` when `Content` is unset — satisfies "JSON in `mcp.TextContent`" +- Avoids low-level schema boilerplate + +**Alternative considered (option B):** Low-level `Server.AddTool` or return `*jsonrpc.Error` so infrastructure failures are JSON-RPC protocol errors +- **Rejected** — protocol errors deviate from sibling MCP servers and force a different TARSy error path. Option A meets the acceptance criteria without that complexity. + +### Decision 4: Session ID from `X-Session-ID` header only — never a tool parameter + +```go +if req.Extra == nil || req.Extra.Header.Get("X-Session-ID") == "" { + return nil, BashOutput{}, fmt.Errorf("missing X-Session-ID header") +} +sessionID := req.Extra.Header.Get("X-Session-ID") +``` + +**Rationale:** +- Parent design decision Q1: LLM never manages session IDs — eliminates hallucination/typo class of errors +- SDK `RequestExtra.Header` is populated by `StreamableHTTPHandler` from the incoming HTTP request +- Nil-safe check on `req.Extra` covers unit tests and non-HTTP transports + +### Decision 5: Timeout clamping — default 60, max 300 + +```go +timeout := 60 +if input.Timeout != nil && *input.Timeout > 0 { + timeout = *input.Timeout + if timeout > 300 { + timeout = 300 + } +} +``` + +**Rationale:** +- Matches design doc and acceptance criteria exactly +- Omitted, null, zero, or negative timeout → default 60 (pointer omitempty + `> 0` check) +- Values above 300 are clamped, not rejected — avoids LLM retries for "timeout too large" +- Passed through to `ExecuteCommand` / agent as seconds + +### Decision 6: Empty command is a validation error + +Reject `command == ""` (after trim? **no trim of intentional whitespace-only — treat empty string only**) with `fmt.Errorf("command is required")`. + +**Rationale:** +- Schema marks `command` as required, but explicit check gives a clear error if schema validation is bypassed in tests +- Whitespace-only commands are valid bash (no-ops) and are left to the agent — do not over-validate + +### Decision 7: Tool annotations — not read-only + +```go +Annotations: &mcp.ToolAnnotations{ + // DestructiveHint omitted / false — full bash can mutate workspace state + ReadOnlyHint: false, +} +``` + +**Rationale:** +- Sandbox bash is intentionally unrestricted (pipes, redirects, package installs in workspace, etc.) +- Marking `ReadOnlyHint: true` would mislead clients/LLMs +- No allowlist (unlike `vm-ssh` in mcp-server-devsandbox) + +### Decision 8: Add go-sdk dependency at the version used by mcp-server-devsandbox + +Pin `github.com/modelcontextprotocol/go-sdk` to **v1.4.x** (same major/minor family as mcp-server-devsandbox's current require). Exact patch resolved by `go get` at implementation time. + +**Rationale:** +- Tool registration requires the SDK; `go.mod` does not include it yet +- Aligning versions reduces surprise when SANDBOX-1814 also pulls mcp-common middleware that may transitively depend on the SDK + +### Decision 9: `BashOutput` field names match `agent.ExecResponse` JSON tags + +```go +type BashOutput struct { + Stdout string `json:"stdout"` + Stderr string `json:"stderr"` + ExitCode int `json:"exit_code"` + DurationMs int64 `json:"duration_ms"` +} +``` + +Map 1:1 from `*agent.ExecResponse`. Do not transform or truncate output. + +**Rationale:** +- Stable contract for TARSy / LLM parsing +- Design doc: output handling is as-is; masking/summarization is TARSy's job + +## Risks / Trade-offs + +**Risk:** Design-doc pseudocode showed infrastructure failures as protocol errors; option A surfaces them as tool `IsError` instead. +→ **Mitigation:** Accepted and documented in Decision 3. Clients still see a clear failure without fake command output, consistent with mcp-server-devsandbox. + +**Risk:** `req.Extra` may be nil when the tool is invoked outside Streamable HTTP (e.g., future stdio transport or direct unit invocation). +→ **Mitigation:** Explicit nil check; error message identical to missing header case. + +**Risk:** Adding go-sdk pulls transitive deps (`jsonschema-go`, etc.) into the module before the server binary exists. +→ **Accepted:** Required for compiling `pkg/tools`. SANDBOX-1814 will use the same dependency. + +**Trade-off:** Unit tests in this story vs deferring all tests to SANDBOX-1816. +→ **Accepted:** Follow SANDBOX-1810/1811/1812 pattern — each story ships its own focused unit tests. SANDBOX-1816 remains the broad coverage story. + +**Trade-off:** Not refactoring session manager onto `AgentClient` in this story. +→ **Accepted:** Tool only needs `ExecuteCommand`. Refactor is mechanical and can land independently without blocking TARSy tool wiring. + +## Open Questions + +None — Decision 3 is locked to option A (tool `IsError` for infrastructure/validation failures, not JSON-RPC protocol errors). diff --git a/openspec/changes/2026-07-13-bash-mcp-tool-handler/proposal.md b/openspec/changes/2026-07-13-bash-mcp-tool-handler/proposal.md new file mode 100644 index 0000000..133015d --- /dev/null +++ b/openspec/changes/2026-07-13-bash-mcp-tool-handler/proposal.md @@ -0,0 +1,40 @@ +## Why + +The session manager (`pkg/session`) can resolve a sandbox pod and proxy commands to the agent, and the typed agent HTTP client (`pkg/agent`) is in place — but nothing yet exposes that capability as an MCP tool. TARSy investigation agents need a standard `tools/call` entry point that accepts a shell command, routes it to the correct session via the `X-Session-ID` header, and returns structured stdout/stderr/exit-code output the LLM can reason over. + +SANDBOX-1813 introduces `pkg/tools/bash.go` — the `bash` MCP tool handler. It extracts the session ID from request headers (never from tool params), clamps timeouts, calls `SessionManager.ExecuteCommand`, and maps the agent response to `BashOutput` JSON. Non-zero command exit codes are returned as tool results with `IsError: true` (still carrying output); only infrastructure failures (missing session header, pod creation failure, agent unreachable) surface as handler errors. + +SANDBOX-1810 delivered session routing and `ExecuteCommand`. SANDBOX-1812 delivered the agent HTTP client the session manager uses. This story is the MCP-facing glue between those layers and TARSy. + +## What Changes + +- Add `pkg/tools/bash.go` with: + - `BashInput` / `BashOutput` types matching the design doc wire contract + - `CommandExecutor` interface (narrow surface over `SessionManager.ExecuteCommand`) for testability + - `BashTool` struct holding the executor and MCP tool definition + - `NewBashTool(executor CommandExecutor)` constructor + - `RegisterWith(s *mcp.Server)` — registers via `mcp.AddTool` (typed handler, auto schema) + - Handler logic: header extraction → timeout clamp → execute → map result / errors + +- Add `pkg/tools/bash_test.go` with unit tests against a mock `CommandExecutor` + +- Add `github.com/modelcontextprotocol/go-sdk` dependency (required for tool registration; not yet in `go.mod`) + +## Capabilities + +### New Capabilities +- `bash-tool`: MCP `bash` tool that routes shell commands to session-scoped sandbox pods via `X-Session-ID`, returns structured `BashOutput`, and distinguishes command failures (`IsError` + output) from infrastructure failures (handler error) + +### Modified Capabilities + + +## Impact + +- **Affected code**: + - `pkg/tools/bash.go` — New file with bash tool handler and registration + - `pkg/tools/bash_test.go` — New file with unit tests + - `go.mod` / `go.sum` — Add `github.com/modelcontextprotocol/go-sdk` (aligned with mcp-server-devsandbox at v1.4.x) +- **API changes**: New Go API in `pkg/tools`. The `bash` MCP tool becomes registrable; actual server wiring is SANDBOX-1814. +- **Dependencies**: Depends on SANDBOX-1810 (`SessionManager.ExecuteCommand`) and SANDBOX-1812 (`agent.ExecResponse` types). Depended on by SANDBOX-1814 (MCP server entry point registers the tool). +- **No breaking changes** — entirely new package. Existing session/agent code is not modified. +- **Out of scope**: MCP server bootstrap, HTTP mux, Cobra CLI (SANDBOX-1814); refactoring `manager.go` to use `AgentClient` (optional follow-up noted in SANDBOX-1812); full server-side unit-test suite beyond bash tool tests (SANDBOX-1816 expands coverage). diff --git a/openspec/changes/2026-07-13-bash-mcp-tool-handler/specs/bash-tool/spec.md b/openspec/changes/2026-07-13-bash-mcp-tool-handler/specs/bash-tool/spec.md new file mode 100644 index 0000000..e7ad392 --- /dev/null +++ b/openspec/changes/2026-07-13-bash-mcp-tool-handler/specs/bash-tool/spec.md @@ -0,0 +1,128 @@ +## ADDED Requirements + +### Requirement: bash tool is registered with typed input and output schemas + +The `bash` MCP tool SHALL be registerable on an `mcp.Server` via `BashTool.RegisterWith`, using `mcp.AddTool` with typed `BashInput` and `BashOutput` so input/output JSON Schemas are derived from Go types. + +#### Scenario: Tool name and description +- **WHEN** `NewBashTool` is constructed +- **THEN** the tool name SHALL be `"bash"` +- **AND** the tool description SHALL indicate that it executes a shell command in a persistent sandbox bash session (pipes, redirects, and chaining supported) +- **AND** `ReadOnlyHint` SHALL be false (or unset / not true) + +#### Scenario: BashInput schema fields +- **WHEN** the tool's input schema is derived from `BashInput` +- **THEN** it SHALL include required field `command` (string) +- **AND** it SHALL include optional field `timeout` (integer, seconds) +- **AND** it SHALL NOT include a session ID field (session identity is header-only) + +#### Scenario: BashOutput schema fields +- **WHEN** the tool returns structured output +- **THEN** `BashOutput` SHALL serialize with JSON fields `stdout`, `stderr`, `exit_code`, and `duration_ms` + +#### Scenario: RegisterWith adds the tool to the server +- **WHEN** `RegisterWith` is called with an `mcp.Server` +- **THEN** the server SHALL expose a tool named `"bash"` that dispatches to the bash handler + +### Requirement: Session ID is extracted from the X-Session-ID HTTP header + +The handler SHALL read the session ID exclusively from `req.Extra.Header.Get("X-Session-ID")` and SHALL reject requests that lack it. + +#### Scenario: Successful header extraction +- **WHEN** a tools/call request includes `X-Session-ID: inv-abc123` in `req.Extra.Header` +- **THEN** the handler SHALL use `inv-abc123` as the session ID passed to `ExecuteCommand` + +#### Scenario: Missing X-Session-ID header +- **WHEN** `req.Extra` is nil +- **OR** `req.Extra.Header.Get("X-Session-ID")` is empty +- **THEN** the handler SHALL return an error whose message includes `missing X-Session-ID header` +- **AND** it SHALL NOT call `ExecuteCommand` + +#### Scenario: Session ID is not a tool parameter +- **WHEN** the LLM supplies tool arguments +- **THEN** there SHALL be no `session_id` (or equivalent) field in `BashInput` +- **AND** any session-like value in arguments SHALL be ignored for routing purposes + +### Requirement: Timeout is clamped to default 60s and maximum 300s + +The handler SHALL normalize the optional `timeout` argument before calling `ExecuteCommand`. + +#### Scenario: Default timeout when omitted +- **WHEN** `timeout` is omitted (nil) +- **THEN** `ExecuteCommand` SHALL be called with `timeoutSec == 60` + +#### Scenario: Default timeout when non-positive +- **WHEN** `timeout` is present and less than or equal to 0 +- **THEN** `ExecuteCommand` SHALL be called with `timeoutSec == 60` + +#### Scenario: Timeout within range is passed through +- **WHEN** `timeout` is present and in the range 1..300 inclusive +- **THEN** `ExecuteCommand` SHALL be called with that exact value + +#### Scenario: Timeout above maximum is clamped +- **WHEN** `timeout` is present and greater than 300 +- **THEN** `ExecuteCommand` SHALL be called with `timeoutSec == 300` +- **AND** the handler SHALL NOT return an error solely because the requested timeout exceeded the max + +### Requirement: Successful command execution returns structured BashOutput + +On successful agent execution (including non-zero exit codes), the handler SHALL map `*agent.ExecResponse` to `BashOutput` without truncating or transforming stdout/stderr. + +#### Scenario: Zero exit code is a successful tool result +- **WHEN** `ExecuteCommand` returns an `ExecResponse` with `ExitCode == 0` +- **THEN** the handler SHALL return no error +- **AND** the result SHALL include `BashOutput` with matching `stdout`, `stderr`, `exit_code`, and `duration_ms` +- **AND** `CallToolResult.IsError` SHALL be false (or unset) + +#### Scenario: Non-zero exit code sets IsError but still returns output +- **WHEN** `ExecuteCommand` returns an `ExecResponse` with `ExitCode != 0` +- **THEN** the handler SHALL return no Go error +- **AND** `CallToolResult.IsError` SHALL be true +- **AND** the structured `BashOutput` SHALL still contain the command's `stdout`, `stderr`, `exit_code`, and `duration_ms` +- **AND** the text content SHALL include the JSON serialization of that `BashOutput` (via SDK population of `Content` when unset) + +#### Scenario: Output is returned as-is +- **WHEN** the agent returns stdout/stderr of any length within agent client limits +- **THEN** the handler SHALL NOT truncate, mask, or rewrite those fields + +### Requirement: Infrastructure and validation failures return handler errors without BashOutput + +Only infrastructure and validation failures SHALL cause the handler to return a Go error. Those failures MUST NOT be presented as a successful command result with fabricated exit codes. + +#### Scenario: ExecuteCommand failure is an infrastructure error +- **WHEN** `ExecuteCommand` returns an error (pod creation failed, agent unreachable, etc.) +- **THEN** the handler SHALL return a Go error wrapping or including that failure (message SHOULD include context such as `sandbox exec failed`) +- **AND** it SHALL NOT return a populated success `BashOutput` for that call + +#### Scenario: Empty command is rejected +- **WHEN** `command` is the empty string +- **THEN** the handler SHALL return an error indicating the command is required +- **AND** it SHALL NOT call `ExecuteCommand` + +#### Scenario: Command failure vs infrastructure failure are distinguishable +- **WHEN** a command exits non-zero +- **THEN** the client receives structured `BashOutput` with `IsError: true` +- **WHEN** infrastructure fails +- **THEN** the client receives a tool error without structured `BashOutput` command fields as a successful mapping of agent output + +### Requirement: Handler depends on CommandExecutor for testability + +The bash tool SHALL accept a `CommandExecutor` interface rather than a concrete `*session.SessionManager`, so unit tests can mock execution. + +#### Scenario: SessionManager satisfies CommandExecutor +- **WHEN** the server is wired in SANDBOX-1814 +- **THEN** `*session.SessionManager` SHALL be usable as the `CommandExecutor` without an adapter (method signature compatible) + +#### Scenario: Unit tests use a mock executor +- **WHEN** `pkg/tools/bash_test.go` runs +- **THEN** tests SHALL exercise the handler with a mock `CommandExecutor` +- **AND** tests SHALL NOT require a real Kubernetes cluster or live agent HTTP server + +### Requirement: go-sdk dependency is available for compilation + +The module SHALL require `github.com/modelcontextprotocol/go-sdk` so `pkg/tools` compiles. + +#### Scenario: Module builds with tools package +- **WHEN** `go test ./pkg/tools/...` is run after implementation +- **THEN** the package SHALL compile and tests SHALL pass +- **AND** the go-sdk version SHALL be in the v1.4.x family aligned with mcp-server-devsandbox diff --git a/openspec/changes/2026-07-13-bash-mcp-tool-handler/tasks.md b/openspec/changes/2026-07-13-bash-mcp-tool-handler/tasks.md new file mode 100644 index 0000000..5805e11 --- /dev/null +++ b/openspec/changes/2026-07-13-bash-mcp-tool-handler/tasks.md @@ -0,0 +1,53 @@ +## 1. Package skeleton and types + +- [ ] 1.1 Create `pkg/tools/bash.go` with package `tools` +- [ ] 1.2 Define `BashInput` struct: `Command string` (`json:"command"`, jsonschema required + description), `Timeout *int` (`json:"timeout,omitempty"`, jsonschema description default 60 max 300) +- [ ] 1.3 Define `BashOutput` struct: `Stdout`, `Stderr` string; `ExitCode` int (`json:"exit_code"`); `DurationMs` int64 (`json:"duration_ms"`) +- [ ] 1.4 Define `CommandExecutor` interface with `ExecuteCommand(ctx context.Context, sessionID, command string, timeoutSec int) (*agent.ExecResponse, error)` +- [ ] 1.5 Define `BashTool` struct holding `executor CommandExecutor` and `tool *mcp.Tool` + +## 2. Constructor and registration + +- [ ] 2.1 Implement `NewBashTool(executor CommandExecutor) *BashTool` — panics or returns clearly if executor is nil is optional; prefer simple assignment (caller responsibility) +- [ ] 2.2 Construct `mcp.Tool` with `Name: "bash"`, descriptive `Description`, and `Annotations` with `ReadOnlyHint: false` +- [ ] 2.3 Implement `RegisterWith(s *mcp.Server)` calling `mcp.AddTool(s, t.tool, t.handle)` +- [ ] 2.4 Add `github.com/modelcontextprotocol/go-sdk` v1.4.x to `go.mod` via `go get` + +## 3. Handler — validation and session routing + +- [ ] 3.1 Implement `handle(ctx context.Context, req *mcp.CallToolRequest, input BashInput) (*mcp.CallToolResult, BashOutput, error)` +- [ ] 3.2 If `req.Extra == nil` or `X-Session-ID` header is empty, return error `"missing X-Session-ID header"` +- [ ] 3.3 If `input.Command == ""`, return error indicating command is required +- [ ] 3.4 Clamp timeout: default 60 if nil or `<= 0`; otherwise `min(timeout, 300)` + +## 4. Handler — execution and result mapping + +- [ ] 4.1 Call `executor.ExecuteCommand(ctx, sessionID, input.Command, timeout)` +- [ ] 4.2 On executor error, return `fmt.Errorf("sandbox exec failed: %w", err)` (no BashOutput payload) +- [ ] 4.3 On success, map `ExecResponse` → `BashOutput` field-for-field +- [ ] 4.4 If `ExitCode == 0`, return `(nil, output, nil)` +- [ ] 4.5 If `ExitCode != 0`, return `(&mcp.CallToolResult{IsError: true}, output, nil)` + +## 5. Unit tests + +- [ ] 5.1 Create `pkg/tools/bash_test.go` with a mock `CommandExecutor` (func field or testify mock) +- [ ] 5.2 Test: missing `X-Session-ID` (nil Extra and empty header) → error, executor not called +- [ ] 5.3 Test: empty command → error, executor not called +- [ ] 5.4 Test: omitted timeout → executor called with 60 +- [ ] 5.5 Test: timeout 0 / negative → executor called with 60 +- [ ] 5.6 Test: timeout 120 → executor called with 120 +- [ ] 5.7 Test: timeout 500 → executor called with 300 +- [ ] 5.8 Test: successful exit 0 → `IsError` false/nil result, BashOutput fields match +- [ ] 5.9 Test: exit code 1 → `CallToolResult.IsError == true`, BashOutput still populated, no Go error +- [ ] 5.10 Test: executor returns error → Go error contains `sandbox exec failed`, no success output mapping +- [ ] 5.11 Test: session ID from header is forwarded to executor unchanged +- [ ] 5.12 Test: `RegisterWith` registers a tool named `bash` on a new `mcp.Server` (list tools or equivalent) +- [ ] 5.13 Run `go test ./pkg/tools/...` and ensure pass + +## 6. Verification against acceptance criteria + +- [ ] 6.1 Confirm AC: extracts `X-Session-ID`; rejects if missing +- [ ] 6.2 Confirm AC: timeout default 60, max 300 +- [ ] 6.3 Confirm AC: non-zero exit → `IsError: true` with output retained +- [ ] 6.4 Confirm AC: only infrastructure/validation failures return handler errors +- [ ] 6.5 Confirm AC: structured `BashOutput` JSON available via SDK text/structured content