feat(acp): let ACP agents configure MCP servers

__tests__/api/agent-server-adapter.test.ts

@@ -480,7 +480,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<string, unknown> & { llm: Record<string, unknown> };
+      agent_settings: Record<string, unknown> & {
+        llm: Record<string, unknown>;
+      };
     };
 
     function getModelFrom(
@@ -510,10 +512,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", {}],
     ])(
@@ -945,12 +956,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 {
@@ -983,6 +993,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: {

__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}
-      <span data-testid="styled-tooltip-content">{content}</span>
-    </>
-  ),
-}));
-
 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(<ExtensionsNavigation />);
 
     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 <a>; the disabled
-    // branch renders <span>. Tagging matters because the disabled
-    // version has no href, breaking direct URL navigation.
+    // `NavigationLink` renders as <a> 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(<ExtensionsNavigation />);
 
     const nav = screen.getByTestId("extensions-navbar-desktop");
     const mcpItem = within(nav).getByTestId("sidebar-extensions-/mcp");
-    expect(mcpItem).toHaveAttribute("aria-disabled", "true");
-    // Disabled rendering uses <span>, not <a> — 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
-    // <span data-testid="styled-tooltip-content">. 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 <CUSTOM_SECRETS>/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(<ExtensionsNavigation />);
 
     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

__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);
   });
 });

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