diff --git a/README.ja-JP.md b/README.ja-JP.md index 8c67db0..2e9d8c3 100644 --- a/README.ja-JP.md +++ b/README.ja-JP.md @@ -673,6 +673,11 @@ OCRは4層の優先度チェーンを使ってレビュールールを解決し | `llm.extra_headers` | string | カンマ区切りの `key=value` HTTPヘッダー | | `llm.model` | string | `claude-opus-4-6` | | `llm.use_anthropic` | boolean | `true` \| `false` | +| `mcp_servers..command` | string | MCPサーバーを起動するコマンド | +| `mcp_servers..args` | array | MCPサーバーのコマンドライン引数 | +| `mcp_servers..env` | array | 環境変数(`KEY=VALUE`形式) | +| `mcp_servers..tools` | array | 許可するツール名(空の場合はすべてのツール) | +| `mcp_servers..setup` | string | サーバー起動前に実行するセットアップコマンド | | `language` | string | 任意の言語名、例:`English`、`Chinese`(デフォルト:`English`) | | `telemetry.enabled` | boolean | `true` \| `false` | | `telemetry.exporter` | string | `console` \| `otlp` | @@ -681,6 +686,43 @@ OCRは4層の優先度チェーンを使ってレビュールールを解決し 環境変数は設定ファイルより優先されます。 +### MCPサーバー + +Open Code Reviewは[Model Context Protocol (MCP)](https://modelcontextprotocol.io/)サーバーをサポートしており、レビューエージェントがstdioトランスポートを介してコードレビュー中に外部ツールを使用できます。 + +CLIからMCPサーバーを設定します: + +```bash +# MCPサーバーを追加 +ocr config set mcp_servers..command +ocr config set mcp_servers..args '["arg1","arg2"]' +ocr config set mcp_servers..env '["KEY=VALUE"]' +ocr config set mcp_servers..tools '["tool_name"]' +ocr config set mcp_servers..setup '' + +# MCPサーバーを削除 +ocr config unset mcp_servers. +``` + +| フィールド | 必須 | 説明 | +|-----------|------|------| +| `command` | はい | MCPサーバーを起動する実行コマンド | +| `args` | いいえ | サーバーに渡すコマンドライン引数 | +| `env` | いいえ | 環境変数(`KEY=VALUE`形式) | +| `tools` | いいえ | 許可するツール名。空の場合、サーバーのすべてのツールが利用可能 | +| `setup` | いいえ | サーバー起動前に実行するシェルコマンド(例:インデックスの構築) | + +> **注意:** MCPツールの名前が組み込みツールと競合する場合、そのツールは警告付きでスキップされます。`setup`コマンドのタイムアウトは5分です。 + +**例:[CodeGraph](https://github.com/nicholasgasior/codegraph)を追加してコード構造分析を強化** + +```bash +ocr config set mcp_servers.codegraph.command codegraph +ocr config set mcp_servers.codegraph.args '["serve","--mcp"]' +ocr config set mcp_servers.codegraph.tools '["codegraph_explore"]' +ocr config set mcp_servers.codegraph.setup 'codegraph init && codegraph index' +``` + ### 環境変数 | 変数 | 用途 | diff --git a/README.ko-KR.md b/README.ko-KR.md index 90b7225..06302f1 100644 --- a/README.ko-KR.md +++ b/README.ko-KR.md @@ -631,6 +631,11 @@ Config file: `~/.opencodereview/config.json` | `llm.extra_headers` | string | 쉼표로 구분된 `key=value` HTTP 헤더 | | `llm.model` | string | `claude-opus-4-6` | | `llm.use_anthropic` | boolean | `true` \| `false` | +| `mcp_servers..command` | string | MCP 서버를 시작하는 명령어 | +| `mcp_servers..args` | array | MCP 서버의 커맨드라인 인수 | +| `mcp_servers..env` | array | 환경 변수 (`KEY=VALUE` 형식) | +| `mcp_servers..tools` | array | 허용할 도구 이름 (비어 있으면 모든 도구 허용) | +| `mcp_servers..setup` | string | 서버 시작 전에 실행할 설정 명령어 | | `language` | string | 임의의 언어 이름, 예: `English`, `Chinese` (기본값: `English`) | | `telemetry.enabled` | boolean | `true` \| `false` | | `telemetry.exporter` | string | `console` \| `otlp` | @@ -639,6 +644,43 @@ Config file: `~/.opencodereview/config.json` 환경 변수는 config file보다 우선합니다. +### MCP Server + +Open Code Review는 [Model Context Protocol (MCP)](https://modelcontextprotocol.io/) 서버를 지원하여 리뷰 에이전트가 stdio 전송을 통해 코드 리뷰 중에 외부 도구를 사용할 수 있습니다. + +CLI로 MCP 서버를 설정합니다: + +```bash +# MCP 서버 추가 +ocr config set mcp_servers..command +ocr config set mcp_servers..args '["arg1","arg2"]' +ocr config set mcp_servers..env '["KEY=VALUE"]' +ocr config set mcp_servers..tools '["tool_name"]' +ocr config set mcp_servers..setup '' + +# MCP 서버 삭제 +ocr config unset mcp_servers. +``` + +| 필드 | 필수 | 설명 | +|------|------|------| +| `command` | 예 | MCP 서버를 시작하는 실행 명령어 | +| `args` | 아니오 | 서버에 전달할 커맨드라인 인수 | +| `env` | 아니오 | 환경 변수 (`KEY=VALUE` 형식) | +| `tools` | 아니오 | 허용할 도구 이름. 비어 있으면 서버의 모든 도구 사용 가능 | +| `setup` | 아니오 | 서버 시작 전에 실행할 셸 명령어 (예: 인덱스 빌드) | + +> **참고:** MCP 도구의 이름이 내장 도구와 충돌하면 경고와 함께 건너뜁니다. `setup` 명령어의 타임아웃은 5분입니다. + +**예시: [CodeGraph](https://github.com/nicholasgasior/codegraph)를 추가하여 코드 구조 분석 강화** + +```bash +ocr config set mcp_servers.codegraph.command codegraph +ocr config set mcp_servers.codegraph.args '["serve","--mcp"]' +ocr config set mcp_servers.codegraph.tools '["codegraph_explore"]' +ocr config set mcp_servers.codegraph.setup 'codegraph init && codegraph index' +``` + ### Environment Variables | Variable | Purpose | diff --git a/README.md b/README.md index 4186fe0..8910699 100644 --- a/README.md +++ b/README.md @@ -678,6 +678,11 @@ Config file: `~/.opencodereview/config.json` | `llm.extra_headers` | string | Comma-separated `key=value` HTTP headers | | `llm.model` | string | `claude-opus-4-6` | | `llm.use_anthropic` | boolean | `true` \| `false` | +| `mcp_servers..command` | string | Command to start the MCP server | +| `mcp_servers..args` | array | Command-line arguments for the MCP server | +| `mcp_servers..env` | array | Environment variables in `KEY=VALUE` format | +| `mcp_servers..tools` | array | Allowed tool names (empty = all tools) | +| `mcp_servers..setup` | string | Setup command to run before starting the server | | `language` | string | Any language name, e.g. `English`, `Chinese` (default: `English`) | | `telemetry.enabled` | boolean | `true` \| `false` | | `telemetry.exporter` | string | `console` \| `otlp` | @@ -686,6 +691,43 @@ Config file: `~/.opencodereview/config.json` Environment variables take precedence over the config file. +### MCP Server + +Open Code Review supports [Model Context Protocol (MCP)](https://modelcontextprotocol.io/) servers, allowing the review agent to use external tools during code review via the stdio transport. + +Configure MCP servers via the CLI: + +```bash +# Add an MCP server +ocr config set mcp_servers..command +ocr config set mcp_servers..args '["arg1","arg2"]' +ocr config set mcp_servers..env '["KEY=VALUE"]' +ocr config set mcp_servers..tools '["tool_name"]' +ocr config set mcp_servers..setup '' + +# Delete an MCP server +ocr config unset mcp_servers. +``` + +| Field | Required | Description | +|-------|----------|-------------| +| `command` | Yes | The executable command to start the MCP server | +| `args` | No | Command-line arguments passed to the server | +| `env` | No | Environment variables in `KEY=VALUE` format | +| `tools` | No | Allowed tool names; if empty, all tools from the server are available | +| `setup` | No | A shell command to run before starting the server (e.g. build an index) | + +> **Note:** If an MCP tool's name conflicts with a built-in tool, it will be skipped with a warning. The `setup` command has a 5-minute timeout. + +**Example: Add [CodeGraph](https://github.com/nicholasgasior/codegraph) for code structure analysis** + +```bash +ocr config set mcp_servers.codegraph.command codegraph +ocr config set mcp_servers.codegraph.args '["serve","--mcp"]' +ocr config set mcp_servers.codegraph.tools '["codegraph_explore"]' +ocr config set mcp_servers.codegraph.setup 'codegraph init && codegraph index' +``` + ### Environment Variables | Variable | Purpose | diff --git a/README.ru-RU.md b/README.ru-RU.md index c22b8e0..7034883 100644 --- a/README.ru-RU.md +++ b/README.ru-RU.md @@ -675,6 +675,11 @@ OCR разрешает правила ревью по цепочке приор | `llm.extra_headers` | string | HTTP-заголовки `key=value` через запятую | | `llm.model` | string | `claude-opus-4-6` | | `llm.use_anthropic` | boolean | `true` \| `false` | +| `mcp_servers..command` | string | Команда для запуска MCP-сервера | +| `mcp_servers..args` | array | Аргументы командной строки для MCP-сервера | +| `mcp_servers..env` | array | Переменные окружения в формате `KEY=VALUE` | +| `mcp_servers..tools` | array | Разрешённые имена инструментов (пусто = все инструменты) | +| `mcp_servers..setup` | string | Команда настройки перед запуском сервера | | `language` | string | Любое название языка, например `English`, `Chinese` (по умолчанию: `English`) | | `telemetry.enabled` | boolean | `true` \| `false` | | `telemetry.exporter` | string | `console` \| `otlp` | @@ -683,6 +688,43 @@ OCR разрешает правила ревью по цепочке приор Переменные окружения имеют приоритет над файлом конфигурации. +### MCP-сервер + +Open Code Review поддерживает серверы [Model Context Protocol (MCP)](https://modelcontextprotocol.io/), позволяя агенту ревью использовать внешние инструменты во время проверки кода через stdio-транспорт. + +Настройка MCP-серверов через CLI: + +```bash +# Добавить MCP-сервер +ocr config set mcp_servers..command +ocr config set mcp_servers..args '["arg1","arg2"]' +ocr config set mcp_servers..env '["KEY=VALUE"]' +ocr config set mcp_servers..tools '["tool_name"]' +ocr config set mcp_servers..setup '' + +# Удалить MCP-сервер +ocr config unset mcp_servers. +``` + +| Поле | Обязательно | Описание | +|------|-------------|----------| +| `command` | Да | Исполняемая команда для запуска MCP-сервера | +| `args` | Нет | Аргументы командной строки для сервера | +| `env` | Нет | Переменные окружения в формате `KEY=VALUE` | +| `tools` | Нет | Разрешённые имена инструментов; если пусто — доступны все инструменты сервера | +| `setup` | Нет | Shell-команда для выполнения перед запуском сервера (например, построение индекса) | + +> **Примечание:** Если имя MCP-инструмента конфликтует со встроенным инструментом, он будет пропущен с предупреждением. Таймаут команды `setup` составляет 5 минут. + +**Пример: добавление [CodeGraph](https://github.com/nicholasgasior/codegraph) для усиления анализа структуры кода** + +```bash +ocr config set mcp_servers.codegraph.command codegraph +ocr config set mcp_servers.codegraph.args '["serve","--mcp"]' +ocr config set mcp_servers.codegraph.tools '["codegraph_explore"]' +ocr config set mcp_servers.codegraph.setup 'codegraph init && codegraph index' +``` + ### Переменные окружения | Переменная | Назначение | diff --git a/README.zh-CN.md b/README.zh-CN.md index 01df8a1..83524ea 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -663,6 +663,11 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则 | `llm.extra_headers` | string | 逗号分隔的 `key=value` HTTP 头 | | `llm.model` | string | `claude-opus-4-6` | | `llm.use_anthropic` | boolean | `true` \| `false` | +| `mcp_servers..command` | string | 启动 MCP 服务器的命令 | +| `mcp_servers..args` | array | MCP 服务器的命令行参数 | +| `mcp_servers..env` | array | 环境变量,`KEY=VALUE` 格式 | +| `mcp_servers..tools` | array | 允许使用的工具名称(为空则允许所有工具) | +| `mcp_servers..setup` | string | 启动服务器前运行的初始化命令 | | `language` | string | 任意语言名称,例如 `English`、`Chinese`(默认:`English`) | | `telemetry.enabled` | boolean | `true` \| `false` | | `telemetry.exporter` | string | `console` \| `otlp` | @@ -671,6 +676,43 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则 环境变量优先级高于配置文件。 +### MCP Server + +Open Code Review 支持 [Model Context Protocol (MCP)](https://modelcontextprotocol.io/) 服务器,允许评审 Agent 在代码评审过程中通过 stdio 传输协议调用外部工具。 + +通过 CLI 配置 MCP 服务器: + +```bash +# 添加 MCP 服务器 +ocr config set mcp_servers..command +ocr config set mcp_servers..args '["arg1","arg2"]' +ocr config set mcp_servers..env '["KEY=VALUE"]' +ocr config set mcp_servers..tools '["tool_name"]' +ocr config set mcp_servers..setup '' + +# 删除 MCP 服务器 +ocr config unset mcp_servers. +``` + +| 字段 | 必填 | 说明 | +|------|------|------| +| `command` | 是 | 启动 MCP 服务器的可执行命令 | +| `args` | 否 | 传递给服务器的命令行参数 | +| `env` | 否 | 环境变量,`KEY=VALUE` 格式 | +| `tools` | 否 | 允许使用的工具名称;为空则服务器的所有工具均可用 | +| `setup` | 否 | 启动服务器前运行的 shell 命令(例如构建索引) | + +> **注意:** 如果 MCP 工具的名称与内置工具冲突,该工具将被跳过并输出警告。`setup` 命令的超时时间为 5 分钟。 + +**示例:添加 [CodeGraph](https://github.com/nicholasgasior/codegraph) 增强代码结构分析能力** + +```bash +ocr config set mcp_servers.codegraph.command codegraph +ocr config set mcp_servers.codegraph.args '["serve","--mcp"]' +ocr config set mcp_servers.codegraph.tools '["codegraph_explore"]' +ocr config set mcp_servers.codegraph.setup 'codegraph init && codegraph index' +``` + ### 环境变量 | 变量 | 用途 | diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 41cac15..64b511b 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -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.") + if len(parts) != 2 || parts[1] == "" { + return fmt.Errorf("unset supports custom_providers. and mcp_servers.") } - 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. and mcp_servers.") + } } 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.., custom_providers.., 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.., custom_providers.., mcp_servers.., 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..", 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{} diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index af1e43a..8c7de98 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -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 { diff --git a/cmd/opencodereview/flags.go b/cmd/opencodereview/flags.go index 4432ceb..ccda515 100644 --- a/cmd/opencodereview/flags.go +++ b/cmd/opencodereview/flags.go @@ -275,6 +275,7 @@ func printConfigUsage() { Usage: ocr config set ocr config unset custom_providers. Delete a custom provider + ocr config unset mcp_servers. 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.., custom_providers.., 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.., custom_providers.., mcp_servers.., 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`) } diff --git a/cmd/opencodereview/procattr_unix.go b/cmd/opencodereview/procattr_unix.go new file mode 100644 index 0000000..6861ead --- /dev/null +++ b/cmd/opencodereview/procattr_unix.go @@ -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) + } +} diff --git a/cmd/opencodereview/procattr_windows.go b/cmd/opencodereview/procattr_windows.go new file mode 100644 index 0000000..4b947be --- /dev/null +++ b/cmd/opencodereview/procattr_windows.go @@ -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. +} diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index 91a7491..d76951b 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -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)) diff --git a/cmd/opencodereview/shell_unix.go b/cmd/opencodereview/shell_unix.go new file mode 100644 index 0000000..a66ce86 --- /dev/null +++ b/cmd/opencodereview/shell_unix.go @@ -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) +} diff --git a/cmd/opencodereview/shell_windows.go b/cmd/opencodereview/shell_windows.go new file mode 100644 index 0000000..bfb0311 --- /dev/null +++ b/cmd/opencodereview/shell_windows.go @@ -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) +} diff --git a/go.mod b/go.mod index 3f48112..84dc3a3 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/open-code-review/open-code-review -go 1.25.0 +go 1.25.5 require ( charm.land/bubbles/v2 v2.1.0 @@ -8,6 +8,7 @@ require ( charm.land/lipgloss/v2 v2.0.4 github.com/anthropics/anthropic-sdk-go v1.52.0 github.com/bmatcuk/doublestar/v4 v4.10.0 + github.com/modelcontextprotocol/go-sdk v1.6.1 github.com/openai/openai-go/v3 v3.41.0 github.com/pkoukk/tiktoken-go v0.1.8 go.opentelemetry.io/otel v1.44.0 @@ -35,9 +36,10 @@ require ( github.com/charmbracelet/x/windows v0.2.2 // indirect github.com/clipperhouse/displaywidth v0.11.0 // indirect github.com/clipperhouse/uax29/v2 v2.7.0 // indirect - github.com/dlclark/regexp2 v1.10.0 // indirect + github.com/dlclark/regexp2 v1.11.0 // indirect github.com/go-logr/logr v1.4.3 // indirect github.com/go-logr/stdr v1.2.2 // indirect + github.com/google/jsonschema-go v0.4.3 // indirect github.com/google/uuid v1.6.0 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.29.0 // indirect github.com/invopop/jsonschema v0.14.0 // indirect @@ -46,17 +48,21 @@ require ( github.com/muesli/cancelreader v0.2.2 // indirect github.com/pb33f/ordered-map/v2 v2.3.1 // indirect github.com/rivo/uniseg v0.4.7 // indirect + github.com/segmentio/asm v1.1.3 // indirect + github.com/segmentio/encoding v0.5.4 // indirect github.com/standard-webhooks/standard-webhooks/libraries v0.0.1 // indirect github.com/tidwall/gjson v1.18.0 // indirect github.com/tidwall/match v1.1.1 // indirect github.com/tidwall/pretty v1.2.1 // indirect github.com/tidwall/sjson v1.2.5 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect + github.com/yosida95/uritemplate/v3 v3.0.2 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.44.0 // indirect go.opentelemetry.io/proto/otlp v1.10.0 // indirect go.yaml.in/yaml/v4 v4.0.0-rc.2 // indirect golang.org/x/net v0.55.0 // indirect + golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.45.0 // indirect golang.org/x/text v0.37.0 // indirect diff --git a/go.sum b/go.sum index 5085065..bb44b6b 100644 --- a/go.sum +++ b/go.sum @@ -40,8 +40,8 @@ github.com/clipperhouse/uax29/v2 v2.7.0 h1:+gs4oBZ2gPfVrKPthwbMzWZDaAFPGYK72F0NJ github.com/clipperhouse/uax29/v2 v2.7.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/dlclark/regexp2 v1.10.0 h1:+/GIL799phkJqYW+3YbOd8LCcbHzT0Pbo8zl70MHsq0= -github.com/dlclark/regexp2 v1.10.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= +github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= +github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/dnaeon/go-vcr v1.2.0 h1:zHCHvJYTMh1N7xnV7zf1m1GPBF9Ad0Jk/whtQ1663qI= github.com/dnaeon/go-vcr v1.2.0/go.mod h1:R4UdLID7HZT3taECzJs4YgbbH6PIGXB6W/sc5OLb6RQ= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= @@ -49,10 +49,14 @@ github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= +github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= +github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= +github.com/google/jsonschema-go v0.4.3 h1:/DBOLZTfDow7pe2GmaJNhltueGTtDKICi8V8p+DQPd0= +github.com/google/jsonschema-go v0.4.3/go.mod h1:r5quNTdLOYEz95Ru18zA0ydNbBuYoo9tgaYcxEYhJVE= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/grpc-ecosystem/grpc-gateway/v2 v2.29.0 h1:5VipnvEpbqr2gA2VbM+nYVbkIF28c5ZQfqCBQ5g2xfk= @@ -63,6 +67,8 @@ github.com/lucasb-eyer/go-colorful v1.4.0 h1:UtrWVfLdarDgc44HcS7pYloGHJUjHV/4FwW github.com/lucasb-eyer/go-colorful v1.4.0/go.mod h1:R4dSotOR9KMtayYi1e77YzuveK+i7ruzyGqttikkLy0= github.com/mattn/go-runewidth v0.0.23 h1:7ykA0T0jkPpzSvMS5i9uoNn2Xy3R383f9HDx3RybWcw= github.com/mattn/go-runewidth v0.0.23/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs= +github.com/modelcontextprotocol/go-sdk v1.6.1 h1:0zOSupjKUxPKSocPT1Wtago+mUHU2/uZ4xSOY0FGReU= +github.com/modelcontextprotocol/go-sdk v1.6.1/go.mod h1:kzm3kzFL1/+AziGOE0nUs3gvPoNxMCvkxokMkuFapXQ= github.com/muesli/cancelreader v0.2.2 h1:3I4Kt4BQjOR54NavqnDogx/MIoWBFa0StPA8ELUXHmA= github.com/muesli/cancelreader v0.2.2/go.mod h1:3XuTXfFS2VjM+HTLZY9Ak0l6eUKfijIfMUZ4EgX0QYo= github.com/openai/openai-go/v3 v3.41.0 h1:9GkxcN02U5NG0WGdQjZ0cTSu/pMXEyzL2LfF0ruZCck= @@ -75,6 +81,10 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= +github.com/segmentio/asm v1.1.3 h1:WM03sfUOENvvKexOLp+pCqgb/WDjsi7EK8gIsICtzhc= +github.com/segmentio/asm v1.1.3/go.mod h1:Ld3L4ZXGNcSLRg4JBsZ3//1+f/TjYl0Mzen/DQy1EJg= +github.com/segmentio/encoding v0.5.4 h1:OW1VRern8Nw6ITAtwSZ7Idrl3MXCFwXHPgqESYfvNt0= +github.com/segmentio/encoding v0.5.4/go.mod h1:HS1ZKa3kSN32ZHVZ7ZLPLXWvOVIiZtyJnO1gPH1sKt0= github.com/standard-webhooks/standard-webhooks/libraries v0.0.1 h1:uOfcYT+3QungH6tIGSVCR/Y3KJmgJiHcojJbMTPDZAI= github.com/standard-webhooks/standard-webhooks/libraries v0.0.1/go.mod h1:L1MQhA6x4dn9r007T033lsaZMv9EmBAdXyU/+EF40fo= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= @@ -91,6 +101,8 @@ github.com/tidwall/sjson v1.2.5 h1:kLy8mja+1c9jlljvWTlSazM7cKDRfJuR/bOJhcY5NcY= github.com/tidwall/sjson v1.2.5/go.mod h1:Fvgq9kS/6ociJEDnK0Fk1cpYF4FIW6ZF7LAe+6jwd28= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= +github.com/yosida95/uritemplate/v3 v3.0.2 h1:Ed3Oyj9yrmi9087+NczuL5BwkIc4wvTb5zIM+UJPGz4= +github.com/yosida95/uritemplate/v3 v3.0.2/go.mod h1:ILOh0sOhIJR3+L/8afwt/kE++YT040gmv5BQTMR2HP4= go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= go.opentelemetry.io/otel v1.44.0 h1:JjwHmHpA4iZ3wBxluu2fbbE7j4kqlE8jXyAyPXH7HqU= @@ -125,12 +137,16 @@ golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= +golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= +golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc= golang.org/x/text v0.37.0/go.mod h1:a5sjxXGs9hsn/AJVwuElvCAo9v8QYLzvavO5z2PiM38= +golang.org/x/tools v0.44.0 h1:UP4ajHPIcuMjT1GqzDWRlalUEoY+uzoZKnhOjbIPD2c= +golang.org/x/tools v0.44.0/go.mod h1:KA0AfVErSdxRZIsOVipbv3rQhVXTnlU6UhKxHd1seDI= gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= gonum.org/v1/gonum v0.17.0/go.mod h1:El3tOrEuMpv2UdMrbNlKEh9vd86bmQ6vqIcDwxEOc1E= google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa h1:Kjn0N0tCrDgiAFW+lGO4JZ3ck44CehvJQMAwj9QF0G8= diff --git a/internal/llmloop/loop.go b/internal/llmloop/loop.go index 5239a04..d206a78 100644 --- a/internal/llmloop/loop.go +++ b/internal/llmloop/loop.go @@ -260,8 +260,32 @@ func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath // resolution / re-location. func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.ToolCall, rec *session.TaskRecord) tool.TaskCheckpoint { t := tool.OfName(call.Function.Name) + if !t.IsKnown() { - return tool.Of(tool.NotAvailableMsg) + p, ok := r.deps.Tools.Get(call.Function.Name) + if !ok { + return tool.Of(tool.NotAvailableMsg) + } + r.recordToolCall(call.Function.Name) + var dynArgs map[string]any + if err := json.Unmarshal([]byte(call.Function.Arguments), &dynArgs); err != nil { + return tool.Of(fmt.Sprintf("Error parsing tool arguments for %s: %v", call.Function.Name, err)) + } + telemetry.PrintToolCallStarted(call.Function.Name, dynArgs) + startTime := time.Now() + result, err := p.Execute(ctx, dynArgs) + dur := time.Since(startTime) + if err != nil { + telemetry.RecordToolCall(ctx, call.Function.Name, dur, false) + telemetry.PrintToolCallError(call.Function.Name, err) + return tool.Of(fmt.Sprintf("Error executing tool %s: %v", call.Function.Name, err)) + } + telemetry.RecordToolCall(ctx, call.Function.Name, dur, true) + telemetry.PrintToolCallFinished(call.Function.Name, dur) + if rec != nil { + rec.AddToolResult(call.Function.Name, call.Function.Arguments, result) + } + return tool.Of(result) } if t == tool.TaskDone { diff --git a/internal/mcp/client.go b/internal/mcp/client.go new file mode 100644 index 0000000..b06d7cd --- /dev/null +++ b/internal/mcp/client.go @@ -0,0 +1,103 @@ +package mcp + +import ( + "context" + "fmt" + "os" + "os/exec" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// Client wraps a single MCP server connection via stdio transport. +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 +// 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. +func NewClient(ctx context.Context, name, command string, args, env []string, dir, version string) (*Client, error) { + cmd := exec.Command(command, args...) + cmd.Env = append(os.Environ(), env...) + if dir != "" { + cmd.Dir = dir + } + + client := mcp.NewClient( + &mcp.Implementation{Name: "open-code-review", Version: version}, + nil, + ) + + transport := &mcp.CommandTransport{Command: cmd} + session, err := client.Connect(ctx, transport, nil) + if err != nil { + return nil, fmt.Errorf("connect to MCP server %q: %w", name, 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 MCP server %q: %w", name, err) + } + + success = true + return &Client{ + name: name, + session: session, + tools: toolsResult.Tools, + }, nil +} + +func (c *Client) Name() string { return c.name } +func (c *Client) Tools() []*mcp.Tool { return c.tools } + +// CallTool invokes a tool on the MCP server and returns the text result. +func (c *Client) CallTool(ctx context.Context, name string, args map[string]any) (string, error) { + params := &mcp.CallToolParams{ + Name: name, + Arguments: args, + } + + result, err := c.session.CallTool(ctx, params) + if err != nil { + return "", fmt.Errorf("call MCP tool %q: %w", name, err) + } + + if result.IsError { + return fmt.Sprintf("MCP tool %q returned an error: %s", name, contentToText(result.Content)), nil + } + + return contentToText(result.Content), nil +} + +func (c *Client) Close() error { + return c.session.Close() +} + +func contentToText(contents []mcp.Content) string { + var parts []string + for _, item := range contents { + switch v := item.(type) { + case *mcp.TextContent: + parts = append(parts, v.Text) + default: + parts = append(parts, fmt.Sprintf("[unsupported content type: %T]", item)) + } + } + if len(parts) == 0 { + return "" + } + return strings.Join(parts, "\n") +} diff --git a/internal/mcp/client_test.go b/internal/mcp/client_test.go new file mode 100644 index 0000000..07aee15 --- /dev/null +++ b/internal/mcp/client_test.go @@ -0,0 +1,230 @@ +package mcp + +import ( + "context" + "encoding/json" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +func TestContentToText_SingleText(t *testing.T) { + contents := []mcp.Content{ + &mcp.TextContent{Text: "hello"}, + } + got := contentToText(contents) + if got != "hello" { + t.Errorf("contentToText() = %q, want %q", got, "hello") + } +} + +func TestContentToText_MultipleText(t *testing.T) { + contents := []mcp.Content{ + &mcp.TextContent{Text: "line1"}, + &mcp.TextContent{Text: "line2"}, + } + got := contentToText(contents) + want := "line1\nline2" + if got != want { + t.Errorf("contentToText() = %q, want %q", got, want) + } +} + +func TestContentToText_Empty(t *testing.T) { + got := contentToText(nil) + if got != "" { + t.Errorf("contentToText(nil) = %q, want empty", got) + } + + got = contentToText([]mcp.Content{}) + if got != "" { + t.Errorf("contentToText([]) = %q, want empty", got) + } +} + +func TestContentToText_UnsupportedType(t *testing.T) { + contents := []mcp.Content{ + &mcp.ImageContent{MIMEType: "image/png", Data: []byte("data")}, + } + got := contentToText(contents) + if got == "" { + t.Error("expected non-empty for unsupported type") + } +} + +func TestClient_NameAndTools(t *testing.T) { + tools := []*mcp.Tool{{Name: "t1"}, {Name: "t2"}} + c := &Client{name: "test-srv", tools: tools} + + if c.Name() != "test-srv" { + t.Errorf("Name() = %q, want %q", c.Name(), "test-srv") + } + if len(c.Tools()) != 2 { + t.Errorf("Tools() len = %d, want 2", len(c.Tools())) + } +} + +func TestProvider_Execute_Integration(t *testing.T) { + server := mcp.NewServer( + &mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, + nil, + ) + server.AddTool( + &mcp.Tool{ + Name: "greet", + Description: "Greets", + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "name": map[string]any{"type": "string"}, + }, + }, + }, + func(_ context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + var args struct { + Name string `json:"name"` + } + if err := json.Unmarshal(req.Params.Arguments, &args); err != nil { + return nil, err + } + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "hello " + args.Name}}, + }, nil + }, + ) + + ctx := context.Background() + st, ct := mcp.NewInMemoryTransports() + if _, err := server.Connect(ctx, st, nil); err != nil { + t.Fatalf("server connect: %v", err) + } + sdkClient := mcp.NewClient( + &mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, + nil, + ) + session, err := sdkClient.Connect(ctx, ct, nil) + if err != nil { + t.Fatalf("client connect: %v", err) + } + + toolsResult, err := session.ListTools(ctx, nil) + if err != nil { + t.Fatalf("list tools: %v", err) + } + + c := &Client{name: "test-srv", session: session, tools: toolsResult.Tools} + defer c.Close() + + p := &Provider{toolName: "greet", client: c} + result, err := p.Execute(ctx, map[string]any{"name": "world"}) + if err != nil { + t.Fatalf("Execute: %v", err) + } + if result != "hello world" { + t.Errorf("Execute result = %q, want %q", result, "hello world") + } +} + +func TestNewClient_Integration(t *testing.T) { + server := mcp.NewServer( + &mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, + nil, + ) + server.AddTool( + &mcp.Tool{ + Name: "echo", + Description: "Echoes input", + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "message": map[string]any{"type": "string"}, + }, + }, + }, + func(_ context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + var args struct { + Message string `json:"message"` + } + if err := json.Unmarshal(req.Params.Arguments, &args); err != nil { + return nil, err + } + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "echo: " + args.Message}}, + }, nil + }, + ) + + server.AddTool( + &mcp.Tool{ + Name: "fail", + Description: "Always fails", + InputSchema: map[string]any{"type": "object"}, + }, + func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: "something went wrong"}}, + }, nil + }, + ) + + ctx := context.Background() + st, ct := mcp.NewInMemoryTransports() + if _, err := server.Connect(ctx, st, nil); err != nil { + t.Fatalf("server connect: %v", err) + } + + sdkClient := mcp.NewClient( + &mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, + nil, + ) + session, err := sdkClient.Connect(ctx, ct, nil) + if err != nil { + t.Fatalf("client connect: %v", err) + } + + toolsResult, err := session.ListTools(ctx, nil) + if err != nil { + t.Fatalf("list tools: %v", err) + } + + c := &Client{ + name: "test-srv", + session: session, + tools: toolsResult.Tools, + } + defer c.Close() + + if c.Name() != "test-srv" { + t.Errorf("Name() = %q", c.Name()) + } + if len(c.Tools()) != 2 { + t.Errorf("Tools() len = %d, want 2", len(c.Tools())) + } + + t.Run("CallTool_success", func(t *testing.T) { + result, err := c.CallTool(ctx, "echo", map[string]any{"message": "hi"}) + if err != nil { + t.Fatalf("CallTool: %v", err) + } + if result != "echo: hi" { + t.Errorf("CallTool result = %q, want %q", result, "echo: hi") + } + }) + + t.Run("CallTool_error_result", func(t *testing.T) { + result, err := c.CallTool(ctx, "fail", nil) + if err != nil { + t.Fatalf("CallTool: %v", err) + } + if result == "" { + t.Error("expected non-empty error message") + } + }) + + t.Run("Close", func(t *testing.T) { + if err := c.Close(); err != nil { + t.Errorf("Close: %v", err) + } + }) +} diff --git a/internal/mcp/provider.go b/internal/mcp/provider.go new file mode 100644 index 0000000..8fe0f0d --- /dev/null +++ b/internal/mcp/provider.go @@ -0,0 +1,115 @@ +package mcp + +import ( + "context" + "fmt" + "os" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/open-code-review/open-code-review/internal/llm" + "github.com/open-code-review/open-code-review/internal/tool" +) + +// Provider adapts a single MCP tool to the tool.Provider interface. +type Provider struct { + toolName string + client *Client +} + +func (p *Provider) Tool() tool.Tool { + return tool.Dynamic(p.toolName) +} + +func (p *Provider) Execute(ctx context.Context, args map[string]any) (string, error) { + return p.client.CallTool(ctx, p.toolName, args) +} + +// RegisterAll registers tools from the MCP client into the tool registry. +// When allowedTools is non-empty, only tools whose names appear in the list are registered. +// Tools whose names conflict with built-in or already-registered tools are skipped with a warning. +func RegisterAll(reg *tool.Registry, c *Client, allowedTools []string) { + allowed := make(map[string]struct{}, len(allowedTools)) + for _, name := range allowedTools { + allowed[name] = struct{}{} + } + filtering := len(allowed) > 0 + + matched := make(map[string]struct{}) + for _, t := range c.Tools() { + if filtering { + if _, ok := allowed[t.Name]; !ok { + continue + } + matched[t.Name] = struct{}{} + } + if tool.IsReserved(t.Name) { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: MCP server %q tool %q conflicts with built-in tool, skipping\n", c.Name(), t.Name) + continue + } + if _, exists := reg.Get(t.Name); exists { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: MCP server %q tool %q conflicts with already-registered tool, skipping\n", c.Name(), t.Name) + continue + } + reg.Register(&Provider{ + toolName: t.Name, + client: c, + }) + } + + for name := range allowed { + if _, ok := matched[name]; !ok { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: MCP server %q allowed tool %q not found in server's tool list\n", c.Name(), name) + } + } +} + +// ToToolDef converts an MCP tool definition to an llm.ToolDef. +func ToToolDef(t *mcp.Tool) llm.ToolDef { + params := map[string]any{"type": "object"} + + switch schema := t.InputSchema.(type) { + case map[string]any: + for k, v := range schema { + params[k] = v + } + if _, ok := params["type"]; !ok { + params["type"] = "object" + } + case nil: + // No schema — keep the default {"type": "object"}. + default: + fmt.Fprintf(os.Stderr, "[ocr] WARNING: MCP tool %q has unexpected InputSchema type %T, using empty object schema\n", t.Name, t.InputSchema) + } + + return llm.ToolDef{ + Type: "function", + Function: llm.FunctionDef{ + Name: t.Name, + Description: t.Description, + Parameters: params, + }, + } +} + +// CollectToolDefs gathers tool definitions from MCP clients, filtering out +// tools that conflict with built-in tools or were not successfully registered. +func CollectToolDefs(clients []*Client, reg *tool.Registry) []llm.ToolDef { + var defs []llm.ToolDef + seen := make(map[string]struct{}) + for _, c := range clients { + for _, t := range c.Tools() { + if tool.IsReserved(t.Name) { + continue + } + if _, exists := reg.Get(t.Name); !exists { + continue + } + if _, dup := seen[t.Name]; dup { + continue + } + seen[t.Name] = struct{}{} + defs = append(defs, ToToolDef(t)) + } + } + return defs +} diff --git a/internal/mcp/provider_test.go b/internal/mcp/provider_test.go new file mode 100644 index 0000000..1a5cc48 --- /dev/null +++ b/internal/mcp/provider_test.go @@ -0,0 +1,242 @@ +package mcp + +import ( + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/open-code-review/open-code-review/internal/llm" + "github.com/open-code-review/open-code-review/internal/tool" +) + +func newTestClient(name string, tools []*mcp.Tool) *Client { + return &Client{name: name, tools: tools} +} + +func TestProvider_Tool(t *testing.T) { + c := newTestClient("srv", nil) + p := &Provider{toolName: "my_tool", client: c} + got := p.Tool() + if got.Name() != "my_tool" { + t.Errorf("Tool().Name() = %q, want %q", got.Name(), "my_tool") + } +} + +func TestRegisterAll_Basic(t *testing.T) { + tools := []*mcp.Tool{ + {Name: "alpha"}, + {Name: "beta"}, + } + c := newTestClient("srv", tools) + reg := tool.NewRegistry() + + RegisterAll(reg, c, nil) + + for _, name := range []string{"alpha", "beta"} { + if _, ok := reg.Get(name); !ok { + t.Errorf("expected tool %q to be registered", name) + } + } +} + +func TestRegisterAll_AllowedFilter(t *testing.T) { + tools := []*mcp.Tool{ + {Name: "alpha"}, + {Name: "beta"}, + {Name: "gamma"}, + } + c := newTestClient("srv", tools) + reg := tool.NewRegistry() + + RegisterAll(reg, c, []string{"alpha", "gamma"}) + + if _, ok := reg.Get("alpha"); !ok { + t.Error("expected alpha to be registered") + } + if _, ok := reg.Get("beta"); ok { + t.Error("expected beta to be filtered out") + } + if _, ok := reg.Get("gamma"); !ok { + t.Error("expected gamma to be registered") + } +} + +func TestRegisterAll_SkipsReservedTools(t *testing.T) { + tools := []*mcp.Tool{ + {Name: "file_read"}, + {Name: "custom_tool"}, + } + c := newTestClient("srv", tools) + reg := tool.NewRegistry() + + RegisterAll(reg, c, nil) + + if _, ok := reg.Get("file_read"); ok { + t.Error("reserved tool file_read should not be registered") + } + if _, ok := reg.Get("custom_tool"); !ok { + t.Error("expected custom_tool to be registered") + } +} + +func TestRegisterAll_SkipsDuplicateTools(t *testing.T) { + tools := []*mcp.Tool{{Name: "dup_tool"}} + c := newTestClient("srv", tools) + reg := tool.NewRegistry() + + stub := tool.NewStub(tool.Dynamic("dup_tool")) + reg.Register(stub) + + RegisterAll(reg, c, nil) + + got, ok := reg.Get("dup_tool") + if !ok { + t.Fatal("expected dup_tool to be registered") + } + if _, isProvider := got.(*Provider); isProvider { + t.Error("expected dup_tool to remain the original stub, not MCP provider") + } +} + +func TestRegisterAll_WarnsUnmatchedAllowed(t *testing.T) { + tools := []*mcp.Tool{{Name: "alpha"}} + c := newTestClient("srv", tools) + reg := tool.NewRegistry() + + RegisterAll(reg, c, []string{"alpha", "nonexistent"}) + + if _, ok := reg.Get("alpha"); !ok { + t.Error("expected alpha to be registered") + } +} + +func TestToToolDef_MapSchema(t *testing.T) { + mt := &mcp.Tool{ + Name: "search", + Description: "Search things", + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "query": map[string]any{"type": "string"}, + }, + }, + } + + got := ToToolDef(mt) + + want := llm.ToolDef{ + Type: "function", + Function: llm.FunctionDef{ + Name: "search", + Description: "Search things", + }, + } + if got.Type != want.Type || got.Function.Name != want.Function.Name || got.Function.Description != want.Function.Description { + t.Errorf("ToToolDef() basic fields mismatch: got %+v", got) + } + if got.Function.Parameters["type"] != "object" { + t.Errorf("expected type=object, got %v", got.Function.Parameters["type"]) + } + if got.Function.Parameters["properties"] == nil { + t.Error("expected properties to be set") + } +} + +func TestToToolDef_NilSchema(t *testing.T) { + mt := &mcp.Tool{ + Name: "noop", + InputSchema: nil, + } + + got := ToToolDef(mt) + + if got.Function.Parameters["type"] != "object" { + t.Errorf("expected default type=object, got %v", got.Function.Parameters["type"]) + } +} + +func TestToToolDef_UnexpectedSchemaType(t *testing.T) { + mt := &mcp.Tool{ + Name: "weird", + InputSchema: "not-a-map", + } + + got := ToToolDef(mt) + + if got.Function.Parameters["type"] != "object" { + t.Errorf("expected fallback type=object, got %v", got.Function.Parameters["type"]) + } +} + +func TestCollectToolDefs_Basic(t *testing.T) { + tools := []*mcp.Tool{ + {Name: "alpha", Description: "A"}, + {Name: "beta", Description: "B"}, + } + c := newTestClient("srv", tools) + reg := tool.NewRegistry() + RegisterAll(reg, c, nil) + + defs := CollectToolDefs([]*Client{c}, reg) + + if len(defs) != 2 { + t.Fatalf("expected 2 defs, got %d", len(defs)) + } + names := map[string]bool{} + for _, d := range defs { + names[d.Function.Name] = true + } + if !names["alpha"] || !names["beta"] { + t.Errorf("expected alpha and beta in defs, got %v", names) + } +} + +func TestCollectToolDefs_FiltersReserved(t *testing.T) { + tools := []*mcp.Tool{ + {Name: "file_read"}, + {Name: "custom"}, + } + c := newTestClient("srv", tools) + reg := tool.NewRegistry() + RegisterAll(reg, c, nil) + + defs := CollectToolDefs([]*Client{c}, reg) + + for _, d := range defs { + if d.Function.Name == "file_read" { + t.Error("reserved tool file_read should not appear in defs") + } + } +} + +func TestCollectToolDefs_FiltersUnregistered(t *testing.T) { + tools := []*mcp.Tool{ + {Name: "registered_tool"}, + {Name: "unregistered_tool"}, + } + c := newTestClient("srv", tools) + reg := tool.NewRegistry() + reg.Register(&Provider{toolName: "registered_tool", client: c}) + + defs := CollectToolDefs([]*Client{c}, reg) + + if len(defs) != 1 { + t.Fatalf("expected 1 def, got %d", len(defs)) + } + if defs[0].Function.Name != "registered_tool" { + t.Errorf("expected registered_tool, got %s", defs[0].Function.Name) + } +} + +func TestCollectToolDefs_Dedup(t *testing.T) { + tools := []*mcp.Tool{{Name: "shared", Description: "A"}} + c1 := newTestClient("srv1", tools) + c2 := newTestClient("srv2", tools) + reg := tool.NewRegistry() + reg.Register(&Provider{toolName: "shared", client: c1}) + + defs := CollectToolDefs([]*Client{c1, c2}, reg) + + if len(defs) != 1 { + t.Errorf("expected 1 deduped def, got %d", len(defs)) + } +} diff --git a/internal/mcp/stdio_test.go b/internal/mcp/stdio_test.go new file mode 100644 index 0000000..ea9f3d0 --- /dev/null +++ b/internal/mcp/stdio_test.go @@ -0,0 +1,118 @@ +package mcp + +import ( + "context" + "encoding/json" + "log" + "os" + "runtime" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +const runAsServerEnv = "_OCR_MCP_TEST_SERVER" + +func TestMain(m *testing.M) { + if os.Getenv(runAsServerEnv) != "" { + runTestMCPServer() + return + } + os.Exit(m.Run()) +} + +func runTestMCPServer() { + server := mcp.NewServer( + &mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, + nil, + ) + server.AddTool( + &mcp.Tool{ + Name: "echo", + Description: "Echoes input", + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "message": map[string]any{"type": "string"}, + }, + }, + }, + func(_ context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + var args struct { + Message string `json:"message"` + } + if err := json.Unmarshal(req.Params.Arguments, &args); err != nil { + return nil, err + } + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "echo: " + args.Message}}, + }, nil + }, + ) + + if err := server.Run(context.Background(), &mcp.StdioTransport{}); err != nil { + log.Fatal(err) + } +} + +func TestNewClient_Stdio(t *testing.T) { + requireExec(t) + + exe, err := os.Executable() + if err != nil { + t.Fatal(err) + } + + ctx := context.Background() + c, err := NewClient(ctx, "test-srv", exe, nil, []string{runAsServerEnv + "=1"}, "", "v0.0.1") + if err != nil { + t.Fatalf("NewClient: %v", err) + } + defer c.Close() + + if c.Name() != "test-srv" { + t.Errorf("Name() = %q, want %q", c.Name(), "test-srv") + } + + found := false + for _, tool := range c.Tools() { + if tool.Name == "echo" { + found = true + break + } + } + if !found { + t.Error("expected echo tool in Tools()") + } + + result, err := c.CallTool(ctx, "echo", map[string]any{"message": "hello"}) + if err != nil { + t.Fatalf("CallTool: %v", err) + } + if result != "echo: hello" { + t.Errorf("CallTool result = %q, want %q", result, "echo: hello") + } + + if err := c.Close(); err != nil { + t.Errorf("Close: %v", err) + } +} + +func TestNewClient_Stdio_BadCommand(t *testing.T) { + requireExec(t) + + ctx := context.Background() + _, err := NewClient(ctx, "bad", "/nonexistent/mcp-server-binary", nil, nil, "", "v0.0.1") + if err == nil { + t.Fatal("expected error for bad command, got nil") + } +} + +func requireExec(t *testing.T) { + t.Helper() + switch runtime.GOOS { + case "darwin", "linux", "windows": + default: + t.Skip("unsupported OS for subprocess test") + } +} diff --git a/internal/tool/definitions.go b/internal/tool/definitions.go index cf24f99..6cbab77 100644 --- a/internal/tool/definitions.go +++ b/internal/tool/definitions.go @@ -3,6 +3,7 @@ package tool import ( "context" "errors" + "fmt" ) // Tool represents a single review tool. @@ -33,6 +34,27 @@ func allTools() []Tool { return []Tool{Unknown, TaskDone, CodeComment, FileRead, FileFind, FileReadDiff, CodeSearch} } +// IsReserved reports whether name matches any built-in tool name (including Unknown). +func IsReserved(name string) bool { + for _, t := range allTools() { + if t.name == name { + return true + } + } + return false +} + +// Dynamic creates a Tool with the given name for dynamically discovered tools (e.g. MCP). +func Dynamic(name string) Tool { + if name == "" { + panic("tool: Dynamic called with empty name") + } + if IsReserved(name) { + panic(fmt.Sprintf("tool: Dynamic called with reserved tool name %q", name)) + } + return Tool{name: name} +} + // Name returns the tool's identifier name. func (t Tool) Name() string { return t.name } diff --git a/internal/tool/definitions_test.go b/internal/tool/definitions_test.go index a1f1af3..ae3cf96 100644 --- a/internal/tool/definitions_test.go +++ b/internal/tool/definitions_test.go @@ -86,6 +86,47 @@ func TestRegistry_GetAfterFreeze(t *testing.T) { } } +func TestIsReserved(t *testing.T) { + reserved := []string{"unknown", "task_done", "code_comment", "file_read", "file_find", "file_read_diff", "code_search"} + for _, name := range reserved { + if !IsReserved(name) { + t.Errorf("IsReserved(%q) = false, want true", name) + } + } + + nonReserved := []string{"custom_tool", "my_tool", "search", ""} + for _, name := range nonReserved { + if IsReserved(name) { + t.Errorf("IsReserved(%q) = true, want false", name) + } + } +} + +func TestDynamic(t *testing.T) { + tool := Dynamic("my_custom_tool") + if tool.Name() != "my_custom_tool" { + t.Errorf("Dynamic().Name() = %q, want %q", tool.Name(), "my_custom_tool") + } +} + +func TestDynamic_PanicsOnEmpty(t *testing.T) { + defer func() { + if r := recover(); r == nil { + t.Error("expected panic for empty name") + } + }() + Dynamic("") +} + +func TestDynamic_PanicsOnReserved(t *testing.T) { + defer func() { + if r := recover(); r == nil { + t.Error("expected panic for reserved name") + } + }() + Dynamic("file_read") +} + type dummyProvider struct{} func (d *dummyProvider) Tool() Tool { return CodeSearch }