From fcdc65b2893df82f1b07f17ebd16da83bc4043bb Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 04:56:22 -0400 Subject: [PATCH] fix(web): read the literal, not the parsed number; project cursors and tool args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot's eighth pass, three comments, all real. - The precision check now reads the **source text**. Inferring the literal from `String(value)` was wrong in both directions: it accepted every full-form integer at or above 1e21 (`1000000000000000000001` parses to `1e21` and is written back as `1e+21`), and refused `2^54`, which is written out in full and exactly representable. `hasImpreciseIntegerLiteral` scans the draft for numeric tokens — skipping strings, so a digit-string value is untouched — and compares `BigInt(literal)` against `BigInt(Number(literal))`, which is exact. - `""` is now reported as a surviving cursor for `tools/list` only. It builds its params with `cursor !== undefined` and carries the empty string deliberately; the other four list adapters use a truthiness check and drop it, so the editor showed `{"cursor":""}` while `{}` went out. The asymmetry looks like a latent bug in those four, but this function's job is to describe what the dispatch does. - `callTool` runs every string-valued argument through `convertToolParameters`, so `{"count":"2"}` against a numeric schema is sent as `{"count":2}`. `reshapedReplayParam` now takes the tool and detects that by running the same conversion and comparing, rather than restating its rules — the two cannot drift. `tools` is threaded to `ProtocolEntry` for it, optional throughout: an unknown tool skips the check rather than blocking a replay. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- .../EditReplayModal/EditReplayModal.test.tsx | 2 +- .../EditReplayModal/EditReplayModal.tsx | 19 +++- .../MrtrConversation/MrtrConversation.tsx | 9 ++ .../groups/ProtocolEntry/ProtocolEntry.tsx | 16 +++ .../ProtocolListPanel/ProtocolListPanel.tsx | 10 ++ .../groups/SchemaForm/SchemaForm.test.tsx | 2 +- .../groups/SchemaForm/SchemaForm.tsx | 6 +- .../screens/ProtocolScreen/ProtocolScreen.tsx | 9 ++ .../views/InspectorView/InspectorView.tsx | 3 + clients/web/src/lib/protocolReplay.test.ts | 104 +++++++++++++++++ clients/web/src/lib/protocolReplay.ts | 54 ++++++++- clients/web/src/utils/jsonObjectDraft.test.ts | 71 ++++++++---- clients/web/src/utils/jsonObjectDraft.ts | 106 +++++++++++------- 13 files changed, 336 insertions(+), 75 deletions(-) diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx index e43deaa8..5fdaa6aa 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx @@ -204,7 +204,7 @@ describe("EditReplayModal", () => { await setAceText('{"name":"echo","arguments":{"id":9007199254740993}}'); expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); expect( - screen.getByText(/Whole numbers must be within/), + screen.getByText(/A whole number written out in full must be within/), ).toBeInTheDocument(); }); diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx index c70d258f..480f87cc 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx @@ -1,8 +1,9 @@ import { useState } from "react"; +import type { Tool } from "@modelcontextprotocol/client"; import { Alert, Button, Group, Modal, Stack, Text } from "@mantine/core"; import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; import { - hasRoundedInteger, + hasImpreciseIntegerLiteral, isJsonObject, ROUNDED_INTEGER_ERROR, } from "../../../utils/jsonObjectDraft"; @@ -32,6 +33,12 @@ export interface EditReplayModalProps { * the one thing an inspector must not do. */ 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}. + */ + tool?: Tool; /** * Re-issue the request with these params. `null` means "send no params", * which is a request an empty editor can legitimately express — see @@ -75,6 +82,7 @@ function formatDroppedKeys(keys: string[]): string { function parseParamsDraft( method: string, text: string, + tool: Tool | undefined, ): | { ok: true; value: Record | null } | { ok: false; error: string } { @@ -99,7 +107,7 @@ function parseParamsDraft( "Numbers must be finite — a value like `1e400` overflows and would be sent as null", }; } - if (hasRoundedInteger(parsed)) { + if (hasImpreciseIntegerLiteral(text)) { return { ok: false, error: ROUNDED_INTEGER_ERROR }; } // Seeding from the projection is only half of it: a key the dispatcher does @@ -118,7 +126,7 @@ 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); + const reshaped = reshapedReplayParam(method, parsed, tool); if (reshaped !== null) { return { ok: false, error: reshaped }; } @@ -134,13 +142,14 @@ function EditReplayForm({ method, params, droppedParamKeys, + tool, onSend, onClose, }: Omit) { const [draft, setDraft] = useState(() => params === undefined ? "" : JSON.stringify(params, null, 2), ); - const parsed = parseParamsDraft(method, draft); + const parsed = parseParamsDraft(method, draft, tool); return ( @@ -199,6 +208,7 @@ export function EditReplayModal({ method, params, droppedParamKeys, + tool, onSend, onClose, }: EditReplayModalProps) { @@ -208,6 +218,7 @@ export function EditReplayModal({ method={method} params={params} droppedParamKeys={droppedParamKeys} + tool={tool} onSend={onSend} onClose={onClose} /> diff --git a/clients/web/src/components/groups/MrtrConversation/MrtrConversation.tsx b/clients/web/src/components/groups/MrtrConversation/MrtrConversation.tsx index 47e4fb83..100fdcb9 100644 --- a/clients/web/src/components/groups/MrtrConversation/MrtrConversation.tsx +++ b/clients/web/src/components/groups/MrtrConversation/MrtrConversation.tsx @@ -1,4 +1,5 @@ import { useState } from "react"; +import type { Tool } from "@modelcontextprotocol/client"; import { Badge, Collapse, @@ -28,6 +29,12 @@ export interface MrtrConversationProps { /** Compact per-round layout for the narrow monitoring column. */ embedded?: boolean; onReplay: (id: string, overrideParams?: ReplayParamsOverride) => void; + /** + * The connected server's tools. Forwarded to each entry only so an edited + * `tools/call` replay can tell whether an argument would be coerced by the + * schema on the way out (#2151). + */ + tools?: Tool[]; onTogglePin: (id: string) => void; } @@ -123,6 +130,7 @@ function formatRoundLabel(index: number): string { } export function MrtrConversation({ + tools, requestState, rounds, pinnedIds, @@ -187,6 +195,7 @@ export function MrtrConversation({ onReplay(round.id, overrideParams) } onTogglePin={() => onTogglePin(round.id)} + tools={tools} /> ))} diff --git a/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx b/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx index 1b397421..644a892d 100644 --- a/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx +++ b/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx @@ -12,6 +12,7 @@ import { Text, } from "@mantine/core"; import { RiErrorWarningLine } from "react-icons/ri"; +import type { Tool } from "@modelcontextprotocol/client"; import type { MessageEntry } from "@inspector/core/mcp/types.js"; import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; @@ -70,6 +71,13 @@ export interface ProtocolEntryProps { * correlated HTTP record. */ correlatedHttpStatus?: number; + /** + * The connected server's tools, used only to tell whether an edited + * `tools/call` argument would be coerced by the schema on the way out + * (#2151). Optional: without it that one check is skipped, which is the right + * answer when the tool list is not known rather than a reason to block. + */ + tools?: Tool[]; } const EntryContainer = Card.withProps({ @@ -327,6 +335,7 @@ export function ProtocolEntry({ embedded = false, onRevealInNetwork, correlatedHttpStatus, + tools, }: ProtocolEntryProps) { const [isExpanded, setIsExpanded] = useState(isListExpanded); const [isEditingReplay, setIsEditingReplay] = useState(false); @@ -337,6 +346,12 @@ 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); @@ -488,6 +503,7 @@ export function ProtocolEntry({ // the editor must not offer a field that Send would drop. params={editableParams.params} droppedParamKeys={editableParams.dropped} + tool={replayedTool} onSend={onReplay} onClose={() => setIsEditingReplay(false)} /> diff --git a/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.tsx b/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.tsx index 6456da0f..0ebe4969 100644 --- a/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.tsx +++ b/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.tsx @@ -1,4 +1,5 @@ import { useMemo, useState, type ReactNode } from "react"; +import type { Tool } from "@modelcontextprotocol/client"; import { Button, Collapse, @@ -49,6 +50,12 @@ export interface ProtocolListPanelProps { /** Export just one section's entries. */ onExportSection: (section: ProtocolSectionName) => void; onReplay: (id: string, overrideParams?: ReplayParamsOverride) => void; + /** + * The connected server's tools. Forwarded to each entry only so an edited + * `tools/call` replay can tell whether an argument would be coerced by the + * schema on the way out (#2151). + */ + tools?: Tool[]; onTogglePin: (id: string) => void; sortDirection: SortDirection; onSortChange: (next: SortDirection) => void; @@ -236,6 +243,7 @@ function matchesFilters( } export function ProtocolListPanel({ + tools, entries, pinnedIds, searchText, @@ -319,6 +327,7 @@ export function ProtocolListPanel({ embedded={embedded} onReplay={onReplay} onTogglePin={onTogglePin} + tools={tools} /> ) : ( ), ); diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 485beb5a..6ec97cd9 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -1925,7 +1925,7 @@ describe("SchemaForm raw JSON (#2151)", () => { await setAceTextByLabel(/Arguments JSON/, '{"id":9007199254740993}'); expect(onValidityChange).toHaveBeenLastCalledWith(true); expect( - screen.getByText(/Whole numbers must be within/), + screen.getByText(/A whole number written out in full must be within/), ).toBeInTheDocument(); }); diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index e533c5fc..52598697 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -25,7 +25,7 @@ import { ClearButton } from "../../elements/ClearButton/ClearButton"; import { EnlargeButton } from "../../elements/EnlargeButton/EnlargeButton"; import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; import { - hasRoundedInteger, + hasImpreciseIntegerLiteral, isJsonObject, ROUNDED_INTEGER_ERROR, } from "../../../utils/jsonObjectDraft"; @@ -300,7 +300,7 @@ function parseJsonDraft(text: string): unknown { } catch { return undefined; } - if (!isSerializableJson(parsed) || hasRoundedInteger(parsed)) { + if (!isSerializableJson(parsed) || hasImpreciseIntegerLiteral(text)) { return undefined; } return parsed; @@ -619,7 +619,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 (hasRoundedInteger(parsed)) { + if (hasImpreciseIntegerLiteral(text)) { return { ok: false, error: ROUNDED_INTEGER_ERROR }; } return { ok: true, value: parsed }; diff --git a/clients/web/src/components/screens/ProtocolScreen/ProtocolScreen.tsx b/clients/web/src/components/screens/ProtocolScreen/ProtocolScreen.tsx index 4a7a9c66..c25a9f87 100644 --- a/clients/web/src/components/screens/ProtocolScreen/ProtocolScreen.tsx +++ b/clients/web/src/components/screens/ProtocolScreen/ProtocolScreen.tsx @@ -1,4 +1,5 @@ import { useCallback, useMemo } from "react"; +import type { Tool } from "@modelcontextprotocol/client"; import { Card, Flex, Stack } from "@mantine/core"; import type { ReplayParamsOverride } from "../../../lib/protocolReplay"; import type { ProtocolEra } from "@modelcontextprotocol/client"; @@ -24,6 +25,12 @@ export interface ProtocolScreenProps { onClearSection: (section: "pinned" | "history") => void; onExportSection: (section: "pinned" | "history") => void; onReplay: (id: string, overrideParams?: ReplayParamsOverride) => void; + /** + * The connected server's tools. Forwarded to each entry only so an edited + * `tools/call` replay can tell whether an argument would be coerced by the + * schema on the way out (#2151). + */ + tools?: Tool[]; onTogglePin: (id: string) => void; sortDirection: SortDirection; onSortChange: (next: SortDirection) => void; @@ -70,6 +77,7 @@ const SidebarCard = Card.withProps({ }); export function ProtocolScreen({ + tools, entries, pinnedIds, protocolEra, @@ -155,6 +163,7 @@ export function ProtocolScreen({ onClearSection={onClearSection} onExportSection={onExportSection} onReplay={onReplay} + tools={tools} onTogglePin={onTogglePin} sortDirection={sortDirection} onSortChange={onSortChange} diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index 1d679cd7..05336feb 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -1255,6 +1255,9 @@ export function InspectorView({ onExportSection: onExportProtocolSection, onReplay: onReplayProtocol, onTogglePin: onTogglePinProtocol, + // Only so an edited `tools/call` replay can tell whether an argument would + // be coerced by the schema on the way out (#2151). + tools, sortDirection: protocolSort, onSortChange: setProtocolSort, compact: protocolCompact, diff --git a/clients/web/src/lib/protocolReplay.test.ts b/clients/web/src/lib/protocolReplay.test.ts index 6e1d38cb..5e7f48f1 100644 --- a/clients/web/src/lib/protocolReplay.test.ts +++ b/clients/web/src/lib/protocolReplay.test.ts @@ -103,6 +103,45 @@ describe("replayableParams", () => { ).toEqual({ params: { cursor: "abc" }, dropped: ["_meta"] }); }); + // `listTools` builds its params with `cursor !== undefined`, carrying `""` + // deliberately — its own comment says dropping it asks for page one again. + it("keeps an empty cursor on tools/list, which preserves it", () => { + expect(replayableParams("tools/list", { cursor: "" })).toEqual({ + params: { cursor: "" }, + dropped: [], + }); + }); + + // The other four adapters build theirs with a truthiness check, so `""` never + // reaches the wire. Reporting it as kept would show `{"cursor":""}` in the + // editor while `{}` was sent. + it.each([ + "prompts/list", + "resources/list", + "resources/templates/list", + "tasks/list", + ])("drops an empty cursor on %s, which does not preserve it", (method) => { + expect(replayableParams(method, { cursor: "" })).toEqual({ + params: undefined, + dropped: ["cursor"], + }); + }); + + it("keeps a non-empty cursor on every list method", () => { + for (const method of [ + "tools/list", + "prompts/list", + "resources/list", + "resources/templates/list", + "tasks/list", + ]) { + expect(replayableParams(method, { cursor: "abc" })).toEqual({ + params: { cursor: "abc" }, + dropped: [], + }); + } + }); + // The dispatcher ignores a non-string cursor, so keeping it would put a value // in the editor that changes nothing about what is sent. it("drops a cursor that is not a string", () => { @@ -199,6 +238,71 @@ describe("reshapedReplayParam", () => { ).toBeNull(); }); + // `callTool` runs every *string* entry through `convertToolParameters`, + // because the Tools form hands everything over as text — so a string typed + // against a numeric schema is sent as a number. + it("rejects a tool argument the schema would coerce", () => { + const tool: Tool = { + name: "add", + inputSchema: { + type: "object", + properties: { count: { type: "number" } }, + }, + }; + expect( + reshapedReplayParam( + "tools/call", + { name: "add", arguments: { count: "2" } }, + tool, + ), + ).toMatch(/`count` would be converted/); + }); + + it("accepts a tool argument already written with the declared type", () => { + const tool: Tool = { + name: "add", + inputSchema: { + type: "object", + properties: { count: { type: "number" } }, + }, + }; + expect( + reshapedReplayParam( + "tools/call", + { name: "add", arguments: { count: 2 } }, + tool, + ), + ).toBeNull(); + }); + + it("accepts a string argument the schema declares as a string", () => { + const tool: Tool = { + name: "echo", + inputSchema: { + type: "object", + properties: { message: { type: "string" } }, + }, + }; + expect( + reshapedReplayParam( + "tools/call", + { name: "echo", arguments: { message: "hi" } }, + tool, + ), + ).toBeNull(); + }); + + // Nothing can be said about a coercion whose schema is unknown, and refusing + // on that basis would block a replay that is very likely fine. + it("skips the coercion check when the tool is not known", () => { + expect( + reshapedReplayParam("tools/call", { + name: "add", + arguments: { count: "2" }, + }), + ).toBeNull(); + }); + it("has nothing to say about methods that take no arguments", () => { expect(reshapedReplayParam("tools/list", { cursor: "a" })).toBeNull(); expect(reshapedReplayParam("resources/read", { uri: "x://y" })).toBeNull(); diff --git a/clients/web/src/lib/protocolReplay.ts b/clients/web/src/lib/protocolReplay.ts index 768909d2..95c545fd 100644 --- a/clients/web/src/lib/protocolReplay.ts +++ b/clients/web/src/lib/protocolReplay.ts @@ -9,6 +9,7 @@ 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 { isReplayableProtocolMethod } from "../utils/replayableProtocolMethods"; // Derive `LogEntryData[]` from the MessageLog by filtering for the @@ -113,7 +114,19 @@ export function replayableParams( case "tasks/list": // Only a *string* cursor survives: the dispatcher ignores any other type, // so keeping it would put a value in the editor that changes nothing. - kept = typeof params.cursor === "string" ? ["cursor"] : []; + // + // And an **empty** one survives on `tools/list` alone. `listTools` builds + // its params with `cursor !== undefined`, carrying `""` deliberately — + // its own comment explains that dropping it asks for page one again. The + // other four adapters use a truthiness check and drop it. That asymmetry + // looks like a latent bug in those four rather than an intention, but + // this function's job is to describe what the dispatch *does*, so it + // reports the empty cursor as dropped where it would be dropped. + kept = + typeof params.cursor === "string" && + (method === "tools/list" || params.cursor !== "") + ? ["cursor"] + : []; break; default: // `ping` takes nothing, and an unreplayable method never reaches here. @@ -147,13 +160,19 @@ export function replayableParams( * - `arguments: [1,2]` or `arguments: 4` is not nullish, so the cast carries it * through unchanged into a call whose type says it is a named-argument * record. Nothing reshapes it, but nothing can send it either. - * - For **`prompts/get` only**, a non-string *value*. `getPrompt` runs + * - For **`prompts/get`**, a non-string *value*. `getPrompt` runs * `convertPromptArguments`, which `JSON.stringify`s anything that is not * already a string — so `{"count": 2}` is sent as `{"count": "2"}` while the * editor still shows the number. This is not a quirk to route around: the * spec types `GetPromptRequest.params.arguments` as `Record`, - * so a string is the only thing a prompt argument can be. `tools/call` is - * unaffected — `callTool` sends its arguments as given. + * 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 + * 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. * * `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 @@ -162,6 +181,7 @@ export function replayableParams( export function reshapedReplayParam( method: string, params: Record | undefined, + tool?: Tool, ): string | null { if (!params) return null; if (method !== "tools/call" && method !== "prompts/get") return null; @@ -182,9 +202,35 @@ export function reshapedReplayParam( return `A prompt argument is always a string — ${coerced.join(", ")} would be sent as JSON text`; } } + if (method === "tools/call" && tool) { + const coerced = coercedToolArgs(tool, 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 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/utils/jsonObjectDraft.test.ts b/clients/web/src/utils/jsonObjectDraft.test.ts index 5e8be685..5788817d 100644 --- a/clients/web/src/utils/jsonObjectDraft.test.ts +++ b/clients/web/src/utils/jsonObjectDraft.test.ts @@ -1,49 +1,72 @@ import { describe, it, expect } from "vitest"; -import { hasRoundedInteger, parseJsonObjectDraft } from "./jsonObjectDraft"; +import { + hasImpreciseIntegerLiteral, + parseJsonObjectDraft, +} from "./jsonObjectDraft"; // `isSerializableJson` catches a literal too large to represent (`1e400` → // `Infinity` → `null` on the wire). This is the quieter one: a whole number // past 2^53−1 parses to the nearest double, so the draft shows digits the wire // will not carry. -describe("hasRoundedInteger", () => { +describe("hasImpreciseIntegerLiteral", () => { it("accepts whole numbers inside the safe range", () => { - expect(hasRoundedInteger({ id: Number.MAX_SAFE_INTEGER })).toBe(false); - expect(hasRoundedInteger({ id: -Number.MAX_SAFE_INTEGER })).toBe(false); - expect(hasRoundedInteger({ id: 0 })).toBe(false); + expect(hasImpreciseIntegerLiteral('{"id":9007199254740991}')).toBe(false); + expect(hasImpreciseIntegerLiteral('{"id":-9007199254740991}')).toBe(false); + expect(hasImpreciseIntegerLiteral('{"id":0}')).toBe(false); }); - it("rejects a whole number past the safe range", () => { + it("rejects a whole number that loses digits when parsed", () => { // Typed as …93, parsed as …92. - expect(hasRoundedInteger(JSON.parse('{"id":9007199254740993}'))).toBe(true); + expect(hasImpreciseIntegerLiteral('{"id":9007199254740993}')).toBe(true); + }); + + // The case the previous, value-based check could not see: at or above 1e21 + // JS stringifies in exponent form, so inferring the literal from the parsed + // number said "exponent form, nothing lost" about digits that were lost. + it("rejects a full-form literal at or above 1e21", () => { + expect(hasImpreciseIntegerLiteral('{"id":1000000000000000000001}')).toBe( + true, + ); + }); + + // …and the false positive it also had. Written out in full, past the safe + // range, and exactly representable, so it is sent as written. + it("accepts a full-form literal that is exactly representable", () => { + expect(hasImpreciseIntegerLiteral('{"id":18014398509481984}')).toBe(false); }); // A fractional or exponent-form value is a double by nature and is sent as // the double it parsed to, so nothing is lost between typing and sending. - it("accepts a fractional value however large", () => { - expect(hasRoundedInteger({ ratio: 1.5 })).toBe(false); - expect(hasRoundedInteger({ ratio: 0.1 + 0.2 })).toBe(false); + it("accepts fractional and exponent-form values", () => { + expect(hasImpreciseIntegerLiteral('{"n":1.5}')).toBe(false); + expect(hasImpreciseIntegerLiteral('{"n":1e308}')).toBe(false); + expect(hasImpreciseIntegerLiteral('{"n":1e21}')).toBe(false); + expect(hasImpreciseIntegerLiteral('{"n":-2.5e-7}')).toBe(false); }); - // JS stringifies from 1e21 upward in exponent form, so such a value was typed - // that way and the parsed double is what was asked for. `parseJsonObjectDraft` - // has always accepted these. - it("accepts a value whose shortest form carries an exponent", () => { - expect(hasRoundedInteger({ n: 1e308 })).toBe(false); - expect(hasRoundedInteger({ n: 1e21 })).toBe(false); + // The reason this scans rather than running a regex over the text: those + // digits are a *string*, sent back exactly as written. + it("ignores digits inside a string", () => { + expect(hasImpreciseIntegerLiteral('{"id":"9007199254740993"}')).toBe(false); }); - it("looks inside arrays and nested objects", () => { - expect(hasRoundedInteger(JSON.parse('[{"a":[9007199254740993]}]'))).toBe( - true, - ); - expect(hasRoundedInteger({ a: { b: [1, 2, "x", null, true] } })).toBe( + it("ignores digits inside a key, and an escaped quote inside a string", () => { + expect(hasImpreciseIntegerLiteral('{"9007199254740993":1}')).toBe(false); + expect( + hasImpreciseIntegerLiteral('{"a":"say \\"9007199254740993\\"","b":1}'), + ).toBe(false); + }); + + it("looks through arrays and nesting", () => { + expect(hasImpreciseIntegerLiteral('[{"a":[9007199254740993]}]')).toBe(true); + expect(hasImpreciseIntegerLiteral('{"a":{"b":[1,2,"x",null,true]}}')).toBe( false, ); }); - it("has nothing to say about non-numeric values", () => { - expect(hasRoundedInteger("9007199254740993")).toBe(false); - expect(hasRoundedInteger(null)).toBe(false); + it("says nothing about text that is not JSON", () => { + expect(hasImpreciseIntegerLiteral("")).toBe(false); + expect(hasImpreciseIntegerLiteral("{")).toBe(false); }); }); diff --git a/clients/web/src/utils/jsonObjectDraft.ts b/clients/web/src/utils/jsonObjectDraft.ts index d69ac326..a8474083 100644 --- a/clients/web/src/utils/jsonObjectDraft.ts +++ b/clients/web/src/utils/jsonObjectDraft.ts @@ -21,53 +21,83 @@ export function isJsonObject(value: unknown): value is StrictJsonObject { } /** - * Whether `JSON.parse` dropped digits from a whole number written out in full. + * Whether the draft text writes a whole number out in full that `JSON.parse` + * cannot represent exactly. * - * `isSerializableJson` catches a literal too large to *represent* — `1e400` - * parses to `Infinity` and writes back as `null`. This catches the quieter one: - * a whole number past 2^53−1 parses to the nearest double, so - * `{"id":9007199254740993}` becomes `…992` with no error anywhere. The draft - * still shows what was typed, which is what makes it a misreport rather than a - * formatting difference — the editor displays one value and the wire carries - * another. Snowflake-style ids are the realistic case. + * `isSerializableJson` catches a literal too large to represent at all — + * `1e400` parses to `Infinity` and writes back as `null`. This catches the + * quieter one: `{"id":9007199254740993}` parses to `…992`, and + * `{"id":1000000000000000000001}` parses to `1e21` and is written back as + * `1e+21`. Either way the editor shows digits the wire will not carry. + * Snowflake-style ids are the realistic case. * - * Two conditions, and the second is what keeps this from over-reaching: + * It reads the **source text**, which is the only place the answer exists: the + * parsed number has already lost whatever it lost, so nothing about its value + * distinguishes `1000000000000000000001` from `1e21`. An earlier attempt + * inferred it from `String(value)` and was wrong in both directions — it + * refused `2^54` (written in full and exactly representable) and accepted every + * full-form literal at or above 1e21, where JS switches to exponent form. * - * - **An unsafe whole number.** A fractional value is a double by nature and is - * sent as the double it parsed to, so nothing is lost between typing and - * sending. - * - **Whose shortest representation is written out in full.** JS switches to - * exponent form at 1e21, so a value that stringifies as `1e+308` was typed in - * exponent form, where the parsed double *is* the value the user asked for. - * Without this, `{"n":1e308}` — a legitimate `_meta` value — would be - * refused, which `parseJsonObjectDraft`'s own tests pin against. - * - * It still refuses the rare full-form integer past 2^53−1 that happens to be - * exactly representable (2^54, say), because nothing here can tell it apart - * from one that lost digits without the original literal. That errs toward - * refusing a sendable value rather than misreporting an unsendable one, which - * is the same trade `toNumericValue` makes for the schema form's number input. + * Only integer literals written out in full are checked. An exponent or a + * fraction is a double by nature and is sent as the double it parsed to, so + * nothing is lost between typing and sending — and `{"n":1e308}` stays + * acceptable, which `parseJsonObjectDraft`'s own tests pin. */ -export function hasRoundedInteger(value: unknown): boolean { - if (typeof value === "number") { - return ( - Number.isInteger(value) && - !Number.isSafeInteger(value) && - !String(value).includes("e") - ); - } - if (Array.isArray(value)) return value.some(hasRoundedInteger); - if (value !== null && typeof value === "object") { - return Object.values(value as Record).some( - hasRoundedInteger, - ); +export function hasImpreciseIntegerLiteral(text: string): boolean { + for (const literal of jsonNumberLiterals(text)) { + if (!/^-?\d+$/.test(literal)) continue; + // BigInt on both sides, so the comparison is exact: `Number(literal)` is an + // integer-valued double, and converting it back shows what survived. + if (BigInt(literal) !== BigInt(Number(literal))) return true; } return false; } +/** Matches one JSON number token, anchored where the scan currently stands. */ +const JSON_NUMBER = /-?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?/y; + +/** + * The numeric literals of a JSON document, in source order, skipping anything + * inside a string. + * + * The string-skipping is the whole reason this is a scanner rather than a + * regex over the text: `{"id":"9007199254740993"}` carries those digits as a + * *string*, which is sent back exactly as written and must not be refused. + * Object keys are strings too, so they are skipped by the same rule. + */ +function* jsonNumberLiterals(text: string): Generator { + let index = 0; + let inString = false; + while (index < text.length) { + const char = text[index]; + if (inString) { + // A backslash escapes the next character, including a closing quote. + index += char === "\\" ? 2 : 1; + if (char === '"') inString = false; + continue; + } + if (char === '"') { + inString = true; + index += 1; + continue; + } + if (char === "-" || (char >= "0" && char <= "9")) { + JSON_NUMBER.lastIndex = index; + const match = JSON_NUMBER.exec(text); + /* v8 ignore next -- the sticky pattern always matches from a digit or a + `-`, which is the only way this branch is entered. */ + if (!match) break; + yield match[0]; + index = JSON_NUMBER.lastIndex; + continue; + } + index += 1; + } +} + /** The message every draft parser uses for {@link hasRoundedInteger}. */ export const ROUNDED_INTEGER_ERROR = - "Whole numbers must be within ±(2^53 − 1) — a longer one is rounded when parsed, so a different value would be sent"; + "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"; /** * Read a JSON-object editor's draft text. @@ -109,7 +139,7 @@ export function parseJsonObjectDraft(text: string): JsonObjectDraft { "Numbers must be finite — a value like `1e400` overflows and would be sent as null", }; } - if (hasRoundedInteger(parsed)) { + if (hasImpreciseIntegerLiteral(text)) { return { ok: false, error: ROUNDED_INTEGER_ERROR }; } return { ok: true, value: parsed };