feat(mcp): interactive install host picker, idle-exit default - #114
feat(mcp): interactive install host picker, idle-exit default#114Ariestar wants to merge 2 commits into
Conversation
sivtr mcp install without -p or -y now prompts the user to pick agent hosts, mirroring npx skills; detected hosts come pre-checked. Non-interactive paths (stdin not a TTY, -y, or explicit -p) keep the old auto-detect behavior.
The idle clock now starts at spawn (standard idle semantics) instead of after the first tool call, so servers that are never used still exit and no longer linger per agent session and lock the binary on Windows. [mcp] idle_exit_secs now defaults to 60 (idle exit on; 0 disables), and --idle-exit 0 is honored as disable instead of exiting immediately.
Deploying sivtr with
|
| Latest commit: |
6f1a7a9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://effad92c.sivtr.pages.dev |
| Branch Preview URL: | https://feat-mcp-interactive-install.sivtr.pages.dev |
404b5a3 to
6f1a7a9
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f1a7a9200
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let idle_secs = args.idle_exit.or_else(|| { | ||
| // share (hosts run plain `sivtr mcp serve`). Both accept 0 to | ||
| // mean "never exit on idle"; the config default is 60. | ||
| let idle_secs = args.idle_exit.filter(|&secs| secs > 0).or_else(|| { |
There was a problem hiding this comment.
When the config has the new default of 60 seconds, or any other nonzero value, invoking sivtr mcp serve --idle-exit 0 filters Some(0) into None and then falls back to that config value. The server therefore still exits on idle even though the CLI documents zero as disabling the watchdog and taking precedence over config; handle an explicit zero without consulting the config fallback.
Useful? React with 👍 / 👎.
| /// interactive, so `doctor --fix` / `setup` (which pass `yes: true`) never | ||
| /// prompt and CI pipes keep the old auto-detect behavior. | ||
| fn pick_targets() -> Result<Vec<AgentProvider>> { | ||
| let detected = detect_targets(); |
There was a problem hiding this comment.
When no supported host is installed, detect_targets() injects Claude as a fallback rather than returning the detected set. The new interactive picker therefore presents Claude as pre-checked on machines without Claude, and accepting the defaults writes a ~/.claude.json configuration for an absent host; use detected_hosts() for interactive defaults and reserve the fallback for non-interactive installation.
Useful? React with 👍 / 👎.
| /// Provider host(s) to inject (comma-separated or repeated). | ||
| /// Use registered command names, or `all`. | ||
| /// Default: detect installed hosts. | ||
| /// Omit to pick hosts interactively; with -y, installs to detected hosts. |
There was a problem hiding this comment.
McpInstallArgs is also used by McpAction::Uninstall, so sivtr mcp uninstall --help now claims that omitting -p opens a picker and that -y skips it and installs detected hosts. In reality uninstall() always resolves detected targets without prompting and never reads yes, which can cause a user expecting to choose targets to remove registrations immediately; use neutral shared help or separate install and uninstall argument types.
Useful? React with 👍 / 👎.
| let targets = if args.providers.is_empty() && !args.yes { | ||
| pick_targets()? |
There was a problem hiding this comment.
With sivtr mcp install -l local and no explicit provider, the picker still lists and preselects every detected host, including GlobalOnly entries such as Codex. Accepting one of those choices reaches ensure_location_allowed() only after selection and fails the command, potentially after earlier selected local-capable hosts were already modified; pass the requested location into the picker and exclude unsupported choices or validate the complete selection before writing.
Useful? React with 👍 / 👎.
| if !BUSY.load(Ordering::Relaxed) | ||
| && idle_elapsed() >= idle.as_millis() as u64 | ||
| { | ||
| std::process::exit(0); |
There was a problem hiding this comment.
When the host issues overlapping tool calls, BUSY is only a boolean: if the shorter call finishes first, its guard stores false while the longer call remains in flight. After another idle interval the changed default watchdog can therefore satisfy this condition and terminate the process in the middle of the longer request, despite the comment's no-in-flight-call guarantee; track an in-flight count or otherwise synchronize the watchdog with all active calls.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughChangesMCP behavior updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant InstallCLI
participant McpCommand
participant pick_targets
participant InteractiveHelper
InstallCLI->>McpCommand: install without providers and without --yes
McpCommand->>pick_targets: select installation targets
pick_targets->>InteractiveHelper: present registered and detected hosts
InteractiveHelper-->>pick_targets: return selected host indices
pick_targets-->>McpCommand: return selected providers
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/system/mcp.rs (1)
193-200: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve explicit
--idle-exit 0.Line 195 filters out zero before applying CLI precedence. With the default
[mcp] idle_exit_secs = 60,sivtr mcp serve --idle-exit 0falls back to 60 seconds instead of disabling idle exit. HandleSome(0)asNonebefore loading configuration.Proposed fix
- let idle_secs = args.idle_exit.filter(|&secs| secs > 0).or_else(|| { - SivtrConfig::load() - .ok() - .map(|config| config.mcp.idle_exit_secs) - .filter(|&secs| secs > 0) - }); + let idle_secs = match args.idle_exit { + Some(0) => None, + Some(secs) => Some(secs), + None => SivtrConfig::load() + .ok() + .map(|config| config.mcp.idle_exit_secs) + .filter(|&secs| secs > 0), + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/system/mcp.rs` around lines 193 - 200, Update the idle_secs calculation in the mcp serve command to convert an explicit args.idle_exit value of 0 into None before loading configuration, while preserving other explicit CLI values and using configured idle_exit_secs only when no CLI value is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/mcp.rs`:
- Line 42: Update the CLI reference tables in the English and Chinese reference
documentation to reflect the current provider-selection behavior: omitting
--provider in a TTY opens the interactive host picker, while non-TTY input and
--yes use detected hosts. Keep the existing CLI help wording aligned with this
distinction.
---
Outside diff comments:
In `@src/commands/system/mcp.rs`:
- Around line 193-200: Update the idle_secs calculation in the mcp serve command
to convert an explicit args.idle_exit value of 0 into None before loading
configuration, while preserving other explicit CLI values and using configured
idle_exit_secs only when no CLI value is provided.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 691b4e8a-89bd-4125-87be-ae5232a88bd3
📒 Files selected for processing (6)
crates/sivtr-core/src/config/mod.rsdocs-site/src/content/docs/reference/cli.mddocs-site/src/content/docs/zh-cn/reference/cli.mdsrc/cli/mcp.rssrc/commands/system/mcp.rssrc/mcp/server.rs
| /// Provider host(s) to inject (comma-separated or repeated). | ||
| /// Use registered command names, or `all`. | ||
| /// Default: detect installed hosts. | ||
| /// Omit to pick hosts interactively; with -y, installs to detected hosts. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document terminal-dependent provider selection.
The reference tables in docs-site/src/content/docs/reference/cli.md and docs-site/src/content/docs/zh-cn/reference/cli.md still state that omitting --provider detects installed hosts. In a TTY, omission now opens the host picker. Document that non-TTY input and --yes use detected hosts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/mcp.rs` at line 42, Update the CLI reference tables in the English
and Chinese reference documentation to reflect the current provider-selection
behavior: omitting --provider in a TTY opens the interactive host picker, while
non-TTY input and --yes use detected hosts. Keep the existing CLI help wording
aligned with this distinction.
2 participants
Purpose
Make
sivtr mcp installinteractive (npx-skills style) so users can pick which agent hosts to configure, and default the stdio MCP server to exit after 60s of idle so each agent session does not keep a server alive until exit.Changes
mcp installwith no-p/-ynow shows an interactive multi-select of all MCP-capable hosts, pre-checked to detected ones. Falls back to detected hosts when stdin is not a TTY, sodoctor --fix/setup/ CI pipes keep the old non-interactive behavior.--yesgains its intended meaning: skip the picker and install to detected hosts.[mcp] idle_exit_secsconfig overrides,--idle-exit 0disables. Hosts respawn the server on the next tool use.Validation
cargo fmt --all -- --checkpassescargo clippy --workspace --all-targets -- -D warningspassescargo test --workspace mcppasses (13 tests)Summary by CodeRabbit
New Features
0disables the timeout.Bug Fixes
Documentation