Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Works as described. In /mcps, space on a failed server now goes to Disabled (on the base it stayed Failed and only offered retry), and the core/server tests show the flag written to and removed from the defining config file with comments kept. I checked it by driving the TUI on the base and on this branch, and by running the core, server and tui tests plus typecheck for core, server, tui and protocol.
Public API: no new routes or fields, but the meaning of POST /api/experimental/mcp/{server}/connect and /disconnect changes. They now also write disabled: true / enabled: false into (or remove it from) the config file that defines the server. The desktop/web app (packages/app, gui-extensions) call these endpoints too, so their toggles will now save to config as well. The request and response shapes are unchanged. Config.Interface gains an optional setMcpDisabled.
Concerns:
packages/servertypecheck and the new server test fail:test/mcp-toggle.test.tsimportsjsonc-parser, which isn't a dependency ofpackages/server.JSON.parseis enough there.- The toggle can write to admin-managed config, and connect can silently do nothing (see inline comments).
- Writing to user config files at all is a maintainer call, given the earlier pushback on #17833.
- add Config.setMcpDisabled for native and v1 MCP entries - add test for jsonc comments and enable/disable roundtrip
- change connect to clear the disabled flag in config - change disconnect to set the disabled flag in config - change route descriptions - add test for a failed server
- change space to disable failed, pending and connected servers - change space to enable disabled servers - add tests for every status
- change disconnect to interrupt a running connect attempt - change interrupted connect to skip the failed status - add test for a server that never answers
- change setMcpDisabled to stay runtime-only when managed config defines the server - add test for a managed server
- change connect to start the server even after a config edit - change toggle test to JSON.parse
d40ca8f to
2132536
Compare
VlexWest
changed the title
|
@VlexWest Do you mind taking this PR over the finish line? 🙏 |
- fix conflicts with the MCP connection refactor - change disconnect to interrupt pending starts through startConnection - add pooled variant of the pending disconnect test
|
@eriktisme Done, merged dev & fixed the conflicts, should be good to go now. Now let's hope a maintainer sees & merges it 🙂 (@rekram1-node @kitlangton) |
|
@VlexWest I think this would also fix the V2 location-eviction path in #62868. On 2.2.6 and dev, MCP.disconnect only sets status: "disabled" in memory. After a location services evicted sweep, the Location rebuilds from config and the server reconnects with a new connectionID, so the /mcps choice is silently undone. Writing the flag into the defining config file is what's missing for config-defined servers; mine use the V1 mcp. shape, so enabled: false is the relevant path. It also survives /reload, unlike the runtime toggle. This covers the toggle half of #62868. Would you mind mentioning it in the description as Related: #62868 (rather than Fixes), since the stateful-process loss is separate? It would also be good to add a regression test for the eviction path: disable via the mcp.disconnect endpoint, call LocationServiceMap.invalidate, boot the Location again, and assert the server is still disabled and the config still has the flag. I'm happy to help with that, or open a follow-up test-only PR after this lands if you'd rather keep this one more focused. |
- fix conflict in the MCP dialog test fixture
- add test that a disabled V1 server stays disabled after a location reload - change toggle tests to share one setup
|
@kfunigeneral Ah yes, good catch, thanks! I've added "Related: #62868" to the description & a regression test: a disabled v1 server (mcp.) stays disabled after the Location is being rebuild + the config keeps enabled: false. |
Thanks, that's the exact test I was after. The test shape matches my config. |
Issue for this PR
Closes #56451
Related: #62868 (fixes the toggle half; losing a stateful process on eviction is separate)
Type of change
What does this PR do?
Space in
/mcpsnow disables a server in any state (failed, pending, connected). On a disabled server it enables it again. Before, failed servers could only be retried & pending ones ignored space.The toggle is saved now. Disconnect writes
"disabled": true(or"enabled": falsefor v1 entries) into the config file that defines the server, connect removes it. Edits go through jsonc-parser, so comments & formatting stay. Servers without a config file (runtimeadd, remote config) or defined by managed config keep the old runtime-only behavior.Config.setMcpDisabledfinds the highest-priority json/jsonc file with the server & edits itenable/disable)I know #17833 got pushback for writing to
opencode.json. This only writes when the user toggles a server & only touches that one key, same as Claude Code does.How did you verify your code works?
disabled(times out on the old code)disabledin config & status/api/location/reload, which rebuilds the Location like an idle eviction (MCP servers disabled with /mcps reconnect after an idle Location eviction #62868)bun typecheckin core, server, tui, protocolbun run dev:liveon a failing serverScreenshots / recordings
Checklist