diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 67bb60911..64d19190d 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -3,6 +3,7 @@ package main import ( "encoding/json" "fmt" + "net/url" "os" "path/filepath" "strconv" @@ -199,13 +200,17 @@ type ProviderEntry struct { ExtraHeaders map[string]string `json:"extra_headers,omitempty"` } -// MCPServerConfig holds configuration for a single MCP server (stdio transport). +// MCPServerConfig holds configuration for a single MCP server. +// Type "stdio" (default) uses a subprocess; type "remote" uses Streamable HTTP. type MCPServerConfig struct { - Command string `json:"command"` - Args []string `json:"args,omitempty"` - Env []string `json:"env,omitempty"` - Tools []string `json:"tools,omitempty"` - Setup string `json:"setup,omitempty"` + Type string `json:"type,omitempty"` // "stdio" (default) or "remote" + Command string `json:"command,omitempty"` + Args []string `json:"args,omitempty"` + Env []string `json:"env,omitempty"` + URL string `json:"url,omitempty"` + Headers map[string]string `json:"headers,omitempty"` + Tools []string `json:"tools,omitempty"` + Setup string `json:"setup,omitempty"` } // Config represents the user-level configuration file (~/.opencodereview/config.json). @@ -402,7 +407,7 @@ func setConfigValue(cfg *Config, key, value string) error { } cfg.Llm.ExtraBody = m default: - return fmt.Errorf("unknown config key: %s\nSupported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\nProvider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: command, args, env, tools, setup", key) + return fmt.Errorf("unknown config key: %s\nSupported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\nProvider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key) } return nil } @@ -568,6 +573,11 @@ func setMCPServerValue(cfg *Config, key, value string) error { entry := cfg.MCPServers[name] switch field { + case "type": + if value != "stdio" && value != "remote" { + return fmt.Errorf("invalid MCP server type %q: must be \"stdio\" or \"remote\"", value) + } + entry.Type = value case "command": if value == "" { return fmt.Errorf("MCP server command cannot be empty") @@ -591,6 +601,27 @@ func setMCPServerValue(cfg *Config, key, value string) error { } } entry.Env = env + case "url": + if value == "" { + return fmt.Errorf("MCP server URL cannot be empty") + } + parsed, err := url.Parse(value) + if err != nil { + return fmt.Errorf("invalid MCP server URL %q: %w", value, err) + } + if parsed.Scheme != "http" && parsed.Scheme != "https" { + return fmt.Errorf("MCP server URL must use http or https scheme, got %q", parsed.Scheme) + } + if parsed.Host == "" { + return fmt.Errorf("MCP server URL %q must include a host", value) + } + entry.URL = value + case "headers": + parsed, err := parseMCPHeaders(value) + if err != nil { + return fmt.Errorf("invalid headers for %s: %w", key, err) + } + entry.Headers = parsed case "tools": var tools []string if err := json.Unmarshal([]byte(value), &tools); err != nil { @@ -612,13 +643,31 @@ func setMCPServerValue(cfg *Config, key, value string) error { case "setup": entry.Setup = value default: - return fmt.Errorf("unknown MCP server field %q: supported fields are command, args, env, tools, setup", field) + return fmt.Errorf("unknown MCP server field %q: supported fields are type, command, args, env, url, headers, tools, setup", field) } cfg.MCPServers[name] = entry return nil } +// parseMCPHeaders parses a JSON object of header key-value pairs. +// Example: {"Authorization": "Bearer $TOKEN", "X-Custom": "value"} +func parseMCPHeaders(value string) (map[string]string, error) { + var m map[string]string + if err := json.Unmarshal([]byte(value), &m); err != nil { + return nil, fmt.Errorf("expected JSON object: %w", err) + } + for k, v := range m { + if k == "" { + return nil, fmt.Errorf("header name must not be empty") + } + if v == "" { + return nil, fmt.Errorf("header value for %q must not be empty", k) + } + } + return m, nil +} + func (c *Config) ensureTelemetry() { if c.Telemetry == nil { c.Telemetry = &TelemetryConfig{} diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index 10cd57744..1cba3f99c 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -1211,3 +1211,79 @@ func TestConfigRoundTripPreservesTimeoutSec(t *testing.T) { t.Errorf("llm.timeout_sec = %d, want 60 (lost in round-trip)", got) } } + +func TestSetMCPServerValue_Type(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.gh.type", "remote"); err != nil { + t.Fatalf("setMCPServerValue: %v", err) + } + if cfg.MCPServers["gh"].Type != "remote" { + t.Errorf("Type = %q, want %q", cfg.MCPServers["gh"].Type, "remote") + } +} + +func TestSetMCPServerValue_TypeInvalid(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.gh.type", "invalid"); err == nil { + t.Fatal("expected error for invalid type, got nil") + } +} + +func TestSetMCPServerValue_URL(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.gh.url", "https://api.example.com/mcp"); err != nil { + t.Fatalf("setMCPServerValue: %v", err) + } + if cfg.MCPServers["gh"].URL != "https://api.example.com/mcp" { + t.Errorf("URL = %q, want %q", cfg.MCPServers["gh"].URL, "https://api.example.com/mcp") + } +} + +func TestSetMCPServerValue_URLEmpty(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.gh.url", ""); err == nil { + t.Fatal("expected error for empty URL, got nil") + } +} + +func TestSetMCPServerValue_URLInvalidScheme(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.gh.url", "ftp://example.com/mcp"); err == nil { + t.Fatal("expected error for non-http scheme, got nil") + } +} + +func TestSetMCPServerValue_Headers(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.gh.headers", `{"Authorization":"Bearer $TOKEN","X-Custom":"val"}`); err != nil { + t.Fatalf("setMCPServerValue: %v", err) + } + h := cfg.MCPServers["gh"].Headers + if h["Authorization"] != "Bearer $TOKEN" { + t.Errorf("Authorization = %q, want %q", h["Authorization"], "Bearer $TOKEN") + } + if h["X-Custom"] != "val" { + t.Errorf("X-Custom = %q, want %q", h["X-Custom"], "val") + } +} + +func TestSetMCPServerValue_URLNoHost(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.gh.url", "http://"); err == nil { + t.Fatal("expected error for URL without host, got nil") + } +} + +func TestSetMCPServerValue_HeadersInvalidJSON(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.gh.headers", "not-json"); err == nil { + t.Fatal("expected error for invalid JSON, got nil") + } +} + +func TestSetMCPServerValue_HeadersEmptyValue(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.gh.headers", `{"Authorization":""}`); err == nil { + t.Fatal("expected error for empty header value, got nil") + } +} diff --git a/cmd/opencodereview/flags.go b/cmd/opencodereview/flags.go index 6d6e64c2f..b008dea39 100644 --- a/cmd/opencodereview/flags.go +++ b/cmd/opencodereview/flags.go @@ -324,6 +324,11 @@ Examples: ocr config set mcp_servers.codegraph.args '["-y","@anthropic/codegraph-mcp"]' ocr config set mcp_servers.codegraph.env '["CODEGRAPH_TOKEN=xxx"]' + # Remote MCP server (Streamable HTTP transport) + ocr config set mcp_servers.remote-srv.type remote + ocr config set mcp_servers.remote-srv.url https://mcp.example.com/mcp + ocr config set mcp_servers.remote-srv.headers '{"Authorization":"Bearer $MCP_TOKEN"}' + # Delete an MCP server ocr config unset mcp_servers.codegraph @@ -339,5 +344,5 @@ Examples: Supported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging Provider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers Protocol values: anthropic, openai, openai-responses -MCP server fields: command, args, env, tools, setup`) +MCP server fields: type, command, args, env, url, headers, tools, setup`) } diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index a0e341397..9ce703f5d 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -274,6 +274,26 @@ func initMCPClients(ctx context.Context, cfg *Config, tools *tool.Registry, repo var clients []*mcp.Client for _, name := range mcpNames { serverCfg := cfg.MCPServers[name] + + isRemote := serverCfg.Type == "remote" + + if isRemote { + if serverCfg.URL == "" { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: remote MCP server %q has no URL configured, skipping\n", name) + continue + } + initCtx, initCancel := context.WithTimeout(ctx, 30*time.Second) + mc, err := mcp.NewRemoteClient(initCtx, name, serverCfg.URL, serverCfg.Headers, version) + initCancel() + if err != nil { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: failed to connect to remote MCP server %q: %v\n", name, err) + continue + } + clients = append(clients, mc) + mcp.RegisterAll(tools, mc, serverCfg.Tools) + continue + } + if serverCfg.Command == "" { fmt.Fprintf(os.Stderr, "[ocr] WARNING: MCP server %q has no command configured, skipping\n", name) continue diff --git a/internal/mcp/client.go b/internal/mcp/client.go index b06d7cded..508353fae 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -3,6 +3,8 @@ package mcp import ( "context" "fmt" + "io" + "net/http" "os" "os/exec" "strings" @@ -10,15 +12,15 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" ) -// Client wraps a single MCP server connection via stdio transport. +// Client wraps a single MCP server connection. type Client struct { name string session *mcp.ClientSession tools []*mcp.Tool } -// NewClient starts an MCP server subprocess, initializes the connection, -// and caches the list of available tools. The context governs the +// NewClient starts an MCP server subprocess (stdio transport), initializes the +// connection, and caches the list of available tools. The context governs the // initialization timeout (Connect + ListTools), NOT the subprocess // lifetime — the subprocess stays alive until Close is called. // When dir is non-empty, the subprocess runs with that working directory. @@ -60,6 +62,92 @@ func NewClient(ctx context.Context, name, command string, args, env []string, di }, nil } +// NewRemoteClient connects to a remote MCP server via Streamable HTTP transport. +// Header values may contain $ENV_VAR references which are expanded at runtime. +// Returns an error if any header value expands to an empty string. +func NewRemoteClient(ctx context.Context, name, url string, headers map[string]string, version string) (*Client, error) { + var expanded map[string]string + if len(headers) > 0 { + expanded = make(map[string]string, len(headers)) + for k, v := range headers { + expanded[k] = os.Expand(v, os.Getenv) + if expanded[k] == "" { + return nil, fmt.Errorf("MCP server %q header %q expanded to empty value — check your environment variables", name, k) + } + } + } + httpClient := &http.Client{ + Transport: &headerTransport{ + base: http.DefaultTransport, + headers: expanded, + serverName: name, + }, + } + + client := mcp.NewClient( + &mcp.Implementation{Name: "open-code-review", Version: version}, + nil, + ) + + transport := &mcp.StreamableClientTransport{ + Endpoint: url, + HTTPClient: httpClient, + } + session, err := client.Connect(ctx, transport, nil) + if err != nil { + return nil, fmt.Errorf("connect to remote MCP server %q at %s: %w", name, url, err) + } + + var success bool + defer func() { + if !success { + session.Close() + } + }() + + toolsResult, err := session.ListTools(ctx, nil) + if err != nil { + return nil, fmt.Errorf("list tools from remote MCP server %q: %w", name, err) + } + + success = true + return &Client{ + name: name, + session: session, + tools: toolsResult.Tools, + }, nil +} + +// headerTransport injects custom headers into every HTTP request and surfaces +// clear authentication errors for 401/403 responses. +type headerTransport struct { + base http.RoundTripper + headers map[string]string + serverName string +} + +func (t *headerTransport) RoundTrip(req *http.Request) (*http.Response, error) { + cloned := req.Clone(req.Context()) + for k, v := range t.headers { + cloned.Header.Set(k, v) + } + resp, err := t.base.RoundTrip(cloned) + if err != nil { + return nil, err + } + switch resp.StatusCode { + case http.StatusUnauthorized: + io.Copy(io.Discard, resp.Body) + resp.Body.Close() + return nil, fmt.Errorf("remote MCP server %q returned HTTP 401 Unauthorized — check your token/header configuration", t.serverName) + case http.StatusForbidden: + io.Copy(io.Discard, resp.Body) + resp.Body.Close() + return nil, fmt.Errorf("remote MCP server %q returned HTTP 403 Forbidden — your credentials may lack required permissions", t.serverName) + } + return resp, nil +} + func (c *Client) Name() string { return c.name } func (c *Client) Tools() []*mcp.Tool { return c.tools } diff --git a/internal/mcp/client_test.go b/internal/mcp/client_test.go index c5f04da90..08b5a4666 100644 --- a/internal/mcp/client_test.go +++ b/internal/mcp/client_test.go @@ -3,11 +3,134 @@ package mcp import ( "context" "encoding/json" + "net/http" + "net/http/httptest" + "strings" "testing" "github.com/modelcontextprotocol/go-sdk/mcp" ) +func TestHeaderTransport(t *testing.T) { + headers := map[string]string{ + "Authorization": "Bearer test-token", + "X-Custom": "custom-value", + } + transport := &headerTransport{ + base: http.DefaultTransport, + headers: headers, + serverName: "test-server", + } + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Authorization"); got != "Bearer test-token" { + t.Errorf("Authorization = %q, want %q", got, "Bearer test-token") + } + if got := r.Header.Get("X-Custom"); got != "custom-value" { + t.Errorf("X-Custom = %q, want %q", got, "custom-value") + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + client := &http.Client{Transport: transport} + resp, err := client.Get(ts.URL) + if err != nil { + t.Fatalf("GET: %v", err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Errorf("status = %d, want 200", resp.StatusCode) + } +} + +func TestHeaderTransport_401(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer ts.Close() + + transport := &headerTransport{ + base: http.DefaultTransport, + headers: map[string]string{"Authorization": "Bearer bad-token"}, + serverName: "auth-server", + } + client := &http.Client{Transport: transport} + _, err := client.Get(ts.URL) + if err == nil { + t.Fatal("expected error for 401, got nil") + } + if got := err.Error(); !strings.Contains(got, "401 Unauthorized") || !strings.Contains(got, "auth-server") { + t.Errorf("error = %q, want mention of 401 and server name", got) + } +} + +func TestHeaderTransport_403(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + defer ts.Close() + + transport := &headerTransport{ + base: http.DefaultTransport, + headers: map[string]string{"Authorization": "Bearer limited-token"}, + serverName: "perm-server", + } + client := &http.Client{Transport: transport} + _, err := client.Get(ts.URL) + if err == nil { + t.Fatal("expected error for 403, got nil") + } + if got := err.Error(); !strings.Contains(got, "403 Forbidden") || !strings.Contains(got, "perm-server") { + t.Errorf("error = %q, want mention of 403 and server name", got) + } +} + +func TestNewRemoteClient_HeaderExpandsToEmpty(t *testing.T) { + t.Setenv("OCR_TEST_EMPTY_VAR", "") + + _, err := NewRemoteClient( + context.Background(), + "test-srv", + "http://localhost:9999/mcp", + map[string]string{"Authorization": "$OCR_TEST_EMPTY_VAR"}, + "v0.0.1-test", + ) + if err == nil { + t.Fatal("expected error when header expands to empty, got nil") + } + if !strings.Contains(err.Error(), "expanded to empty") { + t.Errorf("error = %q, want mention of 'expanded to empty'", err.Error()) + } + if !strings.Contains(err.Error(), "Authorization") { + t.Errorf("error = %q, want mention of header name 'Authorization'", err.Error()) + } +} + +func TestNewRemoteClient_HeaderExpandsUnsetVar(t *testing.T) { + t.Setenv("OCR_TEST_UNSET_MARKER", "") + // Ensure the variable is truly unset (Setenv("", "") sets it to empty; + // os.Expand with os.Getenv returns "" for both unset and empty). + // The point: $OCR_TEST_NONEXISTENT_VAR_XYZ is never set. + + _, err := NewRemoteClient( + context.Background(), + "test-srv", + "http://localhost:9999/mcp", + map[string]string{"X-Token": "$OCR_TEST_NONEXISTENT_VAR_XYZ"}, + "v0.0.1-test", + ) + if err == nil { + t.Fatal("expected error when header references unset env var, got nil") + } + if !strings.Contains(err.Error(), "expanded to empty") { + t.Errorf("error = %q, want mention of 'expanded to empty'", err.Error()) + } + if !strings.Contains(err.Error(), "X-Token") { + t.Errorf("error = %q, want mention of header name 'X-Token'", err.Error()) + } +} + func TestContentToText_SingleText(t *testing.T) { contents := []mcp.Content{ &mcp.TextContent{Text: "hello"},