Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion src/agent/tools-mcp-disconnect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const closedGenerations: number[] = [];
let connectGeneration = 0;
let connectOptions: MCPConnectOptions[] = [];
let releaseDeferredConnect: (() => void) | undefined;
let connectMode: "success" | "deferred" = "success";
let connectMode: "success" | "deferred" | "auth-pending" = "success";
// Reconnect tests repoint this to simulate a server whose tool set drifted
// between generations; the default matches the original static payload.
let connectedTools: MCPTool[] = [
Expand Down Expand Up @@ -56,6 +56,14 @@ await withMockedModule(
};
}
}
if (connectMode === "auth-pending") {
return {
ok: false as const,
serverName: config.name,
error: "timed out waiting for the browser",
authPending: true,
};
}
return {
ok: true as const,
client: {
Expand Down Expand Up @@ -440,4 +448,25 @@ describe("setMcpServersSource", () => {
await toolset.dispose();
}
});

test("an auth-pending connect result reaches onStatus marked as such", async () => {
const toolset = await makeToolset();
const states: MCPServerState[] = [];
connectMode = "auth-pending";
try {
await toolset.connectMCPServer(acme, callbacks(states));
const failed = states.filter((s) => s.state === "failed");
expect(failed).toEqual([
{
name: "acme",
state: "failed",
error: "timed out waiting for the browser",
authPending: true,
},
]);
expect(toolset.hasMCPServer("acme")).toBe(false);
} finally {
await toolset.dispose();
}
});
});
19 changes: 17 additions & 2 deletions src/agent/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
} from "../plugins/result-truncation-plugin.js";
import type { CompactionArchive } from "../session/compaction-archive.js";
import {
BrowserAuthPendingError,
connectMCPServer as connectMCPClient,
type MCPClient,
type MCPConnectResult,
Expand Down Expand Up @@ -267,7 +268,13 @@ export type MCPServerState =
| { name: string; state: "connecting" }
| { name: string; state: "needs-auth"; url: string }
| { name: string; state: "connected"; tools: string[] }
| { name: string; state: "failed"; error: string }
| {
name: string;
state: "failed";
error: string;
/** Browser auth was offered but never finished — the auth marker owns it. */
authPending?: boolean;
}
| { name: string; state: "disconnected" };

export interface MCPConnectCallbacks {
Expand Down Expand Up @@ -956,7 +963,14 @@ export async function createAgentToolset(
});
}
if (!disposed)
callbacks.onStatus({ name: config.name, state: "failed", error });
callbacks.onStatus({
name: config.name,
state: "failed",
error,
...(err instanceof BrowserAuthPendingError
? { authPending: true }
: {}),
});
return;
}
if (disposed) {
Expand All @@ -981,6 +995,7 @@ export async function createAgentToolset(
name: config.name,
state: "failed",
error: result.error,
...(result.authPending === true ? { authPending: true } : {}),
});
return;
}
Expand Down
25 changes: 24 additions & 1 deletion src/mcp/client-auth-reauth-cap.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,14 +266,21 @@ const config = {
async function connectWithAuthPrompt(): Promise<{
ok: boolean;
error?: string;
authPending?: boolean;
}> {
const result = await connectMCPServer(config, {
onAuthURL: () => {
authURLCount += 1;
authEvents.push("authURL");
},
});
return result.ok ? { ok: true } : { ok: false, error: result.error };
return result.ok
? { ok: true }
: {
ok: false,
error: result.error,
...(result.authPending === true ? { authPending: true } : {}),
};
}

describe("HTTP MCP re-auth loop prevention", () => {
Expand Down Expand Up @@ -755,6 +762,9 @@ describe("HTTP MCP re-auth loop prevention", () => {
for (let episode = 0; episode < 2; episode += 1) {
const result = await connectWithAuthPrompt();
expect(result.ok).toBe(false);
// The cap is an unfinished authorization, not a dead server: the TUI
// keeps the prompt-box auth marker rather than painting a failure row.
expect(result.authPending).toBe(true);
expect(result.error).toContain(
`MCP authorization for linear failed after ${MAX_BROWSER_AUTH_ATTEMPTS} ${MAX_BROWSER_AUTH_ATTEMPTS === 1 ? "attempt" : "attempts"}`,
);
Expand Down Expand Up @@ -817,6 +827,7 @@ describe("HTTP MCP re-auth loop prevention", () => {
expect(await connectWithAuthPrompt()).toEqual({
ok: false,
error: expect.stringContaining("retrying paused"),
authPending: true,
});
expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS);

Expand Down Expand Up @@ -1008,14 +1019,26 @@ describe("HTTP MCP re-auth loop prevention", () => {
const result = await connectWithAuthPrompt();

expect(result.ok).toBe(false);
expect(result.authPending).toBe(true);
expect(result.error).toContain("timed out waiting for the browser");
expect(result.error).toContain("disconnected");
expect(authURLCount).toBe(1);
expect(waitForCodeCalls).toBe(1);

const capped = await connectWithAuthPrompt();
expect(capped.ok).toBe(false);
expect(capped.authPending).toBe(true);
expect(capped.error).toContain("retrying paused");
expect(authURLCount).toBe(1);
});

test("a failure that is not the authorization itself is not auth-pending", async () => {
connectFailuresLeft = Number.POSITIVE_INFINITY;

const result = await connectWithAuthPrompt();

expect(result.ok).toBe(false);
expect(result.error).toContain("finishAuth exploded");
expect(result.authPending).toBeUndefined();
});
});
23 changes: 20 additions & 3 deletions src/mcp/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,16 @@ export interface MCPClient {

export type MCPConnectResult =
| { ok: true; client: MCPClient }
| { ok: false; serverName: string; error: string };
| {
ok: false;
serverName: string;
error: string;
/**
* The failure is a browser authorization that was offered but never
* finished — a standing operator action, not a dead server.
*/
authPending?: boolean;
};
export interface MCPConnectOptions {
stderr?: "inherit" | "ignore" | "pipe";
onAuthURL?: (serverName: string, authorizationUrl: string) => void;
Expand Down Expand Up @@ -165,20 +174,27 @@ export function setBrowserAuthWaitMs(ms: number): void {
browserAuthWaitMs = ms;
}

/**
* Browser authorization was offered but never finished — the wait timed out
* or hit the attempt cap. The TUI keeps the prompt-box auth marker for these
* instead of painting a generic connect-failure row.
*/
export class BrowserAuthPendingError extends Error {}

function browserAuthCapError(serverName: string): Error {
const minutes = Math.round(BROWSER_AUTH_COOLDOWN_MS / 60_000);
const attempts =
MAX_BROWSER_AUTH_ATTEMPTS === 1
? "1 attempt"
: `${String(MAX_BROWSER_AUTH_ATTEMPTS)} attempts`;
return new Error(
return new BrowserAuthPendingError(
`MCP authorization for ${serverName} failed after ${attempts}; ` +
`retrying paused for ${minutes} minutes. Retry later after the cooldown.`,
);
}

function browserAuthWaitError(serverName: string): Error {
return new Error(
return new BrowserAuthPendingError(
`MCP authorization for ${serverName} timed out waiting for the browser; ` +
`the server is disconnected. Retry later after the cooldown.`,
);
Expand Down Expand Up @@ -735,6 +751,7 @@ async function connectHttp(
ok: false,
serverName: config.name,
error: err instanceof Error ? err.message : String(err),
...(err instanceof BrowserAuthPendingError ? { authPending: true } : {}),
};
}
}
Expand Down
25 changes: 25 additions & 0 deletions src/tui/command-surfaces.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1975,6 +1975,31 @@ describe("mcp surface", () => {
});
});

test("a timed-out authorization failure still offers Enter-retry copy", async () => {
await withWiredShell(async (shell, harness) => {
openCommandSurface(shell, "mcp", {
notify: () => undefined,
mcp: {
// Short timeout wording so the two-line describe zone has room
// left for the impact line — a `what` that wraps to both lines
// crowds `impact` out by design (see describeZoneLines).
list: () => [
{
name: "granola",
state: "failed",
error: "timed out waiting for the browser",
},
],
openAuthURL: () => undefined,
},
});
await harness.renderOnce();
const frame = harness.captureCharFrame();
expect(frame).toContain("granola — failed");
expect(frame).toContain("Enter retries");
});
});

test("Alt+R confirms before removing a custom server", async () => {
await withWiredShell(async (shell, harness) => {
const removed: string[] = [];
Expand Down
11 changes: 9 additions & 2 deletions src/tui/product-host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -477,8 +477,15 @@ export async function mountProductHost(
if (disposed) return;
const parsed = mcpServerState(state);
if (parsed === null) return;
if (parsed.state === "needs-auth") mcpUnauthorized.add(parsed.name);
else mcpUnauthorized.delete(parsed.name);
// An auth wait that timed out is still waiting on the operator — keep
// the marker until the server connects, leaves config, or fails for a
// reason that is not the authorization itself.
if (
parsed.state === "needs-auth" ||
(parsed.state === "failed" && parsed.authPending === true)
) {
mcpUnauthorized.add(parsed.name);
} else mcpUnauthorized.delete(parsed.name);
setMcpNeedsAuth(shell, [...mcpUnauthorized]);
show(mcpNotice(parsed));
}
Expand Down
50 changes: 50 additions & 0 deletions src/tui/runtime-channels.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,32 @@ describe("mcp.status channel", () => {
}
});

test("an auth wait that timed out keeps the prompt-box mark and paints no row", async () => {
const { host, emitter, frame, cleanup } = await mountHeadless();
try {
emitter.emit("mcp.status", {
name: "granola",
state: "needs-auth",
url: "https://mcp.test/auth",
});
expect(await frame()).toContain("mcp !");

emitter.emit("mcp.status", {
name: "granola",
state: "failed",
error: "timed out waiting for the browser",
authPending: true,
});
const painted = await frame();
expect(painted).toContain("mcp !");
expect(host.shell.mcpNeedsAuth).toEqual(["granola"]);
expect(host.shell.streamLog).toEqual([]);
expect(host.shell.statusFlash ?? "").not.toContain("did not connect");
} finally {
cleanup();
}
});

test("connected clears the standing auth mark from state and the painted frame", async () => {
const { host, emitter, frame, cleanup } = await mountHeadless();
try {
Expand All @@ -143,6 +169,30 @@ describe("mcp.status channel", () => {
}
});

test("an ordinary failure after needs-auth clears the standing auth mark", async () => {
const { host, emitter, frame, cleanup } = await mountHeadless();
try {
emitter.emit("mcp.status", {
name: "granola",
state: "needs-auth",
url: "https://mcp.test/auth",
});
expect(await frame()).toContain("mcp !");

emitter.emit("mcp.status", {
name: "granola",
state: "failed",
error: "ECONNREFUSED",
});
const painted = await frame();
expect(host.shell.mcpNeedsAuth).toEqual([]);
expect(painted).not.toContain("mcp !");
expect(host.shell.statusFlash).toContain("mcp granola did not connect");
} finally {
cleanup();
}
});

test("a failed connect keeps the landing mountain and rides the notice strip (CL-5600)", async () => {
// Full product-host path: mcp.status → mcpNotice → surfaceSystemNotice.
// The unit landing suite covers surfaceSystemNotice alone; this locks the
Expand Down
19 changes: 19 additions & 0 deletions src/tui/runtime-notices.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,17 @@ describe("mcpNotice", () => {
expect(notice?.text).toContain("its tools are unavailable");
});

test("an unfinished browser authorization stays on the marker, not a row", () => {
expect(
mcpNotice({
name: "linear",
state: "failed",
error: "timed out waiting for the browser",
authPending: true,
}),
).toBeNull();
});

test("disconnected is not news — the operator chose it", () => {
expect(mcpNotice({ name: "linear", state: "disconnected" })).toBeNull();
});
Expand Down Expand Up @@ -163,6 +174,14 @@ describe("payload validation", () => {
"disconnected",
);
expect(mcpServerState({ name: "a", state: "needs-auth" })).toBeNull();
expect(
mcpServerState({
name: "a",
state: "failed",
error: "x",
authPending: true,
}),
).toMatchObject({ state: "failed", authPending: true });
expect(mcpServerState("nope")).toBeNull();
});

Expand Down
10 changes: 9 additions & 1 deletion src/tui/runtime-notices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ export function mcpNotice(state: MCPServerState): RuntimeNotice | null {
case "disconnected":
return null;
case "failed":
// An unfinished browser authorization is the same standing condition
// as needs-auth — the prompt-box marker and /mcp own it, not a row.
if (state.authPending === true) return null;
return {
kind: "row",
text: `mcp ${state.name} did not connect (${state.error}) — its tools are unavailable; /mcp for detail`,
Expand Down Expand Up @@ -164,7 +167,12 @@ export function lifecycleHookEvent(raw: unknown): LifecycleHookEvent | null {
const mcpState = type({ name: "string", state: "'connecting'" })
.or({ name: "string", state: "'needs-auth'", url: "string" })
.or({ name: "string", state: "'connected'", tools: "string[]" })
.or({ name: "string", state: "'failed'", error: "string" })
.or({
name: "string",
state: "'failed'",
error: "string",
"authPending?": "boolean",
})
.or({ name: "string", state: "'disconnected'" });

export function mcpServerState(raw: unknown): MCPServerState | null {
Expand Down
Loading