From 9c6e4aeb4e827937abbef489ac5cd154a8919d6a Mon Sep 17 00:00:00 2001 From: olaservo Date: Wed, 12 Aug 2026 08:57:44 -0700 Subject: [PATCH 01/30] fix: settle ink input-handler re-subscription after frame waits in TUI App tests Co-Authored-By: Claude Fable 5 --- clients/tui/__tests__/App.test.tsx | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index d70b9c27..9025b52e 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -552,9 +552,29 @@ async function press(r: RenderResult, keys: string[]) { */ const POLL_TRIES = 100; +/** + * One check-phase turn, queued BEHIND React's already-scheduled passive-effect + * flush. A frame observed by a poll predicate is written during React's + * COMMIT, but ink re-arms its useInput listeners in the passive-effect flush + * React schedules (via setImmediate in Node) during that same commit. Node's + * event loop runs the timers phase before the check phase, so a 25ms poll + * tick can observe the new frame and let the test write the next keypress + * BEFORE that flush has run — the key is then dispatched to the previous + * commit's stale useInput closures (where e.g. pendingStepUp is still null) + * and silently swallowed (#1942). Yielding one setImmediate turn after the + * predicate passes sequences the next stdin write after the flush (FIFO + * within the check queue), so "frame visible" once again implies "input + * handlers armed". + */ +const settleInputHandlers = () => + new Promise((resolve) => setImmediate(resolve)); + async function waitUntil(predicate: () => boolean, tries = POLL_TRIES) { for (let i = 0; i < tries; i++) { - if (predicate()) return; + if (predicate()) { + await settleInputHandlers(); + return; + } await tick(); } } From fe5b622fd143b37f94535eafaa19ab2f8972a6f8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 16:52:11 -0400 Subject: [PATCH 02/30] fix: salvage a list result's valid entries when one is malformed (#1909) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SDK validates a list result as a whole, so a single non-conforming entry rejected the entire response and the panel showed "Couldn't load resources" with zero items — the exact opposite of what a debugging tool should do with a server that is 90% right. Reported against a PHP server whose empty `annotations` object reaches the wire as `[]` rather than `{}`. `listAll*` now falls back to a lenient re-walk when — and only when — the SDK rejects a result it received (`InvalidResult` / `UnsupportedResultType`), keeps the entries that validate, and reports the rest. A conforming server takes the strict, cache-aware path exactly as before. Salvaging is not accepting: the dropped entries are named with their reason in a warning above the list, and the Protocol entry is still marked rejected, so the wire truth survives. When every entry validates, the original error is rethrown rather than swallowed — the rejection was about something the per-item pass cannot explain. `isClientDecodeRejection` moves to the new module and is now shared with managedListState, which had the only copy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall --- AGENTS.md | 6 +- clients/web/src/App.test.tsx | 1 + clients/web/src/App.tsx | 2 + .../MalformedItemsWarning.stories.tsx | 89 +++++ .../MalformedItemsWarning.test.tsx | 97 ++++++ .../MalformedItemsWarning.tsx | 99 ++++++ .../groups/PromptControls/PromptControls.tsx | 14 + .../ResourceControls/ResourceControls.tsx | 19 ++ .../groups/ToolControls/ToolControls.tsx | 14 + .../screens/PromptsScreen/PromptsScreen.tsx | 8 + .../ResourcesScreen/ResourcesScreen.tsx | 8 + .../screens/ToolsScreen/ToolsScreen.tsx | 8 + .../views/InspectorView/InspectorView.tsx | 10 + .../web/src/test/core/mcp/listSalvage.test.ts | 232 +++++++++++++ .../inspectorClient-malformed-list.test.ts | 320 ++++++++++++++++++ core/mcp/__tests__/fakeInspectorClient.ts | 13 + core/mcp/index.ts | 3 + core/mcp/inspectorClient.ts | 253 ++++++++++++-- core/mcp/inspectorClientEventTarget.ts | 7 + core/mcp/inspectorClientProtocol.ts | 4 + core/mcp/listSalvage.ts | 199 +++++++++++ core/mcp/state/managedListState.ts | 34 +- core/react/useInspectorClient.ts | 26 ++ 23 files changed, 1402 insertions(+), 64 deletions(-) create mode 100644 clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.stories.tsx create mode 100644 clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.test.tsx create mode 100644 clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.tsx create mode 100644 clients/web/src/test/core/mcp/listSalvage.test.ts create mode 100644 clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts create mode 100644 core/mcp/listSalvage.ts diff --git a/AGENTS.md b/AGENTS.md index 0baea459..a96c06da 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -60,7 +60,11 @@ v2/main/ │ ├── mcp/ # InspectorClient runtime + state stores │ │ # (modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, -│ │ # used by the raw-wire tasks/* channel — #1631) +│ │ # used by the raw-wire tasks/* channel — #1631; +│ │ # listSalvage.ts: per-item salvage for list results — +│ │ # keeps the valid entries when one is non-conforming +│ │ # instead of losing the whole list, and owns the +│ │ # shared isClientDecodeRejection predicate — #1909) │ │ ├── import/ # Config import strategies (#1348): client-config parsers │ │ │ # (Claude Desktop/Cursor/Cline/VS Code), registry │ │ │ # server.json parser, strategy registry + well-known diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 0869d0d5..8ca842ed 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -598,6 +598,7 @@ const DEFAULT_USE_INSPECTOR_CLIENT: ReturnType = { serverInfo: undefined, instructions: undefined, excludedTools: [], + malformedListItems: [], appRendererClient: null, connect: vi.fn().mockResolvedValue(undefined), disconnect: vi.fn().mockResolvedValue(undefined), diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 509fc079..b42a2e3b 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -990,6 +990,7 @@ function App() { protocolEra, discoverResult, excludedTools, + malformedListItems, lastError, } = useInspectorClient(inspectorClient); const { @@ -4376,6 +4377,7 @@ function App() { latencyMs={latencyMs} tools={tools} excludedTools={excludedTools} + malformedListItems={malformedListItems} prompts={prompts} resources={resources} resourceTemplates={resourceTemplates} diff --git a/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.stories.tsx b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.stories.tsx new file mode 100644 index 00000000..756e8667 --- /dev/null +++ b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.stories.tsx @@ -0,0 +1,89 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { MalformedItemsWarning } from "./MalformedItemsWarning"; + +const meta: Meta = { + title: "Elements/MalformedItemsWarning", + component: MalformedItemsWarning, +}; + +export default meta; +type Story = StoryObj; + +/** The failure this was built for: a PHP server whose empty `annotations` + * object reaches the wire as `[]` (#1909). */ +export const OneDroppedTemplate: Story = { + args: { + method: "resources/templates/list", + what: "resource templates", + items: [ + { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: "annotations: Invalid input: expected object, received array", + }, + ], + }, +}; + +export const SeveralDropped: Story = { + args: { + method: "tools/list", + what: "tools", + items: [ + { + method: "tools/list", + index: 0, + label: "get_weather", + reason: "inputSchema: Invalid input: expected object, received string", + }, + { + method: "tools/list", + index: 4, + label: "add", + reason: "name: Invalid input: expected string, received number", + }, + ], + }, +}; + +/** An entry too broken to carry a name is reported by its position. */ +export const UnlabeledEntry: Story = { + args: { + method: "prompts/list", + what: "prompts", + items: [ + { + method: "prompts/list", + index: 2, + reason: "Invalid input: expected object, received null", + }, + ], + }, +}; + +/** Entries belonging to another list method are ignored — the same set is + * passed to every panel, and each selects its own. */ +export const OtherMethodOnly: Story = { + args: { + method: "resources/templates/list", + what: "resource templates", + items: [ + { + method: "tools/list", + index: 0, + label: "get_weather", + reason: "inputSchema: Invalid input: expected object, received string", + }, + ], + }, +}; + +/** The resting state: a conforming server, nothing rendered. */ +export const Conforming: Story = { + args: { + method: "resources/templates/list", + what: "resource templates", + items: [], + }, +}; diff --git a/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.test.tsx b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.test.tsx new file mode 100644 index 00000000..ed8c03f0 --- /dev/null +++ b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.test.tsx @@ -0,0 +1,97 @@ +import { describe, it, expect } from "vitest"; +import { screen } from "@testing-library/react"; +import type { MalformedListItem } from "@inspector/core/mcp"; +import { renderWithMantine } from "../../../test/renderWithMantine"; +import { MalformedItemsWarning } from "./MalformedItemsWarning"; + +const TEMPLATE_ENTRY: MalformedListItem = { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: "annotations: Invalid input: expected object, received array", +}; + +const TOOL_ENTRY: MalformedListItem = { + method: "tools/list", + index: 0, + label: "get_weather", + reason: "inputSchema: Invalid input: expected object, received string", +}; + +describe("MalformedItemsWarning", () => { + it("names the dropped entry and why it was dropped", () => { + renderWithMantine( + , + ); + + expect(screen.getByText("1 malformed entry dropped")).toBeInTheDocument(); + expect(screen.getByText("array_annotations")).toBeInTheDocument(); + expect(screen.getByText(TEMPLATE_ENTRY.reason)).toBeInTheDocument(); + }); + + it("renders nothing when the server was conforming", () => { + renderWithMantine( + , + ); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + }); + + it("ignores entries belonging to another list method", () => { + // Every panel receives the whole set and selects its own — a tools failure + // must not surface above the templates list. + renderWithMantine( + , + ); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + expect(screen.queryByText("get_weather")).not.toBeInTheDocument(); + }); + + it("pluralizes the count and lists every entry", () => { + renderWithMantine( + , + ); + + expect(screen.getByText("2 malformed entries dropped")).toBeInTheDocument(); + expect(screen.getByText("get_weather")).toBeInTheDocument(); + expect(screen.getByText("add")).toBeInTheDocument(); + // The template entry belongs to the other panel. + expect(screen.queryByText("array_annotations")).not.toBeInTheDocument(); + }); + + it("falls back to the position when the entry had no usable name", () => { + renderWithMantine( + , + ); + expect(screen.getByText("entry 2")).toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.tsx b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.tsx new file mode 100644 index 00000000..26bc5d5c --- /dev/null +++ b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.tsx @@ -0,0 +1,99 @@ +import { Alert, Code, List, ScrollArea, Text } from "@mantine/core"; +import type { MalformedListItem } from "@inspector/core/mcp"; + +export interface MalformedItemsWarningProps { + /** + * Every malformed entry the client is currently reporting, across all list + * methods — pass the hook's set straight through; the component selects its + * own by {@link method}. + */ + items: MalformedListItem[]; + /** The list method this panel renders, e.g. `"resources/templates/list"`. */ + method: string; + /** What the entries are, for the alert title — e.g. "resource templates". */ + what: string; +} + +// Yellow, not red: unlike ListLoadError this is not a failure — the list DID +// load and is rendered below. It reports what the server sent that the +// Inspector could not accept. +const WarningAlert = Alert.withProps({ + color: "yellow", + variant: "light", +}); + +// The entry list stays compact; the reasons are the payload, so they get the +// monospace treatment while the labels stay in body type. +const EntryList = List.withProps({ + size: "xs", + spacing: 4, + withPadding: true, +}); + +const EntryReason = Code.withProps({ + variant: "wrapping", +}); + +// A server can be wrong about many entries at once; cap the height so a badly +// broken list can't push the list itself off the sidebar. +const EntryScroll = ScrollArea.withProps({ + mah: 180, + type: "auto", +}); + +const SummaryText = Text.withProps({ + size: "xs", + mb: 4, +}); + +// The entry's identity, emphasized against its monospaced reason. +const EntryLabel = Text.withProps({ + component: "span", + size: "xs", + fw: 600, +}); + +/** `label` when the entry had one, else its position in the response. */ +function describeEntry(item: MalformedListItem): string { + return item.label ?? `entry ${item.index}`; +} + +/** + * "Some entries were dropped" — the visible half of the #1909 salvage. + * + * The client keeps the valid entries of a list whose result failed schema + * validation, rather than losing the whole list to one bad entry. Dropping them + * silently would trade one lie for another, so what was dropped, and why, is + * reported here beside the list that rendered without them. + */ +export function MalformedItemsWarning({ + items, + method, + what, +}: MalformedItemsWarningProps) { + const mine = items.filter((item) => item.method === method); + if (mine.length === 0) return null; + + return ( + + + The server returned {what} that don't match the MCP schema. The + rest of the list is shown below. + + + + {mine.map((item) => ( + + {describeEntry(item)}{" "} + {item.reason} + + ))} + + + + ); +} diff --git a/clients/web/src/components/groups/PromptControls/PromptControls.tsx b/clients/web/src/components/groups/PromptControls/PromptControls.tsx index 2a2d5364..2fbb5633 100644 --- a/clients/web/src/components/groups/PromptControls/PromptControls.tsx +++ b/clients/web/src/components/groups/PromptControls/PromptControls.tsx @@ -3,6 +3,8 @@ import { ClearButton } from "../../elements/ClearButton/ClearButton"; import type { Prompt } from "@modelcontextprotocol/client"; import { ListChangedIndicator } from "../../elements/ListChangedIndicator/ListChangedIndicator"; import { ListLoadError } from "../../elements/ListLoadError/ListLoadError"; +import { MalformedItemsWarning } from "../../elements/MalformedItemsWarning/MalformedItemsWarning"; +import type { MalformedListItem } from "@inspector/core/mcp"; import { ListPaginationControls, type ListPaginationControlsProps, @@ -41,6 +43,12 @@ export interface PromptControlsProps { * A failed list load, surfaced above the list instead of leaving the panel * empty (which reads as "this server has none") (#1953). */ + /** + * Entries the client dropped from this list because they failed the MCP + * schema. Rendered as a warning above the list, which still shows the rest + * (#1909). + */ + malformedListItems?: MalformedListItem[]; loadError?: Error | null; /** Pagination controls (#1721). */ pagination: ListPaginationControlsProps; @@ -54,6 +62,7 @@ export function PromptControls({ searchText = "", listChanged, onRefreshList, + malformedListItems = [], loadError, pagination, onSearchChange, @@ -83,6 +92,11 @@ export function PromptControls({ /> + {filteredPrompts.map((prompt) => ( diff --git a/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx b/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx index adf5aa1a..6a319fb1 100644 --- a/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx +++ b/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx @@ -14,6 +14,8 @@ import { isModernEra } from "../../elements/EraBadge/eraUtils"; import { SubscriptionStreamBadge } from "../../elements/SubscriptionStreamBadge/SubscriptionStreamBadge"; import { ListChangedIndicator } from "../../elements/ListChangedIndicator/ListChangedIndicator"; import { ListLoadError } from "../../elements/ListLoadError/ListLoadError"; +import { MalformedItemsWarning } from "../../elements/MalformedItemsWarning/MalformedItemsWarning"; +import type { MalformedListItem } from "@inspector/core/mcp"; import { ListPaginationControls, type ListPaginationControlsProps, @@ -73,6 +75,12 @@ export interface ResourceControlsProps { * A failed list load, surfaced above the list instead of leaving the panel * empty (which reads as "this server has none") (#1953). */ + /** + * Entries the client dropped from `resources/list` or + * `resources/templates/list` because they failed the MCP schema. Rendered as + * a warning above the list, which still shows the rest (#1909). + */ + malformedListItems?: MalformedListItem[]; loadError?: Error | null; /** Pagination controls for the Resources list (#1721). */ pagination: ListPaginationControlsProps; @@ -117,6 +125,7 @@ export function ResourceControls({ openSections: controlledOpenSections, listChanged, onRefreshList, + malformedListItems = [], loadError, pagination, onSearchChange, @@ -248,6 +257,16 @@ export function ResourceControls({ what="resources" onRetry={onRefreshList} /> + + {/* Stays inline: Accordion is a compound, `multiple`-discriminated generic, so `.withProps({ multiple: true, ... })` loses its JSX call signature (same tooling limit as Box). */} diff --git a/clients/web/src/components/groups/ToolControls/ToolControls.tsx b/clients/web/src/components/groups/ToolControls/ToolControls.tsx index 4ad70d85..01ad1f04 100644 --- a/clients/web/src/components/groups/ToolControls/ToolControls.tsx +++ b/clients/web/src/components/groups/ToolControls/ToolControls.tsx @@ -15,6 +15,8 @@ import type { Tool } from "@modelcontextprotocol/client"; import type { ExcludedTool } from "@inspector/core/mcp/types.js"; import { ListChangedIndicator } from "../../elements/ListChangedIndicator/ListChangedIndicator"; import { ListLoadError } from "../../elements/ListLoadError/ListLoadError"; +import { MalformedItemsWarning } from "../../elements/MalformedItemsWarning/MalformedItemsWarning"; +import type { MalformedListItem } from "@inspector/core/mcp"; import { ListPaginationControls, type ListPaginationControlsProps, @@ -37,6 +39,12 @@ export interface ToolControlsProps { * A failed list load, surfaced above the list instead of leaving the panel * empty (which reads as "this server has none") (#1953). */ + /** + * Entries the client dropped from this list because they failed the MCP + * schema. Rendered as a warning above the list, which still shows the rest + * (#1909). + */ + malformedListItems?: MalformedListItem[]; loadError?: Error | null; /** Pagination controls (#1721). */ pagination: ListPaginationControlsProps; @@ -127,6 +135,7 @@ export function ToolControls({ searchText = "", listChanged, onRefreshList, + malformedListItems = [], loadError, pagination, onSearchChange, @@ -163,6 +172,11 @@ export function ToolControls({ /> + {filteredTools.map(({ tool, key }) => ( diff --git a/clients/web/src/components/screens/PromptsScreen/PromptsScreen.tsx b/clients/web/src/components/screens/PromptsScreen/PromptsScreen.tsx index c599ed47..8b491cc8 100644 --- a/clients/web/src/components/screens/PromptsScreen/PromptsScreen.tsx +++ b/clients/web/src/components/screens/PromptsScreen/PromptsScreen.tsx @@ -8,6 +8,7 @@ import { Stack, Text, } from "@mantine/core"; +import type { MalformedListItem } from "@inspector/core/mcp"; import type { GetPromptResult, Prompt } from "@modelcontextprotocol/client"; import { PromptControls } from "../../groups/PromptControls/PromptControls"; import type { ListPaginationControlsProps } from "../../elements/ListPaginationControls/ListPaginationControls"; @@ -34,6 +35,11 @@ export interface PromptsScreenProps { onUiChange: (next: PromptsUiState) => void; onRefreshList: () => void; /** A failed list load, rendered above the sidebar list (#1953). */ + /** + * Entries dropped from this screen's list result(s) as malformed; the list + * panel warns about them above the list (#1909). + */ + malformedListItems?: MalformedListItem[]; loadError?: Error | null; /** Pagination controls rendered in the sidebar (#1721). */ pagination: ListPaginationControlsProps; @@ -138,6 +144,7 @@ export function PromptsScreen({ completionsSupported, onUiChange, onRefreshList, + malformedListItems, loadError, pagination, onGetPrompt, @@ -277,6 +284,7 @@ export function PromptsScreen({ listChanged={listChanged} onRefreshList={onRefreshList} loadError={loadError} + malformedListItems={malformedListItems} pagination={pagination} onSearchChange={(value) => onUiChange({ ...ui, search: value })} onSelectPrompt={handleSelectPrompt} diff --git a/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.tsx b/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.tsx index a94f9169..15c4a838 100644 --- a/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.tsx +++ b/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.tsx @@ -8,6 +8,7 @@ import { Stack, Text, } from "@mantine/core"; +import type { MalformedListItem } from "@inspector/core/mcp"; import type { ProtocolEra, ReadResourceResult, @@ -58,6 +59,11 @@ export interface ResourcesScreenProps { onUiChange: (next: ResourcesUiState) => void; onRefreshList: () => void; /** A failed list load, rendered above the sidebar list (#1953). */ + /** + * Entries dropped from this screen's list result(s) as malformed; the list + * panel warns about them above the list (#1909). + */ + malformedListItems?: MalformedListItem[]; loadError?: Error | null; /** Pagination controls rendered in the sidebar (#1721). */ pagination: ListPaginationControlsProps; @@ -172,6 +178,7 @@ export function ResourcesScreen({ subscriptionsSupported = true, onUiChange, onRefreshList, + malformedListItems, loadError, pagination, onReadResource, @@ -329,6 +336,7 @@ export function ResourcesScreen({ listChanged={listChanged} onRefreshList={onRefreshList} loadError={loadError} + malformedListItems={malformedListItems} pagination={pagination} onSearchChange={(value) => onUiChange({ ...ui, search: value })} onOpenSectionsChange={(value) => diff --git a/clients/web/src/components/screens/ToolsScreen/ToolsScreen.tsx b/clients/web/src/components/screens/ToolsScreen/ToolsScreen.tsx index f6dd4ed3..a8afeb76 100644 --- a/clients/web/src/components/screens/ToolsScreen/ToolsScreen.tsx +++ b/clients/web/src/components/screens/ToolsScreen/ToolsScreen.tsx @@ -1,4 +1,5 @@ import { Card, Flex, Stack, Text } from "@mantine/core"; +import type { MalformedListItem } from "@inspector/core/mcp"; import type { CallToolResult, ReadResourceResult, @@ -50,6 +51,11 @@ export interface ToolsScreenProps { /** Tools the SDK excluded from `tools/list` for invalid `x-mcp-header` * annotations (SEP-2243), shown in the sidebar with the reason (#1632). */ excludedTools?: ExcludedTool[]; + /** + * Entries dropped from a list result as malformed, across every list method. + * Each list panel filters for its own and warns about them (#1909). + */ + malformedListItems?: MalformedListItem[]; callState?: ToolCallState; ui: ToolsUiState; listChanged: boolean; @@ -147,6 +153,7 @@ const EmptyState = Text.withProps({ export function ToolsScreen({ tools, excludedTools, + malformedListItems, callState, ui, listChanged, @@ -191,6 +198,7 @@ export function ToolsScreen({ { + it("is true for a result the client received and refused", () => { + const err = new SdkError(SdkErrorCode.InvalidResult, "bad result"); + expect(isClientDecodeRejection(err)).toBe(true); + }); + + it("is true for a result type the codec cannot handle", () => { + const err = new SdkError(SdkErrorCode.UnsupportedResultType, "unknown"); + expect(isClientDecodeRejection(err)).toBe(true); + }); + + it("is false for a failure that produced no result to salvage", () => { + // A transport drop / timeout: re-listing would just fail again, and there + // is no response frame to pick entries out of. + expect(isClientDecodeRejection(new Error("socket hang up"))).toBe(false); + expect( + isClientDecodeRejection( + new SdkError(SdkErrorCode.RequestTimeout, "timed out"), + ), + ).toBe(false); + }); +}); + +describe("LenientListPageSchema", () => { + it("keeps the primitive array unvalidated while typing the cursor", () => { + const parsed = LenientListPageSchema.parse({ + resourceTemplates: [{ name: "ok" }, { name: 42 }], + nextCursor: "page-2", + }); + expect(parsed.nextCursor).toBe("page-2"); + expect(rawItemsOf(parsed, "resourceTemplates")).toHaveLength(2); + }); + + it("rejects a page whose cursor is malformed", () => { + // The walk itself depends on `nextCursor`, so a bad one is not salvageable + // — the strict error should stand rather than a partial list being kept. + expect( + LenientListPageSchema.safeParse({ tools: [], nextCursor: 7 }).success, + ).toBe(false); + }); + + it("reads an absent or non-array items key as no entries", () => { + expect(rawItemsOf({}, "tools")).toEqual([]); + expect( + rawItemsOf( + LenientListPageSchema.parse({ tools: "not-a-list" }) as never, + "tools", + ), + ).toEqual([]); + }); +}); + +describe("labelForRawItem", () => { + it("prefers a name, then a uriTemplate, uri, or title", () => { + expect(labelForRawItem({ name: "get_weather" })).toBe("get_weather"); + expect(labelForRawItem({ uriTemplate: "file:///{path}" })).toBe( + "file:///{path}", + ); + expect(labelForRawItem({ uri: "test://one" })).toBe("test://one"); + expect(labelForRawItem({ title: "Titled" })).toBe("Titled"); + }); + + it("has no label for an entry too broken to carry one", () => { + expect(labelForRawItem(null)).toBeUndefined(); + expect(labelForRawItem("just a string")).toBeUndefined(); + expect(labelForRawItem(7)).toBeUndefined(); + expect(labelForRawItem({ name: "" })).toBeUndefined(); + expect(labelForRawItem({ name: 42 })).toBeUndefined(); + }); +}); + +describe("describeIssues", () => { + it("names the offending field so the reader doesn't have to hunt", () => { + const result = ItemSchema.safeParse({ name: "t", annotations: [] }); + expect(result.success).toBe(false); + if (result.success) return; + expect(describeIssues(result.error)).toMatch( + /^annotations: .*expected object/i, + ); + }); + + it("names the expected type even when zod's message omits it", () => { + // The browser build reports a bare "Invalid input" where Node spells out + // "expected object, received array" — without the expected type the warning + // says something is wrong but not what would be right. + const terse = new z.ZodError([ + { + code: "invalid_type", + expected: "object", + path: ["annotations"], + message: "Invalid input", + // Double cast: the terse-message shape is what zod's BROWSER build + // emits, and no parse run under Node can produce it — the issue has to + // be hand-built, and `$ZodIssue`'s union doesn't accept a literal. + } as unknown as z.core.$ZodIssue, + ]); + expect(describeIssues(terse)).toBe( + "annotations: Invalid input (expected object)", + ); + }); + + it("does not repeat an expected type the message already carries", () => { + const result = ItemSchema.safeParse({ name: "t", annotations: [] }); + expect(result.success).toBe(false); + if (result.success) return; + const described = describeIssues(result.error); + expect(described.match(/expected object/g)).toHaveLength(1); + }); + + it("omits the path when the failure is the entry itself", () => { + const result = ItemSchema.safeParse("not an object"); + expect(result.success).toBe(false); + if (result.success) return; + const described = describeIssues(result.error); + expect(described).not.toMatch(/^:/); + expect(described).toMatch(/expected object/i); + }); +}); + +describe("salvageListItems", () => { + it("keeps the valid entries and reports only the broken one", () => { + const { valid, malformed } = salvageListItems({ + method: "resources/templates/list", + items: [ + { name: "empty_annotations", annotations: {} }, + { name: "array_annotations", annotations: [] }, + { name: "full_annotations", annotations: { priority: 0.8 } }, + ], + schema: ItemSchema, + }); + + expect(valid.map((item) => item.name)).toEqual([ + "empty_annotations", + "full_annotations", + ]); + expect(malformed).toEqual([ + { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: expect.stringMatching(/^annotations: /), + }, + ]); + }); + + it("reports an unlabelable entry by its position", () => { + const { valid, malformed } = salvageListItems({ + method: "tools/list", + items: [null], + schema: ItemSchema, + }); + expect(valid).toEqual([]); + expect(malformed[0]).toMatchObject({ index: 0 }); + expect(malformed[0]).not.toHaveProperty("label"); + }); + + it("indexes against the aggregated list, not the page", () => { + // Page 2 of a paginated walk: an index that restarted per page would point + // at the wrong entry in the list the user is looking at. + const { malformed } = salvageListItems({ + method: "tools/list", + items: [{ name: "ok" }, { name: 5 }], + schema: ItemSchema, + startIndex: 10, + }); + expect(malformed[0]?.index).toBe(11); + }); + + it("salvages nothing from a wholly conforming page", () => { + const { valid, malformed } = salvageListItems({ + method: "prompts/list", + items: [{ name: "a" }, { name: "b" }], + schema: ItemSchema, + }); + expect(valid).toHaveLength(2); + expect(malformed).toEqual([]); + }); +}); + +describe("summarizeMalformed", () => { + it("names the single dropped entry and why", () => { + expect( + summarizeMalformed([ + { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: "annotations: expected object, received array", + }, + ]), + ).toBe( + "Dropped 1 malformed entry — array_annotations: annotations: expected object, received array", + ); + }); + + it("counts the rest when several failed, falling back to the index", () => { + const summary = summarizeMalformed([ + { method: "tools/list", index: 0, reason: "expected object" }, + { method: "tools/list", index: 3, reason: "expected object" }, + ]); + expect(summary).toContain("Dropped 2 malformed entries"); + expect(summary).toContain("index 0"); + expect(summary).toContain("(+1 more)"); + }); +}); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts new file mode 100644 index 00000000..d864e9b1 --- /dev/null +++ b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts @@ -0,0 +1,320 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { + createServer, + type IncomingMessage, + type Server, + type ServerResponse, +} from "node:http"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { MessageLogState } from "@inspector/core/mcp/state/index.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; + +/** + * Per-item list salvage against a real connection (#1909). + * + * The reported server was PHP, whose empty `annotations` object reaches the + * wire as `[]` rather than `{}` — one non-conforming entry, and the SDK + * rejected the whole `resources/templates/list` result, so every template + * vanished behind "Couldn't load resources". + * + * A hand-rolled JSON-RPC server is used rather than a composable test server + * because the SDK's own server cannot emit these shapes: they are exactly what + * its types forbid. The transport, the SDK client, and the salvage path are all + * real. + */ + +interface ListPage { + items: unknown[]; + nextCursor?: string; +} + +function jsonRpcResult(id: unknown, result: unknown) { + return { jsonrpc: "2.0", id, result }; +} + +/** + * Minimal streamable-HTTP MCP server that answers `initialize` and the list + * methods it was configured with, returning entries verbatim (malformed ones + * included). + */ +function startMalformedServer(initialPages: { + resourceTemplates?: ListPage[]; + tools?: ListPage[]; +}): Promise<{ + url: string; + stop: () => Promise; + calls: string[]; + /** Swap what the server serves next, to model a server that got fixed. */ + setPages: (next: { + resourceTemplates?: ListPage[]; + tools?: ListPage[]; + }) => void; +}> { + const calls: string[] = []; + let pages = initialPages; + + const pageFor = (key: "resourceTemplates" | "tools", cursor?: string) => { + const configured = pages[key] ?? []; + const index = cursor === undefined ? 0 : Number(cursor); + return configured[index] ?? { items: [] }; + }; + + const handler = async (req: IncomingMessage, res: ServerResponse) => { + const chunks: Buffer[] = []; + for await (const chunk of req) chunks.push(chunk as Buffer); + const raw = Buffer.concat(chunks).toString(); + // The SDK also issues bodyless requests (the SSE GET, the session DELETE on + // teardown). Answer them without pretending they're JSON-RPC. + if (raw.length === 0) { + res.writeHead(405).end(); + return; + } + const body = JSON.parse(raw) as { + id?: unknown; + method: string; + params?: { cursor?: string }; + }; + calls.push(body.method); + + // Notifications carry no id and get an empty 202. + if (body.id === undefined) { + res.writeHead(202).end(); + return; + } + + const send = (result: unknown) => { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify(jsonRpcResult(body.id, result))); + }; + + if (body.method === "initialize") { + send({ + protocolVersion: "2025-06-18", + capabilities: { resources: {}, tools: {} }, + serverInfo: { name: "malformed-list-server", version: "1.0.0" }, + }); + return; + } + if (body.method === "resources/templates/list") { + const page = pageFor("resourceTemplates", body.params?.cursor); + send({ + resourceTemplates: page.items, + ...(page.nextCursor !== undefined && { nextCursor: page.nextCursor }), + }); + return; + } + if (body.method === "tools/list") { + const page = pageFor("tools", body.params?.cursor); + send({ + tools: page.items, + ...(page.nextCursor !== undefined && { nextCursor: page.nextCursor }), + }); + return; + } + send({}); + }; + + const server: Server = createServer((req, res) => { + void handler(req, res); + }); + + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + /* v8 ignore next -- listen() on a fresh server always yields an AddressInfo */ + const port = typeof address === "object" && address ? address.port : 0; + resolve({ + url: `http://127.0.0.1:${port}/mcp`, + calls, + setPages: (next) => { + pages = next; + }, + stop: () => + new Promise((done) => { + server.close(() => done()); + }), + }); + }); + }); +} + +const VALID_TEMPLATE = { + name: "full_annotations", + uriTemplate: "annotated://full/{id}", + annotations: { audience: ["user"], priority: 0.8 }, +}; +// The #1909 shape: `[]` where the spec says object. +const PHP_EMPTY_ANNOTATIONS = { + name: "array_annotations", + uriTemplate: "annotated://array/{id}", + annotations: [], +}; +const EMPTY_OBJECT_ANNOTATIONS = { + name: "empty_annotations", + uriTemplate: "annotated://empty/{id}", + annotations: {}, +}; + +describe("InspectorClient list salvage (#1909)", () => { + let client: InspectorClient | null = null; + let stopServer: (() => Promise) | null = null; + + async function connectTo(url: string) { + client = new InspectorClient( + { type: "streamable-http", url }, + { environment: { transport: createTransportNode } }, + ); + await client.connect(); + return client; + } + + beforeEach(() => { + client = null; + stopServer = null; + }); + + afterEach(async () => { + try { + await client?.disconnect(); + } catch { + // Teardown only — the assertions already ran. + } + client = null; + await stopServer?.(); + stopServer = null; + }); + + it("keeps the valid templates and reports the malformed one", async () => { + const server = await startMalformedServer({ + resourceTemplates: [ + { + items: [ + EMPTY_OBJECT_ANNOTATIONS, + PHP_EMPTY_ANNOTATIONS, + VALID_TEMPLATE, + ], + }, + ], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + const { resourceTemplates } = await connected.listAllResourceTemplates(); + + // Before the fix this threw, and the panel showed zero templates. + expect(resourceTemplates.map((t) => t.name)).toEqual([ + "empty_annotations", + "full_annotations", + ]); + expect(connected.getMalformedListItems()).toEqual([ + { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: expect.stringMatching(/^annotations: /), + }, + ]); + }); + + it("accepts an empty annotations object without salvaging at all", async () => { + // `{}` is legal — every Annotations field is optional — so this must take + // the strict path and report nothing dropped. + const server = await startMalformedServer({ + resourceTemplates: [{ items: [EMPTY_OBJECT_ANNOTATIONS] }], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + const { resourceTemplates } = await connected.listAllResourceTemplates(); + expect(resourceTemplates).toHaveLength(1); + expect(connected.getMalformedListItems()).toEqual([]); + }); + + it("emits malformedListItemsChange so the UI can warn", async () => { + const server = await startMalformedServer({ + resourceTemplates: [{ items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE] }], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + const seen: unknown[] = []; + connected.addEventListener("malformedListItemsChange", (event) => { + seen.push(event.detail); + }); + await connected.listAllResourceTemplates(); + + expect(seen).toHaveLength(1); + expect(seen[0]).toMatchObject([{ label: "array_annotations" }]); + }); + + it("marks the Protocol entry rejected even though the list rendered", async () => { + const server = await startMalformedServer({ + resourceTemplates: [{ items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE] }], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + const log = new MessageLogState(connected); + + await connected.listAllResourceTemplates(); + + const rejected = log + .getMessages() + .filter((entry) => entry.clientError !== undefined); + expect(rejected.length).toBeGreaterThan(0); + expect(rejected.at(-1)?.clientError).toContain("Dropped 1 malformed entry"); + log.destroy(); + }); + + it("salvages across pages, indexing against the aggregate", async () => { + const server = await startMalformedServer({ + tools: [ + { + items: [{ name: "ok_one", inputSchema: { type: "object" } }], + nextCursor: "1", + }, + { items: [{ name: "broken", inputSchema: "not-a-schema" }] }, + ], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + const { tools } = await connected.listAllTools(); + expect(tools.map((t) => t.name)).toEqual(["ok_one"]); + expect(connected.getMalformedListItems()).toMatchObject([ + { method: "tools/list", index: 1, label: "broken" }, + ]); + }); + + it("clears a previous report once the server answers cleanly", async () => { + const server = await startMalformedServer({ + resourceTemplates: [{ items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE] }], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + await connected.listAllResourceTemplates(); + expect(connected.getMalformedListItems()).toHaveLength(1); + + // The server is fixed; the next refresh must clear the stale report rather + // than leaving a warning about entries that are no longer wrong. + server.setPages({ resourceTemplates: [{ items: [VALID_TEMPLATE] }] }); + await connected.listAllResourceTemplates({ cacheMode: "bypass" }); + expect(connected.getMalformedListItems()).toEqual([]); + }); + + it("rethrows when the rejection is not about any single entry", async () => { + // Every entry validates; the result is bad for another reason (a cursor of + // the wrong type). There is no per-item story, so the original error must + // survive rather than being swallowed into a partial list. + const server = await startMalformedServer({ + resourceTemplates: [ + { items: [VALID_TEMPLATE], nextCursor: 42 as unknown as string }, + ], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + await expect(connected.listAllResourceTemplates()).rejects.toThrow(); + expect(connected.getMalformedListItems()).toEqual([]); + }); +}); diff --git a/core/mcp/__tests__/fakeInspectorClient.ts b/core/mcp/__tests__/fakeInspectorClient.ts index 00e3ed16..728ef5f7 100644 --- a/core/mcp/__tests__/fakeInspectorClient.ts +++ b/core/mcp/__tests__/fakeInspectorClient.ts @@ -38,6 +38,7 @@ import type { ExcludedTool, } from "../types.js"; import { INACTIVE_SUBSCRIPTION_STREAM_STATE } from "../types.js"; +import type { MalformedListItem } from "../listSalvage.js"; import type { JsonValue } from "../../json/jsonUtils.js"; type ListResult = { @@ -282,6 +283,18 @@ export class FakeInspectorClient this.dispatchTypedEvent("excludedToolsChange", excluded); } + private malformedListItems: MalformedListItem[] = []; + + getMalformedListItems(): MalformedListItem[] { + return this.malformedListItems; + } + + /** Test helper: set the malformed-entry set and emit the change (#1909). */ + setMalformedListItems(malformed: MalformedListItem[]): void { + this.malformedListItems = malformed; + this.dispatchTypedEvent("malformedListItemsChange", malformed); + } + getDiscoverResult(): DiscoverResult | undefined { return this.discoverResult; } diff --git a/core/mcp/index.ts b/core/mcp/index.ts index f8e37cd3..e83f9a5c 100644 --- a/core/mcp/index.ts +++ b/core/mcp/index.ts @@ -42,6 +42,9 @@ export type { ToolCallInvocation, } from "./types.js"; +// Per-item list salvage (#1909) +export type { MalformedListItem } from "./listSalvage.js"; + // Re-export JSON utilities export type { JsonValue } from "../json/jsonUtils.js"; export { diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index b0b809f3..e37994d5 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -159,10 +159,26 @@ import { ListResourcesResultSchema, ListResourceTemplatesResultSchema, ListPromptsResultSchema, + // Per-primitive item schemas — the salvage fallback validates a list's + // entries one at a time with these when the whole-result parse rejects + // (#1909, see `listAllSalvaging`). + ToolSchema, + ResourceSchema, + ResourceTemplateSchema, + PromptSchema, } from "@modelcontextprotocol/core"; import type { ClientResult } from "@modelcontextprotocol/client"; import { AjvJsonSchemaValidator } from "@modelcontextprotocol/client/validators/ajv"; +import { z } from "zod/v4"; import { validateToolOutput } from "./toolOutputValidation.js"; +import { + LenientListPageSchema, + isClientDecodeRejection, + rawItemsOf, + salvageListItems, + summarizeMalformed, + type MalformedListItem, +} from "./listSalvage.js"; import { TasksListChangedNotificationSchema } from "./taskNotificationSchemas.js"; import { type JsonValue, @@ -407,6 +423,11 @@ export class InspectorClient extends InspectorClientEventTarget { // annotations (SEP-2243), recomputed on every aggregate tools refresh and // surfaced so the Tools tab can show why a tool vanished (#1632). private excludedTools: ExcludedTool[] = []; + // Entries the Inspector dropped from a list result because they failed the + // spec schema for that primitive, keyed by list method. Populated only by the + // salvage fallback in `listAllSalvaging` — empty against a conforming server + // (#1909). + private malformedListItems = new Map(); // The capabilities this Inspector client advertises to the server during the // initialize handshake. Built once in setupClient() and snapshotted here so // UI surfaces (Server Info modal) can display them without poking at the @@ -2211,6 +2232,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.protocolEra = undefined; this.discoverResult = undefined; this.excludedTools = []; + this.malformedListItems.clear(); // Read as "not opted in" while disconnected. This is no longer what stops // it leaking into the next connection — `resetSessionState()` re-derives it // at connect, so removing this would leak nothing (#1629). Note the web @@ -2230,6 +2252,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.dispatchTypedEvent("protocolEraChange", this.protocolEra); this.dispatchTypedEvent("discoverResultChange", this.discoverResult); this.dispatchTypedEvent("excludedToolsChange", this.excludedTools); + this.dispatchTypedEvent("malformedListItemsChange", []); } /** @@ -3126,12 +3149,20 @@ export class InspectorClient extends InspectorClientEventTarget { if (!this.client) { throw new Error("Client is not connected"); } - const response = await this.invokeMcpClient(() => - this.client!.listTools( - this.aggregateListParams(options?.metadata), - this.getCacheableRequestOptions(options?.cacheMode), - ), - ); + const tools = await this.listAllSalvaging({ + method: "tools/list", + itemsKey: "tools", + itemSchema: ToolSchema, + metadata: options?.metadata, + aggregate: async () => [ + ...( + await this.client!.listTools( + this.aggregateListParams(options?.metadata), + this.getCacheableRequestOptions(options?.cacheMode), + ) + ).tools, + ], + }); // Recompute the SEP-2243 excluded-tools set alongside the aggregate. The // SDK already filtered `response.tools`, so it can't tell us what it // dropped — {@link refreshExcludedTools} re-lists the RAW `tools/list` to @@ -3153,7 +3184,7 @@ export class InspectorClient extends InspectorClientEventTarget { "Excluded-tools walk failed; the SEP-2243 excluded list may be incomplete", ); }); - return { tools: [...response.tools] }; + return { tools }; } /** @@ -3187,6 +3218,146 @@ export class InspectorClient extends InspectorClientEventTarget { this.dispatchTypedEvent("responseRejected", { id, reason }); } + /** Every entry dropped from a list result as malformed, across methods (#1909). */ + getMalformedListItems(): MalformedListItem[] { + return [...this.malformedListItems.values()].flat(); + } + + /** + * Record (or clear) the malformed entries for one list method and notify. + * + * A no-op when the method had none and still has none, so the common case — + * every conforming refresh, of which there are many — costs no event and no + * re-render. + */ + private setMalformedListItems( + method: string, + malformed: MalformedListItem[], + ): void { + const previous = this.malformedListItems.get(method) ?? []; + if (previous.length === 0 && malformed.length === 0) return; + if (malformed.length === 0) this.malformedListItems.delete(method); + else this.malformedListItems.set(method, malformed); + this.dispatchTypedEvent( + "malformedListItemsChange", + this.getMalformedListItems(), + ); + } + + /** + * Run a list aggregate, falling back to a per-item salvage when the SDK + * rejects the result it received (#1909). + * + * The strict aggregate runs first and unchanged, so a conforming server pays + * nothing: no extra request, no extra parse, and the SDK's response cache + * still serves it. Only an `InvalidResult` rejection — a response in hand + * that failed validation — reaches {@link salvageList}; every other failure + * (transport drop, timeout, server-sent error) rethrows untouched, because + * re-listing would not produce anything to salvage. + */ + private async listAllSalvaging({ + method, + itemsKey, + itemSchema, + aggregate, + metadata, + }: { + method: string; + itemsKey: string; + itemSchema: z.ZodType; + aggregate: () => Promise; + metadata?: Record; + }): Promise { + try { + const items = await this.invokeMcpClient(aggregate, { method }); + this.setMalformedListItems(method, []); + return items; + } catch (err) { + if (!isClientDecodeRejection(err)) throw err; + return this.salvageList({ method, itemsKey, itemSchema, metadata, err }); + } + } + + /** + * Re-walk a list method with pagination validated but entries left raw, then + * validate each entry on its own — keeping the good ones and reporting the + * rest (#1909). + * + * Two deliberate properties: + * + * - **The original error wins when nothing is salvageable.** If every entry + * validates here, the strict rejection was about something else (a + * top-level field, an era codec decision), so there is no per-item story to + * tell and swallowing it would hide a real failure. Rethrow the original. + * - **A salvaged response is still a rejected response.** The Protocol entry + * is marked (#1953) with what was dropped, so the wire truth survives even + * though the list now renders. + * + * This walk is deliberately un-cached (it uses the raw per-page path), which + * is fine: it only runs against a server that just returned a non-conforming + * result. + */ + private async salvageList({ + method, + itemsKey, + itemSchema, + metadata, + err, + }: { + method: string; + itemsKey: string; + itemSchema: z.ZodType; + metadata?: Record; + err: unknown; + }): Promise { + const valid: T[] = []; + const malformed: MalformedListItem[] = []; + const seenCursors = new Set(); + let cursor: string | undefined; + do { + const effectiveMeta = this.mergeMeta(metadata); + const params = { + ...(effectiveMeta ? { _meta: effectiveMeta } : {}), + ...(cursor ? { cursor } : {}), + }; + const page = await this.invokeMcpClient( + () => + this.client!.request( + { method, params }, + LenientListPageSchema, + this.getRequestOptions(metadata?.progressToken), + ), + { method }, + ); + const salvaged = salvageListItems({ + method, + items: rawItemsOf(page, itemsKey), + schema: itemSchema, + startIndex: valid.length + malformed.length, + }); + valid.push(...salvaged.valid); + malformed.push(...salvaged.malformed); + cursor = page.nextCursor; + if (cursor !== undefined) { + /* v8 ignore next -- defensive: a spec-compliant server never repeats a cursor; this guards a non-converging server from an infinite walk (mirrors the SDK's drainList guard) */ + if (seenCursors.has(cursor)) break; + seenCursors.add(cursor); + } + } while (cursor !== undefined); + + // Nothing per-item was wrong, so the strict rejection was about something + // this fallback cannot explain — surface it rather than hide it. + if (malformed.length === 0) throw err; + + this.markResponseRejected(method, summarizeMalformed(malformed)); + this.setMalformedListItems(method, malformed); + this.logger.warn( + { method, malformed }, + "Dropped malformed entries from a list result; the rest of the list was kept", + ); + return valid; + } + /** The current SEP-2243 excluded-tools set (empty on legacy/stdio). */ getExcludedTools(): ExcludedTool[] { return this.excludedTools; @@ -4347,13 +4518,21 @@ export class InspectorClient extends InspectorClientEventTarget { if (!this.client) { throw new Error("Client is not connected"); } - const response = await this.invokeMcpClient(() => - this.client!.listResources( - this.aggregateListParams(options?.metadata), - this.getCacheableRequestOptions(options?.cacheMode), - ), - ); - return { resources: [...response.resources] }; + const resources = await this.listAllSalvaging({ + method: "resources/list", + itemsKey: "resources", + itemSchema: ResourceSchema, + metadata: options?.metadata, + aggregate: async () => [ + ...( + await this.client!.listResources( + this.aggregateListParams(options?.metadata), + this.getCacheableRequestOptions(options?.cacheMode), + ) + ).resources, + ], + }); + return { resources }; } /** @@ -4504,15 +4683,21 @@ export class InspectorClient extends InspectorClientEventTarget { if (!this.client) { throw new Error("Client is not connected"); } - const response = await this.invokeMcpClient( - () => - this.client!.listResourceTemplates( - this.aggregateListParams(options?.metadata), - this.getCacheableRequestOptions(options?.cacheMode), - ), - { method: "resources/templates/list" }, - ); - return { resourceTemplates: [...response.resourceTemplates] }; + const resourceTemplates = await this.listAllSalvaging({ + method: "resources/templates/list", + itemsKey: "resourceTemplates", + itemSchema: ResourceTemplateSchema, + metadata: options?.metadata, + aggregate: async () => [ + ...( + await this.client!.listResourceTemplates( + this.aggregateListParams(options?.metadata), + this.getCacheableRequestOptions(options?.cacheMode), + ) + ).resourceTemplates, + ], + }); + return { resourceTemplates }; } /** @@ -4559,13 +4744,21 @@ export class InspectorClient extends InspectorClientEventTarget { if (!this.client) { throw new Error("Client is not connected"); } - const response = await this.invokeMcpClient(() => - this.client!.listPrompts( - this.aggregateListParams(options?.metadata), - this.getCacheableRequestOptions(options?.cacheMode), - ), - ); - return { prompts: [...response.prompts] }; + const prompts = await this.listAllSalvaging({ + method: "prompts/list", + itemsKey: "prompts", + itemSchema: PromptSchema, + metadata: options?.metadata, + aggregate: async () => [ + ...( + await this.client!.listPrompts( + this.aggregateListParams(options?.metadata), + this.getCacheableRequestOptions(options?.cacheMode), + ) + ).prompts, + ], + }); + return { prompts }; } /** diff --git a/core/mcp/inspectorClientEventTarget.ts b/core/mcp/inspectorClientEventTarget.ts index d4edee16..572909c8 100644 --- a/core/mcp/inspectorClientEventTarget.ts +++ b/core/mcp/inspectorClientEventTarget.ts @@ -26,6 +26,7 @@ import type { ResourceSubscriptionStreamState, ExcludedTool, } from "./types.js"; +import type { MalformedListItem } from "./listSalvage.js"; import type { Tool, ServerCapabilities, @@ -62,6 +63,12 @@ export interface InspectorClientEventMap { * connections (which don't exclude) and before connect (#1632). */ excludedToolsChange: ExcludedTool[]; + /** + * Entries dropped from a list result because they failed the spec schema for + * their primitive, across every list method. Empty against a conforming + * server; a non-empty set means the list rendered without them (#1909). + */ + malformedListItemsChange: MalformedListItem[]; capabilitiesChange: ServerCapabilities | undefined; serverInfoChange: Implementation | undefined; instructionsChange: string | undefined; diff --git a/core/mcp/inspectorClientProtocol.ts b/core/mcp/inspectorClientProtocol.ts index 6a5322ea..756b560f 100644 --- a/core/mcp/inspectorClientProtocol.ts +++ b/core/mcp/inspectorClientProtocol.ts @@ -37,6 +37,7 @@ import type { Tool, } from "@modelcontextprotocol/client"; import type { JsonValue } from "../json/jsonUtils.js"; +import type { MalformedListItem } from "./listSalvage.js"; import type { InspectorClientEventTarget } from "./inspectorClientEventTarget.js"; import type { SamplingCreateMessage } from "./samplingCreateMessage.js"; import type { ElicitationCreateMessage } from "./elicitationCreateMessage.js"; @@ -65,6 +66,9 @@ export interface InspectorClientProtocol extends InspectorClientEventTarget { /** Tools excluded from `tools/list` for invalid `x-mcp-header` annotations * (SEP-2243); empty on legacy/stdio connections (#1632). */ getExcludedTools(): ExcludedTool[]; + /** Entries dropped from a list result as malformed, across every list method; + * empty against a conforming server (#1909). */ + getMalformedListItems(): MalformedListItem[]; getResourceSubscriptionStreamState(): ResourceSubscriptionStreamState; getDiscoverResult(): DiscoverResult | undefined; getServerSettings(): InspectorServerSettings | undefined; diff --git a/core/mcp/listSalvage.ts b/core/mcp/listSalvage.ts new file mode 100644 index 00000000..a144c2bd --- /dev/null +++ b/core/mcp/listSalvage.ts @@ -0,0 +1,199 @@ +/** + * Per-item salvage for paginated list results (#1909). + * + * The SDK validates a list result as a whole: one non-conforming element + * rejects the entire response, so a server with a single malformed entry + * appears to have NO tools/resources/templates/prompts at all. That is exactly + * backwards for a debugging tool — the one thing the user needs to see is which + * entry is wrong and why, and the other entries are still perfectly usable. + * + * A real report: a PHP server whose empty `annotations` object reaches the wire + * as `[]` (`json_encode` of an empty array, or of `(array) new stdClass()`) + * instead of `{}` made every resource template vanish behind "Couldn't load + * resources" (#1909). + * + * The strict path is left alone. `InspectorClient.listAll*` still calls the + * SDK's cache-aware aggregate first, and only falls back to a lenient re-walk + * when that rejects a result it received — so a conforming server pays nothing, + * and the fallback is a pure recovery path. + * + * Salvaging does NOT mean accepting: a salvaged entry is dropped from the list + * and reported as {@link MalformedListItem}, and the Protocol entry is still + * marked rejected. The Inspector keeps telling the truth about the response; it + * just stops throwing away the valid part of it. + */ + +import { z } from "zod/v4"; +import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; + +/** + * One entry the Inspector dropped from a list result because it failed the + * spec's schema for that primitive. + */ +export interface MalformedListItem { + /** The list method that carried it, e.g. `"resources/templates/list"`. */ + method: string; + /** + * Zero-based position in the aggregated list. An entry too broken to carry a + * name is still identifiable by where it sat in the response. + */ + index: number; + /** Best-effort label read off the raw entry, when it has a usable one. */ + label?: string; + /** Why it was rejected — the first validation issue, with its field path. */ + reason: string; +} + +/** + * Whether a failed fetch is the CLIENT refusing a response it received, rather + * than the request never producing one. + * + * This gates both `markResponseRejected` (#1953) and the salvage fallback + * (#1909), and the distinction is load-bearing for each. For the Protocol-entry + * marking: the 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. For the salvage fallback: re-walking the list only makes sense + * when there IS a result to salvage; re-issuing the request after a timeout + * would just double the wait before failing anyway. + * + * 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. + */ +export function isClientDecodeRejection(err: unknown): boolean { + return ( + SdkError.isInstance(err) && + (err.code === SdkErrorCode.InvalidResult || + err.code === SdkErrorCode.UnsupportedResultType) + ); +} + +/** + * A list page parsed with everything except pagination left unvalidated. + * + * `looseObject` keeps the unknown keys, so the primitive array (`tools`, + * `resources`, `resourceTemplates`, `prompts`) survives as `unknown` for + * per-item validation. Only `nextCursor` is typed, because the walk itself + * depends on it — if THAT is malformed the page is unusable and the strict + * error should stand. + */ +export const LenientListPageSchema = z.looseObject({ + nextCursor: z.string().optional(), +}); + +export type LenientListPage = z.infer; + +/** Read the primitive array out of a leniently-parsed page. */ +export function rawItemsOf(page: LenientListPage, itemsKey: string): unknown[] { + const value = (page as Record)[itemsKey]; + return Array.isArray(value) ? value : []; +} + +/** + * Best-effort human label for a raw entry: whichever identifying string field + * it happens to carry. Undefined when the entry is too broken to have one (a + * bare string, a number, a null) — the caller falls back to the index. + */ +export function labelForRawItem(value: unknown): string | undefined { + if (typeof value !== "object" || value === null) return undefined; + const record = value as Record; + for (const key of ["name", "uriTemplate", "uri", "title"]) { + const candidate = record[key]; + if (typeof candidate === "string" && candidate.length > 0) return candidate; + } + return undefined; +} + +/** + * One-line summary of why an entry failed: the first issue, prefixed with its + * field path so `annotations` is named rather than left for the reader to find + * in a wall of JSON. The path is relative to the entry, not the response, since + * the entry is what we're reporting on. + */ +export function describeIssues(error: z.ZodError): string { + const [issue] = error.issues; + /* v8 ignore next -- zod never produces a failed parse with zero issues; guards against an empty message if it ever did */ + if (!issue) return "did not match the expected shape"; + const path = issue.path.join("."); + const detail = expectedDetail(issue); + const message = detail ? `${issue.message} (${detail})` : issue.message; + return path.length > 0 ? `${path}: ${message}` : message; +} + +/** + * `expected object` — appended because the issue's own message cannot be relied + * on to carry it. zod's browser build reports a bare "Invalid input" where Node + * says "Invalid input: expected object, received array", and the expected type + * is the whole diagnostic: without it the warning says something is wrong but + * not what would be right. Skipped when the message already spells it out, so + * the Node text doesn't end up saying it twice. + */ +function expectedDetail(issue: z.core.$ZodIssue): string | undefined { + if (!("expected" in issue) || typeof issue.expected !== "string") { + return undefined; + } + const detail = `expected ${issue.expected}`; + return issue.message.includes(detail) ? undefined : detail; +} + +/** + * Validate a page's entries one at a time, keeping the ones that pass. + * + * `startIndex` is the running count across pages, so an index in the report + * refers to the aggregated list the user sees rather than resetting per page. + */ +export function salvageListItems({ + method, + items, + schema, + startIndex = 0, +}: { + method: string; + items: unknown[]; + schema: z.ZodType; + startIndex?: number; +}): { valid: T[]; malformed: MalformedListItem[] } { + const valid: T[] = []; + const malformed: MalformedListItem[] = []; + items.forEach((item, offset) => { + const parsed = schema.safeParse(item); + if (parsed.success) { + valid.push(parsed.data); + return; + } + const label = labelForRawItem(item); + malformed.push({ + method, + index: startIndex + offset, + ...(label !== undefined && { label }), + reason: describeIssues(parsed.error), + }); + }); + return { valid, malformed }; +} + +/** + * Summary line used for the Protocol entry's rejection reason, so the Network / + * Protocol view still says the response was non-conforming even though the list + * rendered (#1953 + #1909). + */ +export function summarizeMalformed(malformed: MalformedListItem[]): string { + const [first] = malformed; + /* v8 ignore next -- callers only summarize a non-empty set */ + if (!first) return ""; + const where = first.label ?? `index ${first.index}`; + const rest = malformed.length > 1 ? ` (+${malformed.length - 1} more)` : ""; + return `Dropped ${malformed.length} malformed ${ + malformed.length === 1 ? "entry" : "entries" + } — ${where}: ${first.reason}${rest}`; +} diff --git a/core/mcp/state/managedListState.ts b/core/mcp/state/managedListState.ts index ea5128b5..eaf51f5d 100644 --- a/core/mcp/state/managedListState.ts +++ b/core/mcp/state/managedListState.ts @@ -17,7 +17,7 @@ import type { CacheMode, ServerCapabilities, } from "@modelcontextprotocol/client"; -import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; +import { isClientDecodeRejection } from "../listSalvage.js"; import { isTerminalStatus } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; @@ -30,38 +30,6 @@ 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). diff --git a/core/react/useInspectorClient.ts b/core/react/useInspectorClient.ts index db0910c3..5e9de64a 100644 --- a/core/react/useInspectorClient.ts +++ b/core/react/useInspectorClient.ts @@ -11,6 +11,7 @@ import type { DiscoverResult, } from "@modelcontextprotocol/client"; import type { ExcludedTool } from "../mcp/types.js"; +import type { MalformedListItem } from "../mcp/listSalvage.js"; // Module-scope frozen object so the `?? EMPTY_CLIENT_CAPABILITIES` // fallback below doesn't return a fresh literal on every render — @@ -44,6 +45,12 @@ export interface UseInspectorClientResult { * connections and before connect (#1632). */ excludedTools: ExcludedTool[]; + /** + * Entries the Inspector dropped from a list result because they failed the + * spec schema for their primitive. Empty against a conforming server; a + * non-empty set means that list rendered without them (#1909). + */ + malformedListItems: MalformedListItem[]; /** * Message from the most recent mid-session transport failure (the client's * `error` event — stdio crash, SSE drop, HTTP 5xx). Stays set until the next @@ -97,6 +104,9 @@ export function useInspectorClient( const [excludedTools, setExcludedTools] = useState( inspectorClient?.getExcludedTools() ?? [], ); + const [malformedListItems, setMalformedListItems] = useState< + MalformedListItem[] + >(inspectorClient?.getMalformedListItems() ?? []); const [lastError, setLastError] = useState(undefined); useEffect(() => { @@ -109,6 +119,7 @@ export function useInspectorClient( setProtocolEra(undefined); setDiscoverResult(undefined); setExcludedTools([]); + setMalformedListItems([]); setLastError(undefined); return; } @@ -121,6 +132,7 @@ export function useInspectorClient( setProtocolEra(inspectorClient.getProtocolEra()); setDiscoverResult(inspectorClient.getDiscoverResult()); setExcludedTools(inspectorClient.getExcludedTools()); + setMalformedListItems(inspectorClient.getMalformedListItems()); setLastError(undefined); const onStatusChange = (event: TypedEvent<"statusChange">) => { @@ -156,6 +168,11 @@ export function useInspectorClient( ) => { setDiscoverResult(event.detail); }; + const onMalformedListItemsChange = ( + event: TypedEvent<"malformedListItemsChange">, + ) => { + setMalformedListItems(event.detail); + }; const onExcludedToolsChange = ( event: TypedEvent<"excludedToolsChange">, ) => { @@ -186,6 +203,10 @@ export function useInspectorClient( "excludedToolsChange", onExcludedToolsChange, ); + inspectorClient.addEventListener( + "malformedListItemsChange", + onMalformedListItemsChange, + ); return () => { inspectorClient.removeEventListener("statusChange", onStatusChange); @@ -218,6 +239,10 @@ export function useInspectorClient( "excludedToolsChange", onExcludedToolsChange, ); + inspectorClient.removeEventListener( + "malformedListItemsChange", + onMalformedListItemsChange, + ); }; }, [inspectorClient]); @@ -247,6 +272,7 @@ export function useInspectorClient( protocolEra, discoverResult, excludedTools, + malformedListItems, lastError, appRendererClient: inspectorClient?.getAppRendererClient() ?? null, connect, From 54c60881149affed72032c52ffa987439abdf098 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 17:30:25 -0400 Subject: [PATCH 03/30] fix: address Copilot review on the list-salvage path (#1909) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three real defects it caught, plus the tests it asked for: - The strict `tools/list` aggregate is filtered by the SDK for invalid `x-mcp-header` annotations (SEP-2243), but the salvage path was not: one ordinary malformed tool would trip the fallback and readmit every invalid-header tool beside it. Salvaged tools now go through the same scan. - `markResponseRejected` resolves "the last response received for this method", and the re-walk answers that same method — so the mark landed on a salvage page that succeeded while the actually-invalid response rendered clean. The correlation id is now captured before the re-walk. - The report was cleared only on an explicit `disconnect()`. The route out that tears down nothing (`onerror` with no `onclose`) carried it into the next connection, so it is now cleared on the start-clean `resetSessionState()` path as well. Fixing the first exposed two more, both found by the new test: - The modern (2026-07-28) codec validates a result against the spec schema before the caller's, so a lenient `client.request` is rejected exactly as the strict one was — there was no salvage on the modern leg at all. It now goes through the raw-wire channel that exists for the modern `tasks/*` methods. `withModernEnvelope` is split out of `withModernTaskEnvelope` so a list request doesn't claim the tasks extension the user may have disabled. - A malformed tool also blanked the SEP-2243 excluded set, leaving a tool that vanished for an invalid header with no explanation — #1909's own complaint one level down. The scan walk now tolerates a malformed sibling. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall --- .../core/react/useInspectorClient.test.tsx | 70 ++++++++ .../inspectorClient-malformed-list.test.ts | 130 +++++++++++++- core/mcp/inspectorClient.ts | 164 ++++++++++++++++-- 3 files changed, 340 insertions(+), 24 deletions(-) diff --git a/clients/web/src/test/core/react/useInspectorClient.test.tsx b/clients/web/src/test/core/react/useInspectorClient.test.tsx index 56287702..f24e3692 100644 --- a/clients/web/src/test/core/react/useInspectorClient.test.tsx +++ b/clients/web/src/test/core/react/useInspectorClient.test.tsx @@ -144,6 +144,76 @@ describe("useInspectorClient", () => { expect(result.current.excludedTools).toEqual([]); }); + it("subscribes to malformedListItemsChange and updates (#1909)", () => { + const client = new FakeInspectorClient(); + const { result } = renderHook(() => useInspectorClient(client)); + expect(result.current.malformedListItems).toEqual([]); + const malformed = [ + { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: "annotations: Invalid input (expected object)", + }, + ]; + act(() => { + client.setMalformedListItems(malformed); + }); + expect(result.current.malformedListItems).toEqual(malformed); + }); + + it("seeds malformedListItems from the client it is given (#1909)", () => { + // A client that already salvaged before this hook mounted (a screen opened + // after the connect-time list load) must show the report immediately. + const client = new FakeInspectorClient({ status: "connected" }); + const malformed = [ + { method: "tools/list", index: 0, reason: "name: expected string" }, + ]; + client.setMalformedListItems(malformed); + const { result } = renderHook(() => useInspectorClient(client)); + expect(result.current.malformedListItems).toEqual(malformed); + }); + + it("resets malformedListItems to [] when client becomes null (#1909)", () => { + const client = new FakeInspectorClient({ status: "connected" }); + client.setMalformedListItems([ + { method: "tools/list", index: 0, reason: "name: expected string" }, + ]); + const { result, rerender } = renderHook(({ c }) => useInspectorClient(c), { + initialProps: { c: client as FakeInspectorClient | null }, + }); + expect(result.current.malformedListItems.length).toBe(1); + rerender({ c: null }); + expect(result.current.malformedListItems).toEqual([]); + }); + + it("re-seeds malformedListItems on a client swap, not the old server's (#1909)", () => { + const first = new FakeInspectorClient({ status: "connected" }); + first.setMalformedListItems([ + { method: "tools/list", index: 0, reason: "name: expected string" }, + ]); + const second = new FakeInspectorClient({ status: "connected" }); + const { result, rerender } = renderHook(({ c }) => useInspectorClient(c), { + initialProps: { c: first as FakeInspectorClient }, + }); + expect(result.current.malformedListItems).toHaveLength(1); + rerender({ c: second }); + expect(result.current.malformedListItems).toEqual([]); + }); + + it("stops listening for malformedListItemsChange after unmount (#1909)", () => { + const client = new FakeInspectorClient({ status: "connected" }); + const { result, unmount } = renderHook(() => useInspectorClient(client)); + unmount(); + act(() => { + client.setMalformedListItems([ + { method: "tools/list", index: 0, reason: "name: expected string" }, + ]); + }); + // A listener left attached would update state on an unmounted tree. + expect(result.current.malformedListItems).toEqual([]); + }); + it("resets protocolEra / discoverResult to defaults when client becomes null", () => { const client = new FakeInspectorClient({ status: "connected", diff --git a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts index d864e9b1..db04bb13 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts @@ -8,6 +8,7 @@ import { import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import { MessageLogState } from "@inspector/core/mcp/state/index.js"; import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js"; /** * Per-item list salvage against a real connection (#1909). @@ -23,6 +24,8 @@ import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; * real. */ +const MODERN_VERSION = "2026-07-28"; + interface ListPage { items: unknown[]; nextCursor?: string; @@ -37,10 +40,14 @@ function jsonRpcResult(id: unknown, result: unknown) { * methods it was configured with, returning entries verbatim (malformed ones * included). */ -function startMalformedServer(initialPages: { - resourceTemplates?: ListPage[]; - tools?: ListPage[]; -}): Promise<{ +function startMalformedServer( + initialPages: { + resourceTemplates?: ListPage[]; + tools?: ListPage[]; + }, + /** Negotiate the modern era, which is what turns the SEP-2243 gate on. */ + modern = false, +): Promise<{ url: string; stop: () => Promise; calls: string[]; @@ -82,14 +89,41 @@ function startMalformedServer(initialPages: { return; } - const send = (result: unknown) => { + const send = (result: Record) => { res.writeHead(200, { "Content-Type": "application/json" }); - res.end(JSON.stringify(jsonRpcResult(body.id, result))); + // The 2026-07-28 codec requires `resultType` on every result; the legacy + // leg ignores it. + res.end( + JSON.stringify( + jsonRpcResult( + body.id, + modern + ? { + resultType: "complete", + // The modern leg also requires the cache hints on a list + // result; `ttlMs: 0` means "don't cache", which keeps each + // call in these tests a real round trip. + ttlMs: 0, + cacheScope: "public", + ...result, + } + : result, + ), + ), + ); }; if (body.method === "initialize") { send({ - protocolVersion: "2025-06-18", + protocolVersion: modern ? MODERN_VERSION : "2025-06-18", + capabilities: { resources: {}, tools: {} }, + serverInfo: { name: "malformed-list-server", version: "1.0.0" }, + }); + return; + } + if (body.method === "server/discover") { + send({ + supportedVersions: [MODERN_VERSION], capabilities: { resources: {}, tools: {} }, serverInfo: { name: "malformed-list-server", version: "1.0.0" }, }); @@ -159,10 +193,13 @@ describe("InspectorClient list salvage (#1909)", () => { let client: InspectorClient | null = null; let stopServer: (() => Promise) | null = null; - async function connectTo(url: string) { + async function connectTo(url: string, era?: "legacy" | "modern") { client = new InspectorClient( { type: "streamable-http", url }, - { environment: { transport: createTransportNode } }, + { + environment: { transport: createTransportNode }, + ...(era ? { versionNegotiation: eraToVersionNegotiation(era) } : {}), + }, ); await client.connect(); return client; @@ -247,6 +284,37 @@ describe("InspectorClient list salvage (#1909)", () => { expect(seen[0]).toMatchObject([{ label: "array_annotations" }]); }); + it("marks the ORIGINAL rejected response, not a salvage page", async () => { + // The re-walk answers the same method again, so a mark taken afterwards + // would land on a page that succeeded and leave the invalid response + // rendering clean — the exact lie #1953 exists to remove. + const server = await startMalformedServer({ + resourceTemplates: [{ items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE] }], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + const log = new MessageLogState(connected); + + await connected.listAllResourceTemplates(); + + const templateCalls = log + .getMessages() + .filter( + (entry) => + "method" in entry.message && + entry.message.method === "resources/templates/list", + ); + // Two exchanges: the strict aggregate that was rejected, then the re-walk. + expect(templateCalls.length).toBeGreaterThanOrEqual(2); + expect(templateCalls[0]?.clientError).toContain( + "Dropped 1 malformed entry", + ); + for (const later of templateCalls.slice(1)) { + expect(later.clientError).toBeUndefined(); + } + log.destroy(); + }); + it("marks the Protocol entry rejected even though the list rendered", async () => { const server = await startMalformedServer({ resourceTemplates: [{ items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE] }], @@ -302,6 +370,50 @@ describe("InspectorClient list salvage (#1909)", () => { expect(connected.getMalformedListItems()).toEqual([]); }); + it("does not readmit a SEP-2243-excluded tool while salvaging", async () => { + // The strict aggregate is filtered by the SDK, so the salvage path has to + // reapply the rule: otherwise one ordinary malformed tool anywhere in the + // list quietly readmits every invalid-header tool beside it, turning a + // rendering fix into a spec violation (#1632 + #1909). + const server = await startMalformedServer( + { + tools: [ + { + items: [ + { name: "ok_tool", inputSchema: { type: "object" } }, + // Malformed: trips the salvage fallback. + { name: "broken", inputSchema: "not-a-schema" }, + // Schema-valid, but its x-mcp-header is not an RFC 9110 token. + { + name: "invalid_header_tool", + inputSchema: { + type: "object", + properties: { + value: { + type: "string", + "x-mcp-header": "Bad Header", + }, + }, + }, + }, + ], + }, + ], + }, + true, + ); + stopServer = server.stop; + const connected = await connectTo(server.url, "modern"); + expect(connected.getProtocolEra()).toBe("modern"); + + const { tools } = await connected.listAllTools(); + expect(tools.map((t) => t.name)).toEqual(["ok_tool"]); + // Still reported as excluded rather than silently gone. + expect(connected.getExcludedTools().map((x) => x.tool.name)).toEqual([ + "invalid_header_tool", + ]); + }); + it("rethrows when the rejection is not about any single entry", async () => { // Every entry validates; the result is bad for another reason (a cursor of // the wrong type). There is no per-item story, so the original error must diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index e37994d5..785c0c35 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -1616,6 +1616,15 @@ export class InspectorClient extends InspectorClientEventTarget { // (#1953). this.outboundRequestMethods.clear(); this.lastAnsweredRequestByMethod.clear(); + // Per-session for the same reason: a report names entries of the PREVIOUS + // server's list. Cleared here as well as in `disconnect()` because the + // route out that tears down nothing (`onerror` with no `onclose`) would + // otherwise carry it into the next connection — indefinitely, if that + // server never lists the method again (#1909). + if (this.malformedListItems.size > 0) { + this.malformedListItems.clear(); + this.dispatchTypedEvent("malformedListItemsChange", []); + } for (const [, controller] of this.taskInputAbortControllers) { controller.abort(new Error("Connection ended")); } @@ -2372,8 +2381,6 @@ export class InspectorClient extends InspectorClientEventTarget { private withModernTaskEnvelope( params: Record, ): Record { - const existingMeta = - (params._meta as Record | undefined) ?? {}; const clientCapabilities = { ...this.clientCapabilities, // Force-stamp the tasks extension regardless of what the client @@ -2385,11 +2392,31 @@ export class InspectorClient extends InspectorClientEventTarget { [TASKS_EXTENSION_KEY]: {}, }, }; - // Use the NEGOTIATED protocol version so the envelope agrees with the - // `MCP-Protocol-Version` header the transport stamps from the same source — - // a future modern-family revision would negotiate a different string, and - // the two must not disagree. The raw channel only runs on a connected modern - // session, so this is always set; the constant is a defensive fallback. + return this.withModernEnvelope(params, clientCapabilities); + } + + /** + * Stamp the `_meta` envelope every raw-wire request needs on the modern leg: + * the negotiated protocol version, the client identity, and the client + * capabilities. + * + * Split out of {@link withModernTaskEnvelope} so the list-salvage re-walk + * (#1909) can reuse it WITHOUT that method's force-stamped tasks extension — + * a `tools/list` has no business claiming an extension the user may have + * deliberately turned off (#1738). + * + * The NEGOTIATED protocol version is used so the envelope agrees with the + * `MCP-Protocol-Version` header the transport stamps from the same source — a + * future modern-family revision would negotiate a different string, and the + * two must not disagree. The raw channel only runs on a connected modern + * session, so this is always set; the constant is a defensive fallback. + */ + private withModernEnvelope( + params: Record, + clientCapabilities: ClientCapabilities = this.clientCapabilities, + ): Record { + const existingMeta = + (params._meta as Record | undefined) ?? {}; /* v8 ignore start -- fallback only if getProtocolVersion() is unset, which can't happen on the connected modern session this runs on. Bracketed so the ignore is reflow-proof however prettier splits the statement. */ @@ -3154,6 +3181,7 @@ export class InspectorClient extends InspectorClientEventTarget { itemsKey: "tools", itemSchema: ToolSchema, metadata: options?.metadata, + finalize: (salvaged) => this.excludeInvalidXMcpHeaderTools(salvaged), aggregate: async () => [ ...( await this.client!.listTools( @@ -3261,12 +3289,19 @@ export class InspectorClient extends InspectorClientEventTarget { itemSchema, aggregate, metadata, + finalize, }: { method: string; itemsKey: string; itemSchema: z.ZodType; aggregate: () => Promise; metadata?: Record; + /** + * Filter applied to SALVAGED entries only, to reproduce a filter the SDK's + * strict aggregate applies for us. Without it the fallback would return a + * longer list than the strict path — see the `tools/list` caller. + */ + finalize?: (items: T[]) => T[]; }): Promise { try { const items = await this.invokeMcpClient(aggregate, { method }); @@ -3274,10 +3309,35 @@ export class InspectorClient extends InspectorClientEventTarget { return items; } catch (err) { if (!isClientDecodeRejection(err)) throw err; - return this.salvageList({ method, itemsKey, itemSchema, metadata, err }); + const salvaged = await this.salvageList({ + method, + itemsKey, + itemSchema, + metadata, + err, + }); + return finalize ? finalize(salvaged) : salvaged; } } + /** + * Drop the tools the SDK would have excluded from `tools/list` for invalid + * `x-mcp-header` annotations (SEP-2243). + * + * The strict aggregate is filtered by the SDK before we see it, so the + * salvage path has to reapply the same rule itself: otherwise one ordinary + * malformed tool anywhere in the list would trip the fallback and quietly + * readmit every invalid-header tool alongside it — turning a rendering fix + * into a spec violation. The dropped tools are still reported through + * `refreshExcludedTools`, which lists them with their reason. + */ + private excludeInvalidXMcpHeaderTools(tools: Tool[]): Tool[] { + if (!this.excludesInvalidXMcpHeaderTools()) return tools; + return tools.filter( + (tool) => scanXMcpHeaderDeclarations(tool.inputSchema).valid, + ); + } + /** * Re-walk a list method with pagination validated but entries left raw, then * validate each entry on its own — keeping the good ones and reporting the @@ -3310,6 +3370,13 @@ export class InspectorClient extends InspectorClientEventTarget { metadata?: Record; err: unknown; }): Promise { + // Capture the correlation id BEFORE re-walking. `markResponseRejected` + // resolves "the last response received for this method", and the re-walk + // below answers that same method one or more times — so marking afterwards + // would stamp the rejection onto a salvage page that succeeded and leave + // the actually-invalid response rendering as a clean success, which is the + // precise lie #1953 exists to remove. + const rejectedResponseId = this.lastAnsweredRequestByMethod.get(method); const valid: T[] = []; const malformed: MalformedListItem[] = []; const seenCursors = new Set(); @@ -3320,13 +3387,27 @@ export class InspectorClient extends InspectorClientEventTarget { ...(effectiveMeta ? { _meta: effectiveMeta } : {}), ...(cursor ? { cursor } : {}), }; + // The modern (2026-07-28) codec validates a result against the SPEC + // schema for the method before the caller's schema is consulted, so a + // lenient `client.request` is rejected exactly as the strict one was — + // there is no salvage without going around it. The raw-wire channel that + // exists for the modern `tasks/*` methods does precisely that: the frame + // goes straight through the transport (still logged for the Protocol / + // Network tabs) and only the caller's schema is applied. Legacy keeps the + // ordinary SDK path, which honors request options and `_meta` for us. const page = await this.invokeMcpClient( () => - this.client!.request( - { method, params }, - LenientListPageSchema, - this.getRequestOptions(metadata?.progressToken), - ), + this.isModernEra() + ? this.rawWireRequest( + method, + this.withModernEnvelope(params), + LenientListPageSchema, + ) + : this.client!.request( + { method, params }, + LenientListPageSchema, + this.getRequestOptions(metadata?.progressToken), + ), { method }, ); const salvaged = salvageListItems({ @@ -3349,7 +3430,12 @@ export class InspectorClient extends InspectorClientEventTarget { // this fallback cannot explain — surface it rather than hide it. if (malformed.length === 0) throw err; - this.markResponseRejected(method, summarizeMalformed(malformed)); + if (rejectedResponseId !== undefined) { + this.dispatchTypedEvent("responseRejected", { + id: rejectedResponseId, + reason: summarizeMalformed(malformed), + }); + } this.setMalformedListItems(method, malformed); this.logger.warn( { method, malformed }, @@ -3382,7 +3468,7 @@ export class InspectorClient extends InspectorClientEventTarget { const seenCursors = new Set(); let cursor: string | undefined; do { - const page = await this.listTools(cursor, metadata); + const page = await this.listToolsForScan(cursor, metadata); for (const tool of page.tools) { const scan = scanXMcpHeaderDeclarations(tool.inputSchema); if (!scan.valid) excluded.push({ tool, reason: scan.reason }); @@ -3400,6 +3486,54 @@ export class InspectorClient extends InspectorClientEventTarget { return excluded; } + /** + * One page of the RAW `tools/list` for the SEP-2243 scan, tolerating a + * malformed sibling entry (#1632 + #1909). + * + * The scan needs the unfiltered list, so it uses the strict single-page call + * — but that call rejects the whole page when any entry is non-conforming, + * which would blank the excluded set and leave a tool that vanished for an + * invalid header with no explanation at all. That is the same failure #1909 + * is about, one level down. So on a decode rejection the page is re-fetched + * leniently and scanned entry by entry; the malformed ones are not "excluded + * tools" and are reported through the salvage channel instead. + */ + private async listToolsForScan( + cursor: string | undefined, + metadata: Record | undefined, + ): Promise<{ tools: Tool[]; nextCursor?: string }> { + try { + return await this.listTools(cursor, metadata); + } catch (err) { + if (!isClientDecodeRejection(err)) throw err; + const effectiveMeta = this.mergeMeta(metadata); + const params = { + ...(effectiveMeta ? { _meta: effectiveMeta } : {}), + ...(cursor ? { cursor } : {}), + }; + const page = this.isModernEra() + ? await this.rawWireRequest( + "tools/list", + this.withModernEnvelope(params), + LenientListPageSchema, + ) + : await this.client!.request( + { method: "tools/list", params }, + LenientListPageSchema, + this.getRequestOptions(metadata?.progressToken), + ); + const { valid } = salvageListItems({ + method: "tools/list", + items: rawItemsOf(page, "tools"), + schema: ToolSchema, + }); + return { + tools: valid, + ...(page.nextCursor !== undefined && { nextCursor: page.nextCursor }), + }; + } + } + /** * Call a tool. Caller must provide the Tool (e.g. from a state manager). * @param tool The tool to call (use tool.name for the request) From 1889e04135b0631f1f0cd796b33a382e064218d1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 17:52:36 -0400 Subject: [PATCH 04/30] fix: address Copilot round 2 on the list-salvage path (#1909) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four findings taken (one visible, three in the suppressed block), plus the `excludedTools` sibling leak the round-1 reply had deferred: - `rawItemsOf` read a missing or non-array list member as an empty page, so a top-level violation (`{ tools: "invalid" }`) on a later page was discarded and the call returned a silently truncated list. It now returns `undefined` for "not a list at all", and the walk rethrows the original strict error. - A failure of the re-walk ITSELF (a malformed `nextCursor`, a connection lost mid-walk) escaped in place of the original error, whose response is the one `rejectedResponseId` was captured for. The walk is now wrapped: the original surfaces, the secondary is logged so it stays diagnosable. - The exclusion scan's lenient fallback discarded its malformed entries despite the comment claiming they were reported. Observable when the aggregate is served from cache: the salvage never runs, so nothing else reports them. The scan now returns them and `refreshExcludedTools` reports them — only ever adding, so a clean scan can't clear what the salvage path just wrote. - An empty-string cursor is a valid cursor, and `cursor ? {cursor} : {}` dropped it, re-requesting page one and duplicating its entries until the repeated-cursor guard fired. Both walks now test `!== undefined`. - `excludedTools` had the same session leak fixed for `malformedListItems` last round: cleared only on an explicit `disconnect()`, so the `onerror`-with-no- `onclose` route carried the previous server's set into the next connection. Cleared on the start-clean path too — safe because every `listAllTools` recomputes it, so this can only drop a stale set. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall --- .../web/src/test/core/mcp/listSalvage.test.ts | 14 +- .../inspectorClient-excluded-tools.test.ts | 22 +++ .../inspectorClient-malformed-list.test.ts | 74 +++++++- core/mcp/inspectorClient.ts | 163 ++++++++++++------ core/mcp/listSalvage.ts | 18 +- 5 files changed, 229 insertions(+), 62 deletions(-) diff --git a/clients/web/src/test/core/mcp/listSalvage.test.ts b/clients/web/src/test/core/mcp/listSalvage.test.ts index 08445051..f0fd7fdc 100644 --- a/clients/web/src/test/core/mcp/listSalvage.test.ts +++ b/clients/web/src/test/core/mcp/listSalvage.test.ts @@ -66,14 +66,16 @@ describe("LenientListPageSchema", () => { ).toBe(false); }); - it("reads an absent or non-array items key as no entries", () => { - expect(rawItemsOf({}, "tools")).toEqual([]); + it("distinguishes an empty page from one that is not a list at all", () => { + // Reading either as "no entries" would let a top-level violation pass as a + // silently truncated list. expect( - rawItemsOf( - LenientListPageSchema.parse({ tools: "not-a-list" }) as never, - "tools", - ), + rawItemsOf(LenientListPageSchema.parse({ tools: [] }), "tools"), ).toEqual([]); + expect(rawItemsOf({}, "tools")).toBeUndefined(); + expect( + rawItemsOf(LenientListPageSchema.parse({ tools: "not-a-list" }), "tools"), + ).toBeUndefined(); }); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts index fa3f9843..8f3bbf88 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts @@ -120,6 +120,28 @@ describe("excluded tools (SEP-2243 x-mcp-header)", () => { expect(excluded.map((x) => x.tool.name)).toEqual(["invalid_header_tool"]); }); + it("does not carry an excluded set into the next connection (#1909)", async () => { + // `resetSessionState()` is the start-clean path, and it is the ONLY one the + // route out that tears down nothing (`onerror` with no `onclose`) reaches. + // A set left behind names tools of the PREVIOUS server — and would persist + // indefinitely against a server that never lists tools again. + const started = await start({}); + const connected = await connect(started.url, "modern"); + await connected.listAllTools(); + expect(connected.getExcludedTools()).toHaveLength(1); + + const events: number[] = []; + connected.addEventListener("excludedToolsChange", (e) => { + events.push(e.detail.length); + }); + // Reconnecting runs the start-clean reset; assert before anything re-lists. + await connected.disconnect(); + await connected.connect(); + + expect(connected.getExcludedTools()).toEqual([]); + expect(events).toContain(0); + }); + it("reports no excluded tools on a legacy connection (no exclusion there)", async () => { const started = await start(undefined); const connected = await connect(started.url, "legacy"); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts index db04bb13..039dcbc0 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts @@ -51,6 +51,8 @@ function startMalformedServer( url: string; stop: () => Promise; calls: string[]; + /** Every `cursor` param the server was sent, in order. */ + cursors: (string | undefined)[]; /** Swap what the server serves next, to model a server that got fixed. */ setPages: (next: { resourceTemplates?: ListPage[]; @@ -60,10 +62,21 @@ function startMalformedServer( const calls: string[] = []; let pages = initialPages; + const cursors: (string | undefined)[] = []; + // Cursor string -> the page index it addresses, learned as pages are served. + // Modeling it this way (rather than parsing the cursor as a number) is what + // makes an EMPTY-STRING cursor meaningful: a client that drops it sends no + // cursor at all, lands back on page one, and duplicates its entries. + const cursorToIndex = new Map(); const pageFor = (key: "resourceTemplates" | "tools", cursor?: string) => { + cursors.push(cursor); const configured = pages[key] ?? []; - const index = cursor === undefined ? 0 : Number(cursor); - return configured[index] ?? { items: [] }; + const index = cursor === undefined ? 0 : (cursorToIndex.get(cursor) ?? 0); + const page = configured[index] ?? { items: [] }; + if (page.nextCursor !== undefined) { + cursorToIndex.set(page.nextCursor, index + 1); + } + return page; }; const handler = async (req: IncomingMessage, res: ServerResponse) => { @@ -160,6 +173,7 @@ function startMalformedServer( resolve({ url: `http://127.0.0.1:${port}/mcp`, calls, + cursors, setPages: (next) => { pages = next; }, @@ -414,6 +428,62 @@ describe("InspectorClient list salvage (#1909)", () => { ]); }); + it("surfaces the ORIGINAL error when the re-walk itself fails", async () => { + // A malformed `nextCursor` fails the lenient page schema too, so the walk + // throws its own error. The original is what the caller's list actually + // failed on — and what `rejectedResponseId` was captured for — so that is + // the one that must surface. + const server = await startMalformedServer({ + resourceTemplates: [ + { + items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE], + nextCursor: 42 as unknown as string, + }, + ], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + await expect(connected.listAllResourceTemplates()).rejects.toThrow( + /annotations/, + ); + expect(connected.getMalformedListItems()).toEqual([]); + }); + + it("keeps the strict error when a page is not a list at all", async () => { + // `{ resourceTemplates: "nope" }` is a top-level violation the per-item + // pass cannot explain; treating it as an empty page would return a + // silently truncated list and discard the error that was right about it. + const server = await startMalformedServer({ + resourceTemplates: [{ items: "nope" as unknown as unknown[] }], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + await expect(connected.listAllResourceTemplates()).rejects.toThrow(); + expect(connected.getMalformedListItems()).toEqual([]); + }); + + it("sends an empty-string cursor rather than re-fetching page one", async () => { + // "" is a valid cursor. A truthiness check would drop it, re-request page + // one, and duplicate its entries until the repeated-cursor guard fired. + const server = await startMalformedServer({ + resourceTemplates: [ + { items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE], nextCursor: "" }, + { items: [{ ...VALID_TEMPLATE, name: "page_two" }] }, + ], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + const { resourceTemplates } = await connected.listAllResourceTemplates(); + expect(resourceTemplates.map((t) => t.name)).toEqual([ + "full_annotations", + "page_two", + ]); + expect(server.cursors).toContain(""); + }); + it("rethrows when the rejection is not about any single entry", async () => { // Every entry validates; the result is bad for another reason (a cursor of // the wrong type). There is no per-item story, so the original error must diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 785c0c35..ac6d5f00 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -1616,15 +1616,23 @@ export class InspectorClient extends InspectorClientEventTarget { // (#1953). this.outboundRequestMethods.clear(); this.lastAnsweredRequestByMethod.clear(); - // Per-session for the same reason: a report names entries of the PREVIOUS + // Per-session for the same reason: both name entries of the PREVIOUS // server's list. Cleared here as well as in `disconnect()` because the // route out that tears down nothing (`onerror` with no `onclose`) would - // otherwise carry it into the next connection — indefinitely, if that - // server never lists the method again (#1909). + // otherwise carry them into the next connection — indefinitely, if that + // server never lists the method again. Safe to clear unconditionally at + // connect: `listAllTools` recomputes the excluded set on every aggregate + // and the salvage report on every list, so this can only drop a stale set, + // never a live one (#1909; the `excludedTools` half is the same latent bug + // from #1632, fixed here rather than left as a known sibling). if (this.malformedListItems.size > 0) { this.malformedListItems.clear(); this.dispatchTypedEvent("malformedListItemsChange", []); } + if (this.excludedTools.length > 0) { + this.excludedTools = []; + this.dispatchTypedEvent("excludedToolsChange", []); + } for (const [, controller] of this.taskInputAbortControllers) { controller.abort(new Error("Connection ended")); } @@ -3381,50 +3389,75 @@ export class InspectorClient extends InspectorClientEventTarget { const malformed: MalformedListItem[] = []; const seenCursors = new Set(); let cursor: string | undefined; - do { - const effectiveMeta = this.mergeMeta(metadata); - const params = { - ...(effectiveMeta ? { _meta: effectiveMeta } : {}), - ...(cursor ? { cursor } : {}), - }; - // The modern (2026-07-28) codec validates a result against the SPEC - // schema for the method before the caller's schema is consulted, so a - // lenient `client.request` is rejected exactly as the strict one was — - // there is no salvage without going around it. The raw-wire channel that - // exists for the modern `tasks/*` methods does precisely that: the frame - // goes straight through the transport (still logged for the Protocol / - // Network tabs) and only the caller's schema is applied. Legacy keeps the - // ordinary SDK path, which honors request options and `_meta` for us. - const page = await this.invokeMcpClient( - () => - this.isModernEra() - ? this.rawWireRequest( - method, - this.withModernEnvelope(params), - LenientListPageSchema, - ) - : this.client!.request( - { method, params }, - LenientListPageSchema, - this.getRequestOptions(metadata?.progressToken), - ), - { method }, - ); - const salvaged = salvageListItems({ - method, - items: rawItemsOf(page, itemsKey), - schema: itemSchema, - startIndex: valid.length + malformed.length, - }); - valid.push(...salvaged.valid); - malformed.push(...salvaged.malformed); - cursor = page.nextCursor; - if (cursor !== undefined) { - /* v8 ignore next -- defensive: a spec-compliant server never repeats a cursor; this guards a non-converging server from an infinite walk (mirrors the SDK's drainList guard) */ - if (seenCursors.has(cursor)) break; - seenCursors.add(cursor); + try { + do { + const effectiveMeta = this.mergeMeta(metadata); + const params = { + ...(effectiveMeta ? { _meta: effectiveMeta } : {}), + // `!== undefined`, not truthiness: "" is a valid cursor, and dropping it + // would re-request page one and duplicate its entries until the + // repeated-cursor guard below stopped the walk. + ...(cursor !== undefined && { cursor }), + }; + // The modern (2026-07-28) codec validates a result against the SPEC + // schema for the method before the caller's schema is consulted, so a + // lenient `client.request` is rejected exactly as the strict one was — + // there is no salvage without going around it. The raw-wire channel that + // exists for the modern `tasks/*` methods does precisely that: the frame + // goes straight through the transport (still logged for the Protocol / + // Network tabs) and only the caller's schema is applied. Legacy keeps the + // ordinary SDK path, which honors request options and `_meta` for us. + const page = await this.invokeMcpClient( + () => + this.isModernEra() + ? this.rawWireRequest( + method, + this.withModernEnvelope(params), + LenientListPageSchema, + ) + : this.client!.request( + { method, params }, + LenientListPageSchema, + this.getRequestOptions(metadata?.progressToken), + ), + { method }, + ); + const items = rawItemsOf(page, itemsKey); + // The page's list member is missing or isn't a list: a top-level + // violation, not a per-item one. Reading it as "no entries" would return + // a silently truncated list and discard the strict error that was right + // about it. + if (items === undefined) throw err; + const salvaged = salvageListItems({ + method, + items, + schema: itemSchema, + startIndex: valid.length + malformed.length, + }); + valid.push(...salvaged.valid); + malformed.push(...salvaged.malformed); + cursor = page.nextCursor; + if (cursor !== undefined) { + /* v8 ignore next -- defensive: a spec-compliant server never repeats a cursor; this guards a non-converging server from an infinite walk (mirrors the SDK's drainList guard) */ + if (seenCursors.has(cursor)) break; + seenCursors.add(cursor); + } + } while (cursor !== undefined); + } catch (walkErr) { + // The re-walk itself failed — its own decode rejection (a malformed + // `nextCursor`, an unsalvageable page), or the connection going away + // mid-walk. Either way the ORIGINAL error is the one to surface: it is + // what the caller's list actually failed on, and `rejectedResponseId` + // above was captured for that response, not this one. The secondary + // failure is logged so it stays diagnosable rather than vanishing. + if (walkErr !== err) { + this.logger.warn( + { err: walkErr, method }, + "Salvage re-walk failed; surfacing the original list error", + ); } - } while (cursor !== undefined); + throw err; + } // Nothing per-item was wrong, so the strict rejection was about something // this fallback cannot explain — surface it rather than hide it. @@ -3462,6 +3495,7 @@ export class InspectorClient extends InspectorClientEventTarget { metadata?: Record, ): Promise { const excluded: ExcludedTool[] = []; + const malformed: MalformedListItem[] = []; // Gated to connections that actually exclude; otherwise this is a pure // no-op (no round trip). The raw `listTools` below guards the connection. if (this.excludesInvalidXMcpHeaderTools()) { @@ -3469,6 +3503,13 @@ export class InspectorClient extends InspectorClientEventTarget { let cursor: string | undefined; do { const page = await this.listToolsForScan(cursor, metadata); + malformed.push( + ...page.malformed.map((entry) => ({ + ...entry, + // Index against the aggregate this walk is building, not the page. + index: entry.index + excluded.length + page.tools.length, + })), + ); for (const tool of page.tools) { const scan = scanXMcpHeaderDeclarations(tool.inputSchema); if (!scan.valid) excluded.push({ tool, reason: scan.reason }); @@ -3481,6 +3522,15 @@ export class InspectorClient extends InspectorClientEventTarget { } } while (cursor !== undefined); } + // Report what the scan itself had to drop. This walk is deliberately + // un-cached, so it can see a malformed tool the `listAllTools` aggregate + // never did — that aggregate may have been served from the response cache, + // in which case its salvage never ran and cleared the report instead. Only + // ever ADDS: a scan that finds nothing must not clear a report the salvage + // path just wrote from its own walk of the same list. + if (malformed.length > 0) { + this.setMalformedListItems("tools/list", malformed); + } this.excludedTools = excluded; this.dispatchTypedEvent("excludedToolsChange", excluded); return excluded; @@ -3501,15 +3551,21 @@ export class InspectorClient extends InspectorClientEventTarget { private async listToolsForScan( cursor: string | undefined, metadata: Record | undefined, - ): Promise<{ tools: Tool[]; nextCursor?: string }> { + ): Promise<{ + tools: Tool[]; + nextCursor?: string; + malformed: MalformedListItem[]; + }> { try { - return await this.listTools(cursor, metadata); + const page = await this.listTools(cursor, metadata); + return { ...page, malformed: [] }; } catch (err) { if (!isClientDecodeRejection(err)) throw err; const effectiveMeta = this.mergeMeta(metadata); const params = { ...(effectiveMeta ? { _meta: effectiveMeta } : {}), - ...(cursor ? { cursor } : {}), + // See `salvageList`: "" is a valid cursor. + ...(cursor !== undefined && { cursor }), }; const page = this.isModernEra() ? await this.rawWireRequest( @@ -3522,14 +3578,19 @@ export class InspectorClient extends InspectorClientEventTarget { LenientListPageSchema, this.getRequestOptions(metadata?.progressToken), ); - const { valid } = salvageListItems({ + const items = rawItemsOf(page, "tools"); + // Not a list at all — a top-level violation the per-item pass can't + // explain, so the strict error stands (mirrors `salvageList`). + if (items === undefined) throw err; + const { valid, malformed } = salvageListItems({ method: "tools/list", - items: rawItemsOf(page, "tools"), + items, schema: ToolSchema, }); return { tools: valid, ...(page.nextCursor !== undefined && { nextCursor: page.nextCursor }), + malformed, }; } } diff --git a/core/mcp/listSalvage.ts b/core/mcp/listSalvage.ts index a144c2bd..5d12f5da 100644 --- a/core/mcp/listSalvage.ts +++ b/core/mcp/listSalvage.ts @@ -93,10 +93,22 @@ export const LenientListPageSchema = z.looseObject({ export type LenientListPage = z.infer; -/** Read the primitive array out of a leniently-parsed page. */ -export function rawItemsOf(page: LenientListPage, itemsKey: string): unknown[] { +/** + * Read the primitive array out of a leniently-parsed page. + * + * Returns `undefined` — NOT an empty array — when the member is missing or is + * not an array, because those are different facts. An empty page is a normal + * server answer; a page whose `tools` is the string `"invalid"` is a top-level + * schema violation this fallback cannot explain, and reading it as "no entries" + * would let the caller return a silently truncated list while the strict error + * that was correct about it is discarded. + */ +export function rawItemsOf( + page: LenientListPage, + itemsKey: string, +): unknown[] | undefined { const value = (page as Record)[itemsKey]; - return Array.isArray(value) ? value : []; + return Array.isArray(value) ? value : undefined; } /** From aea3f6d4c2b9536b840ed35c3c6e79df015dec1d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 18:07:12 -0400 Subject: [PATCH 05/30] fix: address Copilot round 3 on the list-salvage path (#1909) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The visible review was clean; all five findings were in the suppressed block, two of them real defects. - The exclusion scan's aggregate offset was wrong twice over: `excluded.length` counts only invalid-header tools, and on page one it also folded in the current page's valid count. A reported index therefore pointed at a position that doesn't exist — worst for an unlabeled entry, whose index is all the user has. Replaced with a raw-entry offset advanced by each page's valid + malformed counts. - The scan's own refused response was never marked. It is a distinct exchange from the aggregate's, and the lenient re-fetch moves the method correlation off it, so its Protocol entry rendered as a clean success the client had in fact refused. Marked with the captured id, as `salvageList` already does. - Three double casts in the test fixture are gone: `ListPage` now models raw wire data (`items: unknown`, `nextCursor?: unknown`), which is what a server built to emit spec-forbidden shapes actually serves. Declaring the validated type and casting malformed values through it asserted the opposite of what these tests exist to check. The cursor map narrows to `string` at the one place a cursor addresses a page. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall --- .../inspectorClient-malformed-list.test.ts | 95 +++++++++++++++++-- core/mcp/inspectorClient.ts | 25 ++++- 2 files changed, 110 insertions(+), 10 deletions(-) diff --git a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts index 039dcbc0..e562d6f6 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts @@ -26,9 +26,18 @@ import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js"; const MODERN_VERSION = "2026-07-28"; +/** + * One page exactly as the server will put it on the wire. + * + * Both fields are `unknown` on purpose: this server exists to serve shapes the + * spec forbids (a non-array `items`, a numeric `nextCursor`), so the fixture + * models raw wire data rather than the validated type. Declaring the spec type + * and casting malformed values through it at each call site would assert the + * opposite of what these tests are for. + */ interface ListPage { - items: unknown[]; - nextCursor?: string; + items: unknown; + nextCursor?: unknown; } function jsonRpcResult(id: unknown, result: unknown) { @@ -73,7 +82,9 @@ function startMalformedServer( const configured = pages[key] ?? []; const index = cursor === undefined ? 0 : (cursorToIndex.get(cursor) ?? 0); const page = configured[index] ?? { items: [] }; - if (page.nextCursor !== undefined) { + // Only a string cursor can address a page; a malformed one is served + // verbatim so the client can reject it, but it maps to nothing. + if (typeof page.nextCursor === "string") { cursorToIndex.set(page.nextCursor, index + 1); } return page; @@ -437,7 +448,7 @@ describe("InspectorClient list salvage (#1909)", () => { resourceTemplates: [ { items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE], - nextCursor: 42 as unknown as string, + nextCursor: 42, }, ], }); @@ -455,7 +466,7 @@ describe("InspectorClient list salvage (#1909)", () => { // pass cannot explain; treating it as an empty page would return a // silently truncated list and discard the error that was right about it. const server = await startMalformedServer({ - resourceTemplates: [{ items: "nope" as unknown as unknown[] }], + resourceTemplates: [{ items: "nope" }], }); stopServer = server.stop; const connected = await connectTo(server.url); @@ -484,14 +495,82 @@ describe("InspectorClient list salvage (#1909)", () => { expect(server.cursors).toContain(""); }); + it("marks the exclusion scan's own refused response too", async () => { + // The scan is a separate exchange from the aggregate's, and its re-fetch + // moves the method correlation off it — so without marking it here its + // Protocol entry renders as a clean success the client actually refused. + const server = await startMalformedServer( + { + tools: [ + { + items: [ + { name: "ok_tool", inputSchema: { type: "object" } }, + { name: "broken", inputSchema: "not-a-schema" }, + ], + }, + ], + }, + true, + ); + stopServer = server.stop; + const connected = await connectTo(server.url, "modern"); + const log = new MessageLogState(connected); + + await connected.listAllTools(); + + const marked = log + .getMessages() + .filter( + (entry) => + "method" in entry.message && + entry.message.method === "tools/list" && + entry.clientError !== undefined, + ); + // Two refused responses: the aggregate's and the scan's. + expect(marked).toHaveLength(2); + for (const entry of marked) { + expect(entry.clientError).toContain("Dropped 1 malformed entry"); + } + log.destroy(); + }); + + it("reports scan-dropped tools at their position in the whole list", async () => { + // The exclusion scan walks pages of its own, so its report has to be + // offset by the RAW entries of PRIOR pages. An unlabeled entry's index is + // all the user has to go on, so a wrong offset points at nothing. + const server = await startMalformedServer( + { + tools: [ + { + items: [ + { name: "one", inputSchema: { type: "object" } }, + { name: "two", inputSchema: { type: "object" } }, + ], + nextCursor: "p2", + }, + // Page two, entry one (index 2 of the whole list) is unlabelable. + { items: [42, { name: "four", inputSchema: { type: "object" } }] }, + ], + }, + true, + ); + stopServer = server.stop; + const connected = await connectTo(server.url, "modern"); + + const { tools } = await connected.listAllTools(); + expect(tools.map((t) => t.name)).toEqual(["one", "two", "four"]); + const reported = connected.getMalformedListItems(); + expect(reported).toHaveLength(1); + expect(reported[0]?.index).toBe(2); + expect(reported[0]?.label).toBeUndefined(); + }); + it("rethrows when the rejection is not about any single entry", async () => { // Every entry validates; the result is bad for another reason (a cursor of // the wrong type). There is no per-item story, so the original error must // survive rather than being swallowed into a partial list. const server = await startMalformedServer({ - resourceTemplates: [ - { items: [VALID_TEMPLATE], nextCursor: 42 as unknown as string }, - ], + resourceTemplates: [{ items: [VALID_TEMPLATE], nextCursor: 42 }], }); stopServer = server.stop; const connected = await connectTo(server.url); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index ac6d5f00..df03a016 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -3496,6 +3496,9 @@ export class InspectorClient extends InspectorClientEventTarget { ): Promise { const excluded: ExcludedTool[] = []; const malformed: MalformedListItem[] = []; + // Raw entries seen so far, across pages — the frame the reported indices + // are relative to (see the mapping below). + let rawOffset = 0; // Gated to connections that actually exclude; otherwise this is a pure // no-op (no round trip). The raw `listTools` below guards the connection. if (this.excludesInvalidXMcpHeaderTools()) { @@ -3506,10 +3509,15 @@ export class InspectorClient extends InspectorClientEventTarget { malformed.push( ...page.malformed.map((entry) => ({ ...entry, - // Index against the aggregate this walk is building, not the page. - index: entry.index + excluded.length + page.tools.length, + // Offset by the RAW entries of PRIOR pages only. `excluded.length` + // would be wrong twice over: it counts just the invalid-header + // tools, and on page one it would also fold in this page's own + // valid count — putting an unlabeled entry, whose index is all the + // user has to go on, at a position that doesn't exist. + index: entry.index + rawOffset, })), ); + rawOffset += page.tools.length + page.malformed.length; for (const tool of page.tools) { const scan = scanXMcpHeaderDeclarations(tool.inputSchema); if (!scan.valid) excluded.push({ tool, reason: scan.reason }); @@ -3561,6 +3569,13 @@ export class InspectorClient extends InspectorClientEventTarget { return { ...page, malformed: [] }; } catch (err) { if (!isClientDecodeRejection(err)) throw err; + // Capture before the re-fetch, for the reason `salvageList` documents: + // this refused response is a distinct exchange from the aggregate's, and + // the re-fetch below moves the method's correlation off it. Without this + // the scan's Protocol entry renders as a clean success even though the + // client refused the result. + const rejectedResponseId = + this.lastAnsweredRequestByMethod.get("tools/list"); const effectiveMeta = this.mergeMeta(metadata); const params = { ...(effectiveMeta ? { _meta: effectiveMeta } : {}), @@ -3587,6 +3602,12 @@ export class InspectorClient extends InspectorClientEventTarget { items, schema: ToolSchema, }); + if (rejectedResponseId !== undefined && malformed.length > 0) { + this.dispatchTypedEvent("responseRejected", { + id: rejectedResponseId, + reason: summarizeMalformed(malformed), + }); + } return { tools: valid, ...(page.nextCursor !== undefined && { nextCursor: page.nextCursor }), From 573b14e14e80e831254f52f43aa5b75eabf7995d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 18:22:10 -0400 Subject: [PATCH 06/30] fix: address Copilot round 4 on the list-salvage path (#1909) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, both about the fallback accepting more than it should. - The lenient page schema validated pagination and nothing else, so a response carrying a malformed entry AND an unrelated top-level violation salvaged: the entry was reported and the other violation vanished with the strict error that caught it. The page schema is now DERIVED from the method's real result schema with only its item array relaxed (`lenientListPageSchema`), so every other field — `_meta`, `nextCursor`, anything a future revision adds — is still validated. The modern era envelope the raw-wire path bypasses (`resultType`, `ttlMs`, `cacheScope`) is checked explicitly for the same reason. Salvage is also narrowed to `InvalidResult`: `UnsupportedResultType` means the codec doesn't recognize what KIND of frame arrived, so it isn't a list with a bad entry and reporting an item-level reason would hide a protocol failure. Marking the Protocol entry still applies to both, hence two predicates. - On every rethrow path the re-walk had already answered the method, so the caller's own `markResponseRejected` (in `ManagedListState.refresh`) landed on the lenient page that succeeded and left the refused response clean. The correlation is restored before rethrowing — deleted when nothing had answered before, so the outer mark is a no-op rather than a lie. `nextCursorOf` / `rawItemsOf` narrow the page's fields instead of casting: the runtime-keyed `extend()` loses the shape, and asserting it back would be exactly the unjustified cast the last round removed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall --- .../web/src/test/core/mcp/listSalvage.test.ts | 102 +++++++++++++++--- .../inspectorClient-malformed-list.test.ts | 30 ++++++ core/mcp/inspectorClient.ts | 82 ++++++++++++-- core/mcp/listSalvage.ts | 74 +++++++++++-- 4 files changed, 251 insertions(+), 37 deletions(-) diff --git a/clients/web/src/test/core/mcp/listSalvage.test.ts b/clients/web/src/test/core/mcp/listSalvage.test.ts index f0fd7fdc..a117403b 100644 --- a/clients/web/src/test/core/mcp/listSalvage.test.ts +++ b/clients/web/src/test/core/mcp/listSalvage.test.ts @@ -2,14 +2,18 @@ import { describe, it, expect } from "vitest"; import { z } from "zod/v4"; import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; import { - LenientListPageSchema, + ModernResultEnvelopeSchema, describeIssues, isClientDecodeRejection, + isSalvageableRejection, labelForRawItem, + lenientListPageSchema, + nextCursorOf, rawItemsOf, salvageListItems, summarizeMalformed, } from "@inspector/core/mcp/listSalvage.js"; +import { ListToolsResultSchema } from "@modelcontextprotocol/core"; /** * Per-item list salvage (#1909). @@ -48,34 +52,102 @@ describe("isClientDecodeRejection", () => { }); }); -describe("LenientListPageSchema", () => { - it("keeps the primitive array unvalidated while typing the cursor", () => { - const parsed = LenientListPageSchema.parse({ - resourceTemplates: [{ name: "ok" }, { name: 42 }], +describe("lenientListPageSchema", () => { + const ToolsPageSchema = lenientListPageSchema(ListToolsResultSchema, "tools"); + + it("leaves the entries raw while still validating every other field", () => { + // The entries are this module's job to check one at a time; everything + // else is the real schema's, so a second violation can't ride along. + const parsed = ToolsPageSchema.parse({ + tools: [{ name: "ok" }, { name: 42 }], nextCursor: "page-2", }); - expect(parsed.nextCursor).toBe("page-2"); - expect(rawItemsOf(parsed, "resourceTemplates")).toHaveLength(2); + expect(nextCursorOf(parsed)).toBe("page-2"); + expect(rawItemsOf(parsed, "tools")).toHaveLength(2); }); it("rejects a page whose cursor is malformed", () => { // The walk itself depends on `nextCursor`, so a bad one is not salvageable // — the strict error should stand rather than a partial list being kept. expect( - LenientListPageSchema.safeParse({ tools: [], nextCursor: 7 }).success, + ToolsPageSchema.safeParse({ tools: [], nextCursor: 7 }).success, + ).toBe(false); + }); + + it("rejects a top-level violation that rides along with a bad entry", () => { + // A hand-rolled `{ nextCursor? }` schema would accept this, report the bad + // entry, and silently drop the `_meta` violation with the strict error. + expect( + ToolsPageSchema.safeParse({ tools: [{ name: 42 }], _meta: "nope" }) + .success, ).toBe(false); }); it("distinguishes an empty page from one that is not a list at all", () => { - // Reading either as "no entries" would let a top-level violation pass as a - // silently truncated list. - expect( - rawItemsOf(LenientListPageSchema.parse({ tools: [] }), "tools"), - ).toEqual([]); + expect(rawItemsOf(ToolsPageSchema.parse({ tools: [] }), "tools")).toEqual( + [], + ); expect(rawItemsOf({}, "tools")).toBeUndefined(); + expect(rawItemsOf({ tools: "not-a-list" }, "tools")).toBeUndefined(); + expect(rawItemsOf(null, "tools")).toBeUndefined(); + }); + + it("reads no cursor from a non-object page", () => { + expect(nextCursorOf(null)).toBeUndefined(); + expect(nextCursorOf({ nextCursor: 7 })).toBeUndefined(); + }); +}); + +describe("isSalvageableRejection", () => { + it("salvages an invalid result but not an unrecognized result type", () => { + // `UnsupportedResultType` means the codec doesn't know what KIND of frame + // arrived — not a list with a bad entry, so there is nothing to salvage + // and reporting an item-level reason would hide the real failure. expect( - rawItemsOf(LenientListPageSchema.parse({ tools: "not-a-list" }), "tools"), - ).toBeUndefined(); + isSalvageableRejection(new SdkError(SdkErrorCode.InvalidResult, "bad")), + ).toBe(true); + expect( + isSalvageableRejection( + new SdkError(SdkErrorCode.UnsupportedResultType, "unknown"), + ), + ).toBe(false); + // Still marks the Protocol entry, which is why both predicates exist. + expect( + isClientDecodeRejection( + new SdkError(SdkErrorCode.UnsupportedResultType, "unknown"), + ), + ).toBe(true); + }); +}); + +describe("ModernResultEnvelopeSchema", () => { + it("accepts a complete modern result with its cache hints", () => { + expect( + ModernResultEnvelopeSchema.safeParse({ + resultType: "complete", + ttlMs: 0, + cacheScope: "public", + tools: [], + }).success, + ).toBe(true); + }); + + it("rejects a modern result missing the envelope the codec would enforce", () => { + // The raw-wire salvage path goes around that codec, so a response missing + // its cache hints AND carrying a bad entry must not salvage. + expect( + ModernResultEnvelopeSchema.safeParse({ + resultType: "complete", + tools: [], + }).success, + ).toBe(false); + expect( + ModernResultEnvelopeSchema.safeParse({ + resultType: "task", + ttlMs: 0, + cacheScope: "public", + }).success, + ).toBe(false); }); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts index e562d6f6..b9b1f8a6 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts @@ -565,6 +565,36 @@ describe("InspectorClient list salvage (#1909)", () => { expect(reported[0]?.label).toBeUndefined(); }); + it("leaves the correlation on the original response when it rethrows", async () => { + // On a rethrow the re-walk has already answered the same method, so the + // caller's own `markResponseRejected` would land on the lenient page that + // succeeded. The correlation is restored so the outer mark still finds the + // response that was actually refused. + const server = await startMalformedServer({ + resourceTemplates: [{ items: [VALID_TEMPLATE], nextCursor: 42 }], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + const log = new MessageLogState(connected); + + await expect(connected.listAllResourceTemplates()).rejects.toThrow(); + // Simulates what ManagedListState.refresh() does in its catch. + connected.markResponseRejected("resources/templates/list", "outer mark"); + + const calls = log + .getMessages() + .filter( + (entry) => + "method" in entry.message && + entry.message.method === "resources/templates/list", + ); + expect(calls[0]?.clientError).toBe("outer mark"); + for (const later of calls.slice(1)) { + expect(later.clientError).toBeUndefined(); + } + log.destroy(); + }); + it("rethrows when the rejection is not about any single entry", async () => { // Every entry validates; the result is bad for another reason (a cursor of // the wrong type). There is no per-item story, so the original error must diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index df03a016..90a6c37d 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -172,8 +172,10 @@ import { AjvJsonSchemaValidator } from "@modelcontextprotocol/client/validators/ import { z } from "zod/v4"; import { validateToolOutput } from "./toolOutputValidation.js"; import { - LenientListPageSchema, - isClientDecodeRejection, + ModernResultEnvelopeSchema, + isSalvageableRejection, + nextCursorOf, + lenientListPageSchema, rawItemsOf, salvageListItems, summarizeMalformed, @@ -3187,6 +3189,7 @@ export class InspectorClient extends InspectorClientEventTarget { const tools = await this.listAllSalvaging({ method: "tools/list", itemsKey: "tools", + resultSchema: ListToolsResultSchema, itemSchema: ToolSchema, metadata: options?.metadata, finalize: (salvaged) => this.excludeInvalidXMcpHeaderTools(salvaged), @@ -3254,6 +3257,27 @@ export class InspectorClient extends InspectorClientEventTarget { this.dispatchTypedEvent("responseRejected", { id, reason }); } + /** + * Put the method's response correlation back where it was before a salvage + * re-walk, so a caller that goes on to mark the failure still marks the + * ORIGINAL rejected response. + * + * The re-walk answers the same method, so on any rethrow path the map points + * at a lenient page that succeeded. `ManagedListState.refresh()` catches the + * rethrown error and calls `markResponseRejected(method, ...)`, which would + * then stamp "Rejected by the Inspector" onto that page and leave the real + * one rendering clean — the same misattribution the capture inside + * `salvageList` exists to prevent, just one frame further out. Deleting when + * nothing had answered before makes the outer mark a no-op rather than a lie. + */ + private restoreResponseCorrelation( + method: string, + id: string | number | undefined, + ): void { + if (id === undefined) this.lastAnsweredRequestByMethod.delete(method); + else this.lastAnsweredRequestByMethod.set(method, id); + } + /** Every entry dropped from a list result as malformed, across methods (#1909). */ getMalformedListItems(): MalformedListItem[] { return [...this.malformedListItems.values()].flat(); @@ -3295,6 +3319,7 @@ export class InspectorClient extends InspectorClientEventTarget { method, itemsKey, itemSchema, + resultSchema, aggregate, metadata, finalize, @@ -3302,6 +3327,9 @@ export class InspectorClient extends InspectorClientEventTarget { method: string; itemsKey: string; itemSchema: z.ZodType; + /** The method's real result schema — the salvage page schema is derived + * from it so every field except the entries is still validated. */ + resultSchema: z.ZodObject; aggregate: () => Promise; metadata?: Record; /** @@ -3316,11 +3344,15 @@ export class InspectorClient extends InspectorClientEventTarget { this.setMalformedListItems(method, []); return items; } catch (err) { - if (!isClientDecodeRejection(err)) throw err; + // Deliberately narrower than the Protocol-marking predicate — see + // `isSalvageableRejection`: an unrecognized result KIND is not a list + // with a bad entry in it. + if (!isSalvageableRejection(err)) throw err; const salvaged = await this.salvageList({ method, itemsKey, itemSchema, + resultSchema, metadata, err, }); @@ -3369,15 +3401,18 @@ export class InspectorClient extends InspectorClientEventTarget { method, itemsKey, itemSchema, + resultSchema, metadata, err, }: { method: string; itemsKey: string; itemSchema: z.ZodType; + resultSchema: z.ZodObject; metadata?: Record; err: unknown; }): Promise { + const pageSchema = lenientListPageSchema(resultSchema, itemsKey); // Capture the correlation id BEFORE re-walking. `markResponseRejected` // resolves "the last response received for this method", and the re-walk // below answers that same method one or more times — so marking afterwards @@ -3413,15 +3448,23 @@ export class InspectorClient extends InspectorClientEventTarget { ? this.rawWireRequest( method, this.withModernEnvelope(params), - LenientListPageSchema, + pageSchema, ) : this.client!.request( { method, params }, - LenientListPageSchema, + pageSchema, this.getRequestOptions(metadata?.progressToken), ), { method }, ); + // The raw-wire path skips the era codec, so the envelope it would have + // enforced is checked here; a violation is not salvageable. + if ( + this.isModernEra() && + !ModernResultEnvelopeSchema.safeParse(page).success + ) { + throw err; + } const items = rawItemsOf(page, itemsKey); // The page's list member is missing or isn't a list: a top-level // violation, not a per-item one. Reading it as "no entries" would return @@ -3436,7 +3479,7 @@ export class InspectorClient extends InspectorClientEventTarget { }); valid.push(...salvaged.valid); malformed.push(...salvaged.malformed); - cursor = page.nextCursor; + cursor = nextCursorOf(page); if (cursor !== undefined) { /* v8 ignore next -- defensive: a spec-compliant server never repeats a cursor; this guards a non-converging server from an infinite walk (mirrors the SDK's drainList guard) */ if (seenCursors.has(cursor)) break; @@ -3456,12 +3499,16 @@ export class InspectorClient extends InspectorClientEventTarget { "Salvage re-walk failed; surfacing the original list error", ); } + this.restoreResponseCorrelation(method, rejectedResponseId); throw err; } // Nothing per-item was wrong, so the strict rejection was about something // this fallback cannot explain — surface it rather than hide it. - if (malformed.length === 0) throw err; + if (malformed.length === 0) { + this.restoreResponseCorrelation(method, rejectedResponseId); + throw err; + } if (rejectedResponseId !== undefined) { this.dispatchTypedEvent("responseRejected", { @@ -3568,7 +3615,7 @@ export class InspectorClient extends InspectorClientEventTarget { const page = await this.listTools(cursor, metadata); return { ...page, malformed: [] }; } catch (err) { - if (!isClientDecodeRejection(err)) throw err; + if (!isSalvageableRejection(err)) throw err; // Capture before the re-fetch, for the reason `salvageList` documents: // this refused response is a distinct exchange from the aggregate's, and // the re-fetch below moves the method's correlation off it. Without this @@ -3582,17 +3629,26 @@ export class InspectorClient extends InspectorClientEventTarget { // See `salvageList`: "" is a valid cursor. ...(cursor !== undefined && { cursor }), }; + const pageSchema = lenientListPageSchema(ListToolsResultSchema, "tools"); const page = this.isModernEra() ? await this.rawWireRequest( "tools/list", this.withModernEnvelope(params), - LenientListPageSchema, + pageSchema, ) : await this.client!.request( { method: "tools/list", params }, - LenientListPageSchema, + pageSchema, this.getRequestOptions(metadata?.progressToken), ); + // Same era-envelope check as `salvageList` — the raw-wire path bypasses + // the codec that would otherwise enforce it. + if ( + this.isModernEra() && + !ModernResultEnvelopeSchema.safeParse(page).success + ) { + throw err; + } const items = rawItemsOf(page, "tools"); // Not a list at all — a top-level violation the per-item pass can't // explain, so the strict error stands (mirrors `salvageList`). @@ -3608,9 +3664,10 @@ export class InspectorClient extends InspectorClientEventTarget { reason: summarizeMalformed(malformed), }); } + const nextCursor = nextCursorOf(page); return { tools: valid, - ...(page.nextCursor !== undefined && { nextCursor: page.nextCursor }), + ...(nextCursor !== undefined && { nextCursor }), malformed, }; } @@ -4737,6 +4794,7 @@ export class InspectorClient extends InspectorClientEventTarget { const resources = await this.listAllSalvaging({ method: "resources/list", itemsKey: "resources", + resultSchema: ListResourcesResultSchema, itemSchema: ResourceSchema, metadata: options?.metadata, aggregate: async () => [ @@ -4902,6 +4960,7 @@ export class InspectorClient extends InspectorClientEventTarget { const resourceTemplates = await this.listAllSalvaging({ method: "resources/templates/list", itemsKey: "resourceTemplates", + resultSchema: ListResourceTemplatesResultSchema, itemSchema: ResourceTemplateSchema, metadata: options?.metadata, aggregate: async () => [ @@ -4963,6 +5022,7 @@ export class InspectorClient extends InspectorClientEventTarget { const prompts = await this.listAllSalvaging({ method: "prompts/list", itemsKey: "prompts", + resultSchema: ListPromptsResultSchema, itemSchema: PromptSchema, metadata: options?.metadata, aggregate: async () => [ diff --git a/core/mcp/listSalvage.ts b/core/mcp/listSalvage.ts index 5d12f5da..37aeebbe 100644 --- a/core/mcp/listSalvage.ts +++ b/core/mcp/listSalvage.ts @@ -79,19 +79,70 @@ export function isClientDecodeRejection(err: unknown): boolean { } /** - * A list page parsed with everything except pagination left unvalidated. + * Whether a decode rejection is one the per-item fallback may attempt at all. * - * `looseObject` keeps the unknown keys, so the primitive array (`tools`, - * `resources`, `resourceTemplates`, `prompts`) survives as `unknown` for - * per-item validation. Only `nextCursor` is typed, because the walk itself - * depends on it — if THAT is malformed the page is unusable and the strict - * error should stand. + * Narrower than {@link isClientDecodeRejection} on purpose, and the difference + * is the point: `UnsupportedResultType` says the codec does not recognize what + * KIND of frame arrived, so it is not a list result with a bad entry in it — + * it is not a list result at all. Salvaging it would report an item-level + * reason for a protocol-level failure and hide the real one. Marking the + * Protocol entry rejected still applies to both, which is why the broader + * predicate stays. */ -export const LenientListPageSchema = z.looseObject({ - nextCursor: z.string().optional(), -}); +export function isSalvageableRejection(err: unknown): boolean { + return SdkError.isInstance(err) && err.code === SdkErrorCode.InvalidResult; +} -export type LenientListPage = z.infer; +/** + * The page schema for a salvage re-walk: the method's REAL result schema with + * only its item array relaxed to `unknown[]`. + * + * Deriving it from the spec schema rather than hand-rolling a permissive object + * is what keeps the fallback honest. A hand-rolled `{ nextCursor? }` validates + * pagination and nothing else, so a response carrying a malformed item AND an + * unrelated top-level violation would be accepted, the item reported, and the + * other violation silently dropped along with the strict error that caught it. + * Extending the real schema means every top-level field — `_meta`, `nextCursor`, + * anything a future revision adds — is still validated; only the entries, which + * this whole module exists to check one at a time, are left raw. + */ +export function lenientListPageSchema( + resultSchema: z.ZodObject, + itemsKey: string, +): z.ZodObject { + return resultSchema.extend({ [itemsKey]: z.array(z.unknown()) }); +} + +/** + * Read the pagination cursor off a leniently-parsed page. + * + * The page is typed as a plain object because `extend()` on a runtime-selected + * key loses the shape, so the fields are read through narrowing helpers rather + * than asserted with a cast. The schema has already validated that a present + * `nextCursor` is a string; this narrows the static type to match. + */ +export function nextCursorOf(page: unknown): string | undefined { + if (typeof page !== "object" || page === null) return undefined; + const value = (page as Record).nextCursor; + return typeof value === "string" ? value : undefined; +} + +/** + * The era envelope a modern (2026-07-28) result must carry, checked separately + * because it lives in the SDK's codec rather than in the spec result schema — + * and the modern salvage path deliberately goes around that codec. + * + * Without this, a modern response that is missing its cache hints AND has one + * bad entry would salvage: the entry gets reported and the envelope violation + * disappears. A failure here means "not salvageable", so the strict error + * stands — the safe direction if a future revision changes these rules, since + * the strict path remains the authority on what is valid. + */ +export const ModernResultEnvelopeSchema = z.looseObject({ + resultType: z.literal("complete"), + ttlMs: z.number(), + cacheScope: z.enum(["public", "private"]), +}); /** * Read the primitive array out of a leniently-parsed page. @@ -104,9 +155,10 @@ export type LenientListPage = z.infer; * that was correct about it is discarded. */ export function rawItemsOf( - page: LenientListPage, + page: unknown, itemsKey: string, ): unknown[] | undefined { + if (typeof page !== "object" || page === null) return undefined; const value = (page as Record)[itemsKey]; return Array.isArray(value) ? value : undefined; } From 07cafeedbcc28a6f07b12d5d0958398f9d4c6cc9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 18:33:41 -0400 Subject: [PATCH 07/30] feat: surface and decode the OAuth id_token in Connection Info (#2019) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an authorization server returns an `id_token` alongside the access token, the Inspector already persisted it and never showed it. Wire it through to Connection Info as a viewer. - Generalize `OAuthAccessTokenField` into `OAuthTokenField` (label + token), so the ID Token row reuses the existing copy control and "Decode JWT" toggle rather than duplicating them. Access-token behavior is unchanged. - `oauthDetailsFromConnectionState` now surfaces `tokens.id_token` as `idToken`, omitted entirely when absent so no empty row renders. - Docs: update the two `specification/` references to the renamed field. This is deliberately a viewer, not a flow: no OIDC login mode, no signature verification, and the `id_token` is never treated as a credential. The EMA path is untouched. `id_token` is already in `maskSecrets`' `JSON_SENSITIVE_KEYS`, so it stays masked in the Network tab — no gap. Signed-off-by: cliffhall --- .../ConnectionInfoContent.stories.tsx | 34 +++++++++++- .../ConnectionInfoContent.test.tsx | 54 +++++++++++++++++++ .../ConnectionInfoContent.tsx | 38 ++++++++----- ...ield.test.tsx => OAuthTokenField.test.tsx} | 44 +++++++++++---- ...cessTokenField.tsx => OAuthTokenField.tsx} | 35 +++++++----- .../oauthDetailsFromConnectionState.test.ts | 33 ++++++++++++ .../oauthDetailsFromConnectionState.ts | 5 ++ specification/v2_auth_ema.md | 2 +- specification/v2_ux.md | 2 +- 9 files changed, 211 insertions(+), 36 deletions(-) rename clients/web/src/components/groups/ConnectionInfoContent/{OAuthAccessTokenField.test.tsx => OAuthTokenField.test.tsx} (66%) rename clients/web/src/components/groups/ConnectionInfoContent/{OAuthAccessTokenField.tsx => OAuthTokenField.tsx} (74%) diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx index 3b1c919b..a634b9de 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx @@ -3,7 +3,7 @@ import type { InitializeResult, } from "@modelcontextprotocol/client"; import type { Meta, StoryObj } from "@storybook/react-vite"; -import { expect, within } from "storybook/test"; +import { expect, userEvent, within } from "storybook/test"; import { ConnectionInfoContent, SERVER_INFO_NOT_REPORTED_LABEL, @@ -161,3 +161,35 @@ export const WithOAuth: Story = { }, }, }; + +// An authorization server that also returned an `id_token` (#2019). Shown for +// inspection only — decoding it is display-only, and it is never used as a +// credential. +export const WithOAuthIdToken: Story = { + args: { + initializeResult: { + protocolVersion: "2025-03-26", + serverInfo: { name: "OIDC-fronted Server", version: "3.0.0" }, + capabilities: { tools: { listChanged: true } }, + }, + clientCapabilities: { roots: { listChanged: true } }, + transport: "streamable-http", + oauth: { + protocol: "standard", + authorized: true, + authUrl: "https://auth.example.com/oauth2/authorize", + scopes: ["openid", "email", "read"], + accessToken: "opaque-access-token", + // header {"alg":"none"} / payload {"sub":"user-42","email":"a@b.test"} + idToken: + "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ1c2VyLTQyIiwiZW1haWwiOiJhQGIudGVzdCJ9.", + }, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect(canvas.getByText("ID Token")).toBeInTheDocument(); + // Only the ID token is a JWT here, so exactly one decode toggle is offered. + await userEvent.click(canvas.getByRole("button", { name: "Decode JWT" })); + expect(canvas.getByText(/"sub": "user-42"/)).toBeInTheDocument(); + }, +}; diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index 9f9a8ea4..e33c63ad 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -473,6 +473,60 @@ describe("ConnectionInfoContent", () => { expect(screen.queryByText("Auth URL")).not.toBeInTheDocument(); expect(screen.queryByText("Scopes")).not.toBeInTheDocument(); expect(screen.queryByText("Access Token")).not.toBeInTheDocument(); + expect(screen.queryByText("ID Token")).not.toBeInTheDocument(); + }); + + it("renders the ID Token row when the token set carries one", () => { + renderWithMantine( + , + ); + expect(screen.getByText("Access Token")).toBeInTheDocument(); + expect(screen.getByText("ID Token")).toBeInTheDocument(); + expect(screen.getByText("id-token-456")).toBeInTheDocument(); + }); + + it("renders the ID Token row on its own when there is no access token", () => { + renderWithMantine( + , + ); + expect(screen.queryByText("Access Token")).not.toBeInTheDocument(); + expect(screen.getByText("ID Token")).toBeInTheDocument(); + }); + + it("omits the ID Token row when only an access token is present", () => { + renderWithMantine( + , + ); + expect(screen.getByText("Access Token")).toBeInTheDocument(); + expect(screen.queryByText("ID Token")).not.toBeInTheDocument(); }); it("does not render OAuth section when oauth prop is omitted", () => { diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx index a7cd53f2..dde264b6 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx @@ -24,7 +24,7 @@ import { import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; import { EraBadge } from "../../elements/EraBadge/EraBadge"; import { isModernEra } from "../../elements/EraBadge/eraUtils"; -import { OAuthAccessTokenField } from "./OAuthAccessTokenField"; +import { OAuthTokenField } from "./OAuthTokenField"; export interface OAuthDetails { protocol: "standard" | "ema"; @@ -34,6 +34,13 @@ export interface OAuthDetails { authUrl?: string; scopes?: string[]; accessToken?: string; + /** + * OIDC `id_token` from the stored token set, when the authorization server + * returned one. Shown for inspection only — the MCP authorization spec is + * plain OAuth 2.1 and the Inspector never treats this as a credential + * (#2019). Absent when the token set carries none, so no empty row renders. + */ + idToken?: string; /** EMA only — install-level IdP session for legs 1–2. */ idpSession?: "none" | "logged_in" | "expired"; } @@ -370,20 +377,27 @@ export function ConnectionInfoContent({ {formatScopes(oauth.scopes)} )} - {oauth.accessToken ? ( - - ) : ( - onClearOAuth && ( - - - {CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL} - - - ) + )} + {/* Viewer only — an `id_token` the AS happened to return, decoded + on request. It carries no clear action: clearing OAuth state is + one action for the whole token set, owned by the access-token + row (or the standalone button below when there is none). */} + {oauth.idToken && ( + + )} + {!oauth.accessToken && onClearOAuth && ( + + + {CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL} + + )} diff --git a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.test.tsx similarity index 66% rename from clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.test.tsx rename to clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.test.tsx index f1380f12..188c05a7 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.test.tsx @@ -2,13 +2,13 @@ import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import { renderWithMantine, screen } from "../../../test/renderWithMantine"; import { CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL } from "./ConnectionInfoContent"; -import { OAuthAccessTokenField } from "./OAuthAccessTokenField"; +import { OAuthTokenField } from "./OAuthTokenField"; const jwt = "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ1c2VyIn0."; -describe("OAuthAccessTokenField", () => { +describe("OAuthTokenField", () => { it("renders token with copy control beside the content", () => { - renderWithMantine(); + renderWithMantine(); expect(screen.getByText("Access Token")).toBeInTheDocument(); expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); expect(screen.getByText(/eyJzdWIiOiJ1c2VyIn0/)).toBeInTheDocument(); @@ -17,7 +17,7 @@ describe("OAuthAccessTokenField", () => { it("replaces raw token with decoded JSON and restores on toggle", async () => { const user = userEvent.setup(); - renderWithMantine(); + renderWithMantine(); expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); await user.click(screen.getByRole("button", { name: "Decode JWT" })); @@ -31,7 +31,10 @@ describe("OAuthAccessTokenField", () => { it("omits decode toggle for opaque tokens", () => { renderWithMantine( - , + , ); expect( screen.queryByRole("button", { name: "Decode JWT" }), @@ -46,7 +49,7 @@ describe("OAuthAccessTokenField", () => { configurable: true, }); - renderWithMantine(); + renderWithMantine(); await user.click(screen.getByRole("button", { name: "Decode JWT" })); await user.click(screen.getByRole("button", { name: "Copy" })); @@ -64,7 +67,7 @@ describe("OAuthAccessTokenField", () => { configurable: true, }); - renderWithMantine(); + renderWithMantine(); await user.click(screen.getByRole("button", { name: "Copy" })); expect(writeText).toHaveBeenCalledWith(jwt); @@ -74,8 +77,9 @@ describe("OAuthAccessTokenField", () => { const user = userEvent.setup(); const onClear = vi.fn(); renderWithMantine( - , @@ -87,4 +91,26 @@ describe("OAuthAccessTokenField", () => { ); expect(onClear).toHaveBeenCalledTimes(1); }); + + it("labels the row from the label prop and decodes an ID token", async () => { + const user = userEvent.setup(); + // header {"alg":"none"} / payload {"sub":"user"} — same shape an AS returns + // for an `id_token`; the decode path is identical to the access token's. + renderWithMantine(); + + expect(screen.getByText("ID Token")).toBeInTheDocument(); + expect(screen.queryByText("Access Token")).not.toBeInTheDocument(); + + await user.click(screen.getByRole("button", { name: "Decode JWT" })); + expect(screen.getByText(/"sub": "user"/)).toBeInTheDocument(); + }); + + it("omits the clear action when no handler is given", () => { + renderWithMantine(); + expect( + screen.queryByRole("button", { + name: CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL, + }), + ).not.toBeInTheDocument(); + }); }); diff --git a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.tsx b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx similarity index 74% rename from clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.tsx rename to clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx index 825a8507..a9c19b2d 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx @@ -3,8 +3,18 @@ import { Button, Code, Flex, Stack, Text } from "@mantine/core"; import { decodeJwtPayload, isJwtFormat } from "@inspector/core/auth/ema/jwt.js"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; -export interface OAuthAccessTokenFieldProps { - accessToken: string; +/** + * One labelled OAuth token row: the raw value, a copy control, and — when the + * value has the three-segment JWT shape — an in-place "Decode JWT" toggle. + * + * Shared by the Access Token and ID Token rows (#2019). The decode is + * display-only (`decodeJwtPayload` does no signature verification), so this is + * a viewer: neither token is treated as a credential here. + */ +export interface OAuthTokenFieldProps { + /** Row caption, e.g. "Access Token" / "ID Token". */ + label: string; + token: string; onClear?: () => void; clearLabel?: string; } @@ -64,16 +74,17 @@ function JwtTokenText({ token }: { token: string }) { ); } -export function OAuthAccessTokenField({ - accessToken, +export function OAuthTokenField({ + label, + token, onClear, clearLabel = "Clear", -}: OAuthAccessTokenFieldProps) { +}: OAuthTokenFieldProps) { const [showDecoded, setShowDecoded] = useState(false); - const isJwt = isJwtFormat(accessToken); + const isJwt = isJwtFormat(token); const jwtDecoded = useMemo( - () => (isJwt ? decodeJwtPayload(accessToken) : undefined), - [accessToken, isJwt], + () => (isJwt ? decodeJwtPayload(token) : undefined), + [token, isJwt], ); const decodedText = useMemo(() => { @@ -85,12 +96,12 @@ export function OAuthAccessTokenField({ ); }, [jwtDecoded]); - const copyValue = showDecoded && decodedText ? decodedText : accessToken; + const copyValue = showDecoded && decodedText ? decodedText : token; return ( - Access Token + {label} {jwtDecoded && ( + ) : ( - accessToken + token )} diff --git a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts index d6a89da6..d047ab23 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts +++ b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts @@ -34,6 +34,39 @@ describe("oauthDetailsFromConnectionState", () => { }); }); + it("surfaces an id_token when the token set carries one", () => { + const state: OAuthConnectionState = { + authorized: true, + protocol: "standard", + serverUrl: "https://mcp.example.com/mcp", + tokens: { + access_token: "tok", + token_type: "Bearer", + id_token: "header.payload.signature", + }, + }; + + expect(oauthDetailsFromConnectionState(state)).toEqual({ + protocol: "standard", + authorized: true, + accessToken: "tok", + idToken: "header.payload.signature", + }); + }); + + it("omits idToken when the token set carries none", () => { + const state: OAuthConnectionState = { + authorized: true, + protocol: "standard", + serverUrl: "https://mcp.example.com/mcp", + tokens: { access_token: "tok", token_type: "Bearer" }, + }; + + expect(oauthDetailsFromConnectionState(state)).not.toHaveProperty( + "idToken", + ); + }); + it("includes EMA idp session when present", () => { const state: OAuthConnectionState = { authorized: false, diff --git a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts index 14c8847e..2e514633 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts +++ b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts @@ -22,6 +22,11 @@ export function oauthDetailsFromConnectionState( ...(state.tokens?.access_token && { accessToken: state.tokens.access_token, }), + // Surfaced for inspection when the AS returned one; omitted entirely + // otherwise so Connection Info renders no empty row (#2019). + ...(state.tokens?.id_token && { + idToken: state.tokens.id_token, + }), ...(state.protocol === "ema" && state.ema?.idpSession && { idpSession: state.ema.idpSession }), }; diff --git a/specification/v2_auth_ema.md b/specification/v2_auth_ema.md index a7a4c917..bfdb4358 100644 --- a/specification/v2_auth_ema.md +++ b/specification/v2_auth_ema.md @@ -334,7 +334,7 @@ When the user connects to a server with `oauth.enterpriseManaged: true` but inst - **IdP session** (EMA only) — `none` / `logged_in` / `expired` from `idpSessions[issuer]` - **Auth URL** — cached from an in-flight or completed OAuth flow (`OAuthFlowState`), when present - **Scopes** — configured vs granted -- **Access token** — `OAuthAccessTokenField`: copy (raw), decode JWT in place (`core/auth/ema/jwt.ts` helpers); multi-line wrap with segment-aware breaking for JWTs +- **Access token** (and **ID token**, when the authorization server returned an `id_token`) — `OAuthTokenField`: copy (raw), decode JWT in place (`core/auth/ema/jwt.ts` helpers); multi-line wrap with segment-aware breaking for JWTs. Display only — the `id_token` is never treated as a credential (#2019) In-flight OAuth flow state uses `OAuthFlowState` / `getOAuthFlowState()` / `getOAuthFlowStep()`. `getOAuthState()` is the persisted connection snapshot; flow state is separate and ephemeral. diff --git a/specification/v2_ux.md b/specification/v2_ux.md index 08c37b9b..675e9472 100644 --- a/specification/v2_ux.md +++ b/specification/v2_ux.md @@ -294,7 +294,7 @@ Shown as a modal or dedicated screen after successful connection. - Server capabilities: Tools, Resources, Prompts, Logging, Completions, Tasks, Experimental - Client capabilities: Sampling, Elicitation, Roots, Tasks, Experimental - Display server instructions if provided -- Show OAuth connection snapshot when applicable (`InspectorClient.getOAuthState()` → `OAuthConnectionState`): protocol, authorization status, client id, EMA IdP session, cached auth URL, scopes, access token with copy and in-place JWT decode (`OAuthAccessTokenField`) +- Show OAuth connection snapshot when applicable (`InspectorClient.getOAuthState()` → `OAuthConnectionState`): protocol, authorization status, client id, EMA IdP session, cached auth URL, scopes, and the stored tokens — access token, plus the `id_token` when the authorization server returned one — each with copy and in-place JWT decode (`OAuthTokenField`) ### Feature Screens From 71cbe27d83772a7d28f46b5578ea5c5047b6961d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 18:46:54 -0400 Subject: [PATCH 08/30] fix: address Copilot round 5 on the list-salvage path (#1909) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings are the same shape: the fallback accepting what the strict path would not. - `ModernResultEnvelopeSchema` typed `ttlMs` as any number, where the codec requires a non-negative integer. `ttlMs: -1` (or `0.5`) plus one bad entry would salvage, reporting the entry and hiding the envelope violation. Tightened to `z.int().min(0)`. - The item schema was the SDK's era-NEUTRAL `ToolSchema`, and the negotiated era's wire schema is stricter in ways the SDK does not export. Verified against SDK 2.0.0 on a real connection: on legacy, a tool with `outputSchema: { type: "array" }` is refused by the strict aggregate but accepted by the neutral schema; on modern, `execution` is STRIPPED by the strict path but kept by the neutral one. Either way a salvaged list could show — and let the user call — a tool the connection's era says isn't valid. `toolItemSchemaForEra` closes both. Replicating rules that live in the SDK's internals is a real cost; `listSalvage-era.test.ts` is what makes it safe to pay, pinning each rule to the strict path's OBSERVED behavior so a divergence that closes or widens fails loudly instead of rotting. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall --- .../web/src/test/core/mcp/listSalvage.test.ts | 14 ++ .../integration/mcp/listSalvage-era.test.ts | 214 ++++++++++++++++++ core/mcp/inspectorClient.ts | 11 +- core/mcp/listSalvage.ts | 44 +++- 4 files changed, 278 insertions(+), 5 deletions(-) create mode 100644 clients/web/src/test/integration/mcp/listSalvage-era.test.ts diff --git a/clients/web/src/test/core/mcp/listSalvage.test.ts b/clients/web/src/test/core/mcp/listSalvage.test.ts index a117403b..f547eee0 100644 --- a/clients/web/src/test/core/mcp/listSalvage.test.ts +++ b/clients/web/src/test/core/mcp/listSalvage.test.ts @@ -132,6 +132,20 @@ describe("ModernResultEnvelopeSchema", () => { ).toBe(true); }); + it("requires ttlMs to be a non-negative integer, as the codec does", () => { + // A looser `z.number()` would accept `-1` / `0.5`, so a response with a bad + // envelope AND one bad entry would salvage and hide the envelope violation. + for (const ttlMs of [-1, 0.5]) { + expect( + ModernResultEnvelopeSchema.safeParse({ + resultType: "complete", + ttlMs, + cacheScope: "public", + }).success, + ).toBe(false); + } + }); + it("rejects a modern result missing the envelope the codec would enforce", () => { // The raw-wire salvage path goes around that codec, so a response missing // its cache hints AND carrying a bad entry must not salvage. diff --git a/clients/web/src/test/integration/mcp/listSalvage-era.test.ts b/clients/web/src/test/integration/mcp/listSalvage-era.test.ts new file mode 100644 index 00000000..723b47da --- /dev/null +++ b/clients/web/src/test/integration/mcp/listSalvage-era.test.ts @@ -0,0 +1,214 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { + createServer, + type IncomingMessage, + type Server, + type ServerResponse, +} from "node:http"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js"; +import { ToolSchema } from "@modelcontextprotocol/core"; +import { toolItemSchemaForEra } from "@inspector/core/mcp/listSalvage.js"; + +/** + * The era item contract the salvage fallback validates tools against (#1909). + * + * `toolItemSchemaForEra` replicates two rules that live in the SDK's internal + * per-era wire schemas, which are not exported. That is a real cost, and these + * tests are what makes it safe to pay: each one asserts the STRICT path's + * observed behavior on a real connection, so if a future SDK closes or widens a + * divergence, the test fails and the replicated rule gets revisited instead of + * silently rotting. + * + * The failure being prevented: the fallback keeping an entry the strict path + * refused, which the user would then see — and could call — on a connection + * whose era says it isn't valid. + */ + +const ARRAY_OUTPUT_TOOL = { + name: "array_output", + inputSchema: { type: "object" }, + outputSchema: { type: "array" }, +}; + +const EXECUTION_TOOL = { + name: "with_execution", + inputSchema: { type: "object" }, + execution: { taskSupport: "optional" }, +}; + +function startToolsServer( + tools: unknown[], + modern: boolean, +): Promise<{ url: string; stop: () => Promise }> { + const handler = async (req: IncomingMessage, res: ServerResponse) => { + const chunks: Buffer[] = []; + for await (const chunk of req) chunks.push(chunk as Buffer); + const raw = Buffer.concat(chunks).toString(); + if (raw.length === 0) { + res.writeHead(405).end(); + return; + } + const body = JSON.parse(raw) as { id?: unknown; method: string }; + if (body.id === undefined) { + res.writeHead(202).end(); + return; + } + const envelope = modern + ? { resultType: "complete", ttlMs: 0, cacheScope: "public" } + : {}; + const send = (result: object) => { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end( + JSON.stringify({ + jsonrpc: "2.0", + id: body.id, + result: { ...envelope, ...result }, + }), + ); + }; + if (body.method === "initialize") { + send({ + protocolVersion: modern ? "2026-07-28" : "2025-06-18", + capabilities: { tools: {} }, + serverInfo: { name: "era-contract-server", version: "1.0.0" }, + }); + return; + } + if (body.method === "server/discover") { + send({ + supportedVersions: ["2026-07-28"], + capabilities: { tools: {} }, + serverInfo: { name: "era-contract-server", version: "1.0.0" }, + }); + return; + } + if (body.method === "tools/list") { + send({ tools }); + return; + } + send({}); + }; + + const server: Server = createServer((req, res) => { + void handler(req, res); + }); + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + /* v8 ignore next -- listen() on a fresh server always yields an AddressInfo */ + const port = typeof address === "object" && address ? address.port : 0; + resolve({ + url: `http://127.0.0.1:${port}/mcp`, + stop: () => + new Promise((done) => { + server.close(() => done()); + }), + }); + }); + }); +} + +describe("salvage era item contract (#1909)", () => { + let client: InspectorClient | null = null; + let stop: (() => Promise) | null = null; + + afterEach(async () => { + try { + await client?.disconnect(); + } catch { + // Teardown only. + } + client = null; + await stop?.(); + stop = null; + }); + + async function connect(url: string, era: "legacy" | "modern") { + client = new InspectorClient( + { type: "streamable-http", url }, + { + environment: { transport: createTransportNode }, + versionNegotiation: eraToVersionNegotiation(era), + }, + ); + await client.connect(); + return client; + } + + it("legacy: the era rule is what rejects a non-object outputSchema (pins it)", async () => { + // The rule lives in the SDK's high-level aggregate (the era codec), not in + // the neutral schema the single-page call uses — which is precisely the + // divergence `toolItemSchemaForEra` exists to close. + // + // This tool is the page's ONLY problem, so the observable outcome is + // entirely the SDK's rule talking: the strict aggregate refuses the page, + // the fallback runs, and our contract agrees the tool is not era-valid. If + // a future SDK accepts it, the aggregate succeeds instead and this fails — + // which is the signal to revisit the replicated rule. + const server = await startToolsServer([ARRAY_OUTPUT_TOOL], false); + stop = server.stop; + const connected = await connect(server.url, "legacy"); + + const { tools } = await connected.listAllTools(); + expect(tools).toEqual([]); + expect( + connected.getMalformedListItems().map((entry) => entry.label), + ).toEqual(["array_output"]); + }); + + it("modern: the era rule accepts a tool but strips execution (pins it)", async () => { + // The mirror image: here the strict aggregate SUCCEEDS, so no salvage runs + // and what comes back is the SDK's own normalization. If it ever stops + // stripping, `execution` reappears here and this fails. + const server = await startToolsServer([EXECUTION_TOOL], true); + stop = server.stop; + const connected = await connect(server.url, "modern"); + + const { tools } = await connected.listAllTools(); + expect(tools).toHaveLength(1); + expect(tools[0]).not.toHaveProperty("execution"); + expect(connected.getMalformedListItems()).toEqual([]); + }); + + it("the neutral ToolSchema is looser than both, which is why the contract exists", () => { + expect(ToolSchema.safeParse(ARRAY_OUTPUT_TOOL).success).toBe(true); + const neutral = ToolSchema.safeParse(EXECUTION_TOOL); + expect(neutral.success).toBe(true); + expect(neutral.success && neutral.data).toHaveProperty("execution"); + }); + + it("the era contract matches the strict path on both divergences", () => { + // Legacy: rejected, exactly as the strict parse above rejected it. + expect( + toolItemSchemaForEra(false).safeParse(ARRAY_OUTPUT_TOOL).success, + ).toBe(false); + // Modern: accepted, with `execution` stripped, exactly as above. + const modern = toolItemSchemaForEra(true).safeParse(EXECUTION_TOOL); + expect(modern.success).toBe(true); + expect(modern.success && modern.data).not.toHaveProperty("execution"); + }); + + it("keeps only era-valid tools when salvaging beside a malformed one", async () => { + // The end-to-end shape of the defect: a malformed entry trips the + // fallback, and the array-output tool must NOT ride along on the strength + // of the neutral schema accepting it. + const server = await startToolsServer( + [ + { name: "ok_tool", inputSchema: { type: "object" } }, + ARRAY_OUTPUT_TOOL, + { name: "broken", inputSchema: "not-a-schema" }, + ], + false, + ); + stop = server.stop; + const connected = await connect(server.url, "legacy"); + + const { tools } = await connected.listAllTools(); + expect(tools.map((tool) => tool.name)).toEqual(["ok_tool"]); + expect( + connected.getMalformedListItems().map((entry) => entry.label), + ).toEqual(["array_output", "broken"]); + }); +}); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 90a6c37d..94a1abd2 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -161,8 +161,8 @@ import { ListPromptsResultSchema, // Per-primitive item schemas — the salvage fallback validates a list's // entries one at a time with these when the whole-result parse rejects - // (#1909, see `listAllSalvaging`). - ToolSchema, + // (#1909, see `listAllSalvaging`). Tools go through `toolItemSchemaForEra` + // instead, since the neutral schema is looser than the negotiated era's. ResourceSchema, ResourceTemplateSchema, PromptSchema, @@ -174,6 +174,7 @@ import { validateToolOutput } from "./toolOutputValidation.js"; import { ModernResultEnvelopeSchema, isSalvageableRejection, + toolItemSchemaForEra, nextCursorOf, lenientListPageSchema, rawItemsOf, @@ -3190,7 +3191,9 @@ export class InspectorClient extends InspectorClientEventTarget { method: "tools/list", itemsKey: "tools", resultSchema: ListToolsResultSchema, - itemSchema: ToolSchema, + // Era-aware: the neutral `ToolSchema` is more permissive than the + // negotiated era's wire schema — see `toolItemSchemaForEra`. + itemSchema: toolItemSchemaForEra(this.isModernEra()), metadata: options?.metadata, finalize: (salvaged) => this.excludeInvalidXMcpHeaderTools(salvaged), aggregate: async () => [ @@ -3656,7 +3659,7 @@ export class InspectorClient extends InspectorClientEventTarget { const { valid, malformed } = salvageListItems({ method: "tools/list", items, - schema: ToolSchema, + schema: toolItemSchemaForEra(this.isModernEra()), }); if (rejectedResponseId !== undefined && malformed.length > 0) { this.dispatchTypedEvent("responseRejected", { diff --git a/core/mcp/listSalvage.ts b/core/mcp/listSalvage.ts index 37aeebbe..7c5ab1e4 100644 --- a/core/mcp/listSalvage.ts +++ b/core/mcp/listSalvage.ts @@ -25,6 +25,8 @@ import { z } from "zod/v4"; import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; +import { ToolSchema } from "@modelcontextprotocol/core"; +import type { Tool } from "@modelcontextprotocol/client"; /** * One entry the Inspector dropped from a list result because it failed the @@ -140,7 +142,10 @@ export function nextCursorOf(page: unknown): string | undefined { */ export const ModernResultEnvelopeSchema = z.looseObject({ resultType: z.literal("complete"), - ttlMs: z.number(), + // A non-negative INTEGER, matching the codec: `ttlMs: -1` or `0.5` is an + // envelope violation the strict path rejects, and accepting it here on the + // strength of an unrelated bad entry would hide it. + ttlMs: z.int().min(0), cacheScope: z.enum(["public", "private"]), }); @@ -261,3 +266,40 @@ export function summarizeMalformed(malformed: MalformedListItem[]): string { malformed.length === 1 ? "entry" : "entries" } — ${where}: ${first.reason}${rest}`; } + +/** + * The item contract for `tools/list` on a given era. + * + * The public `ToolSchema` is era-NEUTRAL, and the negotiated era's wire schema + * is stricter in ways the SDK does not export. Validating candidates against + * the neutral schema alone would let the fallback keep an entry the strict path + * refused — an entry the user would then see, and could call, on a connection + * whose era says it isn't valid. The two known divergences (verified against + * SDK 2.0.0 by `listSalvage-era.test.ts`, which fails if either changes): + * + * - **legacy (2025)** requires `outputSchema.type === "object"`; the neutral + * schema accepts any object, so a tool with `outputSchema: { type: "array" }` + * would survive salvage while the strict parse rejects the whole page. + * - **modern (2026)** deletes `execution`, and the strict path STRIPS it. Left + * in place, a salvaged tool would carry a field the era doesn't have, and the + * UI would render capability the connection can't honor. + * + * Replicating rules that live in the SDK's internals is a real cost, taken + * deliberately: the alternative is a fallback that is quietly more permissive + * than the path it stands in for. The tests pin each rule to observed SDK + * behavior, so a divergence that closes (or widens) fails loudly rather than + * rotting. + */ +export function toolItemSchemaForEra(isModern: boolean): z.ZodType { + if (isModern) { + // `execution` is not part of the 2026 wire shape; drop it as the codec does. + return ToolSchema.transform((tool) => { + if (tool.execution === undefined) return tool; + const { execution: _execution, ...rest } = tool; + return rest; + }); + } + return ToolSchema.extend({ + outputSchema: z.looseObject({ type: z.literal("object") }).optional(), + }); +} From b25ece8ded1da31003f926b12ce216119e695863 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 18:58:46 -0400 Subject: [PATCH 09/30] feat: per-server custom OAuth authorization-request parameters (#2018) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real deployments gate IdP selection, prompt behavior, or audience on a query parameter the OAuth/OIDC core specs never standardized (Keycloak's `kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's `audience`), and none of them could be driven from the Inspector. The Inspector never builds the authorize URL — the SDK's `auth()` does, and it exposes no hook for extra parameters — so the merge happens at the one seam that sees the finished URL: `BaseOAuthClientProvider.redirectToAuthorization`. It lives in `core/`, so web, CLI, and TUI all inherit it. - `core/auth/authorizationParams.ts`: the reserved-key list (`client_id`, `code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`, `response_type`, `scope`, `state`), the key validator the form renders inline, and the merge itself. Reserved keys are dropped with a warning rather than silently overriding PKCE / the state binding / RFC 8707; blank-key rows are skipped; the caller's URL is never mutated. - Applied to the authorization request only — nothing wires it into the token request. - Persisted as `oauth.authorizationParams` (a string record) on the per-server config beside `scopes`, edited as key/value rows in Server Settings -> Authorization ("Additional authorization parameters"), with an inline error per offending row plus a section-level warning. - The enterprise-managed (EMA) leg redirects to a *different* authorization server (the enterprise IdP), so it goes through the explicit `redirectToExternalAuthorization` entry point, which skips the merge — a `kc_idp_hint` or `audience` meant for the MCP server's AS must never be appended to the IdP's authorize URL. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E Signed-off-by: cliffhall --- AGENTS.md | 9 +- clients/web/src/App.tsx | 8 + .../ServerSettingsForm.test.tsx | 157 ++++++++++++++++++ .../ServerSettingsForm/ServerSettingsForm.tsx | 148 +++++++++++++++++ .../ServerSettingsModal.test.tsx | 25 +++ .../ServerSettingsModal.tsx | 4 + .../core/auth/authorizationParams.test.ts | 111 +++++++++++++ .../core/auth/ema/transportProvider.test.ts | 47 +++++- .../web/src/test/core/auth/providers.test.ts | 62 +++++++ .../web/src/test/core/client/runner.test.ts | 38 +++++ .../web/src/test/core/mcp/serverList.test.ts | 78 +++++++++ .../mcp/remote/server-extra-coverage.test.ts | 52 ++++++ .../mcp/remote/servers-route.test.ts | 48 ++++++ core/auth/authorizationParams.ts | 105 ++++++++++++ core/auth/ema/transportProvider.ts | 6 +- core/auth/index.ts | 9 + core/auth/providers.ts | 40 +++++ core/client/runner.ts | 9 + core/mcp/inspectorClient.ts | 2 + core/mcp/oauthManager.ts | 6 + core/mcp/remote/node/server.ts | 27 ++- core/mcp/serverList.ts | 45 +++++ core/mcp/types.ts | 30 ++++ docs/mcp-server-configuration.md | 4 +- 24 files changed, 1064 insertions(+), 6 deletions(-) create mode 100644 clients/web/src/test/core/auth/authorizationParams.test.ts create mode 100644 core/auth/authorizationParams.ts diff --git a/AGENTS.md b/AGENTS.md index 0baea459..cfd08caa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -38,7 +38,14 @@ v2/main/ │ │ # issuerBinding.ts SEP-2352 callback-leg failure │ │ # classification — separates a recoverable │ │ # "lost authorization state" from a genuine -│ │ # cross-AS issuer mismatch — #1808) +│ │ # cross-AS issuer mismatch — #1808; +│ │ # authorizationParams.ts per-server custom +│ │ # authorization-request parameters — the +│ │ # reserved-key list plus the merge applied in +│ │ # providers.redirectToAuthorization, the only +│ │ # seam that sees the SDK-built authorize URL; +│ │ # authorization request only, never the token +│ │ # request — #2018) │ │ ├── browser/ # Browser-side OAuth (sessionStorage, BrowserNavigation) │ │ ├── node/ # Node-side OAuth (NodeOAuthStorage, OAuthCallbackServer, │ │ │ # runner-interactive-oauth loopback callback flow) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 509fc079..97aefe73 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -63,6 +63,7 @@ import { ManagedRequestorTasksState } from "@inspector/core/mcp/state/managedReq import { ResourceSubscriptionsState } from "@inspector/core/mcp/state/resourceSubscriptionsState.js"; import { cleanRoots, + oauthAuthorizationParamsFromSettings, serializeMcpConfig, } from "@inspector/core/mcp/serverList.js"; import type { ClientConfig } from "@inspector/core/client/types.js"; @@ -2301,11 +2302,15 @@ function App() { .map((m) => [m.key, m.value]), ) : undefined; + const serverAuthorizationParams = savedSettings + ? oauthAuthorizationParamsFromSettings(savedSettings) + : undefined; const oauthFromServer = savedSettings && (savedSettings.oauthClientId || savedSettings.oauthClientSecret || savedSettings.oauthScopes || + serverAuthorizationParams || savedSettings.enterpriseManaged) ? { ...(savedSettings.oauthClientId && { @@ -2317,6 +2322,9 @@ function App() { ...(savedSettings.oauthScopes && { scope: savedSettings.oauthScopes, }), + ...(serverAuthorizationParams && { + authorizationParams: serverAuthorizationParams, + }), ...(savedSettings.enterpriseManaged && { enterpriseManaged: true, }), diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index 43b400bf..b66d76ac 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -700,10 +700,165 @@ describe("ServerSettingsForm", () => { clientId: "a", clientSecret: "", scopes: "", + authorizationParams: [], enterpriseManaged: false, }); }); + // #2018 — custom authorization-request parameters. + it("shows the empty hint for authorization parameters when none are set", () => { + renderWithMantine( + , + ); + expect( + screen.getByText("Additional authorization parameters"), + ).toBeInTheDocument(); + expect( + screen.getByText("No additional parameters configured"), + ).toBeInTheDocument(); + }); + + it("adds an authorization-parameter row through onOAuthChange", async () => { + const user = userEvent.setup(); + const onOAuthChange = vi.fn(); + renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: "+ Add Parameter" })); + expect(onOAuthChange).toHaveBeenCalledWith( + expect.objectContaining({ + authorizationParams: [{ key: "", value: "" }], + }), + ); + }); + + it("edits and removes an authorization-parameter row", async () => { + const user = userEvent.setup(); + const onOAuthChange = vi.fn(); + renderWithMantine( + , + ); + const valueInput = screen.getByDisplayValue("corp"); + await user.type(valueInput, "x"); + expect(onOAuthChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + authorizationParams: [{ key: "kc_idp_hint", value: "corpx" }], + }), + ); + + onOAuthChange.mockClear(); + const removeButton = screen + .getAllByRole("button") + .find((b) => b.textContent === "X"); + await user.click(removeButton as HTMLElement); + expect(onOAuthChange).toHaveBeenLastCalledWith( + expect.objectContaining({ authorizationParams: [] }), + ); + }); + + it("clears an authorization-parameter key via its Clear button", async () => { + const user = userEvent.setup(); + const onOAuthChange = vi.fn(); + renderWithMantine( + , + ); + await user.click(clearButtonFor(screen.getByDisplayValue("kc_idp_hint"))); + expect(onOAuthChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + authorizationParams: [{ key: "", value: "corp" }], + }), + ); + + onOAuthChange.mockClear(); + await user.click(clearButtonFor(screen.getByDisplayValue("corp"))); + expect(onOAuthChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + authorizationParams: [{ key: "kc_idp_hint", value: "" }], + }), + ); + }); + + it("rejects a reserved authorization-parameter key inline and with a warning", () => { + renderWithMantine( + , + ); + expect( + screen.getByText( + '"state" is set by the authorization flow and cannot be overridden.', + ), + ).toBeInTheDocument(); + expect(screen.getByText("Reserved parameters ignored")).toBeInTheDocument(); + expect(screen.getByDisplayValue("state")).toHaveAttribute( + "aria-invalid", + "true", + ); + }); + + it("does not flag a blank authorization-parameter row as reserved", () => { + renderWithMantine( + , + ); + expect( + screen.queryByText("Reserved parameters ignored"), + ).not.toBeInTheDocument(); + }); + + it("pluralizes the reserved-parameter warning for multiple keys", () => { + renderWithMantine( + , + ); + expect(screen.getByText(/state, scope are set/)).toBeInTheDocument(); + }); + it("invokes onOAuthChange with the chosen insufficient-scope policy (SEP-2350)", async () => { const user = userEvent.setup(); const onOAuthChange = vi.fn(); @@ -744,6 +899,7 @@ describe("ServerSettingsForm", () => { clientId: "", clientSecret: "", scopes: "", + authorizationParams: [], enterpriseManaged: true, }); }); @@ -848,6 +1004,7 @@ describe("ServerSettingsForm", () => { clientId: "", clientSecret: "z", scopes: "", + authorizationParams: [], enterpriseManaged: false, }); }); diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index e20ddd44..6a3bee1c 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -1,6 +1,7 @@ import { Accordion, ActionIcon, + Alert, Button, Checkbox, Flex, @@ -26,6 +27,11 @@ import { MODERN_LOG_LEVELS, } from "@inspector/core/mcp/types.js"; import { isOAuthCapableServerType } from "@inspector/core/mcp/config.js"; +import { + RESERVED_AUTHORIZATION_PARAMS, + authorizationParamKeyError, + isReservedAuthorizationParam, +} from "@inspector/core/auth/authorizationParams.js"; import { ADVERTISABLE_EXTENSIONS } from "@inspector/core/mcp/extensions.js"; import type { Root } from "@modelcontextprotocol/client"; @@ -184,6 +190,23 @@ const EmptyHint = Text.withProps({ fs: "italic", }); +// Matches the label/description pair Mantine renders for a single input, so the +// authorization-parameter row list reads as one labelled field next to Scopes. +const FieldLabel = Text.withProps({ + size: "sm", + fw: 500, +}); + +const FieldDescription = Text.withProps({ + size: "xs", + c: "dimmed", +}); + +const ReservedParamError = Text.withProps({ + size: "xs", + c: "var(--inspector-danger-text)", +}); + const ClearStoredOAuthButton = Button.withProps({ variant: "light", color: "red", @@ -245,6 +268,63 @@ function KeyValueRows({ ); } +// Reserved-key rejection is inline per row (#2018): the key input goes red with +// the reason under it, and the section-level warning below repeats it so the +// rejection is visible even when the offending row is scrolled out of view. +function AuthorizationParamRows({ + params, + onChange, + onRemove, +}: { + params: { key: string; value: string }[]; + onChange: (index: number, key: string, value: string) => void; + onRemove: (index: number) => void; +}) { + return ( + <> + {params.map((param, index) => ( + + + + onChange(index, e.currentTarget.value, param.value) + } + rightSection={ + param.key ? ( + onChange(index, "", param.value)} + /> + ) : null + } + /> + + onChange(index, param.key, e.currentTarget.value) + } + rightSection={ + param.value ? ( + onChange(index, param.key, "")} /> + ) : null + } + /> + onRemove(index)}>X + + {authorizationParamKeyError(param.key) ? ( + + {authorizationParamKeyError(param.key)} + + ) : null} + + ))} + + ); +} + function RootRows({ roots, onChange, @@ -374,16 +454,52 @@ export function ServerSettingsForm({ onTimeoutChange(field, numValue); }; + // #2018 — custom authorization-request parameters. The rows ride the single + // `onOAuthChange` callback with the rest of the Authorization section rather + // than growing three more props on this form. + const authorizationParams = settings.oauthAuthorizationParams ?? []; + function currentOAuth(): OAuthSettings { return { clientId: settings.oauthClientId ?? "", clientSecret: settings.oauthClientSecret ?? "", scopes: settings.oauthScopes ?? "", + authorizationParams, enterpriseManaged: settings.enterpriseManaged ?? false, onInsufficientScope: settings.oauthOnInsufficientScope, }; } + const rejectedParamKeys = authorizationParams + .map((p) => p.key.trim()) + .filter((key) => isReservedAuthorizationParam(key)); + + function changeAuthorizationParams( + params: { key: string; value: string }[], + ): void { + onOAuthChange({ ...currentOAuth(), authorizationParams: params }); + } + + function handleAddAuthorizationParam(): void { + changeAuthorizationParams([...authorizationParams, { key: "", value: "" }]); + } + + function handleRemoveAuthorizationParam(index: number): void { + changeAuthorizationParams( + authorizationParams.filter((_, i) => i !== index), + ); + } + + function handleAuthorizationParamChange( + index: number, + key: string, + value: string, + ): void { + changeAuthorizationParams( + authorizationParams.map((p, i) => (i === index ? { key, value } : p)), + ); + } + return ( // Stays inline: Accordion is a compound, `multiple`-discriminated generic, // so `.withProps({ multiple: true, ... })` loses its JSX call signature @@ -699,6 +815,38 @@ export function ServerSettingsForm({ ) : null } /> + + Additional authorization parameters + + Extra query parameters appended to the authorization request + only — never the token request. Use them for provider-specific + hints such as kc_idp_hint, login_hint, prompt, acr_values, or + audience. + + {authorizationParams.length === 0 ? ( + No additional parameters configured + ) : ( + + )} + {rejectedParamKeys.length > 0 ? ( + + {`${rejectedParamKeys.join(", ")} ${ + rejectedParamKeys.length === 1 ? "is" : "are" + } set by the authorization flow and will not be sent. Overriding ${ + rejectedParamKeys.length === 1 ? "it" : "them" + } would break PKCE, the CSRF state binding, or the resource indicator. Reserved: ${RESERVED_AUTHORIZATION_PARAMS.join( + ", ", + )}.`} + + ) : null} + + + Add Parameter + +