diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index b42a2e3b..09645d9a 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -163,6 +163,7 @@ import { import { buildExportFilename, downloadJsonFile } from "./lib/downloadFile"; import { INSPECTOR_SERVERS_TAB } from "./utils/inspectorTabs"; import { enrichProtocolEntries } from "./utils/correlateTransportErrors"; +import { visibleMalformedListItems } from "./utils/malformedListReport"; import { parseDeepLink, deepLinkConfigEquals, @@ -1056,6 +1057,14 @@ function App() { setPaginatedListsOverride(null); }, [persistedPaginatedLists, activeServerId]); const paginatedLists = paginatedListsOverride ?? persistedPaginatedLists; + // The malformed-entry report is written by the aggregate walk's salvage. In + // paginated mode the tools/prompts/resources panels render the paged stores + // instead, which never write or clear it — so it would linger above a page it + // does not describe. See `visibleMalformedListItems` (#1909 + #1721). + const shownMalformedListItems = useMemo( + () => visibleMalformedListItems(malformedListItems, paginatedLists), + [malformedListItems, paginatedLists], + ); const connected = connectionStatus === "connected"; const toolsPagination = usePaginatedList({ connected, @@ -4377,7 +4386,7 @@ function App() { latencyMs={latencyMs} tools={tools} excludedTools={excludedTools} - malformedListItems={malformedListItems} + malformedListItems={shownMalformedListItems} prompts={prompts} resources={resources} resourceTemplates={resourceTemplates} diff --git a/clients/web/src/test/integration/mcp/listSalvage-era.test.ts b/clients/web/src/test/integration/mcp/listSalvage-era.test.ts index e90af022..3b570bc2 100644 --- a/clients/web/src/test/integration/mcp/listSalvage-era.test.ts +++ b/clients/web/src/test/integration/mcp/listSalvage-era.test.ts @@ -197,6 +197,29 @@ describe("salvage era item contract (#1909)", () => { ); }); + it("strips unknown fields the way the strict codec does", async () => { + // The contract validates through a wire schema; returning the ORIGINAL + // value rather than the parsed one would leave salvaged tools carrying + // fields the strict path drops, so the two paths would hand the UI + // differently-shaped tools for the same server. + const strayTool = { + name: "stray", + inputSchema: { type: "object" }, + notAToolField: "should not survive", + }; + const parsed = toolItemSchemaForEra(false).safeParse(strayTool); + expect(parsed.success).toBe(true); + expect(parsed.success && parsed.data).not.toHaveProperty("notAToolField"); + + // And the strict path agrees — this is the shape being matched, not an + // invented rule. + const server = await startToolsServer([strayTool], false); + stop = server.stop; + const connected = await connect(server.url, "legacy"); + const { tools } = await connected.listAllTools(); + expect(tools[0]).not.toHaveProperty("notAToolField"); + }); + it("the era contract matches the strict path on both divergences", () => { // Legacy: rejected, exactly as the strict parse above rejected it. expect( diff --git a/clients/web/src/utils/malformedListReport.test.ts b/clients/web/src/utils/malformedListReport.test.ts new file mode 100644 index 00000000..66fd24fb --- /dev/null +++ b/clients/web/src/utils/malformedListReport.test.ts @@ -0,0 +1,44 @@ +import { describe, it, expect } from "vitest"; +import type { MalformedListItem } from "@inspector/core/mcp"; +import { visibleMalformedListItems } from "./malformedListReport"; + +const entry = (method: string): MalformedListItem => ({ + method, + index: 0, + reason: "annotations: expected object", +}); + +const TOOLS = entry("tools/list"); +const PROMPTS = entry("prompts/list"); +const RESOURCES = entry("resources/list"); +const TEMPLATES = entry("resources/templates/list"); + +describe("visibleMalformedListItems", () => { + it("shows every report in aggregate mode", () => { + const items = [TOOLS, PROMPTS, RESOURCES, TEMPLATES]; + expect(visibleMalformedListItems(items, false)).toEqual(items); + }); + + it("returns the same array in aggregate mode rather than a copy", () => { + // Identity is load-bearing: the value feeds a memo, and a fresh array every + // render would re-render the panels for a filter that removed nothing. + const items = [TOOLS]; + expect(visibleMalformedListItems(items, false)).toBe(items); + }); + + it("suppresses the paged lists' reports in paginated mode", () => { + // Those panels render the paged store, which neither writes nor clears this + // report — so the entry would describe a list that is no longer on screen. + expect( + visibleMalformedListItems([TOOLS, PROMPTS, RESOURCES], true), + ).toEqual([]); + }); + + it("keeps resource templates, which are aggregate-backed in both modes", () => { + // Templates have no paged store, so their report always describes the list + // actually being rendered. + expect( + visibleMalformedListItems([TOOLS, TEMPLATES, PROMPTS], true), + ).toEqual([TEMPLATES]); + }); +}); diff --git a/clients/web/src/utils/malformedListReport.ts b/clients/web/src/utils/malformedListReport.ts new file mode 100644 index 00000000..5b19b0ee --- /dev/null +++ b/clients/web/src/utils/malformedListReport.ts @@ -0,0 +1,46 @@ +import type { MalformedListItem } from "@inspector/core/mcp"; + +/** + * Which malformed-entry reports the UI may show, given the pagination mode + * (#1909 + #1721). + * + * The report is produced by the salvage fallback inside the client's `listAll*` + * aggregate walk. In **paginated** mode the tools, prompts, and resources + * panels do not render that aggregate at all — they render the paged store, + * which fetches one page at a time through a different path that neither writes + * nor clears this report. So a report written while the list was aggregated + * would simply persist, sitting above a freshly fetched page it does not + * describe: the same warning over a page that may be perfectly conforming, with + * indices that refer to positions in an aggregate the user is no longer + * looking at. + * + * Suppressing is the honest option of the two available. The alternative — + * giving the paged path its own per-page salvage and report — is a real feature + * rather than a display fix, and showing a stale warning is worse than showing + * none: this panel's whole purpose is to say which entry the server got wrong, + * so pointing at the wrong entry costs more than staying quiet. + * + * Resource templates are deliberately NOT suppressed. That list has no paged + * store — it is fetched through the aggregate in both modes — so its report is + * always about the list actually on screen. + */ +const PAGED_LIST_METHODS: readonly string[] = [ + "tools/list", + "prompts/list", + "resources/list", +]; + +/** + * The reports to render for the current pagination mode. + * + * Returns the input untouched in aggregate mode — the common case — so the + * memoized identity is preserved and nothing re-renders for a filter that + * removes nothing. + */ +export function visibleMalformedListItems( + items: MalformedListItem[], + paginated: boolean, +): MalformedListItem[] { + if (!paginated) return items; + return items.filter((item) => !PAGED_LIST_METHODS.includes(item.method)); +} diff --git a/core/mcp/listSalvage.ts b/core/mcp/listSalvage.ts index 1f2109c3..053b465b 100644 --- a/core/mcp/listSalvage.ts +++ b/core/mcp/listSalvage.ts @@ -229,21 +229,9 @@ export function describeIssues(error: z.ZodError): string { /* 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 message = describeIssue(issue); - return path.length > 0 ? `${path}: ${message}` : message; -} - -/** - * One issue's message, with the expected type folded in when it adds something. - * - * Split out from {@link describeIssues} because {@link eraToolItemSchema} has to - * apply it EARLY: forwarding another schema's issues re-emits them as `custom` - * ones, which carry a message and a path but no `expected` field, so the detail - * has to be baked into the message while the original issue is still in hand. - */ -function describeIssue(issue: z.core.$ZodIssue): string { const detail = expectedDetail(issue); - return detail ? `${issue.message} (${detail})` : issue.message; + const message = detail ? `${issue.message} (${detail})` : issue.message; + return path.length > 0 ? `${path}: ${message}` : message; } /** @@ -398,40 +386,29 @@ function stripExecution(tool: Tool): Tool { /** * Bridge a per-era WIRE schema to the `Tool` the rest of the client works with. * - * The two are deliberately separate, because on BOTH eras the wire schema - * accepts values the exported `Tool` type cannot describe — `properties: 5` is - * a valid modern `inputSchema` keyword set, and the SDK hands exactly that back - * typed as a `Tool`. So the era rules are expressed as an ordinary schema whose - * output type is irrelevant (it is only ever consulted for a verdict), and - * `z.custom` supplies the type the caller needs. That is what avoids the - * `as unknown as` this would otherwise require — and it is the same assertion - * the SDK itself makes, in the same place. + * The bridge exists because the two cannot be the same schema: on BOTH eras the + * wire shape accepts values the exported `Tool` type cannot describe — + * `properties: 5` is a valid modern `inputSchema` keyword set, and the SDK + * hands exactly that back typed as a `Tool`. So the era rules are expressed as + * an ordinary schema, and its parsed output is asserted to `Tool` here. * - * The wire schema's issues are copied across one by one, each keeping its PATH, - * rather than collapsed into a single "invalid tool" — so `describeIssues` can - * still name the offending field (`inputSchema.type`) instead of reporting a - * bare "Invalid input", which is the whole point of the report this module - * produces. They are re-emitted as `custom` issues (the only code that can - * carry an arbitrary forwarded message), which is why the expected-type detail - * is folded into the message here by {@link describeIssue} rather than left for - * `describeIssues` to recover from a field the new issue will not have. + * The parsed OUTPUT is what gets normalized and returned, not the raw input. + * That matters: both era schemas are `z.object`, so parsing strips unknown + * top-level fields (and unknown keys inside `annotations`) exactly as the + * strict codec does. Validating with the wire schema but returning the original + * value would leave salvaged tools carrying fields the strict path drops, so + * the two paths would hand the UI differently-shaped tools for the same server. + * + * The assertion is single and deliberate — `parsed` is `unknown`, so this is + * the narrow `as` the double-cast rule asks for rather than an `as unknown as` + * — and it is the same claim the SDK makes about its own era-parsed results. + * Issues come straight from the wire parse, so they keep their field paths and + * `describeIssues` can still name `inputSchema.type` rather than reporting a + * bare "Invalid input". */ function eraToolItemSchema( wire: z.ZodType, normalize: (tool: Tool) => Tool, ): z.ZodType { - return z - .custom() - .superRefine((value, ctx) => { - const parsed = wire.safeParse(value); - if (parsed.success) return; - for (const issue of parsed.error.issues) { - ctx.addIssue({ - code: "custom", - path: issue.path, - message: describeIssue(issue), - }); - } - }) - .transform(normalize); + return wire.transform((parsed) => normalize(parsed as Tool)); }