diff --git a/clients/web/README.md b/clients/web/README.md index 1ea62b2399..a72ed8df84 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 687d2a8157..31537192ae 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 8d75f43123..f2d27a061d 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 4ea9fb953f..60baca605d 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 298ce9fcc0..18a903c99a 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 94ecc8933c..7a496a9aa7 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/components/screens/AppsScreen/AppsScreen.test.tsx b/clients/web/src/components/screens/AppsScreen/AppsScreen.test.tsx index 1525d508ad..78502392b9 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 225b80fc18..608e84c970 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(); diff --git a/clients/web/src/lib/protocolReplay.ts b/clients/web/src/lib/protocolReplay.ts index 95c545fdcc..063feaff04 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 29f60d560e..12f4877b97 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 205db2ab2a..527f626e68 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 */