Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { describe, it, expect, vi } from "vitest";
import { SdkErrorCode, SdkHttpError } from "@modelcontextprotocol/client";
import type {
JSONRPCMessage,
Root,
Expand Down Expand Up @@ -1240,4 +1241,64 @@ describe("InspectorClient peer-handler timing (#1797)", () => {
expect(silent.getRoots()).toEqual([{ uri: "file:///a" }]);
await silent.disconnect();
});

/*
* The SDK's StreamableHTTP `_send` reports a failed POST through `onerror`
* and then rethrows it to the sender. A deterministic HTTP refusal is a
* refused request, which its rejection already surfaces, so it must not
* error the whole connection (#2776). Anything else reported here still does.
*/
describe("transport onerror (#2776)", () => {
async function connectedClient() {
const transport = new SampleAfterConnectTransport();
const client = new InspectorClient(
{ type: "stdio", command: "noop", args: [] },
{ environment: { transport: () => ({ transport }) } },
);
await client.connect();
const errors: unknown[] = [];
client.addEventListener("error", (e) => errors.push(e.detail));
return { client, transport, errors };
}

const postFailure = (status: number, statusText: string) =>
new SdkHttpError(
SdkErrorCode.ClientHttpNotImplemented,
`Error POSTing to endpoint: ${statusText}`,
{ status, statusText },
);

it.each([
[400, "Bad Request"],
[403, "Forbidden"],
[404, "Not Found"],
[405, "Method Not Allowed"],
])(
"keeps the connection up for a POST refused with HTTP %i",
async (status, statusText) => {
const { client, transport, errors } = await connectedClient();
transport.onerror?.(postFailure(status, statusText));
expect(client.getStatus()).toBe("connected");
expect(errors).toEqual([]);
await client.disconnect();
},
);

it.each([
["a transient POST failure (HTTP 503)", postFailure(503, "Unavailable")],
[
"an auth failure",
new SdkHttpError(SdkErrorCode.ClientHttpAuthentication, "Forbidden", {
status: 403,
}),
],
["a non-HTTP transport error", new Error("stream broke")],
])("still errors the connection for %s", async (_label, error) => {
const { client, transport, errors } = await connectedClient();
transport.onerror?.(error);
expect(client.getStatus()).toBe("error");
expect(errors).toEqual([error]);
await client.disconnect();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -1275,6 +1275,9 @@ describe("resource subscriptions era fork (#1630)", () => {
refusal: { status: 403, statusText: "Forbidden" },
});
expect(internals(connected).modernReconnectAttempts).toBe(0);
// The Node transport reports the refused POST through `onerror` too;
// a refused request is not a failed connection (#2776).
expect(connected.getStatus()).toBe("connected");
});

it("reports the refusal even when the rollback empties the filter", async () => {
Expand Down Expand Up @@ -1307,6 +1310,13 @@ describe("resource subscriptions era fork (#1630)", () => {
status: "refused",
refusal: { status: 403 },
});
// This is the TUI's path to its refusal line: on the Node transport
// the session must survive the refused connect-time listen (#2776).
expect(connected.getStatus()).toBe("connected");
expect(connected.getResourceSubscriptionStreamState().refusal).toEqual({
status: 403,
statusText: "Forbidden",
});
});

it.each([400, 404, 405])(
Expand All @@ -1333,10 +1343,11 @@ describe("resource subscriptions era fork (#1630)", () => {

/*
* The tests above drive the Node transport, which reports a failed POST
* through `onerror` as well as rejecting it — so the client is already
* `error` by the time the reconnect timer would fire, and the timer bails
* on the terminal status either way. The web client's remote transport
* rejects without `onerror`, which is the path the smoke observed the
* through `onerror` as well as rejecting it. Since #2776 that report no
* longer errors the client for a refusal, so the session stays connected
* there too; before it, the client was already `error` by the time the
* reconnect timer would fire. The web client's remote transport rejects
* without `onerror`, which is the path the smoke observed the
* eight-attempt run on. These stub `listen()` with the error that
* transport rebuilds (#2660) to model it: the session stays connected,
* and only the classification stands between the refusal and the retry.
Expand Down
10 changes: 10 additions & 0 deletions core/mcp/inspectorClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1345,6 +1345,16 @@ export class InspectorClient extends InspectorClientEventTarget {
// from any non-"connecting" state captures the reason regardless of
// ordering.
if (this.status === "connecting") return;
// A deterministic HTTP refusal of a POST is a refused REQUEST, not a
// failed connection (#2776). The SDK's StreamableHTTP `_send` reports it
// here and then rethrows it to the sender, so the request's rejection
// already surfaces it, and #2722's listen refusal settles its own stream
// status from that rejection. Flipping the client to "error" here made
// that status unreachable on the Node transport. The classifier matches
// only the code the SDK throws from its POST path, so a failure with no
// other surface (an SSE GET stream, a dropped connection, a 5xx) and
// every auth failure still land below.
if (subscriptionListenRefusal(error) !== undefined) return;
this.status = "error";
this.dispatchTypedEvent("statusChange", this.status);
this.dispatchTypedEvent("error", error);
Expand Down
Loading