fix(web): resolve the replay tool from the draft, and say why a draft is stuck
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
fcdc65b289
commit
03bf9ece42
6 changed files with 186 additions and 49 deletions
|
|
@ -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(
|
||||
<EditReplayModal {...baseProps} method="tools/call" tools={TOOLS} />,
|
||||
);
|
||||
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(
|
||||
<EditReplayModal
|
||||
{...baseProps}
|
||||
method="tools/call"
|
||||
params={{ name: "echo", arguments: { message: "hi" } }}
|
||||
tools={TOOLS}
|
||||
/>,
|
||||
);
|
||||
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();
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> | 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<EditReplayModalProps, "opened">) {
|
||||
const [draft, setDraft] = useState(() =>
|
||||
params === undefined ? "" : JSON.stringify(params, null, 2),
|
||||
);
|
||||
const parsed = parseParamsDraft(method, draft, tool);
|
||||
const parsed = parseParamsDraft(method, draft, tools);
|
||||
|
||||
return (
|
||||
<Stack gap="sm">
|
||||
|
|
@ -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}
|
||||
/>
|
||||
|
|
|
|||
|
|
@ -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)}
|
||||
/>
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
<SchemaForm schema={schema} values={{}} onChange={vi.fn()} />,
|
||||
);
|
||||
|
||||
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(
|
||||
<SchemaForm schema={schema} values={{}} onChange={onChange} />,
|
||||
);
|
||||
|
||||
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();
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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 (
|
||||
<JsonEditor
|
||||
{...inputProps}
|
||||
ariaLabel={inputProps.label}
|
||||
value={draft}
|
||||
error={
|
||||
hasInvalidDraft
|
||||
? "Not valid JSON — this field will be omitted"
|
||||
: undefined
|
||||
}
|
||||
error={draftProblem ?? undefined}
|
||||
minLines={4}
|
||||
maxLines={16}
|
||||
onChange={(text) => {
|
||||
|
|
@ -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 };
|
||||
}
|
||||
|
|
|
|||
|
|
@ -95,9 +95,15 @@ function* jsonNumberLiterals(text: string): Generator<string> {
|
|||
}
|
||||
}
|
||||
|
||||
/** 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 };
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue