Skip to content

Commit d433576

Browse files
committed
Retry failed MCP servers from the MCP list
The list only stores live status. After a persist-then-connect failure the settings row already exists, so Enter reconnects that config instead of adding a duplicate name.
1 parent ae995cb commit d433576

3 files changed

Lines changed: 206 additions & 34 deletions

File tree

src/tui/command-surfaces.test.ts

Lines changed: 126 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -782,37 +782,50 @@ describe("mcp surface", () => {
782782
test("Enter on an unauthorized server opens the browser and copies the link", async () => {
783783
await withShell((shell) => {
784784
const opened: string[] = [];
785+
const retried: string[] = [];
785786
openCommandSurface(shell, "mcp", {
786787
notify: () => {},
787-
mcp: { list: () => entries, openAuthURL: (url) => opened.push(url) },
788+
mcp: {
789+
list: () => entries,
790+
openAuthURL: (url) => opened.push(url),
791+
retryServer: async (name) => {
792+
retried.push(name);
793+
return { ok: true, message: "should not retry" };
794+
},
795+
},
788796
});
789797
moveOverlaySelection(shell, 1);
790798
acceptOverlaySelection(shell);
791799
expect(opened).toEqual(["https://notion.test/auth"]);
800+
expect(retried).toEqual([]);
792801
expect(shell.statusFlash).toContain("notion");
793802
// The echo would quote "notion — needs auth" back forever, moments
794803
// after the operator authorized it.
795804
expect(shell.streamLog.filter((r) => r.meta === "overlay")).toEqual([]);
796805
});
797806
});
798807

799-
test("Enter on non-auth rows releases the status subscription", async () => {
800-
const nonAuthEntries: readonly McpEntry[] = [
808+
test("Enter on connecting and connected rows releases the status subscription", async () => {
809+
const nonActionEntries: readonly McpEntry[] = [
801810
{ name: "connected", state: "connected", toolCount: 1 },
802811
{ name: "connecting", state: "connecting" },
803-
{ name: "failed", state: "failed", error: "offline" },
804812
];
805813

806-
for (const entry of nonAuthEntries) {
814+
for (const entry of nonActionEntries) {
807815
await withShell((shell) => {
808816
const listeners = new Set<() => void>();
809817
let unsubscribeCalls = 0;
810818
const opened: string[] = [];
819+
const retried: string[] = [];
811820
openCommandSurface(shell, "mcp", {
812821
notify: () => {},
813822
mcp: {
814823
list: () => [entry],
815824
openAuthURL: (url) => opened.push(url),
825+
retryServer: async (name) => {
826+
retried.push(name);
827+
return { ok: true, message: "should not retry" };
828+
},
816829
subscribe: (listener) => {
817830
listeners.add(listener);
818831
return () => {
@@ -826,12 +839,120 @@ describe("mcp surface", () => {
826839
expect(listeners.size).toBe(1);
827840
acceptOverlaySelection(shell);
828841
expect(opened).toEqual([]);
842+
expect(retried).toEqual([]);
829843
expect(unsubscribeCalls).toBe(1);
830844
expect(listeners.size).toBe(0);
831845
});
832846
}
833847
});
834848

849+
test("Enter on a failed server retries connect once without adding again", async () => {
850+
await withShell(async (shell) => {
851+
const added: { name: string; url: string }[] = [];
852+
const retried: string[] = [];
853+
const opened: string[] = [];
854+
const notes: string[] = [];
855+
let liveEntries: readonly McpEntry[] = [
856+
{ name: "sentry", state: "failed", error: "ECONNREFUSED" },
857+
];
858+
openCommandSurface(shell, "mcp", {
859+
notify: (note) => notes.push(note),
860+
mcp: {
861+
list: () => liveEntries,
862+
openAuthURL: (url) => opened.push(url),
863+
addServer: async (name, url) => {
864+
added.push({ name, url });
865+
return { ok: true, message: "should not add" };
866+
},
867+
retryServer: async (name) => {
868+
retried.push(name);
869+
liveEntries = [{ name, state: "connecting" }];
870+
return { ok: true, message: `Retrying ${name}; connecting now.` };
871+
},
872+
},
873+
});
874+
acceptOverlaySelection(shell);
875+
await Promise.resolve();
876+
await Promise.resolve();
877+
878+
expect(retried).toEqual(["sentry"]);
879+
expect(added).toEqual([]);
880+
expect(opened).toEqual([]);
881+
expect(notes).toEqual(["Retrying sentry; connecting now."]);
882+
expect(shell.overlayItems[0]).toBe("sentry — connecting");
883+
});
884+
});
885+
886+
test("Enter on a failed row without retry still releases the status subscription", async () => {
887+
await withShell((shell) => {
888+
const listeners = new Set<() => void>();
889+
let unsubscribeCalls = 0;
890+
const added: { name: string; url: string }[] = [];
891+
openCommandSurface(shell, "mcp", {
892+
notify: () => {},
893+
mcp: {
894+
list: () => [{ name: "sentry", state: "failed", error: "offline" }],
895+
openAuthURL: () => {},
896+
addServer: async (name, url) => {
897+
added.push({ name, url });
898+
return { ok: true, message: "should not add" };
899+
},
900+
subscribe: (listener) => {
901+
listeners.add(listener);
902+
return () => {
903+
unsubscribeCalls += 1;
904+
listeners.delete(listener);
905+
};
906+
},
907+
},
908+
});
909+
910+
expect(listeners.size).toBe(1);
911+
acceptOverlaySelection(shell);
912+
expect(added).toEqual([]);
913+
expect(unsubscribeCalls).toBe(1);
914+
expect(listeners.size).toBe(0);
915+
});
916+
});
917+
918+
test("Alt+A of a failed name still persists through addServer", async () => {
919+
await withShell(async (shell) => {
920+
const added: { name: string; url: string }[] = [];
921+
const retried: string[] = [];
922+
const notes: string[] = [];
923+
openCommandSurface(shell, "mcp", {
924+
notify: (note) => notes.push(note),
925+
mcp: {
926+
list: () => [{ name: "sentry", state: "failed" as const, error: "offline" }],
927+
openAuthURL: () => {},
928+
addServer: async (name, url) => {
929+
added.push({ name, url });
930+
return {
931+
ok: false,
932+
message: `An MCP server named "${name}" already exists or is connecting.`,
933+
};
934+
},
935+
retryServer: async (name) => {
936+
retried.push(name);
937+
return { ok: true, message: "should not retry" };
938+
},
939+
},
940+
});
941+
942+
expect(runOverlayAction(shell, altKey("a"))).toBe(true);
943+
for (const ch of "sentry") runOverlayAction(shell, charKey(ch));
944+
acceptOverlaySelection(shell);
945+
for (const ch of "https://sentry.test/mcp") runOverlayAction(shell, charKey(ch));
946+
acceptOverlaySelection(shell);
947+
await Promise.resolve();
948+
await Promise.resolve();
949+
950+
expect(added).toEqual([{ name: "sentry", url: "https://sentry.test/mcp" }]);
951+
expect(retried).toEqual([]);
952+
expect(notes.at(-1)).toContain("already exists");
953+
});
954+
});
955+
835956
test("Alt+A collects a name and absolute HTTP URL before adding", async () => {
836957
await withShell(async (shell) => {
837958
const added: { name: string; url: string }[] = [];

src/tui/command-surfaces.ts

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,8 @@ export interface McpSurfaceDeps {
145145
readonly openAuthURL: (url: string) => void;
146146
readonly subscribe?: (listener: () => void) => () => void;
147147
readonly addServer?: (name: string, url: string) => Promise<PluginActionResult>;
148+
/** Reconnect a failed persisted server without writing a second settings row. */
149+
readonly retryServer?: (name: string) => Promise<PluginActionResult>;
148150
readonly mcpServersSource?: "local" | "global" | "none";
149151
}
150152

@@ -958,14 +960,42 @@ function mcpDescription(entry: McpEntry): ItemDescription {
958960
impact: "Enter opens the authorization page and copies the link.",
959961
};
960962
case "failed":
961-
return { what: entry.error ?? "Did not connect.", tone: "consequence" };
963+
return {
964+
what: entry.error ?? "Did not connect.",
965+
impact: "Enter retries the existing persisted config without adding a second server.",
966+
tone: "consequence",
967+
};
962968
}
963969
}
964970

965971
function canAddMCPServer(mcp: McpSurfaceDeps): boolean {
966972
return mcp.mcpServersSource !== "local";
967973
}
968974

975+
function runMcpSurfaceAction(
976+
shell: AppShell,
977+
deps: CommandSurfaceDeps,
978+
action: Promise<PluginActionResult>,
979+
failPrefix: string,
980+
): void {
981+
const continuation = captureOverlayContinuation(shell);
982+
void action
983+
.then(
984+
(result) => {
985+
if (!isOverlayContinuationCurrent(shell, continuation)) return;
986+
deps.notify(result.message);
987+
if (isOverlayContinuationCurrent(shell, continuation)) openMcpSurface(shell, deps);
988+
},
989+
(err: unknown) => {
990+
if (!isOverlayContinuationCurrent(shell, continuation)) return;
991+
deps.notify(`${failPrefix}: ${errorText(err)}`);
992+
},
993+
)
994+
.catch(() => {
995+
// UI continuation failures must not escape a fire-and-forget command.
996+
});
997+
}
998+
969999
function mcpSurfaceRows(entries: readonly McpEntry[], canAdd: boolean): ResidualCatalogEntry[] {
9701000
const rows: ResidualCatalogEntry[] = entries.map((e) => ({
9711001
id: e.name,
@@ -1000,22 +1030,7 @@ function openAddMcpURLPane(
10001030
deps.notify("Adding MCP servers is not available in this session.");
10011031
return;
10021032
}
1003-
const continuation = captureOverlayContinuation(shell);
1004-
void addServer(name, url)
1005-
.then(
1006-
(result) => {
1007-
if (!isOverlayContinuationCurrent(shell, continuation)) return;
1008-
deps.notify(result.message);
1009-
if (isOverlayContinuationCurrent(shell, continuation)) openMcpSurface(shell, deps);
1010-
},
1011-
(err: unknown) => {
1012-
if (!isOverlayContinuationCurrent(shell, continuation)) return;
1013-
deps.notify(`Add failed: ${errorText(err)}`);
1014-
},
1015-
)
1016-
.catch(() => {
1017-
// UI continuation failures must not escape a fire-and-forget command.
1018-
});
1033+
runMcpSurfaceAction(shell, deps, addServer(name, url), "Add failed");
10191034
},
10201035
},
10211036
buffer,
@@ -1047,7 +1062,7 @@ function openAddMcpNamePane(
10471062
);
10481063
}
10491064

1050-
/** Configured MCP servers and their live state; Enter authorizes an unauthorized one. */
1065+
/** Configured MCP servers and their live state; Enter authorizes or retries. */
10511066
export function openMcpSurface(
10521067
shell: AppShell,
10531068
deps: CommandSurfaceDeps,
@@ -1090,8 +1105,15 @@ export function openMcpSurface(
10901105
return;
10911106
}
10921107
const target = byName.get(id);
1093-
const url = target?.authURL;
1094-
if (target === undefined || target.state !== "needs-auth" || url === undefined) return;
1108+
if (target === undefined) return;
1109+
if (target.state === "failed") {
1110+
const retryServer = mcp.retryServer;
1111+
if (retryServer === undefined) return;
1112+
runMcpSurfaceAction(shell, deps, retryServer(target.name), "Retry failed");
1113+
return;
1114+
}
1115+
const url = target.authURL;
1116+
if (target.state !== "needs-auth" || url === undefined) return;
10951117
mcp.openAuthURL(url);
10961118
// The copy is the fallback that makes this work over SSH, where the
10971119
// browser that must receive the redirect is not on this machine.

src/tui/runner.ts

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ import {
4343
type Settings,
4444
type LocalSettings,
4545
type PluginConfig,
46+
type MCPServerConfig,
4647
} from "../config/settings.js";
4748
import { addProviderSelectorChoices, providerChoices } from "./provider-setup.js";
4849
import { persistConnectedSelection } from "./provider-setup-submit.js";
@@ -2233,6 +2234,23 @@ export async function runTUI(initialConfig: Config): Promise<number> {
22332234
directorHolder.instance?.updateToolDefinitions(computeAdvertised(definitions)),
22342235
};
22352236

2237+
const connectLateMCPServer = (server: MCPServerConfig): void => {
2238+
void toolset
2239+
.connectMCPServer(server, mcpConnectCallbacks, mcpConnectController.signal)
2240+
.catch((err: unknown) => {
2241+
if (err instanceof Error && err.name === "AbortError") return;
2242+
tuiLogger.error("Late MCP connect failed: {error}", {
2243+
error: err instanceof Error ? err.message : String(err),
2244+
});
2245+
});
2246+
};
2247+
2248+
const persistedMCPServer = (name: string): MCPServerConfig | undefined =>
2249+
(config.mcpServers ?? []).find((server) => server.name === name) ??
2250+
(config.settings?.mcpServers ?? []).find(
2251+
(server): server is MCPServerConfig => server.name === name && !("enabled" in server),
2252+
);
2253+
22362254
const host = await mountRunnerHost({
22372255
// An unnamed session shows nothing rather than a placeholder.
22382256
title: runTaskTitle,
@@ -2560,17 +2578,28 @@ export async function runTUI(initialConfig: Config): Promise<number> {
25602578
: "Enter an absolute HTTP(S) URL first.";
25612579
return { ok: false, message };
25622580
}
2563-
config = { ...config, settings: result.settings };
2564-
void toolset
2565-
.connectMCPServer(result.server, mcpConnectCallbacks, mcpConnectController.signal)
2566-
.catch((err: unknown) => {
2567-
if (err instanceof Error && err.name === "AbortError") return;
2568-
tuiLogger.error("Late MCP connect failed: {error}", {
2569-
error: err instanceof Error ? err.message : String(err),
2570-
});
2571-
});
2581+
config = {
2582+
...config,
2583+
settings: result.settings,
2584+
mcpServers: [
2585+
...(config.mcpServers ?? []).filter((server) => server.name !== result.server.name),
2586+
result.server,
2587+
],
2588+
};
2589+
connectLateMCPServer(result.server);
25722590
return { ok: true, message: `Added ${result.server.name}; connecting now.` };
25732591
},
2592+
retryServer: async (name) => {
2593+
const server = persistedMCPServer(name);
2594+
if (server === undefined) {
2595+
return {
2596+
ok: false,
2597+
message: `No persisted MCP server named "${name}" to retry.`,
2598+
};
2599+
}
2600+
connectLateMCPServer(server);
2601+
return { ok: true, message: `Retrying ${server.name}; connecting now.` };
2602+
},
25742603
},
25752604
hooks: {
25762605
list: () =>

0 commit comments

Comments
 (0)