From d25a5ae816e6dc22db204cfa2e25044726230042 Mon Sep 17 00:00:00 2001 From: ScarletCarpet Date: Fri, 3 Jul 2026 17:29:03 +0800 Subject: [PATCH] fix(mcp): make setup timeout configurable The MCP server setup command (e.g. codegraph init && codegraph index) previously had a hardcoded 5-minute timeout, which was too short for large repositories and caused the process to be killed. - Add `setup_timeout` field to mcp_servers config (default 30m), controlling the setup command's maximum execution time - Update config CLI, tests, and README docs accordingly --- README.ja-JP.md | 5 ++- README.ko-KR.md | 5 ++- README.md | 8 +++- README.ru-RU.md | 5 ++- README.zh-CN.md | 8 +++- cmd/opencodereview/config_cmd.go | 21 ++++++---- cmd/opencodereview/config_cmd_test.go | 27 +++++++++++++ cmd/opencodereview/flags.go | 4 +- cmd/opencodereview/review_cmd.go | 9 ++++- cmd/opencodereview/review_cmd_test.go | 57 +++++++++++++++++++++++++++ 10 files changed, 133 insertions(+), 16 deletions(-) diff --git a/README.ja-JP.md b/README.ja-JP.md index b37648c6..45c79bf9 100644 --- a/README.ja-JP.md +++ b/README.ja-JP.md @@ -700,6 +700,7 @@ 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 '' +ocr config set mcp_servers..setup_timeout 30 # MCPサーバーを削除 ocr config unset mcp_servers. @@ -712,8 +713,9 @@ ocr config unset mcp_servers. | `env` | いいえ | 環境変数（`KEY=VALUE`形式） | | `tools` | いいえ | 許可するツール名。空の場合、サーバーのすべてのツールが利用可能 | | `setup` | いいえ | サーバー起動前に実行するシェルコマンド（例：インデックスの構築） | +| `setup_timeout` | いいえ | setup コマンドのタイムアウト（分）、例：`30`。デフォルト：`30` | -> **注意：** MCPツールの名前が組み込みツールと競合する場合、そのツールは警告付きでスキップされます。`setup`コマンドのタイムアウトは5分です。 +> **注意：** MCPツールの名前が組み込みツールと競合する場合、そのツールは警告付きでスキップされます。 **例：[CodeGraph](https://github.com/nicholasgasior/codegraph)を追加してコード構造分析を強化** @@ -722,6 +724,7 @@ 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' +ocr config set mcp_servers.codegraph.setup_timeout 30 ``` ### 環境変数 diff --git a/README.ko-KR.md b/README.ko-KR.md index 54e507f1..c8eaff11 100644 --- a/README.ko-KR.md +++ b/README.ko-KR.md @@ -658,6 +658,7 @@ 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 '' +ocr config set mcp_servers..setup_timeout 30 # MCP 서버 삭제 ocr config unset mcp_servers. @@ -670,8 +671,9 @@ ocr config unset mcp_servers. | `env` | 아니오 | 환경 변수 (`KEY=VALUE` 형식) | | `tools` | 아니오 | 허용할 도구 이름. 비어 있으면 서버의 모든 도구 사용 가능 | | `setup` | 아니오 | 서버 시작 전에 실행할 셸 명령어 (예: 인덱스 빌드) | +| `setup_timeout` | 아니오 | setup 명령어의 타임아웃(분), 예: `30`. 기본값: `30` | -> **참고:** MCP 도구의 이름이 내장 도구와 충돌하면 경고와 함께 건너뜁니다. `setup` 명령어의 타임아웃은 5분입니다. +> **참고:** MCP 도구의 이름이 내장 도구와 충돌하면 경고와 함께 건너뜁니다. **예시: [CodeGraph](https://github.com/nicholasgasior/codegraph)를 추가하여 코드 구조 분석 강화** @@ -680,6 +682,7 @@ 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' +ocr config set mcp_servers.codegraph.setup_timeout 30 ``` ### Environment Variables diff --git a/README.md b/README.md index 1e82856e..aa9b30a1 100644 --- a/README.md +++ b/README.md @@ -683,7 +683,8 @@ Config file: `~/.opencodereview/config.json` | `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 | +| `mcp_servers..setup` | string | Setup command to run before starting the server (e.g. build an index) | +| `mcp_servers..setup_timeout` | integer | Setup command timeout in minutes (e.g. `30`; default: `30`) | | `language` | string | Any language name, e.g. `English`, `Chinese` (default: `English`) | | `telemetry.enabled` | boolean | `true` \| `false` | | `telemetry.exporter` | string | `console` \| `otlp` | @@ -705,6 +706,7 @@ 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 '' +ocr config set mcp_servers..setup_timeout 30 # Delete an MCP server ocr config unset mcp_servers. @@ -717,8 +719,9 @@ ocr config unset mcp_servers. | `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) | +| `setup_timeout` | No | Setup command timeout in minutes (e.g. `30`). Default: `30` | -> **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. +> **Note:** If an MCP tool's name conflicts with a built-in tool, it will be skipped with a warning. **Example: Add [CodeGraph](https://github.com/nicholasgasior/codegraph) for code structure analysis** @@ -727,6 +730,7 @@ 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' +ocr config set mcp_servers.codegraph.setup_timeout 30 ``` ### Environment Variables diff --git a/README.ru-RU.md b/README.ru-RU.md index d1fbad8f..a438e3e4 100644 --- a/README.ru-RU.md +++ b/README.ru-RU.md @@ -702,6 +702,7 @@ 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 '' +ocr config set mcp_servers..setup_timeout 30 # Удалить MCP-сервер ocr config unset mcp_servers. @@ -714,8 +715,9 @@ ocr config unset mcp_servers. | `env` | Нет | Переменные окружения в формате `KEY=VALUE` | | `tools` | Нет | Разрешённые имена инструментов; если пусто — доступны все инструменты сервера | | `setup` | Нет | Shell-команда для выполнения перед запуском сервера (например, построение индекса) | +| `setup_timeout` | Нет | Таймаут setup-команды в минутах, например `30`. По умолчанию: `30` | -> **Примечание:** Если имя MCP-инструмента конфликтует со встроенным инструментом, он будет пропущен с предупреждением. Таймаут команды `setup` составляет 5 минут. +> **Примечание:** Если имя MCP-инструмента конфликтует со встроенным инструментом, он будет пропущен с предупреждением. **Пример: добавление [CodeGraph](https://github.com/nicholasgasior/codegraph) для усиления анализа структуры кода** @@ -724,6 +726,7 @@ 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' +ocr config set mcp_servers.codegraph.setup_timeout 30 ``` ### Переменные окружения diff --git a/README.zh-CN.md b/README.zh-CN.md index d84bd7f9..af6e762c 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -668,7 +668,8 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则 | `mcp_servers..args` | array | MCP 服务器的命令行参数 | | `mcp_servers..env` | array | 环境变量，`KEY=VALUE` 格式 | | `mcp_servers..tools` | array | 允许使用的工具名称（为空则允许所有工具） | -| `mcp_servers..setup` | string | 启动服务器前运行的初始化命令 | +| `mcp_servers..setup` | string | 启动服务器前运行的初始化命令（例如构建索引） | +| `mcp_servers..setup_timeout` | integer | setup 命令的超时时间（分钟），例如 `30`；默认：`30` | | `language` | string | 任意语言名称，例如 `English`、`Chinese`（默认：`English`） | | `telemetry.enabled` | boolean | `true` \| `false` | | `telemetry.exporter` | string | `console` \| `otlp` | @@ -690,6 +691,7 @@ 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 '' +ocr config set mcp_servers..setup_timeout 30 # 删除 MCP 服务器 ocr config unset mcp_servers. @@ -702,8 +704,9 @@ ocr config unset mcp_servers. | `env` | 否 | 环境变量，`KEY=VALUE` 格式 | | `tools` | 否 | 允许使用的工具名称；为空则服务器的所有工具均可用 | | `setup` | 否 | 启动服务器前运行的 shell 命令（例如构建索引） | +| `setup_timeout` | 否 | setup 命令的超时时间（分钟），例如 `30`。默认：`30` | -> **注意：** 如果 MCP 工具的名称与内置工具冲突，该工具将被跳过并输出警告。`setup` 命令的超时时间为 5 分钟。 +> **注意：** 如果 MCP 工具的名称与内置工具冲突，该工具将被跳过并输出警告。 **示例：添加 [CodeGraph](https://github.com/nicholasgasior/codegraph) 增强代码结构分析能力** @@ -712,6 +715,7 @@ 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' +ocr config set mcp_servers.codegraph.setup_timeout 30 ``` ### 环境变量 diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 64b511b2..4703a58d 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -200,11 +200,12 @@ type ProviderEntry struct { // 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"` + Command string `json:"command"` + Args []string `json:"args,omitempty"` + Env []string `json:"env,omitempty"` + Tools []string `json:"tools,omitempty"` + Setup string `json:"setup,omitempty"` + SetupTimeout int `json:"setup_timeout,omitempty"` } // Config represents the user-level configuration file (~/.opencodereview/config.json). @@ -374,7 +375,7 @@ func setConfigValue(cfg *Config, key, value string) error { } cfg.Llm.ExtraBody = m default: - return fmt.Errorf("unknown config key: %s\nSupported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_header, llm.model, llm.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 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, setup_timeout", key) } return nil } @@ -592,8 +593,14 @@ func setMCPServerValue(cfg *Config, key, value string) error { entry.Tools = filtered case "setup": entry.Setup = value + case "setup_timeout": + n, err := strconv.Atoi(value) + if err != nil || n <= 0 { + return fmt.Errorf("invalid value for %s: must be a positive integer (minutes)", key) + } + entry.SetupTimeout = n default: - return fmt.Errorf("unknown MCP server field %q: supported fields are command, args, env, tools, setup", field) + return fmt.Errorf("unknown MCP server field %q: supported fields are command, args, env, tools, setup, setup_timeout", field) } cfg.MCPServers[name] = entry diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index 8c7de98d..daab2305 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -540,6 +540,33 @@ func TestSetMCPServerValue_Setup(t *testing.T) { } } +func TestSetMCPServerValue_SetupTimeout(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.my-server.setup_timeout", "30"); err != nil { + t.Fatalf("setMCPServerValue: %v", err) + } + if cfg.MCPServers["my-server"].SetupTimeout != 30 { + t.Errorf("SetupTimeout = %d, want %d", cfg.MCPServers["my-server"].SetupTimeout, 30) + } +} + +func TestSetMCPServerValue_SetupTimeoutInvalid(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.my-server.setup_timeout", "not-a-number"); err == nil { + t.Fatal("expected error for invalid timeout") + } +} + +func TestSetMCPServerValue_SetupTimeoutNonPositive(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.my-server.setup_timeout", "0"); err == nil { + t.Fatal("expected error for non-positive timeout") + } + if err := setMCPServerValue(cfg, "mcp_servers.my-server.setup_timeout", "-5"); err == nil { + t.Fatal("expected error for negative timeout") + } +} + func TestSetMCPServerValue_UnknownField(t *testing.T) { cfg := &Config{} if err := setMCPServerValue(cfg, "mcp_servers.my-server.unknown", "val"); err == nil { diff --git a/cmd/opencodereview/flags.go b/cmd/opencodereview/flags.go index ccda5158..16e33857 100644 --- a/cmd/opencodereview/flags.go +++ b/cmd/opencodereview/flags.go @@ -306,6 +306,8 @@ Examples: 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"]' + ocr config set mcp_servers.codegraph.setup "codegraph init && codegraph index" + ocr config set mcp_servers.codegraph.setup_timeout 30 # Delete an MCP server ocr config unset mcp_servers.codegraph @@ -321,5 +323,5 @@ Examples: 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`) +MCP server fields: command, args, env, tools, setup, setup_timeout`) } diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index d76951ba..fd7869a0 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -213,9 +213,16 @@ func initMCPClients(ctx context.Context, cfg *Config, tools *tool.Registry, repo fmt.Fprintf(os.Stderr, "[ocr] WARNING: MCP server %q has no command configured, skipping\n", name) continue } + + // Run the setup phase (time-bounded) before starting the + // long-lived server process. setup_timeout defaults to 30 minutes. 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) + setupTimeout := 30 * time.Minute + if serverCfg.SetupTimeout > 0 { + setupTimeout = time.Duration(serverCfg.SetupTimeout) * time.Minute + } + setupCtx, setupCancel := context.WithTimeout(ctx, setupTimeout) setupCmd := shellCommand(setupCtx, serverCfg.Setup) setupCmd.Dir = repoDir configureProcessGroup(setupCmd) diff --git a/cmd/opencodereview/review_cmd_test.go b/cmd/opencodereview/review_cmd_test.go index eaad1bfc..526227cf 100644 --- a/cmd/opencodereview/review_cmd_test.go +++ b/cmd/opencodereview/review_cmd_test.go @@ -1,8 +1,11 @@ package main import ( + "context" "strings" "testing" + + "github.com/open-code-review/open-code-review/internal/tool" ) func TestValidateReviewRefsRejectsOptionLikeCommit(t *testing.T) { @@ -54,3 +57,57 @@ func TestParseReviewFlagsAllowsFromAndTo(t *testing.T) { t.Fatalf("unexpected opts: from=%q to=%q", opts.from, opts.to) } } + +func TestInitMCPClients_NilConfig(t *testing.T) { + clients := initMCPClients(context.Background(), nil, tool.NewRegistry(), "/tmp", "test") + if clients != nil { + t.Fatalf("expected nil for nil config, got %d clients", len(clients)) + } +} + +func TestInitMCPClients_EmptyServers(t *testing.T) { + clients := initMCPClients(context.Background(), &Config{}, tool.NewRegistry(), "/tmp", "test") + if clients != nil { + t.Fatalf("expected nil for empty servers, got %d clients", len(clients)) + } +} + +func TestInitMCPClients_EmptyCommandSkipped(t *testing.T) { + cfg := &Config{ + MCPServers: map[string]MCPServerConfig{ + "no-cmd": {Setup: "echo hello"}, + }, + } + clients := initMCPClients(context.Background(), cfg, tool.NewRegistry(), "/tmp", "test") + if clients != nil { + t.Fatalf("expected nil when the only server has no command") + } +} + +func TestInitMCPClients_SetupOnly(t *testing.T) { + cfg := &Config{ + MCPServers: map[string]MCPServerConfig{ + "no-cmd": {Command: "echo", Setup: "echo setup-run"}, + }, + } + // Setup runs successfully, then the server is created (echo exits after initCtx timeout). + clients := initMCPClients(context.Background(), cfg, tool.NewRegistry(), "/tmp", "test") + if len(clients) != 0 { + t.Fatalf("expected 0 clients (echo is not an MCP server), got %d", len(clients)) + } +} + +func TestInitMCPClients_SetupTimeoutAborts(t *testing.T) { + cfg := &Config{ + MCPServers: map[string]MCPServerConfig{ + "sleepy": { + Command: "echo", + Setup: "sleep 5", + SetupTimeout: 1, // 1 minute timeout + }, + }, + } + // A 1-minute timeout should be plenty for "sleep 5" to succeed. + clients := initMCPClients(context.Background(), cfg, tool.NewRegistry(), "/tmp", "test") + _ = clients +}