mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
## What Live MCP servers and their tools now flow into the `agentcontext` snapshot and are pushed to coderd via `PushContextState`, stored alongside instruction files and skills. Previously the resolver's MCP seam was unimplemented, so live MCP tool lists never reached the pushed snapshot. `agentcontext` is now **fully self-contained** for MCP: it connects to the MCP servers declared in the `.mcp.json` files its own watcher already discovers, lists their tools, and emits `KindMCPServer` resources. It does **not** depend on or modify `agent/x/agentmcp` — that package is left pristine and keeps serving the agent's MCP HTTP API. The two MCP paths run independently, which means the legacy package can be deleted later without touching this code. ## How - **Self-contained runner** (`agentcontext/mcprunner.go`): a one-shot MCP client (connect → initialize → list tools → close) with its own `.mcp.json` parser. A Manager goroutine (`runMCPSync`) reloads it whenever the discovered `KindMCPConfig` `path:contenthash` set changes, then re-resolves so the new tools are published. Per-server connects run in parallel (bounded) with a per-server timeout; a server that fails to connect is recorded as a failure rather than aborting the batch. Each connect also force-kills its subprocess on close, because mcp-go's stdio `Close()` closes stdin and then blocks on `cmd.Wait()` with no kill — a server that ignores stdin-close would otherwise stall the whole reload loop. - **Resource production** (`agentcontext/mcp.go`): `buildMCPServerResources` turns the runner's non-blocking per-server snapshot into `KindMCPServer` resources. Connected servers carry their sorted tools (`StatusOK`); failed servers surface as `StatusUnreadable` issues instead of vanishing; connected-but-no-tools-yet are skipped until a later reload. The content hash is tool-set sensitive. The resolver consumes this through a plain `MCPResources func() []Resource` field (no `MCPProvider` interface). - **Tool names**: emitted exactly as the server reports them. Flattening into a single namespace (e.g. `server__tool`) is left to the control plane in the next step, since each resource already carries the server name. - **Drift**: MCP resources are excluded from the snapshot aggregate/drift hash (`driftResources`). MCP servers connect asynchronously after boot; without this, a server finishing its connect would dirty every hydrated chat even though nothing the user pinned changed. - **Wiring** (`agent.go`): the manager is given `ManagerOptions.MCPExecer`/`MCPUpdateEnv`; `agent/x/agentmcp` is untouched. - **Config validation**: a structurally broken `.mcp.json` surfaces as `StatusInvalid` rather than silently dropping all its servers. coderd already persists `mcp_server`/`mcp_config` resource bodies (including tools), so no coderd or proto changes were required. ## Testing - **Unit**: `buildMCPServerResources` (grouping/sort/skip/failed/hash sensitivity), MCP resources applied via the resolver seam, MCP exclusion from the aggregate hash, `.mcp.json` parsing (transport inference, env expansion), `toolInputSchema`, and `mcpConfigSet` change detection. - **Proto serialization** (`TestDRPCPusher_HappyPathSerializesAllFields`): a `KindMCPServer` resource (tools + input schema) round-trips through `PushContextState` into the `MCPServerBody` wire form, asserting the server name, tool name/description, and the decoded `input_schema`. - **Manager-level, real subprocess** (`TestManager_MCPServerToolsInSnapshot`): a `.mcp.json` points at a re-exec'd fake stdio MCP server; the runner connects it and its `echo` tool surfaces as a `KindMCPServer` resource in the Manager snapshot — the same snapshot pushed to coderd — exercising `runMCPSync` and the resolver wiring end to end. - **Regression** (`TestManager_MCPServerHangingCloseDoesNotStall`): the fake server ignores stdin-close; the test asserts its tool still surfaces, proving the runner force-kills the subprocess instead of stalling the reload. Verified to fail without the fix. - All pass under `-race`; `go build ./...`, `go vet`, and `golangci-lint` are clean on the touched packages. ## Scope / follow-ups This is the agent-side production+push half. The chatd consumer (reading the pinned MCP resources for prompt/tool injection, including any server-prefix flattening of tool names) and removing the legacy `workspaceMCPToolsCache` pull path remain follow-ups, per the RFC rollout. While both `agent/x/agentmcp` and `agentcontext` exist, stdio MCP servers are spawned by both; this is intentional and temporary until `agentmcp` is removed. <details> <summary>Implementation plan and decisions</summary> **Goal:** produce live MCP server resources (with tools) from `agentcontext` and push them to coderd. **Starting state (main):** proto (`PushContextState`, `MCPServerBody`, `MCPTool`), the drpc adapter, coderd storage (`workspace_agent_context_resources`, body kind `mcp_server`), and the resolver's MCP seam already existed; nothing implemented the seam or fed live tools into the snapshot. **Decision (agentcontext fully separate from agentmcp):** `agentcontext` starts and lists its own MCP servers using only the connect-and-list half of an mcp-go client, driven by the `.mcp.json` files its existing watcher discovers. It shares no state with `agent/x/agentmcp` and does not import it. Two earlier revisions of this branch were discarded: (1) relocating `agentmcp` into `agentcontext` (rejected — it duplicates config parsing and file watching `agentcontext` already does); (2) reading `agentmcp`'s cached server snapshot via new accessors (rejected — unnecessary coupling between two packages that should simply run independently while one is being retired). The temporary double-spawn of stdio servers is the accepted cost of keeping the two paths cleanly separated until `agentmcp` is removed. **Decision (no tool-name prefixing, no MCPProvider interface):** the agent pushes raw, unflattened data — server name plus verbatim tool names — and lets the control plane own any `server__tool` flattening. With a single self-contained producer, the `MCPProvider` interface was collapsed into a `func() []Resource` field on the resolver. **Invariants held:** no secrets (env/headers) in pushed resources, only server/tool metadata; MCP excluded from the drift hash; the seam is non-blocking so the resolver never stalls on MCP I/O. </details> --- *This PR was created by Coder Agents on behalf of @kylecarbs.*
190 lines
5.8 KiB
Go
190 lines
5.8 KiB
Go
package agentcontext_test
|
|
|
|
import (
|
|
"bufio"
|
|
"encoding/json"
|
|
"fmt"
|
|
"os"
|
|
"path/filepath"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/require"
|
|
|
|
"github.com/coder/coder/v2/agent/agentcontext"
|
|
"github.com/coder/coder/v2/agent/agentexec"
|
|
"github.com/coder/coder/v2/testutil"
|
|
)
|
|
|
|
// TestManager_MCPServerToolsInSnapshot exercises the MCP runner end to
|
|
// end against a real subprocess: a .mcp.json in the working directory is
|
|
// discovered by the resolver, the runner connects the declared stdio
|
|
// server, lists its tools, and they surface as a KindMCPServer resource
|
|
// in the manager's snapshot (the same snapshot that is pushed to coderd).
|
|
func TestManager_MCPServerToolsInSnapshot(t *testing.T) {
|
|
t.Parallel()
|
|
dir := t.TempDir()
|
|
writeMCPConfig(t, dir, "fake", map[string]string{"TEST_MCP_FAKE_SERVER": "1"})
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return dir },
|
|
MCPExecer: agentexec.DefaultExecer,
|
|
})
|
|
ctx := testutil.Context(t, testutil.WaitLong)
|
|
go func() { _ = m.Run(ctx) }()
|
|
|
|
require.Eventually(t, func() bool {
|
|
return findMCPServer(m.Snapshot(), "fake") != nil
|
|
}, testutil.WaitLong, testutil.IntervalMedium,
|
|
"the connected MCP server's tools should surface in the snapshot")
|
|
|
|
got := findMCPServer(m.Snapshot(), "fake")
|
|
require.NotNil(t, got)
|
|
require.Equal(t, agentcontext.StatusOK, got.Status)
|
|
require.Len(t, got.Tools, 1)
|
|
require.Equal(t, "echo", got.Tools[0].Name)
|
|
require.Equal(t, "echoes input", got.Tools[0].Description)
|
|
}
|
|
|
|
// TestManager_MCPServerHangingCloseDoesNotStall is a regression test for
|
|
// a server that ignores stdin-close. mcp-go's stdio Close() closes stdin
|
|
// and then blocks on cmd.Wait(); without a force-kill the runner's
|
|
// per-server connect (and thus the whole reload) would hang and the
|
|
// tools would never be published. The runner force-kills the subprocess,
|
|
// so the tool still surfaces in the snapshot.
|
|
func TestManager_MCPServerHangingCloseDoesNotStall(t *testing.T) {
|
|
t.Parallel()
|
|
dir := t.TempDir()
|
|
writeMCPConfig(t, dir, "hang", map[string]string{
|
|
"TEST_MCP_FAKE_SERVER": "1",
|
|
"TEST_MCP_HANG_AFTER_LIST": "1",
|
|
})
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return dir },
|
|
MCPExecer: agentexec.DefaultExecer,
|
|
})
|
|
ctx := testutil.Context(t, testutil.WaitLong)
|
|
go func() { _ = m.Run(ctx) }()
|
|
|
|
require.Eventually(t, func() bool {
|
|
got := findMCPServer(m.Snapshot(), "hang")
|
|
return got != nil && got.Status == agentcontext.StatusOK
|
|
}, testutil.WaitLong, testutil.IntervalMedium,
|
|
"a hanging MCP server must not stall the reload; its tool should still surface")
|
|
}
|
|
|
|
// findMCPServer returns the KindMCPServer resource for the named server,
|
|
// or nil if absent.
|
|
func findMCPServer(snap agentcontext.Snapshot, name string) *agentcontext.Resource {
|
|
for i := range snap.Resources {
|
|
if r := snap.Resources[i]; r.Kind == agentcontext.KindMCPServer && r.Source == name {
|
|
return &r
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// writeMCPConfig writes a .mcp.json into dir declaring a single stdio MCP
|
|
// server that re-execs this test binary into serveFakeMCPServer (via the
|
|
// TEST_MCP_FAKE_SERVER env, which TestMain handles).
|
|
func writeMCPConfig(t *testing.T, dir, name string, env map[string]string) {
|
|
t.Helper()
|
|
testBin, err := os.Executable()
|
|
require.NoError(t, err)
|
|
cfg := map[string]any{
|
|
"mcpServers": map[string]any{
|
|
name: map[string]any{
|
|
"command": testBin,
|
|
"env": env,
|
|
},
|
|
},
|
|
}
|
|
data, err := json.Marshal(cfg)
|
|
require.NoError(t, err)
|
|
require.NoError(t, os.WriteFile(filepath.Join(dir, ".mcp.json"), data, 0o600))
|
|
}
|
|
|
|
// maybeServeFakeMCPServer serves the fake stdio MCP server when
|
|
// TEST_MCP_FAKE_SERVER=1 and reports whether it handled the process so
|
|
// the caller (TestMain) can exit. The runner re-execs the test binary
|
|
// into this, so it must run at the very top of TestMain. When
|
|
// TEST_MCP_HANG_AFTER_LIST=1 the server blocks after serving instead of
|
|
// returning, simulating a server that ignores stdin-close so a test can
|
|
// exercise the runner's force-kill (the process is then killed by the
|
|
// parent and never returns here).
|
|
func maybeServeFakeMCPServer() (served bool) {
|
|
if os.Getenv("TEST_MCP_FAKE_SERVER") != "1" {
|
|
return false
|
|
}
|
|
serveFakeMCPServer()
|
|
if os.Getenv("TEST_MCP_HANG_AFTER_LIST") == "1" {
|
|
select {}
|
|
}
|
|
return true
|
|
}
|
|
|
|
// serveFakeMCPServer serves a minimal MCP protocol over stdin/stdout: it
|
|
// answers initialize and advertises a single "echo" tool, then returns
|
|
// when the client closes stdin (EOF).
|
|
func serveFakeMCPServer() {
|
|
scanner := bufio.NewScanner(os.Stdin)
|
|
for scanner.Scan() {
|
|
line := scanner.Bytes()
|
|
|
|
var req struct {
|
|
JSONRPC string `json:"jsonrpc"`
|
|
ID json.RawMessage `json:"id"`
|
|
Method string `json:"method"`
|
|
}
|
|
if err := json.Unmarshal(line, &req); err != nil {
|
|
continue
|
|
}
|
|
|
|
var resp any
|
|
switch req.Method {
|
|
case "initialize":
|
|
resp = map[string]any{
|
|
"jsonrpc": "2.0",
|
|
"id": req.ID,
|
|
"result": map[string]any{
|
|
"protocolVersion": "2025-03-26",
|
|
"capabilities": map[string]any{"tools": map[string]any{}},
|
|
"serverInfo": map[string]any{"name": "fake-server", "version": "0.0.1"},
|
|
},
|
|
}
|
|
case "notifications/initialized":
|
|
// Notifications take no response.
|
|
continue
|
|
case "tools/list":
|
|
resp = map[string]any{
|
|
"jsonrpc": "2.0",
|
|
"id": req.ID,
|
|
"result": map[string]any{
|
|
"tools": []map[string]any{
|
|
{
|
|
"name": "echo",
|
|
"description": "echoes input",
|
|
"inputSchema": map[string]any{
|
|
"type": "object",
|
|
"properties": map[string]any{},
|
|
},
|
|
},
|
|
},
|
|
},
|
|
}
|
|
default:
|
|
resp = map[string]any{
|
|
"jsonrpc": "2.0",
|
|
"id": req.ID,
|
|
"error": map[string]any{"code": -32601, "message": "method not found"},
|
|
}
|
|
}
|
|
|
|
out, err := json.Marshal(resp)
|
|
if err != nil {
|
|
continue
|
|
}
|
|
_, _ = fmt.Fprintf(os.Stdout, "%s\n", out)
|
|
}
|
|
}
|