Skip to content
This repository was archived by the owner on Feb 25, 2026. It is now read-only.

fix(mcp): persist MCP toggle state to config #608

Closed
Olusammytee wants to merge 1 commit into from
Closed

fix(mcp): persist MCP toggle state to config #608

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions packages/opencode/src/mcp/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,25 @@ export namespace MCP {
return commands
}

async function persistEnabled(name: string, enabled: boolean, fallback?: Config.Mcp) {
const cfg = await Config.get()
const config = cfg.mcp ?? {}
const current = config[name]
const entry = isMcpConfigured(current) ? current : fallback
if (!entry) return

await Config.updateGlobal({
Copy link
Contributor

@kilo-code-bot kilo-code-bot bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mcp: {
[name]: {
...entry,
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
enabled,
},
},
}).catch((error) => {
log.warn("failed to persist mcp enabled state", { name, enabled, error })
})
}
Comment on lines +264 to +274
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

export async function add(name: string, mcp: Config.Mcp) {
const s = await state()
const result = await create(name, mcp)
Expand Down Expand Up @@ -534,6 +553,7 @@ export namespace MCP {
status: "failed",
error: "Unknown error during connection",
}
await persistEnabled(name, true, mcp)
Copy link
Contributor

@kilo-code-bot kilo-code-bot bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return
}

Expand All @@ -549,9 +569,14 @@ export namespace MCP {
}
s.clients[name] = result.mcpClient
}

await persistEnabled(name, true, mcp)
}

export async function disconnect(name: string) {
const cfg = await Config.get()
const config = cfg.mcp ?? {}
const current = config[name]
const s = await state()
const client = s.clients[name]
if (client) {
Expand All @@ -561,6 +586,7 @@ export namespace MCP {
delete s.clients[name]
}
s.status[name] = { status: "disabled" }
await persistEnabled(name, false, isMcpConfigured(current) ? current : undefined)
}

export async function tools() {
Expand Down
Loading