Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 57 additions & 8 deletions cmd/opencodereview/config_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"encoding/json"
"fmt"
"net/url"
"os"
"path/filepath"
"strconv"
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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.<name>.<field>, custom_providers.<name>.<field>, mcp_servers.<name>.<field>, 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.<name>.<field>, custom_providers.<name>.<field>, mcp_servers.<name>.<field>, 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
}
Expand Down Expand Up @@ -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")
Expand All @@ -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)
}
Comment on lines +608 to +614

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The url.Parse function in Go is very permissive and rarely returns an error. For example, url.Parse("http://") succeeds with Host == "", which would pass this validation but is not a usable endpoint. Consider adding a check for parsed.Host == "" to catch URLs that have a valid scheme but no host.

Suggestion:

Suggested change
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)
}
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)
}

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 {
Expand All @@ -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{}
Expand Down
76 changes: 76 additions & 0 deletions cmd/opencodereview/config_cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
7 changes: 6 additions & 1 deletion cmd/opencodereview/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -339,5 +344,5 @@ Examples:
Supported keys: provider, model, providers.<name>.<field>, custom_providers.<name>.<field>, mcp_servers.<name>.<field>, 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`)
}
20 changes: 20 additions & 0 deletions cmd/opencodereview/review_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +285 to +291

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a header value contains an env var reference like $AUTH_TOKEN and that variable is unset, os.Expand will produce an empty string. The NewRemoteClient function only prints a warning but still proceeds to connect with the empty header value. This could lead to silent authentication failures or, worse, unauthenticated requests being sent to the remote server.

Consider either:

  1. Treating empty-after-expansion headers as a fatal error for this server (skip it), or
  2. At minimum, making the warning more prominent (e.g., ERROR level) so users don't miss it in logs.

This is especially important for Authorization headers where an empty value means the request goes out unauthenticated.

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
Expand Down
94 changes: 91 additions & 3 deletions internal/mcp/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,24 @@ package mcp
import (
"context"
"fmt"
"io"
"net/http"
"os"
"os/exec"
"strings"

"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.
Expand Down Expand Up @@ -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)
}
}
Comment on lines +72 to +77

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a header value expands to an empty string (e.g., because the referenced environment variable is unset), the code prints a warning but still proceeds to set the empty header on outgoing requests. This can lead to silent authentication failures or protocol errors when connecting to the remote MCP server.

Consider either returning an error or skipping the empty header, rather than sending a request known to have invalid headers.

Suggestion:

Suggested change
for k, v := range headers {
expanded[k] = os.Expand(v, os.Getenv)
if expanded[k] == "" {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: MCP server %q header %q expanded to empty value — check your environment variables\n", name, k)
}
}
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 }

Expand Down
Loading
Loading