fix(mcp): persist disabled MCP state across config reloads#28434
Open
danielxxomg wants to merge 2 commits into
Open
fix(mcp): persist disabled MCP state across config reloads#28434danielxxomg wants to merge 2 commits into
danielxxomg wants to merge 2 commits into
Conversation
When a user changes a provider in opencode.json, ALL MCPs restart and re-activate — including ones explicitly disabled via /mcps toggle. The disabled state was only in-memory (s.status[name]) and lost on any Instance reload. This persists disabled MCP IDs to Global.Path.state/mcp-state.json using the same AppFileSystem pattern as model.json. MCP.state() now reads this file during init and respects previously toggled disabled servers. Changes: - src/mcp/index.ts: readDisabledState/writeDisabledState helpers, wiring in state()/connect()/disconnect(), uses Effect.catchCause for defect safety - test/fixture/fixture.ts: add AppFileSystem.defaultLayer to test layer chain - test/mcp/lifecycle.test.ts: 7 persistence tests covering all spec scenarios Tests: 28/28 pass. Spec compliance: 8/8 scenarios. Closes anomalyco#28428
github-actions
Bot
added
the
needs:compliance
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds persistence for MCP “disabled” server state by reading/writing mcp-state.json, and introduces tests to validate lifecycle behavior across restarts.
Changes:
- Implement read/write of persisted disabled server IDs in the MCP service layer.
- Update test fixture runtime to provide
AppFileSystemservices. - Add lifecycle tests covering persistence (missing/corrupt state file, precedence rules, connect/disconnect behavior).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/opencode/src/mcp/index.ts | Reads/writes mcp-state.json and applies persisted disabled IDs during MCP initialization; updates connect/disconnect to maintain persistence. |
| packages/opencode/test/fixture/fixture.ts | Provides AppFileSystem.defaultLayer so tests can use filesystem-backed persistence. |
| packages/opencode/test/mcp/lifecycle.test.ts | Adds test cases validating persistence semantics for disabled MCP servers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+684
to
687
| const s = yield* InstanceState.get(state) | ||
| s.disabledFromPersistence.delete(name) | ||
| yield* writeDisabledState(s.disabledFromPersistence) | ||
| yield* createAndStore(name, { ...mcp, enabled: true }) |
Comment on lines
+278
to
+291
| const readDisabledState = Effect.fnUntraced(function* () { | ||
| return yield* fs.readJson(path.join(Global.Path.state, "mcp-state.json")).pipe( | ||
| Effect.map((data) => { | ||
| if (data === null || typeof data !== "object") return new Set<string>() | ||
| const obj = data as Record<string, unknown> | ||
| if (!Array.isArray(obj.disabledMcpServerIds)) return new Set<string>() | ||
| return new Set(obj.disabledMcpServerIds.filter((x): x is string => typeof x === "string")) | ||
| }), | ||
| Effect.catchCause((cause) => { | ||
| log.warn("failed to read mcp-state.json", { cause: Cause.squash(cause) }) | ||
| return Effect.succeed(new Set<string>()) | ||
| }), | ||
| ) | ||
| }) |
Comment on lines
+293
to
+301
| const writeDisabledState = Effect.fnUntraced(function* (ids: Set<string>) { | ||
| yield* fs | ||
| .writeJson(path.join(Global.Path.state, "mcp-state.json"), { disabledMcpServerIds: [...ids].sort() }, 0o600) | ||
| .pipe( | ||
| Effect.catch((error) => { | ||
| log.error("failed to write mcp-state.json", { error }) | ||
| return Effect.void | ||
| }), | ||
| ) |
Comment on lines
549
to
+553
| const s: State = { | ||
| status: {}, | ||
| clients: {}, | ||
| defs: {}, | ||
| disabledFromPersistence: disabled, |
Comment on lines
+570
to
+573
| if (s.disabledFromPersistence.has(key)) { | ||
| s.status[key] = { status: "disabled" } | ||
| return | ||
| } |
Comment on lines
+891
to
+909
| it.instance( | ||
| "disconnect persists disabled ID to mcp-state.json", | ||
| () => | ||
| MCP.Service.use((mcp: MCPNS.Interface) => | ||
| Effect.gen(function* () { | ||
| lastCreatedClientName = "persist-disc-server" | ||
| getOrCreateClientState("persist-disc-server") | ||
|
|
||
| yield* mcp.add("persist-disc-server", { | ||
| type: "local", | ||
| command: ["echo", "test"], | ||
| }) | ||
| yield* mcp.disconnect("persist-disc-server") | ||
|
|
||
| const fs = yield* AppFileSystem.Service | ||
| const data = yield* fs | ||
| .readJson(path.join(Global.Path.state, "mcp-state.json")) | ||
| .pipe(Effect.catch(() => Effect.succeed(null))) | ||
| expect(data).not.toBeNull() |
github-actions
Bot
removed
the
needs:compliance
Contributor
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
NotFound is expected on first run — silence it. Still warn on actual errors like corrupt JSON or permission denied.
danielxxomg
force-pushed
the
fix/mcp-disabled-persistence
branch
from
7b29ce7 to
08b1301
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue for this PR
Closes #28428
Type of change
What does this PR do?
The
/mcpstoggle only stores disabled state in memory. Any config change that triggers an Instance reload (e.g., editing a provider inopencode.json) wipes that state and re-enables all MCPs.This persists disabled MCP IDs to
Global.Path.state/mcp-state.json— same directory and pattern asmodel.json.MCP.state()reads this file during init, so disabled servers stay off across reloads and restarts.Files changed:
src/mcp/index.ts:readDisabledState/writeDisabledStatehelpers, wired intostate()/connect()/disconnect(). UsesEffect.catchCauseso corrupt JSON doesn't crash.test/fixture/fixture.ts: addedAppFileSystem.defaultLayerso tests can exercise file I/O.test/mcp/lifecycle.test.ts: 7 tests covering missing file, corrupt file, stale entries, configenabled: falseprecedence, and the full toggle→persist→reload cycle.How did you verify your code works?
bun test test/mcp/lifecycle.test.ts— 28 pass, 0 fail. The 7 new persistence tests verify:mcp-state.jsonenabled: falsein config always takes precedenceScreenshots / recordings
N/A, no UI changes.
Checklist