feat: add standard MCP tool support (#212)

* feat(mcp): add Model Context Protocol server support

Add MCP client and provider packages that allow integrating external
MCP tool servers into the review loop. Includes config commands for
managing MCP servers, stdio subprocess integration tests, and
comprehensive test coverage.

* refactor(mcp): rename loop variable in contentToText to avoid shadowing Client receiver

* fix(mcp): use platform-specific shell for setup command

The MCP server setup command was hardcoded to use `sh -c`, which
fails on Windows. Extract a `shellCommand` helper behind build tags
to use `cmd /c` on Windows and `sh -c` elsewhere.

* docs(mcp): add MCP server documentation to all README locales
This commit is contained in:
kite 2026-07-01 19:10:16 +08:00 committed by GitHub
parent 8e049189f7
commit 6adcb1ecd4
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
23 changed files with 1616 additions and 19 deletions

View file

@ -94,17 +94,23 @@ func runConfigSet(key, value string) error {
func runConfigUnset(key string) error {
parts := strings.SplitN(key, ".", 2)
if len(parts) != 2 || parts[0] != "custom_providers" || parts[1] == "" {
return fmt.Errorf("unset only supports custom_providers.<name>")
if len(parts) != 2 || parts[1] == "" {
return fmt.Errorf("unset supports custom_providers.<name> and mcp_servers.<name>")
}
name := parts[1]
configPath, err := defaultConfigPath()
if err != nil {
return err
}
return unsetCustomProvider(configPath, name)
switch parts[0] {
case "custom_providers":
return unsetCustomProvider(configPath, parts[1])
case "mcp_servers":
return unsetMCPServer(configPath, parts[1])
default:
return fmt.Errorf("unset supports custom_providers.<name> and mcp_servers.<name>")
}
}
func unsetCustomProvider(configPath, name string) error {
@ -130,6 +136,32 @@ func unsetCustomProvider(configPath, name string) error {
return nil
}
func unsetMCPServer(configPath, name string) error {
cfg, err := loadOrCreateConfig(configPath)
if err != nil {
return fmt.Errorf("load config: %w", err)
}
if cfg.MCPServers == nil {
return fmt.Errorf("MCP server %q not found", name)
}
if _, exists := cfg.MCPServers[name]; !exists {
return fmt.Errorf("MCP server %q not found", name)
}
delete(cfg.MCPServers, name)
if len(cfg.MCPServers) == 0 {
cfg.MCPServers = nil
}
if err := saveConfig(configPath, cfg); err != nil {
return err
}
fmt.Printf("Deleted MCP server %q.\n", name)
return nil
}
// deleteCustomProvider removes a custom provider from cfg in memory.
// Returns true if the deleted provider was the active one.
func deleteCustomProvider(cfg *Config, name string) (bool, error) {
@ -166,15 +198,25 @@ type ProviderEntry struct {
ExtraHeaders map[string]string `json:"extra_headers,omitempty"`
}
// MCPServerConfig holds configuration for a single MCP server (stdio transport).
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"`
}
// Config represents the user-level configuration file (~/.opencodereview/config.json).
type Config struct {
Provider string `json:"provider,omitempty"`
Model string `json:"model,omitempty"`
Providers map[string]ProviderEntry `json:"providers,omitempty"`
CustomProviders map[string]ProviderEntry `json:"custom_providers,omitempty"`
Llm LlmConfig `json:"llm,omitempty"`
Language string `json:"language,omitempty"`
Telemetry *TelemetryConfig `json:"telemetry,omitempty"`
Provider string `json:"provider,omitempty"`
Model string `json:"model,omitempty"`
Providers map[string]ProviderEntry `json:"providers,omitempty"`
CustomProviders map[string]ProviderEntry `json:"custom_providers,omitempty"`
Llm LlmConfig `json:"llm,omitempty"`
Language string `json:"language,omitempty"`
Telemetry *TelemetryConfig `json:"telemetry,omitempty"`
MCPServers map[string]MCPServerConfig `json:"mcp_servers,omitempty"`
}
type LlmConfig struct {
@ -234,6 +276,9 @@ func setConfigValue(cfg *Config, key, value string) error {
if strings.HasPrefix(key, "custom_providers.") {
return setCustomProviderValue(cfg, key, value)
}
if strings.HasPrefix(key, "mcp_servers.") {
return setMCPServerValue(cfg, key, value)
}
switch key {
case "provider":
@ -329,7 +374,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>, llm.url, llm.auth_token, llm.auth_header, llm.model, 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", 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.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\nMCP server fields: command, args, env, tools, setup", key)
}
return nil
}
@ -491,6 +536,70 @@ func setCustomProviderField(cfg *Config, name, field, key, value string) error {
return nil
}
func setMCPServerValue(cfg *Config, key, value string) error {
parts := strings.SplitN(key, ".", 3)
if len(parts) != 3 || parts[1] == "" || parts[2] == "" {
return fmt.Errorf("invalid MCP server key %q: expected mcp_servers.<name>.<field>", key)
}
name, field := parts[1], parts[2]
if cfg.MCPServers == nil {
cfg.MCPServers = make(map[string]MCPServerConfig)
}
entry := cfg.MCPServers[name]
switch field {
case "command":
if value == "" {
return fmt.Errorf("MCP server command cannot be empty")
}
entry.Command = value
case "args":
var args []string
if err := json.Unmarshal([]byte(value), &args); err != nil {
return fmt.Errorf("invalid JSON array for %s: %w", key, err)
}
entry.Args = args
case "env":
var env []string
if err := json.Unmarshal([]byte(value), &env); err != nil {
return fmt.Errorf("invalid JSON array for %s: %w", key, err)
}
for _, e := range env {
idx := strings.Index(e, "=")
if idx <= 0 {
return fmt.Errorf("invalid env entry %q: must be in KEY=VALUE format", e)
}
}
entry.Env = env
case "tools":
var tools []string
if err := json.Unmarshal([]byte(value), &tools); err != nil {
return fmt.Errorf("invalid JSON array for %s: %w", key, err)
}
seen := make(map[string]struct{}, len(tools))
filtered := make([]string, 0, len(tools))
for _, t := range tools {
if t == "" {
return fmt.Errorf("tool names in %s must not be empty", key)
}
if _, dup := seen[t]; dup {
continue
}
seen[t] = struct{}{}
filtered = append(filtered, t)
}
entry.Tools = filtered
case "setup":
entry.Setup = value
default:
return fmt.Errorf("unknown MCP server field %q: supported fields are command, args, env, tools, setup", field)
}
cfg.MCPServers[name] = entry
return nil
}
func (c *Config) ensureTelemetry() {
if c.Telemetry == nil {
c.Telemetry = &TelemetryConfig{}

View file

@ -445,6 +445,236 @@ func TestMergeModelLists(t *testing.T) {
}
}
func TestSetMCPServerValue_Command(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.command", "npx"); err != nil {
t.Fatalf("setMCPServerValue: %v", err)
}
if cfg.MCPServers["my-server"].Command != "npx" {
t.Errorf("Command = %q, want %q", cfg.MCPServers["my-server"].Command, "npx")
}
}
func TestSetMCPServerValue_CommandEmpty(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.command", ""); err == nil {
t.Fatal("expected error for empty command")
}
}
func TestSetMCPServerValue_Args(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.args", `["--port","8080"]`); err != nil {
t.Fatalf("setMCPServerValue: %v", err)
}
args := cfg.MCPServers["my-server"].Args
if len(args) != 2 || args[0] != "--port" || args[1] != "8080" {
t.Errorf("Args = %v", args)
}
}
func TestSetMCPServerValue_ArgsInvalidJSON(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.args", "not-json"); err == nil {
t.Fatal("expected error for invalid JSON")
}
}
func TestSetMCPServerValue_Env(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.env", `["FOO=bar","BAZ=qux"]`); err != nil {
t.Fatalf("setMCPServerValue: %v", err)
}
env := cfg.MCPServers["my-server"].Env
if len(env) != 2 || env[0] != "FOO=bar" {
t.Errorf("Env = %v", env)
}
}
func TestSetMCPServerValue_EnvInvalidJSON(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.env", "not-json"); err == nil {
t.Fatal("expected error for invalid JSON")
}
}
func TestSetMCPServerValue_EnvInvalidFormat(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.env", `["NOEQUALS"]`); err == nil {
t.Fatal("expected error for env entry without KEY=VALUE format")
}
}
func TestSetMCPServerValue_Tools(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.tools", `["search","read","search"]`); err != nil {
t.Fatalf("setMCPServerValue: %v", err)
}
tools := cfg.MCPServers["my-server"].Tools
if len(tools) != 2 || tools[0] != "search" || tools[1] != "read" {
t.Errorf("Tools = %v (expected deduped)", tools)
}
}
func TestSetMCPServerValue_ToolsInvalidJSON(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.tools", "not-json"); err == nil {
t.Fatal("expected error for invalid JSON")
}
}
func TestSetMCPServerValue_ToolsEmptyName(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.tools", `["search",""]`); err == nil {
t.Fatal("expected error for empty tool name")
}
}
func TestSetMCPServerValue_Setup(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.setup", "init-script.sh"); err != nil {
t.Fatalf("setMCPServerValue: %v", err)
}
if cfg.MCPServers["my-server"].Setup != "init-script.sh" {
t.Errorf("Setup = %q", cfg.MCPServers["my-server"].Setup)
}
}
func TestSetMCPServerValue_UnknownField(t *testing.T) {
cfg := &Config{}
if err := setMCPServerValue(cfg, "mcp_servers.my-server.unknown", "val"); err == nil {
t.Fatal("expected error for unknown field")
}
}
func TestSetMCPServerValue_InvalidKey(t *testing.T) {
cfg := &Config{}
tests := []string{
"mcp_servers",
"mcp_servers.",
"mcp_servers..command",
"mcp_servers.name",
}
for _, key := range tests {
if err := setMCPServerValue(cfg, key, "val"); err == nil {
t.Errorf("expected error for key %q", key)
}
}
}
func TestSetMCPServerValue_ExistingServer(t *testing.T) {
cfg := &Config{
MCPServers: map[string]MCPServerConfig{
"srv": {Command: "old-cmd"},
},
}
if err := setMCPServerValue(cfg, "mcp_servers.srv.command", "new-cmd"); err != nil {
t.Fatalf("setMCPServerValue: %v", err)
}
if cfg.MCPServers["srv"].Command != "new-cmd" {
t.Errorf("Command = %q, want %q", cfg.MCPServers["srv"].Command, "new-cmd")
}
}
func TestUnsetMCPServer(t *testing.T) {
dir := t.TempDir()
configPath := dir + "/config.json"
cfg := &Config{
MCPServers: map[string]MCPServerConfig{
"srv1": {Command: "cmd1"},
"srv2": {Command: "cmd2"},
},
}
if err := saveConfig(configPath, cfg); err != nil {
t.Fatalf("saveConfig: %v", err)
}
if err := unsetMCPServer(configPath, "srv1"); err != nil {
t.Fatalf("unsetMCPServer: %v", err)
}
cfg, err := loadOrCreateConfig(configPath)
if err != nil {
t.Fatalf("reload: %v", err)
}
if _, exists := cfg.MCPServers["srv1"]; exists {
t.Error("srv1 should have been deleted")
}
if _, exists := cfg.MCPServers["srv2"]; !exists {
t.Error("srv2 should still exist")
}
}
func TestUnsetMCPServer_LastEntry(t *testing.T) {
dir := t.TempDir()
configPath := dir + "/config.json"
cfg := &Config{
MCPServers: map[string]MCPServerConfig{
"only": {Command: "cmd"},
},
}
if err := saveConfig(configPath, cfg); err != nil {
t.Fatalf("saveConfig: %v", err)
}
if err := unsetMCPServer(configPath, "only"); err != nil {
t.Fatalf("unsetMCPServer: %v", err)
}
cfg, err := loadOrCreateConfig(configPath)
if err != nil {
t.Fatalf("reload: %v", err)
}
if cfg.MCPServers != nil {
t.Errorf("MCPServers should be nil after deleting last entry, got %v", cfg.MCPServers)
}
}
func TestUnsetMCPServer_NotFound(t *testing.T) {
dir := t.TempDir()
configPath := dir + "/config.json"
cfg := &Config{}
if err := saveConfig(configPath, cfg); err != nil {
t.Fatalf("saveConfig: %v", err)
}
if err := unsetMCPServer(configPath, "nonexistent"); err == nil {
t.Fatal("expected error for nil MCPServers")
}
cfg = &Config{
MCPServers: map[string]MCPServerConfig{
"other": {Command: "cmd"},
},
}
if err := saveConfig(configPath, cfg); err != nil {
t.Fatalf("saveConfig: %v", err)
}
if err := unsetMCPServer(configPath, "nonexistent"); err == nil {
t.Fatal("expected error for missing server")
}
}
func TestRunConfigUnset_UnknownPrefix(t *testing.T) {
if err := runConfigUnset("providers.anthropic"); err == nil {
t.Fatal("expected error for unsupported prefix")
}
}
func TestSetConfigValueMCPServer(t *testing.T) {
cfg := &Config{}
if err := setConfigValue(cfg, "mcp_servers.my-server.command", "npx"); err != nil {
t.Fatalf("setConfigValue: %v", err)
}
if cfg.MCPServers["my-server"].Command != "npx" {
t.Errorf("Command = %q", cfg.MCPServers["my-server"].Command)
}
}
func TestEnsureTelemetry(t *testing.T) {
cfg := &Config{}
if cfg.Telemetry != nil {

View file

@ -275,6 +275,7 @@ func printConfigUsage() {
Usage:
ocr config set <key> <value>
ocr config unset custom_providers.<name> Delete a custom provider
ocr config unset mcp_servers.<name> Delete an MCP server
ocr config provider Interactive provider setup
ocr config model Interactive model selection
@ -301,6 +302,14 @@ Examples:
# Delete a custom provider
ocr config unset custom_providers.my-gateway
# MCP server configuration (stdio transport)
ocr config set mcp_servers.codegraph.command npx
ocr config set mcp_servers.codegraph.args '["-y","@anthropic/codegraph-mcp"]'
ocr config set mcp_servers.codegraph.env '["CODEGRAPH_TOKEN=xxx"]'
# Delete an MCP server
ocr config unset mcp_servers.codegraph
# Legacy endpoint configuration
ocr config set llm.url https://xx/v1/openai/chat/completions
ocr config set llm.auth_token xxxxxxxxxx
@ -310,6 +319,7 @@ Examples:
ocr config set language English
ocr config set telemetry.enabled true
Supported keys: provider, model, providers.<name>.<field>, custom_providers.<name>.<field>, llm.url, llm.auth_token, llm.auth_header, llm.model, llm.use_anthropic, llm.extra_body, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging
Provider fields: api_key, url, protocol, model, models, auth_header, extra_body`)
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.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
MCP server fields: command, args, env, tools, setup`)
}

View file

@ -0,0 +1,18 @@
//go:build !windows
package main
import (
"os/exec"
"syscall"
)
func configureProcessGroup(cmd *exec.Cmd) {
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
cmd.Cancel = func() error {
if cmd.Process == nil {
return nil
}
return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
}
}

View file

@ -0,0 +1,12 @@
//go:build windows
package main
import "os/exec"
func configureProcessGroup(cmd *exec.Cmd) {
// On Windows, exec.CommandContext sends os.Kill which terminates the
// direct child. Grandchild processes (e.g. from sh -c) may survive.
// Full process-tree cleanup would need Windows Job Objects, but sh -c
// is rare on Windows so this is an acceptable limitation.
}

View file

@ -5,10 +5,12 @@ import (
"fmt"
"os"
"path/filepath"
"sort"
"strings"
"time"
"github.com/open-code-review/open-code-review/internal/agent"
"github.com/open-code-review/open-code-review/internal/mcp"
"github.com/open-code-review/open-code-review/internal/telemetry"
"github.com/open-code-review/open-code-review/internal/tool"
)
@ -61,6 +63,19 @@ func runReview(args []string) error {
}
tools := buildToolRegistry(rt.Collector, fileReader)
mcpClients := initMCPClients(context.Background(), rt.AppCfg, tools, cc.RepoDir, Version)
defer func() {
for _, mc := range mcpClients {
if err := mc.Close(); err != nil {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: failed to close MCP server %q: %v\n", mc.Name(), err)
}
}
}()
mcpToolDefs := mcp.CollectToolDefs(mcpClients, tools)
rt.PlanToolDefs = append(rt.PlanToolDefs, mcpToolDefs...)
rt.MainToolDefs = append(rt.MainToolDefs, mcpToolDefs...)
ag := agent.New(agent.Args{
RepoDir: cc.RepoDir,
From: opts.from,
@ -180,6 +195,58 @@ func runPreview(cc *commonContext, opts reviewOptions) error {
return nil
}
func initMCPClients(ctx context.Context, cfg *Config, tools *tool.Registry, repoDir, version string) []*mcp.Client {
if cfg == nil || len(cfg.MCPServers) == 0 {
return nil
}
mcpNames := make([]string, 0, len(cfg.MCPServers))
for name := range cfg.MCPServers {
mcpNames = append(mcpNames, name)
}
sort.Strings(mcpNames)
var clients []*mcp.Client
for _, name := range mcpNames {
serverCfg := cfg.MCPServers[name]
if serverCfg.Command == "" {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: MCP server %q has no command configured, skipping\n", name)
continue
}
if serverCfg.Setup != "" {
fmt.Fprintf(os.Stderr, "[ocr] Running setup for MCP server %q: %s\n", name, serverCfg.Setup)
setupCtx, setupCancel := context.WithTimeout(ctx, 5*time.Minute)
setupCmd := shellCommand(setupCtx, serverCfg.Setup)
setupCmd.Dir = repoDir
configureProcessGroup(setupCmd)
output, err := setupCmd.CombinedOutput()
setupCancel()
if err != nil {
fmt.Fprintf(os.Stderr, "[ocr] ERROR: MCP server %q setup command failed.\n", name)
fmt.Fprintf(os.Stderr, "[ocr] Command: %s\n", serverCfg.Setup)
fmt.Fprintf(os.Stderr, "[ocr] Working directory: %s\n", repoDir)
fmt.Fprintf(os.Stderr, "[ocr] Error: %v\n", err)
if len(output) > 0 {
fmt.Fprintf(os.Stderr, "[ocr] Output:\n%s\n", string(output))
}
fmt.Fprintf(os.Stderr, "[ocr] Skipping MCP server %q — review will proceed without it.\n", name)
continue
}
}
initCtx, initCancel := context.WithTimeout(ctx, 30*time.Second)
mc, err := mcp.NewClient(initCtx, name, serverCfg.Command, serverCfg.Args, serverCfg.Env, repoDir, version)
initCancel()
if err != nil {
fmt.Fprintf(os.Stderr, "[ocr] WARNING: failed to start MCP server %q: %v\n", name, err)
continue
}
clients = append(clients, mc)
mcp.RegisterAll(tools, mc, serverCfg.Tools)
}
return clients
}
func buildToolRegistry(collector *tool.CommentCollector, fr *tool.FileReader) *tool.Registry {
reg := tool.NewRegistry()
reg.Register(tool.NewFileRead(fr))

View file

@ -0,0 +1,12 @@
//go:build !windows
package main
import (
"context"
"os/exec"
)
func shellCommand(ctx context.Context, script string) *exec.Cmd {
return exec.CommandContext(ctx, "sh", "-c", script)
}

View file

@ -0,0 +1,12 @@
//go:build windows
package main
import (
"context"
"os/exec"
)
func shellCommand(ctx context.Context, script string) *exec.Cmd {
return exec.CommandContext(ctx, "cmd", "/c", script)
}