From 5ebaa376d75d7bfb6d91ecf854b2b1ad8fb3fc95 Mon Sep 17 00:00:00 2001 From: Rohit C Prasad Date: Thu, 20 Aug 2026 13:50:13 -0700 Subject: [PATCH] Add MCP server flow: Remote URL + JSON tabs, Test connection Explicit connect reports stderr tails; a 401 on an anonymous http probe becomes needs-sign-in with a one-click OAuth switch. Test button probes any enabled server row without opening a session. --- coworker/mcp/oauth.py | 15 ++ coworker/server/manager.py | 27 +++- .../results.json | 1 + surfaces/gui/e2e/fixtures.ts | 43 +++++- surfaces/gui/e2e/mcp-add-test.spec.ts | 71 +++++++++ surfaces/gui/src/api.ts | 2 + surfaces/gui/src/components/ManageTabs.tsx | 137 ++++++++++++++++-- tests/test_mcp.py | 82 +++++++++++ 8 files changed, 362 insertions(+), 16 deletions(-) create mode 100644 node_modules/.vite/vitest/da39a3ee5e6b4b0d3255bfef95601890afd80709/results.json create mode 100644 surfaces/gui/e2e/mcp-add-test.spec.ts diff --git a/coworker/mcp/oauth.py b/coworker/mcp/oauth.py index c6043e69..d6bcf2af 100644 --- a/coworker/mcp/oauth.py +++ b/coworker/mcp/oauth.py @@ -113,6 +113,21 @@ def is_auth_required(exc: BaseException) -> bool: return is_auth_required(cause) if cause is not None else False +def is_http_auth_error(exc: BaseException) -> bool: + """True if an HTTP 401/403 is anywhere in the exception tree — an anonymous + connect hit a server that wants credentials, so the fix is sign-in (switch + the entry to `auth: oauth`), not a different config. Same tree walk as + is_auth_required: the transport's task groups wrap and chain freely.""" + status = getattr(getattr(exc, "response", None), "status_code", None) + if status in (401, 403): + return True + for sub in getattr(exc, "exceptions", None) or []: # ExceptionGroup + if is_http_auth_error(sub): + return True + cause = exc.__cause__ or exc.__context__ + return is_http_auth_error(cause) if cause is not None else False + + # -- single-slot interactive flow ------------------------------------------------ _pending: Optional[asyncio.Future] = None # The last authorize URL we sent the user to — surfaced over REST so the GUI can offer diff --git a/coworker/server/manager.py b/coworker/server/manager.py index 9e4be4c8..492dc833 100644 --- a/coworker/server/manager.py +++ b/coworker/server/manager.py @@ -166,6 +166,9 @@ class SessionManager: # feeds list_mcp's status so the GUI can show "authorizing…" and failures. self._mcp_authorizing: set[str] = set() self._mcp_errors: dict[str, str] = {} + # http servers whose anonymous connect came back 401/403 — the failure is + # "needs sign-in", so the GUI offers the OAuth switch instead of a raw error. + self._mcp_auth_hints: set[str] = set() # Servers that failed to connect while preparing a session's tools — # drained once by the WS handler to append a transcript notice. self._mcp_session_failures: dict[str, list[str]] = {} @@ -1060,6 +1063,7 @@ class SessionManager: "requires_approval": bool(raw.get("requires_approval", True)), "auth": "oauth" if is_oauth else None, "status": status, + "auth_hint": name in self._mcp_auth_hints, "last_error": self._mcp_errors.get(name), "tool_count": ( len(self.mcp._conns[name].tools) if connected else None @@ -1073,6 +1077,8 @@ class SessionManager: """Connect one server NOW — for OAuth servers this may open the browser and wait for the loopback callback, so callers run it as a background task and watch list_mcp for the status flip.""" + from ..mcp import oauth as mcp_oauth + for server in load_mcp_servers( self.default_workspace, secrets=self.secrets, @@ -1082,12 +1088,27 @@ class SessionManager: continue self._mcp_authorizing.add(name) self._mcp_errors.pop(name, None) + self._mcp_auth_hints.discard(name) try: # The ONE place a browser sign-in may start: an explicit connect. conn = await self.mcp.ensure(server, interactive=True) return {"ok": True, "tools": len(conn.tools)} except Exception as exc: - self._mcp_errors[name] = str(exc) or exc.__class__.__name__ + if ( + server.transport == "http" + and server.auth != "oauth" + and mcp_oauth.is_http_auth_error(exc) + ): + # Anonymous probe of a guarded server (the add-by-URL flow): + # the answer is sign-in, not a raw 401 dump. + self._mcp_auth_hints.add(name) + msg = "authentication required — sign in to connect" + else: + msg = str(exc) or exc.__class__.__name__ + tail = self.mcp.last_stderr(name) + if tail: + msg = f"{msg} — {tail}" + self._mcp_errors[name] = msg[:500] return {"ok": False, "error": self._mcp_errors[name]} finally: self._mcp_authorizing.discard(name) @@ -1151,6 +1172,10 @@ class SessionManager: def delete_mcp(self, name: str) -> dict[str, Any]: ok = delete_global_server(name) + if ok: + # A later re-add under the same name starts clean, not pre-failed. + self._mcp_errors.pop(name, None) + self._mcp_auth_hints.discard(name) return {"ok": ok, "name": name} async def mcp_tools(self, name: str) -> dict[str, Any]: diff --git a/node_modules/.vite/vitest/da39a3ee5e6b4b0d3255bfef95601890afd80709/results.json b/node_modules/.vite/vitest/da39a3ee5e6b4b0d3255bfef95601890afd80709/results.json new file mode 100644 index 00000000..713bd5c7 --- /dev/null +++ b/node_modules/.vite/vitest/da39a3ee5e6b4b0d3255bfef95601890afd80709/results.json @@ -0,0 +1 @@ +{"version":"4.1.11","results":[[":surfaces/gui/e2e/ask-upgrades.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/onboarding.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/components/Transcript.test.tsx",{"duration":8.069375000000036,"failed":true}],[":surfaces/gui/src/components/Sidebar.test.tsx",{"duration":6.320917000000009,"failed":true}],[":surfaces/gui/src/components/SkillsTab.test.tsx",{"duration":7.332417000000021,"failed":true}],[":surfaces/gui/src/components/Composer.skills.test.tsx",{"duration":8.12216699999999,"failed":true}],[":surfaces/gui/src/components/ApprovalCard.test.tsx",{"duration":8.156458000000043,"failed":true}],[":surfaces/gui/e2e/automations-quickstart.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/components/PersonaView.test.tsx",{"duration":4.019416999999976,"failed":true}],[":surfaces/gui/e2e/access-section.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/settings.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/github-page.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/gallery.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/unattended.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/slack-workspaces.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/sidebar-sessions.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/sources-channels.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/itemsFromMessages.test.ts",{"duration":3.396917000000002,"failed":false}],[":surfaces/gui/e2e/usage-chip.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/composer.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/providers/ProviderSetup.test.tsx",{"duration":4.667082999999991,"failed":true}],[":surfaces/gui/e2e/standing-approvals.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/chat.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/hubspot-page.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/persona-surfacing.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/session-shell.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/components/Composer.voice.test.tsx",{"duration":5.643791999999991,"failed":true}],[":surfaces/gui/e2e/cloud.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/session-intro.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/inbox.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/gmail-page.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/slack-directory.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/provider-keys.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/approval-card.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e-live/persona-install.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/components/ConnectorMessageCard.test.tsx",{"duration":5.102540999999974,"failed":true}],[":surfaces/gui/src/components/UpdateBanner.test.tsx",{"duration":12.045458999999994,"failed":true}],[":surfaces/gui/e2e/transcript-scroll.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/mcp-add-test.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/sidebar-automations.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/compaction.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/slack-health.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/sidebar-account.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/accounts-page.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/skills-settings.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/connectors-list.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/slack-howitworks.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/mcp-connectors.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/google-paused.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/connectors/ConnectorIcon.test.tsx",{"duration":4.432082999999977,"failed":true}],[":surfaces/gui/src/usage.test.ts",{"duration":3.7393339999999995,"failed":false}],[":surfaces/gui/e2e/connector-page.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/roots.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/gcal-page.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/automations-manage.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/cloud-status-pending.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/available-detail.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/error-retry.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/components/ModelChecklist.test.tsx",{"duration":3.568208000000027,"failed":true}],[":surfaces/gui/e2e/nav-collapse.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/family-gate.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/skills-session.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/boot.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/automation-toast.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/skills-upload.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/streamGate.test.ts",{"duration":2.195999999999998,"failed":false}],[":surfaces/gui/e2e-live/inbox.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e-live/persistence.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/cloud-signin-placement.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/mcp-oauth.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/components/Markdown.test.tsx",{"duration":6.359124999999949,"failed":true}],[":surfaces/gui/e2e-live/api-smoke.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/automations.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e-live/approval.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e-live/fib.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/reasoning.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/interrupt-partial.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/model-switch.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/skills-forcerun.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/api.auth.test.ts",{"duration":10.473583000000005,"failed":false}],[":surfaces/gui/e2e/composer-platform.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/composer-model-loading.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/src/humanize.skills.test.ts",{"duration":2.190916999999999,"failed":false}],[":surfaces/gui/e2e/sidebar-rows.spec.ts",{"duration":0,"failed":true}],[":surfaces/gui/e2e/smoke.spec.ts",{"duration":0,"failed":true}]]} \ No newline at end of file diff --git a/surfaces/gui/e2e/fixtures.ts b/surfaces/gui/e2e/fixtures.ts index ec4deba7..51519488 100644 --- a/surfaces/gui/e2e/fixtures.ts +++ b/surfaces/gui/e2e/fixtures.ts @@ -1531,8 +1531,16 @@ export async function mockApi(page: import("@playwright/test").Page) { if (p.endsWith("/v1/mcp") && m === "GET") { for (const s2 of mcpServers) { if (s2.status === "authorizing" && s2._flip) { - s2.status = "connected"; - s2.tool_count = 6; + // Servers named locked-* simulate a guarded remote: the anonymous + // probe 401s (→ needs sign-in) until the entry is switched to oauth. + if (s2.name.startsWith("locked") && s2.auth !== "oauth") { + s2.status = "error"; + s2.auth_hint = true; + s2.last_error = "authentication required — sign in to connect"; + } else { + s2.status = "connected"; + s2.tool_count = 6; + } } if (s2.status === "authorizing") s2._flip = true; } @@ -1547,6 +1555,7 @@ export async function mockApi(page: import("@playwright/test").Page) { requires_approval: true, auth: b.config?.auth === "oauth" ? "oauth" : null, status: b.config?.auth === "oauth" ? "needs_auth" : "configured", + auth_hint: false, last_error: null, tool_count: null, config: b.config || {}, @@ -1557,7 +1566,12 @@ export async function mockApi(page: import("@playwright/test").Page) { const mc = p.match(/\/v1\/mcp\/([^/]+)\/connect$/); if (mc && m === "POST") { const s2 = mcpServers.find((x) => x.name === decodeURIComponent(mc[1])); - if (s2) s2.status = "authorizing"; + if (s2) { + s2.status = "authorizing"; + s2.auth_hint = false; + s2.last_error = null; + s2._flip = false; + } return json({ ok: true, started: true }); } const ms = p.match(/\/v1\/mcp\/([^/]+)\/signout$/); @@ -1570,6 +1584,29 @@ export async function mockApi(page: import("@playwright/test").Page) { } return json({ ok: true }); } + const mp = p.match(/\/v1\/mcp\/([^/]+)$/); + if (mp && m === "PATCH") { + const s2 = mcpServers.find((x) => x.name === decodeURIComponent(mp[1])); + const b = req.postDataJSON() || {}; + if (s2) { + if (b.enabled !== undefined) s2.enabled = b.enabled; + if (b.auth === "oauth") { + // The needs-sign-in fix: entry switches to oauth; the follow-up + // connect runs the browser flow. + s2.auth = "oauth"; + s2.auth_hint = false; + s2.status = "needs_auth"; + } + s2.config = { ...s2.config, ...b }; + } + return json({ ok: !!s2, name: mp[1] }); + } + const md = p.match(/\/v1\/mcp\/([^/]+)$/); + if (md && m === "DELETE") { + const i = mcpServers.findIndex((x) => x.name === decodeURIComponent(md[1])); + if (i >= 0) mcpServers.splice(i, 1); + return json({ ok: i >= 0 }); + } } if (p.endsWith("/v1/unrouted")) return json([]); diff --git a/surfaces/gui/e2e/mcp-add-test.spec.ts b/surfaces/gui/e2e/mcp-add-test.spec.ts new file mode 100644 index 00000000..26e99d0e --- /dev/null +++ b/surfaces/gui/e2e/mcp-add-test.spec.ts @@ -0,0 +1,71 @@ +// UX-033: the Add MCP server flow (Remote URL + JSON tabs) and the Test button. +// Remote URL adds an http entry and probes it immediately (testing… → connected); +// a guarded server (mock: locked-*) lands on "needs sign-in" with the OAuth switch; +// the JSON paste box remains for stdio/advanced, and every row can be re-tested. +import { expect } from "@playwright/test"; +import { test } from "./fixtures"; + +async function openMcpTab(page) { + await page.goto("/"); + await page.getByTestId("account-row").click(); + await page.getByRole("button", { name: "Connectors", exact: true }).click(); + await page.getByRole("button", { name: "MCP servers", exact: true }).click(); +} + +test("remote URL tab: add & test flips to connected with tool count", async ({ page }) => { + await openMcpTab(page); + await page.getByRole("button", { name: "Add a server" }).click(); + + // URL tab is the default door; bad URL is caught before anything is added. + await page.getByTestId("mcp-add-name").fill("notes"); + await page.getByTestId("mcp-add-url").fill("mcp.example.com/mcp"); + await page.getByRole("button", { name: "Add & test" }).click(); + await expect(page.getByText("Enter the server's full URL")).toBeVisible(); + + await page.getByTestId("mcp-add-url").fill("https://mcp.example.com/mcp"); + await page.getByRole("button", { name: "Add & test" }).click(); + + const row = page.locator(".space-y-2 > div").filter({ hasText: "notes" }).first(); + await expect(row).toContainText("testing…"); + await expect(row).toContainText("connected", { timeout: 10_000 }); + await expect(row).toContainText("6 tools"); +}); + +test("guarded server: 401 → needs sign-in → OAuth switch connects", async ({ page }) => { + await openMcpTab(page); + await page.getByRole("button", { name: "Add a server" }).click(); + await page.getByTestId("mcp-add-name").fill("locked-crm"); + await page.getByTestId("mcp-add-url").fill("https://mcp.locked.example/mcp"); + await page.getByRole("button", { name: "Add & test" }).click(); + + // The anonymous probe 401s: the row says needs sign-in and offers the fix. + const row = page.locator(".space-y-2 > div").filter({ hasText: "locked-crm" }).first(); + await expect(row).toContainText("needs sign-in", { timeout: 10_000 }); + await expect(row).toContainText("authentication required"); + + // Sign in switches the entry to oauth and starts the browser flow; the poll + // flips it to connected. + await row.getByTestId("mcp-authfix-locked-crm").click(); + await expect(row).toContainText("signing in…"); + await expect(row).toContainText("connected", { timeout: 10_000 }); + await expect(row).toContainText("oauth"); +}); + +test("JSON tab still adds stdio servers; Test probes an existing row", async ({ page }) => { + await openMcpTab(page); + await page.getByRole("button", { name: "Add a server" }).click(); + await page.getByTestId("mcp-add-tab-json").click(); + await page + .locator("textarea") + .fill('{"files": {"command": "npx", "args": ["-y", "@modelcontextprotocol/server-filesystem"]}}'); + await page.getByRole("button", { name: "Add", exact: true }).click(); + + const row = page.locator(".space-y-2 > div").filter({ hasText: "files" }).first(); + await expect(row).toContainText("stdio · configured"); + + // Test on the untouched row: testing… then the mock's connected · 6 tools. + await row.getByTestId("mcp-test-files").click(); + await expect(row).toContainText("testing…"); + await expect(row).toContainText("connected", { timeout: 10_000 }); + await expect(row).toContainText("6 tools"); +}); diff --git a/surfaces/gui/src/api.ts b/surfaces/gui/src/api.ts index 13cca52e..3638959c 100644 --- a/surfaces/gui/src/api.ts +++ b/surfaces/gui/src/api.ts @@ -273,6 +273,8 @@ export interface McpServer { // "needs_auth" (no tokens yet) | "authorizing" (browser sign-in in flight) status: string; auth?: "oauth" | null; + // http server whose anonymous connect hit a 401/403 — offer OAuth sign-in. + auth_hint?: boolean; last_error?: string | null; tool_count: number | null; config: Record; diff --git a/surfaces/gui/src/components/ManageTabs.tsx b/surfaces/gui/src/components/ManageTabs.tsx index f9ab2992..e78e369d 100644 --- a/surfaces/gui/src/components/ManageTabs.tsx +++ b/surfaces/gui/src/components/ManageTabs.tsx @@ -374,6 +374,23 @@ function McpRow({ await signoutMcp(server.name); onRefresh(); }; + // Test = the same explicit connect the OAuth Sign-in uses, for every server: + // the row flips to "testing…" and the tab's poll lands on connected · N tools + // or the error/stderr excerpt. A connected server just reports live state. + const runTest = async () => { + await connectMcp(server.name); + onRefresh(); + // The connect runs as a background task; if the first refresh outpaced its + // start, the row never shows "authorizing" and the tab's poll never arms. + window.setTimeout(onRefresh, 600); + }; + // Anonymous connect came back 401/403: the fix is sign-in, so switch the entry + // to OAuth (DCR — nothing to register) and start the browser flow right away. + const signInWithOauth = async () => { + await patchMcpServer(server.name, { auth: "oauth" }); + await connectMcp(server.name); + onRefresh(); + }; const loadTools = async () => { if (tools) { @@ -395,12 +412,28 @@ function McpRow({
{server.name}
- {server.transport} · {authorizing ? "signing in…" : server.status.replace("_", " ")} + {server.transport} ·{" "} + {authorizing + ? isOauth + ? "signing in…" + : "testing…" + : server.auth_hint && !isOauth + ? "needs sign-in" + : server.status.replace("_", " ")} {server.tool_count != null ? ` · ${server.tool_count} tools` : ""} {server.requires_approval ? " · asks" : ""} {isOauth ? " · oauth" : ""}
+ {!isOauth && server.auth_hint && !authorizing && ( + + )} {isOauth && (server.status === "needs_auth" ? ( ) : null)} + {server.enabled && + !authorizing && + !server.auth_hint && + !(isOauth && server.status !== "connected") && ( + + )}