test: address Copilot review — tighten progress-token guard and de-flake relay tests (#2028)

- progressTokenOf now delegates to the SDK's ProgressTokenSchema instead of a
  bare typeof check, so fractional and past-MAX_SAFE_INTEGER numbers are
  rejected (matching isProgressToken in inspectorClient.ts); tests cover both.
- Browser relay timeout tests use fake timers with advanceTimersByTimeAsync and
  flush the SSE consumer between pushes, replacing real-sleep windows that a
  loaded runner could race (#1596).
- The log-only timeout test holds and awaits its rejection rather than
  swallowing it in an empty catch.
- progressToken.test.ts gains a file header; the remote-session malformed-token
  fixture uses a single-cast helper instead of `as unknown as`.

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

clients/web/src/test/core/mcp/remote/progressToken.test.ts

@@ -1,3 +1,11 @@
+/**
+ * Unit tests for the shared `progressTokenOf` guard (#2028) — the string/safe
+ * integer extractor both relay waits (browser `RemoteClientTransport` and Node
+ * `RemoteSession`) use to correlate a `notifications/progress` with the request
+ * whose timeout it should re-arm. Covers the protocol's accept set and, since
+ * the guard delegates to the SDK's `ProgressTokenSchema`, the fractional and
+ * past-`MAX_SAFE_INTEGER` values a bare `typeof` check would have let through.
+ */
 import { describe, it, expect } from "vitest";
 import { progressTokenOf } from "@inspector/core/mcp/remote/progressToken.js";
 import type { JSONRPCMessage } from "@modelcontextprotocol/client";
@@ -70,4 +78,38 @@ describe("progressTokenOf (#2028)", () => {
       ),
     ).toBeUndefined();
   });
+
+  it("rejects a fractional numeric token (the protocol allows only integers)", () => {
+    expect(
+      progressTokenOf(
+        msg({
+          jsonrpc: "2.0",
+          method: "notifications/progress",
+          params: { progressToken: 1.5, progress: 1 },
+        }),
+      ),
+    ).toBeUndefined();
+  });
+
+  it("rejects an integer past MAX_SAFE_INTEGER", () => {
+    expect(
+      progressTokenOf(
+        msg({
+          jsonrpc: "2.0",
+          method: "notifications/progress",
+          params: { progressToken: Number.MAX_SAFE_INTEGER + 1, progress: 1 },
+        }),
+      ),
+    ).toBeUndefined();
+  });
+
+  it("accepts a safe integer at the boundary", () => {
+    expect(
+      progressTokenOf({
+        jsonrpc: "2.0",
+        method: "notifications/progress",
+        params: { progressToken: Number.MAX_SAFE_INTEGER, progress: 1 },
+      }),
+    ).toBe(Number.MAX_SAFE_INTEGER);
+  });
 });

clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts

@@ -1,4 +1,4 @@
-import { describe, it, expect, vi } from "vitest";
+import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
 import { RemoteClientTransport } from "@inspector/core/mcp/remote/remoteClientTransport.js";
 import type { MCPServerConfig } from "@inspector/core/mcp/types.js";
 
