Repository navigation
Conversation
…Agent Canvas is deployed
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Review posted for
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
The flag plumbing is correct. ENABLE_AGENT_CANVAS_MCP goes through _env_flag_enabled, so 'true' and '1' both enable it, and it is off by default. Framing works under this app's own headers: CSP has frame-src 'self' and frame-ancestors 'self', and X-Frame-Options is SAMEORIGIN. Saving the search key sends only search_api_key, so it cannot overwrite server edits made in the frame. The add-integration setup step already points at /canvas/mcp and is marked done from server state, so it does not depend on the removed Add Server button. CI is green on b31ecdf, and npx vitest run __tests__/routes/mcp-settings.test.tsx passes locally (10/10). I could not run the Python injector tests locally because the sandbox has no Docker or testcontainers. The CI Python shards pass.
Blocking: live evidence is missing. This changes the UI of /settings/mcp. The PR template asks for a video or screenshots of a new GUI feature being tested, and the description says the recording is "to follow". Please attach a recording or screenshots from a running install with ENABLE_AGENT_CANVAS_MCP=true and a Canvas build that includes OpenHands/OpenHands#19134. It should show /settings/mcp rendering /canvas/mcp?embed=1 in the frame, a server being added or edited there, and the change persisting after a reload. The unit tests and mock UI cannot stand in for this; the description itself says the mock has no Canvas to frame.
The search-key section is not shown on OHE (see inline). The PR says the Tavily key stays on the classic page. But that section is still wrapped in !isSaasMode, and OHE runs the enterprise server with app_mode: "saas" (server/config.py). With the flag on, the OHE page is therefore just the header and the frame. The new test passes only because createMockWebClientConfig defaults to app_mode: "oss". I reran the same test with app_mode: "saas" and mcp-search-settings-section was not found. This affects the comparison with #1106: on OHE, the frame option currently keeps nothing on the classic page that the link option would lose.
Deployment order (non-blocking). OpenHands/OpenHands#19134 (the ?embed=1 support) is still an open draft. The Canvas build pinned in this repo (v1.32.0 in frontend/scripts/build-agent-canvas.sh) has a /mcp route but no embed mode. If the OpenHands-Cloud chart turns this flag on before the Canvas image includes #19134, the frame will load the whole Canvas app, sidebar included. Users could then navigate inside the frame to conversations and other pages from within Settings. Please make the chart change depend on a Canvas release that contains #19134.
Minor: AGENTS.md asks for a '1' test next to the 'true' test for new env toggles. Only 'true' is covered in test_default_web_client_config_injector.py.
🔄 CHANGES REQUESTED
…Agent Canvas is deployed
|
Addressed in ccd22d8:
Generated by Claude Code |
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Re-review of ccd22d8. Every finding from my earlier review is now resolved:
- Live evidence: resolved. The description now has screenshots and a recording from a running OHE 0.90.1-based install. They show
/settings/mcpframing Canvas's MCP page with no Canvas sidebar, a server renamed after its key was re-entered, and a second server added inside the frame. That is the production path the flag enables. - Search key on OHE (SaaS mode): resolved. The description now says the OHE page holds only the frame. A new route test pins the
app_mode: "saas"case: the frame shows, and neither the server list nor the search section renders. I resolved that thread. - Deployment order: the description now says the chart flag (OpenHands-Cloud#1508) must ship only with a Canvas release that includes OpenHands/OpenHands#19134.
'1'toggle test: added, along with a test for the structuredOH_WEB_CLIENT_FEATURE_FLAGS_ENABLE_AGENT_CANVAS_MCPform.
This revision changes only tests, and I found nothing new. Locally, the route tests pass (11/11), and so do the four enable_agent_canvas_mcp injector tests. All CI checks on this head pass.
✅ APPROVED
LLM profile: all-hands-bot-reviewer_claude-opus-5-5 · Model: openhands/claude-opus-5-5
HUMAN:
AGENT:
Why
An OHE install with Agent Canvas has two MCP server editors writing the same server list: Canvas's MCP page (Customize → MCP Servers) and the classic Account settings page at
/settings/mcp. They behave differently. On 0.90.1 the classic page cannot name remote servers at all (they are saved asshttp,shttp_1), while Canvas can. OHE users work in Canvas, so the classic editor mostly adds confusion.Summary
enable_agent_canvas_mcp(envENABLE_AGENT_CANVAS_MCP), off by default. The OHE chart is meant to turn it on whenever agent-canvas is deployed (separate OpenHands-Cloud PR)./canvas/mcp?embed=1, same origin and session) in place of its server list and Add Server button, so users edit servers in one place without leaving settings.Issue Number
FDE-254
How to Test
11 route tests pass (including one in SaaS mode, as on OHE); the new frame test fails without this change. 111 config-injector tests pass (the flag from
true,1and the structuredOH_WEB_CLIENT_FEATURE_FLAGS_ENABLE_AGENT_CANVAS_MCPform). Lint, typecheck and translation completeness pass. Mock UI:VITE_MOCK_AGENT_CANVAS_MCP=true npm run dev:mock, then open/settings/mcp.Type
Notes
Ship the chart flag (OpenHands/OpenHands-Cloud#1508) only with an Agent Canvas release that includes OpenHands/OpenHands#19134; with an older Canvas the frame shows the full Canvas app, sidebar included.
One of two options being compared for one MCP editor on OHE; the other (#1106) links to the Canvas page instead of framing it. Needs OpenHands/OpenHands#19134 (
?embed=1renders Canvas without its sidebar and navigation); without it the frame shows Canvas's full layout. Works with OpenHands/OpenHands#19132, which lets Canvas rename a server once its credential is re-entered.Video/Screenshots
Live on a test install running the OHE 0.90.1 versions (enterprise-server 1.79.0, Agent Canvas 1.32.0) with test builds of this change and OpenHands/OpenHands#19132 and #19134,
ENABLE_AGENT_CANVAS_MCPset throughOH_WEB_CLIENT_FEATURE_FLAGS_ENABLE_AGENT_CANVAS_MCP. A server saved asshttpwith a${SECRET}key was edited inside the frame on the classic page, renamed tosonarqubeafter re-entering the key, a second serverdocswas added, and one conversation calledsonarqube_read_wiki_structure. Test account cleaned up and the install restored afterwards. Two trade-offs show in the recording: the page and Canvas both show a "Model Context Protocol (MCP)" heading, and Canvas dialogs dim only the frame.In order: the classic page, today vs. hand-off vs. embed (right is this PR); rename after re-entering the key; recording of the flow.
2.mp4
Enterprise server image for this PR: