feat(acp): let ACP agents configure MCP servers#995
Conversation
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) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review — feat(acp): let ACP agents configure MCP servers
🟢 Good taste — Clean, focused removal of a restriction that no longer holds now that the SDK forwards mcp_config to the ACP subprocess. The implementation is sound and well-tested.
What I looked at
| File | Observation |
|---|---|
src/api/agent-server-adapter.ts |
New mcp_config forwarding block mirrors the identical guard already used for the OpenHands agent path (buildConfiguredOpenHandsAgentSettings, line 623–628). Consistent pattern, correct behaviour. |
src/routes/mcp.tsx |
clientLoader removed cleanly; the explanatory comment is appropriately verbose given the deliberate asymmetry with /settings and /settings/condenser. |
src/components/features/skills/extensions-navigation.tsx |
disabledByAcp, useSettings, ACP_PROVIDERS imports and all conditional rendering dropped. No dead references left behind. |
src/utils/acp-route-guard.ts |
Doc update is precise — explicitly calls out that /mcp is intentionally NOT guarded. Good for future maintainers. |
__tests__/api/agent-server-adapter.test.ts |
Two new tests exercise the real code path (forwarded with servers; omitted when empty). The existing "LLM-only fields don't leak" test was correctly narrowed — mcp_config no longer belongs in that list. |
__tests__/routes/mcp.test.tsx |
expect("clientLoader" in mcpRoute).toBe(false) is an elegant regression guard. |
__tests__/components/features/skills/extensions-navigation.test.tsx |
Mocks for useSettings and StyledTooltip removed because the component no longer needs them. Tests are simpler and still catch regressions. |
One gap worth noting
Missing evidence — The PR description mentions tests passing and linting clean, but there is no screenshot or screen-recording confirming the MCP page is actually reachable and usable under an active ACP agent, nor a link to the originating agent conversation. Since this is a visible UX change (an item that was greyed-out is now a live link), a screenshot of the MCP nav item being clickable under ACP would strengthen confidence that nothing in the rendering path regressed.
Risk Assessment
⚠️ Risk: 🟢 LOWThe change removes a client-side redirect and a disabled-state UI. It cannot break existing OpenHands-agent behaviour (code paths are separate). The worst realistic regression is the MCP page loading correctly but the ACP subprocess not picking up the config — that would be caught by the SDK-level change this PR depends on (
OpenHands/software-agent-sdk#3458). No API contract changes, no new dependencies.
VERDICT: ✅ Worth merging — the core logic is correct, the test coverage matches the stated behaviours, and the net diff is a satisfying −114 lines.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better by adding a
.agents/skills/custom-codereview-guide.mdfile to your branch with the/codereviewtrigger and the context the reviewer is missing.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Live end-to-end validation (addresses the review's "missing evidence" note)Validated the full path with a real Claude Code ACP agent using local auth ( The agent actually called the MCP tool (not just echoed text): The Depends on OpenHands/software-agent-sdk#3458. |
📸 Snapshot Test Report✅ All snapshots match the main branch baselines.
✅ Unchanged snapshots (73)
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers. |
Closes #993.
What
Lets ACP agents (Claude Code / Codex / Gemini CLI) use MCP servers through the existing MCP page — no new editor needed. Previously the MCP surfaces were gated off for ACP because the SDK rejected
mcp_configonACPAgentinit.Depends on the SDK change that forwards
mcp_configto the ACP subprocess at session creation: OpenHands/software-agent-sdk#3458 (+ docs in OpenHands/typescript-client#197).Changes
agent-server-adapter.ts— includemcp_configin the ACP conversation-start payload. It's a shared field (not one of the strippedACP_SETTINGS_KEYS), handled explicitly likellm/agent_context; empty/serverless configs are dropped so we never sendmcp_config: {}./mcproute — remove theredirectIfAcpActiveclientLoader. The existing MCP editor already reads/writesagent_settings.mcp_config(viaparseMcpConfig/toSdkMcpConfigand the add/update/delete mutations), so it drives both agent kinds with zero new UI.extensions-navigation.tsx— drop the now-obsoletedisabledByAcpmechanism (MCP was its only consumer; no extensions item needs ACP-gating anymore). The MCP nav item is a normal link under ACP.acp-route-guard.ts— docs updated:/settingsand/settings/condenserstill redirect for ACP;/mcpintentionally does not.Behavior
The same
agent_settings.mcp_configstorage and editor now back both agent kinds. Saving the Agent settings page doesn't clobbermcp_config(buildAcpAgentSettingsDiffdoesn't touch it). MCP changes take effect on the next ACP session (the servers bind at session creation).Validation
End-to-end through this PR's code path (real backend, local auth). Ran the actual
buildStartConversationRequestadapter (this PR's changes) — it produced aStartConversationRequestwithagent_kind:"acp"andmcp_config. POSTed that exact request to a locally-builtopenhands-agent-serverrunning the branch SDK, viaPOST /api/conversations(the same endpoint the canvas uses) → it created the ACP conversation, forwardedmcp_configto theclaude-agent-acpsubprocess, and the agent actually called the MCP tool:Verified on the tool event's output, not the reply text. Full chain exercised: canvas adapter payload → agent-server →
ACPAgentSettings→ACPAgent(mcp_config)→ ACP subprocess → MCP server (only the browser clicks were skipped — the same HTTP API the browser drives was used).SDK layer — the underlying forwarding is independently validated against all three harnesses (Claude / Codex / Gemini) over both stdio and HTTP: OpenHands/software-agent-sdk#3458.
Tests
agent-server-adapter: forwardsmcp_configfor ACP; omits it when serverless. Refocused the existing "LLM-only fields don't leak" test.extensions-navigation: MCP item stays clickable under ACP.mcproute: regression guard that the route exports no ACP-redirect clientLoader.typecheck + eslint + prettier clean; affected suites green.
🤖 Generated with Claude Code
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.24.0-pythonopenhands-automation==1.0.0a50fcae6461694e4dc1bd3cdc7a3a159beba1287f6Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-0fcae64Run
All tags pushed for this build
About Multi-Architecture Support
sha-0fcae64) is a multi-arch manifest supporting both amd64 and arm64sha-0fcae64-amd64) are also available if needed