Repository navigation
fix(desktop): toggle a connector through its per-server enabled endpoint - #135694
Closed
Yicheng-Mark wants to merge 1 commit into
Closed
Yicheng-Mark wants to merge 1 commit into
Yicheng-Mark wants to merge 1 commit into
Conversation
The Connectors toggle wrote the whole mcp_servers map via PUT
/api/mcp/servers. That replace 400s when any single entry in the map
fails validation, so one bad entry made every enable/disable a silent
no-op: persist() ignored the {ok:false} outcome and painted the cache
anyway, the staleTime:0 refetch flipped the switch back, and nothing
ever surfaced an error (135014).
Route setServerEnabled through PUT /api/mcp/servers/{name}/enabled --
the endpoint the CLI-parity surface already exposes -- notify on
rejection, and paint the config cache only after the backend accepted
the write.
15 of 19 tasks
Author
|
Superseded by #137863 — same fix and the same new test file, re-posted as a ready PR rather than a draft parked behind the open-PR limit. |
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.
Symptom
The Desktop Connectors per-server enable/disable switch does nothing: the state never changes, nothing persists to
mcp_servers.<name>.enabledin config, and no error is ever shown. Meanwhile the CLI equivalent (hermes config set mcp_servers.<name>.enabled false) works against the same server, so the failure reads as "the toggle is dead". As reported in #135014 (first symptom).Cause
The toggle went through the whole-map replace:
setServerEnabled→writeEntry→persist()→saveMcpServers→PUT /api/mcp/serverswith the entiremcp_serversmap.Two layers turn a backend rejection into silence:
replace_mcp_serversvalidates the whole map and 400s if any entry fails — one bad entry among eleven poisons every toggle, while the single-key CLI write path never touches that validation.persist()awaitedsaveMcpServersbut never read its{ ok }outcome, then painted the React Query cache unconditionally. The optimistic paint flips the switch, thestaleTime: 0background refetch returns the untouched config and flips it back, and neitherwriteEntry'sfalsereturn norsetServerEnablednotifies — the reporter sees "the state never changes (and does not persist to config)", with no error anywhere.The per-server endpoint for exactly this toggle already exists (
PUT /api/mcp/servers/{name}/enabled, exposed bysetMcpServerEnabled), but only in ambient-scope form — the Capabilities surface needs thecapabilityScoped(profile)routing every other desktop MCP write uses.Fix
setServerEnabledstops round-tripping the whole map:setMcpServerEnabledFor(name, enabled, profile?)inapi/mcp.ts, scoped likesaveMcpServersbut hitting the per-server endpoint.setServerEnabledcalls it, and:ok !== trueor a thrown transport error) callsnotifyErrorand returns — no cache paint, so the switch reflects reality;withEnabled, mirrors into the draft, reloads, and probes when enabling — same post-write stepspersist()performed, minus the map round-trip.writeEntry's guards: unknown server name andprofilePendingstay silent no-ops, and theprofileEpochcheck still fences cross-profile races.The second symptom in the issue (clicking a row opening what looks like a different server's details) is deliberately not addressed here: it traces into how merged hosted+local cards pick their dialog and default way, which is a design question rather than a wiring bug, and belongs in its own discussion on the issue.
Regression tests (
use-mcp-servers.test.tsx, new): the toggle PUTs the per-server endpoint and never the whole map; a rejected toggle notifies and leaves the cache unpainted; re-enabling drops theenabledkey (the documented on-by-default) and probes; unknown server names short-circuit.Fixes #135014 (toggle symptom)
Type of Change
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — TS files; local verification was transpile-level (tsc, clean of syntax errors); suite runs in CIDocumentation & Housekeeping
cli-config.yaml.example— N/ACONTRIBUTING.mdorAGENTS.md— N/A