From 0972a1600a330ad5677aa475b462eff9f529bb78 Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Wed, 25 Jun 2025 13:15:09 +0300 Subject: [PATCH] fix legacy mcp config --- AGENTS.md | 20 ++++- README.md | 24 ++++- internal/config/config.go | 3 + internal/config/config_test.go | 155 +++++++++++++++++++++++++++++++++ internal/tools/mcp.go | 29 +++++- 5 files changed, 226 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index bc53630d..a39a7a07 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,6 +44,7 @@ MCPHost supports a simplified configuration schema with three server types: - Maintains full backward compatibility with existing configurations - Automatic detection and conversion of legacy formats - Custom `UnmarshalJSON` method handles format migration seamlessly +- **Bug Fix**: Improved stdio transport reliability for legacy configurations with external processes (Docker, NPX, etc.) ### Transport Mapping - `"local"` type → `stdio` transport (launches local processes) @@ -108,4 +109,21 @@ MCPHost includes a powerful script system for automation and reusable workflows. ### Testing - **Test Coverage**: 25+ test cases covering all variable scenarios - **Edge Cases**: Empty defaults, complex values, mixed required/optional variables -- **Backward Compatibility**: Ensures existing scripts continue working unchanged \ No newline at end of file +- **Backward Compatibility**: Ensures existing scripts continue working unchanged + +## Recent Bug Fixes + +### Legacy MCP Server Configuration Fix +- **Issue**: Legacy stdio transport configurations (using `command` + `args` format) were failing with timeout errors +- **Root Cause**: MCP client creation was not properly handling legacy argument parsing when `Command` array was incomplete +- **Fix**: Added fallback logic in `internal/tools/mcp.go` to use legacy `Args` field when `Command` array only contains the command +- **Impact**: Fixes Docker-based MCP servers, NPX-based servers, and other external process configurations +- **Files Modified**: + - `internal/tools/mcp.go`: Added legacy args fallback logic + - `internal/config/config.go`: Enhanced headers support for remote servers +- **Testing**: Verified with both Docker (`ghcr.io/mark3labs/phalcon-mcp:latest`) and NPX (`@modelcontextprotocol/server-filesystem`) servers + +### MCP Client Initialization Improvements +- **Timeout**: Increased initialization timeout from 10s to 30s for slower external processes +- **Capabilities**: Added explicit `ClientCapabilities{}` to initialization request for better compatibility +- **Headers**: Enhanced SSE transport to support custom headers for authentication \ No newline at end of file diff --git a/README.md b/README.md index d9f81588..6fcbcb3d 100644 --- a/README.md +++ b/README.md @@ -138,7 +138,8 @@ For remote MCP servers accessible via HTTP: "mcpServers": { "websearch": { "type": "remote", - "url": "https://api.example.com/mcp" + "url": "https://api.example.com/mcp", + "headers": ["Authorization: Bearer your-api-token"] }, "weather": { "type": "remote", @@ -151,6 +152,7 @@ For remote MCP servers accessible via HTTP: Each remote server entry requires: - `type`: Must be set to `"remote"` - `url`: The URL where the MCP server is accessible +- `headers`: (Optional) Array of HTTP headers for authentication and custom headers Remote servers automatically use the StreamableHTTP transport for optimal performance. @@ -246,7 +248,7 @@ All MCP server types support tool filtering to restrict which tools are availabl ### Legacy Configuration Support -MCPHost maintains full backward compatibility with the previous configuration format: +MCPHost maintains full backward compatibility with the previous configuration format. **Note**: A recent bug fix improved legacy stdio transport reliability for external MCP servers (Docker, NPX, etc.). #### Legacy STDIO Format ```json @@ -275,6 +277,24 @@ MCPHost maintains full backward compatibility with the previous configuration fo } ``` +#### Legacy Docker/Container Format +```json +{ + "mcpServers": { + "phalcon": { + "command": "docker", + "args": [ + "run", + "-i", + "--rm", + "ghcr.io/mark3labs/phalcon-mcp:latest", + "serve" + ] + } + } +} +``` + #### Legacy Streamable HTTP Format ```json { diff --git a/internal/config/config.go b/internal/config/config.go index e9e3897e..68ad030a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -34,6 +34,7 @@ func (s *MCPServerConfig) UnmarshalJSON(data []byte) error { Command []string `json:"command,omitempty"` Environment map[string]string `json:"environment,omitempty"` URL string `json:"url,omitempty"` + Headers []string `json:"headers,omitempty"` Name string `json:"name,omitempty"` Options map[string]any `json:"options,omitempty"` AllowedTools []string `json:"allowedTools,omitempty"` @@ -59,6 +60,7 @@ func (s *MCPServerConfig) UnmarshalJSON(data []byte) error { s.Command = newConfig.Command s.Environment = newConfig.Environment s.URL = newConfig.URL + s.Headers = newConfig.Headers s.Name = newConfig.Name s.Options = newConfig.Options s.AllowedTools = newConfig.AllowedTools @@ -283,6 +285,7 @@ func createDefaultConfig(homeDir string) error { # name: "fetch" # # # Remote MCP servers - connect via StreamableHTTP transport +# # Optional 'headers' field can be used for authentication and custom headers # websearch: # type: "remote" # url: "https://api.example.com/mcp" diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ecc6615c..b3ce55a6 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -401,6 +401,161 @@ func TestIssue76_ExactReproduction(t *testing.T) { } } +func TestMCPServerConfig_RemoteFormatWithHeaders(t *testing.T) { + tests := []struct { + name string + jsonData string + expectedType string + expectedURL string + expectedHeaders []string + expectedTransport string + }{ + { + name: "Remote server with headers", + jsonData: `{ + "type": "remote", + "url": "https://api.example.com/mcp", + "headers": ["Authorization: Bearer token123", "X-API-Key: key456"] + }`, + expectedType: "remote", + expectedURL: "https://api.example.com/mcp", + expectedHeaders: []string{"Authorization: Bearer token123", "X-API-Key: key456"}, + expectedTransport: "streamable", + }, + { + name: "Remote server without headers", + jsonData: `{ + "type": "remote", + "url": "https://api.example.com/mcp" + }`, + expectedType: "remote", + expectedURL: "https://api.example.com/mcp", + expectedHeaders: nil, + expectedTransport: "streamable", + }, + { + name: "Legacy remote server with headers", + jsonData: `{ + "url": "https://legacy.example.com/mcp", + "headers": ["Content-Type: application/json", "User-Agent: MCPHost/1.0"] + }`, + expectedType: "", + expectedURL: "https://legacy.example.com/mcp", + expectedHeaders: []string{"Content-Type: application/json", "User-Agent: MCPHost/1.0"}, + expectedTransport: "sse", + }, + { + name: "Legacy remote server with explicit transport and headers", + jsonData: `{ + "url": "https://legacy.example.com/mcp", + "transport": "sse", + "headers": ["Authorization: Basic dXNlcjpwYXNz"] + }`, + expectedType: "", + expectedURL: "https://legacy.example.com/mcp", + expectedHeaders: []string{"Authorization: Basic dXNlcjpwYXNz"}, + expectedTransport: "sse", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var config MCPServerConfig + err := json.Unmarshal([]byte(tt.jsonData), &config) + if err != nil { + t.Fatalf("Failed to unmarshal: %v", err) + } + + if config.Type != tt.expectedType { + t.Errorf("Expected type '%s', got '%s'", tt.expectedType, config.Type) + } + + if config.URL != tt.expectedURL { + t.Errorf("Expected URL '%s', got '%s'", tt.expectedURL, config.URL) + } + + if len(config.Headers) != len(tt.expectedHeaders) { + t.Errorf("Expected %d headers, got %d", len(tt.expectedHeaders), len(config.Headers)) + } + + for i, expectedHeader := range tt.expectedHeaders { + if i >= len(config.Headers) || config.Headers[i] != expectedHeader { + t.Errorf("Header %d: expected '%s', got '%s'", i, expectedHeader, config.Headers[i]) + } + } + + transportType := config.GetTransportType() + if transportType != tt.expectedTransport { + t.Errorf("Expected transport type '%s', got '%s'", tt.expectedTransport, transportType) + } + }) + } +} + +func TestMCPServerConfig_HeadersParsing(t *testing.T) { + // Test that headers are properly parsed in both new and legacy formats + tests := []struct { + name string + jsonData string + expected []string + }{ + { + name: "New format with multiple headers", + jsonData: `{ + "type": "remote", + "url": "https://api.example.com", + "headers": [ + "Authorization: Bearer abc123", + "Content-Type: application/json", + "X-Custom-Header: custom-value" + ] + }`, + expected: []string{ + "Authorization: Bearer abc123", + "Content-Type: application/json", + "X-Custom-Header: custom-value", + }, + }, + { + name: "Legacy format with headers", + jsonData: `{ + "url": "https://legacy.example.com", + "headers": ["API-Key: secret123", "Accept: application/json"] + }`, + expected: []string{"API-Key: secret123", "Accept: application/json"}, + }, + { + name: "Empty headers array", + jsonData: `{ + "type": "remote", + "url": "https://api.example.com", + "headers": [] + }`, + expected: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var config MCPServerConfig + err := json.Unmarshal([]byte(tt.jsonData), &config) + if err != nil { + t.Fatalf("Failed to unmarshal: %v", err) + } + + if len(config.Headers) != len(tt.expected) { + t.Errorf("Expected %d headers, got %d", len(tt.expected), len(config.Headers)) + } + + for i, expected := range tt.expected { + if i >= len(config.Headers) || config.Headers[i] != expected { + t.Errorf("Header %d: expected '%s', got '%s'", i, expected, config.Headers[i]) + } + } + }) + } +} + func TestEnsureConfigExistsWhenFileExists(t *testing.T) { // Create a temporary directory for testing tempDir, err := os.MkdirTemp("", "mcphost_config_test") diff --git a/internal/tools/mcp.go b/internal/tools/mcp.go index 1f9d1a47..f1118a4b 100644 --- a/internal/tools/mcp.go +++ b/internal/tools/mcp.go @@ -228,6 +228,10 @@ func (m *MCPToolManager) createMCPClient(ctx context.Context, serverName string, command = serverConfig.Command[0] if len(serverConfig.Command) > 1 { args = serverConfig.Command[1:] + } else if len(serverConfig.Args) > 0 { + // Legacy fallback: Command only has the command, Args has the arguments + // This handles cases where legacy config conversion didn't work properly + args = serverConfig.Args } } @@ -245,6 +249,8 @@ func (m *MCPToolManager) createMCPClient(ctx context.Context, serverName string, } } + + stdioClient, err := client.NewStdioMCPClient(command, env, args...) if err != nil { return nil, fmt.Errorf("failed to create stdio client: %v", err) @@ -260,7 +266,25 @@ func (m *MCPToolManager) createMCPClient(ctx context.Context, serverName string, case "sse": // SSE client - sseClient, err := client.NewSSEMCPClient(serverConfig.URL) + var options []transport.ClientOption + + // Add headers if specified + if len(serverConfig.Headers) > 0 { + headers := make(map[string]string) + for _, header := range serverConfig.Headers { + parts := strings.SplitN(header, ":", 2) + if len(parts) == 2 { + key := strings.TrimSpace(parts[0]) + value := strings.TrimSpace(parts[1]) + headers[key] = value + } + } + if len(headers) > 0 { + options = append(options, transport.WithHeaders(headers)) + } + } + + sseClient, err := client.NewSSEMCPClient(serverConfig.URL, options...) if err != nil { return nil, err } @@ -315,7 +339,7 @@ func (m *MCPToolManager) createMCPClient(ctx context.Context, serverName string, func (m *MCPToolManager) initializeClient(ctx context.Context, client client.MCPClient) error { // Create a timeout context for initialization to prevent deadlocks - initCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + initCtx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() initRequest := mcp.InitializeRequest{} @@ -324,6 +348,7 @@ func (m *MCPToolManager) initializeClient(ctx context.Context, client client.MCP Name: "mcphost", Version: "1.0.0", } + initRequest.Params.Capabilities = mcp.ClientCapabilities{} _, err := client.Initialize(initCtx, initRequest) if err != nil {