diff --git a/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts b/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts index fe33943e..939af2ef 100644 --- a/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts @@ -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); diff --git a/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts b/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts index bc865d7a..be88cd5f 100644 --- a/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts @@ -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); diff --git a/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts b/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts index 1af9d8e7..7944f3a4 100644 --- a/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts @@ -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); diff --git a/clients/web/src/test/core/mcp/state/managedToolsState.test.ts b/clients/web/src/test/core/mcp/state/managedToolsState.test.ts index 8e9e45db..8455dd74 100644 --- a/clients/web/src/test/core/mcp/state/managedToolsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedToolsState.test.ts @@ -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 () => { diff --git a/core/mcp/state/managedListState.ts b/core/mcp/state/managedListState.ts index 62b9fdfa..ea5128b5 100644 --- a/core/mcp/state/managedListState.ts +++ b/core/mcp/state/managedListState.ts @@ -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);