From 616dd9c196801175ebd4ed2f6b5ce728a8e752a2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 18:27:28 -0400 Subject: [PATCH] fix: exclude `undefined` from `_meta` values, and cover the JSON worker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `JsonValue` admits `undefined`, so `RequestMetadata` accepted values that do not survive serialization: `JSON.stringify` drops an `undefined` object member and rewrites an `undefined` array element to `null`. The type contradicted the one promise this payload makes. Added `StrictJsonValue` / `StrictJsonObject` and used them for the metadata path. Deliberately not a change to the repo-wide `JsonValue`: for tool *arguments* `undefined` legitimately means "not supplied". The narrowing is sound wherever it lands here, since every one of these values comes from `JSON.parse`, which cannot produce `undefined`. The CLI's `parseKeyValuePair` needed the same tightening — caught by `clients/cli`'s typecheck, which the web `tsc -b` does not cover. - Nothing asserted the JSON worker's annotations, and the wrapper's own "Not valid JSON" message comes from our parser rather than from Ace — so a broken worker URL would have left every test passing while the gutter diagnostics silently disappeared. Added a Storybook play function that waits for a real `session.getAnnotations()` error entry, and verified it fails when the worker module URL is pointed at a bad path. - `aria-describedby` referenced only the error, so a supplied `description` was never announced and would have been dropped entirely once the draft went invalid. Both ids are referenced now, with the description surviving the error. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- clients/cli/src/cli.ts | 15 ++++--- .../JsonObjectInput.stories.tsx | 41 +++++++++++++++-- .../JsonObjectInput/JsonObjectInput.test.tsx | 44 ++++++++++++++++--- .../JsonObjectInput/JsonObjectInput.tsx | 29 +++++++++--- clients/web/src/utils/jsonObjectDraft.ts | 14 ++++-- core/json/jsonUtils.ts | 23 ++++++++++ core/mcp/types.ts | 15 +++++-- 7 files changed, 151 insertions(+), 30 deletions(-) diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 35ca3dd9..6f9c8550 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -24,6 +24,7 @@ import { parseHeaderPair, } from "@inspector/core/mcp/node/index.js"; import type { JsonValue } from "@inspector/core/mcp/index.js"; +import type { StrictJsonValue } from "@inspector/core/json/jsonUtils.js"; import { canonicalUrlHost, isAllInterfacesHost, @@ -473,8 +474,8 @@ function buildHandoff( function parseKeyValuePair( value: string, - previous: Record = {}, -): Record { + previous: Record = {}, +): Record { const parts = value.split("="); const key = parts[0]; const val = parts.slice(1).join("="); @@ -485,9 +486,11 @@ function parseKeyValuePair( ); } - let parsedValue: JsonValue; + // `StrictJsonValue`: `JSON.parse` cannot produce `undefined`, and these values + // become `_meta`, which must reach the wire exactly as written (#1910). + let parsedValue: StrictJsonValue; try { - parsedValue = JSON.parse(val) as JsonValue; + parsedValue = JSON.parse(val) as StrictJsonValue; } catch { parsedValue = val; } @@ -735,8 +738,8 @@ async function parseArgs(argv?: string[]): Promise { promptName?: string; promptArgs?: Record; logLevel?: LoggingLevel; - metadata?: Record; - toolMetadata?: Record; + metadata?: Record; + toolMetadata?: Record; cwd?: string; transport?: "sse" | "http" | "stdio"; serverUrl?: string; diff --git a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.stories.tsx b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.stories.tsx index cdbcfc0f..82a6bcc3 100644 --- a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.stories.tsx +++ b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.stories.tsx @@ -1,7 +1,7 @@ import { useState } from "react"; import type { Meta, StoryObj } from "@storybook/react-vite"; -import { expect, userEvent, within } from "storybook/test"; -import type { JsonObject } from "@inspector/core/json/jsonUtils.js"; +import { expect, userEvent, waitFor, within } from "storybook/test"; +import type { StrictJsonObject } from "@inspector/core/json/jsonUtils.js"; import { JsonObjectInput } from "./JsonObjectInput"; const LABEL = "Request metadata JSON"; @@ -15,8 +15,8 @@ const LABEL = "Request metadata JSON"; * place Ace's keyboard behavior can be tested: the unit suite runs in * happy-dom, where Ace's hidden-textarea input path does not work at all. */ -function ControlledJsonObjectInput({ initial }: { initial: JsonObject }) { - const [value, setValue] = useState(initial); +function ControlledJsonObjectInput({ initial }: { initial: StrictJsonObject }) { + const [value, setValue] = useState(initial); return ( { + await focusAndClear(canvasElement); + // A trailing comma on line 2: valid-looking until the closing brace, so the + // worker has to locate it rather than just reject the document. + await userEvent.keyboard('{{"a": 1,{Enter}}'); + await waitFor( + async () => { + const annotations = aceEditor(canvasElement).session.getAnnotations(); + await expect(annotations.length).toBeGreaterThan(0); + await expect(annotations.some((a) => a.type === "error")).toBe(true); + }, + // The worker is asynchronous and debounced, so this needs a real wait + // rather than a tick. + { timeout: 5000 }, + ); + }, +}; diff --git a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx index 85643475..eaf4bb1b 100644 --- a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx +++ b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.test.tsx @@ -1,7 +1,7 @@ import { useState } from "react"; import { describe, it, expect, vi } from "vitest"; import { act, screen } from "@testing-library/react"; -import type { JsonObject } from "@inspector/core/json/jsonUtils.js"; +import type { StrictJsonObject } from "@inspector/core/json/jsonUtils.js"; import { renderWithMantine } from "../../../test/renderWithMantine"; import { aceLabel, getAceText, setAceText } from "../../../test/aceEditor"; import { JsonObjectInput } from "./JsonObjectInput"; @@ -13,10 +13,10 @@ function Harness({ initial, onChange, }: { - initial: JsonObject; - onChange?: (v: JsonObject) => void; + initial: StrictJsonObject; + onChange?: (v: StrictJsonObject) => void; }) { - const [value, setValue] = useState(initial); + const [value, setValue] = useState(initial); return ( <> { // An external reset (switching servers, a reloaded catalog) must reach the // editor; an in-progress edit must not be overwritten by what it emitted. function Externally() { - const [value, setValue] = useState({ a: 1 }); + const [value, setValue] = useState({ a: 1 }); return ( <> { // Regression: `echoed` used to hold only what this component last emitted, // so returning to a previously-shown value matched the stale entry and was // dismissed as our own echo, stranding the editor on B. - function Externally({ value }: { value: JsonObject }) { + function Externally({ value }: { value: StrictJsonObject }) { return ( {}} /> ); @@ -208,6 +208,38 @@ describe("JsonObjectInput", () => { ); }); + it("announces a supplied description, and keeps it when the draft breaks", async () => { + function WithDescription() { + const [value, setValue] = useState({}); + return ( + + ); + } + renderWithMantine(); + + const described = () => + (textarea().getAttribute("aria-describedby") ?? "") + .split(" ") + .filter(Boolean) + .map((id) => document.getElementById(id)?.textContent); + + // Valid draft: described by the description alone. + expect(described()).toEqual(["Sent with every request"]); + + // Invalid draft: the error joins it rather than replacing it — the + // description is still true, and dropping it would silently un-describe + // the control at the moment it needs explaining most. + await setAceText("[1]"); + expect(described()).toHaveLength(2); + expect(described()[0]).toBe("Sent with every request"); + expect(described()[1]).toMatch(/Must be a JSON object/); + }); + it("clears the invalid marking once the text parses again", async () => { renderWithMantine(); await setAceText("[1]"); diff --git a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx index a65b850d..9c5842f9 100644 --- a/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx +++ b/clients/web/src/components/elements/JsonObjectInput/JsonObjectInput.tsx @@ -10,7 +10,7 @@ import "ace-builds/src-noconflict/theme-github_dark"; // Without it Ace fetches `worker-json.js` from a path that does not exist in a // bundled app, silently loses its annotations, and logs a fetch error. import jsonWorkerUrl from "ace-builds/src-noconflict/worker-json.js?url"; -import type { JsonObject } from "@inspector/core/json/jsonUtils.js"; +import type { StrictJsonObject } from "@inspector/core/json/jsonUtils.js"; import { useValueChange } from "../../../hooks/useValueChange"; import { parseJsonObjectDraft } from "../../../utils/jsonObjectDraft"; @@ -19,13 +19,13 @@ import { parseJsonObjectDraft } from "../../../utils/jsonObjectDraft"; ace.config.setModuleUrl("ace/mode/json_worker", jsonWorkerUrl); /** Two-space canonical form — the same shape the catalog file is written in. */ -function serialize(value: JsonObject): string { +function serialize(value: StrictJsonObject): string { return JSON.stringify(value, null, 2); } export interface JsonObjectInputProps { - value: JsonObject; - onChange: (value: JsonObject) => void; + value: StrictJsonObject; + onChange: (value: StrictJsonObject) => void; label?: string; description?: string; /** @@ -167,14 +167,29 @@ export function JsonObjectInput({ synchronously on mount, so this effect always sees an element. */ if (!element) return; element.id = wrapperId; + + // Both the description and the error are rendered by the wrapper and both + // describe this control, so both ids belong here — a supplied description + // is otherwise never announced, and it must not be dropped just because the + // draft went invalid. Mantine renders each only when its prop is set, so + // referencing an absent one would point at nothing. + const describedBy = [ + description === undefined ? null : `${wrapperId}-description`, + parsed.ok ? null : `${wrapperId}-error`, + ].filter((id) => id !== null); + + if (describedBy.length > 0) { + element.setAttribute("aria-describedby", describedBy.join(" ")); + } else { + element.removeAttribute("aria-describedby"); + } + if (parsed.ok) { element.removeAttribute("aria-invalid"); - element.removeAttribute("aria-describedby"); } else { element.setAttribute("aria-invalid", "true"); - element.setAttribute("aria-describedby", `${wrapperId}-error`); } - }, [wrapperId, parsed.ok]); + }, [wrapperId, parsed.ok, description]); return ( ; +export type RequestMetadata = Record; /** * Represents a complete resource read invocation, including request parameters,