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.
This commit is contained in:
Rohit C Prasad
2026-08-20 13:50:13 -07:00
parent 8e71256ede
commit 5ebaa376d7
8 changed files with 362 additions and 16 deletions
+15
View File
@@ -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
+26 -1
View File
@@ -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]:
File diff suppressed because one or more lines are too long
+40 -3
View File
@@ -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([]);
+71
View File
@@ -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");
});
+2
View File
@@ -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<string, any>;
+125 -12
View File
@@ -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({
<div className="flex-1 min-w-0">
<div className="text-[14px] font-medium">{server.name}</div>
<div className="text-[11.5px] text-faint">
{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" : ""}
</div>
</div>
{!isOauth && server.auth_hint && !authorizing && (
<button
className={BTN_ACCENT}
onClick={signInWithOauth}
data-testid={`mcp-authfix-${server.name}`}
>
Sign in
</button>
)}
{isOauth &&
(server.status === "needs_auth" ? (
<button className={BTN_ACCENT} onClick={signIn} data-testid={`mcp-signin-${server.name}`}>
@@ -417,6 +450,18 @@ function McpRow({
sign out
</button>
) : null)}
{server.enabled &&
!authorizing &&
!server.auth_hint &&
!(isOauth && server.status !== "connected") && (
<button
className="text-[12px] text-muted hover:text-ink shrink-0"
onClick={runTest}
data-testid={`mcp-test-${server.name}`}
>
test
</button>
)}
<button
className="text-[12px] text-muted hover:text-ink shrink-0"
onClick={loadTools}
@@ -450,6 +495,9 @@ function McpRow({
);
}
const INPUT =
"w-full text-[13px] px-3 py-2 rounded-lg border border-line bg-paper text-ink outline-none focus:border-accent";
function AddForm({
onCancel,
onAdded,
@@ -459,9 +507,33 @@ function AddForm({
onAdded: () => void;
onError: (e: string | null) => void;
}) {
// Two doors, one flow: Remote URL (name + URL — most hosted servers) and JSON
// (the paste box — stdio and advanced configs). Both end in the row's Test.
const [tab, setTab] = useState<"url" | "json">("url");
const [name, setName] = useState("");
const [url, setUrl] = useState("");
const [text, setText] = useState(EXAMPLE);
const save = async () => {
const saveUrl = async () => {
onError(null);
const n = name.trim();
const u = url.trim();
if (!n) {
onError("Give the server a name.");
return;
}
if (!/^https?:\/\/\S+$/.test(u)) {
onError("Enter the server's full URL (https://…).");
return;
}
await addMcpServer(n, { type: "http", url: u });
// Probe anonymously right away — the row shows testing…, then connected · N
// tools, an error, or "needs sign-in" (401 → the OAuth switch).
await connectMcp(n);
onAdded();
};
const saveJson = async () => {
onError(null);
let parsed: any;
try {
@@ -486,19 +558,60 @@ function AddForm({
onAdded();
};
const tabBtn = (active: boolean) =>
"text-[12px] px-2.5 py-1 rounded-md border shrink-0 " +
(active
? "border-accent text-accent font-medium"
: "border-line text-muted hover:text-ink");
return (
<div className="space-y-2">
<div className="text-[12.5px] text-muted">Paste server JSON (name config):</div>
<textarea
value={text}
onChange={(e) => setText(e.target.value)}
spellCheck={false}
rows={9}
className="w-full font-mono text-[12px] px-3 py-2.5 rounded-lg border border-line bg-paper text-ink outline-none focus:border-accent resize-y"
/>
<div className="flex items-center gap-1.5">
<button className={tabBtn(tab === "url")} onClick={() => setTab("url")} data-testid="mcp-add-tab-url">
Remote URL
</button>
<button className={tabBtn(tab === "json")} onClick={() => setTab("json")} data-testid="mcp-add-tab-json">
JSON
</button>
</div>
{tab === "url" ? (
<>
<div className="text-[12.5px] text-muted">
Connect a hosted MCP server. If it needs sign-in, the row will offer it after the
first test.
</div>
<input
value={name}
onChange={(e) => setName(e.target.value)}
placeholder="Name (shown in the server list)"
spellCheck={false}
className={INPUT}
data-testid="mcp-add-name"
/>
<input
value={url}
onChange={(e) => setUrl(e.target.value)}
placeholder="https://mcp.example.com/mcp"
spellCheck={false}
className={INPUT + " font-mono text-[12px]"}
data-testid="mcp-add-url"
/>
</>
) : (
<>
<div className="text-[12.5px] text-muted">Paste server JSON (name config):</div>
<textarea
value={text}
onChange={(e) => setText(e.target.value)}
spellCheck={false}
rows={9}
className="w-full font-mono text-[12px] px-3 py-2.5 rounded-lg border border-line bg-paper text-ink outline-none focus:border-accent resize-y"
/>
</>
)}
<div className="flex items-center gap-3">
<button className={BTN_ACCENT} onClick={save}>
Add
<button className={BTN_ACCENT} onClick={tab === "url" ? saveUrl : saveJson}>
{tab === "url" ? "Add & test" : "Add"}
</button>
<button className="text-[12.5px] text-muted hover:text-ink" onClick={onCancel}>
cancel
+82
View File
@@ -312,3 +312,85 @@ async def test_prepare_records_failure_status_and_session_notice(
assert [n for n, _ in drained] == ["sales-db"]
assert "boom: bad args" in (drained[0][1] or "")
assert manager.pop_mcp_failures("s1") == [] # one-shot
# -- explicit connect (UX-033: add → Test → fix, without opening a session) ------
@pytest.mark.asyncio
async def test_connect_mcp_failure_includes_stderr_tail(tmp_path, monkeypatch):
"""The Test button's connect path reports the same stderr evidence as the
session path a crashing stdio server yields error + status=error."""
monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state"))
_write_json(
tmp_path / "state" / "mcp.json",
{
"mcpServers": {
"doomed": {
"command": "/bin/sh",
"args": ["-c", "echo 'usage: doomed --flag' >&2; exit 7"],
"enabled": True,
}
}
},
)
manager = SessionManager(data_dir=tmp_path / "data")
result = await manager.connect_mcp("doomed")
assert result["ok"] is False
assert "usage: doomed --flag" in result["error"]
listed = {s["name"]: s for s in manager.list_mcp()}
assert listed["doomed"]["status"] == "error"
assert listed["doomed"]["auth_hint"] is False
# Removing the server takes its stale failure state with it.
manager.delete_mcp("doomed")
assert manager._mcp_errors.get("doomed") is None
@pytest.mark.asyncio
async def test_connect_mcp_http_401_sets_auth_hint(tmp_path, monkeypatch):
"""An anonymous connect that hits 401 is reported as "needs sign-in" (the GUI
offers the OAuth switch), not as a raw HTTP error dump."""
import http.server
import threading
class _Deny(http.server.BaseHTTPRequestHandler):
def _deny(self):
self.send_response(401)
self.send_header("Content-Length", "0")
self.end_headers()
do_GET = do_POST = do_DELETE = _deny
def log_message(self, *args): # keep pytest output clean
pass
srv = http.server.ThreadingHTTPServer(("127.0.0.1", 0), _Deny)
threading.Thread(target=srv.serve_forever, daemon=True).start()
try:
monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state"))
_write_json(
tmp_path / "state" / "mcp.json",
{
"mcpServers": {
"guarded": {
"url": f"http://127.0.0.1:{srv.server_address[1]}/mcp",
"enabled": True,
}
}
},
)
manager = SessionManager(data_dir=tmp_path / "data")
result = await manager.connect_mcp("guarded")
assert result["ok"] is False
assert "sign in" in result["error"]
listed = {s["name"]: s for s in manager.list_mcp()}
assert listed["guarded"]["auth_hint"] is True
assert listed["guarded"]["status"] == "error"
finally:
srv.shutdown()
srv.server_close()