From 4379d17f7627c08e2cf6ff96f6d9f261dc54e193 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 09:21:46 -0400 Subject: [PATCH 01/35] fix(web): surface a failed post-write server-list reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `useServers`' mutators awaited `refresh()`, whose failure was swallowed into the hook's `error` state rather than thrown. `ServerConfigModal` renders `submitError` only for something `onSubmit` threw, so a successful `POST /api/servers` followed by a failed `GET /api/servers` closed the Add Server modal as though the whole thing had worked — no inline error, no toast, and no new row, since the list read is what would have produced it. That is the silent failure reported in #1914; #1848 removed the trigger (a keyring-less container 500'ing the list read) but not the swallow, so any other cause reproduces it. Split the list read into `loadServers` and give the mutators their own `refreshAfterWrite`, which records the failure in `error` as before and then rethrows. `refreshInternal` is unchanged for the mount effect and the SSE loop — neither has anywhere to put a rejection. The thrown message says the write landed, because it did; only reading the list back failed. A bare "could not add server" would send the user straight into a retry that trips the duplicate-id check, which is the dead end the original report described. Every consumer of these mutators already handles a rejection: ServerConfigModal and ServerRemoveConfirmModal into an inline error, useSettingsDraft and the pagination toggle into a toast, and both import flows into their per-entry outcome list. Closes #1914 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- .../src/test/core/react/useServers.test.tsx | 151 ++++++++++++++++++ core/react/useServers.ts | 77 ++++++--- 2 files changed, 210 insertions(+), 18 deletions(-) diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index c0d5260c..0c011109 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -1199,6 +1199,157 @@ describe("useServers", () => { } }); + /** + * #1914 — a write that lands but whose list reload fails must reject. + * + * `ServerConfigModal` renders `submitError` only for something `onSubmit` + * threw, so while the post-write refresh swallowed its failure into the + * `error` state the Add Server modal closed as though nothing had gone + * wrong. That is the "silent failure" in the report: the POST succeeded, + * the GET 500'd, and the user saw neither the new row nor an error. + * + * `failGetAfterWrite` lets the seed load and the write itself through, then + * fails every subsequent list read — the shape a container with no + * reachable keychain produced before #1848. + */ + function failGetAfterWrite(message: string): { + fetchFn: typeof fetch; + startFailing: () => void; + } { + let failing = false; + const fetchFn: typeof fetch = async (input, init) => { + const req = + input instanceof Request + ? input + : new Request(input as string | URL, init); + if (failing && req.method === "GET" && req.url.endsWith("/api/servers")) { + return new Response(JSON.stringify({ error: message }), { + status: 500, + headers: { "Content-Type": "application/json" }, + }); + } + return h.fetchFn(req); + }; + return { + fetchFn, + startFailing: () => { + failing = true; + }, + }; + } + + it("addServer rejects when the post-write list reload fails (#1914)", async () => { + const { fetchFn, startFailing } = failGetAfterWrite("keyring unavailable"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + + startFailing(); + await act(async () => { + await expect( + result.current.addServer("alpha", { type: "stdio", command: "node" }), + ).rejects.toThrow( + /The server was added, but the server list could not be reloaded: keyring unavailable/, + ); + }); + + // The write itself landed, and the message says so — a bare failure would + // send the user into a retry that trips the duplicate-id check. + expect(readConfig(h.configPath).mcpServers.alpha).toEqual({ + type: "stdio", + command: "node", + }); + // Still recorded in `error` for any list-level UI reading it. + await waitFor(() => + expect(result.current.error).toBe("keyring unavailable"), + ); + expect(result.current.loading).toBe(false); + }); + + it("updateServer rejects when the post-write list reload fails (#1914)", async () => { + const { fetchFn, startFailing } = failGetAfterWrite("disk full"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + const existing = result.current.servers[0]!.id; + + startFailing(); + await act(async () => { + await expect( + result.current.updateServer(existing, existing, { + type: "stdio", + command: "node", + }), + ).rejects.toThrow( + /The server was saved, but the server list could not be reloaded: disk full/, + ); + }); + }); + + it("updateServerSettings rejects when the post-write list reload fails (#1914)", async () => { + const { fetchFn, startFailing } = failGetAfterWrite("disk full"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + const existing = result.current.servers[0]!.id; + + startFailing(); + await act(async () => { + await expect( + result.current.updateServerSettings(existing, { + headers: [], + env: [], + metadata: [], + roots: [], + connectionTimeout: 5000, + requestTimeout: 30000, + taskTtl: 30000, + maxFetchRequests: 1000, + paginatedLists: true, + }), + ).rejects.toThrow( + /The server was saved, but the server list could not be reloaded: disk full/, + ); + }); + }); + + it("removeServer rejects when the post-write list reload fails (#1914)", async () => { + const { fetchFn, startFailing } = failGetAfterWrite("disk full"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + const existing = result.current.servers[0]!.id; + + startFailing(); + await act(async () => { + await expect(result.current.removeServer(existing)).rejects.toThrow( + /The server was removed, but the server list could not be reloaded: disk full/, + ); + }); + expect(readConfig(h.configPath).mcpServers[existing]).toBeUndefined(); + }); + + it("a plain refresh() still resolves on a failed list read, recording the error (#1914)", async () => { + // The rethrow is scoped to post-write reloads. `refresh()` is awaited by + // the mount effect and the SSE loop, neither of which has anywhere to put + // a rejection, so it must keep swallowing into `error`. + const { fetchFn, startFailing } = failGetAfterWrite("keyring unavailable"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + + startFailing(); + await act(async () => { + await expect(result.current.refresh()).resolves.toBeUndefined(); + }); + expect(result.current.error).toBe("keyring unavailable"); + }); + it("uses DEFAULT_SEED_CONFIG keys on the first load (seed-write contract)", async () => { const { result } = renderHook(() => useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }), diff --git a/core/react/useServers.ts b/core/react/useServers.ts index c9713344..bcba4690 100644 --- a/core/react/useServers.ts +++ b/core/react/useServers.ts @@ -132,33 +132,74 @@ export function useServers(opts: UseServersOptions): UseServersResult { const [loading, setLoading] = useState(true); const [error, setError] = useState(undefined); + // The list read itself, throwing on failure. Every refresh path goes + // through it; they differ only in what they do with a rejection. + const loadServers = useCallback(async (): Promise => { + const res = await doFetch(`${base}/api/servers`, { + method: "GET", + headers: buildHeaders(authToken, false), + }); + if (!res.ok) { + throw new Error(await readErrorMessage(res)); + } + const body = (await res.json()) as MCPConfig; + return mcpConfigToServerEntries(body); + }, [base, authToken, doFetch]); + // Inner refresh that the public `refresh` and the SSE-triggered background // refresh both share. `background` skips the loading-state toggle so // consumers rendering a spinner or skeleton don't flash on every external // mcp.json edit — the existing list stays on screen while the re-fetch // resolves. `error` is still reset / set either way so a real error // surfaces even from a background refresh. + // + // A failure is recorded in `error` and NOT rethrown: the two callers are + // the mount effect and the SSE loop, neither of which has anywhere to put + // a rejection. Post-write reloads use `refreshAfterWrite` instead. const refreshInternal = useCallback( async (background: boolean): Promise => { if (!background) setLoading(true); setError(undefined); try { - const res = await doFetch(`${base}/api/servers`, { - method: "GET", - headers: buildHeaders(authToken, false), - }); - if (!res.ok) { - throw new Error(await readErrorMessage(res)); - } - const body = (await res.json()) as MCPConfig; - setServers(mcpConfigToServerEntries(body)); + setServers(await loadServers()); } catch (err) { setError(err instanceof Error ? err.message : String(err)); } finally { if (!background) setLoading(false); } }, - [base, authToken, doFetch], + [loadServers], + ); + + // Post-write reload, used by every mutator. Unlike `refreshInternal` it + // rethrows, because a mutator's caller learns that something went wrong + // only from a rejection — `ServerConfigModal` renders `submitError` for a + // thrown error and nothing else, so a successful POST followed by a failed + // GET closed the Add Server modal as though the whole thing had worked + // (#1914). The reported failure was silent for exactly this reason. + // + // The message says the write landed, because it did: only reading the list + // back failed. A bare "could not add server" would send the user straight + // into a retry that trips the duplicate-id check, which is the dead end the + // original report described. + const refreshAfterWrite = useCallback( + async (verb: string): Promise => { + setLoading(true); + setError(undefined); + try { + setServers(await loadServers()); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + setError(message); + throw new Error( + `The server was ${verb}, but the server list could not be reloaded: ${message}`, + { cause: err }, + ); + } finally { + setLoading(false); + } + }, + [loadServers], ); const refresh = useCallback( @@ -241,9 +282,9 @@ export function useServers(opts: UseServersOptions): UseServersResult { if (!res.ok) { throw new Error(await readErrorMessage(res)); } - await refresh(); + await refreshAfterWrite("added"); }, - [base, authToken, doFetch, refresh], + [base, authToken, doFetch, refreshAfterWrite], ); const importSource = useCallback( @@ -282,9 +323,9 @@ export function useServers(opts: UseServersOptions): UseServersResult { if (!res.ok) { throw new Error(await readErrorMessage(res)); } - await refresh(); + await refreshAfterWrite("saved"); }, - [base, authToken, doFetch, refresh], + [base, authToken, doFetch, refreshAfterWrite], ); const updateServerSettings = useCallback( @@ -307,9 +348,9 @@ export function useServers(opts: UseServersOptions): UseServersResult { if (!res.ok) { throw new Error(await readErrorMessage(res)); } - await refresh(); + await refreshAfterWrite("saved"); }, - [base, authToken, doFetch, refresh], + [base, authToken, doFetch, refreshAfterWrite], ); const removeServer = useCallback( @@ -324,9 +365,9 @@ export function useServers(opts: UseServersOptions): UseServersResult { if (!res.ok) { throw new Error(await readErrorMessage(res)); } - await refresh(); + await refreshAfterWrite("removed"); }, - [base, authToken, doFetch, refresh], + [base, authToken, doFetch, refreshAfterWrite], ); const reorderServers = useCallback( From fd21d0d52801c0dcad3ecf8803957db8a0bdbef7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 09:39:47 -0400 Subject: [PATCH 02/35] fix(web): don't roll back an optimistic setting a failed reload didn't undo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review r1. `updateServerSettings` is also driven by the pagination toggle, which updates optimistically and — on any rejection — reverts the override and rolls the live client back to `prevSettings` (#1721). Making the post-write reload throw handed that catch a rejection whose PUT had *succeeded*, so it reverted the UI and the client to a value that no longer matched disk. Give the reload failure its own type, `ServerListReloadError`, and branch on it at that call site: report it, leave the optimistic state alone. The distinction is what the callers need — a failed write must roll back, a failed read-back must not — and a type carries it without the call site matching on a message. The `useServers` mock in App.test.tsx now spreads `importOriginal()` so the class is the genuine one; a stub would make the `instanceof` check vacuously false and the branch untestable. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 47 ++++++++++++++++++- clients/web/src/App.tsx | 28 +++++++---- .../src/test/core/react/useServers.test.tsx | 34 +++++++++++++- core/react/useServers.ts | 25 +++++++++- 4 files changed, 122 insertions(+), 12 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index fdf01413..3d95bf9d 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -225,7 +225,13 @@ const { updateServerSettingsSpy } = vi.hoisted(() => ({ const { clearToolsListChangedSpy } = vi.hoisted(() => ({ clearToolsListChangedSpy: vi.fn(), })); -vi.mock("@inspector/core/react/useServers.js", () => ({ +vi.mock("@inspector/core/react/useServers.js", async (importOriginal) => ({ + // `ServerListReloadError` is a real class the pagination toggle branches on + // with `instanceof`, so it must be the genuine one — a stub would make the + // check vacuously false and the #1914 branch untestable (#1914 review r1). + ...(await importOriginal< + typeof import("@inspector/core/react/useServers.js") + >()), useServers: vi.fn(() => ({ servers: [SERVER_A], addServer: vi.fn(), @@ -584,7 +590,10 @@ import { usePagedTools } from "@inspector/core/react/usePagedTools.js"; import { useMessageLog } from "@inspector/core/react/useMessageLog.js"; import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js"; import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft.js"; -import { useServers } from "@inspector/core/react/useServers.js"; +import { + ServerListReloadError, + useServers, +} from "@inspector/core/react/useServers.js"; import type { InspectorServerSettings, MessageEntry, @@ -2423,6 +2432,40 @@ describe("App paginated list pagination toggle (#1721)", () => { }; expect(lastPush?.paginatedLists).toBeFalsy(); }); + + it("keeps the optimistic toggle when only the list reload failed (#1914)", async () => { + // The mirror of the test above. `refreshAfterWrite` rejects with a + // `ServerListReloadError` when the PUT landed and only reading the list + // back failed — the setting IS on disk, so the #1721 rollback would put + // the UI and the live client on the value the user just changed away + // from, and contradict what a reload would show. + const user = userEvent.setup(); + updateServerSettingsSpy.mockRejectedValueOnce( + new ServerListReloadError( + "The server was saved, but the server list could not be reloaded: disk full", + ), + ); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("paginated-on")); + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + // Give the rejection a turn to land, then confirm nothing reverted it. + await act(async () => { + await Promise.resolve(); + }); + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"); + const client = clientInstances[0] as unknown as { + setServerSettings: ReturnType; + }; + const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as { + paginatedLists?: boolean; + }; + expect(lastPush?.paginatedLists).toBe(true); + }); }); // A background command runs through `runCommandInBackground`, which owns the diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 67f7d0ff..a79098ca 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -98,7 +98,10 @@ import { } from "@inspector/core/auth/index.js"; import { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js"; -import { useServers } from "@inspector/core/react/useServers.js"; +import { + ServerListReloadError, + useServers, +} from "@inspector/core/react/useServers.js"; import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft.js"; import { useClientSettingsDraft } from "@inspector/core/react/useClientSettingsDraft.js"; import { useEmaIdpLoginState } from "@inspector/core/react/useEmaIdpLoginState.js"; @@ -3761,14 +3764,23 @@ function App() { activeServerId, next, ).catch((err: unknown) => { - // Persist failed: revert the optimistic override (the effect only - // clears it when the persisted value changes, which won't happen here) - // and roll the live client setting back, so the UI and client reflect - // the value that's actually on disk rather than the failed edit (#1721). - setPaginatedListsOverride(null); - inspectorClient?.setServerSettings(prevSettings); + // A ServerListReloadError means the PUT landed and only reading the + // list back failed, so the new setting IS on disk (#1914). Rolling + // back here would put the UI and the live client on the *old* value + // and contradict it — report it and leave the optimistic state alone. + const reloadFailed = err instanceof ServerListReloadError; + if (!reloadFailed) { + // Persist failed: revert the optimistic override (the effect only + // clears it when the persisted value changes, which won't happen here) + // and roll the live client setting back, so the UI and client reflect + // the value that's actually on disk rather than the failed edit (#1721). + setPaginatedListsOverride(null); + inspectorClient?.setServerSettings(prevSettings); + } notifications.show({ - title: "Failed to save pagination setting", + title: reloadFailed + ? "Pagination setting saved, but the server list did not reload" + : "Failed to save pagination setting", message: err instanceof Error ? err.message : String(err), color: "red", }); diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index 0c011109..2ca1d334 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -10,7 +10,10 @@ import { act, renderHook, waitFor } from "@testing-library/react"; import { mkdtempSync, rmSync, writeFileSync, readFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { useServers } from "@inspector/core/react/useServers"; +import { + ServerListReloadError, + useServers, +} from "@inspector/core/react/useServers"; import { createRemoteApp } from "@inspector/core/mcp/remote/node/server"; import { DEFAULT_SEED_CONFIG } from "@inspector/core/mcp/serverList"; import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store"; @@ -1316,6 +1319,35 @@ describe("useServers", () => { }); }); + it("tags a post-write reload failure as ServerListReloadError, not a failed write (#1914)", async () => { + // The pagination toggle updates optimistically and reverts the live + // client on a rejection. That is right for a failed PUT and wrong for a + // failed reload — the setting IS on disk — so the two must be tellable + // apart by type, not by message matching. + const { fetchFn, startFailing } = failGetAfterWrite("disk full"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + + startFailing(); + await act(async () => { + await expect( + result.current.addServer("alpha", { type: "stdio", command: "node" }), + ).rejects.toBeInstanceOf(ServerListReloadError); + }); + + // A rejected *write* stays a plain Error, so a caller branching on the + // type still rolls back for the case the rollback was written for. + await act(async () => { + const err = await result.current + .addServer("alpha", { type: "stdio", command: "node" }) + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(Error); + expect(err).not.toBeInstanceOf(ServerListReloadError); + }); + }); + it("removeServer rejects when the post-write list reload fails (#1914)", async () => { const { fetchFn, startFailing } = failGetAfterWrite("disk full"); const { result } = renderHook(() => diff --git a/core/react/useServers.ts b/core/react/useServers.ts index bcba4690..3b66a0c8 100644 --- a/core/react/useServers.ts +++ b/core/react/useServers.ts @@ -62,6 +62,25 @@ export interface UseServersResult { importSource: (type: string) => Promise; } +/** + * Thrown when a mutation's *write* landed but reading the list back + * afterwards failed — see `refreshAfterWrite`. + * + * Distinct from a rejected write because the two call for opposite handling + * in an optimistic caller. The pagination toggle in `App.tsx` reverts its + * optimistic override and rolls the live client back on a rejection, which is + * right for a failed PUT and wrong here: the setting *is* on disk, so the + * rollback would show the user a value that no longer matches it. Callers + * that update optimistically must branch on this; callers that only report + * (the modals) can treat it as any other error. + */ +export class ServerListReloadError extends Error { + constructor(message: string, options?: ErrorOptions) { + super(message, options); + this.name = "ServerListReloadError"; + } +} + function buildHeaders( authToken: string | undefined, includeJsonBody: boolean, @@ -182,6 +201,10 @@ export function useServers(opts: UseServersOptions): UseServersResult { // back failed. A bare "could not add server" would send the user straight // into a retry that trips the duplicate-id check, which is the dead end the // original report described. + // + // The rejection is a `ServerListReloadError` rather than a bare `Error` so + // an optimistic caller can tell it from a failed write and skip its + // rollback — reverting here would contradict what is actually on disk. const refreshAfterWrite = useCallback( async (verb: string): Promise => { setLoading(true); @@ -191,7 +214,7 @@ export function useServers(opts: UseServersOptions): UseServersResult { } catch (err) { const message = err instanceof Error ? err.message : String(err); setError(message); - throw new Error( + throw new ServerListReloadError( `The server was ${verb}, but the server list could not be reloaded: ${message}`, { cause: err }, ); From 269ff5fefcfcfa9a13bbe823df9bdf745d096ded Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 09:58:56 -0400 Subject: [PATCH 03/35] fix(web): follow a rename whose list reload failed, and drop a double cast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review r2. `onConfigSubmit` moved `activeServerId` to the renamed id only after `updateServer` resolved. Once a failed list reload rejects, the rename is still on disk — so the active id stayed on the old one and the next successful refresh left it pointing at a row that no longer exists. Apply the rename from the catch when the rejection is a `ServerListReloadError`, then rethrow so the modal still shows the error. `addServerHighlighted` gets the same treatment: the row is on disk, so it belongs in the highlight batch the next successful refresh renders. `activeServerId` is not rendered anywhere, so the test reads it back through the one handler that hands it to a spy — the pagination toggle's `updateServerSettings(activeServerId, …)` — after letting the renamed row land as a later refresh would. It fails without the catch-side call. The new describe installs its own `useServers` return value and restores the previous implementation afterwards, since earlier describes' overrides outlive them (which is why the first draft's spy was never called). Also replaced the `as unknown as` in the new test with a single `EventTarget & {…}` intersection, per the TypeScript rules. The other 19 in this file are the same pattern and are left for a separate sweep. The third comment — that the pagination toggle's rollback baseline goes stale after a failed list read — is a real defect but pre-existing and unchanged by this PR; the end state is identical before and after it. Filed as #2089 rather than widening this fix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 103 ++++++++++++++++++++++++++++++++++- clients/web/src/App.tsx | 36 ++++++++++-- 2 files changed, 132 insertions(+), 7 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 3d95bf9d..fabee0b9 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -220,6 +220,12 @@ const SERVER_A = { const { updateServerSettingsSpy } = vi.hoisted(() => ({ updateServerSettingsSpy: vi.fn(() => Promise.resolve()), })); + +// Same idea for the edit-modal rename path (#1914): the test needs to make the +// PUT's *reload* fail, which only a spy it controls can do. +const { updateServerSpy } = vi.hoisted(() => ({ + updateServerSpy: vi.fn(() => Promise.resolve()), +})); // Stable spy for the tools list-changed acknowledgement, so a test can assert // the paginated Refresh clears the indicator (#1721). const { clearToolsListChangedSpy } = vi.hoisted(() => ({ @@ -235,7 +241,7 @@ vi.mock("@inspector/core/react/useServers.js", async (importOriginal) => ({ useServers: vi.fn(() => ({ servers: [SERVER_A], addServer: vi.fn(), - updateServer: vi.fn(), + updateServer: updateServerSpy, updateServerSettings: updateServerSettingsSpy, removeServer: vi.fn(), })), @@ -418,6 +424,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ onClearCompletedTasks: () => void; onRefreshTasks: () => void; onServerSettings: (id: string) => void; + onServerEdit: (id: string) => void; onClearProtocol: () => void; onReplayProtocol: (id: string) => void; onTogglePinProtocol: (id: string) => void; @@ -543,6 +550,9 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ + {/* The real server grid (and its Edit button) lives inside this mocked + view, so the edit modal is only reachable through its callback. */} + {Array.from(props.pinnedProtocolIds ?? []).join(",")} @@ -2458,7 +2468,10 @@ describe("App paginated list pagination toggle (#1721)", () => { await Promise.resolve(); }); expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"); - const client = clientInstances[0] as unknown as { + // Single intersection assertion rather than the `as unknown as` this file + // uses elsewhere: the fake client really is an EventTarget, and widening + // it keeps that relationship instead of erasing it (#1914 r2). + const client = clientInstances[0] as EventTarget & { setServerSettings: ReturnType; }; const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as { @@ -2468,6 +2481,92 @@ describe("App paginated list pagination toggle (#1721)", () => { }); }); +// `onConfigSubmit` moves `activeServerId` to the new id only after +// `updateServer` *resolves*. Once a failed list reload rejects, the rename is +// still on disk — so without the catch-side `followRename` the active id stays +// on the old one, and the next successful refresh leaves it pointing at a row +// that no longer exists (#1914 review r2). +describe("App edit rename with a failed list reload (#1914)", () => { + // Earlier describes install their own `useServers` return value, which + // outlives them — so this block installs its own and puts the previous + // implementation back, rather than trusting whatever leaked in. + let restoreUseServers: (() => void) | undefined; + + const serversResult = (ids: string[]): ReturnType => ({ + servers: ids.map((id) => ({ ...SERVER_A, id }) as ServerEntry), + loading: false, + error: undefined, + refresh: vi.fn().mockResolvedValue(undefined), + addServer: vi.fn().mockResolvedValue(undefined), + updateServer: updateServerSpy, + updateServerSettings: updateServerSettingsSpy, + removeServer: vi.fn().mockResolvedValue(undefined), + reorderServers: vi.fn().mockResolvedValue(undefined), + importSource: vi.fn().mockResolvedValue({ servers: {} }), + }); + + beforeEach(() => { + clientInstances.length = 0; + const previous = vi.mocked(useServers).getMockImplementation(); + restoreUseServers = previous + ? () => vi.mocked(useServers).mockImplementation(previous) + : undefined; + updateServerSpy.mockClear(); + updateServerSettingsSpy.mockClear(); + vi.mocked(useServers).mockReturnValue(serversResult(["A"])); + }); + + afterEach(() => { + restoreUseServers?.(); + }); + + it("follows the rename so the active id isn't orphaned (#1914)", async () => { + // `activeServerId` isn't rendered anywhere, so it's read back through the + // one handler that passes it straight to a spy: the pagination toggle + // calls `updateServerSettings(activeServerId, …)`. + const user = userEvent.setup(); + updateServerSpy.mockRejectedValueOnce( + new ServerListReloadError( + "The server was saved, but the server list could not be reloaded: disk full", + ), + ); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("edit-server")); + const idField = await screen.findByLabelText(/Server ID/); + await user.clear(idField); + await user.type(idField, "A-renamed"); + await user.click(screen.getByRole("button", { name: "Save" })); + + await waitFor(() => + expect(updateServerSpy).toHaveBeenCalledWith( + "A", + "A-renamed", + expect.anything(), + ), + ); + // The rejection still reaches the modal, which stays open showing it. + expect( + await screen.findByText(/could not be reloaded: disk full/), + ).toBeInTheDocument(); + + // Now let the row land, as a later successful refresh would, and read the + // active id back off the toggle. + vi.mocked(useServers).mockReturnValue(serversResult(["A-renamed"])); + await user.click(screen.getByRole("button", { name: "Cancel" })); + await user.click(screen.getByText("paginated-on")); + + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledWith( + "A-renamed", + expect.objectContaining({ paginatedLists: true }), + ), + ); + }); +}); + // A background command runs through `runCommandInBackground`, which owns the // rejection. Before #2049 the 17 `void runWithCommandAuthRecovery(...)` sites // had no handler, so any non-auth failure of the wrapped operation became an diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index a79098ca..169fd9c3 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -4075,8 +4075,20 @@ function App() { // opens (see the menu handlers). const addServerHighlighted = useCallback( async (id: string, config: MCPServerConfig) => { - await addServer(id, config); - setHighlightedServerIds((ids) => (ids.includes(id) ? ids : [...ids, id])); + const markAdded = () => + setHighlightedServerIds((ids) => + ids.includes(id) ? ids : [...ids, id], + ); + try { + await addServer(id, config); + } catch (err) { + // A failed list reload still means the row is on disk, so it belongs + // in the highlight batch — the next successful refresh is what + // renders it, and it would otherwise arrive unmarked (#1914 r2). + if (err instanceof ServerListReloadError) markAdded(); + throw err; + } + markAdded(); }, [addServer], ); @@ -4086,10 +4098,24 @@ function App() { async (id: string, config: MCPServerConfig) => { if (configModal?.mode === "edit" && configModal.targetId) { const originalId = configModal.targetId; - await updateServer(originalId, id, config); - if (originalId === activeServerId && id !== originalId) { - setActiveServerId(id); + const followRename = () => { + if (originalId === activeServerId && id !== originalId) { + setActiveServerId(id); + } + }; + try { + await updateServer(originalId, id, config); + } catch (err) { + // A `ServerListReloadError` means the PUT landed and only reading + // the list back failed — the rename IS on disk. The active + // selection has to follow it anyway, or the next successful + // refresh leaves `activeServerId` pointing at an id that no longer + // exists (#1914 r2). Still rethrow, so the modal shows the reload + // error rather than closing as if nothing happened. + if (err instanceof ServerListReloadError) followRename(); + throw err; } + followRename(); return; } // add or clone From b49d4f20f519c294933b85fd6be5b76a474ab86f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 10:14:22 -0400 Subject: [PATCH 04/35] fix(web): stop the remaining consumers calling a landed write a failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review r3. Three findings, all the same root: a ServerListReloadError rejects after the write succeeded, so any consumer still reading "rejection" as "the write failed" now says something false. - `useImportClientConfig.applyEntry` gave a successfully imported server a red "Failed" badge. It now reports the intended status and puts the reload message in `detail`, which renders in red beneath it either way — so the failure stays visible without claiming the row wasn't created. - The settings-draft toast said "Failed to save settings". It now branches its title; this toast is the only signal there, since the modal has usually closed by the time a flush rejects. - An edit modal left open by a failed reload kept targeting the old id. When the row landed under the new one, `configModalTarget` went undefined and ServerConfigModal's own reset blanked the open form and cleared the error it was showing. An edit/clone modal whose target has left the list now closes — adjusted during render via `useValueChange`, so no frame paints the blanked form. That also covers the ordinary case of an external mcp.json edit removing the server being edited. The reachability here is not remote: the write is what triggers the backend's change broadcast, so the SSE-driven refresh that lands the renamed row is the likely next event. Also adds the regression test r3 asked for on the add path — the highlight batch has to be marked from the catch for the same reason the rename is. Both new App tests and the import one fail without their fix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 79 +++++++++++++++---- clients/web/src/App.tsx | 34 +++++++- .../src/hooks/useImportClientConfig.test.tsx | 26 ++++++ .../web/src/hooks/useImportClientConfig.ts | 17 ++-- 4 files changed, 134 insertions(+), 22 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index fabee0b9..db5a28d5 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -223,8 +223,9 @@ const { updateServerSettingsSpy } = vi.hoisted(() => ({ // Same idea for the edit-modal rename path (#1914): the test needs to make the // PUT's *reload* fail, which only a spy it controls can do. -const { updateServerSpy } = vi.hoisted(() => ({ +const { updateServerSpy, addServerSpy } = vi.hoisted(() => ({ updateServerSpy: vi.fn(() => Promise.resolve()), + addServerSpy: vi.fn(() => Promise.resolve()), })); // Stable spy for the tools list-changed acknowledgement, so a test can assert // the paginated Refresh clears the indicator (#1721). @@ -240,7 +241,7 @@ vi.mock("@inspector/core/react/useServers.js", async (importOriginal) => ({ >()), useServers: vi.fn(() => ({ servers: [SERVER_A], - addServer: vi.fn(), + addServer: addServerSpy, updateServer: updateServerSpy, updateServerSettings: updateServerSettingsSpy, removeServer: vi.fn(), @@ -425,6 +426,8 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ onRefreshTasks: () => void; onServerSettings: (id: string) => void; onServerEdit: (id: string) => void; + onServerAdd: () => void; + highlightedServerIds?: string[]; onClearProtocol: () => void; onReplayProtocol: (id: string) => void; onTogglePinProtocol: (id: string) => void; @@ -550,9 +553,14 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ - {/* The real server grid (and its Edit button) lives inside this mocked - view, so the edit modal is only reachable through its callback. */} + {/* The real server grid (and its Add / Edit controls) lives inside this + mocked view, so the config modal is only reachable through these + callbacks — and the highlight batch only observable through this prop. */} + + + {(props.highlightedServerIds ?? []).join(",") || "none"} + {Array.from(props.pinnedProtocolIds ?? []).join(",")} @@ -2481,12 +2489,11 @@ describe("App paginated list pagination toggle (#1721)", () => { }); }); -// `onConfigSubmit` moves `activeServerId` to the new id only after -// `updateServer` *resolves*. Once a failed list reload rejects, the rename is -// still on disk — so without the catch-side `followRename` the active id stays -// on the old one, and the next successful refresh leaves it pointing at a row -// that no longer exists (#1914 review r2). -describe("App edit rename with a failed list reload (#1914)", () => { +// A post-write reload failure rejects *after* the write landed, so every piece +// of state `onConfigSubmit` used to apply on the resolve path has to be applied +// on the reject path too — the rename target and the highlight batch both +// describe a row that really is on disk (#1914 review r2 / r3). +describe("App config submit with a failed list reload (#1914)", () => { // Earlier describes install their own `useServers` return value, which // outlives them — so this block installs its own and puts the previous // implementation back, rather than trusting whatever leaked in. @@ -2497,7 +2504,7 @@ describe("App edit rename with a failed list reload (#1914)", () => { loading: false, error: undefined, refresh: vi.fn().mockResolvedValue(undefined), - addServer: vi.fn().mockResolvedValue(undefined), + addServer: addServerSpy, updateServer: updateServerSpy, updateServerSettings: updateServerSettingsSpy, removeServer: vi.fn().mockResolvedValue(undefined), @@ -2512,6 +2519,7 @@ describe("App edit rename with a failed list reload (#1914)", () => { ? () => vi.mocked(useServers).mockImplementation(previous) : undefined; updateServerSpy.mockClear(); + addServerSpy.mockClear(); updateServerSettingsSpy.mockClear(); vi.mocked(useServers).mockReturnValue(serversResult(["A"])); }); @@ -2530,7 +2538,7 @@ describe("App edit rename with a failed list reload (#1914)", () => { "The server was saved, but the server list could not be reloaded: disk full", ), ); - renderWithMantine(); + const { rerender } = renderWithMantine(); await user.click(screen.getByText("connect")); await waitFor(() => expect(clientInstances).toHaveLength(1)); @@ -2552,12 +2560,19 @@ describe("App edit rename with a failed list reload (#1914)", () => { await screen.findByText(/could not be reloaded: disk full/), ).toBeInTheDocument(); - // Now let the row land, as a later successful refresh would, and read the - // active id back off the toggle. + // Now let the row land, as the SSE refresh the write itself triggers + // would. The modal's target ("A") is gone, so it closes rather than + // blanking its own form — `initialId`/`initialConfig` would go undefined + // and ServerConfigModal's reset would wipe the open form and the error + // it is showing (#1914 r3). vi.mocked(useServers).mockReturnValue(serversResult(["A-renamed"])); - await user.click(screen.getByRole("button", { name: "Cancel" })); - await user.click(screen.getByText("paginated-on")); + rerender(); + await waitFor(() => + expect(screen.queryByLabelText(/Server ID/)).not.toBeInTheDocument(), + ); + // With the modal gone, read the active id back off the toggle. + await user.click(screen.getByText("paginated-on")); await waitFor(() => expect(updateServerSettingsSpy).toHaveBeenCalledWith( "A-renamed", @@ -2565,6 +2580,38 @@ describe("App edit rename with a failed list reload (#1914)", () => { ), ); }); + + it("keeps a successful add in the highlight batch when the reload failed (#1914)", async () => { + // `addServerHighlighted` marked the new id only after `addServer` + // resolved. The row is on disk either way, so on a reload failure it has + // to be marked from the catch — the next successful refresh is what + // renders it, and it would otherwise arrive unhighlighted. + const user = userEvent.setup(); + addServerSpy.mockRejectedValueOnce( + new ServerListReloadError( + "The server was added, but the server list could not be reloaded: disk full", + ), + ); + renderWithMantine(); + expect(screen.getByTestId("highlighted-servers")).toHaveTextContent("none"); + + await user.click(screen.getByText("add-server")); + await user.type(await screen.findByLabelText(/Server ID/), "brand-new"); + await user.type(screen.getByLabelText(/^Command/), "node"); + await user.click(screen.getByRole("button", { name: "Add" })); + + await waitFor(() => + expect(addServerSpy).toHaveBeenCalledWith("brand-new", expect.anything()), + ); + expect( + await screen.findByText(/could not be reloaded: disk full/), + ).toBeInTheDocument(); + await waitFor(() => + expect(screen.getByTestId("highlighted-servers")).toHaveTextContent( + "brand-new", + ), + ); + }); }); // A background command runs through `runCommandInBackground`, which owns the diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 169fd9c3..a99f212e 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -113,6 +113,7 @@ import { usePagedTools } from "@inspector/core/react/usePagedTools.js"; import { usePagedPrompts } from "@inspector/core/react/usePagedPrompts.js"; import { usePagedResources } from "@inspector/core/react/usePagedResources.js"; import { usePaginatedList } from "./hooks/usePaginatedList"; +import { useValueChange } from "./hooks/useValueChange"; import type { ListPaginationControlsProps } from "./components/elements/ListPaginationControls/ListPaginationControls"; import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js"; import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js"; @@ -4140,6 +4141,30 @@ function App() { return servers.find((s) => s.id === configModal.targetId); }, [configModal, servers]); + // An edit/clone modal whose target has left the list can no longer render a + // coherent form: `initialId`/`initialConfig` go undefined, and + // ServerConfigModal's own `useValueChange` reset then blanks the open form + // *and* clears the error it is showing. + // + // Only reachable since #1914 made the modal stay open on a failed post-write + // reload: rename A → B, the PUT lands, the reload fails, and the modal sits + // there targeting A. The write itself is what triggers the backend's change + // broadcast, so the SSE-driven background refresh that lands B (and drops A) + // is the *likely* next event, not a remote one. Closing is right — the edit + // is on disk and the list now agrees — and it covers the ordinary case too, + // where an external mcp.json edit removes the server being edited. + // + // Adjusted during render rather than in an effect (see `useValueChange`), so + // no frame ever paints the blanked form. + useValueChange( + configModal?.mode === "add" || configModal?.targetId === undefined + ? true + : servers.some((s) => s.id === configModal.targetId), + (targetPresent) => { + if (!targetPresent) setConfigModal(null); + }, + ); + const settingsModalTarget = useMemo(() => { if (!settingsModalTargetId) return undefined; return servers.find((s) => s.id === settingsModalTargetId); @@ -4188,7 +4213,14 @@ function App() { // didn't (especially painful for the OAuth client secret). onError: (id, err) => { notifications.show({ - title: `Failed to save settings for "${id}"`, + // A `ServerListReloadError` means the PUT landed and only reading the + // list back failed, so "Failed to save" would be false — and this + // toast is the user's only signal here, since the modal has usually + // closed by the time a flush rejects (#1914 r3). + title: + err instanceof ServerListReloadError + ? `Saved settings for "${id}", but the server list did not reload` + : `Failed to save settings for "${id}"`, message: err instanceof Error ? err.message : String(err), color: "red", }); diff --git a/clients/web/src/hooks/useImportClientConfig.test.tsx b/clients/web/src/hooks/useImportClientConfig.test.tsx index 9900f283..def42e7e 100644 --- a/clients/web/src/hooks/useImportClientConfig.test.tsx +++ b/clients/web/src/hooks/useImportClientConfig.test.tsx @@ -6,6 +6,7 @@ import { } from "./useImportClientConfig"; import type { ImportSourceResult } from "@inspector/core/mcp/import/index.js"; import type { MCPConfig } from "@inspector/core/mcp/types.js"; +import { ServerListReloadError } from "@inspector/core/react/useServers.js"; function config(...ids: string[]): MCPConfig { const mcpServers: MCPConfig["mcpServers"] = {}; @@ -207,6 +208,31 @@ describe("useImportClientConfig", () => { expect(outcome.detail).toBe("add failed"); }); + it("reports a reload failure as imported-with-a-warning, not failed (#1914)", async () => { + // A `ServerListReloadError` rejects *after* the write landed, so the entry + // really was added. A red "Failed" badge would contradict the error's own + // message and invite a retry that then collides with the row it claims + // wasn't created. The reason still shows, as `detail`. + const onAddServer = vi.fn(async () => { + throw new ServerListReloadError( + "The server was added, but the server list could not be reloaded: disk full", + ); + }); + const { result } = setup({ + onFetchSource: vi.fn(async () => foundSource(config("alpha"))), + onAddServer, + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + await act(async () => { + await result.current.runImport(); + }); + const outcome = result.current.outcomes[0]; + expect(outcome.status).toBe("added"); + expect(outcome.detail).toMatch(/could not be reloaded: disk full/); + }); + it("resolves conflicts by overwrite, skip, and rename", async () => { const onAddServer = vi.fn(async () => {}); const onUpdateServer = vi.fn(async () => {}); diff --git a/clients/web/src/hooks/useImportClientConfig.ts b/clients/web/src/hooks/useImportClientConfig.ts index 61a01478..c5e439f8 100644 --- a/clients/web/src/hooks/useImportClientConfig.ts +++ b/clients/web/src/hooks/useImportClientConfig.ts @@ -8,6 +8,7 @@ import { type ImportSourceResult, } from "@inspector/core/mcp/import/index.js"; import { validateStoreId } from "@inspector/core/storage/store-id.js"; +import { ServerListReloadError } from "@inspector/core/react/useServers.js"; import type { MCPConfig, MCPServerConfig } from "@inspector/core/mcp/types.js"; export type ImportPhase = "select" | "loading" | "review" | "summary"; @@ -198,11 +199,17 @@ export function useImportClientConfig({ await write(); return { id, status: outcome }; } catch (err) { - return { - id, - status: "failed", - detail: err instanceof Error ? err.message : String(err), - }; + const detail = err instanceof Error ? err.message : String(err); + // A `ServerListReloadError` means the write landed and only reading the + // list back failed, so this entry really was imported. Reporting it as + // `failed` would contradict the error's own message and invite a retry + // that then collides with the row it says wasn't created (#1914 r3). + // The status is the truth about the write; `detail` renders in red + // beneath it either way, so the reload failure is still visible. + if (err instanceof ServerListReloadError) { + return { id, status: outcome, detail }; + } + return { id, status: "failed", detail }; } } From 6c1cc6c1ae0a1cf9774eb625e2b07730d82b5b42 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 10:27:28 -0400 Subject: [PATCH 05/35] test(web): cover the settings toast's reload-failure title MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review r4. The r3 title branch was the one modified behavior with no test of its own — the existing ServerListReloadError cases cover the pagination, add, and rename paths only, so a regression back to "Failed to save settings" would have gone green. `useSettingsDraft` is mocked in this suite, so the test invokes the `onError` App handed it and asserts both titles: the reload-failure one, and that a genuine write failure still reads as a failure. Forcing the branch off fails it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index db5a28d5..032f46e9 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -2581,6 +2581,39 @@ describe("App config submit with a failed list reload (#1914)", () => { ); }); + it("labels a settings save whose reload failed as saved, not failed (#1914)", () => { + // The settings draft debounces and flushes on close, so this toast is the + // user's only signal — a flush that rejects usually does so after the + // modal is gone. `useSettingsDraft` is mocked here, so the App's `onError` + // is invoked directly with the options it was handed. + renderWithMantine(); + const onError = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0].onError; + expect(onError).toBeDefined(); + + act(() => { + onError!( + "A", + new ServerListReloadError( + "The server was saved, but the server list could not be reloaded: disk full", + ), + ); + }); + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: 'Saved settings for "A", but the server list did not reload', + message: expect.stringContaining("disk full"), + }), + ); + + // A genuinely failed write still reads as a failure. + act(() => { + onError!("A", new Error("disk full")); + }); + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ title: 'Failed to save settings for "A"' }), + ); + }); + it("keeps a successful add in the highlight batch when the reload failed (#1914)", async () => { // `addServerHighlighted` marked the new id only after `addServer` // resolved. The row is on disk either way, so on a reload failure it has From b33532c024f1bd8fb1550f80dff5595bd9d33c99 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 12:23:40 -0400 Subject: [PATCH 06/35] feat: allow complex JSON values in request _meta MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1910 `_meta` was typed `Record` end to end in the Inspector — on disk as a `{ key, value }[]` pair array, in the Server Settings form as key/value rows, and in every request verb's per-call metadata parameter. Nothing in the MCP spec restricts a `_meta` value to a string, and the SDK models the field as a `z.looseObject`, so the restriction was ours alone. Widen the whole path to `RequestMetadata` (`Record`): - Per-server default metadata is a JSON **object** on disk and in memory. The pre-#1910 pair array is still read (`normalizeStoredMetadata`) so an existing `mcp.json` keeps working; it is never written back, so a save migrates the entry. - The Server Settings → Request Metadata section is a JSON editor (`JsonObjectInput`) instead of key/value rows — rows cannot express an object value. It keeps the typed text as the display source and emits only parsed JSON, the same draft split `SchemaJsonField` uses, so the box stays typeable; invalid text is flagged inline and not applied. - The CLI stops flattening `--metadata` / `--tool-metadata` through `metaValueToString`. Those values were already JSON-parsed, so `trace={"id":"abc"}` used to reach the wire as the *string* `{"id":"abc"}` and now reaches it as an object. - `servers/show` redaction replaces a sensitive key's whole value rather than a string, so a structured value cannot leak through its members. - `progressTokenOf` narrows the reserved `progressToken` key back to `string | number` before it reaches the SDK; any other JSON still travels inside `_meta` but wires up no progress callback. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- clients/cli/README.md | 6 +- .../cli/__tests__/helpers/oauth-test-fakes.ts | 2 +- clients/cli/__tests__/metadata.test.ts | 27 +-- clients/cli/__tests__/method-types.test.ts | 9 - .../__tests__/programmatic-ergonomics.test.ts | 2 +- clients/cli/__tests__/servers-list.test.ts | 23 ++- clients/cli/src/cli.ts | 23 +-- clients/cli/src/handlers/collect-app-info.ts | 3 +- clients/cli/src/handlers/connect-timeout.ts | 2 +- clients/cli/src/handlers/method-types.ts | 15 +- clients/cli/src/handlers/servers-list.ts | 13 +- clients/tui/__tests__/App.test.tsx | 2 +- .../tui/__tests__/helpers/oauth-test-fakes.ts | 2 +- clients/tui/src/App.tsx | 10 +- clients/web/src/App.test.tsx | 2 +- clients/web/src/App.tsx | 12 +- .../JsonObjectInput.stories.tsx | 92 ++++++++++ .../JsonObjectInput/JsonObjectInput.test.tsx | 172 ++++++++++++++++++ .../JsonObjectInput/JsonObjectInput.tsx | 113 ++++++++++++ .../ServerSettingsForm.stories.tsx | 46 ++--- .../ServerSettingsForm.test.tsx | 83 +++------ .../ServerSettingsForm/ServerSettingsForm.tsx | 42 ++--- .../ServerSettingsModal.stories.tsx | 6 +- .../ServerSettingsModal.test.tsx | 95 ++-------- .../ServerSettingsModal.tsx | 22 +-- .../web/src/test/core/client/runner.test.ts | 12 +- ...nspectorClient-peer-handler-timing.test.ts | 2 +- .../src/test/core/mcp/node/servers.test.ts | 2 +- .../mcp/remote/remoteClientTransport.test.ts | 2 +- .../web/src/test/core/mcp/serverList.test.ts | 128 +++++++++++-- .../mcp/state/managedPromptsState.test.ts | 2 +- .../managedResourceTemplatesState.test.ts | 2 +- .../mcp/state/managedResourcesState.test.ts | 2 +- .../core/mcp/state/managedToolsState.test.ts | 2 +- .../core/mcp/state/pagedPromptsState.test.ts | 2 +- .../mcp/state/pagedResourcesState.test.ts | 2 +- .../core/mcp/state/pagedToolsState.test.ts | 2 +- .../src/test/core/react/useServers.test.tsx | 10 +- .../inspectorClient-coverage-backfill.test.ts | 4 +- .../mcp/inspectorClient-modern-era.test.ts | 6 +- .../integration/mcp/inspectorClient.test.ts | 52 +++++- .../integration/mcp/node/transport.test.ts | 8 +- .../mcp/remote/server-extra-coverage.test.ts | 6 +- .../mcp/remote/servers-route.test.ts | 83 ++++----- .../integration/mcp/remote/transport.test.ts | 2 +- core/mcp/__tests__/fakeInspectorClient.ts | 11 +- core/mcp/inspectorClient.ts | 107 ++++++----- core/mcp/inspectorClientEventTarget.ts | 3 +- core/mcp/inspectorClientProtocol.ts | 29 +-- core/mcp/node/servers.ts | 2 +- core/mcp/remote/node/server.ts | 28 ++- core/mcp/serverList.ts | 66 ++++++- core/mcp/state/managedListState.ts | 11 +- core/mcp/state/pagedPromptsState.ts | 3 +- core/mcp/state/pagedResourceTemplatesState.ts | 3 +- core/mcp/state/pagedResourcesState.ts | 3 +- core/mcp/types.ts | 49 +++-- core/react/usePagedPrompts.ts | 5 +- core/react/usePagedResourceTemplates.ts | 5 +- core/react/usePagedResources.ts | 5 +- docs/mcp-server-configuration.md | 14 +- 61 files changed, 1004 insertions(+), 495 deletions(-) create mode 100644 clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.stories.tsx create mode 100644 clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx create mode 100644 clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx diff --git a/clients/cli/README.md b/clients/cli/README.md index d5c033df..976f2ec6 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -110,15 +110,15 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--prompt-name ` | Prompt name (for `prompts/get`). | | `--prompt-args ` | Prompt arguments; repeat for multiple. | | `--log-level ` | Logging level for `logging/setLevel` (e.g. `debug`, `info`). | -| `--metadata ` | General metadata (key=value); applied to all methods. | -| `--tool-metadata ` | Tool-specific metadata for `tools/call`. | +| `--metadata ` | General `_meta` entries (key=value); applied to all methods. The value is JSON-parsed when it parses, so `trace={"id":"abc"}` sends a real object and `n=3` a number; anything that is not valid JSON is sent as the literal string. | +| `--tool-metadata ` | Tool-specific `_meta` entries for `tools/call`. Same JSON-parsed value handling as `--metadata`. | | `--connect-timeout ` | Connection timeout in ms. Defaults to `15000` for ad-hoc `--server-url`/target runs (so a black-holed host fails fast) and to the file-level timeout for `--catalog`/`--config` runs. `0` disables the timeout. | | `--app-info` | Probe a tool's MCP App UI metadata without invoking it. With `--method tools/call --tool-name `: prints one JSON line (`hasApp`, `resourceUri`, `csp`, `permissions`, `domain`, …) and exits `0` if the tool has an app or `2` (`no_app`) if not. With `--method tools/list`: emits NDJSON — one app-info line per tool over a single connection. | | `--format ` | Output format. `text` (default) pretty-prints the result. `json` emits a single JSON object on stdout (`{ "result": … }`, plus `{ "appInfo": … }` as a sibling key for App tools) with no banners, so the whole output pipes cleanly into `jq`. | | `--relogin` | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. Requires an HTTP/SSE URL (rejected for stdio). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth` / catalog short-circuits. | | `--stored-auth-only` | **CI / non-interactive safe:** never start interactive OAuth / step-up (and never auto-open a browser); use the shared store if present, otherwise fail immediately with `auth_required`. Prefer this over a bare pipe/CI run that would otherwise attempt interactive login. | -`servers/show` redacts secret-bearing fields (`env` values, sensitive headers / `settings.metadata` keys, `requestInit` / `eventSourceInit` headers, `oauthClientSecret`). It does **not** scrub credentials embedded in a server `url` (userinfo or query tokens) or in stdio `args` — treat `detail` / raw URL fields as potentially sensitive before pasting into issues. +`servers/show` redacts secret-bearing fields (`env` values, sensitive headers, sensitive `settings.metadata` keys whose whole value is replaced whether or not it is structured, `requestInit` / `eventSourceInit` headers, `oauthClientSecret`). It does **not** scrub credentials embedded in a server `url` (userinfo or query tokens) or in stdio `args` — treat `detail` / raw URL fields as potentially sensitive before pasting into issues. #### App probing (`--app-info`) and machine-readable output (`--format json`) diff --git a/clients/cli/__tests__/helpers/oauth-test-fakes.ts b/clients/cli/__tests__/helpers/oauth-test-fakes.ts index b53e1570..8c5e91f9 100644 --- a/clients/cli/__tests__/helpers/oauth-test-fakes.ts +++ b/clients/cli/__tests__/helpers/oauth-test-fakes.ts @@ -63,7 +63,7 @@ export function makeFakeServerSettings( ): InspectorServerSettings { return { headers: [], - metadata: [], + metadata: {}, env: [], connectionTimeout: 0, requestTimeout: 0, diff --git a/clients/cli/__tests__/metadata.test.ts b/clients/cli/__tests__/metadata.test.ts index b5867d50..a4cb05fc 100644 --- a/clients/cli/__tests__/metadata.test.ts +++ b/clients/cli/__tests__/metadata.test.ts @@ -373,16 +373,17 @@ describe("Metadata Tests", () => { expectCliSuccess(result); - // Validate metadata values are sent as strings + // A JSON-parseable value goes out as that JSON type, not as its + // string spelling (#1910) — `_meta` takes any JSON. const recordedRequests = server.getRecordedRequests(); const toolsListRequest = recordedRequests.find( (r) => r.method === "tools/list", ); expect(toolsListRequest).toBeDefined(); expect(toolsListRequest?.metadata).toEqual({ - integer_value: "42", - decimal_value: "3.14159", - negative_value: "-10", + integer_value: 42, + decimal_value: 3.14159, + negative_value: -10, }); } finally { await server.stop(); @@ -428,7 +429,7 @@ describe("Metadata Tests", () => { } }); - it("JSON.stringifies object/array metadata (not String → [object Object])", async () => { + it("sends object/array/boolean metadata as real JSON, not stringified (#1910)", async () => { const server = createTestServerHttp({ serverInfo: createTestServerInfo(), tools: [createEchoTool()], @@ -456,9 +457,9 @@ describe("Metadata Tests", () => { (r) => r.method === "tools/list", ); expect(toolsListRequest?.metadata).toEqual({ - nested: '{"key":"value"}', - list: "[1,2,3]", - flag: "true", + nested: { key: "value" }, + list: [1, 2, 3], + flag: true, }); } finally { await server.stop(); @@ -819,7 +820,7 @@ describe("Metadata Tests", () => { ); expect(toolsListRequest).toBeDefined(); expect(toolsListRequest?.metadata).toEqual({ - integration_test: "true", + integration_test: true, test_phase: "all_methods", }); } finally { @@ -866,8 +867,8 @@ describe("Metadata Tests", () => { ); expect(toolCallRequest).toBeDefined(); expect(toolCallRequest?.metadata).toEqual({ - session_id: "12345", - user_id: "67890", + session_id: 12345, + user_id: 67890, timestamp: "2024-01-01T00:00:00Z", request_id: "req-abc-123", tool_session: "session-xyz-789", @@ -919,8 +920,8 @@ describe("Metadata Tests", () => { expect(toolCallRequest).toBeDefined(); expect(toolCallRequest?.metadata).toEqual({ valid_key: "valid_value", - numeric_key: "123", - boolean_key: "true", + numeric_key: 123, + boolean_key: true, json_key: '\'{"test":"value"}\'', // Single quotes are preserved special_key: "!@#$%^&*()", unicode_key: "🚀🎉✨", diff --git a/clients/cli/__tests__/method-types.test.ts b/clients/cli/__tests__/method-types.test.ts index 42c71025..85230043 100644 --- a/clients/cli/__tests__/method-types.test.ts +++ b/clients/cli/__tests__/method-types.test.ts @@ -1,7 +1,6 @@ import { describe, it, expect } from "vitest"; import { isOneShotMethod, - metaValueToString, ONE_SHOT_METHODS, SESSION_RPC_METHODS, } from "../src/handlers/method-types.js"; @@ -26,11 +25,3 @@ describe("ONE_SHOT_METHODS", () => { expect(ONE_SHOT_METHODS).not.toContain("logging/tail"); }); }); - -describe("metaValueToString", () => { - it("passes strings through and JSON-encodes structured values", () => { - expect(metaValueToString("plain")).toBe("plain"); - expect(metaValueToString({ a: 1 })).toBe('{"a":1}'); - expect(metaValueToString([1, 2])).toBe("[1,2]"); - }); -}); diff --git a/clients/cli/__tests__/programmatic-ergonomics.test.ts b/clients/cli/__tests__/programmatic-ergonomics.test.ts index 1c6fdee7..88f42f10 100644 --- a/clients/cli/__tests__/programmatic-ergonomics.test.ts +++ b/clients/cli/__tests__/programmatic-ergonomics.test.ts @@ -12,7 +12,7 @@ import { describe("withConnectTimeout", () => { const baseSettings: InspectorServerSettings = { headers: [{ key: "X-Test", value: "1" }], - metadata: [], + metadata: {}, env: [], connectionTimeout: 42, requestTimeout: 0, diff --git a/clients/cli/__tests__/servers-list.test.ts b/clients/cli/__tests__/servers-list.test.ts index 8ac7b599..5772ab3e 100644 --- a/clients/cli/__tests__/servers-list.test.ts +++ b/clients/cli/__tests__/servers-list.test.ts @@ -219,10 +219,14 @@ describe("showServerEntry / servers/show", () => { { key: "Authorization", value: "Bearer x" }, { key: "X-Custom", value: "ok" }, ], - metadata: [ - { key: "Authorization", value: "Bearer meta" }, - { key: "X-Custom", value: "ok" }, - ], + metadata: { + Authorization: "Bearer meta", + "X-Custom": "ok", + // A structured value under a sensitive key: the redaction must replace + // the whole value, not walk into it (#1910). + "X-Api-Key": { primary: "sk-live-1", fallback: "sk-live-2" }, + nested: { keep: true }, + }, env: [ { key: "TOKEN", value: "secret" }, { key: "", value: "still-secret" }, @@ -242,10 +246,13 @@ describe("showServerEntry / servers/show", () => { { key: "Authorization", value: "[redacted]" }, { key: "X-Custom", value: "ok" }, ]); - expect(sanitized.metadata).toEqual([ - { key: "Authorization", value: "[redacted]" }, - { key: "X-Custom", value: "ok" }, - ]); + expect(sanitized.metadata).toEqual({ + Authorization: "[redacted]", + "X-Custom": "ok", + "X-Api-Key": "[redacted]", + // A structured value under a non-sensitive key survives intact. + nested: { keep: true }, + }); expect(sanitized.env).toEqual([ { key: "TOKEN", value: "[redacted]" }, { key: "", value: "[redacted]" }, diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index f4d418ae..35ca3dd9 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -33,7 +33,6 @@ import { consumeMethodOutcome } from "./handlers/consume-outcome.js"; import { runMethod } from "./handlers/run-method.js"; import { isOneShotMethod, - metaValueToString, ONE_SHOT_METHODS, type MethodArgs, } from "./handlers/method-types.js"; @@ -999,22 +998,12 @@ async function parseArgs(argv?: string[]): Promise { promptName: options.promptName, promptArgs: options.promptArgs, logLevel: options.logLevel, - metadata: options.metadata - ? Object.fromEntries( - Object.entries(options.metadata).map(([key, value]) => [ - key, - metaValueToString(value), - ]), - ) - : undefined, - toolMeta: options.toolMetadata - ? Object.fromEntries( - Object.entries(options.toolMetadata).map(([key, value]) => [ - key, - metaValueToString(value), - ]), - ) - : undefined, + // `--metadata`/`--tool-metadata` values are parsed as JSON, and `_meta` + // takes any JSON — so they go through unflattened (#1910). They used to be + // squeezed through `metaValueToString`, which sent `{"a":1}` as the + // *string* `'{"a":1}'`. + metadata: options.metadata, + toolMeta: options.toolMetadata, appInfo: options.appInfo === true, format: options.format, }; diff --git a/clients/cli/src/handlers/collect-app-info.ts b/clients/cli/src/handlers/collect-app-info.ts index aa2df1fc..d46e3754 100644 --- a/clients/cli/src/handlers/collect-app-info.ts +++ b/clients/cli/src/handlers/collect-app-info.ts @@ -2,6 +2,7 @@ import { InspectorClient } from "@inspector/core/mcp/index.js"; import { extractAppInfo } from "@inspector/core/mcp/apps.js"; import type { AppInfo } from "@inspector/core/mcp/apps.js"; import type { CliAppInfo } from "./method-types.js"; +import type { RequestMetadata } from "@inspector/core/mcp/types.js"; /** * Build the CLI's app-info for a tool. Never throws — failures fold into @@ -10,7 +11,7 @@ import type { CliAppInfo } from "./method-types.js"; export async function collectAppInfo( client: Pick, tool: Parameters[0], - metadata: Record | undefined, + metadata: RequestMetadata | undefined, ): Promise { let base: AppInfo; try { diff --git a/clients/cli/src/handlers/connect-timeout.ts b/clients/cli/src/handlers/connect-timeout.ts index 7d5a7b2c..8bd8cf65 100644 --- a/clients/cli/src/handlers/connect-timeout.ts +++ b/clients/cli/src/handlers/connect-timeout.ts @@ -22,7 +22,7 @@ export function withConnectTimeout( if (settings) return { ...settings, connectionTimeout }; return { headers: [], - metadata: [], + metadata: {}, env: [], connectionTimeout, requestTimeout: 0, diff --git a/clients/cli/src/handlers/method-types.ts b/clients/cli/src/handlers/method-types.ts index b0505425..424b941b 100644 --- a/clients/cli/src/handlers/method-types.ts +++ b/clients/cli/src/handlers/method-types.ts @@ -1,4 +1,5 @@ import type { JsonValue } from "@inspector/core/mcp/index.js"; +import type { RequestMetadata } from "@inspector/core/mcp/types.js"; import type { AppInfo } from "@inspector/core/mcp/apps.js"; import type { LoggingLevel } from "@modelcontextprotocol/client"; import type { OutputFormat } from "./format-output.js"; @@ -20,8 +21,8 @@ export type MethodArgs = { logLevel?: LoggingLevel; toolName?: string; toolArg?: Record; - toolMeta?: Record; - metadata?: Record; + toolMeta?: RequestMetadata; + metadata?: RequestMetadata; appInfo?: boolean; format?: OutputFormat; /** Task id for tasks/get, tasks/cancel, tasks/result. */ @@ -110,13 +111,3 @@ export type OneShotMethod = (typeof ONE_SHOT_METHODS)[number]; export function isOneShotMethod(method: string): method is OneShotMethod { return (ONE_SHOT_METHODS as readonly string[]).includes(method); } - -/** - * Encode a metadata / tool-metadata value for MethodArgs (string map). - * Strings pass through; objects/arrays use JSON.stringify so structure is kept - * (avoids String(obj) → "[object Object]"). - */ -export function metaValueToString(value: JsonValue): string { - if (typeof value === "string") return value; - return JSON.stringify(value); -} diff --git a/clients/cli/src/handlers/servers-list.ts b/clients/cli/src/handlers/servers-list.ts index 78bd896f..d0247851 100644 --- a/clients/cli/src/handlers/servers-list.ts +++ b/clients/cli/src/handlers/servers-list.ts @@ -157,10 +157,15 @@ export function sanitizeServerSettings( key: h.key, value: isSensitiveHeader(h.key) ? REDACTED : h.value, })), - metadata: (settings.metadata ?? []).map((m) => ({ - key: m.key, - value: isSensitiveHeader(m.key) ? REDACTED : m.value, - })), + // Metadata is a JSON object whose values may be any JSON (#1910), so the + // redaction replaces the whole value rather than a string — a sensitive + // key holding an object must not leak through its members. + metadata: Object.fromEntries( + Object.entries(settings.metadata ?? {}).map(([key, value]) => [ + key, + isSensitiveHeader(key) ? REDACTED : value, + ]), + ), env: (settings.env ?? []).map((e) => ({ key: e.key, value: REDACTED, diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index a66fe603..3e749dff 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -340,7 +340,7 @@ function oneEmaHttp(): Record { config: { type: "streamable-http", url: "http://localhost:8080/mcp" }, settings: { requestTimeout: 0, - metadata: [], + metadata: {}, headers: [], env: [], roots: [], diff --git a/clients/tui/__tests__/helpers/oauth-test-fakes.ts b/clients/tui/__tests__/helpers/oauth-test-fakes.ts index efff339e..5e6e0fae 100644 --- a/clients/tui/__tests__/helpers/oauth-test-fakes.ts +++ b/clients/tui/__tests__/helpers/oauth-test-fakes.ts @@ -19,7 +19,7 @@ export function makeFakeServerSettings( ): InspectorServerSettings { return { headers: [], - metadata: [], + metadata: {}, env: [], connectionTimeout: 0, requestTimeout: 0, diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 240bbd82..df88e3a9 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -291,13 +291,9 @@ function App({ transport: createTransportNode, logger: getTuiLogger(), }; - const defaultMetadata = savedSettings?.metadata - ? Object.fromEntries( - savedSettings.metadata - .filter((m) => m.key.trim() !== "") - .map((m) => [m.key, m.value]), - ) - : undefined; + // Per-server default `_meta` is already a JSON object (#1910) — no + // pair-array flattening left to do; `{}` means "no defaults". + const defaultMetadata = savedSettings?.metadata; const clientAuthOptions = buildRunnerClientAuthOptions( clientConfig, savedSettings, diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index fdf01413..e5056190 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -1635,7 +1635,7 @@ const settingsWithRoots = ( ): InspectorServerSettings => ({ headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 67f7d0ff..1be3d046 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -384,7 +384,7 @@ async function replayProtocolRequest( const EMPTY_SETTINGS: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: DEFAULT_TASK_TTL_MS, @@ -2466,13 +2466,9 @@ function App() { const activeCimdUrl = getActiveCimdClientMetadataUrl(clientConfig); // Flatten the persisted settings into the InspectorClient options shape. // Empty / zero values stay unset so the SDK defaults apply. - const defaultMetadata = savedSettings?.metadata - ? Object.fromEntries( - savedSettings.metadata - .filter((m) => m.key.trim() !== "") - .map((m) => [m.key, m.value]), - ) - : undefined; + // Per-server default `_meta` is already a JSON object (#1910) — no + // pair-array flattening left to do; `{}` means "no defaults". + const defaultMetadata = savedSettings?.metadata; const serverAuthorizationParams = savedSettings ? oauthAuthorizationParamsFromSettings(savedSettings) : undefined; diff --git a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.stories.tsx b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.stories.tsx new file mode 100644 index 00000000..51405384 --- /dev/null +++ b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.stories.tsx @@ -0,0 +1,92 @@ +import { useState } from "react"; +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, userEvent, within } from "storybook/test"; +import type { JsonObject } from "@inspector/core/json/jsonUtils.js"; +import { JsonObjectInput } from "./JsonObjectInput"; + +/** + * The component is controlled, so every story drives it through a small + * stateful wrapper — otherwise the box would not accept a keystroke and the + * play functions could not exercise the draft behavior at all. + */ +function ControlledJsonObjectInput({ initial }: { initial: JsonObject }) { + const [value, setValue] = useState(initial); + return ( + + ); +} + +const meta: Meta = { + title: "Elements/JsonObjectInput", + component: ControlledJsonObjectInput, +}; + +export default meta; +type Story = StoryObj; + +/** A structured payload renders as formatted JSON, nesting intact. */ +export const Populated: Story = { + args: { + initial: { + tenant: "acme", + trace: { id: "abc123", sampled: true }, + features: ["apps", "tasks"], + }, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const box = await canvas.findByLabelText("Request metadata JSON"); + await expect(JSON.parse((box as HTMLTextAreaElement).value)).toEqual({ + tenant: "acme", + trace: { id: "abc123", sampled: true }, + features: ["apps", "tasks"], + }); + }, +}; + +/** An empty payload opens as `{}` and carries no error. */ +export const Empty: Story = { + args: { initial: {} }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const box = await canvas.findByLabelText("Request metadata JSON"); + await expect(box).toHaveValue("{}"); + await expect(canvas.queryByText(/Not valid JSON/)).toBeNull(); + }, +}; + +/** + * Half-typed text is displayed as typed and flagged, rather than being + * re-escaped back into the box each keystroke (the #1928 failure this draft + * split exists to avoid). + */ +export const InvalidDraft: Story = { + args: { initial: {} }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const box = await canvas.findByLabelText("Request metadata JSON"); + await userEvent.clear(box); + await userEvent.type(box, '{{"a":'); + await expect(box).toHaveValue('{"a":'); + await expect(await canvas.findByText(/Not valid JSON/)).toBeVisible(); + }, +}; + +/** Valid JSON that is not an object is rejected with its own message. */ +export const NotAnObject: Story = { + args: { initial: {} }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const box = await canvas.findByLabelText("Request metadata JSON"); + await userEvent.clear(box); + await userEvent.type(box, "[[1,2]"); + await expect( + await canvas.findByText(/Must be a JSON object/), + ).toBeVisible(); + }, +}; diff --git a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx new file mode 100644 index 00000000..37d02dfb --- /dev/null +++ b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx @@ -0,0 +1,172 @@ +import { useState } from "react"; +import { describe, it, expect, vi } from "vitest"; +import { screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import type { JsonObject } from "@inspector/core/json/jsonUtils.js"; +import { renderWithMantine } from "../../../test/renderWithMantine"; +import { JsonObjectInput, parseObjectDraft } from "./JsonObjectInput"; + +const LABEL = "Payload JSON"; + +/** Controlled harness: the component never owns the value, only the draft. */ +function Harness({ + initial, + onChange, +}: { + initial: JsonObject; + onChange?: (v: JsonObject) => void; +}) { + const [value, setValue] = useState(initial); + return ( + <> + { + setValue(next); + onChange?.(next); + }} + /> + {JSON.stringify(value)} + + ); +} + +function box() { + return screen.getByLabelText(LABEL) as HTMLTextAreaElement; +} + +describe("parseObjectDraft", () => { + it("reads empty text as the empty object rather than an error", () => { + expect(parseObjectDraft("")).toEqual({ ok: true, value: {} }); + expect(parseObjectDraft(" \n ")).toEqual({ ok: true, value: {} }); + }); + + it("accepts a JSON object with values of any JSON type", () => { + const result = parseObjectDraft( + '{"s":"a","n":1,"b":true,"z":null,"arr":[1,{"k":2}],"o":{"deep":{"er":1}}}', + ); + expect(result).toEqual({ + ok: true, + value: { + s: "a", + n: 1, + b: true, + z: null, + arr: [1, { k: 2 }], + o: { deep: { er: 1 } }, + }, + }); + }); + + it("rejects text that is not JSON", () => { + expect(parseObjectDraft('{"a":')).toEqual({ + ok: false, + error: "Not valid JSON — changes are not applied", + }); + }); + + it.each([ + ["an array", "[1,2]"], + ["a string", '"hello"'], + ["a number", "42"], + ["null", "null"], + ])("rejects valid JSON that is not an object: %s", (_label, text) => { + const result = parseObjectDraft(text); + expect(result.ok).toBe(false); + expect(result.ok === false && result.error).toMatch( + /Must be a JSON object/, + ); + }); +}); + +describe("JsonObjectInput", () => { + it("opens with the value serialized two-space, nesting intact", () => { + renderWithMantine(); + expect(box().value).toBe( + '{\n "a": {\n "b": [\n 1,\n 2\n ]\n }\n}', + ); + }); + + it("emits the parsed object — a structured value stays structured", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine(); + await user.clear(box()); + await user.type(box(), '{{"trace":{{"id":"x"},"n":3}'); + expect(onChange).toHaveBeenLastCalledWith({ trace: { id: "x" }, n: 3 }); + }); + + it("shows the text as typed while it is invalid and does not emit", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine(); + // Append a stray character rather than clearing: clearing is itself a + // valid edit (it means `{}`) and would emit, which is not what is under + // test here. + await user.click(box()); + await user.keyboard("{End}x"); + // Displayed verbatim — not re-escaped back through JSON.stringify. + expect(box().value.endsWith("}x")).toBe(true); + expect( + screen.getByText("Not valid JSON — changes are not applied"), + ).toBeInTheDocument(); + // Nothing emitted, so the caller still holds the last valid object. + expect(onChange).not.toHaveBeenCalled(); + expect(screen.getByTestId("emitted").textContent).toBe('{"keep":"me"}'); + }); + + it("flags valid JSON that is not an object without emitting", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine(); + await user.clear(box()); + // The clear is a legitimate edit to `{}`; only what follows is under test. + expect(onChange).toHaveBeenLastCalledWith({}); + onChange.mockClear(); + await user.type(box(), "[[1]"); + expect(screen.getByText(/Must be a JSON object/)).toBeInTheDocument(); + expect(onChange).not.toHaveBeenCalled(); + }); + + it("treats a cleared box as the empty object", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine(); + await user.clear(box()); + expect(onChange).toHaveBeenLastCalledWith({}); + expect(screen.queryByText(/Not valid JSON/)).toBeNull(); + }); + + it("re-syncs when the parent's value diverges from the draft", async () => { + // An external reset (switching servers, a reloaded catalog) must reach the + // box; an in-progress edit must not be overwritten by the value it emitted. + function Externally() { + const [value, setValue] = useState({ a: 1 }); + return ( + <> + + + + ); + } + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByRole("button", { name: "reset" })); + expect(JSON.parse(box().value)).toEqual({ b: 2 }); + }); + + it("leaves an in-progress edit alone when the parent echoes it back", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.clear(box()); + // Typed without the closing brace auto-pair: the text is what the user has + // so far, and the echo of the parsed value must not reformat it mid-edit. + await user.type(box(), '{{"a":1}'); + expect(box().value).toBe('{"a":1}'); + }); +}); diff --git a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx new file mode 100644 index 00000000..93df2516 --- /dev/null +++ b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx @@ -0,0 +1,113 @@ +import { useState } from "react"; +import { JsonInput } from "@mantine/core"; +import type { JsonObject } from "@inspector/core/json/jsonUtils.js"; +import { useValueChange } from "../../../hooks/useValueChange"; + +const ObjectJsonInput = JsonInput.withProps({ + formatOnBlur: true, + autosize: true, + minRows: 4, + spellCheck: false, +}); + +/** Two-space canonical form — the same shape the catalog file is written in. */ +function serialize(value: JsonObject): string { + return JSON.stringify(value, null, 2); +} + +/** A plain JSON object, i.e. not an array and not a scalar. */ +function isJsonObject(value: unknown): value is JsonObject { + return value !== null && typeof value === "object" && !Array.isArray(value); +} + +/** + * What a draft's text currently means: the object to emit, or why it can't be. + * + * Empty text is the object `{}` rather than an error — clearing the box is how + * "send no metadata" is spelled, and an empty editor showing a red error would + * read as broken. + */ +type DraftState = + | { ok: true; value: JsonObject } + | { ok: false; error: string }; + +export function parseObjectDraft(text: string): DraftState { + if (text.trim() === "") return { ok: true, value: {} }; + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + return { ok: false, error: "Not valid JSON — changes are not applied" }; + } + if (!isJsonObject(parsed)) { + return { + ok: false, + error: "Must be a JSON object (`{ … }`) — changes are not applied", + }; + } + return { ok: true, value: parsed }; +} + +export interface JsonObjectInputProps { + value: JsonObject; + onChange: (value: JsonObject) => void; + label?: string; + description?: string; + placeholder?: string; + "aria-label"?: string; + "data-testid"?: string; +} + +/** + * A JSON-object editor: the user types JSON, the parent receives the parsed + * object. + * + * This is the editor for payloads whose *values* may be any JSON, which is why + * it exists beside the key/value row editors used for headers and env. `_meta` + * is the case that forced it (#1910) — nothing in the MCP spec restricts a + * `_meta` value to a string, so a `{ key, value }` row editor cannot express + * what the protocol allows. + * + * The text the user is typing is the source of truth for what is *displayed*; + * the parent only ever sees a parsed object. That split is what makes the box + * typeable: driving it off the parent's value and stringifying on every render + * re-escapes half-typed text once per keystroke, which is the #1928 symptom. + * `SchemaJsonField` in `SchemaForm.tsx` carries the same split for the same + * reason — see the long comment there. + * + * While the text is invalid the parent is **not** told, so the last valid + * object stands. The alternative — emitting `{}` — would silently discard a + * user's configured metadata the moment they typed a stray character, and this + * form has no Save button to gate instead: `onChange` writes straight through. + * The state is not silent, though; the field says so on itself via `error`. + */ +export function JsonObjectInput({ + value, + onChange, + ...inputProps +}: JsonObjectInputProps) { + const [draft, setDraft] = useState(() => serialize(value)); + + // Re-sync only when the parent's value genuinely diverges from what the draft + // parses to, so an external reset (switching servers, a reloaded catalog) + // reaches the box while an in-progress edit is left alone. Compared through + // canonical JSON because the parent hands back a fresh object each time. + const parsed = parseObjectDraft(draft); + const draftJson = parsed.ok ? serialize(parsed.value) : null; + useValueChange(serialize(value), (next) => { + if (next !== draftJson) setDraft(next); + }); + + return ( + { + setDraft(text); + const result = parseObjectDraft(text); + if (result.ok) onChange(result.value); + }} + /> + ); +} diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx index 19c54849..c6e6fb69 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx @@ -10,7 +10,7 @@ import { const defaultSettings: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 30000, requestTimeout: 60000, taskTtl: 60000, @@ -57,34 +57,9 @@ function InteractiveRender(args: ServerSettingsFormProps) { }, }); }} - onAddMetadata={() => { - args.onAddMetadata(); - updateArgs({ - settings: { - ...args.settings, - metadata: [...args.settings.metadata, { key: "", value: "" }], - }, - }); - }} - onRemoveMetadata={(index) => { - args.onRemoveMetadata(index); - updateArgs({ - settings: { - ...args.settings, - metadata: args.settings.metadata.filter((_, i) => i !== index), - }, - }); - }} - onMetadataChange={(index, key, value) => { - args.onMetadataChange(index, key, value); - updateArgs({ - settings: { - ...args.settings, - metadata: args.settings.metadata.map((m, i) => - i === index ? { key, value } : m, - ), - }, - }); + onMetadataChange={(metadata) => { + args.onMetadataChange(metadata); + updateArgs({ settings: { ...args.settings, metadata } }); }} onAddEnv={() => { args.onAddEnv(); @@ -197,8 +172,6 @@ const meta: Meta = { onRemoveEnv: fn(), onEnvChange: fn(), onCwdChange: fn(), - onAddMetadata: fn(), - onRemoveMetadata: fn(), onMetadataChange: fn(), onTimeoutChange: fn(), onAutoRefreshChange: fn(), @@ -278,10 +251,13 @@ export const AllConfigured: Story = { { key: "X-Request-Id", value: "req-456" }, ], env: [], - metadata: [ - { key: "userId", value: "user-789" }, - { key: "sessionId", value: "session-012" }, - ], + metadata: { + userId: "user-789", + sessionId: "session-012", + // A structured value — the case a `{ key, value }` row editor could + // not express (#1910). + client: { name: "inspector", features: ["apps", "tasks"], retries: 3 }, + }, connectionTimeout: 15000, requestTimeout: 45000, taskTtl: 45000, diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index 7ea0e396..cd5e168a 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -22,7 +22,7 @@ function clearButtonFor(input: HTMLElement): HTMLElement { const emptySettings: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 30000, requestTimeout: 60000, taskTtl: 60000, @@ -33,7 +33,7 @@ const emptySettings: InspectorServerSettings = { const populatedSettings: InspectorServerSettings = { headers: [{ key: "Authorization", value: "Bearer abc" }], env: [], - metadata: [{ key: "userId", value: "u-1" }], + metadata: { userId: "u-1", tenant: { id: 7, tags: ["a"] } }, connectionTimeout: 30000, requestTimeout: 60000, taskTtl: 60000, @@ -63,8 +63,6 @@ const baseHandlers = { onRemoveEnv: vi.fn(), onEnvChange: vi.fn(), onCwdChange: vi.fn(), - onAddMetadata: vi.fn(), - onRemoveMetadata: vi.fn(), onMetadataChange: vi.fn(), onTimeoutChange: vi.fn(), onAutoRefreshChange: vi.fn(), @@ -239,7 +237,7 @@ describe("ServerSettingsForm", () => { expect(screen.getByText("Log Level per Request")).toBeInTheDocument(); }); - it("shows empty hints for headers and metadata when no entries exist", () => { + it("shows the empty hint for headers, and an empty JSON object for metadata", () => { renderWithMantine( { expect( screen.getByText("No custom headers configured"), ).toBeInTheDocument(); - expect( - screen.getByText("No request metadata configured"), - ).toBeInTheDocument(); + // Metadata has no rows to be absent — the editor is always present, and + // "none configured" is spelled `{}` in the box itself. + expect(screen.getByLabelText("Request metadata JSON")).toHaveValue("{}"); }); it("invokes onAddHeader when + Add Header is clicked", async () => { @@ -270,21 +268,6 @@ describe("ServerSettingsForm", () => { expect(onAddHeader).toHaveBeenCalledTimes(1); }); - it("invokes onAddMetadata when + Add Metadata is clicked", async () => { - const user = userEvent.setup(); - const onAddMetadata = vi.fn(); - renderWithMantine( - , - ); - await user.click(screen.getByRole("button", { name: "+ Add Metadata" })); - expect(onAddMetadata).toHaveBeenCalledTimes(1); - }); - it("invokes onHeaderChange when typing in header value input", async () => { const user = userEvent.setup(); const onHeaderChange = vi.fn(); @@ -340,29 +323,38 @@ describe("ServerSettingsForm", () => { expect(onRemoveHeader).toHaveBeenCalledWith(0); }); - it("invokes onMetadataChange and onRemoveMetadata for metadata rows", async () => { - const user = userEvent.setup(); - const onMetadataChange = vi.fn(); - const onRemoveMetadata = vi.fn(); + it("renders the configured metadata as formatted JSON, nested values included", () => { renderWithMantine( , ); - const valueInput = screen.getByDisplayValue("u-1"); - await user.type(valueInput, "Z"); - expect(onMetadataChange).toHaveBeenCalled(); + const editor = screen.getByLabelText("Request metadata JSON"); + // The point of #1910: a nested object survives into the editor instead of + // being flattened into a `{ key, value }` string row. + expect(JSON.parse((editor as HTMLTextAreaElement).value)).toEqual({ + userId: "u-1", + tenant: { id: 7, tags: ["a"] }, + }); + }); - await user.click( - screen.getByRole("button", { - name: "Remove metadata entry, userId, row 1", - }), + it("reports the whole edited object through onMetadataChange", async () => { + const user = userEvent.setup(); + const onMetadataChange = vi.fn(); + renderWithMantine( + , ); - expect(onRemoveMetadata).toHaveBeenCalledWith(0); + const editor = screen.getByLabelText("Request metadata JSON"); + await user.clear(editor); + await user.type(editor, '{{"n":[[1,2]}'); + expect(onMetadataChange).toHaveBeenLastCalledWith({ n: [1, 2] }); }); it("invokes onTimeoutChange when typing in connection timeout", async () => { @@ -1470,23 +1462,6 @@ describe("ServerSettingsForm", () => { expect(onClearStoredOAuth).toHaveBeenCalledTimes(1); }); - it("clears a metadata value via its Clear button (onMetadataChange with empty value)", async () => { - const user = userEvent.setup(); - const onMetadataChange = vi.fn(); - renderWithMantine( - , - ); - // populatedSettings has one metadata { key: "userId", value: "u-1" } - const valueInput = screen.getByDisplayValue("u-1"); - await user.click(clearButtonFor(valueInput)); - expect(onMetadataChange).toHaveBeenCalledWith(0, "userId", ""); - }); - it("omits the Clear buttons for an empty header key/value row", () => { // A row whose key and value are both empty renders no Clear button in // either field (the null branch of `item.key ?` / `item.value ?`). diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index 18f0c522..36179c9b 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -4,6 +4,7 @@ import { Alert, Button, Checkbox, + Code, Flex, Group, NumberInput, @@ -13,11 +14,13 @@ import { TextInput, } from "@mantine/core"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; +import { JsonObjectInput } from "../../elements/JsonObjectInput/JsonObjectInput"; import type { ProtocolEra } from "@modelcontextprotocol/client"; import type { InspectorServerSettings, ModernLogLevel, OAuthSettings, + RequestMetadata, ServerProtocolEra, ServerType, } from "@inspector/core/mcp/types.js"; @@ -78,9 +81,12 @@ export interface ServerSettingsFormProps { onRemoveEnv: (index: number) => void; onEnvChange: (index: number, key: string, value: string) => void; onCwdChange: (value: string) => void; - onAddMetadata: () => void; - onRemoveMetadata: (index: number) => void; - onMetadataChange: (index: number, key: string, value: string) => void; + /** + * Replace the whole default-`_meta` payload. Not the `(index, key, value)` + * row callback the header/env editors use — metadata is edited as a JSON + * object, since a `_meta` value may be any JSON (#1910). + */ + onMetadataChange: (metadata: RequestMetadata) => void; onTimeoutChange: ( field: "connectionTimeout" | "requestTimeout" | "taskTtl", value: number, @@ -434,8 +440,6 @@ export function ServerSettingsForm({ onRemoveEnv, onEnvChange, onCwdChange, - onAddMetadata, - onRemoveMetadata, onMetadataChange, onTimeoutChange, onAutoRefreshChange, @@ -724,22 +728,18 @@ export function ServerSettingsForm({ Request Metadata - - - Metadata sent with every MCP request (included in _meta field) - - + Add Metadata - - {settings.metadata.length === 0 ? ( - No request metadata configured - ) : ( - - )} + + Metadata merged into the `_meta` field of every MCP request to + this server, as a JSON object. Values may be any JSON — objects, + arrays, numbers, booleans, `null` — not just strings. Leave it as{" "} + {"{}"} to send none. + + diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.stories.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.stories.tsx index e6cb9e14..453d41cb 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.stories.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.stories.tsx @@ -14,7 +14,7 @@ const initialSettings: InspectorServerSettings = { { key: "X-Request-Id", value: "req-456" }, ], env: [], - metadata: [{ key: "userId", value: "user-789" }], + metadata: { userId: "user-789", client: { name: "inspector" } }, connectionTimeout: 30000, requestTimeout: 60000, taskTtl: 60000, @@ -74,7 +74,7 @@ export const EmptySettings: Story = { settings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 30000, requestTimeout: 60000, taskTtl: 60000, @@ -96,7 +96,7 @@ export const StdioServer: Story = { { key: "API_KEY", value: "abc-123" }, { key: "LOG_LEVEL", value: "debug" }, ], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx index ba0303d4..c42fba16 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx @@ -7,7 +7,7 @@ import { ServerSettingsModal } from "./ServerSettingsModal"; const initialSettings: InspectorServerSettings = { headers: [{ key: "Authorization", value: "Bearer abc" }], env: [], - metadata: [{ key: "userId", value: "u-1" }], + metadata: { userId: "u-1" }, connectionTimeout: 30000, requestTimeout: 60000, taskTtl: 60000, @@ -21,7 +21,7 @@ const initialSettings: InspectorServerSettings = { const emptySettings: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 30000, requestTimeout: 60000, taskTtl: 60000, @@ -310,76 +310,6 @@ describe("ServerSettingsModal", () => { expect(lastCall.headers[0].value).toContain("Bearer abc"); }); - it("calls onSettingsChange when adding metadata after expanding the section", async () => { - const user = userEvent.setup(); - const onSettingsChange = vi.fn(); - renderWithMantine( - , - ); - await user.click(screen.getByRole("button", { name: "Request Metadata" })); - await user.click(screen.getByRole("button", { name: "+ Add Metadata" })); - expect(onSettingsChange).toHaveBeenCalledWith({ - ...emptySettings, - metadata: [{ key: "", value: "" }], - }); - }); - - it("calls onSettingsChange when removing metadata", async () => { - const user = userEvent.setup(); - const onSettingsChange = vi.fn(); - renderWithMantine( - , - ); - await user.click(screen.getByRole("button", { name: "Request Metadata" })); - // Both the header and metadata rows have a remove button; each is named - // for the row it belongs to, so no positional guess is needed. - await user.click( - screen.getByRole("button", { - name: "Remove metadata entry, userId, row 1", - }), - ); - expect(onSettingsChange).toHaveBeenCalledWith({ - ...initialSettings, - metadata: [], - }); - }); - - it("calls onSettingsChange when typing in metadata key/value", async () => { - const user = userEvent.setup(); - const onSettingsChange = vi.fn(); - renderWithMantine( - , - ); - await user.click(screen.getByRole("button", { name: "Request Metadata" })); - const valueInput = screen.getByDisplayValue("u-1"); - await user.type(valueInput, "9"); - expect(onSettingsChange).toHaveBeenCalled(); - const lastCall = - onSettingsChange.mock.calls[onSettingsChange.mock.calls.length - 1][0]; - expect(lastCall.metadata[0].key).toBe("userId"); - }); - it("calls onSettingsChange when changing timeouts", async () => { const user = userEvent.setup(); const onSettingsChange = vi.fn(); @@ -703,20 +633,13 @@ describe("ServerSettingsModal", () => { expect(lastCall.headers[1]).toEqual({ key: "B", value: "2" }); }); - it("preserves sibling rows when editing one of several metadata entries", async () => { + it("replaces the whole metadata object when the JSON editor changes", async () => { const user = userEvent.setup(); const onSettingsChange = vi.fn(); - const twoMeta: InspectorServerSettings = { - ...emptySettings, - metadata: [ - { key: "m1", value: "v1" }, - { key: "m2", value: "v2" }, - ], - }; renderWithMantine( { />, ); await user.click(screen.getByRole("button", { name: "Request Metadata" })); - await user.type(screen.getByDisplayValue("v1"), "x"); + const editor = screen.getByLabelText("Request metadata JSON"); + await user.clear(editor); + await user.type(editor, '{{"trace":{{"id":"abc"}}}'); const lastCall = onSettingsChange.mock.calls.at(-1)?.[0]; - expect(lastCall.metadata[1]).toEqual({ key: "m2", value: "v2" }); + // The nested object arrives as an object, not a stringified one — the + // whole point of #1910. + expect(lastCall.metadata).toEqual({ trace: { id: "abc" } }); + // Every other setting is carried through untouched. + expect(lastCall.headers).toEqual(emptySettings.headers); }); it("preserves sibling rows when editing one of several roots", async () => { diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx index e2af62f5..fbdfa7ee 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx @@ -5,6 +5,7 @@ import type { InspectorServerSettings, ModernLogLevel, OAuthSettings, + RequestMetadata, ServerProtocolEra, ServerType, } from "@inspector/core/mcp/types.js"; @@ -155,24 +156,7 @@ export function ServerSettingsModal({ onSettingsChange({ ...settings, cwd: value }); } - function handleAddMetadata() { - onSettingsChange({ - ...settings, - metadata: [...settings.metadata, { key: "", value: "" }], - }); - } - - function handleRemoveMetadata(index: number) { - onSettingsChange({ - ...settings, - metadata: settings.metadata.filter((_, i) => i !== index), - }); - } - - function handleMetadataChange(index: number, key: string, value: string) { - const metadata = settings.metadata.map((m, i) => - i === index ? { key, value } : m, - ); + function handleMetadataChange(metadata: RequestMetadata) { onSettingsChange({ ...settings, metadata }); } @@ -303,8 +287,6 @@ export function ServerSettingsModal({ onRemoveEnv={handleRemoveEnv} onEnvChange={handleEnvChange} onCwdChange={handleCwdChange} - onAddMetadata={handleAddMetadata} - onRemoveMetadata={handleRemoveMetadata} onMetadataChange={handleMetadataChange} onTimeoutChange={handleTimeoutChange} onAutoRefreshChange={handleAutoRefreshChange} diff --git a/clients/web/src/test/core/client/runner.test.ts b/clients/web/src/test/core/client/runner.test.ts index 06dd0e1b..0c8660a4 100644 --- a/clients/web/src/test/core/client/runner.test.ts +++ b/clients/web/src/test/core/client/runner.test.ts @@ -76,7 +76,7 @@ describe("runner client auth options", () => { taskTtl: 60000, maxFetchRequests: 10, autoRefreshOnListChanged: false, - metadata: [], + metadata: {}, headers: [], env: [], roots: [], @@ -99,7 +99,7 @@ describe("runner client auth options", () => { taskTtl: 60000, maxFetchRequests: 10, autoRefreshOnListChanged: false, - metadata: [], + metadata: {}, headers: [], env: [], roots: [], @@ -116,7 +116,7 @@ describe("runner client auth options", () => { taskTtl: 60000, maxFetchRequests: 10, autoRefreshOnListChanged: false, - metadata: [], + metadata: {}, headers: [], env: [], roots: [], @@ -135,7 +135,7 @@ describe("runner client auth options", () => { taskTtl: 60000, maxFetchRequests: 10, autoRefreshOnListChanged: false, - metadata: [], + metadata: {}, headers: [], env: [], roots: [], @@ -156,7 +156,7 @@ describe("runner client auth options", () => { taskTtl: 60000, maxFetchRequests: 10, autoRefreshOnListChanged: false, - metadata: [], + metadata: {}, headers: [], env: [], roots: [], @@ -187,7 +187,7 @@ describe("runner client auth options", () => { taskTtl: 60000, maxFetchRequests: 10, autoRefreshOnListChanged: false, - metadata: [], + metadata: {}, headers: [], env: [], roots: [], diff --git a/clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts b/clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts index 84f019cc..cfc22003 100644 --- a/clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts +++ b/clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts @@ -854,7 +854,7 @@ describe("InspectorClient peer-handler timing (#1797)", () => { serverSettings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, diff --git a/clients/web/src/test/core/mcp/node/servers.test.ts b/clients/web/src/test/core/mcp/node/servers.test.ts index 05373af5..a54aaa8d 100644 --- a/clients/web/src/test/core/mcp/node/servers.test.ts +++ b/clients/web/src/test/core/mcp/node/servers.test.ts @@ -30,7 +30,7 @@ describe("headersToServerSettings", () => { expect(settings?.headers).toEqual([ { key: "Authorization", value: "Bearer t" }, ]); - expect(settings?.metadata).toEqual([]); + expect(settings?.metadata).toEqual({}); expect(settings?.roots).toEqual([]); }); }); diff --git a/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts b/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts index c0478851..c2ddcc2a 100644 --- a/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts +++ b/clients/web/src/test/core/mcp/remote/remoteClientTransport.test.ts @@ -361,7 +361,7 @@ describe("RemoteClientTransport", () => { const settings = { headers: [{ key: "X-Tenant", value: "acme" }], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, diff --git a/clients/web/src/test/core/mcp/serverList.test.ts b/clients/web/src/test/core/mcp/serverList.test.ts index 0492622d..13559751 100644 --- a/clients/web/src/test/core/mcp/serverList.test.ts +++ b/clients/web/src/test/core/mcp/serverList.test.ts @@ -11,6 +11,7 @@ import { mcpConfigToServerEntries, mergeSecretsIntoStored, normalizeServerType, + normalizeStoredMetadata, oauthAuthorizationParamsFromSettings, oauthEndpointOverridesFromSettings, serverEntriesToMcpConfig, @@ -240,7 +241,7 @@ describe("serverEntriesToMcpConfig", () => { }); it("round-trips a populated set of Inspector-extension fields (post-#1358 flat shape)", () => { - // Disk shape: top-level `headers` (Record), `metadata` (pair-array), + // Disk shape: top-level `headers` (Record), `metadata` (JSON object), // numeric timeouts, nested `oauth`. Round-trip must preserve the on- // disk shape byte-equivalent so a hand-edited file is stable. const original: MCPConfig = { @@ -249,7 +250,7 @@ describe("serverEntriesToMcpConfig", () => { type: "streamable-http", url: "https://x.test/mcp", headers: { Authorization: "Bearer xyz" }, - metadata: [{ key: "tenant", value: "acme" }], + metadata: { tenant: "acme", limits: { rps: 10 } }, connectionTimeout: 30000, requestTimeout: 60000, oauth: { @@ -581,7 +582,7 @@ describe("serverEntriesToMcpConfig", () => { headers: [{ key: "X-Tenant", value: "acme" }], // Non-stdio server → empty env mirror in memory (for the form) env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, // Absent taskTtl on disk → product default in memory (for the form) @@ -663,7 +664,7 @@ describe("serverEntriesToMcpConfig", () => { { key: " ", value: "whitespace" }, ], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -690,7 +691,7 @@ describe("serverEntriesToMcpConfig", () => { settings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -719,7 +720,7 @@ describe("serverEntriesToMcpConfig", () => { settings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -758,7 +759,7 @@ describe("serverEntriesToMcpConfig", () => { settings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -785,7 +786,7 @@ describe("serverEntriesToMcpConfig", () => { settings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -1163,7 +1164,7 @@ describe("enterpriseManaged oauth settings", () => { const stored = inspectorSettingsToStoredFields({ headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, @@ -1180,7 +1181,7 @@ describe("enterpriseManaged oauth settings", () => { const stored = inspectorSettingsToStoredFields({ headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, @@ -1196,7 +1197,7 @@ describe("oauthOnInsufficientScope (SEP-2350)", () => { const baseSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, @@ -1374,6 +1375,109 @@ describe("oauthOnInsufficientScope (SEP-2350)", () => { }); }); +describe("normalizeStoredMetadata (#1910)", () => { + let warn: ReturnType; + beforeEach(() => { + warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + afterEach(() => { + warn.mockRestore(); + }); + + it("passes an object through, values of any JSON type intact", () => { + const meta = { + s: "a", + n: 1, + b: false, + z: null, + arr: [1, { k: 2 }], + o: { deep: true }, + }; + expect(normalizeStoredMetadata(meta)).toEqual(meta); + }); + + it("copies rather than aliasing the stored object", () => { + // The settings object is edited in place by the form; sharing the + // reference with the parsed catalog would mutate it behind the caller. + const meta = { a: 1 }; + const out = normalizeStoredMetadata(meta); + out.b = 2; + expect(meta).toEqual({ a: 1 }); + }); + + it("reads the pre-#1910 pair array so an existing mcp.json keeps working", () => { + expect( + normalizeStoredMetadata([ + { key: "tenant", value: "acme" }, + { key: "n", value: 3 }, + ]), + ).toEqual({ tenant: "acme", n: 3 }); + }); + + it("skips a blank-key legacy pair", () => { + expect( + normalizeStoredMetadata([ + { key: "", value: "orphan" }, + { key: " ", value: "orphan" }, + { key: "kept", value: "yes" }, + ]), + ).toEqual({ kept: "yes" }); + }); + + it("reads a legacy pair with no value as an empty string", () => { + expect(normalizeStoredMetadata([{ key: "k" }])).toEqual({ k: "" }); + }); + + it("drops a malformed legacy entry with a warning, keeping its siblings", () => { + expect( + normalizeStoredMetadata([ + "nope", + null, + { value: "no key" }, + { key: "kept", value: 1 }, + ]), + ).toEqual({ kept: 1 }); + expect(warn).toHaveBeenCalledTimes(3); + }); + + it.each([ + ["undefined", undefined], + ["null", null], + ])("reads %s as the empty object without warning", (_label, input) => { + expect(normalizeStoredMetadata(input)).toEqual({}); + expect(warn).not.toHaveBeenCalled(); + }); + + it.each([ + ["a string", "oops"], + ["a number", 7], + ["a boolean", true], + ])("drops %s with a warning", (_label, input) => { + expect(normalizeStoredMetadata(input)).toEqual({}); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("migrates a legacy file to the object shape on the next write", () => { + // Read side accepts the pair array; write side emits only the object, so + // one round-trip is the migration. + const entries = mcpConfigToServerEntries({ + mcpServers: { + legacy: { + type: "streamable-http", + url: "https://x.test/mcp", + metadata: [{ key: "tenant", value: "acme" }] as unknown as Record< + string, + never + >, + }, + }, + }); + expect(entries[0]?.settings?.metadata).toEqual({ tenant: "acme" }); + const round = serverEntriesToMcpConfig(entries); + expect(round.mcpServers.legacy?.metadata).toEqual({ tenant: "acme" }); + }); +}); + describe("envRecordToPairs / envPairsToRecord", () => { it("envRecordToPairs preserves key insertion order", () => { expect(envRecordToPairs({ B: "2", A: "1" })).toEqual([ @@ -1469,7 +1573,7 @@ describe("stdio env / cwd mirroring", () => { headers: [], env: [{ key: "API_KEY", value: "secret" }], cwd: "/srv/app", - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, diff --git a/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts b/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts index 939af2ef..1d0717f9 100644 --- a/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts @@ -13,7 +13,7 @@ function prompt(name: string): Prompt { const AUTO_REFRESH_SETTINGS: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, diff --git a/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts b/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts index be88cd5f..643b9f8a 100644 --- a/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts @@ -13,7 +13,7 @@ function template(name: string): ResourceTemplate { const AUTO_REFRESH_SETTINGS: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, diff --git a/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts b/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts index 7944f3a4..15fb404a 100644 --- a/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts @@ -13,7 +13,7 @@ function resource(uri: string): Resource { const AUTO_REFRESH_SETTINGS: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, diff --git a/clients/web/src/test/core/mcp/state/managedToolsState.test.ts b/clients/web/src/test/core/mcp/state/managedToolsState.test.ts index 8455dd74..f62118b6 100644 --- a/clients/web/src/test/core/mcp/state/managedToolsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedToolsState.test.ts @@ -13,7 +13,7 @@ function tool(name: string): Tool { const AUTO_REFRESH_SETTINGS: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, diff --git a/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts b/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts index b82828b8..715facf2 100644 --- a/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts @@ -11,7 +11,7 @@ function prompt(name: string): Prompt { const PAGINATED_SETTINGS: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, diff --git a/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts b/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts index c8fee7e7..87c405f4 100644 --- a/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts @@ -11,7 +11,7 @@ function resource(uri: string): Resource { const PAGINATED_SETTINGS: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, diff --git a/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts b/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts index b8ee12f5..463a304f 100644 --- a/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts @@ -11,7 +11,7 @@ function tool(name: string): Tool { const PAGINATED_SETTINGS: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 60000, diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index c0d5260c..15cb01fb 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -412,7 +412,7 @@ describe("useServers", () => { await result.current.updateServerSettings("alpha", { headers: [{ key: "X-Tenant", value: "acme" }], env: [], - metadata: [{ key: "trace", value: "abc" }], + metadata: { trace: "abc" }, connectionTimeout: 5000, requestTimeout: 30000, taskTtl: 30000, @@ -425,7 +425,7 @@ describe("useServers", () => { expect(result.current.servers[0]?.settings).toEqual({ headers: [{ key: "X-Tenant", value: "acme" }], env: [], - metadata: [{ key: "trace", value: "abc" }], + metadata: { trace: "abc" }, connectionTimeout: 5000, requestTimeout: 30000, taskTtl: 30000, @@ -444,7 +444,7 @@ describe("useServers", () => { expect(stored.url).toBe("https://x.test/mcp"); expect(stored).not.toHaveProperty("settings"); expect(stored.headers).toEqual({ "X-Tenant": "acme" }); - expect(stored.metadata).toEqual([{ key: "trace", value: "abc" }]); + expect(stored.metadata).toEqual({ trace: "abc" }); expect(stored.connectionTimeout).toBe(5000); expect(stored.requestTimeout).toBe(30000); expect(stored.taskTtl).toBe(30000); @@ -461,7 +461,7 @@ describe("useServers", () => { await result.current.updateServerSettings("nonexistent", { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -511,7 +511,7 @@ describe("useServers", () => { expect(result.current.servers[0]?.settings).toEqual({ headers: [{ key: "X-Keep", value: "yes" }], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, // Absent taskTtl on disk reads back as the product default for the form. diff --git a/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts index d873453c..d0067baf 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts @@ -389,7 +389,7 @@ describe("InspectorClient coverage backfill", () => { serverSettings: { headers: [], env: [], - metadata: [{ key: "x-test", value: "1" }], + metadata: { "x-test": "1" }, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -655,7 +655,7 @@ describe("InspectorClient coverage backfill", () => { serverSettings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 1, requestTimeout: 0, taskTtl: 0, diff --git a/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts index cb346ec0..06b932d5 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts @@ -506,7 +506,7 @@ describe("modern-era negotiation (2026-07-28)", () => { versionNegotiation: eraToVersionNegotiation("modern"), serverSettings: { headers: [], - metadata: [], + metadata: {}, env: [], connectionTimeout: 0, requestTimeout: 0, @@ -535,7 +535,7 @@ describe("modern-era negotiation (2026-07-28)", () => { versionNegotiation: eraToVersionNegotiation("modern"), serverSettings: { headers: [], - metadata: [], + metadata: {}, env: [], connectionTimeout: 0, requestTimeout: 0, @@ -586,7 +586,7 @@ describe("modern-era negotiation (2026-07-28)", () => { versionNegotiation: eraToVersionNegotiation("modern"), serverSettings: { headers: [], - metadata: [], + metadata: {}, env: [], connectionTimeout: 0, requestTimeout: 0, diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 8c05b7ed..9fbb0608 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -53,6 +53,7 @@ import type { MessageEntry, ConnectionStatus, FetchRequestEntryBase, + RequestMetadata, } from "@inspector/core/mcp/types.js"; import type { JsonValue } from "@inspector/core/json/jsonUtils.js"; import type { @@ -164,7 +165,7 @@ function settleInFlight(call: Promise): InFlightCall { /** Get all resources from the client via listResources() (paginates if needed). */ async function getAllResources( client: InspectorClient, - metadata?: Record, + metadata?: RequestMetadata, ): Promise { const collected: Resource[] = []; let cursor: string | undefined; @@ -180,7 +181,7 @@ async function getAllResources( /** Get all resource templates via listResourceTemplates() (paginates if needed). */ async function getAllResourceTemplates( client: InspectorClient, - metadata?: Record, + metadata?: RequestMetadata, ): Promise { const collected: ResourceTemplate[] = []; let cursor: string | undefined; @@ -196,7 +197,7 @@ async function getAllResourceTemplates( /** Get all prompts via listPrompts() (paginates if needed). */ async function getAllPrompts( client: InspectorClient, - metadata?: Record, + metadata?: RequestMetadata, ): Promise { const collected: Prompt[] = []; let cursor: string | undefined; @@ -386,7 +387,7 @@ describe("InspectorClient", () => { serverSettings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 50, requestTimeout: 0, taskTtl: 0, @@ -756,7 +757,7 @@ describe("InspectorClient", () => { const initial = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -1372,6 +1373,45 @@ describe("InspectorClient", () => { messageLogState.destroy(); }); + it("sends a structured default _meta value as JSON, not as a string (#1910)", async () => { + // The whole point of #1910: a `_meta` value may be an object, an array, + // a number, a boolean or `null`. The Inspector used to type this map as + // `Record`, so a nested payload could only be sent by + // stringifying it — which is a different wire message than the one the + // user configured. + const structured = { + trace: { id: "abc123", sampled: true, hops: [1, 2] }, + retries: 3, + beta: false, + unset: null, + }; + client = new InspectorClient( + { + type: "stdio", + command: serverCommand.command, + args: serverCommand.args, + }, + { + environment: { transport: createTransportNode }, + defaultMetadata: structured, + }, + ); + const messageLogState = new MessageLogState(client); + await client.connect(); + await client.listTools(); + + const listToolsReq = messageLogState + .getMessages() + .find( + (m) => + m.direction === "request" && + (m.message as { method?: string }).method === "tools/list", + ); + expect(listToolsReq).toBeDefined(); + expect(metaOf(listToolsReq!)).toMatchObject(structured); + messageLogState.destroy(); + }); + it("call-time metadata overrides defaultMetadata on key collision", async () => { client = new InspectorClient( { @@ -4241,7 +4281,7 @@ describe("InspectorClient", () => { timestamp: Date; success: boolean; error?: string; - metadata?: Record; + metadata?: RequestMetadata; }> = []; client!.addEventListener( diff --git a/clients/web/src/test/integration/mcp/node/transport.test.ts b/clients/web/src/test/integration/mcp/node/transport.test.ts index af68081f..97cedc9e 100644 --- a/clients/web/src/test/integration/mcp/node/transport.test.ts +++ b/clients/web/src/test/integration/mcp/node/transport.test.ts @@ -218,7 +218,7 @@ describe("Transport", () => { { key: "", value: "ignored-empty-key" }, ], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -267,7 +267,7 @@ describe("Transport", () => { const baseSettings: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -305,7 +305,7 @@ describe("Transport", () => { const settings: InspectorServerSettings = { headers: [{ key: "X-Tenant", value: "acme" }], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, @@ -350,7 +350,7 @@ describe("Transport", () => { const settings: InspectorServerSettings = { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index e6140467..cd95f46d 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -457,7 +457,7 @@ describe("server.ts supplemental coverage", () => { const base = { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, }; @@ -891,7 +891,7 @@ describe("server.ts supplemental coverage", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthClientSecret: "shh", @@ -1039,7 +1039,7 @@ describe("server.ts supplemental coverage", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthClientSecret: "new-secret", diff --git a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts index 9862f55d..d0f9d407 100644 --- a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts +++ b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts @@ -425,10 +425,11 @@ describe("/api/servers routes", () => { // Wire envelope unchanged from #1353: pair-array headers, flat // oauth* fields. Backend splats these into the flat disk shape: // object headers, nested oauth, plus the inspector-only fields - // at the top level. + // at the top level. Metadata is the exception — it crosses as a + // JSON object, since a `_meta` value may be any JSON (#1910). settings: { headers: [{ key: "Authorization", value: "Bearer xyz" }], - metadata: [{ key: "tenant", value: "acme" }], + metadata: { tenant: "acme", limits: { rps: 10 } }, connectionTimeout: 30000, requestTimeout: 60000, oauthClientId: "client-abc", @@ -442,7 +443,7 @@ describe("/api/servers routes", () => { // Disk shape: flat, no `settings` wrapper, object headers, nested oauth. expect(stored).not.toHaveProperty("settings"); expect(stored.headers).toEqual({ Authorization: "Bearer xyz" }); - expect(stored.metadata).toEqual([{ key: "tenant", value: "acme" }]); + expect(stored.metadata).toEqual({ tenant: "acme", limits: { rps: 10 } }); expect(stored.connectionTimeout).toBe(30000); expect(stored.requestTimeout).toBe(60000); expect(stored.oauth).toEqual({ @@ -462,7 +463,7 @@ describe("/api/servers routes", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthAuthorizationParams: [ @@ -488,7 +489,7 @@ describe("/api/servers routes", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthAuthorizationParams: { kc_idp_hint: "corp" }, @@ -509,7 +510,7 @@ describe("/api/servers routes", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthAuthorizationUrl: "https://staging.test/authorize", @@ -534,7 +535,7 @@ describe("/api/servers routes", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthAuthorizationUrl: "", @@ -557,7 +558,7 @@ describe("/api/servers routes", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthTokenUrl: 42, @@ -583,7 +584,7 @@ describe("/api/servers routes", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [{ key: "X-Tenant", value: "acme" }], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 45000, }, @@ -617,7 +618,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, autoRefreshOnListChanged: true, @@ -649,7 +650,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, autoRefreshOnListChanged: false, @@ -671,7 +672,7 @@ describe("/api/servers routes", () => { config: { type: "stdio", command: "node" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, autoRefreshOnListChanged: "yes", @@ -696,7 +697,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, maxFetchRequests: 5000, @@ -724,7 +725,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, maxFetchRequests: 0, @@ -756,7 +757,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, maxFetchRequests: 1000, @@ -778,7 +779,7 @@ describe("/api/servers routes", () => { config: { type: "stdio", command: "node" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, maxFetchRequests: -5, @@ -884,7 +885,7 @@ describe("/api/servers routes", () => { // headers should be an array of {key, value}; "oops" is a string. settings: { headers: "oops", - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, }, @@ -915,7 +916,7 @@ describe("/api/servers routes", () => { // write lock and apply only the settings patch. settings: { headers: [{ key: "X-Tenant", value: "acme" }], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, }, @@ -957,7 +958,7 @@ describe("/api/servers routes", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthClientId: "", @@ -983,7 +984,7 @@ describe("/api/servers routes", () => { config: { type: "stdio", command: "node" }, settings: { headers: [{ key: "X-A", value: "1" }], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, // Unknown stowaway — must not survive the validator. @@ -1101,7 +1102,7 @@ describe("/api/servers routes", () => { url: "https://x.test/mcp", settings: { headers: [{ key: "X-Tenant", value: "acme" }], - metadata: [], + metadata: {}, connectionTimeout: 30000, requestTimeout: 0, oauthClientId: "client-abc", @@ -1207,7 +1208,7 @@ describe("/api/servers routes", () => { config: { type: "stdio", command: "node" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, roots: [ @@ -1255,7 +1256,7 @@ describe("/api/servers routes", () => { config: { type: "stdio", command: "node" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, roots: [{ uri: " " }], @@ -1285,7 +1286,7 @@ describe("/api/servers routes", () => { config: { type: "stdio", command: "node" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, // uri must be a string. @@ -1336,7 +1337,7 @@ describe("/api/servers routes", () => { { key: "Authorization", value: "Bearer the-token" }, { key: "X-Tenant", value: "acme" }, ], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, }, @@ -1368,7 +1369,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, env: [{ key: "API_KEY", value: "abc-123" }], @@ -1416,7 +1417,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 5000, requestTimeout: 0, }, @@ -1484,7 +1485,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, env: [], @@ -1516,7 +1517,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, env: [ @@ -1587,7 +1588,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, env: "nope", @@ -1614,7 +1615,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, env: [], @@ -1642,7 +1643,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, env: [], @@ -1675,7 +1676,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, env: [{ key: "API_KEY", value: "abc" }], @@ -1775,7 +1776,7 @@ describe("/api/servers routes", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthClientId: "cid", @@ -1935,7 +1936,7 @@ describe("/api/servers routes", () => { body: JSON.stringify({ settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthClientId: "cid", @@ -2327,7 +2328,7 @@ describe("/api/servers routes", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthClientId: "cid", @@ -2361,7 +2362,7 @@ describe("/api/servers routes", () => { config: { type: "streamable-http", url: "https://x.test/mcp" }, settings: { headers: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, oauthClientId: "cid", @@ -2658,7 +2659,7 @@ describe("a config-only PUT must not delete stored secrets (#2083)", () => { settings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 30000, requestTimeout: 60000, taskTtl: 60000, @@ -2726,7 +2727,7 @@ describe("a config-only PUT must not delete stored secrets (#2083)", () => { settings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 30000, requestTimeout: 60000, taskTtl: 60000, @@ -2879,7 +2880,7 @@ describe("plaintext migration against a session-scoped store (#1950)", () => { settings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 45000, requestTimeout: 60000, taskTtl: 60000, @@ -2927,7 +2928,7 @@ describe("plaintext migration against a session-scoped store (#1950)", () => { settings: { headers: [], env: [], - metadata: [], + metadata: {}, connectionTimeout: 30000, requestTimeout: 60000, taskTtl: 60000, diff --git a/clients/web/src/test/integration/mcp/remote/transport.test.ts b/clients/web/src/test/integration/mcp/remote/transport.test.ts index 2e2b55ec..98feef7e 100644 --- a/clients/web/src/test/integration/mcp/remote/transport.test.ts +++ b/clients/web/src/test/integration/mcp/remote/transport.test.ts @@ -412,7 +412,7 @@ describe("Remote transport e2e", () => { { key: "X-Trace", value: "abc123" }, ], env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: 0, diff --git a/core/mcp/__tests__/fakeInspectorClient.ts b/core/mcp/__tests__/fakeInspectorClient.ts index 728ef5f7..8e3519e9 100644 --- a/core/mcp/__tests__/fakeInspectorClient.ts +++ b/core/mcp/__tests__/fakeInspectorClient.ts @@ -36,6 +36,7 @@ import type { ResourceSubscriptionStreamState, ToolCallInvocation, ExcludedTool, + RequestMetadata, } from "../types.js"; import { INACTIVE_SUBSCRIPTION_STREAM_STATE } from "../types.js"; import type { MalformedListItem } from "../listSalvage.js"; @@ -145,25 +146,25 @@ export class FakeInspectorClient listAllTools = vi.fn( async (_options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }) => ({ tools: drainPages(this.toolPages, "tools") }), ); listAllPrompts = vi.fn( async (_options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }) => ({ prompts: drainPages(this.promptPages, "prompts") }), ); listAllResources = vi.fn( async (_options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }) => ({ resources: drainPages(this.resourcePages, "resources") }), ); listAllResourceTemplates = vi.fn( async (_options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }) => ({ resourceTemplates: drainPages( this.resourceTemplatePages, @@ -224,7 +225,7 @@ export class FakeInspectorClient _argumentName: string, _argumentValue: string, _context?: Record, - _metadata?: Record, + _metadata?: RequestMetadata, ): Promise<{ values: string[]; total?: number; hasMore?: boolean }> => ({ values: [], }), diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index fb0ad80e..c4ad06db 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -17,6 +17,7 @@ import type { PendingRequestOrigin, ResourceSubscriptionStreamState, ExcludedTool, + RequestMetadata, } from "./types.js"; import { scanXMcpHeaderDeclarations, @@ -356,8 +357,8 @@ function notificationMethodFromSchema(schema: unknown): string | undefined { interface ToolCallRequest { tool: Tool; args: Record; - generalMetadata?: Record; - toolSpecificMetadata?: Record; + generalMetadata?: RequestMetadata; + toolSpecificMetadata?: RequestMetadata; taskOptions?: { ttl?: number }; options?: { skipOutputValidation?: boolean }; } @@ -420,7 +421,7 @@ export class InspectorClient extends InspectorClientEventTarget { private progress: boolean; private resetTimeoutOnProgress: boolean; private requestTimeout: number | undefined; - private defaultMetadata: Record | undefined; + private defaultMetadata: RequestMetadata | undefined; private serverSettings: InspectorServerSettings | undefined; private versionNegotiation: VersionNegotiationOptions; private status: ConnectionStatus = "disconnected"; @@ -1035,7 +1036,7 @@ export class InspectorClient extends InspectorClientEventTarget { * injecting an empty `_meta` field. */ private mergeMeta( - callMetadata?: Record, + callMetadata?: RequestMetadata, ): Record | undefined { const defaults = this.defaultMetadata; // Modern-era per-request log level (#1629): stamp the opt-in `_meta` key on @@ -1055,6 +1056,26 @@ export class InspectorClient extends InspectorClientEventTarget { return Object.keys(merged).length > 0 ? merged : undefined; } + /** + * The `progressToken` a caller stamped on its request metadata, when it is + * one the SDK will accept. + * + * `_meta` values are arbitrary JSON (#1910), but `progressToken` is a + * reserved key the spec types as `string | number`. Anything else — an + * object, an array, `null`, a boolean — is not a token the SDK can correlate + * a `notifications/progress` back to, so it is read as "no token" rather than + * forwarded. The key still reaches the wire inside `_meta`; this only governs + * whether the client wires up a progress callback for it. + */ + private progressTokenOf( + metadata: RequestMetadata | undefined, + ): ProgressToken | undefined { + const token = metadata?.progressToken; + return typeof token === "string" || typeof token === "number" + ? token + : undefined; + } + private getRequestOptions( progressToken?: ProgressToken, signal?: AbortSignal, @@ -1110,7 +1131,7 @@ export class InspectorClient extends InspectorClientEventTarget { * so the merge/omit branch is defined once. */ private aggregateListParams( - metadata?: Record, + metadata?: RequestMetadata, ): { _meta: Record } | undefined { const effectiveMeta = this.mergeMeta(metadata); return effectiveMeta ? { _meta: effectiveMeta } : undefined; @@ -3405,7 +3426,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ async listTools( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise<{ tools: Tool[]; nextCursor?: string }> { if (!this.client) { throw new Error("Client is not connected"); @@ -3423,7 +3444,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.client!.request( { method: "tools/list", params }, ListToolsResultSchema, - this.getRequestOptions(metadata?.progressToken), + this.getRequestOptions(this.progressTokenOf(metadata)), ), ); const tools = [...(response.tools || [])]; @@ -3444,7 +3465,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ async listAllTools(options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }): Promise<{ tools: Tool[] }> { if (!this.client) { throw new Error("Client is not connected"); @@ -3596,7 +3617,7 @@ export class InspectorClient extends InspectorClientEventTarget { * from it so every field except the entries is still validated. */ resultSchema: z.ZodObject; aggregate: () => Promise; - metadata?: Record; + metadata?: RequestMetadata; /** * 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 @@ -3674,7 +3695,7 @@ export class InspectorClient extends InspectorClientEventTarget { itemsKey: string; itemSchema: z.ZodType; resultSchema: z.ZodObject; - metadata?: Record; + metadata?: RequestMetadata; err: unknown; }): Promise { const pageSchema = lenientListPageSchema(resultSchema, itemsKey); @@ -3725,7 +3746,7 @@ export class InspectorClient extends InspectorClientEventTarget { : this.client!.request( { method, params }, pageSchema, - this.getRequestOptions(metadata?.progressToken), + this.getRequestOptions(this.progressTokenOf(metadata)), ), { method }, ); @@ -3811,7 +3832,7 @@ export class InspectorClient extends InspectorClientEventTarget { * stops the walk (non-converging-server guard, mirroring the SDK). */ async refreshExcludedTools( - metadata?: Record, + metadata?: RequestMetadata, ): Promise { const excluded: ExcludedTool[] = []; // Gated to connections that actually exclude; otherwise this is a pure @@ -3881,7 +3902,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ private async listToolsForScan( cursor: string | undefined, - metadata: Record | undefined, + metadata: RequestMetadata | undefined, ): Promise<{ tools: Tool[]; nextCursor?: string; @@ -3924,7 +3945,7 @@ export class InspectorClient extends InspectorClientEventTarget { : this.client!.request( { method: "tools/list", params }, pageSchema, - this.getRequestOptions(metadata?.progressToken), + this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: "tools/list" }, ); @@ -4016,8 +4037,8 @@ export class InspectorClient extends InspectorClientEventTarget { async callTool( tool: Tool, args: Record, - generalMetadata?: Record, - toolSpecificMetadata?: Record, + generalMetadata?: RequestMetadata, + toolSpecificMetadata?: RequestMetadata, taskOptions?: { ttl?: number }, options?: { skipOutputValidation?: boolean }, ): Promise { @@ -4245,7 +4266,7 @@ export class InspectorClient extends InspectorClientEventTarget { const convertedArgs = this.convertStringToolArgs(tool, args); // Merge general metadata with tool-specific metadata; tool-specific wins. - const callMetadata: Record | undefined = + const callMetadata: RequestMetadata | undefined = generalMetadata || toolSpecificMetadata ? { ...(generalMetadata || {}), ...(toolSpecificMetadata || {}) } : undefined; @@ -4270,7 +4291,7 @@ export class InspectorClient extends InspectorClientEventTarget { } const requestOptions = this.getRequestOptions( - metadata?.progressToken, + this.progressTokenOf(metadata), signal, ); this.applyMirroredParamHeaders(tool, convertedArgs, requestOptions); @@ -4360,11 +4381,11 @@ export class InspectorClient extends InspectorClientEventTarget { private dispatchFailedToolCall( tool: Tool, args: Record, - generalMetadata: Record | undefined, - toolSpecificMetadata: Record | undefined, + generalMetadata: RequestMetadata | undefined, + toolSpecificMetadata: RequestMetadata | undefined, errorMessage: string, ): void { - const callMetadata: Record | undefined = + const callMetadata: RequestMetadata | undefined = generalMetadata || toolSpecificMetadata ? { ...(generalMetadata || {}), ...(toolSpecificMetadata || {}) } : undefined; @@ -4848,8 +4869,8 @@ export class InspectorClient extends InspectorClientEventTarget { async callToolStream( tool: Tool, args: Record, - generalMetadata?: Record, - toolSpecificMetadata?: Record, + generalMetadata?: RequestMetadata, + toolSpecificMetadata?: RequestMetadata, taskOptions?: { ttl?: number }, ): Promise { if (!this.client) { @@ -4859,7 +4880,7 @@ export class InspectorClient extends InspectorClientEventTarget { const convertedArgs = this.convertStringToolArgs(tool, args); // Merge general metadata with tool-specific metadata; tool-specific wins. - const callMetadata: Record | undefined = + const callMetadata: RequestMetadata | undefined = generalMetadata || toolSpecificMetadata ? { ...(generalMetadata || {}), ...(toolSpecificMetadata || {}) } : undefined; @@ -4895,7 +4916,9 @@ export class InspectorClient extends InspectorClientEventTarget { // attach one here either — doing so would request a progress token (and // emit requestorTaskProgress) for task calls only, bypassing the toggle // that governs every other call path. - const requestOptions = this.getRequestOptions(metadata?.progressToken); + const requestOptions = this.getRequestOptions( + this.progressTokenOf(metadata), + ); // The task-augmented `tools/call` needs the same SEP-2243 mirroring as the // plain one — a strict modern server rejects it with -32020 otherwise. this.applyMirroredParamHeaders(tool, convertedArgs, requestOptions); @@ -5056,7 +5079,7 @@ export class InspectorClient extends InspectorClientEventTarget { return invocation; } catch (error) { // Merge general metadata with tool-specific metadata for error case - const callMetadata: Record | undefined = + const callMetadata: RequestMetadata | undefined = generalMetadata || toolSpecificMetadata ? { ...(generalMetadata || {}), ...(toolSpecificMetadata || {}) } : undefined; @@ -5086,7 +5109,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ async listResources( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise<{ resources: Resource[]; nextCursor?: string }> { if (!this.client) { throw new Error("Client is not connected"); @@ -5100,7 +5123,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.client!.request( { method: "resources/list", params }, ListResourcesResultSchema, - this.getRequestOptions(metadata?.progressToken), + this.getRequestOptions(this.progressTokenOf(metadata)), ), ); return { @@ -5117,7 +5140,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ async listAllResources(options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }): Promise<{ resources: Resource[] }> { if (!this.client) { throw new Error("Client is not connected"); @@ -5148,7 +5171,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ async readResource( uri: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise { if (!this.client) { throw new Error("Client is not connected"); @@ -5167,7 +5190,7 @@ export class InspectorClient extends InspectorClientEventTarget { "resources/read", params, ReadResourceResultSchema, - this.getRequestOptions(metadata?.progressToken), + this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: "resources/read" }, ); @@ -5198,7 +5221,7 @@ export class InspectorClient extends InspectorClientEventTarget { async readResourceFromTemplate( uriTemplate: string, params: Record, - metadata?: Record, + metadata?: RequestMetadata, ): Promise { if (!this.client) { throw new Error("Client is not connected"); @@ -5260,7 +5283,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ async listResourceTemplates( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise<{ resourceTemplates: ResourceTemplate[]; nextCursor?: string }> { if (!this.client) { throw new Error("Client is not connected"); @@ -5275,7 +5298,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.client!.request( { method: "resources/templates/list", params }, ListResourceTemplatesResultSchema, - this.getRequestOptions(metadata?.progressToken), + this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: "resources/templates/list" }, ); @@ -5293,7 +5316,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ async listAllResourceTemplates(options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }): Promise<{ resourceTemplates: ResourceTemplate[] }> { if (!this.client) { throw new Error("Client is not connected"); @@ -5324,7 +5347,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ async listPrompts( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise<{ prompts: Prompt[]; nextCursor?: string }> { if (!this.client) { throw new Error("Client is not connected"); @@ -5338,7 +5361,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.client!.request( { method: "prompts/list", params }, ListPromptsResultSchema, - this.getRequestOptions(metadata?.progressToken), + this.getRequestOptions(this.progressTokenOf(metadata)), ), ); return { @@ -5355,7 +5378,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ async listAllPrompts(options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }): Promise<{ prompts: Prompt[] }> { if (!this.client) { throw new Error("Client is not connected"); @@ -5388,7 +5411,7 @@ export class InspectorClient extends InspectorClientEventTarget { async getPrompt( name: string, args?: Record, - metadata?: Record, + metadata?: RequestMetadata, ): Promise { if (!this.client) { throw new Error("Client is not connected"); @@ -5412,7 +5435,7 @@ export class InspectorClient extends InspectorClientEventTarget { "prompts/get", params, GetPromptResultSchema, - this.getRequestOptions(metadata?.progressToken), + this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: "prompts/get", toolName: name }, ); @@ -5451,7 +5474,7 @@ export class InspectorClient extends InspectorClientEventTarget { argumentName: string, argumentValue: string, context?: Record, - metadata?: Record, + metadata?: RequestMetadata, ): Promise<{ values: string[]; total?: number; hasMore?: boolean }> { if (!this.client) { return { values: [] }; @@ -5473,7 +5496,7 @@ export class InspectorClient extends InspectorClientEventTarget { () => this.client!.complete( params, - this.getRequestOptions(metadata?.progressToken), + this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: "completion/complete", diff --git a/core/mcp/inspectorClientEventTarget.ts b/core/mcp/inspectorClientEventTarget.ts index 572909c8..58aa41f6 100644 --- a/core/mcp/inspectorClientEventTarget.ts +++ b/core/mcp/inspectorClientEventTarget.ts @@ -25,6 +25,7 @@ import type { ResourceTemplateReadInvocation, ResourceSubscriptionStreamState, ExcludedTool, + RequestMetadata, } from "./types.js"; import type { MalformedListItem } from "./listSalvage.js"; import type { @@ -112,7 +113,7 @@ export interface InspectorClientEventMap { timestamp: Date; success: boolean; error?: string; - metadata?: Record; + metadata?: RequestMetadata; /** Non-fatal outputSchema mismatch detected on the skipOutputValidation path. */ outputValidationError?: string; }; diff --git a/core/mcp/inspectorClientProtocol.ts b/core/mcp/inspectorClientProtocol.ts index 756b560f..ef290f9e 100644 --- a/core/mcp/inspectorClientProtocol.ts +++ b/core/mcp/inspectorClientProtocol.ts @@ -21,6 +21,7 @@ import type { ToolCallInvocation, ResourceSubscriptionStreamState, ExcludedTool, + RequestMetadata, } from "./types.js"; import type { CacheMode, @@ -82,19 +83,19 @@ export interface InspectorClientProtocol extends InspectorClientEventTarget { // Paginated list methods used by managed/paged state stores listTools( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise<{ tools: Tool[]; nextCursor?: string }>; listPrompts( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise<{ prompts: Prompt[]; nextCursor?: string }>; listResources( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise<{ resources: Resource[]; nextCursor?: string }>; listResourceTemplates( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise<{ resourceTemplates: ResourceTemplate[]; nextCursor?: string }>; listRequestorTasks( cursor?: string, @@ -126,41 +127,41 @@ export interface InspectorClientProtocol extends InspectorClientEventTarget { // `cacheMode` uses the SDK default ('use'). listAllTools(options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }): Promise<{ tools: Tool[] }>; listAllPrompts(options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }): Promise<{ prompts: Prompt[] }>; listAllResources(options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }): Promise<{ resources: Resource[] }>; listAllResourceTemplates(options?: { cacheMode?: CacheMode; - metadata?: Record; + metadata?: RequestMetadata; }): Promise<{ resourceTemplates: ResourceTemplate[] }>; // Invocation methods used by paged result panels callTool( tool: Tool, args: Record, - generalMetadata?: Record, - toolSpecificMetadata?: Record, + generalMetadata?: RequestMetadata, + toolSpecificMetadata?: RequestMetadata, ): Promise; readResource( uri: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise; readResourceFromTemplate( uriTemplate: string, params: Record, - metadata?: Record, + metadata?: RequestMetadata, ): Promise; getPrompt( name: string, params?: Record, - metadata?: Record, + metadata?: RequestMetadata, ): Promise; // Misc surface required by hooks/state @@ -182,6 +183,6 @@ export interface InspectorClientProtocol extends InspectorClientEventTarget { argumentName: string, argumentValue: string, context?: Record, - metadata?: Record, + metadata?: RequestMetadata, ): Promise<{ values: string[]; total?: number; hasMore?: boolean }>; } diff --git a/core/mcp/node/servers.ts b/core/mcp/node/servers.ts index 9e10c69a..2f296c12 100644 --- a/core/mcp/node/servers.ts +++ b/core/mcp/node/servers.ts @@ -49,7 +49,7 @@ export function headersToServerSettings( return { headers: Object.entries(headers).map(([key, value]) => ({ key, value })), env: [], - metadata: [], + metadata: {}, connectionTimeout: 0, requestTimeout: 0, taskTtl: DEFAULT_TASK_TTL_MS, diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 6f778c66..0b3bffe5 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -41,6 +41,7 @@ import { } from "../../types.js"; import type { InspectorServerSettings, + RequestMetadata, MCPConfig, MCPServerConfig, StdioServerConfig, @@ -1212,6 +1213,11 @@ export function createRemoteApp( // credentials are intentionally lost on first read (hard cutover per // #1358 decision 4). Users re-enter via the form or hand-edit into the // flat shape. + // A plain JSON object — the container `_meta` requires. Values are + // deliberately unchecked: `_meta` takes arbitrary JSON (#1910), and anything + // that survived `JSON.parse` already is. + const isJsonObject = (v: unknown): v is RequestMetadata => + v !== null && typeof v === "object" && !Array.isArray(v); const isStringRecord = (v: unknown): v is Record => { if (v === null || typeof v !== "object" || Array.isArray(v)) return false; for (const val of Object.values(v as Record)) { @@ -1324,10 +1330,18 @@ export function createRemoteApp( ); delete valObj.headers; } - if ("metadata" in valObj && !isKvArray(valObj.metadata)) { + // A JSON object post-#1910; the pre-#1910 `{ key, value }[]` pair array + // is still accepted so an existing file keeps working (it is normalized + // by `normalizeStoredMetadata` on the way into the settings shape, and + // rewritten as an object on the next save). + if ( + "metadata" in valObj && + !isJsonObject(valObj.metadata) && + !isKvArray(valObj.metadata) + ) { logWarn( { route: "/api/servers", id, droppedKey: "metadata" }, - "Dropping malformed `metadata` field — expected `Array<{ key: string, value: string }>`.", + "Dropping malformed `metadata` field — expected a JSON object.", ); delete valObj.metadata; } @@ -1556,10 +1570,14 @@ export function createRemoteApp( error: "settings.headers must be an array of { key, value }", }; } - if (!isKvArray(obj.metadata)) { + // `_meta` takes any JSON, so metadata crosses the wire as a plain JSON + // object rather than the `{ key, value }` rows headers/env still use + // (#1910). Only the container is checked — the values are arbitrary JSON + // by design, and anything that survived `c.req.json()` already is. + if (!isJsonObject(obj.metadata)) { return { ok: false, - error: "settings.metadata must be an array of { key, value }", + error: "settings.metadata must be a JSON object", }; } // env is optional on the wire (older clients / non-stdio servers won't send @@ -1735,7 +1753,7 @@ export function createRemoteApp( // Absent → empty list, matching the read side. The write-through drops // empty-key rows and clears `config.env` when the list is empty. env: isKvArray(obj.env) ? obj.env : [], - metadata: obj.metadata as { key: string; value: string }[], + metadata: obj.metadata as RequestMetadata, connectionTimeout: obj.connectionTimeout as number, requestTimeout: obj.requestTimeout as number, // Absent → product default, matching the read side diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index f4be5f4a..5f4b10bc 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -15,6 +15,7 @@ import { import type { Root } from "@modelcontextprotocol/client"; import type { InspectorServerSettings, + RequestMetadata, MCPConfig, MCPServerConfig, ServerEntry, @@ -28,6 +29,7 @@ import { envSecretField, } from "../auth/secret-fields.js"; import { toRecord } from "../json/jsonUtils.js"; +import type { JsonValue } from "../json/jsonUtils.js"; // The full set of valid `type` discriminator values, used to reject anything // else read off disk so unknown strings can't propagate to narrowing sites. @@ -194,6 +196,60 @@ export function envPairsToRecord( return out; } +/** + * Read a server's `metadata` off disk into the in-memory `RequestMetadata` + * object, accepting both the current object shape and the pre-#1910 + * `{ key, value }[]` pair array. + * + * The pair array is the shape every `mcp.json` written before #1910 carries, so + * dropping it would silently stop sending a user's configured `_meta` on the + * first read of an existing file. It is read here and never written back: + * `inspectorSettingsToStoredFields` emits only the object form, so the file is + * migrated the next time the entry is saved. + * + * Anything else — a string, a number, an array of non-pairs — is not metadata + * the Inspector can send, so it is dropped with a warning rather than half- + * interpreted, following `cleanRoots` / `cleanAuthorizationParams` above. A + * pair whose key is blank is skipped for the same reason the write side omits + * it: `_meta` has no meaningful empty key. + */ +export function normalizeStoredMetadata( + metadata: StoredMCPServer["metadata"] | unknown, +): RequestMetadata { + if (metadata === undefined || metadata === null) return {}; + + if (Array.isArray(metadata)) { + const out: RequestMetadata = {}; + for (const entry of metadata) { + if ( + typeof entry !== "object" || + entry === null || + typeof (entry as { key?: unknown }).key !== "string" + ) { + console.warn( + "Ignoring malformed legacy metadata entry (expected `{ key, value }`):", + entry, + ); + continue; + } + const { key, value } = entry as { key: string; value?: JsonValue }; + if (key.trim() === "") continue; + out[key] = value ?? ""; + } + return out; + } + + if (typeof metadata === "object") { + return { ...(metadata as RequestMetadata) }; + } + + console.warn( + "Ignoring malformed `metadata` (expected a JSON object):", + metadata, + ); + return {}; +} + /** * Validate a server's `oauth.authorizationParams` as it comes off disk, or * `undefined` when there is nothing usable. (#2018) @@ -349,7 +405,7 @@ export function storedFieldsToInspectorSettings( const settings: InspectorServerSettings = { headers: headersPairs, env: envRecordToPairs(stored.env), - metadata: stored.metadata ?? [], + metadata: normalizeStoredMetadata(stored.metadata), connectionTimeout: stored.connectionTimeout ?? 0, requestTimeout: stored.requestTimeout ?? 0, // Unlike the timeouts (0 = "SDK default"), task TTL has a concrete product @@ -458,9 +514,11 @@ export function inspectorSettingsToStoredFields( out.headers = headersRecord; } - const metadataFiltered = settings.metadata.filter((m) => m.key.trim() !== ""); - if (metadataFiltered.length > 0) { - out.metadata = metadataFiltered; + // Blank-key entries can't exist in the object form (an object has no blank + // key the user can leave mid-edit, the way a `{key,value}` row could), so + // there is nothing to filter — only the empty-object case to omit. + if (Object.keys(settings.metadata).length > 0) { + out.metadata = settings.metadata; } if (settings.connectionTimeout > 0) { diff --git a/core/mcp/state/managedListState.ts b/core/mcp/state/managedListState.ts index eaf51f5d..bfe6467a 100644 --- a/core/mcp/state/managedListState.ts +++ b/core/mcp/state/managedListState.ts @@ -19,6 +19,7 @@ import type { } from "@modelcontextprotocol/client"; import { isClientDecodeRejection } from "../listSalvage.js"; import { isTerminalStatus } from "../types.js"; +import type { RequestMetadata } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; /** @@ -67,7 +68,7 @@ export interface ManagedListConfig { fetchAll: ( client: InspectorClientProtocol, cacheMode: CacheMode | undefined, - metadata: Record | undefined, + metadata: RequestMetadata | undefined, ) => Promise; /** * Whether this list drives a list-changed indicator. When true, a @@ -97,7 +98,7 @@ export abstract class ManagedListState< protected items: T[] = []; protected client: InspectorClientProtocol | null = null; private unsubscribe: (() => void) | null = null; - private _metadata: Record | undefined = undefined; + private _metadata: RequestMetadata | undefined = undefined; private listChanged = false; // The last fetch's failure, kept as observable state so a load that fails // (a transport error, or a result the SDK codec rejects as invalid) is @@ -282,7 +283,7 @@ export abstract class ManagedListState< this.emit("errorChange", value); } - setMetadata(metadata?: Record): void { + setMetadata(metadata?: RequestMetadata): void { this._metadata = metadata; } @@ -299,7 +300,7 @@ export abstract class ManagedListState< * the `list_changed` auto-refresh) catch it explicitly (#1953). */ async refresh( - metadata?: Record, + metadata?: RequestMetadata, cacheMode?: CacheMode, ): Promise { let next: T[] | null; @@ -336,7 +337,7 @@ export abstract class ManagedListState< * the console; empty list is the right semantics). */ private async fetchItems( - metadata: Record | undefined, + metadata: RequestMetadata | undefined, cacheMode: CacheMode | undefined, ): Promise { const client = this.client; diff --git a/core/mcp/state/pagedPromptsState.ts b/core/mcp/state/pagedPromptsState.ts index 80fbe6ca..5d15ae5c 100644 --- a/core/mcp/state/pagedPromptsState.ts +++ b/core/mcp/state/pagedPromptsState.ts @@ -15,6 +15,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { Prompt } from "@modelcontextprotocol/client"; import { isTerminalStatus } from "../types.js"; +import type { RequestMetadata } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; import type { PagePaginationState } from "./pagedToolsState.js"; @@ -112,7 +113,7 @@ export class PagedPromptsState extends TypedEventTarget, + metadata?: RequestMetadata, ): Promise { const c = this.client; if (!c || c.getStatus() !== "connected") { diff --git a/core/mcp/state/pagedResourceTemplatesState.ts b/core/mcp/state/pagedResourceTemplatesState.ts index 2fd2e2af..efbedbd2 100644 --- a/core/mcp/state/pagedResourceTemplatesState.ts +++ b/core/mcp/state/pagedResourceTemplatesState.ts @@ -14,6 +14,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprotocol/client"; import { isTerminalStatus } from "../types.js"; +import type { RequestMetadata } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; export interface PagedResourceTemplatesStateEventMap { @@ -59,7 +60,7 @@ export class PagedResourceTemplatesState extends TypedEventTarget, + metadata?: RequestMetadata, ): Promise { const c = this.client; if (!c || c.getStatus() !== "connected") { diff --git a/core/mcp/state/pagedResourcesState.ts b/core/mcp/state/pagedResourcesState.ts index bd5194ac..60640faf 100644 --- a/core/mcp/state/pagedResourcesState.ts +++ b/core/mcp/state/pagedResourcesState.ts @@ -16,6 +16,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { Resource } from "@modelcontextprotocol/client"; import { isTerminalStatus } from "../types.js"; +import type { RequestMetadata } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; import type { PagePaginationState } from "./pagedToolsState.js"; @@ -113,7 +114,7 @@ export class PagedResourcesState extends TypedEventTarget, + metadata?: RequestMetadata, ): Promise { const c = this.client; if (!c || c.getStatus() !== "connected") { diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 79475bc4..1b94f9ea 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -24,7 +24,7 @@ import type { OAuthClientProvider } from "@modelcontextprotocol/client"; import type { Transport } from "@modelcontextprotocol/client"; import type { InspectorLogger } from "../logging/logger.js"; import type { AppElicitationRenderer } from "./appElicitation.js"; -import type { JsonValue } from "../json/jsonUtils.js"; +import type { JsonObject, JsonValue } from "../json/jsonUtils.js"; import type { ClientConfig, EnterpriseManagedAuthIdpConfig, @@ -99,11 +99,20 @@ export type StoredMCPServer = MCPServerConfig & { */ headers?: Record; /** - * Default `_meta` keys merged into every outgoing MCP request. Inspector- - * specific (no analog in the broader mcp.json ecosystem), so the pair-array - * shape is preserved on disk and in memory. + * Default `_meta` payload merged into every outgoing MCP request. Inspector- + * specific (no analog in the broader mcp.json ecosystem). + * + * A JSON object, so a value may be any JSON — object, array, number, boolean, + * `null` — not just a string (#1910). Nothing in the MCP spec restricts + * `_meta` to string values, and the SDK models it as a passthrough object; + * the string-pair restriction was the Inspector's own. + * + * The pre-#1910 on-disk shape was a `{ key, value }[]` pair array. It is + * still **read** (see `normalizeStoredMetadata` in `serverList.ts`) so an + * existing `mcp.json` keeps working, but it is never written back — a + * round-trip through the Inspector rewrites the field as an object. */ - metadata?: { key: string; value: string }[]; + metadata?: JsonObject; /** * Protocol era to negotiate with this server (`"legacy" | "auto" | "modern"`), * orthogonal to the transport `type`. Inspector-specific (no analog in the @@ -361,6 +370,19 @@ export interface ServerState { stderrLogs: StderrLogEntry[]; } +/** + * A `_meta` payload: a JSON object whose values may be any JSON, not just + * strings (#1910). + * + * The MCP spec places no restriction on `_meta` value types and the SDK models + * the field as a passthrough object, so the Inspector must be able to send — + * and to record — nested objects, arrays, numbers, booleans and `null`. Used + * for both the per-server default payload (`InspectorServerSettings.metadata`, + * `InspectorClientOptions.defaultMetadata`) and the per-call metadata every + * request verb accepts. + */ +export type RequestMetadata = Record; + /** * Represents a complete resource read invocation, including request parameters, * response, and metadata. @@ -369,7 +391,7 @@ export interface ResourceReadInvocation { result: ReadResourceResult; timestamp: Date; uri: string; - metadata?: Record; + metadata?: RequestMetadata; } /** @@ -382,7 +404,7 @@ export interface ResourceTemplateReadInvocation { result: ReadResourceResult; timestamp: Date; params: Record; - metadata?: Record; + metadata?: RequestMetadata; } /** @@ -394,7 +416,7 @@ export interface PromptGetInvocation { timestamp: Date; name: string; params?: Record; - metadata?: Record; + metadata?: RequestMetadata; } /** @@ -408,7 +430,7 @@ export interface ToolCallInvocation { timestamp: Date; success: boolean; error?: string; - metadata?: Record; + metadata?: RequestMetadata; /** * Set only on the `skipOutputValidation` path: present when the (delivered) * result's structuredContent does NOT match the tool's declared outputSchema. @@ -689,7 +711,12 @@ export function eraToVersionNegotiation( */ export interface InspectorServerSettings { headers: { key: string; value: string }[]; - metadata: { key: string; value: string }[]; + /** + * Default `_meta` payload for this server, edited as a JSON object rather + * than key/value rows so a value can be any JSON (#1910). Always present; + * `{}` means "send no default metadata". + */ + metadata: RequestMetadata; /** * Environment variables for stdio servers, edited as controlled key/value * rows (mirrors `headers`). Only meaningful for stdio transports; non-stdio @@ -1075,7 +1102,7 @@ export interface InspectorClientOptions { * site metadata wins on key collision. Set this from `InspectorServerSettings.metadata` * so persisted server-wide metadata reaches the wire on the first request. */ - defaultMetadata?: Record; + defaultMetadata?: RequestMetadata; /** * Optional per-server runtime settings forwarded to the transport factory diff --git a/core/react/usePagedPrompts.ts b/core/react/usePagedPrompts.ts index 3e3efd44..6f62c524 100644 --- a/core/react/usePagedPrompts.ts +++ b/core/react/usePagedPrompts.ts @@ -1,5 +1,6 @@ import { useState, useEffect, useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; +import type { RequestMetadata } from "../mcp/types.js"; import type { PagedPromptsState, PagedPromptsStateEventMap, @@ -23,7 +24,7 @@ export interface UsePagedPromptsResult { error: Error | null; loadPage: ( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ) => Promise; clear: () => void; } @@ -84,7 +85,7 @@ export function usePagedPrompts( const loadPage = useCallback( async ( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise => { if (!pagedPromptsState || !client) { return { prompts: [], nextCursor: undefined }; diff --git a/core/react/usePagedResourceTemplates.ts b/core/react/usePagedResourceTemplates.ts index ab4707a4..c5202596 100644 --- a/core/react/usePagedResourceTemplates.ts +++ b/core/react/usePagedResourceTemplates.ts @@ -1,5 +1,6 @@ import { useState, useEffect, useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; +import type { RequestMetadata } from "../mcp/types.js"; import type { PagedResourceTemplatesState, PagedResourceTemplatesStateEventMap, @@ -12,7 +13,7 @@ export interface UsePagedResourceTemplatesResult { resourceTemplates: ResourceTemplate[]; loadPage: ( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ) => Promise; clear: () => void; } @@ -58,7 +59,7 @@ export function usePagedResourceTemplates( const loadPage = useCallback( async ( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise => { if (!pagedResourceTemplatesState || !client) { return { resourceTemplates: [], nextCursor: undefined }; diff --git a/core/react/usePagedResources.ts b/core/react/usePagedResources.ts index 3e5e3f8a..00b52a26 100644 --- a/core/react/usePagedResources.ts +++ b/core/react/usePagedResources.ts @@ -1,5 +1,6 @@ import { useState, useEffect, useCallback } from "react"; import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; +import type { RequestMetadata } from "../mcp/types.js"; import type { PagedResourcesState, PagedResourcesStateEventMap, @@ -23,7 +24,7 @@ export interface UsePagedResourcesResult { error: Error | null; loadPage: ( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ) => Promise; clear: () => void; } @@ -90,7 +91,7 @@ export function usePagedResources( const loadPage = useCallback( async ( cursor?: string, - metadata?: Record, + metadata?: RequestMetadata, ): Promise => { if (!pagedResourcesState || !client) { return { resources: [], nextCursor: undefined }; diff --git a/docs/mcp-server-configuration.md b/docs/mcp-server-configuration.md index ac105f3a..c6bbd29b 100644 --- a/docs/mcp-server-configuration.md +++ b/docs/mcp-server-configuration.md @@ -168,7 +168,7 @@ These have no analog in the broader `mcp.json` ecosystem. Each is **omitted on w | `protocolEra` | `"legacy"` | `"legacy"` \| `"auto"` \| `"modern"` — which protocol era to negotiate, orthogonal to the transport | | `modernLogLevel` | `"debug"` | Per-request log level stamped on modern connections, or `"off"`. Legacy connections ignore it | | `roots` | — | Roots advertised via the `roots` client capability; each is `{ uri, name? }` | -| `metadata` | — | Default `_meta` keys merged into every outgoing request | +| `metadata` | — | Default `_meta` payload merged into every outgoing request — a JSON object, values may be any JSON | | `connectionTimeout` / `requestTimeout` | — | Timeouts in ms | | `taskTtl` | `60000` | TTL in ms for tasks created via "Run as task" (`DEFAULT_TASK_TTL_MS`) | | `autoRefreshOnListChanged` | `false` | Refresh lists automatically on `*/list_changed` instead of only flagging the indicator | @@ -177,6 +177,18 @@ These have no analog in the broader `mcp.json` ecosystem. Each is **omitted on w | `maxFetchRequests` | `1000` | Network-log retention for this server (`DEFAULT_MAX_FETCH_REQUESTS`); `0` means unlimited | | `oauth` | — | `{ clientId, clientSecret, scopes, authorizationParams, authorizationUrl, tokenUrl, enterpriseManaged, onInsufficientScope }` | +`metadata` is a JSON **object**, and its values may be any JSON — an object, an array, a number, a boolean, `null` — not only a string. Nothing in the MCP spec restricts `_meta` value types, and the SDK models the field as a passthrough object, so the Inspector does not narrow it either ([#1910](https://github.com/modelcontextprotocol/inspector/issues/1910)). Edit it in Server Settings → Request Metadata, which is a JSON editor rather than the key/value rows `headers` and `env` use; text that is not a JSON object is flagged inline and not applied. + +```jsonc +"metadata": { + "tenant": "acme", + "trace": { "id": "abc123", "sampled": true }, + "features": ["apps", "tasks"] +} +``` + +> **Reading an older file.** Before #1910 this field was a `[{ "key": …, "value": … }]` pair array. That shape is still **read**, so an existing `mcp.json` keeps working unchanged; it is never written back, so the field is rewritten as an object the next time the Inspector saves the entry. + `oauth.authorizationParams` is a string→string record of extra query parameters merged into the OAuth **authorization request** URL only — never the token request. Use it for provider-specific hints the core specs don't standardize (Keycloak's `kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's `audience`). The protocol-critical parameters — `client_id`, `code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`, `response_type`, `scope`, `state` — are **reserved**: the web form rejects them inline, and any that reach the merge anyway are dropped with a warning rather than overriding what the flow set (overriding them breaks PKCE, the CSRF state binding, or RFC 8707). Edit them in Server Settings → Authorization ("Additional authorization parameters"), beside Scopes. `oauth.authorizationUrl` and `oauth.tokenUrl` override the `authorization_endpoint` and `token_endpoint` that authorization-server metadata discovery resolved. The Inspector deliberately has no such fields by default — it resolves both from the AS's metadata document, exactly as a real MCP host does — but a server under development often advertises its *production* authorization server while you want to hit staging. Set either (they are independent) to an absolute `http(s)` URL and it replaces what the metadata returned, for both the authorization request and the token request; leave blank to use discovery. A malformed value is flagged inline in the form and dropped with a warning at connect time rather than failing the connection. Edit them in Server Settings → Authorization ("Authorization URL override" / "Token URL override"). From 3a64ce7f4e9d0fe6b1199b256d239f5694715b1f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 12:29:27 -0400 Subject: [PATCH 07/35] refactor: move the JSON-object draft parser into utils `react-refresh/only-export-components` rejects a non-component export from a component module, so `parseObjectDraft` could not live beside `JsonObjectInput`. It is a pure transform, which puts it in `utils` under the `lib`/`utils` rule anyway. Also render the metadata hint's inline literals through `` rather than markdown backticks, which a Mantine `Text` shows verbatim. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- .../JsonObjectInput/JsonObjectInput.test.tsx | 46 +----------------- .../JsonObjectInput/JsonObjectInput.tsx | 38 ++------------- .../ServerSettingsForm/ServerSettingsForm.tsx | 8 ++-- clients/web/src/utils/jsonObjectDraft.test.ts | 46 ++++++++++++++++++ clients/web/src/utils/jsonObjectDraft.ts | 47 +++++++++++++++++++ 5 files changed, 101 insertions(+), 84 deletions(-) create mode 100644 clients/web/src/utils/jsonObjectDraft.test.ts create mode 100644 clients/web/src/utils/jsonObjectDraft.ts diff --git a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx index 37d02dfb..b1da0ffb 100644 --- a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx +++ b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx @@ -4,7 +4,7 @@ import { screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import type { JsonObject } from "@inspector/core/json/jsonUtils.js"; import { renderWithMantine } from "../../../test/renderWithMantine"; -import { JsonObjectInput, parseObjectDraft } from "./JsonObjectInput"; +import { JsonObjectInput } from "./JsonObjectInput"; const LABEL = "Payload JSON"; @@ -36,50 +36,6 @@ function box() { return screen.getByLabelText(LABEL) as HTMLTextAreaElement; } -describe("parseObjectDraft", () => { - it("reads empty text as the empty object rather than an error", () => { - expect(parseObjectDraft("")).toEqual({ ok: true, value: {} }); - expect(parseObjectDraft(" \n ")).toEqual({ ok: true, value: {} }); - }); - - it("accepts a JSON object with values of any JSON type", () => { - const result = parseObjectDraft( - '{"s":"a","n":1,"b":true,"z":null,"arr":[1,{"k":2}],"o":{"deep":{"er":1}}}', - ); - expect(result).toEqual({ - ok: true, - value: { - s: "a", - n: 1, - b: true, - z: null, - arr: [1, { k: 2 }], - o: { deep: { er: 1 } }, - }, - }); - }); - - it("rejects text that is not JSON", () => { - expect(parseObjectDraft('{"a":')).toEqual({ - ok: false, - error: "Not valid JSON — changes are not applied", - }); - }); - - it.each([ - ["an array", "[1,2]"], - ["a string", '"hello"'], - ["a number", "42"], - ["null", "null"], - ])("rejects valid JSON that is not an object: %s", (_label, text) => { - const result = parseObjectDraft(text); - expect(result.ok).toBe(false); - expect(result.ok === false && result.error).toMatch( - /Must be a JSON object/, - ); - }); -}); - describe("JsonObjectInput", () => { it("opens with the value serialized two-space, nesting intact", () => { renderWithMantine(); diff --git a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx index 93df2516..fbec5ee4 100644 --- a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx +++ b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx @@ -2,6 +2,7 @@ import { useState } from "react"; import { JsonInput } from "@mantine/core"; import type { JsonObject } from "@inspector/core/json/jsonUtils.js"; import { useValueChange } from "../../../hooks/useValueChange"; +import { parseJsonObjectDraft } from "../../../utils/jsonObjectDraft"; const ObjectJsonInput = JsonInput.withProps({ formatOnBlur: true, @@ -15,39 +16,6 @@ function serialize(value: JsonObject): string { return JSON.stringify(value, null, 2); } -/** A plain JSON object, i.e. not an array and not a scalar. */ -function isJsonObject(value: unknown): value is JsonObject { - return value !== null && typeof value === "object" && !Array.isArray(value); -} - -/** - * What a draft's text currently means: the object to emit, or why it can't be. - * - * Empty text is the object `{}` rather than an error — clearing the box is how - * "send no metadata" is spelled, and an empty editor showing a red error would - * read as broken. - */ -type DraftState = - | { ok: true; value: JsonObject } - | { ok: false; error: string }; - -export function parseObjectDraft(text: string): DraftState { - if (text.trim() === "") return { ok: true, value: {} }; - let parsed: unknown; - try { - parsed = JSON.parse(text); - } catch { - return { ok: false, error: "Not valid JSON — changes are not applied" }; - } - if (!isJsonObject(parsed)) { - return { - ok: false, - error: "Must be a JSON object (`{ … }`) — changes are not applied", - }; - } - return { ok: true, value: parsed }; -} - export interface JsonObjectInputProps { value: JsonObject; onChange: (value: JsonObject) => void; @@ -92,7 +60,7 @@ export function JsonObjectInput({ // parses to, so an external reset (switching servers, a reloaded catalog) // reaches the box while an in-progress edit is left alone. Compared through // canonical JSON because the parent hands back a fresh object each time. - const parsed = parseObjectDraft(draft); + const parsed = parseJsonObjectDraft(draft); const draftJson = parsed.ok ? serialize(parsed.value) : null; useValueChange(serialize(value), (next) => { if (next !== draftJson) setDraft(next); @@ -105,7 +73,7 @@ export function JsonObjectInput({ error={parsed.ok ? undefined : parsed.error} onChange={(text) => { setDraft(text); - const result = parseObjectDraft(text); + const result = parseJsonObjectDraft(text); if (result.ok) onChange(result.value); }} /> diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index 36179c9b..0a61a8db 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -729,10 +729,10 @@ export function ServerSettingsForm({ - Metadata merged into the `_meta` field of every MCP request to - this server, as a JSON object. Values may be any JSON — objects, - arrays, numbers, booleans, `null` — not just strings. Leave it as{" "} - {"{}"} to send none. + Metadata merged into the _meta field of every MCP + request to this server, as a JSON object. Values may be any JSON — + objects, arrays, numbers, booleans, null — not just + strings. Leave it as {"{}"} to send none. { + it("reads empty text as the empty object rather than an error", () => { + expect(parseJsonObjectDraft("")).toEqual({ ok: true, value: {} }); + expect(parseJsonObjectDraft(" \n ")).toEqual({ ok: true, value: {} }); + }); + + it("accepts a JSON object with values of any JSON type", () => { + const result = parseJsonObjectDraft( + '{"s":"a","n":1,"b":true,"z":null,"arr":[1,{"k":2}],"o":{"deep":{"er":1}}}', + ); + expect(result).toEqual({ + ok: true, + value: { + s: "a", + n: 1, + b: true, + z: null, + arr: [1, { k: 2 }], + o: { deep: { er: 1 } }, + }, + }); + }); + + it("rejects text that is not JSON", () => { + expect(parseJsonObjectDraft('{"a":')).toEqual({ + ok: false, + error: "Not valid JSON — changes are not applied", + }); + }); + + it.each([ + ["an array", "[1,2]"], + ["a string", '"hello"'], + ["a number", "42"], + ["null", "null"], + ])("rejects valid JSON that is not an object: %s", (_label, text) => { + const result = parseJsonObjectDraft(text); + expect(result.ok).toBe(false); + expect(result.ok === false && result.error).toMatch( + /Must be a JSON object/, + ); + }); +}); diff --git a/clients/web/src/utils/jsonObjectDraft.ts b/clients/web/src/utils/jsonObjectDraft.ts new file mode 100644 index 00000000..b5397079 --- /dev/null +++ b/clients/web/src/utils/jsonObjectDraft.ts @@ -0,0 +1,47 @@ +import type { JsonObject } from "@inspector/core/json/jsonUtils.js"; + +/** + * What a JSON-object editor's draft text currently means: the object to emit, + * or why it cannot be emitted. + */ +export type JsonObjectDraft = + | { ok: true; value: JsonObject } + | { ok: false; error: string }; + +/** A plain JSON object, i.e. not an array and not a scalar. */ +export function isJsonObject(value: unknown): value is JsonObject { + return value !== null && typeof value === "object" && !Array.isArray(value); +} + +/** + * Read a JSON-object editor's draft text. + * + * Empty text is the object `{}` rather than an error — clearing the box is how + * "send nothing" is spelled, and an empty editor wearing a red error would read + * as broken. + * + * The two failure modes are reported separately because they are different + * mistakes: text that is not JSON at all is usually mid-edit, while text that + * is valid JSON but not an object (`[1,2]`, `"a"`, `42`) is a shape the caller + * cannot use no matter how finished it is. + * + * Lives in `utils` rather than beside its component because it is a pure + * transform — and because exporting it from the component's module trips + * `react-refresh/only-export-components`. + */ +export function parseJsonObjectDraft(text: string): JsonObjectDraft { + if (text.trim() === "") return { ok: true, value: {} }; + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + return { ok: false, error: "Not valid JSON — changes are not applied" }; + } + if (!isJsonObject(parsed)) { + return { + ok: false, + error: "Must be a JSON object (`{ … }`) — changes are not applied", + }; + } + return { ok: true, value: parsed }; +} From 04b5e70a5393f084394ec15ff26998a20f353e63 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 12:56:13 -0400 Subject: [PATCH 08/35] fix(a11y): make the form error color meet WCAG AA in both schemes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mantine's `--mantine-color-error` defaults to `red-6` in light and `red-8` in dark. Both fail the 4.5:1 AA threshold that applies at the 12px size it renders input error text: 4.48:1 on white, and 1.93:1 on our charcoal body. Override to `red-7` (5.72:1) and `red-4` (6.44:1) — the latter matching the one-to-two-step lift App.css already applies to the semantic status/log reds for dark. Surfaced by the JsonObjectInput stories, which are the first in the repo to render an input error state and so the first to put this in front of the Storybook a11y gate. It is app-wide, not specific to that component — every form error in the Inspector was affected. It has to go through `cssVariablesResolver`: MantineProvider injects these variables into a `