From fad848037617195fb86a7ec88eb8582816b5a48c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 8 Aug 2026 17:26:36 -0400 Subject: [PATCH] fix: only attribute a decode rejection to a Protocol entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt --- .../mcp/state/managedPromptsState.test.ts | 3 +- .../managedResourceTemplatesState.test.ts | 3 +- .../mcp/state/managedResourcesState.test.ts | 3 +- .../core/mcp/state/managedToolsState.test.ts | 77 +++++++++++++++++-- core/mcp/state/managedListState.ts | 46 +++++++++-- 5 files changed, 117 insertions(+), 15 deletions(-) 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);