fix(mcp): make setup timeout configurable

[ScarletCarpet]

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 timeout field to mcp_servers config (default 30m), controlling the setup command's maximum execution time
• Update config CLI, tests, and README docs accordingly

Description

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

• make test passes locally
• Manual testing (describe below)

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA

Related Issues

[github-actions]

🔍 OpenCodeReview found 1 issue(s) in this PR.

• ✅ 1 posted as inline comment(s)
• 📝 0 posted as summary

[github-actions]

JSON serialization issue with time.Duration: Go's time.Duration is an int64 (nanoseconds) and does not implement json.Marshaler/json.Unmarshaler. This means:

1. When saved to the JSON config file via saveConfig, the timeout will be stored as a raw nanosecond integer (e.g., 1800000000000 for 30 minutes), which is not human-readable.
2. If a user manually edits the JSON config and writes "timeout": "30m", json.Unmarshal in loadOrCreateConfig / LoadAppConfig will fail because it expects a number, not a string.

Consider using a custom type that implements json.Marshaler/json.Unmarshaler to serialize/deserialize as a human-readable duration string (e.g., "30m"), or use a string field and parse it at point-of-use.

[lizhengfeng101]

1. Field type & naming — timeout (time.Duration)

Two issues with the current Timeout time.Duration:

• Serialization is opaque and fragile. time.Duration has no JSON string form, so 30m is written to config.json as nanoseconds: "timeout": 1800000000000. Worse, if a user hand-edits the file to the documented "30m", the entire config fails to load (cannot unmarshal string into ... time.Duration).
• timeout is too broad a name. At the MCP-server level it reads like a connection/request timeout, but it only bounds the setup command.

Suggest storing a plain positive integer in minutes and renaming the field to tie it to setup — setup_timeout:

SetupTimeout int `json:"setup_timeout,omitempty"` // minutes

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

setupTimeout := 30 * time.Minute
if serverCfg.SetupTimeout > 0 {
    setupTimeout = time.Duration(serverCfg.SetupTimeout) * time.Minute
}

This stores "setup_timeout": 30 — readable and safe to hand-edit — and the name makes the association with setup explicit. Usage becomes ocr config set mcp_servers.<name>.setup_timeout 30. Since the value is a bare number in minutes, please state the unit clearly in the README/help text. (The time import in config_cmd.go is then no longer needed.)

2. Localized READMEs not synced

Per the repo convention, README changes must be mirrored to all localized copies. This PR updates README.md and README.zh-CN.md, but README.ja-JP.md, README.ko-KR.md, and README.ru-RU.md all contain the same MCP setup section — and still carry the stale "5-minute timeout" note. Please update those three as well.

3. New tests don't assert anything

TestInitMCPClients_SetupOnly and TestInitMCPClients_TimeoutConfig end with _ = clients and pass regardless of behavior — they don't actually verify the timeout feature. They also spawn a real echo and try to exec /nonexistent/cmd, so they carry environment side effects for no coverage gain. Consider asserting real behavior, e.g. a very short timeout against a sleep setup command should abort setup. The positive/negative cases in config_cmd_test.go are a good template.

Nits / positives

• Nice consistency: the field is threaded through the unknown-key error (config_cmd.go), the flags.go help text, and the setMCPServerValue error. With the rename, update those same three strings and the config_cmd_test.go cases too.
• Raising the default from 5m → 30m and documenting it is a sensible change on its own.

[ScarletCarpet]

all fixed:

1. timeout rename to setup_timeout
2. update all docs
3. use intergal to save timeout

[lizhengfeng101]

Test feedback (cmd/opencodereview/review_cmd_test.go)

1. TestInitMCPClients_SetupTimeoutAborts is misnamed

The name implies it verifies the timeout-abort path, but the logic (and its own comment) does the opposite: "A 1-minute timeout should be plenty for sleep 5 to succeed." It never exercises an abort, and the body ends with _ = clients — no assertion at all.

• The name misleads readers into thinking timeout-abort is covered when it isn't.
• It runs a real sleep 5, making a unit test take ≥5s for zero verification.

Suggestion: either make it genuinely trigger a timeout (very short timeout + longer setup, then assert the server is skipped), or rename to e.g. TestInitMCPClients_SetupWithinTimeoutSucceeds and shorten the sleep with a real assertion.

2. Tests spawn real processes — stability/speed concern (minor)

TestInitMCPClients_SetupOnly and TestInitMCPClients_SetupTimeoutAborts drive the full initMCPClients, forcing real echo/sleep spawns through mcp.NewClient's 30s init context. This couples the tests to PATH, shell-builtin availability, and platform behavior (the repo ships shell_windows.go, so Windows is a target — sleep/echo differ there), making the suite slow and non-deterministic.

Recommended approach: don't test the timeout logic end-to-end through initMCPClients. Extract the piece this PR actually changed into a pure function and test that directly — no processes, no platform dependency:

func effectiveSetupTimeout(cfg MCPServerConfig) time.Duration {
	if cfg.SetupTimeout > 0 {
		return time.Duration(cfg.SetupTimeout) * time.Minute
	}
	return 30 * time.Minute
}

The new logic is then covered by a fast, deterministic table test (0 → 30m default, negative → 30m fallback, 30 → 30m, 1 → 1m), and the misnamed SetupTimeoutAborts test can be deleted.

If you additionally want to cover the real execution paths (setup fails → server skipped; setup exceeds timeout → aborted), extract a runner that takes a time.Duration rather than int-minutes, so an abort can be tested in milliseconds instead of a full minute:

func runMCPSetup(ctx context.Context, script, repoDir string, timeout time.Duration) ([]byte, error) {
	setupCtx, cancel := context.WithTimeout(ctx, timeout)
	defer cancel()
	cmd := shellCommand(setupCtx, script)
	cmd.Dir = repoDir
	configureProcessGroup(cmd)
	return cmd.CombinedOutput()
}

• success: exit 0 with a generous timeout
• failure: exit 1, assert error returned
• abort: sleep 5 with 10 * time.Millisecond, assert the context/error is non-nil — millisecond-fast and actually validates the abort

For any remaining tests that must launch real processes, guard them with if testing.Short() { t.Skip(...) } and route commands through shellCommand (or skip on Windows), so make test stays fast and CI stays stable across platforms.