Skip to content

Commit 5963041

Browse files
committed
Preserve MCP authorization marker after timeout
1 parent a61cd76 commit 5963041

8 files changed

Lines changed: 154 additions & 10 deletions

src/agent/tools-mcp-disconnect.test.ts

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ const closedGenerations: number[] = [];
1717
let connectGeneration = 0;
1818
let connectOptions: MCPConnectOptions[] = [];
1919
let releaseDeferredConnect: (() => void) | undefined;
20-
let connectMode: "success" | "deferred" = "success";
20+
let connectMode: "success" | "deferred" | "auth-pending" = "success";
2121
// Reconnect tests repoint this to simulate a server whose tool set drifted
2222
// between generations; the default matches the original static payload.
2323
let connectedTools: MCPTool[] = [
@@ -56,6 +56,14 @@ await withMockedModule(
5656
};
5757
}
5858
}
59+
if (connectMode === "auth-pending") {
60+
return {
61+
ok: false as const,
62+
serverName: config.name,
63+
error: "timed out waiting for the browser",
64+
authPending: true,
65+
};
66+
}
5967
return {
6068
ok: true as const,
6169
client: {
@@ -440,4 +448,25 @@ describe("setMcpServersSource", () => {
440448
await toolset.dispose();
441449
}
442450
});
451+
452+
test("an auth-pending connect result reaches onStatus marked as such", async () => {
453+
const toolset = await makeToolset();
454+
const states: MCPServerState[] = [];
455+
connectMode = "auth-pending";
456+
try {
457+
await toolset.connectMCPServer(acme, callbacks(states));
458+
const failed = states.filter((s) => s.state === "failed");
459+
expect(failed).toEqual([
460+
{
461+
name: "acme",
462+
state: "failed",
463+
error: "timed out waiting for the browser",
464+
authPending: true,
465+
},
466+
]);
467+
expect(toolset.hasMCPServer("acme")).toBe(false);
468+
} finally {
469+
await toolset.dispose();
470+
}
471+
});
443472
});

src/agent/tools.ts

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import {
2929
} from "../plugins/result-truncation-plugin.js";
3030
import type { CompactionArchive } from "../session/compaction-archive.js";
3131
import {
32+
BrowserAuthPendingError,
3233
connectMCPServer as connectMCPClient,
3334
type MCPClient,
3435
type MCPConnectResult,
@@ -267,7 +268,13 @@ export type MCPServerState =
267268
| { name: string; state: "connecting" }
268269
| { name: string; state: "needs-auth"; url: string }
269270
| { name: string; state: "connected"; tools: string[] }
270-
| { name: string; state: "failed"; error: string }
271+
| {
272+
name: string;
273+
state: "failed";
274+
error: string;
275+
/** Browser auth was offered but never finished — the auth marker owns it. */
276+
authPending?: boolean;
277+
}
271278
| { name: string; state: "disconnected" };
272279

273280
export interface MCPConnectCallbacks {
@@ -956,7 +963,14 @@ export async function createAgentToolset(
956963
});
957964
}
958965
if (!disposed)
959-
callbacks.onStatus({ name: config.name, state: "failed", error });
966+
callbacks.onStatus({
967+
name: config.name,
968+
state: "failed",
969+
error,
970+
...(err instanceof BrowserAuthPendingError
971+
? { authPending: true }
972+
: {}),
973+
});
960974
return;
961975
}
962976
if (disposed) {
@@ -981,6 +995,7 @@ export async function createAgentToolset(
981995
name: config.name,
982996
state: "failed",
983997
error: result.error,
998+
...(result.authPending === true ? { authPending: true } : {}),
984999
});
9851000
return;
9861001
}

