fix: address round-2 review — stale disconnect, lint-project gap, coverage

Copilot round 2 (PR #2039) surfaced no new visible comments but six
suppressed ones. Four were actionable.

`clients/tui/src/App.tsx` — a stale-completion race in the new banner:
start disconnecting server A, switch to B, then let A's disconnect
reject, and A's failure appears under B. Rapid same-server retries had
the same shape, and the stale attempt is the one most likely to reject.
Claim a monotonic attempt token before awaiting and publish only if it
is still current and the server has not changed; `handleConnect` bumps
the same counter so a connect retires an in-flight disconnect. The
rejection is still owned when superseded — just not published. The
regression test drives a deferred rejection across a server switch, and
fails without the guard.

`tsconfig.lint.json` — the root type-aware block matches
`test-servers/src/**/*.tsx` but the parser project omitted that
extension, so such a file would have failed with "not found in any of
the provided project(s)" instead of being linted. Added, and AGENTS.md
now states the invariant: the include must stay a superset of the lint
globs.

`clients/web/server/{server,vite-hono-plugin}.ts` — the `open()`
rejection branch was untested, and duplicated. Both were fixed by
removing the duplication: `openBrowser` now lives beside
`printServerBanner` in `web-server-config.ts`, with one implementation
to test. Two copies of a one-line `.catch` is two chances for one to
drift back to a bare `open(url)`. The dev plugin is not booted in tests
— it returns early under VITEST by design — but it calls the same
tested helper.

`core/mcp/remote/node/server.ts` — `closeAbortedStream`'s rejecting-close
path, the entire reason the helper exists, had no test. Exported it (as
this file already does for `requestIdForSendWait` / `mcpParamHeadersOnly`)
and covered both legs, asserting no unhandledRejection fires.

Also corrects AGENTS.md, which said each client lists "both" tsconfig
projects — true for cli/tui/launcher, but web lists four leaf projects.

Signed-off-by: cliffhall <cliff@futurescale.com>

AGENTS.md

@@ -723,7 +723,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab
   - **The class it catches is invisible at review time.** A floated call reads like an awaited one minus four characters, and the unhandled rejection it produces surfaces somewhere else entirely — a different test, a different file, a stack pointing at SDK internals. Two un-held `client.callTool(...)` promises made `npm run ci` unpassable in #1947: `disconnect()` rejected them with `Connection closed`, the unhandled rejection failed the whole vitest run, and the chain aborted at `coverage`, silently skipping `verify:build-gate`, `smoke`, and Storybook. Attributing it took a full investigation; the fix was two lines.
   - **Prefer holding and settling the promise.** `void` is an escape hatch, not a fix — it is visible at review time (strictly better than nothing) but still discards the rejection. Reach for it only when the callee already owns its failures (it ends in a `catch` that surfaces the message) or the caller genuinely cannot await — a synchronous `useEffect` body, an Ink `useInput` key handler, a Hono `stream.onAbort` listener. Say **which** of those it is in a one-line comment; an unexplained `void` is a review finding. Where the callee does *not* own its failures, give it a `catch` rather than voiding the call (see `handleDisconnect` in `clients/tui/src/App.tsx`), or terminate with `.catch(…)` at the call site (see `open(url)` in `clients/web/server/{server,vite-hono-plugin}.ts`).
   - **In Storybook play functions, `expect(...)` from `storybook/test` returns a promise.** Storybook instruments it so the interactions panel can trace each assertion, so every `expect` in a play function is awaited — as is any shared helper that wraps one (`src/test/scrollAreaStoryAssertions.ts` is `async` for this reason).
-  - **The rule is type-aware, so each scope's ESLint config carries a parser project.** The four clients point at their own tsconfig projects (both of them, since `src` and `__tests__`/`server` live in different ones); the root config points at **`tsconfig.lint.json`**, which exists solely to give the parser a program covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts` — none of which is rooted in a tsconfig of its own. That file emits nothing and gates nothing: type *checking* for those sources stays where it was (`core/` through `clients/web`'s `tsc -b`, `test-servers/src` through `clients/cli`'s test project). It sets `moduleResolution: bundler` deliberately — `core/` uses extensionless relative imports, which NodeNext fails to resolve, degrading every import to `any` so the rule silently stops seeing promises at all. **When adding a first-party TS location to the root lint scope, add it to `tsconfig.lint.json`'s `include` too**, or the parser fails it outright with "was not found in any of the provided project(s)". Note also that a tsconfig `include` does **not** expand braces — `core/**/*.{ts,tsx}` matches nothing; list the extensions separately.
+  - **The rule is type-aware, so each scope's ESLint config carries a parser project.** Each client's config must name **every leaf project covering its lint surface**, since the parser needs a program that literally *contains* the file — for cli, tui, and launcher that is the two they already typecheck (`tsconfig.json` + `tsconfig.test.json`, `src` in the first and `__tests__` only in the second), and for **web it is four** (`tsconfig.app.json`, `tsconfig.node.json`, `tsconfig.storybook.json`, `tsconfig.test.json`) — web's `tsconfig.json` is a solution file with `files: []`, so naming it alone would contain nothing. Adding a leaf project to a client means adding it here too. The root config instead points at **`tsconfig.lint.json`**, which exists solely to give the parser a program covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts` — none of which is rooted in a tsconfig of its own. That file emits nothing and gates nothing: type *checking* for those sources stays where it was (`core/` through `clients/web`'s `tsc -b`, `test-servers/src` through `clients/cli`'s test project). It sets `moduleResolution: bundler` deliberately — `core/` uses extensionless relative imports, which NodeNext fails to resolve, degrading every import to `any` so the rule silently stops seeing promises at all. **Its `include` must stay a superset of the root config's type-aware `files` globs** — a file the lint block matches but the project omits fails outright with "was not found in any of the provided project(s)" rather than being checked, so widening one means widening the other (that is why the `include` carries `test-servers/src/**/*.tsx`, which nothing has produced yet). Note also that a tsconfig `include` does **not** expand braces — `core/**/*.{ts,tsx}` matches nothing; list the extensions separately.
   - Type-aware linting costs real time: `clients/web`'s `eslint .` went from ~8s to ~19s, and `lint` runs inside `validate`, the fast inner-loop check. That is the price of the guarantee; if it needs reducing later, narrowing the projects each scope loads is the lever, not dropping the rule.
 
 ## Web source layout: `src/lib` vs `src/utils`

clients/tui/__tests__/App.test.tsx

@@ -697,6 +697,31 @@ describe("App (foundation)", () => {
     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("switches tabs via accelerator keys", async () => {
     const r = await mount(stdioServer());
     await press(r, ["t"]); // tools tab (server is auto-selected)

clients/tui/src/App.tsx

@@ -170,6 +170,8 @@ 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
@@ -769,7 +771,9 @@ function App({
     if (!selectedServer || !selectedInspectorClient || !selectedServerConfig) {
       return;
     }
-    // A connect attempt supersedes whatever the last disconnect reported.
+    // 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 () => {
@@ -901,6 +905,14 @@ function App({
   // Disconnect handler
   const handleDisconnect = useCallback(async () => {
     if (!selectedServer) return;
+    // 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 {
@@ -908,7 +920,14 @@ function App({
       // 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.
+      // 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]);

clients/web/server/server.ts

@@ -9,7 +9,6 @@ import { readFileSync } from "node:fs";
 import { join, dirname } from "node:path";
 import { fileURLToPath } from "node:url";
 import { randomBytes } from "node:crypto";
-import open from "open";
 import { serve } from "@hono/node-server";
 import { serveStatic } from "@hono/node-server/serve-static";
 import { Hono } from "hono";
@@ -22,6 +21,7 @@ import type { WebServerConfig } from "./web-server-config.js";
 import {
   webServerConfigToInitialPayload,
   printServerBanner,
+  openBrowser,
 } from "./web-server-config.js";
 import type { WebServerHandle } from "./types.js";
 
@@ -125,13 +125,9 @@ export async function startHonoServer(
         sandboxUrl ?? undefined,
       );
       if (config.autoOpen) {
-        // A headless or otherwise browser-less host rejects here. The server
-        // is already listening and the banner above printed the URL, so warn
-        // and carry on rather than letting the rejection escape this
-        // synchronous listen callback and take the process down.
-        open(url).catch((err: unknown) => {
-          console.warn("Could not open a browser automatically:", err);
-        });
+        // Never a bare `open(url)`: this callback is synchronous, so a
+        // rejection would escape it. See openBrowser.
+        openBrowser(url);
       }
     },
   );

clients/web/server/vite-hono-plugin.ts

@@ -9,7 +9,6 @@
 
 import type { IncomingMessage, ServerResponse } from "node:http";
 import type { Plugin } from "vite";
-import open from "open";
 // Direct leaf import — the `core/mcp/remote/node/index.ts` barrel re-exports
 // from `../constants.js`, which would otherwise drag the broader
 // `core/mcp/remote/index.ts` barrel (including `createRemoteLogger`, with its
@@ -22,6 +21,7 @@ import type { WebServerConfig } from "./web-server-config.js";
 import {
   webServerConfigToInitialPayload,
   printServerBanner,
+  openBrowser,
 } from "./web-server-config.js";
 
 export function honoMiddlewarePlugin(config: WebServerConfig): Plugin {
@@ -114,12 +114,9 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin {
         );
 
         if (config.autoOpen) {
-          // Same as the prod server: a browser-less host rejects, the dev
-          // server is already up, and the banner above printed the URL — so
-          // warn rather than let the rejection escape this sync helper.
-          open(url).catch((err: unknown) => {
-            console.warn("Could not open a browser automatically:", err);
-          });
+          // Never a bare `open(url)`: `logBanner` is synchronous, so a
+          // rejection would escape it. See openBrowser.
+          openBrowser(url);
         }
       };
 

clients/web/server/web-server-config.ts

@@ -2,6 +2,7 @@
  * Config object for the web server (dev and prod). Passed in-process; no env handoff.
  */
 
+import open from "open";
 import pino from "pino";
 import type { Logger } from "pino";
 import type { MCPConfig, MCPServerConfig } from "../../../core/mcp/types.ts";
@@ -203,6 +204,24 @@ export function printServerBanner(
   return url;
 }
 
+/**
+ * Launch the user's browser at `url`, reporting a failure rather than throwing.
+ *
+ * Both callers — the prod Hono server's `serve()` listen callback and the dev
+ * plugin's `logBanner` — are synchronous, so a rejection has nowhere to go and
+ * escapes as an unhandled rejection (#1959). On a headless or otherwise
+ * browser-less host that rejection is the ordinary outcome, not an edge case,
+ * and by the time it happens the server is already listening and
+ * `printServerBanner` has printed a perfectly usable URL. So warn and carry
+ * on. Shared between the two servers deliberately: two copies of a one-line
+ * `.catch` is two chances for one of them to drift back to a bare `open(url)`.
+ */
+export function openBrowser(url: string): void {
+  open(url).catch((err: unknown) => {
+    console.warn("Could not open a browser automatically:", err);
+  });
+}
+
 export interface BuildWebServerConfigOptions {
   initialMcpConfig?: MCPServerConfig | null;
   /** Catalog/session file the backend should serve; see {@link WebServerConfig.mcpConfigPath}. */

clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts

@@ -20,6 +20,7 @@ import { serve } from "@hono/node-server";
 import type { ServerType } from "@hono/node-server";
 import type pinoType from "pino";
 import {
+  closeAbortedStream,
   createRemoteApp,
   requestIdForSendWait,
   mcpParamHeadersOnly,
@@ -1107,4 +1108,37 @@ describe("server.ts supplemental coverage", () => {
       }
     });
   });
+  describe("closeAbortedStream", () => {
+    it("owns a rejected close instead of letting it go unhandled", async () => {
+      // The whole reason the helper exists. Both SSE `onAbort` listeners run
+      // after the peer is already gone, so `close()` can lose the race and
+      // reject — and Hono invokes abort subscribers with a bare
+      // `subscriber()`, so nothing upstream would catch it. A regression here
+      // does not fail at the call site; it fails the whole vitest run from
+      // wherever the rejection happens to surface.
+      const rejected = Promise.reject(new Error("peer already gone"));
+      const unhandled: unknown[] = [];
+      const onUnhandled = (err: unknown) => unhandled.push(err);
+      process.on("unhandledRejection", onUnhandled);
+      try {
+        expect(closeAbortedStream({ close: () => rejected })).toBeUndefined();
+        // An unhandledRejection fires on a later macrotask, not this one.
+        await new Promise((resolve) => setTimeout(resolve, 20));
+      } finally {
+        process.off("unhandledRejection", onUnhandled);
+      }
+      expect(unhandled).toEqual([]);
+    });
+
+    it("returns without waiting on a close that resolves", async () => {
+      let closed = false;
+      closeAbortedStream({
+        close: async () => {
+          closed = true;
+        },
+      });
+      await Promise.resolve();
+      expect(closed).toBe(true);
+    });
+  });
 });

clients/web/src/test/integration/server/server-auto-open.test.ts

@@ -0,0 +1,149 @@
+/**
+ * `autoOpen` browser launch (#1959).
+ *
+ * Both servers call `openBrowser` from a synchronous callback — the prod
+ * server from `serve()`'s listen callback, the dev plugin from `logBanner` —
+ * so a rejection there has nowhere to go. On a headless or otherwise
+ * browser-less host that rejection is the *ordinary* outcome, not an edge
+ * case, and before #1959 it escaped as an unhandled rejection from a server
+ * that had already started listening and printed a perfectly usable URL.
+ *
+ * The helper is shared precisely so this behavior has one implementation to
+ * pin. The unit tests below cover it directly; the end-to-end test then
+ * proves the prod server routes through it and survives the failure. The dev
+ * plugin is not booted here — it returns early under `VITEST` by design (see
+ * `vite-hono-plugin.ts`) — but it calls the same tested helper.
+ */
+import { describe, it, expect, afterEach, vi } from "vitest";
+import { createServer } from "node:net";
+import { mkdtemp, writeFile, rm } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import type { WebServerConfig } from "../../../../server/web-server-config.js";
+import type { WebServerHandle } from "../../../../server/types.js";
+
+const openMock = vi.hoisted(() => vi.fn());
+vi.mock("open", () => ({ default: openMock }));
+
+// Imported after the mock is registered so both bind to the double.
+const { openBrowser } = await import("../../../../server/web-server-config.js");
+const { startHonoServer } = await import("../../../../server/server.js");
+
+const WARNING = "Could not open a browser automatically:";
+
+async function findFreePort(): Promise<number> {
+  return new Promise((resolve, reject) => {
+    const srv = createServer();
+    srv.unref();
+    srv.on("error", reject);
+    srv.listen(0, "127.0.0.1", () => {
+      const addr = srv.address();
+      if (addr && typeof addr === "object") {
+        const { port } = addr;
+        srv.close(() => resolve(port));
+      } else {
+        srv.close(() => reject(new Error("Could not resolve a free port")));
+      }
+    });
+  });
+}
+
+/** An unhandledRejection fires on a later macrotask than the rejection. */
+async function settle() {
+  await new Promise((resolve) => setTimeout(resolve, 50));
+}
+
+const INDEX_HTML =
+  "<!doctype html><html><body><div id=root></div></body></html>";
+
+let handle: WebServerHandle | undefined;
+let staticRoot: string | undefined;
+
+afterEach(async () => {
+  await handle?.close();
+  handle = undefined;
+  if (staticRoot) await rm(staticRoot, { recursive: true, force: true });
+  staticRoot = undefined;
+  openMock.mockReset();
+  vi.restoreAllMocks();
+});
+
+describe("openBrowser", () => {
+  it("owns a rejection and warns instead of letting it go unhandled", async () => {
+    // Suppress the expected warning rather than letting it pollute the run,
+    // but assert on it — the warning IS the user-visible half of the fix.
+    const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
+    const unhandled: unknown[] = [];
+    const onUnhandled = (err: unknown) => unhandled.push(err);
+    process.on("unhandledRejection", onUnhandled);
+    try {
+      openMock.mockRejectedValue(new Error("no browser on this host"));
+      expect(openBrowser("http://127.0.0.1:6274")).toBeUndefined();
+      await settle();
+    } finally {
+      process.off("unhandledRejection", onUnhandled);
+    }
+    expect(unhandled).toEqual([]);
+    expect(warn).toHaveBeenCalledWith(WARNING, expect.any(Error));
+  });
+
+  it("stays quiet when the browser opens", async () => {
+    const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
+    openMock.mockResolvedValue(undefined);
+    openBrowser("http://127.0.0.1:6274");
+    await settle();
+    expect(openMock).toHaveBeenCalledWith("http://127.0.0.1:6274");
+    expect(warn).not.toHaveBeenCalledWith(WARNING, expect.anything());
+  });
+});
+
+describe("startHonoServer autoOpen", () => {
+  it("keeps serving when the browser cannot be opened", async () => {
+    const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
+    const unhandled: unknown[] = [];
+    const onUnhandled = (err: unknown) => unhandled.push(err);
+    process.on("unhandledRejection", onUnhandled);
+
+    try {
+      openMock.mockRejectedValue(new Error("no browser on this host"));
+
+      staticRoot = await mkdtemp(join(tmpdir(), "inspector-autoopen-"));
+      await writeFile(join(staticRoot, "index.html"), INDEX_HTML, "utf-8");
+      const port = await findFreePort();
+      const baseUrl = `http://127.0.0.1:${port}`;
+      const config: WebServerConfig = {
+        port,
+        hostname: "127.0.0.1",
+        authToken: "auto-open-token",
+        dangerouslyOmitAuth: false,
+        initialMcpConfig: null,
+        mcpConfigPath: undefined,
+        writable: true,
+        initialServers: null,
+        storageDir: undefined,
+        allowedOrigins: [baseUrl],
+        sandboxPort: 0,
+        sandboxHost: "127.0.0.1",
+        logger: undefined,
+        autoOpen: true,
+        staticRoot,
+      };
+      handle = await startHonoServer(config);
+
+      // `serve()`'s listen callback fires after startHonoServer resolves.
+      for (let i = 0; i < 100 && openMock.mock.calls.length === 0; i++) {
+        await new Promise((resolve) => setTimeout(resolve, 10));
+      }
+      await settle();
+
+      expect(openMock).toHaveBeenCalledWith(expect.stringContaining(baseUrl));
+      expect(unhandled).toEqual([]);
+      expect(warn).toHaveBeenCalledWith(WARNING, expect.any(Error));
+      // The point of warning rather than throwing: the server is still up.
+      const res = await fetch(`${baseUrl}/`);
+      expect(res.status).toBe(200);
+    } finally {
+      process.off("unhandledRejection", onUnhandled);
+    }
+  });
+});

core/mcp/remote/node/server.ts

@@ -111,7 +111,7 @@ const SSE_PRIMING_COMMENT = ":\n\n";
  * `async` listener into the same unhandled rejection one layer up. Swallowing
  * it here is the only place the rejection has an owner.
  */
-function closeAbortedStream(stream: { close(): Promise<void> }): void {
+export function closeAbortedStream(stream: { close(): Promise<void> }): void {
   stream.close().catch(() => {});
 }
 

tsconfig.lint.json

@@ -27,6 +27,7 @@
     "core/**/*.ts",
     "core/**/*.tsx",
     "test-servers/src/**/*.ts",
+    "test-servers/src/**/*.tsx",
     "test-servers/src/**/*.mts",
     "test-servers/src/**/*.cts",
     "vitest.shared.mts"