test/docs: address Copilot review round 1 (#2320)

- Assert headersToServerSettings carries DEFAULT_CONNECTION_TIMEOUT_MS.
- Type the fake transports in inspectorClient.test.ts as the SDK's
  Transport via two helpers instead of an as-unknown-as double cast; the
  two pre-existing copies of the same fakes in that describe block use the
  helpers too.
- Bring serverList.ts's converter comments and specification/v2_servers_file.md
  in line with the new contract: 30 s default, explicit 0 persisted, and
  requestTimeout's 0 unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4h8Xa6Ed9zwtv5ojX35MT
Signed-off-by: cliffhall <cliff@futurescale.com>

clients/web/src/test/core/mcp/node/servers.test.ts

@@ -18,6 +18,7 @@ import {
   selectServerEntry,
   type ResolvedServer,
 } from "@inspector/core/mcp/node/servers.js";
+import { DEFAULT_CONNECTION_TIMEOUT_MS } from "@inspector/core/mcp/types.js";
 
 describe("headersToServerSettings", () => {
   it("returns undefined when no headers are given", () => {
@@ -32,6 +33,10 @@ describe("headersToServerSettings", () => {
     ]);
     expect(settings?.metadata).toEqual({});
     expect(settings?.roots).toEqual([]);
+    // The header-only shell is what the CLI/TUI hand the client when a file
+    // sets no timeout, so it must carry the product default — a regression
+    // back to 0 here would silently unbound every such connect (#2320).
+    expect(settings?.connectionTimeout).toBe(DEFAULT_CONNECTION_TIMEOUT_MS);
   });
 });
 

clients/web/src/test/integration/mcp/inspectorClient.test.ts

@@ -1,5 +1,6 @@
 import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
 import { resolve } from "node:path";
+import type { Transport } from "@modelcontextprotocol/client";
 import * as z from "zod/v4";
 import {
   InspectorClient,
@@ -367,25 +368,40 @@ describe("InspectorClient", () => {
       messageLogState.destroy();
     });
 
+    /**
+     * A transport whose `start()` never resolves: the "server that accepts
+     * the TCP connection and never answers" from the #2320 measurement.
+     * Typed as the SDK's `Transport` rather than cast to it, so an interface
+     * change upstream is caught here instead of at runtime.
+     */
+    function hangingTransport(): Transport {
+      return {
+        start: () => new Promise<void>(() => {}),
+        send: async () => {},
+        close: async () => {},
+      };
+    }
+
+    /** A transport whose `start()` rejects with a recoverable 401. */
+    function unauthorizedTransport(): Transport {
+      return {
+        start: async () => {
+          const err = new Error("Unauthorized") as Error & { status?: number };
+          err.status = 401;
+          throw err;
+        },
+        send: async () => {},
+        close: async () => {},
+      };
+    }
+
     it("rejects connect() with a timeout error when serverSettings.connectionTimeout fires", async () => {
       // Stub transport whose start() never resolves — simulates a slow /
       // unreachable upstream. InspectorClient.connect() should race against
       // serverSettings.connectionTimeout and reject with a descriptive error;
       // status should end up in "error", and the client should have
       // internally torn down the transport (next connect() must rebuild).
-      const hangingTransport = {
+      const fakeFactory = () => ({ transport: hangingTransport() });
-        start: () => new Promise<void>(() => {}),
-        send: async () => {},
-        close: async () => {},
-        onclose: undefined,
-        onerror: undefined,
-        onmessage: undefined,
-        sessionId: undefined,
-      };
-      const fakeFactory = () => ({
-        transport:
-          hangingTransport as unknown as import("@modelcontextprotocol/client").Transport,
-      });
       client = new InspectorClient(
         { type: "streamable-http", url: "http://localhost:1/never" },
         {
@@ -417,19 +433,7 @@ describe("InspectorClient", () => {
     });
 
     it("names the bound that fired and where to raise it in the timeout message (#2320)", async () => {
-      const hangingTransport = {
+      const fakeFactory = () => ({ transport: hangingTransport() });
-        start: () => new Promise<void>(() => {}),
-        send: async () => {},
-        close: async () => {},
-        onclose: undefined,
-        onerror: undefined,
-        onmessage: undefined,
-        sessionId: undefined,
-      };
-      const fakeFactory = () => ({
-        transport:
-          hangingTransport as unknown as import("@modelcontextprotocol/client").Transport,
-      });
       client = new InspectorClient(
         { type: "streamable-http", url: "http://localhost:1/never" },
         {
@@ -464,19 +468,7 @@ describe("InspectorClient", () => {
       // Fake timers so the 30 s default can fire without waiting it out.
       vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] });
       try {
-        const hangingTransport = {
+        const fakeFactory = () => ({ transport: hangingTransport() });
-          start: () => new Promise<void>(() => {}),
-          send: async () => {},
-          close: async () => {},
-          onclose: undefined,
-          onerror: undefined,
-          onmessage: undefined,
-          sessionId: undefined,
-        };
-        const fakeFactory = () => ({
-          transport:
-            hangingTransport as unknown as import("@modelcontextprotocol/client").Transport,
-        });
         client = new InspectorClient(
           { type: "streamable-http", url: "http://localhost:1/never" },
           { environment: { transport: fakeFactory } },
@@ -504,19 +496,7 @@ describe("InspectorClient", () => {
       vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] });
       try {
         let settled = false;
-        const hangingTransport = {
+        const fakeFactory = () => ({ transport: hangingTransport() });
-          start: () => new Promise<void>(() => {}),
-          send: async () => {},
-          close: async () => {},
-          onclose: undefined,
-          onerror: undefined,
-          onmessage: undefined,
-          sessionId: undefined,
-        };
-        const fakeFactory = () => ({
-          transport:
-            hangingTransport as unknown as import("@modelcontextprotocol/client").Transport,
-        });
         client = new InspectorClient(
           { type: "streamable-http", url: "http://localhost:1/never" },
           {
@@ -551,23 +531,7 @@ describe("InspectorClient", () => {
     });
 
     it("holds status at connecting when connect fails with a recoverable 401", async () => {
-      const unauthorizedTransport = {
+      const fakeFactory = () => ({ transport: unauthorizedTransport() });
-        start: async () => {
-          const err = new Error("Unauthorized") as Error & { status?: number };
-          err.status = 401;
-          throw err;
-        },
-        send: async () => {},
-        close: async () => {},
-        onclose: undefined,
-        onerror: undefined,
-        onmessage: undefined,
-        sessionId: undefined,
-      };
-      const fakeFactory = () => ({
-        transport:
-          unauthorizedTransport as unknown as import("@modelcontextprotocol/client").Transport,
-      });
       client = new InspectorClient(
         { type: "streamable-http", url: "http://localhost:8081/mcp" },
         { environment: { transport: fakeFactory } },
@@ -584,23 +548,7 @@ describe("InspectorClient", () => {
       // recovery. With a non-zero default that gate would have fired for
       // every user, so the teardown is now keyed to the timer actually
       // winning the race, and this is the case that proves it.
-      const unauthorizedTransport = {
+      const fakeFactory = () => ({ transport: unauthorizedTransport() });
-        start: async () => {
-          const err = new Error("Unauthorized") as Error & { status?: number };
-          err.status = 401;
-          throw err;
-        },
-        send: async () => {},
-        close: async () => {},
-        onclose: undefined,
-        onerror: undefined,
-        onmessage: undefined,
-        sessionId: undefined,
-      };
-      const fakeFactory = () => ({
-        transport:
-          unauthorizedTransport as unknown as import("@modelcontextprotocol/client").Transport,
-      });
       client = new InspectorClient(
         { type: "streamable-http", url: "http://localhost:8081/mcp" },
         {

core/mcp/serverList.ts

@@ -491,8 +491,11 @@ export function oauthEndpointOverridesFromSettings(
  *
  * `headers` becomes a pair-array preserving the object's key insertion order;
  * `oauth.*` becomes the flat `oauthClientId` / `oauthClientSecret` /
- * `oauthScopes` fields. Numeric timeouts default to 0 when absent — the form
+ * `oauthScopes` fields. The numeric timeouts get concrete values when absent
- * needs concrete values to render and 0 is the SDK's "no timeout" signal.
+ * because the form needs them to render: `connectionTimeout` reads back as
+ * `DEFAULT_CONNECTION_TIMEOUT_MS` (30 s), while `requestTimeout` reads back as
+ * 0, the SDK's "use your own default" signal. An explicit `connectionTimeout`
+ * of 0 is preserved — it is the user's "no timeout" opt-out (#2320).
  *
  * `env` / `cwd` are SDK config fields (not Inspector-extension keys), but they
  * are mirrored into the settings here so the Server Settings modal can edit
@@ -637,8 +640,10 @@ export function storedFieldsToInspectorSettings(
  * Splat the form-shape `InspectorServerSettings` back into the on-disk
  * Inspector-extension fields (object-form `headers`, nested `oauth`, etc.).
  * Empty-key rows are dropped — the form lets users leave new rows blank
- * mid-edit and those shouldn't reach disk. Numeric timeouts at 0 are omitted
+ * mid-edit and those shouldn't reach disk. Timeouts at their default are
- * so the file diff stays minimal for entries that never touched them.
+ * omitted so the file diff stays minimal for entries that never touched them:
+ * that is `DEFAULT_CONNECTION_TIMEOUT_MS` for `connectionTimeout` (an explicit
+ * 0 is a chosen opt-out and is written) and 0 for `requestTimeout`.
  *
  * Returns the field deltas to merge onto a `StoredMCPServer`; callers can
  * spread the result.
@@ -723,10 +728,11 @@ export function inspectorSettingsToStoredFields(
     out.modernLogLevel = settings.modernLogLevel;
   }
 
-  // Persist only when it differs from the default. Unlike the timeouts, 0 is a
+  // Persist only when it differs from the default. Like connectionTimeout and
-  // meaningful value here (unlimited), so the omit-sentinel is the default
+  // unlike requestTimeout, 0 is a meaningful value here (unlimited), so the
-  // itself rather than 0 — writing the default would inject the field into
+  // omit-sentinel is the default itself rather than 0 — writing the default
-  // hand-edited files that never had it and break byte-stable round-trips.
+  // would inject the field into hand-edited files that never had it and break
+  // byte-stable round-trips.
   if (settings.maxFetchRequests !== DEFAULT_MAX_FETCH_REQUESTS) {
     out.maxFetchRequests = settings.maxFetchRequests;
   }

specification/v2_servers_file.md

@@ -240,7 +240,7 @@ Each server entry may carry these Inspector-extension fields at the top level:
 
 - **Shape**:
   - On disk (`StoredMCPServer` in `core/mcp/types.ts`): `headers` as `Record<string, string>`, `metadata` as a pair-array (Inspector-only — no compat target), numeric timeouts, `oauth` as a nested `{ clientId?, clientSecret?, scopes? }` object. Every field optional; absent fields are omitted on disk so the file diff stays minimal.
-  - In memory + on the wire (`InspectorServerSettings` in `core/mcp/types.ts`): `headers` as a pair-array `{ key, value }[]`, flat `oauthClientId` / `oauthClientSecret` / `oauthScopes` fields, required `metadata` (pair-array) + required numeric `connectionTimeout` / `requestTimeout` (the form needs concrete values to render; 0 is the SDK's "no timeout" signal).
+  - In memory + on the wire (`InspectorServerSettings` in `core/mcp/types.ts`): `headers` as a pair-array `{ key, value }[]`, flat `oauthClientId` / `oauthClientSecret` / `oauthScopes` fields, required `metadata` (pair-array) + required numeric `connectionTimeout` / `requestTimeout` (the form needs concrete values to render). For `requestTimeout`, 0 is the SDK's "use your own default" signal. For `connectionTimeout`, an absent on-disk value reads back as `DEFAULT_CONNECTION_TIMEOUT_MS` (30 s, #2320) and an explicit 0 means "no timeout" and is persisted as such.
   - The bidirectional conversion lives in `core/mcp/serverList.ts` (`storedFieldsToInspectorSettings` / `inspectorSettingsToStoredFields`) and is invoked by `mcpConfigToServerEntries` / `serverEntriesToMcpConfig` and by the server route's `buildStoredEntry`.
 - **Where it takes effect**:
   - `headers` → wire headers on every SSE / streamable-http request (`core/mcp/node/transport.ts` consumes the pair-array via `InspectorServerSettings.headers`).