From 03bf9ece426c835907f47525a3ca19bca7763d69 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 05:16:23 -0400 Subject: [PATCH] fix(web): resolve the replay tool from the draft, and say why a draft is stuck MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot's ninth pass, three comments, two of them consequences of round 8. - The coercion check resolved the tool from the *entry's* name while the modal lets `name` be edited, so retargeting an `echo` replay at `add` validated against `echo`. The modal now takes the whole tool list and resolves from the draft's current name, which is the one the dispatch will look up. - `IMPRECISE_INTEGER_ERROR` (was `ROUNDED_INTEGER_ERROR`) no longer names the safe-integer range, because the range is not the rule: a whole number past ±(2^53 − 1) that is exactly representable round-trips and is accepted. It talks about exact representability instead, and its doc links the helper that actually exists. - `SchemaJsonField` carries the parser's reason instead of flattening every case to "Not valid JSON", which misdiagnosed a document that parses fine and holds a number this client cannot send — leaving the user reading about syntax while looking at syntactically fine JSON. The last one had a bug the gate caught rather than the tests: empty text is not a *problem*, so routing the value through the reason check let `JSON.parse("")` throw out of the change handler. It surfaced as five unhandled errors across four suites with every assertion still green, which is why `npm run ci` and not `npm run test` is the gate. Excluded explicitly, with a regression test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- .../EditReplayModal/EditReplayModal.test.tsx | 50 ++++++++++++- .../EditReplayModal/EditReplayModal.tsx | 35 ++++++--- .../groups/ProtocolEntry/ProtocolEntry.tsx | 10 +-- .../groups/SchemaForm/SchemaForm.test.tsx | 51 ++++++++++++- .../groups/SchemaForm/SchemaForm.tsx | 75 +++++++++++++------ clients/web/src/utils/jsonObjectDraft.ts | 14 +++- 6 files changed, 186 insertions(+), 49 deletions(-) diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx index 5fdaa6aa..ff8614ca 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx @@ -2,8 +2,23 @@ import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import { renderWithMantine, screen } from "../../../test/renderWithMantine"; import { getAceText, setAceText } from "../../../test/aceEditor"; +import type { Tool } from "@modelcontextprotocol/client"; import { EditReplayModal } from "./EditReplayModal"; +const TOOLS: Tool[] = [ + { + name: "echo", + inputSchema: { + type: "object", + properties: { message: { type: "string" } }, + }, + }, + { + name: "add", + inputSchema: { type: "object", properties: { count: { type: "number" } } }, + }, +]; + const baseProps = { opened: true, method: "tools/call", @@ -204,7 +219,7 @@ describe("EditReplayModal", () => { await setAceText('{"name":"echo","arguments":{"id":9007199254740993}}'); expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); expect( - screen.getByText(/A whole number written out in full must be within/), + screen.getByText(/cannot be represented exactly/), ).toBeInTheDocument(); }); @@ -217,6 +232,39 @@ describe("EditReplayModal", () => { ).toBeInTheDocument(); }); + // `callTool` coerces string arguments to the type the schema declares, so + // `"2"` against a numeric field is sent as `2`. + it("disables Send for a tool argument the schema would coerce", async () => { + renderWithMantine( + , + ); + await setAceText('{"name":"add","arguments":{"count":"2"}}'); + expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); + expect(screen.getByText(/`count` would be converted/)).toBeInTheDocument(); + }); + + // `name` is editable, so the schema to validate against is whichever tool the + // draft *now* names — not the one the entry happened to call. + it("re-validates against the tool the draft retargets to", async () => { + renderWithMantine( + , + ); + expect(screen.getByRole("button", { name: "Send" })).not.toBeDisabled(); + + // Retargeted at `add`, whose `count` is a number — the string would coerce. + await setAceText('{"name":"add","arguments":{"count":"2"}}'); + expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); + + // And back: `echo`'s `message` is a string, so nothing is converted. + await setAceText('{"name":"echo","arguments":{"message":"2"}}'); + expect(screen.getByRole("button", { name: "Send" })).not.toBeDisabled(); + }); + it("closes without sending when cancelled", async () => { const user = userEvent.setup(); const onSend = vi.fn(); diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx index 480f87cc..17bee0f1 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx @@ -5,7 +5,7 @@ import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; import { hasImpreciseIntegerLiteral, isJsonObject, - ROUNDED_INTEGER_ERROR, + IMPRECISE_INTEGER_ERROR, } from "../../../utils/jsonObjectDraft"; import { replayableParams, @@ -34,11 +34,15 @@ export interface EditReplayModalProps { */ droppedParamKeys?: string[]; /** - * The tool a `tools/call` replay would invoke, when it is known. Only used to - * detect an argument the client would coerce to the type the schema declares - * — see {@link reshapedReplayParam}. + * The connected server's tools, used only to detect an argument the client + * would coerce to the type the schema declares — see + * {@link reshapedReplayParam}. + * + * The whole list rather than the one the entry called, because `name` is + * itself editable: retargeting an `echo` replay at `add` has to be validated + * against `add`'s schema, not the one the entry happened to use. */ - tool?: Tool; + tools?: Tool[]; /** * Re-issue the request with these params. `null` means "send no params", * which is a request an empty editor can legitimately express — see @@ -82,7 +86,7 @@ function formatDroppedKeys(keys: string[]): string { function parseParamsDraft( method: string, text: string, - tool: Tool | undefined, + tools: Tool[] | undefined, ): | { ok: true; value: Record | null } | { ok: false; error: string } { @@ -108,7 +112,7 @@ function parseParamsDraft( }; } if (hasImpreciseIntegerLiteral(text)) { - return { ok: false, error: ROUNDED_INTEGER_ERROR }; + return { ok: false, error: IMPRECISE_INTEGER_ERROR }; } // Seeding from the projection is only half of it: a key the dispatcher does // not read can also be *added* to the draft, and Send would then discard an @@ -126,7 +130,14 @@ function parseParamsDraft( } // Surviving the key check is not the same as being sent as written: see // `reshapedReplayParam` for the two ways `arguments` does not. - const reshaped = reshapedReplayParam(method, parsed, tool); + // Resolved from the *draft's* name, so retargeting the call re-validates + // against the tool it would actually reach. + const named = typeof parsed.name === "string" ? parsed.name : undefined; + const reshaped = reshapedReplayParam( + method, + parsed, + tools?.find((candidate) => candidate.name === named), + ); if (reshaped !== null) { return { ok: false, error: reshaped }; } @@ -142,14 +153,14 @@ function EditReplayForm({ method, params, droppedParamKeys, - tool, + tools, onSend, onClose, }: Omit) { const [draft, setDraft] = useState(() => params === undefined ? "" : JSON.stringify(params, null, 2), ); - const parsed = parseParamsDraft(method, draft, tool); + const parsed = parseParamsDraft(method, draft, tools); return ( @@ -208,7 +219,7 @@ export function EditReplayModal({ method, params, droppedParamKeys, - tool, + tools, onSend, onClose, }: EditReplayModalProps) { @@ -218,7 +229,7 @@ export function EditReplayModal({ method={method} params={params} droppedParamKeys={droppedParamKeys} - tool={tool} + tools={tools} onSend={onSend} onClose={onClose} /> diff --git a/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx b/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx index 644a892d..a6abd2e7 100644 --- a/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx +++ b/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx @@ -346,12 +346,6 @@ export function ProtocolEntry({ const requestParams = extractParams(entry); const canReplay = isReplayableProtocolMethod(method); const editableParams = replayableParams(method, requestParams); - // The tool this entry called, when it called one — the schema an edited - // argument would be coerced against. - const replayedTool = - method === "tools/call" && typeof requestParams?.name === "string" - ? tools?.find((candidate) => candidate.name === requestParams.name) - : undefined; const resultType = extractResultType(entry); const subscriptionId = extractSubscriptionId(entry); @@ -503,7 +497,9 @@ export function ProtocolEntry({ // the editor must not offer a field that Send would drop. params={editableParams.params} droppedParamKeys={editableParams.dropped} - tool={replayedTool} + // The whole list, not this entry's tool: `name` is editable, so the + // schema to validate against is whichever the draft now names. + tools={tools} onSend={onReplay} onClose={() => setIsEditingReplay(false)} /> diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 6ec97cd9..77b6dbb9 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -767,6 +767,55 @@ describe("SchemaForm", () => { expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument(); }); + // "Not valid JSON" would misdiagnose a document that parses perfectly well + // and holds a number this client cannot put on the wire — leaving the user + // reading about syntax while looking at syntactically fine JSON. + it("says why the draft cannot be sent, not just that it cannot", async () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + renderWithMantine( + , + ); + + await setAceTextByLabel(/Config/, "[1,"); + expect(screen.getByText(/^Not valid JSON/)).toBeInTheDocument(); + + await setAceTextByLabel(/Config/, "[1e400]"); + expect(screen.getByText(/^Numbers must be finite/)).toBeInTheDocument(); + + await setAceTextByLabel(/Config/, "[9007199254740993]"); + expect( + screen.getByText(/^This whole number cannot be represented exactly/), + ).toBeInTheDocument(); + + await setAceTextByLabel(/Config/, "[1,2]"); + expect(screen.queryByText(/this field will be omitted/)).toBeNull(); + }); + + // An untouched optional field is not a problem, but it is not a value either + // — excluding it before the reason check rather than by it is what keeps + // `JSON.parse("")` from throwing out of the change handler. + it("reports no value and no error for an emptied JSON field", async () => { + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config", default: [1] }, + }, + }; + renderWithMantine( + , + ); + + await setAceTextByLabel(/Config/, ""); + expect(onChange).toHaveBeenLastCalledWith({ config: undefined }); + expect(screen.queryByText(/this field will be omitted/)).toBeNull(); + }); + it("shows no error for an empty optional field", () => { const schema: InspectorFormSchema = { type: "object", @@ -1925,7 +1974,7 @@ describe("SchemaForm raw JSON (#2151)", () => { await setAceTextByLabel(/Arguments JSON/, '{"id":9007199254740993}'); expect(onValidityChange).toHaveBeenLastCalledWith(true); expect( - screen.getByText(/A whole number written out in full must be within/), + screen.getByText(/cannot be represented exactly/), ).toBeInTheDocument(); }); diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 52598697..73cbcccc 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -27,7 +27,7 @@ import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; import { hasImpreciseIntegerLiteral, isJsonObject, - ROUNDED_INTEGER_ERROR, + IMPRECISE_INTEGER_ERROR, } from "../../../utils/jsonObjectDraft"; import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; import { useValueChange } from "../../../hooks/useValueChange"; @@ -284,26 +284,55 @@ function toNumericValue(raw: string | number): number | undefined { * That covers text that is not JSON *yet* — the mid-edit state this field's * whole draft/value split exists for — and text that parses but does not * survive being written back out: `JSON.parse("1e400")` yields `Infinity`, - * which `JSON.stringify` writes as `null`, and a whole number past 2^53−1 is - * rounded to the nearest double. All three report no value, so the field shows - * its error and `onValidityChange` blocks submission, rather than sending a - * number the user never typed. Same guards, same reason, as - * `parseJsonObjectDraft` and the raw-arguments editor above. + * which `JSON.stringify` writes as `null`, and a whole number written out in + * full that cannot be represented exactly is rounded. All three report no + * value, so the field shows its error and `onValidityChange` blocks submission, + * rather than sending a number the user never typed. Same guards, same reason, + * as `parseJsonObjectDraft` and the raw-arguments editor above. + * + * The *reason* is `describeJsonDraftProblem`'s to give — this only answers + * whether there is a value. */ function parseJsonDraft(text: string): unknown { - if (text.trim() === "") { + // Empty text is not a *problem* — an untouched optional field is fine — but + // it is not a value either, so it is excluded before the reason check rather + // than by it. Missing that let `JSON.parse("")` throw. + if (text.trim() === "" || describeJsonDraftProblem(text) !== null) { return undefined; } + // Cannot throw: a reason of `null` means the text already parsed. Re-reading + // it here rather than having the reason function return the value keeps one + // set of rules, which is what stops the two from disagreeing. + return JSON.parse(text); +} + +/** + * Why this draft cannot be sent, phrased for the field, or `null` when it can. + * + * Three different mistakes, and the field says which. Flattening them to "Not + * valid JSON" misdiagnoses a document that parses perfectly well and happens to + * hold a number this client cannot put on the wire — leaving the user reading a + * message about syntax while looking at syntactically fine JSON, with no clue + * what to change. + * + * Each ends with the same consequence, because it is the same one: an + * unsendable draft reports no value, so the argument is omitted. + */ +function describeJsonDraftProblem(text: string): string | null { + if (text.trim() === "") return null; let parsed: unknown; try { parsed = JSON.parse(text); } catch { - return undefined; + return "Not valid JSON — this field will be omitted"; } - if (!isSerializableJson(parsed) || hasImpreciseIntegerLiteral(text)) { - return undefined; + if (!isSerializableJson(parsed)) { + return "Numbers must be finite — this field will be omitted"; } - return parsed; + if (hasImpreciseIntegerLiteral(text)) { + return `${IMPRECISE_INTEGER_ERROR} — this field will be omitted`; + } + return null; } /** Structural equality for the draft/value re-sync, via canonical JSON. */ @@ -435,24 +464,22 @@ function SchemaJsonField({ } }); - // Text that is present but does not parse yields no value, so the field would - // otherwise submit as absent while the user is still looking at what they - // typed. Saying so on the field keeps that from being silent; reporting it - // upward is what keeps it from being submittable. - const hasInvalidDraft = - draft.trim() !== "" && parseJsonDraft(draft) === undefined; - useDraftValidity(fieldName, !hasInvalidDraft, onValidityChange); + // Text that is present but yields no value would otherwise submit as absent + // while the user is still looking at what they typed. Saying so on the field + // keeps that from being silent; reporting it upward is what keeps it from + // being submittable. The *reason* is carried through rather than flattened — + // "not valid JSON" is a misdiagnosis of a document that parses fine and holds + // a number this client cannot send. + const draftProblem = + draft.trim() === "" ? null : describeJsonDraftProblem(draft); + useDraftValidity(fieldName, draftProblem === null, onValidityChange); return ( { @@ -620,7 +647,7 @@ function parseRawArgumentsDraft( // The quieter half of the same defect: a whole number past 2^53−1 parses to // the nearest double, so the draft shows digits the wire will not carry. if (hasImpreciseIntegerLiteral(text)) { - return { ok: false, error: ROUNDED_INTEGER_ERROR }; + return { ok: false, error: IMPRECISE_INTEGER_ERROR }; } return { ok: true, value: parsed }; } diff --git a/clients/web/src/utils/jsonObjectDraft.ts b/clients/web/src/utils/jsonObjectDraft.ts index a8474083..d06ccdc9 100644 --- a/clients/web/src/utils/jsonObjectDraft.ts +++ b/clients/web/src/utils/jsonObjectDraft.ts @@ -95,9 +95,15 @@ function* jsonNumberLiterals(text: string): Generator { } } -/** The message every draft parser uses for {@link hasRoundedInteger}. */ -export const ROUNDED_INTEGER_ERROR = - "A whole number written out in full must be within ±(2^53 − 1) — a longer one loses digits when parsed, so a different value would be sent"; +/** + * The message every draft parser uses for {@link hasImpreciseIntegerLiteral}. + * + * Phrased as "cannot be represented exactly" rather than naming the safe-integer + * range, because the range is not the rule: a whole number past ±(2^53 − 1) that + * *is* exactly representable — 2^54, say — round-trips and is accepted. + */ +export const IMPRECISE_INTEGER_ERROR = + "This whole number cannot be represented exactly — it is rounded when parsed, so a different value would be sent"; /** * Read a JSON-object editor's draft text. @@ -140,7 +146,7 @@ export function parseJsonObjectDraft(text: string): JsonObjectDraft { }; } if (hasImpreciseIntegerLiteral(text)) { - return { ok: false, error: ROUNDED_INTEGER_ERROR }; + return { ok: false, error: IMPRECISE_INTEGER_ERROR }; } return { ok: true, value: parsed }; }