From 6adcb1ecd4289c33923a6770b7f3ced87b2ffe26 Mon Sep 17 00:00:00 2001 From: kite Date: Wed, 1 Jul 2026 19:10:16 +0800 Subject: [PATCH] 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 --- README.ja-JP.md | 42 +++++ README.ko-KR.md | 42 +++++ README.md | 42 +++++ README.ru-RU.md | 42 +++++ README.zh-CN.md | 42 +++++ cmd/opencodereview/config_cmd.go | 133 ++++++++++++-- cmd/opencodereview/config_cmd_test.go | 230 +++++++++++++++++++++++ cmd/opencodereview/flags.go | 14 +- cmd/opencodereview/procattr_unix.go | 18 ++ cmd/opencodereview/procattr_windows.go | 12 ++ cmd/opencodereview/review_cmd.go | 67 +++++++ cmd/opencodereview/shell_unix.go | 12 ++ cmd/opencodereview/shell_windows.go | 12 ++ go.mod | 10 +- go.sum | 20 +- internal/llmloop/loop.go | 26 ++- internal/mcp/client.go | 103 +++++++++++ internal/mcp/client_test.go | 230 +++++++++++++++++++++++ internal/mcp/provider.go | 115 ++++++++++++ internal/mcp/provider_test.go | 242 +++++++++++++++++++++++++ internal/mcp/stdio_test.go | 118 ++++++++++++ internal/tool/definitions.go | 22 +++ internal/tool/definitions_test.go | 41 +++++ 23 files changed, 1616 insertions(+), 19 deletions(-) create mode 100644 cmd/opencodereview/procattr_unix.go create mode 100644 cmd/opencodereview/procattr_windows.go create mode 100644 cmd/opencodereview/shell_unix.go create mode 100644 cmd/opencodereview/shell_windows.go create mode 100644 internal/mcp/client.go create mode 100644 internal/mcp/client_test.go create mode 100644 internal/mcp/provider.go create mode 100644 internal/mcp/provider_test.go create mode 100644 internal/mcp/stdio_test.go 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 }