From 519de03da586b59b6fce04b86adf6375bb1291e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sat, 18 Apr 2026 18:37:09 +0100 Subject: [PATCH] fix(agentctl): inject HTTP MCP server for Codex ACP support (#641) * fix(agentctl): inject HTTP MCP server for Codex ACP support Codex ACP only supports HTTP-based MCP servers (not SSE). The kandev MCP server was only injected with SSE transport, causing it to be filtered out by Codex ACP's capability check. Now inject both SSE and HTTP variants of the kandev MCP server: - SSE at /sse (for Claude Code) - HTTP at /mcp (for Codex ACP) The agent's capability filtering automatically selects the appropriate transport based on what the agent supports. * refactor(agentctl): address code review feedback - Add name deduplication to filterMcpServersByCapabilities to prevent duplicate MCP server entries if an agent supports both transports - Extract injectKandevMcpServers helper to centralize dual-transport injection logic in agent.go - Add constants for MCP server name and transport types - Add test for duplicate name deduplication * fix(agentctl): add MCP injection to handleWSResetSession Apply injectKandevMcpServers to session reset handler, ensuring MCP tools remain available after context window reset. --- .../server/adapter/transport/acp/adapter.go | 9 +++ .../server/adapter/transport/acp/mcp_test.go | 40 +++++++++ .../internal/agentctl/server/api/agent.go | 81 +++++++++++-------- .../internal/agentctl/server/config/config.go | 24 ++++-- 4 files changed, 113 insertions(+), 41 deletions(-) diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go index ed911465dc..1f4741626f 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go @@ -294,8 +294,11 @@ func (a *Adapter) NewSession(ctx context.Context, mcpServers []types.McpServer) // filterMcpServersByCapabilities removes MCP servers that the agent doesn't support. // Stdio servers are always allowed; SSE/HTTP servers require the corresponding capability. +// If multiple servers share the same name (e.g., dual SSE+HTTP injection), only the first +// surviving entry is kept to prevent duplicate tool registration. func filterMcpServersByCapabilities(servers []types.McpServer, caps acp.McpCapabilities, logger *logger.Logger) []types.McpServer { filtered := make([]types.McpServer, 0, len(servers)) + seenNames := make(map[string]bool) for _, s := range servers { switch s.Type { case "sse": @@ -309,6 +312,12 @@ func filterMcpServersByCapabilities(servers []types.McpServer, caps acp.McpCapab continue } } + // Skip duplicate names - first surviving entry wins + if seenNames[s.Name] { + logger.Debug("skipping duplicate MCP server name", zap.String("name", s.Name), zap.String("type", s.Type)) + continue + } + seenNames[s.Name] = true filtered = append(filtered, s) } return filtered diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/mcp_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/mcp_test.go index 1d3084d396..f1c07b34f7 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/mcp_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/mcp_test.go @@ -346,6 +346,46 @@ func TestFilterMcpServersByCapabilities_Mixed(t *testing.T) { } } +func TestFilterMcpServersByCapabilities_DeduplicatesSameName(t *testing.T) { + log := newTestLoggerForMcp() + // Simulate dual injection: same name "kandev" with different types + servers := []types.McpServer{ + {Name: "kandev", Type: "sse", URL: "http://localhost:10005/sse"}, + {Name: "kandev", Type: "http", URL: "http://localhost:10005/mcp"}, + {Name: "other", Type: "stdio", Command: "cmd"}, + } + + // Agent supports both SSE and HTTP - should only get first kandev entry + result := filterMcpServersByCapabilities(servers, acp.McpCapabilities{Sse: true, Http: true}, log) + if len(result) != 2 { + t.Fatalf("expected 2 (first kandev + other), got %d", len(result)) + } + if result[0].Name != "kandev" || result[0].Type != "sse" { + t.Errorf("expected first entry to be kandev/sse, got %s/%s", result[0].Name, result[0].Type) + } + if result[1].Name != "other" { + t.Errorf("expected second entry to be other, got %s", result[1].Name) + } + + // Agent supports only HTTP - should get kandev/http (SSE filtered, then HTTP kept) + result = filterMcpServersByCapabilities(servers, acp.McpCapabilities{Sse: false, Http: true}, log) + if len(result) != 2 { + t.Fatalf("expected 2 (kandev/http + other), got %d", len(result)) + } + if result[0].Name != "kandev" || result[0].Type != "http" { + t.Errorf("expected first entry to be kandev/http, got %s/%s", result[0].Name, result[0].Type) + } + + // Agent supports only SSE - should get kandev/sse (HTTP filtered) + result = filterMcpServersByCapabilities(servers, acp.McpCapabilities{Sse: true, Http: false}, log) + if len(result) != 2 { + t.Fatalf("expected 2 (kandev/sse + other), got %d", len(result)) + } + if result[0].Name != "kandev" || result[0].Type != "sse" { + t.Errorf("expected first entry to be kandev/sse, got %s/%s", result[0].Name, result[0].Type) + } +} + // newTestLoggerForMcp creates a logger for MCP tests. func newTestLoggerForMcp() *logger.Logger { return newTestAdapter().logger diff --git a/apps/backend/internal/agentctl/server/api/agent.go b/apps/backend/internal/agentctl/server/api/agent.go index 17516b8b64..bc40da3196 100644 --- a/apps/backend/internal/agentctl/server/api/agent.go +++ b/apps/backend/internal/agentctl/server/api/agent.go @@ -19,6 +19,15 @@ import ( "go.uber.org/zap" ) +// MCP server constants for kandev injection. +const ( + kandevMcpServerName = "kandev" + mcpTransportSSE = "sse" + mcpTransportHTTP = "http" + mcpPathSSE = "/sse" + mcpPathHTTP = "/mcp" +) + // InitializeRequest is a request to initialize the agent session. type InitializeRequest struct { ClientName string `json:"client_name"` @@ -301,6 +310,35 @@ func (s *Server) handleWSInitialize(ctx context.Context, msg *ws.Message) *ws.Me return resp } +// injectKandevMcpServers prepends the local kandev MCP server to the list of MCP servers. +// Both SSE and HTTP variants are injected - the agent's capability filtering will select +// the appropriate one based on what the agent supports (SSE for Claude Code, HTTP for Codex ACP). +// Any existing kandev server in the list is filtered out to avoid duplicates. +func (s *Server) injectKandevMcpServers(mcpServers []types.McpServer) []types.McpServer { + kandevMcpSse := types.McpServer{ + Name: kandevMcpServerName, + Type: mcpTransportSSE, + URL: fmt.Sprintf("http://localhost:%d%s", s.cfg.Port, mcpPathSSE), + } + kandevMcpHttp := types.McpServer{ + Name: kandevMcpServerName, + Type: mcpTransportHTTP, + URL: fmt.Sprintf("http://localhost:%d%s", s.cfg.Port, mcpPathHTTP), + } + filtered := make([]types.McpServer, 0, len(mcpServers)+2) + filtered = append(filtered, kandevMcpSse, kandevMcpHttp) + for _, srv := range mcpServers { + if srv.Name != kandevMcpServerName { + filtered = append(filtered, srv) + } + } + s.logger.Debug("injected local kandev MCP servers (sse+http)", + zap.String("sse_url", kandevMcpSse.URL), + zap.String("http_url", kandevMcpHttp.URL), + zap.Int("total_servers", len(filtered))) + return filtered +} + func (s *Server) handleWSNewSession(ctx context.Context, msg *ws.Message) *ws.Message { var req NewSessionRequest if err := msg.ParsePayload(&req); err != nil { @@ -327,22 +365,7 @@ func (s *Server) handleWSNewSession(ctx context.Context, msg *ws.Message) *ws.Me // If MCP server is enabled, prepend the local kandev MCP server to the list. mcpServers := req.McpServers if s.mcpServer != nil { - localKandevMcp := types.McpServer{ - Name: "kandev", - Type: "sse", - URL: fmt.Sprintf("http://localhost:%d/sse", s.cfg.Port), - } - filtered := make([]types.McpServer, 0, len(mcpServers)+1) - filtered = append(filtered, localKandevMcp) - for _, srv := range mcpServers { - if srv.Name != "kandev" { - filtered = append(filtered, srv) - } - } - mcpServers = filtered - s.logger.Debug("injected local kandev MCP server", - zap.String("url", localKandevMcp.URL), - zap.Int("total_servers", len(mcpServers))) + mcpServers = s.injectKandevMcpServers(mcpServers) } sessionID, err := adapter.NewSession(ctx, mcpServers) @@ -387,25 +410,9 @@ func (s *Server) handleWSLoadSession(ctx context.Context, msg *ws.Message) *ws.M } // If MCP server is enabled, prepend the local kandev MCP server to the list. - // This mirrors handleWSNewSession to ensure MCP tools are available after resume. mcpServers := req.McpServers if s.mcpServer != nil { - localKandevMcp := types.McpServer{ - Name: "kandev", - Type: "sse", - URL: fmt.Sprintf("http://localhost:%d/sse", s.cfg.Port), - } - filtered := make([]types.McpServer, 0, len(mcpServers)+1) - filtered = append(filtered, localKandevMcp) - for _, srv := range mcpServers { - if srv.Name != "kandev" { - filtered = append(filtered, srv) - } - } - mcpServers = filtered - s.logger.Debug("injected local kandev MCP server for loaded session", - zap.String("url", localKandevMcp.URL), - zap.Int("total_servers", len(mcpServers))) + mcpServers = s.injectKandevMcpServers(mcpServers) } if err := adapter.LoadSession(ctx, req.SessionID, mcpServers); err != nil { @@ -602,7 +609,13 @@ func (s *Server) handleWSResetSession(ctx context.Context, msg *ws.Message) *ws. return resp } - sessionID, err := sr.ResetSession(ctx, req.McpServers) + // If MCP server is enabled, prepend the local kandev MCP server to the list. + mcpServers := req.McpServers + if s.mcpServer != nil { + mcpServers = s.injectKandevMcpServers(mcpServers) + } + + sessionID, err := sr.ResetSession(ctx, mcpServers) if err != nil { s.logger.Error("session reset failed", zap.Error(err)) resp, _ := ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, err.Error(), nil) diff --git a/apps/backend/internal/agentctl/server/config/config.go b/apps/backend/internal/agentctl/server/config/config.go index ab760e79a5..6cb03f10c0 100644 --- a/apps/backend/internal/agentctl/server/config/config.go +++ b/apps/backend/internal/agentctl/server/config/config.go @@ -386,21 +386,31 @@ func getEnvBool(key string, defaultValue bool) bool { return defaultValue } +// kandevMcpServerName is the name used for the local kandev MCP server. +const kandevMcpServerName = "kandev" + // injectKandevMcpServer prepends the local kandev MCP server to the list of MCP servers. // This replaces any existing kandev server to avoid duplicates. // The kandev MCP server provides tools like ask_user_question to the agent. +// Both SSE and HTTP variants are injected - agent capability filtering will select the appropriate one. func injectKandevMcpServer(servers []McpServerConfig, port int) []McpServerConfig { - localKandevMcp := McpServerConfig{ - Name: "kandev", + portStr := strconv.Itoa(port) + kandevMcpSse := McpServerConfig{ + Name: kandevMcpServerName, Type: "sse", - URL: "http://localhost:" + strconv.Itoa(port) + "/sse", + URL: "http://localhost:" + portStr + "/sse", + } + kandevMcpHttp := McpServerConfig{ + Name: kandevMcpServerName, + Type: "http", + URL: "http://localhost:" + portStr + "/mcp", } - // Filter out any existing kandev server and prepend the local one - result := make([]McpServerConfig, 0, len(servers)+1) - result = append(result, localKandevMcp) + // Filter out any existing kandev server and prepend the local ones + result := make([]McpServerConfig, 0, len(servers)+2) + result = append(result, kandevMcpSse, kandevMcpHttp) for _, srv := range servers { - if srv.Name != "kandev" { + if srv.Name != kandevMcpServerName { result = append(result, srv) } }