src/mcp/client-auth-reauth-cap.test.ts

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,14 +266,21 @@ const config = {
266266
async function connectWithAuthPrompt(): Promise<{
267267
ok: boolean;
268268
error?: string;
269+
authPending?: boolean;
269270
}> {
270271
const result = await connectMCPServer(config, {
271272
onAuthURL: () => {
272273
authURLCount += 1;
273274
authEvents.push("authURL");
274275
},
275276
});
276-
return result.ok ? { ok: true } : { ok: false, error: result.error };
277+
return result.ok
278+
? { ok: true }
279+
: {
280+
ok: false,
281+
error: result.error,
282+
...(result.authPending === true ? { authPending: true } : {}),
283+
};
277284
}
278285

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

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

10101021
expect(result.ok).toBe(false);
1022+
expect(result.authPending).toBe(true);
10111023
expect(result.error).toContain("timed out waiting for the browser");
10121024
expect(result.error).toContain("disconnected");
10131025
expect(authURLCount).toBe(1);
10141026
expect(waitForCodeCalls).toBe(1);
10151027

10161028
const capped = await connectWithAuthPrompt();
10171029
expect(capped.ok).toBe(false);
1030+
expect(capped.authPending).toBe(true);
10181031
expect(capped.error).toContain("retrying paused");
10191032
expect(authURLCount).toBe(1);
10201033
});
1034+
1035+
test("a failure that is not the authorization itself is not auth-pending", async () => {
1036+
connectFailuresLeft = Number.POSITIVE_INFINITY;
1037+
1038+
const result = await connectWithAuthPrompt();
1039+
1040+
expect(result.ok).toBe(false);
1041+
expect(result.error).toContain("finishAuth exploded");
1042+
expect(result.authPending).toBeUndefined();
1043+
});
10211044
});

