From 24ac0e561491d54c448abd43f35f197dfa62189d Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Mon, 1 Jun 2026 19:55:29 +0200 Subject: [PATCH] feat(acp): let ACP agents configure MCP servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ACP agents now forward `mcp_config` to their subprocess at session creation (see software-agent-sdk), so the MCP surfaces no longer need to be gated off for ACP: - agent-server-adapter: include `mcp_config` in the ACP conversation-start payload (a shared field, handled explicitly like `llm`/`agent_context`), dropping empty/serverless configs. - /mcp route: remove the `redirectIfAcpActive` clientLoader so ACP users can reach the MCP editor; the existing editor + `agent_settings.mcp_config` storage already drive both agent kinds. - extensions-navigation: drop the now-obsolete `disabledByAcp` mechanism (MCP was its only user); the MCP nav item is clickable under ACP. - acp-route-guard: update docs — `/mcp` is intentionally no longer guarded. Closes #993. Co-Authored-By: Claude Opus 4.8 (1M context) --- __tests__/api/agent-server-adapter.test.ts | 68 +++++++++++++-- .../skills/extensions-navigation.test.tsx | 85 +++---------------- __tests__/routes/mcp.test.tsx | 73 ++-------------- src/api/agent-server-adapter.ts | 9 ++ .../features/skills/extensions-navigation.tsx | 54 ------------ src/routes/mcp.tsx | 16 ++-- src/utils/acp-route-guard.ts | 13 +-- 7 files changed, 102 insertions(+), 216 deletions(-) diff --git a/__tests__/api/agent-server-adapter.test.ts b/__tests__/api/agent-server-adapter.test.ts index 15ac2fc08..7c9ff2d30 100644 --- a/__tests__/api/agent-server-adapter.test.ts +++ b/__tests__/api/agent-server-adapter.test.ts @@ -479,7 +479,9 @@ describe("buildStartConversationRequest", () => { // @spec LLD-001 — Frontend always sends its chosen default model describe("llm.model fallback — frontend always sends its chosen default", () => { type ModelPayload = { - agent_settings: Record & { llm: Record }; + agent_settings: Record & { + llm: Record; + }; }; function getModelFrom( @@ -509,10 +511,19 @@ describe("buildStartConversationRequest", () => { // SettingsValue includes scalars so inline literals are type-safe here. it.each([ ["undefined", { ...DEFAULT_SETTINGS.agent_settings, llm: {} }], - ["an empty string", { ...DEFAULT_SETTINGS.agent_settings, llm: { model: "" } }], - ["whitespace only", { ...DEFAULT_SETTINGS.agent_settings, llm: { model: " " } }], + [ + "an empty string", + { ...DEFAULT_SETTINGS.agent_settings, llm: { model: "" } }, + ], + [ + "whitespace only", + { ...DEFAULT_SETTINGS.agent_settings, llm: { model: " " } }, + ], // No llm key at all — SettingsValue accepts plain scalars. - ["absent (no llm block)", { schema_version: 1, agent_kind: "openhands", agent: "CodeActAgent" }], + [ + "absent (no llm block)", + { schema_version: 1, agent_kind: "openhands", agent: "CodeActAgent" }, + ], // Mirrors a fresh user who skipped onboarding: server returns {}. ["entirely empty", {}], ])( @@ -944,12 +955,11 @@ describe("buildStartConversationRequest — ACP discriminator", () => { acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"], acp_model: "claude-opus-4-5", // These fields are LLM-only and must NOT leak into ACP settings. + // (mcp_config is handled separately — it IS forwarded for ACP; see + // the dedicated tests below.) agent: "CodeActAgent", llm: { model: "gpt-4", api_key: "should-not-appear" }, condenser: { enabled: true, max_size: 240 }, - mcp_config: { - mcpServers: { fake: { command: "x", args: [] } }, - }, }, }, }) as { @@ -982,6 +992,50 @@ describe("buildStartConversationRequest — ACP discriminator", () => { expect(payload.tags).toEqual({ [ACP_SERVER_TAG_KEY]: "claude-code" }); }); + it("forwards mcp_config to the ACP subprocess when servers are configured", () => { + // mcp_config is a shared field: the SDK's ACPAgent forwards these servers + // to the ACP subprocess at session creation, so the start payload must + // carry it (it is intentionally NOT one of the stripped ACP-only fields). + const payload = buildStartConversationRequest({ + settings: { + ...DEFAULT_SETTINGS, + agent_settings: { + schema_version: 1, + agent_kind: "acp", + acp_server: "claude-code", + acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"], + mcp_config: { + mcpServers: { + fetch: { command: "uvx", args: ["mcp-server-fetch"] }, + }, + }, + }, + }, + }) as { agent_settings: { mcp_config?: unknown } }; + + expect(payload.agent_settings.mcp_config).toEqual({ + mcpServers: { fetch: { command: "uvx", args: ["mcp-server-fetch"] } }, + }); + }); + + it("omits mcp_config from the ACP payload when it carries no servers", () => { + // An empty / serverless mcp_config must not be sent as ``mcp_config: {}``. + const payload = buildStartConversationRequest({ + settings: { + ...DEFAULT_SETTINGS, + agent_settings: { + schema_version: 1, + agent_kind: "acp", + acp_server: "claude-code", + acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"], + mcp_config: {}, + }, + }, + }) as { agent_settings: { mcp_config?: unknown } }; + + expect(payload.agent_settings.mcp_config).toBeUndefined(); + }); + it("does not include ACP-only fields in OpenHands agent settings", () => { const payload = buildStartConversationRequest({ settings: { diff --git a/__tests__/components/features/skills/extensions-navigation.test.tsx b/__tests__/components/features/skills/extensions-navigation.test.tsx index 1581ab65a..9bb7474e4 100644 --- a/__tests__/components/features/skills/extensions-navigation.test.tsx +++ b/__tests__/components/features/skills/extensions-navigation.test.tsx @@ -1,35 +1,11 @@ import type { ReactNode } from "react"; import { render, screen, within } from "@testing-library/react"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it } from "vitest"; import { MemoryRouter } from "react-router"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { ActiveBackendProvider } from "#/contexts/active-backend-context"; import { useSidebarStore } from "#/stores/sidebar-store"; -const useSettingsMock = vi.fn(); -vi.mock("#/hooks/query/use-settings", () => ({ - useSettings: () => useSettingsMock(), -})); - -// HeroUI's Tooltip only mounts content on real-DOM hover; stub the -// wrapper to render content eagerly so we can assert "the tooltip -// would say X" via the DOM. Mirrors the pattern in -// ``settings-navigation.test.tsx``. -vi.mock("#/components/shared/buttons/styled-tooltip", () => ({ - StyledTooltip: ({ - content, - children, - }: { - content: React.ReactNode; - children: React.ReactNode; - }) => ( - <> - {children} - {content} - - ), -})); - import { ExtensionsNavigation } from "#/components/features/skills/extensions-navigation"; function renderExtensionsNavigation(ui: ReactNode) { @@ -48,61 +24,30 @@ function renderExtensionsNavigation(ui: ReactNode) { describe("ExtensionsNavigation", () => { it("renders the MCP item as a clickable link for non-ACP agents", () => { - useSettingsMock.mockReturnValue({ - data: { agent_settings: { agent_kind: "openhands" } }, - }); - renderExtensionsNavigation(); const nav = screen.getByTestId("extensions-navbar-desktop"); const mcpItem = within(nav).getByTestId("sidebar-extensions-/mcp"); expect(mcpItem).not.toHaveAttribute("aria-disabled"); - // Active link — `NavigationLink` renders as ; the disabled - // branch renders . Tagging matters because the disabled - // version has no href, breaking direct URL navigation. + // `NavigationLink` renders as with an href so direct URL + // navigation works. expect(mcpItem.tagName).toBe("A"); }); - it("greys out the MCP item and wraps it in the ACP tooltip when ACP is active", () => { - // Regression guard for the comment in PR #416 review: with an ACP - // agent active, /mcp configuration is silently no-op (the SDK's - // ``ACPAgent`` rejects ``mcp_config`` on init). Greying the nav - // item plus the explanatory tooltip mirrors how /settings, - // /settings/condenser already behave under ACP. - useSettingsMock.mockReturnValue({ - data: { - agent_settings: { agent_kind: "acp", acp_server: "claude-code" }, - }, - }); - + it("keeps the MCP item clickable when ACP is active", () => { + // ACP agents now forward ``mcp_config`` to their subprocess at session + // creation, so the MCP page is meaningful under ACP too — it is no + // longer greyed out (unlike /settings and /settings/condenser, which + // stay inert for ACP). renderExtensionsNavigation(); const nav = screen.getByTestId("extensions-navbar-desktop"); const mcpItem = within(nav).getByTestId("sidebar-extensions-/mcp"); - expect(mcpItem).toHaveAttribute("aria-disabled", "true"); - // Disabled rendering uses , not — no href means no - // accidental navigation if the user keyboard-tabs onto it. - expect(mcpItem.tagName).toBe("SPAN"); - // The StyledTooltip mock writes its ``content`` prop into a - // . Its presence proves - // the disabled branch wrapped the link with the explanatory - // tooltip; the absence on enabled control (see Skills below) - // proves we don't over-wrap. - expect( - within(nav).queryByTestId("styled-tooltip-content"), - ).toBeInTheDocument(); + expect(mcpItem).not.toHaveAttribute("aria-disabled"); + expect(mcpItem.tagName).toBe("A"); }); - it("leaves the Skills item clickable even when ACP is active", () => { - // Skills isn't ACP-gated — the ACP subprocess can still benefit - // from rendered skills in its /system suffix. Only - // /mcp goes grey; /skills stays a normal link. - useSettingsMock.mockReturnValue({ - data: { - agent_settings: { agent_kind: "acp", acp_server: "claude-code" }, - }, - }); - + it("leaves the Skills item clickable", () => { renderExtensionsNavigation(); const nav = screen.getByTestId("extensions-navbar-desktop"); @@ -126,14 +71,6 @@ describe("ExtensionsNavigation", () => { }); } - beforeEach(() => { - // Arrange: a non-ACP agent so every nav item is clickable and the - // hide rule is the only thing that can suppress the aside. - useSettingsMock.mockReturnValue({ - data: { agent_settings: { agent_kind: "openhands" } }, - }); - }); - afterEach(() => { setViewport(originalInnerWidth); // The Zustand sidebar store is a module singleton — reset it so diff --git a/__tests__/routes/mcp.test.tsx b/__tests__/routes/mcp.test.tsx index fad014fe4..38f31f68d 100644 --- a/__tests__/routes/mcp.test.tsx +++ b/__tests__/routes/mcp.test.tsx @@ -1,68 +1,13 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { clientLoader } from "#/routes/mcp"; -import SettingsService from "#/api/settings-service/settings-service.api"; -import { __resetActiveStoreForTests } from "#/api/backend-registry/active-store"; -import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers"; -import { queryClient } from "#/query-client-config"; +import { describe, expect, it } from "vitest"; +import * as mcpRoute from "#/routes/mcp"; describe("mcp route", () => { - beforeEach(() => { - vi.restoreAllMocks(); - window.localStorage.clear(); - __resetActiveStoreForTests(); - queryClient.clear(); - }); - - afterEach(() => { - window.localStorage.clear(); - __resetActiveStoreForTests(); - queryClient.clear(); - }); - - it("redirects to /settings/agent when the active agent is ACP", async () => { - // The SDK's ``ACPAgent`` rejects ``mcp_config`` on init, so the - // /mcp editor would silently no-op against the running ACP - // subprocess. The clientLoader bounces the user to the Agent - // settings page (same UX as /settings, /settings/condenser). - vi.spyOn(SettingsService, "getSettings").mockResolvedValue({ - ...MOCK_DEFAULT_USER_SETTINGS, - agent_settings: { - ...MOCK_DEFAULT_USER_SETTINGS.agent_settings, - agent_kind: "acp", - acp_server: "claude-code", - }, - }); - - const response = (await clientLoader()) as Response; - - expect(response.status).toBe(302); - expect(response.headers.get("Location")).toBe("/settings/agent"); - }); - - it("does not redirect when the active agent is OpenHands", async () => { - vi.spyOn(SettingsService, "getSettings").mockResolvedValue({ - ...MOCK_DEFAULT_USER_SETTINGS, - agent_settings: { - ...MOCK_DEFAULT_USER_SETTINGS.agent_settings, - agent_kind: "openhands", - }, - }); - - const result = await clientLoader(); - - expect(result).toBeNull(); - }); - - it("falls through when settings can't be fetched (no redirect-loop on errors)", async () => { - // ``redirectIfAcpActive`` swallows errors and returns ``null`` so a - // transient settings-fetch failure (unauthed, offline, agent-server - // not running) doesn't trap the user on a permanent redirect. - vi.spyOn(SettingsService, "getSettings").mockRejectedValue( - new Error("network down"), - ); - - const result = await clientLoader(); - - expect(result).toBeNull(); + it("does not gate the MCP page behind an ACP redirect", () => { + // ACP agents now forward ``mcp_config`` to their subprocess at session + // creation, so the MCP page is meaningful under ACP. Unlike /settings and + // /settings/condenser (which stay inert for ACP and keep their + // ``redirectIfAcpActive`` clientLoader), /mcp must NOT export a + // clientLoader that bounces ACP users away. + expect("clientLoader" in mcpRoute).toBe(false); }); }); diff --git a/src/api/agent-server-adapter.ts b/src/api/agent-server-adapter.ts index 075cf3943..26c75e731 100644 --- a/src/api/agent-server-adapter.ts +++ b/src/api/agent-server-adapter.ts @@ -564,6 +564,15 @@ function buildConfiguredAcpAgentSettings( } } + // ``mcp_config`` is a *shared* field (not in ACP_SETTINGS_KEYS): forward it + // so the ACP subprocess connects to the configured MCP servers at session + // creation. Only include it when it actually carries servers — an empty or + // malformed value is dropped rather than sending ``mcp_config: {}``. + const mcpConfig = toRecord(agentSettings.mcp_config); + if (Object.keys(mcpConfig).length > 0 && "mcpServers" in mcpConfig) { + payload.mcp_config = mcpConfig; + } + // Saved settings may carry ``acp_model: null`` (existing users predating // the default-model registry, or saved fields the agent-server stripped). // Fall back to the provider's ``default_model`` so the conversation starts diff --git a/src/components/features/skills/extensions-navigation.tsx b/src/components/features/skills/extensions-navigation.tsx index d688b57c5..be2b5c4c1 100644 --- a/src/components/features/skills/extensions-navigation.tsx +++ b/src/components/features/skills/extensions-navigation.tsx @@ -1,8 +1,5 @@ import { useTranslation } from "react-i18next"; import { NavigationLink } from "#/components/shared/navigation-link"; -import { StyledTooltip } from "#/components/shared/buttons/styled-tooltip"; -import { useSettings } from "#/hooks/query/use-settings"; -import { ACP_PROVIDERS } from "#/constants/acp-providers"; import { cn } from "#/utils/utils"; import SkillsIcon from "#/icons/skills.svg?react"; import ServerProcessIcon from "#/icons/server-process.svg?react"; @@ -21,14 +18,6 @@ interface ExtensionNavItem { icon: React.ReactElement; end?: boolean; comingSoon?: boolean; - /** - * When true, this item greys out (and the /route's ``clientLoader`` - * bounces to ``/settings/agent``) while an ACP agent is active. - * The ACP sub-agent manages its own MCP servers; the SDK rejects - * ``mcp_config`` on ``ACPAgent`` init outright, so the OpenHands- - * side editor would silently no-op against the running subprocess. - */ - disabledByAcp?: boolean; } export const EXTENSIONS_NAV_ITEMS: ExtensionNavItem[] = [ @@ -43,7 +32,6 @@ export const EXTENSIONS_NAV_ITEMS: ExtensionNavItem[] = [ label: "MCP Servers", icon: , end: true, - disabledByAcp: true, }, { to: "/plugins", @@ -73,7 +61,6 @@ export const EXTENSIONS_NAV_ITEMS: ExtensionNavItem[] = [ export function ExtensionsNavigation() { const { t } = useTranslation("openhands"); - const { data: settings } = useSettings(); const sidebarCollapsed = useSidebarStore((state) => state.collapsed); // At iPad portrait widths (md to key === acpServerKey)?.display_name ?? - "ACP Agent") - : undefined; if (hideForExpandedSidebar) return null; @@ -103,7 +81,6 @@ export function ExtensionsNavigation() {
{EXTENSIONS_NAV_ITEMS.map((item) => { - const disabled = !!(isAcpAgent && item.disabledByAcp); const baseRow = ( {item.icon} @@ -116,37 +93,6 @@ export function ExtensionsNavigation() { ); - if (disabled) { - // Render a non-clickable surrogate so the URL and a11y tree - // both communicate "you can't go here right now," then wrap - // in StyledTooltip for the why. Mirrors the SettingsNavLink - // disabled rendering — same flag (``disabledByAcp``), same - // explanatory tooltip ("Disabled while {agentName} is the - // active agent"), same greyed styles. - return ( - - - {baseRow} - {label} - {comingSoonBadge} - - - ); - } - return ( redirectIfAcpActive(); +// No ACP guard here (unlike `/settings` and `/settings/condenser`): MCP +// servers configured via `agent_settings.mcp_config` are now forwarded to +// the ACP subprocess at session creation, so this page is meaningful for +// both OpenHands and ACP agents. The same editor and `mcp_config` storage +// drive both kinds. export default function MCPPage() { const { t } = useTranslation("openhands"); diff --git a/src/utils/acp-route-guard.ts b/src/utils/acp-route-guard.ts index eb0550650..cc5cb7ce9 100644 --- a/src/utils/acp-route-guard.ts +++ b/src/utils/acp-route-guard.ts @@ -9,12 +9,13 @@ import { queryClient } from "#/query-client-config"; * Issue a ``redirect`` to ``/settings/agent`` when the personal settings * say the active agent is ACP. * - * The ACP sub-agent owns its own LLM, MCP servers, and condenser, so the - * canvas-side surfaces that configure those concepts (``/settings``, - * ``/settings/condenser``, ``/mcp``) have nothing useful to do while ACP - * is active. Doing the redirect in a ``clientLoader`` (instead of a - * per-route ``useEffect``) prevents the one-frame flash of the old - * content before the guard fires. + * The ACP sub-agent owns its own LLM and condenser, so the canvas-side + * surfaces that configure those concepts (``/settings``, + * ``/settings/condenser``) have nothing useful to do while ACP is active. + * (``/mcp`` is intentionally *not* guarded: ``mcp_config`` is now forwarded + * to the ACP subprocess at session creation.) Doing the redirect in a + * ``clientLoader`` (instead of a per-route ``useEffect``) prevents the + * one-frame flash of the old content before the guard fires. * * ``staleTime: 0`` is intentional: the read drives a redirect, and a * 5-minute stale tolerance would let a cross-tab agent-kind flip route