Merge pull request #2039 from modelcontextprotocol/v2/chore/1959-no-floating-promises

chore(lint): enable @typescript-eslint/no-floating-promises in all five ESLint scopes

clients/tui/__tests__/App.test.tsx

@@ -706,6 +706,91 @@ describe("App (foundation)", () => {
     expect(h.disconnect).toHaveBeenCalled();
   });
 
+  it("surfaces a disconnect failure instead of floating the rejection", async () => {
+    // 'd' is a key handler, so it cannot await handleDisconnect — the handler
+    // has to own the failure itself or it escapes as an unhandled rejection
+    // and fails the whole run from somewhere else (#1959).
+    // The banner is deliberately independent of connection status: a rejected
+    // disconnect leaves the status "connected", so anything gated on
+    // `status === "error"` would never be seen.
+    h.ctrl.status = "connected";
+    h.disconnect.mockRejectedValue(new Error("discfail"));
+    const r = await mount(oneStdio());
+    r.stdin.write("d");
+    await expectFrame(r, "Disconnect failed: discfail");
+  });
+
+  it("owns a non-Error disconnect rejection too", async () => {
+    // The catch stringifies a non-Error rejection rather than reading
+    // `.message` off it; a throw here would escape the same way.
+    h.ctrl.status = "connected";
+    h.disconnect.mockRejectedValue("plainstring");
+    const r = await mount(oneStdio());
+    r.stdin.write("d");
+    await expectFrame(r, "Disconnect failed: plainstring");
+  });
+
+  it("clears a disconnect failure once a retry succeeds", async () => {
+    // A stale banner would keep reporting a failure the user has since fixed.
+    h.ctrl.status = "connected";
+    h.disconnect.mockRejectedValueOnce(new Error("discfail"));
+    const r = await mount(oneStdio());
+    r.stdin.write("d");
+    await expectFrame(r, "Disconnect failed: discfail");
+    r.stdin.write("d");
+    await waitUntil(() => !(r.lastFrame() ?? "").includes("Disconnect failed"));
+    expect(r.lastFrame() ?? "").not.toContain("Disconnect failed");
+  });
+
+  it("drops a disconnect rejection that lands after the user switched servers", async () => {
+    // The stale attempt is the one that usually rejects, so without an
+    // attempt token server alpha's failure would surface in beta's header.
+    h.ctrl.status = "connected";
+    let rejectDisconnect: (err: Error) => void = () => {};
+    h.disconnect.mockImplementationOnce(
+      () =>
+        new Promise<void>((_resolve, reject) => {
+          rejectDisconnect = reject;
+        }),
+    );
+    const r = await mount(stdioServer());
+    r.stdin.write("d"); // alpha's disconnect starts, and hangs
+    await tick();
+    await press(r, [DOWN]); // switch to beta
+    await expectFrame(r, "beta");
+    rejectDisconnect(new Error("stale-alpha-failure"));
+    // Give the rejection every chance to be published before asserting it
+    // wasn't — a bare tick would pass even with the guard removed.
+    await waitUntil(() =>
+      (r.lastFrame() ?? "").includes("stale-alpha-failure"),
+    );
+    expect(r.lastFrame() ?? "").not.toContain("stale-alpha-failure");
+  });
+
+  it("drops a stale disconnect rejection across an A → B → A round trip", async () => {
+    // The server-name check alone passes here: by the time the rejection
+    // lands, alpha is selected again. Only retiring the attempt token on
+    // every switch catches it.
+    h.ctrl.status = "connected";
+    let rejectDisconnect: (err: Error) => void = () => {};
+    h.disconnect.mockImplementationOnce(
+      () =>
+        new Promise<void>((_resolve, reject) => {
+          rejectDisconnect = reject;
+        }),
+    );
+    const r = await mount(stdioServer());
+    r.stdin.write("d"); // alpha's disconnect starts, and hangs
+    await tick();
+    await press(r, [DOWN]); // alpha -> beta
+    await expectFrame(r, "b.js");
+    await press(r, [UP]); // beta -> alpha again
+    await expectFrame(r, "s.js");
+    rejectDisconnect(new Error("round-trip-failure"));
+    await waitUntil(() => (r.lastFrame() ?? "").includes("round-trip-failure"));
+    expect(r.lastFrame() ?? "").not.toContain("round-trip-failure");
+  });
+
   it("switches tabs via accelerator keys", async () => {
     const r = await mount(stdioServer());
     await press(r, ["t"]); // tools tab (server is auto-selected)

clients/tui/__tests__/tsupConfig.test.ts

@@ -207,7 +207,9 @@ describe("tui tsup config", () => {
     // the double cast is bridging a real structural gap, not hiding a mismatch.
     // A hook the patch called but the stub lacks fails as undefined here rather
     // than passing silently.
-    inkFormLabelPatch.setup(
+    // esbuild types `setup` as `void | Promise<void>`; await it so the
+    // assertions below can't run before an async setup has registered.
+    await inkFormLabelPatch.setup(
       build as unknown as Parameters<typeof inkFormLabelPatch.setup>[0],
     );
 

clients/tui/eslint.config.js

@@ -30,4 +30,19 @@ export default defineConfig([
       "react-hooks/exhaustive-deps": "warn",
     },
   },
+  {
+    // Type-aware pass for `no-floating-promises` (#1959) — see the CLI config
+    // for the reasoning. Both tsconfig projects are listed because `src` and
+    // `__tests__` live in different ones.
+    files: ["**/*.{ts,tsx}"],
+    languageOptions: {
+      parserOptions: {
+        project: ["./tsconfig.json", "./tsconfig.test.json"],
+        tsconfigRootDir: import.meta.dirname,
+      },
+    },
+    rules: {
+      "@typescript-eslint/no-floating-promises": "error",
+    },
+  },
 ]);

clients/tui/src/App.tsx

@@ -170,6 +170,17 @@ function App({
     pendingStepUpRef.current = pendingStepUp;
   }, [pendingStepUp]);
   const [connectError, setConnectError] = useState<string | null>(null);
+  // Monotonic token for in-flight disconnects — see handleDisconnect.
+  const disconnectAttemptRef = useRef(0);
+  // A failed disconnect is deliberately NOT folded into `connectError`. That
+  // one is only rendered by InfoTab when the status is "error", which a
+  // rejected disconnect leaves untouched (the status stays "connected"), so
+  // the message would never be seen — and it feeds
+  // `connectError ?? inspectorLastError`, where a stale value would go on
+  // masking a later, real connection error. This renders in the header
+  // regardless of status and is cleared on the next connect/disconnect
+  // attempt and on a server switch.
+  const [disconnectError, setDisconnectError] = useState<string | null>(null);
   const oauthInProgressRef = useRef(false);
   const callbackServerRef = useRef<OAuthCallbackServer | null>(null);
   const selectedServerRef = useRef<string | null>(null);
@@ -434,6 +445,13 @@ function App({
   useEffect(() => {
     setOauthStatus("idle");
     setOauthMessage(null);
+    // The header banner is server-scoped, so a failure from the server we just
+    // left must not be read as this one's. Clearing is not enough: a switch
+    // away and back (A → B → A) would leave both the attempt token and the
+    // server name matching again, so the stale rejection would land on the
+    // re-selected A. Retiring the token on every switch is what closes that.
+    disconnectAttemptRef.current++;
+    setDisconnectError(null);
     const stepUp = pendingStepUpRef.current;
     if (stepUp && selectedServer && stepUp.serverName !== selectedServer) {
       setPendingStepUp(null);
@@ -757,6 +775,10 @@ function App({
     if (!selectedServer || !selectedInspectorClient || !selectedServerConfig) {
       return;
     }
+    // A connect attempt supersedes whatever the last disconnect reported —
+    // including one still in flight, which is what the counter bump retires.
+    disconnectAttemptRef.current++;
+    setDisconnectError(null);
 
     const finishConnect = async () => {
       await connectInspector();
@@ -887,8 +909,31 @@ function App({
   // Disconnect handler
   const handleDisconnect = useCallback(async () => {
     if (!selectedServer) return;
-    await disconnectInspector();
-    // InspectorClient will update status automatically, and data is preserved
+    // Claim this attempt before awaiting. A disconnect can still be in flight
+    // when the user switches servers or presses 'd' again, and it is the
+    // *stale* one that usually rejects — so the catch below must be able to
+    // tell "my failure" from "one that has since been superseded", or server
+    // A's error lands in server B's header. `handleConnect` bumps the same
+    // counter for the same reason.
+    const attempt = ++disconnectAttemptRef.current;
+    const attemptServer = selectedServer;
+    // Clear first, so a retry that succeeds leaves no stale message behind.
+    setDisconnectError(null);
+    try {
+      await disconnectInspector();
+      // InspectorClient will update status automatically, and data is preserved
+    } catch (err) {
+      // Nothing above this catches: the only caller is the key handler, which
+      // cannot await, so without this the rejection escapes unhandled. A
+      // superseded one is still owned here — just not published.
+      if (
+        disconnectAttemptRef.current !== attempt ||
+        selectedServerRef.current !== attemptServer
+      ) {
+        return;
+      }
+      setDisconnectError(err instanceof Error ? err.message : String(err));
+    }
   }, [selectedServer, disconnectInspector]);
 
   const handleClearOAuth = useCallback(async () => {
@@ -1428,12 +1473,15 @@ function App({
         input.toLowerCase() === "c" &&
         (inspectorStatus === "disconnected" || inspectorStatus === "error")
       ) {
-        handleConnect();
+        // Both handlers own their failures internally (each ends in a catch
+        // that surfaces the message), so there is nothing left for a key
+        // handler — which cannot await — to do with the promise.
+        void handleConnect();
       } else if (
         input.toLowerCase() === "d" &&
         (inspectorStatus === "connected" || inspectorStatus === "connecting")
       ) {
-        handleDisconnect();
+        void handleDisconnect();
       }
     }
   });
@@ -1617,6 +1665,11 @@ function App({
                   <Text color="red">OAuth: {oauthMessage}</Text>
                 </Box>
               )}
+              {disconnectError && (
+                <Box marginTop={1}>
+                  <Text color="red">Disconnect failed: {disconnectError}</Text>
+                </Box>
+              )}
             </Box>
           </Box>
 

clients/tui/src/components/PromptsTab.tsx

@@ -53,8 +53,10 @@ export function PromptsTab({
         if (selectedPrompt.arguments && selectedPrompt.arguments.length > 0) {
           onFetchPrompt(selectedPrompt);
         } else {
-          // No arguments, fetch directly
-          (async () => {
+          // No arguments, fetch directly. The IIFE catches everything it can
+          // throw, so there is no rejection for this key handler — which
+          // cannot await — to own.
+          void (async () => {
             try {
               const invocation = await inspectorClient.getPrompt(
                 selectedPrompt.name,

clients/tui/src/components/ResourcesTab.tsx

@@ -186,7 +186,9 @@ export function ResourcesTab({
       }
     };
 
-    fetchContent();
+    // An effect body is synchronous, and fetchContent already catches
+    // everything it can throw, so there is no rejection left to own.
+    void fetchContent();
   }, [shouldFetchResource, inspectorClient, onAuthRecoveryRequired]);
 
   const listWidth = Math.floor(width * 0.4);