fix(mcp): persist MCP toggle state to config

[Olusammytee]

Summary

• persist enabled state to config whenever MCP servers are connected or disconnected
• keep runtime status updates unchanged while adding durable state across restarts
• include fallback handling so persistence still works when toggled entries were only in-memory during the action

Fixes #565

[github-actions]

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

1. Open an issue describing the bug/feature (if one doesn't exist)
2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

[kiloconnect]

WARNING: Config.updateGlobal triggers void Instance.disposeAll() as a side effect (see config.ts:1556). This means every call to persistEnabled will asynchronously tear down all instances — including the MCP state — closing every connected MCP client.

In connect(), this causes the just-connected client (and all other MCP clients) to be disposed in the background. The state will lazily re-initialize on next access (re-reading config and reconnecting), but this creates:

1. An unnecessary disconnect/reconnect cycle for all MCP servers, not just the toggled one
2. A window where MCP tools are unavailable
3. Potential race conditions if other operations access MCP state during disposal

Consider writing the config file directly without going through updateGlobal, or adding a mechanism to persist config without triggering full instance disposal.

[kiloconnect]

WARNING: Persisting enabled: true even when create() returned null (connection failed) will cause Config.updateGlobal → Instance.disposeAll() to fire, tearing down all other healthy MCP connections. On a failed connect, the side-effect cost of disposal is especially undesirable since there's no new client to show for it.

[kiloconnect]

Code Review Summary

Status: 2 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	0
WARNING	2
SUGGESTION	0

Issue Details (click to expand)

WARNING

File	Line	Issue
packages/opencode/src/mcp/index.ts	264	Config.updateGlobal triggers void Instance.disposeAll() as a side effect, causing all MCP clients to be torn down and reconnected every time toggle state is persisted
packages/opencode/src/mcp/index.ts	556	Persisting enabled state on failed connection triggers unnecessary disposal of all healthy MCP connections

Details

The core concern is that persistEnabled calls Config.updateGlobal, which internally fires void Instance.disposeAll() (fire-and-forget). This means:

1. In connect(): After successfully connecting an MCP server and storing the client in state, the persistEnabled call triggers background disposal of ALL instances — closing the just-connected client and every other MCP client. State will lazily re-initialize on next access (re-reading the now-updated config), but this creates an unnecessary disconnect/reconnect cycle for all MCP servers and a window where tools are unavailable.

2. In disconnect(): After manually closing one client, persistEnabled triggers disposal of all remaining clients via the same mechanism.

Consider either:

• Writing the config file directly (bypassing updateGlobal's disposal side effect)
• Adding a config update path that doesn't trigger Instance.disposeAll()
• Or documenting that this is acceptable because the re-initialization will use the correct persisted state

Files Reviewed (1 files)

• packages/opencode/src/mcp/index.ts - 2 issues

Fix these issues in Kilo Cloud

[Copilot]

Pull request overview

This PR aims to make MCP enable/disable toggles durable by persisting the enabled state into configuration when MCP servers are connected or disconnected, addressing loss of toggle state after restart (Fixes #565).

Changes:

• Add persistEnabled() helper to update config with the MCP server’s enabled state.
• Persist enabled: true on connect() and enabled: false on disconnect().
• Add fallback handling to persist even when the current config entry isn’t directly usable.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

persistEnabled writes the entire resolved MCP config back into the user config (via ...entry). Because config loading expands {env:VAR} / {file:...} placeholders before parsing, this risks persisting secrets (e.g., auth headers) and/or large inlined file contents into the config file, and can also overwrite JSONC formatting/comments. Safer approach: only persist the enabled field (the schema already supports an mcp[name] = { enabled: boolean } override) so the existing entry stays untouched.

Suggested change

...entry,

[Copilot]

Calling Config.updateGlobal() on every connect/disconnect has a large side effect: it triggers Instance.disposeAll() (via Config.updateGlobal), which will dispose state for all instances and close existing MCP clients. This can cause unrelated runtime state to reset and other MCP connections to drop when toggling a single server. Consider persisting the flag without disposing instances (or provide a lighter-weight config write path) since runtime status is already updated in-memory.