modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: only attribute a decode rejection to a Protocol entry

Browse source 
markResponseRejected was called for ANY refresh failure. The id it
recovers is "the last response received for this method", which is the
failing exchange only when a response actually arrived and was refused
while decoding. A transport drop, a timeout, or an abort produces no
response frame at all — the last-answered id then still points at an
EARLIER successful call, so marking it stamped "Rejected by the
Inspector" onto an exchange that worked. That is the same class of lie
this issue exists to remove.

A server-sent JSON-RPC error is excluded for a different reason: the id
would be right, but the failure is the server's and its entry already
renders as an error from the error frame.

Gated on SdkErrorCode.InvalidResult / UnsupportedResultType — decisions
the client made about a frame in hand. The failure is still recorded as
list state either way; only the Protocol attribution is gated.

Caught in Copilot's review of #1953.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
This commit is contained in:
cliffhall 2026-08-08 17:26:36 -04:00
commit fad8480376
5 changed files with 117 additions and 15 deletions
Download patch file Download diff file Expand all files Collapse all files

3
clients/web/src/test/core/mcp/state/managedPromptsState.test.ts
View file

@ -1,4 +1,5 @@
import { describe, it, expect, beforeEach } from "vitest";
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
import type { Prompt } from "@modelcontextprotocol/client";
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
import { ManagedPromptsState } from "@inspector/core/mcp/state/managedPromptsState";
@ -289,7 +290,7 @@ describe("ManagedPromptsState", () => {
// pins THIS list's method string, which is what attributes a failure to the
// right Protocol entry (#1953).
it("records a failed load and attributes it to prompts/list", async () => {
const boom = new Error("nope");
const boom = new SdkError(SdkErrorCode.InvalidResult, "nope");
client.setStatus("connected");
client.listAllPrompts.mockRejectedValueOnce(boom);

3
clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts
View file

@ -1,4 +1,5 @@
import { describe, it, expect, beforeEach } from "vitest";
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprotocol/client";
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
import { ManagedResourceTemplatesState } from "@inspector/core/mcp/state/managedResourceTemplatesState";
@ -270,7 +271,7 @@ describe("ManagedResourceTemplatesState", () => {
// pins THIS list's method string, which is what attributes a failure to the
// right Protocol entry (#1953).
it("records a failed load and attributes it to resources/templates/list", async () => {
const boom = new Error("nope");
const boom = new SdkError(SdkErrorCode.InvalidResult, "nope");
client.setStatus("connected");
client.listAllResourceTemplates.mockRejectedValueOnce(boom);

3
clients/web/src/test/core/mcp/state/managedResourcesState.test.ts
View file

@ -1,4 +1,5 @@
import { describe, it, expect, beforeEach } from "vitest";
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
import type { Resource } from "@modelcontextprotocol/client";
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
import { ManagedResourcesState } from "@inspector/core/mcp/state/managedResourcesState";
@ -296,7 +297,7 @@ describe("ManagedResourcesState", () => {
// pins THIS list's method string, which is what attributes a failure to the
// right Protocol entry (#1953).
it("records a failed load and attributes it to resources/list", async () => {
const boom = new Error("nope");
const boom = new SdkError(SdkErrorCode.InvalidResult, "nope");
client.setStatus("connected");
client.listAllResources.mockRejectedValueOnce(boom);

77
clients/web/src/test/core/mcp/state/managedToolsState.test.ts
View file

@ -1,4 +1,5 @@
import { describe, it, expect, beforeEach } from "vitest";
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
import type { Tool } from "@modelcontextprotocol/client";
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
import { ManagedToolsState } from "@inspector/core/mcp/state/managedToolsState";
@ -453,14 +454,76 @@ describe("ManagedToolsState", () => {
expect(state.getError()?.message).toBe("just a string");
});
it("attributes the failure to its Protocol entry", async () => {
client.setStatus("connected");
client.listAllTools.mockRejectedValueOnce(boom);
await expect(state.refresh()).rejects.toThrow(boom);
expect(client.markResponseRejected).toHaveBeenCalledWith(
"tools/list",
boom.message,
// Only a decode rejection may be attributed to a Protocol entry. The id is
// recovered as "the last response for this method", which is the failing
// exchange ONLY when a response actually arrived and was refused — see
// isClientDecodeRejection.
describe("Protocol-entry attribution", () => {
const decodeRejection = new SdkError(
SdkErrorCode.InvalidResult,
"Invalid result for tools/list: ttlMs required",
);
it("attributes a decode rejection to its Protocol entry", async () => {
client.setStatus("connected");
client.listAllTools.mockRejectedValueOnce(decodeRejection);
await expect(state.refresh()).rejects.toThrow(decodeRejection);
expect(client.markResponseRejected).toHaveBeenCalledWith(
"tools/list",
decodeRejection.message,
);
});
it("attributes an unsupported resultType too", async () => {
client.setStatus("connected");
client.listAllTools.mockRejectedValueOnce(
new SdkError(SdkErrorCode.UnsupportedResultType, "unknown type"),
);
await expect(state.refresh()).rejects.toThrow();
expect(client.markResponseRejected).toHaveBeenCalledWith(
"tools/list",
"unknown type",
);
});
// The regression this guard exists for: no response frame arrived, so the
// last-answered id still points at an EARLIER successful call. Marking it
// would stamp "Rejected by the Inspector" onto an exchange that worked.
it("does NOT attribute a transport failure", async () => {
client.setStatus("connected");
client.listAllTools.mockRejectedValueOnce(
new SdkError(SdkErrorCode.ConnectionClosed, "Connection closed"),
);
await expect(state.refresh()).rejects.toThrow();
expect(client.markResponseRejected).not.toHaveBeenCalled();
});
it("does NOT attribute a request timeout", async () => {
client.setStatus("connected");
client.listAllTools.mockRejectedValueOnce(
new SdkError(SdkErrorCode.RequestTimeout, "Request timed out"),
);
await expect(state.refresh()).rejects.toThrow();
expect(client.markResponseRejected).not.toHaveBeenCalled();
});
// A real response, so the id would be right — but the failure is the
// server's, and its entry already renders as an error from the error
// frame. Blaming the Inspector would misattribute the cause.
it("does NOT attribute a plain (non-SDK) error", async () => {
client.setStatus("connected");
client.listAllTools.mockRejectedValueOnce(boom);
await expect(state.refresh()).rejects.toThrow(boom);
expect(client.markResponseRejected).not.toHaveBeenCalled();
});
it("still records every failure as state, attributed or not", async () => {
client.setStatus("connected");
client.listAllTools.mockRejectedValueOnce(boom);
await expect(state.refresh()).rejects.toThrow(boom);
expect(state.getError()).toBe(boom);
expect(client.markResponseRejected).not.toHaveBeenCalled();
});
});
it("clears the error once a refresh succeeds", async () => {

46
core/mcp/state/managedListState.ts
View file

@ -17,6 +17,7 @@ import type {
CacheMode,
ServerCapabilities,
} from "@modelcontextprotocol/client";
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
import { isTerminalStatus } from "../types.js";
import { TypedEventTarget } from "../typedEventTarget.js";
@ -29,6 +30,38 @@ import { TypedEventTarget } from "../typedEventTarget.js";
*/
export const DEFAULT_LIST_CHANGED_DEBOUNCE_MS = 250;
/**
* Whether a failed fetch is the CLIENT refusing a response it received, rather
* than the request never producing one.
*
* This gates `markResponseRejected`, and the distinction is load-bearing. That
* correlation recovers the request id as "the last response received for this
* method", which is only the failing exchange when a response actually just
* arrived and was refused while decoding. A transport drop, a timeout, or an
* aborted request produces no response frame at all — the last-answered id then
* still points at some EARLIER, successful call, and marking it would stamp
* "Rejected by the Inspector" onto an exchange that succeeded. That is the very
* class of lie this issue exists to remove, so it must not be traded for
* another one.
*
* A server-sent JSON-RPC error is excluded for a different reason: it is a real
* response, so the id would be right, but the failure is the server's. Its
* entry already renders as an error from the error frame itself, and blaming
* the Inspector for it would misattribute the cause.
*
* `SdkErrorCode.InvalidResult` is exactly "a result arrived and failed
* validation for the negotiated era"; `UnsupportedResultType` is its sibling
* for a `resultType` the codec has no handling for. Both are decisions the
* client made about a frame in hand.
*/
function isClientDecodeRejection(err: unknown): boolean {
return (
SdkError.isInstance(err) &&
(err.code === SdkErrorCode.InvalidResult ||
err.code === SdkErrorCode.UnsupportedResultType)
);
}
/**
* Every managed-list event map carries the list-changed indicator event and the
* last-fetch error (#1953).
@ -310,11 +343,14 @@ export abstract class ManagedListState<
// Attribute the failure to the response it came from, so the Protocol
// entry stops rendering a rejected result as a clean success (#1953).
// Must happen in this catch, while the correlation window the client
// documents is still valid.
this.client?.markResponseRejected?.(
this.config.listMethod,
error.message,
);
// documents is still valid — and ONLY for a decode rejection, see
// `isClientDecodeRejection`.
if (isClientDecodeRejection(err)) {
this.client?.markResponseRejected?.(
this.config.listMethod,
error.message,
);
}
throw err;
}
this.setError(null);