@@ -727,12 +727,22 @@ describe("RemoteClientTransport", () => {
       return { fetchFn, getSse: () => sse, getSentId: () => sentId };
     }
 
-    const delay = (ms: number) => new Promise((r) => setTimeout(r, ms));
+    // Fake timers so the deadline is driven deterministically rather than
+    // raced against a real `setTimeout` — a real-sleep test can let the timeout
+    // fire before a loaded runner processes the enqueued SSE frame (#1596: a
+    // race is fixed with fake timers, never with headroom). Each push is
+    // followed by a 0ms async advance to flush the SSE consumer's stream read
+    // so the frame is parsed before time moves.
+    const TIMEOUT_MS = 1000;
+    const flushSse = () => vi.advanceTimersByTimeAsync(0);
+
+    beforeEach(() => vi.useFakeTimers());
+    afterEach(() => vi.useRealTimers());
 
     it("re-arms the wait on each progress note, so a call outliving the timeout still resolves", async () => {
       const { fetchFn, getSse, getSentId } = backendHoldingSend();
       const transport = new RemoteClientTransport(
-        { baseUrl, fetchFn, sseResponseTimeoutMs: 150 },
+        { baseUrl, fetchFn, sseResponseTimeoutMs: TIMEOUT_MS },
         config,
       );
       await transport.start();
@@ -742,22 +752,25 @@ describe("RemoteClientTransport", () => {
         id: 1,
         method: "tools/call",
       });
+      await flushSse();
 
-      // Emit progress every 100ms (< the 150ms window) for ~400ms total, then
-      // the response. Without the re-arm the wait dies at 150ms.
-      for (let i = 0; i < 4; i++) {
-        await delay(100);
+      // Three progress notes, each after 900ms (< the 1000ms window): without
+      // the re-arm the wait dies at 1000ms, well before the third lands.
+      for (let i = 0; i < 3; i++) {
         getSse().pushMessage({
           jsonrpc: "2.0",
           method: "notifications/progress",
-          params: { progressToken: getSentId(), progress: i, total: 4 },
+          params: { progressToken: getSentId(), progress: i, total: 3 },
         });
+        await flushSse();
+        await vi.advanceTimersByTimeAsync(900);
       }
       getSse().pushMessage({
         jsonrpc: "2.0",
         id: getSentId(),
         result: { ok: true },
       });
+      await flushSse();
 
       await expect(sent).resolves.toBeUndefined();
       await transport.close();
@@ -766,7 +779,7 @@ describe("RemoteClientTransport", () => {
     it("does not re-arm on notifications/message, so a log-only call still times out", async () => {
       const { fetchFn, getSse, getSentId } = backendHoldingSend();
       const transport = new RemoteClientTransport(
-        { baseUrl, fetchFn, sseResponseTimeoutMs: 150 },
+        { baseUrl, fetchFn, sseResponseTimeoutMs: TIMEOUT_MS },
         config,
       );
       await transport.start();
@@ -776,24 +789,23 @@ describe("RemoteClientTransport", () => {
         id: 1,
         method: "tools/call",
       });
-      void sent.catch(() => {});
-
-      // Only log messages — these must not extend the deadline.
-      for (let i = 0; i < 4; i++) {
-        await delay(50);
-        getSse().pushMessage({
-          jsonrpc: "2.0",
-          method: "notifications/message",
-          params: {
-            level: "info",
-            data: { msg: `tick ${i}`, token: getSentId() },
-          },
-        });
-      }
-
-      await expect(sent).rejects.toThrow(
+      // Hold the expected rejection now (as the RemoteSession timeout tests do)
+      // so it is asserted rather than suppressed by an empty catch.
+      const rejection = expect(sent).rejects.toThrow(
         /Timed out waiting for MCP response on SSE/,
       );
+      await flushSse();
+
+      // A log message before the deadline must not extend it.
+      getSse().pushMessage({
+        jsonrpc: "2.0",
+        method: "notifications/message",
+        params: { level: "info", data: { msg: "tick", token: getSentId() } },
+      });
+      await flushSse();
+      await vi.advanceTimersByTimeAsync(TIMEOUT_MS);
+
+      await rejection;
       await transport.close();
     });
   });

clients/web/src/test/integration/mcp/remote/remote-session.test.ts

@@ -7,11 +7,16 @@
  */
 
 import { describe, it, expect, vi } from "vitest";
-import type { Transport } from "@modelcontextprotocol/client";
+import type { Transport, JSONRPCMessage } from "@modelcontextprotocol/client";
 import { RemoteSession } from "@inspector/core/mcp/remote/node/remote-session.js";
 import type { FetchRequestEntryBase } from "@inspector/core/mcp/types.js";
 import { AuthChallengeError } from "@inspector/core/auth/challenge.js";
 
+// Stage a deliberately off-spec message as `unknown` and narrow with a single
+// cast — the guard branches these fixtures exercise are ones the typed
+// `JSONRPCMessage` shape forbids, and `unknown` is the safe top type to pass in.
+const asMsg = (m: unknown): JSONRPCMessage => m as JSONRPCMessage;
+
 function makeFetchEntry(
   overrides: Partial<FetchRequestEntryBase> = {},
 ): FetchRequestEntryBase {
@@ -289,11 +294,13 @@ describe("RemoteSession", () => {
     const wait = session.waitForRequestResponse(8, 1000);
     const rejection = expect(wait).rejects.toThrow(/timed out/);
     // A malformed token (object) must be ignored, not coerced to a key.
-    session.onMessage({
-      jsonrpc: "2.0",
-      method: "notifications/progress",
-      params: { progressToken: { bad: true }, progress: 1 },
-    } as unknown as Parameters<RemoteSession["onMessage"]>[0]);
+    session.onMessage(
+      asMsg({
+        jsonrpc: "2.0",
+        method: "notifications/progress",
+        params: { progressToken: { bad: true }, progress: 1 },
+      }),
+    );
     await vi.advanceTimersByTimeAsync(1000);
     await rejection;
     vi.useRealTimers();

core/mcp/remote/progressToken.ts

@@ -4,15 +4,20 @@
  * and the Node `RemoteSession` import it rather than each carrying a copy.
  */
 
+import { ProgressTokenSchema } from "@modelcontextprotocol/core";
 import type { JSONRPCMessage } from "@modelcontextprotocol/client";
 
 /**
- * The id of the request a `notifications/progress` belongs to, or `undefined`
- * for any other message. The SDK stamps `progressToken: messageId` on the
- * outbound request, so a progress notification's `progressToken` is the id of
- * the request whose wait should be re-armed. A JSON-RPC progress token is a
- * string or a number; anything else is ignored so a malformed token can't be
- * coerced into a map key.
+ * The token identifying the request a `notifications/progress` belongs to, or
+ * `undefined` for any other message. The SDK stamps `progressToken: messageId`
+ * on the outbound request, so a progress notification's `progressToken` is the
+ * id of the request whose wait should be re-armed.
+ *
+ * Validation is delegated to the SDK's own `ProgressTokenSchema` rather than a
+ * hand-rolled `typeof` check — the protocol allows only a string or a *safe*
+ * integer, and zod's `.int()` rejects a fractional or past-`MAX_SAFE_INTEGER`
+ * value that a bare `typeof token === "number"` would wave through (the same
+ * drift `isProgressToken` in `core/mcp/inspectorClient.ts` exists to avoid).
  */
 export function progressTokenOf(
   message: JSONRPCMessage,
@@ -24,9 +29,11 @@ export function progressTokenOf(
     typeof message.params === "object" &&
     message.params !== null
   ) {
-    const token = (message.params as { progressToken?: unknown }).progressToken;
-    if (typeof token === "string" || typeof token === "number") {
-      return token;
+    const parsed = ProgressTokenSchema.safeParse(
+      (message.params as { progressToken?: unknown }).progressToken,
+    );
+    if (parsed.success) {
+      return parsed.data;
     }
   }
   return undefined;