src/mcp/client.ts

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,16 @@ export interface MCPClient {
4646

4747
export type MCPConnectResult =
4848
| { ok: true; client: MCPClient }
49-
| { ok: false; serverName: string; error: string };
49+
| {
50+
ok: false;
51+
serverName: string;
52+
error: string;
53+
/**
54+
* The failure is a browser authorization that was offered but never
55+
* finished — a standing operator action, not a dead server.
56+
*/
57+
authPending?: boolean;
58+
};
5059
export interface MCPConnectOptions {
5160
stderr?: "inherit" | "ignore" | "pipe";
5261
onAuthURL?: (serverName: string, authorizationUrl: string) => void;
@@ -165,20 +174,27 @@ export function setBrowserAuthWaitMs(ms: number): void {
165174
browserAuthWaitMs = ms;
166175
}
167176

177+
/**
178+
* Browser authorization was offered but never finished — the wait timed out
179+
* or hit the attempt cap. The TUI keeps the prompt-box auth marker for these
180+
* instead of painting a generic connect-failure row.
181+
*/
182+
export class BrowserAuthPendingError extends Error {}
183+
168184
function browserAuthCapError(serverName: string): Error {
169185
const minutes = Math.round(BROWSER_AUTH_COOLDOWN_MS / 60_000);
170186
const attempts =
171187
MAX_BROWSER_AUTH_ATTEMPTS === 1
172188
? "1 attempt"
173189
: `${String(MAX_BROWSER_AUTH_ATTEMPTS)} attempts`;
174-
return new Error(
190+
return new BrowserAuthPendingError(
175191
`MCP authorization for ${serverName} failed after ${attempts}; ` +
176192
`retrying paused for ${minutes} minutes. Retry later after the cooldown.`,
177193
);
178194
}
179195

180196
function browserAuthWaitError(serverName: string): Error {
181-
return new Error(
197+
return new BrowserAuthPendingError(
182198
`MCP authorization for ${serverName} timed out waiting for the browser; ` +
183199
`the server is disconnected. Retry later after the cooldown.`,
184200
);
@@ -735,6 +751,7 @@ async function connectHttp(
735751
ok: false,
736752
serverName: config.name,
737753
error: err instanceof Error ? err.message : String(err),
754+
...(err instanceof BrowserAuthPendingError ? { authPending: true } : {}),
738755
};
739756
}
740757
}

src/tui/product-host.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -477,8 +477,15 @@ export async function mountProductHost(
477477
if (disposed) return;
478478
const parsed = mcpServerState(state);
479479
if (parsed === null) return;
480-
if (parsed.state === "needs-auth") mcpUnauthorized.add(parsed.name);
481-
else mcpUnauthorized.delete(parsed.name);
480+
// An auth wait that timed out is still waiting on the operator — keep
481+
// the marker until the server connects, leaves config, or fails for a
482+
// reason that is not the authorization itself.
483+
if (
484+
parsed.state === "needs-auth" ||
485+
(parsed.state === "failed" && parsed.authPending === true)
486+
) {
487+
mcpUnauthorized.add(parsed.name);
488+
} else mcpUnauthorized.delete(parsed.name);
482489
setMcpNeedsAuth(shell, [...mcpUnauthorized]);
483490
show(mcpNotice(parsed));
484491
}

src/tui/runtime-channels.test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,32 @@ describe("mcp.status channel", () => {
121121
}
122122
});
123123

124+
test("an auth wait that timed out keeps the prompt-box mark and paints no row", async () => {
125+
const { host, emitter, frame, cleanup } = await mountHeadless();
126+
try {
127+
emitter.emit("mcp.status", {
128+
name: "granola",
129+
state: "needs-auth",
130+
url: "https://mcp.test/auth",
131+
});
132+
expect(await frame()).toContain("mcp !");
133+
134+
emitter.emit("mcp.status", {
135+
name: "granola",
136+
state: "failed",
137+
error: "timed out waiting for the browser",
138+
authPending: true,
139+
});
140+
const painted = await frame();
141+
expect(painted).toContain("mcp !");
142+
expect(host.shell.mcpNeedsAuth).toEqual(["granola"]);
143+
expect(host.shell.streamLog).toEqual([]);
144+
expect(host.shell.statusFlash ?? "").not.toContain("did not connect");
145+
} finally {
146+
cleanup();
147+
}
148+
});
149+
124150
test("connected clears the standing auth mark from state and the painted frame", async () => {
125151
const { host, emitter, frame, cleanup } = await mountHeadless();
126152
try {

src/tui/runtime-notices.test.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,17 @@ describe("mcpNotice", () => {
111111
expect(notice?.text).toContain("its tools are unavailable");
112112
});
113113

114+
test("an unfinished browser authorization stays on the marker, not a row", () => {
115+
expect(
116+
mcpNotice({
117+
name: "linear",
118+
state: "failed",
119+
error: "timed out waiting for the browser",
120+
authPending: true,
121+
}),
122+
).toBeNull();
123+
});
124+
114125
test("disconnected is not news — the operator chose it", () => {
115126
expect(mcpNotice({ name: "linear", state: "disconnected" })).toBeNull();
116127
});
@@ -163,6 +174,14 @@ describe("payload validation", () => {
163174
"disconnected",
164175
);
165176
expect(mcpServerState({ name: "a", state: "needs-auth" })).toBeNull();
177+
expect(
178+
mcpServerState({
179+
name: "a",
180+
state: "failed",
181+
error: "x",
182+
authPending: true,
183+
}),
184+
).toMatchObject({ state: "failed", authPending: true });
166185
expect(mcpServerState("nope")).toBeNull();
167186
});
168187

src/tui/runtime-notices.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,9 @@ export function mcpNotice(state: MCPServerState): RuntimeNotice | null {
9999
case "disconnected":
100100
return null;
101101
case "failed":
102+
// An unfinished browser authorization is the same standing condition
103+
// as needs-auth — the prompt-box marker and /mcp own it, not a row.
104+
if (state.authPending === true) return null;
102105
return {
103106
kind: "row",
104107
text: `mcp ${state.name} did not connect (${state.error}) — its tools are unavailable; /mcp for detail`,
@@ -164,7 +167,12 @@ export function lifecycleHookEvent(raw: unknown): LifecycleHookEvent | null {
164167
const mcpState = type({ name: "string", state: "'connecting'" })
165168
.or({ name: "string", state: "'needs-auth'", url: "string" })
166169
.or({ name: "string", state: "'connected'", tools: "string[]" })
167-
.or({ name: "string", state: "'failed'", error: "string" })
170+
.or({
171+
name: "string",
172+
state: "'failed'",
173+
error: "string",
174+
"authPending?": "boolean",
175+
})
168176
.or({ name: "string", state: "'disconnected'" });
169177

170178
export function mcpServerState(raw: unknown): MCPServerState | null {

0 commit comments

Comments
 (0)