mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-10-07 14:58:39 +08:00
921ea743ef
* Fix MCP server delete causing duplicates and not removing entries
The agent-server's PATCH /api/settings applies agent_settings_diff via
deep-merge (see openhands.agent_server.persistence.models._deep_merge in
the SDK). For scalar fields that's fine, but mcp_config.mcpServers is a
name-keyed map and deep-merge cannot remove keys: a diff that omits a
server leaves the stale key behind, and a diff whose generated names
shift after a deletion produces duplicate entries pointing to the wrong
config.
The frontend's toSdkMcpConfig regenerates server names from a shared
counter on each save, so after deleting any server the resulting key
set shifts and the merge both fails to delete the target and creates
duplicates of the surviving servers.
Compensate inside SettingsService.saveSettings by sending a
{mcp_config: null} PATCH ahead of the real write whenever the diff
sets mcp_config to a non-null value. null is not a dict, so the
deep-merge takes the replace branch; the follow-up PATCH then writes
the new value into a freshly-cleared field. Skip the pre-clear when
the caller is already wiping mcp_config (null) — a single PATCH
already replaces in that case.
Co-authored-by: openhands <openhands@all-hands.dev>
* Stop bumping MCP suffix numbers on unrelated edits
toSdkMcpConfig used a single counter shared across the sse/shttp/stdio
loops, so a stdio server named 'myname' became 'myname_1' when any
sse/shttp entry was persisted ahead of it and got renamed to 'myname_2'
the moment another sse server was added. Every edit that changed the
count of any other server type would shift the suffix on everything
that came after it in the iteration order, which is exactly the
'numbers change every time I edit' behaviour the user hit.
Suffix names only when the same base actually collides, tracked
per-base against the running output dict. Bare 'sse'/'shttp' stay bare
unless there's a real duplicate within their own type, and stdio names
stay verbatim regardless of how many other server types exist.
Co-authored-by: openhands <openhands@all-hands.dev>
* Make Tavily install work, allow multiple instances of the same MCP entry
Two related marketplace bugs:
1. Clicking an already-installed catalog tile (e.g. Slack) opened
the install modal in *edit* mode, so saving overwrote the
existing server instead of adding a second one. There was no way
to install two Slack workspaces, two Postgres connections, etc.
2. Tavily used a fake 'tavily-builtin' template kind that called
saveSettings({ search_api_key }). That field is not part of
agent_settings_diff / conversation_settings_diff, isn't in
APP_PREFERENCE_FIELDS, and isn't forwarded by saveCloudSettings,
so it was silently dropped on both backends. The SDK has no
first-class Tavily integration either — the 'wires up the
Tavily MCP server automatically' comment was aspirational.
Make the marketplace install modal strictly add-only: editing an
existing server already goes through CustomServerEditor from the
installed-server-card's edit button, so the modal's edit path was
redundant and conflicting. Combined with the per-base name
collision suffixing in toSdkMcpConfig, the user can now install a
second Slack and it lands as 'slack_1' alongside the original
'slack' without clobbering it.
Convert the Tavily catalog entry to a regular stdio MCP server
(npx -y tavily-mcp + TAVILY_API_KEY env) so it goes through the
same mcp_config write path as every other catalog entry and works
identically on local and cloud backends.
Strip the now-dead tavily-builtin scaffolding (the MarketplaceTemplate
union variant, the ExistingInstall discriminated union and
isMcpInstall helper, findInstalledMatch's tavily branch, the
InstalledServerCard catalogIdOverride prop, the InstalledServersSection
virtual Tavily card, MCPPage's Tavily-specific Configure/Remove
plumbing, and the marketplace-card transport-label case).
Co-authored-by: openhands <openhands@all-hands.dev>
* Restore mcp_config on rollback when the second PATCH fails
Address review feedback (PR #388) on the two-step PATCH atomicity gap.
The pre-clear (`mcp_config: null`) is destructive at the backend.
If the follow-up write fails after the clear succeeds, the user's
MCP config was previously left silently empty — bad data-loss UX
for what should be an idempotent retry.
Before pre-clearing, snapshot the previous mcp_config in raw SDK
shape (read directly from `fetchCloudSettings` for cloud or
`fetchSettingsFromApi` for local — `getSettings` returns the GUI's
parsed MCPConfig with empty-array defaults, which is not safe to
round-trip back). On second-write failure, attempt a best-effort
rollback PATCH that restores the snapshot, then re-throw the
original error so react-query mutations surface the failure to the
user.
The rollback is intentionally single-shot (no withRetry) — we want
the original error to surface promptly. Rollback errors are
swallowed so they don't shadow the user-actionable failure.
Three new tests cover:
- Successful rollback on cloud-backend second-write failure
- Successful rollback on local-backend second-write failure
- No bogus rollback PATCH when there was nothing to snapshot
(first-time install where the snapshot has no mcp_config)
Co-authored-by: openhands <openhands@all-hands.dev>
* chore: Remove PR-only artifacts
---------
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: allhands-bot <allhands-bot@users.noreply.github.com>