fix: address Copilot review round 2 on #2318
- fetchTracking: media types and HTTP methods compare case-insensitively; a `Request` input's own method is honoured; the SSE line scanner accepts LF, CRLF and a bare CR, holding a trailing CR until the next chunk decides whether it is half of a CRLF. - MessageTrackingTransport does not report a send failure for a request the caller aborted through `requestSignal`, and InspectorClient keeps a request whose send failed with a request timeout — both are sent, unanswered requests, which is the state to keep reporting. - createTransportNode builds a fetch tracker when any tracking callback is supplied, not only `onFetchRequest`. - RemoteClientTransport's relay wait expires with the SDK's own timeout shape, so the web client's relay timeout is annotated like every other. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APKSYHGxXBUpnR7ErFiLJn Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
1a5dd6820d
commit
c565b56fcd
6 changed files with 266 additions and 27 deletions
|
|
@ -117,7 +117,7 @@ describe("RemoteClientTransport send contract", () => {
|
|||
const started = Date.now();
|
||||
await expect(
|
||||
transport.send({ jsonrpc: "2.0", id: 1, method: "tools/list" }),
|
||||
).rejects.toThrow(/Timed out waiting for MCP response on SSE/i);
|
||||
).rejects.toThrow(/Request timed out/);
|
||||
expect(Date.now() - started).toBeLessThan(5000);
|
||||
|
||||
await transport.close();
|
||||
|
|
|
|||
|
|
@ -826,6 +826,56 @@ describe("createFetchTracker long-lived stream watching (#2318)", () => {
|
|||
expect(last.closedAt).toBeInstanceOf(Date);
|
||||
});
|
||||
|
||||
it("accepts bare CR line endings, and a CRLF split across chunks", async () => {
|
||||
const server = controlledStream();
|
||||
const { updates } = await trackStream(server.stream);
|
||||
server.push("data: a\r\rdata: b\r");
|
||||
await flush();
|
||||
// The trailing CR is held: it may be the first half of a CRLF.
|
||||
expect(updates.map((u) => u.stream.eventCount)).toEqual([0, 1]);
|
||||
server.push("\n\r\n");
|
||||
await flush();
|
||||
expect(updates.map((u) => u.stream.eventCount)).toEqual([0, 1, 2]);
|
||||
});
|
||||
|
||||
it("classifies the content type and the method case-insensitively", async () => {
|
||||
const server = controlledStream();
|
||||
const updates: unknown[] = [];
|
||||
const fetcher = createFetchTracker(
|
||||
(async () =>
|
||||
new Response(server.stream, {
|
||||
headers: { "content-type": "Text/Event-Stream" },
|
||||
})) as typeof fetch,
|
||||
{ updateStream: (id, state) => updates.push({ id, state }) },
|
||||
);
|
||||
await fetcher("https://example.com/mcp", { method: "get" });
|
||||
expect(updates).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("takes the method from a Request input when init carries none", async () => {
|
||||
// A bounded POST reply must go down the body-capture path, not the
|
||||
// watcher, when the caller passed a `Request` rather than `init`.
|
||||
const bodies: string[] = [];
|
||||
const updates: unknown[] = [];
|
||||
const tracked: FetchRequestEntryBase[] = [];
|
||||
const fetcher = createFetchTracker(
|
||||
(async () =>
|
||||
new Response("data: x\n\n", {
|
||||
headers: { "content-type": "text/event-stream" },
|
||||
})) as typeof fetch,
|
||||
{
|
||||
trackRequest: (entry) => tracked.push(entry),
|
||||
updateResponseBody: (_id, body) => bodies.push(body),
|
||||
updateStream: (id, state) => updates.push({ id, state }),
|
||||
},
|
||||
);
|
||||
await fetcher(new Request("https://example.com/mcp", { method: "POST" }));
|
||||
await flush();
|
||||
expect(tracked[0]?.method).toBe("POST");
|
||||
expect(bodies).toEqual(["data: x\n\n"]);
|
||||
expect(updates).toEqual([]);
|
||||
});
|
||||
|
||||
it("reports the close when the server ends the stream", async () => {
|
||||
const server = controlledStream();
|
||||
const { updates, id } = await trackStream(server.stream);
|
||||
|
|
|
|||
|
|
@ -0,0 +1,75 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
|
||||
import type { JSONRPCRequest } from "@modelcontextprotocol/client";
|
||||
import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js";
|
||||
import type { MessageTrackingCallbacks } from "@inspector/core/mcp/messageTrackingTransport.js";
|
||||
|
||||
/**
|
||||
* The outstanding-request bookkeeping behind the connection diagnostics
|
||||
* (#2318), driven through the message-tracking callbacks the client hands its
|
||||
* transport — the same seam the transport uses, without a connection.
|
||||
*/
|
||||
describe("InspectorClient diagnostics tracking callbacks", () => {
|
||||
interface Internals {
|
||||
createMessageTrackingCallbacks: () => MessageTrackingCallbacks;
|
||||
}
|
||||
|
||||
function makeClient(): {
|
||||
client: InspectorClient;
|
||||
callbacks: MessageTrackingCallbacks;
|
||||
} {
|
||||
const client = new InspectorClient(
|
||||
{ type: "stdio", command: "noop", args: [] },
|
||||
// Never connects; the callbacks are exercised directly.
|
||||
{ environment: { transport: () => ({}) as never } },
|
||||
);
|
||||
const callbacks = (
|
||||
client as unknown as Internals
|
||||
).createMessageTrackingCallbacks();
|
||||
return { client, callbacks };
|
||||
}
|
||||
|
||||
const request = (id: number): JSONRPCRequest => ({
|
||||
jsonrpc: "2.0",
|
||||
id,
|
||||
method: "tools/list",
|
||||
});
|
||||
|
||||
const outstandingIds = (client: InspectorClient) =>
|
||||
client.getConnectionDiagnostics().outstandingRequests.map((r) => r.id);
|
||||
|
||||
it("rolls a request back when its send failed before reaching the wire", () => {
|
||||
const { client, callbacks } = makeClient();
|
||||
callbacks.trackRequest!(request(1), "client");
|
||||
expect(outstandingIds(client)).toEqual([1]);
|
||||
callbacks.trackSendFailure!(request(1), new Error("connection closed"));
|
||||
expect(outstandingIds(client)).toEqual([]);
|
||||
});
|
||||
|
||||
it("keeps a request whose send failed with a request timeout", () => {
|
||||
// The browser's remote transport awaits the response inside `send`, so
|
||||
// its relay timeout surfaces as a send failure — the request reached the
|
||||
// server and is still unanswered.
|
||||
const { client, callbacks } = makeClient();
|
||||
callbacks.trackRequest!(request(1), "client");
|
||||
callbacks.trackSendFailure!(
|
||||
request(1),
|
||||
new SdkError(SdkErrorCode.RequestTimeout, "Request timed out", {
|
||||
timeout: 60_000,
|
||||
}),
|
||||
);
|
||||
expect(outstandingIds(client)).toEqual([1]);
|
||||
});
|
||||
|
||||
it("only tracks client-originated requests, and ignores a rollback for an unknown id", () => {
|
||||
const { client, callbacks } = makeClient();
|
||||
callbacks.trackRequest!(request(7), "server");
|
||||
expect(outstandingIds(client)).toEqual([]);
|
||||
let changes = 0;
|
||||
client.addEventListener("connectionDiagnosticsChange", () => {
|
||||
changes += 1;
|
||||
});
|
||||
callbacks.trackSendFailure!(request(7), new Error("nope"));
|
||||
expect(changes).toBe(0);
|
||||
});
|
||||
});
|
||||
|
|
@ -48,6 +48,18 @@ describe("MessageTrackingTransport.send", () => {
|
|||
await expect(failing.send(request)).rejects.toBe(boom);
|
||||
expect(callbacks.trackRequest).toHaveBeenCalledWith(request, "client");
|
||||
expect(trackSendFailure).toHaveBeenCalledWith(request, boom);
|
||||
// A caller's own abort is a cancellation, not a send failure: the
|
||||
// request may well have reached the server, and stays unanswered.
|
||||
trackSendFailure.mockClear();
|
||||
const controller = new AbortController();
|
||||
controller.abort();
|
||||
await expect(
|
||||
failing.send(
|
||||
{ jsonrpc: "2.0", id: 2, method: "tools/call" },
|
||||
{ requestSignal: controller.signal },
|
||||
),
|
||||
).rejects.toBe(boom);
|
||||
expect(trackSendFailure).not.toHaveBeenCalled();
|
||||
// Only requests roll back: a failed notification has no pending entry.
|
||||
trackSendFailure.mockClear();
|
||||
await expect(
|
||||
|
|
|
|||
|
|
@ -188,6 +188,35 @@ describe("Transport", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it("tracks fetches when only onFetchResponseBody is supplied (#2318)", async () => {
|
||||
// Any one of the tracking callbacks earns the tracker — a caller that
|
||||
// wants only bodies (or only stream updates) must not be handed the
|
||||
// bare fetch and silently receive nothing.
|
||||
const server = createTestServerHttp({
|
||||
serverInfo: createTestServerInfo(),
|
||||
tools: [createEchoTool()],
|
||||
serverType: "streamable-http",
|
||||
});
|
||||
try {
|
||||
await server.start();
|
||||
const bodies: string[] = [];
|
||||
const result = createTransportNode(
|
||||
{ type: "streamable-http", url: server.url },
|
||||
{ onFetchResponseBody: (_id, body) => bodies.push(body) },
|
||||
);
|
||||
const client = new Client(
|
||||
{ name: "test-client", version: "1.0.0" },
|
||||
{ capabilities: {} },
|
||||
);
|
||||
await client.connect(result.transport);
|
||||
await client.listTools();
|
||||
await client.close();
|
||||
expect(bodies.some((body) => body.includes("tools"))).toBe(true);
|
||||
} finally {
|
||||
await server.stop();
|
||||
}
|
||||
});
|
||||
|
||||
it("should call onFetchRequest callback for streamable-http transport", async () => {
|
||||
const server = createTestServerHttp({
|
||||
serverInfo: createTestServerInfo(),
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ import type { RemoteTransportOptions } from "@inspector/core/mcp/remote/remoteCl
|
|||
import type { MCPServerConfig } from "@inspector/core/mcp/types.js";
|
||||
import type { RemoteEvent } from "@inspector/core/mcp/remote/types.js";
|
||||
import type { JSONRPCMessage } from "@modelcontextprotocol/client";
|
||||
import { SdkErrorCode } from "@modelcontextprotocol/client";
|
||||
import {
|
||||
AuthChallengeError,
|
||||
AuthRecoveryRequiredError,
|
||||
|
|
@ -615,6 +616,27 @@ describe("RemoteClientTransport (focused branch coverage)", () => {
|
|||
).rejects.toThrow(/Transport is closed/);
|
||||
});
|
||||
|
||||
it("rejects an expired SSE response wait with the SDK's own timeout shape (#2318)", async () => {
|
||||
// The relay's budget, not the request's, can be the one that expires;
|
||||
// shaped as the SDK's timeout so InspectorClient annotates it the same.
|
||||
const t = makeTransport(
|
||||
{
|
||||
events: () => openEventsResponse(),
|
||||
send: () => jsonResponse({ ok: true }),
|
||||
},
|
||||
{ sseResponseTimeoutMs: 50 },
|
||||
);
|
||||
await t.start();
|
||||
await expect(
|
||||
t.send({ jsonrpc: "2.0", id: 1, method: "tools/list" }),
|
||||
).rejects.toMatchObject({
|
||||
code: SdkErrorCode.RequestTimeout,
|
||||
message: "Request timed out",
|
||||
data: { timeout: 50 },
|
||||
});
|
||||
await t.close();
|
||||
});
|
||||
|
||||
it("does not wait for an SSE response on subscriptions/listen (long-lived stream)", async () => {
|
||||
// A `subscriptions/listen` never yields a JSON-RPC response — only an
|
||||
// acknowledged notification over the event channel — so send() must
|
||||
|
|
|
|||
Loading…
Reference in a new issue