From 37e156c035f926f365ea39b29b7fd68df223160d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 22:21:37 -0400 Subject: [PATCH 1/3] fix(web): surface failed resource subscribe/unsubscribe (#2174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both handlers discarded their promise with a bare `void`. Nothing owned those failures: `subscribeToResource` throws when the client is disconnected, when the server declares no subscription support, and (wrapped) on a failed request — so a subscribe that failed did nothing visible and surfaced only as an unhandled browser rejection, while every neighbouring command in the hook already routed through the shared recovery. Both now go through `runCommandInBackground`, so a mid-session 401 recovers the way it does on a refresh and any other failure toasts. The source is `ambient`, not `resource`: a `resource` step-up failure is routed into `readResourceState` — the read-preview panel — which describes a read of whatever is selected rather than this subscribe, so marking it errored would contradict a read that succeeded. Subscribing has no panel of its own, the same position the refreshes and the pagination toggle are in. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- .../web/src/hooks/useServerCommands.test.tsx | 112 ++++++++++++++++++ clients/web/src/hooks/useServerCommands.tsx | 33 +++++- 2 files changed, 141 insertions(+), 4 deletions(-) diff --git a/clients/web/src/hooks/useServerCommands.test.tsx b/clients/web/src/hooks/useServerCommands.test.tsx index 8704d1b6..b8de9e2d 100644 --- a/clients/web/src/hooks/useServerCommands.test.tsx +++ b/clients/web/src/hooks/useServerCommands.test.tsx @@ -862,6 +862,118 @@ describe("subscriptions and completion", () => { expect(h.api().onSubscribeResource).toBeTypeOf("function"); }); + // #2174. Both used to discard the promise with a bare `void`, so a failure + // was invisible in the UI and surfaced only as an unhandled rejection. + it("toasts a failed subscribe rather than swallowing it", async () => { + const h = harness({ + client: client({ + subscribeToResource: vi + .fn() + .mockRejectedValue( + new Error("Server does not support resource subscriptions"), + ), + }), + }); + await act(async () => h.api().onSubscribeResource("u")); + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to subscribe to resource", + message: "Server does not support resource subscriptions", + }), + ), + ); + }); + + it("toasts a failed unsubscribe rather than swallowing it", async () => { + const h = harness({ + client: client({ + unsubscribeFromResource: vi + .fn() + .mockRejectedValue(new Error("Client is not connected")), + }), + }); + await act(async () => h.api().onUnsubscribeResource("u")); + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to unsubscribe from resource", + message: "Client is not connected", + }), + ), + ); + }); + + it("recovers a lapsed authorization on subscribe, and retries", async () => { + const recover = vi.fn().mockResolvedValue(true); + const subscribeToResource = vi + .fn() + .mockRejectedValueOnce(authError()) + .mockResolvedValue(undefined); + const h = harness({ + client: client({ subscribeToResource }), + activeServerId: "a", + recovery: { handleCommandScopedAuthRecovery: recover }, + }); + await act(async () => h.api().onSubscribeResource("u")); + await waitFor(() => expect(subscribeToResource).toHaveBeenCalledTimes(2)); + // `ambient`, not `resource`: a `resource` step-up failure is routed into + // the read-preview panel, which this command has nothing to do with. + expect(recover).toHaveBeenCalledWith( + expect.any(AuthRecoveryRequiredError), + { + serverId: "a", + source: "ambient", + retryOperation: expect.any(Function), + }, + ); + // The recovery owns the prompt, so the command itself stays quiet. + expect(notificationsMock.show).not.toHaveBeenCalled(); + }); + + it("recovers a lapsed authorization on unsubscribe, and retries", async () => { + const recover = vi.fn().mockResolvedValue(true); + const unsubscribeFromResource = vi + .fn() + .mockRejectedValueOnce(authError()) + .mockResolvedValue(undefined); + const h = harness({ + client: client({ unsubscribeFromResource }), + activeServerId: "a", + recovery: { handleCommandScopedAuthRecovery: recover }, + }); + await act(async () => h.api().onUnsubscribeResource("u")); + await waitFor(() => + expect(unsubscribeFromResource).toHaveBeenCalledTimes(2), + ); + expect(recover).toHaveBeenCalledWith( + expect.any(AuthRecoveryRequiredError), + { + serverId: "a", + source: "ambient", + retryOperation: expect.any(Function), + }, + ); + expect(notificationsMock.show).not.toHaveBeenCalled(); + }); + + it("does not toast a subscribe whose recovery was left unsatisfied", async () => { + // The recovery has taken over — a redirect is pending or a step-up prompt + // is open — so the wrapper resolves `undefined` rather than rejecting, and + // a toast here would talk over the prompt the user is looking at. + const recover = vi.fn().mockResolvedValue(false); + const h = harness({ + client: client({ + subscribeToResource: vi.fn().mockRejectedValue(authError()), + }), + activeServerId: "a", + recovery: { handleCommandScopedAuthRecovery: recover }, + }); + await act(async () => h.api().onSubscribeResource("u")); + await waitFor(() => expect(recover).toHaveBeenCalled()); + expect(notificationsMock.show).not.toHaveBeenCalled(); + }); + it("returns the completion values", async () => { const c = client(); const h = harness({ client: c }); diff --git a/clients/web/src/hooks/useServerCommands.tsx b/clients/web/src/hooks/useServerCommands.tsx index f4cb49f1..61aabae8 100644 --- a/clients/web/src/hooks/useServerCommands.tsx +++ b/clients/web/src/hooks/useServerCommands.tsx @@ -564,20 +564,45 @@ export function useServerCommands({ [inspectorClient, activeServerId, handleCommandScopedAuthRecovery], ); + // Both route through the shared recovery like every other command (#2174). + // They used to discard the promise with a bare `void`, which neither the + // callee nor anything else owned: `subscribeToResource` throws when the + // client is disconnected, when the server declares no subscription support, + // and (wrapped) on a failed request, so a subscribe that failed did nothing + // visible and surfaced only as an unhandled rejection. + // + // The source is `ambient`, not `resource`. A `resource` step-up failure is + // routed into `readResourceState` — the *preview* panel — which describes a + // read of whatever resource is selected, not this subscribe; marking it + // errored would contradict a read that succeeded. Subscribing has no panel + // of its own (the tile only flips its button label), which is the same + // position the refreshes and the pagination toggle are in, and they are + // `ambient` for the same reason. + // + // Both pass an `errorTitle`: nothing else records these failures, so without + // one the button would go on silently doing nothing. const onSubscribeResource = useCallback( (uri: string) => { if (!inspectorClient) return; - void inspectorClient.subscribeToResource(uri); + runCommandInBackground( + () => inspectorClient.subscribeToResource(uri), + "ambient", + "Failed to subscribe to resource", + ); }, - [inspectorClient], + [inspectorClient, runCommandInBackground], ); const onUnsubscribeResource = useCallback( (uri: string) => { if (!inspectorClient) return; - void inspectorClient.unsubscribeFromResource(uri); + runCommandInBackground( + () => inspectorClient.unsubscribeFromResource(uri), + "ambient", + "Failed to unsubscribe from resource", + ); }, - [inspectorClient], + [inspectorClient, runCommandInBackground], ); const onCompleteArgument = useCallback( From 9516299d8d7b91ea74f8205b5e6f1d7deda539b0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 22:57:29 -0400 Subject: [PATCH 2/3] fix(web): refuse a raw-JSON tool argument the schema would retype (#2171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Tools and Apps tabs' "Edit as JSON" editor sent its draft through the string-to-declared-type conversion `callTool` applies for the widget form, so `{"count":"01"}` against a numeric schema left as `{"count":1}` — the editor showed one payload and the wire carried another. Both JSON-authoring surfaces now refuse such a draft and name the value to rewrite. Edit-and-replay already did; this makes the Tools and Apps forms agree with it, which is the consistency #2171's acceptance asks for. `coercedArgumentNames` and `coercedArgumentsError` move into core/json so the two surfaces share one predicate and one sentence — the check runs the real conversion and compares rather than restating its rules, so it cannot drift from what would actually be sent. Enforcement is opt-in (`SchemaFormProps.enforceToolArgumentTypes`): an elicitation renders through the same form and its values are never converted, so enforcing there would refuse a draft for a reason that is not true of it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- clients/web/README.md | 23 ++++ .../AppDetailPanel/AppDetailPanel.test.tsx | 23 ++++ .../groups/AppDetailPanel/AppDetailPanel.tsx | 4 + .../groups/SchemaForm/SchemaForm.test.tsx | 84 +++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 61 ++++++++- .../ToolDetailPanel/ToolDetailPanel.test.tsx | 24 ++++ .../ToolDetailPanel/ToolDetailPanel.tsx | 4 + clients/web/src/lib/protocolReplay.ts | 43 +++---- clients/web/src/test/core/jsonUtils.test.ts | 118 ++++++++++++++++++ core/json/jsonUtils.ts | 78 +++++++++++- 10 files changed, 435 insertions(+), 27 deletions(-) diff --git a/clients/web/README.md b/clients/web/README.md index 1ea62b23..a72ed8df 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -298,6 +298,29 @@ Every surface in this client where JSON is **typed**, plus `ContentViewer`'s rea | `groups/ImportServerJsonPanel`, `groups/ExperimentalFeaturesPanel` | The panel owns the text and validates it itself. | | `elements/ContentViewer`'s JSON branch | Read-only — see below. | +**A raw-JSON draft is refused when the client would retype it.** `callTool` +converts every *string-valued* argument to the type the tool's `inputSchema` +declares, because the widget form hands everything over as text — so `"2"` +against a numeric field has to become `2`. A JSON draft already carries its own +types, so a value that conversion would touch is one whose visible text is not +what the wire would carry, and showing one payload while sending another is the +one thing an inspector must not do. Both JSON-authoring surfaces therefore +refuse such a draft and name the value to rewrite +([#2171](https://github.com/modelcontextprotocol/inspector/issues/2171)): +**Edit as JSON** in the Tools and Apps tabs (which had been sending it, coerced +and silently), and **Edit and replay**, which already did. + +One helper decides it for both — `coercedArgumentNames` in +[`core/json/jsonUtils.ts`](../../core/json/jsonUtils.ts), which runs the real +conversion and compares rather than restating its rules, so a surface cannot +drift from what would actually be sent. `coercedArgumentsError` is the sentence +they share, so the same refusal cannot be worded two ways. + +The check is **opt-in** (`SchemaFormProps.enforceToolArgumentTypes`), not +inferred from the schema: an elicitation renders through the same `SchemaForm` +and its values are never converted, so enforcing there would refuse a draft for +a reason that is not true of it. Only the Tools and Apps panels pass it. + **Read-only mode is what `ContentViewer` renders JSON as**, and through it every payload that reaches the app that way: Protocol and Network entries, tool results, structured output, resource previews, server cards. Highlighting is *not* what that buys — JSON already highlighted, via the lazily-imported Prism grammar `CodeHighlight` loads. What Ace adds is **folding**, line numbers and a gutter on a large payload. The Prism `json` grammar was dropped in the same change rather than kept beside it: two highlighters for one language drift, and nothing else asks for `json`. Two cases stay on the plain renderer — a `wrap={false}` caller (the server card's fixed-height, single-line box) and untyped text that only *looks* like JSON but does not parse, which in an editor would frame a server's prose as a malformed document. Two read-only JSON displays deliberately do **not** go through `ContentViewer`, and so are not on this editor — worth knowing before assuming it is the route for all of them. `ExperimentalFeaturesPanel` renders its JSON-RPC **response** in a read-only `Textarea` (its *request* box is on the editor), and `ConnectionInfoContent/OAuthTokenField` renders a decoded JWT in a `Code` block beside the raw token, where the decoded/raw toggle and the copy affordance belong to the field rather than to a viewer. Both are candidates for the same treatment; neither is in [#2151](https://github.com/modelcontextprotocol/inspector/issues/2151)'s scope. diff --git a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx index 687d2a81..31537192 100644 --- a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx +++ b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx @@ -136,6 +136,29 @@ describe("AppDetailPanel", () => { expect(screen.getByRole("button", { name: /open app/i })).toBeDisabled(); }); + // #2171: an App's arguments are a `tools/call` like any other, so its form + // opts into the same enforcement the Tools tab uses. + it("refuses a raw-JSON argument the schema would retype", async () => { + const user = userEvent.setup(); + const numericTool: Tool = { + name: "chart", + inputSchema: { + type: "object", + properties: { count: { type: "number" } }, + }, + }; + renderWithMantine( + , + ); + await user.click(screen.getByLabelText("Edit as JSON")); + await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}'); + + expect( + screen.getByText(/`count` would be converted to the type/), + ).toBeInTheDocument(); + expect(screen.getByRole("button", { name: /open app/i })).toBeDisabled(); + }); + it("invokes onOpenApp when the button is clicked", async () => { const user = userEvent.setup(); const onOpenApp = vi.fn(); diff --git a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx index 8d75f431..f2d27a06 100644 --- a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx +++ b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx @@ -89,6 +89,10 @@ export function AppDetailPanel({ // SchemaFormProps.resetKey. resetKey={tool.name} onValidityChange={setHasInvalidDraft} + // These values become `tools/call` arguments, so a raw-JSON draft the + // client would retype is refused rather than sent as something other + // than what the editor shows (#2171). + enforceToolArgumentTypes /> { function RawHarness({ initial = {}, onValidityChange, + enforceToolArgumentTypes, resetKey, schema: override, }: { initial?: Record; onValidityChange?: (hasInvalidDraft: boolean) => void; + enforceToolArgumentTypes?: boolean; resetKey?: string; schema?: InspectorFormSchema; }) { @@ -1850,6 +1852,7 @@ describe("SchemaForm raw JSON (#2151)", () => { onChange={setValues} resetKey={resetKey} onValidityChange={onValidityChange} + enforceToolArgumentTypes={enforceToolArgumentTypes} /> ); } @@ -1858,6 +1861,87 @@ describe("SchemaForm raw JSON (#2151)", () => { await user.click(screen.getByLabelText("Edit as JSON")); } + // #2171. The widgets hand every value over as text, so `callTool` retypes a + // string to whatever the schema declares — which for a JSON draft means the + // wire would carry something other than what the editor showed. The draft is + // refused instead, and the value to rewrite is named. + describe("tool-argument type enforcement", () => { + it("refuses a draft the client would retype, and says which value", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine( + , + ); + await enableRawJson(user); + await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}'); + + expect( + screen.getByText(/`count` would be converted to the type/), + ).toBeInTheDocument(); + // Not merely flagged: the value must not reach the parent either, or a + // submit gated on something else would still send it. + expect(onChange).not.toHaveBeenCalledWith({ count: "01" }); + }); + + it("blocks submission while such a draft is held", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + renderWithMantine( + , + ); + await enableRawJson(user); + await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}'); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + + // Rewritten with the declared type, it is sendable again. + await setAceTextByLabel(/Arguments JSON/, '{"count":1}'); + expect(onValidityChange).toHaveBeenLastCalledWith(false); + }); + + // The conversion only ever looks at strings, so a value already written + // with its declared type is not a mismatch — and neither is a string in a + // field the schema declares as one. + it("accepts values already written with the declared type", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine( + , + ); + await enableRawJson(user); + await setAceTextByLabel(/Arguments JSON/, '{"name":"a","count":2}'); + expect(screen.queryByText(/would be converted/)).toBeNull(); + expect(onChange).toHaveBeenLastCalledWith({ name: "a", count: 2 }); + }); + + // Off by default, and that default is load-bearing: an elicitation renders + // through this same form and its values are never converted, so enforcing + // there would refuse a draft for a reason that is not true of it. + it("does not enforce when the caller has not opted in", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine( + , + ); + await enableRawJson(user); + await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}'); + expect(screen.queryByText(/would be converted/)).toBeNull(); + expect(onChange).toHaveBeenLastCalledWith({ count: "01" }); + }); + }); + it("replaces the widgets with one editor holding the current values", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 4ea9fb95..60baca60 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -33,6 +33,10 @@ import { } from "../../../utils/jsonObjectDraft"; import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; import { useValueChange } from "../../../hooks/useValueChange"; +import { + coercedArgumentNames, + coercedArgumentsError, +} from "@inspector/core/json/jsonUtils.js"; import type { InspectorFormSchema, JsonSchemaConst, @@ -626,6 +630,7 @@ function SchemaNumberInput({ */ function parseRawArgumentsDraft( text: string, + coercionSchema?: InspectorFormSchema, ): { ok: true; value: Record } | { ok: false; error: string } { if (text.trim() === "") return { ok: true, value: {} }; let parsed: unknown; @@ -661,6 +666,21 @@ function parseRawArgumentsDraft( if (duplicate !== null) { return { ok: false, error: duplicateKeyError(duplicate) }; } + // Last, because it is the only check that needs a *valid* object to run + // against — and the only one that is about the schema rather than the JSON. + // `callTool` retypes every string-valued argument to what the schema + // declares, so a draft it would touch is one whose visible text is not what + // the wire would carry; refuse it here instead, and say which value to + // rewrite (#2171). Skipped entirely when no schema is supplied: only the + // Tools and Apps forms feed `tools/call`, and an elicitation's values are + // never converted, so imposing this there would block a submission for a + // reason that is not true of it. + if (coercionSchema) { + const coerced = coercedArgumentNames(coercionSchema, parsed); + if (coerced.length > 0) { + return { ok: false, error: coercedArgumentsError(coerced) }; + } + } return { ok: true, value: parsed }; } @@ -670,6 +690,12 @@ interface RawArgumentsFieldProps { disabled: boolean; /** Mirrors {@link SchemaFormProps.onValidityChange} for this one editor. */ onInvalidChange: (hasInvalidDraft: boolean) => void; + /** + * The tool `inputSchema` this draft's values will be sent against, when they + * will be — see {@link SchemaFormProps.enforceToolArgumentTypes}. Undefined + * turns the type check off. + */ + coercionSchema?: InspectorFormSchema; } /** @@ -697,6 +723,7 @@ function RawArgumentsField({ onChange, disabled, onInvalidChange, + coercionSchema, }: RawArgumentsFieldProps) { const [draft, setDraft] = useState(() => serializeJson(values)); // Canonical JSON of the last object this editor emitted, so the re-sync below @@ -705,7 +732,7 @@ function RawArgumentsField({ // change would then look external. See `JsonObjectInput` for the long form. const [echoed, setEchoed] = useState(() => serializeJson(values)); - const parsed = parseRawArgumentsDraft(draft); + const parsed = parseRawArgumentsDraft(draft, coercionSchema); useValueChange(serializeJson(values), (next) => { if (next === echoed) return; @@ -732,7 +759,7 @@ function RawArgumentsField({ maxLines={24} onChange={(text) => { setDraft(text); - const next = parseRawArgumentsDraft(text); + const next = parseRawArgumentsDraft(text, coercionSchema); if (!next.ok) return; setEchoed(serializeJson(next.value)); onChange(next.value); @@ -790,6 +817,30 @@ export interface SchemaFormProps { * inside the first the moment both were on. */ allowRawJson?: boolean; + /** + * Whether these values become `tools/call` arguments, and so are subject to + * the string-to-declared-type conversion `InspectorClient.callTool` applies + * (#2171). + * + * That conversion exists for the widgets, which hand every value over as + * text: `"2"` against a numeric field has to become `2`. A **raw-JSON** + * draft already carries its own types, so a value the conversion would touch + * is one whose visible text is not what the wire would carry — and showing + * one payload while sending another is the one thing an inspector must not + * do. With this on, the raw editor refuses such a draft and names the value + * to rewrite, exactly as the Edit-and-replay modal does for the same + * conversion. + * + * Off by default, and deliberately opt-in rather than inferred from the + * schema: an elicitation renders through this same form and its values are + * never converted, so the check would be refusing those drafts for a reason + * that is not true of them. Only the Tools and Apps panels pass it. + * + * Read against {@link SchemaFormProps.schema} — the tool's own `inputSchema`, + * root composition included, which is what the client resolves the + * conversion against too. + */ + enforceToolArgumentTypes?: boolean; } function getDefaultValue(fieldSchema: InspectorFormSchema): unknown { @@ -817,6 +868,7 @@ export function SchemaForm({ resetKey, onValidityChange, allowRawJson = true, + enforceToolArgumentTypes = false, }: SchemaFormProps) { // Composition at the root of the schema, flattened before anything is // rendered (#2123). `allOf` is folded into `base`; a top-level `oneOf`/`anyOf` @@ -1524,6 +1576,11 @@ export function SchemaForm({ onChange={handleRawArgumentsChange} disabled={disabled} onInvalidChange={reportRawJsonValidity} + // The schema as the CLIENT resolves it — the whole `inputSchema`, + // root composition and all — not the branch this form happens to be + // rendering. `coercedArgumentNames` does its own branch selection + // from the supplied values, the same way the conversion does. + coercionSchema={enforceToolArgumentTypes ? schema : undefined} /> ) : ( Object.entries(properties).map(([fieldName, fieldSchema]) => diff --git a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx index 298ce9fc..18a903c9 100644 --- a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx +++ b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx @@ -246,6 +246,30 @@ describe("ToolDetailPanel", () => { expect(onExecute).toHaveBeenCalledTimes(1); }); + // #2171: the panel opts its form into tool-argument type enforcement, so a + // raw-JSON draft the client would retype is refused here rather than sent as + // something other than what the editor showed. + it("refuses a raw-JSON argument the schema would retype", async () => { + const user = userEvent.setup(); + const numericTool: Tool = { + name: "add", + inputSchema: { + type: "object", + properties: { count: { type: "number" } }, + }, + }; + renderWithMantine( + , + ); + await user.click(screen.getByLabelText("Edit as JSON")); + await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}'); + + expect( + screen.getByText(/`count` would be converted to the type/), + ).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Execute Tool" })).toBeDisabled(); + }); + it("disables the Execute Tool button while executing and renders Cancel", () => { renderWithMantine( , diff --git a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx index 94ecc893..7a496a9a 100644 --- a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx +++ b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx @@ -355,6 +355,10 @@ export function ToolDetailPanel({ // tool's in-progress field text. See SchemaFormProps.resetKey. resetKey={resetKey ?? name} onValidityChange={setHasInvalidDraft} + // These values become `tools/call` arguments, so a raw-JSON draft the + // client would retype is refused rather than sent as something other + // than what the editor shows (#2171). + enforceToolArgumentTypes /> {progress && } diff --git a/clients/web/src/lib/protocolReplay.ts b/clients/web/src/lib/protocolReplay.ts index 95c545fd..063feaff 100644 --- a/clients/web/src/lib/protocolReplay.ts +++ b/clients/web/src/lib/protocolReplay.ts @@ -9,7 +9,10 @@ import type { MessageEntry } from "@inspector/core/mcp/types.js"; // `components → lib → utils` direction still holds: `LogEntryData` is the shape // the Logs screen renders, and this module exists to produce exactly that. import type { LogEntryData } from "../components/elements/LogEntry/LogEntry"; -import { convertToolParameters } from "@inspector/core/json/jsonUtils.js"; +import { + coercedArgumentNames, + coercedArgumentsError, +} from "@inspector/core/json/jsonUtils.js"; import { isReplayableProtocolMethod } from "../utils/replayableProtocolMethods"; // Derive `LogEntryData[]` from the MessageLog by filtering for the @@ -167,13 +170,19 @@ export function replayableParams( * spec types `GetPromptRequest.params.arguments` as `Record`, * so a string is the only thing a prompt argument can be. * - For **`tools/call`**, the mirror image. `callTool` runs every *string* - * entry through `convertToolParameters`, because the Tools form hands + * entry through `convertToolParameters`, because the widget form hands * everything over as text — so `{"count": "2"}` against a schema declaring a * number is sent as `{"count": 2}`. Detected by running that same conversion * and comparing, rather than by reimplementing its rules, so the two cannot * drift. Needs the `tool`; without one this check is skipped, since nothing * can be said about a coercion whose schema is unknown. * + * The Tools and Apps tabs' own "Edit as JSON" switch refuses the identical + * draft, through the identical helper (#2171). Both are JSON documents whose + * values already carry their types, so in both the conversion would send + * something other than what the editor showed — and one of the two answering + * differently is the inconsistency #2171 was filed to remove. + * * `name` and `uri` are deliberately not checked here: the dispatch already * refuses a missing or non-string one with a reason the caller surfaces as a * toast, so those fail visibly rather than silently. @@ -203,34 +212,20 @@ export function reshapedReplayParam( } } if (method === "tools/call" && tool) { - const coerced = coercedToolArgs(tool, args as Record); + // The same check the Tools/Apps form's raw-JSON editor makes, from the same + // helper, so the two JSON-authoring surfaces cannot disagree about which + // drafts are sendable (#2171). + const coerced = coercedArgumentNames( + tool.inputSchema, + args as Record, + ); if (coerced.length > 0) { - return `${coerced.join(", ")} would be converted to the type ${tool.name}'s schema declares — write the value with that type instead`; + return coercedArgumentsError(coerced, tool.name); } } return null; } -/** - * The string-valued argument names `callTool` would convert, per the tool's - * schema. - * - * Runs the conversion the client runs and compares, rather than restating its - * rules: `convertToolParameters` is the function on the other side, so asking - * it is the only way this cannot drift from what is actually sent. - */ -function coercedToolArgs(tool: Tool, args: Record): string[] { - const stringArgs: Record = {}; - for (const [key, value] of Object.entries(args)) { - if (typeof value === "string") stringArgs[key] = value; - } - if (Object.keys(stringArgs).length === 0) return []; - const converted = convertToolParameters(tool, stringArgs); - return Object.keys(stringArgs) - .filter((key) => converted[key] !== stringArgs[key]) - .map((key) => `\`${key}\``); -} - export async function replayProtocolRequest( client: ReplayClient, method: string, diff --git a/clients/web/src/test/core/jsonUtils.test.ts b/clients/web/src/test/core/jsonUtils.test.ts index 29f60d56..12f4877b 100644 --- a/clients/web/src/test/core/jsonUtils.test.ts +++ b/clients/web/src/test/core/jsonUtils.test.ts @@ -3,6 +3,8 @@ import { convertParameterValue, convertToolParameters, convertPromptArguments, + coercedArgumentNames, + coercedArgumentsError, toRecord, } from "@inspector/core/json/jsonUtils.js"; import type { Tool } from "@modelcontextprotocol/client"; @@ -61,6 +63,122 @@ describe("JSON Utils", () => { }); }); + // #2171: the predicate both raw-JSON editors refuse on. It must agree with + // the conversion exactly — a false negative sends a payload other than the + // one shown, and a false positive blocks a draft that was fine. + describe("coercedArgumentNames", () => { + const numeric: Tool = { + name: "add", + inputSchema: { + type: "object", + properties: { count: { type: "number" }, label: { type: "string" } }, + }, + }; + + it("names a string the schema would retype", () => { + expect( + coercedArgumentNames(numeric.inputSchema, { count: "01" }), + ).toEqual(["count"]); + }); + + // The conversion only inspects strings, so a value already written with + // its declared type is untouched and must not be reported. + it("ignores a value already of the declared type", () => { + expect(coercedArgumentNames(numeric.inputSchema, { count: 1 })).toEqual( + [], + ); + }); + + it("ignores a string the schema declares as a string", () => { + expect( + coercedArgumentNames(numeric.inputSchema, { label: "01" }), + ).toEqual([]); + }); + + // An argument the schema does not declare is passed through by the + // conversion, so there is nothing to warn about. + it("ignores an undeclared argument", () => { + expect(coercedArgumentNames(numeric.inputSchema, { extra: "x" })).toEqual( + [], + ); + }); + + it("reports every offending name, in supplied order", () => { + const two: Tool = { + name: "pair", + inputSchema: { + type: "object", + properties: { a: { type: "number" }, b: { type: "boolean" } }, + }, + }; + expect( + coercedArgumentNames(two.inputSchema, { a: "1", b: "true" }), + ).toEqual(["a", "b"]); + }); + + // Root composition (#2123): the declared type can live on a branch, and + // the conversion selects that branch from the supplied values — so this + // must too, or a branch's arguments would look unconvertible. + it("resolves a type declared on a root union branch", () => { + const union: Tool = { + name: "u", + inputSchema: { + type: "object", + oneOf: [ + { + type: "object", + properties: { + kind: { const: "n" }, + value: { type: "number" }, + }, + required: ["kind"], + }, + { + type: "object", + properties: { + kind: { const: "s" }, + value: { type: "string" }, + }, + required: ["kind"], + }, + ], + }, + }; + expect( + coercedArgumentNames(union.inputSchema, { kind: "n", value: "2" }), + ).toEqual(["value"]); + expect( + coercedArgumentNames(union.inputSchema, { kind: "s", value: "2" }), + ).toEqual([]); + }); + + it("says nothing when no argument is a string", () => { + expect(coercedArgumentNames(numeric.inputSchema, { count: 1 })).toEqual( + [], + ); + }); + }); + + describe("coercedArgumentsError", () => { + // Both surfaces refuse the same drafts, so they must not word it two ways; + // the tool name is included only where the caller knows it. + it("names the tool when it is known", () => { + expect(coercedArgumentsError(["count"], "add")).toBe( + "`count` would be converted to the type add's schema declares — write the value with that type instead", + ); + }); + + it("omits the tool when the caller is already rendering one", () => { + expect(coercedArgumentsError(["count"])).toBe( + "`count` would be converted to the type the schema declares — write the value with that type instead", + ); + }); + + it("lists several names", () => { + expect(coercedArgumentsError(["a", "b"], "pair")).toContain("`a`, `b`"); + }); + }); + describe("convertToolParameters", () => { const tool: Tool = { name: "test-tool", diff --git a/core/json/jsonUtils.ts b/core/json/jsonUtils.ts index 205db2ab..527f626e 100644 --- a/core/json/jsonUtils.ts +++ b/core/json/jsonUtils.ts @@ -294,12 +294,32 @@ function suppliedMatchesConst(value: string, constValue: unknown): boolean { export function convertToolParameters( tool: Tool, params: Record, +): Record { + return convertParametersForSchema(tool.inputSchema, params); +} + +/** + * {@link convertToolParameters} against a bare `inputSchema` rather than a + * `Tool`. + * + * Split out because the two callers hold different things: the client has the + * `Tool`, while a form holds only the schema it is rendering (which is that + * same object, structurally narrowed). Both must reach the SAME conversion — + * one of them decides what goes on the wire and the other decides whether to + * let the user send it, so a second implementation would let them disagree + * about which values are convertible. + */ +export function convertParametersForSchema( + inputSchema: unknown, + params: Record, ): Record { const result: Record = {}; // A property's schema can live on a root composition branch rather than on // the root itself (#2123); see `coercionProperties` for how the branch is // identified when it does. - const { base, branches } = resolveRootUnion(tool.inputSchema ?? {}); + const { base, branches } = resolveRootUnion( + (inputSchema ?? {}) as RootUnionSchema, + ); const properties = coercionProperties(base, branches, params); for (const [key, value] of Object.entries(params)) { const declared = properties[key]; @@ -338,6 +358,62 @@ export function convertToolParameters( return result; } +/** + * The argument names a `tools/call` would silently retype, per the tool's + * schema — empty when every value is already the type the schema declares. + * + * Runs the conversion the client runs and compares, rather than restating its + * rules: {@link convertParametersForSchema} is the function on the other side, + * so asking it is the only way a caller cannot drift from what is actually + * sent. + * + * Only string-valued arguments can be retyped, because that is all the + * conversion looks at — a JSON draft that already writes `2` as a number is + * passed through untouched, and is not reported here. + * + * Used by the two places a payload is authored as JSON rather than through + * widgets — the Tools/Apps form's "Edit as JSON" switch and the + * Edit-and-replay modal — each of which refuses a draft this returns anything + * for, so what the editor shows is what the wire carries (#2171). + */ +export function coercedArgumentNames( + inputSchema: unknown, + args: Record, +): string[] { + const stringArgs: Record = {}; + for (const [key, value] of Object.entries(args)) { + if (typeof value === "string") stringArgs[key] = value; + } + if (Object.keys(stringArgs).length === 0) return []; + const converted = convertParametersForSchema(inputSchema, stringArgs); + // `Object.keys` of the *supplied* names, so an argument the schema does not + // declare (which the conversion passes through) cannot be reported. + return Object.keys(stringArgs).filter( + (key) => converted[key] !== stringArgs[key], + ); +} + +/** + * The refusal {@link coercedArgumentNames} justifies, as one sentence. + * + * Shared for the same reason the check is: the Tools/Apps raw-JSON editor and + * the Edit-and-replay modal refuse the same drafts, so they must not word it + * two different ways. `toolName` is included when the caller knows which tool + * the draft targets — the replay modal does, because its `name` is editable + * and the schema is looked up from it; a form is already rendering one tool + * and would only be repeating itself. + */ +export function coercedArgumentsError( + names: string[], + toolName?: string, +): string { + const quoted = names.map((name) => `\`${name}\``).join(", "); + const whose = toolName + ? `the type ${toolName}'s schema declares` + : "the type the schema declares"; + return `${quoted} would be converted to ${whose} — write the value with that type instead`; +} + /** * Convert prompt arguments (JsonValue) to strings for prompt API */ From 61f58f9ae0ff3cc778dcf9dbe87ddf68d740c318 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 23:32:32 -0400 Subject: [PATCH 3/3] test(web): pin that a refused raw-JSON draft dispatches nothing (#2171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The panel tests stopped at the disabled button, which leaves the wire claim unverified: a screen that flagged the draft and still fired its callback would have satisfied every one of them. `ToolsScreen` and `AppsScreen` now assert `onCallTool` / `onOpenApp` are not called for a refused draft, and ARE called once the value is rewritten with the declared type — so the draft is demonstrably what blocks the dispatch rather than an inert screen. The plain and "Run as task" paths need no separate case: the panel has one Execute button whose disabled state does not depend on the toggle, and the callTool/callToolStream split happens later in App.tsx, downstream of the callback a refused draft never reaches. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- .../screens/AppsScreen/AppsScreen.test.tsx | 30 +++++++++++++++ .../screens/ToolsScreen/ToolsScreen.test.tsx | 38 +++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/clients/web/src/components/screens/AppsScreen/AppsScreen.test.tsx b/clients/web/src/components/screens/AppsScreen/AppsScreen.test.tsx index 1525d508..78502392 100644 --- a/clients/web/src/components/screens/AppsScreen/AppsScreen.test.tsx +++ b/clients/web/src/components/screens/AppsScreen/AppsScreen.test.tsx @@ -10,6 +10,7 @@ import { screen, within, } from "../../../test/renderWithMantine"; +import { setAceTextByLabel } from "../../../test/aceEditor"; import { AppsScreen, type AppsScreenProps, @@ -320,6 +321,35 @@ describe("AppsScreen", () => { expect(screen.getByTitle("Weather Widget")).toBeInTheDocument(); }); + // The Apps counterpart of the same wire claim (#2171): an App's arguments are + // a `tools/call` like any other, so a refused draft must not open the app. + it("dispatches nothing for a draft the schema would retype", async () => { + const user = userEvent.setup(); + const onOpenApp = vi.fn(); + const numericApp: Tool = { + name: "chart", + title: "Chart Widget", + inputSchema: { + type: "object", + properties: { count: { type: "number" } }, + }, + _meta: { ui: { resourceUri: "ui://apps/chart" } }, + }; + renderWithMantine( + , + ); + await user.click(screen.getByText("Chart Widget")); + await user.click(screen.getByLabelText("Edit as JSON")); + await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}'); + await user.click(screen.getByRole("button", { name: /Open App/ })); + + expect(onOpenApp).not.toHaveBeenCalled(); + + await setAceTextByLabel(/Arguments JSON/, '{"count":1}'); + await user.click(screen.getByRole("button", { name: /Open App/ })); + expect(onOpenApp).toHaveBeenCalledWith("chart", { count: 1 }); + }); + it("seeds schema defaults so untouched fields are sent on Open App", async () => { const user = userEvent.setup(); const onOpenApp = vi.fn(); diff --git a/clients/web/src/components/screens/ToolsScreen/ToolsScreen.test.tsx b/clients/web/src/components/screens/ToolsScreen/ToolsScreen.test.tsx index 225b80fc..608e84c9 100644 --- a/clients/web/src/components/screens/ToolsScreen/ToolsScreen.test.tsx +++ b/clients/web/src/components/screens/ToolsScreen/ToolsScreen.test.tsx @@ -3,6 +3,7 @@ import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import type { Tool } from "@modelcontextprotocol/client"; import { renderWithMantine, screen } from "../../../test/renderWithMantine"; +import { setAceTextByLabel } from "../../../test/aceEditor"; import { noopPagination } from "../../../test/fixtures/pagination"; import { ToolsScreen, @@ -157,6 +158,43 @@ describe("ToolsScreen", () => { ); }); + // #2171's acceptance asks for a test that pins the WIRE rather than the form + // state. Under the refusal answer the wire claim is that nothing is sent, and + // `onCallTool` is where a dispatch would begin — so a screen that flagged the + // draft but still fired the callback would satisfy every other test here. + // + // The plain and "Run as task" paths need no separate case: the split between + // `callTool` and `callToolStream` happens in `App.tsx`, downstream of this + // callback, so a gate that stops the callback stops both. + it("dispatches nothing for a draft the schema would retype", async () => { + const user = userEvent.setup(); + const onCallTool = vi.fn(); + const numeric: Tool[] = [ + { + name: "add", + inputSchema: { + type: "object", + properties: { count: { type: "number" } }, + }, + }, + ]; + renderWithMantine( + , + ); + await user.click(screen.getByText("add")); + await user.click(screen.getByLabelText("Edit as JSON")); + await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}'); + await user.click(screen.getByRole("button", { name: /Execute/ })); + + expect(onCallTool).not.toHaveBeenCalled(); + + // And it is the draft that blocks it, not the screen: rewritten with the + // declared type, the very same click dispatches. + await setAceTextByLabel(/Arguments JSON/, '{"count":1}'); + await user.click(screen.getByRole("button", { name: /Execute/ })); + expect(onCallTool).toHaveBeenCalledWith("add", { count: 1 }, false); + }); + it("filters the sidebar list as the search text changes", async () => { const user = userEvent.setup(); renderWithMantine();