mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
refactor: consolidate agent MCP onto a single persistent engine (#26599)
Two MCP code paths both spawned the servers declared in a workspace's `.mcp.json`: the persistent engine in `agent/x/agentmcp` (which owns tool-call execution via `CallTool`) and an ephemeral one-shot runner in `agent/agentcontext` (`mcprunner.go`) that connected, listed tools, and immediately closed each server purely for discovery. Every declared server was launched twice, and the discovery path duplicated the engine's `.mcp.json` parse, transport-build, env-resolve, and connect logic. This makes `agent/x/agentmcp` the single persistent MCP engine. The `agentcontext` manager now reads that engine's per-server catalog in-process through an injected `MCPCatalog` option and surfaces each server as a `KindMCPServer` resource. The engine wires `SetOnReload` to the manager's `Trigger`, so a reload (startup connect or `.mcp.json` edit) re-resolves and re-pushes the pinned resources. Tool-call execution is unchanged: it still flows through the engine's `CallTool` over `POST /api/v0/mcp/call-tool`. The now-dead HTTP discovery surface is removed: the agent `GET /api/v0/mcp/tools` route with `agentmcp.API.handleListTools`, and `workspacesdk.AgentConn.ListMCPTools` with `ListMCPToolsResponse` (mock regenerated). The change nets roughly `-1370` lines, mostly the deleted duplicate runner and its tests. <details> <summary>Decision log</summary> The merge of #26585 made pinned `chat_context_resources` the sole source of workspace context, which surfaced the duplicate spawning. Two options were considered: - **Option A + dependency injection (chosen):** keep `agent/x/agentmcp` as the single persistent engine; `agentcontext` consumes its catalog in-process and stays the orchestrator/owner at the API boundary (it still pushes `KindMCPServer` resources). This is low-risk because `agentcontext` already exposed the `resolver.MCPResources` seam, so the change just rebinds it from the ephemeral runner to the shared engine. - **Option B (rejected):** reimplement persistent pooling, reconnect, singleflight, and race handling inside `agentcontext` and delete `agentmcp`. Too broad, and it discards the engine's tested lifecycle for no behavioral gain. `agentcontext`'s discovery was never what kept servers alive; its runner closed each server immediately after listing tools. The component holding persistent connections was always `agentmcp`, which is why execution already lived there. Consolidating onto it removes the duplicated stack rather than a whole package: both packages survive with distinct roles (`agentmcp` is the engine, `agentcontext` is the orchestrator/owner). </details> Coder Agents generated on behalf of @kylecarbs
This commit is contained in:
@@ -262,8 +262,6 @@ func newWorkspaceToolTestServer(
|
||||
mockConn.EXPECT().SetExtraHeaders(gomock.Any()).AnyTimes()
|
||||
mockConn.EXPECT().ContextConfig(gomock.Any()).
|
||||
Return(workspacesdk.ContextConfigResponse{}, xerrors.New("not supported")).AnyTimes()
|
||||
mockConn.EXPECT().ListMCPTools(gomock.Any()).
|
||||
Return(workspacesdk.ListMCPToolsResponse{}, nil).AnyTimes()
|
||||
mockConn.EXPECT().LS(gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
Return(workspacesdk.LSResponse{AbsolutePathString: "/home/coder"}, nil).AnyTimes()
|
||||
mockConn.EXPECT().ReadFile(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
@@ -898,17 +896,6 @@ func TestExploreChatUsesPersistedMCPSnapshot(t *testing.T) {
|
||||
mockConn.EXPECT().ContextConfig(gomock.Any()).
|
||||
Return(workspacesdk.ContextConfigResponse{}, xerrors.New("not supported")).AnyTimes()
|
||||
workspaceToolName := "workspace-snapshot-mcp__echo"
|
||||
mockConn.EXPECT().ListMCPTools(gomock.Any()).
|
||||
Return(workspacesdk.ListMCPToolsResponse{Tools: []workspacesdk.MCPToolInfo{{
|
||||
ServerName: "workspace-snapshot-mcp",
|
||||
Name: workspaceToolName,
|
||||
Description: "Workspace echo tool",
|
||||
Schema: map[string]any{
|
||||
"input": map[string]any{"type": "string"},
|
||||
},
|
||||
Required: []string{"input"},
|
||||
}}}, nil).
|
||||
AnyTimes()
|
||||
mockConn.EXPECT().LS(gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
Return(workspacesdk.LSResponse{AbsolutePathString: "/home/coder"}, nil).AnyTimes()
|
||||
mockConn.EXPECT().ReadFile(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
@@ -3124,10 +3111,6 @@ func TestPersistToolResultWithBinaryData(t *testing.T) {
|
||||
ContextConfig(gomock.Any()).
|
||||
Return(workspacesdk.ContextConfigResponse{}, xerrors.New("not supported")).
|
||||
AnyTimes()
|
||||
mockConn.EXPECT().
|
||||
ListMCPTools(gomock.Any()).
|
||||
Return(workspacesdk.ListMCPToolsResponse{}, nil).
|
||||
AnyTimes()
|
||||
mockConn.EXPECT().
|
||||
LS(gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
Return(workspacesdk.LSResponse{}, nil).
|
||||
@@ -6478,14 +6461,11 @@ func toolMessageForTest(
|
||||
func setupToolExecutionAgentConn(
|
||||
t *testing.T,
|
||||
mockConn *agentconnmock.MockAgentConn,
|
||||
mcpTools ...workspacesdk.MCPToolInfo,
|
||||
) {
|
||||
t.Helper()
|
||||
mockConn.EXPECT().SetExtraHeaders(gomock.Any()).AnyTimes()
|
||||
mockConn.EXPECT().ContextConfig(gomock.Any()).
|
||||
Return(workspacesdk.ContextConfigResponse{}, xerrors.New("not supported")).AnyTimes()
|
||||
mockConn.EXPECT().ListMCPTools(gomock.Any()).
|
||||
Return(workspacesdk.ListMCPToolsResponse{Tools: mcpTools}, nil).AnyTimes()
|
||||
mockConn.EXPECT().LS(gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
Return(workspacesdk.LSResponse{AbsolutePathString: "/home/coder"}, nil).AnyTimes()
|
||||
mockConn.EXPECT().ReadFile(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
@@ -9438,10 +9418,6 @@ func TestComputerUseSubagentToolsAndModel(t *testing.T) {
|
||||
// for the initial screenshot check in the computer use path.
|
||||
ctrl := gomock.NewController(t)
|
||||
mockConn := agentconnmock.NewMockAgentConn(ctrl)
|
||||
mockConn.EXPECT().
|
||||
ListMCPTools(gomock.Any()).
|
||||
Return(workspacesdk.ListMCPToolsResponse{}, nil).
|
||||
AnyTimes()
|
||||
mockConn.EXPECT().
|
||||
ExecuteDesktopAction(gomock.Any(), gomock.Any()).
|
||||
Return(workspacesdk.DesktopActionResponse{
|
||||
@@ -10205,8 +10181,6 @@ func TestMCPServerToolInvocation(t *testing.T) {
|
||||
mockConn.EXPECT().SetExtraHeaders(gomock.Any()).AnyTimes()
|
||||
mockConn.EXPECT().ContextConfig(gomock.Any()).
|
||||
Return(workspacesdk.ContextConfigResponse{}, xerrors.New("not supported")).AnyTimes()
|
||||
mockConn.EXPECT().ListMCPTools(gomock.Any()).
|
||||
Return(workspacesdk.ListMCPToolsResponse{}, nil).AnyTimes()
|
||||
mockConn.EXPECT().LS(gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
Return(workspacesdk.LSResponse{}, nil).AnyTimes()
|
||||
mockConn.EXPECT().ReadFile(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
@@ -10714,8 +10688,6 @@ func TestMCPServerOAuth2TokenRefresh(t *testing.T) {
|
||||
mockConn.EXPECT().SetExtraHeaders(gomock.Any()).AnyTimes()
|
||||
mockConn.EXPECT().ContextConfig(gomock.Any()).
|
||||
Return(workspacesdk.ContextConfigResponse{}, xerrors.New("not supported")).AnyTimes()
|
||||
mockConn.EXPECT().ListMCPTools(gomock.Any()).
|
||||
Return(workspacesdk.ListMCPToolsResponse{}, nil).AnyTimes()
|
||||
mockConn.EXPECT().LS(gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
Return(workspacesdk.LSResponse{}, nil).AnyTimes()
|
||||
mockConn.EXPECT().ReadFile(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
@@ -12670,8 +12642,6 @@ func setupWorkspaceContextAgentConn(
|
||||
}, nil
|
||||
},
|
||||
).AnyTimes()
|
||||
mockConn.EXPECT().ListMCPTools(gomock.Any()).
|
||||
Return(workspacesdk.ListMCPToolsResponse{}, nil).AnyTimes()
|
||||
mockConn.EXPECT().LS(gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
Return(workspacesdk.LSResponse{AbsolutePathString: "/home/coder"}, nil).AnyTimes()
|
||||
mockConn.EXPECT().ReadFile(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
|
||||
|
||||
Reference in New Issue
Block a user